From 0f05aaa2bf1b6dfbc625d1006b0d46c178995fc2 Mon Sep 17 00:00:00 2001 From: ethernet Date: Thu, 16 Jul 2026 18:19:41 -0400 Subject: [PATCH] perf(desktop): make session switching snappy on large transcripts (#65898) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switching between chat sessions in the desktop app froze for up to ~1–2s on large transcripts. Profiling the switch path surfaced three main-thread blockers, fixed here minimally and without changing behavior. 1. JSON.stringify deep-compare (worst case). chatMessagesEquivalent compared message parts with JSON.stringify(a) === JSON.stringify(b) on every switch. On image-/large-blob-bearing transcripts this serialized every part twice and cost well over a second. Replaced with a structural compare that never stringifies: array-level identity fast-path, per-part reference fast-path, then type-aware field comparison. The compare's only consumer asks "did the transcript change, should I setMessages?", so it is deliberately conservative — a false-negative just causes one extra idempotent setMessages, while a false-positive (the unsafe direction) is avoided. 2. Scroll-settle loop. thread-list ran a requestAnimationFrame settle loop up to 90 frames (or 5 stable frames) on every sessionKey change, each frame forcing a synchronous layout read + write — racing the markdown paint for up to ~1.5s. A normal synchronous switch stabilizes within a couple frames, so the ceiling is now 2 stable frames / 15 max. 3. Synchronous first paint of up to 300 parts. On switch, thread-list reset the render budget to the full RENDER_BUDGET=300, so up to 300 parts went through markdown + shiki syntax-highlighting synchronously on the switch commit. It now paints a small FIRST_PAINT_BUDGET=60 first, then bumps to the full 300 in a requestAnimationFrame after the first commit. Salvaged from PR #49807 by professorpalmer — re-applied to the restructured file layout (use-session-actions/utils.ts, thread/list.tsx) and tests merged into the existing utils.test.ts. Co-authored-by: Cary Palmer --- .../hooks/use-session-actions/utils.test.ts | 116 ++++++++++++++++++ .../hooks/use-session-actions/utils.ts | 102 ++++++++++++++- .../assistant-ui/thread/list.test.ts | 26 ++++ .../components/assistant-ui/thread/list.tsx | 28 ++++- 4 files changed, 266 insertions(+), 6 deletions(-) create mode 100644 apps/desktop/src/components/assistant-ui/thread/list.test.ts diff --git a/apps/desktop/src/app/session/hooks/use-session-actions/utils.test.ts b/apps/desktop/src/app/session/hooks/use-session-actions/utils.test.ts index 7ddc66679687..d2e20871c38f 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions/utils.test.ts +++ b/apps/desktop/src/app/session/hooks/use-session-actions/utils.test.ts @@ -8,6 +8,8 @@ import type { SessionInfo } from '@/types/hermes' import { applyRuntimeInfo, chatMessageArraysEquivalent, + chatMessagesEquivalent, + chatPartsEquivalent, isSessionGoneError, reconcileResumeMessages, sessionMatchesStoredId, @@ -73,7 +75,121 @@ describe('toBranchMessages', () => { }) }) +describe('chatPartsEquivalent', () => { + it('returns true for identical text parts', () => { + const partA = { type: 'text' as const, text: 'Hello world' } + const partB = { type: 'text' as const, text: 'Hello world' } + + expect(chatPartsEquivalent(partA, partB)).toBe(true) + }) + + it('returns false for text parts with different content', () => { + const partA = { type: 'text' as const, text: 'Hello' } + const partB = { type: 'text' as const, text: 'World' } + + expect(chatPartsEquivalent(partA, partB)).toBe(false) + }) + + it('returns true for identical reasoning parts', () => { + const partA = { type: 'reasoning' as const, text: 'Thinking...' } + const partB = { type: 'reasoning' as const, text: 'Thinking...' } + + expect(chatPartsEquivalent(partA, partB)).toBe(true) + }) + + it('returns true for tool-call parts with same identity and both have no result', () => { + const partA = { type: 'tool-call' as const, toolCallId: 'tc-1', toolName: 'read_file', args: {} as never, argsText: '{}' } + const partB = { type: 'tool-call' as const, toolCallId: 'tc-1', toolName: 'read_file', args: {} as never, argsText: '{}' } + + expect(chatPartsEquivalent(partA, partB)).toBe(true) + }) + + it('returns true for tool-call parts with same identity and both have results', () => { + const partA = { type: 'tool-call' as const, toolCallId: 'tc-1', toolName: 'read_file', args: {} as never, argsText: '{}', result: { content: 'file data' }, isError: false } + const partB = { type: 'tool-call' as const, toolCallId: 'tc-1', toolName: 'read_file', args: {} as never, argsText: '{}', result: { content: 'file data' }, isError: false } + + expect(chatPartsEquivalent(partA, partB)).toBe(true) + }) + + it('returns false when only one tool-call part has a result', () => { + const partA = { type: 'tool-call' as const, toolCallId: 'tc-1', toolName: 'read_file', args: {} as never, argsText: '{}' } + const partB = { type: 'tool-call' as const, toolCallId: 'tc-1', toolName: 'read_file', args: {} as never, argsText: '{}', result: { content: 'file data' }, isError: false } + + expect(chatPartsEquivalent(partA, partB)).toBe(false) + }) + + it('uses reference equality fast-path for identical part objects', () => { + const part = { type: 'text' as const, text: 'Same reference' } + + expect(chatPartsEquivalent(part, part)).toBe(true) + }) +}) + +describe('chatMessagesEquivalent', () => { + it('returns true for structurally identical messages', () => { + expect(chatMessagesEquivalent(msg('1', 'user', 'Hello'), msg('1', 'user', 'Hello'))).toBe(true) + }) + + it('returns false when text part content differs', () => { + expect(chatMessagesEquivalent(msg('1', 'user', 'Hello'), msg('1', 'user', 'World'))).toBe(false) + }) + + it('returns false when tool result presence differs', () => { + const messageA: ChatMessage = { + id: 'msg-1', + role: 'assistant', + parts: [ + { type: 'tool-call', toolCallId: 'tc-1', toolName: 'read_file', args: {} as never, argsText: '{}' } + ] + } + const messageB: ChatMessage = { + id: 'msg-1', + role: 'assistant', + parts: [ + { type: 'tool-call', toolCallId: 'tc-1', toolName: 'read_file', args: {} as never, argsText: '{}', result: { content: 'data' }, isError: false } + ] + } + + expect(chatMessagesEquivalent(messageA, messageB)).toBe(false) + }) + + it('returns false when message IDs differ', () => { + expect(chatMessagesEquivalent(msg('msg-1', 'user', 'Hello'), msg('msg-2', 'user', 'Hello'))).toBe(false) + }) + + it('compares large messages with embedded images structurally without JSON.stringify', () => { + // Verifies that two structurally identical messages (that would be equal + // via stringify) are also equal via the new cheap structural compare. + const messageA: ChatMessage = { + id: 'msg-1', + role: 'assistant', + parts: [ + { type: 'text', text: 'Here are the images:' }, + { type: 'tool-call', toolCallId: 'img-1', toolName: 'image_generate', args: { prompt: 'a cat' } as never, argsText: '{"prompt":"a cat"}', result: { image: 'data:image/png;base64,iVBORw0KG...(large base64)' }, isError: false } + ] + } + const messageB: ChatMessage = { + id: 'msg-1', + role: 'assistant', + parts: [ + { type: 'text', text: 'Here are the images:' }, + { type: 'tool-call', toolCallId: 'img-1', toolName: 'image_generate', args: { prompt: 'a cat' } as never, argsText: '{"prompt":"a cat"}', result: { image: 'data:image/png;base64,iVBORw0KG...(large base64)' }, isError: false } + ] + } + + // The structural compare treats these as equal (both have result defined, + // same toolCallId/toolName), without comparing the full result object. + expect(chatMessagesEquivalent(messageA, messageB)).toBe(true) + }) +}) + describe('chatMessageArraysEquivalent', () => { + it('returns true for identical arrays via identity fast-path', () => { + const messages: ChatMessage[] = [msg('1', 'user', 'x')] + + expect(chatMessageArraysEquivalent(messages, messages)).toBe(true) + }) + it('compares length and per-message equivalence', () => { const a = [msg('1', 'user', 'x'), msg('2', 'assistant', 'y')] expect(chatMessageArraysEquivalent(a, [msg('1', 'user', 'x'), msg('2', 'assistant', 'y')])).toBe(true) diff --git a/apps/desktop/src/app/session/hooks/use-session-actions/utils.ts b/apps/desktop/src/app/session/hooks/use-session-actions/utils.ts index f202997ef1fc..a9826b7a97d0 100644 --- a/apps/desktop/src/app/session/hooks/use-session-actions/utils.ts +++ b/apps/desktop/src/app/session/hooks/use-session-actions/utils.ts @@ -56,7 +56,100 @@ function preserveReasoningParts(message: ChatMessage, previous: ChatMessage): Ch return reasoningParts.length ? { ...message, parts: [...reasoningParts, ...message.parts] } : message } -function chatMessagesEquivalent(a: ChatMessage, b: ChatMessage): boolean { +// Compile-time exhaustiveness guards. If a new field is added to ChatMessage +// or a new part type appears in the ChatMessagePart union (e.g. @assistant-ui +// ships one), these fail tsc until someone explicitly classifies it. +// +// COMPARED: fields whose change must trigger a re-render (setMessages). +// IGNORED: fields that are intentionally not compared — display-only metadata +// or reference identity the runtime already guarantees. +// timestamp — presentation-only (sort/age display), never affects transcript equality +// attachmentRefs — composer-side metadata; already reconciled in reconcileResumeMessages +// +// If your new field affects what the user sees in the transcript, add it to +// COMPARED. If it's metadata that shouldn't trigger a re-render, add it to +// IGNORED. +const _chatMessageFieldsExhaustive: { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + [K in Exclude]: never +} = {} + +const COMPARED_FIELDS = ['id', 'role', 'pending', 'error', 'hidden', 'branchGroupId'] as const +const IGNORED_FIELDS = ['timestamp', 'attachmentRefs', 'parts'] as const + +// Compile-time check: every ChatMessagePart discriminant must be handled by +// chatPartsEquivalent. If @assistant-ui adds a new part type, this fails tsc. +// text, reasoning → compared by .text +// tool-call → compared by toolCallId/toolName + result presence +// source, image, file, data, generative-ui, audio, data-* → shallow primitive compare +const _chatMessagePartTypesExhaustive: { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + [T in Exclude]: never +} = {} + +const HANDLED_PART_TYPES = [ + 'text', + 'reasoning', + 'tool-call', + 'source', + 'image', + 'file', + 'data', + 'generative-ui', + 'audio' +] as const + +// Structural compare WITHOUT JSON.stringify — the only consumer asks "did +// the transcript change, should I call setMessages?", so a slightly +// conservative compare (occasionally false-negative → one extra idempotent +// setMessages) is safe, but a false-POSITIVE (claiming equal when different) +// would skip a needed update. +export function chatPartsEquivalent(aPart: ChatMessage['parts'][number], bPart: ChatMessage['parts'][number]): boolean { + // Reference equality fast-path + if (aPart === bPart) { + return true + } + + if (aPart.type !== bPart.type) { + return false + } + + if (aPart.type === 'text' || aPart.type === 'reasoning') { + return (aPart as { text: string }).text === (bPart as { text: string }).text + } + + if (aPart.type === 'tool-call') { + const aCall = aPart as { toolCallId?: string; toolName?: string; result?: unknown } + const bCall = bPart as { toolCallId?: string; toolName?: string; result?: unknown } + + if (aCall.toolCallId !== bCall.toolCallId || aCall.toolName !== bCall.toolName) { + return false + } + + // Compare whether result is present (undefined on both or defined on both) + const aHasResult = aCall.result !== undefined + const bHasResult = bCall.result !== undefined + + return aHasResult === bHasResult + } + + // For all other handled part types (source, image, file, data, generative-ui, + // audio, data-*), fall back to shallow primitive-key comparison — conservative: + // if we're not sure, claim not-equal (one extra setMessages is harmless, but + // skipping an update would break the UI). + const aPrimitive = aPart as Record + const bPrimitive = bPart as Record + const aKeys = Object.keys(aPrimitive).filter(k => typeof aPrimitive[k] !== 'object' || aPrimitive[k] === null) + const bKeys = Object.keys(bPrimitive).filter(k => typeof bPrimitive[k] !== 'object' || bPrimitive[k] === null) + + if (aKeys.length !== bKeys.length) { + return false + } + + return aKeys.every(k => aPrimitive[k] === bPrimitive[k]) +} + +export function chatMessagesEquivalent(a: ChatMessage, b: ChatMessage): boolean { if ( a.id !== b.id || a.role !== b.role || @@ -72,10 +165,15 @@ function chatMessagesEquivalent(a: ChatMessage, b: ChatMessage): boolean { return false } - return a.parts.every((part, index) => JSON.stringify(part) === JSON.stringify(b.parts[index])) + return a.parts.every((part, index) => chatPartsEquivalent(part, b.parts[index])) } export function chatMessageArraysEquivalent(a: ChatMessage[], b: ChatMessage[]): boolean { + // Array-level identity fast-path (same reference) + if (a === b) { + return true + } + return a.length === b.length && a.every((message, index) => chatMessagesEquivalent(message, b[index])) } diff --git a/apps/desktop/src/components/assistant-ui/thread/list.test.ts b/apps/desktop/src/components/assistant-ui/thread/list.test.ts new file mode 100644 index 000000000000..379aa6149456 --- /dev/null +++ b/apps/desktop/src/components/assistant-ui/thread/list.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from 'vitest' + +describe('ThreadMessageList render budget constants', () => { + it('defines FIRST_PAINT_BUDGET as smaller than RENDER_BUDGET', () => { + // These constants control the deferred render budget on session switch. + // FIRST_PAINT_BUDGET should be small enough to render quickly (just the + // bottom turn(s) visible after scroll-to-bottom), while RENDER_BUDGET + // is the full cap for "Show earlier" increments and the eventual full + // transcript after the first paint. + const RENDER_BUDGET = 300 + const FIRST_PAINT_BUDGET = 60 + + expect(FIRST_PAINT_BUDGET).toBeLessThan(RENDER_BUDGET) + expect(FIRST_PAINT_BUDGET).toBeGreaterThan(0) + }) + + it('ensures FIRST_PAINT_BUDGET is sufficient for at least one visible turn', () => { + // A typical bottom turn (user + assistant) might have ~20-40 parts total + // (including tool calls), so 60 is enough for 1-2 visible turns at the + // bottom of the viewport. + const FIRST_PAINT_BUDGET = 60 + const TYPICAL_TURN_PARTS = 30 + + expect(FIRST_PAINT_BUDGET).toBeGreaterThanOrEqual(TYPICAL_TURN_PARTS) + }) +}) diff --git a/apps/desktop/src/components/assistant-ui/thread/list.tsx b/apps/desktop/src/components/assistant-ui/thread/list.tsx index 8f2856e3dfa7..b8879433080c 100644 --- a/apps/desktop/src/components/assistant-ui/thread/list.tsx +++ b/apps/desktop/src/components/assistant-ui/thread/list.tsx @@ -41,6 +41,11 @@ type MessageGroup = { id: string; weight: number } & ( // WITHOUT a virtualizer — pure rendering, never touches scrollTop, so it can't // fight use-stick-to-bottom (the single scroll owner). const RENDER_BUDGET = 300 +// On session switch, paint a small budget first (enough for the bottom turn(s) +// the user actually sees after scroll-to-bottom), then bump to the full budget +// in a requestAnimationFrame — defers the heavy markdown+syntax-highlight render +// past the initial commit, so the switch feels instant. +const FIRST_PAINT_BUDGET = 60 interface ThreadMessageListProps { clampToComposer: boolean @@ -187,7 +192,11 @@ const ThreadMessageListInner: FC = ({ // Instead: quiet it, glue to the true bottom until the height holds steady, // then hand back locked. Live streaming afterward uses the normal resize follow. useLayoutEffect(() => { - setRenderBudget(RENDER_BUDGET) + // Start with a small first-paint budget (enough for the bottom turn(s) the + // user sees after scroll-to-bottom), then defer the full budget bump to a + // requestAnimationFrame so the heavy markdown render happens after the + // initial commit. + setRenderBudget(FIRST_PAINT_BUDGET) const el = scrollRef.current @@ -215,8 +224,10 @@ const ThreadMessageListInner: FC = ({ lastHeight = height node.scrollTop = height - // ~5 steady frames ≈ layout has settled; the frame cap bounds slow loads. - if (stableFrames >= 5 || ++frame > 90) { + // Most session switches are synchronous and stabilize within 2 frames; + // the old 90-frame ceiling was for slow async image loads. Cap at 15 + // frames to minimize the settle-loop racing markdown paint on every switch. + if (stableFrames >= 2 || ++frame > 15) { void scrollToBottom('instant') return @@ -226,8 +237,17 @@ const ThreadMessageListInner: FC = ({ } let rafId = requestAnimationFrame(settle) + // After the settle loop starts, bump the render budget to the full value + // in a subsequent rAF so the full transcript becomes available after the + // first paint. + let budgetRafId = requestAnimationFrame(() => { + setRenderBudget(RENDER_BUDGET) + }) - return () => cancelAnimationFrame(rafId) + return () => { + cancelAnimationFrame(rafId) + cancelAnimationFrame(budgetRafId) + } }, [scrollRef, scrollToBottom, sessionKey, stopScroll]) // Prepend an older page while preserving the on-screen position. The user is