feat(constraints): helper-first constraints integration#234
Merged
RtlZeroMemory merged 8 commits intomainfrom Mar 3, 2026
Merged
feat(constraints): helper-first constraints integration#234RtlZeroMemory merged 8 commits intomainfrom
RtlZeroMemory merged 8 commits intomainfrom
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:
📝 WalkthroughWalkthroughAdds a helper-first constraint system: a frozen/parsable expr DSL with LRU caching, AST types and parser, graph builder (cycle detection/fingerprinting), resolver with caching and sibling aggregations, deep integration into layout/render pipeline, expanded layout types/validation, extensive tests/docs/templates, and a codemod + template checks to migrate legacy patterns. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Renderer as WidgetRenderer
participant Parser as ExprCache/Parser
participant Graph as GraphBuilder
participant Resolver as ConstraintResolver
participant Layout as LayoutEngine
Client->>Renderer: submitFrame(runtime root with constraint props)
Renderer->>Parser: expr(source) lookup / parse
Parser-->>Renderer: ConstraintExpr (cached AST)
Renderer->>Graph: buildConstraintGraph(runtimeRoot)
Graph-->>Renderer: ConstraintGraph or fatal error
Renderer->>Resolver: resolveConstraints(graph, viewport/parent/intrinsics)
Resolver-->>Renderer: ResolvedConstraintValues (cacheHit?)
Renderer->>Renderer: applyConstraintOverridesToVNode()
Renderer->>Layout: measure/layout using resolved values
Layout-->>Renderer: final rects
Renderer->>Client: rendered frame and diagnostics
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 229002d794
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (23)
packages/core/src/layout/__tests__/layout.edgecases.test.ts (1)
294-332: Test logic and expectations look correct.The migration from percentage-based heights to fixed pixel heights correctly tests constraint slot allocation with sparse children. The layout math checks out:
- Container height 10, gap 1, two children requesting height 5
- Second child is correctly clamped to height 4 (remaining space after first child + gap)
One minor naming inconsistency: the variable
sparsePercentColumnon line 312 still references "Percent" even though the test now uses fixed heights. Consider renaming tosparseFixedColumnfor consistency with the updated test name.✏️ Optional: rename variable for consistency
- const sparsePercentColumn = { + const sparseFixedColumn = { kind: "column", props: { height: 10, gap: 1 }, children: Object.freeze([ { kind: "box", props: { border: "none", height: 5 }, children: Object.freeze([]) }, undefined, { kind: "box", props: { border: "none", height: 5 }, children: Object.freeze([]) }, ]), } as unknown as VNode; - const columnLayout = layout(sparsePercentColumn, 0, 0, 3, 10, "column"); + const columnLayout = layout(sparseFixedColumn, 0, 0, 3, 10, "column");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/__tests__/layout.edgecases.test.ts` around lines 294 - 332, Rename the test variable sparsePercentColumn to sparseFixedColumn and update all references to it in this test (the object declaration and the subsequent usage in the layout(...) call and any related assertions) so the variable name matches the test intent and the fixed-height inputs; locate the declaration of sparsePercentColumn and the const columnLayout = layout(sparsePercentColumn, ...) and change both to sparseFixedColumn.packages/core/src/layout/engine/intrinsic.ts (1)
175-180: Consistent application of the finite-check pattern.Good to see the same robust validation applied to the max-content path. Both
measureLeafMinContentandmeasureLeafMaxContentnow handle edge cases identically.♻️ Optional: Extract common pattern to a helper
If this pattern appears elsewhere or you anticipate reuse, consider a small helper:
function resolveFiniteMaxWidth(raw: unknown): number | undefined { return typeof raw === "number" && Number.isFinite(raw) ? raw : undefined; }Then both call sites become:
const maxWidth = resolveFiniteMaxWidth(propsRes.value.maxWidth);This is minor given only two usages, so feel free to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/engine/intrinsic.ts` around lines 175 - 180, The numeric-finiteness check used to normalize propsRes.value.maxWidth should be extracted into a small helper (e.g., resolveFiniteMaxWidth) and used from both measureLeafMaxContent and measureLeafMinContent to avoid duplication; implement resolveFiniteMaxWidth(raw: unknown): number | undefined that returns the number when typeof raw === "number" && Number.isFinite(raw) else undefined, then replace the current inline checks in measureLeafMaxContent and measureLeafMinContent with const maxWidth = resolveFiniteMaxWidth(propsRes.value.maxWidth) and keep the subsequent capped logic unchanged.packages/core/src/layout/__tests__/layout.percentage.test.ts (1)
5-5: Prefer reusing the sharedAxistype alias from layout types.Keeping a local duplicate type works, but importing the canonical
Axisalias avoids drift if the union evolves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/__tests__/layout.percentage.test.ts` at line 5, Replace the local duplicate type alias Axis with the shared canonical Axis export: remove the local `type Axis = "row" | "column";` and import the existing `Axis` type from the shared layout types module, then update the test to use the imported `Axis` symbol (ensuring the import name matches the exported alias) so the test reuses the central definition.packages/create-rezi/templates/starship/src/screens/cargo.ts (2)
72-72: Use theclamphelper for consistency.Line 72 manually implements clamping with
Math.max(12, Math.min(24, chartWidth - 14)), but aclamphelper was just added above. Use it for consistency and clarity.♻️ Proposed fix
- const nameWidth = Math.max(12, Math.min(24, chartWidth - 14)); + const nameWidth = clamp(chartWidth - 14, 12, 24);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-rezi/templates/starship/src/screens/cargo.ts` at line 72, Replace the manual clamp expression that computes nameWidth with the shared clamp helper: locate the calculation of nameWidth (const nameWidth = Math.max(12, Math.min(24, chartWidth - 14));) and change it to call clamp(chartWidth - 14, 12, 24) so it uses the existing clamp utility for consistency and clarity; ensure you import or reference the same clamp helper used above in this file if necessary.
25-29: Consider importingclampfrom../helpers/layout.tsto avoid local duplication.The
clamputility is already defined in the same template's helpers module and is duplicated across multiple screen files. Importing it from the shared helpers would reduce code duplication and maintain a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-rezi/templates/starship/src/screens/cargo.ts` around lines 25 - 29, Remove the locally defined clamp function and import the shared clamp implementation from the template's helpers (layout) module instead; update the top-of-file imports to pull in clamp and delete the local function definition (the symbol to remove is clamp), leaving all existing usages unchanged so they reference the imported clamp.packages/create-rezi/templates/animation-lab/src/screens/reactor-lab.ts (2)
18-60: Consider documenting why literal colors are used here.The Animation Lab template uses literal
rgb()values and hex strings for palette colors rather than semantic design tokens. While coding guidelines prefer semantic tokens, this appears intentional for demonstrating specific visual effects.Consider adding a brief comment at the
PALETTESdefinition explaining this is a demo showcase requiring precise color control, so future maintainers understand the deviation from the token-based approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-rezi/templates/animation-lab/src/screens/reactor-lab.ts` around lines 18 - 60, Add a short explanatory comment above the PALETTES constant (and mention the Palette type) stating that literal rgb() and hex color values are intentionally used for the Animation Lab demo to preserve exact visual effects and transitions, and that this is a deliberate deviation from semantic design tokens for showcase purposes; keep the comment brief and include guidance for future maintainers about not converting these palettes to tokens unless the visual fidelity is re-validated.
723-725: Consider usingui.page()for the root container.The coding guidelines specify that root views should use
ui.page()orui.appShell()rather than a bareui.column(). While the current implementation correctly includesp: 1for terminal edge padding, wrapping inui.page()would provide additional semantic structure and potential future layout features.♻️ Optional: Wrap in ui.page()
export function renderReactorLab(state: AnimationLabState): VNode { - return ui.column({ key: "root", p: 1, gap: 1 }, [ + return ui.page({ key: "root", p: 1, gap: 1, body: [ ui.row({ key: "brand-row", gap: 1, wrap: true }, [ // ... existing content ]), // ... rest of content - ]); + ]}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-rezi/templates/animation-lab/src/screens/reactor-lab.ts` around lines 723 - 725, Change the root container in renderReactorLab to use ui.page() instead of ui.column(): locate the renderReactorLab(AnimationLabState) function and replace the outer ui.column({ key: "root", p: 1, gap: 1 }, ...) with ui.page({ key: "root", p: 1 }, ...) (or ui.appShell(...) if you prefer app shell semantics), preserving the inner children (brand-row row and others) and existing padding prop so the layout and terminal edge padding remain unchanged.packages/node/src/__tests__/worker_integration.test.ts (1)
49-65: Consider extracting shared worker entry resolution logic.This duplicates
resolveWorkerEntryfromnodeBackend.ts. While acceptable for test isolation, you could extract this to a shared internal module to avoid drift between implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/node/src/__tests__/worker_integration.test.ts` around lines 49 - 65, The resolveWorkerEntry function in the test duplicates logic from nodeBackend.ts; extract the logic into a new shared internal module (e.g., export function resolveWorkerEntry from an internal util file) and replace the duplicate implementations by importing that exported resolveWorkerEntry in both packages/node/src/__tests__/worker_integration.test.ts and nodeBackend.ts; ensure the new shared function preserves the same signature and behavior (returning { entry: URL, options: WorkerOptions } and throwing the same error message) and update imports accordingly so both files reference the single source of truth.packages/create-rezi/templates/starship/src/screens/crew.ts (1)
335-386: Replace nested view-tree ternaries with core conditional helpers.This layout branch is still composed with nested JS ternaries. Please switch this section to
show()/when()/match()composition for guideline compliance and easier maintenance.As per coding guidelines, "Use
show(),when(),maybe(), andmatch()from core for conditional rendering" and "never use JavaScript ternaries or&&operators in the view tree".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-rezi/templates/starship/src/screens/crew.ts` around lines 335 - 386, The current deckLayout uses nested JavaScript ternaries (checking layout.wide and showDetailPane) which violates the view-tree guideline; replace this logic by composing core conditional helpers (show/when/match) so the tree is explicit and declarative: locate deckLayout and use match(layout.wide) or when(layout.wide) to branch wide vs narrow, and inside each branch use show(showDetailPane) or maybe/when to choose between the master-detail composition (ui.row with the conditionalConstraints.ifThenElse viewportWidthAtLeast(120) width for the manifest box and the detailPanel box) and the single manifestBlock fallback; keep the same ui.row/ui.column/manifestBlock/detailPanel structure and preserve properties (border, p, flex, overflow, width/height/minHeight) while removing all ternary operators.packages/create-rezi/templates/starship/src/helpers/layout.ts (1)
23-29: Function name misleads:toPositiveIntcan return negative integers.For negative input like
-1.5,Math.ceilreturns-1, which is not positive. While downstreamclampcalls rescue this, the function name creates a false expectation.Consider renaming to
toIntor handling negatives explicitly:🔧 Option: Rename or guard negative case
-function toPositiveInt(value: number, fallback: number): number { +function toInt(value: number, fallback: number): number { if (typeof value !== "number" || !Number.isFinite(value)) { return fallback; } - if (value >= 0) return Math.floor(value); - return Math.ceil(value); + return Math.trunc(value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-rezi/templates/starship/src/helpers/layout.ts` around lines 23 - 29, The function toPositiveInt is misleading because it can return negative values; rename the function to toInt (update its declaration and all callers) or alter its logic to guarantee non-negative results; specifically update the function named toPositiveInt in layout.ts to either (a) be renamed to toInt and keep current behavior, and update every reference (e.g., where clamp is used) to call toInt, or (b) change the implementation to return Math.max(0, Math.floor(value)) / Math.max(0, Math.ceil(value)) for negative inputs so it always returns a non-negative integer, and adjust any callers accordingly (ensure references to toPositiveInt are replaced with the new name if renaming).scripts/migrate-constraints-to-helpers.mjs (2)
66-70: Remove redundant identity map.The
.map((e) => e)on line 69 has no effect and can be removed.♻️ Proposed fix
function collectFilesRecursiveSorted(dir) { const out = []; const entries = readdirSync(dir, { withFileTypes: true }) - .map((e) => e) .sort((a, b) => a.name.localeCompare(b.name));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/migrate-constraints-to-helpers.mjs` around lines 66 - 70, In collectFilesRecursiveSorted, remove the redundant identity mapping in the pipeline: drop the .map((e) => e) call so the entries assignment reads from readdirSync(...).sort(...); adjust the chaining around entries (used later in collectFilesRecursiveSorted) if needed to ensure entries remains an array of Dirent objects; no other logic changes required.
302-325: Regex may not handle all edge cases in expr() calls.The regex
/\bexpr\s*\(\s*(['"])([^'"\\]*(?:\\.[^'"\\]*)*)\1\s*\)/gmuhandles basic escaped characters but won't correctly match:
- Nested parentheses within the string
- Template literals (backticks)
- Strings containing the same quote type even when escaped in complex patterns
For a conservative codemod this is acceptable, but consider documenting these limitations or adding a note when unmatched
expr(patterns remain in a file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/migrate-constraints-to-helpers.mjs` around lines 302 - 325, The regex exprCallRegex used to find expr(...) calls can miss edge cases (nested parentheses, backtick template literals, complex escaped quotes); update the scanning logic in the migrate script to detect any remaining unmatched "expr(" occurrences and add a note when they exist: after the current regex loop (which uses exprCallRegex, replacementForExpr, replacements, neededImports, notes and the variable text) run a conservative fallback search for the literal substring "expr(" and if any occurrences are not covered by the replacements array, push a descriptive note into notes (or otherwise record it) indicating there may be unhandled/complex expr() calls to review manually so these edge cases are surfaced without changing the existing regex behavior.packages/core/src/layout/types.ts (1)
1-10: Import placement before module doc comment.The import on line 1 appears before the module documentation comment (lines 2-9). While this works, conventionally the module doc comment comes first. This is a minor style nit.
💅 Suggested reordering
+/** + * packages/core/src/layout/types.ts — Layout primitive type definitions. + * + * Why: Defines the fundamental geometric types used throughout the layout + * system. All coordinates are in terminal cell units (not pixels). + * + * `@see` docs/guide/layout.md + */ + import type { ConstraintExpr } from "../constraints/types.js"; -/** - * packages/core/src/layout/types.ts — Layout primitive type definitions. - * - * Why: Defines the fundamental geometric types used throughout the layout - * system. All coordinates are in terminal cell units (not pixels). - * - * `@see` docs/guide/layout.md - */ import type { FluidValue } from "./responsive.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/types.ts` around lines 1 - 10, The module doc comment should appear before any imports; move the comment block starting with "/**\n * packages/core/src/layout/types.ts — Layout primitive type definitions." to the very top of the file so it precedes the imports for ConstraintExpr and FluidValue (the import lines referencing "../constraints/types.js" and "./responsive.js"), keeping the same text and spacing and leaving the import statements unchanged otherwise.packages/core/src/app/runtimeBreadcrumbs.ts (1)
111-112: Consider simplifying the optional nullable pattern.The field
constraints?: RuntimeBreadcrumbConstraintsSummary | nullcombines optionality (?) with explicitnull. This means consumers must handle three states:undefined,null, and a value. If bothundefinedandnullrepresent "no constraints," consider using just one form for clarity.💡 Suggested simplification
- constraints?: RuntimeBreadcrumbConstraintsSummary | null; + constraints: RuntimeBreadcrumbConstraintsSummary | null;This makes the field always present (like other fields in the snapshot), simplifying consumer checks to just
snapshot.constraints !== null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/runtimeBreadcrumbs.ts` around lines 111 - 112, The field declaration combines optionality and nullability — change the property from optional nullable ("constraints?: RuntimeBreadcrumbConstraintsSummary | null") to a non-optional nullable ("constraints: RuntimeBreadcrumbConstraintsSummary | null") so consumers only handle two states (value or null); update any places that construct or spread this snapshot type (factories/constructors/serializers) to always include constraints (use null when absent) and adjust related type usages to expect constraints to be present but possibly null.packages/create-rezi/templates/starship/src/screens/engineering.ts (1)
32-36: Extract clamp to a shared, exported utility to reduce duplication across the template.The
clamphelper is currently defined in at least 8 separate files (primitives.ts, crew.ts, cargo.ts, engineering.ts, bridge.ts, and helpers/formatters.ts, helpers/layout.ts, helpers/state.ts), each with an identical implementation. To reduce code duplication and improve maintainability, consider exportingclampfrom a central location (e.g.,helpers/utils.tsorprimitives.ts) and importing it across all files that need it. Note thatclampis not currently exported from primitives.ts, so a dedicated shared utility would be needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/create-rezi/templates/starship/src/screens/engineering.ts` around lines 32 - 36, Extract the clamp function into a single shared, exported utility (e.g., create a helpers/utils.ts that exports function clamp(value: number, min: number, max: number): number) and replace the duplicate local definitions by importing that exported clamp wherever it currently appears; ensure the exported signature matches the existing implementation, remove the duplicate local clamp declarations (e.g., in primitives.ts, crew.ts, cargo.ts, engineering.ts, bridge.ts, helpers/formatters.ts, helpers/layout.ts, helpers/state.ts) and update their imports to use the new helper.packages/core/src/constraints/graph.ts (2)
104-110: Duplicate helper functions.
makeNodeKeyandmakeInstancePropKeyhave identical implementations. Consider consolidating to a single function to reduce surface area and maintenance burden.♻️ Suggested consolidation
function makeNodeKey(instanceId: InstanceId, prop: ConstraintNodeProp): string { return `${String(instanceId)}:${prop}`; } -function makeInstancePropKey(instanceId: InstanceId, prop: ConstraintNodeProp): string { - return `${String(instanceId)}:${prop}`; -}Then replace all
makeInstancePropKeycalls withmakeNodeKey.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/constraints/graph.ts` around lines 104 - 110, The two helpers makeNodeKey and makeInstancePropKey are identical; remove makeInstancePropKey and consolidate to a single function (keep makeNodeKey) that takes (instanceId: InstanceId, prop: ConstraintNodeProp) and returns the same `${String(instanceId)}:${prop}`; update all usages to call makeNodeKey, remove the duplicate function declaration, and update any exports/exports list or tests that referenced makeInstancePropKey to prevent broken imports while keeping types (InstanceId, ConstraintNodeProp) intact.
122-135: VerifyRefProptype coverage in switch statement.The switch handles
"w","h","min_w","min_h"but silently defaults to"width"for any other value. Based on the types.ts snippet,RefProp = "w" | "h" | "min_w" | "min_h"which means all cases are covered. However, the default case returning"width"could mask future additions toRefProp.Consider using an exhaustive switch pattern to catch missing cases at compile time:
♻️ Exhaustive switch pattern
function refPropToConstraintProp(prop: RefProp): ConstraintNodeProp { switch (prop) { case "w": return "width"; case "h": return "height"; case "min_w": return "minWidth"; case "min_h": return "minHeight"; - default: - return "width"; } + // Exhaustive check - TypeScript will error if RefProp gains new values + const _exhaustive: never = prop; + return _exhaustive; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/constraints/graph.ts` around lines 122 - 135, The function refPropToConstraintProp currently returns a default "width" for unknown RefProp values which can mask future enum additions; update refPropToConstraintProp to use an exhaustive switch by removing the default branch and adding a final throw (or assertUnreachable) for unknown values so the compiler flags unhandled RefProp variants (reference RefProp and ConstraintNodeProp and the refPropToConstraintProp function to locate the change).docs/guide/performance-with-constraints.md (1)
36-36: Minor wording polish for readability.Line 36 reads a bit awkwardly; “a few discrete breakpoints” is slightly cleaner than “a small number of discrete breakpoints.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide/performance-with-constraints.md` at line 36, Replace the phrase "a small number of discrete breakpoints" with "a few discrete breakpoints" in the sentence that reads "Use `steps(...)` or a small number of discrete breakpoints rather than many micro-variants." to improve readability; update the text so it reads "Use `steps(...)` or a few discrete breakpoints rather than many micro-variants."docs/getting-started/constraints-quickstart.md (1)
35-67: Useui.page()(orui.appShell()) as the root in the quickstart example.Starting the example from a bare
ui.row()teaches a non-canonical root pattern.✏️ Suggested snippet adjustment
-ui.row({ gap: 1, width: "full", height: "full" }, [ - // sidebar/main/rail... -]) +ui.page({ + p: 1, + body: ui.row({ gap: 1, width: "full", height: "full" }, [ + // sidebar/main/rail... + ]), +})As per coding guidelines: Root view MUST use
ui.page()orui.appShell()and root MUST havep: 1minimum padding from terminal edges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/getting-started/constraints-quickstart.md` around lines 35 - 67, Replace the bare ui.row() root with a canonical root view by wrapping it in ui.page() (or ui.appShell()), ensure the root call includes p: 1 to satisfy the minimum padding requirement, and keep the existing ui.row(...) as the child layout passed into the page/appShell; locate the ui.row(...) snippet in the example and change the top-level element to ui.page({... p: 1 ...}, [ ui.row(... ) ]) (or equivalent ui.appShell) so the quickstart demonstrates the required root pattern.docs/guide/constraints.md (1)
3-3: Align opening sentence with helper-first contract.The intro currently frames
expr("...")as canonical, which conflicts with the helper-first guidance used throughout the rest of this PR.✏️ Suggested wording
-Rezi's constraint DSL (`expr("...")`) is the canonical way to express derived layout relationships in the breaking alpha branch. +Rezi’s helper-first constraints are the canonical way to express derived layout relationships in the breaking alpha branch; use `expr("...")` as the advanced escape hatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide/constraints.md` at line 3, Update the opening sentence to follow the helper-first contract used across the PR: instead of calling expr("...") the "canonical" way, describe expr("...") as an available escape/higher-power for complex derived layout relationships while recommending the helper-first approach and helper functions as the preferred initial pattern; reference the DSL symbol expr("...") and the "helper-first" guidance in the single introductory line so readers know helpers are primary and expr is for edge cases.packages/core/src/layout/engine/guards.ts (1)
63-73: Avoid no-op percent guards that always returnfalse.These helpers now silently disable local detection. If any caller relies on them for fail-fast validation, invalid
%usage won’t be caught at that stage.🔧 Suggested defensive implementation
export function childHasPercentInMainAxis(vnode: unknown, axis: Axis): boolean { - void vnode; - void axis; - return false; + if (!isVNode(vnode)) return false; + const p = getConstraintProps(vnode); + if (!p) return false; + const size = axis === "row" ? p.width : p.height; + return typeof size === "string" && size.trim().endsWith("%"); } export function childHasPercentInCrossAxis(vnode: unknown, axis: Axis): boolean { - void vnode; - void axis; - return false; + if (!isVNode(vnode)) return false; + const p = getConstraintProps(vnode); + if (!p) return false; + const size = axis === "row" ? p.height : p.width; + return typeof size === "string" && size.trim().endsWith("%"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/engine/guards.ts` around lines 63 - 73, The two guard functions childHasPercentInMainAxis and childHasPercentInCrossAxis currently return false unconditionally, which disables percent-detection; replace the no-op implementations with real checks that inspect the vnode (and its style/layout props) for percentage values on the appropriate axis and return true if any child or relevant prop uses a '%' value; specifically, in childHasPercentInMainAxis(vnode, axis) check width/height or main-axis size/style properties (depending on Axis) on the vnode and its children for string values ending with '%' or numeric percent representations, and implement childHasPercentInCrossAxis(vnode, axis) similarly for the cross-axis properties so callers relying on these guards receive accurate boolean results.packages/core/src/layout/kinds/leaf.ts (1)
45-54: Consider the fallthrough case behavior.Line 53 returns
0for unrecognized values (e.g., malformed strings or unexpected types). While this is defensive, it could silently produce zero-sized widgets that are invisible but still in the tree.If this is intentional fallback behavior, this is fine. If validation is expected to catch such cases earlier, consider whether logging a dev warning would help surface misconfiguration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/kinds/leaf.ts` around lines 45 - 54, The function resolveLeafSizeConstraint currently falls through to return 0 for any unrecognized resolved value; update resolveLeafSizeConstraint to surface unexpected/malformed inputs by emitting a dev-time warning (e.g., via an existing devWarning utility or console.warn) when resolved is neither a finite number nor one of "full"|"auto"|undefined before returning 0; reference resolveLeafSizeConstraint and resolveResponsiveValue so the check runs after calling resolveResponsiveValue and include the offending resolved value in the warning message to aid debugging.packages/core/src/app/widgetRenderer.ts (1)
2714-2760: Consider documenting the pooled Set assignment pattern.Lines 2758-2759 assign pooled Sets directly to instance fields rather than copying. This works because the pooled sets are cleared at the start of this method and not reused elsewhere before the next call. A brief comment would help future maintainers understand this intentional optimization.
📝 Suggested documentation
this._pooledConstraintVisibilityStack.push(hidden); } } + // Assign pooled sets directly to avoid copying; safe because they're only + // cleared/reused at the start of this method or when clearing constraint state. this._hiddenConstraintInstanceIds = this._pooledHiddenConstraintInstanceIds; this._hiddenConstraintWidgetIds = this._pooledHiddenConstraintWidgetIds; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/app/widgetRenderer.ts` around lines 2714 - 2760, In rebuildConstraintHiddenState, add a brief comment above the final assignments to explain that _pooledHiddenConstraintInstanceIds and _pooledHiddenConstraintWidgetIds are intentionally reused and swapped into _hiddenConstraintInstanceIds/_hiddenConstraintWidgetIds (rather than copied) because the pooled sets are cleared at the start of this method and not read elsewhere before the next call; reference the pooled fields (_pooledHiddenConstraintInstanceIds, _pooledHiddenConstraintWidgetIds) and the public fields they are assigned to (_hiddenConstraintInstanceIds, _hiddenConstraintWidgetIds) so future maintainers understand this optimization and its safety constraints.
🤖 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/constraints.md`:
- Line 13: The RFC link on docs/guide/constraints.md referencing "[RFC-001]"
points to a non-existent target and breaks the docs pipeline; update the link
target to the correct published path or replace it with the proper existing RFC
document reference (e.g., correct RFC file name or URL) so the link resolves in
the built docs and re-run validation; locate the link text "[RFC-001]" in
constraints.md and change its href to the valid RFC file path or remove the link
if no corresponding RFC should be referenced.
In `@docs/widgets/grid.md`:
- Line 71: The CPU value in the grid example uses ui.text("42") but should
include the percent unit for consistency with other metrics; update the example
so the CPU cell uses ui.text("42%") (locate the ui.text call that renders the
CPU metric in the grid example) to restore the percent sign.
In `@mkdocs.yml`:
- Around line 141-144: Consolidate the duplicated navigation "Reference" entries
in mkdocs.yml by removing the redundant Reference block (the one that repeats
Glossary) so there is only a single "Reference:" section; keep the canonical
block that contains the entries "Glossary", "Constraints API", and "Constraint
Expressions" and remove the duplicate Reference section and duplicate "Glossary"
entry so navigation contains one unified Reference list.
In `@packages/core/src/__tests__/constraints/graph.test.ts`:
- Around line 6-11: The ESM bootstrap uses dynamicImport("./graph.test.impl.js")
which doesn't exist while the CJS branch uses "./graph.test.impl.ts"; change the
ESM import to "./graph.test.impl.ts" so both branches target the same
implementation file, and when catching the import error preserve and include the
original error message in the thrown Error (keep the existing "Failed to load
graph constraint test bootstrap." text but include the caught error details) for
better diagnostics; locate the dynamicImport calls in this test file
(dynamicImport("tsx") and the subsequent .then/.catch chain) and update the ESM
path and error construction accordingly.
In `@packages/core/src/__tests__/constraints/helpers.test.ts`:
- Around line 5-12: The test file uses a fire-and-forget Promise chain started
by the dynamicImport function which creates a race where helpers.test.impl.js
may not register its node:test describe()/test() handlers before module
evaluation finishes; replace the Promise.then/.catch chain with top-level
awaits: await dynamicImport("tsx") followed by await
dynamicImport("./helpers.test.impl.js") so the module loading is sequential and
failures propagate immediately (remove the process.nextTick throw pattern), and
apply the same change to the other constraint test files using dynamicImport.
In `@packages/core/src/constraints/resolver.ts`:
- Around line 394-400: The cache key generation for resolution (variable
cacheKey created from options.cacheKey ??
createResolutionCacheKey(graph.fingerprint, options.viewport, options.parent))
omits per-instance inputs (baseValues, parentValues, intrinsicValues) and can
return stale results; update the logic that builds the cache key (either by
extending createResolutionCacheKey or composing a new serialized key) to
incorporate options.baseValues, options.parentValues, and
options.intrinsicValues (serializing deterministic maps/objects) along with
graph.fingerprint, options.viewport, and options.parent so cached entries are
invalidated per-instance; make the same change for the second cacheKey use site
(the other occurrence in this file) to ensure both cache lookups include these
dynamic inputs.
In `@packages/core/src/layout/__tests__/layout.flex-shrink.test.ts`:
- Around line 97-108: Tests using the mustRow helper should be migrated to the
repo's test harness: replace calls to mustRow(...) with creating a renderer via
createTestRenderer({ viewport: { width: 20 } }), render the same UI tree (the
two ui.box(...) children with flex/flexBasis) using renderer.render(...), then
query renderer.root.children to compute widths for assertions (same expected
[12, 8]); apply the same migration for the other test at lines 125–128 so both
tests use createTestRenderer instead of direct layout/mustRow calls and keep the
same assertions and viewport configuration.
In `@packages/core/src/layout/__tests__/layout.percentage.test.ts`:
- Line 3: Replace direct calls to layout() and measure() in
layout.percentage.test.ts with the testing harness: use createTestRenderer() to
render components with the viewport helper, then query nodes via
renderer.findById()/findText()/findAll() and assert sizes using
renderer.toText() or measured layout properties; preserve the existing
assertions about legacy prop rejection by rendering a node that uses the legacy
prop and asserting the renderer throws or returns the expected rejection
message. Locate usages of layout and measure in the test and refactor them to
createTestRenderer(), use the renderer instance for querying and asserting
viewport-based measurements, and keep the legacy-prop check logic but driven
through the test renderer APIs.
In `@packages/core/src/layout/constraints.ts`:
- Around line 91-103: readOptionalNonNegative currently returns
Math.floor(resolved) but allows negative numbers from expression-resolved or
numeric inputs to pass; change the final returned value to be clamped to
non-negative (e.g. Math.max(0, Math.floor(resolvedValueOrResolved))) so any
negative numbers become 0, and apply the same clamp to the analogous function
around the other block that uses resolveResponsiveValue and isConstraintExpr
(the block at ~142-145) so both numeric and resolved values are floored then
clamped to >=0.
In `@packages/core/src/layout/engine/layoutEngine.ts`:
- Around line 544-548: The code zeroes rectW/rectH when display is false but
still continues into kind-specific layout dispatch, causing unnecessary
recursion and possible hidden-descendant leaks; modify the layout flow in
layoutEngine so that when hiddenByDisplay (the computed flag using
vnode.props.display) is true you short-circuit immediately after computing
rectW/rectH (and any associated sizeRes/forcedW/forcedH/maxW/maxH handling),
return/apply the zero size result and skip all widget-specific layout dispatch
and child recursion (i.e., do not call the kind-specific layout handlers or
recurse into children), while preserving the existing invariants used by
clampNonNegative, sizeRes, and the reconciliation path.
In `@packages/create-rezi/templates/starship/src/screens/crew.ts`:
- Around line 351-356: The inline comment for the responsive width is
inaccurate: the else branch of the width assignment uses a fixed 100 (columns)
not "full". Update the code so intent and implementation match by either
changing the fallback value in the width property to the string "full" (replace
the numeric 100) or adjust the comment to say "narrow gets fixed 100 columns";
locate the width assignment using conditionalConstraints.ifThenElse and
visibilityConstraints.viewportWidthAtLeast to make the change.
In `@packages/create-rezi/templates/starship/src/screens/shell.ts`:
- Around line 156-160: The right-rail UI is currently gated by options.rightRail
instead of the computed predicate showRightRail (and doesn't respect
rightRailDisplay), which can leave an empty reserved rail; update the rendering
logic around rightRailNode (and the row branch that renders the flex-1 rail
wrapper) to only render the rail wrapper and rightRailNode when both
showRightRail and rightRailDisplay are truthy (i.e., replace checks of
options.rightRail with showRightRail and add the rightRailDisplay conjunction),
so the flex-1 rail wrapper is not allocated when the rail is hidden by
constraints or layout.hideNonCritical.
In `@packages/node/src/__tests__/config_guards.test.ts`:
- Around line 186-197: The test named "config guard: native worker mode remains
inline even when worker is requested" is a duplicate of the earlier "native
worker mode falls back to inline by default in TTY" test; either remove this
duplicate test or change its inputs to exercise a different branch (for example
call selectNodeBackendExecutionMode with requestedExecutionMode: "auto" instead
of "worker" and update the expected resolvedExecutionMode/selectedExecutionMode
accordingly). Locate the test using the test name and the call to
selectNodeBackendExecutionMode and modify or delete it so each test covers a
unique scenario.
In `@scripts/check-create-rezi-templates.mjs`:
- Around line 30-35: The two guardrail regex entries in
scripts/check-create-rezi-templates.mjs (the first regex and the Object.freeze
entry with name "responsive-map layout constraint") are too narrow and miss
decimal percentages and object-mapped numeric values (e.g., width: "12.5%" or
width: { md: 20, lg: 30 }); update the regex definitions (the regex properties
inside both the standalone pattern and the Object.freeze named "responsive-map
layout constraint") to: 1) match percentage values with optional decimal
fractions (e.g., allow \d+(\.\d+)?%) and quoted or unquoted percent strings, and
2) detect object map forms where a property is assigned an object whose keys map
to numeric or percent values (e.g., :\s*{ ... key: (number or percent or
quoted-percent) ... }), so the helper-first policy cannot be bypassed by decimal
percentages or responsive-object syntax.
In `@scripts/run-tests.mjs`:
- Around line 90-97: The --filter branch currently accepts any next token
(variable v) including option-like tokens (e.g., "--scope"), so update the a ===
"--filter" block to reject such tokens: after reading v = argv[i+1], validate
that v is a string, non-empty when trimmed, and does not start with '-' (or
otherwise look like an option); if it fails, throw the same run-tests: --filter
requires a non-empty pattern string error (or a similar clear error) instead of
treating option tokens as values, then set filter = v and increment i as before.
---
Nitpick comments:
In `@docs/getting-started/constraints-quickstart.md`:
- Around line 35-67: Replace the bare ui.row() root with a canonical root view
by wrapping it in ui.page() (or ui.appShell()), ensure the root call includes p:
1 to satisfy the minimum padding requirement, and keep the existing ui.row(...)
as the child layout passed into the page/appShell; locate the ui.row(...)
snippet in the example and change the top-level element to ui.page({... p: 1
...}, [ ui.row(... ) ]) (or equivalent ui.appShell) so the quickstart
demonstrates the required root pattern.
In `@docs/guide/constraints.md`:
- Line 3: Update the opening sentence to follow the helper-first contract used
across the PR: instead of calling expr("...") the "canonical" way, describe
expr("...") as an available escape/higher-power for complex derived layout
relationships while recommending the helper-first approach and helper functions
as the preferred initial pattern; reference the DSL symbol expr("...") and the
"helper-first" guidance in the single introductory line so readers know helpers
are primary and expr is for edge cases.
In `@docs/guide/performance-with-constraints.md`:
- Line 36: Replace the phrase "a small number of discrete breakpoints" with "a
few discrete breakpoints" in the sentence that reads "Use `steps(...)` or a
small number of discrete breakpoints rather than many micro-variants." to
improve readability; update the text so it reads "Use `steps(...)` or a few
discrete breakpoints rather than many micro-variants."
In `@packages/core/src/app/runtimeBreadcrumbs.ts`:
- Around line 111-112: The field declaration combines optionality and
nullability — change the property from optional nullable ("constraints?:
RuntimeBreadcrumbConstraintsSummary | null") to a non-optional nullable
("constraints: RuntimeBreadcrumbConstraintsSummary | null") so consumers only
handle two states (value or null); update any places that construct or spread
this snapshot type (factories/constructors/serializers) to always include
constraints (use null when absent) and adjust related type usages to expect
constraints to be present but possibly null.
In `@packages/core/src/app/widgetRenderer.ts`:
- Around line 2714-2760: In rebuildConstraintHiddenState, add a brief comment
above the final assignments to explain that _pooledHiddenConstraintInstanceIds
and _pooledHiddenConstraintWidgetIds are intentionally reused and swapped into
_hiddenConstraintInstanceIds/_hiddenConstraintWidgetIds (rather than copied)
because the pooled sets are cleared at the start of this method and not read
elsewhere before the next call; reference the pooled fields
(_pooledHiddenConstraintInstanceIds, _pooledHiddenConstraintWidgetIds) and the
public fields they are assigned to (_hiddenConstraintInstanceIds,
_hiddenConstraintWidgetIds) so future maintainers understand this optimization
and its safety constraints.
In `@packages/core/src/constraints/graph.ts`:
- Around line 104-110: The two helpers makeNodeKey and makeInstancePropKey are
identical; remove makeInstancePropKey and consolidate to a single function (keep
makeNodeKey) that takes (instanceId: InstanceId, prop: ConstraintNodeProp) and
returns the same `${String(instanceId)}:${prop}`; update all usages to call
makeNodeKey, remove the duplicate function declaration, and update any
exports/exports list or tests that referenced makeInstancePropKey to prevent
broken imports while keeping types (InstanceId, ConstraintNodeProp) intact.
- Around line 122-135: The function refPropToConstraintProp currently returns a
default "width" for unknown RefProp values which can mask future enum additions;
update refPropToConstraintProp to use an exhaustive switch by removing the
default branch and adding a final throw (or assertUnreachable) for unknown
values so the compiler flags unhandled RefProp variants (reference RefProp and
ConstraintNodeProp and the refPropToConstraintProp function to locate the
change).
In `@packages/core/src/layout/__tests__/layout.edgecases.test.ts`:
- Around line 294-332: Rename the test variable sparsePercentColumn to
sparseFixedColumn and update all references to it in this test (the object
declaration and the subsequent usage in the layout(...) call and any related
assertions) so the variable name matches the test intent and the fixed-height
inputs; locate the declaration of sparsePercentColumn and the const columnLayout
= layout(sparsePercentColumn, ...) and change both to sparseFixedColumn.
In `@packages/core/src/layout/__tests__/layout.percentage.test.ts`:
- Line 5: Replace the local duplicate type alias Axis with the shared canonical
Axis export: remove the local `type Axis = "row" | "column";` and import the
existing `Axis` type from the shared layout types module, then update the test
to use the imported `Axis` symbol (ensuring the import name matches the exported
alias) so the test reuses the central definition.
In `@packages/core/src/layout/engine/guards.ts`:
- Around line 63-73: The two guard functions childHasPercentInMainAxis and
childHasPercentInCrossAxis currently return false unconditionally, which
disables percent-detection; replace the no-op implementations with real checks
that inspect the vnode (and its style/layout props) for percentage values on the
appropriate axis and return true if any child or relevant prop uses a '%' value;
specifically, in childHasPercentInMainAxis(vnode, axis) check width/height or
main-axis size/style properties (depending on Axis) on the vnode and its
children for string values ending with '%' or numeric percent representations,
and implement childHasPercentInCrossAxis(vnode, axis) similarly for the
cross-axis properties so callers relying on these guards receive accurate
boolean results.
In `@packages/core/src/layout/engine/intrinsic.ts`:
- Around line 175-180: The numeric-finiteness check used to normalize
propsRes.value.maxWidth should be extracted into a small helper (e.g.,
resolveFiniteMaxWidth) and used from both measureLeafMaxContent and
measureLeafMinContent to avoid duplication; implement resolveFiniteMaxWidth(raw:
unknown): number | undefined that returns the number when typeof raw ===
"number" && Number.isFinite(raw) else undefined, then replace the current inline
checks in measureLeafMaxContent and measureLeafMinContent with const maxWidth =
resolveFiniteMaxWidth(propsRes.value.maxWidth) and keep the subsequent capped
logic unchanged.
In `@packages/core/src/layout/kinds/leaf.ts`:
- Around line 45-54: The function resolveLeafSizeConstraint currently falls
through to return 0 for any unrecognized resolved value; update
resolveLeafSizeConstraint to surface unexpected/malformed inputs by emitting a
dev-time warning (e.g., via an existing devWarning utility or console.warn) when
resolved is neither a finite number nor one of "full"|"auto"|undefined before
returning 0; reference resolveLeafSizeConstraint and resolveResponsiveValue so
the check runs after calling resolveResponsiveValue and include the offending
resolved value in the warning message to aid debugging.
In `@packages/core/src/layout/types.ts`:
- Around line 1-10: The module doc comment should appear before any imports;
move the comment block starting with "/**\n * packages/core/src/layout/types.ts
— Layout primitive type definitions." to the very top of the file so it precedes
the imports for ConstraintExpr and FluidValue (the import lines referencing
"../constraints/types.js" and "./responsive.js"), keeping the same text and
spacing and leaving the import statements unchanged otherwise.
In `@packages/create-rezi/templates/animation-lab/src/screens/reactor-lab.ts`:
- Around line 18-60: Add a short explanatory comment above the PALETTES constant
(and mention the Palette type) stating that literal rgb() and hex color values
are intentionally used for the Animation Lab demo to preserve exact visual
effects and transitions, and that this is a deliberate deviation from semantic
design tokens for showcase purposes; keep the comment brief and include guidance
for future maintainers about not converting these palettes to tokens unless the
visual fidelity is re-validated.
- Around line 723-725: Change the root container in renderReactorLab to use
ui.page() instead of ui.column(): locate the renderReactorLab(AnimationLabState)
function and replace the outer ui.column({ key: "root", p: 1, gap: 1 }, ...)
with ui.page({ key: "root", p: 1 }, ...) (or ui.appShell(...) if you prefer app
shell semantics), preserving the inner children (brand-row row and others) and
existing padding prop so the layout and terminal edge padding remain unchanged.
In `@packages/create-rezi/templates/starship/src/helpers/layout.ts`:
- Around line 23-29: The function toPositiveInt is misleading because it can
return negative values; rename the function to toInt (update its declaration and
all callers) or alter its logic to guarantee non-negative results; specifically
update the function named toPositiveInt in layout.ts to either (a) be renamed to
toInt and keep current behavior, and update every reference (e.g., where clamp
is used) to call toInt, or (b) change the implementation to return Math.max(0,
Math.floor(value)) / Math.max(0, Math.ceil(value)) for negative inputs so it
always returns a non-negative integer, and adjust any callers accordingly
(ensure references to toPositiveInt are replaced with the new name if renaming).
In `@packages/create-rezi/templates/starship/src/screens/cargo.ts`:
- Line 72: Replace the manual clamp expression that computes nameWidth with the
shared clamp helper: locate the calculation of nameWidth (const nameWidth =
Math.max(12, Math.min(24, chartWidth - 14));) and change it to call
clamp(chartWidth - 14, 12, 24) so it uses the existing clamp utility for
consistency and clarity; ensure you import or reference the same clamp helper
used above in this file if necessary.
- Around line 25-29: Remove the locally defined clamp function and import the
shared clamp implementation from the template's helpers (layout) module instead;
update the top-of-file imports to pull in clamp and delete the local function
definition (the symbol to remove is clamp), leaving all existing usages
unchanged so they reference the imported clamp.
In `@packages/create-rezi/templates/starship/src/screens/crew.ts`:
- Around line 335-386: The current deckLayout uses nested JavaScript ternaries
(checking layout.wide and showDetailPane) which violates the view-tree
guideline; replace this logic by composing core conditional helpers
(show/when/match) so the tree is explicit and declarative: locate deckLayout and
use match(layout.wide) or when(layout.wide) to branch wide vs narrow, and inside
each branch use show(showDetailPane) or maybe/when to choose between the
master-detail composition (ui.row with the conditionalConstraints.ifThenElse
viewportWidthAtLeast(120) width for the manifest box and the detailPanel box)
and the single manifestBlock fallback; keep the same
ui.row/ui.column/manifestBlock/detailPanel structure and preserve properties
(border, p, flex, overflow, width/height/minHeight) while removing all ternary
operators.
In `@packages/create-rezi/templates/starship/src/screens/engineering.ts`:
- Around line 32-36: Extract the clamp function into a single shared, exported
utility (e.g., create a helpers/utils.ts that exports function clamp(value:
number, min: number, max: number): number) and replace the duplicate local
definitions by importing that exported clamp wherever it currently appears;
ensure the exported signature matches the existing implementation, remove the
duplicate local clamp declarations (e.g., in primitives.ts, crew.ts, cargo.ts,
engineering.ts, bridge.ts, helpers/formatters.ts, helpers/layout.ts,
helpers/state.ts) and update their imports to use the new helper.
In `@packages/node/src/__tests__/worker_integration.test.ts`:
- Around line 49-65: The resolveWorkerEntry function in the test duplicates
logic from nodeBackend.ts; extract the logic into a new shared internal module
(e.g., export function resolveWorkerEntry from an internal util file) and
replace the duplicate implementations by importing that exported
resolveWorkerEntry in both
packages/node/src/__tests__/worker_integration.test.ts and nodeBackend.ts;
ensure the new shared function preserves the same signature and behavior
(returning { entry: URL, options: WorkerOptions } and throwing the same error
message) and update imports accordingly so both files reference the single
source of truth.
In `@scripts/migrate-constraints-to-helpers.mjs`:
- Around line 66-70: In collectFilesRecursiveSorted, remove the redundant
identity mapping in the pipeline: drop the .map((e) => e) call so the entries
assignment reads from readdirSync(...).sort(...); adjust the chaining around
entries (used later in collectFilesRecursiveSorted) if needed to ensure entries
remains an array of Dirent objects; no other logic changes required.
- Around line 302-325: The regex exprCallRegex used to find expr(...) calls can
miss edge cases (nested parentheses, backtick template literals, complex escaped
quotes); update the scanning logic in the migrate script to detect any remaining
unmatched "expr(" occurrences and add a note when they exist: after the current
regex loop (which uses exprCallRegex, replacementForExpr, replacements,
neededImports, notes and the variable text) run a conservative fallback search
for the literal substring "expr(" and if any occurrences are not covered by the
replacements array, push a descriptive note into notes (or otherwise record it)
indicating there may be unhandled/complex expr() calls to review manually so
these edge cases are surfaced without changing the existing regex behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
docs/assets/constraints/animation-lab-reactor.svgis excluded by!**/*.svgdocs/assets/constraints/dashboard-inspector.svgis excluded by!**/*.svgdocs/assets/constraints/starship-shell.svgis excluded by!**/*.svg
📒 Files selected for processing (105)
CHANGELOG.mddocs/design-principles.mddocs/getting-started/constraints-quickstart.mddocs/guide/constraint-demos.mddocs/guide/constraint-recipes.mddocs/guide/constraints.mddocs/guide/debugging-constraints.mddocs/guide/debugging.mddocs/guide/layout-decision-tree.mddocs/guide/layout.mddocs/guide/migration-from-manual-layout-math.mddocs/guide/performance-with-constraints.mddocs/guide/styling-with-constraints.mddocs/packages/core.mddocs/reference/constraint-expressions.mddocs/reference/constraints-api.mddocs/widgets/grid.mddocs/widgets/index.mdmkdocs.ymlpackage.jsonpackages/core/src/__tests__/constraints/graph.test.impl.tspackages/core/src/__tests__/constraints/graph.test.tspackages/core/src/__tests__/constraints/helpers.test.impl.tspackages/core/src/__tests__/constraints/helpers.test.tspackages/core/src/__tests__/constraints/integration.test.impl.tspackages/core/src/__tests__/constraints/integration.test.tspackages/core/src/__tests__/constraints/package.jsonpackages/core/src/__tests__/constraints/parser.test.impl.tspackages/core/src/__tests__/constraints/parser.test.tspackages/core/src/__tests__/constraints/resolver.test.impl.tspackages/core/src/__tests__/constraints/resolver.test.tspackages/core/src/abi.tspackages/core/src/app/createApp.tspackages/core/src/app/runtimeBreadcrumbs.tspackages/core/src/app/widgetRenderer.tspackages/core/src/app/widgetRenderer/cursorBreadcrumbs.tspackages/core/src/app/widgetRenderer/submitFramePipeline.tspackages/core/src/constraints/aggregation.tspackages/core/src/constraints/expr.tspackages/core/src/constraints/graph.tspackages/core/src/constraints/helpers.tspackages/core/src/constraints/parser.tspackages/core/src/constraints/resolver.tspackages/core/src/constraints/types.tspackages/core/src/index.tspackages/core/src/layout/__tests__/constraints.golden.test.tspackages/core/src/layout/__tests__/incrementalLayout.phase1.test.tspackages/core/src/layout/__tests__/incrementalLayout.phase2.test.tspackages/core/src/layout/__tests__/incrementalLayout.phase3.test.tspackages/core/src/layout/__tests__/layout.absolute.test.tspackages/core/src/layout/__tests__/layout.aspect-ratio.test.tspackages/core/src/layout/__tests__/layout.auto-sizing.test.tspackages/core/src/layout/__tests__/layout.edgecases.test.tspackages/core/src/layout/__tests__/layout.feedback-loop.test.tspackages/core/src/layout/__tests__/layout.flex-distribution.test.tspackages/core/src/layout/__tests__/layout.flex-shrink.test.tspackages/core/src/layout/__tests__/layout.percentage.test.tspackages/core/src/layout/__tests__/layout.perf-invariants.test.tspackages/core/src/layout/__tests__/layout.responsive-fluid.test.tspackages/core/src/layout/__tests__/layout.stability-coverage.test.tspackages/core/src/layout/__tests__/layout.stability-signature.test.tspackages/core/src/layout/__tests__/layout.wrap.test.tspackages/core/src/layout/__tests__/spacing.test.tspackages/core/src/layout/constraints.tspackages/core/src/layout/engine/guards.tspackages/core/src/layout/engine/intrinsic.tspackages/core/src/layout/engine/layoutEngine.tspackages/core/src/layout/kinds/grid.tspackages/core/src/layout/kinds/leaf.tspackages/core/src/layout/kinds/overlays.tspackages/core/src/layout/types.tspackages/core/src/layout/validateProps.tspackages/core/src/runtime/__tests__/mouse.mixed.test.tspackages/core/src/widgets/inspectorOverlay.tspackages/core/src/widgets/types.tspackages/core/src/widgets/ui.tspackages/create-rezi/templates/animation-lab/README.mdpackages/create-rezi/templates/animation-lab/src/__tests__/reducer.test.tspackages/create-rezi/templates/animation-lab/src/__tests__/render.test.tspackages/create-rezi/templates/animation-lab/src/helpers/state.tspackages/create-rezi/templates/animation-lab/src/screens/reactor-lab.tspackages/create-rezi/templates/animation-lab/src/types.tspackages/create-rezi/templates/dashboard/src/screens/overview.tspackages/create-rezi/templates/starship/src/__tests__/render.test.tspackages/create-rezi/templates/starship/src/helpers/layout.tspackages/create-rezi/templates/starship/src/main.tspackages/create-rezi/templates/starship/src/screens/bridge.tspackages/create-rezi/templates/starship/src/screens/cargo.tspackages/create-rezi/templates/starship/src/screens/comms.tspackages/create-rezi/templates/starship/src/screens/crew.tspackages/create-rezi/templates/starship/src/screens/engineering.tspackages/create-rezi/templates/starship/src/screens/primitives.tspackages/create-rezi/templates/starship/src/screens/settings.tspackages/create-rezi/templates/starship/src/screens/shell.tspackages/jsx/src/__tests__/layout.test.tsxpackages/jsx/src/index.tspackages/native/loader.cjspackages/node/src/__tests__/config_guards.test.tspackages/node/src/__tests__/worker_integration.test.tspackages/node/src/backend/nodeBackend.tspackages/node/src/worker/engineWorker.bootstrap.jspackages/node/src/worker/engineWorker.tsscripts/check-create-rezi-templates.mjsscripts/migrate-constraints-to-helpers.mjsscripts/run-tests.mjs
💤 Files with no reviewable changes (2)
- packages/core/src/layout/tests/layout.responsive-fluid.test.ts
- packages/create-rezi/templates/animation-lab/src/types.ts
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
packages/core/src/layout/__tests__/layout.percentage.test.ts (1)
6-6: Consider importingAxisfrom the layout types module.This local type definition duplicates
Axisfrompackages/core/src/layout/types.ts. Importing it would reduce duplication and ensure consistency if the canonical type ever changes.♻️ Suggested import
import { assert, describe, test } from "@rezi-ui/testkit"; import { type VNode, ui } from "../../index.js"; import { createTestRenderer } from "../../testing/renderer.js"; -import { measure } from "../layout.js"; +import { measure, type Axis } from "../layout.js"; -type Axis = "row" | "column";If
Axisisn't re-exported fromlayout.js, import fromtypes.jsinstead:import type { Axis } from "../types.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/__tests__/layout.percentage.test.ts` at line 6, The local type alias Axis in the test duplicates the canonical Axis type; remove the inline `type Axis = "row" | "column";` and instead import the canonical type (e.g. `import type { Axis } from "../types.js";` or from `types.ts` if not re-exported) so the test uses the single source of truth (`Axis`) and stays consistent with the layout types.packages/core/src/layout/__tests__/layout.flex-shrink.test.ts (1)
10-34: Consider collapsing duplicated node-measure helpers into one generic helper.
childWidthsandchildHeightsdiffer only byrectfield access. A single helper reduces duplication and keeps future assertion helpers consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/__tests__/layout.flex-shrink.test.ts` around lines 10 - 34, Collapse the duplicated helpers childWidths and childHeights into one generic helper (e.g., getChildRectProp or childMeasure) that accepts the same out and ids parameters plus either a property key ("w" | "h") or a selector function to extract the value from node.rect; reuse createTestRenderer's render return type and the existing out.findById/null check logic, failing with assert.fail if node is missing, and return the mapped values by applying the selector to node.rect for each id.packages/core/src/layout/engine/layoutEngine.ts (1)
872-881: Deduplicate legacy guard construction across public entrypoints.Line 872 and Line 907 repeat the same legacy-check + fatal payload block. Consider extracting a small helper to keep behavior synchronized and reduce drift risk.
Also applies to: 907-916
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/engine/layoutEngine.ts` around lines 872 - 881, Duplicate legacy-check + error construction around findLegacyConstraintUsage(node) should be extracted into a small helper to avoid drift; add a function (e.g., buildLegacyConstraintError or ensureNoLegacyConstraint) that calls findLegacyConstraintUsage(node) and, if non-null, returns the exact error object { ok: false, fatal: { code: "ZRUI_INVALID_PROPS", detail: `Legacy size constraint usage detected: ${legacyUsage}` } } (or returns null/undefined when OK), then replace both repeated blocks with a call to that helper (keep the same return shape and symbol names so callers like the code currently using the inline block continue to work).scripts/check-create-rezi-templates.mjs (1)
72-74: Minor cleanup: redundant operations in recursive collector.
map((entry) => entry)and the trailingout.sort()are unnecessary given the existing sorted traversal.♻️ Proposed simplification
function walk(currentDir) { const entries = readdirSync(currentDir, { withFileTypes: true }) - .map((entry) => entry) .sort((left, right) => left.name.localeCompare(right.name)); @@ walk(dir); - out.sort(); return out; }Also applies to: 86-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-create-rezi-templates.mjs` around lines 72 - 74, Remove the redundant identity map and the unnecessary final sort: stop using .map((entry) => entry) when building entries (use readdirSync(currentDir, { withFileTypes: true }).sort(...)) and remove the trailing out.sort() that re-sorts results after the already-sorted traversal; apply the same cleanup to the other occurrence around the entries variable at lines 86–87 so the recursive collector relies solely on the sorted entries variable for deterministic ordering.packages/core/src/__tests__/constraints/resolver.test.impl.ts (1)
239-273: Add a fractional dynamic-input cache-key regression case.At Line 239-Line 273, cache-key coverage uses integer inputs only. Add one case with fractional
parentValues/intrinsicValuesto catch numeric-collision regressions.💡 Suggested additional test case
+ test("default cache key distinguishes fractional dynamic inputs", () => { + const node = boxWithId(81, "fractionalCacheNode", { width: expr("parent.w + intrinsic.w") }); + const built = buildConstraintGraph(runtimeNode(80, {}, [node])); + assert.equal(built.ok, true); + if (!built.ok) return; + + const cache = new ConstraintResolutionCache(); + const first = resolveConstraints(built.value, { + viewport: { w: 120, h: 40 }, + parent: { w: 100, h: 40 }, + parentValues: new Map([[81, { w: 80.1 }]]), + intrinsicValues: new Map([[81, { w: 1.2 }]]), + cache, + }); + const second = resolveConstraints(built.value, { + viewport: { w: 120, h: 40 }, + parent: { w: 100, h: 40 }, + parentValues: new Map([[81, { w: 80.9 }]]), + intrinsicValues: new Map([[81, { w: 1.8 }]]), + cache, + }); + + assert.equal(first.cacheHit, false); + assert.equal(second.cacheHit, false); + assert.notEqual(first.values.get(81)?.width, second.values.get(81)?.width); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/__tests__/constraints/resolver.test.impl.ts` around lines 239 - 273, The test "default cache key accounts for dynamic base/parent/intrinsic maps" currently only exercises integer inputs; add a fractional-input regression case by calling resolveConstraints (using the existing ConstraintResolutionCache instance `cache`) with fractional numbers in the parentValues/intrinsicValues maps for node id 71 (e.g., parentValues { w: 80.5, h: 20.5, min_w: 80.5, min_h: 20.5 } and intrinsicValues { w: 1.2, h: 1.2, min_w: 1.2, min_h: 1.2 } or another fractional pair), assert the first fractional call yields cacheHit false and computes the expected fractional width (based on expression parent.w + intrinsic.w), then call again with the same fractional inputs to assert cacheHit true; update assertions for widths accordingly and reuse symbols boxWithId, buildConstraintGraph, runtimeNode, resolveConstraints, and ConstraintResolutionCache to locate the test code.
🤖 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/constraints.md`:
- Around line 3-18: The guide currently calls expr("...") the canonical way but
the PR and helper-first contract require the helper layer to be canonical and
expr(...) to be an escape hatch; update the wording so the Alpha Contract states
helpers (visibilityConstraints, widthConstraints, heightConstraints,
spaceConstraints, groupConstraints, conditionalConstraints) are the
preferred/default approach and that expr("...") is the advanced/escape-hatch for
derived values (width, height, min/max, flexBasis, and display), and ensure any
lines referencing expr("...") as canonical are changed to reflect this
helper-first policy and that display: expr("...") is described only as an escape
hatch for layout/viewport visibility decisions.
In `@packages/core/src/constraints/helpers.ts`:
- Around line 319-322: The loop over the steps array currently silently skips
falsy entries (if (!step) continue;) which allows malformed/degenerate steps to
pass through; instead, reject such entries with a helper-level argument error:
replace the continue with a thrown/descriptive error (using the same validation
pattern as assertInt32NonNegative) that references the offending index (e.g.,
"options.steps[${i}] is required and must be an object") so callers get
immediate feedback; apply the same change to the other loop at the second
occurrence (around lines 397-400) and keep using assertInt32NonNegative for
validating step.below/step.above after this check.
In `@packages/core/src/constraints/resolver.ts`:
- Around line 70-73: The cache-key serializer serializeFiniteOrEmpty currently
uses Math.trunc and discards fractional parts causing distinct numeric inputs to
collide; change serializeFiniteOrEmpty to return a deterministic string
representation of the full numeric value (e.g., return String(value) after the
Number.isFinite check) instead of Math.trunc so decimals are preserved, and
update any other occurrences in this file where Math.trunc was used for
cache-key generation (the same code paths referenced in the review) to use the
new serializer or the same String(value) approach to avoid stale cached
resolutions.
- Around line 398-405: The cached Map instance returned and stored by the
resolver can be mutated by callers despite the ReadonlyMap typing; update the
resolver's get(key: string) and the corresponding set/store path (the code that
writes into this.#entries) to shallow-copy Maps when reading from or writing to
the cache: when returning a cached value from get, return new Map(hit) instead
of the original hit, and when inserting into this.#entries store new Map(value)
(where value is the Map<InstanceId, ResolvedConstraintValues>) so the internal
this.#entries Map stays isolated from external mutations.
In `@packages/core/src/layout/__tests__/layout.flex-shrink.test.ts`:
- Line 3: The test imports createTestRenderer from an internal path
("../../testing/renderer.js"); update the import to use the package export
surface instead (import { createTestRenderer } from the package export that
re-exports the testing utilities, e.g. '@rezi-ui/core' or
'@rezi-ui/core/testing') so the test uses the public API; change the import
statement in layout.flex-shrink.test.ts to reference the package export and run
tests to ensure the symbol createTestRenderer is resolved.
In `@packages/core/src/layout/engine/layoutEngine.ts`:
- Around line 107-163: The DFS in findLegacyConstraintUsage can infinite-loop on
cyclic VNode graphs; add a visited guard (use a WeakSet<VNode>) and check
visited.has(node) before processing a popped frame and before pushing any
child/modal/layer content onto the stack to avoid re-processing the same VNode;
ensure you only mark nodes visited once (visited.add(node) when first
processed), keep use of symbols like stack, frame, VNode, LEGACY_SIZE_PROP_NAMES
and isLegacyBreakpointMap intact, and follow TypeScript strictness and runtime
invariants from docs/dev/code-standards.md (no behavioral changes besides
preventing cycles).
In `@scripts/check-create-rezi-templates.mjs`:
- Around line 131-133: The current check uses
content.includes(TEMPLATE_EXPR_ESCAPE_HATCH.allowMarker) which can match the
marker inside string literals; replace that includes check with a comment-aware
regex that only matches the allowMarker when it appears inside JavaScript-style
comments (// ... or /* ... */). Use TEMPLATE_EXPR_ESCAPE_HATCH.allowMarker and
TEMPLATE_EXPR_ESCAPE_HATCH.regex to build/compare against a regex that searches
for the marker within comment contexts (e.g., a pattern that looks for
(//.*marker|/*...marker...*/)), and use that regex in place of
content.includes(...) so only markers placed in actual comments satisfy the
guardrail.
- Around line 90-102: The current findLineMatch does a line-by-line regex test
which fails for LEGACY_LAYOUT_PATTERNS whose patterns span multiple lines (e.g.,
responsive-map objects) and the escape-hatch check uses
content.includes(TEMPLATE_EXPR_ESCAPE_HATCH.allowMarker) which is too
permissive; instead, run each pattern against the full file content (e.g., use
pattern.regex.exec(content) or content.match) to get a match and its index,
compute the line by counting newlines up to match.index, and return the same {
line, snippet } shape (snippet can be the trimmed matching line or the matched
text trimmed); also tighten the escape-hatch check to require the allowMarker
appear in a comment context (e.g., match it with a comment-aware regex rather
than a plain substring search of TEMPLATE_EXPR_ESCAPE_HATCH.allowMarker).
---
Nitpick comments:
In `@packages/core/src/__tests__/constraints/resolver.test.impl.ts`:
- Around line 239-273: The test "default cache key accounts for dynamic
base/parent/intrinsic maps" currently only exercises integer inputs; add a
fractional-input regression case by calling resolveConstraints (using the
existing ConstraintResolutionCache instance `cache`) with fractional numbers in
the parentValues/intrinsicValues maps for node id 71 (e.g., parentValues { w:
80.5, h: 20.5, min_w: 80.5, min_h: 20.5 } and intrinsicValues { w: 1.2, h: 1.2,
min_w: 1.2, min_h: 1.2 } or another fractional pair), assert the first
fractional call yields cacheHit false and computes the expected fractional width
(based on expression parent.w + intrinsic.w), then call again with the same
fractional inputs to assert cacheHit true; update assertions for widths
accordingly and reuse symbols boxWithId, buildConstraintGraph, runtimeNode,
resolveConstraints, and ConstraintResolutionCache to locate the test code.
In `@packages/core/src/layout/__tests__/layout.flex-shrink.test.ts`:
- Around line 10-34: Collapse the duplicated helpers childWidths and
childHeights into one generic helper (e.g., getChildRectProp or childMeasure)
that accepts the same out and ids parameters plus either a property key ("w" |
"h") or a selector function to extract the value from node.rect; reuse
createTestRenderer's render return type and the existing out.findById/null check
logic, failing with assert.fail if node is missing, and return the mapped values
by applying the selector to node.rect for each id.
In `@packages/core/src/layout/__tests__/layout.percentage.test.ts`:
- Line 6: The local type alias Axis in the test duplicates the canonical Axis
type; remove the inline `type Axis = "row" | "column";` and instead import the
canonical type (e.g. `import type { Axis } from "../types.js";` or from
`types.ts` if not re-exported) so the test uses the single source of truth
(`Axis`) and stays consistent with the layout types.
In `@packages/core/src/layout/engine/layoutEngine.ts`:
- Around line 872-881: Duplicate legacy-check + error construction around
findLegacyConstraintUsage(node) should be extracted into a small helper to avoid
drift; add a function (e.g., buildLegacyConstraintError or
ensureNoLegacyConstraint) that calls findLegacyConstraintUsage(node) and, if
non-null, returns the exact error object { ok: false, fatal: { code:
"ZRUI_INVALID_PROPS", detail: `Legacy size constraint usage detected:
${legacyUsage}` } } (or returns null/undefined when OK), then replace both
repeated blocks with a call to that helper (keep the same return shape and
symbol names so callers like the code currently using the inline block continue
to work).
In `@scripts/check-create-rezi-templates.mjs`:
- Around line 72-74: Remove the redundant identity map and the unnecessary final
sort: stop using .map((entry) => entry) when building entries (use
readdirSync(currentDir, { withFileTypes: true }).sort(...)) and remove the
trailing out.sort() that re-sorts results after the already-sorted traversal;
apply the same cleanup to the other occurrence around the entries variable at
lines 86–87 so the recursive collector relies solely on the sorted entries
variable for deterministic ordering.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
docs/guide/constraints.mdmkdocs.ymlpackages/core/src/__tests__/constraints/graph.test.tspackages/core/src/__tests__/constraints/helpers.test.impl.tspackages/core/src/__tests__/constraints/helpers.test.tspackages/core/src/__tests__/constraints/integration.test.tspackages/core/src/__tests__/constraints/parser.test.impl.tspackages/core/src/__tests__/constraints/parser.test.tspackages/core/src/__tests__/constraints/resolver.test.impl.tspackages/core/src/__tests__/constraints/resolver.test.tspackages/core/src/constraints/expr.tspackages/core/src/constraints/helpers.tspackages/core/src/constraints/resolver.tspackages/core/src/layout/__tests__/layout.edgecases.test.tspackages/core/src/layout/__tests__/layout.flex-shrink.test.tspackages/core/src/layout/__tests__/layout.percentage.test.tspackages/core/src/layout/constraints.tspackages/core/src/layout/engine/layoutEngine.tspackages/create-rezi/templates/starship/src/screens/crew.tspackages/create-rezi/templates/starship/src/screens/shell.tspackages/node/src/__tests__/config_guards.test.tsscripts/check-create-rezi-templates.mjsscripts/run-tests.mjs
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/core/src/tests/constraints/parser.test.ts
- packages/core/src/tests/constraints/helpers.test.ts
- packages/core/src/tests/constraints/integration.test.ts
- packages/node/src/tests/config_guards.test.ts
- scripts/run-tests.mjs
- packages/core/src/tests/constraints/helpers.test.impl.ts
- packages/core/src/tests/constraints/graph.test.ts
- packages/core/src/tests/constraints/parser.test.impl.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
docs/guide/constraints.md (1)
89-137: Consider adding one helper-first visibility example before theexpr(...)examples.Most examples in this section still demonstrate
display: expr(...). Adding a first-class helper example here would reduce policy drift and reinforce the migration direction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide/constraints.md` around lines 89 - 137, Add a helper-first visibility example above the existing expr(...) examples: introduce a short example using the first-class helper (e.g., a named visibility helper like showIf or visibility helper in the same style as ui.box/ui.grid usage) before the ui.box display: expr(...) snippet, and update the ui.column/grid section to mention using that helper as the preferred migration path instead of inline expr where possible; reference the existing examples (ui.box with display: expr(...), ui.grid's columns limitations, and the ui.column responsive composition) so the new helper example appears immediately before those expr-based samples.scripts/check-create-rezi-templates.mjs (2)
54-65: Hotspot regex won't catch multiline Math expressions.The
[^)\n]*in the HOTSPOT_VIEWPORT_MATH pattern stops at newlines, so calls formatted across lines won't be detected:Math.floor( viewport.width / 2 // not caught )Since these checks target specific known files and template style likely keeps such calls on single lines, this is low risk. If stricter enforcement is needed, consider
[^)]*(without\nexclusion) or a multiline-aware approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-create-rezi-templates.mjs` around lines 54 - 65, HOTSPOT_VIEWPORT_MATH's regex uses [^)\n]* which prevents matching Math calls that span multiple lines (e.g., Math.floor(\n viewport.width / 2\n)), so update the regex for both entries (the objects with templateKey "starship" and "animation-lab" and their regex fields) to allow newlines inside the parentheses—either switch [^)\n]* to [^)]* or use a multiline-safe pattern like [\s\S]*? or enable the DOTALL equivalent so Math\.(?:floor|ceil|min|max)\([^\)]*\(?:viewport|cols|rows) will match multi-line argument lists. Ensure both regex literals are modified consistently.
115-122: Comment stripping doesn't account for string literals.
stripTypeScriptCommentswill incorrectly blank out//or/*sequences inside string literals:const url = "http://example.com"; // → "http: " const re = /\/*.*/; // → blanked incorrectlyThis could cause false negatives (pattern not detected because blanked) or alter the content in unexpected ways. A robust solution would use a tokenizer or state machine, but for a guardrail script a pragmatic approach is acceptable if templates don't contain such edge cases.
Consider adding a code comment noting this limitation, or verify templates don't use URLs or regex literals in the checked source paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-create-rezi-templates.mjs` around lines 115 - 122, stripTypeScriptComments currently blanks comment-like sequences even when they occur inside string literals or regexes (e.g., "http://..." or /\/*.*/), causing incorrect outputs; update stripTypeScriptComments to use a simple state machine/tokenizer that tracks context (normal, single-line comment, block comment, single-quote string, double-quote string, template literal, and regex literal) and only blank characters when in comment states while preserving newlines and escape sequences, or if you prefer a smaller change, add a clear code comment above stripTypeScriptComments documenting this limitation and add a validation/assertion that checked templates do not contain URLs or regex literals so callers are aware of the risk.packages/core/src/__tests__/constraints/helpers.test.impl.ts (1)
30-73: Consider table-driven cases to reduce repetition in viewport-derived sizing tests.This block is solid, but a small table-driven refactor would make future helper additions easier to maintain.
Refactor sketch (table-driven assertions)
+ test("creates viewport-derived sizing constraints", () => { + const cases: Array<{ actual: string; expected: string }> = [ + { actual: widthConstraints.percentOfViewport(0.62).source, expected: "viewport.w * 0.62" }, + { + actual: widthConstraints.clampedViewportMinus({ minus: 4, min: 20, max: 140 }).source, + expected: "clamp(20, viewport.w - 4, 140)", + }, + { + actual: widthConstraints.minViewportPercent({ ratio: 0.34, min: 56 }).source, + expected: "max(56, viewport.w * 0.34)", + }, + { + actual: widthConstraints.stepsByViewportWidth({ + steps: [ + { below: 80, value: 10 }, + { below: 120, value: 20 }, + { below: 160, value: 30 }, + ], + }).source, + expected: "steps(viewport.w, 80: 10, 120: 20, 160: 30)", + }, + { actual: heightConstraints.percentOfViewport(0.34).source, expected: "viewport.h * 0.34" }, + { + actual: heightConstraints.minViewportPercent({ ratio: 0.34, min: 12 }).source, + expected: "max(12, viewport.h * 0.34)", + }, + { + actual: heightConstraints.stepsByViewportHeight({ + steps: [ + { below: 22, value: 0 }, + { below: 34, value: 1 }, + ], + }).source, + expected: "steps(viewport.h, 22: 0, 34: 1)", + }, + { + actual: heightConstraints.clampedPercentOfViewport({ ratio: 0.34, min: 12, max: 22 }).source, + expected: "clamp(12, viewport.h * 0.34, 22)", + }, + { + actual: heightConstraints.clampedViewportMinus({ minus: 4, min: 8, max: 40 }).source, + expected: "clamp(8, viewport.h - 4, 40)", + }, + ]; + for (const c of cases) assert.equal(c.actual, c.expected); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/__tests__/constraints/helpers.test.impl.ts` around lines 30 - 73, The test "creates viewport-derived sizing constraints" repeats many assert.equal calls; refactor into a table-driven test by creating an array of cases each containing a description, a call expression invoking the helper (e.g., widthConstraints.percentOfViewport, widthConstraints.clampedViewportMinus, widthConstraints.minViewportPercent, widthConstraints.stepsByViewportWidth, heightConstraints.percentOfViewport, heightConstraints.minViewportPercent, heightConstraints.stepsByViewportHeight, heightConstraints.clampedPercentOfViewport, heightConstraints.clampedViewportMinus) and the expected .source string, then iterate (cases.forEach) and run assert.equal(case.call.source, case.expected) with a helpful message; this consolidates the repeated assertions and makes adding new viewport-derived helpers easier to maintain.packages/core/src/layout/engine/layoutEngine.ts (1)
875-885: Consider extracting duplicated legacy-validation failure block.
measure()andlayout()construct the same error payload; centralizing this in a helper will keep behavior/messages in sync over time.♻️ Optional refactor sketch
+function validateNoLegacySizeConstraints(node: VNode): LayoutResult<null> | null { + const legacyUsage = findLegacyConstraintUsage(node); + if (legacyUsage === null) return null; + return { + ok: false, + fatal: { + code: "ZRUI_INVALID_PROPS", + detail: `Legacy size constraint usage detected: ${legacyUsage}`, + }, + }; +} export function measure(node: VNode, maxW: number, maxH: number, axis: Axis): LayoutResult<Size> { - const legacyUsage = findLegacyConstraintUsage(node); - if (legacyUsage !== null) { - return { - ok: false, - fatal: { - code: "ZRUI_INVALID_PROPS", - detail: `Legacy size constraint usage detected: ${legacyUsage}`, - }, - }; - } + const legacyValidation = validateNoLegacySizeConstraints(node); + if (legacyValidation) return legacyValidation; return measureNode(node, maxW, maxH, axis); } export function layout( node: VNode, @@ ): LayoutResult<LayoutTree> { - const legacyUsage = findLegacyConstraintUsage(node); - if (legacyUsage !== null) { - return { - ok: false, - fatal: { - code: "ZRUI_INVALID_PROPS", - detail: `Legacy size constraint usage detected: ${legacyUsage}`, - }, - }; - } + const legacyValidation = validateNoLegacySizeConstraints(node); + if (legacyValidation) return legacyValidation;Also applies to: 910-919
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/layout/engine/layoutEngine.ts` around lines 875 - 885, Both measure() and layout() duplicate the same legacy-constraint failure payload; extract that block into a single helper (e.g., makeLegacyConstraintError or buildLegacyConstraintFailure) that accepts the legacyUsage message and returns the { ok: false, fatal: { code: "ZRUI_INVALID_PROPS", detail: ... } } object; replace the duplicated if (legacyUsage !== null) branches in measure() and layout() with a call to this new helper and return its result, ensuring you still call findLegacyConstraintUsage(node) to produce the legacyUsage passed to the helper (referencing findLegacyConstraintUsage, measureNode, measure(), and layout()).
🤖 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/__tests__/constraints/helpers.test.impl.ts`:
- Around line 3-11: The tests import internal modules via relative paths; change
the imports to use the package's public exports instead (import expr,
conditionalConstraints, groupConstraints, heightConstraints, spaceConstraints,
visibilityConstraints, widthConstraints from the package entrypoint such as the
public `@rezi-ui/core` export), and if those symbols aren’t re-exported yet, add
them to the package's public export surface so the tests can import them from
the package entrypoint rather than internal paths; update the import statements
in the test file to reference the package export names (expr and the
*Constraints identifiers) accordingly.
In `@packages/core/src/constraints/resolver.ts`:
- Around line 466-482: The baseline `display` check currently overrides an
already-resolved positive `display` value; update the logic in resolver.ts
around the readResolvedProp calls so that the baseline `baselineDisplay` <= 0
only returns 0 when there is no valid positive `resolvedDisplay` from
readResolvedProp(resolvedByInstance.get(instanceId), "display") (i.e., only run
the baseline short-circuit if resolvedDisplay is undefined or not a finite > 0
number). Reference the variables/functions: resolvedDisplay, baselineDisplay,
readResolvedProp, resolvedByInstance, options.baseValues, and instanceId; ensure
TypeScript type-narrowing and existing Rezi error-handling/invariant patterns
are preserved when gating the baseline check.
---
Nitpick comments:
In `@docs/guide/constraints.md`:
- Around line 89-137: Add a helper-first visibility example above the existing
expr(...) examples: introduce a short example using the first-class helper
(e.g., a named visibility helper like showIf or visibility helper in the same
style as ui.box/ui.grid usage) before the ui.box display: expr(...) snippet, and
update the ui.column/grid section to mention using that helper as the preferred
migration path instead of inline expr where possible; reference the existing
examples (ui.box with display: expr(...), ui.grid's columns limitations, and the
ui.column responsive composition) so the new helper example appears immediately
before those expr-based samples.
In `@packages/core/src/__tests__/constraints/helpers.test.impl.ts`:
- Around line 30-73: The test "creates viewport-derived sizing constraints"
repeats many assert.equal calls; refactor into a table-driven test by creating
an array of cases each containing a description, a call expression invoking the
helper (e.g., widthConstraints.percentOfViewport,
widthConstraints.clampedViewportMinus, widthConstraints.minViewportPercent,
widthConstraints.stepsByViewportWidth, heightConstraints.percentOfViewport,
heightConstraints.minViewportPercent, heightConstraints.stepsByViewportHeight,
heightConstraints.clampedPercentOfViewport,
heightConstraints.clampedViewportMinus) and the expected .source string, then
iterate (cases.forEach) and run assert.equal(case.call.source, case.expected)
with a helpful message; this consolidates the repeated assertions and makes
adding new viewport-derived helpers easier to maintain.
In `@packages/core/src/layout/engine/layoutEngine.ts`:
- Around line 875-885: Both measure() and layout() duplicate the same
legacy-constraint failure payload; extract that block into a single helper
(e.g., makeLegacyConstraintError or buildLegacyConstraintFailure) that accepts
the legacyUsage message and returns the { ok: false, fatal: { code:
"ZRUI_INVALID_PROPS", detail: ... } } object; replace the duplicated if
(legacyUsage !== null) branches in measure() and layout() with a call to this
new helper and return its result, ensuring you still call
findLegacyConstraintUsage(node) to produce the legacyUsage passed to the helper
(referencing findLegacyConstraintUsage, measureNode, measure(), and layout()).
In `@scripts/check-create-rezi-templates.mjs`:
- Around line 54-65: HOTSPOT_VIEWPORT_MATH's regex uses [^)\n]* which prevents
matching Math calls that span multiple lines (e.g., Math.floor(\n
viewport.width / 2\n)), so update the regex for both entries (the objects with
templateKey "starship" and "animation-lab" and their regex fields) to allow
newlines inside the parentheses—either switch [^)\n]* to [^)]* or use a
multiline-safe pattern like [\s\S]*? or enable the DOTALL equivalent so
Math\.(?:floor|ceil|min|max)\([^\)]*\(?:viewport|cols|rows) will match
multi-line argument lists. Ensure both regex literals are modified consistently.
- Around line 115-122: stripTypeScriptComments currently blanks comment-like
sequences even when they occur inside string literals or regexes (e.g.,
"http://..." or /\/*.*/), causing incorrect outputs; update
stripTypeScriptComments to use a simple state machine/tokenizer that tracks
context (normal, single-line comment, block comment, single-quote string,
double-quote string, template literal, and regex literal) and only blank
characters when in comment states while preserving newlines and escape
sequences, or if you prefer a smaller change, add a clear code comment above
stripTypeScriptComments documenting this limitation and add a
validation/assertion that checked templates do not contain URLs or regex
literals so callers are aware of the risk.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/guide/constraints.mdpackages/core/src/__tests__/constraints/helpers.test.impl.tspackages/core/src/__tests__/constraints/resolver.test.impl.tspackages/core/src/constraints/helpers.tspackages/core/src/constraints/resolver.tspackages/core/src/layout/__tests__/layout.flex-shrink.test.tspackages/core/src/layout/__tests__/layout.percentage.test.tspackages/core/src/layout/engine/layoutEngine.tsscripts/check-create-rezi-templates.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/tests/constraints/resolver.test.impl.ts
- packages/core/src/constraints/helpers.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/__tests__/constraints/integration.test.ts`:
- Around line 29-33: The fire-and-forget import pattern using "void
import(...).catch(...)" should be changed to a deterministic top-level await:
replace the discarded promise call with "await
import('./integration.test.impl.js')" (and corresponding module paths in the
other constraint test bootstraps) and remove the process.nextTick(...) throw
wrapper; instead let the thrown error propagate so the test runner sees
bootstrap failures immediately; keep the integrationBootstrapError(...) usage by
calling it when awaiting the import fails so the same error transformation
occurs but synchronously via await.
In `@packages/core/src/__tests__/constraints/resolver.test.ts`:
- Around line 27-31: The test file currently uses a fire-and-forget import (void
import("./resolver.test.impl.js")) which can cause test registration races;
replace that with a blocking import by using await
import("./resolver.test.impl.js") (or an async IIFE that awaits the import) and
handle errors by catching and passing them to resolverBootstrapError as before
(e.g., try { await import(...) } catch (error) { process.nextTick(() => { throw
resolverBootstrapError(error); }); }). Apply the same change to the other
constraint test files (graph, parser, helpers, integration) that use the same
pattern.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/core/src/__tests__/constraints/graph.test.tspackages/core/src/__tests__/constraints/helpers.test.impl.tspackages/core/src/__tests__/constraints/helpers.test.tspackages/core/src/__tests__/constraints/integration.test.tspackages/core/src/__tests__/constraints/parser.test.tspackages/core/src/__tests__/constraints/resolver.test.impl.tspackages/core/src/__tests__/constraints/resolver.test.tspackages/core/src/constraints/resolver.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/core/src/tests/constraints/resolver.test.impl.ts
- packages/core/src/tests/constraints/parser.test.ts
- packages/core/src/tests/constraints/helpers.test.ts
- packages/core/src/constraints/resolver.ts
- packages/core/src/tests/constraints/helpers.test.impl.ts
- packages/core/src/tests/constraints/graph.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/__tests__/constraints/parser.test.ts (1)
22-33: Consider using the resolved path for consistency.The TS branch uses the resolved absolute path (
tsImplPath), but the JS branch uses a relative path literal. For consistency and maintainability, consider using the resolved path for the JS import as well.♻️ Suggested refactor for path consistency
try { if (existsSync(tsImplPath)) { require("tsx/cjs"); require(tsImplPath); } else if (existsSync(jsImplPath)) { - await import("./parser.test.impl.js"); + await import(jsImplPath); } else { throw new Error("Missing parser constraint test implementation"); } } catch (error: unknown) { throw parserBootstrapError(error); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/__tests__/constraints/parser.test.ts` around lines 22 - 33, The JS branch should import using the resolved path (jsImplPath) like the TS branch does; change the dynamic import await import("./parser.test.impl.js") to import the resolved jsImplPath (convert to a file:// URL if needed using pathToFileURL(jsImplPath).href) so the code consistently uses the resolved paths (referencing tsImplPath and jsImplPath) and retain the existing error handling via parserBootstrapError in the catch block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/__tests__/constraints/parser.test.ts`:
- Around line 22-33: The JS branch should import using the resolved path
(jsImplPath) like the TS branch does; change the dynamic import await
import("./parser.test.impl.js") to import the resolved jsImplPath (convert to a
file:// URL if needed using pathToFileURL(jsImplPath).href) so the code
consistently uses the resolved paths (referencing tsImplPath and jsImplPath) and
retain the existing error handling via parserBootstrapError in the catch block.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/__tests__/constraints/graph.test.tspackages/core/src/__tests__/constraints/helpers.test.tspackages/core/src/__tests__/constraints/integration.test.tspackages/core/src/__tests__/constraints/parser.test.tspackages/core/src/__tests__/constraints/resolver.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/tests/constraints/integration.test.ts
- packages/core/src/tests/constraints/helpers.test.ts
This was referenced Mar 3, 2026
This was referenced Mar 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Productizes the constraint layout system end-to-end so it feels first-class (helper-first) across core API + JSX parity, templates, docs, tooling guardrails, tests, and dev instrumentation.
What’s included
@rezi-ui/core(semantic namespaces) withexpr(...)kept as the advanced escape hatch.scripts/check-create-rezi-templates.mjsenforces helper-first policy.scripts/migrate-constraints-to-helpers.mjscodemod for common conversions.scripts/run-tests.mjs --filter <regex>support (docs/runtime parity).ctrl+shift+i).Notes
docs/rfc/*anddocs/dev/constraint-*.Validation
npm run lintnpm run typechecknpm run buildnpm run check:create-rezi-templatesnode scripts/run-tests.mjsnpm run check:docsSummary by CodeRabbit
New Features
Documentation
Chores
Tests