-
Notifications
You must be signed in to change notification settings - Fork 711
fix: added warning message to the pipeline default destination node #8596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 4a9bfaa
Previous suggestionsSuggestions up to commit aae43a3
|
aae43a3 to
a9bc479
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Summary
This PR adds warning messages when users attempt to delete default destination nodes in realtime pipelines. The implementation spans four files:
- Enhanced ConfirmDialog component to support warning messages with theme-appropriate styling using Tailwind classes
- Added detection logic in
useDnD.tsto identify default destination nodes by comparing stream names between input and output nodes - Integrated warning display in both Stream.vue and CustomNode.vue components to show the warning when deleting default destination nodes
Key changes:
- Warning appears only for realtime pipelines where a default destination node was automatically added
- Uses conditional logic to show appropriate warning message explaining data flow impact
- Maintains consistent UI styling across light and dark themes
Confidence Score: 4/5
- This PR is generally safe to merge with minor improvements needed
- The implementation is solid but has one potential runtime error and minor style issues that should be addressed
- Focus on web/src/plugins/pipelines/useDnD.ts for the null check fix
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| web/src/components/ConfirmDialog.vue | 5/5 | Enhanced dialog component to support warning messages with theme-appropriate styling |
| web/src/components/pipeline/NodeForm/Stream.vue | 4/5 | Added warning message support for default destination node deletion with conditional logic |
| web/src/plugins/pipelines/CustomNode.vue | 4/5 | Integrated warning message for default destination node deletion in node context menu |
| web/src/plugins/pipelines/useDnD.ts | 3/5 | Core logic for identifying default destination nodes with stream name comparison |
Sequence Diagram
sequenceDiagram
participant User
participant StreamNode as Stream/CustomNode
participant DnDComposable as useDnD.ts
participant ConfirmDialog as ConfirmDialog.vue
User->>StreamNode: Click delete on node
StreamNode->>DnDComposable: checkIfDefaultDestinationNode(nodeId)
DnDComposable->>DnDComposable: getInputNodeStream()
DnDComposable-->>StreamNode: Returns true if default destination
alt Is Default Destination Node
StreamNode->>StreamNode: Set warningMessage
Note over StreamNode: "If you delete this default destination node..."
else Not Default Destination
StreamNode->>StreamNode: Clear warningMessage
end
StreamNode->>ConfirmDialog: Show dialog with warning
ConfirmDialog->>User: Display confirmation with warning banner
User->>ConfirmDialog: Click OK/Cancel
alt User confirms
ConfirmDialog->>StreamNode: Emit confirm
StreamNode->>DnDComposable: deletePipelineNode(nodeId)
else User cancels
ConfirmDialog->>StreamNode: Emit cancel
StreamNode->>StreamNode: Close dialog
end
4 files reviewed, 3 comments
| const checkIfDefaultDestinationNode = (id: string) => { | ||
| const inputNodeStream = getInputNodeStream(); | ||
| const nodes = pipelineObj.currentSelectedPipeline?.nodes ?? []; | ||
| if(inputNodeStream){ | ||
| return nodes.some((node: any) => node.id === id && node.type === 'output' && (node.data.stream_name.value === inputNodeStream || node.data.stream_name === inputNodeStream)); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: potential runtime error if inputNode is undefined
| const checkIfDefaultDestinationNode = (id: string) => { | |
| const inputNodeStream = getInputNodeStream(); | |
| const nodes = pipelineObj.currentSelectedPipeline?.nodes ?? []; | |
| if(inputNodeStream){ | |
| return nodes.some((node: any) => node.id === id && node.type === 'output' && (node.data.stream_name.value === inputNodeStream || node.data.stream_name === inputNodeStream)); | |
| } | |
| }; | |
| const getInputNodeStream = () => { | |
| const nodes = pipelineObj.currentSelectedPipeline?.nodes ?? []; | |
| const inputNode = nodes.find((node: any) => node.io_type === "input"); | |
| if(!inputNode) return null; | |
| if(inputNode.data.hasOwnProperty('node_type') && inputNode.data.node_type === 'stream'){ | |
| return inputNode?.data?.stream_name?.value || inputNode.data.stream_name || ""; | |
| } | |
| else { | |
| return null; | |
| } | |
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes fixed that
| // //here we will check if the destination node is added by default if yes then we will show a warning message to the user | ||
| if(pipelineObj.currentSelectedNodeData?.data.hasOwnProperty('node_type') && pipelineObj.currentSelectedNodeData?.data.node_type === 'stream' && checkIfDefaultDestinationNode(pipelineObj.currentSelectedNodeID)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: inconsistent comment formatting - remove extra slash
| // //here we will check if the destination node is added by default if yes then we will show a warning message to the user | |
| if(pipelineObj.currentSelectedNodeData?.data.hasOwnProperty('node_type') && pipelineObj.currentSelectedNodeData?.data.node_type === 'stream' && checkIfDefaultDestinationNode(pipelineObj.currentSelectedNodeID)){ | |
| // here we will check if the destination node is added by default if yes then we will show a warning message to the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
| //here we will check if the destination node is added by default if yes then we will show a warning message to the user | ||
| if(props.data?.hasOwnProperty('node_type') && props.data.node_type === 'stream' && checkIfDefaultDestinationNode(id)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: inconsistent comment formatting - remove extra slash
| //here we will check if the destination node is added by default if yes then we will show a warning message to the user | |
| if(props.data?.hasOwnProperty('node_type') && props.data.node_type === 'stream' && checkIfDefaultDestinationNode(id)){ | |
| // here we will check if the destination node is added by default if yes then we will show a warning message to the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
385c176 to
00fb99d
Compare
3a0f62a to
4a9bfaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Summary
This PR adds warning messages when users attempt to delete default destination nodes in realtime pipelines. The implementation correctly identifies default destination nodes by comparing the destination stream name with the input stream name and displays contextual warnings through an enhanced confirm dialog.
Key Changes:
- Enhanced ConfirmDialog: Added optional
warningMessageprop with themed warning banner and responsive width - Default Detection Logic: New
checkIfDefaultDestinationNode()helper in useDnD.ts to identify auto-created destination nodes - Warning Integration: Both Stream.vue and CustomNode.vue now show warnings when deleting default destinations
- Constants Extraction: Warning message moved to dedicated constants file for reusability
Issues Found:
- Minor potential null access issue in
getInputNodeStream()function that should be addressed
The implementation follows good separation of concerns with the detection logic centralized in useDnD.ts and shared constants. The UI properly handles both light and dark themes for the warning display.
Confidence Score: 4/5
- This PR is safe to merge with minimal risk after addressing the null access issue
- Score reflects well-structured implementation with good separation of concerns, proper theme handling, and effective warning system. One minor logical issue with potential null access reduces confidence slightly but doesn't impact core functionality.
- Pay close attention to web/src/plugins/pipelines/useDnD.ts for the potential null access issue
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| web/src/plugins/pipelines/useDnD.ts | 4/5 | Added helper functions to detect default destination nodes and get input stream name, with one potential null access issue |
| web/src/components/ConfirmDialog.vue | 5/5 | Enhanced dialog with optional warning message support, proper theme handling, and responsive width |
| web/src/components/pipeline/NodeForm/Stream.vue | 5/5 | Integrated warning message for default destination node deletion using new helper function |
| web/src/plugins/pipelines/CustomNode.vue | 5/5 | Added warning message support to node deletion dialog using same default destination detection |
| web/src/utils/pipelines/constants.ts | 5/5 | New constant file containing the warning message for default destination node deletion |
Sequence Diagram
sequenceDiagram
participant U as User
participant SV as Stream.vue
participant CN as CustomNode.vue
participant UD as useDnD.ts
participant CD as ConfirmDialog.vue
participant C as Constants
Note over U,C: Default Destination Node Warning Flow
U->>+SV: Click delete on stream node
SV->>+UD: checkIfDefaultDestinationNode(nodeId)
UD->>+UD: getInputNodeStream()
UD-->>-UD: return input stream name
UD->>+UD: Check if node matches default destination
UD-->>-SV: return true/false
alt Is default destination node
SV->>+C: Import defaultDestinationNodeWarningMessage
C-->>-SV: Warning message text
SV->>SV: Set dialog.warningMessage
else Not default destination
SV->>SV: Clear dialog.warningMessage
end
SV->>+CD: Show dialog with warning message
CD->>CD: Render warning banner if message exists
CD->>CD: Adjust dialog width dynamically
CD-->>U: Display warning dialog
alt User confirms deletion
CD-->>-SV: Emit update:ok
SV->>UD: deletePipelineNode(nodeId)
else User cancels
CD-->>SV: Emit update:cancel
end
Note over U,CN: Similar flow for CustomNode.vue
U->>+CN: Click delete on custom node
CN->>+UD: checkIfDefaultDestinationNode(nodeId)
UD-->>-CN: return true/false
CN->>+CD: Show dialog with warning
CD-->>-U: Display warning dialog
5 files reviewed, 1 comment
| const nodes = pipelineObj.currentSelectedPipeline?.nodes ?? []; | ||
| const inputNode = nodes.find((node: any) => node.io_type === "input"); | ||
| if(inputNode?.data.hasOwnProperty('node_type') && inputNode.data.node_type === 'stream'){ | ||
| return inputNode?.data?.stream_name?.value || inputNode.data.stream_name || ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: potential null access if inputNode is undefined
| return inputNode?.data?.stream_name?.value || inputNode.data.stream_name || ""; | |
| return inputNode?.data?.stream_name?.value || inputNode?.data.stream_name || ""; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/plugins/pipelines/useDnD.ts
Line: 588:588
Comment:
logic: potential null access if `inputNode` is undefined
```suggestion
return inputNode?.data?.stream_name?.value || inputNode?.data.stream_name || "";
```
How can I resolve this? If you propose a fix, please make it concise.
User description
This PR solves the issue of deleting the default destination node added implicitly so it will throw a warning message and also it will be only available for realtime pipeline because we only add default destination node for realtime pipelines #8597 8597
PR Type
Bug fix, Enhancement
Description
Warn on deleting default destination node
Detect default output for realtime pipelines
Add reusable warning message constant
Extend confirm dialog to show banner
Diagram Walkthrough
File Walkthrough
useDnD.ts
Add utilities to detect default destination nodeweb/src/plugins/pipelines/useDnD.ts
checkIfDefaultDestinationNode.ConfirmDialog.vue
Enhance dialog to display warning bannerweb/src/components/ConfirmDialog.vue
warningMessageprop.constants.ts
Centralize default destination warning messageweb/src/utils/pipelines/constants.ts
defaultDestinationNodeWarningMessageconstant.Stream.vue
Show warning when deleting default destination (form)web/src/components/pipeline/NodeForm/Stream.vue
warningMessageto ConfirmDialog.checkIfDefaultDestinationNodebefore delete.CustomNode.vue
Warn on deleting default destination (node card)web/src/plugins/pipelines/CustomNode.vue
warningMessageto ConfirmDialog.