feat(#23): CoreAudio process tap 駆動のスペクトラムアナライザを実装#296
Merged
Conversation
- TrackInteractor.audioSource: shared NowPlaying ストリームから pid/再生状態を派生 - AudioTapDataSource: ProcessTapEngine (macOS 14.4+ API, availability-erased) + swift-atomics SPSC リングバッファ (IOProc は RT-safe) - FrequencyAnalyzer: Hann 窓 → vDSP FFT → dB 正規化 → bar 変換 (純粋・依存なし) - SpectrumInteractor: audioSource を AsyncStream 直列キューで消費しタップ生成/破棄を逐次適用 - SpectrumPresenter: DisplayLink tick で指数減衰マージ、binHeights() は読み取り専用 - SpectrumView: #252/#258 パターン (条件付き include + TimelineView paused) でアイドルコストゼロ - [spectrum] 設定 (bar_count/bar_color/placement/decay_rate 等) と DI 配線 - 排他は NSLock でなく OSAllocatedUnfairLock(state:) を採用 - Info.plist に NSAudioCaptureUsageDescription を追加
- FrequencyAnalyzer: sine ピーク位置 / 無音 / 短入力 / クランプ / fftSize 丸め - SampleRingBuffer: 充填前 empty / 最新窓 / wraparound / 容量超過 / 容量丸め - AudioTapDataSourceImpl: タップなし empty / 不明 pid 失敗 (CI-safe, TCC 非発火) / stop 冪等 - SpectrumInteractorImpl: 再生でタップ開始 / 一時停止で破棄 / アプリ切替 re-tap / disabled 不活性 - SpectrumPresenter: tick の減衰マージ / アニメーションゲート / アイドル不活性 - [spectrum] TOML デコード (デフォルト / 全項目 / グラデーション / 部分指定 / 不正 placement) - 既存 TrackInteractor スタブ 8 箇所に audioSource を追加、テンプレートスナップショット更新
- CLAUDE.md: mermaid グラフ / VIPER・Layer Summary / Key Design Decisions に spectrum 系モジュールを追記 - README: [spectrum] 設定リファレンス、ブラウザ全体タップの既知の制限、macOS 14.4+ 要件
Chromium 系ブラウザは音声出力を専用ヘルパープロセス (例: Arc の Browser Helper) が担うため、メイン pid だけをタップすると無音になる。 kAudioHardwarePropertyProcessObjectList を列挙し ppid ウォークで サブツリーに属するプロセスオブジェクトを全て CATapDescription に渡す。 Arc + 実機検証でバー描画を確認済み。
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR adds a real-time spectrum analyzer overlay: CoreAudio process-tap audio capture ( ChangesSpectrum Analyzer Feature
NowPlayingRepository Multicast Refactor
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant PlaybackUseCase
participant SpectrumInteractorImpl
participant AudioTapDataSourceImpl
participant ProcessTapEngine
participant FrequencyAnalyzer
participant SpectrumPresenter
participant SpectrumView
PlaybackUseCase->>SpectrumInteractorImpl: observeNowPlaying() event (pid, playbackRate)
SpectrumInteractorImpl->>AudioTapDataSourceImpl: startTap(pid)
AudioTapDataSourceImpl->>ProcessTapEngine: create tap/aggregate/IOProc
ProcessTapEngine-->>AudioTapDataSourceImpl: samples via ring buffer
SpectrumPresenter->>SpectrumInteractorImpl: magnitudes()
SpectrumInteractorImpl->>AudioTapDataSourceImpl: latestSamples(count)
SpectrumInteractorImpl->>FrequencyAnalyzer: magnitudes(of: samples)
FrequencyAnalyzer-->>SpectrumInteractorImpl: bar magnitudes
SpectrumInteractorImpl-->>SpectrumPresenter: bar magnitudes
SpectrumPresenter->>SpectrumView: binHeights()
PlaybackUseCase->>SpectrumInteractorImpl: pause / session gone
SpectrumInteractorImpl->>AudioTapDataSourceImpl: stopTap()
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements issue #23 by adding a real-time spectrum analyzer overlay driven by the now-playing app’s audio via a CoreAudio per-process (process-tree) tap, including config/schema support, new modules (tap engine + FFT analyzer), presenter/view wiring, and DI registrations.
Changes:
- Add CoreAudio process-tap capture pipeline (tap → private aggregate device → IOProc → lock-free ring buffer) and an FFT-based
FrequencyAnalyzer. - Introduce
[spectrum]config/style types and integrate spectrum ticking/rendering into the overlay window lifecycle. - Add unit tests for the new analyzer/tap/interactor/presenter behavior and bump version to 2.18.0.
Reviewed changes
Copilot reviewed 43 out of 44 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/ViewsTests/ViewRenderingTests.swift | Update fixtures to satisfy new TrackInteractor.audioSource and pass SpectrumPresenter into overlay view construction |
| Tests/SpectrumInteractorTests/SpectrumInteractorImplTests.swift | New interactor tests for tap lifecycle and magnitudes behavior |
| Tests/PresentersTests/SpectrumPresenterTests.swift | New presenter tests for ticking/decay/animation gating |
| Tests/PresentersTests/LyricsPresenterTests.swift | Update TrackInteractor stub to include audioSource |
| Tests/PresentersTests/LyricsPresenterDuplicateTests.swift | Update TrackInteractor stub to include audioSource |
| Tests/PresentersTests/LyricsPresenterColumnsTests.swift | Update TrackInteractor stub to include audioSource |
| Tests/PresentersTests/HeaderPresenterTests.swift | Update TrackInteractor stub to include audioSource |
| Tests/PresentersTests/HeaderPresenterDuplicateTests.swift | Update TrackInteractor stub to include audioSource |
| Tests/FrequencyAnalyzerTests/FrequencyAnalyzerTests.swift | New FFT/analyzer correctness tests |
| Tests/ConfigDataSourceTests/ConfigTemplateTests.swift | Add [spectrum] to config template expectations |
| Tests/ConfigDataSourceTests/ConfigDecodingTests.swift | Add TOML decoding coverage for [spectrum] |
| Tests/AudioTapDataSourceTests/SampleRingBufferTests.swift | New lock-free ring buffer tests |
| Tests/AudioTapDataSourceTests/AudioTapDataSourceImplTests.swift | New tap datasource tests for inert/unknown-pid/idempotent stop behavior |
| Tests/AppRouterTests/AppLaunchEnvironmentTests.swift | Update window factory signature and fixtures to include SpectrumPresenter |
| Sources/Views/Spectrum/SpectrumView.swift | New SwiftUI Canvas/TimelineView spectrum bar rendering |
| Sources/Views/Overlay/OverlayContentView.swift | Wire SpectrumView into overlay composition |
| Sources/Views/Overlay/AppWindow.swift | Plumb SpectrumPresenter into hosting root view init |
| Sources/VersionHandler/Resources/version.txt | Bump version 2.17.0 → 2.18.0 |
| Sources/TrackInteractor/TrackInteractorImpl.swift | Derive audioSource publisher from shared now-playing stream |
| Sources/SpectrumInteractor/SpectrumInteractorImpl.swift | New interactor managing tap lifecycle + providing magnitudes |
| Sources/Presenters/Spectrum/SpectrumPresenter.swift | New presenter handling per-frame decay/merge and animation gating |
| Sources/FrequencyAnalyzer/FrequencyAnalyzer.swift | New Accelerate/vDSP FFT → bar-magnitude converter |
| Sources/Entity/Style/SpectrumStyle.swift | New resolved spectrum style type |
| Sources/Entity/Style/SpectrumPlacement.swift | New placement enum for spectrum positioning |
| Sources/Entity/Style/AppStyle.swift | Add spectrum style to app style surface |
| Sources/Entity/Config/SpectrumConfig.swift | New [spectrum] config decoding with defaults |
| Sources/Entity/Config/AppConfig.swift | Add spectrum config to app config surface |
| Sources/Entity/AudioSourceState.swift | New entity describing now-playing pid + audibility |
| Sources/Domain/Interactor/TrackInteractor.swift | Add audioSource publisher requirement |
| Sources/Domain/Interactor/SpectrumInteractor.swift | New spectrum interactor protocol + dependency key |
| Sources/Domain/DataSource/AudioTapDataSource.swift | New audio tap datasource protocol + dependency key |
| Sources/DependencyInjection/InteractorRegistration.swift | Register live SpectrumInteractorImpl |
| Sources/DependencyInjection/DataSourceRegistration.swift | Register live AudioTapDataSourceImpl |
| Sources/ConfigRepository/ConfigRepositoryImpl.swift | Map SpectrumConfig → SpectrumStyle |
| Sources/CLI/Info.plist | Add NSAudioCaptureUsageDescription (and bundle identifier is present) |
| Sources/AudioTapDataSource/SampleRingBuffer.swift | New SPSC lock-free float ring buffer using swift-atomics |
| Sources/AudioTapDataSource/ProcessTapEngine.swift | New CoreAudio process-tap + aggregate device + IOProc engine |
| Sources/AudioTapDataSource/AudioTapDataSourceImpl.swift | New live datasource availability-erasing ProcessTapEngine |
| Sources/AppRouter/AppRouter.swift | Create/start/stop spectrum presenter; add per-frame tick handler when enabled |
| Sources/AppRouter/AppDependencyBootstrap.swift | UI-test fixture additions for audioSource |
| README.md | Document [spectrum] config, macOS 14.4+ requirement, and known browser limitation |
| Package.swift | Add new targets/tests + swift-atomics; embed Info.plist via linker flags |
| Package.resolved | Add swift-atomics pin |
| .claude/CLAUDE.md | Update architecture graph/docs to include spectrum components |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+126
to
+133
| var objects = [AudioObjectID]( | ||
| repeating: AudioObjectID(kAudioObjectUnknown), | ||
| count: Int(size) / MemoryLayout<AudioObjectID>.size) | ||
| guard | ||
| AudioObjectGetPropertyData( | ||
| AudioObjectID(kAudioObjectSystemObject), &address, 0, nil, &size, &objects) | ||
| == noErr | ||
| else { return [] } |
Owner
Author
There was a problem hiding this comment.
Swift では &array を C API のポインタ引数へ渡すと、Array 値そのものではなく連続要素ストレージへの一時ポインタに変換されます（inout-to-pointer 変換の配列特例で、AudioObjectGetPropertyData を Swift から呼ぶ際の標準イディオムです）。実際このコードパスでプロセス列挙 → サブツリー照合 → タップ作成まで実機で動作確認済みです。
Comment on lines
+64
to
+76
| let scratch = UnsafeMutablePointer<Float>.allocate(capacity: Self.scratchCapacity) | ||
| scratch.initialize(repeating: 0, count: Self.scratchCapacity) | ||
| self.scratch = scratch | ||
| let capturedRing = ring | ||
| let status = AudioDeviceCreateIOProcIDWithBlock(&ioProcID, aggregateID, nil) { | ||
| _, inInputData, _, _, _ in | ||
| Self.mixDownToMono(inInputData, into: scratch, ring: capturedRing) | ||
| } | ||
| guard status == noErr, let ioProcID, AudioDeviceStart(aggregateID, ioProcID) == noErr | ||
| else { | ||
| rollBack() | ||
| return nil | ||
| } |
Owner
Author
There was a problem hiding this comment.
Swift のクラスは、全 stored property の初期化完了後に failable init が nil を返した場合、deinit を実行します。ProcessTapEngine の失敗分岐はいずれも scratch 確保を含む全プロパティ初期化後にあるため、init? が nil を返しても deinit → scratch?.deallocate() が走り、リークしません。実証プローブ:
final class Probe {
let buffer = UnsafeMutablePointer<Float>.allocate(capacity: 8)
init?(fail: Bool) { guard !fail else { return nil } }
deinit { print("deinit ran"); buffer.deallocate() }
}
_ = Probe(fail: true) // → "deinit ran"
Comment on lines
+55
to
+56
| try? await Task.sleep(for: .milliseconds(50)) | ||
| #expect(harness.tap.startedPids.isEmpty) |
Owner
Author
There was a problem hiding this comment.
固定 sleep を撤去しました（c6491b1）。disabled 時の start() はタスク生成前に同期 return するため、イベントを消費する主体がそもそも存在せず、即時 assert で安全に検証できます。
6 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Sources/AudioTapDataSource/AudioTapDataSourceImpl.swift (1)
27-31: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLock held across CoreAudio tap creation may block
latestSamples.
ProcessTapEngine(pid:ring:)performs the full CoreAudio setup (tap + aggregate + IOProc) and can block on a first-run TCC permission prompt. That work runs while holdingengine's lock, andlatestSamples(Line 44) acquires the same lock, so a per-frame consumer call can stall for as long as the prompt/setup takes. Please confirm which thread invokeslatestSamples; if it is the display/UI path, consider constructing the engine outside the critical section and only publishing the reference under the lock.🤖 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/AudioTapDataSource/AudioTapDataSourceImpl.swift` around lines 27 - 31, Lock contention in AudioTapDataSourceImpl’s engine initialization is blocking latestSamples because ProcessTapEngine(pid:ring:) is created inside the same engine lock. Move the CoreAudio tap/aggregate/IOProc setup out of the critical section, then reacquire the lock only to stop any existing engine and publish the new ProcessTapEngine reference; keep latestSamples on the same lock path but avoid holding it during construction. Verify the call site of latestSamples and, if it can run on the UI/display path, ensure it cannot be stalled by tap creation.Sources/SpectrumInteractor/SpectrumInteractorImpl.swift (1)
21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
Pipelinefields could belet.
Pipelineis only ever constructed and swapped wholesale (state = Pipeline(...)instart(),state = Pipeline()instop()); no code mutates an individual field of an existingPipelinevalue. The three stored properties could beletinstead ofvar.As per coding guidelines,
**/*.swift: "Preferletand functional transforms. Everyvarshould have a real reason to exist."♻️ Suggested change
private struct Pipeline { - var cancellable: AnyCancellable? - var processor: Task<Void, Never>? - var continuation: AsyncStream<AudioSourceState>.Continuation? + let cancellable: AnyCancellable? + let processor: Task<Void, Never>? + let continuation: AsyncStream<AudioSourceState>.Continuation? }🤖 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/SpectrumInteractor/SpectrumInteractorImpl.swift` around lines 21 - 25, The Pipeline stored properties are mutable without being mutated individually, so switch cancellable, processor, and continuation in SpectrumInteractorImpl.Pipeline from var to let. Update the Pipeline struct definition only, since start() and stop() already replace the whole Pipeline value rather than modifying fields in place.Source: Coding guidelines
Sources/Domain/Interactor/SpectrumInteractor.swift (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCombine type in Domain protocol signature.
isCapturing: AnyPublisher<Bool, Never>puts a Combine-specific type in a Domain-layer protocol requirement. ConsiderAsyncStream<Bool>(plain Swift) to keep the Domain module framework-agnostic, consistent with the rest of the file which already uses plain sync functions.This may already be an established pattern elsewhere in Domain (e.g.
TrackInteractor.audioSource), in which case this is consistent rather than a new problem — worth confirming before changing.As per coding guidelines, "Domain module must not import Foundation. Use plain Swift types in protocol signatures, and put concrete data types in Entity."
🤖 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/Domain/Interactor/SpectrumInteractor.swift` at line 11, The SpectrumInteractor protocol currently exposes a Combine-specific AnyPublisher in the Domain layer, which breaks the framework-agnostic contract. Update the isCapturing requirement to use a plain Swift async stream type such as AsyncStream<Bool>, and adjust any conforming implementations and call sites accordingly while keeping the Domain protocol free of Combine imports. Check whether this should follow the same pattern used by other Domain protocols like TrackInteractor.audioSource so the signature stays consistent across the module.Source: Coding guidelines
🤖 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 `@Sources/AudioTapDataSource/ProcessTapEngine.swift`:
- Around line 64-77: The init path in ProcessTapEngine leaks the preallocated
scratch buffer when setup fails because rollBack() only tears down CoreAudio
state and init? can return nil before deinit runs. Update the failure handling
around AudioDeviceCreateIOProcIDWithBlock and AudioDeviceStart so the allocated
scratch pointer is freed before returning nil, either by extending rollBack() to
deallocate self.scratch or by explicitly deallocating it in the failure branch
of init. Use the existing scratch property and rollBack() cleanup path to keep
the teardown consistent.
In `@Sources/ConfigRepository/ConfigRepositoryImpl.swift`:
- Around line 38-50: Clamp the spectrum bar count during config decoding in
ConfigRepositoryImpl before building SpectrumStyle, since barCount can still
flow into SpectrumPresenter.merged(...) unnormalized. Update the SpectrumStyle
initialization so barCount is coerced to at least 1, similar to the existing
fftSize normalization path, and keep the change localized to the config-to-model
mapping.
In `@Tests/SpectrumInteractorTests/SpectrumInteractorImplTests.swift`:
- Around line 49-57: The disabled spectrum test uses a fixed Task.sleep to wait
for no tap start, which should be replaced with the suite’s polling pattern.
Update disabledIsInert in SpectrumInteractorImplTests to use pollUntil with a
deadline and early exit, asserting that harness.tap.startedPids stays empty
after harness.interactor.start() and the AudioSourceState send. Keep the change
localized to the test helper flow used by SpectrumInteractorImplTests so the
absence check is event-driven instead of delay-based.
---
Nitpick comments:
In `@Sources/AudioTapDataSource/AudioTapDataSourceImpl.swift`:
- Around line 27-31: Lock contention in AudioTapDataSourceImpl’s engine
initialization is blocking latestSamples because ProcessTapEngine(pid:ring:) is
created inside the same engine lock. Move the CoreAudio tap/aggregate/IOProc
setup out of the critical section, then reacquire the lock only to stop any
existing engine and publish the new ProcessTapEngine reference; keep
latestSamples on the same lock path but avoid holding it during construction.
Verify the call site of latestSamples and, if it can run on the UI/display path,
ensure it cannot be stalled by tap creation.
In `@Sources/Domain/Interactor/SpectrumInteractor.swift`:
- Line 11: The SpectrumInteractor protocol currently exposes a Combine-specific
AnyPublisher in the Domain layer, which breaks the framework-agnostic contract.
Update the isCapturing requirement to use a plain Swift async stream type such
as AsyncStream<Bool>, and adjust any conforming implementations and call sites
accordingly while keeping the Domain protocol free of Combine imports. Check
whether this should follow the same pattern used by other Domain protocols like
TrackInteractor.audioSource so the signature stays consistent across the module.
In `@Sources/SpectrumInteractor/SpectrumInteractorImpl.swift`:
- Around line 21-25: The Pipeline stored properties are mutable without being
mutated individually, so switch cancellable, processor, and continuation in
SpectrumInteractorImpl.Pipeline from var to let. Update the Pipeline struct
definition only, since start() and stop() already replace the whole Pipeline
value rather than modifying fields in place.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b400810-1dc9-43d8-b62b-33ba83dc762f
📒 Files selected for processing (44)
.claude/CLAUDE.mdPackage.resolvedPackage.swiftREADME.mdSources/AppRouter/AppDependencyBootstrap.swiftSources/AppRouter/AppRouter.swiftSources/AudioTapDataSource/AudioTapDataSourceImpl.swiftSources/AudioTapDataSource/ProcessTapEngine.swiftSources/AudioTapDataSource/SampleRingBuffer.swiftSources/CLI/Info.plistSources/ConfigRepository/ConfigRepositoryImpl.swiftSources/DependencyInjection/DataSourceRegistration.swiftSources/DependencyInjection/InteractorRegistration.swiftSources/Domain/DataSource/AudioTapDataSource.swiftSources/Domain/Interactor/SpectrumInteractor.swiftSources/Domain/Interactor/TrackInteractor.swiftSources/Entity/AudioSourceState.swiftSources/Entity/Config/AppConfig.swiftSources/Entity/Config/SpectrumConfig.swiftSources/Entity/Style/AppStyle.swiftSources/Entity/Style/SpectrumPlacement.swiftSources/Entity/Style/SpectrumStyle.swiftSources/FrequencyAnalyzer/FrequencyAnalyzer.swiftSources/Presenters/Spectrum/SpectrumPresenter.swiftSources/SpectrumInteractor/SpectrumInteractorImpl.swiftSources/TrackInteractor/TrackInteractorImpl.swiftSources/VersionHandler/Resources/version.txtSources/Views/Overlay/AppWindow.swiftSources/Views/Overlay/OverlayContentView.swiftSources/Views/Spectrum/SpectrumView.swiftTests/AppRouterTests/AppLaunchEnvironmentTests.swiftTests/AudioTapDataSourceTests/AudioTapDataSourceImplTests.swiftTests/AudioTapDataSourceTests/SampleRingBufferTests.swiftTests/ConfigDataSourceTests/ConfigDecodingTests.swiftTests/ConfigDataSourceTests/ConfigTemplateTests.swiftTests/FrequencyAnalyzerTests/FrequencyAnalyzerTests.swiftTests/PresentersTests/HeaderPresenterDuplicateTests.swiftTests/PresentersTests/HeaderPresenterTests.swiftTests/PresentersTests/LyricsPresenterColumnsTests.swiftTests/PresentersTests/LyricsPresenterDuplicateTests.swiftTests/PresentersTests/LyricsPresenterTests.swiftTests/PresentersTests/SpectrumPresenterTests.swiftTests/SpectrumInteractorTests/SpectrumInteractorImplTests.swiftTests/ViewsTests/ViewRenderingTests.swift
MediaRemote helper のパイプが単一消費者である制約を TrackInteractor の 共有 publisher で吸収していたため、SpectrumInteractor → TrackInteractor という VIPER の層規律を破るエッジが生じていた。共有点を Repository に 下ろし、stream() を単一ポンプ + 全購読者ブロードキャスト + 直近値 replay の multicast にすることで、SpectrumInteractor は PlaybackUseCase.observeNowPlaying() を直接消費できるようになった。 - NowPlayingRepositoryImpl: 遅延起動の単一 poll ポンプで fan-out、 後着購読者には直近値を即時 replay、EOF で全購読者を finish - SpectrumInteractorImpl: for await 1 本で直列化（AsyncStream 中継を 廃止）、AudioSourceState の重複排除を自前で実施 - TrackInteractor: audioSource を protocol / 実装 / スタブから削除
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
SpectrumInteractor が AudioTapDataSource に直結し、UseCase と Repository を飛ばす依存になっていた（既存コードに一つもなかった例外）。 SpectrumUseCase（ビジネスロジック: capture lifecycle + FrequencyAnalyzer による PCM→バー振幅変換）と AudioCaptureRepository（タップ DataSource の オーケストレーション）を新設し、Interactor の依存を ConfigUseCase / PlaybackUseCase / SpectrumUseCase のみに揃えた。 - SpectrumInteractor から FrequencyAnalyzer 直 import も撤去 - テスト: SpectrumUseCase / AudioCaptureRepository スイート新設、 Interactor テストは SpectrumUseCase スタブ化 + 固定 sleep を排除 （disabled は同期即時 assert、tick 重複排除は pause マーカーで決定的に）
ProcessTapEngine の構築（TCC 初回プロンプトで長時間ブロックし得る CoreAudio セットアップ）を engine ロックの臨界区間内で行っていたため、 DisplayLink 毎フレームの latestSamples が同じロックで停滞する恐れが あった。旧エンジン停止 → ロック外で構築 → ロック内で公開の順に変更。 SPSC リングに書き手が2つ存在する瞬間も無くなる。
負値やゼロの bar_count が SpectrumPresenter のバー生成 Range を、 極小の fft_size がリングバッファ読み出しをクラッシュさせ得たため、 config→style 変換で barCount ≥ 1 / fftSize ≥ 64 に正規化。
SwiftPM が同一警告の文言を「conflicts with dependency on」から 「Conflicting identity for swift-syntax」に変更し、既存の除外 フィルタをすり抜けて警告ゲートが誤検知していた。新旧両文言を除外。
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/SpectrumInteractor/SpectrumInteractorImpl.swift (1)
48-87: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCreate the task only after acquiring the processor lock
Task { ... }is created beforeprocessor.withLockdecides whether this call owns the processor. If anotherstart()wins first, the losing task can still begin executing and callstartCapture/stopCapturebeforecandidate.cancel()takes effect, which races the tap lifecycle. Move task creation inside the locked section so a losing call never starts work.🤖 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/SpectrumInteractor/SpectrumInteractorImpl.swift` around lines 48 - 87, The `start()` method in `SpectrumInteractorImpl` creates `candidate` before `processor.withLock` decides ownership, so a losing call can still begin executing `Task` work and race the tap lifecycle. Move the `Task { ... }` creation inside the `processor.withLock` block after the `task == nil` check, so only the winning `start()` call creates and stores the task. Keep the existing `candidate.cancel()`/adoption behavior intact, but ensure no work starts before lock ownership is established.
🧹 Nitpick comments (1)
Tests/NowPlayingRepositoryTests/NowPlayingRepositoryTests.swift (1)
159-178: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a concurrent first-subscriber regression.
streamAandstreamBare constructed sequentially, so this validates multicast delivery but not theensurePumping()creation race. Start severalrepo.stream()subscriptions concurrently and have the gated data source assertmaxActivePolls == 1.🤖 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 `@Tests/NowPlayingRepositoryTests/NowPlayingRepositoryTests.swift` around lines 159 - 178, Add a concurrent first-subscriber regression test around NowPlayingRepositoryImpl.stream() to cover the ensurePumping() race that sequential streamA/streamB setup misses. Use GatedMediaRemoteDataSource to assert maxActivePolls stays at 1 while several repo.stream() subscriptions are created concurrently, and keep the existing multicast coverage by collecting from multiple subscribers in the same test.
🤖 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 `@Sources/NowPlayingRepository/NowPlayingRepositoryImpl.swift`:
- Around line 74-79: The single-poller contract is still broken because
`NowPlayingRepositoryImpl.fetch()` calls `dataSource.poll()` directly instead of
going through the shared pump/owner used by `pumpTask()`. Update `fetch()` to
route polling through the same serialized hub as `pumpTask()` so only one
consumer reads helper events and all results are broadcast consistently to
stream subscribers.
- Around line 60-67: In ensurePumping and pumpTask, the pump Task can start
running before it is adopted into hub.state.pump, which allows an early .eof to
reset the hub and then a completed task to be stored back into state.pump;
change the flow so the task is created but not allowed to poll until after
adoption succeeds. Also update pumpTask to check Task cancellation before
entering dataSource.poll() so a losing/canceled task exits immediately instead
of doing extra work.
In `@Sources/SpectrumInteractor/SpectrumInteractorImpl.swift`:
- Around line 89-105: The stop() teardown in SpectrumInteractorImpl is not
scoped to the capture generation, so a newer start() can be affected by an older
async cleanup. Update stop() to capture and validate the current generation/task
before scheduling the Task cleanup, and ensure the async block only stops the
tap and sends false if it still matches the same processor/capture session. Use
the existing stop(), processor, tap, capturingSubject, and
AudioTapDataSourceImpl.stopTap() flow to keep teardown from cancelling a newer
capture.
In `@Tests/SpectrumInteractorTests/SpectrumInteractorImplTests.swift`:
- Around line 49-60: The periodicTickDedup test in SpectrumInteractorImplTests
is using a fixed Task.sleep to wait for dedup behavior, which can hide
regressions and slow failures. Replace the sleep-based check with a second
pollUntil that exits as soon as an unexpected extra tap start appears or the
deadline is reached, using the existing Harness and tap.startedPids assertions
to verify the dedup holds over time.
---
Outside diff comments:
In `@Sources/SpectrumInteractor/SpectrumInteractorImpl.swift`:
- Around line 48-87: The `start()` method in `SpectrumInteractorImpl` creates
`candidate` before `processor.withLock` decides ownership, so a losing call can
still begin executing `Task` work and race the tap lifecycle. Move the `Task {
... }` creation inside the `processor.withLock` block after the `task == nil`
check, so only the winning `start()` call creates and stores the task. Keep the
existing `candidate.cancel()`/adoption behavior intact, but ensure no work
starts before lock ownership is established.
---
Nitpick comments:
In `@Tests/NowPlayingRepositoryTests/NowPlayingRepositoryTests.swift`:
- Around line 159-178: Add a concurrent first-subscriber regression test around
NowPlayingRepositoryImpl.stream() to cover the ensurePumping() race that
sequential streamA/streamB setup misses. Use GatedMediaRemoteDataSource to
assert maxActivePolls stays at 1 while several repo.stream() subscriptions are
created concurrently, and keep the existing multicast coverage by collecting
from multiple subscribers in the same test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a3b6ed4-16c8-4a01-a172-7ac77ae04a73
📒 Files selected for processing (8)
.claude/CLAUDE.mdSources/AudioTapDataSource/ProcessTapEngine.swiftSources/NowPlayingRepository/NowPlayingRepositoryImpl.swiftSources/SpectrumInteractor/SpectrumInteractorImpl.swiftTests/AppRouterTests/AppLaunchEnvironmentTests.swiftTests/NowPlayingRepositoryTests/NowPlayingRepositoryTests.swiftTests/SpectrumInteractorTests/SpectrumInteractorImplTests.swiftTests/ViewsTests/ViewRenderingTests.swift
💤 Files with no reviewable changes (2)
- Tests/ViewsTests/ViewRenderingTests.swift
- Tests/AppRouterTests/AppLaunchEnvironmentTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- .claude/CLAUDE.md
- Sources/AudioTapDataSource/ProcessTapEngine.swift
Owner
Author
|
CodeRabbit レビュー本文の nitpick 3 件への
|
| guard let first = buffers.first, let data = first.mData else { return } | ||
| let channels = max(Int(first.mNumberChannels), 1) | ||
| let sampleCount = Int(first.mDataByteSize) / MemoryLayout<Float>.size | ||
| let frames = min(sampleCount / channels, scratchCapacity) |
Owner
Author
There was a problem hiding this comment.
mixDownToMono は private static func（L167）で、scratchCapacity も private static let（L13）です。static メソッドから同じ型の static メンバーへは修飾なしの裸参照が正当な Swift で、instance メソッドの場合にのみ Self. 修飾が必要になります。実際にローカルの swift build / swift test（1027 件）および CI はこのコードのままコンパイル・パスしています。
- ensurePumping: .idle → .starting(token) → .running(task) の予約遷移で pump タスクの二重生成を構造的に排除(単一イテレータへの並行 poll 防止)。 即時 EOF で finishAll がハブをリセットした場合はトークン不一致で 完了済みタスクを格納しない(次の購読者の pump 起動を塞がない) - fetch(): dataSource.poll() の直接呼び出しを廃止し、multicast stream の 先頭値を返す形に変更。稼働中の pump とイテレータを奪い合う経路が消える - テスト追加: 並行初回購読者 8 本で maxActivePolls == 1 / EOF リセット後の pump 再起動(退行時はハングせず fail)
- start(): processor を .idle → .starting(token) → .running(task) の 予約遷移に変更。競合する start() はタスクを生成すらしないため、 敗者がキャンセル前にタップ操作を実行する窓が消える - stop(): 排水後に新世代が採用済みなら teardown をスキップ。 stop 直後の再 start で古いクリーンアップが新しいキャプチャを 破壊して capturing を false に戻す競合を防ぐ - テスト追加: 二重 start で単一プロセッサ / stop→start の再開。 playback スタブを履歴 replay 付き multicast 化(live の NowPlayingRepository.stream() と同じ購読モデル)
Owner
Author
|
CodeRabbit レビュー本文（diff 外 1 件 + nitpick 1 件）への
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
概要
now-playing アプリの音声を CoreAudio process tap で捕捉し、オーバーレイにリアルタイムのスペクトラムアナライザ（バーグラフ）を描画する。Closes #23
変更内容
SpectrumConfig/SpectrumStyle/SpectrumPlacement/AudioSourceStateを追加。[spectrum]TOML セクション（bar_count / bar_color / placement / decay_rate / fft_size 等、全項目デフォルトあり・既定 disabled）stream()を multicast 化（単一ポンプ + 全購読者ブロードキャスト + 直近値 replay）。MediaRemote ヘルパーのパイプは単一消費者だが、任意のレイヤーがobserveNowPlaying()を自由に購読できるProcessTapEngine（macOS 14.4+ API、14.0 ターゲット向けに availability erasure）+ swift-atomics による SPSC ロックフリーリングバッファ。IOProc コールバックは RT-safe（アロケーション・ロック・Swift concurrency なし）kAudioHardwarePropertyProcessObjectListを ppid ウォークでフィルタして全対象をCATapDescriptionに渡すSpectrumUseCaseがサンプル取得（Repository 経由）と FrequencyAnalyzer による周波数変換を組み合わせ、AudioCaptureRepositoryがタップ DataSource を抽象化する（層飛ばしゼロ）PlaybackUseCase.observeNowPlaying()を単一のfor awaitで直接消費（Interactor 間依存なし）。直列消費により pause/play 連打でもタップ生成/破棄が交錯せず、pid/再生状態の重複排除も自前で行う。排他はOSAllocatedUnfairLock(state:)。キャプチャ操作と magnitudes 取得はSpectrumUseCaseへ委譲binHeights()は読み取り専用（Canvas 描画中の @published 変更なし）。bug: パフォーマンス低下と電力消費・発熱・ファン回転の悪化 #252/観測タスク: lyra daemon (28% CPU) のホットパスを sample / Instruments で特定 #258 のゼロアイドルコストパターン（条件付き include +TimelineView(paused:)）NSAudioCaptureUsageDescriptionを追加（システムオーディオ録音 TCC）背景・動機
issue #23 の本体実装。前提工事の PID 伝搬は #294 で完了済み。設計判断（14.4 フロア / ブラウザはそのままタップ / swift-atomics）はセッション内で確認済み。
テスト計画
swift test全 1027 件パス（新規: FrequencyAnalyzer / SampleRingBuffer / AudioTapDataSourceImpl / SpectrumInteractorImpl / SpectrumUseCaseImpl / AudioCaptureRepositoryImpl / SpectrumPresenter /[spectrum]デコード / NowPlayingRepository multicast）make lintパス備考