Repository navigation
fix(terminal): keep the reattach grid push alive for a hidden pane - #13155
Merged
Merged
Conversation
A restored Floating Workspace pane is display:none for its whole PTY reattach, because the panel always mounts closed after a restart. The display:none guard in safeFitAndThen dropped the reattach continuation outright, so the PTY never received the client grid and no explicit SIGWINCH was sent to repaint the restored TUI after the replay. Parking the continuation is not an option: the structural replay coordinator holds live PTY bytes behind its promise, so a closed panel would stall output indefinitely. Settle the promise immediately instead and move the continuation to a per-pane deferred bucket that the first measurable fit drains. Deferral is opt-in so the bounded-degradation contract still covers every other caller, and the continuation re-checks mobile PTY ownership at fire time now that the wait is unbounded.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds pane-scoped and terminal-keyed registries for deferred fit continuations. Hidden panes retain continuations until they become measurable. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e5ac4692-e96e-4365-89c1-b7fae5dc3b5c
📒 Files selected for processing (6)
src/renderer/src/components/terminal-pane/pty-connection.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/lib/pane-manager/pane-fit-continuation-registry.tssrc/renderer/src/lib/pane-manager/pane-fit-deferred-continuations.tssrc/renderer/src/lib/pane-manager/pane-fit.test.tssrc/renderer/src/lib/pane-manager/pane-fit.ts
Comment on lines
+220
to
+225
| if (isPendingSafeFitContinuationCurrent(pane, operationKey, pending)) { | ||
| if (!safeFit(pane) && options.retryIfUnmeasurable) { | ||
| if (isManagedPaneDisplayNone(pane)) { | ||
| cancel() | ||
| // Why not the frame retry: a zero-box pane can stay hidden indefinitely, so | ||
| // burning the budget only logs an exhaustion crumb. Hand it to the reveal instead. | ||
| releaseSafeFitContinuationUntilMeasurable(pane, operationKey, pending) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep hidden-pane retry behavior for non-opt-in continuations.
These branches release every display:none continuation. releaseSafeFitContinuationUntilMeasurable resolves it as incomplete and only defers it when deferIfHidden is true. Therefore, callers such as hidden-snapshot-pty-resize, which set retryIfUnmeasurable: true without deferIfHidden, no longer use the bounded retry path.
src/renderer/src/lib/pane-manager/pane-fit.ts#L220-L225: only release whenpending.deferIfHiddenis true. Otherwise, arm the retry.src/renderer/src/lib/pane-manager/pane-fit.ts#L236-L240: apply the same condition in the direct fit path.src/renderer/src/lib/pane-manager/pane-fit-continuation-registry.ts#L120-L125: only release hidden entries whenpending.deferIfHiddenis true. Keep other entries pending for retry.
📍 Affects 2 files
src/renderer/src/lib/pane-manager/pane-fit.ts#L220-L225(this comment)src/renderer/src/lib/pane-manager/pane-fit.ts#L236-L240src/renderer/src/lib/pane-manager/pane-fit-continuation-registry.ts#L120-L125
Contributor
Author
Electron QA — PASS (
|
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
…tablyai#13155) * fix(terminal): keep the reattach grid push alive for a hidden pane A restored Floating Workspace pane is display:none for its whole PTY reattach, because the panel always mounts closed after a restart. The display:none guard in safeFitAndThen dropped the reattach continuation outright, so the PTY never received the client grid and no explicit SIGWINCH was sent to repaint the restored TUI after the replay. Parking the continuation is not an option: the structural replay coordinator holds live PTY bytes behind its promise, so a closed panel would stall output indefinitely. Settle the promise immediately instead and move the continuation to a per-pane deferred bucket that the first measurable fit drains. Deferral is opt-in so the bounded-degradation contract still covers every other caller, and the continuation re-checks mobile PTY ownership at fire time now that the wait is unbounded. * fix(terminal): preserve replacement deferred grid push
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
After an app restart, the Floating Workspace panel always mounts closed, so its restored terminal panes are
display:nonefor the entire PTY reattach. Reattach pins xterm to the snapshot's grid and writes the replay there — correct, and required, because xterm's serialize format is width-implicit (#7279). The step that then pushes the client's real grid back to the PTY and sends the explicitSIGWINCHthat makes a restored TUI fully repaint after the replay —fitAfterReattachRestore— was permanently dropped by thedisplay:noneguard insafeFitAndThen.The guard exists for a real reason:
structuralReplayCoordinatorholds live PTY bytes behind that continuation's promise, so simply parking it would stall output for a closed panel indefinitely. This splits the two concerns instead — the completion promise still settles immediately, but the continuation moves to a per-pane deferred bucket that the first measurable fit drains.Deferral is opt-in (
deferIfHidden) and used only by the reattach grid push. The otherretryIfUnmeasurablecaller (hidden-snapshot-pty-resize, which hitsdisplay:noneon ordinary background tabs) keeps its current behavior rather than being changed implicitly.What this does not fix. On macOS the shell is spawned under
/usr/bin/loginfor TCC attribution (macos-tcc-login-shell.ts:321), andsendSignaltargets that pid (local-pty-provider.ts:1205).loginneither handles nor forwardsSIGWINCH, so the explicit repaint signal is swallowed before it reaches the shell. That is pre-existing and also affects the visible reattach path, so a restored TUI on macOS still may not repaint cleanly. Filed separately; this PR is the prerequisite half (the signal has to be sent before it can be delivered).pane-fit.tswas already exactly at the 300-linemax-linescap, so the pending-continuation registry moved topane-fit-continuation-registry.tsrather than adding a disable.Screenshots
No visual change— this restores a signal and a resize on a path that previously sent neither. Behavioral evidence is the A/B below rather than a screenshot.Testing
pnpm lintpnpm typecheckpnpm testpnpm buildLive A/B on a restarted app, with the panel sized identically before and after the restart so the kernel contributes no
SIGWINCHof its own — isolating the explicit one. A shell trappingWINCHcounted deliveries on panel open:stty sizebefore quit32 11332 113Run on an isolated profile (own daemon) against a throwaway repo, with
ORCA_DISABLE_MACOS_LOGIN_SHELL=1so the signal was observable at all — see the macOS caveat above. Verified per arm that the pane's ancestor chain really wasdisplay:nonefor the whole reattach, and that the served module was actually patched (no HMR trust).Five regression tests, each individually confirmed to fail with its specific fix patched out:
pty-connection.test.ts— forwards the destination grid on reveal after adisplay:nonereattach. Deliberately a near-copy of the existing "waits for a recovered destination fit…" test above it, differing only in the hidden ancestor.pty-connection.test.ts— suppresses the deferred push when mobile claims the PTY while the pane is hidden.pane-fit.test.ts— runs adisplay:nonecontinuation on the first fit after reveal; also asserts the completion promise still resolvesfalseimmediately, that noterminal_safe_fit_retry_exhaustedbreadcrumb is logged, and that the continuation is one-shot.pane-fit.test.ts— parks on one pane object and drains on a different one. PTY connections hold atoPublicPane()wrapper whilePaneManagerdrains via internal panes, so a pane-identity key is silently unreachable; this is the trappane-metric-options-deferral.ts:16-18already documents. An earlier revision of this PR had exactly that bug and its tests could not see it.pane-fit.test.ts—cancel()after deferral, and same-key replacement not double-firing.Local full-suite has 18 pre-existing failures across
src/main,src/relayand a root-directory guard — zero overlap with this diff, all environment-shaped (realHOME/ORCA_*leaking into env-assertion tests).AI Review Report
Two independent passes: an adversarial review of the diff, and a history sweep of every prior PR that established an invariant in this code. Both found real defects, all fixed here:
ManagedPane, which the drain path never sees, making the change a no-op. Re-keyed onpane.terminal, matching fix(terminal): defer metric option writes to unmeasurable panes #12944's rule.docs/mobile-presence-lock.md) — the pre-check ran once, but the wait is now unbounded, so a phone could claim the PTY in between. Since this path callstransport.resizedirectly it bypassesforwardPtyResize's guard, andpty:signalhas no mobile-lock twin. The continuation now re-checksshouldSuppressDesktopPtyResize()at fire time, matching the pattern atTerminalPane.tsx:1733.cancel()became a no-op after deferral — it now clears the bucket too, restoring the contract (fix(terminal): make viewport ownership transactional across output, replay, and reflow #8674) that a new stream generation cannot inherit an old replay's grid push.onExhaustedpath deliberately still drops; perf: bound repeated watcher, Session History, and terminal work #12828's twodisplay:noneguard tests pass unchanged.SIGWINCHwhere nothing was sent before. It reachesprocess.killinside atry/catch, so an unsupported signal name on Windows/ConPTY is swallowed; remote/SSH PTYs are excluded byisRemoteRuntimePtyId. The macOSlogin-wrapper delivery gap is documented above. No shortcuts, labels, or path handling touched. SSH and folder-workspace cases share this renderer path with no behavioral difference.Security Audit
No new IPC surface, no input handling, no command execution, no path handling, no auth or secrets, no dependency changes. The only privileged call reached is the existing
pty:signalIPC with a literal'SIGWINCH'and a pty id the transport already owns, double-guarded by a generation check and a pty-id equality check. Net change is renderer-local bookkeeping of a callback that already existed.Notes
display:none→ measure-lease (laid out full size but still hidden) without becoming visible, the deferred continuation can fire while hidden. That matchesfitAfterReattachRestore's existing intent — it deliberately bypasses the visibility gate because it is the reattach repair — and the measure-lease reasoning settled in fix(terminal): restore paired snapshots at the source grid #12768. Flagging it because hidden SIGWINCH resetting full-screen TUIs is a known hazard here.sendSignalshould target the foreground process group rather than theloginwrapper pid, or the explicit reattach repaint never lands on macOS.floatingTerminalOpenisuseState(false)inApp.tsxand never persisted, so the Floating Workspace always returns closed regardless of how you left it. That is what puts these panes on the hidden path.onExhaustedpath drops the reattach continuation the same way this fixes fordisplay:none; an existing test asserts that contract deliberately.