Conversation
Improves the Twitch integration settings view by adding a status chip for better visual feedback on the connection state. Refines the UI with layout adjustments and improved button styles for a more consistent and user-friendly experience. Adds a confirmation dialog for disconnecting from the channel to prevent accidental disconnections.
Enhances the Twitch integration UI and UX in settings, using the modern `List` based sidebar and simplifying the overall aesthetic. The update provides a more intuitive and streamlined user experience by reducing visual clutter, modernizing UI elements, and improving accessibility. WW-6-twitch-integration-change-the-button-text-on-click
Removes the custom implementation for toggling the sidebar. The system-provided default is now used.
Centralizes app configuration, identifiers, and secure storage. This commit introduces a comprehensive set of constants within the AppConstants enum, improving code maintainability and reducing duplication. It also implements KeychainService for secure credential storage, enhancing application security. These changes lay the groundwork for Twitch integration and other core functionalities.
Consolidates settings UI constants and refactors Twitch service handling for improved clarity and maintainability. Simplifies the settings UI configuration by removing redundant dimensions and introducing menu labels for better organization. Also, the Twitch service is directly retrieved from the application delegate, ensuring more consistent state management. Updates the initial connection message to be set to a default value of true.
Improves the user experience for Twitch authentication. Streamlines button text and descriptions for clarity, guiding users through the authorization process more effectively. Adds connection logging for debugging the Twitch chat connection process. Fixes WW-6
Streamlines logging within the Twitch integration for clarity and performance. Removes excessive debug/info level logging in production code. Consolidates relevant logging to `TwitchChatService` and simplifies log messages in `MusicPlaybackMonitor`. Debug logging is now only compiled in DEBUG builds.
Marks the logger's isDebugLoggingEnabled property as nonisolated to prevent data races when accessed from multiple threads. Also removes a duplicate MARK comment. Related to WW-6
Configures keychain access groups in the entitlement files for both development and production builds. This ensures proper sharing of keychain data. Relates to WW-6
|
Caution Review failedThe pull request is closed. WalkthroughAdds centralized AppConstants, expanded Twitch OAuth/device-code flow and UI (dialogs/window controller), improved TwitchChatService thread-safety/rate-limit handling, Keychain error handling, large SettingsView/Twitch UI refactors, AppDelegate-based lifecycle/menu integration, and entitlements for keychain access. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SettingsView
participant TwitchViewModel
participant TwitchDeviceAuth
participant TwitchChatService
User->>SettingsView: Tap "Get Started with Twitch"
SettingsView->>TwitchViewModel: startOAuth()
TwitchViewModel->>TwitchDeviceAuth: requestDeviceCode()
TwitchDeviceAuth-->>TwitchViewModel: TwitchDeviceCodeResponse
TwitchViewModel->>SettingsView: set auth state => authorizing
SettingsView->>User: show DeviceCodeView
User->>DeviceCodeView: Copy code / Open activation URL
User->>TwitchDeviceAuth: Authorize via Twitch (external)
par Polling loop
TwitchViewModel->>TwitchDeviceAuth: pollForToken(deviceCode, interval, expiresIn)
TwitchDeviceAuth-->>TwitchViewModel: token or OAuth error (authorization_pending / slow_down / access_denied / expired_token)
end
TwitchViewModel->>TwitchChatService: connect(with: token)
TwitchChatService->>TwitchChatService: establish connection, join channel
TwitchChatService-->>TwitchViewModel: connection state -> connected
TwitchViewModel->>SettingsView: update UI -> connected
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/wolfwave/Services/Twitch/TwitchChatService.swift (1)
383-389: ResetisProcessingDisconnecton reconnect to avoid dropping all messages.Line 616 sets
isProcessingDisconnect = true, andhandleEventSubMessageearly‑returns while it’s true. There’s no reset when a new connection is established, so future messages are ignored after a disconnect/reconnect cycle.🐛 Proposed fix (clear the flag on join)
- self.botUsername = nil - hasSentConnectionMessage = false + self.botUsername = nil + disconnectLock.withLock { isProcessingDisconnect = false } + hasSentConnectionMessage = falseAlso applies to: 616-639
src/wolfwave/Monitors/MusicPlaybackMonitor.swift (1)
209-211: Duplicate MARK comment.There are two consecutive
// MARK: - Setup & Schedulingcomments on lines 209 and 211. Remove the duplicate.Proposed fix
// MARK: - Setup & Scheduling - // MARK: - Setup & Scheduling - /// Subscribes to distributed notifications from Music.app.src/wolfwave/Views/Twitch/TwitchViewModel.swift (2)
188-225: Guard against duplicate NotificationCenter observers.
loadSavedCredentials()registers observers each time it's called. Since it's triggered by.onAppearin TwitchSettingsView, repeated view presentations will register duplicate observers, causing redundant callbacks during the object's lifetime despite proper cleanup indeinit.Add a guard flag to register observers only once:
Suggested fix
/// Cached reference to the Twitch chat service private var cachedTwitchService: TwitchChatService? + private var observersRegistered = false func loadSavedCredentials() { if let username = KeychainService.loadTwitchUsername() { botUsername = username } if let token = KeychainService.loadTwitchToken() { oauthToken = token credentialsSaved = true } if let channel = KeychainService.loadTwitchChannelID() { channelID = channel } // Load reauth needed flag from UserDefaults reauthNeeded = UserDefaults.standard.bool(forKey: AppConstants.UserDefaults.twitchReauthNeeded) + + guard !observersRegistered else { return } + observersRegistered = true NotificationCenter.default.addObserver( self, selector: `#selector`(reAuthStatusChanged), name: NSNotification.Name(AppConstants.Notifications.twitchReauthNeededChanged), object: nil )
271-342: Handle CancellationError in the outer catch block to prevent error UI on OAuth cancellation.When
oAuthTaskis cancelled duringrequestDeviceCode()(line 295), the outer catch block (lines 336–342) will catch theCancellationErrorand display an error state. The inner polling task already handles this correctly by checkingif !(error is CancellationError)before showing errors (line 327). Apply the same pattern to the outer catch to avoid surfacing error messages when cancellation is intentional.🛠️ Proposed fix
} catch { + if error is CancellationError { + await MainActor.run { self.oAuthTask = nil } + return + } updateAuthState(.error(error.localizedDescription)) statusMessage = "❌ OAuth setup failed: \(error.localizedDescription)" await MainActor.run { self.oAuthTask = nil } }
🤖 Fix all issues with AI agents
In `@src/wolfwave/Services/Twitch/TwitchChatService.swift`:
- Around line 95-99: The boolean hasSentConnectionMessage is accessed from
multiple threads and must be protected by the existing connectionLock; update
all reads and writes of hasSentConnectionMessage (including where it is set in
joinChannel() and where it is read on the main thread around lines referenced)
to perform the access inside connectionLock.sync (or the same locking pattern
used elsewhere in TwitchChatService) so the property is always read/modified
under connectionLock; ensure the declaration stays private and remove
unsynchronized accesses in methods that currently reference
hasSentConnectionMessage directly.
In `@src/wolfwave/Views/Twitch/TwitchDeviceAuthDialog.swift`:
- Around line 168-179: The two delayed resets in TwitchDeviceAuthDialog.swift
are mismatched: isCodeCopied is reset at 1.5s while showCopyFeedback is
dismissed at 2.0s, causing the button icon to revert before the toast hides;
change the isCodeCopied reset to use the same delay as showCopyFeedback (use
.now() + 2.0) or move the isCodeCopied = false assignment inside the same
DispatchQueue.main.asyncAfter block that hides showCopyFeedback so both state
and UI dismiss in sync.
- Around line 148-155: The deviceCode is interpolated directly into the URL in
TwitchDeviceAuthDialog (within the DispatchQueue.main.asyncAfter closure) and
must be URL-encoded to avoid malformed URLs; change the URL construction to
percent-encode deviceCode (e.g., via
addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) or by building
the URL with URLComponents/URLQueryItem) and only call
NSWorkspace.shared.open(url) if encoding/URL construction succeeds, otherwise
handle the failure (e.g., log or call onAuthorizePressed without opening).
In `@src/wolfwave/Views/Twitch/TwitchDeviceAuthWindowController.swift`:
- Around line 57-58: Doc says the controller should follow macOS system
appearance but the code forces window.appearance = NSAppearance(named: .aqua);
update the behavior by removing or clearing the forced appearance (remove the
assignment to window.appearance or set it to nil) in
TwitchDeviceAuthWindowController so the window inherits the system light/dark
setting, or alternatively update the class doc comment to reflect the forced
.aqua appearance if keeping it intentionally.
- Around line 80-87: The show() method creates a local
TwitchDeviceAuthWindowController that is not retained and can be deallocated
while its window is open; fix this by adding a stored property (e.g., var
retainedTwitchController: TwitchDeviceAuthWindowController?) on the same class
and assign the newly created TwitchDeviceAuthWindowController to that property
inside show(), then clear the property when the window closes (observe the
controller/window close via windowWillClose, completion handler, or
NSWindowDelegate) so the controller is retained while visible and released
afterward.
In `@src/wolfwave/Views/Twitch/TwitchReauthView.swift`:
- Around line 93-117: The copy button in TwitchReauthView's authorizingContent
currently only sets viewModel.statusMessage and doesn't actually copy the device
code; update the Button(action:) closure to write viewModel.authState.userCode
to the macOS pasteboard using NSPasteboard (same approach as
DeviceCodeView.swift and TwitchDeviceAuthDialog.swift), ensure you clear and set
the pasteboard contents (e.g., declare pasteboard = NSPasteboard.general;
pasteboard.clearContents(); pasteboard.setString(...)), then set
viewModel.statusMessage = "Code copied to clipboard!" after the pasteboard write
so the behavior matches the other implementations.
In `@src/wolfwave/Views/Twitch/TwitchViewModel.swift`:
- Around line 454-497: The joinChannel() flow currently uses optional chaining
on self.twitchService so connectToChannel(...) can be skipped when twitchService
is nil, yet channelConnected is still set true; add an explicit guard let
service = self.twitchService (or early return) before setting
service?.shouldSendConnectionMessageOnSubscribe and calling try await
service.connectToChannel(...), and only set self.channelConnected = true after a
successful connectToChannel call on the non-optional service; ensure the same
ConnectionError/Error handling remains but references the unwrapped service
variable instead of self.twitchService.
In `@src/wolfwave/Views/WebSocket/WebSocketSettingsView.swift`:
- Around line 67-74: The hover handlers using onContinuousHover are repeatedly
calling NSCursor.pointingHand.push() and causing unmatched pushes; replace each
onContinuousHover { phase in ... } block with an onHover { isInside in ... }
handler so you call NSCursor.pointingHand.push() once when isInside is true and
NSCursor.pop() when isInside is false; update the four occurrences in
WebSocketSettingsView.swift (the closures that currently reference
NSCursor.pointingHand.push() / NSCursor.pop()) so the push/pop are paired by
onHover rather than the repeated .active events from onContinuousHover.
🧹 Nitpick comments (10)
src/wolfwave/Monitors/MusicPlaybackMonitor.swift (1)
42-57: Consider using centralized AppConstants.The local
Constantsenum duplicates values that exist inAppConstants.Music(bundleIdentifier, playerInfoNotification) andAppConstants.DispatchQueues(musicPlaybackMonitor). While the current approach is functional, consider referencingAppConstantsfor consistency across the codebase.src/wolfwave/Core/KeychainService.swift (3)
63-78: Unused error caseinvalidData.
KeychainError.invalidDatais defined but never thrown anywhere in the codebase. Either remove it or add it to load operations that encounter corrupted data.Option 1: Remove unused case
enum KeychainError: LocalizedError { /// Failed to save data to Keychain with given Security framework status code. case saveFailed(OSStatus) - - /// Invalid or corrupted data read from Keychain. - case invalidData var errorDescription: String? { switch self { case .saveFailed(let status): return "Failed to save token to Keychain (status: \(status))" - case .invalidData: - return "Invalid token data" } } }
225-239: Inconsistent error logging across save methods.
saveTwitchBotUserIDandsaveTwitchChannelIDdon't log errors before throwing, unlikesaveToken,saveTwitchToken, andsaveTwitchUsername. Consider addingLog.errorcalls for consistency.Proposed fix for saveTwitchBotUserID
guard status == errSecSuccess else { + Log.error("Failed to save Twitch bot user ID - OSStatus \(status)", category: "Keychain") throw KeychainError.saveFailed(status) }Proposed fix for saveTwitchChannelID
guard status == errSecSuccess else { + Log.error("Failed to save Twitch channel ID - OSStatus \(status)", category: "Keychain") throw KeychainError.saveFailed(status) }Also applies to: 268-282
39-58: Consider referencing AppConstants.Keychain.service.The local
serviceconstant duplicatesAppConstants.Keychain.service. For consistency with the centralized constants pattern, consider using the shared constant.src/wolfwave/Views/Twitch/TwitchDeviceAuthDialog.swift (2)
157-179: Duplicate copy logic with DeviceCodeView.This
copyDeviceCode()function is nearly identical to the one inDeviceCodeView.swift(lines 110-132). Consider extracting a shared utility to reduce duplication.
120-138: Redundant copy feedback UI.Both an inline indicator inside
DeviceCodeEntryView(lines 235-248) and a toast overlay (lines 121-138) show "Copied to clipboard" simultaneously. Consider using only one feedback mechanism for a cleaner UX.Also applies to: 234-248
src/wolfwave/Views/Twitch/DeviceCodeView.swift (2)
22-22: Remove redundant nil initialization.Optionals default to
nil; explicit initialization is unnecessary.Proposed fix
- var onActivate: (() -> Void)? = nil + var onActivate: (() -> Void)?
122-131: Combine asyncAfter calls with the same delay.Both
showCopyFeedback = falseandisCodeCopied = falseare scheduled at1.3seconds. Merge them into a single dispatch for clarity.Proposed fix
- // Auto-dismiss feedback and reset state - DispatchQueue.main.asyncAfter(deadline: .now() + 1.3) { - withAnimation(.easeInOut(duration: 0.18)) { - showCopyFeedback = false - } - } - - DispatchQueue.main.asyncAfter(deadline: .now() + 1.3) { - isCodeCopied = false - } + // Auto-dismiss feedback and reset state + DispatchQueue.main.asyncAfter(deadline: .now() + 1.3) { + withAnimation(.easeInOut(duration: 0.18)) { + showCopyFeedback = false + } + isCodeCopied = false + }src/wolfwave/Core/AppConstants.swift (2)
141-144: Consider renamingUIto a more descriptive name.SwiftLint flags
UIas too short. More importantly, there's alsoSettingsUIwith overlapping concerns. Consider consolidating or renaming toDimensionsorWindowSizesfor clarity.
15-15: Unused import.
AppKitis imported but not used in this constants-only file. Consider removing it.Proposed fix
import Foundation -import AppKit
src/wolfwave/Views/Twitch/TwitchDeviceAuthWindowController.swift
Outdated
Show resolved
Hide resolved
Enhances the Twitch device authorization process by: - Adding URL encoding for the device code. - Centralizing the "hasSentConnectionMessage" flag management with a lock. - Correcting auto-dismiss timing for the copy feedback. - Implementing window delegate to manage the lifecycle of the device authorization window, preventing unexpected deallocation. - Ensures that the Twitch service is initialized before attempting connection.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/wolfwave/Services/Twitch/TwitchChatService.swift`:
- Around line 141-143: The three nonisolated(unsafe) vars reconnectChannelName,
reconnectToken, and reconnectClientID are being accessed from multiple threads
without synchronization; change their storage so accesses are protected by the
existing reconnectionLock (or move them into the actor scope) by removing
nonisolated(unsafe) and routing all reads/writes through synchronized accessors
that acquire reconnectionLock (or convert the fields into actor-isolated
properties and update the network monitor callback to call the actor method),
ensuring every use-site of reconnectChannelName/reconnectToken/reconnectClientID
uses the protected getters/setters.
In `@src/wolfwave/Views/WebSocket/WebSocketSettingsView.swift`:
- Around line 48-73: The WIP banner's onHover closure currently pushes
NSCursor.pointingHand which wrongly suggests interactivity; update the onHover
block in WebSocketSettingsView (the closure attached to the VStack) to push
NSCursor.operationNotAllowed instead of NSCursor.pointingHand when isInside is
true, and keep the existing NSCursor.pop() call when leaving so the cursor state
is restored.
♻️ Duplicate comments (1)
src/wolfwave/Services/Twitch/TwitchChatService.swift (1)
720-728: TOCTOU race insendConnectionMessage()– read and write should be atomic.The flag is read (Line 722) and written (Line 726) under separate lock acquisitions, allowing two concurrent calls to both pass the guard and send duplicate messages.
🔒 Proposed fix: single atomic check-and-set
func sendConnectionMessage() { DispatchQueue.main.asyncAfter(deadline: .now() + 1.5) { [weak self] in guard let self = self else { return } - let hasSent = self.connectionLock.withLock { self.hasSentConnectionMessage } - if hasSent { - return - } - self.connectionLock.withLock { self.hasSentConnectionMessage = true } + let shouldSend = self.connectionLock.withLock { + guard !self.hasSentConnectionMessage else { return false } + self.hasSentConnectionMessage = true + return true + } + guard shouldSend else { return } self.sendMessage("WolfWave Application is connected! 🎵") } }
🧹 Nitpick comments (4)
src/wolfwave/Views/Twitch/TwitchDeviceAuthDialog.swift (2)
148-160: URL encoding and error logging issues from previous review addressed.The device code is now properly URL-encoded (Line 151). However, Line 156 uses
Logger.shared.log(...)while the rest of the codebase usesLog.error(...). Consider using the consistentLogutility.Use consistent logging utility
} else { // Log failure if encoding/URL construction fails - Logger.shared.log("Failed to encode device code or construct authorization URL", level: .error, category: "TwitchDeviceAuth") + Log.error("Failed to encode device code or construct authorization URL", category: "TwitchDeviceAuth") }
236-249: Dual copy feedback (toast + inline) may be intentional but redundant.Both the toast overlay (Lines 120-138) and the inline feedback (Lines 236-249) appear when
isCodeCopiedis true. If both are intentional for emphasis, consider documenting this choice; otherwise, one could be removed for cleaner UX.src/wolfwave/Views/Twitch/TwitchDeviceAuthWindowController.swift (1)
36-48: Minor: Style mask set twice.
styleMaskis set in the initializer (Line 37) and again aftersuper.init(Line 45). The second assignment is redundant.Remove redundant style mask assignment
// Configure window properties for macOS system dialog appearance window.title = "Reconnect to Twitch" - window.styleMask = [.titled, .closable] window.isReleasedWhenClosed = falsesrc/wolfwave/Views/Twitch/TwitchReauthView.swift (1)
110-117: Copy button lacks visual feedback compared toTwitchDeviceAuthDialog.In
TwitchDeviceAuthDialog, the copy button shows a checkmark icon after copying (isCodeCopiedstate). This view only updatesstatusMessage. Consider adding similar visual feedback for consistency.Add visual feedback state
+ `@State` private var isCodeCopied = false + private func copyDeviceCode() { let pasteboard = NSPasteboard.general pasteboard.clearContents() pasteboard.setString(viewModel.authState.userCode, forType: .string) + isCodeCopied = true viewModel.statusMessage = "Code copied to clipboard!" + + DispatchQueue.main.asyncAfter(deadline: .now() + 2.0) { + isCodeCopied = false + } }Then update the button icon:
Button(action: { copyDeviceCode() }) { - Image(systemName: "doc.on.doc") + Image(systemName: isCodeCopied ? "checkmark.circle.fill" : "doc.on.doc") .font(.body) + .foregroundColor(isCodeCopied ? .green : nil) }
Removes the redundant error logging when encoding the device code or constructing the authorization URL in the Twitch device authorization dialog. The error is not actionable at this point, as the user is already being directed to Twitch's website.
Updates the home page with a revised value proposition targeting broadcasters, emphasizing WebSocket and Helix API integration for Apple Music and Twitch. Refines the feature grid with updated descriptions and icons, focusing on WebSocket broadcasting, Twitch bot integration, and native security. Removes redundant file.
Enhances the Twitch chat service by implementing thread-safe reconnection credential management. This change addresses potential race conditions when accessing and updating reconnection credentials, ensuring a more reliable and stable connection to Twitch chat. Additionally, this commit updates the cursor style on the WebSocket settings to indicate that the button is not currently enabled.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.