EPIC 6: packed style pipeline + Zireael vendor bump#217
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.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughConsolidates drawlist builder variants into a single DrawlistBuilder/createDrawlistBuilder API, migrates color model from object RGB to packed 24-bit Rgb24 with rgb(...) helpers, removes multi-version frame-writer branching in renderers/backends, adds resource-backed drawlist support (strings/blobs) and native resource APIs, and updates many tests and tooling accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant WidgetRenderer
participant DrawlistBuilder
participant Backend
participant NativeEngine
App->>WidgetRenderer: commit → render
WidgetRenderer->>DrawlistBuilder: emit draw commands (drawText, addBlob, drawCanvas, setLink, setCursor)
DrawlistBuilder->>DrawlistBuilder: manage resources (DEF_STRING/DEF_BLOB), build()/buildInto()
DrawlistBuilder->>Backend: requestFrame(built.bytes) + resource defs
Backend->>NativeEngine: preflight resources → execute drawlist
NativeEngine-->>Backend: execution result (ok / error)
Backend-->>App: frame ack / error
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a19b132e14
ℹ️ 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".
packages/core/src/renderer/renderToDrawlist/widgets/renderCanvasWidgets.ts
Show resolved
Hide resolved
…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
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/core/src/widgets/field.ts (1)
11-24:⚠️ Potential issue | 🟠 MajorReplace hardcoded error color with a semantic theme token.
At Line 24,
rgb(255, 0, 0)is still a hardcoded RGB value (just in packed form), which breaks the design-system/theming rule. Please use the existing semantic error token from theme/design tokens forFIELD_ERROR_STYLE.fgand remove the newrgbimport at Line 11 if no longer needed.As per coding guidelines: “Never hardcode RGB color values; let the theme handle colors via tokens. Use semantic color tokens/design system props.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/field.ts` around lines 11 - 24, The FIELD_ERROR_STYLE constant uses a hardcoded rgb(...) value for fg; replace that value with the semantic error color token from the design/theme (e.g., the theme's error/semantic token) by assigning the token to FIELD_ERROR_STYLE.fg, and remove the now-unused rgb import at the top (import { rgb } from "./style.js") if nothing else in this file uses it; update the Object.freeze definition for FIELD_ERROR_STYLE accordingly so the file uses the theme token instead of rgb(255,0,0).packages/core/src/widgets/__tests__/basicWidgets.render.test.ts (1)
126-139:⚠️ Potential issue | 🟠 Major
renderBytesandrenderBytesV3are now identical, but later tests still assert version-specific differences.Line 126 and Line 138 both use
createDrawlistBuilder(), so both helpers now produce the same protocol output. That conflicts with downstream assertions that expect v3-only fields and v1 degradation, and can cause invalid/failing tests.Suggested direction (collapse to single current-protocol expectations)
-function renderBytesV3( - vnode: VNode, - viewport: Readonly<{ cols: number; rows: number }> = { cols: 64, rows: 20 }, - opts: Readonly<{ - focusedId?: string | null; - theme?: Theme; - focusAnnouncement?: string | null; - }> = {}, -): Uint8Array { - return renderBytesWithBuilder(vnode, () => createDrawlistBuilder(), viewport, opts); -} +// Unified protocol path: keep a single render helper.- test("link encodes hyperlink refs on v3 and degrades on v1", () => { - const v3 = renderBytesV3(ui.link("https://example.com", "Docs"), { cols: 40, rows: 4 }); - const v1 = renderBytes(ui.link("https://example.com", "Docs"), { cols: 40, rows: 4 }); - ... - assert.equal((v3Docs?.linkUriRef ?? 0) > 0, true); - assert.equal(v1Docs?.linkUriRef ?? 0, 0); - }); + test("link encodes hyperlink refs", () => { + const bytes = renderBytes(ui.link("https://example.com", "Docs"), { cols: 40, rows: 4 }); + const docs = parseDrawTextCommands(bytes).find((cmd) => cmd.text === "Docs"); + assert.equal((docs?.linkUriRef ?? 0) > 0, true); + });Based on learnings, public-facing protocol references should consistently use v1 after the reset, not legacy version splits.
🤖 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 126 - 139, renderBytes and renderBytesV3 currently call the same builder (createDrawlistBuilder) so both produce identical protocol output; change renderBytes to use the v1 drawlist builder so it emits the legacy/public protocol (e.g., call the v1 factory such as createDrawlistBuilderV1 or the builder with version:1 options) while leaving renderBytesV3 using createDrawlistBuilder(), ensuring tests that assert v1 vs v3 differences can pass.packages/core/src/runtime/commit.ts (1)
108-151:⚠️ Potential issue | 🔴 Critical
textStyleEqualmisses underline fields, which can cause incorrect leaf reuse.When only underline style/color changes, this equality can still return true, and the leaf fast path may skip required updates.
🐛 Proposed fix
function textStyleEqual( a: | { bold?: boolean; dim?: boolean; italic?: boolean; underline?: boolean; + underlineStyle?: unknown; + underlineColor?: unknown; inverse?: boolean; strikethrough?: boolean; overline?: boolean; blink?: boolean; fg?: unknown; bg?: unknown; } | undefined, b: | { bold?: boolean; dim?: boolean; italic?: boolean; underline?: boolean; + underlineStyle?: unknown; + underlineColor?: unknown; inverse?: boolean; strikethrough?: boolean; overline?: boolean; blink?: boolean; fg?: unknown; bg?: unknown; } | undefined, ): boolean { @@ a.underline === b.underline && + a.underlineStyle === b.underlineStyle && + colorEqual(a.underlineColor, b.underlineColor) && a.inverse === b.inverse && a.strikethrough === b.strikethrough && a.overline === b.overline && a.blink === b.blink && colorEqual(a.fg, b.fg) &&🤖 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 108 - 151, The textStyleEqual function currently omits the underline color field from its type and equality checks, causing underline-only style changes to be treated as equal; update the style shape in textStyleEqual to include underlineColor?: unknown (alongside fg/bg) and add a corresponding comparison using colorEqual(a.underlineColor, b.underlineColor) in the returned boolean expression, referencing the existing textStyleEqual and colorEqual helpers so underline color differences trigger updates.packages/core/src/widgets/__tests__/styleUtils.test.ts (1)
37-41:⚠️ Potential issue | 🟠 MajorFix stale underlineColor expectation shape
At Line 37, the test now passes packed RGB input but still expects an
{ r, g, b }object.sanitizeTextStyle()now emits packedRgb24, so this assertion will fail.Suggested patch
test("sanitizeTextStyle preserves underlineColor rgb", () => { - assert.deepEqual(sanitizeTextStyle({ underlineColor: ((1 << 16) | (2 << 8) | 3) }).underlineColor, { - r: 1, - g: 2, - b: 3, - }); + assert.deepEqual( + sanitizeTextStyle({ underlineColor: ((1 << 16) | (2 << 8) | 3) }).underlineColor, + ((1 << 16) | (2 << 8) | 3), + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/__tests__/styleUtils.test.ts` around lines 37 - 41, The test is asserting that sanitizeTextStyle returns an object {r,g,b} for underlineColor but the implementation now returns a packed Rgb24 integer; update the expectation in the assert.deepEqual call inside the styleUtils.test (the line using sanitizeTextStyle({ underlineColor: ((1 << 16) | (2 << 8) | 3) }).underlineColor) to compare against the packed numeric value ((1 << 16) | (2 << 8) | 3) instead of an { r, g, b } object so the assertion matches the new Rgb24 output.packages/core/src/drawlist/__tests__/builder.round-trip.test.ts (1)
385-406:⚠️ Potential issue | 🟡 MinorRename stale
v2test titles to match actual protocol assertions.These test names still say
v2, but Line 396 and Line 519 assertZR_DRAWLIST_VERSION_V1. The mismatch makes failures harder to interpret.Based on learnings: In the Rezi project, public API references and documentation must consistently use the current public protocol version (v1) after the reset.✏️ Suggested title updates
- test("v2 header uses version 2 and correct cmd byte/count totals", () => { + test("v1 header uses version 1 and correct cmd byte/count totals", () => { @@ - test("v2 setCursor readback preserves payload fields and reserved byte", () => { + test("v1 setCursor readback preserves payload fields and reserved byte", () => { @@ - test("v2 multiple cursor commands are emitted in-order", () => { + test("v1 multiple cursor commands are emitted in-order", () => { @@ - test("v2 mixed frame keeps aligned sections and expected total byte size", () => { + test("v1 mixed frame keeps aligned sections and expected total byte size", () => {Also applies to: 426-506
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/__tests__/builder.round-trip.test.ts` around lines 385 - 406, Test titles in builder.round-trip.test.ts incorrectly reference "v2" while assertions (e.g., comparing h.version to ZR_DRAWLIST_VERSION_V1 in the tests that call createDrawlistBuilder(), readHeader(), assert.equal(... ZR_DRAWLIST_VERSION_V1)) verify the v1 protocol; update those test name strings (for example the tests currently titled "v2 header uses version 2 and correct cmd byte/count totals" and "v2 setCursor readback preserves payload fields and reserved byte" and any other "v2" titled tests in the same block) to reflect v1 (or remove the version prefix) so the descriptions match the asserted ZR_DRAWLIST_VERSION_V1 behavior.packages/core/src/drawlist/__tests__/builder.cursor.test.ts (1)
2-10:⚠️ Potential issue | 🟡 MinorUpdate stale v2 wording in the test header comment.
The block comment still says “Correct v2 header version,” but this test validates
ZR_DRAWLIST_VERSION_V1.📝 Proposed fix
- * - Correct v2 header version + * - Correct current header version (v1)🤖 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 2 - 10, The test header comment incorrectly states "Correct v2 header version" while the test validates ZR_DRAWLIST_VERSION_V1; update the block comment in builder.cursor.test.ts to refer to the v1 header version (e.g., "Correct v1 header version" or "Correct header version (v1)") and ensure any other v2 wording in that top comment is changed to match ZR_DRAWLIST_VERSION_V1 so the comment accurately reflects the test's intent.packages/core/src/renderer/__tests__/renderer.damage.test.ts (1)
486-526:⚠️ Potential issue | 🟠 MajorPacked color migration currently weakens style assertions in this test harness.
colorKey()doesn’t handle numericRgb24, so distinct packed colors collapse to"?". This can hide real color regressions in framebuffer comparisons.💡 Proposed fix
function colorKey(value: unknown): string { if (value === undefined || value === null) return ""; if (typeof value === "string") return `s:${value}`; + if (typeof value === "number" && Number.isFinite(value)) { + return `n:${String((value >>> 0) & 0xffffff)}`; + } if (typeof value === "object") { const rgb = value as { r?: unknown; g?: unknown; b?: unknown }; if ( typeof rgb.r === "number" &&🤖 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 486 - 526, The test harness collapses distinct packed colors because colorKey() doesn't handle numeric Rgb24 values, causing different packed colors to become "?" and masking regressions; update colorKey() to detect numeric packed RGB24 values (e.g., the same packed ints used in renderer.damage.test.ts and represented by type Rgb24) and return a stable unique string (such as a hex "#RRGGBB" or "rgb(r,g,b)") instead of "?", so framebuffer comparisons and style assertions remain precise; modify the implementation used by the tests (where colorKey is referenced) to convert the packed int into separate R, G, B components and produce that deterministic representation.
🧹 Nitpick comments (14)
packages/ink-compat/src/_bench_layout_profile.ts (2)
44-55: IncludeframeCountin the dump output for complete snapshots.
frameCountis tracked and incremented, but it’s missing fromdumpLayoutCounters, so profile dumps are slightly incomplete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/_bench_layout_profile.ts` around lines 44 - 55, The layout profile dump is missing the tracked frameCount value; update the dumpLayoutCounters output (where the array of lines is built using c.* values) to include a line like `frameCount: ${c.frameCount}` so the snapshot includes frameCount alongside layoutNodeCalls, measureNodeCalls, etc.; locate the array construction in dumpLayoutCounters (references to c.layoutNodeCalls, c.measureCacheHits, c.textMeasureTotalMs) and insert the frameCount entry in the list.
13-39: Avoid duplicating the counter schema across initialization and reset.The same field list is maintained in two places (
layoutCountersinit andresetLayoutCounters), which is easy to desync when adding/removing counters.♻️ Proposed refactor
+type LayoutCounters = { + layoutNodeCalls: number; + measureNodeCalls: number; + measureCacheHits: number; + layoutCacheHits: number; + constraintPassCount: number; + textMeasureCalls: number; + textMeasureTotalChars: number; + textMeasureTotalMs: number; + stackLayoutCalls: number; + leafLayoutCalls: number; + frameCount: number; +}; + +const ZERO_LAYOUT_COUNTERS: Omit<LayoutCounters, "frameCount"> = { + layoutNodeCalls: 0, + measureNodeCalls: 0, + measureCacheHits: 0, + layoutCacheHits: 0, + constraintPassCount: 0, + textMeasureCalls: 0, + textMeasureTotalChars: 0, + textMeasureTotalMs: 0, + stackLayoutCalls: 0, + leafLayoutCalls: 0, +}; + -export const layoutCounters = { - layoutNodeCalls: 0, - measureNodeCalls: 0, - measureCacheHits: 0, - layoutCacheHits: 0, - constraintPassCount: 0, - textMeasureCalls: 0, - textMeasureTotalChars: 0, - textMeasureTotalMs: 0, - stackLayoutCalls: 0, - leafLayoutCalls: 0, - frameCount: 0, -}; +export const layoutCounters: LayoutCounters = { ...ZERO_LAYOUT_COUNTERS, frameCount: 0 }; export function resetLayoutCounters(): void { - layoutCounters.layoutNodeCalls = 0; - layoutCounters.measureNodeCalls = 0; - layoutCounters.measureCacheHits = 0; - layoutCounters.layoutCacheHits = 0; - layoutCounters.constraintPassCount = 0; - layoutCounters.textMeasureCalls = 0; - layoutCounters.textMeasureTotalChars = 0; - layoutCounters.textMeasureTotalMs = 0; - layoutCounters.stackLayoutCalls = 0; - layoutCounters.leafLayoutCalls = 0; - layoutCounters.frameCount += 1; + const nextFrame = layoutCounters.frameCount + 1; + Object.assign(layoutCounters, ZERO_LAYOUT_COUNTERS, { frameCount: nextFrame }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/_bench_layout_profile.ts` around lines 13 - 39, The counter fields are duplicated between the layoutCounters object and resetLayoutCounters, risking drift; create a single source of truth (e.g., an initialLayoutCounters constant or a function getInitialLayoutCounters()) and have resetLayoutCounters reuse it (for example via Object.assign(layoutCounters, initialLayoutCounters) and then increment layoutCounters.frameCount) instead of manually resetting each field; update references to layoutCounters and resetLayoutCounters accordingly so adding/removing counters only requires changing the initializer.packages/core/src/renderer/__tests__/renderer.scrollbar.test.ts (1)
508-508: Packed RGB color representation is correct, butObject.freezeon a primitive is a no-op.The packed 24-bit RGB value
((1 << 16) | (2 << 8) | 3)correctly encodes RGB(1,2,3) as0x010203. However,Object.freeze()has no effect on primitive numbers in JavaScript—it returns the value unchanged. Consider removing the freeze for clarity:🔧 Suggested simplification
- const customFg = Object.freeze(((1 << 16) | (2 << 8) | 3)); + const customFg = (1 << 16) | (2 << 8) | 3;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/renderer.scrollbar.test.ts` at line 508, The test declares const customFg = Object.freeze(((1 << 16) | (2 << 8) | 3)) but Object.freeze is a no-op on primitives; remove the unnecessary Object.freeze wrapper around the packed RGB expression so customFg is simply the numeric value ((1 << 16) | (2 << 8) | 3), leaving the packed 24-bit RGB representation intact and clearer in the test.packages/core/src/widgets/commandPalette.ts (1)
284-290: Replace hardcoded palette RGB constants with theme token keys.This keeps command palette visuals aligned with theme overrides and avoids hardcoded color drift.
As per coding guidelines "Never hardcode RGB color values; let the theme handle colors via tokens. Use
variantprops on badges, tags, callouts, status indicators for semantic colors. UsedsVariant/dsToneon buttons and interactive widgets for consistent theming".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/commandPalette.ts` around lines 284 - 290, PALETTE_COLORS currently hardcodes rgb(...) values; replace these with theme token keys so the command palette follows theme overrides. Update the PALETTE_COLORS export to map keys (background, border, selectedBg, highlight, placeholder) to appropriate theme tokens or token-resolving helpers used in the codebase (e.g., use tokens like token('palette.background') or theme.get('palette.background')), and for interactive/semantic colors prefer using component props (dsVariant/dsTone or variant on badges/tags/callouts/status indicators) where those colors are applied; ensure any consumer of PALETTE_COLORS expects token strings or resolved values consistently.packages/core/src/pipeline.ts (1)
34-35: Avoid exporting temporary profiling hooks from the shared pipeline entrypoint.
__layoutProfileis labeled temporary, but exporting it here makes downstream coupling likely. Prefer keeping it on an internal/unstable path to avoid accidental API lock-in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/pipeline.ts` around lines 34 - 35, The temporary profiling symbol __layoutProfile should not be exported from the shared pipeline entrypoint; remove the export statement "export { __layoutProfile } from './layout/engine/layoutEngine.js'" from the pipeline entrypoint and instead expose it only from an internal/unstable module (or behind a clearly marked unstable flag/namespace) so downstream code doesn't accidentally depend on it; update any consumers to import __layoutProfile from the new internal/unstable path or the guarded export location.packages/core/src/widgets/splitPane.ts (1)
373-373: Consider using a theme token for divider color.The migration from
{ r: 80, g: 80, b: 80 }torgb(80, 80, 80)correctly aligns with the packed Rgb24 representation. However, per coding guidelines, hardcoded RGB values should be avoided in favor of theme tokens for consistency and theming support.If this is intentional infrastructure code that must remain constant regardless of theme, this is acceptable. Otherwise, consider exposing this as a theme token.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/splitPane.ts` at line 373, DIVIDER_COLOR is currently a hardcoded rgb(80, 80, 80); replace it with a theme token so the divider color follows theming: introduce or use an existing theme token (e.g., "divider" or "colors.divider") and have DIVIDER_COLOR reference that token (with an explicit fallback to rgb(80,80,80) if the theme token is absent) so consumers like the SplitPane component read from the theme instead of a fixed Rgb24 value; update usages of DIVIDER_COLOR accordingly to consume the theme-aware value.packages/core/src/renderer/styles.ts (1)
9-9: Prefer semantic theme color over hardcoded disabled grayAt Line 9, hardcoding
rgb(128, 128, 128)bypasses theme presets and can drift from design-token contrast choices. Consider deriving disabled text color from theme tokens at call site (or by threading theme into this helper).As per coding guidelines "Never hardcode RGB color values; let the theme handle colors via tokens."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/styles.ts` at line 9, Replace the hardcoded DISABLED_FG constant with a theme-driven value: remove or convert const DISABLED_FG = rgb(128, 128, 128) in styles.ts and instead derive the disabled text color from the theme token at use-site or by changing DISABLED_FG into a function that accepts the Theme (e.g., getDisabledFg(theme) or using theme.colors.disabledText / theme.tokens.text.disabled). Update callers to pass the theme or use the theme accessor so colors follow the design tokens rather than a hardcoded RGB.packages/core/src/app/createApp.ts (1)
206-219: Centralize drawlist version policy to avoid drift.Validation is currently split between marker parsing (
[1..5]) and startup enforcement (=== 1). Consider using a single constant/policy path so future version changes only need one edit.Also applies to: 598-605
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/createApp.ts` around lines 206 - 219, Centralize the drawlist version policy by introducing a single constant (e.g., ALLOWED_DRAWLIST_VERSIONS or DRAWLIST_VERSION_POLICY with min/max/current) and use it in readBackendDrawlistVersionMarker (replace the hard-coded [1..5] checks) and in the startup enforcement logic (the code that currently checks === 1) so both parsing/validation and startup enforcement reference the same constant; update readBackendDrawlistVersionMarker and the startup check to read from that shared constant and adjust the invalidProps message to reference the policy constant.packages/core/src/drawlist/__tests__/builder_style_attrs.test.ts (1)
33-77: Deduplicate equivalent dim-attr tests after unified builder migration.These cases now hit the same builder path and no longer validate distinct behavior. Consolidating them will reduce noise and keep the suite intent clear.
♻️ Suggested cleanup
- test("drawText attrs include dim without shifting existing bits (repeat)", () => { - const b = createDrawlistBuilder(); - b.drawText(0, 0, "dim", { dim: true }); - const res = b.build(); - assert.equal(res.ok, true); - if (!res.ok) return; - - assert.equal(drawTextAttrs(res.bytes), 1 << 4); - }); + // Removed duplicate case: identical coverage to previous drawText dim test. - test("v1 text-run attrs include dim without shifting existing bits", () => { + test("text-run attrs include dim without shifting existing bits", () => { const b = createDrawlistBuilder(); const blobIndex = b.addTextRunBlob([ { text: "dim", style: { dim: true } }, { text: "base", style: { bold: true, italic: true, underline: true, inverse: true } }, ]); @@ - test("v2 text-run attrs include dim without shifting existing bits", () => { - const b = createDrawlistBuilder(); - const blobIndex = b.addTextRunBlob([ - { text: "dim", style: { dim: true } }, - { text: "base", style: { bold: true, italic: true, underline: true, inverse: true } }, - ]); - assert.equal(blobIndex, 0); - if (blobIndex === null) return; - - b.drawTextRun(0, 0, blobIndex); - const res = b.build(); - assert.equal(res.ok, true); - if (!res.ok) return; - - assert.equal(textRunAttrs(res.bytes, 0), 1 << 4); - assert.equal(textRunAttrs(res.bytes, 1), (1 << 0) | (1 << 1) | (1 << 2) | (1 << 3)); - }); + // Removed duplicate text-run case: same setup and assertions.Based on learnings: In the Rezi project, public API references and documentation must consistently use the current public protocol version (v1) after the reset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/__tests__/builder_style_attrs.test.ts` around lines 33 - 77, Consolidate duplicate dim-attribute tests: keep the drawText attrs test for drawText (createDrawlistBuilder + drawText + drawTextAttrs) and replace the two redundant text-run tests ("v1 text-run attrs include dim..." and "v2 text-run attrs include dim...") with a single text-run test that adds a blob via addTextRunBlob, calls drawTextRun, builds, and asserts textRunAttrs for index 0 equals 1<<4 and for index 1 equals (1<<0)|(1<<1)|(1<<2)|(1<<3); remove the extra v1/v2 variants so the suite validates the unified builder path using createDrawlistBuilder, addTextRunBlob, drawTextRun, textRunAttrs, and drawTextAttrs exactly once each.packages/core/src/app/__tests__/rawRender.test.ts (1)
31-39: KeepbuildInto()consistent withbuild()in the main stub.Line 31-35 enforces single-build behavior, but Line 37-39 bypasses it. If the runtime uses
buildInto(), this test double can mask duplicate-build regressions.♻️ Suggested adjustment
- buildInto() { - return { ok: true, bytes } as const; - }, + buildInto() { + return this.build(); + },🤖 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 31 - 39, The test stub's buildInto() bypasses the single-build guard present in build(): update buildInto() to mirror build() behavior by checking the same built flag, returning the identical error shape ({ ok: false, error: { code: "ZRDL_INTERNAL", detail: "built twice" } } as const) when built is already true, and otherwise set built = true and return { ok: true, bytes } as const; keep the same const type assertions and use the same bytes variable so both build() and buildInto() enforce single-build semantics.packages/core/src/theme/resolve.ts (1)
68-75: JSDoc examples show outdated RGB object format.The code examples in the docstring still show
{ r: 230, g: 225, b: 207 }object format, but the function now returnsRgb24(packed 24-bit integer). Consider updating the examples to reflect the new return type.📝 Suggested documentation update
* `@example` * ```typescript * const color = resolveColorToken(darkTheme, "fg.primary"); -* // { r: 230, g: 225, b: 207 } +* // 0xe6e1cf (packed RGB24) * * const error = resolveColorToken(darkTheme, "error"); -* // { r: 240, g: 113, b: 120 } +* // 0xf07178 (packed RGB24) * ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/theme/resolve.ts` around lines 68 - 75, Update the JSDoc examples in the resolveColorToken docstring to show the new Rgb24 packed 24-bit integer return format instead of the old { r,g,b } object; specifically change the example outputs for resolveColorToken(darkTheme, "fg.primary") and resolveColorToken(darkTheme, "error") to their hex-packed Rgb24 values (e.g., 0xe6e1cf and 0xf07178) and mention "packed RGB24" so the example matches the actual return type of resolveColorToken and any related types like Rgb24.packages/core/src/drawlist/__tests__/builder.reset.test.ts (1)
103-104: Consider updating test names to reflect unified builder.The test names still reference "v1" and "v2" (e.g., "v1 reset clears prior commands", "v2 reset drops cursor"), but these now use the unified
createDrawlistBuilder(). Consider renaming to avoid confusion, e.g., "reset clears prior commands/strings/blobs" and "reset drops cursor and string state".Also applies to: 134-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/__tests__/builder.reset.test.ts` around lines 103 - 104, Tests still reference "v1"/"v2" but use the unified createDrawlistBuilder(); rename the test titles (e.g., change "v1 reset clears prior commands/strings/blobs" to "reset clears prior commands/strings/blobs" and "v2 reset drops cursor" to "reset drops cursor and string state") so names reflect the unified builder; update the test descriptions in the files that call createDrawlistBuilder() (the tests around the occurrences currently labeled with v1/v2) to remove version prefixes and use the suggested clearer wording.packages/ink-compat/src/translation/propsToVNode.ts (1)
501-502: HoistNATIVE_PERCENT_PROPSto module scope to avoid per-node allocations.
new Set([...])is created on everytranslateBox()call.🤖 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 501 - 502, NATIVE_PERCENT_PROPS is being instantiated inside translateBox() causing a new Set allocation on every call; move the declaration const NATIVE_PERCENT_PROPS = new Set(["width","height","flexBasis"]) to module scope (top of the file) so translateBox() and any other functions reuse the single Set instance, leaving all uses of NATIVE_PERCENT_PROPS unchanged.packages/core/src/drawlist/__tests__/builder_v6_resources.test.ts (1)
10-10: Rename legacyv6test artifact tov1terminology.The test file naming still carries pre-reset version wording (
builder_v6_*), which can mislead readers about the active public protocol version.Based on learnings: In the Rezi project, public API references and documentation must consistently use the current public protocol version (v1) after the reset, and must not reference the old v6 iteration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/__tests__/builder_v6_resources.test.ts` at line 10, Rename legacy v6 references to v1 in this test: change the file name from builder_v6_resources.test.ts to builder_v1_resources.test.ts and update any in-file artifacts that mention "v6" (including test descriptions, snapshot names, constants, or import paths) to use "v1" instead; specifically update the describe string and any identifiers or fixtures referencing "v6" so they read "v1" (e.g., update "DrawlistBuilder resource inputs" contexts or any variables like builder_v6_* to builder_v1_*), run tests to ensure imports/snapshots are found and update snapshot filenames if needed.
🤖 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/src/profile-packed-style.ts`:
- Around line 75-77: The three computed-key property accesses should use dot
notation for clarity and to satisfy linting: change
counters["style_merges_performed"] ?? 0 to counters.style_merges_performed ?? 0,
counters["style_objects_created"] ?? 0 to counters.style_objects_created ?? 0,
and counters["packRgb_calls"] ?? 0 to counters.packRgb_calls ?? 0; update the
assignments for merges, styleObjects, and packRgbCalls accordingly while
preserving the nullish-coalescing defaults.
In `@packages/core/src/animation/interpolate.ts`:
- Line 5: The import specifiers in interpolate.ts are not sorted per CI; reorder
the specifiers in the import from "../widgets/style.js" to match the project's
import-sorting rule (alphabetical, case-insensitive) so that the names (rgb,
rgbB, rgbG, rgbR, type Rgb24) appear in the correct sorted order.
In `@packages/core/src/app/widgetRenderer.ts`:
- Around line 541-543: Remove the redundant union types and unused helper:
delete the unused isV2Builder function and replace any parameter or field
annotations that read as redundant unions (e.g., "DrawlistBuilder |
DrawlistBuilder" and "DrawlistBuilder | DrawlistBuilder | DrawlistBuilder") with
the single type DrawlistBuilder; update the builder field declaration (the
member named "builder") to be typed simply as DrawlistBuilder and remove any
references to isV2Builder so dead/unneeded code is eliminated.
In `@packages/core/src/drawlist/builder.ts`:
- Around line 188-191: The validation currently masks state.shape with bitwise
(& 0xff) before checking range, which hides non-finite values; update the
setCursor handling to validate state.shape for finitude and integrality before
coercion (use the existing requireI32 or the same pre-coercion check used for
x/y and zLayer in drawImage) and only then compute const shape = state.shape &
0xff;; if validation fails call this.fail("ZRDL_BAD_PARAMS", ...) as before.
Ensure you reference state.shape, validateParams, requireI32 (or the same check
used for zLayer) and keep the allowed range check for 0–2 after coercion.
In `@packages/core/src/runtime/commit.ts`:
- Around line 1295-1299: The diagnostic object pushed into __commitDiag should
assign detail directly instead of using a redundant type assertion; update the
object literal in the __commitDiag.push call (the one using instanceId,
vnode.kind and childDiffs) to set detail: "children-changed" without the as
"was-dirty" | undefined cast so the value matches the declared union type and
remains consistent with other pushes in this file.
In `@packages/core/src/theme/extract.ts`:
- Around line 7-8: The falsy check on the packed color token (const bgBase =
c["bg.base"] as Rgb24 | undefined; if (!bgBase) return null;) incorrectly treats
a valid black value (0) as missing; change the presence check to explicitly test
for undefined/null (e.g., if (bgBase === undefined) return null or if (bgBase ==
null) return null) so that Rgb24 value 0 is accepted and semantic token
extraction continues; update any similar checks in this module that use
truthiness on packed color tokens (references: bgBase, c["bg.base"], Rgb24).
In `@packages/core/src/widgets/__tests__/graphicsWidgets.test.ts`:
- Around line 169-173: The test "link encodes hyperlink refs in v3 and degrades
to text in v1" incorrectly calls renderBytes with the same
createDrawlistBuilder() for both v3 and v1, so it no longer tests distinct
paths; fix by either constructing v1 with the legacy builder (e.g., use the
legacy builder factory used elsewhere such as createLegacyDrawlistBuilder or
createDrawlistBuilderV1) when calling renderBytes for the v1 variable, or
simplify the test to only assert the v3 behavior by removing the v1 render and
its assertion; update references to ui.link, renderBytes, createDrawlistBuilder
(and the legacy builder name you choose), and parseOpcodes accordingly so the
test again validates two different builder paths or only v3 as intended.
In `@packages/core/src/widgets/__tests__/overlays.typecheck.ts`:
- Around line 26-27: Update the stale `@ts-expect-error` comment above the
declaration of dropdownFrameInvalid: DropdownProps["frameStyle"] to reflect the
current type mismatch (object literal vs numeric packed color) instead of
"missing b component"; locate the const dropdownFrameInvalid definition and
change the rationale text to state that the error is due to using an object {
border: { r: 1, g: 2 } } where a packed numeric color is expected.
In `@packages/core/src/widgets/diffViewer.ts`:
- Around line 232-238: The colors for the diff widget are hardcoded as rgb(...)
literals (addBg, deleteBg, addFg, deleteFg, hunkHeader, lineNumber, border) —
replace these with theme/design-system tokens or semantic variant props so
colors are centrally themeable; locate the color map in diffViewer.ts (the
object containing addBg/deleteBg/etc) and swap each rgb(...) value for the
appropriate token lookup or semantic variant (e.g., using the app theme API or
Badge/Status variants) so addition/deletion/background/foreground/hunk
header/line number/border use theme tokens instead of raw RGBs, keeping the same
semantic mapping.
In `@packages/core/src/widgets/logsConsole.ts`:
- Around line 209-215: LEVEL_COLORS currently uses hardcoded rgb(...) literals;
replace these with theme/design tokens so colors come from the centralized theme
system. Import and use the appropriate token accessors (e.g., your design token
map or helper like getTokenColor/resolveColor) and map each LogLevel key in
LEVEL_COLORS (trace, debug, info, warn, error) to the corresponding semantic
token instead of rgb(...), ensuring the values are converted/typed to Rgb24 (or
use an existing token-to-Rgb24 helper) so types (LogLevel, Rgb24) still match.
In `@packages/core/src/widgets/style.ts`:
- Around line 5-6: The Rgb24 typedef's comment incorrectly claims value 0 is a
reserved/unset sentinel while rgb(r,g,b) encodes black as 0; update the
documentation for Rgb24 to reflect the actual encoding semantics (packed
0x00RRGGBB) and that 0 represents black (rgb(0,0,0)) rather than being reserved,
and ensure any mention of rgb() behavior in the same file (e.g., the rgb
function) is consistent with the revised comment.
In `@packages/core/src/widgets/toast.ts`:
- Around line 172-177: TOAST_COLORS currently hardcodes RGB values (rgb(…)) for
each Toast["type"]); replace those literal RGBs with theme/design tokens or
semantic variants so the toast border colors come from the design system. Update
the TOAST_COLORS mapping to reference the theme token names or semantic props
(e.g., use token keys or a helper that maps toast types to token values) and,
where applicable, switch to using component semantic props like
variant/dsVariant or dsTone on the toast Badge/Callout/StatusIndicator
components instead of raw rgb(...) so palette overrides and theming are
respected.
In `@packages/ink-compat/src/runtime/createInkRenderer.ts`:
- Around line 523-526: The early-return in createInkRenderer's fast path (when
isFirstFrame/forceLayout/prevRoot.dirty checks allow returning
cachedOps/cachedNodes) skips emitting any tracing events; update that branch to
emit/complete the same trace/span used elsewhere in this module (the module's
trace/tracer API) before returning and include timings (commitMs, totalMs and
layoutSkipped: true) so skipped frames appear in traces/metrics; locate the
fast-path using the variables isFirstFrame, forceLayout, prevRoot.dirty,
cachedLayoutTree, cachedOps and cachedNodes and call the existing trace API in
the same manner as other code paths rather than returning immediately.
In `@packages/ink-compat/src/runtime/render.ts`:
- Around line 2517-2543: The appendFileSync call in the frame profiling block
should avoid string concatenation and the use of the any type: build the object
to stringify first, extracting a safely-typed layout profile via a type guard
(e.g., const timings = result.timings as unknown; if (timings && typeof timings
=== 'object' && '_layoutProfile' in timings) { layoutProfile = { _lp: (timings
as any)['_layoutProfile'] } } else layoutProfile = {};), merge that
layoutProfile into the payload, then call appendFileSync(frameProfileFile,
`${JSON.stringify(payload)}\n`) using a template literal; update references to
result.timings/_layoutProfile and result.ops/result.nodes as needed in the
payload construction and remove the inline any cast.
- Around line 1160-1172: The checks treating 0 as "unset" prevent explicit black
(0x000000) from being applied; update the predicates in render.ts so they test
for undefined/null rather than falsy zero: replace `style.fg !== 0` and
`style.bg !== 0` with explicit existence checks (e.g., `style.fg !== undefined`
/ `style.bg !== undefined`) in the normalization block that uses isRgb24,
clampRgb24 and CORE_DEFAULT_FG/CORE_DEFAULT_BG, and likewise replace falsy
checks in styleToSgr (`if (style.fg)` and `if (style.bg)`) with explicit
undefined checks; alternatively, choose and document a distinct sentinel (e.g.,
-1) across the codebase and update usages accordingly so 0 (black) is treated as
a valid color.
In `@packages/ink-compat/src/translation/colorMap.ts`:
- Around line 54-59: The trace code incorrectly treats result as falsy so a
valid black color (packed Rgb24 value 0) is recorded as null; change the
conditional that builds the trace payload to check for undefined (e.g., result
!== undefined) instead of using a truthy check so pushTranslationTrace receives
{ r: rgbR(result), g: rgbG(result), b: rgbB(result) } for valid black parses;
update the block around isTranslationTraceEnabled()/pushTranslationTrace and
reference result, rgbR, rgbG, rgbB, and parseColorInner to ensure only undefined
indicates parse failure.
In `@packages/ink-compat/src/translation/propsToVNode.ts`:
- Around line 15-16: Props color checks treat numeric 0 (black) as falsy; update
the checks in propsToVNode.ts to detect presence explicitly (e.g., use fg !==
undefined && fg !== null or fg != null) instead of if (fg) so 0x000000 is
preserved; apply the same change for bg and borderColor (and any helper like
mapColorProp or serializeColor) wherever you currently use truthy checks to gate
color propagation.
In `@packages/native/vendor/zireael/src/core/zr_drawlist.h`:
- Around line 89-101: Add documentation to the backend native docs (or a new
resource management section) describing the C resource lifecycle and how it
integrates with drawlist execution: explain the purpose and usage of
zr_dl_resources_init, zr_dl_resources_release, zr_dl_resources_swap,
zr_dl_resources_clone, and zr_dl_resources_clone_shallow; describe how
zr_dl_preflight_resources and zr_dl_execute interact with the resource subsystem
during preflight/execute, why preflight takes shallow snapshots for stage
borrowing (zr_image_frame_t image_stage / image_frame_stage) versus when deep
clones are required, and give guidance/examples for correct ownership semantics
and when to call init/release/swap to avoid leaks or double-free. Ensure the
docs clarify the behavioral differences between deep vs shallow clones and when
each function should be used in the persistent drawlist resource lifecycle.
In `@packages/native/vendor/zireael/src/core/zr_engine_present.inc`:
- Around line 341-365: The code clears e->diff_prev_hashes_valid on
copy/clone/fallback errors but zr_engine_swap_diff_hashes_on_commit later
re-enables it, undoing the invalidation; update the logic so a failure in
zr_fb_copy_damage_rects, zr_fb_links_clone_from or the fb_prev shape check
permanently preserves the invalid state across the commit swap. Concretely,
either (A) add/keep a preservation flag (e.g.
e->preserve_diff_prev_hashes_invalidation) set to true when you set
e->diff_prev_hashes_valid = 0u in the failure branches and have
zr_engine_swap_diff_hashes_on_commit check that flag before re-enabling hashes,
or (B) change zr_engine_swap_diff_hashes_on_commit to respect the current
e->diff_prev_hashes_valid (only enable hashes if it is already true) so the
earlier invalidation from the branches (those using zr_fb_copy_damage_rects and
zr_fb_links_clone_from) is not overwritten.
In `@packages/native/vendor/zireael/src/core/zr_framebuffer.c`:
- Around line 208-270: zr_fb_copy_damage_rects copies zr_cell_t (including
style.link_ref) but doesn't sync dst link tables, causing copied link references
to be invalid; after the initial input validation (in function
zr_fb_copy_damage_rects, before the for-loop that copies rects) call
zr_fb_links_reset(dst) and then return or call zr_fb_links_clone_from(dst, src)
as in zr_engine_fb_copy_noalloc to synchronize dst->links and dst->link_bytes
with src; follow the same ordering/pattern used in zr_engine_fb_copy_noalloc
(memcpy then zr_fb_links_reset(dst) and zr_fb_links_clone_from(dst, src)) so
link tables match after damage-region copying.
---
Outside diff comments:
In `@packages/core/src/drawlist/__tests__/builder.cursor.test.ts`:
- Around line 2-10: The test header comment incorrectly states "Correct v2
header version" while the test validates ZR_DRAWLIST_VERSION_V1; update the
block comment in builder.cursor.test.ts to refer to the v1 header version (e.g.,
"Correct v1 header version" or "Correct header version (v1)") and ensure any
other v2 wording in that top comment is changed to match ZR_DRAWLIST_VERSION_V1
so the comment accurately reflects the test's intent.
In `@packages/core/src/drawlist/__tests__/builder.round-trip.test.ts`:
- Around line 385-406: Test titles in builder.round-trip.test.ts incorrectly
reference "v2" while assertions (e.g., comparing h.version to
ZR_DRAWLIST_VERSION_V1 in the tests that call createDrawlistBuilder(),
readHeader(), assert.equal(... ZR_DRAWLIST_VERSION_V1)) verify the v1 protocol;
update those test name strings (for example the tests currently titled "v2
header uses version 2 and correct cmd byte/count totals" and "v2 setCursor
readback preserves payload fields and reserved byte" and any other "v2" titled
tests in the same block) to reflect v1 (or remove the version prefix) so the
descriptions match the asserted ZR_DRAWLIST_VERSION_V1 behavior.
In `@packages/core/src/renderer/__tests__/renderer.damage.test.ts`:
- Around line 486-526: The test harness collapses distinct packed colors because
colorKey() doesn't handle numeric Rgb24 values, causing different packed colors
to become "?" and masking regressions; update colorKey() to detect numeric
packed RGB24 values (e.g., the same packed ints used in renderer.damage.test.ts
and represented by type Rgb24) and return a stable unique string (such as a hex
"#RRGGBB" or "rgb(r,g,b)") instead of "?", so framebuffer comparisons and style
assertions remain precise; modify the implementation used by the tests (where
colorKey is referenced) to convert the packed int into separate R, G, B
components and produce that deterministic representation.
In `@packages/core/src/runtime/commit.ts`:
- Around line 108-151: The textStyleEqual function currently omits the underline
color field from its type and equality checks, causing underline-only style
changes to be treated as equal; update the style shape in textStyleEqual to
include underlineColor?: unknown (alongside fg/bg) and add a corresponding
comparison using colorEqual(a.underlineColor, b.underlineColor) in the returned
boolean expression, referencing the existing textStyleEqual and colorEqual
helpers so underline color differences trigger updates.
In `@packages/core/src/widgets/__tests__/basicWidgets.render.test.ts`:
- Around line 126-139: renderBytes and renderBytesV3 currently call the same
builder (createDrawlistBuilder) so both produce identical protocol output;
change renderBytes to use the v1 drawlist builder so it emits the legacy/public
protocol (e.g., call the v1 factory such as createDrawlistBuilderV1 or the
builder with version:1 options) while leaving renderBytesV3 using
createDrawlistBuilder(), ensuring tests that assert v1 vs v3 differences can
pass.
In `@packages/core/src/widgets/__tests__/styleUtils.test.ts`:
- Around line 37-41: The test is asserting that sanitizeTextStyle returns an
object {r,g,b} for underlineColor but the implementation now returns a packed
Rgb24 integer; update the expectation in the assert.deepEqual call inside the
styleUtils.test (the line using sanitizeTextStyle({ underlineColor: ((1 << 16) |
(2 << 8) | 3) }).underlineColor) to compare against the packed numeric value ((1
<< 16) | (2 << 8) | 3) instead of an { r, g, b } object so the assertion matches
the new Rgb24 output.
In `@packages/core/src/widgets/field.ts`:
- Around line 11-24: The FIELD_ERROR_STYLE constant uses a hardcoded rgb(...)
value for fg; replace that value with the semantic error color token from the
design/theme (e.g., the theme's error/semantic token) by assigning the token to
FIELD_ERROR_STYLE.fg, and remove the now-unused rgb import at the top (import {
rgb } from "./style.js") if nothing else in this file uses it; update the
Object.freeze definition for FIELD_ERROR_STYLE accordingly so the file uses the
theme token instead of rgb(255,0,0).
---
Nitpick comments:
In `@packages/core/src/app/__tests__/rawRender.test.ts`:
- Around line 31-39: The test stub's buildInto() bypasses the single-build guard
present in build(): update buildInto() to mirror build() behavior by checking
the same built flag, returning the identical error shape ({ ok: false, error: {
code: "ZRDL_INTERNAL", detail: "built twice" } } as const) when built is already
true, and otherwise set built = true and return { ok: true, bytes } as const;
keep the same const type assertions and use the same bytes variable so both
build() and buildInto() enforce single-build semantics.
In `@packages/core/src/app/createApp.ts`:
- Around line 206-219: Centralize the drawlist version policy by introducing a
single constant (e.g., ALLOWED_DRAWLIST_VERSIONS or DRAWLIST_VERSION_POLICY with
min/max/current) and use it in readBackendDrawlistVersionMarker (replace the
hard-coded [1..5] checks) and in the startup enforcement logic (the code that
currently checks === 1) so both parsing/validation and startup enforcement
reference the same constant; update readBackendDrawlistVersionMarker and the
startup check to read from that shared constant and adjust the invalidProps
message to reference the policy constant.
In `@packages/core/src/drawlist/__tests__/builder_style_attrs.test.ts`:
- Around line 33-77: Consolidate duplicate dim-attribute tests: keep the
drawText attrs test for drawText (createDrawlistBuilder + drawText +
drawTextAttrs) and replace the two redundant text-run tests ("v1 text-run attrs
include dim..." and "v2 text-run attrs include dim...") with a single text-run
test that adds a blob via addTextRunBlob, calls drawTextRun, builds, and asserts
textRunAttrs for index 0 equals 1<<4 and for index 1 equals
(1<<0)|(1<<1)|(1<<2)|(1<<3); remove the extra v1/v2 variants so the suite
validates the unified builder path using createDrawlistBuilder, addTextRunBlob,
drawTextRun, textRunAttrs, and drawTextAttrs exactly once each.
In `@packages/core/src/drawlist/__tests__/builder_v6_resources.test.ts`:
- Line 10: Rename legacy v6 references to v1 in this test: change the file name
from builder_v6_resources.test.ts to builder_v1_resources.test.ts and update any
in-file artifacts that mention "v6" (including test descriptions, snapshot
names, constants, or import paths) to use "v1" instead; specifically update the
describe string and any identifiers or fixtures referencing "v6" so they read
"v1" (e.g., update "DrawlistBuilder resource inputs" contexts or any variables
like builder_v6_* to builder_v1_*), run tests to ensure imports/snapshots are
found and update snapshot filenames if needed.
In `@packages/core/src/drawlist/__tests__/builder.reset.test.ts`:
- Around line 103-104: Tests still reference "v1"/"v2" but use the unified
createDrawlistBuilder(); rename the test titles (e.g., change "v1 reset clears
prior commands/strings/blobs" to "reset clears prior commands/strings/blobs" and
"v2 reset drops cursor" to "reset drops cursor and string state") so names
reflect the unified builder; update the test descriptions in the files that call
createDrawlistBuilder() (the tests around the occurrences currently labeled with
v1/v2) to remove version prefixes and use the suggested clearer wording.
In `@packages/core/src/pipeline.ts`:
- Around line 34-35: The temporary profiling symbol __layoutProfile should not
be exported from the shared pipeline entrypoint; remove the export statement
"export { __layoutProfile } from './layout/engine/layoutEngine.js'" from the
pipeline entrypoint and instead expose it only from an internal/unstable module
(or behind a clearly marked unstable flag/namespace) so downstream code doesn't
accidentally depend on it; update any consumers to import __layoutProfile from
the new internal/unstable path or the guarded export location.
In `@packages/core/src/renderer/__tests__/renderer.scrollbar.test.ts`:
- Line 508: The test declares const customFg = Object.freeze(((1 << 16) | (2 <<
8) | 3)) but Object.freeze is a no-op on primitives; remove the unnecessary
Object.freeze wrapper around the packed RGB expression so customFg is simply the
numeric value ((1 << 16) | (2 << 8) | 3), leaving the packed 24-bit RGB
representation intact and clearer in the test.
In `@packages/core/src/renderer/styles.ts`:
- Line 9: Replace the hardcoded DISABLED_FG constant with a theme-driven value:
remove or convert const DISABLED_FG = rgb(128, 128, 128) in styles.ts and
instead derive the disabled text color from the theme token at use-site or by
changing DISABLED_FG into a function that accepts the Theme (e.g.,
getDisabledFg(theme) or using theme.colors.disabledText /
theme.tokens.text.disabled). Update callers to pass the theme or use the theme
accessor so colors follow the design tokens rather than a hardcoded RGB.
In `@packages/core/src/theme/resolve.ts`:
- Around line 68-75: Update the JSDoc examples in the resolveColorToken
docstring to show the new Rgb24 packed 24-bit integer return format instead of
the old { r,g,b } object; specifically change the example outputs for
resolveColorToken(darkTheme, "fg.primary") and resolveColorToken(darkTheme,
"error") to their hex-packed Rgb24 values (e.g., 0xe6e1cf and 0xf07178) and
mention "packed RGB24" so the example matches the actual return type of
resolveColorToken and any related types like Rgb24.
In `@packages/core/src/widgets/commandPalette.ts`:
- Around line 284-290: PALETTE_COLORS currently hardcodes rgb(...) values;
replace these with theme token keys so the command palette follows theme
overrides. Update the PALETTE_COLORS export to map keys (background, border,
selectedBg, highlight, placeholder) to appropriate theme tokens or
token-resolving helpers used in the codebase (e.g., use tokens like
token('palette.background') or theme.get('palette.background')), and for
interactive/semantic colors prefer using component props (dsVariant/dsTone or
variant on badges/tags/callouts/status indicators) where those colors are
applied; ensure any consumer of PALETTE_COLORS expects token strings or resolved
values consistently.
In `@packages/core/src/widgets/splitPane.ts`:
- Line 373: DIVIDER_COLOR is currently a hardcoded rgb(80, 80, 80); replace it
with a theme token so the divider color follows theming: introduce or use an
existing theme token (e.g., "divider" or "colors.divider") and have
DIVIDER_COLOR reference that token (with an explicit fallback to rgb(80,80,80)
if the theme token is absent) so consumers like the SplitPane component read
from the theme instead of a fixed Rgb24 value; update usages of DIVIDER_COLOR
accordingly to consume the theme-aware value.
In `@packages/ink-compat/src/_bench_layout_profile.ts`:
- Around line 44-55: The layout profile dump is missing the tracked frameCount
value; update the dumpLayoutCounters output (where the array of lines is built
using c.* values) to include a line like `frameCount: ${c.frameCount}` so the
snapshot includes frameCount alongside layoutNodeCalls, measureNodeCalls, etc.;
locate the array construction in dumpLayoutCounters (references to
c.layoutNodeCalls, c.measureCacheHits, c.textMeasureTotalMs) and insert the
frameCount entry in the list.
- Around line 13-39: The counter fields are duplicated between the
layoutCounters object and resetLayoutCounters, risking drift; create a single
source of truth (e.g., an initialLayoutCounters constant or a function
getInitialLayoutCounters()) and have resetLayoutCounters reuse it (for example
via Object.assign(layoutCounters, initialLayoutCounters) and then increment
layoutCounters.frameCount) instead of manually resetting each field; update
references to layoutCounters and resetLayoutCounters accordingly so
adding/removing counters only requires changing the initializer.
In `@packages/ink-compat/src/translation/propsToVNode.ts`:
- Around line 501-502: NATIVE_PERCENT_PROPS is being instantiated inside
translateBox() causing a new Set allocation on every call; move the declaration
const NATIVE_PERCENT_PROPS = new Set(["width","height","flexBasis"]) to module
scope (top of the file) so translateBox() and any other functions reuse the
single Set instance, leaving all uses of NATIVE_PERCENT_PROPS unchanged.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/drawlist/__tests__/builder.round-trip.test.ts (1)
381-398:⚠️ Potential issue | 🟡 MinorRename stale
v2test names to match v1 assertions.These test titles still say “v2” (and one says “version 2”), but the body intentionally validates
ZR_DRAWLIST_VERSION_V1. Rename-only cleanup will prevent misleading CI failures.Proposed rename-only diff
- test("v2 header uses version 2 and correct cmd byte/count totals", () => { + test("v1 header uses version 1 and correct cmd byte/count totals", () => { @@ - test("v2 setCursor readback preserves payload fields and reserved byte", () => { + test("v1 setCursor readback preserves payload fields and reserved byte", () => { @@ - test("v2 multiple cursor commands are emitted in-order", () => { + test("v1 multiple cursor commands are emitted in-order", () => { @@ - test("v2 mixed frame keeps aligned sections and expected total byte size", () => { + test("v1 mixed frame keeps aligned sections and expected total byte size", () => {Based on learnings: In the Rezi project, public API references and documentation must consistently use the current public protocol version (v1) after the reset.
Also applies to: 400-420, 422-463, 501-529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/__tests__/builder.round-trip.test.ts` around lines 381 - 398, Rename the stale test titles that say "v2" or "version 2" to reflect v1 so they match the assertions (which check ZR_DRAWLIST_VERSION_V1); update the test descriptions in the test cases around the assertions and helper calls (e.g., the test currently named "v2 header uses version 2 and correct cmd byte/count totals") to mention v1/version 1, and do the same for the other affected tests referenced (lines ~400-420, 422-463, 501-529) so test names accurately reflect the verified constant ZR_DRAWLIST_VERSION_V1.packages/core/src/widgets/__tests__/basicWidgets.render.test.ts (1)
126-138:⚠️ Potential issue | 🟠 MajorThese test assertions will fail because the pre-alpha ABI reset unified v1 and v3 builders.
After commit ede8948 (drawlist: make v1 the only protocol), there is only a single
createDrawlistBuilder()implementation. However, the test functionsrenderBytes()andrenderBytesV3()both call the same unified builder, producing identical output. The tests at lines 555–565 and 567–616 expect different behavior:
- Line 563–564 expect v3 to encode
linkUriRef > 0but v1 to havelinkUriRef === 0- Line 606–614 expect v1 to have zero values for
underlineStyle,underlineColorRgb, andlinkUriRefBoth assertions cannot pass with a unified builder that encodes these fields in all cases. Either remove/rename the test functions to reflect that v1 is now the only protocol, or delete the
renderBytesV3()helper entirely if it no longer serves a purpose.🤖 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 126 - 138, renderBytesV3 duplicates renderBytes but the ABI now uses a single unified createDrawlistBuilder(), so having a V3-specific helper causes tests to expect differing outputs that no longer exist; remove the renderBytesV3 function (and any references) or consolidate tests to use renderBytes only, and update the assertions that expected v1/v3 differences (e.g., checks for linkUriRef, underlineStyle, underlineColorRgb) so they match the unified builder behavior; key symbols: renderBytes, renderBytesV3, createDrawlistBuilder, and the failing assertions around linkUriRef/underlineStyle/underlineColorRgb.
♻️ Duplicate comments (4)
packages/core/src/widgets/toast.ts (1)
172-177:⚠️ Potential issue | 🟠 MajorReplace hardcoded toast RGB literals with semantic theme tokens.
Line 174 through Line 177 still hardcode palette values, which bypasses theming and design-system overrides for toast semantics.
As per coding guidelines: "Never hardcode RGB color values; let the theme handle colors via tokens. Use
variantprops on badges, tags, callouts, status indicators for semantic colors. UsedsVariant/dsToneon buttons and interactive widgets for consistent theming."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/toast.ts` around lines 172 - 177, TOAST_COLORS currently hardcodes RGB values (using rgb(...)) which bypasses theming; replace these literals by mapping Toast["type"] to semantic theme tokens/variants instead. Update the TOAST_COLORS definition so it references the design-system theme tokens or a centralized color-variant lookup (e.g., use the project's semantic token accessor or badge/tag variant mapping) for keys info, success, warning, error, and remove direct rgb(...) calls; ensure the map still returns Rgb24 values by converting tokens via the existing theme-to-Rgb24 helper (or the same helper used elsewhere in widgets) so toast rendering uses theme-driven colors.packages/ink-compat/src/runtime/render.ts (1)
1157-1166:⚠️ Potential issue | 🟠 MajorTreat
0x000000as a valid color, not “unset”.Line 1157/Line 1165, Line 1219/Line 1228, and Line 1289/Line 1290 currently treat
0as absent. With packedRgb24,0is valid black, so black fg/bg gets dropped during normalize/merge/SGR emission.🔧 Proposed fix
- if (isRgb24(style.fg) && style.fg !== 0) { + if (isRgb24(style.fg)) { const fg = clampRgb24(style.fg); if (!isSameRgb(fg, CORE_DEFAULT_FG)) { normalized.fg = fg; } } - if (isRgb24(style.bg) && style.bg !== 0) { + if (isRgb24(style.bg)) { const bg = clampRgb24(style.bg); if (!isSameRgb(bg, CORE_DEFAULT_BG)) { normalized.bg = bg; } } - if (style.fg) { + if (style.fg !== undefined) { ... } - if (style.bg) { + if (style.bg !== undefined) { ... } const merged: CellStyle = {}; const bg = overlay.bg ?? base.bg; const fg = overlay.fg ?? base.fg; - if (bg) merged.bg = bg; - if (fg) merged.fg = fg; + if (bg !== undefined) merged.bg = bg; + if (fg !== undefined) merged.fg = fg; - if (cell.style?.bg) + if (cell.style?.bg !== undefined) entry["bg"] = `${rgbR(cell.style.bg)},${rgbG(cell.style.bg)},${rgbB(cell.style.bg)}`; - if (cell.style?.fg) + if (cell.style?.fg !== undefined) entry["fg"] = `${rgbR(cell.style.fg)},${rgbG(cell.style.fg)},${rgbB(cell.style.fg)}`;Also applies to: 1219-1230, 1287-1290, 900-903
packages/ink-compat/src/runtime/createInkRenderer.ts (1)
524-530:⚠️ Potential issue | 🟠 MajorSkipped frames are not traced in the fast path.
When
traceis configured, Line 526 returns before any trace event is emitted, so layout-skipped frames disappear from metrics/diagnostics.Suggested fix
if (!isFirstFrame && !forceLayout && !prevRoot.dirty && cachedLayoutTree !== null) { const totalMs = performance.now() - t0; + if (trace) { + const textStartedAt = performance.now(); + const screenText = opsToText(cachedOps, viewport); + const textMs = performance.now() - textStartedAt; + const opSummary = summarizeOps(cachedOps); + const nodeSummary = summarizeNodes(cachedNodes); + const textSummary = summarizeText(screenText); + trace({ + renderId, + viewport, + focusedId: null, + tick: 0, + timings: { commitMs, layoutMs: 0, drawMs: 0, textMs, totalMs }, + nodeCount: cachedNodes.length, + opCount: cachedOps.length, + opCounts: opSummary.opCounts, + clipDepthMax: opSummary.clipDepthMax, + textChars: textSummary.textChars, + textLines: textSummary.textLines, + nonBlankLines: textSummary.nonBlankLines, + widestLine: textSummary.widestLine, + minRectY: nodeSummary.minRectY, + maxRectBottom: nodeSummary.maxRectBottom, + zeroHeightRects: nodeSummary.zeroHeightRects, + detailIncluded: defaultTraceDetail, + layoutSkipped: true, + ...(defaultTraceDetail ? { nodes: cachedNodes, ops: cachedOps, text: screenText } : {}), + }); + } return { ops: cachedOps, nodes: cachedNodes,🤖 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 524 - 530, The fast-return path in createInkRenderer (the block checking isFirstFrame, forceLayout, prevRoot.dirty, cachedLayoutTree) exits before emitting any trace events, so frames where layout is skipped are missing from diagnostics; before returning from that branch, call the configured trace API (e.g., trace(...) or trace.event/traceFrame depending on your tracer) to record this frame with timings (totalMs, commitMs, layoutMs: 0, drawMs: 0) and layoutSkipped: true, using the same t0/performance.now() calculation and include any identifying metadata (ops/nodes or frame id) so skipped frames appear in traces. Ensure you guard the call with the existing trace variable (e.g., if (trace) ...) and do not change the returned payload (ops, nodes, timings).packages/core/src/runtime/commit.ts (1)
1366-1375:⚠️ Potential issue | 🟡 MinorType assertion on
detailis still incorrect.The assignment
detail: "children-changed" as "was-dirty" | undefinedcasts to a narrower type that doesn't include the actual value being assigned. The value"children-changed"is already a valid member of thedetailunion type and should be assigned directly.♻️ Proposed fix
__commitDiag.push({ id: instanceId as number, kind: vnode.kind, reason: "fast-reuse", - detail: "children-changed" as "was-dirty" | undefined, + detail: "children-changed", childDiffs, prevChildren: prevChildren.length, nextChildren: (nextChildren as readonly RuntimeInstance[]).length, });🤖 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 1366 - 1375, The diagnostic object pushed into __commitDiag uses an incorrect type assertion for the detail field; replace the cast '"children-changed" as "was-dirty" | undefined' with a direct assignment of the proper literal (detail: "children-changed") so the value matches the declared union type, i.e., update the object literal created in the __commitDiag.push call (referencing instanceId, vnode.kind, childDiffs, prevChildren, nextChildren) to assign detail: "children-changed" without the narrowing as-cast.
🧹 Nitpick comments (10)
packages/core/src/renderer/__tests__/renderer.scrollbar.test.ts (1)
508-508: UnnecessaryObject.freeze()on primitive number.The packed Rgb24 representation is correct:
(1 << 16) | (2 << 8) | 3produces0x010203. However,Object.freeze()on a primitive number is a no-op since numbers are already immutable—it just returns the number unchanged.🧹 Simplify by removing unnecessary freeze
- const customFg = Object.freeze((1 << 16) | (2 << 8) | 3); + const customFg = (1 << 16) | (2 << 8) | 3;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/renderer.scrollbar.test.ts` at line 508, Remove the unnecessary Object.freeze call on the primitive number by changing the declaration of customFg to a plain numeric assignment (replace const customFg = Object.freeze((1 << 16) | (2 << 8) | 3); with const customFg = (1 << 16) | (2 << 8) | 3;), keeping the packed Rgb24 expression intact and leaving the symbol customFg unchanged elsewhere.packages/core/src/layout/engine/layoutEngine.ts (1)
59-76: Profiling instrumentation looks good; consider tracking the cleanup.The
__layoutProfileexport is well-structured with a clean reset mechanism and default-disabled state that ensures no production overhead. The__prefix appropriately signals internal/temporary usage.Since the comment explicitly marks this as temporary ("remove after investigation"), consider opening an issue to track the removal once the performance investigation concludes—this helps prevent temporary instrumentation from becoming permanent technical debt.
Would you like me to open an issue to track the removal of this profiling instrumentation once the investigation is complete?
🤖 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 59 - 76, Create a tracker issue in the repo for removing this temporary profiling instrumentation (mention file packages/core/src/layout/engine/layoutEngine.ts and the exported symbol __layoutProfile) with a clear title and scope/acceptance criteria (why it was added, how to validate removal). After creating the issue, add a short TODO/FIXME comment directly above the __layoutProfile export that references the new issue number (e.g., "TODO: remove profiling instrumentation — see `#1234`") so future maintainers can find the task and reason quickly.packages/core/src/widgets/heatmap.ts (1)
64-93: Consider validating monotonic stop ordering inbuildScaleTable().
buildScaleTable()currently assumesstopsare sorted byt. A lightweight guard (dev-time assert or early throw) would prevent silent gradient corruption if future scales are added incorrectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/heatmap.ts` around lines 64 - 93, The buildScaleTable function assumes the input stops array is sorted by stop.t and within [0,1]; add a lightweight validation at the start of buildScaleTable that checks each stop.t is a finite number in [0,1] and that stops[i].t <= stops[i+1].t for all consecutive pairs (or throw/assert in dev builds). If validation fails, throw an informative error (or dev-only assertion) that references the offending index and values so callers can correct the stop ordering or bounds; keep the rest of buildScaleTable logic unchanged.packages/core/src/ui/recipes.ts (1)
40-42: Consider using hex literals for clarity in target color construction.The bit-shift operations are correct but verbose. Hex literals would be more readable for these well-known color constants.
✨ Suggested simplification
function adjustBrightness(color: Rgb24, amount: number): Rgb24 { - const target = amount > 0 ? (255 << 16) | (255 << 8) | 255 : (0 << 16) | (0 << 8) | 0; + const target = amount > 0 ? 0xffffff : 0x000000; return blendRgb(color, target, Math.abs(amount)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/ui/recipes.ts` around lines 40 - 42, In adjustBrightness, replace the verbose bit-shift construction of the target color with explicit hex literals for white and black (use 0xFFFFFF for white and 0x000000 for black) so the intent is clearer; update the ternary that currently builds target to return those hex constants while keeping the rest of the function (blendRgb, Math.abs(amount), and Rgb24 usage) unchanged.packages/core/src/renderer/__tests__/renderer.border.test.ts (1)
537-537: Minor:Object.freezeon primitive is unnecessary.
Object.freezehas no effect on primitive numbers. The packed Rgb24 value is already immutable by nature.Suggested simplification
- const pressedFg = Object.freeze((255 << 16) | (64 << 8) | 64); + const pressedFg = (255 << 16) | (64 << 8) | 64;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/renderer.border.test.ts` at line 537, The test defines pressedFg using Object.freeze on a primitive number which is unnecessary; change the declaration of pressedFg (the const pressedFg = Object.freeze((255 << 16) | (64 << 8) | 64)) to simply compute and assign the packed Rgb24 value without Object.freeze (e.g., const pressedFg = (255 << 16) | (64 << 8) | 64) so the value remains the same while removing the redundant freeze call in the renderer.border.test.ts test.packages/core/src/renderer/renderToDrawlist/widgets/renderCanvasWidgets.ts (1)
360-360: Redundant fallback:hashImageBytescannot return nullish.Per the code snippet from
packages/core/src/widgets/image.ts,hashImageBytesalways returnshash >>> 0, which is a number. The?? 0fallback afterhashImageBytes(analyzed.bytes)is defensive but unnecessary.Suggested simplification
- const imageId = readNonNegativeInt(props.imageId) ?? hashImageBytes(analyzed.bytes) ?? 0; + const imageId = readNonNegativeInt(props.imageId) ?? hashImageBytes(analyzed.bytes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/renderToDrawlist/widgets/renderCanvasWidgets.ts` at line 360, The current expression uses an unnecessary fallback "?? 0" because hashImageBytes(analyzed.bytes) always returns a non-nullish number; update the assignment for imageId to remove that redundant fallback by changing the line using readNonNegativeInt(props.imageId) and hashImageBytes(analyzed.bytes) only (e.g., const imageId = readNonNegativeInt(props.imageId) ?? hashImageBytes(analyzed.bytes);), referencing the imageId variable, readNonNegativeInt, hashImageBytes, and analyzed.bytes to locate and simplify the code.packages/core/src/drawlist/__tests__/builder_v6_resources.test.ts (1)
1-50: Rename file to remove v6 reference per protocol reset.The filename
builder_v6_resources.test.tsreferences the old v6 iteration. Based on learnings, after the protocol reset, public API references must consistently use the current public protocol version (v1) and must not reference v6.Consider renaming to
builder.resources.test.tsorbuilder_v1_resources.test.tsto align with the protocol reset naming convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/drawlist/__tests__/builder_v6_resources.test.ts` around lines 1 - 50, Rename the test file that references the old v6 protocol so it no longer mentions v6: move builder_v6_resources.test.ts to a new name such as builder.resources.test.ts (or builder_v1_resources.test.ts) and update any test suite globs or exports if needed so the tests using createDrawlistBuilder, expectOk, and the described tests continue to run under the new filename; ensure no remaining references to "v6" remain in the test file or related test configuration.packages/ink-compat/src/runtime/createInkRenderer.ts (1)
453-467: Remove temporary unused debug helper before merge.
countSelfDirtyis currently unused and explicitly marked temporary; keeping it here adds dead code in a hot-path 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 453 - 467, Remove the temporary debug helper function countSelfDirty from createInkRenderer.ts: delete the entire function definition "countSelfDirty" (including its declaration and body) and any imports/exports or references to it so there are no dangling identifiers; after removal, run typecheck/build/tests to ensure nothing else depended on countSelfDirty and fix any remaining references (e.g., replace with existing public API or inline minimal logic where actually required).packages/core/src/runtime/commit.ts (1)
1783-1795: Remove temporary debug tracing before merge.The comment explicitly marks this as temporary code to "remove after investigation." Debug instrumentation that reads from
globalThisshould not remain in production code.♻️ Proposed removal
- // 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"})`, - ); - } - } -🤖 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 1783 - 1795, Remove the temporary global debug tracing block that was added for investigation: delete the commitDebug declaration and the conditional that checks commitDebug.__commitDebug and pushes to commitDebug.__commitDebugLog inside commitNode (the code referencing commitDebug, __commitDebug, __commitDebugLog, instanceId, vnode, and prev). Ensure no other parts of commitNode depend on __commitDebug or __commitDebugLog; if any references remain, remove them too so commitNode contains only production logic.packages/core/src/widgets/__tests__/basicWidgets.render.test.ts (1)
555-565: Test name is now misleading and assertions are contradictory.The test "link encodes hyperlink refs on v3 and degrades on v1" no longer reflects reality since both
renderBytesV3andrenderBytesuse the unified builder. Consider either:
- Removing the v1/v3 differentiation test since versioning is unified, or
- Renaming and updating assertions to reflect the new unified behavior
♻️ Option: Update test to reflect unified builder behavior
- test("link encodes hyperlink refs on v3 and degrades on v1", () => { - const v3 = renderBytesV3(ui.link("https://example.com", "Docs"), { cols: 40, rows: 4 }); - const v1 = renderBytes(ui.link("https://example.com", "Docs"), { cols: 40, rows: 4 }); - assert.equal(parseOpcodes(v3).includes(8), false); - assert.equal(parseOpcodes(v1).includes(8), false); - assert.equal(parseInternedStrings(v3).includes("https://example.com"), true); - const v3Docs = parseDrawTextCommands(v3).find((cmd) => cmd.text === "Docs"); - const v1Docs = parseDrawTextCommands(v1).find((cmd) => cmd.text === "Docs"); - assert.equal((v3Docs?.linkUriRef ?? 0) > 0, true); - assert.equal(v1Docs?.linkUriRef ?? 0, 0); + test("link encodes hyperlink refs with unified builder", () => { + const bytes = renderBytes(ui.link("https://example.com", "Docs"), { cols: 40, rows: 4 }); + assert.equal(parseOpcodes(bytes).includes(8), false); + assert.equal(parseInternedStrings(bytes).includes("https://example.com"), true); + const docsCmd = parseDrawTextCommands(bytes).find((cmd) => cmd.text === "Docs"); + assert.equal((docsCmd?.linkUriRef ?? 0) > 0, true); });🤖 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 555 - 565, The test "link encodes hyperlink refs on v3 and degrades on v1" is outdated because renderBytesV3 and renderBytes now use a unified builder; rename the test (e.g., "link encodes hyperlink refs with unified builder") and update assertions to expect the unified behavior: both rendered outputs should intern the URL (parseInternedStrings(...).includes("https://example.com") === true) and both draw-text commands (from parseDrawTextCommands for each render) should have linkUriRef > 0 (replace the v1Docs zero-check). Keep or adjust the parseOpcodes assertions only if they remain meaningful for the unified builder.
🤖 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/src/profile-packed-style.ts`:
- Around line 85-87: Replace the dot-accesses on the index-signature `counters`
with bracket notation and add an ESLint suppression for the dot-notation rule;
e.g. change occurrences like `const merges = counters.style_merges_performed ??
0;` to `// eslint-disable-next-line `@typescript-eslint/dot-notation`` followed by
`const merges = counters['style_merges_performed'] ?? 0;` (and do the same for
`style_objects_created` -> `['style_objects_created']` and `packRgb_calls` ->
`['packRgb_calls']`); this keeps TypeScript happy while silencing the linter
conflict (alternatively, update the `perfSnapshot()`/`counters` type to include
explicit properties if you prefer not to suppress).
In `@packages/core/src/widgets/canvas.ts`:
- Around line 76-83: The current truthy check incorrectly treats numeric color 0
(black) as absent; update the condition around parsedHex in the color parsing
branch so it accepts numeric 0 (e.g., check parsedHex !== null / !== undefined
or use typeof parsedHex === "number"), then return the frozen pixel using
rgbR(parsedHex), rgbG(parsedHex), rgbB(parsedHex), clampU8(...), and a:255
instead of falling back to DEFAULT_SOLID_PIXEL; keep the rest of the logic
(functions rgbR, rgbG, rgbB, clampU8) unchanged.
In `@packages/core/src/widgets/heatmap.ts`:
- Around line 8-44: The palette anchors in heatmap.ts (objects viridis, plasma,
inferno, magma, turbo, grayscale) are using hardcoded rgb(...) literals; replace
those inline RGB values with centralized theme/design tokens (e.g., import the
project's palette/token registry and reference token names or exported color
constants) so the heatmap uses semantic theme colors; keep the same t positions
and Object.freeze wrappers but swap each rgb(...) entry to the corresponding
token reference (or a small mapping that resolves token -> Rgb24) and update any
use of the rgb function in this file to accept token-based values.
In `@packages/ink-compat/src/runtime/createInkRenderer.ts`:
- Around line 506-531: Early-skip ignores viewport changes so a changed
renderOpts.viewport can return stale cachedOps/cachedNodes; add tracking of the
current normalized viewport (e.g., a closure variable lastViewport or
cachedViewport) and compare it to the new viewport from
normalizeViewport(renderOpts.viewport ?? defaultViewport) before taking the
early-skip path in the block using
isFirstFrame/forceLayout/prevRoot.dirty/cachedLayoutTree; if the viewports
differ, treat as not-skippable (invalidate cachedOps/cachedNodes or force
layout), and after a successful commit update the saved cachedViewport to the
new normalized viewport so future frames can correctly early-skip when viewport
is unchanged.
In `@scripts/guardrails.sh`:
- Around line 77-78: The guardrail in scripts/guardrails.sh is using a regex
that misses the actual import and rejects single-quote styles; update the
ripgrep check (the -e pattern used to scan builder.ts) to a quote-agnostic,
properly shell-escaped regex that matches import statements that end with
./writers.gen.js regardless of single or double quotes and that allows typical
import forms (e.g., named/default/side-effect imports); replace the current
pattern string in the rg invocation with this new pattern and ensure it is
passed to rg with -e and appropriate quoting/escaping so the shell and ripgrep
interpret backslashes and quotes correctly.
---
Outside diff comments:
In `@packages/core/src/drawlist/__tests__/builder.round-trip.test.ts`:
- Around line 381-398: Rename the stale test titles that say "v2" or "version 2"
to reflect v1 so they match the assertions (which check ZR_DRAWLIST_VERSION_V1);
update the test descriptions in the test cases around the assertions and helper
calls (e.g., the test currently named "v2 header uses version 2 and correct cmd
byte/count totals") to mention v1/version 1, and do the same for the other
affected tests referenced (lines ~400-420, 422-463, 501-529) so test names
accurately reflect the verified constant ZR_DRAWLIST_VERSION_V1.
In `@packages/core/src/widgets/__tests__/basicWidgets.render.test.ts`:
- Around line 126-138: renderBytesV3 duplicates renderBytes but the ABI now uses
a single unified createDrawlistBuilder(), so having a V3-specific helper causes
tests to expect differing outputs that no longer exist; remove the renderBytesV3
function (and any references) or consolidate tests to use renderBytes only, and
update the assertions that expected v1/v3 differences (e.g., checks for
linkUriRef, underlineStyle, underlineColorRgb) so they match the unified builder
behavior; key symbols: renderBytes, renderBytesV3, createDrawlistBuilder, and
the failing assertions around linkUriRef/underlineStyle/underlineColorRgb.
---
Duplicate comments:
In `@packages/core/src/runtime/commit.ts`:
- Around line 1366-1375: The diagnostic object pushed into __commitDiag uses an
incorrect type assertion for the detail field; replace the cast
'"children-changed" as "was-dirty" | undefined' with a direct assignment of the
proper literal (detail: "children-changed") so the value matches the declared
union type, i.e., update the object literal created in the __commitDiag.push
call (referencing instanceId, vnode.kind, childDiffs, prevChildren,
nextChildren) to assign detail: "children-changed" without the narrowing
as-cast.
In `@packages/core/src/widgets/toast.ts`:
- Around line 172-177: TOAST_COLORS currently hardcodes RGB values (using
rgb(...)) which bypasses theming; replace these literals by mapping
Toast["type"] to semantic theme tokens/variants instead. Update the TOAST_COLORS
definition so it references the design-system theme tokens or a centralized
color-variant lookup (e.g., use the project's semantic token accessor or
badge/tag variant mapping) for keys info, success, warning, error, and remove
direct rgb(...) calls; ensure the map still returns Rgb24 values by converting
tokens via the existing theme-to-Rgb24 helper (or the same helper used elsewhere
in widgets) so toast rendering uses theme-driven colors.
In `@packages/ink-compat/src/runtime/createInkRenderer.ts`:
- Around line 524-530: The fast-return path in createInkRenderer (the block
checking isFirstFrame, forceLayout, prevRoot.dirty, cachedLayoutTree) exits
before emitting any trace events, so frames where layout is skipped are missing
from diagnostics; before returning from that branch, call the configured trace
API (e.g., trace(...) or trace.event/traceFrame depending on your tracer) to
record this frame with timings (totalMs, commitMs, layoutMs: 0, drawMs: 0) and
layoutSkipped: true, using the same t0/performance.now() calculation and include
any identifying metadata (ops/nodes or frame id) so skipped frames appear in
traces. Ensure you guard the call with the existing trace variable (e.g., if
(trace) ...) and do not change the returned payload (ops, nodes, timings).
---
Nitpick comments:
In `@packages/core/src/drawlist/__tests__/builder_v6_resources.test.ts`:
- Around line 1-50: Rename the test file that references the old v6 protocol so
it no longer mentions v6: move builder_v6_resources.test.ts to a new name such
as builder.resources.test.ts (or builder_v1_resources.test.ts) and update any
test suite globs or exports if needed so the tests using createDrawlistBuilder,
expectOk, and the described tests continue to run under the new filename; ensure
no remaining references to "v6" remain in the test file or related test
configuration.
In `@packages/core/src/layout/engine/layoutEngine.ts`:
- Around line 59-76: Create a tracker issue in the repo for removing this
temporary profiling instrumentation (mention file
packages/core/src/layout/engine/layoutEngine.ts and the exported symbol
__layoutProfile) with a clear title and scope/acceptance criteria (why it was
added, how to validate removal). After creating the issue, add a short
TODO/FIXME comment directly above the __layoutProfile export that references the
new issue number (e.g., "TODO: remove profiling instrumentation — see `#1234`") so
future maintainers can find the task and reason quickly.
In `@packages/core/src/renderer/__tests__/renderer.border.test.ts`:
- Line 537: The test defines pressedFg using Object.freeze on a primitive number
which is unnecessary; change the declaration of pressedFg (the const pressedFg =
Object.freeze((255 << 16) | (64 << 8) | 64)) to simply compute and assign the
packed Rgb24 value without Object.freeze (e.g., const pressedFg = (255 << 16) |
(64 << 8) | 64) so the value remains the same while removing the redundant
freeze call in the renderer.border.test.ts test.
In `@packages/core/src/renderer/__tests__/renderer.scrollbar.test.ts`:
- Line 508: Remove the unnecessary Object.freeze call on the primitive number by
changing the declaration of customFg to a plain numeric assignment (replace
const customFg = Object.freeze((1 << 16) | (2 << 8) | 3); with const customFg =
(1 << 16) | (2 << 8) | 3;), keeping the packed Rgb24 expression intact and
leaving the symbol customFg unchanged elsewhere.
In `@packages/core/src/renderer/renderToDrawlist/widgets/renderCanvasWidgets.ts`:
- Line 360: The current expression uses an unnecessary fallback "?? 0" because
hashImageBytes(analyzed.bytes) always returns a non-nullish number; update the
assignment for imageId to remove that redundant fallback by changing the line
using readNonNegativeInt(props.imageId) and hashImageBytes(analyzed.bytes) only
(e.g., const imageId = readNonNegativeInt(props.imageId) ??
hashImageBytes(analyzed.bytes);), referencing the imageId variable,
readNonNegativeInt, hashImageBytes, and analyzed.bytes to locate and simplify
the code.
In `@packages/core/src/runtime/commit.ts`:
- Around line 1783-1795: Remove the temporary global debug tracing block that
was added for investigation: delete the commitDebug declaration and the
conditional that checks commitDebug.__commitDebug and pushes to
commitDebug.__commitDebugLog inside commitNode (the code referencing
commitDebug, __commitDebug, __commitDebugLog, instanceId, vnode, and prev).
Ensure no other parts of commitNode depend on __commitDebug or __commitDebugLog;
if any references remain, remove them too so commitNode contains only production
logic.
In `@packages/core/src/ui/recipes.ts`:
- Around line 40-42: In adjustBrightness, replace the verbose bit-shift
construction of the target color with explicit hex literals for white and black
(use 0xFFFFFF for white and 0x000000 for black) so the intent is clearer; update
the ternary that currently builds target to return those hex constants while
keeping the rest of the function (blendRgb, Math.abs(amount), and Rgb24 usage)
unchanged.
In `@packages/core/src/widgets/__tests__/basicWidgets.render.test.ts`:
- Around line 555-565: The test "link encodes hyperlink refs on v3 and degrades
on v1" is outdated because renderBytesV3 and renderBytes now use a unified
builder; rename the test (e.g., "link encodes hyperlink refs with unified
builder") and update assertions to expect the unified behavior: both rendered
outputs should intern the URL
(parseInternedStrings(...).includes("https://example.com") === true) and both
draw-text commands (from parseDrawTextCommands for each render) should have
linkUriRef > 0 (replace the v1Docs zero-check). Keep or adjust the parseOpcodes
assertions only if they remain meaningful for the unified builder.
In `@packages/core/src/widgets/heatmap.ts`:
- Around line 64-93: The buildScaleTable function assumes the input stops array
is sorted by stop.t and within [0,1]; add a lightweight validation at the start
of buildScaleTable that checks each stop.t is a finite number in [0,1] and that
stops[i].t <= stops[i+1].t for all consecutive pairs (or throw/assert in dev
builds). If validation fails, throw an informative error (or dev-only assertion)
that references the offending index and values so callers can correct the stop
ordering or bounds; keep the rest of buildScaleTable logic unchanged.
In `@packages/ink-compat/src/runtime/createInkRenderer.ts`:
- Around line 453-467: Remove the temporary debug helper function countSelfDirty
from createInkRenderer.ts: delete the entire function definition
"countSelfDirty" (including its declaration and body) and any imports/exports or
references to it so there are no dangling identifiers; after removal, run
typecheck/build/tests to ensure nothing else depended on countSelfDirty and fix
any remaining references (e.g., replace with existing public API or inline
minimal logic where actually required).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (66)
packages/bench/src/profile-packed-style.tspackages/core/src/animation/__tests__/interpolate.easing.test.tspackages/core/src/animation/interpolate.tspackages/core/src/app/__tests__/partialDrawlistEmission.test.tspackages/core/src/drawlist/__tests__/builder.golden.test.tspackages/core/src/drawlist/__tests__/builder.graphics.test.tspackages/core/src/drawlist/__tests__/builder.round-trip.test.tspackages/core/src/drawlist/__tests__/builder.string-cache.test.tspackages/core/src/drawlist/__tests__/builder.string-intern.test.tspackages/core/src/drawlist/__tests__/builder.style-encoding.test.tspackages/core/src/drawlist/__tests__/builder.text-run.test.tspackages/core/src/drawlist/__tests__/builder_v6_resources.test.tspackages/core/src/drawlist/builder.tspackages/core/src/layout/__tests__/layout.edgecases.test.tspackages/core/src/layout/engine/layoutEngine.tspackages/core/src/renderer/__tests__/focusIndicators.test.tspackages/core/src/renderer/__tests__/recipeRendering.test-utils.tspackages/core/src/renderer/__tests__/renderer.border.test.tspackages/core/src/renderer/__tests__/renderer.damage.test.tspackages/core/src/renderer/__tests__/renderer.scrollbar.test.tspackages/core/src/renderer/__tests__/tableRecipeRendering.test.tspackages/core/src/renderer/__tests__/textStyle.opacity.test.tspackages/core/src/renderer/renderToDrawlist/textStyle.tspackages/core/src/renderer/renderToDrawlist/widgets/editors.tspackages/core/src/renderer/renderToDrawlist/widgets/renderCanvasWidgets.tspackages/core/src/renderer/shadow.tspackages/core/src/renderer/styles.tspackages/core/src/runtime/__tests__/commit.fastReuse.regression.test.tspackages/core/src/runtime/__tests__/hooks.useTheme.test.tspackages/core/src/runtime/commit.tspackages/core/src/theme/__tests__/theme.contrast.test.tspackages/core/src/theme/__tests__/theme.extend.test.tspackages/core/src/theme/__tests__/theme.interop.test.tspackages/core/src/theme/__tests__/theme.resolution.test.tspackages/core/src/theme/__tests__/theme.switch.test.tspackages/core/src/theme/__tests__/theme.test.tspackages/core/src/theme/__tests__/theme.transition.test.tspackages/core/src/theme/blend.tspackages/core/src/theme/contrast.tspackages/core/src/theme/defaultTheme.tspackages/core/src/theme/tokens.tspackages/core/src/ui/__tests__/themed.test.tspackages/core/src/ui/recipes.tspackages/core/src/widgets/__tests__/basicWidgets.render.test.tspackages/core/src/widgets/__tests__/canvas.primitives.test.tspackages/core/src/widgets/__tests__/collections.test.tspackages/core/src/widgets/__tests__/containers.test.tspackages/core/src/widgets/__tests__/graphics.golden.test.tspackages/core/src/widgets/__tests__/overlays.test.tspackages/core/src/widgets/__tests__/overlays.typecheck.tspackages/core/src/widgets/__tests__/renderer.regressions.test.tspackages/core/src/widgets/__tests__/style.attributes.test.tspackages/core/src/widgets/__tests__/style.inheritance.test.tspackages/core/src/widgets/__tests__/style.merge-fuzz.test.tspackages/core/src/widgets/__tests__/style.merge.test.tspackages/core/src/widgets/__tests__/style.utils.test.tspackages/core/src/widgets/__tests__/styleUtils.test.tspackages/core/src/widgets/__tests__/styled.test.tspackages/core/src/widgets/__tests__/table.typecheck.tspackages/core/src/widgets/canvas.tspackages/core/src/widgets/heatmap.tspackages/core/src/widgets/logsConsole.tspackages/core/src/widgets/toast.tspackages/ink-compat/src/runtime/createInkRenderer.tspackages/ink-compat/src/runtime/render.tsscripts/guardrails.sh
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/layout/tests/layout.edgecases.test.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- packages/core/src/widgets/tests/styled.test.ts
- packages/core/src/renderer/tests/textStyle.opacity.test.ts
- packages/core/src/widgets/tests/overlays.test.ts
- packages/core/src/theme/tests/theme.transition.test.ts
- packages/core/src/widgets/logsConsole.ts
- packages/core/src/drawlist/tests/builder.string-cache.test.ts
- packages/core/src/widgets/tests/styleUtils.test.ts
- packages/core/src/theme/tests/theme.contrast.test.ts
- packages/core/src/runtime/tests/hooks.useTheme.test.ts
- packages/core/src/widgets/tests/style.merge-fuzz.test.ts
- packages/core/src/drawlist/tests/builder.graphics.test.ts
- packages/core/src/animation/tests/interpolate.easing.test.ts
- packages/core/src/widgets/tests/style.merge.test.ts
- packages/core/src/widgets/tests/renderer.regressions.test.ts
- packages/core/src/renderer/tests/focusIndicators.test.ts
- packages/core/src/drawlist/tests/builder.golden.test.ts
- packages/core/src/widgets/tests/table.typecheck.ts
- packages/core/src/theme/tests/theme.resolution.test.ts
- packages/core/src/drawlist/tests/builder.text-run.test.ts
- packages/core/src/theme/tests/theme.test.ts
- packages/core/src/renderer/tests/tableRecipeRendering.test.ts
- packages/core/src/widgets/tests/containers.test.ts
- packages/core/src/renderer/shadow.ts
- packages/core/src/runtime/tests/commit.fastReuse.regression.test.ts
- packages/core/src/widgets/tests/overlays.typecheck.ts
Summary
EPIC 6 is implemented in this branch with two outcomes:
origin/mainfrom/home/k3nig/Zireael.A) Zireael Vendor Bump
packages/native/vendor/zireael/includepackages/native/vendor/zireael/srcpackages/native/vendor/VENDOR_COMMIT.txtnow points to:52f3fd81a437e5a351dd026a75b2f9b01ca44a76requested_drawlist_version,negotiated_drawlist_version), so these were retained where mapped.B) Packed Style Redesign
Rgbobject style representation removed from exported API usage in the render pipeline.Rgb24values are used through style APIs and renderer paths.rgb(...)now returns packed numbers and all touched call sites were updated accordingly.attrsbitmask.Additional Cleanup
{r,g,b}to packed values.Rgbobject channels.Tests / Validation
Pass:
npm run typechecknpm -w @rezi-ui/native run build:nativenpm -w @rezi-ui/native run test:native:smokenpm -w @rezi-ui/core run buildnode --test packages/core/dist/drawlist/__tests__/builder.style-encoding.test.jsnpm -w @rezi-ui/ink-compat run buildnpx tsc -p packages/bench/tsconfig.json --pretty falseKnown in this branch:
npm -w @rezi-ui/core run testcurrently reports broader existing failures outside this epic-focused validation set.Perf Proof
Added counters and profiler coverage for packed-style merge behavior:
style_merges_performed,style_objects_created,packRgb_callspackages/bench/src/profile-packed-style.tsObserved run (
REZI_PERF=1 REZI_PERF_DETAIL=1 npx tsx src/profile-packed-style.ts):style_merges_performed:192000style_objects_created:44800packRgb_calls:076.67%render avg:6115µsdrawlist_build avg:11µsThese numbers demonstrate removal of pack calls from hot style merge/render paths and reduced style object churn.
Summary by CodeRabbit