RichHome layout, artwork stability, Settings Permissions/Now Playing source - #29
Merged
Conversation
… fixes Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Renames the type/file and rewords the handful of comments that named a specific reference app, replacing them with neutral descriptions of the actual design intent (rounder pill chrome, fuller rich-mode layout). Purely mechanical — no behavior change. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…w-playing card The row's alignment was .top, but centerColumn is height-locked to SurfaceMetrics.lyricsColumnHeight (130pt) while NowPlayingCard runs 220-260pt — top-aligning pinned the lyrics box to the row's top edge with dead space below it. Changed to .center. Also gives the HStack an explicit idealWidth. The vendored NookView lays out the expanded surface under an unconstrained (nil) size proposal via .fixedSize(), so without an idealWidth the HStack's own ideal width was just NowPlayingCard's ideal plus the lyrics text's natural width — narrower than the 640pt window floor, so the outer frame(minWidth:) centered the whole narrow HStack as a block instead of letting centerColumn's maxWidth:.infinity actually fill the window. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
progressSection sat right below the artwork/trackInfo row with only the outer VStack's 10pt spacing between them, reading as too tight. Added 8pt of top padding on progressSection itself, leaving its own internal spacing and the gap to controlsSection unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The black circle chip behind the source logo/symbol read as an unexplained decoration. Removed it and enlarged the icon from 18pt to 40pt (roughly a third of the 128pt artwork) so it reads clearly on its own, keeping the existing 0.3s delayed bounce-in. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… download failure lastSpotifyURL was written before the download attempt, so a single transient failure (network flakiness, DNS) permanently blocked retries for that track — the same URL would never pass the dedup guard again. Moved the write to after a successful download; NowPlayingManager's existing needsFetch check (state.artwork == nil) already retries on the next notification when it's missing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
com.apple.Music.playerInfo only fires on Play/Pause toggle or track change (no frequent position notifications like Spotify), so a single failed AppleScript artwork fetch right after a track change had no further chance to retry. Piggybacks on the existing 1.5s pollAppleMusicPosition() loop instead of adding a bespoke timer: retries up to 3 times (~4.5s window) per track when artwork is still nil, then gives up so a track that genuinely has none doesn't retry forever. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…cleanup Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The old ScrollView + gradient mask never actually hid off-window lines — lineOpacity() floored at 0.20-0.65 rather than 0, and the mask only faded the outer 5-8% of the view, so lines beyond the intended 3-4 always showed through faintly. Replaced with a plain VStack rendering exactly the 4 lines in view (fewer if the song has less), computed from activeIndex and crossfaded when the window shifts — no lines exist outside that set, so there's nothing left to peek through. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…the row Centering centerColumn against NowPlayingCard's own height ignores that IslandTopBar (44pt) plus its Divider sit above the whole row — the header's height visibly pulls the true center of the visible pill higher than NowPlayingCard's center alone, so lyrics rendered noticeably low relative to the full panel. Added SurfaceMetrics.headerHeight (an approximation derived from the header's own padding/control sizes) and offset centerColumn up by half of it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The previous fix (.padding(.top, 8)) only widened the gap above progressSection, leaving it 18pt from the artwork row but only 10pt from controlsSection — still visibly closer to the controls than centered. Changed to .padding(.vertical, 8) so both gaps match at 18pt, putting the seek bar block midway between the two. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Dropped trackInfo's top Spacer so title/artist/album pin to the artwork's top edge instead of self-centering in the 128pt row — the enlarged source badge (Phase B7) sits closer to a vertically centered block than a top-aligned one. Also removed the album Text's lineLimit(2) so long album names wrap in full instead of truncating. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Moving the cursor off the expanded surface already collapses it (the existing hover-exit behavior), so the explicit xmark button was redundant chrome. AppState.collapse() itself is untouched — kept as a public API other triggers may use, and still covered by AppStateTests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ModuleSwitcher's HStack spacing (24pt) plus each button's own 6pt horizontal padding read as an unnaturally wide gap between the two icons. Reduced spacing to 8pt. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…eaks Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The custom Info.plist never referenced PRODUCT_BUNDLE_IDENTIFIER, so every build shipped with no CFBundleIdentifier at all — codesign synthesized a different fallback identifier per build environment (confirmed: `just run`'s DerivedData build signed as "perch" with a Development cert/Team ID, while /Applications/perch.app carried an ad-hoc-signed hashed identifier with no team). macOS's Automation TCC grants are scoped per signing identity, so the two were tracked as separate apps — Automation permission granted to one (enabling artwork fetch and playback controls, both AppleScript-driven) simply didn't apply to the other. Verified the built app now reports Identifier=com.tukuyomi032.perch via codesign. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Phase B8 pinned title/artist/album flush to the artwork's top edge to put distance between them and the enlarged source badge — too far up per follow-up feedback. Added 10pt of top padding as a middle ground, still clearly separated from the badge but not pinned to the very top. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Active-line emphasis (spring response 0.40) and window crossfade (easeInOut 0.4s) both read as too abrupt per feedback. Bumped to 0.55 for each — timestamp-driven timing (activeIndex) is unchanged, only how the transition itself moves. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ostics Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Without a usage description string, macOS never shows the Automation permission prompt for AppleScript control of Spotify/Music — it just fails silently (errAEEventNotPermitted, -1743) on the first attempt. This combined with the Phase B9 CFBundleIdentifier fix (which makes the app a genuinely new TCC identity requiring fresh consent) left artwork fetching and playback controls broken on every launch path, since there was no way to ever get prompted for permission. Verified the string now appears in the built app's Info.plist. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Previously swallowed errors silently, making it impossible to tell a permission denial (-1743) from any other failure when playback controls or artwork stopped working. Logs the NSAppleScript error number and message at debug level. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Added a Logger (matching NowPlayingManager's pattern) and logging on both AppleScript failure paths (fetchAppleMusicArtworkData, the shared runAppleScript used by Spotify's URL fetch), distinguishing permission denial (-1743) from other AppleScript errors and from a track that simply has no artwork. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Switched the VStack, multilineTextAlignment, and each line's frame alignment from center to leading per feedback. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tatus New @observable store following CalendarStore's authorization-state pattern. refreshAll() preflight-checks all four permissions without ever prompting (AEDeterminePermissionToAutomateTarget with askUserIfNeeded: false for Automation, CGPreflightScreenCaptureAccess for Screen Recording, EKEventStore.authorizationStatus for Calendar). request(_:) triggers the real OS prompt when a permission is still notDetermined; once denied, macOS won't show another system dialog no matter how many times the app asks, so request(_:) opens the relevant System Settings pane instead. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Lists Calendar, Screen Recording, and Automation (Spotify/Apple Music) with a live status (color-coded) and a per-row action button — "request" when not yet determined, "open System Settings" once denied (macOS won't show another system prompt for a denied permission, so that's the only remaining path). Status refreshes via PermissionsStore.refreshAll() on .onAppear. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
en/ja pairs for the tab label, section header, permission names, status labels, and action buttons. Verified both bundle correctly into the built app's .lproj resources. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
420pt was sized for 6 tabs; adding Permissions in Phase B11 pushed it to 7, which no longer fit and pushed Permissions/Updates into TabView's ">>" overflow menu — unreachable per user report. Widened to 600pt so all tabs render in a single row. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
User reconsidered the Phase B10 left-alignment change and asked to go back to centered. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Bumped LyricsView's default fontSize and NowPlayingLyricsColumn's explicit call site to match. Updated lyricsColumnHeight's derivation comment for the new font size — the 130pt value itself is unchanged, still a floor with a little slack over the ~118pt estimate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tup detection Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ger font size Phase B12's font bump (13pt -> 15pt) pushed some lyric lines from 2 wrapped lines to 3, hitting lineLimit(2) and truncating with an ellipsis instead of wrapping fully. scaleEffect on the active line doesn't affect layout/wrapping (it's a post-layout render transform), so this affected any line long enough to need a third line, not just the active one. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…e picker Two bugs fixed: 1. The three Spotify/Apple Music/YouTube Music toggles were checked only when a new notification arrived, with no way to clear an already-active state or react to the toggle itself changing — turning a source off while it was playing left its activity showing indefinitely. Replaced with a single NowPlayingSourcePreference (auto/spotify/appleMusic/youTubeMusic), gated once in applyState, and observed via Defaults.observe so a change clears currentState immediately when it no longer matches. 2. A track already playing when Perch launches was never detected — Spotify/Music's DistributedNotifications only fire on a state *transition*, so nothing arrived until the user paused/resumed. Added active AppleScript queries (detectActiveSpotifyState, detectActiveAppleMusicState) plus a MediaRemoteBridge.currentState read for YouTube Music, run once at launch and again whenever the preferred source changes. Settings' NowPlaying tab now shows one menu-style Picker instead of three toggles, matching the single-select pattern requested. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follows migrateIslandChromeStyle's shape: gated on the legacy keys actually having been written (object(forKey:) != nil), consuming them once migrated. Exactly one enabled source carries over as that explicit choice; the old all-enabled default and any other combination fall back to .auto. Added PreferencesMigrationTests coverage for the pure mapping function and the store-level migration. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
📝 WalkthroughWalkthroughNow Playing のソース選択と旧設定移行を追加した。起動時検出、権限管理、アートワーク取得、歌詞表示、Rich UI のレイアウトと名称を更新した。 ChangesNow Playing ソース設定と移行
Now Playing 検出と権限
Rich UI と Now Playing 表示
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsView
participant NowPlayingManager
participant Spotify
participant AppleMusic
participant YouTubeMusic
SettingsView->>NowPlayingManager: preferredNowPlayingSource の変更を通知する
NowPlayingManager->>Spotify: 再生状態を取得する
NowPlayingManager->>AppleMusic: 再生状態を取得する
NowPlayingManager->>YouTubeMusic: MediaRemote 状態を取得する
NowPlayingManager->>SettingsView: 選択ソースの状態を表示する
Possibly related PRs
Poem
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
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
1 participant
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.
Summary
Phase B7〜B13の実機フィードバック対応をまとめたPR。
シークバー位置、trackInfo位置、ヘッダー整理）
AtollStyleExpandedView→RichHomeView（参考先アプリ名の除去、機械的リネーム）NSAppleEventsUsageDescription欠落によりAutomation許可プロンプトが一切出ない不具合を修正CFBundleIdentifier欠落による起動方法別の権限不整合を修正Now Playingソース選択を独立トグル3つから排他Picker（自動/Spotify/Apple Music/YouTube Music）
に置き換え、設定ウィンドウ拡幅
Test plan
xcodebuild build成功を確認済みxcodebuild test -only-testing:perchTests全体を複数回実行、回帰なしを確認済みPreferencesMigrationTestsに新規移行ロジックのテストを追加🤖 Generated with Claude Code
https://claude.ai/code/session_01EFKkNRhr4QbyBdZKNtPm8d
Summary by CodeRabbit
新機能
改善