fix(apollo-react): disable task three-dot menu while loading [MST-8171]#471
fix(apollo-react): disable task three-dot menu while loading [MST-8171]#471KodudulaAshishUiPath wants to merge 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dependency License Review
License distribution
Excluded packages
|
There was a problem hiding this comment.
Pull request overview
This PR updates the StageNode task-loading model to support per-task loading state and uses it to suppress task menus/actions while async operations are in progress.
Changes:
- Introduces
loadingTaskIdsonStageNodeand wires it through to task renderers. - Disables the task “three-dot” menu button and suppresses right-click context menus for loading tasks (including adhoc tasks).
- Updates StageNode/DraggableTask tests and stories to reflect the new loading-state behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/apollo-react/src/canvas/components/StageNode/TaskMenu.tsx | Adds disabled prop and blocks menu open via click/contextmenu when disabled. |
| packages/apollo-react/src/canvas/components/StageNode/StageNode.types.ts | Replaces addTaskLoading with loadingTaskIds in public props. |
| packages/apollo-react/src/canvas/components/StageNode/StageNode.tsx | Computes loading state and passes per-task loading flag down to task components; uses loading to disable add-task UX/toolbox. |
| packages/apollo-react/src/canvas/components/StageNode/StageNode.test.tsx | Updates loading-related tests to use loadingTaskIds. |
| packages/apollo-react/src/canvas/components/StageNode/StageNode.stories.tsx | Updates “AddTaskLoading” story and adds a per-task loading demo stage. |
| packages/apollo-react/src/canvas/components/StageNode/DraggableTask.types.ts | Adds isTaskLoading prop. |
| packages/apollo-react/src/canvas/components/StageNode/DraggableTask.tsx | Disables menu + suppresses right-click context menu when loading. |
| packages/apollo-react/src/canvas/components/StageNode/DraggableTask.test.tsx | Adds tests around loading-state menu behavior. |
| packages/apollo-react/src/canvas/components/StageNode/AdhocTask.tsx | Applies the same loading-state menu suppression to adhoc tasks. |
| headerChips?: StageHeaderChip[]; | ||
| }; | ||
| addTaskLabel?: string; | ||
| addTaskLoading?: boolean; | ||
| replaceTaskLabel?: string; | ||
| taskOptions?: ListItem[]; | ||
| execution?: { |
There was a problem hiding this comment.
StageNodeBaseProps removes the previously public addTaskLoading prop, which is a breaking API change for external consumers. Consider keeping addTaskLoading?: boolean as a deprecated alias (mapping it to loadingTaskIds/isAnyTaskLoading) for at least one release, or document/major-bump appropriately.
| onReplaceTaskFromToolbox?: (newTask: ListItem, groupIndex: number, taskIndex: number) => void; | ||
| onTaskPlay?: (taskId: string) => Promise<void>; | ||
| hideParallelOptions?: boolean; | ||
| loadingTaskIds?: Set<string>; |
There was a problem hiding this comment.
loadingTaskIds is typed as a mutable Set<string>. In React props, mutating a Set in-place can change .size/.has() results without triggering a re-render, leading to stale UI. Prefer ReadonlySet<string> (and treat it immutably by replacing the instance when it changes).
| loadingTaskIds?: Set<string>; | |
| loadingTaskIds?: ReadonlySet<string>; |
| className="task-menu-icon-button" | ||
| sx={{ | ||
| color: 'var(--colorIconDefault) !important', | ||
| color: disabled ? 'var(--colorIconDisabled) !important' : 'var(--colorIconDefault) !important', |
There was a problem hiding this comment.
The disabled color uses var(--colorIconDisabled), but this CSS variable doesn’t appear to be defined in the repo (and the existing canvas variables use kebab-case like --color-icon-default). This can result in an incorrect icon color; consider relying on the built-in disabled styling for ApIconButton or switching to an existing, defined CSS variable.
| color: disabled ? 'var(--colorIconDisabled) !important' : 'var(--colorIconDefault) !important', |
| it('does not attach onContextMenu when isTaskLoading is true', () => { | ||
| const onRemove = vi.fn(); | ||
| const menuItems = createMenuItems(onRemove); | ||
|
|
||
| render( | ||
| <DraggableTask | ||
| {...defaultProps} | ||
| getContextMenuItems={() => menuItems} | ||
| isTaskLoading={true} | ||
| /> | ||
| ); | ||
|
|
||
| const task = screen.getByTestId('stage-task-task-1'); | ||
| // onContextMenu should not be set — right-click should not open menu | ||
| expect(task.oncontextmenu).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
This test is not asserting what it intends: React event handlers are attached via event delegation, so element.oncontextmenu will typically remain null even when an onContextMenu prop is present. To verify right-click is suppressed when loading, fire a contextMenu event and assert the menu does not open (or that the handler is not invoked via a spy).
| <StageTask | ||
| ref={taskRef} | ||
| data-testid={`stage-task-${task.id}`} | ||
| selected={isSelected} | ||
| status={taskExecution?.status} | ||
| onClick={handleClick} | ||
| {...(getContextMenuItems && { onContextMenu: handleContextMenu })} | ||
| {...(getContextMenuItems && !isTaskLoading && { onContextMenu: handleContextMenu })} | ||
| > | ||
| <TaskContent task={task} taskExecution={taskExecution} /> | ||
| {onTaskPlay && <AdhocTaskPlayButton taskId={task.id} onTaskPlay={onTaskPlay} />} | ||
| {getContextMenuItems && ( | ||
| <TaskMenu | ||
| ref={menuRef} | ||
| taskId={task.id} | ||
| getContextMenuItems={getContextMenuItems} | ||
| onMenuOpenChange={handleMenuOpenChange} | ||
| taskRef={taskRef} | ||
| disabled={isTaskLoading} | ||
| /> |
There was a problem hiding this comment.
Loading-state behavior was added for adhoc tasks (isTaskLoading disables the 3-dot menu and suppresses right-click), but there are no corresponding assertions in the existing AdhocTaskItem tests. Please add unit tests covering disabled menu button and context-menu suppression for adhoc tasks.
| it('should pass loadingTaskIds to Toolbox when toolbox is open', async () => { | ||
| const user = userEvent.setup(); | ||
| const onAddTaskFromToolbox = vi.fn(); | ||
| const { rerender } = renderStageNode({ onAddTaskFromToolbox, addTaskLoading: false }); | ||
| const { rerender } = renderStageNode({ onAddTaskFromToolbox, loadingTaskIds: new Set() }); | ||
|
|
There was a problem hiding this comment.
Test name is misleading: the component doesn’t pass loadingTaskIds to Toolbox; it derives a boolean loading from it. Consider renaming this test to reflect the actual behavior being asserted (e.g., Toolbox loading becomes true when loadingTaskIds is non-empty).
86f9cde to
f81530c
Compare
Summary
loadingTaskIdsprop toStageNodeto track which tasks are currently loadingaddTaskLoadingwithloadingTaskIdsfor per-task granularityDemo
Screen.Recording.2026-04-08.at.15.04.57.mov
Test plan
AddTaskLoadingstory, third stage)DraggableTask.test.tsx— 4 new tests for loading state behaviorStageNode.test.tsx— updated tests useloadingTaskIdsinstead ofaddTaskLoading