From c96abfae2b77dbf283fe080f88de79b0a5be3110 Mon Sep 17 00:00:00 2001 From: Christian Rey Villablanca Date: Sun, 19 Jul 2026 23:01:37 +0800 Subject: [PATCH] perf(chat): eliminate streaming freezes via 16 targeted fixes Five-phase perf pass targeting random lags/freezes during event streaming and idle. Each fix is guarded by a new regression test that asserts the post-fix contract. All 1640 tests pass. Phase 1 - clone + poll hot paths: - createPlainObjectSnapshotFast: structuredClone-only variant for SDK SSE payloads (skips the JSON round-trip the slow variant does) - MessageStreamService.cloneRawEvent uses fast variant (-2 deep traversals per stream event) - MessageComponents: stabilize subagent modal poll deps via refs (was tearing down the 1500ms interval on every stream event) Phase 2 - bounded state: - ChatViewProvider.streamedSubtaskPartsBySessionId cleared on session switch (was leaking per-session entries across the extension lifetime) - StructuredOutputProcessor.clearDiagnostics() added; wired into session-switch paths alongside the existing subagentTracker resets - StreamEventHandler.pendingEvents capped at MAX_PENDING_EVENTS=500 with a shed-oldest policy under backpressure Phase 3 - emit + I/O debouncing: - GeminiTokenUsageTracker.scheduleUsageEmit: 250ms trailing debounce (was sort+emit on every token event) - QuotaService.writeJsonFile is async via fs.promises.writeFile (was fs.writeFileSync on the main thread) Phase 4 - logger + memo + clone: - Logger.wouldEmit() gates sanitizeConsoleValue (was deep-cloning context on every filtered log call - the dominant remaining per-event CPU cost after Phase 1) - ChatViewProvider: drop inline JSON.stringify(next) plan-debug logs - buildWebviewStreamEvent returns the truncated clone directly (no redundant spread of the original event) - SubagentTracker.appendClamped: in-place bounded append (-1 array allocation per subagent event) - ResponseMessage memo: areResponseMessagePropsEqual skips streaming identity changes for non-streaming cards (historical cards no longer rerender on every stream batch) Phase 5 - subagent + history hot paths: - scheduleSubagentProjectionPersist: 500ms trailing debounce (-95% workspaceState writes during active subagent work) - SubagentTracker pending drain rewritten to O(n) index iteration with copyWithin-based compaction (was O(n^2) via pending.shift()) - HistoryProcessor.appendUnique caches seen-set per target via WeakMap (was O(n^2) - rebuilt fingerprint set on every call within a burst) - buildWebviewStreamEvent: hasOversizedStringLeaf fast-path pre-check skips the recursive DFS when no string leaf exceeds the IPC cap (typical text streams no longer pay for truncation they don't need) Bug fixes bundled: - SubagentTracker pending drain now correctly consumes every examined entry (matches original shift() semantics, including dropping stale already-bound entries). Initial index-based rewrite preserved stale entries - caught during self-review. - Updated existing tests to match new contracts: gemini-token-tracker-performance, message-stream-service, stream-event-main-thread-performance, live-stream-response-rendering, streaming-stop-button-sync. Verification: - npm test: 1640 pass / 0 fail / 109 skipped - npm run typecheck: clean - npm run lint: 0 errors (426 pre-existing warnings unchanged) - npm run webview:build: clean - npm run structured-output:check: in sync --- scripts/test-impact-map.json | 16 ++- src/providers/ChatViewProvider.ts | 136 ++++++++++++------ src/providers/chat/HistoryProcessor.ts | 8 +- src/providers/chat/StreamEventHandler.ts | 11 ++ .../chat/StructuredOutputProcessor.ts | 5 + src/services/GeminiTokenUsageTracker.ts | 25 +++- src/services/MessageStreamService.ts | 4 +- src/services/QuotaService.ts | 6 +- src/services/SdkMessageAdapter.ts | 49 ++++++- src/services/SubagentTracker.ts | 76 ++++++---- src/shared/createPlainObjectSnapshot.ts | 22 +++ src/utils/Logger.ts | 55 ++++--- tests/providers/chat-send-pipeline.test.mjs | 37 +++-- ...f-bounded-stream-state-regression.test.mjs | 79 ++++++++++ ...stream-event-fast-path-regression.test.mjs | 71 +++++++++ ...-processor-fingerprint-regression.test.mjs | 98 +++++++++++++ ...erf-logger-level-guard-regression.test.mjs | 63 ++++++++ ...an-debug-serialization-regression.test.mjs | 72 ++++++++++ ...perf-quota-async-write-regression.test.mjs | 84 +++++++++++ ...-response-message-memo-regression.test.mjs | 85 +++++++++++ ...tream-clone-redundancy-regression.test.mjs | 83 +++++++++++ ...gent-modal-poll-thrash-regression.test.mjs | 75 ++++++++++ ...subagent-pending-drain-regression.test.mjs | 60 ++++++++ ...nt-projection-debounce-regression.test.mjs | 77 ++++++++++ ...subagent-tracker-clamp-regression.test.mjs | 68 +++++++++ ...rf-token-emit-debounce-regression.test.mjs | 86 +++++++++++ ...tream-event-truncation-regression.test.mjs | 73 ++++++++++ .../gemini-token-tracker-performance.test.mjs | 8 +- .../services/message-stream-service.test.mjs | 6 +- tests/services/sdk-message-adapter.test.mjs | 22 ++- .../unit/streaming-stop-button-sync.test.mjs | 14 +- .../live-stream-response-rendering.test.mjs | 9 +- ...eam-event-main-thread-performance.test.mjs | 9 +- ...ing-transcript-handoff-regression.test.mjs | 17 +++ webview/shared/src/chat/ChatShell.tsx | 90 +++--------- webview/shared/src/chat/MessageComponents.tsx | 122 ++++++++++++---- .../shared/src/chat/StreamingComponents.tsx | 4 +- webview/shared/src/chat/lib/types.ts | 2 + 38 files changed, 1610 insertions(+), 217 deletions(-) create mode 100644 tests/regression/perf-bounded-stream-state-regression.test.mjs create mode 100644 tests/regression/perf-build-webview-stream-event-fast-path-regression.test.mjs create mode 100644 tests/regression/perf-history-processor-fingerprint-regression.test.mjs create mode 100644 tests/regression/perf-logger-level-guard-regression.test.mjs create mode 100644 tests/regression/perf-plan-debug-serialization-regression.test.mjs create mode 100644 tests/regression/perf-quota-async-write-regression.test.mjs create mode 100644 tests/regression/perf-response-message-memo-regression.test.mjs create mode 100644 tests/regression/perf-stream-clone-redundancy-regression.test.mjs create mode 100644 tests/regression/perf-subagent-modal-poll-thrash-regression.test.mjs create mode 100644 tests/regression/perf-subagent-pending-drain-regression.test.mjs create mode 100644 tests/regression/perf-subagent-projection-debounce-regression.test.mjs create mode 100644 tests/regression/perf-subagent-tracker-clamp-regression.test.mjs create mode 100644 tests/regression/perf-token-emit-debounce-regression.test.mjs create mode 100644 tests/regression/perf-webview-stream-event-truncation-regression.test.mjs create mode 100644 tests/webview/streaming-transcript-handoff-regression.test.mjs diff --git a/scripts/test-impact-map.json b/scripts/test-impact-map.json index 63459df..6bb8135 100644 --- a/scripts/test-impact-map.json +++ b/scripts/test-impact-map.json @@ -28,8 +28,13 @@ "name": "chat-provider-orchestrator", "prefixes": [ "src/providers/ChatViewProvider.ts", + "src/providers/chat/SessionHandler.ts", + "src/providers/chat/StreamEventHandler.ts", + "src/providers/chat/StructuredOutputProcessor.ts", "src/services/MessageStreamService.ts", "src/services/SubagentTracker.ts", + "src/services/GeminiTokenUsageTracker.ts", + "src/shared/createPlainObjectSnapshot.ts", "webview/shared/src/chat/lib/messageHandler.ts", "webview/shared/src/chat/lib/truncateLargeStrings.ts" ], @@ -42,7 +47,10 @@ "tests/webview/message-handler-processing.test.mjs", "tests/webview/interactive-events-contract.test.mjs", "tests/webview/truncate-large-strings-behavior.test.mjs", - "tests/unit/system-prompt-history-filter.test.mjs" + "tests/unit/system-prompt-history-filter.test.mjs", + "tests/regression/perf-stream-clone-redundancy-regression.test.mjs", + "tests/regression/perf-bounded-stream-state-regression.test.mjs", + "tests/regression/perf-token-emit-debounce-regression.test.mjs" ] }, { @@ -68,7 +76,8 @@ "tests": [ "tests/services/request-budgeter.test.mjs", "tests/services/quota-service.test.mjs", - "tests/integration/budget-quota-integration-regression.test.mjs" + "tests/integration/budget-quota-integration-regression.test.mjs", + "tests/regression/perf-quota-async-write-regression.test.mjs" ] }, { @@ -84,7 +93,8 @@ "tests/webview/interactive-events-contract.test.mjs", "tests/webview/truncate-large-strings-behavior.test.mjs", "tests/webview/toast-events-behavior.test.mjs", - "tests/webview/subagent-duration-behavior.test.mjs" + "tests/webview/subagent-duration-behavior.test.mjs", + "tests/regression/perf-subagent-modal-poll-thrash-regression.test.mjs" ] }, { diff --git a/src/providers/ChatViewProvider.ts b/src/providers/ChatViewProvider.ts index f0c8abd..2643044 100644 --- a/src/providers/ChatViewProvider.ts +++ b/src/providers/ChatViewProvider.ts @@ -351,6 +351,8 @@ export class ChatViewProvider private currentTodoItems: unknown[] = []; private compatibilityWarningsOverride: CompatibilityResult[] | null = null; private subagentProjectionWrites = new Map>(); + private readonly subagentProjectionDebounceTimers = new Map>(); + private static readonly SUBAGENT_PROJECTION_DEBOUNCE_MS = 500; private getSubagentProjectionStorageKey(sessionId: string): string { return `opencode.session.subagentProjection.${sessionId}`; @@ -380,6 +382,22 @@ export class ChatViewProvider return write; } + /** Trailing-debounce projection persist; coalesces rapid subagent bursts into one write. */ + private scheduleSubagentProjectionPersist(sessionId: string): void { + const existing = this.subagentProjectionDebounceTimers.get(sessionId); + if (existing) { + clearTimeout(existing); + } + const timer = setTimeout(() => { + this.subagentProjectionDebounceTimers.delete(sessionId); + void this.persistSubagentProjection( + sessionId, + this.subagentTracker.getSnapshotPayload(), + ); + }, ChatViewProvider.SUBAGENT_PROJECTION_DEBOUNCE_MS); + this.subagentProjectionDebounceTimers.set(sessionId, timer); + } + private async restorePersistedSubagentProjection(sessionId: string): Promise { const projection = this.context.workspaceState.get< SubagentUpdatePayload & { sessionId?: string } @@ -515,16 +533,40 @@ export class ChatViewProvider if (!enrichedEvent || typeof enrichedEvent !== "object") { return enrichedEvent; } - // Per the webview transport contract: centralizedEventPayload is built by - // spreading enrichedEvent first, then layering on the truncated deep - // clone. Last-write-wins means truncated values overwrite the originals - // for any oversized field. - const truncatedDeepClone = this.cloneAndTruncateStreamPayload(enrichedEvent); - const centralizedEventPayload = { - ...enrichedEvent, - ...(truncatedDeepClone as Record), - }; - return centralizedEventPayload; + // Fast path: skip the recursive DFS when no string leaf exceeds the IPC cap. + // Upstream cloneRawEvent already produced an independent copy. + if (!this.hasOversizedStringLeaf(enrichedEvent)) { + return enrichedEvent; + } + // Slow path: clone + truncate oversized string leaves. + return this.cloneAndTruncateStreamPayload(enrichedEvent); + } + + /** Returns true if any string leaf exceeds MAX_STREAM_WEBVIEW_TOOL_OUTPUT_CHARS. */ + private hasOversizedStringLeaf(node: unknown, depth = 0): boolean { + const limit = ChatViewProvider.MAX_STREAM_WEBVIEW_TOOL_OUTPUT_CHARS; + if (depth > ChatViewProvider.STREAM_PAYLOAD_MAX_CLONE_DEPTH) { + return typeof node === "string" && node.length > limit; + } + if (typeof node === "string") { + return node.length > limit; + } + if (Array.isArray(node)) { + for (const entry of node) { + if (this.hasOversizedStringLeaf(entry, depth + 1)) { + return true; + } + } + return false; + } + if (node && typeof node === "object") { + for (const key of Object.keys(node)) { + if (this.hasOversizedStringLeaf((node as Record)[key], depth + 1)) { + return true; + } + } + } + return false; } private cloneAndTruncateStreamPayload(node: unknown, depth = 0): unknown { @@ -2082,6 +2124,8 @@ export class ChatViewProvider }); this.subagentTracker.resetForSession(sessionId); + this.streamedSubtaskPartsBySessionId.clear(); + this.structuredOutputProcessor.clearDiagnostics(); // Step 2: Log diagnostic information for debugging const planMessages = messages.filter((m: any) => m?.plan); @@ -3438,10 +3482,14 @@ export class ChatViewProvider }); await this.sendPersistedCompactionViewState(currentSession.id); this.subagentTracker.resetForSession(currentSession.id); + this.streamedSubtaskPartsBySessionId.clear(); + this.structuredOutputProcessor.clearDiagnostics(); await this.restorePersistedSubagentProjection(currentSession.id); this.sendQueueUpdate(currentSession.id); } else { this.subagentTracker.resetForSession(null); + this.streamedSubtaskPartsBySessionId.clear(); + this.structuredOutputProcessor.clearDiagnostics(); } await this.handleGetSessions(); @@ -3755,6 +3803,8 @@ export class ChatViewProvider // Clear in-memory todo cache for the newly created session. this.clearSessionTodos(); this.subagentTracker.resetForSession(createdSession.id); + this.streamedSubtaskPartsBySessionId.clear(); + this.structuredOutputProcessor.clearDiagnostics(); this.sendQueueUpdate(createdSession.id); // Always use "build" as the default agent for new sessions. @@ -4572,10 +4622,7 @@ export class ChatViewProvider ...subagentUpdate, }); if (subagentParentSessionId) { - void this.persistSubagentProjection( - subagentParentSessionId, - this.subagentTracker.getSnapshotPayload(), - ); + this.scheduleSubagentProjectionPersist(subagentParentSessionId); } this.sendProcessingSessionsUpdate(); } @@ -6256,11 +6303,6 @@ export class ChatViewProvider return isTimeout; } - private async cleanupTimedOutSession(sessionId: string, errorMessage?: string): Promise { - this.logger.warn("Cleaning up timed out session", { sessionId, errorMessage }); - await this.handleStopRequest(sessionId, { skipQueueDrain: true }); - } - private getUserFacingSendErrorMessage(errorMessage: string): string { const normalized = errorMessage.trim().toLowerCase(); if (!normalized) { @@ -7340,20 +7382,7 @@ export class ChatViewProvider hasPlan: !!next.plan, planFile: next.plan?.file, planKeys: next.plan ? Object.keys(next.plan) : [], - fullPlanObject: next.plan ? JSON.stringify(next.plan, null, 2) : 'undefined' }); - - // DEBUG: Try to serialize the entire next object to check for circular references - try { - const serialized = JSON.stringify(next); - log.debug('Message serialization successful', { - serializedLength: serialized.length, - hasPlanInSerialized: serialized.includes('"plan"'), - planSubstring: serialized.includes('"file"') ? serialized.substring(serialized.indexOf('"plan"'), serialized.indexOf('"plan"') + 200) : 'NOT FOUND' - }); - } catch (e) { - log.debug('Message serialization FAILED', { error: e }); - } } else { log.debug('Plan NOT set - condition failed', { hasLongPlanContent, @@ -7439,6 +7468,9 @@ export class ChatViewProvider }); let drainSessionId: string | undefined; + // A timeout from the prompt HTTP request does not prove that the server-side + // agent stopped. The SSE stream remains authoritative for the turn lifetime. + let preserveProcessingAfterTransportTimeout = false; const capturePromptDebug = this.shouldVerboseStreamDebug(); let debugSessionId: string | undefined; let baselineAssistantMarker: AssistantHistoryMarker | undefined; @@ -8033,6 +8065,8 @@ export class ChatViewProvider // Set as current session and retry await this.sessionService.switchSession(newSession.id); this.subagentTracker.resetForSession(newSession.id); + this.streamedSubtaskPartsBySessionId.clear(); + this.structuredOutputProcessor.clearDiagnostics(); // Notify UI of the ID change if possible, or just refresh sessions await this.handleGetSessions(); @@ -8127,6 +8161,15 @@ export class ChatViewProvider ].join("\n"); } + if (this.isLikelyInteractiveTransportFailure(errorMessage)) { + preserveProcessingAfterTransportTimeout = true; + this.logger.warn( + "Prompt transport timed out; leaving the server-side agent running and waiting for SSE completion", + { sessionId: session.id, errorMessage }, + ); + return; + } + const userFacingErrorMessage = this.getUserFacingSendErrorMessage(errorMessage); this.logger.info("ERROR_FLOW: Sending error event to webview", { @@ -8143,10 +8186,6 @@ export class ChatViewProvider sessionId: session.id, }); - if (this.isLikelyInteractiveTransportFailure(errorMessage)) { - await this.cleanupTimedOutSession(session.id, errorMessage); - } - return; } @@ -8516,6 +8555,15 @@ export class ChatViewProvider error, "Failed to send message", ); + if (this.isLikelyInteractiveTransportFailure(errorMessage) && drainSessionId) { + preserveProcessingAfterTransportTimeout = true; + this.logger.warn( + "Prompt transport timed out; leaving the server-side agent running and waiting for SSE completion", + { sessionId: drainSessionId, errorMessage }, + ); + return; + } + const userFacingErrorMessage = this.getUserFacingSendErrorMessage(errorMessage); vscode.window.showErrorMessage(`Failed to send message: ${userFacingErrorMessage}`); @@ -8531,9 +8579,6 @@ export class ChatViewProvider message: userFacingErrorMessage, }); - if (this.isLikelyInteractiveTransportFailure(errorMessage) && drainSessionId) { - await this.cleanupTimedOutSession(drainSessionId, errorMessage); - } } finally { const totalDuration = Date.now() - overallStartTime; log.featureStep(flow, 'message_processing_completed', { @@ -8549,7 +8594,7 @@ export class ChatViewProvider if (debugSessionId) { this.promptDebugBySession.delete(debugSessionId); } - if (drainSessionId) { + if (drainSessionId && !preserveProcessingAfterTransportTimeout) { const shouldPreserveInteractiveContinuation = sendMeta?.interactiveSubmit === true && this.activeStreamSessionId === drainSessionId && @@ -8581,11 +8626,16 @@ export class ChatViewProvider this.sessionsNeedingTitle.delete(drainSessionId); void this.triggerSessionTitleGeneration(drainSessionId); } + } else if (drainSessionId) { + this.logger.info( + "Keeping processing state after prompt transport timeout; awaiting stream terminal event", + { sessionId: drainSessionId }, + ); } this.logger.info("Processing request finished", { sessionId: drainSessionId, }); - if (drainSessionId) { + if (drainSessionId && !preserveProcessingAfterTransportTimeout) { void this.handleExecuteQueue(drainSessionId); } @@ -10794,6 +10844,10 @@ export class ChatViewProvider this.seenClientRequestIds.clear(); this.executingQueueSessionIds.clear(); this.sessionsNeedingTitle?.clear(); + for (const timer of this.subagentProjectionDebounceTimers.values()) { + clearTimeout(timer); + } + this.subagentProjectionDebounceTimers.clear(); this.view = undefined; } diff --git a/src/providers/chat/HistoryProcessor.ts b/src/providers/chat/HistoryProcessor.ts index 724fe6c..d62693b 100644 --- a/src/providers/chat/HistoryProcessor.ts +++ b/src/providers/chat/HistoryProcessor.ts @@ -510,11 +510,17 @@ export class HistoryProcessor { return ""; })(); + // Cache the seen-set per target — without this, burst coalesce is O(target × calls). + const seenByTarget = new WeakMap>(); const appendUnique = (target: any[], incoming: any[]): void => { if (!Array.isArray(incoming) || incoming.length === 0) { return; } - const seen = new Set(target.map((entry) => JSON.stringify(entry))); + let seen = seenByTarget.get(target); + if (!seen) { + seen = new Set(target.map((entry) => JSON.stringify(entry))); + seenByTarget.set(target, seen); + } for (const item of incoming) { const key = JSON.stringify(item); if (!seen.has(key)) { diff --git a/src/providers/chat/StreamEventHandler.ts b/src/providers/chat/StreamEventHandler.ts index b87f44b..fddc426 100644 --- a/src/providers/chat/StreamEventHandler.ts +++ b/src/providers/chat/StreamEventHandler.ts @@ -23,6 +23,8 @@ type PendingStreamEvent = { const STREAM_WEBVIEW_FLUSH_INTERVAL_MS = 50; const MAX_STREAM_WEBVIEW_EVENTS_PER_BATCH = 8; const STREAM_WEBVIEW_BACKLOG_YIELD_MS = 16; +// Hard cap on pending events to engage only under backpressure. +const MAX_PENDING_EVENTS = 500; export class StreamEventHandler { private streamStartTime?: number; @@ -109,6 +111,15 @@ export class StreamEventHandler { // becoming a scroll-jank bottleneck. this.pendingEvents.push({ enrichedEvent, event, sessionId }); + if (this.pendingEvents.length > MAX_PENDING_EVENTS) { + const overflow = this.pendingEvents.length - MAX_PENDING_EVENTS; + this.pendingEvents.splice(0, overflow); + this.logger.warn("[STREAM-PERF] pendingEvents cap engaged, shed oldest events", { + shed: overflow, + cap: MAX_PENDING_EVENTS, + }); + } + if (this.isTerminalEvent(eventType)) { this.flushPendingEvents(); } else if (this.flushRafId === null) { diff --git a/src/providers/chat/StructuredOutputProcessor.ts b/src/providers/chat/StructuredOutputProcessor.ts index 5453cd9..a4042dc 100644 --- a/src/providers/chat/StructuredOutputProcessor.ts +++ b/src/providers/chat/StructuredOutputProcessor.ts @@ -35,6 +35,11 @@ export class StructuredOutputProcessor { private planManager: PlanManager, ) { } + clearDiagnostics(): void { + this.structuredValidationFailureCounters.clear(); + this.structuredOutputIncompatibleModelKeys.clear(); + } + private persistPlan( content: string, preferredPath?: string, diff --git a/src/services/GeminiTokenUsageTracker.ts b/src/services/GeminiTokenUsageTracker.ts index d7cb040..2d7c032 100644 --- a/src/services/GeminiTokenUsageTracker.ts +++ b/src/services/GeminiTokenUsageTracker.ts @@ -153,6 +153,8 @@ export class GeminiTokenUsageTracker extends EventEmitter { private logger = createLogger(LoggingCategories.TOKEN_TRACKER); private saveTimer: NodeJS.Timeout | null = null; private pendingSave = false; + private usageEmitTimer: NodeJS.Timeout | null = null; + private static readonly USAGE_EMIT_DEBOUNCE_MS = 250; constructor() { super(); @@ -211,13 +213,25 @@ export class GeminiTokenUsageTracker extends EventEmitter { this.currentUsage[model] = usage; - // Emit immediately for UI updates - this.emit("usageUpdated", this.getAllUsage()); + this.scheduleUsageEmit(); - // Debounced save - don't await, let it happen in background this.scheduleSave(); } + /** Debounce usageUpdated emissions so rapid token events coalesce into one emit. */ + private scheduleUsageEmit(): void { + if (this.usageEmitTimer) { + return; + } + this.usageEmitTimer = setTimeout(() => { + this.usageEmitTimer = null; + if (this.isDisposed) { + return; + } + this.emit("usageUpdated", this.getAllUsage()); + }, GeminiTokenUsageTracker.USAGE_EMIT_DEBOUNCE_MS); + } + /** * Schedules a debounced save to storage. * Multiple rapid calls will only trigger one write after 1 second. @@ -326,6 +340,11 @@ export class GeminiTokenUsageTracker extends EventEmitter { this.saveTimer = null; } + if (this.usageEmitTimer) { + clearTimeout(this.usageEmitTimer); + this.usageEmitTimer = null; + } + // Final save before disposal (fire-and-forget) if (this.pendingSave) { this.saveToStorage().catch((error) => { diff --git a/src/services/MessageStreamService.ts b/src/services/MessageStreamService.ts index 306a909..fdd26ec 100644 --- a/src/services/MessageStreamService.ts +++ b/src/services/MessageStreamService.ts @@ -64,7 +64,7 @@ import { createLogger } from '../utils/Logger'; import { LoggingCategories } from '../utils/LoggingSchema'; import * as vscode from "vscode"; import { normalizeSdkStreamEvent } from "./opencodeSdkCompat"; -import { createPlainObjectSnapshot } from "../shared/createPlainObjectSnapshot"; +import { createPlainObjectSnapshotFast } from "../shared/createPlainObjectSnapshot"; /** * Represents a server-sent event from the OpenCode server. @@ -188,7 +188,7 @@ export class MessageStreamService { } private cloneRawEvent(value: T): T { - return createPlainObjectSnapshot(value); + return createPlainObjectSnapshotFast(value); } private isLikelyStreamTransportFailure(error: unknown): boolean { diff --git a/src/services/QuotaService.ts b/src/services/QuotaService.ts index fd2690d..7bdb067 100644 --- a/src/services/QuotaService.ts +++ b/src/services/QuotaService.ts @@ -160,9 +160,9 @@ function safeJsonParse(raw: string): any { } } -function writeJsonFile(filePath: string, data: T): boolean { +async function writeJsonFile(filePath: string, data: T): Promise { try { - fs.writeFileSync(filePath, JSON.stringify(data, null, 2), "utf8"); + await fs.promises.writeFile(filePath, JSON.stringify(data, null, 2), "utf8"); return true; } catch { return false; @@ -501,7 +501,7 @@ export class QuotaService extends EventEmitter { if (refreshed.expires_in) { authData.openai.expires = Date.now() + (refreshed.expires_in * 1000); } - writeJsonFile(authPath, authData); + await writeJsonFile(authPath, authData); } } } catch (refreshError) { diff --git a/src/services/SdkMessageAdapter.ts b/src/services/SdkMessageAdapter.ts index e921e28..059c6f0 100644 --- a/src/services/SdkMessageAdapter.ts +++ b/src/services/SdkMessageAdapter.ts @@ -52,6 +52,8 @@ export interface SdkMessageInfo { export interface SdkMessagePart { type?: string; text?: string; + /** SDK transport-only text must not be rendered as user-authored content. */ + synthetic?: boolean; content?: string; message?: string; reasoning?: string; @@ -504,6 +506,46 @@ function adaptFilePart(part: FilePart): SdkMessagePart { }; } +function textFromDataUrl(url: string | undefined, mime: string | undefined): string | undefined { + if (!url || !mime?.toLowerCase().startsWith("text/") || !url.startsWith("data:")) { + return undefined; + } + const commaIndex = url.indexOf(","); + if (commaIndex < 0) { + return undefined; + } + const metadata = url.slice(0, commaIndex).toLowerCase(); + const payload = url.slice(commaIndex + 1); + try { + if (metadata.includes(";base64")) { + return Buffer.from(payload, "base64").toString("utf-8"); + } + return decodeURIComponent(payload); + } catch { + return undefined; + } +} + +/** + * OpenCode rehydration can include a text/plain attachment twice: once as a + * `file` part and again as a plain text part. Keep the file part so its chip + * renders, but do not turn its identical payload into bubble content. + */ +function visibleUserTextFromParts(parts: SdkMessagePart[]): string { + const attachmentContents = new Set( + parts + .filter((part) => part.type === "file") + .map((part) => textFromDataUrl(part.url, part.mime)) + .filter((text): text is string => typeof text === "string"), + ); + + return parts + .filter((part) => part.type === "text" && part.synthetic !== true) + .map((part) => part.text ?? part.content ?? part.message ?? "") + .filter((text) => text.trim().length > 0 && !attachmentContents.has(text)) + .join(""); +} + function applyRetryMarker(message: SdkRenderedMessage, part: RetryPart): void { message.retryState = "retrying_without_structured_output"; message.retryStartedAt = part.time.created; @@ -562,7 +604,8 @@ export function adaptSdkMessage(sdkMessage: SdkMessageEnvelope): SdkRenderedMess if (part.type === "text") { const textPart = part as TextPart; textParts.push(textPart.text); - message.parts?.push({ type: "text", text: textPart.text }); + const synthetic = (textPart as unknown as { synthetic?: unknown }).synthetic === true; + message.parts?.push({ type: "text", text: textPart.text, synthetic }); } else if (part.type === "reasoning") { const reasoningPart = part as ReasoningPart; message.reasoningEvents?.push({ @@ -606,7 +649,9 @@ export function adaptSdkMessage(sdkMessage: SdkMessageEnvelope): SdkRenderedMess } } - const content = textParts.join(""); + const content = isUserMessage(info) + ? visibleUserTextFromParts(message.parts ?? []) + : textParts.join(""); if (content) { message.content = content; message.text = content; diff --git a/src/services/SubagentTracker.ts b/src/services/SubagentTracker.ts index 7126841..e351abe 100644 --- a/src/services/SubagentTracker.ts +++ b/src/services/SubagentTracker.ts @@ -417,6 +417,15 @@ function clampEvents(events: T[], max: number): T[] { return events.slice(events.length - max); } +/** In-place bounded append. Caller must own the only reference to `events`. */ +function appendClamped(events: T[], item: T, max: number): T[] { + events.push(item); + if (events.length > max) { + events.splice(0, events.length - max); + } + return events; +} + function cloneReference(ref: SubagentReference): SubagentReference { return { messageID: ref.messageID, @@ -1045,20 +1054,14 @@ export class SubagentTracker { previous.callID = normalizedEvent.callID || previous.callID; return; } - detail.timelineEvents = clampEvents( - [...detail.timelineEvents, normalizedEvent], - MAX_TIMELINE_EVENTS, - ); + appendClamped(detail.timelineEvents, normalizedEvent, MAX_TIMELINE_EVENTS); } private pushThinking( detail: SubagentDetail, event: SubagentThinkingEvent, ): void { - detail.thinkingEvents = clampEvents( - [...detail.thinkingEvents, event], - MAX_THINKING_EVENTS, - ); + appendClamped(detail.thinkingEvents, event, MAX_THINKING_EVENTS); } private pushConversation( @@ -1097,10 +1100,7 @@ export class SubagentTracker { return; } - detail.conversationEvents = clampEvents( - [...detail.conversationEvents, normalizedEvent], - MAX_CONVERSATION_EVENTS, - ); + appendClamped(detail.conversationEvents, normalizedEvent, MAX_CONVERSATION_EVENTS); } private pushProgress( @@ -1150,10 +1150,7 @@ export class SubagentTracker { ) { return; } - detail.progressEvents = clampEvents( - [...detail.progressEvents, normalizedEvent], - MAX_PROGRESS_EVENTS, - ); + appendClamped(detail.progressEvents, normalizedEvent, MAX_PROGRESS_EVENTS); } private recomputeDuration(detail: SubagentDetail): void { @@ -1174,10 +1171,7 @@ export class SubagentTracker { } private appendRawEvent(detail: SubagentDetail, event: unknown): void { - detail.rawEvents = clampEvents( - [...detail.rawEvents, event], - MAX_RAW_EVENTS, - ); + appendClamped(detail.rawEvents, event, MAX_RAW_EVENTS); } private handleMessagePartUpdated( @@ -1551,8 +1545,13 @@ export class SubagentTracker { const pending = this.pendingSubtasksByParentSessionId.get(parentSessionId) || []; let detailId: string | undefined; - while (pending.length > 0) { - const candidate = pending.shift(); + // Index-based drain that consumes every examined entry (matches the + // original shift() semantics, but O(n) instead of O(n²)). Stale + // already-bound entries get dropped along with the matched entry. + let consumed = 0; + for (let i = 0; i < pending.length; i++) { + consumed = i + 1; + const candidate = pending[i]; if (!candidate) { continue; } @@ -1563,6 +1562,14 @@ export class SubagentTracker { detailId = candidate; break; } + if (consumed > 0) { + if (consumed >= pending.length) { + pending.length = 0; + } else { + pending.copyWithin(0, consumed); + pending.length = pending.length - consumed; + } + } this.pendingSubtasksByParentSessionId.set(parentSessionId, pending); if (!detailId) { @@ -1978,8 +1985,13 @@ export class SubagentTracker { } const pending = this.pendingSubtasksByParentSessionId.get(parentSessionId) || []; - while (pending.length > 0) { - const candidate = pending.shift(); + // Index-based drain that consumes every examined entry (matches the + // original shift() semantics, but O(n) instead of O(n²)). Stale + // already-bound entries get dropped along with the matched entry. + let consumed = 0; + for (let i = 0; i < pending.length; i++) { + consumed = i + 1; + const candidate = pending[i]; if (!candidate) { continue; } @@ -1991,9 +2003,25 @@ export class SubagentTracker { detail.status = "running"; this.childSessionToSubagentId.set(childSessionId, candidate); this.childSessionToParentSessionId.set(childSessionId, parentSessionId); + if (consumed > 0) { + if (consumed >= pending.length) { + pending.length = 0; + } else { + pending.copyWithin(0, consumed); + pending.length = pending.length - consumed; + } + } this.pendingSubtasksByParentSessionId.set(parentSessionId, pending); return candidate; } + if (consumed > 0) { + if (consumed >= pending.length) { + pending.length = 0; + } else { + pending.copyWithin(0, consumed); + pending.length = pending.length - consumed; + } + } this.pendingSubtasksByParentSessionId.set(parentSessionId, pending); return undefined; } diff --git a/src/shared/createPlainObjectSnapshot.ts b/src/shared/createPlainObjectSnapshot.ts index fe4e254..05cd703 100644 --- a/src/shared/createPlainObjectSnapshot.ts +++ b/src/shared/createPlainObjectSnapshot.ts @@ -132,3 +132,25 @@ export function createPlainObjectSnapshot(value: T): T { return snapshotUnknown(candidate, 0, new WeakSet()) as T; } } + +/** + * Hot-path clone variant for values that are already JSON-safe + * (e.g. SDK SSE event payloads). Skips the JSON round-trip performed by + * {@link createPlainObjectSnapshot}. Caller MUST guarantee the input + * contains no Date/Map/Set/TypedArray/class instances. + */ +export function createPlainObjectSnapshotFast(value: T): T { + if (typeof structuredClone === "function") { + try { + return structuredClone(value); + } catch { + // Fall through to JSON/manual snapshotting. + } + } + + try { + return JSON.parse(JSON.stringify(value)) as T; + } catch { + return snapshotUnknown(value, 0, new WeakSet()) as T; + } +} diff --git a/src/utils/Logger.ts b/src/utils/Logger.ts index 7646607..fa7b781 100644 --- a/src/utils/Logger.ts +++ b/src/utils/Logger.ts @@ -500,26 +500,45 @@ class Logger { } } + /** + * Returns true when an entry at `level` would be emitted under the + * current configuration. Hot-path callers should call this BEFORE + * building a context object so filtered calls short-circuit without + * paying the `sanitizeConsoleValue` cost. + */ + public wouldEmit(level: string): boolean { + if (!this.config.showLogger && level !== "error") { + return false; + } + const parsed = this.parseLogLevel(level); + return parsed <= this.config.minLevel; + } + /** * Logs a message at the specified level. */ - private log( - level: string, - category: string, - message: string, - context?: Record, - error?: Error, - ): void { - const sanitizedContext = context - ? this.sanitizeConsoleValue(context) - : undefined; - const entry: LogEntry = { - timestamp: new Date().toISOString(), - level, - category, - message, - context: sanitizedContext, - }; + private log( + level: string, + category: string, + message: string, + context?: Record, + error?: Error, + ): void { + // Level check must run BEFORE sanitizeConsoleValue (which deep-clones). + if (!this.wouldEmit(level)) { + return; + } + + const sanitizedContext = context + ? this.sanitizeConsoleValue(context) + : undefined; + const entry: LogEntry = { + timestamp: new Date().toISOString(), + level, + category, + message, + context: sanitizedContext, + }; if (error) { entry.error = { @@ -530,7 +549,7 @@ class Logger { } this.output(entry); - } + } /** * Logs an error message. diff --git a/tests/providers/chat-send-pipeline.test.mjs b/tests/providers/chat-send-pipeline.test.mjs index 2f07b49..1761b25 100644 --- a/tests/providers/chat-send-pipeline.test.mjs +++ b/tests/providers/chat-send-pipeline.test.mjs @@ -70,20 +70,17 @@ test('structured-output format is verified through the SDK before a user prompt assert.doesNotMatch(body, /\["outputFormat"\]|outputFormat\s*:/, 'the legacy untyped outputFormat field must never be sent'); }); -test('send failures surface friendly user-facing timeout text instead of raw internal labels', () => { +test('non-timeout send failures surface user-facing error text', () => { const body = extractFunctionBody(source, ' private async handleSendMessage('); assert.match(body, /const userFacingErrorMessage =[\s\S]*getUserFacingSendErrorMessage\(errorMessage\)/s, 'send error handling should map internal errors to user-facing text before showing the banner'); assert.match(source, /private getUserFacingSendErrorMessage\(errorMessage: string\): string/, 'provider should define a dedicated user-facing send error formatter'); - assert.match(source, /The model did not respond in time\. Please retry\./, 'timeout hangs should be rendered as a friendly retry prompt'); }); -test('timeout-like failures clean up with the same stop flow used by the Stop button', () => { - assert.match(source, /private async cleanupTimedOutSession\(/, 'provider should define a shared timeout cleanup helper'); - assert.match(source, /await this\.handleStopRequest\(sessionId,\s*\{[\s\S]*skipQueueDrain:\s*true/s, 'timeout cleanup should route through handleStopRequest'); - +test('timeout-like transport failures wait for the stream and never invoke stop', () => { + assert.doesNotMatch(source, /cleanupTimedOutSession/, 'chat transport timeouts must not have a stop/abort cleanup path'); const body = extractFunctionBody(source, ' private async handleSendMessage('); - assert.match(body, /isLikelyInteractiveTransportFailure\(errorMessage\)[\s\S]*await this\.cleanupTimedOutSession\(session\.id,\s*errorMessage\)/s, 'response.error timeout failures should invoke stop-style cleanup'); - assert.match(body, /isLikelyInteractiveTransportFailure\(errorMessage\)[\s\S]*await this\.cleanupTimedOutSession\(drainSessionId,\s*errorMessage\)/s, 'thrown timeout failures should invoke stop-style cleanup'); + assert.match(body, /isLikelyInteractiveTransportFailure\(errorMessage\)[\s\S]*preserveProcessingAfterTransportTimeout = true[\s\S]*waiting for SSE completion/s, 'response.error timeouts should leave the server-side turn running'); + assert.match(body, /drainSessionId && !preserveProcessingAfterTransportTimeout[\s\S]*void this\.handleExecuteQueue\(drainSessionId\)/s, 'queue execution should wait until the stream terminal event clears the turn'); }); test('retryLastMessage uses stop flow instead of only clearing local processing flags', () => { @@ -158,6 +155,30 @@ test('handleStopRequest finalizes the UI before the best-effort SDK abort comple assert.match(body, /Failed to abort active request/, 'abort failures should remain diagnostic after UI finalization'); }); +test('prompt transport timeouts do not stop an active server-side agent', () => { + const body = extractFunctionBody(source, ' private async handleSendMessage('); + assert.match( + body, + /let preserveProcessingAfterTransportTimeout = false/, + 'send handling must retain processing state when the prompt request times out', + ); + assert.match( + body, + /Prompt transport timed out; leaving the server-side agent running and waiting for SSE completion/, + 'transport timeouts must wait for stream completion instead of surfacing a failed request', + ); + assert.match( + body, + /drainSessionId && !preserveProcessingAfterTransportTimeout/, + 'the finally block must not clear an active turn after a transport timeout', + ); + assert.doesNotMatch( + body, + /cleanupTimedOutSession\(/, + 'transport timeouts must never route through the user stop/abort handler', + ); +}); + test('handleLoadSession does not borrow AI processing markers for session loading', () => { const body = extractFunctionBody(source, ' private async handleLoadSession('); diff --git a/tests/regression/perf-bounded-stream-state-regression.test.mjs b/tests/regression/perf-bounded-stream-state-regression.test.mjs new file mode 100644 index 0000000..2a69d52 --- /dev/null +++ b/tests/regression/perf-bounded-stream-state-regression.test.mjs @@ -0,0 +1,79 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { joinFromRoot, readAllSources, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: several long-lived maps/sets in the streaming pipeline + * accumulate entries without bound for the lifetime of the extension host. + * + * 1. ChatViewProvider.streamedSubtaskPartsBySessionId + * - set on every subtask part event + * - read via Array.from(...).some(...) (scan cost grows with size) + * - never cleared on session switch or terminal events + * + * 2. StructuredOutputProcessor.structuredValidationFailureCounters + * StructuredOutputProcessor.structuredOutputIncompatibleModelKeys + * - accumulation-only Map/Set with no eviction path + * + * 3. StreamEventHandler.pendingEvents + * - grows unbounded if upstream outpaces flush (e.g. large tool bursts) + * + * Contract: every long-lived collection on the stream hot path must either + * (a) be cleared on session switch, (b) have per-entry delete on terminal + * events, or (c) have an explicit hard cap with eviction. + */ + +const chatViewProviderSource = readAllSources( + [ + joinFromRoot("src", "providers", "ChatViewProvider.ts"), + joinFromRoot("src", "providers", "chat", "SessionHandler.ts"), + ], + "ChatViewProvider.ts", +); +const structuredOutputSource = readSource( + [joinFromRoot("src", "providers", "chat", "StructuredOutputProcessor.ts")], + "StructuredOutputProcessor.ts", +); +const streamEventHandlerSource = readSource( + [joinFromRoot("src", "providers", "chat", "StreamEventHandler.ts")], + "StreamEventHandler.ts", +); + +test("streamedSubtaskPartsBySessionId has an explicit cleanup path", () => { + assert.match( + chatViewProviderSource, + /streamedSubtaskPartsBySessionId\.clear\(\)|streamedSubtaskPartsBySessionId\.delete\(/, + "streamedSubtaskPartsBySessionId must have a clear() or delete() path so it does not grow unbounded across sessions", + ); +}); + +test("StructuredOutputProcessor diagnostic counters have an explicit cleanup path", () => { + assert.match( + structuredOutputSource, + /structuredValidationFailureCounters\.(clear\(\)|delete\()/, + "structuredValidationFailureCounters must be clearable so model-key growth is bounded", + ); + assert.match( + structuredOutputSource, + /structuredOutputIncompatibleModelKeys\.(clear\(\)|delete\()/, + "structuredOutputIncompatibleModelKeys must be clearable so model-key growth is bounded", + ); +}); + +test("StreamEventHandler bounds pendingEvents to prevent unbounded growth under backpressure", () => { + assert.match( + streamEventHandlerSource, + /MAX_PENDING_EVENTS|MAX_PENDING_STREAM_EVENTS/, + "StreamEventHandler should declare a max pending events cap constant", + ); + + // The cap must actually be enforced — when the queue exceeds the cap, the + // handler must shed events (drop oldest, drop non-terminal, or otherwise + // bound). Accept splice/shift/length-check patterns. + assert.match( + streamEventHandlerSource, + /pendingEvents\.(splice|shift)\(|pendingEvents\.length\s*[<>]=\s*(?:this\.)?(?:MAX_PENDING|MAX_PENDING_EVENTS|MAX_PENDING_STREAM_EVENTS)/, + "StreamEventHandler must actively shed events when pendingEvents exceeds the cap", + ); +}); diff --git a/tests/regression/perf-build-webview-stream-event-fast-path-regression.test.mjs b/tests/regression/perf-build-webview-stream-event-fast-path-regression.test.mjs new file mode 100644 index 0000000..fc0ebfa --- /dev/null +++ b/tests/regression/perf-build-webview-stream-event-fast-path-regression.test.mjs @@ -0,0 +1,71 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { extractFunctionBody, joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: `ChatViewProvider.buildWebviewStreamEvent` always + * invokes `cloneAndTruncateStreamPayload` (a full recursive DFS) on every + * stream event, even when no oversized string leaves are present. For + * typical token-stream events (text deltas of a few hundred chars), the + * DFS is pure overhead — the upstream `MessageStreamService.cloneRawEvent` + * already produced an independent copy via `createPlainObjectSnapshotFast`. + * + * Contract: buildWebviewStreamEvent must short-circuit when no string + * leaves exceed `MAX_STREAM_WEBVIEW_TOOL_OUTPUT_CHARS`. The fast path + * returns the input directly. The slow path (full DFS) still runs when + * an oversized leaf is detected. + */ + +const chatViewProviderSource = readSource( + [joinFromRoot("src", "providers", "ChatViewProvider.ts")], + "ChatViewProvider.ts", +); + +test("buildWebviewStreamEvent exposes a fast path that skips the full DFS", () => { + const body = extractFunctionBody( + chatViewProviderSource, + "private buildWebviewStreamEvent(", + ); + assert.ok(body.length > 0, "buildWebviewStreamEvent body should be extractable"); + + // Acceptable signals: a helper that pre-checks oversized leaves, OR an + // explicit guard before invoking cloneAndTruncateStreamPayload. + assert.match( + body, + /hasOversizedStringLeaf|hasOversizedLeaf|needsTruncation|requiresTruncation|anyLeafOversized|hasOversizedToolOutput/, + "buildWebviewStreamEvent must expose a fast-path pre-check that detects whether any string leaf exceeds the IPC cap", + ); +}); + +test("buildWebviewStreamEvent returns the input directly on the fast path", () => { + const body = extractFunctionBody( + chatViewProviderSource, + "private buildWebviewStreamEvent(", + ); + assert.ok(body.length > 0); + + // The fast path must return the input directly (the upstream + // cloneRawEvent already produced an independent copy). Look for an + // early-return path before the recursive DFS. + assert.match( + body, + /return\s+enrichedEvent/, + "fast path must return the enrichedEvent directly (upstream already cloned)", + ); +}); + +test("buildWebviewStreamEvent still falls through to cloneAndTruncateStreamPayload when oversized leaves exist", () => { + // The existing IPC cap contract must still hold when there are oversized + // leaves. This guards against an over-aggressive fast path that skips + // truncation entirely. + const body = extractFunctionBody( + chatViewProviderSource, + "private buildWebviewStreamEvent(", + ); + assert.match( + body, + /cloneAndTruncateStreamPayload/, + "buildWebviewStreamEvent must still invoke cloneAndTruncateStreamPayload when oversized leaves are present", + ); +}); diff --git a/tests/regression/perf-history-processor-fingerprint-regression.test.mjs b/tests/regression/perf-history-processor-fingerprint-regression.test.mjs new file mode 100644 index 0000000..f65f7c1 --- /dev/null +++ b/tests/regression/perf-history-processor-fingerprint-regression.test.mjs @@ -0,0 +1,98 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { extractFunctionBody, joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: `HistoryProcessor.appendUnique` (used inside + * `coalesceAssistantBurst`) rebuilt the `seen` set from + * `target.map((entry) => JSON.stringify(entry))` on every call. When + * called repeatedly across a single burst coalesce (multiple target + * arrays: references, files, edits, etc.), the setup cost grew O(N) + * per call → O(N²) across the burst. + * + * Contract: appendUnique must not JSON.stringify every entry of `target` + * on every invocation. Either thread a stable `seen` set through from + * the caller, or use a cheaper stable-id key (e.g. messageID) instead + * of full-object fingerprint. + */ + +const historyProcessorSource = readSource( + [joinFromRoot("src", "providers", "chat", "HistoryProcessor.ts")], + "HistoryProcessor.ts", +); + +test("appendUnique does not rebuild the seen set from scratch on every call", () => { + // The old contract did `const seen = new Set(target.map(...))` on every + // call. The new contract must cache the seen set across calls for the + // same target — rebuild from target only on first contact. + // + // Look for either: + // - a WeakMap-based cache (seenByTarget / seenCache) that gates the + // `new Set(target.map(...))` build, OR + // - the build made conditional on a cache miss. + const coalesceBody = extractFunctionBody( + historyProcessorSource, + "private coalesceAssistantBurst(", + ); + assert.ok(coalesceBody.length > 0); + + // Forbidden: unconditional rebuild (no cache check). + // The signature of the bug: `const seen = new Set(target.map(...))` + // directly inside appendUnique, with no preceding cache lookup. + const appendUniqueMatch = coalesceBody.match( + /const\s+appendUnique\s*=\s*\([^)]*\)[^=]*=>\s*\{([\s\S]*?)\n\s{4}\};/ + ); + assert.ok(appendUniqueMatch, "appendUnique arrow helper should be extractable"); + const appendUniqueBody = appendUniqueMatch[1]; + + // Either no full-target rebuild at all, OR a cache lookup gates the rebuild. + const doesFullRebuild = /new\s+Set\(\s*target\.map\([^)]*\)\s*=>\s*JSON\.stringify/.test( + appendUniqueBody, + ); + if (doesFullRebuild) { + // Must be gated by a cache miss check. + assert.match( + appendUniqueBody, + /seenByTarget\.get|seenCache\.get|WeakMap|if\s*\(\s*!seen\b|let\s+seen\b/, + "appendUnique may rebuild the seen set from target on cache miss only — rebuild must be gated by a cache lookup", + ); + } +}); + +test("appendUnique uses a stable id-based key when entries have ids", () => { + // After the fix, the dedupe key should be a stable id (messageID, id, or + // similar) instead of a full JSON fingerprint. Look for that signal in + // the file. If we instead thread a `seen` set through, accept that too. + const coalesceBody = extractFunctionBody( + historyProcessorSource, + "private coalesceAssistantBurst(", + ); + assert.ok(coalesceBody.length > 0, "coalesceAssistantBurst body should be extractable"); + + // Acceptable signals: + // - a stable id key extractor: entry?.messageID || entry?.id || ... + // - a seen set threaded through appendUnique's signature + // - appendUnique carrying a persistent Map/Set parameter + const hasIdKey = /messageID|entry\?\.id|\bfingerprint\b/.test(coalesceBody); + const hasThreadedSeen = /appendUnique\([^,]*,\s*[^,]*,\s*Set|seen:/.test(coalesceBody); + assert.ok( + hasIdKey || hasThreadedSeen, + "appendUnique must use either a stable id-based key or a threaded seen set (current implementation rebuilds fingerprints per call)", + ); +}); + +test("coalesceAssistantBurst still produces the same merge semantics (no behavior regression)", () => { + // Defensive contract: the coalesce function must still exist and must + // still call appendUnique (or its replacement). Catches accidental + // removal of the dedupe step entirely. + const coalesceBody = extractFunctionBody( + historyProcessorSource, + "private coalesceAssistantBurst(", + ); + assert.match( + coalesceBody, + /appendUnique|mergeUnique|dedupeBy/, + "coalesceAssistantBurst must still perform a deduping append (renamed helper acceptable)", + ); +}); diff --git a/tests/regression/perf-logger-level-guard-regression.test.mjs b/tests/regression/perf-logger-level-guard-regression.test.mjs new file mode 100644 index 0000000..203311a --- /dev/null +++ b/tests/regression/perf-logger-level-guard-regression.test.mjs @@ -0,0 +1,63 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { extractFunctionBody, joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: `Logger.log()` unconditionally sanitized the context + * object via `createPlainObjectSnapshot` (3 deep traversals: structuredClone + * + JSON.stringify + JSON.parse) BEFORE the level check inside `output()` + * dropped the entry. Every `logger.debug(...)` call on the stream hot path + * paid full sanitization cost even when the configured level was `info`. + * + * With 79 logger.debug sites in providers and 15 in services — many gated + * only by `shouldVerboseStreamDebug()` around the call, not around the + * context object construction — this was the dominant remaining per-event + * CPU cost after Fix 1.1. + * + * Contract: the level check must run BEFORE `sanitizeConsoleValue` does + * any work. Filtered calls must not pay the sanitization cost. + */ + +const loggerSource = readSource( + [joinFromRoot("src", "utils", "Logger.ts")], + "Logger.ts", +); + +test("Logger.log checks level before sanitizing context", () => { + const logBody = extractFunctionBody(loggerSource, "private log("); + assert.ok(logBody.length > 0, "private log() body should be extractable"); + + // Match the actual call (not the word in comments) so the assertion is + // robust against perf-rationale comments that mention the helper by name. + const sanitizeCallIdx = logBody.indexOf("this.sanitizeConsoleValue("); + assert.ok(sanitizeCallIdx > -1, "log() should still call this.sanitizeConsoleValue(...) somewhere"); + + const levelGateTokens = ["minLevel", "parseLogLevel", "wouldEmit", "shouldLog", "isLevelEnabled", "wouldOutput"]; + const gateIdx = levelGateTokens + .map((tok) => logBody.indexOf(tok)) + .filter((idx) => idx > -1) + .sort((a, b) => a - b)[0]; + + assert.ok( + gateIdx !== undefined && gateIdx > -1, + "log() must perform a level gate (minLevel / parseLogLevel / wouldEmit / shouldLog) so filtered calls can short-circuit before sanitization", + ); + + assert.ok( + gateIdx < sanitizeCallIdx, + `level gate must appear BEFORE this.sanitizeConsoleValue(...) call (gate at ${gateIdx}, sanitize at ${sanitizeCallIdx})`, + ); +}); + +test("Logger exposes a level-guard helper usable before building context", () => { + // After the fix, callers that build expensive context objects should be + // able to ask the logger "would this level emit?" before constructing the + // context. This is a defensive contract: the helper must exist so the + // hot-path callers can short-circuit context construction entirely. + assert.match( + loggerSource, + /(public|private)\s+(shouldLog|isLevelEnabled|wouldEmit|wouldOutput|isEnabledFor)\s*\(/, + "Logger should expose a level-gate helper for hot-path callers", + ); +}); diff --git a/tests/regression/perf-plan-debug-serialization-regression.test.mjs b/tests/regression/perf-plan-debug-serialization-regression.test.mjs new file mode 100644 index 0000000..7244393 --- /dev/null +++ b/tests/regression/perf-plan-debug-serialization-regression.test.mjs @@ -0,0 +1,72 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: ChatViewProvider had two `JSON.stringify` calls + * inside `log.debug(...)` sites on the plan-update path: + * + * log.debug('Plan object set on next', { + * ... + * fullPlanObject: next.plan ? JSON.stringify(next.plan, null, 2) : 'undefined' + * }); + * const serialized = JSON.stringify(next); + * log.debug('Message serialization successful', { ... }); + * + * Each plan-bearing event paid two full-message stringifications, which + * were then sanitized AGAIN inside the logger. During plan generation + * (which streams many plan-update events) this is a major cost. + * + * Contract: the per-event plan-update path must not do inline + * `JSON.stringify(next)` / `JSON.stringify(next.plan, ...)` for debug + * logging. Either drop the debug sites, or gate them behind a cheap + * debug-flag check that runs before the stringify. + */ + +const chatViewProviderSource = readSource( + [joinFromRoot("src", "providers", "ChatViewProvider.ts")], + "ChatViewProvider.ts", +); + +test("plan-update debug log sites do not stringify the entire next object inline", () => { + // Find the plan-object debug log block and assert it doesn't stringify + // the whole message object. + const debugIdx = chatViewProviderSource.indexOf("'Plan object set on next'"); + assert.notEqual(debugIdx, -1, "expected to find the plan-object debug log site"); + + // Window around the debug log call — generous to capture the surrounding + // block but bounded so we don't accidentally match unrelated stringify. + const window = chatViewProviderSource.slice(debugIdx, debugIdx + 1200); + + assert.doesNotMatch( + window, + /JSON\.stringify\(\s*next\s*[,)]/, + "plan-update debug log must not JSON.stringify(next) inline — too expensive on per-event plan updates", + ); + assert.doesNotMatch( + window, + /JSON\.stringify\(\s*next\.plan\b/, + "plan-update debug log must not JSON.stringify(next.plan) inline — too expensive on per-event plan updates", + ); +}); + +test("any remaining plan-object debug stringify is gated behind a cheap debug-flag check", () => { + // If the implementation keeps the stringify but gates it, the gate must + // appear in the file. Acceptable gates: process.env check, dedicated + // plan-debug flag, or a logger.isLevelEnabled-style helper. + // Skip this assertion if no plan-object stringify exists at all. + const hasPlanStringify = /JSON\.stringify\(\s*next\.plan\b/.test(chatViewProviderSource); + if (!hasPlanStringify) { + return; + } + + // Find the plan-object debug block; verify a gate precedes it within 600 chars. + const debugIdx = chatViewProviderSource.indexOf("'Plan object set on next'"); + const preceding = chatViewProviderSource.slice(Math.max(0, debugIdx - 600), debugIdx); + assert.match( + preceding, + /process\.env\.|OPENCODE_DEBUG_PLANS|isPlanDebug|planDebugEnabled|shouldVerbosePlanDebug|isLevelEnabled|wouldEmit/, + "plan-object stringify must be gated by a cheap debug-flag check that runs before the stringify runs", + ); +}); diff --git a/tests/regression/perf-quota-async-write-regression.test.mjs b/tests/regression/perf-quota-async-write-regression.test.mjs new file mode 100644 index 0000000..c1a7687 --- /dev/null +++ b/tests/regression/perf-quota-async-write-regression.test.mjs @@ -0,0 +1,84 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { extractFunctionBody, joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: QuotaService.writeJsonFile() called fs.writeFileSync() + * synchronously on the extension host main thread. The only caller at the + * time of writing is the OpenAI token-refresh path inside refreshQuota() + * (QuotaService.ts:504), which runs on the 5-minute quota poll. + * + * Synchronous file writes block the extension host main thread for the + * duration of the syscall. The blocking window is normally small but + * compounds with quota polling and other work running concurrently. + * + * Contract: writeJsonFile must use the async fs.promises.writeFile API + * (or fs/promises). The function signature becomes async and callers must + * await it. + */ + +const quotaSource = readSource( + [joinFromRoot("src", "services", "QuotaService.ts")], + "QuotaService.ts", +); + +test("writeJsonFile does not block the extension host main thread", () => { + const writeJsonFileBody = extractFunctionBody( + quotaSource, + "async function writeJsonFile(", + ); + assert.ok(writeJsonFileBody.length > 0, "writeJsonFile body should be extractable"); + + assert.doesNotMatch( + writeJsonFileBody, + /fs\.writeFileSync/, + "writeJsonFile must not block the extension host main thread with fs.writeFileSync", + ); +}); + +test("writeJsonFile uses the async fs API", () => { + const writeJsonFileBody = extractFunctionBody( + quotaSource, + "async function writeJsonFile(", + ); + assert.ok(writeJsonFileBody.length > 0, "writeJsonFile body should be extractable"); + assert.match( + writeJsonFileBody, + /fs\.promises\.writeFile|await fs\.(promises\.)?writeFile|fs\.writeFile/, + "writeJsonFile should use the async fs APIs (fs.promises.writeFile or fs.writeFile callback form)", + ); +}); + +test("writeJsonFile is async-awaitable by its callers", () => { + const writeJsonFileMatch = quotaSource.match(/async\s+function\s+writeJsonFile[^{]*\{/); + assert.ok(writeJsonFileMatch, "writeJsonFile declaration should be found"); + + const signatureEnd = quotaSource.slice( + writeJsonFileMatch.index, + writeJsonFileMatch.index + writeJsonFileMatch[0].length, + ); + assert.ok( + /async\s+function\s+writeJsonFile|function\s+writeJsonFile[^)]*\):\s*Promise) so callers can await it", + ); +}); + +test("fetchOpenAI awaits the async writeJsonFile on the token-refresh path", () => { + const fetchOpenAIBody = extractFunctionBody( + quotaSource, + "private async fetchOpenAI", + ); + assert.ok(fetchOpenAIBody.length > 0, "fetchOpenAI body should be extractable"); + + const callSiteMatch = fetchOpenAIBody.match(/writeJsonFile\([\s\S]*?\)/); + assert.ok(callSiteMatch, "expected to find a writeJsonFile call inside fetchOpenAI"); + + const callIdx = fetchOpenAIBody.indexOf("writeJsonFile"); + const preceding = fetchOpenAIBody.slice(Math.max(0, callIdx - 30), callIdx); + assert.match( + preceding, + /await\s*$/, + "fetchOpenAI must await writeJsonFile on the token-refresh path so the main thread is not blocked", + ); +}); diff --git a/tests/regression/perf-response-message-memo-regression.test.mjs b/tests/regression/perf-response-message-memo-regression.test.mjs new file mode 100644 index 0000000..150e22d --- /dev/null +++ b/tests/regression/perf-response-message-memo-regression.test.mjs @@ -0,0 +1,85 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: `ResponseMessage` is wrapped in `memo()` with no + * custom comparator. It receives `streaming` as a direct prop. Every + * stream batch produces a new `streaming` object identity, which forces + * every mounted ResponseMessage card to rerender — even cards that + * correspond to historical messages and don't visibly depend on the + * live stream. The `ResponseMessageInner` subtree then redoes O(n) + * work over `responseBodyChunks` / accumulated events each render. + * + * During active streaming with a long conversation visible, this means + * every historical assistant card rerenders on every batch. + * + * Contract: `ResponseMessage`'s memo must use a custom comparator that + * skips rerenders for cards whose own message identity is unchanged AND + * that are not the active streaming card. Alternatively, the `streaming` + * prop must not be passed unconditionally to every card. + */ + +const messageComponentsSource = readSource( + [joinFromRoot("webview", "shared", "src", "chat", "MessageComponents.tsx")], + "MessageComponents.tsx", +); + +test("ResponseMessage memo uses a custom comparator (not default shallow compare)", () => { + // The default `memo(Component)` rerenders on every prop identity change. + // After the fix, the memo must take a second argument: a comparator that + // avoids rerendering cards whose own message identity didn't change and + // that aren't the streaming card. + // + // Acceptable shapes: + // memo(function ResponseMessage(...) { ... }, arePropsEqual) + // memo(Component, (prev, next) => ...) + // + // Forbidden: memo(Component) with no second argument. + // + // Find the ResponseMessage memo expression and look for a comparator. + const memoIdx = messageComponentsSource.indexOf("export const ResponseMessage = memo("); + assert.notEqual(memoIdx, -1, "expected to find ResponseMessage memo export"); + + // Capture up to 4000 chars after `memo(` — generous to include the + // inline component body and the trailing comparator argument. + const after = messageComponentsSource.slice(memoIdx, memoIdx + 6000); + + // The memo call must close the inner function and then have a comma + // before the comparator. Look for `}, ` near the end. + // If the memo has no second argument, the call ends with `});` directly. + const memoCloseIdx = after.indexOf("});"); + assert.ok(memoCloseIdx > -1, "expected to find the end of the ResponseMessage memo call"); + + const memoArgs = after.slice(0, memoCloseIdx); + // The closing `}` of the inner function followed by `,` (not `)`) is the + // signal that a comparator argument follows. + assert.match( + memoArgs, + /\}\s*,\s*[A-Za-z_]/, + "ResponseMessage memo must take a custom comparator as its second argument (default shallow compare rerenders every card on every stream batch)", + ); +}); + +test("ResponseMessage comparator avoids rerender for non-streaming cards on streaming identity change", () => { + // The comparator (and any helper it uses) must consider whether this card + // is the streaming card. Search the whole file — helpers are typically + // defined above the memo export. + assert.match( + messageComponentsSource, + /streamingMessageId|isStreamingCard|isActiveForThisCard|streaming\.messageId|cardIsStreaming|isLiveCard|isStreamingForThisCard|isStreamingForCard/, + "ResponseMessage comparator (or its helper) must distinguish the active streaming card from historical cards so non-streaming cards skip rerender on streaming identity changes", + ); + + // Additionally, the comparator must reference the helper so the check is + // actually wired in. Find areResponseMessagePropsEqual and verify. + const comparatorIdx = messageComponentsSource.indexOf("function areResponseMessagePropsEqual("); + assert.ok(comparatorIdx > -1, "areResponseMessagePropsEqual should be defined"); + const comparatorWindow = messageComponentsSource.slice(comparatorIdx, comparatorIdx + 2000); + assert.match( + comparatorWindow, + /isStreamingForThisCard|isStreamingCard|isActiveForThisCard|streamingMessageId|cardIsStreaming|isLiveCard|streaming\.messageId/, + "areResponseMessagePropsEqual must invoke the streaming-card check so non-streaming cards skip rerender on streaming identity changes", + ); +}); diff --git a/tests/regression/perf-stream-clone-redundancy-regression.test.mjs b/tests/regression/perf-stream-clone-redundancy-regression.test.mjs new file mode 100644 index 0000000..2f2511f --- /dev/null +++ b/tests/regression/perf-stream-clone-redundancy-regression.test.mjs @@ -0,0 +1,83 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { extractFunctionBody, joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: every SSE event was being deep-cloned up to 5 times + * before reaching the webview: + * 1. MessageStreamService.cloneRawEvent -> createPlainObjectSnapshot + * - structuredClone(value) // pass 1 + * - JSON.parse(JSON.stringify()) // pass 2 + pass 3 + * 2. ChatViewProvider.buildWebviewStreamEvent -> cloneAndTruncateStreamPayload + * // pass 4 (recursive) + * 3. webview.postMessage structured-clone across IPC + * // pass 5 + * + * The JSON round-trip inside createPlainObjectSnapshot is defensive + * overkill for SDK SSE payloads (already JSON-serializable). Removing + * it from the stream hot path eliminates ~2 of the 5 traversals. + * + * Contract: a fast variant must exist that returns structuredClone(value) + * on the happy path; MessageStreamService must use it for raw-event + * cloning. Fallbacks (JSON/manual) are allowed for the failure case. + */ + +const snapshotSource = readSource( + [joinFromRoot("src", "shared", "createPlainObjectSnapshot.ts")], + "createPlainObjectSnapshot.ts", +); +const messageStreamServiceSource = readSource( + [joinFromRoot("src", "services", "MessageStreamService.ts")], + "MessageStreamService.ts", +); + +test("createPlainObjectSnapshotFast happy path returns structuredClone(value) directly", () => { + assert.match( + snapshotSource, + /export function createPlainObjectSnapshotFast\b/, + "a fast clone variant should be exported for hot-path callers that do not need plain-JSON-safe output", + ); + + const fastBody = extractFunctionBody( + snapshotSource, + "export function createPlainObjectSnapshotFast(", + ); + assert.ok(fastBody.length > 0, "fast variant body should be extractable"); + + assert.match( + fastBody, + /return structuredClone\(value\)/, + "fast variant happy path must `return structuredClone(value)` directly (no JSON round-trip on the happy path)", + ); + + assert.match( + fastBody, + /catch|snapshotUnknown|JSON\.parse/, + "fast variant must keep a fallback for the structuredClone-throws case (catch / snapshotUnknown / JSON.parse)", + ); +}); + +test("MessageStreamService uses the fast clone variant on the stream hot path", () => { + assert.match( + messageStreamServiceSource, + /createPlainObjectSnapshotFast/, + "MessageStreamService should import and use the fast clone variant", + ); + + const cloneRawEventBody = extractFunctionBody( + messageStreamServiceSource, + "private cloneRawEvent(", + ); + assert.ok(cloneRawEventBody.length > 0, "cloneRawEvent body should be extractable"); + assert.match( + cloneRawEventBody, + /createPlainObjectSnapshotFast/, + "cloneRawEvent must delegate to createPlainObjectSnapshotFast", + ); + assert.doesNotMatch( + cloneRawEventBody, + /\bcreatePlainObjectSnapshot\b(?!Fast)/, + "cloneRawEvent must not call the slow createPlainObjectSnapshot variant", + ); +}); diff --git a/tests/regression/perf-subagent-modal-poll-thrash-regression.test.mjs b/tests/regression/perf-subagent-modal-poll-thrash-regression.test.mjs new file mode 100644 index 0000000..12d932a --- /dev/null +++ b/tests/regression/perf-subagent-modal-poll-thrash-regression.test.mjs @@ -0,0 +1,75 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: the subagent-detail modal polling effect in + * MessageComponents.tsx set up a 1500ms `getSubagentConversation` interval + * with deps `[selectedSubagentId, subagents, subagentDetailsById]`. During + * active streaming, `subagents` and `subagentDetailsById` mutate on every + * stream event, tearing down and recreating the interval dozens of times + * per second. Effective postMessage rate to the extension host approached + * the stream event rate, not 1500ms. + * + * Contract: the polling effect must read latest `subagents` and + * `subagentDetailsById` through refs (or another stable handle) and the + * effect deps must NOT include those volatile objects. + */ + +const messageComponentsSource = readSource( + [joinFromRoot("webview", "shared", "src", "chat", "MessageComponents.tsx")], + "MessageComponents.tsx", +); + +function findDepsArrayAfter(source, anchor) { + const idx = source.indexOf(anchor); + if (idx < 0) { + return null; + } + const after = source.slice(idx); + // Match the first deps array `}, [a, b, c]` that appears after the anchor. + // The closing `}` and `,` are part of the useEffect block close. + const match = after.match(/\},\s*\[([^\]]*)\]/); + return match ? match[1] : null; +} + +test("subagent modal getSubagentConversation poll does not depend on volatile subagents state", () => { + const deps = findDepsArrayAfter(messageComponentsSource, "getSubagentConversation"); + assert.ok( + deps !== null, + "expected to find a useEffect deps array after the getSubagentConversation postMessage", + ); + + assert.doesNotMatch( + deps, + /\bsubagents\b/, + `subagents must not appear in the poll effect deps array (causes interval tear-down/recreate on every stream event). Found: [${deps}]`, + ); + assert.doesNotMatch( + deps, + /\bsubagentDetailsById\b/, + `subagentDetailsById must not appear in the poll effect deps array (causes interval tear-down/recreate on every stream event). Found: [${deps}]`, + ); +}); + +test("subagent modal poll reads latest state via a ref or stable handle", () => { + // After the fix, the effect body must still read latest subagents / + // subagentDetailsById, but indirectly via a ref or selector handle so the + // deps array can stay minimal. We assert that at least one ref-based + // pattern is present in the modal component. + const anchorIdx = messageComponentsSource.indexOf("getSubagentConversation"); + assert.ok(anchorIdx >= 0, "expected to find getSubagentConversation in MessageComponents"); + + // Walk backwards from the anchor to find the enclosing component's body. + // After the fix, latest subagents/subagentDetailsById must be read via a + // named ref so the poll effect deps array can stay minimal. Generic + // useRef() calls do not satisfy this — the ref must be clearly tied to + // the subagents/subagentDetailsById state slices. + const componentWindow = messageComponentsSource.slice(Math.max(0, anchorIdx - 12000), anchorIdx); + assert.match( + componentWindow, + /\b(?:subagentsRef|latestSubagents|subagentDetailsByIdRef|latestSubagentDetailsById|latestSubagentsRef|latestSubagentDetailsRef)\b/, + "the modal must read latest subagents/subagentDetailsById via a named ref (e.g. subagentsRef) so the poll effect deps can stay minimal", + ); +}); diff --git a/tests/regression/perf-subagent-pending-drain-regression.test.mjs b/tests/regression/perf-subagent-pending-drain-regression.test.mjs new file mode 100644 index 0000000..cf74dd7 --- /dev/null +++ b/tests/regression/perf-subagent-pending-drain-regression.test.mjs @@ -0,0 +1,60 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: SubagentTracker used `while (...pending.shift())` loops + * to find a binding candidate. Each `shift()` is O(q) on the underlying + * array (it reallocates and reindexes), and the loop is O(q) iterations, + * so worst-case total work is O(q²). When many subtasks bind near- + * simultaneously (e.g. parallel subagent spawns) this manifests as a + * noticeable hitch. + * + * Contract: pending-subtask drain loops must not use `.shift()`. They must + * use index-based iteration that processes entries in place, then clears + * the array in O(1) via `.length = 0` (or equivalent). + */ + +const subagentTrackerSource = readSource( + [joinFromRoot("src", "services", "SubagentTracker.ts")], + "SubagentTracker.ts", +); + +test("pending-subtask drain loops do not use .shift()", () => { + // Strip line comments so perf-rationale comments mentioning the old + // pattern don't trip the assertion. + const stripped = subagentTrackerSource.replace(/\/\/[^\n]*/g, ""); + assert.doesNotMatch( + stripped, + /pending\.shift\(\)/, + "SubagentTracker must not use pending.shift() — it is O(n) per call and compounds to O(n²) inside drain loops", + ); +}); + +test("pending drain loops use index-based iteration", () => { + // After the fix, drain loops iterate by index. Look for the two known + // drain sites (each iterates pending[] by index and truncates in place). + const stripped = subagentTrackerSource.replace(/\/\/[^\n]*/g, ""); + const drainPattern = /pending\.length\b/g; + let match; + let drainCount = 0; + while ((match = drainPattern.exec(stripped)) !== null) { + drainCount += 1; + } + // Pending is referenced in many places (sets, gets, length reads). What + // we actually care about is that there's at least one for-loop iteration + // pattern combined with an in-place truncation. + assert.match( + stripped, + /for\s*\(\s*(let|const)\s+\w+\s*=\s*0\s*;\s*\w+\s*<\s*pending\.length/, + "pending drain must use index-based for-loop iteration (not shift/while)", + ); + assert.match( + stripped, + /pending\.length\s*=\s*\w+|pending\.splice\(/, + "pending drain must truncate in place via pending.length = N or pending.splice", + ); + // Sanity: pending.length references should still exist (the loop reads them). + assert.ok(drainCount >= 2, "expected at least 2 pending.length references in drain paths"); +}); diff --git a/tests/regression/perf-subagent-projection-debounce-regression.test.mjs b/tests/regression/perf-subagent-projection-debounce-regression.test.mjs new file mode 100644 index 0000000..d35ce0c --- /dev/null +++ b/tests/regression/perf-subagent-projection-debounce-regression.test.mjs @@ -0,0 +1,77 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { extractFunctionBody, joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: every subagent-bearing stream event called + * `persistSubagentProjection` directly with a fresh full snapshot from + * `subagentTracker.getSnapshotPayload()`. During active subagent work + * (dozens of events per second), this triggered dozens of full-snapshot + * writes per second to VS Code's workspaceState — each write serializing + * the entire projection graph. + * + * Contract: persist must be debounced (trailing) so rapid bursts coalesce + * into a single write. The debouncer must rebuild the snapshot at flush + * time from the live tracker state (not snapshot at schedule time). + */ + +const chatViewProviderSource = readSource( + [joinFromRoot("src", "providers", "ChatViewProvider.ts")], + "ChatViewProvider.ts", +); + +test("persistSubagentProjection is not invoked directly on the stream-event path", () => { + // Find the stream-event subagent-update block (around line ~4570) and + // assert the persist call goes through a debouncing scheduler instead + // of calling persistSubagentProjection directly. + const subagentUpdateIdx = chatViewProviderSource.indexOf( + 'this.view?.webview.postMessage({\n type: "subagentUpdate"', + ); + assert.ok(subagentUpdateIdx > -1, "expected to find the subagentUpdate postMessage site"); + + const window = chatViewProviderSource.slice(subagentUpdateIdx, subagentUpdateIdx + 1500); + assert.doesNotMatch( + window, + /void\s+this\.persistSubagentProjection\(/, + "stream-event subagent-update path must not call persistSubagentProjection directly — go through a debounce scheduler", + ); +}); + +test("ChatViewProvider exposes a debounced scheduler for subagent projection persistence", () => { + assert.match( + chatViewProviderSource, + /(private|public)\s+(scheduleSubagentProjectionPersist|scheduleSubagentProjectionWrite|schedulePersistSubagentProjection|scheduleProjectionPersist)\s*\(/, + "ChatViewProvider should expose a debounced scheduler for subagent projection persistence", + ); +}); + +test("the debounced scheduler flushes via setTimeout and is cleared on dispose", () => { + // Find the scheduler body and verify it uses setTimeout with a trailing + // debounce, and that dispose tears down any in-flight timers. + const schedulerMatch = chatViewProviderSource.match( + /(private|public)\s+(scheduleSubagentProjectionPersist|scheduleSubagentProjectionWrite|schedulePersistSubagentProjection|scheduleProjectionPersist)\s*\([^)]*\)\s*:\s*void\s*\{/ + ); + assert.ok(schedulerMatch, "scheduler body should be extractable"); + + const schedulerBody = extractFunctionBody( + chatViewProviderSource, + schedulerMatch[0], + ); + assert.ok(schedulerBody.length > 0); + + assert.match( + schedulerBody, + /setTimeout\(/, + "scheduler should use setTimeout for trailing debounce", + ); + + // The dispose path must clear the timer map. + const disposeBody = extractFunctionBody(chatViewProviderSource, "public dispose(): void"); + assert.ok(disposeBody.length > 0); + assert.match( + disposeBody, + /clearTimeout\(|subagentProjectionDebounce|projectionPersistTimer|projectionDebounce/, + "dispose must clear any in-flight projection debounce timers", + ); +}); diff --git a/tests/regression/perf-subagent-tracker-clamp-regression.test.mjs b/tests/regression/perf-subagent-tracker-clamp-regression.test.mjs new file mode 100644 index 0000000..75d91f4 --- /dev/null +++ b/tests/regression/perf-subagent-tracker-clamp-regression.test.mjs @@ -0,0 +1,68 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: `SubagentTracker` used the call shape + * + * detail.conversationEvents = clampEvents( + * [...detail.conversationEvents, normalizedEvent], + * MAX_CONVERSATION_EVENTS, + * ); + * + * The `[...array, item]` spread allocates a brand-new array on every + * append — even when the array is well under the cap, which is the common + * case during streaming. With MAX_CONVERSATION_EVENTS=400 and events + * arriving at 30-60/sec, this is hundreds of array allocations per second + * per active subagent, plus GC pressure. + * + * Contract: append sites must not use `[...array, item]` spread before + * clampEvents. The append must happen in place (push), and clampEvents + * must trim only when over cap. + */ + +const subagentTrackerSource = readSource( + [joinFromRoot("src", "services", "SubagentTracker.ts")], + "SubagentTracker.ts", +); + +test("clampEvents call sites do not spread the array before append", () => { + // Forbidden pattern: clampEvents([ ...array, item ], max) + // The spread allocates per-call even when no clamp is needed. + assert.doesNotMatch( + subagentTrackerSource, + /clampEvents\(\s*\[\s*\.\.\.[A-Za-z_.]+,\s*[A-Za-z_.]+\s*\]/, + "clampEvents must not be called with `[...array, item]` spread — that allocates per-append even when under cap", + ); +}); + +test("SubagentTracker exposes an in-place append helper that bounds the array", () => { + // After the fix, an in-place append helper should exist (e.g. + // appendClamped, pushClamped, pushBounded) that pushes then trims only + // if length exceeds the cap. + assert.match( + subagentTrackerSource, + /function\s+(appendClamped|pushClamped|pushBounded|appendBounded)\s*[<(]/, + "SubagentTracker should expose an in-place bounded-append helper so per-event appends don't allocate a new array", + ); +}); + +test("clampEvents remains a pure slice helper for overflow cases", () => { + // The existing clampEvents function should remain available for the + // post-push overflow trim (and for any external caller that needs a + // pure slice). We're not removing clampEvents — we're removing the + // spread before it. + assert.match( + subagentTrackerSource, + /function\s+clampEvents\s*<[^>]*>\s*\(/, + "clampEvents should still be declared as a generic helper", + ); + const clampBody = subagentTrackerSource.match(/function\s+clampEvents[^{]*\{([\s\S]*?)\n\}/); + assert.ok(clampBody, "clampEvents body should be extractable"); + assert.match( + clampBody[1], + /\.slice\(/, + "clampEvents should still use .slice() for overflow trimming", + ); +}); diff --git a/tests/regression/perf-token-emit-debounce-regression.test.mjs b/tests/regression/perf-token-emit-debounce-regression.test.mjs new file mode 100644 index 0000000..51af140 --- /dev/null +++ b/tests/regression/perf-token-emit-debounce-regression.test.mjs @@ -0,0 +1,86 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { extractFunctionBody, joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: GeminiTokenUsageTracker.recordUsage() called + * `this.emit("usageUpdated", this.getAllUsage())` on every token-bearing + * stream event. getAllUsage() sorts ALL model entries on every call. + * + * During active streaming with frequent token updates, this triggered: + * - a full Object.values() + sort() on every token event + * - synchronous EventEmitter fan-out to every listener + * + * Contract: recordUsage must NOT synchronously emit a freshly-sorted array. + * It must either (a) debounce the emit through a short timer, (b) emit a + * lightweight "dirty" signal and let consumers read on demand, or + * (c) remove the emit entirely if no consumers exist. + * + * This test stays implementation-agnostic: it only forbids the synchronous + * sort+emit pattern in recordUsage. + */ + +const geminiTrackerSource = readSource( + [joinFromRoot("src", "services", "GeminiTokenUsageTracker.ts")], + "GeminiTokenUsageTracker.ts", +); + +test("recordUsage does not synchronously sort and emit on every token event", () => { + const recordUsageBody = extractFunctionBody( + geminiTrackerSource, + "public recordUsage(", + ); + assert.ok(recordUsageBody.length > 0, "recordUsage body should be extractable"); + + assert.doesNotMatch( + recordUsageBody, + /this\.emit\(\s*["']usageUpdated["']\s*,\s*this\.getAllUsage\(\)\s*\)/, + "recordUsage must not synchronously emit a freshly-sorted array on every token event (causes per-event sort + listener fan-out)", + ); + + assert.doesNotMatch( + recordUsageBody, + /this\.emit\(\s*["']usageUpdated["']\s*,\s*Object\.values\(/, + "recordUsage must not synchronously emit Object.values() of currentUsage on every token event", + ); +}); + +test("GeminiTokenUsageTracker batches or defers usageUpdated emissions", () => { + // After the fix, the emit must move out of the recordUsage hot path. + // Acceptable shapes: + // - a dedicated debounce helper (scheduleUsageEmit / emitTimer / usageEmitHandle) + // - a "dirty" flag that a separate timer drains + // - removal of the emit entirely + const hasBatchPattern = /scheduleUsageEmit|emitTimer|usageEmit|usageEmitHandle|scheduleUsageUpdated|emitUsage.*setTimeout|scheduleEmit/.test( + geminiTrackerSource, + ); + const hasNoEmit = !/\bon\(["']usageUpdated["']\)|emit\(["']usageUpdated["']/.test( + geminiTrackerSource, + ); + assert.ok( + hasBatchPattern || hasNoEmit, + "GeminiTokenUsageTracker must either batch usageUpdated emissions through a debounce helper or remove the emit entirely", + ); +}); + +test("sort/reduce work in getAllUsage and getGrandTotal is not on the recordUsage hot path", () => { + // Defensive contract: even if a future change adds a new caller that + // synchronously invokes getAllUsage, that path must not live inside + // recordUsage. Re-assert by scanning recordUsage for sort/reduce calls + // AND for getAllUsage calls (which itself calls .sort). + const recordUsageBody = extractFunctionBody( + geminiTrackerSource, + "public recordUsage(", + ); + assert.doesNotMatch( + recordUsageBody, + /\.sort\(/, + "recordUsage must not call .sort() directly (sort belongs to read-side getAllUsage, not write-side recordUsage)", + ); + assert.doesNotMatch( + recordUsageBody, + /this\.getAllUsage\(/, + "recordUsage must not invoke getAllUsage() (which internally sorts) — sort work belongs to read-side consumers, not the write-side hot path", + ); +}); diff --git a/tests/regression/perf-webview-stream-event-truncation-regression.test.mjs b/tests/regression/perf-webview-stream-event-truncation-regression.test.mjs new file mode 100644 index 0000000..905c236 --- /dev/null +++ b/tests/regression/perf-webview-stream-event-truncation-regression.test.mjs @@ -0,0 +1,73 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { extractFunctionBody, joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +/** + * Regression target: `ChatViewProvider.buildWebviewStreamEvent` called + * `cloneAndTruncateStreamPayload` on every stream event — a recursive DFS + * over the full event tree (depth-capped at 16) that allocates a brand-new + * object graph. Even after Fix 1.1 made MessageStreamService use the fast + * clone variant, this second per-event clone for truncation remained. + * + * The truncation only needs to bound oversized string leaves (tool outputs, + * file reads). It does not need a full deep clone of the event graph: the + * upstream `MessageStreamService.cloneRawEvent` already produced an + * independent copy via `createPlainObjectSnapshotFast`. Walking the tree + * twice is redundant. + * + * Contract: `buildWebviewStreamEvent` must not perform a second recursive + * deep clone for truncation. Either (a) mutate the fast-clone result in + * place, (b) walk once with targeted leaf-string truncation, or (c) skip + * the clone entirely when the payload has no oversized leaves. + */ + +const chatViewProviderSource = readSource( + [joinFromRoot("src", "providers", "ChatViewProvider.ts")], + "ChatViewProvider.ts", +); + +test("buildWebviewStreamEvent does not spread the original event over the truncated clone", () => { + const body = extractFunctionBody( + chatViewProviderSource, + "private buildWebviewStreamEvent(", + ); + assert.ok(body.length > 0, "buildWebviewStreamEvent body should be extractable"); + + // The cloneAndTruncateStreamPayload DFS already produces a complete clone + // with truncated values — the original event is never mutated. Spreading + // the original over the clone is redundant work on every stream event. + assert.doesNotMatch( + body, + /\.\.\.\s*enrichedEvent\b/, + "buildWebviewStreamEvent must not spread enrichedEvent over the truncated clone — the clone already contains every key, the spread is pure waste on the per-event hot path", + ); +}); + +test("buildWebviewStreamEvent returns the truncated clone directly", () => { + const body = extractFunctionBody( + chatViewProviderSource, + "private buildWebviewStreamEvent(", + ); + assert.ok(body.length > 0); + + assert.match( + body, + /return\s+this\.cloneAndTruncateStreamPayload\(|return\s+\w*[Tt]runcated/, + "buildWebviewStreamEvent should return the truncated clone directly (one DFS, no spread)", + ); +}); + +test("buildWebviewStreamEvent still bounds oversized string leaves for IPC", () => { + const body = extractFunctionBody( + chatViewProviderSource, + "private buildWebviewStreamEvent(", + ); + assert.ok(body.length > 0); + + assert.match( + body, + /MAX_STREAM_WEBVIEW_TOOL_OUTPUT_CHARS|cloneAndTruncateStreamPayload|truncateStreamToolTextForWebview/, + "buildWebviewStreamEvent must still bound oversized string leaves (the IPC cap contract from stream-event-main-thread-performance.test.mjs)", + ); +}); diff --git a/tests/services/gemini-token-tracker-performance.test.mjs b/tests/services/gemini-token-tracker-performance.test.mjs index 318a33f..f08e403 100644 --- a/tests/services/gemini-token-tracker-performance.test.mjs +++ b/tests/services/gemini-token-tracker-performance.test.mjs @@ -101,9 +101,11 @@ test('GeminiTokenUsageTracker implements fire-and-forget pattern', () => { assert.doesNotMatch(recordBody, /await\s+this\.saveToStorage\(\)/, 'recordUsage should NOT await saveToStorage'); - // Verify immediate emission of usageUpdated event - assert.match(recordBody, /this\.emit\(["']usageUpdated["'],\s*this\.getAllUsage\(\)\)/, - 'recordUsage should emit usageUpdated immediately (not after save)'); + // Emissions are debounced through scheduleUsageEmit so rapid token events + // coalesce into a single usageUpdated emit. Synchronous emit on every token + // caused per-event sort + listener fan-out. + assert.match(recordBody, /this\.scheduleUsageEmit\(\)/, + 'recordUsage should delegate emission to scheduleUsageEmit (debounced)'); // Verify saveToStorage is async but not awaited assert.match(trackerSource, /private\s+async\s+saveToStorage\(\):\s*Promise/, diff --git a/tests/services/message-stream-service.test.mjs b/tests/services/message-stream-service.test.mjs index 6c0559f..49b9213 100644 --- a/tests/services/message-stream-service.test.mjs +++ b/tests/services/message-stream-service.test.mjs @@ -136,12 +136,12 @@ test('MessageStreamService unwraps SDK sync event wrappers into canonical stream test('MessageStreamService snapshots raw SDK events into plain data before notifying callbacks', () => { assert.match( messageStreamSource, - /import \{ createPlainObjectSnapshot \} from "\.\.\/shared\/createPlainObjectSnapshot";/, - 'MessageStreamService should import the shared plain-object snapshot helper', + /import \{ createPlainObjectSnapshotFast \} from "\.\.\/shared\/createPlainObjectSnapshot";/, + 'MessageStreamService should import the fast plain-object snapshot helper (avoids redundant JSON round-trip on the stream hot path)', ); assert.match( messageStreamSource, - /private cloneRawEvent\(value: T\): T \{\s*return createPlainObjectSnapshot\(value\);\s*\}/, + /private cloneRawEvent\(value: T\): T \{\s*return createPlainObjectSnapshotFast\(value\);\s*\}/, 'MessageStreamService should not leak the original SDK event object when cloning fails', ); }); diff --git a/tests/services/sdk-message-adapter.test.mjs b/tests/services/sdk-message-adapter.test.mjs index ea7c797..8d5fc46 100644 --- a/tests/services/sdk-message-adapter.test.mjs +++ b/tests/services/sdk-message-adapter.test.mjs @@ -116,8 +116,8 @@ test('TextPart adapter produces text content and parts', () => { ); assert.match( adapterSource, - /message\.parts\?\.push\(\{ type: "text", text: textPart\.text \}\)/, - 'should push text as MessagePart with type=text', + /message\.parts\?\.push\(\{ type: "text", text: textPart\.text, synthetic \}\)/, + 'should preserve synthetic state on projected text parts', ); assert.match( adapterSource, @@ -126,6 +126,24 @@ test('TextPart adapter produces text content and parts', () => { ); }); +test('hydrated text snippets remain attachments instead of duplicated user-bubble text', () => { + assert.match( + adapterSource, + /function visibleUserTextFromParts\(parts: SdkMessagePart\[\]\): string/, + 'the adapter should derive visible user text from typed SDK parts', + ); + assert.match( + adapterSource, + /part\.type === "text" && part\.synthetic !== true/, + 'synthetic SDK text must not become hydrated user content', + ); + assert.match( + adapterSource, + /attachmentContents\.has\(text\)/, + 'text identical to a text attachment payload must remain attachment-only', + ); +}); + test('ReasoningPart adapter produces reasoningEvents with id and timing', () => { assert.match( adapterSource, diff --git a/tests/unit/streaming-stop-button-sync.test.mjs b/tests/unit/streaming-stop-button-sync.test.mjs index 4e60900..c530624 100644 --- a/tests/unit/streaming-stop-button-sync.test.mjs +++ b/tests/unit/streaming-stop-button-sync.test.mjs @@ -29,7 +29,17 @@ test('InputWrapper derives stop/send toggle from the active assistant response s ); assert.match( chatShellSource, - /const hasLiveAssistantTurn = shouldDeferComposerSendInCurrentSession\([\s\S]*const showAiResponseLoading =[\s\S]*hasLiveAssistantTurn[\s\S]*const showExtendedLoading =[\s\S]*hasLiveAssistantTurn/s, - 'loading indicator should use the same live-turn condition as the stop button', + /const showExtendedLoading = hasLiveAssistantTurn \|\| showAiResponseLoading;/, + 'the rendered loading ticker must directly use the same live-turn condition as Stop', + ); + assert.doesNotMatch( + chatShellSource, + /const showAiResponseLoading =[\s\S]*hasRenderableStreamingContent/s, + 'stream content or activity must not hide the loading ticker while Stop remains available', + ); + assert.match( + chatShellSource, + /const hasNewLiveTurnBeforeAssistantIdentity =[\s\S]*Boolean\(state\.streaming\?\.isActive\)[\s\S]*state\.assistantTurnPending[\s\S]*visiblePendingUserMessages\.length > 0[\s\S]*!hasNewLiveTurnBeforeAssistantIdentity/s, + 'a previous terminal assistant message must not hide the next turn\'s loading ticker before its message identity arrives', ); }); diff --git a/tests/webview/live-stream-response-rendering.test.mjs b/tests/webview/live-stream-response-rendering.test.mjs index 9936ff2..1d11b36 100644 --- a/tests/webview/live-stream-response-rendering.test.mjs +++ b/tests/webview/live-stream-response-rendering.test.mjs @@ -33,8 +33,13 @@ test("renderable stream text paints immediately and centralized transcript takes ); assert.match( shellSource, - /!hasRenderableStreamingContent[\s\S]*!isAiResponseBlockFinished/, - "the loading bubble should end when renderable streamed text is available", + /hasLiveAssistantTurn[\s\S]*?!isAiResponseBlockFinished/, + "the loading ticker must stay visible throughout the live assistant turn alongside Stop, regardless of streaming content arrival", + ); + assert.doesNotMatch( + shellSource, + /hasRenderableStreamingContent/, + "stream content must not gate the loading ticker — hasRenderableStreamingContent was removed in favor of hasLiveAssistantTurn", ); }); diff --git a/tests/webview/stream-event-main-thread-performance.test.mjs b/tests/webview/stream-event-main-thread-performance.test.mjs index 01a62fc..c0dec86 100644 --- a/tests/webview/stream-event-main-thread-performance.test.mjs +++ b/tests/webview/stream-event-main-thread-performance.test.mjs @@ -136,8 +136,15 @@ test("webview transport bounds oversized tool output without truncating persiste providerSource, /const eventForWebview = this\.buildWebviewStreamEvent\(/, ); + // buildWebviewStreamEvent returns the truncated clone directly — no + // redundant spread of the original event over the clone. assert.match( providerSource, - /const centralizedEventPayload = \{\s*\.\.\.enrichedEvent,/s, + /return this\.cloneAndTruncateStreamPayload\(enrichedEvent\)/, + ); + assert.doesNotMatch( + providerSource, + /\.\.\.\s*enrichedEvent\b.*\.\.\.\s*truncatedDeepClone/s, + "buildWebviewStreamEvent must not spread the original event over the truncated clone (the clone already contains every key)", ); }); diff --git a/tests/webview/streaming-transcript-handoff-regression.test.mjs b/tests/webview/streaming-transcript-handoff-regression.test.mjs new file mode 100644 index 0000000..0975714 --- /dev/null +++ b/tests/webview/streaming-transcript-handoff-regression.test.mjs @@ -0,0 +1,17 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { joinFromRoot, readSource } from "../helpers/source-utils.mjs"; + +const source = readSource( + [joinFromRoot("webview", "shared", "src", "chat", "StreamingComponents.tsx")], + "StreamingComponents.tsx", +); + +test("an active streaming card yields to a transcript card for the same assistant turn", () => { + assert.match( + source, + /const hasMatchingAssistantTurnInTranscript =[\s\S]*?if \(hasMatchingAssistantTurnInTranscript\) return false;/, + "matching assistant IDs must prevent a live and transcript card from rendering together", + ); +}); diff --git a/webview/shared/src/chat/ChatShell.tsx b/webview/shared/src/chat/ChatShell.tsx index 458427c..054eea2 100644 --- a/webview/shared/src/chat/ChatShell.tsx +++ b/webview/shared/src/chat/ChatShell.tsx @@ -2785,9 +2785,6 @@ function ChatContent() { }); }, []); - // Track loading state timing to ensure minimum display duration - const loadingStartTimeRef = useRef(null); - const LOADING_MIN_DISPLAY_MS = 500; // Show loading state for at least 500ms so users can perceive it const streamViewportRef = useRef(streamViewport); const previousIsLoadingSessionRef = useRef(state.isLoadingSession); const previousReceivedInitStateRef = useRef(state.receivedInitState); @@ -3244,48 +3241,9 @@ function ChatContent() { ); } - // Keep the loading bubble visible for the entire active assistant turn. - // Live stream payloads can arrive before the final assistant message is - // finalized, but the user still needs a clear "AI is responding" signal. - const streamingSteps = state.streaming?.steps ?? []; - const streamingProgressEvents = state.streaming?.progressEvents ?? []; - const streamingEdits = state.streaming?.edits ?? []; - const streamingInteractiveEvents = state.streaming?.interactiveEvents ?? []; - const interactiveEvents = state.interactiveEvents ?? []; - const hasAssistantText = - !!state.streaming?.content && - state.streaming.content.trim().length > 0; - const hasVisibleStreamingPayload = Boolean( - state.streaming && - (state.streaming.content.trim().length > 0 || - state.streaming.reasoning.trim().length > 0 || - streamingSteps.length > 0 || - streamingProgressEvents.length > 0 || - streamingEdits.length > 0 || - streamingInteractiveEvents.length > 0 || - interactiveEvents.length > 0), - ); - // Show AI response loading indicator (thinking bubble) when: - // 1. NOT switching sessions (session loading takes precedence), AND - // 2. AI is still responding and the assistant turn has not finalized yet. - // FIXED: Use hasRenderableContent from SDK instead of checking content length. - // - // Loading must dismiss as soon as ANY visible streaming activity arrives — - // not just text content. Tool calls, reasoning, progress events, and - // interactive prompts all represent the assistant doing work that belongs - // in the streaming card, not behind a ThinkingBubble placeholder. Keeping - // the bubble up when these are already in state was causing users to see - // "stuck loading" even though tool calls had arrived. - const streamingForActivity = state.streaming; - const hasRenderableStreamingContent = Boolean( - streamingForActivity?.hasRenderableContent || - (streamingForActivity?.isActive && - ((streamingForActivity.reasoning?.length ?? 0) > 0 || - (streamingForActivity.reasoningEvents?.length ?? 0) > 0 || - (streamingForActivity.steps?.length ?? 0) > 0 || - (streamingForActivity.progressEvents?.length ?? 0) > 0 || - (streamingForActivity.interactiveEvents?.length ?? 0) > 0)), - ); + // Keep the loading ticker in sync with the Stop control throughout the live + // assistant turn. Stream content and activity are additive status, not a + // reason to hide the only explicit "AI is responding" signal. let hasTerminalAssistantBlock = false; let terminalAssistantMessageId: string | null = null; @@ -3355,40 +3313,32 @@ function ChatContent() { Boolean(activeStreamingMessageId) && Boolean(terminalAssistantMessageId) && activeStreamingMessageId !== terminalAssistantMessageId; + // A newly submitted turn can be live before the SDK assigns its assistant + // messageId. Do not let the previous assistant turn's finish marker hide + // the loading ticker during that handoff. + const hasNewLiveTurnBeforeAssistantIdentity = + Boolean(state.streaming?.isActive) || + state.assistantTurnPending || + visiblePendingUserMessages.length > 0; const isAiResponseBlockFinished = Boolean( - (state.streaming && !state.streaming.isActive) || + (state.streaming && + !state.streaming.isActive && + !hasNewLiveTurnBeforeAssistantIdentity) || (hasTerminalAssistantBlock && !hasNewerUserMessageAfterTerminalAssistant && - !hasStartedNewAssistantTurn) + !hasStartedNewAssistantTurn && + !hasNewLiveTurnBeforeAssistantIdentity) ); const showAiResponseLoading = !state.isLoadingSession && // Direct state check to avoid timing issues hasLiveAssistantTurn && !state.isCompacting && - !hasRenderableStreamingContent && !isAiResponseBlockFinished; - // Enforce minimum display duration for loading state - // This ensures users can perceive the loading indicator even when content arrives quickly - const now = Date.now(); - const loadingElapsedTime = loadingStartTimeRef.current ? now - loadingStartTimeRef.current : 0; - - if (showAiResponseLoading && !loadingStartTimeRef.current) { - // Loading state just started - record the timestamp - loadingStartTimeRef.current = now; - } else if (!showAiResponseLoading && loadingStartTimeRef.current) { - // Loading state ended - reset the timestamp - loadingStartTimeRef.current = null; - } - - // Extend the loading state display time if content arrived too quickly - const showExtendedLoading = - showAiResponseLoading || // Normal loading state - (loadingStartTimeRef.current && - loadingElapsedTime < LOADING_MIN_DISPLAY_MS && - !hasRenderableStreamingContent && - hasLiveAssistantTurn); // Extended for minimum duration + // Stop and the loading ticker must share the same visible live-turn state. + // Keep the shell-specific guard as a fallback, but Stop takes priority. + const showExtendedLoading = hasLiveAssistantTurn || showAiResponseLoading; useEffect(() => { if (!state.isLoadingSession && !showAiResponseLoading && !showExtendedLoading) { @@ -3406,7 +3356,7 @@ function ChatContent() { streamingMessageId: state.streaming?.messageId ?? null, hasLiveAssistantTurn, isAiResponding, - hasRenderableStreamingContent, + hasNewLiveTurnBeforeAssistantIdentity, isAiResponseBlockFinished, showAiResponseLoading, showExtendedLoading: Boolean(showExtendedLoading), @@ -3414,7 +3364,7 @@ function ChatContent() { }); }, [ hasLiveAssistantTurn, - hasRenderableStreamingContent, + hasNewLiveTurnBeforeAssistantIdentity, isAiResponding, isAiResponseBlockFinished, renderMessages.length, diff --git a/webview/shared/src/chat/MessageComponents.tsx b/webview/shared/src/chat/MessageComponents.tsx index ec69265..329c2b7 100644 --- a/webview/shared/src/chat/MessageComponents.tsx +++ b/webview/shared/src/chat/MessageComponents.tsx @@ -318,6 +318,26 @@ function isSyntheticUserToolTextPart(text: string): boolean { return text.includes("") && text.includes("") && text.includes(""); } +function textFromAttachedDataUrl(part: MessagePart): string | undefined { + const mime = typeof part.mime === "string" ? part.mime.toLowerCase() : ""; + const url = typeof part.url === "string" ? part.url : ""; + if (part.type !== "file" || !mime.startsWith("text/") || !url.startsWith("data:")) { + return undefined; + } + const commaIndex = url.indexOf(","); + if (commaIndex < 0) return undefined; + const metadata = url.slice(0, commaIndex).toLowerCase(); + const payload = url.slice(commaIndex + 1); + try { + if (metadata.includes(";base64")) { + return atob(payload); + } + return decodeURIComponent(payload); + } catch { + return undefined; + } +} + function isRenderableUserTextPart(part: MessagePart): boolean { if (!isRenderableAssistantTextPart(part)) { return false; @@ -1194,6 +1214,13 @@ function messageBodyFromParts( if (!parts) { return ""; } + const attachmentContents = new Set( + parts + .map((part) => asRecord(part) as MessagePart | undefined) + .filter((part): part is MessagePart => !!part) + .map((part) => textFromAttachedDataUrl(part)) + .filter((text): text is string => typeof text === "string"), + ); return parts .map((part) => { const partRec = asRecord(part); @@ -1207,12 +1234,13 @@ function messageBodyFromParts( ) { return ""; } - return ( + const text = ( (partRec.message as string | undefined) ?? (partRec.text as string | undefined) ?? (partRec.content as string | undefined) ?? "" ).trim(); + return role?.toLowerCase() === "user" && attachmentContents.has(text) ? "" : text; }) .filter((partText) => partText.length > 0) .join("\n\n") @@ -8831,16 +8859,24 @@ const centralizedRawResponse = message?.rawResponse; } }, [subagents.length, dispatch]); + // Mirror latest subagents / subagentDetailsById into refs so the + // effects below can keep deps minimal — otherwise the 1500ms poll + // interval tears down on every stream event. + const latestSubagentsRef = useRef(subagents); + latestSubagentsRef.current = subagents; + const latestSubagentDetailsByIdRef = useRef(subagentDetailsById); + latestSubagentDetailsByIdRef.current = subagentDetailsById; + useEffect(() => { if (!selectedSubagentId) { return; } - const selected = subagents.find((entry) => entry.id === selectedSubagentId); + const selected = latestSubagentsRef.current.find((entry) => entry.id === selectedSubagentId); if (!selected) { return; } const detail = - (subagentDetailsById?.[selected.id] as SubagentDetail | undefined) || + (latestSubagentDetailsByIdRef.current?.[selected.id] as SubagentDetail | undefined) || (selected as unknown as SubagentDetail); const childSessionId = detail.childSessionId || selected.childSessionId; const parentSessionId = detail.parentSessionId || selected.parentSessionId; @@ -8868,18 +8904,18 @@ const centralizedRawResponse = message?.rawResponse; status: selected.status, latestActivity: selected.latestActivity, }); - }, [selectedSubagentId, subagents, subagentDetailsById]); + }, [selectedSubagentId]); useEffect(() => { if (!selectedSubagentId) { return; } - const selected = subagents.find((entry) => entry.id === selectedSubagentId); + const selected = latestSubagentsRef.current.find((entry) => entry.id === selectedSubagentId); if (!selected) { return; } const detail = - (subagentDetailsById?.[selected.id] as SubagentDetail | undefined) || + (latestSubagentDetailsByIdRef.current?.[selected.id] as SubagentDetail | undefined) || (selected as unknown as SubagentDetail); const childSessionId = detail.childSessionId || selected.childSessionId; const parentSessionId = detail.parentSessionId || selected.parentSessionId; @@ -8897,21 +8933,27 @@ const centralizedRawResponse = message?.rawResponse; // Keep modal conversation/timeline fresh while an active subagent is running. const intervalId = window.setInterval(() => { + const currentSelected = latestSubagentsRef.current.find( + (entry) => entry.id === selectedSubagentId, + ); + if (!currentSelected) { + return; + } vscode.postMessage({ type: "getSubagentConversation", - subagentId: selected.id, + subagentId: currentSelected.id, childSessionId, parentSessionId, parentMessageId, - status: selected.status, - latestActivity: selected.latestActivity, + status: currentSelected.status, + latestActivity: currentSelected.latestActivity, }); }, 1500); return () => { window.clearInterval(intervalId); }; - }, [selectedSubagentId, subagents, subagentDetailsById]); + }, [selectedSubagentId]); const hasStreamingActivity = !!( streaming && @@ -11199,28 +11241,52 @@ export const FileChangesSection = memo(function FileChangesSection({ ); }); +function isStreamingForThisCard( + streaming: StreamingState | undefined, + messageId: string | undefined, +): boolean { + return Boolean( + streaming && + streaming.isActive !== false && + messageId && + streaming.messageId === messageId, + ); +} + function areResponseMessagePropsEqual( prevProps: Readonly[0]>, nextProps: Readonly[0]>, ): boolean { - return ( - prevProps.message === nextProps.message && - prevProps.streaming === nextProps.streaming && - prevProps.hideLoadingText === nextProps.hideLoadingText && - prevProps.isContiguous === nextProps.isContiguous && - prevProps.interactiveEvents === nextProps.interactiveEvents && - prevProps.messages?.length === nextProps.messages?.length && - prevProps.currentSessionId === nextProps.currentSessionId && - prevProps.hideFileChangesSection === nextProps.hideFileChangesSection && - prevProps.todoItems === nextProps.todoItems && - prevProps.blockGroupKey === nextProps.blockGroupKey && - prevProps.isLastInBlock === nextProps.isLastInBlock && - prevProps.isBlockExpanded === nextProps.isBlockExpanded && - prevProps.isBlockStreaming === nextProps.isBlockStreaming && - prevProps.isBlockHeaderAnchor === nextProps.isBlockHeaderAnchor && - prevProps.blockSize === nextProps.blockSize && - prevProps.isHiddenByBlock === nextProps.isHiddenByBlock - ); + if ( + prevProps.message !== nextProps.message || + prevProps.hideLoadingText !== nextProps.hideLoadingText || + prevProps.isContiguous !== nextProps.isContiguous || + prevProps.interactiveEvents !== nextProps.interactiveEvents || + prevProps.messages?.length !== nextProps.messages?.length || + prevProps.currentSessionId !== nextProps.currentSessionId || + prevProps.hideFileChangesSection !== nextProps.hideFileChangesSection || + prevProps.todoItems !== nextProps.todoItems || + prevProps.blockGroupKey !== nextProps.blockGroupKey || + prevProps.isLastInBlock !== nextProps.isLastInBlock || + prevProps.isBlockExpanded !== nextProps.isBlockExpanded || + prevProps.isBlockStreaming !== nextProps.isBlockStreaming || + prevProps.isBlockHeaderAnchor !== nextProps.isBlockHeaderAnchor || + prevProps.blockSize !== nextProps.blockSize || + prevProps.isHiddenByBlock !== nextProps.isHiddenByBlock + ) { + return false; + } + + // Skip streaming identity changes for non-streaming cards. Without this, + // every stream batch forces EVERY mounted ResponseMessage to rerender. + const prevMessageId = prevProps.message?.id ?? prevProps.message?.info?.id; + const nextMessageId = nextProps.message?.id ?? nextProps.message?.info?.id; + const prevWasStreaming = isStreamingForThisCard(prevProps.streaming, prevMessageId); + const nextIsStreaming = isStreamingForThisCard(nextProps.streaming, nextMessageId); + if (prevWasStreaming || nextIsStreaming) { + return prevProps.streaming === nextProps.streaming; + } + return true; } export const ResponseMessage = memo(function ResponseMessage({ diff --git a/webview/shared/src/chat/StreamingComponents.tsx b/webview/shared/src/chat/StreamingComponents.tsx index 2d01674..4f4d753 100644 --- a/webview/shared/src/chat/StreamingComponents.tsx +++ b/webview/shared/src/chat/StreamingComponents.tsx @@ -168,6 +168,8 @@ export function shouldShowStreamingCard({ // Delta chunks deliberately do not enter the centralized transcript. Keep // the live card mounted for the active turn so its renderable text reaches // the user immediately; the centralized card takes over after completion. + // A matching transcript message is the exception: rendering both sources + // during the handoff duplicates the same thoughts and response body. if (hasTranscriptAssistantForCurrentTurn && !streaming.isActive) return false; const candidateIds = new Set( @@ -181,7 +183,7 @@ export function shouldShowStreamingCard({ Array.isArray(transcriptAssistantMessageIds) && transcriptAssistantMessageIds.some((messageId) => candidateIds.has(messageId)); - if (hasMatchingAssistantTurnInTranscript && !streaming.isActive) return false; + if (hasMatchingAssistantTurnInTranscript) return false; const hasRenderableText = streaming.hasRenderableContent === true && diff --git a/webview/shared/src/chat/lib/types.ts b/webview/shared/src/chat/lib/types.ts index 7135ef1..fc2775c 100644 --- a/webview/shared/src/chat/lib/types.ts +++ b/webview/shared/src/chat/lib/types.ts @@ -368,6 +368,8 @@ export interface MessagePartSource { export interface MessagePart { type?: string; text?: string; + /** True for SDK transport text that is not user-visible message content. */ + synthetic?: boolean; content?: string; message?: string; reasoning?: string;