Skip to content

improvement(mcp): improved deployed mcp ui #2758

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.

Sign up for GitHub

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

Open
aadamgough wants to merge 5 commits into
base: staging
Choose a base branch
from
+449 −125
Open

improvement(mcp): improved deployed mcp ui #2758

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
64b382e
ui improvement
Jan 10, 2026
4622b05
fixed component and removed comments
Jan 10, 2026
b7a3a4a
Removed comment
Jan 10, 2026
210bf41
added validation and greptile comments
Jan 10, 2026
8099e82
greptile comments resolved
Jan 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 104 additions & 85 deletions ...flowId]/components/panel/components/deploy/components/deploy-modal/components/mcp/mcp.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ export function McpDeploy({
[]
)

const selectedServerIds = useMemo(() => {
const actualServerIds = useMemo(() => {
const ids: string[] = []
for (const server of servers) {
const toolInfo = serverToolsMap[server.id]
Expand All @@ -184,6 +184,20 @@ export function McpDeploy({
return ids
}, [servers, serverToolsMap])

const [pendingSelectedServerIds, setPendingSelectedServerIds] = useState<string[] | null>(null)

const selectedServerIds = pendingSelectedServerIds ?? actualServerIds

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])
Comment on lines 193 to 202
Copy link
Contributor

@greptile-apps greptile-apps bot Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const hasLoadedInitialData = useRef(false)

useEffect(() => {
Expand Down Expand Up @@ -241,7 +255,17 @@ export function McpDeploy({
}, [toolName, toolDescription, parameterDescriptions, savedValues])

const hasDeployedTools = selectedServerIds.length > 0

const hasServerSelectionChanges = useMemo(() => {
if (pendingSelectedServerIds === null) return false
const pendingSet = new Set(pendingSelectedServerIds)
const actualSet = new Set(actualServerIds)
if (pendingSet.size !== actualSet.size) return true
return ![...pendingSet].every((id) => actualSet.has(id))
}, [pendingSelectedServerIds, actualServerIds])

const hasChanges = useMemo(() => {
if (hasServerSelectionChanges && selectedServerIds.length > 0) return true
if (!savedValues || !hasDeployedTools) return false
if (toolName !== savedValues.toolName) return true
if (toolDescription !== savedValues.toolDescription) return true
Expand All @@ -251,7 +275,15 @@ export function McpDeploy({
return true
}
return false
}, [toolName, toolDescription, parameterDescriptions, hasDeployedTools, savedValues])
}, [
toolName,
toolDescription,
parameterDescriptions,
hasDeployedTools,
savedValues,
hasServerSelectionChanges,
selectedServerIds.length,
])

useEffect(() => {
onCanSaveChange?.(hasChanges && hasDeployedTools && !!toolName.trim())
Expand All @@ -262,74 +294,19 @@ export function McpDeploy({
}, [servers.length, onHasServersChange])

/**
* Save tool configuration to all deployed servers
* Save tool configuration to all selected servers.
* This handles both adding to new servers and updating existing tools.
*/
const handleSave = useCallback(async () => {
if (!toolName.trim()) return

const toolsToUpdate: Array<{ serverId: string; toolId: string }> = []
for (const server of servers) {
const toolInfo = serverToolsMap[server.id]
if (toolInfo?.tool) {
toolsToUpdate.push({ serverId: server.id, toolId: toolInfo.tool.id })
}
}

if (toolsToUpdate.length === 0) return
if (selectedServerIds.length === 0) return

onSubmittingChange?.(true)
try {
for (const { serverId, toolId } of toolsToUpdate) {
await updateToolMutation.mutateAsync({
workspaceId,
serverId,
toolId,
toolName: toolName.trim(),
toolDescription: toolDescription.trim() || undefined,
parameterSchema,
})
}
// Update saved values after successful save (triggers re-render → hasChanges becomes false)
setSavedValues({
toolName,
toolDescription,
parameterDescriptions: { ...parameterDescriptions },
})
onCanSaveChange?.(false)
onSubmittingChange?.(false)
} catch (error) {
logger.error('Failed to save tool configuration:', error)
onSubmittingChange?.(false)
}
}, [
toolName,
toolDescription,
parameterDescriptions,
parameterSchema,
servers,
serverToolsMap,
workspaceId,
updateToolMutation,
onSubmittingChange,
onCanSaveChange,
])

const serverOptions: ComboboxOption[] = useMemo(() => {
return servers.map((server) => ({
label: server.name,
value: server.id,
}))
}, [servers])

const handleServerSelectionChange = useCallback(
async (newSelectedIds: string[]) => {
if (!toolName.trim()) return

const currentIds = new Set(selectedServerIds)
const newIds = new Set(newSelectedIds)

const toAdd = newSelectedIds.filter((id) => !currentIds.has(id))
const toRemove = selectedServerIds.filter((id) => !newIds.has(id))
const actualSet = new Set(actualServerIds)
const toAdd = selectedServerIds.filter((id) => !actualSet.has(id))
const toRemove = actualServerIds.filter((id) => !selectedServerIds.includes(id))
const toUpdate = selectedServerIds.filter((id) => actualSet.has(id))

for (const serverId of toAdd) {
setPendingServerChanges((prev) => new Set(prev).add(serverId))
Expand All @@ -342,11 +319,8 @@ export function McpDeploy({
toolDescription: toolDescription.trim() || undefined,
parameterSchema,
})
refetchServers()
onAddedToServer?.()
logger.info(`Added workflow ${workflowId} as tool to server ${serverId}`)
} catch (error) {
logger.error('Failed to add tool:', error)
} finally {
setPendingServerChanges((prev) => {
const next = new Set(prev)
Expand All @@ -371,9 +345,6 @@ export function McpDeploy({
delete next[serverId]
return next
})
refetchServers()
} catch (error) {
logger.error('Failed to remove tool:', error)
} finally {
setPendingServerChanges((prev) => {
const next = new Set(prev)
Expand All @@ -383,21 +354,69 @@ export function McpDeploy({
}
}
}
},
[
selectedServerIds,
serverToolsMap,
toolName,
toolDescription,
workspaceId,
workflowId,
parameterSchema,
addToolMutation,
deleteToolMutation,
refetchServers,
onAddedToServer,
]
)

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,
})
}
}
Copy link
Contributor

@greptile-apps greptile-apps bot Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.


refetchServers()

setPendingSelectedServerIds(null)
setSavedValues({
toolName,
toolDescription,
parameterDescriptions: { ...parameterDescriptions },
})
onCanSaveChange?.(false)
onSubmittingChange?.(false)
} catch (error) {
logger.error('Failed to save tool configuration:', error)
onSubmittingChange?.(false)
}
}, [
toolName,
toolDescription,
parameterDescriptions,
parameterSchema,
servers,
serverToolsMap,
workspaceId,
workflowId,
selectedServerIds,
actualServerIds,
addToolMutation,
deleteToolMutation,
updateToolMutation,
refetchServers,
onSubmittingChange,
onCanSaveChange,
onAddedToServer,
])

const serverOptions: ComboboxOption[] = useMemo(() => {
return servers.map((server) => ({
label: server.name,
value: server.id,
}))
}, [servers])

/**
* Handle server selection change - only updates local state.
* Actual add/remove operations happen when user clicks Save.
*/
const handleServerSelectionChange = useCallback((newSelectedIds: string[]) => {
setPendingSelectedServerIds(newSelectedIds)
}, [])

const selectedServersLabel = useMemo(() => {
const count = selectedServerIds.length
Expand Down
Loading