Skip to content

fix(web): preserve composer focus during preview presses - #9530

Closed
0bkevin wants to merge 4 commits into
pingdotgg:mainfrom
0bkevin:fix/5792-preserve-composer-focus
Closed

fix(web): preserve composer focus during preview presses#9530
0bkevin wants to merge 4 commits into
pingdotgg:mainfrom
0bkevin:fix/5792-preserve-composer-focus

Conversation

@0bkevin

@0bkevin 0bkevin commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Problem

preview_press temporarily focuses the preview guest WebContents so Electron can deliver native keyboard input. Restoring the outer renderer afterward did not restore its active DOM element, interrupting users typing in either the current thread or another thread.

Fix

Capture the renderer's meaningful active element around preview_press and restore it after success or failure while that element remains connected.

Restoration is best-effort, avoids scrolling, skips detached elements, and remains scoped to the press path that transfers native focus.

Testing

  • ./node_modules/.bin/vp test run apps/web/src/lib/previewAutomationFocus.test.ts — 4 tests passed
  • ./node_modules/.bin/vp lint --report-unused-disable-directives apps/web/src/lib/previewAutomationFocus.ts apps/web/src/lib/previewAutomationFocus.test.ts
  • ./node_modules/.bin/vp fmt apps/web/src/lib/previewAutomationFocus.ts apps/web/src/lib/previewAutomationFocus.test.ts --check
  • ./node_modules/.bin/vp run -F @t3tools/web typecheck
  • git diff --check
  • Integrated Electron pass: reproduced the unfixed branch moving focus from composer-editor to a host button and dropping the next keystroke; verified the fixed branch keeps composer-editor focused and accepts the next keystroke
  • Captured direct, unedited 1440×900/30fps before and after recordings

Fixes #5792

Implemented with GPT-5.6 Luna and reviewed with GPT-5.6 Sol in T3 Code through the Codex harness.


Note

Medium Risk
Changes native focus/keyboard cleanup and IPC contract for press, with nuanced restore vs preserve behavior; mistakes could leave stuck keys or steal composer focus, but scope is limited to preview automation press.

Overview
Preview automation press now reports whether native focus was restored or preserved, and surfaces human takeover as an explicit interrupted outcome instead of a silent void.

Contracts add DesktopPreviewAutomationPressResult (completed with restored / preserved, or interrupted). Desktop PreviewManager.automationPress returns that disposition: cleanup runs uninterruptibly (key-up and focus emulation off even on fiber interrupt), native restore is skipped when the app is unfocused or focus moved elsewhere, and human control during press still fails with PreviewAutomationControlInterruptedError without restoring renderer focus. IPC maps that error to { status: "interrupted" } via automationPressResult.

On web, preview_press runs inside new withPreviewAutomationFocus, which serializes focus transactions, tracks trusted user focus intent, and restores the prior DOM element only when the desktop result says focusDisposition === "restored". Interrupted presses throw PreviewAutomationControlInterruptedHostError and serialize to PreviewAutomationControlInterruptedError for automation clients.

Breaking: DesktopPreviewBridge.automation.press and the preview IPC handler return DesktopPreviewAutomationPressResult instead of void.

Reviewed by Cursor Bugbot for commit d293589. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Preserve composer focus during preview automation presses

  • Desktop performAutomationPress now inspects the focused WebContents and BrowserWindow during cleanup and restores the prior renderer only when automation still owns focus or the prior renderer already has it; human-control takeover completes cleanup before re-emitting the interruption.
  • Adds withPreviewAutomationFocus in previewAutomationFocus.ts to serialize concurrent focus transactions, track trusted user focus vs synthetic input via trackFocusIntent, and restore the prior host element only when the document stays focused and no newer transaction supersedes it.
  • Changes the desktop press IPC contract from Promise<void> to Promise<DesktopPreviewAutomationPressResult> in ipc.ts, returning either a completed result with focusDisposition or an interrupted status; the web host converts interrupted results into the new PreviewAutomationControlInterruptedHostError.
  • Behavioral Change: DesktopPreviewBridge.automation.press and PreviewManager.automationPress now return a typed DesktopPreviewAutomationPressFocusDisposition or DesktopPreviewAutomationPressResult instead of void.

Macroscope summarized d293589.

@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 4, 2026
@0bkevin

0bkevin commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Alternatives considered

I reviewed the two earlier open approaches and then verified the behavior in the real Electron desktop app:

The integrated Electron pass also caught and corrected an earlier guard in this PR: native Tab dispatch can move focus to another connected host button, so restoration must not be conditional on document.activeElement becoming body/null. The final implementation always restores the previously focused element when it remains connected, on both success and failure. On the unfixed branch, the next typed character was dropped; on this branch, focus stayed on composer-editor and the character was accepted.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 4, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This changes preview press behavior across the web renderer, desktop native focus handling, and the IPC contract, adding nontrivial focus tracking, concurrency coordination, cancellation cleanup, and interruption reporting. The cross-layer runtime behavior is substantially more involved than a small isolated fix and merits human review.

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

@0bkevin
0bkevin force-pushed the fix/5792-preserve-composer-focus branch from e7a5459 to c88499a Compare September 4, 2026 01:29
@macroscopeapp
macroscopeapp Bot dismissed their stale review September 4, 2026 01:29

Dismissing prior approval to re-evaluate c88499a

Comment thread apps/web/src/components/preview/PreviewAutomationHosts.tsx Outdated
@0bkevin

0bkevin commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Live T3 Connect validation

I also exercised this through the hosted web client against a real connected environment:

  • Opened app.t3.codes and confirmed under Settings → Connections that vm-t3code-dev was Connected via T3 Connect with the relay online.
  • Created a separate live thread and asked the agent to use browser preview automation against https://httpbin.org/forms/post.
  • The remote run opened the page, typed into Customer name and Telephone, clicked Medium and Bacon, scrolled, filled Comments, and verified the final values from a snapshot.
  • While that run executed preview_press(Tab), I kept FOCUS-PROBE-A in the hosted composer. The composer remained the active element and accepted a following -B, ending at FOCUS-PROBE-A-B.

Scope note: the hosted web right panel explicitly reports that the Browser surface is only available in the desktop app. This validates the T3 Connect/relay path and hosted-composer behavior, but it does not execute the Electron-specific renderer focus wrapper changed by this PR; that path is covered by the separate integrated Electron before/after pass.

The live browser run also exposed a separate preview-driver quirk: Tab did not advance to the intended form field, so the agent corrected the two fields explicitly before the final snapshot. That is distinct from this PR's host-composer focus restoration and is not hidden by the successful final values.

Prevent preview automation from reclaiming a stale composer or native renderer focus after the user takes control during an awaited press. Report native focus disposition and interruptions through the typed IPC contract, and cover renderer, desktop, and response serialization behavior with focused tests.

Model: GPT-5
Harness: Codex CLI
@github-actions github-actions Bot added size:L 100-499 changed lines (additions + deletions). and removed size:M 30-99 changed lines (additions + deletions). labels Sep 4, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6da3508. Configure here.

Comment thread apps/desktop/src/preview/Manager.ts
Treat a successful press as natively restored when the previously focused renderer still owns focus after the operation. This lets the host restore its DOM focus without issuing a redundant native focus call, while different focus owners remain preserved.

Model: GPT-5
Harness: Codex CLI
Comment thread apps/desktop/src/preview/Manager.ts Outdated
Run preview press key-up and focus-emulation cleanup in an uninterruptible region so cancellation cannot leave Chromium with held input or stale focus emulation. Add deterministic Deferred-backed cancellation coverage at the cleanup boundary.

Model: GPT-5
Harness: Codex CLI
@0bkevin

0bkevin commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

This PR is superseded by the two focused draft PRs:

  • #9848 — native desktop focus handling.
  • #9849 — renderer DOM focus restoration.

Both branches are independently based on main, keep the existing Promise<void> IPC contract, and are pending live Electron evidence. Closing #9530 keeps the original combined cross-layer change out of review while the two focused PRs proceed separately.

@0bkevin 0bkevin closed this Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 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]: Browser automation steals composer focus across threads

1 participant