feat: harden semantic theming and design system#266
Merged
RtlZeroMemory merged 10 commits intomainfrom Mar 6, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR delivers a comprehensive refactor of the theming architecture, widget composition model, and focus management system. It transitions from legacy Theme-based APIs to semantic ThemeDefinition with Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant compileTheme as compileTheme()
participant Theme as Theme (compiled)
participant Widget as Widget Renderer
participant FocusIndicator as focusIndicator Config
participant ColorTokens as ColorTokens
App->>compileTheme: createThemeDefinition(name, colors, options)
compileTheme->>Theme: compile & validate definition
Theme->>Theme: build color index, spacing, focusIndicator
compileTheme-->>App: return Theme
App->>Widget: render with theme
Widget->>ColorTokens: getColorTokens(theme)
ColorTokens-->>Widget: return theme.definition.colors
Widget->>FocusIndicator: check theme.focusIndicator
FocusIndicator->>Widget: bold, underline, focusRingColor
Widget->>Widget: build focused style with flags
Widget-->>App: emit draw operations
sequenceDiagram
participant User
participant FocusManager as Focus Manager
participant TrapStack as Trap Stack
participant Widget as Widget Meta
participant FinalizeLogic as finalizeFocus()
User->>FocusManager: trigger focus event
FocusManager->>TrapStack: retrieve active trap
TrapStack->>Widget: get trap focusables & initialFocus
Widget-->>TrapStack: return trap metadata (kind, focusables)
alt Trap Active & No Focusables
TrapStack->>FinalizeLogic: set focus to initialFocus or null
else Trap Active & Has Focusables
TrapStack->>FinalizeLogic: if focus outside trap, reassign to initialFocus or first focusable
else No Trap
TrapStack->>FinalizeLogic: retain existing focus logic
end
FinalizeLogic-->>User: focus reassigned
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes This PR involves substantial heterogeneous changes across the theme system (API removal/refactoring), widget composition (breaking changes to WidgetContext and wrapper defaults), focus management (new trap-aware logic), animation subsystem (frame driver sharing), and extensive documentation updates. The changes span many files with different types of modifications (removals, additions, refactors, type changes), requiring separate reasoning for each cohort. While individual changes within a cohort are often repetitive, the overall scope and variety of affected subsystems elevates this to Complex. Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ink-compat/src/runtime/createInkRenderer.ts (1)
27-33:⚠️ Potential issue | 🟠 MajorUpdate
InkRendererOptions.themeto useThemeDefinitioninstead ofThemeto align with the PR migration objective.The import path
@rezi-ui/core/pipelineis valid and publicly exported. However,InkRendererOptions.themeis typed asTheme(which wrapsThemeDefinition) rather thanThemeDefinitiondirectly. Per the PR objective of migrating toThemeDefinitionas the primary contract, this should be:- theme?: Theme; + theme?: ThemeDefinition;This requires importing
ThemeDefinitionfrom the main@rezi-ui/coreexport:+ import { type ThemeDefinition } from "@rezi-ui/core";The
Themetype exported frompipelineis a wrapper containingdefinition,colors,spacing, andfocusIndicatorfields, whereasThemeDefinitionis the canonical minimal type for theme configuration.🤖 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 27 - 33, The InkRendererOptions.theme is currently typed as Theme (from `@rezi-ui/core/pipeline`) but should use the canonical ThemeDefinition; change the InkRendererOptions interface to use ThemeDefinition for the theme property, add an import for ThemeDefinition from the main `@rezi-ui/core` export, and remove or stop using the wrapped Theme type from `@rezi-ui/core/pipeline` (leave other pipeline imports unchanged); update any references to InkRendererOptions.theme to match the new ThemeDefinition shape.
🧹 Nitpick comments (10)
packages/core/src/widgets/__tests__/graphics.golden.test.ts (1)
325-326: Consider usingpackRgbhere as well for consistency.Line 325 still uses raw bit manipulation
(0 << 16) | (170 << 8) | 255while line 317 now uses thepackRgbhelper. UsingpackRgb(0, 170, 255)would improve readability and maintain consistency within the test.♻️ Suggested consistency fix
text: "warn", style: { underlineStyle: "dashed", - underlineColor: (0 << 16) | (170 << 8) | 255, + underlineColor: packRgb(0, 170, 255), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/__tests__/graphics.golden.test.ts` around lines 325 - 326, Replace the raw bit-manipulation used for underlineColor with the packRgb helper for consistency and readability: change the value assigned to underlineColor (currently `(0 << 16) | (170 << 8) | 255`) to use packRgb(0, 170, 255) so it matches the other color usage (e.g., the earlier packRgb call) in the test file.packages/core/src/renderer/renderToDrawlist/widgets/editors.ts (1)
68-78: Consider unifying color resolution helpers.
resolveDiffThemeColorandresolveSyntaxThemeColorhave identical implementations. A singleresolveThemeColorhelper would reduce duplication while remaining equally readable.♻️ Optional consolidation
-function resolveSyntaxThemeColor(theme: Theme, key: string, fallback: Rgb24) { - return theme.colors[key] ?? fallback; -} - -function resolveDiffThemeColor( - theme: Theme, - key: string, - fallback: Rgb24, -): Rgb24 { - return theme.colors[key] ?? fallback; -} +function resolveThemeColor(theme: Theme, key: string, fallback: Rgb24): Rgb24 { + return theme.colors[key] ?? fallback; +}Then update call sites to use
resolveThemeColor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/renderToDrawlist/widgets/editors.ts` around lines 68 - 78, Both resolveSyntaxThemeColor and resolveDiffThemeColor are identical; replace them with a single helper (e.g., resolveThemeColor) that takes (theme: Theme, key: string, fallback: Rgb24) and returns theme.colors[key] ?? fallback, then update all call sites that reference resolveSyntaxThemeColor or resolveDiffThemeColor to use resolveThemeColor instead to remove duplication while preserving behavior.packages/core/src/widgets/__tests__/composition.useAnimatedValue.test.ts (1)
213-265: Stabilize pause snapshot to avoid timing-race flakiness.On Line 240,
pausedValueis captured before Line 241 flushes effects. A last in-flight tick can slip in and make the assertion at Line 251 intermittently fail.Proposed deterministic assertion update
- pausedValue = render.result.value; - h.runPending(render.pendingEffects); - - await sleep(80); - render = h.render((hooks) => + h.runPending(render.pendingEffects); + render = h.render((hooks) => useAnimatedValue(hooks, 10, { mode: "transition", transition: { duration: 120, easing: "linear", playback: { paused: true } }, }), ); - h.runPending(render.pendingEffects); - assert.ok(Math.abs(render.result.value - pausedValue) <= 0.1); - assert.equal(render.result.isAnimating, false); + pausedValue = render.result.value; + h.runPending(render.pendingEffects); + + await sleep(80); + const pausedAfterSleep = h.render((hooks) => + useAnimatedValue(hooks, 10, { + mode: "transition", + transition: { duration: 120, easing: "linear", playback: { paused: true } }, + }), + ); + h.runPending(pausedAfterSleep.pendingEffects); + assert.ok(Math.abs(pausedAfterSleep.result.value - pausedValue) <= 0.1); + assert.equal(pausedAfterSleep.result.isAnimating, false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/__tests__/composition.useAnimatedValue.test.ts` around lines 213 - 265, The pause snapshot is taken before flushing pending effects causing a race; after calling useAnimatedValue and rendering the paused playback state, call h.runPending(render.pendingEffects) first and then read pausedValue = render.result.value (i.e., move the pausedValue capture to after the h.runPending call for the paused render) so the in-flight tick is flushed before asserting the paused value and isAnimating state.packages/core/src/renderer/__tests__/selectRecipeRendering.test.ts (1)
62-81: Broaden this regression to the full built-in theme matrix.Line 62 currently validates semantic select styling only on
defaultTheme(effectively the dark compiled path). Please parameterize this case across Dark, Light, Dimmed, High-contrast, Nord, and Dracula to catch token-mapping regressions that appear only in specific presets.As per coding guidelines "Validate screens in all built-in themes (Dark, Light, Dimmed, High-contrast, Nord, Dracula) without code changes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/selectRecipeRendering.test.ts` around lines 62 - 81, The test "default theme keeps semantic select recipe styling enabled" only checks defaultTheme; change it to iterate over the built-in theme matrix (Dark, Light, Dimmed, High-contrast, Nord, Dracula) and run the same assertions for each theme. Update the test to call renderOps(...) with each theme from that list (use the same ui.select({ id: "default", value: "a", options }) and options variable) and assert the presence of the fillRect with style.bg matching theme.colors["bg.elevated"], plus the selected text and caret drawText checks; you can implement this with a parameterized test (e.g., test.each or a for/of inside the test) while keeping renderOps, ui.select, and the existing assertions unchanged.packages/core/src/theme/__tests__/theme.validation.test.ts (1)
56-63: Consider adding a whitespace-onlynamenegative test.You already assert empty-string rejection; adding
" "would harden this path against trim-related regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/theme/__tests__/theme.validation.test.ts` around lines 56 - 63, Add a new negative test alongside the existing "rejects invalid name" test that sets the cloned theme's name to a whitespace-only string (e.g. theme["name"] = " ") and asserts the same validation failure using expectValidationError; locate where cloneDarkTheme() and expectValidationError(...) are used in the existing test and mirror that pattern to ensure whitespace-only names are rejected (use the same expected message as the empty-string case).packages/core/src/theme/__tests__/theme.transition.test.ts (1)
63-81: Optional dedup: reusethemeWithPrimaryinsemanticThemeWithAccent.Both helpers currently build the same shape; consolidating reduces drift risk in future token-path changes.
♻️ Proposed simplification
function semanticThemeWithAccent(r: number, g: number, b: number) { - return extendTheme(darkTheme, { - colors: { - accent: { - primary: (r << 16) | (g << 8) | b, - }, - }, - }); + return themeWithPrimary(r, g, b); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/theme/__tests__/theme.transition.test.ts` around lines 63 - 81, semanticThemeWithAccent duplicates themeWithPrimary by returning the same extendTheme(darkTheme, { colors: { accent: { primary: ... } } }); replace the duplicate by calling themeWithPrimary from semanticThemeWithAccent (i.e., have semanticThemeWithAccent forward its r,g,b args to themeWithPrimary) so both helpers share the same implementation; update any tests that expect distinct identities if necessary and keep references to extendTheme and darkTheme only in themeWithPrimary.packages/core/src/renderer/__tests__/tableRecipeRendering.test.ts (1)
86-115: Consider adding one additional viewport-size variant for this test case.A compact-width assertion here would better protect table recipe behavior under responsive constraints.
Based on learnings: "Applies to **/*.test.{ts,tsx,js,jsx} : Test responsive behavior by validating UI at multiple viewport sizes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/__tests__/tableRecipeRendering.test.ts` around lines 86 - 115, Add a second viewport-size variant to the "default theme keeps semantic table recipe styling enabled" test by invoking renderOps a second time with ui.table(...) (same props: id "default-table", columns, data, getRowKey, stripedRows, sortColumn "name", sortDirection "asc", border "none") but with a compact viewport (e.g., smaller cols like 30 and appropriate rows) and defaultTheme; then repeat the same assertions (use firstDrawText to find the sorted header and assert style fg equals defaultTheme.colors["fg.secondary"], bold and underline true, and assert striped row fillRect uses defaultTheme.colors["bg.subtle") so the test verifies behavior at both default and compact widths.packages/core/src/renderer/renderToDrawlist/widgets/overlays.ts (1)
309-323: Please add a focused regression test for the new dropdown width pre-pass.This branch now depends on max label/shortcut measurements; a narrow-viewport case with mixed shortcut widths would help lock behavior.
Based on learnings: "Risk triage: docs-only updates (low risk) need lint/grep validation; widget prop renames (medium risk) need compile + affected tests + docs parity; runtime/router/reconcile/layout/drawlist changes (high risk) need full test suite + integration coverage + PTY evidence".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/renderer/renderToDrawlist/widgets/overlays.ts` around lines 309 - 323, Add a focused regression test that covers the new dropdown width pre-pass which uses maxLabelW/maxShortcutW measured via measureTextCells(readString(...)); create a narrow-viewport scenario with menu items that include mixed-length labels and varying shortcut strings (including empty shortcuts and wide shortcuts) and render through the same path that triggers the logic in overlays.ts (the item measurement loop and subsequent contentPx calculation). In the test, assert the computed/final menu content width or pixel placement (the outcome of contentPx / widestItemW) matches the expected centered/clamped value for that viewport (e.g., ensure the shortcut slot is reserved when maxShortcutW>0 and that overall width clamps to itemPx), and run it through the renderer used by renderToDrawlist so this regression is caught by CI.packages/core/src/theme/__tests__/theme.interop.test.ts (1)
84-84: Consider renaming this test for semantic clarity.The title says “preserves unrelated parent tokens,” but the assertion checks override/default spacing, not parent spacing preservation.
✏️ Optional rename
- test("mergeThemeOverride preserves unrelated parent tokens", () => { + test("full ThemeDefinition override uses override/default spacing (not parent spacing)", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/theme/__tests__/theme.interop.test.ts` at line 84, Rename the test currently declared as test("mergeThemeOverride preserves unrelated parent tokens", ...) to a title that accurately describes what it asserts—e.g., test("mergeThemeOverride prefers override spacing values over defaults")—so the name reflects that it checks override/default spacing behavior; update the test name string where mergeThemeOverride is exercised to keep semantics clear.docs/design-system.md (1)
57-62: Consider varying sentence structure for readability.Three consecutive bullet points begin with "
recipe.*exists, but..." which reads repetitively. Consider rephrasing for variety.📝 Suggested rephrasing
-Important distinctions: - -- `recipe.surface(...)` exists, but there is no standalone `ui.surface(...)`. -- `recipe.text(...)` exists, but plain `ui.text(...)` is not globally recipe-driven. -- `recipe.scrollbar(...)` exists, but overflow scrollbars are not universally - rendered through that recipe path yet. +Important distinctions: + +- `recipe.surface(...)` exists, but there is no standalone `ui.surface(...)`. +- Plain `ui.text(...)` is not globally recipe-driven, though `recipe.text(...)` is available. +- Overflow scrollbars are not universally rendered through `recipe.scrollbar(...)` yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design-system.md` around lines 57 - 62, The bullet list in docs/design-system.md is repetitive — three bullets start with "recipe.* exists, but..."; edit the bullets (referencing recipe.surface, ui.surface, recipe.text, ui.text, recipe.scrollbar, ui.divider) to vary sentence openings and cadence (e.g., start one with "There is a recipe.surface, however ui.surface is not available", another with "While recipe.text is provided, ui.text is not globally recipe-driven", and rephrase the scrollbar/divider lines similarly) so readability improves while preserving the original meanings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guide/recommended-patterns.md`:
- Around line 627-630: The snippet hard-codes darkTheme.spacing which prevents
runtime theme switching; update the recipe.button call to use the provided
tokens' spacing instead (e.g., replace darkTheme.spacing with tokens.spacing) so
recipe.button(tokens, { size: "lg", spacing: tokens.spacing }) uses the active
theme spacing at runtime.
In `@docs/guide/styling.md`:
- Around line 14-21: The example uses hardcoded RGB literals in widget styles
(rgb(...) inside ui.text and ui.box); replace those with semantic theme tokens
instead (e.g., use your theme token name such as theme.colors.warning,
tokens.palette.warning, or ui.tokens.background/foreground) so examples
demonstrate token-driven styling and respect overrides; update the ui.text
call's style.fg and the ui.box style.bg to reference semantic token identifiers
rather than rgb(...) while keeping bold and border settings unchanged.
In `@docs/styling/index.md`:
- Around line 17-20: Update the “Beautiful Defaults” widget list to include
ui.textarea: find the paragraph describing recipe-styled widgets (mentions
ui.button, ui.input, ui.select, ui.checkbox, ui.progress, ui.callout) and add
ui.textarea to that list so it reads something like ui.button, ui.input,
ui.textarea, ui.select, ui.checkbox, ui.progress, ui.callout; ensure the wording
still notes that recipes apply when the active theme provides semantic color
tokens.
In `@packages/core/src/app/createApp.ts`:
- Line 59: The import of describeThrown from "../debug/describeThrown.js" in
createApp.ts is referencing a non-existent module; remove that import and either
replace usages of describeThrown in createApp (if any) with the existing
error-serialization helper used elsewhere (e.g., describeError/describeThrown
from the correct path) or add the missing module; specifically, locate the
import line for describeThrown and either (a) change it to the correct module
path used in other files (match the implementation used in commit.ts fixes) or
(b) delete the import and refactor any calls to use the existing error helper
function that actually exists in the codebase.
- Around line 734-749: registerAppBindings is passing an unsupported option
(sourceTag) into registerBindings: change registerAppBindings to only pass
supported RegisterBindingsOptions (e.g., mode) or add sourceTag to the
registerBindings API; specifically, update the registerAppBindings
implementation around registerBindings(keybindingState, bindings, { ... }) to
stop spreading options?.sourceTag (remove the sourceTag branch) or alternatively
extend the RegisterBindingsOptions/type and registerBindings signature to accept
sourceTag and propagate it through registerBindings, keeping references to
registerAppBindings, registerBindings, keybindingState, and
RegisterBindingsOptions to locate the change.
In `@packages/core/src/keybindings/__tests__/keybinding.conflicts.test.ts`:
- Around line 615-617: The test currently calls pushEvents(...) then only
flushMicrotasks(20), which doesn't explicitly release the backend frame gate and
can cause flaky races; update the test to explicitly release the backend frame
gate the same way neighboring tests do (i.e., after await pushEvents(backend,
[...]) call the helper used elsewhere to release the frame gate such as await
releaseFrameGate(backend) or await backend.releaseFrameGate()) so the first
committed frame is guaranteed before continuing, leaving the pushEvents and
flushMicrotasks usage intact otherwise.
In `@packages/core/src/renderer/renderToDrawlist/renderPackets.ts`:
- Around line 148-151: The hash currently collapses unset versus explicit zero
for focusRingColor because code uses "?? 0" when computing the value sent to
mixHash; change this so hashTheme (in renderPackets.ts) distinguishes undefined
from the explicit sentinel 0 by mapping undefined to a distinct sentinel value
(e.g., -1 or 0xFFFFFFFF) instead of 0 before truncation and masking. Update the
expression that feeds mixHash (the Math.trunc(... & HASH_MASK_32) >>> 0 path) to
first check theme.focusIndicator.focusRingColor === undefined and use a unique
UNSET_FOCUS_RING_COLOR constant for that case, otherwise use the numeric color,
preserving use of HASH_MASK_32 and mixHash.
In `@packages/core/src/renderer/renderToDrawlist/widgets/focusConfig.ts`:
- Around line 61-67: The focus ring FG is never applied because the code tests
out.fg (a ResolvedTextStyle that always has fg) instead of whether the caller
provided an explicit fg; change the condition used when merging the focus ring
color in the mergeTextStyle call so it checks the original/unresolved input (the
variable that represented the incoming text style or an explicit-flag indicating
an override) rather than out.fg, e.g., use the pre-resolved style.fg or a
hadExplicitFg boolean to decide whether to set fg to
theme.focusIndicator.focusRingColor ?? colorTokens.focus.ring inside the merge;
update the mergeTextStyle call around out to use that corrected condition.
In `@packages/core/src/runtime/commit.ts`:
- Line 21: The import of describeThrown is invalid; remove the line importing
describeThrown and either define a local helper or eliminate its usages in this
file: delete the import statement, add a small local function
describeThrown(error) that returns a readable string (copy the implementation
pattern used in other files) and replace all calls to describeThrown(...) in
commit.ts (e.g., inside functions that catch errors during commit operations) to
use the new local helper, or alternately remove those calls and inline
equivalent error-string logic so the file no longer depends on the non-existent
module.
In `@packages/core/src/theme/theme.ts`:
- Around line 66-88: The compiled color key map in buildColorIndex is using keys
that don't match the public ColorPath names used by resolveColor (e.g. change
"widget.diff.addBg" / "widget.diff.deleteBg" etc. to the dot-separated contract
names like "widget.diff.add.bg" and switch any
"widget.logs.trace"/"widget.logs.level.trace" variants to the exact ColorPath
strings); update the keys in the object created by buildColorIndex to exactly
match the ColorPath identifiers used in resolveColor (also apply the same
key-name fixes for the other occurrences noted around the second block mentioned
— lines ~236-239) so resolveColor finds semantic paths instead of falling back
to defaults.
In `@packages/core/src/widgets/__tests__/composition.animationPlayback.test.ts`:
- Around line 398-406: The test currently only asserts eventual completion which
allows a non-restarting linear playback to pass; modify the assertion around
useSequence/snapFrames to prove a restart-on-easing-change by first triggering
the easing change, then asserting the sequence's rendered value (next.result
from h.render/useSequence) drops to a small value (e.g., <1 or near 0) shortly
after the change (showing a restart) and then subsequently progresses to the
expected end (>=9.5); use h.runPending(next.pendingEffects) between steps to
flush effects so you can observe the immediate reset and subsequent progression.
In `@packages/core/src/widgets/hooks/data.ts`:
- Around line 262-267: The clamp currently only forces values <= 0 to
MIN_STREAM_RECONNECT_MS in normalizeReconnectDelayMs, allowing small positive
delays (1–9ms) to slip through; update normalizeReconnectDelayMs to use
normalizeNonNegativeInteger(value, fallback) and return normalized <
MIN_STREAM_RECONNECT_MS ? MIN_STREAM_RECONNECT_MS : normalized so any value
below MIN_STREAM_RECONNECT_MS is clamped; this will ensure callers like
useEventSource and useWebSocket respect the minimum reconnect delay.
---
Outside diff comments:
In `@packages/ink-compat/src/runtime/createInkRenderer.ts`:
- Around line 27-33: The InkRendererOptions.theme is currently typed as Theme
(from `@rezi-ui/core/pipeline`) but should use the canonical ThemeDefinition;
change the InkRendererOptions interface to use ThemeDefinition for the theme
property, add an import for ThemeDefinition from the main `@rezi-ui/core` export,
and remove or stop using the wrapped Theme type from `@rezi-ui/core/pipeline`
(leave other pipeline imports unchanged); update any references to
InkRendererOptions.theme to match the new ThemeDefinition shape.
---
Nitpick comments:
In `@docs/design-system.md`:
- Around line 57-62: The bullet list in docs/design-system.md is repetitive —
three bullets start with "recipe.* exists, but..."; edit the bullets
(referencing recipe.surface, ui.surface, recipe.text, ui.text, recipe.scrollbar,
ui.divider) to vary sentence openings and cadence (e.g., start one with "There
is a recipe.surface, however ui.surface is not available", another with "While
recipe.text is provided, ui.text is not globally recipe-driven", and rephrase
the scrollbar/divider lines similarly) so readability improves while preserving
the original meanings.
In `@packages/core/src/renderer/__tests__/selectRecipeRendering.test.ts`:
- Around line 62-81: The test "default theme keeps semantic select recipe
styling enabled" only checks defaultTheme; change it to iterate over the
built-in theme matrix (Dark, Light, Dimmed, High-contrast, Nord, Dracula) and
run the same assertions for each theme. Update the test to call renderOps(...)
with each theme from that list (use the same ui.select({ id: "default", value:
"a", options }) and options variable) and assert the presence of the fillRect
with style.bg matching theme.colors["bg.elevated"], plus the selected text and
caret drawText checks; you can implement this with a parameterized test (e.g.,
test.each or a for/of inside the test) while keeping renderOps, ui.select, and
the existing assertions unchanged.
In `@packages/core/src/renderer/__tests__/tableRecipeRendering.test.ts`:
- Around line 86-115: Add a second viewport-size variant to the "default theme
keeps semantic table recipe styling enabled" test by invoking renderOps a second
time with ui.table(...) (same props: id "default-table", columns, data,
getRowKey, stripedRows, sortColumn "name", sortDirection "asc", border "none")
but with a compact viewport (e.g., smaller cols like 30 and appropriate rows)
and defaultTheme; then repeat the same assertions (use firstDrawText to find the
sorted header and assert style fg equals defaultTheme.colors["fg.secondary"],
bold and underline true, and assert striped row fillRect uses
defaultTheme.colors["bg.subtle") so the test verifies behavior at both default
and compact widths.
In `@packages/core/src/renderer/renderToDrawlist/widgets/editors.ts`:
- Around line 68-78: Both resolveSyntaxThemeColor and resolveDiffThemeColor are
identical; replace them with a single helper (e.g., resolveThemeColor) that
takes (theme: Theme, key: string, fallback: Rgb24) and returns theme.colors[key]
?? fallback, then update all call sites that reference resolveSyntaxThemeColor
or resolveDiffThemeColor to use resolveThemeColor instead to remove duplication
while preserving behavior.
In `@packages/core/src/renderer/renderToDrawlist/widgets/overlays.ts`:
- Around line 309-323: Add a focused regression test that covers the new
dropdown width pre-pass which uses maxLabelW/maxShortcutW measured via
measureTextCells(readString(...)); create a narrow-viewport scenario with menu
items that include mixed-length labels and varying shortcut strings (including
empty shortcuts and wide shortcuts) and render through the same path that
triggers the logic in overlays.ts (the item measurement loop and subsequent
contentPx calculation). In the test, assert the computed/final menu content
width or pixel placement (the outcome of contentPx / widestItemW) matches the
expected centered/clamped value for that viewport (e.g., ensure the shortcut
slot is reserved when maxShortcutW>0 and that overall width clamps to itemPx),
and run it through the renderer used by renderToDrawlist so this regression is
caught by CI.
In `@packages/core/src/theme/__tests__/theme.interop.test.ts`:
- Line 84: Rename the test currently declared as test("mergeThemeOverride
preserves unrelated parent tokens", ...) to a title that accurately describes
what it asserts—e.g., test("mergeThemeOverride prefers override spacing values
over defaults")—so the name reflects that it checks override/default spacing
behavior; update the test name string where mergeThemeOverride is exercised to
keep semantics clear.
In `@packages/core/src/theme/__tests__/theme.transition.test.ts`:
- Around line 63-81: semanticThemeWithAccent duplicates themeWithPrimary by
returning the same extendTheme(darkTheme, { colors: { accent: { primary: ... } }
}); replace the duplicate by calling themeWithPrimary from
semanticThemeWithAccent (i.e., have semanticThemeWithAccent forward its r,g,b
args to themeWithPrimary) so both helpers share the same implementation; update
any tests that expect distinct identities if necessary and keep references to
extendTheme and darkTheme only in themeWithPrimary.
In `@packages/core/src/theme/__tests__/theme.validation.test.ts`:
- Around line 56-63: Add a new negative test alongside the existing "rejects
invalid name" test that sets the cloned theme's name to a whitespace-only string
(e.g. theme["name"] = " ") and asserts the same validation failure using
expectValidationError; locate where cloneDarkTheme() and
expectValidationError(...) are used in the existing test and mirror that pattern
to ensure whitespace-only names are rejected (use the same expected message as
the empty-string case).
In `@packages/core/src/widgets/__tests__/composition.useAnimatedValue.test.ts`:
- Around line 213-265: The pause snapshot is taken before flushing pending
effects causing a race; after calling useAnimatedValue and rendering the paused
playback state, call h.runPending(render.pendingEffects) first and then read
pausedValue = render.result.value (i.e., move the pausedValue capture to after
the h.runPending call for the paused render) so the in-flight tick is flushed
before asserting the paused value and isAnimating state.
In `@packages/core/src/widgets/__tests__/graphics.golden.test.ts`:
- Around line 325-326: Replace the raw bit-manipulation used for underlineColor
with the packRgb helper for consistency and readability: change the value
assigned to underlineColor (currently `(0 << 16) | (170 << 8) | 255`) to use
packRgb(0, 170, 255) so it matches the other color usage (e.g., the earlier
packRgb call) in the test file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e3bc76a-e454-4abe-9155-bd858138cd6d
⛔ Files ignored due to path filters (13)
packages/testkit/fixtures/zrdl-v1-graphics/widgets/barchart_highres.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1-graphics/widgets/link_docs.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1-graphics/widgets/richtext_underline_ext.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1-graphics/widgets/sparkline_highres.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1/widgets/button_focus_states.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1/widgets/divider_with_label.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1/widgets/input_basic.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1/widgets/input_disabled.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1/widgets/input_focused_inverse.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1/widgets/layer_backdrop_opaque.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1/widgets/modal_backdrop_dim.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1/widgets/spinner_tick_0.binis excluded by!**/*.binpackages/testkit/fixtures/zrdl-v1/widgets/spinner_tick_1.binis excluded by!**/*.bin
📒 Files selected for processing (93)
CHANGELOG.mdCLAUDE.mddocs/design-system.mddocs/guide/animation.mddocs/guide/composition.mddocs/guide/hooks-reference.mddocs/guide/recommended-patterns.mddocs/guide/styling.mddocs/styling/index.mddocs/styling/style-props.mddocs/styling/theme.mddocs/widgets/checkbox.mddocs/widgets/index.mddocs/widgets/input.mddocs/widgets/table.mdpackages/core/src/app/createApp.tspackages/core/src/app/inspectorOverlayHelper.tspackages/core/src/app/types.tspackages/core/src/forms/__tests__/form.disabled.test.tspackages/core/src/forms/__tests__/form.fieldArray.test.tspackages/core/src/forms/__tests__/form.wizard.test.tspackages/core/src/forms/__tests__/harness.tspackages/core/src/forms/__tests__/useForm.test.tspackages/core/src/index.tspackages/core/src/keybindings/__tests__/keybinding.conflicts.test.tspackages/core/src/pipeline.tspackages/core/src/renderer/__tests__/checkboxRecipeRendering.test.tspackages/core/src/renderer/__tests__/focusIndicators.test.tspackages/core/src/renderer/__tests__/inputRecipeRendering.test.tspackages/core/src/renderer/__tests__/recipeRendering.test-utils.tspackages/core/src/renderer/__tests__/selectRecipeRendering.test.tspackages/core/src/renderer/__tests__/sliderRecipeRendering.test.tspackages/core/src/renderer/__tests__/tableRecipeRendering.test.tspackages/core/src/renderer/renderToDrawlist/renderPackets.tspackages/core/src/renderer/renderToDrawlist/renderTree.tspackages/core/src/renderer/renderToDrawlist/themeTokens.tspackages/core/src/renderer/renderToDrawlist/widgets/collections.tspackages/core/src/renderer/renderToDrawlist/widgets/editors.tspackages/core/src/renderer/renderToDrawlist/widgets/focusConfig.tspackages/core/src/renderer/renderToDrawlist/widgets/overlays.tspackages/core/src/renderer/renderToDrawlist/widgets/renderChartWidgets.tspackages/core/src/renderer/renderToDrawlist/widgets/renderFormWidgets.tspackages/core/src/renderer/renderToDrawlist/widgets/renderTextWidgets.tspackages/core/src/router/__tests__/helpers.test.tspackages/core/src/runtime/__tests__/focus.layers.test.tspackages/core/src/runtime/__tests__/hooks.useTheme.test.tspackages/core/src/runtime/__tests__/reconcile.composite.test.tspackages/core/src/runtime/commit.tspackages/core/src/runtime/router/layer.tspackages/core/src/testing/renderer.tspackages/core/src/theme/__tests__/theme.interop.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/__tests__/theme.validation.test.tspackages/core/src/theme/defaultTheme.tspackages/core/src/theme/extract.tspackages/core/src/theme/index.tspackages/core/src/theme/interop.tspackages/core/src/theme/resolve.tspackages/core/src/theme/theme.tspackages/core/src/theme/tokens.tspackages/core/src/theme/types.tspackages/core/src/theme/validate.tspackages/core/src/ui/__tests__/designSystem.renderer.test.tspackages/core/src/ui/__tests__/themed.test.tspackages/core/src/ui/recipes.tspackages/core/src/widgets/__tests__/accordion.test.tspackages/core/src/widgets/__tests__/basicWidgets.render.test.tspackages/core/src/widgets/__tests__/breadcrumb.test.tspackages/core/src/widgets/__tests__/composition.animationHooks.test.tspackages/core/src/widgets/__tests__/composition.animationOrchestration.test.tspackages/core/src/widgets/__tests__/composition.animationPlayback.test.tspackages/core/src/widgets/__tests__/composition.test.tspackages/core/src/widgets/__tests__/composition.useAnimatedValue.test.tspackages/core/src/widgets/__tests__/graphics.golden.test.tspackages/core/src/widgets/__tests__/layers.golden.test.tspackages/core/src/widgets/__tests__/modal.focus.test.tspackages/core/src/widgets/__tests__/overlay.edge.test.tspackages/core/src/widgets/__tests__/pagination.test.tspackages/core/src/widgets/__tests__/renderer.regressions.test.tspackages/core/src/widgets/__tests__/tabs.test.tspackages/core/src/widgets/__tests__/vnode.prop-validation.test.tspackages/core/src/widgets/composition.tspackages/core/src/widgets/hooks/animation.tspackages/core/src/widgets/hooks/data.tspackages/core/src/widgets/protocol.tspackages/core/src/widgets/tests/protocol.test.tspackages/core/src/widgets/types.tspackages/core/src/widgets/ui.tspackages/ink-compat/src/runtime/createInkRenderer.tspackages/jsx/src/__tests__/navigation.test.tsxpackages/node/src/index.ts
packages/core/src/widgets/__tests__/composition.animationPlayback.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/debug/describeThrown.ts (1)
1-8: LGTM — solid defensive utility for error formatting.The fallback handling for unstringifiable values is a nice touch. One optional hardening: the Error branch (line 2) could theoretically throw if
nameormessageare defined as throwing getters on exotic Error subclasses. For maximum resilience in error-handling paths, you could wrap the entire body:🛡️ Optional: full try/catch envelope
export function describeThrown(value: unknown): string { + try { if (value instanceof Error) return `${value.name}: ${value.message}`; - try { return String(value); - } catch { + } catch { return "[unstringifiable thrown value]"; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/debug/describeThrown.ts` around lines 1 - 8, Wrap the entire body of describeThrown in a try/catch so any accessors on Error subclasses (like throwing getters for name or message) or other unexpected exceptions are safely handled; specifically, protect the Error branch in describeThrown (the instanceof Error check and subsequent `${value.name}: ${value.message}` construction) by moving all logic inside a single try and returning the existing fallback "[unstringifiable thrown value]" in the catch so the function never rethrows while formatting thrown values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guide/styling.md`:
- Around line 15-18: The example hard-binds color resolution to darkTheme (calls
to resolveColorToken(darkTheme, ...)) which breaks runtime theme switching;
update the ui.text and ui.box examples to resolve colors against the active
runtime theme (e.g., replace darkTheme with the runtime theme variable or getter
used in your app such as theme or ui.theme) or explicitly label these snippets
as "dark-theme only" examples; look for resolveColorToken, darkTheme, ui.text
and ui.box in the snippet and either swap the theme argument to the runtime
theme accessor or add a clear comment/title indicating this is a fixed
dark-theme example.
In `@packages/core/src/renderer/renderToDrawlist/widgets/editors.ts`:
- Around line 53-63: The function logLevelToThemeColor should explicitly handle
the "debug" level so theme.colors["widget.logs.debug"] is used instead of
falling through to the default; update logLevelToThemeColor to add a case
"debug" that returns theme.colors["widget.logs.debug"] with an appropriate
fallback (e.g. theme.colors.muted or another existing color) and leave the other
cases unchanged.
---
Nitpick comments:
In `@packages/core/src/debug/describeThrown.ts`:
- Around line 1-8: Wrap the entire body of describeThrown in a try/catch so any
accessors on Error subclasses (like throwing getters for name or message) or
other unexpected exceptions are safely handled; specifically, protect the Error
branch in describeThrown (the instanceof Error check and subsequent
`${value.name}: ${value.message}` construction) by moving all logic inside a
single try and returning the existing fallback "[unstringifiable thrown value]"
in the catch so the function never rethrows while formatting thrown values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff6afb74-38e5-4116-a16b-f91e45dfc1ac
📒 Files selected for processing (16)
docs/guide/recommended-patterns.mddocs/guide/styling.mddocs/styling/index.mdpackages/core/src/debug/describeThrown.tspackages/core/src/keybindings/__tests__/keybinding.conflicts.test.tspackages/core/src/keybindings/index.tspackages/core/src/keybindings/manager.tspackages/core/src/renderer/renderToDrawlist/renderPackets.tspackages/core/src/renderer/renderToDrawlist/widgets/editors.tspackages/core/src/renderer/renderToDrawlist/widgets/focusConfig.tspackages/core/src/theme/theme.tspackages/core/src/widgets/__tests__/composition.animationPlayback.test.tspackages/core/src/widgets/__tests__/graphics.golden.test.tspackages/core/src/widgets/diffViewer.tspackages/core/src/widgets/hooks/data.tspackages/core/src/widgets/logsConsole.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/renderer/renderToDrawlist/widgets/focusConfig.ts
- docs/guide/recommended-patterns.md
- packages/core/src/widgets/tests/graphics.golden.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/core/src/testing/__tests__/snapshot.test.ts (1)
115-129: Add multi-theme snapshot coverage for semantic regression protection.These tests now use
compileTheme, but only withdarkTheme. Please add a small table-driven case over all built-in presets so semantic token regressions are caught across themes, not just dark.Suggested test pattern
+const themesUnderTest = [ + { name: "dark", definition: darkTheme }, + // add remaining built-in presets exported from ../../theme/presets.js +]; + +for (const { name, definition } of themesUnderTest) { + it(`captures deterministic output for ${name} theme`, () => { + const theme = compileTheme(definition); + const vnode = ui.column({ gap: 1 }, [ + ui.text("Title", { style: { bold: true } }), + ui.text("Body text"), + ]); + const snap1 = captureSnapshot("det", vnode, { viewport: { cols: 40, rows: 5 }, theme }, name); + const snap2 = captureSnapshot("det", vnode, { viewport: { cols: 40, rows: 5 }, theme }, name); + assert.equal(snap1.content, snap2.content); + }); +}As per coding guidelines, "Validate screens in all built-in themes (Dark, Light, Dimmed, High-contrast, Nord, Dracula) without code changes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/testing/__tests__/snapshot.test.ts` around lines 115 - 129, The test only validates snapshots with darkTheme; add table-driven coverage over all built-in presets by iterating a themes array (e.g., darkTheme, lightTheme, dimmedTheme, highContrastTheme, nordTheme, draculaTheme), calling compileTheme for each, then creating the same snapshot via captureSnapshot("simple-text", ui.text("Hello"), { viewport: { cols: 40, rows: 5 }, theme }, <themeName>) and asserting snapshot.metadata.scene === "simple-text", snapshot.metadata.theme === <themeName>, and that snapshot.content includes "Hello"; place this loop inside the existing test (or a new it block) so compileTheme, captureSnapshot, ui.text, and the same assert.equal/assert.ok checks run for every theme.packages/core/src/forms/__tests__/form.disabled.test.ts (1)
24-71: Consider using the sharedcreateFormHarnessfromharness.ts.This test file defines its own
createTestContextthat is nearly identical tocreateFormHarnessinpackages/core/src/forms/__tests__/harness.ts. Theform.fieldArray.test.tsfile already uses the shared harness. Consolidating to the shared harness would reduce duplication and ensure consistent test context behavior across form tests.The only difference is the missing
getInvalidateCountmethod here, which could be added if needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/forms/__tests__/form.disabled.test.ts` around lines 24 - 71, Swap the local createTestContext implementation in this test with the shared createFormHarness from harness.ts: remove the createTestContext function and import createFormHarness, then update calls to use the harness's render/unmount API (match the existing useForm call sites). If any tests rely on getInvalidateCount, add that helper to the shared createFormHarness in harness.ts (expose a getInvalidateCount method that returns the registry's invalidate counter) so tests keep the same behavior; ensure types for the harness render method match UseFormOptions/UseFormReturn used here.packages/core/src/forms/__tests__/useForm.test.ts (1)
36-89: Same test harness duplication pattern.This file defines its own
createTestContextthat is nearly identical to the shared harness, though it does includegetInvalidateCount. Consider consolidating with the sharedcreateFormHarness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/forms/__tests__/useForm.test.ts` around lines 36 - 89, The local createTestContext duplicates the shared harness; replace it by importing and using the shared createFormHarness instead of defining createTestContext, and ensure the shared harness exposes the same getInvalidateCount API (or extend createFormHarness to return getInvalidateCount) so existing tests that reference getInvalidateCount continue to work; update references from createTestContext(...) to createFormHarness(...) and remove the duplicate function.packages/core/src/forms/__tests__/form.wizard.test.ts (1)
22-69: Same test harness duplication asform.disabled.test.ts.This file also defines its own
createTestContextthat duplicates the sharedcreateFormHarness. Consider migrating to the shared harness for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/forms/__tests__/form.wizard.test.ts` around lines 22 - 69, The test duplicates the local createTestContext function; replace it with the shared harness createFormHarness: remove the createTestContext implementation and import createFormHarness from the test utilities, then update tests to call createFormHarness() (and adapt type generics if needed) so the tests use the common harness used by form.disabled.test.ts; ensure references to registry/instance lifecycle (unmount) are covered by the shared harness API and adjust calls from createTestContext.render/unmount to the corresponding createFormHarness methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/guide/styling.md`:
- Around line 45-49: The example uses app.setTheme(darkTheme) but the app was
already initialized with darkTheme, so update the runtime-switching snippet to
apply a different theme than the initial one (e.g., app.setTheme(lightTheme) or
app.setTheme(anotherTheme)) and ensure the alternate theme variable
(lightTheme/anotherTheme) is declared in the example (or show creating it
inline) so the snippet truly demonstrates a runtime theme transition; modify the
code around app.setTheme(darkTheme) and the theme declarations (darkTheme,
lightTheme) accordingly.
---
Nitpick comments:
In `@packages/core/src/forms/__tests__/form.disabled.test.ts`:
- Around line 24-71: Swap the local createTestContext implementation in this
test with the shared createFormHarness from harness.ts: remove the
createTestContext function and import createFormHarness, then update calls to
use the harness's render/unmount API (match the existing useForm call sites). If
any tests rely on getInvalidateCount, add that helper to the shared
createFormHarness in harness.ts (expose a getInvalidateCount method that returns
the registry's invalidate counter) so tests keep the same behavior; ensure types
for the harness render method match UseFormOptions/UseFormReturn used here.
In `@packages/core/src/forms/__tests__/form.wizard.test.ts`:
- Around line 22-69: The test duplicates the local createTestContext function;
replace it with the shared harness createFormHarness: remove the
createTestContext implementation and import createFormHarness from the test
utilities, then update tests to call createFormHarness() (and adapt type
generics if needed) so the tests use the common harness used by
form.disabled.test.ts; ensure references to registry/instance lifecycle
(unmount) are covered by the shared harness API and adjust calls from
createTestContext.render/unmount to the corresponding createFormHarness methods.
In `@packages/core/src/forms/__tests__/useForm.test.ts`:
- Around line 36-89: The local createTestContext duplicates the shared harness;
replace it by importing and using the shared createFormHarness instead of
defining createTestContext, and ensure the shared harness exposes the same
getInvalidateCount API (or extend createFormHarness to return
getInvalidateCount) so existing tests that reference getInvalidateCount continue
to work; update references from createTestContext(...) to createFormHarness(...)
and remove the duplicate function.
In `@packages/core/src/testing/__tests__/snapshot.test.ts`:
- Around line 115-129: The test only validates snapshots with darkTheme; add
table-driven coverage over all built-in presets by iterating a themes array
(e.g., darkTheme, lightTheme, dimmedTheme, highContrastTheme, nordTheme,
draculaTheme), calling compileTheme for each, then creating the same snapshot
via captureSnapshot("simple-text", ui.text("Hello"), { viewport: { cols: 40,
rows: 5 }, theme }, <themeName>) and asserting snapshot.metadata.scene ===
"simple-text", snapshot.metadata.theme === <themeName>, and that
snapshot.content includes "Hello"; place this loop inside the existing test (or
a new it block) so compileTheme, captureSnapshot, ui.text, and the same
assert.equal/assert.ok checks run for every theme.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d226e1f-54c2-47a1-89e4-861c69571d84
📒 Files selected for processing (13)
docs/guide/styling.mdexamples/gallery/src/index.tspackages/core/src/forms/__tests__/form.disabled.test.tspackages/core/src/forms/__tests__/form.fieldArray.test.tspackages/core/src/forms/__tests__/form.wizard.test.tspackages/core/src/forms/__tests__/harness.tspackages/core/src/forms/__tests__/useForm.test.tspackages/core/src/renderer/renderToDrawlist/widgets/editors.tspackages/core/src/repro/replay.tspackages/core/src/runtime/router/types.tspackages/core/src/testing/__tests__/snapshot.test.tspackages/core/src/widgets/__tests__/composition.animationPlayback.test.tspackages/node/src/__tests__/config_guards.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/forms/tests/harness.ts
- packages/core/src/widgets/tests/composition.animationPlayback.test.ts
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/core/src/runtime/__tests__/hooks.useTheme.test.ts (1)
72-83:⚠️ Potential issue | 🟡 MinorTest title is slightly misleading after fallback hardening.
The title says composite context does not provide tokens, but Line 38 always injects default tokens when omitted. Consider renaming to reflect “omitted in options” instead of “not provided in context.”
✏️ Proposed title tweak
- test("falls back to default theme tokens when composite context does not provide one", () => { + test("uses default theme tokens when colorTokens is omitted", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/__tests__/hooks.useTheme.test.ts` around lines 72 - 83, Rename the test title string used in the test(...) call to accurately reflect that default tokens are injected when tokens are omitted in the options (not that the composite context fails to provide them); update the title on the test containing defineWidget, ctx.useTheme, createCompositeHarness and the assertion against defaultTheme.definition.colors to something like "falls back to default theme tokens when tokens are omitted in options" so it matches the behavior where default tokens are injected when omitted.packages/core/src/app/createApp.ts (1)
619-625:⚠️ Potential issue | 🟡 MinorAvoid double-enqueuing
renderRequestduring theme transitions.Line 624 enqueues a
renderRequest, and Lines 1536-1539 can enqueue another one in the same turn becauserenderRequestQueuedForCurrentTurnis not set inscheduleThemeTransitionContinuation(). This creates redundant scheduler work during transitions.♻️ Proposed fix
function scheduleThemeTransitionContinuation(): void { if (!themeTransition || sm.state !== "Running") return; // Theme-aware composite widgets resolve recipe styles during commit, so // transition frames must invalidate view/commit, not only render. - markDirty(DIRTY_VIEW, false); - scheduler.enqueue({ kind: "renderRequest" }); + markDirty(DIRTY_VIEW); }Also applies to: 1536-1539
🤖 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 619 - 625, scheduleThemeTransitionContinuation() can enqueue a redundant renderRequest because it doesn't set or check renderRequestQueuedForCurrentTurn; update it to either check renderRequestQueuedForCurrentTurn before calling scheduler.enqueue({ kind: "renderRequest" }) or set renderRequestQueuedForCurrentTurn = true immediately after enqueuing so the later enqueue path (the code around renderRequestQueuedForCurrentTurn at lines ~1536-1539) won't enqueue again in the same turn; reference scheduleThemeTransitionContinuation(), renderRequestQueuedForCurrentTurn and scheduler.enqueue to locate and change the logic.packages/core/src/forms/useForm.ts (1)
1374-1379:⚠️ Potential issue | 🟠 MajorAdd submission-epoch guards for in-flight async callbacks.
In-flight async validation/submit callbacks can still apply state updates (or invoke submit continuation) after
reset()or a newer submit starts. This can reintroduce stale errors/state and trigger outdated submit flows.🛠️ Proposed fix (submission epoch invalidation)
@@ const submittingRef = ctx.useRef(false); + const submitEpochRef = ctx.useRef(0); @@ const reset = (): void => { + submitEpochRef.current += 1; // invalidate in-flight submit/validation callbacks submittingRef.current = false; asyncValidatorRef.current?.cancel(); fieldArrayKeysRef.current = {}; updateFormState(createInitialState(options)); }; @@ const handleSubmit = (): void => { + const submitEpoch = ++submitEpochRef.current; const snapshot = stateRef.current; @@ const failSubmit = (error: unknown): void => { + if (submitEpoch !== submitEpochRef.current) return; @@ const finishSuccessfulSubmit = (): void => { + if (submitEpoch !== submitEpochRef.current) return; @@ void submitResult.then( () => { + if (submitEpoch !== submitEpochRef.current) return; submittingRef.current = false; finishSuccessfulSubmit(); }, (error) => { + if (submitEpoch !== submitEpochRef.current) return; submittingRef.current = false; failSubmit(error); }, ); @@ void runAsyncValidationFiltered(submitValues, snapshot).then( (asyncErrors) => { + if (submitEpoch !== submitEpochRef.current) return; const allErrors = mergeValidationErrors(syncErrors, asyncErrors); @@ (error) => { + if (submitEpoch !== submitEpochRef.current) return; submittingRef.current = false; updateFormState((prev) => ({ ...prev, isSubmitting: false, submitError: error, })); }, );As per coding guidelines: "
**/*.ts: All code changes must comply with docs/dev/code-standards.md including TypeScript strictness, runtime/layout/reconciliation invariants, and callback safety with async cancellation."Also applies to: 1459-1491, 1499-1530
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/forms/useForm.ts` around lines 1374 - 1379, The reset() path must invalidate any in-flight async callbacks by bumping a submission epoch counter and having async validators/submission continuations check that epoch before applying state/continuing; add a submissionEpochRef (number) incremented in reset() and at submit start, capture the epoch in any asyncValidatorRef tasks and in submit/validation callbacks, and before calling updateFormState, mutating submittingRef, invoking submit continuation, or touching fieldArrayKeysRef, compare the captured epoch to submissionEpochRef.current and abort if mismatched; update createInitialState/reset/submit-related code paths to maintain this epoch invariant so stale async results are ignored.
🧹 Nitpick comments (9)
packages/core/src/layout/validateProps.ts (1)
948-949: Update the Input validator JSDoc to includereadOnly.The function summary is now slightly stale relative to the validated shape.
📝 Suggested doc tweak
-/** Validate Input props: id (required, non-empty), value (required), disabled (default false). */ +/** Validate Input props: id (required, non-empty), value (required), disabled/readOnly (default false). */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/validateProps.ts` around lines 948 - 949, Update the JSDoc for the validateInputProps function to reflect the current validated shape by adding readOnly to the summary/list of validated properties; specifically edit the comment on the validateInputProps export so it documents id (required, non-empty), value (required), disabled (default false) and readOnly (default false/optional as appropriate), and ensure the description matches the ValidatedInputProps type referenced by the function.packages/ink-compat/src/runtime/createInkRenderer.ts (1)
480-498: Remove unused debug helper.The
countSelfDirtyfunction is defined but never called within this file. The comment explicitly states it's temporary and should be removed after investigation. Consider removing this dead code.🧹 Proposed removal
-// --------------------------------------------------------------------------- -// Debug helpers (temporary — remove after investigation) -// --------------------------------------------------------------------------- - -function countSelfDirty(root: RuntimeInstance | null): number { - if (!root) return 0; - let count = 0; - const stack: RuntimeInstance[] = [root]; - while (stack.length > 0) { - const node = stack.pop(); - if (!node) continue; - if (node.selfDirty) count++; - for (let i = node.children.length - 1; i >= 0; i--) { - const child = node.children[i]; - if (child) stack.push(child); - } - } - return count; -} -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ink-compat/src/runtime/createInkRenderer.ts` around lines 480 - 498, The file contains an unused debug helper function countSelfDirty (and its surrounding debug comment block) that is dead code; remove the entire countSelfDirty function and the "// Debug helpers (temporary — remove after investigation)" comment block so no unused symbols remain (the function references RuntimeInstance and RuntimeInstance.children/selfDirty — ensure there are no other references to countSelfDirty elsewhere before deleting).packages/core/src/runtime/__tests__/hooks.useTheme.test.ts (1)
117-139: Consider adding a built-in theme matrix variant of this scoped-override test.This scoped override assertion is good; adding a parameterized pass across Dark/Light/Dimmed/High-contrast/Nord/Dracula would harden regression coverage for theme switching guarantees.
Based on learnings: Support theme switching without code changes across built-in themes: Dark, Light, Dimmed, High-contrast, Nord, Dracula.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/__tests__/hooks.useTheme.test.ts` around lines 117 - 139, Add a parameterized variant of the "resolves scoped themed overrides for composites" test that iterates the built-in themes (darkTheme, lightTheme, dimmedTheme, highContrastTheme, nordTheme, draculaTheme): for each theme, call compileTheme(theme) to get baseTheme, mergeThemeOverride(baseTheme, override) to build scopedTheme, create the Widget via defineWidget that captures ctx.useTheme(), commit with createCompositeHarness and ui.themed(override, [Widget({})]) using getColorTokens(baseTheme) and assert.deepEqual(seenTokens, scopedTheme.definition.colors; reuse the same override and test logic so the scoped override behavior is verified across all listed themes.packages/core/src/theme/__tests__/theme.test.ts (1)
7-13: Parameterize this runtime compilation test across all built-in presets.This only validates
darkTheme; the same alias/spacing invariants should be asserted across all built-in themes to catch preset-specific regressions during semantic theme switching.Based on learnings: Support theme switching without code changes across built-in themes: Dark, Light, Dimmed, High-contrast, Nord, Dracula.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/theme/__tests__/theme.test.ts` around lines 7 - 13, Replace the single-theme assertion with a parameterized test that iterates over all built-in theme presets and runs the same invariants; specifically, update the test around compileTheme and darkTheme to loop through the preset array (e.g., [darkTheme, lightTheme, dimmedTheme, highContrastTheme, nordTheme, draculaTheme]) and for each preset call compileTheme(preset) and assert that compiledTheme.colors.primary equals preset.colors.accent.primary, compiledTheme.colors["accent.primary"] equals preset.colors.accent.primary, and compiledTheme.spacing deep-equals the expected spacing array; keep using the existing test name (or create a descriptive one) and the same assertions but executed for each theme so the invariants are validated across all presets.packages/core/src/theme/__tests__/theme.switch.test.ts (1)
156-157: Broaden runtime-switch coverage to all built-in themes.These scenarios now validate Light/Dark paths, but the switch-hardening objective would be safer with one parameterized case that cycles all built-ins (Dark, Light, Dimmed, High-contrast, Nord, Dracula) and asserts state preservation + frame submission behavior.
📌 Suggested test shape
+import { + darkTheme, + lightTheme, + dimmedTheme, + highContrastTheme, + nordTheme, + draculaTheme, +} from "../presets.js"; + +const builtInThemes = [ + ["darkTheme", darkTheme], + ["lightTheme", lightTheme], + ["dimmedTheme", dimmedTheme], + ["highContrastTheme", highContrastTheme], + ["nordTheme", nordTheme], + ["draculaTheme", draculaTheme], +] as const; + +for (const [name, nextTheme] of builtInThemes) { + test(`theme switch preserves focus when switching to ${name}`, async () => { + // bootstrap + focus a widget + // app.setTheme(nextTheme) + // assert focused action/selection behavior remains correct + }); +}Based on learnings: Support theme switching without code changes across built-in themes: Dark, Light, Dimmed, High-contrast, Nord, Dracula.
Also applies to: 342-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/theme/__tests__/theme.switch.test.ts` around lines 156 - 157, Update the test to be parameterized over the built-in themes (darkTheme, lightTheme, dimmedTheme, highContrastTheme, nordTheme, draculaTheme) instead of only Light/Dark: for each initial built-in theme create the app via createApp({ backend, initialState: 0, theme: <theme> }), then cycle the app through all built-in themes (using the existing theme-switch API on the app instance) and assert that the app preserves its state (initialState remains) and that frame submission behavior (submitFrame or the existing frame submission hook used in other tests) continues to be invoked/works after each switch; reuse existing assertions from the Light/Dark cases and consolidate into a single parameterized test to cover all built-ins.packages/core/src/widgets/__tests__/basicWidgets.render.test.ts (1)
522-530: Consider a tiny helper for repeated theme-override setup.The current pattern is correct, but extracting it once would reduce duplication and improve scanability.
♻️ Proposed refactor
+function compileWithDefaultTheme(overrides: Parameters<typeof extendTheme>[1]): Theme { + return compileTheme(extendTheme(defaultTheme.definition, overrides)); +} -const theme = compileTheme( - extendTheme(defaultTheme.definition, { - colors: { - diagnostic: { - info: (1 << 16) | (2 << 8) | 3, - }, - }, - }), -); +const theme = compileWithDefaultTheme({ + colors: { + diagnostic: { + info: (1 << 16) | (2 << 8) | 3, + }, + }, +});Also applies to: 564-572, 601-611, 640-649
🤖 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 522 - 530, Extract the repeated theme-override setup into a small helper (e.g., buildTestTheme or makeDiagnosticTheme) and replace the repeated calls to compileTheme(extendTheme(defaultTheme.definition, { colors: { diagnostic: { info: ... } } })) with that helper; ensure the helper accepts the diagnostic color value (or a partial override) and returns the compiled theme so callers in tests (currently using compileTheme, extendTheme, defaultTheme.definition and the colors.diagnostic.info override) simply call the helper to reduce duplication and improve readability.packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts (1)
165-218: Avoid fixedsleep(80)delays; they can make async animation tests flaky.The fixed waits can fail intermittently under CI jitter. Prefer condition-based polling only (same
waitForpattern) for pause stability checks.Suggested deterministic pattern (apply similarly in both tests)
- await sleep(80); - render = h.render((hooks) => useParallel(hooks, paused)); - h.runPending(render.pendingEffects); - assert.ok(Math.abs((render.result[0]?.value ?? 0) - pausedValue) <= 0.1); + let stableSamples = 0; + let previous = pausedValue; + await waitFor(() => { + const next = h.render((hooks) => useParallel(hooks, paused)); + h.runPending(next.pendingEffects); + const value = next.result[0]?.value ?? 0; + stableSamples = Math.abs(value - previous) <= 0.1 ? stableSamples + 1 : 0; + previous = value; + render = next; + return stableSamples >= 3; + }); + assert.ok(Math.abs((render.result[0]?.value ?? 0) - pausedValue) <= 0.1);Also applies to: 312-366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts` around lines 165 - 218, Replace the fixed sleep(80) pause check with a condition-based poll using the existing waitFor pattern: after switching to the paused config (the render using useParallel with playback.paused true), capture pausedValue from render.result[0]?.value and then call waitFor to repeatedly render/useParallel (and run pending effects) until the current result[0]?.value stays within a small delta of pausedValue (e.g., <= 0.1) and result[0]?.isAnimating is false; remove the sleep call and the immediate re-render that assumes a fixed delay—apply the same replacement for the other test mention (lines 312-366) so pause stability is asserted deterministically using waitFor and h.render/useParallel.packages/core/src/runtime/commit.ts (1)
862-880: Consider centralizing container-kind classification to avoid drift.
fragmenthad to be added in multiple container switch/lists. A shared predicate/set would reduce future mismatch risk across commit traversal/rewrite paths.Refactor sketch
+const CONTAINER_KINDS = new Set<VNode["kind"]>([ + "fragment", + "box", + "row", + "column", + "themed", + "grid", + "focusZone", + "focusTrap", + "layers", + "field", + "tabs", + "accordion", + "breadcrumb", + "pagination", + "splitPane", + "panelGroup", + "resizablePanel", + "modal", + "layer", +]); + +function isContainerKind(kind: VNode["kind"]): boolean { + return CONTAINER_KINDS.has(kind); +}Then reuse this in
commitChildrenForVNode,isContainerVNode, andrewriteCommittedVNode.Also applies to: 1189-1211, 1244-1263
🤖 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 862 - 880, Centralize the container-kind classification by introducing a single shared predicate or constant set (e.g., isContainerKind(kind: string) or CONTAINER_KINDS = new Set([...])) and replace the duplicated kind-lists in commitChildrenForVNode, isContainerVNode, and rewriteCommittedVNode with calls to that predicate or membership checks; update each function to import/use the shared helper so adding/removing container kinds only needs one edit and ensure the new helper covers all kinds currently listed across the three locations (including fragment, box, row, column, themed, grid, focusZone, focusTrap, layers, tabs, accordion, breadcrumb, pagination, splitPane, panelGroup).packages/core/src/theme/interop.ts (1)
35-39: The cast toThemeOverridesis safe due to downstream validation.Line 39 casts
overridetoThemeOverrideswithout pre-validation, butextendThemeimmediately validates the merged result viavalidateTheme(), which rejects malformed input. The cast is acceptable sinceThemeOverridesaccepts partial structures and validation occurs downstream.For improved fail-fast behavior and clearer error context, consider adding a lightweight shape check before the cast (similar to
isThemeDefinition), though this is optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/theme/interop.ts` around lines 35 - 39, mergeThemeOverride currently casts override to ThemeOverrides when calling extendTheme without a prior shape check; either add a lightweight guard like an isThemeOverride/shape check (similar to isThemeDefinition) before casting and call compileTheme(extendTheme(...)) only if it passes, or keep the cast but add an explicit comment/assertion referencing downstream validateTheme to document why the cast is safe; update the mergeThemeOverride function and mention extendTheme, compileTheme, isThemeDefinition, isObject, ThemeOverrides and validateTheme so reviewers can find and verify the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/forms/useForm.ts`:
- Around line 550-563: runWizardStepValidation is merging stale source.errors
with freshly computed syncStepErrors in the wrong precedence, causing old errors
to reappear when validateOnChange is false; update the merge order in
runWizardStepValidation so the freshly computed errors (syncStepErrors and
customStepErrors) take precedence over source.errors (i.e., merge source.errors
into the final result, not the other way around), adjust the calls involving
mergeValidationErrors/pickValidationFields/filterDisabledValidationErrors
accordingly, and add a unit test that simulates: step validation fails → user
corrects value → retry transition with validateOnChange disabled to ensure the
stale error is cleared; also review resolveWizardTransition, nextStep, and
goToStep to ensure they rely on the corrected runWizardStepValidation behavior.
In `@packages/core/src/keybindings/manager.ts`:
- Around line 398-407: validateModeGraph currently only checks parent existence
but misses cycles; update validateModeGraph to detect and reject cycles by
performing a DFS (or equivalent) over the modes graph using two sets: visited
and inStack (recursion stack). For each modeName in modes, run a recursive
visit(modeName) that marks inStack, follows mode.parent (if present) and if you
encounter a parent already inStack throw an Error describing the cycle (include
the involved mode names), otherwise mark visited and remove from inStack; ensure
you reference CompiledMode and the modes map and throw a clear error from
validateModeGraph when a cycle is detected.
In `@packages/core/src/renderer/renderToDrawlist/widgets/containers.ts`:
- Around line 579-595: The fragment case is creating an implicit clip by setting
childClip = currentClip ?? { x: rect.x, y: rect.y, w: rect.w, h: rect.h } which
causes fragment-wrapped children to be clipped even when no parent clip is
active; change the logic in the "fragment" branch so childClip is undefined when
currentClip is undefined (i.e., use currentClip directly or currentClip ??
undefined) and pass that to pushChildrenWithLayout and clipOnFirstQueuedChild
(references: pushChildrenWithLayout, currentClip, childClip,
clipOnFirstQueuedChild) so fragments do not introduce their own clipping.
In `@packages/core/src/runtime/commit.ts`:
- Around line 1049-1056: readCompositeColorTokens currently calls
composite.getColorTokens(theme) directly which can throw or return
null/undefined; wrap the call to composite.getColorTokens in a try/catch to
prevent exceptions escaping commit (refer to function readCompositeColorTokens
and the composite.getColorTokens callback) and normalize the result so the
function always returns a valid ColorTokens object (fallback to
theme.definition.colors or defaultTheme.definition.colors on error or falsy
return). Also ensure any downstream usage (the caller referenced in the review)
relies on the non-null normalized return rather than assuming the runtime
callback honored the TypeScript typing.
In `@packages/core/src/runtime/widgetMeta.ts`:
- Around line 467-468: CollectedTrap.kind being optional causes modal-specific
focus logic in finalizeFocusWithPreCollectedMetadata() to be skipped for traps
produced without kind (e.g., the single-pass WidgetMetadataCollector path); make
kind mandatory or ensure a default is set when collecting metadata: update the
CollectedTrap type so kind?: "focusTrap" | "modal" becomes kind: "focusTrap" |
"modal", and/or modify WidgetMetadataCollector (the code path that creates
traps) to always assign kind = "focusTrap" when none is provided so
finalizeFocusWithPreCollectedMetadata() can reliably check activeTrap.kind ===
"modal".
In `@packages/core/src/theme/__tests__/theme.switch.test.ts`:
- Around line 51-56: The helper function themeWithPrimary now returns a
ThemeDefinition (from extendTheme) rather than a compiled Theme, so the test
titled "setTheme no-ops for identical Theme object identity" no longer exercises
identical compiled Theme instances; update the test to restore intent by either
renaming the test to reflect ThemeDefinition identity or by supplying a compiled
Theme to the assertion (e.g., compile the ThemeDefinition before passing it to
the setter used in the test); locate the test by its title and the helper
function themeWithPrimary to change the test name or to ensure you pass a
compiled Theme instance to the setTheme call so the test accurately covers
identical Theme object identity behavior.
In `@packages/core/src/theme/__tests__/theme.test.ts`:
- Around line 23-28: The test block for resolveSpacing misses coverage for
non-finite inputs; update the test in theme.test.ts (the section using
compileTheme(darkTheme) and resolveSpacing) to assert the function's contract
for non-finite values by adding assertions that resolveSpacing(theme, NaN),
resolveSpacing(theme, Infinity), and resolveSpacing(theme, -Infinity) return the
guarded value or expected fallback (matching the implementation) — locate the
compileTheme/darkTheme setup and append these assertions to prevent regressions.
- Around line 30-35: The test "resolveColorToken covers widget extension paths"
currently only checks valid token paths; add at least one assertion that an
invalid token path returns null to lock the contract. Update the test in
theme.test.ts to call resolveColorToken(darkTheme, "<some.invalid.path>") (e.g.
"widget.nonexistent.path" or "widget.toast.invalidKey") and assert it === null
using the same assertion style (assert.equal or similar) alongside the existing
checks so the behavior for invalid paths is explicitly tested.
In `@packages/core/src/theme/__tests__/theme.validation.test.ts`:
- Around line 46-54: The test currently models a missing token by setting the
value to undefined with setPath(theme, ["widget","toast","info"], undefined)
which leaves the key present; instead remove the key entirely so validation sees
a true missing path—replace that line with code that deletes the token key (for
example call a provided delete/unset helper like deletePath(theme,
["widget","toast","info"]) or traverse to theme.widget.toast and use the delete
operator to remove the info property) while keeping the rest of the test
(cloneDarkTheme, expectValidationError) unchanged.
In
`@packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts`:
- Around line 124-163: The tests using createHarness()/h (e.g., the "useParallel
uses the latest onComplete callback without restarting" test which calls waitFor
and assertions) must ensure h.unmount() runs even if waitFor or assertions
throw; wrap the test body logic in try/finally and call h.unmount() in the
finally block so timers/effects are always torn down. Apply the same try/finally
cleanup pattern to the other failing tests referenced (the blocks around lines
165-218, 268-310, 312-366) that create a harness and rely on waitFor/asserts,
ensuring each finally calls h.unmount().
---
Outside diff comments:
In `@packages/core/src/app/createApp.ts`:
- Around line 619-625: scheduleThemeTransitionContinuation() can enqueue a
redundant renderRequest because it doesn't set or check
renderRequestQueuedForCurrentTurn; update it to either check
renderRequestQueuedForCurrentTurn before calling scheduler.enqueue({ kind:
"renderRequest" }) or set renderRequestQueuedForCurrentTurn = true immediately
after enqueuing so the later enqueue path (the code around
renderRequestQueuedForCurrentTurn at lines ~1536-1539) won't enqueue again in
the same turn; reference scheduleThemeTransitionContinuation(),
renderRequestQueuedForCurrentTurn and scheduler.enqueue to locate and change the
logic.
In `@packages/core/src/forms/useForm.ts`:
- Around line 1374-1379: The reset() path must invalidate any in-flight async
callbacks by bumping a submission epoch counter and having async
validators/submission continuations check that epoch before applying
state/continuing; add a submissionEpochRef (number) incremented in reset() and
at submit start, capture the epoch in any asyncValidatorRef tasks and in
submit/validation callbacks, and before calling updateFormState, mutating
submittingRef, invoking submit continuation, or touching fieldArrayKeysRef,
compare the captured epoch to submissionEpochRef.current and abort if
mismatched; update createInitialState/reset/submit-related code paths to
maintain this epoch invariant so stale async results are ignored.
In `@packages/core/src/runtime/__tests__/hooks.useTheme.test.ts`:
- Around line 72-83: Rename the test title string used in the test(...) call to
accurately reflect that default tokens are injected when tokens are omitted in
the options (not that the composite context fails to provide them); update the
title on the test containing defineWidget, ctx.useTheme, createCompositeHarness
and the assertion against defaultTheme.definition.colors to something like
"falls back to default theme tokens when tokens are omitted in options" so it
matches the behavior where default tokens are injected when omitted.
---
Nitpick comments:
In `@packages/core/src/layout/validateProps.ts`:
- Around line 948-949: Update the JSDoc for the validateInputProps function to
reflect the current validated shape by adding readOnly to the summary/list of
validated properties; specifically edit the comment on the validateInputProps
export so it documents id (required, non-empty), value (required), disabled
(default false) and readOnly (default false/optional as appropriate), and ensure
the description matches the ValidatedInputProps type referenced by the function.
In `@packages/core/src/runtime/__tests__/hooks.useTheme.test.ts`:
- Around line 117-139: Add a parameterized variant of the "resolves scoped
themed overrides for composites" test that iterates the built-in themes
(darkTheme, lightTheme, dimmedTheme, highContrastTheme, nordTheme,
draculaTheme): for each theme, call compileTheme(theme) to get baseTheme,
mergeThemeOverride(baseTheme, override) to build scopedTheme, create the Widget
via defineWidget that captures ctx.useTheme(), commit with
createCompositeHarness and ui.themed(override, [Widget({})]) using
getColorTokens(baseTheme) and assert.deepEqual(seenTokens,
scopedTheme.definition.colors; reuse the same override and test logic so the
scoped override behavior is verified across all listed themes.
In `@packages/core/src/runtime/commit.ts`:
- Around line 862-880: Centralize the container-kind classification by
introducing a single shared predicate or constant set (e.g.,
isContainerKind(kind: string) or CONTAINER_KINDS = new Set([...])) and replace
the duplicated kind-lists in commitChildrenForVNode, isContainerVNode, and
rewriteCommittedVNode with calls to that predicate or membership checks; update
each function to import/use the shared helper so adding/removing container kinds
only needs one edit and ensure the new helper covers all kinds currently listed
across the three locations (including fragment, box, row, column, themed, grid,
focusZone, focusTrap, layers, tabs, accordion, breadcrumb, pagination,
splitPane, panelGroup).
In `@packages/core/src/theme/__tests__/theme.switch.test.ts`:
- Around line 156-157: Update the test to be parameterized over the built-in
themes (darkTheme, lightTheme, dimmedTheme, highContrastTheme, nordTheme,
draculaTheme) instead of only Light/Dark: for each initial built-in theme create
the app via createApp({ backend, initialState: 0, theme: <theme> }), then cycle
the app through all built-in themes (using the existing theme-switch API on the
app instance) and assert that the app preserves its state (initialState remains)
and that frame submission behavior (submitFrame or the existing frame submission
hook used in other tests) continues to be invoked/works after each switch; reuse
existing assertions from the Light/Dark cases and consolidate into a single
parameterized test to cover all built-ins.
In `@packages/core/src/theme/__tests__/theme.test.ts`:
- Around line 7-13: Replace the single-theme assertion with a parameterized test
that iterates over all built-in theme presets and runs the same invariants;
specifically, update the test around compileTheme and darkTheme to loop through
the preset array (e.g., [darkTheme, lightTheme, dimmedTheme, highContrastTheme,
nordTheme, draculaTheme]) and for each preset call compileTheme(preset) and
assert that compiledTheme.colors.primary equals preset.colors.accent.primary,
compiledTheme.colors["accent.primary"] equals preset.colors.accent.primary, and
compiledTheme.spacing deep-equals the expected spacing array; keep using the
existing test name (or create a descriptive one) and the same assertions but
executed for each theme so the invariants are validated across all presets.
In `@packages/core/src/theme/interop.ts`:
- Around line 35-39: mergeThemeOverride currently casts override to
ThemeOverrides when calling extendTheme without a prior shape check; either add
a lightweight guard like an isThemeOverride/shape check (similar to
isThemeDefinition) before casting and call compileTheme(extendTheme(...)) only
if it passes, or keep the cast but add an explicit comment/assertion referencing
downstream validateTheme to document why the cast is safe; update the
mergeThemeOverride function and mention extendTheme, compileTheme,
isThemeDefinition, isObject, ThemeOverrides and validateTheme so reviewers can
find and verify the change.
In `@packages/core/src/widgets/__tests__/basicWidgets.render.test.ts`:
- Around line 522-530: Extract the repeated theme-override setup into a small
helper (e.g., buildTestTheme or makeDiagnosticTheme) and replace the repeated
calls to compileTheme(extendTheme(defaultTheme.definition, { colors: {
diagnostic: { info: ... } } })) with that helper; ensure the helper accepts the
diagnostic color value (or a partial override) and returns the compiled theme so
callers in tests (currently using compileTheme, extendTheme,
defaultTheme.definition and the colors.diagnostic.info override) simply call the
helper to reduce duplication and improve readability.
In
`@packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts`:
- Around line 165-218: Replace the fixed sleep(80) pause check with a
condition-based poll using the existing waitFor pattern: after switching to the
paused config (the render using useParallel with playback.paused true), capture
pausedValue from render.result[0]?.value and then call waitFor to repeatedly
render/useParallel (and run pending effects) until the current result[0]?.value
stays within a small delta of pausedValue (e.g., <= 0.1) and
result[0]?.isAnimating is false; remove the sleep call and the immediate
re-render that assumes a fixed delay—apply the same replacement for the other
test mention (lines 312-366) so pause stability is asserted deterministically
using waitFor and h.render/useParallel.
In `@packages/ink-compat/src/runtime/createInkRenderer.ts`:
- Around line 480-498: The file contains an unused debug helper function
countSelfDirty (and its surrounding debug comment block) that is dead code;
remove the entire countSelfDirty function and the "// Debug helpers (temporary —
remove after investigation)" comment block so no unused symbols remain (the
function references RuntimeInstance and RuntimeInstance.children/selfDirty —
ensure there are no other references to countSelfDirty elsewhere before
deleting).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bb5578a-2069-412c-b236-7449bf11c902
📒 Files selected for processing (30)
packages/core/src/app/createApp.tspackages/core/src/app/widgetRenderer.tspackages/core/src/forms/types.tspackages/core/src/forms/useForm.tspackages/core/src/keybindings/manager.tspackages/core/src/layout/engine/layoutEngine.tspackages/core/src/layout/validateProps.tspackages/core/src/renderer/__tests__/inputRecipeRendering.test.tspackages/core/src/renderer/renderToDrawlist/widgets/containers.tspackages/core/src/renderer/renderToDrawlist/widgets/editors.tspackages/core/src/renderer/renderToDrawlist/widgets/renderChartWidgets.tspackages/core/src/runtime/__tests__/focus.layers.test.tspackages/core/src/runtime/__tests__/focus.traps.test.tspackages/core/src/runtime/__tests__/hooks.useTheme.test.tspackages/core/src/runtime/commit.tspackages/core/src/runtime/focus.tspackages/core/src/runtime/widgetMeta.tspackages/core/src/testing/renderer.tspackages/core/src/theme/__tests__/theme.switch.test.tspackages/core/src/theme/__tests__/theme.test.tspackages/core/src/theme/__tests__/theme.validation.test.tspackages/core/src/theme/interop.tspackages/core/src/theme/theme.tspackages/core/src/ui/__tests__/themed.test.tspackages/core/src/widgets/__tests__/basicWidgets.render.test.tspackages/core/src/widgets/__tests__/composition.animationOrchestration.test.tspackages/core/src/widgets/types.tspackages/core/src/widgets/ui.tspackages/ink-compat/src/runtime/createInkRenderer.tspackages/node/src/__tests__/config_guards.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/src/renderer/tests/inputRecipeRendering.test.ts
- packages/core/src/ui/tests/themed.test.ts
- packages/core/src/renderer/renderToDrawlist/widgets/renderChartWidgets.ts
- packages/core/src/widgets/types.ts
- packages/node/src/tests/config_guards.test.ts
packages/core/src/widgets/__tests__/composition.animationOrchestration.test.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/core/src/runtime/commit.ts (1)
1049-1059:⚠️ Potential issue | 🟠 Major
getColorTokensis still an unguarded user callback in commit.At Line 1055,
composite.getColorTokens(theme)can throw or returnnull/undefinedat runtime despite stricter TS types. At Line 1709, that call path is not protected, so exceptions can escape commit instead of returningZRUI_USER_CODE_THROW.Proposed hardening patch
function readCompositeColorTokens(ctx: CommitCtx): ColorTokens { const composite = ctx.composite; if (!composite) return defaultTheme.definition.colors; const theme = currentCompositeTheme(ctx); if (theme !== null) { - return composite.getColorTokens ? composite.getColorTokens(theme) : theme.definition.colors; + if (!composite.getColorTokens) return theme.definition.colors; + const resolved = composite.getColorTokens(theme); + return resolved ?? theme.definition.colors; } return composite.colorTokens ?? defaultTheme.definition.colors; } @@ - const colorTokens = readCompositeColorTokens(ctx); + let colorTokens: ColorTokens; + try { + colorTokens = readCompositeColorTokens(ctx); + } catch (e: unknown) { + return { + ok: false, + fatal: { + code: "ZRUI_USER_CODE_THROW", + detail: describeThrown(e), + }, + }; + }As per coding guidelines:
**/*.tschanges must preserve runtime/reconciliation invariants and callback safety with async cancellation.Also applies to: 1076-1079, 1709-1710, 2061-2064
🤖 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 1049 - 1059, readCompositeColorTokens currently calls the user-provided composite.getColorTokens(theme) without guarding against thrown exceptions or null/undefined returns; wrap that call in a try/catch and treat any thrown error as a user-code failure by returning the sentinel ZRUI_USER_CODE_THROW (or the existing runtime fallback), and if the callback returns null/undefined fall back to theme.definition.colors (or composite.colorTokens/defaultTheme.definition.colors) so commit never lets exceptions escape; apply the same hardening pattern to all other places that invoke composite.getColorTokens (including the other call sites around currentCompositeTheme and the commit call-paths noted) so every composite.getColorTokens invocation is exception- and null-safe.packages/core/src/runtime/widgetMeta.ts (1)
498-499:⚠️ Potential issue | 🟠 MajorMake
CollectedTrap.kindrequired and emitted in all collection paths.
CollectedTrap.kindis optional at Line 498, and the single-pass collector still emits traps withoutkind(Line 1039), whilecollectFocusTraps()now includes it. This makes trap metadata shape path-dependent and can break modal-specific focus handling.Suggested patch
export type CollectedTrap = Readonly<{ id: string; - kind?: "focusTrap" | "modal"; + kind: "focusTrap" | "modal"; active: boolean; returnFocusTo: string | null; initialFocus: string | null; focusableIds: readonly string[]; }>; @@ this._trapDataById.set(id, { id, + kind: vnode.kind, active, returnFocusTo, initialFocus, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/runtime/widgetMeta.ts` around lines 498 - 499, CollectedTrap currently has an optional kind field but some collection paths (the single-pass collector and other emitters) still produce traps without kind; make kind required on the CollectedTrap type and ensure every place that constructs or emits a CollectedTrap supplies a valid kind ("focusTrap" or "modal"). Update the CollectedTrap interface/typedef to remove the optional marker, then audit and modify the emitters/constructors including collectFocusTraps() and the single-pass collector to pass the appropriate kind value when creating the trap objects so all collection paths produce the same shape.
🧹 Nitpick comments (2)
packages/core/src/app/widgetRenderer.ts (1)
3281-3281: Consolidate force-full-render gating into one path.At Line 5084, the field is reset before
shouldAttemptIncrementalRender()runs, so the guard at Line 3281 is not the effective check in this flow. Consider centralizing the decision in one place to reduce maintenance ambiguity.♻️ Suggested simplification
- private shouldAttemptIncrementalRender( - doLayout: boolean, - viewport: Viewport, - theme: Theme, - ): boolean { - if (this.forceFullRenderOnNextSubmit) return false; + private shouldAttemptIncrementalRender( + doLayout: boolean, + viewport: Viewport, + theme: Theme, + forceFullRenderThisSubmit = false, + ): boolean { + if (forceFullRenderThisSubmit) return false; return shouldAttemptIncrementalRenderImpl({ hasRenderedFrame: this._hasRenderedFrame, doLayout, @@ - if ( - !forceFullRenderThisSubmit && - this.shouldAttemptIncrementalRender(doLayout, viewport, theme) - ) { + if ( + this.shouldAttemptIncrementalRender( + doLayout, + viewport, + theme, + forceFullRenderThisSubmit, + ) + ) {Also applies to: 5083-5088
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer.ts` at line 3281, The early guard using this.forceFullRenderOnNextSubmit (see the return at the top of the render path) is redundant because the field is reset earlier in the submit flow before shouldAttemptIncrementalRender() is evaluated; consolidate the gating by removing the early return and centralizing the decision inside shouldAttemptIncrementalRender() (or vice‑versa), and ensure the reset of forceFullRenderOnNextSubmit happens only after the centralized decision is made so the flag is effective; update references in shouldAttemptIncrementalRender(), submit handling, and any reset code (forceFullRenderOnNextSubmit reset) to use that single decision point.packages/core/src/app/createApp.ts (1)
921-925:assertLifecycleIdlecurrently duplicates an existing lifecycle-busy check path.These new per-method calls are redundant because the same condition is already enforced by
assertOperational(directly or throughassertKeybindingMutationAllowed). Consider consolidating to one guard to reduce drift.Also applies to: 1689-1690, 1705-1706, 1727-1728, 1749-1750, 1778-1779, 1791-1792, 1806-1807, 2006-2007, 2012-2013, 2018-2019
🤖 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 921 - 925, The assertLifecycleIdle function duplicates lifecycle-busy checks already enforced by assertOperational (and where applicable assertKeybindingMutationAllowed); remove the per-method calls to assertLifecycleIdle and rely on the existing guards instead—i.e., delete redundant assertLifecycleIdle(...) invocations in the affected methods and ensure each method calls assertOperational or assertKeybindingMutationAllowed as appropriate (look for usages around the symbol assertLifecycleIdle and the methods near lines referenced, and keep only the single existing guard such as assertOperational/assertKeybindingMutationAllowed to prevent lifecycle mutations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 40: Update the Unreleased CHANGELOG entry for the "core/theme + renderer"
change to explicitly mention color override/subtree inheritance: locate the
existing bullet that starts "**core/theme + renderer**: Removed the public
legacy theme API..." and append or modify the phrase about scoped override
inheritance to include "color subtree inheritance" (e.g., "completed scoped
override inheritance for spacing, focus indicators, and color subtree
inheritance") so the note correctly reflects the PR's color inheritance
behavior.
- Around line 26-42: There are duplicate subheadings ("### Breaking Changes",
"### Bug Fixes", "### Documentation") under the same "## [Unreleased]" section
which triggers MD024; open CHANGELOG.md, locate the repeated headings (match the
exact heading text shown), merge the bullet lists from duplicate occurrences
into a single instance of each heading (preserve item content and order or
dedupe if identical), remove the extra duplicate heading blocks so that each of
"### Breaking Changes", "### Bug Fixes", and "### Documentation" appears only
once under "## [Unreleased]", and run the markdown linter to confirm MD024 is
resolved.
---
Duplicate comments:
In `@packages/core/src/runtime/commit.ts`:
- Around line 1049-1059: readCompositeColorTokens currently calls the
user-provided composite.getColorTokens(theme) without guarding against thrown
exceptions or null/undefined returns; wrap that call in a try/catch and treat
any thrown error as a user-code failure by returning the sentinel
ZRUI_USER_CODE_THROW (or the existing runtime fallback), and if the callback
returns null/undefined fall back to theme.definition.colors (or
composite.colorTokens/defaultTheme.definition.colors) so commit never lets
exceptions escape; apply the same hardening pattern to all other places that
invoke composite.getColorTokens (including the other call sites around
currentCompositeTheme and the commit call-paths noted) so every
composite.getColorTokens invocation is exception- and null-safe.
In `@packages/core/src/runtime/widgetMeta.ts`:
- Around line 498-499: CollectedTrap currently has an optional kind field but
some collection paths (the single-pass collector and other emitters) still
produce traps without kind; make kind required on the CollectedTrap type and
ensure every place that constructs or emits a CollectedTrap supplies a valid
kind ("focusTrap" or "modal"). Update the CollectedTrap interface/typedef to
remove the optional marker, then audit and modify the emitters/constructors
including collectFocusTraps() and the single-pass collector to pass the
appropriate kind value when creating the trap objects so all collection paths
produce the same shape.
---
Nitpick comments:
In `@packages/core/src/app/createApp.ts`:
- Around line 921-925: The assertLifecycleIdle function duplicates
lifecycle-busy checks already enforced by assertOperational (and where
applicable assertKeybindingMutationAllowed); remove the per-method calls to
assertLifecycleIdle and rely on the existing guards instead—i.e., delete
redundant assertLifecycleIdle(...) invocations in the affected methods and
ensure each method calls assertOperational or assertKeybindingMutationAllowed as
appropriate (look for usages around the symbol assertLifecycleIdle and the
methods near lines referenced, and keep only the single existing guard such as
assertOperational/assertKeybindingMutationAllowed to prevent lifecycle
mutations).
In `@packages/core/src/app/widgetRenderer.ts`:
- Line 3281: The early guard using this.forceFullRenderOnNextSubmit (see the
return at the top of the render path) is redundant because the field is reset
earlier in the submit flow before shouldAttemptIncrementalRender() is evaluated;
consolidate the gating by removing the early return and centralizing the
decision inside shouldAttemptIncrementalRender() (or vice‑versa), and ensure the
reset of forceFullRenderOnNextSubmit happens only after the centralized decision
is made so the flag is effective; update references in
shouldAttemptIncrementalRender(), submit handling, and any reset code
(forceFullRenderOnNextSubmit reset) to use that single decision point.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f15a6d5-4f91-4b66-9e39-cae224c7365a
📒 Files selected for processing (17)
CHANGELOG.mdpackages/core/src/app/__tests__/keybindings.api.test.tspackages/core/src/app/createApp.tspackages/core/src/app/widgetRenderer.tspackages/core/src/forms/useForm.tspackages/core/src/index.tspackages/core/src/keybindings/__tests__/keybinding.modes.test.tspackages/core/src/keybindings/manager.tspackages/core/src/runtime/__tests__/focus.layers.test.tspackages/core/src/runtime/__tests__/focus.traps.test.tspackages/core/src/runtime/commit.tspackages/core/src/runtime/widgetMeta.tspackages/core/src/widgets/__tests__/composition.animationOrchestration.test.tspackages/core/src/widgets/__tests__/composition.animationPlayback.test.tspackages/core/src/widgets/__tests__/composition.useAnimatedValue.test.tspackages/core/src/widgets/__tests__/modal.focus.test.tspackages/core/src/widgets/types.ts
💤 Files with no reviewable changes (3)
- packages/core/src/widgets/tests/composition.animationOrchestration.test.ts
- packages/core/src/forms/useForm.ts
- packages/core/src/widgets/tests/composition.useAnimatedValue.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/widgets/tests/modal.focus.test.ts
- packages/core/src/widgets/types.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ThemeDefinitionthe only supported app/runtime theming contractWhat changed
colors,spacing,focusIndicator, andwidgetpalettesTextStyle/ override guidanceValidation
npm run build -- --forcenode scripts/run-tests.mjsnpm run build:nativenode scripts/frame-audit-report.mjs /tmp/rezi-frame-audit.ndjson --latest-pidrecords=10923backend_submitted=520worker_completed=520hash_mismatch_backend_vs_worker=0Notes
Theme/createThemeusage is removed in favor ofThemeDefinition.Summary by CodeRabbit
Release Notes
Breaking Changes
New Features
Bug Fixes
Documentation