feat(reminder): wire LocalReminderApplication with in-memory adapters - #238
feat(reminder): wire LocalReminderApplication with in-memory adapters#238gac0812 wants to merge 4 commits into
Conversation
Replace mock reminder composition with the real coordinator so native fire events no longer stack JS dialogs and permission returns can rebuild.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
… queue startInternal already owns enqueueOp, so calling public confirm/snooze during hydration deadlocked cold start for confirmed and snoozed rows.
|
@fennoai /review |
Review Complete
Focused tests were not runnable because |
There was a problem hiding this comment.
Review
Found two integration issues that prevent the new local reminder wiring from functioning in the production composition root. Focused verification could not run because the checked-in frontend/node_modules/.bin/vitest is not executable/available (vitest: Permission denied) and TypeScript dependencies are missing.
Load active SQLite rows before start/rebuild, and map native fired/disposition callbacks onto NativeAlarmScheduler.
|
@fennoai /review |
Review Complete
|
There was a problem hiding this comment.
Updated Review
The two findings from the previous review are partially addressed: schedules are now hydrated from SQLite and the native event APIs are exposed. I found four remaining integration/lifecycle issues in the new wiring.
Verification passed: npm ci, focused Jest tests (40 tests), and full npm run check (298 tests across Vitest/Jest, lint, formatting, and typecheck).
| let cancelled = false; | ||
| void (async () => { | ||
| if (isHydratableScheduleReader(reminderPorts.schedules)) { | ||
| await hydrateInMemorySchedulesFromLocalDb(reminderPorts.schedules, accountId); |
There was a problem hiding this comment.
[P1] Prevent an obsolete account hydration from mutating the shared reader. The cleanup flag is checked only after hydrateInMemorySchedulesFromLocalDb has already called reader.replaceAll(...). If account A's query is in flight while the user signs out and authenticates as B, A can resolve last and overwrite the reader with A's schedules; B's running reminder application is subscribed to that reader and can then register A's reminders. Make hydration return a snapshot and commit it only if the effect generation/account is still current, or otherwise make the write account-scoped/cancellable.
| if (isHydratableScheduleReader(reminderPorts.schedules)) { | ||
| await hydrateInMemorySchedulesFromLocalDb(reminderPorts.schedules, accountId); | ||
| } | ||
| await reminder.rebuild(); |
There was a problem hiding this comment.
[P2] Avoid rebuilding twice on every foreground transition. For the in-memory reader, hydrateInMemorySchedulesFromLocalDb calls replaceAll, which synchronously notifies LocalReminderApplication and enqueues a rebuild; this explicit reminder.rebuild() immediately enqueues a second rebuild. Each pass drops registrations and cancels/reschedules native alarms, causing unnecessary alarm churn and a possible scheduling gap. Only call this explicitly for readers that do not notify, or suppress one of the rebuild paths.
| reader.replaceAll(schedules); | ||
| return schedules.length; | ||
| } catch { | ||
| reader.replaceAll([]); |
There was a problem hiding this comment.
[P1] Do not erase the last valid reminder snapshot on a transient SQLite failure. This hydration runs whenever the app returns to the foreground; any temporary open/query error executes replaceAll([]), whose notification makes the running application drop registrations and cancel all scheduled alarms. The error is swallowed, so the user receives no indication and can miss reminders until a later successful foreground refresh. Preserve the current same-account snapshot on refresh failure while clearing explicitly during account teardown/switch.
| return subscribeNativeAlarmEvents((payload) => { | ||
| listener({ | ||
| type: payload.type, | ||
| schedule_id: payload.scheduleId, |
There was a problem hiding this comment.
[P1] Pass request.schedule_id into the native scheduling call so this field can be populated. The native bridge's schedule API still accepts only (triggerAtMillis, title), and NativeAlarmScheduler.schedule never sends the schedule id, yet events and consumed dispositions are now expected to return payload.scheduleId. The native layer therefore has no schedule identifier to persist/emit for the alarm, so these events cannot be correlated and LocalReminderApplication will ignore or mis-handle them. The current tests inject a pre-correlated payload directly and do not cover the schedule-to-event round trip.
1 participant
Summary
InMemoryLocalScheduleReader与LocalReminderAdapters（送达记账、NoopPopup、本地 recovery/disposition sync）createAppServices将MockReminderApplication换成LocalReminderApplication，日程/送达/状态/恢复走内存适配器；device保持NativeDeviceCapabilityLocalReminderApplication订阅原生 fire/snooze/dismiss：原生已响铃时不再叠 JS 弹窗/TTSAppProviders挂上useReminderPermissionsOnLaunch，并在回到前台时rebuildMerge Order
Scope
frontend/src/features/reminder/data/local/InMemoryLocalScheduleReader.tsfrontend/src/features/reminder/data/local/LocalReminderAdapters.tsfrontend/src/features/reminder/application/LocalReminderApplication.tsfrontend/src/features/reminder/application/interfaces/AlarmSchedulerPort.ts（可选 subscribe/stopRinging）frontend/src/app/composition/createAppServices.tsfrontend/src/app/AppProviders.tsxfrontend/tests/unit/features/reminder/inMemoryLocalScheduleReader.test.tsfrontend/tests/unit/features/reminder/localReminderAdapters.test.tsfrontend/tests/unit/features/reminder/localReminderApplication.nativeAlarm.test.tsOut of scope
timeflow-alarm原生模块本体（见 feat(reminder): add timeflow-alarm native module and Expo wiring #237）Verification
npm run checknpm ciCloses #233