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
Branch
arcpatch-D13576
Lint
No Lint Coverage
Unit
No Test Coverage

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 44836

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...