feat(agent): unify deepchat and acp agents#1384
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a unified agent management system: SQLite-backed persistence, an AgentRepository, ConfigPresenter agent APIs/migration, renderer UIs for managing DeepChat agents, session-scoped DeepChat config resolution (used by compaction and session creation), IPC readiness/queuing for settings window, and extensive i18n and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as DeepChatAgentsSettings.vue
participant Config as ConfigPresenter
participant Repo as AgentRepository
participant Table as AgentsTable
participant DB as SQLite
User->>UI: Edit agent & Save
UI->>Config: updateDeepChatAgent(agentId, updates)
Config->>Repo: updateDeepChatAgent(agentId, updates)
Repo->>Repo: merge & serialize config
Repo->>Table: update(agentId, input)
Table->>DB: UPDATE agents ...
DB-->>Table: OK
Table-->>Repo: done
Repo-->>Config: updated Agent
Config->>Config: emit CONFIG_EVENTS.AGENTS_CHANGED
Config-->>UI: return updated Agent
sequenceDiagram
participant NewThread as NewThreadPage.vue
participant Presenter as NewAgentPresenter.createSession
participant Config as ConfigPresenter
participant Repo as AgentRepository
participant DeepChat as DeepChatAgentPresenter
NewThread->>Presenter: createSession(input)
Presenter->>Config: getAgentType(agentId)
Config->>Repo: getAgentType(agentId)
Repo-->>Config: 'deepchat' | 'acp'
Config-->>Presenter: agent type
alt deepchat
Presenter->>Config: resolveDeepChatAgentConfig(agentId)
Config->>Repo: resolveDeepChatAgentConfig(agentId)
Repo-->>Config: merged agent config
Config-->>Presenter: DeepChatAgentConfig
Presenter->>Presenter: apply agent defaults (projectDir, models, tools)
else acp
Presenter->>Presenter: apply ACP-specific defaults
end
Presenter->>DeepChat: initSession(sessionId, config)
DeepChat-->>Presenter: ok
Presenter-->>NewThread: sessionId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7f4c5c8a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/presenter/deepchatAgentPresenter/index.ts (1)
395-405:⚠️ Potential issue | 🟠 MajorDon't let compaction planning drop the prompt.
prepareForNextUserTurn()is now awaited before any user message is created. If agent-config resolution or another compaction-prep step rejects, the send fails and the submitted prompt never lands in history. Falling back to “no compaction” here keeps the send path resilient.💡 Suggested guard
- const compactionIntent = await this.compactionService.prepareForNextUserTurn({ - sessionId, - providerId: state.providerId, - modelId: state.modelId, - systemPrompt: baseSystemPrompt, - contextLength: generationSettings.contextLength, - reserveTokens: maxTokens, - supportsVision, - preserveInterleavedReasoning: interleavedReasoning.preserveReasoningContent, - newUserContent: normalizedInput - }) + let compactionIntent: CompactionIntent | null = null + try { + compactionIntent = await this.compactionService.prepareForNextUserTurn({ + sessionId, + providerId: state.providerId, + modelId: state.modelId, + systemPrompt: baseSystemPrompt, + contextLength: generationSettings.contextLength, + reserveTokens: maxTokens, + supportsVision, + preserveInterleavedReasoning: interleavedReasoning.preserveReasoningContent, + newUserContent: normalizedInput + }) + } catch (error) { + console.warn( + '[DeepChatAgent] Failed to prepare compaction for next turn; continuing without compaction.', + error + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/deepchatAgentPresenter/index.ts` around lines 395 - 405, The current await of compactionService.prepareForNextUserTurn (assigning compactionIntent) can throw and abort the send before the user message and history are created; wrap the prepareForNextUserTurn call in a try/catch and on any error log it and set compactionIntent to a safe "no compaction" fallback (e.g., null or an explicit no-op intent) so the subsequent code that uses normalizedInput, interleavedReasoning.preserveReasoningContent, sessionId, providerId, and modelId still proceeds to create and persist the user message; ensure you still call the existing send path with the fallback compactionIntent rather than returning or throwing.src/renderer/src/components/chat/ChatStatusBar.vue (1)
2222-2235:⚠️ Potential issue | 🟠 MajorSwitching DeepChat agents can leave the previous agent's defaults loaded.
syncGenerationSettings()now depends on the selected DeepChat agent, but this watcher never observes that id. If two agents share the same draft model, changing agents will not refreshlocalSettings.systemPromptor other per-agent defaults.💡 Proposed fix
watch( [ () => sessionStore.activeSessionId, () => sessionStore.activeSession?.providerId, () => sessionStore.activeSession?.modelId, () => draftModelSelection.value?.providerId, () => draftModelSelection.value?.modelId, - () => isAcpAgent.value + () => isAcpAgent.value, + () => selectedDeepChatAgentId.value ], () => { void syncGenerationSettings() },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/chat/ChatStatusBar.vue` around lines 2222 - 2235, The watcher for syncGenerationSettings is missing the DeepChat agent identifier, so switching agents can leave per-agent defaults (like localSettings.systemPrompt) stale; update the watched sources to include the agent id (e.g., sessionStore.activeSession?.agentId or whichever property holds the DeepChat agent) alongside the existing keys (sessionStore.activeSessionId, providerId, modelId, draftModelSelection, isAcpAgent) so that syncGenerationSettings() runs when the active agent changes; locate the watcher block around the call to syncGenerationSettings and add the agent id accessor to the dependency array.src/main/presenter/newAgentPresenter/index.ts (1)
127-156:⚠️ Potential issue | 🔴 CriticalACP sessions still initialize with the default LLM model.
This branch now knows
agentType, but ACP sessions still fall through todefaultModel/DeepChat defaults. That meansproviderIdis usually something likeopenaiinstead of'acp', so the ACP workdir guard is skipped andinitSession()gets the wrong backend/model.💡 Proposed fix
// Resolve provider/model const defaultModel = this.configPresenter.getDefaultModel() - const providerId = - input.providerId ?? - deepChatAgentConfig?.defaultModelPreset?.providerId ?? - defaultModel?.providerId ?? - '' - const modelId = - input.modelId ?? - deepChatAgentConfig?.defaultModelPreset?.modelId ?? - defaultModel?.modelId ?? - '' + const providerId = + agentType === 'acp' + ? 'acp' + : input.providerId ?? + deepChatAgentConfig?.defaultModelPreset?.providerId ?? + defaultModel?.providerId ?? + '' + const modelId = + agentType === 'acp' + ? agentId + : input.modelId ?? + deepChatAgentConfig?.defaultModelPreset?.modelId ?? + defaultModel?.modelId ?? + ''🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/newAgentPresenter/index.ts` around lines 127 - 156, The code resolves providerId/modelId using defaults and deepChatAgentConfig but ignores the agentType, causing ACP agents to inherit the default LLM provider (e.g., "openai") and skip the ACP workdir guard; update the resolution logic in NewAgentPresenter so that when agentType === 'acp' you force providerId = 'acp' and set modelId from deepChatAgentConfig or input specific to ACP (falling back only if appropriate), then call this.assertAcpSessionHasWorkdir(providerId, projectDir) when agentType === 'acp' (or providerId === 'acp') before initSession; adjust the code paths that compute providerId, modelId and the assertAcpSessionHasWorkdir invocation to reference agentType, providerId, modelId, deepChatAgentConfig, getDefaultModel(), and initSession accordingly.
🧹 Nitpick comments (12)
src/renderer/src/i18n/zh-TW/settings.json (1)
1472-1521: Please sync this new agent configuration surface inAGENTS.md.This adds a sizable agent settings contract; documenting it will reduce drift for future agent changes.
Based on learnings: Document agent implementations and configurations in AGENTS.md.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/zh-TW/settings.json` around lines 1472 - 1521, Add the new DeepChat agent configuration surface (the "deepchatAgents" object and its keys such as builtIn, name/namePlaceholder, avatarTitle/avatarLucide/avatarMonogram and their Descs, modelsTitle with chatModel/assistantModel/visionModel, temperature/contextLength/maxTokens/thinkingBudget/reasoningEffort/verbosity/interleaved, systemPrompt/systemPromptPlaceholder, defaultProjectPath/defaultProjectPathPlaceholder, permissionMode/permissionFullAccess/permissionDefault, toolsTitle, compactionTitle/compactionEnabled/compactionDescription/compactionThreshold/compactionRetainPairs, and deleteConfirm) to AGENTS.md; document each field’s purpose, data type/expected values (e.g., numeric vs boolean vs string), defaults/placeholder behavior, and any UX implications (avatar options, permission modes, compaction effects) so the agent configuration contract in AGENTS.md matches the new settings.json surface.src/main/presenter/index.ts (1)
131-136: Type assertion could be simplified with interface extension.The double cast through
unknownand inline type extension work but are verbose. Consider extendingIConfigPresenterinterface to includesetAgentRepositoryif this dependency injection pattern will be reused.// Current approach works but is verbose: ;(this.configPresenter as IConfigPresenter & { setAgentRepository?: (repository: AgentRepository) => void }).setAgentRepository?.(agentRepository)This is acceptable for now given the optional nature of the setter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/index.ts` around lines 131 - 136, The code is using a verbose double type assertion to call a setter on this.configPresenter; instead extend IConfigPresenter to include an optional setAgentRepository?(repository: AgentRepository): void and then remove the casts by typing this.configPresenter as IConfigPresenter (or ensure the concrete presenter implements the extended interface), then call this.configPresenter.setAgentRepository?.(agentRepository) directly; reference the symbols AgentRepository, SQLitePresenter, IConfigPresenter, setAgentRepository and configPresenter when making the change.src/renderer/settings/main.ts (1)
72-103: Consider normalizing route position values.The route positions use fractional values (2.5, 3.5, 4.5) which work but may become harder to manage as more routes are added. Consider renumbering with integer increments in a future refactor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/main.ts` around lines 72 - 103, The route entries (e.g., routes with name 'settings-mcp', 'settings-deepchat-agents', 'settings-acp') use fractional meta.position values (2.5, 3.5, 4.5); update these meta.position fields to a normalized integer sequence (e.g., 2, 3, 4 or another consistent integer ordering) across all route definitions to avoid gaps and make future inserts simpler, ensuring each route's meta.position is unique and ordered consistently.src/renderer/src/i18n/en-US/settings.json (1)
115-115: Prefer full wording over “compat” in user-facing copy.Using the full term improves clarity in settings UI text.
✏️ Suggested copy tweak
- "interleaved": "Force interleaved thinking compat", + "interleaved": "Force interleaved thinking compatibility",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/en-US/settings.json` at line 115, The key "interleaved" in settings UI copy uses the abbreviation "compat"; update the value to use the full wording for clarity—replace "Force interleaved thinking compat" with something like "Force interleaved thinking compatibility" or "Force interleaved thinking compatibility mode" in the "interleaved" entry of src/renderer/src/i18n/en-US/settings.json so the user-facing setting uses the full term.test/renderer/components/AgentWelcomePage.test.ts (1)
1-8: Consider relocating test file to mirror source structure.Per coding guidelines, test files should mirror source structure. The source is at
src/renderer/src/pages/AgentWelcomePage.vue, but this test is attest/renderer/components/. Consider moving totest/renderer/pages/AgentWelcomePage.test.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/AgentWelcomePage.test.ts` around lines 1 - 8, Test file location does not mirror the source structure; move the test for AgentWelcomePage to follow the same directory layout as the source so it's discoverable and consistent. Rename/move test/renderer/components/AgentWelcomePage.test.ts to test/renderer/pages/AgentWelcomePage.test.ts and update any import paths referencing AgentWelcomePage (e.g., the component import used in the test and any relative paths to '@/events' or other helpers) so the test still mounts the AgentWelcomePage.vue and uses SETTINGS_EVENTS correctly.src/renderer/src/pages/AgentWelcomePage.vue (1)
68-79: Consider adding a comment explaining the navigation retry.The 250ms delayed retry for
navigateToSettingsappears to be a workaround for timing issues with the settings window initialization. A brief comment would help future maintainers understand the intent.📝 Suggested documentation
const openAgentSettings = async () => { await windowPresenter.createSettingsWindow() const settingsWindowId = windowPresenter.getSettingsWindowId() if (settingsWindowId != null) { navigateToSettings(settingsWindowId) + // Retry navigation after a short delay in case the settings window + // hasn't fully initialized its router yet window.setTimeout(() => { if (windowPresenter.getSettingsWindowId() === settingsWindowId) { navigateToSettings(settingsWindowId) } }, 250) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/pages/AgentWelcomePage.vue` around lines 68 - 79, The retry call in openAgentSettings that runs navigateToSettings after 250ms is a timing-workaround; add an inline comment above the setTimeout explaining that createSettingsWindow may not immediately register the settings window ID (race with window initialization), so we re-check windowPresenter.getSettingsWindowId() and call navigateToSettings again to ensure the new window is focused/selected; reference navigateToSettings, windowPresenter.createSettingsWindow and windowPresenter.getSettingsWindowId in the comment so future readers understand the intent.test/renderer/components/DeepChatAgentsSettings.test.ts (1)
1-4: Move this suite under the mirrored settings test path.The source lives at
src/renderer/settings/components/DeepChatAgentsSettings.vue, so this test should mirror that undertest/renderer/settings/components/.As per coding guidelines "locate test files mirroring source structure under
test/main/**andtest/renderer/**; name files as*.test.tsor*.spec.ts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/DeepChatAgentsSettings.test.ts` around lines 1 - 4, The test file DeepChatAgentsSettings.test.ts is in the wrong directory; move it to mirror the source component path under test/renderer/settings/components/ so it sits alongside the source-style path for src/renderer/settings/components/DeepChatAgentsSettings.vue. After moving, update any relative imports in the test (e.g., the component import or helper paths used by the test suite) to reflect the new location and ensure the test filename remains DeepChatAgentsSettings.test.ts.src/main/presenter/configPresenter/index.ts (3)
464-473: Silent failure when repository is unavailable.
updateBuiltinDeepChatConfigsilently returns whenagentRepositoryis null, which could lead to lost configuration updates without any indication to the caller.💡 Consider logging a warning
private updateBuiltinDeepChatConfig(updates: Partial<DeepChatAgentConfig>): void { if (!this.agentRepository) { + console.warn('[Config] Cannot update DeepChat config: agent repository not initialized') return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/index.ts` around lines 464 - 473, The method updateBuiltinDeepChatConfig currently returns silently if this.agentRepository is falsy causing lost updates; change it to surface the failure by logging a warning or throwing an error: inside updateBuiltinDeepChatConfig check this.agentRepository and if absent call the presenter logger (or console) to warn (including BUILTIN_DEEPCHAT_AGENT_ID and the attempted updates) or throw a descriptive Error, otherwise continue calling this.agentRepository.updateDeepChatAgent(...) and this.notifyAcpAgentsChanged(); reference updateBuiltinDeepChatConfig, this.agentRepository, BUILTIN_DEEPCHAT_AGENT_ID and notifyAcpAgentsChanged when making the change.
1685-1689: Consider handling empty or whitespace-only agentId more explicitly.The
resolveDeepChatAgentConfigmethod usesagentId || BUILTIN_DEEPCHAT_AGENT_ID, which will also match empty strings. This is likely intentional but could benefit from explicit documentation or trimming.async resolveDeepChatAgentConfig(agentId: string): Promise<DeepChatAgentConfig> { return this.getAgentRepositoryOrThrow().resolveDeepChatAgentConfig( agentId?.trim() || BUILTIN_DEEPCHAT_AGENT_ID ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/index.ts` around lines 1685 - 1689, In resolveDeepChatAgentConfig, explicitly handle empty or whitespace-only agentId by trimming and defaulting to BUILTIN_DEEPCHAT_AGENT_ID before calling getAgentRepositoryOrThrow().resolveDeepChatAgentConfig; change the argument expression to use agentId?.trim() || BUILTIN_DEEPCHAT_AGENT_ID so blank strings won’t be treated as valid IDs and the fallback is explicit.
384-386: Consider adding error handling for manual agent migration.If
createManualAcpAgentfails for one agent (e.g., due to a database constraint), the entire migration could leave some agents unmigrated while setting the migration version.💡 Suggested improvement
- this.acpConfHelper.getManualAgents().forEach((agent) => { - repository.createManualAcpAgent(agent) - }) + this.acpConfHelper.getManualAgents().forEach((agent) => { + try { + repository.createManualAcpAgent(agent) + } catch (error) { + console.warn(`[Agents] Failed to migrate manual agent ${agent.id}:`, error) + } + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/configPresenter/index.ts` around lines 384 - 386, The current migration loops this.acpConfHelper.getManualAgents() and calls repository.createManualAcpAgent(agent) without handling failures; wrap each call in a try/catch so a single failing agent doesn’t abort the whole loop, log or collect per-agent errors, and after the loop fail the overall migration (throw or return an error) if any agent failed so the migration version is not set; specifically modify the block using this.acpConfHelper.getManualAgents() and repository.createManualAcpAgent(...) to catch individual errors, continue migrating remaining agents, and surface aggregated errors to prevent advancing the migration state.src/main/presenter/agentRepository/index.ts (2)
223-244: Consider documenting the upsert behavior forcreateManualAcpAgent.The method uses
upsertwhich will update an existing agent if the ID already exists. This is useful for migration but could be surprising for callers expecting pure creation semantics./** * Creates a manual ACP agent, or updates if agent with same ID exists. * This upsert behavior supports migration of existing agents. */ createManualAcpAgent(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentRepository/index.ts` around lines 223 - 244, Add a short doc comment to createManualAcpAgent explaining that it uses an upsert on agentsTable and will update an existing agent if the provided id already exists (so callers may get update semantics rather than pure create), and note this is intentional to support migrations; reference the function name createManualAcpAgent and the use of this.sqlitePresenter.agentsTable.upsert so reviewers can find the behavior to adjust callers if needed.
333-340:setAgentEnabledallows enabling non-ACP agents through ACP-intended code paths.Unlike
setAgentEnvOverrideandsetAgentInstallState, this method doesn't validateagent_type. If this is intentional (to support both ACP and DeepChat agents), consider adding a comment. Otherwise, add a type check for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentRepository/index.ts` around lines 333 - 340, setAgentEnabled currently flips enabled for any agent without validating agent_type, which diverges from setAgentEnvOverride and setAgentInstallState; update setAgentEnabled to fetch the agent row (this.sqlitePresenter.agentsTable.get(agentId)), verify row.agent_type matches the ACP type used elsewhere (e.g., 'acp' or the project constant) and return false if it does not, then proceed to update enabled; alternatively, if enabling non-ACP agents is intentional, add an explicit comment in setAgentEnabled documenting that behavior and why it differs from setAgentEnvOverride/setAgentInstallState.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/deepchatAgentPresenter/compactionService.ts`:
- Around line 401-407: getCompactionSettings currently awaits
resolveSessionConfig and will reject the hot path if session config lookup
fails; make it best-effort by catching errors from resolveSessionConfig and
returning safe defaults (enabled: true, triggerThreshold: 80, retainRecentPairs:
2) instead of propagating the rejection so prepareForNextUserTurn and
prepareForResumeTurn don't block message sends. Specifically, wrap the
resolveSessionConfig call inside getCompactionSettings in a try/catch (or use
.catch) and on error log or swallow the error and return the default
CompactionSettings object; keep the existing default logic when config exists.
Ensure you reference getCompactionSettings and resolveSessionConfig so reviewers
can locate the change.
In `@src/main/presenter/newAgentPresenter/index.ts`:
- Around line 850-857: getAgents currently returns agents regardless of each
agent's enabled flag and only checks the global ACP toggle; update getAgents to
first exclude agents with enabled === false and then apply the existing
ACP/deepchat check (i.e., filter agents where agent.enabled is truthy AND
(agent.type === 'deepchat' || acpEnabled)). Use the existing listAgents() and
getAcpEnabled() calls and modify the final return filter in getAgents to
reference agent.enabled along with agent.type and acpEnabled so disabled agents
are not returned to the renderer.
In `@src/renderer/settings/components/DeepChatAgentsSettings.vue`:
- Around line 830-835: parseNum currently returns any finite number (including
0, negatives, decimals, and out-of-range values) which allows invalid compaction
settings to be persisted; change parseNum to clamp the parsed value to the
intended UI range by accepting or importing the min/max constants used for
compaction (e.g. COMPACTION_MIN, COMPACTION_MAX), parse the value as a Number,
return undefined for non-finite/parsing failures, otherwise clamp with
Math.min(Math.max(parsed, COMPACTION_MIN), COMPACTION_MAX) and (if the UI
expects integers) round with Math.round before returning; apply the same
clamping/rounding logic to the other input parsing instances referenced around
the 1045-1060 region so all persisted compaction/retain values are constrained.
In `@src/renderer/src/components/chat-input/McpIndicator.vue`:
- Around line 289-332: currentAgent can end up with objects that only expose
agentType (not type), causing isDeepchatContext (which checks only .type) to
misclassify DeepChat agents as ACP; update currentAgent and/or resolveAgentType
to normalize type by preferring agent.agentType over agent.type when building
the fallback/selected agent objects and when resolving a matchedAgent (use
matchedAgent?.agentType ?? matchedAgent?.type and same for
agentStore.selectedAgent), and change isDeepchatContext to treat an agent as
DeepChat when (currentAgent.value.type === 'deepchat' ||
currentAgent.value.agentType === 'deepchat') so custom/missing agents that only
expose agentType are correctly recognized.
In `@src/renderer/src/components/chat/ChatStatusBar.vue`:
- Around line 863-879: The inferAgentType fallback incorrectly maps any unknown
non-'deepchat' agentId to 'acp'; update the final return in inferAgentType so it
only returns 'deepchat' when agentId === 'deepchat' and otherwise returns null
(i.e., replace the unconditional "'acp'" fallback), so that when availableAgents
or agentStore.selectedAgent haven't loaded the agent metadata the function
yields null instead of incorrectly classifying agents as 'acp'.
In `@src/renderer/src/i18n/fr-FR/settings.json`:
- Around line 1300-1302: Update the translation value for the "auto" key in the
settings JSON so it reads as automatic selection rather than fallback; replace
"Repli automatique" with a phrase like "Automatique" or "Sélection automatique"
(the "auto" key next to "system" and "builtin") to clearly convey automatic
runtime selection in the picker.
In `@src/renderer/src/i18n/pt-BR/settings.json`:
- Around line 1300-1302: The translation for the "auto" key is incorrect—replace
the value for the "auto" key in src/renderer/src/i18n/pt-BR/settings.json
(currently "Retorno automático") with "Automático" to reflect automatic runtime
selection; leave the "system" and "builtin" keys ("Runtime do sistema", "Runtime
integrado") unchanged.
In `@src/renderer/src/pages/AgentWelcomePage.vue`:
- Around line 26-28: The agent type label currently falls back to a hardcoded
'ACP' string instead of using i18n; update the template to use the vue-i18n key
(replace the 'ACP' literal with t('welcome.agentPage.acpType')) and add the
corresponding locale entries for welcome.agentPage.acpType in all locale files
so all user-facing text is localized; locate the usage where agent.type is
evaluated (the expression using t('welcome.agentPage.deepchatType')) and mirror
that pattern for the ACP case.
In `@test/renderer/components/NewThreadPage.test.ts`:
- Around line 295-314: The test in NewThreadPage.test checks
draftStore.projectDir but the draftStore mock used by setup lacks a projectDir
property; update the mock returned by setup (or the draftStore mock used in this
test) to include a mutable projectDir field (initialized to the expected
default, e.g., '/workspaces/global' or undefined) and ensure any methods that
change it (if present) update that property so the assertion
expect(draftStore.projectDir).toBe('/workspaces/agent-writer') can pass; locate
the mock/draftStore in the setup helper and add projectDir and any setter
behavior to reflect project selection changes.
---
Outside diff comments:
In `@src/main/presenter/deepchatAgentPresenter/index.ts`:
- Around line 395-405: The current await of
compactionService.prepareForNextUserTurn (assigning compactionIntent) can throw
and abort the send before the user message and history are created; wrap the
prepareForNextUserTurn call in a try/catch and on any error log it and set
compactionIntent to a safe "no compaction" fallback (e.g., null or an explicit
no-op intent) so the subsequent code that uses normalizedInput,
interleavedReasoning.preserveReasoningContent, sessionId, providerId, and
modelId still proceeds to create and persist the user message; ensure you still
call the existing send path with the fallback compactionIntent rather than
returning or throwing.
In `@src/main/presenter/newAgentPresenter/index.ts`:
- Around line 127-156: The code resolves providerId/modelId using defaults and
deepChatAgentConfig but ignores the agentType, causing ACP agents to inherit the
default LLM provider (e.g., "openai") and skip the ACP workdir guard; update the
resolution logic in NewAgentPresenter so that when agentType === 'acp' you force
providerId = 'acp' and set modelId from deepChatAgentConfig or input specific to
ACP (falling back only if appropriate), then call
this.assertAcpSessionHasWorkdir(providerId, projectDir) when agentType === 'acp'
(or providerId === 'acp') before initSession; adjust the code paths that compute
providerId, modelId and the assertAcpSessionHasWorkdir invocation to reference
agentType, providerId, modelId, deepChatAgentConfig, getDefaultModel(), and
initSession accordingly.
In `@src/renderer/src/components/chat/ChatStatusBar.vue`:
- Around line 2222-2235: The watcher for syncGenerationSettings is missing the
DeepChat agent identifier, so switching agents can leave per-agent defaults
(like localSettings.systemPrompt) stale; update the watched sources to include
the agent id (e.g., sessionStore.activeSession?.agentId or whichever property
holds the DeepChat agent) alongside the existing keys
(sessionStore.activeSessionId, providerId, modelId, draftModelSelection,
isAcpAgent) so that syncGenerationSettings() runs when the active agent changes;
locate the watcher block around the call to syncGenerationSettings and add the
agent id accessor to the dependency array.
---
Nitpick comments:
In `@src/main/presenter/agentRepository/index.ts`:
- Around line 223-244: Add a short doc comment to createManualAcpAgent
explaining that it uses an upsert on agentsTable and will update an existing
agent if the provided id already exists (so callers may get update semantics
rather than pure create), and note this is intentional to support migrations;
reference the function name createManualAcpAgent and the use of
this.sqlitePresenter.agentsTable.upsert so reviewers can find the behavior to
adjust callers if needed.
- Around line 333-340: setAgentEnabled currently flips enabled for any agent
without validating agent_type, which diverges from setAgentEnvOverride and
setAgentInstallState; update setAgentEnabled to fetch the agent row
(this.sqlitePresenter.agentsTable.get(agentId)), verify row.agent_type matches
the ACP type used elsewhere (e.g., 'acp' or the project constant) and return
false if it does not, then proceed to update enabled; alternatively, if enabling
non-ACP agents is intentional, add an explicit comment in setAgentEnabled
documenting that behavior and why it differs from
setAgentEnvOverride/setAgentInstallState.
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 464-473: The method updateBuiltinDeepChatConfig currently returns
silently if this.agentRepository is falsy causing lost updates; change it to
surface the failure by logging a warning or throwing an error: inside
updateBuiltinDeepChatConfig check this.agentRepository and if absent call the
presenter logger (or console) to warn (including BUILTIN_DEEPCHAT_AGENT_ID and
the attempted updates) or throw a descriptive Error, otherwise continue calling
this.agentRepository.updateDeepChatAgent(...) and this.notifyAcpAgentsChanged();
reference updateBuiltinDeepChatConfig, this.agentRepository,
BUILTIN_DEEPCHAT_AGENT_ID and notifyAcpAgentsChanged when making the change.
- Around line 1685-1689: In resolveDeepChatAgentConfig, explicitly handle empty
or whitespace-only agentId by trimming and defaulting to
BUILTIN_DEEPCHAT_AGENT_ID before calling
getAgentRepositoryOrThrow().resolveDeepChatAgentConfig; change the argument
expression to use agentId?.trim() || BUILTIN_DEEPCHAT_AGENT_ID so blank strings
won’t be treated as valid IDs and the fallback is explicit.
- Around line 384-386: The current migration loops
this.acpConfHelper.getManualAgents() and calls
repository.createManualAcpAgent(agent) without handling failures; wrap each call
in a try/catch so a single failing agent doesn’t abort the whole loop, log or
collect per-agent errors, and after the loop fail the overall migration (throw
or return an error) if any agent failed so the migration version is not set;
specifically modify the block using this.acpConfHelper.getManualAgents() and
repository.createManualAcpAgent(...) to catch individual errors, continue
migrating remaining agents, and surface aggregated errors to prevent advancing
the migration state.
In `@src/main/presenter/index.ts`:
- Around line 131-136: The code is using a verbose double type assertion to call
a setter on this.configPresenter; instead extend IConfigPresenter to include an
optional setAgentRepository?(repository: AgentRepository): void and then remove
the casts by typing this.configPresenter as IConfigPresenter (or ensure the
concrete presenter implements the extended interface), then call
this.configPresenter.setAgentRepository?.(agentRepository) directly; reference
the symbols AgentRepository, SQLitePresenter, IConfigPresenter,
setAgentRepository and configPresenter when making the change.
In `@src/renderer/settings/main.ts`:
- Around line 72-103: The route entries (e.g., routes with name 'settings-mcp',
'settings-deepchat-agents', 'settings-acp') use fractional meta.position values
(2.5, 3.5, 4.5); update these meta.position fields to a normalized integer
sequence (e.g., 2, 3, 4 or another consistent integer ordering) across all route
definitions to avoid gaps and make future inserts simpler, ensuring each route's
meta.position is unique and ordered consistently.
In `@src/renderer/src/i18n/en-US/settings.json`:
- Line 115: The key "interleaved" in settings UI copy uses the abbreviation
"compat"; update the value to use the full wording for clarity—replace "Force
interleaved thinking compat" with something like "Force interleaved thinking
compatibility" or "Force interleaved thinking compatibility mode" in the
"interleaved" entry of src/renderer/src/i18n/en-US/settings.json so the
user-facing setting uses the full term.
In `@src/renderer/src/i18n/zh-TW/settings.json`:
- Around line 1472-1521: Add the new DeepChat agent configuration surface (the
"deepchatAgents" object and its keys such as builtIn, name/namePlaceholder,
avatarTitle/avatarLucide/avatarMonogram and their Descs, modelsTitle with
chatModel/assistantModel/visionModel,
temperature/contextLength/maxTokens/thinkingBudget/reasoningEffort/verbosity/interleaved,
systemPrompt/systemPromptPlaceholder,
defaultProjectPath/defaultProjectPathPlaceholder,
permissionMode/permissionFullAccess/permissionDefault, toolsTitle,
compactionTitle/compactionEnabled/compactionDescription/compactionThreshold/compactionRetainPairs,
and deleteConfirm) to AGENTS.md; document each field’s purpose, data
type/expected values (e.g., numeric vs boolean vs string), defaults/placeholder
behavior, and any UX implications (avatar options, permission modes, compaction
effects) so the agent configuration contract in AGENTS.md matches the new
settings.json surface.
In `@src/renderer/src/pages/AgentWelcomePage.vue`:
- Around line 68-79: The retry call in openAgentSettings that runs
navigateToSettings after 250ms is a timing-workaround; add an inline comment
above the setTimeout explaining that createSettingsWindow may not immediately
register the settings window ID (race with window initialization), so we
re-check windowPresenter.getSettingsWindowId() and call navigateToSettings again
to ensure the new window is focused/selected; reference navigateToSettings,
windowPresenter.createSettingsWindow and windowPresenter.getSettingsWindowId in
the comment so future readers understand the intent.
In `@test/renderer/components/AgentWelcomePage.test.ts`:
- Around line 1-8: Test file location does not mirror the source structure; move
the test for AgentWelcomePage to follow the same directory layout as the source
so it's discoverable and consistent. Rename/move
test/renderer/components/AgentWelcomePage.test.ts to
test/renderer/pages/AgentWelcomePage.test.ts and update any import paths
referencing AgentWelcomePage (e.g., the component import used in the test and
any relative paths to '@/events' or other helpers) so the test still mounts the
AgentWelcomePage.vue and uses SETTINGS_EVENTS correctly.
In `@test/renderer/components/DeepChatAgentsSettings.test.ts`:
- Around line 1-4: The test file DeepChatAgentsSettings.test.ts is in the wrong
directory; move it to mirror the source component path under
test/renderer/settings/components/ so it sits alongside the source-style path
for src/renderer/settings/components/DeepChatAgentsSettings.vue. After moving,
update any relative imports in the test (e.g., the component import or helper
paths used by the test suite) to reflect the new location and ensure the test
filename remains DeepChatAgentsSettings.test.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f61764b-8dbd-4426-b728-42a16b8b0509
📒 Files selected for processing (76)
src/main/events.tssrc/main/presenter/agentRepository/index.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/deepchatAgentPresenter/compactionService.tssrc/main/presenter/deepchatAgentPresenter/index.tssrc/main/presenter/floatingButtonPresenter/layout.tssrc/main/presenter/index.tssrc/main/presenter/newAgentPresenter/index.tssrc/main/presenter/sqlitePresenter/index.tssrc/main/presenter/sqlitePresenter/tables/agents.tssrc/main/presenter/sqlitePresenter/tables/newSessions.tssrc/renderer/settings/components/CommonSettings.vuesrc/renderer/settings/components/DeepChatAgentsSettings.vuesrc/renderer/settings/main.tssrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/components/chat-input/McpIndicator.vuesrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/icons/AgentAvatar.vuesrc/renderer/src/components/popup/TranslatePopup.vuesrc/renderer/src/events.tssrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/da-DK/routes.jsonsrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/da-DK/welcome.jsonsrc/renderer/src/i18n/en-US/routes.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/en-US/welcome.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fa-IR/routes.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fa-IR/welcome.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/fr-FR/routes.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/fr-FR/welcome.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/he-IL/routes.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/he-IL/welcome.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ja-JP/routes.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ja-JP/welcome.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/ko-KR/routes.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ko-KR/welcome.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/pt-BR/routes.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/pt-BR/welcome.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/ru-RU/routes.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/ru-RU/welcome.jsonsrc/renderer/src/i18n/zh-CN/routes.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-CN/welcome.jsonsrc/renderer/src/i18n/zh-HK/routes.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-HK/welcome.jsonsrc/renderer/src/i18n/zh-TW/routes.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/i18n/zh-TW/welcome.jsonsrc/renderer/src/pages/AgentWelcomePage.vuesrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/ui/agent.tssrc/renderer/src/views/ChatTabView.vuesrc/shared/types/agent-interface.d.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/types/presenters/new-agent.presenter.d.tstest/main/presenter/deepchatAgentPresenter/compactionService.test.tstest/main/presenter/newAgentPresenter/newAgentPresenter.test.tstest/renderer/components/AgentWelcomePage.test.tstest/renderer/components/DeepChatAgentsSettings.test.tstest/renderer/components/NewThreadPage.test.ts
💤 Files with no reviewable changes (1)
- src/renderer/settings/components/CommonSettings.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/renderer/src/components/chat-input/McpIndicator.vue (1)
290-305:⚠️ Potential issue | 🟠 MajorUnknown agent ids still collapse into ACP context.
resolveAgentType()still defaults every unresolved non-deepchatid toacp, andcurrentAgentcan still pass throughagentStore.selectedAgentwithout normalizingagentTypeintotype. A custom DeepChat agent that is temporarily missing fromavailableAgentswill therefore render as ACP and hide the DeepChat system-prompt/tool controls. Please normalize fromagentTypeas well, and make the missing-record fallback default back to DeepChat whenever the active session/selection is not explicitly ACP.Also applies to: 308-332
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/chat-input/McpIndicator.vue` around lines 290 - 305, resolveAgentType currently treats any unknown non-'deepchat' id as 'acp' and reads only matchedAgent.type/agentType inconsistently; update resolveAgentType to (1) normalize both agentType and type from matchedAgent and agentStore.selectedAgent (e.g., prefer matchedAgent.type ?? matchedAgent.agentType, then selectedAgent.type ?? selectedAgent.agentType) and (2) change the missing-record fallback so that if neither source explicitly indicates 'acp' you return 'deepchat' (i.e., only return 'acp' when an explicit type/agentType === 'acp'); apply the same normalization & fallback logic in the other similar block referenced around lines 308-332.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/windowPresenter/index.ts`:
- Around line 446-449: sendToWindow currently returns success immediately when
shouldQueueSettingsMessage enqueues a settings:* message
(pendingSettingsMessages.push({ channel, args })) but those queued messages can
be dropped by resetSettingsWindowState(true) with no notice; update
sendToWindow/shouldQueueSettingsMessage to surface failures by either returning
false/error when a queued message is later dropped or by logging the dropped
channels when resetSettingsWindowState(true) clears pendingSettingsMessages.
Specifically, modify the logic around shouldQueueSettingsMessage/sendToWindow to
record an identifier for each queued message and have
resetSettingsWindowState(true) iterate and log (via the same logger used
elsewhere) the dropped channel names and identifiers, or change sendToWindow to
await confirmation from createSettingsWindow/open renderer bootstrap and return
a failure boolean so callers like createSettingsWindow,
lifecyclePresenter/hooks/ready/eventListenerSetupHook.ts and
toolPresenter/agentTools/chatSettingsTools.ts can handle the error instead of
silently assuming success.
In `@src/renderer/src/pages/AgentWelcomePage.vue`:
- Line 5: The <img> in AgentWelcomePage.vue (logo image) is missing an alt
attribute; update the <img> element to include an explicit accessibility
strategy: either set alt="" and aria-hidden="true" for a purely decorative logo,
or supply a localized alt string (e.g., via i18n) if the logo conveys meaning,
ensuring the <img> tag includes the chosen alt and aria attribute accordingly.
---
Duplicate comments:
In `@src/renderer/src/components/chat-input/McpIndicator.vue`:
- Around line 290-305: resolveAgentType currently treats any unknown
non-'deepchat' id as 'acp' and reads only matchedAgent.type/agentType
inconsistently; update resolveAgentType to (1) normalize both agentType and type
from matchedAgent and agentStore.selectedAgent (e.g., prefer matchedAgent.type
?? matchedAgent.agentType, then selectedAgent.type ?? selectedAgent.agentType)
and (2) change the missing-record fallback so that if neither source explicitly
indicates 'acp' you return 'deepchat' (i.e., only return 'acp' when an explicit
type/agentType === 'acp'); apply the same normalization & fallback logic in the
other similar block referenced around lines 308-332.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c6d1adea-264a-40ec-9f48-db5ac280f1de
📒 Files selected for processing (26)
src/main/events.tssrc/main/presenter/lifecyclePresenter/hooks/ready/eventListenerSetupHook.tssrc/main/presenter/windowPresenter/index.tssrc/renderer/settings/App.vuesrc/renderer/src/components/chat-input/McpIndicator.vuesrc/renderer/src/events.tssrc/renderer/src/i18n/da-DK/welcome.jsonsrc/renderer/src/i18n/en-US/welcome.jsonsrc/renderer/src/i18n/fa-IR/welcome.jsonsrc/renderer/src/i18n/fr-FR/welcome.jsonsrc/renderer/src/i18n/he-IL/welcome.jsonsrc/renderer/src/i18n/ja-JP/welcome.jsonsrc/renderer/src/i18n/ko-KR/welcome.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/pt-BR/welcome.jsonsrc/renderer/src/i18n/ru-RU/welcome.jsonsrc/renderer/src/i18n/zh-CN/welcome.jsonsrc/renderer/src/i18n/zh-HK/welcome.jsonsrc/renderer/src/i18n/zh-TW/welcome.jsonsrc/renderer/src/pages/AgentWelcomePage.vuesrc/renderer/src/pages/WelcomePage.vuetest/main/presenter/windowPresenter.test.tstest/renderer/components/AgentWelcomePage.test.tstest/renderer/components/NewThreadPage.test.tstest/renderer/components/SettingsApp.test.tstest/renderer/components/WelcomePage.test.ts
💤 Files with no reviewable changes (1)
- src/main/presenter/lifecyclePresenter/hooks/ready/eventListenerSetupHook.ts
✅ Files skipped from review due to trivial changes (7)
- src/renderer/src/i18n/da-DK/welcome.json
- src/renderer/src/i18n/he-IL/welcome.json
- src/renderer/src/i18n/zh-CN/welcome.json
- src/renderer/src/i18n/ko-KR/welcome.json
- src/renderer/src/i18n/ru-RU/welcome.json
- src/renderer/src/i18n/zh-TW/welcome.json
- src/renderer/src/i18n/pt-BR/welcome.json
🚧 Files skipped from review as they are similar to previous changes (9)
- src/renderer/src/i18n/en-US/welcome.json
- src/renderer/src/i18n/ja-JP/welcome.json
- src/renderer/src/events.ts
- src/renderer/src/i18n/fr-FR/welcome.json
- test/renderer/components/NewThreadPage.test.ts
- src/renderer/src/i18n/zh-HK/welcome.json
- src/renderer/src/i18n/fa-IR/welcome.json
- test/renderer/components/AgentWelcomePage.test.ts
- src/main/events.ts
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
1 participant
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 by CodeRabbit
New Features
UI Components
Localization
Settings/UX