perf(ink-compat): finalize benchmark harness, renderer fixes, and docs#230
Merged
RtlZeroMemory merged 32 commits intomainfrom Feb 28, 2026
Merged
perf(ink-compat): finalize benchmark harness, renderer fixes, and docs#230RtlZeroMemory merged 32 commits intomainfrom
RtlZeroMemory merged 32 commits intomainfrom
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (13)
✏️ Tip: You can disable in-progress messages and the fortune message in your review settings. 📝 WalkthroughWalkthroughAdds a benchmark suite (app, harness, runner), benchmarking docs and reports, Ink compatibility migration/docs, renderer and layout performance changes (caching, pooling, ANSI text), ink-compat runtime/translation instrumentation, and multiple utility scripts for preparing, running, and verifying benchmarked comparisons. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Bench Runner (CLI)
participant FS as Filesystem / Symlinks
participant Control as Control Socket Server
participant PTY as PTY (bench-app child)
participant Harness as Bench Harness (screen, proc sampler)
participant Results as Results Dir / JSON
Runner->>FS: prepare symlink (real-ink / ink-compat)
Runner->>Control: open control server (socket)
Runner->>PTY: spawn bench-app in PTY (env + socket)
PTY->>Control: connect and handshake (init)
Runner->>PTY: send tokens/ticks via Control
PTY->>Harness: stream stdout -> harness screen model
Harness->>Runner: sample proc stats, detect meaningful paint, write raw/snapshots
PTY->>Results: write frames.jsonl / run-meta
Runner->>Results: collect frames, compute per-run/batch summaries, write run-summary.json
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
packages/ink-compat/src/reconciler/hostConfig.ts-28-31 (1)
28-31:⚠️ Potential issue | 🟡 MinorFix linter error: avoid non-null assertion.
The pipeline flagged
lint/style/noNonNullAssertionon line 29. SinceObject.keys()returnsstring[]and you're iterating within bounds, the assertion is technically safe but violates the project's lint rules.🔧 Proposed fix
const keys = Object.keys(source); for (let index = 0; index < keys.length; index += 1) { - const key = keys[index]!; + const key = keys[index]; if (key === "children" || key === "key" || key === "ref") continue; + if (key === undefined) continue; out[key] = source[key]; }Alternatively, use
for...ofwhich avoids the assertion entirely:- const keys = Object.keys(source); - for (let index = 0; index < keys.length; index += 1) { - const key = keys[index]!; + for (const key of Object.keys(source)) { if (key === "children" || key === "key" || key === "ref") continue; out[key] = source[key]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/reconciler/hostConfig.ts` around lines 28 - 31, The loop uses a non-null assertion on `keys[index]!` which trips the lint rule; replace the indexed loop with a safer iteration (e.g., `for...of`) over `keys` so you can write `for (const key of keys) { if (key === "children" || key === "key" || key === "ref") continue; out[key] = source[key]; }`, removing the `!` non-null assertion and referencing the same symbols (`keys`, `key`, `out`, `source`) to preserve behavior.packages/bench-app/src/types/ink-module.d.ts-1-4 (1)
1-4:⚠️ Potential issue | 🟡 MinorFix Biome formatting to pass CI.
The pipeline reports a formatting issue. Run
npx biome check --write packages/bench-app/src/types/ink-module.d.tsto auto-fix.The module declaration pattern correctly aliases
"ink"to@jrichman/inktypes, enabling type resolution regardless of which ink implementation is symlinked at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-app/src/types/ink-module.d.ts` around lines 1 - 4, Run the Biome formatter to fix the formatting CI failure for the module declaration file: execute "npx biome check --write packages/bench-app/src/types/ink-module.d.ts" to auto-fix formatting; ensure the file continues to declare module "ink" and export everything from "@jrichman/ink" (the lines "declare module \"ink\"" and "export * from \"@jrichman/ink\"") remain intact after formatting.packages/bench-runner/tsconfig.json-1-11 (1)
1-11:⚠️ Potential issue | 🟡 MinorFix Biome formatting to pass CI.
Same formatting issue as
packages/bench-harness/tsconfig.json. Runnpx biome check --write packages/bench-runner/tsconfig.jsonto auto-fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-runner/tsconfig.json` around lines 1 - 11, The tsconfig.json in the bench-runner package has Biome formatting issues; run the formatter to fix them by executing the command npx biome check --write packages/bench-runner/tsconfig.json (same fix used for the bench-harness tsconfig.json), then commit the reformatted file; focus on the JSON file named tsconfig.json and its compilerOptions block (rootDir/outDir/lib) when verifying the changes.packages/bench-harness/tsconfig.json-1-11 (1)
1-11:⚠️ Potential issue | 🟡 MinorFix Biome formatting to pass CI.
The pipeline reports a formatting error. Biome expects specific JSON formatting. Run
npx biome check --write packages/bench-harness/tsconfig.jsonto auto-fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-harness/tsconfig.json` around lines 1 - 11, The tsconfig.json formatting doesn't meet Biome rules; run the Biome auto-fixer (npx biome check --write) on this tsconfig.json or reformat the file to match Biome's JSON style, then commit; focus on preserving the existing top-level keys ("extends", "compilerOptions" with "composite", "rootDir", "outDir", "lib") and "include" while letting Biome normalize spacing/ordering.results/report_2026-02-27.md-201-201 (1)
201-201:⚠️ Potential issue | 🟡 MinorFix terminology spelling in heading.
cpuprofilesshould beCPU profilesfor consistency/readability in user-facing docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@results/report_2026-02-27.md` at line 201, Update the user-facing heading "Latest cpuprofiles:" to use correct terminology and capitalization by changing it to "Latest CPU profiles:" in the document (look for the exact heading string "Latest cpuprofiles:" to locate where to edit).CHANGELOG.md-7-10 (1)
7-10:⚠️ Potential issue | 🟡 MinorPlease add an Unreleased entry for this PR’s user-facing changes.
The new top section captures alpha.48 history, but the current PR (
#230) benchmark harness/renderer/docs updates should also be reflected under## [Unreleased]before merge.As per coding guidelines:
CHANGELOG.md: UpdateCHANGELOG.mdfor user-facing changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 7 - 10, Add a short Unreleased changelog entry under the existing "## [Unreleased]" heading that summarizes this PR (`#230`) — mention the benchmark harness, renderer, and docs updates — so user-facing changes are recorded; ensure the new entry appears above "## [0.1.0-alpha.48] - 2026-02-27" and follows the same formatting style used in other entries in CHANGELOG.md.packages/bench-harness/src/ptyRun.ts-84-89 (1)
84-89:⚠️ Potential issue | 🟡 MinorFire-and-forget async in
onDatamay cause ordering issues.
applyChunkis async, butvoid applyChunk(data)discards the promise. If data arrives faster thanscreen.writeprocesses, chunks may be applied out of order. Consider queuing writes or awaiting sequentially.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-harness/src/ptyRun.ts` around lines 84 - 89, The onData handler is fire-and-forgeting applyChunk (term.onData -> applyChunk) which can reorder writes; instead serialize processing by introducing a sequential queue or promise-chain so each incoming data buffer (raw, rawBytes) is enqueued and processed only after the previous applyChunk finishes. Concretely, replace the lone void applyChunk(data) call with logic that pushes the buffer to a processing queue or updates a module-scoped promise (e.g., lastApplyPromise = lastApplyPromise.then(() => applyChunk(data)).catch(/*log*/)) to ensure applyChunk runs sequentially and surface errors rather than discarding the promise.scripts/ink-compat-bench/summarize-cpuprofile.mjs-30-42 (1)
30-42:⚠️ Potential issue | 🟡 MinorPipeline failure: Avoid reassigning function parameter.
Line 35 reassigns
url, which the linter flags. Use a new variable instead.🔧 Suggested fix
function toDisplayUrl(url) { if (!url) return "unknown"; + let normalized = url; if (url.startsWith("file://")) { try { const fsPath = new URL(url).pathname; - url = fsPath; + normalized = fsPath; } catch {} } - const idx = url.indexOf("/packages/"); - if (idx >= 0) return url.slice(idx + 1); - const parts = url.split(/[\\/]/).filter(Boolean); + const idx = normalized.indexOf("/packages/"); + if (idx >= 0) return normalized.slice(idx + 1); + const parts = normalized.split(/[\\/]/).filter(Boolean); return parts.slice(Math.max(0, parts.length - 3)).join("/"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ink-compat-bench/summarize-cpuprofile.mjs` around lines 30 - 42, The function toDisplayUrl reassigns its parameter `url` (linter error); avoid mutating the parameter by introducing a new variable (e.g., `normalized` or `fsPath`) to hold the file-path conversion result and use that variable in subsequent logic (replace occurrences of `url` after the try/catch with the new variable), while keeping the original `url` for the initial checks and final return path computation.packages/bench-runner/src/cli.ts-379-381 (1)
379-381:⚠️ Potential issue | 🟡 MinorMisleading metric name:
meanRenderTotalMscontains the sum, not the mean.
renderTotalMsis computed as a sum at line 369, butmeanRenderTotalMsat line 381 assigns the total, not the mean. Either rename tototalRenderMsor compute the actual mean.🔧 Suggested fix
- meanRenderTotalMs: renderTotalMs, + totalRenderMs: renderTotalMs, + meanRenderMs: frames.length > 0 ? renderTotalMs / frames.length : null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-runner/src/cli.ts` around lines 379 - 381, The metric meanRenderTotalMs is misleading because renderTotalMs is the summed value but meanRenderTotalMs is assigned that total; update the code that sets meanRenderTotalMs to either compute the real mean (e.g., meanRenderTotalMs = renderTotalMs / (result.renders?.length ?? result.runs ?? 1)) or rename the output field to totalRenderMs and assign renderTotalMs directly; change references to meanRenderTotalMs accordingly so the name matches the computed value (look for the variables renderTotalMs and meanRenderTotalMs in the result aggregation code).packages/bench-runner/src/verify.ts-80-100 (1)
80-100:⚠️ Potential issue | 🟡 Minor
driveScenariodoesn't check for early child exit.Unlike
cli.ts's version, thisdriveScenariodoesn't accept or check achildFinishedcallback. If the child process exits early, the driver continues sending events to a closed socket.♻️ Suggested improvement
async function driveScenario( scenario: string, seed: number, control: Awaited<ReturnType<typeof openControlServer>>, + childFinished: () => boolean, ): Promise<void> { const ok = await control.waitForClient(4000); if (!ok) return; control.sendLine({ type: "init", seed }); if (scenario === "streaming-chat") { for (let i = 0; i < 120; i++) { + if (childFinished()) break; control.sendLine({ type: "token", text: `t=${i} verify` }); await delay(8); } } else { for (let i = 0; i < 60; i++) { + if (childFinished()) break; control.sendLine({ type: "tick" }); await delay(16); } } control.sendLine({ type: "done" }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-runner/src/verify.ts` around lines 80 - 100, driveScenario currently keeps sending events even if the child process exits; update its signature to accept an optional childFinished callback (e.g. childFinished: () => boolean | Promise<boolean>) and call it after waitForClient and inside both event loops (after each delay and before sending the next sendLine) to bail out early if it returns true; ensure you stop sending further events (and skip or handle the final sendLine({type:"done"}) appropriately) when childFinished reports the child has exited. Use the existing symbols driveScenario, waitForClient, sendLine, and delay to locate where to insert the checks.scripts/ink-compat-bench/summarize-cpuprofile.mjs-26-26 (1)
26-26:⚠️ Potential issue | 🟡 MinorPipeline failure: Use template literal instead of concatenation.
The linter flags string concatenation at line 26. Use a template literal for consistency.
🔧 Suggested fix
- ].join("\n") + "\n", + ].join("\n") + `\n`,Or more idiomatically, include the trailing newline in the array:
" - `--active` excludes `(idle)` samples when computing percentages.", + "", - ].join("\n") + "\n", + ].join("\n"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ink-compat-bench/summarize-cpuprofile.mjs` at line 26, Replace the string concatenation expression "].join('\n') + '\n'" with a template literal or include the trailing newline in the array; e.g., change "].join('\n') + '\n'" to `${yourArray.join('\n')}\n` or add an empty string element so the join already produces the trailing newline, ensuring the linter no longer flags concatenation.packages/bench-harness/src/screenDiff.ts-19-25 (1)
19-25:⚠️ Potential issue | 🟡 MinorPotential mismatch if
lines.lengthdiffers fromrows.The dimension check at line 11 verifies
a.rows === b.rows, but iterates overa.lines.length. If a snapshot'slinesarray has fewer entries thanrows,b.lines[i]could beundefinedwhilea.lines[i]exists, leading to a false diff. Consider also checkinga.lines.length === b.lines.lengthor iterating overMath.max(a.lines.length, b.lines.length).🛡️ Suggested defensive check
export function diffScreens(a: ScreenSnapshot, b: ScreenSnapshot): ScreenDiff { - if (a.cols !== b.cols || a.rows !== b.rows) { + if (a.cols !== b.cols || a.rows !== b.rows || a.lines.length !== b.lines.length) { return { equal: false, firstDiffRow: null, - aLine: `size=${a.cols}x${a.rows}`, - bLine: `size=${b.cols}x${b.rows}`, + aLine: `size=${a.cols}x${a.rows} lines=${a.lines.length}`, + bLine: `size=${b.cols}x${b.rows} lines=${b.lines.length}`, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-harness/src/screenDiff.ts` around lines 19 - 25, The loop in screenDiff that compares a.lines and b.lines can miss or mis-report differences when a.rows === b.rows but the arrays have different lengths; update the check to verify a.lines.length === b.lines.length (or iterate to Math.max(a.lines.length, b.lines.length)) before or during the comparison in the function that uses a, b (the code block using a.lines[i], b.lines[i]) and return a proper diff when lengths differ, ensuring firstDiffRow maps to the correct 1-based row and include aLine/bLine as null when an index is out of range.packages/bench-harness/src/procSampler.ts-1-3 (1)
1-3:⚠️ Potential issue | 🟡 MinorFix import statement ordering to satisfy linter.
The CI pipeline reports that import statements should be sorted. Biome expects alphabetical ordering.
🔧 Proposed fix
-import { readFileSync } from "node:fs"; -import { execFileSync } from "node:child_process"; +import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; import { setTimeout as delay } from "node:timers/promises";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-harness/src/procSampler.ts` around lines 1 - 3, Reorder the top-level imports in procSampler.ts so they are alphabetically sorted by module specifier to satisfy the linter: import from "node:child_process" (execFileSync), then "node:fs" (readFileSync), then "node:timers/promises" (setTimeout as delay); update the import declarations that reference execFileSync, readFileSync, and delay accordingly and ensure no other import groups are out of order.packages/bench-harness/src/index.ts-1-9 (1)
1-9:⚠️ Potential issue | 🟡 MinorFix export statement ordering to satisfy formatter.
The CI pipeline reports that import/export statements should be sorted alphabetically.
🔧 Proposed fix
+export type { ProcSample, ProcSamplerOptions } from "./procSampler.js"; +export { sampleProcUntilExit } from "./procSampler.js"; export type { PtyRunOptions, PtyRunResult } from "./ptyRun.js"; export { runInPty } from "./ptyRun.js"; export type { ScreenSnapshot } from "./screen.js"; export { createScreen } from "./screen.js"; export type { ScreenDiff } from "./screenDiff.js"; export { diffScreens } from "./screenDiff.js"; -export type { ProcSample, ProcSamplerOptions } from "./procSampler.js"; -export { sampleProcUntilExit } from "./procSampler.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-harness/src/index.ts` around lines 1 - 9, Export statements are out of alphabetical order; reorder the exported symbols in packages/bench-harness/src/index.ts so imports/exports are sorted alphabetically (by exported identifier) — specifically arrange: createScreen, diffScreens, diff type ScreenDiff, export types ProcSample and ProcSamplerOptions, export sampleProcUntilExit, and the pty exports (PtyRunOptions, PtyRunResult, runInPty) into alphabetical order by name; update the sequence of the export lines referencing the symbols PtyRunOptions, PtyRunResult, runInPty, ScreenSnapshot, createScreen, ScreenDiff, diffScreens, ProcSample, ProcSamplerOptions, sampleProcUntilExit accordingly so the formatter/CI passes.packages/ink-compat/src/runtime/render.ts-1988-1993 (1)
1988-1993:⚠️ Potential issue | 🟡 MinorFix optional chaining as flagged by linter.
The pipeline reports that
markerCache && markerCache.has(vnodeObject)should use optional chaining.🔧 Proposed fix
const vnodeObject = vnode as object; - if (markerCache && markerCache.has(vnodeObject)) { - return markerCache.get(vnodeObject) === true; + if (markerCache?.has(vnodeObject)) { + return markerCache?.get(vnodeObject) === true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/runtime/render.ts` around lines 1988 - 1993, In hasPercentMarkers, replace the null-check pattern "markerCache && markerCache.has(vnodeObject)" with optional chaining "markerCache?.has(vnodeObject)" (and similarly use "markerCache?.get(vnodeObject)" when reading) so the WeakMap checks use optional chaining without changing logic; keep the early return when vnode is not an object and preserve the truthy comparison "=== true" on the retrieved value.
🧹 Nitpick comments (16)
packages/core/src/layout/engine/layoutEngine.ts (1)
113-119: Consider documenting the invariant-check pattern.This helper throws
RangeErrorfor invalid input, whereas other validation in this file (e.g., lines 196–207) returns aLayoutResultwithok: false. The difference is intentional—this is an internal invariant check rather than external data validation—but a brief comment clarifying that callers are responsible for ensuring non-negative values would aid future maintainers.📝 Suggested documentation
+/** + * Convert a nullable forced dimension to a cache key. + * `@throws` RangeError if value is negative (internal invariant violation). + */ function forcedDimensionKey(value: number | null): number {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/engine/layoutEngine.ts` around lines 113 - 119, Add a short explanatory comment above forcedDimensionKey describing that it is an internal invariant-check helper (it returns NULL_FORCED_DIMENSION for null input and throws RangeError for negative values) and that callers are expected to validate/guarantee non-negative forced dimensions before calling; mention the difference in behavior compared to external validation paths that return LayoutResult { ok: false } so future maintainers understand why it throws.packages/ink-compat/src/reconciler/hostConfig.ts (1)
121-146: LGTM on the logic; same non-null assertion pattern applies.The
prepareUpdatechanges correctly:
- Return
nullfor "no update" (falsy, equivalent to oldfalse)- Return
truewhen any non-ignored key differs or key counts differ- Handle the dual-pass comparison to detect both changed and removed keys
Lines 131 and 140 use the same
keys[index]!pattern that was flagged on line 29. Apply the same fix for consistency.♻️ Consistent fix for non-null assertions
let newCount = 0; - const newKeys = Object.keys(newObj); - for (let index = 0; index < newKeys.length; index += 1) { - const key = newKeys[index]!; + for (const key of Object.keys(newObj)) { if (key === "children" || key === "key" || key === "ref") continue; newCount += 1; if (oldObj[key] !== newObj[key]) return true; } let oldCount = 0; - const oldKeys = Object.keys(oldObj); - for (let index = 0; index < oldKeys.length; index += 1) { - const key = oldKeys[index]!; + for (const key of Object.keys(oldObj)) { if (key === "children" || key === "key" || key === "ref") continue; oldCount += 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/reconciler/hostConfig.ts` around lines 121 - 146, In prepareUpdate, remove the non-null assertion usage on keys[index]! (occurring for newKeys and oldKeys) and replace it with a safe access pattern — e.g., iterate with for (const key of newKeys)/for (const key of oldKeys) or assign const key = newKeys[index] (or oldKeys[index]) and narrow/guard undefined — so the code no longer relies on "!" while preserving the existing ignore checks for "children", "key", and "ref" and the comparisons in prepareUpdate.packages/ink-compat/src/runtime/createInkRenderer.ts (1)
480-498: Remove the unusedcountSelfDirtyfunction (lines 484–498).The function is marked as temporary and is not called anywhere in the codebase. Cleaning it up aligns with finalizing the benchmark harness work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/runtime/createInkRenderer.ts` around lines 480 - 498, Remove the unused debug helper function countSelfDirty (and its surrounding "Debug helpers" comment block if it becomes empty); search for and delete the entire function declaration referencing RuntimeInstance and .selfDirty so no unused symbol remains in createInkRenderer.ts, and ensure there are no remaining references or exports to countSelfDirty afterward.packages/core/src/testing/__tests__/testRenderer.test.ts (1)
119-133: Add a runtime-modefindText()assertion to lock behavior.This test is a good start; adding
assert.notEqual(result.findText("Runtime Mode"), null)will guard runtime query parity more completely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/testing/__tests__/testRenderer.test.ts` around lines 119 - 133, The test "runtime mode keeps query helpers and lazy text access" should assert that text lookup works in runtime mode; update the test that uses createTestRenderer(...) and the result variable to include an assertion that result.findText("Runtime Mode") is not null (e.g., assert.notEqual(result.findText("Runtime Mode"), null)) to lock the runtime query parity alongside existing checks like result.findById and result.findAll.scripts/ink-compat-bench/prepare-real-ink.mjs (1)
13-16: Consider validating target path exists before symlinking.If
@jrichman/inkis not installed,symlinkSyncwill create a broken symlink, leading to confusing runtime errors. A pre-check would provide a clearer failure message.🔧 Proposed validation
+import { existsSync, mkdirSync, rmSync, symlinkSync } from "node:fs"; -import { mkdirSync, rmSync, symlinkSync } from "node:fs"; import path from "node:path"; import { fileURLToPath } from "node:url"; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); const repoRoot = path.resolve(__dirname, "../.."); const benchAppNodeModules = path.join(repoRoot, "packages/bench-app/node_modules"); mkdirSync(benchAppNodeModules, { recursive: true }); const inkLinkPath = path.join(benchAppNodeModules, "ink"); const realInkPath = path.join(repoRoot, "node_modules/@jrichman/ink"); +if (!existsSync(realInkPath)) { + console.error(`[ink-compat-bench] ERROR: real ink not found at ${realInkPath}`); + console.error("Run 'npm install' first to install `@jrichman/ink`"); + process.exit(1); +} + rmSync(inkLinkPath, { force: true }); symlinkSync(realInkPath, inkLinkPath, "junction");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ink-compat-bench/prepare-real-ink.mjs` around lines 13 - 16, Before calling symlinkSync, verify that the target path referenced by realInkPath exists (e.g., using fs.existsSync or fs.accessSync) and if it does not, emit a clear error that `@jrichman/ink` is not installed and abort (throw or process.exit) instead of creating a broken symlink; update the block around realInkPath, rmSync, and symlinkSync to perform this presence check and handle the missing-package case with a descriptive message.scripts/ink-compat-bench/prepare-ink-compat.mjs (2)
13-16: Same validation suggestion applies here.Consider adding an
existsSynccheck forcompatPath(i.e.,packages/ink-compat) to fail fast with a clear message if ink-compat hasn't been built yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ink-compat-bench/prepare-ink-compat.mjs` around lines 13 - 16, Add a pre-check that verifies the target package directory exists before removing or creating the symlink: check fs.existsSync(compatPath) (where compatPath is computed from repoRoot + "packages/ink-compat") and if it returns false, throw or log a clear error and exit; only then proceed to call rmSync(inkLinkPath, { force: true }) and symlinkSync(compatPath, inkLinkPath, "junction") so the script fails fast with a helpful message when ink-compat hasn't been built.
1-17: Consider extracting shared logic withprepare-real-ink.mjs.Both scripts are nearly identical, differing only in the target path. For maintainability, consider a shared helper:
// scripts/ink-compat-bench/link-ink.mjs export function linkInk(targetPath, label) { // shared logic here }Then each script becomes a one-liner calling the helper with different arguments.
That said, these are simple dev scripts and the duplication is minor—this is a nice-to-have improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ink-compat-bench/prepare-ink-compat.mjs` around lines 1 - 17, Extract the duplicated linking logic in prepare-ink-compat.mjs into a shared helper (e.g., export function linkInk(targetNodeModulesPath, sourcePath, label) in a new scripts/ink-compat-bench/link-ink.mjs) that performs mkdirSync(..., { recursive: true }), rmSync(..., { force: true }), symlinkSync(..., "junction") and the console.log; then update prepare-ink-compat.mjs to compute only benchAppNodeModules/compatPath and call linkInk(benchAppNodeModules, compatPath, "ink-compat") and do the same in prepare-real-ink.mjs (calling linkInk with its target and label) so both scripts reuse the single linkInk helper (preserve existing symbols __filename, __dirname, repoRoot, benchAppNodeModules, inkLinkPath, compatPath computation in each script).BENCHMARK_VALIDITY.md (1)
57-73: Add an explicit frame-audit evidence step to the correctness gate.Consider documenting
REZI_FRAME_AUDIT=1capture andnode scripts/frame-audit-report.mjsas a required adjunct to final-screen checks for rendering regressions.Based on learnings: For rendering/layout/theme regressions, validate with live PTY (not just unit snapshots) and collect frame audit evidence via
REZI_FRAME_AUDITlogs andnode scripts/frame-audit-report.mjs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BENCHMARK_VALIDITY.md` around lines 57 - 73, Add an explicit frame-audit evidence step to the Output Equivalence (Correctness Gate): when running the verify harness (the path that captures pty-output.bin and produces screen-final.txt and runs `npm run verify`), require setting `REZI_FRAME_AUDIT=1` to capture per-frame audit logs and mandate running `node scripts/frame-audit-report.mjs` to generate the frame-audit report alongside final-screen comparison; update the rules to mark a scenario valid only if both final-screen equivalence and the generated frame-audit report (from `REZI_FRAME_AUDIT` + `frame-audit-report.mjs`) show no rendering/layout/theme regressions and record the report as required evidence.packages/bench-harness/src/ptyRun.ts (1)
98-107: Silent error swallowing and potential post-exit writes.The
setTimeoutcallbacks may fire after the process exits, and the emptycatch {}at line 105 silently swallows all errors including unexpected ones. Consider checkingexitedbefore writing/resizing, and logging unexpected errors.♻️ Suggested improvement
for (const step of script) { setTimeout(() => { - try { - if (step.kind === "write") term.write(step.data); - else { - term.resize(step.cols, step.rows); - void screen.resize(step.cols, step.rows); - } - } catch {} + if (exited) return; + if (step.kind === "write") { + term.write(step.data); + } else { + term.resize(step.cols, step.rows); + void screen.resize(step.cols, step.rows); + } }, Math.max(0, step.atMs)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-harness/src/ptyRun.ts` around lines 98 - 107, The setTimeout callback currently swallows all errors and may run after the process has exited; update the callback in ptyRun so it first checks an `exited` (or similar) flag and returns early if true, and replace the empty `catch {}` with `catch (err)` that logs unexpected errors (including context: step.kind, step.atMs) instead of silencing them; ensure operations reference the same symbols (`step.kind`, `term.write`, `term.resize`, `screen.resize`, and the `exited` flag) so post-exit writes/resizes are avoided and failures are visible in logs.packages/bench-runner/src/verify.ts (2)
17-27: Significant code duplication withcli.ts.
linkInkForRenderer(lines 17-27) andopenControlServer(lines 41-78) are nearly identical to their counterparts incli.ts. Consider extracting these to a shared module (e.g.,packages/bench-runner/src/common.ts) to reduce maintenance burden.Also applies to: 41-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-runner/src/verify.ts` around lines 17 - 27, The functions linkInkForRenderer and openControlServer are duplicated from cli.ts; extract them into a new shared module (e.g., create a packages/bench-runner/src/common.ts) and export the functions (linkInkForRenderer, openControlServer) so both verify.ts and cli.ts import them; update imports in verify.ts and cli.ts to remove the local copies, ensure any referenced symbols (RendererName, repoRoot, path, mkdirSync, rmSync, symlinkSync, createServer, WebSocketServer, etc.) are either exported from or imported into the new common module, and run tests/TS build to confirm no missing imports or type issues.
178-186: Emptyhashmay cause unexpected behavior if used downstream.
toSnapsetshash: ""butScreenSnapshotlikely expects a meaningful hash. WhilediffScreensdoesn't appear to usehash, this could cause issues if the snapshot is used elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-runner/src/verify.ts` around lines 178 - 186, The toSnap helper returns a ScreenSnapshot with an empty hash which can break downstream consumers; compute and set a deterministic hash from the snapshot content instead of "". In the toSnap function, join the normalized lines (the same array produced for lines) and compute a stable digest (e.g. using Node's crypto.createHash('sha256').update(joinedLines).digest('hex') or your project's existing hashing util) and assign that value to hash so ScreenSnapshot.hash is meaningful and repeatable.packages/bench-runner/src/cli.ts (2)
416-426:linkInkForRendereris defined aftermain()call site.While JavaScript hoists function declarations, placing utility functions after the main entry point reduces readability. Consider moving
linkInkForRendererabovemain()with other helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-runner/src/cli.ts` around lines 416 - 426, Move the helper function linkInkForRenderer so it appears above the main() call site alongside the other utility helpers to improve readability; locate the function declaration for linkInkForRenderer (which references RendererName, repoRoot, and uses mkdirSync/rmSync/symlinkSync) and cut/paste it to sit before the main() definition/invocation so main() and other top-level logic only reference already-declared helpers.
61-63: UsefindLast()to avoid unnecessary array allocation.
[...samples].reverse().find(...)creates a copy of the array. Since the project targets Node.js 22, use the nativefindLast()method (available since Node 18) to iterate backwards without allocation overhead.♻️ Suggested improvement
- const last = [...samples] - .reverse() - .find((s) => s.cpuUserTicks != null && s.cpuSystemTicks != null); + const last = samples.findLast((s) => s.cpuUserTicks != null && s.cpuSystemTicks != null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-runner/src/cli.ts` around lines 61 - 63, The current code builds a reversed copy to find the last sample with cpuUserTicks and cpuSystemTicks: replace the allocation pattern that sets last via [...samples].reverse().find(...) with the native samples.findLast((s) => s.cpuUserTicks != null && s.cpuSystemTicks != null) to iterate backwards in-place; update the variable assignment where `last` is declared so it uses `findLast` (project targets Node 22, so this runtime method is available).scripts/ink-compat-bench/preload.mjs (1)
88-90: Silent catch may hide debugging information.While defensive error handling is appropriate for a preload script, consider logging caught errors to stderr in debug mode (e.g., when
process.env.DEBUGis set) to aid troubleshooting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ink-compat-bench/preload.mjs` around lines 88 - 90, The empty catch block swallows errors during the preload phase; change the anonymous catch to catch (err) and, inside it, conditionally write the error to stderr when process.env.DEBUG is truthy (e.g., console.error('preload error', err) or process.stderr.write), preserving the current silent behavior when DEBUG is not set; locate the catch in preload.mjs (the try/catch around the preload logic) and implement the conditional logging.scripts/ink-compat-bench/summarize-cpuprofile.mjs (1)
5-10:readArgdoes not guard against flag-like values.If a user passes
--filter --json,readArg("filter")returns"--json"instead of falling back. The snippet inscripts/hsr/record-runner.mjsincludes a guard for this. Consider aligning behavior.♻️ Suggested guard
function readArg(name, fallback) { const idx = process.argv.indexOf(`--${name}`); if (idx === -1) return fallback; const v = process.argv[idx + 1]; + if (!v || v.startsWith("--")) return fallback; return v ?? fallback; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ink-compat-bench/summarize-cpuprofile.mjs` around lines 5 - 10, The readArg function returns the following token even when it's another flag (e.g., "--json"); update readArg to verify the next token exists and is not a flag by checking that process.argv[idx+1] is defined and does not start with "--" (align behavior with the guard used in scripts/hsr/record-runner.mjs), and return the fallback if that check fails; reference the readArg function to locate and update this logic.packages/bench-app/src/entry.tsx (1)
170-197: Consider adding error handler for TCP client connection.The TCP client socket doesn't have an error handler attached. Connection failures or socket errors could cause unhandled exceptions during benchmark runs.
🛡️ Proposed fix to add error handling
const client = net.createConnection(socketPath); client.setEncoding("utf8"); + client.on("error", () => { + // Silently ignore connection errors during benchmarks + }); client.on("data", (chunk) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bench-app/src/entry.tsx` around lines 170 - 197, The TCP client created in useControlSocket has no error handler and can emit unhandled 'error' events; attach a listener to the client returned by net.createConnection (referencing client and useControlSocket) to handle 'error' (and optionally 'close') events, log or forward the error via onMsg or a logger, and ensure the cleanup in the returned disposer removes the error listener (and closes/destroys the socket) to avoid leaks and uncaught exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bench-harness/src/screen.ts`:
- Around line 24-25: The code uses a cast to any in the Terminal construction
("new (Terminal as any)(...)") which fails the lint rule; replace the any cast
with a minimal typed constructor/interface and use that to construct the
instance. Define a small constructor type (e.g., TerminalConstructor { new(opts:
TerminalOptions): TerminalInstance }) or reuse the actual Terminal types if
available, cast Terminal via unknown to that constructor (Terminal as unknown as
TerminalConstructor) and call new on it to create term; reference the symbols
Terminal and term and adjust imports/types (TerminalOptions/TerminalInstance)
accordingly so no explicit any is used.
In `@packages/bench-runner/src/cli.ts`:
- Around line 305-311: The data string for the large-list-scroll inputScript is
currently a literal backslash-u sequence and not the actual ESC control
character; update the data value produced in the scenario ===
"large-list-scroll" branch (the array mapping that creates objects with
kind/write, atMs, data) to use an actual escape character (e.g. "\x1b[B" or
"\u001b[B" with a single backslash) so the terminal receives the ANSI arrow-down
sequence rather than the nine-character literal.
In `@packages/core/src/layout/kinds/stack.ts`:
- Around line 995-1060: The measureMaxMain array seeded earlier can become out
of sync when mainSizes are modified during the grow and shrink passes; update
measureMaxMain whenever you mutate mainSizes in the grow allocation loop (after
computing next and assigning mainSizes[item.index] = next) and in the shrink
application loop (after assigning mainSizes[item.index] = next) so it reflects
the new clamped/allocated sizes used later in cross-axis planning; reference the
grow loop handling growItems (alloc from distributeFlex) and the shrink loop
handling shrinkItems (shrunk from shrinkFlex, using clampWithin) and ensure you
set measureMaxMain[item.index] = mainSizes[item.index] (or the computed next)
and keep any bounds logic consistent with maxMains/minMains before releasing
alloc/shrunk arrays.
In `@packages/core/src/testing/renderer.ts`:
- Around line 230-233: The runtime branch currently strips text from text nodes
causing findText() (which checks node.text) to fail; modify the object spread
near the vnode handling so that when node.vnode.kind === "text" you always
include the text property (e.g., set text: (node.vnode as Readonly<{ text:
string }>).text) regardless of mode, or alternatively have findText() consult
node.vnode.text instead of node.text; update the code around the vnode-to-node
mapping (the spread that uses mode and node.vnode.kind) so text is preserved for
text nodes so findText() works in runtime mode.
In `@packages/ink-compat/src/translation/propsToVNode.ts`:
- Around line 413-414: The current computation metaMask = metaAfterMask &
~metaBeforeMask (using toMetaMask(context.meta) as metaBeforeMask) is
order-dependent and can lose flags when sibling order/shape changes; instead
compute and cache an intrinsic per-node mask that represents this node's own
metadata contribution (e.g., compute nodeMetaMask = toMetaMask(node.meta) or
accumulate only this node's meta delta) and use that stored mask when
applying/invalidating cache entries so the result doesn't depend on earlier
siblings; update the logic around metaBeforeMask/metaAfterMask/metaMask (and the
related block at the lines handling masks 428–440) to derive and persist a
node-local mask and use it for mask calculations and cache keys.
- Around line 1031-1037: The multiline branch misses passing textProps (which
carries __inkTransform) into translateMultilineRichText, so update the call to
translateMultilineRichText(spans, rootProps, textProps) and modify
translateMultilineRichText's signature to accept the textProps parameter; inside
translateMultilineRichText, ensure each rendered line merges/apply textProps (so
__inkTransform is preserved) when creating line nodes rather than only using
rootProps, and keep the existing accessibilityLabel/rootProps behavior intact.
---
Minor comments:
In `@CHANGELOG.md`:
- Around line 7-10: Add a short Unreleased changelog entry under the existing
"## [Unreleased]" heading that summarizes this PR (`#230`) — mention the benchmark
harness, renderer, and docs updates — so user-facing changes are recorded;
ensure the new entry appears above "## [0.1.0-alpha.48] - 2026-02-27" and
follows the same formatting style used in other entries in CHANGELOG.md.
In `@packages/bench-app/src/types/ink-module.d.ts`:
- Around line 1-4: Run the Biome formatter to fix the formatting CI failure for
the module declaration file: execute "npx biome check --write
packages/bench-app/src/types/ink-module.d.ts" to auto-fix formatting; ensure the
file continues to declare module "ink" and export everything from
"@jrichman/ink" (the lines "declare module \"ink\"" and "export * from
\"@jrichman/ink\"") remain intact after formatting.
In `@packages/bench-harness/src/index.ts`:
- Around line 1-9: Export statements are out of alphabetical order; reorder the
exported symbols in packages/bench-harness/src/index.ts so imports/exports are
sorted alphabetically (by exported identifier) — specifically arrange:
createScreen, diffScreens, diff type ScreenDiff, export types ProcSample and
ProcSamplerOptions, export sampleProcUntilExit, and the pty exports
(PtyRunOptions, PtyRunResult, runInPty) into alphabetical order by name; update
the sequence of the export lines referencing the symbols PtyRunOptions,
PtyRunResult, runInPty, ScreenSnapshot, createScreen, ScreenDiff, diffScreens,
ProcSample, ProcSamplerOptions, sampleProcUntilExit accordingly so the
formatter/CI passes.
In `@packages/bench-harness/src/procSampler.ts`:
- Around line 1-3: Reorder the top-level imports in procSampler.ts so they are
alphabetically sorted by module specifier to satisfy the linter: import from
"node:child_process" (execFileSync), then "node:fs" (readFileSync), then
"node:timers/promises" (setTimeout as delay); update the import declarations
that reference execFileSync, readFileSync, and delay accordingly and ensure no
other import groups are out of order.
In `@packages/bench-harness/src/ptyRun.ts`:
- Around line 84-89: The onData handler is fire-and-forgeting applyChunk
(term.onData -> applyChunk) which can reorder writes; instead serialize
processing by introducing a sequential queue or promise-chain so each incoming
data buffer (raw, rawBytes) is enqueued and processed only after the previous
applyChunk finishes. Concretely, replace the lone void applyChunk(data) call
with logic that pushes the buffer to a processing queue or updates a
module-scoped promise (e.g., lastApplyPromise = lastApplyPromise.then(() =>
applyChunk(data)).catch(/*log*/)) to ensure applyChunk runs sequentially and
surface errors rather than discarding the promise.
In `@packages/bench-harness/src/screenDiff.ts`:
- Around line 19-25: The loop in screenDiff that compares a.lines and b.lines
can miss or mis-report differences when a.rows === b.rows but the arrays have
different lengths; update the check to verify a.lines.length === b.lines.length
(or iterate to Math.max(a.lines.length, b.lines.length)) before or during the
comparison in the function that uses a, b (the code block using a.lines[i],
b.lines[i]) and return a proper diff when lengths differ, ensuring firstDiffRow
maps to the correct 1-based row and include aLine/bLine as null when an index is
out of range.
In `@packages/bench-harness/tsconfig.json`:
- Around line 1-11: The tsconfig.json formatting doesn't meet Biome rules; run
the Biome auto-fixer (npx biome check --write) on this tsconfig.json or reformat
the file to match Biome's JSON style, then commit; focus on preserving the
existing top-level keys ("extends", "compilerOptions" with "composite",
"rootDir", "outDir", "lib") and "include" while letting Biome normalize
spacing/ordering.
In `@packages/bench-runner/src/cli.ts`:
- Around line 379-381: The metric meanRenderTotalMs is misleading because
renderTotalMs is the summed value but meanRenderTotalMs is assigned that total;
update the code that sets meanRenderTotalMs to either compute the real mean
(e.g., meanRenderTotalMs = renderTotalMs / (result.renders?.length ??
result.runs ?? 1)) or rename the output field to totalRenderMs and assign
renderTotalMs directly; change references to meanRenderTotalMs accordingly so
the name matches the computed value (look for the variables renderTotalMs and
meanRenderTotalMs in the result aggregation code).
In `@packages/bench-runner/src/verify.ts`:
- Around line 80-100: driveScenario currently keeps sending events even if the
child process exits; update its signature to accept an optional childFinished
callback (e.g. childFinished: () => boolean | Promise<boolean>) and call it
after waitForClient and inside both event loops (after each delay and before
sending the next sendLine) to bail out early if it returns true; ensure you stop
sending further events (and skip or handle the final sendLine({type:"done"})
appropriately) when childFinished reports the child has exited. Use the existing
symbols driveScenario, waitForClient, sendLine, and delay to locate where to
insert the checks.
In `@packages/bench-runner/tsconfig.json`:
- Around line 1-11: The tsconfig.json in the bench-runner package has Biome
formatting issues; run the formatter to fix them by executing the command npx
biome check --write packages/bench-runner/tsconfig.json (same fix used for the
bench-harness tsconfig.json), then commit the reformatted file; focus on the
JSON file named tsconfig.json and its compilerOptions block (rootDir/outDir/lib)
when verifying the changes.
In `@packages/ink-compat/src/reconciler/hostConfig.ts`:
- Around line 28-31: The loop uses a non-null assertion on `keys[index]!` which
trips the lint rule; replace the indexed loop with a safer iteration (e.g.,
`for...of`) over `keys` so you can write `for (const key of keys) { if (key ===
"children" || key === "key" || key === "ref") continue; out[key] = source[key];
}`, removing the `!` non-null assertion and referencing the same symbols
(`keys`, `key`, `out`, `source`) to preserve behavior.
In `@packages/ink-compat/src/runtime/render.ts`:
- Around line 1988-1993: In hasPercentMarkers, replace the null-check pattern
"markerCache && markerCache.has(vnodeObject)" with optional chaining
"markerCache?.has(vnodeObject)" (and similarly use
"markerCache?.get(vnodeObject)" when reading) so the WeakMap checks use optional
chaining without changing logic; keep the early return when vnode is not an
object and preserve the truthy comparison "=== true" on the retrieved value.
In `@results/report_2026-02-27.md`:
- Line 201: Update the user-facing heading "Latest cpuprofiles:" to use correct
terminology and capitalization by changing it to "Latest CPU profiles:" in the
document (look for the exact heading string "Latest cpuprofiles:" to locate
where to edit).
In `@scripts/ink-compat-bench/summarize-cpuprofile.mjs`:
- Around line 30-42: The function toDisplayUrl reassigns its parameter `url`
(linter error); avoid mutating the parameter by introducing a new variable
(e.g., `normalized` or `fsPath`) to hold the file-path conversion result and use
that variable in subsequent logic (replace occurrences of `url` after the
try/catch with the new variable), while keeping the original `url` for the
initial checks and final return path computation.
- Line 26: Replace the string concatenation expression "].join('\n') + '\n'"
with a template literal or include the trailing newline in the array; e.g.,
change "].join('\n') + '\n'" to `${yourArray.join('\n')}\n` or add an empty
string element so the join already produces the trailing newline, ensuring the
linter no longer flags concatenation.
---
Nitpick comments:
In `@BENCHMARK_VALIDITY.md`:
- Around line 57-73: Add an explicit frame-audit evidence step to the Output
Equivalence (Correctness Gate): when running the verify harness (the path that
captures pty-output.bin and produces screen-final.txt and runs `npm run
verify`), require setting `REZI_FRAME_AUDIT=1` to capture per-frame audit logs
and mandate running `node scripts/frame-audit-report.mjs` to generate the
frame-audit report alongside final-screen comparison; update the rules to mark a
scenario valid only if both final-screen equivalence and the generated
frame-audit report (from `REZI_FRAME_AUDIT` + `frame-audit-report.mjs`) show no
rendering/layout/theme regressions and record the report as required evidence.
In `@packages/bench-app/src/entry.tsx`:
- Around line 170-197: The TCP client created in useControlSocket has no error
handler and can emit unhandled 'error' events; attach a listener to the client
returned by net.createConnection (referencing client and useControlSocket) to
handle 'error' (and optionally 'close') events, log or forward the error via
onMsg or a logger, and ensure the cleanup in the returned disposer removes the
error listener (and closes/destroys the socket) to avoid leaks and uncaught
exceptions.
In `@packages/bench-harness/src/ptyRun.ts`:
- Around line 98-107: The setTimeout callback currently swallows all errors and
may run after the process has exited; update the callback in ptyRun so it first
checks an `exited` (or similar) flag and returns early if true, and replace the
empty `catch {}` with `catch (err)` that logs unexpected errors (including
context: step.kind, step.atMs) instead of silencing them; ensure operations
reference the same symbols (`step.kind`, `term.write`, `term.resize`,
`screen.resize`, and the `exited` flag) so post-exit writes/resizes are avoided
and failures are visible in logs.
In `@packages/bench-runner/src/cli.ts`:
- Around line 416-426: Move the helper function linkInkForRenderer so it appears
above the main() call site alongside the other utility helpers to improve
readability; locate the function declaration for linkInkForRenderer (which
references RendererName, repoRoot, and uses mkdirSync/rmSync/symlinkSync) and
cut/paste it to sit before the main() definition/invocation so main() and other
top-level logic only reference already-declared helpers.
- Around line 61-63: The current code builds a reversed copy to find the last
sample with cpuUserTicks and cpuSystemTicks: replace the allocation pattern that
sets last via [...samples].reverse().find(...) with the native
samples.findLast((s) => s.cpuUserTicks != null && s.cpuSystemTicks != null) to
iterate backwards in-place; update the variable assignment where `last` is
declared so it uses `findLast` (project targets Node 22, so this runtime method
is available).
In `@packages/bench-runner/src/verify.ts`:
- Around line 17-27: The functions linkInkForRenderer and openControlServer are
duplicated from cli.ts; extract them into a new shared module (e.g., create a
packages/bench-runner/src/common.ts) and export the functions
(linkInkForRenderer, openControlServer) so both verify.ts and cli.ts import
them; update imports in verify.ts and cli.ts to remove the local copies, ensure
any referenced symbols (RendererName, repoRoot, path, mkdirSync, rmSync,
symlinkSync, createServer, WebSocketServer, etc.) are either exported from or
imported into the new common module, and run tests/TS build to confirm no
missing imports or type issues.
- Around line 178-186: The toSnap helper returns a ScreenSnapshot with an empty
hash which can break downstream consumers; compute and set a deterministic hash
from the snapshot content instead of "". In the toSnap function, join the
normalized lines (the same array produced for lines) and compute a stable digest
(e.g. using Node's crypto.createHash('sha256').update(joinedLines).digest('hex')
or your project's existing hashing util) and assign that value to hash so
ScreenSnapshot.hash is meaningful and repeatable.
In `@packages/core/src/layout/engine/layoutEngine.ts`:
- Around line 113-119: Add a short explanatory comment above forcedDimensionKey
describing that it is an internal invariant-check helper (it returns
NULL_FORCED_DIMENSION for null input and throws RangeError for negative values)
and that callers are expected to validate/guarantee non-negative forced
dimensions before calling; mention the difference in behavior compared to
external validation paths that return LayoutResult { ok: false } so future
maintainers understand why it throws.
In `@packages/core/src/testing/__tests__/testRenderer.test.ts`:
- Around line 119-133: The test "runtime mode keeps query helpers and lazy text
access" should assert that text lookup works in runtime mode; update the test
that uses createTestRenderer(...) and the result variable to include an
assertion that result.findText("Runtime Mode") is not null (e.g.,
assert.notEqual(result.findText("Runtime Mode"), null)) to lock the runtime
query parity alongside existing checks like result.findById and result.findAll.
In `@packages/ink-compat/src/reconciler/hostConfig.ts`:
- Around line 121-146: In prepareUpdate, remove the non-null assertion usage on
keys[index]! (occurring for newKeys and oldKeys) and replace it with a safe
access pattern — e.g., iterate with for (const key of newKeys)/for (const key of
oldKeys) or assign const key = newKeys[index] (or oldKeys[index]) and
narrow/guard undefined — so the code no longer relies on "!" while preserving
the existing ignore checks for "children", "key", and "ref" and the comparisons
in prepareUpdate.
In `@packages/ink-compat/src/runtime/createInkRenderer.ts`:
- Around line 480-498: Remove the unused debug helper function countSelfDirty
(and its surrounding "Debug helpers" comment block if it becomes empty); search
for and delete the entire function declaration referencing RuntimeInstance and
.selfDirty so no unused symbol remains in createInkRenderer.ts, and ensure there
are no remaining references or exports to countSelfDirty afterward.
In `@scripts/ink-compat-bench/preload.mjs`:
- Around line 88-90: The empty catch block swallows errors during the preload
phase; change the anonymous catch to catch (err) and, inside it, conditionally
write the error to stderr when process.env.DEBUG is truthy (e.g.,
console.error('preload error', err) or process.stderr.write), preserving the
current silent behavior when DEBUG is not set; locate the catch in preload.mjs
(the try/catch around the preload logic) and implement the conditional logging.
In `@scripts/ink-compat-bench/prepare-ink-compat.mjs`:
- Around line 13-16: Add a pre-check that verifies the target package directory
exists before removing or creating the symlink: check fs.existsSync(compatPath)
(where compatPath is computed from repoRoot + "packages/ink-compat") and if it
returns false, throw or log a clear error and exit; only then proceed to call
rmSync(inkLinkPath, { force: true }) and symlinkSync(compatPath, inkLinkPath,
"junction") so the script fails fast with a helpful message when ink-compat
hasn't been built.
- Around line 1-17: Extract the duplicated linking logic in
prepare-ink-compat.mjs into a shared helper (e.g., export function
linkInk(targetNodeModulesPath, sourcePath, label) in a new
scripts/ink-compat-bench/link-ink.mjs) that performs mkdirSync(..., { recursive:
true }), rmSync(..., { force: true }), symlinkSync(..., "junction") and the
console.log; then update prepare-ink-compat.mjs to compute only
benchAppNodeModules/compatPath and call linkInk(benchAppNodeModules, compatPath,
"ink-compat") and do the same in prepare-real-ink.mjs (calling linkInk with its
target and label) so both scripts reuse the single linkInk helper (preserve
existing symbols __filename, __dirname, repoRoot, benchAppNodeModules,
inkLinkPath, compatPath computation in each script).
In `@scripts/ink-compat-bench/prepare-real-ink.mjs`:
- Around line 13-16: Before calling symlinkSync, verify that the target path
referenced by realInkPath exists (e.g., using fs.existsSync or fs.accessSync)
and if it does not, emit a clear error that `@jrichman/ink` is not installed and
abort (throw or process.exit) instead of creating a broken symlink; update the
block around realInkPath, rmSync, and symlinkSync to perform this presence check
and handle the missing-package case with a descriptive message.
In `@scripts/ink-compat-bench/summarize-cpuprofile.mjs`:
- Around line 5-10: The readArg function returns the following token even when
it's another flag (e.g., "--json"); update readArg to verify the next token
exists and is not a flag by checking that process.argv[idx+1] is defined and
does not start with "--" (align behavior with the guard used in
scripts/hsr/record-runner.mjs), and return the fallback if that check fails;
reference the readArg function to locate and update this logic.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f4341381a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/architecture/ink-compat.md (1)
86-103: Consider adding a live PTY + frame-audit verification step.The CI wiring checks are strong; adding a renderer parity check here would make the runbook more complete for regressions.
Suggested doc patch
## Wiring verification (recommended in CI) @@ 3. For bundled CLIs, rebuild the bundle after aliasing and validate expected compat-only markers in generated output. + +4. For rendering/layout/theme parity checks, validate with a live PTY run and collect frame-audit evidence (`REZI_FRAME_AUDIT` + `node scripts/frame-audit-report.mjs`).Based on learnings: For rendering/layout/theme regressions, validate with live PTY (not just unit snapshots) and collect frame audit evidence via
REZI_FRAME_AUDITlogs andnode scripts/frame-audit-report.mjs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/architecture/ink-compat.md` around lines 86 - 103, Add a live PTY renderer-parity step to the CI wiring verification: spawn the Ink-compatible renderer in a PTY, set REZI_FRAME_AUDIT=1 to emit frame-audit logs, run the provided verifier script (node scripts/frame-audit-report.mjs) to collect and assert frame/audit evidence, and fail the job on mismatches; reference REZI_FRAME_AUDIT and node scripts/frame-audit-report.mjs when adding the new check alongside the existing package-resolution and path-resolve commands so CI validates both module wiring and runtime renderer parity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ink-compat/README.md`:
- Around line 168-170: The README's "Debugging runbook" entry uses a relative
path `docs/dev/ink-compat-debugging.md` that won't resolve from
packages/ink-compat; update that link to the absolute docs URL used elsewhere in
this file (replace the `docs/dev/ink-compat-debugging.md` reference in the
"Debugging runbook" section with the canonical absolute docs link) so the README
points to the correct location.
---
Nitpick comments:
In `@docs/architecture/ink-compat.md`:
- Around line 86-103: Add a live PTY renderer-parity step to the CI wiring
verification: spawn the Ink-compatible renderer in a PTY, set REZI_FRAME_AUDIT=1
to emit frame-audit logs, run the provided verifier script (node
scripts/frame-audit-report.mjs) to collect and assert frame/audit evidence, and
fail the job on mismatches; reference REZI_FRAME_AUDIT and node
scripts/frame-audit-report.mjs when adding the new check alongside the existing
package-resolution and path-resolve commands so CI validates both module wiring
and runtime renderer parity.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.mddocs/architecture/ink-compat.mddocs/getting-started/faq.mddocs/index.mddocs/migration/ink-to-ink-compat.mddocs/migration/ink-to-rezi.mdmkdocs.ymlpackages/ink-compat/README.md
✅ Files skipped from review due to trivial changes (2)
- docs/migration/ink-to-rezi.md
- docs/migration/ink-to-ink-compat.md
Owner
Author
|
Addressed review feedback in:
Key fixes:
All previously open review threads are resolved. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/ink-compat/README.md (1)
154-167: Add explicit live-PTY frame-audit verification steps in Diagnostics.The diagnostics section is solid, but it should include the frame-audit workflow command so parity/perf regressions are validated beyond snapshots (e.g., mention
REZI_FRAME_AUDITlogs andnode scripts/frame-audit-report.mjs).Based on learnings: “For rendering/layout/theme regressions, validate with live PTY (not just unit snapshots) and collect frame audit evidence via
REZI_FRAME_AUDITlogs andnode scripts/frame-audit-report.mjs.”Also applies to: 178-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/README.md` around lines 154 - 167, Update the Diagnostics section to include explicit live-PTY frame-audit verification steps: instruct users to enable REZI_FRAME_AUDIT logging (e.g., export REZI_FRAME_AUDIT=1) during a live PTY run to capture frame-audit logs, run the frame-audit reporter with node scripts/frame-audit-report.mjs to produce human-readable audit reports, and mention collecting the generated log file alongside the existing INK_COMPAT_TRACE env gates; add a short note under the same Diagnostics area (also applied to the later Diagnostics block) describing when to use REZI_FRAME_AUDIT vs snapshot tests and what artifacts (log file + report) to attach for parity/perf regression investigations.packages/ink-compat/src/runtime/createInkRenderer.ts (3)
166-170: Storing segments without defensive copy relies on caller discipline.The
segmentsarray is now stored directly without.slice(). While thereadonlytype annotation signals immutability, callers could still mutate the array at runtime, corrupting the stored blob. If this is an intentional optimization (avoiding allocation), consider adding a brief comment documenting that callers must not mutate the array after passing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/runtime/createInkRenderer.ts` around lines 166 - 170, The addTextRunBlob method stores the caller-supplied segments array directly into this.textRunBlobs, risking later mutation; update addTextRunBlob (and the textRunBlobs usage) to either make a defensive copy of the incoming segments (e.g., push a shallow copy) before storing or, if avoiding allocation is intentional, add a concise comment on addTextRunBlob documenting that callers must not mutate the segments after passing them (and enforce readonly in types where possible); reference the addTextRunBlob method and the textRunBlobs field when making the change.
258-264: Double-buffering optimization partially negated by.slice().The swap mechanism avoids reallocating the builder's internal arrays each frame, but
out.slice()on line 263 still allocates a new array per frame. If callers can safely consume the returned array before the nextswapAndGetOps()call, returningoutdirectly (typed asreadonly InkRenderOp[]) would eliminate this allocation. Otherwise, consider documenting why the copy is necessary.💡 Potential zero-copy alternative (if safe)
swapAndGetOps(): readonly InkRenderOp[] { const out = this._ops; this._ops = this._prevOps; this._ops.length = 0; this._prevOps = out; - return out.slice(); + return out; // Caller must not retain reference past next swapAndGetOps() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/runtime/createInkRenderer.ts` around lines 258 - 264, The swapAndGetOps method currently returns out.slice(), causing a new array allocation each frame which negates the double-buffering; change swapAndGetOps to return out directly (typed as readonly InkRenderOp[]) after swapping _ops and _prevOps so no per-frame allocation happens, and audit callers of swapAndGetOps to ensure they consume/immutably read the returned array before the next swap (or make a defensive copy at callsite if they mutate it); if retaining the copy is required, add a clear comment above swapAndGetOps explaining why out.slice() is necessary for safety.
480-498: Unused debug helper should be removed.The
countSelfDirtyfunction is defined but never called in this file. The comment indicates it's temporary for investigation. Consider removing it to avoid dead code, or if still needed for debugging, guard it with a conditional or move it to a debug utility module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/runtime/createInkRenderer.ts` around lines 480 - 498, Remove the dead debug helper countSelfDirty since it’s defined but never used: either delete the entire function declaration for countSelfDirty or, if you still need it for local debugging, move it to a separate debug utility module (e.g., debug/utils) and export it, or wrap its definition behind a development-only guard (e.g., if (process.env.NODE_ENV !== 'production') { function countSelfDirty(...) { ... } }). Locate the function by the name countSelfDirty and apply one of the above fixes to eliminate unused code in createInkRenderer.packages/ink-compat/src/translation/propsToVNode.ts (1)
1025-1042: Please run live PTY/frame-audit validation for this text-rendering path.Given this touches wrap transforms and multiline rendering behavior, verify with live PTY and frame-audit logs in addition to snapshots.
Based on learnings: For rendering/layout/theme regressions, validate with live PTY and collect frame-audit evidence via
REZI_FRAME_AUDITlogs andnode scripts/frame-audit-report.mjs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/translation/propsToVNode.ts` around lines 1025 - 1042, This change affects soft-wrap and multiline rendering (see INK_SOFT_WRAP_TRANSFORM, textProps, fullText check and translateMultilineRichText), so run live PTY validation and collect frame-audit evidence: execute the component in a real terminal session exercising wrap/truncate variants, enable REZI_FRAME_AUDIT logs, and generate the frame-audit report via node scripts/frame-audit-report.mjs; attach the PTY run outputs, REZI_FRAME_AUDIT logs, and the report to the PR so we can verify there are no layout/theme regressions from the soft-wrap and multiline code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ink-compat/README.md`:
- Around line 172-177: Add a new item under "Known boundaries" in
packages/ink-compat README noting that the Rgb24 sentinel value 0 (Rgb24=0) is
intentionally treated as unset/terminal-default, so fg/bg values of 0 do not
represent explicit black (0x000000); document that explicit black is not
representable and recommend using a near-black value like 0x010101 when a true
black is required. Reference the Rgb24 sentinel and the ink-compat migration
context so readers understand this common gotcha.
---
Nitpick comments:
In `@packages/ink-compat/README.md`:
- Around line 154-167: Update the Diagnostics section to include explicit
live-PTY frame-audit verification steps: instruct users to enable
REZI_FRAME_AUDIT logging (e.g., export REZI_FRAME_AUDIT=1) during a live PTY run
to capture frame-audit logs, run the frame-audit reporter with node
scripts/frame-audit-report.mjs to produce human-readable audit reports, and
mention collecting the generated log file alongside the existing
INK_COMPAT_TRACE env gates; add a short note under the same Diagnostics area
(also applied to the later Diagnostics block) describing when to use
REZI_FRAME_AUDIT vs snapshot tests and what artifacts (log file + report) to
attach for parity/perf regression investigations.
In `@packages/ink-compat/src/runtime/createInkRenderer.ts`:
- Around line 166-170: The addTextRunBlob method stores the caller-supplied
segments array directly into this.textRunBlobs, risking later mutation; update
addTextRunBlob (and the textRunBlobs usage) to either make a defensive copy of
the incoming segments (e.g., push a shallow copy) before storing or, if avoiding
allocation is intentional, add a concise comment on addTextRunBlob documenting
that callers must not mutate the segments after passing them (and enforce
readonly in types where possible); reference the addTextRunBlob method and the
textRunBlobs field when making the change.
- Around line 258-264: The swapAndGetOps method currently returns out.slice(),
causing a new array allocation each frame which negates the double-buffering;
change swapAndGetOps to return out directly (typed as readonly InkRenderOp[])
after swapping _ops and _prevOps so no per-frame allocation happens, and audit
callers of swapAndGetOps to ensure they consume/immutably read the returned
array before the next swap (or make a defensive copy at callsite if they mutate
it); if retaining the copy is required, add a clear comment above swapAndGetOps
explaining why out.slice() is necessary for safety.
- Around line 480-498: Remove the dead debug helper countSelfDirty since it’s
defined but never used: either delete the entire function declaration for
countSelfDirty or, if you still need it for local debugging, move it to a
separate debug utility module (e.g., debug/utils) and export it, or wrap its
definition behind a development-only guard (e.g., if (process.env.NODE_ENV !==
'production') { function countSelfDirty(...) { ... } }). Locate the function by
the name countSelfDirty and apply one of the above fixes to eliminate unused
code in createInkRenderer.
In `@packages/ink-compat/src/translation/propsToVNode.ts`:
- Around line 1025-1042: This change affects soft-wrap and multiline rendering
(see INK_SOFT_WRAP_TRANSFORM, textProps, fullText check and
translateMultilineRichText), so run live PTY validation and collect frame-audit
evidence: execute the component in a real terminal session exercising
wrap/truncate variants, enable REZI_FRAME_AUDIT logs, and generate the
frame-audit report via node scripts/frame-audit-report.mjs; attach the PTY run
outputs, REZI_FRAME_AUDIT logs, and the report to the PR so we can verify there
are no layout/theme regressions from the soft-wrap and multiline code paths.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/architecture/ink-compat.mdpackages/bench-harness/src/screen.tspackages/bench-runner/src/cli.tspackages/core/src/testing/__tests__/testRenderer.test.tspackages/core/src/testing/renderer.tspackages/ink-compat/README.mdpackages/ink-compat/src/runtime/createInkRenderer.tspackages/ink-compat/src/translation/propsToVNode.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/testing/tests/testRenderer.test.ts
- docs/architecture/ink-compat.md
- packages/bench-runner/src/cli.ts
Comment on lines
172
to
+177
| ## Known boundaries | ||
|
|
||
| - Minor visual differences can occur across terminal emulators and OS TTY behavior. | ||
| - App-version and install-mode messaging differences are expected and not renderer bugs. | ||
| - Gradient interpolation can differ slightly from upstream while preserving overall behavior. | ||
| - Minor visual differences can occur across terminal emulators / OS TTY behavior. | ||
| - App/version-specific messaging differences are expected and are not renderer bugs. | ||
| - Gradient interpolation can differ slightly while preserving overall behavior. | ||
|
|
There was a problem hiding this comment.
Document the Rgb24=0 sentinel caveat in Known boundaries.
Please add a boundary note that fg/bg = 0 is treated as terminal-default/unset, so explicit black (0x000000) is not representable in ink-compat migration (use near-black like 0x010101 when needed). This is a common migration gotcha and belongs in this README.
Based on learnings: “In packages/ink-compat, Rgb24 value 0 is an intentional sentinel meaning unset/terminal default… Explicit black (0x000000) is not representable.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ink-compat/README.md` around lines 172 - 177, Add a new item under
"Known boundaries" in packages/ink-compat README noting that the Rgb24 sentinel
value 0 (Rgb24=0) is intentionally treated as unset/terminal-default, so fg/bg
values of 0 do not represent explicit black (0x000000); document that explicit
black is not representable and recommend using a near-black value like 0x010101
when a true black is required. Reference the Rgb24 sentinel and the ink-compat
migration context so readers understand this common gotcha.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
npx biome check docs/architecture/ink-compat.md packages/bench-app/src/entry.tsx packages/bench-runner/src/cli.ts packages/bench-runner/src/verify.ts packages/core/src/renderer/__tests__/renderer.text.test.ts packages/core/src/renderer/renderToDrawlist/widgets/renderTextWidgets.ts packages/ink-compat/README.mdnpm run -s typechecknode scripts/run-tests.mjs --filter renderer.textNotes
npm run -s checkcurrently fails incheck-create-rezi-templatesforpackages/create-rezi/templates/animation-lab/src/screens/reactor-lab.ts(template TS errors outside files touched in this cleanup commit).Summary by CodeRabbit
New Features
Performance
Documentation
Tests
Chores