fix(ui): SubWork Flow Multi Select Deletion #743
Conversation
WalkthroughThe pull request modifies the Changes
Suggested reviewers
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. |
✅ 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)
165-173: Consider wrapping Yjs operations in a transaction.While the descending order approach correctly handles index shifting, consider wrapping the Yjs operations in a transaction for better performance and atomicity.
+ // Wrap deletions in a transaction for better performance and atomicity + yWorkflows.doc?.transact(() => { indexesToRemove.forEach((index) => { yWorkflows.delete(index); }); + });Also, consider enhancing the comment to better explain the reasoning:
- // Indexes in descending order to avoid index shifting problems + // Sort indexes in descending order (highest to lowest) to ensure that + // deleting an item doesn't affect the indexes of subsequent deletions
175-183: Consider batching state updates.The state updates could be combined using a single React state update for better performance. Also, there's an indentation inconsistency in the
setOpenWorkflowIdscall.- setWorkflows((currentWorkflows) => { - const remainingWorkflows = currentWorkflows.filter( - (w) => !workflowIds.includes(w.id), - ); - return remainingWorkflows; - }); - - setOpenWorkflowIds((ids) => - ids.filter((id) => !workflowIds.includes(id)), - ); + const updateStates = () => { + setWorkflows((currentWorkflows) => + currentWorkflows.filter((w) => !workflowIds.includes(w.id)) + ); + setOpenWorkflowIds((ids) => + ids.filter((id) => !workflowIds.includes(id)) + ); + }; + updateStates();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/lib/yjs/useYWorkflow.ts(1 hunks)
🔇 Additional comments (2)
ui/src/lib/yjs/useYWorkflow.ts (2)
150-183: Overall implementation looks good! 👍The changes successfully fix the multi-select deletion issue by:
- Correctly handling index shifting during multiple deletions
- Maintaining consistency between Yjs and React states
- Properly handling nested subworkflows
The suggested improvements above are minor and can be addressed in follow-up PRs.
150-163: 🛠️ Refactor suggestionAdd validation and error handling for workflow IDs.
The current implementation assumes every nodeId (except DEFAULT_ENTRY_GRAPH_ID) is a valid workflow ID. Consider adding validation to ensure the workflow exists before adding it to workflowIds.
nodeIds.forEach((nid) => { if (nid === DEFAULT_ENTRY_GRAPH_ID) return; - workflowIds.push(nid); const workflow = rawWorkflows.find((w) => w.id === nid); + if (!workflow) { + console.warn(`Workflow with ID ${nid} not found`); + return; + } + workflowIds.push(nid); const nodes = workflow?.nodes;Also, consider adding a depth limit to prevent potential infinite recursion in case of circular references between subworkflows.
ui/src/lib/yjs/useYWorkflow.ts
Outdated
| // Loop over workflow and remove any subworkflow nodes | ||
| if (nodes && Array.isArray(nodes)) { |
There was a problem hiding this comment.
This change iterates on one level deep, and then stops. This is why removeNodes was using itself, to be able to reach ALL subworkflows, no matter how deep.
If you can console.log rawWorkflows and test to make sure when deleting subworkflows that contain subworkflows, that all appropriate workflows are removed on subworkflow node deletion 🙏🏻
…h/reearth-flow into ui/multi-delete-sub-work-flow-bug
ui/src/lib/yjs/useYWorkflow.ts
Outdated
| @@ -173,6 +174,7 @@ export default ({ | |||
| setOpenWorkflowIds((ids) => | |||
| ids.filter((id) => !workflowIds.includes(id)), | |||
| ); | |||
| console.log("workflowIds", rawWorkflows); | |||
…delete-sub-work-flow-bug
…h/reearth-flow into ui/multi-delete-sub-work-flow-bug
Overview
Originally subwork flows could only be deleted one by one and multi selecting subwork flows for deletion would not work (error: “Exceeds maximum length”)
What I've done
What I haven't done
How I tested
Manually
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit