{{ message }}
Repository navigation
show affected blocks in confirmation dialog when deleting blocks or parameters - #4519
Merged
celalzamanoglu merged 1 commit intoJan 22, 2026
Merged
Conversation
Contributor
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 3cd8b68 in 57 seconds. Click for details.
- Reviewed
1000lines of code in10files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern-frontend/src/routes/workflows/hooks/useRequestDeleteNodeCallback.ts:4
- Draft comment:
Good simple hook extract; the use of optional chaining with a proper error throw ensures the hook is only used within a provider. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. skyvern-frontend/src/store/BlockActionContext.ts:11
- Draft comment:
The BlockActionContext is well-typed and created without a default value, ensuring proper usage within a provider; naming and structure are clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_9ZfbPBfFug4t2UOM
You can customize 
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3cd8b68 in 59 seconds. Click for details.
- Reviewed
998lines of code in10files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern-frontend/src/store/BlockActionContext.ts:1
- Draft comment:
The context is defined and typed correctly. Using undefined as the default value is common practice in React and works well with the custom hook (which throws if not provided). - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. skyvern-frontend/src/routes/workflows/editor/Workspace.tsx:1494
- Draft comment:
Typo: In the comment on line 1494, consider capitalizing the acronym "ID" in "current debug session id" to "current debug session ID" to improve clarity and consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_GlMKVeBRnAbW1tPn
You can customize
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@skyvern-frontend/src/routes/workflows/editor/FlowRenderer.tsx`:
- Around line 320-331: The requestDeleteNode callback uses deleteNode but
doesn't include it in its dependency array; wrap the deleteNode implementation
in a stable useCallback (including its internal dependencies like nodes, edges,
etc.) and then add that memoized deleteNode to requestDeleteNode's dependency
array (i.e., change deleteNode to a useCallback with correct dependencies and
update requestDeleteNode's deps to [onRequestDeleteNode, deleteNode]) so the
function won't close over stale state.
In
`@skyvern-frontend/src/routes/workflows/editor/panels/WorkflowParametersPanel.tsx`:
- Around line 87-89: Reformat the return statement in
WorkflowParametersPanel.tsx where you call removeKeyFromNodesParameterKeys so it
matches the project's linting/formatting conventions (fix any extra
whitespace/indentation or semicolon mismatch on the line "return
removeKeyFromNodesParameterKeys(nodesWithRemovedRefs, parameterKey);"); run the
project formatter (prettier --write) or eslint --fix on the file to enforce
consistent style and re-run the linter.
In `@skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts`:
- Line 2829: The long conditional call to objectContainsJinjaReference needs
reformatting to satisfy the linter: break the call across lines or extract
arguments into a descriptive local variable before the if. Locate the if using
objectContainsJinjaReference(node.data, key, SKIP_KEYS_FOR_JINJA_CHECK) and
refactor so the condition is wrapped across lines (or assign
objectContainsJinjaReference(...) result to a well-named boolean like
hasJinjaRef and use if (hasJinjaRef)) to keep line length under the limit while
preserving the same logic.
🧹 Nitpick comments (2)
skyvern-frontend/src/components/DeleteConfirmationDialog.tsx (1)
78-85: Consider closing the dialog after confirmation.The Delete button calls
onConfirm()but doesn't close the dialog. While callers can handle this in theironConfirmimplementations, it would be more intuitive to close the dialog automatically after confirmation to prevent double-clicks and ensure consistent UX.♻️ Suggested improvement
<Button variant="destructive" onClick={() => { onConfirm(); + onOpenChange(false); }} > Delete </Button>skyvern-frontend/src/routes/workflows/editor/Workspace.tsx (1)
269-280: Consider removing unusednodeIdfrom dialog state.The
nodeIdis stored indeleteBlockDialogStatebut doesn't appear to be used anywhere - affected blocks are computed fromnodeLabel, and the confirm callback handles the actual deletion. Consider simplifying:const [deleteBlockDialogState, setDeleteBlockDialogState] = useState<{ open: boolean; - nodeId: string | null; nodeLabel: string | null; }>({ open: false, - nodeId: null, nodeLabel: null, });Unless there's a planned use for
nodeId, this would reduce unnecessary state.
celalzamanoglu
force-pushed
the
celal/dialog-for-removing-blocks-and-parameters
branch
from
January 22, 2026 21:20
3cd8b68 to
954713d
Compare
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 954713d in 2 minutes and 21 seconds. Click for details.
- Reviewed
1127lines of code in10files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts:2668
- Draft comment:
In the replaceJinjaReference function, the regex replacement only updates the '{{' prefix and does not explicitly re-insert the closing braces. Please add a comment clarifying that this behavior is intentional and that the rest of the template (including closing '}}') remains unaffected. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts:392
- Draft comment:
The getWorkflowErrors function checks if the first workflow block node is a 'validation' block and produces an error. Consider verifying whether the ordering of nodes inworkflowBlockNodestruly corresponds to the workflow's starting block. If node order matters, sorting or a more explicit check might be clearer. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. skyvern-frontend/src/routes/workflows/editor/panels/WorkflowParametersPanel.tsx:76
- Draft comment:
Consider wrapping the handleDeleteParameter callback with useCallback for performance optimization to avoid unnecessary re-renders of child components. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a code quality/performance suggestion. The rules state that "Comments that suggest code quality refactors are good! But only if they are actionable and clear." However, I need to consider: 1) Is this optimization actually necessary? The function is only passed to one component (DeleteConfirmationDialog). 2) Without seeing the implementation ofDeleteConfirmationDialog, I can't verify if it's memoized or if this would actually prevent re-renders. 3) The comment is speculative about performance benefits ("to avoid unnecessary re-renders") without strong evidence that this is actually causing a problem. 4) This is a minor optimization that may not have any real impact. The rules say not to make speculative comments and to only keep comments with STRONG EVIDENCE they are correct. While usinguseCallbackis a React best practice, without evidence thatDeleteConfirmationDialogis memoized or that this is causing actual performance issues, this suggestion is speculative. The comment assumes there will be performance benefits without demonstrating that the current implementation has problems. The comment is making a speculative performance claim without strong evidence. We don't know ifDeleteConfirmationDialogis memoized, and even if it is, this might be premature optimization. The rules explicitly state not to make speculative comments and require STRONG EVIDENCE for keeping a comment. This comment should be deleted. It's a speculative performance optimization suggestion without strong evidence that it's necessary or would have meaningful impact. The rules require strong evidence and discourage speculative comments.
4. skyvern-frontend/src/routes/workflows/editor/nodes/NodeActionMenu.tsx:36
- Draft comment:
For improved accessibility, consider adding aria-label or role attributes on the dropdown trigger (the DotsHorizontalIcon) so screen readers can better describe its purpose. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. skyvern-frontend/src/routes/workflows/hooks/useRequestDeleteNodeCallback.ts:4
- Draft comment:
The useRequestDeleteNodeCallback hook cleanly retrieves the callback from context. Ensure that all consumers of this hook are wrapped inside a BlockActionContextProvider to avoid runtime errors. No changes required but a note for future maintainers. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. skyvern-frontend/src/store/BlockActionContext.ts:1
- Draft comment:
The BlockActionContext file defines the context for node actions. The type definitions are clear. Ensure that any updates to the callback signatures in the future maintain backward compatibility. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_DgfAJ0dVgEtYV79I
You can customize
…src/'
## Summary
When users delete a block or parameter in the workflow editor, they now see a confirmation dialog that displays which other blocks will be affected by the deletion. This helps users understand the impact of their action before confirming.
## What was the problem
1. **No confirmation for block deletion**: Deleting a block was immediate with no warning, even when other blocks referenced its output via `{{ block_name_output }}` or through parameter selectors.
2. **Parameter deletion dialog lacked context**: The existing parameter deletion confirmation was a simple "Are you sure?" dialog without showing which blocks would be affected.
3. **Duplicated cleanup code**: The logic for removing references from nodes when deleting blocks or parameters was duplicated across multiple files with verbose node-type-specific handling (~150+ lines each).
4. **UI bug with dropdown + dialog interaction**: Opening the delete confirmation dialog from the block action menu (dropdown) caused pointer-events to get stuck, making the UI unresponsive after closing the dialog.
## What changed
- Added a generic `DeleteConfirmationDialog` component that displays affected blocks before deletion, used for both block and parameter deletion
- Affected blocks are detected by checking both `parameterKeys` arrays and jinja template references in text fields
- Created `getAffectedBlocks()` utility to find blocks that reference a given key
- Created `removeKeyFromNodesParameterKeys()` utility to consolidate the cleanup logic, reducing code duplication significantly
- Fixed the pointer-events bug by setting `modal={false}` on the `DropdownMenu` in `NodeActionMenu`, preventing conflict with the dialog's modal behavior
- Simplified `BlockActionContext` by removing the unused `deleteNodeCallback` in favor of `requestDeleteNodeCallback` which supports the confirmation flow
## Screenshots
<img width="2293" height="670" alt="image" src="https://github.com/user-attachments/assets/a4bbd31b-d2f8-42b1-9ba1-47f49c0453b5" />
<img width="2391" height="651" alt="image" src="https://github.com/user-attachments/assets/a8005ab8-068a-45da-afe8-7bbc78492aac" />
<!-- ELLIPSIS_HIDDEN -->
----
> [!IMPORTANT]
> Introduces a confirmation dialog showing affected blocks when deleting blocks or parameters, consolidates cleanup logic, and fixes a UI bug.
>
> - **Behavior**:
> - Added `DeleteConfirmationDialog` component to show affected blocks before deletion in `DeleteConfirmationDialog.tsx`.
> - Affected blocks detected via `parameterKeys` and jinja template references.
> - Introduced `getAffectedBlocks()` and `removeKeyFromNodesParameterKeys()` utilities in `workflowEditorUtils.ts`.
> - Fixed pointer-events bug in `NodeActionMenu.tsx` by setting `modal={false}`.
> - **Refactoring**:
> - Consolidated cleanup logic for node references in `FlowRenderer.tsx` and `Workspace.tsx`.
> - Removed `useDeleteNodeCallback.ts` in favor of `useRequestDeleteNodeCallback.ts`.
> - **UI**:
> - Enhanced deletion workflow with clearer confirmation steps in `Workspace.tsx` and `WorkflowParametersPanel.tsx`.
>
> <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 39f818c1a3677ea854c29bf17b66fb25dc7b65dc. You can [customize](https://app.ellipsis.dev/Skyvern-AI/settings/summaries) this summary. It will automatically update as commits are pushed.</sup>
----
<!-- ELLIPSIS_HIDDEN -->
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **New Features**
* Added a reusable delete confirmation dialog that lists affected blocks when removing workflow nodes or parameters.
* **UI Improvements**
* Centralized and clarified the deletion flow with explicit confirm/cancel actions and clearer impact visibility.
* **Refactor**
* Deletion now routes through a request-confirmation flow so deletions can be deferred for user confirmation and cascade updates consistently.
<sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
celalzamanoglu
force-pushed
the
celal/dialog-for-removing-blocks-and-parameters
branch
from
January 22, 2026 21:27
954713d to
9963678
Compare
Contributor
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 9963678 in 1 minute and 28 seconds. Click for details.
- Reviewed
1132lines of code in10files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. skyvern-frontend/src/components/DeleteConfirmationDialog.tsx:1
- Draft comment:
The DeleteConfirmationDialog component is clear and reusable. Consider externalizing static texts (e.g. confirmation messages) for localization. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. skyvern-frontend/src/routes/workflows/editor/FlowRenderer.tsx:532
- Draft comment:
Good refactoring using useCallback and deleteNodeRef for latest deletion logic. The fallback that directly deletes if no confirmation handler is provided is a neat safeguard. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. skyvern-frontend/src/routes/workflows/editor/Workspace.tsx:268
- Draft comment:
The new state and memoized calculation for affected blocks looks good. Ensure that getOutputParameterKey always returns a valid key when nodeLabel is set. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. skyvern-frontend/src/routes/workflows/editor/nodes/NodeActionMenu.tsx:36
- Draft comment:
Setting modal={false} on DropdownMenu resolves the pointer-events bug; this is an effective workaround. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. skyvern-frontend/src/routes/workflows/editor/nodes/components/NodeHeader.tsx:18
- Draft comment:
Using useRequestDeleteNodeCallback in place of the old hook is a clean update. The callback is used correctly with nodeId and blockLabel. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. skyvern-frontend/src/routes/workflows/editor/panels/WorkflowParametersPanel.tsx:66
- Draft comment:
The refactoring in the parameter deletion flow to use DeleteConfirmationDialog and useMemo for affectedBlocks is well done. State management here is clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts:2710
- Draft comment:
The new utility functions (e.g. getAffectedBlocks, removeKeyFromNodesParameterKeys) include useful documentation. Consider caching the compiled RegExp if performance becomes an issue on very large inputs. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. skyvern-frontend/src/routes/workflows/hooks/useRequestDeleteNodeCallback.ts:1
- Draft comment:
The hook is simple and correctly throws an error when used outside the context provider. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. skyvern-frontend/src/store/BlockActionContext.ts:11
- Draft comment:
The BlockActionContext now provides the requestDeleteNodeCallback; the type definitions are clear and consistent with the new deletion flow. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. skyvern-frontend/src/routes/workflows/editor/Workspace.tsx:1494
- Draft comment:
Minor suggestion: The comment "block runs history for current debug session id" could be rephrased for clarity. Consider updating it to something like "block run history for current debug session ID" to ensure both grammatical consistency and proper capitalization of 'ID'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_khDzrRL97iiEHa2m
You can customize
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@skyvern-frontend/src/routes/workflows/editor/panels/WorkflowParametersPanel.tsx`:
- Around line 185-195: The delete button is an icon-only control and needs an
accessible label for screen readers; update the button that calls
setDeleteDialogState({ open: true, parameterKey: parameter.key }) to include an
aria-label (or aria-labelledby) describing its action (e.g., "Delete parameter"
or including parameter.key) so the GarbageIcon-only button is accessible to
assistive technologies.
♻️ Duplicate comments (1)
skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts (1)
2829-2829: Wrap the longobjectContainsJinjaReferencecall to satisfy lint.The formatter expects this condition to be wrapped to meet the configured line-length rule.
🔧 Suggested fix
- if (objectContainsJinjaReference(node.data, key, SKIP_KEYS_FOR_JINJA_CHECK)) { + if ( + objectContainsJinjaReference(node.data, key, SKIP_KEYS_FOR_JINJA_CHECK) + ) {
Comment on lines
+185
to
+195
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| setDeleteDialogState({ | ||
| open: true, | ||
| parameterKey: parameter.key, | ||
| }); | ||
| }} | ||
| > | ||
| <GarbageIcon className="size-4 cursor-pointer text-destructive-foreground text-red-600" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add an accessible label to the icon-only delete button.
Icon-only controls need an aria-label for screen readers.
♿ Suggested fix
- <button
- type="button"
- onClick={() => {
+ <button
+ type="button"
+ aria-label={`Delete parameter ${parameter.key}`}
+ onClick={() => {📝 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.
Suggested change
🤖 Prompt for AI Agents
In
`@skyvern-frontend/src/routes/workflows/editor/panels/WorkflowParametersPanel.tsx`
around lines 185 - 195, The delete button is an icon-only control and needs an
accessible label for screen readers; update the button that calls
setDeleteDialogState({ open: true, parameterKey: parameter.key }) to include an
aria-label (or aria-labelledby) describing its action (e.g., "Delete parameter"
or including parameter.key) so the GarbageIcon-only button is accessible to
assistive technologies.
celalzamanoglu
deleted the
celal/dialog-for-removing-blocks-and-parameters
branch
January 22, 2026 21:33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When users delete a block or parameter in the workflow editor, they now see a confirmation dialog that displays which other blocks will be affected by the deletion. This helps users understand the impact of their action before confirming.
What was the problem
No confirmation for block deletion: Deleting a block was immediate with no warning, even when other blocks referenced its output via
{{ block_name_output }}or through parameter selectors.Parameter deletion dialog lacked context: The existing parameter deletion confirmation was a simple "Are you sure?" dialog without showing which blocks would be affected.
Duplicated cleanup code: The logic for removing references from nodes when deleting blocks or parameters was duplicated across multiple files with verbose node-type-specific handling (~150+ lines each).
UI bug with dropdown + dialog interaction: Opening the delete confirmation dialog from the block action menu (dropdown) caused pointer-events to get stuck, making the UI unresponsive after closing the dialog.
What changed
DeleteConfirmationDialogcomponent that displays affected blocks before deletion, used for both block and parameter deletionparameterKeysarrays and jinja template references in text fieldsgetAffectedBlocks()utility to find blocks that reference a given keyremoveKeyFromNodesParameterKeys()utility to consolidate the cleanup logic, reducing code duplication significantlymodal={false}on theDropdownMenuinNodeActionMenu, preventing conflict with the dialog's modal behaviorBlockActionContextby removing the unuseddeleteNodeCallbackin favor ofrequestDeleteNodeCallbackwhich supports the confirmation flowScreenshots
Important
Adds confirmation dialog showing affected blocks when deleting blocks or parameters, refactors deletion logic, and fixes UI bug in dropdown menus.
DeleteConfirmationDialogcomponent to show affected blocks when deleting blocks or parameters.parameterKeysand jinja template references.getAffectedBlocks()andremoveKeyFromNodesParameterKeys()utilities inworkflowEditorUtils.ts.deleteNodeCallbackin favor ofrequestDeleteNodeCallbackinBlockActionContext.NodeActionMenuby settingmodal={false}onDropdownMenu.This description was created by
for 9963678. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
UI Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.