direct fbs support#571
Merged
softmarshmallow merged 18 commits intomainfrom Mar 13, 2026
Merged
Conversation
Skip off-screen layers during draw by building a visible-node set from the R-tree frame plan and passing it to new culled-draw methods in Painter. Camera pan/zoom on a 4900-node scene is 67x faster when zoomed in (~1% visible) and 1529x faster when the viewport is empty. - Add draw_layer_list_culled / draw_render_commands_culled to Painter - Build HashSet<NodeId> from FramePlan regions in draw_layers_with_scene_cache - Add viewport_culling correctness tests (3 integration tests) - Add bench_viewport_culling criterion benchmark (5K and 50K nodes)
Add HeadlessGpu for windowless GPU-backed Skia rendering, useful for benchmarks, tests, CLI tools, and future SDK usage. Gated behind the native-gl-context feature flag so cg remains platform-agnostic by default. - Add cg::window::headless module (CGL on macOS, EGL on Linux) - Add headless_gpu example with per-scenario timing stats - Add glutin + raw-window-handle as optional deps behind native-gl-context - Enable native-gl-context in grida-dev
…y culling Use a Vec<bool> indexed by NodeId (sequential u64) instead of HashSet<NodeId> for the per-frame visible-node set. This eliminates hashing overhead and HashMap bucket allocation, giving O(1) array lookups with better cache locality on every draw call.
The R-tree query returns layer indices in arbitrary order. The previous sort ensured Z-order, but with culled drawing the Z-order comes from the command tree traversal, not the index array. The indices are only used to build the visibility bitset and prefill the picture cache, neither of which is order-dependent.
Changed the environment variable from NEXT_PUBLIC_GRIDA_WASM_SERVE_URL to NEXT_PUBLIC_GRIDA_WASM_DEV_SERVE_URL in the .env.example file and updated the locateFile function to use the new variable for local WASM file serving. This change clarifies the purpose of the variable for development environments.
Reverts the draw-time culling introduced in: - feat(cg): viewport culling at draw time - perf(cg): replace HashSet<NodeId> with Vec<bool> bitset for visibility culling - perf(cg): remove unnecessary sort of R-tree indices in frame plan Removes draw_layer_list_culled / draw_render_commands_culled / draw_layer_list_outline_culled from Painter, restores the draw_layer_list call in draw_layers_with_scene_cache, and re-adds the indices.sort() in the frame plan builder for Z-order correctness. Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
WalkthroughThis PR bumps the schema version from 0.90.0 to 0.91.0, introducing breaking FlatBuffers schema changes (NodeSlot wrapper table for node storage), fixes a widespread typo (text_decoration_thinkness → text_decoration_thickness across multiple files), adds multi-scene support to the native demo window, introduces comprehensive L0 test fixtures, and establishes FlatBuffers freshness validation via CI. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FileLoader as File Loader
participant DemoApp as Demo App<br/>(native_demo)
participant NativeApp as Native Application
participant Renderer as Renderer/Scene
participant Window as Window
User->>FileLoader: Load .grida file
FileLoader->>FileLoader: Detect format (ZIP/FBS/JSON)
FileLoader->>DemoApp: decode_all() → Vec<Scene>
alt Multiple Scenes Found
DemoApp->>NativeApp: run_demo_window_multi(scenes)
NativeApp->>NativeApp: Initialize app.scenes<br/>app.scene_index = 0
NativeApp->>Renderer: Load initial scene
Renderer->>Window: Render scene [1/N]
User->>Window: Press PageDown
Window->>NativeApp: NextScene command
NativeApp->>NativeApp: scene_index++<br/>Load new scene
NativeApp->>Renderer: load_scene()<br/>fit_camera_to_scene()
Renderer->>Window: Render scene [2/N]
else Single Scene
DemoApp->>NativeApp: run_demo_window(scene)
NativeApp->>Renderer: Load scene
Renderer->>Window: Render scene
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Updated image source collection to exclude `system://` built-in resources, ensuring they are not embedded in archives. - Introduced `persistable_image_srcs()` method to filter out non-persistable image sources, improving clarity and functionality in image management. This change refines how image sources are processed, aligning with the overall architecture for resource handling.
- Enhanced error handling in the CanvasPlayground component to quarantine stale files when document data becomes unreadable, preserving user data for potential future migration. - Introduced a new `quarantine` method in the grida-canvas-io package to move document files into a `_quarantine` directory, ensuring that original files are removed to prevent data loss after schema changes. - Improved documentation within the quarantine method to clarify its purpose and usage. These changes improve data integrity and user experience by safeguarding against data corruption.
![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: 58b6068fc4
ℹ️ 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".
| use crate::cg::types::ImageMaskType; | ||
| match fbs_mask_type { | ||
| fbs::LayerMaskType::NONE => None, | ||
| fbs::LayerMaskType::LayerMaskTypeImage => Some(LayerMaskType::Image(ImageMaskType::Alpha)), |
There was a problem hiding this comment.
This decoder hard-codes every LayerMaskTypeImage to ImageMaskType::Alpha, so files authored with a luminance mask are silently loaded as alpha masks and render differently after decode. The encoder already preserves Alpha vs Luminance, so this creates a one-way data loss bug whenever a .grida document uses mask: luminance.
Useful? React with 👍 / 👎.
| println!("Loaded {} scenes (PageUp/PageDown to switch)", scenes.len()); | ||
| run_demo_window_multi(scenes).await; | ||
| } else { | ||
| run_demo_window(scenes.into_iter().next().unwrap()).await; |
There was a problem hiding this comment.
run_scene unconditionally unwraps the first element when len() <= 1, so an empty decode result crashes the CLI instead of returning a recoverable error. This is reachable when the input parses but contains no scene entries (e.g., malformed or partially migrated .grida data), and the panic bypasses the existing Result-based error handling path.
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: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
crates/grida-canvas/README.md-113-116 (1)
113-116:⚠️ Potential issue | 🟡 MinorUse
cginstead ofskia-safein the doc command.The README documents
skia-safe, which is a rendering dependency, not this crate's own documentation. The package name for this crate iscg(fromCargo.toml).Suggested fix
- cargo doc --package skia-safe --open --no-deps + cargo doc --package cg --open --no-deps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/README.md` around lines 113 - 116, The README's cargo doc example incorrectly documents the skia-safe package; change the command to generate docs for this crate by replacing "skia-safe" with this crate's package name "cg" (as defined in Cargo.toml), i.e., update the snippet in README.md so it runs "cargo doc --package cg --open --no-deps" to build this crate's docs instead of skia-safe's.fixtures/test-grida/README.md-18-18 (1)
18-18:⚠️ Potential issue | 🟡 MinorRemove reference to non-existent
basic.gridafixture from documentation.The fixture file
basic.gridadocumented at line 18 does not exist infixtures/test-grida/(which only containsL0.gridaandd1-20251209.grida1.zip). Additionally, the referenced testcargo test --package cg --test fbs_roundtripgenerates test data programmatically in-memory—it does not load file-based fixtures. Either create the fixture file or remove this documentation entry and reference only the actual in-memory test generation approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fixtures/test-grida/README.md` at line 18, The README mentions a non-existent fixture "basic.grida"; remove that bullet (the line referencing **`basic.grida`**) and replace it with a short note pointing to the actual in-memory test generation used by the tests (referencing the test command `cargo test --package cg --test fbs_roundtrip`) or explicitly reference the real files (`L0.grida`, `d1-20251209.grida1.zip`) if you prefer a file-based list; update the text so it no longer claims a file-based fixture exists and instead explains that test data is generated in-memory by the fbs_roundtrip test.format/README.md-57-60 (1)
57-60:⚠️ Potential issue | 🟡 MinorClarify whether Rust bindings are committed or ephemeral.
This new note points readers at in-repo generated Rust output, but the section above still says generated FlatBuffers bindings are not committed. With the new automation in this PR, that leaves contributors guessing whether diffs under
crates/grida-canvas/src/io/generated/are expected or a bug. Please make the policy explicit per target.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@format/README.md` around lines 57 - 60, The README currently lists in-repo generated outputs for TypeScript (packages/grida-format/) and Rust (crates/grida-canvas/src/io/generated/) but doesn't state whether the Rust FlatBuffers bindings are committed; update the In-repo generated code + automation section to explicitly state per target whether generated artifacts are committed or ephemeral (e.g., "TypeScript bindings are committed and regenerated by prebuild" vs "Rust FlatBuffers bindings under crates/grida-canvas/src/io/generated/ are NOT committed and are generated during CI/build"), and add guidance for contributors about expected diffs and the correct workflow (how to regenerate locally with pnpm build and when to commit generated files) so reviewers know if changes under crates/grida-canvas/src/io/generated/ are intentional.crates/grida-canvas/Cargo.toml-25-25 (1)
25-25:⚠️ Potential issue | 🟡 MinorAlign the Rust
flatbuffersruntime version with the repo-pinnedflatcgenerator version.This adds
flatbuffers = "25.9.23", but the repo pinsflatc v25.12.19inbin/activate-flatc. When Rust bindings are regenerated fromformat/grida.fbs, the newer generator (25.12.19) may produce code requiring APIs not available in the older runtime (25.9.23). Update the runtime version to25.12.19to match, or document the compatibility contract explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/Cargo.toml` at line 25, The flatbuffers runtime version in Cargo.toml is out of sync with the repo-pinned flatc generator; update the flatbuffers dependency version string from "25.9.23" to "25.12.19" so the Rust runtime matches the generator used by bin/activate-flatc (and avoid generated code/API mismatches when regenerating bindings from format/grida.fbs).crates/grida-canvas/tests/fixtures/README.md-18-356 (1)
18-356:⚠️ Potential issue | 🟡 MinorAdd language tags to the fenced examples.
markdownlint is flagging every unlabeled code block here (MD040), so docs/lint will keep failing until these fences use an explicit language such as
textorshwhere appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/fixtures/README.md` around lines 18 - 356, The fenced code blocks in this README (e.g., the blocks that start with scene "L0 Shapes", scene "L0 Shape Arc", scene "L0 Shape Polygon", etc.) lack language tags and trigger markdownlint MD040; update every triple-backtick fence in this file to include an explicit language token (for example use "text" for plain scenes or "sh" for shell-like snippets) so each fenced example becomes ```text (or ```sh) … ```; ensure you update every occurrence throughout the file including all scene/example sections (L0-vector, L0-paints, L0-effects, L0-type, etc.) to satisfy the linter.crates/grida-dev/src/grida_file.rs-19-25 (1)
19-25:⚠️ Potential issue | 🟡 MinorMatch the full ZIP magic, not just three bytes.
Line 24 only checks
bytes[2], so any payload beginning withPK\x03?orPK\x05?is classified as ZIP and then fails later with a misleading “failed to open .grida ZIP” error. Check byte 3 as well so non-ZIP data falls through toFormat::Unknown.💡 Suggested fix
- if bytes.len() >= 4 - && bytes[0] == 0x50 - && bytes[1] == 0x4b - && (bytes[2] == 0x03 || bytes[2] == 0x05) + if bytes.len() >= 4 + && bytes[0] == 0x50 + && bytes[1] == 0x4b + && ((bytes[2] == 0x03 && bytes[3] == 0x04) + || (bytes[2] == 0x05 && bytes[3] == 0x06)) { return Format::Zip; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/grida_file.rs` around lines 19 - 25, The ZIP detection in fn detect(bytes: &[u8]) -> Format currently only checks bytes[2] and misclassifies inputs like "PK\x03?" as ZIP; update the condition in detect to match the full four-byte ZIP magic by checking both bytes[2] and bytes[3], i.e. treat as ZIP only when (bytes[2] == 0x03 && bytes[3] == 0x04) or (bytes[2] == 0x05 && bytes[3] == 0x06), so non-ZIP payloads fall through to Format::Unknown.crates/grida-canvas/tests/fixtures/l0_layout_position.rs-33-37 (1)
33-37:⚠️ Potential issue | 🟡 MinorThis fixture does not actually cover inset-vs-Cartesian child positioning.
Both
c1andc2go through the samerect(x, y, w, h, paint)helper, so they only differ by coordinates. If this fixture is meant to validate two distinct positioning modes, the inset case needs to be built explicitly instead of reusing the generic rect helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/fixtures/l0_layout_position.rs` around lines 33 - 37, The test fixture uses the same rect(x, y, w, h, paint) helper for both c1 and c2 so it doesn't exercise inset-based positioning; change the c2 construction to create an inset-positioned child explicitly (instead of calling rect) by constructing the node using the inset/top/left API or layout constructor used elsewhere in tests (the symbol to change is c2 and stop using the rect helper), ensuring the inset fields (top=50, left=150) are set on the layout node so the fixture actually validates inset vs Cartesian positioning.crates/grida-canvas/tests/fixtures/mod.rs-97-125 (1)
97-125:⚠️ Potential issue | 🟡 MinorUse the caller-provided scene key when deriving serialized ids.
The first element of
scenes: &[(&str, Scene)]is ignored today. That makes scene ids and node-id prefixes depend on slice order, so simply reordering the inputs churns the persisted fixture identifiers.♻️ Proposed fix
- for (i, (_, scene)) in scenes.iter().enumerate() { - let scene_id = format!("scene{}", i + 1); + for (scene_key, scene) in scenes.iter() { + let scene_id = (*scene_key).to_owned(); let mut id_map = HashMap::new(); let mut position_map = HashMap::new(); - build_maps_prefixed(scene, &mut id_map, &mut position_map, &format!("s{}_", i)); + build_maps_prefixed(scene, &mut id_map, &mut position_map, &format!("{scene_id}_")); entries_data.push((scene_id, id_map, position_map)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/fixtures/mod.rs` around lines 97 - 125, write_multi_fixture currently ignores the caller-provided scene key in scenes: &[(&str, Scene)] and instead generates scene IDs like "scene1", causing IDs to change when order changes; update write_multi_fixture to use the first tuple element (the &str key) as the scene_id and as the prefix passed to build_maps_prefixed (instead of format!("scene{}", i+1) and format!("s{}_", i)), so id_map/position_map and entries_data are derived from the caller-provided key; ensure you propagate that key into entries_data and the later entries vector (keeping build_maps_prefixed, entries_data, and entries usage intact).
🧹 Nitpick comments (8)
packages/grida-format/src/__tests__/index.test.ts (1)
18-18: Use the canonical schema version constant instead of duplicating the literal.This test currently hardcodes the version in both the builder input and the assertion, so it can drift from the actual public schema contract without catching anything meaningful. Import the shared schema version constant and assert against that value instead.
♻️ Suggested change
+import { SCHEMA_VERSION } from "../../grida"; // or the canonical exported location -const schemaVersion = builder.createString("0.91.0-beta+20260311"); +const schemaVersion = builder.createString(SCHEMA_VERSION); -expect(document?.schemaVersion()).toBe("0.91.0-beta+20260311"); +expect(document?.schemaVersion()).toBe(SCHEMA_VERSION);Based on learnings, "When writer behavior changes meaningfully (especially for breaking changes), bump
CanvasDocument.schema_versionand keep it in sync with TSgrida.program.document.SCHEMA_VERSION."Also applies to: 49-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/grida-format/src/__tests__/index.test.ts` at line 18, Test hardcodes the schema version literal; import and use the shared canonical schema version constant (e.g. grida.program.document.SCHEMA_VERSION or CanvasDocument.schema_version) instead of the string literal so the builder.createString call and the assertion both reference that single constant; update the test at the builder.createString("0.91.0-beta+20260311") location and the corresponding expect/assert to use the imported SCHEMA_VERSION symbol.editor/scaffolds/editor/editor.tsx (1)
160-164: UseSCHEMA_VERSIONconstant instead of hardcoding.The schema version is hardcoded, but the codebase pattern uses
grida.program.document.SCHEMA_VERSION(already imported viagridaon line 32). This prevents version drift—other files likeeditor/services/new/index.tsandeditor/scaffolds/editor/reducer.tsfollow this pattern.♻️ Suggested refactor
return await client .from("canvas_document") .update({ data: document ? ({ - __schema_version: "0.91.0-beta+20260311", + __schema_version: grida.program.document.SCHEMA_VERSION, ...document, } satisfies CanvasDocumentSnapshotSchema as {}) : null, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/scaffolds/editor/editor.tsx` around lines 160 - 164, Replace the hardcoded schema string "__schema_version: \"0.91.0-beta+20260311\"" with the shared constant used elsewhere (grida.program.document.SCHEMA_VERSION or imported SCHEMA_VERSION) so the object built in the data field uses that constant when document is present; update the object spread where CanvasDocumentSnapshotSchema is satisfied to reference grida.program.document.SCHEMA_VERSION instead of the literal to prevent version drift and keep consistent with editor/services/new/index.ts and editor/scaffolds/editor/reducer.ts.editor/scaffolds/editor/sync/agent-startpage.sync.tsx (1)
33-33: Use the shared schema version constant here instead of another literal.This write path now has its own copy of the release string, which makes the next format bump easy to miss. Please source
__schema_versionfrom the shared schema/version constant instead of hard-coding it again.Based on learnings: "When writer behavior changes meaningfully (especially for breaking changes), bump
CanvasDocument.schema_versionand keep it in sync with TSgrida.program.document.SCHEMA_VERSION"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/scaffolds/editor/sync/agent-startpage.sync.tsx` at line 33, Replace the hard-coded "__schema_version: \"0.91.0-beta+20260311\"" with the shared schema constant: import and use the canonical SCHEMA_VERSION (or CanvasDocument.schema_version / grida.program.document.SCHEMA_VERSION constant) instead of the literal; update the object at "__schema_version" to reference that imported constant and ensure the import is added to the top of agent-startpage.sync.tsx so the write path stays in sync with the central schema/version value.editor/scaffolds/editor/init.ts (1)
317-320: Avoid duplicating schema-version literals in these loaders.These guards now have to stay manually synced with
grida.program.document.SCHEMA_VERSION, which makes the next schema bump easy to miss in one path. Prefer a shared constant/import for the canvas check, and the same pattern for the form-startpage gate if it has a canonical version source too.Based on learnings: When writer behavior changes meaningfully (especially for breaking changes), bump
CanvasDocument.schema_versionand keep it in sync with TSgrida.program.document.SCHEMA_VERSION.Also applies to: 351-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/scaffolds/editor/init.ts` around lines 317 - 320, Replace the hard-coded schema version literal used in the guard with a shared constant: import and use grida.program.document.SCHEMA_VERSION (or export CanvasDocument.schema_version as a single source) instead of the string literal in the check that references SchemaMayVaryDocumentServerObject; do the same for the other guard (the form/startpage gate) so both guards reference the canonical constant (CanvasDocument.schema_version or grida.program.document.SCHEMA_VERSION) to avoid duplicated literals and keep schema bumps in one place.editor/grida-canvas/editor.ts (1)
3155-3158: Use a shared document-format version constant here.Hard-coding the payload version in the editor makes the next schema bump easy to miss in one path. Please source this from a single schema/io constant so export/load code and fixtures can't drift.
Based on learnings,
Treat changes to format/grida.fbs (FlatBuffers schema) like public API changes—they ripple into Rust and TypeScript codecs and eventually into real user files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/editor.ts` around lines 3155 - 3158, Replace the hard-coded version string used when serializing the payload (the JSON.stringify call that builds variable p with version "0.91.0-beta+20260311") to read a single canonical constant instead; import the shared document-format/version constant (e.g. DOCUMENT_FORMAT_VERSION or SCHEMA_VERSION from your central schema/io module) and use that when constructing the payload object with payloadDocument, so all export/load paths reference the same version constant and cannot drift.crates/grida-canvas/tests/fixtures/l0_boolean_operation.rs (1)
16-19: PreferNodeIdhere instead of rawu64.These fixture graphs are still exercising internal scene structures, so keeping the collections typed as
NodeIdwill stay aligned with the engine boundary and avoid raw-ID drift across helpers.As per coding guidelines,
crates/grida-canvas/**/*.rs: UseNodeId(u64) for internal structs (NodeRecs, SceneGraph, caches) in the rendering engine for high-performance operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/fixtures/l0_boolean_operation.rs` around lines 16 - 19, Replace raw u64 IDs in the fixture with the engine's NodeId type: change nodes from Vec<(u64, Node)> to Vec<(NodeId, Node)>, links from HashMap<u64, Vec<u64>> to HashMap<NodeId, Vec<NodeId>>, roots from Vec<u64> to Vec<NodeId>, and make next_id a NodeId initialized from 1 (e.g., NodeId::from(1) or the crate's NodeId constructor). Update any subsequent uses in this file to treat these values as NodeId rather than raw u64.crates/grida-canvas/tests/fbs_roundtrip.rs (1)
62-70: Consider avoiding unnecessary clone.If
get_childrenreturnsOption<&Vec<NodeId>>, the clone may be unavoidable due to borrow checker constraints. However, if the API could be adjusted to return an iterator or slice, this allocation could be avoided.♻️ Optional: Use iterator directly if API permits
If
get_childrenreturns a reference that can be iterated without cloning:- if let Some(children) = graph.get_children(nid) { - for (i, child) in children.clone().iter().enumerate() { + if let Some(children) = graph.get_children(nid) { + for (i, child) in children.iter().enumerate() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/fbs_roundtrip.rs` around lines 62 - 70, The code clones the children Vec when iterating in walk by calling `children.clone()` which is an unnecessary allocation; change the loop to iterate over the borrowed collection returned by `graph.get_children(nid)` (e.g., iterate over `children.iter().enumerate()` or accept an iterator) and remove the `clone()` so you borrow items for use with `position_map.insert(*child, pos)` and `walk(graph, child, counter, id_map, position_map)`; if the API currently returns an owned Vec, consider changing `get_children` to return a slice (`&[NodeId]`) or an iterator to avoid allocations.crates/grida-canvas/tests/fixtures/mod.rs (1)
130-145: Round-trip validation only checks scene count and names.A codec bug that drops nodes, roots, or ordering will still write a “valid” fixture here. Before persisting, compare at least graph/root/node counts, or full
Sceneequality if that type already supports it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/tests/fixtures/mod.rs` around lines 130 - 145, The tests currently only compare scene count and names after io_grida_fbs::decode_all; extend the round-trip validation in this test to detect dropped nodes/roots or ordering changes by comparing more of the Scene contents: after decoding, for each pair (original, decoded_scene) assert either full Scene equality (if Scene implements PartialEq/Eq) or at minimum compare graph/root/node counts and node IDs/order and any root ordering using the original variable scenes and decoded (decoded_scene) values; update the assertions around decode_all, scenes, and decoded_scene to fail when these structural mismatches occur.
🤖 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/package.json`:
- Around line 7-10: The scripts flatc:clean and flatc:generate use POSIX-only
commands (`rm` and `mv`) and will fail on Windows; update package.json to
replace those shell commands in the "flatc:clean" and "flatc:generate" scripts
(which are invoked by "generate" and "prebuild") with cross-platform
alternatives—e.g., call a small Node.js helper (using
fs.unlinkSync/fs.renameSync) or use cross-platform npm utilities like rimraf and
a move utility (or write a node -e one-liner) to perform the remove and rename
operations; ensure the new commands preserve the same behavior (removing
src/io/generated/grida.rs then running bin/activate-flatc and renaming the
generated file) and that "flatc:generate", "flatc:clean", "generate", and
"prebuild" script names are updated accordingly.
In `@crates/grida-canvas/src/window/application.rs`:
- Around line 298-300: The match currently treats ApplicationCommand::NextScene
and ::PrevScene as no-ops; implement these in UnknownTargetApplication::command
by adding arms that perform scene navigation (e.g., call your scene controller
or methods like next_scene()/prev_scene() on the application or scene manager,
or emit the appropriate internal event) so emscripten dispatch works the same as
native; if you cannot implement navigation here, add a clear API-level comment
on UnknownTargetApplication::command requiring all platforms to intercept and
handle ApplicationCommand::NextScene and ApplicationCommand::PrevScene before
delegating to the shared handler.
In `@crates/grida-canvas/tests/fbs_fixtures.rs`:
- Around line 9-35: The test function write_l0 currently writes files as a side
effect; mark it to be skipped during normal test runs by adding the ignore
attribute to the test (change #[test] on fn write_l0() to #[test] #[ignore]) so
fixtures::write_multi_fixture(&scenes, "L0") only runs when explicitly requested
(or move this fixture generation out of unit tests into an explicit tool if
preferred).
In `@crates/grida-canvas/tests/fixtures/l0_effects.rs`:
- Around line 35-41: The test uses effect_rect with a LayerEffects backdrop_blur
(FeBackdropBlur in effect_rect) but places the panel in an empty gap so there is
nothing to sample; update the fixture so the blurred panel actually has backdrop
content to sample from (for example, render a filled rect behind or change
placement so it overlaps an existing sibling) by modifying the test setup that
constructs effect_rect/LayerEffects/FeBackdropBlur (or add a background draw
before calling effect_rect) so backdrop sampling is exercised.
In `@crates/grida-canvas/tests/fixtures/l0_layout_transform.rs`:
- Around line 3-38: The test is currently creating three sibling nodes via
flat_scene so the 90° rotation on rotated_container never composes with the
other nodes; instead, nest rotated_rect and rotated_line as children of
rotated_container and use build_scene to create a hierarchical scene (i.e.
attach rotated_rect and rotated_line under the ContainerNodeRec for
rotated_container — or set layout_child on rotated_container to include those
nodes — then call build_scene("L0 Layout Transform", <root node or nodes>)
rather than flat_scene).
In `@crates/grida-canvas/tests/fixtures/l0_masks.rs`:
- Around line 15-31: The alpha-mask test is currently ineffective because
mask_img (Node::Rectangle with ImageMaskType::Alpha) exactly matches content_img
and uses a fully opaque white fill, yielding no visual difference; update the
fixture so the mask actually varies alpha or geometry — for example modify
mask_img (the RectangleNodeRec used for mask_img) to use semi-transparent or
gradient fills (non-255 alpha) or change its size/position to only cover part of
content_img so alpha sampling is exercised when rendering with
ImageMaskType::Alpha.
In `@crates/grida-canvas/tests/fixtures/l0_type.rs`:
- Around line 195-201: The field on TextDecorationRec is misspelled: replace all
occurrences of text_decoration_thinkness with text_decoration_thickness; update
the struct definition (TextDecorationRec) and every usage site (tests like the
fixture showing ts.text_decoration, conversion/mapping functions such as those
in io_grida and types-related code) to use the corrected identifier, ensuring
any serialization/deserialization, builders, and pattern matches are updated to
the new name.
In `@crates/grida-dev/src/main.rs`:
- Around line 285-293: The new implementations of load_scene_from_source and
load_scenes_from_source always call grida_file::decode/_all which only accepts
FlatBuffers/ZIP and thus breaks loading of JSON scenes; modify both functions to
detect JSON inputs (e.g., inspect the source string for a ".json" suffix or peek
at the bytes from read_source_bytes) and route JSON data to the JSON
deserializer (the existing JSON loader used by load_master_scene_from_path or a
grida_file::decode_json/decode_all_json equivalent) while keeping FlatBuffer/ZIP
handling via grida_file::decode and grida_file::decode_all for non-JSON payloads
so legacy .json scenes still load correctly.
- Around line 128-134: The code unconditionally unwraps the first scene after
load_scenes_from_source, causing a panic when decode_all returns an empty Vec;
instead, check scenes.is_empty() after load_scenes_from_source(source).await?
and return an appropriate Err (e.g., via anyhow! or the crate's error type) with
a helpful message like "no scenes decoded from source"; if not empty, branch to
run_demo_window_multi(scenes) when len>1 or run_demo_window(first_scene) when
len==1. Update the logic around load_scenes_from_source, run_demo_window_multi,
and run_demo_window to avoid unwrap and propagate a normal CLI error when zero
scenes are found.
In `@crates/grida-dev/src/platform/native_demo.rs`:
- Around line 18-24: The scene-switch code calls renderer.load_scene(scene) but
never triggers image preloading; after each renderer.load_scene(...) (notably
inside the ApplicationCommand::NextScene/PrevScene handler and any other
locations that switch scenes), call renderer.load_scene_images() (or otherwise
enqueue the load_scene_images task on the renderer) before
fit_camera_to_scene(renderer) and renderer.queue_unstable(); update
run_demo_window_multi/run_demo_window_core_multi usage accordingly so
non-initial scenes also preload their image payloads.
In `@editor/grida-canvas-hosted/playground/playground.tsx`:
- Around line 503-518: The current catch calls opfs?.quarantine() for any load
error; change it to only quarantine when the error is an explicit
decode/schema-corruption error (e.g., a specific error class or a distinctive
error.message like "decode"/"schema"/"unreadable") and otherwise rethrow or
allow the error to propagate so transient or runtime errors from reset() /
loadImages() aren't archived; update the conditional around the quarantine call
(the block that calls opfs?.quarantine()) to check the error type/message before
invoking quarantine and only log+quarantine on confirmed decode/schema failures,
leaving other errors unmodified.
In `@fixtures/test-grida/README.md`:
- Around line 28-33: Update the Rust writer to stop emitting the hardcoded
"0.0.0" and instead use the actual schema version constant (replace the two
occurrences of schema_version = "0.0.0" in io_grida_fbs.rs with a SCHEMA_VERSION
constant or build-injected value matching the TS constant
"0.91.0-beta+20260311"), and add reader-side validation in the FlatBuffers
decoder (the CanvasDocument deserializer/reader function in io_grida_fbs.rs) to
check CanvasDocument.schema_version and return a clear error (or invoke an
explicit migration path) when the version is incompatible; ensure the Rust
writer and reader use the same single source of truth for the schema version
(shared constant, build-time injection, or CI check) so the emitted
schema_version and the reader validation remain synchronized.
In `@format/grida.fbs`:
- Around line 1530-1538: The FlatBuffers schema change altered the wire shape
and reused field ids for CanvasDocument (the nodes:[NodeSlot] field moved to
id:1 and scenes to id:2), which breaks on-disk compatibility; revert to
preserving the original field ids for nodes and scenes (do not renumber or reuse
ids), add new fields with fresh unused ids for the new NodeSlot storage (e.g.,
nodes_v2 or nodes_slot) instead of replacing nodes, and implement a versioned
migration/fallback in the decoder paths that checks CanvasDocument version or
presence of the new field names and gracefully reads either the old nodes union
layout or the new NodeSlot vector (update code paths that reference nodes,
scenes, and any serializers/deserializers to handle both shapes).
In `@packages/grida-canvas-io/format.ts`:
- Around line 5452-5459: Check the document schema version before iterating
document.nodes() and refuse to decode NodeSlot for pre-0.91 files: use the
existing schemaVersion value (read earlier) and if it's less than 0.91
throw/return a clear error stating the CanvasDocument field-id reindex is
incompatible, or implement an explicit migration path, before calling
document.nodes(), unionToNode, or treating entries as NodeSlot; this prevents
misdecoding old 0.90.x layout when unionToNode/NodeSlot is invoked.
---
Minor comments:
In `@crates/grida-canvas/Cargo.toml`:
- Line 25: The flatbuffers runtime version in Cargo.toml is out of sync with the
repo-pinned flatc generator; update the flatbuffers dependency version string
from "25.9.23" to "25.12.19" so the Rust runtime matches the generator used by
bin/activate-flatc (and avoid generated code/API mismatches when regenerating
bindings from format/grida.fbs).
In `@crates/grida-canvas/README.md`:
- Around line 113-116: The README's cargo doc example incorrectly documents the
skia-safe package; change the command to generate docs for this crate by
replacing "skia-safe" with this crate's package name "cg" (as defined in
Cargo.toml), i.e., update the snippet in README.md so it runs "cargo doc
--package cg --open --no-deps" to build this crate's docs instead of
skia-safe's.
In `@crates/grida-canvas/tests/fixtures/l0_layout_position.rs`:
- Around line 33-37: The test fixture uses the same rect(x, y, w, h, paint)
helper for both c1 and c2 so it doesn't exercise inset-based positioning; change
the c2 construction to create an inset-positioned child explicitly (instead of
calling rect) by constructing the node using the inset/top/left API or layout
constructor used elsewhere in tests (the symbol to change is c2 and stop using
the rect helper), ensuring the inset fields (top=50, left=150) are set on the
layout node so the fixture actually validates inset vs Cartesian positioning.
In `@crates/grida-canvas/tests/fixtures/mod.rs`:
- Around line 97-125: write_multi_fixture currently ignores the caller-provided
scene key in scenes: &[(&str, Scene)] and instead generates scene IDs like
"scene1", causing IDs to change when order changes; update write_multi_fixture
to use the first tuple element (the &str key) as the scene_id and as the prefix
passed to build_maps_prefixed (instead of format!("scene{}", i+1) and
format!("s{}_", i)), so id_map/position_map and entries_data are derived from
the caller-provided key; ensure you propagate that key into entries_data and the
later entries vector (keeping build_maps_prefixed, entries_data, and entries
usage intact).
In `@crates/grida-canvas/tests/fixtures/README.md`:
- Around line 18-356: The fenced code blocks in this README (e.g., the blocks
that start with scene "L0 Shapes", scene "L0 Shape Arc", scene "L0 Shape
Polygon", etc.) lack language tags and trigger markdownlint MD040; update every
triple-backtick fence in this file to include an explicit language token (for
example use "text" for plain scenes or "sh" for shell-like snippets) so each
fenced example becomes ```text (or ```sh) … ```; ensure you update every
occurrence throughout the file including all scene/example sections (L0-vector,
L0-paints, L0-effects, L0-type, etc.) to satisfy the linter.
In `@crates/grida-dev/src/grida_file.rs`:
- Around line 19-25: The ZIP detection in fn detect(bytes: &[u8]) -> Format
currently only checks bytes[2] and misclassifies inputs like "PK\x03?" as ZIP;
update the condition in detect to match the full four-byte ZIP magic by checking
both bytes[2] and bytes[3], i.e. treat as ZIP only when (bytes[2] == 0x03 &&
bytes[3] == 0x04) or (bytes[2] == 0x05 && bytes[3] == 0x06), so non-ZIP payloads
fall through to Format::Unknown.
In `@fixtures/test-grida/README.md`:
- Line 18: The README mentions a non-existent fixture "basic.grida"; remove that
bullet (the line referencing **`basic.grida`**) and replace it with a short note
pointing to the actual in-memory test generation used by the tests (referencing
the test command `cargo test --package cg --test fbs_roundtrip`) or explicitly
reference the real files (`L0.grida`, `d1-20251209.grida1.zip`) if you prefer a
file-based list; update the text so it no longer claims a file-based fixture
exists and instead explains that test data is generated in-memory by the
fbs_roundtrip test.
In `@format/README.md`:
- Around line 57-60: The README currently lists in-repo generated outputs for
TypeScript (packages/grida-format/) and Rust
(crates/grida-canvas/src/io/generated/) but doesn't state whether the Rust
FlatBuffers bindings are committed; update the In-repo generated code +
automation section to explicitly state per target whether generated artifacts
are committed or ephemeral (e.g., "TypeScript bindings are committed and
regenerated by prebuild" vs "Rust FlatBuffers bindings under
crates/grida-canvas/src/io/generated/ are NOT committed and are generated during
CI/build"), and add guidance for contributors about expected diffs and the
correct workflow (how to regenerate locally with pnpm build and when to commit
generated files) so reviewers know if changes under
crates/grida-canvas/src/io/generated/ are intentional.
---
Nitpick comments:
In `@crates/grida-canvas/tests/fbs_roundtrip.rs`:
- Around line 62-70: The code clones the children Vec when iterating in walk by
calling `children.clone()` which is an unnecessary allocation; change the loop
to iterate over the borrowed collection returned by `graph.get_children(nid)`
(e.g., iterate over `children.iter().enumerate()` or accept an iterator) and
remove the `clone()` so you borrow items for use with
`position_map.insert(*child, pos)` and `walk(graph, child, counter, id_map,
position_map)`; if the API currently returns an owned Vec, consider changing
`get_children` to return a slice (`&[NodeId]`) or an iterator to avoid
allocations.
In `@crates/grida-canvas/tests/fixtures/l0_boolean_operation.rs`:
- Around line 16-19: Replace raw u64 IDs in the fixture with the engine's NodeId
type: change nodes from Vec<(u64, Node)> to Vec<(NodeId, Node)>, links from
HashMap<u64, Vec<u64>> to HashMap<NodeId, Vec<NodeId>>, roots from Vec<u64> to
Vec<NodeId>, and make next_id a NodeId initialized from 1 (e.g., NodeId::from(1)
or the crate's NodeId constructor). Update any subsequent uses in this file to
treat these values as NodeId rather than raw u64.
In `@crates/grida-canvas/tests/fixtures/mod.rs`:
- Around line 130-145: The tests currently only compare scene count and names
after io_grida_fbs::decode_all; extend the round-trip validation in this test to
detect dropped nodes/roots or ordering changes by comparing more of the Scene
contents: after decoding, for each pair (original, decoded_scene) assert either
full Scene equality (if Scene implements PartialEq/Eq) or at minimum compare
graph/root/node counts and node IDs/order and any root ordering using the
original variable scenes and decoded (decoded_scene) values; update the
assertions around decode_all, scenes, and decoded_scene to fail when these
structural mismatches occur.
In `@editor/grida-canvas/editor.ts`:
- Around line 3155-3158: Replace the hard-coded version string used when
serializing the payload (the JSON.stringify call that builds variable p with
version "0.91.0-beta+20260311") to read a single canonical constant instead;
import the shared document-format/version constant (e.g. DOCUMENT_FORMAT_VERSION
or SCHEMA_VERSION from your central schema/io module) and use that when
constructing the payload object with payloadDocument, so all export/load paths
reference the same version constant and cannot drift.
In `@editor/scaffolds/editor/editor.tsx`:
- Around line 160-164: Replace the hardcoded schema string "__schema_version:
\"0.91.0-beta+20260311\"" with the shared constant used elsewhere
(grida.program.document.SCHEMA_VERSION or imported SCHEMA_VERSION) so the object
built in the data field uses that constant when document is present; update the
object spread where CanvasDocumentSnapshotSchema is satisfied to reference
grida.program.document.SCHEMA_VERSION instead of the literal to prevent version
drift and keep consistent with editor/services/new/index.ts and
editor/scaffolds/editor/reducer.ts.
In `@editor/scaffolds/editor/init.ts`:
- Around line 317-320: Replace the hard-coded schema version literal used in the
guard with a shared constant: import and use
grida.program.document.SCHEMA_VERSION (or export CanvasDocument.schema_version
as a single source) instead of the string literal in the check that references
SchemaMayVaryDocumentServerObject; do the same for the other guard (the
form/startpage gate) so both guards reference the canonical constant
(CanvasDocument.schema_version or grida.program.document.SCHEMA_VERSION) to
avoid duplicated literals and keep schema bumps in one place.
In `@editor/scaffolds/editor/sync/agent-startpage.sync.tsx`:
- Line 33: Replace the hard-coded "__schema_version: \"0.91.0-beta+20260311\""
with the shared schema constant: import and use the canonical SCHEMA_VERSION (or
CanvasDocument.schema_version / grida.program.document.SCHEMA_VERSION constant)
instead of the literal; update the object at "__schema_version" to reference
that imported constant and ensure the import is added to the top of
agent-startpage.sync.tsx so the write path stays in sync with the central
schema/version value.
In `@packages/grida-format/src/__tests__/index.test.ts`:
- Line 18: Test hardcodes the schema version literal; import and use the shared
canonical schema version constant (e.g. grida.program.document.SCHEMA_VERSION or
CanvasDocument.schema_version) instead of the string literal so the
builder.createString call and the assertion both reference that single constant;
update the test at the builder.createString("0.91.0-beta+20260311") location and
the corresponding expect/assert to use the imported SCHEMA_VERSION symbol.
🪄 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: c1b8c7e3-cbd8-4929-872d-5639b6e78f90
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockcrates/grida-canvas/src/io/generated/.gitattributesis excluded by!**/generated/**crates/grida-canvas/src/io/generated/grida.rsis excluded by!**/generated/**crates/grida-canvas/src/io/generated/mod.rsis excluded by!**/generated/**
📒 Files selected for processing (70)
.github/workflows/check-generated-fbs.ymlbin/activate-flatccrates/grida-canvas-wasm/example/demo.grida1crates/grida-canvas-wasm/example/gradient.grida1crates/grida-canvas-wasm/example/rectangle.grida1crates/grida-canvas-wasm/lib/__test__/environment-node-raster-export.test.tscrates/grida-canvas/Cargo.tomlcrates/grida-canvas/README.mdcrates/grida-canvas/package.jsoncrates/grida-canvas/src/io/io_grida.rscrates/grida-canvas/src/io/io_grida_fbs.rscrates/grida-canvas/src/io/mod.rscrates/grida-canvas/src/node/scene_graph.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/src/window/command.rscrates/grida-canvas/tests/fbs_fixtures.rscrates/grida-canvas/tests/fbs_roundtrip.rscrates/grida-canvas/tests/fixtures/README.mdcrates/grida-canvas/tests/fixtures/l0_boolean_operation.rscrates/grida-canvas/tests/fixtures/l0_container.rscrates/grida-canvas/tests/fixtures/l0_effects.rscrates/grida-canvas/tests/fixtures/l0_effects_glass.rscrates/grida-canvas/tests/fixtures/l0_group.rscrates/grida-canvas/tests/fixtures/l0_image.rscrates/grida-canvas/tests/fixtures/l0_image_filters.rscrates/grida-canvas/tests/fixtures/l0_layout_flex.rscrates/grida-canvas/tests/fixtures/l0_layout_position.rscrates/grida-canvas/tests/fixtures/l0_layout_transform.rscrates/grida-canvas/tests/fixtures/l0_masks.rscrates/grida-canvas/tests/fixtures/l0_paints.rscrates/grida-canvas/tests/fixtures/l0_paints_stack.rscrates/grida-canvas/tests/fixtures/l0_shape_arc.rscrates/grida-canvas/tests/fixtures/l0_shape_polygon.rscrates/grida-canvas/tests/fixtures/l0_shapes.rscrates/grida-canvas/tests/fixtures/l0_strokes.rscrates/grida-canvas/tests/fixtures/l0_strokes_rect.rscrates/grida-canvas/tests/fixtures/l0_type.rscrates/grida-canvas/tests/fixtures/l0_type_features.rscrates/grida-canvas/tests/fixtures/l0_type_fvar.rscrates/grida-canvas/tests/fixtures/l0_vector.rscrates/grida-canvas/tests/fixtures/mod.rscrates/grida-dev/Cargo.tomlcrates/grida-dev/src/grida_file.rscrates/grida-dev/src/main.rscrates/grida-dev/src/platform/native_application.rscrates/grida-dev/src/platform/native_demo.rscrates/math2/src/transform.rseditor/grida-canvas-hosted/playground/playground.tsxeditor/grida-canvas/editor.tseditor/grida-canvas/query/index.tseditor/public/examples/canvas/blank.grida1editor/public/examples/canvas/component-01.grida1editor/public/examples/canvas/globals-01.grida1editor/public/examples/canvas/helloworld.grida1editor/public/examples/canvas/hero-main-demo.grida1editor/public/examples/canvas/layout-01.grida1editor/public/examples/canvas/poster-happy-new-year-2026.grida1editor/scaffolds/editor/editor.tsxeditor/scaffolds/editor/init.tseditor/scaffolds/editor/sync/agent-startpage.sync.tsxfixtures/test-grida/L0.gridafixtures/test-grida/README.mdformat/CHANGELOG.mdformat/README.mdformat/TODO.mdformat/grida.fbspackages/grida-canvas-io/format.tspackages/grida-canvas-io/index.tspackages/grida-canvas-schema/grida.tspackages/grida-format/src/__tests__/index.test.ts
Comment on lines
+7
to
+10
| "flatc:clean": "rm -f src/io/generated/grida.rs", | ||
| "flatc:generate": "pnpm flatc:clean && python3 ../../bin/activate-flatc -- --rust -o src/io/generated ../../format/grida.fbs && mv src/io/generated/grida_generated.rs src/io/generated/grida.rs", | ||
| "generate": "pnpm flatc:generate", | ||
| "prebuild": "pnpm flatc:generate", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "package.json" -path "*/crates/grida-canvas/*" 2>/dev/null | head -5Repository: gridaco/grida
Length of output: 39
🏁 Script executed:
git ls-files | grep -E "(crates/grida-canvas|bin/activate-flatc|format/README)" | head -20Repository: gridaco/grida
Length of output: 976
🏁 Script executed:
cat -n crates/grida-canvas/package.jsonRepository: gridaco/grida
Length of output: 659
🏁 Script executed:
cat -n bin/activate-flatcRepository: gridaco/grida
Length of output: 18320
🏁 Script executed:
cat -n format/README.mdRepository: gridaco/grida
Length of output: 2238
🏁 Script executed:
# Check for Windows/cross-platform support claims in root or docs
find . -maxdepth 3 -type f \( -name "README.md" -o -name "CONTRIBUTING.md" -o -name ".github" \) 2>/dev/null | head -10Repository: gridaco/grida
Length of output: 290
🏁 Script executed:
# Search for other build scripts in package.json files to see cross-platform patterns
rg -l "package\.json" --type json | head -10 | xargs -I {} grep -l "flatc\|prebuild" {} 2>/dev/nullRepository: gridaco/grida
Length of output: 39
These npm scripts use POSIX-only commands that will break on Windows.
The flatc:clean and flatc:generate scripts use rm and mv, which won't work on Windows. Since bin/activate-flatc is explicitly designed to support Windows (as stated in its docstring: "macOS/Windows are supported for developer convenience"), and these scripts are wired into prebuild (which runs during cargo build), developers on Windows will encounter failures when building locally.
Replace rm and mv with cross-platform alternatives—either via Node.js helper methods (fs.unlinkSync, fs.renameSync), a lightweight npm utility like rimraf, or Node.js subshell invocation instead of shell syntax.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/package.json` around lines 7 - 10, The scripts
flatc:clean and flatc:generate use POSIX-only commands (`rm` and `mv`) and will
fail on Windows; update package.json to replace those shell commands in the
"flatc:clean" and "flatc:generate" scripts (which are invoked by "generate" and
"prebuild") with cross-platform alternatives—e.g., call a small Node.js helper
(using fs.unlinkSync/fs.renameSync) or use cross-platform npm utilities like
rimraf and a move utility (or write a node -e one-liner) to perform the remove
and rename operations; ensure the new commands preserve the same behavior
(removing src/io/generated/grida.rs then running bin/activate-flatc and renaming
the generated file) and that "flatc:generate", "flatc:clean", "generate", and
"prebuild" script names are updated accordingly.
Comment on lines
+298
to
+300
| ApplicationCommand::None | ||
| | ApplicationCommand::NextScene | ||
| | ApplicationCommand::PrevScene => {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Scene navigation command references =="
rg -n -C3 'ApplicationCommand::(NextScene|PrevScene)|\b(NextScene|PrevScene)\b' --type rust
echo
echo "== Shared command entry points =="
rg -n -C3 'fn command\(&mut self, cmd: ApplicationCommand\)' --type rustRepository: gridaco/grida
Length of output: 4946
Implement scene-navigation commands in the shared handler.
ApplicationCommand::NextScene and ApplicationCommand::PrevScene are matched here but not implemented—they fall through as silent no-ops. The native platform avoids this by intercepting these commands before delegating to the shared handler, but the emscripten platform (application_emscripten.rs:195–197) delegates directly, meaning scene navigation will not work on that platform.
Either implement the commands in UnknownTargetApplication::command or document that all platforms must intercept and handle these commands before calling the shared handler.
🤖 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 298 - 300, The
match currently treats ApplicationCommand::NextScene and ::PrevScene as no-ops;
implement these in UnknownTargetApplication::command by adding arms that perform
scene navigation (e.g., call your scene controller or methods like
next_scene()/prev_scene() on the application or scene manager, or emit the
appropriate internal event) so emscripten dispatch works the same as native; if
you cannot implement navigation here, add a clear API-level comment on
UnknownTargetApplication::command requiring all platforms to intercept and
handle ApplicationCommand::NextScene and ApplicationCommand::PrevScene before
delegating to the shared handler.
Comment on lines
+35
to
+41
| let backdrop = effect_rect(gap, LayerEffects { | ||
| backdrop_blur: Some(FeBackdropBlur { | ||
| active: true, | ||
| blur: FeBlur::Gaussian(FeGaussianBlur { radius: 8.0 }), | ||
| }), | ||
| ..LayerEffects::default() | ||
| }); |
There was a problem hiding this comment.
Backdrop blur has no backdrop to sample.
This panel is placed in an empty gap between siblings, so it won’t exercise backdrop sampling at all. The fixture will render much closer to an unblurred rect unless something is painted behind that area or overlaps it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/tests/fixtures/l0_effects.rs` around lines 35 - 41, The
test uses effect_rect with a LayerEffects backdrop_blur (FeBackdropBlur in
effect_rect) but places the panel in an empty gap so there is nothing to sample;
update the fixture so the blurred panel actually has backdrop content to sample
from (for example, render a filled rect behind or change placement so it
overlaps an existing sibling) by modifying the test setup that constructs
effect_rect/LayerEffects/FeBackdropBlur (or add a background draw before calling
effect_rect) so backdrop sampling is exercised.
Comment on lines
+3
to
+38
| pub fn build() -> Scene { | ||
| // Container rotated 90°, Cartesian position | ||
| let rotated_container = Node::Container(ContainerNodeRec { | ||
| active: true, | ||
| opacity: 1.0, | ||
| blend_mode: LayerBlendMode::PassThrough, | ||
| mask: None, | ||
| rotation: 90.0, | ||
| position: LayoutPositioningBasis::Cartesian(CGPoint::new(300.0, 50.0)), | ||
| layout_container: LayoutContainerStyle::default(), | ||
| layout_dimensions: LayoutDimensionStyle { | ||
| layout_target_width: Some(200.0), | ||
| layout_target_height: Some(150.0), | ||
| layout_min_width: None, layout_max_width: None, | ||
| layout_min_height: None, layout_max_height: None, | ||
| layout_target_aspect_ratio: None, | ||
| }, | ||
| layout_child: None, | ||
| corner_radius: RectangularCornerRadius::default(), | ||
| corner_smoothing: CornerSmoothing(0.0), | ||
| fills: Paints::new(vec![solid(59, 100, 220, 255)]), | ||
| strokes: Paints::new(vec![]), | ||
| stroke_style: StrokeStyle::default(), | ||
| stroke_width: StrokeWidth::None, | ||
| effects: LayerEffects::default(), | ||
| clip: false, | ||
| }); | ||
|
|
||
| // Rectangle rotated 45° | ||
| let rotated_rect = rect_rotated(50.0, 50.0, 120.0, 80.0, 45.0, solid(220, 59, 59, 255)); | ||
|
|
||
| // Line rotated 45° | ||
| let rotated_line = line(400.0, 200.0, 200.0, 45.0, 2.0); | ||
|
|
||
| flat_scene("L0 Layout Transform", vec![rotated_container, rotated_rect, rotated_line]) | ||
| } |
There was a problem hiding this comment.
This fixture never exercises parent-child transform composition.
flat_scene makes rotated_container, rotated_rect, and rotated_line siblings, so the 90° container rotation never affects the other two nodes. If this is meant to cover layout-transform nesting, wire the rect/line under the container with build_scene instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/tests/fixtures/l0_layout_transform.rs` around lines 3 -
38, The test is currently creating three sibling nodes via flat_scene so the 90°
rotation on rotated_container never composes with the other nodes; instead, nest
rotated_rect and rotated_line as children of rotated_container and use
build_scene to create a hierarchical scene (i.e. attach rotated_rect and
rotated_line under the ContainerNodeRec for rotated_container — or set
layout_child on rotated_container to include those nodes — then call
build_scene("L0 Layout Transform", <root node or nodes>) rather than
flat_scene).
Comment on lines
+18
to
+24
| pub async fn run_demo_window_multi(scenes: Vec<Scene>) { | ||
| let first = scenes | ||
| .first() | ||
| .cloned() | ||
| .expect("run_demo_window_multi requires at least one scene"); | ||
| run_demo_window_core_multi(first, scenes, |_, _, _, _| {}, None).await; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'NextScene|PrevScene|LoadScene|load_scene_images|scene_index|app\.scenes' crates/grida-dev/src
fd 'native_application.rs$' crates/grida-dev --exec sed -n '1,260p' {}Repository: gridaco/grida
Length of output: 19665
Scene switches must also preload image resources.
When scenes are switched via PageUp/PageDown (handled in native_application.rs lines 232–260), the code calls renderer.load_scene(scene) but never enqueues load_scene_images(). Only the initial scene receives image preloading (in native_demo.rs line 99). Non-initial scenes will render without their image payloads.
Scene switch handler (native_application.rs)
ApplicationCommand::NextScene | ApplicationCommand::PrevScene => {
if !self.scenes.is_empty() {
let new_index = match &cmd {
ApplicationCommand::NextScene => {
(self.scene_index + 1) % self.scenes.len()
}
ApplicationCommand::PrevScene => {
(self.scene_index + self.scenes.len() - 1) % self.scenes.len()
}
_ => unreachable!(),
};
if new_index != self.scene_index {
self.scene_index = new_index;
let scene = self.scenes[new_index].clone();
let title = format!(
"[{}/{}] {}",
new_index + 1,
self.scenes.len(),
scene.name,
);
let renderer = self.app.renderer_mut();
renderer.load_scene(scene); // ← Missing load_scene_images call here
fit_camera_to_scene(renderer);
renderer.queue_unstable();
}
}
}
Also applies to: 50–53, 113–118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-dev/src/platform/native_demo.rs` around lines 18 - 24, The
scene-switch code calls renderer.load_scene(scene) but never triggers image
preloading; after each renderer.load_scene(...) (notably inside the
ApplicationCommand::NextScene/PrevScene handler and any other locations that
switch scenes), call renderer.load_scene_images() (or otherwise enqueue the
load_scene_images task on the renderer) before fit_camera_to_scene(renderer) and
renderer.queue_unstable(); update
run_demo_window_multi/run_demo_window_core_multi usage accordingly so
non-initial scenes also preload their image payloads.
Comment on lines
+503
to
+518
| // Decode failure (likely a schema change) or other corruption. | ||
| // Quarantine the stale files so the user's data is preserved | ||
| // for possible future migration, then fall through to the | ||
| // default document. | ||
| console.warn( | ||
| "OPFS document unreadable (possible schema change), quarantining:", | ||
| error | ||
| ); | ||
| try { | ||
| await opfs?.quarantine(); | ||
| } catch (quarantineError) { | ||
| console.error( | ||
| "Failed to quarantine stale OPFS data:", | ||
| quarantineError | ||
| ); | ||
| } |
There was a problem hiding this comment.
Don’t quarantine on every non-not found load failure.
This catch currently archives the active OPFS document for any error thrown in the whole load path, including transient OPFS read issues or bugs in reset() / loadImages(). That can sideline perfectly valid user data even when the bytes themselves are fine. Please narrow quarantine to explicit decode/schema-corruption failures and let unrelated runtime errors fall through without moving the stored files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/grida-canvas-hosted/playground/playground.tsx` around lines 503 - 518,
The current catch calls opfs?.quarantine() for any load error; change it to only
quarantine when the error is an explicit decode/schema-corruption error (e.g., a
specific error class or a distinctive error.message like
"decode"/"schema"/"unreadable") and otherwise rethrow or allow the error to
propagate so transient or runtime errors from reset() / loadImages() aren't
archived; update the conditional around the quarantine call (the block that
calls opfs?.quarantine()) to check the error type/message before invoking
quarantine and only log+quarantine on confirmed decode/schema failures, leaving
other errors unmodified.
Comment on lines
+28
to
+33
| - **0.91.0-beta+20260311** ⚠️ Breaking FBS schema change: | ||
| - Added `NodeSlot` wrapper table in `grida.fbs` to work around a `flatc --rust` limitation (Rust codegen does not support `[Node]` — vectors of unions). | ||
| - `CanvasDocument.nodes` field type changed: `[Node]` → `[NodeSlot]`. | ||
| - `CanvasDocument` field IDs reindexed as a result: `nodes` moved to `id:1`, `scenes` moved to `id:2`. | ||
| - Added Rust FlatBuffers decoder: `crates/grida-canvas/src/io/io_grida_fbs.rs`. | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify schema_version bump and coordinated model updates
echo "=== Checking schema_version in format/grida.fbs ==="
rg -n 'schema_version' format/grida.fbs
echo -e "\n=== Checking CanvasDocument field IDs in format/grida.fbs ==="
rg -A 20 'table CanvasDocument' format/grida.fbs | rg -P '(nodes|scenes).*id:'
echo -e "\n=== Checking Rust runtime model updates ==="
rg -n 'NodeSlot' crates/grida-canvas/src/node/schema.rs
echo -e "\n=== Checking TS document model updates ==="
rg -n 'NodeSlot' packages/grida-canvas-schema/grida.ts
echo -e "\n=== Checking for schema version constants ==="
rg -n 'SCHEMA_VERSION' packages/grida-canvas-schema/ crates/grida-canvas/
echo -e "\n=== Checking for version validation/migration logic ==="
rg -nP 'schema_version|SchemaVersion' crates/grida-canvas/src/io/Repository: gridaco/grida
Length of output: 2734
🏁 Script executed:
#!/bin/bash
echo "=== Searching for NodeSlot definition ==="
rg -n 'NodeSlot' --type rust --type typescript
echo -e "\n=== Checking io_grida_fbs.rs context around schema_version ==="
sed -n '1630,1645p' crates/grida-canvas/src/io/io_grida_fbs.rs
echo "---"
sed -n '1698,1713p' crates/grida-canvas/src/io/io_grida_fbs.rs
echo -e "\n=== Checking if there's version validation in readers ==="
rg -B5 -A5 'schema_version' crates/grida-canvas/src/io/io_grida_fbs.rs | head -50
echo -e "\n=== Checking format/grida.fbs for CanvasDocument full definition ==="
rg -B2 -A30 'table CanvasDocument' format/grida.fbsRepository: gridaco/grida
Length of output: 3128
🏁 Script executed:
#!/bin/bash
echo "=== Searching for NodeSlot table definition in format/grida.fbs ==="
rg -B2 -A10 'table NodeSlot' format/grida.fbs
echo -e "\n=== Checking entire CanvasDocument section ==="
sed -n '1520,1560p' format/grida.fbs
echo -e "\n=== Checking for reader version validation in io_grida_fbs.rs ==="
rg -B10 -A10 'read.*CanvasDocument|parse.*document|from_fbs.*CanvasDocument' crates/grida-canvas/src/io/io_grida_fbs.rs | head -80
echo -e "\n=== Checking for any TODO or FIXME related to schema_version ==="
rg -n 'TODO|FIXME' crates/grida-canvas/src/io/io_grida_fbs.rs | grep -i 'version\|0\.0\.0\|schema'Repository: gridaco/grida
Length of output: 1603
🏁 Script executed:
#!/bin/bash
echo "=== Searching for reader/parsing functions in io_grida_fbs.rs ==="
rg -n 'pub fn|pub async fn' crates/grida-canvas/src/io/io_grida_fbs.rs | head -30
echo -e "\n=== Checking for version comparison/validation anywhere in codebase ==="
rg -n 'schema_version.*==' crates/grida-canvas/src/io/
echo -e "\n=== Checking for actual schema_version value hardcodes ==="
rg -n '"0\.' crates/grida-canvas/src/io/io_grida_fbs.rs
echo -e "\n=== Checking TS model for version validation ==="
rg -B5 -A10 'SCHEMA_VERSION' packages/grida-canvas-schema/grida.ts | head -40
echo -e "\n=== Checking if 0.0.0 appears elsewhere as intentional ==="
rg -n '0\.0\.0' crates/grida-canvas/src/Repository: gridaco/grida
Length of output: 1351
Add version validation and fix Rust writer to emit correct schema version.
The breaking FlatBuffers schema change (field ID reindexing for nodes and scenes) requires explicit version validation in the reader, but the code lacks it. Additionally, the Rust writer hardcodes schema_version: "0.0.0" at crates/grida-canvas/src/io/io_grida_fbs.rs lines 1635 and 1703, which does not sync with the TypeScript version constant "0.91.0-beta+20260311".
Without reader-side schema_version validation, old files with reindexed field IDs will deserialize silently with incorrect data. Per the FlatBuffers breaking-change pattern: either migrate or fail fast—do not silently misinterpret.
Issues to fix:
- Update Rust writer to emit the actual schema version (not hardcoded "0.0.0")
- Add reader validation that checks
CanvasDocument.schema_versionand either rejects old versions or implements explicit migration - Ensure version string stays synchronized across Rust and TypeScript codebases
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fixtures/test-grida/README.md` around lines 28 - 33, Update the Rust writer
to stop emitting the hardcoded "0.0.0" and instead use the actual schema version
constant (replace the two occurrences of schema_version = "0.0.0" in
io_grida_fbs.rs with a SCHEMA_VERSION constant or build-injected value matching
the TS constant "0.91.0-beta+20260311"), and add reader-side validation in the
FlatBuffers decoder (the CanvasDocument deserializer/reader function in
io_grida_fbs.rs) to check CanvasDocument.schema_version and return a clear error
(or invoke an explicit migration path) when the version is incompatible; ensure
the Rust writer and reader use the same single source of truth for the schema
version (shared constant, build-time injection, or CI check) so the emitted
schema_version and the reader validation remain synchronized.
Comment on lines
1530
to
+1538
| /// Flat node repository. | ||
| nodes:[Node] (id: 2); | ||
| /// | ||
| /// Stored as `[NodeSlot]` (vector of wrapper tables) rather than `[Node]` | ||
| /// (vector of unions) to maintain compatibility with Rust FlatBuffers codegen, | ||
| /// which does not support vectors of unions directly. | ||
| nodes:[NodeSlot] (id: 1); | ||
|
|
||
| /// Scene node ids (scene nodes are also stored in `nodes`). | ||
| scenes:[NodeIdentifier] (id: 3); | ||
| scenes:[NodeIdentifier] (id: 2); |
There was a problem hiding this comment.
This is a breaking on-disk schema change for CanvasDocument.
nodes now has a different wire shape, and per the PR summary this also renumbers the field ids (nodes 2→1, scenes 3→2). Older .grida files will no longer decode safely under this schema, and older readers will not understand newly written files. Please keep the old field ids immutable and introduce an explicit versioned migration/fallback for the new node storage instead of reusing the slots.
Based on learnings, "Treat changes to format/grida.fbs (FlatBuffers schema) like public API changes—they ripple into Rust and TypeScript codecs and eventually into real user files." As per coding guidelines, "In format/grida.fbs, never change or reuse field ids—treat them as immutable; if you remove a field, leave the id unused."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@format/grida.fbs` around lines 1530 - 1538, The FlatBuffers schema change
altered the wire shape and reused field ids for CanvasDocument (the
nodes:[NodeSlot] field moved to id:1 and scenes to id:2), which breaks on-disk
compatibility; revert to preserving the original field ids for nodes and scenes
(do not renumber or reuse ids), add new fields with fresh unused ids for the new
NodeSlot storage (e.g., nodes_v2 or nodes_slot) instead of replacing nodes, and
implement a versioned migration/fallback in the decoder paths that checks
CanvasDocument version or presence of the new field names and gracefully reads
either the old nodes union layout or the new NodeSlot vector (update code paths
that reference nodes, scenes, and any serializers/deserializers to handle both
shapes).
Comment on lines
+5452
to
+5459
| const slot = document.nodes(i); | ||
| if (!slot) continue; | ||
|
|
||
| const nodeType = slot.nodeType(); | ||
| if (nodeType === fbs.Node.NONE) continue; | ||
|
|
||
| // Unwrap NodeSlot to get the typed node table | ||
| const typedNode = unionToNode(nodeType, (obj: any) => slot.node(obj)); |
There was a problem hiding this comment.
Fail fast on pre-0.91 documents before decoding NodeSlots.
Line 5439 already reads schemaVersion, but Line 5452 immediately starts decoding document.nodes() as NodeSlot. After the CanvasDocument field-id reindex in this PR, a 0.90.x file will feed the old layout into this path and get misdecoded instead of rejected cleanly. Gate known-incompatible versions here, or add an explicit migration path, before entering the node loop.
💡 Suggested fix
const schemaVersion =
document.schemaVersion() || grida.program.document.SCHEMA_VERSION;
+ if (schemaVersion.startsWith("0.90.")) {
+ throw new Error(
+ `Unsupported .grida schema version "${schemaVersion}": migrate 0.90.x files before decoding with the 0.91 NodeSlot layout.`
+ );
+ }
// Decode nodes array and collect parent references
const nodes: Record<string, grida.program.nodes.Node> = {};🤖 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 5452 - 5459, Check the
document schema version before iterating document.nodes() and refuse to decode
NodeSlot for pre-0.91 files: use the existing schemaVersion value (read earlier)
and if it's less than 0.91 throw/return a clear error stating the CanvasDocument
field-id reindex is incompatible, or implement an explicit migration path,
before calling document.nodes(), unionToNode, or treating entries as NodeSlot;
this prevents misdecoding old 0.90.x layout when unionToNode/NodeSlot is
invoked.
- Renamed `text_decoration_thinkness` to `text_decoration_thickness` across multiple files for consistency and clarity. - Updated package documentation and examples to reflect the new naming convention. - Increased the FlatBuffers schema version to `0.91.0-beta+20260311` in relevant files to ensure compatibility with the latest changes. These updates improve code readability and maintain alignment with the updated schema version.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
editor/grida-canvas-hosted/playground/playground.tsx (1)
502-506:⚠️ Potential issue | 🟠 MajorNarrow quarantine matching to explicit decode/schema failures only.
Line 504 still includes
invalid, which is generic and can match non-corruption runtime errors; that can still quarantine healthy OPFS data.Suggested patch
- } else if ( - error instanceof Error && - /decode|schema|corrupt|unreadable|invalid|flatbuffers/i.test( - error.message - ) - ) { + } else if ( + error instanceof Error && + /decode|schema|corrupt|unreadable|flatbuffers/i.test(error.message) + ) {Based on learnings: "Treat changes to
format/grida.fbs(FlatBuffers schema) like public API changes—they ripple into Rust and TypeScript codecs and eventually into real user files".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas-hosted/playground/playground.tsx` around lines 502 - 506, The current error-quarantine conditional uses a broad regex that includes "invalid" and may match unrelated runtime errors; update the conditional that checks "error instanceof Error && /decode|schema|corrupt|unreadable|invalid|flatbuffers/i.test(error.message)" to remove the generic "invalid" token and narrow the pattern to explicit decode/schema-related terms (e.g., keep "decode", "schema", "corrupt", "unreadable", "flatbuffers") so only genuine FlatBuffers/codec failures trigger quarantine; locate this conditional in playground.tsx (the error handling branch that tests error.message) and replace the regex accordingly.crates/grida-canvas/examples/fixtures/l0_effects.rs (1)
35-41:⚠️ Potential issue | 🟠 MajorBackdrop blur case is still not exercising meaningful backdrop sampling.
At Line 35, the backdrop-blur panel is positioned in its own column (
x = gap) away from richer underlying content, so this fixture can under-test the effect behavior.Suggested minimal fix
- let backdrop = effect_rect(gap, LayerEffects { + // Overlap with previously drawn content so backdrop sampling is visible + let backdrop = effect_rect(80.0, LayerEffects { backdrop_blur: Some(FeBackdropBlur { active: true, blur: FeBlur::Gaussian(FeGaussianBlur { radius: 8.0 }), }), ..LayerEffects::default() });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/examples/fixtures/l0_effects.rs` around lines 35 - 41, The backdrop-blur panel created by effect_rect(gap, LayerEffects { backdrop_blur: Some(... ) }) is placed away from richer underlying content (using gap) so it doesn't exercise meaningful backdrop sampling; change the first argument of effect_rect from gap to a position that overlaps the richer content (for example 0 or another coordinate that places the panel directly over the colorful/complex fixture beneath) so LayerEffects.backdrop_blur actually samples varied background pixels.crates/grida-canvas/examples/fixtures/l0_masks.rs (1)
15-31:⚠️ Potential issue | 🟠 MajorThe alpha-mask case is still a no-op.
mask_imgmatchescontent_imgbounds and uses fully opaque white fill, soImageMaskType::Alphaproduces the same result as no mask. Vary the alpha or the mask geometry so this fixture actually exercises alpha sampling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/examples/fixtures/l0_masks.rs` around lines 15 - 31, The alpha-mask fixture is a no-op because mask_img (Node::Rectangle / RectangleNodeRec) is the same size/position as content_img and uses a fully opaque fill (fills: Paints::new(vec![solid(255,255,255,255)])), so ImageMaskType::Alpha doesn't change pixels; modify RectangleNodeRec used by mask_img to actually vary alpha or geometry—e.g., set the fill alpha < 255 or change transform/size or corner radii so the mask differs from content_img—so the alpha sampling path in the rendering code is exercised (look for mask_img, RectangleNodeRec, ImageMaskType::Alpha, and fills).crates/grida-canvas/examples/fixtures/l0_layout_transform.rs (1)
31-37:⚠️ Potential issue | 🟠 MajorThis still doesn't test transform composition.
flat_scenekeeps the container, rect, and line as siblings, so the 90° container rotation never applies to the other two nodes. Usebuild_sceneand attach the rect/line underrotated_containerif this fixture is meant to cover parent-child layout transforms.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/examples/fixtures/l0_layout_transform.rs` around lines 31 - 37, The fixture currently uses flat_scene so rotated_container, rotated_rect, and rotated_line are siblings and do not exercise transform composition; change to using build_scene and make rotated_rect and rotated_line children of rotated_container (attach them to the rotated_container node) so the container's 90° rotation composes with the children's local transforms; update the final scene construction to return build_scene("L0 Layout Transform", rotated_container) or equivalent that nests rotated_rect and rotated_line under rotated_container.
🧹 Nitpick comments (6)
editor/grida-canvas-hosted/playground/playground.tsx (1)
525-527: Fix comment/code mismatch in the non-quarantine branch.Line 526 says “rethrow,” but this branch only logs and continues to fallback. Please update the comment to reflect actual behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas-hosted/playground/playground.tsx` around lines 525 - 527, The comment above the non-quarantine branch is misleading: it says “rethrow” but the code only logs and falls back; change the comment to accurately describe behavior (e.g., "Transient or runtime error (e.g. from reset() / loadImages()). Do not quarantine — log and continue to fallback so the caller can handle it.") and ensure it sits immediately above the console.error("OPFS load error (not quarantining):", error) statement so the text matches the implemented behavior.crates/grida-dev/src/platform/native_application.rs (1)
258-258: Minor: Consider usingprintln!for consistency.Line 258 uses
eprintln!for the scene title, but elsewhere in the codebase (e.g.,native_demo.rsline 66) informational output usesprintln!. Consider usingprintln!for consistency.✨ Optional fix
- eprintln!("{title}"); + println!("{title}");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/platform/native_application.rs` at line 258, Replace the stderr print with a stdout print for consistency: locate the eprintln!("{title}") call (the scene title print using the title variable) and change it to println!("{title}") so informational output matches other files like native_demo.rs; ensure no additional formatting or error handling changes are made.crates/grida-dev/src/grida_file.rs (1)
33-41: BOM handling is overly permissive but functionally correct.The current logic skips any byte matching
0xEF,0xBB, or0xBFanywhere in the leading bytes, not just a proper UTF-8 BOM sequence at the start. While this won't cause false positives for JSON detection, it's slightly imprecise.✨ Optional: More precise BOM handling
// JSON: starts with '{' or '[' (after optional UTF-8 BOM / whitespace) - let trimmed = bytes - .iter() - .position(|&b| !b.is_ascii_whitespace() && b != 0xEF && b != 0xBB && b != 0xBF) - .map(|i| bytes[i]) - .unwrap_or(0); + let start = if bytes.starts_with(&[0xEF, 0xBB, 0xBF]) { 3 } else { 0 }; + let trimmed = bytes[start..] + .iter() + .find(|&&b| !b.is_ascii_whitespace()) + .copied() + .unwrap_or(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-dev/src/grida_file.rs` around lines 33 - 41, The code currently treats any leading 0xEF/0xBB/0xBF bytes as BOM even when they don't form the proper UTF-8 BOM sequence; change the logic to detect an actual BOM sequence only at the very start and otherwise only skip ASCII whitespace. Specifically, in the function handling JSON detection, check bytes.starts_with(&[0xEF,0xBB,0xBF]) and set start = 3 when true (otherwise start = 0), then scan from start for the first non-whitespace byte (using is_ascii_whitespace) and assign that to trimmed; finally compare trimmed to b'{' or b'[' to return Format::Json (references: bytes, trimmed, Format::Json).crates/grida-canvas/examples/fixtures/l0_container.rs (1)
4-5:InitialContainerisn't covered yet.The doc comment says this fixture exercises
InitialContainer, butbuild_sceneis still called withNoneand every node here is a regularContainerNodeRec. If the initial-container path has distinct layout semantics, this scene won't catch regressions there. Either add a real initial container or narrow the fixture description.Also applies to: 179-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/examples/fixtures/l0_container.rs` around lines 4 - 5, The fixture claims to exercise InitialContainer but build() calls build_scene with None and only creates ContainerNodeRec nodes, so add a real InitialContainer path or update the docstring; to fix, modify the build() function to call build_scene(Some(InitialContainer::new(...))) or construct/insert an InitialContainer node variant where relevant (look for build(), build_scene, and ContainerNodeRec in this file) so the scene actually uses InitialContainer semantics, or alternatively change the doc comment to remove the InitialContainer claim if you intend to keep only regular containers.crates/grida-canvas/examples/fixtures/mod.rs (1)
74-80: Unnecessary clone before iteration.
children.clone().iter()creates an unnecessary clone. Sinceget_childrenreturns a reference, you can iterate directly without cloning.♻️ Suggested optimization
if let Some(children) = graph.get_children(nid) { - for (i, child) in children.clone().iter().enumerate() { + for (i, child) in children.iter().enumerate() { let pos = format!("a{i:04}"); position_map.insert(*child, pos); walk(graph, child, counter, id_map, position_map, prefix); } }As per coding guidelines: Run
cargo clippy --no-deps --all-targets --all-featuresfor linting to check style, performance, and correctness suggestions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/examples/fixtures/mod.rs` around lines 74 - 80, The loop is cloning the children collection unnecessarily; replace the clone by iterating over the referenced slice/iter returned from graph.get_children(nid) directly — i.e., obtain children via if let Some(children) = graph.get_children(nid) { for (i, child) in children.iter().enumerate() { ... } } — and then call position_map.insert(*child, pos) and walk(graph, child, counter, id_map, position_map, prefix) as before (keep using the same symbols: graph.get_children, children, position_map.insert, walk, id_map.entry).crates/grida-canvas/examples/fixtures/l0_paints_stack.rs (1)
51-60: Note: Consistent usage ofalignementfield, though underlying field has typo.The
alignementfield spelling matches theImagePaintstruct definition intypes.rs(line 2867). While this fixture correctly uses the existing API, the underlying struct field name appears to be a typo for "alignment". Consider addressing this in a follow-up to avoid API confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/grida-canvas/examples/fixtures/l0_paints_stack.rs` around lines 51 - 60, The ImagePaint struct uses a misspelled field name `alignement`; update the API by renaming the field to `alignment` in the ImagePaint definition (and its serde/derive annotations if present) and then update all usages (e.g., in this fixture and any other references) to use `alignment: Alignment::CENTER`; ensure any constructor calls, pattern matches, tests, and docs referencing `alignement` are adjusted and run cargo check/tests to verify no remaining references.
🤖 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/examples/fixtures/l0_image.rs`:
- Around line 23-36: The r4 fixture uses BlendMode::Screen but has no underlying
content to blend with, so the sample is a no-op; modify the scene so r4 sits
over an explicit underlay (e.g., create a filled rect or colored base paint
placed just before r4 using the same rect() helper) that overlaps r4’s bounds to
provide a controlled backdrop for Paint::Image with BlendMode::Screen; locate
the r4 creation (rect(..., Paint::Image { ... BlendMode::Screen, quarter_turns:
2, ... })) and add a preceding underlay node (filled Paint::Color) that covers
the same area or extends beneath it so the blend mode is exercised.
---
Duplicate comments:
In `@crates/grida-canvas/examples/fixtures/l0_effects.rs`:
- Around line 35-41: The backdrop-blur panel created by effect_rect(gap,
LayerEffects { backdrop_blur: Some(... ) }) is placed away from richer
underlying content (using gap) so it doesn't exercise meaningful backdrop
sampling; change the first argument of effect_rect from gap to a position that
overlaps the richer content (for example 0 or another coordinate that places the
panel directly over the colorful/complex fixture beneath) so
LayerEffects.backdrop_blur actually samples varied background pixels.
In `@crates/grida-canvas/examples/fixtures/l0_layout_transform.rs`:
- Around line 31-37: The fixture currently uses flat_scene so rotated_container,
rotated_rect, and rotated_line are siblings and do not exercise transform
composition; change to using build_scene and make rotated_rect and rotated_line
children of rotated_container (attach them to the rotated_container node) so the
container's 90° rotation composes with the children's local transforms; update
the final scene construction to return build_scene("L0 Layout Transform",
rotated_container) or equivalent that nests rotated_rect and rotated_line under
rotated_container.
In `@crates/grida-canvas/examples/fixtures/l0_masks.rs`:
- Around line 15-31: The alpha-mask fixture is a no-op because mask_img
(Node::Rectangle / RectangleNodeRec) is the same size/position as content_img
and uses a fully opaque fill (fills: Paints::new(vec![solid(255,255,255,255)])),
so ImageMaskType::Alpha doesn't change pixels; modify RectangleNodeRec used by
mask_img to actually vary alpha or geometry—e.g., set the fill alpha < 255 or
change transform/size or corner radii so the mask differs from content_img—so
the alpha sampling path in the rendering code is exercised (look for mask_img,
RectangleNodeRec, ImageMaskType::Alpha, and fills).
In `@editor/grida-canvas-hosted/playground/playground.tsx`:
- Around line 502-506: The current error-quarantine conditional uses a broad
regex that includes "invalid" and may match unrelated runtime errors; update the
conditional that checks "error instanceof Error &&
/decode|schema|corrupt|unreadable|invalid|flatbuffers/i.test(error.message)" to
remove the generic "invalid" token and narrow the pattern to explicit
decode/schema-related terms (e.g., keep "decode", "schema", "corrupt",
"unreadable", "flatbuffers") so only genuine FlatBuffers/codec failures trigger
quarantine; locate this conditional in playground.tsx (the error handling branch
that tests error.message) and replace the regex accordingly.
---
Nitpick comments:
In `@crates/grida-canvas/examples/fixtures/l0_container.rs`:
- Around line 4-5: The fixture claims to exercise InitialContainer but build()
calls build_scene with None and only creates ContainerNodeRec nodes, so add a
real InitialContainer path or update the docstring; to fix, modify the build()
function to call build_scene(Some(InitialContainer::new(...))) or
construct/insert an InitialContainer node variant where relevant (look for
build(), build_scene, and ContainerNodeRec in this file) so the scene actually
uses InitialContainer semantics, or alternatively change the doc comment to
remove the InitialContainer claim if you intend to keep only regular containers.
In `@crates/grida-canvas/examples/fixtures/l0_paints_stack.rs`:
- Around line 51-60: The ImagePaint struct uses a misspelled field name
`alignement`; update the API by renaming the field to `alignment` in the
ImagePaint definition (and its serde/derive annotations if present) and then
update all usages (e.g., in this fixture and any other references) to use
`alignment: Alignment::CENTER`; ensure any constructor calls, pattern matches,
tests, and docs referencing `alignement` are adjusted and run cargo check/tests
to verify no remaining references.
In `@crates/grida-canvas/examples/fixtures/mod.rs`:
- Around line 74-80: The loop is cloning the children collection unnecessarily;
replace the clone by iterating over the referenced slice/iter returned from
graph.get_children(nid) directly — i.e., obtain children via if let
Some(children) = graph.get_children(nid) { for (i, child) in
children.iter().enumerate() { ... } } — and then call
position_map.insert(*child, pos) and walk(graph, child, counter, id_map,
position_map, prefix) as before (keep using the same symbols:
graph.get_children, children, position_map.insert, walk, id_map.entry).
In `@crates/grida-dev/src/grida_file.rs`:
- Around line 33-41: The code currently treats any leading 0xEF/0xBB/0xBF bytes
as BOM even when they don't form the proper UTF-8 BOM sequence; change the logic
to detect an actual BOM sequence only at the very start and otherwise only skip
ASCII whitespace. Specifically, in the function handling JSON detection, check
bytes.starts_with(&[0xEF,0xBB,0xBF]) and set start = 3 when true (otherwise
start = 0), then scan from start for the first non-whitespace byte (using
is_ascii_whitespace) and assign that to trimmed; finally compare trimmed to b'{'
or b'[' to return Format::Json (references: bytes, trimmed, Format::Json).
In `@crates/grida-dev/src/platform/native_application.rs`:
- Line 258: Replace the stderr print with a stdout print for consistency: locate
the eprintln!("{title}") call (the scene title print using the title variable)
and change it to println!("{title}") so informational output matches other files
like native_demo.rs; ensure no additional formatting or error handling changes
are made.
In `@editor/grida-canvas-hosted/playground/playground.tsx`:
- Around line 525-527: The comment above the non-quarantine branch is
misleading: it says “rethrow” but the code only logs and falls back; change the
comment to accurately describe behavior (e.g., "Transient or runtime error (e.g.
from reset() / loadImages()). Do not quarantine — log and continue to fallback
so the caller can handle it.") and ensure it sits immediately above the
console.error("OPFS load error (not quarantining):", error) statement so the
text matches the implemented behavior.
🪄 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: 45bf31f9-45d6-4bdd-ab96-83e990f1351c
📒 Files selected for processing (49)
crates/grida-canvas/AGENTS.mdcrates/grida-canvas/Cargo.tomlcrates/grida-canvas/README.mdcrates/grida-canvas/examples/fixtures/README.mdcrates/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_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_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_type_decoration.rscrates/grida-canvas/examples/golden_type_decoration_color.rscrates/grida-canvas/examples/golden_type_emoji_placeholder.rscrates/grida-canvas/examples/tool_gen_fixtures.rscrates/grida-canvas/src/cg/types.rscrates/grida-canvas/src/io/io_figma.rscrates/grida-canvas/src/io/io_grida.rscrates/grida-canvas/src/io/io_grida_fbs.rscrates/grida-canvas/src/sk/mappings.rscrates/grida-canvas/src/text/attributed_text_conv.rscrates/grida-dev/src/grida_file.rscrates/grida-dev/src/main.rscrates/grida-dev/src/platform/native_application.rscrates/grida-dev/src/platform/native_demo.rseditor/grida-canvas-hosted/playground/playground.tsxeditor/grida-canvas/editor.tseditor/scaffolds/editor/editor.tsxeditor/scaffolds/editor/init.tseditor/scaffolds/editor/sync/agent-startpage.sync.tsxfixtures/test-grida/L0.gridafixtures/test-grida/README.mdformat/README.md
✅ Files skipped from review due to trivial changes (1)
- crates/grida-canvas/examples/fixtures/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- editor/scaffolds/editor/init.ts
- fixtures/test-grida/README.md
- crates/grida-canvas/Cargo.toml
- crates/grida-canvas/src/io/io_grida.rs
Comment on lines
+23
to
+36
| // Quarter turns + alignment + Screen blend | ||
| let r4 = rect(gap * 3.0, 0.0, s, s, | ||
| Paint::Image(ImagePaint { | ||
| active: true, | ||
| image: img(), | ||
| fit: ImagePaintFit::Fit(BoxFit::Cover), | ||
| filters: ImageFilters::default(), | ||
| opacity: 1.0, | ||
| blend_mode: BlendMode::Screen, | ||
| quarter_turns: 2, | ||
| alignement: Alignment::BOTTOM_RIGHT, | ||
| })); | ||
|
|
||
| flat_scene("L0 Image", vec![r1, r2, r3, r4]) |
There was a problem hiding this comment.
The Screen sample has no controlled backdrop.
r4 does not overlap any earlier node, so BlendMode::Screen is either a no-op or coupled to whatever canvas background the renderer injects. Add an explicit underlay/base fill here so this fixture actually exercises image blending.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/examples/fixtures/l0_image.rs` around lines 23 - 36, The
r4 fixture uses BlendMode::Screen but has no underlying content to blend with,
so the sample is a no-op; modify the scene so r4 sits over an explicit underlay
(e.g., create a filled rect or colored base paint placed just before r4 using
the same rect() helper) that overlaps r4’s bounds to provide a controlled
backdrop for Paint::Image with BlendMode::Screen; locate the r4 creation
(rect(..., Paint::Image { ... BlendMode::Screen, quarter_turns: 2, ... })) and
add a preceding underlay node (filled Paint::Color) that covers the same area or
extends beneath it so the blend mode is exercised.
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.
.gridafbs decode/encode support from rsNew Features
Bug Fixes
Documentation