Repository navigation
Conversation
…size Field bug (v1.4.144-rc.4): switching to a workspace with a streaming inline TUI (Codex) periodically shows a mostly-blank pane (viewport stranded mid-buffer, input box missing) or a bottom gap (PTY rows != pane rows) until a manual resize forces SIGWINCH. Three independent losing legs on the reveal path, each fixed: 1. Stranded corrective fit: the reattach/hidden-snapshot replay parks its PTY-resize continuation behind "the next successful fit"; when the pane is momentarily unmeasurable mid-reveal, no later fit is guaranteed (container size may already be final), leaving xterm at snapshot dims with no forwarded resize. safeFitAndThen now actively retries on settled frames (bounded) and breadcrumbs on exhaustion. 2. Swallowed replay paint: the cold-park remount path has no reveal repaint, so a replay parsed while xterm's RenderService is paused (IntersectionObserver lags the reveal) never painted. The settle refresh now drives one synchronous render through the pause latch (extends the #7614 release to the remount path). 3. Viewport stranded at the snapshot bottom: the reveal's bottom anchor is a one-shot that runs before deferred live chunks parse; an inline TUI keeps appending across the reveal. Re-enforce the restored scroll intent after the deferred drain parses (pins preserved). New regression harness: tests/e2e/terminal-inline-tui-reveal-convergence drives a codex-shaped inline fixture (agent-marked tab, scrollback inserts + bottom live block, synchronized output, mouse reporting) through hidden/parked/flapping/resize reveals under CPU throttle and asserts convergence with no resize: viewport at bottom, fresh frame + input row visible, still following, xterm == fit proposal == applied PTY size, and painted-pixel ink in the live-block band (WebGL forced). Co-authored-by: Orca <help@stably.ai>
|
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 (2)
📝 WalkthroughWalkthroughUpdates terminal reattach handling to reapply scroll intent after deferred output parsing, preserve hidden-output replay continuity, and reassert PTY size. Adds bounded safe-fit retries with cancellation, stale-continuation handling, and exhaustion breadcrumbs, plus render-pause refresh gating and unit tests. Introduces an inline TUI fixture and end-to-end scenarios covering hidden-pane reveals, resizing, repeated flapping, CPU throttling, geometry alignment, live output, and painted pixels. 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dda4ae23-73ab-4004-8c42-fe85935a2514
📒 Files selected for processing (7)
src/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/lib/pane-manager/pane-fit.test.tssrc/renderer/src/lib/pane-manager/pane-fit.tssrc/renderer/src/lib/pane-manager/pane-terminal-foreground-render-settle.test.tssrc/renderer/src/lib/pane-manager/pane-terminal-foreground-render-settle.tstests/e2e/fixtures/codex-inline-live-block-fixture.cjstests/e2e/terminal-inline-tui-reveal-convergence.spec.ts
| // Swallow query replies / mouse reports / keys like a real TUI agent. | ||
| process.stdin.resume() | ||
| if (process.stdin.isTTY) { | ||
| process.stdin.setRawMode(true) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make fixture shutdown explicit. Raw mode prevents \x03 from generating SIGINT, so the current cleanup leaves each infinite streaming fixture alive.
tests/e2e/fixtures/codex-inline-live-block-fixture.cjs#L112-L116: handle the shutdown byte explicitly or provide another deterministic termination mechanism.tests/e2e/terminal-inline-tui-reveal-convergence.spec.ts#L353-L356: invoke that termination mechanism and wait for shutdown before the next serial test begins.
📍 Affects 2 files
tests/e2e/fixtures/codex-inline-live-block-fixture.cjs#L112-L116(this comment)tests/e2e/terminal-inline-tui-reveal-convergence.spec.ts#L353-L356
| if (probe!.appliedPtySize) { | ||
| expect( | ||
| probe!.appliedPtySize, | ||
| `${label}: PTY applied size diverges: ${describeProbe(probe)}` | ||
| ).toEqual({ cols: probe!.cols, rows: probe!.rows }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require the PTY-applied size instead of silently skipping it.
For this local fixture, getSize should be available. Treating null as a pass can miss the stale-PTY-grid regression this suite explicitly targets.
Proposed fix
- if (probe!.appliedPtySize) {
- expect(
- probe!.appliedPtySize,
- `${label}: PTY applied size diverges: ${describeProbe(probe)}`
- ).toEqual({ cols: probe!.cols, rows: probe!.rows })
- }
+ expect(
+ probe!.appliedPtySize,
+ `${label}: PTY applied size unavailable: ${describeProbe(probe)}`
+ ).not.toBeNull()
+ expect(
+ probe!.appliedPtySize,
+ `${label}: PTY applied size diverges: ${describeProbe(probe)}`
+ ).toEqual({ cols: probe!.cols, rows: probe!.rows })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (probe!.appliedPtySize) { | |
| expect( | |
| probe!.appliedPtySize, | |
| `${label}: PTY applied size diverges: ${describeProbe(probe)}` | |
| ).toEqual({ cols: probe!.cols, rows: probe!.rows }) | |
| } | |
| expect( | |
| probe!.appliedPtySize, | |
| `${label}: PTY applied size unavailable: ${describeProbe(probe)}` | |
| ).not.toBeNull() | |
| expect( | |
| probe!.appliedPtySize, | |
| `${label}: PTY applied size diverges: ${describeProbe(probe)}` | |
| ).toEqual({ cols: probe!.cols, rows: probe!.rows }) |
| const tabBId = await createActiveTerminalTab(orcaPage, setup.worktreeId) | ||
| expect(tabBId).not.toBe(setup.tabId) | ||
| await orcaPage.waitForTimeout(Math.min(PARKING_DELAY_MS / 2, 2_000) + 1_000) | ||
|
|
||
| await activateTerminalTab(orcaPage, setup.tabId) | ||
| await waitForActiveTerminalManager(orcaPage, 30_000) | ||
| await assertRevealConvergence(orcaPage, testInfo, setup, 'hidden-mounted-reveal') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep this reveal inside the mounted-tab parking window.
With the default 500 ms delay, Line 536 waits 1,250 ms, so tab A can cold-park before activation. That makes this test duplicate the parked-remount path instead of covering hidden-but-mounted restoration.
Remove the delay, assert that window.__paneManagers still contains setup.tabId, and increase the configured parking delay if tab creation itself can exceed the window.
…rain flush Review hardening on the reveal-convergence fixes: 1. transport.resize is fire-and-forget on daemon/SSH; a silently dropped resize leaves the PTY at stale rows while xterm is correct — the "input box floats above the pane bottom, walking down as output accumulates" field variant (#7951 class). The remount path has no visibility-resume reassert, so fitAfterReattachRestore now verifies the provider's APPLIED size after the reattach fit and re-forwards only on drift (one getSize IPC per parked reveal, zero steady-state). 2. The post-drain scroll-intent enforce used an unbounded synchronous flush; a flood pane's queue could exceed the deferral budget at reveal. Now bounded to MAX_DEFERRED_REATTACH_LIVE_CHARS with a FIFO parse probe — enforcing after a partial drain stays correct (follow anchors at the bottom and native follow tracks the remainder). Co-authored-by: Orca <help@stably.ai>
|
Hardening commit
Re-verified: full |
The 750ms foreground deadline can abandon a hidden-output restore while a RESOLVED snapshot's replay is still parsing. Those replay writes are already queued in xterm's FIFO and WILL paint everything at or before the snapshot seq, but the abandon (a) rewrote its parked live chunks raw — duplicating every line the replay covers (field: TUI lines stacked twice on reveal) — and (b) skipped baseline adoption, so main's ACK-backlog re-deliveries below the seq duplicated again. Track the in-flight replay snapshot from the moment its writes are queued; on abandon, seq-slice the pending write-through against it (raw fallback only for unmappable offsets — availability still wins) and adopt it as the live reconcile baseline with the drain's continuity advance. Abandons with no started replay keep the existing raw path. Deterministic repro: pty-connection.test.ts stages snapshot-resolve → replay parse held → deadline abandon, asserting no rewrite of covered chunks, dedup of post-abandon backlog, and write-through of new bytes. Co-authored-by: Orca <help@stably.ai>
|
Commit Root cause: the 750ms foreground deadline can abandon a hidden-output restore while a resolved snapshot's replay is still parsing. The replay writes are already in xterm's FIFO and will paint everything ≤ snapshot seq, but the abandon (a) rewrote its parked live chunks raw — duplicating every replay-covered line — and (b) skipped Fix: track the in-flight replay snapshot from the moment its writes are queued; on abandon, seq-slice the pending write-through against it (raw fallback only for unmappable offsets) and adopt it as the live reconcile baseline with the drain's continuity advance. Abandons where no replay started keep the existing raw write-through (correct there — nothing painted). Repro-first: new deterministic unit test stages snapshot-resolve → replay-parse-held → deadline-abandon; red before the fix at the exact duplication assertion, green after. Full battery re-verified: 447 pty-connection tests, seq-slicing fuzz suite, typecheck/lint, and 15 e2e (parking, convergence, visual-restore suites) against a rebuilt bundle. Note: a visually identical symptom also arises when Codex's erase misses the old live-block rows under a PTY/xterm row mismatch — that trigger is closed by this PR's size-convergence fixes, so both duplication sources are addressed here. |
|
Superseded by #9503, which carries the hardened version of this fix on current main and has now merged. Closing this older PR to keep the review path unambiguous. |
Field bug
On v1.4.144-rc.4, switching to a workspace whose Codex TUI kept streaming while hidden periodically shows:
Both persist until a manual resize (e.g. Cmd+L sidebar toggle) forces SIGWINCH and Codex repaints. Same family as #7951.
Since parking landed, a workspace hidden >30s beyond the hot-retain budget fully unmounts its terminal and rebuilds from a daemon snapshot on reveal — a path none of the prior fixes (#7614/#8625/#8674) protect. Inline-mode TUIs are uniquely exposed because they keep appending across the reveal, so the snapshot's bottom is never the live bottom (alt-screen fixtures in existing specs can't catch this).
Three independent losing legs, each fixed
Stranded corrective fit (
pane-fit.ts) — the reattach/hidden-snapshot replay parks its PTY-resize continuation behind "the next successful fit". When the pane is momentarily unmeasurable mid-reveal, no later fit is guaranteed (the container size may already be final, so no further ResizeObserver event ever fires) and xterm stays at snapshot dims with no forwarded resize/SIGWINCH → the bottom-gap state.safeFitAndThennow actively retries on settled frames (bounded at 40 attempts) and records aterminal_safe_fit_retry_exhaustedbreadcrumb if it ever gives up, so a residual field case names itself.Swallowed replay paint (
pane-terminal-foreground-render-settle.ts) — the cold-park remount path has no reveal-repaint, so a replay parsed while xterm's RenderService is paused (its IntersectionObserver lags the reveal by a frame, worse under load) never painted: buffer perfect, canvas blank until drag-select/resize. The settle refresh now drives one synchronous full render through the pause latch (extends the fix(terminal): repaint revealed panes stuck behind xterm's paused-render gate #7614 release, which only covered hidden-but-mounted reveals, to every foreground/replay refresh).Viewport stranded at the snapshot bottom (
pty-connection.ts) — the reveal's bottom anchor is a one-shot that runs before the live chunks deferred during reattach get parsed. The restored scroll intent is now re-enforced after the deferred drain parses: following panes re-anchor to the live bottom; a deliberate scroll-up pin is preserved (enforceTerminalCurrentScrollIntentsemantics). Deferred data is capped at 512KB, bounding the extra parse cost.Repro / regression harness
New
tests/e2e/terminal-inline-tui-reveal-convergence.spec.ts+tests/e2e/fixtures/codex-inline-live-block-fixture.cjs: a codex-shaped inline TUI (agent-classified tab via launch telemetry, scrollback inserts + bottom live-block redraws, synchronized output, mouse reporting, startup queries, heartbeat file) driven through six reveal shapes — hidden-mounted, worktree switch after a hidden-time window resize, parked reveal ×6 cycles under 6× CPU throttle with 8k-line scrollback, rapid tab flapping, rapid worktree flapping, and parked reveal after a parked-time resize. Each requires convergence with no resize: viewport at buffer bottom, a fresh frame + input-box row on screen, still following afterwards, xterm grid == fit proposal == PTY-applied size, and painted-pixel ink in the live-block band (WebGL forced) so paint-layer bugs can't hide behind a healthy buffer.The exact field interleavings are sub-frame races that even the escalated harness could not force on demand, so the product changes are repairs of statically verified latches — legs 1 and 2 each carry a deterministic unit repro that is red without the fix (
pane-fit.test.ts,pane-terminal-foreground-render-settle.test.ts) — and the e2e suite is the standing regression net for the symptom family.Verification
pty-connectionsuite (446), scroll-intent, pane-tree-ops, fit-observer, write-callback-guard, render-pause-release, reveal-repaint suites green.pnpm typecheckand oxlint clean.Behavior notes / trade-offs
Known follow-ups (out of scope)
pty-connection.tsabandon), which can duplicate visible lines under sustained floods — needs its own fuzz-mirror repro.Made with Orca 🐋