refactor(cg): extract shared vocabulary crate - #59
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (49)
WalkthroughA standalone ChangesCanvas graphics crate
Conversion boundaries
Documentation and architecture
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/grida/src/backends/skia/mappings.rs (1)
50-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate
Blenderconversion 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 toBlender::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 valueSimplify binary search with
partition_point.You can cleanly replace the manual binary search loop with the standard library's
partition_pointmethod, 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 winUse distinct off-diagonal values in
json_transform2d_conversion. The current roundtrip only usesm01 == 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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
AGENTS.mdCargo.tomlREADME.mdcrates/cg/Cargo.tomlcrates/cg/src/alignment.rscrates/cg/src/color.rscrates/cg/src/colormatrix.rscrates/cg/src/fe.rscrates/cg/src/lib.rscrates/cg/src/prelude.rscrates/cg/src/rect.rscrates/cg/src/stroke_dasharray.rscrates/cg/src/stroke_width.rscrates/cg/src/svg.rscrates/cg/src/tilemode.rscrates/cg/src/transform.rscrates/cg/src/types.rscrates/cg/src/varwidth.rscrates/grida/Cargo.tomlcrates/grida/examples/golden_colors.rscrates/grida/examples/golden_layout_flex.rscrates/grida/examples/golden_layout_padding.rscrates/grida/examples/golden_type_attributed.rscrates/grida/src/backends/skia/mappings.rscrates/grida/src/backends/skia/mod.rscrates/grida/src/htmlcss/paint.rscrates/grida/src/import/svg/from_usvg.rscrates/grida/src/import/svg/packed_scene.rscrates/grida/src/layout/into_taffy.rscrates/grida/src/lib.rscrates/grida/src/overlay/ui/render.rscrates/grida/src/painter/effects_noise.rscrates/grida/src/painter/geometry.rscrates/grida/src/painter/gradient.rscrates/grida/src/painter/paint.rscrates/grida/src/painter/painter.rscrates/grida/src/painter/shadow.rscrates/grida/src/runtime/filter.rscrates/grida/src/runtime/scene.rscrates/grida/src/shape/stroke.rscrates/grida/src/shape/stroke_rect.rscrates/grida/src/text/mod.rscrates/grida/src/text/paragraph_cache_layout.rscrates/grida/src/text/text_style.rscrates/grida/src/text_edit/attributed_text/mod.rscrates/grida/tests/cg_architecture.rscrates/grida/tests/svg_import_architecture.rsdocs/wg/feat-painting/paint-model.mddocs/wg/feat-svg/pattern.md
There was a problem hiding this comment.
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 winDelegate
Blenderconversion 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 toBlender::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 valueSimplify binary search with
partition_point.You can cleanly replace the manual binary search loop with the standard library's
partition_pointmethod, 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 winUse distinct off-diagonal values in
json_transform2d_conversion. The current roundtrip only usesm01 == 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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
AGENTS.mdCargo.tomlREADME.mdcrates/cg/Cargo.tomlcrates/cg/src/alignment.rscrates/cg/src/color.rscrates/cg/src/colormatrix.rscrates/cg/src/fe.rscrates/cg/src/lib.rscrates/cg/src/prelude.rscrates/cg/src/rect.rscrates/cg/src/stroke_dasharray.rscrates/cg/src/stroke_width.rscrates/cg/src/svg.rscrates/cg/src/tilemode.rscrates/cg/src/transform.rscrates/cg/src/types.rscrates/cg/src/varwidth.rscrates/grida/Cargo.tomlcrates/grida/examples/golden_colors.rscrates/grida/examples/golden_layout_flex.rscrates/grida/examples/golden_layout_padding.rscrates/grida/examples/golden_type_attributed.rscrates/grida/src/backends/skia/mappings.rscrates/grida/src/backends/skia/mod.rscrates/grida/src/htmlcss/paint.rscrates/grida/src/import/svg/from_usvg.rscrates/grida/src/import/svg/packed_scene.rscrates/grida/src/layout/into_taffy.rscrates/grida/src/lib.rscrates/grida/src/overlay/ui/render.rscrates/grida/src/painter/effects_noise.rscrates/grida/src/painter/geometry.rscrates/grida/src/painter/gradient.rscrates/grida/src/painter/paint.rscrates/grida/src/painter/painter.rscrates/grida/src/painter/shadow.rscrates/grida/src/runtime/filter.rscrates/grida/src/runtime/scene.rscrates/grida/src/shape/stroke.rscrates/grida/src/shape/stroke_rect.rscrates/grida/src/text/mod.rscrates/grida/src/text/paragraph_cache_layout.rscrates/grida/src/text/text_style.rscrates/grida/src/text_edit/attributed_text/mod.rscrates/grida/tests/cg_architecture.rscrates/grida/tests/svg_import_architecture.rsdocs/wg/feat-painting/paint-model.mddocs/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()onf32::partial_cmpwill cause a panic if the profile stops contain aNaNposition. Usef32::total_cmpto 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
NaNinputs.If
uisNaN,u.clamp(0.0, 1.0)evaluates toNaN. Because all relational comparisons withNaNevaluate to false, the early-return boundary checks fail. If the profile contains exactly one stop (stops.len() == 1), execution falls through toself.stops[lo + 1], which will panic with an out-of-bounds access. Add an early return to handleNaNinputs 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.
What
grida::cgvocabulary into a new skia-free workspace crate,crates/cg.pub use cg, including the existing module tree, prelude, and root-export asymmetry.Fromimplementations into localIntoSkia,IntoCg, andIntoTaffytraits.This is charter PR 2 in the consolidation program tracked by #43. It does not adopt
cginn0, grant a capability, score either engine, or touch a frozen surface.Why
cgis 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
crates/grida/src/cg, including their inline caveats and tests.grida::cgand its child modules remain available through a crate re-export. No repository consumer needs a namespace migration.Fromaccess to those pairs is the deliberate API drop; Rust's orphan rule makes it unrepresentable after extraction. The defining-crate component oftype_namealso necessarily changes.cghas onlymath2andserdeas normal dependencies andserde_jsonfor tests. It gains no Skia, usvg, Taffy,grida, orn0dependency.format/grida.fbsandcrates/grida-canvas-wasmare 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-diffproves the extracted patch is byte-for-byte the reviewed local patch, replayed without the unrelated anchor-doc parent.L0.exactPNGs byte-identical, corpusf1892dcdab29442e.cargo test --locked -p cg: 37 unit tests pass; doctests: 21 pass, 23 ignored.cargo check --locked -p cg -p grida -p grida-canvas-wasm -p grida_dev -p n0 -p n0-modelpasses.cargo clippy --no-deps --workspace --exclude grida-canvas-wasm -- -D warningspasses.cargo fmt --all -- --checkpasses.crates/cg, 10 added lines, 0 removed.git diff --check, frozen-surface check, no-n0check, link audit, and public-repo/secrets audit pass.Extended local evidence on the identical patch before replay:
d85d13b60bc0ff7367a291bda3870fa8fe764c0ee39025c511aec696ef2a3265.n0host gate, and the optimized wasm package build/typecheck/Vitest smoke pass.Refs: #43, #28, #29.