perf(#257): elapsed を Presenter 側で補間し helper Timer を 3 秒に延長#264
Conversation
PlaybackPosition Entity を rawElapsed + timestamp + playbackRate の純粋スナップショットに変更し、 LyricsPresenter.updateActiveLineTick で `\.date.now` を使って毎フレーム補間する。 これにより helper の polling 間隔を 1s → 3s に延長しても歌詞ハイライトのズレは出ない (DisplayLink tick 毎に最新の補間値が計算されるため)。 #263 で swift-interpret モードに戻して helper のホストが swift-frontend 常駐になった 文脈で、polling 削減の idle CPU 効果が以前より大きい。pause/seek は MediaRemote の notification (kMRMediaRemoteNowPlayingInfoDidChange) で即時反映される。
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughPlaybackPosition now includes snapshot fields ( ChangesPlayback Position Interpolation for Lyric Sync
Sequence DiagramsequenceDiagram
participant MediaRemote
participant TrackInteractor
participant LyricsPresenter
participant LyricsView
MediaRemote->>TrackInteractor: NowPlaying(rawElapsed, timestamp?, playbackRate) (periodic / notification)
TrackInteractor->>LyricsPresenter: PlaybackPosition(rawElapsed, timestamp?, playbackRate)
activate LyricsPresenter
LyricsPresenter->>LyricsPresenter: store latestRawElapsed & latestTimestamp
LyricsPresenter->>LyricsPresenter: interpolatedElapsed = rawElapsed + playbackRate * (now - timestamp)
LyricsPresenter->>LyricsView: update activeLineIndex
MediaRemote->>TrackInteractor: kMRMediaRemoteNowPlayingInfoDidChange (pause/seek)
TrackInteractor->>LyricsPresenter: immediate PlaybackPosition update
deactivate LyricsPresenter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR reduces idle cost of the MediaRemote helper by extending its polling interval while preserving (and improving) lyric highlight accuracy by interpolating elapsed time on the client side (Presenter) using (rawElapsed, timestamp, playbackRate) snapshots.
Changes:
- Change
PlaybackPositionto a snapshot model (rawElapsed,timestamp,playbackRate) and pass it through fromTrackInteractorImpl. - Interpolate elapsed every DisplayLink tick in
LyricsPresenterusing an injecteddate.now. - Extend
media-remote-helper.swiftTimer polling from 1s to 3s and add presenter tests for interpolation + timestamp-nil fallback; bump version to2.13.7.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/PresentersTests/LyricsPresenterTests.swift | Updates existing tests to rawElapsed and adds new tests validating interpolation + nil-timestamp fallback. |
| Tests/PresentersTests/LyricsPresenterDuplicateTests.swift | Updates playback position construction to use rawElapsed. |
| Sources/VersionHandler/Resources/version.txt | Bumps patch version to 2.13.7. |
| Sources/TrackInteractor/TrackInteractorImpl.swift | Stops pre-interpolating elapsed in the interactor; publishes raw snapshot fields. |
| Sources/Presenters/Track/LyricsPresenter.swift | Stores latest snapshot and interpolates elapsed per tick using @Dependency(\.date.now). |
| Sources/MediaRemoteDataSource/Resources/media-remote-helper.swift | Extends polling interval to 3s and documents snapshot approach. |
| Sources/Entity/PlaybackPosition.swift | Renames elapsed → rawElapsed and adds timestamp to support client interpolation. |
| Sources/AppRouter/AppDependencyBootstrap.swift | Updates UI-test dependency bootstrap to the new PlaybackPosition initializer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Periodic fallback for snapshot refresh (rawElapsed/timestamp/playbackRate). | ||
| // The client (LyricsPresenter) interpolates elapsed on every DisplayLink tick | ||
| // from this snapshot, so 3s polling is sufficient for lyric sync. pause/seek | ||
| // is delivered immediately via `kMRMediaRemoteNowPlayingInfoDidChange`. |
There was a problem hiding this comment.
ごの通りです。コメント内の notification 名を
kMRMediaRemoteNowPlayingInfoDidChangeNotification にしました (2b7f7d9)。PR body の同じ箇所もあわせて
しています。
| try? await Task.sleep(for: .milliseconds(50)) | ||
|
|
||
| presenter.updateActiveLineTick() | ||
| #expect(presenter.activeLineIndex == 1) |
There was a problem hiding this comment.
deadline polling パターン (3s) に置き換えました (2b7f7d9)。updateActiveLineTick を内側で繰り返し呼び activeLineIndex == 1 で break する形に揃え、CLAUDE.md "Async test timing" のガイドラインに
しました。
| try? await Task.sleep(for: .milliseconds(50)) | ||
|
|
||
| presenter.updateActiveLineTick() | ||
| #expect(presenter.activeLineIndex == 1) |
There was a problem hiding this comment.
deadline polling パターン (3s) に置き換えました (2b7f7d9)。updateActiveLineTick を内側で繰り返し呼び activeLineIndex == 1 で break する形に揃え、CLAUDE.md "Async test timing" のガイドラインに
しました。
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 090761dc7c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The client (LyricsPresenter) interpolates elapsed on every DisplayLink tick | ||
| // from this snapshot, so 3s polling is sufficient for lyric sync. pause/seek | ||
| // is delivered immediately via `kMRMediaRemoteNowPlayingInfoDidChange`. | ||
| Timer.scheduledTimer(withTimeInterval: 3.0, repeats: true) { _ in fetchAndPrint() } |
There was a problem hiding this comment.
With MediaRemote sources that omit kMRMediaRemoteNowPlayingInfoTimestamp, the presenter explicitly falls back to rawElapsed without interpolation (LyricsPresenter.interpolatedElapsed returns base when the timestamp is nil), so increasing the helper fallback to 3 seconds makes lyric highlighting update only once every 3 seconds for those sources. The previous 1-second fallback was the only mechanism advancing elapsed time in this scenario; either keep a shorter interval for timestamp-less snapshots or synthesize/update a timestamp before relying on the 3-second poll.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
指摘の懸念に対し、helper 側で fetch 時刻を timestamp としてする形に修正しました (2b7f7d9)。
kMRMediaRemoteNowPlayingInfoTimestamp がした snapshot では
Date() を代替値として使用し、client 側の補間が常に有効になるようにしています。これにより timestamp-less ソースでも 3 秒 polling 間隔のままフレーム単位精度をできます。
playbackRate == 0 (pause) 時は LyricsPresenter 側で補間自体をする (
updateActiveLineTick のガード) ため、停止中に誤って前進する副作用はありません。
| rawElapsed: 0, | ||
| timestamp: fixedNow.addingTimeInterval(-7), | ||
| playbackRate: 1.0)) | ||
| try? await Task.sleep(for: .milliseconds(50)) |
There was a problem hiding this comment.
This newly added test waits for Combine delivery with a fixed Task.sleep, and the repo's root AGENTS.md testing rules explicitly say not to wait on async state with fixed sleeps. On a slower or busier CI worker the publisher delivery can exceed 50 ms and make the assertion flaky; poll the expected presenter state until a deadline as the surrounding tests already do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
deadline polling パターン (3s) に置き換えました (2b7f7d9)。updateActiveLineTick を内側で繰り返し呼び activeLineIndex == 1 で break する形に揃え、CLAUDE.md "Async test timing" のガイドラインに
しました。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/VersionHandler/Resources/version.txt (1)
1-2:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove unintended trailing
2from version resource.The file now contains two lines; many version readers expect exactly one SemVer string and may fail or read an invalid value.
Suggested fix
2.13.7 -2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/VersionHandler/Resources/version.txt` around lines 1 - 2, The version resource currently has an unintended extra line containing "2"; edit version.txt so it contains exactly one line with the SemVer string "2.13.7" (remove the stray second-line "2" and any trailing blank lines) so version readers will parse a single valid version value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/PresentersTests/LyricsPresenterTests.swift`:
- Around line 268-313: Replace the racy try? await Task.sleep(...) calls in the
LyricsPresenterTests (the interpolation test block and the
fallsBackWhenTimestampMissing test) with a poll-until-deadline loop that
repeatedly checks for the expected state instead of sleeping: after sending the
PlaybackPosition to the positionSubject, poll until presenter.activeLineIndex ==
1 (or until a short deadline like 500ms) before calling
presenter.updateActiveLineTick(), failing the test if the deadline is exceeded;
update both occurrences that currently call Task.sleep so they mirror the
existing wait/poll pattern used elsewhere (you can reuse the same polling helper
used by waitForLyricsSuccess) to ensure the Combine sink has delivered the
position.
---
Outside diff comments:
In `@Sources/VersionHandler/Resources/version.txt`:
- Around line 1-2: The version resource currently has an unintended extra line
containing "2"; edit version.txt so it contains exactly one line with the SemVer
string "2.13.7" (remove the stray second-line "2" and any trailing blank lines)
so version readers will parse a single valid version value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 835d71f2-d7f3-4653-b6c1-a8eaf2a4cf09
📒 Files selected for processing (8)
Sources/AppRouter/AppDependencyBootstrap.swiftSources/Entity/PlaybackPosition.swiftSources/MediaRemoteDataSource/Resources/media-remote-helper.swiftSources/Presenters/Track/LyricsPresenter.swiftSources/TrackInteractor/TrackInteractorImpl.swiftSources/VersionHandler/Resources/version.txtTests/PresentersTests/LyricsPresenterDuplicateTests.swiftTests/PresentersTests/LyricsPresenterTests.swift
Codex P2 / Copilot レビュー指摘への対応。 - helper: MediaRemote が timestamp を返さないソース向けに、fetch 時刻 を timestamp として注入。これがないと client 側の補間が base (rawElapsed そのもの) に縮退し、3 秒 polling 間隔のまま歌詞 ハイライトが追従しなくなる。playbackRate=0 (pause) では補間自体を スキップするため副作用なし。 - helper: コメント内の notification 名を kMRMediaRemoteNowPlayingInfoDidChange → kMRMediaRemoteNowPlayingInfoDidChangeNotification に修正 (Apple 公式の正式名にあわせる)。 - tests: 新規追加した 2 件 (interpolatesFromSnapshot / fallsBackWhenTimestampMissing) で使っていた固定 Task.sleep(50ms) を deadline polling (3s) に置換。CLAUDE.md "Async test timing" の ガイドラインに準拠し、CI 負荷で flaky 化するリスクを除去。
|
@coderabbitai Outside-diff の
|
|
(´･ω･`) ＼(ﾟヮﾟ)/ 誤検出スミマセン！ ✏️ Learnings added
✅ Actions performedComments resolved. Approval is disabled; enable |
本 PR で変更した TrackInteractorImpl.playbackPosition publisher (lines 64-69) がテストでカバーされておらず Codecov patch coverage が 失敗していたため、専用テスト suite を追加。 検証内容: - rawElapsed / timestamp / playbackRate を NowPlaying からそのまま PlaybackPosition に passthrough すること - nil rawElapsed / nil timestamp / rate=0 (pause) でも例外なく流れること - NowPlaying が連続して変化した際に複数 snapshot を emit すること 既存の TrackInteractorArtworkTests と同じパターン (Stub + Collector + makeInteractor helper) に揃え、suite は .serialized で安定性を担保。
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/TrackInteractorTests/TrackInteractorPlaybackPositionTests.swift`:
- Around line 148-153: The test indexes snapshots[0] and snapshots[1] after a
non-fatal check (`#expect`(snapshots.count == 2)), which can still crash; change
that to a hard precondition so failures are diagnostic: replace the non-fatal
expect with a hard requirement (for example use `#require`(snapshots.count == 2)
or a guard like guard snapshots.count >= 2 else { XCTFail(...); return }) before
accessing snapshots[0] and snapshots[1] in TrackInteractorPlaybackPositionTests
(use the local variables collector and snapshots to locate the code).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3ddaf23-6a74-4771-b3ee-d8263bdb740c
📒 Files selected for processing (3)
Sources/MediaRemoteDataSource/Resources/media-remote-helper.swiftTests/PresentersTests/LyricsPresenterTests.swiftTests/TrackInteractorTests/TrackInteractorPlaybackPositionTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/PresentersTests/LyricsPresenterTests.swift
PR の主目的 (line 64-69 のカバー) を超えるが、関連する周辺コードの 未カバー領域もまとめて潰す。残り 4 lines は cancellation race (`Task.isCancelled`) の経路で時序的に強制できないため対象外。 追加内容: - volume-mute pattern (line 30) TrackInteractorPlaybackPositionTests に test 追加。 「同一 title で artist が消失 → 抑制」を 3 つの emission 列で検証。 - computed style properties (lines 84-94) TrackInteractorStyleTests を新規追加。 decodeEffectConfig / textLayout / artworkStyle の 3 つを 非デフォルト・デフォルト両方で検証。 - nil title/artist guard (line 123) TrackInteractorRaceTests に test 追加。 resolveTrack の guard で Just(loading) に短絡することと、 以後 resolved/notFound が emit されないことを検証。 結果: - lines coverage 92.94% → 97.49% (+4.55pt) - function coverage 71.05% → 81.58% - region coverage 66.67% → 75.76%
| // Send a paused playback position (rate = 0) | ||
| positionSubject.send(PlaybackPosition(elapsed: 6, playbackRate: 0)) | ||
| positionSubject.send(PlaybackPosition(rawElapsed: 6, playbackRate: 0)) | ||
| // Allow Combine to deliver the position update | ||
| try? await Task.sleep(for: .milliseconds(50)) | ||
|
|
There was a problem hiding this comment.
deadline polling にしました (78acf8b)。1 秒間
updateActiveLineTick を polling し、activeLineIndex が nil 以外に flip したら早期 break する構造に変更したため、paused-guard の破れがあれば即座にできます。
| // Send position at 6s — should highlight Line B (time=5) | ||
| positionSubject.send(PlaybackPosition(elapsed: 6, playbackRate: 1.0)) | ||
| positionSubject.send(PlaybackPosition(rawElapsed: 6, playbackRate: 1.0)) | ||
| // Allow Combine to deliver the position update | ||
| try? await Task.sleep(for: .milliseconds(50)) | ||
|
|
There was a problem hiding this comment.
deadline polling に置換しました (78acf8b)。3 秒以内に activeLineIndex == 1 にすれば即 break する形で、Combine
の遅延に強くしています。
| // Set position while playing | ||
| positionSubject.send(PlaybackPosition(elapsed: 6.0, playbackRate: 1.0)) | ||
| positionSubject.send(PlaybackPosition(rawElapsed: 6.0, playbackRate: 1.0)) | ||
| try? await Task.sleep(for: .milliseconds(200)) | ||
| presenter.updateActiveLineTick() | ||
| #expect(presenter.activeLineIndex == 1) | ||
|
|
||
| // Pause (rate = 0), send new position | ||
| positionSubject.send(PlaybackPosition(elapsed: 6.0, playbackRate: 0)) | ||
| positionSubject.send(PlaybackPosition(rawElapsed: 6.0, playbackRate: 0)) | ||
| try? await Task.sleep(for: .milliseconds(200)) |
There was a problem hiding this comment.
200ms 固定 sleep を 2 箇所とも deadline polling に置換しました (78acf8b)。rate=1.0 の活性化は 3 秒 polling で activeLineIndex == 1 到達まで待ち、rate=0 後は 1 秒 polling で「逸脱したら早期 break」する形に変更。paused-guard のを fast-fail で
できる構造になりました。
…re に強化 CodeRabbit / Copilot レビュー指摘への対応。 ### Copilot: 固定 Task.sleep 撤廃 (CLAUDE.md "Async test timing" 準拠) 3 箇所で残っていた固定 sleep を deadline polling に置換: - LyricsPresenterTests `skips update when playback rate is 0 (paused)` Task.sleep(50ms) → 1s deadline で activeLineIndex==nil を polling 観測 - LyricsPresenterTests `updates active line index for timed lyrics` Task.sleep(50ms) → 3s deadline で activeLineIndex==1 になるまで polling - LyricsPresenterDuplicateTests `pausedPlaybackKeepsIndex` Task.sleep(200ms) × 2 → それぞれ 3s / 1s deadline polling に。 rate=1.0 配信は activeLineIndex==1 になるまで、rate=0 後は逸脱したら early break する形で「paused-guard 破れ」を高速検知できるように。 ### CodeRabbit: 配列 index アクセス前に #require で固める snapshots[0] / snapshots[1] アクセスは #expect(count == N) だけでは non-fatal で素通りし、count が想定より小さいと out-of-bounds で test runner ごと落ちる可能性。 try #require に強化: - TrackInteractorPlaybackPositionTests.emitsMultipleSnapshots (count==2) - TrackInteractorPlaybackPositionTests.volumeMutePatternSuppressed (count==2)
Closes #257
背景
#263 で
に巻き戻したため、helper のホストプロセスは 
バイナリ × 高頻度」から「interpreter ホスト × 高頻度」に変化し、idle 時の常駐コストが以前より重くなった。
media-remote-helper.swiftを swiftc 事前コンパイルから swift interpretswift-frontend -interpret(Apple 署名) の常駐となった。これにより 1 秒毎の polling Timer のコストが「MediaRemote が返す
timestampとplaybackRateから elapsed は client 側で完全に補間できる:つまり polling 間隔を延ばしても、補間さえ正しく行えば歌詞
に支障は出ない。pause/seek は MediaRemote の 
されるため、Timer は「補間の基準点を再取得する役」だけに専念できる。
kMRMediaRemoteNowPlayingInfoDidChangeNotificationnotification でEntity / Interactor
PlaybackPositionを(elapsed, playbackRate)から純粋スナップショット(rawElapsed, timestamp, playbackRate)にTrackInteractorImpl.playbackPositionは補間せず raw データを passthrough するだけにPresenter
LyricsPresenterが@Dependency(\.date.now)を持つlatestRawElapsed/latestTimestamp/latestPlaybackRate) を保持updateActiveLineTick(DisplayLink 60fps) で毎回base + rate * (now - timestamp)を計算Helper
Timer.scheduledTimerの間隔を1.0→3.0秒に延長保持
PlaybackUseCase.elapsedTime(for:)はTrackHandler(CLIlyra track) で使われているため温存interpolates elapsed from snapshot (rawElapsed + timestamp + playbackRate)\.date.nowを固定値に override し、7 秒前の timestamp でrawElapsed=0, rate=1.0を送るとupdateActiveLineTickでelapsed=7扱いになり Line B (time=5) が highlight されるfalls back to rawElapsed when timestamp is nilelapsed:→rawElapsed:にトレードオフ
歌詞精度はむしろ 向上 している (補間が tick 毎に効くため)。
Summary by CodeRabbit
Chores
Refactor
Tests