Skip to content

Commit 543cb98

Browse files
Dumbrisclaude
andcommitted
fix(web): review fixes — bridge no-config connect in wizard + per-client Connect All backups
Codex review of the Spec 078 US2 slice found two majors and a minor; all three verified against the code and fixed: 1. OnboardingWizard ClientRow rendered 'Not installed' for EVERY exists=false client, including bridge clients (Claude Desktop), which the backend explicitly marks connectable without a config file (internal/connect/connect.go:48; connect creates the file, returning an empty backup_path — connect_test.go:253). That made the wizard's "no prior file to back up" branch (FR-006 / US2 independent test) unreachable. ClientRow now gates on !exists && !bridge, matching ConnectModal's connectableClients predicate; non-bridge missing clients still show 'Not installed'. 2. ConnectModal connectAll() let each connect() overwrite resultBackupPath, so a bulk run over N modified configs displayed only the Nth backup (SC-005 requires 100%). connectAll now accumulates a per-client bulkBackups list ({name, path|no-prior-file} per successful connect, each with its own Copy affordance) and suppresses the single- result line for bulk runs. The loop also iterates a snapshot of connectableClients — connect() refetches the listing mid-loop, which mutated the computed while it was being iterated. Single connects (connectSingle) and disconnects clear the bulk list. 3. Minor: the wizard's open watcher reset connectMessage but not connectBackups/copiedBackupClient, so backup rows from a previous wizard session replayed on reopen. Both are now session-scoped. TDD: 4 new failing cases first — wizard bridge-no-config connect, wizard non-bridge stays 'Not installed', wizard reopen clears backup rows, ConnectModal Connect All shows all three per-client outcomes (two paths + one no-prior-file) with per-row copy. Full frontend suite: 32 files, 239 tests pass; vue-tsc clean; make build rebuilds the embedded dist; golangci-lint v2 + go test -race ./internal/connect/... clean (no Go changes in this commit). npm-ci metadata churn in frontend/package-lock.json reverted again, not committed (same as the base commit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 3b33951 commit 543cb98

4 files changed

Lines changed: 308 additions & 12 deletions

File tree

frontend/src/components/ConnectModal.vue

Lines changed: 99 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@
6969
</button>
7070
<button
7171
v-else
72-
@click="connect(client.id)"
72+
@click="connectSingle(client.id)"
7373
class="btn btn-primary btn-xs"
7474
:disabled="loading.clients[client.id]"
7575
>
@@ -157,6 +157,37 @@
157157
>
158158
No prior config file existed, so no backup was needed.
159159
</div>
160+
<!-- Spec 078 US2 / SC-005: Connect All renders EVERY successful
161+
client's backup outcome — one line per client, each with its own
162+
copy affordance — instead of only the last connect's path. -->
163+
<div v-if="bulkBackups.length > 0" data-test="connect-bulk-backups" class="mt-2 space-y-1">
164+
<div
165+
v-for="b in bulkBackups"
166+
:key="b.id"
167+
:data-test="`connect-bulk-backup-${b.id}`"
168+
class="flex items-start justify-between gap-2 rounded-lg bg-base-200 px-3 py-2"
169+
>
170+
<span class="text-xs leading-relaxed min-w-0">
171+
<span class="font-medium">{{ b.name }}:</span>
172+
<template v-if="b.backupPath">
173+
a backup of your previous config was saved to
174+
<code class="font-mono text-[11px] break-all" :title="b.backupPath">{{ b.backupPath }}</code>
175+
</template>
176+
<template v-else>
177+
No prior config file existed, so no backup was needed.
178+
</template>
179+
</span>
180+
<button
181+
v-if="b.backupPath"
182+
:data-test="`connect-bulk-copy-${b.id}`"
183+
@click="copyBulkBackupPath(b)"
184+
class="btn btn-ghost btn-xs shrink-0"
185+
title="Copy this backup path to the clipboard"
186+
>
187+
{{ copiedBulkClient === b.id ? 'Copied ✓' : 'Copy path' }}
188+
</button>
189+
</div>
190+
</div>
160191
</div>
161192

162193
<div class="modal-action">
@@ -203,6 +234,12 @@ const resultSuccess = ref(false)
203234
// back up; undefined = no successful operation to report on.
204235
const resultBackupPath = ref<string | null | undefined>(undefined)
205236
const copiedBackup = ref(false)
237+
// Spec 078 US2 / SC-005: per-client backup outcomes for Connect All. Every
238+
// successful connect in the bulk run keeps its own entry (string = backup
239+
// created; null = no prior file), so no client's backup path is overwritten
240+
// by the next one. Empty when the last operation was a single connect.
241+
const bulkBackups = ref<Array<{ id: string; name: string; backupPath: string | null }>>([])
242+
const copiedBulkClient = ref<string | null>(null)
206243
const loading = reactive({
207244
initial: false,
208245
clients: {} as Record<string, boolean>,
@@ -289,7 +326,17 @@ async function fetchClients() {
289326
}
290327
}
291328
292-
async function connect(clientId: string) {
329+
// Single-connect entry point (row button): a fresh single operation supersedes
330+
// any earlier Connect All per-client backup list.
331+
async function connectSingle(clientId: string) {
332+
bulkBackups.value = []
333+
copiedBulkClient.value = null
334+
await connect(clientId)
335+
}
336+
337+
// Returns the outcome so connectAll can accumulate per-client backup results
338+
// (ok=true with backupPath string = backup created; null = no prior file).
339+
async function connect(clientId: string): Promise<{ ok: boolean; backupPath: string | null }> {
293340
loading.clients[clientId] = true
294341
resultMessage.value = ''
295342
resultBackupPath.value = undefined
@@ -301,34 +348,38 @@ async function connect(clientId: string) {
301348
resultMessage.value = response.data.message || `Connected to ${clientId}`
302349
resultSuccess.value = true
303350
// Empty/absent backup_path on success means no prior file existed.
304-
resultBackupPath.value = response.data.backup_path || null
351+
const backupPath = response.data.backup_path || null
352+
resultBackupPath.value = backupPath
305353
await fetchClients()
306354
systemStore.addToast({
307355
type: 'success',
308356
title: 'Client Connected',
309357
message: `MCPProxy registered in ${clientId}`,
310358
})
311-
} else {
312-
resultMessage.value = response.error || 'Failed to connect'
313-
resultSuccess.value = false
314-
// The write may have failed because macOS blocked the config. Resolve the
315-
// access state in-band so a denial surfaces as the actionable banner.
316-
void checkAccess(clientId)
359+
return { ok: true, backupPath }
317360
}
361+
resultMessage.value = response.error || 'Failed to connect'
362+
resultSuccess.value = false
363+
// The write may have failed because macOS blocked the config. Resolve the
364+
// access state in-band so a denial surfaces as the actionable banner.
365+
void checkAccess(clientId)
318366
} catch (err) {
319367
resultMessage.value = err instanceof Error ? err.message : 'Unknown error'
320368
resultSuccess.value = false
321369
void checkAccess(clientId)
322370
} finally {
323371
loading.clients[clientId] = false
324372
}
373+
return { ok: false, backupPath: null }
325374
}
326375
327376
async function disconnect(clientId: string) {
328377
loading.clients[clientId] = true
329378
resultMessage.value = ''
330379
resultBackupPath.value = undefined
331380
copiedBackup.value = false
381+
bulkBackups.value = []
382+
copiedBulkClient.value = null
332383
333384
try {
334385
const client = clients.value.find(c => c.id === clientId)
@@ -429,16 +480,51 @@ async function copyBackupPath() {
429480
}
430481
}
431482
483+
// Spec 078 US2 / SC-005: Connect All accumulates every successful client's
484+
// backup outcome instead of letting each connect() overwrite the previous
485+
// client's backup path.
432486
async function connectAll() {
433-
for (const client of connectableClients.value) {
434-
await connect(client.id)
487+
bulkBackups.value = []
488+
copiedBulkClient.value = null
489+
// Snapshot: connect() refetches the client list mid-loop, which mutates the
490+
// connectableClients computed while we iterate it.
491+
const targets = [...connectableClients.value]
492+
const collected: Array<{ id: string; name: string; backupPath: string | null }> = []
493+
for (const client of targets) {
494+
const outcome = await connect(client.id)
495+
if (outcome.ok) {
496+
collected.push({ id: client.id, name: client.name, backupPath: outcome.backupPath })
497+
}
498+
}
499+
if (collected.length > 0) {
500+
bulkBackups.value = collected
501+
// The per-client list is authoritative for a bulk run; suppress the
502+
// single-result line that would otherwise repeat only the last backup.
503+
resultBackupPath.value = undefined
504+
}
505+
}
506+
507+
// Per-row copy for the Connect All backup list.
508+
async function copyBulkBackupPath(entry: { id: string; backupPath: string | null }) {
509+
if (!entry.backupPath) return
510+
try {
511+
await navigator.clipboard.writeText(entry.backupPath)
512+
copiedBulkClient.value = entry.id
513+
setTimeout(() => {
514+
if (copiedBulkClient.value === entry.id) copiedBulkClient.value = null
515+
}, 2000)
516+
} catch {
517+
// Clipboard unavailable (e.g. insecure context): the full path is already
518+
// rendered, so the user can select and copy it manually.
435519
}
436520
}
437521
438522
function close() {
439523
resultMessage.value = ''
440524
resultBackupPath.value = undefined
441525
copiedBackup.value = false
526+
bulkBackups.value = []
527+
copiedBulkClient.value = null
442528
emit('close')
443529
}
444530
@@ -452,6 +538,8 @@ watch(() => props.show, (newVal) => {
452538
resultMessage.value = ''
453539
resultBackupPath.value = undefined
454540
copiedBackup.value = false
541+
bulkBackups.value = []
542+
copiedBulkClient.value = null
455543
}
456544
})
457545
</script>

frontend/src/components/OnboardingWizard.vue

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -707,6 +707,10 @@ watch(() => props.show, async (open) => {
707707
if (open) {
708708
serverAddedJustNow.value = false
709709
connectMessage.value = ''
710+
// Backup lines are session-scoped (Spec 078 US2): don't replay backup
711+
// rows from connects performed in a previous wizard session.
712+
for (const k of Object.keys(connectBackups)) delete connectBackups[k]
713+
copiedBackupClient.value = null
710714
await onboarding.fetchState()
711715
await Promise.all([
712716
fetchClients(),
@@ -1160,7 +1164,11 @@ const ClientRow: FunctionalComponent<
11601164
h('div', { class: 'shrink-0 ml-2' }, [
11611165
!c.supported
11621166
? h('span', { class: 'badge badge-ghost badge-sm' }, c.reason || 'Not supported')
1163-
: !c.exists
1167+
// Bridge clients (e.g. Claude Desktop) are connectable even without
1168+
// an existing config file — Connect creates it (parity with
1169+
// ConnectModal's connectableClients gating; Spec 078 US2/FR-006:
1170+
// this is the path that produces the "no prior file" backup case).
1171+
: !c.exists && !c.bridge
11641172
? h('span', { class: 'text-xs opacity-40' }, 'Not installed')
11651173
: c.connected
11661174
? h('span', { class: 'badge badge-success badge-sm' }, 'Connected')

frontend/tests/unit/connect-modal.spec.ts

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -513,6 +513,103 @@ describe('ConnectModal', () => {
513513
expect(backup.text()).toContain('opencode.json.bak.20260702-110000')
514514
})
515515

516+
// Spec 078 US2 / SC-005: Connect All must surface EVERY successful client's
517+
// backup outcome, not just the last one — 100% of successful connects that
518+
// modified an existing config display their backup path.
519+
it('Connect All surfaces a per-client backup line for every successful connect', async () => {
520+
;(api.getConnectStatus as any).mockResolvedValue({
521+
success: true,
522+
data: [
523+
{
524+
id: 'cursor',
525+
name: 'Cursor',
526+
config_path: '/Users/test/.cursor/mcp.json',
527+
exists: true,
528+
connected: false,
529+
supported: true,
530+
icon: 'cursor',
531+
},
532+
{
533+
id: 'codex',
534+
name: 'Codex CLI',
535+
config_path: '/Users/test/.codex/config.toml',
536+
exists: true,
537+
connected: false,
538+
supported: true,
539+
icon: 'codex',
540+
},
541+
{
542+
// Bridge client with no config file: connect creates it → no backup.
543+
id: 'claude-desktop',
544+
name: 'Claude Desktop',
545+
config_path: '/Users/test/Library/Application Support/Claude/claude_desktop_config.json',
546+
exists: false,
547+
connected: false,
548+
supported: true,
549+
bridge: true,
550+
icon: 'claude-desktop',
551+
},
552+
],
553+
})
554+
;(api.connectClient as any).mockImplementation((id: string) => {
555+
const backups: Record<string, string | undefined> = {
556+
cursor: '/Users/test/.cursor/mcp.json.bak.20260702-101530',
557+
codex: '/Users/test/.codex/config.toml.bak.20260702-101531',
558+
'claude-desktop': undefined,
559+
}
560+
return Promise.resolve({
561+
success: true,
562+
data: {
563+
success: true,
564+
client: id,
565+
config_path: '',
566+
backup_path: backups[id],
567+
server_name: 'mcpproxy',
568+
action: 'added',
569+
message: `MCPProxy registered in ${id}`,
570+
},
571+
})
572+
})
573+
const writeText = vi.fn().mockResolvedValue(undefined)
574+
Object.assign(navigator, { clipboard: { writeText } })
575+
576+
const wrapper = mount(ConnectModal, {
577+
props: { show: false },
578+
global: { plugins: [pinia] },
579+
})
580+
await wrapper.setProps({ show: true })
581+
await flushPromises()
582+
583+
const connectAllBtn = wrapper
584+
.findAll('button')
585+
.find(b => b.text().includes('Connect All'))!
586+
expect(connectAllBtn).toBeTruthy()
587+
await connectAllBtn.trigger('click')
588+
await flushPromises()
589+
590+
expect(api.connectClient).toHaveBeenCalledTimes(3)
591+
592+
// Every modified-config client shows ITS backup path.
593+
const cursorRow = wrapper.find('[data-test="connect-bulk-backup-cursor"]')
594+
expect(cursorRow.exists()).toBe(true)
595+
expect(cursorRow.text()).toContain('/Users/test/.cursor/mcp.json.bak.20260702-101530')
596+
const codexRow = wrapper.find('[data-test="connect-bulk-backup-codex"]')
597+
expect(codexRow.exists()).toBe(true)
598+
expect(codexRow.text()).toContain('/Users/test/.codex/config.toml.bak.20260702-101531')
599+
// The no-prior-file client states its case explicitly.
600+
const desktopRow = wrapper.find('[data-test="connect-bulk-backup-claude-desktop"]')
601+
expect(desktopRow.exists()).toBe(true)
602+
expect(desktopRow.text()).toContain('No prior config file existed')
603+
604+
// The single-result backup line must not duplicate the last client's path.
605+
expect(wrapper.find('[data-test="connect-backup-path"]').exists()).toBe(false)
606+
607+
// Per-row copy copies THAT client's path.
608+
await wrapper.find('[data-test="connect-bulk-copy-cursor"]').trigger('click')
609+
await flushPromises()
610+
expect(writeText).toHaveBeenCalledWith('/Users/test/.cursor/mcp.json.bak.20260702-101530')
611+
})
612+
516613
// Spec 075 US1/US2: the explicit per-client "Check access" action reads one
517614
// client's config on demand (the only privacy-prompt-eligible call) and
518615
// resolves its access_state in-band, surfacing a denial without a full connect.

0 commit comments

Comments
 (0)