Fix post-refactor regressions and bump native vendor to Zireael #103#223
Merged
RtlZeroMemory merged 20 commits intomainfrom Feb 27, 2026
Merged
Fix post-refactor regressions and bump native vendor to Zireael #103#223RtlZeroMemory merged 20 commits intomainfrom
RtlZeroMemory merged 20 commits intomainfrom
Conversation
- Add concurrency group to ci.yml and codeql.yml to cancel in-progress runs on re-push (biggest win for runner contention) - Extract lint/typecheck/codegen/portability/unicode into a dedicated `checks` job that gates the matrix — lint failures caught in ~2 min instead of after the full 15-min pipeline - Dynamic matrix: 5 runners on PRs (Linux × Node 18/20/22, macOS × 22, Windows × 22), full 3×3 on push to main - Remove redundant lint/typecheck/codegen/biome-install steps from each matrix cell and the bun job - Remove duplicate docs job (already handled by docs.yml) Net effect on PRs: 13 jobs → 9, ~44% fewer runners, fast-fail on static checks, stale runs cancelled automatically. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…trix" This reverts commit 183532e.
…ized-renderer # Conflicts: # packages/core/src/abi.ts # packages/core/src/app/__tests__/partialDrawlistEmission.test.ts # packages/core/src/app/createApp.ts # packages/core/src/app/rawRenderer.ts # packages/core/src/app/widgetRenderer.ts # packages/core/src/drawlist/__tests__/builder.build-into.test.ts # packages/core/src/drawlist/__tests__/builder.cursor.test.ts # packages/core/src/drawlist/__tests__/builder.round-trip.test.ts # packages/core/src/drawlist/__tests__/writers.gen.test.ts # packages/core/src/drawlist/builder.ts # packages/core/src/drawlist/builder_v1.ts # packages/core/src/drawlist/builder_v2.ts # packages/core/src/drawlist/index.ts # packages/core/src/drawlist/types.ts # packages/core/src/index.ts # packages/core/src/renderer/renderToDrawlist/widgets/renderCanvasWidgets.ts # packages/node/src/__tests__/config_guards.test.ts # packages/node/src/backend/nodeBackend.ts # packages/node/src/backend/nodeBackendInline.ts
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (29)
📝 WalkthroughWalkthroughConsolidates drawlist builders into a single builder, introduces DEF_STRING/DEF_BLOB resource ops and shared decoders, migrates colors to packed Rgb24, adds optional frame-audit instrumentation and native vendor integrity CI checks, and adds a regression-dashboard example with tests. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Renderer
participant Backend
participant Worker
rect rgba(200,200,255,0.5)
App->>Renderer: render frame -> build drawlist (createDrawlistBuilder)
Renderer->>Renderer: compute fingerprint (drawlistFingerprint)
Renderer->>Backend: beginFrame / submit drawlist (BEGIN_FRAME or publish)
Backend->>Worker: transfer or SAB publish (frame payload)
Worker->>Backend: ack/accept (accepted/completed)
Backend->>Renderer: accepted/completed event (frame lifecycle)
Renderer->>App: emit frame-audit events (emitFrameAudit)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
![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: 47375c9ea7
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/core/src/widgets/__tests__/graphicsWidgets.test.ts (2)
471-483:⚠️ Potential issue | 🟡 MinorAvoid raw VNode literals; use
ui.image()in this test case.The same test file already uses
ui.image({...})on line 461. The raw{ kind, props }object on line 473 is inconsistent with that pattern and violates the widget-construction invariant. All widget construction should go through theui.*()factory functions.♻️ Proposed fix
- const bytes = renderBytes( - { - kind: "image", - props: { - src: "bad-bytes" as unknown as Uint8Array, - width: 20, - height: 4, - alt: "Broken image", - }, - }, - () => createDrawlistBuilder(), - ); + const bytes = renderBytes( + ui.image({ + src: "bad-bytes" as unknown as Uint8Array, + width: 20, + height: 4, + alt: "Broken image", + }), + () => createDrawlistBuilder(), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/__tests__/graphicsWidgets.test.ts` around lines 471 - 483, The test "image invalid source falls back without DRAW_IMAGE" is constructing a raw VNode literal; replace that literal with the widget factory call ui.image(...) so the test uses the same invariant as other tests (see ui.image used on line 461). Update the renderBytes invocation to pass ui.image({ src: "bad-bytes" as unknown as Uint8Array, width: 20, height: 4, alt: "Broken image" }) instead of the { kind: "image", props: { ... } } object and keep the createDrawlistBuilder() argument unchanged.
114-147:⚠️ Potential issue | 🟠 MajorUse
createTestRenderer()as the test entrypoint.This helper directly wires commit/layout/render, but test files in this repo are required to render through the testing module API.
#!/bin/bash # Verify this file uses low-level rendering pipeline instead of createTestRenderer. rg -n -C2 'createTestRenderer|commitVNodeTree|renderToDrawlist|layout\(' packages/core/src/widgets/__tests__/graphicsWidgets.test.tsAs per coding guidelines
**/__tests__/**/*.test.{ts,tsx}: Test with createTestRenderer() from the testing module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/__tests__/graphicsWidgets.test.ts` around lines 114 - 147, The test uses the low-level pipeline functions (renderBytes calling commitVNodeTree, layout, renderToDrawlist and builder.build) instead of the required testing API; replace the custom renderBytes path with the testing helper createTestRenderer() so the test renders via the module entrypoint: locate the renderBytes helper and update tests to call createTestRenderer() (or refactor renderBytes to wrap createTestRenderer()) so commit/layout/render are driven by createTestRenderer rather than directly invoking commitVNodeTree, layout, or renderToDrawlist; ensure the viewport and terminalProfile parameters are passed through the createTestRenderer invocation to preserve existing behavior.packages/ink-compat/src/translation/colorMap.ts (1)
4-22:⚠️ Potential issue | 🟠 MajorExplicit black (0) is suppressed as unset; all parse paths must normalize to near-black.
The migration creates multiple code paths that produce
rgb(0, 0, 0): named color"black"(line 5), hex formats"#000"/"#000000"(lines 79–94),"rgb(0, 0, 0)"literals (lines 96–109), andansi256(0)(line 114). All pack to value0, which is the sentinel for "unset/terminal default" in ink-compat. InstyleToSgr(render.ts lines 1220, 1231), falsy checks onfgandbgskip emitting color SGR codes entirely, so explicit black disappears on output.Normalize all explicit-black inputs to near-black (e.g.,
0x010101) to preserve rendering intent and avoid collision with the unset sentinel.Suggested approach
+const EXPLICIT_BLACK: Rgb24 = rgb(1, 1, 1); + +function normalizeExplicitBlack(value: Rgb24 | undefined): Rgb24 | undefined { + return value === 0 ? EXPLICIT_BLACK : value; +}Then apply to the return points in
parseColorInner(e.g., after line 65, line 86, line 94, line 109) and indecodeAnsi256Color(e.g., line 114).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/translation/colorMap.ts` around lines 4 - 22, Named "black" and other explicit-black parse paths currently produce rgb(0,0,0) which collides with the unset sentinel; update the color normalization so any explicit-black result is mapped to near-black (0x010101) before returning. Concretely: adjust NAMED_COLORS (symbol) or apply a post-parse normalization in parseColorInner (all return points that return rgb(0,0,0) from named, hex, rgb-literal branches) to convert {r:0,g:0,b:0} to {r:1,g:1,b:1}, and do the same in decodeAnsi256Color for the case that returns black (ansi256 0) so callers like styleToSgr will see a non-falsy color and emit SGR codes.packages/core/src/drawlist/__tests__/builder.cursor.test.ts (1)
1-10:⚠️ Potential issue | 🟡 MinorDocumentation comment inconsistency with code.
The JSDoc comment at line 5 mentions "Correct v2 header version" but the test at line 49 verifies
ZR_DRAWLIST_VERSION_V1. The comment should be updated to reflect the current version.📝 Suggested fix
/** - * Unit tests for DrawlistBuilder SET_CURSOR command encoding. + * Unit tests for DrawlistBuilder SET_CURSOR command encoding (ZRDL v1). * * Verifies: - * - Correct v2 header version + * - Correct v1 header version * - SET_CURSOR command byte layout matches C struct🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/__tests__/builder.cursor.test.ts` around lines 1 - 10, Update the top JSDoc to match the actual assertion: change the header note from "Correct v2 header version" to "Correct v1 header version" (or otherwise reflect V1) so it aligns with the test that checks ZR_DRAWLIST_VERSION_V1 in builder.cursor.test.ts; locate the JSDoc block at the top of the file and edit the sentence to reference v1 header version.
🟡 Minor comments (12)
packages/core/src/perf/perf.ts-247-249 (1)
247-249:⚠️ Potential issue | 🟡 MinorUse a null-prototype dictionary for dynamic counter keys.
Building
counterswith{}can behave unexpectedly for special keys. UseObject.create(null)for safer key/value materialization.🛡️ Proposed fix
- const counters: Record<string, number> = {}; + const counters = Object.create(null) as Record<string, number>; for (const [name, value] of this.counters) counters[name] = value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/perf/perf.ts` around lines 247 - 249, The current dynamic materialization uses a plain object for counters which can collide with inherited properties; change the local counters map in the method that reads this.counters to use a null-prototype object (create with Object.create(null) and cast to Record<string, number> as needed) instead of {} so keys are safe, then populate it from this.counters and return as before with Object.freeze(phases) and Object.freeze(counters); update the declaration of the local counters variable referenced in this function to use the null-prototype creation.packages/core/src/widgets/__tests__/style.merge-fuzz.test.ts-25-29 (1)
25-29:⚠️ Potential issue | 🟡 Minor
attrsvalue correctness is no longer asserted.After Line [28] made
attrsoptional, Line [195] only checks type. That allows packed-attrs regressions to pass while boolean field assertions still succeed. Please restore value-levelattrsverification with an independent expected-attrs calculation.Based on learnings: Changes to drawlist generation code are a danger zone requiring extra care and full test suite verification.
Also applies to: 176-178, 194-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/__tests__/style.merge-fuzz.test.ts` around lines 25 - 29, The test removed a value-level assertion for the packed attrs field after making MutableResolvedTextStyle.attrs optional, which allows regressions to slip by; restore an independent expected-attrs calculation in the fuzz test and assert equality against the actual attrs value (not just its type). Locate the assertions that check resolved text style boolean fields and the MutableResolvedTextStyle shape (references: MutableResolvedTextStyle and the test block that computes expected resolved style) and add code to compute expectedAttrs from the boolean flags the same way production does, then assert expectedAttrs === actual.attrs (or treat undefined as 0 if the code packs that way) in addition to the existing boolean and type checks. Ensure this explicit packed-attrs verification is added in the same test cases that validate boolean fields so regressions in drawlist/packing are caught.packages/create-rezi/templates/starship/src/main.ts-629-649 (1)
629-649:⚠️ Potential issue | 🟡 MinorPolling loop can overlap async debug queries.
setIntervalschedulespump()regardless of whether the prior query finished, so slow queries can overlap and race on shared debug state.Suggested fix (in-flight guard)
- const pump = async () => { - if (!debugController) return; + let pumpInFlight = false; + const pump = async () => { + if (!debugController || pumpInFlight) return; + pumpInFlight = true; try { const records = await debugController.query({ maxRecords: 256 }); if (records.length === 0) return; for (const record of records) { if (record.header.recordId <= debugLastRecordId) continue; debugLastRecordId = record.header.recordId; snapshotDebugRecord(record); } } catch (error) { debugSnapshot("runtime.debug.query.error", { message: error instanceof Error ? error.message : String(error), route: currentRouteId(), }); + } finally { + pumpInFlight = false; } };As per coding guidelines: all TypeScript changes must follow
docs/dev/code-standards.md, including async cancellation/error-handling guardrails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-rezi/templates/starship/src/main.ts` around lines 629 - 649, The polling loop started by debugTraceTimer can overlap calls to pump because setInterval fires regardless of pump completion; fix by adding an in-flight guard or switching to a self-scheduling async loop so only one pump runs at a time: introduce a boolean flag (e.g., debugPumpInFlight) or a mutex checked/set at the start of pump and cleared on exit (including in catch/finally), or replace the setInterval with an async function that awaits pump then uses setTimeout to schedule the next run; ensure you reference and update the same shared state variables used in pump (debugController, debugLastRecordId, snapshotDebugRecord) and still clear the timer/stop the loop when debugging is cancelled.packages/core/src/app/__tests__/rawRender.test.ts-37-39 (1)
37-39:⚠️ Potential issue | 🟡 MinorMirror
buildInto()state transitions withbuild()in the stub.
makeStubBuilder().buildInto()always succeeds and never flipsbuilt, so this test can miss accidental double-build behavior inRawRenderer.🔧 Suggested patch
buildInto() { - return { ok: true, bytes } as const; + if (built) + return { ok: false, error: { code: "ZRDL_INTERNAL", detail: "built twice" } } as const; + built = true; + return { ok: true, bytes } as const; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/__tests__/rawRender.test.ts` around lines 37 - 39, The stub builder's buildInto() always succeeds and never flips the internal built state, so update the test stub created by makeStubBuilder() so buildInto() mirrors build()'s state transitions: have buildInto() check the same built flag and return a failure result when already built, and set built = true on successful first call (or simply delegate to the existing build() implementation if present). This ensures RawRenderer double-build behavior is exercised; locate makeStubBuilder, build(), and buildInto() in the test file and make buildInto follow build's logic (respecting the built flag and returning the same ok/error shape).packages/core/src/perf/frameAudit.ts-95-97 (1)
95-97:⚠️ Potential issue | 🟡 MinorTighten command-stream validation with alignment checks.
cmdStreamValidcan currently report true for non-4-byte-aligned command streams. Add alignment checks oncmdOffset,cmdBytes,off, andsizeto keep audit validity consistent with drawlist format invariants.Suggested fix
- if (cmdOffset < 0 || cmdBytes < 0 || cmdOffset + cmdBytes > bytes.byteLength) { + if ( + cmdOffset < 0 || + cmdBytes < 0 || + (cmdOffset & 3) !== 0 || + (cmdBytes & 3) !== 0 || + cmdOffset + cmdBytes > bytes.byteLength + ) { return Object.freeze({ histogram: Object.freeze({}), valid: false }); } @@ - if (size < 8 || off + size > end) { + if ((off & 3) !== 0 || (size & 3) !== 0 || size < 8 || off + size > end) { return Object.freeze({ histogram: Object.freeze(hist), valid: false }); }Also applies to: 103-110, 115-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/perf/frameAudit.ts` around lines 95 - 97, The validation branch that sets cmdStreamValid needs 4-byte alignment checks: ensure cmdOffset and cmdBytes are non-negative, cmdOffset + cmdBytes <= bytes.byteLength, and both cmdOffset and cmdBytes are 4-byte aligned (e.g. cmdOffset % 4 === 0 && cmdBytes % 4 === 0); if any fail return the same frozen invalid result. Likewise, in the other blocks that validate draw commands (the checks using off and size around lines handling individual commands), add alignment checks for off and size (off % 4 === 0 && size % 4 === 0) in addition to their existing bounds checks and return the frozen invalid result when misaligned so cmdStreamValid remains consistent with the drawlist format invariants.packages/core/src/renderer/__tests__/renderer.text.test.ts-165-167 (1)
165-167:⚠️ Potential issue | 🟡 MinorGuard
OP_FREE_BLOBpayload reads with a command-size check.
u32(bytes, off + 8)is executed without validating payload size; malformed frames can throw unexpectedly during parsing.💡 Suggested fix
if (cmd.opcode === OP_FREE_BLOB) { - const blobId = u32(bytes, off + 8); - if (blobId > 0) textRunBlobsByIndex[blobId - 1] = Object.freeze({ segments: Object.freeze([]) }); + if (cmd.size >= 12) { + const blobId = u32(bytes, off + 8); + if (blobId > 0) { + textRunBlobsByIndex[blobId - 1] = Object.freeze({ segments: Object.freeze([]) }); + } + } continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/renderer.text.test.ts` around lines 165 - 167, The code reads u32(bytes, off + 8) for OP_FREE_BLOB without ensuring the command payload is large enough; guard this read by checking the command size/available bytes before calling u32. Before calling u32(bytes, off + 8) inside the OP_FREE_BLOB branch, verify that cmd.size (or the available bytes length from off) is at least 12 (off + 8 + 4) and only then compute blobId and touch textRunBlobsByIndex; otherwise skip the operation or treat it as a malformed frame. This uses the existing symbols OP_FREE_BLOB, cmd, bytes, off, u32, and textRunBlobsByIndex to locate and fix the check.packages/core/src/widgets/__tests__/basicWidgets.render.test.ts-70-76 (1)
70-76:⚠️ Potential issue | 🟡 MinorFail fast on DRAW_TEXT decode/header desync.
Line 75 currently falls back to
""whendecoded[drawTextIndex]is missing, which can mask parser drift and make tests pass for the wrong reason.Suggested fix
- const decodedCmd = decoded[drawTextIndex]; + const decodedCmd = decoded[drawTextIndex]; + assert.ok(decodedCmd, `missing decoded DRAW_TEXT at index ${String(drawTextIndex)}`); drawTextIndex += 1; @@ - text: decodedCmd?.text ?? "", + text: decodedCmd.text,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/__tests__/basicWidgets.render.test.ts` around lines 70 - 76, The test currently masks a desync by defaulting to an empty string when decoded[drawTextIndex] is missing; instead, update the DRAW_TEXT handling in the test so it fails fast: check that decoded[drawTextIndex] is defined (use the existing variables decoded and drawTextIndex) and throw a clear error or assert with context (e.g., include off and drawTextIndex or the expected header/size) if it's undefined, rather than falling back to "". This ensures parser/header desyncs are surfaced immediately in basicWidgets.render tests.packages/core/src/drawlist/__tests__/builder.alignment.test.ts-129-133 (1)
129-133:⚠️ Potential issue | 🟡 MinorUse the returned
blobIndexinstead of hardcoding0indrawTextRun.This test computes
blobIndexbut ignores it on Line 133, which makes the assertion more fragile than needed.Suggested fix
const blobIndex = b.addBlob(new Uint8Array([9, 8, 7, 6])); assert.equal(blobIndex, 0); +if (blobIndex === null) throw new Error("expected blob index"); b.clear(); b.drawText(0, 0, "x"); -b.drawTextRun(2, 1, 0); +b.drawTextRun(2, 1, blobIndex);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/__tests__/builder.alignment.test.ts` around lines 129 - 133, The test computes a blobIndex via b.addBlob(...) but then hardcodes 0 when calling b.drawTextRun(2, 1, 0); update the call to use the computed blobIndex instead of 0 (i.e., call b.drawTextRun(2, 1, blobIndex)) so the test uses the actual returned value from addBlob and is not fragile; verify this change in the test that uses addBlob, blobIndex, drawText, and drawTextRun.examples/regression-dashboard/src/screens/overview.ts-60-61 (1)
60-61:⚠️ Potential issue | 🟡 MinorUse state timestamps instead of
Date.now()during render.Using wall-clock time in render makes the screen non-deterministic and can cause noisy/flickery update-rate output.
🛠️ Proposed fix
- const uptimeSec = Math.max(1, Math.floor((Date.now() - state.startedAtMs) / 1000)); + const elapsedMs = Math.max(0, state.lastUpdatedMs - state.startedAtMs); + const uptimeSec = Math.max(1, Math.floor(elapsedMs / 1000));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/regression-dashboard/src/screens/overview.ts` around lines 60 - 61, Replace the use of Date.now() with the renderable timestamp from state so the UI is deterministic: compute uptimeSec as Math.max(1, Math.floor((state.nowMs - state.startedAtMs) / 1000)) (using state.nowMs instead of Date.now()), and then compute updateRate from state.tick / uptimeSec; ensure you reference the existing symbols uptimeSec, updateRate, state.startedAtMs and state.nowMs so the component uses the state-provided time source.packages/core/src/__tests__/drawlistDecode.ts-263-269 (1)
263-269:⚠️ Potential issue | 🟡 MinorAdd explicit DEF payload bounds checks in command decoders.
These branches currently rely on
subarray(...), which truncates silently on malformed lengths. That can mask protocol corruption in tests.🔧 Suggested patch
case OP_DEF_STRING: { + if (cmd.size < 16) { + throw new Error(`DEF_STRING too small at offset ${String(cmd.offset)} size=${String(cmd.size)}`); + } const id = u32(bytes, cmd.offset + 8); const byteLen = u32(bytes, cmd.offset + 12); const dataStart = cmd.offset + 16; const dataEnd = dataStart + byteLen; + if (dataEnd > cmd.offset + cmd.size) { + throw new Error( + `DEF_STRING payload overflow at offset ${String(cmd.offset)} (len=${String(byteLen)}, size=${String(cmd.size)})`, + ); + } strings.set(id, cloneBytes(bytes.subarray(dataStart, dataEnd))); break; } @@ case OP_DEF_BLOB: { + if (cmd.size < 16) { + throw new Error(`DEF_BLOB too small at offset ${String(cmd.offset)} size=${String(cmd.size)}`); + } const id = u32(bytes, cmd.offset + 8); const byteLen = u32(bytes, cmd.offset + 12); const dataStart = cmd.offset + 16; const dataEnd = dataStart + byteLen; + if (dataEnd > cmd.offset + cmd.size) { + throw new Error( + `DEF_BLOB payload overflow at offset ${String(cmd.offset)} (len=${String(byteLen)}, size=${String(cmd.size)})`, + ); + } blobs.set(id, cloneBytes(bytes.subarray(dataStart, dataEnd))); break; }Also applies to: 316-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/__tests__/drawlistDecode.ts` around lines 263 - 269, The OP_DEF_STRING decoder branch (and the similar branch around lines 316-322) currently uses subarray and can silently truncate malformed payloads; update the decoder to validate payload bounds before slicing: compute id = u32(bytes, cmd.offset + 8), byteLen = u32(bytes, cmd.offset + 12) and dataStart = cmd.offset + 16, then check that byteLen is non-negative and that dataEnd = dataStart + byteLen is <= bytes.length (and >= dataStart) and that cmd.offset + 16 is within the buffer; if any check fails, throw a descriptive error (rather than calling subarray), otherwise use cloneBytes(bytes.subarray(dataStart, dataEnd)) and set the string. Ensure the same explicit bounds checks are applied to the other DEF payload decoder(s) referenced.packages/core/src/runtime/commit.ts-1788-1800 (1)
1788-1800:⚠️ Potential issue | 🟡 MinorRemove debug tracing code before merge.
The comment explicitly states "Temporary debug: trace commit matching (remove after investigation)". This debug infrastructure adds runtime overhead on every
commitNodecall (globalThis property lookup and type cast) even when disabled. Given that this is the core reconciliation hot path, this should be removed before merging to main.🗑️ Proposed fix: remove debug code
- // Temporary debug: trace commit matching (remove after investigation) - const commitDebug = globalThis as Record<string, unknown> & { - __commitDebug?: unknown; - __commitDebugLog?: string[] | undefined; - }; - if (commitDebug.__commitDebug) { - const debugLog = commitDebug.__commitDebugLog; - if (debugLog) { - debugLog.push( - `commitNode(${String(instanceId)}, ${vnode.kind}, prev=${prev ? `${prev.vnode.kind}:${String(prev.instanceId)}` : "null"})`, - ); - } - } - // Leaf nodes — fast path: reuse previous RuntimeInstance when content is unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/commit.ts` around lines 1788 - 1800, Remove the temporary debug tracing block in commitNode that inspects globalThis for __commitDebug and pushes strings into __commitDebugLog; specifically delete the commitDebug declaration and the if (commitDebug.__commitDebug) { ... } branch that references __commitDebug, __commitDebugLog, instanceId, vnode, and prev so the commitNode hot path no longer performs globalThis lookups or log pushes.packages/core/src/runtime/commit.ts-1334-1386 (1)
1334-1386:⚠️ Potential issue | 🟡 MinorIn-place RuntimeInstance mutation is intentional but requires full test verification.
The mutation pattern on lines 1337–1338 and 1381–1382 is a deliberate performance optimization to preserve reference identity and prevent cascading parent updates. While
RuntimeInstance.vnodecan be reassigned (it's not markedreadonly),VNodeitself is immutable (Readonly<...>structure), so mutations only update the reference, not VNode contents.Downstream code (layout, renderer, widgetMeta) reads from
.vnodeproperties transiently without caching references, and tests explicitly verify this behavior (seecommit.fastReuse.regression.test.ts). This pattern is safe in practice.Critical requirement: After any changes to this mutation logic, run the full test suite with
node scripts/run-tests.mjsbefore committing. Changes to reconciliation/commit paths are a danger zone—subtle regressions are common and must be caught by comprehensive testing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/commit.ts` around lines 1334 - 1386, The code mutates RuntimeInstance.vnode and children references in-place (see rewriteCommittedVNode, canFastReuseContainerSelf, and assignments to prev.vnode and prev.children) as a deliberate fast-reuse optimization; keep that behavior but ensure test coverage and run the full test suite. Preserve the in-place mutation logic: when fast-reusing, assign prev.vnode = rewriteCommittedVNode(...) and prev.children = nextChildren and set prev.selfDirty/prev.dirty appropriately (as done in the fast-reuse and in-place mutation branches referencing prev, vnode, prevChildren, nextChildren, committedChildVNodes); do not make vnode or children readonly or replace with new RuntimeInstance objects, and after any edits to these branches run node scripts/run-tests.mjs to verify commit/reconciliation/regression tests (e.g., commit.fastReuse.regression.test.ts) pass. Ensure commit diagnostic (__commitDiag) updates remain correct when toggling these 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 `@examples/regression-dashboard/src/main.ts`:
- Around line 167-176: The current uncaughtException and unhandledRejection
handlers only set process.exitCode and may let the event loop keep the process
alive; change both handlers (the process.on("uncaughtException", ...) and
process.on("unhandledRejection", ...) blocks) to force synchronous/immediate
termination after logging by calling process.exit(1) (or calling process.exit in
a next tick if you must flush logs, e.g., stderrLog(...); setImmediate(() =>
process.exit(1))); apply the same change to the duplicate handler block
referenced at lines 183-188 so the process cannot continue running after these
fatal conditions.
In `@packages/core/src/perf/perf.ts`:
- Around line 232-234: The count method on the Perf tracker allows arbitrary
names which can make this.counters grow unbounded; modify count(name: string,
delta = 1) to guard cardinality by enforcing a maximum distinct key count (e.g.,
MAX_COUNTER_KEYS constant) and a fallback for new keys once the limit is reached
(such as incrementing a special "__other__" bucket or
rejecting/high-frequency-sampled keys), and ensure snapshot() and any consumers
handle the fallback key; update references to this.counters, the count method,
and snapshot to use the capped behavior and add clear tests for hitting the cap.
In `@packages/core/src/renderer/renderToDrawlist/overflowCulling.ts`:
- Around line 48-51: The transparent-wrapper check in
isTransparentOverflowWrapper is too narrow and must include the renderer's
transparent container kinds so subtreeCanOverflowBounds can descend into them;
update isTransparentOverflowWrapper (and any logic in subtreeCanOverflowBounds)
to also treat vnode.kind === "layers" as transparent and treat vnode.kind ===
"layer" as transparent when that layer is in pass-through/transparent mode
(check the layer vnode props that indicate pass-through/transparent mode), so
overflowing descendants inside "layers" and pass-through "layer" are not
short-circuited.
In `@packages/core/src/renderer/shadow.ts`:
- Around line 47-51: DEFAULT_SHADOW.color is currently rgb(0,0,0) which packs to
numeric 0 and gets suppressed by the truthy checks in styleToSgr
(style.fg/style.bg), so change DEFAULT_SHADOW.color to a non-zero dark value
(e.g. rgb(1,1,1)) in the DEFAULT_SHADOW object to ensure the shadow color is
emitted; alternatively, adjust styleToSgr to treat packed zero as a valid color,
but the quick fix is updating DEFAULT_SHADOW.color to rgb(1,1,1).
In `@packages/core/src/theme/heatmapPalettes.ts`:
- Around line 45-47: The first grayscale stop currently uses rgb(0, 0, 0) which
packs to the reserved sentinel value 0 (unset/default); update the start stop in
the grayscale Object.freeze array so it does not use the zero sentinel (e.g.,
replace rgb(0, 0, 0) with a non-zero near-black like rgb(1, 1, 1) or another
minimally different value), leaving the t:0 entry and the rest of the array
unchanged; update any tests/comments referencing explicit black if present.
In `@packages/core/src/theme/interop.ts`:
- Around line 142-147: setColor currently produces numeric Rgb24 values where 0
is a valid sentinel, but consumers may be using truthy checks (e.g., if (base) /
if (primary)), which will drop 0; update all consumers of setColor’s return and
alias wiring to test explicitly for undefined (e.g., value === undefined or
value !== undefined) or use typeof value === 'number' instead of truthiness, and
ensure any aliasing logic that reads from the out map (the Record<string, Rgb24>
named out, keys referenced by key) treats 0 as a valid color rather than falsy
so aliases are not left stale.
- Around line 92-93: isRgb currently returns true for any finite number, letting
non-integer or out-of-range values slip into Theme.colors via flat overrides;
change the type guard isRgb(v: unknown): v is Rgb24 to only accept integers in
the 0–255 range (Number.isInteger(v) && v >= 0 && v <= 255) so only valid 8‑bit
channel values are treated as Rgb24; also ensure downstream
normalization/packing code that assumes packed RGB still runs for values that
fail this guard.
In `@packages/core/src/widgets/heatmap.ts`:
- Around line 23-27: The heatmap may emit the sentinel Rgb24 value 0 (0x000000)
which ink-compat treats as "unset"; update buildScaleTable and any color
interpolation/packing code that produces Rgb24 (e.g., where left/right stops are
chosen and where interpolated rgb values are packed) to guarantee the packed
Rgb24 is never 0: replace any default black stop rgb(0,0,0) with a non-zero
sentinel (or clamp/offset the final packed value to at least 1) and ensure the
packRgb/serialize path post-interpolation enforces value !== 0 so explicit black
is represented without using the reserved zero sentinel.
In `@packages/create-rezi/templates/starship/src/main.ts`:
- Around line 606-621: setupDebugTrace currently awaits
debugController.enable/reset and will throw on failure, which can block startup
since it's called before app.start; modify setupDebugTrace (and the same pattern
at the other occurrence) to guard all async debug calls (debugController.enable
and debugController.reset) with try/catch, log or processLogger.error the
failure including the error object, and ensure the function returns gracefully
(disable/cleanup the debugController or set a flag like debugController =
undefined) rather than rethrowing so app.start is never prevented by debug
instrumentation errors. Also ensure callers (where setupDebugTrace is awaited)
tolerate a non-throwing, failed debug initialization (i.e., proceed even if
debugController is unset).
---
Outside diff comments:
In `@packages/core/src/drawlist/__tests__/builder.cursor.test.ts`:
- Around line 1-10: Update the top JSDoc to match the actual assertion: change
the header note from "Correct v2 header version" to "Correct v1 header version"
(or otherwise reflect V1) so it aligns with the test that checks
ZR_DRAWLIST_VERSION_V1 in builder.cursor.test.ts; locate the JSDoc block at the
top of the file and edit the sentence to reference v1 header version.
In `@packages/core/src/widgets/__tests__/graphicsWidgets.test.ts`:
- Around line 471-483: The test "image invalid source falls back without
DRAW_IMAGE" is constructing a raw VNode literal; replace that literal with the
widget factory call ui.image(...) so the test uses the same invariant as other
tests (see ui.image used on line 461). Update the renderBytes invocation to pass
ui.image({ src: "bad-bytes" as unknown as Uint8Array, width: 20, height: 4, alt:
"Broken image" }) instead of the { kind: "image", props: { ... } } object and
keep the createDrawlistBuilder() argument unchanged.
- Around line 114-147: The test uses the low-level pipeline functions
(renderBytes calling commitVNodeTree, layout, renderToDrawlist and
builder.build) instead of the required testing API; replace the custom
renderBytes path with the testing helper createTestRenderer() so the test
renders via the module entrypoint: locate the renderBytes helper and update
tests to call createTestRenderer() (or refactor renderBytes to wrap
createTestRenderer()) so commit/layout/render are driven by createTestRenderer
rather than directly invoking commitVNodeTree, layout, or renderToDrawlist;
ensure the viewport and terminalProfile parameters are passed through the
createTestRenderer invocation to preserve existing behavior.
In `@packages/ink-compat/src/translation/colorMap.ts`:
- Around line 4-22: Named "black" and other explicit-black parse paths currently
produce rgb(0,0,0) which collides with the unset sentinel; update the color
normalization so any explicit-black result is mapped to near-black (0x010101)
before returning. Concretely: adjust NAMED_COLORS (symbol) or apply a post-parse
normalization in parseColorInner (all return points that return rgb(0,0,0) from
named, hex, rgb-literal branches) to convert {r:0,g:0,b:0} to {r:1,g:1,b:1}, and
do the same in decodeAnsi256Color for the case that returns black (ansi256 0) so
callers like styleToSgr will see a non-falsy color and emit SGR codes.
---
Minor comments:
In `@examples/regression-dashboard/src/screens/overview.ts`:
- Around line 60-61: Replace the use of Date.now() with the renderable timestamp
from state so the UI is deterministic: compute uptimeSec as Math.max(1,
Math.floor((state.nowMs - state.startedAtMs) / 1000)) (using state.nowMs instead
of Date.now()), and then compute updateRate from state.tick / uptimeSec; ensure
you reference the existing symbols uptimeSec, updateRate, state.startedAtMs and
state.nowMs so the component uses the state-provided time source.
In `@packages/core/src/__tests__/drawlistDecode.ts`:
- Around line 263-269: The OP_DEF_STRING decoder branch (and the similar branch
around lines 316-322) currently uses subarray and can silently truncate
malformed payloads; update the decoder to validate payload bounds before
slicing: compute id = u32(bytes, cmd.offset + 8), byteLen = u32(bytes,
cmd.offset + 12) and dataStart = cmd.offset + 16, then check that byteLen is
non-negative and that dataEnd = dataStart + byteLen is <= bytes.length (and >=
dataStart) and that cmd.offset + 16 is within the buffer; if any check fails,
throw a descriptive error (rather than calling subarray), otherwise use
cloneBytes(bytes.subarray(dataStart, dataEnd)) and set the string. Ensure the
same explicit bounds checks are applied to the other DEF payload decoder(s)
referenced.
In `@packages/core/src/app/__tests__/rawRender.test.ts`:
- Around line 37-39: The stub builder's buildInto() always succeeds and never
flips the internal built state, so update the test stub created by
makeStubBuilder() so buildInto() mirrors build()'s state transitions: have
buildInto() check the same built flag and return a failure result when already
built, and set built = true on successful first call (or simply delegate to the
existing build() implementation if present). This ensures RawRenderer
double-build behavior is exercised; locate makeStubBuilder, build(), and
buildInto() in the test file and make buildInto follow build's logic (respecting
the built flag and returning the same ok/error shape).
In `@packages/core/src/drawlist/__tests__/builder.alignment.test.ts`:
- Around line 129-133: The test computes a blobIndex via b.addBlob(...) but then
hardcodes 0 when calling b.drawTextRun(2, 1, 0); update the call to use the
computed blobIndex instead of 0 (i.e., call b.drawTextRun(2, 1, blobIndex)) so
the test uses the actual returned value from addBlob and is not fragile; verify
this change in the test that uses addBlob, blobIndex, drawText, and drawTextRun.
In `@packages/core/src/perf/frameAudit.ts`:
- Around line 95-97: The validation branch that sets cmdStreamValid needs 4-byte
alignment checks: ensure cmdOffset and cmdBytes are non-negative, cmdOffset +
cmdBytes <= bytes.byteLength, and both cmdOffset and cmdBytes are 4-byte aligned
(e.g. cmdOffset % 4 === 0 && cmdBytes % 4 === 0); if any fail return the same
frozen invalid result. Likewise, in the other blocks that validate draw commands
(the checks using off and size around lines handling individual commands), add
alignment checks for off and size (off % 4 === 0 && size % 4 === 0) in addition
to their existing bounds checks and return the frozen invalid result when
misaligned so cmdStreamValid remains consistent with the drawlist format
invariants.
In `@packages/core/src/perf/perf.ts`:
- Around line 247-249: The current dynamic materialization uses a plain object
for counters which can collide with inherited properties; change the local
counters map in the method that reads this.counters to use a null-prototype
object (create with Object.create(null) and cast to Record<string, number> as
needed) instead of {} so keys are safe, then populate it from this.counters and
return as before with Object.freeze(phases) and Object.freeze(counters); update
the declaration of the local counters variable referenced in this function to
use the null-prototype creation.
In `@packages/core/src/renderer/__tests__/renderer.text.test.ts`:
- Around line 165-167: The code reads u32(bytes, off + 8) for OP_FREE_BLOB
without ensuring the command payload is large enough; guard this read by
checking the command size/available bytes before calling u32. Before calling
u32(bytes, off + 8) inside the OP_FREE_BLOB branch, verify that cmd.size (or the
available bytes length from off) is at least 12 (off + 8 + 4) and only then
compute blobId and touch textRunBlobsByIndex; otherwise skip the operation or
treat it as a malformed frame. This uses the existing symbols OP_FREE_BLOB, cmd,
bytes, off, u32, and textRunBlobsByIndex to locate and fix the check.
In `@packages/core/src/runtime/commit.ts`:
- Around line 1788-1800: Remove the temporary debug tracing block in commitNode
that inspects globalThis for __commitDebug and pushes strings into
__commitDebugLog; specifically delete the commitDebug declaration and the if
(commitDebug.__commitDebug) { ... } branch that references __commitDebug,
__commitDebugLog, instanceId, vnode, and prev so the commitNode hot path no
longer performs globalThis lookups or log pushes.
- Around line 1334-1386: The code mutates RuntimeInstance.vnode and children
references in-place (see rewriteCommittedVNode, canFastReuseContainerSelf, and
assignments to prev.vnode and prev.children) as a deliberate fast-reuse
optimization; keep that behavior but ensure test coverage and run the full test
suite. Preserve the in-place mutation logic: when fast-reusing, assign
prev.vnode = rewriteCommittedVNode(...) and prev.children = nextChildren and set
prev.selfDirty/prev.dirty appropriately (as done in the fast-reuse and in-place
mutation branches referencing prev, vnode, prevChildren, nextChildren,
committedChildVNodes); do not make vnode or children readonly or replace with
new RuntimeInstance objects, and after any edits to these branches run node
scripts/run-tests.mjs to verify commit/reconciliation/regression tests (e.g.,
commit.fastReuse.regression.test.ts) pass. Ensure commit diagnostic
(__commitDiag) updates remain correct when toggling these paths.
In `@packages/core/src/widgets/__tests__/basicWidgets.render.test.ts`:
- Around line 70-76: The test currently masks a desync by defaulting to an empty
string when decoded[drawTextIndex] is missing; instead, update the DRAW_TEXT
handling in the test so it fails fast: check that decoded[drawTextIndex] is
defined (use the existing variables decoded and drawTextIndex) and throw a clear
error or assert with context (e.g., include off and drawTextIndex or the
expected header/size) if it's undefined, rather than falling back to "". This
ensures parser/header desyncs are surfaced immediately in basicWidgets.render
tests.
In `@packages/core/src/widgets/__tests__/style.merge-fuzz.test.ts`:
- Around line 25-29: The test removed a value-level assertion for the packed
attrs field after making MutableResolvedTextStyle.attrs optional, which allows
regressions to slip by; restore an independent expected-attrs calculation in the
fuzz test and assert equality against the actual attrs value (not just its
type). Locate the assertions that check resolved text style boolean fields and
the MutableResolvedTextStyle shape (references: MutableResolvedTextStyle and the
test block that computes expected resolved style) and add code to compute
expectedAttrs from the boolean flags the same way production does, then assert
expectedAttrs === actual.attrs (or treat undefined as 0 if the code packs that
way) in addition to the existing boolean and type checks. Ensure this explicit
packed-attrs verification is added in the same test cases that validate boolean
fields so regressions in drawlist/packing are caught.
In `@packages/create-rezi/templates/starship/src/main.ts`:
- Around line 629-649: The polling loop started by debugTraceTimer can overlap
calls to pump because setInterval fires regardless of pump completion; fix by
adding an in-flight guard or switching to a self-scheduling async loop so only
one pump runs at a time: introduce a boolean flag (e.g., debugPumpInFlight) or a
mutex checked/set at the start of pump and cleared on exit (including in
catch/finally), or replace the setInterval with an async function that awaits
pump then uses setTimeout to schedule the next run; ensure you reference and
update the same shared state variables used in pump (debugController,
debugLastRecordId, snapshotDebugRecord) and still clear the timer/stop the loop
when debugging is cancelled.
---
Nitpick comments:
In `@examples/regression-dashboard/src/helpers/formatters.ts`:
- Around line 38-45: The fleetCounts function does three separate array filters
causing redundant scans; refactor fleetCounts to iterate services once (e.g., a
single for loop or reduce) and increment counters for healthy/warning/down based
on service.status, then return Object.freeze({ healthy, warning, down });
reference the fleetCounts function and the Service.status checks to locate where
to replace the three filters with a single-pass accumulation.
In `@examples/regression-dashboard/src/helpers/state.ts`:
- Around line 123-137: createInitialState currently assigns SEED_SERVICES
directly to the services field, risking shared mutable references between state
instances; change createInitialState to initialize services with a deep copy of
SEED_SERVICES (e.g., map over SEED_SERVICES and clone each service object or use
structuredClone) so each DashboardState gets its own independent service
objects; update references in createInitialState, and ensure DashboardState
consumers expect the cloned shape rather than the original shared objects.
In `@examples/regression-dashboard/src/theme.ts`:
- Around line 18-22: THEME_BY_NAME is a constant configuration object that
should be protected from accidental mutation; freeze the registry after creation
by applying Object.freeze to THEME_BY_NAME (and optionally Object.freeze each
value inside THEME_BY_NAME to make the ThemeSpec entries immutable) so consumers
cannot modify the map or its entries; update where THEME_BY_NAME is
defined/initialized to call Object.freeze(THEME_BY_NAME) (and Object.freeze on
each THEME_BY_NAME[key]) to enforce immutability.
In `@packages/core/src/app/createApp.ts`:
- Around line 596-603: The conditional checking backendDrawlistVersion is
redundant because readBackendDrawlistVersionMarker() already throws for non-1
values and returns 1 | null; remove the entire if block that inspects
backendDrawlistVersion and calls invalidProps (the code referencing
backendDrawlistVersion and invalidProps) so only the call to
readBackendDrawlistVersionMarker(backend) remains (or remove the assignment if
unused), leaving no duplicate validation logic.
In `@packages/core/src/app/widgetRenderer.ts`:
- Around line 587-594: The hardcoded debug strings in the needles array used by
the audit helper (the const needles = [...] block) and gated under
FRAME_AUDIT_TREE_ENABLED should be made configurable or explicitly documented:
extract the array into a single exported constant or configuration provider
(e.g., getAuditNeedles or AUDIT_NEEDLES) and load it from a config/env or from a
top-level constant with a descriptive name and JSDoc explaining their purpose,
or add a clear inline comment above FRAME_AUDIT_TREE_ENABLED usage describing
that these values are debug/test-only and how to change them; update references
in widgetRenderer.ts (where needles and FRAME_AUDIT_TREE_ENABLED are used) to
consume the new config/export.
- Around line 761-762: The BFS loop currently uses array.shift() which is O(n)
per pop; change the loop to an index-based queue by introducing a head/index
variable (e.g., let head = 0) and iterate while head < queue.length, reading the
current item via queue[head++] instead of queue.shift(); update any places that
assumed shift's return value and ensure children are still pushed onto queue as
before (or alternatively swap to a dedicated queue class), referencing the
existing queue variable and the while (queue.length > 0) { const current =
queue.shift(); } block in widgetRenderer.ts.
- Around line 2860-2936: Wrap the entire fallback sequence (the block guarded by
doCommit && shapeMismatch !== null that resets this._layoutTreeCache and runs
layout/fallbackLayoutRes/directLayoutRes) with lightweight perf markers via
emitFrameAudit when FRAME_AUDIT_ENABLED is true: emit a "layout.fallback.start"
event immediately before resetting this._layoutTreeCache (include context fields
like path/depth/runtimeKind/layoutKind) and emit a "layout.fallback.end" event
after the final shapeMismatch check (or on each early return) that includes
attemptsPerformed (1/2/3), duration (timestamp difference), and which final
layoutRes used (initial/fallback/direct). Use the existing symbols
shapeMismatch, layout, fallbackLayoutRes, directLayoutRes, emitFrameAudit, and
FRAME_AUDIT_ENABLED to locate and instrument the code so you can monitor
frequency and duration of the fallback paths without changing the core logic.
In `@packages/core/src/drawlist/__tests__/builder_v6_resources.test.ts`:
- Line 1: Rename the test file to remove the "v6" protocol reference (e.g.,
rename builder_v6_resources.test.ts → builder.resources.test.ts) and update any
test runner / import references to this file so the test suite still discovers
it; ensure the top-level test declarations (describe/test) in this file remain
unchanged and that any CI or test patterns that filter by filename are adjusted
to accept the new name.
In `@packages/core/src/drawlist/__tests__/writers.gen.v6.test.ts`:
- Around line 74-88: Add a complementary test where the declared byteLen exceeds
the source payload length to assert that the writer zero-fills the tail: create
a bytes buffer (Uint8Array) initialized with 0xcc, a DataView, a short payload
(e.g., 3 bytes), call writeDefString with a larger declared length (e.g., 9) and
assert the returned end index, the u32 size fields (u32(bytes, 4), u32(bytes,
12)), that the payload bytes are written and that the remaining declared payload
bytes are zero-filled (check subarray region for zeros), and that the byte after
the declared region remains 0xcc; repeat the same complementary “declared length
> source length” test for each corresponding writer variant referenced in this
test file (e.g., other DEF_* writers) to cover the regression.
In `@packages/core/src/drawlist/writers.gen.ts`:
- Around line 260-261: The generated writer should validate the string length in
the generator template (scripts/drawlist-spec.ts) before emitting code that sets
payloadBytes and size; update the template so that instead of directly doing
"const payloadBytes = byteLen >>> 0;" it first checks that byteLen is a finite
number, non-negative, and within the uint32 range (e.g. Number.isFinite(byteLen)
&& byteLen >= 0 && byteLen <= 0xFFFFFFFF) and throw a RangeError for invalid
values, then compute payloadBytes = byteLen >>> 0 and compute size =
align4(DEF_STRING_BASE_SIZE + payloadBytes); ensure the emitted symbols in
writers.gen.ts remain payloadBytes, byteLen, align4, DEF_STRING_BASE_SIZE and
size so callers get a clear, defensive validation at generation time.
In `@packages/core/src/layout/engine/layoutEngine.ts`:
- Around line 59-76: The __layoutProfile object is exported from core runtime
but is temporary and should be confined to perf/test-only surfaces to avoid API
leakage; make __layoutProfile non-exported (or move it into a dedicated dev/perf
module) and provide a dev-only accessor (e.g. getLayoutProfile()) that only
exports or returns the profile when a build/dev flag or NODE_ENV !==
'production' (or an explicit ENABLE_LAYOUT_PROFILE flag) is set, update any
references to use the accessor, and remove the public export so production
builds have no runtime coupling to __layoutProfile.
In `@packages/core/src/perf/perf.ts`:
- Around line 309-312: The perfSnapshot function currently allocates and freezes
new objects on every disabled call; create a module-level frozen singleton
(e.g., const DISABLED_PERF_SNAPSHOT = Object.freeze({ phases: Object.freeze({}),
counters: Object.freeze({}) })) and return that singleton when PERF_ENABLED is
false inside perfSnapshot; update references in perfSnapshot to return
DISABLED_PERF_SNAPSHOT to avoid repeated allocations and preserve immutability.
In `@packages/core/src/pipeline.ts`:
- Around line 34-35: Remove the temporary profiling export from the stable
barrel by deleting the exported symbol __layoutProfile from pipeline.ts and
instead re-export it from an explicitly-internal/unstable entrypoint (e.g., an
internal profiling module); update any internal callers to import
__layoutProfile from that new internal entrypoint (not from pipeline.ts) so the
symbol is not leaked as part of the public API. Ensure the new internal module
clearly names the export as unstable/internal and do not add it back into the
stable barrel export list.
In `@packages/core/src/renderer/__tests__/renderer.partial.test.ts`:
- Around line 64-73: The test's DrawlistBuilder stub methods (setCursor,
hideCursor, setLink, drawCanvas, drawImage) are no-ops so the test can't detect
mismatches; update each stub to record an operation into the builder's recording
structure (same ops array used elsewhere in this test harness) by pushing an
object or tuple that includes the operation name (e.g., "setCursor",
"hideCursor", "setLink", "drawCanvas", "drawImage") and the received arguments
(use ..._args) so the partial-render test can compare emitted ops against the
full plan and surface blind spots in cursor/link/canvas/image emission.
In `@packages/core/src/renderer/__tests__/textStyle.opacity.test.ts`:
- Line 2: The tests currently call the production helper rgbBlend inside
assertions, coupling the oracle to implementation; update the tests in
textStyle.opacity.test.ts to use explicit expected RGB24 values (or a simple
test-local blend helper) instead of rgbBlend when computing expected results for
assertions that reference rgbBlend (also update the occurrences around the other
noted lines); modify the assertions to compare against hard-coded RGB triplets
(or the tiny local function) and remove imports/uses of rgbBlend from the test
so the expected-value logic is independent from the production rgbBlend
implementation.
In `@packages/core/src/renderer/renderToDrawlist/renderTree.ts`:
- Around line 131-135: subtreeCanOverflowBounds is being recomputed many times
during a single damage traversal; add a per-frame memoization layer (e.g., a Map
or WeakMap keyed by the node) and replace direct calls to
subtreeCanOverflowBounds(...) inside renderTree traversal with a cached wrapper
(name it something like subtreeCanOverflowBoundsCached(node)). Initialize/clear
the cache at the start of the frame/damage traversal (where frame state is
prepared) so results are only reused within one traversal, and ensure the
wrapper delegates to the original subtreeCanOverflowBounds when the entry is
missing and stores the result before returning.
In `@packages/core/src/renderer/renderToDrawlist/widgets/renderCanvasWidgets.ts`:
- Line 353: The fallback "?? 0" after hashImageBytes is redundant because
hashImageBytes returns a number; update the imageId assignment in
renderCanvasWidgets.ts (the const imageId line using readNonNegativeInt and
hashImageBytes) to remove the unreachable "?? 0" fallback so the expression
becomes readNonNegativeInt(props.imageId) ?? hashImageBytes(analyzed.bytes).
- Around line 225-227: The helper addBlobAligned now just forwards to
builder.addBlob(bytes) but its name implies alignment semantics that no longer
exist; either rename the function to something explicit like addBlob (or remove
it entirely) and replace all callers to call builder.addBlob(bytes) directly, or
inline the call sites and delete addBlobAligned; update all references to
addBlobAligned (including other occurrences) and adjust imports/exports
accordingly so there are no dangling references to addBlobAligned.
In `@packages/core/src/testing/renderer.ts`:
- Around line 176-188: RecordingDrawlistBuilder currently leaves
cursor/link/canvas/image methods as no-ops so tests don't record those ops;
implement these methods (setCursor, hideCursor, setLink, drawCanvas, drawImage)
to append appropriate op records to the builder's internal operations list using
the same shape used by existing op methods, and update buildInto to serialize
into the provided _dst (or call build and copy the resulting bytes into _dst) so
tests can observe the recorded ops; locate RecordingDrawlistBuilder, its
internal ops array and the build() method to mirror their encoding/recording
pattern for these new methods.
In `@packages/core/src/theme/__tests__/theme.extend.test.ts`:
- Around line 141-145: The test currently asserts
Object.isFrozen(extended.colors.accent.primary) but that is tautological because
packed Rgb24 colors are JS primitives and Object.isFrozen always returns true
for primitives; update the test to remove that primitive-freeze assertion and
instead verify the freeze behavior of the containing object and the color value
itself by asserting Object.isFrozen(extended.colors.accent) is true, asserting
typeof extended.colors.accent.primary === "number" (or comparing to the expected
numeric packed color), and verifying equality to the expected color value so the
test proves both immutability of the container and correctness of the packed
color rather than relying on Object.isFrozen on a primitive.
In `@packages/core/src/theme/resolve.ts`:
- Around line 191-223: The function isValidColorPath recreates the validPaths
Set on every call; extract the array/set into a module-level constant (e.g.,
const VALID_COLOR_PATHS: ReadonlySet<string> or build it from a const array) and
have isValidColorPath(path: string): path is ColorPath simply return
VALID_COLOR_PATHS.has(path); keep the same type (ReadonlySet<string>) and the
same element strings so behavior does not change.
In `@packages/core/src/ui/recipes.ts`:
- Around line 40-42: The adjustBrightness function currently uses 0 as the black
target which collides with the Rgb24 sentinel; change the darkening target from
0 to a near-black floor (e.g., 0x010101 or (1<<16)|(1<<8)|1) so extreme
darkening blends toward an explicit near-black instead of the sentinel; update
the target calculation in adjustBrightness and keep using blendRgb(color,
target, Math.abs(amount)) so other behavior remains unchanged.
In `@packages/core/src/widgets/__tests__/style.merge-fuzz.test.ts`:
- Around line 58-64: Don't call mergeTextStyle from the fuzz helper; instead
construct the fuzz base style locally so the test oracle is independent from the
SUT. Create a plain object by copying the relevant DEFAULT_BASE_STYLE fields
(fg, bg and any explicit keys you need), set fg/bg via randomRgb(rng) and then
assign BOOL_ATTRS entries explicitly based on rng.u32() % 3 (leave
undefined/omit when state === 0, set false/true for 1/2). Do the same change for
the other helper at lines 68-74 so both fuzz bases are built locally and not via
mergeTextStyle.
In `@packages/core/src/widgets/commandPalette.ts`:
- Around line 284-290: Replace the hardcoded PALETTE_COLORS constant with
theme-driven tokens: add command-palette color tokens to your theme interface
(names matching the existing keys: background, border, selectedBg, highlight,
placeholder) and register default values equal to the current rgb(...) values;
then update code that references PALETTE_COLORS to read from the theme (e.g.,
theme.getToken or theme.commandPalette.*) so the palette colors come from the
theme system rather than the PALETTE_COLORS constant (keep the rgb(...) defaults
when registering the tokens).
In `@packages/core/src/widgets/field.ts`:
- Line 11: Replace the hardcoded RGB usage introduced via the rgb import in
packages/core/src/widgets/field.ts with a semantic theme token: find where
rgb(255, 0, 0) (or similar hardcoded rgb(...) calls) is used for the error
foreground in this file (e.g., within Field, FieldLabel, or whatever component
uses the error foreground) and swap it for a theme token accessor like
theme.colors.error or getToken("color.error") (use your app's token API and
prefer "error" or "danger" as the token name); update imports to pull the
theme/token helper instead of rgb and ensure the component consumes the token
value rather than a literal RGB.
In `@packages/core/src/widgets/toast.ts`:
- Around line 173-181: TOAST_COLORS is computed once from defaultTheme.colors so
it won't reflect runtime theme changes; change the static TOAST_COLORS const
into a function (e.g., getToastColors(theme) or toastColorsFor(theme)) that
reads colors from the provided theme (use the same keys:
"widget.toast.info"/info, "widget.toast.success"/success, etc.), and update all
call sites that reference TOAST_COLORS to call this function with the current
theme (similar to logLevelToThemeColor in editors.ts) so toast border colors
update when the active theme changes.
In `@packages/ink-compat/src/__tests__/translation/propsToVNode.test.ts`:
- Around line 304-310: Add a negative assertion to the test to ensure
marker-based flex-basis is not present: in the same block that checks
vnode.props.width/height/flexBasis and the
__inkPercentMinWidth/__inkPercentMinHeight markers, assert that
vnode.props.__inkPercentFlexBasis is undefined (or otherwise not present) so the
test fails if a marker-based flex-basis regresses back in; update the assertion
near the existing checks for vnode.props.flexBasis and
__inkPercentMinWidth/__inkPercentMinHeight.
In `@packages/ink-compat/src/translation/propsToVNode.ts`:
- Around line 495-502: The NATIVE_PERCENT_PROPS Set is currently created inside
the box translation path causing per-node allocations; hoist it to module/file
scope by moving the declaration of NATIVE_PERCENT_PROPS (the Set of
"width","height","flexBasis") out of the function where propsToVNode/box
translation runs and declare it once at top-level so all invocations reuse the
same Set instance.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/renderer/renderToDrawlist/overflowCulling.ts (1)
21-39: Consider checkingblurandspreadfor complete overflow detection.The shadow configuration may include
blurandspreadproperties that can cause overflow even when offsets are zero. Currently onlyoffsetXandoffsetYare checked.If this is an intentional simplification for the culling heuristic (conservative approximation is acceptable), this is fine. Otherwise, you may want to extend the check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/renderToDrawlist/overflowCulling.ts` around lines 21 - 39, hasBoxShadowOverflow currently only checks offsetX/offsetY via readShadowOffset; extend it to also consider shadow.blur and shadow.spread since nonzero blur/spread can create overflow. In the hasBoxShadowOverflow function, after reading offsetX/offsetY from config, read blur and spread from the same config using the existing shadow-reading helpers (e.g., readShadowBlur and readShadowSpread or analogous utilities used elsewhere) with the same default semantics, then return true if offsetX>0 || offsetY>0 || blur>0 || spread>0; keep the existing early returns for non-box nodes and non-object shadow configs and reuse the same config variable names (config, offsetX, offsetY) to locate the change.packages/create-rezi/templates/starship/src/main.ts (1)
646-667: Add in-flight guard to prevent overlapping debug queries.The async
pump()function at line 646 is called fromsetInterval()every 250ms without waiting for completion. IfdebugController.query()takes longer than the interval, overlapping requests will accumulate, creating unnecessary query/log pressure in debug mode.Add a simple
debugTraceInFlightflag to serialize the pump calls:let debugTraceTimer: ReturnType<typeof setInterval> | null = null; +let debugTraceInFlight = false; ... const pump = async () => { - if (!debugController) return; + if (!debugController || debugTraceInFlight) return; + debugTraceInFlight = true; try { const records = await debugController.query({ maxRecords: 256 }); if (records.length === 0) return; for (const record of records) { if (record.header.recordId <= debugLastRecordId) continue; debugLastRecordId = record.header.recordId; snapshotDebugRecord(record); } } catch (error) { debugSnapshot("runtime.debug.query.error", { message: error instanceof Error ? error.message : String(error), route: currentRouteId(), }); + } finally { + debugTraceInFlight = false; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-rezi/templates/starship/src/main.ts` around lines 646 - 667, Add a boolean guard (e.g., debugTraceInFlight) around the async pump to prevent overlapping runs: declare debugTraceInFlight in the same scope as pump/debugTraceTimer, at the top of pump return immediately if debugTraceInFlight is true, set debugTraceInFlight = true when starting the work (before calling debugController.query) and ensure debugTraceInFlight is reset to false in a finally block so it clears on both success and error; keep existing logic using debugController.query, debugLastRecordId, snapshotDebugRecord/debugSnapshot and currentRouteId unchanged.
🤖 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/core/src/renderer/renderToDrawlist/overflowCulling.ts`:
- Around line 13-19: readShadowOffset currently clamps negative shadow offsets
to 0 causing hasBoxShadowOverflow to miss overflows; update readShadowOffset
(the function named readShadowOffset) to return the absolute magnitude of the
truncated numeric input instead of zero for negative values (i.e., use Math.abs
on the truncated value and then ensure non-negative result or fallback), keeping
the fallback behavior when input is not a finite number so negative offsets like
-5 correctly contribute to overflow detection.
---
Nitpick comments:
In `@packages/core/src/renderer/renderToDrawlist/overflowCulling.ts`:
- Around line 21-39: hasBoxShadowOverflow currently only checks offsetX/offsetY
via readShadowOffset; extend it to also consider shadow.blur and shadow.spread
since nonzero blur/spread can create overflow. In the hasBoxShadowOverflow
function, after reading offsetX/offsetY from config, read blur and spread from
the same config using the existing shadow-reading helpers (e.g., readShadowBlur
and readShadowSpread or analogous utilities used elsewhere) with the same
default semantics, then return true if offsetX>0 || offsetY>0 || blur>0 ||
spread>0; keep the existing early returns for non-box nodes and non-object
shadow configs and reuse the same config variable names (config, offsetX,
offsetY) to locate the change.
In `@packages/create-rezi/templates/starship/src/main.ts`:
- Around line 646-667: Add a boolean guard (e.g., debugTraceInFlight) around the
async pump to prevent overlapping runs: declare debugTraceInFlight in the same
scope as pump/debugTraceTimer, at the top of pump return immediately if
debugTraceInFlight is true, set debugTraceInFlight = true when starting the work
(before calling debugController.query) and ensure debugTraceInFlight is reset to
false in a finally block so it clears on both success and error; keep existing
logic using debugController.query, debugLastRecordId,
snapshotDebugRecord/debugSnapshot and currentRouteId unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
examples/regression-dashboard/src/main.tspackages/core/src/renderer/renderToDrawlist/overflowCulling.tspackages/create-rezi/templates/starship/src/main.tspackages/native/vendor/VENDOR_COMMIT.txtpackages/native/vendor/zireael/include/zr/zr_drawlist.hpackages/native/vendor/zireael/include/zr/zr_version.hpackages/native/vendor/zireael/src/core/zr_config.cpackages/native/vendor/zireael/src/core/zr_diff.cpackages/native/vendor/zireael/src/core/zr_drawlist.cpackages/native/vendor/zireael/src/core/zr_engine_present.incvendor/zireael
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/regression-dashboard/src/main.ts
…ized-renderer # Conflicts: # packages/core/src/__tests__/integration/integration.dashboard.test.ts # packages/core/src/__tests__/integration/integration.file-manager.test.ts # packages/core/src/__tests__/integration/integration.form-editor.test.ts # packages/core/src/__tests__/integration/integration.reflow.test.ts # packages/core/src/__tests__/integration/integration.resize.test.ts # packages/core/src/__tests__/stress/stress.large-trees.test.ts # packages/core/src/abi.ts # packages/core/src/app/__tests__/partialDrawlistEmission.test.ts # packages/core/src/app/__tests__/rawRender.test.ts # packages/core/src/app/__tests__/widgetRenderer.integration.test.ts # packages/core/src/app/createApp.ts # packages/core/src/app/rawRenderer.ts # packages/core/src/app/widgetRenderer.ts # packages/core/src/drawlist/__tests__/builder.alignment.test.ts # packages/core/src/drawlist/__tests__/builder.golden.test.ts # packages/core/src/drawlist/__tests__/builder.graphics.test.ts # packages/core/src/drawlist/__tests__/builder.limits.test.ts # packages/core/src/drawlist/__tests__/builder.reset.test.ts # packages/core/src/drawlist/__tests__/builder.round-trip.test.ts # packages/core/src/drawlist/__tests__/builder.string-cache.test.ts # packages/core/src/drawlist/__tests__/builder.string-intern.test.ts # packages/core/src/drawlist/__tests__/builder.style-encoding.test.ts # packages/core/src/drawlist/__tests__/builder.text-run.test.ts # packages/core/src/drawlist/__tests__/builder.validate-caps.test.ts # packages/core/src/drawlist/__tests__/builder_style_attrs.test.ts # packages/core/src/drawlist/__tests__/builder_v6_resources.test.ts # packages/core/src/drawlist/builder.ts # packages/core/src/drawlist/builderBase.ts # packages/core/src/drawlist/index.ts # packages/core/src/drawlist/types.ts # packages/core/src/index.ts # packages/core/src/renderer/__tests__/focusIndicators.test.ts # packages/core/src/renderer/__tests__/overlay.edge.test.ts # packages/core/src/renderer/__tests__/persistentBlobKeys.test.ts # packages/core/src/renderer/__tests__/recipeRendering.test-utils.ts # packages/core/src/renderer/__tests__/render.golden.test.ts # packages/core/src/renderer/__tests__/renderer.border.test.ts # packages/core/src/renderer/__tests__/renderer.clip.test.ts # packages/core/src/renderer/__tests__/renderer.damage.test.ts # packages/core/src/renderer/__tests__/renderer.partial.perf.test.ts # packages/core/src/renderer/__tests__/renderer.scrollbar.test.ts # packages/core/src/renderer/__tests__/renderer.text.test.ts # packages/core/src/renderer/renderToDrawlist/renderTree.ts # packages/core/src/renderer/renderToDrawlist/widgets/renderCanvasWidgets.ts # packages/core/src/testing/renderer.ts # packages/core/src/theme/__tests__/theme.extend.test.ts # packages/core/src/theme/__tests__/theme.switch.test.ts # packages/core/src/ui/__tests__/themed.test.ts # packages/core/src/widgets/__tests__/basicWidgets.render.test.ts # packages/core/src/widgets/__tests__/graphics.golden.test.ts # packages/core/src/widgets/__tests__/graphicsWidgets.test.ts # packages/core/src/widgets/__tests__/renderer.regressions.test.ts # packages/core/src/widgets/__tests__/style.utils.test.ts # packages/core/src/widgets/__tests__/styleUtils.test.ts # packages/core/src/widgets/__tests__/widgetRenderSmoke.test.ts # packages/ink-compat/src/runtime/createInkRenderer.ts # packages/native/vendor/VENDOR_COMMIT.txt # packages/native/vendor/zireael/include/zr/zr_drawlist.h # packages/native/vendor/zireael/src/core/zr_drawlist.c # packages/native/vendor/zireael/src/core/zr_engine.c # packages/native/vendor/zireael/src/core/zr_engine_present.inc # packages/node/src/backend/nodeBackend.ts # packages/testkit/fixtures/zrdl-v1/golden/draw_text_interned.bin
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/drawlist/builderBase.ts (1)
405-427:⚠️ Potential issue | 🟠 MajorSynchronize protocol specification with builder interface refactoring.
The
appendDrawTextCommandsignature was refactored to remove thebyteOffparameter, but the drawlist protocol specification and generated writers code were not updated to match. Currently:
scripts/drawlist-spec.tsstill definesDRAW_TEXTwith thebyteOfffield at offset 20writers.gen.tsstill includesbyteOffin the generated signaturebuilder.tsworks around this by hardcodingbyteOffto0when callingwriteDrawTextdocs/protocol/zrdl.mdstill documents the old signature:DRAW_TEXT (stringId, byteOff, byteLen)To complete the refactoring:
- Update
scripts/drawlist-spec.tsto remove thebyteOfffield from DRAW_TEXT- Regenerate with
npm run codegenand validate withnpm run codegen:check- Update
docs/protocol/zrdl.mdto reflect the new signature- Update
docs/protocol/versioning.mdif this is a breaking change- Update byte-level tests in
packages/core/src/drawlist/__tests__/writers.gen.test.tsThis is a binary protocol change requiring full synchronization per project guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/builderBase.ts` around lines 405 - 427, The protocol and generated writers were not updated after refactoring appendDrawTextCommand to remove byteOff: update the DRAW_TEXT entry in scripts/drawlist-spec.ts to remove the byteOff field (so offsets no longer reserve offset 20), run npm run codegen and npm run codegen:check to regenerate writers (writers.gen.ts) so writeDrawText signatures no longer include byteOff, remove the temporary hardcoded 0 in builder.ts where writeDrawText is called, then update docs/protocol/zrdl.md (and docs/protocol/versioning.md if this is a breaking change) to reflect the new DRAW_TEXT signature and finally adjust byte-level tests in packages/core/src/drawlist/__tests__/writers.gen.test.ts to match the new binary layout.
🧹 Nitpick comments (3)
packages/core/src/drawlist/__tests__/writers.gen.test.ts (1)
530-541: Consider adding explicit tests for new def block sizes and writers.The new
writeDefStringandwriteDefBlobfunctions are only tested implicitly via the round-trip integration test. For consistency with other writers and better fault isolation:
- Add size constant tests for
DEF_STRING_BASE_SIZEandDEF_BLOB_BASE_SIZEin this section- Consider adding byte identity tests for the def writers (though their variable-length nature may require a different test pattern)
📝 Suggested additions to the size constants section
describe("writers.gen - size constants", () => { test("CLEAR_SIZE", () => assert.equal(CLEAR_SIZE, 8)); test("FILL_RECT_SIZE", () => assert.equal(FILL_RECT_SIZE, 52)); test("DRAW_TEXT_SIZE", () => assert.equal(DRAW_TEXT_SIZE, 60)); test("PUSH_CLIP_SIZE", () => assert.equal(PUSH_CLIP_SIZE, 24)); test("BLIT_RECT_SIZE", () => assert.equal(BLIT_RECT_SIZE, 32)); test("POP_CLIP_SIZE", () => assert.equal(POP_CLIP_SIZE, 8)); test("DRAW_TEXT_RUN_SIZE", () => assert.equal(DRAW_TEXT_RUN_SIZE, 24)); test("SET_CURSOR_SIZE", () => assert.equal(SET_CURSOR_SIZE, 20)); test("DRAW_CANVAS_SIZE", () => assert.equal(DRAW_CANVAS_SIZE, 32)); test("DRAW_IMAGE_SIZE", () => assert.equal(DRAW_IMAGE_SIZE, 40)); + test("DEF_STRING_BASE_SIZE", () => assert.equal(DEF_STRING_BASE_SIZE, 16)); + test("DEF_BLOB_BASE_SIZE", () => assert.equal(DEF_BLOB_BASE_SIZE, 16)); });Based on learnings: "When changing drawlist command layout/opcodes/field offsets: update byte-level tests in
packages/core/src/drawlist/__tests__/writers.gen.test.ts"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/__tests__/writers.gen.test.ts` around lines 530 - 541, Add explicit tests for the new def block sizes and writers: in the existing "writers.gen - size constants" test group add assertions that DEF_STRING_BASE_SIZE and DEF_BLOB_BASE_SIZE equal their expected base values (mirroring the pattern used for CLEAR_SIZE, DRAW_TEXT_SIZE, etc.), and add focused byte-identity tests for writeDefString and writeDefBlob that write a short known string/blob and assert the produced buffer length and header bytes match the expected base size + payload length (or compare the header slice to an expected header); reference the writeDefString, writeDefBlob, DEF_STRING_BASE_SIZE, and DEF_BLOB_BASE_SIZE symbols when locating code to update.packages/core/src/drawlist/builderBase.ts (1)
690-741: Error messages ininternStringhave misleading context when called fromaddTextRunBlob.The
internStringmethod is called from bothdrawText(line 205) andaddTextRunBlob(line 320), but error messages (lines 695, 703, 724) are hardcoded with "drawText:" prefix. When string interning fails duringaddTextRunBlob, the error will misleadingly reference "drawText".Consider using a generic prefix like "internString:" or passing the caller context as a parameter.
♻️ Proposed fix
- protected internString(text: string): number | null { + protected internString(text: string, callerHint = "internString"): number | null { const existing = this.stringIndexByValue.get(text); if (existing !== undefined) return existing; if (!this.encoder) { - this.fail("ZRDL_INTERNAL", "drawText: TextEncoder is not available"); + this.fail("ZRDL_INTERNAL", `${callerHint}: TextEncoder is not available`); return null; } const nextIndex = this.stringSpanOffs.length; if (nextIndex + 1 > this.maxStrings) { this.fail( "ZRDL_TOO_LARGE", - `drawText: maxStrings exceeded (count=${nextIndex + 1}, max=${this.maxStrings})`, + `${callerHint}: maxStrings exceeded (count=${nextIndex + 1}, max=${this.maxStrings})`, ); return null; } // ... similar change for maxStringBytes error at line 724🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/builderBase.ts` around lines 690 - 741, The error strings inside internString currently hardcode the "drawText:" prefix which misattributes failures when internString is invoked from other callers like addTextRunBlob; update internString to use a neutral prefix (e.g., "internString:") or accept an optional callerContext parameter and include that in the process of failing so errors reflect the true caller (update all fail(...) calls in internString and adjust callers such as drawText and addTextRunBlob to pass context if you choose the parameter approach). Ensure you update references to internString, drawText, and addTextRunBlob so tests/logs show the new message format consistently.packages/core/src/renderer/__tests__/renderer.damage.test.ts (1)
67-74: Minor inconsistency with other test file'sblitRectstub.This file uses explicit parameters for
blitRect, whilerenderer.partial.test.tsuses...args: Parameters<DrawlistBuilder["blitRect"]>. Both work correctly, but using theParameters<>approach would be more maintainable if the signature changes.♻️ Optional: Use Parameters<> for consistency
- blitRect( - _srcX: number, - _srcY: number, - _w: number, - _h: number, - _dstX: number, - _dstY: number, - ): void {} + blitRect(..._args: Parameters<DrawlistBuilder["blitRect"]>): void {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/renderer.damage.test.ts` around lines 67 - 74, Replace the explicit parameter list in the blitRect stub with a variadic typed parameter using Parameters to match other tests: change the signature currently named blitRect(_srcX: number, _srcY: number, _w: number, _h: number, _dstX: number, _dstY: number) to use ...args: Parameters<DrawlistBuilder["blitRect"]> so the test stays consistent and will automatically track future signature changes of DrawlistBuilder.blitRect.
🤖 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/core/src/drawlist/builder.ts`:
- Around line 286-292: getTextPerfCounters currently returns a hardcoded
textEncoderCalls: 0 which is misleading; update the implementation and API so
the metric is accurate: either add a textEncoderCalls counter property to the
builder class and increment it inside the text encoding paths (where strings are
encoded/added — look for methods performing text encoding or writing into string
arena) and return that counter in getTextPerfCounters alongside
this.stringBytesLen and this.textPerfSegments, or remove textEncoderCalls from
the DrawlistTextPerfCounters interface and from getTextPerfCounters so the
contract matches implemented metrics; adjust any callers/tests expecting the
field accordingly.
- Around line 315-322: Update the protocol docs and consumer code to reflect
that link references are stored 1-based: document in docs/protocol/zrdl.md (and
update docs/protocol/versioning.md if compatibility is impacted) that the style
fields linkUriRef and linkIdRef are 1-based indices with 0 as the sentinel for
"no active link"; reference the builder behavior where internString(...) result
is stored as (idx + 1) >>> 0 in activeLinkUriRef and activeLinkIdRef to make the
scheme explicit. Also update the renderer (Zireael) to read these style fields
as 1-based IDs (subtract 1 before indexing into interned arrays) and add a
compatibility note in versioning.md if this change affects older renderers.
Ensure examples in zrdl.md show both the sentinel 0 and a sample 1-based value.
---
Outside diff comments:
In `@packages/core/src/drawlist/builderBase.ts`:
- Around line 405-427: The protocol and generated writers were not updated after
refactoring appendDrawTextCommand to remove byteOff: update the DRAW_TEXT entry
in scripts/drawlist-spec.ts to remove the byteOff field (so offsets no longer
reserve offset 20), run npm run codegen and npm run codegen:check to regenerate
writers (writers.gen.ts) so writeDrawText signatures no longer include byteOff,
remove the temporary hardcoded 0 in builder.ts where writeDrawText is called,
then update docs/protocol/zrdl.md (and docs/protocol/versioning.md if this is a
breaking change) to reflect the new DRAW_TEXT signature and finally adjust
byte-level tests in packages/core/src/drawlist/__tests__/writers.gen.test.ts to
match the new binary layout.
---
Nitpick comments:
In `@packages/core/src/drawlist/__tests__/writers.gen.test.ts`:
- Around line 530-541: Add explicit tests for the new def block sizes and
writers: in the existing "writers.gen - size constants" test group add
assertions that DEF_STRING_BASE_SIZE and DEF_BLOB_BASE_SIZE equal their expected
base values (mirroring the pattern used for CLEAR_SIZE, DRAW_TEXT_SIZE, etc.),
and add focused byte-identity tests for writeDefString and writeDefBlob that
write a short known string/blob and assert the produced buffer length and header
bytes match the expected base size + payload length (or compare the header slice
to an expected header); reference the writeDefString, writeDefBlob,
DEF_STRING_BASE_SIZE, and DEF_BLOB_BASE_SIZE symbols when locating code to
update.
In `@packages/core/src/drawlist/builderBase.ts`:
- Around line 690-741: The error strings inside internString currently hardcode
the "drawText:" prefix which misattributes failures when internString is invoked
from other callers like addTextRunBlob; update internString to use a neutral
prefix (e.g., "internString:") or accept an optional callerContext parameter and
include that in the process of failing so errors reflect the true caller (update
all fail(...) calls in internString and adjust callers such as drawText and
addTextRunBlob to pass context if you choose the parameter approach). Ensure you
update references to internString, drawText, and addTextRunBlob so tests/logs
show the new message format consistently.
In `@packages/core/src/renderer/__tests__/renderer.damage.test.ts`:
- Around line 67-74: Replace the explicit parameter list in the blitRect stub
with a variadic typed parameter using Parameters to match other tests: change
the signature currently named blitRect(_srcX: number, _srcY: number, _w: number,
_h: number, _dstX: number, _dstY: number) to use ...args:
Parameters<DrawlistBuilder["blitRect"]> so the test stays consistent and will
automatically track future signature changes of DrawlistBuilder.blitRect.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/core/src/drawlist/__tests__/builder.text-arena.equivalence.test.tspackages/core/src/drawlist/__tests__/writers.gen.test.tspackages/core/src/drawlist/__tests__/writers.gen.v6.test.tspackages/core/src/drawlist/builder.tspackages/core/src/drawlist/builderBase.tspackages/core/src/drawlist/writers.gen.tspackages/core/src/renderer/__tests__/persistentBlobKeys.test.tspackages/core/src/renderer/__tests__/renderer.damage.test.tspackages/core/src/renderer/__tests__/renderer.partial.test.tspackages/core/src/renderer/renderToDrawlist/overflowCulling.tspackages/core/src/renderer/renderToDrawlist/widgets/containers.tspackages/core/src/runtime/commit.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/drawlist/tests/writers.gen.v6.test.ts
- packages/core/src/drawlist/writers.gen.ts
- packages/core/src/renderer/renderToDrawlist/overflowCulling.ts
- packages/core/src/renderer/tests/persistentBlobKeys.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ink-compat/src/runtime/createInkRenderer.ts (1)
458-476: Dead code:countSelfDirtyis defined but never used.The comment indicates this is a temporary debug helper. Since the investigation appears complete, consider removing it to avoid leaving dead code in production.
🧹 Proposed cleanup
-// --------------------------------------------------------------------------- -// Debug helpers (temporary — remove after investigation) -// --------------------------------------------------------------------------- - -function countSelfDirty(root: RuntimeInstance | null): number { - if (!root) return 0; - let count = 0; - const stack: RuntimeInstance[] = [root]; - while (stack.length > 0) { - const node = stack.pop(); - if (!node) continue; - if (node.selfDirty) count++; - for (let i = node.children.length - 1; i >= 0; i--) { - const child = node.children[i]; - if (child) stack.push(child); - } - } - return count; -} -🤖 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 458 - 476, Remove the unused debug helper function countSelfDirty from createInkRenderer (the function signature and its internal traversal logic referencing RuntimeInstance and properties like selfDirty and children); delete the entire function block to eliminate dead code and any related temporary comment header ("Debug helpers (temporary — remove after investigation)"), ensuring no other code references countSelfDirty remain (if they do, either remove those calls or replace them with appropriate production logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ink-compat/src/runtime/createInkRenderer.ts`:
- Around line 458-476: Remove the unused debug helper function countSelfDirty
from createInkRenderer (the function signature and its internal traversal logic
referencing RuntimeInstance and properties like selfDirty and children); delete
the entire function block to eliminate dead code and any related temporary
comment header ("Debug helpers (temporary — remove after investigation)"),
ensuring no other code references countSelfDirty remain (if they do, either
remove those calls or replace them with appropriate production logic).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
docs/protocol/versioning.mddocs/protocol/zrdl.mdpackages/core/src/drawlist/__tests__/builder.text-arena.equivalence.test.tspackages/core/src/drawlist/builder.tspackages/core/src/drawlist/builderBase.tspackages/ink-compat/src/runtime/createInkRenderer.ts
✅ Files skipped from review due to trivial changes (1)
- docs/protocol/zrdl.md
This was referenced Feb 27, 2026
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
scripts/check-native-vendor-integrity.mjs+ CI/test coverage)packages/native/vendor/zireaelto Zireael PR head268e21fand updateVENDOR_COMMIT.txtpackages/native/src/lib.rsValidation
npm -w @rezi-ui/core run buildnode scripts/run-tests.mjs --scope all . .(4597/4597 pass)cargo testinpackages/nativenpm -w @rezi-ui/native run test:native:smokeDependency
268e21f; if merge strategy rewrites SHA, I will push the follow-up pin update in this PR.Summary by CodeRabbit
New Features
Improvements
Documentation