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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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