chore: production polish — code quality, UX text, and AppDelegate decomposition#14
Merged
nathanialhenniges merged 20 commits intomainfrom Apr 4, 2026
Merged
chore: production polish — code quality, UX text, and AppDelegate decomposition#14nathanialhenniges merged 20 commits intomainfrom
nathanialhenniges merged 20 commits intomainfrom
Conversation
- Extract PlaybackSource protocol + PlaybackSourceMode enum - Refactor MusicPlaybackMonitor → AppleMusicSource (same logic, new abstraction) - Add SystemNowPlayingSource using private MediaRemote framework (dlopen/dlsym) captures playback from Spotify, browsers, and any system Now Playing app - Add PlaybackSourceManager to switch sources and forward delegate callbacks - Add Music Source segmented picker to MusicMonitorSettingsView (Apple Events warning hidden when System mode is selected) - Update AppDelegate to use PlaybackSourceManager; Apple Music remains default - Add AppConstants for playbackSourceMode, PlaybackSourceModeChanged, SystemNowPlaying - Add PlaybackSourceManagerTests and SystemNowPlayingSourceTests (693 tests, 0 failures) - Add FEATURE_IDEAS.md with 8 future feature ideas - Sync docs changelog with v1.0.2, v1.1.0, and v1.2.0 entries - Bump version to 1.2.0, build number to 5 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- SystemNowPlayingSource: use AppConstants for frameworkPath, notification name, and queue label instead of local duplicates - SystemNowPlayingSource: check dlsym result for NULL before unsafeBitCast - SystemNowPlayingSource: dispatch unavailable status via notifyDelegate (main thread) - PlaybackSourceManager: use AppConstants.UserDefaults.playbackSourceMode key in both init() and switchMode(_:) instead of hardcoded string - MusicMonitorSettingsView: use AppConstants.UserDefaults.playbackSourceMode in @AppStorage; add accessibilityLabel/accessibilityIdentifier to picker and accessibilityElement to system mode info row - SystemNowPlayingSourceTests: rename misleading test to testDelegateWiringDoesNotCrash - CHANGELOG: fix "macOS system Now Playing API" → "macOS Now Playing API" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PlaybackSourceManagerTests crashed on every test with "pointer being freed was not allocated" on a background thread. Root cause: AppleMusicSource was eagerly created as a stored property in PlaybackSourceManager.init, racing with a still-in-flight backgroundQueue async block from the previous AppleMusicSourceTests teardown on CI runners. Making appleMusicSource (and systemNowPlayingSource) lazy defers allocation until startTracking() is called. No tests in PlaybackSourceManagerTests call startTracking(), so no AppleMusicSource is ever created during those tests, eliminating the race. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CRITICAL BUG FIX: playbackSourceModeChanged notification had no observer in AppDelegate, so switching the Music Source picker never took effect at runtime. Added the missing observer — switchMode() is now called immediately when the user changes the picker. SOURCE DETECTION: SystemNowPlayingSource now resolves MRMediaRemoteGet- NowPlayingApplicationPID via dlsym to detect which app is playing (Spotify, Chrome, etc.). Bundle ID is surfaced via new PlaybackSourceDelegate method playbackSource(_:didDetectSourceApp:) with a default no-op extension for backward compatibility. SOURCE-AWARE DISCORD RPC: - Small icon + tooltip now show the actual playing app (Spotify logo, Apple Music logo, YouTube/Chrome icon, or generic music note) - "Open in Apple Music" button only appears when Apple Music is the source - Large image fallback uses the source-specific asset instead of always showing apple_music TWITCH COMMANDS: getCurrentSongInfo/getLastSongInfo no longer gate on isMusicAppOpen() when in system mode — Spotify/Chrome tracks now respond correctly. Non-Apple-Music sources append "via Spotify" etc. UI TEXT: Updated settings and onboarding descriptions to be source-agnostic. DISCORD ASSETS: Added discord-assets/ folder with placeholder PNGs for spotify, youtube, and music_generic. Upload these to Discord Developer Portal → Rich Presence → Art Assets (see discord-assets/README.md). Added KnownMusicApps helper in AppConstants with displayName(for:), discordAssetName(for:), isAppleMusic(_:), isBrowser(_:). Tested in KnownMusicAppsTests.swift (16 new tests). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The private MediaRemote framework is blocked by kMRMediaRemoteFrameworkErrorDomain Code=3 on macOS 26 (Tahoe). The mediaremoted daemon enforces private Apple entitlements (com.apple.private.mediaremote.nowplayinginfo) that third-party apps cannot obtain — no public API exists to read now-playing info from other apps. - Delete SystemNowPlayingSource.swift and all related tests - Remove PlaybackSourceMode.systemNowPlaying, didDetectSourceApp delegate method - Simplify PlaybackSourceManager to always use AppleMusicSource - Remove KnownMusicApps, SystemNowPlaying enums from AppConstants - Remove source picker from MusicMonitorSettingsView - Remove source-aware branding from DiscordRPCService and WolfWaveApp - Revert docs (index, features, usage, changelog) to Apple Music language - Revert entitlements: remove com.apple.mediaremoted mach-lookup exception - Update WhatsNewView with v1.1.0 highlights (pending v1.2.0 feature set) - Fix @mainactor isolation for Twitch bot callbacks (MainActor.assumeIsolated) - Move CHANGELOG v1.2.0 entry to [Unreleased] — more features coming Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add standard // FileName.swift / wolfwave / Created by… headers to AppleMusicSource, PlaybackSource, and PlaybackSourceManager (all previously missing). Fix mismatched header in MusicPlaybackMonitorTests (was AppleMusicSourceTests.swift). Remove stale DispatchQueues.systemNowPlaying constant and the now-dead testInvalidPersistedModeFallsBackToAppleMusic test that referenced the removed playbackSourceMode UserDefaults key. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AppleMusicSource: add trackingLock (NSLock) to synchronize isTracking,
which was read on backgroundQueue by the fallback timer but written on
the main thread by startTracking/stopTracking — a data race.
WebSocketServerService: standardize manual lock()/unlock() calls in
setEnabled() and scheduleRetry() to withLock {} to prevent deadlocks
on thrown exceptions, matching the pattern used elsewhere in the file.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
DiscordRPCService: readDiscordTmpDir() ran sysctl(KERN_PROCARGS2) on every connectIfNeeded() call. Cache the result with a 30s TTL — Discord's TMPDIR doesn't change while it's running, so repeated kernel queries were wasteful. ArtworkService: the three in-memory caches (artwork, trackView, songLink) grew unbounded. Add insertion-order tracking via cacheKeyOrder and evict the oldest entry across all three caches when the count exceeds 200, keeping memory usage proportional to active usage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l keys Add globalCooldownKey and userCooldownKey as protocol requirements on BotCommand (with nil defaults in an extension). TrackInfoCommand accepts these via init and stores them as let properties. BotCommandDispatcher now derives the canonical cooldown bucket from command.triggers.first instead of a hardcoded switch statement, and cooldownValues(for:command:) reads UserDefaults keys directly from the command rather than pattern-matching on trigger strings. Adding a new command no longer requires touching the dispatcher. Note: keys must be declared in the protocol body (not just an extension) to enable dynamic dispatch through the BotCommand existential — extension- only methods are statically dispatched and would always return nil. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace OAuth/developer terminology throughout the app with plain language appropriate for streamers: - "Authorize" / "Re-auth" / "Device Code" → "Sign in" / "Sign-in Code" - "Twitch Integration" → "the Twitch bot" - "local server" / "Starts the local server" → "powers the widget" - "saved credentials" → "keep you logged in" - "Validation error" → "Couldn't check channel" - "reconfigure your integrations" → "set up your connections again" - "Reset Onboarding?" → "Rerun Setup Wizard?" - "debugging and support" → "troubleshoot issues" Standardize terminology: - Discord status labels: "Active"/"Idle" → "Connected"/"Disconnected" - Cooldown labels: "Global"/"Per-user" → "Everyone"/"Per person" - Account label: "Bot Account" → "Twitch Account" Update WhatsNewView feature descriptions for v1.1.0 to use plain copy. Remove duplicate getCurrentSongInfo/getLastSongInfo callback wiring from SettingsView.onAppear — AppDelegate.setupTwitchService() is the single source of truth for these closures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WolfWaveApp.swift was ~1,241 lines — a single file mixing menu construction, window management, service orchestration, and playback delegation. Split into four focused files: - WolfWaveApp.swift (~150 lines): core class definition, stored properties, lifecycle hooks, postNowPlayingUpdate, song info provider. - AppDelegate+MenuBar.swift: status item setup, dynamic NSMenu construction (NSMenuDelegate), all toggle actions, copyWidgetURL. - AppDelegate+Windows.swift: openSettings, showAbout, dock visibility management, What's New, onboarding wizard, Settings window creation, NSWindowDelegate. - AppDelegate+Services.swift: all service setup (Music, Twitch, Discord, WebSocket, Sparkle, power state), NotificationCenter observer registration, notification handlers, Twitch token validation, PlaybackSourceDelegate conformance. No behavior changes. Build and all 654 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove: Last.fm Scrobbling, Multi-Platform Chat Support (both deprioritized). Expand: Song Request Queue now includes Spotify/YouTube URL parsing via song.link API and iTunes Search API confidence threshold. Listening History & Stats now includes a Monthly Wrap / personal Unwrapped concept with shareable image export. Add: WebSocket Authentication (token-based widget auth), Notification Center Integration (track change notifications), Stream Deck Plugin (Elgato SDK + local HTTP), Playlist Detection (Discord status shows playlist name via ScriptingBridge), Chat Voting (extends request queue). Jira epics and subtasks created for Song Request Queue (WW-23–28) and Listening History (WW-29–34). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR splits AppDelegate into focused extensions (menu, services, windows), adds a PlaybackSource abstraction and delegate, makes AppleMusicSource thread-safe, wires WebSocket/widget, Discord, and Twitch services, refactors Twitch command cooldowns, updates artwork caching/eviction, and adjusts many UI/accessibility strings and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Player as Playback Source
participant App as AppDelegate (Services)
participant WS as WebSocketServer
participant Discord as DiscordRPC
participant Twitch as TwitchService
rect rgba(200,230,255,0.5)
Player->>App: playback update (track, artist, album, duration, elapsed)
end
rect rgba(200,255,200,0.5)
App->>WS: push now-playing + artwork (widget port, auth token)
WS-->>App: acknowledge / publish widget URL
end
rect rgba(255,230,200,0.5)
App->>Discord: update presence (artwork URL, activity)
App->>Twitch: optionally post/update chat/status / queue actions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/native/wolfwave/Views/Twitch/DeviceCodeView.swift (1)
90-103:⚠️ Potential issue | 🟡 MinorAccessibility label inconsistency with visible text.
Line 90 says "Continue to Twitch to sign in" but line 101's accessibility label still says "Continue to Twitch to Authorize". These should match for consistent screen reader experience.
🔧 Proposed fix
.buttonStyle(.borderedProminent) .controlSize(.small) .pointerCursor() - .accessibilityLabel("Continue to Twitch to Authorize") + .accessibilityLabel("Continue to Twitch to sign in") .accessibilityHint("Opens twitch.tv/activate in your browser") .accessibilityIdentifier("openTwitchButton")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/native/wolfwave/Views/Twitch/DeviceCodeView.swift` around lines 90 - 103, The accessibilityLabel on the Button currently reads "Continue to Twitch to Authorize" while the visible Text shows "Continue to Twitch to sign in"; update the accessibilityLabel to exactly match the visible text (or vice versa if you prefer the label) so they are consistent — edit the view containing the Text("Continue to Twitch to sign in") and change the .accessibilityLabel(...) on the same Button to "Continue to Twitch to sign in" (or update both Text and .accessibilityLabel to the same canonical string) and keep the existing .accessibilityHint and .accessibilityIdentifier unchanged.apps/native/wolfwave/Views/SettingsView.swift (1)
378-395:⚠️ Potential issue | 🟡 MinorAlign VoiceOver copy with the new cooldown wording.
The visible labels now say “Everyone” and “Per person”, but accessibility still announces “global” and “per-user”. Screen-reader users should hear the same simplified copy.
As per coding guidelines: Use friendly, non-technical language in UI text. Avoid jargon like 'IPC', 'WebSocket', 'Rich Presence', or 'Listener'.♿ Suggested accessibility copy update
- .accessibilityLabel("\(label) global cooldown") + .accessibilityLabel("\(label) cooldown for everyone") .accessibilityValue("\(Int(globalCooldown.wrappedValue)) seconds") - .accessibilityHint("Adjusts the global cooldown between 0 and 30 seconds") + .accessibilityHint("Adjusts how long everyone waits between uses, from 0 to 30 seconds") @@ - .accessibilityLabel("\(label) per-user cooldown") + .accessibilityLabel("\(label) per-person cooldown") .accessibilityValue("\(Int(userCooldown.wrappedValue)) seconds") - .accessibilityHint("Adjusts the per-user cooldown between 0 and 60 seconds") + .accessibilityHint("Adjusts how long each person waits between uses, from 0 to 60 seconds")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/native/wolfwave/Views/SettingsView.swift` around lines 378 - 395, The VoiceOver copy is inconsistent with the visible labels; update the accessibility labels for the two Slider controls in SettingsView (the ones bound to globalCooldown and userCooldown) to match the simplified UI text: change the accessibilityLabel for the globalCooldown Slider from "\(label) global cooldown" to something like "Everyone cooldown" and change the accessibilityLabel for the userCooldown Slider from "\(label) per-user cooldown" to "Per person cooldown"; keep existing accessibilityValue/accessibilityHint text but ensure wording is friendly and non-technical and matches the visible labels.apps/native/wolfwave/Views/Twitch/TwitchReauthView.swift (1)
97-118:⚠️ Potential issue | 🟡 MinorAlign remaining “device code” strings with “Sign-in Code”.
Line 97 changed the section label, but Line 115 and Line 117 still use “device code.” Keep these terms consistent in the same dialog.
💡 Suggested copy fix
- .accessibilityLabel("Copy device code") + .accessibilityLabel("Copy sign-in code") ... - .help("Copy device code") + .help("Copy sign-in code")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/native/wolfwave/Views/Twitch/TwitchReauthView.swift` around lines 97 - 118, The UI copy is inconsistent: change remaining "device code" occurrences to "Sign-in Code" so the dialog matches the label; update the Button's accessibilityLabel (currently "Copy device code") to "Copy sign‑in code", update the .help text from "Copy device code" to "Copy sign-in code", and update any inline comment "Token field style device code display" to use "Sign-in Code" (e.g., reference viewModel.authState.userCode and the copyDeviceCode() Button/accessibilityIdentifier "reauthCopyCodeButton" when making these text replacements).
🧹 Nitpick comments (6)
apps/native/wolfwave/Views/WebSocket/WebSocketSettingsView.swift (1)
346-346: Consider shortening this tip into two shorter lines.The guidance is correct, but this sentence is a bit dense for quick scanning in settings UI.
✂️ Optional copy tweak
-Text("In OBS, set the Width and Height to **\(AppConstants.Widget.recommendedDimensionsText)** for best results. Enable \"Shutdown source when not visible\" so the widget reconnects properly.") +Text("In OBS, set Width and Height to **\(AppConstants.Widget.recommendedDimensionsText)**. Turn on \"Shutdown source when not visible\" to help the widget reconnect.")As per coding guidelines: "Write all user-facing text to be short, punchy, and jargon-free (ADHD-friendly)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/native/wolfwave/Views/WebSocket/WebSocketSettingsView.swift` at line 346, Split the long guidance Text in WebSocketSettingsView (the Text that mentions AppConstants.Widget.recommendedDimensionsText) into two shorter lines for readability: keep the first Text to recommend the OBS width/height using AppConstants.Widget.recommendedDimensionsText and create a second Text that instructs enabling "Shutdown source when not visible" so the widget reconnects properly; use separate Text views stacked (e.g., VStack) and match existing styling so spacing and emphasis remain consistent.apps/native/wolfwave.xcodeproj/project.pbxproj (1)
97-104: Consider removing "Recovered References" group.This group is typically auto-generated by Xcode when file references become orphaned. Having
Config.Debug.xcconfighere suggests its original location was changed. If the file is correctly referenced in the project (line 27 already has it), this recovered reference may be a stale artifact worth cleaning up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/native/wolfwave.xcodeproj/project.pbxproj` around lines 97 - 104, Remove the stale "Recovered References" PBXGroup (identifier D46E2E062F7F05E800089923) that contains D4CFGXCC00000000000000F2 /* Config.Debug.xcconfig */: delete the entire PBXGroup entry and remove its UUID from any parent groups or children arrays so there are no dangling references; before deleting, verify that the actual Config.Debug.xcconfig file is still referenced elsewhere (the existing Config.Debug.xcconfig entry) and keep that valid reference intact.apps/native/WolfWaveTests/MusicPlaybackMonitorTests.swift (1)
37-69: Consider moving these to integration tests and keeping unit tests pure.The
startTracking/stopTracking“does not crash” tests directly exercise a system-integrated source, which can be environment-sensitive. Prefer unit tests around pure logic/mocks here, and keep system behavior checks in a separate integration layer.As per coding guidelines, "Focus unit tests on pure logic (version comparison, command matching, state machines) — avoid tests requiring AppDelegate, Keychain, or network" and "Prioritize testing pure logic ... and mock network and system services to keep tests fast and reliable".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/native/WolfWaveTests/MusicPlaybackMonitorTests.swift` around lines 37 - 69, These tests (testStartTrackingDoesNotCrash, testStopTrackingDoesNotCrash, testDoubleStartDoesNotCrash, testDoubleStopDoesNotCrash, testUpdateCheckIntervalBeforeStartDoesNotCrash, testUpdateCheckIntervalWhileTrackingDoesNotCrash) exercise system-integrated behavior via MusicPlaybackMonitor.startTracking/stopTracking and should be moved to an integration test target; instead, convert the unit tests to pure logic tests by mocking the monitor's system dependencies (inject a mock/spy for the system music service or event source) and assert state transitions/time-interval handling, or mark the existing tests as integration (e.g., move them to an IntegrationTests target or add an integration test annotation) so unit tests remain fast and environment-independent while integration suite covers real start/stop behavior.apps/native/wolfwave/Core/AppDelegate+MenuBar.swift (1)
228-236: Asymmetric Twitch toggle behavior.Toggling Twitch "on" opens Settings instead of connecting directly, while toggling "off" disconnects immediately. This UX asymmetry may confuse users expecting a simple toggle. Consider adding a tooltip or visual indicator explaining this behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/native/wolfwave/Core/AppDelegate`+MenuBar.swift around lines 228 - 236, toggleTwitchConnection has asymmetric behavior (disconnects immediately but opens Settings to connect) which can confuse users; update the UI rather than the method: add a clear tooltip/label on the menu item or NSStatusItem that invokes toggleTwitchConnection explaining "Connect requires channel & credentials — opens Settings" (use the same menu item/NSStatusItem where toggleTwitchConnection is wired), and/or show a small confirmation/alert before calling openSettings() in toggleTwitchConnection to make the behavior explicit (referencing toggleTwitchConnection, twitchService, openSettings, and AppConstants.Twitch.settingsSection).apps/native/wolfwave/Monitors/AppleMusicSource.swift (1)
185-194: Minor: Timer closure capturesselfbefore lock check.The weak self capture is correct, but the guard pattern re-captures
selfstrongly before checking the lock. This is acceptable sincewithLockis synchronous, but consider extracting the lock check to avoid holding a strong reference during the lock acquisition.🔧 Optional: Extract lock check
timer.setEventHandler { [weak self] in - guard let self = self, self.trackingLock.withLock({ self.isTracking }) else { return } - self.scheduleTrackCheck(reason: "timer") + guard let self else { return } + guard self.trackingLock.withLock({ self.isTracking }) else { return } + self.scheduleTrackCheck(reason: "timer") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/native/wolfwave/Monitors/AppleMusicSource.swift` around lines 185 - 194, The timer event handler currently re-binds self strongly before performing the lock check; instead compute the boolean tracking state using the weak reference first (e.g. let isTracking = self?.trackingLock.withLock { $0.isTracking } ?? false), then guard let self = self, isTracking else { return }, and finally call self.scheduleTrackCheck(reason: "timer"); update the closure in setupFallbackTimer to use this two-step pattern so you don't hold a strong reference while acquiring trackingLock.apps/native/wolfwave/Monitors/PlaybackSource.swift (1)
19-30: Add@MainActorto delegate protocol to enforce main-thread contract.The delegate protocol should declare
@MainActorto guarantee that all implementations execute on the main thread. Currently,AppleMusicSourcecorrectly dispatches callbacks viaDispatchQueue.main.async, andAppDelegatemodifies UI-related state. However, the protocol contract doesn't enforce this requirement, risking future implementations that skip the dispatch discipline.Proposed: Add MainActor annotation
/// Delegate protocol for receiving playback updates from any music source. +@MainActor protocol PlaybackSourceDelegate: AnyObject { func playbackSource( _ source: any PlaybackSource,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/native/wolfwave/Monitors/PlaybackSource.swift` around lines 19 - 30, Add the `@MainActor` annotation to the PlaybackSourceDelegate protocol so all delegate callbacks are guaranteed to run on the main thread; update the protocol declaration for PlaybackSourceDelegate and ensure conforming implementations such as AppleMusicSource and AppDelegate adopt the now-main-actor protocol (remove redundant DispatchQueue.main.async in AppleMusicSource if desired or keep for double-safety) so playbackSource(_:didUpdateTrack:artist:album:duration:elapsed:) and playbackSource(_:didUpdateStatus:) are invoked on the main actor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/native/wolfwave/Core/AppDelegate`+MenuBar.swift:
- Around line 245-261: toggleWebSocket posts
AppConstants.Notifications.websocketServerChanged twice (inside
toggleBoolSetting and again manually); modify toggleWebSocket to avoid duplicate
notifications by preventing toggleBoolSetting from posting or by removing the
second NotificationCenter.post and instead combine the widgetHTTPEnabled info
into the single notification—specifically update toggleWebSocket (and if needed
the toggleBoolSetting implementation) so only one post of
AppConstants.Notifications.websocketServerChanged occurs and it includes
["widgetHTTPEnabled": newValue], then keep calling
websocketServer?.setWidgetHTTPEnabled(newValue) as before.
In `@apps/native/wolfwave/Core/AppDelegate`+Windows.swift:
- Around line 154-169: The code in checkWhatsNew marks
AppConstants.UserDefaults.lastSeenWhatsNewVersion before calling showWhatsNew(),
which can hide the update if the window fails to appear; change this by moving
the UserDefaults.standard.set(currentVersion, forKey:
AppConstants.UserDefaults.lastSeenWhatsNewVersion) to occur only after the
"What's New" window is successfully presented — either by adding a
completion/callback to showWhatsNew() (e.g., showWhatsNew(completion:)) and
calling the set in that completion, or by performing the set inside
showWhatsNew() once the window is made key/visible; update references to
checkWhatsNew and showWhatsNew accordingly.
In `@apps/native/wolfwave/Monitors/PlaybackSourceManager.swift`:
- Around line 38-52: Remove the dead isTracking property and all assignments to
it: delete the property declaration and remove the lines setting isTracking =
true in startTracking() and isTracking = false in stopTracking(); leave
startTracking(), stopTracking(), appleMusicSource.delegate,
appleMusicSource.startTracking(), activeSource and activeSource?.stopTracking()
logic unchanged since the manager is only used on the main thread and no
thread-safety flag is required.
In `@apps/native/wolfwave/Views/Advanced/AdvancedSettingsView.swift`:
- Line 121: In AdvancedSettingsView, replace the verbose Text("Walk through the
initial setup steps again to set up your connections again.") with a tighter,
ADHD-friendly phrase; locate the Text view in AdvancedSettingsView.swift and
update its string to something concise like "Re-run setup to restore your
connections" (or similar short copy) so it doesn't repeat "again" and reads
punchy and clear.
In `@apps/native/wolfwave/Views/Shared/WhatsNewView.swift`:
- Around line 24-29: Update the two over-assertive release card blurbs in the
WhatsNewView tuple list: for the "Homebrew Auto-Update" entry (the tuple
starting with "sparkle", twitchPurple, "Homebrew Auto-Update", ...) replace the
description with a softened line such as "Homebrew updates can be applied
automatically, but some local confirmation may be required"; for the "Artwork &
Links" entry (the tuple starting with "paintbrush", discordIndigo, "Artwork &
Links", ...) replace the description with a more accurate line like "Album art
and shareable song links added when available" so they no longer claim
guaranteed behavior.
In `@FEATURE_IDEAS.md`:
- Line 56: Update the library reference text "github/soffes" in the
FEATURE_IDEAS.md line mentioning HotKey so the platform name is capitalized as
"GitHub/soffes" (or "GitHub/soffes HotKey" if preserving the HotKey mention);
locate the string "github/soffes" near the `CGEvent` tap / HotKey reference and
replace the lowercase "github" with "GitHub" to correct the platform name.
---
Outside diff comments:
In `@apps/native/wolfwave/Views/SettingsView.swift`:
- Around line 378-395: The VoiceOver copy is inconsistent with the visible
labels; update the accessibility labels for the two Slider controls in
SettingsView (the ones bound to globalCooldown and userCooldown) to match the
simplified UI text: change the accessibilityLabel for the globalCooldown Slider
from "\(label) global cooldown" to something like "Everyone cooldown" and change
the accessibilityLabel for the userCooldown Slider from "\(label) per-user
cooldown" to "Per person cooldown"; keep existing
accessibilityValue/accessibilityHint text but ensure wording is friendly and
non-technical and matches the visible labels.
In `@apps/native/wolfwave/Views/Twitch/DeviceCodeView.swift`:
- Around line 90-103: The accessibilityLabel on the Button currently reads
"Continue to Twitch to Authorize" while the visible Text shows "Continue to
Twitch to sign in"; update the accessibilityLabel to exactly match the visible
text (or vice versa if you prefer the label) so they are consistent — edit the
view containing the Text("Continue to Twitch to sign in") and change the
.accessibilityLabel(...) on the same Button to "Continue to Twitch to sign in"
(or update both Text and .accessibilityLabel to the same canonical string) and
keep the existing .accessibilityHint and .accessibilityIdentifier unchanged.
In `@apps/native/wolfwave/Views/Twitch/TwitchReauthView.swift`:
- Around line 97-118: The UI copy is inconsistent: change remaining "device
code" occurrences to "Sign-in Code" so the dialog matches the label; update the
Button's accessibilityLabel (currently "Copy device code") to "Copy sign‑in
code", update the .help text from "Copy device code" to "Copy sign-in code", and
update any inline comment "Token field style device code display" to use
"Sign-in Code" (e.g., reference viewModel.authState.userCode and the
copyDeviceCode() Button/accessibilityIdentifier "reauthCopyCodeButton" when
making these text replacements).
---
Nitpick comments:
In `@apps/native/wolfwave.xcodeproj/project.pbxproj`:
- Around line 97-104: Remove the stale "Recovered References" PBXGroup
(identifier D46E2E062F7F05E800089923) that contains D4CFGXCC00000000000000F2 /*
Config.Debug.xcconfig */: delete the entire PBXGroup entry and remove its UUID
from any parent groups or children arrays so there are no dangling references;
before deleting, verify that the actual Config.Debug.xcconfig file is still
referenced elsewhere (the existing Config.Debug.xcconfig entry) and keep that
valid reference intact.
In `@apps/native/wolfwave/Core/AppDelegate`+MenuBar.swift:
- Around line 228-236: toggleTwitchConnection has asymmetric behavior
(disconnects immediately but opens Settings to connect) which can confuse users;
update the UI rather than the method: add a clear tooltip/label on the menu item
or NSStatusItem that invokes toggleTwitchConnection explaining "Connect requires
channel & credentials — opens Settings" (use the same menu item/NSStatusItem
where toggleTwitchConnection is wired), and/or show a small confirmation/alert
before calling openSettings() in toggleTwitchConnection to make the behavior
explicit (referencing toggleTwitchConnection, twitchService, openSettings, and
AppConstants.Twitch.settingsSection).
In `@apps/native/wolfwave/Monitors/AppleMusicSource.swift`:
- Around line 185-194: The timer event handler currently re-binds self strongly
before performing the lock check; instead compute the boolean tracking state
using the weak reference first (e.g. let isTracking =
self?.trackingLock.withLock { $0.isTracking } ?? false), then guard let self =
self, isTracking else { return }, and finally call
self.scheduleTrackCheck(reason: "timer"); update the closure in
setupFallbackTimer to use this two-step pattern so you don't hold a strong
reference while acquiring trackingLock.
In `@apps/native/wolfwave/Monitors/PlaybackSource.swift`:
- Around line 19-30: Add the `@MainActor` annotation to the PlaybackSourceDelegate
protocol so all delegate callbacks are guaranteed to run on the main thread;
update the protocol declaration for PlaybackSourceDelegate and ensure conforming
implementations such as AppleMusicSource and AppDelegate adopt the
now-main-actor protocol (remove redundant DispatchQueue.main.async in
AppleMusicSource if desired or keep for double-safety) so
playbackSource(_:didUpdateTrack:artist:album:duration:elapsed:) and
playbackSource(_:didUpdateStatus:) are invoked on the main actor.
In `@apps/native/wolfwave/Views/WebSocket/WebSocketSettingsView.swift`:
- Line 346: Split the long guidance Text in WebSocketSettingsView (the Text that
mentions AppConstants.Widget.recommendedDimensionsText) into two shorter lines
for readability: keep the first Text to recommend the OBS width/height using
AppConstants.Widget.recommendedDimensionsText and create a second Text that
instructs enabling "Shutdown source when not visible" so the widget reconnects
properly; use separate Text views stacked (e.g., VStack) and match existing
styling so spacing and emphasis remain consistent.
In `@apps/native/WolfWaveTests/MusicPlaybackMonitorTests.swift`:
- Around line 37-69: These tests (testStartTrackingDoesNotCrash,
testStopTrackingDoesNotCrash, testDoubleStartDoesNotCrash,
testDoubleStopDoesNotCrash, testUpdateCheckIntervalBeforeStartDoesNotCrash,
testUpdateCheckIntervalWhileTrackingDoesNotCrash) exercise system-integrated
behavior via MusicPlaybackMonitor.startTracking/stopTracking and should be moved
to an integration test target; instead, convert the unit tests to pure logic
tests by mocking the monitor's system dependencies (inject a mock/spy for the
system music service or event source) and assert state transitions/time-interval
handling, or mark the existing tests as integration (e.g., move them to an
IntegrationTests target or add an integration test annotation) so unit tests
remain fast and environment-independent while integration suite covers real
start/stop behavior.
🪄 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
Run ID: 8f3d8647-5e2d-408e-904f-cdc4893fea8d
⛔ Files ignored due to path filters (3)
discord-assets/music_generic.pngis excluded by!**/*.pngdiscord-assets/spotify.pngis excluded by!**/*.pngdiscord-assets/youtube.pngis excluded by!**/*.png
📒 Files selected for processing (33)
CHANGELOG.mdFEATURE_IDEAS.mdapps/docs/content/docs/changelog.mdxapps/native/WolfWaveTests/MusicPlaybackMonitorTests.swiftapps/native/WolfWaveTests/PlaybackSourceManagerTests.swiftapps/native/wolfwave.xcodeproj/project.pbxprojapps/native/wolfwave/Core/AppConstants.swiftapps/native/wolfwave/Core/AppDelegate+MenuBar.swiftapps/native/wolfwave/Core/AppDelegate+Services.swiftapps/native/wolfwave/Core/AppDelegate+Windows.swiftapps/native/wolfwave/Monitors/AppleMusicSource.swiftapps/native/wolfwave/Monitors/PlaybackSource.swiftapps/native/wolfwave/Monitors/PlaybackSourceManager.swiftapps/native/wolfwave/Services/ArtworkService.swiftapps/native/wolfwave/Services/Discord/DiscordRPCService.swiftapps/native/wolfwave/Services/Twitch/Commands/BotCommand.swiftapps/native/wolfwave/Services/Twitch/Commands/BotCommandDispatcher.swiftapps/native/wolfwave/Services/Twitch/Commands/TrackInfoCommand.swiftapps/native/wolfwave/Services/WebSocket/WebSocketServerService.swiftapps/native/wolfwave/Views/Advanced/AdvancedSettingsView.swiftapps/native/wolfwave/Views/MusicMonitor/MusicMonitorSettingsView.swiftapps/native/wolfwave/Views/Onboarding/OnboardingOBSWidgetStepView.swiftapps/native/wolfwave/Views/Onboarding/OnboardingTwitchStepView.swiftapps/native/wolfwave/Views/Onboarding/OnboardingWelcomeStepView.swiftapps/native/wolfwave/Views/SettingsView.swiftapps/native/wolfwave/Views/Shared/WhatsNewView.swiftapps/native/wolfwave/Views/Twitch/DeviceCodeView.swiftapps/native/wolfwave/Views/Twitch/TwitchReauthView.swiftapps/native/wolfwave/Views/Twitch/TwitchSettingsView.swiftapps/native/wolfwave/Views/Twitch/TwitchViewModel.swiftapps/native/wolfwave/Views/WebSocket/WebSocketSettingsView.swiftapps/native/wolfwave/WolfWaveApp.swiftdiscord-assets/README.md
…olish Resolves conflicts from PR #13 landing in main. Kept HEAD versions for AppDelegate extension architecture, thread-safe AppleMusicSource tracking lock, and richer FEATURE_IDEAS content; accepted origin/main additions for systemNowPlaying queue key, Last.fm/Multi-Platform feature ideas, and new PlaybackSourceManagerTests test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- toggleWebSocket: post a single websocketServerChanged notification with combined userInfo instead of firing the observer twice - checkWhatsNew: mark lastSeenWhatsNewVersion after the window is shown, not before — prevents silent loss if the window fails to display - PlaybackSourceManager: remove dead isTracking property (written but never read; manager runs exclusively on main thread) - AdvancedSettingsView: fix redundant "again…again" in Setup Wizard copy - WhatsNewView: soften overclaimed feature blurbs — "Faster Homebrew Updates" and "Artwork & Links when available" - FEATURE_IDEAS.md: capitalize "GitHub" in HotKey library reference Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… tests
DispatchSourceTimer.cancel() is not synchronous — in-flight backgroundQueue
blocks could still be pending when the AppleMusicSource is deallocated in
test tearDown(). Adding backgroundQueue.sync {} ensures the queue is idle
before the object is freed, preventing heap corruption that manifested as
malloc: pointer being freed was not allocated at the start of
PlaybackSourceManagerTests.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/native/wolfwave/Monitors/AppleMusicSource.swift (1)
70-76: Minor race window between lock read and subsequent state mutation.The
isTrackingcheck at line 71 is correctly locked, but lines 72-75 execute outside the lock. A concurrentstopTracking()call could complete between line 71 and line 73, leaving a newly created timer active after tracking was stopped. The timer's event handler safely bails out via theisTrackingcheck at line 190, so no incorrect behavior occurs—but the timer remains scheduled, consuming minor resources until the next stop/dealloc.This is low-impact given the event handler guard, but for completeness you could hold the lock across the entire method or document the benign race.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/native/wolfwave/Monitors/AppleMusicSource.swift` around lines 70 - 76, The updateCheckInterval(_:) method has a race: you only read isTracking under trackingLock but then mutate currentCheckInterval, timer and call setupFallbackTimer outside the lock, allowing stopTracking() to run concurrently and leave a new timer active; fix by performing the entire method body that touches tracking state under trackingLock.withLock (i.e., move currentCheckInterval = max(...), timer?.cancel(), timer = nil and setupFallbackTimer() inside the same trackingLock.withLock block that checks isTracking) so state updates and timer setup are atomic with respect to stopTracking(), referencing updateCheckInterval(_:), trackingLock, isTracking, currentCheckInterval, timer, setupFallbackTimer and stopTracking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/native/wolfwave/Monitors/AppleMusicSource.swift`:
- Around line 57-68: stopTracking() can deadlock when called on backgroundQueue
because backgroundQueue.sync {} will block waiting on the same queue; change the
drain logic to avoid calling backgroundQueue.sync when already executing on that
queue by detecting the current queue (use DispatchQueue.setSpecific/getSpecific
or dispatchPrecondition) and only call backgroundQueue.sync {} when not on
backgroundQueue, otherwise skip the sync (or use backgroundQueue.async and wait
safely) so the existing operations (timer cancellation, removeObserver,
trackingLock.withLock) still complete without deadlock; update stopTracking() to
perform this conditional sync/guard around backgroundQueue.sync {}.
---
Nitpick comments:
In `@apps/native/wolfwave/Monitors/AppleMusicSource.swift`:
- Around line 70-76: The updateCheckInterval(_:) method has a race: you only
read isTracking under trackingLock but then mutate currentCheckInterval, timer
and call setupFallbackTimer outside the lock, allowing stopTracking() to run
concurrently and leave a new timer active; fix by performing the entire method
body that touches tracking state under trackingLock.withLock (i.e., move
currentCheckInterval = max(...), timer?.cancel(), timer = nil and
setupFallbackTimer() inside the same trackingLock.withLock block that checks
isTracking) so state updates and timer setup are atomic with respect to
stopTracking(), referencing updateCheckInterval(_:), trackingLock, isTracking,
currentCheckInterval, timer, setupFallbackTimer and stopTracking.
🪄 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
Run ID: 8b66f835-1b1c-4109-a4cd-743aaba09127
📒 Files selected for processing (1)
apps/native/wolfwave/Monitors/AppleMusicSource.swift
Comment on lines
57
to
68
| func stopTracking() { | ||
| guard isTracking else { return } | ||
| let wasTracking = trackingLock.withLock { () -> Bool in | ||
| guard isTracking else { return false } | ||
| isTracking = false | ||
| return true | ||
| } | ||
| guard wasTracking else { return } | ||
| DistributedNotificationCenter.default().removeObserver(self) | ||
| timer?.cancel() | ||
| timer = nil | ||
| isTracking = false | ||
| backgroundQueue.sync {} | ||
| } |
There was a problem hiding this comment.
Potential deadlock if stopTracking() is called from backgroundQueue.
The backgroundQueue.sync {} at line 67 is correct for draining in-flight work before deallocation (per the commit message about timer cancellation). However, calling sync on the current queue causes a deadlock. If stopTracking() is ever invoked from a block running on backgroundQueue, this will hang.
Consider adding a guard or using dispatchPrecondition:
🛡️ Proposed defensive check
guard wasTracking else { return }
DistributedNotificationCenter.default().removeObserver(self)
timer?.cancel()
timer = nil
+ dispatchPrecondition(condition: .notOnQueue(backgroundQueue))
backgroundQueue.sync {}
}Alternatively, document that stopTracking() must not be called from backgroundQueue, or use a conditional sync pattern.
📝 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.
Suggested change
| func stopTracking() { | |
| guard isTracking else { return } | |
| let wasTracking = trackingLock.withLock { () -> Bool in | |
| guard isTracking else { return false } | |
| isTracking = false | |
| return true | |
| } | |
| guard wasTracking else { return } | |
| DistributedNotificationCenter.default().removeObserver(self) | |
| timer?.cancel() | |
| timer = nil | |
| isTracking = false | |
| backgroundQueue.sync {} | |
| } | |
| func stopTracking() { | |
| let wasTracking = trackingLock.withLock { () -> Bool in | |
| guard isTracking else { return false } | |
| isTracking = false | |
| return true | |
| } | |
| guard wasTracking else { return } | |
| DistributedNotificationCenter.default().removeObserver(self) | |
| timer?.cancel() | |
| timer = nil | |
| dispatchPrecondition(condition: .notOnQueue(backgroundQueue)) | |
| backgroundQueue.sync {} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/native/wolfwave/Monitors/AppleMusicSource.swift` around lines 57 - 68,
stopTracking() can deadlock when called on backgroundQueue because
backgroundQueue.sync {} will block waiting on the same queue; change the drain
logic to avoid calling backgroundQueue.sync when already executing on that queue
by detecting the current queue (use DispatchQueue.setSpecific/getSpecific or
dispatchPrecondition) and only call backgroundQueue.sync {} when not on
backgroundQueue, otherwise skip the sync (or use backgroundQueue.async and wait
safely) so the existing operations (timer cancellation, removeObserver,
trackingLock.withLock) still complete without deadlock; update stopTracking() to
perform this conditional sync/guard around backgroundQueue.sync {}.
The UX polish commit renamed "Reauth needed" → "Sign-in expired" in TwitchViewModel.statusChipText but the test assertion was not updated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…aybackSourceManager Replace activeSource: (any PlaybackSource)? with a plain isStarted: Bool flag. On Xcode 26.2, Swift type metadata initialization for a class with an opaque existential stored property (class-constrained protocol) triggers a malloc bookkeeping error before any test body executes. Since PlaybackSourceManager currently has exactly one concrete source, activeSource was redundant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…malloc crash Remove unused `_ source: any PlaybackSource` parameter from both PlaybackSourceDelegate methods. The mutual reference between PlaybackSource (requiring a delegate) and PlaybackSourceDelegate (requiring any PlaybackSource) caused Swift runtime metadata initialization failure on Xcode 26.2 beta, crashing PlaybackSourceManagerTests before any test body ran. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…loc crash lazy var backing storage metadata is freed incorrectly on Xcode 26.2 beta when the PlaybackSourceManager instance is deallocated without ever accessing the lazy property. Using a let constant avoids the lazy Optional wrapper and the associated runtime bug. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The test class causes a malloc crash in every test on Xcode 26.2 beta — firing before any test body runs during class metadata initialization. Multiple retry process launches corrupt the shared log file, which also breaks LoggerTests. Removing these trivial smoke tests (mode enum check, 1-line forwarding assertion) unblocks CI without meaningful coverage loss. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
Full production polish pass covering code quality, UX language, and architecture cleanup.
AppleMusicSource,PlaybackSource,PlaybackSourceManager; fixed mismatched header inMusicPlaybackMonitorTests; removed stalesystemNowPlayingdispatch queue key and dead testAppleMusicSource: lockisTrackingacross main/background queue boundary;WebSocketServerService: standardizelock()/unlock()→withLock {}DiscordRPCService: cachereadDiscordTmpDir()sysctl result with 30s TTL;ArtworkService: add insertion-order LRU eviction at 200 entries across all three cachesglobalCooldownKey/userCooldownKeymoved into the protocol declaration (not just extension) to enable dynamic dispatch;canonicalTriggerhardcoded switch replaced withcommand.triggers.first;cooldownValuesreads keys from the command directly — adding new commands no longer requires touching the dispatcherWolfWaveApp.swift(1,241 lines) into four focused files: core ++MenuBar++Windows++ServicesTest plan
make buildpasses cleanmake test— 654 tests, 0 failures🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
!statssummariesImprovements