Skip to content

refactor(cg): extract shared vocabulary crate - #59

Merged
softmarshmallow merged 1 commit into
mainfrom
chore/consolidation-cg-crate-cut
Jul 20, 2026
Merged

refactor(cg): extract shared vocabulary crate#59
softmarshmallow merged 1 commit into
mainfrom
chore/consolidation-cg-crate-cut

Conversation

@softmarshmallow

Copy link
Copy Markdown
Member

What

  • Extract the complete legacy grida::cg vocabulary into a new skia-free workspace crate, crates/cg.
  • Preserve legacy source paths through pub use cg, including the existing module tree, prelude, and root-export asymmetry.
  • Re-home the conversion bodies that Rust coherence no longer permits as standard From implementations into local IntoSkia, IntoCg, and IntoTaffy traits.
  • Retarget the architecture locks, doctests, workspace inventories, and implementation links.

This is charter PR 2 in the consolidation program tracked by #43. It does not adopt cg in n0, grant a capability, score either engine, or touch a frozen surface.

Why

cg is shared graphics vocabulary currently owned by the legacy engine crate. Cutting it at the agreed seam gives both engines a neutral, skia-free dependency boundary while keeping the legacy engine behavior and namespace intact.

Patrol and captured-essence ledger

  • Moved estate: all 14 files and 6,748 lines formerly under crates/grida/src/cg, including their inline caveats and tests.
  • Preserved contract: exact serde/wire spellings; ordered and tri-state paint semantics; stroke normalization and explicit-zero behavior; UTF-8 attributed-text invariants; the spec-faithful SVG IR boundary; effects, image, layout, and geometry vocabulary; and known alignment, NaN, cache, gradient, and historical-spelling quirks.
  • Legacy compatibility: grida::cg and its child modules remain available through a crate re-export. No repository consumer needs a namespace migration.
  • Coherence disposition: 38 now-illegal standard conversions retain the same match arms, fallbacks, comments, and float operations as 12 cg-to-Skia, 19 usvg-to-cg, and 7 cg-to-Taffy local-trait mappings. Direct standard From access to those pairs is the deliberate API drop; Rust's orphan rule makes it unrepresentable after extraction. The defining-crate component of type_name also necessarily changes.
  • Boundary: cg has only math2 and serde as normal dependencies and serde_json for tests. It gains no Skia, usvg, Taffy, grida, or n0 dependency.
  • No silent drops: no rendering, serialization, encoded-surface, fixture, golden, test, documentation caveat, or legacy namespace behavior is removed.
  • Frozen surfaces: format/grida.fbs and crates/grida-canvas-wasm are unchanged.

The commit message carries the full caveat-by-caveat ledger and provenance.

Gate evidence

Fresh on this branch against live base 182bb7f23418efcb77ad75f17dbac6ac9a90bc36:

  • git range-diff proves the extracted patch is byte-for-byte the reviewed local patch, replayed without the unrelated anchor-doc parent.
  • Legacy pixel sweep: 65/65 L0.exact PNGs byte-identical, corpus f1892dcdab29442e.
  • cargo test --locked -p cg: 37 unit tests pass; doctests: 21 pass, 23 ignored.
  • All five legacy seam-lock targets pass (15 tests total).
  • cargo check --locked -p cg -p grida -p grida-canvas-wasm -p grida_dev -p n0 -p n0-model passes.
  • CI-equivalent cargo clippy --no-deps --workspace --exclude grida-canvas-wasm -- -D warnings passes.
  • cargo fmt --all -- --check passes.
  • Cargo.lock additions-only gate passes: crates/cg, 10 added lines, 0 removed.
  • git diff --check, frozen-surface check, no-n0 check, link audit, and public-repo/secrets audit pass.

Extended local evidence on the identical patch before replay:

  • Full local iosvg A/B: 1,679 cases / 4,448 artifacts byte-identical, manifest SHA-256 d85d13b60bc0ff7367a291bda3870fa8fe764c0ee39025c511aec696ef2a3265.
  • 31 FBS round trips, 139 HTML snapshots, 48 SVG packs, the full workspace test/doctest suite, the n0 host gate, and the optimized wasm package build/typecheck/Vitest smoke pass.

Refs: #43, #28, #29.

Patrol and name: move all 14 files and 6,748 lines from crates/grida/src/cg into the new skia-free crates/cg package. Keep the blessed cg name and preserve grida::cg through a crate re-export, including every child module, the prelude, and the existing root-export asymmetry.

Captured essence: retain exact serde and wire spellings, ordered and tri-state paint semantics, stroke normalization and explicit-zero fallbacks, UTF-8 attributed-text invariants, the spec-faithful SVG IR boundary, effects/image/layout vocabulary, known alignment/NaN/cache/gradient quirks, and all inline caveats and tests. Retarget seam locks, doctests, implementation links, and workspace inventories. Leave format/grida.fbs and the published @grida/canvas-wasm@0.91.0-canary.22 freeze contract untouched.

Coherence disposition: the cut makes 38 standard From implementations unrepresentable under Rust's orphan rule. Rehome the same bodies and call behavior as 12 cg-to-Skia IntoSkia mappings, 19 usvg-to-cg IntoCg mappings, and 7 private cg-to-Taffy IntoTaffy mappings. The deliberate API drop is limited to those direct standard From conversions and the unavoidable defining-crate type_name identity change; no rendering, serialization, encoded-surface, test, fixture, golden, or legacy namespace semantics are dropped.

Boundary: cg has only math2 and serde as normal dependencies and serde_json for tests; it gains no Skia, usvg, Taffy, grida, or n0 dependency. No n0 chassis adoption or capability grant occurs in this step.

Gate evidence: 65/65 declared L0.exact PNGs are byte-identical against 84da90a (corpus f1892dcdab29442e). The local 1,679-case iosvg A/B produced 4,448 byte-identical artifacts (manifest SHA-256 d85d13b60bc0ff7367a291bda3870fa8fe764c0ee39025c511aec696ef2a3265). cg has 37 passing unit tests and 21 passing doctests; all seam locks, 31 FBS round trips, 139 HTML snapshots, 48 SVG packs, the full workspace test/doctest suite, locked all-target checks, clippy -D warnings, and n0 GATE pass. The optimized wasm32-unknown-emscripten build, package bundle, TypeScript typecheck, and Vitest smoke also pass.

Refs: #43, #28, #29.
@vercel

vercel Bot commented Jul 20, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
nothing Ready Ready Preview, Comment Jul 20, 2026 8:53am

Request Review

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown

Review Change Stack

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 46b6cf3e-b42f-4636-9968-ba845bd34006

📥 Commits

Reviewing files that changed from the base of the PR and between 182bb7f and 2b9b0df.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (49)
  • AGENTS.md
  • Cargo.toml
  • README.md
  • crates/cg/Cargo.toml
  • crates/cg/src/alignment.rs
  • crates/cg/src/color.rs
  • crates/cg/src/colormatrix.rs
  • crates/cg/src/fe.rs
  • crates/cg/src/lib.rs
  • crates/cg/src/prelude.rs
  • crates/cg/src/rect.rs
  • crates/cg/src/stroke_dasharray.rs
  • crates/cg/src/stroke_width.rs
  • crates/cg/src/svg.rs
  • crates/cg/src/tilemode.rs
  • crates/cg/src/transform.rs
  • crates/cg/src/types.rs
  • crates/cg/src/varwidth.rs
  • crates/grida/Cargo.toml
  • crates/grida/examples/golden_colors.rs
  • crates/grida/examples/golden_layout_flex.rs
  • crates/grida/examples/golden_layout_padding.rs
  • crates/grida/examples/golden_type_attributed.rs
  • crates/grida/src/backends/skia/mappings.rs
  • crates/grida/src/backends/skia/mod.rs
  • crates/grida/src/htmlcss/paint.rs
  • crates/grida/src/import/svg/from_usvg.rs
  • crates/grida/src/import/svg/packed_scene.rs
  • crates/grida/src/layout/into_taffy.rs
  • crates/grida/src/lib.rs
  • crates/grida/src/overlay/ui/render.rs
  • crates/grida/src/painter/effects_noise.rs
  • crates/grida/src/painter/geometry.rs
  • crates/grida/src/painter/gradient.rs
  • crates/grida/src/painter/paint.rs
  • crates/grida/src/painter/painter.rs
  • crates/grida/src/painter/shadow.rs
  • crates/grida/src/runtime/filter.rs
  • crates/grida/src/runtime/scene.rs
  • crates/grida/src/shape/stroke.rs
  • crates/grida/src/shape/stroke_rect.rs
  • crates/grida/src/text/mod.rs
  • crates/grida/src/text/paragraph_cache_layout.rs
  • crates/grida/src/text/text_style.rs
  • crates/grida/src/text_edit/attributed_text/mod.rs
  • crates/grida/tests/cg_architecture.rs
  • crates/grida/tests/svg_import_architecture.rs
  • docs/wg/feat-painting/paint-model.md
  • docs/wg/feat-svg/pattern.md

Walkthrough

A standalone cg workspace crate is introduced with backend-neutral graphics types and utilities. Grida now uses explicit IntoCg, IntoSkia, and IntoTaffy conversion traits, while architecture tests and documentation reference the extracted crate paths.

Changes

Canvas graphics crate

Layer / File(s) Summary
CG crate contracts and public types
Cargo.toml, crates/cg/*
Registers the new crate and adds public coordinate, rectangle, color-matrix, stroke-width, tiling, transform, and variable-width APIs with serialization and unit tests.

Conversion boundaries

Layer / File(s) Summary
Importer and layout conversion boundaries
crates/grida/src/import/svg/*, crates/grida/src/layout/into_taffy.rs, crates/grida/src/lib.rs
Replaces generic SVG and layout conversions with IntoCg and IntoTaffy methods and re-exports the external cg crate.
Skia conversion boundary and call sites
crates/grida/src/backends/skia/*, crates/grida/src/painter/*, crates/grida/src/runtime/*, crates/grida/src/text/*, crates/grida/examples/*
Introduces IntoSkia mappings and updates rendering, text, effects, stroke, and example code to use explicit Skia conversions.

Documentation and architecture

Layer / File(s) Summary
Architecture and path updates
AGENTS.md, README.md, crates/grida/tests/*, docs/wg/*, crates/cg/src/{color,fe,stroke_dasharray,svg,types}.rs
Updates workspace descriptions, Rustdoc imports, architectural path checks, and documentation references for crates/cg.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

  • Issue 14 — The extracted SVG paint vocabulary and SVGPaint location align with the issue’s planned pattern extension point.
  • Issue 589 — The PR directly performs the SVGPaint vocabulary extraction and updates the from_usvg conversion path.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: extracting the shared cg vocabulary into its own crate.
Description check ✅ Passed The description is directly aligned with the changeset and explains the crate extraction, trait rewrites, and retargeted docs/tests.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/consolidation-cg-crate-cut

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.

@softmarshmallow
softmarshmallow marked this pull request as ready for review July 20, 2026 09:22

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
crates/grida/src/backends/skia/mappings.rs (1)

50-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Delegate Blender conversion to eliminate duplicated match arms.

This implementation duplicates the 16-variant mapping logic found in the IntoSkia<skia_safe::BlendMode> block immediately below it. You can simplify this by delegating to that conversion and passing its result to Blender::mode.

♻️ Proposed fix to reuse the existing mapping
-        let val = self;
-        use skia_safe::BlendMode::*;
-        let sk_blend_mode = match val {
-            BlendMode::Normal => SrcOver,
-            BlendMode::Multiply => Multiply,
-            BlendMode::Screen => Screen,
-            BlendMode::Overlay => Overlay,
-            BlendMode::Darken => Darken,
-            BlendMode::Lighten => Lighten,
-            BlendMode::ColorDodge => ColorDodge,
-            BlendMode::ColorBurn => ColorBurn,
-            BlendMode::HardLight => HardLight,
-            BlendMode::SoftLight => SoftLight,
-            BlendMode::Difference => Difference,
-            BlendMode::Exclusion => Exclusion,
-            BlendMode::Hue => Hue,
-            BlendMode::Saturation => Saturation,
-            BlendMode::Color => Color,
-            BlendMode::Luminosity => Luminosity,
-        };
-        skia_safe::Blender::mode(sk_blend_mode)
+        let sk_blend_mode: skia_safe::BlendMode = self.into_skia();
+        skia_safe::Blender::mode(sk_blend_mode)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/grida/src/backends/skia/mappings.rs` around lines 50 - 73, Update the
IntoSkia<skia_safe::Blender>::into_skia implementation to reuse the existing
BlendMode-to-skia_safe::BlendMode conversion from the
IntoSkia<skia_safe::BlendMode> implementation, then pass that converted mode to
skia_safe::Blender::mode. Remove the duplicated 16-variant match while
preserving the current mapping behavior.
crates/cg/src/varwidth.rs (1)

74-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Simplify binary search with partition_point.

You can cleanly replace the manual binary search loop with the standard library's partition_point method, which is safer and more idiomatic.

♻️ Proposed refactor
-        // binary search segment
-        let mut lo = 0usize;
-        let mut hi = self.stops.len() - 1;
-        while lo + 1 < hi {
-            let mid = (lo + hi) / 2;
-            if self.stops[mid].u <= x {
-                lo = mid;
-            } else {
-                hi = mid;
-            }
-        }
+        // Find the segment containing `x`
+        let lo = self.stops.partition_point(|s| s.u <= x).saturating_sub(1);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/cg/src/varwidth.rs` around lines 74 - 84, Replace the manual lo/hi
binary-search loop in the surrounding varwidth lookup logic with the standard
library partition_point on self.stops, using the existing stop-u comparison to
find the last stop at or below x. Preserve the resulting segment-selection
behavior and subsequent indexing.
crates/cg/src/transform.rs (1)

71-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Use distinct off-diagonal values in json_transform2d_conversion. The current roundtrip only uses m01 == m10 == 0.0, so it won’t catch a swapped off-diagonal mapping. Add an asymmetric matrix case.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/cg/src/transform.rs` around lines 71 - 89, Update the
json_transform2d_conversion test to use distinct, nonzero values for the
off-diagonal matrix entries m01 and m10, then preserve the roundtrip assertions
so swapped mappings are detected. Keep the existing conversion implementations
unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/cg/src/varwidth.rs`:
- Around line 42-60: Update the stop sorting in build_sampler to use
f32::total_cmp for the u positions instead of partial_cmp(...).unwrap(),
ensuring NaN values are ordered safely without panicking while preserving the
existing sort behavior.
- Around line 63-65: Update the r method to detect a NaN input immediately after
clamping u and return safely before any stop-index calculations or boundary
checks. Preserve the existing behavior for finite inputs, including empty and
single-stop profiles, while preventing access to self.stops[lo + 1] for NaN
values.

---

Nitpick comments:
In `@crates/cg/src/transform.rs`:
- Around line 71-89: Update the json_transform2d_conversion test to use
distinct, nonzero values for the off-diagonal matrix entries m01 and m10, then
preserve the roundtrip assertions so swapped mappings are detected. Keep the
existing conversion implementations unchanged.

In `@crates/cg/src/varwidth.rs`:
- Around line 74-84: Replace the manual lo/hi binary-search loop in the
surrounding varwidth lookup logic with the standard library partition_point on
self.stops, using the existing stop-u comparison to find the last stop at or
below x. Preserve the resulting segment-selection behavior and subsequent
indexing.

In `@crates/grida/src/backends/skia/mappings.rs`:
- Around line 50-73: Update the IntoSkia<skia_safe::Blender>::into_skia
implementation to reuse the existing BlendMode-to-skia_safe::BlendMode
conversion from the IntoSkia<skia_safe::BlendMode> implementation, then pass
that converted mode to skia_safe::Blender::mode. Remove the duplicated
16-variant match while preserving the current mapping behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 46b6cf3e-b42f-4636-9968-ba845bd34006

📥 Commits

Reviewing files that changed from the base of the PR and between 182bb7f and 2b9b0df.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (49)
  • AGENTS.md
  • Cargo.toml
  • README.md
  • crates/cg/Cargo.toml
  • crates/cg/src/alignment.rs
  • crates/cg/src/color.rs
  • crates/cg/src/colormatrix.rs
  • crates/cg/src/fe.rs
  • crates/cg/src/lib.rs
  • crates/cg/src/prelude.rs
  • crates/cg/src/rect.rs
  • crates/cg/src/stroke_dasharray.rs
  • crates/cg/src/stroke_width.rs
  • crates/cg/src/svg.rs
  • crates/cg/src/tilemode.rs
  • crates/cg/src/transform.rs
  • crates/cg/src/types.rs
  • crates/cg/src/varwidth.rs
  • crates/grida/Cargo.toml
  • crates/grida/examples/golden_colors.rs
  • crates/grida/examples/golden_layout_flex.rs
  • crates/grida/examples/golden_layout_padding.rs
  • crates/grida/examples/golden_type_attributed.rs
  • crates/grida/src/backends/skia/mappings.rs
  • crates/grida/src/backends/skia/mod.rs
  • crates/grida/src/htmlcss/paint.rs
  • crates/grida/src/import/svg/from_usvg.rs
  • crates/grida/src/import/svg/packed_scene.rs
  • crates/grida/src/layout/into_taffy.rs
  • crates/grida/src/lib.rs
  • crates/grida/src/overlay/ui/render.rs
  • crates/grida/src/painter/effects_noise.rs
  • crates/grida/src/painter/geometry.rs
  • crates/grida/src/painter/gradient.rs
  • crates/grida/src/painter/paint.rs
  • crates/grida/src/painter/painter.rs
  • crates/grida/src/painter/shadow.rs
  • crates/grida/src/runtime/filter.rs
  • crates/grida/src/runtime/scene.rs
  • crates/grida/src/shape/stroke.rs
  • crates/grida/src/shape/stroke_rect.rs
  • crates/grida/src/text/mod.rs
  • crates/grida/src/text/paragraph_cache_layout.rs
  • crates/grida/src/text/text_style.rs
  • crates/grida/src/text_edit/attributed_text/mod.rs
  • crates/grida/tests/cg_architecture.rs
  • crates/grida/tests/svg_import_architecture.rs
  • docs/wg/feat-painting/paint-model.md
  • docs/wg/feat-svg/pattern.md

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 2

🧹 Nitpick comments (3)
crates/grida/src/backends/skia/mappings.rs (1)

50-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Delegate Blender conversion to eliminate duplicated match arms.

This implementation duplicates the 16-variant mapping logic found in the IntoSkia<skia_safe::BlendMode> block immediately below it. You can simplify this by delegating to that conversion and passing its result to Blender::mode.

♻️ Proposed fix to reuse the existing mapping
-        let val = self;
-        use skia_safe::BlendMode::*;
-        let sk_blend_mode = match val {
-            BlendMode::Normal => SrcOver,
-            BlendMode::Multiply => Multiply,
-            BlendMode::Screen => Screen,
-            BlendMode::Overlay => Overlay,
-            BlendMode::Darken => Darken,
-            BlendMode::Lighten => Lighten,
-            BlendMode::ColorDodge => ColorDodge,
-            BlendMode::ColorBurn => ColorBurn,
-            BlendMode::HardLight => HardLight,
-            BlendMode::SoftLight => SoftLight,
-            BlendMode::Difference => Difference,
-            BlendMode::Exclusion => Exclusion,
-            BlendMode::Hue => Hue,
-            BlendMode::Saturation => Saturation,
-            BlendMode::Color => Color,
-            BlendMode::Luminosity => Luminosity,
-        };
-        skia_safe::Blender::mode(sk_blend_mode)
+        let sk_blend_mode: skia_safe::BlendMode = self.into_skia();
+        skia_safe::Blender::mode(sk_blend_mode)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/grida/src/backends/skia/mappings.rs` around lines 50 - 73, Update the
IntoSkia<skia_safe::Blender>::into_skia implementation to reuse the existing
BlendMode-to-skia_safe::BlendMode conversion from the
IntoSkia<skia_safe::BlendMode> implementation, then pass that converted mode to
skia_safe::Blender::mode. Remove the duplicated 16-variant match while
preserving the current mapping behavior.
crates/cg/src/varwidth.rs (1)

74-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Simplify binary search with partition_point.

You can cleanly replace the manual binary search loop with the standard library's partition_point method, which is safer and more idiomatic.

♻️ Proposed refactor
-        // binary search segment
-        let mut lo = 0usize;
-        let mut hi = self.stops.len() - 1;
-        while lo + 1 < hi {
-            let mid = (lo + hi) / 2;
-            if self.stops[mid].u <= x {
-                lo = mid;
-            } else {
-                hi = mid;
-            }
-        }
+        // Find the segment containing `x`
+        let lo = self.stops.partition_point(|s| s.u <= x).saturating_sub(1);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/cg/src/varwidth.rs` around lines 74 - 84, Replace the manual lo/hi
binary-search loop in the surrounding varwidth lookup logic with the standard
library partition_point on self.stops, using the existing stop-u comparison to
find the last stop at or below x. Preserve the resulting segment-selection
behavior and subsequent indexing.
crates/cg/src/transform.rs (1)

71-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Use distinct off-diagonal values in json_transform2d_conversion. The current roundtrip only uses m01 == m10 == 0.0, so it won’t catch a swapped off-diagonal mapping. Add an asymmetric matrix case.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/cg/src/transform.rs` around lines 71 - 89, Update the
json_transform2d_conversion test to use distinct, nonzero values for the
off-diagonal matrix entries m01 and m10, then preserve the roundtrip assertions
so swapped mappings are detected. Keep the existing conversion implementations
unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/cg/src/varwidth.rs`:
- Around line 42-60: Update the stop sorting in build_sampler to use
f32::total_cmp for the u positions instead of partial_cmp(...).unwrap(),
ensuring NaN values are ordered safely without panicking while preserving the
existing sort behavior.
- Around line 63-65: Update the r method to detect a NaN input immediately after
clamping u and return safely before any stop-index calculations or boundary
checks. Preserve the existing behavior for finite inputs, including empty and
single-stop profiles, while preventing access to self.stops[lo + 1] for NaN
values.

---

Nitpick comments:
In `@crates/cg/src/transform.rs`:
- Around line 71-89: Update the json_transform2d_conversion test to use
distinct, nonzero values for the off-diagonal matrix entries m01 and m10, then
preserve the roundtrip assertions so swapped mappings are detected. Keep the
existing conversion implementations unchanged.

In `@crates/cg/src/varwidth.rs`:
- Around line 74-84: Replace the manual lo/hi binary-search loop in the
surrounding varwidth lookup logic with the standard library partition_point on
self.stops, using the existing stop-u comparison to find the last stop at or
below x. Preserve the resulting segment-selection behavior and subsequent
indexing.

In `@crates/grida/src/backends/skia/mappings.rs`:
- Around line 50-73: Update the IntoSkia<skia_safe::Blender>::into_skia
implementation to reuse the existing BlendMode-to-skia_safe::BlendMode
conversion from the IntoSkia<skia_safe::BlendMode> implementation, then pass
that converted mode to skia_safe::Blender::mode. Remove the duplicated
16-variant match while preserving the current mapping behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 46b6cf3e-b42f-4636-9968-ba845bd34006

📥 Commits

Reviewing files that changed from the base of the PR and between 182bb7f and 2b9b0df.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (49)
  • AGENTS.md
  • Cargo.toml
  • README.md
  • crates/cg/Cargo.toml
  • crates/cg/src/alignment.rs
  • crates/cg/src/color.rs
  • crates/cg/src/colormatrix.rs
  • crates/cg/src/fe.rs
  • crates/cg/src/lib.rs
  • crates/cg/src/prelude.rs
  • crates/cg/src/rect.rs
  • crates/cg/src/stroke_dasharray.rs
  • crates/cg/src/stroke_width.rs
  • crates/cg/src/svg.rs
  • crates/cg/src/tilemode.rs
  • crates/cg/src/transform.rs
  • crates/cg/src/types.rs
  • crates/cg/src/varwidth.rs
  • crates/grida/Cargo.toml
  • crates/grida/examples/golden_colors.rs
  • crates/grida/examples/golden_layout_flex.rs
  • crates/grida/examples/golden_layout_padding.rs
  • crates/grida/examples/golden_type_attributed.rs
  • crates/grida/src/backends/skia/mappings.rs
  • crates/grida/src/backends/skia/mod.rs
  • crates/grida/src/htmlcss/paint.rs
  • crates/grida/src/import/svg/from_usvg.rs
  • crates/grida/src/import/svg/packed_scene.rs
  • crates/grida/src/layout/into_taffy.rs
  • crates/grida/src/lib.rs
  • crates/grida/src/overlay/ui/render.rs
  • crates/grida/src/painter/effects_noise.rs
  • crates/grida/src/painter/geometry.rs
  • crates/grida/src/painter/gradient.rs
  • crates/grida/src/painter/paint.rs
  • crates/grida/src/painter/painter.rs
  • crates/grida/src/painter/shadow.rs
  • crates/grida/src/runtime/filter.rs
  • crates/grida/src/runtime/scene.rs
  • crates/grida/src/shape/stroke.rs
  • crates/grida/src/shape/stroke_rect.rs
  • crates/grida/src/text/mod.rs
  • crates/grida/src/text/paragraph_cache_layout.rs
  • crates/grida/src/text/text_style.rs
  • crates/grida/src/text_edit/attributed_text/mod.rs
  • crates/grida/tests/cg_architecture.rs
  • crates/grida/tests/svg_import_architecture.rs
  • docs/wg/feat-painting/paint-model.md
  • docs/wg/feat-svg/pattern.md
🛑 Comments failed to post (2)
crates/cg/src/varwidth.rs (2)

42-60: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Prevent panic during profile sort.

Calling unwrap() on f32::partial_cmp will cause a panic if the profile stops contain a NaN position. Use f32::total_cmp to ensure a safe, total ordering.

🐛 Proposed fix
-        stops.sort_by(|a, b| a.u.partial_cmp(&b.u).unwrap());
+        stops.sort_by(|a, b| a.u.total_cmp(&b.u));
📝 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.

    pub fn build_sampler(profile: &VarWidthProfile) -> VarWidthSampler {
        let mut stops = profile.stops.clone();
        stops.sort_by(|a, b| a.u.total_cmp(&b.u));
        // dedupe tiny pos deltas
        let mut dedup: Vec<WidthStop> = Vec::with_capacity(stops.len());
        for s in stops.into_iter() {
            if let Some(last) = dedup.last_mut() {
                if (last.u - s.u).abs() < 1e-6 {
                    last.r = s.r;
                    continue;
                }
            }
            dedup.push(s);
        }
        VarWidthSampler {
            base: profile.base,
            stops: dedup,
        }
    }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/cg/src/varwidth.rs` around lines 42 - 60, Update the stop sorting in
build_sampler to use f32::total_cmp for the u positions instead of
partial_cmp(...).unwrap(), ensuring NaN values are ordered safely without
panicking while preserving the existing sort behavior.

63-65: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Prevent bounds-check panic for NaN inputs.

If u is NaN, u.clamp(0.0, 1.0) evaluates to NaN. Because all relational comparisons with NaN evaluate to false, the early-return boundary checks fail. If the profile contains exactly one stop (stops.len() == 1), execution falls through to self.stops[lo + 1], which will panic with an out-of-bounds access. Add an early return to handle NaN inputs safely.

🐛 Proposed fix
     pub fn r(&self, u: f32) -> f32 {
+        if u.is_nan() {
+            return self.base.max(0.0);
+        }
         let x = u.clamp(0.0, 1.0);
         if self.stops.is_empty() {
             return self.base.max(0.0);
📝 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.

    pub fn r(&self, u: f32) -> f32 {
        if u.is_nan() {
            return self.base.max(0.0);
        }
        let x = u.clamp(0.0, 1.0);
        if self.stops.is_empty() {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/cg/src/varwidth.rs` around lines 63 - 65, Update the r method to
detect a NaN input immediately after clamping u and return safely before any
stop-index calculations or boundary checks. Preserve the existing behavior for
finite inputs, including empty and single-stop profiles, while preventing access
to self.stops[lo + 1] for NaN values.

@softmarshmallow
softmarshmallow merged commit cdba65c into main Jul 20, 2026
14 checks passed
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