Canary: skip_layout optimization, stroke dash arrays, mipmap simplification, Figma ID preservation, and scene bounding box#598
Merged
softmarshmallow merged 25 commits intomainfrom Mar 23, 2026
Merged
Conversation
- Introduced stroke_dash_array property to ContainerNode, LineNode, PathNode, VectorNode, and BooleanPathOperationNode, enhancing stroke styling capabilities. - Implemented roundtrip tests to ensure proper serialization and deserialization of stroke_dash_array across different node types, verifying functionality and consistency.
- Introduced PackOptions interface to allow users to specify the ZIP compression level (0-9) when packing documents. - Updated the pack function to accept the new options parameter, defaulting to level 6 if not specified. - Adjusted the fig2grida-core to utilize the new compression level feature, setting it to 0 for faster packing of merged documents.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a schema-only "skip_layout" fast path and runtime toggle, per-stage scene-load and resize benchmarking with a new load-bench CLI, removes custom mipmap code in favor of Skia mipmaps, introduces change-tracking APIs and paragraph measurement stats, updates WASM/JS bindings (config_flags, runtime toggle, "" bounds), and many tests/fixtures/formatting edits. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor
participant Surface as WASM Surface
participant App as Canvas App
participant Renderer as Renderer
participant Layout as Layout Engine
Editor->>Surface: createWebGLCanvasSurface(el, {config: {skip_layout: true}})
Surface->>App: _init_with_backend(..., config_flags)
App->>Renderer: new_with_options(config)
rect rgba(100,150,200,0.5)
Note over Editor,Layout: Scene loading flow (per-stage timing)
end
Editor->>Surface: loadScene(scene)
Surface->>App: load_scene(scene)
App->>Renderer: load_scene(scene)
alt skip_layout enabled
Renderer->>Layout: compute_schema_only(scene)
Layout-->>Renderer: LayoutResult (schema positions)
else normal path
Renderer->>Layout: compute(scene) (Taffy + text measurement)
Layout-->>Renderer: LayoutResult (taffy)
end
Renderer->>Renderer: update_geometry / effects / layers
Renderer-->>App: scene loaded
App-->>Surface: ready
Surface-->>Editor: sceneReady
sequenceDiagram
participant CLI as load-bench CLI
participant Loader as Scene Loader
participant Measure as measure_load_scene
participant Layout as Layout Engine
participant Stats as ParagraphStats
CLI->>Loader: open .grida, select scene
loop per iteration
CLI->>Measure: run per-stage timings
Measure->>Renderer: collect_scene_font_families
alt --skip-layout
Measure->>Layout: compute_schema_only()
else
Measure->>Layout: compute() (may call text measure)
Layout->>Stats: paragraph stats (calls/hits/misses)
end
Measure->>Renderer: geometry / effects / layers
Measure-->>CLI: return stage durations + stats
end
CLI->>CLI: aggregate & print percentiles
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
- Introduced a new benchmarking suite for the fig2grida pipeline, allowing performance evaluation of various stages including gzip decompression, JSON parsing, and document conversion. - Added a README file to guide users on required fixture files for running benchmarks. - Updated package.json to include a new script for running benchmarks.
- Changed the camera fit method across multiple components to use "<scene>" instead of "*", ensuring consistent behavior when fitting the camera to the scene. - Updated relevant code in editor.ts, uxhost-menu.tsx, renderer.tsx, hotkeys.tsx, use-refig-editor.ts, and ext-zoom.tsx to reflect this change.
- Updated the getNodeAbsoluteBoundingBox method to accept a BoundsTarget, allowing retrieval of bounding boxes for both specific nodes and the active scene. - Introduced a new BoundsTarget enum to differentiate between node IDs and the scene target. - Modified related TypeScript definitions and implementations across WASM and DOM backends to accommodate the new functionality. - Improved documentation for clarity on usage of the new target parameter.
- Created a symbolic link in the .claude directory pointing to the skills directory in .agents, facilitating easier access and management of skills resources.
- Added entries for Claude Code local settings and session files to the .gitignore, ensuring that sensitive and temporary files are not tracked by Git. - Included patterns for settings.local.json, plans, todos.json, worktrees, and CLAUDE.local.md.
- Added support for preserving original Figma node IDs in the fig2grida pipeline, allowing emitted events to carry these IDs instead of generating new ones. - Introduced a new optional transform function in the EmbedBridge to decode synthetic Figma IDs back to their original format. - Updated the useRefigEditor hook to utilize the new decoding function, enhancing compatibility with Figma's ID structure. - Added tests to ensure correct functionality of ID preservation and decoding logic across various scenarios.
…ipmap support - Deleted the mipmap module and related structures, simplifying the image handling process. - Updated the ImageRepository to store images with Skia's native mipmap chain, enhancing performance and reducing complexity. - Refactored benchmarks and tests to validate the new mipmap functionality using Skia's built-in methods. - Adjusted image shader creation to leverage mipmap sampling directly from Skia, ensuring correct LOD selection during rendering.
- Modified the research skill description to include Taffy as a relevant project for layout algorithms. - Enhanced the known citations section by adding Taffy, detailing its role in flexbox/grid layout algorithms and Rust-native layout engine internals.
softmarshmallow
changed the title
- Added detailed sections on scene loading and layout performance, highlighting the cold-start bottleneck and the impact of layout on load times for large documents. - Introduced the `skip_layout` optimization for absolute-positioned documents, along with diagnostic tooling for performance measurement. - Updated the organization of optimization categories to reflect new insights on layout and scene loading processes.
- Documented the current limitations of auto-layout support in the Figma conversion process, highlighting that all nodes are emitted with absolute positioning and flow layout. - Outlined necessary mappings and adjustments required for true auto-layout round-trip functionality, including layout modes, alignment, and sizing properties. - Emphasized the implications of the current implementation on layout semantics and child reflow behavior.
- Introduced a new `--resize` flag to the benchmarking tool, allowing measurement of `resize()` and `redraw()` costs per cycle. - Updated documentation to include instructions for running the resize benchmark. - Implemented the `measure_resize` and `run_resize_pass` functions to simulate viewport size changes and gather performance metrics. - Enhanced the `BenchArgs` struct to support the new resize option, improving the overall benchmarking capabilities.
- Extended Fig2GridaOptions and RestJsonToGridaOptions to include prefer_fixed_text_sizing, allowing for fixed-size text frames that match Figma's rendered output. - Updated fig2grida and related functions to support the new option, ensuring compatibility with the existing image and layout handling. - Enhanced CLI argument parsing to accommodate the prefer_fixed_text_sizing flag, improving user control over text sizing behavior.
- Upgraded `grid` to version 1.0.0 and `taffy` to version 0.9.2 in Cargo.lock and Cargo.toml. - Introduced a new benchmark for `load_scene`, measuring performance across different scene types and sizes. - Added `bench_load_scene.rs` to evaluate layout, geometry, effects, and layers during scene rendering.
- Added `skip_layout` configuration to bypass Taffy layout computation for absolute-positioned documents, improving load performance. - Implemented `LoadBench` command to measure `load_scene()` timings across various stages, including layout, geometry, effects, and layers. - Enhanced `Renderer` and `ApplicationApi` to support the new layout skipping feature, allowing for more efficient scene loading in specific scenarios. - Updated documentation and tests to reflect the new benchmarking capabilities and layout optimizations.
- Introduced a new `RefigRenderConfig` interface to optimize rendering by skipping layout calculations and using fixed text sizing for Figma imports. - Updated the `useRefigEditor` function to apply the new configuration during initialization, enhancing performance for documents with absolute positioning. - Ensured compatibility with existing text node handling by integrating the `prefer_fixed_text_sizing` option.
![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: 96afff17b0
ℹ️ 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
3231
to
3233
| if (targetScene) { | ||
| __switchSceneGuard++; | ||
| surface.switchScene(targetScene); |
There was a problem hiding this comment.
Because syncDocument() now increments __switchSceneGuard before every surface.switchScene(), the initial mount path leaves the counter at 1: syncDocument(...) is called at mount time before the scene_id subscription is registered, so nothing consumes that first increment. The first real scene change after mount then hits the guard at editor.ts:3315-3317 and returns without calling switchScene, so multi-page documents can no longer switch to the next scene on the first attempt.
Useful? React with 👍 / 👎.
Comment on lines
338
to
+340
| prefer_path_for_geometry: true, | ||
| placeholder_for_missing_images: placeholderForMissing, | ||
| preserve_figma_ids: options?.preserve_figma_ids, |
There was a problem hiding this comment.
.fig import path
prefer_fixed_text_sizing is forwarded for REST JSON imports, but the binary .fig path drops it here. In refig we enable both cg_skip_layout and prefer_fixed_text_sizing; when the source is a .fig file, TEXT nodes still keep "auto" width/height in iofigma (lib.ts:1618-1625), and the new compute_schema_only() fast path treats those dimensions as 0. The result is collapsed or missing text on .fig imports even though the option is supposedly on.
Useful? React with 👍 / 👎.
Comment on lines
163
to
+165
| this.post({ | ||
| type: "grida:selection-change", | ||
| selection: state.selection, | ||
| selection: this.mapIds(state.selection), |
There was a problem hiding this comment.
Once __dangerously_transform_node_id is set (the refig page passes decodeSyntheticFigmaId), outbound events start exposing transformed IDs here, but handleCommand() still forwards grida:select straight to Editor.commands.select() using the host-supplied IDs. A host that echoes IDs back from selection-change or document-load can no longer re-select synthetic Figma children or instance clones, because the editor's document still stores the unmapped internal IDs.
Useful? React with 👍 / 👎.
Comment on lines
+84
to
+85
| // DOM backend does not support "<scene>" — fall back to null | ||
| if (target === "<scene>") return null; |
There was a problem hiding this comment.
<scene> fit target
The DOM geometry provider now returns null for "<scene>", but several DOM-rendered pages switched their initial-fit effect to editor.camera.fit("<scene>") (for example grida-canvas-react/renderer.tsx:166-167, used by app/(workbench)/[org]/[proj]/[id]/canvas/page.tsx:50-52). On those pages the first fit now becomes a no-op, so the canvas opens at the default transform instead of framing the document.
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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/wg/feat-2d/optimization.md (1)
1-3:⚠️ Potential issue | 🟡 MinorAdd
format: mdto the frontmatter.This page is plain markdown, so it should opt out of MDX parsing while you're touching it.
As per coding guidelines
docs/{wg,reference}/**/*.{md,mdx}: For documentation files that don't use JSX/MDX features, opt out of MDX parsing by addingformat: mdto the frontmatter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/feat-2d/optimization.md` around lines 1 - 3, The frontmatter is missing the MD opt-out flag; update the YAML frontmatter block that currently contains "title: Rendering Optimization Strategies" to include "format: md" (i.e., add a new frontmatter key format: md alongside title) so this Markdown file opts out of MDX parsing.
🧹 Nitpick comments (8)
packages/grida-canvas-io/__tests__/format-roundtrip.test.ts (1)
2894-2908: Add an explicit empty-array roundtrip case ([]) to prevent normalization regressions.You currently test “set” and “undefined”, but not whether
[]is preserved as[](instead of being collapsed toundefined).Proposed test addition
it("roundtrips undefined stroke_dash_array (no dash)", () => { @@ ); }); + + it("roundtrips empty stroke_dash_array as empty array", () => { + const sceneId = "0-1"; + const nodeId = "0-2"; + const doc = createDocument(sceneId, { + [nodeId]: { + ...baseVector(nodeId), + stroke_dash_array: [], + }, + }); + roundtripTest<grida.program.nodes.VectorNode>( + doc, + nodeId, + "vector", + (node) => { + expect(node.stroke_dash_array).toEqual([]); + } + ); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io/__tests__/format-roundtrip.test.ts` around lines 2894 - 2908, Add a test that verifies an explicit empty stroke dash array is preserved (not normalized to undefined): create a test similar to the existing "roundtrips undefined stroke_dash_array (no dash)" but set the vector node's stroke_dash_array to [] when building the document (using createDocument and baseVector with nodeId) and call roundtripTest<grida.program.nodes.VectorNode>(...) asserting expect(node.stroke_dash_array).toEqual([]); this ensures roundtripTest, createDocument and baseVector are exercised and [] is kept rather than collapsed to undefined.crates/grida-canvas/src/layout/cache.rs (1)
48-51: Use total-target semantics when reserving map capacity.
HashMap::reserve(n)treatsnas additional entries, not total capacity. While the current code is always called afterclear(), the method should defensively handle all cases. If this method is called on a non-empty HashMap, it will over-allocate.♻️ Proposed fix
pub fn reserve(&mut self, capacity: usize) { - if self.layouts.capacity() < capacity { - self.layouts.reserve(capacity); - } + let additional = capacity.saturating_sub(self.layouts.len()); + if additional > 0 { + self.layouts.reserve(additional); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/layout/cache.rs` around lines 48 - 51, The reserve method currently calls self.layouts.reserve(capacity) treating capacity as additional entries; change it to reserve only the additional slots needed by computing let additional = capacity.saturating_sub(self.layouts.len()) and then call self.layouts.reserve(additional) so the method uses total-target semantics and won't over-allocate when layouts is non-empty (refer to the reserve method and the layouts HashMap).crates/grida-canvas/src/layout/tree.rs (1)
60-68: Strengthenreserve()so preallocation is always applied to both maps.Right now, reserve work is skipped unless
scene_to_taffyis below capacity. If map capacities diverge,taffy_to_scenecan remain under-provisioned and miss the intended allocation win.♻️ Proposed adjustment
pub(crate) fn reserve(&mut self, capacity: usize) { + debug_assert!( + self.scene_to_taffy.is_empty() && self.taffy_to_scene.is_empty(), + "LayoutTree::reserve should be called after clear()" + ); + // TaffyTree::with_capacity pre-allocates its internal SlotMaps. // After clear() the backing vecs keep their old capacity, so we // only need to replace the tree when the new count exceeds it. - if self.scene_to_taffy.capacity() < capacity { + if self.scene_to_taffy.capacity() < capacity || self.taffy_to_scene.capacity() < capacity { self.taffy = TaffyTree::with_capacity(capacity); - self.scene_to_taffy.reserve(capacity); - self.taffy_to_scene.reserve(capacity); } + self.scene_to_taffy + .reserve(capacity.saturating_sub(self.scene_to_taffy.len())); + self.taffy_to_scene + .reserve(capacity.saturating_sub(self.taffy_to_scene.len())); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/layout/tree.rs` around lines 60 - 68, The current reserve() only reserves taffy_to_scene when scene_to_taffy.capacity() < capacity, which can leave taffy_to_scene under-provisioned; update reserve() so it always calls self.scene_to_taffy.reserve(capacity) and self.taffy_to_scene.reserve(capacity), and keep the existing conditional only for replacing self.taffy via TaffyTree::with_capacity when needed (i.e., recreate self.taffy only if scene_to_taffy.capacity() < capacity).crates/grida-dev/src/bench/runner.rs (1)
200-208: Call out that this excludes render-target resize.
measure_resize()intentionally skips GPU surface recreation, so--resizeis currently measuring renderer-side rebuild/cache work, not end-to-end resize cost. Either resize the headless surface here or make that limitation explicit in the CLI output so these numbers are not compared against real window/browser resize timings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/bench/runner.rs` around lines 200 - 208, The benchmark's measure_resize() currently skips GPU surface recreation so it only measures renderer-side rebuilds/caches rather than end-to-end resize cost; update the runner to either perform a headless surface size change during measure_resize() (so it triggers the same surface recreation path as Application::resize()) or, if surface resize isn't feasible here, update the CLI/benchmark output for the --resize flag to explicitly state that render-target/GPU surface resize is excluded and the reported times are renderer-only; reference measure_resize(), Application::resize(), and the --resize CLI flag when making the change.crates/grida-canvas/src/cache/paragraph.rs (1)
174-178: Keep the benchmark-only skip switch out of the shared cache state.
skip_text_measuremakesmeasure()return zero geometry, but it's a public field onParagraphCache, so any non-benchmark caller can silently poison text layout for the whole renderer. A benchmark-only wrapper or an explicit per-call override would keep that failure mode out of the runtime API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/cache/paragraph.rs` around lines 174 - 178, The public boolean field skip_text_measure on ParagraphCache can silently break layout for all callers; make it not part of the shared cache state by removing it as a public field and implementing one of two safe options: (a) make skip_text_measure private on ParagraphCache and add an explicit per-call override parameter to the ParagraphCache::measure(...) method (e.g., measure(&self, params..., skip_text_measure: bool)) so callers must opt-in, or (b) remove the flag from ParagraphCache and add a separate BenchmarkParagraphCache wrapper type that holds the skip flag and delegates to ParagraphCache::measure() with the override; keep ParagraphMeasureStats and other public APIs unchanged. Ensure all places that referenced ParagraphCache::skip_text_measure are updated to use the new per-call parameter or the benchmark wrapper.packages/grida-canvas-io-figma/fig2grida-core.ts (2)
227-233: Consider using named options to avoid positionalundefinedplaceholders.The two
undefinedarguments before{ level: 0 }make the call harder to read and prone to errors if the signature changes. Ifio.archive.packis under your control, consider consolidating all optional parameters into a single options object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io-figma/fig2grida-core.ts` around lines 227 - 233, The call to io.archive.pack uses positional args with two placeholder undefined values which is unclear; change the API to accept a single options object (or an overload) and update the call site so you pass named properties instead of positional undefineds—e.g. refactor io.archive.pack signature to accept something like pack({ document, imageRecord, level }) and then call it with merged.document and merged.imageRecord via that options object (update references to io.archive.pack and its callers accordingly).
594-632: Consider exposingpreserve_figma_idsinRestJsonToGridaOptions.
restJsonToGridaDocumenthardcodespreserveFigmaIdstoundefined(line 630), meaning it will always generate new IDs. If this is the intended public API behavior, consider adding a comment explaining the design choice. Otherwise, consider addingpreserve_figma_idstoRestJsonToGridaOptionsfor API consistency withfig2grida.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io-figma/fig2grida-core.ts` around lines 594 - 632, The public API currently ignores the preserve flag because restJsonToGridaDocument calls restJsonToMergedDocument with the preserveFigmaIds argument hardcoded to undefined; update the RestJsonToGridaOptions interface to include a preserve_figma_ids?: boolean (or preserveFigmaIds) option and pass options?.preserve_figma_ids into the restJsonToMergedDocument call instead of undefined (reference RestJsonToGridaOptions, restJsonToGridaDocument and restJsonToMergedDocument), or if the behavior is intentional, add a short comment in restJsonToGridaDocument explaining why preserveFigmaIds is omitted rather than wiring the option through.editor/grida-canvas/editor.ts (1)
3195-3318: Guard counter may accumulate if scene doesn't change.The
__switchSceneGuardis incremented insyncDocumentbeforeswitchScene(), but only decremented in thescene_idsubscriber. IfsyncDocumentis called but the scene_id state doesn't actually change (e.g., on patch-failure fallback to the same scene), the subscriber won't fire and the guard won't decrement.This is unlikely to cause issues in practice since
syncDocumentis typically called with scene changes, but for defensive coding, consider decrementing the guard after the synchronousswitchScenecompletes rather than relying on the subscriber:🛡️ Suggested defensive approach
if (targetScene) { __switchSceneGuard++; surface.switchScene(targetScene); + // Decrement immediately; the subscriber will see guard > 0 and skip. + // If subscriber doesn't fire (same scene), guard is still correctly 0. + __switchSceneGuard--; }Then update the subscriber to just check (not decrement):
- if (__switchSceneGuard > 0) { - __switchSceneGuard--; - return; - } + if (__switchSceneGuard > 0) { + return; + }🤖 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 3195 - 3318, The guard counter __switchSceneGuard can leak because syncDocument increments it before calling surface.switchScene(targetScene) but only the scene_id subscriber decrements it; move the decrement to immediately after the synchronous call so the counter is balanced even if the subscriber doesn't fire: in syncDocument (around surface.switchScene(targetScene)) decrement __switchSceneGuard (use try/finally to ensure decrement on exceptions), and update the state.scene_id subscriber handler to only check (__switchSceneGuard > 0) without decrementing.
🤖 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/canvas.ts`:
- Around line 455-463: The generated TypeScript declarations are stale: update
the .d.ts outputs so the new API (Scene.getNodeAbsoluteBoundingBox accepting
"<scene>" and the new Scene.runtime_renderer_set_skip_layout method) is
reflected; regenerate declarations by running the project's build that invokes
tsup with dts enabled (as configured in tsup.config.ts) so dist/index.d.ts is
rebuilt from the updated canvas.ts and canvas-bindings.d.ts sources, then verify
dist/index.d.ts contains the updated signatures for getNodeAbsoluteBoundingBox
and runtime_renderer_set_skip_layout.
In `@crates/grida-canvas/benches/bench_load_scene.rs`:
- Around line 256-264: The benchmark is including setup work and leaking a
renderer each iteration; fix by moving expensive setup out of b.iter and
ensuring the renderer is freed. Create the renderer once before the closure
(call make_renderer() once) and clone or prepare scene once outside the b.iter
so the inner closure only calls renderer.load_scene(black_box(&scene)); if you
need a fresh renderer per iteration use b.iter_batched or b.iter_batched_ref to
construct a renderer in the setup closure and then call renderer.load_scene in
the measurement closure and call Renderer::free() (or explicitly drop the
renderer) in the teardown so the raw raster surface isn't leaked.
In `@crates/grida-canvas/src/layout/engine.rs`:
- Around line 152-198: compute_schema_only currently never receives viewport
size so nodes like InitialContainer resolve to 0×0; change compute_schema_only
signature to accept a viewport_size (e.g. Option<(f32,f32)> or a Size reference)
and thread that viewport through extract_schema_only_recursive when resolving
sizes; update extract_schema_only_recursive to accept the viewport parameter and
call get_schema_size(node, viewport_size) (or otherwise make get_schema_size
read the passed viewport) so InitialContainer roots use the scene viewport when
computing width/height; ensure callers of compute_schema_only (and any tests)
pass the appropriate viewport from Scene when available.
In `@crates/grida-canvas/src/runtime/scene.rs`:
- Around line 835-845: set_skip_layout currently only mutates
self.config.skip_layout and is a no-op for an already-loaded scene; change it to
check the previous value and if it actually changed and a scene is loaded, call
the scene cache/layout refresh (invoke rebuild_scene_caches()) and enqueue a
layout/scene update so the runtime reapplies the new skip_layout behavior
immediately (alternatively call load_scene/switch_scene flow if that’s the
canonical refresh path); ensure you only trigger this when old != skip to avoid
redundant work and preserve any required locking/ordering semantics.
In `@crates/grida-canvas/src/window/application.rs`:
- Around line 45-53: BoundsTarget::from_str currently interprets the literal
"<scene>" as the internal Scene sentinel and thus collides with real user IDs;
change the public boundary so string-based parsing accepts a UserNodeId (String)
and never maps any user-supplied string to the Scene sentinel. Concretely, stop
special-casing "<scene>" inside BoundsTarget::from_str — make from_str accept
only a UserNodeId (or rename to from_user_id) and return
BoundsTarget::Node(UserNodeId) for all inputs; provide a separate constructor or
explicit API (e.g., BoundsTarget::scene() or a distinct enum/variant used only
internally) to construct the Scene sentinel so that user-visible IDs are
preserved.
In `@crates/grida-canvas/tests/mipmap.rs`:
- Around line 34-45: The test tiny_image_mipmaps currently just unwraps or uses
the original image and never asserts behavior; update it to assert the resulting
image is 1×1 (use image.width()/height() or the appropriate accessor on the
returned image from with_default_mipmaps()) to verify the fallback, or instead
exercise ImageRepository::insert() by attempting to insert the result of
image.with_default_mipmaps().unwrap_or(image) and asserting the repository
contains an image with the expected 1×1 dimensions; locate and update
tiny_image_mipmaps, use with_default_mipmaps, image_snapshot, and
ImageRepository::insert as needed to add the assertion.
In `@crates/grida-dev/src/bench/load_bench.rs`:
- Around line 362-374: measure_load_scene_via_renderer currently constructs a
default Renderer (via Renderer::new) that doesn't honor the same flags used
during staged timings (e.g. --skip-layout or --skip-text) and it never releases
resources; update measure_load_scene_via_renderer to accept/propagate the
relevant flags (or a RendererOptions struct) into the Renderer::new call so the
e2e path matches the staged configuration (or omit this e2e timing when the
configuration cannot be matched, e.g. --skip-text), and ensure you call
renderer.free() (or the appropriate release method) before returning after
renderer.load_scene to avoid leaking the raster surface.
In `@crates/grida-dev/src/bench/runner.rs`:
- Around line 1229-1231: The alternate viewport size (size_b) can compute to
zero for very small args.width/height; update the computation of size_b in
runner.rs to clamp each dimension to a minimum of 1 (e.g., use std::cmp::max(1,
args.width * 2 / 3) and similarly for height) so the resize path never uses a
zero-sized viewport; locate the size_b binding and replace the raw
multiplication with the clamped expression.
In `@docs/wg/feat-2d/optimization.md`:
- Around line 867-910: The numbered strategy list is out of sequence (items
starting at "Skip Layout for Absolute-Position Documents" labeled 40..44 while
the next section resumes at 34); renumber these entries so the list is strictly
increasing and matches existing cross-references—update the numbers for the
items "Skip Layout for Absolute-Position Documents", "Pre-Allocate Layout Data
Structures", "Deferred / Viewport-Only Layout", "Pre-Measure Text Before Taffy",
and "Cache Text Measurements by Width Constraint" to continue from the prior
list number, and adjust any subsequent referenced numbers in the document to
preserve consistency.
In `@editor/grida-canvas/backends/dom-content.ts`:
- Around line 84-86: The early return for target === "<scene>" currently returns
null and breaks parity; instead detect that case in the resolver inside
backends/dom-content.ts and compute the scene bounds by aggregating all node
rects: iterate the DOM graph's nodes (use the same node-fetching logic used
elsewhere in this file), collect each node's rect (e.g., the
getNodeRect/getBoundingClientRect equivalent used in this module), compute the
union/encapsulating rectangle (min x/y and max x+width/y+height), and return
that aggregated rect instead of null; keep the existing code path for other
targets (node_id) intact.
In `@editor/grida-canvas/editor.i.ts`:
- Around line 2585-2592: The public API getNodeAbsoluteBoundingRect currently
overloads node IDs with the string sentinel "<scene>", which collides with a
possible real node ID; change the method signature so it no longer accepts a
string sentinel—replace the target parameter with an explicit discriminated
union or separate flag (e.g. Target = { id: string } | { scene: true } or add an
optional boolean isScene parameter) and update all call sites to construct the
new shape (or call the new scene-specific method). Update the method
implementation/overloads for getNodeAbsoluteBoundingRect to check the
discriminant (e.g. .id vs .scene) instead of comparing to "<scene>", and adjust
any types that referenced the old (string | "<scene>") form to the new Target
type to ensure real node IDs like "<scene>" are treated as IDs.
In `@editor/grida-canvas/embed-bridge.ts`:
- Around line 120-123: The mapIds method can emit duplicates because transformId
is many-to-one; update mapIds (and references to this.transformId) so that after
applying this.transformId to ids you deduplicate the results (e.g., collect into
a Set or filter unique values) and return the unique array; specifically modify
private mapIds(ids: string[]): string[] to apply transformId if present, then
remove duplicates before returning to ensure selection-change and grida:pong
never emit duplicate IDs.
In `@editor/scaffolds/embed/use-refig-editor.ts`:
- Around line 286-290: The current INSTANCE_CLONE_RE (/^.+::(.+)$/) matches any
ID containing "::" and should be tightened to only match the documented clone
shape "{prefix}::{counter}::{originalId}"; update INSTANCE_CLONE_RE to require a
numeric counter (for example use a regex like /^[^:]+::\d+::(.+)$/ or
equivalent) so the capture group returns only the original Figma ID, and apply
the same tightened regex to the other occurrence mentioned (lines ~304-313)
where the clone-ID pattern is used (search for INSTANCE_CLONE_RE or the existing
/^.+::(.+)$/ and replace both).
In `@packages/grida-canvas-io-figma/__bench__/fig2grida.bench.ts`:
- Around line 38-40: The benchmark copies the fixture before gzip decompression
by doing new Uint8Array(FIXTURE_GZ), which adds allocation+copy to the timed
"gzip decompress" stage; change the code that initializes decompressedBytes (and
the similar occurrence around the second gunzipSync use) to pass FIXTURE_GZ
directly to gunzipSync (or ensure FIXTURE_GZ is already a Uint8Array/Buffer) so
gunzipSync operates on the original buffer without creating a copy, updating the
initialization of decompressedBytes and the other gunzipSync call accordingly.
In `@packages/grida-canvas-io-figma/__bench__/README.md`:
- Line 3: Update the README entry that currently references the stale benchmark
path "__tests__/fig2grida.bench.ts" to the correct path
"__bench__/fig2grida.bench.ts" so the sentence reads that this directory holds
large fixture files used by "__bench__/fig2grida.bench.ts"; edit the string in
the README to replace the old path with the new one.
In `@packages/grida-canvas-io-figma/__tests__/fig2grida.test.ts`:
- Around line 343-351: The test currently silently passes when no synthetic
children exist because the loop is skipped; update the test around the
syntheticIds variable to assert that syntheticIds is non-empty before iterating
(e.g., expect(syntheticIds.length).toBeGreaterThan(0) or otherwise fail fast) so
the regression is caught; reference the syntheticIds variable and the subsequent
loop that uses FIGMA_ID_RE to ensure the test fails immediately if no
`_fill_`/`_stroke_` nodes are produced.
- Around line 252-263: The current FIGMA_ID_RE only matches a prefix allowing
invalid IDs; update the tests to first call normalizeDerivedFigmaId(id) (or the
function that strips clone/synthetic wrappers) and then assert the result
matches a fully anchored base ID regex (rename/introduce BASE_FIGMA_ID_RE that
exactly matches formats like /^\d+:\d+$|^I\d+:\d+(?:;[\d:]+)?$/), and replace
uses of FIGMA_ID_RE in the preserve-ID assertions with BASE_FIGMA_ID_RE to
ensure the entire normalized ID is validated.
In `@packages/grida-canvas-io-figma/fig2grida-core.ts`:
- Line 340: The kiwi conversion call is missing the prefer_fixed_text_sizing
flag, so update the call that sets preserve_figma_ids:
options?.preserve_figma_ids to also pass prefer_fixed_text_sizing:
options?.prefer_fixed_text_sizing when invoking convertPageToScene (the same
option used in the shared text layout logic and present in the REST factory
path); locate the kiwi converter invocation in fig2grida-core.ts and add the
prefer_fixed_text_sizing property from options to the converter options object
so text sizing behavior is consistent across both paths.
In `@packages/grida-canvas-io-figma/fig2grida.ts`:
- Around line 98-100: The new CLI flag args.prefer_fixed_text_sizing is being
parsed but not documented; update the in-file usage/help text and header docs so
the flag appears in CLI help. Locate the usage/options string or help output
near the top of fig2grida.ts (the block that prints usage/--help) and add an
entry for --prefer-fixed-text-sizing with a short description and default
behavior; also add a one-line mention to the file header docs if present. Ensure
the flag name exactly matches args.prefer_fixed_text_sizing and include it in
any generated option lists so it appears when the program prints help.
---
Outside diff comments:
In `@docs/wg/feat-2d/optimization.md`:
- Around line 1-3: The frontmatter is missing the MD opt-out flag; update the
YAML frontmatter block that currently contains "title: Rendering Optimization
Strategies" to include "format: md" (i.e., add a new frontmatter key format: md
alongside title) so this Markdown file opts out of MDX parsing.
---
Nitpick comments:
In `@crates/grida-canvas/src/cache/paragraph.rs`:
- Around line 174-178: The public boolean field skip_text_measure on
ParagraphCache can silently break layout for all callers; make it not part of
the shared cache state by removing it as a public field and implementing one of
two safe options: (a) make skip_text_measure private on ParagraphCache and add
an explicit per-call override parameter to the ParagraphCache::measure(...)
method (e.g., measure(&self, params..., skip_text_measure: bool)) so callers
must opt-in, or (b) remove the flag from ParagraphCache and add a separate
BenchmarkParagraphCache wrapper type that holds the skip flag and delegates to
ParagraphCache::measure() with the override; keep ParagraphMeasureStats and
other public APIs unchanged. Ensure all places that referenced
ParagraphCache::skip_text_measure are updated to use the new per-call parameter
or the benchmark wrapper.
In `@crates/grida-canvas/src/layout/cache.rs`:
- Around line 48-51: The reserve method currently calls
self.layouts.reserve(capacity) treating capacity as additional entries; change
it to reserve only the additional slots needed by computing let additional =
capacity.saturating_sub(self.layouts.len()) and then call
self.layouts.reserve(additional) so the method uses total-target semantics and
won't over-allocate when layouts is non-empty (refer to the reserve method and
the layouts HashMap).
In `@crates/grida-canvas/src/layout/tree.rs`:
- Around line 60-68: The current reserve() only reserves taffy_to_scene when
scene_to_taffy.capacity() < capacity, which can leave taffy_to_scene
under-provisioned; update reserve() so it always calls
self.scene_to_taffy.reserve(capacity) and self.taffy_to_scene.reserve(capacity),
and keep the existing conditional only for replacing self.taffy via
TaffyTree::with_capacity when needed (i.e., recreate self.taffy only if
scene_to_taffy.capacity() < capacity).
In `@crates/grida-dev/src/bench/runner.rs`:
- Around line 200-208: The benchmark's measure_resize() currently skips GPU
surface recreation so it only measures renderer-side rebuilds/caches rather than
end-to-end resize cost; update the runner to either perform a headless surface
size change during measure_resize() (so it triggers the same surface recreation
path as Application::resize()) or, if surface resize isn't feasible here, update
the CLI/benchmark output for the --resize flag to explicitly state that
render-target/GPU surface resize is excluded and the reported times are
renderer-only; reference measure_resize(), Application::resize(), and the
--resize CLI flag when making the change.
In `@editor/grida-canvas/editor.ts`:
- Around line 3195-3318: The guard counter __switchSceneGuard can leak because
syncDocument increments it before calling surface.switchScene(targetScene) but
only the scene_id subscriber decrements it; move the decrement to immediately
after the synchronous call so the counter is balanced even if the subscriber
doesn't fire: in syncDocument (around surface.switchScene(targetScene))
decrement __switchSceneGuard (use try/finally to ensure decrement on
exceptions), and update the state.scene_id subscriber handler to only check
(__switchSceneGuard > 0) without decrementing.
In `@packages/grida-canvas-io-figma/fig2grida-core.ts`:
- Around line 227-233: The call to io.archive.pack uses positional args with two
placeholder undefined values which is unclear; change the API to accept a single
options object (or an overload) and update the call site so you pass named
properties instead of positional undefineds—e.g. refactor io.archive.pack
signature to accept something like pack({ document, imageRecord, level }) and
then call it with merged.document and merged.imageRecord via that options object
(update references to io.archive.pack and its callers accordingly).
- Around line 594-632: The public API currently ignores the preserve flag
because restJsonToGridaDocument calls restJsonToMergedDocument with the
preserveFigmaIds argument hardcoded to undefined; update the
RestJsonToGridaOptions interface to include a preserve_figma_ids?: boolean (or
preserveFigmaIds) option and pass options?.preserve_figma_ids into the
restJsonToMergedDocument call instead of undefined (reference
RestJsonToGridaOptions, restJsonToGridaDocument and restJsonToMergedDocument),
or if the behavior is intentional, add a short comment in
restJsonToGridaDocument explaining why preserveFigmaIds is omitted rather than
wiring the option through.
In `@packages/grida-canvas-io/__tests__/format-roundtrip.test.ts`:
- Around line 2894-2908: Add a test that verifies an explicit empty stroke dash
array is preserved (not normalized to undefined): create a test similar to the
existing "roundtrips undefined stroke_dash_array (no dash)" but set the vector
node's stroke_dash_array to [] when building the document (using createDocument
and baseVector with nodeId) and call
roundtripTest<grida.program.nodes.VectorNode>(...) asserting
expect(node.stroke_dash_array).toEqual([]); this ensures roundtripTest,
createDocument and baseVector are exercised and [] is kept rather than collapsed
to undefined.
🪄 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: 36d02b24-8a35-47cc-ad5c-c71b64500e3a
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/grida-canvas-wasm/lib/bin/grida_canvas_wasm.wasmis excluded by!**/*.wasm
📒 Files selected for processing (103)
.agents/skills/cg-perf/SKILL.md.agents/skills/research/SKILL.md.claude/skills.gitignore.nvmrccrates/grida-canvas-wasm/lib/__test__/environment-node-api-spec-validation.test.tscrates/grida-canvas-wasm/lib/bin/grida-canvas-wasm.jscrates/grida-canvas-wasm/lib/index.tscrates/grida-canvas-wasm/lib/modules/canvas-bindings.d.tscrates/grida-canvas-wasm/lib/modules/canvas.tscrates/grida-canvas-wasm/package.jsoncrates/grida-canvas-wasm/src/wasm_application.rscrates/grida-canvas/Cargo.tomlcrates/grida-canvas/benches/bench_load_scene.rscrates/grida-canvas/benches/bench_mipmap.rscrates/grida-canvas/examples/fixture_helpers/mod.rscrates/grida-canvas/examples/fixtures/cover.rscrates/grida-canvas/examples/fixtures/l0_boolean_operation.rscrates/grida-canvas/examples/fixtures/l0_container.rscrates/grida-canvas/examples/fixtures/l0_effects.rscrates/grida-canvas/examples/fixtures/l0_effects_glass.rscrates/grida-canvas/examples/fixtures/l0_effects_progressive_blur.rscrates/grida-canvas/examples/fixtures/l0_group.rscrates/grida-canvas/examples/fixtures/l0_image.rscrates/grida-canvas/examples/fixtures/l0_image_filters.rscrates/grida-canvas/examples/fixtures/l0_layout_flex.rscrates/grida-canvas/examples/fixtures/l0_layout_position.rscrates/grida-canvas/examples/fixtures/l0_layout_transform.rscrates/grida-canvas/examples/fixtures/l0_masks.rscrates/grida-canvas/examples/fixtures/l0_paints.rscrates/grida-canvas/examples/fixtures/l0_paints_stack.rscrates/grida-canvas/examples/fixtures/l0_shape_arc.rscrates/grida-canvas/examples/fixtures/l0_shape_polygon.rscrates/grida-canvas/examples/fixtures/l0_shapes.rscrates/grida-canvas/examples/fixtures/l0_strokes.rscrates/grida-canvas/examples/fixtures/l0_strokes_rect.rscrates/grida-canvas/examples/fixtures/l0_strokes_varwidth.rscrates/grida-canvas/examples/fixtures/l0_type.rscrates/grida-canvas/examples/fixtures/l0_type_features.rscrates/grida-canvas/examples/fixtures/l0_type_fvar.rscrates/grida-canvas/examples/fixtures/l0_vector.rscrates/grida-canvas/examples/fixtures/mod.rscrates/grida-canvas/examples/golden_grain_noise_effect.rscrates/grida-canvas/examples/golden_outline_mode.rscrates/grida-canvas/examples/golden_pdf.rscrates/grida-canvas/examples/golden_pixel_preview.rscrates/grida-canvas/examples/golden_pixel_preview_stability.rscrates/grida-canvas/examples/golden_svg.rscrates/grida-canvas/examples/golden_svg_gradients.rscrates/grida-canvas/examples/golden_type_stroke.rscrates/grida-canvas/examples/tool_gen_cover.rscrates/grida-canvas/src/cache/mipmap.rscrates/grida-canvas/src/cache/mod.rscrates/grida-canvas/src/cache/paragraph.rscrates/grida-canvas/src/cache/scene.rscrates/grida-canvas/src/layout/cache.rscrates/grida-canvas/src/layout/engine.rscrates/grida-canvas/src/layout/tree.rscrates/grida-canvas/src/painter/image.rscrates/grida-canvas/src/painter/paint.rscrates/grida-canvas/src/painter/painter.rscrates/grida-canvas/src/runtime/config.rscrates/grida-canvas/src/runtime/image_repository.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/src/vectornetwork/vn_painter.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/src/window/application_emscripten.rscrates/grida-canvas/tests/mipmap.rscrates/grida-canvas/tests/resources_image_rid.rscrates/grida-dev/AGENTS.mdcrates/grida-dev/examples/sys_camera.rscrates/grida-dev/src/bench/args.rscrates/grida-dev/src/bench/load_bench.rscrates/grida-dev/src/bench/mod.rscrates/grida-dev/src/bench/runner.rscrates/grida-dev/src/main.rscrates/grida-dev/src/platform/native_demo.rscrates/grida-dev/src/reftest/render.rsdocs/wg/feat-2d/optimization.mdeditor/app/(embed)/embed/v1/refig/page.tsxeditor/grida-canvas-hosted/playground/uxhost-menu.tsxeditor/grida-canvas-react/renderer.tsxeditor/grida-canvas-react/use-embed-bridge.tseditor/grida-canvas-react/viewport/hotkeys.tsxeditor/grida-canvas/backends/dom-content.tseditor/grida-canvas/backends/wasm.tseditor/grida-canvas/editor.i.tseditor/grida-canvas/editor.tseditor/grida-canvas/embed-bridge.tseditor/scaffolds/embed/__tests__/decode-synthetic-figma-id.test.tseditor/scaffolds/embed/use-refig-editor.tseditor/scaffolds/sidecontrol/controls/ext-zoom.tsxpackages/grida-canvas-io-figma/__bench__/.gitignorepackages/grida-canvas-io-figma/__bench__/README.mdpackages/grida-canvas-io-figma/__bench__/fig2grida.bench.tspackages/grida-canvas-io-figma/__tests__/fig2grida.test.tspackages/grida-canvas-io-figma/fig2grida-core.tspackages/grida-canvas-io-figma/fig2grida.tspackages/grida-canvas-io-figma/lib.tspackages/grida-canvas-io-figma/package.jsonpackages/grida-canvas-io/__tests__/format-roundtrip.test.tspackages/grida-canvas-io/format.tspackages/grida-canvas-io/index.ts
💤 Files with no reviewable changes (3)
- crates/grida-canvas/src/cache/scene.rs
- crates/grida-canvas/src/cache/mod.rs
- crates/grida-canvas/src/cache/mipmap.rs
Comment on lines
+455
to
+463
| /** | ||
| * Get the absolute bounding box of a node or the active scene. | ||
| * | ||
| * @param target - A node ID, or `"<scene>"` to get the union bounds of the | ||
| * active scene's root children (computed in a single WASM call). | ||
| */ | ||
| getNodeAbsoluteBoundingBox( | ||
| target: (string & {}) | "<scene>" | ||
| ): types.Rect | null { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
while IFS= read -r file; do
echo "== $file =="
rg -n "getNodeAbsoluteBoundingBox|runtime_renderer_set_skip_layout|_runtime_renderer_set_skip_layout|<scene>" "$file" || true
done < <(
fd -t f 'grida-canvas-wasm\.d\.ts$|canvas-bindings\.d\.ts$|canvas\.ts$|index\.ts$' crates/grida-canvas-wasm
)Repository: gridaco/grida
Length of output: 113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding .d.ts files in crates/grida-canvas-wasm ==="
fd -t f '\.d\.ts$' crates/grida-canvas-wasm
echo ""
echo "=== Searching for getNodeAbsoluteBoundingBox in .d.ts files ==="
fd -t f '\.d\.ts$' crates/grida-canvas-wasm -x rg -l "getNodeAbsoluteBoundingBox" {} \;
echo ""
echo "=== Searching for runtime_renderer_set_skip_layout in .d.ts files ==="
fd -t f '\.d\.ts$' crates/grida-canvas-wasm -x rg -l "runtime_renderer_set_skip_layout" {} \;
echo ""
echo "=== Full content of main .d.ts file (if exists) ==="
if [ -f "crates/grida-canvas-wasm/grida-canvas-wasm.d.ts" ]; then
cat crates/grida-canvas-wasm/grida-canvas-wasm.d.ts
else
echo "grida-canvas-wasm.d.ts not found at root of crates/grida-canvas-wasm"
fi
echo ""
echo "=== Checking lib/index.ts for exports ==="
if [ -f "crates/grida-canvas-wasm/lib/index.ts" ]; then
cat crates/grida-canvas-wasm/lib/index.ts
fiRepository: gridaco/grida
Length of output: 603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Content of canvas-bindings.d.ts ==="
cat crates/grida-canvas-wasm/lib/modules/canvas-bindings.d.ts
echo ""
echo "=== Searching for runtime_renderer_set_skip_layout ==="
rg "runtime_renderer_set_skip_layout|_runtime_renderer_set_skip_layout" crates/grida-canvas-wasm/ || echo "NOT FOUND"
echo ""
echo "=== Check what getNodeAbsoluteBoundingBox looks like in canvas.ts ==="
sed -n '450,470p' crates/grida-canvas-wasm/lib/modules/canvas.ts
echo ""
echo "=== Check what runtime_renderer_set_skip_layout looks like in canvas.ts ==="
sed -n '715,740p' crates/grida-canvas-wasm/lib/modules/canvas.tsRepository: gridaco/grida
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== canvas.ts: getNodeAbsoluteBoundingBox implementation ==="
sed -n '450,475p' crates/grida-canvas-wasm/lib/modules/canvas.ts
echo ""
echo "=== canvas.ts: runtime_renderer_set_skip_layout implementation ==="
sed -n '715,745p' crates/grida-canvas-wasm/lib/modules/canvas.ts
echo ""
echo "=== Content of api.d.ts ==="
cat crates/grida-canvas-wasm/lib/api.d.ts 2>/dev/null || echo "File not found"
echo ""
echo "=== Content of grida-canvas-wasm.d.ts (if in lib root) ==="
cat crates/grida-canvas-wasm/lib/grida-canvas-wasm.d.ts 2>/dev/null || echo "File not found"
echo ""
echo "=== Search for Scene class/interface declaration ==="
rg -n "class Scene|interface Scene" crates/grida-canvas-wasm/lib/ -A 5 | head -40Repository: gridaco/grida
Length of output: 10267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check lib/index.ts to see what's exported ==="
cat crates/grida-canvas-wasm/lib/index.ts
echo ""
echo "=== Check tsup.config.ts to see declaration generation ==="
cat crates/grida-canvas-wasm/tsup.config.ts
echo ""
echo "=== Check if there's a .d.ts in lib/modules ==="
ls -la crates/grida-canvas-wasm/lib/modules/*.d.ts 2>/dev/null | head -20
echo ""
echo "=== Check package.json for types field ==="
grep -A 5 '"types"' crates/grida-canvas-wasm/package.json || echo "No types field found"Repository: gridaco/grida
Length of output: 12644
Regenerate TypeScript declarations to sync with API changes.
Scene.getNodeAbsoluteBoundingBox() now accepts the "<scene>" sentinel, and Scene.runtime_renderer_set_skip_layout() is a new public method. The source code in canvas.ts is correct, and the C-level bindings in canvas-bindings.d.ts are properly declared. However, the generated TypeScript declaration files (dist/index.d.ts) need to be regenerated by running the build (tsup with dts: true is configured in tsup.config.ts). Without regenerating these declarations, TypeScript consumers will compile against stale type information.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas-wasm/lib/modules/canvas.ts` around lines 455 - 463, The
generated TypeScript declarations are stale: update the .d.ts outputs so the new
API (Scene.getNodeAbsoluteBoundingBox accepting "<scene>" and the new
Scene.runtime_renderer_set_skip_layout method) is reflected; regenerate
declarations by running the project's build that invokes tsup with dts enabled
(as configured in tsup.config.ts) so dist/index.d.ts is rebuilt from the updated
canvas.ts and canvas-bindings.d.ts sources, then verify dist/index.d.ts contains
the updated signatures for getNodeAbsoluteBoundingBox and
runtime_renderer_set_skip_layout.
Comment on lines
+152
to
+198
| /// Produce layout results directly from schema positions and sizes, | ||
| /// bypassing the Taffy layout engine entirely. | ||
| /// | ||
| /// This is a fast path for documents where all nodes are absolutely | ||
| /// positioned (e.g. Figma imports without auto-layout). Each node's | ||
| /// schema position and size are copied verbatim into the layout result. | ||
| /// | ||
| /// Only nodes reachable from the scene's roots are included (same | ||
| /// scope as the full Taffy path). | ||
| pub fn compute_schema_only(&mut self, scene: &crate::node::schema::Scene) -> &LayoutResult { | ||
| self.result.clear(); | ||
| let graph = &scene.graph; | ||
| let node_count = graph.node_count(); | ||
| self.result.reserve(node_count); | ||
|
|
||
| let roots: Vec<NodeId> = graph.roots().to_vec(); | ||
| for root_id in &roots { | ||
| self.extract_schema_only_recursive(root_id, graph); | ||
| } | ||
| &self.result | ||
| } | ||
|
|
||
| /// Recursively extract schema positions/sizes for a node and its children. | ||
| fn extract_schema_only_recursive( | ||
| &mut self, | ||
| id: &NodeId, | ||
| graph: &crate::node::scene_graph::SceneGraph, | ||
| ) { | ||
| if let Ok(node) = graph.get_node(id) { | ||
| let (x, y) = Self::get_schema_position(node); | ||
| let (width, height) = Self::get_schema_size(node); | ||
| self.result.insert( | ||
| *id, | ||
| ComputedLayout { | ||
| x, | ||
| y, | ||
| width, | ||
| height, | ||
| }, | ||
| ); | ||
| } | ||
| if let Some(children) = graph.get_children(id) { | ||
| for child_id in children { | ||
| self.extract_schema_only_recursive(child_id, graph); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Pass the viewport into compute_schema_only() for InitialContainer roots.
This fast path never receives viewport_size, so InitialContainer still resolves to 0×0 via get_schema_size(). With skip_layout enabled on any ICB-based scene, the root collapses and descendants get the wrong containing block.
Possible fix
- pub fn compute_schema_only(&mut self, scene: &crate::node::schema::Scene) -> &LayoutResult {
+ pub fn compute_schema_only(
+ &mut self,
+ scene: &crate::node::schema::Scene,
+ viewport_size: Size,
+ ) -> &LayoutResult {
self.result.clear();
let graph = &scene.graph;
let node_count = graph.node_count();
self.result.reserve(node_count);
let roots: Vec<NodeId> = graph.roots().to_vec();
for root_id in &roots {
- self.extract_schema_only_recursive(root_id, graph);
+ self.extract_schema_only_recursive(root_id, graph, viewport_size);
}
&self.result
}
fn extract_schema_only_recursive(
&mut self,
id: &NodeId,
graph: &crate::node::scene_graph::SceneGraph,
+ viewport_size: Size,
) {
if let Ok(node) = graph.get_node(id) {
let (x, y) = Self::get_schema_position(node);
- let (width, height) = Self::get_schema_size(node);
+ let (width, height) = match node {
+ Node::InitialContainer(_) => (viewport_size.width, viewport_size.height),
+ _ => Self::get_schema_size(node),
+ };
self.result.insert(
*id,
ComputedLayout {
@@
}
if let Some(children) = graph.get_children(id) {
for child_id in children {
- self.extract_schema_only_recursive(child_id, graph);
+ self.extract_schema_only_recursive(child_id, graph, viewport_size);
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/layout/engine.rs` around lines 152 - 198,
compute_schema_only currently never receives viewport size so nodes like
InitialContainer resolve to 0×0; change compute_schema_only signature to accept
a viewport_size (e.g. Option<(f32,f32)> or a Size reference) and thread that
viewport through extract_schema_only_recursive when resolving sizes; update
extract_schema_only_recursive to accept the viewport parameter and call
get_schema_size(node, viewport_size) (or otherwise make get_schema_size read the
passed viewport) so InitialContainer roots use the scene viewport when computing
width/height; ensure callers of compute_schema_only (and any tests) pass the
appropriate viewport from Scene when available.
Comment on lines
+835
to
+845
| /// Enable or disable layout computation during `load_scene`. | ||
| /// | ||
| /// When `skip` is true, `load_scene` bypasses the Taffy flexbox engine | ||
| /// and derives layout results directly from each node's schema position | ||
| /// and size. This is appropriate for documents with only absolute | ||
| /// positioning (e.g. imported Figma files without auto-layout). | ||
| /// | ||
| /// This flag must be set **before** calling `load_scene` / `switch_scene`. | ||
| pub fn set_skip_layout(&mut self, skip: bool) { | ||
| self.config.skip_layout = skip; | ||
| } |
There was a problem hiding this comment.
set_skip_layout() does not affect an already-loaded scene.
This only flips self.config.skip_layout. If a scene is already loaded, the new WASM runtime setter leaves the existing layout/geometry/layers intact until a later load_scene() or explicit rebuild_scene_caches(), so the API looks live but behaves like a no-op. Either rebuild and requeue here when the value changes, or make the public surface explicitly pre-load-only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/runtime/scene.rs` around lines 835 - 845,
set_skip_layout currently only mutates self.config.skip_layout and is a no-op
for an already-loaded scene; change it to check the previous value and if it
actually changed and a scene is loaded, call the scene cache/layout refresh
(invoke rebuild_scene_caches()) and enqueue a layout/scene update so the runtime
reapplies the new skip_layout behavior immediately (alternatively call
load_scene/switch_scene flow if that’s the canonical refresh path); ensure you
only trigger this when old != skip to avoid redundant work and preserve any
required locking/ordering semantics.
Comment on lines
+45
to
+53
| impl<'a> BoundsTarget<'a> { | ||
| /// Parse from a string, recognizing `"<scene>"` as the scene target. | ||
| pub fn from_str(s: &'a str) -> Self { | ||
| if s == "<scene>" { | ||
| Self::Scene | ||
| } else { | ||
| Self::Node(s) | ||
| } | ||
| } |
There was a problem hiding this comment.
"<scene>" now steals a valid external node ID.
Any real UserNodeId equal to "<scene>" becomes unreachable through the string-based host APIs because from_str() rewrites it to the sentinel. Please keep scene targeting out of the user-ID namespace at the boundary.
As per coding guidelines, use UserNodeId (String) for public APIs to provide stable, serializable external identifiers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/src/window/application.rs` around lines 45 - 53,
BoundsTarget::from_str currently interprets the literal "<scene>" as the
internal Scene sentinel and thus collides with real user IDs; change the public
boundary so string-based parsing accepts a UserNodeId (String) and never maps
any user-supplied string to the Scene sentinel. Concretely, stop special-casing
"<scene>" inside BoundsTarget::from_str — make from_str accept only a UserNodeId
(or rename to from_user_id) and return BoundsTarget::Node(UserNodeId) for all
inputs; provide a separate constructor or explicit API (e.g.,
BoundsTarget::scene() or a distinct enum/variant used only internally) to
construct the Scene sentinel so that user-visible IDs are preserved.
Comment on lines
+252
to
+263
| /** | ||
| * Figma IDs have several formats: | ||
| * - Simple: `{sessionID}:{localID}` (e.g. "1:2", "42:17") | ||
| * - Instance: `I{id};{id}` (e.g. "I1620:1441;291:113") | ||
| * | ||
| * Synthetic children append: `_fill_{N}` or `_stroke_{N}`. | ||
| * Instance clones use: `{prefix}::{counter}::{originalId}`. | ||
| * | ||
| * This regex matches any ID that is or derives from a Figma-format ID. | ||
| */ | ||
| const FIGMA_ID_RE = /^(I?\d+:\d+|I\d+:\d+;[\d:;]+)/; | ||
|
|
There was a problem hiding this comment.
Tighten the Figma ID matcher.
FIGMA_ID_RE only validates a prefix, so malformed values like 42:17_garbage or truncated instance IDs still satisfy the preserve-ID assertions below. Normalize clone/synthetic wrappers first and then match the fully anchored base ID.
💡 Possible fix
- const FIGMA_ID_RE = /^(I?\d+:\d+|I\d+:\d+;[\d:;]+)/;
+ const BASE_FIGMA_ID_RE = /^(?:\d+:\d+|I\d+:\d+(?:;\d+:\d+)+)$/;
+ const normalizeDerivedFigmaId = (id: string) => {
+ const withoutSynthetic = id.replace(/_(fill|stroke)_\d+$/, "");
+ return withoutSynthetic.includes("::")
+ ? withoutSynthetic.split("::").at(-1)!
+ : withoutSynthetic;
+ };expect(normalizeDerivedFigmaId(id)).toMatch(BASE_FIGMA_ID_RE);📝 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
| /** | |
| * Figma IDs have several formats: | |
| * - Simple: `{sessionID}:{localID}` (e.g. "1:2", "42:17") | |
| * - Instance: `I{id};{id}` (e.g. "I1620:1441;291:113") | |
| * | |
| * Synthetic children append: `_fill_{N}` or `_stroke_{N}`. | |
| * Instance clones use: `{prefix}::{counter}::{originalId}`. | |
| * | |
| * This regex matches any ID that is or derives from a Figma-format ID. | |
| */ | |
| const FIGMA_ID_RE = /^(I?\d+:\d+|I\d+:\d+;[\d:;]+)/; | |
| /** | |
| * Figma IDs have several formats: | |
| * - Simple: `{sessionID}:{localID}` (e.g. "1:2", "42:17") | |
| * - Instance: `I{id};{id}` (e.g. "I1620:1441;291:113") | |
| * | |
| * Synthetic children append: `_fill_{N}` or `_stroke_{N}`. | |
| * Instance clones use: `{prefix}::{counter}::{originalId}`. | |
| * | |
| * This regex matches any ID that is or derives from a Figma-format ID. | |
| */ | |
| const BASE_FIGMA_ID_RE = /^(?:\d+:\d+|I\d+:\d+(?:;\d+:\d+)+)$/; | |
| const normalizeDerivedFigmaId = (id: string) => { | |
| const withoutSynthetic = id.replace(/_(fill|stroke)_\d+$/, ""); | |
| return withoutSynthetic.includes("::") | |
| ? withoutSynthetic.split("::").at(-1)! | |
| : withoutSynthetic; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io-figma/__tests__/fig2grida.test.ts` around lines 252
- 263, The current FIGMA_ID_RE only matches a prefix allowing invalid IDs;
update the tests to first call normalizeDerivedFigmaId(id) (or the function that
strips clone/synthetic wrappers) and then assert the result matches a fully
anchored base ID regex (rename/introduce BASE_FIGMA_ID_RE that exactly matches
formats like /^\d+:\d+$|^I\d+:\d+(?:;[\d:]+)?$/), and replace uses of
FIGMA_ID_RE in the preserve-ID assertions with BASE_FIGMA_ID_RE to ensure the
entire normalized ID is validated.
- Introduced `ChangeFlags` and `ChangeSet` to manage change notifications across the renderer, improving cache invalidation efficiency. - Updated the `Renderer` to utilize the new change tracking system, replacing ad-hoc cache invalidation with a centralized approach. - Enhanced methods for marking changes, including `mark_changed` and `mark_node_changed`, to streamline the invalidation process. - Refactored existing cache invalidation logic to leverage the new system, reducing potential bugs related to over- and under-invalidation. - Updated relevant documentation and tests to reflect the new change tracking functionality.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/grida-canvas/src/runtime/scene.rs (1)
838-848: Design is intentional per documentation.The
set_skip_layoutmethod is documented as a pre-load configuration that "must be set before callingload_scene/switch_scene". This addresses the concern from previous review—the API is explicitly not designed to affect already-loaded scenes. Consider adding a debug assertion or log warning if called while a scene is already loaded to catch misuse during development.🛡️ Optional: Add debug warning for misuse
pub fn set_skip_layout(&mut self, skip: bool) { + #[cfg(debug_assertions)] + if self.scene.is_some() && self.config.skip_layout != skip { + eprintln!("warning: set_skip_layout called after scene load; change will only affect next load_scene call"); + } self.config.skip_layout = skip; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/runtime/scene.rs` around lines 838 - 848, The setter set_skip_layout currently changes self.config.skip_layout but can be misused after a scene is loaded; add a development-time safeguard so callers are warned if they call set_skip_layout after loading. Modify set_skip_layout to emit a debug_assert! (or debug_assert_eq!) ensuring no scene is currently loaded (e.g., check your scene presence flag like self.scene.is_some() / self.scene_loaded) and/or call log::warn! when that check fails; keep the existing assignment to self.config.skip_layout but ensure the assertion/warning references set_skip_layout, load_scene and switch_scene to make the intended ordering clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/grida-canvas/src/runtime/scene.rs`:
- Around line 838-848: The setter set_skip_layout currently changes
self.config.skip_layout but can be misused after a scene is loaded; add a
development-time safeguard so callers are warned if they call set_skip_layout
after loading. Modify set_skip_layout to emit a debug_assert! (or
debug_assert_eq!) ensuring no scene is currently loaded (e.g., check your scene
presence flag like self.scene.is_some() / self.scene_loaded) and/or call
log::warn! when that check fails; keep the existing assignment to
self.config.skip_layout but ensure the assertion/warning references
set_skip_layout, load_scene and switch_scene to make the intended ordering
clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a804934b-d885-4b9a-940d-6f856fcf4e94
📒 Files selected for processing (7)
crates/grida-canvas/src/runtime/changes.rscrates/grida-canvas/src/runtime/mod.rscrates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/src/window/application_emscripten.rscrates/grida-dev/src/bench/runner.rseditor/grida-canvas/editor.ts
✅ Files skipped from review due to trivial changes (1)
- crates/grida-canvas/src/runtime/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/grida-dev/src/bench/runner.rs
- editor/grida-canvas/editor.ts
- crates/grida-canvas/src/window/application_emscripten.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/grida-canvas/benches/bench_load_scene.rs (1)
335-343: Minor:SceneCache::new()allocation included in geometry/effects measurements.Creating an empty
SceneCacheinsideb.iter()includes allocation overhead. For geometry benchmarks this is minimal compared to the actual computation, but for consistency with theload_scenebenchmark pattern, consider usingiter_batchedif cold-start allocation is not the intent.♻️ Optional: Use iter_batched for consistency
For the geometry benchmark:
b.iter(|| { - let mut scene_cache = cache::scene::SceneCache::new(); - scene_cache.update_geometry_with_layout( + b.iter_batched( + cache::scene::SceneCache::new, + |mut scene_cache| { + scene_cache.update_geometry_with_layout( black_box(scene), &fonts, layout_result, viewport_size(), - ); - }); + ); + }, + criterion::BatchSize::SmallInput, + );Also applies to: 364-367
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/benches/bench_load_scene.rs` around lines 335 - 343, The benchmark currently constructs a fresh SceneCache inside b.iter(), which includes allocation overhead; change to use Criterion's iter_batched (or iter_batched_ref) so SceneCache::new() runs in the setup closure and update_geometry_with_layout(...) is the measured closure (e.g., pass a closure that creates the SceneCache in the setup and a closure that calls cache.update_geometry_with_layout(black_box(scene), &fonts, layout_result, viewport_size()) in the measurement), and apply the same refactor for the other similar block that constructs SceneCache in-loop; reference SceneCache::new, update_geometry_with_layout, b.iter and iter_batched when making the change.packages/grida-canvas-io-figma/__bench__/fig2grida.bench.ts (1)
103-115: Consider adding inline comments for positionalundefinedarguments.The two
undefinedarguments (forschemaVersionandbitmaps) before{ level: 0 }are easy to misread. A brief comment or using named variables would improve clarity.💡 Suggested improvement
bench( "stage: io.archive.pack (level 0, store)", () => { io.archive.pack( preConverted!.document, preConverted!.assets, - undefined, - undefined, + undefined /* schemaVersion */, + undefined /* bitmaps */, { level: 0 } ); }, { iterations: 3, warmupIterations: 1 } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-canvas-io-figma/__bench__/fig2grida.bench.ts` around lines 103 - 115, The call to io.archive.pack passes two positional undefineds (schemaVersion and bitmaps) which harms readability; update the call in the bench block to either replace those undefineds with clearly named variables (e.g., const schemaVersion = undefined; const bitmaps = undefined;) or add inline comments after each undefined (/* schemaVersion */, /* bitmaps */) so it's obvious what arguments correspond to schemaVersion and bitmaps when calling io.archive.pack(preConverted!.document, preConverted!.assets, undefined /* schemaVersion */, undefined /* bitmaps */, { level: 0 });
🤖 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/benches/bench_load_scene.rs`:
- Around line 410-414: The benchmark is cloning SceneCache inside the
measurement loop which skews timings; change the bench to use criterion's
iter_batched (or iter_batched_ref) so the expensive scene_cache.clone() happens
in the setup phase and only SceneCache::update_layers (the update_layers method
called on sc) is measured; locate the closure that currently calls
scene_cache.clone() and update_layers (the b.iter block) and replace it with a
setup closure that clones scene_cache once per iteration and a measurement
closure that calls sc.update_layers(black_box(scene)), using
BatchSize::SmallInput or appropriate size.
In `@editor/scaffolds/embed/use-refig-editor.ts`:
- Around line 290-316: The INSTANCE_CLONE_RE uses non-greedy prefix and
therefore captures after the first "::" instead of the last; update
INSTANCE_CLONE_RE to use a greedy prefix so it captures everything after the
last "::" (e.g. change /^.+?::\d+::(.+)$/ to /^.*::\d+::(.+)$/ or an equivalent
greedy pattern), keep the rest of decodeSyntheticFigmaId unchanged (function
name: decodeSyntheticFigmaId, constant: INSTANCE_CLONE_RE) and run related tests
to verify nested instance-clone IDs like "outer::0::inner::1::5:3" decode to
"5:3".
In `@packages/grida-canvas-io-figma/__bench__/fig2grida.bench.ts`:
- Around line 122-145: The file uses preConverted!.document at describe scope
which will throw when the fixture is missing; relocate or guard that logic:
either move creation of docForFb, persistedDocument/snapshotPayload and the call
to format.document.encode.toFlatbuffer inside the bench callbacks (so they only
run when the bench executes), or wrap the describe body in a runtime check of
HAS_FIXTURE and return early (or skip) to avoid evaluating preConverted;
specifically adjust the code that builds docForFb, the destructuring that
produces persistedDocument, and the snapshotPayload so they only access
preConverted when HAS_FIXTURE is true (or move into the bench callback where
format.document.encode.toFlatbuffer is invoked).
---
Nitpick comments:
In `@crates/grida-canvas/benches/bench_load_scene.rs`:
- Around line 335-343: The benchmark currently constructs a fresh SceneCache
inside b.iter(), which includes allocation overhead; change to use Criterion's
iter_batched (or iter_batched_ref) so SceneCache::new() runs in the setup
closure and update_geometry_with_layout(...) is the measured closure (e.g., pass
a closure that creates the SceneCache in the setup and a closure that calls
cache.update_geometry_with_layout(black_box(scene), &fonts, layout_result,
viewport_size()) in the measurement), and apply the same refactor for the other
similar block that constructs SceneCache in-loop; reference SceneCache::new,
update_geometry_with_layout, b.iter and iter_batched when making the change.
In `@packages/grida-canvas-io-figma/__bench__/fig2grida.bench.ts`:
- Around line 103-115: The call to io.archive.pack passes two positional
undefineds (schemaVersion and bitmaps) which harms readability; update the call
in the bench block to either replace those undefineds with clearly named
variables (e.g., const schemaVersion = undefined; const bitmaps = undefined;) or
add inline comments after each undefined (/* schemaVersion */, /* bitmaps */) so
it's obvious what arguments correspond to schemaVersion and bitmaps when calling
io.archive.pack(preConverted!.document, preConverted!.assets, undefined /*
schemaVersion */, undefined /* bitmaps */, { level: 0 });
🪄 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: c714157f-6f34-431c-9a8c-8da28968e295
⛔ Files ignored due to path filters (1)
crates/grida-canvas-wasm/lib/bin/grida_canvas_wasm.wasmis excluded by!**/*.wasm
📒 Files selected for processing (14)
crates/grida-canvas-wasm/lib/bin/grida-canvas-wasm.jscrates/grida-canvas-wasm/package.jsoncrates/grida-canvas/benches/bench_load_scene.rscrates/grida-canvas/tests/mipmap.rscrates/grida-dev/src/bench/load_bench.rscrates/grida-dev/src/bench/runner.rsdocs/wg/feat-2d/optimization.mdeditor/grida-canvas/embed-bridge.tseditor/scaffolds/embed/use-refig-editor.tspackages/grida-canvas-io-figma/__bench__/README.mdpackages/grida-canvas-io-figma/__bench__/fig2grida.bench.tspackages/grida-canvas-io-figma/__tests__/fig2grida.test.tspackages/grida-canvas-io-figma/fig2grida-core.tspackages/grida-canvas-io-figma/fig2grida.ts
✅ Files skipped from review due to trivial changes (5)
- crates/grida-canvas-wasm/package.json
- packages/grida-canvas-io-figma/bench/README.md
- packages/grida-canvas-io-figma/fig2grida.ts
- packages/grida-canvas-io-figma/tests/fig2grida.test.ts
- crates/grida-dev/src/bench/load_bench.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- editor/grida-canvas/embed-bridge.ts
- crates/grida-dev/src/bench/runner.rs
- docs/wg/feat-2d/optimization.md
- packages/grida-canvas-io-figma/fig2grida-core.ts
Comment on lines
+410
to
+414
| b.iter(|| { | ||
| // Clone so each iteration rebuilds layers | ||
| let mut sc = scene_cache.clone(); | ||
| sc.update_layers(black_box(scene)); | ||
| }); |
There was a problem hiding this comment.
Expensive scene_cache.clone() inside measurement loop skews update_layers timings.
Per the SceneCache definition, cloning deep-copies HashMap<NodeId, Picture>, HashMap<NodeId, GeometryEntry>, Vec<LayerEntry>, and RTree<IndexedLayer>. At 100k nodes, this clone cost likely dominates the actual update_layers work being measured.
Use iter_batched to move the clone into the setup phase:
🔧 Proposed fix to isolate update_layers measurement
b.iter(|| {
- // Clone so each iteration rebuilds layers
- let mut sc = scene_cache.clone();
- sc.update_layers(black_box(scene));
- });
+ b.iter_batched(
+ || scene_cache.clone(),
+ |mut sc| {
+ sc.update_layers(black_box(scene));
+ },
+ criterion::BatchSize::LargeInput,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/benches/bench_load_scene.rs` around lines 410 - 414, The
benchmark is cloning SceneCache inside the measurement loop which skews timings;
change the bench to use criterion's iter_batched (or iter_batched_ref) so the
expensive scene_cache.clone() happens in the setup phase and only
SceneCache::update_layers (the update_layers method called on sc) is measured;
locate the closure that currently calls scene_cache.clone() and update_layers
(the b.iter block) and replace it with a setup closure that clones scene_cache
once per iteration and a measurement closure that calls
sc.update_layers(black_box(scene)), using BatchSize::SmallInput or appropriate
size.
Comment on lines
+290
to
+316
| const INSTANCE_CLONE_RE = /^.+?::\d+::(.+)$/; | ||
|
|
||
| /** | ||
| * Decode a Grida node ID that may contain synthetic suffixes back to the | ||
| * closest real Figma node ID. This is refig-specific logic and should only | ||
| * be used in the embed/refig context where `preserve_figma_ids` is true. | ||
| * | ||
| * - `"42:17"` → `"42:17"` (real node, unchanged) | ||
| * - `"42:17_fill_0"` → `"42:17"` (synthetic fill child → parent) | ||
| * - `"42:17_stroke_1"` → `"42:17"` (synthetic stroke child → parent) | ||
| * - `"42:17::0::5:3"` → `"5:3"` (instance clone → original) | ||
| * - `"42:17::0::5:3_fill_0"` → `"5:3"` (instance clone + synthetic → original) | ||
| * - `"scene-1"` → `"scene-1"` (non-Figma ID, unchanged) | ||
| */ | ||
| export function decodeSyntheticFigmaId(id: string): string { | ||
| let decoded = id; | ||
|
|
||
| // Strip instance-clone prefix: `{prefix}::{counter}::{originalId}` → `{originalId}` | ||
| // The `::` delimiter never appears in Figma IDs, so its presence is | ||
| // unambiguous. We take everything after the last `::`. | ||
| const cloneMatch = decoded.match(INSTANCE_CLONE_RE); | ||
| if (cloneMatch) { | ||
| decoded = cloneMatch[1]; | ||
| } | ||
|
|
||
| // Strip synthetic geometry suffix | ||
| decoded = decoded.replace(SYNTHETIC_SUFFIX_RE, ""); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the current regex captures the wrong segment for nested clone IDs.
node <<'NODE'
const current = /^.+?::\d+::(.+)$/;
const fixed = /^.*::\d+::(.+)$/;
const sample = "outer::0::inner::1::5:3_fill_0";
console.log("current:", sample.match(current)?.[1]);
console.log("fixed: ", sample.match(fixed)?.[1]);
NODERepository: gridaco/grida
Length of output: 106
🏁 Script executed:
# First, let's examine the full decodeSyntheticFigmaId function and surrounding code
sed -n '285,330p' editor/scaffolds/embed/use-refig-editor.tsRepository: gridaco/grida
Length of output: 1488
🏁 Script executed:
# Check if SYNTHETIC_SUFFIX_RE is defined and what it matches
rg "SYNTHETIC_SUFFIX_RE" editor/scaffolds/embed/use-refig-editor.ts -B 2 -A 2Repository: gridaco/grida
Length of output: 283
🏁 Script executed:
# Search for usage of decodeSyntheticFigmaId to understand how it's applied
rg "decodeSyntheticFigmaId" --type ts --type tsxRepository: gridaco/grida
Length of output: 84
🏁 Script executed:
# Search for nested clone patterns in the codebase to see if they exist
rg "::\d+::" --type ts --type tsx | head -20Repository: gridaco/grida
Length of output: 84
🏁 Script executed:
# Check for EmbedBridge and its usage of this function
rg "EmbedBridge" --type ts --type tsx -A 5 | head -40Repository: gridaco/grida
Length of output: 84
🏁 Script executed:
# Search for usage of decodeSyntheticFigmaId
rg "decodeSyntheticFigmaId" -A 3 -B 2Repository: gridaco/grida
Length of output: 7114
🏁 Script executed:
# Look for where instance clone IDs are created with :: pattern
rg "::\d+::" -B 2 -A 2 | head -50Repository: gridaco/grida
Length of output: 1884
🏁 Script executed:
# Search for EmbedBridge
rg "EmbedBridge" -A 5 | head -60Repository: gridaco/grida
Length of output: 4062
🏁 Script executed:
# Look for test files related to this function
fd ".*test.*" editor/scaffolds/embed/ | head -20Repository: gridaco/grida
Length of output: 157
🏁 Script executed:
# Check if there are any examples or tests for decodeSyntheticFigmaId
rg "decodeSyntheticFigmaId" -C 5Repository: gridaco/grida
Length of output: 8442
🏁 Script executed:
# Search for where instance clone IDs are constructed (the :: pattern)
rg "::" --type ts -B 2 -A 2 | grep -E "(::|\{prefix\}|counter)" | head -40Repository: gridaco/grida
Length of output: 5047
🏁 Script executed:
# Look specifically for clone ID creation logic
rg "clone.*ID\|INSTANCE_CLONE\|::.*::" -B 3 -A 3 | head -60Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Check refig editor implementation for clone handling
cat -n editor/scaffolds/embed/use-refig-editor.ts | sed -n '1,50p'Repository: gridaco/grida
Length of output: 1866
🏁 Script executed:
# Search for nested node cloning logic
rg "instance.*clone\|clone.*instance" -i | head -30Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look at the clone ID creation in the IO library
cat -n packages/grida-canvas-io-figma/lib.ts | grep -B 5 -A 5 "newId = \`"Repository: gridaco/grida
Length of output: 551
🏁 Script executed:
# Check if originalId can itself be a clone ID (nested possibility)
rg "originalId.*::|cloneId.*::" -B 3 -A 3Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
# Look for any recursive or nested clone handling
rg "clone.*clone|nested.*clone" -iRepository: gridaco/grida
Length of output: 50370
🏁 Script executed:
# Verify the current test file completely to see if there's any nested test
cat editor/scaffolds/embed/__tests__/decode-synthetic-figma-id.test.tsRepository: gridaco/grida
Length of output: 2183
Fix regex to match comment intent: capture from the last :: separator, not the first.
The regex /^.+?::\d+::(.+)$/ uses non-greedy matching (.+?), which stops at the first ::<counter>::. This contradicts the function's documented behavior: "We take everything after the last ::." For nested instance clones like outer::0::inner::1::5:3, the current regex returns inner::1::5:3 instead of 5:3, leaking synthetic IDs when EmbedBridge transforms outgoing node and scene IDs.
Proposed fix
-const INSTANCE_CLONE_RE = /^.+?::\d+::(.+)$/;
+const INSTANCE_CLONE_RE = /^.*::\d+::(.+)$/;📝 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
| const INSTANCE_CLONE_RE = /^.+?::\d+::(.+)$/; | |
| /** | |
| * Decode a Grida node ID that may contain synthetic suffixes back to the | |
| * closest real Figma node ID. This is refig-specific logic and should only | |
| * be used in the embed/refig context where `preserve_figma_ids` is true. | |
| * | |
| * - `"42:17"` → `"42:17"` (real node, unchanged) | |
| * - `"42:17_fill_0"` → `"42:17"` (synthetic fill child → parent) | |
| * - `"42:17_stroke_1"` → `"42:17"` (synthetic stroke child → parent) | |
| * - `"42:17::0::5:3"` → `"5:3"` (instance clone → original) | |
| * - `"42:17::0::5:3_fill_0"` → `"5:3"` (instance clone + synthetic → original) | |
| * - `"scene-1"` → `"scene-1"` (non-Figma ID, unchanged) | |
| */ | |
| export function decodeSyntheticFigmaId(id: string): string { | |
| let decoded = id; | |
| // Strip instance-clone prefix: `{prefix}::{counter}::{originalId}` → `{originalId}` | |
| // The `::` delimiter never appears in Figma IDs, so its presence is | |
| // unambiguous. We take everything after the last `::`. | |
| const cloneMatch = decoded.match(INSTANCE_CLONE_RE); | |
| if (cloneMatch) { | |
| decoded = cloneMatch[1]; | |
| } | |
| // Strip synthetic geometry suffix | |
| decoded = decoded.replace(SYNTHETIC_SUFFIX_RE, ""); | |
| const INSTANCE_CLONE_RE = /^.*::\d+::(.+)$/; | |
| /** | |
| * Decode a Grida node ID that may contain synthetic suffixes back to the | |
| * closest real Figma node ID. This is refig-specific logic and should only | |
| * be used in the embed/refig context where `preserve_figma_ids` is true. | |
| * | |
| * - `"42:17"` → `"42:17"` (real node, unchanged) | |
| * - `"42:17_fill_0"` → `"42:17"` (synthetic fill child → parent) | |
| * - `"42:17_stroke_1"` → `"42:17"` (synthetic stroke child → parent) | |
| * - `"42:17::0::5:3"` → `"5:3"` (instance clone → original) | |
| * - `"42:17::0::5:3_fill_0"` → `"5:3"` (instance clone + synthetic → original) | |
| * - `"scene-1"` → `"scene-1"` (non-Figma ID, unchanged) | |
| */ | |
| export function decodeSyntheticFigmaId(id: string): string { | |
| let decoded = id; | |
| // Strip instance-clone prefix: `{prefix}::{counter}::{originalId}` → `{originalId}` | |
| // The `::` delimiter never appears in Figma IDs, so its presence is | |
| // unambiguous. We take everything after the last `::`. | |
| const cloneMatch = decoded.match(INSTANCE_CLONE_RE); | |
| if (cloneMatch) { | |
| decoded = cloneMatch[1]; | |
| } | |
| // Strip synthetic geometry suffix | |
| decoded = decoded.replace(SYNTHETIC_SUFFIX_RE, ""); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/scaffolds/embed/use-refig-editor.ts` around lines 290 - 316, The
INSTANCE_CLONE_RE uses non-greedy prefix and therefore captures after the first
"::" instead of the last; update INSTANCE_CLONE_RE to use a greedy prefix so it
captures everything after the last "::" (e.g. change /^.+?::\d+::(.+)$/ to
/^.*::\d+::(.+)$/ or an equivalent greedy pattern), keep the rest of
decodeSyntheticFigmaId unchanged (function name: decodeSyntheticFigmaId,
constant: INSTANCE_CLONE_RE) and run related tests to verify nested
instance-clone IDs like "outer::0::inner::1::5:3" decode to "5:3".
Comment on lines
+122
to
+145
| describe.skipIf(!HAS_FIXTURE)("io.archive.pack sub-stages", () => { | ||
| const docForFb = { | ||
| ...preConverted!.document, | ||
| images: {}, | ||
| bitmaps: {}, | ||
| }; | ||
|
|
||
| bench( | ||
| "sub: toFlatbuffer", | ||
| () => { | ||
| format.document.encode.toFlatbuffer(docForFb); | ||
| }, | ||
| { iterations: 3, warmupIterations: 1 } | ||
| ); | ||
|
|
||
| const { | ||
| images: _images, | ||
| bitmaps: _bitmaps, | ||
| ...persistedDocument | ||
| } = preConverted!.document; | ||
| const snapshotPayload = { | ||
| version: "1.0", | ||
| document: persistedDocument, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Vitest describe.skipIf execute the callback function when condition is true?
💡 Result:
No, Vitest describe.skipIf does not skip execution of the callback function when the condition is true. The callback is always executed to collect the inner tests/suites, but those inner tests are marked as skipped and not run.
Citations:
- 1: https://main.vitest.dev/api/describe
- 2: https://v1.vitest.dev/api/
- 3: Skipping all describe section with benchmarks still failing in non benchmarks mode vitest-dev/vitest#6899
- 4: describe.skip does not skip the test vitest-dev/vitest#2371
The non-null assertion accessing preConverted!.document at describe scope will fail when the fixture is missing.
The describe callback always executes regardless of describe.skipIf condition—only the inner tests are marked as skipped. When HAS_FIXTURE is false, preConverted will be null, and accessing preConverted!.document (lines 123-127, 137-145, 155-164) will throw before any tests run.
Move the setup code into bench callbacks, or add a guard check that actually prevents execution:
describe.skipIf(!HAS_FIXTURE)("io.archive.pack sub-stages", () => {
+ if (!preConverted) return;
+
const docForFb = {
- ...preConverted!.document,
+ ...preConverted.document,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io-figma/__bench__/fig2grida.bench.ts` around lines 122
- 145, The file uses preConverted!.document at describe scope which will throw
when the fixture is missing; relocate or guard that logic: either move creation
of docForFb, persistedDocument/snapshotPayload and the call to
format.document.encode.toFlatbuffer inside the bench callbacks (so they only run
when the bench executes), or wrap the describe body in a runtime check of
HAS_FIXTURE and return early (or skip) to avoid evaluating preConverted;
specifically adjust the code that builds docForFb, the destructuring that
produces persistedDocument, and the snapshotPayload so they only access
preConverted when HAS_FIXTURE is true (or move into the bench callback where
format.document.encode.toFlatbuffer is invoked).
# Conflicts: # docs/wg/feat-2d/optimization.md
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
Canary batch: 20 commits spanning canvas rendering, Figma import pipeline, and editor UX.
Canvas / Rendering (Rust)
skip_layoutoptimization — Bypass Taffy layout for absolute-positioned documents imported from Figma.compute_schema_only()copies schema positions in a single walk, cuttingload_scenetime dramatically for 100K+ node documents. Exposed viaruntime_renderer_set_skip_layout(true)in WASM.stroke_dash_arraysupport — Add dashed-stroke property toContainerNode,LineNode,PathNode,VectorNode, andBooleanPathOperationNodewith roundtrip serialization tests.ImageRepository, simplifying image shader creation and LOD selection.getNodeAbsoluteBoundingBoxwith aBoundsTargetenum so callers can query the active scene's bounds, not just individual nodes.prefer_fixed_text_sizing— New config option for fixed-size text frames matching Figma's rendered output, avoiding Taffy text measurement overhead.Benchmarking & Diagnostics
load-benchCLI (grida-dev) — Per-stage timing forload_scene()(layout, geometry, effects, layers). Supports--skip-text,--skip-layout,--list-scenes.bench_load_scene— Synthetic benchmarks for layout at scale (100–150K nodes).--resizeflag for GPU bench — Measuresresize()+redraw()cost per cycle.Figma I/O (
@grida/io-figma)PackOptionswith configurable compression level (0–9); fig2grida packs merged documents at level 0 for speed.prefer_fixed_text_sizingoption — Flows throughFig2GridaOptionsandRestJsonToGridaOptionsinto the CLI.Editor
<scene>selector — Replace"*"camera fit target with"<scene>"for consistent zoom-to-fit behavior across editor, playground, renderer, and hotkeys.RefigRenderConfigappliesskip_layout+prefer_fixed_text_sizingduring embed canvas initialization.Dependency Updates
taffy0.9.1 → 0.9.2,grid0.18.0 → 1.0.0Housekeeping
.claude/skillssymlink →.agents/skills.gitignoreentries for Claude Code local settingscg-perfskill docs with scene loading & layout sectionsresearchskill known citationsSummary by CodeRabbit
New Features
Improvements
Tests & Docs