Conversation
Add agent icon sidebar with liquid drop styling and session list with date-grouped mock data. Main content area gets rounded corner and shadow separation. Sidebar/appbar use semi-transparent background.
Move browser and settings buttons from AppBar to WindowSideBar bottom icon column. Remove history button, ThreadView overlay, and all associated toggle/event handling code.
Add a pure visual mock for the redesigned new thread page using shadcn primitives. Includes centered logo/heading, textarea input with attach/mic/send toolbar, and a status bar with model, effort, and permissions selectors.
…rouping toggle Refactor NewThreadMock into reusable mock components (MockInputBox, MockInputToolbar, MockStatusBar). Add MockChatPage with sticky top bar and input area, MockMessageList with sample conversation, and shared mock view state composable. Wire sidebar session clicks to switch between new thread and chat views. Add project grouping toggle to sidebar header.
…at top bar Add a dropdown project/workdir selector to NewThreadMock with recent projects and an open folder option. Thread project directory through mock view state so MockTopBar displays a folder breadcrumb prefix alongside the session title.
…apse - Add MockWelcomePage with provider grid and ACP agent setup option - Add DeepChat as default first agent in sidebar - Add empty state for session list - Add sidebar collapse/expand with macOS traffic light accommodation - Add debug toggle button for welcome page
* docs: add spec for default model setting * feat: #1174 support default models setting * fix(settings): sync model selections with ACP mode and improve error handling - Add ACP mode check when applying default model in NewThread.vue - Add error handling in DefaultModelSettingsSection.vue syncModelSelections - Update imageModel i18n keys to remove 'select' verb for display-only fields
* fix(settings): disable auto-fallback in default model selection Remove automatic model fallback in DefaultModelSettingsSection to prevent unintended model changes when providers load late. Models now only change on explicit user selection. * fix: format
…ideBar Remove multi-tab/multi-window complexity from presenters, simplify shell UI by removing SideBar component and streamlining AppBar, and clean up unused event handlers and shortcut bindings.
… and sidebar integration Implement Phases 1-4 of the new UI architecture: - Phase 1: Add 4 Pinia stores (pageRouter, session, agent, project) with IPC mapping to presenters, event listeners, and error handling - Phase 2: Create 3 page components (WelcomePage, NewThreadPage, ChatPage) and refactor ChatTabView to route via pageRouter store - Phase 3: Create 5 chat components (ChatTopBar, MessageList, ChatInputBox, ChatInputToolbar, ChatStatusBar) matching mock designs exactly - Phase 4: Refactor WindowSideBar to use store data instead of mock data - Add specs and implementation plan documentation
The agentPresenter.sendMessage expects content as JSON-encoded
UserMessageContent ({text, files, links, search, think}), not raw
text. Raw text caused JSON.parse failures in MessageManager and
TypeError when setting properties on non-object content.
ChatPage now uses useChatStore().variantAwareMessages for the message list and chatStore.sendMessage() for sending. MessageList component updated to handle real Message types (UserMessageContent for user messages, AssistantMessageBlock[] for assistant messages) instead of plain text placeholders.
…hase 6) - NewThreadPage passes agentStore.selectedAgentId to session creation, with providerId/modelId set for ACP agents - Session creation uses forceNewAndActivate to prevent backend from reusing empty conversations with different agent settings - ChatStatusBar wired to chatStore.chatConfig and modelStore for real model/effort selection; shows selected agent context on NewThreadPage - Effort selector hidden for ACP agents (not applicable) - MessageList rewritten to use existing MessageItemAssistant and MessageItemUser components for full markdown/code/tool rendering - modelStore.initialize() added to ChatTabView onMounted - Phase 6 specs and todo tracking added
Implement agent-centric architecture replacing old sessionPresenter pattern: - Shared types: IAgentImplementation, Agent, Session, ChatMessageRecord, etc. - DB tables: new_sessions, new_projects, deepchat_sessions, deepchat_messages - DeepChatAgentPresenter: message persistence, LLM coreStream consumption, batched stream flushing (120ms renderer, 600ms DB), crash recovery - NewAgentPresenter: agent registry, session manager, message routing - ProjectPresenter: project CRUD with directory picker - Renderer stores: session, message, agent, project, draft (all new arch) - ChatPage wired to new message store with streaming block display - NewThreadPage resolves model via defaultModel/preferredModel/first enabled - 94 unit + integration tests across 9 test files - Debug logging throughout processing pipeline
- Add contextBuilder with system prompt injection, conversation history assembly, and token-based truncation using approximateTokenSize - Wire buildContext into processMessage, building context before DB persist to avoid duplicate user messages in LLM calls - Add optimistic user message in renderer message store so user messages appear immediately without waiting for stream completion - Add auto-scroll to ChatPage: scroll to bottom on load, on new messages, and during streaming; respect user scroll-up to stop auto-following - Update v0 tests and add new tests for context builder and multi-turn integration (110 tests passing across 10 files)
Add tool calling support to the DeepChat agent via an agent loop that passes tool definitions to coreStream, accumulates tool calls from the stream, executes them via ToolPresenter.callTool(), and re-calls the LLM until it stops requesting tools. Includes block accumulation across loop iterations, server info enrichment, grouped truncation of tool messages in context builder, and proper abort/error path delegation between streamHandler and agentLoop.
DeepSeek Reasoner (and similar models like kimi-k2-thinking, glm-4.7) requires a reasoning_content field on assistant messages that contain tool_calls. Without it, the API returns a 400 error. Extract reasoning blocks separately in both the agent loop and context builder, and include them on the assistant message when the model requires it.
For interleaved thinking models (DeepSeek Reasoner, kimi-k2-thinking), reasoning_content is only required on assistant messages in the current agent loop exchange (after the last user message), not on historical messages from the DB. Also fix content field to always be a string (not undefined) on assistant messages with tool_calls, matching the existing agentLoopHandler behavior.
Replace tangled streamHandler.ts and agentLoop.ts with a clean 5-module architecture: types.ts (shared state), accumulator.ts (pure event→block mutations), echo.ts (batched renderer/DB flushing), dispatch.ts (tool execution and finalization), and process.ts (unified loop). Eliminates tools-vs-no-tools branching in index.ts — single processStream() call handles both simple completions and multi-turn tool calling. Extracts reusable trailing-edge throttle utility to shared/utils/throttle.ts. Pure refactor with no behavior changes.
New agent sessions live in deepchat_sessions, not the legacy conversations table. Passing the session ID as conversationId caused SkillPresenter.getActiveSkills to throw "Conversation not found". Drop the conversationId param since the new agent doesn't use skills.
Mark v2 spec as complete and superseded by v3 module structure. Update v3 spec to match final implementation. Clean up tasks to remove references to deleted files and add v2/v3 task sections with all items checked off.
📝 WalkthroughWalkthroughRemoves a macOS dyld entitlement; adds default model/vision settings and UI; implements a new agent architecture (NewAgent + DeepChatAgent) with modular streaming/tool-calling, new SQLite tables, renderer page/router and stores, many new UI components, tests, and related presenter/type surface extensions. Changes
Sequence Diagram(s)mermaid Renderer->>NewAgentPresenter: createSession / sendMessage Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/renderer/src/components/ModelSelect.vue (1)
72-85: 🛠️ Refactor suggestion | 🟠 MajorUse a typed props interface + withDefaults for defaults.
This file now changes the props block, but it still uses an object literal for
defineProps. The guideline requires TypeScript interfaces for props. Consider switching to an interface withdefineProps<T>()andwithDefaultsfor default values.✅ Suggested refactor
-const props = defineProps({ - type: { - type: Array as PropType<ModelType[]>, - default: undefined // ← explicit for clarity - }, - excludeProviders: { - type: Array as PropType<string[]>, - default: () => [] - }, - visionOnly: { - type: Boolean, - default: false - } -}) +interface ModelSelectProps { + type?: ModelType[] + excludeProviders?: string[] + visionOnly?: boolean +} + +const props = withDefaults(defineProps<ModelSelectProps>(), { + excludeProviders: () => [], + visionOnly: false +})As per coding guidelines, "Define TypeScript interfaces for Vue component props and data structures."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/ModelSelect.vue` around lines 72 - 85, Replace the object-literal props with a typed props interface and use withDefaults + defineProps to provide defaults: create an interface (e.g., Props) declaring type?: ModelType[], excludeProviders?: string[], visionOnly?: boolean; then call defineProps<Props>() and wrap it with withDefaults to set type to undefined, excludeProviders to () => [], and visionOnly to false, updating any references to the existing props variable (const props) accordingly.src/main/presenter/shortcutPresenter.ts (1)
154-210:⚠️ Potential issue | 🟠 MajorRegistered-but-no-op shortcuts still consume OS-level keybindings.
The four tab-switch methods (lines 200–210) are correctly gutted, but the
globalShortcut.registercall-sites at lines 154–193 are still active. Electron'sglobalShortcutmodule registers shortcuts with the operating system, and they work even when the app does not have keyboard focus. Registering a shortcut with an empty callback means the key combination is captured and silently swallowed — Electron will override shortcuts of other applications; if you try to use this shortcut while Chrome or another app is open, your Electron application will override it.Concretely:
Ctrl+Tab,Ctrl+Shift+Tab,Ctrl+1–Ctrl+8, andCtrl+9will cease to work in any other application while DeepChat is running, and they'll do nothing inside DeepChat either.Fix: Remove the now-purposeless registration blocks entirely:
🔧 Proposed fix — drop the four no-op registration blocks
- // Command+Tab 或 Ctrl+Tab 切换到下一个标签页 - if (this.shortcutKeys.SwitchNextTab) { - globalShortcut.register(this.shortcutKeys.SwitchNextTab, () => { - const focusedWindow = presenter.windowPresenter.getFocusedWindow() - if (focusedWindow?.isFocused()) { - this.switchToNextTab(focusedWindow.id) - } - }) - } - - // Ctrl+Shift+Tab 切换到上一个标签页 - if (this.shortcutKeys.SwitchPrevTab) { - globalShortcut.register(this.shortcutKeys.SwitchPrevTab, () => { - const focusedWindow = presenter.windowPresenter.getFocusedWindow() - if (focusedWindow?.isFocused()) { - this.switchToPreviousTab(focusedWindow.id) - } - }) - } - - // 注册标签页数字快捷键 (1-8) - if (this.shortcutKeys.NumberTabs) { - for (let i = 1; i <= 8; i++) { - globalShortcut.register(`${CommandKey}+${i}`, () => { - const focusedWindow = presenter.windowPresenter.getFocusedWindow() - if (focusedWindow?.isFocused()) { - this.switchToTabByIndex(focusedWindow.id, i - 1) - } - }) - } - } - - // Command+9 或 Ctrl+9 切换到最后一个标签页 - if (this.shortcutKeys.SwtichToLastTab) { - globalShortcut.register(this.shortcutKeys.SwtichToLastTab, () => { - const focusedWindow = presenter.windowPresenter.getFocusedWindow() - if (focusedWindow?.isFocused()) { - this.switchToLastTab(focusedWindow.id) - } - }) - } - this.showHideWindow()If the shortcuts are intended to be re-wired to the new page-routing system in a follow-up, consider leaving a
// TODOcomment explaining the intent rather than silently capturing and discarding the keystrokes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/shortcutPresenter.ts` around lines 154 - 210, The globalShortcut.register calls for shortcutKeys.SwitchNextTab, SwitchPrevTab, NumberTabs and SwtichToLastTab are still registering OS-level shortcuts while the corresponding handlers (switchToNextTab, switchToPreviousTab, switchToTabByIndex, switchToLastTab) are no-ops; remove those registration blocks (the globalShortcut.register(…) usages tied to this.shortcutKeys.SwitchNextTab, .SwitchPrevTab, .NumberTabs, and .SwtichToLastTab) so the app stops capturing Ctrl/Cmd+Tab, Ctrl/Cmd+Shift+Tab, Ctrl/Cmd+1–9, etc.; if you plan to rewire them later, replace each removed block with a brief TODO comment referencing the handler names (switchToNextTab, switchToPreviousTab, switchToTabByIndex, switchToLastTab) so future work knows where to re-add registrations.src/main/presenter/sessionPresenter/managers/conversationManager.ts (1)
177-201:⚠️ Potential issue | 🔴 Critical
DEFAULT_SETTINGSis mutated in-place — corrupts state across all subsequent calls.At line 177,
defaultSettingsis assigned by reference toDEFAULT_SETTINGS. When!latestConversation?.settingsis true (precisely the condition required forshouldApplyDefaultModelto betrue), the block at lines 178-186 is skipped, sodefaultSettingsnever becomes a copy — it remains the same object reference. Lines 198-199 then directly write toDEFAULT_SETTINGS.modelIdandDEFAULT_SETTINGS.providerId, permanently polluting the module-level constant for the remainder of the process lifetime. Every subsequent call tocreateConversationthat lacks alatestConversation?.settingswill inherit the wrong model.🐛 Proposed fix — shallow-copy DEFAULT_SETTINGS at the declaration site
- let defaultSettings = DEFAULT_SETTINGS + let defaultSettings = { ...DEFAULT_SETTINGS } if (latestConversation?.settings) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/sessionPresenter/managers/conversationManager.ts` around lines 177 - 201, defaultSettings is being assigned by reference to DEFAULT_SETTINGS so later writes (e.g., setting modelId/providerId) mutate the module-level constant; change the initialization to create a shallow copy (e.g., use an object spread) when declaring defaultSettings so mutations only affect the local variable, i.e., replace the direct assignment to DEFAULT_SETTINGS with a shallow-copy of DEFAULT_SETTINGS before any modifications in the createConversation flow (references: DEFAULT_SETTINGS and the local variable defaultSettings in conversationManager.ts).src/main/presenter/mcpPresenter/inMemoryServers/imageServer.ts (2)
52-88:⚠️ Potential issue | 🟠 Major
getEffectiveModel()'s live-lookup and error-throw branches are dead code due to constructor hard-coding.Because the constructor unconditionally assigns
this.providerandthis.model— falling back to'openai'/'gpt-4o'when nothing else is configured — both fields are always truthy after construction.getEffectiveModel()therefore always returns the first branch and never reaches thegetDefaultVisionModel()re-fetch or thethrow. Two consequences:
- If the user updates the default vision model while the server is running, the change is not picked up (stale values baked in at construction).
- If no vision model is configured at all, the server silently uses
openai/gpt-4oinstead of surfacing the "no vision model configured" error thatgetEffectiveModel()is supposed to throw.🐛 Proposed fix — remove hard defaults from constructor, rely on `getEffectiveModel()` for live resolution
- private provider: string - private model: string + private provider: string | undefined + private model: string | undefined constructor(provider?: string, model?: string) { - const defaultVisionModel = presenter.configPresenter.getDefaultVisionModel() - this.provider = provider || defaultVisionModel?.providerId || 'openai' - this.model = model || defaultVisionModel?.modelId || 'gpt-4o' + this.provider = provider + this.model = model this.server = new Server(
getEffectiveModel()already handles all resolution paths correctly whenthis.provider/this.modelcan beundefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/inMemoryServers/imageServer.ts` around lines 52 - 88, The constructor currently hard-sets this.provider and this.model to defaults, which prevents getEffectiveModel() from performing live lookups or throwing when no model is configured; change the constructor to assign only the incoming parameters (this.provider = provider; this.model = model) so they can be undefined, rely on getEffectiveModel() everywhere that needs the resolved model (call getEffectiveModel() in request handlers or methods that use provider/model), and keep the existing error-throw and default-resolution logic inside getEffectiveModel() intact so updates to presenter.configPresenter.getDefaultVisionModel() are picked up at runtime.
122-122:⚠️ Potential issue | 🟠 MajorMIME type hardcoded to
image/jpegfor all images.Both
queryImageWithModelandocrImageWithModelconstructdata:image/jpeg;base64,...regardless of the file's actual extension. Sending a PNG or WEBP file asimage/jpegcan cause the model to misparse or reject it.🐛 Proposed fix using extension-based lookup
+ const ext = path.extname(filePath).toLowerCase().slice(1) + const mimeMap: Record<string, string> = { + jpg: 'image/jpeg', jpeg: 'image/jpeg', png: 'image/png', + gif: 'image/gif', webp: 'image/webp', bmp: 'image/bmp' + } + const mime = mimeMap[ext] ?? 'image/jpeg' - const dataUrl = `data:image/jpeg;base64,${base64Image}` + const dataUrl = `data:${mime};base64,${base64Image}`Apply the same fix to the equivalent block in
ocrImageWithModel(Line 168).Also applies to: 168-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/mcpPresenter/inMemoryServers/imageServer.ts` at line 122, The data URL is hardcoded to image/jpeg which breaks non-JPEG inputs; update both queryImageWithModel and ocrImageWithModel so the MIME type is derived from the file extension (or actual buffer detection) and used when building the dataUrl (the line creating `dataUrl = \`data:image/...;base64,${base64Image}\``). Locate the `dataUrl` and `base64Image` usage in both functions, map extensions (e.g., .png → image/png, .webp → image/webp, .gif → image/gif) or use a mime lookup utility, and replace the constant "image/jpeg" with the resolved MIME type; ensure fallback to image/octet-stream or image/jpeg if resolution fails and apply the same change at the analogous line in `ocrImageWithModel`.src/main/presenter/index.ts (1)
350-362:⚠️ Potential issue | 🟠 MajorAdd cleanup for
newAgentPresenterindestroy().
newAgentPresenterholdsDeepChatAgentPresenter(which maintainsabortControllersandruntimeStatemaps) and manages sessions/messages. These resources are never cleaned up on app shutdown, causing potential leaks of active abort controllers and in-memory state.Add
this.newAgentPresenter.destroy()to the destroy method (after implementing the method inNewAgentPresenterandDeepChatAgentPresenterto clean up their internal resources).🤖 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 350 - 362, The destroy() in presenter index omits cleanup for newAgentPresenter which leaves DeepChatAgentPresenter's abortControllers and runtimeState maps and session/message state leaking; implement a destroy method on NewAgentPresenter (and ensure DeepChatAgentPresenter exposes/cleans its internal abortControllers and runtimeState maps) to abort any pending controllers, clear maps and session/message state, then add this.newAgentPresenter.destroy() into the index's destroy() sequence (place it alongside other presenters' destroy calls) so all agent-related resources are released on shutdown.
♻️ Duplicate comments (5)
src/renderer/src/i18n/da-DK/settings.json (1)
62-67: Duplicate: locale completeness for new settings keys.Same concern as in
src/renderer/src/i18n/fa-IR/settings.jsonLine 62-67—please ensure the new keys exist across all locales and stay consistent.Based on learnings: Add new translations to ALL language files (da-DK, en-US, fa-IR, fr-FR, he-IL, ja-JP, ko-KR, pt-BR, ru-RU, zh-CN, zh-HK, zh-TW) with consistent key names across all locales.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/da-DK/settings.json` around lines 62 - 67, The new i18n keys ("fileMaxSizeHint" and the nested "defaultModel" keys: "title", "chatModel", "visionModel") were added to da-DK but are missing or inconsistent across locales; update every locale file (en-US, fa-IR, fr-FR, he-IL, ja-JP, ko-KR, pt-BR, ru-RU, zh-CN, zh-HK, zh-TW and da-DK) to include these exact keys with appropriate translations so all locale JSONs have the same key structure and names ("fileMaxSizeHint", "defaultModel.title", "defaultModel.chatModel", "defaultModel.visionModel").src/renderer/src/i18n/pt-BR/settings.json (1)
62-67: Duplicate: locale completeness for new settings keys.Same concern as in
src/renderer/src/i18n/fa-IR/settings.jsonLine 62-67—please ensure the new keys exist across all locales and stay consistent.Based on learnings: Add new translations to ALL language files (da-DK, en-US, fa-IR, fr-FR, he-IL, ja-JP, ko-KR, pt-BR, ru-RU, zh-CN, zh-HK, zh-TW) with consistent key names across all locales.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/pt-BR/settings.json` around lines 62 - 67, Add the new translation keys introduced here—fileMaxSizeHint and the nested defaultModel keys (title, chatModel, visionModel)—to every locale file so key names are consistent across all locales (da-DK, en-US, fa-IR, fr-FR, he-IL, ja-JP, ko-KR, pt-BR, ru-RU, zh-CN, zh-HK, zh-TW); find the same settings JSON structure in each locale and add equivalent translated strings for "fileMaxSizeHint" and defaultModel.{title,chatModel,visionModel} to match the key names used in this diff.src/renderer/src/i18n/zh-TW/settings.json (1)
62-67: Duplicate: locale completeness for new settings keys.Same concern as in
src/renderer/src/i18n/fa-IR/settings.jsonLine 62-67—please ensure the new keys exist across all locales and stay consistent.Based on learnings: Add new translations to ALL language files (da-DK, en-US, fa-IR, fr-FR, he-IL, ja-JP, ko-KR, pt-BR, ru-RU, zh-CN, zh-HK, zh-TW) with consistent key names across all locales.
🤖 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 62 - 67, The new translation keys fileMaxSizeHint and the defaultModel object (with nested keys title, chatModel, visionModel) are present in zh-TW but missing or inconsistent across other locales; update all locale settings.json files (da-DK, en-US, fa-IR, fr-FR, he-IL, ja-JP, ko-KR, pt-BR, ru-RU, zh-CN, zh-HK) to include these exact keys with appropriate translations so key names remain consistent across all locale files.src/renderer/src/components/icons/MinimizeIcon.vue (1)
1-19: Same Iconify lucide guideline concern asRestoreIcon.vue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/icons/MinimizeIcon.vue` around lines 1 - 19, The MinimizeIcon.vue SVG does not follow the Iconify/lucide accessibility/consistency guideline (same issue as RestoreIcon.vue); update the <svg> in MinimizeIcon.vue to match the standardized icon pattern used in RestoreIcon.vue by adding the required attributes (e.g., role="img", focusable="false", aria-hidden="true", keep xmlns and viewBox), keep the :fill="fill" prop and the withDefaults/defineProps setup for fill, and ensure the SVG markup otherwise mirrors the RestoreIcon.vue implementation for accessibility and consistency.src/renderer/src/components/icons/CloseIcon.vue (1)
1-19: Same Iconify lucide guideline concern asRestoreIcon.vue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/icons/CloseIcon.vue` around lines 1 - 19, The CloseIcon.vue SVG doesn't follow the same Iconify/lucide guidelines as RestoreIcon.vue; update the <svg> element (in the template) to use scalable sizing and accessibility attributes: replace fixed width/height "10" with "1em", add role="img", aria-hidden="true" and focusable="false" attributes, and keep using the existing fill prop (defineProps/withDefaults for fill) so color is inherited via 'currentColor'.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (131)
build/entitlements.mac.plistdocs/architecture/new-ui-implementation-plan.mddocs/specs/default-model-settings/plan.mddocs/specs/default-model-settings/spec.mddocs/specs/default-model-settings/tasks.mddocs/specs/new-agent/plan.mddocs/specs/new-agent/spec.mddocs/specs/new-agent/tasks.mddocs/specs/new-agent/v1-spec.mddocs/specs/new-agent/v2-spec.mddocs/specs/new-agent/v3-spec.mddocs/specs/new-ui-agent-session/spec.mddocs/specs/new-ui-agent-store/spec.mddocs/specs/new-ui-chat-components/spec.mddocs/specs/new-ui-implementation/todo.mddocs/specs/new-ui-markdown-rendering/spec.mddocs/specs/new-ui-page-state/spec.mddocs/specs/new-ui-pages/spec.mddocs/specs/new-ui-project-store/spec.mddocs/specs/new-ui-session-store/spec.mddocs/specs/new-ui-sidebar/spec.mddocs/specs/new-ui-status-bar/spec.mdsrc/main/events.tssrc/main/presenter/agentPresenter/utility/utilityHandler.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/deepchatAgentPresenter/accumulator.tssrc/main/presenter/deepchatAgentPresenter/contextBuilder.tssrc/main/presenter/deepchatAgentPresenter/dispatch.tssrc/main/presenter/deepchatAgentPresenter/echo.tssrc/main/presenter/deepchatAgentPresenter/index.tssrc/main/presenter/deepchatAgentPresenter/messageStore.tssrc/main/presenter/deepchatAgentPresenter/process.tssrc/main/presenter/deepchatAgentPresenter/sessionStore.tssrc/main/presenter/deepchatAgentPresenter/types.tssrc/main/presenter/deeplinkPresenter/index.tssrc/main/presenter/index.tssrc/main/presenter/mcpPresenter/inMemoryServers/builder.tssrc/main/presenter/mcpPresenter/inMemoryServers/imageServer.tssrc/main/presenter/newAgentPresenter/agentRegistry.tssrc/main/presenter/newAgentPresenter/index.tssrc/main/presenter/newAgentPresenter/messageManager.tssrc/main/presenter/newAgentPresenter/sessionManager.tssrc/main/presenter/projectPresenter/index.tssrc/main/presenter/sessionPresenter/index.tssrc/main/presenter/sessionPresenter/managers/conversationManager.tssrc/main/presenter/shortcutPresenter.tssrc/main/presenter/sqlitePresenter/index.tssrc/main/presenter/sqlitePresenter/tables/deepchatMessages.tssrc/main/presenter/sqlitePresenter/tables/deepchatSessions.tssrc/main/presenter/sqlitePresenter/tables/newProjects.tssrc/main/presenter/sqlitePresenter/tables/newSessions.tssrc/main/presenter/syncPresenter/index.tssrc/main/presenter/windowPresenter/index.tssrc/preload/index.tssrc/renderer/settings/components/CommonSettings.vuesrc/renderer/settings/components/common/DefaultModelSettingsSection.vuesrc/renderer/shell/App.vuesrc/renderer/shell/components/AppBar.vuesrc/renderer/src/App.vuesrc/renderer/src/assets/style.csssrc/renderer/src/components/AppBar.vuesrc/renderer/src/components/ModelSelect.vuesrc/renderer/src/components/NewThread.vuesrc/renderer/src/components/NewThreadMock.vuesrc/renderer/src/components/SideBar.vuesrc/renderer/src/components/WindowSideBar.vuesrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/components/chat/ChatInputToolbar.vuesrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/chat/ChatTopBar.vuesrc/renderer/src/components/chat/MessageList.vuesrc/renderer/src/components/icons/CloseIcon.vuesrc/renderer/src/components/icons/MaximizeIcon.vuesrc/renderer/src/components/icons/MinimizeIcon.vuesrc/renderer/src/components/icons/ModelIcon.vuesrc/renderer/src/components/icons/RestoreIcon.vuesrc/renderer/src/components/mcp-config/mcpServerForm.vuesrc/renderer/src/components/mock/MockChatPage.vuesrc/renderer/src/components/mock/MockInputBox.vuesrc/renderer/src/components/mock/MockInputToolbar.vuesrc/renderer/src/components/mock/MockMessageList.vuesrc/renderer/src/components/mock/MockStatusBar.vuesrc/renderer/src/components/mock/MockTopBar.vuesrc/renderer/src/components/mock/MockWelcomePage.vuesrc/renderer/src/composables/useMockViewState.tssrc/renderer/src/events.tssrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/pages/ChatPage.vuesrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/pages/WelcomePage.vuesrc/renderer/src/stores/ui/agent.tssrc/renderer/src/stores/ui/draft.tssrc/renderer/src/stores/ui/message.tssrc/renderer/src/stores/ui/pageRouter.tssrc/renderer/src/stores/ui/project.tssrc/renderer/src/stores/ui/session.tssrc/renderer/src/views/ChatTabView.vuesrc/shadcn/components/ui/tooltip/TooltipContent.vuesrc/shared/types/agent-interface.d.tssrc/shared/types/core/chat-message.tssrc/shared/types/presenters/index.d.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/types/presenters/new-agent.presenter.d.tssrc/shared/types/presenters/project.presenter.d.tssrc/shared/utils/throttle.tstest/main/presenter/configPresenter/defaultModelSettings.test.tstest/main/presenter/deepchatAgentPresenter/accumulator.test.tstest/main/presenter/deepchatAgentPresenter/contextBuilder.test.tstest/main/presenter/deepchatAgentPresenter/deepchatAgentPresenter.test.tstest/main/presenter/deepchatAgentPresenter/dispatch.test.tstest/main/presenter/deepchatAgentPresenter/echo.test.tstest/main/presenter/deepchatAgentPresenter/messageStore.test.tstest/main/presenter/deepchatAgentPresenter/process.test.tstest/main/presenter/newAgentPresenter/agentRegistry.test.tstest/main/presenter/newAgentPresenter/integration.test.tstest/main/presenter/newAgentPresenter/messageManager.test.tstest/main/presenter/newAgentPresenter/newAgentPresenter.test.tstest/main/presenter/newAgentPresenter/sessionManager.test.tstest/main/presenter/projectPresenter/projectPresenter.test.tstest/shared/utils/throttle.test.ts
💤 Files with no reviewable changes (2)
- build/entitlements.mac.plist
- src/renderer/src/components/SideBar.vue
| function handleKeydown(e: KeyboardEvent) { | ||
| if (e.key === 'Enter' && !e.shiftKey) { | ||
| e.preventDefault() | ||
| emit('submit') | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing e.isComposing guard causes premature submit for CJK IME users.
Each keydown in an IME is responded to by the browser, so when a user hits Enter to confirm an IME selection, the code immediately processes a keydown event — triggering submit before the user has finished composing their text. The Enter key is mis-recognized and sends the chat message instead of ending the composition.
The fix is a single additional guard:
🐛 Proposed fix
function handleKeydown(e: KeyboardEvent) {
- if (e.key === 'Enter' && !e.shiftKey) {
+ if (e.key === 'Enter' && !e.shiftKey && !e.isComposing) {
e.preventDefault()
emit('submit')
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/chat/ChatInputBox.vue` around lines 30 - 35, The
handleKeydown function submits on Enter currently without checking IME
composition state; update handleKeydown to skip handling when the event
indicates composition (use the KeyboardEvent.isComposing property) so Enter used
to confirm CJK/IME composition won't trigger emit('submit'); locate the function
named handleKeydown and add a guard that returns early if e.isComposing is true
before the existing Enter/shift checks.
| async function initialize(): Promise<void> { | ||
| try { | ||
| // 1. Check if any provider is enabled | ||
| const enabledProviders = configPresenter.getEnabledProviders() | ||
| if (!enabledProviders || enabledProviders.length === 0) { | ||
| route.value = { name: 'welcome' } | ||
| return | ||
| } | ||
|
|
||
| // 2. Check for active session on this tab | ||
| const tabId = window.api.getWebContentsId() | ||
| const activeSession = await sessionPresenter.getActiveSession(tabId) | ||
| if (activeSession) { | ||
| route.value = { name: 'chat', sessionId: activeSession.sessionId } | ||
| return | ||
| } | ||
|
|
||
| // 3. Default to new thread | ||
| route.value = { name: 'newThread' } | ||
| } catch (e) { | ||
| error.value = String(e) | ||
| route.value = { name: 'newThread' } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find presenter-related files and interfaces
find . -type f \( -name "*.ts" -o -name "*.d.ts" \) | grep -i presenter | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 1266
🏁 Script executed:
# Search for getEnabledProviders definition
rg -n "getEnabledProviders" --type=ts -A 2 -B 2Repository: ThinkInAIXYZ/deepchat
Length of output: 2848
🏁 Script executed:
# Search for usePresenter implementation
rg -n "usePresenter" --type=ts -l | head -10Repository: ThinkInAIXYZ/deepchat
Length of output: 469
🏁 Script executed:
# Find usePresenter composable
fd -t f "usePresenter" --extension ts --extension tsxRepository: ThinkInAIXYZ/deepchat
Length of output: 109
🏁 Script executed:
# Search for usePresenter definition
rg -n "export.*usePresenter|function usePresenter|const usePresenter" --type=ts -A 10Repository: ThinkInAIXYZ/deepchat
Length of output: 1079
🏁 Script executed:
# Read usePresenter.ts to understand the proxy implementation
cat -n src/renderer/src/composables/usePresenter.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 3993
Add await to getEnabledProviders() calls — the usePresenter proxy wraps all methods as async.
Although getEnabledProviders() is defined as synchronous in the main process, the usePresenter proxy (lines 73–74 of src/renderer/src/composables/usePresenter.ts) wraps every method call as async via ipcRenderer.invoke(). This means at the renderer level, the call returns Promise<LLM_PROVIDER[]> instead of the array itself.
Without await, enabledProviders at lines 24 and 67 becomes a Promise object. The check at line 25 (enabledProviders.length === 0) evaluates to undefined === 0 (false), so the condition fails and the welcome route is never displayed even when all providers are disabled.
Fix by awaiting both calls:
Diff
async function initialize(): Promise<void> {
try {
// 1. Check if any provider is enabled
- const enabledProviders = configPresenter.getEnabledProviders()
+ const enabledProviders = await configPresenter.getEnabledProviders()
if (!enabledProviders || enabledProviders.length === 0) {
route.value = { name: 'welcome' }
return
}The same issue occurs at line 67 in the event listener handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/stores/ui/pageRouter.ts` around lines 21 - 44, The renderer
is calling getEnabledProviders() via the usePresenter proxy which returns a
Promise, so update calls in initialize() and the provider-change event handler
to await the result (e.g., const enabledProviders = await
configPresenter.getEnabledProviders()); if the handler is not async, mark it
async so you can await inside it; keep using the enabledProviders variable for
the length check and subsequent routing logic (route.value = { name: 'welcome' }
/ 'newThread' / 'chat') unchanged.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
docs/specs/agentpresenter-mvp-replacement/plan.md (4)
112-117: Consider additional migration and operational risks.The identified risks are valid, but several others should be documented:
Performance degradation: Dual-stack overhead during transition phases may slow down the application. Mitigation: performance benchmarks and monitoring.
Data loss: Migration failures could lose or corrupt sessions/messages. Mitigation: backup before migration, atomic transactions, rollback capability.
Concurrent operation conflicts: What if a user edits a message while a retry is in progress? Mitigation: operation locking or queueing per session.
Schema migration failures: Database schema changes might fail on some user machines. Mitigation: migration verification, graceful degradation, error reporting.
User workflow disruption: Breaking changes to familiar UI/UX. Mitigation: user communication, migration guide, feedback channels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/agentpresenter-mvp-replacement/plan.md` around lines 112 - 117, The "8. 风险与缓解" section currently lists three risks but omits several important migration and operational risks; update that section (heading "8. 风险与缓解") by adding entries for Performance degradation, Data loss, Concurrent operation conflicts, Schema migration failures, and User workflow disruption, and for each provide a concise mitigation (e.g., benchmarking/monitoring for performance, backups/atomic transactions/rollback for data loss, per-session locking or operation queueing for concurrent conflicts, migration verification/graceful degradation/error reporting for schema issues, and user communication/migration guide/feedback channels for workflow disruption) so the plan includes these additional risks and corresponding mitigations.
118-124: Consider additional quality gates for production readiness.The current gates are good, but consider adding:
- Build verification:
pnpm run buildsucceeds without warnings- Manual QA: Each phase should have a checklist of manual test scenarios
- Documentation: README, architecture docs, and API docs updated
- Performance: No regression in key metrics (startup time, message send latency)
- Accessibility: WCAG compliance for new UI components
Also, consider defining what "关键单测与集成测试" (key unit and integration tests) means concretely—perhaps a specific list or percentage threshold.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/agentpresenter-mvp-replacement/plan.md` around lines 118 - 124, Update section "9. 质量门槛" to add concrete production-readiness gates: include a build verification step (`pnpm run build`) that must pass without warnings, require a per-phase Manual QA checklist (link or embed checklist template), mandate Documentation tasks (README, architecture and API docs updated) as a gate, add Performance regression checks (e.g., baseline metrics for startup time and message latency) and an Accessibility check (WCAG compliance for new UI components); also replace the vague "关键单测与集成测试" with a concrete definition—either a named list of critical tests or a coverage/threshold policy (e.g., X% coverage on critical modules and a list of key test IDs).
106-111: Expand testing strategy with specific requirements.The current testing strategy is too high-level for a migration of this complexity. Consider adding:
Coverage targets: Specify minimum code coverage (e.g., 80% for new presenter code, 100% for permission logic).
Security testing: Given the permission system is central to MVP, add explicit security test cases:
- Permission boundary violations (projectDir escapes)
- Whitelist bypass attempts
- Race conditions in permission approval flows
Performance testing: Test session/message operations at scale (e.g., 1000+ messages, concurrent sessions).
UI/E2E testing: Specify the framework (Playwright? Cypress?) and critical user flows to test end-to-end.
Backward compatibility: Specific test cases for migrating from each legacy schema version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/agentpresenter-mvp-replacement/plan.md` around lines 106 - 111, Update the "测试策略" section to add concrete requirements: define coverage targets (e.g., minimum 80% for new presenter code and 100% for permission logic), add explicit security tests (cases for projectDir escape, whitelist bypass, and race conditions in permission approval flows), add performance tests for session/message scale (e.g., 1000+ messages, concurrent sessions), specify an E2E framework (Playwright or Cypress) and enumerate critical UI flows to test, and list backward-compatibility migration test cases for each legacy schema version; reference the existing items "Main 单测", "集成测试", and "迁移回归" to extend rather than replace them.
17-35: Clarify permission approval UX and audit logging.The permission model logic is clear, but two operational aspects need documentation:
User approval flow: How are permission requests surfaced in the UI when in
defaultmode?
- Modal dialog? Inline banner? Chat message?
- What happens if the user doesn't respond? (timeout behavior)
- Can users batch-approve multiple requests?
Audit logging: For compliance and debugging, document whether permission decisions (approve/deny/auto-grant) are logged with:
- Timestamp
- Session/message context
- Tool name and requested path
- Decision outcome and reason
Line 34 clarification: In
defaultmode, ifprojectDiris set, are out-of-bounds requests auto-rejected or do they still go through whitelist matching? The order suggests boundary check happens first, but the description of default mode focuses on whitelist matching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/agentpresenter-mvp-replacement/plan.md` around lines 17 - 35, Update the design doc to explicitly define the user approval UX and audit logging for the `permission_mode` (values `default | full`) introduced on `new_sessions`: specify that in `default` mode permission requests are surfaced via a modal dialog (with an optional inline banner fallback) that shows session/message context, tool name and requested path, supports batch-approve/deny for multiple pending requests, and auto-times out after a configurable period with a clear fallback (e.g., auto-deny or escalate); document the exact audit log schema to record every permission decision (timestamp, sessionId, message/context, toolName, requested path, decision outcome, and reason) and where logs are stored; and clarify the rule referenced on Line 34 by stating that for `default` mode the `projectDir` boundary check (session.projectDir) is applied first and any out-of-bounds requests are automatically rejected before whitelist (`sessionId + toolName + pathPattern`) matching is attempted.docs/specs/agentpresenter-mvp-replacement/spec.md (2)
63-67: Clarify "context pollution" in retry/regenerate behavior.Line 67 states "避免被替代消息污染后续生成" but "污染" (pollution) is somewhat vague. Consider specifying what constitutes pollution—for example, whether it refers to:
- Including deleted/replaced messages in the context window
- Mixing variants into the prompt history
- Leaking state from previous generation attempts
More explicit criteria would help ensure consistent implementation across the presenter layer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/agentpresenter-mvp-replacement/spec.md` around lines 63 - 67, Clarify the vague term "避免被替代消息污染后续生成" under the "D. Retry/Regenerate（无 variants）" section by replacing it with explicit criteria: state that replaced/deleted messages must not be included in the context window for subsequent generations, variants or ephemeral candidate messages must never be merged into the persistent prompt history, and any transient state (e.g., retry attempt metadata, generation tokens, or temporary choice flags) must not be carried over to later requests; reference the "Retry/Regenerate" behavior and the rule "每次 retry/regenerate 追加新的 assistant 消息" to enforce that each retry appends a single new assistant message while the canonical conversation history remains free of removed or variant content.
86-89: Document or reference the migration strategy for old chat data.Line 89 mentions "旧 chat 数据有明确兼容迁移策略（静默升级或等价兼容）" but doesn't provide details. For an MVP that replaces core functionality, the migration strategy is critical and should either:
- Be detailed in this spec (what gets migrated, how, when, rollback plan)
- Reference a separate migration design document
- Clarify if migration is out of scope for this MVP phase
Without this clarity, teams may implement incompatible approaches or overlook migration entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/agentpresenter-mvp-replacement/spec.md` around lines 86 - 89, Update section "H. chat 模式清理" to explicitly specify the migration strategy for old chat data: state whether migration is in-scope for the MVP or reference a separate migration design doc, and if in-scope describe what fields/objects are migrated, the transformation rules or data mappings, migration trigger/timing (online vs. batch), rollback/verification steps, monitoring and error handling, and any compatibility guarantees; refer to the "H. chat 模式清理" heading when adding these details or include a clear link/reference to the separate migration document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/agentpresenter-mvp-replacement/plan.md`:
- Around line 36-61: Clarify identifiers and error flows: explicitly state
whether conversationId is a direct alias of sessionId or requires
transformation/validation (reference conversationId and sessionId), and add
validation rules and error codes when they differ; document workspace binding
error handling for session.projectDir: define behavior when missing (fail fast
with specific error, prompt user to bind, or queue operations) and specify
retry/backoff and logging policies; for forkSessionFromMessage, enumerate
exactly what is copied into the new session (e.g., session.settings,
permissionMode, projectDir, whitelistedRules, conversationId mapping policy,
auth tokens, and any ephemeral state) and describe which items are
shallow-copied vs deep-copied and how inherited permissions/approvals are
enforced or re-evaluated.
- Around line 62-93: Expand the phased plan to include explicit migration and
safety details: in Phase 0/1 add a "Data migration" subsection describing how to
migrate old_sessions/chat_sessions/legacy messages into new_sessions and
deepchat_sessions/deepchat_messages (migration scripts, id mapping, idempotency,
schema transforms and verification), add a "Rollback & versioning" subsection
specifying DB schema versioning, backups, restore steps, and feature flags to
toggle between old/new stacks (e.g., feature flag gating newAgentPresenter and
ChatStatusBar flows), add a "Gradual rollout" subsection describing staged user
cohorts (canary/beta → 10% → 50% → all) and monitoring/metrics to abort, and
clarify Phase dependencies (call out that Phase 2 depends on Phase 1
permissions/Default/Full access implementations and that migration must complete
before removing legacy settings). Ensure each subsection references the related
symbols (newAgentPresenter, ChatStatusBar, new_sessions, deepchat_sessions,
deepchat_messages, old_sessions/chat_sessions) and includes acceptance checks
and automated rollback criteria.
- Around line 94-105: Update the IPC section to require renderer components to
call the new IPC methods through the usePresenter composable (not direct
ipcRenderer calls) and document that the main-side presenter instance
newAgentPresenter will emit EventBus events (e.g., session-updated,
message-added, permission-requested, tool-execution-started) that renderers must
subscribe to; for each INewAgentPresenter method (setSessionPermissionMode,
editUserMessage, retryAssistantMessage, forkSessionFromMessage) add method
signatures including return types and possible error responses, and add the
shared types PermissionMode and PermissionWhitelistRule (with toolName and
pathPattern) to the spec so implementers can wire typed IPC and EventBus
handlers consistently.
- Around line 9-16: Add explicit documentation describing use of EventBus for
main→renderer inter-process events and MCP tools integration: state that
newAgentPresenter will emit session and message-stream events over EventBus
(e.g., SESSION_EVENTS, message streaming events) to the renderer, that
deepchatAgentPresenter will subscribe/consume those EventBus events in its
execution flow, and that MCP tools are wired into the deepchatAgentPresenter via
the two-layer LLM provider architecture (Agent Loop + Provider) so tools are
invoked from the Provider layer during agent execution; also mention using the
Presenter pattern in the main process to own EventBus interactions and tool
orchestration.
In `@docs/specs/agentpresenter-mvp-replacement/spec.md`:
- Around line 42-50: The "Full access" flow currently skips validating
operations against session.projectDir; update the permission logic so
auto-approval only permits paths inside session.projectDir by normalizing paths,
resolving symlinks, and performing a strict boundary check before any tool
executes (update the permission handler and FilePermissionService to validate
using resolved absolute paths); implement rejection responses for out-of-bounds
requests in each tool handler that currently has TODOs (e.g., imageServer.ts)
and add unit/integration tests per Task T4 covering path traversal, relative
escapes, and symlink scenarios to ensure boundary enforcement and correct
rejection behavior.
---
Nitpick comments:
In `@docs/specs/agentpresenter-mvp-replacement/plan.md`:
- Around line 112-117: The "8. 风险与缓解" section currently lists three risks but
omits several important migration and operational risks; update that section
(heading "8. 风险与缓解") by adding entries for Performance degradation, Data loss,
Concurrent operation conflicts, Schema migration failures, and User workflow
disruption, and for each provide a concise mitigation (e.g.,
benchmarking/monitoring for performance, backups/atomic transactions/rollback
for data loss, per-session locking or operation queueing for concurrent
conflicts, migration verification/graceful degradation/error reporting for
schema issues, and user communication/migration guide/feedback channels for
workflow disruption) so the plan includes these additional risks and
corresponding mitigations.
- Around line 118-124: Update section "9. 质量门槛" to add concrete
production-readiness gates: include a build verification step (`pnpm run build`)
that must pass without warnings, require a per-phase Manual QA checklist (link
or embed checklist template), mandate Documentation tasks (README, architecture
and API docs updated) as a gate, add Performance regression checks (e.g.,
baseline metrics for startup time and message latency) and an Accessibility
check (WCAG compliance for new UI components); also replace the vague
"关键单测与集成测试" with a concrete definition—either a named list of critical tests or
a coverage/threshold policy (e.g., X% coverage on critical modules and a list of
key test IDs).
- Around line 106-111: Update the "测试策略" section to add concrete requirements:
define coverage targets (e.g., minimum 80% for new presenter code and 100% for
permission logic), add explicit security tests (cases for projectDir escape,
whitelist bypass, and race conditions in permission approval flows), add
performance tests for session/message scale (e.g., 1000+ messages, concurrent
sessions), specify an E2E framework (Playwright or Cypress) and enumerate
critical UI flows to test, and list backward-compatibility migration test cases
for each legacy schema version; reference the existing items "Main 单测", "集成测试",
and "迁移回归" to extend rather than replace them.
- Around line 17-35: Update the design doc to explicitly define the user
approval UX and audit logging for the `permission_mode` (values `default |
full`) introduced on `new_sessions`: specify that in `default` mode permission
requests are surfaced via a modal dialog (with an optional inline banner
fallback) that shows session/message context, tool name and requested path,
supports batch-approve/deny for multiple pending requests, and auto-times out
after a configurable period with a clear fallback (e.g., auto-deny or escalate);
document the exact audit log schema to record every permission decision
(timestamp, sessionId, message/context, toolName, requested path, decision
outcome, and reason) and where logs are stored; and clarify the rule referenced
on Line 34 by stating that for `default` mode the `projectDir` boundary check
(session.projectDir) is applied first and any out-of-bounds requests are
automatically rejected before whitelist (`sessionId + toolName + pathPattern`)
matching is attempted.
In `@docs/specs/agentpresenter-mvp-replacement/spec.md`:
- Around line 63-67: Clarify the vague term "避免被替代消息污染后续生成" under the "D.
Retry/Regenerate（无 variants）" section by replacing it with explicit criteria:
state that replaced/deleted messages must not be included in the context window
for subsequent generations, variants or ephemeral candidate messages must never
be merged into the persistent prompt history, and any transient state (e.g.,
retry attempt metadata, generation tokens, or temporary choice flags) must not
be carried over to later requests; reference the "Retry/Regenerate" behavior and
the rule "每次 retry/regenerate 追加新的 assistant 消息" to enforce that each retry
appends a single new assistant message while the canonical conversation history
remains free of removed or variant content.
- Around line 86-89: Update section "H. chat 模式清理" to explicitly specify the
migration strategy for old chat data: state whether migration is in-scope for
the MVP or reference a separate migration design doc, and if in-scope describe
what fields/objects are migrated, the transformation rules or data mappings,
migration trigger/timing (online vs. batch), rollback/verification steps,
monitoring and error handling, and any compatibility guarantees; refer to the
"H. chat 模式清理" heading when adding these details or include a clear
link/reference to the separate migration document.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/specs/agentpresenter-mvp-replacement/plan.mddocs/specs/agentpresenter-mvp-replacement/spec.mddocs/specs/agentpresenter-mvp-replacement/tasks.md
✅ Files skipped from review due to trivial changes (1)
- docs/specs/agentpresenter-mvp-replacement/tasks.md
| ## 2. 核心架构决策 | ||
|
|
||
| 1. 会话真源：`new_sessions + deepchat_sessions`。 | ||
| 2. 消息真源：`deepchat_messages`。 | ||
| 3. 主执行链路：`newAgentPresenter -> deepchatAgentPresenter`。 | ||
| 4. 新 UI 页面不再依赖 `useChatStore` 与旧 `sessionPresenter` 主流程。 | ||
| 5. variants 本轮下线，fork 保留为唯一分叉能力。 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document EventBus for inter-process events and MCP tools integration.
The core architecture section defines the execution chain but doesn't explicitly mention EventBus for communicating events from main process to renderer, or the integration of MCP tools. Based on learnings, the architecture should use EventBus for inter-process events and integrate MCP tools as part of the two-layer provider architecture.
Consider adding:
- How
newAgentPresenter→ renderer communication will use EventBus (e.g., SESSION_EVENTS, message streaming events) - How MCP tools integrate into the
deepchatAgentPresenterexecution flow
Based on learnings: Use Presenter pattern in main process, EventBus for inter-process events, and two-layer LLM provider architecture (Agent Loop + Provider) with integrated MCP tools.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/specs/agentpresenter-mvp-replacement/plan.md` around lines 9 - 16, Add
explicit documentation describing use of EventBus for main→renderer
inter-process events and MCP tools integration: state that newAgentPresenter
will emit session and message-stream events over EventBus (e.g., SESSION_EVENTS,
message streaming events) to the renderer, that deepchatAgentPresenter will
subscribe/consume those EventBus events in its execution flow, and that MCP
tools are wired into the deepchatAgentPresenter via the two-layer LLM provider
architecture (Agent Loop + Provider) so tools are invoked from the Provider
layer during agent execution; also mention using the Presenter pattern in the
main process to own EventBus interactions and tool orchestration.
| ## 4. 关键能力设计 | ||
|
|
||
| ### 4.1 Workspace 绑定 | ||
|
|
||
| 1. 工具执行上下文绑定 `session.projectDir`。 | ||
| 2. 工具链路统一传递 `conversationId = sessionId`。 | ||
| 3. 权限消息、审批、执行回执必须在同一 `sessionId` 闭环。 | ||
|
|
||
| ### 4.2 编辑历史 user 消息 | ||
|
|
||
| 1. API：`editUserMessage(sessionId, messageId, newContent)`。 | ||
| 2. 行为：编辑后截断目标消息后的全部消息。 | ||
| 3. 随后自动 regenerate，生成新的 assistant 消息。 | ||
|
|
||
| ### 4.3 Retry/Regenerate（无 variants） | ||
|
|
||
| 1. API：`retryAssistantMessage(sessionId, messageId)`。 | ||
| 2. 行为：不创建 variants；仅追加 assistant 消息。 | ||
| 3. 使用消息边界标记避免旧分支内容污染上下文。 | ||
|
|
||
| ### 4.4 Fork | ||
|
|
||
| 1. API：`forkSessionFromMessage(sessionId, messageId)`。 | ||
| 2. 切点定义：从“当前 assistant 消息（含它）”截取。 | ||
| 3. 新 session 继承必要上下文后可继续对话。 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify error handling and context inheritance details.
A few implementation details could prevent confusion:
-
Line 42: The statement "conversationId = sessionId" equates these two identifiers, but it's unclear if this is a direct mapping or if there's any transformation/validation logic.
-
Workspace binding error handling: When tools require
session.projectDirbut it's missing, the error handling flow isn't documented. Should operations fail immediately, prompt for binding, or queue until workspace is set? -
Fork context inheritance (line 60): "继承必要上下文" (inherit necessary context) is vague. Specify what gets copied: session settings, permission mode, projectDir, whitelisted rules, etc.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/specs/agentpresenter-mvp-replacement/plan.md` around lines 36 - 61,
Clarify identifiers and error flows: explicitly state whether conversationId is
a direct alias of sessionId or requires transformation/validation (reference
conversationId and sessionId), and add validation rules and error codes when
they differ; document workspace binding error handling for session.projectDir:
define behavior when missing (fail fast with specific error, prompt user to
bind, or queue operations) and specify retry/backoff and logging policies; for
forkSessionFromMessage, enumerate exactly what is copied into the new session
(e.g., session.settings, permissionMode, projectDir, whitelistedRules,
conversationId mapping policy, auth tokens, and any ephemeral state) and
describe which items are shallow-copied vs deep-copied and how inherited
permissions/approvals are enforced or re-evaluated.
| ## 5. 分阶段迁移 | ||
|
|
||
| ### Phase 0：稳定主链路 | ||
|
|
||
| 1. 清理新 UI 对 `useChatStore` 的依赖点。 | ||
| 2. active session 查询与事件分发统一到 `newAgentPresenter`。 | ||
| 3. 建立最小回归测试基线。 | ||
|
|
||
| ### Phase 1：权限 + Workspace（MVP 核心） | ||
|
|
||
| 1. 打通 `ChatStatusBar` 权限模式选择到 session 持久化。 | ||
| 2. 实现 `Default` 权限审批与白名单命中。 | ||
| 3. 实现 `Full access` 自动放行 + `projectDir` 越界拒绝。 | ||
|
|
||
| ### Phase 2：消息操作（MVP 核心） | ||
|
|
||
| 1. 实现历史 user 消息编辑（截断+再生成）。 | ||
| 2. 实现 retry/regenerate 追加 assistant（无 variants）。 | ||
| 3. 实现 fork（含当前 assistant）。 | ||
|
|
||
| ### Phase 3：设置收敛 | ||
|
|
||
| 1. conversation settings UI 与逻辑下线。 | ||
| 2. agent 默认配置在 session 中落地。 | ||
| 3. 清理 legacy settings 读取/写入路径。 | ||
|
|
||
| ### Phase 4：chat 模式清理 | ||
|
|
||
| 1. 类型层移除 `chat`。 | ||
| 2. UI 与 presenter 中移除 chat 分支。 | ||
| 3. 完成兼容迁移后删除残留代码。 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add migration safety mechanisms and data handling details.
The phased approach is well-structured, but several implementation-critical details are missing:
-
Data migration: How will existing
old_sessions,chat_sessions, or legacy message data be migrated tonew_sessionsanddeepchat_sessions/deepchat_messages? This should be explicit in Phase 0 or 1. -
Rollback strategy: If a phase introduces critical bugs, how do you roll back? Consider documenting:
- Database schema versioning
- Feature flags to toggle between old/new stacks
- Data backup/restore procedures
-
Gradual rollout: Consider a staged user rollout (e.g., beta users first) rather than all-or-nothing deployment.
-
Phase dependencies: Make explicit dependencies clearer. For example, "Phase 2 requires Phase 1 permission system completion because edit/retry/fork operations must respect permission boundaries."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/specs/agentpresenter-mvp-replacement/plan.md` around lines 62 - 93,
Expand the phased plan to include explicit migration and safety details: in
Phase 0/1 add a "Data migration" subsection describing how to migrate
old_sessions/chat_sessions/legacy messages into new_sessions and
deepchat_sessions/deepchat_messages (migration scripts, id mapping, idempotency,
schema transforms and verification), add a "Rollback & versioning" subsection
specifying DB schema versioning, backups, restore steps, and feature flags to
toggle between old/new stacks (e.g., feature flag gating newAgentPresenter and
ChatStatusBar flows), add a "Gradual rollout" subsection describing staged user
cohorts (canary/beta → 10% → 50% → all) and monitoring/metrics to abort, and
clarify Phase dependencies (call out that Phase 2 depends on Phase 1
permissions/Default/Full access implementations and that migration must complete
before removing legacy settings). Ensure each subsection references the related
symbols (newAgentPresenter, ChatStatusBar, new_sessions, deepchat_sessions,
deepchat_messages, old_sessions/chat_sessions) and includes acceptance checks
and automated rollback criteria.
| ## 6. IPC 与类型面 | ||
|
|
||
| 1. `INewAgentPresenter` 扩展： | ||
| - `setSessionPermissionMode` | ||
| - `editUserMessage` | ||
| - `retryAssistantMessage` | ||
| - `forkSessionFromMessage` | ||
| 2. shared types 补充： | ||
| - `PermissionMode` | ||
| - `PermissionWhitelistRule`（含 `toolName` 与 `pathPattern`） | ||
| 3. preload 暴露新增方法，保持 typed IPC。 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document renderer-to-main communication pattern and event types.
The IPC section lists new methods but misses two critical aspects:
-
Renderer → Main communication: The document doesn't explicitly state that renderer components should call these IPC methods via the
usePresentercomposable pattern (not direct IPC calls). This is important for maintainability and consistency. -
Main → Renderer events: The document doesn't list the event types that
newAgentPresenterwill emit via EventBus (e.g., session-updated, message-added, permission-requested, tool-execution-started). These events are necessary for the renderer to react to state changes. -
Method signatures: Consider documenting the return types and error responses for each IPC method to aid implementation.
Based on learnings: Use usePresenter composable for main process communication instead of direct IPC calls, and Main sends to Renderer via EventBus.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/specs/agentpresenter-mvp-replacement/plan.md` around lines 94 - 105,
Update the IPC section to require renderer components to call the new IPC
methods through the usePresenter composable (not direct ipcRenderer calls) and
document that the main-side presenter instance newAgentPresenter will emit
EventBus events (e.g., session-updated, message-added, permission-requested,
tool-execution-started) that renderers must subscribe to; for each
INewAgentPresenter method (setSessionPermissionMode, editUserMessage,
retryAssistantMessage, forkSessionFromMessage) add method signatures including
return types and possible error responses, and add the shared types
PermissionMode and PermissionWhitelistRule (with toolName and pathPattern) to
the spec so implementers can wire typed IPC and EventBus handlers consistently.
| ### A. 权限模式 | ||
|
|
||
| - [ ] `ChatStatusBar` 可选择 `Default` 与 `Full access`。 | ||
| - [ ] 权限模式持久化在 session 维度。 | ||
| - [ ] 当 `session.projectDir` 为空时，`Full access` 不可选并提示先绑定 workspace。 | ||
| - [ ] `Default` 走权限确认流程，白名单按 `session` 维度隔离。 | ||
| - [ ] `Default` 白名单匹配粒度为 `toolName + pathPattern`。 | ||
| - [ ] `Full access` 自动通过请求，但任何越出 `projectDir` 的操作必须拒绝。 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat docs/specs/agentpresenter-mvp-replacement/spec.mdRepository: ThinkInAIXYZ/deepchat
Length of output: 2195
🏁 Script executed:
# Search for projectDir-related implementation
echo "=== Searching for projectDir implementation ==="
rg "projectDir" --type ts --type tsx --type js -C 2 | head -80
# Search for Full access mode implementation
echo -e "\n=== Searching for Full access mode ==="
rg "Full\s*access|fullAccess|full_access" --type ts --type tsx --type js -C 2 | head -60
# Search for permission validation tests
echo -e "\n=== Searching for permission/security tests ==="
find . -type f \( -name "*.test.ts" -o -name "*.test.tsx" -o -name "*.spec.ts" \) | grep -i "permission\|access\|security\|projectDir" | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 544
🏁 Script executed:
# Search more broadly for permission/access related files
echo "=== Searching for permission-related implementation files ==="
find . -type f \( -name "*permission*.ts" -o -name "*permission*.js" \) | grep -v node_modules | grep -v dist | head -30
echo -e "\n=== Searching for access control implementation ==="
find . -type f \( -name "*access*.ts" -o -name "*access*.js" \) | grep -v node_modules | grep -v dist | head -20
# Look at the permission test files to understand what's being tested
echo -e "\n=== Content of agentPresenter permission test ==="
cat ./test/main/presenter/agentPresenter/permission/permissionHandler.resume.test.ts | head -100
echo -e "\n=== Checking commandPermissionService test ==="
head -80 ./test/main/presenter/permission/commandPermissionService.test.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 6305
🏁 Script executed:
# Read the permissionHandler implementation
echo "=== Content of permissionHandler.ts ==="
cat ./src/main/presenter/agentPresenter/permission/permissionHandler.ts
echo -e "\n=== Search for projectDir in permission-related files ==="
rg "projectDir" ./src/main/presenter --type ts -A 3 -B 3
echo -e "\n=== Search for path validation logic ==="
rg "path.*traversal|normalize.*path|resolve.*path|\.\./" ./src/main/presenter --type ts -B 2 -A 2 | head -100Repository: ThinkInAIXYZ/deepchat
Length of output: 50377
🏁 Script executed:
# Search more specifically for projectDir boundary enforcement
echo "=== Searching for projectDir boundary validation ==="
rg "projectDir.*reject|reject.*projectDir|outside.*projectDir|boundary" --type ts -B 2 -A 2
# Search for path normalization and validation
echo -e "\n=== Searching for path validation in tool execution ==="
rg "Full\s*access|fullAccess|path.*valid|sanitize.*path" --type ts -B 2 -A 2 | head -80
# Search for tool execution context setup where projectDir is passed
echo -e "\n=== Searching for tool execution with projectDir context ==="
rg "callTool|executeTool" ./src/main/presenter --type ts -B 3 -A 3 | head -100
# Check what's in the constraint section of the spec
echo -e "\n=== Looking at constraint section in spec ==="
tail -30 docs/specs/agentpresenter-mvp-replacement/spec.mdRepository: ThinkInAIXYZ/deepchat
Length of output: 18229
🏁 Script executed:
# Check if there's explicit projectDir validation in Full access approval
echo "=== Checking if Full access mode checks projectDir ==="
rg "Full.*access|fullAccess|approveAllAccess" ./src/main/presenter --type ts -B 3 -A 3 | head -60
# Look at how filePermissionService approves files - does it check projectDir?
echo -e "\n=== Checking filePermissionService implementation ==="
fd "filePermissionService" ./src/main/presenter --type ts | head -5
# Check the ACP filesystem handler for projectDir validation
echo -e "\n=== Checking acpFsHandler projectDir validation ==="
cat ./src/main/presenter/agentPresenter/acp/acpFsHandler.ts | head -150Repository: ThinkInAIXYZ/deepchat
Length of output: 4537
🏁 Script executed:
# Search for where Full access mode is implemented/checked
echo "=== Searching for Full access mode handling ==="
rg "Full\s*access|fullAccess|full_access|mode.*=.*full|Full.*Access" ./src/main/presenter --max-count 20 -B 2 -A 2
# Search for where AcpFsHandler is instantiated to see if projectDir is properly passed
echo -e "\n=== Searching for AcpFsHandler instantiation ==="
rg "new\s+AcpFsHandler|AcpFsHandler\(" ./src/main/presenter -B 3 -A 5 | head -80
# Check the agent tool manager to see if it enforces projectDir
echo -e "\n=== Looking for agentToolManager and projectDir enforcement ==="
find ./src/main/presenter -name "agentToolManager*" -o -name "*AgentTool*" | grep -v node_modules | head -10Repository: ThinkInAIXYZ/deepchat
Length of output: 1969
🏁 Script executed:
# Examine the agentToolManager to understand how tools are executed
echo "=== Checking agentToolManager.ts ==="
head -150 ./src/main/presenter/agentPresenter/acp/agentToolManager.ts
# Search for where permission modes (Default vs Full access) are actually enforced
echo -e "\n=== Searching for permission mode enforcement logic ==="
rg "permission.*mode|permissionMode|mode.*permission|Default.*Full|Full.*Default" ./src/main/presenter -B 2 -A 3 | head -100
# Check if there are any tests that verify projectDir boundary
echo -e "\n=== Searching for projectDir tests ==="
rg "projectDir.*test|test.*projectDir" ./test/main -B 2 -A 2 | head -80Repository: ThinkInAIXYZ/deepchat
Length of output: 5988
🏁 Script executed:
# Search for session permission mode or access level settings
echo "=== Searching for permission/access level in session ==="
rg "accessLevel|permissionLevel|permission.*mode|mode.*default|mode.*full" ./src/main/presenter -i -B 2 -A 2 | head -100
# Check filePermissionService to understand the approval mechanism
echo -e "\n=== Searching for filePermissionService ==="
find ./src/main/presenter -name "*filePermission*" -type f
# Look at the sessionPresenter permission model
echo -e "\n=== Checking newSessionsTable schema ==="
grep -A 20 "project_dir" ./src/main/presenter/sqlitePresenter/tables/newSessions.ts | head -40
# Check if there's any documentation or comments about Full access enforcement
echo -e "\n=== Searching for Full access comments/docs ==="
rg "Full access|full.*access" docs/ -i -B 2 -A 2 | head -80Repository: ThinkInAIXYZ/deepchat
Length of output: 14001
🏁 Script executed:
# Final check: Look at the task tracking to see status
echo "=== Checking task status for Full access boundary ==="
grep -A 5 "T4 Full access" docs/specs/agentpresenter-mvp-replacement/tasks.md
# Check filePermissionService for any projectDir enforcement
echo -e "\n=== Examining filePermissionService.ts ==="
head -100 ./src/main/presenter/permission/filePermissionService.ts
# Verify if there are any test files specifically for Full access projectDir enforcement
echo -e "\n=== Checking tests for Full access projectDir ==="
find ./test -name "*.test.ts" -exec grep -l "Full.*access\|projectDir.*Full\|access.*boundary" {} \;Repository: ThinkInAIXYZ/deepchat
Length of output: 1909
Implement and test projectDir boundary enforcement for "Full access" mode.
Per the spec (lines 49-50 and constraint section), "Full access" must reject all operations outside session.projectDir. However, Task T4 shows this critical requirement remains unimplemented:
- Auto-approval logic restricted to projectDir operations only
- Path normalization and boundary detection
- Rejection response for out-of-bounds requests
- Comprehensive tests covering path traversal, symlinks, and relative path escapes
Current implementation gaps:
- Permission handler grants "Full access" without projectDir checks
FilePermissionServicetracks approvals but doesn't validate against projectDir- Multiple tools (e.g.,
imageServer.ts) have unimplemented path validation TODOs - No tests for boundary violation scenarios
Ensure all tool handlers enforce the projectDir boundary before executing operations, with special attention to symlink resolution and path normalization to prevent traversal attacks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/specs/agentpresenter-mvp-replacement/spec.md` around lines 42 - 50, The
"Full access" flow currently skips validating operations against
session.projectDir; update the permission logic so auto-approval only permits
paths inside session.projectDir by normalizing paths, resolving symlinks, and
performing a strict boundary check before any tool executes (update the
permission handler and FilePermissionService to validate using resolved absolute
paths); implement rejection responses for out-of-bounds requests in each tool
handler that currently has TODOs (e.g., imageServer.ts) and add unit/integration
tests per Task T4 covering path traversal, relative escapes, and symlink
scenarios to ensure boundary enforcement and correct rejection behavior.
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests
Chores