Page MenuHomePhabricator

Save message instantly to redux for both foregrounded and backgrounded app
ClosedPublic

Authored by marcin on Mar 27 2023, 6:54 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 7, 6:01 AM
Unknown Object (File)
Thu, Mar 7, 6:01 AM
Unknown Object (File)
Thu, Mar 7, 6:01 AM
Unknown Object (File)
Thu, Mar 7, 6:01 AM
Unknown Object (File)
Thu, Mar 7, 6:01 AM
Unknown Object (File)
Feb 27 2024, 10:15 AM
Unknown Object (File)
Feb 27 2024, 10:15 AM
Unknown Object (File)
Feb 27 2024, 10:15 AM
Subscribers

Details

Summary

This changes our notification handling so that event about received notifications is sent to JS when the app is backgrounded as well.

Test Plan
  1. Build the app prior to applying this diff.
  2. Background it, send a notif.
  3. Kill keyserver.
  4. Open the app.

Message is not visible in the thread.

  1. Repeat the steps above after applying this diff.

Message is visible in the thread even though keyserver is down.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This was easier than I realized!! Thanks for renaming everything. Will leave the diff on others' queues to get some other perspectives

native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
115–117 ↗(On Diff #24190)

What happens if the Android app is killed, then receives a notif and starts again. Will the JS thread be started? Will this Intent be triggered even if the JS thread is not running? I think ideally, we would avoid starting the JS thread in that scenario. We only need to inform the JS thread about the message if the JS thread is already running; otherwise, when it starts it will pull the latest data from SQLite

native/push/push-handler.react.js
533 ↗(On Diff #24190)

That's interesting – so currently, this would get called if the app receives a message while in the foreground? The name of onPushNotifBootsApp implies it should not get called if the app has been in the foreground recently, but I guess it works today...

This looks good to me, but full disclosure I have limited familiarity with this side of the codebase... so please keep that in mind before landing (aka maybe consider having someone else take a final look)

This revision is now accepted and ready to land.Mar 28 2023, 1:35 PM
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
115–117 ↗(On Diff #24190)

This intent doesn't have capability of starting JS thread. When JS thread starts it registers (in CommAndroidNotificationsEventEmitter) relevant broadcast receiver that can receive this intent. However if there are not broadcast receivers registered this line is a no op. For sure there must be a way to check if JS thread is already running in native Java, but my intuition is that it is not worth the effort considering high priority of this issue.

native/push/push-handler.react.js
533 ↗(On Diff #24190)

Method onPushNotifBootsApp sets detectUnsupervisedBackground field of RootContext to false. The last differential that altered this field was: https://phab.comm.dev/D928 (which isn't particularly insightful). If we aren't 100% sure whether it is ok to call onPushNotifBootsApp in case when app wasn't actually booted by notification we can easily introduce additional event type that would just trigger JS to call saveMessageInfos and nothing else.

native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
115–117 ↗(On Diff #24190)

Thanks @marcin!

native/push/push-handler.react.js
533 ↗(On Diff #24190)

Probably not worth it