improvement(response): only allow singleton #2764
Merged
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
Greptile OverviewGreptile SummaryThis PR implements singleton enforcement for Response blocks to ensure only one Response block can exist per workflow. The implementation follows the existing pattern used for trigger blocks and includes comprehensive validation across most user interaction paths. What Works WellCore Implementation:
Validation Coverage:
Critical Issue Found❌ Action Bar Duplicate Function Bypass: Minor IssueError Message Inconsistency: Architecture NotesThe solution cleverly reuses TriggerUtils class (despite the name) to handle both trigger singleton constraints and general block singleton constraints, maintaining consistency with existing patterns. The validation occurs at all major entry points except the action bar duplicate handler. Confidence Score: 3/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant UI as UI Layer
participant Validation as validateTriggerPaste
participant TriggerUtils
participant BlockRegistry as Block Registry
participant Store as Workflow Store
Note over User,Store: Adding Response Block via Toolbar
User->>UI: Drag Response block to canvas
UI->>UI: checkTriggerConstraints(blockType)
UI->>TriggerUtils: getSingleInstanceBlockIssue(blocks, 'response')
TriggerUtils->>BlockRegistry: getBlock('response')
BlockRegistry-->>TriggerUtils: { singleInstance: true, name: 'Response' }
TriggerUtils->>TriggerUtils: Check if 'response' exists in blocks
alt Response already exists
TriggerUtils-->>UI: { issue: 'duplicate', blockName: 'Response' }
UI->>User: Show error notification
else No existing Response
TriggerUtils-->>UI: null
UI->>Store: addBlock('response', ...)
Store-->>User: Block added successfully
end
Note over User,Store: Pasting/Duplicating Response Block
User->>UI: Paste (Cmd+V) or Context Menu Duplicate
UI->>Validation: validateTriggerPaste([{type:'response'}], blocks, action)
Validation->>TriggerUtils: getSingleInstanceBlockIssue(blocks, 'response')
TriggerUtils->>BlockRegistry: getBlock('response')
BlockRegistry-->>TriggerUtils: { singleInstance: true, name: 'Response' }
TriggerUtils->>TriggerUtils: wouldViolateSingleInstanceBlock(blocks, 'response')
alt Response already exists
TriggerUtils-->>Validation: { issue: 'duplicate', blockName: 'Response' }
Validation-->>UI: { isValid: false, message: 'Cannot duplicate...' }
UI->>User: Show error notification
else No existing Response
TriggerUtils-->>Validation: null
Validation-->>UI: { isValid: true }
UI->>Store: collaborativeBatchAddBlocks(...)
Store-->>User: Block(s) added successfully
end
Note over User,Store: VULNERABILITY: Action Bar Duplicate Button
User->>UI: Click duplicate on Response action bar
UI->>UI: handleDuplicateBlock()
Note right of UI: ⚠️ NO VALIDATION!
UI->>Store: collaborativeBatchAddBlocks(duplicateBlock)
Store-->>User: Duplicate created (violates singleton!)
|
Contributor
Additional Comments (1)
The context menu duplicate handler ( FixAdd validation before duplicating: You'll also need to import Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/workflow-block/components/action-bar/action-bar.tsx
Line: 44:67
Comment:
The `handleDuplicateBlock` function does not validate single-instance block constraints before duplicating. This means a user can click the duplicate button on a Response block's action bar and create multiple Response blocks, bypassing the singleton constraint that this PR is supposed to enforce.
The context menu duplicate handler (`handleContextDuplicate` in workflow.tsx) correctly validates using `validateTriggerPaste`, but this direct duplication path does not.
## Fix
Add validation before duplicating:
```suggestion
const handleDuplicateBlock = useCallback(() => {
const sourceBlock = blocks[blockId]
if (!sourceBlock) return
// Validate single-instance constraints
const validation = validateTriggerPaste([{ type: sourceBlock.type }], blocks, 'duplicate')
if (!validation.isValid) {
addNotification({
level: 'error',
message: validation.message!,
workflowId: activeWorkflowId || undefined,
})
return
}
const newId = crypto.randomUUID()
const newName = getUniqueBlockName(sourceBlock.name, blocks)
const subBlockValues = subBlockStore.workflowValues[activeWorkflowId || '']?.[blockId] || {}
const { block, subBlockValues: filteredValues } = prepareDuplicateBlockState({
sourceBlock,
newId,
newName,
positionOffset: DEFAULT_DUPLICATE_OFFSET,
subBlockValues,
})
collaborativeBatchAddBlocks([block], [], {}, {}, { [newId]: filteredValues })
}, [
blockId,
blocks,
activeWorkflowId,
subBlockStore.workflowValues,
collaborativeBatchAddBlocks,
addNotification,
])
```
You'll also need to import `validateTriggerPaste` and `addNotification` at the top of the file.
How can I resolve this? If you propose a fix, please make it concise. |
Collaborator
Author
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
48
to
52
| const singleInstanceIssue = TriggerUtils.getSingleInstanceBlockIssue(existingBlocks, block.type) | ||
| if (singleInstanceIssue) { | ||
| const actionText = action === 'paste' ? 'paste' : 'duplicate' | ||
| const message = `A workflow can only have one ${singleInstanceIssue.blockName} block. ${action === 'paste' ? 'Please remove the existing one before pasting.' : `Cannot ${actionText}.`}` | ||
| return { isValid: false, message } |
Contributor
There was a problem hiding this comment.
Style: Inconsistent error message formatting
The error message for single-instance blocks uses different formatting than trigger blocks:
Single-instance blocks (line 51):
- Paste: "...Please remove the existing one before pasting."
- Duplicate: "...Cannot duplicate."
Trigger blocks (line 43):
- Paste: "...Please remove the existing one before pasting."
- Duplicate: "...Cannot duplicate."
While functionally equivalent, for better consistency with the existing trigger error message pattern on line 43, consider using the same format:
Suggested change
| const singleInstanceIssue = TriggerUtils.getSingleInstanceBlockIssue(existingBlocks, block.type) | |
| if (singleInstanceIssue) { | |
| const actionText = action === 'paste' ? 'paste' : 'duplicate' | |
| const message = `A workflow can only have one ${singleInstanceIssue.blockName} block. ${action === 'paste' ? 'Please remove the existing one before pasting.' : `Cannot ${actionText}.`}` | |
| return { isValid: false, message } | |
| const singleInstanceIssue = TriggerUtils.getSingleInstanceBlockIssue(existingBlocks, block.type) | |
| if (singleInstanceIssue) { | |
| const actionText = action === 'paste' ? 'paste' : 'duplicate' | |
| const message = `A workflow can only have one ${singleInstanceIssue.blockName} block. ${action === 'paste' ? 'Please remove the existing one before pasting.' : 'Cannot duplicate.'}` | |
| return { isValid: false, message } | |
| } |
This maintains identical phrasing to the trigger validation above for a more cohesive user experience.
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/workflow-canvas-helpers.ts
Line: 48:52
Comment:
## Style: Inconsistent error message formatting
The error message for single-instance blocks uses different formatting than trigger blocks:
**Single-instance blocks (line 51):**
- Paste: "...Please remove the existing one before pasting."
- Duplicate: "...Cannot duplicate."
**Trigger blocks (line 43):**
- Paste: "...Please remove the existing one before pasting."
- Duplicate: "...Cannot duplicate."
While functionally equivalent, for better consistency with the existing trigger error message pattern on line 43, consider using the same format:
```suggestion
const singleInstanceIssue = TriggerUtils.getSingleInstanceBlockIssue(existingBlocks, block.type)
if (singleInstanceIssue) {
const actionText = action === 'paste' ? 'paste' : 'duplicate'
const message = `A workflow can only have one ${singleInstanceIssue.blockName} block. ${action === 'paste' ? 'Please remove the existing one before pasting.' : 'Cannot duplicate.'}`
return { isValid: false, message }
}
```
This maintains identical phrasing to the trigger validation above for a more cohesive user experience.
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Additional Comments (1)
Critical: Single-instance constraint bypass in handleDuplicateBlockThe Impact: Users could potentially duplicate Response blocks through:
Comparison with other duplication paths:
Fix Required: You'll need to import:
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/workflow-block/components/action-bar/action-bar.tsx
Line: 44:67
Comment:
## Critical: Single-instance constraint bypass in handleDuplicateBlock
The `handleDuplicateBlock` function does not validate single-instance block constraints before duplicating. While the duplicate button is now hidden for Response blocks (line 143), this function can still be invoked programmatically or through other UI paths, allowing users to bypass the singleton constraint.
**Impact:** Users could potentially duplicate Response blocks through:
1. Programmatic invocation
2. Future UI changes that expose this path
3. Browser dev tools or extensions
**Comparison with other duplication paths:**
- ✅ Context menu duplicate (`handleContextDuplicate` in workflow.tsx) validates via `validateTriggerPaste`
- ✅ Keyboard paste (Cmd+V in workflow.tsx) validates via `validateTriggerPaste`
- ✅ Context menu paste validates via `validateTriggerPaste`
- ❌ Action bar duplicate does NOT validate
**Fix Required:**
Add validation before duplicating:
```suggestion
const handleDuplicateBlock = useCallback(() => {
const sourceBlock = blocks[blockId]
if (!sourceBlock) return
// Validate single-instance constraints
const validation = validateTriggerPaste([{ type: sourceBlock.type }], blocks, 'duplicate')
if (!validation.isValid) {
addNotification({
level: 'error',
message: validation.message!,
workflowId: activeWorkflowId || undefined,
})
return
}
const newId = crypto.randomUUID()
```
You'll need to import:
- `validateTriggerPaste` from `'@/app/workspace/[workspaceId]/w/[workflowId]/utils/workflow-canvas-helpers'`
- `useNotificationStore` from `'@/stores/notifications'`
How can I resolve this? If you propose a fix, please make it concise. |
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.
Summary
Response blocks should be singleton. Causing issues with workflow block, and in general should be like an API contract.
Type of Change
Testing
Tested manually
Checklist