feat(reminder): wire real engine into the app, drop remaining mocks - #271
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
LUPENGHAN
force-pushed
the
feature/reminder-wiring
branch
from
August 17, 2026 05:23
51b58e0 to
726361f
Compare
LUPENGHAN
force-pushed
the
feature/reminder-wiring
branch
from
August 17, 2026 07:00
726361f to
828327a
Compare
LUPENGHAN
force-pushed
the
feature/reminder-wiring
branch
from
August 17, 2026 07:26
828327a to
6ddfa0a
Compare
LUPENGHAN
force-pushed
the
feature/reminder-wiring
branch
from
August 17, 2026 07:53
6ddfa0a to
6314d1e
Compare
LUPENGHAN
force-pushed
the
feature/reminder-wiring
branch
from
August 17, 2026 10:22
6314d1e to
87f8782
Compare
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 17, 2026
Closes 1024XEngineer#272 voice.command.result gains an occurrence_overrides field alongside schedule/schedules. Backend: CommandOutcome/CommandResult carry it through from ScheduleMutationResult.occurrence_overrides (already populated by _delete_recurring_range for scope=this_occurrence, but previously dropped before reaching the wire); schedule_tools.py's _mutation_result serializes each ScheduleOccurrenceOverrideSnapshot the same way _snapshot_for_client already does for schedules. Frontend: AppliedCommand carries the new field through from the WS message; LocalScheduleWriter.applyCommandResult now handles schedule and occurrence_overrides independently (a this_occurrence delete produces only an override, no new schedule snapshot) and writes each override via the already-existing, already-tested ScheduleLocalRepository.upsertOccurrenceOverride. Independent of the reminder-integration branch stack (1024XEngineer#264-1024XEngineer#271) -- the occurrence-override table and upsertOccurrenceOverride already exist on main; this only needed LocalScheduleWriter's plain-main shape, not the reminder stack's SqliteLocalScheduleReader wiring.
Wintercom
pushed a commit
that referenced
this pull request
Aug 18, 2026
* feat(schedule): sync occurrence overrides to the client Closes #272 voice.command.result gains an occurrence_overrides field alongside schedule/schedules. Backend: CommandOutcome/CommandResult carry it through from ScheduleMutationResult.occurrence_overrides (already populated by _delete_recurring_range for scope=this_occurrence, but previously dropped before reaching the wire); schedule_tools.py's _mutation_result serializes each ScheduleOccurrenceOverrideSnapshot the same way _snapshot_for_client already does for schedules. Frontend: AppliedCommand carries the new field through from the WS message; LocalScheduleWriter.applyCommandResult now handles schedule and occurrence_overrides independently (a this_occurrence delete produces only an override, no new schedule snapshot) and writes each override via the already-existing, already-tested ScheduleLocalRepository.upsertOccurrenceOverride. Independent of the reminder-integration branch stack (#264-#271) -- the occurrence-override table and upsertOccurrenceOverride already exist on main; this only needed LocalScheduleWriter's plain-main shape, not the reminder stack's SqliteLocalScheduleReader wiring. * test(schedule): cover the this_occurrence override serialization path Prettier fix for LocalScheduleWriter.ts (npm run check caught it). Adds a realtime-toolbox test exercising a delete with scope=this_occurrence that produces an occurrence_override -- the only path that actually calls _override_for_client. Closes the 2-line patch-coverage gap Codecov flagged on the previous commit. * test(schedule): cover LocalScheduleWriter's missing-field rejection Closes the 1-line patch-coverage gap Codecov flagged: requireString's throw path had no test on this branch (the file existed on main with no dedicated test at all before this PR added one). * fix(schedule): surface every schedule a mutation produced, not just the first _delete_recurring_range's this_occurrence path, when it hits an existing replace override, soft-deletes the replacement schedule and returns it alongside the untouched parent in ScheduleMutationResult.schedules. _mutation_result only ever forwarded schedules[0] to the client, so that soft-delete never reached voice.command.result and the client kept showing the stale replacement as active. CommandOutcome already had a schedules (plural) field - only list_schedules populated it. _mutation_result now fills it with every schedule the mutation touched; schedule (singular) stays as schedules[0] for backward compat with clients not yet updated. * fix(schedule): check write results and transact LocalScheduleWriter applyCommandResult ignored the boolean applyCloudSchedule()/ upsertOccurrenceOverride() return, so a failed write (missing parent schedule, account mismatch) still let the caller send message.ack status=applied - the server believed the command was persisted when it wasn't. Now every write's return value is checked and a false throws, and all writes for one command result run inside a single transaction via the repository's new withTransaction(). A command can produce multiple schedule and override writes that together represent one voice command landing; without the transaction a partial failure would leave state the server never actually had, and since the caller already skips the ack on any throw, that state would never get retried either. Also switches to consuming the schedules (plural) field the backend now populates for mutations, explicitly excluding list_schedules by operation instead of relying on the coincidence that query results never set schedule/occurrence_overrides.
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.
Part of 1024XEngineer#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 1024XEngineer#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.
Swaps the composition root over to the real implementations added in the previous three commits: LocalReminderApplication replaces MockReminderApplication, SqliteLocalScheduleReader/SqliteReminderStateStore replace their Mock counterparts, and every device port (audio/notification/vibration/alarm/location) now points at its real adapter. ExpoLocationMonitor (system geofencing) is used for location monitoring; NativeLocationMonitor (Baidu SDK) stays in the repo but unwired -- see the comment in createAppServices.ts for how to switch. AppProviders/AppRoot gain the reminder permission-request flow (useReminderPermissionsOnLaunch, now driven by an injected AlertDialogPort instead of calling Alert.alert directly, with a settings-page fallback for denied background-location permission) and rebuild() the engine once permissions change. Removes MockReminderPresenter, the last remaining Mock* adapter.
Every branch in promptNext() except the overlay/full_screen/battery confirm-and-continue path returns from inside the try block, so the setTimeout(runPrompt, 250) that was meant to advance to the next missing permission -- placed after the try/finally -- was dead code for those branches. In practice: grant notifications, and exact_alarm (the next permission in line) would just never get prompted; same for any declined dialog, or a granted/denied location permission. Moved the continuation check into the finally block so it always runs regardless of which branch returned. Also replaces the old mock-based useReminderPermissionsOnLaunch test (deleted upstream when this hook's signature changed to take an injected AlertDialogPort + onPermissionsUpdated callback, with no replacement written) and drops the now-redundant .gitkeep placeholders left over from directories that have had real files in them since earlier commits in this stack.
LUPENGHAN
force-pushed
the
feature/reminder-wiring
branch
from
August 20, 2026 08:08
87f8782 to
4776bbf
Compare
LUPENGHAN
marked this pull request as ready for review
August 20, 2026 08: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}
Contributor
There was a problem hiding this comment.
审阅了组合根接线、提醒数据层、展示器以及启动权限流程。整体接线方向与 PR 描述一致；git diff --check 通过。当前环境没有可执行的 node_modules 测试/类型检查二进制，因此未能运行 Jest 或 TypeScript 验证。
Additional findings
frontend/src/features/reminder/presentation/AlertReminderPresenter.ts:?: [P1] Make hide dismiss the native alert, not only suppress actions:LocalReminderApplication.teardownDelivery()callspresenter.hide()during stop, rebuild, and replacement of an active delivery. This implementation only records the schedule as suppressed; theAlertDialogPorthas no dismiss operation and the already displayed React NativeAlertremains on screen. A logout, runtime stop, or schedule teardown can therefore leave a stale reminder dialog covering the next screen, with buttons that no longer have an active application listener. The presenter/dialog contract needs a real dismissal path (or a dismissible custom presenter) andhide()should use it.
AssistantConversationService.handleClose() nulled unsubscribeConnection without calling it. Switching from push-to-talk to continuous mode makes the shared AuthenticatedWebSocketClient drop and reopen the connection (the two modes negotiate different voiceMode), so the old service stayed subscribed to the new connection's TTS/PCM and pushed the same reply into the player alongside the continuous service -- audible as one sentence played twice, overlapping. dispose() already unsubscribed correctly; only the close path was missing it. Carried over from 1024XEngineer#245, which this stacked series replaces -- the fix is not reminder-scoped so none of 1024XEngineer#264-1024XEngineer#270 picked it up.
A barge-in that lands after the model already finished delivering a reply cancelled whatever was playing *next*, not the reply the phone was still sounding out. Two halves: Backend: _Turn reset _audio_id before that late interrupted() ran, so it sent AudioCanceled(audio_id=""). Added _last_audio_id, which survives the reset, and send that instead -- the cancellation now names the audio it actually refers to. Frontend: voice.tts.canceled routed stop() through playbackChain, so every PCM chunk already queued was still fed to the native player before the stop landed. It now bypasses the chain via stopPlaybackImmediately(), and chainPlayback() tags each queued operation with a playbackGeneration that stop bumps, so the stale queue is dropped rather than replayed. tts.end and tts.canceled are both matched against currentAudioId/canceledAudioId, so the server's follow-up tts.end for a cancelled reply no longer ends a newer stream or flips interrupted back to listening. The empty-audio_id case is still handled on the client so a not-yet-updated backend cannot stop a newer reply. 1024XEngineer#297 removed only the user-facing interrupt button; backend barge-in still drives voice.tts.canceled, so this path is live. Carried over from 1024XEngineer#245, which this stacked series replaces -- not reminder-scoped, so none of 1024XEngineer#264-1024XEngineer#270 picked it up.
Code review (PR 1024XEngineer#271, fennoai): the cleanup only cleared the timer and unsubscribed onAppActive, not awaitingReturnRef/skippedRef. If a user opened settings for exact_alarm (or a denied location permission) and logged out before returning, AppProviders reruns this effect with device=null, but the stale awaitingReturnRef stayed true. On the next login the new effect's promptNext() reads that same ref (it's a component-level useRef, not reset by the effect re-running) and returns immediately every time, and since the app is already active there's no new onAppActive event left to clear it -- every permission prompt stays disabled until the process restarts. Reset both refs in the cleanup so a fresh login starts a clean prompt round.
Codecov flagged 60.79% patch coverage across six files from this branch's recent commits. Closed each: - InMemoryLocalScheduleReader.ts: 0% because nothing in the app or tests actually uses it -- only re-exported from two barrels, never imported or instantiated anywhere in feature/reminder-wiring's own history. Deleted the file and its two re-exports instead of testing dead code. - AlertReminderPresenter.ts: new AlertReminderPresenter.test.ts covers every reason-specific message, the title fallback, confirm/snooze dispatch, unsubscribe, and hide()'s suppression window. The `?? '...'` message fallback is unreachable (MESSAGE_BY_REASON already covers every ReminderTriggerReason), so it's istanbul-ignored with a stated reason instead of faked with an invalid reason value. - useReminderPermissionsOnLaunch.ts: added 7 tests for branches the existing suite didn't reach -- denied notifications, failed openSettings on both the direct-settings and location paths, granted location, the bottom settings-redirect branch, a rejected getStatus(), and a dismissed (vs declined) dialog. Its own similarly-unreachable `prompt == null` branch (all 7 DevicePermission values already have a prompt) got the same istanbul-ignore treatment. - AppProviders.tsx: new AppProviders.test.tsx isolates the onPermissionsUpdated -> reminder.rebuild() wiring with a mocked useReminderPermissionsOnLaunch, instead of relying on AppRoot.test.tsx's much heavier integration setup for one line. - AssistantContinuousConversationService.ts: dismissReply() had no coverage at all before this branch touched one line of it (routing through stopPlaybackImmediately()); added a test that drives a reply through voice.tts.start/voice.dialogue.reply and asserts dismissReply() clears it and stops playback. - backend agent.py: the interrupted()-with-nothing-ever-spoken branch (_last_audio_id is None) wasn't exercised; added test_a_barge_in_before_any_reply_started_sends_no_cancellation. Verified: frontend tsc/eslint/prettier clean, Jest 520/520, Vitest 87/87; backend ruff/mypy clean, pytest 97.53% coverage.
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
Closes #263
Closes #262
依赖 #267、#268、#269、#270 全部先合（先合并前面所有适配器 PR，这个才是干净的接线 diff；现在这个 draft 的 diff 包含它们还没合的全部内容）。替代 #245（原来的整体合并 PR，会关掉指向这个拆分序列）。
改动
验证
npx tsc --noEmit、npx eslint .全绿npm run test：34 个套件、253 个用例全过本轮不含（见 #263 Out of Scope）