Page MenuHomePhabricator

[web] Handle clicking on notifications
ClosedPublic

Authored by michal on Feb 22 2023, 3:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Feb 21, 7:02 PM
Unknown Object (File)
Wed, Feb 21, 1:55 PM
Unknown Object (File)
Wed, Feb 21, 1:55 PM
Unknown Object (File)
Wed, Feb 21, 1:55 PM
Unknown Object (File)
Wed, Feb 21, 12:57 PM
Unknown Object (File)
Wed, Feb 21, 12:57 PM
Unknown Object (File)
Sun, Feb 18, 1:51 AM
Unknown Object (File)
Sun, Feb 18, 1:48 AM
Subscribers

Details

Summary

When user clicks a notification we want to:

  • if a comm tab is already focused navigate to a correct thread
  • if it's not focused we want to focus it
  • if all tabs are closed we want to open a new tab
Test Plan

Check if tab navigates to a thread, focuses if needed. Check that a tab is created if needen.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

web/push-notif/push-notifs-handler.js
116–117 ↗(On Diff #22927)

Is the ? after serviceWorker necessary here? It seems like you check that on line 96

web/push-notif/push-notifs-handler.js
117 ↗(On Diff #22927)

Shouldn't there be a return?

web/push-notif/push-notifs-handler.js
117 ↗(On Diff #22927)

return; is implied at the end of a function

web/push-notif/push-notifs-handler.js
117 ↗(On Diff #22927)

I meant that we have to return the cleanup lambda from the useEffect

return () => navigator.serviceWorker?.removeEventListener('message', callback);

is that not right?

@inka was right, I forgot a return for the cleanup callback. Also removed one ? but the second one is still needed because there's a function call after condition so flow can't be sure if the variable wasn't modified.

This revision is now accepted and ready to land.Feb 27 2023, 12:06 PM