feat(reminder): add reminder engine core (domain + application) - #266
Merged
Wintercom merged 7 commits intoAug 19, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This was referenced Aug 17, 2026
LUPENGHAN
force-pushed
the
feature/reminder-engine-core
branch
from
August 17, 2026 05:22
7570d6c to
4aeac3e
Compare
LUPENGHAN
force-pushed
the
feature/reminder-engine-core
branch
from
August 17, 2026 07:00
4aeac3e to
17c2c62
Compare
LUPENGHAN
force-pushed
the
feature/reminder-engine-core
branch
from
August 17, 2026 07:25
17c2c62 to
cdcce81
Compare
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 17, 2026
Code review (PR 1024XEngineer#266): acknowledgeNativeFire() only checked the persisted disposition_state (confirmed/pending) before proceeding, not the in-memory activeDeliveries/deliverLocks sets. Those sets are exactly what canDeliver() checks on the JS-driven handleTime path to skip a schedule the native alarm already owns -- but the guard only worked in that one direction. If handleTime was already mid-flight for a schedule (added to activeDeliveries, not yet persisted any disposition change) at the moment the native alarm for the same schedule fired, acknowledgeNativeFire would fall through and the two channels could both run -- the exact double-fire the surrounding comment says this mechanism eliminates. Added the activeDeliveries check as an additional early-return, matching canDeliver()'s guard.
LocalReminderApplication and its ports (AlarmSchedulerPort, LocationMonitorPort, DeviceCapabilityPort, NotificationChannels, ReminderDeliveryPort, ReminderApplicationPort), plus the domain layer (reminder.ts, strengthDelivery.ts) that drives arm/fire/confirm state transitions and recurring-schedule advancement. Pure logic layer: no device-specific adapters yet, nothing wired into the app. shared/time gains format.ts (used by the delivery strength calc); MockClock/MockTimeListener are removed since nothing in this stack still needs a fake clock once the real engine lands.
Code review (PR 1024XEngineer#266): acknowledgeNativeFire() only checked the persisted disposition_state (confirmed/pending) before proceeding, not the in-memory activeDeliveries/deliverLocks sets. Those sets are exactly what canDeliver() checks on the JS-driven handleTime path to skip a schedule the native alarm already owns -- but the guard only worked in that one direction. If handleTime was already mid-flight for a schedule (added to activeDeliveries, not yet persisted any disposition change) at the moment the native alarm for the same schedule fired, acknowledgeNativeFire would fall through and the two channels could both run -- the exact double-fire the surrounding comment says this mechanism eliminates. Added the activeDeliveries check as an additional early-return, matching canDeliver()'s guard.
Two changes on top of the rebase onto upstream/main's native alarm module (1024XEngineer#264/1024XEngineer#265): - AlarmSchedulerPort.consumeNativeDispositions (single-phase, clears the native buffer on read) is now peekNativeDispositions + ackNativeDispositions (two-phase), matching the split 1024XEngineer#265 already made on the native side. hydrateNativeDispositions() now acks only after the whole batch persists successfully, so a mid-batch failure leaves the un-acked rows in the native buffer for retry instead of losing them. - This PR's own interface changes (DeviceCapabilityPort.onAppActive, LocationMonitorPort.rebuild's new LocationRebuildTarget param, dropping MockTimeListener from shared/time) were never propagated to their implementers, so `npm run check` failed standalone -- expected for a mid-stack PR per its own description, but not acceptable for merging. Added the minimal implementations needed to close the gap: onAppActive on Native/MockDeviceCapability, MockLocationMonitor.rebuild's signature, and a new infrastructure/time/MockTimeListener placeholder. 1024XEngineer#269 replaces all of these with real adapters; this just keeps 1024XEngineer#266 green on its own. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
LUPENGHAN
force-pushed
the
feature/reminder-engine-core
branch
from
August 18, 2026 09:09
e050333 to
a057e5e
Compare
LUPENGHAN
marked this pull request as ready for review
August 18, 2026 09:10
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
…, add adapter tests hydrateNativeDispositions() runs inside startInternal(), which is itself the task currently occupying opChain. It was calling the public confirm()/snooze() wrappers, which re-enqueue onto that same opChain -- but that chain can't advance until startInternal() (the caller) returns, and startInternal() won't return until those re-enqueued calls resolve. Any cold start with a confirmed/snoozed row sitting in the native disposition buffer deadlocked permanently. Fixed by calling confirmInternal()/snoozeInternal() directly, since hydrateNativeDispositions() is already running as the sole active opChain task and doesn't need to re-enqueue. Also closes the Codecov patch-coverage gap on the adapter changes from the previous commit: onAppActive on Native/MockDeviceCapability, and the MockLocationMonitor/MockTimeListener mocks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
|
【测试覆盖 / 阻塞合并】本 PR 修改了 |
Collaborator
|
【P1 / 周期提醒不会推进到下一次发生时间】PR 描述声称修复了循环提醒推进，但当前 |
Wintercom
requested changes
Aug 18, 2026
Wintercom
left a comment
Wintercom
left a comment
Collaborator
There was a problem hiding this comment.
按照 git_rules.txt 的合并标准，本轮需要先处理以下阻塞项：
- PR 声称修复循环提醒推进，但当前实现触发后将
next_trigger_at清空，没有计算并保存下一次 RRULE occurrence，也没有重新注册下一次 AlarmManager；周期提醒只能触发一次。 LocalReminderApplication核心逻辑有大量变更，却没有对应应用层测试，无法证明冷启动 peek/ack、双通道竞争、确认/延后重排、stop/重启和周期推进正确。- PR 使用
Part of #263，但没有 Milestone；关联 Issue 在 MS3，过程追踪不完整。
具体问题已使用 gh 逐条评论。此前 reviewer 指出的 hydrateNativeDispositions 同一 opChain 死锁已在 ba17aa9 修复，本轮未重复评论。
…cation Covers what review flagged as missing: cold-start peek/ack (batch-level ack only after every row persists, none on partial failure), hydration for each native disposition state (confirmed/snoozed/pending), the native/JS dual-channel race guard, alarm rescheduling after confirm/snooze, stop/ restart cleanup, and recurring-reminder re-arming across two occurrences (demonstrating the next_trigger_at signal / state-layer-computes-the-next- occurrence split with actual test evidence, not just an explanation). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
三条阻塞项逐一回复： 1. 周期提醒推进（P1）
2. 应用层测试覆盖
另外，此前指出的 hydrateNativeDispositions 同一 opChain 死锁（cold start 只要原生缓冲区里有一条 confirmed/snoozed 记录就会永久卡住）已在 ba17aa9 修复：改成调用 confirmInternal()/snoozeInternal() 直接执行，不再通过公开的 confirm()/snooze() 重新排队。 3. Milestone 现在 |
Codecov flagged patch coverage at 66% after the application-layer test commit -- LocalReminderApplication.ts's location-schedule path (arm on leave, deliver on re-entry, no re-fire while still armed) and strengthDelivery.ts's low/high branches had zero coverage. Both were only exercised indirectly (or not at all) by the existing tests, which focused on time-triggered schedules. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…event routing Codecov target for this PR's patch is 91.16%, was at 76.78%. Adds: - low strength delivery: system notification only, no popup/vibration/audio - high strength delivery: popup + vibration + tts, falls back to local audio when tts fails - native "snoozed"/"dismissed" events routed through alarms.subscribe() to snooze()/confirm(), including unsubscribe on stop()
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 19, 2026
This PR's own commits removed MockLocalScheduleReader, MockReminderApplication, MockReminderDispositionSync, and MockReminderStateStore, but createAppServices.ts and features/reminder/index.ts (owned by the wiring PR, 1024XEngineer#271) still referenced them, so npm run check failed standalone -- expected for a mid-stack PR per its own description, but not acceptable for merging with a crashing composition root (11 test failures: "MockLocalScheduleReader is not a constructor"). Swapped in this PR's own real/interim replacements: InMemoryLocalScheduleReader, MemoryReminderStateStore, LocalReminderDispositionSync, and the real LocalReminderApplication instead of the mock. 1024XEngineer#271 will replace InMemoryLocalScheduleReader with the real SQLite-backed reader this PR also adds; this just keeps the branch green on its own in the meantime, mirroring what a057e5e already did for 1024XEngineer#266.
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 19, 2026
createAppServices() still constructed MockReminderApplication, whose start()/handleTime()/deliver() are all no-ops. Wiring IntervalTimeListener and ExpoAudioPlayback into reminderPorts (earlier commits on this PR) therefore had no effect in production: nothing ever called time.start() or audio.playTts(), because the engine that's supposed to call them was a stub that never touches its dependencies. Swapped in the real LocalReminderApplication (1024XEngineer#266). The other ports this PR doesn't own (schedules, location, notifications, state, disposition sync) stay on their existing Mocks -- LocalReminderApplication works against the ReminderApplicationDependencies interface regardless of which side of each port is real, same as the equivalent swap already done for 1024XEngineer#270. Added an assertion to the existing createAppServices test that starting the runtime actually calls through to reminderPorts.time.start(), proving the composition root wires an engine that consumes its ports instead of one that ignores them. Code review (PR 1024XEngineer#267, Wintercom).
Wintercom
pushed a commit
that referenced
this pull request
Aug 19, 2026
* feat(reminder): add audio playback and interval time adapters Part of #263. ExpoAudioPlayback (implements ReminderDeliveryPort's audio side) + audioDataUri helper, and IntervalTimeListener (implements the time port with a plain setInterval). Both only import from application interfaces already on main -- no dependency on the other adapter PRs in this stack (notifications/location/data layer), can be reviewed and merged independently of them. Removes MockAudioPlayback. * fix(reminder): retry audio mode setup after a failed attempt Code review (PR #267): ensureAudioMode() cached setAudioModeAsync()'s promise unconditionally, including on rejection (the .catch swallowed it into a resolved-undefined promise). A transient failure on the first reminder -- e.g. called before the native audio module finishes initializing after app launch -- would permanently skip playsInSilentMode/shouldPlayInBackground configuration for every reminder afterward, for the rest of the app session. Reset modeReady to null in the catch so the next call retries instead. * fix(reminder): wire IntervalTimeListener into the composition root createAppServices() still constructed MockTimeListener, whose start() explicitly never fires the listener. LocalReminderApplication.handleTime() therefore never received periodic ticks, so foreground due/overdue reminders were never triggered via the JS time channel even though this PR's own IntervalTimeListener was sitting right there, exported but unused. Code review (PR #267, fennoai bot). * test(reminder): cover the audio and interval-time adapters Patch coverage was 6.86% on this PR (Codecov, 95 lines missing) -- ExpoAudioPlayback.ts, audioDataUri.ts, and IntervalTimeListener.ts had no tests at all. audioDataUri.ts and IntervalTimeListener.ts are straightforward to test directly (fake timers for the interval, known base64 vectors for the encoder). ExpoAudioPlayback.ts was not: its dynamic import('expo-audio') throws in this Jest environment without --experimental-vm-modules, so jest.mock('expo-audio', ...) can never be reached -- the try/catch around the import swallows that TypeError the same way it would swallow a real "native module unavailable" failure, making the class's actual play/pause/ mode-setup logic structurally unreachable from a test. Added a constructor seam (loadExpoAudioModule, defaulting to the real loadExpoAudio) so tests can inject a fake module and exercise the real logic instead of only ever hitting the fallback branch. The sole production call site (createAppServices.ts) still does `new ExpoAudioPlayback()` unchanged. Patch coverage on the three files is now 94-100%. * style(reminder): run prettier on ExpoAudioPlayback and its test CI format:check failure after e718dcf. * fix(reminder): report played: false when audio mode setup fails ensureAudioMode()'s .catch(() => { this.modeReady = null; }) turned a rejected setAudioModeAsync() into a resolved promise -- the rejection never propagated past ensureAudioMode(), so playBytes()/playBundledAlarm() always proceeded to call player.play() and returned played: true regardless of whether silent-mode/background playback was actually configured. LocalReminderApplication trusts played: true and won't fall back to another delivery channel, so a mode setup failure could make a reminder silent with no fallback ever triggered. ensureAudioMode() now returns whether the mode is actually ready; the two callers bail out with played: false when it isn't, instead of proceeding to a play() that's unlikely to be heard. Retry-on-next-attempt behavior is unchanged: a failure still clears modeReady so the next play tries again. Code review (PR #267, Wintercom). * fix(reminder): wire LocalReminderApplication into the composition root createAppServices() still constructed MockReminderApplication, whose start()/handleTime()/deliver() are all no-ops. Wiring IntervalTimeListener and ExpoAudioPlayback into reminderPorts (earlier commits on this PR) therefore had no effect in production: nothing ever called time.start() or audio.playTts(), because the engine that's supposed to call them was a stub that never touches its dependencies. Swapped in the real LocalReminderApplication (#266). The other ports this PR doesn't own (schedules, location, notifications, state, disposition sync) stay on their existing Mocks -- LocalReminderApplication works against the ReminderApplicationDependencies interface regardless of which side of each port is real, same as the equivalent swap already done for #270. Added an assertion to the existing createAppServices test that starting the runtime actually calls through to reminderPorts.time.start(), proving the composition root wires an engine that consumes its ports instead of one that ignores them. Code review (PR #267, Wintercom).
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 20, 2026
…neer#268), fix conflicts 1024XEngineer#270 was still based on 1024XEngineer#266's merge point; 1024XEngineer#267 and 1024XEngineer#268 merged since then and both touched createAppServices.ts (real port swaps) and, for 1024XEngineer#267, the same reminder = new LocalReminderApplication(...) line 1024XEngineer#270 had already changed independently. Resolved by keeping every adapter's real implementation (both sides had already done their own swap for different ports) instead of picking one branch's version. AppRoot.tsx/AppRoot.test.tsx conflicts were two unrelated sets of new props (protectedClient from main, reminderState/scheduleReader from this branch) threaded through the same component chain -- combined both, dropped one genuinely unused import (AuthController) picked up along the way. Also fixed two real bugs the rebase surfaced, not introduced by it: - The branch's own "binds the reminder SQLite adapters" test passed authController={controller} to AppRoot, a prop it hasn't accepted since the pre-1024XEngineer#266 composition root shape; controller was otherwise unused. Collapsed to the one authenticated `services` instance every other test in the file already uses. - mockedCreateScheduleSnapshotPreparation's repository stub was a bare {}, fine before this branch existed. AppRoot's ready-state effect now calls scheduleReader.refresh() unconditionally, which calls through to repository.listSchedules() -- gave the stub real getSchedule/ listSchedules methods matching the ScheduleLocalRepository mock already used elsewhere in the same file. Verified: tsc/eslint/prettier clean, jest 464/464, vitest 87/87.
MeteorsLiu
pushed a commit
that referenced
this pull request
Aug 20, 2026
* feat(reminder): add SQLite-backed reminder data layer Part of #263. SqliteLocalScheduleReader / SqliteReminderStateStore read and persist against the real local database (ScheduleLocalRepository) instead of in-memory fixtures; geofence_radius_meters is hardcoded to 200m for now (known simplification, see Issue #263 Out of Scope). InMemoryLocalScheduleReader is kept as a non-persisted alternative implementation of the same port. LocalScheduleWriter's post-write hook refreshes the new reader after a voice-driven schedule mutation lands. Only depends on application interfaces already on main and the existing ScheduleLocalRepository -- independent of the audio/location/ notifications adapter PRs in this stack. Removes MockLocalScheduleReader, MockReminderApplication, MockReminderDispositionSync, MockReminderStateStore, mockReminderSchedules. * chore(reminder): remove unused LocalSystemNotification placeholder Code review (PR #270): LocalSystemNotification was a no-op SystemNotificationPort placeholder, exported but never imported anywhere -- fully superseded by the real ExpoSystemNotification adapter from the notifications PR, which is what createAppServices.ts on the wiring branch actually uses. Dead code, removed. * fix(reminder): wire SQLite adapters into app lifecycle * test(reminder): cover local data adapters * test(reminder): cover data adapters in Jest * fix(reminder): rebase onto upstream/main (#266-#268), fix conflicts #270 was still based on #266's merge point; #267 and #268 merged since then and both touched createAppServices.ts (real port swaps) and, for #267, the same reminder = new LocalReminderApplication(...) line #270 had already changed independently. Resolved by keeping every adapter's real implementation (both sides had already done their own swap for different ports) instead of picking one branch's version. AppRoot.tsx/AppRoot.test.tsx conflicts were two unrelated sets of new props (protectedClient from main, reminderState/scheduleReader from this branch) threaded through the same component chain -- combined both, dropped one genuinely unused import (AuthController) picked up along the way. Also fixed two real bugs the rebase surfaced, not introduced by it: - The branch's own "binds the reminder SQLite adapters" test passed authController={controller} to AppRoot, a prop it hasn't accepted since the pre-#266 composition root shape; controller was otherwise unused. Collapsed to the one authenticated `services` instance every other test in the file already uses. - mockedCreateScheduleSnapshotPreparation's repository stub was a bare {}, fine before this branch existed. AppRoot's ready-state effect now calls scheduleReader.refresh() unconditionally, which calls through to repository.listSchedules() -- gave the stub real getSchedule/ listSchedules methods matching the ScheduleLocalRepository mock already used elsewhere in the same file. Verified: tsc/eslint/prettier clean, jest 464/464, vitest 87/87.
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
2 participants
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.
关联 Issue
Part of #263
#264、#265 已合并；本分支已 rebase 到最新 upstream/main（丢掉了 5 个已被 #264/#265 吸收的重复提交，只保留两个真正独有的：reminder engine core 本体、和一个双通道去重修复），diff 现在只剩这个 PR 自己的内容。
改动
验收边界：周期提醒推进
next_trigger_at 置空是这一层的信号，不是完整实现：confirmInternal/runDeliver 触发/确认后把它置 null，标记"该往前挪一格了"；真正按 RRULE 算出下一次 occurrence 是 #270（feature/reminder-adapters-data）的 SqliteReminderStateStore 的职责。本 PR 接的是 MockReminderStateStore，不做这个计算，单独看只会触发一次——这是分层设计，不是遗漏，#270 合并后才闭环。测试里已经用真实状态层的行为模拟验证了两次推进。
验证
npx eslint .、npx tsc --noEmit、npm run test（jest 327/327、vitest 47/47）全绿，本 PR 现在单独可过完整 CI本轮不含