web] refactor recordNotifPermissionAlertActionType to recordAlertActionType
ClosedPublic
Actions

Authored by ginsu on Tue, Apr 9, 10:25 AM.

Details

Reviewers

atul
inka

Commits

rCOMMc1de79d8d1b9: [lib/native/web] refactor recordNotifPermissionAlertActionType to…

Summary

This diff refactors recordNotifPermissionAlertActionType and into a more generic recordAlertActionType. I also updated the payload for this action to include the alert type so that the alert info reducer knows which alert info state in the alert store should be updated

Please note this diff + subsequent diffs won't be landed until the migration diff which will be introduced here shortly

Linear task: https://linear.app/comm/issue/ENG-7625/introduce-recordconnectfarcasteralertactiontype

Depends on D11599

Test Plan

flow + confirmed that the existing notif permission alert mechanism still 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, 10:25 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptTue, Apr 9, 10:25 AM

Harbormaster completed remote builds in B28100: Diff 38955.Tue, Apr 9, 10:44 AM

ginsu requested review of this revision.Tue, Apr 9, 10:44 AM

found a bug

actually jk, I confused myself there is no bug with this diff

ginsu added a child revision: D11601: [native/web] introduce migration to update notifPermissionAlertInfo to alertStore in redux.Tue, Apr 9, 12:19 PM

atul accepted this revision.Wed, Apr 10, 10:39 AM

atul added inline comments.

lib/reducers/alert-reducer.js
15 ↗	(On Diff #38955)	Is existing `state.alertInfos[action.payload.alertType].totalAlerts` expected to always be a `number`? Do we need to handle `null`/`undefined` case? Looks like `number` is required for `RecordAlertActionPayload`, but is it possible for an `RecordAlertActionPayload` to be missing for given `alertType`?
21 ↗	(On Diff #38955)	Feel free to keep newline if intentional

This revision is now accepted and ready to land.Wed, Apr 10, 10:39 AM

ginsu added inline comments.Wed, Apr 10, 12:32 PM

lib/reducers/alert-reducer.js
15 ↗	(On Diff #38955)	Spoke to @atul about this IRL but based on how `RecordAlertActionPayload` is typed, the dev must pass in a valid `alertType` into the payload. If they pass in something not valid flow will throw an error which should address @atul's concerns