Daily RC#476
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughLayout gains optional text-measure integration (TextMeasureProvider) threaded through compute and the layout tree; TextSpan geometry now uses layout results when present. Hit testing can optionally cull by scene-graph clipping. Example/test call sites and runtime code updated; cursor assets and small editor UI/cursor tweaks added. Changes
Sequence Diagram(s)sequenceDiagram
participant Runtime as Runtime/Scene
participant Cache as ParagraphCache
participant Engine as LayoutEngine
participant Tree as LayoutTree
participant Taffy as Taffy
participant Geom as GeometryCache
Note over Runtime,Cache: Optional text-measure path during layout compute
Runtime->>Cache: borrow mutably
Runtime->>Runtime: build TextMeasureProvider(cache, fonts)
Runtime->>Engine: compute(scene, viewport_size, Some(provider))
Engine->>Tree: compute_layout(measure_provider)
alt Text node + provider present
Tree->>Tree: new_text_leaf(node_id, TextMeasureContext)
Tree->>Cache: request measurement via provider
Cache-->>Tree: measured size (w,h)
Tree->>Taffy: set text constraints and create leaf
else no provider
Tree->>Taffy: create leaf without text context
end
Taffy->>Taffy: compute layout
Taffy-->>Tree: layout results
Tree-->>Engine: &LayoutResult
Engine-->>Runtime: &LayoutResult
Runtime->>Geom: compute_node_bounds(using layout result)
Geom-->>Runtime: node local/world bounds
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/grida-canvas/src/cache/geometry.rs (1)
403-432: Critical: Return value must be world_bounds, not local_bounds.Line 432 returns
local_bounds(local coordinate space), but all other node types in this function return world-space bounds (absolute_bounding_boxorunion_world_bounds). This breaks the function's contract and will cause incorrect bounds calculations when parent nodes attempt to union child bounds.Impact:
- Parent containers will compute incorrect union bounds for TextSpan children
- World-space hit testing and culling will fail for TextSpan nodes
- Transforms from parent nodes won't be properly accounted for
🔎 Proposed fix
- local_bounds + world_boundsThis matches the pattern used by all other node types (e.g., leaf nodes return
entry.absolute_bounding_boxon line 502).crates/grida-canvas/src/hittest/hit_tester.rs (1)
248-268: Center-point approximation may cause false negatives for edge-case selections.The center-point check for rectangle intersections can miss nodes where the selection rectangle overlaps clipped content but the center falls outside clip bounds. This could cause unexpected behavior during marquee/lasso selection near clip boundaries.
Consider either:
- Checking if any corner of the selection rectangle is within clip bounds, or
- Using rectangle-rectangle intersection with clip bounds
However, given the existing documentation and that this is an edge case, this may be acceptable for initial implementation.
🧹 Nitpick comments (4)
crates/grida-canvas/src/cache/geometry.rs (1)
393-401: Consider investigating the need for MIN_SIZE_DIRTY_HACK.The constant explicitly marks this as a "dirty hack" to avoid zero-size bounds. Consider whether:
- The root cause of zero-size measurements or layout results should be addressed upstream
- A more explicit zero-size handling strategy would be clearer
- This minimum size of 1.0 has unintended visual or layout side effects
crates/grida-canvas/src/layout/engine.rs (1)
275-290: Consider usingcopied()instead ofclone()forOption<usize>.
Option<usize>implementsCopy, so cloning is unnecessary. Using.copied()or direct assignment would be more idiomatic.🔎 Proposed fix
Node::TextSpan(n) => { let ctx = TextMeasureContext { scene_node_id: *node_id, text: n.text.clone(), text_style: n.text_style.clone(), text_align: n.text_align.clone(), - max_lines: n.max_lines.clone(), + max_lines: n.max_lines, ellipsis: n.ellipsis.clone(), width: n.width, height: n.height, }; self.tree.new_text_leaf(*node_id, style, ctx).ok() }crates/grida-canvas/src/layout/tree.rs (2)
131-132: Remove unnecessary rebinding.
let provider = provider;is a no-op that doesn't affect behavior. This appears to be leftover from refactoring.🔎 Proposed fix
if let Some(provider) = measure_provider { - let provider = provider; self.taffy.compute_layout_with_measure(
159-162: Consider documenting the minimum 1.0 size constraint rationale.Clamping dimensions to
max(1.0)prevents zero-size nodes in layout, which is a reasonable guard against layout edge cases. However, this means empty text will occupy at least 1×1 pixels. Consider adding a comment explaining this design decision.🔎 Proposed documentation
+ // Ensure non-zero dimensions to prevent layout collapse. + // Taffy may have issues with zero-sized nodes in certain layouts. Size { width: width.max(1.0), height: height.max(1.0), }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
editor/public/assets/css-cursors-grida/bend-vertex-mirror-all-64-x12y12-000000.pngis excluded by!**/*.pngeditor/public/assets/css-cursors-grida/bend-vertex-mirror-all-64-x12y12-000000.svgis excluded by!**/*.svgeditor/public/assets/css-cursors-grida/lasso-64-x12y12-000000.pngis excluded by!**/*.pngeditor/public/assets/css-cursors-grida/lasso-64-x12y12-000000.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
crates/grida-canvas/examples/golden_layout_flex.rs(4 hunks)crates/grida-canvas/examples/golden_layout_flex_alignment.rs(2 hunks)crates/grida-canvas/examples/golden_layout_flex_padding.rs(1 hunks)crates/grida-canvas/examples/golden_layout_padding.rs(1 hunks)crates/grida-canvas/src/cache/geometry.rs(2 hunks)crates/grida-canvas/src/hittest/hit_tester.rs(7 hunks)crates/grida-canvas/src/layout/engine.rs(22 hunks)crates/grida-canvas/src/layout/tree.rs(6 hunks)crates/grida-canvas/src/runtime/scene.rs(2 hunks)crates/grida-canvas/src/window/application.rs(1 hunks)crates/grida-canvas/tests/dashed_stroke.rs(1 hunks)crates/grida-canvas/tests/hit_test.rs(1 hunks)crates/grida-canvas/tests/open_path_stroke_align.rs(1 hunks)editor/components/cursor/cursor-data.ts(1 hunks)editor/grida-canvas-react/provider.tsx(1 hunks)editor/grida-canvas-react/viewport/ui/point.tsx(1 hunks)editor/grida-canvas-react/viewport/ui/surface-vector-editor.tsx(2 hunks)editor/public/assets/css-cursors-grida/README.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
crates/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
crates/**/*.rs: Use Rust 2024 edition for wasm builds and graphics core
Use Skia graphics backend for 2D graphics, bound with skia-safe
Rust crates in /crates directory are under rapid development and serve as the new rendering backend; ensure high quality implementations
Files:
crates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/tests/open_path_stroke_align.rscrates/grida-canvas/examples/golden_layout_flex.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/layout/engine.rscrates/grida-canvas/src/hittest/hit_tester.rscrates/grida-canvas/examples/golden_layout_flex_padding.rscrates/grida-canvas/examples/golden_layout_padding.rscrates/grida-canvas/examples/golden_layout_flex_alignment.rscrates/grida-canvas/src/layout/tree.rscrates/grida-canvas/tests/dashed_stroke.rscrates/grida-canvas/tests/hit_test.rs
crates/grida-canvas/**/*.rs
📄 CodeRabbit inference engine (crates/grida-canvas/AGENTS.md)
crates/grida-canvas/**/*.rs: UseNodeId(u64) for internal structs (NodeRecs, SceneGraph, caches) in the rendering engine for high-performance operations
UseUserNodeId(String) for public APIs that accept or return node IDs for stability and serialization
Handle NodeId to UserNodeId conversion viaIdConverterduring .grida file loading
Auto-generate IDs (ID=0) inNodeRepositoryfor factory-created nodes
Maintain bidirectional mapping between NodeId and UserNodeId at the application layer for API boundary management
Useskia-safecrate for painting operations in the rendering engine
Usemath2crate for geometry and common math operations
Runcargo fmtto maintain code formatting standards
Runcargo clippy --no-deps --all-targets --all-featuresfor linting to check style, performance, and correctness suggestions
Files:
crates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/tests/open_path_stroke_align.rscrates/grida-canvas/examples/golden_layout_flex.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/layout/engine.rscrates/grida-canvas/src/hittest/hit_tester.rscrates/grida-canvas/examples/golden_layout_flex_padding.rscrates/grida-canvas/examples/golden_layout_padding.rscrates/grida-canvas/examples/golden_layout_flex_alignment.rscrates/grida-canvas/src/layout/tree.rscrates/grida-canvas/tests/dashed_stroke.rscrates/grida-canvas/tests/hit_test.rs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript 5 as the main language for most apps
Use Lucide or Radix Icons for icons
Files:
editor/grida-canvas-react/viewport/ui/surface-vector-editor.tsxeditor/grida-canvas-react/provider.tsxeditor/grida-canvas-react/viewport/ui/point.tsxeditor/components/cursor/cursor-data.ts
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use React.js 19 for web applications
Files:
editor/grida-canvas-react/viewport/ui/surface-vector-editor.tsxeditor/grida-canvas-react/provider.tsxeditor/grida-canvas-react/viewport/ui/point.tsx
{editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Files:
editor/grida-canvas-react/viewport/ui/surface-vector-editor.tsxeditor/grida-canvas-react/provider.tsxeditor/grida-canvas-react/viewport/ui/point.tsxeditor/components/cursor/cursor-data.ts
editor/grida-*/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to /packages
Files:
editor/grida-canvas-react/viewport/ui/surface-vector-editor.tsxeditor/grida-canvas-react/provider.tsxeditor/grida-canvas-react/viewport/ui/point.tsx
editor/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use /editor/components for generally reusable components
Files:
editor/components/cursor/cursor-data.ts
🧠 Learnings (23)
📚 Learning: 2025-12-20T08:11:16.201Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.201Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `NodeId` (u64) for internal structs (NodeRecs, SceneGraph, caches) in the rendering engine for high-performance operations
Applied to files:
crates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/examples/golden_layout_flex.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/layout/engine.rscrates/grida-canvas/src/hittest/hit_tester.rscrates/grida-canvas/examples/golden_layout_flex_padding.rscrates/grida-canvas/examples/golden_layout_padding.rscrates/grida-canvas/examples/golden_layout_flex_alignment.rscrates/grida-canvas/tests/dashed_stroke.rscrates/grida-canvas/tests/hit_test.rs
📚 Learning: 2025-12-20T08:11:16.201Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.201Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `skia-safe` crate for painting operations in the rendering engine
Applied to files:
crates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/tests/open_path_stroke_align.rscrates/grida-canvas/examples/golden_layout_flex.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/hittest/hit_tester.rscrates/grida-canvas/examples/golden_layout_flex_padding.rscrates/grida-canvas/examples/golden_layout_padding.rscrates/grida-canvas/examples/golden_layout_flex_alignment.rscrates/grida-canvas/tests/dashed_stroke.rscrates/grida-canvas/tests/hit_test.rs
📚 Learning: 2025-12-20T08:11:16.201Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.201Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo fmt` to maintain code formatting standards
Applied to files:
crates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/tests/open_path_stroke_align.rscrates/grida-canvas/examples/golden_layout_flex.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/hittest/hit_tester.rscrates/grida-canvas/examples/golden_layout_flex_padding.rscrates/grida-canvas/examples/golden_layout_padding.rscrates/grida-canvas/examples/golden_layout_flex_alignment.rscrates/grida-canvas/tests/dashed_stroke.rscrates/grida-canvas/tests/hit_test.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/src/runtime/scene.rscrates/grida-canvas/tests/open_path_stroke_align.rscrates/grida-canvas/tests/dashed_stroke.rscrates/grida-canvas/tests/hit_test.rs
📚 Learning: 2025-12-20T08:11:16.201Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.201Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `math2` crate for geometry and common math operations
Applied to files:
crates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/tests/open_path_stroke_align.rscrates/grida-canvas/examples/golden_layout_flex.rscrates/grida-canvas/src/window/application.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/hittest/hit_tester.rscrates/grida-canvas/examples/golden_layout_flex_padding.rscrates/grida-canvas/examples/golden_layout_padding.rscrates/grida-canvas/examples/golden_layout_flex_alignment.rscrates/grida-canvas/tests/dashed_stroke.rscrates/grida-canvas/tests/hit_test.rs
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to crates/**/*.rs : Rust crates in /crates directory are under rapid development and serve as the new rendering backend; ensure high quality implementations
Applied to files:
crates/grida-canvas/src/runtime/scene.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/**/scenario_*.rs : Scenario-specific comprehensive tests should be organized in files matching `scenario_*.rs`
Applied to files:
crates/grida-canvas/src/runtime/scene.rscrates/grida-canvas/tests/hit_test.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/open_path_stroke_align.rscrates/grida-canvas/src/cache/geometry.rseditor/grida-canvas-react/provider.tsxcrates/grida-canvas/src/hittest/hit_tester.rscrates/grida-canvas/tests/dashed_stroke.rseditor/components/cursor/cursor-data.ts
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to crates/**/*.rs : Use Skia graphics backend for 2D graphics, bound with skia-safe
Applied to files:
crates/grida-canvas/tests/open_path_stroke_align.rscrates/grida-canvas/src/cache/geometry.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/open_path_stroke_align.rscrates/grida-canvas/tests/dashed_stroke.rscrates/grida-canvas/tests/hit_test.rs
📚 Learning: 2025-12-20T08:11:16.201Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.201Z
Learning: Applies to crates/grida-canvas/**/*.rs : Run `cargo clippy --no-deps --all-targets --all-features` for linting to check style, performance, and correctness suggestions
Applied to files:
crates/grida-canvas/tests/open_path_stroke_align.rscrates/grida-canvas/examples/golden_layout_flex.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/hittest/hit_tester.rscrates/grida-canvas/examples/golden_layout_flex_padding.rscrates/grida-canvas/examples/golden_layout_flex_alignment.rscrates/grida-canvas/tests/dashed_stroke.rscrates/grida-canvas/tests/hit_test.rs
📚 Learning: 2025-12-20T08:11:16.201Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.201Z
Learning: Applies to crates/grida-canvas/**/*.rs : Maintain bidirectional mapping between NodeId and UserNodeId at the application layer for API boundary management
Applied to files:
crates/grida-canvas/tests/open_path_stroke_align.rscrates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/hittest/hit_tester.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.rscrates/grida-canvas/tests/dashed_stroke.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:
editor/grida-canvas-react/viewport/ui/surface-vector-editor.tsxeditor/grida-canvas-react/viewport/ui/point.tsx
📚 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 : When adding new shape types, update the Shape type union, add cases in drawShape() function, add cases in shapeToSVG() function, and add SelectItem in UI
Applied to files:
editor/grida-canvas-react/viewport/ui/surface-vector-editor.tsxeditor/grida-canvas-react/provider.tsxeditor/grida-canvas-react/viewport/ui/point.tsx
📚 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:
editor/grida-canvas-react/viewport/ui/surface-vector-editor.tsxeditor/grida-canvas-react/provider.tsxeditor/grida-canvas-react/viewport/ui/point.tsx
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to {editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}} : Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Applied to files:
editor/grida-canvas-react/viewport/ui/surface-vector-editor.tsxeditor/grida-canvas-react/provider.tsxeditor/grida-canvas-react/viewport/ui/point.tsx
📚 Learning: 2025-12-20T08:11:16.201Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.201Z
Learning: Applies to crates/grida-canvas/**/*.rs : Use `UserNodeId` (String) for public APIs that accept or return node IDs for stability and serialization
Applied to files:
crates/grida-canvas/src/cache/geometry.rscrates/grida-canvas/src/hittest/hit_tester.rs
📚 Learning: 2025-12-20T08:11:16.201Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.201Z
Learning: Applies to crates/grida-canvas/**/*.rs : Handle NodeId to UserNodeId conversion via `IdConverter` during .grida file loading
Applied to files:
crates/grida-canvas/src/cache/geometry.rs
📚 Learning: 2025-12-20T08:11:16.201Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: crates/grida-canvas/AGENTS.md:0-0
Timestamp: 2025-12-20T08:11:16.201Z
Learning: Applies to crates/grida-canvas/**/*.rs : Auto-generate IDs (ID=0) in `NodeRepository` for factory-created nodes
Applied to files:
crates/grida-canvas/src/hittest/hit_tester.rscrates/grida-canvas/examples/golden_layout_flex_padding.rscrates/grida-canvas/tests/dashed_stroke.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 React hooks for state management (imageSrc, shape, grid, maxRadius, gamma, jitter, opacity, color, customShapeImage, imageDataRef, sizeRef)
Applied to files:
editor/grida-canvas-react/viewport/ui/point.tsx
📚 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_level2_*.rs : Level 2+ placeholder and limitation tests should be organized in files matching `scenario_level2_*.rs`
Applied to files:
crates/grida-canvas/tests/hit_test.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/hit_test.rs
🧬 Code graph analysis (2)
crates/grida-canvas/src/window/application.rs (1)
crates/grida-canvas/src/hittest/hit_tester.rs (2)
with_graph(65-70)new(58-60)
crates/grida-canvas/src/layout/tree.rs (1)
crates/grida-canvas/src/cache/paragraph.rs (1)
paragraph(361-426)
🪛 LanguageTool
editor/public/assets/css-cursors-grida/README.md
[style] ~20-~20: Using many exclamation marks might seem excessive (in this case: 11 exclamation marks for a text that’s 3560 characters long)
Context: ... | 64 | x28 y28 | 000000 | 
| default-64-x28y28-000000.svg | | lasso | 64 | x12 y12 | 000000 | 
| lasso-64-x12y12-000000.svg | | bend-vertex-mirror-all | 64 | x12 y12 | 000000 | 
| bend-vertex-mirror-all-64-x12y12-000000.svg | | ew-resize | 64 | x32 y32 | 000000 | 
| ew-resize-64-x32y32-000000.svg | | ew-resize-scale | 64 | x32 y32 | 000000 | 
| ew-resize-scale-64-x32y32-000000.svg | | ns-resize | 64 | x32 y32 | 000000 | 
| ns-resize-64-x32y32-000000.svg | | ns-resize-scale | 64 | x32 y32 | 000000 | 
| ns-resize-scale-64-x32y32-000000.svg | | nesw-resize | 64 | x32 y32 | 000000 | 
| nesw-resize-64-x32y32-000000.svg | | nesw-resize-scale | 64 | x32 y32 | 000000 | 
| nesw-resize-scale-64-x32y32-000000.svg | | nwse-resize | 64 | x32 y32 | 000000 | 
| nwse-resize-64-x32y32-000000.svg | | nwse-resize-scale | 64 | x32 y32 | 000000 | 
🔇 Additional comments (26)
crates/grida-canvas/tests/open_path_stroke_align.rs (1)
3-3: LGTM! Good cleanup of unused import.Removing the unused
Pathimport is correct. ThePathtype is implicitly inferred frombuilder.detach()and only used via borrows, so the explicit import is unnecessary.crates/grida-canvas/src/cache/geometry.rs (1)
378-391: LGTM: Layout-aware text measurement integration.The logic correctly prioritizes layout-computed width for text measurement, ensuring consistency between the layout engine and paragraph cache. The fallback to node width maintains backward compatibility when no layout result is available.
editor/grida-canvas-react/viewport/ui/point.tsx (1)
59-59: LGTM! Subtle hover state refinement.The reduced border opacity (from 50% to 25%) provides a more subtle hover indication, which aligns with the removal of explicit cursor styles mentioned in the summary.
editor/grida-canvas-react/viewport/ui/surface-vector-editor.tsx (2)
479-479: LGTM! Cursor inheritance from parent context.Removing the explicit
cursor: "pointer"allows the Point to inherit the appropriate cursor from the parent context, which now provides tool-specific cursors (e.g., bend_vertex_png) via the provider.
647-649: LGTM! Conditional cursor for bend tool.The conditional cursor ensures that when the bend tool is active, vertex points inherit the custom bend cursor from the parent context. Otherwise, they fall back to default cursor behavior.
editor/public/assets/css-cursors-grida/README.md (1)
18-30: LGTM! Cursor documentation updated with new assets.The table has been reformatted with improved inline image display and now includes the new
lassoandbend-vertex-mirror-allcursor entries. The hotspot coordinates (x12 y12) are properly documented and align with the cursor-data.ts implementation.Note: The static analysis warning about "excessive exclamation marks" is a false positive—these are markdown image syntax (
![...]), not punctuation.editor/components/cursor/cursor-data.ts (1)
45-56: LGTM! New cursor exports follow established patterns.The
lasso_pngandbend_vertex_pngexports are correctly structured following the same pattern asdefault_png. The hotspot coordinates (12, 12) are appropriately divided by 2 inpngsetcssto produce the correct CSS hotspot values (6, 6).editor/grida-canvas-react/provider.tsx (1)
482-484: Both cursor PNG assets are present in the repository and correctly referenced in the code.
- ✓
/editor/public/assets/css-cursors-grida/lasso-64-x12y12-000000.pngexists- ✓
/editor/public/assets/css-cursors-grida/bend-vertex-mirror-all-64-x12y12-000000.pngexistsThe cursor mappings for the
lassoandbendtools are properly configured.crates/grida-canvas/tests/dashed_stroke.rs (1)
3-3: LGTM! Clean import optimization.Removing the unused
Pathimport while keepingPathBuilderis appropriate.crates/grida-canvas/examples/golden_layout_flex.rs (1)
142-142: LGTM! Correct API adaptation.All four
computecalls properly updated to passNonefor the new text measurement provider parameter. This is appropriate for layout examples that don't require text measurement.Also applies to: 202-202, 261-261, 329-329
crates/grida-canvas/examples/golden_layout_flex_padding.rs (1)
60-60: LGTM! Consistent API adaptation.The
computecall correctly includes the new text measurement provider parameter asNone.crates/grida-canvas/examples/golden_layout_padding.rs (1)
118-118: LGTM! Correct API adaptation.The
computecall properly includes the new text measurement provider parameter asNone.crates/grida-canvas/src/window/application.rs (1)
708-713: LGTM! Proper integration of graph-based clipping.The conditional construction correctly enables clipping checks when a scene graph is available. The pattern gracefully falls back to the basic
HitTester::newwhen no scene is present, ensuring the API remains resilient.crates/grida-canvas/examples/golden_layout_flex_alignment.rs (1)
136-136: LGTM! Consistent API adaptation.Both
computecalls correctly updated to include the new text measurement provider parameter asNone.Also applies to: 203-203
crates/grida-canvas/src/runtime/scene.rs (1)
389-399: LGTM! Clean text measurement integration.Both integration points properly use scoped blocks to manage the mutable borrow of
paragraph_cache. TheTextMeasureProviderconstruction correctly wires the paragraph cache and font repository into the layout computation. This pattern ensures borrow conflicts are avoided while enabling text measurement during layout.Also applies to: 458-468
crates/grida-canvas/tests/hit_test.rs (1)
130-207: LGTM! Comprehensive clipping test coverage.The new test thoroughly validates parent container clipping behavior:
- Properly configures the test scenario with
clip=trueand an overflowing child- Uses
HitTester::with_graphto enable graph-based clipping checks- Tests multiple boundary conditions: visible region, culled region, outside bounds, and edge cases
- Correctly uses
assert_ne!to verify that points in culled regions do not hit the clipped nodeThis test provides strong validation for the new clipping feature introduced in this PR.
crates/grida-canvas/src/layout/engine.rs (2)
76-108: LGTM!The text measurement integration into the
computemethod is well-designed. The optionalTextMeasureProviderparameter maintains backward compatibility while enabling text measurement when available. Themutbinding correctly allows reusing the provider across multiple root subtree computations viaas_mut().
399-406: LGTM!All test invocations correctly pass
Nonefor the text measurement provider, maintaining backward compatibility and ensuring existing layout tests continue to work without requiring font/paragraph infrastructure.crates/grida-canvas/src/hittest/hit_tester.rs (4)
8-31: LGTM - Good documentation of performance trade-offs.The TODO comment clearly documents the current O(depth × candidates) complexity and proposes a concrete optimization path (pre-computing effective clip bounds in GeometryCache). This is valuable for future maintainability.
77-113: LGTM!The parent clipping traversal correctly walks up the hierarchy checking only
Containernodes withclip = true. The early-return pattern efficiently short-circuits when a point fails any clip check. Based on learnings, usingNodeId(u64) for internal operations aligns with coding guidelines.
116-131: LGTM!The clipping check is correctly placed after the bounds containment check, ensuring the more expensive parent traversal only occurs for nodes that pass initial spatial filtering. This maintains good performance characteristics.
228-242:containsmethod lacks clipping check - verify if intentional.Unlike other hit testing methods (
hit_first,hits, etc.), thecontainsmethod does not apply parent clipping checks. This means callingcontains(id, point)will returntrueeven if the node is clipped by a parent container.If this is intentional (e.g., for use cases where you want to know if a specific node geometrically contains a point regardless of visibility), consider adding a doc comment clarifying this behavior difference.
crates/grida-canvas/src/layout/tree.rs (4)
8-31: LGTM - Well-structured types for text measurement.The
TextMeasureContextcaptures all necessary text properties for measurement, andTextMeasureProviderproperly encapsulates the paragraph cache and font repository dependencies. Thepub(crate)visibility is appropriate for internal module use.
78-98: LGTM!The
new_text_leafmethod correctly creates a Taffy leaf with context and maintains bidirectional mapping consistency. The mapping cleanup logic is appropriately duplicated to ensure each method is self-contained.
163-168: LGTM!The fallback for non-text nodes correctly returns Taffy's known dimensions or zero. This handles edge cases where a node enters the measure callback without text context.
289-297: LGTM!Test correctly updated to pass
Nonefor the measure provider, maintaining existing test behavior without requiring text measurement infrastructure.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Node::Container(n) => { | ||
| if n.clip { | ||
| // Check if the point is within this container's bounds | ||
| if let Some(bounds) = self.cache.geometry.get_world_bounds(&id) { | ||
| if !rect::contains_point(&bounds, point) { | ||
| // Point is outside this clipping container's bounds |
There was a problem hiding this comment.
is_point_within_parent_clip_bounds uses get_world_bounds for container culling, but containers’ world bounds are stored as the union of their own box plus all children (see the container case in geometry.rs where union_world_bounds accumulates child bounds). That means a clipped container that has an overflowing child expands its bounds to the child’s extent, so rect::contains_point returns true even for points outside the container’s clipping area and the child still registers in hit tests. For example, a clip=true 100×100 container with a 100×100 child at (50,50) will still hit at (120,120) even though that region should be culled. The clip check needs to use the container’s own render/layout box instead of the union of descendants.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
editor/scaffolds/sidecontrol/controls/font-family.tsx (1)
207-222: Consider displaying the popular fonts count for UI consistency.Other category options display item counts (e.g., "All fonts (1234)"), but "Popular" doesn't. Consider adding the count for consistency:
🔎 Suggested change
- <NativeSelectOption value="popular">Popular</NativeSelectOption> + <NativeSelectOption value="popular"> + Popular ({(popularFonts || []).filter((name) => availableFontSet.has(name)).length}) + </NativeSelectOption>Note: This would require passing
popularFontsandavailableFontSetto the JSX scope, or computing the count separately.editor/hooks/use-grida-fonts-search.ts (1)
128-139: Potential SWR cache key instability.The
useMemoreturns["grida-fonts-search", options], butoptionsis the original object reference passed to the hook, not a new stable object. SWR uses referential equality for key comparison, so if the caller doesn't memoize the options object, it will trigger refetches.Consider creating a stable key from the individual properties:
🔎 Suggested fix
// Create a stable key for SWR caching const key = useMemo( - () => ["grida-fonts-search", options], + () => [ + "grida-fonts-search", + options.q, + options.category, + options.property, + options.sort, + options.page, + options.limit, + ], [ options.q, options.category, options.property, options.sort, options.page, options.limit, ] );Then update the fetcher to reconstruct options from the key array.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
editor/grida-canvas-hosted/library/icons-browser.tsx(1 hunks)editor/grida-canvas-hosted/library/icons-logos-browser.tsx(1 hunks)editor/hooks/use-grida-fonts-search.ts(1 hunks)editor/scaffolds/sidecontrol/controls/font-family.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript 5 as the main language for most apps
Use Lucide or Radix Icons for icons
Files:
editor/grida-canvas-hosted/library/icons-logos-browser.tsxeditor/hooks/use-grida-fonts-search.tseditor/grida-canvas-hosted/library/icons-browser.tsxeditor/scaffolds/sidecontrol/controls/font-family.tsx
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use React.js 19 for web applications
Files:
editor/grida-canvas-hosted/library/icons-logos-browser.tsxeditor/grida-canvas-hosted/library/icons-browser.tsxeditor/scaffolds/sidecontrol/controls/font-family.tsx
{editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Files:
editor/grida-canvas-hosted/library/icons-logos-browser.tsxeditor/hooks/use-grida-fonts-search.tseditor/grida-canvas-hosted/library/icons-browser.tsxeditor/scaffolds/sidecontrol/controls/font-family.tsx
editor/grida-*/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to /packages
Files:
editor/grida-canvas-hosted/library/icons-logos-browser.tsxeditor/grida-canvas-hosted/library/icons-browser.tsx
editor/scaffolds/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use /editor/scaffolds for feature-specific larger components, pages, and editors
Files:
editor/scaffolds/sidecontrol/controls/font-family.tsx
🧠 Learnings (8)
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to editor/grida-*/**/*.{ts,tsx} : Use /editor/grida-* directories to isolate domain-specific modules; promote well-defined modules to <root>/packages
Applied to files:
editor/hooks/use-grida-fonts-search.tseditor/scaffolds/sidecontrol/controls/font-family.tsx
📚 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:
editor/hooks/use-grida-fonts-search.ts
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to packages/grida-canvas-*/**/*.{ts,tsx,js,jsx} : Packages under /packages/grida-canvas-* power the canvas; some are published to npm, refer to individual package README
Applied to files:
editor/hooks/use-grida-fonts-search.tseditor/scaffolds/sidecontrol/controls/font-family.tsx
📚 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 : When adding new shape types, update the Shape type union, add cases in drawShape() function, add cases in shapeToSVG() function, and add SelectItem in UI
Applied to files:
editor/hooks/use-grida-fonts-search.ts
📚 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 React hooks for state management (imageSrc, shape, grid, maxRadius, gamma, jitter, opacity, color, customShapeImage, imageDataRef, sizeRef)
Applied to files:
editor/scaffolds/sidecontrol/controls/font-family.tsx
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to {editor/**/*.{ts,tsx},packages/grida-canvas-*/**/*.{ts,tsx}} : Use DOM (plain DOM as canvas) for website builder canvas, bound with React
Applied to files:
editor/scaffolds/sidecontrol/controls/font-family.tsx
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to **/components/ui/**/*.{ts,tsx} : Use Shadcn UI for UI component library
Applied to files:
editor/scaffolds/sidecontrol/controls/font-family.tsx
📚 Learning: 2025-12-14T23:30:42.112Z
Learnt from: CR
Repo: gridaco/grida PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T23:30:42.112Z
Learning: Applies to editor/components/ui/**/*.{ts,tsx} : Use /editor/components/ui for shadcn UI components
Applied to files:
editor/scaffolds/sidecontrol/controls/font-family.tsx
🧬 Code graph analysis (1)
editor/scaffolds/sidecontrol/controls/font-family.tsx (3)
editor/grida-canvas/query/index.ts (1)
fonts(606-614)editor/hooks/use-grida-fonts-search.ts (1)
useGridaFontsSearch(125-191)editor/components/ui-editor/popover.tsx (1)
PopoverContent(62-62)
⏰ 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). (3)
- GitHub Check: cargo test
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (13)
editor/grida-canvas-hosted/library/icons-logos-browser.tsx (1)
255-255: LGTM: Visual separator added.The horizontal rule provides clear visual separation between the search input and filter controls, improving the header's visual hierarchy.
editor/grida-canvas-hosted/library/icons-browser.tsx (1)
416-416: LGTM: Visual separator added.The horizontal rule provides clear visual separation between the search input and vendor/variant controls. This change mirrors the separator added in the logos browser, maintaining consistency across the icon library UI.
editor/scaffolds/sidecontrol/controls/font-family.tsx (7)
24-24: LGTM!The new import for the custom font search hook aligns with the feature requirements.
91-100: LGTM!The optional
popularFontsprop is correctly typed and integrated into the component signature.
121-123: LGTM!The category type union is correctly expanded to include the new "popular" option.
125-162: LGTM!The filtering logic is well-structured:
availableFontSetprovides O(1) lookup for popular font validation- The category-based filtering with subsequent query filtering is efficient
- Dependencies are correctly specified for both
useMemocalls
261-261: LGTM!Adding the
titleattribute improves accessibility by providing a tooltip with the font name.
294-301: LGTM!The popular fonts integration is well implemented. SWR's caching ensures the API call is deduplicated across renders, and the
useMemoefficiently extracts family names only when the data changes.
323-328: LGTM!The
collisionPadding={8}prop improves layout behavior by preventing the popover from being clipped at viewport edges.editor/hooks/use-grida-fonts-search.ts (4)
1-4: LGTM!The
"use client"directive is correctly placed for a hook that uses SWR, and imports are minimal and appropriate.
82-113: LGTM!The
searchGridaFontsfunction is well-implemented with sensible defaults and proper error handling.
141-190: LGTM!The SWR integration and result transformation are well-implemented with proper error normalization and sensible default values during loading states.
23-31: AlignGridaFontsSearchFilters.categorytype with search API capabilities.Google Fonts supports five categories: serif, sans-serif, display, handwriting, and monospace. However,
GridaFont.categoryincludes all five whileGridaFontsSearchFilters.categoryandUseGridaFontsSearchOptions.categoryonly support four (excluding "handwriting"). The code references the Grida Fonts search API, which appears to intentionally limit filtering to the four supported categories. Either document this limitation with a comment explaining why "handwriting" is excluded from the search filter, or align the types by adding "handwriting" if the backend now supports it.
1 participant
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.