perf: audit startup reliability fixes#1878
Merged
Merged
Conversation
Generated provider catalog and ACP registry refresh from build preflight.
Verified audit report covering 17 findings, plus SDD spec/plan/tasks. 12 per-finding fix docs, each reviewed against current code with locate/fix/verify.
Contributor
📝 WalkthroughWalkthroughThis PR implements twelve performance-audit remediation items: startup workload coordination via StartupWorkloadCoordinator, SQLite init phase timing, MCP soft/hard connect timeouts and bounded-concurrency shutdown, async protocol handlers, deferred ACP notifications, lazy markdown workers, message store LRU/ordering, provider list collapse, session fetch dedup/hydration, model icon registry, generated icon whitelist, pending-assistant de-duplication, and ACP stream provider/model metadata propagation, alongside extensive documentation, model/ACP registry data updates, and tests. ChangesPerformance Audit Fixes
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant McpPresenter
participant ServerManager
participant McpClient
participant SDKConnection
McpPresenter->>ServerManager: startServer(name, { onBackgroundConnected })
ServerManager->>McpClient: connect({ phase: 'startup' })
McpClient->>SDKConnection: initiate connect
alt connects before soft timeout
SDKConnection-->>McpClient: connected
McpClient-->>ServerManager: 'connected'
ServerManager-->>McpPresenter: 'connected'
McpPresenter->>McpPresenter: emit SERVER_STARTED
else soft timeout elapses
McpClient-->>ServerManager: 'soft-timeout-released'
ServerManager-->>McpPresenter: 'soft-timeout-released'
McpClient->>McpClient: emit 'retrying' in background
SDKConnection-->>McpClient: connected later
McpClient->>ServerManager: getConnectionCompletion() resolves
ServerManager->>ServerManager: handleStartupConnectResult()
ServerManager->>McpPresenter: onBackgroundConnected()
end
sequenceDiagram
participant AgentRuntimePresenter
participant Dispatch
participant EventBus
participant SessionStore
participant MessageStore
AgentRuntimePresenter->>Dispatch: flushBlocksToRenderer(io)
Dispatch->>EventBus: publish chat.stream.updated { providerId, modelId, blocks }
EventBus->>MessageStore: applyStreamingBlocksToMessage(id, sessionId, blocks, metadata)
MessageStore->>MessageStore: set pending assistant metadata
SessionStore->>SessionStore: hydrateActiveSessionSummary(sessionId)
SessionStore->>SessionStore: getActive() before routing
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/main/presenter/mcpPresenter/mcpClient.ts (1)
594-619: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTreat connect cancellation as a stop, not a failure
disconnect()already marks the client as stopped, but the in-flightconnect()still throws a generic error and thecatchpath emitsfailed/connect-error. That turns a user-initiated stop during startup into a false startup failure and error notification. Handle the cancellation path separately and letServerManagerignore the rejected connect for a stopped client.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/mcpPresenter/mcpClient.ts` around lines 594 - 619, Treat connect cancellation as a stopped state instead of a startup failure in mcpClient.ts: the `connect()` flow currently throws a generic error when `connectAborted` is set, which then falls into the `catch` block and emits `failed/connect-error`. Update the `connect()`/`catch` handling to detect this cancellation case separately, avoid emitting a failure status or error log for a user-initiated stop, and let `ServerManager` treat a rejected connect from a stopped client as expected. Use the existing `connectAborted`, `disconnect()`, `cleanupResources()`, and `emitServerStatusChanged()` paths to keep the stopped state consistent.src/main/presenter/agentSessionPresenter/index.ts (3)
3448-3529: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftAvoid writes inside the active
.iterate()cursor.openSQLiteDatabase()only sets WAL mode; it does not enableunsafeMode(), so thereplaceForSession(),upsert(), andsetAgentSetting()calls here can throw the connection-busyTypeErroron the shared handle. Materialize the rows first or split the read and write phases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentSessionPresenter/index.ts` around lines 3448 - 3529, The `runMainlineNormalizationBackfill` flow is performing writes (`replaceForSession`, `upsert`, and `setAgentSetting`) while still iterating over `sessionRows` and `messageRows` from the same SQLite handle. Refactor this method to separate the read phase from the write phase by materializing the results of the `.iterate()` cursors first, then processing the writes afterward, or otherwise batch the read and write steps outside the active cursor. Use `runMainlineNormalizationBackfill` and the affected table calls as the main points to update.
3737-3838: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid writes inside the live usage-candidate cursor
iterAssistantUsageCandidates()is a live.iterate()cursor, and this loop writes viausageStatsTable.upsert(...)(plussetUsageStatsBackfillStatus(...)) on the same connection. That can tripSQLITE_BUSYand abort the backfill; materialize the rows first or use a separate connection for the writes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentSessionPresenter/index.ts` around lines 3737 - 3838, The live cursor returned by iterAssistantUsageCandidates() is being consumed while runUsageStatsBackfill() writes with usageStatsTable.upsert() and updates setUsageStatsBackfillStatus() on the same connection, which can trigger SQLITE_BUSY. Fix this by materializing the candidate rows before entering the write loop, or by switching the writes to a separate database connection, so the backfill does not read and write through the same active cursor.
3571-3599: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid writing while streaming
new_sessionshere.sessionManager.updateDisabledAgentTools()uses the same SQLite connection, andbetter-sqlite3rejects writes while a.iterate()cursor is open. Load the rows with.all()first, then update in a second pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentSessionPresenter/index.ts` around lines 3571 - 3599, The streaming read over new_sessions in the session migration loop is still writing through sessionManager.updateDisabledAgentTools() while a .iterate() cursor may be open, which conflicts with better-sqlite3. Update the logic in agentSessionPresenter/index.ts so the sessionRowsStatement result is fully materialized with .all() before any writes happen, then perform the disabledAgentTools normalization and update pass over that in-memory array using normalizeDisabledAgentTools and updateDisabledAgentTools separately.resources/model-db/providers.json (1)
215634-215701: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
grok-4-20-non-reasoningstill defaults reasoning onIn
resources/model-db/providers.json, this row mirrors the reasoning variant (reasoning.default: trueandextra_capabilities.reasoning.supported: true), so it won’t behave like a non-reasoning model. Setreasoning.defaulttofalsehere; if this alias should truly be non-reasoning,reasoning.supportedshould match that too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/model-db/providers.json` around lines 215634 - 215701, The grok-4-20-non-reasoning entry is still configured like the reasoning variant in providers.json. Update the grok-4-20-non-reasoning model record so its reasoning.default is false, and verify whether reasoning.supported in both the reasoning block and extra_capabilities should also be disabled to match the non-reasoning alias.
🧹 Nitpick comments (8)
src/main/presenter/lifecyclePresenter/hooks/beforeStart/protocolRegistrationHook.ts (1)
156-159: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winStream
deepcdnresponses too.This path still buffers the full asset with
fsp.readFile, which undercuts the new streaming helper for larger.wasm/.dataresources.Proposed streaming update
-const fileContent = await fsp.readFile(fullPath) -return new Response(fileContent, { - headers: { 'Content-Type': mimeType } +const stat = await fsp.stat(fullPath) +if (stat.isDirectory()) { + return new Response(`File not found: ${filePath}`, { + status: 404, + headers: { 'Content-Type': 'text/plain' } + }) +} + +return await createStreamingResponse(fullPath, stat, { + headers: { 'Content-Type': mimeType } })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/lifecyclePresenter/hooks/beforeStart/protocolRegistrationHook.ts` around lines 156 - 159, In protocolRegistrationHook, the current Response construction still buffers assets with fsp.readFile, so update this deepcdn path to use the existing streaming helper instead of reading the whole file into memory. Locate the handler that returns the asset Response and replace the fileContent-based flow with streamed output for large .wasm and .data resources, keeping the same content-type header behavior.src/renderer/src/stores/ui/message.ts (2)
191-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaming nit: these comparators are recursive (deep), not shallow.
shallowPayloadValueEqualrecurses through arrays/records viashallowArrayEqual/shallowRecordEqual, so it performs a full structural comparison rather than a one-level one. The behavior matches the replacedJSON.stringifyequality, but theshallow*naming may mislead future readers into assuming a cheap one-level check. Consider renaming todeep*/structural*for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/stores/ui/message.ts` around lines 191 - 218, The comparator helpers in message.ts are recursive structural checks, so the current shallow* names are misleading. Rename shallowPayloadValueEqual, shallowArrayEqual, and shallowRecordEqual to reflect deep/structural equality, and update their internal references accordingly so the names match the actual behavior.
129-148: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDerive
hasMessageIdfromcachedRecord.
messageIds.value.includes(record.id)is an O(n) scan on every streaming update. The cache lookup already gives you the same signal here, so this can stay O(1) on the hot path.♻️ Derive existence from the cache lookup
function upsertMessageRecord(record: ChatMessageRecord): void { const cachedRecord = messageCache.value.get(record.id) - const hasMessageId = messageIds.value.includes(record.id) + const hasMessageId = cachedRecord !== undefined messageCache.value.set(record.id, record)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/stores/ui/message.ts` around lines 129 - 148, The hot path in upsertMessageRecord is doing an unnecessary O(n) includes scan to compute hasMessageId. Derive that existence check from the existing cachedRecord lookup in messageCache.value.get(record.id), then keep the rest of the flow in upsertMessageRecord unchanged so streaming updates stay O(1) when the message is already cached.docs/architecture/perf-fixes-from-audit/fix-V1-settings-hidden-route.md (1)
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared resolver in the example.
The
#/mcpliteral makes the sample route-specific and undermines the recommendation to source paths fromresolveSettingsNavigationPath(). Please show the helper returning the hash instead of hardcoding one route.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/perf-fixes-from-audit/fix-V1-settings-hidden-route.md` around lines 27 - 35, The example in the settings E2E helper still hardcodes a route-specific hash, which bypasses the shared navigation resolver. Update the sample and `openSettingsTab()` flow to derive the hidden-route hash from `resolveSettingsNavigationPath()` (via the existing `getSettingsRouteItems()`/routeName lookup) instead of embedding `#/mcp` directly, so the helper can navigate hidden settings pages generically.test/e2e/helpers/settings.ts (1)
57-62: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRequire
routeNamefor hidden-route navigation. The no-routeNamebranch is still used by the hidden-route smoke tests, and the tab-id heuristic is already inconsistent withsettings-acp(settings-tab-acp-agents). PassrouteNamethrough those callers and remove the fallback to avoid silent mismatches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/helpers/settings.ts` around lines 57 - 62, The tab ID fallback in getRouteTabTestId is still being used for hidden-route navigation and can produce mismatched IDs, especially for settings-acp. Update the hidden-route callers to always pass routeName through to getRouteTabTestId, then remove the item.routeName.replace(/^settings-/, '') fallback so the ID is derived only from the route path segment or an explicit routeName input.src/main/presenter/agentSessionPresenter/index.ts (1)
1710-1716: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
startRtkHealthCheckTaskdoesn't accept ataskContext, unlike its siblings.Every other
start*Taskmethod in this file threads throughStartupWorkloadTaskContextfor cooperative yielding/progress, but this one doesn't. If RTK health check work is short/bounded this is fine, but worth a quick sanity check that it can't block the coordinator's resource slot for long without yielding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentSessionPresenter/index.ts` around lines 1710 - 1716, The RTK health check task is inconsistent with the other start*Task methods because startRtkHealthCheckTask does not accept or thread through StartupWorkloadTaskContext. Update startRtkHealthCheck and startRtkHealthCheckTask in agentSessionPresenter/index.ts to either accept the same taskContext parameter and pass it into rtkRuntimeService.startHealthCheck, or explicitly keep it non-cooperative only if the health check is guaranteed short and bounded; align the behavior with the sibling task methods and verify the runtime service API supports the chosen approach.test/renderer/components/ChatStatusBar.test.ts (1)
1914-1928: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't actually exercise coalescing — assertion is trivially true.
With
hasActiveSession: false,sessionStore.activeSessionIdstaysnull, sorunSyncGenerationSettingsnever enters theif (sessionId)branch andagentSessionPresenter.getSessionGenerationSettingsis never called — regardless of whether the sync calls are coalesced or run N times. Additionally, mutatingdraftStore.providerId/draftStore.modelIddirectly doesn't route through anything watched:draftModelSelection(the ref actually watched at the generation-sync trigger) is only updated insidesyncDraftModelSelection(), and no watcher in this file depends on rawdraftStore.providerId/modelId. So this test would pass identically even if thegenerationSyncQueuedcoalescing wrapper were removed entirely, providing no real regression protection for the feature it's named after.Consider using an active session and asserting the mock is called exactly once despite multiple same-tick changes, e.g.:
🧪 Suggested rewrite to actually validate coalescing
- it('coalesces generation settings syncs triggered in the same tick', async () => { - const { draftStore, agentSessionPresenter } = await setup({ - hasActiveSession: false, - defaultModel: { providerId: 'openai', modelId: 'gpt-4' }, - preferredModel: undefined - }) - await flushPromises() - agentSessionPresenter.getSessionGenerationSettings.mockClear() - - draftStore.providerId = 'anthropic' - draftStore.modelId = 'claude-3-5-sonnet' - await flushPromises() - - expect(agentSessionPresenter.getSessionGenerationSettings).not.toHaveBeenCalled() - }) + it('coalesces generation settings syncs triggered in the same tick', async () => { + const { sessionStore, agentSessionPresenter } = await setup({ + hasActiveSession: true, + activeProviderId: 'openai', + activeModelId: 'gpt-4' + }) + await flushPromises() + agentSessionPresenter.getSessionGenerationSettings.mockClear() + + // Trigger multiple dependency changes synchronously within the same tick. + sessionStore.activeSession.providerId = 'anthropic' + sessionStore.activeSession.modelId = 'claude-3-5-sonnet' + await flushPromises() + + expect(agentSessionPresenter.getSessionGenerationSettings).toHaveBeenCalledTimes(1) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/renderer/components/ChatStatusBar.test.ts` around lines 1914 - 1928, The current test does not validate coalescing because `hasActiveSession: false` prevents `runSyncGenerationSettings` from calling `agentSessionPresenter.getSessionGenerationSettings`, and direct `draftStore.providerId`/`modelId` mutation bypasses the watched `draftModelSelection` path. Update `ChatStatusBar.test.ts` to use an active session and trigger the real sync flow via `syncDraftModelSelection`/the watched ref, then assert `agentSessionPresenter.getSessionGenerationSettings` is called exactly once after multiple same-tick changes so the `generationSyncQueued` behavior is actually covered.src/renderer/src/components/icons/ModelIcon.vue (1)
197-220: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove the candidate-key cache to module scope If this is meant to speed up many
<ModelIcon>instances,ICON_CANDIDATE_KEYS,iconMatchCache, andresolveIconKeyneed to live outside<script setup>; right now each mount gets a freshMap, so the cache only helps within a single instance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/icons/ModelIcon.vue` around lines 197 - 220, The icon lookup cache in ModelIcon is currently recreated per component instance, so it only helps within a single mount. Move ICON_CANDIDATE_KEYS, iconMatchCache, and resolveIconKey out of the <script setup> instance scope into module scope so all <ModelIcon> instances share the same cached icon matches and reuse the same lookup logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture/perf-fixes-from-audit/fix-F03-backfill-coordinator.md`:
- Around line 13-52: The markdown links in this doc are resolving relative to
the docs folder instead of the repo root because they are missing the proper
parent path prefix. Update every referenced path in the affected section to use
the correct repo-relative form so they point to the actual source files,
including the links for startupWorkloadCoordinator, agentSessionPresenter, and
rtkRuntimeService symbols mentioned in the review comment.
In `@docs/architecture/perf-fixes-from-audit/fix-F05-mcp-startup-timeout.md`:
- Around line 10-24: The relative references in this section are using the wrong
prefix, so they resolve from the docs folder instead of the repository root.
Update the links in this block to use the same repo-root relative form as the
later references in the file, and check the affected markdown section that cites
initializeMcp, McpPresenter.initialize, McpPresenter.shutdown,
ServerManager.startServer, McpClient.emitServerStatusChanged, McpClient.connect,
and checkAndHandleSessionError.
In `@docs/architecture/perf-fixes-from-audit/fix-F08-shutdown-observability.md`:
- Around line 9-16: The markdown links in this audit note are resolving relative
to the current folder instead of the repo root, so update the referenced symbols
like requestShutdown(), executeHooksByPriority(), Presenter.destroy(),
PluginHost.shutdown(), McpPresenter.shutdown(), McpClient, and
serverManager.stopServer() to use the correct ../../../ prefix. Also remove the
stray backtick after skill in the destroy-step list so the markdownlint warning
is cleared.
In `@docs/architecture/perf-fixes-from-audit/fix-F09-markdown-workers-lazy.md`:
- Around line 7-46: The markdown document has broken relative links because the
refs point as if they were relative to the current folder instead of the repo
root. Update every affected link in this doc by adding the repo-root prefix
`../../../` so the references to `src/renderer/src/main.ts`,
`src/renderer/src/lib/markdownWorkerLifecycle.ts`,
`src/renderer/src/components/markdown/MarkdownRenderer.vue`, and
`src/renderer/src/components/think-content/ThinkContent.vue` resolve correctly
from `docs/architecture/perf-fixes-from-audit/`.
In `@docs/architecture/perf-fixes-from-audit/fix-F11-icons-bundle-slim.md`:
- Around line 70-77: The current whitelist filtering in iconLoader.ts still
imports the full `@iconify-json/`*/icons.json payload, so the unused icon data
remains in the emitted chunk. Move the whitelist generation to build time by
adding a script that scans source icon literals and emits
icon-whitelist.generated.ts or JSON, then have preloadIcons() and
ensureIconsLoaded() consume the reduced pack/module before importing icons.json
so only the needed icons are bundled and registered.
In `@docs/architecture/perf-fixes-from-audit/fix-F13-message-store-sort.md`:
- Around line 46-90: The tie-breaker in isMessageIdsSortedByOrderSeq and
findInsertIndexByOrderSeq currently uses localeCompare(), which can produce
locale-dependent ordering and diverge from the fallback sort for message IDs.
Replace it with a deterministic string comparison in both places so the fast
binary-insert path and the validation path use the same pure lexicographic
order.
In `@docs/architecture/perf-fixes-from-audit/fix-F16-provider-list-render.md`:
- Around line 25-30: The iconKey optimization in the provider list rendering
must preserve the existing first-match precedence from the current
Object.keys(icons) scan. Avoid changing candidate ordering in a way that lets
longer keys win unless you add regression coverage proving the same behavior;
otherwise keep the original iteration order while optimizing elsewhere, such as
caching resolved modelId/apiType matches or reusing a prebuilt candidate list
without altering match semantics.
In `@docs/architecture/perf-fixes-from-audit/spec.md`:
- Around line 20-32: The “不修复” section has a count mismatch: the heading says 6
items, but only 5 findings are listed and the closing note also refers to 5.
Update the heading and summary to match the actual enumerated scope, or add the
missing finding if one was intended, keeping the list consistent with report.md
references.
In `@resources/model-db/providers.json`:
- Around line 93470-93501: The model entry for
Qwen/Qwen3-Coder-480B-A35B-Instruct-Turbo has a likely typo in its token limit
data: update the limit.output value from 66536 to 65536 to match the intended
tier used by similar models. Make this correction in the provider metadata for
that model only, keeping the rest of the entry unchanged.
- Around line 211783-211857: The model entries for gemini-3.1-flash-image,
gemini-3-pro-image, gemini-2.5-flash-image, and gpt-image-2 are still configured
as chat models, which causes them to be routed incorrectly. Update these
provider records in providers.json so they are marked as imageGeneration and
include the proper image modalities, keeping the existing reasoning settings
intact. Use the IDs above to locate the affected entries and ensure the model
config flow no longer infers them as chat-only.
In `@src/main/presenter/agentSessionPresenter/index.ts`:
- Around line 3505-3560: The `messageRows` cursor in
`runSQLiteMainlineNormalizationBackfill` is kept open while
`backfillNormalizedMessageRow()` and `yieldForBatch()` perform writes on the
same better-sqlite3 connection, which can trigger a busy-statement failure.
Materialize the `deepchat_messages` query result up front using `all()` or
otherwise buffer the rows before entering the write/backfill loop, then iterate
over the in-memory list instead of the live cursor.
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 621-640: Move the pending ACP flush in setAgentRepository into the
finally block so it always executes even if initializeUnifiedAgents(),
reconcileLegacyBuiltinAgentSelections(), or
cleanupDeprecatedBuiltinAgentSelections() throws. Keep the
isAttachingAgentRepository reset in the same finally, and consume
pendingAcpAgentsChanged there before returning by queueing
notifyAcpAgentsChanged() so no attachment-time notification is stranded.
In `@src/main/presenter/index.ts`:
- Around line 972-1027: The teardown flow in destroy() is still failing fast
because runDestroyStep() rethrows, so cleanup after memoryPresenter.dispose,
sqlitePresenter.close, workspacePresenter.destroy, and skillPresenter.destroy
can be skipped. Update destroy() and/or runDestroyStep() so these teardown steps
are best-effort: keep the existing logger warnings in runDestroyStep, but do not
propagate the error from these unguarded cleanup calls, allowing the remaining
destroy steps to continue executing.
In
`@src/main/presenter/lifecyclePresenter/hooks/beforeStart/protocolRegistrationHook.ts`:
- Around line 152-153: The path handling in protocol registration is vulnerable
because `deepcdn` and `imgcache` build filesystem paths directly from
request.url before stat/read/stream operations. Update the
`register...`/protocol handler logic in `protocolRegistrationHook.ts` to apply
the same root-containment check used by `workspace-preview`, ensuring the
resolved path stays under the intended `cdn` or `images` root before any
filesystem access. Use the existing path construction around
`findDeepCdnResourcesDir`, `path.join`, and the related `stat`, `readFile`, or
stream code paths to add the containment guard consistently.
In `@src/main/presenter/mcpPresenter/index.ts`:
- Around line 257-260: The shutdown flow in mcpPresenter’s client loop is still
serial, so a hung MCP server can make shutdown scale linearly with active
clients. Update the shutdown logic around `getActiveClients()` and
`stopServerDuringShutdown()` to run with a small bounded worker pool or limited
concurrency, similar to the plugin shutdown approach, so multiple MCP clients
are stopped in parallel without making app shutdown unbounded.
In `@src/main/presenter/mcpPresenter/mcpClient.ts`:
- Around line 276-328: The connect lifecycle is dropping the requested phase
because `performConnect()` always emits completion events as `startup` even when
`connect({ phase })` was called with another phase. Thread the `phase` from
`connect`/`waitForConnectSoftTimeout` into `performConnect`, then use that value
when emitting the `connected` and `failed` server status events so they match
the original request.
- Around line 288-305: Make the soft-timeout timer local to each waiter instead
of storing it on the shared startupSoftTimeout field in
waitForConnectSoftTimeout. Create and track the timeout handle inside this
method’s Promise.race setup, then clear that specific handle in both the success
path and the catch/finally path so concurrent connect() waiters do not cancel
each other’s timers.
In `@src/main/presenter/mcpPresenter/serverManager.ts`:
- Around line 258-260: The startup flow in serverManager.startServer() is
treating any resolved client.connect() as a successful “started” state, but a
'soft-timeout-released' result should not trigger start notifications yet.
Update the logic around handleStartupConnectResult and the
startServer/startServerInBackground call path so the McpConnectResult is
propagated to McpPresenter, and only emit/log “started” after
getConnectionCompletion() confirms a real connection. Keep the legacy started
event deferred for soft-timeout cases until the background connection actually
succeeds.
In `@src/main/presenter/pluginPresenter/index.ts`:
- Around line 168-170: The shutdown flow in presenter plugin cleanup only checks
this.mcpPresenter.isServerRunning(serverName), which misses active plugin-owned
MCP clients that are not yet marked running. Update the stop path in this
presenter flow to use an active-aware API on mcpPresenter, or make
mcpPresenter.stopServer(serverName) safely stop active/non-running clients
without emitting a false stopped event. Keep the fix centered around
this.mcpPresenter.isServerRunning and this.mcpPresenter.stopServer so plugin
shutdown always terminates background MCP work.
In `@src/shared/contracts/events/mcp.events.ts`:
- Around line 16-18: Replace the MCP status contract validators that currently
use z.custom in McpServerLifecycleStatusSchema, McpServerStatusPhaseSchema, and
McpServerStatusReasonSchema with z.enum-based schemas backed by the finite
allowed string values. Update the status union in the same events contract so
lifecycleStatus, phase, reason, and status only accept the enum members at
runtime. Keep the schema names and their usage in the MCP event contract aligned
so all event payload validation remains strict and consistent.
---
Outside diff comments:
In `@resources/model-db/providers.json`:
- Around line 215634-215701: The grok-4-20-non-reasoning entry is still
configured like the reasoning variant in providers.json. Update the
grok-4-20-non-reasoning model record so its reasoning.default is false, and
verify whether reasoning.supported in both the reasoning block and
extra_capabilities should also be disabled to match the non-reasoning alias.
In `@src/main/presenter/agentSessionPresenter/index.ts`:
- Around line 3448-3529: The `runMainlineNormalizationBackfill` flow is
performing writes (`replaceForSession`, `upsert`, and `setAgentSetting`) while
still iterating over `sessionRows` and `messageRows` from the same SQLite
handle. Refactor this method to separate the read phase from the write phase by
materializing the results of the `.iterate()` cursors first, then processing the
writes afterward, or otherwise batch the read and write steps outside the active
cursor. Use `runMainlineNormalizationBackfill` and the affected table calls as
the main points to update.
- Around line 3737-3838: The live cursor returned by
iterAssistantUsageCandidates() is being consumed while runUsageStatsBackfill()
writes with usageStatsTable.upsert() and updates setUsageStatsBackfillStatus()
on the same connection, which can trigger SQLITE_BUSY. Fix this by materializing
the candidate rows before entering the write loop, or by switching the writes to
a separate database connection, so the backfill does not read and write through
the same active cursor.
- Around line 3571-3599: The streaming read over new_sessions in the session
migration loop is still writing through
sessionManager.updateDisabledAgentTools() while a .iterate() cursor may be open,
which conflicts with better-sqlite3. Update the logic in
agentSessionPresenter/index.ts so the sessionRowsStatement result is fully
materialized with .all() before any writes happen, then perform the
disabledAgentTools normalization and update pass over that in-memory array using
normalizeDisabledAgentTools and updateDisabledAgentTools separately.
In `@src/main/presenter/mcpPresenter/mcpClient.ts`:
- Around line 594-619: Treat connect cancellation as a stopped state instead of
a startup failure in mcpClient.ts: the `connect()` flow currently throws a
generic error when `connectAborted` is set, which then falls into the `catch`
block and emits `failed/connect-error`. Update the `connect()`/`catch` handling
to detect this cancellation case separately, avoid emitting a failure status or
error log for a user-initiated stop, and let `ServerManager` treat a rejected
connect from a stopped client as expected. Use the existing `connectAborted`,
`disconnect()`, `cleanupResources()`, and `emitServerStatusChanged()` paths to
keep the stopped state consistent.
---
Nitpick comments:
In `@docs/architecture/perf-fixes-from-audit/fix-V1-settings-hidden-route.md`:
- Around line 27-35: The example in the settings E2E helper still hardcodes a
route-specific hash, which bypasses the shared navigation resolver. Update the
sample and `openSettingsTab()` flow to derive the hidden-route hash from
`resolveSettingsNavigationPath()` (via the existing
`getSettingsRouteItems()`/routeName lookup) instead of embedding `#/mcp`
directly, so the helper can navigate hidden settings pages generically.
In `@src/main/presenter/agentSessionPresenter/index.ts`:
- Around line 1710-1716: The RTK health check task is inconsistent with the
other start*Task methods because startRtkHealthCheckTask does not accept or
thread through StartupWorkloadTaskContext. Update startRtkHealthCheck and
startRtkHealthCheckTask in agentSessionPresenter/index.ts to either accept the
same taskContext parameter and pass it into rtkRuntimeService.startHealthCheck,
or explicitly keep it non-cooperative only if the health check is guaranteed
short and bounded; align the behavior with the sibling task methods and verify
the runtime service API supports the chosen approach.
In
`@src/main/presenter/lifecyclePresenter/hooks/beforeStart/protocolRegistrationHook.ts`:
- Around line 156-159: In protocolRegistrationHook, the current Response
construction still buffers assets with fsp.readFile, so update this deepcdn path
to use the existing streaming helper instead of reading the whole file into
memory. Locate the handler that returns the asset Response and replace the
fileContent-based flow with streamed output for large .wasm and .data resources,
keeping the same content-type header behavior.
In `@src/renderer/src/components/icons/ModelIcon.vue`:
- Around line 197-220: The icon lookup cache in ModelIcon is currently recreated
per component instance, so it only helps within a single mount. Move
ICON_CANDIDATE_KEYS, iconMatchCache, and resolveIconKey out of the <script
setup> instance scope into module scope so all <ModelIcon> instances share the
same cached icon matches and reuse the same lookup logic.
In `@src/renderer/src/stores/ui/message.ts`:
- Around line 191-218: The comparator helpers in message.ts are recursive
structural checks, so the current shallow* names are misleading. Rename
shallowPayloadValueEqual, shallowArrayEqual, and shallowRecordEqual to reflect
deep/structural equality, and update their internal references accordingly so
the names match the actual behavior.
- Around line 129-148: The hot path in upsertMessageRecord is doing an
unnecessary O(n) includes scan to compute hasMessageId. Derive that existence
check from the existing cachedRecord lookup in
messageCache.value.get(record.id), then keep the rest of the flow in
upsertMessageRecord unchanged so streaming updates stay O(1) when the message is
already cached.
In `@test/e2e/helpers/settings.ts`:
- Around line 57-62: The tab ID fallback in getRouteTabTestId is still being
used for hidden-route navigation and can produce mismatched IDs, especially for
settings-acp. Update the hidden-route callers to always pass routeName through
to getRouteTabTestId, then remove the item.routeName.replace(/^settings-/, '')
fallback so the ID is derived only from the route path segment or an explicit
routeName input.
In `@test/renderer/components/ChatStatusBar.test.ts`:
- Around line 1914-1928: The current test does not validate coalescing because
`hasActiveSession: false` prevents `runSyncGenerationSettings` from calling
`agentSessionPresenter.getSessionGenerationSettings`, and direct
`draftStore.providerId`/`modelId` mutation bypasses the watched
`draftModelSelection` path. Update `ChatStatusBar.test.ts` to use an active
session and trigger the real sync flow via `syncDraftModelSelection`/the watched
ref, then assert `agentSessionPresenter.getSessionGenerationSettings` is called
exactly once after multiple same-tick changes so the `generationSyncQueued`
behavior is actually covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01f07710-ab67-4ddc-9d02-0472fda08554
📒 Files selected for processing (77)
docs/architecture/perf-fixes-from-audit/fix-F01-sqlite-startup-defer.mddocs/architecture/perf-fixes-from-audit/fix-F03-backfill-coordinator.mddocs/architecture/perf-fixes-from-audit/fix-F05-mcp-startup-timeout.mddocs/architecture/perf-fixes-from-audit/fix-F06-protocol-handler-async.mddocs/architecture/perf-fixes-from-audit/fix-F08-shutdown-observability.mddocs/architecture/perf-fixes-from-audit/fix-F09-markdown-workers-lazy.mddocs/architecture/perf-fixes-from-audit/fix-F11-icons-bundle-slim.mddocs/architecture/perf-fixes-from-audit/fix-F12-session-fetch-dedup.mddocs/architecture/perf-fixes-from-audit/fix-F13-message-store-sort.mddocs/architecture/perf-fixes-from-audit/fix-F15-chatstatusbar-watchers.mddocs/architecture/perf-fixes-from-audit/fix-F16-provider-list-render.mddocs/architecture/perf-fixes-from-audit/fix-V1-settings-hidden-route.mddocs/architecture/perf-fixes-from-audit/plan.mddocs/architecture/perf-fixes-from-audit/spec.mddocs/architecture/perf-fixes-from-audit/tasks.mddocs/features/cron-jobs-timezone-selector/spec.mddocs/issues/acp-startup-notification-order/spec.mddocs/issues/assistant-pending-duplicate/plan.mddocs/issues/assistant-pending-duplicate/spec.mddocs/issues/assistant-pending-duplicate/tasks.mddocs/issues/cron-job-run-insert-values/spec.mddocs/issues/cron-scheduler-heartbeat-status/spec.mddocs/issues/cron-scheduler-stop-exit-error/spec.mddocs/issues/cron-scheduler-utility-start-crash/spec.mddocs/issues/performance-audit-report/plan.mddocs/issues/performance-audit-report/report.mddocs/issues/performance-audit-report/spec.mddocs/issues/performance-audit-report/tasks.mddocs/issues/scheduled-ui-label-layout/spec.mdresources/acp-registry/registry.jsonresources/model-db/providers.jsonsrc/main/appMain.tssrc/main/presenter/agentSessionPresenter/index.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/index.tssrc/main/presenter/lifecyclePresenter/DatabaseInitializer.tssrc/main/presenter/lifecyclePresenter/hooks/after-start/disabledSearchToolCleanupHook.tssrc/main/presenter/lifecyclePresenter/hooks/after-start/legacyImportHook.tssrc/main/presenter/lifecyclePresenter/hooks/after-start/rtkHealthCheckHook.tssrc/main/presenter/lifecyclePresenter/hooks/after-start/sqliteMainlineNormalizationHook.tssrc/main/presenter/lifecyclePresenter/hooks/after-start/usageStatsBackfillHook.tssrc/main/presenter/lifecyclePresenter/hooks/beforeStart/protocolRegistrationHook.tssrc/main/presenter/lifecyclePresenter/hooks/init/databaseInitHook.tssrc/main/presenter/mcpPresenter/index.tssrc/main/presenter/mcpPresenter/mcpClient.tssrc/main/presenter/mcpPresenter/serverManager.tssrc/main/presenter/pluginPresenter/index.tssrc/main/presenter/sqlitePresenter/index.tssrc/main/presenter/sqlitePresenter/tables/deepchatMessages.tssrc/main/presenter/usageStats.tssrc/renderer/settings/components/ModelProviderSettings.vuesrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/icons/ModelIcon.vuesrc/renderer/src/components/markdown/MarkdownRenderer.vuesrc/renderer/src/components/think-content/ThinkContent.vuesrc/renderer/src/main.tssrc/renderer/src/pages/ChatPage.vuesrc/renderer/src/stores/modelStore.tssrc/renderer/src/stores/ui/message.tssrc/renderer/src/stores/ui/session.tssrc/shared/contracts/events/mcp.events.tssrc/shared/types/agent-interface.d.tssrc/shared/types/core/mcp.tssrc/shared/types/index.d.tstest/e2e/helpers/settings.tstest/e2e/specs/04-settings-navigation.smoke.spec.tstest/main/presenter/configPresenter/fontSizeSettings.test.tstest/renderer/components/ChatPage.test.tstest/renderer/components/ChatStatusBar.test.tstest/renderer/components/MarkdownRenderer.test.tstest/renderer/components/ModelIcon.test.tstest/renderer/components/ModelProviderSettings.test.tstest/renderer/components/ThinkContent.test.tstest/renderer/shell/mainMarkdownWorkers.test.tstest/renderer/stores/messageStore.test.tstest/renderer/stores/modelStore.test.tstest/renderer/stores/sessionStore.test.ts
💤 Files with no reviewable changes (7)
- docs/issues/cron-scheduler-utility-start-crash/spec.md
- docs/issues/cron-scheduler-stop-exit-error/spec.md
- docs/issues/cron-job-run-insert-values/spec.md
- docs/issues/cron-scheduler-heartbeat-status/spec.md
- docs/features/cron-jobs-timezone-selector/spec.md
- src/renderer/src/main.ts
- docs/issues/scheduled-ui-label-layout/spec.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/src/components/icons/AgentAvatar.vue (1)
78-90: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd error handling around
ensureIconAvailable.If
ensureIconAvailablerejects (e.g., network/registry failure),lucideIconVersionnever increments and the icon silently stays unrendered with no fallback or retry. Wrap the await in a try/catch so failures degrade gracefully.🔧 Proposed fix
watch( () => (props.agent.avatar?.kind === 'lucide' ? props.agent.avatar.icon.trim() : ''), async (iconName) => { if (!iconName) { return } - await ensureIconAvailable(`lucide:${iconName}`) - lucideIconVersion.value += 1 + try { + await ensureIconAvailable(`lucide:${iconName}`) + } catch (error) { + console.error(`Failed to load lucide icon "${iconName}"`, error) + } finally { + lucideIconVersion.value += 1 + } }, { immediate: true } )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/icons/AgentAvatar.vue` around lines 78 - 90, The watcher in AgentAvatar.vue does not handle failures from ensureIconAvailable, so a rejected icon fetch leaves lucideIconVersion unchanged and the avatar blank. Update the watch callback for the lucide icon name to wrap the await ensureIconAvailable(...) call in try/catch, and on failure degrade gracefully without throwing from the watcher. Keep the existing immediate behavior and still increment lucideIconVersion only when the icon load succeeds, using the same watch block and lucideIconVersion ref as the fix point.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/generate-icon-collections.mjs`:
- Around line 82-102: The createSubsetCollection() helper currently copies
matching entries from collection.aliases without also preserving the alias
chain’s parent icon, which can leave a dangling alias in the subset. Update
createSubsetCollection() so that whenever an alias is included, it also
recursively or iteratively adds its parent icon(s) from collection.aliases until
a real icon from collection.icons is reached, ensuring the subset contains the
full alias chain. Keep the fix localized to createSubsetCollection() and use the
existing icons and aliases maps to avoid duplicate or missing entries.
---
Nitpick comments:
In `@src/renderer/src/components/icons/AgentAvatar.vue`:
- Around line 78-90: The watcher in AgentAvatar.vue does not handle failures
from ensureIconAvailable, so a rejected icon fetch leaves lucideIconVersion
unchanged and the avatar blank. Update the watch callback for the lucide icon
name to wrap the await ensureIconAvailable(...) call in try/catch, and on
failure degrade gracefully without throwing from the watcher. Keep the existing
immediate behavior and still increment lucideIconVersion only when the icon load
succeeds, using the same watch block and lucideIconVersion ref as the fix point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f307ccd1-b2fd-4129-9435-371fb7d63e05
📒 Files selected for processing (39)
docs/architecture/perf-fixes-from-audit/fix-F03-backfill-coordinator.mddocs/architecture/perf-fixes-from-audit/fix-F05-mcp-startup-timeout.mddocs/architecture/perf-fixes-from-audit/fix-F08-shutdown-observability.mddocs/architecture/perf-fixes-from-audit/fix-F09-markdown-workers-lazy.mddocs/architecture/perf-fixes-from-audit/fix-F11-icons-bundle-slim.mddocs/architecture/perf-fixes-from-audit/fix-F12-session-fetch-dedup.mddocs/architecture/perf-fixes-from-audit/fix-F13-message-store-sort.mddocs/architecture/perf-fixes-from-audit/fix-F16-provider-list-render.mddocs/architecture/perf-fixes-from-audit/fix-V1-settings-hidden-route.mddocs/architecture/perf-fixes-from-audit/spec.mdpackage.jsonresources/model-db/providers.jsonscripts/generate-icon-collections.mjssrc/main/presenter/agentSessionPresenter/index.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/index.tssrc/main/presenter/lifecyclePresenter/hooks/after-start/rtkHealthCheckHook.tssrc/main/presenter/lifecyclePresenter/hooks/beforeStart/protocolRegistrationHook.tssrc/main/presenter/mcpPresenter/index.tssrc/main/presenter/mcpPresenter/mcpClient.tssrc/main/presenter/mcpPresenter/serverManager.tssrc/main/presenter/pluginPresenter/index.tssrc/renderer/settings/components/DataSettings.vuesrc/renderer/src/components/icons/AgentAvatar.vuesrc/renderer/src/components/icons/ModelIcon.vuesrc/renderer/src/components/icons/modelIconRegistry.tssrc/renderer/src/lib/iconLoader.tssrc/renderer/src/lib/icons/icon-collections.generated.tssrc/renderer/src/lib/icons/icon-whitelist.generated.tssrc/renderer/src/stores/ui/message.tssrc/shared/contracts/events/mcp.events.tssrc/shared/types/presenters/core.presenter.d.tstest/e2e/helpers/settings.tstest/e2e/specs/11-remote-control-readonly-route.smoke.spec.tstest/e2e/specs/13-mcp-readonly-route.smoke.spec.tstest/e2e/specs/16-skills-readonly-route.smoke.spec.tstest/e2e/specs/19-skill-sync-readonly-route.smoke.spec.tstest/main/presenter/mcpPresenter.test.tstest/renderer/components/ChatStatusBar.test.ts
💤 Files with no reviewable changes (1)
- src/main/presenter/index.ts
✅ Files skipped from review due to trivial changes (11)
- src/renderer/settings/components/DataSettings.vue
- src/renderer/src/lib/icons/icon-whitelist.generated.ts
- docs/architecture/perf-fixes-from-audit/fix-V1-settings-hidden-route.md
- docs/architecture/perf-fixes-from-audit/fix-F09-markdown-workers-lazy.md
- docs/architecture/perf-fixes-from-audit/spec.md
- docs/architecture/perf-fixes-from-audit/fix-F03-backfill-coordinator.md
- docs/architecture/perf-fixes-from-audit/fix-F05-mcp-startup-timeout.md
- docs/architecture/perf-fixes-from-audit/fix-F16-provider-list-render.md
- docs/architecture/perf-fixes-from-audit/fix-F12-session-fetch-dedup.md
- src/renderer/src/lib/icons/icon-collections.generated.ts
- docs/architecture/perf-fixes-from-audit/fix-F11-icons-bundle-slim.md
🚧 Files skipped from review as they are similar to previous changes (12)
- src/main/presenter/lifecyclePresenter/hooks/after-start/rtkHealthCheckHook.ts
- test/renderer/components/ChatStatusBar.test.ts
- test/e2e/helpers/settings.ts
- docs/architecture/perf-fixes-from-audit/fix-F08-shutdown-observability.md
- src/main/presenter/pluginPresenter/index.ts
- src/shared/contracts/events/mcp.events.ts
- src/main/presenter/lifecyclePresenter/hooks/beforeStart/protocolRegistrationHook.ts
- src/main/presenter/configPresenter/index.ts
- src/renderer/src/stores/ui/message.ts
- src/main/presenter/mcpPresenter/mcpClient.ts
- resources/model-db/providers.json
- src/main/presenter/agentSessionPresenter/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/renderer/components/AgentAvatar.test.ts (1)
1-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest path doesn't mirror source directory.
AgentAvatar.vuelives atsrc/renderer/src/components/icons/AgentAvatar.vue, but this test is attest/renderer/components/AgentAvatar.test.ts, missing theiconssegment.As per path instructions, "Place Vitest renderer tests under
test/renderer/to mirror source structure."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/renderer/components/AgentAvatar.test.ts` around lines 1 - 55, The test location does not mirror the source structure for AgentAvatar, so move the AgentAvatar test to the matching renderer path that includes the icons segment. Keep the existing assertions and mocking around AgentAvatar.vue, but place this spec under the test/renderer hierarchy in a way that reflects src/renderer/src/components/icons/AgentAvatar.vue so future tests stay aligned with the component path.Source: Path instructions
src/main/presenter/agentRuntimePresenter/echo.ts (1)
25-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the duplicated
chat.stream.updatedpayload builder.The exact same payload-construction block (
kind,requestId,sessionId,messageId,providerId,modelId,updatedAt,blocks) is duplicated here and indispatch.ts'sflushBlocksToRenderer. This PR had to update both copies identically to addproviderId/modelId, which is a sign a shared helper would reduce future drift risk.♻️ Suggested shared helper
+function buildStreamUpdatedPayload(io: Pick<IoParams, 'requestId' | 'sessionId' | 'messageId' | 'providerId' | 'modelId'>, blocks: AssistantMessageBlock[]) { + return { + kind: 'snapshot' as const, + requestId: io.requestId, + sessionId: io.sessionId, + messageId: io.messageId, + providerId: io.providerId, + modelId: io.modelId, + updatedAt: Date.now(), + blocks: cloneBlocksForRenderer(blocks) + } +}Then call
publishDeepchatEvent('chat.stream.updated', buildStreamUpdatedPayload(io, state.blocks))from bothecho.tsanddispatch.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/agentRuntimePresenter/echo.ts` around lines 25 - 38, The `chat.stream.updated` payload construction is duplicated between `startEcho` in `echo.ts` and `flushBlocksToRenderer` in `dispatch.ts`, so extract it into a shared helper to prevent future drift. Add a reusable payload builder that takes the common `io` fields and `blocks`, includes `kind`, `requestId`, `sessionId`, `messageId`, `providerId`, `modelId`, `updatedAt`, and `blocks`, then call it from both places. Keep the existing `publishDeepchatEvent('chat.stream.updated', ...)` usage but replace the inline object with the helper result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/src/stores/ui/session.ts`:
- Around line 494-504: The stale hydration flow in hydrateActiveSessionSummary
can overwrite the current session summary and cause wrong navigation because it
only validates the fetched active session against the requested sessionId.
Update hydrateActiveSessionSummary, and the selectSession and onActivated call
sites, to re-check activeSessionId.value === sessionId immediately before
calling applyRestoredSession or pageRouter.goToChat, matching the guard pattern
already used in setSessionModel, moveSessionToAgent, and
setSessionSubagentEnabled.
---
Nitpick comments:
In `@src/main/presenter/agentRuntimePresenter/echo.ts`:
- Around line 25-38: The `chat.stream.updated` payload construction is
duplicated between `startEcho` in `echo.ts` and `flushBlocksToRenderer` in
`dispatch.ts`, so extract it into a shared helper to prevent future drift. Add a
reusable payload builder that takes the common `io` fields and `blocks`,
includes `kind`, `requestId`, `sessionId`, `messageId`, `providerId`, `modelId`,
`updatedAt`, and `blocks`, then call it from both places. Keep the existing
`publishDeepchatEvent('chat.stream.updated', ...)` usage but replace the inline
object with the helper result.
In `@test/renderer/components/AgentAvatar.test.ts`:
- Around line 1-55: The test location does not mirror the source structure for
AgentAvatar, so move the AgentAvatar test to the matching renderer path that
includes the icons segment. Keep the existing assertions and mocking around
AgentAvatar.vue, but place this spec under the test/renderer hierarchy in a way
that reflects src/renderer/src/components/icons/AgentAvatar.vue so future tests
stay aligned with the component path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a32218eb-58d1-481c-8a6b-7fa3162cf5e1
📒 Files selected for processing (21)
docs/architecture/perf-fixes-from-audit/fix-F13-message-store-sort.mddocs/architecture/perf-fixes-from-audit/fix-F16-provider-list-render.mddocs/issues/acp-stream-assistant-name/spec.mdscripts/generate-icon-collections.mjssrc/main/presenter/agentRuntimePresenter/dispatch.tssrc/main/presenter/agentRuntimePresenter/echo.tssrc/main/presenter/agentRuntimePresenter/index.tssrc/main/presenter/agentRuntimePresenter/types.tssrc/renderer/src/components/icons/AgentAvatar.vuesrc/renderer/src/lib/icons/icon-collections.generated.tssrc/renderer/src/stores/ui/message.tssrc/renderer/src/stores/ui/messageIpc.tssrc/renderer/src/stores/ui/session.tssrc/renderer/src/stores/ui/sessionIpc.tssrc/shared/contracts/events/chat.events.tstest/main/presenter/agentRuntimePresenter/dispatch.test.tstest/main/presenter/agentRuntimePresenter/echo.test.tstest/main/routes/contracts.test.tstest/renderer/components/AgentAvatar.test.tstest/renderer/stores/messageStore.test.tstest/renderer/stores/sessionStore.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/renderer/src/lib/icons/icon-collections.generated.ts
- docs/architecture/perf-fixes-from-audit/fix-F16-provider-list-render.md
- docs/architecture/perf-fixes-from-audit/fix-F13-message-store-sort.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/renderer/src/components/icons/AgentAvatar.vue
- test/renderer/stores/messageStore.test.ts
- scripts/generate-icon-collections.mjs
Comment on lines
+494
to
+504
| const hydrateActiveSessionSummary = async (sessionId: string): Promise<void> => { | ||
| try { | ||
| const active = await sessionClient.getActive() | ||
| if (active.session?.id === sessionId) { | ||
| applyRestoredSession(active.session) | ||
| } | ||
| } catch (restoreError) { | ||
| console.warn('[sessionStore] Failed to hydrate selected session:', restoreError) | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Race: stale session hydration can clobber active summary and mis-navigate.
hydrateActiveSessionSummary only checks the response matches the requested sessionId, not that activeSessionId.value is still sessionId when the awaited getActive() resolves. Since applyRestoredSession unconditionally sets activeSessionSummary.value, this diverges from the pattern used elsewhere in this file (setSessionModel, moveSessionToAgent, setSessionSubagentEnabled all gate on activeSessionId.value === sessionId before calling applyRestoredSession).
Both callers (selectSession line 686, onActivated line 1006) then unconditionally call pageRouter.goToChat(sessionId) after the await, using the stale closure value. If two activations race (rapid session switching, or a cross-window activated IPC event racing a local selectSession), the slower one can overwrite the summary/navigate to the wrong session after a newer one has already taken over.
🔒 Proposed fix: re-check current session before applying/navigating
const hydrateActiveSessionSummary = async (sessionId: string): Promise<void> => {
try {
const active = await sessionClient.getActive()
- if (active.session?.id === sessionId) {
+ if (activeSessionId.value === sessionId && active.session?.id === sessionId) {
applyRestoredSession(active.session)
}
} catch (restoreError) {
console.warn('[sessionStore] Failed to hydrate selected session:', restoreError)
}
} await hydrateActiveSessionSummary(sessionId)
- pageRouter.goToChat(sessionId)
+ if (activeSessionId.value === sessionId) {
+ pageRouter.goToChat(sessionId)
+ }(apply the second diff at both the selectSession and onActivated call sites)
Also applies to: 676-686, 996-1008
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/stores/ui/session.ts` around lines 494 - 504, The stale
hydration flow in hydrateActiveSessionSummary can overwrite the current session
summary and cause wrong navigation because it only validates the fetched active
session against the requested sessionId. Update hydrateActiveSessionSummary, and
the selectSession and onActivated call sites, to re-check activeSessionId.value
=== sessionId immediately before calling applyRestoredSession or
pageRouter.goToChat, matching the guard pattern already used in setSessionModel,
moveSessionToAgent, and setSessionSubagentEnabled.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/architecture/perf-fixes-from-audit/fix-F01-sqlite-startup-defer.md (1)
68-81: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNumbering skips "3." in "步骤拆分" list.
Item 2 (去掉重复观察性诊断) is immediately followed by item "4." (非关键表延迟建表列为可选后续项), with no "3." — likely a leftover from removing/reordering a step.
📝 Proposed fix
-4. **非关键表延迟建表列为可选后续项** +3. **非关键表延迟建表列为可选后续项**🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/perf-fixes-from-audit/fix-F01-sqlite-startup-defer.md` around lines 68 - 81, The “步骤拆分” list has a numbering gap because item 2 is followed by item 4, so update the markdown numbering to be sequential. Fix the list in this document by renumbering the current “4.” entry to “3.” and ensure any other step labels in the same section stay consistent with the ordered flow around SQLitePresenter.initializeDatabase() and DatabaseInitializer.initialize()/diagnoseStartupSchema().src/main/presenter/sqlitePresenter/tables/deepchatMessages.ts (1)
259-318: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAdd an index for assistant-usage lookups in
src/main/presenter/sqlitePresenter/tables/deepchatMessages.ts
iterAssistantUsageCandidates()andlistAssistantUsageCandidatesPage()filter onm.role = 'assistant'and order bym.created_at/m.id, butdeepchat_messagesonly hasidx_deepchat_messages_session(session_id, order_seq). That leaves these backfill reads doing a scan/sort on every page fetch, so add a composite index on(role, created_at, id)and a migration for existing databases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/sqlitePresenter/tables/deepchatMessages.ts` around lines 259 - 318, The assistant-usage queries in DeepChatMessageTable are scanning and sorting because `iterAssistantUsageCandidates()` and `listAssistantUsageCandidatesPage()` filter by `m.role = 'assistant'` and order by `m.created_at`/`m.id` without a supporting index. Add a composite index on `deepchat_messages` for `(role, created_at, id)` and wire it into the existing schema/migration path so current databases get it too. Make sure the index supports both the iterator and paged lookup in `deepchatMessages.ts` without changing their query logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/main/presenter/mcpPresenter/serverManager.test.ts`:
- Around line 141-146: Avoid eagerly creating the rejected connection-completion
promise in serverManager.test.ts: the current getConnectionCompletion mock uses
Promise.reject(...) at setup time, which can trigger an unhandled rejection
before startServer runs. Update the mock on clientMocks.getConnectionCompletion
to use a deferred rejection pattern consistent with mockRejectedValueOnce
elsewhere, and keep the test flow around manager.startServer('regular')
unchanged.
---
Outside diff comments:
In `@docs/architecture/perf-fixes-from-audit/fix-F01-sqlite-startup-defer.md`:
- Around line 68-81: The “步骤拆分” list has a numbering gap because item 2 is
followed by item 4, so update the markdown numbering to be sequential. Fix the
list in this document by renumbering the current “4.” entry to “3.” and ensure
any other step labels in the same section stay consistent with the ordered flow
around SQLitePresenter.initializeDatabase() and
DatabaseInitializer.initialize()/diagnoseStartupSchema().
In `@src/main/presenter/sqlitePresenter/tables/deepchatMessages.ts`:
- Around line 259-318: The assistant-usage queries in DeepChatMessageTable are
scanning and sorting because `iterAssistantUsageCandidates()` and
`listAssistantUsageCandidatesPage()` filter by `m.role = 'assistant'` and order
by `m.created_at`/`m.id` without a supporting index. Add a composite index on
`deepchat_messages` for `(role, created_at, id)` and wire it into the existing
schema/migration path so current databases get it too. Make sure the index
supports both the iterator and paged lookup in `deepchatMessages.ts` without
changing their query logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88f57ae6-e265-4a34-a67c-2e64f88b76d1
📒 Files selected for processing (35)
docs/architecture/perf-audit-review-hardening/plan.mddocs/architecture/perf-audit-review-hardening/spec.mddocs/architecture/perf-audit-review-hardening/tasks.mddocs/architecture/perf-fixes-from-audit/fix-F01-sqlite-startup-defer.mddocs/architecture/perf-fixes-from-audit/fix-F03-backfill-coordinator.mddocs/architecture/perf-fixes-from-audit/plan.mddocs/architecture/perf-fixes-from-audit/tasks.mdpackage.jsonscripts/generate-icon-collections.mjssrc/main/presenter/agentSessionPresenter/index.tssrc/main/presenter/lifecyclePresenter/DatabaseInitializer.tssrc/main/presenter/lifecyclePresenter/hooks/beforeStart/protocolRegistrationHook.tssrc/main/presenter/lifecyclePresenter/hooks/init/databaseInitHook.tssrc/main/presenter/mcpPresenter/index.tssrc/main/presenter/mcpPresenter/mcpClient.tssrc/main/presenter/mcpPresenter/serverManager.tssrc/main/presenter/pluginPresenter/index.tssrc/main/presenter/sqlitePresenter/tables/deepchatMessages.tssrc/renderer/src/lib/icons/icon-collections.generated.tssrc/renderer/src/lib/icons/icon-whitelist.generated.tssrc/renderer/src/pages/ChatPage.vuesrc/renderer/src/stores/ui/session.tssrc/shared/contracts/domainSchemas.tssrc/shared/contracts/events/mcp.events.tstest/main/contracts/zodV4Migration.test.tstest/main/presenter/agentSessionPresenter/usageDashboard.test.tstest/main/presenter/lifecyclePresenter/DatabaseInitializer.test.tstest/main/presenter/mcpClient.test.tstest/main/presenter/mcpPresenter.test.tstest/main/presenter/mcpPresenter/serverManager.test.tstest/main/presenter/pluginPresenter.test.tstest/renderer/components/ChatPage.test.tstest/renderer/lib/iconWhitelist.test.tstest/renderer/shell/mainMarkdownWorkers.test.tstest/renderer/stores/sessionStore.test.ts
💤 Files with no reviewable changes (2)
- src/main/presenter/lifecyclePresenter/hooks/beforeStart/protocolRegistrationHook.ts
- src/main/presenter/lifecyclePresenter/DatabaseInitializer.ts
✅ Files skipped from review due to trivial changes (9)
- test/renderer/lib/iconWhitelist.test.ts
- docs/architecture/perf-audit-review-hardening/tasks.md
- docs/architecture/perf-audit-review-hardening/spec.md
- test/main/presenter/lifecyclePresenter/DatabaseInitializer.test.ts
- src/main/presenter/lifecyclePresenter/hooks/init/databaseInitHook.ts
- src/renderer/src/lib/icons/icon-whitelist.generated.ts
- src/renderer/src/lib/icons/icon-collections.generated.ts
- docs/architecture/perf-fixes-from-audit/tasks.md
- docs/architecture/perf-fixes-from-audit/plan.md
🚧 Files skipped from review as they are similar to previous changes (14)
- package.json
- test/renderer/shell/mainMarkdownWorkers.test.ts
- test/renderer/components/ChatPage.test.ts
- src/shared/contracts/events/mcp.events.ts
- src/renderer/src/stores/ui/session.ts
- src/main/presenter/mcpPresenter/index.ts
- src/main/presenter/mcpPresenter/serverManager.ts
- src/main/presenter/pluginPresenter/index.ts
- src/renderer/src/pages/ChatPage.vue
- scripts/generate-icon-collections.mjs
- test/renderer/stores/sessionStore.test.ts
- test/main/presenter/mcpPresenter.test.ts
- src/main/presenter/mcpPresenter/mcpClient.ts
- src/main/presenter/agentSessionPresenter/index.ts
Comment on lines
+141
to
+146
| clientMocks.getConnectionCompletion.mockReturnValueOnce( | ||
| Promise.reject(new McpConnectionCancelledError('regular')) | ||
| ) | ||
|
|
||
| await expect(manager.startServer('regular')).resolves.toBe('soft-timeout-released') | ||
| await Promise.resolve() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid eagerly constructing the rejected Promise.
Promise.reject(...) executes immediately when this line runs (at mock setup, before startServer is even called), risking an unhandled-rejection warning if the implementation doesn't attach a handler in the same microtask tick. Use a deferred factory instead, consistent with mockRejectedValueOnce used elsewhere in this PR (e.g. pluginPresenter.test.ts).
🛠️ Proposed fix
- clientMocks.getConnectionCompletion.mockReturnValueOnce(
- Promise.reject(new McpConnectionCancelledError('regular'))
- )
+ clientMocks.getConnectionCompletion.mockImplementationOnce(() =>
+ Promise.reject(new McpConnectionCancelledError('regular'))
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| clientMocks.getConnectionCompletion.mockReturnValueOnce( | |
| Promise.reject(new McpConnectionCancelledError('regular')) | |
| ) | |
| await expect(manager.startServer('regular')).resolves.toBe('soft-timeout-released') | |
| await Promise.resolve() | |
| clientMocks.getConnectionCompletion.mockImplementationOnce(() => | |
| Promise.reject(new McpConnectionCancelledError('regular')) | |
| ) | |
| await expect(manager.startServer('regular')).resolves.toBe('soft-timeout-released') | |
| await Promise.resolve() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/main/presenter/mcpPresenter/serverManager.test.ts` around lines 141 -
146, Avoid eagerly creating the rejected connection-completion promise in
serverManager.test.ts: the current getConnectionCompletion mock uses
Promise.reject(...) at setup time, which can trigger an unhandled rejection
before startServer runs. Update the mock on clientMocks.getConnectionCompletion
to use a deferred rejection pattern consistent with mockRejectedValueOnce
elsewhere, and keep the test flow around manager.startServer('regular')
unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Summary by CodeRabbit
New Features
Bug Fixes