improvement(mcp): improved deployed mcp ui #2758
Open
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. |
aadamgough
changed the title
Contributor
Greptile OverviewGreptile SummaryThis PR refactors the MCP deployment UI to make deployed MCPs more explicit through a "deferred save" pattern and adds workflow validation to ensure only workflows with start blocks can be deployed as MCP tools. Key Changes1. Workflow Validation System
2. Deferred Save Pattern in Deploy Modal
3. Multi-Select Workflow Creation
Critical Issues Found1. Partial Failure Handling (mcp.tsx - CRITICAL)When some operations fail during save, the code still executes success indicators:
2. Security Vulnerability (validate/route.ts - HIGH)The validation endpoint doesn't verify workflows belong to the requesting user's workspace:
3. Undefined vs False Handling (workflow-mcp-servers.tsx - MEDIUM)Validation logic treats
4. Silent Validation Failures (workflow-mcp-servers.ts - MEDIUM)When validation API fails,
5. Race Condition in State Management (mcp.tsx - LOW)Manual
6. Inconsistent Initial Data Loading (mcp.tsx - LOW)When workflow is deployed to multiple servers with different metadata:
Positive Aspects
Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant DeployModal as Deploy Modal<br/>(mcp.tsx)
participant SettingsModal as Settings Modal<br/>(workflow-mcp-servers.tsx)
participant ValidateAPI as /api/mcp/workflow-servers/validate
participant ToolsAPI as /api/mcp/workflow-servers/.../tools
participant Hook as useDeployedWorkflows Hook
participant DB as Database
Note over User,DB: Workflow Validation Flow (New)
Hook->>ValidateAPI: POST /validate with workflowIds[]
ValidateAPI->>DB: loadWorkflowFromNormalizedTables(workflowId)
DB-->>ValidateAPI: workflow state
ValidateAPI->>ValidateAPI: hasValidStartBlockInState(state)
ValidateAPI-->>Hook: {workflowId: boolean}
Hook-->>DeployModal: workflows with hasStartBlock flag
Hook-->>SettingsModal: workflows with hasStartBlock flag
Note over User,DB: Deferred Save Pattern (Deploy Modal)
User->>DeployModal: Select servers
DeployModal->>DeployModal: setPendingSelectedServerIds(serverIds)
Note over DeployModal: No API calls yet - changes held in state
User->>DeployModal: Edit tool metadata
DeployModal->>DeployModal: Update local state (toolName, description, etc)
User->>DeployModal: Click Save
DeployModal->>DeployModal: Calculate toAdd, toRemove, toUpdate
loop For each server in toAdd
DeployModal->>ToolsAPI: POST /tools (add workflow)
ToolsAPI->>DB: Verify workflow, check start block
ToolsAPI->>DB: Insert workflowMcpTool
DB-->>ToolsAPI: Created tool
ToolsAPI-->>DeployModal: Success or Error
alt Error occurred
DeployModal->>DeployModal: Add to errors array
end
end
loop For each server in toRemove
DeployModal->>ToolsAPI: DELETE /tools/:toolId
ToolsAPI->>DB: Delete workflowMcpTool
DB-->>ToolsAPI: Deleted
ToolsAPI-->>DeployModal: Success or Error
DeployModal->>DeployModal: Manually update serverToolsMap
alt Error occurred
DeployModal->>DeployModal: Add to errors array
end
end
loop For each server in toUpdate
DeployModal->>ToolsAPI: PATCH /tools/:toolId
ToolsAPI->>DB: Update workflowMcpTool metadata
DB-->>ToolsAPI: Updated tool
ToolsAPI-->>DeployModal: Success or Error
alt Error occurred
DeployModal->>DeployModal: Add to errors array
end
end
alt Errors occurred (partial failure)
DeployModal->>DeployModal: ISSUE: Still executes refetchServers()
DeployModal->>DeployModal: ISSUE: Clears pending state
DeployModal->>User: Show error but UI shows success state
else All succeeded
DeployModal->>DeployModal: refetchServers(), clear pending state
DeployModal->>User: Success
end
Note over User,DB: Multi-Select Workflow Creation (Settings Modal)
User->>SettingsModal: Fill server name + select workflows
SettingsModal->>SettingsModal: Validate hasStartBlock !== true
User->>SettingsModal: Click "Add Server"
SettingsModal->>ToolsAPI: POST /workflow-servers (create server)
ToolsAPI->>DB: Insert workflowMcpServer
DB-->>ToolsAPI: Created server
ToolsAPI-->>SettingsModal: server object
loop For each selected workflow
SettingsModal->>ToolsAPI: POST /tools (add workflow to server)
ToolsAPI->>DB: Verify & insert workflowMcpTool
ToolsAPI-->>SettingsModal: Success or Error
alt Error occurred
SettingsModal->>SettingsModal: Add to workflowErrors array
end
end
alt Some workflows failed to add
SettingsModal->>SettingsModal: ISSUE: Show error but don't reset form
SettingsModal->>User: Error shown, unclear next steps
else All succeeded
SettingsModal->>SettingsModal: resetForm()
SettingsModal->>User: Success, form cleared
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
358
to
370
| for (const serverId of toUpdate) { | ||
| const toolInfo = serverToolsMap[serverId] | ||
| if (toolInfo?.tool) { | ||
| await updateToolMutation.mutateAsync({ | ||
| workspaceId, | ||
| serverId, | ||
| toolId: toolInfo.tool.id, | ||
| toolName: toolName.trim(), | ||
| toolDescription: toolDescription.trim() || undefined, | ||
| parameterSchema, | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This loop has no error handling at all. If updateToolMutation.mutateAsync() throws an error for any server:
- The entire
handleSaveoperation fails and jumps to the outer catch block (line 382) - Earlier successful add/remove operations are committed, but updates are not
- State cleanup (lines 374-381) doesn't execute, leaving
pendingSelectedServerIdsset and the UI in an inconsistent state
This creates a worse failure mode than the add/remove loops because partial success + partial failure leaves the system in a confusing state where the user can't retry without understanding which operations completed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
Line: 358:370
Comment:
This loop has **no error handling at all**. If `updateToolMutation.mutateAsync()` throws an error for any server:
1. The entire `handleSave` operation fails and jumps to the outer catch block (line 382)
2. Earlier successful add/remove operations are committed, but updates are not
3. State cleanup (lines 374-381) doesn't execute, leaving `pendingSelectedServerIds` set and the UI in an inconsistent state
This creates a worse failure mode than the add/remove loops because partial success + partial failure leaves the system in a confusing state where the user can't retry without understanding which operations completed.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
591
to
601
| if (selectedWorkflowIds.length > 0 && server?.id) { | ||
| await Promise.all( | ||
| selectedWorkflowIds.map((workflowId) => | ||
| addToolMutation.mutateAsync({ | ||
| workspaceId, | ||
| serverId: server.id, | ||
| workflowId, | ||
| }) | ||
| ) | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Critical error handling issue: if the server is created successfully (line 586-589) but adding any workflow fails in the Promise.all() (line 592-600), the catch block at line 604 logs the error but:
- The form is NOT reset -
resetForm()on line 603 is skipped - No error feedback to user - they see the error only in console, not in the UI
- Orphaned server - a server was created but user can't see it or know it exists
- Partial failure - some workflows may have been added before the failure
This leaves the UI in an inconsistent state where the form still shows the data, but clicking "Add Server" again would try to create a duplicate server.
Solution: Consider either:
- Using
Promise.allSettled()to add all workflows and show which ones failed - Rolling back the server creation if workflow additions fail
- Always calling
resetForm()in afinallyblock so the form clears regardless - Showing error state in UI, not just logging
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/workflow-mcp-servers/workflow-mcp-servers.tsx
Line: 591:601
Comment:
Critical error handling issue: if the server is created successfully (line 586-589) but adding any workflow fails in the `Promise.all()` (line 592-600), the catch block at line 604 logs the error but:
1. **The form is NOT reset** - `resetForm()` on line 603 is skipped
2. **No error feedback to user** - they see the error only in console, not in the UI
3. **Orphaned server** - a server was created but user can't see it or know it exists
4. **Partial failure** - some workflows may have been added before the failure
This leaves the UI in an inconsistent state where the form still shows the data, but clicking "Add Server" again would try to create a duplicate server.
**Solution**: Consider either:
- Using `Promise.allSettled()` to add all workflows and show which ones failed
- Rolling back the server creation if workflow additions fail
- Always calling `resetForm()` in a `finally` block so the form clears regardless
- Showing error state in UI, not just logging
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Additional Comments (2)
Issue: If adding a tool to one server fails, the code continues to process other servers, and then still executes lines 372-381 (refetchServers, setPendingSelectedServerIds(null), setSavedValues, etc.) at the end. This means:
Impact: Users may believe all servers were updated successfully when some actually failed, leading to confusion about which tools are deployed where. Solution: Either:
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
Line: 311:331
Comment:
The error handling in this loop is incomplete. Errors thrown by `addToolMutation.mutateAsync()` are silently caught by the outer try-catch but not properly handled, which can leave the system in an inconsistent state.
**Issue**: If adding a tool to one server fails, the code continues to process other servers, and then still executes lines 372-381 (refetchServers, setPendingSelectedServerIds(null), setSavedValues, etc.) at the end. This means:
1. The user sees "success" indicators even though some operations failed
2. The pending state is cleared, hiding the failed operation
3. No feedback is provided about which servers failed
**Impact**: Users may believe all servers were updated successfully when some actually failed, leading to confusion about which tools are deployed where.
**Solution**: Either:
- Add a `catch` block to track failed operations and report them to the user
- Accumulate errors and throw at the end if any operations failed
- Use `Promise.allSettled()` to handle all operations and report results
How can I resolve this? If you propose a fix, please make it concise.
The manual state update on line 343-347 ( Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
Line: 333:356
Comment:
Same error handling issue as the `toAdd` loop. Errors from `deleteToolMutation.mutateAsync()` are silently swallowed, which can cause:
1. The tool to remain on the server but be removed from local state (line 343-347)
2. Success indicators shown to user even when deletion failed
3. No way for user to know which deletions succeeded or failed
The manual state update on line 343-347 (`setServerToolsMap`) is particularly problematic because it happens even if the deletion fails, creating a mismatch between the UI state and server state.
How can I resolve this? If you propose a fix, please make it concise. |
Collaborator
Author
Comment on lines
192
to
200
| useEffect(() => { | ||
| if (pendingSelectedServerIds !== null) { | ||
| const pendingSet = new Set(pendingSelectedServerIds) | ||
| const actualSet = new Set(actualServerIds) | ||
| if (pendingSet.size === actualSet.size && [...pendingSet].every((id) => actualSet.has(id))) { | ||
| setPendingSelectedServerIds(null) | ||
| } | ||
| } | ||
| }, [actualServerIds, pendingSelectedServerIds]) |
Contributor
There was a problem hiding this comment.
The effect that automatically clears pendingSelectedServerIds when it matches actualServerIds can create a race condition during save operations. If actualServerIds updates from the query invalidation (line 397) before all operations complete, this effect could clear pendingSelectedServerIds prematurely, causing the UI to show the wrong state.
Consider moving this logic inside handleSave after all operations complete successfully, or add a flag to prevent this effect from running during active save operations.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
Line: 192:200
Comment:
The effect that automatically clears `pendingSelectedServerIds` when it matches `actualServerIds` can create a race condition during save operations. If `actualServerIds` updates from the query invalidation (line 397) before all operations complete, this effect could clear `pendingSelectedServerIds` prematurely, causing the UI to show the wrong state.
Consider moving this logic inside `handleSave` after all operations complete successfully, or add a flag to prevent this effect from running during active save operations.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| } | ||
|
|
||
| refetchServers() |
Contributor
There was a problem hiding this comment.
refetchServers() is called after all operations complete, but before checking if there were errors. This can cause the actualServerIds to update prematurely, triggering the auto-clear effect (lines 192-200) even when there are errors. This creates inconsistent UI state where pending changes are cleared but errors are shown.
Move refetchServers() inside the if (errors.length > 0) else branch at line 401, so it only refetches when all operations succeed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
Line: 397:397
Comment:
`refetchServers()` is called after all operations complete, but before checking if there were errors. This can cause the `actualServerIds` to update prematurely, triggering the auto-clear effect (lines 192-200) even when there are errors. This creates inconsistent UI state where pending changes are cleared but errors are shown.
Move `refetchServers()` inside the `if (errors.length > 0)` else branch at line 401, so it only refetches when all operations succeed.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
617
to
638
| if (selectedWorkflowIds.length > 0 && server?.id) { | ||
| const workflowErrors: string[] = [] | ||
|
|
||
| for (const workflowId of selectedWorkflowIds) { | ||
| try { | ||
| await addToolMutation.mutateAsync({ | ||
| workspaceId, | ||
| serverId: server.id, | ||
| workflowId, | ||
| }) | ||
| } catch (err) { | ||
| const workflowName = | ||
| deployedWorkflows.find((w) => w.id === workflowId)?.name || workflowId | ||
| workflowErrors.push(workflowName) | ||
| logger.error(`Failed to add workflow ${workflowId} to server:`, err) | ||
| } | ||
| } | ||
|
|
||
| if (workflowErrors.length > 0) { | ||
| setCreateError(`Server created but failed to add workflows: ${workflowErrors.join(', ')}`) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
When adding workflows to a newly created server fails, the function shows an error message but still calls resetForm() at line 640, which clears the error message immediately. This means users won't see the partial failure error.
The error state will be cleared before users can read it. Move resetForm() inside a condition that only executes when there are no workflow errors:
Suggested change
| if (selectedWorkflowIds.length > 0 && server?.id) { | |
| const workflowErrors: string[] = [] | |
| for (const workflowId of selectedWorkflowIds) { | |
| try { | |
| await addToolMutation.mutateAsync({ | |
| workspaceId, | |
| serverId: server.id, | |
| workflowId, | |
| }) | |
| } catch (err) { | |
| const workflowName = | |
| deployedWorkflows.find((w) => w.id === workflowId)?.name || workflowId | |
| workflowErrors.push(workflowName) | |
| logger.error(`Failed to add workflow ${workflowId} to server:`, err) | |
| } | |
| } | |
| if (workflowErrors.length > 0) { | |
| setCreateError(`Server created but failed to add workflows: ${workflowErrors.join(', ')}`) | |
| } | |
| } | |
| if (selectedWorkflowIds.length > 0 && server?.id) { | |
| const workflowErrors: string[] = [] | |
| for (const workflowId of selectedWorkflowIds) { | |
| try { | |
| await addToolMutation.mutateAsync({ | |
| workspaceId, | |
| serverId: server.id, | |
| workflowId, | |
| }) | |
| } catch (err) { | |
| const workflowName = | |
| deployedWorkflows.find((w) => w.id === workflowId)?.name || workflowId | |
| workflowErrors.push(workflowName) | |
| logger.error(`Failed to add workflow ${workflowId} to server:`, err) | |
| } | |
| } | |
| if (workflowErrors.length > 0) { | |
| setCreateError(`Server created but failed to add workflows: ${workflowErrors.join(', ')}`) | |
| return | |
| } | |
| } | |
| resetForm() |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/workflow-mcp-servers/workflow-mcp-servers.tsx
Line: 617:638
Comment:
When adding workflows to a newly created server fails, the function shows an error message but still calls `resetForm()` at line 640, which clears the error message immediately. This means users won't see the partial failure error.
The error state will be cleared before users can read it. Move `resetForm()` inside a condition that only executes when there are no workflow errors:
```suggestion
if (selectedWorkflowIds.length > 0 && server?.id) {
const workflowErrors: string[] = []
for (const workflowId of selectedWorkflowIds) {
try {
await addToolMutation.mutateAsync({
workspaceId,
serverId: server.id,
workflowId,
})
} catch (err) {
const workflowName =
deployedWorkflows.find((w) => w.id === workflowId)?.name || workflowId
workflowErrors.push(workflowName)
logger.error(`Failed to add workflow ${workflowId} to server:`, err)
}
}
if (workflowErrors.length > 0) {
setCreateError(`Server created but failed to add workflows: ${workflowErrors.join(', ')}`)
return
}
}
resetForm()
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
584
to
591
| const invalidWorkflows = useMemo(() => { | ||
| return selectedWorkflowIds | ||
| .map((id) => deployedWorkflows.find((w) => w.id === id)) | ||
| .filter((w) => w && w.hasStartBlock === false) | ||
| .map((w) => w!.name) | ||
| }, [selectedWorkflowIds, deployedWorkflows]) | ||
|
|
||
| const hasInvalidWorkflows = invalidWorkflows.length > 0 |
Contributor
There was a problem hiding this comment.
The invalidWorkflows computation filters workflows with hasStartBlock === false, but hasStartBlock is optional (type is boolean | undefined). When the validation API fails or hasn't loaded yet, hasStartBlock will be undefined, not false.
This means workflows that haven't been validated yet won't be caught as invalid, allowing users to select and try to add workflows that might not have start blocks. Change the filter condition to catch both false and undefined:
Suggested change
| const invalidWorkflows = useMemo(() => { | |
| return selectedWorkflowIds | |
| .map((id) => deployedWorkflows.find((w) => w.id === id)) | |
| .filter((w) => w && w.hasStartBlock === false) | |
| .map((w) => w!.name) | |
| }, [selectedWorkflowIds, deployedWorkflows]) | |
| const hasInvalidWorkflows = invalidWorkflows.length > 0 | |
| const invalidWorkflows = useMemo(() => { | |
| return selectedWorkflowIds | |
| .map((id) => deployedWorkflows.find((w) => w.id === id)) | |
| .filter((w) => w && w.hasStartBlock !== true) | |
| .map((w) => w!.name) | |
| }, [selectedWorkflowIds, deployedWorkflows]) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/workflow-mcp-servers/workflow-mcp-servers.tsx
Line: 584:591
Comment:
The `invalidWorkflows` computation filters workflows with `hasStartBlock === false`, but `hasStartBlock` is optional (type is `boolean | undefined`). When the validation API fails or hasn't loaded yet, `hasStartBlock` will be `undefined`, not `false`.
This means workflows that haven't been validated yet won't be caught as invalid, allowing users to select and try to add workflows that might not have start blocks. Change the filter condition to catch both `false` and `undefined`:
```suggestion
const invalidWorkflows = useMemo(() => {
return selectedWorkflowIds
.map((id) => deployedWorkflows.find((w) => w.id === id))
.filter((w) => w && w.hasStartBlock !== true)
.map((w) => w!.name)
}, [selectedWorkflowIds, deployedWorkflows])
```
How can I resolve this? If you propose a fix, please make it concise.| return availableWorkflows.find((w) => w.id === selectedWorkflowId) | ||
| }, [availableWorkflows, selectedWorkflowId]) | ||
|
|
||
| const selectedWorkflowInvalid = selectedWorkflow && selectedWorkflow.hasStartBlock === false |
Contributor
There was a problem hiding this comment.
Same issue as with the create form - this check only catches workflows with hasStartBlock === false, but not undefined. When validation fails or hasn't completed, hasStartBlock will be undefined, allowing invalid workflows to pass validation.
Suggested change
| const selectedWorkflowInvalid = selectedWorkflow && selectedWorkflow.hasStartBlock === false | |
| const selectedWorkflowInvalid = selectedWorkflow && selectedWorkflow.hasStartBlock !== true |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/workflow-mcp-servers/workflow-mcp-servers.tsx
Line: 207:207
Comment:
Same issue as with the create form - this check only catches workflows with `hasStartBlock === false`, but not `undefined`. When validation fails or hasn't completed, `hasStartBlock` will be `undefined`, allowing invalid workflows to pass validation.
```suggestion
const selectedWorkflowInvalid = selectedWorkflow && selectedWorkflow.hasStartBlock !== true
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+448
to
+463
| let startBlockMap: Record<string, boolean> = {} | ||
| if (deployedWorkflows.length > 0) { | ||
| try { | ||
| const validateResponse = await fetch('/api/mcp/workflow-servers/validate', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ workflowIds: deployedWorkflows.map((w) => w.id) }), | ||
| }) | ||
| if (validateResponse.ok) { | ||
| const validateData = await validateResponse.json() | ||
| startBlockMap = validateData.data || {} | ||
| } | ||
| } catch (error) { | ||
| logger.warn('Failed to validate workflows for MCP:', error) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
When the validation API call fails (lines 450-462), the code catches the error and logs a warning, then continues with an empty startBlockMap. This means all workflows will have hasStartBlock: false (line 470) when validation fails, which is incorrect - they should probably be marked as undefined to indicate unknown status.
This causes the UI to incorrectly block valid workflows from being added when the validation endpoint is temporarily unavailable. Change line 470 to use undefined for failed validations:
Suggested change
| let startBlockMap: Record<string, boolean> = {} | |
| if (deployedWorkflows.length > 0) { | |
| try { | |
| const validateResponse = await fetch('/api/mcp/workflow-servers/validate', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ workflowIds: deployedWorkflows.map((w) => w.id) }), | |
| }) | |
| if (validateResponse.ok) { | |
| const validateData = await validateResponse.json() | |
| startBlockMap = validateData.data || {} | |
| } | |
| } catch (error) { | |
| logger.warn('Failed to validate workflows for MCP:', error) | |
| } | |
| } | |
| return deployedWorkflows.map((w) => ({ | |
| id: w.id, | |
| name: w.name, | |
| description: w.description, | |
| isDeployed: w.isDeployed, | |
| hasStartBlock: w.id in startBlockMap ? startBlockMap[w.id] : undefined, | |
| })) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/hooks/queries/workflow-mcp-servers.ts
Line: 448:463
Comment:
When the validation API call fails (lines 450-462), the code catches the error and logs a warning, then continues with an empty `startBlockMap`. This means all workflows will have `hasStartBlock: false` (line 470) when validation fails, which is incorrect - they should probably be marked as `undefined` to indicate unknown status.
This causes the UI to incorrectly block valid workflows from being added when the validation endpoint is temporarily unavailable. Change line 470 to use `undefined` for failed validations:
```suggestion
return deployedWorkflows.map((w) => ({
id: w.id,
name: w.name,
description: w.description,
isDeployed: w.isDeployed,
hasStartBlock: w.id in startBlockMap ? startBlockMap[w.id] : undefined,
}))
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+351
to
+355
| setServerToolsMap((prev) => { | ||
| const next = { ...prev } | ||
| delete next[serverId] | ||
| return next | ||
| }) |
Contributor
There was a problem hiding this comment.
Manually updating serverToolsMap after a successful delete can cause state inconsistencies. The ServerToolsQuery components (lines 516-524) are continuously querying for tools and updating this same map via handleServerToolData. This creates a race condition where:
- Delete succeeds, local state updated to remove the tool
- Query refetch happens (line 397)
- Query results come back and
ServerToolsQueryupdates the map again - If the backend hasn't fully processed the delete yet, the tool might reappear
The manual state update is unnecessary since refetchServers() (line 397) will trigger the queries to update naturally. Remove this manual update and rely on the query invalidation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
Line: 351:355
Comment:
Manually updating `serverToolsMap` after a successful delete can cause state inconsistencies. The `ServerToolsQuery` components (lines 516-524) are continuously querying for tools and updating this same map via `handleServerToolData`. This creates a race condition where:
1. Delete succeeds, local state updated to remove the tool
2. Query refetch happens (line 397)
3. Query results come back and `ServerToolsQuery` updates the map again
4. If the backend hasn't fully processed the delete yet, the tool might reappear
The manual state update is unnecessary since `refetchServers()` (line 397) will trigger the queries to update naturally. Remove this manual update and rely on the query invalidation.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+65
to
+67
| const selectedWorkflows = useMemo(() => { | ||
| return workflows.filter((w) => selectedIds.includes(w.id)) | ||
| }, [workflows, selectedIds]) |
Contributor
There was a problem hiding this comment.
If a workflow is selected (in selectedIds) but then gets undeployed or removed from the workflows array, selectedWorkflows will not include it, but selectedIds will still contain its ID. This creates a mismatch between the badges shown and the actual selection state.
While this is an edge case, it could confuse users who see fewer badges than expected. Consider adding logic to clean up selectedIds when workflows are removed from the available list, or show a warning badge for missing workflows.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/workflow-mcp-servers/workflow-mcp-servers.tsx
Line: 65:67
Comment:
If a workflow is selected (in `selectedIds`) but then gets undeployed or removed from the `workflows` array, `selectedWorkflows` will not include it, but `selectedIds` will still contain its ID. This creates a mismatch between the badges shown and the actual selection state.
While this is an edge case, it could confuse users who see fewer badges than expected. Consider adding logic to clean up `selectedIds` when workflows are removed from the available list, or show a warning badge for missing workflows.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+39
to
+45
| interface WorkflowTagSelectProps { | ||
| workflows: { id: string; name: string }[] | ||
| selectedIds: string[] | ||
| onSelectionChange: (ids: string[]) => void | ||
| isLoading?: boolean | ||
| disabled?: boolean | ||
| } |
Contributor
There was a problem hiding this comment.
The WorkflowTagSelectProps interface expects workflows to have only id and name, but it's being passed DeployedWorkflow[] which has additional fields including hasStartBlock. This works due to TypeScript's structural typing, but creates a type mismatch.
The component doesn't use hasStartBlock, but the parent component (WorkflowMcpServers) needs it for validation. Consider either:
- Updating the interface to accept
DeployedWorkflow[]directly - Or keeping it generic but documenting why the narrower type is used
This is a minor style issue but improves type clarity.
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/components/sidebar/components/settings-modal/components/workflow-mcp-servers/workflow-mcp-servers.tsx
Line: 39:45
Comment:
The `WorkflowTagSelectProps` interface expects `workflows` to have only `id` and `name`, but it's being passed `DeployedWorkflow[]` which has additional fields including `hasStartBlock`. This works due to TypeScript's structural typing, but creates a type mismatch.
The component doesn't use `hasStartBlock`, but the parent component (`WorkflowMcpServers`) needs it for validation. Consider either:
1. Updating the interface to accept `DeployedWorkflow[]` directly
2. Or keeping it generic but documenting why the narrower type is used
This is a minor style issue but improves type clarity.
<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.
Collaborator
Author
Comment on lines
+20
to
+24
| const body = await request.json() | ||
| const { workflowIds } = body | ||
|
|
||
| if (!Array.isArray(workflowIds) || workflowIds.length === 0) { | ||
| return NextResponse.json({ error: 'workflowIds must be a non-empty array' }, { status: 400 }) |
Contributor
There was a problem hiding this comment.
The request body parsing lacks type validation for workflowIds. If a malicious or buggy client sends workflowIds as a non-array value (e.g., {"workflowIds": "string"} or {"workflowIds": null}), the Array.isArray() check on line 23 will catch it, but there's no validation for:
- Maximum array size: A client could send thousands of workflow IDs, causing performance issues or timeouts
- Array element types: The code doesn't validate that each element in the array is a string
Recommendation: Add validation for array size and element types:
if (!Array.isArray(workflowIds) || workflowIds.length === 0) {
return NextResponse.json({ error: 'workflowIds must be a non-empty array' }, { status: 400 })
}
if (workflowIds.length > 100) {
return NextResponse.json({ error: 'Maximum 100 workflow IDs allowed' }, { status: 400 })
}
if (!workflowIds.every(id => typeof id === 'string')) {
return NextResponse.json({ error: 'All workflow IDs must be strings' }, { status: 400 })
}Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/api/mcp/workflow-servers/validate/route.ts
Line: 20:24
Comment:
The request body parsing lacks type validation for `workflowIds`. If a malicious or buggy client sends `workflowIds` as a non-array value (e.g., `{"workflowIds": "string"}` or `{"workflowIds": null}`), the `Array.isArray()` check on line 23 will catch it, but there's no validation for:
1. **Maximum array size**: A client could send thousands of workflow IDs, causing performance issues or timeouts
2. **Array element types**: The code doesn't validate that each element in the array is a string
**Recommendation**: Add validation for array size and element types:
```typescript
if (!Array.isArray(workflowIds) || workflowIds.length === 0) {
return NextResponse.json({ error: 'workflowIds must be a non-empty array' }, { status: 400 })
}
if (workflowIds.length > 100) {
return NextResponse.json({ error: 'Maximum 100 workflow IDs allowed' }, { status: 400 })
}
if (!workflowIds.every(id => typeof id === 'string')) {
return NextResponse.json({ error: 'All workflow IDs must be strings' }, { status: 400 })
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+400
to
+414
| if (errors.length > 0) { | ||
| setSaveError(errors.join('. ')) | ||
| } else { | ||
| refetchServers() | ||
| setPendingSelectedServerIds(null) | ||
| setSavedValues({ | ||
| toolName, | ||
| toolDescription, | ||
| parameterDescriptions: { ...parameterDescriptions }, | ||
| }) | ||
| onCanSaveChange?.(false) | ||
| onSubmittingChange?.(false) | ||
| } catch (error) { | ||
| logger.error('Failed to save tool configuration:', error) | ||
| onSubmittingChange?.(false) | ||
| } | ||
|
|
||
| isSavingRef.current = false | ||
| onSubmittingChange?.(false) |
Contributor
There was a problem hiding this comment.
Critical issue: When partial failures occur during the save operation, the success indicators are still triggered even though some operations failed. This creates a misleading user experience.
The Problem:
- If adding to servers 1, 2, and 3, and server 2 fails, the code still executes:
refetchServers()(line 403)setPendingSelectedServerIds(null)(line 404) - clears pending statesetSavedValues(...)(line 405-409) - marks values as savedonCanSaveChange?.(false)(line 410) - disables save button
Impact:
- User sees
saveErrormessage but the UI behaves as if everything succeeded - The pending state is cleared, hiding which operations failed
- The save button is disabled, preventing immediate retry
refetchServers()may show the tool was added to servers 1 and 3, but the user has no clear indication that server 2 failed
Solution:
Only execute success actions when errors.length === 0:
if (errors.length > 0) {
setSaveError(errors.join('. '))
// Don't clear pending state or mark as saved on partial failure
} else {
refetchServers()
setPendingSelectedServerIds(null)
setSavedValues({
toolName,
toolDescription,
parameterDescriptions: { ...parameterDescriptions },
})
onCanSaveChange?.(false)
}This way, users can see what failed, keep the pending changes visible, and retry the save operation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
Line: 400:414
Comment:
Critical issue: When partial failures occur during the save operation, the success indicators are still triggered even though some operations failed. This creates a misleading user experience.
**The Problem:**
- If adding to servers 1, 2, and 3, and server 2 fails, the code still executes:
- `refetchServers()` (line 403)
- `setPendingSelectedServerIds(null)` (line 404) - clears pending state
- `setSavedValues(...)` (line 405-409) - marks values as saved
- `onCanSaveChange?.(false)` (line 410) - disables save button
**Impact:**
1. User sees `saveError` message but the UI behaves as if everything succeeded
2. The pending state is cleared, hiding which operations failed
3. The save button is disabled, preventing immediate retry
4. `refetchServers()` may show the tool was added to servers 1 and 3, but the user has no clear indication that server 2 failed
**Solution:**
Only execute success actions when `errors.length === 0`:
```typescript
if (errors.length > 0) {
setSaveError(errors.join('. '))
// Don't clear pending state or mark as saved on partial failure
} else {
refetchServers()
setPendingSelectedServerIds(null)
setSavedValues({
toolName,
toolDescription,
parameterDescriptions: { ...parameterDescriptions },
})
onCanSaveChange?.(false)
}
```
This way, users can see what failed, keep the pending changes visible, and retry the save operation.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+602
to
+609
| const invalidWorkflows = useMemo(() => { | ||
| return selectedWorkflowIds | ||
| .map((id) => deployedWorkflows.find((w) => w.id === id)) | ||
| .filter((w) => w && w.hasStartBlock !== true) | ||
| .map((w) => w!.name) | ||
| }, [selectedWorkflowIds, deployedWorkflows]) | ||
|
|
||
| const hasInvalidWorkflows = invalidWorkflows.length > 0 |
Contributor
There was a problem hiding this comment.
The validation logic for hasStartBlock doesn't distinguish between false (explicitly invalid) and undefined (validation failed or not yet loaded). This can lead to incorrect behavior:
Current behavior:
w.hasStartBlock !== truetreats bothfalseandundefinedas invalid- If the validation API fails in
useDeployedWorkflows, all workflows gethasStartBlock: undefined - These workflows are then incorrectly flagged as invalid
Issue:
Line 605 filters workflows where hasStartBlock !== true, which includes:
- Workflows that legitimately lack a start block (
hasStartBlock === false) - Workflows where validation failed or hasn't loaded yet (
hasStartBlock === undefined)
Recommendation:
Be explicit about what constitutes an invalid workflow:
const invalidWorkflows = useMemo(() => {
return selectedWorkflowIds
.map((id) => deployedWorkflows.find((w) => w.id === id))
.filter((w) => w && w.hasStartBlock === false) // Only explicitly false, not undefined
.map((w) => w!.name)
}, [selectedWorkflowIds, deployedWorkflows])Additionally, consider showing a loading state or warning when hasStartBlock is undefined to inform users that validation is pending or failed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/workflow-mcp-servers/workflow-mcp-servers.tsx
Line: 602:609
Comment:
The validation logic for `hasStartBlock` doesn't distinguish between `false` (explicitly invalid) and `undefined` (validation failed or not yet loaded). This can lead to incorrect behavior:
**Current behavior:**
- `w.hasStartBlock !== true` treats both `false` and `undefined` as invalid
- If the validation API fails in `useDeployedWorkflows`, all workflows get `hasStartBlock: undefined`
- These workflows are then incorrectly flagged as invalid
**Issue:**
Line 605 filters workflows where `hasStartBlock !== true`, which includes:
- Workflows that legitimately lack a start block (`hasStartBlock === false`)
- Workflows where validation failed or hasn't loaded yet (`hasStartBlock === undefined`)
**Recommendation:**
Be explicit about what constitutes an invalid workflow:
```typescript
const invalidWorkflows = useMemo(() => {
return selectedWorkflowIds
.map((id) => deployedWorkflows.find((w) => w.id === id))
.filter((w) => w && w.hasStartBlock === false) // Only explicitly false, not undefined
.map((w) => w!.name)
}, [selectedWorkflowIds, deployedWorkflows])
```
Additionally, consider showing a loading state or warning when `hasStartBlock` is `undefined` to inform users that validation is pending or failed.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+635
to
657
| if (selectedWorkflowIds.length > 0 && server?.id) { | ||
| const workflowErrors: string[] = [] | ||
|
|
||
| for (const workflowId of selectedWorkflowIds) { | ||
| try { | ||
| await addToolMutation.mutateAsync({ | ||
| workspaceId, | ||
| serverId: server.id, | ||
| workflowId, | ||
| }) | ||
| } catch (err) { | ||
| const workflowName = | ||
| deployedWorkflows.find((w) => w.id === workflowId)?.name || workflowId | ||
| workflowErrors.push(workflowName) | ||
| logger.error(`Failed to add workflow ${workflowId} to server:`, err) | ||
| } | ||
| } | ||
|
|
||
| if (workflowErrors.length > 0) { | ||
| setCreateError(`Server created but failed to add workflows: ${workflowErrors.join(', ')}`) | ||
| return | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Partial failure handling during server creation can leave the UI in an inconsistent state. When workflows fail to be added after the server is successfully created:
Current behavior:
- Server is created successfully (line 625-633)
- Workflow additions may fail (lines 638-650)
- Error is set but
resetForm()is NOT called (line 654) - The form remains visible with the error message
Issues:
- The successfully created server is not visible to the user because the form is still open
- The form still shows the failed workflows in
selectedWorkflowIds - Clicking "Add Server" again would attempt to create another server with the same name (likely failing with a duplicate name error)
- User has no clear path to:
- View the successfully created server
- Retry adding the failed workflows to the existing server
- Start fresh with a new server
Recommendation:
After a partial failure, either:
- Reset the form and let the user navigate to the server details to retry adding workflows:
if (workflowErrors.length > 0) {
const errorMsg = `Server created but failed to add workflows: ${workflowErrors.join(', ')}. You can add them manually from the server details.`
// Show error in a toast/notification instead of form
resetForm()
// Optionally navigate to the server details
setSelectedServerId(server.id)
}OR
- Keep the form open but update it to "edit mode" for the existing server:
if (workflowErrors.length > 0) {
setCreateError(`Server created but failed to add workflows: ${workflowErrors.join(', ')}. Please try again.`)
// Update form to reference the existing server
setFormData({ name: server.name })
// Remove successfully added workflows from selection
const failedWorkflowIds = selectedWorkflowIds.filter(id =>
workflowErrors.includes(deployedWorkflows.find(w => w.id === id)?.name || '')
)
setSelectedWorkflowIds(failedWorkflowIds)
return // Don't call resetForm
}Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/workflow-mcp-servers/workflow-mcp-servers.tsx
Line: 635:657
Comment:
Partial failure handling during server creation can leave the UI in an inconsistent state. When workflows fail to be added after the server is successfully created:
**Current behavior:**
- Server is created successfully (line 625-633)
- Workflow additions may fail (lines 638-650)
- Error is set but `resetForm()` is NOT called (line 654)
- The form remains visible with the error message
**Issues:**
1. The successfully created server is not visible to the user because the form is still open
2. The form still shows the failed workflows in `selectedWorkflowIds`
3. Clicking "Add Server" again would attempt to create another server with the same name (likely failing with a duplicate name error)
4. User has no clear path to:
- View the successfully created server
- Retry adding the failed workflows to the existing server
- Start fresh with a new server
**Recommendation:**
After a partial failure, either:
1. Reset the form and let the user navigate to the server details to retry adding workflows:
```typescript
if (workflowErrors.length > 0) {
const errorMsg = `Server created but failed to add workflows: ${workflowErrors.join(', ')}. You can add them manually from the server details.`
// Show error in a toast/notification instead of form
resetForm()
// Optionally navigate to the server details
setSelectedServerId(server.id)
}
```
OR
2. Keep the form open but update it to "edit mode" for the existing server:
```typescript
if (workflowErrors.length > 0) {
setCreateError(`Server created but failed to add workflows: ${workflowErrors.join(', ')}. Please try again.`)
// Update form to reference the existing server
setFormData({ name: server.name })
// Remove successfully added workflows from selection
const failedWorkflowIds = selectedWorkflowIds.filter(id =>
workflowErrors.includes(deployedWorkflows.find(w => w.id === id)?.name || '')
)
setSelectedWorkflowIds(failedWorkflowIds)
return // Don't call resetForm
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+448
to
+471
| let startBlockMap: Record<string, boolean> = {} | ||
| if (deployedWorkflows.length > 0) { | ||
| try { | ||
| const validateResponse = await fetch('/api/mcp/workflow-servers/validate', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ workflowIds: deployedWorkflows.map((w) => w.id) }), | ||
| }) | ||
| if (validateResponse.ok) { | ||
| const validateData = await validateResponse.json() | ||
| startBlockMap = validateData.data || {} | ||
| } | ||
| } catch (error) { | ||
| logger.warn('Failed to validate workflows for MCP:', error) | ||
| } | ||
| } | ||
|
|
||
| return deployedWorkflows.map((w) => ({ | ||
| id: w.id, | ||
| name: w.name, | ||
| description: w.description, | ||
| isDeployed: w.isDeployed, | ||
| hasStartBlock: startBlockMap[w.id], | ||
| })) |
Contributor
There was a problem hiding this comment.
Silent validation failure leaves hasStartBlock as undefined for all workflows when the validation API fails. This causes downstream issues in components that check hasStartBlock !== true.
The Problem:
- If the validation API call fails (line 451-462 catch block),
startBlockMapremains an empty object{} - Line 470 then sets
hasStartBlock: startBlockMap[w.id]which evaluates toundefinedfor all workflows - Components treating
undefinedthe same asfalsewill incorrectly flag all workflows as invalid
Impact:
When validation fails:
- All deployed workflows appear as invalid in the UI
- Users cannot add any workflows to MCP servers
- The only indication is a warning in the console (line 461)
Recommendation:
- Make validation failures more explicit:
let startBlockMap: Record<string, boolean> = {}
let validationFailed = false
if (deployedWorkflows.length > 0) {
try {
const validateResponse = await fetch('/api/mcp/workflow-servers/validate', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ workflowIds: deployedWorkflows.map((w) => w.id) }),
})
if (validateResponse.ok) {
const validateData = await validateResponse.json()
startBlockMap = validateData.data || {}
} else {
validationFailed = true
}
} catch (error) {
logger.warn('Failed to validate workflows for MCP:', error)
validationFailed = true
}
}
return deployedWorkflows.map((w) => ({
id: w.id,
name: w.name,
description: w.description,
isDeployed: w.isDeployed,
hasStartBlock: validationFailed ? undefined : startBlockMap[w.id],
}))- Update components to handle
undefinedgracefully (show loading/warning state) - Consider retrying the validation or allowing users to manually refresh
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/hooks/queries/workflow-mcp-servers.ts
Line: 448:471
Comment:
Silent validation failure leaves `hasStartBlock` as `undefined` for all workflows when the validation API fails. This causes downstream issues in components that check `hasStartBlock !== true`.
**The Problem:**
- If the validation API call fails (line 451-462 catch block), `startBlockMap` remains an empty object `{}`
- Line 470 then sets `hasStartBlock: startBlockMap[w.id]` which evaluates to `undefined` for all workflows
- Components treating `undefined` the same as `false` will incorrectly flag all workflows as invalid
**Impact:**
When validation fails:
1. All deployed workflows appear as invalid in the UI
2. Users cannot add any workflows to MCP servers
3. The only indication is a warning in the console (line 461)
**Recommendation:**
1. Make validation failures more explicit:
```typescript
let startBlockMap: Record<string, boolean> = {}
let validationFailed = false
if (deployedWorkflows.length > 0) {
try {
const validateResponse = await fetch('/api/mcp/workflow-servers/validate', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ workflowIds: deployedWorkflows.map((w) => w.id) }),
})
if (validateResponse.ok) {
const validateData = await validateResponse.json()
startBlockMap = validateData.data || {}
} else {
validationFailed = true
}
} catch (error) {
logger.warn('Failed to validate workflows for MCP:', error)
validationFailed = true
}
}
return deployedWorkflows.map((w) => ({
id: w.id,
name: w.name,
description: w.description,
isDeployed: w.isDeployed,
hasStartBlock: validationFailed ? undefined : startBlockMap[w.id],
}))
```
2. Update components to handle `undefined` gracefully (show loading/warning state)
3. Consider retrying the validation or allowing users to manually refresh
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+193
to
+202
| useEffect(() => { | ||
| if (isSavingRef.current) return | ||
| if (pendingSelectedServerIds !== null) { | ||
| const pendingSet = new Set(pendingSelectedServerIds) | ||
| const actualSet = new Set(actualServerIds) | ||
| if (pendingSet.size === actualSet.size && [...pendingSet].every((id) => actualSet.has(id))) { | ||
| setPendingSelectedServerIds(null) | ||
| } | ||
| } | ||
| }, [actualServerIds, pendingSelectedServerIds]) |
Contributor
There was a problem hiding this comment.
Race condition: The effect that clears pendingSelectedServerIds can incorrectly reset user changes when data loads after the user has made selections.
Scenario:
- Component mounts,
actualServerIdsis empty (data still loading) - User immediately selects servers →
pendingSelectedServerIds = ['server1', 'server2'] - Data loads →
actualServerIds = [](no tools deployed yet) - Effect on line 193 runs: compares
['server1', 'server2']with[] - They don't match, so pending state is NOT cleared
- User clicks Save → operations complete
actualServerIdsupdates to['server1', 'server2']- Effect runs again and clears
pendingSelectedServerIds→ CORRECT
However, consider this scenario:
- Workflow already has tools on
['server1'] actualServerIds = ['server1']after initial load- User changes selection to
['server2']→pendingSelectedServerIds = ['server2'] - Background refetch occurs (from another component) →
actualServerIdsupdates to['server1']again - Effect compares
['server2']with['server1'], they don't match → pending state preserved (CORRECT) - But if
isSavingRef.currentis false when this happens, and the user hasn't saved yet, this is working correctly
Actually, on closer inspection, the logic appears sound. The guard if (isSavingRef.current) return prevents clearing during save operations, and the equality check ensures pending state is only cleared when it matches actual state. The comment should be withdrawn.
Retraction: After deeper analysis, this logic is actually correct. The effect appropriately handles the synchronization between pending and actual state.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
Line: 193:202
Comment:
Race condition: The effect that clears `pendingSelectedServerIds` can incorrectly reset user changes when data loads after the user has made selections.
**Scenario:**
1. Component mounts, `actualServerIds` is empty (data still loading)
2. User immediately selects servers → `pendingSelectedServerIds = ['server1', 'server2']`
3. Data loads → `actualServerIds = []` (no tools deployed yet)
4. Effect on line 193 runs: compares `['server1', 'server2']` with `[]`
5. They don't match, so pending state is NOT cleared
6. User clicks Save → operations complete
7. `actualServerIds` updates to `['server1', 'server2']`
8. Effect runs again and clears `pendingSelectedServerIds` → CORRECT
However, consider this scenario:
1. Workflow already has tools on `['server1']`
2. `actualServerIds = ['server1']` after initial load
3. User changes selection to `['server2']` → `pendingSelectedServerIds = ['server2']`
4. Background refetch occurs (from another component) → `actualServerIds` updates to `['server1']` again
5. Effect compares `['server2']` with `['server1']`, they don't match → pending state preserved (CORRECT)
6. But if `isSavingRef.current` is false when this happens, and the user hasn't saved yet, this is working correctly
Actually, on closer inspection, the logic appears sound. The guard `if (isSavingRef.current) return` prevents clearing during save operations, and the equality check ensures pending state is only cleared when it matches actual state. The comment should be withdrawn.
**Retraction:** After deeper analysis, this logic is actually correct. The effect appropriately handles the synchronization between pending and actual state.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+344
to
+370
| for (const serverId of toRemove) { | ||
| const toolInfo = serverToolsMap[serverId] | ||
| if (toolInfo?.tool) { | ||
| setPendingServerChanges((prev) => new Set(prev).add(serverId)) | ||
| try { | ||
| await deleteToolMutation.mutateAsync({ | ||
| workspaceId, | ||
| serverId, | ||
| toolId: toolInfo.tool.id, | ||
| }) | ||
| setServerToolsMap((prev) => { | ||
| const next = { ...prev } | ||
| delete next[serverId] | ||
| return next | ||
| }) | ||
| } catch (error) { | ||
| const serverName = servers.find((s) => s.id === serverId)?.name || serverId | ||
| errors.push(`Failed to remove from "${serverName}"`) | ||
| logger.error(`Failed to remove tool from server ${serverId}:`, error) | ||
| } finally { | ||
| setPendingServerChanges((prev) => { | ||
| const next = new Set(prev) | ||
| next.delete(serverId) | ||
| return next | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The toRemove loop has a potential issue: it manually updates serverToolsMap state after deletion, but this update may not reflect the actual deletion result if the API call succeeded but the local state update is based on stale data.
Issue:
Lines 354-358 manually delete from serverToolsMap:
setServerToolsMap((prev) => {
const next = { ...prev }
delete next[serverId]
return next
})This happens inside the try block, meaning it executes even if deleteToolMutation.mutateAsync() succeeds. However:
- The deletion mutation already invalidates queries (see
workflow-mcp-servers.tslines 420-429), which will trigger a refetch - This manual state update could race with the query invalidation refetch
- If the refetch completes before this manual update, the manual update could overwrite fresh data with stale data
Impact:
- Could cause UI inconsistencies where tools appear to be deleted but then reappear
- The
refetchServers()on line 403 should handle updating this data anyway
Recommendation:
Remove the manual state update and rely on the query invalidation:
for (const serverId of toRemove) {
const toolInfo = serverToolsMap[serverId]
if (toolInfo?.tool) {
setPendingServerChanges((prev) => new Set(prev).add(serverId))
try {
await deleteToolMutation.mutateAsync({
workspaceId,
serverId,
toolId: toolInfo.tool.id,
})
// Remove manual state update - let query invalidation handle it
} catch (error) {
const serverName = servers.find((s) => s.id === serverId)?.name || serverId
errors.push(`Failed to remove from "${serverName}"`)
logger.error(`Failed to remove tool from server ${serverId}:`, error)
} finally {
setPendingServerChanges((prev) => {
const next = new Set(prev)
next.delete(serverId)
return next
})
}
}
}The ServerToolsQuery components will automatically update serverToolsMap when the queries refetch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
Line: 344:370
Comment:
The `toRemove` loop has a potential issue: it manually updates `serverToolsMap` state after deletion, but this update may not reflect the actual deletion result if the API call succeeded but the local state update is based on stale data.
**Issue:**
Lines 354-358 manually delete from `serverToolsMap`:
```typescript
setServerToolsMap((prev) => {
const next = { ...prev }
delete next[serverId]
return next
})
```
This happens inside the try block, meaning it executes even if `deleteToolMutation.mutateAsync()` succeeds. However:
1. The deletion mutation already invalidates queries (see `workflow-mcp-servers.ts` lines 420-429), which will trigger a refetch
2. This manual state update could race with the query invalidation refetch
3. If the refetch completes before this manual update, the manual update could overwrite fresh data with stale data
**Impact:**
- Could cause UI inconsistencies where tools appear to be deleted but then reappear
- The `refetchServers()` on line 403 should handle updating this data anyway
**Recommendation:**
Remove the manual state update and rely on the query invalidation:
```typescript
for (const serverId of toRemove) {
const toolInfo = serverToolsMap[serverId]
if (toolInfo?.tool) {
setPendingServerChanges((prev) => new Set(prev).add(serverId))
try {
await deleteToolMutation.mutateAsync({
workspaceId,
serverId,
toolId: toolInfo.tool.id,
})
// Remove manual state update - let query invalidation handle it
} catch (error) {
const serverName = servers.find((s) => s.id === serverId)?.name || serverId
errors.push(`Failed to remove from "${serverName}"`)
logger.error(`Failed to remove tool from server ${serverId}:`, error)
} finally {
setPendingServerChanges((prev) => {
const next = new Set(prev)
next.delete(serverId)
return next
})
}
}
}
```
The `ServerToolsQuery` components will automatically update `serverToolsMap` when the queries refetch.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+29
to
+36
| for (const workflowId of workflowIds) { | ||
| try { | ||
| const state = await loadWorkflowFromNormalizedTables(workflowId) | ||
| results[workflowId] = hasValidStartBlockInState(state) | ||
| } catch (error) { | ||
| logger.warn(`Failed to validate workflow ${workflowId}:`, error) | ||
| results[workflowId] = false | ||
| } |
Contributor
There was a problem hiding this comment.
Security vulnerability: The validation loop doesn't verify that the workflows belong to the authenticated user's workspace. A malicious user could validate workflows from other workspaces by sending arbitrary workflow IDs.
The vulnerability:
loadWorkflowFromNormalizedTables(workflowId)(line 31) loads workflow data without any workspace authorization check- An attacker could send workflow IDs from other users' workspaces
- While this only returns boolean validation results (not sensitive data), it still:
- Allows enumeration of workflow IDs across workspaces
- Leaks information about workflow structure (whether they have start blocks)
- Could enable timing attacks to infer workflow complexity
Fix:
Add workspace validation before checking each workflow:
for (const workflowId of workflowIds) {
try {
// Verify workflow belongs to user's workspace
const [workflowRecord] = await db
.select({ id: workflow.id, workspaceId: workflow.workspaceId })
.from(workflow)
.where(eq(workflow.id, workflowId))
.limit(1)
if (!workflowRecord || workflowRecord.workspaceId !== session.user.workspaceId) {
logger.warn(`Unauthorized workflow access attempt: ${workflowId}`)
results[workflowId] = false
continue
}
const state = await loadWorkflowFromNormalizedTables(workflowId)
results[workflowId] = hasValidStartBlockInState(state)
} catch (error) {
logger.warn(`Failed to validate workflow ${workflowId}:`, error)
results[workflowId] = false
}
}Note: You'll need to verify the session object structure to get the correct workspace ID (might be session.user.activeWorkspaceId or require a separate query).
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/api/mcp/workflow-servers/validate/route.ts
Line: 29:36
Comment:
Security vulnerability: The validation loop doesn't verify that the workflows belong to the authenticated user's workspace. A malicious user could validate workflows from other workspaces by sending arbitrary workflow IDs.
**The vulnerability:**
- `loadWorkflowFromNormalizedTables(workflowId)` (line 31) loads workflow data without any workspace authorization check
- An attacker could send workflow IDs from other users' workspaces
- While this only returns boolean validation results (not sensitive data), it still:
1. Allows enumeration of workflow IDs across workspaces
2. Leaks information about workflow structure (whether they have start blocks)
3. Could enable timing attacks to infer workflow complexity
**Fix:**
Add workspace validation before checking each workflow:
```typescript
for (const workflowId of workflowIds) {
try {
// Verify workflow belongs to user's workspace
const [workflowRecord] = await db
.select({ id: workflow.id, workspaceId: workflow.workspaceId })
.from(workflow)
.where(eq(workflow.id, workflowId))
.limit(1)
if (!workflowRecord || workflowRecord.workspaceId !== session.user.workspaceId) {
logger.warn(`Unauthorized workflow access attempt: ${workflowId}`)
results[workflowId] = false
continue
}
const state = await loadWorkflowFromNormalizedTables(workflowId)
results[workflowId] = hasValidStartBlockInState(state)
} catch (error) {
logger.warn(`Failed to validate workflow ${workflowId}:`, error)
results[workflowId] = false
}
}
```
Note: You'll need to verify the session object structure to get the correct workspace ID (might be `session.user.activeWorkspaceId` or require a separate query).
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+69
to
+72
| const validSelectedIds = useMemo(() => { | ||
| const workflowIds = new Set(workflows.map((w) => w.id)) | ||
| return selectedIds.filter((id) => workflowIds.has(id)) | ||
| }, [workflows, selectedIds]) |
Contributor
There was a problem hiding this comment.
The validSelectedIds filtering could cause silent data loss if workflows are removed from the workflows list while still selected.
Scenario:
- User selects workflows A, B, C →
selectedIds = ['A', 'B', 'C'] - Workflow B is deleted/undeployed by another user or in another tab
workflowslist updates to only contain A and CvalidSelectedIdsfilters to['A', 'C'](line 71)- This filtered list is passed to Combobox's
multiSelectValues(line 117) - User makes no changes and clicks "Add Server"
- Only workflows A and C are added - B is silently dropped
Issue:
The user never receives feedback that workflow B was removed from their selection. They might expect all three workflows to be added.
Recommendation:
Detect and notify when selected workflows become invalid:
useEffect(() => {
const invalidSelections = selectedIds.filter(
id => !workflows.some(w => w.id === id)
)
if (invalidSelections.length > 0) {
// Notify user and clean up selection
const invalidNames = invalidSelections.map(id => {
// Try to get name from previous state or use ID
return id
}).join(', ')
console.warn(`Some selected workflows are no longer available: ${invalidNames}`)
onSelectionChange(validSelectedIds)
}
}, [workflows, selectedIds])Or show a warning badge in the UI when validSelectedIds.length < selectedIds.length.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/settings-modal/components/workflow-mcp-servers/workflow-mcp-servers.tsx
Line: 69:72
Comment:
The `validSelectedIds` filtering could cause silent data loss if workflows are removed from the `workflows` list while still selected.
**Scenario:**
1. User selects workflows A, B, C → `selectedIds = ['A', 'B', 'C']`
2. Workflow B is deleted/undeployed by another user or in another tab
3. `workflows` list updates to only contain A and C
4. `validSelectedIds` filters to `['A', 'C']` (line 71)
5. This filtered list is passed to Combobox's `multiSelectValues` (line 117)
6. User makes no changes and clicks "Add Server"
7. Only workflows A and C are added - B is silently dropped
**Issue:**
The user never receives feedback that workflow B was removed from their selection. They might expect all three workflows to be added.
**Recommendation:**
Detect and notify when selected workflows become invalid:
```typescript
useEffect(() => {
const invalidSelections = selectedIds.filter(
id => !workflows.some(w => w.id === id)
)
if (invalidSelections.length > 0) {
// Notify user and clean up selection
const invalidNames = invalidSelections.map(id => {
// Try to get name from previous state or use ID
return id
}).join(', ')
console.warn(`Some selected workflows are no longer available: ${invalidNames}`)
onSelectionChange(validSelectedIds)
}
}, [workflows, selectedIds])
```
Or show a warning badge in the UI when `validSelectedIds.length < selectedIds.length`.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Additional Comments (2)
Potential issue: Current code: return generateToolInputSchema(fieldsWithDescriptions) as unknown as Record<string, unknown>Recommendation: function generateParameterSchema(
inputFormat: NormalizedField[],
descriptions: Record<string, string>
): Record<string, unknown> {
const fieldsWithDescriptions = inputFormat.map((field) => ({
...field,
description: descriptions[field.name]?.trim() || undefined,
}))
const schema = generateToolInputSchema(fieldsWithDescriptions)
// Validate schema is an object
if (!schema || typeof schema !== 'object' || Array.isArray(schema)) {
logger.error('Invalid schema generated from input format', { inputFormat })
return {}
}
return schema as Record<string, unknown>
}This ensures that even if Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
Line: 52:61
Comment:
The `generateParameterSchema` function returns `unknown` type which is then cast to `Record<string, unknown>`, but there's no runtime validation that the result is actually an object.
**Potential issue:**
If `generateToolInputSchema` has a bug or returns an unexpected value (null, array, primitive), the type assertion on line 60 will succeed at compile time but could cause runtime errors when the schema is used.
**Current code:**
```typescript
return generateToolInputSchema(fieldsWithDescriptions) as unknown as Record<string, unknown>
```
**Recommendation:**
Add runtime validation:
```typescript
function generateParameterSchema(
inputFormat: NormalizedField[],
descriptions: Record<string, string>
): Record<string, unknown> {
const fieldsWithDescriptions = inputFormat.map((field) => ({
...field,
description: descriptions[field.name]?.trim() || undefined,
}))
const schema = generateToolInputSchema(fieldsWithDescriptions)
// Validate schema is an object
if (!schema || typeof schema !== 'object' || Array.isArray(schema)) {
logger.error('Invalid schema generated from input format', { inputFormat })
return {}
}
return schema as Record<string, unknown>
}
```
This ensures that even if `generateToolInputSchema` has issues, the function returns a safe default.
How can I resolve this? If you propose a fix, please make it concise.
The Problem: Scenario:
Additional Issue:
Recommendation:
useEffect(() => {
const toolsWithData = servers
.map(server => serverToolsMap[server.id]?.tool)
.filter(Boolean)
.sort((a, b) => new Date(b.updatedAt).getTime() - new Date(a.updatedAt).getTime())
const mostRecentTool = toolsWithData[0]
if (mostRecentTool && !hasLoadedInitialData.current) {
// Load from most recent tool
setToolName(mostRecentTool.toolName)
// ... rest of the loading logic
}
}, [servers, serverToolsMap, workflowName])
const hasConflictingMetadata = servers.length > 1 &&
new Set(servers.map(s => serverToolsMap[s.id]?.tool?.toolName)).size > 1Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
Line: 206:242
Comment:
Data loading effect can cause race condition when multiple tools exist on different servers with conflicting metadata.
**The Problem:**
The effect breaks at the first tool found (line 239), but different servers might have the same workflow with different tool names, descriptions, or parameter schemas. The "first" tool found depends on the iteration order of `servers`, which may not be deterministic.
**Scenario:**
1. Workflow deployed to Server A with toolName="workflow_v1", description="Old description"
2. Same workflow deployed to Server B with toolName="workflow_v2", description="New description"
3. Effect runs and finds Server A's tool first
4. UI shows "workflow_v1" and "Old description"
5. User expects to see the current/latest configuration but sees stale data
**Additional Issue:**
This effect only runs when `serverToolsMap` changes (line 242), so:
- If the user updates a tool on Server A, this effect might not re-run to pick up the changes
- The `hasLoadedInitialData.current` flag prevents re-initialization, so stale data persists
**Recommendation:**
Either:
1. **Use the most recently updated tool:**
```typescript
useEffect(() => {
const toolsWithData = servers
.map(server => serverToolsMap[server.id]?.tool)
.filter(Boolean)
.sort((a, b) => new Date(b.updatedAt).getTime() - new Date(a.updatedAt).getTime())
const mostRecentTool = toolsWithData[0]
if (mostRecentTool && !hasLoadedInitialData.current) {
// Load from most recent tool
setToolName(mostRecentTool.toolName)
// ... rest of the loading logic
}
}, [servers, serverToolsMap, workflowName])
```
2. **Or show a warning when tools have conflicting metadata:**
```typescript
const hasConflictingMetadata = servers.length > 1 &&
new Set(servers.map(s => serverToolsMap[s.id]?.tool?.toolName)).size > 1
```
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
Made Deployed MCPs more explicit to the user.
Type of Change
Testing
Tested manually
Checklist