-
Notifications
You must be signed in to change notification settings - Fork 131
[Grida Canvas] RC #365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Grida Canvas] RC #365
Changes from all commits
1046ef2
788d6d6
e16027c
d73932f
731e72f
9f77907
65f665a
4a9961b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -138,6 +138,10 @@ export class Editor | |||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public __get_node_siblings(node_id: string): string[] { | ||||||||||||||||||||||||||
| return editor.dq.getSiblings(this.mstate.document_ctx, node_id); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+141
to
+143
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve method naming and document the inherited limitation. The new method provides a useful interface to sibling retrieval, but there are some concerns:
Consider these improvements: - public __get_node_siblings(node_id: string): string[] {
- return editor.dq.getSiblings(this.mstate.document_ctx, node_id);
- }
+ /**
+ * Get sibling node IDs for the given node.
+ *
+ * @warning Root node siblings are not scoped by scene - may include
+ * siblings from other scenes. See issue #<issue-number>
+ */
+ public getNodeSiblings(node_id: string): string[] {
+ return editor.dq.getSiblings(this.mstate.document_ctx, node_id);
+ }This provides:
📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public dispatch(action: Action, force: boolean = false) { | ||||||||||||||||||||||||||
| if (this._locked && !force) return; | ||||||||||||||||||||||||||
| this.mstate = reducer(this.mstate, action); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1418,6 +1418,7 @@ export namespace editor.dq { | |||||
| const parent_id = getParentId(context, node_id); | ||||||
|
|
||||||
| if (!parent_id) { | ||||||
| // FIXME: this is not scoped by the scene - may result unexpected behavior. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Document the known issue with a TODO or track it separately. The comment correctly identifies a known issue where sibling retrieval for root nodes is not scoped by scene. Consider:
Consider updating the comment to be more actionable: - // FIXME: this is not scoped by the scene - may result unexpected behavior.
+ // TODO: #<issue-number> - Root node siblings should be scoped by scene to prevent unexpected behavior across scenes📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| // If the node has no parent, it is at the root level, and all nodes without parents are its "siblings." | ||||||
| return Object.keys(context.__ctx_nid_to_parent_id).filter( | ||||||
| (id) => context.__ctx_nid_to_parent_id[id] === null | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ import { | |
| } from "./methods"; | ||
| import eventTargetReducer from "./event-target.reducer"; | ||
| import documentReducer from "./document.reducer"; | ||
| import equal from "deep-equal"; | ||
| import equal from "fast-deep-equal"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainLGTM! Performance improvement with dependency standardization. The switch from Since this affects the history system's change detection, please verify that the equality behavior remains consistent: 🏁 Script executed: #!/bin/bash
# Description: Verify that fast-deep-equal is used consistently across the codebase and check for any remaining deep-equal imports
# Search for any remaining deep-equal imports
rg "from ['\"]deep-equal['\"]" --type=ts --type=js
# Search for fast-deep-equal usage to confirm consistency
rg "from ['\"]fast-deep-equal['\"]" --type=ts --type=js -A 2Length of output: 1475 Replace all remaining We’ve standardized on • editor/scaffolds/editor/feed.tsx Example replacement: - import equal from "deep-equal";
+ import equal from "fast-deep-equal";After making these changes, rerun a global search to confirm no
|
||
| import grida from "@grida/schema"; | ||
| import { editor } from "@/grida-canvas"; | ||
| import nid from "./tools/id"; | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential stale closure issue with refactored state management.
The refactoring introduces a potential bug where the
nodescomputation references theselectionvariable from a separateuseEditorStatecall. This could lead to stale closure issues where theselectionvalue used in the seconduseEditorStatemight not be the most current one.Consider refactoring to use a single
useEditorStatecall:📝 Committable suggestion
🤖 Prompt for AI Agents