fix(ui): cut to copy paste and prevent readers copy/cut #1028
Conversation
WalkthroughThis pull request integrates a new cut operation into the editor’s copy-paste functionality. The changes add a Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant E as Editor Component
participant H as useCanvasCopyPaste Hook
participant C as useCopyPaste Hook
participant S as State Manager
U->>E: Press Command+X
E->>H: Trigger handleCut
H->>C: Invoke cut() with clipboard data
C->>S: Update state (remove nodes/edges)
S-->>C: Confirm update
C-->>H: Return updated state
H-->>E: Refresh canvas view
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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 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 (5)
ui/src/lib/i18n/locales/zh.json (1)
140-140: Missing Chinese translation for Cut featureThe translation key has been added, but the value is empty. Consider providing a Chinese translation for "Cut the Selected Nodes" to maintain a consistent multilingual experience.
- "Cut the Selected Nodes": "", + "Cut the Selected Nodes": "剪切选定节点",ui/src/lib/i18n/locales/fr.json (1)
140-140: Missing French translation for Cut featureThe translation key has been added, but the value is empty. Consider providing a French translation for "Cut the Selected Nodes" to ensure a consistent user experience across languages.
- "Cut the Selected Nodes": "", + "Cut the Selected Nodes": "Couper les nœuds sélectionnés",ui/src/lib/i18n/locales/es.json (1)
140-140: Missing Spanish translation for the cut action.The Spanish translation for "Cut the Selected Nodes" is currently empty. Consider adding an appropriate Spanish translation to maintain consistency across all language files.
- "Cut the Selected Nodes": "", + "Cut the Selected Nodes": "Cortar los nodos seleccionados",ui/src/features/Editor/useCanvasCopyPaste.ts (2)
39-40: Consider adding a comment explaining the reader restrictionThe check preventing users from copying reader nodes is implemented correctly, but it would be helpful to add a comment explaining why reader nodes cannot be copied.
const selectedNodes = nodes.filter((n) => selected.nodeIds.includes(n.id)); +// Reader nodes cannot be copied to prevent duplicating input sources if (selectedNodes.some((n) => n.type === "reader")) return;
184-188: Consider documenting the purpose of this cut operation after pastingThe cut operation after pasting seems to be preparing the clipboard for subsequent paste operations with the newly created nodes. Adding a comment would make this intention clearer.
+// After pasting, update the clipboard with the newly created nodes and edges +// so subsequent paste operations will use these items (with proper offsets) cut({ nodeIds: newNodes.map((n) => n.id), edges: newEdges, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ui/src/features/Editor/hooks.ts(2 hunks)ui/src/features/Editor/useCanvasCopyPaste.ts(6 hunks)ui/src/features/KeyboardShortcutDialog/hooks.ts(1 hunks)ui/src/hooks/useCopyPaste.ts(2 hunks)ui/src/lib/i18n/locales/en.json(1 hunks)ui/src/lib/i18n/locales/es.json(1 hunks)ui/src/lib/i18n/locales/fr.json(1 hunks)ui/src/lib/i18n/locales/ja.json(1 hunks)ui/src/lib/i18n/locales/zh.json(1 hunks)ui/src/types/shortcuts.ts(3 hunks)
🔇 Additional comments (19)
ui/src/lib/i18n/locales/en.json (1)
140-140: Added localization entry for new Cut featureThe new localization entry for "Cut the Selected Nodes" has been successfully added, which will support the cut functionality in the user interface.
ui/src/features/KeyboardShortcutDialog/hooks.ts (2)
72-75: Added keyboard shortcut entry for Cut functionalityThe addition of the cut operation to the keyboard shortcuts dialog is well-implemented. This ensures that users are aware of the new functionality and how to access it.
1-109:✅ Verification successful
Request for verification of keyboard shortcut functionality
While the keyboard shortcut entry has been added correctly, it would be beneficial to verify that the underlying implementation for the cut operation is working properly, especially considering the PR title mentions preventing readers from copy/cut operations.
🏁 Script executed:
#!/bin/bash # Check if the cut functionality is properly implemented in the hooks and if there's any reader-specific logic # Look for the cut implementation in hooks echo "Searching for cut implementation in hooks..." rg -A 10 "handleCut" --type ts # Check if there's any reader-specific logic for preventing cut operations echo "Checking for reader-specific restrictions on cut operations..." rg -A 5 "reader.*cut|cut.*reader" --type tsLength of output: 2768
Verification complete: Keyboard shortcut and cut-operation functionality confirmed
The keyboard shortcut entry in the dialog is wired correctly, and our review of the code confirms that the underlying cut logic is implemented as expected. In particular:
- The cut functionality in
ui/src/features/Editor/useCanvasCopyPaste.tsincludes a check for reader nodes (if (selected.nodes.some((n) => n.type === "reader")) return;), ensuring that cut operations are prevented for readers.- The implementation reference via
handleCutinui/src/features/Editor/hooks.tsfurther validates that the callback respects the reader-related restriction.Based on this verification, the current implementation aligns with the PR title's intention to prevent copy/cut operations for readers. No further changes are required.
ui/src/lib/i18n/locales/ja.json (1)
140-140: Japanese translation for cut action looks good.The Japanese translation for the cut action has been properly added.
ui/src/hooks/useCopyPaste.ts (2)
31-31: Export of the newcutfunction is properly included.The
cutfunction is correctly exported in the return object of the hook.
16-21:❓ Verification inconclusive
Implementation of
cutis identical tocopy.The current implementation of
cutis identical to thecopyfunction. In typical cut operations, the original data should be removed after copying. However, I understand this might be handled by thehandleCutmethod in the Editor hooks.Just ensure that the elements are properly removed from the canvas after being copied to the clipboard in the corresponding Editor hook implementation.
🏁 Script executed:
#!/bin/bash # Search for `handleCut` implementation to verify it properly removes nodes/edges after copying rg -A 10 "handleCut" --type tsLength of output: 2488
Please verify that the cut operation properly removes canvas elements.
The current implementation in
ui/src/hooks/useCopyPaste.tshascutdefined similarly tocopy, relying on thehandleCutfunction in the Editor hooks. I checked thehandleCutimplementation inui/src/features/Editor/useCanvasCopyPaste.ts—it gathers the selected nodes and edges and performs early returns based on selection criteria. However, the snippet doesn’t show the subsequent logic that actually removes the selected elements from the canvas.
- Confirm that after copying to the clipboard, the
handleCutfunction fully removes the selected nodes/edges from the canvas.- If the removal isn’t implemented there, consider adding the proper deletion logic or adjusting the integration between these hooks.
ui/src/types/shortcuts.ts (3)
23-23: Keyboard shortcut for cut operation added correctly.The "x" key has been added to the
CanvasKeystype with a proper comment explaining its usage with the Command key.
38-38: Cut action added to possible actions.The "cut" action has been properly added to the
PossibleActionstype.
92-92: Keyboard binding for cut operation configured correctly.The keyboard binding for the cut operation has been properly configured, binding it to the "x" key with the command key enabled, which follows standard conventions (Command+X for cut).
ui/src/features/Editor/hooks.ts (3)
161-161: LGTM - Clean destructuring of the handleCut functionThe addition of
handleCutto the destructuring assignment properly integrates the new cut functionality with the existing copy-paste mechanism.
169-169: LGTM - handleEdgesChange integrationPassing
handleYEdgesChangeas thehandleEdgesChangeparameter is necessary for the cut operation to properly remove edges from the canvas.
247-250: LGTM - Standard keyboard shortcut implementationThe Command+X (Ctrl+X on Windows) shortcut binding for cut follows platform conventions and is consistently implemented alongside the existing copy (Command+C) and paste (Command+V) shortcuts.
ui/src/features/Editor/useCanvasCopyPaste.ts (7)
1-1: LGTM - Required EdgeChange import addedThe addition of EdgeChange import is necessary for the type definition in the handleEdgesChange parameter.
16-17: LGTM - handleEdgesChange parameter correctly integratedThe handleEdgesChange parameter has been properly added to both the parameter object and its type definition, enabling the cut operation to remove edges.
Also applies to: 29-30
31-31: LGTM - Updated hook destructuringThe destructuring now includes the
cutmethod from useCopyPaste, correctly setting up the foundation for the cut functionality.
46-63: LGTM - Well-implemented cut functionalityThe handleCut function is well-implemented with appropriate checks and operations:
- It correctly filters selected nodes and edges
- It prevents cutting reader nodes (consistent with copy behavior)
- It properly removes the cut elements from the canvas using handleNodesChange and handleEdgesChange
- The dependency array includes all necessary dependencies
66-70: LGTM - Paste now handles both copied and cut contentThe paste function has been updated to correctly handle both copied and cut content by differentiating between them in the returned data structure and using the appropriate nodes for pasting.
Also applies to: 75-76
205-205: LGTM - Updated dependencies arrayThe 'cut' function has been correctly added to the dependencies array of the useCallback hook for handlePaste.
215-215: LGTM - Export of new functionalityThe handleCut function is correctly exposed in the return object, making it available for use in the editor.
…nto ui/cut-copy-paste
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ui/src/features/Editor/useCanvasCopyPaste.ts (1)
33-71: Well-implemented getRelatedNodes helper functionThis function thoroughly handles the complexity of finding related nodes, including batch children and subworkflow nodes. The implementation is robust with proper tracking of processed IDs to avoid infinite recursion.
Consider adding a brief JSDoc comment explaining the purpose and return value of this helper function for better maintainability.
+/** + * Recursively finds all related nodes to the selected nodes, including batch children + * and subworkflow nodes. Returns both the related nodes and any subworkflow data. + */ const getRelatedNodes = useCallback( (selectedNodeIds: string[]) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/features/Editor/useCanvasCopyPaste.ts(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ci-ui / ci
- GitHub Check: Redirect rules - reearth-flow
- GitHub Check: Header rules - reearth-flow
- GitHub Check: Pages changed - reearth-flow
🔇 Additional comments (8)
ui/src/features/Editor/useCanvasCopyPaste.ts (8)
16-16: LGTM: Added handleEdgesChange parameter to support cut operationsThe addition of the
handleEdgesChangeparameter is necessary to support removing edges when nodes are cut, keeping the interface consistent with other change handlers.Also applies to: 29-29
31-31: LGTM: Updated useCopyPaste import to include cut functionalityCorrectly destructured the
cutfunction from theuseCopyPastehook to support the new cut operation.
79-80: LGTM: Prevention of copying reader nodesGood implementation of the requirement to prevent readers from being copied, which aligns with the PR objective.
86-124: Well-structured handleCut implementationThe
handleCutfunction follows a clear pattern:
- Identify selected nodes and edges
- Check for reader nodes and prevent cutting if found
- Get all related nodes using the helper function
- Execute the cut operation
- Remove the cut nodes and edges from the canvas
The dependency array is comprehensive, including all necessary references.
127-138: LGTM: Enhanced paste handling for cut nodesThe paste function now correctly handles both copied and cut nodes by checking for
pastedCutNodesfirst and falling back tocopiedPastedNodesif not available.
191-212: Clear distinction between cut and copy operations for subworkflowsThe code now properly handles both cut and copy operations for subworkflows with clear conditionals. The implementation maintains the same pattern for creating new nodes and edges while correctly handling the subworkflow reference.
300-300: LGTM: Added handleCut to the returned objectCorrectly exposed the new
handleCutfunction in the returned object, making it available to consumers of this hook.
269-273:❓ Verification inconclusive
Verify the need for cutting after copying during paste
The added
cutoperation after thecopyoperation in the paste handler is interesting. It appears to be maintaining the clipboard state for subsequent paste operations, but confirm if this is the intended behavior.When a user pastes content and then immediately pastes again, will they expect the same content (copy behavior) or nothing (cut behavior)?
🏁 Script executed:
#!/bin/bash # Look for any other instances where copy and cut are used together rg --type ts "copy.*cut|cut.*copy" --glob "!ui/src/features/Editor/useCanvasCopyPaste.ts"Length of output: 91
Subject: Confirm Clipboard Behavior After Paste Operation
The current paste handler in
ui/src/features/Editor/useCanvasCopyPaste.ts(lines 269-273) callscutimmediately aftercopy. This sequence results in the pasted content being removed from the clipboard right after the paste operation. As there are no other instances of a combined copy‑then‑cut pattern in the codebase, please verify whether this behavior is intended.
- Should users be allowed to paste the same content multiple times (copy behavior), or is it correct that the content is cleared after a single paste (cut behavior)?
- Confirm that maintaining clipboard state for subsequent paste operations is as designed, rather than inadvertently preventing further pasting.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/src/features/Editor/useCanvasCopyPaste.ts (1)
136-137: Extract reader node check to reduce code duplicationThe check for reader nodes is duplicated in both
handleCopyandhandleCut. Consider extracting this to a shared function to improve maintainability.+ const canPerformOperation = useCallback((selectedNodeIds: string[]) => { + const selectedNodes = nodes.filter((n) => selectedNodeIds.includes(n.id)); + return !selectedNodes.some((n) => n.type === "reader"); + }, [nodes]); + const handleCopy = useCallback(async () => { const selected: { nodeIds: string[]; edges: Edge[] } | undefined = { nodeIds: nodes.filter((n) => n.selected).map((n) => n.id), edges: edges.filter((e) => e.selected), }; - const selectedNodes = nodes.filter((n) => selected.nodeIds.includes(n.id)); - if (selectedNodes.some((n) => n.type === "reader")) return; + if (!canPerformOperation(selected.nodeIds)) return;Apply the same change to
handleCut.Also applies to: 154-155
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/src/features/Editor/useCanvasCopyPaste.ts(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - reearth-flow
- GitHub Check: Header rules - reearth-flow
- GitHub Check: Pages changed - reearth-flow
🔇 Additional comments (5)
ui/src/features/Editor/useCanvasCopyPaste.ts (5)
202-205: Review thecutoperation inhandlePastefunctionThe
cutcall during paste has a different structure than your other cut operations. It usesnodeIdsinstead ofnodesand references the fulledgesarray rather than just the pasted edges.Is this intentional behavior? If so, please add a comment explaining why. If not, consider adjusting to be consistent with the structure used in
handleCut:- cut({ - nodeIds: pastedNodes.map((n: Node) => n.id), - edges: edges, - }); + cut({ + nodes: pastedNodes, + edges: pastedEdges, + });
169-177: Comprehensive node and edge removal inhandleCutThe implementation correctly identifies nodes to remove and also removes edges connected to those nodes. This approach ensures there are no dangling edges after the cut operation.
33-128: Good refactoring of node and edge creation logicExtracting
newEdgeCreationandnewNodeCreationas reusable callbacks improves code organization and maintainability. This allows the same logic to be used in both copy and cut operations.
130-146: Appropriate reader node protection in copy operationAdding the protection against copying reader nodes (line 137) aligns with the PR objective of "preventing readers copy/cut".
230-234: Successfully integrated cut operation with existing functionsThe hook now properly exports the new
handleCutfunction alongside the existing copy and paste functions, making it available to consumers.
| const selectedNodes = nodes.filter((n) => selected?.nodeIds.includes(n.id)); | ||
| if (selectedNodes.some((n) => n.type === "reader")) return; |
There was a problem hiding this comment.
Fix inconsistent null-safety handling in selected access
In handleCut, you're using optional chaining with selected?.nodeIds on line 154, but then you access selected.nodeIds directly on line 157 without optional chaining. This inconsistency could lead to runtime errors.
- const selectedNodes = nodes.filter((n) => selected?.nodeIds.includes(n.id));
+ const selectedNodes = nodes.filter((n) => selected.nodeIds.includes(n.id));Since you're creating the selected object on lines 149-152, it's never undefined, so the optional chaining operator is unnecessary.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const selectedNodes = nodes.filter((n) => selected?.nodeIds.includes(n.id)); | |
| if (selectedNodes.some((n) => n.type === "reader")) return; | |
| const selectedNodes = nodes.filter((n) => selected.nodeIds.includes(n.id)); | |
| if (selectedNodes.some((n) => n.type === "reader")) return; |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
ui/src/features/Editor/useCanvasCopyPaste.ts (1)
188-219: 🛠️ Refactor suggestionClarify the logic behind reusing “cut” in paste flow.
Lines 202-205 call
cutwith all existing edges whenever pasting. Though it updates the clipboard, the name “cut” implies removal from the graph, yet here it’s used to set new clipboard content. This can be confusing. Consider renaming or introducing a dedicated function (likeupdateClipboardSelection) to reflect the intention of updating the clipboard without actually removing nodes/edges from the canvas.
🧹 Nitpick comments (2)
ui/src/features/Editor/useCanvasCopyPaste.ts (2)
33-58: Check for missed edge creation edge cases.The
newEdgeCreationfunction correctly generates new edges by mapping old node IDs to newly created node IDs. However, consider:
- Error feedback: If an edge is skipped due to missing source/target (line 42), logs or warnings might help identify misconfigurations.
- Performance: The repeated array reassignment for each loop iteration is acceptable here, but be mindful of potential performance overhead if node counts increase dramatically.
130-146: Provide user feedback when preventing reader copy.The check
if (selectedNodes.some((n) => n.type === "reader")) return;silently stops copying if any “reader” type node is selected. Consider displaying a user-facing error or warning to signal that copying reader nodes is disallowed, avoiding silent failures that might confuse users.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ui/src/features/Editor/hooks.ts(2 hunks)ui/src/features/Editor/useCanvasCopyPaste.ts(7 hunks)ui/src/lib/i18n/locales/en.json(1 hunks)ui/src/lib/i18n/locales/es.json(1 hunks)ui/src/lib/i18n/locales/fr.json(1 hunks)ui/src/lib/i18n/locales/ja.json(1 hunks)ui/src/lib/i18n/locales/zh.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- ui/src/lib/i18n/locales/es.json
- ui/src/lib/i18n/locales/en.json
- ui/src/lib/i18n/locales/fr.json
- ui/src/lib/i18n/locales/zh.json
- ui/src/lib/i18n/locales/ja.json
- ui/src/features/Editor/hooks.ts
🧰 Additional context used
🧬 Code Definitions (1)
ui/src/features/Editor/useCanvasCopyPaste.ts (4)
ui/src/hooks/useCopyPaste.ts (1) (1)
useCopyPaste(6-34)ui/src/types/node.ts (1) (1)
Node(52-52)ui/src/lib/gql/__gen__/plugins/graphql-request.ts (1) (1)
Node(492-494)ui/src/lib/gql/__gen__/graphql.ts (1) (1)
Node(491-493)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Collect information
🔇 Additional comments (3)
ui/src/features/Editor/useCanvasCopyPaste.ts (3)
16-29: Ensure consistent edge-handling function signature.Declaring
handleEdgesChange: (changes: EdgeChange[]) => void;alongside the other handlers is a reasonable approach. This signature matches the typical React Flow pattern for handling edge changes, so there is no immediate concern regarding correctness.
148-178: Remove inconsistent optional chaining usage.You are using optional chaining in line 154 (
selected?.nodeIds) but directly accessingselected.nodeIdsin line 157. Previous reviews flagged this inconsistency, so unify your approach. Ifselectedis guaranteed to exist, remove?.. Otherwise, add null checks consistently.
59-128:❓ Verification inconclusive
Verify recursive subworkflow creation.
The
newNodeCreationfunction properly recreates subworkflow nodes and edges by recursively calling itself for subworkflow nodes. However, if there are cyclical references (e.g. a subworkflow referencing itself), this recursion could become infinite.Please run a quick search in the codebase to confirm whether subworkflows can point to themselves:
If no cyclical references exist, or if such references are prevented at a higher level, the current approach is safe.
🏁 Script executed:
#!/bin/bash # Description: Verify if subworkflow references can be cyclical. rg --context 5 "subworkflowId\s*=\s*.+parentWorkflowId"Length of output: 57
Attention: Please Manually Confirm Self-referencing Safeguards in Subworkflow Handling
The current implementation of
newNodeCreationleverages recursion for handling subworkflow nodes. Our automated search for patterns indicating a potential cyclical (self-referencing) subworkflow did not produce any output—but that result is inconclusive. This means that while no issues were immediately flagged, we cannot definitively confirm that cyclical subworkflow references (which could lead to infinite recursion) are prevented at a higher level.
- Action: Manually verify that subworkflow nodes cannot reference themselves (or create cyclical loops) via their
subworkflowId. Ensure that any higher-level validations or constraints are effectively preventing self-references.- Note: If the codebase already includes a safeguard against such cyclic references, the current recursion approach is safe. Otherwise, consider implementing additional checks to avoid infinite recursion.
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
Localization