feat(notifications): sync filter tabs and push toggles across all types#3363
Conversation
Notification types were exposed inconsistently: the filter bar offered 5
of the 10 enum values, and push toggles covered 9 types while delegations,
payouts, account_update and weekly_earnings had none.
Add the missing filter entries and push toggles so every type that the
notifications feed renders can also be filtered and muted.
notifyTypesConst values are enotify's ACTIVITY_MAIN_TYPE_* ints, validated
server-side at device registration; payouts is 19 (16 is retired upstream
and must not be reused). The map is duplicated in three containers, so all
three are updated together.
Filter labels move to their own notification.filters.* namespace. They
previously reused notification.<key>, which for the newer types holds the
notification BODY string - payouts is "Payout received: ${amount}" and
delegations is "delegated", so reusing them would render template text in
the dropdown.
Defaults follow the existing mobile convention: initialState true plus a
v21 migration backfilling the four keys for existing installs, exactly as
v20 did for scheduledPublished. autoMergeLevel1 keeps persisted
notificationDetails as-is, so a new key is never defaulted from
initialState without a migration.
Greptile SummaryThis PR makes notification filters and push settings consistent across supported notification types. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "chore(deps): bump @ecency/sdk to 2.3.61" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| title={intl.formatMessage({ | ||
| id: 'settings.notification.delegations', | ||
| })} | ||
| type="toggle" |
There was a problem hiding this comment.
When a user changes any of the four new toggles, its action type reaches _handleToggleChanged, which has no matching case and falls through to default. _handleNotification never runs, so Redux and device registration remain unchanged and the toggle snaps back.
| dropdownIconName="arrow-drop-down" | ||
| options={FILTERS.map((item) => | ||
| intl.formatMessage({ id: `notification.${item.key}` }).toUpperCase(), | ||
| intl.formatMessage({ id: `notification.filters.${item.key}` }).toUpperCase(), |
There was a problem hiding this comment.
This lookup moves every filter label to notification.filters.*, but only the English locale defines that namespace. With another supported locale selected, these calls have no defaultMessage, so the filter bar can display uppercased message IDs such as NOTIFICATION.FILTERS.REPLIES instead of readable labels.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a2984323a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| id: 'settings.notification.delegations', | ||
| })} | ||
| type="toggle" | ||
| actionType="notification.delegations" |
There was a problem hiding this comment.
When a user changes this new toggle (and the three new toggles that follow), SettingsItem passes the actionType to handleOnChange, but SettingsContainer._handleToggleChanged only forwards notification action types through notification.scheduledPublished; notification.delegations, notification.payouts, notification.accountUpdate, and notification.weeklyEarnings all fall through the default case. In that scenario no Redux action is dispatched and _setPushToken is never called, so users cannot actually mute these newly added push types from Settings.
Useful? React with 👍 / 👎.
_handleToggleChanged switches on actionType and only forwarded the pre-existing notification.* cases, so the four new toggles fell through to default: no Redux action dispatched, _setPushToken never called, and the switch snapped back with nothing persisted. Add the four action types to the notification case group.
|
Triaged both P1s. Toggles skip the update path — confirmed and fixed in da42a52. Translated labels lose their fallback — not reproducible, declining. messages={{ ...flattenMessages(messages['en-US']), ...flattenMessages(messages[locale]) }}
Crowdin picks the new keys up from en-US on the next sync, as with any other added string. |
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFour notification categories are added to filters, localized labels, Redux state, settings toggles, persistence migration, and push notification handling. Filter dropdown labels now use the ChangesNotification settings expansion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsScreen
participant Redux
participant ApplicationContainer
participant PushSettings
SettingsScreen->>Redux: toggle notification category
Redux-->>SettingsScreen: update notificationDetails
SettingsScreen->>ApplicationContainer: enable notification settings
ApplicationContainer->>PushSettings: save notify_types
PushSettings-->>ApplicationContainer: notification settings updated
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/redux/reducers/applicationReducer.ts`:
- Around line 83-86: Update the CHANGE_ALL_NOTIFICATION_SETTINGS reducer case to
include delegationsNotification, payoutsNotification, accountUpdateNotification,
and weeklyEarningsNotification in the bulk update. Use explicit false-preserving
payload handling so legacy payloads that provide false remain false while
unspecified fields retain their existing behavior.
In `@src/screens/application/container/applicationContainer.tsx`:
- Around line 1081-1084: Update the foreground and WebSocket notification
allowlists in the relevant handlers to include the server event names for
payout, account-update, and weekly-earnings notifications, alongside the
existing event types. Ensure both conversion/surfacing and unread-count refresh
paths recognize these new events consistently.
In `@src/screens/settings/screen/settingsScreen.tsx`:
- Around line 354-389: Update SettingsContainer._handleToggleChanged to handle
notification.delegations, notification.payouts, notification.accountUpdate, and
notification.weeklyEarnings emitted by the four SettingsItem controls,
dispatching the corresponding Redux and push-token setting updates instead of
falling through default. Preserve existing toggle handling for all other action
types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b490bdb8-05ef-4cc8-bedf-80ff957708ab
📒 Files selected for processing (13)
src/components/notification/view/notificationView.tsxsrc/config/locales/en-US.jsonsrc/providers/ecency/ecency.types.tssrc/redux/actions/applicationActions.tssrc/redux/constants/constants.tssrc/redux/reducers/applicationReducer.tssrc/redux/store/store.tssrc/screens/application/container/applicationContainer.tsxsrc/screens/login/container/loginContainer.tsxsrc/screens/settings/container/settingsContainer.tsxsrc/screens/settings/screen/settingsScreen.tsxsrc/utils/migrationHelpers.test.tssrc/utils/migrationHelpers.ts
| delegationsNotification: boolean; | ||
| payoutsNotification: boolean; | ||
| accountUpdateNotification: boolean; | ||
| weeklyEarningsNotification: boolean; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include the new fields in the bulk notification-settings reducer.
CHANGE_ALL_NOTIFICATION_SETTINGS at Lines 289-308 does not update these four fields. Any “enable/disable all notifications” action will leave delegations, payouts, account updates, and weekly earnings unchanged. Preserve explicit false values for legacy payloads.
Suggested fix
notificationDetails: {
...state.notificationDetails,
+ delegationsNotification:
+ action.payload.delegationsNotification ??
+ state.notificationDetails.delegationsNotification,
+ payoutsNotification:
+ action.payload.payoutsNotification ??
+ state.notificationDetails.payoutsNotification,
+ accountUpdateNotification:
+ action.payload.accountUpdateNotification ??
+ state.notificationDetails.accountUpdateNotification,
+ weeklyEarningsNotification:
+ action.payload.weeklyEarningsNotification ??
+ state.notificationDetails.weeklyEarningsNotification,
mentionNotification: action.payload.mentionNotification,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/redux/reducers/applicationReducer.ts` around lines 83 - 86, Update the
CHANGE_ALL_NOTIFICATION_SETTINGS reducer case to include
delegationsNotification, payoutsNotification, accountUpdateNotification, and
weeklyEarningsNotification in the bulk update. Use explicit false-preserving
payload handling so legacy payloads that provide false remain false while
unspecified fields retain their existing behavior.
| <SettingsItem | ||
| title={intl.formatMessage({ | ||
| id: 'settings.notification.delegations', | ||
| })} | ||
| type="toggle" | ||
| actionType="notification.delegations" | ||
| isOn={delegationsNotification} | ||
| handleOnChange={handleOnChange} | ||
| /> | ||
| <SettingsItem | ||
| title={intl.formatMessage({ | ||
| id: 'settings.notification.payouts', | ||
| })} | ||
| type="toggle" | ||
| actionType="notification.payouts" | ||
| isOn={payoutsNotification} | ||
| handleOnChange={handleOnChange} | ||
| /> | ||
| <SettingsItem | ||
| title={intl.formatMessage({ | ||
| id: 'settings.notification.account_update', | ||
| })} | ||
| type="toggle" | ||
| actionType="notification.accountUpdate" | ||
| isOn={accountUpdateNotification} | ||
| handleOnChange={handleOnChange} | ||
| /> | ||
| <SettingsItem | ||
| title={intl.formatMessage({ | ||
| id: 'settings.notification.weekly_earnings', | ||
| })} | ||
| type="toggle" | ||
| actionType="notification.weeklyEarnings" | ||
| isOn={weeklyEarningsNotification} | ||
| handleOnChange={handleOnChange} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Route the four new notification toggle actions.
These controls emit action types that SettingsContainer._handleToggleChanged does not handle (Line 472-483), so toggling them falls through default; Redux state and push-token settings are never updated.
Suggested handler addition
case 'notification.scheduledPublished':
+ case 'notification.delegations':
+ case 'notification.payouts':
+ case 'notification.accountUpdate':
+ case 'notification.weeklyEarnings':
this._handleNotification(action, actionType);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/screens/settings/screen/settingsScreen.tsx` around lines 354 - 389,
Update SettingsContainer._handleToggleChanged to handle
notification.delegations, notification.payouts, notification.accountUpdate, and
notification.weeklyEarnings emitted by the four SettingsItem controls,
dispatching the corresponding Redux and push-token setting updates instead of
falling through default. Preserve existing toggle handling for all other action
types.
… paths Both the FCM foreground handler and the websocket handler allowlist event types explicitly, and payouts, account_update and weekly_earnings were in neither. They produced no foreground notification and no unread-count refresh, so the toggles added here looked functional but delivered nothing while the app was open. Adding them to the allowlist alone would have been worse than leaving them out: the websocket->FCM title was a ternary chain ending in a delegation fallback, so every newly-allowed type would have announced itself as a delegation. Replace that chain with a switch that handles each allowlisted type explicitly and falls back to the bare source handle.
|
Triaged the CodeRabbit findings. Two of three actionable, one declined. Foreground + websocket allowlists — confirmed and fixed in c144c4a. Worth flagging that the suggested fix alone would have made things worse: the websocket→FCM Route the four new toggle actions — already fixed in da42a52, before this review landed. Bulk notification-settings reducer — declining. The suggested diff is also behaviourally identical to the current code. Legacy realm payloads predate these fields, so |
Picks up the notification enum additions released from vision-next#1229 (NotificationFilter.ACCOUNT_UPDATES/WEEKLY_EARNINGS and the NotifyTypes ints for delegations/payouts/account_update/weekly_earnings), keeping the shared definitions aligned with the filter and toggle work in this PR. 2.3.57 -> 2.3.61 changes no dependencies, so the lockfile edit is limited to the one entry; verified with yarn install --frozen-lockfile against an isolated modules folder.
1 participant
Companion to ecency/vision-next#1229, which does the same for web.
Notification types were exposed inconsistently on mobile:
The filter bar offered 5 of the 10 enum values. This adds the missing filter entries and push toggles so every type the feed renders can also be filtered and muted.
Notes
notifyTypesConstvalues are enotify'sACTIVITY_MAIN_TYPE_*ints, validated server-side at device registration. Payouts is 19 — 16 is commented out upstream and must not be reused. The map is duplicated acrossloginContainer,settingsContainerandapplicationContainer, so all three are updated together (noteapplicationContainerkeys on the full*Notificationname while the other two use the short name).account_updatesfilter path is plural while the notificationtypestring isaccount_update(singular). enotify routes on the plural.Filter labels moved to their own namespace
Labels previously came from
notification.<key>, which for the newer types holds the notification body string —payoutsis"Payout received: ${amount}"anddelegationsis"delegated". Reusing those would have rendered template text in the dropdown, so labels now live undernotification.filters.*.Defaults
Follows the existing convention rather than leaving the new toggles off:
initialState: trueplus a v21 migration backfilling the four keys, exactly as v20 did forscheduledPublished.autoMergeLevel1keeps persistednotificationDetailsas-is on rehydration, so a new key is never defaulted frominitialStatewithout a migration.This means existing users start receiving delegation, payout, account-update and weekly-earnings pushes after upgrading. That is deliberate and matches how every other mobile notification type behaves, but it is the reviewable decision in this PR.
Verification
eslinton all touched paths: 0 errors/api/weekly_earnings/<user>→type: weekly_earnings,/api/account_updates/<user>→type: account_updateThe api-proxy allowlist entries these filters depend on were added separately; without them the new tabs would return empty.
Summary by CodeRabbit