Fixed sound sync issue - added new sync settings to sync b/w FV and s…#65
Fixed sound sync issue - added new sync settings to sync b/w FV and s…#65
Conversation
…ystem sound ; also have independent control
WalkthroughThis PR refactors the app's service architecture to introduce lazy initialization with a startup gate via AppServices, adds conditional audio device synchronization with system settings, enhances device binding logic in ASRService, and bumps the version to 1.5.1. Changes
Sequence Diagram(s)sequenceDiagram
participant App as fluidApp
participant Content as ContentView
participant AppSvc as AppServices
participant AudioObs as AudioObserver
participant ASR as ASRService
participant Device as AudioDeviceService
participant Settings as SettingsStore
App->>AppSvc: `@EnvironmentObject` initialization
Note over Content: ContentView loads
rect rgb(220, 240, 255)
Note over Content,AppSvc: Startup Sequence (1.5s delay)
Content->>AppSvc: signalUIReady()
AppSvc->>AppSvc: isUIReady = true
end
rect rgb(240, 255, 240)
Note over AppSvc,ASR: Lazy Service Initialization
Content->>AppSvc: access appServices.audioObserver
AppSvc->>AudioObs: create instance (lazy)
AppSvc->>AppSvc: setupAudioObserverForwarding()
Content->>AppSvc: access appServices.asr
AppSvc->>ASR: create instance (lazy)
AppSvc->>AppSvc: setupASRForwarding()
end
rect rgb(255, 245, 220)
Note over ASR,Settings: Device Binding (sync-aware)
ASR->>Settings: check syncAudioDevicesWithSystem
alt syncAudioDevicesWithSystem = false
ASR->>Device: getInputDevice(byUID: preferred)
Device-->>ASR: Device or nil
ASR->>ASR: bindPreferredInputDeviceIfNeeded()
ASR->>ASR: setEngineInputDevice(deviceID)
else syncAudioDevicesWithSystem = true
ASR->>ASR: use system defaults
end
end
rect rgb(255, 230, 230)
Note over ASR,Settings: Hardware Change Handling
ASR->>Settings: check syncAudioDevicesWithSystem
alt sync enabled
ASR->>ASR: restart engine, bind to new default
else sync disabled
ASR->>ASR: log & ignore (preserve user preference)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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: 1
🧹 Nitpick comments (3)
Sources/Fluid/ContentView.swift (1)
538-573: Minor: Consider extracting device sync logic into a dedicated method.The hardware change handler correctly implements conditional behavior based on
syncAudioDevicesWithSystem. However, the nested conditionals with similar patterns for input and output devices could be extracted into a helper method to reduce duplication.Also, on lines 555 and 565,
inputDevicesandoutputDevicesare accessed directly withoutself., unlike other property accesses in this file. This is a minor inconsistency.🔎 Suggested consistency fix
} else { // Independent mode: Only update if preferred device is no longer available if let prefIn = SettingsStore.shared.preferredInputDeviceUID, - inputDevices.contains(where: { $0.uid == prefIn }) + self.inputDevices.contains(where: { $0.uid == prefIn }) { self.selectedInputUID = prefIn } else if let sysIn = AudioDevice.getDefaultInputDevice()?.uid { // Fallback to system default if preferred device disconnected self.selectedInputUID = sysIn SettingsStore.shared.preferredInputDeviceUID = sysIn } if let prefOut = SettingsStore.shared.preferredOutputDeviceUID, - outputDevices.contains(where: { $0.uid == prefOut }) + self.outputDevices.contains(where: { $0.uid == prefOut }) { self.selectedOutputUID = prefOut } else if let sysOut = AudioDevice.getDefaultOutputDevice()?.uid {Sources/Fluid/Services/AppServices.swift (1)
101-115:initializeServicesIfNeeded()provides defense-in-depth but may be redundant.This method provides an explicit initialization path, but the lazy getters already handle initialization on first access. The method is useful as documentation and for explicit initialization ordering, but the side-effect-only access (
_ = self.audioObserver) could be clearer with a comment.Consider whether this method adds value beyond what ContentView already does by directly accessing
audioObserver.startObserving()andasr.initialize().Sources/Fluid/Services/ASRService.swift (1)
530-537: BlockingThread.sleepon MainActor could cause brief UI freezes.
Thread.sleep(forTimeInterval: 0.1)on line 531 blocks the main thread for 100ms per retry attempt (up to 300ms total for 3 attempts). While this is a retry path for engine start failures, it could cause noticeable UI stutter.Consider using
Task.sleepinstead for non-blocking delays, though this would require makingstartEngine()async. Given that this is in a retry path for rare failures, the current implementation may be acceptable as a pragmatic tradeoff.🔎 Alternative using async sleep (requires method signature change)
If you want to avoid blocking the main thread:
- private func startEngine() throws { + private func startEngine() async throws { self.engine.reset() var attempts = 0 while attempts < 3 { do { try self.engine.start() return } catch { attempts += 1 - Thread.sleep(forTimeInterval: 0.1) + try? await Task.sleep(nanoseconds: 100_000_000) self.engine.reset() // After a reset, the underlying AUHAL unit may revert to system-default input. // Re-create the input node and re-bind the preferred device (independent mode). _ = self.engine.inputNode self.bindPreferredInputDeviceIfNeeded() } } throw NSError(domain: "ASRService", code: -1) }This would require updating callers to await the method.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Info.plist(1 hunks)Sources/Fluid/ContentView.swift(3 hunks)Sources/Fluid/Persistence/SettingsStore.swift(2 hunks)Sources/Fluid/Services/ASRService.swift(4 hunks)Sources/Fluid/Services/AppServices.swift(2 hunks)Sources/Fluid/Services/AudioDeviceService.swift(1 hunks)Sources/Fluid/Services/CommandModeService.swift(1 hunks)Sources/Fluid/Services/GlobalHotkeyManager.swift(1 hunks)Sources/Fluid/Services/MenuBarManager.swift(1 hunks)Sources/Fluid/Services/RewriteModeService.swift(1 hunks)Sources/Fluid/UI/SettingsView.swift(4 hunks)Sources/Fluid/fluidApp.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
Sources/Fluid/UI/SettingsView.swift (3)
Sources/Fluid/ContentView.swift (1)
refreshDevices(960-963)Sources/Fluid/Services/AudioDeviceService.swift (4)
getDefaultInputDevice(78-81)getDefaultOutputDevice(83-86)setDefaultInputDevice(88-92)setDefaultOutputDevice(94-98)Sources/Fluid/Services/DebugLogger.swift (1)
info(141-143)
Sources/Fluid/Services/CommandModeService.swift (1)
Sources/Fluid/Persistence/SettingsStore.swift (1)
isReasoningModel(600-609)
Sources/Fluid/Services/GlobalHotkeyManager.swift (1)
Sources/Fluid/Services/DebugLogger.swift (1)
warning(145-147)
Sources/Fluid/Services/ASRService.swift (2)
Sources/Fluid/Services/AudioDeviceService.swift (1)
getInputDevice(101-103)Sources/Fluid/Services/DebugLogger.swift (4)
warning(145-147)error(149-151)info(141-143)debug(153-155)
Sources/Fluid/Services/AppServices.swift (1)
Sources/Fluid/Services/DebugLogger.swift (2)
info(141-143)warning(145-147)
🔇 Additional comments (22)
Sources/Fluid/Services/RewriteModeService.swift (1)
217-217: Numeric literal formatting improves readability.The change from
32000to32_000follows Swift idioms for formatting large numbers. This cosmetic improvement has no functional impact and enhances code clarity.Sources/Fluid/Services/CommandModeService.swift (1)
834-834: Numeric literal formatting improves readability.The change from
32000to32_000follows Swift idioms for formatting large numbers. This cosmetic improvement has no functional impact and enhances code clarity.Sources/Fluid/Services/MenuBarManager.swift (1)
290-290: LGTM!Formatting-only change with no functional impact.
Info.plist (1)
14-14: LGTM!Version bump from beta to release (
1.5.1-beta.3→1.5.1) is appropriate for the feature additions in this PR.Sources/Fluid/Services/AudioDeviceService.swift (1)
100-108: LGTM!Clean helper methods that extend the API without modifying existing behavior. The read-only design correctly avoids side effects on system audio settings.
Sources/Fluid/fluidApp.swift (1)
15-22: LGTM!This is the correct SwiftUI pattern for wrapping a shared singleton in
@StateObject. The explicitinit()with_appServices = StateObject(wrappedValue:)ensures proper ownership semantics and avoids potential issues with inline initialization of shared instances.Sources/Fluid/Persistence/SettingsStore.swift (2)
32-32: LGTM!New key follows the existing naming convention and is properly placed in the Keys enum.
294-305: LGTM!The property follows established patterns in this file: uses
object(forKey:)to handle nil vs false distinction, callsobjectWillChange.send()before mutation, and has clear documentation explaining the sync behavior.Sources/Fluid/UI/SettingsView.swift (6)
451-468: LGTM!Clean refactor moving the refresh functionality to the header. CoreAudio calls happen on user action (button tap) rather than in view body, which is the correct pattern.
490-493: Core feature: Conditional system sync.This correctly gates the system audio device change behind the new
syncAudioDevicesWithSystemsetting. When disabled, FluidVoice maintains independent device selection.
542-545: Consistent implementation for output device.Matches the input device pattern at lines 490-493.
568-578: Good fix for CoreAudio race condition.Using cached state variables instead of direct
AudioDevice.getDefaultInputDevice()calls in the view body avoids theHALSystem::InitializeShell()race with SwiftUI's AttributeGraph.
580-606: Sync toggle implementation looks correct.When sync is enabled, adopting the system's current devices as source of truth is the right behavior. The nil checks on lines 592 and 596 provide safe fallback when devices are unavailable.
One minor observation: when sync is toggled ON and the system default differs from the current selection, this triggers the
onChange(of: selectedInputUID)handler which will restart ASR if running—this is correct behavior to apply the new device immediately.
749-752: Critical fix applied correctly.Populating cached device names inside
onAppear(afterAudioStartupGate.shared.waitUntilOpen()) ensures CoreAudio calls happen after UI is settled, avoiding the race condition documented in the comments.Sources/Fluid/ContentView.swift (3)
39-49: Service accessor pattern looks good.The transition from
@StateObjectto@EnvironmentObjectwith computed property accessors correctly centralizes service lifecycle management while maintaining backward compatibility with existing code. This approach avoids duplicate service instantiation that was causing startup crashes.
168-190: Startup gate and lazy initialization strategy is well-documented.The multi-layer defensive strategy (consolidation, lazy init, startup gate, delayed audio init) with clear comments explains the rationale for the 1.5s delay. The sequence correctly signals UI readiness before accessing services, allowing lazy initialization to proceed safely.
527-537: Alert binding refactor is correct.The two-way binding using
Binding(get:set:)properly synchronizes the alert presentation state withasr.showError, allowing the alert dismissal to update the underlying state.Sources/Fluid/Services/AppServices.swift (1)
46-56: Lazy initialization pattern is sound but has a minor ordering consideration.The lazy initialization works correctly. However,
setupAudioObserverForwarding()is called after assigning to_audioObserverbut before returning. SincesetupAudioObserverForwarding()checksguard let observer = _audioObserver, this works correctly.One observation: if
audioObserveris accessed concurrently from multiple call sites on the main actor, the first caller creates the instance and subsequent callers get the cached one. This is correct behavior given the@MainActorconstraint.Sources/Fluid/Services/ASRService.swift (4)
453-459: Good defensive instantiation of inputNode before binding.Forcing
_ = self.engine.inputNodeensures the AUHAL AudioUnit exists before attempting to bind to a specific device. This prevents potential crashes when the AudioUnit hasn't been created yet.
461-482: Device binding logic is well-structured with appropriate fallback.The method correctly:
- Guards against sync-with-system mode
- Guards against empty/nil preferred UID
- Logs a warning and falls back gracefully when the preferred device isn't found
- Logs binding failures without crashing
555-575: Sync flag check inhandleDefaultInputChangedis correct.The early return when
syncAudioDevicesWithSystemis false correctly prevents the engine from restarting to follow system default changes in independent mode. The debug log provides visibility into why the change was ignored.
484-520: AudioUnit device binding implementation is correct.The use of kAudioOutputUnitProperty_CurrentDevice with kAudioUnitScope_Global is the standard approach for binding an AUHAL unit to a specific device. Error handling with OSStatus checking is appropriate and necessary.
The nil-check for
inputNode.audioUnitis important—if the audioUnit returns nil, it indicates the input node hasn't been properly initialized. While the current error logging handles this, it's worth monitoring if this occurs frequently as it may signal a deeper initialization issue.
| // macOS can temporarily disable event taps (e.g. timeouts, user input protection). | ||
| // If we don't immediately re-enable here, hotkeys will silently stop working until our | ||
| // periodic health check kicks in, and the OS may handle the key (e.g. system dictation). | ||
| if type == .tapDisabledByTimeout || type == .tapDisabledByUserInput { | ||
| let reason = (type == .tapDisabledByTimeout) ? "timeout" : "user input" | ||
| DebugLogger.shared.warning("Event tap disabled by \(reason) — attempting immediate re-enable", source: "GlobalHotkeyManager") | ||
|
|
||
| if let tap = self.eventTap { | ||
| CGEvent.tapEnable(tap: tap, enable: true) | ||
| } | ||
|
|
||
| // If re-enable failed, recreate the tap. | ||
| if !self.isEventTapEnabled() { | ||
| DebugLogger.shared.warning("Event tap re-enable failed — recreating tap", source: "GlobalHotkeyManager") | ||
| self.setupGlobalHotkeyWithRetry() | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Critical: Fix concurrency violation when recreating the event tap.
The call to setupGlobalHotkeyWithRetry() on line 231 is problematic because:
handleKeyEventisnonisolated(invoked from a C callback)setupGlobalHotkeyWithRetry()is implicitly@MainActor(the class is marked@MainActor)- Synchronously calling a
@MainActormethod from a nonisolated context violates Swift concurrency rules and can cause runtime errors or deadlocks
🔎 Proposed fix: Dispatch the retry call asynchronously to MainActor
// If re-enable failed, recreate the tap.
if !self.isEventTapEnabled() {
DebugLogger.shared.warning("Event tap re-enable failed — recreating tap", source: "GlobalHotkeyManager")
- self.setupGlobalHotkeyWithRetry()
+ Task { @MainActor in
+ self.setupGlobalHotkeyWithRetry()
+ }
}📝 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.
| // macOS can temporarily disable event taps (e.g. timeouts, user input protection). | |
| // If we don't immediately re-enable here, hotkeys will silently stop working until our | |
| // periodic health check kicks in, and the OS may handle the key (e.g. system dictation). | |
| if type == .tapDisabledByTimeout || type == .tapDisabledByUserInput { | |
| let reason = (type == .tapDisabledByTimeout) ? "timeout" : "user input" | |
| DebugLogger.shared.warning("Event tap disabled by \(reason) — attempting immediate re-enable", source: "GlobalHotkeyManager") | |
| if let tap = self.eventTap { | |
| CGEvent.tapEnable(tap: tap, enable: true) | |
| } | |
| // If re-enable failed, recreate the tap. | |
| if !self.isEventTapEnabled() { | |
| DebugLogger.shared.warning("Event tap re-enable failed — recreating tap", source: "GlobalHotkeyManager") | |
| self.setupGlobalHotkeyWithRetry() | |
| } | |
| return nil | |
| } | |
| // macOS can temporarily disable event taps (e.g. timeouts, user input protection). | |
| // If we don't immediately re-enable here, hotkeys will silently stop working until our | |
| // periodic health check kicks in, and the OS may handle the key (e.g. system dictation). | |
| if type == .tapDisabledByTimeout || type == .tapDisabledByUserInput { | |
| let reason = (type == .tapDisabledByTimeout) ? "timeout" : "user input" | |
| DebugLogger.shared.warning("Event tap disabled by \(reason) — attempting immediate re-enable", source: "GlobalHotkeyManager") | |
| if let tap = self.eventTap { | |
| CGEvent.tapEnable(tap: tap, enable: true) | |
| } | |
| // If re-enable failed, recreate the tap. | |
| if !self.isEventTapEnabled() { | |
| DebugLogger.shared.warning("Event tap re-enable failed — recreating tap", source: "GlobalHotkeyManager") | |
| Task { @MainActor in | |
| self.setupGlobalHotkeyWithRetry() | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Sources/Fluid/Services/GlobalHotkeyManager.swift around lines 217-235: the code
synchronously calls the @MainActor method setupGlobalHotkeyWithRetry() from the
nonisolated C callback context in handleKeyEvent, violating Swift concurrency
rules; instead, dispatch the retry asynchronously to the MainActor (for example,
create a Task that calls await MainActor.run { await
self.setupGlobalHotkeyWithRetry() } or Task { await
self.setupGlobalHotkeyWithRetry() } so the call happens asynchronously on the
actor, avoiding synchronous cross-actor calls and potential deadlocks.
1 participant
…ystem sound ; also have independent control
Description
Brief description of what this PR does.
Type of Change
Related Issues
Closes #(issue number)
Testing
brew install swiftlint && swiftlint --strict --config .swiftlint.ymlbrew install swiftformat && swiftformat --config .swiftformat SourcesScreenshots / Video
Add screenshots or Video recording of the app after you have made your changes
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.