fix(#265): モニタ抜き差し後のレイアウト崩れとアートワーク消失を修復#266
Conversation
モニタの抜き差し中は window server がアプリの裏でオーバーレイウィンドウを 移動・リサイズするが、従来は resolveLayout() のモデル値が変化したときしか applyLayout が走らないため (removeDuplicates によるゲート)、モデル値が 同じままシステム側だけが動かしたズレを永遠に修復できなかった。 hostingView (autoresizing なし) と AVPlayerLayer (比例 autoresizing のみ) が実ウィンドウと食い違ったまま固定され、歌詞の重なり・壁紙の偏り・ ヘッダ崩れとして現れる。再起動で直るのはこのため。 - AppPresenter.onWindowFrameChange: removeDuplicates を撤去し、 解決レイアウトの emit ごとに毎回再適用する - AppWindow.apply: 実状態と照合する冪等な再適用に変更。windowFrame が 一致していれば setFrame をスキップ (通知ループ防止)、AVPlayerLayer は transform を保ったまま bounds + position で幾何を再アサート - ScreenInteractor.screenChanges: NSWindow.didMove / didResize 通知を merge し、システムがウィンドウを動かした直後にも再解決が走るようにする
MediaRemote の再ブロードキャスト (ディスプレイ再構成時などに発生) は artwork バイトを含まないことがあり、artwork ストリームが artworkData を 素通ししていたため表示中のアートワークが nil で上書きされて消えていた。 scan で直前のトラックと比較し、同一トラック (sameTrack 判定) の間は 最後に得たアートワークを保持する。トラックが変わって新トラックに アートワークが無い場合は従来どおり nil を流してクリアする。
|
Caution Review failedPull request was closed or merged during review 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 (10)
📝 WalkthroughWalkthroughIdempotent geometry reconciliation introduced for AppWindow.apply; ScreenInteractor emits on window move/resize; AppPresenter invokes layout reassertion for every resolved layout emission (no frame dedup); TrackInteractor preserves artwork across same-track nil updates; docs updated and version bumped to 2.13.8. ChangesMonitor hot-plug layout recovery
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
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/TrackInteractorArtworkTests.swift`:
- Around line 167-168: Replace the fixed Task.sleep by polling with a deadline:
repeatedly check collector.snapshot for equality with [realArt] in a short-loop
(sleeping tiny increments, e.g. Task.sleep for a few milliseconds) until a
deadline (Date()+timeout) is reached, then assert collector.snapshot ==
[realArt] and additionally assert that no further emissions appear after that
deadline; remove the one-off try? await Task.sleep(for: .milliseconds(100)) and
implement the deadline-based loop in TrackInteractorArtworkTests using the
existing collector and realArt symbols to locate the check.
🪄 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: 8218b98a-c2d7-48ca-a5f7-db7926b24d3c
📒 Files selected for processing (10)
.claude/CLAUDE.mdSources/Presenters/App/AppPresenter.swiftSources/ScreenInteractor/ScreenInteractorImpl.swiftSources/TrackInteractor/TrackInteractorImpl.swiftSources/VersionHandler/Resources/version.txtSources/Views/Overlay/AppWindow.swiftTests/PresentersTests/AppPresenterTests.swiftTests/ScreenInteractorTests/ScreenInteractorImplTests.swiftTests/TrackInteractorTests/TrackInteractorArtworkTests.swiftTests/ViewsTests/AppWindowTests.swift
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
CodeRabbit 指摘 (no fixed sleep guideline) への対応。同一トラックの nil イベント後に必ず emit される次トラックのイベントをフェンスとし、 イベントが順序どおり処理される性質を利用して「nil が emit されなかった」 ことを決定的に検証する。
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
CodeRabbit の Docstring Coverage 警告に対応するため、今回の PR で変更のあった主要クラス・メンバにドキュメントコメントを追加しました。
| public lazy var artwork: AnyPublisher<Data?, Never> = | ||
| activeNowPlaying | ||
| .map(\.artworkData) | ||
| .scan((track: NowPlaying?.none, data: Data?.none)) { state, incoming in |
There was a problem hiding this comment.
同一トラック判定に Self.sameTrack を使用していますが、NowPlaying の title と artist が共に nil の場合(未知のトラック同士)も true を返します。通常、再生中であればタイトルは存在しますが、より安全にするならタイトルが非 nil であることも条件に含めることを検討してください。
| .scan((track: NowPlaying?.none, data: Data?.none)) { state, incoming in | |
| let isSameTrack = state.track.map { $0.title != nil && Self.sameTrack($0, incoming) } ?? false |
There was a problem hiding this comment.
しました。ご提案どおり artwork の同一トラック判定に title 非 nil 条件を追加し、未知トラック同士で artwork が引き継がれないことを検証する回帰テスト (
artworkDoesNotStickAcrossNilTitleTracks) も追加しています。740cdae を参照。
| } | ||
| guard let containerView = surface.contentView, containerView !== hostingView else { return } | ||
| containerView.frame = CGRect(origin: .zero, size: layout.windowFrame.size) | ||
| guard let playerLayer = playerLayer(in: surface) else { return } |
There was a problem hiding this comment.
containerView.frame を設定する前に、現在のフレームと比較するガードを入れることで、不要な AppKit のレイアウトパスをさらに削減できます(applyWindowFrame と同様のパターン)。
| guard let playerLayer = playerLayer(in: surface) else { return } | |
| if containerView.frame != CGRect(origin: .zero, size: layout.windowFrame.size) { | |
| containerView.frame = CGRect(origin: .zero, size: layout.windowFrame.size) | |
| } |
There was a problem hiding this comment.
しました。提案の if 文形式ではなく、既存の
applyWindowFrame と揃えた guard ヘルパー (applyFrame(_:to:)) として実装し、containerView に加えて hostingView の frame 再代入にも同じスキップを適用しています。188edab を参照。
| public func onWindowFrameChange(_ handler: @escaping @MainActor (ScreenLayout) -> Void) { | ||
| $layout | ||
| .removeDuplicates { $0.windowFrame == $1.windowFrame } | ||
| .dropFirst() |
レビュー指摘対応。title が両方 nil の場合 sameTrack は true を返すため、 未知トラック同士で前の artwork が残り続ける可能性があった。artwork の 同一トラック判定に title 非 nil 条件を追加し、回帰テストを追加。
レビュー指摘対応。applyWindowFrame と同じ guard ヘルパー形式で 不要な AppKit レイアウトパスを削減。
|
@coderabbitai full review |
✅ Action performedFull review finished. |
      Closes #267 ## <img src="https://mojiemoji.jozo.beer/emoji/%E6%A6%82%E8%A6%81?font=toge&color=f43f5e&animation=kage_neon&background=transparent&outline=5ef43f&outline_width=2" alt="概要" height="24" align="absmiddle"> CodeRabbit の Pre-merge check「Docstring Coverage」を `.coderabbit.yaml` で<img src="https://mojiemoji.jozo.beer/emoji/%E7%84%A1%E5%8A%B9?font=zero&color=4ade80&animation=mabataki&background=transparent&outline=4a16a3&outline_width=2" alt="無効" height="24" align="absmiddle">化する。 ## 背景 <img src="https://mojiemoji.jozo.beer/emoji/%E8%A8%88%E6%B8%AC?font=dela&color=22c55e&animation=kirari&background=transparent&outline=5e22c5&outline_width=2" alt="計測" height="24" align="absmiddle">エンジンが Swift の `///` doc comment を認識せず、coverage が常に 0.00% と<img src="https://mojiemoji.jozo.beer/emoji/%E5%A0%B1%E5%91%8A?font=toge&color=f472b6&animation=yokomoya&background=transparent&outline=77db27&outline_width=2" alt="報告" height="24" align="absmiddle">される。<img src="https://mojiemoji.jozo.beer/emoji/PR?font=hachimaru&color=ef4444&animation=bure&background=transparent&outline=44ef44&outline_width=2" alt="PR" height="24" align="absmiddle"> #266 で実証済み: <img src="https://mojiemoji.jozo.beer/emoji/%E5%A4%89%E6%9B%B4?font=gothic&color=22d3ee&animation=tate_scroll&background=transparent&outline=b20891&outline_width=2" alt="変更" height="24" align="absmiddle"><img src="https://mojiemoji.jozo.beer/emoji/%E5%AF%BE%E8%B1%A1?font=maru-bold&color=8b5cf6&animation=psycho&background=transparent&outline_width=0" alt="対象" height="24" align="absmiddle">の public シンボルすべてに docstring が付いた状態で `@coderabbitai full review` により再計算させても `0.00% (required 80.00%)` のまま変化しなかった。<img src="https://mojiemoji.jozo.beer/emoji/%E8%A8%88%E6%B8%AC?font=dela&color=22c55e&animation=kirari&background=transparent&outline=5e22c5&outline_width=2" alt="計測" height="24" align="absmiddle">不能なチェックが全 <img src="https://mojiemoji.jozo.beer/emoji/PR?font=hachimaru&color=ef4444&animation=bure&background=transparent&outline=44ef44&outline_width=2" alt="PR" height="24" align="absmiddle"> に<img src="https://mojiemoji.jozo.beer/emoji/%E8%AD%A6%E5%91%8A?font=noto&color=fb7185&animation=kirari&background=transparent&outline=85fb71&outline_width=2" alt="警告" height="24" align="absmiddle">を出し続けるのはノイズでしかないため off にする。 ## <img src="https://mojiemoji.jozo.beer/emoji/%E5%A4%89%E6%9B%B4?font=hachimaru&color=eab308&animation=yokomoya&background=transparent&outline=08eab3&outline_width=2" alt="変更" height="24" align="absmiddle"><img src="https://mojiemoji.jozo.beer/emoji/%E5%86%85%E5%AE%B9?font=gothic&color=fdba74&animation=kira&background=transparent&outline_width=0" alt="内容" height="24" align="absmiddle"> - `.coderabbit.yaml` を<img src="https://mojiemoji.jozo.beer/emoji/%E6%96%B0%E8%A6%8F?font=kurobara&color=f472b6&animation=disco&background=transparent&outline_width=0" alt="新規" height="24" align="absmiddle">作成し `reviews.pre_merge_checks.docstrings.mode: "off"` を<img src="https://mojiemoji.jozo.beer/emoji/%E8%A8%AD%E5%AE%9A?font=maru&color=60a5fa&animation=yurayura&background=transparent&outline=eb2563&outline_width=2" alt="設定" height="24" align="absmiddle"> - yaml-language-server 向けの `$schema` コメントを先頭に付与(公式 schema.v2.json で `mode: off | warning | error` を<img src="https://mojiemoji.jozo.beer/emoji/%E7%A2%BA%E8%AA%8D?font=toge&color=f59e0b&animation=zanzo&background=transparent&outline=0bf59e&outline_width=2" alt="確認" height="24" align="absmiddle">済み) - <img src="https://mojiemoji.jozo.beer/emoji/%E7%84%A1%E5%8A%B9?font=mincho&color=fbbf24&animation=yoko_scroll&background=transparent&outline=06d977&outline_width=2" alt="無効" height="24" align="absmiddle">化の<img src="https://mojiemoji.jozo.beer/emoji/%E7%90%86%E7%94%B1?font=rampart&color=f87171&animation=disco&background=transparent&outline_width=0" alt="理由" height="24" align="absmiddle">をファイル内コメントに記録 - バージョンを 2.13.9 に patch バンプ ## <img src="https://mojiemoji.jozo.beer/emoji/%E7%A2%BA%E8%AA%8D?font=noto&color=fb7185&animation=mochimochi&background=transparent&outline=85fb71&outline_width=2" alt="確認" height="24" align="absmiddle">方法 この <img src="https://mojiemoji.jozo.beer/emoji/PR?font=hachimaru&color=ef4444&animation=bure&background=transparent&outline=44ef44&outline_width=2" alt="PR" height="24" align="absmiddle"> 自体の CodeRabbit walkthrough の Pre-merge checks から Docstring Coverage <img src="https://mojiemoji.jozo.beer/emoji/%E8%AD%A6%E5%91%8A?font=noto&color=fb7185&animation=kirari&background=transparent&outline=85fb71&outline_width=2" alt="警告" height="24" align="absmiddle">が消えていれば<img src="https://mojiemoji.jozo.beer/emoji/%E8%A8%AD%E5%AE%9A?font=maru-bold&color=f472b6&animation=mochimochi&background=transparent&outline=b6f472&outline_width=2" alt="設定" height="24" align="absmiddle">が効いている。
Closes #265
根本原因
1. レイアウト崩れ(歌詞の重なり・壁紙の偏り)
ディスプレイ再構成中、window server はアプリの裏でウィンドウを移動・リサイズする(切断ディスプレイからの退避、再接続時の復元など)。一方 lyra がジオメトリを再適用するのは モデル値 (
resolveLayout()の結果) が前回と変わったときだけ だった:AppPresenter.onWindowFrameChangeがremoveDuplicates { $0.windowFrame == $1.windowFrame }でゲートズレたウィンドウ内では、AppKit が contentView を自動リサイズする一方:
hostingView(autoresizing なし)は古いhostingFrameのまま → SwiftUI の GeometryReader が誤ったサイズで歌詞カラムを組む → 行の重なり・はみ出しAVPlayerLayerは attach 時の frame +比例 autoresizing 頼みで再適用されない → 壁紙が画面の一部に偏る抜き差しを繰り返すほどズレが固定化し、再起動(全ジオメトリ再構築)でのみ直る — issue の症状と一致します。
2. アートワーク消失
TrackInteractorImpl.artworkがmap(\.artworkData)の素通しだったため、同一トラックの artwork なし再ブロードキャスト(ディスプレイ再構成時などに MediaRemote が発する)で表示中のアートワークが nil 上書きされていた。修正方針 — 照合は履歴とでなく実状態と
AppPresenter.onWindowFrameChangeremoveDuplicatesを撤去。解決レイアウトの emit ごとに毎回再適用AppWindow.applysurface.frameが一致していればsetFrameをスキップ(通知ループ防止)し、container / hostingView /AVPlayerLayerのジオメトリを毎回再アサート。playerLayer は wallpaper scale の transform を保持するためframeでなくbounds+positionで設定ScreenInteractor.screenChangesNSWindow.didMove/didResizeを merge。システムがウィンドウを動かした直後にも再解決が走る(apply 側が冪等なので自己発火はループしない)TrackInteractorImpl.artworkscanで直前トラックと比較し、同一トラック(sameTrack判定)の間は最後のアートワークを保持。トラック変更時の nil は従来どおりクリアループ安全性の説明
applyLayoutがsetFrame→didResize通知 →screenChanges→ 再解決 →applyという循環が起きうるが、2 周目ではsurface.frame == layout.windowFrameが成立してsetFrameがスキップされるため、通知が再発火せず 1 バウンドで収束する。vacantモードの 5 秒ポーリングも同様に no-op になる。テスト
AppPresenterTests: 旧仕様テスト(dedup を検証)を新仕様(同一フレームでも毎回発火)に置き換えAppWindowTests: ① frame 一致時のsetFrameスキップ、② システムに改変された container / hostingView / playerLayer ジオメトリの修復、③ wallpaper scale transform の保持ScreenInteractorImplTests:NSWindow.didMove/didResizeでの emit(parameterized)TrackInteractorArtworkTests: 同一トラック nil イベントでの保持+トラック変更時のクリアswift test全 924 件パス(2 連続)、make lintクリーン。検証の限界
実モニタの抜き差しを伴う再現は手元で実施できていないため、根本原因の特定はコード解析とスクリーンショットの幾何(壁紙レイヤが部分領域に固定)に基づきます。マージ前にデバッグビルドでの実機確認(
.claude/rules/dev-verification.mdのフロー)を推奨します。TODO
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores