Skip to content

skia-safe 0.91.0#463

Merged
softmarshmallow merged 6 commits intomainfrom
skia-91
Dec 14, 2025
Merged

skia-safe 0.91.0#463
softmarshmallow merged 6 commits intomainfrom
skia-91

Conversation

@softmarshmallow
Copy link
Copy Markdown
Member

@softmarshmallow softmarshmallow commented Dec 9, 2025

  • update path building practices
  • update font managing practices (needs review)
  • introduce tests including actual rendering pipeline, automated, for testing wasm bins

Investigate: texts wont render properly only on wasm target (rust-skia/rust-skia#1234)

need comprehensive render tests over text rendering and path building as 91+ introduces major changes.

Links:

Internal notes (AI researched, may contain false):

Font Manager Changes in skia-safe 0.89.0 → 0.91.0

Background: Upgrading from skia-safe 0.89.0 to 0.91.0 pulls in Skia’s milestone 143, which significantly changed font handling. In particular, Skia removed the use of a built-in “default” font manager (especially in builds with no system fonts, like WASM). As a result, the default font/typeface is now essentially empty, meaning if you don’t explicitly provide a font, Skia will find nothing and render the “.notdef” missing-glyph box . This is why after the upgrade your text shows no characters – the font manager isn’t automatically picking up any fonts.

New FontMgr::custom_empty API: The Rust bindings responded to this change in PR #1224, which introduced a new FontMgr::custom_empty() function . This function creates a font manager with no built-in fonts. The presence of this API (and the lack of a detailed changelog) hints that the default FontMgr::new() no longer returns any usable fonts in certain contexts. In a WASM build (where system fonts are disabled), FontMgr::new() will effectively be empty – hence the need for a “custom empty” font manager that you populate manually. The 0.91.0 release notes explicitly mention this addition, aligning with Skia’s removal of default fonts .

Origin of the Change: Upstream Skia decided to remove the global default SkFontMgr to avoid conflicts and give developers control over font loading (see Skia milestone 143). In practice, this means Skia no longer provides any fallback fonts out-of-the-box on platforms like WASM. The Rust-skia maintainers noted that “there were some changes with the default font/typeface (empty since 0.70.0)”, advising developers to be explicit about which fonts to use . Your upgrade to 0.91.0 hit this exact issue: the default font manager became empty, so without changes your custom fonts weren’t being found or used – resulting in only “notdef” glyphs being drawn.

Related Issues: This change has been observed in the community. For example, an issue report showed that trying to load fonts via FontMgr on WASM failed silently. The user found that FontMgr::new().new_from_data(...) always returned an error (no font) on WASM, whereas using Typeface::from_data(...) directly did work . In other words, creating a typeface from data was fine, but using the font manager’s method was not – because the font manager was essentially empty in that environment. This aligns with what you’re seeing after the upgrade.

How to Migrate (Avoiding “notdef” Glyphs)

To fix font rendering after 0.91.0, explicitly supply and use your custom fonts – you can no longer rely on any default font fallback. Here are steps you can take:
• Use a Typeface Provider: If you’re using Skia’s text layout (e.g. skia_safe::textlayout module), create a TypefaceFontProvider and register your font data with it (via register_typeface). Then use a FontCollection to set this provider as the default. Make sure to call font_collection.set_default_font_manager(FontMgr::custom_empty(), some_provider) or similar, so that the text shaper knows about your fonts and doesn’t try to use system fonts. This ensures your text runs through the fonts you provided and nothing else.
• Directly set Typefaces: If you are drawing text via SkPaint/Canvas::drawTextBlob (without the higher-level paragraph API), load your custom font with Typeface::from_data (or FontMgr::new_from_data if available) and then set that Typeface on your Font or Paint. For example, in skia-safe you might do let tf = Typeface::from_data(font_data, None).unwrap() and then paint.set_typeface(tf). This way you bypass the default font manager entirely and use your font directly. The Rust-skia maintainer specifically suggests using TextStyle::set_typeface() to avoid issues with empty defaults .
• Avoid relying on FontMgr::new(): In WASM builds, FontMgr::new() (which creates a system font manager) is effectively a no-op now – it returns an object with no fonts. Instead, use FontMgr::custom_empty()  or FontMgr::empty() and then populate it with fonts if needed. The new custom_empty was added precisely to let you create a font manager that you control. For instance, you could call let fm = FontMgr::custom_empty(); and then use Skia’s C++ API (via SkTypeface or font provider) to add fonts to that manager.

By updating your code to explicitly load and set your custom fonts, you should regain proper text rendering. In summary, the upgrade to 0.91.0 broke font rendering because Skia-safe stopped providing any default fonts – the change originates from Skia’s decision to remove the default font manager, captured in Rust-skia’s 0.91.0 release (see PR #1224) . After migrating to the new approach (using custom_empty font manager or setting typefaces directly), you’ll avoid the “notdef” issue and get your custom fonts rendering again.

References:
• Rust-skia 0.91.0 release notes – added FontMgr::custom_empty and updated to Skia m143 .
• Maintainer’s Q&A on font default being empty (no default fonts since 0.70) .
• Issue example (WASM): using FontMgr::new().new_from_data yielded no font; direct Typeface::from_data was required .

Summary by CodeRabbit

  • Chores

    • Upgraded skia-safe to 0.91.0 (dependency features expanded) and bumped wasm package version.
  • Refactor

    • Simplified and standardized internal path construction and transformation logic across the codebase for more consistent behavior.
  • New Features

    • Added paragraph_to_vector_network to produce vector networks from text.

✏️ Tip: You can customize this high-level summary in your review settings.

@vercel
Copy link
Copy Markdown

vercel Bot commented Dec 9, 2025

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

Project Deployment Review Updated (UTC)
backgrounds Ready Ready Preview, Comment Dec 14, 2025 9:14am
blog Ready Ready Preview, Comment Dec 14, 2025 9:14am
docs Ready Ready Preview, Comment Dec 14, 2025 9:14am
grida Error Error Dec 14, 2025 9:14am
viewer Ready Ready Preview, Comment Dec 14, 2025 9:14am
2 Skipped Deployments
Project Deployment Review Updated (UTC)
code Ignored Ignored Dec 14, 2025 9:14am
legacy Ignored Ignored Dec 14, 2025 9:14am

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Dec 9, 2025

Walkthrough

Upgrades skia-safe to 0.91.0 and consistently migrates path construction from mutable Path APIs to skia_safe::PathBuilder, replacing in-place transforms with functional make_transform and adding a new paragraph_to_vector_network helper.

Changes

Cohort / File(s) Summary
Dependency Manifests
crates/grida-canvas/Cargo.toml, crates/grida-dev/Cargo.toml, crates/grida-canvas-wasm/package.json
Bump skia-safe to 0.91.0 (features updated in grida-dev); package version bump for wasm crate.
Examples
crates/grida-canvas/examples/*
crates/grida-canvas/examples/golden_boolop.rs, .../golden_path_discrete.rs, .../golden_sk_mask.rs, .../golden_sk_paints.rs, .../golden_sk_paragraph_path_vector.rs, .../golden_sk_svg_filters/main.rs, .../golden_sk_text_backdrop_blur_path.rs
Replace manual Path construction with Path::rect/Path::circle and PathBuilder usage; remove mutable Path mutations.
Devtools & Overlays
crates/grida-canvas/src/devtools/*
.../hit_overlay.rs, .../ruler_overlay.rs, .../stroke_overlay.rs, .../text_overlay.rs
Swap path.transform(...) for path = path.make_transform(...); where building paths, use PathBuilder and detach().
Painter & Hit-testing
crates/grida-canvas/src/painter/*, crates/grida-canvas/src/hittest/hit_tester.rs
.../geometry.rs, .../layer.rs, .../painter.rs, .../text_stroke.rs, .../hit_tester.rs
Migrate glyph/mask/path composition to PathBuilder; apply make_transform instead of in-place transforms; adjust per-glyph offset handling and detach builders.
Shapes
crates/grida-canvas/src/shape/*
.../ellipse.rs, .../ellipse_ring.rs, .../ellipse_ring_sector.rs, .../ellipse_sector.rs, .../polygon.rs, .../rect.rs, .../rrect.rs, .../srrect_orthogonal.rs, .../stroke_rect.rs, .../stroke_varwidth.rs
Replace Path mutations with PathBuilder across shape builders; use direct constructors (Path::oval, Path::rect, Path::polygon) where applicable; update helper signature for catmull segments to accept &mut PathBuilder.
Core Path Utilities & Text
crates/grida-canvas/src/*
.../sk_tiny/mod.rs, .../svg/from_usvg_tree.rs, .../text/mod.rs, .../vectornetwork/vn.rs, .../window/application.rs
Use PathBuilder for segment assembly; change mutable path parameters to immutable where appropriate; add pub fn paragraph_to_vector_network(paragraph: &mut Paragraph) -> VectorNetwork.
Tests
crates/grida-canvas/tests/*
.../dashed_stroke.rs, .../open_path_stroke_align.rs, .../stroke_dash_width.rs
Update test path setup to build paths with PathBuilder and detach() instead of mutating Path.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Wide but homogeneous refactor: many files (~40+) changed with the same patterns (PathBuilder adoption, make_transform).
  • Review focus:
    • Confirm PathBuilder usage and detach() semantics match prior mutable-path behavior.
    • Verify make_transform yields equivalent transformed geometry in hit-testing, overlays, and painter flows.
    • Inspect stroke_varwidth::add_catmull_segments signature change and all updated call sites.
    • Check new public helper paragraph_to_vector_network for correctness and potential API stability.

Possibly related PRs

Suggested labels

canvas, wasm32, migration

Poem

🐰
I hop and stitch each curve anew,
Builders lay the pathways true.
From mutable mud to tidy art,
Skia’s new steps — a gentle start.
— your refactoring rabbit 🎨✨

Pre-merge checks and finishing touches

✅ 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 "skia-safe 0.91.0" directly and clearly identifies the primary change: upgrading the skia-safe dependency to version 0.91.0. The changeset extensively reflects this upgrade across multiple files, path building refactoring patterns, and dependency updates in Cargo.toml files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch skia-91

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4c9c6ae and b881549.

⛔ Files ignored due to path filters (1)
  • crates/grida-canvas-wasm/lib/bin/grida_canvas_wasm.wasm is excluded by !**/*.wasm
📒 Files selected for processing (2)
  • crates/grida-canvas-wasm/package.json (1 hunks)
  • crates/grida-dev/Cargo.toml (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • crates/grida-canvas-wasm/package.json
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:28.164Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `skia-safe` crate for all painting and rendering operations
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/*.rs : Run all tests with: `cargo test`
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T00:21:48.564Z
Learning: Use Skia as the graphics backend for 2D graphics bound with skia-safe
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Use Canvas 2D API with path commands for rendering geometric shapes (circle, square, triangle, etc.)
📚 Learning: 2025-12-01T00:22:28.164Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:28.164Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `skia-safe` crate for all painting and rendering operations

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/Cargo.toml : Use `ttf-parser = "0.25"` as the required dependency for font parsing backend

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:36.510Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-dev/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:36.510Z
Learning: Applies to crates/grida-dev/grida-dev/Cargo.toml : The `grida-dev` crate is marked `publish = false` and is intended solely for local development workflows and devtools

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/Cargo.toml : Include dev-dependency `serde_json = "1.0"` for JSON testing

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:28.164Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:28.164Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `math2` crate for all geometry and common math operations

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/*.rs : Run all tests with: `cargo test`

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/Cargo.toml : Include optional `serde = "1.0"` with `derive` feature for JSON serialization support

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/+(grida-canvas-wasm.js|grida-canvas-wasm.wasm) : Include WASM artifacts (`grida-canvas-wasm.js` and `grida-canvas-wasm.wasm`) in git for faster CI builds

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/Cargo.toml : Use the target `wasm32-unknown-emscripten` when building Rust code for WebAssembly compilation

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/serde_test.rs : Run tests with serde feature using: `cargo test --features serde`

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/Cargo.toml : Set environment variables `CC=emcc`, `CXX=em++`, and `AR=emar` when building Rust code for WebAssembly targets

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:36.510Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-dev/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:36.510Z
Learning: The `cg` crate must remain platform-agnostic; all winit/glutin integration code must live in the `grida-dev` crate

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:22:28.164Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:28.164Z
Learning: Applies to crates/grida-canvas/**/*.rs : All internal structs (NodeRecs, SceneGraph, caches) must use `NodeId` (u64) for high-performance counter-based IDs in the rendering engine

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:21:48.564Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T00:21:48.564Z
Learning: Applies to crates/**/*.rs : Use Rust 2024 edition for WASM builds and graphics core

Applied to files:

  • crates/grida-dev/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: cargo test
  • GitHub Check: test
🔇 Additional comments (1)
crates/grida-dev/Cargo.toml (1)

22-22: Feature alignment is consistent with grida-canvas.

The expanded feature set ["gpu", "gl", "textlayout", "pdf", "svg", "webp"] matches grida-canvas exactly, and the version bump to 0.91.0 aligns with the broader workspace update.


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.

@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

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: 8

Caution

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

⚠️ Outside diff range comments (4)
crates/grida-canvas/src/shape/srrect_orthogonal.rs (1)

87-209: Remove builder.arc_to() calls—this method does not exist on PathBuilder in skia-safe 0.91.0.

The migration to PathBuilder is incomplete. While move_to, line_to, and cubic_to exist on PathBuilder, arc_to is only available on the mutable Path type in skia-safe 0.91.0. Lines 126, 154, 177, and 199 use builder.arc_to(), which will fail to compile.

Revert this function to use mutable Path::new() or refactor the arc drawing to use an alternative approach compatible with PathBuilder (e.g., tessellating the arc into cubic Bézier curves).

crates/grida-canvas/examples/golden_boolop.rs (1)

73-82: Golden output directory creation is inconsistent with the write target path.

create_dir_all("goldens") depends on CWD, but the write uses CARGO_MANIFEST_DIR/goldens/...; this can fail when run from workspace root.

-    std::fs::create_dir_all("goldens").unwrap();
-    std::fs::write(
-        concat!(env!("CARGO_MANIFEST_DIR"), "/goldens/boolop.png"),
-        data.as_bytes(),
-    )
-    .unwrap();
+    let goldens_dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("goldens");
+    std::fs::create_dir_all(&goldens_dir).unwrap();
+    std::fs::write(goldens_dir.join("boolop.png"), data.as_bytes()).unwrap();
crates/grida-canvas/examples/golden_sk_text_backdrop_blur_path.rs (2)

160-167: Embedded font is not registered with the FontCollection—use TypefaceFontProvider instead.

The created Typeface is stored but never registered. To make the embedded Geist font available to text layout, create a TypefaceFontProvider, call register_typeface() with the created typeface, and attach that provider to the FontCollection. The current pattern of setting a default font manager without explicit typeface registration will not resolve the font family name, especially in WASM where system fonts are unavailable.

let provider = TypefaceFontProvider::new();
let typeface = font_mgr
    .new_from_data(&Data::new_copy(cg::fonts::embedded::geist::BYTES), None)
    .expect("Failed to create typeface");
provider.register_typeface(&typeface);
font_collection.set_asset_font_manager(Some(provider));

180-216: Fix non-existent API calls and mutable reference requirements in paragraph_to_path function.

The code contains critical compilation errors that go beyond API semantic concerns:

  1. Line 200: builder.add_path_with_transform() does not exist in skia-safe 0.91.0. The correct API is Path::add_path_matrix(src: &Path, matrix: &Matrix, mode: impl Into<Option<AddPathMode>>). You need to call this on the Path object, not PathBuilder.

  2. Line 213: path.make_transform(&transform) is called on an immutable reference, but the method signature requires &mut self. The path must be declared as mutable.

The double-transform logic (combining glyph positions + origin at line 199, then applying final (x, y) offset at line 212) is correct in intent but requires the API fixes above.

🧹 Nitpick comments (10)
crates/grida-canvas/examples/golden_sk_paragraph_path_vector.rs (1)

509-516: Tighten readability (and avoid a long single line) around per-glyph transforms.
This is fine functionally, but splitting the matrix creation improves clarity and makes it easier to tweak later (e.g., if you need additional transforms).

                 if let Some(glyph_path) = font.get_path(*glyph) {
                     let offset = Point::new(
                         pos.x + run_origin.x + origin.x,
                         pos.y + run_origin.y + origin.y,
                     );
-                    builder.add_path_with_transform(&glyph_path, &Matrix::translate((offset.x, offset.y)), None);
+                    let m = Matrix::translate((offset.x, offset.y));
+                    builder.add_path_with_transform(&glyph_path, &m, None);
                 }
crates/grida-canvas/src/shape/ellipse_ring.rs (1)

13-35: Make outer oval direction explicit + validate inner_radius_ratio

To avoid relying on library defaults (and to match the “clockwise” comment), I’d suggest being explicit about winding, and optionally clamping or asserting the documented range:

 pub fn build_ring_path(shape: &EllipticalRingShape) -> skia_safe::Path {
     let mut builder = skia_safe::PathBuilder::new();
+    debug_assert!((0.0..=1.0).contains(&shape.inner_radius_ratio));

     // ...
-    builder.add_oval(outer_rect, None, None);
+    builder.add_oval(outer_rect, Some(skia_safe::PathDirection::CW), Some(0));
crates/grida-canvas/src/shape/ellipse_ring_sector.rs (1)

61-98: Builder conversion seems faithful; please sanity-check arc_to semantics with negative sweep

The migration reads correct (move_to → arc_to → inner path → close → detach), but because correctness hinges on Skia’s arc_to behavior—especially -sweep_deg for the inner arc—I’d recommend confirming output parity (CW/CCW & hole fill) with a quick render/golden test for a few angles (e.g., 90°, 270°, negative sweep).

Also applies to: 109-170, 181-269

crates/grida-canvas/src/painter/text_stroke.rs (1)

27-42: Verify PathBuilder::add_path API/semantics; consider avoiding per-glyph make_transform if builder supports transforms

Main thing to verify is that builder.add_path(&path) is the intended 0.91.0 API and preserves the previous “append” behavior (previously you had AddPathMode in play). If PathBuilder supports adding with an offset/matrix directly, that’d likely be preferable to allocating a transformed Path per glyph.

crates/grida-canvas/src/shape/stroke_rect.rs (2)

263-277: Solid-stroke ring creation via builder-detach is OK; consider de-duplicating the rrect→path boilerplate.

A small local helper like fn path_from_rrect(rr: RRect) -> Path would remove repeated builder setup and reduce churn in future edits.


356-373: Clipping ring paths rebuilt via PathBuilder is correct; same de-duplication suggestion applies here.

crates/grida-canvas/src/svg/from_usvg_tree.rs (1)

220-228: Immutability change is good; avoid shadowing path for readability.

Consider:

-        let path = path.make_offset((-bounds.x, -bounds.y));
-        let data = path.to_svg();
+        let offset_path = path.make_offset((-bounds.x, -bounds.y));
+        let data = offset_path.to_svg();
crates/grida-canvas/src/painter/painter.rs (2)

284-306: Backdrop-blur glyph path accumulation looks correct; watch for per-glyph allocation costs.

If skia-safe 0.91 exposes an “add path with matrix/offset” API on PathBuilder, consider using it to avoid glyph_path.make_transform(...) per glyph.


956-974: Mask-path recursion works, but detach()/new_path() in the loop may be costly.

If this shows up in profiles, a common pattern is: recurse into a fresh Path, then builder.add_path(&nested) (instead of rebuilding the builder from a detached path each time).

crates/grida-canvas/src/shape/stroke_varwidth.rs (1)

204-207: Optimize path concatenation to avoid repeated builder reconstructions in the segment loop.

Each iteration rebuilds a new PathBuilder (new_path + add_path + detach) when concatenating segment paths. For loops with many segments, consider creating the builder once on the second segment and appending all remaining paths before a single final detach() to avoid repeated path structure copying.

The suggested diff (keeping the first segment as Path, then building once) is a reasonable minimal-churn approach. If feasible, a persistent builder across all iterations would eliminate repeated reconstructions entirely.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0cf34f7 and 4c9c6ae.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (36)
  • crates/grida-canvas/Cargo.toml (1 hunks)
  • crates/grida-canvas/examples/golden_boolop.rs (3 hunks)
  • crates/grida-canvas/examples/golden_path_discrete.rs (1 hunks)
  • crates/grida-canvas/examples/golden_sk_mask.rs (3 hunks)
  • crates/grida-canvas/examples/golden_sk_paints.rs (1 hunks)
  • crates/grida-canvas/examples/golden_sk_paragraph_path_vector.rs (3 hunks)
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs (1 hunks)
  • crates/grida-canvas/examples/golden_sk_text_backdrop_blur_path.rs (3 hunks)
  • crates/grida-canvas/src/devtools/hit_overlay.rs (1 hunks)
  • crates/grida-canvas/src/devtools/ruler_overlay.rs (3 hunks)
  • crates/grida-canvas/src/devtools/stroke_overlay.rs (1 hunks)
  • crates/grida-canvas/src/devtools/text_overlay.rs (2 hunks)
  • crates/grida-canvas/src/hittest/hit_tester.rs (3 hunks)
  • crates/grida-canvas/src/painter/geometry.rs (3 hunks)
  • crates/grida-canvas/src/painter/layer.rs (2 hunks)
  • crates/grida-canvas/src/painter/painter.rs (4 hunks)
  • crates/grida-canvas/src/painter/text_stroke.rs (2 hunks)
  • crates/grida-canvas/src/shape/ellipse.rs (1 hunks)
  • crates/grida-canvas/src/shape/ellipse_ring.rs (2 hunks)
  • crates/grida-canvas/src/shape/ellipse_ring_sector.rs (10 hunks)
  • crates/grida-canvas/src/shape/ellipse_sector.rs (3 hunks)
  • crates/grida-canvas/src/shape/polygon.rs (1 hunks)
  • crates/grida-canvas/src/shape/rect.rs (1 hunks)
  • crates/grida-canvas/src/shape/rrect.rs (1 hunks)
  • crates/grida-canvas/src/shape/srrect_orthogonal.rs (6 hunks)
  • crates/grida-canvas/src/shape/stroke_rect.rs (4 hunks)
  • crates/grida-canvas/src/shape/stroke_varwidth.rs (5 hunks)
  • crates/grida-canvas/src/sk_tiny/mod.rs (1 hunks)
  • crates/grida-canvas/src/svg/from_usvg_tree.rs (1 hunks)
  • crates/grida-canvas/src/text/mod.rs (2 hunks)
  • crates/grida-canvas/src/vectornetwork/vn.rs (6 hunks)
  • crates/grida-canvas/src/window/application.rs (1 hunks)
  • crates/grida-canvas/tests/dashed_stroke.rs (1 hunks)
  • crates/grida-canvas/tests/open_path_stroke_align.rs (1 hunks)
  • crates/grida-canvas/tests/stroke_dash_width.rs (1 hunks)
  • crates/grida-dev/Cargo.toml (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
crates/**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

Use Rust 2024 edition for WASM builds and graphics core

Files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-canvas/src/shape/ellipse_ring.rs
  • crates/grida-canvas/src/devtools/text_overlay.rs
  • crates/grida-canvas/src/text/mod.rs
  • crates/grida-canvas/src/painter/geometry.rs
  • crates/grida-canvas/src/shape/polygon.rs
  • crates/grida-canvas/src/devtools/hit_overlay.rs
  • crates/grida-canvas/src/shape/rrect.rs
  • crates/grida-canvas/src/shape/srrect_orthogonal.rs
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/src/vectornetwork/vn.rs
  • crates/grida-canvas/src/shape/rect.rs
  • crates/grida-canvas/examples/golden_path_discrete.rs
  • crates/grida-canvas/src/devtools/stroke_overlay.rs
  • crates/grida-canvas/src/shape/ellipse_ring_sector.rs
  • crates/grida-canvas/src/painter/painter.rs
  • crates/grida-canvas/src/svg/from_usvg_tree.rs
  • crates/grida-canvas/tests/dashed_stroke.rs
  • crates/grida-canvas/examples/golden_sk_paints.rs
  • crates/grida-canvas/examples/golden_sk_text_backdrop_blur_path.rs
  • crates/grida-canvas/src/shape/stroke_rect.rs
  • crates/grida-canvas/src/painter/text_stroke.rs
  • crates/grida-canvas/src/shape/ellipse.rs
  • crates/grida-canvas/examples/golden_sk_mask.rs
  • crates/grida-canvas/src/sk_tiny/mod.rs
  • crates/grida-canvas/src/window/application.rs
  • crates/grida-canvas/src/shape/ellipse_sector.rs
  • crates/grida-canvas/src/shape/stroke_varwidth.rs
  • crates/grida-canvas/examples/golden_boolop.rs
  • crates/grida-canvas/src/hittest/hit_tester.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
  • crates/grida-canvas/src/painter/layer.rs
  • crates/grida-canvas/tests/open_path_stroke_align.rs
  • crates/grida-canvas/examples/golden_sk_paragraph_path_vector.rs
crates/grida-canvas/**/*.rs

📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)

crates/grida-canvas/**/*.rs: All internal structs (NodeRecs, SceneGraph, caches) must use NodeId (u64) for high-performance counter-based IDs in the rendering engine
Public APIs must accept and return UserNodeId (String) instead of NodeId for stability and serialization
Use IdConverter to handle conversion between NodeId and UserNodeId during .grida file loading
NodeRepository must auto-generate IDs for factory-created nodes, assigning ID=0 as the default
Use skia-safe crate for all painting and rendering operations
Use math2 crate for all geometry and common math operations

Files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-canvas/src/shape/ellipse_ring.rs
  • crates/grida-canvas/src/devtools/text_overlay.rs
  • crates/grida-canvas/src/text/mod.rs
  • crates/grida-canvas/src/painter/geometry.rs
  • crates/grida-canvas/src/shape/polygon.rs
  • crates/grida-canvas/src/devtools/hit_overlay.rs
  • crates/grida-canvas/src/shape/rrect.rs
  • crates/grida-canvas/src/shape/srrect_orthogonal.rs
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/src/vectornetwork/vn.rs
  • crates/grida-canvas/src/shape/rect.rs
  • crates/grida-canvas/examples/golden_path_discrete.rs
  • crates/grida-canvas/src/devtools/stroke_overlay.rs
  • crates/grida-canvas/src/shape/ellipse_ring_sector.rs
  • crates/grida-canvas/src/painter/painter.rs
  • crates/grida-canvas/src/svg/from_usvg_tree.rs
  • crates/grida-canvas/tests/dashed_stroke.rs
  • crates/grida-canvas/examples/golden_sk_paints.rs
  • crates/grida-canvas/examples/golden_sk_text_backdrop_blur_path.rs
  • crates/grida-canvas/src/shape/stroke_rect.rs
  • crates/grida-canvas/src/painter/text_stroke.rs
  • crates/grida-canvas/src/shape/ellipse.rs
  • crates/grida-canvas/examples/golden_sk_mask.rs
  • crates/grida-canvas/src/sk_tiny/mod.rs
  • crates/grida-canvas/src/window/application.rs
  • crates/grida-canvas/src/shape/ellipse_sector.rs
  • crates/grida-canvas/src/shape/stroke_varwidth.rs
  • crates/grida-canvas/examples/golden_boolop.rs
  • crates/grida-canvas/src/hittest/hit_tester.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
  • crates/grida-canvas/src/painter/layer.rs
  • crates/grida-canvas/tests/open_path_stroke_align.rs
  • crates/grida-canvas/examples/golden_sk_paragraph_path_vector.rs
🧠 Learnings (24)
📓 Common learnings
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:28.164Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `skia-safe` crate for all painting and rendering operations
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/*.rs : Run all tests with: `cargo test`
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T00:21:48.564Z
Learning: Use Skia as the graphics backend for 2D graphics bound with skia-safe
📚 Learning: 2025-12-01T00:22:28.164Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:28.164Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `skia-safe` crate for all painting and rendering operations

Applied to files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-canvas/src/shape/ellipse_ring.rs
  • crates/grida-dev/Cargo.toml
  • crates/grida-canvas/src/devtools/text_overlay.rs
  • crates/grida-canvas/src/text/mod.rs
  • crates/grida-canvas/src/painter/geometry.rs
  • crates/grida-canvas/src/shape/polygon.rs
  • crates/grida-canvas/src/devtools/hit_overlay.rs
  • crates/grida-canvas/src/shape/rrect.rs
  • crates/grida-canvas/src/shape/srrect_orthogonal.rs
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/src/vectornetwork/vn.rs
  • crates/grida-canvas/src/shape/rect.rs
  • crates/grida-canvas/examples/golden_path_discrete.rs
  • crates/grida-canvas/src/devtools/stroke_overlay.rs
  • crates/grida-canvas/src/shape/ellipse_ring_sector.rs
  • crates/grida-canvas/src/painter/painter.rs
  • crates/grida-canvas/src/svg/from_usvg_tree.rs
  • crates/grida-canvas/Cargo.toml
  • crates/grida-canvas/tests/dashed_stroke.rs
  • crates/grida-canvas/examples/golden_sk_paints.rs
  • crates/grida-canvas/examples/golden_sk_text_backdrop_blur_path.rs
  • crates/grida-canvas/src/shape/stroke_rect.rs
  • crates/grida-canvas/src/painter/text_stroke.rs
  • crates/grida-canvas/src/shape/ellipse.rs
  • crates/grida-canvas/examples/golden_sk_mask.rs
  • crates/grida-canvas/src/sk_tiny/mod.rs
  • crates/grida-canvas/src/window/application.rs
  • crates/grida-canvas/src/shape/ellipse_sector.rs
  • crates/grida-canvas/src/shape/stroke_varwidth.rs
  • crates/grida-canvas/examples/golden_boolop.rs
  • crates/grida-canvas/src/hittest/hit_tester.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
  • crates/grida-canvas/src/painter/layer.rs
  • crates/grida-canvas/tests/open_path_stroke_align.rs
  • crates/grida-canvas/examples/golden_sk_paragraph_path_vector.rs
📚 Learning: 2025-12-01T00:22:28.164Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:28.164Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `math2` crate for all geometry and common math operations

Applied to files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-canvas/src/shape/ellipse_ring.rs
  • crates/grida-dev/Cargo.toml
  • crates/grida-canvas/src/devtools/text_overlay.rs
  • crates/grida-canvas/src/text/mod.rs
  • crates/grida-canvas/src/painter/geometry.rs
  • crates/grida-canvas/src/shape/polygon.rs
  • crates/grida-canvas/src/devtools/hit_overlay.rs
  • crates/grida-canvas/src/shape/rrect.rs
  • crates/grida-canvas/src/shape/srrect_orthogonal.rs
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/src/vectornetwork/vn.rs
  • crates/grida-canvas/src/shape/rect.rs
  • crates/grida-canvas/examples/golden_path_discrete.rs
  • crates/grida-canvas/src/devtools/stroke_overlay.rs
  • crates/grida-canvas/src/shape/ellipse_ring_sector.rs
  • crates/grida-canvas/src/painter/painter.rs
  • crates/grida-canvas/Cargo.toml
  • crates/grida-canvas/tests/dashed_stroke.rs
  • crates/grida-canvas/examples/golden_sk_paints.rs
  • crates/grida-canvas/examples/golden_sk_text_backdrop_blur_path.rs
  • crates/grida-canvas/src/shape/stroke_rect.rs
  • crates/grida-canvas/src/painter/text_stroke.rs
  • crates/grida-canvas/src/shape/ellipse.rs
  • crates/grida-canvas/examples/golden_sk_mask.rs
  • crates/grida-canvas/src/sk_tiny/mod.rs
  • crates/grida-canvas/src/window/application.rs
  • crates/grida-canvas/src/shape/ellipse_sector.rs
  • crates/grida-canvas/src/shape/stroke_varwidth.rs
  • crates/grida-canvas/examples/golden_boolop.rs
  • crates/grida-canvas/src/hittest/hit_tester.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
  • crates/grida-canvas/src/painter/layer.rs
  • crates/grida-canvas/tests/open_path_stroke_align.rs
📚 Learning: 2025-12-01T00:22:28.164Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:28.164Z
Learning: Applies to crates/grida-canvas/**/*.rs : Public APIs must accept and return `UserNodeId` (String) instead of `NodeId` for stability and serialization

Applied to files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-canvas/src/shape/ellipse_ring.rs
  • crates/grida-dev/Cargo.toml
  • crates/grida-canvas/src/devtools/text_overlay.rs
  • crates/grida-canvas/src/text/mod.rs
  • crates/grida-canvas/src/painter/geometry.rs
  • crates/grida-canvas/src/shape/polygon.rs
  • crates/grida-canvas/src/devtools/hit_overlay.rs
  • crates/grida-canvas/src/shape/srrect_orthogonal.rs
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/src/vectornetwork/vn.rs
  • crates/grida-canvas/src/devtools/stroke_overlay.rs
  • crates/grida-canvas/src/shape/ellipse_ring_sector.rs
  • crates/grida-canvas/src/painter/painter.rs
  • crates/grida-canvas/src/svg/from_usvg_tree.rs
  • crates/grida-canvas/tests/dashed_stroke.rs
  • crates/grida-canvas/examples/golden_sk_paints.rs
  • crates/grida-canvas/src/painter/text_stroke.rs
  • crates/grida-canvas/src/sk_tiny/mod.rs
  • crates/grida-canvas/src/window/application.rs
  • crates/grida-canvas/src/shape/ellipse_sector.rs
  • crates/grida-canvas/src/shape/stroke_varwidth.rs
  • crates/grida-canvas/src/hittest/hit_tester.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
  • crates/grida-canvas/src/painter/layer.rs
  • crates/grida-canvas/tests/open_path_stroke_align.rs
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/ui_parser_test.rs : High-level UI API tests should use `parse_ui` and be organized in `ui_parser_test.rs`

Applied to files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-canvas/src/text/mod.rs
  • crates/grida-canvas/src/devtools/hit_overlay.rs
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/src/devtools/stroke_overlay.rs
  • crates/grida-canvas/src/painter/painter.rs
  • crates/grida-canvas/tests/dashed_stroke.rs
  • crates/grida-canvas/examples/golden_sk_text_backdrop_blur_path.rs
  • crates/grida-canvas/src/hittest/hit_tester.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
  • crates/grida-canvas/tests/open_path_stroke_align.rs
📚 Learning: 2025-12-01T00:22:28.164Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:28.164Z
Learning: Applies to crates/grida-canvas/**/*.rs : All internal structs (NodeRecs, SceneGraph, caches) must use `NodeId` (u64) for high-performance counter-based IDs in the rendering engine

Applied to files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-canvas/src/shape/ellipse_ring.rs
  • crates/grida-canvas/src/painter/geometry.rs
  • crates/grida-canvas/src/devtools/hit_overlay.rs
  • crates/grida-canvas/src/shape/srrect_orthogonal.rs
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/src/vectornetwork/vn.rs
  • crates/grida-canvas/src/shape/rect.rs
  • crates/grida-canvas/src/devtools/stroke_overlay.rs
  • crates/grida-canvas/src/shape/ellipse_ring_sector.rs
  • crates/grida-canvas/src/painter/painter.rs
  • crates/grida-canvas/tests/dashed_stroke.rs
  • crates/grida-canvas/examples/golden_sk_paints.rs
  • crates/grida-canvas/src/shape/stroke_rect.rs
  • crates/grida-canvas/examples/golden_sk_mask.rs
  • crates/grida-canvas/src/sk_tiny/mod.rs
  • crates/grida-canvas/src/window/application.rs
  • crates/grida-canvas/examples/golden_boolop.rs
  • crates/grida-canvas/src/hittest/hit_tester.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
  • crates/grida-canvas/src/painter/layer.rs
  • crates/grida-canvas/tests/open_path_stroke_align.rs
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/*.rs : Run all tests with: `cargo test`

Applied to files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-dev/Cargo.toml
  • crates/grida-canvas/Cargo.toml
  • crates/grida-canvas/tests/dashed_stroke.rs
  • crates/grida-canvas/examples/golden_sk_text_backdrop_blur_path.rs
  • crates/grida-canvas/tests/open_path_stroke_align.rs
📚 Learning: 2025-12-01T00:22:28.164Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:28.164Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `IdConverter` to handle conversion between `NodeId` and `UserNodeId` during .grida file loading

Applied to files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-canvas/src/painter/geometry.rs
  • crates/grida-canvas/src/devtools/hit_overlay.rs
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/src/vectornetwork/vn.rs
  • crates/grida-canvas/src/devtools/stroke_overlay.rs
  • crates/grida-canvas/src/shape/ellipse_ring_sector.rs
  • crates/grida-canvas/src/painter/painter.rs
  • crates/grida-canvas/tests/dashed_stroke.rs
  • crates/grida-canvas/src/sk_tiny/mod.rs
  • crates/grida-canvas/src/window/application.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/main.rs : Update `grida-canvas-wasm.d.ts` TypeScript definitions file when new APIs are introduced via `main.rs`

Applied to files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-canvas/src/shape/ellipse_ring.rs
  • crates/grida-dev/Cargo.toml
  • crates/grida-canvas/src/devtools/hit_overlay.rs
  • crates/grida-canvas/src/shape/srrect_orthogonal.rs
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/src/shape/rect.rs
  • crates/grida-canvas/src/devtools/stroke_overlay.rs
  • crates/grida-canvas/src/painter/painter.rs
  • crates/grida-canvas/Cargo.toml
  • crates/grida-canvas/tests/dashed_stroke.rs
  • crates/grida-canvas/src/shape/ellipse.rs
  • crates/grida-canvas/examples/golden_sk_mask.rs
  • crates/grida-canvas/src/sk_tiny/mod.rs
  • crates/grida-canvas/src/window/application.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
  • crates/grida-canvas/src/painter/layer.rs
  • crates/grida-canvas/tests/open_path_stroke_align.rs
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : Use Canvas 2D API with path commands for rendering geometric shapes (circle, square, triangle, etc.)

Applied to files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-canvas/src/painter/geometry.rs
  • crates/grida-canvas/src/devtools/hit_overlay.rs
  • crates/grida-canvas/src/shape/srrect_orthogonal.rs
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/src/vectornetwork/vn.rs
  • crates/grida-canvas/src/devtools/stroke_overlay.rs
  • crates/grida-canvas/src/painter/painter.rs
  • crates/grida-canvas/tests/dashed_stroke.rs
  • crates/grida-canvas/examples/golden_sk_paints.rs
  • crates/grida-canvas/examples/golden_sk_text_backdrop_blur_path.rs
  • crates/grida-canvas/src/painter/text_stroke.rs
  • crates/grida-canvas/examples/golden_sk_mask.rs
  • crates/grida-canvas/src/shape/ellipse_sector.rs
  • crates/grida-canvas/examples/golden_boolop.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
📚 Learning: 2025-12-01T00:22:28.164Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:28.164Z
Learning: Applies to crates/grida-canvas/**/*.rs : NodeRepository must auto-generate IDs for factory-created nodes, assigning ID=0 as the default

Applied to files:

  • crates/grida-canvas/tests/stroke_dash_width.rs
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/tests/dashed_stroke.rs
📚 Learning: 2025-12-01T00:22:36.510Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-dev/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:36.510Z
Learning: Applies to crates/grida-dev/grida-dev/Cargo.toml : The `grida-dev` crate is marked `publish = false` and is intended solely for local development workflows and devtools

Applied to files:

  • crates/grida-dev/Cargo.toml
  • crates/grida-canvas/Cargo.toml
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/Cargo.toml : Use `ttf-parser = "0.25"` as the required dependency for font parsing backend

Applied to files:

  • crates/grida-dev/Cargo.toml
  • crates/grida-canvas/Cargo.toml
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/Cargo.toml : Include dev-dependency `serde_json = "1.0"` for JSON testing

Applied to files:

  • crates/grida-dev/Cargo.toml
  • crates/grida-canvas/Cargo.toml
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/Cargo.toml : Include optional `serde = "1.0"` with `derive` feature for JSON serialization support

Applied to files:

  • crates/grida-dev/Cargo.toml
  • crates/grida-canvas/Cargo.toml
📚 Learning: 2025-12-01T00:22:36.510Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-dev/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:36.510Z
Learning: The `cg` crate must remain platform-agnostic; all winit/glutin integration code must live in the `grida-dev` crate

Applied to files:

  • crates/grida-dev/Cargo.toml
📚 Learning: 2025-12-01T00:21:48.564Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T00:21:48.564Z
Learning: Applies to crates/**/*.rs : Use Rust 2024 edition for WASM builds and graphics core

Applied to files:

  • crates/grida-dev/Cargo.toml
  • crates/grida-canvas/src/devtools/ruler_overlay.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
📚 Learning: 2025-12-01T00:22:56.899Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: editor/app/(tools)/tools/halftone/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:56.899Z
Learning: Applies to editor/app/(tools)/tools/halftone/app/(tools)/tools/halftone/_page.tsx : For SVG export, convert circles to <circle> elements, rectangles to <rect> elements, and polygons to <polygon> elements with calculated points

Applied to files:

  • crates/grida-canvas/src/devtools/stroke_overlay.rs
📚 Learning: 2025-12-01T00:21:48.564Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-01T00:21:48.564Z
Learning: Use Skia as the graphics backend for 2D graphics bound with skia-safe

Applied to files:

  • crates/grida-canvas/src/painter/painter.rs
  • crates/grida-canvas/Cargo.toml
  • crates/grida-canvas/examples/golden_sk_text_backdrop_blur_path.rs
  • crates/grida-canvas/src/sk_tiny/mod.rs
  • crates/grida-canvas/examples/golden_boolop.rs
  • crates/grida-canvas/examples/golden_sk_svg_filters/main.rs
  • crates/grida-canvas/examples/golden_sk_paragraph_path_vector.rs
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/+(grida-canvas-wasm.js|grida-canvas-wasm.wasm) : Include WASM artifacts (`grida-canvas-wasm.js` and `grida-canvas-wasm.wasm`) in git for faster CI builds

Applied to files:

  • crates/grida-canvas/Cargo.toml
📚 Learning: 2025-12-01T00:22:19.083Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:19.083Z
Learning: Applies to crates/grida-canvas-wasm/**/Cargo.toml : Use the target `wasm32-unknown-emscripten` when building Rust code for WebAssembly compilation

Applied to files:

  • crates/grida-canvas/Cargo.toml
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/scenario_*.rs : Scenario-specific comprehensive tests should be organized in files matching `scenario_*.rs`

Applied to files:

  • crates/grida-canvas/tests/open_path_stroke_align.rs
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/serde_test.rs : Run tests with serde feature using: `cargo test --features serde`

Applied to files:

  • crates/grida-canvas/tests/open_path_stroke_align.rs
📚 Learning: 2025-12-01T00:22:06.800Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas-fonts/AGENTS.md:0-0
Timestamp: 2025-12-01T00:22:06.800Z
Learning: Applies to crates/grida-canvas-fonts/**/italic_level1.rs : Core font selection and italic detection tests should be organized in `italic_level1.rs`

Applied to files:

  • crates/grida-canvas/tests/open_path_stroke_align.rs
🧬 Code graph analysis (11)
crates/grida-canvas/src/painter/geometry.rs (1)
crates/grida-canvas/src/sk/mod.rs (1)
  • sk_matrix (3-6)
crates/grida-canvas/src/devtools/hit_overlay.rs (3)
third_party/usvg/src/tree/text.rs (1)
  • path (436-438)
crates/grida-canvas/src/sk/mod.rs (1)
  • sk_matrix (3-6)
crates/grida-canvas/src/painter/layer.rs (2)
  • transform (101-101)
  • transform (122-128)
crates/grida-canvas/src/shape/rrect.rs (2)
crates/grida-canvas/src/shape/stroke_rect.rs (1)
  • build_rrect (7-17)
crates/grida-canvas/src/painter/layer.rs (2)
  • shape (102-102)
  • shape (130-136)
crates/grida-canvas/src/devtools/stroke_overlay.rs (1)
crates/grida-canvas/src/sk/mod.rs (1)
  • sk_matrix (3-6)
crates/grida-canvas/src/painter/painter.rs (1)
crates/grida-canvas/src/sk/mod.rs (1)
  • sk_matrix (3-6)
crates/grida-canvas/examples/golden_sk_paints.rs (1)
crates/grida-canvas/src/node/schema.rs (9)
  • rect (867-867)
  • rect (929-936)
  • rect (1109-1116)
  • rect (1266-1273)
  • rect (1398-1405)
  • rect (1606-1623)
  • rect (1686-1688)
  • rect (1800-1807)
  • rect (1923-1930)
crates/grida-canvas/src/sk_tiny/mod.rs (1)
third_party/usvg/src/tree/text.rs (1)
  • path (436-438)
crates/grida-canvas/src/window/application.rs (2)
crates/math2/src/transform.rs (1)
  • translate (168-171)
crates/grida-canvas/src/runtime/camera.rs (1)
  • translate (112-116)
crates/grida-canvas/examples/golden_boolop.rs (1)
.ref/canvaskit/canvaskit.d.ts (1)
  • PathOp (5010-5010)
crates/grida-canvas/src/hittest/hit_tester.rs (3)
third_party/usvg/src/tree/text.rs (1)
  • path (436-438)
crates/grida-canvas/src/sk/mod.rs (1)
  • sk_matrix (3-6)
crates/grida-canvas/src/painter/layer.rs (2)
  • transform (101-101)
  • transform (122-128)
crates/grida-canvas/src/painter/layer.rs (2)
third_party/usvg/src/tree/text.rs (1)
  • path (436-438)
crates/grida-canvas/src/sk/mod.rs (1)
  • sk_matrix (3-6)
🔇 Additional comments (39)
crates/grida-canvas/examples/golden_sk_svg_filters/main.rs (1)

1975-1996: LGTM! Clean migration to PathBuilder pattern.

The refactor correctly adopts the PathBuilder pattern for skia-safe 0.91.0. The implementation properly:

  • Initializes the builder
  • Maps all TinyPath segments to corresponding builder methods
  • Finalizes with detach() to produce the immutable Path

This aligns with the PR objectives to update path building practices.

Based on learnings, the skia-safe crate is used for all painting and rendering operations in this codebase.

crates/grida-canvas/examples/golden_sk_mask.rs (4)

3-4: LGTM! Import changes support PathBuilder migration.

The imports correctly add PathBuilder for the new path construction pattern and maintain TileMode for the gradient shaders.


41-48: Excellent PathBuilder pattern usage.

The diagonal band path construction correctly adopts the PathBuilder pattern: creating a builder, chaining operations, closing, and detaching to obtain an immutable Path. This is the idiomatic approach for skia-safe 0.91.0.


54-58: Clean circular path construction.

Using Path::circle is the idiomatic way to create circular paths in skia-safe 0.91.0, eliminating the need for manual path building for simple geometric shapes.


69-73: Consistent circular path construction for outline.

The outline path correctly uses the same Path::circle pattern and geometry as the clip path, ensuring visual consistency between the clipped region and its outline.

crates/grida-canvas/examples/golden_sk_paints.rs (1)

368-376: No action needed—code is correct as-is.

Path::rect(rect, None) is the idiomatic pattern throughout the codebase (used consistently in geometry.rs, shape/rect.rs, and other examples). The recent commit "update examples path building" confirms this is an intentional alignment with production code. No behavior drift or winding/direction concerns exist.

crates/grida-canvas/src/shape/rrect.rs (1)

38-40: LGTM! Simplified path construction aligns with the migration pattern.

The direct use of Path::rrect() is more concise and idiomatic than the previous mutable approach. The API is supported in skia-safe 0.91.0 with the signature pub fn rrect(rect: impl AsRef<RRect>, dir: impl Into<Option<PathDirection>>) -> Self, and your usage with None for the optional direction parameter is correct.

crates/grida-canvas/examples/golden_sk_paragraph_path_vector.rs (1)

501-525: No API correctness issues found — this code correctly implements the expected skia-safe 0.91.0 semantics.

The Paragraph::visit callback signature FnMut(usize, Option<&'a VisitorInfo>) matches the usage in the code (|_, run|). The None parameter in add_path_with_transform means "no additional transform" (identity), which is correct—the offset positioning is already applied via Matrix::translate before the None is passed, so no winding or path behavior is affected. The implementation is idiomatic and sound.

Likely an incorrect or invalid review comment.

crates/grida-canvas/src/shape/polygon.rs (1)

15-18: No changes required. build_path_from_points relies on Path::polygon, which safely handles all point counts (0, 1, 2, and 3+ points) without panicking or exhibiting undefined behavior. All in-tree callers already enforce the n >= 3 precondition through assertions in build_simple_polygon_path and build_simple_polygon_vector_network. No external callers exist in the codebase, eliminating the theoretical risk of unvalidated input from public consumers.

crates/grida-canvas/src/devtools/ruler_overlay.rs (3)

3-3: LGTM! PathBuilder import added for 0.91.0 migration.

The import correctly adds PathBuilder to support the new path construction pattern.


94-94: Correct path finalization.

Using builder.detach() correctly finalizes the PathBuilder and produces the immutable Path for caching and rendering.


67-89: PathBuilder pattern correctly applied.

The migration from direct Path construction to PathBuilder follows the correct pattern: create builder, accumulate path operations via move_to/line_to, then finalize with detach(). The logic for constructing vertical and horizontal grid lines is preserved.

Since this is a major version upgrade (0.89.0 → 0.91.0), verify that the PathBuilder API usage aligns with the skia-safe 0.91.0 documentation:

skia-safe 0.91.0 PathBuilder API documentation
crates/grida-canvas/src/painter/layer.rs (1)

1073-1076: The original concern about Path::make_transform returning Option<Path> in skia-safe 0.91.0 is unfounded. The method signature is pub fn make_transform(&mut self, m: &Matrix, pc: impl Into<Option<ApplyPerspectiveClip>>) -> Path, returning an owned Path value directly. The code at lines 1075 and 1092 correctly assigns the return value without optional handling. No changes needed.

crates/grida-canvas/src/shape/ellipse_sector.rs (1)

1-48: PathBuilder migration is correct; arc_to signature verified.

The arc_to(oval, start_angle, sweep_angle, force_move_to) call is properly implemented:

  • outer_rect correctly bounds the ellipse
  • start_deg and sweep_deg use standard Skia angles (0° = +X axis, positive = clockwise)
  • force_move_to=false is the right choice since the path is non-empty (after move_to and line_to to the start point); this appends a line connecting the current point to the arc start, which is the intended behavior for a sector

No changes needed.

crates/grida-canvas/examples/golden_path_discrete.rs (1)

5-5: Good simplification; Path::circle(..., None) correctly uses the default winding direction.

The third parameter accepts PathDirection (CW or CCW) to control the circle's winding direction. Passing None applies the default behavior, preserving the expected semantics from any prior implementation.

crates/grida-canvas/src/shape/ellipse.rs (1)

8-13: None for the direction parameter is correct and uses Skia's default clockwise direction.

In skia-safe 0.91.0, Path::oval(rect, dir) accepts an optional PathDirection as the second argument. Passing None means the underlying Skia default (kCW / clockwise) is used. This preserves correct ellipse geometry and is an appropriate simplification.

crates/grida-canvas/src/shape/rect.rs (1)

18-20: Path::rect(&rect, None) is correct for this conversion trait. It constructs a new Path from the rectangle using the appropriate defaults (clockwise direction, starting at upper-left). Since the trait implementation needs to return a new Path rather than mutate an existing one, Path::rect() (constructor) is the right choice, not add_rect() (appender).

crates/grida-canvas/src/devtools/text_overlay.rs (1)

32-41: PathBuilder baseline construction looks correct

Builder-based baseline segments should behave the same as the previous Path mutation pattern, and returning Some(builder.detach()) is consistent with the rest of the migration.

crates/grida-canvas/src/devtools/hit_overlay.rs (1)

88-90: Transform order preserved; confirm make_transform failure semantics

The sequential reassignment matches the old “apply layer, then apply camera” order. Only concern is whether make_transform can fail/produce empty paths in cases where the old transform mutated in-place—worth confirming via build + a quick devtools smoke test.

crates/grida-canvas/src/sk_tiny/mod.rs (1)

8-30: TinyPath → SkPath conversion via PathBuilder looks correct

The segment mapping is direct and should preserve contour boundaries and closes.

crates/grida-canvas/src/painter/geometry.rs (1)

121-134: LGTM on direct constructors; double-check Node::Line intent + Skia API signatures

  • to_path() simplification reads clean and should reduce unnecessary path mutation.
  • Path::line((0,0),(width,0)) is fine if “Line” is defined as a horizontal segment in local space (with transforms handling rotation); just confirm that n.size.height is not meant to participate here.

Also applies to: 163-164, 334-335

crates/grida-canvas/examples/golden_boolop.rs (3)

1-1: Import addition is fine; keep skia-safe path construction consistent across examples.

This aligns with the broader PathBuilder migration (and matches the “use skia-safe for rendering” guideline). As per coding guidelines, use skia-safe for painting/rendering operations.


15-16: Constructor-based Path::rect/Path::circle usage looks correct.

Nice simplification vs mutable path building; should reduce accidental mutation and match skia-safe 0.91+ patterns.

Also applies to: 27-28, 50-51, 59-60


36-42: Remove unwrap() on Path::op at line 61; the method can legitimately return None.

Line 61 uses .unwrap() on a Path::op call that can fail. Since this is an example/test file that runs in CI, it should handle the failure gracefully rather than panic:

-        let union = Path::op(&r, &c, PathOp::Union).unwrap();
+        let Some(union) = Path::op(&r, &c, PathOp::Union) else {
+            return;
+        };

All other Path::op calls in this file (lines 17, 29, 43, 52, 68) are already correctly using if let Some(...) pattern matching.

Likely an incorrect or invalid review comment.

crates/grida-canvas/src/shape/stroke_rect.rs (1)

3-4: Import update is fine for the PathBuilder migration.

crates/grida-canvas/src/painter/painter.rs (2)

16-19: Import extension for PathBuilder is fine.


976-982: Layer transform via make_transform is the right direction (immutable transform).

Looks consistent with the broader migration away from in-place mutation.

crates/grida-canvas/src/vectornetwork/vn.rs (3)

271-315: Builder-based segment-to-path conversion looks correct.

Returning builder.detach() for empty segments is a clean way to produce an empty Path.


362-409: Region path construction + set_fill_type before detach() is good.

Optional hardening: if loops are supposed to always close, consider a final builder.close() when a loop finishes but didn’t return to its start (to avoid surprising fill artifacts from malformed data).


417-423: Appending paths via a single builder is clean and consistent with the migration.

crates/grida-canvas/src/text/mod.rs (3)

5-5: LGTM: Updated imports for PathBuilder migration.

The imports correctly add Matrix and PathBuilder to support the new path construction pattern.


34-37: LGTM: Clean API addition for vector network conversion.

The new paragraph_to_vector_network function provides a convenient public API for converting paragraphs directly to vector networks. The implementation correctly delegates to paragraph_to_path and leverages the existing VectorNetwork::from conversion.


8-31: LGTM: PathBuilder migration is clean and includes zero-offset optimization.

The refactored path construction correctly uses the builder pattern and adds an optimization to skip identity transforms when the glyph offset is zero. The conditional transform logic (lines 19-25) is a good performance enhancement. All APIs (PathBuilder, add_path, make_transform, detach) are properly available in skia-safe 0.91.0 and are correctly applied here.

crates/grida-canvas/tests/stroke_dash_width.rs (1)

5-18: LGTM: Test helper functions updated to PathBuilder pattern.

Both helper functions correctly adopt the PathBuilder pattern for path construction. The changes are mechanical and preserve the original test behavior.

crates/grida-canvas/src/hittest/hit_tester.rs (1)

91-91: LGTM: Transform operations updated to immutable pattern.

The change from path.transform() to path = path.make_transform() adopts an immutable transformation approach, which is safer and aligns with the PathBuilder migration. Since the path is already cloned before transformation (lines 87, 117, 138), this change doesn't introduce additional overhead.

Also applies to: 121-121, 142-142

crates/grida-canvas/tests/dashed_stroke.rs (1)

7-10: LGTM: Test updated to PathBuilder pattern.

The test correctly adopts the PathBuilder pattern for path construction, maintaining the original test logic and assertions.

crates/grida-canvas/tests/open_path_stroke_align.rs (1)

7-10: LGTM: Test migrated to PathBuilder pattern.

The path construction correctly uses PathBuilder, preserving the test's original behavior and assertions for stroke alignment on open paths.

crates/grida-canvas/examples/golden_sk_text_backdrop_blur_path.rs (1)

19-25: PathBuilder import aligns with the migration (but double-check 0.91.0 API surface).
Looks consistent with the new paragraph_to_path implementation. Since this relies on skia-safe 0.91.0 API changes, please confirm the exact signatures you’re targeting (especially around PathBuilder methods).

crates/grida-canvas/src/shape/stroke_varwidth.rs (1)

3-3: Import is correct for skia-safe 0.91.0.

The use skia_safe::{Path, PathBuilder}; import is properly exposed in skia-safe 0.91.0, and the feature flags in Cargo.toml ("gpu", "gl", "textlayout", "pdf", "svg", "webp") do not restrict Path/PathBuilder availability. All methods used in the file—PathBuilder::new_path(), add_path(), detach(), move_to(), and close()—are valid 0.91.0 APIs.

[dependencies]
# skia
skia-safe = { version = "0.89.0", features = ["gpu", "gl", "textlayout", "pdf", "svg", "webp"] }
skia-safe = { version = "0.91.0", features = ["gpu", "gl", "textlayout", "pdf", "svg", "webp"] }
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 | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

skia-safe 0.91.0 release notes changelog

💡 Result:

Summary of skia-safe 0.91.0 release (changelog)

  • Released: 08 Dec (tag v0.91.0). Aligns the Rust bindings to Skia milestone 143. (github.com)
  • Notable items:
    • CI publishing workflows changed to give finer control over generated prebuilt binaries; some binary combinations were removed. (github.com)
    • Path mutation functions were removed; use PathBuilder + snapshot() or detach() instead. (github.com)
    • Fixes and small changes: broken Skottie README link, expose a custom_empty FontMgr, milestone 143 integration, and workflow simplification PRs. (github.com)

For the full changelog and individual PR links see the release entry on the rust-skia GitHub releases page. (github.com)

Citations:


🌐 Web query:

skia-safe 0.91.0 Path make_transform rect oval circle API signatures

💡 Result:

Here are the Rust API signatures (skia-safe / rust-skia v0.91.0) you asked for:

  • Path::make_transform

    • pub fn make_transform(&mut self, m: &Matrix, pc: impl Into<Option>) -> Path. [1][2]
  • Path::rect variants

    • pub fn rect(rect: impl AsRef, dir: impl Into<Option>) -> Self.
    • pub fn rect_with_fill_type(rect: impl AsRef, fill_type: PathFillType, dir: impl Into<Option>) -> Self. [1]
  • Path::oval variants

    • pub fn oval(oval: impl AsRef, dir: impl Into<Option>) -> Self.
    • pub fn oval_with_start_index(oval: impl AsRef, dir: PathDirection, start_index: usize) -> Self.
    • pub fn add_oval(&mut self, oval: impl AsRef, dir_start: Option<(PathDirection, usize)>) -> &mut Self. [1][2]
  • Path::circle variants

    • pub fn circle(center: impl Into, radius: scalar, dir: impl Into<Option>) -> Self.
    • pub fn add_circle(&mut self, p: impl Into, radius: scalar, dir: impl Into<Option>) -> &mut Self. [1]

Sources: rust-skia docs. [1] rust-skia Path type and methods. [2] rust-skia path module (make_transform listed).


🏁 Script executed:

# Check the current Cargo.toml and edition setting
cat crates/grida-canvas/Cargo.toml | head -20

Repository: gridaco/grida

Length of output: 420


🏁 Script executed:

# Search for usage of Path methods in the codebase
rg "Path::(make_transform|rect|oval|circle)" crates/grida-canvas/ -A 2 -B 2

Repository: gridaco/grida

Length of output: 6349


🏁 Script executed:

# Check for any Rust 2024 edition requirements in the repo
rg "edition\s*=\s*\"2024\"" crates/ --type toml

Repository: gridaco/grida

Length of output: 102


🏁 Script executed:

# Search for mutable Path operations that may have been removed
rg "(add_rect|add_oval|add_circle|make_transform)" crates/grida-canvas/ -A 2 -B 2

Repository: gridaco/grida

Length of output: 7862


🏁 Script executed:

# Search for any &mut Path usage patterns
rg "&mut.*Path" crates/grida-canvas/ -A 2 -B 2

Repository: gridaco/grida

Length of output: 2711


🌐 Web query:

skia-safe 0.91.0 PathBuilder snapshot detach migration path mutation removal

💡 Result:

Short answer: In skia-safe 0.91.0 snapshot/detach are methods on Path (Path::snapshot(), Path::detach()) — not on PathBuilder — so migration is to finish building your path and call snapshot() / detach() on the Path you built (or use Path::raw / Path::new helpers). See the 0.91.0 docs showing Path::snapshot() and Path::detach(). [1][2]

If you have code that previously called snapshot/detach on a PathBuilder, change it to:

  • obtain the built Path (e.g. let path = path_builder.finish(); or otherwise get the Path),
  • then call path.snapshot() (returns a copy) or path.detach() (returns a copy and clears the original) as needed.

References:

  • skia-safe Path documentation (shows snapshot & detach in 0.91.0). [1]
  • skia-safe crate listing / 0.91.0 release docs. [2]

If you want, paste the snippet you need migrated and I’ll convert it.


Critical: skia-safe 0.91.0 removes Path::make_transform() – extensive codebase usage requires migration.

The upgrade to 0.91.0 introduces a breaking change: path mutation functions (including make_transform()) have been removed. The codebase has 15+ active uses of path.make_transform() across painter, text, hittest, and devtools modules that will fail to compile.

Migration: Instead of path.make_transform(&matrix), use PathBuilder to construct a new path with transformations applied, or build paths without mutation. Example pattern currently in ellipse_ring.rs shows the correct approach: use PathBuilder with add_oval() and detach().

Additionally, the package edition remains "2021" while repo guidelines recommend "2024" for graphics core—consider addressing in this PR or follow-up.

🤖 Prompt for AI Agents
crates/grida-canvas/Cargo.toml around line 11: skia-safe 0.91.0 removed
Path::make_transform(), breaking ~15 uses of path.make_transform() across
painter, text, hittest, and devtools; replace each mutate-in-place call with
code that constructs a new transformed Path via PathBuilder (or other
non-mutating API) — e.g., create a PathBuilder, add the original geometry (or
use the same builder pattern used in ellipse_ring.rs: add_oval()/other add_*
calls), apply the matrix transformation while building, then call detach() to
get the new Path; update all call sites accordingly to stop relying on path
mutation. Also consider updating the package edition from "2021" to "2024" in
this Cargo.toml either in this PR or in a follow-up per repo guidelines.

Comment on lines 17 to 26
use skia_safe::{
self as sk,
font_style::{Slant, Weight, Width},
path::AddPathMode,
surfaces,
textlayout::{
FontCollection, Paragraph, ParagraphBuilder, ParagraphStyle, TextAlign, TextStyle,
TypefaceFontProvider,
},
Color, Data, FontMgr, Path, Point,
Color, Data, FontMgr, Matrix, Path, PathBuilder, Point,
};
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

Golden determinism + WASM parity risk: relies on system fonts (FontMgr::new() / “Arial”).
Since this file is used to generate a golden PNG, system-font selection can differ across machines/CI and diverge even more on WASM (per the PR note). Consider making all text (title/labels too) use fixture fonts (same TypefaceFontProvider/FontCollection approach you use elsewhere) so the output is deterministic. Based on learnings, skia-safe should be the backend, but the font source should be controlled.

🤖 Prompt for AI Agents
In crates/grida-canvas/examples/golden_sk_paragraph_path_vector.rs around lines
17 to 26, the example currently relies on system fonts (FontMgr::new()/“Arial”)
which breaks golden determinism and WASM parity; replace system font usage by
loading the fixture font(s) into a TypefaceFontProvider, attach that provider to
a FontCollection, and use that FontCollection for all Paragraph/ParagraphBuilder
TextStyles (title and labels included). Ensure you load the bundled font data
(e.g., via include_bytes! or the project fixture loader), register it with the
TypefaceFontProvider, set the default typeface in ParagraphStyle/TextStyle where
needed, and remove any calls to FontMgr::new() or hardcoded family names so the
generated PNG is deterministic across machines and WASM.

Comment on lines +81 to +82
path = path.make_transform(&sk::sk_matrix(transform.matrix));
path = path.make_transform(&sk::sk_matrix(camera.view_matrix().matrix));
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 | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, check the skia-safe version in the project
fd -e "Cargo.toml" | head -5 | xargs grep -l "skia-safe" | head -1 | xargs cat | grep -A 2 "skia-safe"

Repository: gridaco/grida

Length of output: 39


🌐 Web query:

skia-safe 0.91.0 Path::make_transform return type documentation

💡 Result:

In skia-safe 0.91.0 the Path::make_transform signature is:

pub fn make_transform(&mut self, m: &Matrix, pc: impl Into<Option>) -> Path

It returns an owned Path (i.e., a new Path value). [1][2]

Sources:
[1] rust-skia docs — Path method list.
[2] rust-skia API page for Path.


🏁 Script executed:

# Search for make_transform usage in the codebase
rg "make_transform" --type rust -B 2 -A 2

Repository: gridaco/grida

Length of output: 6937


Compose the two transform matrices into a single matrix, then apply one make_transform call to reduce path allocations.

The return type of make_transform is Path (not Option), so the code compiles. However, applying two sequential transforms creates two path allocations. Pre-compose transform.matrix with camera.view_matrix().matrix using AffineTransform::compose() (or equivalent matrix composition), then call make_transform once. This pattern is already used elsewhere in the codebase (e.g., painter/geometry.rs, painter/layer.rs).

-                    path = path.make_transform(&sk::sk_matrix(transform.matrix));
-                    path = path.make_transform(&sk::sk_matrix(camera.view_matrix().matrix));
+                    let composed = camera.view_matrix().compose(&transform);
+                    path = path.make_transform(&sk::sk_matrix(composed.matrix));

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In crates/grida-canvas/src/devtools/stroke_overlay.rs around lines 81-82, the
code applies two sequential make_transform calls which allocates two
intermediate Paths; instead, compose transform.matrix with
camera.view_matrix().matrix (e.g., via AffineTransform::compose() or equivalent
matrix multiplication) into a single AffineTransform, then call path =
path.make_transform(&sk::sk_matrix(composed.matrix)) once to apply both
transforms in one allocation.

Comment on lines 320 to 324
// Create path from centerline
let mut centerline_path = Path::new();
centerline_path.add_rrect(centerline_rrect, None);
let mut centerline_builder = PathBuilder::new();
centerline_builder.add_rrect(centerline_rrect, None, None);
let centerline_path = centerline_builder.detach();

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 | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n crates/grida-canvas/src/shape/stroke_rect.rs | head -50

Repository: gridaco/grida

Length of output: 2188


🏁 Script executed:

cat -n crates/grida-canvas/src/shape/stroke_rect.rs | sed -n '1,30p'

Repository: gridaco/grida

Length of output: 1337


🏁 Script executed:

cat -n crates/grida-canvas/src/shape/stroke_rect.rs | sed -n '315,360p'

Repository: gridaco/grida

Length of output: 2040


Standardize PathBuilder qualification for consistency. PathBuilder is imported at line 3, but line 349 uses the fully qualified skia_safe::PathBuilder::new() while lines 321, 356, and 360 use the imported name directly. Use PathBuilder::new() everywhere to match the import style.

🤖 Prompt for AI Agents
In crates/grida-canvas/src/shape/stroke_rect.rs around lines 320-360, the file
imports PathBuilder but one occurrence uses the fully-qualified
skia_safe::PathBuilder::new() (line ~349) while other sites use
PathBuilder::new(); change that fully-qualified call to the imported
PathBuilder::new() so all usages consistently use the imported name.

Comment on lines +280 to 286
let mut builder = PathBuilder::new();
add_catmull_segments(&mut builder, &left, false);
right.reverse();
add_catmull_segments(&mut path, &right, true);
path.close();
path
add_catmull_segments(&mut builder, &right, true);
builder.close();
builder.detach()
}
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 | 🔴 Critical

Guard samples == 0 (division-by-zero) before building with PathBuilder.

for i in 0..=samples { let u = i as f32 / samples as f32; } will divide by zero when samples == 0. Since this is a pub fn, it should fail safe.

 pub fn variable_width_stroke_geometry<FS, FT, FW>(
     sample: FS,
     tangent: FT,
     width: FW,
     samples: usize,
 ) -> Path
 where
@@
 {
+    if samples == 0 {
+        return Path::new();
+    }
     // Arc-length LUT (denser than output samples for stability)
     let (ts, us) = build_arc_lut(&sample, samples * 3);
@@
     let mut builder = PathBuilder::new();
     add_catmull_segments(&mut builder, &left, false);
     right.reverse();
     add_catmull_segments(&mut builder, &right, true);
     builder.close();
     builder.detach()
 }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In crates/grida-canvas/src/shape/stroke_varwidth.rs around lines 280 to 286, the
code builds a PathBuilder after sampling, but the sampling loop uses `i as f32 /
samples as f32` which will panic on `samples == 0`; add a guard at the start of
the function (or immediately before building the path) to handle `samples == 0`
safely — either return an empty/degenerate Path (e.g., create and detach a new
PathBuilder without looping) or early-return a default Path value; ensure no
division by zero occurs by checking `if samples == 0 { /* return empty path */
}` before any loop that divides by `samples`.

Comment on lines +391 to 400
/// * `builder` - The PathBuilder to add segments to
/// * `pts` - Array of points to interpolate through
/// * `continue_path` - Whether to continue from the current path position or start a new subpath
fn add_catmull_segments(path: &mut Path, pts: &[(f32, f32)], continue_path: bool) {
fn add_catmull_segments(builder: &mut PathBuilder, pts: &[(f32, f32)], continue_path: bool) {
if pts.is_empty() {
return;
}
if !continue_path {
path.move_to(pts[0]);
builder.move_to(pts[0]);
}
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 | 🔴 Critical

continue_path=true likely needs an explicit join to pts[0] (otherwise first cubic starts from the wrong point).

When continuing, the current point is whatever the previous sub-path ended at, but the Catmull control math for the first segment is based on p1 = pts[0]. Without moving/connecting to pts[0], the first cubic_to will start at the wrong anchor and can distort the outline join.

Suggested fix:

 fn add_catmull_segments(builder: &mut PathBuilder, pts: &[(f32, f32)], continue_path: bool) {
     if pts.is_empty() {
         return;
     }
-    if !continue_path {
-        builder.move_to(pts[0]);
-    }
+    if continue_path {
+        builder.line_to(pts[0]);
+    } else {
+        builder.move_to(pts[0]);
+    }
@@
-        builder.cubic_to(c1, c2, p2);
+        builder.cubic_to(c1, c2, p2);
     }
 }

Please verify PathBuilder::line_to exists in skia-safe 0.91.0 and that (f32, f32) tuples still coerce into the expected Skia point type for move_to/line_to/cubic_to.

skia-safe 0.91.0 PathBuilder move_to/line_to/cubic_to accepted argument types (Point vs tuples) and method signatures

Also applies to: 433-434

🤖 Prompt for AI Agents
In crates/grida-canvas/src/shape/stroke_varwidth.rs around lines 391-400 (and
similarly at 433-434), when continue_path is true you must explicitly join the
current point to pts[0] before emitting Catmull-Rom-derived cubics; add a call
to PathBuilder::line_to(pts[0]) (instead of move_to) when continue_path is true
so the first cubic starts from the correct anchor, and ensure you only call
move_to(pts[0]) when starting a new subpath; verify that skia-safe 0.91.0
exposes PathBuilder::line_to and that (f32, f32) tuples coerce to the Skia Point
type accepted by move_to/line_to/cubic_to before applying the change.

Comment on lines 358 to 361
if y_offset != 0.0 {
path.transform(&Matrix::translate((0.0, y_offset)));
path = path.make_transform(&Matrix::translate((0.0, y_offset)));
}
Some(VectorNetwork::from(&path))
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 | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Find the file and examine the context around lines 358-361
head -n 370 crates/grida-canvas/src/window/application.rs | tail -n 25

Repository: gridaco/grida

Length of output: 1044


🏁 Script executed:

# Search for y_offset definition and usage in the file
rg "y_offset" crates/grida-canvas/src/window/application.rs -B 5 -A 2

Repository: gridaco/grida

Length of output: 875


🏁 Script executed:

# Check the broader context - find the paragraph_to_path function and how y_offset is derived
rg "paragraph_to_path|layout_height" crates/grida-canvas/src/window/application.rs -B 3 -A 3

Repository: gridaco/grida

Length of output: 1005


🏁 Script executed:

# Find where n.height comes from and understand the Node structure
rg "struct.*Node|pub height" crates/grida-canvas/src --type rs -A 3 | head -50

Repository: gridaco/grida

Length of output: 83


🏁 Script executed:

# Check if there's any validation on height values when nodes are created/loaded
rg "height.*is_finite|is_nan|is_infinite" crates/grida-canvas/src --type rs

Repository: gridaco/grida

Length of output: 83


🏁 Script executed:

# Look at the paragraph module to understand height() return type and behavior
fd "paragraph" crates/grida-canvas/src --type f

Repository: gridaco/grida

Length of output: 99


🏁 Script executed:

# Check skia-safe crate version and see if we can find usage patterns
rg "skia_safe.*Path|make_transform" crates/grida-canvas/src --type rs -A 1 -B 1 | head -40

Repository: gridaco/grida

Length of output: 83


🏁 Script executed:

# Fix the rg commands - search for Node struct definition
rg "struct Node|pub height" crates/grida-canvas/src -A 3 | head -60

Repository: gridaco/grida

Length of output: 3527


🏁 Script executed:

# Check paragraph.rs to understand height() behavior
cat crates/grida-canvas/src/cache/paragraph.rs | head -80

Repository: gridaco/grida

Length of output: 3679


🏁 Script executed:

# Search for make_transform usage patterns
rg "make_transform" crates/grida-canvas/src -B 2 -A 2

Repository: gridaco/grida

Length of output: 6427


🏁 Script executed:

# Search for Node struct definition to understand height field
rg "pub struct Node|^struct Node" crates/grida-canvas/src -A 20 | head -100

Repository: gridaco/grida

Length of output: 6476


🏁 Script executed:

# Check if there's any validation when nodes are loaded/created
rg "\.height\s*=" crates/grida-canvas/src -B 3 -A 1 | head -60

Repository: gridaco/grida

Length of output: 4257


🏁 Script executed:

# Look for any is_finite or validation patterns for floating point values
rg "is_finite|is_nan|is_normal|clamp" crates/grida-canvas/src | head -30

Repository: gridaco/grida

Length of output: 3205


🏁 Script executed:

# Check paragraph.height() implementation
rg "fn height|pub fn height" crates/grida-canvas/src -A 3

Repository: gridaco/grida

Length of output: 39


🏁 Script executed:

# Look at the specific context around the zoom_factor check to understand the pattern
rg "zoom_factor.is_finite" crates/grida-canvas/src/window/application.rs -B 5 -A 3

Repository: gridaco/grida

Length of output: 488


🏁 Script executed:

# Check the image.rs pattern to see how is_finite is used for derived values
rg "scale.is_finite" crates/grida-canvas/src/painter/image.rs -B 5 -A 3

Repository: gridaco/grida

Length of output: 284


🏁 Script executed:

# Search for where y_offset is computed to understand the full context
rg "let y_offset|y_offset\s*=" crates/grida-canvas/src/window/application.rs -B 10 -A 1

Repository: gridaco/grida

Length of output: 594


Align y_offset guard with codebase defensive pattern

The code applies make_transform to y_offset, which is computed from user-provided n.height and layout-derived layout_height. Following the existing defensive pattern used for similar user-derived values (e.g., zoom_factor.is_finite() in the zoom path, sanitize_scale in image rendering), add a finiteness check:

- if y_offset != 0.0 {
-     path = path.make_transform(&Matrix::translate((0.0, y_offset)));
- }
+ if y_offset.is_finite() && y_offset != 0.0 {
+     path = path.make_transform(&Matrix::translate((0.0, y_offset)));
+ }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if y_offset != 0.0 {
path.transform(&Matrix::translate((0.0, y_offset)));
path = path.make_transform(&Matrix::translate((0.0, y_offset)));
}
Some(VectorNetwork::from(&path))
if y_offset.is_finite() && y_offset != 0.0 {
path = path.make_transform(&Matrix::translate((0.0, y_offset)));
}
Some(VectorNetwork::from(&path))
🤖 Prompt for AI Agents
In crates/grida-canvas/src/window/application.rs around lines 358–361, the code
applies path.make_transform using y_offset derived from user inputs without
validating it; add a defensive finiteness check (e.g., if y_offset.is_finite())
so the transform is only applied when y_offset is finite (otherwise skip the
make_transform or treat as 0.0) to match the existing pattern used for
user-derived values.

Comment thread crates/grida-dev/Cargo.toml Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant