Copy functionality for nodes and actions #195
Conversation
📝 WalkthroughWalkthroughThe changes introduce a node copy feature to the flow editor. The implementation adds copy button UI components to the TitleBar, wires copy handlers in the Action and Node components, and introduces a copyNode thunk that deep-copies nodes with UUID remapping for all sub-structures while managing store state and positioning. Changes
Sequence DiagramsequenceDiagram
actor User
participant Component as Action/Node<br/>Component
participant TitleBar
participant Store as Store/<br/>Thunk
participant NodeMap as Node Map
User->>TitleBar: Clicks copy button
TitleBar->>Component: onCopy callback triggered
Component->>Store: Dispatches copyNode(renderNode)
Store->>Store: Generates new UUIDs for:<br/>- Node<br/>- Actions<br/>- Exits<br/>- Router components
Store->>Store: Remaps UUID references<br/>Nullifies destinations<br/>Offsets position
Store->>NodeMap: Merges copied node<br/>into node map
Store->>Store: Marks state as dirty
Store-->>Component: Returns updated RenderNodeMap
Component-->>User: Display shows copied node
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/store/thunks.ts:
- Around line 994-1024: The code currently casts clonedNode.node.router to
SwitchRouter unconditionally which is imprecise; change the typing to the base
Router (or use a type guard) and only treat it as a SwitchRouter when confirmed:
replace the unconditional cast of clonedNode.node.router as SwitchRouter with
either casting to Router or, better, guard with if (router.type ===
RouterTypes.switch) before accessing SwitchRouter-specific fields (router.cases,
router.default_category_uuid), while keeping the existing optional checks on
router.categories to preserve runtime safety.
🧹 Nitpick comments (2)
src/components/titlebar/TitleBar.module.scss (1)
40-50: Minor inconsistency in hiding approach between copy and remove buttons.The
.copy_buttonusesopacity: 0/opacity: 1for hide/show, while.remove_buttonusesvisibility: hidden/visibility: visible. While both work,visibility: hiddenprevents mouse events on the hidden element, whereasopacity: 0still allows clicks on the invisible button. Consider usingvisibilityfor consistency with the existing pattern, or addpointer-events: nonewhen opacity is 0.♻️ Optional: Use visibility for consistency
.copy_button { padding-right: 5px; width: 20px; color: rgba(255, 255, 255, 0.8); display: flex; align-items: center; justify-content: center; cursor: pointer; - opacity: 0; - transition: opacity 0.2s; + visibility: hidden; }&:hover { .copy_button { - opacity: 1; + visibility: visible; }Also applies to: 70-72
src/components/flow/actions/action/Action.tsx (1)
104-113: Redundant guard in handleCopy.The
if (this.props.first)check on line 110 is redundant becauseshowCopy(line 213) already ensures the copy button is only rendered whenthis.props.firstis true. The handler will only be called when the button is visible.That said, keeping it as a defensive check is acceptable if you prefer explicit safety.
♻️ Optional: Remove redundant guard
public handleCopy(event: React.MouseEvent<HTMLElement>): void { if (event) { event.preventDefault(); event.stopPropagation(); } - // Only show copy on the first action to copy the entire node - if (this.props.first) { - this.props.copyNode(this.props.renderNode); - } + this.props.copyNode(this.props.renderNode); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
src/components/flow/actions/action/__snapshots__/Action.test.ts.snapis excluded by!**/*.snapsrc/components/flow/actions/action/__snapshots__/Action.test.tsx.snapis excluded by!**/*.snapsrc/components/flow/node/__snapshots__/Node.test.tsx.snapis excluded by!**/*.snapsrc/components/titlebar/__snapshots__/TitleBar.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
src/components/flow/actions/action/Action.tsxsrc/components/flow/node/Node.tsxsrc/components/titlebar/TitleBar.module.scsssrc/components/titlebar/TitleBar.tsxsrc/store/thunks.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/store/thunks.ts (3)
src/store/flowContext.ts (4)
RenderNode(26-32)RenderNodeMap(18-20)nodes(244-251)updateNodes(176-181)src/utils/index.tsx (2)
createUUID(384-386)NODE_SPACING(26-26)src/flowTypes.ts (4)
Exit(192-195)SwitchRouter(241-245)Category(223-227)Case(214-221)
src/components/flow/node/Node.tsx (1)
src/store/thunks.ts (1)
CopyNode(101-101)
src/components/flow/actions/action/Action.tsx (1)
src/store/thunks.ts (1)
CopyNode(101-101)
src/components/titlebar/TitleBar.tsx (1)
src/utils/index.tsx (1)
createClickHandler(217-234)
🔇 Additional comments (13)
src/components/titlebar/TitleBar.tsx (3)
16-17: LGTM!The new props
onCopyandshowCopyfollow the same optional pattern as the existingonMoveUpandshowMoveprops.
184-184: LGTM!The copy element is properly integrated into the render flow, positioned between the title text and the remove button, which is a logical placement for the UI.
Also applies to: 193-193
111-129: Implementation follows established patterns.The
getCopy()method correctly mirrors the structure ofgetMoveArrow()andgetRemove(), usingcreateClickHandlerconsistently and providing a placeholder element when the copy button is not shown to maintain layout consistency. Thecopyicon exists in the temba-icon component library.src/components/flow/node/Node.tsx (4)
36-38: LGTM!The imports are correctly added following the existing import pattern from
store/thunks.
217-221: LGTM!The
handleCopymethod correctly prevents event propagation (matching the pattern inhandleRemoval) and invokes thecopyNodethunk with the current render node.
382-383: LGTM!The
showCopyandonCopyprops are correctly wired to the TitleBar, withshowCopyfollowing the same condition asshowRemoval(disabled during translation mode).
556-556: LGTM!The
copyNodethunk is correctly added to the dispatch bindings.src/components/flow/actions/action/Action.tsx (2)
213-214: LGTM!The copy button is correctly shown only on the first action and when not translating, ensuring users copy the entire node from a single, consistent location.
262-263: LGTM!The dispatch mapping and connect options are correctly updated.
Also applies to: 268-270
src/store/thunks.ts (4)
13-13: LGTM!The imports for
CaseandSwitchRouterare correctly added to support the router UUID remapping logic.Also applies to: 23-23
101-102: LGTM!The
CopyNodetype is correctly defined to match the thunk signature pattern used elsewhere in this file.
956-1039: Overall implementation is solid.The
copyNodethunk correctly:
- Deep-clones the node structure
- Remaps all internal UUIDs (node, actions, exits, router components)
- Maintains structural relationships (category→exit, case→category, default_category)
- Nullifies exit destinations (copied node is disconnected)
- Offsets position to avoid overlap
- Clears inbound connections and ghost flag
- Dispatches update and marks state dirty
The use of
JSON.parse(JSON.stringify())for deep cloning is acceptable here sinceRenderNodecontains only serializable data.
1026-1029: No action needed—operand references in ui.config correctly reference external assets.The deep clone creates a copy of the entire node structure without explicitly remapping
ui.config. Since operand references (e.g.,operand.id) point to external assets in the assetStore (fields, results) rather than internal node UUIDs, they should remain unchanged when cloning. This behavior is correct.
| if (clonedNode.node.router) { | ||
| const router = clonedNode.node.router as SwitchRouter; | ||
|
|
||
| if (router.categories) { | ||
| router.categories.forEach((category: Category) => { | ||
| const newCategoryUUID = createUUID(); | ||
| uuidMap[category.uuid] = newCategoryUUID; | ||
| category.uuid = newCategoryUUID; | ||
|
|
||
| if (uuidMap[category.exit_uuid]) { | ||
| category.exit_uuid = uuidMap[category.exit_uuid]; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| if (router.cases) { | ||
| router.cases.forEach((caseItem: Case) => { | ||
| const newCaseUUID = createUUID(); | ||
| uuidMap[caseItem.uuid] = newCaseUUID; | ||
| caseItem.uuid = newCaseUUID; | ||
|
|
||
| if (uuidMap[caseItem.category_uuid]) { | ||
| caseItem.category_uuid = uuidMap[caseItem.category_uuid]; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| if (router.default_category_uuid && uuidMap[router.default_category_uuid]) { | ||
| router.default_category_uuid = uuidMap[router.default_category_uuid]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for router interface definitions
rg -n "interface.*Router" --type ts | head -20
rg -n "type.*Router" --type ts | head -20Repository: glific/floweditor
Length of output: 3935
🏁 Script executed:
#!/bin/bash
# Look at the specific file and surrounding context
head -1030 src/store/thunks.ts | tail -100Repository: glific/floweditor
Length of output: 2730
🏁 Script executed:
#!/bin/bash
# Find where SwitchRouter and other routers are defined
fd . --type f --name "*.ts" | xargs rg -l "SwitchRouter|RandomRouter" | head -10Repository: glific/floweditor
Length of output: 647
🏁 Script executed:
#!/bin/bash
# Get the Router and SwitchRouter interface definitions
sed -n '202,300p' src/flowTypes.tsRepository: glific/floweditor
Length of output: 1661
🏁 Script executed:
#!/bin/bash
# Search for all router type definitions
rg -n "Router.*extends|Router.*{" src/flowTypes.ts -A 5 | head -60Repository: glific/floweditor
Length of output: 174
🏁 Script executed:
#!/bin/bash
# Check if RandomRouter is defined and what properties it has
rg -n "RandomRouter" src/flowTypes.ts -B 2 -A 10Repository: glific/floweditor
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Look for RandomRouter definition and check all router types
rg -n "interface RandomRouter|type RandomRouter" src/flowTypes.ts -A 10Repository: glific/floweditor
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check what router types exist in RouterTypes enum
rg -n "enum RouterTypes" src/flowTypes.ts -A 20Repository: glific/floweditor
Length of output: 475
🏁 Script executed:
#!/bin/bash
# Check how copyNode function is used and what router types can be passed
rg -n "copyNode" src/store/thunks.ts -B 5 -A 5Repository: glific/floweditor
Length of output: 390
The type casting approach is safe at runtime but should be reconsidered for type safety.
The codebase only has two router types: switch and random. Since RandomRouter has no specific interface definition, it inherits only the base Router interface which includes categories as a required property. The SwitchRouter-specific properties (cases, default_category_uuid) are safely guarded by optional checks before access, so the code won't crash on non-SwitchRouter types. However, casting to SwitchRouter unconditionally is imprecise—consider casting to the Router base type instead or using a type guard (if (router.type === RouterTypes.switch)) for better type safety and clarity.
🤖 Prompt for AI Agents
In @src/store/thunks.ts around lines 994 - 1024, The code currently casts
clonedNode.node.router to SwitchRouter unconditionally which is imprecise;
change the typing to the base Router (or use a type guard) and only treat it as
a SwitchRouter when confirmed: replace the unconditional cast of
clonedNode.node.router as SwitchRouter with either casting to Router or, better,
guard with if (router.type === RouterTypes.switch) before accessing
SwitchRouter-specific fields (router.cases, router.default_category_uuid), while
keeping the existing optional checks on router.categories to preserve runtime
safety.
Summary by CodeRabbit
Release Notes
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.