feat(reminder): add reminder engine core (domain + application) - #266
Open
LUPENGHAN wants to merge 4 commits into
Open
feat(reminder): add reminder engine core (domain + application)#266LUPENGHAN wants to merge 4 commits into
LUPENGHAN wants to merge 4 commits into
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✅ All modified and coverable lines are covered by tests. 📢 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 修复，本轮未重复评论。
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（先合，这个 draft 现在的 diff 包含它们还没合的内容，合完 rebase 会瘦身）
改动
验证
npx eslint .全绿npx tsc --noEmit这个 PR 单独跑不过：旧的 Mock 实现和 createAppServices.ts 还没跟上这次端口签名变化，等后面的适配器/接线 PR 落地才会全绿——这层本身是纯逻辑，不影响运行时行为本轮不含