feat(THU-618): support editing MCP servers in settings

src/dal/index.ts

@@ -72,6 +72,8 @@ export {
   deleteMcpServer,
   getAllMcpServers,
   getRemoteMcpServers,
+  updateMcpServer,
+  updateMcpServerWithCredentials,
   type McpServerWithCredential,
 } from './mcp-servers'
 

src/dal/mcp-servers.test.ts

@@ -13,6 +13,7 @@ import {
   deleteMcpServer,
   getAllMcpServers,
   getRemoteMcpServers,
+  updateMcpServerWithCredentials,
 } from './mcp-servers'
 import { getMcpServerCredentials } from './mcp-secrets'
 import { resetTestDatabase, setupTestDatabase, teardownTestDatabase } from './test-utils'
@@ -316,6 +317,69 @@ describe('MCP Servers DAL', () => {
     })
   })
 
+  describe('updateMcpServerWithCredentials', () => {
+    it('patches mutable fields and leaves credentials alone when none given', async () => {
+      const db = getDb()
+      const id = uuidv7()
+      await createMcpServerWithCredentials(
+        db,
+        { id, name: 'Original', type: 'http', url: 'https://example.com/mcp', enabled: 1 },
+        { type: 'bearer', token: 'original-token' },
+      )
+
+      await updateMcpServerWithCredentials(db, id, { name: 'Renamed', url: 'https://new.example.com/mcp' })
+
+      const servers = await getAllMcpServers(db)
+      expect(servers[0]?.name).toBe('Renamed')
+      expect(servers[0]?.url).toBe('https://new.example.com/mcp')
+      expect(await getMcpServerCredentials(db, id)).toEqual({ type: 'bearer', token: 'original-token' })
+    })
+
+    it('replaces the credential when an object is given', async () => {
+      const db = getDb()
+      const id = uuidv7()
+      await createMcpServerWithCredentials(
+        db,
+        { id, name: 'Server', type: 'http', url: 'https://example.com/mcp', enabled: 1 },
+        { type: 'bearer', token: 'old-token' },
+      )
+
+      await updateMcpServerWithCredentials(db, id, { name: 'Server' }, { type: 'bearer', token: 'new-token' })
+
+      expect(await getMcpServerCredentials(db, id)).toEqual({ type: 'bearer', token: 'new-token' })
+    })
+
+    it('deletes the credential when null is given', async () => {
+      const db = getDb()
+      const id = uuidv7()
+      await createMcpServerWithCredentials(
+        db,
+        { id, name: 'Server', type: 'http', url: 'https://example.com/mcp', enabled: 1 },
+        { type: 'bearer', token: 'doomed' },
+      )
+
+      await updateMcpServerWithCredentials(db, id, { name: 'Server' }, null)
+
+      expect(await getMcpServerCredentials(db, id)).toBeNull()
+    })
+
+    it('adds a credential to a previously-uncredentialed server', async () => {
+      const db = getDb()
+      const id = uuidv7()
+      await createMcpServerWithCredentials(db, {
+        id,
+        name: 'Open Server',
+        type: 'http',
+        url: 'https://example.com/mcp',
+        enabled: 1,
+      })
+
+      await updateMcpServerWithCredentials(db, id, {}, { type: 'bearer', token: 'fresh' })
+
+      expect(await getMcpServerCredentials(db, id)).toEqual({ type: 'bearer', token: 'fresh' })
+    })
+  })
+
   describe('createMcpServer', () => {
     it('should create a new MCP server', async () => {
       const serverId = uuidv7()

src/dal/mcp-servers.ts

@@ -6,7 +6,7 @@ import { and, eq, inArray, isNotNull, isNull } from 'drizzle-orm'
 import type { AnyDrizzleDatabase } from '../db/database-interface'
 import { mcpSecretsTable, mcpServersTable } from '../db/tables'
 import { clearNullableColumns, nowIso } from '../lib/utils'
-import { setMcpServerCredentials, type McpServerCredentials } from './mcp-secrets'
+import { deleteMcpServerCredentials, setMcpServerCredentials, type McpServerCredentials } from './mcp-secrets'
 import { type McpServer } from '@/types'
 import type { DrizzleQueryWithPromise } from '@/types'
 
@@ -81,6 +81,46 @@ export const createMcpServerWithCredentials = async (
   })
 }
 
+/**
+ * Patches an MCP server row (and bumps updatedAt). The patch must NOT include
+ * `id` or `createdAt`; touch only mutable columns. No-op when the id doesn't
+ * match (the update silently affects zero rows).
+ */
+export const updateMcpServer = async (
+  db: AnyDrizzleDatabase,
+  id: string,
+  patch: Partial<Omit<McpServer, 'id' | 'createdAt'>>,
+): Promise<void> => {
+  await db
+    .update(mcpServersTable)
+    .set({ ...patch, updatedAt: nowIso() })
+    .where(eq(mcpServersTable.id, id))
+}
+
+/**
+ * Updates an MCP server row and (optionally) its on-device credentials in a
+ * single transaction. Symmetric to {@link createMcpServerWithCredentials}.
+ * `credentials` semantics:
+ *   - `undefined`: leave existing credential alone (e.g. rename without touching the token)
+ *   - `null`: delete existing credential (user cleared the bearer field)
+ *   - object: replace existing credential
+ */
+export const updateMcpServerWithCredentials = async (
+  db: AnyDrizzleDatabase,
+  id: string,
+  patch: Partial<Omit<McpServer, 'id' | 'createdAt'>>,
+  credentials?: McpServerCredentials | null,
+): Promise<void> => {
+  await db.transaction(async (tx) => {
+    if (credentials === null) {
+      await deleteMcpServerCredentials(tx, id)
+    } else if (credentials) {
+      await setMcpServerCredentials(tx, id, credentials)
+    }
+    await updateMcpServer(tx, id, patch)
+  })
+}
+
 /** One server row plus its optional on-device credential, for batch creation. */
 export type McpServerWithCredential = {
   server: Partial<McpServer> & Pick<McpServer, 'id' | 'name'>

src/hooks/use-add-server-form.test.ts

@@ -206,6 +206,50 @@ describe('useAddServerForm', () => {
     expect(result.current.testResult.kind).toBe('needs-oauth')
   })
 
+  it('keeps a passing test result when only the name is edited', async () => {
+    const { result } = renderForm(makeDeps())
+
+    act(() => result.current.openDialog())
+    act(() => result.current.changeUrl('https://tools.example.com/mcp'))
+    await act(async () => {
+      getClock().tick(700)
+      await getClock().runAllAsync()
+    })
+    expect(result.current.testResult.kind).toBe('success')
+
+    // Renaming doesn't change what the probe verifies, so the success must survive —
+    // otherwise Save Changes gets stuck disabled with no obvious recovery.
+    act(() => result.current.changeName('My Server'))
+    expect(result.current.testResult.kind).toBe('success')
+  })
+
+  it('reports hasConnectionEdits only for connection-affecting fields in edit mode', () => {
+    const { result } = renderForm(makeDeps())
+
+    act(() =>
+      result.current.openEditDialog(
+        { id: 's1', name: 'GitHub', url: 'https://api.github.com/mcp', type: 'http', enabled: 1 } as never,
+        'tok-1',
+      ),
+    )
+    expect(result.current.hasConnectionEdits).toBe(false)
+
+    // Name is metadata, not part of the probe — must not flip the flag.
+    act(() => result.current.changeName('Renamed'))
+    expect(result.current.hasConnectionEdits).toBe(false)
+
+    act(() => result.current.changeUrl('https://api.github.com/mcp/v2'))
+    expect(result.current.hasConnectionEdits).toBe(true)
+  })
+
+  it('hasConnectionEdits is true in Add mode (no original snapshot)', () => {
+    const { result } = renderForm(makeDeps())
+
+    act(() => result.current.openDialog())
+    // No original to diff against — Add must keep the existing test-success Save gate.
+    expect(result.current.hasConnectionEdits).toBe(true)
+  })
+
   it('resets all form state on resetAddDialog', async () => {
     const { result } = renderForm(makeDeps())
 

src/hooks/use-add-server-form.ts

@@ -9,6 +9,7 @@ import type { probeMcpServerTools } from '@/lib/mcp-connection-test'
 import { buildMcpHeaders, createMcpTransport, type MCPTransportType } from '@/lib/mcp-transport'
 import { validateMcpServerUrl } from '@/lib/mcp-url-validation'
 import type { FetchFn } from '@/lib/proxy-fetch'
+import type { McpServer } from '@/types'
 import { useEffect, useReducer, useRef } from 'react'
 
 /**
@@ -44,6 +45,8 @@ export const generateServerName = (url: string): string => {
 
 type AddServerFormState = {
   isAddDialogOpen: boolean
+  /** Non-null when the dialog is editing an existing server (id) instead of adding one. */
+  editingServerId: string | null
   name: string
   /** True once the user edits the name field, so the URL stops re-deriving it. */
   nameManuallyEdited: boolean
@@ -52,10 +55,16 @@ type AddServerFormState = {
   token: string
   isTestingConnection: boolean
   testResult: TestConnectionResult | { kind: 'idle' }
+  /** Snapshot of url/transport/token at edit-open. Used to detect whether the
+   *  user changed a connection-affecting field, so the Save gate can skip the
+   *  fresh-probe requirement on a metadata-only edit (e.g. rename) where the
+   *  existing credential — including OAuth — is presumed valid. Null in Add mode. */
+  originalConnection: { url: string; transport: MCPTransportType; token: string } | null
 }
 
 type AddServerFormAction =
   | { type: 'open-dialog' }
+  | { type: 'open-edit-dialog'; server: McpServer; bearerToken: string | null }
   | { type: 'reset' }
   | { type: 'set-name'; value: string }
   | { type: 'set-url'; value: string; derivedName: string | null }
@@ -68,19 +77,39 @@ type AddServerFormAction =
 
 const initialState: AddServerFormState = {
   isAddDialogOpen: false,
+  editingServerId: null,
   name: '',
   nameManuallyEdited: false,
   url: '',
   transport: 'http',
   token: '',
   isTestingConnection: false,
   testResult: { kind: 'idle' },
+  originalConnection: null,
 }
 
 const addServerFormReducer = (state: AddServerFormState, action: AddServerFormAction): AddServerFormState => {
   switch (action.type) {
     case 'open-dialog':
       return { ...state, isAddDialogOpen: true }
+    case 'open-edit-dialog': {
+      // Edit prefills every field from the existing row. `nameManuallyEdited`
+      // is set so a URL change during edit doesn't clobber the existing name.
+      const url = action.server.url ?? ''
+      const transport: MCPTransportType = action.server.type === 'sse' ? 'sse' : 'http'
+      const token = action.bearerToken ?? ''
+      return {
+        ...initialState,
+        isAddDialogOpen: true,
+        editingServerId: action.server.id,
+        name: action.server.name ?? '',
+        nameManuallyEdited: true,
+        url,
+        transport,
+        token,
+        originalConnection: { url, transport, token },
+      }
+    }
     case 'reset':
       return initialState
     case 'set-name':
@@ -119,7 +148,11 @@ export type AddServerFormDeps = {
 
 export type UseAddServerFormResult = {
   isAddDialogOpen: boolean
+  /** Id of the server being edited, or null when the dialog is in Add mode. */
+  editingServerId: string | null
   openDialog: () => void
+  /** Opens the dialog in Edit mode with all fields prefilled from the existing server. */
+  openEditDialog: (server: McpServer, bearerToken: string | null) => void
   /** Closes the dialog and clears all add-form state (Cancel / Escape / overlay). */
   resetAddDialog: () => void
   name: string
@@ -134,6 +167,12 @@ export type UseAddServerFormResult = {
   testResult: TestConnectionResult | { kind: 'idle' }
   isTestingConnection: boolean
   serverCapabilities: string[]
+  /** True when a connection-affecting field (url/transport/token) differs from
+   *  the value loaded at edit-open. Always true in Add mode (no original
+   *  snapshot). Callers gate the Save-Changes test-success requirement on this
+   *  so a metadata-only edit can save without re-probing — important for OAuth
+   *  servers, whose empty-token probe would classify as `needs-oauth`. */
+  hasConnectionEdits: boolean
   testConnection: () => Promise<void>
   /** Leaving the URL field probes immediately (unless the debounce already did). */
   handleUrlBlur: () => void
@@ -171,6 +210,17 @@ export const useAddServerForm = ({
 
   const openDialog = () => dispatch({ type: 'open-dialog' })
 
+  // Open the dialog with every field prefilled from an existing server row +
+  // its on-device bearer token (null for OAuth or no-cred). The auto-detect
+  // effect will probe the prefilled URL after the standard 700ms debounce, so
+  // the user must still pass Test Connection before saving — same gate as Add.
+  const openEditDialog = (server: McpServer, bearerToken: string | null) => {
+    probeIdRef.current += 1
+    lastAutoTestedUrlRef.current = null
+    onClearDialogError()
+    dispatch({ type: 'open-edit-dialog', server, bearerToken })
+  }
+
   // Closes the Add dialog and clears all add-form state. Bumps the probe id so an
   // in-flight connection probe can't land its result after the dialog is gone.
   const resetAddDialog = () => {
@@ -281,7 +331,9 @@ export const useAddServerForm = ({
   }
 
   const changeName = (value: string) => {
-    resetConnectionTest()
+    // Name doesn't participate in the probe (only url/transport/token do), so a
+    // rename must not invalidate a passing test — otherwise Save Changes gets
+    // stuck disabled until the user re-edits a connection field or retests.
     dispatch({ type: 'set-name', value })
   }
 
@@ -302,7 +354,9 @@ export const useAddServerForm = ({
 
   return {
     isAddDialogOpen: state.isAddDialogOpen,
+    editingServerId: state.editingServerId,
     openDialog,
+    openEditDialog,
     resetAddDialog,
     name: state.name,
     url: state.url,
@@ -316,6 +370,11 @@ export const useAddServerForm = ({
     isTestingConnection: state.isTestingConnection,
     // Derived: the discovered tools live on a successful result — no separate state to keep in sync.
     serverCapabilities: state.testResult.kind === 'success' ? state.testResult.tools : [],
+    hasConnectionEdits:
+      !state.originalConnection ||
+      state.url !== state.originalConnection.url ||
+      state.transport !== state.originalConnection.transport ||
+      state.token !== state.originalConnection.token,
     testConnection,
     handleUrlBlur,
     resolveServerName,

src/hooks/use-mcp-sync.tsx

@@ -11,7 +11,7 @@ import { useEffect } from 'react'
 
 export const useMcpSync = () => {
   const db = useDatabase()
-  const { servers, addServer, removeServer, updateServerStatus } = useMCP()
+  const { servers, addServer, removeServer, updateServer } = useMCP()
 
   const { data: dbServers = [] } = useQuery({
     queryKey: ['mcp-servers'],
@@ -45,17 +45,35 @@ export const useMcpSync = () => {
         }
       }
 
-      // Update server status for existing servers
+      // Patch any row whose name/url/type/enabled diverged from the in-memory
+      // entry. `updateServer` redials when enabled so a URL or transport change
+      // actually takes effect (the previous sync only handled enable toggles,
+      // leaving editors connected to the old endpoint).
       for (const dbServer of dbServers) {
         const providerServer = servers.find((s) => s.id === dbServer.id)
-        if (providerServer && providerServer.enabled !== (dbServer.enabled === 1)) {
-          updateServerStatus(dbServer.id, dbServer.enabled === 1)
+        if (!providerServer) {
+          continue
+        }
+        const next = {
+          id: dbServer.id,
+          name: dbServer.name ?? '',
+          url: dbServer.url || '',
+          type: dbServer.type === 'sse' ? ('sse' as const) : ('http' as const),
+          enabled: dbServer.enabled === 1,
+        }
+        if (
+          providerServer.name !== next.name ||
+          providerServer.url !== next.url ||
+          providerServer.type !== next.type ||
+          providerServer.enabled !== next.enabled
+        ) {
+          updateServer(next)
         }
       }
     }
 
     syncServers()
-  }, [dbServers, servers, addServer, removeServer, updateServerStatus])
+  }, [dbServers, servers, addServer, removeServer, updateServer])
 
   return { servers, dbServers }
 }