Page MenuHomePhabricator
Differential D11601

[native/web] introduce migration to update notifPermissionAlertInfo to alertStore in redux
ClosedPublic
Actions

Authored by ginsu on Tue, Apr 9, 12:18 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 21, 1:03 AM
Unknown Object (File)
Wed, Apr 17, 8:30 AM
Unknown Object (File)
Tue, Apr 16, 7:49 AM
Unknown Object (File)
Mon, Apr 15, 1:57 PM
Unknown Object (File)
Mon, Apr 15, 11:03 AM
Unknown Object (File)
Sun, Apr 14, 5:48 PM
Unknown Object (File)
Sat, Apr 13, 4:44 PM
Unknown Object (File)
Sat, Apr 13, 2:25 PM
View All 14 Files
Subscribers
ashoat
kamil
tomek

Details

Reviewers
atul
inka
kamil
Commits
rCOMMdbc17db48d23: [native/web] introduce migration to update notifPermissionAlertInfo to…
Summary

This diff introduces a migration to update notifPermissionAlertInfo to alertStore in redux. After this migration all the alerts regarding rehydration warning no longer appear

Adding @kamil as an extra reviewer to make sure my changes to persistWhitelist make sense

Linear task: https://linear.app/comm/issue/ENG-7624/introduce-new-migration-to-update-notifpermissionalertinfo-to

Depends on D11600

Test Plan

The rehydration alerts no longer appear + confirmed that the notif permission alert works as expected

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu created this revision.Tue, Apr 9, 12:18 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptTue, Apr 9, 12:18 PM
ginsu edited the summary of this revision. (Show Details)Tue, Apr 9, 12:19 PM
ginsu added reviewers: atul, inka.
ginsu added a parent revision: D11600: [lib/native/web] refactor recordNotifPermissionAlertActionType to recordAlertActionType.
ginsu added a reviewer: kamil.
ginsu edited the summary of this revision. (Show Details)
ginsu added inline comments.Tue, Apr 9, 12:24 PM
web/redux/persist.js
51 ↗(On Diff #38956)

@kamil wondering if this change here is okay/wondering if there are other things I need to do to change notifPermissionAlertInfo => alertStore. I don't see anymore rehydration errors on native + everything looks good when I open up the redux dev tools, but just wanted to double check with you and see what you think!

Harbormaster completed remote builds in B28101: Diff 38956.Tue, Apr 9, 12:35 PM
ginsu requested review of this revision.Tue, Apr 9, 12:35 PM
ginsu added a child revision: D11613: [lib/native] introduce connect farcaster alert handler.Tue, Apr 9, 10:37 PM
kamil resigned from this revision.Wed, Apr 10, 5:54 AM

Not familiar with this project so letting others review, persistWhitelist looks good

web/redux/persist.js
51 ↗(On Diff #38956)

this seems fine

atul requested changes to this revision.Wed, Apr 10, 10:41 AM

Might be missing something, but don't we also need to update default states? For example in defaultWebState?

This revision now requires changes to proceed.Wed, Apr 10, 10:41 AM
atul accepted this revision.EditedWed, Apr 10, 10:43 AM

{F1510339}

jk, you already handled this in stack

This revision is now accepted and ready to land.Wed, Apr 10, 10:43 AM
ginsu updated this revision to Diff 39014.Wed, Apr 10, 12:37 PM
ginsu added a subscriber: kamil.

rebase before landing

Harbormaster completed remote builds in B28146: Diff 39014.Wed, Apr 10, 1:20 PM
Closed by commit rCOMMdbc17db48d23: [native/web] introduce migration to update notifPermissionAlertInfo to… (authored by ginsu). · Explain WhyWed, Apr 10, 1:32 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMMdbc17db48d23: [native/web] introduce migration to update notifPermissionAlertInfo to….

Revision Contents
Changeset List

PathSize
native/
redux/
15 lines
web/
redux/
16 lines

Diff 39022

View Options

native/redux/persist.js

Loading...
View Options

web/redux/persist.js

Loading...