fix(waveform): unify gradient direction and honor real audio - #23
fix(waveform): unify gradient direction and honor real audio#23tukuyomil032 wants to merge 10 commits into
Conversation
All six bars now share a single top→bottom LinearGradient (highlight → primary → secondary) so the waveform reads as one artwork-tinted shape whose height varies per band, instead of a mosaic where bars 2 and 5 ran the gradient in reverse. blendedLevels() no longer mixes a 35% synthetic sine flourish into real audio — real levels are shaped and passed through directly (Atoll-style honesty). Synthetic waves remain only for the usesSyntheticFallback path where no capture is available. Bar animation shortened 55ms → 28ms and analyzer release times reduced ~35% (max 155ms) so the visual snaps down like the real Dynamic Island. Adds AudioSpectrumAnalyzerTests to guard against future release-time regressions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughNow Playing のスペクトラムと波形描画を調整し、アートワーク取得をエラー処理・フォールバック・再試行対応に更新しました。歌詞取得には正規化、キャッシュ、複数プロバイダ競合、duration 連携を追加しています。 ChangesNow Playing メディア更新
歌詞取得
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NowPlayingManager
participant ArtworkFetcher
participant AppleScript
participant ITunesSearch
NowPlayingManager->>ArtworkFetcher: Apple Music アートワーク取得
ArtworkFetcher->>AppleScript: メタデータ取得
AppleScript-->>ArtworkFetcher: データまたはエラー
ArtworkFetcher->>ITunesSearch: フォールバック検索
ITunesSearch-->>ArtworkFetcher: アートワークデータ
ArtworkFetcher-->>NowPlayingManager: データまたは取得エラー
sequenceDiagram
participant NowPlayingCard
participant LyricsStore
participant LRCLIB
participant LyricsKitFetcher
NowPlayingCard->>LyricsStore: title、artist、album、duration
LyricsStore->>LRCLIB: 歌詞検索
LRCLIB-->>LyricsStore: 歌詞または空結果
LyricsStore->>LyricsKitFetcher: フォールバック取得
LyricsKitFetcher-->>LyricsStore: 検証済み歌詞
LyricsStore-->>NowPlayingCard: LyricsLine 配列または nil
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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
🤖 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 `@perchTests/NowPlaying/AudioSpectrumAnalyzerTests.swift`:
- Around line 10-39: Adjust releaseIsSnappyAfterSilence to evaluate the analyzer
after approximately 500 ms of silence rather than 25 × 2,048-sample chunks
(~1.16 seconds). Keep the existing silence input and peak assertion, while
ensuring the test specifically detects decay below 0.10 within the interval
named by the 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a0549c3b-db09-43db-a4af-5e58dca441e9
📒 Files selected for processing (3)
perch/Features/NowPlaying/AudioSpectrumAnalyzer.swiftperch/Features/NowPlaying/WaveformView.swiftperchTests/NowPlaying/AudioSpectrumAnalyzerTests.swift
| @Test("Levels decay below 0.10 within ~500ms of silence") | ||
| func releaseIsSnappyAfterSilence() { | ||
| let sampleRate: Float = 44_100 | ||
| let analyzer = AudioSpectrumAnalyzer(publishRateHz: 0) | ||
|
|
||
| // Feed in FFT-sized chunks so consume() doesn't hit its internal | ||
| // accumulator-truncation guard (fftSize * 8) and drop hops silently. | ||
| let chunkSize = 2_048 | ||
|
|
||
| // Prime ~2.8 s of broadband noise so every band's smoothed level | ||
| // reaches a stable elevated value before silence begins. | ||
| for chunk in 0..<60 { | ||
| let noise = generateNoise(sampleCount: chunkSize, amplitude: 0.7, seed: UInt64(chunk + 1)) | ||
| _ = analyzer.consume(samples: noise, sampleRate: sampleRate) | ||
| } | ||
|
|
||
| // Feed ~1.1 s of silence in the same chunk size. With release times | ||
| // ≤ 155 ms this should decay the loudest band well below 0.10 — | ||
| // guards against anyone reverting to the sluggish 150-260 ms range. | ||
| let silenceChunk = [Float](repeating: 0, count: chunkSize) | ||
| var lastPeak: Float = 0 | ||
| for _ in 0..<25 { | ||
| if let levels = analyzer.consume(samples: silenceChunk, sampleRate: sampleRate) { | ||
| lastPeak = levels.max() ?? 0 | ||
| } | ||
| } | ||
|
|
||
| #expect( | ||
| lastPeak < 0.10, | ||
| "Release times ≤ 155ms should decay bars below 0.10 after ~1s silence; got peak=\(lastPeak)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
約500msの減衰を実際に検証してください。
25 × 2,048 samples を 44,100 Hz で投入しているため、無音区間は約 1.16 秒です。現在の条件では、テスト名どおりの約500ms時点でのリリース劣化を検出できません。
修正例
- // Feed ~1.1 s of silence in the same chunk size. With release times
+ // Feed ~500 ms of silence in the same chunk size. With release times
@@
- for _ in 0..<25 {
+ for _ in 0..<11 {
@@
- "Release times ≤ 155ms should decay bars below 0.10 after ~1s silence; got peak=\(lastPeak)")
+ "Release times ≤ 155ms should decay bars below 0.10 after ~500ms silence; got peak=\(lastPeak)")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test("Levels decay below 0.10 within ~500ms of silence") | |
| func releaseIsSnappyAfterSilence() { | |
| let sampleRate: Float = 44_100 | |
| let analyzer = AudioSpectrumAnalyzer(publishRateHz: 0) | |
| // Feed in FFT-sized chunks so consume() doesn't hit its internal | |
| // accumulator-truncation guard (fftSize * 8) and drop hops silently. | |
| let chunkSize = 2_048 | |
| // Prime ~2.8 s of broadband noise so every band's smoothed level | |
| // reaches a stable elevated value before silence begins. | |
| for chunk in 0..<60 { | |
| let noise = generateNoise(sampleCount: chunkSize, amplitude: 0.7, seed: UInt64(chunk + 1)) | |
| _ = analyzer.consume(samples: noise, sampleRate: sampleRate) | |
| } | |
| // Feed ~1.1 s of silence in the same chunk size. With release times | |
| // ≤ 155 ms this should decay the loudest band well below 0.10 — | |
| // guards against anyone reverting to the sluggish 150-260 ms range. | |
| let silenceChunk = [Float](repeating: 0, count: chunkSize) | |
| var lastPeak: Float = 0 | |
| for _ in 0..<25 { | |
| if let levels = analyzer.consume(samples: silenceChunk, sampleRate: sampleRate) { | |
| lastPeak = levels.max() ?? 0 | |
| } | |
| } | |
| #expect( | |
| lastPeak < 0.10, | |
| "Release times ≤ 155ms should decay bars below 0.10 after ~1s silence; got peak=\(lastPeak)") | |
| `@Test`("Levels decay below 0.10 within ~500ms of silence") | |
| func releaseIsSnappyAfterSilence() { | |
| let sampleRate: Float = 44_100 | |
| let analyzer = AudioSpectrumAnalyzer(publishRateHz: 0) | |
| // Feed in FFT-sized chunks so consume() doesn't hit its internal | |
| // accumulator-truncation guard (fftSize * 8) and drop hops silently. | |
| let chunkSize = 2_048 | |
| // Prime ~2.8 s of broadband noise so every band's smoothed level | |
| // reaches a stable elevated value before silence begins. | |
| for chunk in 0..<60 { | |
| let noise = generateNoise(sampleCount: chunkSize, amplitude: 0.7, seed: UInt64(chunk + 1)) | |
| _ = analyzer.consume(samples: noise, sampleRate: sampleRate) | |
| } | |
| // Feed ~500 ms of silence in the same chunk size. With release times | |
| // ≤ 155 ms this should decay the loudest band well below 0.10 — | |
| // guards against anyone reverting to the sluggish 150-260 ms range. | |
| let silenceChunk = [Float](repeating: 0, count: chunkSize) | |
| var lastPeak: Float = 0 | |
| for _ in 0..<11 { | |
| if let levels = analyzer.consume(samples: silenceChunk, sampleRate: sampleRate) { | |
| lastPeak = levels.max() ?? 0 | |
| } | |
| } | |
| `#expect`( | |
| lastPeak < 0.10, | |
| "Release times ≤ 155ms should decay bars below 0.10 after ~500ms silence; got peak=\(lastPeak)") |
🤖 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 `@perchTests/NowPlaying/AudioSpectrumAnalyzerTests.swift` around lines 10 - 39,
Adjust releaseIsSnappyAfterSilence to evaluate the analyzer after approximately
500 ms of silence rather than 25 × 2,048-sample chunks (~1.16 seconds). Keep the
existing silence input and peak assertion, while ensuring the test specifically
detects decay below 0.10 within the interval named by the test.
…-audio-reactivity
The AGC target and normalization ceiling were both -14 dB, which mathematically guaranteed that the peak band's linear value clamped to 1.0 in steady state. The pow(0.88) lifter + remapForDynamicIsland's even cross-mix then propagated that saturation to every bar, and WaveformView's second pow(0.92) lifter finished pushing everything to maxHeight. The old 35% sin blend hid this by injecting fake motion; removing that blend exposed the underlying pin. Changes: AGC target moved 8 dB below ceiling (-22 dB), min-clamp widened to -20 dB so hot masters can be attenuated. remapForDynamicIsland rewritten with 70% main-band dominance + two 15% accents per bar — a saturated band no longer drags every bar upward. WaveformView.blendedLevels drops the second-pass pow(0.92) so analyzer levels pass through unmodified. Adds a saturation-guard test that fails if any band settles above 0.95 for loud broadband input. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Apple Music's AppleScript artwork descriptor sometimes returns nil right after a track change while Music.app is still populating metadata (especially for streaming / DRM / Apple Music catalog tracks). fetchAndApplyArtwork silently gave up on the first nil, so the compact pill stayed on the music-note placeholder — and the waveform lost its artwork-derived gradient — until the user happened to expand the pill and let Compact rebuild against a later state. Adds a dedicated fetchAppleMusicArtworkWithRetry with 250ms/500ms/1s/2s backoff, all guarded by a track-identity check so a fetch spawned for a previous song can't overwrite a newer one. Existing pollAppleMusicPosition (1.5s cadence) now piggybacks a retry whenever artwork is still missing, catching cases where all initial retries fired before the descriptor was ready. applyState's Equatable early-return is relaxed so a same-state playerInfo notification triggers a re-fetch when Apple Music artwork is still nil. Also drops the sleep-and-recheck dead code in NowPlayingCompact that captured by value and could never observe the parent's update. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
perch/Features/NowPlaying/NowPlayingManager.swift (1)
339-352: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winApple Musicアートワーク再取得の重複起動を防ぐガードがない
同一トラックに対して次の3箇所が独立に再取得を起動できます:
- L345-352: ポーリングごとに
tryFetchAppleMusicArtworkを呼ぶピギーバック。- L478-493: 同一state通知を受けるたびに
fetchAppleMusicArtworkWithRetryを新規Taskで起動。- L596-611: 上記2つから呼ばれる本体の再試行チェーン（最大約3.75秒、4回分のバックオフ）。
同一trackに対して短時間に同一state通知が複数回届く、またはポーリング周期とタイミングが重なると、複数の再試行チェーンが並行してAppleScriptを実行することになります。
currentState?.artwork != nilチェック（L608）はTOCTOU的なガードにとどまり、進行中フラグのような排他制御がありません。トラックあたりの進行中フラグ（例:Set<String>やBoolプロパティ）を用意して重複起動を防ぐことを推奨します。また、アートワークが元々存在しない曲（埋め込みアートワークなし）の場合、L349-352 はトラック再生が続く限りポーリングごと（1.5秒間隔）に無期限でAppleScriptを呼び続けます。上限回数や経過時間によるカットオフを設けることも検討してください。
Also applies to: 478-493, 592-611
🤖 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 `@perch/Features/NowPlaying/NowPlayingManager.swift` around lines 339 - 352, Prevent duplicate Apple Music artwork retry chains across pollAppleMusicPosition, the state-notification retry launch, and fetchAppleMusicArtworkWithRetry by adding per-track in-flight coordination (such as a Set<String> or equivalent flag), clearing it when the chain completes, and ensuring concurrent callers reuse or skip the existing attempt. Also add a per-track retry cutoff based on attempts or elapsed time so pollAppleMusicPosition cannot invoke AppleScript indefinitely for tracks without embedded artwork.
🤖 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 `@perch/Features/NowPlaying/NowPlayingManager.swift`:
- Around line 613-623: Update tryFetchAppleMusicArtwork so that, after artwork
loading and sameAppleMusicTrack validation succeed, it applies artwork by
enriching the latest currentState rather than the captured state parameter.
Preserve the false returns for fetch, image decoding, or track validation
failures, and return true only after updating currentState.
---
Nitpick comments:
In `@perch/Features/NowPlaying/NowPlayingManager.swift`:
- Around line 339-352: Prevent duplicate Apple Music artwork retry chains across
pollAppleMusicPosition, the state-notification retry launch, and
fetchAppleMusicArtworkWithRetry by adding per-track in-flight coordination (such
as a Set<String> or equivalent flag), clearing it when the chain completes, and
ensuring concurrent callers reuse or skip the existing attempt. Also add a
per-track retry cutoff based on attempts or elapsed time so
pollAppleMusicPosition cannot invoke AppleScript indefinitely for tracks without
embedded artwork.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a5c90eb1-fd73-42e1-a6d1-46ecbc5fbc28
📒 Files selected for processing (5)
perch/Features/NowPlaying/AudioSpectrumAnalyzer.swiftperch/Features/NowPlaying/NowPlayingCompact.swiftperch/Features/NowPlaying/NowPlayingManager.swiftperch/Features/NowPlaying/WaveformView.swiftperchTests/NowPlaying/AudioSpectrumAnalyzerTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- perch/Features/NowPlaying/WaveformView.swift
| /// Returns true when artwork was fetched, validated, and applied. Callable | ||
| /// both from the retry chain and from position-polling piggyback. | ||
| @discardableResult | ||
| private func tryFetchAppleMusicArtwork(for state: NowPlayingState) async -> Bool { | ||
| guard let data = await ArtworkFetcher.shared.fetchAppleMusicArtworkData(), | ||
| let image = NSImage(data: data) | ||
| else { return false } | ||
| guard sameAppleMusicTrack(as: state) else { return false } | ||
| currentState = state.enriched(artwork: image) | ||
| return true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -A8 'func enriched\(' --type=swift
rg -nP -B2 -A15 'struct NowPlayingState' --type=swiftRepository: tukuyomil032/Perch
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -u
echo "Swift files around NowPlaying:"
fd -e swift 'NowPlaying|NowPlayingState|NowPlayingManager' . || true
echo
echo "Search for NowPlayingState and enriched:"
rg -n "NowPlayingState|enriched\\(" --type=swift . || true
echo
echo "Search Apple Music artwork methods:"
rg -n "tryFetchAppleMusicArtwork|fetchAppleMusicArtworkWithRetry|sameAppleMusicTrack|sameAppleMusicTrack" --type=swift . || trueRepository: tukuyomil032/Perch
Length of output: 7017
🏁 Script executed:
#!/bin/bash
set -u
sed -n '1,120p' perch/Features/NowPlaying/NowPlayingState.swift
sed -n '330,635p' perch/Features/NowPlaying/NowPlayingManager.swiftRepository: tukuyomil032/Perch
Length of output: 17156
最新 state で artwork を適用してください
enriched(artwork:) は elapsedTime/timestamp/isPlaying も受け取った値をコピーするため、tryFetchAppleMusicArtwork で取得中に pollAppleMusicPosition が更新した最新の再生位置が、古いスナップショットの state.enriched(artwork:) で上書きされます。失敗しない場合に限り、currentState から最新の位置情報を使って artwork を追加してください。
🤖 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 `@perch/Features/NowPlaying/NowPlayingManager.swift` around lines 613 - 623,
Update tryFetchAppleMusicArtwork so that, after artwork loading and
sameAppleMusicTrack validation succeed, it applies artwork by enriching the
latest currentState rather than the captured state parameter. Preserve the false
returns for fetch, image decoding, or track validation failures, and return true
only after updating currentState.
…-audio-reactivity
…pleScript silent failure Since macOS 10.14, apps compiled with a modern SDK cannot send Apple Events without this Info.plist key. macOS never surfaces a TCC dialog — NSAppleScript.executeAndReturnError simply returns errAEEventNotPermitted (-1743) in the error dict. Perch's ArtworkFetcher (both Apple Music and Spotify branches) and pollAppleMusicPosition all discard that error and return nil, so the entire AppleScript path has been silently broken since day one. Spotify's DistributedNotification kept state visible so the failure only surfaced for Apple Music, where artwork depends entirely on this path. First-launch users will see a Music.app / Spotify.app automation permission prompt. Existing installs may need 'tccutil reset AppleEvents com.tukuyomi032.perch' to clear a stale deny cache. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… Music ArtworkFetcher is now throws-based and returns a typed ArtworkFetchError so callers can log why a fetch failed (permission denied, HTTP error, no results, decode failure, etc.). The previous Data?/nil convention swallowed every AppleScript error dict, which is how the NSAppleEventsUsageDescription bug went undiagnosed for so long. fetchAppleMusicArtworkData now takes title/artist/album and transparently falls back to a shared iTunes Search API helper when the AppleScript path throws — this handles both the TCC-permission case and streaming-only tracks that Music.app never caches artwork for locally. NowPlayingManager updated: catch/log errors for Spotify, Apple Music, YouTube Music paths. Apple Music retry loop reduced from 4 exponential retries to initial + one 500ms retry (the fallback is deterministic so extra retries only help network flake). Added an appleMusicArtworkPollAttempts counter (capped at 3, reset on track change) so pollAppleMusicPosition can't loop forever hammering both paths when they truly have no artwork available. Clean-room: iTunes Search API is a public Apple-documented endpoint. No Atoll code inspected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
LRCLIB's /api/get is strict-match: track_name + artist_name + optional album_name + optional duration. Perch was only sending title+artist, so any Apple Music track whose name carried a subtitle like '(feat. XX)', '(TV Size)', '[Remaster]', or a trailing '- Radio Edit' missed on the first attempt and often on the fuzzy /api/search fallback too. The LyricsKit fallback (netease/qq/kugou) then rarely rescued Japanese tracks because they're not in Chinese lyric databases. LyricsStore now: sends album+duration to /api/get; retries with a subtitle-stripped title if the initial pair misses; retries again with just the primary artist (dropping 'feat.'/'&'); skips fetches when artist is empty (guaranteed 404); and negatively caches misses for 10 minutes so a rapidly re-rendered card doesn't re-request the same missing lyrics on every tick. Adds info-level diagnostics at each stage — the previous debug-only messages made it impossible to see why lyrics didn't load in prod builds. LyricsNormalizer is exposed as a nonisolated enum so it's callable from actor context and testable in isolation. New LyricsNormalizationTests cover the strip-subtitle and split-artist patterns end-to-end. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…der timeout The previous implementation walked netease/qq/kugou sequentially with no timeout. One slow provider could stall the whole fallback chain for tens of seconds, effectively making the LyricsKit tier a no-op for any request that hit a hanging endpoint. Now every provider races in a TaskGroup — first non-empty result wins and the rest are cancelled — with a 3s bounded lifetime each so a hung request can't pin the actor. The Japanese-detection guard is extended: title detection now includes CJK Unified Ideographs (U+4E00–U+9FFF) so kanji-only邦楽 titles still trigger the check. Lyrics validation still requires hiragana or katakana (unambiguous Japanese signal) to accept the result — kanji alone is ambiguous between Japanese and Chinese and would let Chinese lyrics through. Logs each provider by name at info level on success and debug level on timeout / mismatch, so failures are visible without turning on verbose logging. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Real music rolls off 15-20 dB from bass to treble, so with only +7 dB of high-shelf compensation the low band (s[0]) was 8-13 dB louder than the high band (s[5]) even after gain matching. remapForDynamicIsland gives bar0 70% of s[0], so any bass-heavy song (i.e. almost everything) had bar0 pinned tall while bar5 stayed short — the 'lopsided' waveform Atoll doesn't have. bandGainDB updated from [0, 1.0, 2.0, 3.5, 5.0, 7.0] to [0, 3.0, 6.0, 10.0, 14.0, 18.0]. The +18 dB top-shelf brings treble roughly level with bass so every bar dances based on its band's momentary activity, not on the fixed spectral tilt of the source. AGC still targets peak at ceiling−8dB so the saturation guard (peak < 0.95) holds — verified by the existing doesNotPinAtCeiling test. New barsStayBalanced test: after priming with broadband noise, max/min bar ratio must stay under 2.0. Guards against anyone reverting to a shallow shelf without noticing the visual bias returned. Clean-room: Perch-specific tuning based on the well-documented spectral-rolloff-of-music heuristic, not values borrowed from Atoll's GPL source. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@perch/Features/NowPlaying/ArtworkFetcher.swift`:
- Around line 118-131: Update the query encoding in the rawQuery/searchURL
construction to use query-value-safe percent encoding that excludes /, &, +, and
=, and converts spaces to %20 rather than +. Preserve the existing URL
construction and noResultsFound fallback while ensuring song titles, artists,
and albums cannot alter query parameter parsing.
- Around line 5-9: ArtworkFetcher 内の Apple Music および Spotify の AppleScript 実行から
Task.detached を हटし、NSAppleScript.executeAndReturnError を単一の直列実行コンテキスト（既存の actor
隔離、または専用 serial queue／@MainActor）で呼び出すよう更新してください。各処理の非同期結果は維持しつつ、AppleScript
呼び出しが actor 外の並行スレッドへ移らないようにし、ドキュメントの直列化保証と一致させてください。
In `@perch/Features/NowPlaying/LyricsKitFetcher.swift`:
- Around line 61-65: Update the timeout task in the inner task group to stop
immediately when Task.sleep is cancelled, instead of swallowing
CancellationError and calling logTimeout. Only invoke logTimeout when the sleep
completes normally, while preserving the existing timeout return behavior.
In `@perch/Features/NowPlaying/LyricsStore.swift`:
- Around line 260-267:
LyricsStoreの検索結果ループで、非空のsyncedLyricsを解析する前にresult["trackName"]がtitleとcontains一致するか検証してください。一致しない結果はcontinueして次の候補を試し、LyricsKitFetcher.fetchFromProviderと同じタイトル判定を再利用し、一致した結果の非空パース結果だけを返してください。
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a20ac79-36cf-4ecb-8655-f6b910030257
📒 Files selected for processing (9)
perch/Features/NowPlaying/ArtworkFetcher.swiftperch/Features/NowPlaying/AudioSpectrumAnalyzer.swiftperch/Features/NowPlaying/LyricsKitFetcher.swiftperch/Features/NowPlaying/LyricsStore.swiftperch/Features/NowPlaying/NowPlayingCard.swiftperch/Features/NowPlaying/NowPlayingManager.swiftperch/Resources/Info.plistperchTests/NowPlaying/AudioSpectrumAnalyzerTests.swiftperchTests/NowPlaying/LyricsNormalizationTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- perch/Features/NowPlaying/AudioSpectrumAnalyzer.swift
| /// Fetches album artwork for Spotify, Apple Music, and YouTube Music. | ||
| /// Actor isolation serializes AppleScript calls (NSAppleScript is not thread-safe). | ||
| actor ArtworkFetcher { | ||
| static let shared = ArtworkFetcher() | ||
| private init() {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
# Swift の Task.detached が actor の isolation を継承しないことを確認
rg -n 'Task\.detached' perch/Features/NowPlaying/ArtworkFetcher.swiftRepository: tukuyomil032/Perch
Length of output: 231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline perch/Features/NowPlaying/ArtworkFetcher.swift --view expanded || true
echo "== relevant ArtworkFetcher sections =="
sed -n '1,110p' perch/Features/NowPlaying/ArtworkFetcher.swift | cat -n
sed -n '160,200p' perch/Features/NowPlaying/ArtworkFetcher.swift | cat -n
echo "== NowPlayingManager usages / artwork fetch call graph =="
git ls-files | rg 'NowPlayingManager.swift|ArtworkFetcher.swift|.*NowPlaying.*\.swift$' || true
rg -n "ArtworkFetcher|fetchAppleMusicArtwork|fetchSpotifyArtwork|fetchArtworkViaITunesSearch|lastSpotifyURL|retry|concurrent|actor" perch/Features perch -g '*.swift' | head -200Repository: tukuyomil032/Perch
Length of output: 17487
🌐 Web query:
Apple NSAppleScript executeAndReturnError thread safety not thread safe
💡 Result:
The NSAppleScript class is not thread-safe [1][2][3]. Official Apple documentation explicitly states that NSAppleScript should be used only from the main thread of an application [2][4]. Because NSAppleScript is not thread-safe, calling methods such as executeAndReturnError: from any thread other than the main thread can lead to unpredictable behavior, crashes, or data corruption [1][3]. Furthermore, even when executed on the main thread, the call to executeAndReturnError: is not necessarily "safe" in the context of a complex application; while it appears to be a blocking call, it can allow other main-thread activities—such as timers or event processing—to execute while the script is running, potentially introducing reentrancy issues or invalid state assumptions in your code [5]. To safely execute AppleScripts in a multithreaded Cocoa application, you should ensure that all interactions with NSAppleScript (including initialization and execution) are performed exclusively on the main thread, typically by dispatching the work to the main queue using dispatch_async or performSelectorOnMainThread: [1][3][5]. Given the reentrancy risks, it is also recommended to ensure the script execution is isolated from other logic that might depend on a stable main-thread state [5].
Citations:
- 1: https://stackoverflow.com/questions/4983262/nsapplescript-and-thread-safety
- 2: https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/Multithreading/ThreadSafetySummary/ThreadSafetySummary.html
- 3: https://cocoadev.github.io/NSAppleScript/
- 4: https://leopard-adc.pepas.com/documentation/Cocoa/Reference/Foundation/Classes/NSAppleScript_Class/Reference/Reference.html
- 5: https://www.stairways.com/blog/2014-04-24-nsapplescript-not-thread-safe
🌐 Web query:
Swift Task.detached inherits actor isolation documentation
💡 Result:
No, Task.detached does not inherit actor isolation [1][2][3]. When you create a detached task using Task.detached, it runs independently of the current actor context [2][3][4]. This stands in contrast to the standard Task initializer (Task {... }), which inherits the actor isolation, priority, and task-local storage of the surrounding context [1][2][3]. The technical distinction is driven by how these initializers are implemented in the Swift standard library. The standard Task initializer is decorated with the @_inheritActorContext attribute, which explicitly tells the compiler to propagate the caller's isolation to the new task [5]. Task.detached lacks this attribute, intentionally creating an unstructured task that is not bound to the caller's actor or priority [5][1][3]. Because detached tasks do not inherit the caller's context, they are often used in scenarios where you need to perform work that should explicitly avoid being restricted by the current actor's execution context, such as avoiding the MainActor's serial executor [5][1]. However, because they also do not inherit task-local values or parent priority, they should be used deliberately [5][1][6].
Citations:
- 1: https://forums.swift.org/t/is-task-detached-still-useful/83580
- 2: https://forums.swift.org/t/so-is-task-unstructured-or-what/78111
- 3: https://www.hackingwithswift.com/quick-start/concurrency/whats-the-difference-between-a-task-and-a-detached-task
- 4: https://stackoverflow.com/questions/71575803/difference-of-creating-regular-or-detached-task-from-task-less-context
- 5: https://developer.apple.com/forums/thread/761150
- 6: https://forums.swift.org/t/task-and-task-detached/80861
NSAppleScript の実行を Task.detached から直列実行に揃える
Apple Music (Line 64-87) と Spotify (Line 179-190) の AppleScript は Task.detached { ... }.value で actor の isolation を渡り、NSAppleScript.executeAndReturnError が actor 外部の並行タスク/スレッドで実行されます。NSAppleScript はスレッドセーフトリではないためこのままではハング/クラッシュのリスクがあり、ドキュメントの「Actor isolation serializes AppleScript calls」とも矛盾します。NSAppleScript は専用 serial queue、厳密には main queue/@MainActor に隔離して呼ぶべきです。
🤖 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 `@perch/Features/NowPlaying/ArtworkFetcher.swift` around lines 5 - 9,
ArtworkFetcher 内の Apple Music および Spotify の AppleScript 実行から Task.detached を
हटし、NSAppleScript.executeAndReturnError を単一の直列実行コンテキスト（既存の actor 隔離、または専用 serial
queue／@MainActor）で呼び出すよう更新してください。各処理の非同期結果は維持しつつ、AppleScript 呼び出しが actor
外の並行スレッドへ移らないようにし、ドキュメントの直列化保証と一致させてください。
Source: Coding guidelines
| let rawQuery: String | ||
| if let album, !album.isEmpty { | ||
| rawQuery = "\(title) \(artist) \(album)" | ||
| } else { | ||
| rawQuery = "\(title) \(artist)" | ||
| } | ||
| guard | ||
| let encoded = rawQuery.addingPercentEncoding( | ||
| withAllowedCharacters: .urlQueryAllowed), | ||
| let searchURL = URL( | ||
| string: "https://itunes.apple.com/search?term=\(encoded)&entity=song&limit=5") | ||
| else { | ||
| throw ArtworkFetchError.noResultsFound | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does CharacterSet.urlQueryAllowed escape ampersand and plus characters in Swift?
💡 Result:
No, CharacterSet.urlQueryAllowed does not escape the ampersand (&) or plus (+) characters [1][2][3]. Because these characters are considered legal within a URL query component according to the character set's definition, the addingPercentEncoding(withAllowedCharacters:) method will leave them as they are [4][2]. If you need to encode these characters (for example, when constructing a URL query string manually where these characters have special meaning as delimiters), you should either: 1. Use URLComponents: This is the recommended approach for building URLs, as it automatically handles the proper percent-encoding of query items [4][5]. 2. Custom CharacterSet: Create a custom character set by starting with.urlQueryAllowed and removing the characters you want to force to be encoded [1][3]. Example of removing characters from the set: var allowed = CharacterSet.urlQueryAllowed allowed.remove(charactersIn: "&+") let encodedString = myString.addingPercentEncoding(withAllowedCharacters: allowed) [1][3]
Citations:
- 1: https://stackoverflow.com/questions/39818301/swift-encoding-and-decoding-string-for-special-characters
- 2: https://stackoverflow.com/questions/41561853/couldnt-encode-plus-character-in-url-swift
- 3: https://stackoverflow.com/questions/34102116/how-to-escape-ampersand-in-url-query
- 4: https://stackoverflow.com/questions/43052657/encode-using-urlcomponents-in-swift
- 5: https://nshipster.com/characterset/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file =="
fd -a 'ArtworkFetcher.swift' . || true
echo "== relevant lines =="
file="$(fd 'ArtworkFetcher.swift' . | head -n 1)"
if [ -n "${file:-}" ]; then
nl -ba "$file" | sed -n '80,145p'
fi
echo "== references to query construction/ArtworkFetchError =="
rg -n "addingPercentEncoding|urlQueryAllowed|ArtworkFetchError|searchURL|term=" . -g '*.swift'
echo "== Swift availability =="
if command -v swift >/dev/null 2>&1; then
swift --version | head -n 1
python3 - <<'PY'
# Read-only behavioral model of Swift's public documentation for addingPercentEncoding
# does not modify repository files; using installed swift is only for availability check.
from pathlib import Path
print("swift availability ok")
PY
else
echo "swift unavailable"
fiRepository: tukuyomil032/Perch
Length of output: 305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file =="
fd -a 'ArtworkFetcher.swift' . || true
echo "== relevant lines =="
file="$(fd 'ArtworkFetcher.swift' . | head -n 1)"
if [ -n "${file:-}" ]; then
nl -ba "$file" | sed -n '80,145p'
fi
echo "== references to query construction/ArtworkFetchError =="
rg -n "addingPercentEncoding|urlQueryAllowed|ArtworkFetchError|searchURL|term=" . -g '*.swift'
echo "== Swift availability =="
if command -v swift >/dev/null 2>&1; then
swift --version | head -n 1
else
echo "swift unavailable"
fiRepository: tukuyomil032/Perch
Length of output: 305
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'ArtworkFetcher.swift' . | head -n 1)"
echo "== file: $file =="
if [ -n "${file:-}" ]; then
sed -n '80,145p' "$file"
fi
echo "== references =="
rg -n "addingPercentEncoding|urlQueryAllowed|ArtworkFetchError|searchURL|term=" -g '*.swift'
echo "== Swift availability =="
if command -v swift >/dev/null 2>&1; then
swift --version | head -n 1
else
echo "swift unavailable"
fiRepository: tukuyomil032/Perch
Length of output: 2597
クエリ値用に / や &・&+・= を除外し、+ を %20 に置くエンコーディングを使ってください
.urlQueryAllowed では /・&・+・= などがパスされず、曲名に / が含まれると search?term=.../... の後がパラメータとして解釈され、Florence + the Machine のような + はスペースに、& はパラメータ区切りに解釈されます。addingPercentEncoding(withAllowedCharacters:) の後で残った + も %20 に置換するか、構造化された URLComponents を使って値をセットしてください。
🤖 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 `@perch/Features/NowPlaying/ArtworkFetcher.swift` around lines 118 - 131,
Update the query encoding in the rawQuery/searchURL construction to use
query-value-safe percent encoding that excludes /, &, +, and =, and converts
spaces to %20 rather than +. Preserve the existing URL construction and
noResultsFound fallback while ensuring song titles, artists, and albums cannot
alter query parameter parsing.
| inner.addTask { | ||
| try? await Task.sleep(for: .seconds(Self.perProviderTimeoutSeconds)) | ||
| await self.logTimeout(providerName) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
キャンセル時に誤った「timed out」ログが出ます。
プロバイダのfetchが先に完了すると inner.cancelAll() が sleep タスクをキャンセルしますが、try? が CancellationError を握りつぶすため処理が続行し、logTimeout が呼ばれてしまいます。結果として fetch 成功時や外側 group.cancelAll() で敗退した各プロバイダに対しても実際にはタイムアウトしていない debug ログが出力され、遅延調査時にミスリードになります。キャンセルとタイムアウトを区別してください。
🩹 キャンセルとタイムアウトの区別
inner.addTask {
- try? await Task.sleep(for: .seconds(Self.perProviderTimeoutSeconds))
- await self.logTimeout(providerName)
- return nil
+ do {
+ try await Task.sleep(for: .seconds(Self.perProviderTimeoutSeconds))
+ } catch {
+ return nil // 他タスクが勝ってキャンセルされた — タイムアウトではない
+ }
+ await self.logTimeout(providerName)
+ return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inner.addTask { | |
| try? await Task.sleep(for: .seconds(Self.perProviderTimeoutSeconds)) | |
| await self.logTimeout(providerName) | |
| return nil | |
| } | |
| inner.addTask { | |
| do { | |
| try await Task.sleep(for: .seconds(Self.perProviderTimeoutSeconds)) | |
| } catch { | |
| return nil // 他タスクが勝ってキャンセルされた — タイムアウトではない | |
| } | |
| await self.logTimeout(providerName) | |
| return nil | |
| } |
🤖 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 `@perch/Features/NowPlaying/LyricsKitFetcher.swift` around lines 61 - 65,
Update the timeout task in the inner task group to stop immediately when
Task.sleep is cancelled, instead of swallowing CancellationError and calling
logTimeout. Only invoke logTimeout when the sleep completes normally, while
preserving the existing timeout return behavior.
| for result in results { | ||
| guard let synced = result["syncedLyrics"] as? String, !synced.isEmpty else { continue } | ||
| guard let synced = result["syncedLyrics"] as? String, !synced.isEmpty else { | ||
| continue | ||
| } | ||
| let lines = LRCParser.parse(synced) | ||
| if !lines.isEmpty { return lines } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
LRCLIB /api/search response JSON fields trackName
💡 Result:
In the LRCLIB /api/search response, the trackName field is a string that represents the title of the track [1][2][3]. The /api/search endpoint returns a JSON array of objects, where each object includes the following fields: id (integer): The unique identifier of the track [3][4]. trackName (string): The title of the track [1][2][4]. artistName (string): The name of the artist [1][2][4]. albumName (string): The name of the album [1][2][4]. duration (number/float): The duration of the track [1][2][3]. instrumental (boolean): Indicates whether the track is instrumental [1][2][3]. plainLyrics (string or null): The plain text lyrics [1][2][4]. syncedLyrics (string or null): The time-synchronized lyrics in LRC format [1][2][4]. Some documentation or secondary sources may also refer to the track name field as name in addition to trackName [3][4].
Citations:
- 1: https://lrclib.net/docs
- 2: https://lrclib.js.org/types/FindLyricsResponse.html
- 3: https://context7.com/tranxuanthang/lrclib/llms.txt
- 4: https://cdn.jsdelivr.net/npm/lrclib-api@2.0.4/dist/index.d.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locates file =="
git ls-files | rg 'LyricsStore\.swift$|LyricsKitFetcher\.swift$|LRCParser\.swift$' || true
echo "== outline LyricsStore =="
ast-grep outline perch/Features/NowPlaying/LyricsStore.swift --view compact 2>/dev/null | head -200 || true
echo "== relevant LyricsStore lines =="
nl -ba perch/Features/NowPlaying/LyricsStore.swift | sed -n '220,285p'
echo "== search provider fetch pattern =="
rg -n "fetchFromProvider|trackName|name|syncedLyrics|lowercased|contains\\(" perch/Features/NowPlaying -SRepository: tukuyomil032/Perch
Length of output: 368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== LyricsStore size and relevant lines =="
wc -l perch/Features/NowPlaying/LyricsStore.swift perch/Features/NowPlaying/LyricsKitFetcher.swift perch/Features/NowPlaying/LRCParser.swift 2>/dev/null || true
sed -n '220,290p' perch/Features/NowPlaying/LyricsStore.swift
echo "== LyricsKitFetcher lines =="
sed -n '1,220p' perch/Features/NowPlaying/LyricsKitFetcher.swift
echo "== occurrences =="
grep -RIn "fetchFromProvider\|trackName\|name\|syncedLyrics\|lowercased\|contains" perch/Features/NowPlaying || trueRepository: tukuyomil032/Perch
Length of output: 14458
/api/search の返却トラック名をタイトル一致でフィルタリングしてください。
syncedLyrics 付きの結果のうち最初的非空 Parse 結果をそのまま返しているため、fuzzy 検索が別トラックを返した場合に誤った歌詞が表示されます。LyricsKitFetcher.fetchFromProvider のように、return result["trackName"] as? String を title と contains 一致させるか、一致しない場合は continue して次の候補を試すようにしてください。
🤖 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 `@perch/Features/NowPlaying/LyricsStore.swift` around lines 260 - 267,
LyricsStoreの検索結果ループで、非空のsyncedLyricsを解析する前にresult["trackName"]がtitleとcontains一致するか検証してください。一致しない結果はcontinueして次の候補を試し、LyricsKitFetcher.fetchFromProviderと同じタイトル判定を再利用し、一致した結果の非空パース結果だけを返してください。
1 participant
Summary
Now Playing コンパクトピル右端の波形（6本バー）に対する2つの品質改善。ユーザー観察（Atoll との比較）とプランに基づき実施。
1. グラデーション方向の統一
全6本バーが同一の top→bottom
LinearGradient（highlight → primary → secondary）を共有するように変更。従来はindex % 3で以下3パターンに分岐しており、bar 2 と bar 5 だけグラデーションが下→上に反転していたため、複数色構成が「バラバラ」に見える原因になっていた。2. 実オーディオへの追従を Atoll に近づける
blendedLevels()から常時35%混ぜていた sin 合成波を削除。実オーディオが得られている時は shaped real level のみを使う（Atoll-style honesty）。合成波はusesSyntheticFallback経路（音キャプチャ不可時）に残存.easeOut(0.055)→.easeOut(0.028)で 30fps 1フレーム以内に収める3. 回帰防止テスト
AudioSpectrumAnalyzerTestsを新規追加。~1秒無音後にピークレベルが 0.10 未満まで減衰することを検証し、release time が旧値に戻された場合に検知できるようにした。Test plan
xcodebuild -scheme perch -configuration Debug build— ビルド通過xcodebuild -scheme perch test -only-testing:perchTests— 全 unit test 通過（新規テスト2本含む）usesSyntheticFallbackの合成波が従来通り動作することを回帰確認Plan file:
/Users/hosiyomi322/.claude/plans/1-1-1-2-drifting-spindle.md🤖 Generated with Claude Code
Summary by CodeRabbit