fix(ui/store): clear missing server fields on merge (#438)

Reported by Jason Landbridge (issue #438): after clicking "Approve all
tools" on a server, navigating to the Servers overview page kept showing
the "5 pending approval" quarantine badge even though everything was
approved. A hard refresh fixed it.

Root cause is a generalisation of the previously-special-cased
last_error bug:

The Pinia stores/servers.ts mergeServers helper updates each existing
server in place via Object.assign(existingServer, incomingServer). The
backend's enrichServersWithQuarantineStats (internal/httpapi/server.go)
only sets `Quarantine` when `pending > 0 || changed > 0`; once all tools
are approved the field disappears from the JSON entirely. Object.assign
adds and overwrites — it never deletes. The stale `quarantine: { pending_count: 5 }`
therefore survived on the in-place reactive object and ServerCard's
`server.quarantine?.pending_count` kept rendering the badge until a full
page reload replaced the array.

The author had already special-cased `last_error` for this exact reason.
The same pattern affects every conditional optional field — `quarantine`,
`oauth_status`, `token_expires_at`, `user_logged_out`, `health`,
`security_scan`, `diagnostic`, `error_code`, etc. — and Jason flagged
that he sees similar staleness "in several areas of the UI."

Fix: replace the one-field special case with a general key-sync. For
each key on the existing server that is absent on the incoming server,
delete it before Object.assign. This treats the API response as
authoritative (which it is) and works for every conditional field
without enumerating them. The existing object reference is still
preserved, so v-memo on ServerCard keeps its stable identity and the
list does not visually rerender.

Tests in frontend/tests/unit/servers-store.spec.ts:
  * drops the stale quarantine field when the backend stops emitting it
  * drops last_error on recovery (regression coverage for the original
    special case the new code subsumes)
  * drops oauth_status / token_expires_at / user_logged_out together
  * preserves the existing object reference across merges (identity
    stability for v-memo)
  * sanity: still updates and adds present fields

  8 tests, all passing (5 new + 3 existing).

Note: a complementary backend change — emit `servers.changed` SSE on
tool approval — is in a separate PR so a Servers/overview page open in
another tab also reactively updates without depending on the Approve
flow's explicit fetchServers() call. This PR is the primary fix; the
SSE one is the cross-tab improvement.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: claude

frontend/src/stores/servers.ts

@@ -47,23 +47,26 @@ export const useServersStore = defineStore('servers', () => {
       const existingServer = existingMap.get(incomingServer.name)
 
       if (existingServer) {
-        // Update existing server in-place (preserves object reference)
-        // Only update properties that have changed
-        let hasChanges = false
-
-        // IMPORTANT: Clear last_error if not present in incoming server
-        // Object.assign won't clear properties that are missing from source
-        if (!('last_error' in incomingServer) && existingServer.last_error) {
-          delete existingServer.last_error
-          hasChanges = true
+        // Update existing server in-place (preserves object reference so
+        // ServerCard's v-memo / consumers' computeds keep their identity).
+        //
+        // CRITICAL: the API response is authoritative — any field absent on
+        // the incoming server has been cleared on the backend and must be
+        // dropped from the existing reactive object. Object.assign alone
+        // would leak a stale `quarantine: { pending_count: 5 }` (and other
+        // conditionally-emitted fields) after the user approves all tools,
+        // because the backend stops emitting the field when its count hits
+        // zero (see internal/httpapi/server.go enrichServersWithQuarantineStats).
+        // This was the root cause of issue #438.
+        const existingKeys = Object.keys(existingServer) as (keyof Server)[]
+        const incomingKeys = new Set(Object.keys(incomingServer))
+        for (const key of existingKeys) {
+          if (!incomingKeys.has(key)) {
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            delete (existingServer as any)[key]
+          }
         }
-
         Object.assign(existingServer, incomingServer)
-        hasChanges = true
-
-        if (hasChanges) {
-          console.log(`Server ${existingServer.name} updated with changes`)
-        }
 
         result.push(existingServer)
       } else {

frontend/tests/unit/servers-store.spec.ts

@@ -11,6 +11,135 @@ vi.mock('@/services/api', () => ({
   },
 }))
 
+describe('useServersStore — mergeServers field-clearing (issue #438)', () => {
+  beforeEach(() => {
+    setActivePinia(createPinia())
+    vi.clearAllMocks()
+  })
+
+  function mkServer(overrides: Record<string, unknown> = {}) {
+    return {
+      name: 'srv',
+      protocol: 'http' as const,
+      enabled: true,
+      quarantined: false,
+      connected: true,
+      connecting: false,
+      tool_count: 4,
+      ...overrides,
+    }
+  }
+
+  it('drops the stale `quarantine` field when the backend stops emitting it', async () => {
+    // The backend's enrichServersWithQuarantineStats sets `Quarantine` only
+    // when pending > 0 || changed > 0. After "Approve all tools" the field
+    // disappears from the JSON entirely; without a clear-on-merge path the
+    // old `pending_count: 5` would survive on the in-place reactive object
+    // and ServerCard would keep rendering the "5 pending approval" badge.
+    ;(api.getServers as any).mockResolvedValueOnce({
+      success: true,
+      data: { servers: [mkServer({ quarantine: { pending_count: 5, changed_count: 0 } })] },
+    })
+
+    const store = useServersStore()
+    await store.fetchServers()
+    expect(store.servers[0].quarantine).toEqual({ pending_count: 5, changed_count: 0 })
+
+    ;(api.getServers as any).mockResolvedValueOnce({
+      success: true,
+      data: { servers: [mkServer()] }, // no quarantine field at all
+    })
+
+    await store.fetchServers()
+    expect(store.servers[0].quarantine).toBeUndefined()
+  })
+
+  it('drops `last_error` on recovery (regression coverage for the original special case)', async () => {
+    ;(api.getServers as any).mockResolvedValueOnce({
+      success: true,
+      data: { servers: [mkServer({ last_error: 'connection refused' })] },
+    })
+
+    const store = useServersStore()
+    await store.fetchServers()
+    expect(store.servers[0].last_error).toBe('connection refused')
+
+    ;(api.getServers as any).mockResolvedValueOnce({
+      success: true,
+      data: { servers: [mkServer()] },
+    })
+
+    await store.fetchServers()
+    expect(store.servers[0].last_error).toBeUndefined()
+  })
+
+  it('drops `oauth_status`, `token_expires_at`, and `user_logged_out` when no longer present', async () => {
+    ;(api.getServers as any).mockResolvedValueOnce({
+      success: true,
+      data: {
+        servers: [
+          mkServer({
+            oauth_status: 'expired',
+            token_expires_at: '2026-04-30T00:00:00Z',
+            user_logged_out: true,
+          }),
+        ],
+      },
+    })
+    const store = useServersStore()
+    await store.fetchServers()
+
+    ;(api.getServers as any).mockResolvedValueOnce({
+      success: true,
+      data: { servers: [mkServer()] },
+    })
+    await store.fetchServers()
+
+    expect(store.servers[0].oauth_status).toBeUndefined()
+    expect(store.servers[0].token_expires_at).toBeUndefined()
+    expect(store.servers[0].user_logged_out).toBeUndefined()
+  })
+
+  it('preserves the existing object reference across merges (identity stability for v-memo)', async () => {
+    ;(api.getServers as any).mockResolvedValueOnce({
+      success: true,
+      data: { servers: [mkServer({ quarantine: { pending_count: 2, changed_count: 0 } })] },
+    })
+    const store = useServersStore()
+    await store.fetchServers()
+    const ref1 = store.servers[0]
+
+    ;(api.getServers as any).mockResolvedValueOnce({
+      success: true,
+      data: { servers: [mkServer()] },
+    })
+    await store.fetchServers()
+    const ref2 = store.servers[0]
+
+    // Same reactive object — v-memo on ServerCard still sees stable identity.
+    expect(ref2).toBe(ref1)
+    expect(ref2.quarantine).toBeUndefined()
+  })
+
+  it('still updates and adds present fields (sanity: the clear logic does not break normal merges)', async () => {
+    ;(api.getServers as any).mockResolvedValueOnce({
+      success: true,
+      data: { servers: [mkServer({ tool_count: 1 })] },
+    })
+    const store = useServersStore()
+    await store.fetchServers()
+
+    ;(api.getServers as any).mockResolvedValueOnce({
+      success: true,
+      data: { servers: [mkServer({ tool_count: 9, last_error: 'oops' })] },
+    })
+    await store.fetchServers()
+
+    expect(store.servers[0].tool_count).toBe(9)
+    expect(store.servers[0].last_error).toBe('oops')
+  })
+})
+
 describe('useServersStore — securityApproveServer (F-04)', () => {
   beforeEach(() => {
     setActivePinia(createPinia())