Conversation
…de toggle - Replace engine.reset() with engine recreation to prevent format corruption - Add engine.prepare() before start() for proper AudioUnit initialization - Handle aggregate device binding gracefully (OSStatus -10851 for Bluetooth) - Remove sync mode toggle - always use system audio device settings - Add comprehensive debug logging throughout audio engine lifecycle - Add user-facing note about device syncing in Settings UI - Update documentation with AVAudioEngine best practices Fixes crash when starting 2nd recording after first recording completes. Fixes freeze when disconnecting Bluetooth devices while not recording. Simplifies UX by removing non-functional independent device mode. Technical details documented in docs/AV_AUDIOENGINE_REFERENCE.md
📝 WalkthroughWalkthroughBumps app to 1.5.3 and updates FluidAudio; preserves public signatures. Enforces always-on system device sync, adds a thread-safe AudioCapturePipeline, extensive ASR device-binding/monitoring and engine-retry logic, improves AudioDeviceService listener lifecycle, and wires NavigationSplitView column visibility state in ContentView. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as SettingsView / ContentView
participant ASR as ASRService
participant AudioDev as AudioDeviceService
participant CoreAudio as Core Audio System
User->>UI: Start Recording
UI->>ASR: start()
rect rgb(240,248,255)
note right of ASR: Pre-flight & Device Binding
ASR->>ASR: pre-flight checks, clear buffers
ASR->>AudioDev: getInputDevice(byUID) / cachedDeviceUIDs
AudioDev->>CoreAudio: query device properties
CoreAudio-->>AudioDev: device info
AudioDev-->>ASR: Device descriptor
ASR->>ASR: bindPreferredInputDeviceIfNeeded()
ASR->>ASR: setEngineInputDevice()/setEngineOutputDevice()
end
rect rgb(245,245,220)
note right of ASR: Start Engine & Monitoring
ASR->>ASR: startEngine() (bind before prepare, retries)
ASR->>ASR: setupEngineTap() -> pipeline.handle(buffer:)
ASR->>ASR: registerDeviceListChangeListener()
end
rect rgb(230,245,230)
note right of ASR: Audio Capture & Streaming
ASR->>ASR: AudioCapturePipeline -> resample/mono16k -> queue
ASR->>ASR: runStreamingLoop() -> processStreamingChunk() -> transcription
end
CoreAudio-->>ASR: device list / default device change event
alt preferred Bluetooth reappears
ASR->>ASR: update SettingsStore.preferredInputDeviceUID
ASR->>ASR: restartEngineWithDevice(newDevice)
else current device disconnected
ASR->>ASR: handleDeviceAvailabilityChanged() -> bind fallback or stop
end
User->>UI: Stop Recording
UI->>ASR: stop()
rect rgb(255,240,245)
note right of ASR: Shutdown
ASR->>ASR: disable pipeline, stopMonitoringDevice(), remove taps
ASR->>ASR: await pending tasks, ensure final transcription
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 6
🧹 Nitpick comments (3)
Info.plist (1)
11-14: Consider incrementing CFBundleVersion.
CFBundleShortVersionStringis updated to 1.5.3, butCFBundleVersionremains "1". Apple recommends incrementingCFBundleVersionwith each build to help with crash report symbolication and App Store submission. This is a minor concern but worth noting for release hygiene.Sources/Fluid/Persistence/SettingsStore.swift (1)
315-326: Approve the always-on sync mode for reliability.Hardcoding
syncAudioDevicesWithSystemto always returntrueis a sound fix for the OSStatus -10851 errors with Bluetooth/aggregate devices. The comment clearly documents the rationale.Note: Code that checks
SettingsStore.shared.syncAudioDevicesWithSystem(e.g., ContentView lines 552, 578) now has effectively deadelsebranches. Consider cleaning these up in a follow-up to reduce confusion.Sources/Fluid/Services/ASRService.swift (1)
918-972: Unused methods:unbindInputDevice()andunbindOutputDevice()are never called.These methods are well-documented for releasing Bluetooth devices to restore A2DP quality, but they appear to be dead code—no call sites exist in the codebase. Either implement the calls if they're needed for proper audio device cleanup, or remove them if obsolete.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Info.plistPackage.resolvedPackage.swiftSources/Fluid/ContentView.swiftSources/Fluid/Persistence/SettingsStore.swiftSources/Fluid/Services/ASRService.swiftSources/Fluid/Services/AudioDeviceService.swiftSources/Fluid/UI/SettingsView.swift
🧰 Additional context used
🧬 Code graph analysis (3)
Sources/Fluid/ContentView.swift (1)
Sources/Fluid/UI/MouseTracker.swift (1)
withMouseTracking(90-92)
Sources/Fluid/UI/SettingsView.swift (2)
Sources/Fluid/Services/DebugLogger.swift (1)
warning(145-147)Sources/Fluid/Services/AudioDeviceService.swift (3)
setDefaultInputDevice(91-95)getDefaultInputDevice(81-84)getDefaultOutputDevice(86-89)
Sources/Fluid/Services/ASRService.swift (6)
Sources/Fluid/Services/AudioDeviceService.swift (5)
listInputDevices(73-75)getInputDevice(104-106)getOutputDevice(109-111)getDefaultInputDevice(81-84)getDefaultOutputDevice(86-89)Sources/Fluid/Services/DebugLogger.swift (5)
info(141-143)error(149-151)warning(145-147)debug(153-155)clear(118-122)Sources/Fluid/Services/ThreadSafeAudioBuffer.swift (3)
clear(17-21)getPrefix(31-36)append(10-14)Sources/Fluid/Services/AppleSpeechAnalyzerProvider.swift (2)
transcribe(140-234)prepare(39-267)Sources/Fluid/Services/AppleSpeechProvider.swift (2)
transcribe(62-116)prepare(32-50)Sources/Fluid/Services/WhisperProvider.swift (2)
transcribe(68-85)prepare(36-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build FluidVoice
🔇 Additional comments (26)
Package.resolved (1)
26-27: LGTM!The resolved version 0.8.1 correctly satisfies the
from: "0.8.0"constraint in Package.swift.Sources/Fluid/ContentView.swift (1)
132-148: LGTM! Layout change from NavigationSplitView to fixed HStack.The refactor to a fixed-width sidebar (250pt) with a non-draggable separator addresses the resize-to-zero issue mentioned in the PR. The implementation is clean and uses proper theming for the separator color.
One consideration: the fixed 250pt sidebar width may feel cramped on smaller windows, but this is a reasonable trade-off for preventing the zero-width bug.
Sources/Fluid/UI/SettingsView.swift (3)
533-543: LGTM! Clear user communication about sync behavior.The info note effectively communicates that audio devices are synced with macOS System Settings, replacing the removed manual toggle. The UI is clean and uses appropriate styling.
564-574: Good defensive guard against device changes during recording.Preventing device changes while
asr.isRunningis true avoids potential audio corruption or crashes. The revert tooldUIDensures the UI stays consistent with the actual state.
556-558: LGTM! Cached device name avoids CoreAudio race condition.Using the cached
cachedDefaultInputNameinstead of callingAudioDevice.getDefaultInputDevice()during view body evaluation prevents the HALSystem initialization race with SwiftUI's AttributeGraph.Sources/Fluid/Services/AudioDeviceService.swift (5)
44-54: LGTM! Safer buffer access pattern.Using
withUnsafeMutableByteswith explicitbaseAddressunwrapping is the correct modern Swift pattern for passing buffers to C APIs. ReturningkAudioHardwareUnspecifiedErroron nil baseAddress is appropriate defensive handling.
108-111: LGTM! Symmetric API addition.Adding
getOutputDevice(byUID:)mirrors the existinggetInputDevice(byUID:)method, providing consistent access patterns for both device types.
189-189: Correct alignment fix.Using
MemoryLayout<AudioBufferList>.alignmentis more accurate than the previous alignment and ensures proper memory access for theAudioBufferListstructure.
216-218: Excellent listener lifecycle management.Storing listener tokens as instance variables and implementing proper partial-failure cleanup (lines 272-288) prevents resource leaks and orphaned listeners. The cleanup logic correctly removes any successfully registered listeners before returning on failure.
Also applies to: 258-293
296-331: LGTM! Robust unregistration with token cleanup.The unregister method now correctly removes listeners using their stored tokens and resets the token state. This ensures clean teardown and prevents double-removal attempts.
Package.swift (1)
13-13: Review FluidAudio 0.8.0 and 0.8.1 release notes for compatibility with existing code.The dependency is updated from 0.7.4 to 0.8.0 (resolved to 0.8.1). Version 0.8.0 introduces significant new features including Parakeet EOU streaming ASR (StreamingEouAsrManager, RnntDecoder, NeMoMelSpectrogram) and automatic Hugging Face model downloads, while 0.8.1 adds transcription progress percentage, speaker constraints, and bug fixes. Verify that the codebase integrates properly with these changes and that no existing usages of FluidAudio APIs are affected.
Sources/Fluid/Services/ASRService.swift (15)
267-277: LGTM - Good thread-safety pattern for audio capture.The lazy
AudioCapturePipelinecorrectly decouples the realtime audio callback from the@MainActorstate. Using[weak self]in theonLevelclosure prevents retain cycles, and dispatching to the main queue for Combine sends is appropriate.
301-305: LGTM - Proper device monitoring initialization.The device list listener registration and cache initialization establish the baseline for detecting newly connected devices. This ordering ensures accurate change detection.
436-441: LGTM - Device monitoring during recording.Starting device monitoring for the currently bound device enables detection of mid-recording disconnections, which is critical for the Bluetooth stability fix mentioned in PR objectives.
533-538: LGTM - Engine recreation instead of reset.Replacing
engine.reset()withengineStorage = nil(forcing lazy recreation) is the correct fix for the format corruption issue that caused freezes on consecutive recordings. The comment properly documents the rationale.
600-634: LGTM - Consistent with stop() changes.The
stopWithoutTranscription()method mirrors the engine recreation pattern fromstop(), maintaining consistency in the shutdown behavior.
666-708: LGTM - Robust device binding with fallback.The method properly handles the case where the preferred device is unavailable by falling back to system defaults. The return value allows callers to detect binding failures.
710-752: LGTM - Consistent with input device binding pattern.The output device binding follows the same robust fallback pattern as input device binding.
754-818: LGTM - Fallback helpers for system defaults.These helper methods properly handle the edge case where no system default device exists, returning
falseto signal binding failure to the caller.
848-866: LGTM - Key Bluetooth stability fix.The special handling of OSStatus
-10851(aggregate device error) for Bluetooth devices is the correct approach. Treating this as a warning rather than a fatal error allows graceful fallback to system defaults, addressing the app hanging issue on Bluetooth disconnect.
869-916: LGTM - Consistent with input device binding.The output device binding follows the same pattern with proper handling of aggregate device errors.
974-1058: LGTM - Robust engine start with retry logic.The 3-attempt retry loop with engine recreation between failures provides good resilience. Preserving the underlying error in the final exception helps debugging. The decision to continue even when device binding fails is appropriate since the engine falls back to system defaults.
1103-1111: LGTM - Consistent engine recreation on device change.The handler now follows the same engine recreation pattern used in
stop(), maintaining consistency.
1331-1340: LGTM - Proper handling of device disconnect during recording.The code correctly cancels the recording and notifies the user when the audio device disconnects mid-session. Using
Task { @MainActor in }is appropriate since the Core Audio callback runs on DispatchQueue.main but not within the MainActor executor.
1887-2027: LGTM - Well-designed thread-safe audio capture pipeline.The
AudioCapturePipelineclass correctly isolates realtime audio processing from the@MainActor. Key observations:
NSLockusage is appropriate with short critical sections- vDSP is used for efficient audio level calculation
- The
onLevelcallback properly delegates main-thread delivery to the callerThe linear interpolation resampler (lines 2006-2026) may introduce minor aliasing, but this is acceptable for ASR where speech intelligibility matters more than audio fidelity.
1696-1716: LGTM - Good defensive validation before transcription.The minimum sample requirement (16,000 samples = 1 second at 16kHz) prevents submitting insufficient audio that would cause transcription failures. The empty chunk check at lines 1712-1716 handles a rare edge case where the buffer returns empty despite a positive count.
| guard inFormat.sampleRate > 0, inFormat.channelCount > 0 else { | ||
| DebugLogger.shared.error( | ||
| "❌ INVALID INPUT FORMAT: \(inFormat.sampleRate)Hz \(inFormat.channelCount)ch - Cannot install tap!", | ||
| source: "ASRService" | ||
| ) | ||
| fatalError("AVAudioEngine input format is invalid. This should never happen with fresh engine instance.") | ||
| } |
There was a problem hiding this comment.
Consider replacing fatalError() with recoverable error handling.
While the comment states this "should never happen with fresh engine instance," using fatalError() will crash the app if the assumption is violated. A more resilient approach would be to throw an error, allowing start() to handle the failure gracefully and notify the user.
Suggested fix
guard inFormat.sampleRate > 0, inFormat.channelCount > 0 else {
DebugLogger.shared.error(
"❌ INVALID INPUT FORMAT: \(inFormat.sampleRate)Hz \(inFormat.channelCount)ch - Cannot install tap!",
source: "ASRService"
)
- fatalError("AVAudioEngine input format is invalid. This should never happen with fresh engine instance.")
+ // Don't install tap - start() will fail when isRunning check happens
+ // Caller should detect this via engine state
+ return
}Then update start() to verify the tap was successfully installed before setting isRunning = true.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Sources/Fluid/Services/ASRService.swift around lines 1071–1077, replace the
unconditional fatalError() on invalid AVAudioEngine input format with
recoverable error handling: define (or add) an ASRServiceError case such as
invalidInputFormat(sampleRate: Int, channelCount: Int), log the error with
DebugLogger including the values, and throw that error instead of calling
fatalError(); then update the calling start() function signature to throw or to
catch this error, verify the tap installation succeeded before setting isRunning
= true, and handle the failure path by logging/propagating the error and not
toggling isRunning so the app can recover gracefully.
| // Check for newly connected Bluetooth devices (auto-switch) | ||
| for device in currentDevices { | ||
| if device.name.localizedCaseInsensitiveContains("airpods") || | ||
| device.name.localizedCaseInsensitiveContains("bluetooth") | ||
| { | ||
| // Check if this is a newly appeared device | ||
| let wasAvailable = self.checkDeviceWasAvailable(device.uid) | ||
| if !wasAvailable { | ||
| DebugLogger.shared.info( | ||
| "🎧 New Bluetooth device detected: '\(device.name)'. Auto-switching...", | ||
| source: "ASRService" | ||
| ) | ||
|
|
||
| // Update preferred device setting | ||
| SettingsStore.shared.preferredInputDeviceUID = device.uid | ||
| DebugLogger.shared.debug("Updated preferred input device to: \(device.uid)", source: "ASRService") | ||
|
|
||
| // If recording, restart with new device | ||
| if self.isRunning { | ||
| DebugLogger.shared.info("Recording in progress - restarting engine with new Bluetooth device", source: "ASRService") | ||
| self.restartEngineWithDevice(device) | ||
| } else { | ||
| DebugLogger.shared.info("Not recording - Bluetooth device will be used on next recording", source: "ASRService") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Auto-switching overwrites user's preferred device without consent.
When a Bluetooth device is detected, line 1285 overwrites preferredInputDeviceUID with the new device's UID. This may surprise users who have intentionally configured a different preferred device.
Consider:
- Only auto-switch if no preferred device is explicitly set, OR
- Prompt the user before changing their preference, OR
- Use a temporary binding without persisting to preferences
Also, substring matching on "airpods" or "bluetooth" is fragile and may match unintended devices.
Suggested fix
// Check for newly connected Bluetooth devices (auto-switch)
for device in currentDevices {
if device.name.localizedCaseInsensitiveContains("airpods") ||
device.name.localizedCaseInsensitiveContains("bluetooth")
{
// Check if this is a newly appeared device
let wasAvailable = self.checkDeviceWasAvailable(device.uid)
if !wasAvailable {
DebugLogger.shared.info(
"🎧 New Bluetooth device detected: '\(device.name)'. Auto-switching...",
source: "ASRService"
)
- // Update preferred device setting
- SettingsStore.shared.preferredInputDeviceUID = device.uid
- DebugLogger.shared.debug("Updated preferred input device to: \(device.uid)", source: "ASRService")
+ // Only auto-switch if user hasn't set a preferred device
+ // or if the preferred device is the one we're already using
+ let hasExplicitPreference = SettingsStore.shared.preferredInputDeviceUID != nil
+ && !SettingsStore.shared.preferredInputDeviceUID!.isEmpty
+
+ if !hasExplicitPreference {
+ SettingsStore.shared.preferredInputDeviceUID = device.uid
+ DebugLogger.shared.debug("Updated preferred input device to: \(device.uid)", source: "ASRService")
+ } else {
+ DebugLogger.shared.debug("User has explicit preference, not auto-updating", source: "ASRService")
+ }
// If recording, restart with new device📝 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.
| // Check for newly connected Bluetooth devices (auto-switch) | |
| for device in currentDevices { | |
| if device.name.localizedCaseInsensitiveContains("airpods") || | |
| device.name.localizedCaseInsensitiveContains("bluetooth") | |
| { | |
| // Check if this is a newly appeared device | |
| let wasAvailable = self.checkDeviceWasAvailable(device.uid) | |
| if !wasAvailable { | |
| DebugLogger.shared.info( | |
| "🎧 New Bluetooth device detected: '\(device.name)'. Auto-switching...", | |
| source: "ASRService" | |
| ) | |
| // Update preferred device setting | |
| SettingsStore.shared.preferredInputDeviceUID = device.uid | |
| DebugLogger.shared.debug("Updated preferred input device to: \(device.uid)", source: "ASRService") | |
| // If recording, restart with new device | |
| if self.isRunning { | |
| DebugLogger.shared.info("Recording in progress - restarting engine with new Bluetooth device", source: "ASRService") | |
| self.restartEngineWithDevice(device) | |
| } else { | |
| DebugLogger.shared.info("Not recording - Bluetooth device will be used on next recording", source: "ASRService") | |
| } | |
| } | |
| } | |
| // Check for newly connected Bluetooth devices (auto-switch) | |
| for device in currentDevices { | |
| if device.name.localizedCaseInsensitiveContains("airpods") || | |
| device.name.localizedCaseInsensitiveContains("bluetooth") | |
| { | |
| // Check if this is a newly appeared device | |
| let wasAvailable = self.checkDeviceWasAvailable(device.uid) | |
| if !wasAvailable { | |
| DebugLogger.shared.info( | |
| "🎧 New Bluetooth device detected: '\(device.name)'. Auto-switching...", | |
| source: "ASRService" | |
| ) | |
| // Only auto-switch if user hasn't set a preferred device | |
| // or if the preferred device is the one we're already using | |
| let hasExplicitPreference = SettingsStore.shared.preferredInputDeviceUID != nil | |
| && !SettingsStore.shared.preferredInputDeviceUID!.isEmpty | |
| if !hasExplicitPreference { | |
| SettingsStore.shared.preferredInputDeviceUID = device.uid | |
| DebugLogger.shared.debug("Updated preferred input device to: \(device.uid)", source: "ASRService") | |
| } else { | |
| DebugLogger.shared.debug("User has explicit preference, not auto-updating", source: "ASRService") | |
| } | |
| // If recording, restart with new device | |
| if self.isRunning { | |
| DebugLogger.shared.info("Recording in progress - restarting engine with new Bluetooth device", source: "ASRService") | |
| self.restartEngineWithDevice(device) | |
| } else { | |
| DebugLogger.shared.info("Not recording - Bluetooth device will be used on next recording", source: "ASRService") | |
| } | |
| } | |
| } |
| do { | ||
| // Recreate engine for hot-swap instead of reset | ||
| DebugLogger.shared.debug("Recreating engine for device switch...", source: "ASRService") | ||
| self.engineStorage = nil | ||
| _ = self.engine.inputNode // Create fresh engine with new node | ||
| _ = self.setEngineInputDevice(deviceID: device.id, deviceUID: device.uid, deviceName: device.name) | ||
|
|
||
| DebugLogger.shared.debug("Starting engine with new device...", source: "ASRService") | ||
| try self.startEngine() | ||
|
|
||
| DebugLogger.shared.debug("Setting up engine tap...", source: "ASRService") | ||
| self.setupEngineTap() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the ASRService.swift file and understand the method context
fd "ASRService.swift" --type fRepository: altic-dev/FluidVoice
Length of output: 103
🏁 Script executed:
# Read the restartEngineWithDevice method (lines 1353-1396 as mentioned in the comment)
sed -n '1353,1396p' Sources/Fluid/Services/ASRService.swiftRepository: altic-dev/FluidVoice
Length of output: 2039
🏁 Script executed:
# Now let's find and read the startEngine method
rg -n "func startEngine\(" Sources/Fluid/Services/ASRService.swift -A 20Repository: altic-dev/FluidVoice
Length of output: 1455
🏁 Script executed:
# Find bindPreferredInputDeviceIfNeeded method
rg -n "func bindPreferredInputDeviceIfNeeded" Sources/Fluid/Services/ASRService.swift -A 15Repository: altic-dev/FluidVoice
Length of output: 1017
🏁 Script executed:
# Find the setEngineInputDevice method for comparison
rg -n "func setEngineInputDevice" Sources/Fluid/Services/ASRService.swift -A 20Repository: altic-dev/FluidVoice
Length of output: 1099
🏁 Script executed:
# Check the syncAudioDevicesWithSystem logic
rg -n "syncAudioDevicesWithSystem" Sources/Fluid/Services/ASRService.swift -B 2 -A 5Repository: altic-dev/FluidVoice
Length of output: 1607
🏁 Script executed:
# Read the full bindPreferredInputDeviceIfNeeded method
sed -n '670,710p' Sources/Fluid/Services/ASRService.swiftRepository: altic-dev/FluidVoice
Length of output: 2120
🏁 Script executed:
# Read the full setEngineInputDevice method
sed -n '823,880p' Sources/Fluid/Services/ASRService.swiftRepository: altic-dev/FluidVoice
Length of output: 2833
Logic issue confirmed: startEngine() overwrites the device bound for hot-swap.
At line 1376, setEngineInputDevice() binds the AudioUnit to the target device. However, startEngine() at line 1379 calls bindPreferredInputDeviceIfNeeded(), which will rebind to a different device if:
syncAudioDevicesWithSystem == false(independent mode), ANDpreferredInputDeviceUIDis set and differs from the hot-swap target
The rebinding occurs silently via AudioUnitSetProperty() at line 695 in bindPreferredInputDeviceIfNeeded(), causing the hot-swap to use the wrong device without error.
Suggested fixes are appropriate. Consider updating preferredInputDeviceUID temporarily before calling startEngine(), creating a hot-swap variant of startEngine() that skips device binding, or passing the target device to startEngine() to use instead of stored preferences.
🤖 Prompt for AI Agents
In Sources/Fluid/Services/ASRService.swift around lines 1371–1382, startEngine()
currently calls bindPreferredInputDeviceIfNeeded() which can silently rebind the
AudioUnit and override the device just set by setEngineInputDevice(...). Fix by
ensuring startEngine() does not rebind during a hot-swap: either (A) temporarily
set preferredInputDeviceUID = device.uid before calling startEngine() and
restore the previous value immediately after, or (B) add a hot-swap-safe variant
to startEngine(e.g. startEngine(skipDeviceBinding: Bool)) that skips calling
bindPreferredInputDeviceIfNeeded() when true and call that variant here;
implement the chosen option and ensure the previous preference is restored or
skip-flag is used only for this hot-swap path.
| // Health check: detect if audio is not being captured | ||
| loopCount += 1 | ||
| if loopCount >= 3 { // After 3 loops (~6 seconds with 2s interval) | ||
| let currentBufferCount = self.audioBuffer.count | ||
| if currentBufferCount == lastBufferCount, currentBufferCount < 16_000 { | ||
| DebugLogger.shared.warning( | ||
| "Audio buffer not growing after \(loopCount * 2) seconds (count: \(currentBufferCount)). " + | ||
| "Audio capture may have failed. Check if engine is running and tap is installed.", | ||
| source: "ASRService" | ||
| ) | ||
| } | ||
| lastBufferCount = currentBufferCount | ||
| loopCount = 0 | ||
| } |
There was a problem hiding this comment.
Comment references incorrect interval.
The comment at line 1653 says "After 3 loops (~6 seconds with 2s interval)" but chunkDurationSeconds is 0.6 seconds (line 257), so 3 loops ≈ 1.8 seconds. The warning message at line 1657 also incorrectly references loopCount * 2 seconds.
Suggested fix
// Health check: detect if audio is not being captured
loopCount += 1
- if loopCount >= 3 { // After 3 loops (~6 seconds with 2s interval)
+ if loopCount >= 5 { // After 5 loops (~3 seconds with 0.6s interval)
let currentBufferCount = self.audioBuffer.count
if currentBufferCount == lastBufferCount, currentBufferCount < 16_000 {
DebugLogger.shared.warning(
- "Audio buffer not growing after \(loopCount * 2) seconds (count: \(currentBufferCount)). " +
+ "Audio buffer not growing after ~\(String(format: "%.1f", Double(loopCount) * chunkDurationSeconds)) seconds (count: \(currentBufferCount)). " +
"Audio capture may have failed. Check if engine is running and tap is installed.",
source: "ASRService"
)📝 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.
| // Health check: detect if audio is not being captured | |
| loopCount += 1 | |
| if loopCount >= 3 { // After 3 loops (~6 seconds with 2s interval) | |
| let currentBufferCount = self.audioBuffer.count | |
| if currentBufferCount == lastBufferCount, currentBufferCount < 16_000 { | |
| DebugLogger.shared.warning( | |
| "Audio buffer not growing after \(loopCount * 2) seconds (count: \(currentBufferCount)). " + | |
| "Audio capture may have failed. Check if engine is running and tap is installed.", | |
| source: "ASRService" | |
| ) | |
| } | |
| lastBufferCount = currentBufferCount | |
| loopCount = 0 | |
| } | |
| // Health check: detect if audio is not being captured | |
| loopCount += 1 | |
| if loopCount >= 5 { // After 5 loops (~3 seconds with 0.6s interval) | |
| let currentBufferCount = self.audioBuffer.count | |
| if currentBufferCount == lastBufferCount, currentBufferCount < 16_000 { | |
| DebugLogger.shared.warning( | |
| "Audio buffer not growing after ~\(String(format: "%.1f", Double(loopCount) * chunkDurationSeconds)) seconds (count: \(currentBufferCount)). " + | |
| "Audio capture may have failed. Check if engine is running and tap is installed.", | |
| source: "ASRService" | |
| ) | |
| } | |
| lastBufferCount = currentBufferCount | |
| loopCount = 0 | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
Sources/Fluid/Services/ASRService.swift (4)
1372-1383: Hot-swap logic relies on sync mode being always-on to avoid rebinding conflict.The direct
setEngineInputDevice()call at line 1377 could be overwritten bybindPreferredInputDeviceIfNeeded()insidestartEngine()if sync mode were ever disabled. Currently, sync mode is always enabled (per PR summary), sobindPreferredInputDeviceIfNeeded()returns early. Consider adding a comment documenting this dependency, or passing a skip-binding flag tostartEngine()for future-proofing.
1072-1078: Consider replacingfatalError()with recoverable error handling.While the comment states this "should never happen with fresh engine instance," using
fatalError()will crash the app if the assumption is violated. A more resilient approach would be to throw an error or return early, allowingstart()to handle the failure gracefully.
1272-1297: Auto-switching overwrites user's preferred device without consent.When a Bluetooth device is detected, line 1286 unconditionally overwrites
preferredInputDeviceUID. This may surprise users who have intentionally configured a different preferred device. Consider only auto-switching if no preferred device is explicitly set.
1652-1665: Comment references incorrect interval.The comment at line 1654 says "~6 seconds with 2s interval" but
chunkDurationSecondsis 0.6 seconds (line 257). Three loops equals approximately 1.8 seconds. The warning message at line 1658 also incorrectly referencesloopCount * 2seconds.
🧹 Nitpick comments (2)
Sources/Fluid/Services/ASRService.swift (2)
1032-1038: Consider logging configuration errors during retry.Using
try?at line 1036 silently discards configuration errors. While the next iteration will fail if configuration is broken, logging the error would aid debugging.Suggested improvement
if attempts < 3 { DebugLogger.shared.debug("⚠️ Start failed, recreating engine for retry...", source: "ASRService") self.engineStorage = nil // Deallocate failed engine // Need to reconfigure the new engine - try? self.configureSession() + do { + try self.configureSession() + } catch { + DebugLogger.shared.warning("configureSession() failed during retry: \(error)", source: "ASRService") + } DebugLogger.shared.debug("✅ Engine recreated and reconfigured, will retry", source: "ASRService") }
1106-1112: Silent error swallowing in device change handler.The empty
catch {}block at line 1112 silently ignores configuration/startup errors when handling device changes. Consider logging the error for debugging purposes.Suggested improvement
do { try self.configureSession() try self.startEngine() self.setupEngineTap() - } catch {} + } catch { + DebugLogger.shared.error("Failed to restart engine after device change: \(error)", source: "ASRService") + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Sources/Fluid/Services/ASRService.swiftSources/Fluid/Services/AudioDeviceService.swift
🧰 Additional context used
🧬 Code graph analysis (1)
Sources/Fluid/Services/ASRService.swift (3)
Sources/Fluid/Services/AudioDeviceService.swift (5)
listInputDevices(73-75)getInputDevice(104-106)getOutputDevice(109-111)getDefaultInputDevice(81-84)getDefaultOutputDevice(86-89)Sources/Fluid/Services/DebugLogger.swift (5)
info(141-143)error(149-151)warning(145-147)debug(153-155)clear(118-122)Sources/Fluid/Services/ThreadSafeAudioBuffer.swift (3)
clear(17-21)getPrefix(31-36)append(10-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build FluidVoice
🔇 Additional comments (15)
Sources/Fluid/Services/AudioDeviceService.swift (6)
44-54: LGTM - Safe buffer access pattern.Using
withUnsafeMutableByteswith a guard onbaseAddressis the correct approach for safely passing the buffer to CoreAudio APIs. ReturningkAudioHardwareUnspecifiedErrorwhenbaseAddressis nil handles the edge case appropriately.
108-111: LGTM - Symmetric helper method.The new
getOutputDevice(byUID:)mirrors the existinggetInputDevice(byUID:)pattern, providing consistent API surface for device lookup.
167-173: LGTM - Memory management corrected.The change from
takeUnretainedValue()totakeRetainedValue()correctly handles CoreAudio's +1 retained CFString ownership semantics. This fixes the memory leak identified in the previous review.
189-189: LGTM - Correct alignment for AudioBufferList.Using
MemoryLayout<AudioBufferList>.alignmentinstead of a generic alignment ensures proper memory layout for the variable-lengthAudioBufferListstructure.
216-218: LGTM - Robust listener lifecycle management.Storing listener tokens and performing partial cleanup on registration failure prevents resource leaks. The pattern ensures consistent state even when only some listeners register successfully.
Also applies to: 258-293
296-331: LGTM - Token-based unregistration.Explicitly removing listeners using their stored tokens ensures proper cleanup. The nil-checks before removal and resetting token state afterward maintain consistency.
Sources/Fluid/Services/ASRService.swift (9)
267-277: LGTM - Thread-safe audio capture pipeline.The
AudioCapturePipelineproperly isolates CoreAudio's realtime thread from@MainActorstate. Using[weak self]in theonLevelclosure and dispatching to main queue for Combine sends is correct.
396-472: LGTM - Comprehensive error handling and logging.The enhanced
start()method provides granular debug logging for troubleshooting and posts a user-friendly notification on failure. The error message extraction fromNSErrordomains is thorough.
533-538: Good approach - Engine recreation instead of reset.Replacing
engine.reset()with full engine recreation (engineStorage = nil) prevents format corruption issues. This aligns with the PR objective of fixing consecutive recording crashes.
660-662: Comment now matches implementation.The comment correctly states that device binding occurs in
startEngine()beforeengine.prepare(), which matches the actual code flow at lines 986-1004.
849-863: Good handling of aggregate device errors.The special case handling for OSStatus
-10851(kAudioUnitErr_InvalidPropertyValue) gracefully handles Bluetooth and aggregate devices that cannot be directly bound. Logging it as a warning rather than an error is appropriate since the system default fallback will be used.
1697-1708: LGTM - Reasonable minimum audio threshold.Requiring at least 1 second (16,000 samples at 16kHz) before transcription aligns with typical ASR model requirements and prevents wasted processing cycles on very short audio snippets.
1888-1935: LGTM - Thread-safe audio pipeline design.The
AudioCapturePipelineclass properly isolates realtime audio thread operations from@MainActorstate. UsingNSLockfor the short critical sections is appropriate. The design cleanly separates buffer management from level computation.
2007-2027: Linear interpolation resampling is acceptable for speech.The simple linear interpolation resampler at lines 2014-2025 is sufficient for speech transcription where ultra-high fidelity is not required. For production audio quality, a proper sinc-based resampler (e.g., via
vDSPorAudioConverter) would be preferred, but this is fine for ASR purposes.
1304-1351: LGTM - Robust device disconnection handling.The
handleDeviceAvailabilityChangedmethod properly distinguishes between recording and idle states, canceling recording on disconnect and stopping monitoring gracefully otherwise. The notification allows UI to inform users.
| /// Explicitly unbinds the input device from AVAudioEngine's AudioUnit | ||
| /// This is CRITICAL for releasing Bluetooth devices so macOS can switch back to high-quality A2DP mode | ||
| private func unbindInputDevice() { | ||
| DebugLogger.shared.debug("unbindInputDevice() - Releasing input device binding to restore Bluetooth quality", source: "ASRService") | ||
|
|
||
| guard let audioUnit = self.engine.inputNode.audioUnit else { | ||
| DebugLogger.shared.warning("No AudioUnit for input node - cannot unbind device", source: "ASRService") | ||
| return | ||
| } | ||
|
|
||
| // Set device to kAudioObjectUnknown (0) to explicitly release the device binding | ||
| var unknownDevice = AudioObjectID(kAudioObjectUnknown) | ||
| let status = AudioUnitSetProperty( | ||
| audioUnit, | ||
| kAudioOutputUnitProperty_CurrentDevice, | ||
| kAudioUnitScope_Global, | ||
| 0, | ||
| &unknownDevice, | ||
| UInt32(MemoryLayout<AudioObjectID>.size) | ||
| ) | ||
|
|
||
| if status == noErr { | ||
| DebugLogger.shared.info("✅ Input device unbound - Bluetooth can now return to high-quality mode", source: "ASRService") | ||
| } else { | ||
| DebugLogger.shared.error("❌ Failed to unbind input device: OSStatus \(status)", source: "ASRService") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
CoreAudio AudioUnitSetProperty kAudioOutputUnitProperty_CurrentDevice kAudioObjectUnknown device unbind
💡 Result:
Short answer: You can't "unbind" an AUHAL by passing kAudioObjectUnknown to kAudioOutputUnitProperty_CurrentDevice. The CurrentDevice property expects a valid AudioDeviceID; using kAudioObjectUnknown is not a supported way to detach and will fail. Instead disable IO on the unit or set the property to a valid device (for example the system default) before initializing/starting the unit. [1][2][3]
Why / how to do it:
- Apple TN2091 and docs show kAudioOutputUnitProperty_CurrentDevice is read/write of an AudioDeviceID and must be set to a real device (and must be set after enabling IO and before initialization in many cases). They show examples setting it to the default device, not kAudioObjectUnknown. [1][2]
- Common implementations (Chromium, other projects) enable IO, set CurrentDevice to a concrete AudioDeviceID, then register callbacks; they do not use kAudioObjectUnknown to unbind. To stop association you either turn off IO on that bus (kAudioOutputUnitProperty_EnableIO = 0) or set CurrentDevice to another valid device (e.g. the default). [3][4]
References:
- Apple Technical Note TN2091 (setting kAudioOutputUnitProperty_CurrentDevice example). [1]
- Apple AudioUnit property doc / kAudioOutputUnitProperty_CurrentDevice (property is an AudioObjectID). [2]
- Chromium audio code showing proper usage (enable IO then set device; no kAudioObjectUnknown unbind). [3]
- Example guidance on enabling/disabling IO and setting device before init (community examples / guides). [4]
If you want, tell me whether you need example code to (a) disable IO to detach the unit, or (b) switch to the system default device — I can paste a minimal C/Swift example.
Use a valid AudioDeviceID instead of kAudioObjectUnknown—setting the device property to kAudioObjectUnknown is not supported and will fail.
CoreAudio documentation (Apple TN2091) and implementation examples from Chromium show that kAudioOutputUnitProperty_CurrentDevice requires a real AudioDeviceID. To release the input device binding, either disable IO on the unit via kAudioOutputUnitProperty_EnableIO = 0 or set CurrentDevice to a valid device such as the system default. Using kAudioObjectUnknown will not unbind the device and the operation will fail, preventing Bluetooth from returning to high-quality mode.
🤖 Prompt for AI Agents
In Sources/Fluid/Services/ASRService.swift around lines 919-945, the code
attempts to unbind the input device by writing kAudioObjectUnknown into
kAudioOutputUnitProperty_CurrentDevice which is unsupported and will fail;
instead either (A) set the unit's input IO to disabled by calling
AudioUnitSetProperty with kAudioOutputUnitProperty_EnableIO = 0 on the input
scope/index, or (B) obtain a valid AudioDeviceID (for example the system default
input device via AudioObjectGetPropertyData for
kAudioHardwarePropertyDefaultInputDevice) and write that AudioDeviceID into
kAudioOutputUnitProperty_CurrentDevice; update the function to choose one of
these valid approaches, check OSStatus results, and log success/failure
accordingly.
…hstack version as it was inconsistent among different OS versions
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Sources/Fluid/ContentView.swift (2)
130-130: Column visibility state looks good.The
columnVisibilitystate properly controls the NavigationSplitView's column display. If you'd like users' column visibility preference to persist across app launches, consider storing this inSettingsStoresimilar to other preferences.
133-143: Consider simplifying the intermediate variable pattern.The code creates multiple intermediate
letvariables (layout,tracked,env,nav) within thebodycomputed property. While functional, this pattern is unconventional in SwiftUI. Typically, modifiers are chained directly on the view. However, if this pattern aids in debugging or readability for your team, it's acceptable to keep.🔎 Alternative: Direct modifier chaining
- let layout = AnyView( - NavigationSplitView(columnVisibility: self.$columnVisibility) { + return NavigationSplitView(columnVisibility: self.$columnVisibility) { self.sidebarView .navigationSplitViewColumnWidth(min: 220, ideal: 250, max: 300) } detail: { self.detailView } .navigationSplitViewStyle(.balanced) - ) - - let tracked = layout.withMouseTracking(self.mouseTracker) - let env = tracked.environmentObject(self.mouseTracker) - let nav = env.onChange(of: self.menuBarManager.requestedNavigationDestination) { _, destination in + .withMouseTracking(self.mouseTracker) + .environmentObject(self.mouseTracker) + .onChange(of: self.menuBarManager.requestedNavigationDestination) { _, destination in self.handleMenuBarNavigation(destination) } - - return nav.onAppear { + .onAppear { self.appear = true // ... rest of onAppearNote: This would also remove the need for
AnyViewif no type erasure is required.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/Fluid/ContentView.swift
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build FluidVoice
|
#70 - fixed in this PR! Thanks for feedback as it was a critical bug! |
1 participant
Description
This PR addresses critical stability issues in the audio engine, specifically resolving crashes during consecutive recordings and freezes occurring during Bluetooth device disconnection.
Key Changes
engine.reset()with full engine recreation and addedengine.prepare()to ensure clean AudioUnit initialization and prevent format corruption.Fixes
Detailed technical implementation can be found in
docs/AV_AUDIOENGINE_REFERENCE.md.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
New Features
Bug Fixes
UI/UX
Chores
✏️ Tip: You can customize this high-level summary in your review settings.