fix: play recording sounds regardless of mute system audio setting#112
Closed
Mgtantheta wants to merge 1 commit intoamicalhq:mainfrom
Closed
fix: play recording sounds regardless of mute system audio setting#112Mgtantheta wants to merge 1 commit intoamicalhq:mainfrom
Mgtantheta wants to merge 1 commit intoamicalhq:mainfrom
Conversation
Decouple sound playback from muteSystemAudio/restoreSystemAudio by introducing a dedicated playSound RPC method. Sounds now play unconditionally on recording start/stop, fixing the regression where chimes were silenced when "Mute system audio" was disabled. Also fix AudioService to always invoke the completion handler (even on failure/interruption) and propagate playback success via a Bool flag. Closes amicalhq#105 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughIntroduces a new playSound RPC method that enables recording start/stop sound effects independent of the "Mute system audio" setting. Changes span the desktop manager, native bridge, RPC infrastructure, and Swift audio service implementation, with completion callbacks added to track sound playback success. Changes
Sequence DiagramsequenceDiagram
participant RM as Recording Manager
participant NB as Native Bridge
participant RH as RPC Handler
participant AS as Audio Service
RM->>NB: playSound("rec-start")
NB->>RH: RPC dispatch playSound
RH->>RH: Validate & decode params
RH->>AS: playSound(named: "rec-start")
AS->>AS: Load & play audio
AS-->>RH: completion(success: Bool)
RH-->>NB: Send PlaySoundResult
NB-->>RM: Result received
Note over RM,AS: Recording completes...
RM->>NB: playSound("rec-stop")
NB->>RH: RPC dispatch playSound
RH->>AS: playSound(named: "rec-stop")
AS->>AS: Load & play audio
AS-->>RH: completion(success: Bool)
RH-->>NB: Send PlaySoundResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/managers/recording-manager.ts`:
- Around line 304-309: The nativeBridge.call("playSound", { sound: "rec-start"
}) invocation currently only catches rejected promises but ignores resolved
responses where playback failed (response.success === false); update the await
calls in RecordingManager (the rec-start playback and the later playSound usage)
to inspect the returned value, and if the response exists and has success ===
false, log a warning via logger.audio.warn (including the response/error
details) and treat it as a failure path (e.g., do not proceed to mute or
continue as if sound played); ensure both call sites that invoke playSound check
the response.success flag and handle/log accordingly.
In `@packages/native-helpers/swift-helper/Sources/SwiftHelper/RpcHandler.swift`:
- Around line 227-232: The completion handler for audioService.playSound in
RpcHandler.swift currently returns early when self is nil and thereby never
responds to the RPC; modify the nil-self branch to send a failure RPC response
(e.g., call sendError or sendResult with an error) using the request.id so the
caller is notified instead of hanging. Locate the closure passed to
audioService.playSound (the block capturing [weak self]) and ensure that when
guard let self fails you call the RpcHandler's response helper
(sendError/sendResult) with a descriptive message including request.id before
returning.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/main/managers/recording-manager.tsapps/desktop/src/services/platform/native-bridge-service.tspackages/native-helpers/swift-helper/Sources/SwiftHelper/AudioService.swiftpackages/native-helpers/swift-helper/Sources/SwiftHelper/RpcHandler.swiftpackages/types/src/index.tspackages/types/src/schemas/methods/play-sound.tspackages/types/src/schemas/rpc/request.ts
Comment on lines
+304
to
+309
| // Always play rec-start sound, await completion before muting (so chime isn't cut off) | ||
| try { | ||
| await nativeBridge.call("playSound", { sound: "rec-start" }); | ||
| } catch (error) { | ||
| logger.audio.warn("Failed to play rec-start sound", { error }); | ||
| } |
There was a problem hiding this comment.
Handle playSound business failures (success: false) explicitly.
At Line [306] and Line [384], the call can resolve successfully while playback still fails (success: false). Today only rejected promises are logged, so silent playback failures are missed.
Suggested patch
- try {
- await nativeBridge.call("playSound", { sound: "rec-start" });
+ try {
+ const startSound = await nativeBridge.call("playSound", {
+ sound: "rec-start",
+ });
+ if (!startSound.success) {
+ logger.audio.warn("rec-start playback reported failure", {
+ message: startSound.message,
+ });
+ }
} catch (error) {
logger.audio.warn("Failed to play rec-start sound", { error });
}
@@
this.serviceManager
.getService("nativeBridge")
.call("playSound", { sound: "rec-stop" })
+ .then((stopSound) => {
+ if (!stopSound.success) {
+ logger.audio.warn("rec-stop playback reported failure", {
+ message: stopSound.message,
+ });
+ }
+ })
.catch((error) => {
logger.audio.warn("Failed to play rec-stop sound", { error });
});Also applies to: 381-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/managers/recording-manager.ts` around lines 304 - 309,
The nativeBridge.call("playSound", { sound: "rec-start" }) invocation currently
only catches rejected promises but ignores resolved responses where playback
failed (response.success === false); update the await calls in RecordingManager
(the rec-start playback and the later playSound usage) to inspect the returned
value, and if the response exists and has success === false, log a warning via
logger.audio.warn (including the response/error details) and treat it as a
failure path (e.g., do not proceed to mute or continue as if sound played);
ensure both call sites that invoke playSound check the response.success flag and
handle/log accordingly.
Comment on lines
+227
to
+232
| audioService.playSound(named: playSoundParams.sound) { [weak self] success in | ||
| guard let self = self else { | ||
| HelperLogger.logToStderr( | ||
| "[IOBridge] self is nil in playSound completion. ID: \(request.id)") | ||
| return | ||
| } |
There was a problem hiding this comment.
Do not drop the RPC response when self is nil in playSound completion.
At Line [228], the nil-self branch returns without sendResult/sendError, so the request can hang until timeout.
Suggested patch
- audioService.playSound(named: playSoundParams.sound) { [weak self] success in
- guard let self = self else {
- HelperLogger.logToStderr(
- "[IOBridge] self is nil in playSound completion. ID: \(request.id)")
- return
- }
- self.sendResult(
+ audioService.playSound(named: playSoundParams.sound) { success in
+ self.sendResult(
id: request.id,
result: PlaySoundResultSchema(
message: success ? "Sound playback completed" : "Sound playback failed",
success: success))
}📝 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
| audioService.playSound(named: playSoundParams.sound) { [weak self] success in | |
| guard let self = self else { | |
| HelperLogger.logToStderr( | |
| "[IOBridge] self is nil in playSound completion. ID: \(request.id)") | |
| return | |
| } | |
| audioService.playSound(named: playSoundParams.sound) { success in | |
| self.sendResult( | |
| id: request.id, | |
| result: PlaySoundResultSchema( | |
| message: success ? "Sound playback completed" : "Sound playback failed", | |
| success: success)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/native-helpers/swift-helper/Sources/SwiftHelper/RpcHandler.swift`
around lines 227 - 232, The completion handler for audioService.playSound in
RpcHandler.swift currently returns early when self is nil and thereby never
responds to the RPC; modify the nil-self branch to send a failure RPC response
(e.g., call sendError or sendResult with an error) using the request.id so the
caller is notified instead of hanging. Locate the closure passed to
audioService.playSound (the block capturing [weak self]) and ensure that when
guard let self fails you call the RpcHandler's response helper
(sendError/sendResult) with a descriptive message including request.id before
returning.
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
playSoundRPC method, decoupling sound playback frommuteSystemAudio/restoreSystemAudiorec-startis awaited before muting so the chime isn't cut off;rec-stopis fire-and-forgetAdditional fixes
AudioService: completion handler is now always called (even on playback failure/interruption), preventing RPC requests from hangingAudioService: completion handler signature changed to(Bool) -> Voidso callers receive accurate success/failure statusNativeBridge:playSoundlog level changed from INFO to DEBUG (less critical than audio routing operations)Closes #105
🤖 Generated with Claude Code
Summary by CodeRabbit