Add configurable feedback sounds (closes #107)#270
Conversation
📝 WalkthroughWalkthroughAdds persisted configuration for recording start, stop, and error sounds, Settings pickers with previews, and runtime playback helpers that replace hard-coded macOS alert sounds. ChangesRecording sound configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsView
participant AppState
participant UserDefaults
participant macOS as macOS alert sounds
SettingsView->>AppState: Select or preview a sound
AppState->>UserDefaults: Persist selected sound name
AppState->>macOS: Play configured start, stop, or error sound
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (3)
Sources/SettingsView.swift (2)
1334-1341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit accessibility label to icon-only preview buttons.
.help("Preview this sound")alone doesn't clearly convey which sound is being previewed to VoiceOver users when there are three consecutive icon-only buttons. An explicitaccessibilityLabelnaming the event (e.g. "Preview (label) sound") would improve clarity.♿️ Suggested fix
Button { appState.playAlertSound(named: selection.wrappedValue) } label: { Image(systemName: "play.circle") } .buttonStyle(.plain) .foregroundStyle(.secondary) .help("Preview this sound") + .accessibilityLabel("Preview \(label) sound")🤖 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 `@Sources/SettingsView.swift` around lines 1334 - 1341, Add an explicit accessibility label to the icon-only preview Button in the sound settings section, using the associated event name (for example, “Preview \(label) sound”) so VoiceOver identifies which sound will play. Keep the existing help text and button behavior unchanged.
1316-1343: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPer-row preview buttons don't surface the muted/low-volume hint.
The top-level "Preview" button (Lines 1271-1278) checks
SystemAudioStatus.isDefaultOutputMuted()/defaultOutputVolume()and shows a hint when audio is muted or very low. The per-sound preview buttons added insoundPickerRowskip this check entirely, so a user previewing the stop/error sound via its dedicated button gets no explanation if nothing is audible.♻️ Suggested fix: reuse the muted-hint check for row previews
private func soundPickerRow(_ label: String, selection: Binding<String>) -> some View { var options = Self.systemSoundNames if !options.contains(selection.wrappedValue) { options.append(selection.wrappedValue) } return HStack(spacing: 8) { Text(label) .font(.caption) Spacer() Picker("", selection: selection) { ForEach(options, id: \.self) { name in Text(name).tag(name) } } .labelsHidden() .frame(width: 130) Button { + let muted = SystemAudioStatus.isDefaultOutputMuted() + let volume = SystemAudioStatus.defaultOutputVolume() + withAnimation(.easeInOut(duration: 0.2)) { + showMutedHint = muted || (volume ?? 1) < 0.10 + } appState.playAlertSound(named: selection.wrappedValue) } label: { Image(systemName: "play.circle") } .buttonStyle(.plain) .foregroundStyle(.secondary) .help("Preview this sound") } }🤖 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 `@Sources/SettingsView.swift` around lines 1316 - 1343, Update soundPickerRow to perform the same muted/low-volume check used by the top-level Preview action before or alongside appState.playAlertSound(named:). Reuse the existing SystemAudioStatus helpers and hint presentation mechanism so each per-sound preview button surfaces the muted/low-volume explanation consistently.Sources/AppState.swift (1)
2442-2445: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared, typed catalog instead of raw
Stringsound names.
startSoundName/stopSoundName/errorSoundNameare plain strings validated only inSettingsView's picker catalog. A typo or externally-setdefaults writevalue silently no-ops inNSSound(named:)with no fallback/validation here. An enum (or a shared constant list) for the 14 built-in sounds, referenced by bothAppStateandSettingsView, would give compile-time safety and a single source of truth.🤖 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 `@Sources/AppState.swift` around lines 2442 - 2445, Replace the raw string sound-name properties used by playStartSound(), playStopSound(), and playErrorSound() with a shared typed catalog (such as an enum) containing all 14 built-in sounds; update SettingsView’s picker to use that same catalog and map persisted or externally supplied values through validated fallback logic before calling playAlertSound, ensuring NSSound receives only known sound identifiers.
🤖 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.
Nitpick comments:
In `@Sources/AppState.swift`:
- Around line 2442-2445: Replace the raw string sound-name properties used by
playStartSound(), playStopSound(), and playErrorSound() with a shared typed
catalog (such as an enum) containing all 14 built-in sounds; update
SettingsView’s picker to use that same catalog and map persisted or externally
supplied values through validated fallback logic before calling playAlertSound,
ensuring NSSound receives only known sound identifiers.
In `@Sources/SettingsView.swift`:
- Around line 1334-1341: Add an explicit accessibility label to the icon-only
preview Button in the sound settings section, using the associated event name
(for example, “Preview \(label) sound”) so VoiceOver identifies which sound will
play. Keep the existing help text and button behavior unchanged.
- Around line 1316-1343: Update soundPickerRow to perform the same
muted/low-volume check used by the top-level Preview action before or alongside
appState.playAlertSound(named:). Reuse the existing SystemAudioStatus helpers
and hint presentation mechanism so each per-sound preview button surfaces the
muted/low-volume explanation consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e253d5fc-3dc3-459b-9048-85c82a20e95b
📒 Files selected for processing (3)
CHANGELOG.mdSources/AppState.swiftSources/SettingsView.swift
1 participant
Split out from #254. This PR carries only the feedback-sounds change — the menu-bar redesign and settings-sidebar redesign that shared the original combined commit are now separate PRs.
The hardcoded Tink/Pop/Basso start/stop/error sounds are now per-event settings with pickers and preview buttons covering all 14 built-in macOS alert sounds. Defaults match the old behavior.
Closes #107
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation