Windows · Review · Partial validation
Ctrl+Insert copies through the existing handler
The PR #8541 change passed 49 focused tests and the scoped format, lint, and web type checks. A synthetic keydown in the real Windows Chrome terminal copied the selection to the Windows OS clipboard and kept it selected. Physical-key and Electron testing remain incomplete because the available browser shortcut tool substitutes copy events. No blocking defect was found in the PR; terminal cleanup exposed one separate baseline issue.
The verdict is a comment
Source: evidence.zip, test-local.log, verification.json, and the two-file PR diff.
Do a physical-key smoke test before treating this as a complete Windows sign-off. Select terminal text in Windows Chrome and Electron, press Ctrl+Insert, and paste into a plain text field. Verify repeated copy, selection retention, and no selection.
Head a6b41c0177fbb25e793dd8284def7d31dee090f4
Base and merge base 6df0add6e65b7c0040b5774635f794e4ff802ddd
Comparison base...head. The PR is not stacked and contains two changed files, 22 additions and 4 deletions. This is the first local review of this head. GitHub listed it as open and non-draft with an existing bot approval.
Environment: Windows build 26200, x64, Chrome 152.0.0.0 with navigator.platform = Win32, Node 24.13.1, pnpm 11.10.0, Vite+ 0.3.0, Vitest 4.1.11. The isolated checkout is E:/Projects/t3code-review-8541. No private projects or live app database were imported.
The change reuses terminal copy
Shortcut detection accepts Ctrl+Insert on non-Mac platforms without Shift or Meta. With a selection, keydown primes the terminal textarea, prevents the default action, and invokes document.execCommand("copy"). The existing copy listener writes the selected text. The key release is suppressed, and the selection stays available for another copy. Without a selection, the existing terminal key encoder handles the key.
The terminal drawer and Settings font preview both use this class. Web and desktop share the implementation; Electron runtime behavior was not tested. Mobile uses a separate client and is outside this Windows shortcut change. Provider adapters, wire contracts, connection modes, command palette actions, and user guidance do not change. Clipboard handling runs in the client, but remote and tunnel sessions were not exercised.
One issue belongs to the baseline
1. P2 baseline: closing a Windows terminal can leave its shell running
Baseline only The terminal disappeared from the UI after Close and Confirm. The server then logged Signals not supported on windows., and its PowerShell child was still alive. Expected behavior is that closing the terminal stops its owned shell.
Manager.ts:1620 calls process.kill("SIGTERM"); NodePtyAdapter.ts:90 forwards it to node-pty. On failure, the manager returns before escalation. Both source files and the dependency lockfile are unchanged between the pinned base and head.
Reproduction: open a terminal in the isolated Windows app, close it, confirm, then inspect the server warning and the recorded shell PID. The shell was PID 53312 with the review server as its parent. A separate fixture using the same node-pty 1.1.0 reproduced rejection of SIGTERM, including a deferred exception. A signal-free kill produced an exit receipt; its console-list helper also logged AttachConsole failed. The owned processes were absent after cleanup.
Recommended correction in separate work: implement Windows-aware termination at the adapter boundary and test both ready and close-before-ready processes. This finding is not a reason to request changes on the copy-shortcut PR.
Source: evidence.zip contains baseline-close.log, pty-baseline.cjs, and pty-baseline-verified.log. Base comparison: git diff BASE HEAD -- apps/server/src/terminal/Manager.ts apps/server/src/terminal/NodePtyAdapter.ts pnpm-lock.yaml returned no changes.
The checks passed within their limits
| Check | Observed result | Boundary |
|---|---|---|
| Format and lint | Passed | Two changed files, pinned local vp |
| Web typecheck | Passed | Web package, no repo-wide suite |
| surface.test.ts | 49 tests passed | Existing suite includes Linux and Mac shortcut cases |
| Ctrl+Insert with selection | OS clipboard matches selection | Synthetic keydown; trusted copy event from execCommand |
| Repeat Ctrl+Insert | Copied again; selection retained | Windows OS clipboard read using Get-Clipboard |
| Ctrl+Insert without selection | Clipboard sentinel unchanged | Existing key-encoding path used |
| Ctrl+Shift+C | OS clipboard matches selection | Synthetic keydown through existing handler |
| Ctrl+C | Selection cleared; browser clipboard updated | Bridge intercepts async clipboard; OS clipboard unchanged |
| Shift+Insert | Marker appeared once in shell input | Browser clipboard fallback; marker never executed |
| Native Windows / Electron | Incomplete | No physical-key injection or Electron UI test |
| Terminal close | Baseline failure | UI closed; SIGTERM rejected; shell survived until cleanup |
Source: verification.json, browser-events.json, screenshots, Windows Get-Clipboard reads, and focused command outputs in evidence.zip.
The browser shortcut API translated both Ctrl+C and Ctrl+Insert into untrusted copy events. Its initial success was discarded as keydown evidence. A temporary, labeled button in the real app then focused the actual terminal input and dispatched synthetic keydown and keyup events. Ctrl+Insert caused a trusted copy event and changed the Windows OS clipboard. All temporary controls and event listeners were removed.
Bun was not installed, so the requested Bun commands could not run. Equivalent scoped vp checks ran instead, honoring the repository's prohibition on repo-wide checks. An initial global Vite+ 0.2.2 test attempt failed during suite initialization; rerunning with the checkout's Vite+ 0.3.0 passed. The initial typecheck also started before installation completed; it passed after setup finished. Cold startup caused dev-server restarts and transient connection errors; the app recovered before clipboard testing. These setup problems are not PR findings.
Not tested: physical keyboard routing, Electron menus and accelerators, Firefox, macOS, Linux, mobile, remote/tunnel sessions, and performance or installer behavior.
The real app showed copy and paste results
The review-only panel makes the input method visible. The checkout name identifies the isolated app. These images show synthetic-handler testing, not physical keyboard testing.
Ctrl+Insert copied E:\Projects\t3code-review-8541> to the Windows OS clipboard and left the range selected.
Shift+Insert pasted the marker once into shell input. Enter was not pressed.
Source: handler-copy.png and shift-insert.png in the evidence bundle. Screenshots were inspected before inclusion.
Reproduce the scoped checks
# From the dedicated checkout, after vp i
node node_modules/vite-plus/bin/vp fmt --check apps/web/src/terminal/ghostty/surface.ts apps/web/src/terminal/ghostty/surface.test.ts
node node_modules/vite-plus/bin/vp lint --report-unused-disable-directives apps/web/src/terminal/ghostty/surface.ts apps/web/src/terminal/ghostty/surface.test.ts
vp run --filter @t3tools/web typecheck
# From apps/web, use the pinned runner
node ../../node_modules/vite-plus/bin/vp test run --project unit src/terminal/ghostty/surface.test.ts
# Isolated application; put local node_modules/.bin first on PATH
node scripts/dev-runner.ts dev --home-dir E:/tmp/windows-pr-8541/app-state
# After a copy probe on Windows
Get-Clipboard -Raw
Observed ports were web 8178 and server 16218. Pair with a fresh token from your own run. Token-bearing logs and private app state are excluded from the allowlisted archive.
Evidence files and cleanup
Download evidence.zip for verification.json, focused logs, event traces, selected screenshots, and the baseline reproduction. Temporary browser instrumentation was removed. The owned development process tree was stopped, and the fixture PIDs were absent. The checkout remains at the pinned head with a clean git status. Worktree and evidence are retained for follow-up.