Skip to content

fix(preview): new tabs open on the window showing the thread - #13773

Open
mkiera wants to merge 1 commit into
pingdotgg:mainfrom
mkiera:preview-tab-follows-thread
Open

mkiera wants to merge 1 commit into
pingdotgg:mainfrom
mkiera:preview-tab-follows-thread

Conversation

@mkiera

@mkiera mkiera commented Sep 26, 2026 •

Copy link
Copy Markdown

Fixes #13771.

What Changed

Each desktop window now adds the thread it is showing to its preview host focus report, as an optional activeThreadId. When no host owns the target tab, the broker ranks hosts showing the requested thread ahead of focus order. The field is optional, so older clients connect as before and rank lower.

  • PreviewAutomationHosts sits outside the router, so AppRoot reads the routed thread with useRouterState({ router }) and passes it down. Each environment's host reports it only for its own environment.
  • The report is sent again when the routed thread changes, in addition to the existing focus, blur and visibility reports. Unchanged reports are still skipped.

Why

#13064 routes a new session to the client that owns or shows the tab. When no tab exists yet, it falls back to the most recently focused window. With two machines connected, that is often the other machine, because the user just clicked into it. The tab then opens on a machine that is not showing the thread, with its own network and browser profile, and the session stays pinned there.

Validation

  • New broker test: an unfocused host showing the requested thread wins over a focused host showing another thread. It fails without the new sort key.
  • vp test run for the broker and PreviewAutomationHosts tests: 43 passed. tsc --noEmit is clean for apps/server, apps/web and packages/contracts. Targeted lint shows one existing React refs warning in PreviewAutomationHosts.tsx that this PR does not touch.
  • Live on two Windows 11 machines: before this change, a fresh preview_open went to the VM because it was the last focused window. With it, a fresh preview_open opened on the desktop showing the thread. The desktop was probably focused at that moment, so the unit test is what covers the ranking.

Surfaces: desktop only, since preview hosts run in Electron. Web and mobile are unchanged. No UI change.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes (no UI change)
  • I included a video for animation/interaction changes (n/a)

Model: Claude Opus 5.5 in Claude Code, running in T3 Code.

Summary by CodeRabbit

  • Bug Fixes
    • Preview automation now prioritizes the host associated with the active thread when choosing where to run a session. This helps ensure actions are handled by the correct preview when multiple hosts are available.
    • The active thread is kept in sync with the current route and host focus, improving host selection as users switch between threads.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 26, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 43a5eab

Macroscope's review found this PR approvable — This is a focused preview automation bug fix that adds optional thread-focus metadata and a narrowly scoped host-selection preference, while preserving existing assignments and older-client compatibility. The behavior is covered by a regression test and does not change product defaults or static-analysis configuration.

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

@coderabbitai

coderabbitai Bot commented Sep 26, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

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: 84ed7630-4ab9-4ebb-9a7b-cfbf878ac76f

📥 Commits

Reviewing files that changed from the base of the PR and between a21b42c and 43a5eab.

📒 Files selected for processing (5)
  • apps/server/src/mcp/PreviewAutomationBroker.test.ts
  • apps/server/src/mcp/PreviewAutomationBroker.ts
  • apps/web/src/AppRoot.tsx
  • apps/web/src/components/preview/PreviewAutomationHosts.tsx
  • packages/contracts/src/previewAutomation.ts

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


📝 Walkthrough

Walkthrough

Preview hosts now report their active thread ID. The broker stores that ID and uses it to prefer a host showing the requested thread when selecting a host for a new session.

Changes

Preview host routing

Layer / File(s) Summary
Report the routed thread
packages/contracts/src/previewAutomation.ts, apps/web/src/AppRoot.tsx, apps/web/src/components/preview/PreviewAutomationHosts.tsx
AppRoot passes the current route reference to preview hosts. A host reports its active thread ID when the route belongs to its environment. The focus contract adds an optional activeThreadId field.
Select hosts by active thread
apps/server/src/mcp/PreviewAutomationBroker.ts, apps/server/src/mcp/PreviewAutomationBroker.test.ts
The broker stores active thread IDs from host registration and focus updates. It ranks a matching thread ahead of focused status and focus order. A test checks routing to the host showing the requested thread.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant AppRoot
  participant PreviewAutomationHosts
  participant PreviewAutomationBroker
  participant MCPCaller
  AppRoot->>PreviewAutomationHosts: Pass routeThreadRef
  PreviewAutomationHosts->>PreviewAutomationBroker: Report activeThreadId
  MCPCaller->>PreviewAutomationBroker: Invoke request for thread
  PreviewAutomationBroker->>PreviewAutomationBroker: Rank matching activeThreadId
Loading

Suggested reviewers: bil0000

Merge Risk: ⚪ Minimal · up to 43a5e

The thread-aware routing change is ready to merge after normal checks; navigating away from a thread does not leave stale host preference behind.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 43a5e

A thread-aware routing preference should keep new tabs with the window showing their thread. The main remaining risk is that a delayed focus update could steer a new session to the wrong desktop and leave it pinned there.

Retained concerns

  • Medium · reliability · inferred: If reports from one live connection arrive out of order, an older routed-thread report can replace a newer one. A subsequent new preview session could select the wrong desktop and remain pinned to its browser profile until that session ends.
Security review details

Security Blast Radius

  • inferred — A mistaken thread preference can move a fresh preview operation between eligible desktop hosts in the same environment, where browser profiles and session state may differ. The observed eligibility filter does not let this preference select a host in another environment.

Trust Boundaries and Controls

  • observed — The broker accepts activeThreadId from a connected host without checking it against reported live tabs. It rejects focus updates for a different environment or connection generation; eligibility and existing tab ownership remain ahead of the new preference.

Resilience and Maintainability Implications

  • inferred — Connection-generation checks reject replaced hosts, but do not distinguish older and newer reports from the same connection. Whether the command transport preserves their order remains unverified.

Hardening Proposals

  • proposed — Establish per-connection ordering for focus reports, or have the broker reject superseded report versions, before relying on routed-thread state for sticky session selection.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 5 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main fix: opening new preview tabs on the window that shows the thread.
Description check ✅ Passed The description includes the required What Changed, Why, UI Changes, and Checklist sections. It explains the implementation, validation, scope, and rationale. The unchecked screenshot and video items …
Linked Issues check ✅ Passed The PR implements [#13771]. AppRoot derives the routed thread reference and passes it to PreviewAutomationHosts. Desktop hosts include activeThreadId in focus reports and resend reports when the…
Out of Scope Changes check ✅ Passed The changes stay within [#13771]. They modify the preview host contract, desktop route reporting, broker selection, and the related broker test. No unrelated product area or web and mobile behavior is…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

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

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: A new preview tab opens on another machine's window instead of the one showing the thread

1 participant