Add workflow node actions and side panel controls - #24411
Conversation
🔍 Visual Regression Review —
|
|
🚀 Preview Environment Ready! Your preview environment is available at: https://unlock-aspects-mill-characters.trycloudflare.com This environment will automatically shut down after 5 hours. |
🔍 Automated Pre-Review✅ No issues detected - This PR is ready for human review. Automated pre-review — human approval still required. |
Greptile SummaryThe PR adds contextual workflow-node actions, extracts reusable workflow option items, and enhances side-panel close and expand controls with labels and shortcuts.
Confidence Score: 4/5The PR appears safe to merge, with non-blocking repository-convention issues in the new button wrapper and Storybook interaction coverage. The workflow and side-panel behavior has no established blocking failure, while the accepted findings concern explicit prop forwarding and missing Storybook play functions. Files Needing Attention: packages/twenty-ui/src/input/IconButtonWithTooltip/IconButtonWithTooltip.tsx; packages/twenty-ui/src/input/IconButtonWithTooltip/stories/IconButtonWithTooltip.stories.tsx; packages/twenty-front/src/modules/workflow/workflow-diagram/components/stories/WorkflowDiagramStepNodeEditableContent.stories.tsx Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Node[Selected workflow node] --> Menu[Node options dropdown]
Menu --> Change[Change node type]
Menu --> Duplicate[Duplicate action node]
Menu --> Delete[Delete node]
Footer[Workflow step footer] --> Shared[Shared option menu items]
Menu --> Shared
Shared --> Change
Shared --> Duplicate
Shared --> Delete
Reviews (1): Last reviewed commit: "Merge branch 'main' into bonapara/workfl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| {/* oxlint-disable-next-line react/jsx-props-no-spreading */} | ||
| <IconButton {...iconButtonProps} /> |
There was a problem hiding this comment.
This wrapper collects every non-tooltip property into iconButtonProps and spreads it onto IconButton, exposing an ambiguous public API and allowing properties to be forwarded unintentionally. Explicitly pass only the button properties this wrapper supports.
Rule Used: Avoid spreading props for button components; only ... (source)
Learned From
twentyhq/twenty#12282
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — the wrapper now exposes and forwards only its supported IconButton props explicitly.
| export const Default: Story = { | ||
| args: { | ||
| Icon: IconSearch, | ||
| ariaLabel: 'Search', | ||
| variant: 'tertiary', | ||
| size: 'small', | ||
| tooltipContent: 'Search', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The new tooltip-button story has no play function, so Storybook does not exercise the newly introduced tooltip interaction. Add an interaction that validates the tooltip behavior; the changed workflow-node catalog story has the same omission.
Rule Used: Always add 'play' functions to stories in Storyboo... (source)
Learned From
twentyhq/twenty#12354
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Added play coverage for keyboard tooltip focus and the workflow node options interaction.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 996b6fb8a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {selected && ( | ||
| <WorkflowDiagramStepNodeOptionsDropdown |
There was a problem hiding this comment.
Keep the id !== EMPTY_NODE_ID guard when rendering this menu. The canvas maps the synthetic empty node to this component, and its click handler can put empty-node into workflowSelectedNodeComponentState; once selected, this menu exposes duplicate and delete actions that call the workflow mutations with the placeholder ID rather than a real step, resulting in failed operations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — the options menu is now guarded by id !== EMPTY_NODE_ID.
| <div data-tooltip-id={tooltipId}> | ||
| {/* oxlint-disable-next-line react/jsx-props-no-spreading */} | ||
| <IconButton {...iconButtonProps} /> |
There was a problem hiding this comment.
Attach the tooltip anchor to the actual button rather than this wrapper. AppTooltip registers a native focus listener on the matched anchor, but focus events do not bubble from the nested IconButton to this div, so keyboard users tabbing to the new close and expand controls never see their tooltips—including the expand shortcut hint—while mouse hover still works.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — the tooltip now anchors to the nested focusable button.
🔍 Visual Regression Review —
|
| Story | Verdict | Confidence | Explained by | |
|---|---|---|---|---|
| 🟡 | pages-settings-domains-settingssubdomain--valid-subdomain |
uncertain | 45% | IconButton.module.scss: tertiary default icon color changed from --t-font-color-… |
Changed stories
| Story | Diff % |
|---|---|
| pages-settings-domains-settingssubdomain--valid-subdomain | 7% |
| modules-aichat-aichatquestioncard--long-question | 0% |
| modules-aichat-aichatquestioncard--multiple-questions | 0% |
| modules-aichat-aichatquestioncard--multi-select-question | 0% |
| modules-aichat-aichatquestioncard--single-question | 0% |
| modules-settings-accounts-blocklist-settingsaccountsblocklisttablerow--default | 0% |
| modules-settings-accounts-blocklist-settingsaccountsblocklisttablerow--delete-first-element-from-blocklist | 0% |
| modules-settings-accounts-blocklist-settingsaccountsblocklisttable--default | 0% |
| modules-settings-accounts-blocklist-settingsaccountsblocklisttable--delete-first-element-from-blocklist | 0% |
| modules-workflow-workflowdiagramstepnodeeditablecontent--catalog | 0% |
| modules-sidepanel-sidepaneltopbar--subpage | 0% |
| modules-workflow-actions-filter-workflowstepfiltercolumn--default | 0% |
View run details · advisory mode
thomtrp
left a comment
thomtrp
left a comment
There was a problem hiding this comment.
Two on the new Escape hotkey.
| const handleSidePanelBackspace = useHandleSidePanelBackspace(); | ||
| const handleSidePanelEscape = useHandleSidePanelEscape(); | ||
|
|
||
| useGlobalHotkeys({ |
There was a problem hiding this comment.
This registers a hotkey straight inside a regular component, while the same PR extracts OptionsDropdownMenuHotkeyEffect to avoid exactly that. Pull it into an effect component too?
There was a problem hiding this comment.
Done — Escape registration now lives in SidePanelTopBarEscapeHotkeyEffect.
| const handleSidePanelBackspace = useHandleSidePanelBackspace(); | ||
| const handleSidePanelEscape = useHandleSidePanelEscape(); | ||
|
|
||
| useGlobalHotkeys({ |
There was a problem hiding this comment.
handleInputKeyDownCapture already calls handleSidePanelEscape on Escape, so the callback is now wired twice and they only avoid double-firing because the capture handler calls stopImmediatePropagation before the document listener runs. That's an undocumented dependency on listener phase — worth a comment, or gating the global one on the input not being focused?
There was a problem hiding this comment.
Fixed — the global handler now skips while the search input is focused; the capture handler owns that case.
✅ Standard review · no findings
High-level — Cohesive extraction/reuse change (shared menu-items, IconButtonWithTooltip) across side-panel and workflow with no data-model, migration, or rollout risk. Reviewed against the |
![twenty-ci-bot-public[bot]](https://avatars.githubusercontent.com/u/119600397?s=60&v=4){kind=small}
|
|
||
| import { SidePanelTopBarEscapeHotkeyEffect } from '@/side-panel/components/SidePanelTopBarEscapeHotkeyEffect'; | ||
|
|
||
| describe('SidePanelTopBarEscapeHotkeyEffect', () => { |
There was a problem hiding this comment.
🟡 Nit · Low-level · frontend tests — effect components
Dedicated .test.tsx on a render-null Effect component is a finding
SidePanelTopBarEscapeHotkeyEffect is a side-effect-only component that renders null, so a standalone test asserting mocked globals is exactly the case the standard says to fold into the host; the SidePanelTopBar test already covers the escape-while-input-not-focused path. Delete this file and cover the effect through SidePanelTopBar.
There was a problem hiding this comment.
Removed — host-level Escape coverage remains in SidePanelTopBar.test.tsx.
| import { fireEvent, render, screen, waitFor } from '@testing-library/react'; | ||
| import { Provider as JotaiProvider } from 'jotai'; | ||
|
|
||
| describe('WorkflowDiagramStepNodeOptionsDropdown', () => { |
There was a problem hiding this comment.
🟡 Nit · Low-level · frontend tests — components render a story
Component that renders a dropdown is covered by a testing-library render test rather than a Storybook story
The standard routes components that render something to a Storybook story with play/findBy, not a render test; the dropdown's behavior is already partly exercised by the WorkflowDiagramStepNodeEditableContent story. Move this coverage into a story play function.
There was a problem hiding this comment.
Moved the dropdown coverage into the workflow node catalog play function and removed the render test.
thomtrp
left a comment
thomtrp
left a comment
There was a problem hiding this comment.
Nice cleanup on the shared menu items. Two things to look at before merge (the failing ui-sb-test story and the Escape handling), rest is minor.
|
|
||
| const canvasBody = within(canvasElement.ownerDocument.body); | ||
|
|
||
| await expect(await canvasBody.findByRole('tooltip')).toHaveTextContent( |
There was a problem hiding this comment.
ui-sb-test is red on this: the Default play can't find role="tooltip" and the story renders the error overlay — did this pass locally with nx storybook:test twenty-ui?
| onEscape(); | ||
| }; | ||
|
|
||
| useGlobalHotkeys({ |
There was a problem hiding this comment.
RecordTableBodyEscapeHotkeyEffect (and the board/calendar ones) also listen for Escape on PageFocusId.RecordIndex and each side stops immediate propagation when it matches, so whichever registered first swallows the other — the table one even with no rows selected. On a record index with the panel open, which one should win, and is that deterministic today?
| import { WorkflowDiagramHandleSource } from '@/workflow/workflow-diagram/workflow-nodes/components/WorkflowDiagramHandleSource'; | ||
| import { WorkflowDiagramHandleTarget } from '@/workflow/workflow-diagram/workflow-nodes/components/WorkflowDiagramHandleTarget'; | ||
| import { WorkflowDiagramStepNodeIcon } from '@/workflow/workflow-diagram/workflow-nodes/components/WorkflowDiagramStepNodeIcon'; | ||
| import { WorkflowDiagramStepNodeOptionsDropdown } from '@/workflow/workflow-diagram/workflow-nodes/components/WorkflowDiagramStepNodeOptionsDropdown'; |
There was a problem hiding this comment.
This swap leaves WorkflowNodeDeleteButton with zero importers, delete the file too?
| } | ||
|
|
||
| const tooltipContent = expandTarget.hasExpandShortcut | ||
| ? `${expandTarget.label} | ${getOsControlSymbol()}⏎` |
There was a problem hiding this comment.
On non-mac this renders "Expand | Ctrl⏎" with Ctrl glued to ⏎ — want a separator like the hotkeys arrays elsewhere?
| onChangeNode={handleChangeNode} | ||
| onDuplicateNode={handleDuplicateNode} | ||
| onDeleteNode={handleDeleteNode} | ||
| deleteNodeAccent="danger" |
There was a problem hiding this comment.
Delete is danger here but default accent in the footer's menu — intentional split?
2 participants
Summary
After
blue button issue is a main problem, not related to this branch