Fix some more device manager nonsense. #564
Conversation
WalkthroughRefactors device selection to use requested/active device IDs, adds permission and active state tracking, consolidates device enumeration and permission handling, updates camera/microphone sources to the new API, gates device UI by broadcast.enabled flags, tweaks status indicators, and changes demo captions guidance to require explicit enablement. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Publish Element
participant Dev as Device (audio/video)
participant Nav as Navigator.mediaDevices
participant Src as Source (Mic/Camera)
Note over UI: Initialization / render
UI->>Dev: start device run (permission-aware)
Dev->>Nav: getUserMedia(kind) to request permission
Nav-->>Dev: permission granted (or denied)
Dev->>Nav: enumerateDevices()
Nav-->>Dev: device list
Dev->>Dev: derive default deviceId, set devices, set permission
Dev-->>UI: available device IDs / requested/default
alt broadcast.enabled == false
UI-->>UI: skip rendering selector
else broadcast.enabled == true
UI->>UI: render selector with value = Dev.requested
UI->>Dev: onChange -> set Dev.requested
end
Note over Src: Start capture flow
UI->>Src: start()
Src->>Dev: read Dev.requested (deviceId string)
Src->>Nav: getUserMedia({ deviceId: { exact: id } }) or no deviceId
Nav-->>Src: MediaStream
Src->>Dev: device.permission.set(true)
Src->>Dev: device.active.set(track.getSettings().deviceId)
Src-->>UI: stream ready / active track set
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/hang-demo/src/publish.html (1)
50-59: Clarify captions opt-in, removetranscribereferences
Replace the “automatic transcription” language andtranscribeattribute mention in js/hang-demo/src/publish.html (lines 57–58) with copy that reflects the new opt-incaptionsattribute:@@ js/hang-demo/src/publish.html:57-58 - They run in a WebWorker to avoid blocking the main thread so unless battery is an issue, keep the automatic transcription enabled! - But if you want to disable it, the `transcribe` attribute controls whether transcription is enabled. + They run in a WebWorker to avoid blocking the main thread. Captions are opt-in; enable them only if you want local transcription. + Toggle captions by adding/removing the `captions` attribute (or via JS: <code>publish.captions = true/false</code>).
🧹 Nitpick comments (5)
js/hang/src/publish/source/camera.ts (1)
56-64: Set permission, propagate active device — consider consistent stream cleanupLogic is good. Minor consistency: microphone clears its stream on effect cleanup via default restore, while camera explicitly restores to
undefined. Pick one behavior for both to avoid transient stale streams.If you prefer explicit clearing, mirror this in microphone:
- effect.set(this.stream, track); + effect.set(this.stream, track, undefined);js/hang/src/publish/source/microphone.ts (1)
57-66: Align stream cleanup behavior with camera to avoid stale track on rerunSet a cleanup value so the signal returns to
undefinedwhen the effect reruns/closes.- effect.set(this.stream, track); + effect.set(this.stream, track, undefined);js/hang/src/publish/element.ts (1)
341-341: Drop debug logging or guard it
console.log("rendering devices", ...)will spam in production.- console.log("rendering devices", devices); + // console.debug("rendering devices", devices);js/hang/src/publish/source/device.ts (2)
56-66: No-permission path clears caches; consider also clearing requested
#runRequestedwill recompute on#deviceschange, but explicitly clearing#requestedhere would make the state immediately consistent.this.#devices.set(undefined); this.#default.set(undefined); + this.#requested.set(undefined); return;
124-135: Method naming: enumerate() actually requests permissionOptional: rename to
requestPermission()or add JSDoc clarifying that it prompts GUM to unlock labels/deviceIds, not just enumerates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
js/hang-demo/src/publish.html(2 hunks)js/hang/src/publish/element.ts(5 hunks)js/hang/src/publish/source/camera.ts(2 hunks)js/hang/src/publish/source/device.ts(3 hunks)js/hang/src/publish/source/microphone.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
All media logic belongs in the hang layer (rs/hang and js/hang)
Files:
js/hang/src/publish/element.tsjs/hang/src/publish/source/camera.tsjs/hang/src/publish/source/microphone.tsjs/hang/src/publish/source/device.ts
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/hang/src/publish/element.tsjs/hang-demo/src/publish.htmljs/hang/src/publish/source/camera.tsjs/hang/src/publish/source/microphone.tsjs/hang/src/publish/source/device.ts
🧬 Code graph analysis (4)
js/hang/src/publish/element.ts (3)
js/hang/src/publish/source/camera.ts (1)
effect(28-66)js/hang/src/publish/source/device.ts (2)
effect(45-111)effect(113-122)js/hang/src/publish/source/microphone.ts (1)
effect(30-68)
js/hang/src/publish/source/camera.ts (2)
js/hang/src/publish/source/device.ts (2)
effect(45-111)effect(113-122)js/hang/src/publish/source/microphone.ts (1)
effect(30-68)
js/hang/src/publish/source/microphone.ts (3)
js/hang/src/publish/source/camera.ts (1)
effect(28-66)js/hang/src/publish/source/device.ts (2)
effect(45-111)effect(113-122)js/hang/src/publish/audio/index.ts (5)
effect(104-147)effect(149-162)effect(164-246)effect(248-270)AudioStreamTrack(26-30)
js/hang/src/publish/source/device.ts (3)
js/signals/src/index.ts (3)
Signal(19-83)Getter(10-13)Effect(88-447)js/hang/src/publish/source/camera.ts (1)
effect(28-66)js/hang/src/publish/source/microphone.ts (1)
effect(30-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (11)
js/hang-demo/src/publish.html (1)
25-25: Opt-in captions via attribute looks goodDefaulting to no captions and requiring
<hang-publish captions>is clear and predictable.js/hang/src/publish/source/camera.ts (1)
32-39: Switch to requested deviceId and constraints merge is correctReading
this.device.requestedand using{ exact: device }while letting explicit constraints override is sound.js/hang/src/publish/source/microphone.ts (1)
34-40: Requested device and deviceId constraint build look goodThe constraint shape matches
MediaTrackConstraintsand aligns with the new Device API.js/hang/src/publish/element.ts (5)
337-343: Gating device UI by audio.enabled is a good UX guardPrevents device queries/select UI until audio is actually in use.
353-355: Select handlers now use preferred/requested correctlyUpdating
preferredon change and reflectingrequestedintoselect.valueis correct.Also applies to: 362-365
425-427: Video device UI gated by video.enabled is consistent with audioGood consistency across sources.
440-442: Camera select wired to preferred/requested correctlyLooks good.
Also applies to: 449-452
534-537: Status color change: confirm intended UXSwitching “Video Only”/“Audio Only” to green implies “healthy/ready.” If green equates to “streaming,” this is fine; otherwise consider keeping yellow for partial.
js/hang/src/publish/source/device.ts (3)
15-17: Public API change: default/requested now strings
default: Getter<string|undefined>andrequested: Getter<string|undefined>are breaking if external consumers expectedMediaDeviceInfo. IfDeviceis a public export, document this and note migration (use.availableto map id→info).I can draft a short migration note if this is a published API.
Also applies to: 27-30
37-40: Device enumeration loop and devicechange handling look solidUsing
effect.spawnwith cancellation anddevicechange→effect.reload()is robust.
101-111: Reasonable default selection heuristicsFront/external/USB for video and default/communications for audio are practical; falling back to first item is acceptable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
js/hang/src/publish/source/device.ts (4)
37-40: Guard against SSR/test environments without navigator.This prevents ReferenceError in Node/SSR and no-op attaches when mediaDevices is missing.
Apply:
this.signals.effect((effect) => { - effect.spawn(this.#run.bind(this, effect)); - effect.event(navigator.mediaDevices, "devicechange", () => effect.reload()); + if (typeof navigator === "undefined" || !navigator.mediaDevices) return; + effect.spawn(this.#run.bind(this, effect)); + effect.event(navigator.mediaDevices, "devicechange", () => effect.reload()); });
67-70: Determinism and debug noise.
- Consider sorting devices by label for stable UX when heuristics don’t match.
- console.debug is fine; if you’re shipping minified builds, consider gating behind DEV.
Apply:
- let defaultDevice: MediaDeviceInfo | undefined; + let defaultDevice: MediaDeviceInfo | undefined; + // Optional: stable order for UI + devices.sort((a, b) => a.label.localeCompare(b.label));Also applies to: 101-111
113-121: Prefer public Getter over private field for readability.Use available instead of #devices to avoid poking private state for reads.
Apply:
- const preferred = effect.get(this.preferred); - if (preferred && effect.get(this.#devices)?.some((d) => d.deviceId === preferred)) { + const preferred = effect.get(this.preferred); + const list = effect.get(this.available) ?? []; + if (preferred && list.some((d) => d.deviceId === preferred)) { // Use the preferred device if it's in our devices list. this.#requested.set(preferred); } else { // Otherwise use the default device. this.#requested.set(effect.get(this.default)); }
124-135: Propagate failure in requestPermission and return a Promise.Set permission=false on failure and expose a Promise so callers can await UI updates if needed.
Apply:
- requestPermission() { - navigator.mediaDevices - .getUserMedia({ [this.kind]: true }) - .then((stream) => { - this.permission.set(true); - stream.getTracks().forEach((track) => { - track.stop(); - }); - }) - .catch(() => undefined); - } + async requestPermission(): Promise<void> { + try { + const stream = await navigator.mediaDevices.getUserMedia({ [this.kind]: true }); + this.permission.set(true); + stream.getTracks().forEach((track) => track.stop()); + } catch { + this.permission.set(false); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
js/hang/src/publish/element.ts(5 hunks)js/hang/src/publish/source/device.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- js/hang/src/publish/element.ts
🧰 Additional context used
📓 Path-based instructions (2)
{rs,js}/hang/**
📄 CodeRabbit inference engine (CLAUDE.md)
All media logic belongs in the hang layer (rs/hang and js/hang)
Files:
js/hang/src/publish/source/device.ts
js/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place JS/TS packages under the js/ directory
Files:
js/hang/src/publish/source/device.ts
🧬 Code graph analysis (1)
js/hang/src/publish/source/device.ts (3)
js/signals/src/index.ts (3)
Signal(19-83)Getter(10-13)Effect(88-447)js/hang/src/publish/source/camera.ts (1)
effect(28-66)js/hang/src/publish/source/microphone.ts (1)
effect(30-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check
🔇 Additional comments (5)
js/hang/src/publish/source/device.ts (5)
15-16: Switching default from object to deviceId is sensible.Reduces churn and simplifies consumers; looks consistent with downstream usage.
21-26: Active/permission signals are a good addition.These enable clean wiring from producers; no issues spotted.
27-30: Requested signal aligns with new selection flow.Clear separation between preferred/default/requested; good.
45-53: Spawn/run pattern is solid and cancel-aware.Using Promise.race(cancel) avoids stale updates; nice.
31-43: Lifecycle: ensure signals.close() is called.Callers should invoke close() when tearing down to remove devicechange listener and cancel effects.
Would you like a small helper to auto-close via AbortSignal or a DOM disconnect hook?
| // An empty deviceId means no permissions, or at the very least, no useful information. | ||
| if (devices.some((d) => d.deviceId === "")) { | ||
| console.warn(`no ${this.kind} permission`); | ||
| this.#devices.set(undefined); | ||
| this.#default.set(undefined); | ||
| return; | ||
| } | ||
|
|
||
| // Assume we have permission now. | ||
| this.permission.set(true); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Permission detection is likely wrong; check label, not deviceId, and reset permission=false.
Browsers typically hide labels (empty string) pre-permission while deviceId may still be non-empty. Also, ensure permission is explicitly set to false on this branch to avoid stale true.
Apply:
- // An empty deviceId means no permissions, or at the very least, no useful information.
- if (devices.some((d) => d.deviceId === "")) {
- console.warn(`no ${this.kind} permission`);
- this.#devices.set(undefined);
- this.#default.set(undefined);
- return;
- }
+ // No permission typically => empty labels (deviceIds may still be non-empty).
+ if (devices.length && devices.every((d) => d.label === "")) {
+ console.warn(`no ${this.kind} permission`);
+ this.permission.set(false);
+ this.#devices.set(undefined);
+ this.#default.set(undefined);
+ return;
+ }
- // Assume we have permission now.
- this.permission.set(true);
+ // Assume we have permission now.
+ this.permission.set(true);To confirm cross-browser behavior:
🌐 Web query:
What does enumerateDevices() return before permission is granted across Chrome, Firefox, and Safari in 2025? Specifically, are deviceId or label empty?
💡 Result:
Short answer
- label: empty on all major browsers (Chrome, Firefox, Safari) unless the page has an active MediaStream or persistent permission. [1][2]
- deviceId: in current browsers it must not expose stable identifiers to origins that lack permission — Chrome and Firefox return opaque/empty IDs before permission; Safari/WebKit has had bugs and regressions that in some cases expose non‑empty deviceId/label when permissions are partially denied (so behavior may be inconsistent on some Safari versions). See citations. [1][3][4]
Sources
- MDN — MediaDeviceInfo / label (labels blank unless active stream or persistent permission). [1]
- MDN — MediaDeviceInfo (deviceId description + privacy note). [1]
- Discussion / bug & community reports showing deviceId is empty before getUserMedia (fingerprinting/privacy changes). [3]
- WebKit bug report showing Safari sometimes returns deviceId/label unexpectedly when permissions are mixed/changed. [4]
If you want, I can run a quick live test snippet for your current browser (or provide code) and report the exact values returned on your machine.
Use labels for permission detection and explicitly reset permission=false
Replace the deviceId check with a label-based check and ensure permission is cleared on the “no permission” branch. For example, in js/hang/src/publish/source/device.ts (lines 56-66):
- // An empty deviceId means no permissions, or at the very least, no useful information.
- if (devices.some((d) => d.deviceId === "")) {
+ // No permission typically → all labels are blank (deviceIds may still be opaque or non-empty in some browsers).
+ if (devices.length && devices.every((d) => d.label === "")) {
console.warn(`no ${this.kind} permission`);
- this.#devices.set(undefined);
- this.#default.set(undefined);
+ this.permission.set(false);
+ this.#devices.set(undefined);
+ this.#default.set(undefined);
return;
}
- // Assume we have permission now.
- this.permission.set(true);
+ // Assume we have permission now.
+ this.permission.set(true);This covers Chrome/Firefox behavior (labels are blank pre-grant) and avoids stale true values.
📝 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.
| // An empty deviceId means no permissions, or at the very least, no useful information. | |
| if (devices.some((d) => d.deviceId === "")) { | |
| console.warn(`no ${this.kind} permission`); | |
| this.#devices.set(undefined); | |
| this.#default.set(undefined); | |
| return; | |
| } | |
| // Assume we have permission now. | |
| this.permission.set(true); | |
| // No permission typically → all labels are blank (deviceIds may still be opaque or non-empty in some browsers). | |
| if (devices.length && devices.every((d) => d.label === "")) { | |
| console.warn(`no ${this.kind} permission`); | |
| this.permission.set(false); | |
| this.#devices.set(undefined); | |
| this.#default.set(undefined); | |
| return; | |
| } | |
| // Assume we have permission now. | |
| this.permission.set(true); |
🤖 Prompt for AI Agents
In js/hang/src/publish/source/device.ts around lines 56 to 66, replace the
current deviceId-based permission detection with a label-based check (use
devices.some(d => d.label === "")) because labels are blank pre-grant in some
browsers, and in the "no permission" branch explicitly clear the permission flag
by calling this.permission.set(false) (alongside this.#devices.set(undefined)
and this.#default.set(undefined)); only set this.permission.set(true) after the
label check confirms permission granted (i.e., in the else branch).
Summary by CodeRabbit
Bug Fixes
Style
Documentation