refactor(ui): move away from Y.Arrays to Y.Maps #1033
Conversation
WalkthroughThis pull request refactors the workflow data structures in multiple parts of the codebase. It replaces array-based structures (using Changes
Sequence Diagram(s)sequenceDiagram
participant C as Component
participant Y as Yjs Document
participant WM as Workflow Manager
participant UI as Renderer
C->>Y: Request workflows (YMap)
Y-->>C: Return YMap<YWorkflow>
C->>WM: Extract workflows using Object.values / Array.from(entries)
WM-->>C: Rebuild workflows (nodes & edges with map-based access)
C->>UI: Render updated workflows
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
ui/src/lib/yjs/conversions/rebuildWorkflow.test.ts (1)
75-83:⚠️ Potential issueInconsistency in second test case
The second test case still uses the array-based approach (
getArrayandpush), which is inconsistent with the map-based changes made elsewhere.Update the second test case to use the map-based approach:
- const yWorkflows = yDoc.getArray<YWorkflow>("workflows"); + const yWorkflows = yDoc.getMap<YWorkflow>("workflows"); const id = "empty-workflow"; const name = "Empty Workflow"; const yWorkflow = yWorkflowConstructor(id, name, [], []); - yWorkflows.push([yWorkflow]); + yWorkflows.set(id, yWorkflow);ui/src/lib/yjs/conversions/yWorkflowConstructor.test.ts (1)
83-91:⚠️ Potential issueInconsistency in data structure usage.
While most of the file has been updated to use YMap, this test case still uses YArray and the push method, which is inconsistent with the refactoring goal.
- const yWorkflows = yDoc.getArray<YWorkflow>("workflows"); + const yWorkflows = yDoc.getMap<YWorkflow>("workflows"); const id = "workflow-1"; const name = "My Workflow"; const yWorkflow = yWorkflowConstructor(id, name); - yWorkflows.push([yWorkflow]); + yWorkflows.set(id, yWorkflow);
🧹 Nitpick comments (6)
ui/src/lib/yjs/useWorkflowTabs.ts (1)
24-31: Consider updating the dependency arrayWhile the current dependency array works, it's better to directly depend on the
rawWorkflowsobject rather than just its length. This would ensure the memo is recalculated whenever the workflows change, not just when their number changes.- [rawWorkflows.length], // eslint-disable-line react-hooks/exhaustive-deps + [rawWorkflows], // eslint-disable-line react-hooks/exhaustive-depsui/src/hooks/useProjectExport.ts (1)
42-55: Initialization logic improvementThe new initialization approach using a metadata map to track initialization status is more robust than checking array length. However, there's redundant checking of the initialization status.
You can simplify this by removing the nested initialization check since you've already verified it in the outer condition:
const metadata = yDoc.getMap("metadata"); if (!metadata.get("initialized")) { yDoc.transact(() => { const yWorkflows = yDoc.getMap<YWorkflow>("workflows"); if (yWorkflows.get(DEFAULT_ENTRY_GRAPH_ID)) return; - if (!metadata.get("initialized")) { const yWorkflow = yWorkflowConstructor( DEFAULT_ENTRY_GRAPH_ID, "Main Workflow", ); yWorkflows.set(DEFAULT_ENTRY_GRAPH_ID, yWorkflow); metadata.set("initialized", true); - } }); }ui/src/lib/yjs/conversions/yWorkflowConstructor.ts (1)
80-96: Consider adding type checking for optional parametersWhile the implementation is correct, you might want to add some defensive coding to handle potential edge cases.
Consider adding empty array checks before the forEach loops:
const yNodes = new Y.Map() as YNodesMap; - nodes?.forEach((n) => { + (nodes || []).forEach((n) => { const newYNode = yNodeConstructor(n); yNodes.set(n.id, newYNode); }); const yEdges = new Y.Map() as YEdgesMap; - edges?.forEach((e) => { + (edges || []).forEach((e) => { const newYEdge = yEdgeConstructor(e); yEdges.set(e.id, newYEdge); });This ensures the code handles undefined values gracefully without throwing errors.
ui/src/lib/yjs/useYWorkflow.ts (2)
158-271: Consider removing commented-out codeThe entire
handleYWorkflowAddFromSelectionfunction is commented out, and line 356 marks it asundefined. If this functionality is no longer needed, consider removing the commented code entirely to improve code maintainability.
312-314: Simplify array conversionThe mapping operation here is redundant since
Array.from(workflowsToRemove)already gives an array of the set values.- const idsToRemove = Array.from(workflowsToRemove) - .map((id) => id) - .filter(isDefined); + const idsToRemove = Array.from(workflowsToRemove).filter(isDefined);ui/src/lib/yjs/useYEdge.test.ts (1)
129-129: Consider simplifying array conversionThe array conversion
Array.from(Object.values(yEdges.toJSON()))works correctly but seems verbose. If you find yourself doing this conversion frequently across tests, consider creating a utility function for better readability.- const expectedArray = Array.from(Object.values(yEdges.toJSON())) as Edge[]; + // If you do this frequently, a helper function might be cleaner: + // const expectedArray = mapToArray(yEdges) as Edge[]; + const expectedArray = Array.from(Object.values(yEdges.toJSON())) as Edge[];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
ui/src/features/Editor/hooks.ts(4 hunks)ui/src/features/Editor/index.tsx(2 hunks)ui/src/features/Editor/useCanvasCopyPaste.ts(2 hunks)ui/src/features/Editor/useDeployment.ts(3 hunks)ui/src/features/SharedCanvas/hooks.ts(4 hunks)ui/src/features/SharedCanvas/index.tsx(2 hunks)ui/src/hooks/useProjectExport.ts(1 hunks)ui/src/hooks/useProjectImport.ts(1 hunks)ui/src/hooks/useWorkflowImport.ts(1 hunks)ui/src/lib/reactFlow/useEdges.ts(1 hunks)ui/src/lib/yjs/YWorkflowClass.tsx(1 hunks)ui/src/lib/yjs/conversions/rebuildWorkflow.test.ts(2 hunks)ui/src/lib/yjs/conversions/rebuildWorkflow.ts(2 hunks)ui/src/lib/yjs/conversions/yWorkflowConstructor.test.ts(4 hunks)ui/src/lib/yjs/conversions/yWorkflowConstructor.ts(2 hunks)ui/src/lib/yjs/types.ts(1 hunks)ui/src/lib/yjs/useParentYWorkflow/index.ts(2 hunks)ui/src/lib/yjs/useParentYWorkflow/removeParentYWorkflowNodePseudoPort.ts(3 hunks)ui/src/lib/yjs/useParentYWorkflow/updateParentYWorkflowEdges.ts(4 hunks)ui/src/lib/yjs/useParentYWorkflow/updateParentYWorkflowNode.ts(3 hunks)ui/src/lib/yjs/useWorkflowTabs.ts(2 hunks)ui/src/lib/yjs/useYEdge.test.ts(5 hunks)ui/src/lib/yjs/useYEdge.ts(2 hunks)ui/src/lib/yjs/useYLayout.ts(1 hunks)ui/src/lib/yjs/useYNode.test.ts(3 hunks)ui/src/lib/yjs/useYNode.ts(5 hunks)ui/src/lib/yjs/useYWorkflow.ts(10 hunks)ui/src/lib/yjs/useYjsSetup.ts(2 hunks)ui/src/lib/yjs/useYjsStore.ts(1 hunks)
🔇 Additional comments (79)
ui/src/lib/yjs/types.ts (1)
11-15: Excellent refactoring from array to map structuresThe transition from array-based to map-based data structures for nodes, edges, and workflows is a significant improvement. Maps provide O(1) lookup by key which is more efficient for collections with unique identifiers like nodes and edges, compared to O(n) searches in arrays. This change will improve performance when accessing specific items by ID and provides better semantics for these data collections.
ui/src/lib/yjs/useParentYWorkflow/index.ts (2)
3-3: Type imports correctly updated to reflect the YMap structureThe imports have been properly updated to use the new map-based types (
YEdgesMap,YNodesMap) instead of the previous array-based types.
14-15:✅ Verification successful
Type assertions updated to match new map structure
The type assertions for
parentYNodesandparentYEdgeshave been correctly updated to use the new map-based types.
🏁 Script executed:
#!/bin/bash # Check if the imported functions (updateParentYWorkflowEdges and updateParentYWorkflowNode) # have been updated to handle maps instead of arrays grep -A 10 "export function updateParentYWorkflowEdges\|export function updateParentYWorkflowNode" ui/src/lib/yjs/useParentYWorkflow/updateParentYWorkflowEdges.ts ui/src/lib/yjs/useParentYWorkflow/updateParentYWorkflowNode.tsLength of output: 2403
Approval: Type assertions and related functions are correctly updated
The type assertions for
parentYNodesandparentYEdgesnow properly use the new map-based types, and the verification shows that bothupdateParentYWorkflowEdgesandupdateParentYWorkflowNodehave been updated accordingly (using.toJSON()to correctly convert maps for processing).
- The snippet in
ui/src/lib/yjs/useParentYWorkflow/index.tsis correct.- The updated functions in
updateParentYWorkflowEdges.tsandupdateParentYWorkflowNode.tsconfirm the new map structure is handled as expected.ui/src/lib/reactFlow/useEdges.ts (1)
26-31: Clean edge creation logic improvementThe refactored edge creation logic is more concise and easier to follow. Instead of creating a separate variable for the edge ID and then creating the edge object, you now:
- Create the complete edge object with ID in one step
- Check for duplicates using the fully formed edge object
- Add the edge if no duplicate exists
This reduces unnecessary variables and improves code readability while maintaining the same functionality.
ui/src/features/Editor/index.tsx (2)
2-2: Import updated to match new map-based data structureCorrectly updated the import to use
Mapfrom yjs instead ofArray, aligning with the refactoring approach.
18-18:❓ Verification inconclusive
Props type correctly updated to use YMap
The
yWorkflowsprop type has been properly updated fromYArray<YWorkflow>toYMap<YWorkflow>, which is consistent with the overall refactoring from array to map structures.
🏁 Script executed:
#!/bin/bash # Verify that the useHooks implementation correctly handles YMap<YWorkflow> # Check for any array-specific methods that might need updating grep -A 10 "useHooks" ui/src/features/Editor/hooks.ts | head -20Length of output: 64
Action Required: Confirm Correct Handling of YMap in Hooks
The update for the
yWorkflowsprop inui/src/features/Editor/index.tsxfromYArray<YWorkflow>toYMap<YWorkflow>looks correct. However, our initial verification of theuseHooksimplementation inui/src/features/Editor/hooks.tsdid not produce any output. Please manually verify that any array-specific logic in the hooks (for example, methods that assume an array structure) has been properly updated to work with a map.
- Confirm that no residual array methods (e.g.,
.map(),.push()) remain in the implementation ofuseHooks.- Ensure that any operations on
yWorkflowsnow correctly handleYMap<YWorkflow>.ui/src/hooks/useProjectImport.ts (1)
86-87: Properly refactored workflow structure from YArray to YMapThis change correctly implements the data structure transition from array-based to map-based for workflows storage, using the specific DEFAULT_ENTRY_GRAPH_ID key to verify workflow existence rather than checking array length.
ui/src/features/SharedCanvas/index.tsx (2)
2-2: Import statement correctly updated for new data structureThe import statement has been properly updated to import YMap instead of YArray, consistent with the refactoring approach.
16-16: Props type correctly updated to use YMapThe Props type definition has been correctly updated to reflect the change from array-based to map-based workflow structure.
ui/src/features/Editor/useDeployment.ts (3)
2-2: Import statement correctly updated for new data structureThe import has been properly updated to support the YMap data structure.
21-21: Parameter type correctly updated to use YMapThe type definition has been properly updated to reflect the change from array-based to map-based workflow structure.
46-48: Workflow iteration logic properly adapted for map structureThe iteration method has been properly updated to work with the map structure by:
- Converting map entries to an array with
Array.from(yWorkflows.entries())- Mapping over the entries and extracting only the workflow value with
([, w])- Preserving the filtering logic
This maintains the same functional behavior while adapting to the new data structure.
ui/src/hooks/useWorkflowImport.ts (1)
96-105: Workflow storage logic properly refactored for map structureThe workflow import and storage logic has been successfully adapted to use YMap:
- Correctly retrieves a map instead of an array with
getMap<YWorkflow>("workflows")- Uses
forEachinstead ofmapto iterate through workflows- Properly uses
set(w.id, yWorkflow)to store each workflow with its ID as the keyThis implementation ensures workflows can be efficiently accessed by ID and prevents potential duplicates that could occur in an array-based structure.
ui/src/lib/yjs/useYjsSetup.ts (4)
56-56: Type change from array to map for workflow storageThis change is part of a broader refactoring to use a map-based structure for workflow storage instead of an array-based approach. Using a map with explicit keys will provide better data organization and more efficient lookups by ID.
58-58: Updated existence check for map-based structureThe check has been properly updated to use the map's
getmethod to check for the existence of a workflow with the DEFAULT_ENTRY_GRAPH_ID key, which is more direct than checking array length.
65-65: Updated workflow insertion for map-based storageThe insertion method has been correctly changed from using array's
insertmethod to map'ssetmethod, using DEFAULT_ENTRY_GRAPH_ID as the key.
85-85: Updated yWorkflows type referenceThe reference to yWorkflows has been properly updated to use the map-based type instead of array.
ui/src/lib/yjs/useYjsStore.ts (2)
21-21: Type declaration change for yWorkflowsThe type has been correctly updated from
Y.Array<YWorkflow>toY.Map<YWorkflow>to reflect the architectural change from array-based to map-based workflow storage.
27-29: Data transformation logic updated for map structureThe logic has been properly updated to handle the map structure, converting map entries to an array using
Array.from(yWorkflows.entries())and then mapping over them to rebuild workflows.ui/src/features/Editor/useCanvasCopyPaste.ts (2)
5-5: Added Workflow type importThe Workflow type has been added to the imports, which improves type clarity in the component.
19-19: Enhanced type definition for rawWorkflowsThe type has been improved from a generic
Record<string, string | Node[] | Edge[]>[]to the more specificWorkflow[], which enhances type safety and readability.ui/src/lib/yjs/conversions/rebuildWorkflow.test.ts (2)
13-13: Updated test to use map-based workflow storageThe test has been updated to use
getMapinstead ofgetArrayfor the first test case, aligning with the architectural change.
65-65: Updated workflow insertion in testThe test now uses
setwith an ID key instead ofpush, which is consistent with the map-based approach.ui/src/lib/yjs/useWorkflowTabs.ts (1)
4-4: Type simplification looks good!The change from
Record<string, string | Node[] | Edge[]>[]toWorkflow[]simplifies the type definition and improves type safety. This aligns well with the refactoring from yarray to ymap mentioned in the PR title.Also applies to: 13-13
ui/src/lib/yjs/useParentYWorkflow/removeParentYWorkflowNodePseudoPort.ts (3)
4-4: Import type change is appropriateUpdating from
YNodesArraytoYNodesMapaligns with the overall refactoring effort to use map-based structures instead of array-based ones.
14-17: Map transformation looks goodChanging from array access to map access and using
Object.values()to transform the map back to an array for processing is the correct approach when migrating from arrays to maps.
63-66: Direct map manipulation is efficientThe new approach of directly setting the updated node in the map is more efficient than the previous approach of creating new arrays, as it avoids unnecessary object creation.
ui/src/hooks/useProjectExport.ts (1)
42-44: Map-based checks for initializationUsing a metadata map with an "initialized" flag is a more explicit and maintainable approach than checking array length. This makes the initialization status clear and self-documenting.
ui/src/lib/yjs/conversions/yWorkflowConstructor.ts (2)
5-12: Type imports updated appropriatelyAdding the new map type imports (
YEdgesMapandYNodesMap) aligns with the refactoring goals of the PR. Good job keeping the type definitions consistent.
87-96: Map-based node and edge storageConverting from array-based to map-based storage for nodes and edges is a good architectural change. This approach:
- Provides faster lookups by ID
- Avoids duplicate nodes/edges
- Makes updates more efficient
The implementation using
.forEach()to populate the maps is clean and readable.ui/src/lib/yjs/conversions/yWorkflowConstructor.test.ts (3)
4-4: Consistent YArray to YMap migration in workflow structure.The change from YEdgesArray/YNodesArray to YEdgesMap/YNodesMap types along with the switch from getArray to getMap and push to set methods aligns well with the refactoring goals.
Also applies to: 17-18, 22-22
72-79: Appropriate array extraction from maps.The code correctly extracts values from the map entries using Array.from and destructuring. This pattern ensures proper conversion from the map-based storage back to array format for testing.
94-95: Correct map data extraction for empty collections.The test correctly uses Array.from on the YMap instances to extract empty collections, maintaining consistency with the refactoring approach.
ui/src/lib/yjs/conversions/rebuildWorkflow.ts (2)
6-6: Updated import to include the new Map types.The import has been correctly updated to include YNodesMap and YEdgesMap types to match the new data structure.
125-135: Refactored to support map-based workflow structure.The code has been correctly updated to handle nodes and edges as maps instead of arrays, using Array.from to convert map entries to an array for processing. The explicit comments also improve readability.
ui/src/lib/yjs/useYLayout.ts (2)
8-8: Updated types to support map-based workflow structure.The type definitions have been correctly updated to use Y.Map and the more specific Workflow[] type, improving type safety.
Also applies to: 15-16
22-42: Refactored layout change handler for map-based workflow structure.The implementation has been significantly updated to:
- Use forEach instead of map for iteration
- Access nodes through the map structure with proper null handling
- Update nodes using the map's set method
This approach is more aligned with the map-based data structure and handles edge cases properly.
ui/src/features/SharedCanvas/hooks.ts (4)
4-4: Updated YJS import and type definition.The code now correctly imports Map from YJS and updates the yWorkflows parameter type to use YMap instead of YArray.
Also applies to: 22-22
34-39: Refactored workflow extraction from map structure.The code now correctly uses Array.from on the map entries to extract workflows and find the current workflow using the map's get method rather than array indexing.
40-44: Updated node retrieval to use Record instead of array.The rawNodes variable is now correctly typed as a Record<string, Node> and accessed using Object.values, which is appropriate for the map-based structure.
Also applies to: 48-48
66-71: Updated edge retrieval to use Record instead of array.Similar to the nodes, edges are now correctly handled as a Record structure and converted to an array using Object.values when needed.
ui/src/lib/yjs/useYWorkflow.ts (3)
28-34: Good transition from YArray to YMap for workflow managementThe change from
YArray<YWorkflow>toYMap<YWorkflow>is a good improvement that allows for direct access to workflows by ID rather than iterating through an array. This should improve performance, especially as the number of workflows increases.
135-141: The new map-based node management looks goodChanging from array-based methods to map-based methods (
setinstead ofinsert) is consistent with the overall refactoring approach and provides more direct access to nodes.
334-345: Map iteration implementation looks goodProperly updated the workflow renaming logic to use map iteration with
forEachinstead of array-based methods.ui/src/lib/yjs/useYNode.test.ts (3)
17-20: Good transition to YMap in testsThe test correctly implements the new map-based structure by using
getMap<YWorkflow>andset("workflow-1", yWorkflow)instead of the previous array-based methods.
57-59: Map-to-array conversion looks goodThe change from direct array access to
Object.values(yNodes.toJSON())correctly adapts the test to the new map-based structure.
26-26:❌ Incorrect review comment
Consider updating rawWorkflows parameter
The test still passes
rawWorkflows: []to theuseYNodehook, but according to the summary, therawWorkflowsparameter has been removed from the hook. Consider updating this test to match the current implementation.
🏁 Script executed:
#!/bin/bash # Check if rawWorkflows is still a parameter in useYNode grep -A 10 "export default" ui/src/lib/yjs/useYNode.tsLength of output: 296
Clarification: The
rawWorkflowsparameter is still requiredThe current implementation of
useYNodestill defines and expects therawWorkflowsparameter, as verified in the source code. Therefore, passingrawWorkflows: []in the test remains valid, and no update is necessary.Likely an incorrect or invalid review comment.
ui/src/lib/yjs/useYEdge.ts (3)
14-14: Good use of optional parameterMaking
currentYWorkflowoptional is a good practice as it handles cases where the workflow might not be defined, preventing potential runtime errors.
21-38: Effective transition to map-based edge managementThe refactoring of edge management to use map-based operations (
setwith key-value pairs) instead of array-based methods is well-implemented and consistent with the overall refactoring approach.
54-79: Cleaner edge manipulation with map operationsThe edge manipulation operations (add, replace, remove) have been successfully updated to use map methods like
setanddeleteinstead of array methods. This approach is more direct and makes the code easier to understand.ui/src/lib/yjs/useYEdge.test.ts (2)
18-21: Good transition to map-based approach in testsThe test correctly implements the new map-based structure by using
getMap<YWorkflow>andset("workflow-1", yWorkflow)instead of array-based methods.
80-87: Well-implemented map initializationThe initialization of edges has been properly updated to use a map-based approach, creating a
YMapand setting each edge with its ID as the key.ui/src/features/Editor/hooks.ts (6)
11-11: Import updated to reflect the use of YMap instead of YArrayThe change from YArray to YMap is consistent with the overall refactoring in this PR to use map-based structures for workflows.
34-34: Type definition updated from YArray to YMapThis change correctly updates the type of yWorkflows to match the new map-based structure being implemented throughout the codebase.
78-81: Changed rawNodes from YArray to YMap with Record typeGood implementation of the transition to map-based structure. The use of a Record type here provides better type safety and clearer intent.
86-99: Updated node processing to use Object.values and added sortingThe change to
Object.values()is appropriate for iterating over a map-based structure. The added sorting logic is a nice improvement that ensures batch nodes are rendered first, which likely helps with rendering dependencies.
103-106: Changed rawEdges from YArray to YMapSimilar to the rawNodes change, this properly implements the transition to a map-based structure for edges.
111-111: Updated edge processing to use Object.valuesAppropriate change to iterate over the map-based structure for edges.
ui/src/lib/yjs/useParentYWorkflow/updateParentYWorkflowNode.ts (5)
4-4: Updated import to use YNodesMap instead of YNodesArrayThis change correctly reflects the transition to map-based structures for nodes.
10-10: Updated parameter type to YNodesMapCorrectly updates the function parameter type to match the new map-based structure.
14-14: Changed parentNodes construction to use Object.valuesThis is the correct approach for extracting an array of nodes from the map structure for processing.
35-35: Updated parameter type in updatePseudoPorts to YNodesMapConsistent type updates throughout the codebase.
75-78: Improved node update logic for map-based structureThe implementation correctly adapts to the map structure by using
set()operations instead of array manipulations. This is more efficient and clearer than the previous approach.ui/src/lib/yjs/YWorkflowClass.tsx (2)
11-55: Well-designed YWorkflow class implementationThe YWorkflow class provides a clean abstraction over the underlying YMap structure. The implementation ensures proper initialization of required properties and provides clear getter/setter methods. Good use of type safety and encapsulation.
The TODO comment on line 11 suggests this class should be integrated further to reduce type assertions elsewhere, which would be a good follow-up improvement.
57-86: Solid YWorkflows class implementation for managing multiple workflowsThe YWorkflows class effectively manages a collection of YWorkflow instances. The implementation of getWorkflow, setWorkflow, and the values iterator provides a clean API for working with workflows while hiding the map-based implementation details.
ui/src/lib/yjs/useYNode.ts (5)
4-4: Updated imports to reflect new types and structureThe imports have been correctly updated to match the transition to map-based structures.
Also applies to: 7-7
19-22: Updated parameter types to match new data structuresThe changes correctly update the parameter types to reflect the transition from array-based to map-based structures. Making currentYWorkflow optional is a good improvement for handling edge cases.
29-39: Improved node addition logic for map-based structureThe implementation has been correctly updated to use map.set() instead of array operations. This simplifies the code and is more efficient for adding nodes to the structure.
58-130: Thoroughly refactored node change handling for map structuresThe node change handling logic has been comprehensively updated to work with map-based structures. Each case now directly accesses nodes by ID using get() instead of searching through arrays, which improves efficiency. The removal logic also properly updates selected node IDs.
159-193: Updated node data update logic for map-based accessThe node data update logic has been properly adapted to work with the new map structure, using direct ID lookup instead of array indexing.
ui/src/lib/yjs/useParentYWorkflow/updateParentYWorkflowEdges.ts (9)
4-4: Type import updated for map-based workflow structure.The imports have been correctly updated to use map-based types (
YEdgesMap,YNodesMap) instead of array-based types, aligning with the PR's focus on refactoring from yarray to ymap.
10-11: Parameter types correctly updated to use map-based structures.The function parameter types have been appropriately modified to use the new map-based types (
YNodesMap,YEdgesMap), ensuring type safety with the refactored data structures.
15-15: Data access pattern updated for map-based structure.The conversion from map to array now correctly uses
Object.values(), which is the appropriate pattern for extracting values from an object returned bytoJSON()on a map structure.
50-59: Edge finding logic updated for map iteration.The edge finding logic has been properly refactored to work with map entries. The use of
Array.from(parentYEdges).find()with destructuring and the nullish coalescing operator provides a clean approach to find an edge by its properties.
60-60: Added null check improves robustness.The added null check ensures that both
yEdgeIdandyEdgeexist before proceeding with edge updates, preventing potential runtime errors when no matching edge is found.
70-70: Edge update logic simplified for map structure.Replacing array-based operations with
parentYEdges.set(yEdgeId, newYEdge)is a cleaner and more direct approach for updating edges in a map structure.
85-86: Edge access and empty check simplified.The edge access has been simplified, and the empty check now properly handles the map structure, focusing on existence rather than length.
89-91: Edge array creation updated for map iteration.Using
Array.from(parentYEdges).map(([, e]) => ...)correctly converts map entries to an array of edges while ignoring the keys.
93-104: Edge removal logic improved for map structure.The edge removal logic has been well-refactored to use direct
deleteoperations on the map rather than recreating the entire array with filtered results. This approach is more efficient and better aligned with map-based data structures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ui/src/lib/yjs/useYNode.ts (1)
159-165: Consider optimization in node lookup for data updatesWhile the code works correctly by converting the map to JSON and then finding the node by ID, this approach is inefficient. Since you already have the node ID and a map structure, you could directly access the node.
- const nodes = Object.values(yNodes.toJSON()) as Node[]; - const prevNode = nodes.find((n) => n.id === nodeId); + const yNode = yNodes.get(nodeId); + const prevNode = yNode ? yNode.toJSON() as Node : undefined;This optimization avoids unnecessary JSON conversion and array search operations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/lib/yjs/useYNode.ts(5 hunks)
🔇 Additional comments (9)
ui/src/lib/yjs/useYNode.ts (9)
4-7: Simplified imports with updated type structureThe import changes reflect the broader refactoring from array to map-based workflow structures, with the removal of Edge type and addition of Workflow type. The YNodesMap type replaces what was likely YNodesArray previously.
19-21: Type signature improvements enhance flexibility and type safetyThe parameter types have been updated to align with the map-based architecture:
- Making
currentYWorkflowoptional allows for more flexible function usage- Changing
yWorkflowsfromY.Array<YWorkflow>toY.Map<YWorkflow>enables direct lookup by ID- Simplifying
rawWorkflowstoWorkflow[]improves type clarityThese changes create a more maintainable API with better type safety.
29-30: Improved node addition with map-based storageThe node addition logic has been updated to use map operations instead of array operations. This change:
- Safely accesses nodes using optional chaining
- Uses direct ID-based insertion with
set()rather than array manipulation- Simplifies the overall code structure
This approach is more efficient for ID-based node management.
Also applies to: 38-38
58-65: Direct node lookup by ID improves position update efficiencyThe position change handler now directly retrieves nodes by ID using
yNodes.get(change.id)instead of array indexing, making the code more efficient and readable.
69-74: Simplified node replacement logicThe replacement logic has been streamlined to directly set the new node in the map using its ID as the key, rather than manipulating array indices. This approach is more direct and less error-prone.
78-91: Better dimensions handling with optional chainingThe dimensions update case now correctly uses optional chaining when setting properties, providing better null safety. The code is also more concise with direct node access by ID.
96-130: Significantly improved node removal logicThe node removal logic has been substantially enhanced:
- Direct node lookup by ID instead of array traversal
- Cleaner handling of subworkflow and routing port cases
- More maintainable parent workflow reference retrieval
- Simplified selected node ID management
This refactoring makes the code more readable and potentially more performant for node removal operations.
174-182: Improved parent workflow handling with direct map accessThe parent workflow lookup and access has been updated to use the map-based approach:
- Properly uses ID-based lookup with
yWorkflows.get(parentWorkflow.id)- Includes appropriate null checks for robustness
- Clearer flow control with early returns
These changes make the parent workflow handling more reliable and maintainable.
192-192: More direct node data accessThe node data access now directly retrieves the node by ID and then accesses its data, which is more efficient than the previous approach (likely array-based indexing).
KaWaite
changed the title
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Refactor
New Features