From 68a6bcfcc4e3d4cac15868f569e57d3d09a8dc0a Mon Sep 17 00:00:00 2001 From: plombeer31 Date: Tue, 25 Aug 2026 20:20:08 +0300 Subject: [PATCH] fix(tui): whole frames, and stop repainting for a phrase nobody can see MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported on Windows 10 / v0.4.1: "UI is shaking and some line broken". **The shaking is frame tearing.** Ink paints by writing a whole frame to stdout — cursor home, every line, the trailing clear. On a terminal that renders as bytes arrive, the interval between the first line and the last is an interval where the screen holds half of the old frame and half of the new. That is not a Windows bug; macOS terminals just coalesce hard enough to hide it, which is how a cross-platform behaviour arrived as a Windows report. DEC private mode 2026 is the fix the terminal side already implemented: `CSI ? 2026 h` holds rendering until `CSI ? 2026 l`, so a frame appears whole or not at all. Every frame is now bracketed in one write — one, not three, because three would put the stream's own chunking between a marker and the frame it is meant to bracket. Sent unconditionally on a TTY, and safe to: an unrecognised DEC private mode is ignored, which is the whole point of "private", and it is why terminals without 2026 have taken these bytes from other TUIs for years. No probe, for the same reason `alt-screen.ts` does not probe — asking costs a round trip on a stdin something else is reading, and the fallback is exactly today's behaviour. `ATOMIC_AGENT_NO_SYNC_OUTPUT=1` opts out. Installed by patching `stdout.write` rather than by handing Ink a wrapper stream: Ink reads `columns`, `rows` and `resize` off that same object, and a proxy that forwards those subtly wrong breaks resize handling in a way far harder to see than tearing. **And there was a repaint going nowhere.** `useRotatingPlaceholder` ran its interval unconditionally, but the phrase is drawn only while the composer is empty. From the first character typed, the timer went on firing every four seconds — a `setState` at the root, a full Ink frame, for a string nobody could see — for the rest of the session. It is now gated on visibility, the same way `useSpinner` and `useAtomField` already were. The cheapest frame is the one that is never painted, and on a tearing terminal it is also the one that cannot flicker. **The broken line is not this.** That is glyph width under a non-UTF-8 Windows console code page, which PR #236 (`fix(cli): switch Windows console to UTF-8 on startup`) addresses directly. Deliberately left alone here so the two do not collide. I do not have a Windows 10 machine, so the tearing fix is reasoned and unit-tested rather than observed. The exact bytes reaching the terminal are asserted; whether Windows Terminal stops shaking needs someone on Windows to run the branch. --- src/tui/components/prompt-shell.tsx | 11 +- .../hooks/use-rotating-placeholder.test.tsx | 55 +++++++- src/tui/hooks/use-rotating-placeholder.ts | 16 ++- src/tui/synchronized-output.test.ts | 115 ++++++++++++++++ src/tui/synchronized-output.ts | 123 ++++++++++++++++++ src/tui/tui-command.ts | 9 ++ 6 files changed, 323 insertions(+), 6 deletions(-) create mode 100644 src/tui/synchronized-output.test.ts create mode 100644 src/tui/synchronized-output.ts diff --git a/src/tui/components/prompt-shell.tsx b/src/tui/components/prompt-shell.tsx index 230a5977..75e7c44d 100644 --- a/src/tui/components/prompt-shell.tsx +++ b/src/tui/components/prompt-shell.tsx @@ -102,12 +102,19 @@ export function PromptShell(props: PromptShellProps): ReactElement { mouseLayer, ...editorProps } = props; + // Rotate only while the phrase is on screen. `effectivePlaceholder` + // below already blanks it for a non-empty buffer; without the same + // condition on the timer, typing left a four-second full-frame repaint + // running behind the composer for the rest of the session. + const placeholderVisible = value.length === 0; const rotated = useRotatingPlaceholder( rotatingPlaceholders ?? [], placeholderRotationMs, + placeholderVisible, ); - const effectivePlaceholder = - value.length === 0 ? (rotated ?? placeholder ?? "") : ""; + const effectivePlaceholder = placeholderVisible + ? (rotated ?? placeholder ?? "") + : ""; const accent = focus && !disabled ? theme.colors.accent : theme.colors.border; // Send is live on exactly the condition Enter is: a non-blank buffer // in an editor that is accepting input. `handleEditorSubmit` drops a diff --git a/src/tui/hooks/use-rotating-placeholder.test.tsx b/src/tui/hooks/use-rotating-placeholder.test.tsx index 4bd60e6d..fa1c293f 100644 --- a/src/tui/hooks/use-rotating-placeholder.test.tsx +++ b/src/tui/hooks/use-rotating-placeholder.test.tsx @@ -7,10 +7,11 @@ import { useRotatingPlaceholder } from "./use-rotating-placeholder.js"; interface ProbeProps { phrases: readonly string[]; intervalMs?: number; + active?: boolean; } -function Probe({ phrases, intervalMs }: ProbeProps): ReactElement { - const value = useRotatingPlaceholder(phrases, intervalMs); +function Probe({ phrases, intervalMs, active }: ProbeProps): ReactElement { + const value = useRotatingPlaceholder(phrases, intervalMs, active); return {value ?? ""}; } @@ -60,4 +61,54 @@ describe("useRotatingPlaceholder", () => { expect(strip(lastFrame() ?? "")).toContain("a"); unmount(); }); + + /** + * The repaint that was not going anywhere. + * + * The phrase is drawn only while the composer is empty, but the timer + * ran regardless — so from the first character typed, the app took a + * full Ink frame every four seconds to advance a string nobody could + * see. On a terminal that renders as bytes arrive, a full frame is a + * visible flicker. + */ + it("schedules nothing while inactive", () => { + const spy = vi.spyOn(global, "setInterval"); + const { unmount } = render( + , + ); + expect(spy).not.toHaveBeenCalled(); + spy.mockRestore(); + unmount(); + }); + + it("stops the timer when it goes inactive, and restarts when it returns", () => { + vi.spyOn(Math, "random").mockReturnValue(0); + const { lastFrame, rerender, unmount } = render( + , + ); + vi.advanceTimersByTime(1000); + rerender(); + expect(strip(lastFrame() ?? "")).toContain("b"); + + rerender(); + vi.advanceTimersByTime(5000); + rerender(); + // Five intervals passed and the phrase did not move, which means no + // render was scheduled for it either. + expect(strip(lastFrame() ?? "")).toContain("b"); + + rerender(); + vi.advanceTimersByTime(1000); + rerender(); + expect(strip(lastFrame() ?? "")).toContain("c"); + unmount(); + }); + + it("defaults to active, so existing callers are unchanged", () => { + const spy = vi.spyOn(global, "setInterval"); + const { unmount } = render(); + expect(spy).toHaveBeenCalled(); + spy.mockRestore(); + unmount(); + }); }); diff --git a/src/tui/hooks/use-rotating-placeholder.ts b/src/tui/hooks/use-rotating-placeholder.ts index ff25fea7..525b1fdb 100644 --- a/src/tui/hooks/use-rotating-placeholder.ts +++ b/src/tui/hooks/use-rotating-placeholder.ts @@ -10,20 +10,32 @@ import { useEffect, useState } from "react"; * Used by the prompt shell to surface a rotating set of "what could I * ask?" hints in the empty-input state — mirrors the opencode prompt * `placeholders.normal` behaviour without depending on Solid signals. + * + * `active` is what stops it repainting the app for nothing. The + * placeholder is drawn only while the composer is empty, but the timer + * used to run regardless: from the first character typed the phrase was + * invisible and the interval went on firing, and each fire is a + * `setState` at the root of the tree — a full Ink frame, every four + * seconds, for a string nobody could see. On a terminal that renders as + * bytes arrive, a full frame is a visible flicker (see + * `synchronized-output.ts`), so the cheapest repaint is the one that + * never happens. Same shape as `useSpinner` and `useAtomField`, which + * were already gated this way. */ export function useRotatingPlaceholder( phrases: readonly string[], intervalMs: number = 4000, + active: boolean = true, ): string | undefined { const initial = phrases.length > 0 ? Math.floor(Math.random() * phrases.length) : 0; const [idx, setIdx] = useState(initial); useEffect(() => { - if (phrases.length <= 1) return; + if (!active || phrases.length <= 1) return; const handle = setInterval(() => { setIdx((prev) => (prev + 1) % phrases.length); }, intervalMs); return () => clearInterval(handle); - }, [phrases, intervalMs]); + }, [active, phrases, intervalMs]); if (phrases.length === 0) return undefined; const safeIdx = idx % phrases.length; return phrases[safeIdx]; diff --git a/src/tui/synchronized-output.test.ts b/src/tui/synchronized-output.test.ts new file mode 100644 index 00000000..1ad05fd3 --- /dev/null +++ b/src/tui/synchronized-output.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it } from "vitest"; +import { + enableSynchronizedOutput, + looksLikeFrame, +} from "./synchronized-output.js"; + +const BSU = "\u001B[?2026h"; +const ESU = "\u001B[?2026l"; + +/** A stdout stand-in that records exactly what reached the terminal. */ +function fakeStdout(isTTY: boolean): NodeJS.WriteStream & { writes: string[] } { + const writes: string[] = []; + const stream = { + isTTY, + writes, + write(chunk: unknown): boolean { + writes.push(String(chunk)); + return true; + }, + }; + return stream as unknown as NodeJS.WriteStream & { writes: string[] }; +} + +const FRAME = "\u001B[H\u001B[2Kline one\nline two\nline three\n"; + +describe("enableSynchronizedOutput", () => { + it("wraps a frame in one write, not three", () => { + // Three writes would put the stream's own chunking between a marker + // and the frame it brackets — which is the thing this prevents. + const stdout = fakeStdout(true); + const controller = enableSynchronizedOutput({ stdout, env: {} }); + stdout.write(FRAME); + controller.restore(); + + expect(stdout.writes[0]).toBe(`${BSU}${FRAME}${ESU}`); + }); + + it("leaves short control sequences alone", () => { + const stdout = fakeStdout(true); + const controller = enableSynchronizedOutput({ stdout, env: {} }); + stdout.write("[?25l"); + controller.restore(); + + expect(stdout.writes[0]).toBe("[?25l"); + }); + + it("does nothing at all off a TTY", () => { + // A pipe cannot tear, and the markers would be noise in a captured + // log or a snapshot test. + const stdout = fakeStdout(false); + const controller = enableSynchronizedOutput({ stdout, env: {} }); + stdout.write(FRAME); + controller.restore(); + + expect(stdout.writes).toEqual([FRAME]); + }); + + it("honours the opt-out", () => { + const stdout = fakeStdout(true); + const controller = enableSynchronizedOutput({ + stdout, + env: { ATOMIC_AGENT_NO_SYNC_OUTPUT: "1" }, + }); + stdout.write(FRAME); + controller.restore(); + + expect(stdout.writes).toEqual([FRAME]); + }); + + it("restores the original write, and closes any open update", () => { + const stdout = fakeStdout(true); + const controller = enableSynchronizedOutput({ stdout, env: {} }); + controller.restore(); + stdout.write(FRAME); + + // The trailing ESU is insurance: a crash between the markers would + // otherwise leave the terminal holding its display. + expect(stdout.writes[0]).toBe(ESU); + expect(stdout.writes[1]).toBe(FRAME); + }); + + it("is safe to restore twice", () => { + const stdout = fakeStdout(true); + const controller = enableSynchronizedOutput({ stdout, env: {} }); + controller.restore(); + controller.restore(); + + expect(stdout.writes.filter((w) => w === ESU)).toHaveLength(1); + }); + + it("does not clobber a patch installed after ours", () => { + const stdout = fakeStdout(true); + const controller = enableSynchronizedOutput({ stdout, env: {} }); + const later = ((chunk: unknown) => { + stdout.writes.push(`later:${String(chunk)}`); + return true; + }) as NodeJS.WriteStream["write"]; + stdout.write = later; + controller.restore(); + + expect(stdout.write).toBe(later); + }); +}); + +describe("looksLikeFrame", () => { + it("counts anything with a newline, or anything long", () => { + expect(looksLikeFrame("a\nb")).toBe(true); + expect(looksLikeFrame("x".repeat(65))).toBe(true); + }); + + it("does not count a bare mode toggle", () => { + expect(looksLikeFrame("[?25h")).toBe(false); + expect(looksLikeFrame("[?1049l")).toBe(false); + }); +}); diff --git a/src/tui/synchronized-output.ts b/src/tui/synchronized-output.ts new file mode 100644 index 00000000..d75723a8 --- /dev/null +++ b/src/tui/synchronized-output.ts @@ -0,0 +1,123 @@ +/** + * Frame tearing, and the one escape sequence that ends it. + * + * Ink paints by writing a whole frame to stdout: cursor home, then every + * line, then the trailing clear. On a terminal that renders as bytes + * arrive, the moment between the first line and the last is a moment + * where the screen holds half of the previous frame and half of the + * next. Redraw often enough and that reads as the UI *shaking* — which + * is the word the Windows 10 report used, and the right one. + * + * It is not equally visible everywhere. macOS terminals coalesce + * aggressively enough to hide most of it; Windows Terminal and conhost + * do not, which is how a cross-platform behaviour arrived as a Windows + * bug report. + * + * DEC private mode 2026 — "synchronized output" — is the fix the + * terminal side already implemented. `CSI ? 2026 h` tells the emulator + * to hold what it renders until `CSI ? 2026 l`, so a frame appears whole + * or not at all. + * + * **Why it is safe to send unconditionally.** An unrecognised DEC private + * mode is ignored — that is what the "private" in the name buys, and it + * is why terminals without 2026 have always received these bytes from + * other TUIs without complaint. There is no probe here for the same + * reason there is none in `alt-screen.ts`: asking costs a round trip on + * a stdin something else is reading, and the fallback is exactly the + * behaviour we have today. + * + * `ATOMIC_AGENT_NO_SYNC_OUTPUT=1` turns it off for anyone who finds a + * terminal that mishandles it. + */ +import { registerTerminalRestore } from "./terminal-restore.js"; + +/** Begin Synchronized Update. */ +const BSU = "\u001B[?2026h"; +/** End Synchronized Update. */ +const ESU = "\u001B[?2026l"; + +export interface SynchronizedOutputController { + /** Puts the original `write` back. Safe to call twice. */ + restore(): void; +} + +export interface SynchronizedOutputOptions { + readonly stdout?: NodeJS.WriteStream; + /** Env source for the opt-out; injectable for tests. */ + readonly env?: NodeJS.ProcessEnv; +} + +/** + * True when `chunk` is worth bracketing. + * + * Wrapping a lone escape sequence in a synchronized update is not wrong, + * but it is two extra sequences spent rendering nothing — and this + * `write` sees every cursor nudge and mode toggle the app makes, not + * only Ink's frames. A frame is many bytes and contains a newline; a + * mode toggle is neither. + */ +export function looksLikeFrame(chunk: string): boolean { + return chunk.length > 64 || chunk.includes("\n"); +} + +/** + * Bracket each frame Ink writes in a synchronized update. + * + * Implemented by replacing `stdout.write` rather than by handing Ink a + * wrapper stream, because Ink reads `columns`, `rows` and the `resize` + * event off that same object — a proxy would have to forward all of it + * correctly, and getting that subtly wrong breaks resize handling in a + * way much harder to spot than tearing. Patching one method on the real + * stream leaves every other property where it was. `alt-screen.ts` and + * `mouse-tracking.ts` already own the terminal this way. + * + * Non-TTY streams (pipes, CI, the test renderer) are left alone: there + * is nothing to tear, and the markers would be noise in a captured log. + */ +export function enableSynchronizedOutput( + options: SynchronizedOutputOptions = {}, +): SynchronizedOutputController { + const stdout = options.stdout ?? process.stdout; + const env = options.env ?? process.env; + if (!stdout.isTTY || env.ATOMIC_AGENT_NO_SYNC_OUTPUT === "1") { + return { restore: () => {} }; + } + const original = stdout.write.bind(stdout) as ( + chunk: unknown, + ...rest: unknown[] + ) => boolean; + let restored = false; + + const patched = (( + chunk: unknown, + encoding?: unknown, + callback?: unknown, + ): boolean => { + const rest = [encoding, callback].filter((arg) => arg !== undefined); + if (typeof chunk === "string" && looksLikeFrame(chunk)) { + // One `write`, not three. Three would put the stream's own + // chunking between a marker and the frame it is meant to bracket, + // which is the very thing this exists to prevent. + return original(`${BSU}${chunk}${ESU}`, ...rest); + } + return original(chunk, ...rest); + }) as NodeJS.WriteStream["write"]; + + stdout.write = patched; + + const restore = (): void => { + if (restored) return; + restored = true; + // Only put it back if nothing else has patched over us since; + // clobbering a later patch would be worse than leaving ours in. + if (stdout.write === patched) { + stdout.write = original as NodeJS.WriteStream["write"]; + } + // A crash between BSU and ESU would leave the terminal holding its + // display indefinitely. Cheap insurance, and inert where 2026 is not + // implemented. + original(ESU); + }; + registerTerminalRestore(restore); + return { restore }; +} diff --git a/src/tui/tui-command.ts b/src/tui/tui-command.ts index 1296e318..c7d203ab 100644 --- a/src/tui/tui-command.ts +++ b/src/tui/tui-command.ts @@ -17,6 +17,7 @@ import type { MetricSample, MetricSink } from "../tracing/metrics-collector.js"; import { isKnownLocalModelId } from "../local-llm/index.js"; import { registerSession } from "../local-llm/session-registry.js"; import { enterAltScreen } from "./alt-screen.js"; +import { enableSynchronizedOutput } from "./synchronized-output.js"; import { ChatOrchestrator } from "./chat-orchestrator.js"; import { parseTuiArgs, nonInteractiveStdinError, @@ -233,6 +234,11 @@ export async function tuiCommand(args: string[]): Promise { const releaseSession = registerSession(config.paths.localModelsDataDir); const altScreen = enterAltScreen({ stdout: process.stdout, hideCursor: false }); + // Immediately after the alt screen and before the first render: every + // frame from here on is bracketed as one synchronized update, so a + // terminal that renders as bytes arrive shows whole frames instead of + // half of the old one and half of the new. + const synchronizedOutput = enableSynchronizedOutput({ stdout: process.stdout }); // Mouse support. Enabling SGR tracking (1000 + 1006) is what makes // clicking panels, rows, tabs and the prompt work at all — the app @@ -633,6 +639,9 @@ export async function tuiCommand(args: string[]): Promise { mouseTracking?.disable(); mouseStdin.dispose(); altScreen.restore(); + // After the alt screen, so the restore's own writes are still + // bracketed, and before `ink.clear()` for the same reason. + synchronizedOutput.restore(); ink.clear(); } catch { // After SIGHUP the tty is gone and these writes raise EIO; the