fix(terminal): stop stale PTY resize after worktree reveal#8502
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change prevents PTY resize reassertion from forwarding dimensions captured before a terminal grid change. It adds unit regressions for mid-flight refits and readback failures, plus an E2E-only delay seam for applied-size reads. A new E2E specification cycles worktree visibility, viewport sizes, and terminal state while checking immediate desynchronization and eventual convergence under natural and delayed read ordering. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The visibility-resume size readback captures xterm's pre-reveal grid as its resize target while the applied-size read is in flight. A reveal fit or snapshot-restore resize can change the grid mid-flight without queuing a newer request, so the resolved callback "repaired" the PTY back to the pre-reveal grid — an idle TUI (Claude Code) then redraws for the wrong grid until a manual resize (refs #7951, #7240). Instrumented traces show the stale capture on every reveal; correctness relied on the read winning FIFO against the fit's resize, which busy daemons and SSH/relay round-trips lose. Re-measure xterm at resolve time and re-run against the fresh grid instead of forwarding a stale target. Adds an e2e seam that delays the readback dispatch to reproduce the losing ordering, unit repros that fail without the guard, and a hidden-resize/reveal-cycle e2e spec.
20e0d3b to
ce54ff3
Compare
|
Live SSH verification (brennan-test-ssh pass): land.
Remaining gap (pre-existing, accepted): |
|
Windows verification (agent on Windows 11 Pro 25H2, PR head
Combined with the live SSH pass above and the macOS evidence, remaining risk is confined to: (1) eyeballs-on-Windows DOM-renderer QA, (2) the pre-existing |
|
Windows DOM-renderer QA (Win11 25H2, DOM renderer confirmed active): mixed — flagging before merge.
Open question before this lands: is that stale-child behavior pre-existing on Windows (base |
…ruth The reveal repro asserts on pty:getSize convergence; the bottom-bar TUI only makes the pane WINCH-reactive. Record that a long-lived process can miss Node's stdout 'resize' event under Windows ConPTY even when the OS PTY was resized, so future readers don't add a flaky bar-content check.
|
Windows flag resolved — not a regression. Cleared to land. A/B on Windows (idle-TUI reveal cycles) decisively attributes the stale-bar symptom to the test probe, not this PR:
So: Orca→ConPTY resize delivery works, the reveal logic isn't suppressing anything, and the earlier 10/10 xterm↔getSize convergence stands. The 0/10 bar result was a synthetic-probe artifact ( Full status: macOS A/B + perf lane green · live SSH pass 10/10 · Windows units 435/435, e2e 5/5 (with #8520), renderer convergence 10/10 · oscillation unit-tested. Remaining out-of-scope: the pre-existing |
|
Windows regression question — RESOLVED: not a regression. The prior bar-stale result was a probe artifact. A/B on Windows (visible-only D1, base
Conclusion: Orca→ConPTY resize delivery works; this PR's reveal logic does not suppress the resize; the symptom predates the PR and occurs without reveal; the 10/10 xterm↔ Net Windows status: units 435/435, e2e 5/5 (with the #8520 fixture fix), renderer convergence 10/10, no regression. Combined with the macOS A/B and the live SSH pass, I have no remaining regression concern for this PR. Only out-of-scope item is the pre-existing |
1 participant
Summary
Fixes the "bottom of the Claude Code TUI garbles after returning to a worktree; resizing repairs it" regression that became frequent in v1.4.136 (refs #7951, #7240).
Mechanism. On reveal,
noteVisibilityResume()requests a PTY size readback that captures xterm's pre-reveal grid as its resize target (pty-size-reassertion.ts). The applied-size read is an async daemon round-trip; the reveal fit refits xterm and resizes the PTY while it is in flight, with no follow-up request queued. Instrumented traces show this stale-capture interleaving on every worktree reveal — correctness relied entirely on the read being processed before the fit's resize (local FIFO luck). When the read loses that ordering — daemon busy serializing reveal snapshots (common since the #7214 hidden-delivery pipeline), SSH/relay round-trips — the resolved callback sees applied ≠ captured target and "repairs" the PTY back to the pre-reveal grid. An idle TUI then redraws for the wrong grid, and nothing heals it: no output means no grid-drift check, and no later layout change means no ResizeObserver request. Reproduced live on an unpatched build (e2e read-delay seam sweeping 15–60ms):FORWARD {123x46}issued while xterm and the PTY were correctly at 81x36.Fix. In
forwardIfDrifted, re-measure xterm at resolve time; if the grid moved while the read was in flight, suppress the forward and re-run against the fresh grid through the existingpendingcoalescing. Under the same adversarial sweep the fixed build logs the guard engaging repeatedly with zero stale forwards.Why it was hard to reproduce locally (also why #7961 couldn't confirm): on a fast idle machine the daemon read wins the FIFO race, and even when the stale forward fires, a ResizeObserver-driven follow-up reassertion heals it within ~30ms. The field lacks both escapes.
Related: #7961 (community PR by @gatsby74) independently proposed the same-shaped guard against #7951 — this PR adds the live reproduction, the read-delay seam, and regression coverage; credit to them for the early diagnosis.
Screenshots
No visual change. (The fix prevents the garbled-bottom-TUI frames; there is no new UI.)
Testing
pnpm lintpnpm typecheckpnpm test— 28 failures, all pre-existing on base550c41a35(A/B-verified via stash: identical 28 failed / 3 passed without this diff; PR-comments + Linear-skill-prompt areas, disjoint from this change). One additionalpty-connection.test.tsfailure under full-suite load is a contention flake: passes 3/3 in isolation. Zero new failures from this PR; scoped terminal-pane + pane-manager suites: 2458 passed.pnpm build—build:desktoppasses;build:nativefails at codesign (errSecInternalComponent) identically on base — local signing environment, unrelated to this renderer-only diff.Targeted validation:
pty-size-reassertion.test.tsreproduce the race and fail on unpatched main (staleforwardResize(80, 24)fires); pass with the fix. This is the deterministic regression guard.tests/e2e/terminal-reveal-stale-pty-resize-repro.spec.ts: hidden-resize/reveal cycles with an idle WINCH-reactive bottom-bar TUI, natural ordering + slow-read (seam) variants; asserts main's applied PTY size converges to xterm's grid every cycle.FORWARD(PTY yanked to the pre-reveal grid) under the seam sweep; fixed build logged 5× guard suppressions, zero stale forwards, all cycles converged.pnpm run test:e2e:terminal-perf: 20 passed, 0 failed (1 skip: missing local OpenCode capture fixture, environment-gated). Typing latency, redraw-freeze, output-scheduler, and worktree-revisit backpressure budgets all green.terminal-hidden-tui-visual-restore.spec.ts: 4/4 passed.550c41a35):terminal-column-desync-repro.spec.ts"vertical split reparent" fails 3/3 on base (2/3 with this PR — noise);terminal-hidden-view-parking.spec.tsfails 4/5 on base.AI Review Report
Reviewed the reassertion flow end to end: target capture, in-flight coalescing, the new resolve-time re-measure, rejection-path fallback, disposal, and PTY-rebinding guards. Key risks checked: (1) unbounded re-run loops — a re-run only occurs when the grid moved during that read's flight and converges once layout is stable for one flight; no timers or retained state. (2) Behavior on the common path — when the grid is unchanged mid-flight the guard is two property reads and behavior is byte-identical. (3) Readback-failure fallback — still performs the one guarded resize, now at the fresh grid instead of a stale one. (4) Fit semantics — the guard re-queues with
pendingonly, so no unrequested fits occur (visibility-resume's fit-avoidance against the transient hidden DOM fallback is preserved).Cross-platform: renderer-only logic, identical on macOS, Linux, and Windows; no shortcuts, labels, paths, shell behavior, or Electron platform APIs. Provider coverage: local/daemon and app-SSH PTYs flow through the same guarded path;
remote:runtime PTYs remain excluded by the existingisRemotePtyIdgate; mobile presence-lock and fit-override suppression (shouldSuppressDesktopResize) are checked before any forward, unchanged.Performance: the reassertion path runs only on visibility resume, settled layout changes, and drift checks — never on typing/output hot paths. Per resolve the guard adds two synchronous property reads; the extra applied-size IPC happens only when the grid moved mid-flight, and each prevented stale forward saves a resize IPC + SIGWINCH + full TUI redraw. The e2e read-delay seam is gated on the e2e build config (
exposeStore) and compiles to a 0-delay early return in packaged builds (one extra microtask hop on an IPC-bound path).Security Audit
No new input handling, command execution, filesystem/path access, auth, secrets, dependencies, or IPC channels. The e2e seam reads a window global only when the e2e build flag is set and can only add latency to an existing read in test builds; it cannot redirect or amplify any privileged operation. The guard strictly narrows which resizes are forwarded to the PTY.
Notes
remote:runtime viewport path is excluded from reassertion entirely (pre-existing); [Bug] Terminal UI breakage on switching #7951's remote-server variant may have a sibling in the relay resize path worth a follow-up.