Page MenuHomePhabricator
Differential D13576

[lib] fix `PushHandler` to support DMs
ClosedPublic
Actions

Authored by tomek on Oct 2 2024, 8:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 4:01 AM
Unknown Object (File)
Fri, Nov 22, 4:32 PM
Unknown Object (File)
Fri, Nov 22, 4:32 PM
Unknown Object (File)
Fri, Nov 22, 12:51 PM
Unknown Object (File)
Fri, Nov 22, 7:15 AM
Unknown Object (File)
Mon, Nov 4, 1:39 AM
Unknown Object (File)
Sat, Nov 2, 6:32 PM
Unknown Object (File)
Sat, Nov 2, 6:31 PM
View All 49 Files
Subscribers
None

Details

Reviewers
ashoat
kamil
Commits
rCOMMd304b0bc35f4: [lib] fix `PushHandler` to support DMs
Summary

Solution described in ENG-9465.

Test Plan

Apply this diff:

  1. Comment this line to make sure we're testing actual notifs code & add some logs to make sure code in reducer was executed.
  2. Open physical device
  3. In DM thread send a test message
  4. Notif delivered
  5. Message immediately visible in UI
  6. Thread marked as unread
  7. Logs from reducer are there

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil created this revision.Oct 2 2024, 8:58 AM
kamil held this revision as a draft.
kamil published this revision for review.Oct 2 2024, 8:59 AM
tomek accepted this revision.Oct 2 2024, 9:06 AM
tomek added inline comments.
lib/reducers/thread-reducer.js
338 ↗(On Diff #44829)

When threadInfo.currentUser.unread we should bump the timestamp.

341–342 ↗(On Diff #44829)

Might be a good idea to reference in getThreadUpdatesForNewMessages this place.

This revision is now accepted and ready to land.Oct 2 2024, 9:06 AM
Harbormaster completed remote builds in B32019: Diff 44829.Oct 2 2024, 9:16 AM
ashoat added inline comments.Oct 2 2024, 9:19 AM
lib/reducers/thread-reducer.js
341–342 ↗(On Diff #44829)

I'm confused why we'd want to set the thread to unread even if the timestamp is newer than the message timestamp

What's the benefit of this? Doesn't it introduce a risk of setting something to unread incorrectly if a notif for message X is received after a Tunnelbroker message is received for message X?

ashoat accepted this revision.Oct 2 2024, 10:54 AM

Preemptively accepting so that after @tomek commandeers, it's not in an In Review state

tomek commandeered this revision.Oct 2 2024, 11:31 AM
tomek edited reviewers, added: kamil; removed: tomek.
tomek updated this revision to Diff 44836.Oct 2 2024, 11:32 AM

Check the timestamp before updating the status

ashoat accepted this revision.Oct 2 2024, 11:39 AM
Harbormaster completed remote builds in B32024: Diff 44836.Oct 2 2024, 11:52 AM
tomek added a comment.Oct 2 2024, 11:55 AM

Performed Kamil's test again (except the logs stuff) and the solution works correctly.

Closed by commit rCOMMd304b0bc35f4: [lib] fix `PushHandler` to support DMs (authored by kamil, committed by tomek). · Explain WhyOct 2 2024, 11:57 AM
This revision was automatically updated to reflect the committed changes.
tomek added a commit: rCOMMd304b0bc35f4: [lib] fix `PushHandler` to support DMs.
ashoat mentioned this in D13766: [lib] Add timestamp check when flipping unread status.Oct 22 2024, 6:02 AM

Revision Contents
Changeset List

PathSize
lib/
reducers/
32 lines
types/
2 lines
native/
push/
3 lines

Diff 44837

View Options

lib/reducers/thread-reducer.js

Loading...
View Options

lib/types/message-types.js

Loading...
View Options

native/push/push-handler.react.js

Loading...