diff --git a/src/renderer/src/lib/pane-manager/pane-reveal-fit.ts b/src/renderer/src/lib/pane-manager/pane-reveal-fit.ts index 759d9dacab7..a59a5337329 100644 --- a/src/renderer/src/lib/pane-manager/pane-reveal-fit.ts +++ b/src/renderer/src/lib/pane-manager/pane-reveal-fit.ts @@ -39,11 +39,11 @@ function proposedGridMatchesTerminal(pane: ManagedPane): boolean { } } -function releaseMeasurableFitContinuations(pane: ManagedPane): void { +function releaseMeasurableFitContinuations(pane: ManagedPane, measurable: boolean): void { // Why: no reflow needed, but a pane that mounted hidden can have replay/reattach // continuations parked on a measurable fit — release them (and any parked scroll // restore, mirroring safeFit's equal-dims path) now it is visible. - if (!canMeasurePaneForFit(pane)) { + if (!measurable) { return } resumePendingFitScrollRestoreAfterFit(pane.terminal) @@ -63,10 +63,12 @@ function releaseMeasurableFitContinuations(pane: ManagedPane): void { // mismatch refits but a transient metric wobble does not reflow; // - grid already correct → leave it alone. export function fitRevealedPane(pane: ManagedPane): void { + // Hoisted: the measurability probe forces style+layout, so read it once per reveal. + const measurable = canMeasurePaneForFit(pane) // Why before the fit, not after: while the render service is latched, every // resize below parks the renderer half and composites the pre-hide backing // store into the new box. Measurable only — a hidden pane must stay paused. - if (canMeasurePaneForFit(pane)) { + if (measurable) { releaseRenderPauseLatch(pane.terminal) } // Why first: the checks below can both say "nothing to do" and return without @@ -83,5 +85,5 @@ export function fitRevealedPane(pane: ManagedPane): void { requestStablePaneFit(pane) return } - releaseMeasurableFitContinuations(pane) + releaseMeasurableFitContinuations(pane, measurable) } diff --git a/src/renderer/src/lib/pane-manager/terminal-render-pause-release.ts b/src/renderer/src/lib/pane-manager/terminal-render-pause-release.ts index e83642604cd..8f395eeec38 100644 --- a/src/renderer/src/lib/pane-manager/terminal-render-pause-release.ts +++ b/src/renderer/src/lib/pane-manager/terminal-render-pause-release.ts @@ -20,6 +20,7 @@ type MaybePausableRenderService = { _isPaused?: boolean _needsFullRefresh?: boolean + _pausedResizeTask?: { flush?: () => void } refreshRows?: (start: number, end: number, sync?: boolean) => void } @@ -61,7 +62,17 @@ export function releaseRenderPauseLatch(terminal: unknown): boolean { if (!service || service._isPaused !== true) { return false } - service._isPaused = false + try { + // Why flush first: a resize parked while paused would otherwise replay on + // its own idle callback after the fit and trim the DOM renderer's row + // elements to stale args (paused-path `set()` used to overwrite it; the + // inline path no longer does). Replayed now, the fit's resize wins. + service._pausedResizeTask?.flush?.() + service._isPaused = false + } catch { + // Never throw into a render frame; a disposed renderer stays latched. + return false + } return true } @@ -83,10 +94,13 @@ export function forceRepaintThroughRenderPause(terminal: unknown): boolean { return false } - // Why: leave the latch as if the pending full refresh was serviced — we are - // about to service it — so the observer's next callback doesn't queue a - // redundant second full repaint. - service._isPaused = false + // Why: releaseRenderPauseLatch also flushes the parked renderer resize; then + // leave the latch as if the pending full refresh was serviced — we are about + // to service it — so the observer's next callback doesn't queue a redundant + // second full repaint. + if (!releaseRenderPauseLatch(terminal)) { + return false + } service._needsFullRefresh = false try { service.refreshRows(0, rows - 1, true) diff --git a/tests/tools/terminal-reveal-raster-analysis.mjs b/tests/tools/terminal-reveal-raster-analysis.mjs index eee5857686c..a8f66274998 100644 --- a/tests/tools/terminal-reveal-raster-analysis.mjs +++ b/tests/tools/terminal-reveal-raster-analysis.mjs @@ -106,8 +106,26 @@ export function compareScaledRevealRaster(baselineBuffer, candidateBuffer) { } } +const baselineAnalysisCache = new WeakMap() + +// The reference crop is compared against every captured frame; decode it and +// derive its frame-invariant metrics once per buffer instead of once per frame. +function baselineAnalysis(baselineBuffer) { + let cached = baselineAnalysisCache.get(baselineBuffer) + if (!cached) { + const image = PNG.sync.read(baselineBuffer) + cached = { image, background: dominantBackground(image), edgeEnergy: edgeEnergy(image) } + baselineAnalysisCache.set(baselineBuffer, cached) + } + return cached +} + export function compareRevealRaster(baselineBuffer, candidateBuffer) { - const baseline = PNG.sync.read(baselineBuffer) + const { + image: baseline, + background, + edgeEnergy: baselineEdgeEnergy + } = baselineAnalysis(baselineBuffer) const candidate = PNG.sync.read(candidateBuffer) if (baseline.width !== candidate.width || baseline.height !== candidate.height) { return { @@ -119,7 +137,6 @@ export function compareRevealRaster(baselineBuffer, candidateBuffer) { } } - const background = dominantBackground(baseline) let diffPixels = 0 let baselineInkPixels = 0 let candidateInkPixels = 0 @@ -158,7 +175,6 @@ export function compareRevealRaster(baselineBuffer, candidateBuffer) { } } const pixelCount = baseline.width * baseline.height - const baselineEdgeEnergy = edgeEnergy(baseline) const candidateEdgeEnergy = edgeEnergy(candidate) return { comparable: true, @@ -236,11 +252,12 @@ export function measureRasterInk(buffer) { } } const pixels = image.width * image.height + const energy = edgeEnergy(image) return { inkPixels, inkRatio: pixels ? inkPixels / pixels : 0, - edgeEnergy: edgeEnergy(image), - edgePerPixel: pixels ? edgeEnergy(image) / pixels : 0, + edgeEnergy: energy, + edgePerPixel: pixels ? energy / pixels : 0, pixels } } diff --git a/tests/tools/terminal-reveal-raster-arms.mjs b/tests/tools/terminal-reveal-raster-arms.mjs index 6a215009aae..f0c4c48cec2 100644 --- a/tests/tools/terminal-reveal-raster-arms.mjs +++ b/tests/tools/terminal-reveal-raster-arms.mjs @@ -35,7 +35,8 @@ async function ensureTargetVisible(page, context) { async function prepareAttempt(page, context) { await ensureTargetVisible(page, context) - const stable = await captureStableReference(page, context.targetTabId, 2) + // Only the settled state is consumed here; skip the full-page screenshots. + const stable = await captureStableReference(page, context.targetTabId, 2, { screenshots: false }) return stable.at(-1).state } diff --git a/tests/tools/terminal-reveal-raster-attempt.mjs b/tests/tools/terminal-reveal-raster-attempt.mjs index 2c16e252627..dc2ead9329d 100644 --- a/tests/tools/terminal-reveal-raster-attempt.mjs +++ b/tests/tools/terminal-reveal-raster-attempt.mjs @@ -200,12 +200,18 @@ function measureRevealSettle(capture, stableState) { } function prepareReference(stableCaptures) { - const prepared = stableCaptures.map((entry) => ({ - ...entry, - crop: entry.state?.screenClip - ? cropTerminalRaster(entry.buffer, entry.state.screenClip, entry.viewport) - : null - })) + const prepared = stableCaptures.map((entry) => { + let crop = null + if (entry.state?.screenClip) { + try { + crop = cropTerminalRaster(entry.buffer, entry.state.screenClip, entry.viewport) + } catch { + // Recorded below as crop-failed: one bad capture scores invalid-run, + // it must not abort the whole campaign. + } + } + return { ...entry, crop } + }) const reference = prepared.at(-1) const stableState = reference?.state const noise = [] @@ -246,7 +252,10 @@ function prepareReference(stableCaptures) { function writeAttemptEvidence(directory, result, capture, prepared) { mkdirSync(directory, { recursive: true }) - writeFileSync(path.join(directory, 'stable.png'), prepared.at(-1).crop) + const referenceCrop = prepared.at(-1)?.crop + if (referenceCrop) { + writeFileSync(path.join(directory, 'stable.png'), referenceCrop) + } writeFileSync( path.join(directory, 'state.jsonl'), `${capture.states.map((state) => JSON.stringify(state)).join('\n')}\n` diff --git a/tests/tools/terminal-reveal-raster-fixture.mjs b/tests/tools/terminal-reveal-raster-fixture.mjs index 661740f97f8..102243e6d38 100644 --- a/tests/tools/terminal-reveal-raster-fixture.mjs +++ b/tests/tools/terminal-reveal-raster-fixture.mjs @@ -64,13 +64,10 @@ function fixtureOutput(mode) { const SYNC_TUI_REDRAW_MS = 350 // One BSU/ESU pair per frame, the way an agent TUI actually draws: sync is held -// only for the write, never between frames. An earlier revision re-asserted BSU -// on a 40 ms heartbeat to guarantee the mode spanned a reveal; that made the -// hold effectively permanent, and a terminal that never paints while DEC 2026 is -// held is spec-correct, not broken — it produced durable "blank" panes on a clean -// tree and would have been reported as a product defect. The reveal-time overlap -// is forced deliberately instead, by the probe's holdSync, which this cadence -// then releases within one frame. +// only for the write, never between frames. Do not re-assert BSU on a heartbeat +// to span reveals — a permanent hold is spec-correct "never paint" and reads as +// a durable blank-pane product defect on a clean tree. Reveal-time overlap is +// forced by the probe's holdSync instead, which this cadence releases in one frame. function syncTuiScript() { const lines = Array.from({ length: 3 }, () => READABLE_LINES).flat() return [ diff --git a/tests/tools/terminal-reveal-raster-repro.mjs b/tests/tools/terminal-reveal-raster-repro.mjs index 5a26cee83ae..ce03cdaf6e1 100644 --- a/tests/tools/terminal-reveal-raster-repro.mjs +++ b/tests/tools/terminal-reveal-raster-repro.mjs @@ -35,6 +35,13 @@ const PARK_DELAY_MS = 1_500 const DEFAULT_ARMS = ['control', 'tab', 'warm', 'cold'] const FIXTURE_MODES = ['static', 'atlas', 'sync-tui'] +// `Number(value) || fallback` would silently turn an explicit 0 (and typos) +// into the fallback; clamp real numbers, reject the rest. +function numericFlag(value, minimum, fallback) { + const parsed = Number(value) + return Number.isFinite(parsed) ? Math.max(minimum, parsed) : fallback +} + function parseArgs() { const args = { arms: DEFAULT_ARMS, @@ -66,27 +73,35 @@ function parseArgs() { args.noHoldSync = true continue } - const [name, value] = arg.split('=', 2) + // split-once: `split('=', 2)` would truncate values containing '='. + const separator = arg.indexOf('=') + const name = separator === -1 ? arg : arg.slice(0, separator) + const value = separator === -1 ? undefined : arg.slice(separator + 1) if (name === '--arms') { - args.arms = (value ?? '') + const requested = (value ?? '') .split(',') .map((item) => item.trim()) - .filter((item) => ARM_NAMES.includes(item)) + .filter(Boolean) + const unknown = requested.filter((item) => !ARM_NAMES.includes(item)) + if (unknown.length > 0) { + throw new Error(`Unknown arms ${unknown.join(', ')}; expected ${ARM_NAMES.join(', ')}`) + } + args.arms = requested } else if (name === '--cycles') { - args.cycles = Math.max(1, Number(value) || 3) + args.cycles = numericFlag(value, 1, 3) } else if (name === '--pressure-tabs') { - args.pressureTabs = Math.max(1, Number(value) || 18) + args.pressureTabs = numericFlag(value, 1, 18) } else if (name === '--pressure-splits') { - args.pressureSplits = Math.max(0, Number(value) || 0) + args.pressureSplits = numericFlag(value, 0, 0) } else if (name === '--capture-ms') { - args.captureMs = Math.max(750, Number(value) || 2_200) + args.captureMs = numericFlag(value, 750, 2_200) } else if (name === '--fixture') { if (!FIXTURE_MODES.includes(value)) { throw new Error(`Unknown fixture ${value}; expected one of ${FIXTURE_MODES.join(', ')}`) } args.fixture = value } else if (name === '--inject-blank-ms') { - args.injectBlankMs = Math.max(0, Number(value) || 0) + args.injectBlankMs = numericFlag(value, 0, 0) } else if (name === '--output') { args.output = value || null } diff --git a/tests/tools/terminal-reveal-raster-session.mjs b/tests/tools/terminal-reveal-raster-session.mjs index 8f5ecd7c00c..ce382ed59c2 100644 --- a/tests/tools/terminal-reveal-raster-session.mjs +++ b/tests/tools/terminal-reveal-raster-session.mjs @@ -412,12 +412,6 @@ export async function installRevealRasterProbe(page) { pinPty(ptyId) { preferredPtyId = ptyId ?? null }, - // Orca stops feeding the pty while a pane is hidden, so a fixture that - // holds DEC 2026 cannot get its BSU across the reveal boundary — measured: - // synchronizedOutput is false for the first ~200 ms of every reveal. Set - // the mode directly on the hidden terminal to reproduce the state an agent - // TUI is actually in when the reveal fit runs; the fixture's next ESU - // releases it exactly as a real TUI frame flush would. // Fault injection, not a reproduction: replays the reverted commit's // timing-free blank shape (bitmap cleared, repaint swallowed, xterm's // recovery zeroed) so the blank-pane measurement, thresholds and plumbing @@ -459,6 +453,12 @@ export async function installRevealRasterProbe(page) { blankFaultFiredAt() { return blankFaultFiredAt }, + // Orca stops feeding the pty while a pane is hidden, so a fixture that + // holds DEC 2026 cannot get its BSU across the reveal boundary — measured: + // synchronizedOutput is false for the first ~200 ms of every reveal. Set + // the mode directly on the hidden terminal to reproduce the state an agent + // TUI is actually in when the reveal fit runs; the fixture's next ESU + // releases it exactly as a real TUI frame flush would. holdSync(tabId) { const terminal = pickPane(window.__paneManagers?.get(tabId))?.terminal const modes = terminal?._core?.coreService?.decPrivateModes @@ -501,13 +501,15 @@ export async function captureCompositorTransition(page, tabId, captureMs, action const frames = [] const cdp = await page.context().newCDPSession(page) cdp.on('Page.screencastFrame', (params) => { - const receiptAt = Date.now() - frames.push({ - buffer: Buffer.from(params.data, 'base64'), - receiptAt, - capturedAt: frameTimestamp(params, receiptAt), - metadata: params.metadata - }) + // Capped in the handler so a long capture cannot hold hundreds of PNGs. + if (frames.length < 400) { + const receiptAt = Date.now() + frames.push({ + buffer: Buffer.from(params.data, 'base64'), + receiptAt, + capturedAt: frameTimestamp(params, receiptAt) + }) + } void cdp.send('Page.screencastFrameAck', { sessionId: params.sessionId }).catch(() => {}) }) await page.evaluate((id) => window.__terminalRevealRasterProbe.start(id), tabId) @@ -520,30 +522,31 @@ export async function captureCompositorTransition(page, tabId, captureMs, action maxHeight: Math.ceil(viewport.height * viewport.dpr) }) await page.waitForTimeout(50) - let actionAt = Date.now() + const actionAt = Date.now() + let states = [] try { - // Arms that spend time off-screen re-stamp the action once the reveal - // itself starts, so the hidden gap is not scored as reveal latency. - await action({ - mark: () => { - actionAt = Date.now() - } - }) + await action() await page.waitForTimeout(captureMs) } finally { + // A throwing action must still stop the screencast, the probe's rAF + // sampler (and its pump element), and the CDP session. await cdp.send('Page.stopScreencast').catch(() => {}) + states = await page.evaluate(() => window.__terminalRevealRasterProbe.stop()).catch(() => []) + await cdp.detach().catch(() => {}) } - const states = await page.evaluate(() => window.__terminalRevealRasterProbe.stop()) - await cdp.detach().catch(() => {}) return { actionAt, - captureViewport: viewport, - frames: frames.slice(0, 400), + frames, states } } -export async function captureStableReference(page, tabId, samples = 3) { +export async function captureStableReference( + page, + tabId, + samples = 3, + { screenshots = true } = {} +) { const captures = [] for (let index = 0; index < samples; index += 1) { // Late repairs (DPR, deferred fits) can still move geometry a few frames @@ -551,12 +554,14 @@ export async function captureStableReference(page, tabId, samples = 3) { await page.waitForTimeout(200) await settleFrames(page, 3) const state = await readTargetState(page, tabId) - const buffer = await page.screenshot() - const viewport = await page.evaluate(() => ({ - width: innerWidth, - height: innerHeight - })) - captures.push({ state, buffer, viewport }) + // Skipping screenshots lets settle-only callers avoid full-page captures; + // viewport comes from the same probe sample instead of a second evaluate. + const buffer = screenshots ? await page.screenshot() : null + captures.push({ + state, + buffer, + viewport: { width: state?.innerWidth ?? 0, height: state?.innerHeight ?? 0 } + }) } return captures }