Skip to content

fix(web): keep known preview tabs while the session list refreshes - #14955

Open
saphid wants to merge 3 commits into
pingdotgg:mainfrom
saphid:upstream-preview-session-sync
Open

saphid wants to merge 3 commits into
pingdotgg:mainfrom
saphid:upstream-preview-session-sync

Conversation

@saphid

@saphid saphid commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Head fde221d4c1 on main 11fd54fcbb, merged with a merge commit. The desktop recordings below were taken at the earlier head ded51886d3; see Verification.

Problem

An agent can open a Browser tab in the background (preview_open with open: false) while the thread's Browser panel is closed. If a preview event from another thread in the same environment arrives after that, reopening the first thread's Browser panel hides the background tab until a fresh session list comes back. The tab's desktop state (zoom, back/forward, viewport) is dropped as well. Locally the tab is missing for about 120 ms; on a slow remote link it is missing for the whole list round trip. When a list request is already in flight, the remounting panel also sends a second one.

Why this qualifies

This is a small, focused correctness fix under CONTRIBUTING: a session the client already knows about should not be discarded because of a response that predates it. There is no linked issue and no prior maintainer agreement. Tab lifetime, defaults and contracts are unchanged.

Change

When the panel mounts, usePreviewSession refreshes the cached preview.list and, on main, immediately reconciles the cached result that is replayed while the refresh is waiting. That cached list predates the background tab, so the tab is removed. On main this is usually hidden: in the same microtask the hook replays the environment's latest preview event, which re-adds the tab if that event belongs to this thread. When the latest event belongs to another thread, nothing re-adds the tab until the fresh list arrives.

With this change the list is evaluated once on mount, and an in-flight request is reused instead of being refreshed again. Only completed, non-waiting successful responses are reconciled. A stale cached result replayed during a refresh can no longer remove known tabs or their desktop overlay state. A completed, authoritative list can still remove a tab that was really closed.

Main has since added a retry: when an event overtakes the first list, the hook asks for the list again instead of treating the partial index as loaded. That retry still runs alongside this change; its waiting state isn't reconciled, and the fresh answer completes it.

Verification

At head fde221d4c1 after merging main 11fd54fcbb (Linux, 2026-10-14):

  • vp test run apps/web/src/components/preview/previewSessionSync.test.ts apps/web/src/previewStateStore.test.ts: 2 files, 31 passed. After the merge, the test imports effect/reactivity (main moved it). The overtaken-list test now also checks that main's retry fires and completes; that assertion fails if the retry is removed.
  • vp run typecheck in apps/web: exit 0. vp lint on both changed files: exit 0.
  • Independent review of the merge and test changes: GPT-6.1 Sol (high) in Codex via T3 Code. No findings.

Earlier evidence at ded51886d3 follows. Environment: macOS 26.5.2 arm64, packaged desktop app on isolated state with a loopback fixture page. Verified on upstream main 8fb068c498 (before) and this branch rebased onto it (after).

Reproduction (one project, threads A and B):

  1. In A, open the Browser panel, then close it.
  2. With A's panel closed, call the real MCP preview_open for A with { url, open: false, reuseExistingTab: false }.
  3. Switch to B, open its Browser and navigate to a page through the address bar, so B's events are the environment's latest preview events. Close B's panel.
  4. Switch back to A and open its Browser panel.

A read-only per-frame DOM trace of the panel's tab labels gives these results:

Run Before (main) After (this PR)
Natural timing Background tab missing for 4 frames (117 ms) after reopening, then restored by the fresh list Present on all 857 reopened frames
preview.list held 3 s on the server Missing for 424 frames (3.5 s), until the response arrived Present on all 850 reopened frames

The second run simulates a slow remote link. It pauses the server's real preview.list handler for 3 s through the owned backend's inspector, and the same pause was applied to both builds. No client code, responses or stores were mocked.

Early reopen frame with the delayed list. Before: only the original Browser tab. After: the background tab is kept (URL label until its title refreshes).

Before (main) After (this PR)

Full timeline with the delayed list:

Before (main) After (this PR)

Videos: before, delayed · after, delayed · before, natural timing · after, natural timing

If the latest environment event is the background tab's own event (the single-thread case), main re-adds the tab before paint, and both builds look the same. That is why a single-thread reproduction does not show the bug.

The regression tests run the real hook, atom registry and preview store; only the transport is substituted. Against main's unchanged usePreviewSession.ts, vp test run apps/web/src/components/preview/previewSessionSync.test.ts apps/web/src/previewStateStore.test.ts gives 2 failed and 29 passed (exit 1):

  • preserves a local tab while cached absence is waiting for a fresh response: main issues a duplicate list request while one is already in flight (expected 0 requests, got 1).
  • refreshes a completed cache without treating its old absence as authoritative: main drops the known tab when the cached list is replayed (expected local, got null).

At this head, the same command passes 31 of 31 (exit 0).

The following ran at this head, each with exit 0:

  • vp run --filter @t3tools/web typecheck
  • vp lint --report-unused-disable-directives apps/web/src/components/preview/previewSessionSync.test.ts apps/web/src/components/preview/usePreviewSession.ts
  • vp fmt --check apps/web/src/components/preview/previewSessionSync.test.ts apps/web/src/components/preview/usePreviewSession.ts
  • vp test run apps/web/src/components/preview: 174 of 174 passed
  • vp run build:desktop
  • git diff --check

Surfaces

  • Desktop: affected; its renderer lives in apps/web, and that is the client exercised here.
  • Standalone and hosted web: affected since feat(preview): run the browser on the environment server #15328 runs the Browser on the environment server, using this same hook in ChatView and PreviewView. The fix applies there unchanged, but it was not captured on web.
  • Mobile: not affected; the native app has no Browser panel and does not use this hook.
  • Local, remote/relay and tunnel: affected when the desktop app is connected to that environment. The tab is missing for one list round trip, so higher latency makes the gap longer. The fix does not depend on the transport. Only local was exercised; remote latency was simulated by delaying the server response.
  • Providers (Codex, Claude, Cursor, Grok, OpenCode, Antigravity): not affected. Preview sessions do not depend on the provider, and any provider that uses T3's MCP preview_open takes the same path.

Not checked

A real remote/relay or tunnel connection was not exercised, and the server-hosted web Browser wasn't captured. The loss of desktop overlay state for the background tab is covered by the regression test and was not shown separately in a real client.

CodeRabbit's docstring-coverage warning is not adopted. The hook and the new nearby comment explain lazy-list evaluation and authoritative responses; adding boilerplate to meet a percentage would conflict with the repository's documentation guidance.

Implementation: Claude Code (exact Claude model not recorded). Earlier review and desktop capture: GPT-6.1 Sol, Claude Opus 5.5 and GPT-6 Astra via T3 Code. Merge with main, test update and body refresh: Claude Opus 5.5 (Claude Code) via T3 Code; review GPT-6.1 Sol.

🤖 Generated with Claude Code

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 3, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at fde221d

Macroscope's review found this PR approvable — This is a focused preview-session synchronization bug fix that preserves known tabs during stale or in-flight list refreshes while retaining authoritative cleanup after fresh responses. The production diff is small and isolated, and the accompanying regression tests cover the changed lifecycle behavior.

You can add or adjust custom eligibility rules. Learn more.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 3, 2026
@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 3, 2026 — with ChatGPT Codex Connector
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 3, 2026 01:39

Dismissing prior approval to re-evaluate d90ee72

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: aeda93f3-db0a-4d5a-bf70-c67eded2711f
📥 Commits

Reviewing files that changed from the base of the PR and between d90ee72 and cea63ae.

📒 Files selected for processing (1)
  • apps/web/src/components/preview/previewSessionSync.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

📝 Walkthrough

The preview-session hook now evaluates the session list before subscribing and reconciles only completed successful results. Tests cover cached-session restoration, waiting and failed list results, and event-versus-list revision ordering.

Changes: Preview session synchronization

Layer / File(s) Summary
Session-list evaluation and reconciliation
apps/web/src/components/preview/usePreviewSession.ts, apps/web/src/components/preview/previewSessionSync.test.ts
The hook evaluates the list before subscribing, skips reconciliation while the result is waiting or unsuccessful, and refreshes only when the current result is neither initial nor waiting. Tests cover cached state, later list results, and revision ordering.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to cea63

This change keeps known preview tabs while the session list refreshes. No merge-blocking risk was identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to cea63

The change preserves existing environment and thread scoping while preventing stale responses from prematurely removing tabs. No introduced security issue was identified, but concurrent refresh and cancellation behavior are not fully established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated effect is confined to client preview session membership, selection, and desktop overlay retention for a scoped environment/thread. The inspected delta does not expand production entrypoint reachability or preview command privileges.

Trust Boundaries and Controls

  • observed — List requests retain environment and thread identity. Event consumption rejects other thread IDs and requests a list refresh on an epoch mismatch. Revision and epoch checks constrain state consistency; they do not establish server-side authorization.

Resilience and Maintainability Implications

  • observed — The existing finalizer marks synchronization disposed, and the queued mount callback checks that flag before acting. The synchronization atom retains its existing one-second idle lifetime; this guard alone does not prove cancellation of provider requests.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title is concise, uses the repository's conventional format, and clearly states the main fix: preserving known preview tabs during session-list refreshes.
Description check Passed The description is complete and relevant. It explains the problem, change, scope rationale, verification results, affected surfaces, limitations, recordings, screenshots, and agent usage. It also iden…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 3, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 3, 2026
@saphid
saphid force-pushed the upstream-preview-session-sync branch from cea63ae to ded5188 Compare October 4, 2026 06:55
@saphid

saphid commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Review requested

Date (UTC) Reviewer Where
2026-10-03 Julius Discord DM

Logged so this PR shows when a maintainer was asked to review it.

@saphid

saphid commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Review requested

Date (UTC) Reviewer Where
2026-10-06 Maria Discord DM

Logged so this PR shows when a maintainer was asked to review it.

saphid and others added 2 commits October 14, 2026 03:51
…st retry

Main moved Effect reactivity to effect/reactivity, so the sync test now
imports it from there. It also checks that main's retry after an event
overtakes the first list still runs, and completes, alongside the waiting
guard. The guard makes the old waiting check in that retry redundant.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 14, 2026 03:55

Dismissing prior approval to re-evaluate fde221d

@github-actions github-actions Bot added size:S 10-29 changed lines (additions + deletions). and removed size:XS 0-9 changed lines (additions + deletions). labels Oct 14, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants