From 8d5eacecb484ff2015b156409e7c781efd39cdee Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 27 Jul 2026 03:03:41 -0700 Subject: [PATCH] Revert "fix(terminal): clear the render model on the plain-refocus repaint path" This reverts commit 0f7ec4458d37010338f16e70ff06957cb335e074. --- .../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, 24 insertions(+), 296 deletions(-) delete mode 100644 src/renderer/src/lib/pane-manager/terminal-render-model-clear.test.ts delete 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 c36aa87fa51..9aa24002b6d 100644 --- a/src/renderer/src/lib/pane-manager/pane-reveal-repaint.ts +++ b/src/renderer/src/lib/pane-manager/pane-reveal-repaint.ts @@ -2,7 +2,6 @@ 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') { @@ -50,21 +49,14 @@ export function schedulePaneRevealRepaint(getPanes: () => Iterable Iterable): void { forEachPaneOnSettledFrame(getPanes, (pane) => { @@ -73,7 +65,6 @@ 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 08842635066..f4f625a8437 100644 --- a/src/renderer/src/lib/pane-manager/pane-webgl-renderer.ts +++ b/src/renderer/src/lib/pane-manager/pane-webgl-renderer.ts @@ -9,7 +9,6 @@ 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 @@ -138,16 +137,7 @@ 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. - 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) - } + pane.webglAddon?.clearTextureAtlas() // 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 deleted file mode 100644 index 8cd91f9d2aa..00000000000 --- a/src/renderer/src/lib/pane-manager/terminal-render-model-clear.test.ts +++ /dev/null @@ -1,67 +0,0 @@ -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 deleted file mode 100644 index abcd417ac5e..00000000000 --- a/src/renderer/src/lib/pane-manager/terminal-render-model-clear.ts +++ /dev/null @@ -1,55 +0,0 @@ -/** - * 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 f01fd0651c1..781f11c6cec 100644 --- a/tests/e2e/terminal-opencode-altscreen-reveal-artifacts.spec.ts +++ b/tests/e2e/terminal-opencode-altscreen-reveal-artifacts.spec.ts @@ -1,40 +1,22 @@ /** - * 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. + * 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. * - * 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. + * 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. * - * ⚠ 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. + * 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. */ import { readFileSync, writeFileSync } from 'node:fs' import path from 'node:path' @@ -616,119 +598,6 @@ 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