Grida Canvas - Scene Graph & CRDT#431
Conversation
Grida Canvas - Service Layer & CRDT Demo
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
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. WalkthroughAdds JSON-Patch transaction support end-to-end (TS → WASM → Rust), changes document shape to nodes + scenes_ref + links, introduces JSONCornerRadius, migrates many editor APIs to commands/surface/camera, adds hosted playground/host and multiplayer demo, unifies exporters, and wide React + IO refactors and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as Editor UI (TS)
participant Scene as Scene (TS)
participant WASM as CanvasModule (WASM)
participant Rust as EmscriptenApplication (Rust)
UI->>Scene: Scene.applyTransactions(batch: unknown[][])
Note right of Scene #E6F2FF: serialize batch → alloc wasm memory
Scene->>WASM: _apply_scene_transactions(ptr,len)
WASM->>Rust: apply_scene_transactions(app_ptr, ptr, len)
Rust->>Rust: parse JSON, apply JSON-Patch transactions sequentially
Rust->>Rust: build reports[] (success/applied/total/error?)
Rust-->>WASM: return ptr_to_CStr(JSON.stringify(reports))
WASM-->>Scene: Ptr to JSON reports
Scene->>Scene: read memory, free, parse → TransactionApplyReport[]
Scene-->>UI: return reports[]
alt any valid scene produced
UI->>UI: reload scene from updated document JSON
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120–180 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (1)editor/app/[(]www[)]/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-09-26T09:29:53.155ZApplied to files:
🧬 Code graph analysis (1)editor/grida-canvas/reducers/index.ts (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (6)
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 |
![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
| const clientIds = this.awarenessClients; | ||
|
|
||
| removeAwarenessStates(this.doc.awareness, Array.from(clientIds), null); |
There was a problem hiding this comment.
When a socket closes, unregisterWebSocket calls removeAwarenessStates with Array.from(this.awarenessClients), which contains all known client IDs, not just those owned by the disconnected socket. As soon as any participant disconnects the server clears the awareness state for every other user, so their cursors disappear until they happen to send another awareness update (which may never happen if they are idle). Track client IDs per socket and only remove the ones that belong to the websocket being closed to avoid wiping active sessions.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
editor/components/formfield/grida-canvas/index.tsx (1)
50-62: Add error handling for template fetch.If the fetch fails or returns invalid JSON, the editor remains in its default empty state with no user feedback. This degrades the user experience and makes debugging difficult.
Apply this diff to add error handling:
useEffect(() => { - fetch("/examples/canvas/sketch-teimplate-01.grida").then((res) => { - res.json().then((file) => { - instance.commands.reset( - editor.state.init({ - editable: true, - document: file.document, - }), - "template" - ); - }); - }); + fetch("/examples/canvas/sketch-teimplate-01.grida") + .then((res) => { + if (!res.ok) { + throw new Error(`Failed to fetch template: ${res.status}`); + } + return res.json(); + }) + .then((file) => { + instance.commands.reset( + editor.state.init({ + editable: true, + document: file.document, + }), + "template" + ); + }) + .catch((error) => { + console.error("Failed to load canvas template:", error); + // TODO: Show user-facing error message + }); }, []);editor/grida-canvas/plugins/follow.ts (1)
55-70: Fix inconsistent API usage and improve callback consistency.Two issues in the subscription callback:
Critical inconsistency (line 60): Uses the old
editor.loadSceneAPI instead of the neweditor.commands.loadScene. This is inconsistent with line 50 and indicates incomplete migration to the command-based API.Minor inconsistency (lines 63-66): The callback receives an
editorparameter but then switches to usingthis.editorfor the camera transform call. For clarity and consistency, use the callback parameter throughout.Apply this diff to fix both issues:
this.__unsubscribe_cursor = this.editor.doc.subscribeWithSelector( (state) => state.cursors[cursor_id], (editor, cursor) => { if (!cursor) return; if (cursor.scene_id && cursor.scene_id !== editor.state.scene_id) { - editor.loadScene(cursor.scene_id); + editor.commands.loadScene(cursor.scene_id); } if (cursor.transform) { - this.editor.camera.transformWithSync( + editor.camera.transformWithSync( this.fit(cursor.transform), false ); } }, equal );editor/grida-canvas-react-starter-kit/starterkit-loading/loading.tsx (1)
36-148: Memory leak: completed animations accumulate in the ref.The
activeAnimationsRefarray grows unbounded because completed animations are never removed—they're only cleared whenstartAnimationis called again or on unmount. For components with frequent loading cycles, this accumulates animation objects in memory.Consider removing animations from the array when they complete naturally:
const createAsymptoticAnimation = ( targetValue: number, startValue: number ) => { const k = Math.log(2) / 2; // Mathematical decay rate const animation = animate(progressValue, targetValue, { duration: Infinity, ease: (t: number) => { // Asymptotic function: approaches target asymptotically const easedTime = easeOut(t); return ( startValue + (targetValue - startValue) * (1 - Math.exp(-k * easedTime * 10)) ); }, onUpdate: (value: number) => setProgress(value), + onComplete: () => { + const index = activeAnimationsRef.current.indexOf(animation); + if (index > -1) { + activeAnimationsRef.current.splice(index, 1); + } + }, }); activeAnimationsRef.current.push(animation); return animation; };Apply similar changes to
createStepAnimationandcreateLinearAnimation. This ensures the array only contains actively running animations.editor/app/(dev)/canvas/examples/[example]/page.tsx (1)
10-16: Fix Next.js params/searchParams typing and sanitize backendIn App Router,
params/searchParamsare plain objects (not Promises). AlsosearchParamsvalues are strings; parse and narrow to "canvas" | "dom". Avoid unnecessaryawait.Apply:
-export async function generateMetadata({ - params, -}: { - params: Promise<Params>; -}): Promise<Metadata | null> { - const { example: example_id } = await params; +export async function generateMetadata({ + params, +}: { + params: Params; +}): Promise<Metadata | null> { + const { example: example_id } = params;-export default async function FileExamplePage({ - params, - searchParams, -}: { - params: Promise<Params>; - searchParams: Promise<{ backend: "canvas" | "dom" }>; -}) { - const { example: example_id } = await params; - const { backend = "canvas" } = await searchParams; +export default async function FileExamplePage({ + params, + searchParams, +}: { + params: Params; + searchParams?: { backend?: string }; +}) { + const { example: example_id } = params; + const raw = searchParams?.backend; + const backend: "canvas" | "dom" = raw === "dom" || raw === "canvas" ? raw : "canvas";Also applies to: 27-36, 35-36, 41-43
editor/app/(dev)/canvas/examples/with-templates/002/page.tsx (1)
184-236: Template IDs used by nodes lack definitions in document.templatesNodes reference template_id "tabs" and "tmp-2503-join-hello", but only invite/join/join-main/portal are defined. Missing defs can break property scaffolding/validation.
Add minimal stubs:
templates: { @@ ["tmp-2503-portal"]: { name: "Portal", type: "template", properties: {}, default: {}, version: "0.0.0", nodes: {}, links: {}, }, + // Used by node "join_hello" + ["tmp-2503-join-hello"]: { + name: "Join Hello", + type: "template", + properties: {}, + default: {}, + version: "0.0.0", + nodes: {}, + links: {}, + }, + // Used by node "join" + ["tabs"]: { + name: "Tabs", + type: "template", + properties: {}, + default: {}, + version: "0.0.0", + nodes: {}, + links: {}, + }, },
🧹 Nitpick comments (37)
editor/grida-canvas-react/viewport/ui/__math/image-transform.test.ts (1)
22-34: Good cleanup removing test noise.Commenting out the console.log statements improves test output clarity.
Consider removing the commented lines entirely rather than leaving them as comments:
- // console.log("=== ORIGINAL DIRECTION SCALING CHALLENGE ==="); - // console.log( - // "REQUIREMENT: Scale sides in original X/Y directions regardless of rotation" - // ); - // console.log( - // "CHALLENGE: This requires advanced matrix manipulation beyond decompose/compose" - // ); - // console.log( - // "CURRENT: Geometry-based scaling that follows the current shape orientation" - // ); - // console.log( - // "FUTURE: Could be implemented with specialized matrix operations" - // ); - expect(true).toBe(true); // Acknowledge the limitationThe describe block's comment (lines 12-19) already documents the mathematical complexity clearly.
editor/components/formfield/grida-canvas/index.tsx (1)
50-62: Consider adding a loading state.The editor initializes empty, then loads the template asynchronously. Users may see a brief flash of empty content or interact with an incomplete editor. Consider adding a loading indicator or skeleton.
Example implementation:
const [isLoading, setIsLoading] = useState(true); useEffect(() => { fetch("/examples/canvas/sketch-teimplate-01.grida") .then((res) => res.json()) .then((file) => { instance.commands.reset( editor.state.init({ editable: true, document: file.document, }), "template" ); }) .catch(console.error) .finally(() => setIsLoading(false)); }, []); // Then conditionally render loading UI in the Dialogeditor/grida-canvas-react-starter-kit/starterkit-loading/loading.tsx (1)
132-132: RemoveprogressValuefrom the dependency array.
progressValueis a stable MotionValue reference that doesn't change between renders. Including it in the dependency array is unnecessary and violates React Hook best practices.Apply this diff:
- }, [loading, expectedDuration, steps, step, maxFakedProgress, progressValue]); + }, [loading, expectedDuration, steps, step, maxFakedProgress]);Similarly on line 140:
- }, [loading, progress, progressValue]); + }, [loading, progress]);editor/app/(dev)/ui/multiplayer/store.ts (1)
56-58: Prefer a collision-resistant id source
Math.random().toString(36).substr(2, 9)is prone to collisions under load. Switching tocrypto.randomUUID()(or a shared id helper) keeps the reducer deterministic and avoids hard-to-debug duplicates.- const id = Math.random().toString(36).substr(2, 9); + const id = typeof crypto !== "undefined" && "randomUUID" in crypto + ? crypto.randomUUID() + : Math.random().toString(36).slice(2);editor/grida-canvas/__tests__/editor.api.patch.test.ts (1)
1-40: Consider additional test cases for edge cases and error handling.The test coverage for JSON Patch utilities is good, but consider adding tests for:
Edge cases:
- Empty path arrays:
toJsonPointerPath([])- Paths with only special characters:
["~", "/"]- Unicode characters in segments
- Very long paths
Error conditions:
- Invalid operations (if validation exists)
- Patches with missing required fields
- Type mismatches in patch values
RFC 6902 compliance:
- Test root path:
toJsonPointerPath([])should return""- Add operation with explicit index vs
-(append)- Move and copy operations (if supported)
Example additional tests:
it("handles empty paths", () => { expect(editor.api.patch.toJsonPointerPath([])).toBe(""); }); it("handles special characters in segments", () => { expect(editor.api.patch.encodeJsonPointerSegment("~/")).toBe("~0~1"); }); it("handles patches with add operation", () => { const patches: editor.history.Patch[] = [ { op: "add", path: ["document", "nodes", "-"], value: { id: "new-node" }, }, ]; expect(editor.api.patch.toJsonPatchOperations(patches)).toEqual([ { op: "add", path: "/document/nodes/-", value: { id: "new-node" }, }, ]); });editor/grida-canvas-react/viewport/surface-hooks.ts (2)
318-339: Good move to geometryProvider; small dedupe nitSwitching to instance.geometryProvider.getNodeAbsoluteBoundingRect aligns with the new provider-based API. Minor nit: you recompute boundingSurfaceRect again at Lines 341–344; reuse bsr to avoid a second transform.
371-378: Unify geometry access across single/group pathsSingle selection uses geometryProvider while computeSurfaceSelectionGroup still relies on geometry.getNodeAbsoluteBoundingRect (Line 178). For consistency and fewer divergence risks, consider migrating the group path to geometryProvider too.
editor/grida-canvas/backends/dom-export.ts (1)
5-5: Avoid Node ‘assert’ in browser bundlesUsing assert from "assert" can pull unnecessary polyfills into web bundles. Prefer an inline guard.
-import assert from "assert"; ... - assert(this.formats.includes(format), "non supported format"); + if (!this.formats.includes(format)) { + throw new Error("non supported format"); + }editor/grida-canvas-react/viewport/hooks/use-dpr.ts (1)
1-50: Solid DPR hook; add Safari fallback for MediaQueryListImplementation is clean and SSR-safe. Consider addListener/removeListener fallback for older Safari.
- if (typeof window.matchMedia === "function") { + if (typeof window.matchMedia === "function") { mediaQuery = window.matchMedia(`(resolution: ${dpr}dppx)`); - mediaQueryListener = () => update(); - mediaQuery.addEventListener("change", mediaQueryListener); + mediaQueryListener = () => update(); + if ("addEventListener" in mediaQuery) { + mediaQuery.addEventListener("change", mediaQueryListener); + } else if ("addListener" in mediaQuery) { + // Safari < 14 + // @ts-expect-error legacy API + mediaQuery.addListener(mediaQueryListener); + } } ... - if (mediaQuery && mediaQueryListener) { - mediaQuery.removeEventListener("change", mediaQueryListener); - } + if (mediaQuery && mediaQueryListener) { + if ("removeEventListener" in mediaQuery) { + mediaQuery.removeEventListener("change", mediaQueryListener); + } else if ("removeListener" in mediaQuery) { + // @ts-expect-error legacy API + mediaQuery.removeListener(mediaQueryListener); + } + }editor/grida-canvas/plugins/recorder.ts (1)
102-106: Ensure replay finishes before resetrequestAnimationFrame is scheduled but not awaited; exit() may run before the last frame dispatch, potentially resetting to final state before final actions apply. Await rAF per frame to guarantee ordering.
- await new Promise((resolve) => setTimeout(resolve, delay)); - - requestAnimationFrame(() => { - this.editor.doc.dispatch(current.a, true); - }); + await new Promise((resolve) => setTimeout(resolve, delay)); + await new Promise<void>((resolve) => + requestAnimationFrame(() => { + this.editor.doc.dispatch(current.a, true); + resolve(); + }) + );Also applies to: 109-110
editor/grida-canvas/backends/noop.ts (1)
27-41: Return a rejected Promise and tighten formats typingImprove contract clarity by rejecting a Promise (instead of throwing) and making
formatstype explicit.export class NoopDefaultExportInterfaceProvider implements editor.api.IDocumentExporterInterfaceProvider { - readonly formats = []; + readonly formats: editor.api.IDocumentExporterInterfaceProvider["formats"] = [] as const; @@ - ): Promise<Uint8Array | string> { - throw new Error("Not implemented"); - } + ): Promise<Uint8Array | string> { + return Promise.reject(new Error("Not implemented")); + }editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-scene.tsx (1)
115-116: Guard against empty selection before loadingAvoid calling
loadScenewithundefinedwhen no item is selected.- setSelectedItems: (items) => { - editor.commands.loadScene((items as string[])[0]); - }, + setSelectedItems: (items) => { + const first = (items as string[])[0]; + if (first) editor.commands.loadScene(first); + },editor/grida-canvas-react/use-mixed-properties.ts (1)
120-127: Unify text font ops under commands for consistency (if available)
fontFamilyandfontStylestill call instance methods; preferinstance.commands.*variants if exposed to keep one mutation path.Also applies to: 156-163
editor/grida-canvas-react/use-data-transfer.ts (3)
245-258: Prevent default when handling Grida clipboard payloadsWe set pasted_from_data_transfer but never call preventDefault in these branches, unlike the vector path. Prevent the browser paste to avoid inconsistent behavior.
- if ( - current_clipboard?.payload_id === grida_payload.clipboard.payload_id - ) { - instance.commands.paste(); - pasted_from_data_transfer = true; - } else if (grida_payload.clipboard.type === "prototypes") { - instance.commands.pastePayload(grida_payload.clipboard); - pasted_from_data_transfer = true; - } else { - instance.commands.paste(); - pasted_from_data_transfer = true; - } + if ( + current_clipboard?.payload_id === grida_payload.clipboard.payload_id + ) { + instance.commands.paste(); + event.preventDefault(); + pasted_from_data_transfer = true; + } else if (grida_payload.clipboard.type === "prototypes") { + instance.commands.pastePayload(grida_payload.clipboard); + event.preventDefault(); + pasted_from_data_transfer = true; + } else { + instance.commands.paste(); + event.preventDefault(); + pasted_from_data_transfer = true; + }
262-289: Prevent default when inserting text/images from clipboardText/image branches insert content but don't cancel the native paste. Prevent default after successful insertion.
case "text": { const { text } = item; insertText(text, { clientX: window.innerWidth / 2, clientY: window.innerHeight / 2, }); + event.preventDefault(); pasted_from_data_transfer = true; break; } @@ const { type, file } = item; insertFromFile(type, file, { clientX: window.innerWidth / 2, clientY: window.innerHeight / 2, }); + event.preventDefault(); pasted_from_data_transfer = true; break; }
167-179: Robust filename parsing for SVG/imagessplit(".svg") and split(".") are brittle with multiple dots/case differences. Use regex-based replacement.
- const name = file.name.split(".svg")[0]; + const name = file.name.replace(/\.svg$/i, ""); @@ - const name = file.name.split(".")[0]; + const name = file.name.replace(/\.[^/.]+$/, "");editor/app/(workbench)/[org]/[proj]/(console)/(campaign)/campaigns/[campaign]/design/template-duo-001-viewer.tsx (1)
120-138: Consider migrating this example to scenes_ref + linksThe code uses the old scenes Record + children_refs. Since the PR standardizes on scenes_ref + links, consider migrating for consistency across examples.
editor/grida-canvas-hosted/distro.ts (1)
5-8: Make snapshot_file_name locale-agnostictoLocaleTimeString varies by locale and may include non-filename-safe chars. Use ISO time for predictable, safe names.
- const now = new Date(); - const date = now.toISOString().split("T")[0]; - const time = now.toLocaleTimeString().replace(/:/g, "."); + const now = new Date(); + const [date, isoTime] = now.toISOString().split("T"); + const time = isoTime.slice(0, 8).replace(/:/g, "."); return `Snapshot ${date} at ${time}.grida`;editor/grida-canvas-react/viewport/hotkeys.tsx (1)
301-304: Typo in keybinding description"shif+w" → "shift+w".
- keys: ["shif+w"], + keys: ["shift+w"],editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx (2)
199-207: Use reactive state (nodes/document_ctx) instead of direct editor.state to avoid stale readsYou already select nodes and document_ctx via useEditorState. Use them here and in deps for consistent updates and less churn.
- const node = editor.state.document.nodes[currentId]; + const node = nodes[currentId]; ... - const parentId = editor.state.document_ctx.lu_parent[currentId]; + const parentId = document_ctx.lu_parent[currentId]; @@ - }, [ - selection, - id, - editor.state.document.nodes, - editor.state.document_ctx.lu_parent, - ]); + }, [selection, id, nodes, document_ctx.lu_parent]);Also applies to: 227-231
288-290: Prefer selected nodes over global editor.stateKeep sourcing consistent with useEditorState to avoid stale reads and improve memoization.
- getItem(itemId) { - return editor.state.document.nodes[itemId]; - }, + getItem(itemId) { + return nodes[itemId]; + },editor/grida-canvas/editor.i.ts (3)
2430-2433: Avoid duplicate vector API surfacesBoth IDocumentVectorInterfaceProvider and IDocumentVectorInterfaceActions expose toVectorNetwork. Consolidate to one to reduce divergence risk.
I can fold Actions into Provider and re-export a single interface if you want.
Also applies to: 2386-2394
2440-2458: Method names with “Sync” returning PromiseloadFontSync/getFontFamilyDetailsSync return Promise. Consider renaming or making them truly sync for clarity.
Rename to loadFont / getFontFamilyDetails or drop “Sync”.
2258-2286: Geometry interface/query duplicationTwo similar geometry interfaces exist (IDocumentGeometryInterfaceProvider and IDocumentGeometryQuery). Unify to one to simplify implementation/typing.
Happy to propose a merged interface and update call sites.
Also applies to: 2397-2428
crates/grida-canvas-fonts/tests/parse_ui_styles.rs (1)
1-3: Consider renaming file to ui_parser_test.rsMatches suite conventions for high-level UI parser tests.
As per coding guidelines / Based on learnings
editor/grida-canvas/history-manager.ts (2)
5-25: Consider restoring actionType check in merge logic.The commented-out actionType comparison (line 17) means that entries of different action types can be merged if they occur within the timeout window. This might lead to unexpected history behavior where unrelated actions are grouped together.
If merging different action types is intentional, consider adding a comment explaining the rationale. Otherwise, uncomment the check:
- if ( - // actionType !== previousEntry.actionType || - currentTimestamp - previousEntry.ts > - timeout - ) { + if ( + actionType !== previousEntry.actionType || + currentTimestamp - previousEntry.ts > timeout + ) { return; }
218-232: Consider documenting hover state reset behavior.Line 231 resets
hovered_node_idafter applying history changes. While this is likely intentional (hover state should not persist across undo/redo), consider adding a comment explaining this choice.draft.document = nextState.document; draft.document_ctx = nextState.document_ctx; draft.content_edit_mode = nextState.content_edit_mode; draft.document_key = nextState.document_key; + // Reset transient UI state that should not persist across history navigation draft.hovered_node_id = null; }crates/grida-canvas/src/node/schema.rs (1)
81-84: Docs clarify links-driven children; add invariants for clarityLooks good. Consider adding brief invariants:
- children must reference ids present in nodes
- not serialized directly; derived from document.links on load
Also applies to: 89-90
AGENTS.md (1)
189-190: High watcher concurrency may degrade local dev stabilityRunning pnpm dev:packages with --concurrency 100 can overwhelm file watchers/CPUs on many machines. Consider documenting a safer default (e.g., 8–16) or making it configurable via env.
editor/grida-canvas-react/renderer.tsx (3)
103-111: Guard against missing scene nodeAvoid non-null assertions; handle absent scene safely.
- const slice = useEditorState(instance, (state) => { - const scene = state.document.nodes[ - state.scene_id! - ] as grida.program.nodes.SceneNode; - return { - backgroundColor: scene?.backgroundColor, - transform: state.transform, - }; - }); + const slice = useEditorState(instance, (state) => { + const sid = state.scene_id; + const node = sid ? state.document.nodes[sid] : undefined; + const bg = + node && node.type === "scene" + ? (node as grida.program.nodes.SceneNode).backgroundColor + : undefined; + return { backgroundColor: bg, transform: state.transform }; + });
115-119: Remove unused variableopacity is computed but unused.
- const [cssBackgroundColor, opacity] = useMemo(() => { + const cssBackgroundColor = useMemo(() => { if (!backgroundColor) return [undefined, 1] as const; const hex = cmath.color.rgba8888_to_hex(backgroundColor); - const opacity = backgroundColor.a; - return [hex, opacity] as const; + return hex; }, [backgroundColor]);
167-168: Effect deps: include editorInclude editor in deps to avoid stale closure.
- useEffect(() => { - editor.camera.fit("*"); - }, [documentKey, sceneId]); + useEffect(() => { + editor.camera.fit("*"); + }, [editor, documentKey, sceneId]);editor/app/(dev)/canvas/experimental/dom/page.tsx (1)
9-20: Fix searchParams typing (Next.js App Router)Next passes a plain object; no Promise needed. Remove await and widen type.
-export default async function CanvasPlaygroundPage({ - searchParams, -}: { - searchParams: Promise<{ room: string }>; -}) { - const { room } = await searchParams; +export default async function CanvasPlaygroundPage({ + searchParams, +}: { + searchParams: { room?: string }; +}) { + const { room } = searchParams; return ( <main className="w-screen h-screen overflow-hidden"> <Editor backend="dom" room_id={room} /> </main> ); }editor/grida-canvas-react/use-context-menu-actions.ts (1)
47-75: Harden node lookups to avoid runtime errorsIf an id is missing in document.nodes, current code will throw. Guard and align predicates.
- const nodes = useEditorState(editor, (s) => { - const map: Record<string, { type: grida.program.nodes.NodeType }> = {}; - ids.forEach((id) => { - map[id] = { type: s.document.nodes[id].type }; - }); - return map; - }); + const nodes = useEditorState(editor, (s) => { + const map: Record<string, { type: grida.program.nodes.NodeType }> = {}; + ids.forEach((id) => { + const n = s.document.nodes[id]; + if (n) map[id] = { type: n.type }; + }); + return map; + }); - const canFlatten = - backend === "canvas" && - hasSelection && - ids.every((id) => supportsFlatten(nodes[id])); + const canFlatten = + backend === "canvas" && + hasSelection && + ids.every((id) => nodes[id] && supportsFlatten(nodes[id])); const canUngroup = backend === "canvas" && hasSelection && - ids.some( - (id) => nodes[id].type === "group" || nodes[id].type === "boolean" - ); + ids.some((id) => nodes[id] && (nodes[id].type === "group" || nodes[id].type === "boolean")); const canPlanarize = backend === "canvas" && hasSelection && - ids.every((id) => nodes[id].type === "vector"); + ids.every((id) => nodes[id] && nodes[id].type === "vector");If available, should removeMask also move under editor.commands for consistency?
editor/grida-canvas-hosted/playground/playground.tsx (1)
251-283: Consider cleaning up WASM mountEffect mounts the canvas but never unmounts/detaches observers on cleanup; can leak GPU resources.
Add a cleanup that calls a corresponding unmount/dispose if available, or guard a future unmount API.
crates/grida-canvas/src/io/io_grida_patch.rs (1)
117-202: Good unit coverageCovers success and failure-then-success flows. Consider adding a zero-op transaction test later.
editor/grida-canvas/backends/wasm.ts (1)
91-110: Consider a more descriptive error message.The error thrown on line 107 could be more helpful for debugging. Consider including the attempted format in the error message.
default: { - throw new Error("Non supported format"); + throw new Error(`Unsupported export format: ${format}. Supported formats: ${this.formats.join(", ")}`); }
| { | ||
| "version": "0.0.1-beta.1+20251010", | ||
| "document": { | ||
| "nodes": { | ||
| "25575e75-a544-4fa3-b199-15d1906588b2": { | ||
| "id": "25575e75-a544-4fa3-b199-15d1906588b2", | ||
| "name": "rectangle", | ||
| "locked": false, | ||
| "active": true, | ||
| "position": "absolute", | ||
| "top": 0, | ||
| "left": 0, | ||
| "opacity": 1, | ||
| "zIndex": 0, | ||
| "rotation": 0, | ||
| "fill": { | ||
| "type": "solid", | ||
| "color": { | ||
| "r": 217, | ||
| "g": 217, | ||
| "b": 217, | ||
| "a": 1 | ||
| } | ||
| }, | ||
| "width": 100, | ||
| "height": 100, | ||
| "style": {}, | ||
| "type": "rectangle", | ||
| "cornerRadius": 0, | ||
| "effects": [], | ||
| "strokeWidth": 0, | ||
| "strokeCap": "butt" | ||
| }, | ||
| "main": { | ||
| "type": "scene", | ||
| "id": "main", | ||
| "name": "main", | ||
| "active": true, | ||
| "locked": false, | ||
| "constraints": { | ||
| "children": "multiple" | ||
| }, | ||
| "guides": [], | ||
| "edges": [], | ||
| "backgroundColor": { | ||
| "r": 245, | ||
| "g": 245, | ||
| "b": 245, | ||
| "a": 1 | ||
| } | ||
| } | ||
| }, | ||
| "links": { | ||
| "main": [ | ||
| "25575e75-a544-4fa3-b199-15d1906588b2" | ||
| ] | ||
| }, | ||
| "scenes_ref": [ | ||
| "main" | ||
| ], | ||
| "bitmaps": {}, | ||
| "images": {}, | ||
| "properties": {} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Approve the new schema structure; verify TypeScript types and documentation are updated.
The new document schema correctly implements the breaking changes outlined in the PR objectives:
- Flat node storage in the
nodesmap - Separate
linksmap for parent-child relationships (replacing thechildrenfield) scenes_refarray for scene references
The data structure is valid and internally consistent. However, ensure that:
- TypeScript type definitions reflect this new schema
- WASM bindings in
grida-canvas-wasm.d.tsare updated to match - Migration documentation explains the breaking change from the old
childrenmodel to the newlinksmodel
Based on learnings.
Run the following script to verify that TypeScript types have been updated:
🏁 Script executed:
#!/bin/bash
# Verify TypeScript types and WASM bindings are updated for new schema
echo "=== Searching for TypeScript type definitions that may need updates ==="
# Check for old 'children' field in types
rg -n --type=ts 'children.*:' crates/grida-canvas-wasm/ -C 3
echo ""
echo "=== Checking WASM bindings declarations ==="
# Look for document schema types in TypeScript declarations
fd -e d.ts . crates/grida-canvas-wasm/ --exec cat {}
echo ""
echo "=== Searching for 'links' and 'scenes_ref' in TypeScript files ==="
# Verify new schema fields are present in types
rg -n --type=ts '(links|scenes_ref)' crates/grida-canvas-wasm/ -C 2Length of output: 19598
Add missing TypeScript schema definitions for nodes, links, and scenes_ref
- The current
.d.tsonly usesTODOand lacks types for the updated JSON schema. - Update
crates/grida-canvas-wasm/grida-canvas-wasm.d.tsto include interfaces matching thenodesmap,linksmap, andscenes_refarray. - Document the migration from the old
childrenmodel to the newlinksmodel.
🤖 Prompt for AI Agents
In crates/grida-canvas-wasm/grida-canvas-wasm.d.ts (update types covering the
JSON shown in rectangle.grida lines 1-64), add proper TypeScript interfaces
replacing the TODOs: define a Nodes map type Record<string, Node> and concrete
Node interfaces (e.g., RectangleNode with id, name, position, top, left, width,
height, fill, type, cornerRadius, effects, strokeWidth, strokeCap, etc.;
SceneNode with id, name, type='scene', constraints, guides, edges,
backgroundColor, etc.), define Links as Record<string, string[]> where each
scene id maps to an array of node ids, and define scenes_ref as string[]; export
a top-level Document interface that uses these types (nodes: Nodes, links:
Links, scenes_ref: string[]), and add a brief JSDoc comment noting the migration
from the old children model to the new links model (scene -> [nodeIds]) so
consumers know how to translate children arrays to links entries.
| let links = canvas_file.document.links; | ||
|
|
||
| // Determine scene_id | ||
| let scene_id = canvas_file | ||
| .document | ||
| .entry_scene_id | ||
| .or_else(|| canvas_file.document.scenes_ref.first().cloned()) | ||
| .expect("no scene found"); | ||
|
|
||
| // Extract scene metadata from SceneNode | ||
| let (scene_name, _bg_color) = if let Some(cg::io::io_grida::JSONNode::Scene(scene_node)) = | ||
| canvas_file.document.nodes.get(&scene_id) | ||
| { | ||
| (scene_node.name.clone(), scene_node.background_color.clone()) | ||
| } else { | ||
| (scene_id.clone(), None) | ||
| }; | ||
|
|
||
| // Get scene children from links | ||
| let scene_children = links | ||
| .get(&scene_id) | ||
| .and_then(|c| c.clone()) | ||
| .unwrap_or_default(); | ||
|
|
||
| // Convert nodes to repository, filtering out scene nodes and populating children from links | ||
| let mut node_repo = cg::node::repository::NodeRepository::new(); | ||
| for (node_id, json_node) in canvas_file.document.nodes { | ||
| // Skip scene nodes - they're handled separately | ||
| if matches!(json_node, cg::io::io_grida::JSONNode::Scene(_)) { | ||
| continue; | ||
| } | ||
|
|
||
| let mut node: cg::node::schema::Node = json_node.into(); | ||
|
|
||
| // Populate children from links | ||
| if let Some(children_opt) = links.get(&node_id) { | ||
| if let Some(children) = children_opt { | ||
| match &mut node { | ||
| cg::node::schema::Node::Container(n) => n.children = children.clone(), | ||
| cg::node::schema::Node::Group(n) => n.children = children.clone(), | ||
| cg::node::schema::Node::BooleanOperation(n) => n.children = children.clone(), | ||
| _ => {} // Other nodes don't have children | ||
| } | ||
| } | ||
| } | ||
|
|
||
| node_repo.insert(node); | ||
| } |
There was a problem hiding this comment.
Fix partial-move compile error and preserve scene background color
- Moving document.links, then accessing document fields causes “use of partially moved value” compile error.
- Scene background_color is computed but ignored; default overwrites actual value.
- Minor: simplify Option flattening.
Apply this refactor:
- let links = canvas_file.document.links;
-
- // Determine scene_id
- let scene_id = canvas_file
- .document
- .entry_scene_id
- .or_else(|| canvas_file.document.scenes_ref.first().cloned())
- .expect("no scene found");
-
- // Extract scene metadata from SceneNode
- let (scene_name, _bg_color) = if let Some(cg::io::io_grida::JSONNode::Scene(scene_node)) =
- canvas_file.document.nodes.get(&scene_id)
- {
- (scene_node.name.clone(), scene_node.background_color.clone())
- } else {
- (scene_id.clone(), None)
- };
-
- // Get scene children from links
- let scene_children = links
- .get(&scene_id)
- .and_then(|c| c.clone())
- .unwrap_or_default();
-
- // Convert nodes to repository, filtering out scene nodes and populating children from links
- let mut node_repo = cg::node::repository::NodeRepository::new();
- for (node_id, json_node) in canvas_file.document.nodes {
+ // Move the document once; borrow fields as needed to avoid partial-move issues.
+ let mut document = canvas_file.document;
+
+ // Determine scene_id without moving fields
+ let scene_id = document
+ .entry_scene_id
+ .as_ref()
+ .cloned()
+ .or_else(|| document.scenes_ref.first().cloned())
+ .expect("no scene found");
+
+ // Take ownership of maps we need
+ let links = document.links;
+ let nodes_map = document.nodes;
+
+ // Extract scene metadata from SceneNode
+ let (scene_name, scene_bg_color) =
+ if let Some(cg::io::io_grida::JSONNode::Scene(scene_node)) = nodes_map.get(&scene_id) {
+ (scene_node.name.clone(), scene_node.background_color.clone())
+ } else {
+ (scene_id.clone(), None)
+ };
+
+ // Get scene children from links
+ let scene_children = links.get(&scene_id).cloned().flatten().unwrap_or_default();
+
+ // Convert nodes to repository, filtering out scene nodes and populating children from links
+ let mut node_repo = cg::node::repository::NodeRepository::new();
+ for (node_id, json_node) in nodes_map {
// Skip scene nodes - they're handled separately
if matches!(json_node, cg::io::io_grida::JSONNode::Scene(_)) {
continue;
}
let mut node: cg::node::schema::Node = json_node.into();
// Populate children from links
- if let Some(children_opt) = links.get(&node_id) {
- if let Some(children) = children_opt {
+ if let Some(children) = links.get(&node_id).cloned().flatten() {
match &mut node {
- cg::node::schema::Node::Container(n) => n.children = children.clone(),
- cg::node::schema::Node::Group(n) => n.children = children.clone(),
- cg::node::schema::Node::BooleanOperation(n) => n.children = children.clone(),
+ cg::node::schema::Node::Container(n) => n.children = children,
+ cg::node::schema::Node::Group(n) => n.children = children,
+ cg::node::schema::Node::BooleanOperation(n) => n.children = children,
_ => {} // Other nodes don't have children
}
- }
}
node_repo.insert(node);
}
Scene {
nodes: node_repo,
id: scene_id,
- name: scene_name,
- children: scene_children,
- background_color: Some(CGColor(230, 230, 230, 255)),
+ name: scene_name,
+ children: scene_children,
+ // Use scene background if present; otherwise fallback to default.
+ background_color: scene_bg_color.or(Some(CGColor(230, 230, 230, 255))),
}Also applies to: 68-74
🤖 Prompt for AI Agents
In crates/grida-canvas/examples/app_grida.rs around lines 19-66, avoid partially
moving canvas_file.document (which causes the “use of partially moved value”
compile error) and preserve the scene background color: clone or borrow the maps
up-front (e.g., let links = canvas_file.document.links.clone(); let nodes =
canvas_file.document.nodes.clone(); or borrow with & when only reading) so you
can still access entry_scene_id/scenes_ref; compute scene background as let
scene_bg = if let Some(JSONNode::Scene(s)) = nodes.get(&scene_id) {
s.background_color.clone() } else { None } (and use scene_bg later instead of
discarding it); simplify the children extraction to
links.get(&scene_id).cloned().unwrap_or_default() and when iterating nodes use
nodes.into_iter() (or nodes.iter() with cloning of values) and populate children
by checking links.get(&node_id).and_then(|c| c.clone()) or
links.get(&node_id).cloned() to avoid extra clones.
| /// Apply a batch of scene transactions represented as JSON Patch operations. | ||
| fn apply_document_transactions( | ||
| &mut self, | ||
| transactions: Vec<Vec<Value>>, | ||
| ) -> Vec<TransactionApplyReport> { | ||
| let _ = transactions; | ||
| Vec::new() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Default impl silently drops transactions; return failure reports or make it abstract
Returning an empty Vec masks caller errors. Either make this method required or return per-transaction failure reports to avoid silent data loss.
Apply this safer default:
fn apply_document_transactions(
&mut self,
transactions: Vec<Vec<Value>>,
) -> Vec<TransactionApplyReport> {
- let _ = transactions;
- Vec::new()
+ transactions
+ .into_iter()
+ .map(|tx| TransactionApplyReport {
+ success: false,
+ applied: 0,
+ total: tx.len(),
+ error: Some("apply_document_transactions not implemented".to_string()),
+ })
+ .collect()
}To ensure no implementations rely on the old behavior, scan for ApplicationApi impls and confirm they override this method:
🏁 Script executed:
#!/bin/bash
ast-grep --pattern $'impl ApplicationApi for $_ { $$$ }' -n
rg -n --type=rust -C2 'fn\s+apply_document_transactions\s*\('Length of output: 1374
Require explicit handling of document transactions
- Remove the default stub in
ApplicationApi::apply_document_transactions(crates/grida-canvas/src/window/application.rs:67-74) so the method is abstract. - In both overrides (application.rs:379-384 and application_emscripten.rs:183-188), replace
Vec::new()with a per-transactionTransactionApplyReport(e.g.success=false,applied=0,total=tx.len(),error=Some("not implemented")).
🤖 Prompt for AI Agents
In crates/grida-canvas/src/window/application.rs around lines 67-74, remove the
default stub implementation of fn apply_document_transactions so the method
becomes abstract (no body) and must be implemented by callers; then in the two
existing overrides (crates/grida-canvas/src/window/application.rs lines ~379-384
and crates/grida-canvas/src/window/application_emscripten.rs lines ~183-188)
replace the current Vec::new() return with a Vec constructed by iterating over
the incoming transactions and returning one TransactionApplyReport per
transaction with success=false, applied=0, total=tx.len() (or
transaction.len()), and error=Some("not implemented") to indicate the
unimplemented behavior.
| let links = file.document.links; | ||
|
|
||
| // Determine scene_id | ||
| let scene_id = file | ||
| .document | ||
| .entry_scene_id | ||
| .or_else(|| file.document.scenes_ref.first().cloned()) | ||
| .unwrap_or_else(|| "scene".to_string()); | ||
|
|
There was a problem hiding this comment.
Move error: links is moved out, then file.document is used
let links = file.document.links; moves links out of file.document, making subsequent access to file.document.* illegal. Clone or borrow links.
Apply this fix:
- let links = file.document.links;
+ let links = file.document.links.clone();📝 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.
| let links = file.document.links; | |
| // Determine scene_id | |
| let scene_id = file | |
| .document | |
| .entry_scene_id | |
| .or_else(|| file.document.scenes_ref.first().cloned()) | |
| .unwrap_or_else(|| "scene".to_string()); | |
| let links = file.document.links.clone(); | |
| // Determine scene_id | |
| let scene_id = file | |
| .document | |
| .entry_scene_id | |
| .or_else(|| file.document.scenes_ref.first().cloned()) | |
| .unwrap_or_else(|| "scene".to_string()); |
🤖 Prompt for AI Agents
In crates/grida-canvas/src/window/application.rs around lines 466 to 474, the
line `let links = file.document.links;` moves `links` out of `file.document`
causing later access to `file.document` to be invalid; change it to borrow or
clone instead (e.g. use `let links = &file.document.links;` if a reference
suffices, or `let links = file.document.links.clone();` if you need an owned
value) and update downstream uses to accept a `&` or owned clone accordingly so
subsequent `file.document.*` accesses remain valid.
| scenes_ref: ["main"], | ||
| links: { | ||
| main: ["a", "b"], | ||
| }, | ||
| nodes: { | ||
| main: { |
There was a problem hiding this comment.
Missing required document.entry_scene_id ("main")
IEditorStateInit requires entry_scene_id. Add it to satisfy the type and runtime expectations.
document: {
+ entry_scene_id: "main",
scenes_ref: ["main"],
links: {
main: ["a", "b"],
},Based on learnings (see editor/grida-canvas/editor.i.ts type snippet).
🤖 Prompt for AI Agents
In editor/app/(dev)/canvas/examples/network/page.tsx around lines 10 to 15, the
initial editor state is missing the required entry_scene_id property ("main");
update the IEditorStateInit object to include entry_scene_id set to the scene id
string ("main") so the runtime and types align with
editor/grida-canvas/editor.i.ts expectations, ensuring the value matches an
existing scene id and adjusting any surrounding object literals to include that
field.
| const { scenesmap, scenes_ref } = useEditorState(editor, (state) => { | ||
| // Build scenes map from scenes_ref for backward compatibility | ||
| const scenesmap: Record<string, grida.program.nodes.SceneNode> = | ||
| state.document.scenes_ref.reduce( | ||
| (acc: any, scene_id: string) => { | ||
| const scene_node = state.document.nodes[ | ||
| scene_id | ||
| ] as grida.program.nodes.SceneNode; | ||
| const children_refs = state.document.links[scene_id] || []; | ||
| acc[scene_id] = { | ||
| ...scene_node, | ||
| children_refs, | ||
| }; | ||
| return acc; | ||
| }, | ||
| {} as { [key: string]: grida.program.nodes.SceneNode } | ||
| ); | ||
|
|
||
| return { | ||
| scenesmap, | ||
| scenes_ref: state.document.scenes_ref, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Type scenesmap to include children_refs to avoid excess-property errors
You spread a SceneNode and add children_refs, but scenesmap is typed as SceneNode, which can trigger TS excess-property checks. Define an extended type for safety.
- const { scenesmap, scenes_ref } = useEditorState(editor, (state) => {
- // Build scenes map from scenes_ref for backward compatibility
- const scenesmap: Record<string, grida.program.nodes.SceneNode> =
+ const { scenesmap, scenes_ref } = useEditorState(editor, (state) => {
+ type SceneWithChildren = grida.program.nodes.SceneNode & { children_refs: string[] };
+ // Build scenes map from scenes_ref for backward compatibility
+ const scenesmap: Record<string, SceneWithChildren> =
state.document.scenes_ref.reduce(
- (acc: any, scene_id: string) => {
+ (acc: Record<string, SceneWithChildren>, scene_id: string) => {
const scene_node = state.document.nodes[
scene_id
] as grida.program.nodes.SceneNode;
const children_refs = state.document.links[scene_id] || [];
- acc[scene_id] = {
+ acc[scene_id] = {
...scene_node,
children_refs,
};
return acc;
},
- {} as { [key: string]: grida.program.nodes.SceneNode }
+ {} as Record<string, SceneWithChildren>
);📝 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.
| const { scenesmap, scenes_ref } = useEditorState(editor, (state) => { | |
| // Build scenes map from scenes_ref for backward compatibility | |
| const scenesmap: Record<string, grida.program.nodes.SceneNode> = | |
| state.document.scenes_ref.reduce( | |
| (acc: any, scene_id: string) => { | |
| const scene_node = state.document.nodes[ | |
| scene_id | |
| ] as grida.program.nodes.SceneNode; | |
| const children_refs = state.document.links[scene_id] || []; | |
| acc[scene_id] = { | |
| ...scene_node, | |
| children_refs, | |
| }; | |
| return acc; | |
| }, | |
| {} as { [key: string]: grida.program.nodes.SceneNode } | |
| ); | |
| return { | |
| scenesmap, | |
| scenes_ref: state.document.scenes_ref, | |
| }; | |
| }); | |
| const { scenesmap, scenes_ref } = useEditorState(editor, (state) => { | |
| type SceneWithChildren = grida.program.nodes.SceneNode & { children_refs: string[] }; | |
| // Build scenes map from scenes_ref for backward compatibility | |
| const scenesmap: Record<string, SceneWithChildren> = | |
| state.document.scenes_ref.reduce( | |
| (acc: Record<string, SceneWithChildren>, scene_id: string) => { | |
| const scene_node = state.document.nodes[ | |
| scene_id | |
| ] as grida.program.nodes.SceneNode; | |
| const children_refs = state.document.links[scene_id] || []; | |
| acc[scene_id] = { | |
| ...scene_node, | |
| children_refs, | |
| }; | |
| return acc; | |
| }, | |
| {} as Record<string, SceneWithChildren> | |
| ); | |
| return { | |
| scenesmap, | |
| scenes_ref: state.document.scenes_ref, | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-scene.tsx
around lines 74-96, the scenesmap is typed as grida.program.nodes.SceneNode but
you spread a SceneNode and add children_refs which triggers excess-property
errors; define a small extended type (e.g. ExtendedSceneNode extends
grida.program.nodes.SceneNode with children_refs: string[]) and use
Record<string, ExtendedSceneNode> (and the same ExtendedSceneNode for the reduce
accumulator) so the resulting scenesmap correctly includes children_refs.
| /** | ||
| * @deprecated needs re-design | ||
| */ |
There was a problem hiding this comment.
Provide migration guidance for deprecated PreviewProvider.
The PreviewProvider component is marked as deprecated with "needs re-design," but no alternative or migration path is documented. Users need clear guidance on what to use instead or a timeline for when the replacement will be available.
Consider updating the deprecation comment to include:
- The reason for deprecation
- Recommended alternative (if available)
- Timeline for removal (if known)
Example:
/**
- * @deprecated needs re-design
+ * @deprecated This component will be removed in v2.0.0. Use the new PreviewManager from @/grida-canvas-react/preview instead. The current implementation has performance issues with expensive document state updates.
*/📝 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.
| /** | |
| * @deprecated needs re-design | |
| */ | |
| /** | |
| * @deprecated This component will be removed in v2.0.0. Use the new PreviewManager from @/grida-canvas-react/preview instead. The current implementation has performance issues with expensive document state updates. | |
| */ |
🤖 Prompt for AI Agents
In editor/grida-canvas-react-starter-kit/starterkit-preview/index.tsx around
lines 44 to 46, the deprecation comment on PreviewProvider is too terse; update
it to explain why it's deprecated, state the recommended alternative or
temporary workaround, and include a removal timeline or link to an issue/PR
tracking the redesign. Replace the single-line "@deprecated needs re-design"
with a short multi-part comment that: 1) briefly states the reason for
deprecation, 2) names the replacement API/component or suggests a migration
workaround if none exists, and 3) gives an expected removal version/date or a
link to the issue where users can follow progress. Ensure the message is concise
and actionable for users needing to migrate.
|
|
||
| export class DOMDefaultExportInterfaceProvider | ||
| implements editor.api.IDocumentExporterInterfaceProvider | ||
| { | ||
| readonly formats = ["PNG", "JPEG", "PDF", "SVG"]; | ||
|
|
||
| readonly image_export: DOMImageExportInterfaceProvider; | ||
| readonly svg_export: DOMSVGExportInterfaceProvider; | ||
|
|
||
| constructor(readonly editor: Editor) { | ||
| this.image_export = new DOMImageExportInterfaceProvider(editor); | ||
| this.svg_export = new DOMSVGExportInterfaceProvider(editor); | ||
| } | ||
|
|
||
| canExportNodeAs( | ||
| _node_id: string, | ||
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | ||
| ): boolean { | ||
| return this.formats.includes(format); | ||
| } | ||
|
|
||
| async exportNodeAs( | ||
| node_id: string, | ||
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | ||
| ): Promise<Uint8Array | string> { | ||
| assert(this.formats.includes(format), "non supported format"); | ||
|
|
||
| switch (format) { | ||
| case "PNG": | ||
| case "JPEG": { | ||
| return this.image_export.exportNodeAsImage( | ||
| node_id, | ||
| format as "PNG" | "JPEG" | ||
| ); | ||
| } | ||
| case "SVG": { | ||
| return this.svg_export.exportNodeAsSVG(node_id); | ||
| } | ||
| } | ||
|
|
||
| throw new Error("Non supported format"); | ||
| } | ||
| } |
There was a problem hiding this comment.
PDF advertised but not implemented — leads to runtime throw
formats includes "PDF" and canExportNodeAs returns true for it, but exportNodeAs lacks a "PDF" branch and will throw. In editor.exportNodeAs, this path will pass both checks and then crash at runtime.
Apply this diff to avoid false support (until PDF is implemented):
- readonly formats = ["PNG", "JPEG", "PDF", "SVG"];
+ readonly formats = ["PNG", "JPEG", "SVG"] as const;Optionally add a specific message if ever called with "PDF":
switch (format) {
case "PNG":
case "JPEG": {
return this.image_export.exportNodeAsImage(
node_id,
format as "PNG" | "JPEG"
);
}
case "SVG": {
return this.svg_export.exportNodeAsSVG(node_id);
}
+ case "PDF": {
+ throw new Error("PDF export is not supported by the DOM backend");
+ }
}📝 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.
| export class DOMDefaultExportInterfaceProvider | |
| implements editor.api.IDocumentExporterInterfaceProvider | |
| { | |
| readonly formats = ["PNG", "JPEG", "PDF", "SVG"]; | |
| readonly image_export: DOMImageExportInterfaceProvider; | |
| readonly svg_export: DOMSVGExportInterfaceProvider; | |
| constructor(readonly editor: Editor) { | |
| this.image_export = new DOMImageExportInterfaceProvider(editor); | |
| this.svg_export = new DOMSVGExportInterfaceProvider(editor); | |
| } | |
| canExportNodeAs( | |
| _node_id: string, | |
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | |
| ): boolean { | |
| return this.formats.includes(format); | |
| } | |
| async exportNodeAs( | |
| node_id: string, | |
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | |
| ): Promise<Uint8Array | string> { | |
| assert(this.formats.includes(format), "non supported format"); | |
| switch (format) { | |
| case "PNG": | |
| case "JPEG": { | |
| return this.image_export.exportNodeAsImage( | |
| node_id, | |
| format as "PNG" | "JPEG" | |
| ); | |
| } | |
| case "SVG": { | |
| return this.svg_export.exportNodeAsSVG(node_id); | |
| } | |
| } | |
| throw new Error("Non supported format"); | |
| } | |
| } | |
| export class DOMDefaultExportInterfaceProvider | |
| implements editor.api.IDocumentExporterInterfaceProvider | |
| { | |
| readonly formats = ["PNG", "JPEG", "SVG"] as const; | |
| readonly image_export: DOMImageExportInterfaceProvider; | |
| readonly svg_export: DOMSVGExportInterfaceProvider; | |
| constructor(readonly editor: Editor) { | |
| this.image_export = new DOMImageExportInterfaceProvider(editor); | |
| this.svg_export = new DOMSVGExportInterfaceProvider(editor); | |
| } | |
| canExportNodeAs( | |
| _node_id: string, | |
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | |
| ): boolean { | |
| return this.formats.includes(format); | |
| } | |
| async exportNodeAs( | |
| node_id: string, | |
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | |
| ): Promise<Uint8Array | string> { | |
| assert(this.formats.includes(format), "non supported format"); | |
| switch (format) { | |
| case "PNG": | |
| case "JPEG": { | |
| return this.image_export.exportNodeAsImage( | |
| node_id, | |
| format as "PNG" | "JPEG" | |
| ); | |
| } | |
| case "SVG": { | |
| return this.svg_export.exportNodeAsSVG(node_id); | |
| } | |
| case "PDF": { | |
| throw new Error("PDF export is not supported by the DOM backend"); | |
| } | |
| } | |
| throw new Error("Non supported format"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In editor/grida-canvas/backends/dom-export.ts around lines 98–140, the class
advertises "PDF" support in the formats array and method signatures but
exportNodeAs has no PDF branch causing a runtime throw; remove "PDF" from the
readonly formats array and from the allowed format union types in
canExportNodeAs and exportNodeAs (so PDF is not reported as supported) or
alternatively add an explicit "PDF" case in exportNodeAs that throws a clear
NotImplemented/unsupported-for-now error; update any related type annotations to
match the chosen approach.
| export interface IDocumentExporterInterfaceProvider { | ||
| readonly formats: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {})[]; | ||
|
|
||
| canExportNodeAs( | ||
| node_id: string, | ||
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | ||
| ): boolean; | ||
|
|
||
| exportNodeAs( | ||
| node_id: string, | ||
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | ||
| ): Promise<Uint8Array | string>; | ||
| } |
There was a problem hiding this comment.
IDocumentExporterInterfaceProvider.formats type is incorrect (string vs array)
formats should be an array; current union allows a single string which breaks consumers.
- export interface IDocumentExporterInterfaceProvider {
- readonly formats: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {})[];
+ export interface IDocumentExporterInterfaceProvider {
+ readonly formats: Array<"PNG" | "JPEG" | "PDF" | "SVG" | (string & {})>;
@@
- canExportNodeAs(
+ canExportNodeAs(
node_id: string,
format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {})
): boolean;
@@
- exportNodeAs(
+ exportNodeAs(
node_id: string,
format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {})
): Promise<Uint8Array | string>;📝 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.
| export interface IDocumentExporterInterfaceProvider { | |
| readonly formats: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {})[]; | |
| canExportNodeAs( | |
| node_id: string, | |
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | |
| ): boolean; | |
| exportNodeAs( | |
| node_id: string, | |
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | |
| ): Promise<Uint8Array | string>; | |
| } | |
| export interface IDocumentExporterInterfaceProvider { | |
| readonly formats: Array<"PNG" | "JPEG" | "PDF" | "SVG" | (string & {})>; | |
| canExportNodeAs( | |
| node_id: string, | |
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | |
| ): boolean; | |
| exportNodeAs( | |
| node_id: string, | |
| format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) | |
| ): Promise<Uint8Array | string>; | |
| } |
🤖 Prompt for AI Agents
In editor/grida-canvas/editor.i.ts around lines 2319 to 2331, the
IDocumentExporterInterfaceProvider.formats type is currently a union that
permits a single string instead of an array; update the declaration so formats
is an array of allowed format strings (e.g. ("PNG" | "JPEG" | "PDF" | "SVG" |
(string & {}))[] or Array<...>), leaving the canExportNodeAs/exportNodeAs
parameter types as single format values; ensure the readonly modifier remains
and adjust any consuming code/types if necessary to treat formats as an array.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
editor/grida-canvas-hosted/playground/toolbar.tsx (1)
91-147: AddaiSettingsto the dependencies array.The
aiSettingsvalue is used within the callback to construct the prompt template, but it's not included in the dependencies array. IfaiSettingschanges, the callback will continue to use the stale value until another dependency triggers re-creation.Apply this diff to fix the missing dependency:
}, - [editor, generate, editableTextNodes] + [editor, generate, editableTextNodes, aiSettings] );
♻️ Duplicate comments (3)
crates/grida-canvas-wasm/example/rectangle.grida (1)
1-64: Schema structure correctly implements breaking changes.The new document schema properly reflects the breaking changes outlined in the PR objectives:
- Flat
nodesmap storing all nodes (rectangle and scene)- Separate
linksmap for parent-child relationships (main → ["rectangle"])scenes_refarray referencing scenes (["main"])This correctly replaces the old nested
childrenmodel with the new links-based architecture.Note: TypeScript type definitions and WASM bindings are already flagged in past review comments.
editor/grida-canvas-react/use-data-transfer.ts (1)
204-207: Safari fallback should prevent default paste.When
clipboardDatais missing, the code invokesinstance.commands.paste()but does not callevent.preventDefault(). This allows the native paste to also occur. For consistency with the fallback at lines 293-294 and to avoid stray input in some browsers, addpreventDefault.Apply this diff to prevent the native paste:
if (!event.clipboardData) { instance.commands.paste(); + event.preventDefault(); return; }editor/grida-canvas/editor.i.ts (1)
2319-2331: IDocumentExporterInterfaceProvider.formats type is incorrect (string vs array).The
formatsfield type allows a single string instead of requiring an array, which will break consumers expecting an array.Apply this diff to fix the type:
export interface IDocumentExporterInterfaceProvider { - readonly formats: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {})[]; + readonly formats: Array<"PNG" | "JPEG" | "PDF" | "SVG" | (string & {})>; canExportNodeAs( node_id: string, format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) ): boolean; exportNodeAs( node_id: string, format: "PNG" | "JPEG" | "PDF" | "SVG" | (string & {}) ): Promise<Uint8Array | string>; }
🧹 Nitpick comments (3)
editor/grida-canvas/editor.i.ts (2)
1368-1520: Document format migration is correct but complex.The dual format support correctly handles backward compatibility by migrating old
scenes: Recordformat to newscenes_ref: string[]format. The migration creates SceneNode objects from Scene inputs and properly excludes the old property via destructuring.Consider:
- Adding unit tests specifically for the migration path to prevent regressions.
- Documenting a deprecation timeline for the old format.
- Adding inline comments explaining the migration strategy for future maintainers.
// Migration strategy: // 1. Old format: scenes: Record<id, Scene> with children_refs // 2. New format: scenes_ref: string[] with nodes containing SceneNode and links: Record<id, children> // 3. Migration creates SceneNode from Scene and moves children_refs to links
2211-2318: API surface expansion is well-organized.The new interfaces provide comprehensive document manipulation capabilities organized by concern (brush, camera, geometry, export, fonts, images). The
ICameraActionsdual API (property + method) is intentional to support both simple transform updates and synchronized updates that recalculate cursor position.Consider documenting the relationship between the property setter and
transformWithSyncmethod in the interface JSDoc to help consumers choose the right API:/** * @get the transform of the camera * @set set the transform of the camera (use transformWithSync if cursor sync is needed) */ transform: cmath.Transform;editor/grida-canvas-react-starter-kit/starterkit-loading/loading.tsx (1)
36-36: Optional: Clean up completed animations individually.The animation tracking correctly stores all animations in
activeAnimationsRefand clears them viaclearAllAnimations. However, completed animations remain in the array until the nextclearAllAnimationscall. While not a bug (callingstop()on completed animations is harmless), consider removing animations from the array when they complete to keep the array lean.Example approach for
createStepAnimation:const createStepAnimation = async ( stepProgress: number, nextStepProgress: number, maxProgress: number ) => { progressValue.set(stepProgress); setProgress(stepProgress); // Animate to next step const animation = animate(progressValue, nextStepProgress, { duration: expectedDuration / 1000, ease: motionSteps(10, "end"), onUpdate: (value: number) => setProgress(value), }); activeAnimationsRef.current.push(animation); await animation; + // Remove completed animation + activeAnimationsRef.current = activeAnimationsRef.current.filter(a => a !== animation); return createAsymptoticAnimation(maxProgress, nextStepProgress); };Apply similar changes to
createLinearAnimationand optionally tocreateAsymptoticAnimationif you add completion tracking.Also applies to: 57-57, 76-77, 95-96, 101-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
crates/grida-canvas-wasm/example/rectangle.grida(1 hunks)editor/app/(dev)/canvas/examples/inset/page.tsx(1 hunks)editor/app/(dev)/canvas/examples/minimal/page.tsx(1 hunks)editor/grida-canvas-hosted/playground/toolbar.tsx(9 hunks)editor/grida-canvas-react-starter-kit/starterkit-loading/loading.tsx(9 hunks)editor/grida-canvas-react/use-data-transfer.ts(8 hunks)editor/grida-canvas-react/viewport/size.tsx(2 hunks)editor/grida-canvas/action.ts(1 hunks)editor/grida-canvas/editor.i.ts(20 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- editor/grida-canvas-react/viewport/size.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-26T09:29:53.155Z
Learnt from: CR
PR: gridaco/grida#0
File: crates/grida-canvas-wasm/AGENTS.md:0-0
Timestamp: 2025-09-26T09:29:53.155Z
Learning: Applies to crates/grida-canvas-wasm/{crates/grida-canvas-wasm/src/main.rs,**/grida-canvas-wasm.d.ts} : When introducing new public APIs in the WASM entrypoint (main.rs), update the TypeScript declarations in grida-canvas-wasm.d.ts to keep bindings in sync
Applied to files:
crates/grida-canvas-wasm/example/rectangle.grida
🔇 Additional comments (19)
editor/app/(dev)/canvas/examples/minimal/page.tsx (1)
41-41: Simplified to default editor initialization.The change from explicit initialization to
useEditor()with default state aligns with the breaking schema changes mentioned in the PR objectives (version 0.0.1-beta.1+20251010). This simplification is appropriate for example code.Verify that the default editor state provides functional parity for this minimal example:
editor/app/(dev)/canvas/examples/inset/page.tsx (1)
18-18: Consistent simplification with default editor state.This change mirrors the pattern in
minimal/page.tsxand removes the detailed document/scenes/nodes configuration mentioned in the AI summary. The simplification is consistent with the PR's breaking schema changes and move toward default initialization.Since this example previously had "detailed document, scenes, nodes, and editable flag setup," verify that the default state provides equivalent functionality:
crates/grida-canvas-wasm/example/rectangle.grida (1)
29-29: Verify cornerRadius format aligns with the enum restructuring.The AI summary indicates a "corner radius enum" restructuring, but this example uses a simple integer (
0). Confirm whether:
- Integer format is still supported for backward compatibility, or
- This should use an enum/object structure like
{ type: "uniform", value: 0 }editor/grida-canvas-react/use-data-transfer.ts (2)
61-61: LGTM! Camera API migration is consistent.The transition from
instance.clientPointToCanvasPointtoinstance.camera.clientPointToCanvasPointis applied consistently across all coordinate translation paths.Also applies to: 87-87, 139-139
65-73: Verify mutation pattern compatibility with CRDT/transaction model.The migration to the commands API is consistent and well-executed. However, after creating nodes via commands (e.g.,
instance.commands.createTextNode), the code directly mutates properties likenode.$.name,node.$.left,node.$.fill, etc.Given that the PR introduces a CRDT model with JSON Patch transactions and batch operations, confirm that this direct mutation pattern is compatible with the transactional/patch-based workflow. If the commands API is transactional and mutations should be captured as patches, consider whether these property assignments should also be routed through commands or wrapped in a transaction.
Also applies to: 95-113, 127-148, 205-205, 234-234, 249-249, 252-252, 255-255, 293-293
editor/grida-canvas-hosted/playground/toolbar.tsx (4)
168-242: LGTM! Consistent migration to surface API.The tool-setting logic has been uniformly migrated to use
editor.surface.surfaceSetToolwith thetoolbar_value_to_cursormodehelper, maintaining consistency across all toolbar tool interactions.
282-286: Verify the tool-setting pattern for bitmap-specific tools.Unlike other tool selections that route through the
toolbar_value_to_cursormodehelper, the flood-fill tool directly passes an object literal tosurfaceSetTool. Confirm whether this inconsistency is intentional (e.g., bitmap tools don't map toToolbarToolType) or if the helper should be extended to handle bitmap-specific tools for consistency.
297-305: LGTM! Correct context binding for the exit handler.The use of
.bind(editor)ensures the correctthiscontext when the method is invoked as an onClick handler.
369-373: LGTM! Correct context binding for the color change handler.The use of
.bind(editor)ensures the correctthiscontext when the method is invoked as an onColorChange callback.editor/grida-canvas/action.ts (1)
16-16: Verify that__InternalResetActionhas been removed from all call sites.The AI summary indicates that
__InternalResetActionwas removed from theInternalActionunion. Ensure that no code still attempts to dispatch or handle this action type.Run the following script to search for any remaining references to
__InternalResetAction:editor/grida-canvas/editor.i.ts (8)
2-2: LGTM!Import changes align with the Action rename and the new tree.graph.Graph usage in the document context initialization.
Also applies to: 15-15
69-132: LGTM!The Mutex implementation correctly enforces mutual exclusion for synchronous code. The documentation clearly explains its purpose (preventing feedback loops in bidirectional bindings) and limitations (synchronous-only). The try/finally block ensures the lock is released even if the callback throws.
134-134: LGTM!The NodeID type alias and IStoreSubscriptionTrait interface are clean additions. The subscription trait correctly matches the React useSyncExternalStore API pattern.
Also applies to: 145-153
890-921: LGTM!The ephemeral chat additions to multiplayer cursors are well-structured. Using
Record<string, MultiplayerCursor>instead of an array provides efficient lookups and natural deduplication. The local_cursor_chat state properly tracks the local user's chat input.Also applies to: 1481-1486
1947-1967: LGTM!The History Patch types align well with JSON Patch format (RFC 6902). Using
path: (string | number)[]as an internal representation is appropriate, and theHistoryEntrystructure with both patches and inversePatches properly supports undo/redo operations.
1991-2048: LGTM!The
editor.multiplayer.AwarenessPayloadstructure is well-designed for CRDT-based collaboration. Separating rarely-changing data (profile) from frequently-changing data (geo) enables efficient updates, and the cursor_chat field properly supports ephemeral messaging.
2051-2083: LGTM!The JSON Patch utilities correctly implement RFC 6901 (JSON Pointer) encoding. The escape sequence
~→~0and/→~1is properly handled inencodeJsonPointerSegment, andtoJsonPatchOperationscorrectly converts the internal Patch format to the standard JsonPatchOperation format.
2085-2097: LGTM!The subscription callback types provide a clean API for reactive UI integration. The selector-based variant properly includes both current and previous values for efficient change detection, and the inclusion of action and patches parameters enables detailed change tracking.
editor/grida-canvas-react-starter-kit/starterkit-loading/loading.tsx (1)
230-230: Verify intentional breaking change: minDuration default increased from 0 to 1000ms.The default
minDurationhas changed from0to1000milliseconds. This means the loading overlay will now always display for at least 1 second, even if the loading operation completes instantly. This is a breaking behavioral change that affects all consumers of this component.Verify that:
- This change is intentional and aligns with the desired UX
- All consumers of
FullscreenLoadingOverlayare aware of this change- Fast operations (< 1s) won't have unnecessarily prolonged loading overlays
Consider documenting this change in the PR description or migration notes if it's part of the breaking changes mentioned in the PR label.
| /** | ||
| * Error message to display in the loading overlay. | ||
| */ | ||
| errmsg?: string | null; |
There was a problem hiding this comment.
Accessibility improved, but core UX issue persists.
The accessibility attributes (role="alert", aria-live="assertive") have been added as suggested in the previous review, which improves screen reader support. However, the core UX concern from the previous review remains unaddressed: displaying an error message while the loading animation and progress bar continue creates a contradictory user experience.
Users seeing both the spinning progress bar and an error message may be confused about whether:
- The operation is still in progress (progress bar suggests this)
- The operation has failed (error message suggests this)
- They should wait or take action
Consider implementing one of these approaches to resolve the UX confusion:
Option 1: Stop loading state when error occurs
useEffect(() => {
- if (loading) {
+ if (loading && !errmsg) {
startAnimation();
} else {
setProgress(0);
progressValue.set(0);
}
- }, [loading, expectedDuration, steps, step, maxFakedProgress]);
+ }, [loading, errmsg, expectedDuration, steps, step, maxFakedProgress]);Option 2: Hide progress bar when error is shown
- <UXProgress
+ {!errmsg && <UXProgress
loading={loading}
expectedDuration={expectedDuration}
steps={steps}
step={step}
maxFakedProgress={maxFakedProgress}
className="w-52"
- />
+ />}Option 3: Replace overlay content entirely on error
Show a distinct error state instead of mixing loading and error UI.
Also applies to: 236-236, 297-308
🤖 Prompt for AI Agents
In editor/grida-canvas-react-starter-kit/starterkit-loading/loading.tsx around
lines 193-196 (also review lines 236 and 297-308), the component currently shows
the loading spinner/progress bar while also rendering errmsg, causing
contradictory UX; change the render logic so that when errmsg is non-null you do
not show the spinner/progress UI — either set loading state to false when errmsg
is set or short-circuit the overlay render to return a dedicated error view
(hide/skip progress bar and spinner and render an accessible error block with
role="alert" and retry/close action). Ensure only the error UI appears whenever
errmsg is present and update any conditional checks around the progress bar and
spinner at the referenced lines accordingly.
| /** | ||
| * Document Reset Action | ||
| * | ||
| * Special marker action emitted when the entire document state is replaced via `reset()`. | ||
| * This action is NOT handled by the global actions reducer - it only marks that a full | ||
| * state replacement occurred. Subscribers can check for this action to distinguish | ||
| * between full resets and incremental changes. | ||
| */ | ||
| export interface DocumentResetAction { | ||
| type: "document/reset"; | ||
| /** | ||
| * Unique identifier for this reset operation (auto-generated timestamp if not provided) | ||
| */ | ||
| document_key: string; | ||
| } |
There was a problem hiding this comment.
Make document_key optional to match the documented auto-generation behavior.
The JSDoc states document_key is "auto-generated timestamp if not provided," but the field is typed as required (document_key: string). This creates a mismatch between the interface contract and the documented behavior.
Apply this diff to make the field optional:
export interface DocumentResetAction {
type: "document/reset";
/**
* Unique identifier for this reset operation (auto-generated timestamp if not provided)
*/
- document_key: string;
+ document_key?: string;
}Alternatively, if document_key is always required at the action level (with auto-generation happening at the call site), update the JSDoc to clarify that the field is mandatory in the action payload:
/**
- * Unique identifier for this reset operation (auto-generated timestamp if not provided)
+ * Unique identifier for this reset operation
*/
document_key: string;📝 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.
| /** | |
| * Document Reset Action | |
| * | |
| * Special marker action emitted when the entire document state is replaced via `reset()`. | |
| * This action is NOT handled by the global actions reducer - it only marks that a full | |
| * state replacement occurred. Subscribers can check for this action to distinguish | |
| * between full resets and incremental changes. | |
| */ | |
| export interface DocumentResetAction { | |
| type: "document/reset"; | |
| /** | |
| * Unique identifier for this reset operation (auto-generated timestamp if not provided) | |
| */ | |
| document_key: string; | |
| } | |
| export interface DocumentResetAction { | |
| type: "document/reset"; | |
| /** | |
| * Unique identifier for this reset operation (auto-generated timestamp if not provided) | |
| */ | |
| document_key?: string; | |
| } |
🤖 Prompt for AI Agents
In editor/grida-canvas/action.ts around lines 18 to 32, the DocumentResetAction
interface documents that document_key is "auto-generated timestamp if not
provided" but currently types document_key as required; make the types match the
docs by changing the interface to mark document_key optional (document_key?:
string) so callers may omit it, or alternatively update the JSDoc to state that
document_key is required if you prefer enforcing generation at the call site;
implement the former unless you intend generation elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
editor/grida-canvas/reducers/document.reducer.ts (1)
921-928: Guard node existence and prefer dq lookup for resizeDirect access can yield undefined. Use dq.__getNodeById and skip scenes.
- for (const node_id of target_node_ids) { - const node = draft.document.nodes[node_id]; - updateNodeTransform(node, { + for (const node_id of target_node_ids) { + const node = dq.__getNodeById(draft, node_id); + if (!node || (node as any).type === "scene") continue; + updateNodeTransform(node as any, { type: "resize", delta: [dx, dy], }); }
🧹 Nitpick comments (4)
editor/grida-canvas/reducers/document.reducer.ts (4)
112-156: Initialize entry_scene_id when adding the first sceneIf this is the first scene, set entry_scene_id so consumers have a stable entry point.
// 1. Add to scenes_ref array draft.document.scenes_ref.push(scene_id); + // Initialize entry scene if missing + if (!draft.document.entry_scene_id) { + draft.document.entry_scene_id = scene_id; + }
1252-1261: Autolayout uses non‑null scene_id; add local guardThis path relies on
state.scene_id!. Add an early return if no scene to avoid throws after removing the global assert.- const groups = Object.groupBy( + if (!state.scene_id) return state; + const groups = Object.groupBy( target_node_ids, (node_id) => dq.getParentId(state.document_ctx, node_id) ?? state.scene_id! );
1823-1833: Use draft within immer recipe to avoid stale readsRead scene children from draft, not state, for consistency and to prevent subtle stale‑state bugs.
- // Get scene children from links - const scene_children = state.document.links[state.scene_id!] || []; + // Get scene children from links + const scene_children = draft.document.links[draft.scene_id!] || []; const root_template_instance = dq.__getNodeById( - draft, - // FIXME: update api interface - scene_children[0] + draft, + // FIXME: update api interface + scene_children[0] );
1969-1973: Flatten helpers assume scene_id/root links; add guardBoth
flatten_with_unionand__flatten_group_with_unionrely ondraft.scene_id!and scene root links. Add a guard to no‑scene cases to avoid runtime errors when the global assert is removed.function flatten_with_union<S extends editor.state.IEditorState>( draft: Draft<S>, supported_node_ids: string[], context: ReducerContext ): string[] { - if (supported_node_ids.length === 0) return []; + if (supported_node_ids.length === 0) return []; + if (!draft.scene_id) return []; ... - const groups = Object.groupBy( + const groups = Object.groupBy( supported_node_ids, (id) => dq.getParentId(draft.document_ctx, id) ?? draft.scene_id! );Also applies to: 1978-1984, 1998-2002
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx(7 hunks)editor/grida-canvas/reducers/document.reducer.ts(65 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx (4)
editor/grida-canvas-react/provider.tsx (1)
useCurrentSceneState(341-347)packages/grida-canvas-schema/grida.ts (1)
Node(1103-1123)editor/grida-canvas-react-starter-kit/starterkit-hierarchy/utils.ts (1)
toReversedCopy(3-8)editor/grida-canvas/editor.ts (2)
name(4039-4045)name(4050-4052)
editor/grida-canvas/reducers/document.reducer.ts (10)
packages/grida-canvas-io-figma/lib.ts (1)
document(218-285)packages/grida-canvas-schema/grida.ts (2)
Document(784-794)SceneNode(1959-1970)editor/grida-canvas/reducers/utils/immer.ts (1)
updateState(3-13)editor/grida-canvas/policy.ts (1)
EDITOR_GRAPH_POLICY(14-64)editor/grida-canvas/reducers/methods/insert.ts (1)
self_insertSubDocument(9-61)editor/grida-canvas/utils/insertion.ts (2)
getPackedSubtreeBoundingRect(14-35)getViewportAwareDelta(49-59)editor/grida-canvas/reducers/node-transform.reducer.ts (1)
updateNodeTransform(70-202)editor/grida-canvas/reducers/methods/wrap.ts (2)
self_wrapNodes(65-153)self_wrapNodesAsBooleanOperation(259-352)editor/grida-canvas/reducers/methods/move.ts (1)
self_moveNode(9-43)editor/grida-canvas/reducers/methods/delete.ts (1)
self_try_remove_node(13-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: cargo test
🔇 Additional comments (10)
editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx (7)
167-173: LGTM! Destructuring aligns with the new scene model.The aliasing of
children_refstochildrenis clear, and the addition ofidproperly supports the scene-as-node model described in the PR objectives.
204-230: LGTM! Parent lookup migration is correct.The refactor to use
editor.state.document_ctx.lu_parentis consistent with the new context-based model, and the stop condition at line 208 correctly prevents expanding the scene itself.
239-241: LGTM! Root ID aligns with scene-as-node model.Using the scene
idas the tree root is consistent with the new architecture where scenes are part of the nodes tree.
252-252: Verify the type assertion is safe.Ensure that
itemsfromsetSelectedItemsare guaranteed to bestring[]by the headless-tree library. If there's any uncertainty, consider adding runtime validation or a proper type guard.
291-294: Previous issue resolved! getChildren now always returns an array.The addition of
?? []ensures thatgetChildrenreturns an empty array whenlu_children[itemId]is undefined, which addresses the previous review comment. The same pattern is correctly applied at line 277 for consistency.
376-390: LGTM! API migrations are consistent.The updates to use:
editor.surface.surfaceHoverNodefor hover state (lines 376, 379)editor.doc.getNodeById(node.id).namesetter for renaming (line 382)editor.commands.toggleNodeLockedandeditor.commands.toggleNodeActivefor toggles (lines 386, 389)are consistent with the new command-based and surface abstraction APIs described in the PR objectives.
263-268: Verify completeness of isItemFolder
tree-node.tsx:263-268currently checks onlyscene,container,group, andboolean. Confirm that allNodevariants with achildrenproperty (e.g.,ComponentNode,InstanceNode, etc.) are included.editor/grida-canvas/reducers/document.reducer.ts (3)
89-100: Helper looks goodgetScene is clear and enforces type via assert. LGTM.
244-255: LGTM: scene background color mutationDirectly updating the SceneNode is consistent with the new scene‑as‑node model.
840-847: Root scene move/order semantics look correct with graph policyUsing scene_id as the implicit root and Graph.order/self_moveNode is consistent with scene‑as‑node; context refreshes are handled. LGTM.
If there are any flows that still assume a dedicated "" sentinel, please verify they’ve been migrated to
scene_idbased roots.Also applies to: 1983-2049
| // TODO: introduce a new mv command, where it preserves the absolute position of the moving node, while entering/exiting the parent | ||
| // simplu calling mv will only change the hierarchy, causing its location to change visually, not the expected ux when working with the tree ui. | ||
| editor.commands.mv(ids, target_id, index); |
There was a problem hiding this comment.
Address the UX issue with node repositioning.
The TODO comment highlights a significant UX problem: when users drag nodes in the hierarchy tree, the current mv command only changes the hierarchy without preserving the node's absolute visual position. This breaks user expectations—moving a node in the tree should maintain its on-canvas location while changing only its parent-child relationship.
This degrades the hierarchy panel UX and should be resolved before merging.
Do you want me to help design a solution that preserves absolute positioning during hierarchy moves, or open an issue to track this task?
| assert(state.scene_id, "scene_id is required for autolayout"); | ||
| const scene = state.document.scenes[state.scene_id]; | ||
|
|
||
| switch (action.type) { |
There was a problem hiding this comment.
Global scene_id assert breaks scenes/new and no‑scene flows
Asserting scene_id before the switch prevents creating the first scene and blocks other flows that don’t require a scene.
Apply this diff to remove the global assert and rely on local guards where needed:
if (!state.editable) return state;
-
- assert(state.scene_id, "scene_id is required for autolayout");Also add explicit asserts/guards inside branches that rely on scene_id (e.g., autolayout, insert, operations using state.scene_id!).
📝 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.
| assert(state.scene_id, "scene_id is required for autolayout"); | |
| const scene = state.document.scenes[state.scene_id]; | |
| switch (action.type) { | |
| if (!state.editable) return state; | |
| switch (action.type) { | |
| // … existing cases … | |
| } |
🤖 Prompt for AI Agents
In editor/grida-canvas/reducers/document.reducer.ts around lines 109–111, remove
the global assert on state.scene_id that runs before the action switch because
it blocks "scenes/new" and no‑scene flows; instead, delete that assert and add
targeted guards or asserts inside only the switch branches that actually require
a scene_id (for example: autolayout, insert, and any branch that currently uses
state.scene_id!); ensure each of those branches either early‑returns or throws a
clear error when scene_id is missing so flows that don’t need a scene continue
unaffected.
| case "scenes/delete": { | ||
| const { scene: scene_id } = action; | ||
| return updateState(state, (draft) => { | ||
| // Use Graph.rm() to remove scene and all its children | ||
| const graph = new tree.graph.Graph(draft.document, EDITOR_GRAPH_POLICY); | ||
| const removed_ids = graph.rm(scene_id); | ||
|
|
||
| // Remove from scenes_ref array | ||
| draft.document.scenes_ref = draft.document.scenes_ref.filter( | ||
| (id) => id !== scene_id | ||
| ); | ||
|
|
||
| // Update context from graph's cached LUT | ||
| draft.document_ctx = graph.lut; | ||
|
|
||
| // Update scene_id if the deleted scene was active | ||
| if (draft.scene_id === scene_id) { | ||
| draft.scene_id = draft.document.scenes_ref[0]; | ||
| } | ||
| if (draft.document.entry_scene_id === scene_id) { | ||
| draft.document.entry_scene_id = draft.scene_id; | ||
| } | ||
| // Clear scene-specific state | ||
| Object.assign(draft, editor.state.__RESET_SCENE_STATE); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Deleting a scene: choose a robust fallback scene_id
Currently picks the first scene after deletion; if none remain, scene_id becomes undefined. Also consider selecting the closest neighbor to the deleted index.
- if (draft.scene_id === scene_id) {
- draft.scene_id = draft.document.scenes_ref[0];
- }
- if (draft.document.entry_scene_id === scene_id) {
- draft.document.entry_scene_id = draft.scene_id;
- }
+ if (draft.scene_id === scene_id) {
+ const deletedIndex = state.document.scenes_ref.indexOf(scene_id);
+ const remaining = draft.document.scenes_ref;
+ const fallback =
+ remaining.length > 0
+ ? remaining[Math.min(deletedIndex, remaining.length - 1)]
+ : undefined;
+ (draft as any).scene_id = fallback as any; // adjust if type allows null
+ if (!fallback) {
+ draft.document.entry_scene_id = undefined as any;
+ } else if (draft.document.entry_scene_id === scene_id) {
+ draft.document.entry_scene_id = fallback;
+ }
+ } else if (draft.document.entry_scene_id === scene_id) {
+ draft.document.entry_scene_id = draft.scene_id;
+ }🤖 Prompt for AI Agents
In editor/grida-canvas/reducers/document.reducer.ts around lines 157-182, the
deletion path currently sets draft.scene_id to the first remaining scene and can
become undefined if no scenes remain; compute a robust fallback by capturing the
pre-deletion scenes_ref and the index of scene_id to delete, then after removing
the id choose the new scene_id as: the previous sibling (index-1) if it exists,
otherwise the next sibling (same index) if it exists, otherwise set
draft.scene_id to null; update draft.document.entry_scene_id the same way (set
to draft.scene_id or null), and ensure you use the pre-deletion index selection
logic before mutating scenes_ref so the fallback picks the closest neighbor when
available.
| // check if the scene exists | ||
| const origin_node = state.document.nodes[scene_id] as | ||
| | grida.program.nodes.SceneNode | ||
| | undefined; | ||
| if (!origin_node || origin_node.type !== "scene") return state; | ||
|
|
||
| const origin_children = state.document.links[scene_id] || []; | ||
| const new_scene_id = context.idgen.next(); | ||
|
|
||
| // Create duplicated SceneNode | ||
| const new_scene_node: grida.program.nodes.SceneNode = { | ||
| ...origin_node, | ||
| id: new_scene_id, | ||
| name: origin_node.name + " copy", | ||
| order: origin_node.order ? origin_node.order + 1 : undefined, | ||
| }; |
There was a problem hiding this comment.
Duplicate scene: order increment bug when origin order is 0
Using a truthy check skips increment for order 0. Use an explicit number check.
- order: origin_node.order ? origin_node.order + 1 : undefined,
+ order:
+ typeof origin_node.order === "number"
+ ? origin_node.order + 1
+ : undefined,📝 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.
| // check if the scene exists | |
| const origin_node = state.document.nodes[scene_id] as | |
| | grida.program.nodes.SceneNode | |
| | undefined; | |
| if (!origin_node || origin_node.type !== "scene") return state; | |
| const origin_children = state.document.links[scene_id] || []; | |
| const new_scene_id = context.idgen.next(); | |
| // Create duplicated SceneNode | |
| const new_scene_node: grida.program.nodes.SceneNode = { | |
| ...origin_node, | |
| id: new_scene_id, | |
| name: origin_node.name + " copy", | |
| order: origin_node.order ? origin_node.order + 1 : undefined, | |
| }; | |
| // Create duplicated SceneNode | |
| const new_scene_node: grida.program.nodes.SceneNode = { | |
| ...origin_node, | |
| id: new_scene_id, | |
| name: origin_node.name + " copy", | |
| order: | |
| typeof origin_node.order === "number" | |
| ? origin_node.order + 1 | |
| : undefined, | |
| }; |
🤖 Prompt for AI Agents
In editor/grida-canvas/reducers/document.reducer.ts around lines 186 to 201, the
scene duplication logic uses a truthy check for origin_node.order which skips
increment when order is 0; change this to an explicit numeric check (e.g.,
typeof origin_node.order === "number" or origin_node.order != null) and set
new_scene_node.order to origin_node.order + 1 when it's a number, otherwise
leave it undefined.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
editor/grida-canvas-react/use-data-transfer.ts (1)
306-348: Missing dependency causes stale closure.The
ondropcallback usesinsertSVG(line 320) but it's not included in the dependency array (line 348). This can cause stale closure bugs whereondropuses an outdated version ofinsertSVG.Apply this diff to fix the missing dependency:
}, - [insertFromFile] + [insertFromFile, insertSVG] );editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx (1)
195-217: Guard against cyclic parent mappings to avoid infinite loopsIf lu_parent has a cycle (e.g., via CRDT merges), the while loop never terminates. Add a visited set guard.
- const getParentIds = (nodeId: string): string[] => { - const parentIds: string[] = []; - let currentId = nodeId; - // Walk up the tree to find all parent IDs - while (currentId) { - const node = editor.state.document.nodes[currentId]; - if (!node) break; - - // Find the parent of this node using the context lookup - const parentId = editor.state.document_ctx.lu_parent[currentId]; - - // Stop at the scene (root) level - don't include the scene itself - if (parentId && parentId !== id) { - parentIds.push(parentId); - currentId = parentId; - } else { - break; - } - } - return parentIds; - }; + const getParentIds = (nodeId: string): string[] => { + const parentIds: string[] = []; + const visited = new Set<string>(); + let currentId = nodeId; + // Walk up the tree to find all parent IDs + while (currentId) { + if (visited.has(currentId)) break; // guard against accidental cycles + visited.add(currentId); + const node = editor.state.document.nodes[currentId]; + if (!node) break; + // Find the parent of this node using the context lookup + const parentId = editor.state.document_ctx.lu_parent[currentId] ?? null; + // Stop at the scene (root) level - don't include the scene itself + if (parentId && parentId !== id) { + parentIds.push(parentId); + currentId = parentId; + } else { + break; + } + } + return parentIds; + };editor/grida-canvas/reducers/document.reducer.ts (1)
2003-2005: Guard order computation when siblings lookup returns no matches.Math.min() on an empty spread becomes Infinity, which can break mv/order logic.
- const order = Math.min( - ...group.map((id) => siblings.indexOf(id)).filter((i) => i >= 0) - ); + const indices = group.map((id) => siblings.indexOf(id)).filter((i) => i >= 0); + const order = indices.length ? Math.min(...indices) : siblings.length;
♻️ Duplicate comments (1)
editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx (1)
291-295: Fixed: getChildren now safely defaults to []Using ?? [] prevents undefined from breaking headless-tree. Good catch.
🧹 Nitpick comments (9)
editor/grida-canvas-react/use-data-transfer.ts (1)
214-224: Consider logging errors for observability.Multiple
try-catchblocks silently swallow errors without logging (clipboard decoding, vector payload parsing, and item insertion). While this is likely intentional for clipboard operations, adding debug/trace-level logging would aid troubleshooting and observability.Example for improved observability:
// In clipboard decoding } catch (error) { console.debug('Failed to decode clipboard item:', error); return null; } // In vector payload parsing } catch (error) { console.debug('Failed to parse vector payload:', error); }Also applies to: 230-237, 262-289
editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx (4)
261-269: Broaden folder detection: include nodes with children; verify type listHard-coding types risks missing droppable/expandable nodes (e.g., component, instance, frame?). Treat nodes with children as folders too.
- isItemFolder: (item) => { - const node = item.getItemData(); - return ( - node.type === "scene" || - node.type === "container" || - node.type === "group" || - node.type === "boolean" - ); - }, + isItemFolder: (item) => { + const node = item.getItemData(); + const hasChildren = + (editor.state.document_ctx.lu_children[node.id]?.length ?? 0) > 0; + return ( + node.type === "scene" || + node.type === "container" || + node.type === "group" || + node.type === "boolean" || + hasChildren + ); + },Please confirm whether other types (e.g., component, instance, frame) should be recognized as folders/droppable targets.
282-285: Drop UX: preserve world position when reparentingmv currently changes hierarchy and may shift visual position. Implement/extend mv to preserve world transform on reparent (e.g., mv(..., { preserveWorldTransform: true }) or a dedicated mvAbs).
I can help sketch the API and compute per-node local transforms post-reparent to keep world-space stable.
286-287: Remove duplicate indent configurationindent is set both in useTree options and Tree props; keep a single source to avoid drift.
- indent: 6, ... - <Tree tree={tree} indent={6}> + <Tree tree={tree} indent={6}>Also applies to: 345-346
340-342: Scope rebuild trigger to hierarchy mapsRebuilding on any document_ctx change may be noisy. Limit to lu_children/lu_parent to reduce unnecessary rebuilds.
-useEffect(() => { - tree.rebuildTree(); -}, [document_ctx]); +useEffect(() => { + tree.rebuildTree(); +}, [document_ctx.lu_children, document_ctx.lu_parent]);editor/grida-canvas/reducers/document.reducer.ts (4)
195-201: Fix duplicate scene order when origin order is 0.Truthiness check skips 0. Use explicit number check.
- order: origin_node.order ? origin_node.order + 1 : undefined, + order: + typeof origin_node.order === "number" + ? origin_node.order + 1 + : origin_node.order,
832-840: Avoid assert crash on placement; provide a safe fallback.walk_to_fit should not crash the reducer if it fails unexpectedly.
- const placement = cmath.packing.ext.walk_to_fit( + let placement = cmath.packing.ext.walk_to_fit( viewport_rect, box, anchors ); - - assert(placement); // placement is always expected since allowOverflow is true + if (!placement) { + // Fallback to 0,0 (canvas space); optional: center viewport instead + placement = { x: 0, y: 0 }; + }
1824-1831: Use draft inside produce recipe, not outer state.Using outer state inside updateState can read stale data when not running under an Immer draft.
- // Get scene children from links - const scene_children = state.document.links[state.scene_id!] || []; + // Get scene children from links + const scene_children = draft.document.links[draft.scene_id!] || [];
2096-2106: Treat missing links as empty to detect empty groups/booleans.Undefined children_refs currently bypass cleanup.
- const children_refs = draft.document.links[parent_id]; - - if (children_refs?.length === 0) { + const children_refs = draft.document.links[parent_id] ?? []; + if (children_refs.length === 0) { // Remove the empty boolean/group node self_try_remove_node(draft, parent_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx(7 hunks)editor/grida-canvas-react/use-data-transfer.ts(8 hunks)editor/grida-canvas/reducers/document.reducer.ts(65 hunks)editor/grida-canvas/reducers/index.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
editor/grida-canvas/reducers/index.ts (5)
packages/grida-canvas-schema/grida.ts (1)
INodeIdGenerator(30-33)editor/grida-canvas/editor.i.ts (4)
IDocumentGeometryQuery(2397-2428)IDocumentVectorInterfaceActions(2430-2432)IEditorState(1300-1319)Patch(1947-1951)editor/grida-canvas/reducers/methods/hover.ts (1)
self_updateSurfaceHoverState(5-43)editor/grida-canvas/reducers/utils/immer.ts (1)
updateState(3-13)editor/grida-canvas/reducers/methods/transform.ts (1)
self_update_gesture_transform(80-112)
editor/grida-canvas/reducers/document.reducer.ts (10)
packages/grida-canvas-schema/grida.ts (2)
Document(784-794)SceneNode(1959-1970)editor/grida-canvas/reducers/utils/immer.ts (1)
updateState(3-13)editor/grida-canvas/policy.ts (1)
EDITOR_GRAPH_POLICY(14-64)editor/grida-canvas/reducers/methods/insert.ts (1)
self_insertSubDocument(9-61)editor/grida-canvas/utils/insertion.ts (2)
getPackedSubtreeBoundingRect(14-35)getViewportAwareDelta(49-59)editor/grida-canvas/reducers/node-transform.reducer.ts (1)
updateNodeTransform(70-202)editor/grida-canvas/reducers/methods/wrap.ts (1)
self_wrapNodes(65-153)editor/grida-canvas/reducers/methods/move.ts (1)
self_moveNode(9-43)packages/grida-tree/src/lib.ts (1)
order(1853-1907)editor/grida-canvas/reducers/methods/delete.ts (1)
self_try_remove_node(13-67)
editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx (3)
editor/grida-canvas-react/provider.tsx (1)
useCurrentSceneState(341-347)editor/grida-canvas-react-starter-kit/starterkit-hierarchy/utils.ts (1)
toReversedCopy(3-8)editor/grida-canvas/editor.ts (2)
name(4039-4045)name(4050-4052)
⏰ 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). (1)
- GitHub Check: cargo test
🔇 Additional comments (10)
editor/grida-canvas-react/use-data-transfer.ts (5)
61-69: LGTM! Clean migration to commands and camera APIs.The coordinate transformation and text node creation have been correctly migrated to use
instance.camera.clientPointToCanvasPointandinstance.commands.createTextNode, aligning with the broader architectural shift to commands-based operations.
87-113: LGTM! Consistent API migration.The coordinate transformation and rectangle node creation correctly use the commands and camera APIs. The
instance.createImagecall remains direct, which is appropriate for resource management operations distinct from node creation.
127-148: LGTM! Proper async handling.The migration to
await instance.commands.createNodeFromSvg(svg)is correct, with the function already declared asasync. Coordinate transformations properly use the camera API.
205-207: LGTM! Past review addressed.The Safari fallback now correctly calls
event.preventDefault()afterinstance.commands.paste(), preventing native paste behavior from interfering with the custom handling.
235-235: LGTM! Complete paste operations migration.All paste operations have been correctly migrated to the commands API (
instance.commands.paste(),instance.commands.pasteVector(),instance.commands.pastePayload()), maintaining consistency with the architectural refactor.Also applies to: 250-256, 294-294
editor/grida-canvas-react-starter-kit/starterkit-hierarchy/tree-node.tsx (1)
239-242: Root as scene id aligns with scene-as-nodeUsing the scene id for rootItemId matches the new model. LGTM.
editor/grida-canvas/reducers/document.reducer.ts (1)
151-155: Verify __RESET_SCENE_STATE does not clobber scene_id or camera transform.Object.assign(draft, __RESET_SCENE_STATE) might reset scene_id or camera unintentionally.
Please confirm the keys in editor.state.__RESET_SCENE_STATE. If it contains scene_id or transform, switch to targeted resets (selection, hovered, edit modes), or explicitly reassign scene_id and preserve transform after reset.
Also applies to: 179-181
editor/grida-canvas/reducers/index.ts (3)
18-19: Good: enable patches for Immer v10.Using enablePatches() with produceWithPatches is the right setup.
Based on learnings
64-79: Load: state vs draft read is fine; confirm __RESET_SCENE_STATE safety.Reading from base state is OK here, but ensure __RESET_SCENE_STATE doesn't reset scene_id unexpectedly.
Please confirm editor.state.__RESET_SCENE_STATE keys as noted in document.reducer comment.
33-41: No action required — all editor/grida-canvas reducer callers destructure [state, patches, inversePatches].
| assert(state.scene_id, "scene_id is required for autolayout"); | ||
| const scene = state.document.scenes[state.scene_id]; | ||
|
|
There was a problem hiding this comment.
Remove unconditional scene_id assert (blocks scenes/new and clean initial load).
Top‑level assert forces scene_id for all actions and can throw on first scene creation or before a scene is set. Rely on per‑branch asserts (already present) instead.
Apply:
- assert(state.scene_id, "scene_id is required for autolayout");
+📝 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.
| assert(state.scene_id, "scene_id is required for autolayout"); | |
| const scene = state.document.scenes[state.scene_id]; |
🤖 Prompt for AI Agents
In editor/grida-canvas/reducers/document.reducer.ts around lines 109-110, remove
the unconditional assert(state.scene_id, "scene_id is required for autolayout")
because it forces scene_id for every action and can throw during initial scene
creation or before a scene is set; instead, delete this global assert and ensure
existing per-branch asserts remain in their respective branches (or add guarded
checks only where scene_id is actually required) so actions that don't need a
scene_id can proceed normally.
| case "scenes/delete": { | ||
| const { scene: scene_id } = action; | ||
| return updateState(state, (draft) => { | ||
| // Use Graph.rm() to remove scene and all its children | ||
| const graph = new tree.graph.Graph(draft.document, EDITOR_GRAPH_POLICY); | ||
| const removed_ids = graph.rm(scene_id); | ||
|
|
||
| // Remove from scenes_ref array | ||
| draft.document.scenes_ref = draft.document.scenes_ref.filter( | ||
| (id) => id !== scene_id | ||
| ); | ||
|
|
||
| // Update context from graph's cached LUT | ||
| draft.document_ctx = graph.lut; | ||
|
|
||
| // Update scene_id if the deleted scene was active | ||
| if (draft.scene_id === scene_id) { | ||
| draft.scene_id = draft.document.scenes_ref[0]; | ||
| } | ||
| if (draft.document.entry_scene_id === scene_id) { | ||
| draft.document.entry_scene_id = draft.scene_id; | ||
| } | ||
| // Clear scene-specific state | ||
| Object.assign(draft, editor.state.__RESET_SCENE_STATE); | ||
| }); |
There was a problem hiding this comment.
Prevent deleting the last scene (or create a fallback).
Deleting the last scene leaves scene_id undefined and breaks subsequent flows. Guard against removing the final scene.
case "scenes/delete": {
const { scene: scene_id } = action;
return updateState(state, (draft) => {
+ // Prevent deleting the last remaining scene
+ if (draft.document.scenes_ref.length <= 1) {
+ console.warn("scenes/delete: refusing to delete the last scene");
+ return;
+ }
// Use Graph.rm() to remove scene and all its children
const graph = new tree.graph.Graph(draft.document, EDITOR_GRAPH_POLICY);
const removed_ids = graph.rm(scene_id);📝 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.
| case "scenes/delete": { | |
| const { scene: scene_id } = action; | |
| return updateState(state, (draft) => { | |
| // Use Graph.rm() to remove scene and all its children | |
| const graph = new tree.graph.Graph(draft.document, EDITOR_GRAPH_POLICY); | |
| const removed_ids = graph.rm(scene_id); | |
| // Remove from scenes_ref array | |
| draft.document.scenes_ref = draft.document.scenes_ref.filter( | |
| (id) => id !== scene_id | |
| ); | |
| // Update context from graph's cached LUT | |
| draft.document_ctx = graph.lut; | |
| // Update scene_id if the deleted scene was active | |
| if (draft.scene_id === scene_id) { | |
| draft.scene_id = draft.document.scenes_ref[0]; | |
| } | |
| if (draft.document.entry_scene_id === scene_id) { | |
| draft.document.entry_scene_id = draft.scene_id; | |
| } | |
| // Clear scene-specific state | |
| Object.assign(draft, editor.state.__RESET_SCENE_STATE); | |
| }); | |
| case "scenes/delete": { | |
| const { scene: scene_id } = action; | |
| return updateState(state, (draft) => { | |
| // Prevent deleting the last remaining scene | |
| if (draft.document.scenes_ref.length <= 1) { | |
| console.warn("scenes/delete: refusing to delete the last scene"); | |
| return; | |
| } | |
| // Use Graph.rm() to remove scene and all its children | |
| const graph = new tree.graph.Graph(draft.document, EDITOR_GRAPH_POLICY); | |
| const removed_ids = graph.rm(scene_id); | |
| // Remove from scenes_ref array | |
| draft.document.scenes_ref = draft.document.scenes_ref.filter( | |
| (id) => id !== scene_id | |
| ); | |
| // Update context from graph's cached LUT | |
| draft.document_ctx = graph.lut; | |
| // Update scene_id if the deleted scene was active | |
| if (draft.scene_id === scene_id) { | |
| draft.scene_id = draft.document.scenes_ref[0]; | |
| } | |
| if (draft.document.entry_scene_id === scene_id) { | |
| draft.document.entry_scene_id = draft.scene_id; | |
| } | |
| // Clear scene-specific state | |
| Object.assign(draft, editor.state.__RESET_SCENE_STATE); | |
| }); | |
| } |
🤖 Prompt for AI Agents
In editor/grida-canvas/reducers/document.reducer.ts around lines 157 to 181, the
delete-scene branch can remove the last scene leaving draft.scene_id undefined;
guard this by checking if draft.document.scenes_ref.length <= 1 before calling
graph.rm and filtering out the scene. If it is the last scene, either abort the
delete (no-op) or create a new fallback scene id, add it to
draft.document.scenes_ref and draft.document (and update
draft.document.entry_scene_id and draft.scene_id) before/after removal;
otherwise proceed with the existing removal logic and then ensure draft.scene_id
and draft.document.entry_scene_id are set to a valid remaining scene id and
update draft.document_ctx and scene-specific state as currently done.
| return updateState(state, (draft: Draft<S>) => { | ||
| draft.gesture_modifiers.tarnslate_with_axis_lock = | ||
| action.tarnslate_with_axis_lock; | ||
| self_update_gesture_transform(draft, context); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Possible typo: tarnslate_with_axis_lock.
Likely meant translate_with_axis_lock. If the state field or action name is spelled differently elsewhere, this silently no-ops.
Proposed fix if it is a typo:
- draft.gesture_modifiers.tarnslate_with_axis_lock =
- action.tarnslate_with_axis_lock;
+ draft.gesture_modifiers.translate_with_axis_lock =
+ action.translate_with_axis_lock;🏁 Script executed:
#!/bin/bash
# Verify consistency of the property name across the repo
rg -nP 'tarnslate_with_axis_lock|translate_with_axis_lock' -C2Length of output: 7614
Global rename tarnslate_with_axis_lock → translate_with_axis_lock
This typo is propagated across the repo—update all occurrences in:
- editor/grida-canvas/action.ts (interface and dispatch)
- editor/grida-canvas/reducers/index.ts
- editor/grida-canvas/reducers/event-target.cem-vector.reducer.ts
- editor/grida-canvas/reducers/methods/transform.ts
- editor/grida-canvas/editor.i.ts
- editor/grida-canvas/editor.ts
🤖 Prompt for AI Agents
In editor/grida-canvas/reducers/index.ts around lines 150 to 153, the state
property and usages are misspelled as "tarnslate_with_axis_lock"; rename this
property everywhere to "translate_with_axis_lock". Update the interface
definition in editor/grida-canvas/action.ts and any dispatch calls to use the
corrected name, change all references in editor/grida-canvas/reducers/index.ts,
editor/grida-canvas/reducers/event-target.cem-vector.reducer.ts,
editor/grida-canvas/reducers/methods/transform.ts,
editor/grida-canvas/editor.i.ts and editor/grida-canvas/editor.ts to the new
spelling, and adjust any typings or tests accordingly so imports and state
accesses remain consistent; run type-check and tests to ensure no remaining
occurrences of the old name.
1 participant
Towards CRDT in production at scale.
Goals:
core-renderer/rs:
core-editor/ts:
Breaking
0.0.1-beta.1+20251010childrenfrom mem modelimpls
Cursor Chat
day-294-grida-canvas-multiplayer-cursor-chat.mp4
Sync
day-295-grida-canvas-multiplayer-sync.mp4
Summary by CodeRabbit
New Features
Refactor
Removed
Documentation
Tests
Chores