fix(ui): initializing a new main workflow before sync #1014
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request updates the event handling for node double-click interactions across several components. The callback signature has been modified from accepting a full node object to receiving a node identifier and an optional subworkflow identifier. In addition, the Yjs integration is refactored by replacing the workflows state ( Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant N as Node Component
participant H as useNodes Hook
participant C as onNodeDoubleClick Callback
U->>N: Double-click on node
N->>H: Call handleNodeDoubleClick(e, nodeId, subworkflowId)
H->>C: Invoke onNodeDoubleClick(e, nodeId, subworkflowId)
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✨ Finishing Touches
🪧 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 canceled.
|
![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
🧹 Nitpick comments (4)
ui/src/lib/reactFlow/nodeTypes/GeneralNode/index.tsx (1)
123-129: Consider removing commented codeThere's a commented-out section for a "Subworkflow from Selection" menu item. If this feature is planned for future implementation, consider adding a TODO comment explaining the plan. Otherwise, it might be cleaner to remove this commented code.
- {/* <ContextMenuItem - className="justify-between gap-4 text-xs" - disabled={!selected}> - {t("Subworkflow from Selection")} - <Graph weight="light" /> - </ContextMenuItem> */} + {/* TODO: Implement "Subworkflow from Selection" feature in future release */}ui/src/lib/reactFlow/nodeTypes/BatchNode/index.tsx (1)
117-122: Consider removing commented code.The commented menu item for "Subworkflow from Selection" could be removed if it's not planned for the near future, or a TODO comment could be added to explain its purpose.
- {/* <ContextMenuItem - className="justify-between gap-4 text-xs" - disabled={!selected}> - {t("Subworkflow from Selection")} - <Graph weight="light" /> - </ContextMenuItem> */}ui/src/lib/reactFlow/nodeTypes/NoteNode/index.tsx (2)
96-104: Redundant element check.There's a redundant element check in the ref callback. The inner "if (element)" check is unnecessary as it's already within an outer element check.
ref={(element) => { if (element) { - if (element) element.style.setProperty( "color", data.params?.textColor || "", "important", ); } }}
134-139: Consider removing commented code.The commented menu item for "Subworkflow from Selection" could be removed if it's not planned for the near future.
- {/* <ContextMenuItem - className="justify-between gap-4 text-xs" - disabled={!selected}> - {t("Subworkflow from Selection")} - <Graph weight="light" /> - </ContextMenuItem> */}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ui/src/features/Canvas/hooks.ts(5 hunks)ui/src/features/Canvas/index.tsx(4 hunks)ui/src/features/Editor/components/LeftPanel/index.tsx(2 hunks)ui/src/features/Editor/editorContext.tsx(1 hunks)ui/src/features/Editor/hooks.ts(3 hunks)ui/src/features/Editor/index.tsx(3 hunks)ui/src/features/SharedCanvas/hooks.ts(3 hunks)ui/src/features/SharedCanvas/index.tsx(2 hunks)ui/src/lib/reactFlow/nodeTypes/BatchNode/index.tsx(2 hunks)ui/src/lib/reactFlow/nodeTypes/GeneralNode/index.tsx(3 hunks)ui/src/lib/reactFlow/nodeTypes/NodeContextMenu.tsx(0 hunks)ui/src/lib/reactFlow/nodeTypes/NoteNode/index.tsx(2 hunks)ui/src/lib/reactFlow/useNodes.ts(3 hunks)ui/src/lib/yjs/useYjsSetup.ts(2 hunks)ui/src/routes/shared.$sharedToken.lazy.tsx(1 hunks)ui/src/routes/workspaces.$workspaceId_.projects_.$projectId.lazy.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- ui/src/lib/reactFlow/nodeTypes/NodeContextMenu.tsx
🔇 Additional comments (47)
ui/src/features/SharedCanvas/index.tsx (1)
1-1: Clean refactoring to use Doc instead of YArrayThe changes appropriately refactor the component to use the full Yjs
Docobject rather than just theyWorkflowsarray. This approach gives the component direct access to the entire document structure, which is more flexible and aligns with the PR's goal of improving workflow initialization before sync.Also applies to: 13-13, 19-19, 40-40
ui/src/features/Canvas/hooks.ts (2)
7-7: Improved node double-click event handler signatureThe updated signature for
onNodeDoubleClicknow takes specific parameters (nodeIdand optionalsubworkflowId) instead of likely passing the entire node object. This is a more precise approach that provides better type safety and clearer API contracts.Also applies to: 18-22
51-51: Consistent implementation of the updated node double-click handlerThe handler is now properly integrated throughout the component: it's destructured from props, extracted from the
useNodeshook, and included in the returned object. This ensures the event propagates correctly through the component hierarchy.Also applies to: 62-62, 69-69, 86-86
ui/src/features/Editor/index.tsx (2)
2-2: Consistent refactoring to use Doc instead of YArrayThe changes in the Editor component follow the same pattern as in SharedCanvas, replacing
yWorkflowswithyDoc. This consistent approach across components simplifies the data flow and ensures the entire document is available where needed.Also applies to: 17-17, 23-23, 66-66
70-72: Using handleNodeDoubleClick as onSecondaryNodeAction in editor contextThe node double-click handler is now being used as the secondary action handler in the editor context. This aligns well with the signature updates in other files and maintains a consistent pattern for node interactions throughout the application.
ui/src/routes/shared.$sharedToken.lazy.tsx (2)
56-60: Improved destructuring from useYjsSetupThe refactoring from using a nested
stateobject to directly destructuring the needed values (yDoc,isSynced,undoTrackerActionWrapper) creates a cleaner and more maintainable structure. This reduces indirection and makes the component's dependencies more explicit.
61-71: Enhanced dependency checking before renderingThe conditional rendering now properly checks for all required dependencies (
yDoc,isSynced,undoTrackerActionWrapper) before rendering the SharedCanvas component. This ensures that all necessary data is available, preventing potential rendering issues or errors.ui/src/features/Canvas/index.tsx (3)
40-44: Updated callback signature for better data flowThe
onNodeDoubleClickprop type has been updated to pass node identifiers rather than the full node object. This is a good practice as it reduces data coupling and makes the component interfaces more explicit about what data they actually need.
72-72: Improved event handling through hooks abstractionThe addition of
handleNodeDoubleClickand its inclusion in the props passed touseHookscreates a cleaner separation of concerns. Event handling logic now resides in a dedicated hook, which follows React best practices.Also applies to: 82-82
132-132: Consistent callback implementationThe
onNodeDoubleClickhandler is now consistently implemented throughhandleNodeDoubleClickfrom the hook rather than directly using the prop. This ensures that all node double-click events are processed through the same code path.ui/src/lib/reactFlow/useNodes.ts (3)
26-30: Well-defined prop type for node double-click eventThe
onNodeDoubleClickprop type is clearly defined with appropriate optional parameters, which improves type safety and code readability.
264-269: Efficient event handler implementationThe
handleNodeDoubleClickfunction correctly extracts only the necessary data (nodeIdandsubworkflowId) from the node object. This approach is more efficient than passing the entire node object and follows the principle of minimizing data transfer between components.
277-277: Complete hook API exposureProperly exposing the
handleNodeDoubleClickfunction in the returned object ensures that consuming components can access this functionality.ui/src/lib/yjs/useYjsSetup.ts (3)
71-71: Simplified Yjs document handlingThe refactoring to use
yDocdirectly instead of maintainingyWorkflowsin state reduces code complexity and potential state synchronization issues. This is a good architectural improvement.Also applies to: 90-91
74-76: On-demand workflows derivationDeriving
yWorkflowsfromyDocwithin the effect is more efficient as it ensures that workflows are only accessed when needed, rather than being stored separately in state.
87-87: Optimized effect dependenciesThe dependency array now correctly includes only
yDoc, reflecting that the effect no longer depends on other variables, which reduces unnecessary re-executions.ui/src/features/Editor/components/LeftPanel/index.tsx (3)
33-37: Consistent prop typing across componentsThe updated
onNodeDoubleClickfunction signature aligns with the changes in other components, ensuring a consistent API throughout the application.
74-74: Updated callback implementationThe call to
onNodeDoubleClicknow correctly passes the node ID and subworkflow ID instead of the entire node object, adhering to the new signature.
30-31: Explicitly defined callback propsThe
onOpenandonNodesAddprops are now explicitly defined in thePropstype and properly destructured from the props. This improves code clarity and maintainability.Also applies to: 46-47
ui/src/features/Editor/editorContext.tsx (2)
9-9: Import statement correctly updated to remove unused Node importThe import statement has been properly updated to remove the
Nodeimport that's no longer needed after the parameter type change inonSecondaryNodeAction.
13-17: Clean function signature improvementThe signature change from accepting a full Node object to just using a nodeId string with an optional subworkflowId improves the API design by:
- Reducing data passing between components
- Decoupling components from the full Node structure
- Making the interface more focused on what's actually needed
The addition of the subworkflowId parameter also provides better support for nested workflow functionality.
ui/src/routes/workspaces.$workspaceId_.projects_.$projectId.lazy.tsx (3)
71-77: State management refactoring using yDoc directlyThe destructuring from
useYjsSetuphas been updated to extractyDoc,isSynced,undoManager, andundoTrackerActionWrapperdirectly, replacing the previous approach of extractingstate. This change appears to be part of a broader refactoring to simplify how Yjs data is accessed throughout the application.
79-79: Updated conditional rendering with additional checksThe conditional rendering now includes checks for
yDocandundoTrackerActionWrapperin addition toisSynced. This properly ensures all required dependencies are available before rendering the Editor component.
82-86: Props updated to match new state management approachThe Editor component now receives
yDocandundoTrackerActionWrapperdirectly as props, rather than accessed through the state object. This is consistent with the changes in the hook destructuring and provides a more direct data flow.ui/src/features/SharedCanvas/hooks.ts (4)
4-4: Import for Doc from yjs correctly addedThe import for
Docfromyjsis properly added to support the parameter type change.
18-18: Parameter type updated from yWorkflows to yDocThe parameter has been changed from
yWorkflowstoyDocof typeDoc, aligning with the application-wide changes to how Yjs data is accessed.Also applies to: 22-22
26-27: Local derivation of yWorkflows from yDocThe hook now locally derives
yWorkflowsfromyDocusinggetArray<YWorkflow>("workflows"). This ensures consistent access to the workflow data across components and follows the pattern established in other parts of the application.
94-104: Node double-click handler updated to use IDs instead of Node objectsThe
handleNodeDoubleClickfunction now acceptsnodeIdand an optionalsubworkflowIdinstead of a fullNodeobject. The internal logic has been properly adjusted to use these new parameters:
- It checks for
subworkflowIddirectly and callshandleWorkflowOpenif it exists- Otherwise, it fits the view using
nodeIdinstead ofnode.id- The call to
handleNodeLockinghas been updated to usenodeIdThis change aligns with the broader refactoring effort to use identifiers rather than objects for node interactions.
ui/src/lib/reactFlow/nodeTypes/GeneralNode/index.tsx (3)
1-9: Imports updated for new context menu implementationThe import statements have been expanded to include additional icons and components from the
@flow/componentslibrary. TheNodeContextMenuhas been replaced with a more structuredContextMenuimplementation, and necessary hooks have been added for editor context and translations.Also applies to: 13-22
40-52: Added callbacks for node operations using editor contextTwo callback functions have been properly implemented:
handleNodeDelete: UsesonNodesChangeto remove the nodehandleSecondaryNodeAction: UsesonSecondaryNodeActionto trigger actions based on node typeBoth functions are correctly wrapped with
useCallbackfor performance optimization and properly utilize the node ID and subworkflow ID as per the updated API design.
66-138: Enhanced UI with structured context menuThe rendering logic has been upgraded to replace the previous menu system with a comprehensive context menu implementation that includes:
- A context menu trigger wrapping the main node display
- Conditional menu items based on node type (subworkflow vs other nodes)
- Appropriate icons and translations for menu options
- A separator and delete option
- Disabled preview option for action nodes
This enhances usability while maintaining the existing node display functionality.
ui/src/lib/reactFlow/nodeTypes/BatchNode/index.tsx (6)
1-7: LGTM: Clean imports for icons.The icon imports from @phosphor-icons/react are well organized and match the context menu requirements.
11-20: Well-structured component imports.Good organization of the new ContextMenu components and necessary hooks. The isActionNodeType import will be helpful for conditional rendering.
27-34: LGTM: Well-implemented node deletion callback.The handleNodeDelete callback is correctly memoized with useCallback and has the appropriate dependencies.
35-39: LGTM: Secondary action handler looks good.The handleSecondaryNodeAction is properly implemented with null checks and appropriate dependencies.
66-93: LGTM: Context menu trigger implementation.The ContextMenuTrigger wraps the node content well, maintaining the existing styling while adding context menu functionality.
94-131: Clean implementation of the context menu.The context menu implementation handles different node types appropriately, with conditional rendering based on node type and features.
ui/src/features/Editor/hooks.ts (4)
31-36: LGTM: API updated from yWorkflows to yDoc.Good refactoring to use the Doc object directly, which provides more flexibility.
39-51: Good initialization of main workflow when empty.The implementation correctly initializes a new main workflow if none exists, ensuring the application always has a valid starting point.
146-168: Well-structured parameter change for node double-click handler.The handler has been refactored to take a nodeId and subworkflowId instead of the entire node object, which is more efficient and clearer.
170-174: LGTM: Updated callback signature for consistency.The handleNodeDoubleClick callback signature has been updated to match the ref function, maintaining consistency.
ui/src/lib/reactFlow/nodeTypes/NoteNode/index.tsx (6)
1-14: LGTM: Clean imports for icons and context menu components.Added necessary imports to support the new context menu functionality.
22-25: Updated component signature to include id and type.Correctly updated the component signature to include the id and type props needed for the context menu functionality.
27-34: LGTM: Well-implemented node deletion callback.The handleNodeDelete callback is properly implemented with useCallback for memoization.
31-34: LGTM: Secondary action handler looks good.The handleSecondaryNodeAction is properly implemented with null checks and appropriate dependencies.
64-109: LGTM: Context menu trigger with proper styling.The implementation of the context menu trigger maintains the existing styling while adding the new menu functionality.
111-147: LGTM: Well-structured context menu content.The context menu content is well-organized with appropriate items based on node type.
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
New Features
Refactor