Conversation
|
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 (8)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds a configurable "reduce volume" recording mode with playback-level and fade-in/out settings, implements session-aware ramping and manual-adjust detection in the recording client, updates UI and settings schema, adjusts transcription hotkey flow, and adds tests, localization, and a changeset. ChangesRecording Volume Ducking with Fade Controls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
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)
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 |
e2868a1 to
28603f6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 1003-1013: The guard in adoptCurrentVolumeForRecording currently
requires volumeFadeTask != nil which prevents re-arming monitoring when a new
session inherits an already-ducked volume; change the logic to only require
recordingSessionID == sessionID and previousVolume != nil (remove the
volumeFadeTask != nil check), and then if volumeFadeTask exists cancel it and
nil it, call advanceVolumeControlGeneration(), set lastAppliedRecordingVolume =
getSystemVolume(), and call startRecordingVolumeMonitor(sessionID:generation:)
so the volumeMonitorTask is restarted; reference adoptCurrentVolumeForRecording,
volumeFadeTask, previousVolume, advanceVolumeControlGeneration,
lastAppliedRecordingVolume, getSystemVolume, and startRecordingVolumeMonitor.
- Around line 380-383: previousVolume is being restored against
getDefaultOutputDevice() instead of the actual device that was ducked, so if the
user switches outputs mid-recording the wrong device gets the restored level;
add a stored device identifier (e.g., previousDuckedDeviceID or reuse a clearly
named property alongside previousVolume and lastAppliedRecordingVolume) when you
duck the volume in the code path that calls getDefaultOutputDevice(), then use
that stored deviceID in all restore/save/manual-adjustment paths (the restore
method, any saveVolume/restoreVolume helpers, and manual-adjust handlers) to
target only the exact AudioDeviceID you changed rather than the current default
device. Ensure the new property is set when applying the duck, cleared after a
successful restore, and used wherever getDefaultOutputDevice() is currently used
to determine which device to adjust.
🪄 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: 0dfde6c5-a62a-4241-8178-10bd9f196da6
📒 Files selected for processing (8)
.changeset/e8998ed7.mdHex/Clients/RecordingClient.swiftHex/Features/Settings/GeneralSectionView.swiftHex/Features/Settings/SettingsFeature.swiftHex/Features/Transcription/TranscriptionFeature.swiftHexCore/Sources/HexCore/Settings/HexSettings.swiftHexCore/Tests/HexCoreTests/HexSettingsMigrationTests.swiftLocalizable.xcstrings
| /// Tracks previous system volume when volume is changed for recording | ||
| private var previousVolume: Float? | ||
| /// Tracks the last output volume Hex intentionally applied during recording volume control. | ||
| private var lastAppliedRecordingVolume: Float? |
There was a problem hiding this comment.
Track the ducked output device with previousVolume.
previousVolume is restored through getDefaultOutputDevice() later, not the device that was actually ducked. If the user switches outputs mid-recording, Hex will restore the old level onto the new default device and never restore the original device's volume.
💡 Suggested direction
- private var previousVolume: Float?
+ private var previousVolume: (deviceID: AudioDeviceID, volume: Float)?Use that stored deviceID in the save/restore/manual-adjustment paths so restore only targets the same output that Hex changed.
🤖 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 380 - 383, previousVolume is
being restored against getDefaultOutputDevice() instead of the actual device
that was ducked, so if the user switches outputs mid-recording the wrong device
gets the restored level; add a stored device identifier (e.g.,
previousDuckedDeviceID or reuse a clearly named property alongside
previousVolume and lastAppliedRecordingVolume) when you duck the volume in the
code path that calls getDefaultOutputDevice(), then use that stored deviceID in
all restore/save/manual-adjustment paths (the restore method, any
saveVolume/restoreVolume helpers, and manual-adjust handlers) to target only the
exact AudioDeviceID you changed rather than the current default device. Ensure
the new property is set when applying the duck, cleared after a successful
restore, and used wherever getDefaultOutputDevice() is currently used to
determine which device to adjust.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2868a11d8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if wasRecording { | ||
| // Stop the recording to release microphone access | ||
| let url = await recording.stopRecording() | ||
| guard !Task.isCancelled else { return } | ||
| try? FileManager.default.removeItem(at: url) |
There was a problem hiding this comment.
In handleCancel, gating recording.stopRecording() on wasRecording can leave the recorder running if cancel arrives just after .stopRecording flips state to transcribing but before that effect actually calls recording.stopRecording(). In that window, wasRecording is false, this path skips the explicit stop, and .cancel(id: CancelID.transcription) can cancel the in-flight stop effect before it reaches the recorder, so mic capture/resources (and related media/volume cleanup) can remain active unexpectedly.
Useful? React with 👍 / 👎.
| if rampGeneration != nil { | ||
| recordingLogger.notice("Restoring system volume to \(String(format: "%.2f", volume))") | ||
| } |
There was a problem hiding this comment.
If startVolumeRamp fails during restore (for example, when output changes to a device without software volume control), this branch leaves previousVolume intact and only logs on success. Since clearMediaState() no longer resets previousVolume, later sessions can incorrectly treat that stale value as an active restore target and unexpectedly change system volume even for behaviors like .doNothing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
HexCore/Tests/HexCoreTests/HexSettingsMigrationTests.swift (1)
44-46: ⚡ Quick winUse the shared default constant in the default-value assertion.
The assertion on Line 45 should reference
HexSettings.defaultRecordingReducedVolumeinstead of0.2to avoid brittle tests when defaults change.Proposed change
func testNewSettingsDefaultRecordingReducedVolume() { - XCTAssertEqual(HexSettings().recordingReducedVolume, 0.2) + XCTAssertEqual(HexSettings().recordingReducedVolume, HexSettings.defaultRecordingReducedVolume) }🤖 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/Tests/HexCoreTests/HexSettingsMigrationTests.swift` around lines 44 - 46, Replace the hard-coded literal 0.2 in the testNewSettingsDefaultRecordingReducedVolume() assertion with the shared default constant by asserting HexSettings().recordingReducedVolume equals HexSettings.defaultRecordingReducedVolume; update the XCTAssertEqual call to reference the HexSettings.defaultRecordingReducedVolume constant instead of the magic number to keep the test resilient to default changes.
🤖 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/Features/Settings/GeneralSectionView.swift`:
- Around line 90-104: The Picker controls created with empty titles (Picker("",
selection: ...)) are not reliably labeled for accessibility; update the Picker
in GeneralSectionView (the Picker bound to
store.hexSettings.recordingAudioBehavior) to include an explicit accessibility
label like .accessibilityLabel("Audio Behavior while Recording"), and likewise
update the Picker in HistorySectionView (the Picker for maximum history entries)
to include .accessibilityLabel("Maximum History Entries") so screen readers get
the proper label.
In `@Localizable.xcstrings`:
- Around line 87-89: The new recording-volume UI strings (e.g., the key "After
recording ends" and the other recording-volume keys added in the same diff
ranges) lack German translations; add corresponding "de" entries for each new
key in Localizable.xcstrings so German users see fully localized text. Locate
the keys including "After recording ends" and the other recording-volume strings
referenced in the comment ranges (around 314-322, 506-508, 560-564) and provide
appropriate German translations under the "de" locale for each key, preserving
the existing key names and file structure.
---
Nitpick comments:
In `@HexCore/Tests/HexCoreTests/HexSettingsMigrationTests.swift`:
- Around line 44-46: Replace the hard-coded literal 0.2 in the
testNewSettingsDefaultRecordingReducedVolume() assertion with the shared default
constant by asserting HexSettings().recordingReducedVolume equals
HexSettings.defaultRecordingReducedVolume; update the XCTAssertEqual call to
reference the HexSettings.defaultRecordingReducedVolume constant instead of the
magic number to keep the test resilient to default changes.
🪄 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: 528df683-befc-4dd1-8010-ccfaba2214f5
📒 Files selected for processing (8)
.changeset/e8998ed7.mdHex/Clients/RecordingClient.swiftHex/Features/Settings/GeneralSectionView.swiftHex/Features/Settings/SettingsFeature.swiftHex/Features/Transcription/TranscriptionFeature.swiftHexCore/Sources/HexCore/Settings/HexSettings.swiftHexCore/Tests/HexCoreTests/HexSettingsMigrationTests.swiftLocalizable.xcstrings
✅ Files skipped from review due to trivial changes (1)
- .changeset/e8998ed7.md
| Picker("", selection: Binding( | ||
| get: { store.hexSettings.recordingAudioBehavior }, | ||
| set: { store.send(.setRecordingAudioBehavior($0)) } | ||
| )) { | ||
| Label("Pause Media", systemImage: "pause") | ||
| .tag(RecordingAudioBehavior.pauseMedia) | ||
| Label("Mute Volume", systemImage: "speaker.slash") | ||
| .tag(RecordingAudioBehavior.mute) | ||
| Label("Reduce Volume", systemImage: "speaker.wave.1") | ||
| .tag(RecordingAudioBehavior.reduceVolume) | ||
| Label("Do Nothing", systemImage: "hand.raised.slash") | ||
| .tag(RecordingAudioBehavior.doNothing) | ||
| } | ||
| .pickerStyle(.menu) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify empty-title pickers in SwiftUI views.
rg -nP 'Picker\(\s*""\s*,\s*selection:' --type swift -C2Repository: kitlangton/Hex
Length of output: 949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show picker context in both files
sed -n '70,120p' Hex/Features/Settings/GeneralSectionView.swift
echo "----"
sed -n '1,80p' Hex/Features/Settings/HistorySectionView.swift
# Check for nearby accessibility modifiers in these files
rg -n "accessibilityLabel|labelsHidden" Hex/Features/Settings/GeneralSectionView.swift Hex/Features/Settings/HistorySectionView.swiftRepository: kitlangton/Hex
Length of output: 3989
Add explicit accessibility labels to empty-title Picker controls
Hex/Features/Settings/GeneralSectionView.swift(Picker("", selection: ...)): nearbyText("Audio Behavior while Recording")doesn’t reliably label the picker; add an explicit accessibility label.Hex/Features/Settings/HistorySectionView.swift(Picker("", selection: ...)): same pattern forText("Maximum History Entries").
Proposed fix
- Picker("", selection: Binding(
+ Picker("Audio Behavior while Recording", selection: Binding(
get: { store.hexSettings.recordingAudioBehavior },
set: { store.send(.setRecordingAudioBehavior($0)) }
)) {
...
}
.pickerStyle(.menu)
+ .labelsHidden()
+ .accessibilityLabel("Audio Behavior while Recording")🤖 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/Settings/GeneralSectionView.swift` around lines 90 - 104, The
Picker controls created with empty titles (Picker("", selection: ...)) are not
reliably labeled for accessibility; update the Picker in GeneralSectionView (the
Picker bound to store.hexSettings.recordingAudioBehavior) to include an explicit
accessibility label like .accessibilityLabel("Audio Behavior while Recording"),
and likewise update the Picker in HistorySectionView (the Picker for maximum
history entries) to include .accessibilityLabel("Maximum History Entries") so
screen readers get the proper label.
| "After recording ends" : { | ||
|
|
||
| }, |
There was a problem hiding this comment.
Add German localizations for the new recording-volume strings.
These newly added user-facing keys currently have no de entries, so German users will see mixed-language settings UI.
Also applies to: 314-322, 506-508, 560-564
🤖 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 `@Localizable.xcstrings` around lines 87 - 89, The new recording-volume UI
strings (e.g., the key "After recording ends" and the other recording-volume
keys added in the same diff ranges) lack German translations; add corresponding
"de" entries for each new key in Localizable.xcstrings so German users see fully
localized text. Locate the keys including "After recording ends" and the other
recording-volume strings referenced in the comment ranges (around 314-322,
506-508, 560-564) and provide appropriate German translations under the "de"
locale for each key, preserving the existing key names and file structure.
28603f6 to
829a7b7
Compare
1 participant
Summary
Screenshot
Testing
cd HexCore && swift testxcodebuild -scheme Hex -configuration Debug -skipMacroValidation -skipPackagePluginValidation CODE_SIGNING_ALLOWED=NO buildSummary by CodeRabbit
New Features
Bug Fixes
Tests
Localization