Grida Canvas - Flex layout#437
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughIntroduce a scene-graph-driven layout system: LayoutEngine/LayoutTree/LayoutResult; thread layout metadata through schema, IO, geometry, painter, runtime, editor UIs and gestures; migrate examples/tests/goldens to layout-driven sizing; bump crates/grida-canvas-wasm package version. Changes
Sequence Diagram(s)%%{init: {"theme":"neutral"}}%%
sequenceDiagram
participant Scene
participant LayoutEngine
participant LayoutTree
participant Taffy
participant LayoutResult
participant GeometryCache
participant Renderer
Scene->>LayoutEngine: compute(scene, viewport_size)
activate LayoutEngine
LayoutEngine->>LayoutTree: build taffy nodes (node_to_taffy_style)
LayoutTree->>Taffy: insert nodes & children
Taffy->>Taffy: compute layouts
Taffy-->>LayoutTree: layouts
LayoutTree-->>LayoutEngine: per-node layouts
LayoutEngine->>LayoutResult: populate cache
LayoutEngine-->>Scene: return & expose LayoutResult
deactivate LayoutEngine
Scene->>GeometryCache: from_scene_with_layout(scene, layout_result, viewport_size)
GeometryCache->>GeometryCache: compute world transforms & bounds (layout-aware)
GeometryCache-->>Renderer: geometry + render bounds
Renderer->>Renderer: build layers & paint
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas to focus review on:
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
softmarshmallow
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
editor/scaffolds/sidecontrol/controls/utils/toggle-group.tsx (1)
11-21: Restore size/variant propagation to ToggleGroupItem via context and apply toggleVariants styling.FlexWrapControl passes
size="sm"to ToggleGroup (line 14 of flex-wrap.tsx), but the current implementation silently drops bothvariantandsizeprops. Additionally, ToggleGroupItem hardcodes all styling instead of applyingtoggleVariants, preventing inheritance of group-level customization.Apply the diff from the review to:
- Wrap ToggleGroup children in a context provider that exposes variant/size
- Have ToggleGroupItem consume context and apply
toggleVariants({ variant: _variant, size: _size })- Remove hardcoded item styles in favor of
toggleVariantsThis restores the ability for group-level size/variant to flow to items as expected.
editor/scaffolds/sidecontrol/ui/index.tsx (1)
384-477: Duplicate "Layout" section renders twice in mixed-selection UI — remove second instance.File:
editor/scaffolds/sidecontrol/sidecontrol-node-selection.tsx(not ui/index.tsx)
Lines: 384-477 and 667-744Two nearly identical "Layout" blocks exist in
ModeMixedNodeProperties, causing duplicate Width/Height/Flow/Alignment controls in the UI. The second block (lines 667-744) is a subset of the first and should be removed.- <SidebarSection - hidden={config.layout === "off"} - className="border-b pb-4" - > - <SidebarSectionHeaderItem> - <SidebarSectionHeaderLabel>Layout</SidebarSectionHeaderLabel> - </SidebarSectionHeaderItem> - <SidebarMenuSectionContent className="space-y-2"> - <PropertyLine hidden={config.size === "off"}> - <PropertyLineLabel>Width</PropertyLineLabel> - <LengthPercentageControl - value={width?.value} - onValueCommit={change.width} - /> - </PropertyLine> - <PropertyLine hidden={config.size === "off"}> - <PropertyLineLabel>Height</PropertyLineLabel> - <LengthPercentageControl - value={height?.value} - onValueCommit={change.height} - /> - </PropertyLine> - {types.has("container") && ( - <PropertyLine> - <PropertyLineLabel>Flow</PropertyLineLabel> - <LayoutControl - value={ - layout?.value === grida.mixed || - direction?.value === grida.mixed || - layout?.value === undefined || - (layout?.value === "flex" && direction?.value === undefined) - ? undefined - : { - layoutMode: layout?.value ?? "flow", - direction: - layout?.value === "flex" - ? direction?.value - : undefined, - } - } - onValueChange={(value) => { - change.layout(value.layoutMode); - if (value.direction) { - change.direction(value.direction); - } - }} - /> - </PropertyLine> - )} - <PropertyLine hidden={!has_flex_container}> - <PropertyLineLabel>Alignment</PropertyLineLabel> - <FlexAlignControl - className="w-full" - direction={ - direction?.value === grida.mixed - ? "horizontal" - : (direction?.value ?? "horizontal") - } - value={ - mainAxisAlignment?.value === grida.mixed || - crossAxisAlignment?.value === grida.mixed || - mainAxisAlignment?.value === undefined || - crossAxisAlignment?.value === undefined - ? undefined - : { - mainAxisAlignment: mainAxisAlignment.value, - crossAxisAlignment: crossAxisAlignment.value, - } - } - onValueChange={(value) => { - change.mainAxisAlignment(value.mainAxisAlignment); - change.crossAxisAlignment(value.crossAxisAlignment); - }} - /> - </PropertyLine> - </SidebarMenuSectionContent> - </SidebarSection>editor/grida-canvas/reducers/document.reducer.ts (1)
1268-1278: Guard geometry reads to avoid runtime panics during autolayoutThe type signature getNodeAbsoluteBoundingRect(node_id: string): cmath.Rectangle | null confirms this method can return null. Your code at lines 1268–1278 uses non-null assertions without guards. This is inconsistent with defensive patterns elsewhere in the file (e.g., line 823 checks
if (!r) return null;, line 2090 checksif (!rect || !flattened) continue;).Add null checks and early-return as suggested:
- const container_rect = - context.geometry.getNodeAbsoluteBoundingRect(container_id)!; + const container_rect = + context.geometry.getNodeAbsoluteBoundingRect(container_id); + if (!container_rect) { + console.warn("autolayout: missing geometry for container", container_id); + return state; + } const delta: cmath.Vector2 = [-container_rect.x, -container_rect.y]; - const rects = children - .map( - (node_id) => context.geometry.getNodeAbsoluteBoundingRect(node_id)! - ) + const rects = children + .map((node_id) => context.geometry.getNodeAbsoluteBoundingRect(node_id)) + .filter((r): r is cmath.Rectangle => !!r) .map((rect) => cmath.rect.translate(rect, delta)) .map((rect) => cmath.rect.quantize(rect, 1));crates/grida-canvas/src/painter/geometry.rs (1)
355-362: Makeboolean_operation_shapeandboolean_operation_pathpub(crate)— they are only called internally.Both functions are currently
puband use&NodeId(internal u64), violating the guideline that public APIs must useUserNodeId(String). However, they are only called internally: fromlayer.rs(line 350) andpainter_debug_node.rs(line 343). Since there are no external callers, restrict visibility topub(crate):-pub fn boolean_operation_path( +pub(crate) fn boolean_operation_path( id: &NodeId,-pub fn boolean_operation_shape( +pub(crate) fn boolean_operation_shape( id: &NodeId,crates/grida-canvas/src/cache/geometry.rs (3)
376-421: TextSpan geometry returns local bounds, ignores layout, and drops effects.
- Returns intrinsic (local) bounds instead of world bounds — breaks parent unions.
- Ignores layout_result; text should respect computed position/size.
- Render bounds ignore text effects.
Apply this fix:
- Node::TextSpan(n) => { - // Get final measured metrics from cache - let measurements = paragraph_cache.measure( - &n.text, - &n.text_style, - &n.text_align, - &n.max_lines, - &n.ellipsis, - n.width, - fonts, - Some(id), - ); - - // Create intrinsic bounds (starting at origin, like other nodes) - /// TODO: Remove this hack to support 0 value visibility - const MIN_SIZE_DIRTY_HACK: f32 = 1.0; - let intrinsic_bounds = Rectangle { - x: 0.0, - y: 0.0, - width: measurements.max_width.max(MIN_SIZE_DIRTY_HACK), - height: n - .height - .unwrap_or(measurements.height) - .max(MIN_SIZE_DIRTY_HACK), - }; - - // Use the node's transform directly (which already includes positioning) - let local_transform = n.transform; - let world_transform = parent_world.compose(&local_transform); - let world_bounds = transform_rect(&intrinsic_bounds, &world_transform); - let render_bounds = compute_render_bounds(node, world_bounds); - - let entry = GeometryEntry { - transform: local_transform, - absolute_transform: world_transform, - bounding_box: intrinsic_bounds, - absolute_bounding_box: world_bounds, - absolute_render_bounds: render_bounds, - parent: parent_id.clone(), - dirty_transform: false, - dirty_bounds: false, - }; - cache.entries.insert(id.clone(), entry.clone()); - intrinsic_bounds - } + Node::TextSpan(n) => { + // Measure text for fallback sizing + let measurements = paragraph_cache.measure( + &n.text, + &n.text_style, + &n.text_align, + &n.max_lines, + &n.ellipsis, + n.width, + fonts, + Some(id), + ); + const MIN_SIZE_DIRTY_HACK: f32 = 1.0; + let fallback_w = n + .width + .unwrap_or(measurements.max_width) + .max(MIN_SIZE_DIRTY_HACK); + let fallback_h = n + .height + .unwrap_or(measurements.height) + .max(MIN_SIZE_DIRTY_HACK); + + // Prefer layout engine result when present + let (x, y, width, height) = + if let Some(res) = layout_result.and_then(|r| r.get(id)) { + (res.x, res.y, res.width, res.height) + } else { + (n.transform.x(), n.transform.y(), fallback_w, fallback_h) + }; + + let local_transform = AffineTransform::new(x, y, n.transform.rotation()); + let local_bounds = Rectangle { + x: 0.0, + y: 0.0, + width, + height, + }; + let world_transform = parent_world.compose(&local_transform); + let world_bounds = transform_rect(&local_bounds, &world_transform); + // Use text effects (not defaults) + let render_bounds = compute_render_bounds_from_style( + world_bounds, + n.stroke_width, + n.stroke_align, + &n.effects, + ); + + let entry = GeometryEntry { + transform: local_transform, + absolute_transform: world_transform, + bounding_box: local_bounds, + absolute_bounding_box: world_bounds, + absolute_render_bounds: render_bounds, + parent: parent_id.clone(), + dirty_transform: false, + dirty_bounds: false, + }; + cache.entries.insert(id.clone(), entry.clone()); + entry.absolute_bounding_box + }
707-713: Honor text effects in render bounds for TextSpan.Use the node’s effects instead of defaults.
Apply this diff:
- Node::TextSpan(n) => compute_render_bounds_from_style( - world_bounds, - n.stroke_width, - n.stroke_align, - &LayerEffects::default(), - ), + Node::TextSpan(n) => compute_render_bounds_from_style( + world_bounds, + n.stroke_width, + n.stroke_align, + &n.effects, + ),
495-536: Public getters leak NodeId; keep internal or provide UserNodeId facade.get_transform/get_world_transform/get_world_bounds/get_render_bounds/get_parent accept NodeId publicly. Align with the guideline by restricting to crate or exposing a UserNodeId-based facade.
As per coding guidelines
Apply this diff (restrict now):
- pub fn get_transform(&self, id: &NodeId) -> Option<AffineTransform> { + pub(crate) fn get_transform(&self, id: &NodeId) -> Option<AffineTransform> { @@ - pub fn get_world_transform(&self, id: &NodeId) -> Option<AffineTransform> { + pub(crate) fn get_world_transform(&self, id: &NodeId) -> Option<AffineTransform> { @@ - pub fn get_world_bounds(&self, id: &NodeId) -> Option<Rectangle> { + pub(crate) fn get_world_bounds(&self, id: &NodeId) -> Option<Rectangle> { @@ - pub fn get_render_bounds(&self, id: &NodeId) -> Option<Rectangle> { + pub(crate) fn get_render_bounds(&self, id: &NodeId) -> Option<Rectangle> { @@ - pub fn get_parent(&self, id: &NodeId) -> Option<NodeId> { + pub(crate) fn get_parent(&self, id: &NodeId) -> Option<NodeId> {Also consider making
filtercrate-visible since it closes over NodeId:- pub fn filter(&self, filter: impl Fn(&NodeId, &GeometryEntry) -> bool) -> Self { + pub(crate) fn filter(&self, filter: impl Fn(&NodeId, &GeometryEntry) -> bool) -> Self {crates/grida-canvas/examples/golden_layout_flex.rs (1)
16-22: Remove unused hasher.The ID hash is computed then discarded.
Apply this diff:
- // Use a simple hash of the string as u64 ID - use std::collections::hash_map::DefaultHasher; - use std::hash::{Hash, Hasher}; - let mut hasher = DefaultHasher::new(); - id.hash(&mut hasher); - let _ = hasher.finish(); // Generate ID but don't store it since it's not usededitor/grida-canvas/editor.i.ts (1)
2992-3005: Use NodeID alias consistently (avoid plain string here)These new flex APIs take node_id: string while the rest of the surface uses NodeID. Align to NodeID to prevent type drift.
Apply this diff:
- changeFlexContainerNodeDirection(node_id: string, direction: cg.Axis): void; + changeFlexContainerNodeDirection(node_id: NodeID, direction: cg.Axis): void; @@ - changeFlexContainerNodeMainAxisAlignment( - node_id: string, + changeFlexContainerNodeMainAxisAlignment( + node_id: NodeID, @@ - changeFlexContainerNodeCrossAxisAlignment( - node_id: string, + changeFlexContainerNodeCrossAxisAlignment( + node_id: NodeID, @@ - changeFlexContainerNodeGap( - node_id: string, + changeFlexContainerNodeGap( + node_id: NodeID, @@ - changeFlexContainerNodeWrap(node_id: string, wrap: "wrap" | "nowrap"): void; + changeFlexContainerNodeWrap(node_id: NodeID, wrap: "wrap" | "nowrap"): void;crates/grida-canvas/src/io/io_figma.rs (1)
1285-1373: Blocker: vector children are created but never linked to the parentYou push path NodeIds into
children(lines 1302, 1324), but never register the parent→children entry intoself.links. As a result, paths become orphaned in the scene graph.All other converters with children—
convert_group(line 1663),convert_boolean_operation(line 1408), etc.—follow the same pattern:
- Call
get_or_create_internal_id(&origin.id)to get a persistent parent ID- Call
self.links.insert(parent_id, children)to register the relationship- Return the node
Apply this fix inside
convert_vector(lines 1282–1374) before returning the parent Container node:// Create a group node containing all the path nodes { let transform = Self::convert_transform(origin.relative_transform.as_ref()); let size = Self::convert_size(origin.size.as_ref()); + // Register parent→children relationship in links + let parent_id = self.get_or_create_internal_id(&origin.id); + if !children.is_empty() { + self.links.insert(parent_id, children); + } Ok(Node::Container(ContainerNodeRec {
🧹 Nitpick comments (71)
editor/.gitignore (1)
47-50: Consider clarifying section organization.The
target/entry is placed under the# Playwrightcomment, but it's Rust-specific rather than Playwright-specific. If this is intentional (e.g., Playwright runs against Rust binaries), consider adding a clarifying comment; otherwise, consider reorganizing into a dedicated Rust section for maintainability.If reorganizing, you could structure it like this:
# Playwright node_modules/ - + +# Rust target/editor/grida-canvas-react/ui-config.ts (1)
1-33: Clarify the relationship between the general and specific constants.The documentation for
MIN_NODE_OVERLAY_INNER_CONTENT_VISIBLE_UI_SIZEindicates it "applies to: corner radius, gap, padding, ...", yet separate constants are defined for each of these (MIN_NODE_OVERLAY_GAP_VISIBLE_UI_SIZE,MIN_NODE_OVERLAY_PADDING_VISIBLE_UI_SIZE, and the existingMIN_NODE_OVERLAY_CORNER_RADIUS_VISIBLE_UI_SIZE). This creates ambiguity about which constant should be used and duplicates the value 100 across multiple declarations.Consider one of the following approaches:
- If these thresholds should remain synchronized, use a single constant and remove the specific ones.
- If specific constants are needed for future flexibility, remove the general constant and keep only the specific ones.
- If both are needed, clarify in documentation that the general constant is for reference only and specific constants should be used in code.
Example refactor (if synchronization is desired):
-/** - * Minimum node size in UI space (pixels) required to display the inner content overlay handle. - * This avoids showing the handle when nodes are too small or far zoomed out, - * ensuring the control stays within the node's bounds for the best user experience. - * - * applies to: - * - corner radius - * - gap - * - padding - * - ... - */ -export const MIN_NODE_OVERLAY_INNER_CONTENT_VISIBLE_UI_SIZE = 100; - /** - * Minimum node size in UI space (pixels) required to display the corner radius overlay handle. + * Minimum node size in UI space (pixels) required to display inner content overlay handles. * This avoids showing the handle when nodes are too small or far zoomed out, * ensuring the control stays within the node's bounds for the best user experience. + * + * Used for: corner radius, gap, padding, and other inner content overlays. */ -export const MIN_NODE_OVERLAY_CORNER_RADIUS_VISIBLE_UI_SIZE = 100; - -/** - * Minimum node size in UI space (pixels) required to display the gap overlay handle. - * This avoids showing the handle when nodes are too small or far zoomed out, - * ensuring the control stays within the node's bounds for the best user experience. - */ -export const MIN_NODE_OVERLAY_GAP_VISIBLE_UI_SIZE = 100; - -/** - * Minimum node size in UI space (pixels) required to display the padding overlay handle. - * This avoids showing the handle when nodes are too small or far zoomed out, - * ensuring the control stays within the node's bounds for the best user experience. - */ -export const MIN_NODE_OVERLAY_PADDING_VISIBLE_UI_SIZE = 100; +export const MIN_NODE_OVERLAY_INNER_CONTENT_VISIBLE_UI_SIZE = 100;editor/scaffolds/sidecontrol/controls/positioning.tsx (1)
212-214: Minor redundancy in disabled click handling.Setting
onClicktoundefinedwhendisabledis true (line 212) is technically redundant, since thedisabledattribute (line 213) already prevents click events. However, this defensive pattern is acceptable and doesn't cause issues.If you prefer to simplify:
- onClick={disabled ? undefined : onClick} + onClick={onClick} disabled={disabled}The disabled attribute alone is sufficient to prevent interaction.
packages/grida-cmath/index.ts (3)
162-163: Type can be derived from Rectangle keys for future‑proofing.Bind the union to Rectangle so refactors don’t drift.
-export type RectangleDimension = "width" | "height"; +export type RectangleDimension = keyof Pick<Rectangle, "width" | "height">;
1188-1195: Keep compile‑time and runtime guarantees tight for the side map.Current map is correct. Optionally use
satisfiesfor exhaustive key checking and keep literal types; you can alsoObject.freezefor runtime immutability.-const __opposite_side_map: Readonly<Record<RectangleSide, RectangleSide>> = { - top: "bottom", - right: "left", - bottom: "top", - left: "right", -} as const; +const __opposite_side_map = Object.freeze({ + top: "bottom", + right: "left", + bottom: "top", + left: "right", +} as const) satisfies Record<RectangleSide, RectangleSide>;
1217-1219: Function is good; consider naming/doc consistency with compass.invertDirection.Optional: rename to
invertSidefor symmetry withcompass.invertDirection, and add a one‑line JSDoc.-export function getOppositeSide(side: RectangleSide): RectangleSide { +/** Returns the opposite rectangle side (top↔bottom, left↔right). */ +export function invertSide(side: RectangleSide): RectangleSide { return __opposite_side_map[side]; }editor/grida-canvas-react/viewport/ui/svg-fill-patterns.tsx (1)
30-52: Consider providing a fallback ID or making the component more resilient.While making
idrequired (as suggested above) is the primary fix, consider whether a generated fallback ID (e.g., usinguseId()) would improve the API. However, given the warning about uniqueness and the need for deterministic IDs for references, a required prop is likely the better design.editor/scaffolds/sidecontrol/controls/padding.tsx (8)
7-10: Use type-only import to avoid bundling schema at runtime.
@grida/schemais only used for types here; importing it as a value adds bundle weight.-import grida from "@grida/schema"; - -type Padding = grida.program.nodes.i.IPadding["padding"]; +// type-only reference to avoid runtime import +type Padding = import("@grida/schema").program.nodes.i.IPadding["padding"];
11-17: Prop typing: makevalueoptional if you default it.You default
value = 0but keep it required in the prop type. Loosen the type for consistency.export function PaddingControl({ value = 0, onValueCommit, }: { - value: Padding; + value?: Padding; onValueCommit?: (value: Padding) => void; }) {
18-19: Initialize UI mode from incoming value.Start per-side UI when
valueis an object; keeps UX aligned with current data.- const [showIndividual, setShowIndividual] = useState(false); + const [showIndividual, setShowIndividual] = useState( + () => typeof value !== "number" + );Optional sync if
valuechanges:+ // import { useEffect } above + useEffect(() => { + if (typeof value !== "number") setShowIndividual(true); + }, [value]);
41-50: Micro-nit: trimuseMemodeps.
uniformValuedepends onisUniformandpaddingValues;valueis redundant.- }, [isUniform, value, paddingValues]); + }, [isUniform, paddingValues]);
51-59: Placeholder order: use CSS shorthand TRBL.Show values as
top right bottom left(spaces, not commas) to match CSS mental model.- return [ - paddingValues.left ?? 0, - paddingValues.top ?? 0, - paddingValues.right ?? 0, - paddingValues.bottom ?? 0, - ].join(", "); + // CSS shorthand order (top right bottom left) + return [ + paddingValues.top ?? 0, + paddingValues.right ?? 0, + paddingValues.bottom ?? 0, + paddingValues.left ?? 0, + ].join(" ");
90-90: Avoid duplicate input styling.
InputPropertyNumberalready appliesWorkbenchUI.inputVariants({ size: "xs" }). Passing it again can cause conflicts.- className={cn(WorkbenchUI.inputVariants({ size: "xs" }), "flex-1")} + className="flex-1"
99-101: Verify Tailwind selector support.
data-[state=on]:*:[svg]:...is nonstandard; ensure your Tailwind config/postcss can parse nested/wildcard child selectors. If not, consider applying state class to the icon (e.g., conditionally set icon color whenpressed).
124-124: Semantic separators.
<hr>is horizontal by default. For vertical dividers, preferrole="separator" aria-orientation="vertical"on adiv, or use a purely decorativedivwitharia-hidden.- <hr className="w-px h-10" /> + <div role="separator" aria-orientation="vertical" className="w-px h-10" />Also applies to: 140-140, 156-156
editor/scaffolds/sidecontrol/controls/layout.tsx (2)
66-72: Avoid@ts-expect-errorfor the disabled “grid” option.Brittle; it will fail if the error disappears. Prefer a typed extension or runtime guard:
- Option 1: Keep generic loose and guard in handler:
- <PropertyEnumToggle<Option> + <PropertyEnumToggle<string> @@ - onValueChange={_onValueChange} + onValueChange={(v) => { + if (v === "normal" || v === "flex-row" || v === "flex-column") _onValueChange(v); + }}
- Option 2: Define
type ExtendedOption = Option | "grid"and keep handler narrowed toOption.
63-77: Consider mixed-state support for multi-selection.
PropertyEnumTogglesupportsTMixed<T>, butLayoutControlalways supplies a concreteOption. If multiple nodes have mixed layout, show mixed to avoid misleading UI.Example approach:
- Accept
valueKey?: TMixed<Option>alongsidevalue?: PartialLayoutProperties.- Use
valueKeyfor the toggle’svalue; keep current mapping for single-state.editor/scaffolds/sidecontrol/controls/flex-align.tsx (1)
164-165: Remove unused variable.
isHorizontalis computed but unused in FlexAlignControl.- const isHorizontal = direction === "horizontal";editor/scaffolds/sidecontrol/controls/flex-wrap.tsx (1)
14-19: Controlled component shouldn’t also set defaultValue; and groupsize="sm"relies on ToggleGroup propagation.
- Drop
defaultValuesincevaluemakes it controlled.size="sm"on the group will be ignored unless ToggleGroup propagates size to items (see toggle-group.tsx fix).- size="sm" - defaultValue="nowrap" + size="sm" value={value} // toggle group can callback with "" when de-selecting, will prevent this. onValueChange={(v) => v !== "" && onValueChange?.(v as "wrap" | "nowrap")}After updating ToggleGroup, verify visual size by checking the wrap buttons render at “sm”.
crates/grida-canvas/src/shape/polygon.rs (1)
64-90: Consider checking for empty input before iteration.The function correctly handles empty input, but the logic flow could be optimized by checking
points.is_empty()before the loop to avoid unnecessary min/max initialization.Apply this diff to improve the logic flow:
pub(crate) fn polygon_bounds(points: &[CGPoint]) -> Rectangle { + if points.is_empty() { + return Rectangle { + x: 0.0, + y: 0.0, + width: 0.0, + height: 0.0, + }; + } + let mut min_x = f32::INFINITY; let mut min_y = f32::INFINITY; let mut max_x = f32::NEG_INFINITY; let mut max_y = f32::NEG_INFINITY; for p in points { min_x = min_x.min(p.x); min_y = min_y.min(p.y); max_x = max_x.max(p.x); max_y = max_y.max(p.y); } - if points.is_empty() { - Rectangle { - x: 0.0, - y: 0.0, - width: 0.0, - height: 0.0, - } - } else { - Rectangle { - x: min_x, - y: min_y, - width: max_x - min_x, - height: max_y - min_y, - } - } + Rectangle { + x: min_x, + y: min_y, + width: max_x - min_x, + height: max_y - min_y, + } }docs/wg/feat-layout/index.md (5)
25-36: Copyedit: hyphenation for clarityTighten phrasing; fix hyphenation.
-**Pros:** - -- Intuitive, simple, battle tested, de facto standards +- **Pros:** + - Intuitive, simple, battle-tested, de facto standards @@ -**Cons:** - -- Not predictable -- Layout-centric, not graphics friendly (non XYWH centric) +- **Cons:** + - Not predictable + - Layout-centric, not graphics-friendly (non-XYWH-centric)
58-67: Copyedit: consistency in “-friendly/-centric”Minor readability polish.
-which is designer friendly and intuitive. +which is designer-friendly and intuitive. @@ -- Designer friendly -- Graphics schema friendly — the XY values are actually defined/computed with the XY field, making the API `node.x = 10` (graphics centric) +- Designer-friendly +- Graphics-schema-friendly — the XY values are defined/computed with the XY field, making the API `node.x = 10` (graphics-centric) @@ -- Can represent "center" alignment, where the position basis is center, even while not actually center positioned +- Can represent "center" alignment, where the position basis is center, even while not actually center-positioned
105-114: Soften “lossless” claim for inset ↔ anchor translationEdge cases (e.g., center constraints, over-constrained states) can be lossy. Recommend hedging.
-The relationship between CSS inset and anchor positioning is fundamentally lossless: +The relationship between CSS inset and anchor positioning can often be translated without loss in common cases: @@ -- **CSS inset ↔ Anchor translation:** Conversion between inset properties and anchors can be performed without loss of layout semantics. +- **CSS inset ↔ Anchor translation:** Conversion between inset properties and anchors is straightforward for many patterns (e.g., parent-anchored edges); document exceptions (center, stretch, over-constraints).
150-174: Levels: align with current scope (anchors at Level 4+)Make it explicit that Level 1/2 ship inset+flex; anchors appear at Level 4 later.
-### Level 1 — Practical Foundation (MVP) +### Level 1 — Practical Foundation (MVP, current) @@ -- **Parent-only anchors:** Support anchors referencing only the parent node (identical semantics to inset + relative positioning). +- **Inset positioning foundation:** Parent-relative positioning via inset semantics (xy/size preserved), compatible with CSS. @@ -### Level 4 — Full Anchor Model (Advanced) +### Level 4 — Full Anchor Model (Advanced, future)
121-123: Wording nitShorten “with respect to”.
-absolute XY positioning with respect to parent anchors +absolute XY positioning relative to parent anchorseditor/grida-canvas-react/viewport/ui/surface-padding-overlay.tsx (3)
52-72: Scale robustness: handle negative/non-uniform scales and clampUse absolute scale components and clamp thickness so rects never go negative.
- const paddingRects = useMemo(() => { + const paddingRects = useMemo(() => { + const sx = Math.abs(transform[0][0] ?? 1); + const sy = Math.abs(transform[1][1] ?? 1); @@ - if (top > 0) { + if (top > 0) { + const h = Math.min(top * sy, height); rects.push({ @@ - height: top * transform[1][1], // Scale by transform + height: h, }, }); } @@ - if (right > 0) { + if (right > 0) { + const w = Math.min(right * sx, width); rects.push({ @@ - x: baseX + width - right * transform[0][0], + x: baseX + width - w, @@ - width: right * transform[0][0], + width: w, }, }); } @@ - if (bottom > 0) { + if (bottom > 0) { + const h = Math.min(bottom * sy, height); rects.push({ @@ - y: baseY + height - bottom * transform[1][1], + y: baseY + height - h, @@ - height: bottom * transform[1][1], + height: h, }, }); } @@ - if (left > 0) { + if (left > 0) { + const w = Math.min(left * sx, width); rects.push({ @@ - width: left * transform[0][0], + width: w, }, }); }Also applies to: 81-121
134-143: SVG pattern ID collision across multiple overlaysStatic id "padding-diagonal-stripes" can collide. Generate a per-instance id and pass to children.
+import { useId } from "react"; @@ export function PaddingOverlay({ @@ }) { + const patternId = useId(); @@ - <SVGPatternDiagonalStripe - id="padding-diagonal-stripes" + <SVGPatternDiagonalStripe + id={patternId} @@ - /> + /> @@ - {isHovered && ( + {isHovered && ( <svg className="absolute inset-0 overflow-visible pointer-events-none"> <rect @@ - fill="url(#padding-diagonal-stripes)" + fill={`url(#${patternId})`} /> </svg> )}If
PaddingEdgeRegionis isolated, passpatternIdas a prop instead.Also applies to: 205-215
64-72: Offset defaultingMinor: default offset to [0,0] to reduce branching.
- const baseX = offset ? surfaceRect.x - offset[0] : 0; - const baseY = offset ? surfaceRect.y - offset[1] : 0; + const [ox, oy] = offset ?? [0, 0]; + const baseX = surfaceRect.x - ox; + const baseY = surfaceRect.y - oy;crates/grida-canvas/examples/taffy_layout_inset.rs (1)
12-24: Font robustnessSystem "Arial" can be missing on CI. Prefer embedded bytes (as in golden_layout_flex.rs) with a system fallback.
- let font_mgr = FontMgr::new(); - let typeface = font_mgr - .match_family_style("Arial", skia_safe::FontStyle::default()) - .unwrap_or_else(|| { - font_mgr - .legacy_make_typeface(None, skia_safe::FontStyle::default()) - .unwrap() - }); + let font_mgr = FontMgr::new(); + let typeface = font_mgr + .new_from_data(cg::fonts::embedded::geist::BYTES, None) + .or_else(|| font_mgr.match_family_style("Arial", skia_safe::FontStyle::default())) + .unwrap();crates/grida-canvas/src/node/scene_graph.rs (1)
210-224: Parent lookup is O(N); consider a reverse index.
get_parentlinearly scanslinks. For frequent calls, maintainparents: HashMap<NodeId, NodeId>updated in append/remove to get O(1) parent lookup.Also, confirm
SceneGraphremains an internal API so returningNodeId (u64)complies with “internal uses NodeId; public APIs use UserNodeId (String)”. As per coding guidelines.editor/grida-canvas/reducers/tools/target.ts (1)
136-175: Marquee selection rules updated — sensible defaults.
- Auto-exclude root-with-children unless selectable type.
- Include direct scene children.
- Require parent be hit and unlocked for nested nodes.
Looks correct. Consider memoizing parent lookups if this runs on large hit sets.
crates/grida-canvas/examples/golden_layout_flex_padding.rs (3)
21-39: ICB config reads clean; double-check field optionality.If
layout_wrap/alignments are Option types in this struct, assign withSome(...). Otherwise this is fine.
63-68: Avoid.unwrap()in example or assert intent.Both
get(id).cloned().unwrap()andget_children(...).unwrap()will panic on mismatch. If this binary is used as a golden generator, either keep explicitassert!(...)with a message or handleNoneto aid debugging.
102-108: Minor hygiene in helper: remove unused hash.
let _ = hasher.finish();computes but discards a value. Since IDs are assigned bySceneGraph, drop the hashing lines to avoid noise.Apply:
- use std::collections::hash_map::DefaultHasher; - use std::hash::{Hash, Hasher}; - let mut hasher = DefaultHasher::new(); - id.hash(&mut hasher); - let _ = hasher.finish(); // Generate ID but don't store it since it's not used + // NodeId is assigned by SceneGraph; no need to hash `id` here.Also applies to: 116-151
crates/grida-canvas/tests/geometry_cache.rs (2)
55-86: Test name vs. assertion mismatch (includes-children vs. fixed 100×100)The test name says “include children”, but assertions match only the container’s own layout box. If children can extend beyond 100×100 (rect at 50,50 sized 100×100), the union would be 150×150. Either:
- Rename the test to reflect “excludes children” (or “uses own layout box”), or
- Update assertions to expect the union of child bounds.
Apply one of:
-#[test] -fn container_world_bounds_include_children() { +#[test] +fn container_world_bounds_excludes_children() {or adjust expected width/height if world bounds are intended to include children.
44-48: Float equality in world transform comparisonsConsider tolerant comparison to avoid brittle failures from float math changes:
-assert_eq!( - cache.get_world_transform(&rect_id).unwrap().matrix, - expected.matrix -); +let got = cache.get_world_transform(&rect_id).unwrap().matrix; +let exp = expected.matrix; +assert!((got[0][2] - exp[0][2]).abs() < 1e-5 && (got[1][2] - exp[1][2]).abs() < 1e-5);editor/grida-canvas/query/index.ts (2)
347-365: Use lu_children for siblings; avoid O(N) scansYou already build lu_children; use it for sibling lookup to reduce scans and preserve declared order.
-// Filter all nodes that share the same parent but exclude the input node itself. -return Object.keys(context.lu_parent).filter( - (id) => context.lu_parent[id] === parent_id && id !== node_id -); +const children = context.lu_children[parent_id] ?? []; +return children.filter((id) => id !== node_id);
471-482: Add runtime deprecation warning for dev environmentThe function is already marked
@deprecatedin JSDoc. Adding a console.warn gate (dev-only) is a good optional refactor to improve discoverability. Both call sites follow the intended fallback pattern: usinggetTopIdWithinScenewhenscene_idis available, andgetRootIdwhen it's null.export function getRootId( /* ... */ ): NodeID | null { + if (process.env.NODE_ENV !== "production") { + // eslint-disable-next-line no-console + console.warn("getRootId is deprecated. Use getTopIdWithinScene instead."); + } // veryfy if exists if (context.lu_keys.includes(node_id)) {crates/grida-canvas/src/layout/mod.rs (1)
10-27: ComputedLayout PartialEq + From<&taffy::Layout> is correctMapping x/y/width/height matches taffy::Layout fields. Consider also implementing Fromtaffy::Layout for ergonomics.
+impl From<taffy::Layout> for ComputedLayout { + fn from(layout: taffy::Layout) -> Self { + (&layout).into() + } +}editor/grida-canvas-react/viewport/hooks/use-surface-gesture.ts (1)
14-41: Prevent click after drag: add cleanup and avoid stale timeoutsClear the timeout on unmount and store the id. Optional: switch to displacement threshold to avoid arbitrary 100ms.
-import { useRef } from "react"; +import { useEffect, useRef } from "react"; @@ - const should_prevent_click = useRef(false); + const should_prevent_click = useRef(false); + const resetTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); + useEffect(() => { + return () => { + if (resetTimerRef.current) { + clearTimeout(resetTimerRef.current); + resetTimerRef.current = null; + } + }; + }, []); @@ - onDragEnd: (e) => { + onDragEnd: (e) => { onDragEnd?.(e); - setTimeout(() => { + if (resetTimerRef.current) clearTimeout(resetTimerRef.current); + resetTimerRef.current = setTimeout(() => { should_prevent_click.current = false; - }, 100); + resetTimerRef.current = null; + }, 100); },If you want to use displacement, __useGesture provides
movement/distanceon drag events; gate suppression on a small threshold (e.g., >3px).editor/grida-canvas/reducers/document.reducer.ts (2)
1152-1157: Single-node align skips scene children; confirm intent and simplify conditionCurrent gate uses getTopIdWithinScene and bails when node is a scene child. If the goal is “align only when parent is not the scene”, check the immediate parent instead. This is clearer and avoids accidental skips if query behavior changes. Suggest:
- const top_id = dq.getTopIdWithinScene( - state.document_ctx, - node_id, - state.scene_id - ); - if (top_id && node_id !== top_id) { - const parent_node_id = dq.getParentId(state.document_ctx, node_id); - assert(parent_node_id, "parent node not found"); + const parent_node_id = dq.getParentId(state.document_ctx, node_id); + // Align only when immediate parent is not the scene (skip root-level nodes) + if (parent_node_id && parent_node_id !== state.scene_id) { + assert(parent_node_id, "parent node not found"); // ... existing rect/translate logic ... }
1369-1387: Magic padding and property resets in contain=true flow
- Padding = 16 for single-child containers is a hidden heuristic. Promote to a constant/config or infer from theme to avoid surprises.
- When resetting children to in-flow, you clear top/left/right/bottom; consider also clearing any layout_child overrides that could fight container flex (if present).
No blocking change required, but worth tightening before wider usage.
Also applies to: 1404-1417
crates/grida-canvas/src/painter/layer.rs (2)
290-295: Avoid panics on missing geometry; fall back or skip renderingget_world_bounds(...).expect("Geometry must exist") will panic if cache misses. Prefer a guarded path:
- If bounds are None, skip emitting the layer for this node (return FlattenResult::default()) or build a conservative rect based on node-local size.
- Log at debug level for visibility.
Applies similarly to Rectangle/Ellipse/Polygon/RegularPolygon/RegularStarPolygon/Line/SVGPath/Image/Error branches.
599-637: Normalize paint filtering for Line nodesOther shape branches filter invisible paints; Line uses raw n.strokes. Align for consistency and perf:
- strokes: n.strokes.clone(), + strokes: Self::filter_visible_paints(&n.strokes),crates/grida-canvas/src/layout/cache.rs (1)
1-46: LayoutResult API is clean and minimalThe wrapper around HashMap with insert/get and basic introspection is appropriate. Consider a read-only iterator if callers need bulk traversal, otherwise LGTM.
crates/grida-canvas/examples/golden_layout_flex_alignment.rs (2)
101-113: Hoist NodeFactory out of the loopNodeFactory::new() per scenario is unnecessary. Create once and reuse for minor perf/clarity gains.
135-139: Avoid unwrap on layout lookups in exampleUse expect with a helpful message to aid failures during engine changes:
- .map(|id| layout_result.get(id).cloned().unwrap()) + .map(|id| layout_result.get(id).cloned().expect("missing child layout"))crates/grida-canvas/examples/golden_layout_padding.rs (3)
21-24: Remove no-op hasher codeThe hash computation is unused. Drop it to reduce noise.
- let mut hasher = DefaultHasher::new(); - id.hash(&mut hasher); - let _ = hasher.finish(); // Generate ID but don't store it since it's not used + // ID hashing removed — not used
66-123: Production pipeline helper reads wellMapping ContainerNodeRec → InitialContainerNodeRec and computing layout via LayoutEngine is clear. One nit: replace unwraps on child layout access with expect(...) for clearer error messages, as in the other example.
377-391: Flex-grow used while feature is marked pending in PR objectivesScenario 7 sets layout_grow = 1.0 and width=None for some children. If flex-grow isn’t implemented yet (PR notes it pending), results may not match expectations. Either guard this scenario behind a feature flag or annotate the output as provisional.
editor/grida-canvas-react/viewport/ui/surface-distribution-overlay.tsx (4)
172-186: Avoid pattern ID collisions; make pattern instance-uniqueMultiple elements use id="gap-diagonal-stripes". IDs collide in the document and may resolve unpredictably. Generate a unique id per instance and bind fill inline.
Apply:
-import React, { useMemo, useState } from "react"; +import React, { useMemo, useState, useId } from "react"; @@ function GapWithHandle({ @@ }) { + const patternId = useId(); @@ <svg className="absolute inset-0 overflow-visible pointer-events-none"> - <SVGPatternDiagonalStripe - id="gap-diagonal-stripes" + <SVGPatternDiagonalStripe + id={patternId} className="text-workbench-accent-pink/50" patternWidth={1} patternSpacing={5} /> @@ - <rect + {/* tinted fill during drag */} + <rect x={0} y={0} width={r.width} height={r.height} - className="fill-transparent group-data-[is-gesture=true]/gap:fill-workbench-accent-pink/20 group-data-[highlighted=true]/gap:fill-[url(#gap-diagonal-stripes)]" + className="fill-transparent group-data-[is-gesture=true]/gap:fill-workbench-accent-pink/20" /> + {/* diagonal stripe fill on hover */} + <rect + x={0} + y={0} + width={r.width} + height={r.height} + fill={`url(#${patternId})`} + className="opacity-0 group-data-[highlighted=true]/gap:opacity-100" + /> </svg>Please QA hover/drag states after this change to ensure visuals match the prior behavior.
185-186: Unify Tailwind data attribute syntaxInconsistent quoting may break variants. Use one style:
- className="fill-transparent group-data-[is-gesture=true]/gap:fill-workbench-accent-pink/20 ..." + className="fill-transparent group-data-[is-gesture=true]/gap:fill-workbench-accent-pink/20 ..." @@ - className="opacity-100 group-data-[is-gesture='true']/gap:opacity-0" + className="opacity-100 group-data-[is-gesture=true]/gap:opacity-0"Also applies to: 194-195
4-4: Remove unused import
useGesture as __useGestureisn’t used in this file.-import { useGesture as __useGesture } from "@use-gesture/react";Run type-check to confirm no implicit reliance.
210-218: Prevent parent handlers from firing on drag startTo avoid selection/hover side-effects, also stop propagation:
const bind = useSurfaceGesture({ - onPointerDown: ({ event }) => { - event.preventDefault(); - }, + onPointerDown: ({ event }) => { + event.preventDefault(); + event.stopPropagation(); + }, onDragStart: ({ event }) => { - event.preventDefault(); + event.preventDefault(); + event.stopPropagation(); onGapGestureStart?.(axis); },editor/grida-canvas/editor.ts (1)
1165-1306: reLayout: robust, but make viewport-safe when clearing flex
- Clear path computes absolute rects before changing layout — correct.
- After converting to flow/absolute, children positions are set once — good.
Recommendations:
- Batch child positioning updates to a single dispatch array to reduce history noise.
- Optional: also clear container gaps/align even if not previously set (defensive, already handled in your patch).
Example batching:
-childrenWithRects.forEach(({ id, rect }) => { - ... - this.changeNodePropertyPositioning(id, { ... }); -}); +this.dispatch(childrenWithRects.map(({ id, rect }) => ({ + type: "node/change/positioning", + node_id: id, + position: "absolute", + left: cmath.quantize(rect.x - parentRect.x, 1), + top: cmath.quantize(rect.y - parentRect.y, 1), + right: undefined, + bottom: undefined, +})));Test cases:
- Container with 0 children -> no-throw, direction switch no-op.
- Mixed positioned children -> all become absolute with correct offsets.
crates/grida-canvas/src/painter/geometry.rs (3)
1-10: Docs over-promise vs current implementationHeader claims “build_shape requires resolved bounds; V2 nodes ALWAYS use provided bounds,” but Rectangle/Image/Line still use intrinsic sizes. Either:
- Update those branches to use
bounds.width/height, or- Soften the docs to reflect partial migration.
145-232: Make Rectangle/Image honor resolved bounds for consistencyTo match bounds-first pipeline and prevent shape/layout drift, prefer
bounds.width/heightfor these nodes too.Example:
- Node::Rectangle(n) => { - let rect = Rect::from_xywh(0.0, 0.0, n.size.width, n.size.height); + Node::Rectangle(n) => { + let rect = Rect::from_xywh(0.0, 0.0, bounds.width, bounds.height); let r = n.corner_radius; if !r.is_zero() { - let rrect = build_rrect(&n.to_own_shape()); + let rrect = build_rrect(&RRectShape { width: bounds.width, height: bounds.height, corner_radius: n.corner_radius }); PainterShape::from_rrect(rrect) } else { PainterShape::from_rect(rect) } } @@ - Node::Image(n) => { + Node::Image(n) => { let r = n.corner_radius; if (!r.is_zero()) { - let rrect = build_rrect(&n.to_own_shape()); + let rrect = build_rrect(&RRectShape { width: bounds.width, height: bounds.height, corner_radius: n.corner_radius }); PainterShape::from_rrect(rrect) } else { - let rect = Rect::from_xywh(0.0, 0.0, n.size.width, n.size.height); + let rect = Rect::from_xywh(0.0, 0.0, bounds.width, bounds.height); PainterShape::from_rect(rect) } }If legacy nodes intentionally ignore layout bounds, clarify the rule in the module docs and keep as-is.
277-317: boolean_operation_path usesexpectfor bounds — acceptable with pipeline contractGiven the “geometry must exist” guarantee, this is fine. If you prefer fail-soft, map missing to skip child instead of panic. Optional.
crates/grida-canvas/src/cache/geometry.rs (2)
1-12: Doc guarantee mentions “Inset nodes PANIC” — likely outdated.The module doesn’t special-case “Inset nodes” and the PR removed Anchors. Please update or drop this bullet to avoid misleading guarantees.
69-79: Default viewport in from_scene_with_paragraph_cache is arbitrary.Thread a real viewport from the caller or Scene to avoid mismatches between layout and geometry.
crates/grida-canvas/src/layout/engine.rs (1)
121-139: Minor: comment claims “no switch-case,” yet we match in get_schema_position().Not a blocker; reword to “universal mapping to styles; minimal matching for root offsets.”
crates/grida-canvas/src/node/factory.rs (1)
7-16: Doc note about placeholder ID is misleading.Node records don’t carry IDs here; SceneGraph assigns NodeId separately. Update or remove the placeholder ID note.
crates/grida-canvas/examples/golden_layout_flex.rs (1)
349-355: Ensure output directory exists before writing.Avoid failures when goldens/ isn’t present.
Apply this diff:
- let output_dir = std::env::var("CARGO_MANIFEST_DIR").unwrap_or_else(|_| ".".to_string()); - let output_path = format!("{}/goldens/layout_flex.png", output_dir); + let output_dir = std::env::var("CARGO_MANIFEST_DIR").unwrap_or_else(|_| ".".to_string()); + let goldens = format!("{}/goldens", output_dir); + let _ = std::fs::create_dir_all(&goldens); + let output_path = format!("{}/layout_flex.png", goldens); std::fs::write(&output_path, data.as_bytes()).unwrap(); println!("✓ Generated {}", output_path);crates/grida-canvas/src/painter/painter_debug_node.rs (1)
33-35: Enum-based shape dispatch looks good; consider avoiding clonesCloning full NodeRec for shape build may be heavier than needed. If this becomes hot, add a lightweight build path that takes a borrowed NodeRec.
Also applies to: 56-58, 88-90, 111-113, 286-288
crates/grida-canvas/src/layout/into_taffy.rs (1)
290-312: Absolute child insets from transformWorks for shape nodes that don’t carry basis; if we later add Inset basis to children, consider merging basis-derived insets here before transform-derived values.
editor/grida-canvas-react/viewport/surface.tsx (3)
105-114: SurfaceFragmentGroup ignores classNameProp accepts className but doesn’t use it. Either wire it or drop it.
-function SurfaceFragmentGroup({ - children, - hidden, -}: React.PropsWithChildren<{ className?: string; hidden?: boolean }>) { - if (hidden) return null; - return <>{children}</>; -} +function SurfaceFragmentGroup({ + children, + hidden, + className, +}: React.PropsWithChildren<{ className?: string; hidden?: boolean }>) { + if (hidden) return null; + return <div className={className}>{children}</div>; +}
169-191: Effect dependency for pointermove bindingUsing eventTargetRef.current in deps won’t reliably re-run when the ref node changes. Prefer eventTargetRef (stable) or a separate ref change observer.
- }, [eventTargetRef.current]); + }, [eventTargetRef]);
1105-1129: PreventDefault to preserve selectionThis mitigates selection flicker; be mindful it can also block inner content interactions. Consider scoping with a stricter condition or stopPropagation when appropriate.
crates/grida-canvas/src/node/schema.rs (3)
838-851: Expose x/y for containers via NodeTransformMixin.Several consumers rely on NodeTransformMixin; ContainerNodeRec doesn’t implement it anymore. Provide x()/y() derived from position basis to avoid downstream special‑cases.
Apply this:
impl ContainerNodeRec { @@ pub fn to_own_shape(&self) -> RRectShape { RRectShape { width: self.layout_dimensions.width.unwrap_or(0.0), height: self.layout_dimensions.height.unwrap_or(0.0), corner_radius: self.corner_radius, } } } + +impl NodeTransformMixin for ContainerNodeRec { + fn x(&self) -> f32 { + self.position.x().unwrap_or(0.0) + } + fn y(&self) -> f32 { + self.position.y().unwrap_or(0.0) + } +}
886-893: Also implement NodeRectMixin for containers.Returning a local rect aligns ContainerNodeRec with other shapes and simplifies editor hit‑testing.
Apply this:
impl ContainerNodeRec { @@ } + +impl NodeRectMixin for ContainerNodeRec { + fn rect(&self) -> Rectangle { + Rectangle { + x: 0.0, + y: 0.0, + width: self.layout_dimensions.width.unwrap_or(0.0), + height: self.layout_dimensions.height.unwrap_or(0.0), + } + } +}
612-626: Clarify Inset semantics for LayoutPositioningBasis::x/y.For Inset, x()/y() currently return left/top. That can be misleading when only right/bottom are set (common in insets). Prefer returning None for Inset or document explicitly that these are “left/top if present.”
Possible tweak:
- Self::Inset(inset) => Some(inset.left), + Self::Inset(inset) => if inset.left != 0.0 { Some(inset.left) } else { None },Same for y() with top.
| position: LayoutPositioningBasis::Cartesian(CGPoint::new( | ||
| node.base.left.unwrap_or(0.0), | ||
| node.base.top.unwrap_or(0.0), | ||
| )), | ||
| corner_radius: merge_corner_radius( |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Right/bottom insets are ignored when building Container.position; wire up the Inset basis.
Currently, ContainerNodeRec.position uses only left/top (Cartesian). If JSON supplies right/bottom (the new Inset model), they’re dropped, leading to incorrect geometry. Build LayoutPositioningBasis::Inset when any of right/bottom are present; otherwise fall back to Cartesian.
Apply this diff:
- position: LayoutPositioningBasis::Cartesian(CGPoint::new(
- node.base.left.unwrap_or(0.0),
- node.base.top.unwrap_or(0.0),
- )),
+ position: {
+ if node.base.right.is_some() || node.base.bottom.is_some() {
+ // Inset basis: preserve all four edges (missing edges default to 0.0)
+ LayoutPositioningBasis::Inset(EdgeInsets {
+ top: node.base.top.unwrap_or(0.0),
+ right: node.base.right.unwrap_or(0.0),
+ bottom: node.base.bottom.unwrap_or(0.0),
+ left: node.base.left.unwrap_or(0.0),
+ })
+ } else {
+ // Cartesian basis (x,y)
+ LayoutPositioningBasis::Cartesian(CGPoint::new(
+ node.base.left.unwrap_or(0.0),
+ node.base.top.unwrap_or(0.0),
+ ))
+ }
+ },Add a unit test to lock this in (example JSON with position absolute and right/bottom only).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
editor/grida-canvas-react/viewport/ui/surface-distribution-overlay.tsx (1)
49-69: Out-of-bounds access still possible in gap pairing loop.The guard condition has been improved from the previous review (now correctly checks
x.gapsinstead ofx.gap), but the loop boundary can still cause an out-of-bounds error. Whenx.gaps.length >= rects.length, accessingx_sorted[i + 1]at iterationi = rects.length - 1will be undefined.For N rectangles, you can have at most N-1 adjacent pairs. The loop should be bounded accordingly.
Apply this diff to prevent out-of-bounds access:
if (x && x.gaps && x.gaps.length > 0) { const x_sorted = [...rects].sort((a, b) => a.x - b.x); - for (let i = 0; i < x.gaps.length; i++) { + const pairCount = Math.min(x.gaps.length, x_sorted.length - 1); + for (let i = 0; i < pairCount; i++) { result.push({ axis: "x", a: x_sorted[i], b: x_sorted[i + 1], }); } } if (y && y.gaps && y.gaps.length > 0) { const y_sorted = [...rects].sort((a, b) => a.y - b.y); - for (let i = 0; i < y.gaps.length; i++) { + const pairCount = Math.min(y.gaps.length, y_sorted.length - 1); + for (let i = 0; i < pairCount; i++) { result.push({ axis: "y", a: y_sorted[i], b: y_sorted[i + 1], }); } }
🧹 Nitpick comments (4)
crates/grida-canvas/src/layout/tree.rs (1)
37-45: Consider removing orphaned taffy nodes to prevent resource accumulation.The bidirectional mapping cleanup correctly addresses the previous review concern. However, when
old_taffy_idis evicted from the mappings (line 39), the corresponding node remains in the underlyingTaffyTree. Over time, frequent re-insertions could accumulate orphaned nodes that consume memory but are never used.Consider removing the old node from the taffy tree:
// Clean up any existing mapping for this scene_node_id if let Some(old_taffy_id) = self.scene_to_taffy.insert(scene_node_id, taffy_id) { self.taffy_to_scene.remove(&old_taffy_id); + // Remove the orphaned node from the taffy tree + let _ = self.taffy.remove(old_taffy_id); }Also applies to: 61-69
editor/grida-canvas-react/viewport/ui/surface-distribution-overlay.tsx (1)
251-253: Consider returningnullinstead of empty fragment.Returning
nullis more idiomatic than<></>for conditional rendering and makes the intent clearer.Apply this diff:
if (!axis) { - return <></>; + return null; }editor/scaffolds/sidecontrol/controls/padding.tsx (2)
18-18: Consider initializingshowIndividualbased on value type.The toggle state is always initialized to
false, even when the incomingvalueis already a non-uniform object. Users with mixed padding values must manually expand the individual controls to see the breakdown.Apply this diff to show individual controls by default when the value is non-uniform:
- const [showIndividual, setShowIndividual] = useState(false); + const [showIndividual, setShowIndividual] = useState(typeof value !== "number");Note: The current behavior may be intentional UX design (always start collapsed), so evaluate based on your design requirements.
111-181: Excellent: Individual controls are fully accessible.All accessibility attributes requested in the previous review have been correctly added:
- Each input has a descriptive
aria-label(lines 125, 142, 159, 176)- Decorative labels have
aria-hidden="true"(lines 127, 144, 161, 178)The visual design (connected borders, separators) effectively communicates the grouped nature of the controls.
The four input sections share identical structure. You could optionally extract a helper component or map over an array like
["left", "top", "right", "bottom"], but the current explicit form is clear and maintainable for this small repetition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/grida-canvas/examples/grida_nested.rs(1 hunks)crates/grida-canvas/src/layout/tree.rs(1 hunks)editor/grida-canvas-react/viewport/hooks/use-surface-gesture.ts(1 hunks)editor/grida-canvas-react/viewport/ui/surface-distribution-overlay.tsx(1 hunks)editor/grida-canvas-react/viewport/ui/vector-region.tsx(2 hunks)editor/grida-canvas/query/index.ts(2 hunks)editor/grida-canvas/reducers/surface.reducer.ts(1 hunks)editor/scaffolds/sidecontrol/controls/padding.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- editor/grida-canvas/reducers/surface.reducer.ts
- editor/grida-canvas-react/viewport/ui/vector-region.tsx
- crates/grida-canvas/examples/grida_nested.rs
- editor/grida-canvas-react/viewport/hooks/use-surface-gesture.ts
🧰 Additional context used
📓 Path-based instructions (4)
editor/scaffolds/**
📄 CodeRabbit inference engine (AGENTS.md)
Place feature-specific larger components/pages/editors under editor/scaffolds
Files:
editor/scaffolds/sidecontrol/controls/padding.tsx
editor/grida-*/**
📄 CodeRabbit inference engine (AGENTS.md)
Use editor/grida-* directories to isolate domain-specific modules pending promotion to /packages
Files:
editor/grida-canvas-react/viewport/ui/surface-distribution-overlay.tsxeditor/grida-canvas/query/index.ts
crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust crates under /crates; this is the monorepo’s Rust workspace for the Canvas implementation
Files:
crates/grida-canvas/src/layout/tree.rs
crates/grida-canvas/**/*.rs
📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)
crates/grida-canvas/**/*.rs: All internal structs (e.g., NodeRecs, SceneGraph, caches) must use NodeId (u64) for identifiers
Public Rust APIs must accept and return UserNodeId (String) instead of NodeId for stability
Do not serialize internal NodeId values; only serialize stable UserNodeId in .grida files
Keep NodeId as u64 and UserNodeId as String consistently across the codebase
Files:
crates/grida-canvas/src/layout/tree.rs
🧠 Learnings (4)
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : Keep NodeId as u64 and UserNodeId as String consistently across the codebase
Applied to files:
crates/grida-canvas/src/layout/tree.rs
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : All internal structs (e.g., NodeRecs, SceneGraph, caches) must use NodeId (u64) for identifiers
Applied to files:
crates/grida-canvas/src/layout/tree.rs
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : Public Rust APIs must accept and return UserNodeId (String) instead of NodeId for stability
Applied to files:
crates/grida-canvas/src/layout/tree.rs
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : Do not serialize internal NodeId values; only serialize stable UserNodeId in .grida files
Applied to files:
crates/grida-canvas/src/layout/tree.rs
🧬 Code graph analysis (4)
editor/scaffolds/sidecontrol/controls/padding.tsx (2)
editor/scaffolds/sidecontrol/ui/number.tsx (1)
InputPropertyNumber(123-238)editor/components/lib/utils/index.ts (1)
cn(4-6)
editor/grida-canvas-react/viewport/ui/surface-distribution-overlay.tsx (6)
editor/grida-canvas-react/viewport/ui/distribution.ts (1)
ObjectsDistributionAnalysis(21-25)editor/grida-canvas-react/provider.tsx (2)
useTransformState(354-372)useGestureState(598-616)editor/components/lib/utils/index.ts (1)
cn(4-6)editor/grida-canvas-react/viewport/ui/svg-fill-patterns.tsx (1)
SVGPatternDiagonalStripe(30-52)editor/grida-canvas-react/viewport/hooks/use-surface-gesture.ts (1)
useSurfaceGesture(4-45)editor/grida-canvas-react/viewport/surface.tsx (1)
GapOverlay(1434-1508)
crates/grida-canvas/src/layout/tree.rs (3)
crates/grida-canvas/src/layout/engine.rs (1)
new(65-70)crates/grida-canvas/src/layout/cache.rs (4)
new(15-19)len(29-31)is_empty(33-35)default(43-45)crates/grida-canvas/examples/taffy_layout_inset.rs (1)
compute_layout(317-375)
editor/grida-canvas/query/index.ts (2)
packages/grida-canvas-schema/grida.ts (2)
INodesRepositoryRuntimeHierarchyContext(898-898)NodeID(1100-1100)editor/grida-canvas/editor.i.ts (1)
NodeID(134-134)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cargo test
- GitHub Check: test
🔇 Additional comments (10)
editor/grida-canvas/query/index.ts (2)
467-482: LGTM! Deprecation and typo fix applied correctly.The function rename to
getRootIdwith a clear deprecation notice improves API clarity. The typo fix at line 475 ("verify") resolves the previous review concern.
484-541: LGTM! Scene-scoping logic is correct.The implementation properly handles all edge cases:
- Returns
nullfor non-existent nodes or when querying the scene itself- Correctly returns
nullwhen the node is not under the scene (line 537) — this addresses the critical scene-scoping bug from the previous review- Properly identifies the direct child of the scene in the ancestor chain
The comprehensive documentation and examples make the function's behavior clear.
crates/grida-canvas/src/layout/tree.rs (3)
9-16: Previous visibility concern addressed.The struct and all methods are now
pub(crate), which correctly keeps NodeId internal to the crate. This aligns with the coding guidelines that internal structs can use NodeId (u64), while public APIs should use UserNodeId (String).Based on learnings
75-135: LGTM!The methods correctly delegate to the underlying TaffyTree and provide clean abstractions for layout computation and querying. Test-only accessors are properly gated with
#[cfg(test)], and utility methods are appropriately marked.
137-345: Excellent test coverage.The tests thoroughly validate the bidirectional mapping lifecycle, including the cleanup scenarios that were flagged in the previous review. The tests for re-insertion and multiple scene IDs provide confidence in the mapping consistency.
editor/scaffolds/sidecontrol/controls/padding.tsx (5)
1-7: LGTM: Imports are appropriate.All imported dependencies are used correctly throughout the component.
41-59: LGTM: Value derivation logic is correct.The
uniformValueandplaceholdercomputations correctly handle both uniform and mixed padding values, providing clear visual feedback when sides differ.
61-77: LGTM: Event handlers correctly normalize padding values.Both handlers properly guard against
undefinedand emit the correct payload shape. The explicit object construction inhandleIndividualChangeis clear and maintainable.
80-109: Excellent: Accessibility improvements implemented.All accessibility attributes requested in the previous review have been correctly added:
- Uniform input has
aria-label="Padding all sides"(line 92)- Toggle has
aria-label="Toggle individual padding controls"(line 101)- Decorative icon has
aria-hidden="true"(line 105)Screen readers will now properly announce the purpose of each control.
186-195: LGTM: Label component is correctly marked as decorative.The helper component properly uses
aria-hidden="true"since the single-letter labels are purely visual hints (the actual inputs have descriptivearia-labelattributes).
…th NodeRectMixin for SVGPathNode, enhance layout engine to utilize schema dimensions for various node types, and implement manual layout results for non-layout nodes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/grida-canvas/src/layout/engine.rs (2)
76-106: [Duplicate] Swallowed Taffy errors in compute().Line 92 ignores Taffy layout computation errors with
let _ =, which can lead to incomplete layout results and unclear error propagation. This was previously flagged in the earlier review.As noted in the previous review, consider returning a
Result:- ) -> &LayoutResult { + ) -> Result<&LayoutResult, taffy::TaffyError> { // ... - let _ = self.tree.compute_layout( + self.tree.compute_layout( root_taffy_id, taffy::Size { width: AvailableSpace::Definite(viewport_size.width), height: AvailableSpace::Definite(viewport_size.height), }, - ); + )?; // ... - &self.result + Ok(&self.result)
59-62: [Duplicate] Public API exposes NodeId; needs UserNodeId facade.The
LayoutEngine::result()method at line 109 returns&LayoutResult, which uses internalNodeId(u64) as keys. Per coding guidelines and previous review comments, public Rust APIs should useUserNodeId(String) instead.This was previously flagged in the earlier review with a suggestion to add a UserNodeId-facing facade or provide accessors like
get_result_by_user_id(&self, user_id: &str) -> Option<&LayoutValue>.As per coding guidelines
Based on learningsAlso applies to: 108-111
🧹 Nitpick comments (1)
crates/grida-canvas/src/node/schema.rs (1)
1443-1467: Performance warning: SVGPath bounds computation is expensive and uncached.The
rect()implementation forSVGPathNodeRecparses SVG path data via Skia on every call. The documentation correctly warns about this, but the layout engine'sget_schema_size()method (in engine.rs line 135) calls this during layout computation.Verify that:
- Layout computation doesn't call this repeatedly for the same node
- Consider caching the result if this becomes a bottleneck
The implementation is correct and properly documented, but worth monitoring for performance in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/grida-canvas/src/cache/geometry.rs(13 hunks)crates/grida-canvas/src/layout/engine.rs(1 hunks)crates/grida-canvas/src/node/schema.rs(28 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust crates under /crates; this is the monorepo’s Rust workspace for the Canvas implementation
Files:
crates/grida-canvas/src/layout/engine.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/node/schema.rs
crates/grida-canvas/**/*.rs
📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)
crates/grida-canvas/**/*.rs: All internal structs (e.g., NodeRecs, SceneGraph, caches) must use NodeId (u64) for identifiers
Public Rust APIs must accept and return UserNodeId (String) instead of NodeId for stability
Do not serialize internal NodeId values; only serialize stable UserNodeId in .grida files
Keep NodeId as u64 and UserNodeId as String consistently across the codebase
Files:
crates/grida-canvas/src/layout/engine.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/node/schema.rs
🧠 Learnings (4)
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : Public Rust APIs must accept and return UserNodeId (String) instead of NodeId for stability
Applied to files:
crates/grida-canvas/src/layout/engine.rs
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : Keep NodeId as u64 and UserNodeId as String consistently across the codebase
Applied to files:
crates/grida-canvas/src/layout/engine.rs
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : All internal structs (e.g., NodeRecs, SceneGraph, caches) must use NodeId (u64) for identifiers
Applied to files:
crates/grida-canvas/src/layout/engine.rs
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : Do not serialize internal NodeId values; only serialize stable UserNodeId in .grida files
Applied to files:
crates/grida-canvas/src/layout/engine.rs
🧬 Code graph analysis (3)
crates/grida-canvas/src/layout/engine.rs (7)
crates/grida-canvas/src/node/schema.rs (37)
layout(878-884)new(244-263)rect(747-747)rect(809-816)rect(993-1000)rect(1123-1130)rect(1257-1264)rect(1449-1466)rect(1533-1535)rect(1655-1662)rect(1786-1793)x(612-618)x(730-730)x(799-801)x(983-985)x(1113-1115)x(1247-1249)x(1434-1436)x(1562-1564)x(1645-1647)x(1776-1778)x(1991-1993)y(620-626)y(731-731)y(803-805)y(987-989)y(1117-1119)y(1251-1253)y(1438-1440)y(1566-1568)y(1649-1651)y(1780-1782)y(1995-1997)default(63-70)default(373-375)default(487-497)default(636-638)crates/grida-canvas/src/layout/tree.rs (2)
taffy(126-128)new(19-25)crates/grida-canvas/src/cache/geometry.rs (2)
new(59-63)from_scene_with_layout(81-106)crates/grida-canvas/src/node/scene_graph.rs (3)
new(69-75)roots(206-208)default(360-362)crates/grida-canvas/src/layout/cache.rs (3)
new(15-19)default(43-45)get(25-27)crates/grida-canvas/src/node/repository.rs (3)
new(15-20)default(80-82)get(37-39)crates/grida-canvas/src/layout/into_taffy.rs (17)
node_to_taffy_style(272-288)from(34-39)from(44-49)from(54-64)from(69-76)from(82-89)from(94-101)from(133-138)from(142-160)from(166-265)from(316-327)from(332-345)from(350-359)from(364-373)from(378-387)from(392-401)from(406-416)
crates/grida-canvas/src/cache/geometry.rs (2)
crates/grida-canvas/src/node/schema.rs (33)
layout(878-884)new(244-263)rect(747-747)rect(809-816)rect(993-1000)rect(1123-1130)rect(1257-1264)rect(1449-1466)rect(1533-1535)rect(1655-1662)rect(1786-1793)x(612-618)x(730-730)x(799-801)x(983-985)x(1113-1115)x(1247-1249)x(1434-1436)x(1562-1564)x(1645-1647)x(1776-1778)x(1991-1993)y(620-626)y(731-731)y(803-805)y(987-989)y(1117-1119)y(1251-1253)y(1438-1440)y(1566-1568)y(1649-1651)y(1780-1782)y(1995-1997)crates/grida-canvas/src/layout/engine.rs (2)
new(65-70)result(109-111)
crates/grida-canvas/src/node/schema.rs (5)
packages/grida-canvas-cg/lib.ts (3)
Axis(582-582)MainAxisAlignment(589-596)CrossAxisAlignment(603-603)crates/grida-canvas/src/cg/types.rs (19)
new(20-22)new(817-822)new(1279-1285)new(1638-1645)default(43-45)default(124-126)default(146-148)default(278-280)default(342-344)default(370-372)default(386-388)default(416-418)default(440-442)default(542-544)zero(16-18)zero(505-512)zero(801-806)zero(873-875)zero(914-916)crates/grida-canvas-wasm/lib/api.d.ts (1)
Rectangle(26-31)crates/grida-canvas/src/vectornetwork/vn.rs (1)
bounds(438-476)crates/grida-canvas/src/shape/polygon.rs (1)
polygon_bounds(64-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cargo test
- GitHub Check: test
🔇 Additional comments (12)
crates/grida-canvas/src/node/schema.rs (5)
165-376: LGTM! Well-designed layout model with clear documentation.The UniformNodeLayout struct provides a comprehensive, geometry-first layout system with:
- Excellent documentation explaining the design philosophy and rationale
- Clean separation between positioning, dimensions, container properties, and child properties
- Consistent builder pattern for ergonomic construction
- Proper merge methods for composing layout styles from different sources
The API design is solid and aligns well with the PR's goal of supporting both freeform graphics and flex layout.
378-592: Excellent separation of container and child layout concerns.The split between
LayoutContainerStyle(parent-side properties) andLayoutChildStyle(child-side properties) is well-designed and clearly documented. This mirrors CSS's model where:
- Container properties affect how children are laid out
- Child properties affect how a node participates in its parent's layout
The documentation explicitly clarifies this relationship and provides examples, making the API intuitive for users familiar with flexbox.
838-892: Container layout integration looks solid.The additions to
ContainerNodeRecproperly integrate the layout system:
- New fields (
rotation,position,layout_container,layout_dimensions,layout_child) follow the layout model structure- The
layout()method correctly merges styles from container, child, dimensions, and position sources- The
to_own_shape()update properly useslayout_dimensionswith safe fallback to 0.0
919-938: InitialContainer design aligns well with web standards.The
InitialContainerNodeRecis well-designed:
- Documentation clearly explains its role as a viewport-filling flex container (similar to HTML's
<html>element)- Contains only structural/layout properties (no visual properties), which is appropriate
- Direct children are positioned by layout engine, deeper descendants use normal geometry
This design provides a clean foundation for viewport-based layout.
957-960: Consistent layout_child integration across all leaf node types.The addition of
layout_child: Option<LayoutChildStyle>across all leaf node types is well-executed:
- Consistent field type and documentation across 8 different node types
- Optional field appropriately handles nodes that don't participate in layout
- Documentation clearly states the purpose
This uniform approach makes the API predictable and easy to understand.
Also applies to: 1046-1049, 1087-1090, 1187-1190, 1507-1510, 1629-1632, 1760-1763, 1852-1854
crates/grida-canvas/src/layout/engine.rs (2)
113-325: Helper methods are well-structured and systematically handle all node types.The layout engine's helper methods demonstrate good design:
get_schema_size()andget_schema_position()exhaustively handle all node variantsshould_participate_in_taffy()clearly separates layout-aware from static nodesbuild_taffy_subtree()properly filters non-participating nodes and handles viewport sizing for ICBextract_all_layouts()implements the critical infinite canvas support by correcting root positions (lines 291-296) and creating manual layout results for non-Taffy nodes (lines 300-316)The infinite canvas post-processing approach (correcting Taffy's (0,0) root positions to schema positions) is well-documented and keeps layout logic centralized in the engine.
328-1337: Excellent test coverage with comprehensive scenarios.The test suite is thorough and well-structured:
- 14 test cases covering diverse scenarios: mixed node types, nested containers, ICB behavior, absolute positioning, wrapping, alignment, and root positioning
- Integration test (lines 1262-1336) validates the full pipeline from SceneGraph → LayoutEngine → GeometryCache
- Edge case coverage: empty containers, multiple roots, mixed absolute/relative children
- Design verification: Tests confirm
flex_shrink: 0.0default and no-shrink overflow behavior, which aligns with design goalsThe descriptive test names and clear assertion messages make the test suite maintainable and self-documenting.
crates/grida-canvas/src/cache/geometry.rs (5)
1-51: Clear pipeline guarantees and context design.The module documentation updates establish clear contracts:
- Every node gets a GeometryEntry
- All transforms are resolved (no None/fallbacks)
- Missing layout for Inset nodes is a PANIC (indicates LayoutEngine bug)
- Missing geometry when accessed is a PANIC (indicates GeometryCache bug)
The
GeometryBuildContextstruct is appropriately minimal, carrying only viewport_size through the recursive geometry construction. This keeps the threading of context simple and focused.
69-106: Layout integration maintains backward compatibility.The constructor updates are well-designed:
from_scene_with_paragraph_cache()(lines 73-79) maintains backward compatibility by calling the newfrom_scene_with_layout()withNonefor layout_resultfrom_scene_with_layout()(lines 81-106) provides the new layout-aware path- Default viewport (1920x1080) is reasonable for fallback cases
- Both
layout_resultandcontextare consistently threaded through recursive callsThis approach allows existing code to work unchanged while enabling new layout-driven geometry construction.
188-238: InitialContainer geometry implementation aligns with layout model.The
InitialContainergeometry handling correctly implements the ICB (Initial Containing Block) concept:
- Uses viewport size from context (lines 191)
- Positioned at origin with identity transform (line 193)
- Recursively builds children geometries that may use computed layouts (lines 206-220)
- Computes union of children bounds (line 219)
- No visual effects applied (line 223), which is correct since ICB is purely structural
This implementation complements the layout engine's ICB viewport sizing.
305-375: Container layout integration correctly enforces pipeline guarantees.The updated Container handling properly implements layout-aware geometry:
- Lines 307-312: When layout_result exists, uses computed position/size with
expect()that enforces the module's panic guarantee ("Container must have layout result when layout engine is used")- Lines 313-320: Fallback to schema provides backward compatibility when layout engine isn't active
- Line 322: Transform construction correctly combines layout position with schema rotation
- Line 355: Children are recursively built with same layout_result for consistency
The dual-path approach (layout vs schema) maintains backward compatibility while supporting the new layout system.
423-491: Leaf node layout integration is systematic and well-structured.The updated leaf node handling implements clean layout integration:
- Lines 424-446: Systematically extracts schema transform and size for all 10 leaf node types
- Lines 451-463: Clear precedence: layout result overrides schema when available, otherwise uses schema transform
- Line 465: Correctly constructs transform combining layout position with schema rotation
This approach ensures:
- Nodes participating in layout (with layout_child field) get layout-computed positions
- Non-participating nodes fall back to schema transforms
- All node types are handled uniformly
The pattern is consistent and maintainable.
| #[derive(Debug, Clone)] | ||
| pub enum LayoutPositioningBasis { | ||
| /// Cartesian position mode is the default mode. | ||
| /// In this mode, the position is specified using x and y coordinates. | ||
| Cartesian(CGPoint), | ||
| /// Inset position mode is used when the position is specified using left, right, top, and bottom insets. | ||
| Inset(EdgeInsets), | ||
| /// Anchored position mode is used when the position is specified using left, right, top, and bottom insets. | ||
| /// In this mode, the position is specified using left, right, top, and bottom insets. | ||
| #[deprecated(note = "will be implemented later")] | ||
| Anchored, | ||
| } | ||
|
|
||
| impl LayoutPositioningBasis { | ||
| pub fn zero() -> Self { | ||
| Self::Cartesian(CGPoint::zero()) | ||
| } | ||
|
|
||
| pub fn x(&self) -> Option<f32> { | ||
| match self { | ||
| Self::Cartesian(point) => Some(point.x), | ||
| Self::Inset(inset) => Some(inset.left), | ||
| Self::Anchored => unreachable!("Anchored positioning is not supported"), | ||
| } | ||
| } | ||
|
|
||
| pub fn y(&self) -> Option<f32> { | ||
| match self { | ||
| Self::Cartesian(point) => Some(point.y), | ||
| Self::Inset(inset) => Some(inset.top), | ||
| Self::Anchored => unreachable!("Anchored positioning is not supported"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsafe use of unreachable!() with deprecated variant.
The Anchored variant is marked deprecated but can still be constructed (the enum is public). Using unreachable!() on lines 616 and 624 will panic if anyone constructs this variant, which violates Rust safety expectations for deprecated APIs.
Per the PR description, the anchor model is NOT supported and removed from the roadmap. Consider:
- Option 1 (preferred): Remove the
Anchoredvariant entirely - Option 2: Replace
unreachable!()withpanic!()orexpect()with a clear message explaining deprecation - Option 3: Make the enum
#[non_exhaustive]to prevent external construction
Apply this diff if choosing Option 2:
pub fn x(&self) -> Option<f32> {
match self {
Self::Cartesian(point) => Some(point.x),
Self::Inset(inset) => Some(inset.left),
- Self::Anchored => unreachable!("Anchored positioning is not supported"),
+ Self::Anchored => panic!("Anchored positioning is deprecated and not supported. Use Cartesian or Inset instead."),
}
}
pub fn y(&self) -> Option<f32> {
match self {
Self::Cartesian(point) => Some(point.y),
Self::Inset(inset) => Some(inset.top),
- Self::Anchored => unreachable!("Anchored positioning is not supported"),
+ Self::Anchored => panic!("Anchored positioning is deprecated and not supported. Use Cartesian or Inset instead."),
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Debug, Clone)] | |
| pub enum LayoutPositioningBasis { | |
| /// Cartesian position mode is the default mode. | |
| /// In this mode, the position is specified using x and y coordinates. | |
| Cartesian(CGPoint), | |
| /// Inset position mode is used when the position is specified using left, right, top, and bottom insets. | |
| Inset(EdgeInsets), | |
| /// Anchored position mode is used when the position is specified using left, right, top, and bottom insets. | |
| /// In this mode, the position is specified using left, right, top, and bottom insets. | |
| #[deprecated(note = "will be implemented later")] | |
| Anchored, | |
| } | |
| impl LayoutPositioningBasis { | |
| pub fn zero() -> Self { | |
| Self::Cartesian(CGPoint::zero()) | |
| } | |
| pub fn x(&self) -> Option<f32> { | |
| match self { | |
| Self::Cartesian(point) => Some(point.x), | |
| Self::Inset(inset) => Some(inset.left), | |
| Self::Anchored => unreachable!("Anchored positioning is not supported"), | |
| } | |
| } | |
| pub fn y(&self) -> Option<f32> { | |
| match self { | |
| Self::Cartesian(point) => Some(point.y), | |
| Self::Inset(inset) => Some(inset.top), | |
| Self::Anchored => unreachable!("Anchored positioning is not supported"), | |
| } | |
| } | |
| } | |
| #[derive(Debug, Clone)] | |
| pub enum LayoutPositioningBasis { | |
| /// Cartesian position mode is the default mode. | |
| /// In this mode, the position is specified using x and y coordinates. | |
| Cartesian(CGPoint), | |
| /// Inset position mode is used when the position is specified using left, right, top, and bottom insets. | |
| Inset(EdgeInsets), | |
| /// Anchored position mode is used when the position is specified using left, right, top, and bottom insets. | |
| /// In this mode, the position is specified using left, right, top, and bottom insets. | |
| #[deprecated(note = "will be implemented later")] | |
| Anchored, | |
| } | |
| impl LayoutPositioningBasis { | |
| pub fn zero() -> Self { | |
| Self::Cartesian(CGPoint::zero()) | |
| } | |
| pub fn x(&self) -> Option<f32> { | |
| match self { | |
| Self::Cartesian(point) => Some(point.x), | |
| Self::Inset(inset) => Some(inset.left), | |
| Self::Anchored => panic!("Anchored positioning is deprecated and not supported. Use Cartesian or Inset instead."), | |
| } | |
| } | |
| pub fn y(&self) -> Option<f32> { | |
| match self { | |
| Self::Cartesian(point) => Some(point.y), | |
| Self::Inset(inset) => Some(inset.top), | |
| Self::Anchored => panic!("Anchored positioning is deprecated and not supported. Use Cartesian or Inset instead."), | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/node/schema.rs around lines 594 to 627, the enum's
deprecated Anchored variant and its unreachable!() arms are unsafe because the
enum is public; remove the Anchored variant entirely (delete the #[deprecated]
Anchored entry and its doc lines), update any documentation/comments referencing
Anchored, and adjust the x() and y() match arms to only handle Cartesian and
Inset (returning Some values as before) so no unreachable!() remains; run cargo
build/tests and fix any call sites that referenced
LayoutPositioningBasis::Anchored.
… engine to correctly position nodes without layout_child, and update related tests for positioning verification.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/grida-canvas/src/io/io_figma.rs (2)
1494-1499: Use Paints directly for LineNodeRec.strokes; drop into_iter().collect().LineNodeRec.strokes is Paints. Converting Paints into an iterator and collecting back is unnecessary and may type-mismatch if FromIterator isn’t implemented.
- strokes: self - .convert_strokes(Some(&origin.strokes)) - .into_iter() - .collect(), + strokes: self.convert_strokes(Some(&origin.strokes)),
1283-1376: Vector children are not linked; paths become orphans in the SceneGraph.convert_vector() inserts child SVGPath nodes but never records the parent→children link. As a result, the returned Container won't own those paths in SceneGraph. Mirror the pattern used in convert_frame/instance/component/section: obtain the container's internal id and insert the children in self.links before returning.
Apply this patch inside convert_vector(), right before returning the container:
- // Create a group node containing all the path nodes - { + // Create a group (container) node containing all the path nodes + { let transform = Self::convert_transform(origin.relative_transform.as_ref()); let size = Self::convert_size(origin.size.as_ref()); + // Wire up children to this vector container + let self_id = self.get_or_create_internal_id(&origin.id); + if !children.is_empty() { + self.links.insert(self_id.clone(), children); + } Ok(Node::Container(ContainerNodeRec {
♻️ Duplicate comments (4)
crates/grida-canvas/src/layout/engine.rs (2)
72-81: Public API exposes NodeId-keyed results; add a UserNodeId-facing facadeKeep NodeId internal; expose getters by UserNodeId (String) or a read-only view that resolves UserNodeId→NodeId.
As per coding guidelines
#!/bin/bash # Locate public NodeId usages in layout result APIs rg -n -C2 -P 'pub\s+struct\s+LayoutResult|pub\s+fn\s+\w+\s*\([^)]*NodeId' crates/grida-canvas/src/layout rg -n -C2 -P 'pub\s+fn\s+result\s*\(' crates/grida-canvas/src/layout/engine.rsAlso applies to: 108-112
92-99: Swallowed Taffy errors in compute(); propagate or surface themIgnoring
compute_layouterrors hides layout failures and can produce silent fallbacks.Apply this incremental, non-breaking wrapper and a fallible variant:
- pub fn compute( + pub fn compute( &mut self, scene: &crate::node::schema::Scene, viewport_size: Size, ) -> &LayoutResult { - // Clear previous state + // Keep signature; log on error in debug. Prefer try_compute() below. + match self.try_compute(scene, viewport_size) { + Ok(_) => {} + Err(e) => { + #[cfg(debug_assertions)] + eprintln!("LayoutEngine::compute: taffy error: {:?}", e); + } + } + &self.result + } + + pub fn try_compute( + &mut self, + scene: &crate::node::schema::Scene, + viewport_size: Size, + ) -> Result<&LayoutResult, taffy::TaffyError> { + // Clear previous state self.tree.clear(); self.result.clear(); @@ - let _ = self.tree.compute_layout( + self.tree.compute_layout( root_taffy_id, taffy::Size { width: AvailableSpace::Definite(viewport_size.width), height: AvailableSpace::Definite(viewport_size.height), }, - ); + )?; @@ - &self.result + Ok(&self.result) }crates/grida-canvas/src/io/io_grida.rs (1)
1160-1167: Right/bottom insets ignored; build Inset basis when provided.Position defaults to Cartesian(left/top) even when right/bottom exist, losing the intended Inset model.
Apply this:
- position: LayoutPositioningBasis::Cartesian(CGPoint::new( - node.base.left.unwrap_or(0.0), - node.base.top.unwrap_or(0.0), - )), + position: { + if node.base.right.is_some() || node.base.bottom.is_some() { + LayoutPositioningBasis::Inset(EdgeInsets { + top: node.base.top.unwrap_or(0.0), + right: node.base.right.unwrap_or(0.0), + bottom: node.base.bottom.unwrap_or(0.0), + left: node.base.left.unwrap_or(0.0), + }) + } else { + LayoutPositioningBasis::Cartesian(CGPoint::new( + node.base.left.unwrap_or(0.0), + node.base.top.unwrap_or(0.0), + )) + } + },Add a unit test (container absolute with only right/bottom) to lock behavior. Based on learnings.
crates/grida-canvas/src/node/schema.rs (1)
594-627: Remove or safely handle deprecated Anchored; unreachable!() is unsafe API.The public Anchored variant still exists and x()/y() use unreachable!(), risking panics.
If removal isn’t feasible now, at least replace with a clear panic:
- Self::Anchored => unreachable!("Anchored positioning is not supported"), + Self::Anchored => panic!("Anchored positioning is deprecated and not supported. Use Cartesian or Inset."),Do the same in y(). Prefer removing Anchored entirely per PR decision. As per coding guidelines.
🧹 Nitpick comments (2)
crates/grida-canvas/src/node/factory.rs (1)
196-205: ICB defaults say “Normal” (non‑flex); ensure mapping honors thisInto-taffy currently forces ICB to Flex regardless of
layout_mode. See proposed fix there.crates/grida-canvas/src/io/io_figma.rs (1)
221-332: Deduplicate repeated From<…::LayoutPositioning> impls via a small macro.Twelve nearly identical impl blocks are hard to maintain. Consider a local macro to generate them.
macro_rules! impl_figma_lp { ($($ty:path),+ $(,)?) => {$( impl From<&$ty> for LayoutPositioning { fn from(p: &$ty) -> Self { match p { <$ty>::Auto => LayoutPositioning::Auto, <$ty>::Absolute => LayoutPositioning::Absolute, } } } )+}; } impl_figma_lp!( figma_api::models::vector_node::LayoutPositioning, figma_api::models::component_node::LayoutPositioning, figma_api::models::instance_node::LayoutPositioning, figma_api::models::section_node::LayoutPositioning, figma_api::models::frame_node::LayoutPositioning, figma_api::models::text_node::LayoutPositioning, figma_api::models::star_node::LayoutPositioning, figma_api::models::line_node::LayoutPositioning, figma_api::models::ellipse_node::LayoutPositioning, figma_api::models::regular_polygon_node::LayoutPositioning, figma_api::models::rectangle_node::LayoutPositioning, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/grida-canvas/examples/grida_vector.rs(10 hunks)crates/grida-canvas/src/io/io_figma.rs(17 hunks)crates/grida-canvas/src/io/io_grida.rs(23 hunks)crates/grida-canvas/src/layout/engine.rs(1 hunks)crates/grida-canvas/src/layout/into_taffy.rs(3 hunks)crates/grida-canvas/src/node/factory.rs(10 hunks)crates/grida-canvas/src/node/schema.rs(30 hunks)crates/grida-canvas/tests/vector_corner_radius.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust crates under /crates; this is the monorepo’s Rust workspace for the Canvas implementation
Files:
crates/grida-canvas/src/layout/engine.rscrates/grida-canvas/tests/vector_corner_radius.rscrates/grida-canvas/src/node/factory.rscrates/grida-canvas/src/layout/into_taffy.rscrates/grida-canvas/src/io/io_figma.rscrates/grida-canvas/src/io/io_grida.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/examples/grida_vector.rs
crates/grida-canvas/**/*.rs
📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)
crates/grida-canvas/**/*.rs: All internal structs (e.g., NodeRecs, SceneGraph, caches) must use NodeId (u64) for identifiers
Public Rust APIs must accept and return UserNodeId (String) instead of NodeId for stability
Do not serialize internal NodeId values; only serialize stable UserNodeId in .grida files
Keep NodeId as u64 and UserNodeId as String consistently across the codebase
Files:
crates/grida-canvas/src/layout/engine.rscrates/grida-canvas/tests/vector_corner_radius.rscrates/grida-canvas/src/node/factory.rscrates/grida-canvas/src/layout/into_taffy.rscrates/grida-canvas/src/io/io_figma.rscrates/grida-canvas/src/io/io_grida.rscrates/grida-canvas/src/node/schema.rscrates/grida-canvas/examples/grida_vector.rs
🧠 Learnings (5)
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : Public Rust APIs must accept and return UserNodeId (String) instead of NodeId for stability
Applied to files:
crates/grida-canvas/src/layout/engine.rs
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : Keep NodeId as u64 and UserNodeId as String consistently across the codebase
Applied to files:
crates/grida-canvas/src/layout/engine.rs
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : All internal structs (e.g., NodeRecs, SceneGraph, caches) must use NodeId (u64) for identifiers
Applied to files:
crates/grida-canvas/src/layout/engine.rs
📚 Learning: 2025-10-16T03:19:20.557Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-10-16T03:19:20.557Z
Learning: Applies to crates/grida-canvas/**/*.rs : Do not serialize internal NodeId values; only serialize stable UserNodeId in .grida files
Applied to files:
crates/grida-canvas/src/layout/engine.rs
📚 Learning: 2025-09-15T10:31:32.864Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-09-15T10:31:32.864Z
Learning: Applies to crates/grida-canvas-fonts/tests/serde_test.rs : Put JSON serialization tests in tests/serde_test.rs
Applied to files:
crates/grida-canvas/src/io/io_grida.rs
🧬 Code graph analysis (6)
crates/grida-canvas/src/layout/engine.rs (7)
crates/grida-canvas/src/node/schema.rs (37)
layout(878-884)new(244-263)rect(747-747)rect(809-816)rect(993-1000)rect(1123-1130)rect(1257-1264)rect(1455-1472)rect(1539-1541)rect(1661-1668)rect(1792-1799)x(612-618)x(730-730)x(799-801)x(983-985)x(1113-1115)x(1247-1249)x(1440-1442)x(1568-1570)x(1651-1653)x(1782-1784)x(1997-1999)y(620-626)y(731-731)y(803-805)y(987-989)y(1117-1119)y(1251-1253)y(1444-1446)y(1572-1574)y(1655-1657)y(1786-1788)y(2001-2003)default(63-70)default(373-375)default(487-497)default(636-638)crates/grida-canvas/src/layout/tree.rs (2)
taffy(126-128)new(19-25)crates/grida-canvas/src/cache/geometry.rs (2)
new(59-63)from_scene_with_layout(81-106)crates/grida-canvas/src/node/scene_graph.rs (3)
new(69-75)roots(206-208)default(360-362)crates/grida-canvas/src/layout/cache.rs (3)
new(15-19)default(43-45)get(25-27)crates/grida-canvas/src/node/repository.rs (3)
new(15-20)default(80-82)get(37-39)crates/grida-canvas/src/layout/into_taffy.rs (17)
node_to_taffy_style(272-296)from(34-39)from(44-49)from(54-64)from(69-76)from(82-89)from(94-101)from(133-138)from(142-160)from(166-265)from(324-335)from(340-353)from(358-367)from(372-381)from(386-395)from(400-409)from(414-424)
crates/grida-canvas/src/node/factory.rs (2)
crates/grida-canvas/src/io/io_grida.rs (6)
default(19-21)default(54-56)default(286-296)default(781-783)default(805-807)default(2150-2152)crates/grida-canvas/src/node/schema.rs (5)
default(63-70)default(373-375)default(487-497)default(636-638)new(244-263)
crates/grida-canvas/src/layout/into_taffy.rs (3)
crates/grida-canvas/src/painter/layer.rs (2)
transform(101-101)transform(122-128)packages/grida-canvas-cg/lib.ts (1)
AffineTransform(65-68)crates/grida-canvas/src/node/schema.rs (14)
default(63-70)default(373-375)default(487-497)default(636-638)layout(878-884)rect(747-747)rect(809-816)rect(993-1000)rect(1123-1130)rect(1257-1264)rect(1455-1472)rect(1539-1541)rect(1661-1668)rect(1792-1799)
crates/grida-canvas/src/io/io_figma.rs (3)
crates/grida-canvas/src/io/io_grida.rs (22)
from(25-29)from(33-35)from(60-66)from(128-133)from(326-333)from(337-346)from(350-352)from(356-364)from(368-377)from(381-500)from(504-509)from(513-518)from(787-793)from(811-816)from(839-854)from(980-995)default(19-21)default(54-56)default(286-296)default(781-783)default(805-807)default(2150-2152)crates/grida-canvas/src/node/schema.rs (5)
new(244-263)default(63-70)default(373-375)default(487-497)default(636-638)crates/grida-canvas/src/cg/types.rs (10)
new(20-22)new(817-822)new(1279-1285)new(1638-1645)default(43-45)default(124-126)default(146-148)default(278-280)default(342-344)default(370-372)
crates/grida-canvas/src/io/io_grida.rs (4)
crates/grida-canvas/src/cg/types.rs (31)
default(43-45)default(124-126)default(146-148)default(278-280)default(342-344)default(370-372)default(386-388)default(416-418)default(440-442)default(542-544)default(554-556)default(566-568)default(599-601)default(647-652)default(723-725)default(836-838)from(88-94)from(263-265)from(1032-1034)from(1689-1691)from(1695-1697)from(1725-1730)from(1735-1740)from(1868-1870)value(1288-1290)all(522-529)all(918-925)new(20-22)new(817-822)new(1279-1285)new(1638-1645)crates/grida-canvas/src/io/io_figma.rs (9)
from(27-34)from(38-45)from(49-168)from(172-178)from(182-189)from(193-199)from(203-209)from(213-218)new(385-394)crates/grida-canvas/src/io/io_css.rs (1)
length(76-81)crates/grida-canvas/src/node/schema.rs (2)
new(244-263)layout(878-884)
crates/grida-canvas/src/node/schema.rs (3)
packages/grida-canvas-cg/lib.ts (3)
Axis(582-582)MainAxisAlignment(589-596)CrossAxisAlignment(603-603)crates/grida-canvas/src/cg/types.rs (19)
new(20-22)new(817-822)new(1279-1285)new(1638-1645)default(43-45)default(124-126)default(146-148)default(278-280)default(342-344)default(370-372)default(386-388)default(416-418)default(440-442)default(542-544)zero(16-18)zero(505-512)zero(801-806)zero(873-875)zero(914-916)crates/grida-canvas/src/shape/polygon.rs (1)
polygon_bounds(64-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: cargo test
🔇 Additional comments (7)
crates/grida-canvas/examples/grida_vector.rs (2)
15-16: Switch to layout_dimensions for root sizing — LGTMMatches the new layout model and keeps example consistent.
47-47: Explicitlayout_child: Noneon vectors — LGTMConsistent initialization across nodes aids predictable layout participation.
Also applies to: 80-80, 114-114, 144-144, 177-177, 219-219, 250-250, 291-291, 327-327
crates/grida-canvas/tests/vector_corner_radius.rs (1)
51-51: Struct init updated — LGTM
layout_child: Nonekeeps tests aligned with schema.crates/grida-canvas/src/node/factory.rs (2)
68-68: Addedlayout_child: Nonedefaults — LGTMKeeps node inits consistent with layout participation model.
Also applies to: 91-91, 112-112, 126-126, 187-187, 223-223, 245-245, 266-266, 285-285, 306-306
170-187: Container defaults via composite layout surfaces — LGTMClean separation of container/child/dimensions; matches new
UniformNodeLayout.crates/grida-canvas/src/layout/into_taffy.rs (1)
474-502: Absolute-position fallback for Vector/SVGPath — LGTMSane default when
layout_childis None; matches engine’s root-position correction tests.If you foresee Vector/SVGPath participating in flow by default later, consider guarding the absolute fallback behind an explicit flag to avoid behavior change.
Also applies to: 504-533
crates/grida-canvas/src/io/io_grida.rs (1)
1126-1146: CSSPosition → LayoutPositioning mapping is correctly implemented.The
impl From<CSSPosition> for LayoutPositioningexists in io_css.rs, with the mapping aligning as expected:CSSPosition::Absolutemaps toLayoutPositioning::Absolute, whileCSSPosition::Relativemaps toLayoutPositioning::Auto. No issues found.
| /// Check if a node should participate in Taffy layout | ||
| /// | ||
| /// Nodes that should be in Taffy tree: | ||
| /// - Layout containers (Container, ICB) - need to lay out children | ||
| /// - Nodes with layout_child field - can participate as flex children | ||
| /// | ||
| /// Nodes skipped from Taffy (use manual schema layout): | ||
| /// - Group, BooleanOperation - no layout_child support (size derived from children) | ||
| fn should_participate_in_taffy(node: &Node) -> bool { | ||
| matches!( | ||
| node, | ||
| Node::Container(_) | ||
| | Node::InitialContainer(_) | ||
| | Node::Rectangle(_) | ||
| | Node::Ellipse(_) | ||
| | Node::Image(_) | ||
| | Node::Line(_) | ||
| | Node::Polygon(_) | ||
| | Node::RegularPolygon(_) | ||
| | Node::RegularStarPolygon(_) | ||
| | Node::TextSpan(_) | ||
| | Node::Error(_) | ||
| | Node::Vector(_) | ||
| | Node::SVGPath(_) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Comment/code mismatch on Taffy participation
Comments say Vector/SVGPath are skipped, but should_participate_in_taffy() includes them and subtree builder does not skip. Align comments or logic.
- // Nodes that don't participate in Taffy layout (Vector, SVGPath, Group, etc.)
+ // Nodes that don't participate in Taffy layout (Group, BooleanOperation)
// are skipped and get manual layout results created in extract_all_layouts()Also applies to: 231-235
🤖 Prompt for AI Agents
In crates/grida-canvas/src/layout/engine.rs around lines 189–214 (and similarly
adjust lines 231–235), the comment says Vector and SVGPath are skipped from
Taffy but the should_participate_in_taffy() function currently includes them
(and the subtree builder doesn't skip them), causing mismatched behavior and
documentation. Fix by deciding the intended behavior: either remove Vector and
SVGPath from the matches list (and ensure subtree builder also skips them) if
they should not participate, or update the comment to list them as participating
and adjust the subtree builder to include them consistently; then make the code
and comment consistent across both locations (189–214 and 231–235).
| /// Convert schema LayoutGap to Taffy gap based on flex direction | ||
| /// | ||
| /// **IMPORTANT**: Taffy's gap is absolute (not direction-relative): | ||
| /// - `gap.width` = column-gap (horizontal spacing) | ||
| /// - `gap.height` = row-gap (vertical spacing) | ||
| /// | ||
| /// Our LayoutGap is direction-relative (main/cross), so we need the flex direction | ||
| /// to map correctly. This function should NOT be used directly - use `layout_gap_to_taffy()` | ||
| /// with the direction parameter instead. | ||
| fn layout_gap_to_taffy(gap: LayoutGap, direction: Axis) -> Size<LengthPercentage> { |
There was a problem hiding this comment.
Doc wording nit: self-referential note about gap mapper
“This function should NOT be used directly - use layout_gap_to_taffy()” references itself. Reword to clarify it requires the direction parameter and shouldn’t use raw Taffy gap.
🤖 Prompt for AI Agents
crates/grida-canvas/src/layout/into_taffy.rs around lines 104 to 113: The
docstring mistakenly says "This function should NOT be used directly - use
`layout_gap_to_taffy()`" which is self-referential and confusing; update the
wording to state that this helper requires a direction parameter and should not
be used directly by callers (they should call the public wrapper that provides
direction), clarifying that this function maps direction-relative LayoutGap to
Taffy's absolute gap and that callers should use the higher-level API which
supplies the Axis/direction.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/grida-canvas/src/io/io_grida.rs (1)
1197-1204: Right/bottom insets are ignored for Container.position; build Inset basis when present.When JSON provides right/bottom (Insets model), position is still forced to Cartesian(left, top). This drops geometry and breaks absolute placement with right/bottom-only specs.
Apply:
- position: LayoutPositioningBasis::Cartesian(CGPoint::new( - node.base.left.unwrap_or(0.0), - node.base.top.unwrap_or(0.0), - )), + position: { + if node.base.right.is_some() || node.base.bottom.is_some() { + // Inset basis preserves all four edges (missing edges default to 0.0) + LayoutPositioningBasis::Inset(EdgeInsets { + top: node.base.top.unwrap_or(0.0), + right: node.base.right.unwrap_or(0.0), + bottom: node.base.bottom.unwrap_or(0.0), + left: node.base.left.unwrap_or(0.0), + }) + } else { + LayoutPositioningBasis::Cartesian(CGPoint::new( + node.base.left.unwrap_or(0.0), + node.base.top.unwrap_or(0.0), + )) + } + },Add a unit test with absolute positioning using only right/bottom to lock this. Based on PR objectives (Inset model primary).
🧹 Nitpick comments (7)
crates/grida-canvas/src/cg/types.rs (3)
565-575: LGTM - Well-designed positioning enum.The
AutovsAbsolutedistinction is clear, and the default ofAutois appropriate for flex layouts.Consider adding
Eqderive for consistency with similar enums likeMainAxisAlignment(line 404) andCrossAxisAlignment(line 434), which both deriveEqin addition toPartialEq:-#[derive(Debug, Clone, Copy, PartialEq, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] pub enum LayoutPositioning {
577-608: LGTM - Excellent documentation.The documentation clearly explains how each anchor variant behaves in both horizontal and vertical contexts, making this type very approachable.
Consider adding
Eqderive for consistency with similar enums in this file:-#[derive(Debug, Clone, Copy, PartialEq, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] pub enum LayoutConstraintAnchor {
610-659: LGTM - Exemplary documentation with practical examples.The three code examples (top-left, centered, stretched) make it immediately clear how to use this type. Excellent developer experience.
Consider adding
Eqderive for consistency:-#[derive(Debug, Clone, Copy, PartialEq, Deserialize)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize)] pub struct LayoutConstraints {crates/grida-canvas/src/io/io_grida.rs (3)
646-660: Corner radius uniformity uses f32::EPSILON; use a looser tolerance.JSON floats rarely match exactly; EPSILON is too strict and causes false negatives.
- if (top_left_radius - top_right_radius).abs() < f32::EPSILON - && (top_right_radius - bottom_right_radius).abs() < f32::EPSILON - && (bottom_right_radius - bottom_left_radius).abs() < f32::EPSILON + const RADIUS_EPS: f32 = 1e-3; + if (top_left_radius - top_right_radius).abs() < RADIUS_EPS + && (top_right_radius - bottom_right_radius).abs() < RADIUS_EPS + && (bottom_right_radius - bottom_left_radius).abs() < RADIUS_EPS
1251-1258: DRY up repeated LayoutChildStyle construction.The same layout_child block is repeated across nodes. Extract a small helper to reduce duplication and future drift.
Example:
+fn child_style_from(pos: Option<CSSPosition>) -> LayoutChildStyle { + LayoutChildStyle { + layout_positioning: pos.map(Into::into).unwrap_or_default(), + layout_grow: 0.0, + } +}Then:
- layout_child: Some(LayoutChildStyle { - layout_positioning: node.base.position.map(|position| position.into()).unwrap_or_default(), - layout_grow: 0.0, - }), + layout_child: Some(child_style_from(node.base.position)),Also applies to: 1286-1293, 1391-1398, 1439-1446, 1537-1544, 1586-1593, 1636-1643, 1683-1690, 1725-1732, 1777-1784
76-101: Prefer a typed enum for image fit over free-form String.String matching invites typos and silent fallbacks. Introduce JSONImageFit enum (transform, tile, contain, cover, fill, none) and switch serde to that.
If keeping strings for backward-compat, accept both via an untagged enum.
crates/grida-canvas/examples/tool_io_grida.rs (1)
80-84: Sort node type output for deterministic logs.Improves readability in CI and diffs.
- if !node_types.is_empty() { - println!(" Node types:"); - for (name, count) in node_types.iter() { - println!(" {}: {}", name, count); - } - } + if !node_types.is_empty() { + println!(" Node types:"); + let mut items: Vec<_> = node_types.into_iter().collect(); + items.sort_by(|a, b| a.0.cmp(&b.0)); + for (name, count) in items { + println!(" {}: {}", name, count); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
crates/grida-canvas/AGENTS.md(1 hunks)crates/grida-canvas/examples/tool_io_grida.rs(1 hunks)crates/grida-canvas/src/cg/types.rs(4 hunks)crates/grida-canvas/src/io/io_grida.rs(28 hunks)editor/public/examples/canvas/blank.grida(1 hunks)editor/public/examples/canvas/component-01.grida(1 hunks)editor/public/examples/canvas/event-page-01.grida(1 hunks)editor/public/examples/canvas/globals-01.grida(1 hunks)editor/public/examples/canvas/helloworld.grida(1 hunks)editor/public/examples/canvas/instagram-post-01.grida(2 hunks)editor/public/examples/canvas/layout-01.grida(1 hunks)editor/public/examples/canvas/poster-01.grida(2 hunks)editor/public/examples/canvas/resume-01.grida(1 hunks)editor/public/examples/canvas/sketch-teimplate-01.grida(1 hunks)editor/public/examples/canvas/slides-01.grida(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- editor/public/examples/canvas/event-page-01.grida
- editor/public/examples/canvas/sketch-teimplate-01.grida
- editor/public/examples/canvas/layout-01.grida
- editor/public/examples/canvas/component-01.grida
- editor/public/examples/canvas/resume-01.grida
🧰 Additional context used
📓 Path-based instructions (2)
crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust crates under /crates; this is the monorepo’s Rust workspace for the Canvas implementation
Files:
crates/grida-canvas/AGENTS.mdcrates/grida-canvas/examples/tool_io_grida.rscrates/grida-canvas/src/cg/types.rscrates/grida-canvas/src/io/io_grida.rs
crates/grida-canvas/**/*.rs
📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)
crates/grida-canvas/**/*.rs: All internal structs (e.g., NodeRecs, SceneGraph, caches) must use NodeId (u64) for identifiers
Public Rust APIs must accept and return UserNodeId (String) instead of NodeId for stability
Do not serialize internal NodeId values; only serialize stable UserNodeId in .grida files
Keep NodeId as u64 and UserNodeId as String consistently across the codebase
Files:
crates/grida-canvas/examples/tool_io_grida.rscrates/grida-canvas/src/cg/types.rscrates/grida-canvas/src/io/io_grida.rs
🧠 Learnings (1)
📚 Learning: 2025-09-15T10:31:32.864Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-09-15T10:31:32.864Z
Learning: Applies to crates/grida-canvas-fonts/tests/serde_test.rs : Put JSON serialization tests in tests/serde_test.rs
Applied to files:
crates/grida-canvas/src/io/io_grida.rs
🧬 Code graph analysis (3)
crates/grida-canvas/examples/tool_io_grida.rs (1)
crates/grida-canvas/src/io/io_grida.rs (1)
parse(1159-1161)
crates/grida-canvas/src/cg/types.rs (3)
crates/grida-canvas/src/node/schema.rs (5)
zero(608-610)default(63-70)default(373-375)default(487-497)default(636-638)crates/grida-canvas/src/io/io_grida.rs (6)
default(19-21)default(54-56)default(286-296)default(818-820)default(842-844)default(2187-2189)packages/grida-canvas-cg/lib.ts (1)
FillRule(177-177)
crates/grida-canvas/src/io/io_grida.rs (3)
crates/grida-canvas/src/cg/types.rs (23)
default(43-45)default(124-126)default(146-148)default(278-280)default(342-344)default(356-358)default(376-378)default(392-394)default(422-424)default(446-448)default(548-550)default(560-562)default(572-574)default(605-607)default(653-658)default(729-731)value(1294-1296)all(528-535)all(924-931)new(20-22)new(823-828)new(1285-1291)new(1644-1651)crates/grida-canvas/src/io/io_css.rs (1)
length(76-81)crates/grida-canvas/src/node/schema.rs (2)
new(244-263)layout(878-884)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: cargo test
🔇 Additional comments (18)
crates/grida-canvas/AGENTS.md (1)
51-78: Both file references are valid and exist at the expected locations. The documentation is accurate and requires no changes.editor/public/examples/canvas/blank.grida (1)
2-2: LGTM! Version metadata added consistently.The version field addition provides proper versioning for the example file format, aligning with the PR's layout model implementation.
editor/public/examples/canvas/globals-01.grida (1)
2-2: LGTM! Version metadata added consistently.The version field addition maintains consistency across example files and supports proper format versioning.
editor/public/examples/canvas/slides-01.grida (2)
2-2: LGTM! Version metadata added consistently.The version field addition aligns with the layout model implementation and provides proper format versioning.
612-612: Good fix! Corrected SVG property name.The typo "fillRile" has been corrected to "fillRule", which is the standard SVG property name. This ensures proper SVG path rendering.
editor/public/examples/canvas/helloworld.grida (1)
2-2: LGTM! Version metadata added consistently.The version field addition maintains consistency with other example files in this PR.
editor/public/examples/canvas/poster-01.grida (2)
2-2: LGTM! Version metadata added consistently.The version field addition aligns with the layout model implementation and ensures proper format versioning.
757-842: Excellent fix! Corrected SVG property name throughout.The typo "fillRile" has been systematically corrected to "fillRule" across all SVG path objects in this file. This is the standard SVG property name and ensures proper path rendering for the "Love is the best thing we do" text graphic.
editor/public/examples/canvas/instagram-post-01.grida (2)
2-2: LGTM! Version metadata added consistently.The version field addition maintains consistency across all example files and supports proper format versioning.
311-311: Good fix! Corrected SVG property name.The typo "fillRile" has been corrected to "fillRule" in the SVG path for the face icon. This is the standard SVG property name and ensures proper icon rendering.
crates/grida-canvas/src/cg/types.rs (3)
16-18: LGTM - Clean zero constructor.The
zero()constructor follows Rust conventions and provides a clear, self-documenting way to create origin points.
42-46: LGTM - Proper Default implementation.Delegating to
zero()is the right approach, maintaining a single source of truth for the default value.
355-359: LGTM - Correct default per web standards.
NonZerois the standard default fill rule in SVG and HTML Canvas, ensuring web-compatible behavior.crates/grida-canvas/src/io/io_grida.rs (4)
187-205: Image repeat/scale defaults look good; minor nits.
- Quarter turns reduced mod 4 is correct.
- Consider validating scale > 0 to avoid degenerate tiles.
Also applies to: 469-493, 1469-1506
875-892: Padding JSON and tests look solid.Uniform and non-uniform deserialization and EdgeInsets conversion behave as expected; tests cover both. Nice.
Also applies to: 1987-2113
3623-3660: Layout props serde and conversions are well covered by tests.Good coverage for layout mode, direction, padding, gaps, wrap, and alignments, including round-trip into ContainerNodeRec.
Consider adding a test for default values (omitting layout/direction/gaps) to assert intended defaults.
Also applies to: 3662-3707, 3709-3744, 3746-3803, 3804-3840, 3841-3900, 3902-3984
234-237: No action required—FillRule implements Default correctly.Verification confirms that
FillRulehas an explicitDefaultimplementation atcrates/grida-canvas/src/cg/types.rs:355. The#[serde(default)]attribute on thefill_rulefield will work as intended without any changes.crates/grida-canvas/examples/tool_io_grida.rs (1)
41-45: CLI and error handling are fine.Usage message, read/parse branching, and exit codes are correct for a simple validator.
Also applies to: 50-56, 86-95
| stroke_width: 0.0, | ||
| stroke_align: node.base.stroke_align.unwrap_or(StrokeAlign::Inside), |
There was a problem hiding this comment.
SVGPath uses hardcoded stroke_width = 0.0; should use node.base.stroke_width.
Current code discards the authored stroke width for SVG paths.
- stroke_width: 0.0,
+ stroke_width: node.base.stroke_width,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/grida-canvas/src/io/io_grida.rs around lines 1680 to 1681, the SVGPath
is being created with a hardcoded stroke_width: 0.0 which discards the authored
value; replace the literal with the node's value (e.g.
node.base.stroke_width.unwrap_or(0.0) or appropriate conversion) so the SVG path
uses the authored stroke width while preserving the existing default behavior if
the field is None.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
crates/grida-canvas/src/io/io_grida.rs (3)
521-542: Inset vs Cartesian positioning helper: good addition; please add right/bottom-only tests.Helper correctly switches to Inset when right/bottom exist. Add unit tests for:
- Only right/bottom set (left/top absent).
- Mixed cases (e.g., right present, bottom absent).
You previously requested this behavior be covered by tests. I can draft a test similar to existing padding_tests if useful.
1223-1228: Container.position now honors right/bottom via Inset: resolves prior feedback.Wiring to json_position_to_layout_basis fixes the dropped right/bottom edges.
1705-1705: SVGPath stroke width hardcoded to 0.0 — use authored stroke width.Current code ignores node.base.stroke_width. Use the node value to render correctly.
Apply:
- stroke_width: 0.0, + stroke_width: node.base.stroke_width,
🧹 Nitpick comments (2)
crates/grida-canvas/src/io/io_grida.rs (2)
234-237: JSONSVGPath fill_rule/fill are parsed but unused in Node conversion.If SVGPathNodeRec supports fill rules or per-path fills, wire them; otherwise remove from JSON or leave a TODO to avoid confusion.
Would you like me to check SVGPathNodeRec and propose the exact wiring?
926-941: Typed layout fields on container: LGTM; consider clamping negative gaps.Currently negative gaps would produce Some(LayoutGap{...}). Clamp to >= 0 to avoid odd layout.
Proposed change:
- layout_gap: if node.main_axis_gap > 0.0 || node.cross_axis_gap > 0.0 { + layout_gap: if node.main_axis_gap != 0.0 || node.cross_axis_gap != 0.0 { Some(LayoutGap { - main_axis_gap: node.main_axis_gap, - cross_axis_gap: node.cross_axis_gap, + main_axis_gap: node.main_axis_gap.max(0.0), + cross_axis_gap: node.cross_axis_gap.max(0.0), }) } else { None },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/grida-canvas-wasm/lib/bin/grida_canvas_wasm.wasmis excluded by!**/*.wasm
📒 Files selected for processing (2)
crates/grida-canvas-wasm/package.json(1 hunks)crates/grida-canvas/src/io/io_grida.rs(29 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
crates/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep Rust crates under /crates; this is the monorepo’s Rust workspace for the Canvas implementation
Files:
crates/grida-canvas-wasm/package.jsoncrates/grida-canvas/src/io/io_grida.rs
crates/grida-canvas/**/*.rs
📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)
crates/grida-canvas/**/*.rs: All internal structs (e.g., NodeRecs, SceneGraph, caches) must use NodeId (u64) for identifiers
Public Rust APIs must accept and return UserNodeId (String) instead of NodeId for stability
Do not serialize internal NodeId values; only serialize stable UserNodeId in .grida files
Keep NodeId as u64 and UserNodeId as String consistently across the codebase
Files:
crates/grida-canvas/src/io/io_grida.rs
🧠 Learnings (3)
📚 Learning: 2025-09-26T09:29:53.155Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-09-26T09:29:53.155Z
Learning: Applies to crates/grida-canvas-wasm/{crates/grida-canvas-wasm/src/main.rs,**/grida-canvas-wasm.d.ts} : When introducing new public APIs in the WASM entrypoint (main.rs), update the TypeScript declarations in grida-canvas-wasm.d.ts to keep bindings in sync
Applied to files:
crates/grida-canvas-wasm/package.json
📚 Learning: 2025-09-26T09:29:53.155Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-09-26T09:29:53.155Z
Learning: Applies to crates/grida-canvas-wasm/lib/bin/{grida-canvas-wasm.js,grida-canvas-wasm.wasm,grida_canvas_wasm.wasm} : Keep the generated Emscripten artifacts (grida-canvas-wasm.js and *.wasm) checked into version control under lib/bin for faster CI builds
Applied to files:
crates/grida-canvas-wasm/package.json
📚 Learning: 2025-09-15T10:31:32.864Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-09-15T10:31:32.864Z
Learning: Applies to crates/grida-canvas-fonts/tests/serde_test.rs : Put JSON serialization tests in tests/serde_test.rs
Applied to files:
crates/grida-canvas/src/io/io_grida.rs
🧬 Code graph analysis (1)
crates/grida-canvas/src/io/io_grida.rs (3)
crates/grida-canvas/src/cg/types.rs (25)
new(20-22)new(823-828)new(1285-1291)new(1644-1651)circular(868-873)circular(933-935)default(43-45)default(124-126)default(146-148)default(278-280)default(342-344)default(356-358)default(376-378)default(392-394)default(422-424)default(446-448)default(548-550)default(560-562)default(572-574)default(605-607)default(653-658)default(729-731)value(1294-1296)all(528-535)all(924-931)crates/grida-canvas/src/node/schema.rs (2)
new(244-263)layout(878-884)crates/grida-canvas/src/io/io_css.rs (1)
length(76-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cargo test
- GitHub Check: test
🔇 Additional comments (6)
crates/grida-canvas-wasm/package.json (1)
4-4: No issues found. Original review comment is based on incorrect assumptions.The verification reveals:
- Cargo.toml versioning: The "0.0.0" is a standard workspace marker pattern for Emscripten/WASM packages; package.json holds the actual published version (0.0.80-canary.3) ✓
- Public API exports: No new public APIs were added to main.rs, so the layout system Part 1 is implemented as internal infrastructure without public exposure
- TypeScript declarations: Correctly reflect the current public API surface (no layout references needed since none are exported) ✓
- WASM artifacts: Already regenerated ✓
The original review's concerns about version consistency and TypeScript declaration sync don't apply here, as no new public APIs were introduced to synchronize.
crates/grida-canvas/src/io/io_grida.rs (5)
591-607: Per-corner object radius: implementation looks correct; add coverage.Conversion to RectangularCornerRadius and uniform detection are sound. Add tests for:
- PerCornerObject into_rectangular.
- PerCornerObject into_uniform (equal vs non-equal).
Also applies to: 639-650, 668-684
708-713: CSSPosition typing: confirm serde mapping and downstream conversion.Ensure "absolute"/"relative"/"fixed"/"sticky"/"static" strings deserialize to CSSPosition and map to LayoutPositioning in layout_child as intended. Add a small test verifying absolute maps to LayoutPositioning::Absolute.
1189-1191: Position precedence: transform(left/top) vs layout_child.positioning.Multiple nodes compute transform from left/top while also setting layout_child.layout_positioning. Confirm the layout engine’s precedence to avoid double-applying offsets or conflicting positioning.
Also applies to: 1304-1306, 1384-1386, 1435-1437, 1485-1487, 1577-1579, 1626-1628, 1676-1681, 1723-1725, 1765-1767, 1818-1820
2012-2138: Padding tests: good coverage.Uniform and non-uniform conversion paths are verified; container wiring is exercised.
3648-4009: Container layout property tests: solid.Covers layout mode, direction, wrap, gaps, padding, and alignments; conversions check out.
1 participant
The Grida Layout Model (
layout) introduces a unified, geometry-first foundation for all 2D layout and positioning scenarios.It merges the conceptual clarity of anchors, the flexibility of flexbox, and the structure of grid—forming a single, coherent system that scales from freeform graphics design to complex UI composition.
Unlike traditional rule-based layouts, this model prioritizes direct manipulation, predictability, and composability, aligning deeply with designer-first workflows and visual editing metaphors.
The result is a layout system that remains ergonomic for creators, precise for engineers, and interoperable with web standards, bridging the gap between WYSIWYG design and production-grade layout semantics.
Proposal: WG/LAYOUT
Update (decision):
Anchor model: NOT supported (planned/suggested earlier but removed from this PR and the current roadmap).
Inset model: WILL be supported and implemented as the primary geometry-based content inset API.
Core
NodeIdasu64Grida Canvas - Layout Prep - NodeId u64 #433Box
paddinginsetConstraints
layout_constraintsAnchors
Flex - on container
layout_modelayout_direction(flex-direction)layout_wrap(flex-wrap)layout_gap(gap)layout_main_axis_alignment(align-items)layout_cross_axis_alignment(justify-items)Flex - on content
layout_grow(flex_grow)layout_positioning(position)flex_shrinkEditor UX
Impls
Pt.1 flex align, wrap
day-297-grida-canvas-layout-pt1-flex-align-wrap.mp4
Pt.2 Padding
day-298-grida-canvas-layout-pt2-padding.mp4
Summary by CodeRabbit
New Features
Changes
Documentation