fix(audio): preserve late frames at the recording cutoff - #259
Open
blackforestboi wants to merge 3 commits into
Open
blackforestboi wants to merge 3 commits into
blackforestboi wants to merge 3 commits into
Conversation
Recording used a short wall-clock grace period before closing its capture file, so late Core Audio callbacks could lose the final spoken frames. Finalization now uses the capture timestamp to drain and trim PCM through the stop boundary, while rejecting incomplete captures instead of transcribing a known partial file.
The capture finalizer uses the Core Audio host-time boundary to retain all PCM frames recorded before the stop event. This exposes an explicit post-stop inclusion delay in Hot Key settings for people who want a deliberate tail instead of relying on a hardcoded grace period.\n\nThe setting defaults to 0 ms, preserving the exact stop boundary by default while still allowing an intentional audio tail when configured.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe recording stop flow now uses an audio-clock boundary and configurable post-roll delay. Settings persist the delay, recording lifecycle handling awaits pending finalization, and tests cover PCM frame inclusion and exclusion around the cutoff. Recording stop-delay flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RecordingClient
participant SuperFastCaptureController
participant AudioTap
RecordingClient->>SuperFastCaptureController: finishRecording with stop delay
SuperFastCaptureController->>AudioTap: request audio-clock boundary
AudioTap->>SuperFastCaptureController: deliver AVAudioTime buffer
SuperFastCaptureController->>SuperFastCaptureController: write PCM frames through boundary
SuperFastCaptureController-->>RecordingClient: return captured, failed, or timed-out result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/SuperFastCaptureController.swift`:
- Around line 560-574: Update scheduleStopDrainTimeout so its sleep duration
includes both postRollDuration and the configured stopDrainTimeout, allowing the
post-roll period to complete before applying the existing timeout failure flow.
Preserve the cancellation check and pendingFinish handling unchanged.
- Around line 350-360: The finishRecording function in
Hex/Clients/SuperFastCaptureController.swift must accept the stop-event host
timestamp and derive targetHostTime from it instead of calling
mach_absolute_time(). Update the RecordingClient stop flow in
Hex/Clients/RecordingClient.swift to pass the timestamp captured by the stop
event source into finishRecording.
In `@HexCore/Sources/HexCore/Settings/HexSettings.swift`:
- Line 250: Update the stopDelayMilliseconds SettingsField decode closure to
clamp decoded negative persisted values to the normalized non-negative value,
matching initializer behavior. Add a migration test covering a persisted
negative stop delay and assert the decoded setting is normalized.
🪄 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: 993c87fa-9ec3-4d68-9d65-cd115b65c36c
📒 Files selected for processing (9)
.changeset/0b4a55dc.md.changeset/c1a097d5.mdHex/Clients/RecordingClient.swiftHex/Clients/SuperFastCaptureController.swiftHex/Features/Settings/HotKeySectionView.swiftHex/Features/Settings/SettingsFeature.swiftHexCore/Sources/HexCore/Settings/HexSettings.swiftHexCore/Tests/HexCoreTests/HexSettingsMigrationTests.swiftHexTests/RecordingRaceTests.swift
Comment on lines
+350
to
+360
| /// Finalizes at an audio-clock boundary rather than after a wall-clock delay. The hotkey | ||
| /// event supplies the boundary in host time; tap timestamps let us retain every PCM frame | ||
| /// through that point even when Core Audio delivers the final buffer late. | ||
| func finishRecording( | ||
| clearBuffer: Bool = true, | ||
| postRollDuration: TimeInterval = 0 | ||
| ) async -> FinishRecordingResult { | ||
| let postRollDuration = max(0, postRollDuration) | ||
| let targetHostTime = mach_absolute_time() + AVAudioTime.hostTime( | ||
| forSeconds: postRollDuration | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant code paths and surrounding context.
sed -n '320,410p' Hex/Clients/SuperFastCaptureController.swift
printf '\n====\n'
sed -n '1320,1405p' Hex/Clients/RecordingClient.swift
# Find where stop-event timestamps are produced and passed around.
printf '\n==== SEARCH stop timestamp usages ====\n'
rg -n "hostTime|mach_absolute_time|stop.*timestamp|timestamp.*stop|finishRecording\(" Hex/Clients -g '*.swift'Repository: kitlangton/Hex
Length of output: 7220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any existing stop-event timing data that could be forwarded.
rg -n "stopRequestAt|requestedAt|stop.*At|stop.*Time|hotkey|key.*time|event.*time|hostTime" Hex/Clients Hex -g '*.swift'
# Inspect the stop path around the caller and related event plumbing.
sed -n '1260,1425p' Hex/Clients/RecordingClient.swiftRepository: kitlangton/Hex
Length of output: 19589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the stop-trigger path in the transcription feature.
sed -n '250,360p' Hex/Features/Transcription/TranscriptionFeature.swift
# Look for any explicit event timestamp plumbing in the input/hotkey pipeline.
rg -n "timestamp|hostTime|mach_absolute_time|NSEvent|CGEvent|keyUp|keyDown|hotkey.*event|event\.timestamp" Hex/Clients Hex/Features -g '*.swift'Repository: kitlangton/Hex
Length of output: 9864
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the event wrapper and how hotkey actions are emitted.
sed -n '1,120p' Hex/Clients/KeyEventMonitorClient.swift
printf '\n====\n'
sed -n '120,260p' Hex/Clients/KeyEventMonitorClient.swift
printf '\n====\n'
sed -n '260,360p' Hex/Clients/KeyEventMonitorClient.swift
# Find the action wiring that consumes hotkey press/release events.
rg -n "handleHotKeyPressed|handleHotKeyReleased|keyEvent|CGEvent|timestamp|hostTime|send\\(.stopRecording\\)|send\\(.startRecording\\)" Hex/Features/Transcription Hex/Clients -g '*.swift'Repository: kitlangton/Hex
Length of output: 18657
Propagate the stop-event host timestamp into finalization.
Sampling mach_absolute_time() inside finishRecording() makes the cutoff depend on async scheduling latency instead of the actual hotkey release.
Hex/Clients/SuperFastCaptureController.swift#L350-L360: accept the stop host timestamp and derive the boundary from it.Hex/Clients/RecordingClient.swift#L1369-L1372: pass through the timestamp captured at the stop event source.
📍 Affects 2 files
Hex/Clients/SuperFastCaptureController.swift#L350-L360(this comment)Hex/Clients/RecordingClient.swift#L1369-L1372
🤖 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 350 - 360, The
finishRecording function in Hex/Clients/SuperFastCaptureController.swift must
accept the stop-event host timestamp and derive targetHostTime from it instead
of calling mach_absolute_time(). Update the RecordingClient stop flow in
Hex/Clients/RecordingClient.swift to pass the timestamp captured by the stop
event source into finishRecording.
Comment on lines
+560
to
+574
| private func scheduleStopDrainTimeout() { | ||
| stopDrainTimeoutTask?.cancel() | ||
| stopDrainTimeoutTask = Task { [weak self] in | ||
| try? await Task.sleep(for: .seconds(SuperFastCaptureConstants.stopDrainTimeout)) | ||
| guard !Task.isCancelled else { return } | ||
| self?.processingQueue.async { [weak self] in | ||
| guard let self, self.pendingFinish != nil else { return } | ||
| self.logger.error("Timed out waiting for capture engine to reach the stop audio boundary") | ||
| let failure = RecordingFailure.captureFinalizationTimedOut | ||
| if let url = self.activeRecording?.url { | ||
| FileManager.default.removeItemIfExists(at: url) | ||
| } | ||
| self.resolvePendingFinish(with: .failed(failure)) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Allow the configured post-roll to elapse before timing out.
The fixed two-second timer starts immediately, so any stop delay above 2,000 ms always fails. Treat stopDrainTimeout as additional drain slack after postRollDuration.
Proposed fix
private func scheduleStopDrainTimeout() {
stopDrainTimeoutTask?.cancel()
+ let timeout = (pendingFinish?.postRollDuration ?? 0)
+ + SuperFastCaptureConstants.stopDrainTimeout
stopDrainTimeoutTask = Task { [weak self] in
- try? await Task.sleep(for: .seconds(SuperFastCaptureConstants.stopDrainTimeout))
+ try? await Task.sleep(for: .seconds(timeout))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func scheduleStopDrainTimeout() { | |
| stopDrainTimeoutTask?.cancel() | |
| stopDrainTimeoutTask = Task { [weak self] in | |
| try? await Task.sleep(for: .seconds(SuperFastCaptureConstants.stopDrainTimeout)) | |
| guard !Task.isCancelled else { return } | |
| self?.processingQueue.async { [weak self] in | |
| guard let self, self.pendingFinish != nil else { return } | |
| self.logger.error("Timed out waiting for capture engine to reach the stop audio boundary") | |
| let failure = RecordingFailure.captureFinalizationTimedOut | |
| if let url = self.activeRecording?.url { | |
| FileManager.default.removeItemIfExists(at: url) | |
| } | |
| self.resolvePendingFinish(with: .failed(failure)) | |
| } | |
| } | |
| private func scheduleStopDrainTimeout() { | |
| stopDrainTimeoutTask?.cancel() | |
| let timeout = (pendingFinish?.postRollDuration ?? 0) | |
| SuperFastCaptureConstants.stopDrainTimeout | |
| stopDrainTimeoutTask = Task { [weak self] in | |
| try? await Task.sleep(for: .seconds(timeout)) | |
| guard !Task.isCancelled else { return } | |
| self?.processingQueue.async { [weak self] in | |
| guard let self, self.pendingFinish != nil else { return } | |
| self.logger.error("Timed out waiting for capture engine to reach the stop audio boundary") | |
| let failure = RecordingFailure.captureFinalizationTimedOut | |
| if let url = self.activeRecording?.url { | |
| FileManager.default.removeItemIfExists(at: url) | |
| } | |
| self.resolvePendingFinish(with: .failed(failure)) | |
| } | |
| } |
🤖 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 560 - 574, Update
scheduleStopDrainTimeout so its sleep duration includes both postRollDuration
and the configured stopDrainTimeout, allowing the post-roll period to complete
before applying the existing timeout failure flow. Preserve the cancellation
check and pendingFinish handling unchanged.
| } | ||
| ).eraseToAny(), | ||
| SettingsField(.minimumKeyTime, keyPath: \.minimumKeyTime, default: defaults.minimumKeyTime).eraseToAny(), | ||
| SettingsField(.stopDelayMilliseconds, keyPath: \.stopDelayMilliseconds, default: defaults.stopDelayMilliseconds).eraseToAny(), |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Normalize decoded stop delays too.
The schema decoder overwrites the initializer’s normalized value, so persisted negative values remain negative. Clamp in the field’s decode closure and add a negative-value migration test.
Proposed fix
- SettingsField(.stopDelayMilliseconds, keyPath: \.stopDelayMilliseconds, default: defaults.stopDelayMilliseconds).eraseToAny(),
+ SettingsField(
+ .stopDelayMilliseconds,
+ keyPath: \.stopDelayMilliseconds,
+ default: defaults.stopDelayMilliseconds,
+ decode: { container, key, defaultValue in
+ max(0, try container.decodeIfPresent(Int.self, forKey: key) ?? defaultValue)
+ }
+ ).eraseToAny(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| SettingsField(.stopDelayMilliseconds, keyPath: \.stopDelayMilliseconds, default: defaults.stopDelayMilliseconds).eraseToAny(), | |
| SettingsField( | |
| .stopDelayMilliseconds, | |
| keyPath: \.stopDelayMilliseconds, | |
| default: defaults.stopDelayMilliseconds, | |
| decode: { container, key, defaultValue in | |
| max(0, try container.decodeIfPresent(Int.self, forKey: key) ?? defaultValue) | |
| } | |
| ).eraseToAny(), |
🤖 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 `@HexCore/Sources/HexCore/Settings/HexSettings.swift` at line 250, Update the
stopDelayMilliseconds SettingsField decode closure to clamp decoded negative
persisted values to the normalized non-negative value, matching initializer
behavior. Add a migration test covering a persisted negative stop delay and
assert the decoded setting is normalized.
Keep the numeric field unlabeled so its placeholder cannot wrap in the trailing value column. Add a timer icon to the setting label while preserving an accessibility label for the value field.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
0, so recording stops exactly at the hotkey boundary unless a user deliberately requests a tail.Problem
The recorder previously closed its file after a fixed wall-clock delay. On some hardware, a Core Audio callback containing audio captured before the hotkey release can arrive after that delay, clipping the final word. Waiting longer is not a sound fix: it introduces an arbitrary tail and is still device-dependent.
This change uses the capture clock itself as the cutoff. The recorder waits until a buffer reaches that boundary, keeps only the included frames, and then finalizes. The optional setting controls only an intentional post-stop inclusion delay.
Validation
xcodebuild -scheme Hex -configuration Debug -skipMacroValidation -skipPackagePluginValidation CODE_SIGNING_ALLOWED=NO buildSummary by CodeRabbit