perf: per-node WASM sync + surgical invalidation, properties catalog#665
Merged
softmarshmallow merged 19 commits intomainfrom Apr 19, 2026
Merged
perf: per-node WASM sync + surgical invalidation, properties catalog#665softmarshmallow merged 19 commits intomainfrom
softmarshmallow merged 19 commits intomainfrom
Conversation
Introduce a runtime-agnostic catalog of every per-node property in the Grida Canvas data model, plus a contributor-facing reference doc that defines the id-stability contract, type vocabulary, FBS mapping, and known asymmetries with grida.fbs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplaces legacy JSON scene I/O with FlatBuffers-only paths, adds per-node WASM APIs (replace/delete) and scene switching, implements a ChangeKind-based invalidation pipeline with SceneDirty-driven partial subtree rebuilds, introduces an Effect-based editor→renderer sync for per-node vs full routing, and adds bench/test tooling and docs for mutation/per-node perf. Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as Editor
participant Reducer as Reducer
participant Sync as Sync
participant WASM as WASM Renderer
participant Scene as Scene/Renderer
Editor->>Reducer: dispatch(action)
Reducer->>Reducer: reduce (may bypass Immer)
Reducer->>Sync: compute effect (effectFromPatches / bypass rules)
Sync-->>Reducer: Effect
Reducer-->>Editor: [state, patches, inverse, effect]
Editor->>WASM: __wasm_on_document_change(effect)
alt effect.kind == "structural"
WASM->>WASM: __wasm_sync_document()
WASM->>Scene: Full scene decode + rebuild
else effect.kind == "nodes"
WASM->>WASM: __wasm_apply_per_node_sync(replace/remove)
WASM->>Scene: replace_node / delete_node -> mark_node_change_kind
alt per-node apply fails
WASM->>WASM: fallback __wasm_sync_document()
WASM->>Scene: Full rebuild
end
else effect.kind == "none"
WASM->>Scene: no renderer update
end
Scene->>Scene: apply_changes(taken SceneDirty) -> partial/patch rebuilds
Scene-->>Scene: patched geometry/layers or full rebuild
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Replaces the reducer-only Tinybench micro-bench with a single-file parametrized suite that drives `Editor.mountHeadless()` against the real raster WASM backend — so `dispatch.wasm.*` spans attribute cost to the same path the browser takes. Extracts `__wasm_sync_document` / `__wasm_on_document_change` / `__wasm_on_scene_id_change` out of `mount()` to make the headless entry point possible. Confirms the dominant cost on 10K-node scenes is JS-side FlatBuffer encode (~100ms), not Immer or Rust decode. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Updated property names in properties.toml to enhance clarity and consistency. Renamed transform properties to include 'post_layout_' prefix and position properties to include 'layout_' prefix, improving the semantic understanding of their roles in the data model.
Updated the editor components to utilize useEditorState instead of the deprecated useDocumentState. This change enhances performance by narrowing subscriptions and improving reactivity in the editor's state management. The useDocumentState function has been marked as deprecated for future removal.
…ng guide Reorient the skill around picking the right signal: browser trace when the user provides one (captures React), editor-bench for proactive JS investigation, Node CPU profile for function-level drilling. Genericize span references so the guide ages well, and fix stale bench file paths. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace full-document sync with targeted replace_node_grida / delete_node C exports so editor dispatch applies changes in ~100µs instead of ~50-200ms at 10K+ nodes. Migrates raster-export tests off legacy .grida1 JSON fixtures to .grida binary fixtures from fixtures/test-grida/ and adds Canvas.switchScene / loadedSceneIds passthroughs so multi-scene documents can be activated through the wrapper.
- turbo.json: test task depends on ^build instead of ^test, so --filter negation actually prunes the task graph (editor#test no longer drags in @grida/canvas-wasm#test) - build-canvas.yml: run canvas-wasm vitest suite after just build, the one job with a freshly-built wasm binary
… guard editor warmup against disposal - build-canvas.yml: install without --filter and build @grida/canvas-io before running canvas-wasm vitest. The suite imports from packages/grida-canvas-io (source path), which transitively depends on @grida/schema, @grida/cg, @grida/cmath, @grida/format, @grida/sequence. canvas-wasm's own package.json has no deps, so the previous filtered install left those missing. - editor.ts: track _disposed and short-circuit _do_legacy_warmup's post-fetch continuation and loadPlatformDefaultFonts. Without this, the orphan google-fonts → setFallbackFonts chain can reach a disposed WASM Scene and throw "Scene is disposed" after tests finish.
![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: 60a394de5c
ℹ️ 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
+127
to
+131
| // event-target/event/on-drag — collect mutated node ids from gesture | ||
| // and content-edit-mode state. Mirrors the bypass clone logic below. | ||
| const replace = new Set<string>(); | ||
| const gesture = state.gesture; | ||
| if (gesture.type !== "idle" && "selection" in gesture && gesture.selection) { |
There was a problem hiding this comment.
effectForBypassAction treats every event-target/event/on-drag update as node-only, but drag gestures can mutate hierarchy (graphInstance.mv(...) when translate_with_hierarchy_change is on, and clone insert/remove paths). In silent bypass mode this routes to replaceNode instead of a full sync, and replaceNode cannot apply document.links changes, so the WASM scene graph can diverge from editor state during drag interactions until a later structural resync.
Useful? React with 👍 / 👎.
Comment on lines
+142
to
+143
| if (gesture.type === "guide" && state.scene_id) { | ||
| replace.add(state.scene_id); |
There was a problem hiding this comment.
Guide drags add the active scene_id to the per-node replace set here, but the new Rust single-node decoder rejects SceneNode buffers (UnsupportedNodeType), so replaceNode fails every frame and falls back to full document re-encode with warning logs. That makes guide movement consistently hit the expensive path; guide updates should be routed as structural (or SceneNode support should be added to per-node decode).
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: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
crates/grida-canvas/src/node/scene_graph.rs (1)
839-843:⚠️ Potential issue | 🟠 MajorClear every parallel graph map during subtree deletion.
remove_subtreedeletes nodes throughremove_node, butremove_nodeonly clearsgeo_dataandnodes. Deleted subtree ids remain inlayer_core,links, andnames, so later compact-map lookups can observe stale deleted nodes.Proposed fix
pub fn remove_node(&mut self, id: &NodeId) -> SceneGraphResult<Node> { self.geo_data.remove(id); + self.layer_core.remove(id); + self.links.remove(id); + self.names.remove(id); self.nodes .remove(id) .ok_or(SceneGraphError::NodeNotFound(*id)) } @@ for removed in &to_remove { - let _ = self.remove_node(removed); + self.remove_node(removed)?; }Also applies to: 900-902
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/node/scene_graph.rs` around lines 839 - 843, remove_node currently only deletes from geo_data and nodes, leaving dangling entries in the parallel maps used by subtree operations; update remove_node (and the same code path referenced around the other occurrence at the later block) to also remove the id from layer_core, links, and names (and any other parallel maps that mirror nodes) so deleted node ids are cleared from every auxiliary structure; locate the remove_node function and the similar deletion block around the later lines and add calls that remove the id from layer_core.remove(id), links.remove(id), names.remove(id) (and any other parallel collections such as parents/children maps) to ensure no stale lookups remain.docs/wg/research/chromium/index.md (1)
1-9:⚠️ Potential issue | 🟡 MinorAdd the required docs frontmatter while editing this page.
This
docs/wgMarkdown page is being updated but still lacksdescription,keywords, andformat: md.Proposed frontmatter update
--- title: "Chromium Compositor Research" +description: "Research notes on Chromium compositor, rendering, invalidation, tiling, and related architecture." +keywords: + - chromium + - compositor + - rendering + - invalidation + - dirty flags +format: md tags: - internal - research - chromiumAs per coding guidelines,
docs/{wg,reference}/**/*.mdpages should include SEO frontmatter withtitle,description, andkeywords, and non-MDX pages should addformat: md.Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/research/chromium/index.md` around lines 1 - 9, The page's YAML frontmatter is missing required SEO fields and the format flag; update the top YAML frontmatter (the --- block) to include description: a concise summary of "Chromium Compositor Research", a keywords: array (e.g., ["chromium","compositor","rendering","compositing"]), and add format: md for a non-MDX Markdown page, keeping the existing title and tags intact so the block contains title, description, keywords, tags, and format: md..agents/skills/io-grida/SKILL.md (1)
16-33:⚠️ Potential issue | 🟡 MinorRemove the stale JSON mention from the Rust decoder role.
After documenting only raw FlatBuffers and ZIP archives, Line 33 still says
FBS/ZIP/JSON → Scene, which keeps the removed legacy path in the skill docs.📝 Proposed docs fix
-| `crates/grida-canvas/src/io/` | Rust decoder (FBS/ZIP/JSON → `Scene`) | +| `crates/grida-canvas/src/io/` | Rust decoder (FBS/ZIP → `Scene`) |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/io-grida/SKILL.md around lines 16 - 33, Update the Key Locations table entry for crates/grida-canvas/src/io/ to remove the stale "JSON" mention: change the description from "Rust decoder (FBS/ZIP/JSON → `Scene`)" to "Rust decoder (FBS/ZIP → `Scene`)" so it reflects only Raw FlatBuffers and ZIP archive inputs; ensure the exact string "FBS/ZIP/JSON → `Scene`" is replaced and commit the edited SKILL.md.editor/grida-canvas-react/viewport/surface-hooks.ts (1)
343-451:⚠️ Potential issue | 🟠 MajorInclude
document_ctxin the effect dependencies.The effect reads
dq.getChildren(document_ctx, node_id)for flex distribution. With the narrowed subscription, a links/context-only update can rerender the hook without rerunning this effect, leaving stale distribution data.Suggested fix
- }, [node, node_id, transform, enabled, parentNode]); + }, [node, node_id, transform, enabled, parentNode, document_ctx]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas-react/viewport/surface-hooks.ts` around lines 343 - 451, The effect builds distribution using dq.getChildren(document_ctx, node_id) but document_ctx is not included in the dependency array, which can leave distribution stale; update the useEffect dependencies (the array at the end of the effect for the effect that sets data in this file) to include document_ctx so the effect re-runs when document_ctx changes (i.e., change [..., transform, enabled, parentNode] to include document_ctx).crates/grida-canvas/src/io/io_grida_fbs.rs (2)
905-906:⚠️ Potential issue | 🟡 MinorAddress the typo-check failure for
alignement.CI is failing on this spelling. If the misspelling is schema-locked/generated, add it to the typo allowlist; otherwise rename it consistently through the schema/generated bindings and call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/io/io_grida_fbs.rs` around lines 905 - 906, The identifier "alignement" is spelled incorrectly; locate the symbol alignement() and its local variable alignement and either (A) if this comes from generated/schema bindings (e.g., flatbuffers schema), add "alignement" to the project's typo allowlist so CI accepts the generated name, or (B) if the name is under our control, rename the schema+generated bindings and all call sites from alignement to alignment consistently (update the schema field, regenerate bindings, and replace usages like ip.alignement() and the local variable) so the spelling is corrected project-wide.
74-83:⚠️ Potential issue | 🟠 MajorEnforce single-node buffer contract and use checked deserialization at API boundary.
The
decode_single_nodefunction silently accepts multi-node buffers and decodes only the first one, contradicting the API contract. Additionally,root_uncheckedis unnecessary here; whiledecode_all_innerjustifies unchecked decoding to avoid stack overflow on large documents, per-node buffers are small and belong in WASM/public API code where checked deserialization is preferable.Add an
InvalidSingleNodeCounterror variant to enforce exactly one node, replaceroot_uncheckedwithroot, and validatenodes_vec.len() == 1before decoding.Note: The typo
alignementat line 905 appears in the FlatBuffers schema-generated accessor and should be corrected at the schema definition level.Suggested fix
pub enum FbsDecodeError { InvalidBuffer(flatbuffers::InvalidFlatbuffer), MissingDocument, MissingScene, /// Single-node buffer has a missing or empty `nodes` vector. EmptyNodes, + /// Single-node buffer must carry exactly one node slot. + InvalidSingleNodeCount(usize), /// Single-node buffer carried a `Node` variant that is not a valid /// partial-replacement target (e.g. `SceneNode`, or unrecognized). UnsupportedNodeType, }FbsDecodeError::EmptyNodes => write!(f, "per-node buffer has no nodes"), + FbsDecodeError::InvalidSingleNodeCount(n) => { + write!(f, "per-node buffer must contain exactly one node, got {n}") + } FbsDecodeError::UnsupportedNodeType => { write!(f, "per-node buffer carries an unsupported node type") }pub fn decode_single_node(bytes: &[u8]) -> Result<DecodedSingleNode, FbsDecodeError> { - // SAFETY: Skip verification (matches decode_all_inner). - let grida_file = unsafe { flatbuffers::root_unchecked::<fbs::GridaFile>(bytes) }; + let grida_file = flatbuffers::root::<fbs::GridaFile>(bytes)?; let document = grida_file .document() .ok_or(FbsDecodeError::MissingDocument)?; let nodes_vec = document.nodes().ok_or(FbsDecodeError::EmptyNodes)?; if nodes_vec.is_empty() { return Err(FbsDecodeError::EmptyNodes); } + if nodes_vec.len() != 1 { + return Err(FbsDecodeError::InvalidSingleNodeCount(nodes_vec.len())); + } let slot = nodes_vec.get(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/io/io_grida_fbs.rs` around lines 74 - 83, Add a new FbsDecodeError::InvalidSingleNodeCount variant and update decode_single_node to enforce exactly one node: after getting nodes_vec from root (use root instead of root_unchecked) check nodes_vec.len() == 1 and return InvalidSingleNodeCount if not; keep the existing EmptyNodes variant for the specific empty-vector case if desired or remove if redundant. Replace any use of root_unchecked within decode_single_node with root to perform checked deserialization at the public API boundary, and ensure subsequent per-node decoding still uses the same safe accessors (e.g., nodes_vec.get(0) after the length check).crates/grida-dev/src/editor/document.rs (1)
119-137:⚠️ Potential issue | 🟡 MinorStale comment: resize no longer needs the "full flush" rationale.
The comment at lines 133-136 claims resize "still goes through full
flushbecause the legacyhandle_incremental_resizepath mutates only the editor's scene." That stopped being true in this PR —handle_incremental_resizenow routes every mutation throughapply_and_mark(see lines 206, 227, 262), which mirrors into the renderer scene and marksChangeKinds just like the translate path. There's noflush()call on this return path either;mutatedis just returned to the caller.✏️ Proposed comment update
- // Resize still goes through full `flush` because the legacy - // `handle_incremental_resize` path mutates only the editor's - // scene. The Transform path above is self-contained and doesn't - // need a separate flush. + // Both the translate and resize paths are self-contained: + // `apply_and_mark` mirrors the mutation into the renderer scene + // and marks per-node `ChangeKind`s, so `apply_changes` picks the + // right invalidation without a separate `flush`. mutated🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/editor/document.rs` around lines 119 - 137, The existing comment claiming resize "still goes through full `flush`" is stale; update the comment around the SurfaceGesture match to reflect that `handle_incremental_resize` now routes mutations through `apply_and_mark` (see `handle_incremental_resize` and `apply_and_mark`) and mirrors changes into the renderer scene, so there is no special full `flush()` path — the function simply returns `mutated`. Replace the old rationale with a short note that both translate and resize use the same apply-and-mark mutation flow and that `mutated` is returned to the caller.editor/grida-canvas/reducers/index.ts (1)
369-378:⚠️ Potential issue | 🟠 MajorGuard the scene-node clone to guide gestures only.
Line 370 currently clones and rewrites the scene node for every bypass action with a
scene_id, including pointer moves andnode/change/*. Becauseguidesis normalized to[], this can mutate the scene node whileeffectForBypassAction()returnsEFFECT_NONEor only the changed node id, leaving WASM sync unaware of the scene-node change.🐛 Proposed fix
- if (state.scene_id && mutableNodes[state.scene_id]) { + if ( + gesture.type === "guide" && + state.scene_id && + mutableNodes[state.scene_id] + ) { const snode = mutableNodes[ state.scene_id ] as grida.program.nodes.SceneNode;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/reducers/index.ts` around lines 369 - 378, The scene-node clone is being done for every bypass action with state.scene_id, which can introduce unintended scene mutations; only perform the mutableNodes[state.scene_id] clone (and the guides mapping) when the bypass action is a guide gesture. Update the branch that currently checks state.scene_id to further guard by verifying the action is a guide gesture—either by checking the action.type (e.g., gesture/guide*) or by calling effectForBypassAction(action) and ensuring it does not return EFFECT_NONE and indicates a guide-related effect—before cloning the SceneNode and rewriting guides; keep references to mutableNodes, state.scene_id, guides, effectForBypassAction, and EFFECT_NONE so the change is easy to locate.editor/grida-canvas/editor.ts (1)
3730-3744:⚠️ Potential issue | 🟠 MajorInstall the transform subscriber before
camera.fit().Line 3737 dispatches a transform update, but the WASM transform subscriber is only registered on Lines 3739-3744, so the initial fit updates editor state without updating the WASM camera. Register the subscriber before calling
camera.fit("<scene>"), or explicitly callsyncTransformagain after the fit.🐛 Proposed fix
- // --- viewport-specific initial sync --- - syncTransform( - this._m_wasm_canvas_scene!, - this.doc.state.transform, - el.width, - el.height - ); - this.camera.fit("<scene>"); - this.doc.subscribeWithSelector( (state) => state.transform, (_, v) => { syncTransform(this._m_wasm_canvas_scene!, v, el.width, el.height); } ); + + // --- viewport-specific initial sync --- + syncTransform( + this._m_wasm_canvas_scene!, + this.doc.state.transform, + el.width, + el.height + ); + this.camera.fit("<scene>");🤖 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 3730 - 3744, The transform subscriber is registered after camera.fit("<scene>") so the initial fit dispatch isn't propagated to the WASM camera; move the subscription block (the call to this.doc.subscribeWithSelector that invokes syncTransform on state.transform) to run before calling this.camera.fit("<scene>") so the WASM scene (this._m_wasm_canvas_scene) receives the update, or alternatively add an explicit syncTransform(this._m_wasm_canvas_scene!, this.doc.state.transform, el.width, el.height) immediately after camera.fit("<scene>"); adjust either the code that references syncTransform, this._m_wasm_canvas_scene, this.doc.subscribeWithSelector, and camera.fit("<scene>") accordingly.crates/grida-canvas/src/window/application.rs (2)
830-834:⚠️ Potential issue | 🟠 MajorKeep
loaded_scenesin sync with per-node mutations.
switch_scenereloads by cloning fromloaded_scenes, butreplace_node_grida/delete_nodemutate onlyself.renderer.scene. Switching away and back can resurrect replaced/deleted nodes, whileid_mappinghas already been updated for deletes.Consider tracking the active scene id and applying the same mutation to that stored scene, or making
renderer.scenethe single source of truth for the active entry. Based on learnings, Maintain bidirectional mapping between NodeId and UserNodeId at the application layer for API boundary.Also applies to: 854-928
🤖 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 830 - 834, switch_scene currently clones scenes from loaded_scenes which can reintroduce nodes after replace_node_grida/delete_node mutate only renderer.scene; make loaded_scenes and renderer.scene consistent by either: 1) track an active_scene_id and, whenever replace_node_grida or delete_node run, apply the same mutation to the corresponding entry inside loaded_scenes (update the tuple in loaded_scenes where id == active_scene_id), or 2) switch to using renderer.scene as the single source of truth for the active scene and remove cloning from loaded_scenes in switch_scene; additionally maintain a bidirectional mapping between NodeId and UserNodeId at the application layer (update id_mapping whenever nodes are deleted/replaced) so id_mapping, loaded_scenes (or renderer.scene) and replace/delete logic remain in sync.
1580-1605:⚠️ Potential issue | 🟠 MajorQueue a frame after marking resource/font invalidations.
mark_global(...)records dirty state, butapply_changesonly runs after the frame loop is invalidated. Async image/font loads and fallback-font changes can remain unapplied until an unrelated redraw occurs.Proposed fix
if updated { self.renderer.mark_global(GlobalFlag::ImageLoaded); + self.queue(); } @@ if updated { self.renderer.mark_global(GlobalFlag::FontLoaded); if font_count > 0 { self.print_font_repository_info(); } + self.queue(); } @@ pub fn set_default_fallback_fonts(&mut self, fonts: Vec<String>) { self.renderer.fonts.set_user_fallback_families(fonts); self.renderer.mark_global(GlobalFlag::FontLoaded); + self.queue(); }Also applies to: 1676-1696
🤖 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 1580 - 1605, process_font_queue (and the analogous image queue handler) mark global dirty state via self.renderer.mark_global(...) but never request a new frame, so apply_changes won't run until an unrelated redraw; after setting updated = true and calling self.renderer.mark_global(GlobalFlag::FontLoaded) (and similarly for ImageLoaded in the image handler), immediately queue/invalidate a frame by calling the frame invalidation method used by this application (e.g., self.invalidate_frame(), self.queue_frame(), or the existing API that triggers the frame loop) so apply_changes runs and the font/image changes are applied promptly.crates/grida-canvas/src/runtime/scene.rs (1)
2095-2202:⚠️ Potential issue | 🟠 MajorInvalidate picture/compositor caches after full rebuilds.
A full
rebuild_scene_caches()can run forGlobalFlag::Layout, viewport-dependent layout, effect-tree, or layer-list dirt, but the later cache invalidation only clears all pictures/compositor entries for scene/font/image globals. For deletes (GlobalFlag::Layout) or layout-driven rebuilds with emptypaint_touched, stale pictures/atlas entries can be reused after geometry/layers changed.Proposed fix
+ let mut ran_full_rebuild = false; + if has_data_changes && !global.scene_load { @@ if needs_layout || needs_effect_tree || needs_layer_list { @@ self.rebuild_scene_caches(); + ran_full_rebuild = true; } else if needs_geometry { @@ - if global.scene_load || global.font_loaded || global.image_loaded { + if global.scene_load || global.font_loaded || global.image_loaded || ran_full_rebuild { self.scene_cache.picture.invalidate(); } else if has_data_changes { @@ - if global.scene_load { + if global.scene_load { self.scene_cache.compositor.clear(); self.compositor_atlas.clear(); - } else if global.font_loaded || global.image_loaded { + } else if global.font_loaded || global.image_loaded || ran_full_rebuild { self.scene_cache.compositor.invalidate_all(); self.compositor_atlas.clear();🤖 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 2095 - 2202, The full-rebuild branch that calls self.rebuild_scene_caches() doesn't currently invalidate per-node pictures/compositor or clear the atlas, which can leave stale entries after layout/structure-driven full rebuilds; after the call to self.rebuild_scene_caches() in the branch guarded by needs_layout || needs_effect_tree || needs_layer_list, add calls to self.scene_cache.picture.invalidate(), self.scene_cache.compositor.invalidate_all(), and self.compositor_atlas.clear() so pictures, compositor entries and the atlas are reset immediately after a full cache rebuild (referencing rebuild_scene_caches, scene_cache.picture.invalidate, scene_cache.compositor.invalidate_all, and compositor_atlas.clear).
🟡 Minor comments (11)
format/properties.md-133-137 (1)
133-137:⚠️ Potential issue | 🟡 MinorAdd language specifier to fenced code block.
The code block listing node variants should specify
textor similar as the language for proper syntax highlighting.📝 Proposed fix
-``` +```text scene, group, tray, initial_container, container, boolean_operation, basic_shape, line, vector, path, text_span, attributed_text, markdown_embed, unknown🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@format/properties.md` around lines 133 - 137, The fenced code block that lists the node variants (the block containing "scene, group, tray, initial_container, container, boolean_operation, basic_shape, line, vector, path, text_span, attributed_text, markdown_embed, unknown") needs a language specifier for proper highlighting; modify the opening fence from ``` to ```text (or another appropriate language) so the block is annotated (e.g., change the code fence that wraps the node variants to include "text").crates/grida-dev/src/bench/args.rs-44-47 (1)
44-47:⚠️ Potential issue | 🟡 MinorConfigure the
--translateflag to allow omitting the value.Line 45 documents that callers can "leave empty", but
Option<String>with only#[arg(long = "translate")]requires a value whenever the flag is present. Update the attribute to support optional values with"first"as the default:Proposed fix
- #[arg(long = "translate")] + #[arg(long = "translate", num_args = 0..=1, default_missing_value = "first")] pub translate: Option<String>,This allows users to write
--translatewith no value (defaulting to "first"),--translate 123for a numeric ID, or omit the flag entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/bench/args.rs` around lines 44 - 47, The translate flag currently requires a value even when callers expect to be able to pass the flag with no value; update the attribute on the translate field so the flag accepts an optional value and defaults to "first" when present with no value. Specifically, modify the #[arg(...)] on the pub translate: Option<String> field (symbol: translate) to include default_missing = "first" (and optionally value_name like "NODE") so users can use --translate (defaults to "first"), --translate 123, or omit the flag entirely.crates/grida-dev/src/editor/mutation.rs-64-68 (1)
64-68:⚠️ Potential issue | 🟡 MinorAvoid
Fullinvalidation for idempotent resize writes.Line 64 treats
resize_node’s boolean as “actual data changed”, butresize_nodesetschanged = truewhenever an axis is provided, even if the stored value is already the same. Repeated resize events can now trigger unnecessary full rebuilds.🐛 Proposed fix pattern
if let Some(w) = width { - n.layout_dimensions.layout_target_width = Some(w); - changed = true; + if n.layout_dimensions.layout_target_width != Some(w) { + n.layout_dimensions.layout_target_width = Some(w); + changed = true; + } } if let Some(h) = height { - n.layout_dimensions.layout_target_height = Some(h); - changed = true; + if n.layout_dimensions.layout_target_height != Some(h) { + n.layout_dimensions.layout_target_height = Some(h); + changed = true; + } }Apply the same compare-before-write pattern to
TextSpan,AttributedText,MarkdownEmbed, andSizefields.Also applies to: 178-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/editor/mutation.rs` around lines 64 - 68, The code treats resize_node’s boolean return as “data changed” and always sets changed = true if an axis was provided, causing unnecessary ChangeKind::Full invalidations; update resize_node to perform a compare-before-write on the Size fields (width/height) so it only sets changed = true when the stored Size actually differs from the requested values, then keep the caller logic that emits (id, ChangeKind::Full) only when true; apply the same compare-before-write pattern to the other mutation writers mentioned (TextSpan, AttributedText, MarkdownEmbed and any Size field updates in the 178-250 region) so writes are idempotent and only mark ChangeKind::Full when real data changes occur.crates/grida-dev/src/editor/mutation.rs-49-53 (1)
49-53:⚠️ Potential issue | 🟡 MinorOnly report translations that actually mutated a node.
translate_nodesilently no-ops for unknown ids and nodes without a mutable position/transform, but Line 52 still emitsChangeKind::Geometry. That violates the empty-return contract and can forward invalid ids to renderer invalidation.🐛 Proposed fix
- for id in ids { - translate_node(&mut scene.graph, id, *dx, *dy); - out.push((*id, ChangeKind::Geometry)); - } + for id in ids { + if translate_node(&mut scene.graph, id, *dx, *dy) { + out.push((*id, ChangeKind::Geometry)); + } + }Update the helper to report whether it changed anything:
-fn translate_node(graph: &mut SceneGraph, id: &NodeId, dx: f32, dy: f32) { +fn translate_node(graph: &mut SceneGraph, id: &NodeId, dx: f32, dy: f32) -> bool { if let Ok(node) = graph.get_node_mut(id) { + let mut changed = true; match node { Node::Container(n) => { let x = n.position.x().unwrap_or(0.0); let y = n.position.y().unwrap_or(0.0); n.position = LayoutPositioningBasis::Cartesian(CGPoint { @@ _ => { if let Some(t) = node_transform_mut(node) { t.translate(dx, dy); + } else { + changed = false; } } } - graph.refresh_node_geo_data(id); + if changed { + graph.refresh_node_geo_data(id); + } + changed + } else { + false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/editor/mutation.rs` around lines 49 - 53, The loop currently always pushes ChangeKind::Geometry for every id even when translate_node did nothing; change translate_node to return a boolean (or otherwise expose whether it actually mutated the node) and modify this loop to call translate_node(&mut scene.graph, id, *dx, *dy) and only push (*id, ChangeKind::Geometry) when that call returns true, so unknown ids or immutable nodes are not reported to the renderer.editor/grida-canvas-react/viewport/surface.tsx-726-738 (1)
726-738:⚠️ Potential issue | 🟡 MinorKeep isolated tray child labels visible.
Line 728 returns after adding only the isolated node, but the component contract above says isolated tray mode should still show direct container children. That makes title bars disappear for root-like containers inside an isolated tray; if this is intentional, update the stale contract, otherwise include the tray-child pass and ensure the single-mode branch does not drop the extra labels.
Possible selector-side fix
if (isolation_root_node_id) { const node = nodes[isolation_root_node_id]; if (node && node.active !== false) { if (node.type === "tray") { labels.push({ node, variant: "badge" }); + const trayChildren = links[isolation_root_node_id]; + if (trayChildren) { + for (const childId of trayChildren) { + const child = nodes[childId]; + if (child && child.active !== false && isContainer(child)) { + labels.push({ + node: child, + variant: "plain", + parentNodeId: isolation_root_node_id, + }); + } + } + } } else if (isContainer(node)) { labels.push({ node, variant: "plain" }); } } return { labeledNodes: labels, isSingleMode };Also applies to: 801-814
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas-react/viewport/surface.tsx` around lines 726 - 738, The early return in the isolation branch only adds the isolated node to labels and drops its direct container children for tray roots; update the isolation handling in surface.tsx (the block using isolation_root_node_id, nodes, labels, isContainer, and isSingleMode) to, when the isolated node exists and node.type === "tray", also iterate its direct children and push any direct container children as labels (variant "plain" or appropriate) in addition to the tray badge; ensure the later single-mode branch (where isSingleMode is used) does not overwrite or drop those extra labels so isolated-tray child title bars remain visible.crates/grida-dev/src/bench/runner.rs-245-259 (1)
245-259:⚠️ Potential issue | 🟡 MinorDon’t let unsupported translate targets produce empty “successful” results.
If
mutation::applyreturns no reports for every frame, the pass exits with zero timings andrun_benchstill prints success. Return an error or at least report skipped/no-op frames so invalid numeric targets do not create misleading perf data.🤖 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 245 - 259, The code treats an empty reports vector from mutation::apply (called on renderer.scene.as_mut() with MutationCommand::Translate for target_id) as a no-op and continues, which lets unsupported/invalid translate targets produce zero timings and a misleading success; update the logic in the loop that sets let reports = ... and checks if reports.is_empty() so that an empty reports result either returns an Err (with context including target_id and frame index) from run_bench or increments a skipped/no-op frame counter that is propagated and causes run_bench to fail (or report skipped frames) rather than silently continuing; locate the reports variable and the continue branch and replace it with an explicit error return or a recorded skipped-frame report so invalid numeric targets do not generate false-success perf data.crates/grida-canvas-wasm/lib/__test__/environment-node-raster-export.test.ts-45-63 (1)
45-63:⚠️ Potential issue | 🟡 MinorDispose raster canvases on every test path.
renderFixtureNodeToPngreturns live canvases that the first three tests never dispose, and the custom-image test skipsdispose()if an assertion throws. Usetry/finally, and copy exported bytes before disposal if needed.Suggested fix pattern
async function renderFixtureNodeToPng(fixture: string) { const { bytes, sceneId, nodeId } = await loadGridaFixture(fixture); const canvas = await createCanvas({ backend: "raster", width: 256, height: 256, useEmbeddedFonts: true, }); - canvas.loadSceneGrida(bytes); - canvas.switchScene(sceneId); + try { + canvas.loadSceneGrida(bytes); + canvas.switchScene(sceneId); - const { data } = canvas.exportNodeAs(nodeId, { - format: "PNG", - constraints: { type: "none", value: 1 }, - }); + const { data } = canvas.exportNodeAs(nodeId, { + format: "PNG", + constraints: { type: "none", value: 1 }, + }); - return { canvas, data, sceneId, nodeId }; + return { + backend: canvas.backend, + data: new Uint8Array(data), + sceneId, + nodeId, + }; + } finally { + canvas.dispose(); + } }- const { canvas, data } = await renderFixtureNodeToPng("L0.grida"); + const { backend, data } = await renderFixtureNodeToPng("L0.grida"); - expect(canvas.backend).toBe("raster"); + expect(backend).toBe("raster");Also applies to: 158-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas-wasm/lib/__test__/environment-node-raster-export.test.ts` around lines 45 - 63, The renderFixtureNodeToPng helper returns live Canvas instances that aren't always disposed, so wrap canvas usage in a try/finally in renderFixtureNodeToPng (and the other helper at lines ~158-191) to ensure canvas.dispose() is always called; before disposing, copy/clone the exported PNG bytes buffer (from the export result `data` or `{ data }` returned by canvas.exportNodeAs) into a new Uint8Array or Buffer so the test assertions can use the bytes after disposal, then return the cloned bytes along with sceneId/nodeId instead of the live canvas where possible (or nullify the canvas before returning).crates/grida-dev/src/bench/runner.rs-199-208 (1)
199-208:⚠️ Potential issue | 🟡 MinorSearch past
InitialContainerfor the default translate target.
--translate firstreturnsNonewhen the only root is anInitialContainer, even if it has movable descendants. Walk descendants when skipping the container so the default works for container-rooted fixtures.Suggested fix
fn pick_translate_target(scene: &Scene) -> Option<cg::node::schema::NodeId> { use cg::node::schema::Node; - for &root_id in scene.graph.roots() { - match scene.graph.get_node(&root_id) { - Ok(Node::InitialContainer(_)) => continue, - Ok(_) => return Some(root_id), - Err(_) => continue, + let mut stack: Vec<_> = scene.graph.roots().iter().rev().copied().collect(); + while let Some(node_id) = stack.pop() { + match scene.graph.get_node(&node_id) { + Ok(Node::InitialContainer(_)) => { + if let Some(children) = scene.graph.get_children(&node_id) { + stack.extend(children.iter().rev().copied()); + } + } + Ok(_) => return Some(node_id), + Err(_) => {} } } None }🤖 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 199 - 208, pick_translate_target currently skips roots that are InitialContainer and returns None even if they contain movable descendants; update pick_translate_target (and keep using Scene, cg::node::schema::Node) so that when you encounter Ok(Node::InitialContainer(_)) you walk that root's descendants (using the graph traversal/children API on scene.graph and get_node) to find the first non-InitialContainer node and return its NodeId; only continue to the next root if no suitable descendant is found; otherwise return Some(found_id).docs/wg/research/chromium/dirty-flag-management.md-1-9 (1)
1-9:⚠️ Potential issue | 🟡 MinorComplete the required docs frontmatter.
This new
docs/wg/**Markdown page is missingdescription,keywords, andformat: md.📝 Proposed frontmatter fix
--- title: "Chromium Dirty-Flag Management" +description: "Survey of Chromium and Blink dirty-flag families, propagation shapes, and frame-phase invalidation behavior." +keywords: + - chromium + - blink + - invalidation + - dirty flags + - rendering pipeline +format: md tags: - internal - researchAs per coding guidelines: “
docs/{wg,reference}/**/*.md: Include SEO frontmatter withtitle,description, andkeywords… Addformat: mdfrontmatter to Markdown pages indocs/wg/anddocs/reference/that do not use MDX/JSX features.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/wg/research/chromium/dirty-flag-management.md` around lines 1 - 9, The frontmatter for the Markdown page that begins with title: "Chromium Dirty-Flag Management" is incomplete; add the required SEO fields and format by updating the YAML frontmatter to include a descriptive description, a keywords array (e.g., ["chromium","dirty flags","invalidation","research"]), and add format: md so the block contains title, description, keywords, tags, and format: md; ensure the new fields appear in the existing frontmatter section at the top of the file that currently only has title and tags.docs/wg/research/chromium/dirty-flag-management.md-35-54 (1)
35-54:⚠️ Potential issue | 🟡 MinorSpecify the fence language for the ASCII pipeline block.
Use
texthere to satisfy markdownlint and keep the block explicitly non-code.📝 Proposed markdown fix
-``` +```text DOM mutation / animation tick / style-sheet change │ ▼ @@ └─────────────────────────┘</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/wg/research/chromium/dirty-flag-management.mdaround lines 35 - 54, The
ASCII pipeline block beginning with "DOM mutation / animation tick / style-sheet
change" is using a bare triple-backtick fence; update that fence to explicitly
declare the language as text (i.e., changetotext) so markdownlint
treats it as non-code and the diagram renders correctly.</details> </blockquote></details> <details> <summary>editor/grida-canvas/__tests__/bench/README.md-20-31 (1)</summary><blockquote> `20-31`: _⚠️ Potential issue_ | _🟡 Minor_ **Use the repo-root benchmark path consistently.** These commands omit the `editor/` prefix, while the skill doc uses the full repo-root path. Unless this README assumes `cd editor`, copy-pasting from the repo root will target the wrong file. <details> <summary>📝 Proposed docs fix</summary> ```diff -GRIDA_PERF=1 pnpm vitest run grida-canvas/__tests__/bench/perf-editor.test.ts +GRIDA_PERF=1 pnpm vitest run editor/grida-canvas/__tests__/bench/perf-editor.test.ts @@ - grida-canvas/__tests__/bench/perf-editor.test.ts + editor/grida-canvas/__tests__/bench/perf-editor.test.ts @@ - grida-canvas/__tests__/bench/perf-editor.test.ts + editor/grida-canvas/__tests__/bench/perf-editor.test.ts ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/__tests__/bench/README.md` around lines 20 - 31, The README’s benchmark command examples use a relative test path that assumes being inside the editor folder; update each example command in the bench README so they use the repo-root benchmark path consistently by prefixing the test invocation with the editor/ segment (i.e., adjust the three commands that run the perf-editor test, including the variants that set GRIDA_PERF_CPUPROFILE and NODE_OPTIONS) so copy-pasting from the repository root targets the correct test file while preserving the existing env vars and flags. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>editor/grida-canvas-react/provider.tsx (1)</summary><blockquote> `334-336`: **Point deprecated callers at the replacement API.** This public hook is deprecated, but the notice does not tell consumers how to migrate. <details> <summary>Suggested wording</summary> ```diff /** - * `@deprecated` will be removed + * `@deprecated` will be removed. Use `useEditorState(...)` with a narrow selector instead. */ ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas-react/provider.tsx` around lines 334 - 336, Update the deprecation JSDoc for the deprecated public hook in provider.tsx to point callers to the replacement API: replace the terse "@deprecated will be removed" with a clear migration note that names the new export (e.g., "Use useNewHook instead"), includes the path/module of the replacement, and a short example or one-line guidance on how to migrate; ensure you update the comment immediately above the hook's declaration (the deprecated export in provider.tsx) so IDEs and docs show the replacement. ``` </details> </blockquote></details> <details> <summary>crates/grida-canvas/src/cache/geometry.rs (1)</summary><blockquote> `521-619`: **Consider a `debug_assert!` for the "no nested dirty roots" precondition.** The doc at lines 511-516 is clear that no ancestor of any root may itself be dirty. If callers accidentally pass both an ancestor and its descendant in `roots`, the outcome depends on HashSet iteration order: if the descendant is visited first, its walk uses the stale parent_world from the cache, and correctness relies on the ancestor's later walk overwriting those entries. This is technically fine today but will silently produce wrong transforms the moment someone adds an early-exit or a write-filter to `build_recursive`. A cheap guard in debug builds: <details> <summary>🛡️ Proposed precondition check</summary> ```diff if affected.is_empty() { return affected; } + + // No root may be a descendant of another root in `roots` — see + // preconditions. This is cheap because `affected` is already + // materialized. + debug_assert!( + roots.iter().all(|&r| { + self.entries + .get(&r) + .and_then(|e| e.parent) + .map(|p| !affected.contains(&p) || roots.contains(&r) && !roots.iter().any(|&other| other != r && { + let mut cur = Some(p); + while let Some(c) = cur { + if c == other { return true; } + cur = self.entries.get(&c).and_then(|e| e.parent); + } + false + })) + .unwrap_or(true) + }), + "update_subtree: roots must not contain nested ancestor/descendant pairs" + ); ``` A simpler version just checks that no root's ancestor chain contains another root — happy to simplify further. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/cache/geometry.rs` around lines 521 - 619, Add a debug-only precondition in update_subtree to ensure no supplied root is an ancestor of another: for each &root in roots call graph.descendants(&root) and assert that none of those descendants intersect the roots set (use debug_assert!); place this check at the start of update_subtree (before enumerating affected) and keep it only in debug builds so there is no release overhead. ``` </details> </blockquote></details> <details> <summary>crates/grida-dev/src/editor/document.rs (1)</summary><blockquote> `64-85`: **Document determinism contract for `mutation::apply` and add divergence safeguard in `apply_and_mark`.** The mirrored apply fast path is sound, but two aspects need to be explicit: 1. **Determinism contract**: `mutation::apply` is deterministic and side-effect free (pure graph mutation only), but this should be documented in `mutation.rs` comments. Without this contract, future maintainers might add RNG, counters, or event emission and silently break the double-apply assumption in `apply_and_mark`. 2. **Scene divergence check**: If editor and renderer scenes have diverged (e.g., direct `load_scene` call on renderer), the `reports` from the editor apply may reference `NodeId`s that don't align with the renderer scene. Add `debug_assert_eq!(reports.len(), renderer_reports.len())` in `apply_and_mark` after the second apply to catch divergence in tests. *Note: The comment at lines 133–136 about resize mutating only the editor's scene is now stale; `handle_incremental_resize` uses `apply_and_mark` to mirror mutations.* <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/editor/document.rs` around lines 64 - 85, Document in mutation.rs that mutation::apply is intended to be deterministic and side‑effect free (pure graph mutation only) so callers can safely reapply mutations to another scene; then in Document::apply_and_mark, capture the result of the second apply (e.g. let renderer_reports = mutation::apply(renderer_scene, cmd);) and add a debug assert to detect divergence in tests (debug_assert_eq!(reports.len(), renderer_reports.len());) before iterating to mark changes; also update/remove the now‑stale comment about resize mutating only the editor scene if present. ``` </details> </blockquote></details> <details> <summary>crates/grida-canvas/src/runtime/invalidation/differ.rs (1)</summary><blockquote> `303-308`: **Add table-driven coverage for `diff_node` classification.** This is now a core invalidation primitive, and the per-variant “other” field lists are easy to regress when schema fields change. A small table covering `None`, `Geometry`, `Paint`, `Full`, and one representative “other field” per supported variant would catch under-invalidation before it reaches rendering tests. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/src/runtime/invalidation/differ.rs` around lines 303 - 308, The test module currently has no unit tests for the core diff_node classification; add a table-driven unit test in the #[cfg(test)] mod tests that calls diff_node (the function under test) with representative pairs covering the variants None, Geometry, Paint, Full and one representative "other" field case for each variant to assert the expected DiffKind (or classification) result; structure the test as a loop over cases (name, left_node, right_node, expected) and include one example per variant that would detect regressions when schema fields change, using the same constructors/types used elsewhere in this module so the test stays resilient to refactors. ``` </details> </blockquote></details> </blockquote></details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `5a45f9d0-8510-4623-9571-974ebd5d7a15` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 1fc4308a19241b3f06146cf053d45177092984a4 and 60a394de5c08a9d26b25f9430e88ade571a75326. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `crates/grida-canvas-wasm/lib/bin/grida_canvas_wasm.wasm` is excluded by `!**/*.wasm` * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (78)</summary> * `.agents/skills/cg-perf/SKILL.md` * `.agents/skills/editor-perf/SKILL.md` * `.agents/skills/io-grida/SKILL.md` * `.github/workflows/build-canvas.yml` * `crates/grida-canvas-wasm/AGENTS.md` * `crates/grida-canvas-wasm/README.md` * `crates/grida-canvas-wasm/example/demo.grida1` * `crates/grida-canvas-wasm/example/main.js` * `crates/grida-canvas-wasm/lib/__test__/bench-load-scene.test.ts` * `crates/grida-canvas-wasm/lib/__test__/environment-node-api-spec-validation.test.ts` * `crates/grida-canvas-wasm/lib/__test__/environment-node-raster-export.test.ts` * `crates/grida-canvas-wasm/lib/api.d.ts` * `crates/grida-canvas-wasm/lib/index.ts` * `crates/grida-canvas-wasm/lib/modules/canvas-bindings.d.ts` * `crates/grida-canvas-wasm/lib/modules/canvas.ts` * `crates/grida-canvas-wasm/src/wasm_application.rs` * `crates/grida-canvas/AGENTS.md` * `crates/grida-canvas/Cargo.toml` * `crates/grida-canvas/README.md` * `crates/grida-canvas/examples/tool_io_grida.rs` * `crates/grida-canvas/package.json` * `crates/grida-canvas/src/cache/geometry.rs` * `crates/grida-canvas/src/cache/scene.rs` * `crates/grida-canvas/src/cg/fe.rs` * `crates/grida-canvas/src/cg/stroke_dasharray.rs` * `crates/grida-canvas/src/cg/stroke_width.rs` * `crates/grida-canvas/src/cg/types.rs` * `crates/grida-canvas/src/cg/varwidth.rs` * `crates/grida-canvas/src/io/id_converter.rs` * `crates/grida-canvas/src/io/io_grida.rs` * `crates/grida-canvas/src/io/io_grida_fbs.rs` * `crates/grida-canvas/src/io/io_grida_file.rs` * `crates/grida-canvas/src/io/io_grida_patch.rs` * `crates/grida-canvas/src/io/mod.rs` * `crates/grida-canvas/src/io/vn_json.rs` * `crates/grida-canvas/src/node/scene_graph.rs` * `crates/grida-canvas/src/node/schema.rs` * `crates/grida-canvas/src/painter/layer.rs` * `crates/grida-canvas/src/runtime/changes.rs` * `crates/grida-canvas/src/runtime/invalidation/change_kind.rs` * `crates/grida-canvas/src/runtime/invalidation/differ.rs` * `crates/grida-canvas/src/runtime/invalidation/lens.rs` * `crates/grida-canvas/src/runtime/invalidation/mod.rs` * `crates/grida-canvas/src/runtime/invalidation/scene_dirty.rs` * `crates/grida-canvas/src/runtime/mod.rs` * `crates/grida-canvas/src/runtime/scene.rs` * `crates/grida-canvas/src/window/application.rs` * `crates/grida-canvas/src/window/application_emscripten.rs` * `crates/grida-canvas/tests/isolation_mode.rs` * `crates/grida-dev/src/bench/args.rs` * `crates/grida-dev/src/bench/runner.rs` * `crates/grida-dev/src/editor/document.rs` * `crates/grida-dev/src/editor/mutation.rs` * `docs/wg/research/chromium/dirty-flag-management.md` * `docs/wg/research/chromium/index.md` * `editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx` * `editor/grida-canvas-react/provider.tsx` * `editor/grida-canvas-react/viewport/surface-hooks.ts` * `editor/grida-canvas-react/viewport/surface.tsx` * `editor/grida-canvas/__tests__/bench/README.md` * `editor/grida-canvas/__tests__/bench/_utils.ts` * `editor/grida-canvas/__tests__/bench/perf-editor.test.ts` * `editor/grida-canvas/__tests__/bench/perf-per-node-sync.test.ts` * `editor/grida-canvas/__tests__/bench/perf-reducer.bench.ts` * `editor/grida-canvas/__tests__/bench/perf-reducer.test.ts` * `editor/grida-canvas/__tests__/headless/translate-correctness.test.ts` * `editor/grida-canvas/editor.i.ts` * `editor/grida-canvas/editor.ts` * `editor/grida-canvas/reducers/index.ts` * `editor/grida-canvas/sync.ts` * `editor/scaffolds/sidecontrol/sidecontrol-document-properties.tsx` * `format/properties.md` * `format/properties.toml` * `packages/grida-canvas-io/format.ts` * `packages/grida-canvas-io/index.ts` * `packages/grida-canvas-sdk-render-figma/lib.ts` * `packages/grida-canvas-sdk-render-figma/package.json` * `turbo.json` </details> <details> <summary>💤 Files with no reviewable changes (8)</summary> * crates/grida-canvas/Cargo.toml * editor/grida-canvas/__tests__/bench/perf-reducer.bench.ts * crates/grida-canvas/examples/tool_io_grida.rs * crates/grida-canvas/src/io/io_grida_patch.rs * crates/grida-canvas/src/runtime/changes.rs * editor/grida-canvas/__tests__/bench/perf-reducer.test.ts * crates/grida-canvas/src/io/id_converter.rs * crates/grida-canvas-wasm/lib/api.d.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+77
to
+83
| # Build the one package the wasm vitest imports — turbo pulls its | ||
| # workspace deps (@grida/schema, cg, cmath, format, sequence). | ||
| - name: Build workspace packages for tests | ||
| run: pnpm turbo build --filter=@grida/canvas-io | ||
|
|
||
| - name: Run canvas wasm smoke test | ||
| run: pnpm --filter @grida/canvas-wasm test |
There was a problem hiding this comment.
Expand the PR path filter for the package dependency this job now builds.
This job now builds @grida/canvas-io, but the workflow only triggers for .github/workflows/** and crates/**. A package-only change can skip the canvas-wasm smoke test even though it depends on that package.
🛠️ Proposed workflow trigger update
pull_request:
paths:
- ".github/workflows/**"
- "crates/**"
+ - "packages/**"
+ - "package.json"
+ - "pnpm-lock.yaml"
+ - "pnpm-workspace.yaml"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-canvas.yml around lines 77 - 83, Update the workflow
trigger so changes to the package the job now builds cause the job to run: add
the package path(s) for `@grida/canvas-io` (and any sibling package paths like
packages or src that own it) to the workflow's "paths" or PR filter so the
"Build workspace packages for tests" and subsequent "Run canvas wasm smoke test"
steps are triggered when `@grida/canvas-io` changes; locate references to those
step names and the package name "@grida/canvas-io" in the workflow and add the
appropriate path patterns (e.g. the package directory) to the workflow trigger
block.
Comment on lines
+304
to
+318
| /** | ||
| * Activate a scene previously decoded by `loadSceneGrida`. | ||
| * Required before `exportNodeAs` or other rendering ops when the document | ||
| * contains multiple scenes or when no default is implicitly activated. | ||
| */ | ||
| switchScene(sceneId: string) { | ||
| this._scene.switchScene(sceneId); | ||
| } | ||
|
|
||
| /** | ||
| * Return the IDs of all scenes decoded by the last `loadSceneGrida` call. | ||
| */ | ||
| loadedSceneIds(): string[] { | ||
| return this._scene.loadedSceneIds(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect checked-in declaration files, if present, for the new Canvas API surface.
# Expect: any published/checked-in Canvas declaration includes switchScene(sceneId: string) and loadedSceneIds(): string[].
fd -e d.ts . crates/grida-canvas-wasm -x sh -c '
echo "## $1"
rg -n -C3 "class Canvas|interface Canvas|switchScene|loadedSceneIds" "$1" || true
' sh {}Repository: gridaco/grida
Length of output: 690
🏁 Script executed:
#!/bin/bash
# Check the main exported declaration file for the Canvas class
# Look at grida-canvas-wasm.d.ts
echo "=== grida-canvas-wasm.d.ts content ==="
if [ -f crates/grida-canvas-wasm/lib/grida-canvas-wasm.d.ts ]; then
cat -n crates/grida-canvas-wasm/lib/grida-canvas-wasm.d.ts
else
echo "File not found at lib/grida-canvas-wasm.d.ts"
fi
# Also check for index.d.ts
echo -e "\n=== index.d.ts content ==="
if [ -f crates/grida-canvas-wasm/lib/index.d.ts ]; then
cat -n crates/grida-canvas-wasm/lib/index.d.ts
else
echo "File not found at lib/index.d.ts"
fi
# List all .d.ts files with their size
echo -e "\n=== All .d.ts files found ==="
fd -e d.ts crates/grida-canvas-wasm -x sh -c 'echo "$1: $(wc -l < "$1") lines"' sh {}Repository: gridaco/grida
Length of output: 651
🏁 Script executed:
#!/bin/bash
# Check the Canvas class definition in lib/index.ts to see what's exported
# Find the Canvas class definition
echo "=== Canvas class in lib/index.ts ==="
rg -n "export class Canvas|switchScene|loadedSceneIds" crates/grida-canvas-wasm/lib/index.ts -A2 -B2Repository: gridaco/grida
Length of output: 596
🏁 Script executed:
#!/bin/bash
# Check package.json to understand declaration file configuration
echo "=== package.json types field ==="
cd crates/grida-canvas-wasm && jq -r '.types // .typings // "not set"' package.jsonRepository: gridaco/grida
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Check build configuration and declaration generation setup
echo "=== tsup.config.ts ==="
if [ -f crates/grida-canvas-wasm/tsup.config.ts ]; then
cat -n crates/grida-canvas-wasm/tsup.config.ts
else
echo "File not found"
fi
echo -e "\n=== package.json exports field ==="
cd crates/grida-canvas-wasm && jq '.exports' package.json
echo -e "\n=== dist directory structure (if exists) ==="
if [ -d crates/grida-canvas-wasm/dist ]; then
fd . crates/grida-canvas-wasm/dist -type f | head -20
else
echo "dist/ not found"
fiRepository: gridaco/grida
Length of output: 711
🏁 Script executed:
#!/bin/bash
# Check if declarations are mentioned in any build scripts or generated files
echo "=== package.json build scripts ==="
cd crates/grida-canvas-wasm && jq '.scripts' package.json
echo -e "\n=== Check for any generated declaration markers ==="
rg -l "generated" crates/grida-canvas-wasm --type typescript --type javascript | head -10Repository: gridaco/grida
Length of output: 427
🏁 Script executed:
#!/bin/bash
# Check full package.json content for types/exports configuration
cd crates/grida-canvas-wasm && cat package.jsonRepository: gridaco/grida
Length of output: 879
Add "types": "dist/index.d.ts" to package.json.
Declarations for the new Canvas.switchScene and Canvas.loadedSceneIds methods will be auto-generated by tsup (via dts: true), since both methods are properly exported from lib/index.ts. However, TypeScript consumers won't discover them without a types field in package.json pointing to the generated declaration file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas-wasm/lib/index.ts` around lines 304 - 318, Add a "types"
entry to package.json so TypeScript consumers can discover the generated
declarations for the new Canvas methods; specifically, after building with tsup
(dts: true) point the package.json "types" field to the generated declaration
file (e.g. "dist/index.d.ts") so declarations for switchScene and loadedSceneIds
exported from lib/index.ts are published and resolvable by TypeScript.
Comment on lines
+27
to
+44
| impl From<JSONVectorNetwork> for VectorNetwork { | ||
| fn from(network: JSONVectorNetwork) -> Self { | ||
| VectorNetwork { | ||
| vertices: network.vertices.into_iter().map(|v| (v.0, v.1)).collect(), | ||
| segments: network | ||
| .segments | ||
| .into_iter() | ||
| .map(|s| VectorNetworkSegment { | ||
| a: s.a, | ||
| b: s.b, | ||
| ta: s.ta, | ||
| tb: s.tb, | ||
| }) | ||
| .collect(), | ||
| regions: vec![], | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Validate segment indices before constructing VectorNetwork.
The deserializable wire type can create segments whose a/b indices are outside vertices. Prefer a fallible conversion so malformed JS/JSON input cannot produce an invalid internal network.
Proposed direction
-impl From<JSONVectorNetwork> for VectorNetwork {
- fn from(network: JSONVectorNetwork) -> Self {
+impl TryFrom<JSONVectorNetwork> for VectorNetwork {
+ type Error = &'static str;
+
+ fn try_from(network: JSONVectorNetwork) -> Result<Self, Self::Error> {
+ let vertex_count = network.vertices.len();
+ if network
+ .segments
+ .iter()
+ .any(|s| s.a >= vertex_count || s.b >= vertex_count)
+ {
+ return Err("vector network segment references an out-of-bounds vertex");
+ }
- VectorNetwork {
+ Ok(VectorNetwork {
vertices: network.vertices.into_iter().map(|v| (v.0, v.1)).collect(),
segments: network
.segments
.into_iter()
@@
.collect(),
regions: vec![],
- }
+ })
}
}📝 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
| impl From<JSONVectorNetwork> for VectorNetwork { | |
| fn from(network: JSONVectorNetwork) -> Self { | |
| VectorNetwork { | |
| vertices: network.vertices.into_iter().map(|v| (v.0, v.1)).collect(), | |
| segments: network | |
| .segments | |
| .into_iter() | |
| .map(|s| VectorNetworkSegment { | |
| a: s.a, | |
| b: s.b, | |
| ta: s.ta, | |
| tb: s.tb, | |
| }) | |
| .collect(), | |
| regions: vec![], | |
| } | |
| } | |
| } | |
| impl TryFrom<JSONVectorNetwork> for VectorNetwork { | |
| type Error = &'static str; | |
| fn try_from(network: JSONVectorNetwork) -> Result<Self, Self::Error> { | |
| let vertex_count = network.vertices.len(); | |
| if network | |
| .segments | |
| .iter() | |
| .any(|s| s.a >= vertex_count || s.b >= vertex_count) | |
| { | |
| return Err("vector network segment references an out-of-bounds vertex"); | |
| } | |
| Ok(VectorNetwork { | |
| vertices: network.vertices.into_iter().map(|v| (v.0, v.1)).collect(), | |
| segments: network | |
| .segments | |
| .into_iter() | |
| .map(|s| VectorNetworkSegment { | |
| a: s.a, | |
| b: s.b, | |
| ta: s.ta, | |
| tb: s.tb, | |
| }) | |
| .collect(), | |
| regions: vec![], | |
| }) | |
| } | |
| } |
Comment on lines
+254
to
+280
| const selectedNodes = useEditorState( | ||
| instance, | ||
| (s) => __node_ids.map((id) => s.document.nodes[id]), | ||
| (a, b) => { | ||
| if (a === b) return true; | ||
| if (a.length !== b.length) return false; | ||
| for (let i = 0; i < a.length; i++) { | ||
| if (a[i] !== b[i]) return false; | ||
| } | ||
| return true; | ||
| } | ||
| ); | ||
| const document_ctx = useEditorState( | ||
| instance, | ||
| (s) => s.document_ctx, | ||
| Object.is | ||
| ); | ||
|
|
||
| const [groups, setGroups] = useState<SurfaceSelectionGroup[]>([]); | ||
|
|
||
| const grouped = useMemo(() => { | ||
| const activenodes = __node_ids | ||
| .map((id) => document.nodes[id]) | ||
| .filter((n) => n?.active); | ||
| const activenodes = selectedNodes.filter((n) => n?.active); | ||
| return Object.groupBy( | ||
| activenodes, | ||
| (it) => dq.getParentId(document_ctx, it.id) ?? "" | ||
| ); | ||
| }, [document.nodes, document_ctx, __node_ids]); | ||
| }, [selectedNodes, document_ctx]); |
There was a problem hiding this comment.
Subscribe to parent geometry dependencies for group overlays.
The group overlay is computed from absolute geometry, but this selector only tracks selected node refs. If a selected node’s parent moves or reflows without changing the child node object, the overlay can stay stale. Include parent/ancestor node refs or a geometry revision in the subscription that drives recomputation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/grida-canvas-react/viewport/surface-hooks.ts` around lines 254 - 280,
The grouped overlay can go stale because the useMemo only depends on
selectedNodes and document_ctx but selectedNodes is derived only from child node
refs; update the subscription to also track parent/ancestor geometry changes by
including parent node refs or a geometry revision in the selector — e.g., when
computing selectedNodes (or when creating grouped) return/include the parent
node objects (s.document.nodes[parentId]) or a shared geometry revision field
from s.document so that grouped (and its call to dq.getParentId) re-runs when a
parent moves; adjust the selector(s) around selectedNodes / document_ctx /
grouped to reference those parent nodes or the geometry revision so overlays
update correctly.
Comment on lines
+36
to
+40
| if (typeof globalThis.reportError === "undefined") { | ||
| ( | ||
| globalThis as unknown as { reportError: (err: unknown) => void } | ||
| ).reportError = (_err: unknown) => {}; | ||
| } |
There was a problem hiding this comment.
Don’t swallow reportError in headless runs.
This no-op can hide async WASM/editor subscriber failures, letting perf tests pass with broken sync. Surface the error asynchronously instead.
Proposed fix
if (typeof globalThis.reportError === "undefined") {
(
globalThis as unknown as { reportError: (err: unknown) => void }
- ).reportError = (_err: unknown) => {};
+ ).reportError = (err: unknown) => {
+ setImmediate(() => {
+ throw err instanceof Error ? err : new Error(String(err));
+ });
+ };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/grida-canvas/__tests__/bench/_utils.ts` around lines 36 - 40, The
current no-op assignment for globalThis.reportError swallows errors; replace the
noop with an implementation that surfaces errors asynchronously (e.g.,
setTimeout(() => { throw err }) or queueMicrotask) so async WASM/editor
subscriber failures are not hidden; modify the block that assigns (globalThis as
unknown as { reportError: (err: unknown) => void }).reportError to call an async
rethrow of _err instead of doing nothing.
Comment on lines
+59
to
+60
| describe("per-node sync (immer patches on)", () => { | ||
| for (const size of [1_000, 10_000] as const) { |
There was a problem hiding this comment.
Gate this benchmark behind GRIDA_PERF.
The file documents GRIDA_PERF=1, but the suite runs unconditionally. That can pull the 10K-node WASM benchmark into normal Vitest/CI runs.
Suggested fix
-describe("per-node sync (immer patches on)", () => {
+const describePerf =
+ process.env.GRIDA_PERF === "1" ? describe : describe.skip;
+
+describePerf("per-node sync (immer patches on)", () => {📝 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
| describe("per-node sync (immer patches on)", () => { | |
| for (const size of [1_000, 10_000] as const) { | |
| const describePerf = | |
| process.env.GRIDA_PERF === "1" ? describe : describe.skip; | |
| describePerf("per-node sync (immer patches on)", () => { | |
| for (const size of [1_000, 10_000] as const) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/grida-canvas/__tests__/bench/perf-per-node-sync.test.ts` around lines
59 - 60, Gate the "per-node sync (immer patches on)" benchmark suite behind the
GRIDA_PERF env var by checking process.env.GRIDA_PERF (or
import.meta.env.GRIDA_PERF if you use Vite envs) before running the tests: if
the variable is not set/truthy, mark the suite skipped (e.g., use describe.skip
or return early) so the 1_000 and 10_000 sizes in the for loop are not executed
in normal CI/Vitest runs; update the describe block name "per-node sync (immer
patches on)" accordingly to only run when GRIDA_PERF is enabled.
Comment on lines
4577
to
+4588
| if (this.fontCollection) { | ||
| void Promise.all(fonts.map((family) => this.loadFontSync({ family }))); | ||
| void this.fontCollection.setFallbackFonts(fonts); | ||
| void Promise.all( | ||
| fonts.map((family) => | ||
| this.loadFontSync({ family }).catch(() => { | ||
| // individual font failures must not reject the warmup chain | ||
| }) | ||
| ) | ||
| ); | ||
| if (this._disposed) return; | ||
| try { | ||
| void this.fontCollection.setFallbackFonts(fonts); | ||
| } catch { |
There was a problem hiding this comment.
Await font warmup before installing fallback fonts.
void Promise.all(...) starts font loads but does not wait for them, so Line 4585 checks _disposed immediately and Line 4587 can run before fonts are loaded. This also makes loadPlatformDefaultFonts() resolve before warmup is actually complete.
🐛 Proposed fix
if (this.fontCollection) {
- void Promise.all(
+ await Promise.all(
fonts.map((family) =>
this.loadFontSync({ family }).catch(() => {
// individual font failures must not reject the warmup chain
})
)
);
if (this._disposed) return;
try {
- void this.fontCollection.setFallbackFonts(fonts);
+ await this.fontCollection.setFallbackFonts(fonts);
} catch {
// scene may have been disposed between the guard and the call
}📝 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
| if (this.fontCollection) { | |
| void Promise.all(fonts.map((family) => this.loadFontSync({ family }))); | |
| void this.fontCollection.setFallbackFonts(fonts); | |
| void Promise.all( | |
| fonts.map((family) => | |
| this.loadFontSync({ family }).catch(() => { | |
| // individual font failures must not reject the warmup chain | |
| }) | |
| ) | |
| ); | |
| if (this._disposed) return; | |
| try { | |
| void this.fontCollection.setFallbackFonts(fonts); | |
| } catch { | |
| if (this.fontCollection) { | |
| await Promise.all( | |
| fonts.map((family) => | |
| this.loadFontSync({ family }).catch(() => { | |
| // individual font failures must not reject the warmup chain | |
| }) | |
| ) | |
| ); | |
| if (this._disposed) return; | |
| try { | |
| await this.fontCollection.setFallbackFonts(fonts); | |
| } catch { |
🤖 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 4577 - 4588, The code currently
fires font warmups with void Promise.all(...) so it doesn't wait for
this.loadFontSync calls to finish before checking this._disposed or calling
this.fontCollection.setFallbackFonts; change the logic to await
Promise.all(fonts.map(family => this.loadFontSync({ family }).catch(() => {})))
so the warmup completes first, then recheck this._disposed and only then call
this.fontCollection.setFallbackFonts; ensure the method that calls this block
(e.g., loadPlatformDefaultFonts) awaits this sequence so callers observe
completion.
Comment on lines
+5192
to
+5201
| function hasLayoutFields( | ||
| node: grida.program.nodes.Node | ||
| ): node is grida.program.nodes.Node & | ||
| Parameters<typeof format.layout.encode.nodeLayout>[1] { | ||
| const n = node as grida.program.nodes.UnknownNode; | ||
| return ( | ||
| n.layout_positioning !== undefined && | ||
| n.layout_target_width !== undefined && | ||
| n.layout_target_height !== undefined | ||
| ); |
There was a problem hiding this comment.
Don’t drop valid auto-sized layout blocks.
nodeLayout already supports missing dimensions by encoding them as "auto", but this guard requires both target width and height. A node with only positioning/insets changed, or an auto-sized container, will be serialized without any layout block in the per-node path.
Suggested adjustment
function hasLayoutFields(
node: grida.program.nodes.Node
): node is grida.program.nodes.Node &
Parameters<typeof format.layout.encode.nodeLayout>[1] {
const n = node as grida.program.nodes.UnknownNode;
- return (
- n.layout_positioning !== undefined &&
- n.layout_target_width !== undefined &&
- n.layout_target_height !== undefined
- );
+ return (
+ n.layout_positioning !== undefined ||
+ n.layout_inset_left !== undefined ||
+ n.layout_inset_top !== undefined ||
+ n.layout_inset_right !== undefined ||
+ n.layout_inset_bottom !== undefined ||
+ n.layout_target_width !== undefined ||
+ n.layout_target_height !== undefined
+ );
}📝 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
| function hasLayoutFields( | |
| node: grida.program.nodes.Node | |
| ): node is grida.program.nodes.Node & | |
| Parameters<typeof format.layout.encode.nodeLayout>[1] { | |
| const n = node as grida.program.nodes.UnknownNode; | |
| return ( | |
| n.layout_positioning !== undefined && | |
| n.layout_target_width !== undefined && | |
| n.layout_target_height !== undefined | |
| ); | |
| function hasLayoutFields( | |
| node: grida.program.nodes.Node | |
| ): node is grida.program.nodes.Node & | |
| Parameters<typeof format.layout.encode.nodeLayout>[1] { | |
| const n = node as grida.program.nodes.UnknownNode; | |
| return ( | |
| n.layout_positioning !== undefined || | |
| n.layout_inset_left !== undefined || | |
| n.layout_inset_top !== undefined || | |
| n.layout_inset_right !== undefined || | |
| n.layout_inset_bottom !== undefined || | |
| n.layout_target_width !== undefined || | |
| n.layout_target_height !== undefined | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io/format.ts` around lines 5192 - 5201, The guard in
hasLayoutFields is too strict (it demands all three fields) and drops valid
auto-sized or positioning-only layout blocks; update the condition to return
true when any layout-related field is present (e.g., n.layout_positioning !==
undefined || n.layout_target_width !== undefined || n.layout_target_height !==
undefined) so format.layout.encode.nodeLayout can encode missing dimensions as
"auto" and still emit the layout block; reference hasLayoutFields and
format.layout.encode.nodeLayout when making the change.
Comment on lines
+5335
to
+5354
| * Encodes a single node as a minimal `GridaFile` buffer containing one | ||
| * `NodeSlot`. The consumer identifies the target by id; hierarchy is | ||
| * not re-encoded and `schema_version` is omitted. | ||
| */ | ||
| export function nodeToFlatbuffer( | ||
| node: grida.program.nodes.Node | ||
| ): Uint8Array { | ||
| const builder = new flatbuffers.Builder(1024); | ||
|
|
||
| const slotOffset = encodeNodeSlot(builder, node, undefined); | ||
|
|
||
| const nodesOffset = fbs.CanvasDocument.createNodesVector(builder, [ | ||
| slotOffset, | ||
| ]); | ||
| const scenesOffset = fbs.CanvasDocument.createScenesVector(builder, []); | ||
|
|
||
| fbs.CanvasDocument.startCanvasDocument(builder); | ||
| fbs.CanvasDocument.addNodes(builder, nodesOffset); | ||
| fbs.CanvasDocument.addScenes(builder, scenesOffset); | ||
| const documentOffset = fbs.CanvasDocument.endCanvasDocument(builder); |
There was a problem hiding this comment.
Include schema_version in single-node FlatBuffers.
nodeToFlatbuffer returns a GridaFile, but omits CanvasDocument.schema_version; fromFlatbuffer treats the missing value as "" and rejects it as incompatible. This makes the new encoder produce buffers that the package’s own decoder cannot read.
Suggested fix
export function nodeToFlatbuffer(
- node: grida.program.nodes.Node
+ node: grida.program.nodes.Node,
+ schemaVersion: string = grida.program.document.SCHEMA_VERSION
): Uint8Array {
const builder = new flatbuffers.Builder(1024);
+ const schemaVersionOffset = builder.createString(schemaVersion);
const slotOffset = encodeNodeSlot(builder, node, undefined);
const nodesOffset = fbs.CanvasDocument.createNodesVector(builder, [
slotOffset,
]);
const scenesOffset = fbs.CanvasDocument.createScenesVector(builder, []);
fbs.CanvasDocument.startCanvasDocument(builder);
+ fbs.CanvasDocument.addSchemaVersion(builder, schemaVersionOffset);
fbs.CanvasDocument.addNodes(builder, nodesOffset);
fbs.CanvasDocument.addScenes(builder, scenesOffset);
const documentOffset = fbs.CanvasDocument.endCanvasDocument(builder);📝 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
| * Encodes a single node as a minimal `GridaFile` buffer containing one | |
| * `NodeSlot`. The consumer identifies the target by id; hierarchy is | |
| * not re-encoded and `schema_version` is omitted. | |
| */ | |
| export function nodeToFlatbuffer( | |
| node: grida.program.nodes.Node | |
| ): Uint8Array { | |
| const builder = new flatbuffers.Builder(1024); | |
| const slotOffset = encodeNodeSlot(builder, node, undefined); | |
| const nodesOffset = fbs.CanvasDocument.createNodesVector(builder, [ | |
| slotOffset, | |
| ]); | |
| const scenesOffset = fbs.CanvasDocument.createScenesVector(builder, []); | |
| fbs.CanvasDocument.startCanvasDocument(builder); | |
| fbs.CanvasDocument.addNodes(builder, nodesOffset); | |
| fbs.CanvasDocument.addScenes(builder, scenesOffset); | |
| const documentOffset = fbs.CanvasDocument.endCanvasDocument(builder); | |
| * Encodes a single node as a minimal `GridaFile` buffer containing one | |
| * `NodeSlot`. The consumer identifies the target by id; hierarchy is | |
| * not re-encoded and `schema_version` is omitted. | |
| */ | |
| export function nodeToFlatbuffer( | |
| node: grida.program.nodes.Node, | |
| schemaVersion: string = grida.program.document.SCHEMA_VERSION | |
| ): Uint8Array { | |
| const builder = new flatbuffers.Builder(1024); | |
| const schemaVersionOffset = builder.createString(schemaVersion); | |
| const slotOffset = encodeNodeSlot(builder, node, undefined); | |
| const nodesOffset = fbs.CanvasDocument.createNodesVector(builder, [ | |
| slotOffset, | |
| ]); | |
| const scenesOffset = fbs.CanvasDocument.createScenesVector(builder, []); | |
| fbs.CanvasDocument.startCanvasDocument(builder); | |
| fbs.CanvasDocument.addSchemaVersion(builder, schemaVersionOffset); | |
| fbs.CanvasDocument.addNodes(builder, nodesOffset); | |
| fbs.CanvasDocument.addScenes(builder, scenesOffset); | |
| const documentOffset = fbs.CanvasDocument.endCanvasDocument(builder); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/grida-canvas-io/format.ts` around lines 5335 - 5354,
nodeToFlatbuffer currently omits CanvasDocument.schema_version which makes
buffers unreadable by fromFlatbuffer; fix by creating a string offset for the
correct schema version (e.g., CURRENT_SCHEMA_VERSION or the package's schema
constant) via builder.createString and call
fbs.CanvasDocument.addSchemaVersion(builder, schemaVersionOffset) before ending
the document in nodeToFlatbuffer so the produced GridaFile includes
schema_version that fromFlatbuffer can validate.
| }, | ||
| "test": { | ||
| "dependsOn": ["^test"] | ||
| "dependsOn": ["^build"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Print the Turbo test task dependency configuration.
jq '.tasks.test' turbo.jsonRepository: gridaco/grida
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Inspect full turbo.json structure to understand build and test tasks
cat turbo.json | jq '.'Repository: gridaco/grida
Length of output: 614
🏁 Script executed:
#!/bin/bash
# Check if there's a build task definition and its outputs
jq '.tasks.build' turbo.jsonRepository: gridaco/grida
Length of output: 208
🏁 Script executed:
#!/bin/bash
# Search for test files to understand what they might depend on
fd -e 'test\.' -o -e 'spec\.' --type f | head -20Repository: gridaco/grida
Length of output: 173
🏁 Script executed:
#!/bin/bash
# Search for test files - fix the fd command syntax
fd -type f \( -name '*.test.*' -o -name '*.spec.*' \) | head -20Repository: gridaco/grida
Length of output: 227
🏁 Script executed:
#!/bin/bash
# Search for references to build artifacts in test files
rg 'dist|build|\.next' --type ts --type tsx -l | grep -i test | head -10Repository: gridaco/grida
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Check if any test configuration references build artifacts or runs build as a prerequisite
find . -maxdepth 3 -name 'jest.config.*' -o -name 'vitest.config.*' -o -name 'package.json' | xargs grep -l "test" | head -5Repository: gridaco/grida
Length of output: 203
🏁 Script executed:
#!/bin/bash
# Check vitest and test configurations to see if they depend on build outputs
cat crates/grida-canvas-wasm/vitest.config.tsRepository: gridaco/grida
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Check test scripts in workspace packages
rg '"test"' --type json -A 2 package.json | head -30Repository: gridaco/grida
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Search for build artifact references in test files
rg 'dist|build|\.next' crates/grida-canvas-wasm/ --max-count 20 | head -20Repository: gridaco/grida
Length of output: 1947
Include the package's own build before test.
^build only targets dependency packages' tasks, not the current package's own build task. Since build outputs (dist/, build/, .next/) are needed for tests to run, especially for WASM-dependent tests, add "build" to ensure each package builds before its tests.
Suggested task graph fix
- "dependsOn": ["^build"]
+ "dependsOn": ["build", "^build"]📝 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
| "dependsOn": ["^build"] | |
| "dependsOn": ["build", "^build"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@turbo.json` at line 9, The test task's dependsOn currently only lists the
task selector "^build" which runs builds for dependencies but not the package's
own build; update the test task's dependsOn to include the literal "build"
alongside "^build" so the package builds itself before running tests (i.e.
change dependsOn from ["^build"] to ["build", "^build"] on the test task).
Flaky on GitHub Actions runners — CPU-bound variable font parse took 21s on run 24632581106, just over the 20s cap. Sibling test parsing 4 fonts finishes in 14s, so 60s is generous headroom without masking real regressions.
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
Started as the properties catalog PR; grew into the end-to-end perf path that catalog work exposed. Three threads land together because they share the same benchmark harness and fixture set:
replaceNode/deleteNoderouted via a first-class Effect protocol.ChangeKind-driven per-subtree rebuild replaces scene-wide cache rebuild.Net effect on 10K-node editor dispatch: ~100ms → ~600µs. On 136K-node scenes: apply_changes 348ms → ~5ms; frame 413ms → 63ms (2.4 → 15.8 fps).
What lands
Properties catalog (
format/)format/properties.toml— 144[[column]]entries covering every persisted field reachable fromNodeingrida.fbs, plus one runtime-only entry (deleted). Each has id, logical type, nullability, applies-to,fbs_path, default semantics, notes.format/properties.md— contributor reference: schema, id-stability contract, topical range allocation, FBS mapping policy, known asymmetries, contributor checklist.post_layout_/layout_prefixes for semantic clarity.Editor → WASM sync (
editor/grida-canvas/,crates/grida-canvas-wasm/)replace_node_grida/delete_nodeC exports replace full-documentsync_documenton every dispatch.Effectprotocol (none | nodes | structural) emitted by the reducer. Both the Immer path (lifted from patches) and the gesture bypass path (computed from action + gesture state) produce the same protocol — the subscriber always knows what changed. Fixes the silent full-re-encode fallback that defeated the gesture bypass.mountShared(surface)consolidatesmountandmountHeadlessso bench and browser run the identical subscriber graph.sync.tsextracts__wasm_sync_document/__wasm_on_document_change/__wasm_on_scene_id_changeout ofmount().Canvas.switchScene/loadedSceneIdspassthroughs so multi-scene documents activate through the wrapper;refigcallsswitchSceneafterloadSceneGrida.useEditorStatereplaces deprecateduseDocumentStatefor narrower subscriptions.Renderer invalidation (
crates/grida-canvas/src/runtime/invalidation/)change_kind.rs(ChangeKind / Damage / GlobalFlag),lens.rs(Motion / Paint lenses),differ.rs(purediff_node(old, new) -> ChangeKind),scene_dirty.rs(per-phase dirty sets + classifier).GeometryCache::update_subtree+ in-placelayer.base.transform+ R-tree remove+insert for the affected subtree. Widenspaint_touchedto the full subtree so picture/compositor cache invalidation covers every node whose world transform changed.rebuild_layer_vecrefreshes fills/strokes/opacity/effects/shape/stroke_path without touching geometry or R-tree.ChangeFlags/ChangeSet/mark_changed/mark_node_changedformark_global(GlobalFlag)+mark_node_change_kind(ChangeKind).PartialEqtoLayerEffects,StrokeStyle,StrokeWidth,StrokeDashArray, layout style types,CGPoint,VarWidthProfile,WidthStop, FE types.patch_layers_for_subtreenow walks the parallelLayerList.commandstree too — previouslyRenderSurface-wrapped nodes (shadow/blur/clip/opacity isolation) kept stale transforms during drag, rendering effect-bearing descendants as ghosts at the old position.clip_pathis reprojected bynew * inv(old)since it was baked in world space.Bench harness (
editor/grida-canvas/__tests__/bench/,crates/grida-dev/)Editor.mountHeadless()against the real raster WASM backend — sodispatch.wasm.*spans attribute cost to the same path the browser takes.grida-dev bench --translate [first|<id>]scenario measures the invalidation pipeline end-to-end withapply_changestiming reported separately..grida1JSON fixtures onto.gridabinary fixtures fromfixtures/test-grida/.Cleanup
.grida1JSON loader:io_grida.rs(4506 lines),io_grida_patch.rs,id_converter.rs,tool_io_grida.rsexample,api.d.ts, demo fixture.Scene.loadScenefallback dropped fromtranslate-correctnesstest.Research / skills
docs/wg/research/chromium/dirty-flag-management.md— survey informing the invalidation design.cg-perf,editor-perf,io-gridaskill docs updated with new bench categories and measurement-tool decision tree.Measured results
node/change/*)apply_changes, 136K nodes670 cg lib tests + 27 integration binaries pass.
Test plan
turbo test --filter='./packages/*' --filter=editorpassescargo test -p cgpasses (670 lib + 27 integration)grida-dev bench --translate firston01-135k.perf.gridareports apply_changes in single-digit msproperties.tomlparses; column count matchesproperties.md🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Breaking Changes
Documentation