Grida Canvas - .grida & .grida1#487
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRenames text nodes to "tspan", migrates absolute positioning to a new layout_* schema, adds a FlatBuffers format and Changes
Sequence Diagram(s)sequenceDiagram
participant UI as CanvasPlayground (UI)
participant Consumer as Playground Consumer / Editor
participant OPFS as OPFS Handle
participant IO as io.GRID / io.archive
UI->>Consumer: load(src | provided document)
Consumer->>OPFS: usePlaygroundOPFS.get(resolvedFilekey)
alt OPFS has document.grida
OPFS-->>Consumer: document bytes
Consumer->>IO: io.GRID.decode(bytes)
IO-->>Consumer: GridaDocument
Consumer-->>UI: initialize editor state
else no OPFS document
Consumer-->>UI: initialize from provided document
end
UI->>Consumer: Cmd/Ctrl+S (save)
Consumer->>IO: io.GRID.encode(document)
IO-->>Consumer: bytes
Consumer->>OPFS: fileHandle.write(bytes)
OPFS-->>Consumer: OK
Consumer-->>UI: onSaved callback / toast
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
…ure deterministic installation across environments
…ngs for FlatBuffers schema
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45a5691f64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| top = hasNonZero && topVal === 0 ? undefined : topVal; | ||
| right = hasNonZero && rightVal === 0 ? undefined : rightVal; | ||
| bottom = hasNonZero && bottomVal === 0 ? undefined : bottomVal; | ||
| left = hasNonZero && leftVal === 0 ? undefined : leftVal; |
There was a problem hiding this comment.
This logic drops any inset value that is exactly 0 whenever another inset is non‑zero, so a node positioned with a valid right=0/bottom=0 (e.g., pinned to the right edge with a non‑zero top) is decoded with that side set to undefined. That changes the layout after a save/load roundtrip because the pin is lost, which is a regression for common “edge‑aligned” cases; if you need to distinguish “unset” from 0, you’ll need an explicit presence flag rather than discarding zeros here.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (33)
crates/grida-canvas/src/cache/paragraph.rs (1)
177-196: Cache key inconsistency with max_lines filtering.The
shape_keyfunction hashesmax_linesdirectly (Line 194), but the layout logic now treatsSome(0)identically toNone(lines 247, 401). This means:
max_lines = Some(0)andmax_lines = Noneproduce different cache keys- Both produce identical paragraph layouts
- Results in unnecessary cache entries and potential cache pollution
🔧 Proposed fix to normalize max_lines in cache key
fn shape_key( text: &str, style: &TextStyleRec, align: &TextAlign, max_lines: &Option<usize>, ) -> u64 { let mut h = DefaultHasher::new(); text.hash(&mut h); style.font_family.hash(&mut h); style.font_size.to_bits().hash(&mut h); style.font_weight.0.hash(&mut h); style.font_style_italic.hash(&mut h); // TODO: Add letter_spacing and line_height to hash // style.letter_spacing.0.to_bits().hash(&mut h); // style.line_height.map(|v| v.to_bits()).hash(&mut h); style.text_transform.hash(&mut h); (*align as u8).hash(&mut h); - max_lines.hash(&mut h); + // Normalize max_lines: treat 0 as None since both are handled identically in layout + max_lines.filter(|&m| m > 0).hash(&mut h); h.finish() }editor/grida-canvas/utils/__tests__/insertion.test.ts (1)
25-51: Type assertions may hide missing required schema properties.The test data uses type assertions (
as grida.program.document.Sceneandas grida.program.nodes.RectangleNode) which bypass TypeScript's structural validation. Based on the Scene interface shown in the relevant code snippets, theconstraintsproperty appears to be required but is missing from the test data at line 26-31. Similarly, RectangleNode extends multiple traits that may require additional properties beyond the minimal set provided.Consider either:
- Removing the type assertions and adding all required properties so TypeScript validates the objects structurally, or
- Using a test helper factory function that creates properly typed objects with sensible defaults
This ensures tests validate schema compliance rather than bypassing type safety.
💡 Example: Remove assertions and add missing properties
const sub: grida.program.document.IPackedSceneDocument = { scene: { type: "scene", id: "s", name: "s", children_refs: ["a", "b"], + constraints: { + children: "multiple", + }, - } as grida.program.document.Scene, + }, nodes: { a: { id: "a", + name: "a", type: "rectangle", layout_inset_left: 10, layout_inset_top: 10, layout_target_width: 20, layout_target_height: 20, layout_positioning: "absolute", - } as grida.program.nodes.RectangleNode, + }, b: { id: "b", + name: "b", type: "rectangle", layout_inset_left: 40, layout_inset_top: 40, layout_target_width: 20, layout_target_height: 20, layout_positioning: "absolute", - } as grida.program.nodes.RectangleNode, + }, },Note: Additional properties may be required based on the full interface definitions.
editor/grida-canvas-react/use-sub-vector-network-editor.ts (1)
94-98: Replace non-null assertions with null coalescing operators for safer fallback handling.The properties
layout_inset_leftandlayout_inset_topare optional in the schema (defined as?: number), and tests confirm they can be undefined. Using non-null assertions on line 98 is inconsistent with defensive handling patterns used elsewhere in the codebase (e.g.,insertion.ts,positioning.tsx). If these properties are undefined when the fallback executes, the offset will propagate undefined values to vector calculations, causing NaN or incorrect rendering.Replace with null coalescing to safely default to
0:Proposed fix
const offset: cmath.Vector2 = absolute ? [absolute.x, absolute.y] - : [node.layout_inset_left!, node.layout_inset_top!]; + : [node.layout_inset_left ?? 0, node.layout_inset_top ?? 0];editor/grida-canvas/reducers/methods/flatten.ts (1)
15-25: Tspan added to FLATTENABLE_NODE_TYPES but lacks conversion implementation.Adding
"tspan"to FLATTENABLE_NODE_TYPES at line 22 creates a broken contract:supportsFlatten()will return true for tspan nodes, butself_flattenNode()will fail becausetoVectorNetworkFallback()has no case for "tspan" and will return null.The wasm backend fallback at line 63 may also fail, leaving no viable path to convert tspan to vector. Either remove "tspan" from FLATTENABLE_NODE_TYPES until the wasm backend and fallback implementation are complete, or add a tspan case to
toVectorNetworkFallback().editor/grida-canvas/reducers/event-target.cem-vector.reducer.ts (2)
92-180: Clearsnapped_segment_pwhen vertex snapping is active (or snapping is disabled) to avoid stale segment-split on click.
on_path_pointer_down()checkssnapped_segment_pfirst; if it remains set from a prior move while the cursor is now vertex-snapped (or snapping is disabled), a subsequent click can incorrectly split a segment.Proposed fix
if (should_snap) { const t = threshold(5, draft.transform); const res = snapToCanvasGeometry( [logical_pos], { points: anchor_points }, { x: t, y: t } ); @@ if (res.by_points) { const idx = res.by_points.hit_points.anchors.findIndex( ([xhit, yhit]) => xhit && yhit ); draft.content_edit_mode.snapped_vertex_idx = idx !== -1 ? idx : null; } else { draft.content_edit_mode.snapped_vertex_idx = null; } - // Compute segment snapping if no vertex is snapped and there's a hovered segment - if (draft.content_edit_mode.snapped_vertex_idx === null) { + // Segment snapping must never survive while vertex snapping is active + if (draft.content_edit_mode.snapped_vertex_idx !== null) { + draft.content_edit_mode.snapped_segment_p = null; + } else { __self_compute_vector_segment_snapping(draft, logical_pos, rect, node); // Update path cursor position to use snapped segment point if available if (draft.content_edit_mode.snapped_segment_p) { const snapped_point = draft.content_edit_mode.snapped_segment_p.point; // Convert local point to absolute coordinates draft.content_edit_mode.cursor = cmath.vector2.add(snapped_point, [ rect.x, rect.y, ]); } } } else { draft.content_edit_mode.cursor = logical_pos; draft.surface_snapping = undefined; draft.content_edit_mode.snapped_vertex_idx = null; + draft.content_edit_mode.snapped_segment_p = null; } }
206-218: Avoidlayout_inset_left!/top!non-null assertions in layout recompute hot paths.If any VectorNode can exist in-memory without
layout_inset_*(migration gaps, partially-constructed nodes, clipboard/import), these!s will turn intoNaNpositioning and corrupt subsequent edits. Prefer an assert + fallback (or normalize once at entry).One pattern to apply (repeat at each site)
- const new_pos = cmath.vector2.add( - [node.layout_inset_left!, node.layout_inset_top!], - delta - ); + const base_left = node.layout_inset_left ?? 0; + const base_top = node.layout_inset_top ?? 0; + const new_pos = cmath.vector2.add([base_left, base_top], delta);Also applies to: 248-260, 323-338, 561-577
editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts (2)
334-362: Bug risk:doc.scenes_ref[0]can throw when onlyentry_scene_idexists
pickTextAndVectorTargetsFromFixture()doesconst scene_id = doc.entry_scene_id ?? doc.scenes_ref[0];(Line 341-342). Elsewhere you use optional chaining (doc.scenes_ref?.[0]). Ifscenes_refis absent, this will throw even whenentry_scene_idis set (because RHS is evaluated only if LHS is nullish—but ifentry_scene_idis set, RHS won’t evaluate; still, if it’s absent andentry_scene_idis also absent, you want a clean error not a TypeError). Align it to optional chaining for consistency and safer failure mode.Proposed fix
- const scene_id = doc.entry_scene_id ?? doc.scenes_ref[0]; + const scene_id = doc.entry_scene_id ?? doc.scenes_ref?.[0]; if (!scene_id) throw new Error("fixture document has no entry scene id");
558-658: Origin semantics test: good coverage, but update legacy wording in the skipped TODO block
This test cleanly asserts thespace: "auto"vsspace: "global"behavior with the newlayout_*fields. Minor: the later skipped TODO block still references overridingleft/top(Lines 664-666) and should be updated tolayout_inset_left/topto avoid confusion during future work.editor/grida-canvas-react/viewport/surface-hooks.ts (1)
362-393: Fix potentialArray.from({ length: -1 })crash for empty flex containers.
If a flex container has 0 children,children_rects.length - 1becomes-1and throws.Proposed change
- const axis = direction === "horizontal" ? "x" : "y"; + const axis = (direction ?? "horizontal") === "horizontal" ? "x" : "y"; const children = dq.getChildren(document_ctx, node_id); const children_rects = children .map((id) => instance.geometryProvider.getNodeAbsoluteBoundingRect(id)) .filter((it): it is cmath.Rectangle => !!it); distribution.rects = children_rects; distribution[axis] = { gap: layout_main_axis_gap, tolerance: 0, gaps: Array.from( - { length: children_rects.length - 1 }, + { length: Math.max(0, children_rects.length - 1) }, () => layout_main_axis_gap ), };editor/scaffolds/sidecontrol/controls/positioning.tsx (1)
93-98: Potential property name mismatch in constraint callback.The
onConstraintChangecallback receivessideas"left" | "right" | "top" | "bottom", but line 96 uses[side]: checked ? 0 : undefinedto set properties. This will set properties likeleft,right, etc., rather thanlayout_inset_left,layout_inset_right, etc., which appears inconsistent with the migration to layout-prefixed properties throughout the rest of this file.🔧 Proposed fix to use layout_inset_* properties
onConstraintChange={(side, checked) => { onValueCommit?.({ ...value, - [side]: checked ? 0 : undefined, + [`layout_inset_${side}`]: checked ? 0 : undefined, }); }}editor/scaffolds/sidecontrol/controls/padding.tsx (1)
10-41: Make thevalue[prop]access type-safe acrossPadding | MixedPadding.
As written, correctness depends ongrida.program.nodes.i.IPaddinghavinglayout_padding_*keys (and being indexable). Consider narrowing or normalizingvaluebefore indexing.Proposed fix (normalize + avoid unsafe indexed access)
@@ type Padding = grida.program.nodes.i.IPadding; @@ export function PaddingControl({ @@ }) { @@ - const getPaddingValue = ( + const normalized = value as (Padding & MixedPadding) | undefined; + + const getPaddingValue = ( prop: | "layout_padding_top" | "layout_padding_right" | "layout_padding_bottom" | "layout_padding_left", defaultValue: number = 0 ): TMixed<number> => { - if (!value) return defaultValue; - const val = value[prop]; + if (!normalized) return defaultValue; + const val = normalized[prop]; if (val === grida.mixed) return grida.mixed; if (val === undefined) return defaultValue; return val; };editor/scaffolds/sidecontrol/controls/gap.tsx (1)
56-60: Fix gap preset detection for value0.
0is a valid preset (“gap-0”), but the current truthy checks treat it as “no value”.Proposed fix
@@ const hasPreset = useMemo(() => { - if (!numericValue || typeof numericValue !== "number") return false; + if (numericValue === undefined || typeof numericValue !== "number") return false; return Object.values(twgap).some((preset) => preset.gap === numericValue); }, [numericValue]); @@ <Select disabled={disabled} - value={hasPreset && numericValue ? String(numericValue) : undefined} + value={hasPreset && numericValue !== undefined ? String(numericValue) : undefined} onValueChange={ disabled ? undefined : (v) => onValueCommit?.(parseInt(v, 10)) } >Also applies to: 80-85
editor/grida-canvas/reducers/event-target.reducer.ts (2)
459-479: Remove the misleadinglayout_target_height: initial_rect.height as 0cast (runtime stays 1).
If the intent was “line nodes have height 0”, this does not enforce it; if the intent wasn’t line nodes, it’s confusing and risks downstream assumptions.Proposed fix
@@ const nnode = initialNode( draft.tool.node, () => context.idgen.next(), { layout_inset_left: initial_rect.x, layout_inset_top: initial_rect.y, layout_target_width: initial_rect.width, - layout_target_height: initial_rect.height as 0, // casting for line node + layout_target_height: initial_rect.height, }, context.paint_constraints );
1090-1103: After moving a child into the new container, set insets even if they were undefined.
The current guards can skip initializinglayout_inset_left/top, leaving the child in the wrong place after reparenting.Proposed fix
@@ const child = dq.__getNodeById( draft, id ) as grida.program.nodes.i.IPositioning; - if (typeof child.layout_inset_left === "number") - child.layout_inset_left = rect.x - container_rect.x; - if (typeof child.layout_inset_top === "number") - child.layout_inset_top = rect.y - container_rect.y; + child.layout_inset_left = rect.x - container_rect.x; + child.layout_inset_top = rect.y - container_rect.y;editor/grida-canvas-hosted/playground/widgets/index.ts (1)
227-238:styleproperty is deprecated onHTMLIFrameNode—use will work but migrate whencssbecomes available.The
iframenode type still acceptsstyle, so no immediate serialization drift. However, the property is marked@deprecatedwith a TODO to rename it tocss. Update this sample prototype once the new property is available to stay ahead of the deprecation timeline.editor/grida-canvas/reducers/methods/wrap.ts (1)
111-152: Guard logic allows relative-positioned children without explicit insets to skip compensation, potentially changing visual placement after wrap/ungroup.The insets adjustment only applies when
layout_inset_leftandlayout_inset_topare both present and numeric. Since these properties are optional in theIPositioninginterface, children usinglayout_positioning: "relative"without explicit inset values (or with undefined insets) will bypass adjustment entirely. This can cause unintended visual shifts when wrapping or ungrouping nodes that rely on relative positioning defaults.Verify that all child node types wrappable through
self_wrapNodes,self_ungroup, andself_wrapNodesAsBooleanOperationeither:
- Always initialize insets explicitly before being wrapped, or
- Enforce absolute positioning for wrappable children, or
- Extend the guard to compensate for missing insets (e.g., treat undefined/missing as 0)
This applies to lines 111–152, 222–233, and 314–355.
editor/grida-canvas-react-renderer-dom/nodes/image.tsx (1)
6-34: Defaultaltto""to avoid undefined leaking to<img>.This keeps behavior stable while preventing accidental
alt={undefined}in the DOM.Proposed patch
export const ImageWidget = ({ src, - alt, + alt = "", layout_target_width: width, layout_target_height: height, style, ...props }: grida.program.document.IComputedNodeReactRenderProps<grida.program.nodes.ImageNode>) => {editor/grida-canvas-react-renderer-dom/nodes/bitmap.tsx (1)
7-33: Guard against invalid/mismatched dimensions before constructingImageData.
BitmapRendererwill throw ifwidthNum/heightNumare non-finite/≤0 or ifdata.length !== widthNum * heightNum * 4. Also,context.bitmaps[imageRef]can be missing and currently crashes.Proposed hardening
export const BitmapWidget = ({ context, layout_target_width: width, layout_target_height: height, imageRef, style, ...props }: grida.program.document.IComputedNodeReactRenderProps<grida.program.nodes.BitmapNode>) => { const { objectFit, objectPosition, ...divStyles } = style || {}; - const imagedata = context.bitmaps[imageRef]; - // FIXME: this will fail with relative dimensions - const widthNum = css.toPxNumber(width); - const heightNum = css.toPxNumber(height); + const imagedata = context.bitmaps[imageRef]; + if (!imagedata) return null; + + // NOTE: BitmapRenderer/ImageData require concrete px sizes that match `data.length`. + const widthNum = css.toPxNumber(width); + const heightNum = css.toPxNumber(height); + const okSize = + Number.isFinite(widthNum) && + Number.isFinite(heightNum) && + widthNum > 0 && + heightNum > 0 && + imagedata.data.length === widthNum * heightNum * 4; + if (!okSize) { + // Consider: fall back to bitmap metadata (preferred) if available in `imagedata`. + return null; + } return ( <div style={{ ...divStyles, overflow: "hidden" }} {...queryattributes(props)} > <BitmapViewer width={widthNum} height={heightNum} data={imagedata.data} frame={imagedata.version} /> </div> ); };editor/grida-canvas-react-renderer-dom/nodes/line.tsx (2)
21-26: Fix Biome “noDangerouslySetInnerHtml” error (and ensure defs are trusted/sanitized).
This is currently a lint/security build-breaker, and potentially an injection sink if any part ofstrokeDefscan be user-controlled.Proposed minimal fix (Biome ignore + explicit trust boundary)
- {strokeDefs && <g dangerouslySetInnerHTML={{ __html: strokeDefs }} />} + {strokeDefs && ( + // biome-ignore lint/security/noDangerouslySetInnerHtml: SVG defs are generated internally (not user-supplied HTML). + <g dangerouslySetInnerHTML={{ __html: strokeDefs }} /> + )}Also applies to: 35-35
16-19: Potential stroke clipping:height={1}withoutoverflow: visible.
Ifstroke_width > 1, the line can get clipped by the SVG viewport.Proposed fix
const style_without_height: React.CSSProperties = { ...style, height: undefined, + overflow: "visible", };Also applies to: 28-45
editor/grida-canvas-react-renderer-dom/nodes/rectangle.tsx (1)
20-25: Corner radius normalization looks incorrect/inconsistent (andisZeromishandlesundefined).
Scaling top corners by width and bottom corners by height will produce different corner radii depending on corner, even when all inputs are the same. Also,undefinedradii currently force the “rounded path” branch.Proposed fix (consistent scaling + correct zero detection)
export function RectangleWidget({ style, layout_target_width: width, layout_target_height: height, fill, stroke, stroke_width, rectangular_corner_radius_top_left, rectangular_corner_radius_top_right, rectangular_corner_radius_bottom_left, rectangular_corner_radius_bottom_right, ...props }: grida.program.document.IComputedNodeReactRenderProps<grida.program.nodes.RectangleNode>) { - const isZero = - rectangular_corner_radius_top_left === 0 && - rectangular_corner_radius_top_right === 0 && - rectangular_corner_radius_bottom_left === 0 && - rectangular_corner_radius_bottom_right === 0; + const tl = rectangular_corner_radius_top_left ?? 0; + const tr = rectangular_corner_radius_top_right ?? 0; + const bl = rectangular_corner_radius_bottom_left ?? 0; + const br = rectangular_corner_radius_bottom_right ?? 0; + + const isZero = tl === 0 && tr === 0 && bl === 0 && br === 0; // ... - const widthApprox = css.toPxNumber(width) || 100; - const heightApprox = css.toPxNumber(height) || 100; + const widthApprox = css.toPxNumber(width) || 100; + const heightApprox = css.toPxNumber(height) || 100; + const minApprox = Math.max(1, Math.min(widthApprox, heightApprox)); const normalizedCornerRadius = useMemo((): [ number, number, number, number, ] => { if (isZero) return [0, 0, 0, 0]; return [ - ((rectangular_corner_radius_top_left ?? 0) * 100) / widthApprox, - ((rectangular_corner_radius_top_right ?? 0) * 100) / widthApprox, - ((rectangular_corner_radius_bottom_right ?? 0) * 100) / heightApprox, - ((rectangular_corner_radius_bottom_left ?? 0) * 100) / heightApprox, + (tl * 100) / minApprox, + (tr * 100) / minApprox, + (br * 100) / minApprox, + (bl * 100) / minApprox, ]; }, [ isZero, - rectangular_corner_radius_top_left, - rectangular_corner_radius_top_right, - rectangular_corner_radius_bottom_right, - rectangular_corner_radius_bottom_left, - widthApprox, - heightApprox, + tl, + tr, + br, + bl, + minApprox, ]);Also applies to: 51-79
editor/grida-canvas/reducers/node-transform.reducer.ts (1)
225-234: Handlenullreturn fromgetNodeAbsoluteBoundingRectgracefully.The assertion on line 234 will crash if the geometry cache is incomplete or the node hasn't been rendered yet. The assertion itself provides no recovery path. Other parts of the codebase explicitly check for null (e.g.,
wrap.tsline 204) and handle it gracefully. Consider an early return or fallback for robustness when resizing nodes that may not yet have cached geometry.editor/scaffolds/sidecontrol/sidecontrol-node-selection.tsx (4)
121-137: Fix stale closure inFontStyleControlScaffoldcallback (missing deps).
handleChangecaptureseditor+selectionbut the dependency array is[], so it can apply styles to an outdated selection after changes.Proposed fix
- const handleChange = React.useCallback( + const handleChange = React.useCallback( (key: editor.font_spec.FontStyleKey) => { selection.forEach((id) => { editor.changeTextNodeFontStyle(id, { fontStyleKey: key, }); }); }, - [] + [editor, selection] );
827-886: Avoidposition!and defaultlayout_positioningdefensively.
If legacy/partial nodes havelayout_positioningunset, theposition!assert can break the Position section at runtime.Proposed fix
- const { position, rotation, top, left, right, bottom } = useNodeState( + const { position, rotation, top, left, right, bottom } = useNodeState( node_id, (node) => ({ position: node.layout_positioning, rotation: node.rotation, top: node.layout_inset_top, left: node.layout_inset_left, right: node.layout_inset_right, bottom: node.layout_inset_bottom, }) ); + const positioning = position ?? "relative"; ... <PositioningConstraintsControl value={{ - layout_positioning: position!, + layout_positioning: positioning, layout_inset_top: top, layout_inset_left: left, layout_inset_right: right, layout_inset_bottom: bottom, }} ... <PositioningModeControl - value={position} + value={positioning} onValueChange={actions.positioningMode} />
915-962: Pass a concrete positioning mode intoPositioningModeControl(notgrida.mixed).
PositioningModeControllikely expects"absolute" | "relative", butmp.position?.valuemay begrida.mixed/undefined.Proposed fix
<PositioningModeControl - value={mp.position?.value} + value={position} onValueChange={(value) => { ids.forEach((id) => { instance.commands.changeNodePropertyPositioningMode(id, value); }); }} />
980-1091: Fixclips_contentlabel wiring + remove unsafe non-null assertions for gaps.
Checkboxusesid="control-clips_content"but the label useshtmlFor="control-clips-content"(mismatch → click label won’t toggle).layout_main_axis_gap!/layout_cross_axis_gapcan becomeundefinedand leak into controls/strings.Proposed fix
- <GapControl + <GapControl mode={layout_wrap === "wrap" ? "multiple" : "single"} value={{ - layout_main_axis_gap: layout_main_axis_gap!, - layout_cross_axis_gap: layout_cross_axis_gap, + layout_main_axis_gap: layout_main_axis_gap ?? 0, + layout_cross_axis_gap: layout_cross_axis_gap ?? 0, }} onValueCommit={actions.gap} /> ... - <Checkbox - id="control-clips_content" + <Checkbox + id="control-clips_content" checked={clips_content ?? false} onCheckedChange={(checked) => { actions.clipsContent(Boolean(checked)); }} /> <PropertyLineLabel - htmlFor="control-clips-content" + htmlFor="control-clips_content" className="w-auto min-w-auto text-foreground" > Clip content </PropertyLineLabel>- <PropertyLineLabel - htmlFor="control-clips_content" + <PropertyLineLabel + htmlFor="control-clips_content" className="w-auto min-w-auto text-foreground" > Clip content </PropertyLineLabel>Also applies to: 1258-1286
packages/grida-canvas-schema/grida.ts (1)
2645-2662: Factory can emit invalidContainerNode(missing required layout fields).
ContainerNodeextendsILayoutContainerTrait(positioning + width/height + flex/padding defaults). The factory branch only setsclips_contentand corner defaults, so callers that don’t supply layout fields viaprototypecan end up with nodes that violate assumptions downstream (rendering/CSS/layout).Proposed fix
case "container": { // Remove children from prototype before spreading to prevent leakage const { children, ...prototypeWithoutChildren } = prototype as any; return { name: prototype.type, type: prototype.type, active: true, locked: false, opacity: 1, z_index: 0, rotation: 0, corner_radius: 0, clips_content: true, + // layout defaults (match createContainerNode) + layout_positioning: "absolute", + layout_mode: "flow", + layout_direction: "horizontal", + layout_main_axis_alignment: "start", + layout_cross_axis_alignment: "start", + layout_main_axis_gap: 0, + layout_cross_axis_gap: 0, + layout_padding_top: 0, + layout_padding_right: 0, + layout_padding_bottom: 0, + layout_padding_left: 0, + layout_target_width: 100, + layout_target_height: 100, ...factory_default_traits.DEFAULT_RECTANGULAR_CORNER_RADIUS, ...prototypeWithoutChildren, id: id, } as ContainerNode; }editor/grida-canvas-hosted/playground/playground.tsx (1)
396-485: Abort fetch on cleanup to avoid leaking in-flight requests.
You allocate anAbortControllerbut never callabort()when the effect cleans up (you only flipcancelled).Proposed fix
useEffect(() => { let cancelled = false; + let controller: AbortController | null = null; const load = async () => { if (src) { // If src is provided, load it and persist to OPFS - const controller = new AbortController(); + controller = new AbortController(); setDocumentReady(false); try { const res = await fetch(src, { signal: controller.signal }); ... } catch (error) { - if (controller.signal.aborted) { + if (controller?.signal.aborted) { return; } ... } }; void load(); return () => { cancelled = true; + controller?.abort(); }; - }, [document, instance, src, opfs]); + }, [document, instance, src, opfs]);editor/grida-canvas-utils/css.ts (1)
41-174: Fix0width/height being dropped and add safe defaults for flex layout.Three bugs in the flex handling:
- Lines 121–122: Truthy check on
widthandheighttreats0as unset, dropping valid zero dimensions.- Line 166: Non-null assertion
direction!will throw ifdirectionis undefined, but it comes fromPartialstyles.- Lines 169–172:
layout_main_axis_gapandlayout_cross_axis_gapare used without nullish coalescing, potentially creating CSS"undefinedpx ..."values.Proposed fix
- width: width ? toDimension(width) : undefined, - height: height ? toDimension(height) : undefined, + width: width !== undefined ? toDimension(width) : undefined, + height: height !== undefined ? toDimension(height) : undefined, ... if (layout === "flex") { + const safeDirection = direction ?? "horizontal"; + const mainGap = layout_main_axis_gap ?? 0; + const crossGap = layout_cross_axis_gap ?? 0; result["display"] = "flex"; - result["flexDirection"] = axisToFlexDirection(direction!); + result["flexDirection"] = axisToFlexDirection(safeDirection); result["justifyContent"] = layout_main_axis_alignment; result["alignItems"] = layout_cross_axis_alignment; result["gap"] = - direction === "horizontal" - ? `${layout_main_axis_gap}px ${layout_cross_axis_gap}px` - : `${layout_cross_axis_gap}px ${layout_main_axis_gap}px`; + safeDirection === "horizontal" + ? `${mainGap}px ${crossGap}px` + : `${crossGap}px ${mainGap}px`; }Note:
MainAxisAlignmentandCrossAxisAlignmentenums use valid CSS keywords ("start", "end", "center", etc.) per the CSS Box Alignment spec, so direct assignment tojustify-contentandalign-itemsis correct.editor/grida-canvas/reducers/document.reducer.ts (1)
612-646: Placement bug: paste/insert translations only updatelayout_inset_left/top(ignores right/bottom-based nodes).In several places you apply deltas/placement by mutating only
layout_inset_leftandlayout_inset_top. If a node is positioned vialayout_inset_right/layout_inset_bottom(or mixes edges), it won’t move with the pasted/inserted group.At minimum, apply the delta to all defined inset edges (left/top/right/bottom). If the model supports “partial inset basis” (some edges undefined), keep that undefined-ness and only move the edges that exist.
Proposed fix (apply delta to all inset edges that are present)
- if ( - "layout_positioning" in node && - node.layout_positioning === "absolute" && - "layout_inset_left" in node && - "layout_inset_top" in node - ) { - node.layout_inset_left = - (node.layout_inset_left ?? 0) + delta[0]; - node.layout_inset_top = - (node.layout_inset_top ?? 0) + delta[1]; - } + if ("layout_positioning" in node && node.layout_positioning === "absolute") { + if ("layout_inset_left" in node && node.layout_inset_left !== undefined) { + node.layout_inset_left += delta[0]; + } + if ("layout_inset_top" in node && node.layout_inset_top !== undefined) { + node.layout_inset_top += delta[1]; + } + if ("layout_inset_right" in node && node.layout_inset_right !== undefined) { + node.layout_inset_right -= delta[0]; + } + if ("layout_inset_bottom" in node && node.layout_inset_bottom !== undefined) { + node.layout_inset_bottom -= delta[1]; + } + }(Apply the same pattern to the parent-rect offset blocks and the insertion/placement blocks.)
Also applies to: 854-886
crates/grida-canvas/src/io/io_grida.rs (1)
718-744: Inconsistent inset-basis handling causes silent data loss for non-container nodes with right/bottom positioning.
json_position_to_layout_basis()(lines 525–543) creates anInsetbasis when either right or bottom is present, but defaults missing edges to0.0. This violates CSS semantics: specifying onlyrightshouldn't implyleft=0.More critically, this function is used only for
ContainerNodeRec(line 1273). All other 9+ node types (GroupNodeRec,TextSpanNodeRec,RectangleNodeRec,CircleNodeRec,EllipseNodeRec,RegularPolygonNodeRec,RegularStarPolygonNodeRec,LineNodeRec,VectorNodeRec,BooleanOpNode) useAffineTransform::from_box_center()with only left/top parameters—right/bottom insets are completely ignored. If a non-container node is serialized with right/bottom insets and missing left/top, it will load at the origin.Clarify the positioning strategy: either model partial insets explicitly (Option per edge), or normalize all insets to left/top at load time using width/height and parent bounds.
Affected lines: 525–543, 1235–1242, 1273–1278, 1373–1379, 1457–1463, 1516–1522, 1576–1582, 1678–1684, 1736–1742, 1792–1798, 1837–1843, 1897–1903
editor/grida-canvas/editor.ts (2)
696-734: Avoid theILayoutTraitcast fromiosvg.convert()unless you validate shape.
Castingresulttogrida.program.nodes.i.ILayoutTraitcan hide missing/invalid fields from the SVG converter. If this is just to satisfyinsert()typing, prefer casting toNodePrototype(or assert required keys liketype/_$id/ layout fields).
5398-5473: Fix positioning calculation: subtract parent position before assigning tolayout_inset_left/top.In
autoSizeTextNode(), after computing the delta and new position, convert from absolute to parent-relative coordinates (like elsewhere in the codebase:layout_inset_left = absolute_x - parent_absolute_x). Currently using absolute rect coordinates directly as parent-relative values will misposition nested text nodes.
🤖 Fix all issues with AI agents
In @bin/activate-flatc:
- Around line 196-237: The _download_asset implementation must be hardened: add
a network timeout to urllib.request.urlopen calls (use the timeout parameter) to
avoid hangs; download to a temp/".part" file in the same directory (e.g.
download_path + ".part") and only atomically rename/replace it to download_path
(os.replace) after the full file is written and the SHA verified; use a simple
inter-process lock around the download_path (e.g. a lockfile or fcntl.lockf) so
concurrent invocations of _download_asset serialize and avoid double-writes; and
when extracting archives elsewhere in this module avoid ZipFile.extractall and
instead iterate members, validate each target path to prevent zip-slip (ensure
normalized destination path is under the intended extraction dir), create parent
dirs, and write files safely (atomic write into temp file then rename). Update
calls in _download_asset that use _release_url, _sha256_file, _ensure_dir and
_best_effort_ssl_context accordingly to perform the temp-file+verify+os.replace
flow and pass a timeout to urlopen.
In @editor/app/(dev)/canvas/editor.tsx:
- Around line 13-22: The Editor component forces warnOnUnsavedChanges to true by
placing it after the spread of props, preventing callers from overriding it; fix
this by moving the default prop before the spread so caller props win (e.g.,
render <PlaygroundCanvas warnOnUnsavedChanges {...props} />) or alternatively
set warnOnUnsavedChanges using a coalescing expression from props (e.g.,
warnOnUnsavedChanges={props.warnOnUnsavedChanges ?? true}) on the
PlaygroundCanvas invocation in the Editor function.
In @editor/grida-canvas-hosted/playground/playground.tsx:
- Around line 399-433: The load function currently uses fetch(src).json() which
fails for binary .grida formats; change load to fetch(src).then(res =>
res.arrayBuffer()) and attempt to parse in this order: 1) try io.GRID.decode(new
Uint8Array(buffer)) to detect GRID bytes and extract file.document, 2) if decode
throws, try TextDecoder to parse buffer as UTF-8 JSON and read file.document,
and 3) if still failing, check Content-Type or filename suffix for zip handling
and pass bytes to your ZIP parser; then call
instance.commands.reset(editor.state.init({ editable: true, document }), src)
and persist using opfs.get("document.grida").write(bytes) when you have the
original bytes. Apply the same strategy for the other load path referenced
around the 606-630 region.
- Around line 121-165: generateFileKeyFromSrc can return an empty string (e.g.,
"/" → ""), which leads to an invalid OPFS segment in usePlaygroundOPFS; update
generateFileKeyFromSrc to ensure it never returns empty by falling back to a
deterministic default (e.g., "root" or "index" or a short hash of src) when the
sanitized result is empty, and update the catch fallback the same way; also add
a defensive check in usePlaygroundOPFS to replace an empty filekey with the same
default before constructing new io.opfs.Handle (referencing
generateFileKeyFromSrc and usePlaygroundOPFS).
In @editor/grida-canvas-react-renderer-dom/nodes/rectangle.tsx:
- Around line 95-96: Replace the inline dangerouslySetInnerHTML usage for
fillDefs and strokeDefs with a small safe renderer component (e.g.,
DefsRenderer) that takes a SVG string prop, documents the trust boundary in a
comment, and uses DOMParser inside a useEffect to parse the SVG string into
nodes and append them to a container ref (clearing on cleanup) instead of
setting innerHTML; then render <DefsRenderer svgString={fillDefs} /> and
<DefsRenderer svgString={strokeDefs} /> in rectangle.tsx and include a clear
comment above DefsRenderer stating that the component consumes trusted SVG
markup (trust boundary).
In @editor/grida-canvas-react-starter-kit/starterkit-icons/node-type-icon.tsx:
- Around line 56-57: The icon switch was changed to handle case "tspan" but
toolbar and cursor mapping still emit "text", causing icons to fall back to
BoxIcon; update the toolbar types and mappings so node type is consistently
"tspan": change ToolbarToolType to return "tspan" instead of "text" in the
toolbar factory (the code that currently returns node: "text"), update the
toolbar_value_to_cursormode mapping to match the "tspan" case instead of "text",
and ensure schema definitions in @grida/schema and grida-tree accept "tspan" as
a valid node type so creation, rendering, and cursor logic are consistent with
the new TextIcon case.
In @editor/grida-canvas/reducers/document.reducer.ts:
- Around line 1028-1039: The code uses non-null assertions
node.layout_inset_left! and node.layout_inset_top! when computing anchor_points
(and earlier when building the vertices list), which can throw if insets are
absent; replace those asserts with nullish coalescing (node.layout_inset_left ??
0 and node.layout_inset_top ?? 0) wherever they are added to vertex positions
(inside the map/filter chain on node.vector_network.vertices and the preceding
vertices computation) so the vector snap path is resilient to missing insets as
done in the other vector clipboard paths.
- Around line 2239-2248: The flattened node currently inherits properties from
base (via ...base) but then sets explicit insets (layout_inset_left/top) treated
as absolute coordinates; remove or override any inherited layout_positioning
from base by forcing the flattened result to use absolute positioning (e.g., set
layout_positioning = "absolute" or delete layout_positioning) after spreading
base so the explicit layout_inset_* values are not combined with a relative
positioning mode—update the flatten_with_union path where
normalizeVectorNodeBBox and node.layout_inset_left/top are used to ensure the
resulting node cannot retain layout_positioning: "relative".
In @editor/public/examples/canvas/globals-01.grida:
- Around line 34-36: The nodes object has a mismatched key "text" while the
node's id is "tspan"; rename the object key from "text" to "tspan" so the key
matches the node's id ("tspan") and then update any links or references that
target or source "text" to instead reference "tspan" to keep lookups consistent.
In @format/grida.fbs:
- Around line 1184-1236: LayerTrait currently declares the field
parent:ParentReference (required) but the comment and ParentReference semantics
say the parent is optional for root nodes; remove the `(required)` qualifier (or
change to an explicit optional marker if your FBS variant uses one) so parent
can be null for root nodes, and keep or adjust the comment to match; reference
LayerTrait.parent and the ParentReference definition when making this change so
schema and documentation remain consistent.
- Around line 474-523: The four types FontOpticalSizing, TextLineHeight,
TextLetterSpacing, and TextWordSpacing (and their companion enums
FontOpticalSizingKind, TextLineHeightKind, TextLetterSpacingKind,
TextWordSpacingKind) are defined as flatbuffer structs which makes them
non‑evolvable; change them to tables (or model the variants as FlatBuffers
unions) so user-authored style variants can be extended in the future: replace
each struct declaration with a table declaration, move fixed_value and
factor_value into optional table fields (or create distinct variant tables used
by a union), keep or adapt the existing enum/union discriminant naming, and
update any code that constructs/parses these types to use table/union accessors
instead of struct layout. Ensure default values are set and update schema
comments/docs to reflect the new table/union design.
In @packages/grida-canvas-io/package.json:
- Around line 10-24: The package declares @grida/schema and @grida/cg under
devDependencies but they are imported at runtime (see imports in index.ts and
format.ts), so move "@grida/schema" and "@grida/cg" from devDependencies into
dependencies in package.json so they are installed for consumers; leave
"@grida/vn" in devDependencies. Update the dependencies section to include exact
entries for "@grida/schema" and "@grida/cg" and remove them from
devDependencies.
🟡 Minor comments (12)
editor/public/examples/canvas/component-01.grida-85-87 (1)
85-87: Inconsistent node identity: name doesn't match id.The node's
idis"description"(line 86) but itsnameis"tspan"(line 87). This inconsistency differs from the pattern used in the"title"node (lines 49-51), where bothidandnameremain"title"despite the type changing to"tspan".While this may not break functionality, it creates confusion and deviates from the consistent pattern elsewhere in the file.
♻️ Suggested fix for consistency
"description": { "id": "description", - "name": "tspan", + "name": "description",editor/grida-canvas/reducers/methods/flatten.ts-81-86 (1)
81-86: Use defensive coding for optional layout properties on lines 84 and 86.The properties
layout_inset_leftandlayout_inset_topare defined as optional in the schema (IPositioninginterface), yet the code uses non-null assertions on these lines. This is inconsistent with the pattern used forfill_ruleon line 79, which uses nullish coalescing (??).Similar defensive patterns exist elsewhere in the codebase (
utils/insertion.tschecks property existence before accessing). Apply the same approach here:layout_inset_left: (node as grida.program.nodes.UnknownNode).layout_inset_left ?? 0, layout_inset_top: (node as grida.program.nodes.UnknownNode).layout_inset_top ?? 0,packages/grida-format/vitest.config.ts-1-7 (1)
1-7: Add"types": ["vitest/globals"]to tsconfig.json if usingtest.globals: true, or remove globals and keep explicit imports.
test.globals: trueis supported in Vitest 4.0.16, but TypeScript needs to be told about the global types. Yourpackages/grida-format/tsconfig.jsondoesn't include"types": ["vitest/globals"]. While the test file uses explicit imports (so it works), this setup is inconsistent—either add the types to tsconfig to properly support globals, or removeglobals: trueand rely on explicit imports as your tests already do.Recommended fix (if keeping globals):
{ "compilerOptions": { "types": ["vitest/globals"], // ... rest of options } }packages/grida-canvas-io/index.ts-849-856 (1)
849-856: Check for missing file before decoding.The check
if (!snapshotJson)occurs after callingstrFromU8(files["snapshot.json"]). Iffiles["snapshot.json"]is undefined,strFromU8may throw or behave unexpectedly. Check for the file's existence first.🔧 Suggested fix
export function unpack(zipData: Uint8Array): SnapshotDocumentModel { const files = unzipSync(zipData); + if (!files["snapshot.json"]) { + throw new Error("Missing snapshot.json in zip file"); + } const snapshotJson = strFromU8(files["snapshot.json"]); - if (!snapshotJson) { - throw new Error("Missing snapshot.json in zip file"); - } return parse(snapshotJson) as SnapshotDocumentModel; }packages/grida-canvas-io/index.ts-692-695 (1)
692-695: Redundant null check on line 695.Line 692 assigns
documentfromfiles["document.grida"], and line 694 checksdocumentis truthy. Line 695 then checks!documentagain, which is unreachable since we're inside theif (document)block from line 694's compound condition.🔧 Suggested fix
if (manifest.document_file === "document.grida" && document) { - if (!document) return { kind: "unknown", bytes }; - const images: Record<string, Uint8Array> = {};packages/grida-canvas-io/index.ts-1038-1048 (1)
1038-1048: Add comment explaining why images/bitmaps can be safely excluded from FlatBuffer encoding.The destructuring removes
imagesandbitmaps, then castspersistedDocument as GridaDocument. While this works in practice (these fields are not persisted in FlatBuffer), the cast is unsafe from a type perspective sinceGridaDocumentrequires these fields. Thedecodefunction clarifies this with a comment—encodeshould include similar explanation for consistency. Consider either adding a comment like// Strip non-persisted fields; they're reinitialized to empty objects on decodeor using a more type-safe approach.editor/grida-canvas/reducers/methods/duplicate.ts-48-65 (1)
48-65: Remove redundant?? 0fallback and add explicit positioning mode check for clarity.The
?? 0on lines 55 and 62 is redundant since the precedingtypeof === "number"guard already ensures a number value. More importantly, add an explicitlayout_positioning === "absolute"check before applying the delta—this is the pattern used consistently elsewhere in the codebase (e.g., document.reducer.ts) and makes the intent clear, even though relative-positioned nodes will naturally haveundefinedinset values and skip the delta anyway.Suggested structure
if (nextdelta) { const clone_node = draft.document.nodes[clone_id]; if (clone_node.layout_positioning === "absolute") { if ("layout_inset_left" in clone_node && typeof clone_node.layout_inset_left === "number") { clone_node.layout_inset_left = clone_node.layout_inset_left + nextdelta[0]; } if ("layout_inset_top" in clone_node && typeof clone_node.layout_inset_top === "number") { clone_node.layout_inset_top = clone_node.layout_inset_top + nextdelta[1]; } draft.document.nodes[clone_id] = clone_node; } }packages/grida-format/src/__tests__/index.test.ts-14-52 (1)
14-52: Don’t hardcode the schema version in a round-trip test unless that’s the intent.This will fail on every version bump even if FlatBuffers read/write still works. Consider using a sentinel value (e.g.
"test-schema-version") and asserting it round-trips, or importing a single source of truth constant if you have one.editor/grida-canvas-utils/css.ts-268-287 (1)
268-287:toPxNumberdoc doesn’t match implementation (percentage case).
The comment says percentage returns 0–100, but the function currently returnsfallbackfor percentage values. Either implement percentage support or update the doc to avoid misleading callers.Proposed fix (implement percentage support)
export function toPxNumber( value?: grida.program.css.LengthPercentage | "auto", fallback = 0 ): number { if (!value || value === "auto") return fallback; if (typeof value === "number") { return value; } if (value.type === "length") { // Only convert px units to numbers; other units default to 0 return value.unit === "px" ? value.value : fallback; } - return fallback; + // percentage + return value.value; }editor/grida-canvas/editor.ts-2841-2850 (1)
2841-2850:archive()ZIP mime type is good; confirmio.archive.pack()output type is stable.
You’re forcingas BlobPart; ifpack()ever returns aBlobalready (or aUint8Array), you may double-wrap or mis-handle it. Consider normalizing explicitly (e.g., if it’sUint8Array, use.buffer).format/grida.fbs-981-990 (1)
981-990: Typo in trait name:CorerRadiusTraitshould beCornerRadiusTrait.
This is a public schema surface; fixing now is cheaper than carrying the typo forever (even if codecs map it).format/grida.fbs-96-132 (1)
96-132: Docs vs schema mismatch: Node IDs described as packed u32, but schema is string-basedNodeIdentifier.
This is fine as an incremental step, but the top “Key decisions” section implies the opposite. Recommend rewriting the header to match reality (or introducing the packed form now) to avoid downstream consumers baking in the wrong assumption.
| def _download_asset(asset: ResolvedAsset) -> str: | ||
| _ensure_dir(_downloads_dir()) | ||
| asset_name, asset_sha = asset | ||
| expected_sha = _normalize_sha256(asset_sha) | ||
| download_path = os.path.join(_downloads_dir(), asset_name) | ||
|
|
||
| # If already present and matches, reuse. | ||
| if os.path.exists(download_path): | ||
| got = _sha256_file(download_path) | ||
| if got.lower() == expected_sha: | ||
| return download_path | ||
| # Corrupt/old file - delete and re-download. | ||
| os.remove(download_path) | ||
|
|
||
| url = _release_url(asset_name) | ||
| try: | ||
| with urllib.request.urlopen(url) as r, open(download_path, "wb") as f: | ||
| shutil.copyfileobj(r, f) | ||
| except Exception: | ||
| # Some environments (notably macOS python.org builds) can lack a properly | ||
| # configured CA bundle, causing CERTIFICATE_VERIFY_FAILED for HTTPS. | ||
| # Retry with a best-effort SSL context that points at a known system CA | ||
| # bundle path when available. | ||
| ctx = _best_effort_ssl_context() | ||
| if ctx is None: | ||
| raise | ||
| with ( | ||
| urllib.request.urlopen(url, context=ctx) as r, | ||
| open(download_path, "wb") as f, | ||
| ): | ||
| shutil.copyfileobj(r, f) | ||
|
|
||
| got = _sha256_file(download_path) | ||
| if got.lower() != expected_sha: | ||
| raise RuntimeError( | ||
| f"SHA256 mismatch for {asset_name}\n" | ||
| f" expected: {expected_sha}\n" | ||
| f" got: {got}\n" | ||
| f" url: {url}\n" | ||
| ) | ||
| return download_path | ||
|
|
There was a problem hiding this comment.
Harden download/extract against hangs + zip-slip + concurrent writers.
Right now a stuck network read can hang builds (no timeout), and ZipFile.extractall(...) is zip-slip prone. Also, concurrent invocations can race on the same cache paths.
Proposed fix (timeout + atomic download + safe zip extraction)
@@
def _download_asset(asset: ResolvedAsset) -> str:
@@
- try:
- with urllib.request.urlopen(url) as r, open(download_path, "wb") as f:
- shutil.copyfileobj(r, f)
+ tmp_path = download_path + ".tmp"
+ try:
+ with urllib.request.urlopen(url, timeout=30) as r, open(tmp_path, "wb") as f:
+ shutil.copyfileobj(r, f)
except Exception:
@@
- with (
- urllib.request.urlopen(url, context=ctx) as r,
- open(download_path, "wb") as f,
- ):
- shutil.copyfileobj(r, f)
+ with (
+ urllib.request.urlopen(url, context=ctx, timeout=30) as r,
+ open(tmp_path, "wb") as f,
+ ):
+ shutil.copyfileobj(r, f)
+ os.replace(tmp_path, download_path)
@@
return download_path
+
+
+def _safe_extract_zip(z: zipfile.ZipFile, dest_dir: str) -> None:
+ dest_abs = os.path.abspath(dest_dir)
+ for info in z.infolist():
+ # Prevent zip-slip: ensure each output path stays within dest_dir.
+ out_path = os.path.abspath(os.path.join(dest_dir, info.filename))
+ if out_path == dest_abs or out_path.startswith(dest_abs + os.sep):
+ continue
+ raise RuntimeError(f"Refusing to extract unsafe path from zip: {info.filename}")
+ z.extractall(dest_dir)
@@
- with zipfile.ZipFile(zip_path) as z:
- z.extractall(install_dir)
+ with zipfile.ZipFile(zip_path) as z:
+ _safe_extract_zip(z, install_dir)Also applies to: 323-346
🤖 Prompt for AI Agents
In @bin/activate-flatc around lines 196 - 237, The _download_asset
implementation must be hardened: add a network timeout to urllib.request.urlopen
calls (use the timeout parameter) to avoid hangs; download to a temp/".part"
file in the same directory (e.g. download_path + ".part") and only atomically
rename/replace it to download_path (os.replace) after the full file is written
and the SHA verified; use a simple inter-process lock around the download_path
(e.g. a lockfile or fcntl.lockf) so concurrent invocations of _download_asset
serialize and avoid double-writes; and when extracting archives elsewhere in
this module avoid ZipFile.extractall and instead iterate members, validate each
target path to prevent zip-slip (ensure normalized destination path is under the
intended extraction dir), create parent dirs, and write files safely (atomic
write into temp file then rename). Update calls in _download_asset that use
_release_url, _sha256_file, _ensure_dir and _best_effort_ssl_context accordingly
to perform the temp-file+verify+os.replace flow and pass a timeout to urlopen.
| const load = async () => { | ||
| try { | ||
| const res = await fetch(src, { signal: controller.signal }); | ||
| if (!res.ok) { | ||
| throw new Error( | ||
| `Failed to fetch document: ${res.status} ${res.statusText}` | ||
| if (src) { | ||
| // If src is provided, load it and persist to OPFS | ||
| const controller = new AbortController(); | ||
| setDocumentReady(false); | ||
|
|
||
| try { | ||
| const res = await fetch(src, { signal: controller.signal }); | ||
| if (!res.ok) { | ||
| throw new Error( | ||
| `Failed to fetch document: ${res.status} ${res.statusText}` | ||
| ); | ||
| } | ||
| const file = await res.json(); | ||
| if (cancelled) { | ||
| return; | ||
| } | ||
| instance.commands.reset( | ||
| editor.state.init({ | ||
| editable: true, | ||
| document: file.document, | ||
| }), | ||
| src | ||
| ); | ||
|
|
||
| // Persist loaded document to OPFS | ||
| if (opfs) { | ||
| try { | ||
| const bytes = io.GRID.encode(file.document); | ||
| await opfs.get("document.grida").write(bytes); | ||
| } catch (error) { | ||
| console.error("Failed to persist src to OPFS:", error); | ||
| } | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
src load path assumes JSON; consider supporting .grida bytes to match the new format direction.
Right now fetch(src).json() will fail for binary .grida (and you already use io.GRID.decode(bytes) elsewhere). Consider detecting by Content-Type or by a try-decode strategy.
In @grida/io, what are the canonical ways to detect/parse a document from:
1) JSON payloads (e.g. { document: ... })
2) GRID bytes (.grida)
3) ZIP archives (.grida zip/bin)
? Is there a single "auto-detect" decode API?
Also applies to: 606-630
🤖 Prompt for AI Agents
In @editor/grida-canvas-hosted/playground/playground.tsx around lines 399 - 433,
The load function currently uses fetch(src).json() which fails for binary .grida
formats; change load to fetch(src).then(res => res.arrayBuffer()) and attempt to
parse in this order: 1) try io.GRID.decode(new Uint8Array(buffer)) to detect
GRID bytes and extract file.document, 2) if decode throws, try TextDecoder to
parse buffer as UTF-8 JSON and read file.document, and 3) if still failing,
check Content-Type or filename suffix for zip handling and pass bytes to your
ZIP parser; then call instance.commands.reset(editor.state.init({ editable:
true, document }), src) and persist using
opfs.get("document.grida").write(bytes) when you have the original bytes. Apply
the same strategy for the other load path referenced around the 606-630 region.
| {fillDefs && <g dangerouslySetInnerHTML={{ __html: fillDefs }} />} | ||
| {strokeDefs && <g dangerouslySetInnerHTML={{ __html: strokeDefs }} />} |
There was a problem hiding this comment.
Fix Biome “noDangerouslySetInnerHtml” errors (and document trust boundary).
Proposed minimal fix
- {fillDefs && <g dangerouslySetInnerHTML={{ __html: fillDefs }} />}
- {strokeDefs && <g dangerouslySetInnerHTML={{ __html: strokeDefs }} />}
+ {fillDefs && (
+ // biome-ignore lint/security/noDangerouslySetInnerHtml: SVG defs are generated internally (not user-supplied HTML).
+ <g dangerouslySetInnerHTML={{ __html: fillDefs }} />
+ )}
+ {strokeDefs && (
+ // biome-ignore lint/security/noDangerouslySetInnerHtml: SVG defs are generated internally (not user-supplied HTML).
+ <g dangerouslySetInnerHTML={{ __html: strokeDefs }} />
+ )}📝 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.
| {fillDefs && <g dangerouslySetInnerHTML={{ __html: fillDefs }} />} | |
| {strokeDefs && <g dangerouslySetInnerHTML={{ __html: strokeDefs }} />} | |
| {fillDefs && ( | |
| // biome-ignore lint/security/noDangerouslySetInnerHtml: SVG defs are generated internally (not user-supplied HTML). | |
| <g dangerouslySetInnerHTML={{ __html: fillDefs }} /> | |
| )} | |
| {strokeDefs && ( | |
| // biome-ignore lint/security/noDangerouslySetInnerHtml: SVG defs are generated internally (not user-supplied HTML). | |
| <g dangerouslySetInnerHTML={{ __html: strokeDefs }} /> | |
| )} |
🧰 Tools
🪛 ast-grep (0.40.4)
[warning] 95-95: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
[error] 95-95: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
[error] 96-96: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🤖 Prompt for AI Agents
In @editor/grida-canvas-react-renderer-dom/nodes/rectangle.tsx around lines 95 -
96, Replace the inline dangerouslySetInnerHTML usage for fillDefs and strokeDefs
with a small safe renderer component (e.g., DefsRenderer) that takes a SVG
string prop, documents the trust boundary in a comment, and uses DOMParser
inside a useEffect to parse the SVG string into nodes and append them to a
container ref (clearing on cleanup) instead of setting innerHTML; then render
<DefsRenderer svgString={fillDefs} /> and <DefsRenderer svgString={strokeDefs}
/> in rectangle.tsx and include a clear comment above DefsRenderer stating that
the component consumes trusted SVG markup (trust boundary).
| /// Rust: `TextDecorationRec` | ||
| /// | ||
| /// Archive model notes: | ||
| /// - `decoration_color` uses RGBA32F (float space) to match this schema's color choice. | ||
| /// - When `decoration_color.a == 0`, runtimes may treat it as "unset" if desired. | ||
| table TextDecorationRec { | ||
| text_decoration_line:TextDecorationLine = None (id: 0); | ||
| text_decoration_color:RGBA32F (id: 1); | ||
| text_decoration_style:TextDecorationStyle = Solid (id: 2); | ||
| text_decoration_skip_ink:bool = true (id: 3); | ||
| text_decoration_thickness:float = 1.0 (id: 4); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's confirm the file exists and check the lines in question
wc -l format/grida.fbsRepository: gridaco/grida
Length of output: 78
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '550,575p' format/grida.fbsRepository: gridaco/grida
Length of output: 896
🏁 Script executed:
# Search for earlier mentions of sentinel values or schema evolution rules
rg -i "sentinel|unset|evolution" format/grida.fbs -B 2 -A 2Repository: gridaco/grida
Length of output: 3569
🏁 Script executed:
# Look for comments about RGBA32F usage
rg "RGBA32F" format/grida.fbs -B 2 -A 2Repository: gridaco/grida
Length of output: 1328
🏁 Script executed:
# Search for nullable patterns in the schema (common ways to handle "unset")
rg "nullable|union|optional" format/grida.fbs | head -20Repository: gridaco/grida
Length of output: 1071
Schema evolution rule violation: using RGBA32F.a == 0 as sentinel for "unset" contradicts documented constraints.
The schema explicitly forbids treating scalar defaults as sentinels: "If 0 or the default enum value has real semantic meaning, it MUST NOT be used to represent 'unset'... If the distinction between 'unset' and 'set' matters, the value MUST be wrapped in a nullable table or expressed via a union."
Since RGBA32F.a == 0 is a valid transparent color, treating it as "unset" loses intent and breaks round-tripping. Either make text_decoration_color nullable (wrap in a table), use a union, or change the sentinel if the distinction truly matters.
| /// Universal node trait. | ||
| table SystemNodeTrait { | ||
| /// unique identifier for the node. (required) | ||
| id:NodeIdentifier (required); | ||
| /// name of the node. (optional) | ||
| name:string; | ||
| /// whether the node is active. (visible, active) | ||
| active:bool; | ||
| /// whether the node is locked. (locked, editor) | ||
| locked:bool; | ||
| } | ||
|
|
||
|
|
||
| /// Shared layer fields used by all layer-node variants. | ||
| /// Layer is what usually user think of as a node. each layer is non-virtual, a real render target. | ||
| table LayerTrait { | ||
| /// Parent reference (optional; root nodes omit this). | ||
| /// | ||
| /// When present, this node is a child of the referenced parent. | ||
| /// Children are ordered by `position` (lexicographic sort). | ||
| /// | ||
| /// Reference: Aligned with Figma's `ParentIndex` pattern (fig.kiwi). | ||
| /// This replaces the previous `children:[NodeIdentifier]` array model. | ||
| /// | ||
| /// To compute children of a parent node: | ||
| /// 1. Filter all nodes where `parent.parent_id == parent.id` | ||
| /// 2. Sort by `parent.position` (lexicographic comparison) | ||
| parent:ParentReference (required); | ||
|
|
||
| /// opacity of the layer. | ||
| opacity:float = 1.0; | ||
|
|
||
| /// Blend mode (archive model). | ||
| /// | ||
| /// Default is `pass_through`. | ||
| blend_mode:LayerBlendMode = PassThrough; | ||
|
|
||
| /// Rust: `LayerMaskType` (union; default is Image(alpha) since it's the first union member) | ||
| mask_type:LayerMaskType; | ||
|
|
||
| /// layer effects. | ||
| effects:LayerEffects; | ||
|
|
||
| /// Geometry transform baseline (identity by default). | ||
| // relative_transform_snapshot:CGTransform2D; | ||
|
|
||
| /// Layout (optional, depending on node type / usage). | ||
| layout:Layout; | ||
|
|
||
| /// post-layout transform, this reflects how css transform is applied after the layout is resolved. this is currently how grida handles rotation. in the future, this might change. | ||
| post_layout_transform:CGTransform2D; | ||
| post_layout_transform_origin:Alignment; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the exact content of the file at the specified lines
head -n 1236 format/grida.fbs | tail -n +1184 | cat -nRepository: gridaco/grida
Length of output: 2264
🏁 Script executed:
# Let's also see a bit more context before and after
head -n 1250 format/grida.fbs | tail -n +1170Repository: gridaco/grida
Length of output: 2554
🏁 Script executed:
# Search for root node references in the codebase
rg -i "root" format/grida.fbs -A 3 -B 3Repository: gridaco/grida
Length of output: 977
🏁 Script executed:
# Search for how parent is used when it's optional or not
rg "parent.*required" format/ -A 2 -B 2Repository: gridaco/grida
Length of output: 647
🏁 Script executed:
# Search for ParentReference definition
rg "table ParentReference" format/grida.fbs -A 10Repository: gridaco/grida
Length of output: 443
🏁 Script executed:
# Search for how nodes are actually used/instantiated, especially root nodes
rg "SceneNode\|root.*node\|parent.*null\|parent.*optional" format/grida.fbs -B 2 -A 5Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look for any Rust code or implementation that handles parent field
fd -e rs -e ts -e tsx -e js -e jsx | head -20 | xargs rg -l "LayerTrait\|parent.*omit\|root.*node" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check how the hierarchy is actually managed in code
rg "parent_id\|ParentReference" --max-count=20 format/grida.fbs -B 1 -A 1Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Search for document structure and how nodes are organized
rg "CanvasDocument\|nodes\|children" format/grida.fbs -A 3 -B 1 | head -40Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look for Scene or root node definition
rg "SceneNode\|table Scene" format/grida.fbs -A 10 | head -50Repository: gridaco/grida
Length of output: 39
Schema contradiction: LayerTrait.parent is marked (required) but comments state it's optional for root nodes.
The comment on line 17 explicitly says "Parent reference (optional; root nodes omit this)", and the ParentReference table definition also references "null for root nodes". However, the field declaration on line 28 marks it as (required). Either make parent optional to allow true root nodes as the comment describes, or require all nodes to have a parent (e.g., under a Scene) and update the comment accordingly.
Proposed fix (make optional)
table LayerTrait {
- parent:ParentReference (required);
+ parent:ParentReference;
opacity:float = 1.0;
blend_mode:LayerBlendMode = PassThrough;
mask_type:LayerMaskType;
effects:LayerEffects;
layout:Layout;
post_layout_transform:CGTransform2D;
post_layout_transform_origin:Alignment;
}📝 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.
| /// Universal node trait. | |
| table SystemNodeTrait { | |
| /// unique identifier for the node. (required) | |
| id:NodeIdentifier (required); | |
| /// name of the node. (optional) | |
| name:string; | |
| /// whether the node is active. (visible, active) | |
| active:bool; | |
| /// whether the node is locked. (locked, editor) | |
| locked:bool; | |
| } | |
| /// Shared layer fields used by all layer-node variants. | |
| /// Layer is what usually user think of as a node. each layer is non-virtual, a real render target. | |
| table LayerTrait { | |
| /// Parent reference (optional; root nodes omit this). | |
| /// | |
| /// When present, this node is a child of the referenced parent. | |
| /// Children are ordered by `position` (lexicographic sort). | |
| /// | |
| /// Reference: Aligned with Figma's `ParentIndex` pattern (fig.kiwi). | |
| /// This replaces the previous `children:[NodeIdentifier]` array model. | |
| /// | |
| /// To compute children of a parent node: | |
| /// 1. Filter all nodes where `parent.parent_id == parent.id` | |
| /// 2. Sort by `parent.position` (lexicographic comparison) | |
| parent:ParentReference (required); | |
| /// opacity of the layer. | |
| opacity:float = 1.0; | |
| /// Blend mode (archive model). | |
| /// | |
| /// Default is `pass_through`. | |
| blend_mode:LayerBlendMode = PassThrough; | |
| /// Rust: `LayerMaskType` (union; default is Image(alpha) since it's the first union member) | |
| mask_type:LayerMaskType; | |
| /// layer effects. | |
| effects:LayerEffects; | |
| /// Geometry transform baseline (identity by default). | |
| // relative_transform_snapshot:CGTransform2D; | |
| /// Layout (optional, depending on node type / usage). | |
| layout:Layout; | |
| /// post-layout transform, this reflects how css transform is applied after the layout is resolved. this is currently how grida handles rotation. in the future, this might change. | |
| post_layout_transform:CGTransform2D; | |
| post_layout_transform_origin:Alignment; | |
| } | |
| /// Universal node trait. | |
| table SystemNodeTrait { | |
| /// unique identifier for the node. (required) | |
| id:NodeIdentifier (required); | |
| /// name of the node. (optional) | |
| name:string; | |
| /// whether the node is active. (visible, active) | |
| active:bool; | |
| /// whether the node is locked. (locked, editor) | |
| locked:bool; | |
| } | |
| /// Shared layer fields used by all layer-node variants. | |
| /// Layer is what usually user think of as a node. each layer is non-virtual, a real render target. | |
| table LayerTrait { | |
| /// Parent reference (optional; root nodes omit this). | |
| /// | |
| /// When present, this node is a child of the referenced parent. | |
| /// Children are ordered by `position` (lexicographic sort). | |
| /// | |
| /// Reference: Aligned with Figma's `ParentIndex` pattern (fig.kiwi). | |
| /// This replaces the previous `children:[NodeIdentifier]` array model. | |
| /// | |
| /// To compute children of a parent node: | |
| /// 1. Filter all nodes where `parent.parent_id == parent.id` | |
| /// 2. Sort by `parent.position` (lexicographic comparison) | |
| parent:ParentReference; | |
| /// opacity of the layer. | |
| opacity:float = 1.0; | |
| /// Blend mode (archive model). | |
| /// | |
| /// Default is `pass_through`. | |
| blend_mode:LayerBlendMode = PassThrough; | |
| /// Rust: `LayerMaskType` (union; default is Image(alpha) since it's the first union member) | |
| mask_type:LayerMaskType; | |
| /// layer effects. | |
| effects:LayerEffects; | |
| /// Geometry transform baseline (identity by default). | |
| // relative_transform_snapshot:CGTransform2D; | |
| /// Layout (optional, depending on node type / usage). | |
| layout:Layout; | |
| /// post-layout transform, this reflects how css transform is applied after the layout is resolved. this is currently how grida handles rotation. in the future, this might change. | |
| post_layout_transform:CGTransform2D; | |
| post_layout_transform_origin:Alignment; | |
| } |
🤖 Prompt for AI Agents
In @format/grida.fbs around lines 1184 - 1236, LayerTrait currently declares the
field parent:ParentReference (required) but the comment and ParentReference
semantics say the parent is optional for root nodes; remove the `(required)`
qualifier (or change to an explicit optional marker if your FBS variant uses
one) so parent can be null for root nodes, and keep or adjust the comment to
match; reference LayerTrait.parent and the ParentReference definition when
making this change so schema and documentation remain consistent.
| layout_padding_top: paddingTop ?? 0, | ||
| layout_padding_right: paddingRight ?? 0, | ||
| layout_padding_bottom: paddingBottom ?? 0, | ||
| layout_padding_left: paddingLeft ?? 0, | ||
| }; | ||
|
|
||
| return { | ||
| expanded, | ||
| padding, | ||
| layout: "flow" as const, | ||
| direction: "horizontal" as const, | ||
| main_axis_alignment: "start" as const, | ||
| cross_axis_alignment: "start" as const, | ||
| main_axis_gap: node.itemSpacing ?? 0, | ||
| cross_axis_gap: node.counterAxisSpacing ?? node.itemSpacing ?? 0, | ||
| layout_mode: "flow" as const, | ||
| layout_direction: "horizontal" as const, | ||
| layout_main_axis_alignment: "start" as const, | ||
| layout_cross_axis_alignment: "start" as const, | ||
| layout_main_axis_gap: node.itemSpacing ?? 0, | ||
| layout_cross_axis_gap: | ||
| node.counterAxisSpacing ?? node.itemSpacing ?? 0, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the full context of the function to understand padding structure
# First, let's find and read the full function implementation
fd -e ts "lib.ts" packages/grida-canvas-io-figma/ | head -5Repository: gridaco/grida
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Read the file to understand the full context around lines 600-616
cat -n packages/grida-canvas-io-figma/lib.ts | sed -n '570,650p'Repository: gridaco/grida
Length of output: 3064
🏁 Script executed:
#!/bin/bash
# Search for type definitions of the returned object and padding structure
rg -n "padding.*layout_padding|layout_padding" packages/grida-canvas-io-figma/ -A 2 -B 2Repository: gridaco/grida
Length of output: 682
🏁 Script executed:
#!/bin/bash
# Check how consumers use the padding property
rg -n "\.padding\s*[=:]|padding\s*\?" packages/grida-canvas-schema/ packages/grida-canvas-core/ -t ts -A 2 | head -50Repository: gridaco/grida
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Find type definitions or interfaces for container layout
rg -n "interface.*Container|type.*Container|padding.*layout_padding" packages/grida-canvas-io-figma/ -t ts -B 2 -A 4 | head -80Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Find where container_layout_trait is called and how padding is used
rg -n "container_layout_trait" packages/grida-canvas-io-figma/ -t ts -B 2 -A 5Repository: gridaco/grida
Length of output: 2088
🏁 Script executed:
#!/bin/bash
# Look for schema definitions in the canvas packages
fd -e ts "schema\|type" packages/grida-canvas-* | grep -E "(schema|type)" | head -20Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Search for how padding is handled in the schema packages
fd -type d packages/grida-canvas-*/ | head -10Repository: gridaco/grida
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Find the ContainerNode type definition
rg -n "ContainerNode|interface.*Container" packages/grida-canvas-schema/ packages/grida-canvas-core-types/ -t ts -B 2 -A 8 | head -100Repository: gridaco/grida
Length of output: 6951
🏁 Script executed:
#!/bin/bash
# Search more broadly for node type definitions
fd -name "*.ts" -type f packages/grida-canvas-* | xargs grep -l "ContainerNode" | head -5Repository: gridaco/grida
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Look at package structure to find type definitions
ls -la packages/ | grep grida-canvasRepository: gridaco/grida
Length of output: 1129
🏁 Script executed:
#!/bin/bash
# Get the full ContainerNode interface definition
cat -n packages/grida-canvas-schema/grida.ts | sed -n '2306,2350p'Repository: gridaco/grida
Length of output: 1406
🏁 Script executed:
#!/bin/bash
# Look for IPadding interface definition to understand padding structure
rg -n "interface IPadding" packages/grida-canvas-schema/ -t ts -A 15Repository: gridaco/grida
Length of output: 1110
🏁 Script executed:
#!/bin/bash
# Check if there's a "padding" property (not layout_padding_*) in the schema
rg -n "padding\s*:" packages/grida-canvas-schema/grida.ts -B 2 -A 2 | grep -v "layout_padding"Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
#!/bin/bash
# Get the full IPadding interface to see all properties
cat -n packages/grida-canvas-schema/grida.ts | sed -n '1571,1605p'Repository: gridaco/grida
Length of output: 1286
🏁 Script executed:
#!/bin/bash
# Check ILayoutContainerTrait more carefully
cat -n packages/grida-canvas-schema/grida.ts | sed -n '2120,2130p'Repository: gridaco/grida
Length of output: 467
Fix padding property structure to match schema.
The container_layout_trait function returns a padding property that doesn't exist in the schema. The ContainerNode type expects individual layout_padding_* properties (defined in IPadding interface at line 1576–1591 of the schema), not a wrapper padding property.
Currently, the function returns either padding: number or padding: { layout_padding_top, ... }, but it should instead spread the individual padding properties directly into the return object, or return nothing if all padding values are undefined (since IPadding is partial).
…ock.yaml to include @grida/cg and @grida/schema
…n text styles; refactor stroke width encoding for rectangles
…ine height; refactor FeBlur handling to use unions
…d ensure null/unset values are correctly encoded and decoded
… order preservation
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @format/grida.fbs:
- Around line 990-993: Rename the misspelled table CorerRadiusTrait to
CornerRadiusTrait in the schema and update all references to that symbol (e.g.,
usages expecting CorerRadiusTrait) accordingly; ensure the field names
corner_radius and corner_smoothing remain unchanged and verify the duplicate
occurrence referenced (around the other instance at the later occurrence) is
renamed as well so no references or generated bindings still use the old
CorerRadiusTrait identifier.
- Around line 1238-1250: The field LayerTrait.parent is declared as required but
the comment says it is optional for root nodes; fix by either making the schema
match the comment or vice versa: (a) prefer making it optional — remove
“(required)” from the ParentReference declaration (i.e., change
parent:ParentReference (required); to parent:ParentReference;), update any
serialization/deserialization code to accept null/missing parent values and
ensure codecs handling ParentReference tolerate absence, or (b) keep it required
— update the comment block to remove the “optional/root omission” language and
audit any upstream assumptions that treat parent as nullable; reference the
ParentReference type and LayerTrait.parent field when applying the change.
- Around line 532-536: The FontOpticalSizing struct is the only struct with a
default on a field which violates the schema rule that structs cannot have
defaults; remove the default assignment from the kind field in struct
FontOpticalSizing (i.e., delete the "= Auto" default on
kind:FontOpticalSizingKind) so the struct matches the other structs in the file
and complies with the documented rule.
- Around line 1259-1261: The comment on the union field mask_type:LayerMaskType
is misleading; FlatBuffers unions implicitly default to NONE (type 0) when
absent, not the first explicit member, so remove or replace the line that claims
"default is Image(alpha) since it's the first union member" with a concise
accurate note stating that unions default to NONE/null and that if you need
Image(alpha) semantics when the field is omitted you must explicitly encode
LayerMaskTypeImage during serialization (or handle the absent case in
deserialization); ensure references to LayerMaskType, mask_type, and
LayerMaskTypeImage are updated accordingly.
In @packages/grida-canvas-io/__tests__/format-roundtrip.test.ts:
- Around line 2371-2420: The test "roundtrips ImagePaint stroke_paints on
ContainerNode" has an input ImagePaint with fit: "fill" but asserts paint.fit
=== "cover"; update the test so the expected fit matches the input (or vice
versa) — e.g., in the roundtripTest assertion for containerNode.paint set
expect(paint.fit).toBe("fill") if you expect preservation, or change the input
fit to "cover" and rename the test if normalization to cover is intended; adjust
only the createDocument input or the expect line in the test (symbols:
createDocument, baseContainer, roundtripTest, containerNode, stroke_paints,
paint).
- Around line 1004-1074: The failing assertion is because
createDocument(sceneId, ...) sets entry_scene_id to sceneId by default; update
the test in "comprehensive integration" to assert that decoded.entry_scene_id
equals sceneId (use expect(decoded.entry_scene_id).toBe(sceneId)) instead of
expecting undefined, or alternatively remove the default-setting in
createDocument if you intend entry_scene_id to be omitted—but most
straightforward is to change the assertion to expect sceneId to match the
roundtrip behavior.
In @packages/grida-canvas-io/package.json:
- Around line 10-20: Your dependency on "flatbuffers" must be pinned to
v25.12.19 to match the activate-flatc script; update the "flatbuffers" entry in
packages/grida-canvas-io (and packages/grida-format) from the loose semver
"^25.9.23" to the exact version "25.12.19", or better add a root-level pnpm
"overrides" entry in the root package.json that forces "flatbuffers" ->
"25.12.19" so all workspace packages use the same runtime as the activate-flatc
tool.
In @packages/grida-format/src/index.ts:
- Around line 1-13: The index.ts re-exports missing FlatBuffers modules which
breaks compilation: remove or fix the broken exports (export * from "./grida";
and the named exports unionToPaint, unionListToPaint, unionToNode,
unionListToNode, unionToFeBlur, unionListToFeBlur) by either running the
FlatBuffers generation to produce the ./grida folder and its paint/node/fe-blur
modules, or delete/replace these export lines with the correct generated module
paths or alternative public API symbols that actually exist in the package;
ensure the final index.ts only exports symbols that are present so TypeScript
can compile.
🧹 Nitpick comments (4)
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts (2)
6-231: Nice test scaffolding, but consider DRY-ing the base-node factories a bit.The
base*helpers make the suite readable, but there’s a lot of repeated “layer-ish” defaults (id/name/active/locked/opacity/z_index/layout/rotation/stroke defaults). A smallbaseLayer(id, overrides)(or similar) would reduce churn when the schema evolves again.
269-281: Prefer Vitest assertions overthrowinroundtripTest()for better failure output.A failed roundtrip currently throws generic errors; using
expect(node).toBeDefined()/expect(node.type).toBe(type)will produce nicer diffs and keep failures consistent with the rest of the suite.format/grida.fbs (2)
739-741: Field id strategy is inconsistent (some tables use(id: N), others omit), which is risky for long-term evolution.
For a “future-proof, evolvable schema”, I’d strongly recommend consistently assigning(id: N)for all fields in all tables and avoiding odd gaps likePaintStackItem.paintusing(id: 1)as the first field.Also applies to: 717-728
1117-1136:layout_target_aspect_ratio:CGSizeuses a struct +(0,0)sentinel (“unset”), which partially contradicts the schema’s own rules.
If(0,0)is truly invalid by definition, this is acceptable; otherwise consider a nullable table wrapper to preserve intent without sentinel coupling.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
crates/grida-canvas-wasm/example/rectangle.gridaeditor/public/examples/canvas/globals-01.gridaformat/grida.fbspackages/grida-canvas-io/__tests__/format-roundtrip.test.tspackages/grida-canvas-io/format.tspackages/grida-canvas-io/package.jsonpackages/grida-format/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/public/examples/canvas/globals-01.grida
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript 5 as the main language for most apps
Use Lucide or Radix Icons for icons
Files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.tspackages/grida-format/src/index.ts
{editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
packages/grida-canvas-*/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
🧠 Learnings (22)
📓 Common learnings
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to packages/grida-canvas-*/**/*.{ts,tsx,js,jsx} : Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Handle NodeId to UserNodeId conversion via `IdConverter` during .grida file loading
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `NodeId` (u64) for internal structs (NodeRecs, SceneGraph, caches) in the rendering engine for high-performance operations
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to packages/grida-canvas-*/**/*.{ts,tsx,js,jsx} : Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.tspackages/grida-format/src/index.tspackages/grida-canvas-io/package.jsoncrates/grida-canvas-wasm/example/rectangle.grida
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.tspackages/grida-format/src/index.tspackages/grida-canvas-io/package.jsoncrates/grida-canvas-wasm/example/rectangle.gridaformat/grida.fbs
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/*.rs : Run all tests with: `cargo test`
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.tspackages/grida-canvas-io/package.json
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/scenario_*.rs : Scenario-specific comprehensive tests should be organized in files matching `scenario_*.rs`
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.grida : Validate .grida files using the `tool_io_grida` CLI tool to check file structure, parse all nodes, and detect parsing errors
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.tspackages/grida-canvas-io/package.jsoncrates/grida-canvas-wasm/example/rectangle.grida
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/ui_parser_test.rs : High-level UI API tests should use `parse_ui` and be organized in `ui_parser_test.rs`
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Use Canvas 2D API with path commands for rendering geometric shapes (circle, square, triangle, etc.)
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo fmt` to maintain code formatting standards
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.tspackages/grida-canvas-io/package.jsoncrates/grida-canvas-wasm/example/rectangle.grida
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to editor/grida-*/**/*.{ts,tsx} : Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to <root>/packages
Applied to files:
packages/grida-format/src/index.tspackages/grida-canvas-io/package.json
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/+(grida-canvas-wasm.js|grida-canvas-wasm.wasm) : Include WASM artifacts (`grida-canvas-wasm.js` and `grida-canvas-wasm.wasm`) in git for faster CI builds
Applied to files:
packages/grida-canvas-io/package.jsoncrates/grida-canvas-wasm/example/rectangle.grida
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/Cargo.toml : Include dev-dependency `serde_json = "1.0"` for JSON testing
Applied to files:
packages/grida-canvas-io/package.json
📚 Learning: 2025-12-01T00:22:36.510Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-dev/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:36.510Z
Learning: Applies to crates/grida-dev/grida-dev/Cargo.toml : The `grida-dev` crate is marked `publish = false` and is intended solely for local development workflows and devtools
Applied to files:
packages/grida-canvas-io/package.json
📚 Learning: 2025-12-01T00:22:36.510Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-dev/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:36.510Z
Learning: The `cg` crate must remain platform-agnostic; all winit/glutin integration code must live in the `grida-dev` crate
Applied to files:
packages/grida-canvas-io/package.json
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/Cargo.toml : Set environment variables `CC=emcc`, `CXX=em++`, and `AR=emar` when building Rust code for WebAssembly targets
Applied to files:
packages/grida-canvas-io/package.json
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo clippy --no-deps --all-targets --all-features` for linting to check style, performance, and correctness suggestions
Applied to files:
packages/grida-canvas-io/package.jsoncrates/grida-canvas-wasm/example/rectangle.grida
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `NodeId` (u64) for internal structs (NodeRecs, SceneGraph, caches) in the rendering engine for high-performance operations
Applied to files:
crates/grida-canvas-wasm/example/rectangle.gridaformat/grida.fbs
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Auto-generate IDs (ID=0) in `NodeRepository` for factory-created nodes
Applied to files:
crates/grida-canvas-wasm/example/rectangle.gridaformat/grida.fbs
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/*.{js,ts,jsx,tsx} : Use Web Workers for heavy graphics operations to improve performance and responsiveness
Applied to files:
crates/grida-canvas-wasm/example/rectangle.grida
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Handle NodeId to UserNodeId conversion via `IdConverter` during .grida file loading
Applied to files:
crates/grida-canvas-wasm/example/rectangle.gridaformat/grida.fbs
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Maintain bidirectional mapping between NodeId and UserNodeId at the application layer for API boundary management
Applied to files:
format/grida.fbs
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `UserNodeId` (String) for public APIs that accept or return node IDs for stability and serialization
Applied to files:
format/grida.fbs
🧬 Code graph analysis (1)
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts (3)
packages/grida-canvas-schema/grida.ts (8)
SceneNode(2155-2169)RectangleNode(2455-2464)TextSpanNode(2202-2220)VectorNode(2393-2409)Node(1184-1184)Document(866-876)format(126-133)RGBA32F(484-490)packages/grida-canvas-io-figma/lib.ts (2)
document(673-974)node(2268-2310)packages/grida-canvas-io/format.ts (7)
scene(4410-4562)type(666-668)type(1657-1661)node(1026-1650)alignment(479-485)paint(1993-2019)paint(2262-2277)
⏰ 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: Cursor Bugbot
- GitHub Check: test
🔇 Additional comments (7)
crates/grida-canvas-wasm/example/rectangle.grida (2)
2-2: LGTM! Version bump reflects the breaking schema changes.The minor version increment and updated build metadata are appropriate for this breaking change that introduces the new layout_* field naming convention.
10-12: LGTM! Layout field renaming is consistent with the new schema.The field renaming follows the standardized
layout_*convention. The new names (layout_positioning,layout_inset_*,layout_target_*) are more explicit and descriptive while preserving the original values. File structure is valid.packages/grida-canvas-io/__tests__/format-roundtrip.test.ts (1)
232-267:createDocument()link ordering +entry_scene_iddefault deserve a quick sanity check.
links: { [sceneId]: Object.keys(nodes) }will follow JS property insertion order (usually stable), but if a caller constructsnodesdynamically, this can become accidental test flakiness.entry_scene_idis always set (defaults tosceneId). Some tests later expect it to decode asundefined, which suggests either (a) the test is wrong, or (b) encode/decode intentionally drops it when “default”.packages/grida-canvas-io/package.json (1)
11-16: Dependency-type move looks right—please double-check this package’s published/consumed surface.Moving
@grida/cg/@grida/schemainto"dependencies"is correct if they’re imported by runtime code (not only tests/tooling). Since packages under/packages/grida-canvas-*power the canvas (and some are published), it’s worth confirming bundling/install expectations even though this one is currently"private": true. Based on learnings, ensure this doesn’t accidentally bloat downstream consumers.Verification: the script in the previous comment’s section (step 1) should show runtime import sites.
Also applies to: 22-24
format/grida.fbs (3)
1467-1481: Union-per-variant node payload modeling is a solid choice here.
The rationale is clear and aligns with forward evolution; just ensure every consumer treats unknown union members as skippable/UnknownNoderather than failing hard.
1487-1503:CanvasDocument.nodes:[Node]isn’t actually a “map”; confirm lookup strategy for large docs.
If large-doc performance matters, document that loaders must build an in-memory index keyed bySystemNodeTrait.id(or introduce an explicit{id, payload}entry table).
551-555: Remove this comment —value:float = nullis valid FlatBuffers syntax for optional scalars.FlatBuffers documentation explicitly supports
= nullon scalar fields to create optional scalars that can represent "unset" values. This syntax compiles correctly with flatc (v25.12.19, as pinned in this repo). The schema as written will not fail to compile.While there may be a design question about whether optional scalars or table-based presence patterns better suit your use case, the current implementation is not a compilation error or invalid syntax.
Likely an incorrect or invalid review comment.
| // Key decisions (draft): | ||
| // - Node IDs are stored as packed u32 (actor:8 | counter:24), matching the DB draft. | ||
| // - TS `grida.id.u32` can pack/unpack/format to/from string. | ||
| // - Document is a flat node map + explicit ordered link list (no nested children arrays). | ||
| // - Node payload is modeled via a FlatBuffers union for forward evolution. | ||
| // |
There was a problem hiding this comment.
Resolve the Node ID representation contradiction (packed u32 vs string table).
Line 12 says IDs are “packed u32”, but NodeIdentifier (Line 139+) is currently string-based. Either update the header “Key decisions” or switch the schema now; keeping both statements will mislead codec implementers.
Also applies to: 132-142
| struct FontOpticalSizing { | ||
| kind:FontOpticalSizingKind = Auto; | ||
| value:float; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and explore the repository structure
git ls-files | head -20
find . -name "grida.fbs" -o -name "*.fbs" | head -20Repository: gridaco/grida
Length of output: 785
🏁 Script executed:
# Read the file to check the lines mentioned
wc -l ./format/grida.fbsRepository: gridaco/grida
Length of output: 80
🏁 Script executed:
# Read the specific sections mentioned in the review
sed -n '50,65p' ./format/grida.fbs
echo "---"
sed -n '525,540p' ./format/grida.fbsRepository: gridaco/grida
Length of output: 1009
🏁 Script executed:
# Search for flatc version in common package files
find . -maxdepth 3 -type f \( -name "package.json" -o -name "Cargo.toml" -o -name "*.gradle" -o -name "*.lock" -o -name ".nvmrc" -o -name "Dockerfile" \) | head -20Repository: gridaco/grida
Length of output: 630
🏁 Script executed:
# Search for flatc references in the repository
rg "flatc" --type-list | head -10
rg "flatc" -iRepository: gridaco/grida
Length of output: 7170
🏁 Script executed:
# Check if there are other structs with defaults in the file
grep -n "^struct\|= " ./format/grida.fbs | head -40Repository: gridaco/grida
Length of output: 910
🏁 Script executed:
# Search for struct definitions with their field defaults more specifically
sed -n '170,250p' ./format/grida.fbsRepository: gridaco/grida
Length of output: 1611
🏁 Script executed:
# Check for all struct definitions with "=" signs (which would indicate defaults)
rg "^struct " ./format/grida.fbs -A 5 | grep -E "struct|="Repository: gridaco/grida
Length of output: 587
🏁 Script executed:
# Look at the exact struct definition again and check if it's in any generated files
sed -n '525,540p' ./format/grida.fbs
# Also search for where FontOpticalSizing is used
rg "FontOpticalSizing" --type ts --type rsRepository: gridaco/grida
Length of output: 462
🏁 Script executed:
# Search for FontOpticalSizing without type filters
rg "FontOpticalSizing"
# Also check if the schema compiles by looking at generated TypeScript
find . -path ./node_modules -prune -o -type f -name "*.ts" -print | xargs grep -l "FontOpticalSizing" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 6785
🏁 Script executed:
# Check if the generated TypeScript files exist and were successfully generated
find ./packages/grida-format -name "*.ts" -o -name "*.js" | head -20Repository: gridaco/grida
Length of output: 186
🏁 Script executed:
# Check the generated TypeScript to see if FontOpticalSizing is there
cat ./packages/grida-format/src/index.ts | grep -A 20 "FontOpticalSizing"Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check if the generated file exists and has content
wc -l ./packages/grida-format/src/index.ts
head -50 ./packages/grida-format/src/index.tsRepository: gridaco/grida
Length of output: 529
🏁 Script executed:
# Check if the generated grida folder exists
ls -la ./packages/grida-format/src/ | head -20
find ./packages/grida-format/src -name "*.ts" -type f | head -20Repository: gridaco/grida
Length of output: 369
🏁 Script executed:
# Check if generated files are in .gitignore
cat ./packages/grida-format/.gitignoreRepository: gridaco/grida
Length of output: 124
🏁 Script executed:
# Check if there's a test or CI that validates the schema compilation
cat ./format/README.mdRepository: gridaco/grida
Length of output: 1651
Remove invalid default value from FontOpticalSizing struct — violates documented schema rule.
The struct has kind:FontOpticalSizingKind = Auto; but FlatBuffers structs cannot have defaults. Your own rule (lines 57–58) explicitly documents this as a REQUIRED RULE: "cannot have defaults". Additionally, all other 17+ structs in this file follow this rule; FontOpticalSizing is the only exception.
Proposed fix
struct FontOpticalSizing {
- kind:FontOpticalSizingKind = Auto;
+ kind:FontOpticalSizingKind;
value:float;
}📝 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.
| struct FontOpticalSizing { | |
| kind:FontOpticalSizingKind = Auto; | |
| value:float; | |
| } | |
| struct FontOpticalSizing { | |
| kind:FontOpticalSizingKind; | |
| value:float; | |
| } |
🤖 Prompt for AI Agents
In @format/grida.fbs around lines 532 - 536, The FontOpticalSizing struct is the
only struct with a default on a field which violates the schema rule that
structs cannot have defaults; remove the default assignment from the kind field
in struct FontOpticalSizing (i.e., delete the "= Auto" default on
kind:FontOpticalSizingKind) so the struct matches the other structs in the file
and complies with the documented rule.
| /// Parent reference (optional; root nodes omit this). | ||
| /// | ||
| /// When present, this node is a child of the referenced parent. | ||
| /// Children are ordered by `position` (lexicographic sort). | ||
| /// | ||
| /// Reference: Aligned with Figma's `ParentIndex` pattern (fig.kiwi). | ||
| /// This replaces the previous `children:[NodeIdentifier]` array model. | ||
| /// | ||
| /// To compute children of a parent node: | ||
| /// 1. Filter all nodes where `parent.parent_id == parent.id` | ||
| /// 2. Sort by `parent.position` (lexicographic comparison) | ||
| parent:ParentReference (required); | ||
|
|
There was a problem hiding this comment.
LayerTrait.parent is marked (required) but comments describe optional/root omission.
Either (a) make it truly optional (remove required) and ensure codecs tolerate null, or (b) keep it required and fix the comments (and any upstream assumptions) to avoid drift.
🤖 Prompt for AI Agents
In @format/grida.fbs around lines 1238 - 1250, The field LayerTrait.parent is
declared as required but the comment says it is optional for root nodes; fix by
either making the schema match the comment or vice versa: (a) prefer making it
optional — remove “(required)” from the ParentReference declaration (i.e.,
change parent:ParentReference (required); to parent:ParentReference;), update
any serialization/deserialization code to accept null/missing parent values and
ensure codecs handling ParentReference tolerate absence, or (b) keep it required
— update the comment block to remove the “optional/root omission” language and
audit any upstream assumptions that treat parent as nullable; reference the
ParentReference type and LayerTrait.parent field when applying the change.
| /// Rust: `LayerMaskType` (union; default is Image(alpha) since it's the first union member) | ||
| mask_type:LayerMaskType; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -E "grida\.fbs" | head -5Repository: gridaco/grida
Length of output: 73
🏁 Script executed:
# Read the context around lines 1259-1261 in format/grida.fbs
wc -l format/grida.fbs
head -1300 format/grida.fbs | tail -100 | cat -nRepository: gridaco/grida
Length of output: 4101
🏁 Script executed:
# Find LayerMaskType union definition
rg -n "LayerMaskType" format/grida.fbs | head -20Repository: gridaco/grida
Length of output: 464
🏁 Script executed:
# Look for the union definition
rg -B3 -A10 "union LayerMaskType" format/grida.fbsRepository: gridaco/grida
Length of output: 466
🏁 Script executed:
# Get the full context around lines 1259-1261
sed -n '1250,1270p' format/grida.fbs | cat -nRepository: gridaco/grida
Length of output: 728
🏁 Script executed:
# Search for mask_type usage in codec/implementation files
rg -n "mask_type" --type-list | head -5Repository: gridaco/grida
Length of output: 446
🏁 Script executed:
# Check for any default handling or initialization of mask_type
rg -B5 -A5 "mask_type" . --type rust | head -100Repository: gridaco/grida
Length of output: 7127
🏁 Script executed:
# Search for FlatBuffers-related code that might handle union defaults
find . -name "*.rs" -type f | xargs grep -l "LayerMaskType" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 355
🏁 Script executed:
sed -n '1240,1280p' format/grida.fbs | cat -nRepository: gridaco/grida
Length of output: 1712
🏁 Script executed:
# Get full context - find what table contains mask_type
rg -B20 "mask_type:LayerMaskType" format/grida.fbs | head -40Repository: gridaco/grida
Length of output: 843
🏁 Script executed:
# Find the LayerMaskType union definition and see all its members
rg -B5 -A15 "^union LayerMaskType" format/grida.fbsRepository: gridaco/grida
Length of output: 568
🏁 Script executed:
# Check if there are any tests or documentation about union defaults in the codebase
find . -name "*.md" -type f 2>/dev/null | xargs grep -l "union" 2>/dev/null | head -5Repository: gridaco/grida
Length of output: 260
🏁 Script executed:
# Check the generated code that uses LayerMaskType
find . -path "*/target" -prune -o -name "*.rs" -type f -print 2>/dev/null | xargs grep -l "fn mask_type" 2>/dev/nullRepository: gridaco/grida
Length of output: 41
🏁 Script executed:
# Find generated FlatBuffers files
find . -name "*_generated.rs" -o -name "*grida*" -path "*generated*" | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Search for generated code from schema
find . -name "*.rs" -path "*/generated*" -o -name "*grida*.rs" | grep -v target | head -10Repository: gridaco/grida
Length of output: 510
🏁 Script executed:
cat -n format/grida.fbs | sed -n '1250,1270p'Repository: gridaco/grida
Length of output: 728
🏁 Script executed:
rg -n "union LayerMaskType" format/grida.fbs -A20Repository: gridaco/grida
Length of output: 652
🏁 Script executed:
# Find the table that contains mask_type
rg -n "mask_type" format/grida.fbsRepository: gridaco/grida
Length of output: 141
🏁 Script executed:
sed -n '1259,1261p' format/grida.fbsRepository: gridaco/grida
Length of output: 179
🏁 Script executed:
sed -n '1240,1280p' format/grida.fbsRepository: gridaco/grida
Length of output: 1425
🏁 Script executed:
rg -n "union LayerMaskType\|table LayerMask" format/grida.fbsRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
rg -n "^union |^table " format/grida.fbs | tail -50Repository: gridaco/grida
Length of output: 1609
🏁 Script executed:
sed -n '1200,1300p' format/grida.fbsRepository: gridaco/grida
Length of output: 3460
🏁 Script executed:
find . -type f \( -name "*.rs" -o -name "*.py" -o -name "*.go" \) 2>/dev/null | xargs grep -B5 -A5 "mask_type" 2>/dev/null | head -60Repository: gridaco/grida
Length of output: 3873
🏁 Script executed:
find . -type f -name "*.rs" 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 1013
🏁 Script executed:
# Look for Codegen or builder patterns
find . -type f -name "*.rs" 2>/dev/null | xargs grep -l "LayerMask\|mask_type" 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 398
🏁 Script executed:
find . -type d -name "generated" -o -name "flatbuffers" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
find . -name "build.rs" -o -name "build.py" -o -name "CMakeLists.txt" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
find . -type f -name "*.py" 2>/dev/null | xargs grep -l "mask_type\|LayerMask" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
sed -n '1150,1350p' format/grida.fbs | cat -nRepository: gridaco/grida
Length of output: 7465
🏁 Script executed:
# Look for any documentation or comments about union handling
rg -B10 -A10 "union" format/grida.fbs | head -100Repository: gridaco/grida
Length of output: 4530
🏁 Script executed:
find . -name "*.toml" -o -name "*.txt" -o -name "*.lock" | xargs grep -i "flatbuf" 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
rg "mask_type" . --type rust --type python --type typescript 2>/dev/null | head -40Repository: gridaco/grida
Length of output: 143
🏁 Script executed:
rg -n "table LayerMask|union LayerMaskType" format/grida.fbs -A30Repository: gridaco/grida
Length of output: 1056
🏁 Script executed:
git log --oneline format/grida.fbs | head -20Repository: gridaco/grida
Length of output: 155
🏁 Script executed:
git diff HEAD format/grida.fbs 2>/dev/null | head -100Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
git diff --cached format/grida.fbs 2>/dev/null | head -100Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
find . -name "*test*.rs" -o -name "*test*.py" 2>/dev/null | xargs grep -l "mask_type\|LayerMask" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
ls -la | head -20Repository: gridaco/grida
Length of output: 1246
🏁 Script executed:
find . -maxdepth 3 -type f -name "*.fbs" 2>/dev/nullRepository: gridaco/grida
Length of output: 75
🏁 Script executed:
head -1261 format/grida.fbs | tail -20 | cat -nRepository: gridaco/grida
Length of output: 854
🏁 Script executed:
grep -n "union LayerMaskType" format/grida.fbsRepository: gridaco/grida
Length of output: 82
🏁 Script executed:
sed -n '1214,1240p' format/grida.fbs | cat -nRepository: gridaco/grida
Length of output: 1091
🏁 Script executed:
sed -n '1210,1265p' format/grida.fbsRepository: gridaco/grida
Length of output: 1836
🏁 Script executed:
rg -r "mask_type" . --max-count 50 2>/dev/nullRepository: gridaco/grida
Length of output: 50372
🌐 Web query:
FlatBuffers "union" type handling when field is absent decoded value NONE null
💡 Result:
Short answer: When a union field is absent FlatBuffers encodes its type as NONE (enum value 0) and the value/offset is absent (treated as null/0). Check the generated "_type" enum for NONE (or use the language accessor that returns null/None) before trying to read the union value. For union vectors, a NONE element uses type NONE and the corresponding value offset must be 0. [1][2][3]
References:
- FlatBuffers schema & unions (NONE denotes no value; check the _type field / accessors). [1]
- Internals: unions encoded as (enum type, value offset); NONE = 0 means not set. [2]
- flatcc/format note: if type is NONE the value field must be absent; union-vector NONE uses value offset 0. [3]
🏁 Script executed:
sed -n '1258,1262p' format/grida.fbsRepository: gridaco/grida
Length of output: 201
🏁 Script executed:
git log -p --all -S "mask_type" -- format/grida.fbs 2>/dev/null | head -100Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
find . -type f \( -name "*.rs" -o -name "*.py" -o -name "*.cpp" \) 2>/dev/null | xargs grep -l "LayerMask\|mask_type" 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 398
🏁 Script executed:
find . -type f \( -name "*.kt" -o -name "*.java" -o -name "*.ts" -o -name "*.js" \) 2>/dev/null | xargs grep -l "mask_type" 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 133
🏁 Script executed:
find . -type f -name "*.dart" 2>/dev/null | xargs grep -l "mask_type" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
ls -la format/Repository: gridaco/grida
Length of output: 294
🏁 Script executed:
find . -type f -name "*grida*" 2>/dev/null | grep -v "\.git\|node_modules\|target" | head -20Repository: gridaco/grida
Length of output: 1022
🏁 Script executed:
find . -name "README*" -o -name "*.md" 2>/dev/null | xargs grep -i "union\|mask" 2>/dev/null | head -30Repository: gridaco/grida
Length of output: 2855
🏁 Script executed:
sed -n '1255,1265p' format/grida.fbs | cat -nRepository: gridaco/grida
Length of output: 442
🏁 Script executed:
rg -B30 "mask_type:LayerMaskType" format/grida.fbs | head -50Repository: gridaco/grida
Length of output: 1160
🏁 Script executed:
sed -n '1256,1262p' format/grida.fbs | cat -nRepository: gridaco/grida
Length of output: 326
🏁 Script executed:
sed -n '1240,1270p' format/grida.fbs | cat -nRepository: gridaco/grida
Length of output: 1293
🌐 Web query:
FlatBuffers union field type and value fields default behavior when absent first member
💡 Result:
Short answer: FlatBuffers reserves an implicit NONE = 0 for unions. When the union type field is absent (or equals NONE) the value/offset field is not present (or is 0 for union vectors). The type field therefore defaults to NONE (0); no union value is stored. Sources: FlatBuffers docs (Unions/Internals) and flatcc notes. [1][2].
Sources
[1] https://flatbuffers.dev/schema/
[2] https://gemfury.com/arrow-nightlies/python%3Ananoarrow/-/content/subprojects/flatcc-fd3c4ae5cd39f0651eda6a3a1a374278070135d6/doc/binary-format.md
🏁 Script executed:
find . -type f -name "*.ts" -o -name "*.js" 2>/dev/null | xargs grep -l "LayerMask\|mask_type" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 199
🌐 Web query:
FlatBuffers "union" absent field behavior default first member Image alpha
💡 Result:
Short answer: a FlatBuffers union does NOT default to its first member. If a union field is absent its hidden type is NONE (0) and the value is null/absent — you must explicitly set the union_type to a non-NONE member and provide the value to get a union value. [1][2]
Relevant docs:
- Schema docs: unions include a NONE marker (encoded 0) meaning “not set”; union fields are implemented as a pair (type enum + value offset). [1]
- Internals: FlatBuffers reserves enum constant NONE (0) for “not set”; when type is NONE the value field must be absent (and the type field may be absent because it defaults to NONE). [2]
If you were worried about defaulting to the union’s first declared member (e.g. Image) or inheriting that member’s default alpha value — that will not happen automatically; you must serialize the union_type and the union value explicitly. [1][2]
Sources:
[1] FlatBuffers Schema / Overview.
[2] FlatBuffers Internals (Unions).
🏁 Script executed:
grep -n "^table LayerMask" format/grida.fbsRepository: gridaco/grida
Length of output: 122
🏁 Script executed:
sed -n '1231,1265p' format/grida.fbs | cat -nRepository: gridaco/grida
Length of output: 1458
🌐 Web query:
FlatBuffers union type field absent null NONE semantics specification
💡 Result:
Short answer
- A FlatBuffers union field is encoded as two fields in the table: a generated enum (the union _type) and the offset to the union value. The enum value NONE (numeric 0) is reserved to mean “no value” / union not set. [1][2]
- If the union_type is NONE (0) there is no associated object stored; callers must check the union_type before casting/reading the union value. [2][3]
- Practically: treat the union_type==NONE as the null/absent case. When serializing from JSON or from code, either omit the union (it will be NONE) or explicitly set the type to NONE to indicate absence. [1][3]
Sources
- FlatBuffers schema docs (Unions).
- FlatBuffers internals (union encoding, NONE == 0).
- FlatBuffers tutorial (how to access and check union_type before reading).
🏁 Script executed:
rg -A8 "union LayerMaskType" format/grida.fbsRepository: gridaco/grida
Length of output: 353
🏁 Script executed:
find . -type f -name "*.py" 2>/dev/null | head -10 | xargs head -1Repository: gridaco/grida
Length of output: 550
🏁 Script executed:
find . -type f -name "*codec*.py" -o -name "*serializ*.py" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
find . -maxdepth 2 -type d 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 342
🏁 Script executed:
find . -type f -name "*.py" 2>/dev/null | xargs grep -l "mask_type\|LayerMask" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
find . -type f \( -name "*.py" -o -name "*.rs" \) 2>/dev/null | xargs grep -l "encode\|decode" 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 578
🏁 Script executed:
ls -la py*/Repository: gridaco/grida
Length of output: 108
🏁 Script executed:
find . -maxdepth 3 -type f -name "*.py" 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 110
🏁 Script executed:
git ls-files | cut -d. -f2 | sort -u | head -30Repository: gridaco/grida
Length of output: 357
🏁 Script executed:
find . -name "*.proto" -o -name "*.json" -o -name "*.yaml" 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 1189
🏁 Script executed:
git log --all --oneline -- format/grida.fbs | head -10Repository: gridaco/grida
Length of output: 155
🏁 Script executed:
git show HEAD:format/grida.fbs | sed -n '1256,1262p'Repository: gridaco/grida
Length of output: 277
Remove or correct the misleading comment — FlatBuffers unions default to NONE/null, not to the first explicit member.
The comment claims "default is Image(alpha) since it's the first union member," which is incorrect:
- FlatBuffers reserves NONE (type 0) as the implicit default for all unions; when
mask_typeis absent, it decodes as NONE/null, not as Image(alpha). - LayerMaskTypeImage (index 1) is not the first member—NONE is implicitly first.
- To ensure "Image(alpha)" semantics when the field is not explicitly set, the codec must encode that value during serialization.
Either remove the comment or rewrite it to accurately describe the union's behavior and how it will be deserialized when absent.
🤖 Prompt for AI Agents
In @format/grida.fbs around lines 1259 - 1261, The comment on the union field
mask_type:LayerMaskType is misleading; FlatBuffers unions implicitly default to
NONE (type 0) when absent, not the first explicit member, so remove or replace
the line that claims "default is Image(alpha) since it's the first union member"
with a concise accurate note stating that unions default to NONE/null and that
if you need Image(alpha) semantics when the field is omitted you must explicitly
encode LayerMaskTypeImage during serialization (or handle the absent case in
deserialization); ensure references to LayerMaskType, mask_type, and
LayerMaskTypeImage are updated accordingly.
| describe("comprehensive integration", () => { | ||
| it("roundtrips complex document with multiple node types and properties", () => { | ||
| const sceneId = "0-1"; | ||
| const rectId = "0-2"; | ||
| const textId = "0-3"; | ||
| const containerId = "0-4"; | ||
| const groupId = "0-5"; | ||
|
|
||
| const doc = createDocument(sceneId, { | ||
| [rectId]: { | ||
| ...baseRectangle(rectId), | ||
| layout_inset_left: 10, | ||
| layout_inset_top: 20, | ||
| layout_target_width: 100, | ||
| layout_target_height: 200, | ||
| rotation: 30, | ||
| }, | ||
| [textId]: { | ||
| ...baseTextSpan(textId), | ||
| layout_positioning: "absolute", | ||
| layout_inset_right: 12, | ||
| layout_inset_bottom: 34, | ||
| layout_target_width: "auto", | ||
| layout_target_height: "auto", | ||
| rotation: 5, | ||
| text: null, | ||
| }, | ||
| [containerId]: { | ||
| ...baseContainer(containerId), | ||
| layout_inset_left: 1, | ||
| layout_inset_top: 2, | ||
| layout_target_width: { type: "percentage" as const, value: 50 }, | ||
| layout_target_height: 100, | ||
| layout_mode: "flex", | ||
| layout_direction: "vertical", | ||
| layout_wrap: "nowrap", | ||
| layout_main_axis_alignment: "space-between", | ||
| layout_cross_axis_alignment: "center", | ||
| layout_main_axis_gap: 11, | ||
| layout_cross_axis_gap: 22, | ||
| layout_padding_top: 3, | ||
| layout_padding_right: 4, | ||
| layout_padding_bottom: 5, | ||
| layout_padding_left: 6, | ||
| }, | ||
| [groupId]: { | ||
| ...baseGroup(groupId), | ||
| opacity: 0.9, | ||
| layout_positioning: "relative", | ||
| layout_inset_left: 5, | ||
| layout_inset_top: 10, | ||
| }, | ||
| }); | ||
|
|
||
| const bytes = format.document.encode.toFlatbuffer(doc); | ||
| const decoded = format.document.decode.fromFlatbuffer(bytes); | ||
|
|
||
| expect(decoded.nodes[rectId]?.type).toBe("rectangle"); | ||
| expect(decoded.nodes[textId]?.type).toBe("tspan"); | ||
| expect(decoded.nodes[containerId]?.type).toBe("container"); | ||
| expect(decoded.nodes[groupId]?.type).toBe("group"); | ||
| expect(decoded.links[sceneId]).toEqual([ | ||
| rectId, | ||
| textId, | ||
| containerId, | ||
| groupId, | ||
| ]); | ||
| expect(decoded.scenes_ref).toEqual([sceneId]); | ||
| expect(decoded.entry_scene_id).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Likely failing assertion: decoded.entry_scene_id expected undefined but input sets it.
createDocument(sceneId, ...) sets entry_scene_id to sceneId by default (Line 236-246), yet the integration test expects decoded.entry_scene_id to be undefined (Line 1072). If roundtrip is meant to be 1:1, this should probably be sceneId. If the format intentionally omits default entry_scene_id, then createDocument() should likely not set it by default (or the test name should mention normalization).
Proposed fix (if you expect preservation)
- expect(decoded.entry_scene_id).toBeUndefined();
+ expect(decoded.entry_scene_id).toBe(sceneId);📝 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.
| describe("comprehensive integration", () => { | |
| it("roundtrips complex document with multiple node types and properties", () => { | |
| const sceneId = "0-1"; | |
| const rectId = "0-2"; | |
| const textId = "0-3"; | |
| const containerId = "0-4"; | |
| const groupId = "0-5"; | |
| const doc = createDocument(sceneId, { | |
| [rectId]: { | |
| ...baseRectangle(rectId), | |
| layout_inset_left: 10, | |
| layout_inset_top: 20, | |
| layout_target_width: 100, | |
| layout_target_height: 200, | |
| rotation: 30, | |
| }, | |
| [textId]: { | |
| ...baseTextSpan(textId), | |
| layout_positioning: "absolute", | |
| layout_inset_right: 12, | |
| layout_inset_bottom: 34, | |
| layout_target_width: "auto", | |
| layout_target_height: "auto", | |
| rotation: 5, | |
| text: null, | |
| }, | |
| [containerId]: { | |
| ...baseContainer(containerId), | |
| layout_inset_left: 1, | |
| layout_inset_top: 2, | |
| layout_target_width: { type: "percentage" as const, value: 50 }, | |
| layout_target_height: 100, | |
| layout_mode: "flex", | |
| layout_direction: "vertical", | |
| layout_wrap: "nowrap", | |
| layout_main_axis_alignment: "space-between", | |
| layout_cross_axis_alignment: "center", | |
| layout_main_axis_gap: 11, | |
| layout_cross_axis_gap: 22, | |
| layout_padding_top: 3, | |
| layout_padding_right: 4, | |
| layout_padding_bottom: 5, | |
| layout_padding_left: 6, | |
| }, | |
| [groupId]: { | |
| ...baseGroup(groupId), | |
| opacity: 0.9, | |
| layout_positioning: "relative", | |
| layout_inset_left: 5, | |
| layout_inset_top: 10, | |
| }, | |
| }); | |
| const bytes = format.document.encode.toFlatbuffer(doc); | |
| const decoded = format.document.decode.fromFlatbuffer(bytes); | |
| expect(decoded.nodes[rectId]?.type).toBe("rectangle"); | |
| expect(decoded.nodes[textId]?.type).toBe("tspan"); | |
| expect(decoded.nodes[containerId]?.type).toBe("container"); | |
| expect(decoded.nodes[groupId]?.type).toBe("group"); | |
| expect(decoded.links[sceneId]).toEqual([ | |
| rectId, | |
| textId, | |
| containerId, | |
| groupId, | |
| ]); | |
| expect(decoded.scenes_ref).toEqual([sceneId]); | |
| expect(decoded.entry_scene_id).toBeUndefined(); | |
| }); | |
| }); | |
| describe("comprehensive integration", () => { | |
| it("roundtrips complex document with multiple node types and properties", () => { | |
| const sceneId = "0-1"; | |
| const rectId = "0-2"; | |
| const textId = "0-3"; | |
| const containerId = "0-4"; | |
| const groupId = "0-5"; | |
| const doc = createDocument(sceneId, { | |
| [rectId]: { | |
| ...baseRectangle(rectId), | |
| layout_inset_left: 10, | |
| layout_inset_top: 20, | |
| layout_target_width: 100, | |
| layout_target_height: 200, | |
| rotation: 30, | |
| }, | |
| [textId]: { | |
| ...baseTextSpan(textId), | |
| layout_positioning: "absolute", | |
| layout_inset_right: 12, | |
| layout_inset_bottom: 34, | |
| layout_target_width: "auto", | |
| layout_target_height: "auto", | |
| rotation: 5, | |
| text: null, | |
| }, | |
| [containerId]: { | |
| ...baseContainer(containerId), | |
| layout_inset_left: 1, | |
| layout_inset_top: 2, | |
| layout_target_width: { type: "percentage" as const, value: 50 }, | |
| layout_target_height: 100, | |
| layout_mode: "flex", | |
| layout_direction: "vertical", | |
| layout_wrap: "nowrap", | |
| layout_main_axis_alignment: "space-between", | |
| layout_cross_axis_alignment: "center", | |
| layout_main_axis_gap: 11, | |
| layout_cross_axis_gap: 22, | |
| layout_padding_top: 3, | |
| layout_padding_right: 4, | |
| layout_padding_bottom: 5, | |
| layout_padding_left: 6, | |
| }, | |
| [groupId]: { | |
| ...baseGroup(groupId), | |
| opacity: 0.9, | |
| layout_positioning: "relative", | |
| layout_inset_left: 5, | |
| layout_inset_top: 10, | |
| }, | |
| }); | |
| const bytes = format.document.encode.toFlatbuffer(doc); | |
| const decoded = format.document.decode.fromFlatbuffer(bytes); | |
| expect(decoded.nodes[rectId]?.type).toBe("rectangle"); | |
| expect(decoded.nodes[textId]?.type).toBe("tspan"); | |
| expect(decoded.nodes[containerId]?.type).toBe("container"); | |
| expect(decoded.nodes[groupId]?.type).toBe("group"); | |
| expect(decoded.links[sceneId]).toEqual([ | |
| rectId, | |
| textId, | |
| containerId, | |
| groupId, | |
| ]); | |
| expect(decoded.scenes_ref).toEqual([sceneId]); | |
| expect(decoded.entry_scene_id).toBe(sceneId); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In @packages/grida-canvas-io/__tests__/format-roundtrip.test.ts around lines
1004 - 1074, The failing assertion is because createDocument(sceneId, ...) sets
entry_scene_id to sceneId by default; update the test in "comprehensive
integration" to assert that decoded.entry_scene_id equals sceneId (use
expect(decoded.entry_scene_id).toBe(sceneId)) instead of expecting undefined, or
alternatively remove the default-setting in createDocument if you intend
entry_scene_id to be omitted—but most straightforward is to change the assertion
to expect sceneId to match the roundtrip behavior.
…agePaintFit handling
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @format/grida.fbs:
- Around line 532-535: FontOpticalSizing is a struct with a discriminant field
kind but should be converted to a union to match the schema's variant pattern
and avoid meaningless value fields; change the declaration of FontOpticalSizing
from a struct to a union with explicit variants (e.g., Auto, None, Fixed) and
move the float value into the Fixed variant only so Auto/None carry no value and
Fixed carries the float, enabling per-variant evolution and aligning with other
unions like LayoutPositioningBasis/Paint.
- Around line 242-250: Update the doc comment for the RectangularStrokeWidth
struct to explicitly state that the all-zeros value is the correct semantic
default and is equivalent to no stroke; replace the conditional phrasing ("can
be interpreted as 'not used'") with a definitive sentence such as: "A stroke
width of zero on all sides is always equivalent to no stroke and is the correct
semantic default when the field is absent." Reference RectangularStrokeWidth in
the comment to ensure the struct follows the schema's Default Semantics
Contract.
In @packages/grida-canvas-io/__tests__/format-roundtrip.test.ts:
- Around line 847-905: Test skipped because ImageNode fit decoding is missing:
update the decoder to properly handle the ImagePaintFit union (variants Fit,
Transform, Tile) instead of defaulting to "cover", or document the limitation
and add a tracking issue. Locate the decode path used by
format.document.decode.fromFlatbuffer and the Image node decoding code (look for
functions handling ImageNodeProperties or ImagePaint/ImagePaintFit), implement
branch handling for the ImagePaintFit::Fit variant to map the FlatBuffers Fit
enum to the runtime cg.BoxFit values (also handle Transform/Tile cases or map
them to appropriate runtime representations), and ensure tests in
format-roundtrip.test.ts that assert node.fit (the ImageNode.fit field) pass;
alternatively, add a clear TODO comment and a linked issue in the decoder where
ImagePaintFit is currently ignored.
🧹 Nitpick comments (1)
format/grida.fbs (1)
281-290: Reconsider including deprecated elements in the new schema version.The schema includes
BasicShapeNodeTypeenum (line 282) and thetypefield inBasicShapeNode(line 1347) with "TODO: remove this" comments. Both are marked as temporary to preserve "node type semantics" during transition.However, since this is a breaking schema migration to v0.90, consider removing these deprecated elements now rather than carrying them forward:
- The
shape: CanonicalLayerShapeunion already encodes the shape type via the discriminant- Keeping redundant fields creates maintenance burden and potential inconsistency (what if
typedoesn't match the union variant?)- Codecs must handle the duplication, increasing complexity
If these are retained for a specific compatibility reason (e.g., incremental rollout), document the migration timeline and removal plan explicitly.
Also applies to: 1345-1347
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
format/grida.fbspackages/grida-canvas-io/__tests__/format-roundtrip.test.tspackages/grida-canvas-io/format.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript 5 as the main language for most apps
Use Lucide or Radix Icons for icons
Files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
{editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
packages/grida-canvas-*/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Handle NodeId to UserNodeId conversion via `IdConverter` during .grida file loading
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `NodeId` (u64) for internal structs (NodeRecs, SceneGraph, caches) in the rendering engine for high-performance operations
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to packages/grida-canvas-*/**/*.{ts,tsx,js,jsx} : Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.tsformat/grida.fbs
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/*.rs : Run all tests with: `cargo test`
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.grida : Validate .grida files using the `tool_io_grida` CLI tool to check file structure, parse all nodes, and detect parsing errors
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/scenario_*.rs : Scenario-specific comprehensive tests should be organized in files matching `scenario_*.rs`
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Use Canvas 2D API with path commands for rendering geometric shapes (circle, square, triangle, etc.)
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/ui_parser_test.rs : High-level UI API tests should use `parse_ui` and be organized in `ui_parser_test.rs`
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : For SVG export, convert circles to <circle> elements, rectangles to <rect> elements, and polygons to <polygon> elements with calculated points
Applied to files:
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Handle NodeId to UserNodeId conversion via `IdConverter` during .grida file loading
Applied to files:
format/grida.fbs
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `NodeId` (u64) for internal structs (NodeRecs, SceneGraph, caches) in the rendering engine for high-performance operations
Applied to files:
format/grida.fbs
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Maintain bidirectional mapping between NodeId and UserNodeId at the application layer for API boundary management
Applied to files:
format/grida.fbs
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Auto-generate IDs (ID=0) in `NodeRepository` for factory-created nodes
Applied to files:
format/grida.fbs
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `UserNodeId` (String) for public APIs that accept or return node IDs for stability and serialization
Applied to files:
format/grida.fbs
⏰ 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: Cursor Bugbot
- GitHub Check: test
🔇 Additional comments (2)
format/grida.fbs (1)
1-112: Excellent schema design with strong evolution principles.The FlatBuffers schema demonstrates exceptional design quality:
- Comprehensive documentation (lines 1-112): The inline documentation of FlatBuffers semantics, struct vs table tradeoffs, and evolution principles is outstanding and will be invaluable for future maintainers.
- Union-based extensibility (lines 1467-1481): The node encoding uses a proper union model with per-variant tables, making it easy to add new node types additively.
- Trait composition (lines 1222-1274): SystemNodeTrait and LayerTrait provide clean reusable building blocks.
- Explicit intent modeling: The schema consistently favors explicitness over compactness (e.g., nullable tables, unions) to preserve semantic intent.
The extensive rationale commentary (e.g., lines 1393-1466 explaining why union encoding was chosen over a flat table) demonstrates thoughtful architecture that will age well.
The schema covers a comprehensive feature set (layout, paints, effects, vector networks, text styling) in a coherent, evolution-friendly way.
Also applies to: 1467-1503
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts (1)
1-267: Excellent comprehensive roundtrip test coverage.The test suite demonstrates high quality and thoroughness:
Strong test structure:
- Well-designed helper functions (lines 7-267) eliminate duplication while maintaining readability
roundtripTesthelper provides a clean, reusable assertion pattern- Fixtures use
satisfiestype assertions for compile-time safety- Organized with clear
describeblocks grouping related functionalityComprehensive coverage:
- ✅ Positioning modes (cartesian, inset, relative)
- ✅ Length types (auto, px, percentage) with explicit null/unset validation
- ✅ All node types (Scene, Rectangle, TextSpan, Container, Group, Ellipse, Line, Vector, Boolean, Polygon, Star)
- ✅ All paint types (solid, gradients, image) on both fills and strokes
- ✅ Paint ordering preservation (important for rendering correctness)
- ✅ Effects (blur, shadows, filters)
- ✅ Edge cases (explicit zero values, mixed set/unset fields, arc data)
- ✅ Enum roundtrips with data-driven
it.eachtests- ✅ Complex integration scenarios with multiple features
The explicit tests for null/unset value handling (lines 433-496) are particularly valuable given FlatBuffers' scalar default semantics.
Also applies to: 283-2596
| struct FontOpticalSizing { | ||
| kind:FontOpticalSizingKind = Auto; | ||
| value:float; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Refactor FontOpticalSizing from struct to union for better evolution and clarity.
FontOpticalSizing is currently a struct with a discriminant kind field, but this violates the schema's own documented principle (lines 52-75) that structs should not be used for types with variants or semantic modes. When kind is Auto or None, the value field is meaningless but still present with a default of 0, creating ambiguity. If future evolution requires adding fields to specific variants (e.g., additional parameters for Fixed), the struct will be frozen.
Consider refactoring to a union pattern:
Proposed union-based alternative
-enum FontOpticalSizingKind : ubyte {
- Auto = 0,
- None = 1,
- Fixed = 2
-}
-
-struct FontOpticalSizing {
- kind:FontOpticalSizingKind = Auto;
- value:float;
-}
+table FontOpticalSizingAuto {}
+table FontOpticalSizingNone {}
+table FontOpticalSizingFixed {
+ value:float (id: 0);
+}
+
+union FontOpticalSizing {
+ FontOpticalSizingAuto,
+ FontOpticalSizingNone,
+ FontOpticalSizingFixed,
+}This approach:
- Eliminates unused fields for each variant
- Allows independent evolution of each variant
- Makes the semantics explicit (no value field when not needed)
- Aligns with the union-based pattern used elsewhere (e.g.,
LayoutPositioningBasis,Paint)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @format/grida.fbs around lines 532 - 535, FontOpticalSizing is a struct with
a discriminant field kind but should be converted to a union to match the
schema's variant pattern and avoid meaningless value fields; change the
declaration of FontOpticalSizing from a struct to a union with explicit variants
(e.g., Auto, None, Fixed) and move the float value into the Fixed variant only
so Auto/None carry no value and Fixed carries the float, enabling per-variant
evolution and aligning with other unions like LayoutPositioningBasis/Paint.
| describe("cg.BoxFit", () => { | ||
| // TODO: Enable when ImageNodeProperties decoding is implemented | ||
| // Currently fit is hardcoded to "cover" in decode | ||
| it.skip.each([ | ||
| ["contain", "contain"], | ||
| ["cover", "cover"], | ||
| ["fill", "fill"], | ||
| ["none", "none"], | ||
| ] as const)("roundtrips fit: %s", (fit, expected) => { | ||
| const sceneId = "0-1"; | ||
| const nodeId = "0-2"; | ||
|
|
||
| const doc = { | ||
| nodes: { | ||
| [sceneId]: { | ||
| type: "scene", | ||
| id: sceneId, | ||
| name: "Scene", | ||
| active: true, | ||
| locked: false, | ||
| guides: [], | ||
| edges: [], | ||
| constraints: { children: "multiple" }, | ||
| }, | ||
| [nodeId]: { | ||
| type: "image", | ||
| id: nodeId, | ||
| name: "Image", | ||
| active: true, | ||
| locked: false, | ||
| opacity: 1, | ||
| z_index: 0, | ||
| layout_positioning: "absolute", | ||
| layout_inset_left: 0, | ||
| layout_inset_top: 0, | ||
| layout_target_width: 100, | ||
| layout_target_height: 100, | ||
| rotation: 0, | ||
| fit, | ||
| } satisfies grida.program.nodes.ImageNode, | ||
| }, | ||
| links: { [sceneId]: [nodeId] }, | ||
| scenes_ref: [sceneId], | ||
| entry_scene_id: sceneId, | ||
| images: {}, | ||
| bitmaps: {}, | ||
| properties: {}, | ||
| } satisfies grida.program.document.Document; | ||
|
|
||
| const bytes = format.document.encode.toFlatbuffer(doc); | ||
| const decoded = format.document.decode.fromFlatbuffer(bytes); | ||
| const node = decoded.nodes[nodeId]; | ||
| if (!node || node.type !== "image") | ||
| throw new Error("Expected image node"); | ||
| node satisfies grida.program.nodes.ImageNode; | ||
|
|
||
| expect(node.fit).toBe(expected); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Complete ImageNode fit decoding implementation or document the limitation.
The test for cg.BoxFit roundtrip is skipped with a TODO indicating that "ImageNodeProperties decoding is not implemented" and "fit is currently hardcoded to 'cover' in decode."
This indicates incomplete implementation:
- The schema supports
ImagePaintFitas a union withFit,Transform, andTilevariants (format/grida.fbs lines 710-715) - But the decoder apparently doesn't handle this field, defaulting all images to
cover - This will cause incorrect rendering for images that should use
contain,fill, ornonefit modes
Either complete the decoder implementation before merging, or document this as a known limitation with a tracking issue.
Would you like me to verify the decoder implementation to confirm the incomplete fit handling?
#!/bin/bash
# Search for ImagePaintFit decoding logic
rg -n --type=ts -A10 -B5 'ImagePaint.*fit|decode.*ImagePaint' packages/grida-canvas-io/🤖 Prompt for AI Agents
In @packages/grida-canvas-io/__tests__/format-roundtrip.test.ts around lines 847
- 905, Test skipped because ImageNode fit decoding is missing: update the
decoder to properly handle the ImagePaintFit union (variants Fit, Transform,
Tile) instead of defaulting to "cover", or document the limitation and add a
tracking issue. Locate the decode path used by
format.document.decode.fromFlatbuffer and the Image node decoding code (look for
functions handling ImageNodeProperties or ImagePaint/ImagePaintFit), implement
branch handling for the ImagePaintFit::Fit variant to map the FlatBuffers Fit
enum to the runtime cg.BoxFit values (also handle Transform/Tile cases or map
them to appropriate runtime representations), and ensure tests in
format-roundtrip.test.ts that assert node.fit (the ImageNode.fit field) pass;
alternatively, add a clear TODO comment and a linked issue in the decoder where
ImagePaintFit is currently ignored.
…and update generateFileKeyFromSrc to ensure non-empty results
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @editor/grida-canvas/editor.i.ts:
- Line 1615: The assignment of sceneNode.position is using scene.position which
does not exist on the Scene type and will be undefined; update the migration to
either remove the line setting sceneNode.position or conditionally assign it
from the original input (e.g., read position from scene_input before calling
init_scene()), so replace the direct use of scene.position with a guard that
uses scene_input.position if present or omit the assignment entirely; check
symbols scene.position, Scene, init_scene(), scene_input, and sceneNode.position
when making the change.
🧹 Nitpick comments (2)
editor/grida-canvas-hosted/playground/playground.tsx (2)
472-479: Consider using specific error types instead of string matching.The error handling uses string matching (
error.message.includes("not found")) to distinguish file-not-found from other errors. While functional, this is fragile if error messages change.♻️ Consider defining specific error types
If the
io.opfs.Handleimplementation exposes specific error classes (e.g.,NotFoundError), useinstanceofchecks instead:} catch (error) { // File not found or other error - continue to fallback - if (error instanceof Error && error.message.includes("not found")) { + if (error instanceof io.opfs.NotFoundError) { // File doesn't exist yet - this is fine, continue to fallback } else { console.error("Failed to load from OPFS:", error); } }This would make the code more maintainable and less prone to breaking if error messages are updated.
615-639: Consider providing feedback when OPFS is unavailable.When
opfsis null (not supported or failed to initialize), the Cmd/Ctrl+S handler silently does nothing. Consider adding a toast notification to inform users that local persistence is unavailable, helping them understand why their save action had no effect.♻️ Provide user feedback for unavailable OPFS
"meta+s, ctrl+s", async () => { if (opfs) { try { const document = instance.getSnapshot().document; const bytes = io.GRID.encode(document); await opfs.get("document.grida").write(bytes); onSaved(); toast.success("Saved", { position: "bottom-left", }); } catch (error) { console.error("Failed to save to OPFS:", error); toast.error("Failed to save", { position: "bottom-left", }); } + } else { + toast.info("Local persistence unavailable", { + position: "bottom-left", + }); } }, { preventDefault: true, } );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
editor/grida-canvas-hosted/playground/playground.tsxeditor/grida-canvas/editor.i.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript 5 as the main language for most apps
Use Lucide or Radix Icons for icons
Files:
editor/grida-canvas/editor.i.tseditor/grida-canvas-hosted/playground/playground.tsx
{editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Files:
editor/grida-canvas/editor.i.tseditor/grida-canvas-hosted/playground/playground.tsx
editor/grida-*/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to /packages
Files:
editor/grida-canvas/editor.i.tseditor/grida-canvas-hosted/playground/playground.tsx
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use React.js 19 for web applications
Files:
editor/grida-canvas-hosted/playground/playground.tsx
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo fmt` to maintain code formatting standards
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `NodeId` (u64) for internal structs (NodeRecs, SceneGraph, caches) in the rendering engine for high-performance operations
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Handle NodeId to UserNodeId conversion via `IdConverter` during .grida file loading
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Maintain bidirectional mapping between NodeId and UserNodeId at the application layer for API boundary management
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Auto-generate IDs (ID=0) in `NodeRepository` for factory-created nodes
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo clippy --no-deps --all-targets --all-features` for linting to check style, performance, and correctness suggestions
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `skia-safe` crate for painting operations in the rendering engine
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `UserNodeId` (String) for public APIs that accept or return node IDs for stability and serialization
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Applied to files:
editor/grida-canvas/editor.i.tseditor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : When adding new shape types, update the Shape type union, add cases in drawShape() function, add cases in shapeToSVG() function, and add SelectItem in UI
Applied to files:
editor/grida-canvas/editor.i.ts
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to packages/grida-canvas-*/**/*.{ts,tsx,js,jsx} : Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Applied to files:
editor/grida-canvas/editor.i.tseditor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Use Canvas 2D API with path commands for rendering geometric shapes (circle, square, triangle, etc.)
Applied to files:
editor/grida-canvas/editor.i.ts
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to {editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}} : Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.grida : Validate .grida files using the `tool_io_grida` CLI tool to check file structure, parse all nodes, and detect parsing errors
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to editor/grida-*/**/*.{ts,tsx} : Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to <root>/packages
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:22:36.510Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-dev/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:36.510Z
Learning: Remote scenes are fetched with `reqwest`; stick to local `.grida` files when offline
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Handle NodeId to UserNodeId conversion via `IdConverter` during .grida file loading
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/*.{js,ts,jsx,tsx} : Use Web Workers for heavy graphics operations to improve performance and responsiveness
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Custom images used as halftone shapes should be loaded as HTMLImageElement for efficient canvas rendering and preserve original image colors in as-is mode
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Use React hooks for state management (imageSrc, shape, grid, maxRadius, gamma, jitter, opacity, color, customShapeImage, imageDataRef, sizeRef)
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to editor/lib/**/*.{ts,tsx} : Use /editor/lib for core, strictly designed modules with non-opinionated, reusable, and stable implementations
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsx
🧬 Code graph analysis (2)
editor/grida-canvas/editor.i.ts (2)
editor/grida-canvas-hosted/playground/widgets/index.ts (1)
text(89-103)editor/grida-canvas/editor.ts (1)
NodeProxy(5600-5761)
editor/grida-canvas-hosted/playground/playground.tsx (1)
packages/grida-canvas-io/index.ts (1)
Handle(1180-1321)
⏰ 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: Cursor Bugbot
- GitHub Check: test
🔇 Additional comments (17)
editor/grida-canvas/editor.i.ts (7)
71-83: LGTM! FNV-1a hash implementation is correct.The FNV-1a 32-bit hash function implementation follows the standard algorithm with correct constants (offset basis:
0x811c9dc5, prime:0x01000193). The use ofMath.imulfor multiplication,>>> 0for unsigned conversion, andpadStart(8, "0")for formatting ensures a deterministic 8-character hex string output.
161-161: LGTM! Typo correction aligns with schema.Correctly updated type references from
UnknwonNodetoUnknownNode, matching the canonical type definition in the schema.Also applies to: 1307-1307
3127-3127: LGTM! Return type updated to reflect schema migration.The return type change from
TextNodetoTextSpanNodealigns with the schema migration where text nodes are now represented as "tspan" type nodes.
3658-3665: Breaking API changes align with schema migration.
Breaking Change (Line 3660): The parameter
layoutis renamed tolayout_modeto match the schema field rename (layout→layout_mode). This is a breaking API change that will affect all callers.New API (Lines 3662-3665): Added
changeContainerNodeClipsContentmethod to support the newclips_contentproperty mentioned in the PR summary.Since the PR is labeled as
breaking, these changes are expected and intentional.
3676-3681: Breaking API change in gap parameter signature.The
changeFlexContainerNodeGapparameter type has changed:
- New signature: Accepts either
number(shorthand for both gaps) or an object withlayout_main_axis_gapandlayout_cross_axis_gapfields.- Breaking: The field names now include the
layout_prefix (e.g.,main_axis_gap→layout_main_axis_gap), requiring updates to all call sites.This breaking change aligns with the schema migration mentioned in the PR summary and is expected given the
breakinglabel.
3747-3747: LGTM! Type references updated consistently.All text node property change methods (
changeTextNodeLineHeight,changeTextNodeLetterSpacing,changeTextNodeWordSpacing) now referenceTextSpanNodeinstead ofTextNode, maintaining type consistency with the schema migration.Also applies to: 3751-3751, 3755-3755
3842-3842: LGTM! Documentation updated to reflect type changes.All a11y method documentation correctly updated to reference
"tspan"instead of"text", aligning with the schema migration fromTextNodetoTextSpanNode.Also applies to: 3861-3861, 3885-3885, 3907-3907, 3927-3927, 3950-3950
editor/grida-canvas-hosted/playground/playground.tsx (10)
118-119: LGTM!The new imports for OPFS persistence (
io) and unsaved-changes warning hook are correctly placed and align with the new functionality.
154-174: LGTM!The hook correctly handles OPFS availability checks, includes appropriate error handling, and uses defensive programming to ensure the filekey is never empty. The
useMemodependency array is correct.
176-209: LGTM!The dirty-flag tracking implementation is well-designed:
- Correctly avoids subscription overhead when disabled
- Distinguishes between user edits and programmatic resets
- Properly cleans up subscriptions
- Has correct dependency array
301-325: LGTM!The new props are well-documented with clear JSDoc comments that explain:
- How
filekeyis used and auto-generated- Why
warnOnUnsavedChangesis opt-in (demo contexts)- Include helpful examples
336-351: LGTM!The filekey resolution logic follows a sensible fallback chain, and the dirty-flag tracking is conditionally enabled based on
warnOnUnsavedChangesto avoid unnecessary overhead in demo contexts.
363-368: LGTM!The unsaved-changes warning integration correctly respects the
warnOnUnsavedChangesflag and uses the trackeddirtystate. The commented-out dev environment check is intentionally disabled per the inline comment.
517-544: LGTM!The Consumer component props are correctly extended to support per-instance persistence:
onSavedcallback for marking clean state after savefilekeyfor per-instance OPFS directory resolutionType signatures are correct and align with the usage.
554-554: LGTM!Consumer correctly creates its own OPFS handle using the provided
filekeyfor per-instance persistence operations.
121-152: The concern abouteditor.fnv1a32returning an empty or undefined string is unfounded. The implementation (ineditor/grida-canvas/editor.i.ts) uses a standard FNV-1a 32-bit hash and always returns an 8-character hex string via.toString(16).padStart(8, "0"). This ensures a non-empty result even for empty input strings, so the function's documented guarantee of "never returns an empty string" is correctly maintained by the fallback logic.Likely an incorrect or invalid review comment.
409-451: The code correctly handles the dual-format workflow for loading documents. All example.gridafiles served atsrcURLs are JSON files with the expected{ "version": "...", "document": { ... } }structure, and the error handling properly catches both fetch failures and JSON parsing errors. No changes required.
…date references across the project
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
editor/components/formfield/grida-canvas/index.tsx (1)
50-62: Add error handling for fetch and JSON parsing failures.The useEffect hook fetches initial canvas data but lacks error handling. If the fetch fails (e.g., file not found, network error) or JSON parsing fails (malformed data), the promise rejection is unhandled and the editor will silently fail to initialize, leaving users with a broken experience.
🛡️ Suggested error handling implementation
useEffect(() => { fetch("/examples/canvas/blank.grida1") - .then((res) => { - res.json().then((file) => { - instance.commands.reset( - editor.state.init({ - editable: true, - document: file.document, - }), - "template" - ); - }); - }); + .then((res) => { + if (!res.ok) { + throw new Error(`Failed to fetch template: ${res.status}`); + } + return res.json(); + }) + .then((file) => { + instance.commands.reset( + editor.state.init({ + editable: true, + document: file.document, + }), + "template" + ); + }) + .catch((error) => { + console.error("Failed to load canvas template:", error); + // TODO: Show user-friendly error message or fallback UI + }); }, []);crates/grida-canvas-wasm/example/main.js (1)
44-49: Add error handling to the fetch chain.The fetch operation lacks error handling. If
demo.grida1is missing or the fetch fails, the application will fail silently, making debugging difficult for developers using this example.🛡️ Proposed fix with error handling
fetch("./demo.grida1") .then((r) => r.text()) .then((txt) => { grida.loadScene(txt); requestAnimationFrame(render); - }); + }) + .catch((err) => { + console.error("Failed to load demo scene:", err); + });
🤖 Fix all issues with AI agents
In @editor/components/formfield/grida-canvas/index.tsx:
- Line 51: The fetch call starting with fetch("/examples/canvas/blank.grida1")
currently lacks error handling for network failures and JSON parse errors; wrap
the fetch/res.json() sequence in a try/catch (or append .catch()) and handle
failures by logging the error and updating component state (e.g., set an error
or isInitialized flag) so the editor can show a user-facing message or fallback
instead of failing silently; specifically modify the code around the fetch(...)
and res.json() usage in the component (where the initial canvas is loaded) to
catch and handle both fetch and JSON parsing errors.
In @editor/grida-canvas/editor.ts:
- Line 1803: Several places cast values with "node as
grida.program.nodes.UnknownNode" and then access properties without runtime
checks; add a type guard function (e.g., function isUnknownNode(node: any): node
is grida.program.nodes.UnknownNode { return node && typeof node === 'object' &&
'type' in node && /* optional further checks for required props */ true; }) and
use it before casting and property access (replace direct casts in occurrences
of "node as grida.program.nodes.UnknownNode" with an if (isUnknownNode(node)) {
const unknown = node; /* safe access */ } else { /* handle unexpected type /
bail out */ }); apply this pattern to every reported location (the exact cast
sites containing "node as grida.program.nodes.UnknownNode" around the lines
referenced) so access is guarded and runtime errors are avoided.
In @editor/public/examples/canvas/component-01.grida1:
- Around line 85-99: The "description" node's name was incorrectly set to
"tspan"; restore the user-facing identifier by changing the node object whose
"id" is "description" so that "name": "description" while leaving "type":
"tspan" intact— locate the object containing "id": "description" and update its
"name" property back to "description" to match the "title" node pattern.
In @editor/public/examples/canvas/globals-01.grida1:
- Line 35: The node id was renamed to "tspan" but the links section still
references "text", breaking the graph; update the links entry that currently
points to "text" so it references "tspan" (or alternatively rename the node id
back to "text") ensuring the child node id ("tspan") and its parent link target
match exactly; locate the "id": "tspan" declaration and the corresponding link
object that mentions "text" and make them consistent.
In @editor/public/examples/canvas/helloworld.grida1:
- Line 124: The "src" field in the example (the signed S3 URL string) points to
an expired signed URL; replace that value with a non-expiring image source
(e.g., a permanent CDN URL, a relative path to a bundled asset, or a public
S3/HTTP URL) so the example image will load reliably—update the "src" property
in the helloworld.grida1 example to the new permanent URL.
In @packages/grida-canvas-io/__tests__/archive.test.ts:
- Around line 212-228: The time-based assertion in the test "performance: should
pack/unpack mixed real files efficiently" is flaky; remove the Date.now() timing
check or gate it behind an opt-in flag instead. Either (a) delete the
start/end/expect(end - start) assertion and only assert functional behavior
(expect(Object.keys(unpacked.images)).toHaveLength(4)), or (b) wrap the test in
a conditional so it runs only when a PERF_TEST (or similar) env var is set (use
it.skip or process.env check) so the io.archive.pack and io.archive.unpack
functional assertions remain but the timing assertion is executed only in
explicit performance runs. Ensure references to io.archive.pack,
io.archive.unpack, and the test title remain unchanged for discoverability.
- Around line 103-124: The assertion comparing bitmap pixel arrays is brittle
because typed-array constructors may differ; instead of
expect(unpacked.bitmaps.preview.data).toEqual(bitmap.data), normalize both sides
to Uint8Array using toU8() and compare with bytesEqual() (or an equivalent
byte-wise comparison) so the test uses
bytesEqual(toU8(unpacked.bitmaps.preview.data), toU8(bitmap.data)); update the
assertion in the "should pack/unpack archive with bitmaps (png)" test that
references unpacked.bitmaps.preview.data and bitmap.data to use these helpers.
- Around line 7-9: The toU8 helper currently creates a new Uint8Array from
bytes.buffer which loses the original view's byteOffset/byteLength; update toU8
(that handles Uint8Array | Uint8ClampedArray) to return the original Uint8Array
when already that type, otherwise construct a new Uint8Array using the source
buffer with the source view's byteOffset and byteLength so the wrapped view
preserves the exact slice of the underlying ArrayBuffer.
In @packages/grida-canvas-io/index.ts:
- Around line 849-856: In unpack, calling strFromU8(files["snapshot.json"]) can
throw if the entry is missing; first check that files["snapshot.json"] is
defined (and is a Uint8Array or Buffer-like) before calling strFromU8, and if
missing throw a clear Error like "Missing snapshot.json in zip file"; update the
unpack function to perform the existence/type guard on files["snapshot.json"]
prior to calling strFromU8 and then call parse(snapshotJson) to return the
SnapshotDocumentModel.
- Around line 673-680: The ZIP detection in fileformat.detect is too permissive:
it checks head4[2] === 0x03 || head4[2] === 0x05 while is_zip() validates the
standard 0x50 0x4B 0x03 0x04 signature; remove the 0x05 branch in
fileformat.detect (use the same strict check as is_zip) so head4 is compared
against the standard ZIP magic only, keeping the validation consistent between
detect() and is_zip().
🧹 Nitpick comments (8)
packages/grida-canvas-io/__tests__/archive.test.ts (2)
57-66: Avoid writing artifacts unconditionally during tests (CI noise + hermeticity).Creating
__tests__/artifactsand writing ZIPs on every run can bloat CI workspaces and cause confusing diffs locally. Consider gating behind an env var and/or writing toos.tmpdir().Proposed change (env-gated)
const artifactsDir = path.join(__dirname, "artifacts"); - if (!fs.existsSync(artifactsDir)) { - fs.mkdirSync(artifactsDir, { recursive: true }); - } function saveArtifact(name: string, data: Uint8Array): string { + if (!process.env.GRIDA_SAVE_TEST_ARTIFACTS) return ""; + if (!fs.existsSync(artifactsDir)) { + fs.mkdirSync(artifactsDir, { recursive: true }); + } const artifactPath = path.join(artifactsDir, `${name}.zip`); fs.writeFileSync(artifactPath, data); return artifactPath; }
187-203: Add a security-focused filename test (path traversal / absolute paths).You cover spaces/dots/etc, but not
../or absolute paths. A malicious filename could escape the intended zip directory if pack doesn’t sanitize. Add a case like"../evil.png"(and/or"/evil.png"on posix,"C:\\evil.png"on win) and assert it’s rejected or sanitized.crates/grida-canvas-wasm/example/main.js (1)
93-96: Consider using Web Workers for the render loop.The render loop executes
grida.redraw()synchronously on the main thread. For production usage or more complex scenes, this could impact UI responsiveness.As per coding guidelines, heavy graphics operations should be offloaded to Web Workers to maintain a responsive main thread. While acceptable for a basic demo, consider documenting this limitation or providing a Web Worker example for production use cases.
editor/grida-canvas-hosted/playground/playground.tsx (2)
121-152: Well-implemented filesystem-safe key generation.The function properly handles URL parsing failures, sanitizes special characters, and provides hash-based fallbacks. The defensive checks and length limiting ensure filesystem safety.
♻️ Optional: Add explicit validation assertion
Consider adding a final assertion to make the non-empty guarantee explicit:
// If sanitized result is empty, fall back to hash of src - return sanitized || `root-${editor.fnv1a32(src)}`; + const result = sanitized || `root-${editor.fnv1a32(src)}`; + if (!result) { + throw new Error("Failed to generate filekey from src"); + } + return result; } }This makes the invariant explicit, though the current code already guarantees non-empty strings through the fallback logic.
405-501: Robust document loading with proper persistence.The loading logic correctly handles both src-based fetching and OPFS-based restoration, with appropriate cancellation handling and error recovery. The dual write of both
.grida(binary) and.grida1(JSON) formats supports the migration path mentioned in the PR objectives.♻️ Consider adding document format validation
The code loads and decodes documents without validating their structure. Consider adding basic validation to detect corrupted or incompatible formats:
// After loading from fetch or OPFS: const loadedDocument = io.GRID.decode(bytes); // Add validation: if (!loadedDocument || typeof loadedDocument !== 'object') { throw new Error('Invalid document format'); } // Optional: Add schema version check if available if (loadedDocument.version && !isCompatibleVersion(loadedDocument.version)) { console.warn('Document version mismatch, attempting migration...'); } instance.commands.reset( editor.state.init({ editable: true, document: loadedDocument, }), src || "opfs" );This would prevent silent failures from corrupted OPFS data or incompatible document versions.
packages/grida-canvas-io/index.ts (2)
903-968: Consider improving type safety for runtime-only fields.Multiple
as anytype assertions throughout this function bypass TypeScript's type checking:
- Lines 910-911: Accessing
.bitmapsvia(document as any)- Line 916-918: Casting modified document back to
Document- Line 927: Casting document to
anyfor destructuringWhile functional, this pattern reduces type safety. Consider defining explicit types that distinguish between runtime document structure (with
images/bitmaps) and persisted document structure (without these fields).Example approach
type RuntimeDocument = grida.program.document.Document & { images?: Record<string, grida.program.document.ImageRef>; bitmaps?: Record<string, io.Bitmap>; }; type PersistedDocument = Omit<grida.program.document.Document, 'images' | 'bitmaps'>;Then use
RuntimeDocumentfor function parameters andPersistedDocumentfor encoding operations.
1041-1051: Same type safety concern asarchive.pack.Similar to
archive.pack(), this function uses type assertions to strip non-persisted fields (images,bitmaps) and cast back toGridaDocument. Consider using the same explicit typing approach suggested forarchive.pack()to improve type safety across both APIs.editor/grida-canvas/editor.ts (1)
719-719: Type assertion without runtime validation.The cast to
ILayoutTraitassumes the SVG conversion result has the expected layout properties, but there's no runtime validation. Ifiosvg.convertreturns an incompatible type, this could lead to runtime errors.Consider adding a runtime check or type guard:
🔍 Suggested validation
if (result) { - result = result as grida.program.nodes.i.ILayoutTrait; + // Validate result has layout properties + if (!('layout_mode' in result) && !('layout_positioning' in result)) { + throw new Error("SVG conversion result missing layout properties"); + } + result = result as grida.program.nodes.i.ILayoutTrait;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.vscode/settings.jsoncrates/grida-canvas-wasm/example/demo.grida1crates/grida-canvas-wasm/example/main.jscrates/grida-canvas-wasm/example/rectangle.grida1editor/app/(www)/(home)/www-embed/demo-canvas/page.tsxeditor/components/formfield/grida-canvas/index.tsxeditor/grida-canvas-hosted/playground/examples.tseditor/grida-canvas-hosted/playground/playground.tsxeditor/grida-canvas/editor.tseditor/public/examples/canvas/blank.grida1editor/public/examples/canvas/component-01.grida1editor/public/examples/canvas/globals-01.grida1editor/public/examples/canvas/helloworld.grida1editor/public/examples/canvas/hero-main-demo.grida1editor/public/examples/canvas/layout-01.grida1editor/public/examples/canvas/poster-happy-new-year-2026.grida1packages/grida-canvas-io/__tests__/archive.test.tspackages/grida-canvas-io/index.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript 5 as the main language for most apps
Use Lucide or Radix Icons for icons
Files:
editor/grida-canvas-hosted/playground/examples.tseditor/components/formfield/grida-canvas/index.tsxeditor/grida-canvas-hosted/playground/playground.tsxeditor/app/(www)/(home)/www-embed/demo-canvas/page.tsxpackages/grida-canvas-io/__tests__/archive.test.tspackages/grida-canvas-io/index.tseditor/grida-canvas/editor.ts
{editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Files:
editor/grida-canvas-hosted/playground/examples.tseditor/components/formfield/grida-canvas/index.tsxeditor/grida-canvas-hosted/playground/playground.tsxeditor/app/(www)/(home)/www-embed/demo-canvas/page.tsxpackages/grida-canvas-io/__tests__/archive.test.tspackages/grida-canvas-io/index.tseditor/grida-canvas/editor.ts
editor/grida-*/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to /packages
Files:
editor/grida-canvas-hosted/playground/examples.tseditor/grida-canvas-hosted/playground/playground.tsxeditor/grida-canvas/editor.ts
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use React.js 19 for web applications
Files:
editor/components/formfield/grida-canvas/index.tsxeditor/grida-canvas-hosted/playground/playground.tsxeditor/app/(www)/(home)/www-embed/demo-canvas/page.tsx
editor/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use /editor/components for generally reusable components
Files:
editor/components/formfield/grida-canvas/index.tsx
crates/grida-canvas-wasm/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (crates/grida-canvas-wasm/AGENTS.md)
Use Web Workers for heavy graphics operations to improve performance and responsiveness
Files:
crates/grida-canvas-wasm/example/main.js
{next.config.*,**/app/**/*.{ts,tsx},**/pages/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
Use Next.js 15 for web applications
Files:
editor/app/(www)/(home)/www-embed/demo-canvas/page.tsx
editor/app/(www)/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Landing page and SEO-purpose static pages are located in /editor/app/(www) directory; add new webpages here
Files:
editor/app/(www)/(home)/www-embed/demo-canvas/page.tsx
packages/grida-canvas-*/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Files:
packages/grida-canvas-io/__tests__/archive.test.tspackages/grida-canvas-io/index.ts
🧠 Learnings (24)
📓 Common learnings
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo fmt` to maintain code formatting standards
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Handle NodeId to UserNodeId conversion via `IdConverter` during .grida file loading
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `NodeId` (u64) for internal structs (NodeRecs, SceneGraph, caches) in the rendering engine for high-performance operations
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo clippy --no-deps --all-targets --all-features` for linting to check style, performance, and correctness suggestions
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `skia-safe` crate for painting operations in the rendering engine
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Maintain bidirectional mapping between NodeId and UserNodeId at the application layer for API boundary management
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `UserNodeId` (String) for public APIs that accept or return node IDs for stability and serialization
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/Cargo.toml : Include optional `serde = "1.0"` with `derive` feature for JSON serialization support
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to packages/grida-canvas-*/**/*.{ts,tsx,js,jsx} : Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Applied to files:
editor/grida-canvas-hosted/playground/examples.tscrates/grida-canvas-wasm/example/rectangle.grida1editor/components/formfield/grida-canvas/index.tsxeditor/public/examples/canvas/blank.grida1editor/public/examples/canvas/layout-01.grida1editor/public/examples/canvas/helloworld.grida1editor/public/examples/canvas/globals-01.grida1editor/grida-canvas-hosted/playground/playground.tsxcrates/grida-canvas-wasm/example/main.jseditor/app/(www)/(home)/www-embed/demo-canvas/page.tsxpackages/grida-canvas-io/__tests__/archive.test.ts.vscode/settings.jsoneditor/public/examples/canvas/component-01.grida1packages/grida-canvas-io/index.tseditor/grida-canvas/editor.ts
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Applied to files:
editor/grida-canvas-hosted/playground/examples.tscrates/grida-canvas-wasm/example/rectangle.grida1editor/components/formfield/grida-canvas/index.tsxeditor/public/examples/canvas/blank.grida1editor/public/examples/canvas/layout-01.grida1editor/public/examples/canvas/helloworld.grida1editor/public/examples/canvas/globals-01.grida1editor/grida-canvas-hosted/playground/playground.tsxcrates/grida-canvas-wasm/example/main.jseditor/app/(www)/(home)/www-embed/demo-canvas/page.tsxpackages/grida-canvas-io/__tests__/archive.test.tseditor/public/examples/canvas/component-01.grida1packages/grida-canvas-io/index.tseditor/grida-canvas/editor.ts
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to {editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}} : Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Applied to files:
editor/grida-canvas-hosted/playground/examples.tseditor/components/formfield/grida-canvas/index.tsxeditor/public/examples/canvas/globals-01.grida1editor/grida-canvas-hosted/playground/playground.tsxeditor/app/(www)/(home)/www-embed/demo-canvas/page.tsxeditor/public/examples/canvas/component-01.grida1editor/grida-canvas/editor.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Use Canvas 2D API with path commands for rendering geometric shapes (circle, square, triangle, etc.)
Applied to files:
editor/grida-canvas-hosted/playground/examples.tseditor/components/formfield/grida-canvas/index.tsxeditor/app/(www)/(home)/www-embed/demo-canvas/page.tsx
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to editor/grida-*/**/*.{ts,tsx} : Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to <root>/packages
Applied to files:
editor/grida-canvas-hosted/playground/examples.tseditor/components/formfield/grida-canvas/index.tsxeditor/grida-canvas-hosted/playground/playground.tsx.vscode/settings.jsonpackages/grida-canvas-io/index.tseditor/grida-canvas/editor.ts
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/*.{js,ts,jsx,tsx} : Use Web Workers for heavy graphics operations to improve performance and responsiveness
Applied to files:
crates/grida-canvas-wasm/example/rectangle.grida1editor/components/formfield/grida-canvas/index.tsxeditor/grida-canvas-hosted/playground/playground.tsxcrates/grida-canvas-wasm/example/main.js
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo fmt` to maintain code formatting standards
Applied to files:
crates/grida-canvas-wasm/example/rectangle.grida1crates/grida-canvas-wasm/example/main.js
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/+(grida-canvas-wasm.js|grida-canvas-wasm.wasm) : Include WASM artifacts (`grida-canvas-wasm.js` and `grida-canvas-wasm.wasm`) in git for faster CI builds
Applied to files:
crates/grida-canvas-wasm/example/rectangle.grida1crates/grida-canvas-wasm/example/main.js
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo clippy --no-deps --all-targets --all-features` for linting to check style, performance, and correctness suggestions
Applied to files:
crates/grida-canvas-wasm/example/rectangle.grida1
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `skia-safe` crate for painting operations in the rendering engine
Applied to files:
crates/grida-canvas-wasm/example/rectangle.grida1
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Auto-generate IDs (ID=0) in `NodeRepository` for factory-created nodes
Applied to files:
crates/grida-canvas-wasm/example/rectangle.grida1
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Use React hooks for state management (imageSrc, shape, grid, maxRadius, gamma, jitter, opacity, color, customShapeImage, imageDataRef, sizeRef)
Applied to files:
editor/components/formfield/grida-canvas/index.tsxeditor/grida-canvas-hosted/playground/playground.tsxeditor/app/(www)/(home)/www-embed/demo-canvas/page.tsx
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : When adding new parameters to the halftone tool, add state with useState, include in useEffect dependency array, pass to renderHalftone() function, use in rendering logic, and add UI control
Applied to files:
editor/components/formfield/grida-canvas/index.tsx
📚 Learning: 2025-12-01T00:22:36.510Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-dev/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:36.510Z
Learning: Remote scenes are fetched with `reqwest`; stick to local `.grida` files when offline
Applied to files:
editor/components/formfield/grida-canvas/index.tsxeditor/grida-canvas-hosted/playground/playground.tsxcrates/grida-canvas-wasm/example/main.js
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.grida : Validate .grida files using the `tool_io_grida` CLI tool to check file structure, parse all nodes, and detect parsing errors
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsxcrates/grida-canvas-wasm/example/main.jspackages/grida-canvas-io/index.ts
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Handle NodeId to UserNodeId conversion via `IdConverter` during .grida file loading
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Custom images used as halftone shapes should be loaded as HTMLImageElement for efficient canvas rendering and preserve original image colors in as-is mode
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsxeditor/app/(www)/(home)/www-embed/demo-canvas/page.tsx
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to editor/lib/**/*.{ts,tsx} : Use /editor/lib for core, strictly designed modules with non-opinionated, reusable, and stable implementations
Applied to files:
editor/grida-canvas-hosted/playground/playground.tsx
📚 Learning: 2025-12-01T00:22:36.510Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-dev/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:36.510Z
Learning: Use `cargo run -p grida-dev -- scene <path-or-url>` command to run local `.grida` files or remote scenes through the dev runtime
Applied to files:
crates/grida-canvas-wasm/example/main.js
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to editor/www/**/*.{ts,tsx} : Use /editor/www for landing page specific components
Applied to files:
editor/app/(www)/(home)/www-embed/demo-canvas/page.tsx
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to editor/scaffolds/**/*.{ts,tsx} : Use /editor/scaffolds for feature-specific larger components, pages, and editors
Applied to files:
editor/app/(www)/(home)/www-embed/demo-canvas/page.tsx
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : When adding new shape types, update the Shape type union, add cases in drawShape() function, add cases in shapeToSVG() function, and add SelectItem in UI
Applied to files:
editor/grida-canvas/editor.ts
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Implement the Blink (Chrome) font selection model in the core font selection pipeline
Applied to files:
editor/grida-canvas/editor.ts
🧬 Code graph analysis (3)
editor/components/formfield/grida-canvas/index.tsx (1)
services/grida-canvas-document-worker-cf/src/do.ts (1)
fetch(87-89)
crates/grida-canvas-wasm/example/main.js (1)
services/grida-canvas-document-worker-cf/src/do.ts (1)
fetch(87-89)
packages/grida-canvas-io/__tests__/archive.test.ts (1)
packages/grida-canvas-io/index.ts (1)
Bitmap(860-865)
⏰ 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: Cursor Bugbot
- GitHub Check: test
🔇 Additional comments (45)
editor/public/examples/canvas/blank.grida1 (1)
2-2: LGTM: Version bump correctly reflects the schema migration.The version update to 0.90.0-beta+20260108 aligns with the PR's schema migration. Since this is a blank canvas example with no nodes requiring layout property updates, the version bump alone is appropriate.
editor/public/examples/canvas/globals-01.grida1 (1)
2-2: Schema migration looks correct for root node.The version bump to 0.90.0 and the layout property migrations (position → layout_positioning, left/top → layout_inset_left/top, width/height → layout_target_width/height) align with the PR objectives.
Also applies to: 15-19
editor/public/examples/canvas/component-01.grida1 (5)
2-2: LGTM on schema migration for component node.The version bump to
0.90.0-beta+20260108and the layout field renames (layout_positioning,layout_inset_*,layout_target_*,layout_padding_*,clips_content) are consistent with the new schema model described in the PR objectives.Also applies to: 15-20, 44-47
57-63: LGTM on title node migration.The type change to
"tspan"and the layout field updates are consistent with the schema migration. The"auto"values forlayout_target_widthandlayout_target_heightcorrectly indicate intrinsic text sizing.
131-135: LGTM on image node layout migration.The layout fields are correctly migrated to the new naming convention with appropriate numeric values for absolute positioning.
148-152: LGTM on rectangle node layout migration.Layout fields correctly follow the new schema convention.
192-196: LGTM on ellipse node layout migration.Layout fields correctly migrated to the new schema convention.
editor/app/(www)/(home)/www-embed/demo-canvas/page.tsx (1)
13-13: No changes needed – file exists and format is consistent with codebase.The file
hero-main-demo.grida1exists at/examples/canvas/hero-main-demo.grida1and the.grida1format is the standard format used throughout all example files in the repository. All other example files ineditor/public/examples/canvas/use the same.grida1extension, making this change consistent with the existing codebase pattern.Likely an incorrect or invalid review comment.
.vscode/settings.json (1)
33-33: Migration to.grida1for legacy JSON format is complete and verified.The VSCode file association change from
.gridato.grida1(both mapping to JSON) is correct. Verification confirms:
- 9
.grida1files (legacy JSON format) exist consistently across the codebase- 0 orphaned
.gridafiles remain- The new
.gridaformat is a ZIP/FlatBuffer binary format introduced on 2026-01-03, which does not require JSON syntax highlighting in VSCodeThe migration and settings update are complete and appropriate.
packages/grida-canvas-io/__tests__/archive.test.ts (3)
17-22: VerifyFile/Blobavailability in the test runtime (Node/Jest env).This test helper will fail in a pure Node test environment without
File/Blobglobals (or if Jest isn’t running with the expected environment).
146-178: Byte-perfect FlatBuffers round-trip equality can be flaky if encoding isn’t canonical.This asserts
encode(decode(bytes))returns the exact same bytes; that’s only safe if the encoder is deterministic/canonical w.r.t. field ordering, defaults, and table/vtable layout. If not guaranteed, assert semantic equivalence (decoded document) instead of byte equality.
254-266: Archive snapshot coverage fordocument.grida1looks good.Nice to see an explicit check that the legacy snapshot is present and parseable, and that the embedded version matches.
crates/grida-canvas-wasm/example/main.js (1)
44-44: LGTM — demo.grida1 asset file exists.The migration from
demo.gridatodemo.grida1is correctly implemented and the asset file is present in the example directory.editor/public/examples/canvas/layout-01.grida1 (1)
5-260: Migration to layout-prefixed schema appears complete and consistent.The transformation from legacy fields to the new layout_* naming convention has been applied thoroughly:
- All positioning/sizing fields properly migrated (position → layout_positioning, width/height → layout_target_*, etc.)
- Padding and layout direction fields consistently prefixed
- Node hierarchy and structure preserved
- "auto" sizing correctly used as string values for flex containers
editor/public/examples/canvas/helloworld.grida1 (2)
49-55: No actionable issues. The layout pattern withlayout_positioning: "absolute"combined with four insets and"auto"dimensions is valid, intentional, and tested. This pattern is correctly used for text nodes that auto-size content while maintaining positional constraints.Likely an incorrect or invalid review comment.
2-2: This review comment is incorrect. The.grida1file extension is not a legacy format indicator—it is the standard JSON snapshot format containing the new 0.90.0 schema. All example files in the repository use.grida1(e.g.,blank.grida1,component-01.grida1,layout-01.grida1), and the files correctly contain the new schema withlayout_*fields andtspantype. The file format and schema version are consistent.Likely an incorrect or invalid review comment.
crates/grida-canvas-wasm/example/rectangle.grida1 (3)
2-2: Version bump aligns with the new schema.The version update to 0.90.0 is consistent with the schema migration described in the PR objectives.
10-12: Layout field renames follow the standardization pattern.The renaming of positioning and dimension fields to
layout_*prefixes is consistent with the schema migration objectives and improves semantic clarity.Also applies to: 16-17
15-15: Therotationfield at line 15 is correctly named. There is no evidence in the codebase that it should be renamed topost_layout_transform—this field does not exist as a replacement. The.grida1format usesrotationas the proper field name, consistent with the new schema version (0.90.0-beta) and all other example files in the repository.Likely an incorrect or invalid review comment.
editor/grida-canvas-hosted/playground/playground.tsx (6)
154-174: Excellent OPFS handle management.The hook properly checks for OPFS support, uses memoization to prevent unnecessary handle recreation, and includes defensive checks for empty filekeys. The error handling and null return on failure enable graceful degradation.
176-209: Solid dirty state tracking implementation.The hook correctly manages subscription lifecycle, optimizes for disabled contexts (e.g., demo embeds), and properly distinguishes between user edits and document resets. The callback stability and cleanup logic follow React best practices.
306-368: Well-designed API and integration.The new props are clearly documented with sensible defaults. The filekey resolution logic (explicit > generated > default) is intuitive, and the conditional unsaved changes warning respects the opt-in nature appropriate for demo contexts.
623-656: Clean save implementation with proper feedback.The save handler correctly persists both formats to OPFS, provides user feedback via toast notifications, and integrates with the dirty state tracking through the
onSavedcallback. Error handling is appropriate for user-facing operations.
525-553: Proper prop threading and separation of concerns.The Consumer component correctly receives and uses the new props, maintaining per-instance OPFS handles and enabling save feedback through the callback pattern.
118-119: No action needed. The@grida/iopackage is an established local monorepo package (not an external npm dependency) with comprehensive OPFS support already implemented and properly validated for browser compatibility.Likely an incorrect or invalid review comment.
editor/grida-canvas-hosted/playground/examples.ts (1)
5-35: LGTM! All referenced.grida1example files exist.The URL updates from
.gridato.grida1are consistent and properly reference existing files ineditor/public/examples/canvas/.packages/grida-canvas-io/index.ts (4)
1-9: LGTM: Import and type alias changes are correct.The change from
import type gridatoimport gridais necessary because the code usesgrida.program.document.SCHEMA_VERSION(a value, not just a type). TheGridaDocumenttype alias is a clean solution to avoid namespace shadowing within theionamespace.
1003-1018: Inconsistent error handling for invalid PNG bitmaps.PNG decoding on line 1010 is not wrapped in a try-catch, which means a corrupted bitmap will cause the entire
unpack()operation to fail. In contrast,fileformat.detect()(lines 714-724) silently ignores invalid bitmaps with a try-catch.Verify whether this difference is intentional:
detect()is lenient (extracts what it can)unpack()is strict (fails on any corruption)If archives with corrupted bitmaps should be rejected entirely, the current behavior is correct. If they should be partially recoverable, add error handling here as well.
1187-1329: LGTM: Solid OPFS implementation with proper concurrency handling.The
Handleclass correctly manages concurrent access through_dirHandlePromisecaching (lines 1219-1244), preventing race conditions during directory initialization. Error handling is appropriate:
- Read operations throw clear errors on missing files
- Write operations auto-create files
- Delete operations are idempotent (ignore NotFoundError)
The file handle caching via
Map(lines 1322-1327) is also correctly implemented.
130-130: The change is correct and aligns with the completed tspan migration. TheencodeClipboardTextfunction properly checks forp.type === "tspan"when extracting text from clipboard prototypes. This change is consistent with the node type migration throughout the codebase—confirmed informat.ts(encoding/decoding) and test coverage inclipboard.test.ts.editor/grida-canvas/editor.ts (15)
586-586: LGTM: Documentation updated for new format.The JSDoc example correctly references the
.grida1format introduced for backward compatibility.
748-749: LGTM: Consistent with layout schema migration.The use of
layout_target_widthandlayout_target_heightaligns with the schema migration to layout-prefixed properties.
1736-1747: LGTM: Clean property mapping implementation.The explicit mapping from axis ("width"/"height") to layout properties (
layout_target_width/layout_target_height) is clear and type-safe.
2491-2497: LGTM: New method for clips_content property.The new
changeContainerNodeClipsContentmethod provides a clean API for theclips_contentproperty, replacing ad-hoc overflow handling as mentioned in the PR summary.
2528-2541: LGTM: Flexible gap parameter handling.The method accepts both a uniform gap value (number) and axis-specific gaps (object), making it backward compatible while supporting the new layout-prefixed property names.
3001-3001: LGTM: Version bumped to 0.90.0.The version string update to "0.90.0-beta+20260108" aligns with the breaking schema changes in this PR.
5398-5473: LGTM with existing FIXME noted.The method correctly uses the new layout properties (
layout_target_width/layout_target_height,layout_inset_left/layout_inset_top). The nested RAF pattern is already documented as a known issue requiring API improvements for text layout size access.
1360-1502: Complex layout transition logic - well structured.The
reLayoutmethod handles multiple layout state transitions (none/clear/flex/flex-direction-switch) with proper property updates tolayout_mode,layout_direction, and layout axis properties. The geometry-based calculations for converting flex to absolute positioning are correct.
5634-5634: LGTM: Documentation updated for UnknownNode.JSDoc comments correctly updated to reference
grida.program.nodes.UnknownNode, matching the implementation changes.Also applies to: 5645-5645, 5652-5652, 5663-5663, 5670-5670, 5681-5681, 5688-5688, 5714-5714, 5740-5740, 5752-5752
1679-1698: LGTM: Positioning API updated to layout schema.The positioning-related methods correctly use
layout_positioningfrom the updatedIPositioninginterface, aligning with the schema migration.
4938-4970: LGTM: Gradient stop deletion with proper validation.The method correctly validates that gradients have more than 2 stops (minimum required) before allowing deletion.
2842-2850: No action needed - versioning is correctly handled.The
io.archive.pack()function has a default parameterschemaVersion = grida.program.document.SCHEMA_VERSION, so calling it without an explicit schemaVersion argument is correct. The manifest and archive format will include the current schema version automatically.
41-61: Remove this comment - the type safety concern is based on incorrect assumptions about UnknownNode.UnknownNode is not a union type as claimed; it's an intersection of
Partialtypes (Partial<A> & Partial<B> & Partial<C> & ...). With this structure,keyof UnknownNodeincludes all possible keys from all node type members, not just common keys. The function correctly accepts properties likestroke_width,font_size,rotation, andopacity, which are valid on their respective node types and accessible through the UnknownNode type. The implementation is type-safe.Likely an incorrect or invalid review comment.
758-772: Return type and node type are correct per schema.The method correctly returns
NodeProxy<TextSpanNode>and creates nodes with type"tspan". Both are consistent with the schema definition and widely established throughout the codebase. The existing call sites ineditor/grida-canvas-hosted/ai/tools/canvas-use.tsandeditor/grida-canvas-react/use-data-transfer.tsalready handle the return type correctly. The TODO comment indicates a future rename tocreateTextSpanNodefor naming clarity.
3356-3357: Breaking change confirmed: Text node type changed from "text" to "tspan".The text node type has been renamed from "text" to "tspan" throughout the codebase with no fallback for the old type. This is a hard cutover affecting all text node type checks in editor/grida-canvas. All methods that operate on text nodes (e.g.,
changeTextNodeFontWeight,changeTextNodeFontSize,toggleTextNodeBold, etc.) now check for type "tspan" before proceeding. The renderer implementation correctly handles the "tspan" type ingrida-canvas-react-renderer-dom/nodes/tspan.tsx.
| @@ -1,21 +1,20 @@ | |||
| { | |||
| "version": "0.89.0-beta+20251219", | |||
| "version": "0.90.0-beta+20260108", | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and inspect .grida and .grida1 files in the example directory
echo "=== Finding .grida and .grida1 files ==="
fd -e grida -e grida1 . crates/grida-canvas-wasm/example/
echo -e "\n=== Checking versions in example files ==="
fd -e grida -e grida1 . crates/grida-canvas-wasm/example/ --exec sh -c 'echo "File: {}"; jq -r ".version" {} 2>/dev/null || echo "No version field"'
echo -e "\n=== Checking document structure and fields ==="
fd -e grida -e grida1 . crates/grida-canvas-wasm/example/ --exec sh -c 'echo "=== File: {} ==="; jq "keys" {} 2>/dev/null'
echo -e "\n=== Checking for layout_ prefix fields vs legacy fields ==="
fd -e grida -e grida1 . crates/grida-canvas-wasm/example/ --exec sh -c 'echo "=== File: {} ==="; jq ".document.nodes | if type == \"object\" then to_entries[0:2] | map({id: .key, keys: (.value | keys)}) else \"Not an object\" end" {} 2>/dev/null'Repository: gridaco/grida
Length of output: 2570
🏁 Script executed:
#!/bin/bash
# Search for documentation about .grida1 format and its purpose
echo "=== Searching for .grida1 documentation ==="
fd -e md -e txt . --exec grep -l "grida1" {} \; 2>/dev/null | head -10
echo -e "\n=== Searching for commit message or changelog mentioning grida1 ==="
git log --all --oneline --grep="grida1" 2>/dev/null | head -20
echo -e "\n=== Searching for grida1 in code comments ==="
rg "grida1" -t rs -t md --context 3 | head -50Repository: gridaco/grida
Length of output: 374
🏁 Script executed:
#!/bin/bash
# Check what the old schema (0.89.0 or earlier) field names were
echo "=== Searching for legacy field patterns (position, top, left, width, height) ==="
rg "\"position\"|\"top\"|\"left\"|\"width\"|\"height\"" crates/grida-canvas-wasm/example/ --context 2 | head -100
echo -e "\n=== Searching for layout_ prefix field patterns ==="
rg "layout_" crates/grida-canvas-wasm/example/ --context 1 | head -100
echo -e "\n=== Searching for schema version references in code ==="
rg "0\.89|0\.90|backward.*compat" crates/grida-canvas-wasm/ -t rs --context 2 | head -100Repository: gridaco/grida
Length of output: 13076
🏁 Script executed:
#!/bin/bash
# Check the commit that introduced .grida1
echo "=== Commit details for .grida1 introduction ==="
git show e66181d9b --stat | head -50
echo -e "\n=== Commit message full ==="
git show e66181d9b --format=fuller | head -30
# Search for schema definitions or migration code
echo -e "\n=== Searching for schema or format definitions ==="
fd -e rs -e ts -e md . crates/grida-canvas-wasm/ | xargs grep -l "grida1\|schema\|backward" 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 4395
🏁 Script executed:
#!/bin/bash
# Look for any .grida files (without the 1) in the entire repo
echo "=== Searching for .grida files (without the 1 suffix) ==="
fd -e grida . --exclude="*.grida1" 2>/dev/null | head -20
# Check what files were changed in the commit
echo -e "\n=== Files changed in the .grida1 introduction commit ==="
git show e66181d9b --name-only | tail -20
# Look for any code that reads or parses .grida1 files
echo -e "\n=== Code referencing file format ==="
rg "grida1" -t toml -t yaml -t json --context 3 | head -100Repository: gridaco/grida
Length of output: 1401
🏁 Script executed:
#!/bin/bash
# Search for any configuration or documentation about file formats
echo "=== Searching for README or AGENTS.md files ==="
fd AGENTS.md README.md . crates/grida-canvas-wasm/
echo -e "\n=== Searching for format migration or compatibility discussion ==="
rg "backward\|compat\|legacy\|0\.89\|0\.90" --context 2 crates/grida-canvas-wasm/ | head -100Repository: gridaco/grida
Length of output: 532
🏁 Script executed:
#!/bin/bash
echo "=== AGENTS.md from grida-canvas-wasm ==="
cat crates/grida-canvas-wasm/AGENTS.md
echo -e "\n=== Searching for file format parsing or loading code ==="
fd . crates/grida-canvas-wasm/src --type f -name "*.rs" | head -10
echo -e "\n=== Looking for .grida1 in any Rust or TypeScript files ==="
rg "grida1\|\.grida" crates/grida-canvas-wasm/ -A 2 -B 2 | head -150Repository: gridaco/grida
Length of output: 7485
🏁 Script executed:
#!/bin/bash
# Check if there's version or schema handling code
echo "=== Searching for version handling code ==="
rg "0\.90\|0\.89\|version" crates/grida-canvas-wasm/src/ --type rust -A 2 -B 2 | head -100
# Look at what the commit actually changed in the main code
echo -e "\n=== Checking git diff for the commit (filtered) ==="
git show e66181d9b -- "crates/grida-canvas-wasm/**" 2>/dev/null | head -200Repository: gridaco/grida
Length of output: 6983
🏁 Script executed:
#!/bin/bash
# Search for any documentation about the purpose of .grida1
echo "=== Main AGENTS.md ==="
head -100 AGENTS.md
echo -e "\n=== Canvas AGENTS.md ==="
head -100 crates/grida-canvas/AGENTS.mdRepository: gridaco/grida
Length of output: 5712
Clarify the purpose and schema versioning of .grida1 files.
The commit message states .grida1 is introduced "for backward compatibility," but the example files (rectangle.grida1, demo.grida1) both use version 0.90.0-beta+20260108 with new layout_* prefixed fields (layout_positioning, layout_inset_*, layout_target_*), not a legacy schema. This creates ambiguity:
- If
.grida1is meant to preserve the old schema format for backward compatibility, the examples should demonstrate the legacy field structure. - If
.grida1represents the new format going forward, the commit message should clarify that it introduces new schema structure.
Add documentation to crates/grida-canvas-wasm/AGENTS.md explaining the purpose of .grida1 format and whether it supports legacy file loading or represents a new format version.
|
|
||
| useEffect(() => { | ||
| fetch("/examples/canvas/blank.grida").then((res) => { | ||
| fetch("/examples/canvas/blank.grida1").then((res) => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for blank.grida1 files in the repository
echo "=== Searching for blank.grida1 files ==="
fd -t f 'blank\.grida1$' 2>/dev/null || echo "No blank.grida1 files found"
# Check for remaining blank.grida references (excluding .grida1)
echo -e "\n=== Checking for remaining blank.grida references ==="
rg -n 'blank\.grida[^1]' --type ts --type tsx 2>/dev/null || echo "No remaining references found"
# Also check all .grida references in TypeScript files
echo -e "\n=== All .grida references in TypeScript files ==="
rg -n '\.grida' --type ts --type tsx 2>/dev/null || echo "No .grida references found"Repository: gridaco/grida
Length of output: 303
🏁 Script executed:
cat -n editor/components/formfield/grida-canvas/index.tsx | head -70Repository: gridaco/grida
Length of output: 2779
🏁 Script executed:
echo "=== Looking for public/examples directory ==="
find . -type d -name examples 2>/dev/null | head -20
echo -e "\n=== Checking for any .grida files in examples ==="
find . -path "*/examples/*" -name "*.grida*" 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 823
Add error handling to the fetch chain.
The file path change to /examples/canvas/blank.grida1 is correct—the file exists. However, the fetch() and res.json() calls lack error handling. If the fetch fails or JSON parsing fails, the promise rejection will be unhandled and the editor will silently fail to initialize. Add .catch() handlers or use async/await with try-catch to ensure users receive feedback if initialization fails.
🤖 Prompt for AI Agents
In @editor/components/formfield/grida-canvas/index.tsx at line 51, The fetch
call starting with fetch("/examples/canvas/blank.grida1") currently lacks error
handling for network failures and JSON parse errors; wrap the fetch/res.json()
sequence in a try/catch (or append .catch()) and handle failures by logging the
error and updating component state (e.g., set an error or isInitialized flag) so
the editor can show a user-facing message or fallback instead of failing
silently; specifically modify the code around the fetch(...) and res.json()
usage in the component (where the initial canvas is loaded) to catch and handle
both fetch and JSON parsing errors.
| // The paintIndex parameter (0) is only used for resolvedIndex calculation, which we ignore | ||
| const { paints: currentFills } = editor.resolvePaints( | ||
| node as grida.program.nodes.UnknwonNode, | ||
| node as grida.program.nodes.UnknownNode, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Type safety concern: Unchecked UnknownNode casts.
Multiple locations cast to UnknownNode before accessing properties without runtime type validation. Since UnknownNode is a union type, these casts bypass TypeScript's type safety. Runtime errors could occur if the node type doesn't have the accessed property.
🔧 Add type guards before casts
Example pattern for safer access:
public changeNodePropertyStrokeWidth(
node_id: string,
strokeWidth: editor.api.NumberChange
) {
try {
- const value = resolveNumberChangeValue(
- this.getNodeSnapshotById(node_id) as grida.program.nodes.UnknownNode,
- "stroke_width",
- strokeWidth
- );
+ const node = this.getNodeSnapshotById(node_id);
+ // Type guard: only nodes with stroke_width can have it changed
+ if (!('stroke_width' in node)) {
+ throw new Error(`Node ${node_id} does not support stroke_width`);
+ }
+ const value = resolveNumberChangeValue(
+ node as grida.program.nodes.UnknownNode,
+ "stroke_width",
+ strokeWidth
+ );Also applies to: 1808-1808, 1880-1880, 2043-2043, 2298-2298, 2325-2325, 2354-2354, 4948-4948, 5702-5702, 5728-5728
🤖 Prompt for AI Agents
In @editor/grida-canvas/editor.ts at line 1803, Several places cast values with
"node as grida.program.nodes.UnknownNode" and then access properties without
runtime checks; add a type guard function (e.g., function isUnknownNode(node:
any): node is grida.program.nodes.UnknownNode { return node && typeof node ===
'object' && 'type' in node && /* optional further checks for required props */
true; }) and use it before casting and property access (replace direct casts in
occurrences of "node as grida.program.nodes.UnknownNode" with an if
(isUnknownNode(node)) { const unknown = node; /* safe access */ } else { /*
handle unexpected type / bail out */ }); apply this pattern to every reported
location (the exact cast sites containing "node as
grida.program.nodes.UnknownNode" around the lines referenced) so access is
guarded and runtime errors are avoided.
| "description": { | ||
| "id": "description", | ||
| "name": "text", | ||
| "name": "tspan", | ||
| "active": true, | ||
| "locked": false, | ||
| "rotation": 0, | ||
| "opacity": 1, | ||
| "z_index": 0, | ||
| "type": "text", | ||
| "type": "tspan", | ||
| "text": "Text values can be programmed via `props`", | ||
| "position": "absolute", | ||
| "left": 59, | ||
| "top": 305, | ||
| "width": "auto", | ||
| "height": "auto", | ||
| "layout_positioning": "absolute", | ||
| "layout_inset_left": 59, | ||
| "layout_inset_top": 305, | ||
| "layout_target_width": "auto", | ||
| "layout_target_height": "auto", |
There was a problem hiding this comment.
The name field should remain "description", not "tspan".
The name field is a user-facing identifier and should remain descriptive. Comparing to the "title" node (line 51) which correctly keeps "name": "title" while changing "type": "tspan", this node's name appears to have been incorrectly changed during migration.
Proposed fix
"description": {
"id": "description",
- "name": "tspan",
+ "name": "description",
"active": true,📝 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.
| "description": { | |
| "id": "description", | |
| "name": "text", | |
| "name": "tspan", | |
| "active": true, | |
| "locked": false, | |
| "rotation": 0, | |
| "opacity": 1, | |
| "z_index": 0, | |
| "type": "text", | |
| "type": "tspan", | |
| "text": "Text values can be programmed via `props`", | |
| "position": "absolute", | |
| "left": 59, | |
| "top": 305, | |
| "width": "auto", | |
| "height": "auto", | |
| "layout_positioning": "absolute", | |
| "layout_inset_left": 59, | |
| "layout_inset_top": 305, | |
| "layout_target_width": "auto", | |
| "layout_target_height": "auto", | |
| "description": { | |
| "id": "description", | |
| "name": "description", | |
| "active": true, | |
| "locked": false, | |
| "rotation": 0, | |
| "opacity": 1, | |
| "z_index": 0, | |
| "type": "tspan", | |
| "text": "Text values can be programmed via `props`", | |
| "layout_positioning": "absolute", | |
| "layout_inset_left": 59, | |
| "layout_inset_top": 305, | |
| "layout_target_width": "auto", | |
| "layout_target_height": "auto", |
🤖 Prompt for AI Agents
In @editor/public/examples/canvas/component-01.grida1 around lines 85 - 99, The
"description" node's name was incorrectly set to "tspan"; restore the
user-facing identifier by changing the node object whose "id" is "description"
so that "name": "description" while leaving "type": "tspan" intact— locate the
object containing "id": "description" and update its "name" property back to
"description" to match the "title" node pattern.
| "text": { | ||
| "id": "text", | ||
| "name": "text", | ||
| "id": "tspan", |
There was a problem hiding this comment.
Critical: Broken node reference in links section.
The text node's ID was changed to "tspan" (line 35), but the links section still references "text" (line 91). This mismatch will break the document graph structure and prevent the child node from being properly linked to its parent.
🐛 Fix the reference mismatch
"links": {
"root": [
- "text"
+ "tspan"
],Also applies to: 91-91
🤖 Prompt for AI Agents
In @editor/public/examples/canvas/globals-01.grida1 at line 35, The node id was
renamed to "tspan" but the links section still references "text", breaking the
graph; update the links entry that currently points to "text" so it references
"tspan" (or alternatively rename the node id back to "text") ensuring the child
node id ("tspan") and its parent link target match exactly; locate the "id":
"tspan" declaration and the corresponding link object that mentions "text" and
make them consistent.
| function toU8(bytes: Uint8Array | Uint8ClampedArray): Uint8Array { | ||
| return bytes instanceof Uint8Array ? bytes : new Uint8Array(bytes.buffer); | ||
| } |
There was a problem hiding this comment.
toU8 should preserve byteOffset/byteLength when wrapping typed arrays.
new Uint8Array(bytes.buffer) can accidentally include unrelated bytes if bytes is a view into a larger buffer. Safer to slice with offset/length.
Proposed fix
function toU8(bytes: Uint8Array | Uint8ClampedArray): Uint8Array {
- return bytes instanceof Uint8Array ? bytes : new Uint8Array(bytes.buffer);
+ // Preserve view bounds (handles non-zero byteOffset cases safely)
+ return bytes instanceof Uint8Array
+ ? bytes
+ : new Uint8Array(bytes.buffer, bytes.byteOffset, bytes.byteLength);
}🤖 Prompt for AI Agents
In @packages/grida-canvas-io/__tests__/archive.test.ts around lines 7 - 9, The
toU8 helper currently creates a new Uint8Array from bytes.buffer which loses the
original view's byteOffset/byteLength; update toU8 (that handles Uint8Array |
Uint8ClampedArray) to return the original Uint8Array when already that type,
otherwise construct a new Uint8Array using the source buffer with the source
view's byteOffset and byteLength so the wrapped view preserves the exact slice
of the underlying ArrayBuffer.
| it("should pack/unpack archive with bitmaps (png)", () => { | ||
| const document = createTestDocument(); | ||
| const bitmap: io.Bitmap = { | ||
| version: 0, | ||
| width: 2, | ||
| height: 2, | ||
| // RGBA pixels (2x2) | ||
| data: new Uint8ClampedArray([ | ||
| 255, 0, 0, 255, 0, 255, 0, 255, 0, 0, 255, 255, 255, 255, 255, 255, | ||
| ]), | ||
| }; | ||
|
|
||
| const packed = io.archive.pack(document, undefined, schemaVersion, { | ||
| preview: bitmap, | ||
| }); | ||
|
|
||
| const unpacked = io.archive.unpack(packed); | ||
| expect(Object.keys(unpacked.bitmaps)).toEqual(["preview"]); | ||
| expect(unpacked.bitmaps.preview.width).toBe(2); | ||
| expect(unpacked.bitmaps.preview.height).toBe(2); | ||
| expect(unpacked.bitmaps.preview.data).toEqual(bitmap.data); | ||
| }); |
There was a problem hiding this comment.
Bitmap pixel equality assertion may be brittle across typed-array constructors.
If unpacked.bitmaps.preview.data comes back as Uint8Array (not Uint8ClampedArray) or vice-versa, toEqual(bitmap.data) might fail depending on the matcher/runtime. Consider normalizing via toU8() + bytesEqual().
🤖 Prompt for AI Agents
In @packages/grida-canvas-io/__tests__/archive.test.ts around lines 103 - 124,
The assertion comparing bitmap pixel arrays is brittle because typed-array
constructors may differ; instead of
expect(unpacked.bitmaps.preview.data).toEqual(bitmap.data), normalize both sides
to Uint8Array using toU8() and compare with bytesEqual() (or an equivalent
byte-wise comparison) so the test uses
bytesEqual(toU8(unpacked.bitmaps.preview.data), toU8(bitmap.data)); update the
assertion in the "should pack/unpack archive with bitmaps (png)" test that
references unpacked.bitmaps.preview.data and bitmap.data to use these helpers.
| it("performance: should pack/unpack mixed real files efficiently", () => { | ||
| const document = createTestDocument(); | ||
| const mixedImages = { | ||
| "checker.png": fixtureImages["checker.png"], | ||
| "stripes.png": fixtureImages["stripes.png"], | ||
| "1024.jpg": fixtureImages["1024.jpg"], | ||
| "512.jpg": fixtureImages["512.jpg"], | ||
| }; | ||
|
|
||
| const start = Date.now(); | ||
| const packed = io.archive.pack(document, mixedImages, schemaVersion); | ||
| const unpacked = io.archive.unpack(packed); | ||
| const end = Date.now(); | ||
|
|
||
| expect(Object.keys(unpacked.images)).toHaveLength(4); | ||
| expect(end - start).toBeLessThan(5000); | ||
| }); |
There was a problem hiding this comment.
Time-based perf assertions are prone to CI flakiness.
Date.now() + “< 5000ms” can fail under contention. If you keep this, consider:
- marking as a separate perf suite (opt-in),
- using
it.skipunless env var enabled, - or asserting only functional properties here.
🤖 Prompt for AI Agents
In @packages/grida-canvas-io/__tests__/archive.test.ts around lines 212 - 228,
The time-based assertion in the test "performance: should pack/unpack mixed real
files efficiently" is flaky; remove the Date.now() timing check or gate it
behind an opt-in flag instead. Either (a) delete the start/end/expect(end -
start) assertion and only assert functional behavior
(expect(Object.keys(unpacked.images)).toHaveLength(4)), or (b) wrap the test in
a conditional so it runs only when a PERF_TEST (or similar) env var is set (use
it.skip or process.env check) so the io.archive.pack and io.archive.unpack
functional assertions remain but the timing assertion is executed only in
explicit performance runs. Ensure references to io.archive.pack,
io.archive.unpack, and the test title remain unchanged for discoverability.
| export async function detect(file: File): Promise<Detected> { | ||
| // ZIP? (cheap: reads 4 bytes) | ||
| const head4 = new Uint8Array(await file.slice(0, 4).arrayBuffer()); | ||
| const isZip = | ||
| head4.length >= 4 && | ||
| head4[0] === 0x50 && | ||
| head4[1] === 0x4b && | ||
| (head4[2] === 0x03 || head4[2] === 0x05); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the full context of the file around line 673-680
head -n 700 packages/grida-canvas-io/index.ts | tail -n 100Repository: gridaco/grida
Length of output: 3184
🏁 Script executed:
# Search for any comments about ZIP signatures or the 0x05 check
rg -n "0x05|0x03|0x04" packages/grida-canvas-io/ -A2 -B2Repository: gridaco/grida
Length of output: 1094
🏁 Script executed:
# Look for tests or usage of the detect function
rg -n "detect.*file\|fileformat" packages/grida-canvas-io/ -A3 -B3Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check if there's any documentation about the detect function or ZIP detection
rg -n "ZIP|zip.*detect\|detect.*zip" packages/grida-canvas-io/ -A5 -B5Repository: gridaco/grida
Length of output: 19582
🏁 Script executed:
# Search for test files or example files that might show 0x05 being used
rg -n "0x05" packages/grida-canvas-io/ -A3 -B3Repository: gridaco/grida
Length of output: 543
🏁 Script executed:
# Check git history for when the 0x05 check was added
git log --oneline -p packages/grida-canvas-io/index.ts | grep -A10 -B10 "0x05" | head -50Repository: gridaco/grida
Length of output: 915
🏁 Script executed:
# Look for any comments or documentation about spanned archives or edge cases
rg -n "spanned|empty.*zip|edge.*case" packages/grida-canvas-io/ -iRepository: gridaco/grida
Length of output: 424
🏁 Script executed:
# Look for any fflate documentation or usage that might explain 0x05
rg -n "unzipSync\|fflate" packages/grida-canvas-io/ -B3 -A3Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check if there are any real test files or fixtures in the test directory
ls -la packages/grida-canvas-io/__tests__/fixtures/Repository: gridaco/grida
Length of output: 148
🏁 Script executed:
# Search for unzipSync usage more broadly
rg -n "unzipSync" packages/grida-canvas-io/ -B5 -A5Repository: gridaco/grida
Length of output: 4911
🏁 Script executed:
# Check if there's any documentation about ZIP format variations in comments or README
find packages/grida-canvas-io -name "*.md" -o -name "*.ts" | xargs grep -l "0x03\|0x05\|ZIP.*format"Repository: gridaco/grida
Length of output: 90
🏁 Script executed:
# Search fflate package documentation in node_modules or check imports
rg -n "from.*fflate\|import.*fflate" packages/grida-canvas-io/Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check fflate documentation or comments about what signatures it accepts
rg -n "fflate" packages/grida-canvas-io/ -B10 -A10 | head -80Repository: gridaco/grida
Length of output: 4774
🏁 Script executed:
# Search for any issue tracking or comments that might mention why 0x05 was added
git log --all --oneline --grep="0x05\|zip.*detect\|magic.*number" packages/grida-canvas-io/ 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Compare the two functions side by side to highlight the inconsistency
cat packages/grida-canvas-io/index.ts | sed -n '626,680p'Repository: gridaco/grida
Length of output: 1674
🏁 Script executed:
# Search for any related issues or discussions about ZIP format support
rg -n "zip.*variant\|empty.*zip\|spanned.*zip" packages/grida-canvas-io/ -iRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check what fflate actually accepts by looking for any error handling or validation
cat packages/grida-canvas-io/index.ts | sed -n '682,740p'Repository: gridaco/grida
Length of output: 2099
🏁 Script executed:
# Let's check if there's any changelog or release notes mentioning this
find packages/grida-canvas-io -name "CHANGELOG*" -o -name "HISTORY*" -o -name "NEWS*" 2>/dev/nullRepository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Search for any PR or commit messages that might mention ZIP detection
git log --all --oneline packages/grida-canvas-io/index.ts 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 224
🏁 Script executed:
# Check the fflate version to see if there's any known issue with certain ZIP formats
cat packages/grida-canvas-io/package.json | grep -A2 -B2 fflateRepository: gridaco/grida
Length of output: 197
🏁 Script executed:
# Let's do a final check - search for any references to ZIP variants, spanned archives, or 0x05 in the entire codebase
rg -n "0x05" . --type=ts --type=tsx --type=js --type=jsx 2>/dev/nullRepository: gridaco/grida
Length of output: 39
Fix inconsistent ZIP magic number validation between is_zip() and fileformat.detect().
The is_zip() function strictly validates the standard ZIP signature (0x50 0x4B 0x03 0x04), but fileformat.detect() accepts head4[2] === 0x03 || head4[2] === 0x05. The byte 0x05 at position [2] matches an end-of-central-directory record signature, which should never appear at file start per the PKWARE specification. This check is undocumented and has no test coverage. Either remove the 0x05 check to align with is_zip() and the documented standard, or document the intent and add test coverage.
🤖 Prompt for AI Agents
In @packages/grida-canvas-io/index.ts around lines 673 - 680, The ZIP detection
in fileformat.detect is too permissive: it checks head4[2] === 0x03 || head4[2]
=== 0x05 while is_zip() validates the standard 0x50 0x4B 0x03 0x04 signature;
remove the 0x05 branch in fileformat.detect (use the same strict check as
is_zip) so head4 is compared against the standard ZIP magic only, keeping the
validation consistent between detect() and is_zip().
| export function unpack(zipData: Uint8Array): SnapshotDocumentModel { | ||
| const files = unzipSync(zipData); | ||
| const snapshotJson = strFromU8(files["snapshot.json"]); | ||
| if (!snapshotJson) { | ||
| throw new Error("Missing snapshot.json in zip file"); | ||
| } | ||
| return value; | ||
| }); | ||
| return parse(snapshotJson) as SnapshotDocumentModel; | ||
| } |
There was a problem hiding this comment.
Improve error handling for missing snapshot.json.
The current error handling is inconsistent. If files["snapshot.json"] is undefined, strFromU8() will throw a TypeError before the check on line 852 can execute.
🔧 Suggested fix
export function unpack(zipData: Uint8Array): SnapshotDocumentModel {
const files = unzipSync(zipData);
- const snapshotJson = strFromU8(files["snapshot.json"]);
- if (!snapshotJson) {
+ if (!files["snapshot.json"]) {
throw new Error("Missing snapshot.json in zip file");
}
+ const snapshotJson = strFromU8(files["snapshot.json"]);
return parse(snapshotJson) as SnapshotDocumentModel;
}📝 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.
| export function unpack(zipData: Uint8Array): SnapshotDocumentModel { | |
| const files = unzipSync(zipData); | |
| const snapshotJson = strFromU8(files["snapshot.json"]); | |
| if (!snapshotJson) { | |
| throw new Error("Missing snapshot.json in zip file"); | |
| } | |
| return value; | |
| }); | |
| return parse(snapshotJson) as SnapshotDocumentModel; | |
| } | |
| export function unpack(zipData: Uint8Array): SnapshotDocumentModel { | |
| const files = unzipSync(zipData); | |
| if (!files["snapshot.json"]) { | |
| throw new Error("Missing snapshot.json in zip file"); | |
| } | |
| const snapshotJson = strFromU8(files["snapshot.json"]); | |
| return parse(snapshotJson) as SnapshotDocumentModel; | |
| } |
🤖 Prompt for AI Agents
In @packages/grida-canvas-io/index.ts around lines 849 - 856, In unpack, calling
strFromU8(files["snapshot.json"]) can throw if the entry is missing; first check
that files["snapshot.json"] is defined (and is a Uint8Array or Buffer-like)
before calling strFromU8, and if missing throw a clear Error like "Missing
snapshot.json in zip file"; update the unpack function to perform the
existence/type guard on files["snapshot.json"] prior to calling strFromU8 and
then call parse(snapshotJson) to return the SnapshotDocumentModel.
….zip with .grida1.zip format
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts:
- Around line 416-417: Replace the skipped test suite declaration
describe.skip("apply-scale round-trip (accuracy)", ...) with
describe("apply-scale round-trip (accuracy)", ...) to re-enable the
fixture-based roundtrip accuracy tests; remove the TODO comment, ensure the
fixture d1-20251209.grida1.zip is present in the test fixtures used by the tests
(and update any fixture path references if they changed), and run the test
locally/CI to confirm the suite executes without additional timeout or
environment changes.
In @packages/grida-canvas-io/index.ts:
- Around line 694-695: The conditional block checks manifest.document_file ===
"document.grida" && document, so the inner guard if (!document) return { kind:
"unknown", bytes }; is unreachable and should be removed; update the block
around the manifest.document_file branch (where the variable document and bytes
are used) to rely on the outer existence check and eliminate the redundant null
check and return.
🧹 Nitpick comments (4)
packages/grida-canvas-io/index.ts (1)
849-856: Improve error handling for missing file in ZIP archive.If
files["document.grida1"]is undefined (file missing from ZIP),strFromU8(undefined)may throw a TypeError before reaching the null check on line 852. Consider checking for file existence before callingstrFromU8.♻️ Proposed fix
export function unpack(zipData: Uint8Array): SnapshotDocumentModel { const files = unzipSync(zipData); - const snapshotJson = strFromU8(files["document.grida1"]); - if (!snapshotJson) { + const file = files["document.grida1"]; + if (!file) { throw new Error("Missing document.grida1 in zip file"); } + const snapshotJson = strFromU8(file); return parse(snapshotJson) as SnapshotDocumentModel; }editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts (3)
157-159: Consider simplifying thesatisfiespattern for property checks.The
satisfiesoperator is compile-time only and serves to verify the string literal is a validUnknownNodePropertiesKey. While this provides type safety, the inline syntax is somewhat awkward. Consider extracting these checks into a type-safe helper function for better readability, or simplify to just theincheck if the compile-time verification isn't critical in this context.Example refactor:
// Extract to a helper function hasProperty<K extends grida.program.nodes.UnknownNodePropertiesKey>( node: grida.program.nodes.UnknownNode, key: K ): boolean { return key in node; } // Then use: hasProperty(tspanNode, "layout_inset_left")Also applies to: 162-164, 184-186, 189-191
642-645: Consider using consistent bracket notation for node access.The test uses dot notation (
state_auto.document.nodes.rect1) to access nodes, which is inconsistent with the bracket notation pattern used throughout the rest of the file (state.document.nodes[id]). While this works for simple IDs, bracket notation is more robust and consistent.♻️ Suggested change for consistency
- const a = state_auto.document.nodes - .rect1 as grida.program.nodes.RectangleNode; - const g = state_global.document.nodes - .rect1 as grida.program.nodes.RectangleNode; + const a = state_auto.document.nodes["rect1"] as grida.program.nodes.RectangleNode; + const g = state_global.document.nodes["rect1"] as grida.program.nodes.RectangleNode;
114-114: Consider reducingas anytype casts for better type safety.Multiple
as anycasts are present throughout the test file. While these may be pragmatic for testing, they reduce type safety and can mask issues. Consider:
- Using proper action types instead of
as anyfor dispatch calls- Defining a typed helper for accessing nodes if the strict typing is problematic
- Using more specific types for the geometry stub return type
These improvements would make the tests more robust and catch type-related issues earlier.
Also applies to: 200-200, 294-294, 326-326, 385-385, 506-506
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
fixtures/test-grida/d1-20251209.grida1.zipis excluded by!**/*.zip
📒 Files selected for processing (3)
editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.tsfixtures/test-grida/README.mdpackages/grida-canvas-io/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- fixtures/test-grida/README.md
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript 5 as the main language for most apps
Use Lucide or Radix Icons for icons
Files:
packages/grida-canvas-io/index.tseditor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts
{editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Files:
packages/grida-canvas-io/index.tseditor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts
packages/grida-canvas-*/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Files:
packages/grida-canvas-io/index.ts
editor/grida-*/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to /packages
Files:
editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to packages/grida-canvas-*/**/*.{ts,tsx,js,jsx} : Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.grida : Validate .grida files using the `tool_io_grida` CLI tool to check file structure, parse all nodes, and detect parsing errors
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Handle NodeId to UserNodeId conversion via `IdConverter` during .grida file loading
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo fmt` to maintain code formatting standards
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Applied to files:
packages/grida-canvas-io/index.tseditor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to packages/grida-canvas-*/**/*.{ts,tsx,js,jsx} : Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Applied to files:
packages/grida-canvas-io/index.tseditor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to editor/grida-*/**/*.{ts,tsx} : Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to <root>/packages
Applied to files:
packages/grida-canvas-io/index.ts
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.grida : Validate .grida files using the `tool_io_grida` CLI tool to check file structure, parse all nodes, and detect parsing errors
Applied to files:
packages/grida-canvas-io/index.tseditor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : For SVG export, convert circles to <circle> elements, rectangles to <rect> elements, and polygons to <polygon> elements with calculated points
Applied to files:
editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : When adding new shape types, update the Shape type union, add cases in drawShape() function, add cases in shapeToSVG() function, and add SelectItem in UI
Applied to files:
editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Use Canvas 2D API with path commands for rendering geometric shapes (circle, square, triangle, etc.)
Applied to files:
editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Cache ImageData and dimensions in refs (imageDataRef, sizeRef) for efficient exports
Applied to files:
editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts
📚 Learning: 2025-12-01T00:22:36.510Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-dev/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:36.510Z
Learning: Remote scenes are fetched with `reqwest`; stick to local `.grida` files when offline
Applied to files:
editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts
🧬 Code graph analysis (2)
packages/grida-canvas-io/index.ts (1)
packages/grida-canvas-schema/index.ts (1)
grida(1-1)
editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts (1)
packages/grida-canvas-schema/grida.ts (3)
UnknownNode(1249-1272)UnknownNodePropertiesKey(1275-1275)Node(1184-1184)
⏰ 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: Cursor Bugbot
- GitHub Check: test
🔇 Additional comments (13)
packages/grida-canvas-io/index.ts (9)
39-50: LGTM!The chunked approach with 32KB chunks effectively prevents stack overflow issues when processing large arrays. The implementation is correct and well-documented.
122-140: LGTM!The type check for
"tspan"aligns with the schema migration mentioned in the PR objectives, where text nodes are renamed to tspan.
642-650: LGTM!The FlatBuffers file identifier check is correctly implemented. The magic bytes validation for "GRID" at offset 4-7 follows the FlatBuffers specification.
753-803: LGTM!The load function correctly handles both ZIP archive and raw FlatBuffers formats. Error handling for missing image dimensions is appropriate, and the delegation to
fileformat.detectavoids redundant file parsing.
903-968: LGTM!The archive pack function correctly handles the ZIP container format with FlatBuffers document, JSON snapshot, images, and bitmaps. The type assertions for accessing non-standard fields (
bitmaps) are necessary for backward compatibility during the schema migration.
1024-1074: LGTM!The GRID namespace provides a clean API for raw FlatBuffers encoding/decoding. The explicit stripping of non-persisted fields (images, bitmaps) before encoding and restoration of empty collections after decoding is correct and well-documented.
1187-1328: LGTM!The OPFS Handle implementation is well-designed with proper caching, error handling, and idempotent operations. The concurrent request handling via
_dirHandlePromiseprevents race conditions, and the special handling ofNotFoundErrorin delete operations makes it idempotent.
1331-1376: LGTM!The zip utilities are straightforward wrappers that provide a clean API for ZIP file creation and data normalization. The implementations are correct.
1378-1383: LGTM!The helper function correctly normalizes PNG data to
Uint8ClampedArray, handling the variability in array types returned by the fast-png library.editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts (4)
8-8: LGTM!The CSS utility import is correctly added to support the new
css.toPxNumber()conversions required by the layout_* field migration.
127-196: LGTM!The
getLocalRectfunction has been correctly updated to work with the new schema:
- Uses
layout_positioning,layout_inset_*, andlayout_target_*fields- Text node handling properly updated to
tspantype with font-based size fallback- Leverages schema helper functions (
hasLayoutWidth,hasLayoutHeight) and CSS utilities (toPxNumber)The logic is sound and defensive programming practices are maintained.
300-308: LGTM!Helper functions
hasNumericAbsoluteBoxandisScaleTrackableNodeare correctly updated:
- Type annotations migrated to
grida.program.nodes.*types- Node type checks updated to
"tspan"- All geometry checks use new
layout_*prefixed fields- Logic preserved from the original implementation
Also applies to: 364-379
271-272: No action needed. Theio.snapshot.grida1zip.unpackAPI exists in the @grida/io package and correctly handles binary zip data. The fixture loading migration to the.grida1.zipformat is properly implemented.
| // TODO: don't skip | ||
| describe.skip("apply-scale round-trip (accuracy)", () => { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the test file structure around the skipped test
head -n 430 editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts | tail -n 30Repository: gridaco/grida
Length of output: 752
🏁 Script executed:
#!/bin/bash
# Check for other apply-scale test files and active tests
find editor/grida-canvas -name "*.test.ts" -o -name "*.spec.ts" | head -20Repository: gridaco/grida
Length of output: 1012
🏁 Script executed:
#!/bin/bash
# Search for active (non-skipped) tests related to apply-scale or scale operations
rg "describe\(|it\(" editor/grida-canvas/reducers/__tests__/ --type=ts -B 2 | head -100Repository: gridaco/grida
Length of output: 3351
🏁 Script executed:
#!/bin/bash
# Check if there are any other describe.skip patterns in the test file
rg "describe\.skip|it\.skip" editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts -nRepository: gridaco/grida
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Check the parametric-scale test for alternative coverage
cat -n editor/grida-canvas/reducers/schema/__tests__/schema.parametric-scale.test.ts | head -50Repository: gridaco/grida
Length of output: 1592
🏁 Script executed:
#!/bin/bash
# Look for fixture files to understand if they're missing
find . -name "*-*.grida1.zip" -o -name "*-grida1.zip" 2>/dev/null | head -20Repository: gridaco/grida
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Check the exact fixture path requirement
rg "FIXTURE_VERSION_SPECIFIER" editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts -B 2 -A 2Repository: gridaco/grida
Length of output: 542
🏁 Script executed:
#!/bin/bash
# Check if fixtures directory exists and what's in it
ls -la fixtures/test-grida/ 2>/dev/null || echo "fixtures/test-grida directory not found"Repository: gridaco/grida
Length of output: 307
Re-enable the skipped test suite.
The main roundtrip accuracy test suite is skipped with a TODO comment, but the required fixture (d1-20251209.grida1.zip) is already present. Without these tests running, scale accuracy regressions could go undetected during the schema migration, even though the comprehensive fixture-based validation is available. While schema-level unit tests provide some coverage, they do not replace the thorough roundtrip validation these fixture-based tests provide.
🤖 Prompt for AI Agents
In @editor/grida-canvas/reducers/__tests__/apply-scale.roundtrip.test.ts around
lines 416 - 417, Replace the skipped test suite declaration
describe.skip("apply-scale round-trip (accuracy)", ...) with
describe("apply-scale round-trip (accuracy)", ...) to re-enable the
fixture-based roundtrip accuracy tests; remove the TODO comment, ensure the
fixture d1-20251209.grida1.zip is present in the test fixtures used by the tests
(and update any fixture path references if they changed), and run the test
locally/CI to confirm the suite executes without additional timeout or
environment changes.
| if (manifest.document_file === "document.grida" && document) { | ||
| if (!document) return { kind: "unknown", bytes }; |
There was a problem hiding this comment.
Remove redundant null check.
Line 694 already verifies that document exists with the condition && document. The subsequent check on line 695 if (!document) is unreachable and redundant.
🔧 Proposed fix
- if (manifest.document_file === "document.grida" && document) {
- if (!document) return { kind: "unknown", bytes };
-
+ if (manifest.document_file === "document.grida" && document) {
const images: Record<string, Uint8Array> = {};📝 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.
| if (manifest.document_file === "document.grida" && document) { | |
| if (!document) return { kind: "unknown", bytes }; | |
| if (manifest.document_file === "document.grida" && document) { | |
| const images: Record<string, Uint8Array> = {}; |
🤖 Prompt for AI Agents
In @packages/grida-canvas-io/index.ts around lines 694 - 695, The conditional
block checks manifest.document_file === "document.grida" && document, so the
inner guard if (!document) return { kind: "unknown", bytes }; is unreachable and
should be removed; update the block around the manifest.document_file branch
(where the variable document and bytes are used) to rely on the outer existence
check and eliminate the redundant null check and return.
…e legacy and plural paint properties
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fixtures/test-grida/README.md (1)
5-54: Well-structured migration documentation.The README clearly documents the file format changes, provides comprehensive field rename mappings, and sets appropriate expectations for test fixtures in the context of a breaking schema change. The "File formats" section, naming convention clarification, and changelog with field mappings are all valuable additions that align well with the PR's schema migration objectives.
Consider adding a note about validating
.gridafiles using thetool_io_gridaCLI tool (if applicable). Based on learnings from the codebase, this can help contributors verify fixture integrity when updating or creating new test files.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fixtures/test-grida/README.mdpackages/grida-canvas-io/format.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-20T08:11:16.220Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.grida : Validate .grida files using the `tool_io_grida` CLI tool to check file structure, parse all nodes, and detect parsing errors
Applied to files:
fixtures/test-grida/README.md
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Applied to files:
fixtures/test-grida/README.md
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/scenario_*.rs : Scenario-specific comprehensive tests should be organized in files matching `scenario_*.rs`
Applied to files:
fixtures/test-grida/README.md
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to packages/grida-canvas-*/**/*.{ts,tsx,js,jsx} : Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Applied to files:
fixtures/test-grida/README.md
⏰ 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: Cursor Bugbot
- GitHub Check: test
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @editor/grida-canvas/editor.ts:
- Around line 2841-2850: The archive() method uses a redundant type assertion;
remove the unnecessary "as BlobPart" on the result of
io.archive.pack(this.getSnapshot().document) since io.archive.pack returns a
Uint8Array which is already a valid BlobPart. Update the archive()
implementation (method name: archive, call site:
io.archive.pack(this.getSnapshot().document)) to pass the returned Uint8Array
directly into new Blob([...]) and keep the MIME type unchanged.
🧹 Nitpick comments (2)
editor/grida-canvas/editor.ts (2)
758-784: TODO comment indicates incomplete refactoring.The comment states "TODO: rename to createTextSpanNode", but the method name remains
createTextNode. If this method is part of the public API surface, consider:
- Renaming it to
createTextSpanNodeto match the newTextSpanNodetype- Keeping the old name with a deprecation notice and aliasing to the new name for backward compatibility
- If renaming is deferred, update the TODO with a tracking issue reference
♻️ Proposed API evolution
If backward compatibility is required:
+ public createTextSpanNode( + text = "" + ): NodeProxy<grida.program.nodes.TextSpanNode> { + return this.createTextNode(text); + } + + /** + * @deprecated Use createTextSpanNode instead. This method will be removed in a future version. + */ - // TODO: rename to createTextSpanNode public createTextNode( text = "" ): NodeProxy<grida.program.nodes.TextSpanNode> {
2981-2987: Consider logging Safari fallback in debug mode.The Safari fallback for
device-pixel-content-boxobservation is silently applied without logging. While the fallback is correct, logging this in debug mode would help developers understand when and why the fallback is being used, especially since the observation behavior differs (logical pixels vs device pixels).♻️ Proposed enhancement for debugging
// Safari doesn't support the "device-pixel-content-box" box option try { ro.observe(el, { box: "device-pixel-content-box" }); } catch (e) { + this.log( + "ResizeObserver: falling back to default box (device-pixel-content-box not supported)" + ); // Fallback for browsers that don't support device-pixel-content-box (e.g., Safari) ro.observe(el); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
editor/grida-canvas/editor.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript 5 as the main language for most apps
Use Lucide or Radix Icons for icons
Files:
editor/grida-canvas/editor.ts
{editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Files:
editor/grida-canvas/editor.ts
editor/grida-*/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to /packages
Files:
editor/grida-canvas/editor.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to packages/grida-canvas-*/**/*.{ts,tsx,js,jsx} : Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.grida : Validate .grida files using the `tool_io_grida` CLI tool to check file structure, parse all nodes, and detect parsing errors
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/*.{js,ts,jsx,tsx} : Use Web Workers for heavy graphics operations to improve performance and responsiveness
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to {editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}} : Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Handle NodeId to UserNodeId conversion via `IdConverter` during .grida file loading
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `skia-safe` crate for painting operations in the rendering engine
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.220Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo fmt` to maintain code formatting standards
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/+(grida-canvas-wasm.js|grida-canvas-wasm.wasm) : Include WASM artifacts (`grida-canvas-wasm.js` and `grida-canvas-wasm.wasm`) in git for faster CI builds
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`
Applied to files:
editor/grida-canvas/editor.ts
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to packages/grida-canvas-*/**/*.{ts,tsx,js,jsx} : Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Applied to files:
editor/grida-canvas/editor.ts
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to {editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}} : Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Applied to files:
editor/grida-canvas/editor.ts
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : When adding new shape types, update the Shape type union, add cases in drawShape() function, add cases in shapeToSVG() function, and add SelectItem in UI
Applied to files:
editor/grida-canvas/editor.ts
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to editor/grida-*/**/*.{ts,tsx} : Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to <root>/packages
Applied to files:
editor/grida-canvas/editor.ts
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Implement the Blink (Chrome) font selection model in the core font selection pipeline
Applied to files:
editor/grida-canvas/editor.ts
⏰ 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: Cursor Bugbot
- GitHub Check: test
🔇 Additional comments (2)
editor/grida-canvas/editor.ts (2)
2491-2497: LGTM! New clips_content API added.The new
changeContainerNodeClipsContentmethod follows the established pattern for property change methods and provides a clean API for controlling container clipping behavior.
2528-2542: LGTM! Gap API updated to support independent axis control.The parameter type evolution from
numbertonumber | { layout_main_axis_gap, layout_cross_axis_gap }provides backward compatibility (single number sets both) while allowing independent control of main and cross axis gaps. Implementation correctly handles both cases.
| public archive(): Blob { | ||
| const documentData = { | ||
| version: "0.89.0-beta+20251219", | ||
| document: this.getSnapshot().document, | ||
| } satisfies io.JSONDocumentFileModel; | ||
|
|
||
| const blob = new Blob([io.archive.pack(documentData) as BlobPart], { | ||
| type: "application/zip", | ||
| }); | ||
| const blob = new Blob( | ||
| [io.archive.pack(this.getSnapshot().document) as BlobPart], | ||
| { | ||
| type: "application/zip", | ||
| } | ||
| ); | ||
|
|
||
| return blob; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for io.archive.pack definition and type
rg -n --type=ts "archive\.pack" editor/grida-canvas/Repository: gridaco/grida
Length of output: 157
🏁 Script executed:
# Find where io is imported or defined in editor.ts
rg -n --type=ts "import.*io|from.*io" editor/grida-canvas/editor.ts | head -20Repository: gridaco/grida
Length of output: 561
🏁 Script executed:
# Search for archive module/package
fd -n "archive" editor/ | grep -E "\.(ts|tsx|js)$"Repository: gridaco/grida
Length of output: 287
🏁 Script executed:
# Look for pack function definition more broadly
rg -n --type=ts "^\s*(export\s+)?(function\s+pack|const\s+pack\s*=)" --max-count=20Repository: gridaco/grida
Length of output: 560
🏁 Script executed:
# Check the io module structure and archive.pack specifically
sed -n '830,850p' packages/grida-canvas-io/index.tsRepository: gridaco/grida
Length of output: 874
🏁 Script executed:
# Look for the archive namespace/object definition
rg -n "archive\s*[=:{]" packages/grida-canvas-io/index.ts | head -10Repository: gridaco/grida
Length of output: 184
🏁 Script executed:
# Get more context around the pack function at line 835
sed -n '830,840p' packages/grida-canvas-io/index.tsRepository: gridaco/grida
Length of output: 488
Remove unnecessary type assertion as BlobPart.
The io.archive.pack() function explicitly returns Uint8Array (defined in packages/grida-canvas-io/index.ts), which is already compatible with BlobPart as an ArrayBufferView. The type assertion is redundant and obscures the actual type. Remove the assertion on line 2843.
🤖 Prompt for AI Agents
In @editor/grida-canvas/editor.ts around lines 2841 - 2850, The archive() method
uses a redundant type assertion; remove the unnecessary "as BlobPart" on the
result of io.archive.pack(this.getSnapshot().document) since io.archive.pack
returns a Uint8Array which is already a valid BlobPart. Update the archive()
implementation (method name: archive, call site:
io.archive.pack(this.getSnapshot().document)) to pass the returned Uint8Array
directly into new Blob([...]) and keep the MIME type unchanged.
1 participant
This PR aims to provide future proof document model with flatbuffers.
to make the schema suitable for evolving without breaking, this refactors..
renames
positionlayout_positioningleftlayout_inset_lefttoplayout_inset_toprightlayout_inset_rightbottomlayout_inset_bottomwidthlayout_target_widthheightlayout_target_heightlayoutlayout_modepadding_toplayout_padding_toppadding_rightlayout_padding_rightpadding_bottomlayout_padding_bottompadding_leftlayout_padding_leftdirectionlayout_directionmain_axis_alignmentlayout_main_axis_alignmentcross_axis_alignmentlayout_cross_axis_alignmentmain_axis_gaplayout_main_axis_gapcross_axis_gaplayout_cross_axis_gaprotationpost_layout_transformfeatures
Note
Format and schema migration
.grida1format and bumps schema to0.90.0-beta+20260108position/left/top/right/bottom/width/height/layout/*withlayout_*fields, usestspanfor text, and addsclips_contentdemo.grida1,rectangle.grida1) andexample/main.jsfetch pathBuild tooling
bin/activate-flatcto fetch/verify/cache a pinned FlatBuffersflatcbinary and exportFLATC/PATHfor CI/local buildsMisc
.grida1with JSON in VS Code settingsformat/)Written by Cursor Bugbot for commit 9b9fa76. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.