Skip to content

feat(reminder): add interval time, expo-audio and vibration adapters - #215

Closed
gac0812 wants to merge 17 commits into
1024XEngineer:mainfrom
gac0812:feat/reminder-playback-adapters
Closed

feat(reminder): add interval time, expo-audio and vibration adapters#215
gac0812 wants to merge 17 commits into
1024XEngineer:mainfrom
gac0812:feat/reminder-playback-adapters

Conversation

@gac0812

@gac0812 gac0812 commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Merge Order

Scope

  • frontend/src/shared/time/IntervalTimeListener.ts
  • frontend/src/shared/time/index.ts
  • frontend/src/infrastructure/audio/ExpoAudioPlayback.ts
  • frontend/src/infrastructure/audio/audioDataUri.ts
  • frontend/src/infrastructure/audio/index.ts
  • frontend/src/infrastructure/notifications/ReactNativeVibration.ts
  • frontend/src/infrastructure/notifications/index.ts
  • frontend/package.json / frontend/package-lock.json
  • frontend/src/types/expo-audio.d.ts
  • frontend/tsconfig.json
  • frontend/tests/unit/shared/time/intervalTimeListener.test.ts
  • frontend/tests/unit/infrastructure/audio/audioDataUri.test.ts
  • frontend/tests/unit/infrastructure/audio/expoAudioPlayback.test.ts
  • frontend/tests/unit/infrastructure/notifications/reactNativeVibration.test.ts
  • 仅适配器类与单测;不改生产提醒运行时接线

Out of scope

Verification

  • npm run check
  • npm ci

Related to #214

mac added 3 commits August 12, 2026 15:30
Introduce TimeflowAlarmBridge and NativeAlarmScheduler, and wire the
composition root alarms port to the native Android scheduler.
Introduce NativeDeviceCapability over TimeflowAlarmBridge and a launch
permission prompt hook, and wire the composition root device port.
Wire IntervalTimeListener, ExpoAudioPlayback, and ReactNativeVibration
into the composition root, and add the expo-audio dependency.
@vercel

vercel Bot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
timeflow Ready Ready Preview Aug 14, 2026 2:02am

mac added 2 commits August 13, 2026 10:48
Return AlarmScheduleReceipt.scheduled, map native schedule rejections to
unscheduled, and propagate the native cancel boolean instead of always
reporting success.
…e-alarm

Keep NativeAlarmScheduler wiring while adopting the auth runtime composition from 1024XEngineer#218.
…evice-permissions

Keep NativeDeviceCapability wiring while adopting the auth runtime from 1024XEngineer#218.
…nder-playback-adapters

Keep playback adapters while adopting the auth runtime from 1024XEngineer#218.
…ack-adapters

Keep IntervalTimeListener, ExpoAudioPlayback, and ReactNativeVibration wiring while adopting 1024XEngineer#208 device-capability fixes and expo-location from main.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one integration issue in the current diff: the new production adapters are injected, but the composition root still constructs the mock reminder application, so the adapters are not reached by the running reminder flow. The review comment identifies the concrete impact and the required integration point.

Verification: repository was locked to the requested base/head SHAs. Local validation could not run because the mounted frontend/node_modules is incomplete (tsc is missing/permission-denied).

Comment thread frontend/src/app/composition/createAppServices.ts Outdated
… switch

MockReminderApplication never invokes time/audio/vibration, so wiring production adapters there had no runtime effect.
@gac0812

gac0812 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

@fennoai /review

@fennoai

fennoai Bot commented Aug 13, 2026

Copy link
Copy Markdown

Review Complete

  • Inspected the fixed ac37def20a925c28b9927ed124763ff002c72332...833a520f6915da6a1ab569cc3b762cfaf5219f23 diff
  • Completed code quality, performance, security, and documentation passes
  • Ran focused adapter tests, typecheck, lint, and full npm run check

No noteworthy issues found. The non-blocking PR review was submitted with the same conclusion.

Verification: npm ci, 4 focused Jest suites (13 tests), and full npm run check (31 Jest suites / 248 tests plus 5 Vitest files / 47 tests) passed. npm ci reported the repository’s existing npm engine mismatch warning (npm 11.16.0 vs required >=10.8.2 <11) and audit findings; neither affected verification.

View job run

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found in this review. The changed adapters and tests are consistent with the current reminder application wiring.

@Wintercom

Copy link
Copy Markdown
Collaborator

【Issue 验收不一致 / 阻塞合并】PR 使用 Closes #214,但 Issue #214 的明确验收项是“组合根 time / audio / vibration 切换为上述适配器”;当前最新提交 833a520 又把三者保留为 mock,PR 的 Out of scope 也写明生产接线不在本次范围。因此本 PR 合并后 Issue 的核心目标仍未交付,不应关闭。请改为 Part of #214 并保持 Issue 开放,或在本 PR 完成 LocalReminderApplication 的真实组合根接线与集成测试后再使用 Closes #214。这也是 git_rules.txt 第 20–25 行要求的 Issue 范围一致与功能可用。

Comment thread frontend/src/infrastructure/audio/ExpoAudioPlayback.ts Outdated
@Wintercom

Copy link
Copy Markdown
Collaborator

【PR 描述需同步最终实现】Summary 仍写“新增适配器”,但早期描述/合并顺序容易让 reviewer 误以为它们已接入运行时;最新代码实际上只新增未接线的适配器与单测。git_rules.txt 第 24–25 行要求作者讲清主要改动、为什么这样改。请明确写出当前产物“不可从生产提醒流程到达”,说明后续由哪个 Issue/PR 完成接线,并将 Verification 区分为适配器单测与端到端真机行为(本 PR 未验证)。

@Wintercom Wintercom left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

按照 git_rules.txt 的合并标准,本轮需要先处理以下阻塞项:

  1. PR 使用 Closes #214,但最新实现明确未把 time / audio / vibration 接入组合根,与 Issue #214 的核心验收项不一致;请改为阶段性交付并保持 Issue 开放,或补齐真实接线与集成测试。
  2. ExpoAudioPlayback 会吞掉 audio mode 设置失败,并在没有确认实际播放状态时返回 played: true;player 创建/replace/play 失败也无法稳定触发本地 fallback。
  3. PR 描述需要明确当前适配器不可从生产提醒流程到达,并指出后续接线 Issue/PR 与本次验证边界。

具体内容已使用 gh 逐条评论。此前 reviewer 关于 mock 组合根导致生产适配器不可达的问题虽已通过撤销接线规避,但由此产生的 Issue 验收不一致仍需处理。

Wait for expo-audio playing status (or a timeout/error) before returning played: true, and treat mode/replace/play exceptions as unplayed.
@gac0812

gac0812 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

@fennoai /review

@fennoai

fennoai Bot commented Aug 14, 2026

Copy link
Copy Markdown

Review Complete

  • Inspected the fixed ac37def20a925c28b9927ed124763ff002c72332...56205773c5fb61956c3dc507f6309987146b3ac6 diff and incremental commit
  • Completed code quality, performance, security, and documentation passes
  • Ran focused playback tests, typecheck, lint, and full npm run check

Found one P2 cleanup issue: when native play() throws, the new confirmation timer/listener remains active for up to two seconds. The focused suite reproduces this with a Jest open-handle warning; the inline review comment contains the remediation direction.

Verification: focused playback suite passed (10 tests); full npm run check passed (5 Vitest files / 47 tests and 31 Jest suites / 253 tests).

View job run

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one issue in the updated playback confirmation path; see the inline comment. All other review passes found no noteworthy issues.

Comment thread frontend/src/infrastructure/audio/ExpoAudioPlayback.ts
@gac0812

gac0812 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

已处理 Wintercom 本轮阻塞项:

  1. Issue 范围:正文已改为 Related to #214(仓库约定里中间 PR 用该关键词,效果等同「Part of」,合并后 不会 关闭 Issue)。本 PR 只交付适配器与单测;组合根切换见 feat(reminder): wire LocalReminderApplication with in-memory adapters #238,生产 time / audio / vibration 接线仍留在 feat(reminder): 接入时间/音频/震动原生播放适配器 #214
  2. 播放假成功:已在 5620577 修复——mode / replace / play 失败或 2s 内未进入 playing 均返回 played: false
  3. PR 描述:Summary / Out of scope 已写明适配器 不可从生产提醒流程到达,并区分本 PR 只跑适配器单测(npm run check),未做真机端到端验证。

The confirmation timer and status listener must be torn down on the play() catch path so failed playback does not leak a 2s handle.
@gac0812 gac0812 closed this Aug 14, 2026
@gac0812
gac0812 deleted the feat/reminder-playback-adapters branch August 14, 2026 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants