Skip to content

cg: enhance file I/O, layout engine, FBS decoder, and add new example fixtures#586

Merged
softmarshmallow merged 8 commits intomainfrom
canary
Mar 17, 2026
Merged

cg: enhance file I/O, layout engine, FBS decoder, and add new example fixtures#586
softmarshmallow merged 8 commits intomainfrom
canary

Conversation

@softmarshmallow
Copy link
Copy Markdown
Member

@softmarshmallow softmarshmallow commented Mar 17, 2026

Summary

A batch of improvements to the cg (grida-canvas) crate spanning file I/O, layout extraction, FlatBuffers decoding, and example fixtures.

File I/O & Inspector (tool_io_grida)

  • Rewritten tool_io_grida from a basic validator into a comprehensive file inspector supporting .grida (FlatBuffers), .grida (ZIP archive), and legacy .grida1 (JSON) formats with auto-detection.
  • New CLI options: --list-scenes, --scene <N>, --verbose (full node tree), --layout-check (diagnoses "Container must have layout result" panics).
  • Extracted a new io_grida_file module for unified file format detection and decoding.
  • Added zip crate dependency (deflate-only, pure Rust, wasm-safe) for ZIP archive support.

Layout Engine Fix

  • Fixed layout extraction for non-Taffy root nodes (e.g., BooleanOperation, Group) — previously these could cause panics in GeometryCache because descendant containers had no layout results.
  • Added tests verifying that descendants of non-Taffy roots correctly receive layout data.

FBS Decoder Enhancements

  • Added DiamondGradientPaint decoding support.
  • Fixed tile mode decoding to use correct FlatBuffers schema values.
  • Added TextDecorationLine and TextDecorationStyle decoding for richer text styling.

New Example Fixtures

  • cover.rs — 1600x900 hero-image scene showcasing gradients, shadows, blend modes, corner smoothing, and typography, with a corresponding tool_gen_cover example and cover.grida output.
  • l0_effects_progressive_blur — Progressive blur (layer + backdrop) with varying gradient directions.
  • l0_strokes_varwidth — Variable-width stroke profiles (taper, reverse taper, bulge, multi-stop, uniform).
  • l0_layout_transform — Expanded from 3 to 16 test cases covering rotation at key angles, custom transform origins, nested container composition, and clipped rotated containers.
  • Updated fixtures README.md with descriptions for all new scenes.
  • Added tool_gen_bench_fixture for generating benchmark .grida files.

Agent/Skill Documentation

  • Added .agents/skills/research/SKILL.md — research workflow for investigating upstream projects (Chromium, Skia, Servo) before writing code.
  • Updated crates/grida-canvas/AGENTS.md to reflect the expanded tool_io_grida capabilities.

Housekeeping

  • Consolidated .grida file reading logic from grida-dev into the new io_grida_file module, reducing duplication.
  • New test .grida fixture files: L0.grida, bench.grida, cover.grida.

Summary by CodeRabbit

  • New Features

    • Progressive blur, variable-width strokes, diamond gradients, richer image filters, expanded text styling, aspect-ratio layout
    • CLI file inspector with auto-detect, scene listing, layout checks, and ZIP/format support
    • Multiple new visual example scenes, cover generator, and benchmark scene
  • Bug Fixes

    • Layout computation updated to cover subtrees under non-layout roots
  • Documentation

    • Added Code Research Skill guide and expanded fixtures/examples docs
  • Chores

    • Reworked inspector UX and module exports

…alidation

- Renamed the tool from "Grida File Validator" to "Grida File Inspector" to better reflect its functionality.
- Expanded the tool's capabilities to inspect and validate both `.grida` and `.grida1` files in various formats (FlatBuffers, ZIP, JSON).
- Added new command-line options for listing scenes, inspecting specific scenes, and running layout checks.
- Updated documentation to include detailed usage instructions and feature descriptions.
- Introduced a new module for file format detection and unified decoding, improving the overall architecture and usability of the tool.
- Updated the layout engine to extract layouts for all root nodes, including those that do not participate in Taffy (e.g., BooleanOperation, Group).
- Added tests to verify that descendant containers receive layout results even when their parent is a non-Taffy node, preventing panics in GeometryCache.
- Enhanced the handling of layout results to ensure all nodes have valid layout data during computation.
- Introduced a new SKILL.md file detailing the research skill for Grida, outlining its purpose and workflow for investigating upstream and peer projects.
- Provided guidelines on when to use the skill, how to orient oneself in existing research, and key discovery queries for effective research practices.
- Included references to relevant repositories and documentation to assist in the research process, enhancing the overall knowledge base for developers.
- Introduced a new example `tool_gen_cover.rs` that generates a cover image and a corresponding `.grida` file from a defined scene.
- Added a `cover.rs` fixture module to build a visually rich scene showcasing the capabilities of the cg engine, including gradients, shadows, and typography.
- Updated the `mod.rs` file to include the new `cover` module for better organization of example fixtures.
- Added a new `cover.grida` file to store the generated scene data.
- Updated the existing `cover.png` with a new version reflecting the latest design changes.
@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 17, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
backgrounds Ready Ready Preview, Comment Mar 17, 2026 0:00am
blog Ready Ready Preview, Comment Mar 17, 2026 0:00am
docs Ready Ready Preview, Comment Mar 17, 2026 0:00am
grida Ready Ready Preview, Comment Mar 17, 2026 0:00am
viewer Ready Ready Preview, Comment Mar 17, 2026 0:00am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
code Ignored Ignored Mar 17, 2026 0:00am
legacy Ignored Ignored Mar 17, 2026 0:00am

Request Review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 17, 2026

Walkthrough

Adds a code-research SKILL doc; introduces multiple new example fixtures (cover, progressive blur, var-width strokes) and expanded transform examples; rewrites the CLI inspector into a format-aware inspector; adds a unified io_grida_file module; extends FlatBuffers encode/decode (progressive blur, varwidth strokes, richer text, diamond gradients, image filters, aspect-aware layout); and updates layout engine to compute layouts for all roots.

Changes

Cohort / File(s) Summary
Documentation & Skill
/.agents/skills/research/SKILL.md, crates/grida-canvas/AGENTS.md
Adds Code Research SKILL doc and updates AGENTS usage/branding.
New Example Fixtures
crates/grida-canvas/examples/fixtures/cover.rs, crates/grida-canvas/examples/fixtures/l0_effects_progressive_blur.rs, crates/grida-canvas/examples/fixtures/l0_strokes_varwidth.rs
Adds cover and L0 example scenes; public build() -> Scene and size constants.
Expanded Fixtures & README
crates/grida-canvas/examples/fixtures/l0_layout_transform.rs, crates/grida-canvas/examples/fixtures/README.md
Broadens transform examples and README with many new cases/exercises.
Fixtures Module & Generators
crates/grida-canvas/examples/fixtures/mod.rs, crates/grida-canvas/examples/tool_gen_cover.rs, crates/grida-canvas/examples/tool_gen_fixtures.rs, crates/grida-canvas/examples/tool_gen_bench_fixture.rs
Exports new fixture modules; adds cover generator and bench scene entry.
CLI Inspector
crates/grida-canvas/examples/tool_io_grida.rs
Rewrites inspector CLI to auto-detect formats (FBS/ZIP/JSON) and add flags: --list-scenes, --scene, --layout-check, --verbose, scene stats, node-tree printing, and layout validation.
Format-aware I/O module
crates/grida-canvas/src/io/io_grida_file.rs, crates/grida-canvas/src/io/mod.rs
New io_grida_file with Format detection, decode/decode_all/decode_with_id_map, ZIP extraction, and rich DecodeError variants.
FlatBuffers codec & schema plumbing
crates/grida-canvas/src/io/io_grida_fbs.rs
Extended encode/decode: Progressive blur, varwidth stroke profiles, richer text (features/variations/decoration/ellipsis), DiamondGradientPaint, image filters, aspect-aware layout dims; updated helper/encoder signatures.
Layout engine & grida-dev wrapper
crates/grida-canvas/src/layout/engine.rs, crates/grida-dev/src/grida_file.rs
Ensure layout computation for Taffy subtrees under non-Taffy roots; added tests; grida-dev now delegates to io_grida_file.
Cargo / Manifests & README
crates/grida-canvas/Cargo.toml, format/README.md
Adds zip = "2" dep (deflate feature), example entry for tool_io_grida, and new references section in format README.

Sequence Diagram(s)

sequenceDiagram
  participant CLI as "tool_io_grida (CLI)"
  participant IO as "io_grida_file"
  participant FBS as "io_grida_fbs"
  participant ZIP as "ZIP extractor"
  participant JSON as "JSON parser"
  participant Layout as "Layout Engine"

  rect rgba(220,240,255,0.5)
    CLI->>IO: supply file bytes
    IO->>IO: detect format (RawFbs / Zip / Json)
  end

  rect rgba(200,255,200,0.5)
    alt RawFbs
      IO->>FBS: decode bytes -> Scene(s) + id_map
    else Zip
      IO->>ZIP: extract document.grida -> fbs bytes
      ZIP->>FBS: pass fbs bytes -> DecodeResult
    else Json
      IO->>JSON: parse JSON -> Canvas AST
      JSON->>IO: convert -> Scene(s)
    end
  end

  rect rgba(255,230,200,0.5)
    CLI->>CLI: evaluate flags (--list-scenes/--scene/--layout-check)
    CLI->>Layout: optionally run layout-check on Scene(s)
    Layout->>Layout: compute layouts (includes extra roots)
    CLI-->>CLI: present results (stats/tree/validation)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

canvas

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: enhancing file I/O, layout engine, FBS decoder, and adding new example fixtures across the grida-canvas crate.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch canary
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

- Introduced a new scene `scene_progressive_blur_grid` showcasing progressive blur with varying gradient directions across a grid of rectangles.
- Added `l0_effects_progressive_blur` fixture demonstrating layer and backdrop progressive blur effects.
- Created `l0_strokes_varwidth` fixture to illustrate variable-width stroke profiles on vector paths with various taper and bulge patterns.
- Updated `mod.rs` to include new fixture modules and enhanced the README with descriptions of the new examples.
- Modified existing examples to incorporate the new features, ensuring comprehensive coverage of effects and strokes.
…ion options

- Added support for decoding `DiamondGradientPaint` in the paint item processing.
- Updated tile mode decoding to utilize the correct values from the FlatBuffers schema.
- Introduced new text decoration options including `TextDecorationLine` and `TextDecorationStyle` for improved text styling capabilities.
- Enhanced existing paint and layout decoding functions to accommodate new features and ensure consistency across the rendering pipeline.
- Updated related tests and documentation to reflect these changes.
@softmarshmallow softmarshmallow changed the title Enhance Grida File Inspector with layout extraction and documentation cg: enhance file I/O, layout engine, FBS decoder, and add new example fixtures Mar 17, 2026
@softmarshmallow softmarshmallow marked this pull request as ready for review March 17, 2026 11:03
@softmarshmallow softmarshmallow added cg Core Graphics chore labels Mar 17, 2026
@softmarshmallow
Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 17, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

- Added references to the Adobe Photoshop File Format Specification and Figma .fig format documentation.
- Enhanced contributor workflow section to guide users on regenerating bindings after editing `grida.fbs`.
- Improved overall clarity and resource accessibility for developers working with file formats.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/grida-canvas/src/io/io_grida_fbs.rs (1)

629-660: ⚠️ Potential issue | 🟠 Major

Preserve tiled image paints in the decode path.

While expanding ImagePaint here, decode_image_paint_fit() still has no ImagePaintFitTile branch, even though the encoder writes that variant at Line 2416. Tiled assets will silently decode as Fit(Cover), which breaks rendering and round-trip fidelity.

🧹 Nitpick comments (2)
crates/grida-canvas/examples/fixtures/l0_layout_transform.rs (1)

214-249: Replace the duplicated numeric IDs with named NodeIds.

build_scene() already accepts NodeId-typed collections (crates/grida-canvas/examples/fixture_helpers/mod.rs:22-35), but this fixture repeats the same bare integers across pairs, links, and roots. That makes later edits brittle: inserting or renumbering one node can silently break the parent/child wiring. Prefer NodeId constants or a tiny allocator so each relationship is declared once.

As per coding guidelines, use NodeId (u64) for internal counter-based IDs in structs like NodeRecs, SceneGraph, and caches 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/examples/fixtures/l0_layout_transform.rs` around lines
214 - 249, The fixture uses raw numeric IDs in pairs, links, and the roots
vector which duplicates and risks breakage; replace these bare integers with
named NodeId values (e.g., consts or a small NodeId allocator) and use the
NodeId type everywhere build_scene is called so the same identifier is declared
once and reused; update the pairs Vec<(u64, Node)> to Vec<(NodeId, Node)> (or
cast/construct NodeId) and change the HashMap keys and roots Vec to the NodeId
type, and ensure container keys (13,15,17) and their child references (14,16,18)
are replaced by those named NodeId bindings so parent/child wiring is only
defined once and remains stable when nodes are inserted or renumbered while
still calling build_scene("L0 Layout Transform", None, pairs, links, roots).
crates/grida-canvas/src/io/io_grida_file.rs (1)

91-134: Implement std::error::Error for DecodeError.

Right now callers like crates/grida-dev/src/grida_file.rs have to stringify and re-wrap these errors, which drops the FbsDecodeError source chain. Adding Error::source() here keeps propagation and diagnostics intact.

♻️ Proposed error-chain support
 impl From<io_grida_fbs::FbsDecodeError> for DecodeError {
     fn from(e: io_grida_fbs::FbsDecodeError) -> Self {
         DecodeError::Fbs(e)
     }
 }
+
+impl std::error::Error for DecodeError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        match self {
+            DecodeError::Fbs(e) => Some(e),
+            _ => None,
+        }
+    }
+}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/grida-canvas/src/io/io_grida_file.rs` around lines 91 - 134, Add an
impl of std::error::Error for DecodeError so callers can access error sources;
implement fn source(&self) -> Option<&(dyn std::error::Error + 'static)> that
returns Some(e) for the Fbs variant (DecodeError::Fbs(e)) and None for the other
variants (UnrecognizedFormat, Zip, Json, Conversion, JsonNotSupported,
NoScenes); ensure io_grida_fbs::FbsDecodeError implements Error (it already
should) and import std::error::Error if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.agents/skills/research/SKILL.md:
- Around line 33-44: The fenced code block in SKILL.md showing the directory
tree lacks a language specifier; update the opening fence (the triple backticks
that start the tree block) to include a language tag (e.g., "text") so the block
becomes ```text and satisfies linting and consistent rendering for the directory
tree visualization.

In `@crates/grida-canvas/examples/fixtures/l0_effects_progressive_blur.rs`:
- Around line 68-84: The backdrop blur card (the variable backdrop created with
effect_rect and LayerEffects.backdrop_blur) has no pixels behind it because
flat_scene sets background_color to None and the card doesn't overlap earlier
nodes; make the test meaningful by placing patterned content or another node
directly behind or overlapping the backdrop card. Modify the scene setup used by
flat_scene (or the vec passed into flat_scene) so that the backdrop node named
backdrop overlaps an earlier node (e.g., move or duplicate
layer_tl_br/layer_top_bottom/layer_both behind it) or add a patterned background
node before backdrop so FeBackdropBlur on backdrop actually samples pixels
behind it. Ensure the change keeps the backdrop variable and effect_rect usage
so the progressive-blur path is exercised.

In `@crates/grida-canvas/examples/tool_gen_cover.rs`:
- Around line 29-31: The test currently writes the serialized scene with
fixture_helpers::write_multi_fixture(&scenes, "cover") but then renders the
original scene variable, which misses encode/decode regressions; after calling
fixture_helpers::write_multi_fixture (or using the bytes it returns if
available), read back the just-written .grida data and decode it into a scene
instance and use that decoded scene for subsequent rendering (replace uses of
the original scene variable when generating cover.png), and make the same change
for the second occurrence around the area noted (line ~46) so both renders use
the round-tripped scene.
- Around line 48-50: Refactor the unsafe borrow of the surface out of the
example by adding a safe helper on Renderer (e.g., a with_surface_canvas or
with_surface<R, F>(&mut self, f: F) -> R where F: FnOnce(&mut Canvas) -> R) that
internally does the backend.get_surface() unsafe borrow, calls the closure with
the surface.canvas(), and releases the borrow; then update the example to call
renderer.with_surface_canvas(|canvas| renderer.render_to_canvas(canvas, width,
height)) (or alternatively move render_to_canvas into a &mut self API that uses
the same helper). Follow the pattern already used by Renderer::snapshot() and
Renderer::canvas() to own the surface-borrow lifecycle and avoid exposing unsafe
backend.get_surface() in examples.

In `@crates/grida-canvas/examples/tool_io_grida.rs`:
- Around line 120-184: The inspect_json function does not handle CLI flags
related to scene selection like --scene or --list-scenes, causing inconsistent
CLI behavior with legacy JSON input. To fix this, add checks for these flags in
inspect_json and return an explicit error message if they are used with legacy
JSON input to prevent silent ignoring. This ensures the CLI behavior remains
predictable when processing legacy grida1 files.

In `@crates/grida-canvas/src/io/io_grida_file.rs`:
- Around line 149-154: The code currently trusts zip::ZipFile::size() and
allocates/reads unbounded (doc, fbs_bytes, read_to_end), so add a hard
extraction limit (e.g. MAX_UNCOMPRESSED_SIZE constant) and enforce it before
allocation and while reading: check doc.size() (and treat missing/zero/too-large
as error), cap allocation to min(doc.size(), MAX_UNCOMPRESSED_SIZE) and replace
the unbounded read_to_end with a bounded reader (e.g.
doc.take(MAX_UNCOMPRESSED_SIZE) or iterative reads that track total bytes) and
return a DecodeError::Zip if the file exceeds MAX_UNCOMPRESSED_SIZE or the
announced size is absurd; reference the variables/functions doc, fbs_bytes,
archive, ZipFile::size(), and read_to_end() when making the changes.

In `@crates/grida-canvas/src/layout/engine.rs`:
- Around line 103-108: build_taffy_subtree currently returns None when
encountering non-Taffy roots (e.g., Group/BooleanOperation), which prevents
visiting children and denies Taffy layout to Taffy-capable descendants; change
build_taffy_subtree so that when a node is not itself Taffy-participating it
does not immediately return None but instead recurses into its children (calling
build_taffy_subtree for each child) to locate and construct Taffy subtrees for
descendants, merging any found subtrees into the result map; keep
extract_all_layouts usage for schema fallbacks but ensure build_taffy_subtree
still visits children of non-Taffy nodes so Containers and other Taffy-capable
nodes receive proper Taffy layout rather than only schema fallbacks.

---

Nitpick comments:
In `@crates/grida-canvas/examples/fixtures/l0_layout_transform.rs`:
- Around line 214-249: The fixture uses raw numeric IDs in pairs, links, and the
roots vector which duplicates and risks breakage; replace these bare integers
with named NodeId values (e.g., consts or a small NodeId allocator) and use the
NodeId type everywhere build_scene is called so the same identifier is declared
once and reused; update the pairs Vec<(u64, Node)> to Vec<(NodeId, Node)> (or
cast/construct NodeId) and change the HashMap keys and roots Vec to the NodeId
type, and ensure container keys (13,15,17) and their child references (14,16,18)
are replaced by those named NodeId bindings so parent/child wiring is only
defined once and remains stable when nodes are inserted or renumbered while
still calling build_scene("L0 Layout Transform", None, pairs, links, roots).

In `@crates/grida-canvas/src/io/io_grida_file.rs`:
- Around line 91-134: Add an impl of std::error::Error for DecodeError so
callers can access error sources; implement fn source(&self) -> Option<&(dyn
std::error::Error + 'static)> that returns Some(e) for the Fbs variant
(DecodeError::Fbs(e)) and None for the other variants (UnrecognizedFormat, Zip,
Json, Conversion, JsonNotSupported, NoScenes); ensure
io_grida_fbs::FbsDecodeError implements Error (it already should) and import
std::error::Error if needed.
🪄 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: 845495a5-8e62-4c5b-864d-7385a8ef21f9

📥 Commits

Reviewing files that changed from the base of the PR and between cb3af59 and f44836e.

⛔ Files ignored due to path filters (1)
  • crates/grida-canvas/cover.png is excluded by !**/*.png
📒 Files selected for processing (21)
  • .agents/skills/research/SKILL.md
  • crates/grida-canvas/AGENTS.md
  • crates/grida-canvas/Cargo.toml
  • crates/grida-canvas/examples/fixtures/README.md
  • crates/grida-canvas/examples/fixtures/cover.rs
  • crates/grida-canvas/examples/fixtures/l0_effects_progressive_blur.rs
  • crates/grida-canvas/examples/fixtures/l0_layout_transform.rs
  • crates/grida-canvas/examples/fixtures/l0_strokes_varwidth.rs
  • crates/grida-canvas/examples/fixtures/mod.rs
  • crates/grida-canvas/examples/tool_gen_bench_fixture.rs
  • crates/grida-canvas/examples/tool_gen_cover.rs
  • crates/grida-canvas/examples/tool_gen_fixtures.rs
  • crates/grida-canvas/examples/tool_io_grida.rs
  • crates/grida-canvas/src/io/io_grida_fbs.rs
  • crates/grida-canvas/src/io/io_grida_file.rs
  • crates/grida-canvas/src/io/mod.rs
  • crates/grida-canvas/src/layout/engine.rs
  • crates/grida-dev/src/grida_file.rs
  • fixtures/test-grida/L0.grida
  • fixtures/test-grida/bench.grida
  • fixtures/test-grida/cover.grida

Comment thread .agents/skills/research/SKILL.md Outdated
Comment thread crates/grida-canvas/examples/fixtures/l0_effects_progressive_blur.rs Outdated
Comment on lines +29 to +31
// ── 2. Write .grida file ────────────────────────────────────────────
let scenes: Vec<(&str, _)> = vec![("cover", scene.clone())];
fixture_helpers::write_multi_fixture(&scenes, "cover");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Render the serialized round-trip, not the original scene.

fixture_helpers::write_multi_fixture() only verifies that decoding succeeds and that scene names match (crates/grida-canvas/examples/fixture_helpers/mod.rs:127-157). Because this tool renders the original scene afterward, encode/decode regressions can still produce a “good” cover.png while cover.grida is wrong. Load the just-written bytes/file back and render that scene instead.

Also applies to: 46-46

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/grida-canvas/examples/tool_gen_cover.rs` around lines 29 - 31, The
test currently writes the serialized scene with
fixture_helpers::write_multi_fixture(&scenes, "cover") but then renders the
original scene variable, which misses encode/decode regressions; after calling
fixture_helpers::write_multi_fixture (or using the bytes it returns if
available), read back the just-written .grida data and decode it into a scene
instance and use that decoded scene for subsequent rendering (replace uses of
the original scene variable when generating cover.png), and make the same change
for the second occurrence around the area noted (line ~46) so both renders use
the round-tripped scene.

Comment on lines +48 to +50
let surface = unsafe { &mut *renderer.backend.get_surface() };
let canvas = surface.canvas();
renderer.render_to_canvas(canvas, width, height);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Renderer / backend surface APIs"
rg -n -C3 '\bfn (render_to_canvas|get_surface|render)\b' crates/grida-canvas/src crates/grida-canvas/examples

echo
echo "Direct get_surface call sites"
rg -n -C2 '\bget_surface\s*\(' crates/grida-canvas/src crates/grida-canvas/examples

Repository: gridaco/grida

Length of output: 8542


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Get the full implementation of render_to_canvas
echo "=== render_to_canvas implementation ==="
sed -n '1468,1500p' crates/grida-canvas/src/runtime/scene.rs

echo
echo "=== Methods called by render_to_canvas (frame, paint_frame, etc.) ==="
# Look for the frame method
sed -n '1400,1468p' crates/grida-canvas/src/runtime/scene.rs | tail -n 70

Repository: gridaco/grida

Length of output: 4203


Encapsulate the unsafe surface access into a safe Renderer API.

The unsafe pattern bypasses the type system unnecessarily. While render_to_canvas currently only takes an immutable self borrow and doesn't access backend, this API design is brittle—future changes to the method could reintroduce unsafe aliasing bugs. Since the same pattern is already used internally in Renderer::snapshot() and Renderer::canvas(), extract this into a safe helper method that owns the full surface borrow lifecycle. This allows the examples to remain safe without relying on external unsafe code and defensive understanding of internal implementation details.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/grida-canvas/examples/tool_gen_cover.rs` around lines 48 - 50,
Refactor the unsafe borrow of the surface out of the example by adding a safe
helper on Renderer (e.g., a with_surface_canvas or with_surface<R, F>(&mut self,
f: F) -> R where F: FnOnce(&mut Canvas) -> R) that internally does the
backend.get_surface() unsafe borrow, calls the closure with the
surface.canvas(), and releases the borrow; then update the example to call
renderer.with_surface_canvas(|canvas| renderer.render_to_canvas(canvas, width,
height)) (or alternatively move render_to_canvas into a &mut self API that uses
the same helper). Follow the pattern already used by Renderer::snapshot() and
Renderer::canvas() to own the surface-borrow lifecycle and avoid exposing unsafe
backend.get_surface() in examples.

Comment thread crates/grida-canvas/examples/tool_io_grida.rs
Comment thread crates/grida-canvas/src/io/io_grida_file.rs
Comment thread crates/grida-canvas/src/layout/engine.rs
- Updated `inspect_json` function to reject unsupported options for legacy JSON files, preventing misuse.
- Enhanced layout engine to compute layouts for Taffy subtrees discovered under non-Taffy parents, ensuring proper layout for nested structures.
- Introduced a hard limit on the uncompressed size of `document.grida` in ZIP archives to protect against zip bombs and excessive memory allocation.
- Improved documentation and comments for clarity on new features and error handling.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
crates/grida-canvas/src/io/io_grida_file.rs (1)

204-211: Consider avoiding allocation for RawFbs path (optional optimization).

resolve_fbs_bytes copies the entire input for RawFbs format (bytes.to_vec()), which is unnecessary since FlatBuffers can verify and access data directly from a borrowed slice.

This could be optimized by returning Cow<'_, [u8]> or having the FBS decoder work directly with the input slice. However, this is a minor performance consideration for file loading—not blocking.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/grida-canvas/src/io/io_grida_file.rs` around lines 204 - 211, The
function resolve_fbs_bytes currently clones the input slice for the RawFbs
branch (bytes.to_vec()), which is unnecessary; update either resolve_fbs_bytes
to return a borrowed-friendly type (e.g., Cow<'_, [u8]>) or change the FBS
decoder to accept a borrowed &[u8] so you can return a borrowed slice for
Format::RawFbs instead of allocating; modify resolve_fbs_bytes signature and
callers accordingly (refer to resolve_fbs_bytes, detect, Format::RawFbs, and
DecodeError) to avoid the allocation while preserving the existing
Zip/Json/Unknown behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/grida-canvas/src/io/io_grida_file.rs`:
- Around line 204-211: The function resolve_fbs_bytes currently clones the input
slice for the RawFbs branch (bytes.to_vec()), which is unnecessary; update
either resolve_fbs_bytes to return a borrowed-friendly type (e.g., Cow<'_,
[u8]>) or change the FBS decoder to accept a borrowed &[u8] so you can return a
borrowed slice for Format::RawFbs instead of allocating; modify
resolve_fbs_bytes signature and callers accordingly (refer to resolve_fbs_bytes,
detect, Format::RawFbs, and DecodeError) to avoid the allocation while
preserving the existing Zip/Json/Unknown behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 80c3c30e-d619-48df-9e10-323b388d89e8

📥 Commits

Reviewing files that changed from the base of the PR and between 1ccf7da and 715a592.

📒 Files selected for processing (5)
  • .agents/skills/research/SKILL.md
  • crates/grida-canvas/examples/fixtures/l0_effects_progressive_blur.rs
  • crates/grida-canvas/examples/tool_io_grida.rs
  • crates/grida-canvas/src/io/io_grida_file.rs
  • crates/grida-canvas/src/layout/engine.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/grida-canvas/examples/fixtures/l0_effects_progressive_blur.rs
  • .agents/skills/research/SKILL.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cg Core Graphics chore

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant