Address review feedback: Add CI, MIT license, template Info.plist, an…#6
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds continuous integration and release automation via GitHub Actions workflows, introduces an Info.plist template to replace inline XML generation, establishes MIT licensing, and refactors VinylApp's playback logic to respond to system notifications (Apple Music and Spotify) while simplifying the polling loop by extracting the core fetch and update logic into a reusable method. ChangesVinyl Application Build and Playback Updates
Sequence DiagramsequenceDiagram
participant DistribNotif as DistributedNotificationCenter
participant AppDelegate as AppDelegate
participant PerformFetch as performFetch
participant LyricFetch as LRCLIB / Apple Music
participant MenuBar as MenuBarEngine
DistribNotif->>AppDelegate: playerStateDidChange (Apple Music or Spotify)
AppDelegate->>PerformFetch: async performFetch()
PerformFetch->>PerformFetch: fetchCurrentState()
PerformFetch->>PerformFetch: detect track key change
PerformFetch->>LyricFetch: LRCLIB fetch (primary)
LyricFetch-->>PerformFetch: lyrics or error
PerformFetch->>LyricFetch: Apple Music native (fallback if Music player)
LyricFetch-->>PerformFetch: native lyrics
PerformFetch->>MenuBar: update(state, lyrics, artwork)
MenuBar->>MenuBar: render scrolled title from lyrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Sources/VinylApp.swift (1)
80-82: ⚡ Quick winRedundant cache write triggers unnecessary disk I/O.
When cached lyrics exist, calling
setLyricsLoaded(cached, forKey: newTrackKey)re-writes the same lyrics to the cache and callssaveCacheToDisk()unnecessarily (seeAppStateActor.swift:58-63).♻️ Proposed fix
Update only the in-memory state without re-caching:
if let cached = await stateActor.getCachedLyrics(forKey: newTrackKey) { - await stateActor.setLyricsLoaded(cached, forKey: newTrackKey) + await stateActor.setLyricsLoaded(cached) } else {This requires adding a new method to
AppStateActorthat updates state without re-caching:public func setLyricsLoaded(_ lyrics: [LyricLine]) { self.lyricsStatus = .loaded self.currentLyrics = lyrics // Skip cache write and disk save }🤖 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/VinylApp.swift` around lines 80 - 82, The current code calls stateActor.setLyricsLoaded(cached, forKey: newTrackKey) even when cached lyrics exist, causing redundant cache write and saveCacheToDisk(); change this to update only in-memory state by adding a new AppStateActor method like setLyricsLoaded(_ lyrics: [LyricLine]) that sets lyricsStatus and currentLyrics without invoking caching or saveCacheToDisk, then replace the call to stateActor.setLyricsLoaded(cached, forKey: newTrackKey) with stateActor.setLyricsLoaded(cached) so cached reads do not trigger disk I/O.
🤖 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 @.github/workflows/release.yml:
- Around line 24-27: The workflow uses the deprecated action
softprops/action-gh-release@v1 (in the job step named "Create Release"); update
the step to use softprops/action-gh-release@v3 to avoid Node-16 runtime issues
on modern runners and ensure the existing input (files: Vinyl.zip) remains
unchanged—locate the step using the "name: Create Release" or the uses:
softprops/action-gh-release@v1 and replace the version tag with `@v3`.
In `@Sources/VinylApp.swift`:
- Around line 29-40: The DistributedNotificationCenter observers registered via
DistributedNotificationCenter.default().addObserver(...) for selector
`#selector`(playerStateDidChange) (names "com.apple.Music.playerInfo" and
"com.spotify.client.PlaybackStateChanged") are never removed, causing
AppDelegate to leak; fix by removing those observers when the app terminates or
when AppDelegate deinitializes—call
DistributedNotificationCenter.default().removeObserver(self) (or
removeObserver(self, name: NSNotification.Name("..."), object: nil) for each
specific name) from applicationWillTerminate(_:) or deinit of AppDelegate to
ensure the observers are unregistered and allow deallocation.
- Around line 85-121: The current untracked Task that fetches lyrics for a Track
can outlive the track and overwrite state for a newer track; replace it with a
cancellable tracked Task stored (e.g., lyricsFetchTask) on the surrounding scope
or actor, cancel any existing lyricsFetchTask when the track changes, and then
assign a new Task that performs the same fetch logic (calling
client.fetchLyrics, bridge.fetchAppleMusicLyrics, client.parseLyrics) and
updates state via stateActor.setLyricsLoaded / setLyricsError only if the Task
has not been cancelled; ensure you reference and cancel the previous
lyricsFetchTask before creating a new one and use Task.checkCancellation or
check Task.isCancelled before writing to state to avoid stale writes.
- Around line 43-47: playerStateDidChange spawns untracked concurrent Tasks
calling performFetch which leads to races on currentTrackKey and duplicated
network/artwork/lyrics work; introduce a stored Task? property (e.g.,
notificationFetchTask) on the same type and, inside playerStateDidChange, cancel
any existing notificationFetchTask before creating and assigning a new Task that
calls await performFetch(), and ensure performFetch (and the artwork/lyrics
sub-tasks it spawns) cooperatively handle cancellation (check Task.isCancelled
or use withTaskCancellationHandler) so stale fetches stop promptly and only one
notification-driven fetch runs at a time.
---
Nitpick comments:
In `@Sources/VinylApp.swift`:
- Around line 80-82: The current code calls stateActor.setLyricsLoaded(cached,
forKey: newTrackKey) even when cached lyrics exist, causing redundant cache
write and saveCacheToDisk(); change this to update only in-memory state by
adding a new AppStateActor method like setLyricsLoaded(_ lyrics: [LyricLine])
that sets lyricsStatus and currentLyrics without invoking caching or
saveCacheToDisk, then replace the call to stateActor.setLyricsLoaded(cached,
forKey: newTrackKey) with stateActor.setLyricsLoaded(cached) so cached reads do
not trigger disk I/O.
🪄 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: 2cc7799b-3d42-45f7-a92a-c82dca0da172
📒 Files selected for processing (9)
.github/workflows/ci.yml.github/workflows/release.ymlInfo.plist.templateLICENSEMakefileREADME.mdSources/MenuBarEngine.swiftSources/VinylApp.swiftTests/VinylTests/VinylTests.swift
💤 Files with no reviewable changes (1)
- Sources/MenuBarEngine.swift
1 participant
…d fix polling
Summary by CodeRabbit
New Features
Documentation
Chores