fix: prevent unnecessary track restarts on unmute when using ideal device constraints #1794
Conversation
🦋 Changeset detectedLatest commit: bc2915b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting 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: 1
🤖 Fix all issues with AI agents
In `@src/room/track/LocalTrack.ts`:
- Around line 250-253: LocalVideoTrack.unmute should mirror
LocalAudioTrack.unmute by clearing the pendingDeviceChange flag when unmuting so
it doesn't remain true across cycles; update the LocalVideoTrack.unmute method
to check this.pendingDeviceChange at the start (or before any restart logic for
camera source), set this.pendingDeviceChange = false and return true (or
otherwise short-circuit the deferred device-change flow) so state is consistent
with LocalAudioTrack.unmute.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/funny-masks-strive.mdsrc/room/track/LocalAudioTrack.tssrc/room/track/LocalTrack.ts
🔇 Additional comments (4)
src/room/track/LocalTrack.ts (1)
68-69: LGTM!The new
pendingDeviceChangeflag is correctly initialized and appropriately scoped asprotectedto allow subclass access.src/room/track/LocalAudioTrack.ts (2)
77-87: Consider resettingpendingDeviceChangein afinallyblock or beforerestartTrack().If
restartTrack()throws,pendingDeviceChangewill still be reset tofalseon line 86, which is correct. However, the current placement means if an exception occurs between line 85 and 86 (unlikely but possible), the flag may not be reset.More importantly, if the restart fails and the user calls
unmute()again, the restart will be attempted again (due toreadyState === 'ended'), so this is likely fine. The current implementation is acceptable.
6-6: LGTM!Clean removal of
unwrapConstraintfrom imports now that device change detection uses thependingDeviceChangeflag instead of constraint comparison..changeset/funny-masks-strive.md (1)
1-5: LGTM!Changeset correctly documents this as a patch release with a clear description of the fix.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if (this.isMuted) { | ||
| this.pendingDeviceChange = true; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check LocalVideoTrack unmute logic for device change handling
# Search for unmute method in LocalVideoTrack
echo "=== Searching for unmute method in LocalVideoTrack ==="
ast-grep --pattern $'class LocalVideoTrack {
$$$
unmute() {
$$$
}
$$$
}'
# Also check for any pendingDeviceChange references
echo -e "\n=== Searching for pendingDeviceChange references ==="
rg -n "pendingDeviceChange" --type ts
# Search for LocalVideoTrack class definition
echo -e "\n=== Searching for LocalVideoTrack class ==="
rg -n "class LocalVideoTrack" --type ts
# Search for unmute in LocalVideoTrack context
echo -e "\n=== Searching for unmute method ==="
rg -n "unmute\(\)" --type ts -A 5 -B 2Repository: livekit/client-sdk-js
Length of output: 5452
🏁 Script executed:
#!/bin/bash
# Get full unmute implementations in both tracks
echo "=== LocalAudioTrack unmute method (full) ==="
sed -n '69,93p' src/room/track/LocalAudioTrack.ts
echo -e "\n=== LocalVideoTrack unmute method (full) ==="
sed -n '161,178p' src/room/track/LocalVideoTrack.ts
echo -e "\n=== LocalTrack device change handling context (lines 245-260) ==="
sed -n '245,260p' src/room/track/LocalTrack.tsRepository: livekit/client-sdk-js
Length of output: 1829
LocalVideoTrack should also check and reset pendingDeviceChange on unmute for consistency.
The flag is set when a device change is deferred due to muting, but only LocalAudioTrack explicitly resets it in its unmute handler. LocalVideoTrack's unmute always restarts the track (if source is Camera), so it implicitly handles the pending change—but does not reset the flag. This can cause pendingDeviceChange to remain true on subsequent mute/unmute cycles, creating inconsistent state. Consider adding the same check and reset logic to LocalVideoTrack.unmute() as found in LocalAudioTrack.unmute() (lines 81–86).
🤖 Prompt for AI Agents
In `@src/room/track/LocalTrack.ts` around lines 250 - 253, LocalVideoTrack.unmute
should mirror LocalAudioTrack.unmute by clearing the pendingDeviceChange flag
when unmuting so it doesn't remain true across cycles; update the
LocalVideoTrack.unmute method to check this.pendingDeviceChange at the start (or
before any restart logic for camera source), set this.pendingDeviceChange =
false and return true (or otherwise short-circuit the deferred device-change
flow) so state is consistent with LocalAudioTrack.unmute.
…vice constraints
When using `{ ideal: 'default' }` device constraints (the default), unmuting
would trigger unnecessary track restarts because the comparison between the
constraint value ('default') and actual device ID (e.g. 'audio') would always
fail.
This adds a `pendingDeviceChange` flag that is only set when `setDeviceId()`
is explicitly called while the track is muted. The unmute logic now checks
this flag instead of comparing constraint values against the actual device,
ensuring restarts only happen when the user actually requested a device change.
mfairley
force-pushed
the
fix/unnecessary-device-restart
branch
from
ac7b855 to
bc2915b
Compare
2 participants
When using
{ ideal: 'default' }device constraints (the default), unmutingwould trigger unnecessary track restarts because the comparison between the
constraint value ('default') and actual device ID (e.g. 'audio') would always
fail.
This adds a
pendingDeviceChangeflag that is only set whensetDeviceId()is explicitly called while the track is muted. The unmute logic now checks
this flag instead of comparing constraint values against the actual device,
ensuring restarts only happen when the user actually requested a device change.
Related issues
Test plan
setDeviceId()while muted still triggers restart on unmuteSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.