Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. Walkthrough
Merge Risk:
|
6357cd8 to
91a7516
Compare
|
Note Written by Verified in a real dev environmentverified as-is at reproduced on because the changed audio signal is desktop-only, also ran the real linux electron client and backend against the dev renderer. used actual looping wav playback in its browser guest, with no injected preview-store state. only one development backend ran at a time. electron used after waiting for the browser surface to initialize, clicked play and pause in the rendered panel through playwright input. an initial automation attempt submitted the address before the new tab finished mounting; navigating the settled tab resolved that setup issue. observed through the running client and electron's native audio state: checked light at 1440x900 and dark at 1024x768. the captures use the same app-created project, threads, and messages; elapsed labels differ. the recording shows click, mute/unmute, pause/resume, and keyboard activation. inspected the captures and played and sought the uploaded recording. blacksmith passed Not verifiedmacos, windows, native ios/android, remote or multi-environment audio, external streaming sites, and performance at scale were not exercised on this linux vm. plain web has no electron audio signal, and mobile has no browser panel; their absence of this indicator is intentional. chrome verification covered the real client/provider flow, not desktop audio detection. |
91a7516 to
05500a8
Compare
…debar Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
05500a8 to
3c7ad11
Compare
A tab that goes headless unmounts its guest, but its last desktop overlay stays in the store, so the speaker stayed on after playback was gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The headless filter missed other ways a guest unmounts, such as losing preview permission. Clearing the overlay when the bridge tears down covers every teardown, so the selector no longer needs its own check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note 🤖 opus-5.5 responding on behalf of Ryan. Review summary at [P2] Finding: The sidebar speaker could stay on for a browser tab whose guest was gone.
|



Note
opus-5.5 responding on behalf of Ryan.
Problem
When a thread's browser tab plays audio and you switch to another thread, nothing in the sidebar shows which thread is making the sound. The speaker glyph only appears on the browser tab inside the panel, so you can't see it until you're already on that thread. With a few threads open you have to click through them one by one. Ideas discussion: #20205.
Change
Thread rows now show a speaker button (lucide
Volume2, the icon the browser tab already uses) while any of the thread's browser tabs plays unmuted audio. It sits next to the existing "terminal process running" button and works the same way: clicking it opens the thread with the playing tab active. It disappears when the audio stops, the tab is muted, or the tab closes.The audible state already reaches
previewStateStorefor every thread's live browser tab, from Electron'saudio-state-changedthroughusePreviewBridge.ElectronBrowserHostmounts every session, not only the active thread's. Each row subscribes to its own thread's preview atom through a selector that returns the tab id ornull, so unrelated preview updates don't re-render rows.usePreviewBridgenow clears a tab's desktop state when its webview host unmounts. Before, the last audible state stayed in the store after the guest was gone, for example when a server tab went headless or the environment lost preview permission, so the speaker could stay on for a tab that no longer played.Scope and approval
This is a new indicator, so under prior approval it needs a maintainer to approve the direction in #20205. That approval isn't there yet. I'm opening this so that, if you approve #20205, this PR can be reviewed and merged without another round-trip.
It replaces #19116 by @Nyxalium, which was closed only because approval was missing. That PR's problem statement and the #20205 discussion are theirs. This version is rebuilt on current
main:sidebar/SidebarThreadRow.tsx.main, rather than a new component test file.Six files: the selector and its test, the button, the row wiring, and the bridge teardown and its test. Playback, muting, defaults, and settings are unchanged.
Other surfaces: browser tabs with audio state exist only in the desktop app. Web without Electron has no tab audio state. Mobile shows streamed browser previews but gets no Electron audio state, so neither shows the indicator. Rows from other environments work because each thread is looked up by its environment-scoped thread ref.
Verification
Current head
ed8856facd, real desktop app. Rebased ontomainat4b87a45770and rebuilt withvp run build:desktop. I ran the samedrive.mjson Linux in a headless Xvfb box against thescripts/fixtures/desktop-launchdatabase:Playing, with "Second thread" open:
After clicking the speaker:
The runs below were on the earlier patch, before the teardown change.
Real desktop app, before and after. I built the desktop app (
vp run build:desktop) formainat6d46336and for this branch. On Linux, each build was launched with Playwright_electronunder Xvfb against the repo'sscripts/fixtures/desktop-launchdatabase, which has one project and two threads. The script (drive.mjs):<audio>webContents.executeJavaScript(..., true)and confirmsisCurrentlyAudible() === trueBefore (
main, audio playing in "Release launch check" while "Second thread" is open):After, playing:
After, hover:
After, paused:
After clicking the speaker:
Crowded row. The speaker shares the meta line with the branch, the terminal-running button, and the PR badge, so I checked the busiest case. crowded.mjs seeds the thread with a long branch (
docs/app-vs-prototype-comparison) and a linked PR, then runssleep 900in its terminal and plays audio. The icons fit on one line at the default sidebar width. The branch name middle-truncates about four more characters ("docs…comparison" becomes "d…comparison"), and nothing wraps or overlaps.Playing, with terminal and PR:
Paused:
Windows. I ran the same two scripts on Windows 11 Pro 25H2 (build 26200), building with Node 26.12.1 and pnpm 12.14 through
vp, and got the same results as on Linux. While playing, 1 row has the speaker, on the playing thread. After pause, no row has it. After playing again, the speaker returns, and clicking it movesaria-currentto the playing thread and shows its browser tab. In the crowded row, the meta line stays on one line with the speaker between the branch and the PR number.Windows, playing, with "Second thread" open:
Windows, paused:
Windows, after clicking the speaker:
Windows, crowded row:
Focused tests.
apps/web/src/previewStateStore.test.tsgains "finds the background tab playing unmuted audio". It covers a silent active tab next to a playing background tab, muting, and closing the tab. The newusePreviewBridge.browser.test.tsxmounts the real hook against a stub bridge, reports an audible state, unmounts, and checks the state is gone. It fails on the earlier head.Checks. At
ed8856facd,vp run check:changedpassed: format, lint, knip 1/2 and 2/2, typecheck (2 packages), tests beside the changes, and the React compiler check (4 files). The preview, browser, right panel, and sidebar tests passed 309/309.Unverified
Headless and permission-loss teardown in the real app. The browser test covers the hook's teardown. I didn't drive a headless fallback or a permission change in Electron.
macOS. I ran Linux and Windows only. The audible signal is Electron's cross-platform
audio-state-changed, which the existing tab glyph already relies on.A real video site. Playback came from a local
<audio>tone. Chromium raises the same audible flag for<video>, and the tab glyph already shows it on sites like the lesson video that prompted this.Performance at scale. I didn't profile a large sidebar. Each row adds one atom subscription with a primitive selector, the same pattern fix(web): identify noisy preview threads in the sidebar #19116 measured: 1,000 rows mount 4.15 → 6.62 ms, with zero redundant commits.
Decisions
Volume2, muted foreground,size-3.5. Runner-up: a pulsing icon like the terminal button.Volume2is already the browser tab's audible glyph, and a constant pulse next to playing media felt noisy.Reviewed with sol-loop: 1 round with gpt-6.1-sol. Sol verdict: approved. It confirmed that
openBrowsergets the server tab id and that rows don't re-render on unrelated preview updates.Council after the rebase: gpt-6.1-sol and gpt-6-astra each found the stale-overlay P2 independently, first for headless tabs at
3c7ad11434, then for permission loss at2d9f84f5c1.ed8856facdfixes it at teardown. Both returned PASS ated8856facdwith no findings.Created with Claude Opus 5.5 in Claude Code (via T3 Code), reviewed by GPT-6.1 Sol (Codex).
🤖 Generated with Claude Code