fix(ai): acquire screen capture from a real user gesture, not boot()'s effect (I76) - #103
Merged
Merged
Conversation
…s effect (I76) WebView2/WKWebView reject getDisplayMedia() on every call unless it runs inside live user-gesture activation, but sampleLoop.ts's boot() called it from a useEffect with no gesture in its call stack. That raised a raw "must be called from a user gesture handler" error, misclassified as screen_capture_unavailable, and tore down the AI sidecar the loop had just started - which is also why the chat dialog would then claim AI wasn't running even though it was enabled. The three real gesture points (starting a session via the AI topic gate, toggling AI on mid-session, and the capture-retry overlay) now acquire the stream synchronously in their click and hand it to the sample loop via a small pending-stream stash, instead of the loop acquiring it later with no gesture to satisfy. onCaptureError now also carries a fatal flag so the in-session AI status chip stops lying "active" once the sidecar has actually died. Verified locally with bun (no npm/node available in this sandbox): lint, build (tsc -b + vite), test (878 tests), and format:check all pass. check-tokens/check-strings fail identically on a clean checkout (a tsx-under-bun module resolution quirk), unrelated to this change. The pre-commit hook itself can't run here since it hard-requires npm. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAI screen capture is now pre-acquired during user gestures, handed to the sample loop, and safely discarded when unused. Capture errors distinguish fatal startup failures from non-fatal tick failures, with corresponding session status updates and regression coverage. ChangesAI capture flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SessionView
participant captureScreen
participant sampleLoop
User->>SessionView: Click session, AI enable, or retry
SessionView->>captureScreen: preacquireScreenStream()
captureScreen-->>SessionView: Pending capture promise
SessionView->>sampleLoop: Start AI sample loop
sampleLoop->>captureScreen: takePendingScreenStream()
captureScreen-->>sampleLoop: MediaStream or CaptureError
sampleLoop-->>SessionView: Capture error with fatal flag
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/ai/captureScreen.ts`:
- Around line 132-175: Replace the singleton pending-screen handoff in
preacquireScreenStream, takePendingScreenStream, and discardPendingScreenStream
with lifecycle support for every gesture-started acquisition required by a
session, preserving cleanup of unconsumed streams. In
src/features/ai/captureScreen.ts lines 132-175, ensure boot can consume all
pre-acquired display streams rather than only the first. In
src/features/ai/sampleLoop.ts lines 244-251, prevent boot-time
additional-display acquisition from falling back to direct getDisplayMedia
unless it is guaranteed to run under a new user gesture, and add regression
coverage for multi-display mode.
In `@src/features/settings/categories/AiCategory.tsx`:
- Around line 189-190: Update the enable flow around preacquireScreenStream so
capture is only preacquired when a selected model and assured startSampleLoop
consumer exist; do not stash it for an inactive-model path. Ensure abandoned
enable and disable transitions explicitly discard or stop any retained screen
stream so capture and its OS indicator cannot remain active.
In `@tests/unit/ai-sample-loop.test.ts`:
- Around line 966-971: In the fatal boot-failure test, assert
handle.__state().stopped is true immediately after verifying onCaptureError was
called with the fatal flag, before invoking handle.stop(). Keep handle.stop()
afterward only for cleanup.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1325053c-3a22-4885-9e32-b913137d7e4f
📒 Files selected for processing (10)
CHANGELOG.mdISSUES.mdsrc/features/ai/captureScreen.tssrc/features/ai/index.tssrc/features/ai/sampleLoop.tssrc/features/session/SessionView.tsxsrc/features/settings/categories/AiCategory.tsxsrc/routes/Home.tsxtests/unit/ai-capture.test.tstests/unit/ai-sample-loop.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Rust (Windows)
- GitHub Check: Rust (macOS)
- GitHub Check: Frontend
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx,css}: Frontend code must use design tokens rather than raw visual values; token compliance is enforced byscripts/check-tokens.ts.
Maintain WCAG AA contrast for every text/background pairing in both themes, provide information without relying on color alone, and respect the global reduced-motion kill switch.
Files:
src/features/ai/index.tssrc/routes/Home.tsxsrc/features/ai/captureScreen.tssrc/features/settings/categories/AiCategory.tsxsrc/features/ai/sampleLoop.tssrc/features/session/SessionView.tsx
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: New motion sites must be gated by the global reduced-motion behavior by default.
Peer wire formats and identity derivation are cross-version contracts; coordinate changes so older peers remain compatible and stored identity/data are not stranded.
The application must remain local-only with no telemetry; never instruct users to paste model files or BIP39 mnemonics into an AI service.
Do not add comments unless the rationale is non-obvious; preserve established provenance tags such asI9,F6, orPR-27when fixing traceable code.
Avoid unrelated refactors, hypothetical abstractions, and adjacent cleanup when implementing a feature or bug fix.
Files:
src/features/ai/index.tssrc/routes/Home.tsxsrc/features/ai/captureScreen.tssrc/features/settings/categories/AiCategory.tsxsrc/features/ai/sampleLoop.tssrc/features/session/SessionView.tsx
**/*.{ts,tsx,js,jsx,css,json,md,rs,toml,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Run repository formatting before committing multi-file or subagent work; Prettier formatting must pass for the whole tree.
Files:
src/features/ai/index.tssrc/routes/Home.tsxCHANGELOG.mdsrc/features/ai/captureScreen.tssrc/features/settings/categories/AiCategory.tsxtests/unit/ai-capture.test.tsISSUES.mdtests/unit/ai-sample-loop.test.tssrc/features/ai/sampleLoop.tssrc/features/session/SessionView.tsx
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Component behavior should be covered by Storybook and the axe-core gate rather than adding
*.test.tsxtests, because Vitest runs in node environment without RTL/jsdom; component-test scope requires deliberate approval.
Files:
src/routes/Home.tsxsrc/features/settings/categories/AiCategory.tsxsrc/features/session/SessionView.tsx
CHANGELOG.md
📄 CodeRabbit inference engine (CLAUDE.md)
Update
CHANGELOG.mdas part of every release.
Files:
CHANGELOG.md
tests/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Vitest for unit, integration, and AI-evaluation tests; do not assume RTL/jsdom-based component tests are available.
Files:
tests/unit/ai-capture.test.tstests/unit/ai-sample-loop.test.ts
🔇 Additional comments (9)
src/features/ai/index.ts (1)
105-107: LGTM!tests/unit/ai-capture.test.ts (1)
7-13: LGTM!Also applies to: 31-35, 341-422
src/features/ai/sampleLoop.ts (1)
56-56: LGTM!Also applies to: 353-358, 911-911, 1058-1066, 1099-1100
tests/unit/ai-sample-loop.test.ts (1)
13-26: LGTM!Also applies to: 1007-1008, 1090-1139
src/features/session/SessionView.tsx (1)
36-39: LGTM!Also applies to: 945-952, 983-998, 1357-1369
src/features/settings/categories/AiCategory.tsx (1)
29-29: LGTM!Also applies to: 211-211
src/routes/Home.tsx (1)
28-28: LGTM!Also applies to: 300-309
CHANGELOG.md (1)
21-42: LGTM!ISSUES.md (1)
13-14: LGTM!Also applies to: 88-90
Comment on lines
+132
to
+175
| let pendingScreenAcquire: Promise<MediaStream> | null = null | ||
|
|
||
| // Returns the same promise that gets stashed — a caller that cares about | ||
| // the immediate outcome (e.g. SessionView's retry, for optimistic UI) may | ||
| // await it. That does NOT consume the stash: promises support multiple | ||
| // independent subscribers, so boot()'s later `takePendingScreenStream()` | ||
| // still observes the same settlement. Callers that don't care just ignore | ||
| // the return value. | ||
| export function preacquireScreenStream(): Promise<MediaStream> { | ||
| const prior = pendingScreenAcquire | ||
| const attempt = acquireScreenStream() | ||
| pendingScreenAcquire = attempt | ||
| // Nothing may ever call takePendingScreenStream() for this attempt (e.g. | ||
| // the session never reaches boot()) — attach a no-op catch so that case | ||
| // doesn't surface as an unhandled promise rejection. Callers that do | ||
| // await the returned promise still observe the original rejection. | ||
| attempt.catch(() => {}) | ||
| // A still-unconsumed stream from an earlier call (rapid re-toggle/retry) | ||
| // must not leak — release it once it settles. | ||
| if (prior) { | ||
| void prior.then(stopStream).catch(() => {}) | ||
| } | ||
| return attempt | ||
| } | ||
|
|
||
| // Consumed by sampleLoop.ts's default screen-acquire runtime hook. Returns | ||
| // null when no gesture-context acquisition is in flight (falls back to a | ||
| // direct getDisplayMedia() call, e.g. in tests or an uncovered code path). | ||
| export function takePendingScreenStream(): Promise<MediaStream> | null { | ||
| const attempt = pendingScreenAcquire | ||
| pendingScreenAcquire = null | ||
| return attempt | ||
| } | ||
|
|
||
| // Release an unconsumed pre-acquired stream (e.g. SessionView unmounting | ||
| // before boot() ever ran) so it doesn't hold the OS recording indicator lit | ||
| // for no reason. | ||
| export function discardPendingScreenStream(): void { | ||
| const attempt = pendingScreenAcquire | ||
| pendingScreenAcquire = null | ||
| if (attempt) { | ||
| void attempt.then(stopStream).catch(() => {}) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the gesture handoff cover every display acquisition.
The stash contains one stream and is cleared on its first use. In “All displays” mode, boot later performs additional runtime.acquireScreenStream() calls; those fall through to direct getDisplayMedia() outside the click and still fail on WebView2/WKWebView.
src/features/ai/captureScreen.ts#L132-L175: replace the singleton handoff with a lifecycle that can represent every gesture-started acquisition needed for the session.src/features/ai/sampleLoop.ts#L244-L251: do not fall through to a fresh display-media call for boot-time additional displays unless it is guaranteed to run under a new user gesture; add regression coverage for multi-display mode.
📍 Affects 2 files
src/features/ai/captureScreen.ts#L132-L175(this comment)src/features/ai/sampleLoop.ts#L244-L251
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/ai/captureScreen.ts` around lines 132 - 175, Replace the
singleton pending-screen handoff in preacquireScreenStream,
takePendingScreenStream, and discardPendingScreenStream with lifecycle support
for every gesture-started acquisition required by a session, preserving cleanup
of unconsumed streams. In src/features/ai/captureScreen.ts lines 132-175, ensure
boot can consume all pre-acquired display streams rather than only the first. In
src/features/ai/sampleLoop.ts lines 244-251, prevent boot-time
additional-display acquisition from falling back to direct getDisplayMedia
unless it is guaranteed to run under a new user gesture, and add regression
coverage for multi-display mode.
Comment on lines
+189
to
+190
| if (next && sessionActive) { | ||
| void preacquireScreenStream() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not retain capture when no loop can consume it.
With no active model, SessionView never starts startSampleLoop(), so this stash is never taken. Turning AI back off also does not discard it, leaving screen capture active and its OS indicator lit. Tie preacquisition to an assured loop-start/model-selection gesture, and discard it on abandoned enable/disable paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/settings/categories/AiCategory.tsx` around lines 189 - 190,
Update the enable flow around preacquireScreenStream so capture is only
preacquired when a selected model and assured startSampleLoop consumer exist; do
not stash it for an inactive-model path. Ensure abandoned enable and disable
transitions explicitly discard or stop any retained screen stream so capture and
its OS indicator cannot remain active.
Comment on lines
+966
to
+971
| await flushMicrotasks(10) | ||
| // fatal === true: this is the boot()-time acquire, which tears the loop | ||
| // (and the just-started sidecar) down — distinct from a tick-time | ||
| // transient failure, which leaves the loop running. | ||
| expect(onCaptureError).toHaveBeenCalledWith(expect.any(CaptureError), true) | ||
| await handle.stop() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the fatal path tears the loop down.
await handle.stop() performs the teardown itself, so this test does not verify the boot failure already stopped the loop. Assert handle.__state().stopped before calling stop().
Proposed fix
expect(onCaptureError).toHaveBeenCalledWith(expect.any(CaptureError), true)
+ expect(handle.__state().stopped).toBe(true)
await handle.stop()📝 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
| await flushMicrotasks(10) | |
| // fatal === true: this is the boot()-time acquire, which tears the loop | |
| // (and the just-started sidecar) down — distinct from a tick-time | |
| // transient failure, which leaves the loop running. | |
| expect(onCaptureError).toHaveBeenCalledWith(expect.any(CaptureError), true) | |
| await handle.stop() | |
| await flushMicrotasks(10) | |
| // fatal === true: this is the boot()-time acquire, which tears the loop | |
| // (and the just-started sidecar) down — distinct from a tick-time | |
| // transient failure, which leaves the loop running. | |
| expect(onCaptureError).toHaveBeenCalledWith(expect.any(CaptureError), true) | |
| expect(handle.__state().stopped).toBe(true) | |
| await handle.stop() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/ai-sample-loop.test.ts` around lines 966 - 971, In the fatal
boot-failure test, assert handle.__state().stopped is true immediately after
verifying onCaptureError was called with the fatal flag, before invoking
handle.stop(). Keep handle.stop() afterward only for cleanup.
scotej
added a commit
that referenced
this pull request
Jul 27, 2026
Both branches claimed the same 1.8.2 CHANGELOG section and appended a row to the ISSUES.md ledger; the code does not overlap. - CHANGELOG.md: one 1.8.2 section covering both fixes, I77's bullet first (the day-one host-camera bug) then I76's, with a short intro paragraph in the style of the 1.8.0 section. - ISSUES.md: both rows kept, I76 then I77; prettier re-aligned the table columns, which is the whole-table churn in this diff. - src/features/session/SessionView.tsx: auto-merged. #103 touches the AI screen-capture gesture handoff (imports from '../ai', onCaptureError's fatal flag, the discard-on-unmount effect, handleCaptureRetry); this branch touches the camera/mic publish effect and the peer-stream binding comment. Disjoint regions, disjoint concerns. Gates on the merged tree: lint, prettier --check, vitest (882 passing), tsc -b + vite build, check-tokens, check-strings, check-contrast all green.
scotej
added a commit
that referenced
this pull request
Jul 27, 2026
fix(session): send the local stream to peers who join later (I77) A guest who joined a friend's session never received the host's camera or mic, while the host saw the guest normally. `SessionView` published the local MediaStream with a single untargeted `room.addStream(stream)`, but trystero 0.24 delivers a stream only to the peers active at that instant and queues nothing for later joiners: `addStream` -> `applyMediaOp` -> `iterate` enumerates `keys(activePeerMap)` at call time (@trystero-p2p/core room.mjs:83, :494), and peer activation (room.mjs:306-314) fires `onPeerJoin` without replaying any previously added local stream. The host is structurally guaranteed to lose that race: `hostSession()` derives a topic from 32 random bytes and joins the room before the invite is sent, so its camera opens while provably alone and the one broadcast reaches nobody, forever. The guest normally wins it, because the session peer activates over the already-open connection to that friend faster than a cold camera opens. Publishing now goes through `publishLocalStream`, which broadcasts to the currently-active peers and, in the immediately adjacent statement, subscribes `onPeerJoin` to re-send the stream targeted at each later joiner (the pattern trystero's README prescribes). Both calls live in one function so the "no await in the seam" invariant is structural: no peer is missed or served twice, and a double-add would desync trystero's FIFO pairing of stream metadata to incoming tracks. The effect cleanup unsubscribes before `stopTracks`, so a "Try again" re-acquire cannot hand a joiner a dead stream. Two comments asserted the opposite of the library's behaviour and are what preserved the bug; both are replaced with the verified semantics and room.mjs line references. The integration bus mock hard-coded the same two false beliefs, which is why CI stayed green: it now models `activePeerMap` honestly (targeted sends honoured, no join replay, no `onPeerStream` replay). `tests/unit/session-publish-stream.test.ts` pins the contract -- 2 of its 4 cases fail against the pre-fix code. Day-one defect: trystero has been pinned ^0.24.0 since the media path was introduced, so host->guest video has never worked in any shipped build. Both sides must update -- a patched host reaches an unpatched guest, but a patched guest still receives nothing from an unpatched host. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Merged with main after #103 (I76) landed: both branches claimed the same 1.8.2 CHANGELOG section and appended an ISSUES.md row. Resolved to one 1.8.2 section carrying both fixes (I77 first, then I76) and both ledger rows; the code does not overlap and SessionView.tsx auto-merged.
This was referenced Jul 27, 2026
This was referenced Aug 6, 2026
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
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
sampleLoop.ts'sboot()acquired the session's long-lived screenMediaStreamviagetDisplayMedia()from a ReactuseEffect— with no user gesture in its call stack. WebView2 (Windows) and WKWebView (macOS) require everygetDisplayMedia()call, not just the first, to run inside live transient user activation, so the call was rejected outright with "getDisplayMedia must be called from a user gesture handler."DOMExceptionname fell outsidemapDisplayMediaError's handled set, so it surfaced as a rawscreen_capture_unavailabletoast instead of the intendedscreen_capture_deniedrecovery overlay — andboot()'s existing failure path tore down the AI sidecar it had just started. That's also why the Ctrl+] chat dialog would separately claim "AI isn't running yet, turn it on in Settings → AI" even though AI was genuinely enabled.preacquireScreenStream()synchronously in that click, which stashes the in-flight acquisition forsampleLoop.ts's defaultacquireScreenStreamruntime hook to consume instead of callinggetDisplayMedia()itself outside gesture context. Unconsumed streams are released (discardPendingScreenStream) on a rapid re-toggle or onSessionViewunmount, so nothing leaks a live stream or a lit OS recording indicator.onCaptureErrornow carries afatalflag (boot()-time acquire failure vs. a recoverable tick-time one), so the in-session AI status chip only flips to "error" when AI has actually died — it previously kept reading "active" through this failure.I76inISSUES.md, with a1.8.2section added toCHANGELOG.md.Test plan
bun run build(tsc -b + vite build) — cleanbun run lint— cleanbun run test— 878/878 passing, including new regression coverage for the pending-stream stash/discard mechanism, default-runtime stash consumption, and theonCaptureErrorfatalflag on both the boot-time and tick-time call sitesbun run format:check— cleanai-capture.test.ts/ai-sample-loop.test.tsNote: this sandbox has no
npm/node(onlybun), so the pre-commit hook (which hard-requiresnpm) could not run; I verified the equivalent checks manually viabun run <script>as listed above and committed with--no-verifyper the user's explicit go-ahead.check-tokens/check-stringsfail identically on a clean, unmodified checkout in this sandbox (atsx-under-bunmodule-resolution quirk), unrelated to this change.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests