Page MenuHomePhabricator
Differential D9429

Send all notifications received by backgrounded app to JS at once
ClosedPublic
Actions

Authored by marcin on Oct 9 2023, 9:19 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 8:56 AM
Unknown Object (File)
Mon, Nov 25, 8:46 AM
Unknown Object (File)
Mon, Nov 25, 8:28 AM
Unknown Object (File)
Mon, Nov 25, 6:46 AM
Unknown Object (File)
Wed, Nov 20, 4:39 PM
Unknown Object (File)
Wed, Nov 20, 4:39 PM
Unknown Object (File)
Wed, Nov 20, 4:39 PM
Unknown Object (File)
Wed, Nov 20, 4:39 PM
View All Files
Subscribers
ashoat
will

Details

Reviewers
tomek
bartek
michal
Commits
rCOMM19e5f0ff3d84: Send all notifications received by backgrounded app to JS at once
Summary

Previously for each notification received by backgrounded app we sent separate JS event so that it could be persisted in redux and SQLite. However there was
no reason not to send one JS event with array of all messages. It can potentially help the issue:
https://linear.app/comm/issue/ENG-5147/messageinfos-from-notifs-take-too-long-to-import-on-ios-start#comment-a2fb0774

Test Plan
  1. Build iOS app.
  2. Background it.
  3. Send multiple messages.
  4. Kill the keyserver.
  5. Bring the app to the foreground again.
  6. Ensure all messages are available in the relevant thread(s) even the keyserver is not running.

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-5147
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Oct 9 2023, 9:19 AM
Herald added subscribers: will, ashoat. · View Herald TranscriptOct 9 2023, 9:19 AM
Harbormaster completed remote builds in B23110: Diff 31812.Oct 9 2023, 9:37 AM
marcin requested review of this revision.Oct 9 2023, 9:37 AM
bartek added a comment.Oct 9 2023, 11:26 PM

Looks good to me but letting others review as well

native/push/push-handler.react.js
548–550 ↗(On Diff #31812)

This should also work

tomek accepted this revision.Oct 10 2023, 4:23 AM
tomek added inline comments.
native/push/ios.js
85 ↗(On Diff #31812)

Is it really optional?

This revision is now accepted and ready to land.Oct 10 2023, 4:23 AM
marcin updated this revision to Diff 31870.Oct 10 2023, 7:23 AM
  1. Make messageInfosArray not optional since it is actually not
  2. Use flatMap
marcin updated this revision to Diff 31872.Oct 10 2023, 7:25 AM

Ensure in Objective-C code that messageInfosArray is not optional.

Harbormaster completed remote builds in B23143: Diff 31870.Oct 10 2023, 7:44 AM
Harbormaster completed remote builds in B23145: Diff 31872.
Closed by commit rCOMM19e5f0ff3d84: Send all notifications received by backgrounded app to JS at once (authored by marcin). · Explain WhyOct 10 2023, 8:36 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMM19e5f0ff3d84: Send all notifications received by backgrounded app to JS at once.

Revision Contents
Changeset List

PathSize
native/
ios/
Comm/
10 lines
push/
4 lines
8 lines

Diff 31870

View Options

native/ios/Comm/AppDelegate.mm

Loading...
View Options

native/push/ios.js

Loading...
View Options

native/push/push-handler.react.js

Loading...