Page MenuHomePhabricator

[web][desktop] refresh device token after receiving message from Tunnelbroker
ClosedPublic

Authored by kamil on Jul 29 2024, 4:07 AM.
Tags
None
Referenced Files
F3347737: D12915.diff
Fri, Nov 22, 1:08 PM
Unknown Object (File)
Sat, Nov 9, 11:36 PM
Unknown Object (File)
Fri, Nov 8, 12:14 AM
Unknown Object (File)
Wed, Nov 6, 11:03 PM
Unknown Object (File)
Oct 19 2024, 8:24 AM
Unknown Object (File)
Oct 18 2024, 9:46 PM
Unknown Object (File)
Oct 18 2024, 9:46 PM
Unknown Object (File)
Oct 18 2024, 9:46 PM
Subscribers

Details

Summary

ENG-8498.

This code is refreshing the device token, this is a side effect of the BadDeviceToken message from Tunnelbroker.

We can also consider adding here logic to refresh the token when the keyserver is sending BAD_DEVICE_TOKEN (to match what PushHandler is doing) - ENG-8922.

Some additional context in ENG-8400.

This is only about localToken which is set for every user, not touching tunnelbrokerToken so it shouldn't affect devices without CSAT and TB connection.

Depends on D12912

Test Plan

Simulate Tunnelbroker throws an error with an invalid device token, and test that a new token is uploaded (value in DDB changed). Additionally, I verified this using logs.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Aug 1 2024, 3:33 AM
kamil retitled this revision from [web] refresh device token after receiving message from Tunnelbroker to [web][desktop] refresh device token after receiving message from Tunnelbroker.
kamil edited the summary of this revision. (Show Details)
tomek requested changes to this revision.Aug 2 2024, 1:40 AM

I'm not sure about the logic here. PushNotificationsHandler contains an effect reacting to prevLocalToken.current && !localToken condition. It also renders useCreateDesktopPushSubscription which contains an effect reacting to the same condition. Can't we have a single effect doing all the work?

web/push-notif/push-notifs-handler.js
130 ↗(On Diff #43015)

Does it result in setting state.tunnelbrokerDeviceToken.localToken Redux state?

This revision now requires changes to proceed.Aug 2 2024, 1:40 AM
kamil requested review of this revision.Aug 5 2024, 2:02 AM

I know this is confusing but this is to split responsibilities between the web and desktop, we have three things here:

  1. PushNotificationsHandler - the component responsible for handling notifs, it's only web-specific, see that logic is gated behind const supported = 'Notification' in window && !electron; flag.
  2. useCreatePushSubscription - web-specific, responsible for communication with the service worker to retrieve the device token.
  3. useCreateDesktopPushSubscription - desktop-specific, handling everything notifs related to the desktop, each function is called on a nullable electron object.

So to split this between web and desktop we'll still have this in two places. The only thing I can imagine here is extract everything from PushNotificationsHandler to a hook - useCreateWebPushSubscription or something similar, and then PushNotificationsHandler will look like this:

function PushNotificationsHandler(): React.Node {
  useCreateDesktopPushSubscription();
  useCreateWebPushSubscription();

  return null;
}

Not sure if this worth it.

web/push-notif/push-notifs-handler.js
130 ↗(On Diff #43015)

Yes, if the device token can be retrieved. this is handled in line 59 in this file

This revision is now accepted and ready to land.Aug 5 2024, 3:19 AM