feat(companion): local due-back reminders for checked-out bookings - #2747
Open
carlosvirreira wants to merge 3 commits into
Open
feat(companion): local due-back reminders for checked-out bookings#2747carlosvirreira wants to merge 3 commits into
carlosvirreira wants to merge 3 commits into
Conversation
Two local notifications per booking checked out from this device — due soon (1h before) and due now — scheduled at checkout and torn down the moment the booking stops being out. Nothing is fire-and-forget: the scheduled set is always re-derived from the server, so reminders can never nag about gear that is already back. Architecture (lib/reminders/): - plan.ts: pure, dependency-free planner (single source of truth for what fires and when; absolute-date triggers so DST cannot shift a reminder). - service.ts: schedule/cancel/reconcile runtime. Every sync re-fetches the booking; 404/403 is authoritative 'gone' (cancels + untracks, healing web deletes and lost workspace access) while network failures leave state untouched for the next reconcile. Map mutations are serialized on a promise queue; the fetch and the interactive permission dialog run OUTSIDE it so a check-in's cancel never waits behind a timeout or an open prompt. Persisting-after-scheduling failures roll the schedules back so storage errors cannot create uncancellable reminders. Pending count stays under iOS's 64-notification cap (future-dated only, furthest-out dropped first, never silently). - notifications-native.ts: lazy guarded require — expo-notifications calls requireNativeModule at module top level, so a static import would crash pre-notifications builds (older dev clients) at startup. - use-booking-reminders.ts: init + foreground reconcile (leaving the foreground marks the debounce stale so every genuine return reconciles) + tap handling with cold/warm dedupe, workspace switch when the reminder belongs to another org, and pushIntoTab-anchored navigation. Wiring: - Schedule on checkout success (scanner fulfil-and-checkout, detail full and partial checkout). Cancel on full check-in, complete partial check-in, cancel, archive, delete, and on sign-out (fetches would 401 forever after it, so tracked records could never heal). - Settings: master 'Booking reminders' toggle mirroring the scan-sound pattern; off cancels everything but keeps the tracked map so on restores via reconcile; a denied OS permission points at device settings. - apiFetch now also returns the HTTP status (additive) so callers can tell an authoritative 404/403 from a network failure. - Permission is asked in context (first checkout / the toggle), never on launch. No app cron: due-times are local notifications scheduled at checkout; the OS delivers them. Needs a new native build (expo-notifications); all logic after that build is plain JS and OTA-patchable.
🩺 React Doctor — companionFindings on the files changed by this PR:
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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: a9c0ea19a9
ℹ️ 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".
…, tap timing - Generation guard: cancellations (check-in, sign-out) bump a per-booking / global counter synchronously; a sync captures both before its fetch and its queued apply aborts if either moved. Closes the race where a reconcile fetch reading ONGOING lands after a check-in's cancel and resurrects reminders for returned gear. isEnabled is also re-checked at apply time for the toggle-off flavour of the same race. - Clear reminders on EVERY authenticated -> signed-out transition (session expiry, revocation, refresh failure via onAuthStateChange), not only the explicit sign-out button — after any of them reconciles 401 forever and a later user of the device must not see the previous account's bookings. - Cold-start reminder taps: wait for the workspace list and mark the response handled only after navigation ran, so a tap arriving before OrgProvider finishes loading is retried (previously it was consumed and the booking never opened). Warm taps during loading are left unhandled for the cold-start path to pick up. - Drop an always-true negation flagged by code scanning.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds booking due-back reminders to the companion app, including planning, native notification scheduling, persisted preferences, lifecycle reconciliation, notification-tap navigation, booking mutation integration, sign-out cleanup, and Expo configuration. ChangesBooking reminders
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BookingScreen
participant ReminderService
participant API
participant Notifications
BookingScreen->>ReminderService: sync after checkout
ReminderService->>API: fetch booking state
API-->>ReminderService: booking and HTTP status
ReminderService->>Notifications: schedule due-back reminders
Notifications-->>BookingScreen: reminder tap
BookingScreen->>BookingScreen: open booking route
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/companion/app/(tabs)/settings.tsx (1)
308-361: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSwitch state can drift from actual OS permission.
remindersOnonly reflects the persisted preference, not the live OS notification permission. If a user grants permission once and later revokes it from device Settings without reopening this screen, the switch still shows ON while no reminders will actually fire, and the mismatch only surfaces the next time the user explicitly toggles.Consider re-querying permission status on screen focus (exporting a lightweight permission-check from
service.ts) so the switch can reflect reality without requiring a toggle round-trip.🤖 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 `@apps/companion/app/`(tabs)/settings.tsx around lines 308 - 361, Update the settings screen’s focus handling around remindersOn to re-query live OS notification permission whenever the screen gains focus, using a lightweight exported permission-check helper from service.ts. Synchronize the switch state with the permission result while preserving the existing persisted preference and toggle behavior in setRemindersEnabled.
🤖 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 `@apps/companion/lib/auth-context.tsx`:
- Around line 65-68: Update clearAllBookingReminders and the signOut flow in
auth-context so reminder cancellation or storage failures are not swallowed as
success. Propagate the failure or return an explicit success result, and make
signOut handle that outcome before completing authentication sign-out, ensuring
cleanup is confirmed before the session is cleared.
---
Nitpick comments:
In `@apps/companion/app/`(tabs)/settings.tsx:
- Around line 308-361: Update the settings screen’s focus handling around
remindersOn to re-query live OS notification permission whenever the screen
gains focus, using a lightweight exported permission-check helper from
service.ts. Synchronize the switch state with the permission result while
preserving the existing persisted preference and toggle behavior in
setRemindersEnabled.
🪄 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 Plus
Run ID: 0b7d24e1-527e-4817-91fc-dcf26355869f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
apps/companion/app.jsonapps/companion/app/(tabs)/bookings/[id].tsxapps/companion/app/(tabs)/scanner.tsxapps/companion/app/(tabs)/settings.tsxapps/companion/app/_layout.tsxapps/companion/lib/api/client.tsapps/companion/lib/auth-context.tsxapps/companion/lib/reminders/index.tsapps/companion/lib/reminders/notifications-native.tsapps/companion/lib/reminders/plan.tsapps/companion/lib/reminders/service.tsapps/companion/lib/reminders/use-booking-reminders.tsapps/companion/package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/companion/lib/auth-context.tsx`:
- Around line 42-48: Register the Supabase auth state listener before calling
getSession in the useEffect, and use the listener’s INITIAL_SESSION payload to
initialize session and hadSessionRef when available. Prevent the asynchronous
getSession result from overwriting a newer auth event, while preserving the
existing setSession and reminder-transition behavior in both initialization and
subsequent auth events.
🪄 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 Plus
Run ID: ad261d04-f27b-424c-b596-413417857dcf
📒 Files selected for processing (3)
apps/companion/lib/auth-context.tsxapps/companion/lib/reminders/service.tsapps/companion/lib/reminders/use-booking-reminders.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/companion/lib/reminders/use-booking-reminders.ts
- apps/companion/lib/reminders/service.ts
…d var - Register onAuthStateChange BEFORE getSession() and never let the stale initial snapshot clobber a fresher auth event, so a sign-out/refresh landing during startup can't be overwritten and its reminder cleanup can't be missed (Supabase's INITIAL_SESSION is caught by the listener). - Remove the now-unused destructured error in the sync fetch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the N3 launch spec: two local notifications per booking checked out from this device — due soon (1h before) and due now — scheduled the moment a checkout succeeds and torn down the moment the booking stops being out. The core promise: reminders can never nag about gear that is already back.
How it holds that promise
apiFetchnow additionally returns the HTTPstatus(additive) to make that distinction possible.requireNativeModuleat module top level, so a static import would crash any pre-notifications binary at startup (verified against the Metro config — inline requires are off). All runtime access goes through a lazy guarded require; pre-notifications dev clients no-op.Wiring
Schedule on: scanner fulfil-and-checkout, detail full + partial checkout (partial may flip RESERVED→ONGOING; sync is idempotent and only schedules when genuinely out).
Cancel on: full check-in, complete partial check-in (scanner + detail), cancel, archive, delete, sign-out.
Tap → opens the booking via
pushIntoTab(anchored back-stack), switching workspace first when the reminder belongs to another org; cold-start and warm taps are deduped.Settings: master toggle mirroring the scan-sound pattern; OFF cancels all but keeps the tracked map so ON restores via reconcile; denied OS permission points to device settings.
Review hardening (pre-PR adversarial pass, 20 agents)
15 confirmed findings fixed before this PR: the import-time crash, 404/403 never healing, sign-out leak, cross-workspace tap, bare
router.pushvspushIntoTab, permission-dialog head-of-line blocking, cap counting already-fired reminders, silent production drops, write-failure orphaning, debounce missing the next foreground, a require cycle through auth-context, dead exports, and doc drift.Verification
typecheck,lint,react-doctor(0 errors) all green.getAllScheduledNotificationsAsyncshows the pair cancelled; extend due time → reconcile reschedules.Rollout
Needs one new native build (expo-notifications). Everything after that build is plain JS and OTA-patchable — including a kill-switch via
eas updateif it misbehaves in the wild (pairs with #2740).🤖 Generated with Claude Code
Summary by CodeRabbit