feat: add Bluetooth HFP mic support with async readiness handling#219
Open
n33pm wants to merge 1 commit into
Open
feat: add Bluetooth HFP mic support with async readiness handling#219n33pm wants to merge 1 commit into
n33pm wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughRecording start now has an explicit "starting microphone" phase. RecordingClient.startRecording returns Bool, selects and probes preferred input (including Bluetooth HFP readiness/settle), SuperFastCaptureController adds guarded rebuild/retries and preserves interrupted recordings, UI gains .preparingMicrophone, and tests assert the two‑phase startup. Recording Startup, Engine Resilience, and UI StateRecording Startup, Engine Resilience, and UI State
sequenceDiagram
participant User as User/Hotkey
participant TF as TranscriptionFeature
participant RC as RecordingClient
participant SFC as SuperFastCaptureController
participant Device as Bluetooth HFP Device
User->>TF: startRecording
TF->>TF: set isStartingRecording = true\nprevent sleep
TF->>RC: startRecording()
RC->>Device: apply preferred/default input\nprobe transport & nominal sample rate
alt Bluetooth HFP requires readiness
RC->>Device: poll waitForHandsFreeInputIfNeeded (100ms retries)
Device-->>RC: nominal sample rate > 0 (ready)
RC->>SFC: settleHandsFreeCaptureIfNeeded()
SFC->>SFC: buildAndStartEngine / restart flow (with retries)
else immediate-ready or non-Bluetooth
RC->>SFC: start capture engine or AVRecorder fallback
end
RC-->>TF: return Bool -> recordingStarted or recordingStartFailed
TF->>TF: set isRecording / clear isStartingRecording\nmaybe defer/execute stop
TF->>User: UI updates (.preparingMicrophone → recording/idle)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Hex/Clients/RecordingClient.swift`:
- Around line 1188-1190: The current code calls applyPreferredInputDevice() and
then passes its result into waitForHandsFreeInputIfNeeded(targetDeviceID:), but
applyPreferredInputDevice() returns the post-set default (which may still be the
old device) so the HFP wait can be skipped; change the call to compute the
actual requested target (either by calling resolvePreferredInputDevice() ??
getDefaultInputDevice() and passing THAT into
waitForHandsFreeInputIfNeeded(targetDeviceID:) or by changing
applyPreferredInputDevice() to return the requested target ID instead of the
current default) so waitForHandsFreeInputIfNeeded always waits on the intended
Bluetooth target. Ensure the unique symbols mentioned
(applyPreferredInputDevice(), waitForHandsFreeInputIfNeeded(targetDeviceID:),
resolvePreferredInputDevice(), getDefaultInputDevice()) are updated accordingly.
In `@Hex/Clients/SuperFastCaptureController.swift`:
- Around line 297-307: When rebuild retries are exhausted in the catch block
after buildAndStartEngine fails (the branch guarded by `attempt < 10`), treat
this as a fatal recording interruption rather than routing through the normal
configuration-change path: clear or finalize the controller's recording state
(e.g., set `activeRecording`/equivalent to nil/false) or invoke a dedicated
fatal-interruption callback instead of calling `onEngineConfigurationChange()`,
so `RecordingClient.handleCaptureEnvironmentChange` (which checks
`captureController.isRecording`) does not defer restart and leave a recording
session active with no engine.
In `@Hex/Features/Transcription/TranscriptionFeature.swift`:
- Around line 306-314: The effect currently calls recording.startRecording()
(and then plays soundEffect.play(.startRecording) and sends .recordingStarted)
even though RecordingClient.startRecording() only returns Void and may fail;
change RecordingClient.startRecording() to report success/failure (either async
throws or return Bool) and update the effect to await the result and only on
success play the start sound and send(.recordingStarted); on failure ensure you
clear isStartingRecording and call sleepManagement.preventSleep()'s release
(i.e., undo the preventSleep assertion) so the UI/state does not flip to
isRecording when the backend never started.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5afd436f-d37f-4061-a0f5-517ab3665ed8
📒 Files selected for processing (5)
Hex/Clients/RecordingClient.swiftHex/Clients/SuperFastCaptureController.swiftHex/Features/Transcription/TranscriptionFeature.swiftHex/Features/Transcription/TranscriptionIndicatorView.swiftHexTests/RecordingRaceTests.swift
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
HexTests/RecordingRaceTests.swift (1)
11-78: ⚡ Quick winPlease add a startup-cancellation/failure regression here too.
This covers the happy-path
startRecording → recordingStartedtransition, but the new behavior also depends on.recordingStartFailedand stop/discard while startup is still pending. That is the path most likely to regress with the async HFP wait/settle logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HexTests/RecordingRaceTests.swift` around lines 11 - 78, Add a second test to cover the startup-cancellation/failure path: create a test similar to newRecordingCancelsPendingDiscardCleanup (e.g., newRecordingCancelsPendingDiscardCleanup_onStartFailure) that uses RecordingProbe and TestStore, sends .startRecording, then sends .discard while the probe is pending, but instead of allowing the probe to complete to .recordingStarted simulate a startup failure by sending .recordingStartFailed (or invoking the store's failure path) while the pending stop is outstanding; then resume the pending stop and finish the store and assert the probe counts and file state match expectations (verify startCalls and stopCalls reflect the failed startup case and that the discard cleanup was still canceled appropriately). Ensure you reference the same symbols: RecordingProbe, TestStore, .startRecording, .discard, .recordingStartFailed, and probe.resumePendingStop()/waitForPendingStop() when adding this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Hex/Clients/RecordingClient.swift`:
- Around line 906-913: The loop currently returns the first ready Bluetooth
input it finds (using getDefaultInputDevice(), isBluetoothDevice(deviceID:),
inputDeviceIsReady(deviceID:)), which lets startup pick the wrong mic; change
the readiness check to wait for the specific requested headset ID (e.g. compare
currentInput to the requested/preferred headset identifier used by your startup
flow—match the variable name used in this class, e.g. requestedHeadsetID or
preferredInputDeviceID) before returning: if currentInput is nil or not equal to
the requested ID, or not ready, continue the loop; only call
recordingLogger.notice and return when currentInput == requestedHeadsetID &&
isBluetoothDevice(...) && inputDeviceIsReady(...), and add a separate log when
another Bluetooth device is ready but not the requested one.
- Around line 898-923: The waitForHandsFreeInputIfNeeded loop currently swallows
cancellation by using try? await Task.sleep and never propagates cancellation to
callers, so change the sleep call to a throwing sleep (remove try?) so
CancellationError can propagate, and after the loop and before returning a
device ensure you check Task.isCancelled (or explicitly throw CancellationError)
so callers can abort startup; also update the downstream recording startup path
that arms the capture engine (the caller that invokes
waitForHandsFreeInputIfNeeded / the capture arming segment in startRecording) to
check Task.isCancelled (or handle thrown CancellationError) and abort/clean up
instead of proceeding to arm the engine. Ensure references to
getDefaultInputDevice(), inputDeviceIsReady(deviceID:), describeDevice(_:) and
recordingLogger remain consistent while allowing cancellation to propagate.
In `@Hex/Clients/SuperFastCaptureController.swift`:
- Around line 230-242: The configuration-change observer currently dispatches on
.main and mutates engine lifecycle state unsafely; change
registerConfigurationChangeObserver to route its handler through the same
serialized context used for lifecycle mutations (i.e., dispatch work onto the
existing processingQueue or invoke the RecordingClientLive actor/task that owns
startIfNeeded(), stop(), and beginRecording()) so
restartEngineForActiveRecording() runs serialized with those methods; ensure
mutations to engine, converter, configurationChangeObserver, and
isReconfiguringEngine happen only within that serialized context and remove
direct main-queue mutations; also replace DispatchQueue.main.asyncAfter retry
scheduling with an async/await Task.sleep-based retry executed inside the same
serialized context so ordering is preserved.
---
Nitpick comments:
In `@HexTests/RecordingRaceTests.swift`:
- Around line 11-78: Add a second test to cover the startup-cancellation/failure
path: create a test similar to newRecordingCancelsPendingDiscardCleanup (e.g.,
newRecordingCancelsPendingDiscardCleanup_onStartFailure) that uses
RecordingProbe and TestStore, sends .startRecording, then sends .discard while
the probe is pending, but instead of allowing the probe to complete to
.recordingStarted simulate a startup failure by sending .recordingStartFailed
(or invoking the store's failure path) while the pending stop is outstanding;
then resume the pending stop and finish the store and assert the probe counts
and file state match expectations (verify startCalls and stopCalls reflect the
failed startup case and that the discard cleanup was still canceled
appropriately). Ensure you reference the same symbols: RecordingProbe,
TestStore, .startRecording, .discard, .recordingStartFailed, and
probe.resumePendingStop()/waitForPendingStop() when adding this test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c31074e6-22d4-4ac2-89ba-5212e622ff4f
📒 Files selected for processing (5)
Hex/Clients/RecordingClient.swiftHex/Clients/SuperFastCaptureController.swiftHex/Features/Transcription/TranscriptionFeature.swiftHex/Features/Transcription/TranscriptionIndicatorView.swiftHexTests/RecordingRaceTests.swift
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Hex/Clients/RecordingClient.swift (1)
1201-1249:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCancellation handlers can clobber a freshly started session.
endRecordingSession()and thestopCaptureController(reason: "start-cancelled")calls in thecatchbranches do not check whethersessionIDis still the current session before mutating shared actor state. Because this methodawaits several times (waitForHandsFreeInputIfNeeded,settleHandsFreeCaptureIfNeeded, internalTask.sleep), the actor is free to service a newstartRecording()call between awaits. If the caller cancels the older effect after a newer session has been installed (recordingSessionID = newID), the cancellationcatchwill runendRecordingSession()which setsrecordingSessionID = nil, cancelsmediaControlTask, and on the second handler also callsstopCaptureController(...)— tearing down state owned by the newer session.Guard each cleanup branch with
isCurrentSession(sessionID)before mutating, mirroring the post-wait guard at Lines 1217–1220 and 1227–1230.Suggested guards
} catch is CancellationError { recordingLogger.notice("Recording start cancelled while waiting for Bluetooth HFP input") - endRecordingSession() - await resumeMediaIfNeeded() + if isCurrentSession(sessionID) { + endRecordingSession() + await resumeMediaIfNeeded() + } return false } catch { recordingLogger.error("Failed while waiting for Bluetooth HFP input: \(error.localizedDescription)") - endRecordingSession() - await resumeMediaIfNeeded() + if isCurrentSession(sessionID) { + endRecordingSession() + await resumeMediaIfNeeded() + } return false } ... } catch is CancellationError { recordingLogger.notice("Recording start cancelled before capture engine became active") - stopCaptureController(reason: "start-cancelled") - endRecordingSession() - await resumeMediaIfNeeded() + if isCurrentSession(sessionID) { + stopCaptureController(reason: "start-cancelled") + endRecordingSession() + await resumeMediaIfNeeded() + } return false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Hex/Clients/RecordingClient.swift` around lines 1201 - 1249, The cancellation/error catch blocks currently call endRecordingSession() and stopCaptureController(reason:) unconditionally which can tear down a newer session if this task was preempted; update each catch (both CancellationError and general error paths) to first check isCurrentSession(sessionID) and only perform mutations (endRecordingSession(), stopCaptureController(reason: "start-cancelled"), cancelling media/resuming media) when that check passes, otherwise just return false/exit the catch; apply the same guard pattern used after awaits (the isCurrentSession(sessionID) guards at the top of the method) to prevent clobbering newer sessions.
🧹 Nitpick comments (3)
Hex/Clients/SuperFastCaptureController.swift (1)
287-326: 💤 Low valueDetached retry
Taskis not cancellable on stop/cleanup.The retry uses an unstructured
Task { ... }withTask.sleepthat is never tracked or cancelled. Ifstop(reason:)ordeinitis invoked between attempts, the in-flight retry will still wake up 200 ms later, dispatch back ontoprocessingQueue, and re-enterrestartEngineForActiveRecordingOnProcessingQueue. The earlyguard activeRecording != nil else { return }saves correctness, but during normal app teardown this can keep the controller alive pastcleanup()and produces noisy log output. Consider holding aTaskhandle (e.g.private var pendingRebuildTask: Task<Void, Never>?) and cancelling it fromstop(reason:).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Hex/Clients/SuperFastCaptureController.swift` around lines 287 - 326, The retry Task in restartEngineForActiveRecordingOnProcessingQueue is untracked and never cancelled; add a private var pendingRebuildTask: Task<Void, Never>? and replace the unstructured Task { ... } with assigning that Task to pendingRebuildTask (use [weak self] capture, await Task.sleep, then dispatch to processingQueue and call restartEngineForActiveRecordingOnProcessingQueue). Cancel and nil out pendingRebuildTask from stop(reason:) and deinit (and also clear it on successful rebuild or when giving up) so pending retries cannot keep the controller alive or produce logs after cleanup.Hex/Features/Transcription/TranscriptionFeature.swift (1)
347-356: 💤 Low valueDrop the redundant assignment after the early-return guard.
By the time execution reaches Line 354,
state.isStartingRecordingis alreadyfalse(the guard on Line 348 returns when it istrue). The assignment is dead code — minor, but worth removing for clarity.Suggested fix
if state.isStartingRecording { state.shouldStopWhenRecordingStarts = true transcriptionFeatureLogger.notice("Deferring stop until microphone is ready") return .none } - state.isStartingRecording = false state.isRecording = false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Hex/Features/Transcription/TranscriptionFeature.swift` around lines 347 - 356, In handleStopRecording(_:), remove the redundant assignment to state.isStartingRecording after the early-return guard — the guard already returns when state.isStartingRecording is true, so the line "state.isStartingRecording = false" is dead code; keep the early-return branch that sets shouldStopWhenRecordingStarts and the remaining state updates (e.g., state.isRecording = false) but delete the redundant state.isStartingRecording = false assignment to clarify intent.Hex/Clients/RecordingClient.swift (1)
906-926: 💤 Low valueTighten the “other Bluetooth input ready” log to once per occurrence.
Line 918–923 emits a
recordingLogger.notice(...)on every 100 ms iteration as long as another Bluetooth input is ready but doesn't matchtargetDeviceID. Over the 3 s window this can produce up to 30 near-identical entries per startup. Either gate it on a state change (only log on transition) or downgrade to.debug.Suggested fix
+ var lastIgnoredInput: AudioDeviceID? for attempt in 0 ..< 30 { let currentInput = getDefaultInputDevice() if let currentInput, @@ if let currentInput, currentInput != targetDeviceID, isBluetoothDevice(deviceID: currentInput), inputDeviceIsReady(deviceID: currentInput) { - recordingLogger.notice("Ignoring ready Bluetooth input \(self.describeDevice(currentInput)); waiting for requested \(self.describeDevice(targetDeviceID))") + if lastIgnoredInput != currentInput { + recordingLogger.notice("Ignoring ready Bluetooth input \(self.describeDevice(currentInput)); waiting for requested \(self.describeDevice(targetDeviceID))") + lastIgnoredInput = currentInput + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Hex/Clients/RecordingClient.swift` around lines 906 - 926, Inside the for attempt loop that checks getDefaultInputDevice() (the 0 ..< 30 retry loop in RecordingClient.swift), avoid emitting recordingLogger.notice repeatedly for the "other Bluetooth input ready" case by either (a) adding a local boolean flag (e.g., sawOtherBluetoothReady) initialized before the loop and only call recordingLogger.notice/notice(...) when the flag transitions from false to true (and set it true afterwards), or (b) change the recordingLogger.notice call for the non-target Bluetooth-ready branch to recordingLogger.debug(...); update the branch that calls isBluetoothDevice(deviceID:) && inputDeviceIsReady(deviceID:) && currentInput != targetDeviceID to use the flag or debug level so the message is logged once per occurrence instead of every retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Hex/Clients/RecordingClient.swift`:
- Around line 1201-1249: The cancellation/error catch blocks currently call
endRecordingSession() and stopCaptureController(reason:) unconditionally which
can tear down a newer session if this task was preempted; update each catch
(both CancellationError and general error paths) to first check
isCurrentSession(sessionID) and only perform mutations (endRecordingSession(),
stopCaptureController(reason: "start-cancelled"), cancelling media/resuming
media) when that check passes, otherwise just return false/exit the catch; apply
the same guard pattern used after awaits (the isCurrentSession(sessionID) guards
at the top of the method) to prevent clobbering newer sessions.
---
Nitpick comments:
In `@Hex/Clients/RecordingClient.swift`:
- Around line 906-926: Inside the for attempt loop that checks
getDefaultInputDevice() (the 0 ..< 30 retry loop in RecordingClient.swift),
avoid emitting recordingLogger.notice repeatedly for the "other Bluetooth input
ready" case by either (a) adding a local boolean flag (e.g.,
sawOtherBluetoothReady) initialized before the loop and only call
recordingLogger.notice/notice(...) when the flag transitions from false to true
(and set it true afterwards), or (b) change the recordingLogger.notice call for
the non-target Bluetooth-ready branch to recordingLogger.debug(...); update the
branch that calls isBluetoothDevice(deviceID:) && inputDeviceIsReady(deviceID:)
&& currentInput != targetDeviceID to use the flag or debug level so the message
is logged once per occurrence instead of every retry.
In `@Hex/Clients/SuperFastCaptureController.swift`:
- Around line 287-326: The retry Task in
restartEngineForActiveRecordingOnProcessingQueue is untracked and never
cancelled; add a private var pendingRebuildTask: Task<Void, Never>? and replace
the unstructured Task { ... } with assigning that Task to pendingRebuildTask
(use [weak self] capture, await Task.sleep, then dispatch to processingQueue and
call restartEngineForActiveRecordingOnProcessingQueue). Cancel and nil out
pendingRebuildTask from stop(reason:) and deinit (and also clear it on
successful rebuild or when giving up) so pending retries cannot keep the
controller alive or produce logs after cleanup.
In `@Hex/Features/Transcription/TranscriptionFeature.swift`:
- Around line 347-356: In handleStopRecording(_:), remove the redundant
assignment to state.isStartingRecording after the early-return guard — the guard
already returns when state.isStartingRecording is true, so the line
"state.isStartingRecording = false" is dead code; keep the early-return branch
that sets shouldStopWhenRecordingStarts and the remaining state updates (e.g.,
state.isRecording = false) but delete the redundant state.isStartingRecording =
false assignment to clarify intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d0fb7f5-88af-425b-aa1f-3cd7b8f8fda5
📒 Files selected for processing (5)
Hex/Clients/RecordingClient.swiftHex/Clients/SuperFastCaptureController.swiftHex/Features/Transcription/TranscriptionFeature.swiftHex/Features/Transcription/TranscriptionIndicatorView.swiftHexTests/RecordingRaceTests.swift
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.
Fixes #218
Summary by CodeRabbit
New Features
Bug Fixes
Tests