Repository navigation
feat(publish): capture="release" closes devices while muted or invisible - #7746
Conversation
`<moq-publish muted>` and `invisible` keep capture open by default, so unmuting is instant but the browser's capture indicator stays lit. The new `capture` attribute picks that per element: `hold` (default) keeps the device open, and `release` closes the microphone while muted and the camera while invisible, stopping a screen share or file once both are off. Clearing `source` or detaching still releases everything. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 minutes. View limit details
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. CodeRabbit Verify
|
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head fb9daaf.
P2: Make screen-share resumption gesture-aware. js/publish/src/element.ts:505–508 now disables the screen source when both tracks are off, ending its share. If an application later clears muted or invisible programmatically without a user gesture, Source.Screen attempts getDisplayMedia again, which requires user activation; it catches the failure and leaves the source empty, with no retry while the gate remains enabled. The advertised automatic resumption therefore silently fails. Please provide a user-gesture restart path and surface the need to reselect the screen, and document the browser prompt requirement; add a test for release followed by a non-gesture resume.
Verification: GitHub-only static review; I did not run browser tests. The PR reports just ci and three local Chromium tests passing.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No blocking issues. Two small test-coverage gaps.
Reviewed changes
captureattribute andCaptureMode—parseCapture, thecontrols.capturesignal, attribute observation/reflection, and the getter/setter, withapi/element.api.mdregenerated.- Per-track capture gates — new
#videoCapture/#audioCapture/#eitherCapturesignals split capture from encoding:Camera/Microphonegate on their own track underrelease,Screen/Fileon either, andAudio.Capturemoves to#audioCapture. - Docs — attribute tables in
doc/lib/js/publish.mdandjs/publish/README.md, plus theMIGRATE.mdentry pointing atcapture="release". - Test — new
publish-ui.browser.tscase for a camera releasing its devices while muted/hidden.
I traced the wiring: hold reduces to the old #enabled gate, so the default path is unchanged, and getter(signal) preserves reactivity into each source. The implementation is sound.
ℹ️ New capture attribute is not exercised by the invalid-value test table
capture refuses an invalid value exactly like source/preview/backend/announce, but it is absent from the parametrized refusal table in publish-ui.browser.ts, so the new parser's throw/suspend/recover path has no coverage.
Technical details
# Add capture to the invalid-refusal table
## Affected sites
- `test/interop/clients/js/publish-ui.browser.ts:559` — the `cases` array enumerates moq-publish `source`, `preview`, `backend`, and `announce`; `capture` is missing.
## Required outcome
- An invalid `capture` value throws from the setter, suspends the element, retains the bad markup attribute, and recovers when corrected, matching the other enumerated attributes.
## Suggested approach (optional)
- Add `["moq-publish", "capture", "sometimes", "capture", "hold"]` to `cases`.deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
| expect( | ||
| await publication.evaluate(({ publish }) => | ||
| [publish.sources.audio.peek(), publish.sources.video.peek()].map((source) => source?.in.enabled.peek()), | ||
| ), | ||
| ).toEqual([false, false]); |
There was a problem hiding this comment.
| expect( | |
| await publication.evaluate(({ publish }) => | |
| [publish.sources.audio.peek(), publish.sources.video.peek()].map((source) => source?.in.enabled.peek()), | |
| ), | |
| ).toEqual([false, false]); | |
| expect( | |
| await publication.evaluate(({ publish }) => { | |
| const audio = publish.sources.audio.peek(); | |
| const video = publish.sources.video.peek(); | |
| return { | |
| inputs: [audio?.in.enabled.peek(), video?.in.enabled.peek()], | |
| outputs: [audio?.out.source.peek(), video?.out.source.peek()], | |
| }; | |
| }), | |
| ).toEqual({ inputs: [false, false], outputs: [undefined, undefined] }); |
Problem
Since moq-dev/moq#7157,
<moq-publish muted>andinvisiblestop only the encoder and keep the microphone and camera open. That makes unmuting instant, which push-to-talk wants, but the browser's capture indicator stays lit while muted or hidden. A page whose users expect muting to turn the indicator off now has no element-level way to get that back. The only route left is to drop the element and build the pipeline from the library.Approach
Add a
captureattribute and property to<moq-publish>,"hold"(default) or"release"(js/publish/src/element.ts):holdis today's behavior: the sources and the audio capture follow only the element's connection gate.releasegates each source on its own track again, the way it worked before feat(publish)!: muted and invisible keep capture open #7157.Source.MicrophoneandAudio.Captureclose while muted, andSource.Cameracloses while invisible. ASource.ScreenorSource.Filecarries both tracks, so it stops only once both are off. Resuming reopens the device.Encoding, the announce gate and the preview blanking still key off
muted/invisible, so they behave the same under either mode. An invalid value is refused like the other enumerated attributes.Docs: the attribute tables in
js/publish/README.mdanddoc/lib/js/publish.md, plus the UnreleasedMIGRATE.mdentry, which now points atcapture="release"instead of the library.Test (
test/interop/clients/js/publish-ui.browser.ts): withcapture="release", a camera that starts muted and hidden opens neither device. Muting ends only the microphone track, and unmuting opens a new one. Hiding ends only the camera track.Impact
@moq/publish/element: newcaptureattribute/property andcontrols.capturesignal, plus the exportedCaptureModetype. The default is unchanged, so this is additive.Alternatives
releaseattribute. Shorter, but it reads like a version and leaves no room for another policy. I chose the enumeratedcapture="hold" | "release", which matchesannounceandpreview. Other names I considered:idle="hold" | "release",release-devices.capture="release-audio"). No consumer needs it yet; the library already offers it.quest/m1/push-to-talk/README.md), soholdstays the default.Checks
just ciagainstorigin/main: passed.bun test ./publish-ui.browser.tsin local Chromium (new test, the device-holding test, and the idle-controls test): 3 pass.Follow-ups
None.
Review
quest review), gpt-6.1-sol, medium effort.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)