From a5bc8f9692a854f6e8ba627b01cbb5d0b3ea64cc Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 8 Aug 2026 17:45:17 -0700 Subject: [PATCH 1/3] Make the landing snow actually render on an idle screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three independent gates kept the falling-snow decoration over the landing mountain from ever drawing: the mount-time paint was still=true, which the old snowOn check tied directly to snow visibility; paintLanding early-returned whenever it wasn't re-entered with animating=true, which never happens while idle; and its only caller was the turn monitor, which deliberately stops ticking once idle. Fix: snow visibility no longer depends on `still` (that flag now only freezes the mountain's own draw/fill/fade timeline, not the flakes over it), the early-return is gone so idle repaints actually happen, and the shell's createAppShell now repaints the landing off the renderer's own FRAME event whenever the landing is up and not mid-turn-animation. That event is already scoped to shell lifetime (wired at construction, unwired in dispose) and paintLanding already no-ops once the landing tears down, so this needed no new timer to arm or leak, and it doesn't touch the turn monitor's cadence at all. The alternative — a separate timer armed from createLandingAbove and stopped in the landing teardown — was rejected: it would duplicate the monitor's own cadence-management responsibility for no real gain, since the FRAME event already has the right lifetime. Added a real-mount-path regression test in landing.test.ts that goes through createAppShell and lets the renderer's FRAME event drive the repaint, unlike every existing test in that file, which drives the mark by calling paintLanding directly with a hand-picked clock. That gap is exactly why this shipped broken and nobody caught it. This supersedes PR #380, which added the snow-drawing code but never made it reachable; that PR should stay open until this one is reviewed and can then be closed in favor of this one. --- src/tui-opentui/landing.test.ts | 51 +++++++++++++++++++++++++++++-- src/tui-opentui/mark-anim.test.ts | 38 +++++++++++++++++------ src/tui-opentui/mark-anim.ts | 20 ++++++++---- src/tui-opentui/shell.ts | 30 +++++++++++++----- 4 files changed, 115 insertions(+), 24 deletions(-) diff --git a/src/tui-opentui/landing.test.ts b/src/tui-opentui/landing.test.ts index a5f3430b8..8ee3c34f7 100644 --- a/src/tui-opentui/landing.test.ts +++ b/src/tui-opentui/landing.test.ts @@ -38,6 +38,7 @@ import { import { LOCKUP_WORDMARK } from "./lockup" import pkg from "../../package.json" with { type: "json" } import { MARK_LARGE, MARK_MID, MARK_SMALL } from "./mark-shape" +import { SNOW_CHAR } from "./mark-anim" import { UI } from "./theme" const SIZE = { width: 80, height: 24 } as const @@ -229,11 +230,14 @@ describe("landing screen", () => { try { await settle(h) const still = markRows(h).join("\n") + const stripSnow = (text: string) => text.replaceAll(SNOW_CHAR, " ") - // Idle re-entry holds the filled frame however far the clock moves. + // Idle re-entry holds the mountain's filled frame however far the + // clock moves — but the snow drifting over it is not still, since the + // idle landing screen is exactly where it needs to animate. paintLanding(shell, 1_700, false) await settle(h) - expect(markRows(h).join("\n")).toBe(still) + expect(stripSnow(markRows(h).join("\n"))).toBe(stripSnow(still)) const frames = new Set() for (const nowMs of [0, 500, 1_100, 1_900, 2_600, 3_400]) { @@ -248,6 +252,49 @@ describe("landing screen", () => { }, SIZE) }) + test( + "an idle mount keeps the snow drifting on the renderer's own frame event", + async () => { + // Regression for CL-5737: every other test in this file drives the mark + // by calling `paintLanding` directly with a hand-picked clock. That is + // exactly why the landing snow shipped completely unreachable — none of + // those tests go through the real driver a running session actually + // uses. This one mounts the shell for real and lets the renderer's own + // FRAME event (wired in `createAppShell`, unwired in `shell.dispose`) + // drive the repaint, the same as production, with no direct calls to + // `paintLanding` or `renderMark`. + await withTestRenderer(async (h) => { + const shell = createAppShell(h.renderer, { + terminal: { columns: 80, rows: 24 }, + wireKeys: false, + run: "idle", + }) + try { + await settle(h) + const before = markRows(h).join("\n") + + // Real elapsed time, not an injected clock: the production driver + // reads the wall clock, so this is the only way to exercise it. + // At the fall speed in mark-anim.ts a few real seconds is enough + // for at least one active flake column to cross a row boundary. + const start = Date.now() + while (Date.now() - start < 5_000) { + await h.renderOnce() + } + const after = markRows(h).join("\n") + + expect(after).not.toBe(before) + + const stripSnow = (text: string) => text.replaceAll(SNOW_CHAR, " ") + expect(stripSnow(after)).toBe(stripSnow(before)) + } finally { + shell.dispose() + } + }, SIZE) + }, + 15_000, + ) + test("a starter key fills the prompt; a typed prompt keeps its digits", async () => { await withTestRenderer(async (h) => { const shell = createAppShell(h.renderer, { diff --git a/src/tui-opentui/mark-anim.test.ts b/src/tui-opentui/mark-anim.test.ts index cb321f7f5..e634e2a9d 100644 --- a/src/tui-opentui/mark-anim.test.ts +++ b/src/tui-opentui/mark-anim.test.ts @@ -106,19 +106,38 @@ describe("renderMark", () => { const grid = renderMark({ nowMs: 0, still: true, grid: MARK_LARGE }) grid.forEach((row, y) => { row.forEach((cell, x) => { - // Still mode has no snow — only space or mountain blocks. - expect(` ${MOUNTAIN_CHARS}`).toContain(cell.char) + // Still mode freezes the mountain, but snow still drifts over the sky. + expect(` ${MOUNTAIN_CHARS}${SNOW_CHAR}`).toContain(cell.char) if ((MARK_LARGE.coverage[y]?.[x] ?? 0) === 1) expect(cell.char).toBe("█") }) }) }) - test("the still frame is clock-independent and has no snow", () => { - const a = markText(renderMark({ nowMs: 0, still: true })) - const b = markText(renderMark({ nowMs: 987_654, still: true })) + test("still holds the mountain fixed while the clock advances", () => { + // Snow moves with the clock even in still mode (the idle landing screen), + // so isolate the mountain by stripping snow before comparing. + const stripSnow = (text: string) => text.replaceAll(SNOW_CHAR, " ") + const a = stripSnow(markText(renderMark({ nowMs: 0, still: true }))) + const b = stripSnow(markText(renderMark({ nowMs: 987_654, still: true }))) expect(b).toBe(a) expect(a.replace(/[\s\n]/g, "").length).toBeGreaterThan(0) - expect(a.includes(SNOW_CHAR)).toBe(false) + }) + + test("snow keeps drifting in still mode while the mountain stays frozen", () => { + const times = [0, 1500, 3000, 4500, 6000, 7500] + const snowSets = times.map((nowMs) => { + const grid = renderMark({ nowMs, still: true, grid: MARK_LARGE }) + const snow: string[] = [] + grid.forEach((row, y) => { + row.forEach((cell, x) => { + if (isSnow(cell.char)) snow.push(`${y},${x}`) + }) + }) + return snow.join("|") + }) + const withSnow = snowSets.filter((s) => s.length > 0) + expect(withSnow.length).toBeGreaterThan(1) + expect(new Set(withSnow).size).toBeGreaterThan(1) }) test("the animated frame advances with the injected clock", () => { @@ -203,11 +222,12 @@ describe("renderMark", () => { expect(mountains).toBeGreaterThan(flakes) }) - test("still mode freezes the mark with no snow motion", () => { + test("still mode freezes the mountain but not the snow", () => { const a = renderMark({ nowMs: 0, still: true, grid: MARK_SMALL }) const b = renderMark({ nowMs: 50_000, still: true, grid: MARK_SMALL }) - expect(markText(b)).toBe(markText(a)) - expect(a.flat().some((cell) => isSnow(cell.char))).toBe(false) + const mountainText = (grid: typeof a) => + grid.map((row) => row.map((cell) => (isMountain(cell.char) ? cell.char : " ")).join("")).join("\n") + expect(mountainText(b)).toBe(mountainText(a)) }) test("snow drops out during the fade-out phase, matching the mark", () => { diff --git a/src/tui-opentui/mark-anim.ts b/src/tui-opentui/mark-anim.ts index a3a62dafe..6f7a60249 100644 --- a/src/tui-opentui/mark-anim.ts +++ b/src/tui-opentui/mark-anim.ts @@ -9,8 +9,11 @@ * * Over the sky (zero-coverage cells) a sparse field of pixel snow falls on the * same injected clock. Density and speed stay low so the ridgeline keeps its - * silhouette; `still` (idle or reduced motion) freezes the mark and drops the - * snow entirely. Mountain cells always win over flakes. + * silhouette; `still` (idle or reduced motion) freezes the mountain's own + * draw/fill/fade timeline to its fully-filled frame, but snow keeps drifting — + * the landing screen is idle by definition, so tying snow to the same flag + * that freezes the mountain would mean it never falls. Mountain cells always + * win over flakes. * * Everything here is pure and clock-injected: `nowMs` is the only time source, * so the caller's existing 250 ms status tick drives the animation and tests @@ -94,7 +97,11 @@ export type MarkCell = { export type MarkInput = { readonly nowMs: number - /** Hold the mark still: idle session, or reduced motion. */ + /** + * Hold the mountain's draw/fill/fade timeline on its fully-filled frame: + * idle session, or reduced motion. Snow is not gated by this — see + * `snowOn` in `renderMark`. + */ readonly still: boolean /** Which baked rasterization to composite. Defaults to the compact grid. */ readonly grid?: MarkGrid @@ -149,9 +156,10 @@ export function renderMark(input: MarkInput): readonly (readonly MarkCell[])[] { const { drawProg, fillProg, alpha } = markFrame(seconds, input.still) const revealed = drawProg * shape.cols const fillLine = shape.rows * (1 - fillProg) - // Fade out drops the snow too so the decoration doesn't outlast the mark - // it drifts over. - const snowOn = !input.still && alpha === 1 + // Independent of `still`: the mountain can be frozen full while snow still + // drifts (the idle landing screen). Fade out drops the snow too so the + // decoration doesn't outlast the mark it drifts over. + const snowOn = alpha === 1 const grid: MarkCell[][] = [] for (let row = 0; row < shape.rows; row++) { diff --git a/src/tui-opentui/shell.ts b/src/tui-opentui/shell.ts index 9e87951c9..c1f1526d0 100644 --- a/src/tui-opentui/shell.ts +++ b/src/tui-opentui/shell.ts @@ -2556,13 +2556,15 @@ function clearLandingMark(shell: AppShell): void { } /** - * Repaint the landing mark for `nowMs`. `animating` runs the draw/fill/fade - * timeline; anything else holds the filled frame. No-op once the landing is - * gone, so the caller can drive it unconditionally. + * Repaint the landing mark for `nowMs`. `animating` runs the mountain's + * draw/fill/fade timeline; anything else holds its filled frame. No-op once + * the landing is gone, so the caller can drive it unconditionally. * - * A still mark draws the same frame for every clock value, so repainting it - * only dirties renderables; the guard mirrors `setLockupFrame` and lets an idle - * session sit without touching the paint tree. + * Always repaints while the landing is up, even when `animating` is false: + * the landing is idle by definition (no turn processing), and snow still + * needs to drift across a frozen mountain. `paintLandingMark`/`renderMark` + * only touch the rows whose content actually changed, so the unchanging + * mountain rows cost nothing extra here. */ export function paintLanding( shell: AppShell, @@ -2572,7 +2574,6 @@ export function paintLanding( const bag = internals.get(shell) const landing = bag?.landing if (bag === undefined || landing === null || landing === undefined) return - if (!animating && !bag.landingAnimating) return bag.landingAnimating = animating bag.landingNowMs = nowMs paintLandingMark(landing.above, nowMs, !animating) @@ -5599,6 +5600,21 @@ export function createAppShell( // starves that pass of room to lay the row out in. syncTranscriptSpacer(shell) syncNoticeAfterLayout(shell) + // The landing's snow needs a frame source that keeps running while the + // turn monitor is deliberately quiet (idle, no session yet). The renderer + // FRAME event is already scoped to shell lifetime (wired here, unwired in + // `dispose` below) and `paintLanding` no-ops once the landing tears down, + // so riding it costs no extra timer to arm or leak. + // + // Only re-enters while idle (`landingAnimating` already false): while a + // turn is processing, `paintPhaseAt` in runtime-bridge.ts drives the + // mountain's own draw/fill/fade loop off the turn monitor's clock, and + // this re-entry must not stomp that with an unrelated real-clock value + // every render pass. + const landingBag = internals.get(shell) + if (landingBag?.landing != null && !landingBag.landingAnimating) { + paintLanding(shell, Date.now(), false) + } } const onResize = (width: number, height: number): void => { From 9165913e8226e635140609b529fcb0106f162786 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 8 Aug 2026 18:00:46 -0700 Subject: [PATCH 2/3] Throttle the idle landing repaint to ~8fps FRAME fires from inside the on-demand render loop, and paintLanding reassigns a fresh StyledText to every row every call regardless of whether content changed, which unconditionally dirties renderables and requests another render. Left unthrottled that turned the idle landing into a perpetual ~60fps render loop instead of riding an existing one. Guard the onFrame-driven repaint with a stored last-paint timestamp so it only actually repaints once every ~125ms, and drop the comment that falsely claimed only changed rows get touched. Also thread an explicit reducedMotion input through the mark renderer (renderMark/markChunks/paintLandingMark/paintLanding), separate from still, so a future reduced-motion setting has a real plumbing path instead of overloading still (which only freezes the mountain's own draw/fill/fade timeline). Defaults to false everywhere; no behavior change until something sets it. Updates the mark-anim.ts and landing.ts docblocks that stated still suppressed snow, which PR #428 made no longer true. --- src/tui-opentui/landing.test.ts | 10 ++++++++ src/tui-opentui/landing.ts | 12 ++++++--- src/tui-opentui/mark-anim.ts | 28 +++++++++++++------- src/tui-opentui/shell.ts | 45 +++++++++++++++++++++++++++++---- 4 files changed, 77 insertions(+), 18 deletions(-) diff --git a/src/tui-opentui/landing.test.ts b/src/tui-opentui/landing.test.ts index 8ee3c34f7..dd8bad51e 100644 --- a/src/tui-opentui/landing.test.ts +++ b/src/tui-opentui/landing.test.ts @@ -273,6 +273,16 @@ describe("landing screen", () => { await settle(h) const before = markRows(h).join("\n") + // A burst of frames faster than the ~125ms idle-repaint throttle + // must not each produce a distinct paint: the FRAME event fires + // from inside the render loop, so an unthrottled repaint here is + // exactly the uncapped render-loop regression this guards against. + const burstStart = Date.now() + while (Date.now() - burstStart < 60) { + await h.renderOnce() + } + expect(markRows(h).join("\n")).toBe(before) + // Real elapsed time, not an injected clock: the production driver // reads the wall clock, so this is the only way to exercise it. // At the fall speed in mark-anim.ts a few real seconds is enough diff --git a/src/tui-opentui/landing.ts b/src/tui-opentui/landing.ts index e58c03ff2..9f5bd1813 100644 --- a/src/tui-opentui/landing.ts +++ b/src/tui-opentui/landing.ts @@ -271,8 +271,9 @@ function markChunks( grid: MarkGrid, nowMs: number, still: boolean, + reducedMotion = false, ): readonly TextChunk[][] { - return renderMark({ nowMs, still, grid }).map((row) => + return renderMark({ nowMs, still, grid, reducedMotion }).map((row) => row.map((cell) => fgChunk(cell.fg)(cell.char)), ) } @@ -403,17 +404,20 @@ export function fitLandingMark(above: LandingAbove, grid: MarkGrid | null): void } /** - * Repaint the mark for the given clock. `still` holds the fully-filled frame — - * the idle state, and the reduced-motion state. + * Repaint the mark for the given clock. `still` holds the mountain's + * draw/fill/fade timeline on its fully-filled frame — the idle state. + * `reducedMotion` is the separate hook that suppresses snow; it does not + * affect `still`'s mountain framing. */ export function paintLandingMark( above: LandingAbove, nowMs: number, still: boolean, + reducedMotion = false, ): void { const grid = above.grid if (grid === null) return - const chunks = markChunks(grid, nowMs, still) + const chunks = markChunks(grid, nowMs, still, reducedMotion) const offset = MARK_LARGE.rows - grid.rows above.markRows.forEach((line, index) => { const row = chunks[index - offset] diff --git a/src/tui-opentui/mark-anim.ts b/src/tui-opentui/mark-anim.ts index 6f7a60249..450cbc9f2 100644 --- a/src/tui-opentui/mark-anim.ts +++ b/src/tui-opentui/mark-anim.ts @@ -9,11 +9,11 @@ * * Over the sky (zero-coverage cells) a sparse field of pixel snow falls on the * same injected clock. Density and speed stay low so the ridgeline keeps its - * silhouette; `still` (idle or reduced motion) freezes the mountain's own - * draw/fill/fade timeline to its fully-filled frame, but snow keeps drifting — - * the landing screen is idle by definition, so tying snow to the same flag - * that freezes the mountain would mean it never falls. Mountain cells always - * win over flakes. + * silhouette. `still` freezes the mountain's own draw/fill/fade timeline to + * its fully-filled frame but leaves snow drifting — the landing screen is + * idle by definition, so tying snow to the same flag that freezes the + * mountain would mean it never falls. `reducedMotion` is the separate hook + * that does suppress snow. Mountain cells always win over flakes. * * Everything here is pure and clock-injected: `nowMs` is the only time source, * so the caller's existing 250 ms status tick drives the animation and tests @@ -103,6 +103,14 @@ export type MarkInput = { * `snowOn` in `renderMark`. */ readonly still: boolean + /** + * Reduced-motion hook: suppresses snow regardless of `still`. Nothing + * wires a live setting into this yet, but the parameter exists so a + * future reduced-motion setting has a real path to gate motion, rather + * than overloading `still` (which only ever freezes the mountain's + * draw/fill/fade timeline). Defaults to off. + */ + readonly reducedMotion?: boolean /** Which baked rasterization to composite. Defaults to the compact grid. */ readonly grid?: MarkGrid } @@ -145,7 +153,8 @@ function snowflakeAt( * the mark is a mountain, and a mountain is opaque. * * Sky cells (zero coverage) may hold a single falling snow pixel. Flakes never - * overwrite mountain coverage, and `still` suppresses them entirely. + * overwrite mountain coverage; `reducedMotion` suppresses them, `still` does + * not (see `snowOn` below). * * `alpha` has no terminal equivalent, so it scales the block height instead: * the mark sinks toward empty rather than blending to black. @@ -157,9 +166,10 @@ export function renderMark(input: MarkInput): readonly (readonly MarkCell[])[] { const revealed = drawProg * shape.cols const fillLine = shape.rows * (1 - fillProg) // Independent of `still`: the mountain can be frozen full while snow still - // drifts (the idle landing screen). Fade out drops the snow too so the - // decoration doesn't outlast the mark it drifts over. - const snowOn = alpha === 1 + // drifts (the idle landing screen). `reducedMotion` is the actual + // motion-suppression hook. Fade out drops the snow too so the decoration + // doesn't outlast the mark it drifts over. + const snowOn = alpha === 1 && !input.reducedMotion const grid: MarkCell[][] = [] for (let row = 0; row < shape.rows; row++) { diff --git a/src/tui-opentui/shell.ts b/src/tui-opentui/shell.ts index c1f1526d0..7aa90922c 100644 --- a/src/tui-opentui/shell.ts +++ b/src/tui-opentui/shell.ts @@ -1930,6 +1930,12 @@ type ShellInternals = { landingAnimating: boolean /** Clock of the last painted mark frame, so a resize can redraw in place. */ landingNowMs: number + /** + * Wall-clock time of the last idle-driven landing repaint (the FRAME-event + * path in `createAppShell`'s `onFrame`), so that path can throttle itself + * to ~8fps instead of repainting on every render pass. + */ + landingLastIdlePaintMs: number /** Chrome content (empty array = zone off). */ chrome: { /** @@ -2555,6 +2561,14 @@ function clearLandingMark(shell: AppShell): void { } } +/** + * Minimum spacing between idle-driven landing repaints (~8fps). The snow + * only needs to advance about half a row per second, so this is well above + * the animation's actual needs while staying far below the render engine's + * uncapped max rate. + */ +const LANDING_IDLE_REPAINT_INTERVAL_MS = 125 + /** * Repaint the landing mark for `nowMs`. `animating` runs the mountain's * draw/fill/fade timeline; anything else holds its filled frame. No-op once @@ -2562,21 +2576,24 @@ function clearLandingMark(shell: AppShell): void { * * Always repaints while the landing is up, even when `animating` is false: * the landing is idle by definition (no turn processing), and snow still - * needs to drift across a frozen mountain. `paintLandingMark`/`renderMark` - * only touch the rows whose content actually changed, so the unchanging - * mountain rows cost nothing extra here. + * needs to drift across a frozen mountain. Every call reassigns a fresh + * `StyledText` to every row regardless of whether its content changed, which + * unconditionally dirties the renderables and requests another render — the + * idle-driven call site in `createAppShell`'s `onFrame` throttles how often + * it calls this rather than relying on this function to skip unchanged rows. */ export function paintLanding( shell: AppShell, nowMs: number, animating: boolean, + reducedMotion = false, ): void { const bag = internals.get(shell) const landing = bag?.landing if (bag === undefined || landing === null || landing === undefined) return bag.landingAnimating = animating bag.landingNowMs = nowMs - paintLandingMark(landing.above, nowMs, !animating) + paintLandingMark(landing.above, nowMs, !animating, reducedMotion) } /** True while the landing composition is still mounted. */ @@ -5611,9 +5628,26 @@ export function createAppShell( // mountain's own draw/fill/fade loop off the turn monitor's clock, and // this re-entry must not stomp that with an unrelated real-clock value // every render pass. + // + // FRAME fires from inside the render loop itself, and `paintLanding` + // always reassigns fresh row content (see its docblock), which + // unconditionally dirties the renderables and requests another render. + // Left unthrottled that turns into a perpetual render loop at the + // engine's max frame rate rather than the app's configured target, for + // as long as the landing sits idle on screen. The snow only needs to + // advance about half a row per second, so gating repaints to roughly + // every `LANDING_IDLE_REPAINT_INTERVAL_MS` keeps the animation smooth + // at a fraction of the render cost. const landingBag = internals.get(shell) if (landingBag?.landing != null && !landingBag.landingAnimating) { - paintLanding(shell, Date.now(), false) + const nowMs = Date.now() + if ( + nowMs - landingBag.landingLastIdlePaintMs >= + LANDING_IDLE_REPAINT_INTERVAL_MS + ) { + landingBag.landingLastIdlePaintMs = nowMs + paintLanding(shell, nowMs, false) + } } } @@ -5742,6 +5776,7 @@ export function createAppShell( landingSuggestionsVisible: true, landingAnimating: false, landingNowMs: 0, + landingLastIdlePaintMs: 0, chrome: { task: [], tasksRaw: [], agents: [] }, tasksPanelHidden: false, }) From 6fbeba63d8746cfa0ec3cdd684d29e8d703dcaf1 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sat, 8 Aug 2026 18:17:03 -0700 Subject: [PATCH 3/3] Drive the idle landing repaint off a mount-scoped timer, not a throttled FRAME hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The FRAME-driven throttle added in the previous commit killed the landing's self-driving loop entirely: the renderer only keeps rendering because each paint dirties a row, which schedules the next FRAME; skipping a paint on a throttled tick breaks that chain on the very next frame and the snow freezes after the first paint. Any throttle above zero frames has the same effect, since the throttle and the frame source were the same mechanism. Replace it with a plain ~125ms interval armed when the shell mounts (the landing exists for the lifetime of the shell until the first transcript row tears it down) and cleared on whichever teardown happens first: the landing going away, or the shell disposing. The timer self-cancels once the renderer reports destroyed, so headless test harnesses that skip explicit shell.dispose() don't leave it firing against torn-down renderables. Also replace the test's manual renderOnce-loop clock with a real wall-clock wait and no frame pumping at all, so the test can no longer stay green while production's self-driving mechanism is dead — that blind spot is exactly how the frozen throttle shipped in the first place. --- src/tui-opentui/landing.test.ts | 36 ++++------- src/tui-opentui/shell.ts | 102 +++++++++++++++++--------------- 2 files changed, 68 insertions(+), 70 deletions(-) diff --git a/src/tui-opentui/landing.test.ts b/src/tui-opentui/landing.test.ts index dd8bad51e..20fe2e938 100644 --- a/src/tui-opentui/landing.test.ts +++ b/src/tui-opentui/landing.test.ts @@ -253,16 +253,17 @@ describe("landing screen", () => { }) test( - "an idle mount keeps the snow drifting on the renderer's own frame event", + "an idle mount keeps the snow drifting on its own, with nothing pumping frames by hand", async () => { // Regression for CL-5737: every other test in this file drives the mark // by calling `paintLanding` directly with a hand-picked clock. That is // exactly why the landing snow shipped completely unreachable — none of // those tests go through the real driver a running session actually - // uses. This one mounts the shell for real and lets the renderer's own - // FRAME event (wired in `createAppShell`, unwired in `shell.dispose`) - // drive the repaint, the same as production, with no direct calls to - // `paintLanding` or `renderMark`. + // uses. This one mounts the shell for real and lets it repaint itself: + // no `paintLanding`/`renderMark` calls, and critically no `renderOnce` + // loop either while waiting — a test that pumps frames by hand can stay + // green even when production's self-driving mechanism is dead, which is + // exactly the blind spot that let the throttled build ship frozen snow. await withTestRenderer(async (h) => { const shell = createAppShell(h.renderer, { terminal: { columns: 80, rows: 24 }, @@ -273,24 +274,13 @@ describe("landing screen", () => { await settle(h) const before = markRows(h).join("\n") - // A burst of frames faster than the ~125ms idle-repaint throttle - // must not each produce a distinct paint: the FRAME event fires - // from inside the render loop, so an unthrottled repaint here is - // exactly the uncapped render-loop regression this guards against. - const burstStart = Date.now() - while (Date.now() - burstStart < 60) { - await h.renderOnce() - } - expect(markRows(h).join("\n")).toBe(before) - - // Real elapsed time, not an injected clock: the production driver - // reads the wall clock, so this is the only way to exercise it. - // At the fall speed in mark-anim.ts a few real seconds is enough - // for at least one active flake column to cross a row boundary. - const start = Date.now() - while (Date.now() - start < 5_000) { - await h.renderOnce() - } + // Real wall-clock wait, no renderOnce in between: only the mount's + // own idle-repaint timer can be advancing the snow here. `flush` + // waits on the renderer's own scheduler settling rather than + // forcing frames, so it does not manufacture the motion itself. + await new Promise((resolve) => setTimeout(resolve, 3_000)) + + await h.flush() const after = markRows(h).join("\n") expect(after).not.toBe(before) diff --git a/src/tui-opentui/shell.ts b/src/tui-opentui/shell.ts index 7aa90922c..458695141 100644 --- a/src/tui-opentui/shell.ts +++ b/src/tui-opentui/shell.ts @@ -471,7 +471,7 @@ function dispatchOverlayAccept( /** Renderer surface required by the shell (CliRenderer / createTestRenderer). */ export type ShellRenderer = Pick< CliRenderer, - "root" | "width" | "height" | "keyInput" | "on" | "off" + "root" | "width" | "height" | "keyInput" | "on" | "off" | "isDestroyed" > export type AppShellOptions = { @@ -1931,11 +1931,12 @@ type ShellInternals = { /** Clock of the last painted mark frame, so a resize can redraw in place. */ landingNowMs: number /** - * Wall-clock time of the last idle-driven landing repaint (the FRAME-event - * path in `createAppShell`'s `onFrame`), so that path can throttle itself - * to ~8fps instead of repainting on every render pass. + * Cancels the mount-scoped idle repaint timer (see `armLandingIdleTimer` + * in `createAppShell`), or null while none is armed. Cleared by whichever + * teardown happens first — the landing going away (`clearLandingMark`) or + * the whole shell disposing (`dispose`) — so it can never outlive either. */ - landingLastIdlePaintMs: number + landingIdleTimerCancel: (() => void) | null /** Chrome content (empty array = zone off). */ chrome: { /** @@ -2539,6 +2540,8 @@ function clearLandingMark(shell: AppShell): void { const landing = bag?.landing if (bag === undefined || landing === null || landing === undefined) return bag.landing = null + bag.landingIdleTimerCancel?.() + bag.landingIdleTimerCancel = null shell.transcript.remove(landing.above.box) destroySubtree(landing.above.box) shell.root.remove(landing.below) @@ -2562,10 +2565,9 @@ function clearLandingMark(shell: AppShell): void { } /** - * Minimum spacing between idle-driven landing repaints (~8fps). The snow - * only needs to advance about half a row per second, so this is well above - * the animation's actual needs while staying far below the render engine's - * uncapped max rate. + * Cadence of the mount-scoped idle repaint timer (see `armLandingIdleTimer` + * in `createAppShell`). The snow only needs to advance about half a row per + * second, so ~8fps is comfortably enough to read as motion. */ const LANDING_IDLE_REPAINT_INTERVAL_MS = 125 @@ -2576,11 +2578,10 @@ const LANDING_IDLE_REPAINT_INTERVAL_MS = 125 * * Always repaints while the landing is up, even when `animating` is false: * the landing is idle by definition (no turn processing), and snow still - * needs to drift across a frozen mountain. Every call reassigns a fresh - * `StyledText` to every row regardless of whether its content changed, which - * unconditionally dirties the renderables and requests another render — the - * idle-driven call site in `createAppShell`'s `onFrame` throttles how often - * it calls this rather than relying on this function to skip unchanged rows. + * needs to drift across a frozen mountain. Driven by the mount-scoped timer + * armed in `createAppShell` (see `armLandingIdleTimer`) rather than a render + * event, so the repaint cadence is independent of however often the renderer + * happens to paint. */ export function paintLanding( shell: AppShell, @@ -5617,38 +5618,6 @@ export function createAppShell( // starves that pass of room to lay the row out in. syncTranscriptSpacer(shell) syncNoticeAfterLayout(shell) - // The landing's snow needs a frame source that keeps running while the - // turn monitor is deliberately quiet (idle, no session yet). The renderer - // FRAME event is already scoped to shell lifetime (wired here, unwired in - // `dispose` below) and `paintLanding` no-ops once the landing tears down, - // so riding it costs no extra timer to arm or leak. - // - // Only re-enters while idle (`landingAnimating` already false): while a - // turn is processing, `paintPhaseAt` in runtime-bridge.ts drives the - // mountain's own draw/fill/fade loop off the turn monitor's clock, and - // this re-entry must not stomp that with an unrelated real-clock value - // every render pass. - // - // FRAME fires from inside the render loop itself, and `paintLanding` - // always reassigns fresh row content (see its docblock), which - // unconditionally dirties the renderables and requests another render. - // Left unthrottled that turns into a perpetual render loop at the - // engine's max frame rate rather than the app's configured target, for - // as long as the landing sits idle on screen. The snow only needs to - // advance about half a row per second, so gating repaints to roughly - // every `LANDING_IDLE_REPAINT_INTERVAL_MS` keeps the animation smooth - // at a fraction of the render cost. - const landingBag = internals.get(shell) - if (landingBag?.landing != null && !landingBag.landingAnimating) { - const nowMs = Date.now() - if ( - nowMs - landingBag.landingLastIdlePaintMs >= - LANDING_IDLE_REPAINT_INTERVAL_MS - ) { - landingBag.landingLastIdlePaintMs = nowMs - paintLanding(shell, nowMs, false) - } - } } const onResize = (width: number, height: number): void => { @@ -5737,6 +5706,7 @@ export function createAppShell( } renderer.off(CliRenderEvents.FRAME, onFrame) renderer.off(CliRenderEvents.RESIZE, onResize) + internals.get(shell)?.landingIdleTimerCancel?.() flashTimers.get(shell)?.() flashTimers.delete(shell) try { @@ -5776,10 +5746,48 @@ export function createAppShell( landingSuggestionsVisible: true, landingAnimating: false, landingNowMs: 0, - landingLastIdlePaintMs: 0, + landingIdleTimerCancel: null, chrome: { task: [], tasksRaw: [], agents: [] }, tasksPanelHidden: false, }) + // The landing's snow needs a frame source that keeps running while the + // turn monitor is deliberately quiet (idle, no session yet). A plain timer + // armed at mount is that source: it does not depend on the renderer + // scheduling further frames, so it cannot stall the way riding the + // renderer's own FRAME event did (see CL-5737 history in the PR). + // + // Only repaints while idle (`landingAnimating` false): while a turn is + // processing, `paintPhaseAt` in runtime-bridge.ts drives the mountain's + // own draw/fill/fade loop off the turn monitor's clock, and this timer + // must not stomp that with an unrelated real-clock value. + // + // Cleared on whichever teardown happens first: the landing going away + // (`clearLandingMark`, first transcript row) or the whole shell disposing + // (`dispose` below, e.g. tests that never grow a transcript). + // + // Also self-cancels on `renderer.isDestroyed`: a real terminal session + // always disposes the shell, but headless test harnesses commonly destroy + // the renderer directly (`withTestRenderer`'s cleanup) without ever + // calling `shell.dispose()`. Without this check the timer would keep + // firing against renderables the harness already tore down. + const landingIdleHandle = setInterval(() => { + if (renderer.isDestroyed) { + clearInterval(landingIdleHandle) + return + } + const bag = internals.get(shell) + if (bag?.landing == null || bag.landingAnimating) return + paintLanding(shell, Date.now(), false) + }, LANDING_IDLE_REPAINT_INTERVAL_MS) + landingIdleHandle.unref?.() + { + const bag = internals.get(shell) + if (bag !== undefined) { + bag.landingIdleTimerCancel = () => clearInterval(landingIdleHandle) + } else { + clearInterval(landingIdleHandle) + } + } transcriptSpacers.set(shell, transcriptSpacer) if (onCommandOpt) setPaletteOnCommand(shell, onCommandOpt) if (onObserveRequestOpt) {