Page MenuHomePhabricator
Differential D6229

Remove threadID to notifID mapping from redux and all related usages
ClosedPublic
Actions

Authored by marcin on Jan 11 2023, 5:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 14, 3:51 AM
Unknown Object (File)
Sat, Dec 14, 3:50 AM
Unknown Object (File)
Sat, Dec 14, 3:50 AM
Unknown Object (File)
Sat, Dec 14, 3:50 AM
Unknown Object (File)
Sat, Dec 14, 3:50 AM
Unknown Object (File)
Sat, Dec 14, 3:50 AM
Unknown Object (File)
Sat, Dec 14, 3:50 AM
Unknown Object (File)
Sat, Dec 14, 3:50 AM
View All Files
Subscribers
ashoat

Details

Reviewers
tomek
atul
ashoat
Commits
rCOMMfff47cdc03bc: Remove threadID to notifID mapping from redux and all related usages
Summary

This differential removes threadID to NotifID mapping from redux on native.

Test Plan

Test plan for previous differential + ensure flow does not complain.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

marcin created this revision.Jan 11 2023, 5:24 AM
Herald added a subscriber: ashoat. · View Herald TranscriptJan 11 2023, 5:24 AM
marcin added a parent revision: D6228: Use native modules to clear notifications for a given thread.Jan 11 2023, 5:25 AM
marcin updated this revision to Diff 20788.Jan 11 2023, 5:31 AM

rebase due to changes in previous commit

Harbormaster completed remote builds in B15246: Diff 20786.Jan 11 2023, 5:39 AM
Harbormaster completed remote builds in B15248: Diff 20788.Jan 11 2023, 5:45 AM
marcin requested review of this revision.Jan 11 2023, 5:45 AM
marcin added a child revision: D6231: Display background notification from Java, remove headless JS task.Jan 11 2023, 6:30 AM
ashoat added a comment.Jan 11 2023, 8:50 AM

Woohoo!! Love seeing this

Do we also need to introduce a redux-persist migration to clear this data from the Redux store, eg. like this?

marcin added a comment.Jan 11 2023, 9:09 AM
In D6229#186704, @ashoat wrote:

Woohoo!! Love seeing this

Do we also need to introduce a redux-persist migration to clear this data from the Redux store, eg. like this?

I think yes - I forgot to do so. That is why I was seeing message that threadIDsToNotifIDs were not rehydrated.

marcin updated this revision to Diff 20920.Jan 13 2023, 1:36 AM

Rebase

Harbormaster completed remote builds in B15340: Diff 20920.Jan 13 2023, 2:00 AM
ashoat requested changes to this revision.Jan 13 2023, 6:28 AM

Requesting changes for redux-persist migration

This revision now requires changes to proceed.Jan 13 2023, 6:28 AM
marcin updated this revision to Diff 20976.Jan 16 2023, 6:23 AM

Add redux migration to remove threadIDsToNotifIDs.

Harbormaster completed remote builds in B15384: Diff 20976.Jan 16 2023, 6:38 AM
marcin updated this revision to Diff 20978.Jan 16 2023, 6:50 AM

Rebase

Harbormaster completed remote builds in B15386: Diff 20978.Jan 16 2023, 7:04 AM
ashoat accepted this revision.Jan 16 2023, 7:29 PM
ashoat added inline comments.
native/redux/persist.js
553 ↗(On Diff #20978)

We will need to remember to bump this when we put out a new release. @marcin, can you take the lead on making sure we do this? cc @atul

This revision is now accepted and ready to land.Jan 16 2023, 7:29 PM
marcin added inline comments.Jan 17 2023, 6:51 AM
native/redux/persist.js
553 ↗(On Diff #20978)

Sure - I will contact @atul to make sure he shows me commits with release changes.

marcin mentioned this in D6274: Send broadcast from CommNotificationsHandler when foreground notification is received. Forward parsed notification to JavaScript.Jan 17 2023, 7:01 AM
tomek accepted this revision.Jan 17 2023, 8:27 AM
marcin updated this revision to Diff 21446.Jan 27 2023, 6:34 AM

Rebase before landing

marcin added inline comments.Jan 27 2023, 6:35 AM
native/redux/persist.js
553 ↗(On Diff #20978)

Just a quick clarifications - should we bump it before we land this? or only after we make a release? If the former I would use an explanation why not when landing.

Harbormaster completed remote builds in B15725: Diff 21446.Jan 27 2023, 6:48 AM
ashoat added inline comments.Jan 27 2023, 7:17 AM
native/redux/persist.js
553 ↗(On Diff #20978)

Up to you. If you do not bump this then the migration will not run. When you bump this, the migration will run. Hoping you can use that information to make an informed decision yourself

marcin updated this revision to Diff 21463.EditedJan 27 2023, 7:26 AM

Bump persist version

Harbormaster completed remote builds in B15742: Diff 21463.Jan 27 2023, 7:41 AM
marcin updated this revision to Diff 21562.Jan 30 2023, 3:07 AM

Rebase before landing

Harbormaster completed remote builds in B15816: Diff 21562.Jan 30 2023, 3:21 AM
Closed by commit rCOMMfff47cdc03bc: Remove threadID to notifID mapping from redux and all related usages (authored by marcin). · Explain WhyJan 30 2023, 4:12 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMfff47cdc03bc: Remove threadID to notifID mapping from redux and all related usages.

Revision Contents
Changeset List

PathSize
native/
push/
19 lines
redux/
10 lines
1 line
18 lines
1 line

Diff 20920

View Options

native/push/android.js

Loading...
View Options

native/push/reducer.js

Loading...
View Options

native/redux/action-types.js

Loading...
View Options

native/redux/persist.js

Loading...
View Options

native/redux/redux-setup.js

Loading...
View Options

native/redux/state-types.js

Loading...