feat(reminder): add SQLite-backed reminder data layer - #270
Merged
MeteorsLiu merged 6 commits intoAug 20, 2026
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-adapters-data
branch
from
August 17, 2026 05:22
e92de83 to
310d4c2
Compare
LUPENGHAN
force-pushed
the
feature/reminder-adapters-data
branch
from
August 17, 2026 07:00
310d4c2 to
4374ff0
Compare
LUPENGHAN
force-pushed
the
feature/reminder-adapters-data
branch
from
August 17, 2026 07:26
4374ff0 to
fc16bc7
Compare
LUPENGHAN
force-pushed
the
feature/reminder-adapters-data
branch
from
August 17, 2026 07:52
fc16bc7 to
93806a6
Compare
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 17, 2026
Code review (PR 1024XEngineer#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.
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 17, 2026
features/reminder/index.ts still re-exported LocalSystemNotification after PR 1024XEngineer#270 removed the class itself (dead code, superseded by ExpoSystemNotification) -- this barrel lives outside that PR's scope so the removal didn't reach it until this branch merged adapters-data back in.
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 19, 2026
Code review (PR 1024XEngineer#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.
LUPENGHAN
force-pushed
the
feature/reminder-adapters-data
branch
2 times, most recently
from
August 19, 2026 06:54
4f6038c to
eb225aa
Compare
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 19, 2026
Code review (PR 1024XEngineer#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.
LUPENGHAN
marked this pull request as ready for review
August 19, 2026 06:56
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
The adapter implementations and repository-level tests cover the SQLite behavior, but the application composition still selects the in-memory ports. As a result, the shipped reminder runtime does not read persisted schedules or persist runtime state; the new adapters are unreachable from the production app. Focused Vitest execution was unavailable in this workspace because the local dependency executable is not runnable/missing, so the conclusion is based on fixed-diff and lifecycle tracing.
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).
This was referenced Aug 19, 2026
This was referenced Aug 19, 2026
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).
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.
Code review (PR 1024XEngineer#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.
…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.
LUPENGHAN
force-pushed
the
feature/reminder-adapters-data
branch
from
August 20, 2026 07:05
5bfc24d to
0b4d894
Compare
MeteorsLiu
approved these changes
Aug 20, 2026
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 20, 2026
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.
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 20, 2026
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.
gac0812
pushed a commit
that referenced
this pull request
Aug 20, 2026
…271) * 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. * feat(reminder): wire real engine into the app, drop remaining mocks 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. * fix(reminder): permission flow stalled after the first prompt 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. * fix(voice): remove stale push-to-talk listeners 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 #245, which this stacked series replaces -- the fix is not reminder-scoped so none of #264-#270 picked it up. * fix(voice): preserve new TTS after interruption 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. #297 removed only the user-facing interrupt button; backend barge-in still drives voice.tts.canceled, so this path is live. Carried over from #245, which this stacked series replaces -- not reminder-scoped, so none of #264-#270 picked it up. * fix(reminder): reset permission-prompt state when the effect unmounts Code review (PR #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. * test(reminder,voice): close Codecov patch-coverage gaps 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
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
依赖 #266(先合)、#267、#268、#269(先合)。跟前三个适配器 PR 改动内容互相独立,可以并行审;
但四个适配器 PR 都改
createAppServices.ts,需按 #267→#268→#269→#270 顺序依次合并,合并前会 rebase 到上一个已合并 PR 的 main。
改动
SqliteLocalScheduleReader(实现schedules端口)/SqliteReminderStateStore(实现state端口):读写真实本地数据库(ScheduleLocalRepository),替换内存态 fixture;两者都不在构造时直接拿
repository/accountId,而是延迟绑定——由调用方在数据库打开、账号确定后调用
attach(repository, accountId),账号切换或登出时调用detach()。应用生命周期内各自只有一个实例
geofence_radius_meters暂时硬编码DEFAULT_GEOFENCE_RADIUS_METERS(200 米),不接可配置项
InMemoryLocalScheduleReader:同一个schedules端口的非持久化替代实现(用于不需要真实数据库的场景,比如测试)
LocalScheduleWriter:构造函数新增scheduleReader参数,语音写入成功(
applyCommandResult)后主动调用scheduleReader.refresh()——提醒引擎读的是SqliteLocalScheduleReader的投影,不是仓储本身,写完不主动刷新的话新建/改动的日程要等下次rebuild()才会被提醒引擎看到createAppServices.ts的schedules/state换成真实的SqliteLocalScheduleReader/SqliteReminderStateStore,两个实例额外暴露在AppServices上(schedules/reminderState)供上层绑定账号AppRoot.tsx:AuthenticatedScheduleRoute新增一个useEffect,账号认证成功、ScheduleLocalRepository就绪(currentLoadState.status === 'ready')后调用reminderState.attach()/scheduleReader.attach()绑定当前账号并触发一次scheduleReader.refresh();账号切换/组件卸载时调用对应detach()。提醒运行时在鉴权成功后立即启动,此时 SQLite 可能还没就绪,这是为什么读写端口要延迟绑定而不是构造时直接拿
LocalScheduleWriter的构造调用点相应传入scheduleReaderMockLocalScheduleReader、MockReminderApplication、MockReminderDispositionSync、MockReminderStateStore、mockReminderSchedules;删除未使用的LocalSystemNotification占位(review 中发现的死代码,见下)
修复/清理(review 中发现)
LocalSystemNotification是一个空操作的SystemNotificationPort占位实现,导出了但整个代码库没有任何地方 import 它——已经被 feat(reminder): add notification, alarm, and dialog device adapters #269 的真实
ExpoSystemNotification完全取代,接线分支(
createAppServices.ts)实际用的也是后者。确认是死代码后直接删除(eb225aa)验证
npx tsc --noEmit、npx eslint .、npx prettier --check .全绿npm run test:CI 全绿(vitest + jest 全过)SqliteLocalScheduleReader/SqliteReminderStateStore的attach/detach/refresh 生命周期、
LocalScheduleWriter写入后触发刷新的链路、AppRoot.tsx账号就绪后绑定/账号切换后解绑的接线;localReminderAdapters.test.ts这一版集成测试后来改写为 Jest 单测
LocalReminderDataAdapters.test.ts(同样覆盖范围,换到项目主测试框架下)
本轮不含(见 #263 Out of Scope)
geofence_radius_meters硬编码 200 米,不做成可配置