fix: address /simplify review — frame-writer dedup desync, scheduler edge cases
Correctness fixes found by max-effort review of the branch diff: - frame-writer.sync() now updates lastFrame alongside log-update's previousOutput. Previously the two dedup layers desynced after a sync() (the clearTerminal path), silently dropping a legitimately-changed frame and emitting an empty BSU/ESU pair. Adds a regression test. - scheduler: the queuePostFlushCb callback now bails if scheduled was reset by cancel(), so a stale callback can't commit on a torn-down tree or re-arm an uncancellable trailing timer. - scheduler.flush() now collects multiple concurrent waiters instead of overwriting a single resolver — fixes a hang when two waitUntilRenderFlush() calls await the same pending commit. - render teardown nulls mountedClear so a post-unmount app.clear() can't write to a torn-down stream. - test-streams getContentWrites imports bsu/esu instead of hardcoding the escape literals. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,4 +1,5 @@
|
||||
import { PassThrough, Writable } from "node:stream";
|
||||
import { bsu, esu } from "../../../runtime/src/io/write-synchronized.ts";
|
||||
|
||||
export interface FakeWritableOptions {
|
||||
columns?: number;
|
||||
@@ -80,7 +81,5 @@ export function captureWrites(stdout: NodeJS.WriteStream): string[] {
|
||||
}
|
||||
|
||||
export function getContentWrites(writes: string[]): string[] {
|
||||
return writes.filter(
|
||||
(w) => w !== "" && !w.startsWith("\x1b[?25") && w !== "\x1b[?2026h" && w !== "\x1b[?2026l",
|
||||
);
|
||||
return writes.filter((w) => w !== "" && !w.startsWith("\x1b[?25") && w !== bsu && w !== esu);
|
||||
}
|
||||
|
||||
@@ -69,6 +69,31 @@ test("debug mode writes complete frames terminated by newline", () => {
|
||||
expect(writes).toEqual(["hello\n", "world\n"]);
|
||||
});
|
||||
|
||||
test("sync() updates the dedup baseline so a later changed frame is not dropped", () => {
|
||||
// Regression: sync() previously updated log-update's previousOutput but not
|
||||
// the frame-writer's own lastFrame. After a sync() (e.g. the clearTerminal
|
||||
// path), re-rendering the pre-sync frame was silently dropped by the stale
|
||||
// lastFrame dedup even though the terminal showed different content.
|
||||
const writes: string[] = [];
|
||||
const stream = new PassThrough() as unknown as NodeJS.WriteStream;
|
||||
Object.assign(stream, { columns: 80, rows: 24, isTTY: true });
|
||||
stream.on("data", (chunk) => writes.push(chunk.toString()));
|
||||
|
||||
const writer = createFrameWriter(stream, {});
|
||||
writer.write("A\n"); // lastFrame = "A\n"
|
||||
const countAfterA = writes.length;
|
||||
|
||||
// Simulate the shouldClear path: terminal is repainted to "B" out-of-band
|
||||
// and the writer is synced to that new baseline.
|
||||
writer.sync("B\n");
|
||||
|
||||
// Re-render "A": content differs from what the terminal now shows ("B"),
|
||||
// so it MUST be emitted, not skipped by a stale lastFrame === "A\n".
|
||||
writer.write("A\n");
|
||||
expect(writes.length).toBeGreaterThan(countAfterA);
|
||||
expect(writes.some((w) => w.includes("A"))).toBe(true);
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Standard rendering
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
@@ -40,6 +40,12 @@ export function createFrameWriter(
|
||||
if (log) log.clear();
|
||||
},
|
||||
sync(frame: string) {
|
||||
// Keep this writer's dedup baseline aligned with log-update's internal
|
||||
// previousOutput. Without this, a later write() of `frame` is skipped by
|
||||
// log-update (state synced) while a write() of the *pre-sync* lastFrame
|
||||
// passes this layer's dedup but is dropped by log-update — desyncing the
|
||||
// two dedup layers and dropping a legitimately-changed frame.
|
||||
lastFrame = frame;
|
||||
if (log) log.sync(frame);
|
||||
},
|
||||
setCursorPosition(pos) {
|
||||
|
||||
@@ -227,6 +227,8 @@ export function createApp(root: Component, rootProps?: RootProps | null): TuiApp
|
||||
// so fullscreen apps get clearTerminal on exit.
|
||||
scheduledCommit = () => {};
|
||||
mountedScheduler?.cancel();
|
||||
// Prevent post-unmount app.clear() from writing to a torn-down stream.
|
||||
mountedClear = null;
|
||||
const stdout = mountedAppContext?.stdout;
|
||||
const stdoutWritable = stdout && !stdout.destroyed && !stdout.writableEnded;
|
||||
if (mountedInteractive && !mountedDebug && mountedCommit && stdoutWritable) {
|
||||
|
||||
@@ -26,7 +26,16 @@ export function createCommitScheduler(
|
||||
const immediate = options.immediate ?? false;
|
||||
const throttleMs = options.throttleMs ?? DEFAULT_THROTTLE_MS;
|
||||
let scheduled = false;
|
||||
let resolveFlush: (() => void) | null = null;
|
||||
// Multiple concurrent flush() callers can be waiting on the same pending
|
||||
// commit; settle all of them rather than overwriting a single resolver.
|
||||
let flushResolvers: (() => void)[] = [];
|
||||
|
||||
function drainFlushResolvers() {
|
||||
if (flushResolvers.length === 0) return;
|
||||
const resolvers = flushResolvers;
|
||||
flushResolvers = [];
|
||||
for (const resolve of resolvers) resolve();
|
||||
}
|
||||
|
||||
// Throttle state (production only): leading+trailing pattern.
|
||||
// The leading call fires immediately, subsequent calls within the window
|
||||
@@ -42,9 +51,7 @@ export function createCommitScheduler(
|
||||
try {
|
||||
commit();
|
||||
} finally {
|
||||
const r = resolveFlush;
|
||||
resolveFlush = null;
|
||||
r?.();
|
||||
drainFlushResolvers();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -52,6 +59,11 @@ export function createCommitScheduler(
|
||||
if (scheduled) return;
|
||||
scheduled = true;
|
||||
queuePostFlushCb(() => {
|
||||
// cancel() (teardown) may run between scheduling and this callback
|
||||
// firing. The callback is a captured closure, so cancel() can't unqueue
|
||||
// it — bail here so it doesn't commit on a torn-down tree or re-arm a
|
||||
// trailing timer that nothing will cancel.
|
||||
if (!scheduled) return;
|
||||
if (immediate) {
|
||||
doCommit();
|
||||
return;
|
||||
@@ -83,7 +95,7 @@ export function createCommitScheduler(
|
||||
function flush(): Promise<void> {
|
||||
if (!scheduled && !hasPendingFlag) return Promise.resolve();
|
||||
return new Promise<void>((resolve) => {
|
||||
resolveFlush = resolve;
|
||||
flushResolvers.push(resolve);
|
||||
});
|
||||
}
|
||||
|
||||
@@ -98,11 +110,9 @@ export function createCommitScheduler(
|
||||
}
|
||||
hasPendingFlag = false;
|
||||
scheduled = false;
|
||||
// Resolve any waiter blocked on flush() — the pending commit will never
|
||||
// fire now, so leaving resolveFlush unsettled would hang waitUntilRenderFlush.
|
||||
const r = resolveFlush;
|
||||
resolveFlush = null;
|
||||
r?.();
|
||||
// Resolve any waiters blocked on flush() — the pending commit will never
|
||||
// fire now, so leaving them unsettled would hang waitUntilRenderFlush.
|
||||
drainFlushResolvers();
|
||||
}
|
||||
|
||||
return { schedule, flush, hasPending, cancel };
|
||||
|
||||
Reference in New Issue
Block a user