fix(ui): pulled out routers await of the undoTrackerActionWrapper #807
Conversation
WalkthroughThe pull request modifies the Changes
Sequence DiagramsequenceDiagram
participant User
participant useYWorkflow
participant Fetcher
participant YWorkflows
User->>useYWorkflow: Trigger workflow add
useYWorkflow->>Fetcher: Fetch router configs
Fetcher-->>useYWorkflow: Return router configs
useYWorkflow->>YWorkflows: Create workflow with routers
YWorkflows-->>useYWorkflow: Workflow created
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
🧹 Nitpick comments (2)
ui/src/lib/yjs/useYWorkflow.ts (2)
35-41: LGTM! Consider adding error handling.Good job extracting the router fetching logic into a reusable function. The use of
Promise.allfor parallel fetching is efficient.Consider adding error handling with specific error messages:
const fetchRouterConfigs = useCallback(async () => { + try { const [inputRouter, outputRouter] = await Promise.all([ fetcher<Action>(`${api}/actions/InputRouter`), fetcher<Action>(`${api}/actions/OutputRouter`), ]); return { inputRouter, outputRouter }; + } catch (error) { + throw new Error(`Failed to fetch router configs: ${error.message}`); + } }, [api]);
162-252: LGTM! Consider extracting node selection logic.The changes align well with the pattern used in
handleYWorkflowAdd. Good job on maintaining consistency.Consider extracting the node selection and position calculation logic into separate functions for better maintainability:
+const getSelectedNodesWithBatchChildren = ( + nodes: Node[], + nodesByParentId: Map<string, Node[]> +): Set<string> => { + const allIncludedNodeIds = new Set<string>(); + const selectedNodes = nodes.filter((n) => n.selected); + + selectedNodes.forEach((node) => { + allIncludedNodeIds.add(node.id); + if (node.type === "batch") { + (nodesByParentId.get(node.id) ?? []).forEach((batchNode) => + allIncludedNodeIds.add(batchNode.id) + ); + } + }); + + return allIncludedNodeIds; +}; + +const calculateInitialPosition = (selectedNodes: Node[]): XYPosition => ({ + x: Math.min(...selectedNodes.map((n) => n.position.x)), + y: Math.min(...selectedNodes.map((n) => n.position.y)) +}); const handleYWorkflowAddFromSelection = useCallback( async (nodes: Node[], edges: Edge[]) => { try { const routers = await fetchRouterConfigs(); undoTrackerActionWrapper(() => { const nodesByParentId = new Map<string, Node[]>(); nodes.forEach((node) => { if (node.parentId) { if (!nodesByParentId.has(node.parentId)) { nodesByParentId.set(node.parentId, []); } nodesByParentId.get(node.parentId)?.push(node); } }); const selectedNodes = nodes.filter((n) => n.selected); if (selectedNodes.length === 0) return; - const allIncludedNodeIds = new Set<string>(); - selectedNodes.forEach((node) => { - allIncludedNodeIds.add(node.id); - if (node.type === "batch") { - getBatchNodes(node.id).forEach((batchNode) => - allIncludedNodeIds.add(batchNode.id) - ); - } - }); + const allIncludedNodeIds = getSelectedNodesWithBatchChildren( + nodes, + nodesByParentId + ); - const position = { - x: Math.min(...selectedNodes.map((n) => n.position.x)), - y: Math.min(...selectedNodes.map((n) => n.position.y)) - }; + const position = calculateInitialPosition(selectedNodes);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/lib/yjs/useYWorkflow.ts(2 hunks)
🔇 Additional comments (2)
ui/src/lib/yjs/useYWorkflow.ts (2)
44-53: LGTM! Good separation of concerns.The updated signature improves function purity by accepting routers as a parameter instead of fetching them internally. This makes the function more testable and reusable.
122-156: LGTM! Verify undo/redo behavior.Good job moving the async router fetching outside of
undoTrackerActionWrapper. This should fix the undo/redo functionality for new workflows.Let's verify that the undo/redo functionality works correctly with the new changes:
✅ Verification successful
Verified: Async operations correctly handled with undoTrackerActionWrapper
The fix is consistently implemented across the codebase. All async operations are performed before the undoTrackerActionWrapper, and only synchronous state updates are wrapped, ensuring proper undo/redo functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other instances of async operations within undoTrackerActionWrapper # to ensure consistency with this fix. echo "Checking for potential async operations within undoTrackerActionWrapper..." rg -B 3 -A 3 "undoTrackerActionWrapper.*=.*async|undoTrackerActionWrapper.*=>.*async" echo "Checking for await usage within undoTrackerActionWrapper..." rg -B 3 -A 3 "undoTrackerActionWrapper.*\{.*await"Length of output: 428
Script:
#!/bin/bash # Find all usages of undoTrackerActionWrapper with context echo "Finding all usages of undoTrackerActionWrapper..." rg -B 5 -A 5 "undoTrackerActionWrapper" # Also search for any async operations near undo-related code echo -e "\nChecking for async operations near undo-related code..." rg -B 5 -A 5 "undo.*async|async.*undo"Length of output: 24915
Overview
Sub workflow redo/undo was not working when we add a new one.
What I've done
Ensured that async requests are not wrapped by the undoTrackerActionWrapper
What I haven't done
How I tested
Manually
Screenshot
Which point I want you to review particularly
Possible refactors and improvements
Memo
Summary by CodeRabbit
New Features
Refactor