Skip to content

Commit 1f9f65d

Browse files
waleedlatif1waleedlatif1
authored
improvement(mcp): make the OAuth experience visible and verbally consistent (#5829)
* improvement(mcp): make the OAuth experience visible and verbally consistent From a full UX-consistency audit of the MCP settings surfaces: - One name for one action: 'Authorize' / 'Reopen authorization' everywhere (list chip, row, detail) — was three different labels across surfaces. - The connecting state is now visible: the row subtitle shows a muted 'Waiting for authorization...' instead of continuing to shout the red 'OAuth authorization required' mid-flow. - The Authorize affordance is a visible chip on the list row (was buried in the overflow menu), so a blocked popup's 'retry' has an obvious target. - Removed the redundant aggregate discovery banner — every failing row already reports its specific error; two red messages for one failure. - The header Refresh chip no longer renders an error sentence as its label (short 'Failed'; the Status field carries the explanation). - Sentence-case sweep (Unnamed server, Not connected, Server name, Add MCP server / Edit MCP server, Add server, Test connection, Edit form, Delete MCP server), 'Search servers...' placeholder, row-subtitle/error text on the canonical tokens, and a Loading empty state instead of a blank flash. * fix(mcp): show stale-discovery failures, best-effort popup-close label clear, drop redundant branch - A failing latest discovery now shows its error even when cached tools exist (the stale tool count silently hid it). - Best-effort popup.closed poll clears only the 'Waiting for authorization...' label share; the flow entry stays registered so a completion that still arrives over the BroadcastChannel is honored (settleFlow skips the double-decrement). Under COOP misreport the worst case is an early label reset, never a dropped completion. - Remove the OAuth refresh-state branch made redundant by the 'Failed' change, plus its now-unused authType/error inputs.
1 parent e3f9deb commit 1f9f65d

8 files changed

Lines changed: 131 additions & 92 deletions

File tree

‎apps/sim/app/workspace/[workspaceId]/settings/components/mcp/components/mcp-server-form-modal/mcp-server-form-modal.tsx‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ function getTestButtonLabel(
116116
if (testResult?.success) return 'Connection success'
117117
if (testResult?.authRequired) return 'Requires OAuth'
118118
if (testResult && !testResult.success) return 'No connection: retry'
119-
return 'Test Connection'
119+
return 'Test connection'
120120
}
121121

122122
interface FormattedInputProps {
@@ -609,8 +609,8 @@ export function McpServerFormModal({
609609
const isSubmitDisabled =
610610
isSubmitting || !isFormValid || isDomainBlocked || (mode === 'edit' && !hasChanges)
611611

612-
const title = mode === 'add' ? 'Add New MCP Server' : 'Edit MCP Server'
613-
const submitLabel = mode === 'add' ? 'Add MCP' : 'Save'
612+
const title = mode === 'add' ? 'Add MCP server' : 'Edit MCP server'
613+
const submitLabel = mode === 'add' ? 'Add server' : 'Save'
614614

615615
const handleToggleJsonMode = () => {
616616
if (testResult) clearTestResult()
@@ -622,7 +622,7 @@ export function McpServerFormModal({
622622
const secondaryAction: ChipModalFooterAction | undefined =
623623
mode === 'add'
624624
? {
625-
label: formMode === 'form' ? 'Edit JSON' : 'Edit Form',
625+
label: formMode === 'form' ? 'Edit JSON' : 'Edit form',
626626
onClick: handleToggleJsonMode,
627627
}
628628
: formMode === 'form'

‎apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx‎

Lines changed: 34 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,10 @@ function ServerListItem({
9999
)
100100
// A live discovery failure whose stored status hasn't caught up yet would otherwise read as
101101
// "0 tools"; surface it directly so a failed row reads as failed, not empty.
102+
// Shown even when cached tools exist: a present discoveryError means the LATEST
103+
// discovery failed, and silently showing the stale tool count would hide that.
102104
const showDiscoveryError =
103105
Boolean(discoveryError) &&
104-
tools.length === 0 &&
105106
server.connectionStatus !== 'error' &&
106107
server.connectionStatus !== 'disconnected'
107108
const hasConnectionIssue =
@@ -114,38 +115,37 @@ function ServerListItem({
114115
<div className='flex min-w-0 flex-col justify-center gap-[1px]'>
115116
<div className='flex items-center gap-1.5'>
116117
<span className='max-w-[200px] truncate text-[var(--text-body)] text-sm'>
117-
{server.name || 'Unnamed Server'}
118+
{server.name || 'Unnamed server'}
118119
</span>
119120
<span className='text-[var(--text-muted)] text-caption'>({transportLabel})</span>
120121
</div>
121122
<p
122123
className={cn(
123-
'truncate text-sm',
124-
hasConnectionIssue ? 'text-[var(--text-error)]' : 'text-[var(--text-muted)]'
124+
'truncate text-caption',
125+
hasConnectionIssue && !isConnecting
126+
? 'text-[var(--text-error)]'
127+
: 'text-[var(--text-muted)]'
125128
)}
126129
>
127-
{isRefreshing
128-
? 'Refreshing...'
129-
: isLoadingTools && tools.length === 0
130-
? 'Loading...'
131-
: showDiscoveryError
132-
? discoveryError
133-
: toolsLabel}
130+
{isConnecting
131+
? 'Waiting for authorization...'
132+
: isRefreshing
133+
? 'Refreshing...'
134+
: isLoadingTools && tools.length === 0
135+
? 'Loading...'
136+
: showDiscoveryError
137+
? discoveryError
138+
: toolsLabel}
134139
</p>
135140
</div>
136141
<div className='flex flex-shrink-0 items-center gap-1'>
142+
{canManage && server.authType === 'oauth' && server.connectionStatus !== 'connected' && (
143+
<Chip onClick={onAuthorize}>{isConnecting ? 'Reopen authorization' : 'Authorize'}</Chip>
144+
)}
137145
<RowActionsMenu
138146
label='Server actions'
139147
actions={[
140148
{ label: 'Details', onSelect: onViewDetails },
141-
...(canManage && server.authType === 'oauth' && server.connectionStatus !== 'connected'
142-
? [
143-
{
144-
label: isConnecting ? 'Reopen authorization' : 'Authorize',
145-
onSelect: onAuthorize,
146-
},
147-
]
148-
: []),
149149
...(canManage
150150
? [
151151
{
@@ -193,11 +193,7 @@ export function MCP() {
193193
isLoading: serversLoading,
194194
error: serversError,
195195
} = useMcpServers(workspaceId)
196-
const {
197-
data: mcpToolsData = [],
198-
error: toolsError,
199-
toolsStateByServer,
200-
} = useMcpToolsQuery(workspaceId)
196+
const { data: mcpToolsData = [], toolsStateByServer } = useMcpToolsQuery(workspaceId)
201197
const { data: storedTools = [], refetch: refetchStoredTools } = useStoredMcpTools(workspaceId)
202198
const forceRefreshToolsMutation = useForceRefreshMcpTools()
203199
const forceRefreshTools = forceRefreshToolsMutation.mutate
@@ -399,15 +395,10 @@ export function MCP() {
399395
return issues
400396
}
401397

402-
// Only a failure to load the server LIST replaces the list. A tool-discovery failure
403-
// (`toolsError`) must not blank the page — the servers still render, each row surfacing its
404-
// own discovery state via `toolsStateByServer`, with a non-blocking notice above the list.
398+
// Only a failure to load the server LIST replaces the list. A tool-discovery failure must
399+
// not blank the page — the servers still render, each row surfacing its own discovery
400+
// state via `toolsStateByServer`.
405401
const listError = serversError
406-
// Any per-server discovery failure — even a partial one where other servers succeeded (which
407-
// suppresses the aggregate `toolsError`) — so the notice below still surfaces it.
408-
const hasDiscoveryError =
409-
Boolean(toolsError) ||
410-
Array.from(toolsStateByServer.values()).some((state) => state.error != null)
411402
const hasServers = servers && servers.length > 0
412403
const showNoResults = searchTerm.trim() && filteredServers.length === 0 && servers.length > 0
413404

@@ -418,15 +409,13 @@ export function MCP() {
418409
const refreshAction = getRefreshActionState({
419410
mutationStatus: isCurrentRefresh ? refreshServerMutation.status : 'idle',
420411
connectionStatus: isCurrentRefresh ? refreshServerMutation.data?.status : undefined,
421-
authType: server.authType,
422-
error: isCurrentRefresh ? refreshServerMutation.data?.error : undefined,
423412
workflowsUpdated: isCurrentRefresh ? refreshServerMutation.data?.workflowsUpdated : undefined,
424413
})
425414

426415
return (
427416
<SettingsPanel
428417
back={{ text: 'MCP tools', icon: ArrowLeft, onSelect: handleBackToList }}
429-
title={server.name || 'Unnamed Server'}
418+
title={server.name || 'Unnamed server'}
430419
actions={
431420
canEdit
432421
? [
@@ -447,8 +436,8 @@ export function MCP() {
447436
<SettingsSection label='Server'>
448437
<div className='flex flex-col gap-4.5'>
449438
<div className='flex flex-col gap-2'>
450-
<span className='text-[var(--text-muted)] text-caption'>Server Name</span>
451-
<p className='text-[var(--text-body)] text-sm'>{server.name || 'Unnamed Server'}</p>
439+
<span className='text-[var(--text-muted)] text-caption'>Server name</span>
440+
<p className='text-[var(--text-body)] text-sm'>{server.name || 'Unnamed server'}</p>
452441
</div>
453442

454443
<div className='flex flex-col gap-2'>
@@ -487,9 +476,7 @@ export function MCP() {
487476
await startOauthForServer(server.id)
488477
}}
489478
>
490-
{connectingOauthServers.has(server.id)
491-
? 'Reopen authorization window'
492-
: 'Connect with OAuth'}
479+
{connectingOauthServers.has(server.id) ? 'Reopen authorization' : 'Authorize'}
493480
</Chip>
494481
</div>
495482
</div>
@@ -651,7 +638,7 @@ export function MCP() {
651638
search={{
652639
value: searchTerm,
653640
onChange: setSearchTerm,
654-
placeholder: 'Search MCPs...',
641+
placeholder: 'Search servers...',
655642
}}
656643
actions={
657644
canEdit
@@ -669,21 +656,18 @@ export function MCP() {
669656
>
670657
{listError ? (
671658
<div className='flex h-full flex-col items-center justify-center gap-2'>
672-
<p className='text-[var(--text-error)] text-xs leading-tight'>
659+
<p className='text-[var(--text-error)] text-small leading-tight'>
673660
{getErrorMessage(listError, 'Failed to load MCP servers')}
674661
</p>
675662
</div>
676-
) : serversLoading ? null : !hasServers ? (
663+
) : serversLoading ? (
664+
<SettingsEmptyState>Loading...</SettingsEmptyState>
665+
) : !hasServers ? (
677666
<SettingsEmptyState>
678667
{canEdit ? 'Click "Add server" above to get started' : 'No MCP servers configured'}
679668
</SettingsEmptyState>
680669
) : (
681670
<div className='flex flex-col gap-2'>
682-
{hasDiscoveryError && (
683-
<p className='text-[var(--text-error)] text-xs leading-tight'>
684-
{getErrorMessage(toolsError, 'Some tools could not be discovered')}
685-
</p>
686-
)}
687671
{filteredServers.map((server) => {
688672
if (!server?.id) return null
689673
const tools = toolsByServer[server.id] || []
@@ -749,8 +733,8 @@ export function MCP() {
749733
onOpenChange={(open) => {
750734
if (!open) setServerToDeleteId(null)
751735
}}
752-
srTitle='Delete MCP Server'
753-
title='Delete MCP Server'
736+
srTitle='Delete MCP server'
737+
title='Delete MCP server'
754738
text={[
755739
'Are you sure you want to delete ',
756740
{

‎apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts‎

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -31,28 +31,11 @@ describe('getRefreshActionState', () => {
3131
})
3232
})
3333

34-
it('shows OAuth authorization required when an OAuth refresh finishes disconnected', () => {
35-
expect(
36-
getRefreshActionState({
37-
mutationStatus: 'success',
38-
connectionStatus: 'disconnected',
39-
authType: 'oauth',
40-
workflowsUpdated: 0,
41-
})
42-
).toEqual({
43-
text: 'OAuth authorization required',
44-
textTone: 'error',
45-
disabled: false,
46-
})
47-
})
48-
4934
it('keeps Failed when a disconnected OAuth refresh has a concrete error', () => {
5035
expect(
5136
getRefreshActionState({
5237
mutationStatus: 'success',
5338
connectionStatus: 'disconnected',
54-
authType: 'oauth',
55-
error: 'The MCP server took too long to respond and timed out',
5639
workflowsUpdated: 0,
5740
})
5841
).toEqual({

‎apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts‎

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,10 @@
11
import type { MutationStatus } from '@tanstack/react-query'
2-
import type { McpServer, RefreshMcpServerResult } from '@/lib/api/contracts/mcp'
2+
import type { RefreshMcpServerResult } from '@/lib/api/contracts/mcp'
33
import type { SettingsAction } from '@/app/workspace/[workspaceId]/settings/components/settings-header/settings-header'
44

55
interface RefreshActionStateInput {
66
mutationStatus: MutationStatus
77
connectionStatus?: RefreshMcpServerResult['status']
8-
authType?: McpServer['authType']
9-
error?: RefreshMcpServerResult['error']
108
workflowsUpdated?: number
119
}
1210

@@ -15,23 +13,12 @@ type RefreshActionState = Pick<SettingsAction, 'text' | 'textTone' | 'disabled'>
1513
export function getRefreshActionState({
1614
mutationStatus,
1715
connectionStatus,
18-
authType,
19-
error,
2016
workflowsUpdated,
2117
}: RefreshActionStateInput): RefreshActionState {
2218
if (mutationStatus === 'pending') {
2319
return { text: 'Refreshing...', textTone: undefined, disabled: true }
2420
}
2521

26-
if (
27-
mutationStatus === 'success' &&
28-
connectionStatus === 'disconnected' &&
29-
authType === 'oauth' &&
30-
!error?.trim()
31-
) {
32-
return { text: 'OAuth authorization required', textTone: 'error', disabled: false }
33-
}
34-
3522
if (
3623
mutationStatus === 'error' ||
3724
(mutationStatus === 'success' && connectionStatus !== 'connected')

‎apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ describe('getServerToolsLabel', () => {
1919
})
2020

2121
it('keeps the generic disconnected state for non-OAuth servers', () => {
22-
expect(getServerToolsLabel([], 'disconnected', null, 'headers')).toBe('Not Connected')
22+
expect(getServerToolsLabel([], 'disconnected', null, 'headers')).toBe('Not connected')
2323
})
2424

2525
it('shows the persisted error for disconnected connections', () => {

‎apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ export function getServerToolsLabel(
1616

1717
if (connectionStatus === 'disconnected') {
1818
return (
19-
lastError?.trim() || (authType === 'oauth' ? 'OAuth authorization required' : 'Not Connected')
19+
lastError?.trim() || (authType === 'oauth' ? 'OAuth authorization required' : 'Not connected')
2020
)
2121
}
2222

‎apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,4 +243,62 @@ describe('useMcpOauthPopup', () => {
243243
expect(order).toEqual(['open', 'start'])
244244
hook.unmount()
245245
})
246+
247+
it('clears the label when the popup closes but still honors a late completion', async () => {
248+
vi.useFakeTimers()
249+
try {
250+
const popup = {
251+
close: vi.fn(),
252+
focus: vi.fn(),
253+
location: { replace: vi.fn() },
254+
closed: false,
255+
}
256+
;(window.open as ReturnType<typeof vi.fn>).mockReturnValue(popup as unknown as Window)
257+
let channelHandler: ((e: { data: unknown }) => void) | null = null
258+
class CapturingChannel {
259+
constructor(public name: string) {}
260+
postMessage(): void {}
261+
close(): void {}
262+
}
263+
Object.defineProperty(CapturingChannel.prototype, 'onmessage', {
264+
set(h) {
265+
channelHandler = h
266+
},
267+
get() {
268+
return channelHandler
269+
},
270+
configurable: true,
271+
})
272+
;(globalThis as unknown as { BroadcastChannel: unknown }).BroadcastChannel = CapturingChannel
273+
mockStartOauth.mockResolvedValue({
274+
status: 'redirect',
275+
authorizationUrl: 'https://as.example/a?state=st',
276+
state: 'st',
277+
})
278+
const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' }))
279+
await act(async () => {
280+
await hook.result().startOauthForServer('s1')
281+
})
282+
expect(hook.result().connectingServers.has('s1')).toBe(true)
283+
284+
// User closes the popup — the label share clears within a poll tick...
285+
popup.closed = true
286+
await act(async () => {
287+
vi.advanceTimersByTime(1500)
288+
})
289+
expect(hook.result().connectingServers.has('s1')).toBe(false)
290+
291+
// ...but the flow stays registered: a late BroadcastChannel completion is still honored.
292+
const invalidateSpy = vi.spyOn(hook.queryClient, 'invalidateQueries')
293+
await act(async () => {
294+
channelHandler?.({ data: { type: 'mcp-oauth', ok: true, serverId: 's1', state: 'st' } })
295+
})
296+
expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: ['mcp', 'servers', 'w1'] })
297+
// And the count did not go negative / re-clear twice.
298+
expect(hook.result().connectingServers.has('s1')).toBe(false)
299+
hook.unmount()
300+
} finally {
301+
vi.useRealTimers()
302+
}
303+
})
246304
})

0 commit comments

Comments
 (0)