feat(mobile): opt-in persistent reminder notification (#9) - #328
Conversation
Opt-in (default off) ongoing Android notification showing streak + today's progress, riding the existing widget feed. sticky+autoDismiss:false on a dedicated LOW-importance channel; stable identifier refreshes it in place. Hooks into syncWidgetData's existing log/foreground/boot triggers (no extra fetch); signed-out cancels. Preferences toggle backed by a persisted Zustand store. i18n in both locales. Mobile-only: an ongoing OS notification has no web counterpart (like the Android widget). No backend change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Code Review: PR #328
Scope: PR #328 — feat(mobile): opt-in persistent reminder notification (#9)
Recommendation: NEEDS WORK
Summary
PR #328 adds an opt-in Android ongoing notification that shows the user's streak and today's habit progress in the system tray. The implementation is well-structured — clean module boundaries, solid type narrowing, 14 unit tests, and correct i18n coverage in both locales. One confirmed correctness bug was found: the persistent reminder notification is not dismissed when the user logs out, despite the PR body claiming "Signed-out cancels it."
Findings
Critical
None.
High
[HIGH] Persistent reminder is not dismissed on logout
· dimension: Correctness (#1)
· location: apps/mobile/stores/auth-store.ts:323-348
· issue: logout() calls clearWidgetToken() but never calls cancelPersistentReminder() or syncWidgetData(). The only dismissal path is syncWidgetData() → refreshPersistentReminder(null), which is only triggered by the AppState 'active' handler in providers.tsx (line 118) — i.e., the next time the user brings the app to foreground after logging out. The PR body states "Signed-out cancels it", which is factually incorrect for the direct logout action.
· risk: A user who logs out (e.g., from a shared device, or to switch accounts) will continue to see a notification in the Android tray bearing streak/habit data that belongs to their session. The notification persists until they next foreground the app. While the tapped deep-link / respects the auth gate and does not expose data, showing a signed-out user's streak count in the notification tray is a data-exposure UX violation.
· fix: Add await cancelPersistentReminder().catch(() => {}) to the logout() action in auth-store.ts, after clearWidgetToken():
await clearWidgetToken().catch(() => {})
await cancelPersistentReminder().catch(() => {}) // add this line
queryClient.clear()Also apply the same fix to checkAuth() at ~line 354 where it calls clearWidgetToken() on the signed-out fast-path (when !token), since that path also skips reminder cancellation.
· reference: CLAUDE.md rule 1 (root cause over workarounds), rule 8 (error handling at boundaries)
Medium
None.
Low / Info
None (below signal gate — not posted).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — intentionally mobile-only (Android OS capability, same category as the home-screen widget; web Preferences page has no counterpart for this OS feature) |
| i18n-syncer | IN SYNC — all 7 persistentReminder.* keys present and correct in both en.json and pt-BR.json; placeholders consistent; all keys have callsites |
| contract-aligner | N/A — no packages/shared/src/types/* or endpoints.ts changes; no backend changes |
| security-reviewer | N/A — no orbit-api changes |
Validation
| Check | Result |
|---|---|
| Lint | N/A (sandbox; author reports 3/3 clean) |
| Type check | N/A (sandbox; author reports 3/3 clean) |
| Tests | N/A (sandbox; author reports mobile 649 passed, shared 1031 passed) |
| Build (api) | N/A — no backend changes |
Deferred
- Dimension 8 (DESIGN.md): N/A for notification content.
PersistentReminderRowreuses existingSettingsRow+Switchprimitives;schemes.purple.accent.dark.primaryfor the notification LED is the identical pattern asuse-push-notifications.ts:168. - Dimension 11 (Contract drift): N/A — no shared type changes; i18n additions are additive.
- Dimension 12 (Security / frontend): Checked — no XSS surface, no PII in notification content, deep-link
'/'is auth-gated. - Dimension 13 (Backend hard rules): N/A — no orbit-api changes.
- Validation: N/A in sandbox.
What's good
- Module injection pattern (
__setPersistentReminderModuleForTests) makes the 14-test suite fast and deterministic withoutvi.mockside effects. extractReminderFeedcorrectly mirrors the widget's "completed of top-level" math —isTopLevelDonehandles flat-complete, all-children-complete, and mixed cases.- Zero extra network calls: the reminder rides the existing
syncWidgetData()feed. - Stable
identifier = 'orbit-persistent-reminder'ensures refreshes replace-in-place rather than stacking — correct ongoing notification pattern. partializein the Zustand store persists onlyenabled, not the full state. Clean.isPersistentReminderSupported()guard ensures the toggle is invisible on web/iOS and the refresh path is a no-op when disabled.
Recommendation
Fix the logout gap in auth-store.ts (one-liner: await cancelPersistentReminder().catch(() => {}) after clearWidgetToken()) and apply the same to the checkAuth signed-out fast-path. Everything else is solid — architecture, test coverage, and i18n handling are all good.
🤖 Generated with Claude Code
logout() and checkAuth()'s signed-out fast-path now call cancelPersistentReminder() right after clearWidgetToken(), so a signed-out user's streak/progress no longer lingers in the Android tray (the ongoing notification can't be swiped away). Previously dismissal only fired on the next foreground via syncWidgetData(null). Two tests pin both paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed in |
|
There was a problem hiding this comment.
Code Review: PR #328 — APPROVE
Summary
Clean, well-tested implementation of an opt-in Android ongoing notification. 14 new unit tests, proper triple-layer platform gating, design system tokens used correctly, i18n in sync. No Critical or High findings.
Findings
Critical: None
High: None
Medium — Missing hook-level tests for usePersistentReminder
- location:
apps/mobile/hooks/use-persistent-reminder.ts(no test file exists) - issue: The hook owns the toggle branching logic (permission-denied stays off, permission-granted enables and calls
syncWidgetData, toggle-off cancels). Only the lower-level functions it calls are tested, not the hook's own state transitions. - fix: Add
apps/mobile/__tests__/hooks/use-persistent-reminder.test.tscovering at minimum: (a) permission denied leavesenabled=false, (b) permission granted setsenabled=trueand triggerssyncWidgetData, (c) toggle-off callscancelPersistentReminder. - reference: CLAUDE.md — "Every new feature needs tests. Tests asserting behavior, not implementation details."
Low/Info: None
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — Android-only by OS capability; UI guarded by isSupported; i18n lands in both locales |
| i18n-syncer | IN SYNC — all 7 persistentReminder.* keys in en.json and pt-BR.json with real translations |
| contract-aligner | N/A — no shared types or endpoints changed |
| security-reviewer | N/A — no orbit-api code changed |
What's good
- Module injection (
__setPersistentReminderModuleForTests) mirrors existing pattern — consistent and test-friendly - Piggybacks
syncWidgetData— no extra fetch - Stable ID (
orbit-persistent-reminder) means re-posts replace in place, not stack - Triple-layer platform guard:
isPersistentReminderSupported(),isSupportedUI prop, per-function internal guard - Signed-out dismiss wired into both
logout()andcheckAuth(no-token), both paths tested schemes.purple.accent.dark.primaryis a real token from color-schemes.ts, not hardcoded- No
any, noconsole.log, comment policy clean across all new files
🤖 Generated with Claude Code



Opt-in persistent reminder (Android ongoing notification)
Closes #9. An opt-in (default OFF) ongoing Android notification that keeps the user's streak + today's progress in the tray.
Mechanism — a true ongoing notification, refreshed in place
expo-notificationssticky: true(AndroidisOngoing— not swipeable) +autoDismiss: false, on a dedicated LOW-importance channel (persistent-reminder— no sound, no heads-up, quiet). A stable identifier (orbit-persistent-reminder) means each refresh replaces it in place rather than stacking. Tap deep-links to Today.Rides the existing widget feed — no extra fetch
syncWidgetData()already fetches the widget payload on exactly three triggers (log, app-foreground, boot). The reminder hooks into that same single fetch —refreshPersistentReminder(data)— projecting streak + progress from the same payload (mirroring the native widget's "completed of top-level" math). Signed-out cancels it.Opt-in toggle
A "Persistent reminder" row in Preferences (default OFF), backed by a
persist-ed Zustand store. Turning on requests notification permission and posts immediately; turning off dismisses it. When off, the refresh path is a no-op — behavior is byte-identical to today.Mobile-only by nature
An ongoing OS notification is an Android capability with no web counterpart (like the Android home-screen widget). i18n keys land in both
en.jsonandpt-BR.json(parity test green). No backend / API / migration change.Tests + validation
14 new unit tests (defaults-off, posts-with-feed-numbers, refresh-in-place, off/signed-out dismiss, feed projection, permission grant/deny).
type-check3/3 clean,lint3/3 zero errors, mobile 649 passed, shared 1031 passed.🤖 Generated with Claude Code