Fix/feedback#215
Conversation
|
Warning Review limit reached
More reviews will be available in 55 minutes and 57 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR introduces a server-controlled 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/flow/actions/action/Action.module.scss (1)
31-67: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAvoid CSS selector duplication for
send_interactive_msg.The
.body.send_interactive_msgselector appears in two locations: the multi-selector block (line 54) that assigns a computedwidth, and again at lines 65-67 with a fixedwidth: 200pxthat overrides it. While this works due to CSS cascade, it's more maintainable to either remove&.send_interactive_msgfrom the multi-selector list or combine the rules to make the override intent clearer.♻️ Option A: Remove from multi-selector, consolidate into lines 65–67
&.add_contact_urn, &.add_input_labels, &.send_msg, &.set_contact_field, &.set_contact_name, &.call_webhook, &.call_resthook, &.call_llm, &.set_contact_language, &.set_contact_status, &.set_contact_channel, &.add_contact_groups, &.set_contact_profile, &.remove_contact_groups, &.play_audio, &.say_msg, &.set_run_result, &.send_email, &.send_broadcast, &.transfer_airtime, &.open_ticket, &.missing, &.wait_for_time, - &.send_interactive_msg, &.wait_for_result, &.enter_flow, &.link_google_sheet, &.request_optin, &.enter_flow, &.set_wa_group_field {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/flow/actions/action/Action.module.scss` around lines 31 - 67, The `.send_interactive_msg` selector is duplicated in the multi-selector block (appearing at line 54) and has a separate rule at lines 65-67 that overrides the width. Remove `&.send_interactive_msg` from the multi-selector list to eliminate the duplication, keeping only the dedicated rule at lines 65-67 that explicitly sets `width: 200px`. This makes the intent clearer and avoids relying on CSS cascade to override the computed width from the multi-selector block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/flowTypes.ts`:
- Line 56: The copyNodeEnabled field in the Endpoints type is currently typed as
string | boolean, but it is consumed as a request URL in src/store/thunks.ts
where it gets passed to axios.get(...). Remove the boolean union type and change
copyNodeEnabled to be typed as only string to match the actual endpoint
contract. If you need to support both a static toggle and a dynamic endpoint
URL, refactor this into two separate fields instead of combining them in a
single union type.
In `@src/store/editor.ts`:
- Line 86: The `copyNodeEnabled` field is defined as optional (with the `?`
operator), which prevents it from being properly cleared during state resets.
Since `mergeEditorState(initialState)` only merges keys that exist on
`initialState`, an old `copyNodeEnabled` value persists when fetching a new
flow, exposing copy functionality on unintended flows. Remove the optional
operator from `copyNodeEnabled` to make it a required boolean field, initialize
it explicitly to `false` in the initial state object, and ensure that when the
endpoint is absent or after fetching a flow completes, the field is explicitly
reset to `false` to guarantee the UI properly gates the copy feature.
In `@src/store/helpers.ts`:
- Around line 819-837: The remapLocalization function is keeping both the
original UUID keys and the newly remapped keys in the result because it clones
the entire langMap at the start with the spread operator in newLangMap
initialization. To fix this, instead of cloning the entire langMap, initialize
newLangMap as an empty object, then populate it only with entries that have been
remapped by checking if the oldUUID exists in the uuidMap and the langMap. This
ensures stale source-flow UUID entries are not carried into the destination
flow.
In `@src/store/thunks.ts`:
- Around line 1253-1266: The code in both copyNode (line 1253-1266) and
pasteNode (line 1319-1335) functions assumes definition.localization exists
without checking, causing crashes when it is undefined. Before calling
extractLocalizationForNode in copyNode and before calling any localization remap
functions in pasteNode, add a guard that normalizes definition.localization to
an empty map if it does not exist (e.g., definition.localization || {}),
ensuring the helper functions always receive a valid localization object rather
than undefined.
---
Outside diff comments:
In `@src/components/flow/actions/action/Action.module.scss`:
- Around line 31-67: The `.send_interactive_msg` selector is duplicated in the
multi-selector block (appearing at line 54) and has a separate rule at lines
65-67 that overrides the width. Remove `&.send_interactive_msg` from the
multi-selector list to eliminate the duplication, keeping only the dedicated
rule at lines 65-67 that explicitly sets `width: 200px`. This makes the intent
clearer and avoids relying on CSS cascade to override the computed width from
the multi-selector block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43a211f5-cdd0-4d29-ac6f-14c43f8a48e7
📒 Files selected for processing (11)
src/components/flow/actions/action/Action.module.scsssrc/components/flow/actions/action/Action.tsxsrc/components/flow/node/Node.tsxsrc/components/titlebar/TitleBar.module.scsssrc/components/titlebar/TitleBar.tsxsrc/components/toast/Toast.module.scsssrc/flowTypes.tssrc/store/editor.tssrc/store/helpers.tssrc/store/thunks.tssrc/test/config.ts
…g for copy/paste actions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/store/thunks.test.ts`:
- Around line 905-907: The `boring` fixture is a shared mutable object that is
reused without cloning at lines 905-907 and lines 1028-1030, causing tests to
become order-dependent when earlier tests mutate `boring`. Clone the `boring`
fixture before assigning it to `flowContext.definition` at both locations to
ensure each test gets a fresh copy of the fixture rather than a mutated version
from previous tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e8dd973-2d6a-42e2-80cd-f1b756775b60
⛔ Files ignored due to path filters (5)
src/components/__snapshots__/index.test.ts.snapis excluded by!**/*.snapsrc/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/store/__snapshots__/thunks.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
src/flowTypes.tssrc/store/editor.tssrc/store/helpers.tssrc/store/thunks.test.tssrc/store/thunks.ts
…wEditor configuration
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/store/thunks.ts (1)
1308-1333:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate clipboard payload shape after JSON parse before using it.
Line 1331 assumes
payload.primaryis valid. A parseable but malformed clipboard value will still throw during clone/remap and break paste UX instead of showing the invalid-clipboard toast.Proposed fix
let payload: ClipboardPayload; try { payload = JSON.parse(raw) as ClipboardPayload; } catch { @@ return; } + + if (!payload?.primary?.node || !payload?.primary?.ui) { + dispatch( + mergeEditorState({ + toast: { + message: 'Clipboard data is invalid. Please copy the node again.', + duration: 5000 + } + }) + ); + return; + } const { primary, paired, pairedOffset, sourceFlowUUID, sourceLocalization } = payload;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/thunks.ts` around lines 1308 - 1333, After the JSON parse succeeds in the try block, add validation to ensure the payload object contains all required properties (primary, paired, pairedOffset, sourceFlowUUID, sourceLocalization) before destructuring and using them. If the validation fails, dispatch the invalid-clipboard toast message and return early, just like the catch block does. This prevents malformed clipboard data from reaching the cloneNodeWithNewUUIDs function where it would throw an unhandled error and break the paste experience.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/index.tsx`:
- Around line 119-123: The componentDidMount method initializes tracking but
provides no session-end handling. Add complementary lifecycle and event handlers
to emit the node_copied_not_pasted event when a session terminates (e.g.,
component unmounts, page visibility changes, or window unloads) while a copy
operation is pending. Implement componentWillUnmount and visibility/unload
listeners that check for pending-copy state and emit the appropriate event to
onEvent to capture abandoned copy operations as required.
In `@src/store/thunks.ts`:
- Around line 1295-1298: There are two analytics event tracking calls that need
to be updated to align with the PR `#5208` contract. At line 1295-1298 in the
track call for flow_node_copied, change the event name to node_copied and add
the org identifier property to the payload. At line 1383-1387 in the track call
for flow_node_pasted, change the event name to node_pasted, add the org
identifier property to the payload, and include the paste conflict metadata
property as specified in the event contract. These changes ensure downstream
PostHog metrics match the expected schema and include all required properties.
In `@src/store/tracking.ts`:
- Around line 7-12: The track function invokes the _track callback without error
handling, which means any exceptions thrown by the callback will propagate to
calling code and break editor actions like copy/paste operations. Wrap the
_track(event, properties) call in the track function with a try-catch block to
catch and suppress any exceptions, ensuring tracking failures remain best-effort
and non-blocking to core editor functionality.
---
Outside diff comments:
In `@src/store/thunks.ts`:
- Around line 1308-1333: After the JSON parse succeeds in the try block, add
validation to ensure the payload object contains all required properties
(primary, paired, pairedOffset, sourceFlowUUID, sourceLocalization) before
destructuring and using them. If the validation fails, dispatch the
invalid-clipboard toast message and return early, just like the catch block
does. This prevents malformed clipboard data from reaching the
cloneNodeWithNewUUIDs function where it would throw an unhandled error and break
the paste experience.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68345700-195c-4c64-bcb8-34df541580ab
📒 Files selected for processing (4)
src/components/index.tsxsrc/flowTypes.tssrc/store/thunks.tssrc/store/tracking.ts
akanshaaa19
left a comment
akanshaaa19
left a comment
There was a problem hiding this comment.
this is still too close, see if you can move it to the left side of the node.
also change the toast message to cmd/ctrl + v - for windows users
otherwise ok
…ft, fix toast message Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/index.tsx (1)
119-123:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
node_copied_not_pastedsession-end emission is still missing.This mount-only wiring initializes tracking but still does not implement the required abandoned-copy session-end path (
node_copied_not_pasted) from Issue#5208. As per prior review, this remains unresolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/index.tsx` around lines 119 - 123, The componentDidMount method initializes tracking via initTracking(onEvent), but the implementation still lacks the abandoned-copy session-end path (node_copied_not_pasted) from Issue `#5208`. You need to implement state tracking to monitor when copyNodeEnabled transitions to true (indicating a copy operation) and when a paste event occurs, then emit the node_copied_not_pasted session-end event in the component's unmount lifecycle (componentWillUnmount) or appropriate cleanup phase if a copy was performed without a subsequent paste, using the onEvent callback from the config context to fire the event.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/index.tsx`:
- Line 123: The mergeEditorState call is hardcoding copyNodeEnabled to true,
which ignores the actual server/config value and breaks the feature-flag control
mechanism. Replace the hardcoded true value in the mergeEditorState call with
the actual copyNodeEnabled variable from your config or props, using the boolean
coercion suggested in the TODO comment (!!copyNodeEnabled) to ensure the proper
config value is respected.
In `@src/components/titlebar/TitleBar.module.scss`:
- Line 43: Verify that the padding-left: 5px implementation on the .copy_button
selector (line 43) produces the intended visual spacing effect. Since
.copy_button uses the default content-box box-sizing model with width: 20px, the
total element width becomes 25px. Confirm visually that this increases the flex
layout spacing between the copy and remove buttons as intended by the PR
objective, and ensure the icon within the copy button is not visually cramped or
obscured by the reduced content area. You can compare the spacing with the
similar styling applied to .up_button to ensure consistency across both buttons.
---
Duplicate comments:
In `@src/components/index.tsx`:
- Around line 119-123: The componentDidMount method initializes tracking via
initTracking(onEvent), but the implementation still lacks the abandoned-copy
session-end path (node_copied_not_pasted) from Issue `#5208`. You need to
implement state tracking to monitor when copyNodeEnabled transitions to true
(indicating a copy operation) and when a paste event occurs, then emit the
node_copied_not_pasted session-end event in the component's unmount lifecycle
(componentWillUnmount) or appropriate cleanup phase if a copy was performed
without a subsequent paste, using the onEvent callback from the config context
to fire the event.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 27b158de-9310-48ec-a627-f7f7a98d9ae1
📒 Files selected for processing (5)
src/components/index.tsxsrc/components/titlebar/TitleBar.module.scsssrc/flowTypes.tssrc/store/thunks.tssrc/test/config.ts
Fixed: 3a1ef4d |
2 participants
Closes: glific/glific#5229
Closes: glific/glific#5208
Summary by CodeRabbit
Summary by CodeRabbit
copyNodeEnabledconfiguration flag.onEvent.