feat(canvas): headless Editor for non-browser testing#630
Merged
softmarshmallow merged 3 commits intomainfrom Apr 6, 2026
Merged
feat(canvas): headless Editor for non-browser testing#630softmarshmallow merged 3 commits intomainfrom
softmarshmallow merged 3 commits intomainfrom
Conversation
Abstract browser dependencies behind injectable interfaces so the
Editor can be fully instantiated and exercised in pure Node.js/Vitest:
- Add editor.api.IViewportApi, editor.api.events.{IModifiers,
IPointerEvent, IKeyboardEvent, IFocusEvent} as platform-neutral
event IRs (browser DOM events satisfy them via duck typing)
- Add HeadlessViewportApi, NoopPropertiesQueryProvider backends
- Add Editor.createHeadless() static factory
- Make Editor constructor accept viewportApi/properties_query injection
- Fix writeClipboardMedia to route through ui.clipboard with fallback
- Extract shared test utilities into __tests__/utils/
- Add 8 headless test files (gate + behavioral: lifecycle, selection,
undo-redo, camera, subscription, surface-tools, node-properties)
- Rewrite history, select-readonly, move-tray tests to use headless
Editor instead of raw reducer calls with copy-pasted stubs
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
WalkthroughThis PR introduces comprehensive infrastructure for headless editor testing, adds Skia GPU benchmarking examples, and generalizes documentation to remove scene-specific fixture references. Changes include platform-neutral event and viewport abstractions enabling the editor to run in Node.js/Vitest environments, test utilities and suites validating headless editor behavior across camera, selection, lifecycle, and UI operations, and Rust benchmark examples measuring rendering performance differences between rect/rrect primitives and text LOD strategies. Changes
Sequence DiagramsequenceDiagram
participant Test as Headless Test
participant Factory as createHeadlessEditor()
participant Backend as HeadlessViewportApi
participant Editor as Editor Instance
participant Camera as Camera
participant Surface as EditorSurface
Test->>Factory: Call with options
Factory->>Backend: Create viewport (1920x1080)
Factory->>Editor: Editor.createHeadless() with backends
Editor->>Editor: Initialize with HeadlessViewportApi
Editor->>Camera: Inject IViewportApi
Editor->>Surface: Setup with noop geometry/properties
Factory-->>Test: Return configured Editor
Test->>Editor: ed.doc.select(nodeId)
Editor->>Editor: Dispatch action
Editor->>Editor: Notify subscribers
Test->>Test: Assert ed.state.selection updated
Test->>Editor: ed.camera.zoom(factor, center)
Editor->>Camera: Apply zoom transform
Camera->>Backend: Query viewport size
Backend-->>Camera: Return viewport bounds
Camera->>Camera: Update transform
Test->>Test: Assert camera.transform.scaleX changed
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly Related PRs
Suggested Labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| */ | ||
| import { Editor } from "@/grida-canvas/editor"; | ||
| import type { editor } from "@/grida-canvas"; | ||
| import { MINIMAL_DOCUMENT, createDocumentWithRects } from "./fixtures"; |
Add research artifacts for zoom-aware Level-of-Detail rendering: - LOD property reference sheet (lod-properties.md): catalogs 50+ per-node properties across 12 categories (geometry, stroke, path, effects, text, fills, clip/mask, container, render-surface, devtools) where zoom-indexed LOD decisions can reduce per-frame work. - Skia RRect vs Rect cost probe (skia_bench_rrect_vs_rect.rs): measures GPU dispatch cost across radii and scales. Key finding: on Apple M2 Metal, sub-pixel rrect radii are 0.72-0.84x the cost of drawRect — the analytic AA shader short-circuits efficiently. Manual rrect→rect collapse would regress performance on this backend. - Skia text paragraph paint cost probe (skia_bench_text_lod.rs): measures paragraph.paint() vs drawRect across font sizes. Key finding: paragraph paint is 2.4-6x more expensive than drawRect at all sizes (0.8 µs/node at small sizes, 2.1 µs at 48px). Greeking text at low zoom is a validated optimization path. - optimization.md items 51-52: design specs for subpixel LOD culling and text LOD (cull + greek) with measured impact data from the 136K-node fixture. Both designed and prototyped but not shipped.
softmarshmallow
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
editor/grida-canvas/__tests__/headless/undo-redo.test.ts (1)
56-58: DoublemockReturnValueOncemay be fragile if implementation changes.The test mocks
Date.now()twice for a singleselect()call (lines 56-57). While this works with the current implementation, it creates implicit coupling to internal call patterns. Consider usingmockReturnValue(2000)for the duration of that action, then resetting:♻️ Suggested approach
// Second action at t=2000 — well outside the 300ms merge window - nowSpy.mockReturnValueOnce(2000); - nowSpy.mockReturnValueOnce(2000); + nowSpy.mockReturnValue(2000); ed.doc.select(["rect-1"]); expect(ed.state.selection).toEqual(["rect-1"]); + nowSpy.mockReset(); // Reset for subsequent calls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/__tests__/headless/undo-redo.test.ts` around lines 56 - 58, The test currently calls nowSpy.mockReturnValueOnce(2000) twice before ed.doc.select(["rect-1"]), which couples the test to the number of internal Date.now() calls; replace the pair of mockReturnValueOnce calls by a single persistent mockReturnValue(2000) for the duration of the action and then reset/restore the spy afterward (use nowSpy.mockReturnValue(2000) before calling ed.doc.select(...) and nowSpy.mockRestore() or nowSpy.mockReset() after) so the test no longer relies on internal call counts of Date.now while still providing deterministic timestamps.editor/grida-canvas/reducers/__tests__/history.test.ts (1)
125-127: Same doublemockReturnValueOncepattern as inundo-redo.test.ts.Consider extracting a shared test helper for time-controlled history operations to reduce duplication and ensure consistency:
// In __tests__/utils/index.ts or similar function withMockedTime(spy: SpyInstance, time: number, fn: () => void) { spy.mockReturnValue(time); fn(); spy.mockReset(); }Also applies to: 154-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/reducers/__tests__/history.test.ts` around lines 125 - 127, The tests duplicate nowSpy.mockReturnValueOnce calls around operations like ed.doc.select(["rect2"]) (and lines 154-156) — extract a shared helper (e.g., withMockedTime or setMockTime) in the test utils to centralize time mocking, call it before performing history-affecting operations (referencing nowSpy and calls such as ed.doc.select and other history tests), and replace the double mockReturnValueOnce occurrences with a single helper invocation that sets the fake time, executes the operation, and resets the spy to keep tests DRY and consistent.editor/grida-canvas/reducers/methods/__tests__/move-tray.test.ts (1)
184-187: Strengthen cycle-rejection verification to ensure no secondary parent link is created.Right now the test only proves
tray1remains underscene. Add a negative assertion ontray2so dual-parent regressions are caught.Suggested assertion hardening
// tray1 should still be at scene root expect(ed.state.document.links["scene"]).toContain("tray1"); + expect(ed.state.document.links["tray2"] ?? []).not.toContain("tray1");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/reducers/methods/__tests__/move-tray.test.ts` around lines 184 - 187, The test calling ed.doc.mv(["tray1"], "tray2") currently only asserts ed.state.document.links["scene"] contains "tray1"; add a negative assertion to ensure no secondary parent link was created by verifying ed.state.document.links["tray2"] does NOT contain "tray1" (or is undefined/empty), so that dual-parent regressions are detected; update the test around ed.doc.mv(["tray1"], "tray2") to include expect(ed.state.document.links["tray2"]).not.toContain("tray1") (or equivalent emptiness check).editor/grida-canvas/__tests__/headless/surface-tools.test.ts (1)
89-91: Note: Test correctly reflects existing typo in source.The property name
tarnslate_with_axis_lockhas a typo (should betranslate_with_axis_lock). The test is correct in matching the current state shape, but this typo exists ineditor.i.tsat theGestureModifiersinterface definition. Consider fixing the typo at the source as a separate cleanup task.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/__tests__/headless/surface-tools.test.ts` around lines 89 - 91, The GestureModifiers property name contains a typo: `tarnslate_with_axis_lock` should be `translate_with_axis_lock`; update the interface definition in GestureModifiers (editor.i.ts) and all code that reads/writes it (e.g., surfaceConfigureTranslateWithAxisLockModifier, any accesses to ed.state.gesture_modifiers.tarnslate_with_axis_lock) to use `translate_with_axis_lock` consistently, and update tests (like surface-tools.test.ts) to assert the corrected property name; ensure any serialization/migration logic handles the rename if needed.editor/grida-canvas/__tests__/headless/camera.test.ts (1)
97-109: Consider usingmockPointerEventfactory for consistency.The inline pointer event object duplicates logic available in the
mockPointerEventfactory from./utils/factories.ts. Using the factory would improve consistency across tests.♻️ Suggested refactor
+import { mockPointerEvent } from "@/grida-canvas/__tests__/utils"; test("pointerEventToViewportPoint extracts correct coords", () => { - const point = ed.camera.pointerEventToViewportPoint({ - clientX: 500, - clientY: 300, - button: 0, - shiftKey: false, - ctrlKey: false, - metaKey: false, - altKey: false, - }); + const point = ed.camera.pointerEventToViewportPoint( + mockPointerEvent({ clientX: 500, clientY: 300 }) + ); expect(point.x).toBe(500); expect(point.y).toBe(300); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/__tests__/headless/camera.test.ts` around lines 97 - 109, The test "pointerEventToViewportPoint extracts correct coords" duplicates an inline pointer event object; replace it with the existing mockPointerEvent factory from ./utils/factories.ts to keep tests consistent. Update the test to import mockPointerEvent and call ed.camera.pointerEventToViewportPoint(mockPointerEvent({ clientX: 500, clientY: 300, button: 0, shiftKey: false, ctrlKey: false, metaKey: false, altKey: false })) and assert the same expectations; ensure the import for mockPointerEvent is added to the test file.editor/grida-canvas/__tests__/utils/fixtures.ts (1)
31-31: Consider using a stricter type instead ofany.The
nodesrecord usesanywhich loses type safety. Consider using the node type for better compile-time checking:- const nodes: Record<string, any> = { + const nodes: Record<string, grida.program.nodes.Node> = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/__tests__/utils/fixtures.ts` at line 31, The tests declare nodes as Record<string, any>, which loses type safety; change the declaration of nodes to use the actual node type (e.g. Record<string, Node> or Record<string, CanvasNode>) or a stricter union/partial of that interface instead of any, importing or defining the Node/CanvasNode type used by the canvas code and updating any test fixtures to conform to that type so the compiler enforces correct properties.editor/grida-canvas/editor.ts (1)
5211-5222: Consider guardingClipboardItemconstruction for robustness in edge environments.The
ClipboardItemconstructor is called unconditionally at line 5212 before the clipboard availability checks. While this practically works becauseexportNodeAs()would fail first in headless mode (no WASM mounted), the construction could fail in edge environments whereClipboardItemis not defined but other parts work.For defensive coding, consider checking for
ClipboardItemavailability before constructing it:♻️ Optional defensive guard
const blob = new Blob([data as BlobPart], { type: "image/png" }); + if (typeof ClipboardItem === "undefined") { + return false; + } const item = new ClipboardItem({ "image/png": blob }); if (this.ui.clipboard) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@editor/grida-canvas/editor.ts` around lines 5211 - 5222, The ClipboardItem is being constructed unconditionally (const blob = new Blob(...); const item = new ClipboardItem(...)) before verifying clipboard APIs; guard the construction by first checking for ClipboardItem availability (e.g., typeof ClipboardItem !== "undefined") and that a writable clipboard API exists (this.ui.clipboard or navigator.clipboard?.write), then create the Blob/ClipboardItem only when supported and proceed with this.ui.clipboard.write or navigator.clipboard.write; update the code paths around ClipboardItem, blob, and the clipboard write checks to avoid constructing ClipboardItem in environments where it may be undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/grida-canvas/examples/skia_bench/skia_bench_text_lod.rs`:
- Around line 13-17: The header doc claims the benchmark varies font size,
number of glyphs per paragraph, and number of paragraphs per frame, but the code
uses a fixed sample_text and a fixed count (e.g., sample_text and count in
skia_bench_text_lod.rs), so update the header to reflect current behavior or
implement the sweeps; specifically either change the top comment to state that
sample_text and count are fixed and only font size is varied, or modify the
benchmark loop that constructs paragraphs (the use of sample_text and the
variable count) to generate varying glyph counts and paragraph counts
(parameterize sample_text selection/substring and the count variable and iterate
over those ranges) and adjust any related rendering logic to consume those
ranges.
- Around line 157-160: The baseline-subtracted timings paint_net and rect_net
can be negative due to timer noise, producing misleading negative per-node
metrics; clamp paint_net and rect_net to non-negative values before computing
ratio and per_node_us (e.g., replace paint_net and rect_net uses with
paint_net_clamped = paint_net.max(0) and rect_net_clamped = rect_net.max(1) or
similar), then compute ratio = paint_net_clamped as f64 / rect_net_clamped as
f64 and per_node_us = paint_net_clamped as f64 / (count as f64) while also
ensuring count > 0 is handled to avoid divide-by-zero.
- Around line 84-142: The benchmark places paragraphs using a fixed 20.0 spacing
which causes overlap because actual paragraph size comes from para.max_width()
and para.height(); update the grid placement logic around the loops that call
para.paint(...) and draw_rect(...) to use spacing derived from para_w and para_h
(e.g., let h_spacing = para_w + padding; let v_spacing = para_h + padding) and
compute x/y with those spacings (replace 20.0 and the hardcoded 40 column math
with a cols variable computed from count or surface width and use (i % cols) as
f32 * h_spacing and (i / cols) as f32 * v_spacing) so paragraphs do not overlap
and the paint vs rect comparison measures per-node cost correctly.
In `@docs/wg/feat-2d/lod-properties.md`:
- Line 210: The sentence containing the fragment "analytic rrect shader" has
extra whitespace; locate that exact text in docs/wg/feat-2d/lod-properties.md
and collapse the multiple spaces to a single space so it reads "analytic rrect
shader" (and while there, verify the word "rrect" is intended—if it's a typo,
change to "rect" so the final text reads "analytic rect shader").
- Around line 17-18: Update the stale item-number references that point to
optimization.md: change the reference for "Subpixel LOD Culling" from item 51 to
item 52 and change the reference for "Text LOD" to item 53 wherever they appear
in this document (the occurrences referencing "Subpixel LOD Culling" and "Text
LOD" — including the other two blocks mentioned) so the cross-links match the
actual items in optimization.md.
---
Nitpick comments:
In `@editor/grida-canvas/__tests__/headless/camera.test.ts`:
- Around line 97-109: The test "pointerEventToViewportPoint extracts correct
coords" duplicates an inline pointer event object; replace it with the existing
mockPointerEvent factory from ./utils/factories.ts to keep tests consistent.
Update the test to import mockPointerEvent and call
ed.camera.pointerEventToViewportPoint(mockPointerEvent({ clientX: 500, clientY:
300, button: 0, shiftKey: false, ctrlKey: false, metaKey: false, altKey: false
})) and assert the same expectations; ensure the import for mockPointerEvent is
added to the test file.
In `@editor/grida-canvas/__tests__/headless/surface-tools.test.ts`:
- Around line 89-91: The GestureModifiers property name contains a typo:
`tarnslate_with_axis_lock` should be `translate_with_axis_lock`; update the
interface definition in GestureModifiers (editor.i.ts) and all code that
reads/writes it (e.g., surfaceConfigureTranslateWithAxisLockModifier, any
accesses to ed.state.gesture_modifiers.tarnslate_with_axis_lock) to use
`translate_with_axis_lock` consistently, and update tests (like
surface-tools.test.ts) to assert the corrected property name; ensure any
serialization/migration logic handles the rename if needed.
In `@editor/grida-canvas/__tests__/headless/undo-redo.test.ts`:
- Around line 56-58: The test currently calls nowSpy.mockReturnValueOnce(2000)
twice before ed.doc.select(["rect-1"]), which couples the test to the number of
internal Date.now() calls; replace the pair of mockReturnValueOnce calls by a
single persistent mockReturnValue(2000) for the duration of the action and then
reset/restore the spy afterward (use nowSpy.mockReturnValue(2000) before calling
ed.doc.select(...) and nowSpy.mockRestore() or nowSpy.mockReset() after) so the
test no longer relies on internal call counts of Date.now while still providing
deterministic timestamps.
In `@editor/grida-canvas/__tests__/utils/fixtures.ts`:
- Line 31: The tests declare nodes as Record<string, any>, which loses type
safety; change the declaration of nodes to use the actual node type (e.g.
Record<string, Node> or Record<string, CanvasNode>) or a stricter union/partial
of that interface instead of any, importing or defining the Node/CanvasNode type
used by the canvas code and updating any test fixtures to conform to that type
so the compiler enforces correct properties.
In `@editor/grida-canvas/editor.ts`:
- Around line 5211-5222: The ClipboardItem is being constructed unconditionally
(const blob = new Blob(...); const item = new ClipboardItem(...)) before
verifying clipboard APIs; guard the construction by first checking for
ClipboardItem availability (e.g., typeof ClipboardItem !== "undefined") and that
a writable clipboard API exists (this.ui.clipboard or
navigator.clipboard?.write), then create the Blob/ClipboardItem only when
supported and proceed with this.ui.clipboard.write or navigator.clipboard.write;
update the code paths around ClipboardItem, blob, and the clipboard write checks
to avoid constructing ClipboardItem in environments where it may be undefined.
In `@editor/grida-canvas/reducers/__tests__/history.test.ts`:
- Around line 125-127: The tests duplicate nowSpy.mockReturnValueOnce calls
around operations like ed.doc.select(["rect2"]) (and lines 154-156) — extract a
shared helper (e.g., withMockedTime or setMockTime) in the test utils to
centralize time mocking, call it before performing history-affecting operations
(referencing nowSpy and calls such as ed.doc.select and other history tests),
and replace the double mockReturnValueOnce occurrences with a single helper
invocation that sets the fake time, executes the operation, and resets the spy
to keep tests DRY and consistent.
In `@editor/grida-canvas/reducers/methods/__tests__/move-tray.test.ts`:
- Around line 184-187: The test calling ed.doc.mv(["tray1"], "tray2") currently
only asserts ed.state.document.links["scene"] contains "tray1"; add a negative
assertion to ensure no secondary parent link was created by verifying
ed.state.document.links["tray2"] does NOT contain "tray1" (or is
undefined/empty), so that dual-parent regressions are detected; update the test
around ed.doc.mv(["tray1"], "tray2") to include
expect(ed.state.document.links["tray2"]).not.toContain("tray1") (or equivalent
emptiness check).
🪄 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: b0ee5cb3-f332-410b-b2b6-8acef8dc07a4
📒 Files selected for processing (32)
.agents/skills/cg-perf/SKILL.mdcrates/grida-canvas/Cargo.tomlcrates/grida-canvas/examples/skia_bench/skia_bench_rrect_vs_rect.rscrates/grida-canvas/examples/skia_bench/skia_bench_text_lod.rscrates/grida-canvas/src/runtime/scene.rsdocs/wg/feat-2d/lod-properties.mddocs/wg/feat-2d/optimization.mddocs/wg/feat-2d/wasm-benchmarking.mddocs/wg/feat-2d/wasm-load-scene-optimization.mdeditor/grida-canvas/__tests__/headless-gate.test.tseditor/grida-canvas/__tests__/headless/camera.test.tseditor/grida-canvas/__tests__/headless/lifecycle.test.tseditor/grida-canvas/__tests__/headless/node-properties.test.tseditor/grida-canvas/__tests__/headless/selection.test.tseditor/grida-canvas/__tests__/headless/subscription.test.tseditor/grida-canvas/__tests__/headless/surface-tools.test.tseditor/grida-canvas/__tests__/headless/undo-redo.test.tseditor/grida-canvas/__tests__/utils/create-headless-editor.tseditor/grida-canvas/__tests__/utils/factories.tseditor/grida-canvas/__tests__/utils/fixtures.tseditor/grida-canvas/__tests__/utils/index.tseditor/grida-canvas/__tests__/utils/stubs.tseditor/grida-canvas/backends/dom.tseditor/grida-canvas/backends/headless.tseditor/grida-canvas/backends/index.tseditor/grida-canvas/backends/noop.tseditor/grida-canvas/commands/text-edit.tseditor/grida-canvas/editor.i.tseditor/grida-canvas/editor.tseditor/grida-canvas/reducers/__tests__/history.test.tseditor/grida-canvas/reducers/__tests__/select-readonly.test.tseditor/grida-canvas/reducers/methods/__tests__/move-tray.test.ts
Comment on lines
+13
to
+17
| //! The test varies: | ||
| //! - font size (in device pixels after projection) | ||
| //! - number of glyphs per paragraph | ||
| //! - number of paragraphs per frame | ||
| //! |
There was a problem hiding this comment.
Header docs describe parameter sweeps that are not implemented.
The comment says glyph count and paragraph count vary, but this benchmark uses fixed sample_text and fixed count. Please either implement those sweeps or narrow the comment to match current behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/examples/skia_bench/skia_bench_text_lod.rs` around lines
13 - 17, The header doc claims the benchmark varies font size, number of glyphs
per paragraph, and number of paragraphs per frame, but the code uses a fixed
sample_text and a fixed count (e.g., sample_text and count in
skia_bench_text_lod.rs), so update the header to reflect current behavior or
implement the sweeps; specifically either change the top comment to state that
sample_text and count are fixed and only font size is varied, or modify the
benchmark loop that constructs paragraphs (the use of sample_text and the
variable count) to generate varying glyph counts and paragraph counts
(parameterize sample_text selection/substring and the count variable and iterate
over those ranges) and adjust any related rendering logic to consume those
ranges.
Comment on lines
+84
to
+142
| println!("Each test: 1000 paragraphs per frame, grid-positioned, non-overlapping."); | ||
| println!("Paragraphs pre-shaped (paint-only cost measured)."); | ||
| println!(); | ||
|
|
||
| let count = 1000usize; | ||
| let font_sizes: &[f32] = &[0.25, 0.5, 1.0, 2.0, 4.0, 6.0, 8.0, 12.0, 16.0, 24.0, 48.0]; | ||
|
|
||
| // Warmup: run a big paragraph paint to compile shaders + prime atlas | ||
| { | ||
| let para = build_paragraph(&fc, sample_text, 16.0, 300.0); | ||
| for _ in 0..20 { | ||
| let canvas = surface.canvas(); | ||
| canvas.clear(skia_safe::Color::WHITE); | ||
| para.paint(canvas, (10.0, 10.0)); | ||
| flush_gpu(surface); | ||
| } | ||
| } | ||
|
|
||
| println!( | ||
| "{:>10} {:>12} {:>12} {:>12} {:>12} {:>10}", | ||
| "font(px)", "paint(us)", "rect(us)", "skip(us)", "paint/rect", "per-node" | ||
| ); | ||
| println!("{}", "─".repeat(78)); | ||
|
|
||
| for &font_size in font_sizes { | ||
| // Build paragraph once per font size — pre-shaped so paint() is measured. | ||
| let para = build_paragraph(&fc, sample_text, font_size, 300.0); | ||
| let para_h = para.height(); | ||
| let para_w = para.max_width(); | ||
|
|
||
| // Test 1: N × paragraph.paint() | ||
| flush_gpu(surface); | ||
| let start = Instant::now(); | ||
| for _ in 0..n_iter { | ||
| let canvas = surface.canvas(); | ||
| canvas.clear(skia_safe::Color::WHITE); | ||
| for i in 0..count { | ||
| let x = (i % 40) as f32 * 20.0; | ||
| let y = (i / 40) as f32 * 20.0; | ||
| para.paint(canvas, (x, y)); | ||
| } | ||
| flush_gpu(surface); | ||
| } | ||
| let paint_us = (start.elapsed() / n_iter as u32).as_micros(); | ||
|
|
||
| // Test 2: N × drawRect (greek) | ||
| let mut paint_obj = skia_safe::Paint::default(); | ||
| paint_obj.set_color(skia_safe::Color::from_argb(180, 80, 80, 80)); | ||
| paint_obj.set_anti_alias(false); // greeking doesn't need AA | ||
| flush_gpu(surface); | ||
| let start = Instant::now(); | ||
| for _ in 0..n_iter { | ||
| let canvas = surface.canvas(); | ||
| canvas.clear(skia_safe::Color::WHITE); | ||
| for i in 0..count { | ||
| let x = (i % 40) as f32 * 20.0; | ||
| let y = (i / 40) as f32 * 20.0; | ||
| canvas.draw_rect(skia_safe::Rect::from_xywh(x, y, para_w, para_h), &paint_obj); | ||
| } |
There was a problem hiding this comment.
Benchmark geometry currently contradicts the “non-overlapping” claim and can skew results.
Line 84 states non-overlapping paragraphs, but placement uses fixed 20px spacing while measured paragraph dimensions are often larger. This introduces heavy overlap/overdraw, making the paint-vs-rect delta reflect fill-rate/clipping artifacts instead of isolated node cost.
📏 Suggested fix (dimension-aware grid placement)
- println!("Each test: 1000 paragraphs per frame, grid-positioned, non-overlapping.");
+ println!("Each test: 1000 paragraphs per frame, grid-positioned with cell size derived from paragraph bounds.");
@@
- // Test 1: N × paragraph.paint()
+ let cell_w = (para_w + 4.0).max(1.0);
+ let cell_h = (para_h + 4.0).max(1.0);
+ let cols = ((1000.0 / cell_w).floor().max(1.0)) as usize;
+
+ // Test 1: N × paragraph.paint()
flush_gpu(surface);
let start = Instant::now();
for _ in 0..n_iter {
let canvas = surface.canvas();
canvas.clear(skia_safe::Color::WHITE);
for i in 0..count {
- let x = (i % 40) as f32 * 20.0;
- let y = (i / 40) as f32 * 20.0;
+ let x = (i % cols) as f32 * cell_w;
+ let y = (i / cols) as f32 * cell_h;
para.paint(canvas, (x, y));
}
flush_gpu(surface);
}
@@
for _ in 0..n_iter {
let canvas = surface.canvas();
canvas.clear(skia_safe::Color::WHITE);
for i in 0..count {
- let x = (i % 40) as f32 * 20.0;
- let y = (i / 40) as f32 * 20.0;
+ let x = (i % cols) as f32 * cell_w;
+ let y = (i / cols) as f32 * cell_h;
canvas.draw_rect(skia_safe::Rect::from_xywh(x, y, para_w, para_h), &paint_obj);
}
flush_gpu(surface);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/examples/skia_bench/skia_bench_text_lod.rs` around lines
84 - 142, The benchmark places paragraphs using a fixed 20.0 spacing which
causes overlap because actual paragraph size comes from para.max_width() and
para.height(); update the grid placement logic around the loops that call
para.paint(...) and draw_rect(...) to use spacing derived from para_w and para_h
(e.g., let h_spacing = para_w + padding; let v_spacing = para_h + padding) and
compute x/y with those spacings (replace 20.0 and the hardcoded 40 column math
with a cols variable computed from count or surface width and use (i % cols) as
f32 * h_spacing and (i / cols) as f32 * v_spacing) so paragraphs do not overlap
and the paint vs rect comparison measures per-node cost correctly.
Comment on lines
+157
to
+160
| let paint_net = paint_us as i64 - skip_us as i64; | ||
| let rect_net = rect_us as i64 - skip_us as i64; | ||
| let ratio = paint_net.max(0) as f64 / rect_net.max(1) as f64; | ||
| let per_node_us = paint_net as f64 / count as f64; |
There was a problem hiding this comment.
Clamp baseline-subtracted values before reporting per-node metrics.
Timer noise can make paint_net/rect_net negative, which yields misleading negative per-node times. Clamp before computing user-facing metrics.
⏱️ Suggested fix
- let paint_net = paint_us as i64 - skip_us as i64;
- let rect_net = rect_us as i64 - skip_us as i64;
- let ratio = paint_net.max(0) as f64 / rect_net.max(1) as f64;
- let per_node_us = paint_net as f64 / count as f64;
+ let paint_net = (paint_us as i64 - skip_us as i64).max(0);
+ let rect_net = (rect_us as i64 - skip_us as i64).max(0);
+ let ratio = paint_net as f64 / rect_net.max(1) as f64;
+ let per_node_us = paint_net as f64 / count as f64;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let paint_net = paint_us as i64 - skip_us as i64; | |
| let rect_net = rect_us as i64 - skip_us as i64; | |
| let ratio = paint_net.max(0) as f64 / rect_net.max(1) as f64; | |
| let per_node_us = paint_net as f64 / count as f64; | |
| let paint_net = (paint_us as i64 - skip_us as i64).max(0); | |
| let rect_net = (rect_us as i64 - skip_us as i64).max(0); | |
| let ratio = paint_net as f64 / rect_net.max(1) as f64; | |
| let per_node_us = paint_net as f64 / count as f64; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/grida-canvas/examples/skia_bench/skia_bench_text_lod.rs` around lines
157 - 160, The baseline-subtracted timings paint_net and rect_net can be
negative due to timer noise, producing misleading negative per-node metrics;
clamp paint_net and rect_net to non-negative values before computing ratio and
per_node_us (e.g., replace paint_net and rect_net uses with paint_net_clamped =
paint_net.max(0) and rect_net_clamped = rect_net.max(1) or similar), then
compute ratio = paint_net_clamped as f64 / rect_net_clamped as f64 and
per_node_us = paint_net_clamped as f64 / (count as f64) while also ensuring
count > 0 is handled to avoid divide-by-zero.
Comment on lines
+17
to
+18
| **item 51 (Subpixel LOD Culling)** in `optimization.md`, which drops | ||
| entire leaves whose projected bounds collapse below a threshold. |
There was a problem hiding this comment.
Fix stale item-number references to optimization.md.
Line 17, Line 135, and Lines 207-208 point to outdated item numbers. In docs/wg/feat-2d/optimization.md, Subpixel LOD Culling is item 52 and Text LOD is item 53.
Suggested doc patch
-**item 51 (Subpixel LOD Culling)** in `optimization.md`, which drops
+**item 52 (Subpixel LOD Culling)** in `optimization.md`, which drops
@@
-| H1 | font size (cull) | font·z < ε_cull | skip text entirely ✅ item 52 |
-| H2 | font size (greek) | ε_cull ≤ font·z < ε_greek | render as SkRect(s) ✅ item 52 |
+| H1 | font size (cull) | font·z < ε_cull | skip text entirely ✅ item 53 |
+| H2 | font size (greek) | ε_cull ≤ font·z < ε_greek | render as SkRect(s) ✅ item 53 |
@@
-- **Item 51 (A1)** — implemented. Subpixel leaf-bounds culling.
-- **Item 52 (H1)** — implemented. Text font-size-below-threshold cull.
+- **Item 52 (A1)** — implemented. Subpixel leaf-bounds culling.
+- **Item 53 (H1)** — implemented. Text font-size-below-threshold cull.Also applies to: 135-137, 207-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/feat-2d/lod-properties.md` around lines 17 - 18, Update the stale
item-number references that point to optimization.md: change the reference for
"Subpixel LOD Culling" from item 51 to item 52 and change the reference for
"Text LOD" to item 53 wherever they appear in this document (the occurrences
referencing "Subpixel LOD Culling" and "Text LOD" — including the other two
blocks mentioned) so the cross-links match the actual items in optimization.md.
| - **Item 51 (A1)** — implemented. Subpixel leaf-bounds culling. | ||
| - **Item 52 (H1)** — implemented. Text font-size-below-threshold cull. | ||
| - **B1 (RRect → Rect)** — measured via `skia_bench_rrect_vs_rect`. | ||
| Needs per-backend decision; on some backends the analytic rrect |
There was a problem hiding this comment.
Minor typo: extra spacing in sentence.
Line 210 has double spaces in “analytic rrect shader”; trimming to single spacing improves readability.
🧰 Tools
🪛 LanguageTool
[grammar] ~210-~210: Ensure spelling is correct
Context: ...decision; on some backends the analytic rrect shader is already cheaper than `drawRe...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/wg/feat-2d/lod-properties.md` at line 210, The sentence containing the
fragment "analytic rrect shader" has extra whitespace; locate that exact text
in docs/wg/feat-2d/lod-properties.md and collapse the multiple spaces to a
single space so it reads "analytic rrect shader" (and while there, verify the
word "rrect" is intended—if it's a typo, change to "rect" so the final text
reads "analytic rect shader").
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Abstracts browser dependencies behind injectable interfaces so the Editor can be fully instantiated and tested in pure Node.js — no jsdom, no browser globals, no polyfills.
editor.api.IViewportApiandeditor.api.events.*(IPointerEvent,IKeyboardEvent,IFocusEvent) as platform-neutral event IRs — browser DOM events satisfy them via duck typing, so existing call-sites need zero changesEditor.createHeadless()static factory withHeadlessViewportApiand noop providers__tests__/utils/(factories, fixtures, stubs)history,select-readonly,move-tray) from raw reducer calls to headless Editor — 46-60% smaller, no more copy-pasted stubswriteClipboardMediato route throughui.clipboardwith graceful fallbackTest plan
vitest run grida-canvas/— 25 files, 221 passed, 4 pre-existing skips, 0 failurestsc --noEmit— cleantypeof window === "undefined"to prove no jsdom leakBreaking changes
None. All signature changes use structural subtyping — browser
PointerEvent,MouseEvent,KeyboardEvent,FocusEventsatisfy the new interfaces without casts.Summary by CodeRabbit
New Features
Editor.createHeadless()factoryDocumentation
Tests
Examples