SVG import: text, gradients, nested transforms, and SVG-to-.grida FlatBuffers pipeline#587
Merged
softmarshmallow merged 32 commits intomainfrom Mar 19, 2026
Merged
SVG import: text, gradients, nested transforms, and SVG-to-.grida FlatBuffers pipeline#587softmarshmallow merged 32 commits intomainfrom
softmarshmallow merged 32 commits intomainfrom
Conversation
- Introduced a new SKILL.md file detailing the design and review process for computer-graphics rendering tests, specifically focusing on reftests and golden tests. - Provided guidelines on when to use each type of test, including terminology and core decision rules for selecting between reftests and golden tests. - Included test design guidance to ensure effective and reliable visual comparison tests, enhancing the overall testing framework for the Grida project.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
WalkthroughRefactors SVG import from a JSON/IR pack to Rust-produced FlatBuffers bytes, replacing svgPack with svgToDocument across WASM, Rust, and editor layers; adopts chunk-based text import, fixes gradient/bbox normalization, updates layout/geometry resolution, removes the Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
- Updated the mouse wheel event handling to support zooming in and out using Cmd/Ctrl + scroll gestures, improving user experience on macOS and other platforms. - Maintained existing panning functionality for standard scroll actions, ensuring consistent behavior across different input methods. - Introduced sensitivity adjustments for zooming to provide a smoother experience when navigating through content.
- Added a variety of new prompts for single vector and SVG illustrations, including designs for logos, icons, and promotional materials. - Enhanced the creative options available for users, catering to diverse design needs such as minimalistic logos, flat illustrations, and typographic posters. - Improved the overall resource for generating visual content, supporting a wider range of artistic expressions.
- Modified the GitHub Actions workflow to save cache only when the branch is 'main', optimizing cache usage. - Removed the option to add a Rust environment hash key, streamlining the caching process for Rust crates.
softmarshmallow
changed the title
- Expanded the SKILL.md file to incorporate details about observation-based probe tests, including their design rules and conventions. - Clarified the distinctions between reftests, golden tests, and probe tests, enhancing the guidance for visual comparison testing. - Added new sections on probe test verification and future tooling, improving the overall documentation for rendering tests in the Grida project.
- Expanded SKILL.md to include definitions for data tests and their use cases, emphasizing assertions on scene graphs and computed values without rendering. - Clarified the hierarchy of testing methods, prioritizing data tests over reftests, probe tests, and golden tests for efficiency. - Updated decision rules and when to use each test type, improving guidance for developers on selecting appropriate testing strategies in rendering.
- Enhanced the GeometryCache to correctly apply world transforms for nodes under Group parents, ensuring accurate positioning when the layout engine is involved. - Added logic to differentiate between layout containers and non-layout nodes, preserving transformations for nodes that do not participate in layout. - Introduced new tests to verify the correctness of world transforms for nested groups and their children, ensuring reliable rendering of SVG structures.
- Eliminated the condition to save cache only for the 'main' branch in the GitHub Actions workflow, simplifying the caching logic. - This change aims to enhance cache management for all branches, improving build efficiency across different development workflows.
![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: f655aff1ec
ℹ️ 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".
Comment on lines
+712
to
+713
| // Decode FBS bytes into the editor's full document model | ||
| const fullDoc = io.GRID.decode(bytes); |
There was a problem hiding this comment.
createNodeFromSvg() now decodes the Rust-generated .grida bytes via io.GRID.decode(), but that decoder still drops non-rotational group transforms. packages/grida-canvas-io/format.ts:5413-5439 only reconstructs a group from layoutFields, and the newly added note there explicitly says post_layout_transform scale/skew are lost. As a result, any imported SVG that uses scaled or skewed <g> elements will look correct in the Rust/FBS round-trip but arrive in the editor without those transforms.
Useful? React with 👍 / 👎.
Comment on lines
+266
to
+270
| // Apply from_bbox_inv = [1/w, 0, -x/w, 0, 1/h, -y/h] | ||
| // as a left-multiply: result = from_bbox_inv * transform | ||
| let inv_w = 1.0 / width; | ||
| let inv_h = 1.0 / height; | ||
| transform.matrix[0][0] *= inv_w; | ||
| transform.matrix[0][1] *= inv_w; | ||
| transform.matrix[0][2] *= inv_w; | ||
| transform.matrix[1][0] *= inv_h; | ||
| transform.matrix[1][1] *= inv_h; | ||
| transform.matrix[1][2] *= inv_h; | ||
| let m = &transform.matrix; |
There was a problem hiding this comment.
For objectBoundingBox gradients, usvg has already post-concatenated the bbox matrix onto the gradient transform, so removing it needs transform * from_bbox_inv. This code applies from_bbox_inv * transform instead, which is why the new paint-linear-gradient-01 cross-boundary check still has to be marked it.fails: even a plain horizontal gradient keeps a stray transform and renders shifted/skewed on imported shapes.
Useful? React with 👍 / 👎.
Comment on lines
202
to
+204
| for chunk in text.chunks() { | ||
| let offset_x = chunk.x().unwrap_or(0.0); | ||
| let offset_y = chunk.y().unwrap_or(0.0); | ||
| let chunk_text = chunk.text(); | ||
| for span in chunk.spans() { | ||
| if !span.is_visible() { | ||
| continue; | ||
| } | ||
| let start = span.start(); | ||
| let end = span.end().min(chunk_text.len()); | ||
| if start >= end { | ||
| continue; | ||
| } | ||
| let span_slice = &chunk_text[start..end]; | ||
| if span_slice.trim().is_empty() { | ||
| continue; | ||
| } | ||
| let mut span_affine: AffineTransform = relative.into(); | ||
| if offset_x != 0.0 || offset_y != 0.0 { | ||
| span_affine.translate(offset_x, offset_y); | ||
| } | ||
|
|
||
| let span_fill = span.fill().map(SVGFillAttributes::from); | ||
| let span_stroke = span.stroke().map(SVGStrokeAttributes::from); | ||
|
|
||
| spans.push(IRSVGTextSpanNode { | ||
| transform: CGTransform2D::from(span_affine), | ||
| text: span_slice.to_string(), | ||
| fill: span_fill, | ||
| stroke: span_stroke, | ||
| font_size: Some(span.font_size().get()), | ||
| anchor: chunk.anchor().into(), | ||
| }); | ||
| let chunk_text = chunk.text().trim(); | ||
|
|
There was a problem hiding this comment.
Calling trim() here removes leading/trailing spaces that are part of the SVG text payload. In common cases like Hello <tspan>world</tspan> or xml:space="preserve", usvg will hand us a chunk ending or starting with a space; trimming it turns Hello world into Helloworld and also makes the stored text disagree with the character positions used for dx/dy.
Useful? React with 👍 / 👎.
Comment on lines
+177
to
+181
| span.text.as_str(), | ||
| span.fill.as_ref().or(text.fill.as_ref()), | ||
| span.stroke.as_ref().or(text.stroke.as_ref()), | ||
| span.font_size, | ||
| &text.bounds, |
There was a problem hiding this comment.
In the multi-chunk path every child span is passed the parent <text> bounds. append_text_span_node() later computes text-anchor by subtracting bounds.width, so multiline or multi-tspan text with text-anchor="middle"/"end" shifts every line by the same overall width instead of each chunk's width. The new text-anchor.svg fixture includes absolute-positioned tspans that depend on per-line anchoring, so those imports will be visibly misaligned.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
editor/grida-canvas/editor.i.ts (1)
429-441:⚠️ Potential issue | 🟠 MajorFix typo-lint blockers called out by CI.
Line 429 (
MOVEMNT) and Line 441 (applyed) are currently failing thetyposworkflow. Please correct them (or add an explicit lint allowlist if intentional) to unblock CI.Minimal text fixes
- * const threshold = Math.ceil(DEFAULT_SNAP_MOVEMNT_THRESHOLD_FACTOR / zoom); + * const threshold = Math.ceil(DEFAULT_SNAP_MOVEMENT_THRESHOLD_FACTOR / zoom); ... - * snap threshold applyed when nudge (fake gesture) is applied + * snap threshold applied when nudge (fake gesture) is applied🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/editor.i.ts` around lines 429 - 441, The exported symbol DEFAULT_SNAP_MOVEMNT_THRESHOLD_FACTOR and a nearby comment "applyed" contain typos; rename the symbol to DEFAULT_SNAP_MOVEMENT_THRESHOLD_FACTOR and update every usage/import of DEFAULT_SNAP_MOVEMNT_THRESHOLD_FACTOR across the codebase (or add a deprecated alias if you must preserve compatibility), and correct the comment word "applyed" to "applied" (in the comment starting "snap threshold applyed when nudge..."); ensure tests/linters pass after updating references.crates/grida-canvas/tests/svg_pack.rs (1)
88-100:⚠️ Potential issue | 🟡 MinorThis test never asserts anything.
Both branches pass, and this also bypasses the sanitized production path used by
assert_pack_ok(). As written, CI won’t tell you whether no-xmlnshandling regressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/svg_pack.rs` around lines 88 - 100, The test currently doesn't assert anything because it matches Ok/Err and silently continues; replace the match with a real assertion so CI catches regressions — call the existing assert_pack_ok helper (or assert!(result.is_ok(), "...")) instead of the match, referencing SVGPackedScene::new_from_svg_str to obtain the result and use assert_pack_ok(svg) (or assert!(result.is_ok(), "no-xmlns SVG must be accepted")) so the sanitized production path and actual assertion are exercised.
🧹 Nitpick comments (6)
crates/grida-dev/src/platform/native_application.rs (1)
48-51: Consider extracting magic numbers as named constants.The values
16.0(line-to-pixel approximation) and0.002(zoom sensitivity) are reasonable heuristics, but naming them would clarify intent and make future tuning easier.✨ Optional: extract constants
+const PIXELS_PER_LINE: f32 = 16.0; +const SCROLL_ZOOM_SENSITIVITY: f32 = 0.002; + fn handle_window_event( event: &WindowEvent, modifiers: &winit::keyboard::ModifiersState, ) -> ApplicationCommand { match event { // ... WindowEvent::MouseWheel { delta, .. } => { if modifiers.super_key() || modifiers.control_key() { let dy = match delta { MouseScrollDelta::PixelDelta(d) => d.y as f32, - MouseScrollDelta::LineDelta(_, y) => *y * 16.0, + MouseScrollDelta::LineDelta(_, y) => *y * PIXELS_PER_LINE, }; - let sensitivity: f32 = 0.002; - let zoom_delta = dy * sensitivity; + let zoom_delta = dy * SCROLL_ZOOM_SENSITIVITY; ApplicationCommand::ZoomDelta { delta: zoom_delta }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/platform/native_application.rs` around lines 48 - 51, Extract the magic numbers into named constants and use them in the scroll->zoom calculation: replace the literal 16.0 used in the MouseScrollDelta::LineDelta branch with a const like LINE_DELTA_TO_PIXELS: f32 = 16.0 and replace the 0.002 assigned to sensitivity with a const like ZOOM_SENSITIVITY: f32 = 0.002; then compute dy and zoom_delta using those constants (the code around MouseScrollDelta::LineDelta, the sensitivity variable, and zoom_delta should reference LINE_DELTA_TO_PIXELS and ZOOM_SENSITIVITY) so intent is clear and tuning is easier.crates/grida-canvas/src/cache/geometry.rs (1)
495-504: Consider extracting the parent check into a helper.The
parent_is_layout_containerlogic is duplicated between TextSpan (lines 401-410) and leaf nodes (lines 495-504). A small helper function would reduce duplication.♻️ Extract helper function
+fn is_layout_container_parent( + parent_id: &Option<NodeId>, + graph: &SceneGraph, +) -> bool { + parent_id + .as_ref() + .and_then(|pid| graph.get_node(pid).ok()) + .map(|parent_node| { + matches!(parent_node, Node::Container(_) | Node::InitialContainer(_)) + }) + .unwrap_or(false) +}Then use
is_layout_container_parent(&parent_id, graph)in both branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/cache/geometry.rs` around lines 495 - 504, The parent_is_layout_container check is duplicated between TextSpan and leaf node handling; extract this into a small helper function (e.g., is_layout_container_parent) that takes (&Option<Id>, &Graph) and returns a bool by performing the current logic of checking parent_id.as_ref().and_then(|pid| graph.get_node(pid).ok()).map(|parent_node| matches!(parent_node, Node::Container(_) | Node::InitialContainer(_))). Replace the duplicated blocks in TextSpan handling and the leaf node branch with calls to is_layout_container_parent(&parent_id, graph) to remove duplication and improve readability.packages/grida-tree/src/lib.ts (1)
2382-2396: Input node objects are mutated when updatingidproperty.The function creates a new
nodesrecord but reuses the original node objects. Whennode.idis updated (line 2393), it mutates the inputsubgraph.nodesentries. This may be intentional for performance, but it's surprising given the function appears to produce a "fresh-key version."Consider either:
- Documenting this mutation behavior in the JSDoc
- Or shallow-copying the node before mutation if immutability is expected
Option A: Document the mutation
* `@param` keygen - Function that generates a fresh key for each old key * `@returns` `{ graph, roots, keyMap }` with all keys replaced + * `@remarks` This function mutates `node.id` on input nodes that have a string `id` property. *Option B: Shallow-copy to avoid mutation
// 2. Remap nodes const nodes: Record<tree.Key, T> = {}; for (const [oldKey, node] of Object.entries(subgraph.nodes)) { const newKey = keyMap[oldKey]!; + let remappedNode = node; // If the node has an `id` property, update it if ( node && typeof node === "object" && "id" in (node as Record<string, unknown>) && typeof (node as Record<string, unknown>).id === "string" ) { - (node as Record<string, unknown>).id = newKey; + remappedNode = { ...node, id: newKey } as T; } - nodes[newKey] = node as T; + nodes[newKey] = remappedNode; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-tree/src/lib.ts` around lines 2382 - 2396, The code currently mutates input objects from subgraph.nodes when updating their id; to avoid surprising side-effects, shallow-copy each node before modifying its id (e.g., create a newObj from node via Object.assign({}, node) or {...node}), set newObj.id = newKey, and assign nodes[newKey] = newObj; locate the logic that iterates over subgraph.nodes, the variables node/newKey/keyMap/nodes, and replace the in-place id mutation with a shallow-copy-and-modify approach (or alternatively, if mutation is intended, update the function JSDoc to explicitly state that subgraph.nodes entries are mutated).crates/grida-canvas/src/io/io_svg.rs (1)
24-52: Avoid a fixed-widtha{:06}ordering key here.These position strings are consumed lexicographically during decode. Once
counterreaches1_000_000,a1000000sorts beforea999999, so very large imports can rebuild siblings in the wrong order. Reusing the encoder’s fractional-position generator would keep both code paths aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/io/io_svg.rs` around lines 24 - 52, The current walk_tree_order builds position_map entries using a fixed-width string format "a{:06}" which will break lexicographic ordering once counter >= 1_000_000; change walk_tree_order to stop using a fixed-width decimal padding and instead call the same fractional-position generator used by the encoder (or central utility that produces monotonic lexicographic position keys) to generate position_map values for each NodeId, keeping id_map creation (format!("svg_{}", node_id)) intact and ensuring walk_tree_order, position_map, and any code that consumes these keys remain compatible with the encoder's position format.crates/grida-canvas/tests/svg_pack.rs (1)
1186-1408: These are encode/decode tests, not true round-trips.Every case stops at
io_grida_fbs::decode(&bytes), so none of them exercise re-encoding with the decodedid_mapandposition_map. If the goal is to protect.gridacompatibility, switch this block todecode_with_id_map()plus a secondencode(); otherwise rename the tests so the coverage is explicit.Based on learnings: Treat changes to
format/grida.fbs(FlatBuffers schema) like public API changes—they ripple into Rust and TypeScript codecs and eventually into real user files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/svg_pack.rs` around lines 1186 - 1408, The tests currently only call svg_to_grida_bytes(...) then io_grida_fbs::decode(&bytes) which verifies decoding but not re-encoding; update each test that intends a full round-trip to call io_grida_fbs::decode_with_id_map(&bytes) to obtain (scene, id_map, position_map) and then call io_grida_fbs::encode(&scene, &id_map, &position_map) (or the crate's equivalent encode function) and assert the resulting bytes/structure (or byte-validity) to complete the encode→decode→encode roundtrip; alternatively, if you only want to test decode behavior, rename the tests (e.g., append _decode_only) to make coverage explicit.editor/grida-canvas/editor.ts (1)
724-758: This manual document rebuild is already lossy.
io.GRID.decode(bytes)gives you the full imported document, but this path reconstructs a newIPackedSceneDocumentand hard-codesimages,bitmaps, andpropertiesto{}. A dedicated helper that strips the wrapper scene while preserving the rest of the decoded payload would be safer than re-encoding the schema shape here by hand.Based on learnings: Treat changes to
format/grida.fbs(FlatBuffers schema) like public API changes—they ripple into Rust and TypeScript codecs and eventually into real user files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/editor.ts` around lines 724 - 758, The current import rebuild manually constructs an IPackedSceneDocument (packedDoc) and blanks images/bitmaps/properties which causes data loss; instead use the full decoded payload from io.GRID.decode(bytes) and strip only the wrapper scene while preserving its nodes, links, images, bitmaps and properties, then remap IDs via tree.graph.remapKeys(...) using this preserved payload and idgen.next(); implement or call a helper (e.g., create a helper like stripWrapperSceneFromDecoded(decoded): IPackedSceneDocument) and replace the packedDoc construction with that helper so remapKeys and subsequent code operate on the complete decoded document rather than hard-coded empty maps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-canvas-wasm/lib/modules/svg.ts`:
- Around line 92-116: The toDocument method currently only frees the WASM result
buffer on the success path; change it so any non-zero pointer returned by
_grida_svg_to_document is always deallocated even if an exception occurs. After
calling this.module._grida_svg_to_document(svgPtr) capture the resultPtr in a
variable visible to the finally block, and in finally call
this.module._deallocate(resultPtr, 4 + len) (or deallocate just the 4-byte
prefix + payload) when resultPtr is non-zero; keep freeing the input string from
_alloc_string (svgPtr) as before. Ensure resultPtr and len are
initialized/guarded so the finally block can safely skip deallocation if
resultPtr is null/0 or len is unknown.
In `@crates/grida-canvas/src/svg/pack.rs`:
- Around line 175-183: The code is reusing the full text block bounds for every
span causing incorrect anchor shifts; update the call to append_text_span_node
so it uses per-span bounds (compute or pass span-specific bounds instead of
&text.bounds) or, if per-span bounds are not yet available, disable the
anchor-based shift for chunked spans inside append_text_span_node (detect when
multiple spans are present and skip the extra x-shift). Make the change around
the append_text_span_node invocation and ensure the function signature/logic
(append_text_span_node) accepts and uses those per-span bounds (or a flag to
skip anchor shift) so middle/end alignment uses each span’s own width rather
than text.bounds.
In `@crates/grida-dev/src/main.rs`:
- Around line 156-170: The loop that writes outputs keys them only by
file_stem() (see the use of path.file_stem(), name, and
output_dir.join(format!("{}.grida", name))), which causes collisions for
identically named files from different subdirectories; to fix, compute each
SVG's path relative to the input base and use that relative path (either
mirrored as subdirectories under output_dir or safely normalized into a unique
filename) when creating the output path, or alternatively scan svgs first to
detect duplicate stems and abort with a clear error; update the code that reads
path.file_stem() and builds out to use the relative path or duplicate-check
logic so writes no longer silently overwrite each other.
In `@docs/wg/feat-svg/pattern.md`:
- Around line 211-218: Update the checklist to require that any change to
format/grida.fbs is accompanied by corresponding updates to the runtime and
document models: modify the Rust runtime model at
crates/grida-canvas/src/node/schema.rs and the TypeScript document model at
packages/grida-canvas-schema/grida.ts when adding the new pattern paint (e.g.,
SVGPatternPaint / Paint::Pattern) so schema, runtime, and TS models stay in
sync; add a short guidance sentence like: "When modifying format/grida.fbs,
update the Rust runtime model at crates/grida-canvas/src/node/schema.rs and the
TS document model at packages/grida-canvas-schema/grida.ts together."
In `@docs/wg/feat-svg/text-import.md`:
- Around line 1-4: Add a YAML frontmatter block containing "format: md" at the
very top of the document that begins with the heading "# SVG Text Import Model"
so the page opts out of MDX parsing; ensure the frontmatter is the first thing
in the file (e.g., ---\nformat: md\n---) and that no MDX-specific syntax is
required elsewhere in the document.
- Around line 53-60: The fenced diagram block in docs/wg/feat-svg/text-import.md
is missing a language tag causing markdownlint MD040; update the opening fence
for the diagram (the triple-backtick block that contains the "SVG Grida Scene
Graph" ASCII diagram) to include an explicit language identifier such as text
(i.e., change ``` to ```text) so the fenced diagram is properly annotated.
In `@editor/app/`(dev)/canvas/tools/io-svg/page.tsx:
- Around line 263-272: The effect clears document JSON on failed conversion but
doesn't update the mounted canvas, leaving the last import visible; after the
checks where setDocumentJson(undefined) is called (in the branches around
useEffect and after editorInstance.svgToDocument(raw) returning falsy), also
clear the mounted editor via editorInstance by loading an empty document or
calling its clear method (e.g., editorInstance.loadDocument(undefined) or
editorInstance.clear()) so the canvas and text panes stay in sync when raw is
empty or conversion fails.
- Around line 261-286: The hook currently sets result and packedScene to the
same stringified JSON (documentJson); instead, keep result as the decoded
document object and packedScene as the pre-rendered JSON string: after decoding
bytes with io.GRID.decode (used in editorInstance.svgToDocument -> bytes ->
io.GRID.decode), set a state for the raw document object (e.g., documentObj) and
a separate state for the JSON string (documentJson), set documentObj to the
decoded doc and documentJson to JSON.stringify(doc, null, 2), and return {
result: documentObj, packedScene: documentJson } so callers that
JSON.stringify(result) receive the actual object.
In `@editor/grida-canvas/editor.i.ts`:
- Around line 2705-2719: The interface docs are misleading: update the comment
for IDocumentSVGInterfaceProvider.svgToDocument (and the top summary) to state
that svgToDocument returns FlatBuffers bytes (Uint8Array | null) rather than an
IPackedSceneDocument directly, and note that callers must decode the bytes using
io.GRID.decode to obtain an IPackedSceneDocument; also adjust the earlier
summary that currently claims svgToDocument produces an IPackedSceneDocument to
reflect the actual return contract and mention svgOptimize similarly if needed.
In `@editor/grida-canvas/editor.ts`:
- Around line 760-769: The code currently picks the root via ids[0], which is
unsafe because insert() returns all created node ids in diff order; instead
obtain the imported root id from the remapping produced during the import (use
remappedRoots[0] or the value that maps the original packedDoc root to the new
id) and use that as rootId when calling getNodeById; update the root selection
logic in the SVG import path (replace the ids[0] usage with the remapped root
lookup) and keep the existing error check and return of
getNodeById<grida.program.nodes.ContainerNode>(rootId).
In `@packages/grida-canvas-io-svg/lib.ts`:
- Around line 276-301: makeTextSpan() is using each span's absolute transform
for layout_inset_* which double-applies the parent text group's translation
(textPosition); change it to use local offsets by subtracting the parent
textPosition from the span's translation (e.g., localLeft = spanPosition.left -
textPosition.left, localTop = spanPosition.top - textPosition.top) and assign
those local values to layout_inset_left/top. Apply the same adjustment to the
analogous span-handling block around lines 331-349.
In `@packages/grida-canvas-io/format.ts`:
- Around line 2289-2294: The decoder's fallback coordinates for gradient
endpoints (xy1 and xy2) differ from the encoder's defaults, causing mismatched
gradient orientation; update the decoder logic that assigns xy1 (currently using
xy1Obj ? ... : [0, 0.5]) and xy2 (xy2Obj ? ... : [1, 0.5]) to use the same
defaults the encoder writes (xy1 = [0, 0] and xy2 = [1, 0]) so decoded gradients
match encoded ones even when fields are missing.
---
Outside diff comments:
In `@crates/grida-canvas/tests/svg_pack.rs`:
- Around line 88-100: The test currently doesn't assert anything because it
matches Ok/Err and silently continues; replace the match with a real assertion
so CI catches regressions — call the existing assert_pack_ok helper (or
assert!(result.is_ok(), "...")) instead of the match, referencing
SVGPackedScene::new_from_svg_str to obtain the result and use
assert_pack_ok(svg) (or assert!(result.is_ok(), "no-xmlns SVG must be
accepted")) so the sanitized production path and actual assertion are exercised.
In `@editor/grida-canvas/editor.i.ts`:
- Around line 429-441: The exported symbol DEFAULT_SNAP_MOVEMNT_THRESHOLD_FACTOR
and a nearby comment "applyed" contain typos; rename the symbol to
DEFAULT_SNAP_MOVEMENT_THRESHOLD_FACTOR and update every usage/import of
DEFAULT_SNAP_MOVEMNT_THRESHOLD_FACTOR across the codebase (or add a deprecated
alias if you must preserve compatibility), and correct the comment word
"applyed" to "applied" (in the comment starting "snap threshold applyed when
nudge..."); ensure tests/linters pass after updating references.
---
Nitpick comments:
In `@crates/grida-canvas/src/cache/geometry.rs`:
- Around line 495-504: The parent_is_layout_container check is duplicated
between TextSpan and leaf node handling; extract this into a small helper
function (e.g., is_layout_container_parent) that takes (&Option<Id>, &Graph) and
returns a bool by performing the current logic of checking
parent_id.as_ref().and_then(|pid| graph.get_node(pid).ok()).map(|parent_node|
matches!(parent_node, Node::Container(_) | Node::InitialContainer(_))). Replace
the duplicated blocks in TextSpan handling and the leaf node branch with calls
to is_layout_container_parent(&parent_id, graph) to remove duplication and
improve readability.
In `@crates/grida-canvas/src/io/io_svg.rs`:
- Around line 24-52: The current walk_tree_order builds position_map entries
using a fixed-width string format "a{:06}" which will break lexicographic
ordering once counter >= 1_000_000; change walk_tree_order to stop using a
fixed-width decimal padding and instead call the same fractional-position
generator used by the encoder (or central utility that produces monotonic
lexicographic position keys) to generate position_map values for each NodeId,
keeping id_map creation (format!("svg_{}", node_id)) intact and ensuring
walk_tree_order, position_map, and any code that consumes these keys remain
compatible with the encoder's position format.
In `@crates/grida-canvas/tests/svg_pack.rs`:
- Around line 1186-1408: The tests currently only call svg_to_grida_bytes(...)
then io_grida_fbs::decode(&bytes) which verifies decoding but not re-encoding;
update each test that intends a full round-trip to call
io_grida_fbs::decode_with_id_map(&bytes) to obtain (scene, id_map, position_map)
and then call io_grida_fbs::encode(&scene, &id_map, &position_map) (or the
crate's equivalent encode function) and assert the resulting bytes/structure (or
byte-validity) to complete the encode→decode→encode roundtrip; alternatively, if
you only want to test decode behavior, rename the tests (e.g., append
_decode_only) to make coverage explicit.
In `@crates/grida-dev/src/platform/native_application.rs`:
- Around line 48-51: Extract the magic numbers into named constants and use them
in the scroll->zoom calculation: replace the literal 16.0 used in the
MouseScrollDelta::LineDelta branch with a const like LINE_DELTA_TO_PIXELS: f32 =
16.0 and replace the 0.002 assigned to sensitivity with a const like
ZOOM_SENSITIVITY: f32 = 0.002; then compute dy and zoom_delta using those
constants (the code around MouseScrollDelta::LineDelta, the sensitivity
variable, and zoom_delta should reference LINE_DELTA_TO_PIXELS and
ZOOM_SENSITIVITY) so intent is clear and tuning is easier.
In `@editor/grida-canvas/editor.ts`:
- Around line 724-758: The current import rebuild manually constructs an
IPackedSceneDocument (packedDoc) and blanks images/bitmaps/properties which
causes data loss; instead use the full decoded payload from
io.GRID.decode(bytes) and strip only the wrapper scene while preserving its
nodes, links, images, bitmaps and properties, then remap IDs via
tree.graph.remapKeys(...) using this preserved payload and idgen.next();
implement or call a helper (e.g., create a helper like
stripWrapperSceneFromDecoded(decoded): IPackedSceneDocument) and replace the
packedDoc construction with that helper so remapKeys and subsequent code operate
on the complete decoded document rather than hard-coded empty maps.
In `@packages/grida-tree/src/lib.ts`:
- Around line 2382-2396: The code currently mutates input objects from
subgraph.nodes when updating their id; to avoid surprising side-effects,
shallow-copy each node before modifying its id (e.g., create a newObj from node
via Object.assign({}, node) or {...node}), set newObj.id = newKey, and assign
nodes[newKey] = newObj; locate the logic that iterates over subgraph.nodes, the
variables node/newKey/keyMap/nodes, and replace the in-place id mutation with a
shallow-copy-and-modify approach (or alternatively, if mutation is intended,
update the function JSDoc to explicitly state that subgraph.nodes entries are
mutated).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d3036b9-048e-4f23-8ee3-0ffc6045316b
⛔ Files ignored due to path filters (39)
crates/grida-canvas-wasm/lib/bin/grida_canvas_wasm.wasmis excluded by!**/*.wasmcrates/grida-canvas/src/io/generated/grida.rsis excluded by!**/generated/**fixtures/test-svg/L0/basic-shapes.svgis excluded by!**/*.svgfixtures/test-svg/L0/clip-paths.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feColorMatrix.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feComponentTransfer.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feComposite.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feConvolveMatrix.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feDiffuseLighting.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feDisplacementMap.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feDropShadow.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feFlood.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feGaussianBlur.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feMorphology.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feSpecularLighting.svgis excluded by!**/*.svgfixtures/test-svg/L0/filters-feTurbulence.svgis excluded by!**/*.svgfixtures/test-svg/L0/gradients.svgis excluded by!**/*.svgfixtures/test-svg/L0/groups.svgis excluded by!**/*.svgfixtures/test-svg/L0/masks.svgis excluded by!**/*.svgfixtures/test-svg/L0/opacity.svgis excluded by!**/*.svgfixtures/test-svg/L0/paint-linear-gradient-01.svgis excluded by!**/*.svgfixtures/test-svg/L0/paint-linear-gradient-vertical-01.svgis excluded by!**/*.svgfixtures/test-svg/L0/paint-radial-gradient-01.svgis excluded by!**/*.svgfixtures/test-svg/L0/paints.svgis excluded by!**/*.svgfixtures/test-svg/L0/paths.svgis excluded by!**/*.svgfixtures/test-svg/L0/patterns.svgis excluded by!**/*.svgfixtures/test-svg/L0/shapes.svgis excluded by!**/*.svgfixtures/test-svg/L0/stroke-dasharray.svgis excluded by!**/*.svgfixtures/test-svg/L0/strokes.svgis excluded by!**/*.svgfixtures/test-svg/L0/text-anchor.svgis excluded by!**/*.svgfixtures/test-svg/L0/text-generic-fonts.svgis excluded by!**/*.svgfixtures/test-svg/L0/text-in-transformed-group.svgis excluded by!**/*.svgfixtures/test-svg/L0/text-tspan.svgis excluded by!**/*.svgfixtures/test-svg/L0/texts.svgis excluded by!**/*.svgfixtures/test-svg/L0/transforms-nested.svgis excluded by!**/*.svgfixtures/test-svg/L0/transforms.svgis excluded by!**/*.svgfixtures/test-svg/L0/use-defs.svgis excluded by!**/*.svgfixtures/test-svg/L0/viewbox-aspect-ratio.svgis excluded by!**/*.svgpackages/grida-canvas-io-svg/__tests__/test.svgis excluded by!**/*.svg
📒 Files selected for processing (37)
.agents/skills/cg-reftest/SKILL.md.github/workflows/test-crates.ymlcrates/grida-canvas-wasm/lib/bin/grida-canvas-wasm.jscrates/grida-canvas-wasm/lib/index.tscrates/grida-canvas-wasm/lib/modules/svg-bindings.d.tscrates/grida-canvas-wasm/lib/modules/svg.tscrates/grida-canvas-wasm/package.jsoncrates/grida-canvas-wasm/src/wasm_svg.rscrates/grida-canvas/examples/tool_svg_batch.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/cg/svg.rscrates/grida-canvas/src/io/io_grida.rscrates/grida-canvas/src/io/io_grida_fbs.rscrates/grida-canvas/src/io/io_svg.rscrates/grida-canvas/src/svg/from_usvg_tree.rscrates/grida-canvas/src/svg/pack.rscrates/grida-canvas/tests/svg_pack.rscrates/grida-dev/src/main.rscrates/grida-dev/src/platform/native_application.rsdocs/wg/feat-svg/pattern.mddocs/wg/feat-svg/text-import.mddocs/wg/research/chromium/index.mddocs/wg/research/chromium/svg-pattern.mdeditor/app/(dev)/canvas/tools/io-svg/page.tsxeditor/grida-canvas/backends/wasm.tseditor/grida-canvas/editor.i.tseditor/grida-canvas/editor.tsfixtures/prompts/canvas-user-prompts.txtfixtures/test-svg/.gitignoreformat/grida.fbspackages/grida-canvas-cg/lib.tspackages/grida-canvas-io-svg/__tests__/convert.test.tspackages/grida-canvas-io-svg/__tests__/fbs-cross-boundary.test.tspackages/grida-canvas-io-svg/__tests__/svgson.example.jsonpackages/grida-canvas-io-svg/lib.tspackages/grida-canvas-io/format.tspackages/grida-tree/src/lib.ts
💤 Files with no reviewable changes (3)
- crates/grida-canvas-wasm/lib/index.ts
- packages/grida-canvas-io-svg/tests/svgson.example.json
- .github/workflows/test-crates.yml
Comment on lines
+156
to
+170
| for path in &svgs { | ||
| let name = path.file_stem().unwrap().to_string_lossy(); | ||
| let svg = match std::fs::read_to_string(path) { | ||
| Ok(s) => s, | ||
| Err(e) => { | ||
| eprintln!(" SKIP {} — read: {}", name, e); | ||
| skip += 1; | ||
| continue; | ||
| } | ||
| }; | ||
| match svg_to_grida_bytes(&svg) { | ||
| Ok(bytes) => { | ||
| let out = output_dir.join(format!("{}.grida", name)); | ||
| std::fs::write(&out, &bytes).expect("write"); | ||
| ok += 1; |
There was a problem hiding this comment.
Don't key outputs only by file_stem() in recursive mode.
In recursive mode, foo/icon.svg and bar/icon.svg both target icon.grida. The later write silently overwrites the earlier one, so some fixtures disappear from .generated and the cross-boundary suite ends up exercising whichever file happened to win last. Either reject duplicate stems up front or plumb relative paths through the generator and reader together.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/main.rs` around lines 156 - 170, The loop that writes
outputs keys them only by file_stem() (see the use of path.file_stem(), name,
and output_dir.join(format!("{}.grida", name))), which causes collisions for
identically named files from different subdirectories; to fix, compute each
SVG's path relative to the input base and use that relative path (either
mirrored as subdirectories under output_dir or safely normalized into a unique
filename) when creating the output path, or alternatively scan svgs first to
detect duplicate stems and abort with a clear error; update the code that reads
path.file_stem() and builds out to use the relative path or duplicate-check
logic so writes no longer silently overwrite each other.
Comment on lines
+211
to
+218
| | File | Change | | ||
| | -------------------------------------------------------- | ------------------------------------------------------------ | | ||
| | `crates/grida-canvas/src/cg/svg.rs` | Add `SVGPatternPaint` struct and `SVGPaint::Pattern` variant | | ||
| | `crates/grida-canvas/src/svg/from_usvg.rs` | Convert `usvg::Paint::Pattern` instead of discarding | | ||
| | `crates/grida-canvas/src/cg/types.rs` | Possibly add `Paint::Pattern(...)` variant | | ||
| | `crates/grida-canvas/src/painter/paint.rs` | Handle pattern shader creation | | ||
| | `crates/grida-canvas-wasm/lib/modules/svg-bindings.d.ts` | Update `SVGPaint` type | | ||
| | `format/grida.fbs` | Add pattern paint type (if persisting to file format) | |
There was a problem hiding this comment.
Add the schema-model follow-ups to this checklist.
Changing format/grida.fbs is not self-contained here. If someone implements pattern persistence from this table alone, they can easily miss the required runtime model sync and leave the schema out of step with the Rust/TS document models. Please list crates/grida-canvas/src/node/schema.rs and packages/grida-canvas-schema/grida.ts alongside the FlatBuffers change. Based on learnings, "When modifying format/grida.fbs, update the Rust runtime model at crates/grida-canvas/src/node/schema.rs and the TS document model at packages/grida-canvas-schema/grida.ts together."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/feat-svg/pattern.md` around lines 211 - 218, Update the checklist to
require that any change to format/grida.fbs is accompanied by corresponding
updates to the runtime and document models: modify the Rust runtime model at
crates/grida-canvas/src/node/schema.rs and the TypeScript document model at
packages/grida-canvas-schema/grida.ts when adding the new pattern paint (e.g.,
SVGPatternPaint / Paint::Pattern) so schema, runtime, and TS models stay in
sync; add a short guidance sentence like: "When modifying format/grida.fbs,
update the Rust runtime model at crates/grida-canvas/src/node/schema.rs and the
TS document model at packages/grida-canvas-schema/grida.ts together."
Comment on lines
263
to
272
| useEffect(() => { | ||
| if (!raw) { | ||
| setConversion(undefined); | ||
| setPackedSceneJson(undefined); | ||
| setDocumentJson(undefined); | ||
| return; | ||
| } | ||
|
|
||
| const packed = editorInstance.svgPack(raw); | ||
|
|
||
| if (!packed) { | ||
| setConversion(undefined); | ||
| setPackedSceneJson(undefined); | ||
| const bytes = editorInstance.svgToDocument(raw); | ||
| if (!bytes) { | ||
| setDocumentJson(undefined); | ||
| return; |
There was a problem hiding this comment.
Keep the preview and panes in sync on failed conversion.
Line 265 and Line 271 clear the JSON state, but the mounted editor keeps the previous document. After an empty or temporarily invalid edit, the text panes say “nothing loaded” while the canvas still shows the last successful import.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/app/`(dev)/canvas/tools/io-svg/page.tsx around lines 263 - 272, The
effect clears document JSON on failed conversion but doesn't update the mounted
canvas, leaving the last import visible; after the checks where
setDocumentJson(undefined) is called (in the branches around useEffect and after
editorInstance.svgToDocument(raw) returning falsy), also clear the mounted
editor via editorInstance by loading an empty document or calling its clear
method (e.g., editorInstance.loadDocument(undefined) or editorInstance.clear())
so the canvas and text panes stay in sync when raw is empty or conversion fails.
Comment on lines
+276
to
+301
| const makeTextSpan = ( | ||
| spanData: svgtypes.ir.IRSVGTextSpanNode, | ||
| spanName: string, | ||
| fallbackFill: cg.Paint | undefined | ||
| ): grida.program.nodes.TextNodePrototype => { | ||
| const spanPosition = map.extractTranslation(spanData.transform); | ||
| const fontSize = spanData.font_size ?? 16; | ||
| const { paint: spanFill } = map.fill(spanData.fill); | ||
|
|
||
| const { paint: textFill } = map.fill(textFillAttr); | ||
| return { | ||
| type: "tspan", | ||
| name: spanName, | ||
| text: spanData.text, | ||
| font_size: fontSize, | ||
| font_weight: 400, | ||
| font_kerning: true, | ||
| text_decoration_line: "none", | ||
| text_align: "left", | ||
| text_align_vertical: "top", | ||
| layout_positioning: "absolute", | ||
| layout_inset_left: spanPosition.left, | ||
| layout_inset_top: spanPosition.top, | ||
| layout_target_width: undefined, | ||
| layout_target_height: undefined, | ||
| fill: spanFill ?? fallbackFill, | ||
| } satisfies grida.program.nodes.TextNodePrototype; |
There was a problem hiding this comment.
Use local offsets for grouped text spans.
The group is already positioned at textPosition, but makeTextSpan() still writes each child's absolute translation into layout_inset_*. Any <text> with a non-zero transform will therefore apply the parent offset twice. pack.rs already strips the parent transform before appending grouped spans; this path needs the same adjustment.
Suggested fix
- const makeTextSpan = (
+ const makeTextSpan = (
spanData: svgtypes.ir.IRSVGTextSpanNode,
spanName: string,
- fallbackFill: cg.Paint | undefined
+ fallbackFill: cg.Paint | undefined,
+ parentOffset: { left: number; top: number } = { left: 0, top: 0 }
): grida.program.nodes.TextNodePrototype => {
const spanPosition = map.extractTranslation(spanData.transform);
const fontSize = spanData.font_size ?? 16;
const { paint: spanFill } = map.fill(spanData.fill);
return {
@@
- layout_inset_left: spanPosition.left,
- layout_inset_top: spanPosition.top,
+ layout_inset_left: spanPosition.left - parentOffset.left,
+ layout_inset_top: spanPosition.top - parentOffset.top,
layout_target_width: undefined,
layout_target_height: undefined,
fill: spanFill ?? fallbackFill,
} satisfies grida.program.nodes.TextNodePrototype;
};
@@
- const children = spans
- .map((span, index) =>
- makeTextSpan(span, `${name}_line_${index}`, textFill)
- )
+ const children = spans
+ .map((span, index) =>
+ makeTextSpan(span, `${name}_line_${index}`, textFill, textPosition)
+ )
.filter(Boolean) as grida.program.nodes.NodePrototype[];Also applies to: 331-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io-svg/lib.ts` around lines 276 - 301, makeTextSpan()
is using each span's absolute transform for layout_inset_* which double-applies
the parent text group's translation (textPosition); change it to use local
offsets by subtracting the parent textPosition from the span's translation
(e.g., localLeft = spanPosition.left - textPosition.left, localTop =
spanPosition.top - textPosition.top) and assign those local values to
layout_inset_left/top. Apply the same adjustment to the analogous span-handling
block around lines 331-349.
- Deleted the @grida/io-svg package, including its implementation, tests, and documentation, to streamline the codebase. - Updated references in pnpm-lock.yaml and package.json to reflect the removal of the SVG I/O functionality. - Introduced new tests for cross-boundary FBS decoding in the grida-canvas-io package to ensure continued functionality without the removed package.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
crates/grida-canvas-wasm/lib/modules/svg.ts (1)
95-116:⚠️ Potential issue | 🟠 MajorDon't call
_deallocate()with the placeholder length.If anything throws after
_grida_svg_to_document()returns but before the 4-byte prefix is parsed,finallyfrees the buffer as4 + 0. The Rust side reconstructs aVecwith that exact size, so this is the wrong free length.Suggested fix
- let resultLen: number = 0; + let resultLen: number | null = null; @@ - resultLen = - heap[resultPtr] | - (heap[resultPtr + 1] << 8) | - (heap[resultPtr + 2] << 16) | - (heap[resultPtr + 3] << 24); + resultLen = + (heap[resultPtr] | + (heap[resultPtr + 1] << 8) | + (heap[resultPtr + 2] << 16) | + (heap[resultPtr + 3] << 24)) >>> 0; @@ - if (resultPtr !== 0) { + if (resultPtr !== 0 && resultLen !== null) { this.module._deallocate(resultPtr, 4 + resultLen); }At minimum, guard the free on
resultLen !== null. If you want to keep the failure path leak-free too, re-read the 4-byte prefix infinallybefore deallocating.#!/bin/bash set -euo pipefail # Compare the Rust allocation contract with the JS cleanup path. sed -n '1,20p' crates/grida-canvas-wasm/src/_internal.rs sed -n '76,108p' crates/grida-canvas-wasm/src/wasm_svg.rs sed -n '92,121p' crates/grida-canvas-wasm/lib/modules/svg.tsExpected result: Rust allocates
4 + lenand expects the same size on free, while the current JS cleanup uses the placeholder0until parsing succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas-wasm/lib/modules/svg.ts` around lines 95 - 116, The finally block may call this.module._deallocate(resultPtr, 4 + resultLen) with resultLen still 0 if an exception occurred before parsing the 4-byte length from the buffer returned by this.module._grida_svg_to_document; update the cleanup in the svg conversion path (symbols: _grida_svg_to_document, resultPtr, resultLen, _deallocate, _alloc_string) so you either (a) only call _deallocate when resultLen has been successfully read (guard on resultLen being set) or (b) if resultPtr !== 0 always re-read the 4-byte length from this.module.HEAPU8 inside the finally to compute the correct 4 + resultLen before deallocating, ensuring the Rust allocation size matches the deallocation size.
🧹 Nitpick comments (4)
packages/grida-canvas-io/__tests__/fbs-svg-cross-boundary.test.ts (1)
54-54: Minor: Simplifyit.eachparameter mapping.The mapping
goldens.map((g) => [g])wraps each string in an array just to destructure it back. You can pass the array directly and use the element.♻️ Optional simplification
- it.each(goldens.map((g) => [g]))("%s decodes successfully", (name) => { + it.each(goldens)("%s decodes successfully", (name) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io/__tests__/fbs-svg-cross-boundary.test.ts` at line 54, The test is needlessly wrapping each golden name in an array with goldens.map((g) => [g]); simplify the table by passing goldens directly to it.each and keep the test callback parameter as the single name (replace it.each(goldens.map((g) => [g])) with it.each(goldens) and ensure the test callback signature still uses (name) to receive each string).crates/grida-canvas/tests/svg_pack.rs (3)
1217-1243: This roundtrip check can miss rotation/skew loss.The fixture includes
rotate(30), but the test only looks for any scale and any translation. A codec regression that zeroes the off-diagonal terms would still pass. Assert that at least one group transform keeps non-zerom01/m10, or compare the expected matrices per group.Possible strengthening
let has_translate = group_transforms.iter().any(|t| { t.matrix[0][2].abs() > 0.1 || t.matrix[1][2].abs() > 0.1 }); assert!( has_translate, "at least one group should have translation" ); + + let has_rotation = group_transforms.iter().any(|t| { + t.matrix[0][1].abs() > 0.01 || t.matrix[1][0].abs() > 0.01 + }); + assert!( + has_rotation, + "at least one group should preserve the rotate(30) off-diagonal terms" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/svg_pack.rs` around lines 1217 - 1243, The test currently verifies only scale and translation on group_transforms, which misses rotation/skew loss; update the assertions to also ensure at least one group's off-diagonal matrix elements are non-trivial by checking t.matrix[0][1] or t.matrix[1][0] exceed a small epsilon (e.g., > 0.01) or alternatively compare each group against the known expected matrices for the fixture; locate the test that builds/iterates group_transforms and add an assertion similar to has_rotate_or_skew that uses t.matrix[0][1] and t.matrix[1][0] to fail if all off-diagonals are effectively zero.
660-700: Avoid keying this regression off DFS indexes.These assertions depend on the exact traversal order and on
rectlowering toPath, so harmless scene-graph refactors can fail the test even when world transforms are still correct. Prefer locating the target nodes by a stable predicate and then asserting their transforms.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/svg_pack.rs` around lines 660 - 700, The test currently depends on DFS indexes (transforms[1], [2], [4], [6]) which is fragile; replace those index-based assertions by locating nodes with stable predicates over the transforms vector: use transforms.iter().find(...) (or .position) to find the Group/Path entries by their label (e.g., "Group" / "Path") and by checking the transform values expected for L1 (translate ≈ (250,250)) and the rect-derived path positions (≈ (150,150) for L1, and NOT ≈ (250,250) for L2/L3) using the existing approx closure; then run the same asserts on the found entries (and panic with a clear message if .find returns None) instead of indexing into transforms by fixed positions.
1288-1300: The “full 2x3 matrix” claim isn’t covered yet.These assertions only validate the 2x2 rotation block. If FBS encoding drops the translation terms from
rotate(45, 0.5, 0.5), this test still passes. Add checks form[0][2]andm[1][2]too.Possible strengthening
assert!( approx(m[0][1], -cos45) && approx(m[1][0], cos45), "off-diagonal should be ±sin(45°), got [{:.4}, {:.4}]", m[0][1], m[1][0] ); + + let expected_tx: f32 = 0.5; + let expected_ty: f32 = 0.5 - cos45; + assert!( + approx(m[0][2], expected_tx) && approx(m[1][2], expected_ty), + "translation should preserve rotate(45°, 0.5, 0.5), got [{:.4}, {:.4}]", + m[0][2], + m[1][2] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/svg_pack.rs` around lines 1288 - 1300, The test currently only checks the 2x2 rotation block (m[0][0], m[1][1], m[0][1], m[1][0]) but ignores the translation terms; compute the expected translation for a rotation about (0.5,0.5) as expected_t = 0.5 * ((1.0 - cos45) + sin45) and add assertions that approx(m[0][2], expected_t) and approx(m[1][2], expected_t) hold (use the same approx helper and error messages), updating the assertions in the test that references m, cos45, sin45, and approx so the full 2x3 matrix is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-canvas/src/svg/pack.rs`:
- Around line 152-170: The code currently always assigns text.transform to the
wrapper group and then tries to compute a local span transform by inverting
text.transform; when text_affine.inverse() returns None this causes the span
transform to be treated as if it's in the group's local space and results in
double application. Fix by detecting the non-invertible case: if
text_affine.inverse() returns None, do not set the group's transform (clear
group.transform or avoid assigning it) and leave local = span_affine so spans
remain in parent coordinates; otherwise, keep group.transform =
Some(text.transform.into()) and compute local = inv.compose(&span_affine) as
before. Update the logic around create_group_node / group.transform and the
local computation accordingly.
In `@editor/grida-canvas/editor.ts`:
- Around line 716-723: The code currently picks the scene via
fullDoc.scenes_ref[0]; change this to use the document's semantic root by
selecting fullDoc.entry_scene_id ?? fullDoc.scenes_ref[0] and if that resolves
to undefined throw or return early (fail fast) so downstream logic (the sceneId
binding and the subsequent removals of fullDoc.nodes and fullDoc.links via {
[sceneId]: ... }) never operates on an undefined sceneId; update the variable
named sceneId accordingly and ensure the guard triggers a clear error when
neither entry_scene_id nor scenes_ref[0] exist.
In `@packages/grida-tree/src/lib.ts`:
- Around line 2398-2412: The current remapping in keyMap for subgraph.links and
roots uses fallbacks (keyMap[childKey] ?? childKey and keyMap[r] ?? r) which can
leave dangling references and cause imported parents to bind to existing nodes
in the destination graph; update the remapping in the block that builds links
and newRoots to fail fast: when iterating subgraph.links and roots, if keyMap
does not contain a mapped value for any childKey or root r, throw an explicit
error (include context like the missing key and reference to subgraph id)
instead of using the original id; adjust code that builds links (the links
variable) and newRoots accordingly so Graph.import() never receives stale IDs.
- Around line 2376-2380: In remapKeys, guard against duplicate outputs from the
keygen callback by detecting when two different original keys map to the same
generated key: after building keyMap from subgraph.nodes (using keygen), iterate
keyMap values and collect duplicates (track seen generated keys and the original
keys that produced them) and throw an explicit error (or return a failure)
listing the conflicting original keys and the duplicated generated key;
reference remapKeys, keyMap, keygen, subgraph.nodes and subgraph.links so you
locate where to add this duplicate-check before applying the remap to
nodes/links.
---
Duplicate comments:
In `@crates/grida-canvas-wasm/lib/modules/svg.ts`:
- Around line 95-116: The finally block may call
this.module._deallocate(resultPtr, 4 + resultLen) with resultLen still 0 if an
exception occurred before parsing the 4-byte length from the buffer returned by
this.module._grida_svg_to_document; update the cleanup in the svg conversion
path (symbols: _grida_svg_to_document, resultPtr, resultLen, _deallocate,
_alloc_string) so you either (a) only call _deallocate when resultLen has been
successfully read (guard on resultLen being set) or (b) if resultPtr !== 0
always re-read the 4-byte length from this.module.HEAPU8 inside the finally to
compute the correct 4 + resultLen before deallocating, ensuring the Rust
allocation size matches the deallocation size.
---
Nitpick comments:
In `@crates/grida-canvas/tests/svg_pack.rs`:
- Around line 1217-1243: The test currently verifies only scale and translation
on group_transforms, which misses rotation/skew loss; update the assertions to
also ensure at least one group's off-diagonal matrix elements are non-trivial by
checking t.matrix[0][1] or t.matrix[1][0] exceed a small epsilon (e.g., > 0.01)
or alternatively compare each group against the known expected matrices for the
fixture; locate the test that builds/iterates group_transforms and add an
assertion similar to has_rotate_or_skew that uses t.matrix[0][1] and
t.matrix[1][0] to fail if all off-diagonals are effectively zero.
- Around line 660-700: The test currently depends on DFS indexes (transforms[1],
[2], [4], [6]) which is fragile; replace those index-based assertions by
locating nodes with stable predicates over the transforms vector: use
transforms.iter().find(...) (or .position) to find the Group/Path entries by
their label (e.g., "Group" / "Path") and by checking the transform values
expected for L1 (translate ≈ (250,250)) and the rect-derived path positions (≈
(150,150) for L1, and NOT ≈ (250,250) for L2/L3) using the existing approx
closure; then run the same asserts on the found entries (and panic with a clear
message if .find returns None) instead of indexing into transforms by fixed
positions.
- Around line 1288-1300: The test currently only checks the 2x2 rotation block
(m[0][0], m[1][1], m[0][1], m[1][0]) but ignores the translation terms; compute
the expected translation for a rotation about (0.5,0.5) as expected_t = 0.5 *
((1.0 - cos45) + sin45) and add assertions that approx(m[0][2], expected_t) and
approx(m[1][2], expected_t) hold (use the same approx helper and error
messages), updating the assertions in the test that references m, cos45, sin45,
and approx so the full 2x3 matrix is validated.
In `@packages/grida-canvas-io/__tests__/fbs-svg-cross-boundary.test.ts`:
- Line 54: The test is needlessly wrapping each golden name in an array with
goldens.map((g) => [g]); simplify the table by passing goldens directly to
it.each and keep the test callback parameter as the single name (replace
it.each(goldens.map((g) => [g])) with it.each(goldens) and ensure the test
callback signature still uses (name) to receive each string).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20583a0c-f6c5-45cf-94aa-2f73ade681cb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
crates/grida-canvas-wasm/lib/modules/svg.tscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/svg/pack.rscrates/grida-canvas/tests/svg_pack.rsdocs/wg/feat-svg/text-import.mdeditor/grida-canvas/editor.i.tseditor/grida-canvas/editor.tseditor/package.jsonpackages/grida-canvas-io-svg/README.mdpackages/grida-canvas-io-svg/__tests__/convert.test.tspackages/grida-canvas-io-svg/index.tspackages/grida-canvas-io-svg/lib.tspackages/grida-canvas-io-svg/package.jsonpackages/grida-canvas-io/__tests__/fbs-svg-cross-boundary.test.tspackages/grida-canvas-io/format.tspackages/grida-tree/src/lib.ts
💤 Files with no reviewable changes (6)
- packages/grida-canvas-io-svg/README.md
- editor/package.json
- packages/grida-canvas-io-svg/package.json
- packages/grida-canvas-io-svg/index.ts
- packages/grida-canvas-io-svg/lib.ts
- packages/grida-canvas-io-svg/tests/convert.test.ts
✅ Files skipped from review due to trivial changes (1)
- crates/grida-canvas/src/cache/geometry.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/grida-canvas/editor.i.ts
Comment on lines
+152
to
+170
| let mut group = self.factory.create_group_node(); | ||
| group.transform = Some(text.transform.into()); | ||
| let group_id = self.graph.append_child(Node::Group(group), parent); | ||
| let group_parent = Parent::NodeId(group_id); | ||
|
|
||
| for span in &text.spans { | ||
| // Each chunk's transform already includes the text's relative | ||
| // transform (applied in convert_text). Since the group also | ||
| // carries the text transform, we need to strip it from each | ||
| // chunk to avoid double-application. The chunk transform is | ||
| // `text_relative * translate(chunk_x, chunk_y)`, so the local | ||
| // transform relative to the group is just `translate(chunk_x, chunk_y)`. | ||
| let text_affine: AffineTransform = text.transform.into(); | ||
| let span_affine: AffineTransform = span.transform.into(); | ||
| let local = if let Some(inv) = text_affine.inverse() { | ||
| inv.compose(&span_affine) | ||
| } else { | ||
| span_affine | ||
| }; |
There was a problem hiding this comment.
Handle singular text transforms without double-applying them.
When text.transform.inverse() returns None, span_affine is already in the parent coordinate space. Reusing it under a wrapper group that still carries text.transform applies the text matrix twice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/svg/pack.rs` around lines 152 - 170, The code
currently always assigns text.transform to the wrapper group and then tries to
compute a local span transform by inverting text.transform; when
text_affine.inverse() returns None this causes the span transform to be treated
as if it's in the group's local space and results in double application. Fix by
detecting the non-invertible case: if text_affine.inverse() returns None, do not
set the group's transform (clear group.transform or avoid assigning it) and
leave local = span_affine so spans remain in parent coordinates; otherwise, keep
group.transform = Some(text.transform.into()) and compute local =
inv.compose(&span_affine) as before. Update the logic around create_group_node /
group.transform and the local computation accordingly.
Comment on lines
+716
to
+723
| const sceneId = fullDoc.scenes_ref[0]; | ||
| const sceneChildren = fullDoc.links[sceneId] ?? []; | ||
|
|
||
| // Remove the scene node itself — we're inserting into the editor's | ||
| // existing scene, not creating a new one. | ||
| const { [sceneId]: _sceneNode, ...contentNodes } = fullDoc.nodes; | ||
| const { [sceneId]: _sceneLinks, ...contentLinks } = fullDoc.links; | ||
|
|
There was a problem hiding this comment.
Use the document’s entry scene here.
This path decodes a full document, but it imports scenes_ref[0]. Elsewhere in this file the semantic root is entry_scene_id, so this becomes order-dependent and degrades badly when the decoded document has no first scene. Prefer fullDoc.entry_scene_id ?? fullDoc.scenes_ref[0] and fail fast if that value is missing.
Suggested guard
- const sceneId = fullDoc.scenes_ref[0];
+ const sceneId = fullDoc.entry_scene_id ?? fullDoc.scenes_ref[0];
+ if (!sceneId) {
+ throw new Error("Decoded SVG document has no entry scene");
+ }
const sceneChildren = fullDoc.links[sceneId] ?? [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/grida-canvas/editor.ts` around lines 716 - 723, The code currently
picks the scene via fullDoc.scenes_ref[0]; change this to use the document's
semantic root by selecting fullDoc.entry_scene_id ?? fullDoc.scenes_ref[0] and
if that resolves to undefined throw or return early (fail fast) so downstream
logic (the sceneId binding and the subsequent removals of fullDoc.nodes and
fullDoc.links via { [sceneId]: ... }) never operates on an undefined sceneId;
update the variable named sceneId accordingly and ensure the guard triggers a
clear error when neither entry_scene_id nor scenes_ref[0] exist.
Comment on lines
+2376
to
+2380
| // 1. Build the mapping | ||
| const keyMap: Record<tree.Key, tree.Key> = {}; | ||
| for (const oldKey of Object.keys(subgraph.nodes)) { | ||
| keyMap[oldKey] = keygen(oldKey); | ||
| } |
There was a problem hiding this comment.
Guard remapKeys() against duplicate outputs from keygen.
A repeated generated key silently overwrites the first node/link pair, so one bad callback can drop part of the imported subtree with no error.
Suggested fix
const keyMap: Record<tree.Key, tree.Key> = {};
+ const seen = new Set<tree.Key>();
for (const oldKey of Object.keys(subgraph.nodes)) {
- keyMap[oldKey] = keygen(oldKey);
+ const newKey = keygen(oldKey);
+ if (seen.has(newKey)) {
+ throw new Error(`remapKeys: duplicate generated key '${newKey}'`);
+ }
+ seen.add(newKey);
+ keyMap[oldKey] = newKey;
}📝 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.
Suggested change
| // 1. Build the mapping | |
| const keyMap: Record<tree.Key, tree.Key> = {}; | |
| for (const oldKey of Object.keys(subgraph.nodes)) { | |
| keyMap[oldKey] = keygen(oldKey); | |
| } | |
| // 1. Build the mapping | |
| const keyMap: Record<tree.Key, tree.Key> = {}; | |
| const seen = new Set<tree.Key>(); | |
| for (const oldKey of Object.keys(subgraph.nodes)) { | |
| const newKey = keygen(oldKey); | |
| if (seen.has(newKey)) { | |
| throw new Error(`remapKeys: duplicate generated key '${newKey}'`); | |
| } | |
| seen.add(newKey); | |
| keyMap[oldKey] = newKey; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-tree/src/lib.ts` around lines 2376 - 2380, In remapKeys, guard
against duplicate outputs from the keygen callback by detecting when two
different original keys map to the same generated key: after building keyMap
from subgraph.nodes (using keygen), iterate keyMap values and collect duplicates
(track seen generated keys and the original keys that produced them) and throw
an explicit error (or return a failure) listing the conflicting original keys
and the duplicated generated key; reference remapKeys, keyMap, keygen,
subgraph.nodes and subgraph.links so you locate where to add this
duplicate-check before applying the remap to nodes/links.
Comment on lines
+2398
to
+2412
| // 3. Remap links | ||
| const links: Record<tree.Key, tree.Key[] | undefined> = {}; | ||
| for (const [oldKey, children] of Object.entries(subgraph.links)) { | ||
| const newKey = keyMap[oldKey]!; | ||
| if (Array.isArray(children)) { | ||
| links[newKey] = children.map( | ||
| (childKey: tree.Key) => keyMap[childKey] ?? childKey | ||
| ); | ||
| } else { | ||
| links[newKey] = undefined; | ||
| } | ||
| } | ||
|
|
||
| // 4. Remap roots | ||
| const newRoots = roots.map((r) => keyMap[r] ?? r); |
There was a problem hiding this comment.
Fail fast on dangling link/root references.
The ?? childKey / ?? r fallback keeps old IDs alive. Graph.import() later copies subgraph.links verbatim, so a stale child ID can accidentally bind an imported parent to an existing node in the destination graph instead of staying inside the imported subgraph.
Suggested fix
const links: Record<tree.Key, tree.Key[] | undefined> = {};
for (const [oldKey, children] of Object.entries(subgraph.links)) {
- const newKey = keyMap[oldKey]!;
+ const newKey = keyMap[oldKey];
+ if (newKey === undefined) {
+ throw new Error(
+ `remapKeys: link parent '${oldKey}' is missing from subgraph.nodes`
+ );
+ }
if (Array.isArray(children)) {
- links[newKey] = children.map(
- (childKey: tree.Key) => keyMap[childKey] ?? childKey
- );
+ links[newKey] = children.map((childKey: tree.Key) => {
+ const mappedChild = keyMap[childKey];
+ if (mappedChild === undefined) {
+ throw new Error(
+ `remapKeys: child '${childKey}' referenced by '${oldKey}' is missing from subgraph.nodes`
+ );
+ }
+ return mappedChild;
+ });
} else {
links[newKey] = undefined;
}
}
// 4. Remap roots
- const newRoots = roots.map((r) => keyMap[r] ?? r);
+ const newRoots = roots.map((r) => {
+ const mappedRoot = keyMap[r];
+ if (mappedRoot === undefined) {
+ throw new Error(
+ `remapKeys: root '${r}' is missing from subgraph.nodes`
+ );
+ }
+ return mappedRoot;
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-tree/src/lib.ts` around lines 2398 - 2412, The current
remapping in keyMap for subgraph.links and roots uses fallbacks
(keyMap[childKey] ?? childKey and keyMap[r] ?? r) which can leave dangling
references and cause imported parents to bind to existing nodes in the
destination graph; update the remapping in the block that builds links and
newRoots to fail fast: when iterating subgraph.links and roots, if keyMap does
not contain a mapped value for any childKey or root r, throw an explicit error
(include context like the missing key and reference to subgraph id) instead of
using the original id; adjust code that builds links (the links variable) and
newRoots accordingly so Graph.import() never receives stale IDs.
- Introduced a new `turbo.json` file to define task dependencies, specifically linking the `test` task to the `build` task. - This configuration aims to streamline the build and testing process within the project.
This was referenced Mar 28, 2026
Merged
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Major improvements to the SVG import pipeline, covering text handling, gradient rendering, nested transforms, and a new end-to-end SVG →
.gridaFlatBuffers conversion path.Changes
SVG Text Import
<text>/<tspan>conversion to Grida IR (TextSpanNodeRec), supporting single-span and multi-span (multiline) text nodesdocs/wg/feat-svg/text-import.md)SVG Gradient & Paint Handling
(x, y, width, height)instead of(width, height), preventing gradient shift on non-square shapesnormalize_gradient_transformto account for bounding box originNested Transforms & Geometry Cache
GeometryCache, differentiating layout containers from non-layout nodestransforms-nested.svgtest fixtureSVG → .grida FlatBuffers Conversion
svg_to_grida_bytesfunction to parse SVG and produce.gridaFlatBuffers bytes directlysvg_packabstraction in favor of the streamlined conversionSvgToGridaCLI command ingrida-devfor batch conversion and cross-boundary codec testingSVG Test Fixtures
Documentation & Research
<pattern>support doc with Chromium/resvg/Skia comparison (docs/wg/research/chromium/svg-pattern.md)cg-reftestagent skill covering reftests, golden tests, probe tests, and data testsEditor & Misc
0.91.0-canary.3GridaFileFlatBuffers table documentationSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests