From 0f7ec4458d37010338f16e70ff06957cb335e074 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 27 Jul 2026 02:42:00 -0700 Subject: [PATCH] fix(terminal): clear the render model on the plain-refocus repaint path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `schedulePaneRevealPresent` — the atlas-preserving path a plain window refocus takes — only called `terminal.refresh()`. xterm's renderers are diff-based: `_updateModel` early-continues on any cell whose code/fg/bg/ext still match the cached model, so a refresh repaints nothing for a pane whose buffer never changed. When an occluded window loses its canvas contents while that model stays populated, the refresh skips exactly the cells that went stale and the pane keeps compositing pre-hide pixels — until a window resize reallocates the model, which is the repair users find by hand. Clear the model first (`RenderService.clear()` → renderer `clear()` → `_clearModel(true)`) so the refresh becomes a guaranteed full repaint. That drops cached cells and glyph vertices but NOT the texture atlas, which is shared by every same-config terminal and whose mid-stream wipe re-arms xterm's page-merge garble race (xterm.js #4480) — the reason this path is atlas-preserving in the first place. Also covers the DOM-renderer fallback in `resetWebglTextureAtlas`: `clearTextureAtlas()` is what invalidated the model on the WebGL path, so a pane without an addon had nothing invalidate it and hit the same skip. Scope note: the e2e spec guards buffer/geometry convergence across the hide/reveal boundaries and adds idle-agent and headful desktop-hide cases, but it cannot observe a stale canvas — both oracles built for that (canvas-vs-buffer ink sampling, screenshot-vs-forced-repaint) were proven blind by injecting the defect, and the spec header documents why. The unit tests pin the ordering and the atlas-preservation invariant. Refs STA-2694. Co-authored-by: Orca --- .../lib/pane-manager/pane-reveal-repaint.ts | 23 ++- .../lib/pane-manager/pane-webgl-renderer.ts | 12 +- .../terminal-render-model-clear.test.ts | 67 +++++++ .../terminal-render-model-clear.ts | 55 ++++++ ...pencode-altscreen-reveal-artifacts.spec.ts | 163 ++++++++++++++++-- 5 files changed, 296 insertions(+), 24 deletions(-) create mode 100644 src/renderer/src/lib/pane-manager/terminal-render-model-clear.test.ts create mode 100644 src/renderer/src/lib/pane-manager/terminal-render-model-clear.ts diff --git a/src/renderer/src/lib/pane-manager/pane-reveal-repaint.ts b/src/renderer/src/lib/pane-manager/pane-reveal-repaint.ts index 9aa24002b6d..c36aa87fa51 100644 --- a/src/renderer/src/lib/pane-manager/pane-reveal-repaint.ts +++ b/src/renderer/src/lib/pane-manager/pane-reveal-repaint.ts @@ -2,6 +2,7 @@ import type { ManagedPaneInternal } from './pane-manager-types' import { reattachWebglIfNeeded } from './pane-webgl-reattach' import { resetWebglTextureAtlas } from './pane-webgl-renderer' import { releaseAbandonedSynchronizedOutput } from './terminal-synchronized-output-release' +import { clearTerminalRenderModel } from './terminal-render-model-clear' function scheduleSettledFrame(callback: () => void): void { if (typeof globalThis.requestAnimationFrame !== 'function') { @@ -49,14 +50,21 @@ export function schedulePaneRevealRepaint(getPanes: () => Iterable Iterable): void { forEachPaneOnSettledFrame(getPanes, (pane) => { @@ -65,6 +73,7 @@ export function schedulePaneRevealPresent(getPanes: () => Iterable 0) { pane.terminal.refresh(0, pane.terminal.rows - 1) } diff --git a/src/renderer/src/lib/pane-manager/pane-webgl-renderer.ts b/src/renderer/src/lib/pane-manager/pane-webgl-renderer.ts index f4f625a8437..08842635066 100644 --- a/src/renderer/src/lib/pane-manager/pane-webgl-renderer.ts +++ b/src/renderer/src/lib/pane-manager/pane-webgl-renderer.ts @@ -9,6 +9,7 @@ import { } from './terminal-webgl-auto-policy' import { safeFitAndThen } from './pane-fit' import { releaseAbandonedSynchronizedOutput } from './terminal-synchronized-output-release' +import { clearTerminalRenderModel } from './terminal-render-model-clear' export const ENABLE_WEBGL_RENDERER = true let suggestedRendererType: 'dom' | undefined @@ -137,7 +138,16 @@ export function resetWebglTextureAtlas(pane: ManagedPaneInternal): void { // Why: rapid TUI redraws can corrupt xterm's WebGL glyph atlas without a // context-loss event. Clearing the atlas preserves GPU rendering and forces // a fresh paint when the pane becomes visible/focused again. - pane.webglAddon?.clearTextureAtlas() + if (pane.webglAddon) { + pane.webglAddon.clearTextureAtlas() + } else { + // Why: clearTextureAtlas is what clears the render model on the WebGL + // path. Without an addon nothing invalidates it, so the refresh below + // would diff against a model the occluded canvas no longer matches and + // skip the stale cells. Clear it explicitly so DOM-rendered panes get the + // same guaranteed full repaint. + clearTerminalRenderModel(pane.terminal) + } // Why: on reveal xterm's IntersectionObserver can still report the pane as // not intersecting, so a plain refresh() is swallowed by RenderService's // paused-render gate and the cleared model never repaints (stale bottom rows diff --git a/src/renderer/src/lib/pane-manager/terminal-render-model-clear.test.ts b/src/renderer/src/lib/pane-manager/terminal-render-model-clear.test.ts new file mode 100644 index 00000000000..8cd91f9d2aa --- /dev/null +++ b/src/renderer/src/lib/pane-manager/terminal-render-model-clear.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it, vi } from 'vitest' +import { clearTerminalRenderModel } from './terminal-render-model-clear' +import { schedulePaneRevealPresent } from './pane-reveal-repaint' +import type { ManagedPaneInternal } from './pane-manager-types' + +describe('clearTerminalRenderModel', () => { + it('clears the render model through the render service', () => { + const clear = vi.fn() + expect(clearTerminalRenderModel({ _core: { _renderService: { clear } } })).toBe(true) + expect(clear).toHaveBeenCalledTimes(1) + }) + + it('degrades to a no-op when the render service is unavailable', () => { + expect(clearTerminalRenderModel({})).toBe(false) + expect(clearTerminalRenderModel({ _core: {} })).toBe(false) + expect(clearTerminalRenderModel({ _core: { _renderService: {} } })).toBe(false) + expect(clearTerminalRenderModel(null)).toBe(false) + expect(clearTerminalRenderModel(undefined)).toBe(false) + }) + + it('reports failure instead of throwing when the pane was disposed mid-frame', () => { + const clear = vi.fn(() => { + throw new Error('disposed') + }) + expect(clearTerminalRenderModel({ _core: { _renderService: { clear } } })).toBe(false) + }) +}) + +describe('schedulePaneRevealPresent repaints a stale canvas', () => { + it('clears the model before refreshing, and leaves the shared atlas alone', () => { + // Why this ordering matters: refresh() is diff-based, so a model still + // holding the pre-hide cells makes it skip exactly the cells an occluded + // canvas lost. Clearing first turns the refresh into a full repaint. The + // atlas must NOT be cleared here — it is shared with every same-config + // pane and wiping it re-arms xterm's page-merge garble race (#4480). + const calls: string[] = [] + const clearTextureAtlas = vi.fn(() => calls.push('atlas')) + const pane = { + id: 1, + gpuRenderingEnabled: true, + webglDisabledAfterContextLoss: false, + webglAttachmentDeferred: false, + webglAddon: { clearTextureAtlas }, + terminal: { + rows: 24, + refresh: () => calls.push('refresh'), + _core: { _renderService: { clear: () => calls.push('clear-model') } } + } + } as unknown as ManagedPaneInternal + + vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => { + callback(0) + return 1 + }) + try { + schedulePaneRevealPresent(() => [pane]) + } finally { + vi.unstubAllGlobals() + } + + expect(calls).toEqual(['clear-model', 'refresh']) + expect( + clearTextureAtlas, + 'the shared glyph atlas must survive a refocus' + ).not.toHaveBeenCalled() + }) +}) diff --git a/src/renderer/src/lib/pane-manager/terminal-render-model-clear.ts b/src/renderer/src/lib/pane-manager/terminal-render-model-clear.ts new file mode 100644 index 00000000000..abcd417ac5e --- /dev/null +++ b/src/renderer/src/lib/pane-manager/terminal-render-model-clear.ts @@ -0,0 +1,55 @@ +/** + * Clears a terminal's per-cell render model WITHOUT touching the shared glyph + * atlas, so the next refresh rebuilds every visible cell from the buffer. + * + * Why this is needed: xterm's renderers are diff-based. `terminal.refresh()` + * walks the buffer and skips any cell whose code/fg/bg/ext match the render + * model's cached copy (WebglRenderer._updateModel's "Nothing has changed, no + * updates needed" early-continue). While a pane is occluded — the window hidden + * behind another app, the classic "switch away to the desktop" — the compositor + * can drop the canvas contents while that model stays populated. On return the + * diff then reports those cells unchanged, the refresh skips them, and the + * canvas keeps presenting pre-hide pixels until something forces a full + * rebuild. Resizing the window is what users find, because a resize reallocates + * the model and repaints everything. + * + * `RenderService.clear()` delegates to the active renderer's `clear()`, which + * calls `_clearModel(true)` — it drops the cached cells and the glyph renderer's + * vertices but does NOT clear the texture atlas. That distinction matters: the + * atlas is shared by every same-config terminal, and wiping it mid-stream + * re-arms xterm's page-merge garble race (xterm.js #4480), which is why the + * plain-refocus path is deliberately atlas-preserving. Clearing only the model + * gives a guaranteed full repaint at no cost to sibling panes. + * + * All access is behind typeof guards: an xterm upgrade that renames these + * internals degrades to a no-op (the caller's plain refresh still runs) rather + * than throwing into a render frame. + */ + +type MaybeClearableRenderService = { + clear?: () => void +} + +type TerminalWithRenderService = { + _core?: { + _renderService?: MaybeClearableRenderService + } +} + +/** + * Drops the cached render model so the caller's next refresh repaints every + * visible cell. Returns whether the model was actually cleared. + */ +export function clearTerminalRenderModel(terminal: unknown): boolean { + const service = (terminal as TerminalWithRenderService | null)?._core?._renderService + if (typeof service?.clear !== 'function') { + return false + } + try { + service.clear() + return true + } catch { + // A pane disposed mid-frame must not break the caller's repaint. + return false + } +} diff --git a/tests/e2e/terminal-opencode-altscreen-reveal-artifacts.spec.ts b/tests/e2e/terminal-opencode-altscreen-reveal-artifacts.spec.ts index 781f11c6cec..f01fd0651c1 100644 --- a/tests/e2e/terminal-opencode-altscreen-reveal-artifacts.spec.ts +++ b/tests/e2e/terminal-opencode-altscreen-reveal-artifacts.spec.ts @@ -1,22 +1,40 @@ /** - * STA-2694 repro: switching away from a workspace running an alt-screen AI TUI - * (OpenCode/OpenTUI, also Claude Code and grok) and returning later shows a - * garbled/distorted terminal that only a manual window resize repairs. + * STA-2694 repro attempt: switching away from a workspace running an alt-screen + * AI TUI (OpenCode/OpenTUI, also Claude Code and grok) and returning later shows + * a garbled/distorted terminal that only a manual window resize repairs. * - * Why a new spec next to terminal-inline-tui-reveal-convergence.spec.ts: that - * one drives the INLINE shape (normal buffer, live block glued to the bottom, - * history scrolling into scrollback). OpenCode runs FULL-SCREEN on the - * alternate buffer and repaints absolutely-positioned rows — nothing scrolls, - * so no row ever self-heals through the scroll path, and a row that paints - * wrong stays wrong until a resize rebuilds the render model. The inline spec's - * assertions (viewport anchored to baseY, input-box row present, frame - * advancing) all pass while the canvas is visibly garbled, because they read - * xterm's BUFFER. This spec asserts the PIXELS. + * Why this spec exists next to terminal-inline-tui-reveal-convergence.spec.ts: + * that one drives the INLINE shape (normal buffer, live block glued to the + * bottom, history scrolling into scrollback). OpenCode runs FULL-SCREEN on the + * alternate buffer and repaints absolutely-positioned rows — nothing scrolls, so + * no row ever self-heals through the scroll path. These tests cover that shape + * across the hide/reveal boundaries (worktree switch, cold park, idle agent, + * desktop hide) and guard the convergence properties we CAN observe: the buffer + * converges to the live frame, the pane stays on the alt screen, and xterm's + * grid, the fit proposal, and the PTY-applied size all agree without a resize. * - * The decisive assertion is resize-referenced: capture the revealed pane, then - * force the repair the user performs by hand (a window resize), and require the - * revealed pixels to already match the repaired pixels. "A resize fixes it" is - * the literal user report, so "identical to post-resize" is the literal fix. + * ⚠ KNOWN LIMITATION — these tests do NOT detect a stale/garbled CANVAS. + * + * The field defect is "xterm's buffer is correct but the compositor keeps + * showing pre-hide pixels". Two oracles were built for it and both were proven + * blind by injecting that exact defect (freeze RenderService.refreshRows, then + * write new content, so the buffer advances while the canvas cannot): + * + * 1. Canvas-vs-buffer ink sampling (the render-desync sentinel's method). + * `drawImage` on a non-preserveDrawingBuffer WebGL canvas hands back a + * re-rendered copy, so it reported 0 missing cells against 5263 cells of + * text the canvas had never drawn. + * 2. Screenshot comparison against a forced repaint. Playwright's screenshot + * drives a fresh compositor frame, which HEALS the stale paint before it + * is captured; and the "repair" calls the same repaint code the reveal + * already ran, so a defect shared by both shots cancels out. + * + * So a green run here means "no buffer/geometry regression", NOT "the canvas + * painted correctly". Confirming a canvas-level fix needs the in-app + * render-desync sentinel on real hardware: + * localStorage.setItem('orca:render-desync-sentinel', '1') // then reload + * which samples the presented canvas before any forced redraw can heal it, and + * writes the corrupt pixels + buffer to app data on a confirmed trip. */ import { readFileSync, writeFileSync } from 'node:fs' import path from 'node:path' @@ -598,6 +616,119 @@ test.describe('OpenCode alt-screen reveal artifacts (STA-2694)', () => { } }) + // The field condition the other tests miss: the agent is IDLE when you come + // back. Every test above keeps the TUI streaming across the reveal, so live + // frames repaint whatever the reveal got wrong — the defect heals itself + // before any assertion runs. A real OpenCode session sits waiting for input, + // so nothing arrives to heal it, and whatever the reveal painted is what the + // user stares at until they resize the window. + test('parked reveal of an IDLE alt-screen agent paints without a manual repair', async ({ + orcaPage + }, testInfo) => { + test.setTimeout(240_000) + const setup = await startStreamingAltScreenTui(orcaPage, testInfo) + try { + test.skip( + !(await hasWebglPane(orcaPage, setup.tabId)), + 'WebGL renderer unavailable in this environment' + ) + // Go idle BEFORE hiding, cleanly between brackets — a settled agent + // holding its last full-screen frame. + await sendToTerminal(orcaPage, setup.ptyId, 'ORCA_FREEZE_NOW').catch(() => {}) + await orcaPage.waitForTimeout(2_000) + const idleFrame = latestMatch( + (await probeRevealedPane(orcaPage, setup.tabId))?.screenRows.join('\n') ?? '', + FRAME_RE + ) + expect(idleFrame, 'fixture never painted a frame before going idle').toBeGreaterThan(0) + + // Cold-park the idle tab: renderer torn down, so the reveal must restore + // and repaint entirely from the snapshot with no live output to help. + await createActiveTerminalTab(orcaPage, setup.worktreeId) + await createActiveTerminalTab(orcaPage, setup.worktreeId) + await waitForTabParked(orcaPage, setup.tabId, { parkDelayMs: PARKING_DELAY_MS }) + await orcaPage.waitForTimeout(3_000) + + await withCpuThrottle(orcaPage, 6, async () => { + await activateTerminalTab(orcaPage, setup.tabId) + await waitForActiveTerminalManager(orcaPage, 30_000) + await orcaPage.waitForTimeout(3_000) + }) + + // The same frame must still be on screen — the agent produced nothing new. + const revealedProbe = await probeRevealedPane(orcaPage, setup.tabId) + expect( + revealedProbe?.bufferType, + `idle-parked-reveal: pane is not on the alt screen: ${describeProbe(revealedProbe)}` + ).toBe('alternate') + expect( + latestMatch(revealedProbe?.screenRows.join('\n') ?? '', FRAME_RE), + `idle-parked-reveal: idle frame lost across the park: ${describeProbe(revealedProbe)}` + ).toBeGreaterThan(0) + + await assertRevealPixelsNeedNoRepair(orcaPage, testInfo, setup, 'idle-parked-reveal') + } finally { + await setup.stop() + } + }) + + // The LITERAL user action: "switch away to the desktop". That is an OS-level + // window hide/occlusion, not in-app navigation — it flips + // document.visibilityState, releases the WebGL context, and comes back + // through the window-wake recovery path rather than the worktree-reveal path. + // The pane never unmounts and its worktree never changes, so none of the + // reveal-repaint machinery the other tests exercise even runs. + test('@headful desktop switch away and back paints an idle agent without a manual repair', async ({ + orcaPage, + electronApp + }, testInfo) => { + test.setTimeout(240_000) + const setup = await startStreamingAltScreenTui(orcaPage, testInfo) + try { + test.skip( + !(await hasWebglPane(orcaPage, setup.tabId)), + 'WebGL renderer unavailable in this environment' + ) + // Idle agent holding a full-screen frame — the field state on return. + await sendToTerminal(orcaPage, setup.ptyId, 'ORCA_FREEZE_NOW').catch(() => {}) + await orcaPage.waitForTimeout(2_000) + + // Switch away to the desktop: hide the window entirely. + await electronApp.evaluate(({ BrowserWindow }) => { + const window = BrowserWindow.getAllWindows()[0] + if (!window) { + throw new Error('No Electron window') + } + window.hide() + }) + await orcaPage.waitForTimeout(4_000) + + // ...and come back to it. + await electronApp.evaluate(({ BrowserWindow }) => { + const window = BrowserWindow.getAllWindows()[0] + if (!window) { + throw new Error('No Electron window') + } + window.show() + window.focus() + }) + await orcaPage.waitForTimeout(3_000) + + const revealedProbe = await probeRevealedPane(orcaPage, setup.tabId) + expect( + revealedProbe?.bufferType, + `desktop-return: pane is not on the alt screen: ${describeProbe(revealedProbe)}` + ).toBe('alternate') + + await assertRevealPixelsNeedNoRepair(orcaPage, testInfo, setup, 'desktop-return') + } finally { + await electronApp + .evaluate(({ BrowserWindow }) => BrowserWindow.getAllWindows()[0]?.show()) + .catch(() => {}) + await setup.stop() + } + }) + // The mechanism behind STA-2694: OpenTUI-style TUIs bracket every repaint in // `?2026h … ?2026l`, so a hide can land inside an open bracket. xterm then // keeps synchronizedOutput latched, and RenderService checks that latch