Restore audio level after it was changed by audio playing#2483
Restore audio level after it was changed by audio playing#2483PhilippeFerreiraDeSousa wants to merge 1 commit intodevfrom
Conversation
📋 PR Review Helper📱 Mobile App Build✅ Ready to test! (commit 🕶️ ASG Client Build⏳ Waiting for build... 🔀 Test Locallygh pr checkout 2483 |
📝 WalkthroughWalkthroughThis change enhances the AudioPlaybackService with volume state tracking and restoration logic for glasses media during A2DP playback. It adds methods to preserve the original volume level, prevent redundant volume bumps, and asynchronously restore volumes at various playback end conditions while allowing intentional preservation during playback transitions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 62d6eaeaf3
ℹ️ 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".
| return | ||
| } | ||
|
|
||
| this.glassesVolumeRestoreLevel = null |
There was a problem hiding this comment.
restoreGlassesMediaVolume() clears glassesVolumeRestoreLevel before awaiting CoreModule.setGlassesMediaVolume, which creates a race for back-to-back prompts: a new play() call can arrive while the restore BLE write is still in flight, see restoreLevel === null, and skip rebumping, then the delayed restore lowers volume during the new playback. In the same path, if the restore call fails transiently, the original level is discarded and cannot be retried, leaving the device at the forced floor volume.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mobile/src/services/AudioPlaybackService.ts (2)
170-181:⚠️ Potential issue | 🟠 MajorClear the failed request out of
currentPlayback.If
replace()orplay()throws after the new state is assigned, this catch path reports failure but leaves that request marked active. The next request can then "interrupt" a playback that never started and invoke its callback twice.Suggested fix
} catch (error) { const errorMessage = error instanceof Error ? error.message : "Unknown error loading audio" console.error(`AUDIO: Failed to play ${requestId}:`, errorMessage) + if (this.currentPlayback?.requestId === requestId) { + this.currentPlayback.completed = true + this.currentPlayback = null + } void this.restoreGlassesMediaVolume() onComplete(requestId, false, errorMessage, null) }Also applies to: 195-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/src/services/AudioPlaybackService.ts` around lines 170 - 181, When assigning this.currentPlayback (including requestId, startTime, completed, onComplete) immediately before calling player.replace({uri: audioUrl}) and player.play(), ensure any exception from replace() or play() clears this.currentPlayback (e.g., set it to null or undefined) before rethrowing or handling the error so the failed request is not left marked active; apply the same fix to the other similar block where currentPlayback is set before playback (the block that calls player.replace/player.play and onComplete), and ensure onComplete is only invoked for the active, successfully-started currentPlayback.
89-137: 🛠️ Refactor suggestion | 🟠 MajorAdd coverage for the restore/preserve state machine.
This change now has distinct BLE volume outcomes for normal finish, interruption with preservation, startup failure, idle failure, and release. Please add unit coverage for those branches and one end-to-end path for rapid back-to-back prompts.
As per coding guidelines, "Add unit test coverage plus end-to-end test paths for features touching pairing, BLE, or transcription".
Also applies to: 158-160, 198-198, 224-225, 245-272, 346-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/src/services/AudioPlaybackService.ts` around lines 89 - 137, Add unit tests and an end-to-end test exercising the glasses volume restore/preserve state machine around ensureGlassesMediaVolumeForA2dp and restoreGlassesMediaVolume: cover normal finish (volume bumped then restored), interruption with preservation (bumped but preserved across interruption, i.e., glassesVolumeRestoreLevel retained), startup failure (CoreModule.getGlassesMediaVolume throws), idle failure (CoreModule.setGlassesMediaVolume throws during bump), and release failure (setGlassesMediaVolume throws during restore), and add one E2E test that sends rapid back-to-back prompts to validate the full path through bump->playback->restore across quick successive plays; mock/stub CoreModule.getGlassesMediaVolume and CoreModule.setGlassesMediaVolume and assert glassesVolumeRestoreLevel transitions and proper logging/handling for each branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mobile/src/services/AudioPlaybackService.ts`:
- Around line 122-136: restoreGlassesMediaVolume() is currently fire-and-forget
and clears glassesVolumeRestoreLevel before the await, so an in-flight BLE write
can race with a new play() and drop volume mid-prompt; change the implementation
to track an in-flight restore Promise (e.g., this._glassesVolumeRestoreInFlight)
and serialize restores: do not clear this.glassesVolumeRestoreLevel until the
await succeeds, set/clear the in-flight Promise around the awaited
CoreModule.setGlassesMediaVolume call, and make play() (and any path that calls
ensureGlassesMediaVolumeForA2dp()) await or cancel/supersede the in-flight
restore before calling ensureGlassesMediaVolumeForA2dp(); apply the same pattern
to the other occurrences mentioned so a new playback either waits for the
restore to finish or atomically supersedes it to avoid mid-prompt volume drops.
---
Outside diff comments:
In `@mobile/src/services/AudioPlaybackService.ts`:
- Around line 170-181: When assigning this.currentPlayback (including requestId,
startTime, completed, onComplete) immediately before calling
player.replace({uri: audioUrl}) and player.play(), ensure any exception from
replace() or play() clears this.currentPlayback (e.g., set it to null or
undefined) before rethrowing or handling the error so the failed request is not
left marked active; apply the same fix to the other similar block where
currentPlayback is set before playback (the block that calls
player.replace/player.play and onComplete), and ensure onComplete is only
invoked for the active, successfully-started currentPlayback.
- Around line 89-137: Add unit tests and an end-to-end test exercising the
glasses volume restore/preserve state machine around
ensureGlassesMediaVolumeForA2dp and restoreGlassesMediaVolume: cover normal
finish (volume bumped then restored), interruption with preservation (bumped but
preserved across interruption, i.e., glassesVolumeRestoreLevel retained),
startup failure (CoreModule.getGlassesMediaVolume throws), idle failure
(CoreModule.setGlassesMediaVolume throws during bump), and release failure
(setGlassesMediaVolume throws during restore), and add one E2E test that sends
rapid back-to-back prompts to validate the full path through
bump->playback->restore across quick successive plays; mock/stub
CoreModule.getGlassesMediaVolume and CoreModule.setGlassesMediaVolume and assert
glassesVolumeRestoreLevel transitions and proper logging/handling for each
branch.
🪄 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: c391fde3-dcba-4798-8c67-dce2c10382de
📒 Files selected for processing (1)
mobile/src/services/AudioPlaybackService.ts
| private async restoreGlassesMediaVolume(): Promise<void> { | ||
| const restoreLevel = this.glassesVolumeRestoreLevel | ||
| if (restoreLevel === null) { | ||
| return | ||
| } | ||
|
|
||
| this.glassesVolumeRestoreLevel = null | ||
|
|
||
| try { | ||
| console.log(`AUDIO: Restoring glasses media volume to ${restoreLevel}`) | ||
| await CoreModule.setGlassesMediaVolume(restoreLevel) | ||
| } catch (e) { | ||
| const msg = e instanceof Error ? e.message : String(e) | ||
| console.warn(`AUDIO: Failed to restore glasses volume to ${restoreLevel}:`, msg) | ||
| } |
There was a problem hiding this comment.
Serialize volume restore with the next playback.
restoreGlassesMediaVolume() is started in fire-and-forget mode from every end path, while the BLE write is still async. If a completion callback immediately queues another prompt, the older restore can land after the new playback begins and drop the glasses volume mid-prompt. Clearing glassesVolumeRestoreLevel before the await also loses the only saved restore target on transient failures. Please track an in-flight restore and make the next play() await or supersede it before calling ensureGlassesMediaVolumeForA2dp().
Also applies to: 198-198, 224-225, 245-256, 263-271, 349-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/src/services/AudioPlaybackService.ts` around lines 122 - 136,
restoreGlassesMediaVolume() is currently fire-and-forget and clears
glassesVolumeRestoreLevel before the await, so an in-flight BLE write can race
with a new play() and drop volume mid-prompt; change the implementation to track
an in-flight restore Promise (e.g., this._glassesVolumeRestoreInFlight) and
serialize restores: do not clear this.glassesVolumeRestoreLevel until the await
succeeds, set/clear the in-flight Promise around the awaited
CoreModule.setGlassesMediaVolume call, and make play() (and any path that calls
ensureGlassesMediaVolumeForA2dp()) await or cancel/supersede the in-flight
restore before calling ensureGlassesMediaVolumeForA2dp(); apply the same pattern
to the other occurrences mentioned so a new playback either waits for the
restore to finish or atomically supersedes it to avoid mid-prompt volume drops.
1 participant
Behaviour verified.
Note that Mentra AI does the bubbly sound for a long time before answering, and this is still forced to be on high volume
Summary by CodeRabbit
Release Notes