feat(reminder): add audio playback and interval time adapters - #267
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-adapters-audio
branch
from
August 17, 2026 05:22
26115c8 to
d7c75ae
Compare
LUPENGHAN
force-pushed
the
feature/reminder-adapters-audio
branch
from
August 17, 2026 07:00
d7c75ae to
cb2e422
Compare
LUPENGHAN
force-pushed
the
feature/reminder-adapters-audio
branch
from
August 17, 2026 07:25
cb2e422 to
e3ffc03
Compare
LUPENGHAN
force-pushed
the
feature/reminder-adapters-audio
branch
from
August 17, 2026 07:51
e3ffc03 to
b714e49
Compare
LUPENGHAN
added a commit
to LUPENGHAN/timeflow
that referenced
this pull request
Aug 17, 2026
Code review (PR 1024XEngineer#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.
Part of 1024XEngineer#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.
Code review (PR 1024XEngineer#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.
LUPENGHAN
force-pushed
the
feature/reminder-adapters-audio
branch
from
August 19, 2026 06:14
1102497 to
d973f40
Compare
LUPENGHAN
marked this pull request as ready for review
August 19, 2026 06:15
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.
审查结论
音频播放适配器、Data URI 编码和 expo-audio@57.0.3 的调用契约已核对；时间适配器本身也能创建和清理周期计时器。但生产组合根仍注入不会产生 tick 的 Mock，因此前台运行时的时间观测链路尚未真正接通。
验证：检查固定范围 0e378c5...d973f40 的完整 diff、AppRoot/AppProviders 生命周期调用和关联 Issue #263 的接线要求；未修改代码。
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 1024XEngineer#267, fennoai bot).
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%.
CI format:check failure after e718dcf.
Wintercom
reviewed
Aug 19, 2026
Collaborator
|
【Issue 验收与生产接线 / 阻塞合并】PR 已将 |
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 1024XEngineer#267, Wintercom).
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).
Contributor
Author
|
组合根换成了真的 LocalReminderApplication(#266)。这个 PR 自己不拥有的端口(schedules/location/通知/state/dispositionSync)还是原来的 Mock,没动——LocalReminderApplication 是照着 ReminderApplicationDependencies 这个接口工作的,哪边端口是真的哪边是 Mock 不影响它,跟 #270 那边同样的换法一致。补了一个断言:services.runtime.start() 之后 reminderPorts.time.start() 真的被调用了,证明现在接的是会消费这些端口的引擎,不是一个 start()/handleTime() 全是空操作的桩。 |
Collaborator
Wintercom
requested changes
Aug 19, 2026
Wintercom
left a comment
Wintercom
left a comment
Collaborator
There was a problem hiding this comment.
按照 git_rules.txt 的合并标准，本轮需要先处理以下阻塞项：
setAudioModeAsync()失败时本次音频仍返回played: true，导致提醒应用不会触发 fallback，可能静音。- 虽然
IntervalTimeListener已接入组合根，但createAppServices()仍构造MockReminderApplication，生产提醒流程不会消费该时间适配器或ExpoAudioPlayback；#263 的真实引擎接线仍未完成。 - PR 未设置 Milestone，关联 #263 在 MS3；且依赖 #266 的合并状态与独立验收边界需要在 PR 中明确。
具体问题已使用 gh 逐条评论。
Contributor
Author
|
三条都处理了:
|
This was referenced Aug 19, 2026
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
依赖 #266（已合并）；
改动
ExpoAudioPlayback（实现ReminderDeliveryPort的音频部分）+audioDataUri工具函数；构造函数留了一个loadExpoAudioModule注入口子，测试用假实现绕开expo-audio这个原生模块IntervalTimeListener（用setInterval实现时间端口）MockAudioPlaybackcreateAppServices()里time/audio换成这个 PR 提供的真实实现，reminder换成真的LocalReminderApplication（#266），不再是全空操作的MockReminderApplication——否则接进reminderPorts的这两个适配器不会被任何东西调用。其它不属于这个 PR 的端口（schedules/location/通知/state/dispositionSync）仍然是 Mock，见下面"验收边界"修复（review 中发现）
ensureAudioMode()原来在setAudioModeAsync()失败时用.catch()把 rejected promise 变成 resolved，调用方完全不知道 mode 设置失败了，照样播放并报played: true——静音模式/后台播放没配置成功时会误报成功，LocalReminderApplication不会触发 fallback，提醒可能静音。现在ensureAudioMode()返回boolean，失败时上报played: false；失败仍然只影响这一次，下次播放会重新尝试 mode setup验证
npx tsc --noEmit、npx eslint .、npx prettier --check .全绿（#266已合并，组合根接线补完后这个 PR 自己能独立跑通，不用等#271）npm run test：vitest 62/62、jest 369/369ExpoAudioPlayback（含 mode 设置失败的 fallback 路径）、audioDataUri、IntervalTimeListener，以及一个组合根集成测试断言runtime.start()真的会调用到reminderPorts.time.start()验收边界
这个 PR 保证的是：音频/时间两个适配器本身逻辑正确，且组合根用的是会真正消费它们的引擎（
LocalReminderApplication），不是死代码。不保证生产提醒流程完整闭环——schedules/location/通知渠道/状态存储/disposition 同步这几个端口还是 Mock，日程数据、地理围栏、系统通知这几条链路要等#268/#269/#270/#271落地才会全部接上真实实现。本轮不含（见 #263 Out of Scope）
#268/#269/#270）#271）