From e30174fa173a4678cc730470e8f0aa3e19b5065a Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Sat, 18 Jul 2026 19:15:57 -0400 Subject: [PATCH] perf(desktop): scope tool-diff subscriptions + narrow profile query invalidation Two structural fixes from the Desktop performance audit (P2 tier): 1. Scope live tool-diff subscriptions. `ToolEntry` subscribed to the whole `$toolDiffs` map via `useStore`, so one `recordToolDiff` re-rendered every mounted tool row. Add a cached per-toolCallId derived atom (`$toolInlineDiff(id)`, mirroring the existing `$toolDisclosureOpen` pattern); computed() only notifies when that id's diff string changes, so a live patch re-renders one row. 2. Narrow profile / gateway-switch query invalidation. Both the active-profile subscription and `wipeSessionListsForGatewaySwitch` called keyless `queryClient.invalidateQueries()`, refetching account/marketplace/onboarding caches on every switch. Add `invalidateProfileScopedQueries()` with a denylist of profile-independent roots (billing, marketplace-themes, onboarding-model-options, contrib-logs-tail). A denylist is correctness-safe: a root we forget just refetches (cheap), whereas an allowlist that misses a profile-scoped key would paint the previous profile's data. Tests: per-tool notify isolation, and real-QueryClient invalidation partition (profile-scoped invalidated, global left intact, unknown keys invalidated). --- .../components/assistant-ui/tool/fallback.tsx | 7 ++- apps/desktop/src/lib/query-client.test.ts | 58 +++++++++++++++++++ apps/desktop/src/lib/query-client.ts | 28 +++++++++ apps/desktop/src/store/gateway-switch.test.ts | 2 +- apps/desktop/src/store/gateway-switch.ts | 6 +- apps/desktop/src/store/profile.test.ts | 8 +-- apps/desktop/src/store/profile.ts | 6 +- apps/desktop/src/store/tool-diffs.test.ts | 47 +++++++++++++++ apps/desktop/src/store/tool-diffs.ts | 19 +++++- 9 files changed, 167 insertions(+), 14 deletions(-) create mode 100644 apps/desktop/src/lib/query-client.test.ts create mode 100644 apps/desktop/src/store/tool-diffs.test.ts diff --git a/apps/desktop/src/components/assistant-ui/tool/fallback.tsx b/apps/desktop/src/components/assistant-ui/tool/fallback.tsx index ae173d2eb5a..96db51f4955 100644 --- a/apps/desktop/src/components/assistant-ui/tool/fallback.tsx +++ b/apps/desktop/src/components/assistant-ui/tool/fallback.tsx @@ -39,7 +39,7 @@ import { useEnterAnimation } from '@/lib/use-enter-animation' import { cn } from '@/lib/utils' import { recordPreviewArtifact } from '@/store/preview-status' import { $activeSessionId, $currentCwd } from '@/store/session' -import { $toolInlineDiffs } from '@/store/tool-diffs' +import { $toolInlineDiff } from '@/store/tool-diffs' import { $toolRowDismissed, dismissToolRow } from '@/store/tool-dismiss' import { $toolDisclosureOpen, $toolViewMode, setToolDisclosureOpen } from '@/store/tool-view' @@ -283,8 +283,9 @@ function ToolEntry({ part }: ToolEntryProps) { const disclosureId = `tool-entry:${messageId}:${toolPartDisclosureId(stablePart)}` const dismissed = useStore($toolRowDismissed(disclosureId)) const isPending = messageRunning && result === undefined - const liveDiffs = useStore($toolInlineDiffs) - const sideDiff = toolCallId ? liveDiffs[toolCallId] || '' : '' + // Subscribe to this tool's diff only, so a live patch for one tool doesn't + // re-render every mounted tool row (the factory caches a per-id atom). + const sideDiff = useStore($toolInlineDiff(toolCallId ?? '')) const inlineDiff = stripInlineDiffChrome(sideDiff) || inlineDiffFromResult(result) const isFileEdit = isFileEditTool(toolName) const defaultOpen = Boolean(inlineDiff) diff --git a/apps/desktop/src/lib/query-client.test.ts b/apps/desktop/src/lib/query-client.test.ts new file mode 100644 index 00000000000..c2f0cc20070 --- /dev/null +++ b/apps/desktop/src/lib/query-client.test.ts @@ -0,0 +1,58 @@ +import { beforeEach, describe, expect, it } from 'vitest' + +import { invalidateProfileScopedQueries, queryClient } from './query-client' + +function invalidated(key: unknown[]): boolean { + return queryClient.getQueryState(key)?.isInvalidated ?? false +} + +describe('invalidateProfileScopedQueries', () => { + beforeEach(() => { + queryClient.clear() + }) + + it('invalidates profile-scoped caches and leaves account/global caches intact', () => { + const profileScoped = [ + ['hermes-config-record'], + ['hermes-config-schema'], + ['skills-list'], + ['toolsets-list'], + ['model-options', 'global'], + ['command-palette', 'sessions'], + ['session-picker', 'sessions'] + ] + + const global = [ + ['billing', 'state'], + ['billing', 'subscription'], + ['marketplace-themes', 'all'], + ['marketplace-themes-settings', 'x'], + ['onboarding-model-options', 'y'], + ['contrib-logs-tail'] + ] + + for (const key of [...profileScoped, ...global]) { + queryClient.setQueryData(key, { seeded: true }) + } + + invalidateProfileScopedQueries() + + for (const key of profileScoped) { + expect(invalidated(key), `${JSON.stringify(key)} should be invalidated`).toBe(true) + } + + for (const key of global) { + expect(invalidated(key), `${JSON.stringify(key)} should be left intact`).toBe(false) + } + }) + + it('invalidates unknown/non-string-rooted keys by default (correctness-safe)', () => { + queryClient.setQueryData(['some-future-profile-query'], 1) + queryClient.setQueryData([{ scope: 'weird' }], 1) + + invalidateProfileScopedQueries() + + expect(invalidated(['some-future-profile-query'])).toBe(true) + expect(invalidated([{ scope: 'weird' }])).toBe(true) + }) +}) diff --git a/apps/desktop/src/lib/query-client.ts b/apps/desktop/src/lib/query-client.ts index dd0df19941b..cfea4f1054f 100644 --- a/apps/desktop/src/lib/query-client.ts +++ b/apps/desktop/src/lib/query-client.ts @@ -18,3 +18,31 @@ export const writeCache = (key: QueryKey) => (next: T | undefined | ((prev: T | undefined) => T | undefined)): void => void queryClient.setQueryData(key, next) + +// Query-key roots that are NOT profile-scoped: account/billing, the theme +// marketplace, onboarding, and contrib log tails all read global or +// account-level state, so a profile/gateway swap must not refetch them. Any +// other key is treated as profile-scoped and invalidated -- a denylist is +// correctness-safe here: a root we forget to list just gets refetched (a small +// cost), whereas an allowlist that misses a profile-scoped key would paint the +// previous profile's data (a bug). +const PROFILE_INDEPENDENT_QUERY_ROOTS = new Set([ + 'billing', + 'marketplace-themes', + 'marketplace-themes-settings', + 'onboarding-model-options', + 'contrib-logs-tail' +]) + +// Invalidate profile-scoped query caches on a profile / gateway switch, leaving +// account/global caches intact. Replaces a keyless invalidateQueries() that +// refetched everything (billing, marketplace, onboarding) on every switch. +export function invalidateProfileScopedQueries(): void { + void queryClient.invalidateQueries({ + predicate: query => { + const root = query.queryKey[0] + + return typeof root !== 'string' || !PROFILE_INDEPENDENT_QUERY_ROOTS.has(root) + } + }) +} diff --git a/apps/desktop/src/store/gateway-switch.test.ts b/apps/desktop/src/store/gateway-switch.test.ts index 5b24c120ccc..5513f4f4b29 100644 --- a/apps/desktop/src/store/gateway-switch.test.ts +++ b/apps/desktop/src/store/gateway-switch.test.ts @@ -19,7 +19,7 @@ import { import { $gatewaySwitching, wipeSessionListsForGatewaySwitch } from './gateway-switch' vi.mock('@/lib/query-client', () => ({ - queryClient: { invalidateQueries: vi.fn() } + invalidateProfileScopedQueries: vi.fn() })) describe('wipeSessionListsForGatewaySwitch', () => { diff --git a/apps/desktop/src/store/gateway-switch.ts b/apps/desktop/src/store/gateway-switch.ts index 0d1414adad0..2b6fb466e8e 100644 --- a/apps/desktop/src/store/gateway-switch.ts +++ b/apps/desktop/src/store/gateway-switch.ts @@ -1,6 +1,6 @@ import { atom } from 'nanostores' -import { queryClient } from '@/lib/query-client' +import { invalidateProfileScopedQueries } from '@/lib/query-client' import { resetSessionsLimit } from '@/store/layout' import { $unreadFinishedSessionIds, @@ -57,5 +57,7 @@ export function wipeSessionListsForGatewaySwitch(): void { setMessages([]) setFreshDraftReady(true) - void queryClient.invalidateQueries() + // Narrowed: account/marketplace/onboarding caches are global, not gateway- + // scoped, so a mode swap must not refetch them. + invalidateProfileScopedQueries() } diff --git a/apps/desktop/src/store/profile.test.ts b/apps/desktop/src/store/profile.test.ts index b7489fe0556..49d6186a43d 100644 --- a/apps/desktop/src/store/profile.test.ts +++ b/apps/desktop/src/store/profile.test.ts @@ -16,14 +16,14 @@ vi.mock('@/hermes', () => ({ getProfiles: vi.fn(async () => ({ profiles: [] })), setApiRequestProfile: vi.fn() })) -vi.mock('@/lib/query-client', () => ({ queryClient: { invalidateQueries: vi.fn() } })) +vi.mock('@/lib/query-client', () => ({ invalidateProfileScopedQueries: vi.fn() })) vi.mock('@/store/starmap', () => ({ resetStarmapGraph })) const { $activeGatewayProfile, $profiles, ensureGatewayProfile, prewarmProfileBackend, refreshProfiles } = await import('./profile') const { $connection } = await import('./session') -const { queryClient } = await import('@/lib/query-client') +const { invalidateProfileScopedQueries } = await import('@/lib/query-client') const { getProfiles } = await import('@/hermes') const profile = (name: string, isDefault = false): ProfileInfo => ({ @@ -53,7 +53,7 @@ beforeEach(() => { $connection.set(localConn()) $profiles.set([]) vi.stubGlobal('window', { hermesDesktop: { getConnection } }) - vi.mocked(queryClient.invalidateQueries).mockClear() + vi.mocked(invalidateProfileScopedQueries).mockClear() resetStarmapGraph.mockClear() }) @@ -114,7 +114,7 @@ describe('profile-scoped cache invalidation', () => { it('drops the memory graph cache when the active gateway profile changes', () => { $activeGatewayProfile.set('coder') - expect(queryClient.invalidateQueries).toHaveBeenCalled() + expect(invalidateProfileScopedQueries).toHaveBeenCalled() expect(resetStarmapGraph).toHaveBeenCalledTimes(1) }) }) diff --git a/apps/desktop/src/store/profile.ts b/apps/desktop/src/store/profile.ts index 1049d315311..c97614884dc 100644 --- a/apps/desktop/src/store/profile.ts +++ b/apps/desktop/src/store/profile.ts @@ -1,7 +1,7 @@ import { atom, computed } from 'nanostores' import { getProfiles, setApiRequestProfile, STARTUP_REQUEST_TIMEOUT_MS } from '@/hermes' -import { queryClient } from '@/lib/query-client' +import { invalidateProfileScopedQueries } from '@/lib/query-client' import { arraysEqual, persistBoolean, @@ -177,7 +177,9 @@ $activeGatewayProfile.subscribe(value => { if (_lastRoutedProfile !== null && _lastRoutedProfile !== key) { // Profile-scoped settings + the unified session list are now stale. - void queryClient.invalidateQueries() + // Narrowed so account/marketplace/onboarding caches don't refetch on + // every profile switch. + invalidateProfileScopedQueries() resetStarmapGraph() } diff --git a/apps/desktop/src/store/tool-diffs.test.ts b/apps/desktop/src/store/tool-diffs.test.ts new file mode 100644 index 00000000000..c4421159ad6 --- /dev/null +++ b/apps/desktop/src/store/tool-diffs.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from 'vitest' + +import { $toolInlineDiff, getToolDiff, recordToolDiff } from './tool-diffs' + +describe('tool-diffs per-tool subscriptions', () => { + it('returns a stable cached atom per toolCallId', () => { + expect($toolInlineDiff('a')).toBe($toolInlineDiff('a')) + expect($toolInlineDiff('a')).not.toBe($toolInlineDiff('b')) + }) + + it('notifies only the tool whose diff changed', () => { + const aCalls: string[] = [] + const bCalls: string[] = [] + const unsubA = $toolInlineDiff('notify-a').listen(v => aCalls.push(v)) + const unsubB = $toolInlineDiff('notify-b').listen(v => bCalls.push(v)) + + recordToolDiff('notify-a', 'diffA') + expect(aCalls).toEqual(['diffA']) + expect(bCalls).toEqual([]) // the unrelated tool row is never notified + + recordToolDiff('notify-b', 'diffB') + expect(aCalls).toEqual(['diffA']) // still not re-notified + expect(bCalls).toEqual(['diffB']) + + unsubA() + unsubB() + }) + + it('does not re-notify when the same diff is recorded again', () => { + const calls: string[] = [] + const unsub = $toolInlineDiff('same').listen(v => calls.push(v)) + + recordToolDiff('same', 'x') + recordToolDiff('same', 'x') + + expect(calls).toEqual(['x']) + unsub() + }) + + it('reads the current diff for a tool and empty for unknown/blank ids', () => { + recordToolDiff('read-me', 'value') + expect(getToolDiff('read-me')).toBe('value') + expect(getToolDiff('missing')).toBe('') + expect(getToolDiff('')).toBe('') + expect($toolInlineDiff('').get()).toBe('') + }) +}) diff --git a/apps/desktop/src/store/tool-diffs.ts b/apps/desktop/src/store/tool-diffs.ts index 01678bc21c7..d4ca7d2ee41 100644 --- a/apps/desktop/src/store/tool-diffs.ts +++ b/apps/desktop/src/store/tool-diffs.ts @@ -1,7 +1,13 @@ -import { atom } from 'nanostores' +import { atom, computed, type ReadableAtom } from 'nanostores' const $toolDiffs = atom>({}) +// Per-tool derived atoms, cached by toolCallId. A `ToolEntry` subscribes only +// to its own id's diff, so recording a diff for one tool re-renders that one +// row -- not every mounted tool row. computed() only notifies when the derived +// string actually changes, so unrelated writes to the map are inert here. +const inlineDiffCache = new Map>() + export function recordToolDiff(toolCallId: string, diff: string) { if (!toolCallId || !diff) { return @@ -20,4 +26,13 @@ export function getToolDiff(toolCallId: string): string { return toolCallId ? $toolDiffs.get()[toolCallId] || '' : '' } -export const $toolInlineDiffs = $toolDiffs +export function $toolInlineDiff(toolCallId: string): ReadableAtom { + let cached = inlineDiffCache.get(toolCallId) + + if (!cached) { + cached = computed($toolDiffs, diffs => (toolCallId ? diffs[toolCallId] || '' : '')) + inlineDiffCache.set(toolCallId, cached) + } + + return cached +}