Skip to content

Conversation

@zerob13
Copy link
Collaborator

@zerob13 zerob13 commented Oct 29, 2025

Summary

  • surface MCP permission requests with full approval context so the renderer shows the approval UI
  • mark granted and denied permissions correctly to update block status and hide the action buttons

Testing

  • pnpm run typecheck

https://chatgpt.com/codex/tasks/task_e_6901e3a46178832c81e9b8b20b870860

Summary by CodeRabbit

  • New Features

    • Enhanced permission workflow: users can grant or deny tool-call permissions with clearer statuses and optional "remember" behavior; pending tool calls can resume after permission is granted.
    • Post-tool execution context: conversations continue smoothly after tool runs or permission outcomes so assistant responses remain coherent.
  • Bug Fixes / Improvements

    • Better UI/state synchronization for permission flows, including continued generation after denial with error context and more reliable recovery from interrupted tool calls.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2025

Walkthrough

ThreadPresenter and promptBuilder were extended to manage MCP tool permission flows: creating permission blocks with extra metadata, handling grant/deny responses, synchronizing generating state (including pending tool calls), resuming or continuing agent generation, and adding post-tool-execution context construction utilities.

Changes

Cohort / File(s) Summary
Presenter: permission flow & resume logic
src/main/presenter/threadPresenter/index.ts
Added MCP imports and handling for tool_call permission-required flows; construct tool_call_permission blocks with extra (needsUserAction, permissionType, serverName, toolName, permissionRequest); introduced methods handlePermissionResponse, restartAgentLoopAfterPermission, finalizeMessageAfterPermissionDenied/continueAfterPermissionDenied, resumeStreamCompletion, resumeAfterPermissionWithPendingToolCall, waitForMcpServiceReady, findPendingToolCallAfterPermission; manage state.generating.pendingToolCall; update block statuses (success → initial, granted, denied); propagate extra and tool_call fields and synchronize memory-state after permission events.
Prompt builder: post-tool execution context & types
src/main/presenter/threadPresenter/promptBuilder.ts
Exported PendingToolCall extended with optional serverName, serverIcons, serverDescription; added PostToolExecutionContextParams and buildPostToolExecutionContext to reconstruct assistant/user messages after tool execution; added internal helper collectAssistantTextBeforePermission; imported AssistantMessage.
Types: generating state
src/main/presenter/threadPresenter/types.ts
GeneratingMessageState augmented with optional pendingToolCall?: PendingToolCall.
Renderer: permission UI text mapping
src/renderer/src/components/message/MessageBlockPermissionRequest.vue
getStatusText now treats status 'success' same as 'granted' for displayed text (maps both to granted text).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Presenter as ThreadPresenter
    participant State as GeneratingState
    participant MCP as MCP Service
    participant Tools as Tool Executor

    User->>Presenter: Agent emits tool_call requiring permission
    Presenter->>State: append permission block (extra: needsUserAction, permissionType, serverName, toolName, permissionRequest)
    Presenter->>User: emit permission-request UI

    alt User grants
        User->>Presenter: handlePermissionResponse(messageId, toolCallId, granted=true,...)
        Presenter->>State: update block status = "granted", clear needsUserAction, persist grantedPermissions
        Presenter->>MCP: waitForMcpServiceReady(serverName)
        Presenter->>Presenter: restartAgentLoopAfterPermission(messageId)
        Presenter->>Tools: load/execute tool (post-permission)
        Tools->>Presenter: emit tool_call event + response
        Presenter->>Presenter: resumeStreamCompletion(conversationId,messageId)
    else User denies
        User->>Presenter: handlePermissionResponse(messageId, toolCallId, granted=false,...)
        Presenter->>State: update block status = "denied", clear needsUserAction, clear pendingToolCall
        Presenter->>Presenter: continueAfterPermissionDenied(messageId)
        Presenter->>State: continue generation with denial/error context (buildPostToolExecutionContext)
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas to focus on:
    • Lifecycle and persistence of state.generating.pendingToolCall
    • Permission block status transitions (successgranted/denied) and UI mapping
    • Restart/resume logic (MCP readiness polling, tool definition loading, tool execution and emitted events)
    • Correctness and ordering in buildPostToolExecutionContext
    • Edge-case guards around missing tool definitions, permission blocks, and model config

Possibly related PRs

  • Feature/add permission check #594 — Implements MCP tool permission request/response flow and ThreadPresenter permission handlers; strongly related to the permission handling and resume logic here.
  • Feature/mcp tool improve #635 — Modifies ThreadPresenter MCP tool handling and tool definition usage; related to resumed tool execution and enabled tool propagation after permissions.

Suggested reviewers

  • deepinfect

Poem

🐰 I paused the tools and tapped my paw,
A tiny permission to open the drawer.
Grant flips the switch, deny nudges the thread,
I stitch messages back so the flow stays well-bred.
Hooray — tools resume, and carrots are fed! 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "fix: restore MCP permission approval flow" directly and accurately describes the primary focus of the changeset. The changes comprehensively implement the MCP permission approval workflow by adding permission state handling (granted/denied), new methods to process permission responses (handlePermissionResponse, restartAgentLoopAfterPermission, etc.), state synchronization, and renderer updates. This aligns perfectly with the stated objectives of surfacing MCP permission requests and marking granted/denied permissions correctly. The title is concise, specific, and clearly conveys the main change without being misleading or overly broad.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codex/mcp

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9cbdae0 and 6409add.

📒 Files selected for processing (1)
  • src/main/presenter/threadPresenter/index.ts (13 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)

**/*.{js,jsx,ts,tsx}: 使用 OxLint 进行代码检查
Log和注释使用英文书写

Files:

  • src/main/presenter/threadPresenter/index.ts
src/{main,renderer}/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)

src/{main,renderer}/**/*.ts: Use context isolation for improved security
Implement proper inter-process communication (IPC) patterns
Optimize application startup time with lazy loading
Implement proper error handling and logging for debugging

Files:

  • src/main/presenter/threadPresenter/index.ts
src/main/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)

Use Electron's built-in APIs for file system and native dialogs

Files:

  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/error-logging.mdc)

**/*.{ts,tsx}: 始终使用 try-catch 处理可能的错误
提供有意义的错误信息
记录详细的错误日志
优雅降级处理
日志应包含时间戳、日志级别、错误代码、错误描述、堆栈跟踪(如适用)、相关上下文信息
日志级别应包括 ERROR、WARN、INFO、DEBUG
不要吞掉错误
提供用户友好的错误信息
实现错误重试机制
避免记录敏感信息
使用结构化日志
设置适当的日志级别

Files:

  • src/main/presenter/threadPresenter/index.ts
src/main/**/*.{ts,js,tsx,jsx}

📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)

主进程代码放在 src/main

Files:

  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx,js,vue}

📄 CodeRabbit inference engine (CLAUDE.md)

Use English for all logs and comments

Files:

  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx,vue}

📄 CodeRabbit inference engine (CLAUDE.md)

Enable and adhere to strict TypeScript typing (avoid implicit any, prefer precise types)

Use PascalCase for TypeScript types and classes

Files:

  • src/main/presenter/threadPresenter/index.ts
src/main/presenter/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Place Electron main-process presenters under src/main/presenter/ (Window, Tab, Thread, Mcp, Config, LLMProvider)

Files:

  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx,js,jsx,vue,css,scss,md,json,yml,yaml}

📄 CodeRabbit inference engine (AGENTS.md)

Prettier style: single quotes, no semicolons, print width 100; run pnpm run format

Files:

  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx,js,jsx,vue}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx,vue}: Use OxLint for JS/TS code; keep lint clean
Use camelCase for variables and functions
Use SCREAMING_SNAKE_CASE for constants

Files:

  • src/main/presenter/threadPresenter/index.ts
🧠 Learnings (3)
📚 Learning: 2025-09-06T03:07:23.817Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-06T03:07:23.817Z
Learning: Applies to src/main/presenter/mcpPresenter/index.ts : Register new MCP tools in src/main/presenter/mcpPresenter/index.ts

Applied to files:

  • src/main/presenter/threadPresenter/index.ts
📚 Learning: 2025-09-06T03:07:23.817Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-06T03:07:23.817Z
Learning: Applies to src/main/presenter/mcpPresenter/inMemoryServers/*.ts : Implement new MCP tools under src/main/presenter/mcpPresenter/inMemoryServers/

Applied to files:

  • src/main/presenter/threadPresenter/index.ts
📚 Learning: 2025-07-21T01:46:52.880Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: .cursor/rules/llm-agent-loop.mdc:0-0
Timestamp: 2025-07-21T01:46:52.880Z
Learning: Applies to src/main/presenter/llmProviderPresenter/index.ts : `src/main/presenter/llmProviderPresenter/index.ts` should manage the overall Agent loop, conversation history, tool execution via `McpPresenter`, and frontend communication via `eventBus`.

Applied to files:

  • src/main/presenter/threadPresenter/index.ts
🧬 Code graph analysis (1)
src/main/presenter/threadPresenter/index.ts (8)
src/shared/chat.d.ts (2)
  • AssistantMessage (39-42)
  • AssistantMessageBlock (84-138)
src/main/eventbus.ts (1)
  • eventBus (151-151)
src/main/events.ts (1)
  • STREAM_EVENTS (59-63)
src/main/presenter/threadPresenter/promptBuilder.ts (1)
  • buildPostToolExecutionContext (234-315)
src/main/presenter/threadPresenter/types.ts (1)
  • GeneratingMessageState (4-36)
src/shared/types/presenters/legacy.presenters.d.ts (2)
  • MCPToolDefinition (1146-1162)
  • MCPToolResponse (1178-1207)
src/shared/types/core/mcp.ts (2)
  • MCPToolDefinition (3-19)
  • MCPToolResponse (58-71)
src/main/presenter/index.ts (1)
  • presenter (223-223)
🔇 Additional comments (7)
src/main/presenter/threadPresenter/index.ts (7)

14-14: LGTM! New imports support permission flow.

The added imports for MCPToolDefinition and buildPostToolExecutionContext are properly used in the new permission handling methods.

Also applies to: 39-43


392-413: Excellent security fix! Least-privilege default applied.

This validation logic properly addresses the previous review concern about defaulting to 'write'. The implementation now:

  • Defaults to 'read' for safety (least-privilege principle)
  • Validates against an explicit allowlist
  • Logs warnings for invalid or non-string values
  • Normalizes case for robustness

469-498: LGTM! Permission-granted flow is clear.

The status change to 'granted' (instead of generic 'success') provides clearer semantics. The logic properly updates the permission block and stores pending tool call information for subsequent execution.


499-515: LGTM! Permission-denied flow is consistent.

Using 'denied' status instead of 'error' provides better semantic clarity. The state cleanup is appropriate.


2856-2900: Critical fix: prevents permission state from being overwritten.

This synchronization logic addresses a race condition where in-memory generating state could overwrite database permission updates. The implementation:

  • Updates the specific permission block in memory after database write
  • Deep-copies nested objects to avoid reference issues
  • Falls back to full content synchronization if block not found
  • Prevents stale state from corrupting persisted permission decisions

2798-2968: LGTM! Well-coordinated permission response handler.

This method effectively orchestrates the permission response flow:

  • Validates message and permission block existence
  • Updates both database and in-memory state
  • For grants: waits for MCP service readiness before restarting
  • For denials: continues generation with error context (improved UX)
  • Comprehensive error handling and logging throughout

3084-3243: Excellent UX improvement: continues generation after denial.

Instead of finalizing the message when permission is denied, this method allows the LLM to continue generation with the denial as a tool execution failure. This:

  • Provides better user experience by allowing recovery
  • Treats permission denial consistently with other tool failures
  • Uses buildPostToolExecutionContext to properly format the failure context
  • Maintains clean state management throughout

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
src/main/presenter/threadPresenter/index.ts (4)

2837-2842: On grant failure, re‑enable user action and surface a retryable state.

Currently sets status='error' but leaves needsUserAction=false, blocking recovery.

         } catch (permissionError) {
           console.error(`[ThreadPresenter] Failed to grant permission:`, permissionError)
           // 权限授予失败,将状态更新为错误
           permissionBlock.status = 'error'
+          if (permissionBlock.extra) {
+            permissionBlock.extra.needsUserAction = true
+          }
+          permissionBlock.content =
+            'Failed to grant permission. Please retry or check MCP server settings.'
           await this.messageManager.editMessage(messageId, JSON.stringify(content))
           throw permissionError
         }

3021-3028: Preserve context when resuming without in‑memory state.

Fallback calls startStreamCompletion without messageId; pass messageId to ensure the correct tool-call context is reconstructed.

-      await this.startStreamCompletion(conversationId)
+      await this.startStreamCompletion(conversationId, messageId)

1239-1239: Avoid logging entire conversation objects (PII/compliance).

This can leak user data in logs. Log IDs/flags only.

-    console.log('sendMessage', conversation)
+    console.debug('[ThreadPresenter] sendMessage', { conversationId, hasSettings: !!conversation.settings })

1218-1224: Critical: clearContext deletes all messages globally.

Calling sqlitePresenter.deleteAllMessages() has no conversation filtering and wipes the entire messages table. Use messageManager.clearAllMessages(conversationId) instead, which deletes only messages in the specified conversation.

  async clearContext(conversationId: string): Promise<void> {
    await this.sqlitePresenter.runTransaction(async () => {
      const conversation = await this.getConversation(conversationId)
      if (conversation) {
-       await this.sqlitePresenter.deleteAllMessages()
+       await this.messageManager.clearAllMessages(conversationId)
      }
    })
  }
🧹 Nitpick comments (5)
src/main/presenter/threadPresenter/index.ts (5)

388-391: Type the permission extra payload.

Record<string, ...> hides structure and weakens TS safety. Define a PermissionExtra type and use it.

+type PermissionExtra = {
+  needsUserAction: boolean
+  permissionType: 'read' | 'write' | 'all'
+  serverName?: string
+  toolName?: string
+  permissionRequest?: unknown
+  grantedPermissions?: 'read' | 'write' | 'all'
+}
-        const extra: Record<string, string | number | object[] | boolean> = {
+        const extra: PermissionExtra = {
           needsUserAction: true,
           permissionType
         }

Also applies to: 425-427


403-405: Avoid JSON.stringify for permissionRequest; keep sanitized, structured data.

Stringifying drops type safety and may bloat storage. Store a minimal, sanitized object instead.

-        if (permission_request) {
-          extra.permissionRequest = JSON.stringify(permission_request)
-        }
+        if (permission_request) {
+          // keep only non-sensitive fields needed by renderer
+          const { permissionType, scope, reason } = permission_request as any
+          extra.permissionRequest = { permissionType, scope, reason }
+        }

3144-3186: Make waitForMcpServiceReady return boolean and branch on timeout.

Void-return resolves even on timeout; caller can’t decide whether to proceed or warn.

-  private async waitForMcpServiceReady(
-    serverName: string,
-    maxWaitTime: number = 3000
-  ): Promise<void> {
+  private async waitForMcpServiceReady(
+    serverName: string,
+    maxWaitTime: number = 3000
+  ): Promise<boolean> {
...
-    return new Promise((resolve) => {
+    return new Promise((resolve) => {
...
-              resolve()
+              resolve(true)
...
-            resolve() // 超时也继续,避免阻塞
+            resolve(false) // timeout
...
-          resolve() // 出错也继续,避免阻塞
+          resolve(false) // error

And handle at call site:

-          await this.waitForMcpServiceReady(serverName)
+          const ready = await this.waitForMcpServiceReady(serverName)
+          if (!ready) {
+            console.warn(`[ThreadPresenter] MCP service ${serverName} not confirmed ready; continuing optimistically`)
+          }

Also applies to: 2828-2836


77-88: Unify logs/comments to English and adopt structured logging.

Multiple logs/comments are in Chinese and use console.* directly. Guidelines require English logs and structured logging with levels and context.

  • Replace console.* with a logger (INFO/WARN/ERROR/DEBUG, timestamp, context).
  • Convert comments and user-facing log text to English.
    As per coding guidelines

Also applies to: 282-339, 850-857, 1294-1298, 1503-1514, 1591-1600, 1769-1772


3189-3215: Pending tool-call finder: OK; consider including serverName.

Works, but including server_name would help downstream context if needed by promptBuilder.

Return { id, name, params, serverName?: string } for completeness if available.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e425fc1 and 1641cba.

📒 Files selected for processing (1)
  • src/main/presenter/threadPresenter/index.ts (5 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)

**/*.{js,jsx,ts,tsx}: 使用 OxLint 进行代码检查
Log和注释使用英文书写

Files:

  • src/main/presenter/threadPresenter/index.ts
src/{main,renderer}/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)

src/{main,renderer}/**/*.ts: Use context isolation for improved security
Implement proper inter-process communication (IPC) patterns
Optimize application startup time with lazy loading
Implement proper error handling and logging for debugging

Files:

  • src/main/presenter/threadPresenter/index.ts
src/main/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)

Use Electron's built-in APIs for file system and native dialogs

Files:

  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/error-logging.mdc)

**/*.{ts,tsx}: 始终使用 try-catch 处理可能的错误
提供有意义的错误信息
记录详细的错误日志
优雅降级处理
日志应包含时间戳、日志级别、错误代码、错误描述、堆栈跟踪(如适用)、相关上下文信息
日志级别应包括 ERROR、WARN、INFO、DEBUG
不要吞掉错误
提供用户友好的错误信息
实现错误重试机制
避免记录敏感信息
使用结构化日志
设置适当的日志级别

Files:

  • src/main/presenter/threadPresenter/index.ts
src/main/**/*.{ts,js,tsx,jsx}

📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)

主进程代码放在 src/main

Files:

  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx,js,vue}

📄 CodeRabbit inference engine (CLAUDE.md)

Use English for all logs and comments

Files:

  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx,vue}

📄 CodeRabbit inference engine (CLAUDE.md)

Enable and adhere to strict TypeScript typing (avoid implicit any, prefer precise types)

Use PascalCase for TypeScript types and classes

Files:

  • src/main/presenter/threadPresenter/index.ts
src/main/presenter/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Place Electron main-process presenters under src/main/presenter/ (Window, Tab, Thread, Mcp, Config, LLMProvider)

Files:

  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx,js,jsx,vue,css,scss,md,json,yml,yaml}

📄 CodeRabbit inference engine (AGENTS.md)

Prettier style: single quotes, no semicolons, print width 100; run pnpm run format

Files:

  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx,js,jsx,vue}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx,vue}: Use OxLint for JS/TS code; keep lint clean
Use camelCase for variables and functions
Use SCREAMING_SNAKE_CASE for constants

Files:

  • src/main/presenter/threadPresenter/index.ts
🧠 Learnings (1)
📚 Learning: 2025-07-21T01:46:52.880Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: .cursor/rules/llm-agent-loop.mdc:0-0
Timestamp: 2025-07-21T01:46:52.880Z
Learning: Applies to src/main/presenter/llmProviderPresenter/index.ts : The main Agent loop should buffer text content, handle tool call events, format tool results for the next LLM call, and manage conversation continuation logic.

Applied to files:

  • src/main/presenter/threadPresenter/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build-check (x64)
🔇 Additional comments (2)
src/main/presenter/threadPresenter/index.ts (2)

437-447: Granted flow OK; ensure extra.grantedPermissions matches union.

Looks correct. After adopting PermissionExtra, this remains type‑safe.

Please verify the renderer uses status === 'granted' and extra.grantedPermissions to show the approved state.


457-461: Denied flow OK.

Block status and needsUserAction reset are correct.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/renderer/src/components/message/MessageBlockPermissionRequest.vue (2)

182-206: Add 'success' handling to icons/styles for consistency.

Currently 'success' shows a clock icon with muted color. Treat it like 'granted'.

 const getStatusIcon = () => {
   switch (props.block.status) {
     case 'granted':
       return 'lucide:check'
+    case 'success':
+      return 'lucide:check'
     case 'denied':
       return 'lucide:x'
     case 'error':
       return 'lucide:alert'
     default:
       return 'lucide:clock'
   }
 }

 const getStatusIconClass = () => {
   switch (props.block.status) {
     case 'granted':
-      return 'bg-green-500 rounded-full text-white p-0.5 dark:bg-green-800'
+      return 'bg-green-500 rounded-full text-white p-0.5 dark:bg-green-800'
+    case 'success':
+      return 'bg-green-500 rounded-full text-white p-0.5 dark:bg-green-800'
     case 'denied':
       return 'text-white p-0.5 bg-red-500 rounded-full dark:bg-red-800'
     case 'error':
       return 'text-white p-0.5 bg-red-500 rounded-full dark:bg-red-800'
     default:
       return 'text-muted-foreground'
   }
 }

208-220: Fix i18n for status text and add missing i18n keys; handle 'success' status consistently across text, icons, and styling.

Three issues found:

  1. Hardcoded strings in getStatusText() violate i18n rules:

    • Line 217: 'Error' → use i18n key
    • Line 219: 'Pending' → use i18n key
  2. Missing i18n keys in components.json:

    • messageBlockPermissionRequest.error (needed)
    • messageBlockPermissionRequest.pending (needed)
  3. Inconsistent 'success' status handling:

    • getStatusText() maps 'success' to 'granted' text
    • getStatusIcon() and getStatusIconClass() lack 'success' case (fall to default clock/muted styling)
    • Must add case 'success': to both icon functions to treat it like 'granted' for visual consistency

Apply the suggested diff, add the missing i18n keys to components.json, and update getStatusIcon() and getStatusIconClass() with:

case 'success':
  // same as 'granted' case
♻️ Duplicate comments (1)
src/main/presenter/threadPresenter/index.ts (1)

392-396: Least‑privilege: default to 'read' and validate permissionType.

Defaulting to 'write' is unsafe; validate against allowlist and fall back to 'read'.

-        const permissionType = permission_request?.permissionType ?? 'write'
+        const requested = permission_request?.permissionType
+        const permissionType: 'read' | 'write' | 'all' =
+          requested === 'read' || requested === 'write' || requested === 'all' ? requested : 'read'
🧹 Nitpick comments (6)
src/renderer/src/components/message/MessageBlockPermissionRequest.vue (2)

6-6: Tailwind class typo: use min-h-[40px], not h-min-[40px].

'h-min-[...]' is not a Tailwind utility; this likely has no effect.

- class="flex flex-col h-min-[40px] hover:bg-muted select-none cursor-pointer pt-3 overflow-hidden w-[380px] break-all shadow-sm my-2 items-start gap-3 rounded-lg border bg-card text-card-foreground"
+ class="flex flex-col min-h-[40px] hover:bg-muted select-none cursor-pointer pt-3 overflow-hidden w-[380px] break-all shadow-sm my-2 items-start gap-3 rounded-lg border bg-card text-card-foreground"
- class="flex flex-col h-min-[40px] overflow-hidden w-[380px] break-all shadow-sm my-2 rounded-lg border bg-card text-card-foreground p-3"
+ class="flex flex-col min-h-[40px] overflow-hidden w-[380px] break-all shadow-sm my-2 rounded-lg border bg-card text-card-foreground p-3"

Also applies to: 32-32


152-176: Standardize comment language to English.

A few inline comments are in Chinese; repo guideline asks logs/comments in English. Convert for consistency. Example:

-  // 处理 undefined content
+  // Handle undefined content
-  // 检查是否是 i18n key
+  // Check if content is an i18n key
-  // 回退到使用 extra 字段中的信息
+  // Fallback to extra fields if JSON parse fails
-  // 向后兼容:直接返回原文本
+  // Backward compatibility: return raw text
src/main/presenter/threadPresenter/promptBuilder.ts (2)

317-339: collectAssistantTextBeforePermission: safe and minimal, but consider trimming whitespace.

Joining without separators can fuse paragraphs; consider parts.join('\n') to preserve readability.


307-311: Hardcoded Chinese in tool execution context should be parameterized for localization.

The Chinese instruction text at line 307 will bias LLM responses regardless of user locale. While your codebase already supports i18n (with getContextMenuLabels, getErrorMessageLabels patterns in other presenters), buildPostToolExecutionContext currently has no access to locale information.

This requires refactoring the function signature to accept locale (either directly or via conversation.settings), similar to how threadPresenter accesses configPresenter.getLanguage(). Note: an identical hardcoded string also appears in src/main/presenter/llmProviderPresenter/index.ts:1268.

src/main/presenter/threadPresenter/index.ts (2)

3062-3207: Continue-after-deny flow is robust; minor polish possible.

Flow sends a tool 'end' with failure reason, rebuilds context via buildPostToolExecutionContext, and resumes streaming. Consider including a machine-readable error code in tool_call_response_raw if upstream supports it, to let renderers style the failure.


245-258: Standardize logs/comments in English for consistency.

Several logs/comments are Chinese. Repo guideline: logs/comments should be in English. Convert in touched areas going forward.

Also applies to: 874-889, 1363-1376

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1641cba and 9cbdae0.

📒 Files selected for processing (4)
  • src/main/presenter/threadPresenter/index.ts (13 hunks)
  • src/main/presenter/threadPresenter/promptBuilder.ts (4 hunks)
  • src/main/presenter/threadPresenter/types.ts (2 hunks)
  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue (1 hunks)
🧰 Additional context used
📓 Path-based instructions (22)
src/renderer/src/**/*

📄 CodeRabbit inference engine (.cursor/rules/i18n.mdc)

src/renderer/src/**/*: All user-facing strings must use i18n keys (avoid hardcoded user-visible text in code)
Use the 'vue-i18n' framework for all internationalization in the renderer
Ensure all user-visible text in the renderer uses the translation system

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
src/renderer/**/*.{vue,ts,js,tsx,jsx}

📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)

渲染进程代码放在 src/renderer

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
src/renderer/src/**/*.{vue,ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/vue-best-practices.mdc)

src/renderer/src/**/*.{vue,ts,tsx,js,jsx}: Use the Composition API for better code organization and reusability
Implement proper state management with Pinia
Utilize Vue Router for navigation and route management
Leverage Vue's built-in reactivity system for efficient data handling

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
src/renderer/src/**/*.vue

📄 CodeRabbit inference engine (.cursor/rules/vue-best-practices.mdc)

Use scoped styles to prevent CSS conflicts between components

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
src/renderer/**/*.{ts,tsx,vue}

📄 CodeRabbit inference engine (.cursor/rules/vue-shadcn.mdc)

src/renderer/**/*.{ts,tsx,vue}: Use descriptive variable names with auxiliary verbs (e.g., isLoading, hasError).
Use TypeScript for all code; prefer types over interfaces.
Avoid enums; use const objects instead.
Use arrow functions for methods and computed properties.
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements.

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
src/renderer/**/*.{vue,ts}

📄 CodeRabbit inference engine (.cursor/rules/vue-shadcn.mdc)

Implement lazy loading for routes and components.

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
src/renderer/**/*.{ts,vue}

📄 CodeRabbit inference engine (.cursor/rules/vue-shadcn.mdc)

src/renderer/**/*.{ts,vue}: Use useFetch and useAsyncData for data fetching.
Implement SEO best practices using Nuxt's useHead and useSeoMeta.

Use Pinia for frontend state management (do not introduce alternative state libraries)

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
**/*.{ts,tsx,js,vue}

📄 CodeRabbit inference engine (CLAUDE.md)

Use English for all logs and comments

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
  • src/main/presenter/threadPresenter/types.ts
  • src/main/presenter/threadPresenter/promptBuilder.ts
  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx,vue}

📄 CodeRabbit inference engine (CLAUDE.md)

Enable and adhere to strict TypeScript typing (avoid implicit any, prefer precise types)

Use PascalCase for TypeScript types and classes

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
  • src/main/presenter/threadPresenter/types.ts
  • src/main/presenter/threadPresenter/promptBuilder.ts
  • src/main/presenter/threadPresenter/index.ts
src/renderer/{src,shell,floating}/**/*.vue

📄 CodeRabbit inference engine (CLAUDE.md)

src/renderer/{src,shell,floating}/**/*.vue: Use Vue 3 Composition API for all components
All user-facing strings must use i18n keys via vue-i18n (no hard-coded UI strings)
Use Tailwind CSS utilities and ensure styles are scoped in Vue components

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
src/renderer/src/components/**/*

📄 CodeRabbit inference engine (CLAUDE.md)

Organize UI components by feature within src/renderer/src/

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
src/renderer/src/**

📄 CodeRabbit inference engine (AGENTS.md)

Place Vue 3 app source under src/renderer/src (components, stores, views, i18n, lib)

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
src/renderer/src/**/*.{vue,ts}

📄 CodeRabbit inference engine (AGENTS.md)

All user-facing strings must use vue-i18n ($t/keys) rather than hardcoded literals

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
**/*.{ts,tsx,js,jsx,vue,css,scss,md,json,yml,yaml}

📄 CodeRabbit inference engine (AGENTS.md)

Prettier style: single quotes, no semicolons, print width 100; run pnpm run format

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
  • src/main/presenter/threadPresenter/types.ts
  • src/main/presenter/threadPresenter/promptBuilder.ts
  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx,js,jsx,vue}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx,vue}: Use OxLint for JS/TS code; keep lint clean
Use camelCase for variables and functions
Use SCREAMING_SNAKE_CASE for constants

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
  • src/main/presenter/threadPresenter/types.ts
  • src/main/presenter/threadPresenter/promptBuilder.ts
  • src/main/presenter/threadPresenter/index.ts
src/renderer/**/*.vue

📄 CodeRabbit inference engine (AGENTS.md)

Name Vue component files in PascalCase (e.g., ChatInput.vue)

Files:

  • src/renderer/src/components/message/MessageBlockPermissionRequest.vue
**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)

**/*.{js,jsx,ts,tsx}: 使用 OxLint 进行代码检查
Log和注释使用英文书写

Files:

  • src/main/presenter/threadPresenter/types.ts
  • src/main/presenter/threadPresenter/promptBuilder.ts
  • src/main/presenter/threadPresenter/index.ts
src/{main,renderer}/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)

src/{main,renderer}/**/*.ts: Use context isolation for improved security
Implement proper inter-process communication (IPC) patterns
Optimize application startup time with lazy loading
Implement proper error handling and logging for debugging

Files:

  • src/main/presenter/threadPresenter/types.ts
  • src/main/presenter/threadPresenter/promptBuilder.ts
  • src/main/presenter/threadPresenter/index.ts
src/main/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/electron-best-practices.mdc)

Use Electron's built-in APIs for file system and native dialogs

Files:

  • src/main/presenter/threadPresenter/types.ts
  • src/main/presenter/threadPresenter/promptBuilder.ts
  • src/main/presenter/threadPresenter/index.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/error-logging.mdc)

**/*.{ts,tsx}: 始终使用 try-catch 处理可能的错误
提供有意义的错误信息
记录详细的错误日志
优雅降级处理
日志应包含时间戳、日志级别、错误代码、错误描述、堆栈跟踪(如适用)、相关上下文信息
日志级别应包括 ERROR、WARN、INFO、DEBUG
不要吞掉错误
提供用户友好的错误信息
实现错误重试机制
避免记录敏感信息
使用结构化日志
设置适当的日志级别

Files:

  • src/main/presenter/threadPresenter/types.ts
  • src/main/presenter/threadPresenter/promptBuilder.ts
  • src/main/presenter/threadPresenter/index.ts
src/main/**/*.{ts,js,tsx,jsx}

📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)

主进程代码放在 src/main

Files:

  • src/main/presenter/threadPresenter/types.ts
  • src/main/presenter/threadPresenter/promptBuilder.ts
  • src/main/presenter/threadPresenter/index.ts
src/main/presenter/**/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Place Electron main-process presenters under src/main/presenter/ (Window, Tab, Thread, Mcp, Config, LLMProvider)

Files:

  • src/main/presenter/threadPresenter/types.ts
  • src/main/presenter/threadPresenter/promptBuilder.ts
  • src/main/presenter/threadPresenter/index.ts
🧠 Learnings (4)
📚 Learning: 2025-09-06T03:07:23.817Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-06T03:07:23.817Z
Learning: Applies to src/main/presenter/mcpPresenter/index.ts : Register new MCP tools in src/main/presenter/mcpPresenter/index.ts

Applied to files:

  • src/main/presenter/threadPresenter/index.ts
📚 Learning: 2025-07-21T01:46:52.880Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: .cursor/rules/llm-agent-loop.mdc:0-0
Timestamp: 2025-07-21T01:46:52.880Z
Learning: Applies to src/main/presenter/llmProviderPresenter/index.ts : `src/main/presenter/llmProviderPresenter/index.ts` should manage the overall Agent loop, conversation history, tool execution via `McpPresenter`, and frontend communication via `eventBus`.

Applied to files:

  • src/main/presenter/threadPresenter/index.ts
📚 Learning: 2025-07-21T01:46:52.880Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: .cursor/rules/llm-agent-loop.mdc:0-0
Timestamp: 2025-07-21T01:46:52.880Z
Learning: Applies to src/main/presenter/llmProviderPresenter/index.ts : The main Agent loop should buffer text content, handle tool call events, format tool results for the next LLM call, and manage conversation continuation logic.

Applied to files:

  • src/main/presenter/threadPresenter/index.ts
📚 Learning: 2025-09-06T03:07:23.817Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-06T03:07:23.817Z
Learning: Applies to src/main/presenter/mcpPresenter/inMemoryServers/*.ts : Implement new MCP tools under src/main/presenter/mcpPresenter/inMemoryServers/

Applied to files:

  • src/main/presenter/threadPresenter/index.ts
🧬 Code graph analysis (3)
src/main/presenter/threadPresenter/types.ts (1)
src/main/presenter/threadPresenter/promptBuilder.ts (1)
  • PendingToolCall (23-30)
src/main/presenter/threadPresenter/promptBuilder.ts (3)
src/shared/types/presenters/legacy.presenters.d.ts (3)
  • CONVERSATION (746-755)
  • ModelConfig (132-150)
  • ChatMessage (1376-1376)
src/shared/chat.d.ts (1)
  • AssistantMessage (39-42)
src/main/presenter/threadPresenter/messageContent.ts (1)
  • buildUserMessageContext (155-164)
src/main/presenter/threadPresenter/index.ts (7)
src/shared/chat.d.ts (2)
  • AssistantMessage (39-42)
  • AssistantMessageBlock (84-138)
src/main/events.ts (1)
  • STREAM_EVENTS (59-63)
src/main/presenter/threadPresenter/promptBuilder.ts (1)
  • buildPostToolExecutionContext (234-315)
src/main/presenter/threadPresenter/types.ts (1)
  • GeneratingMessageState (4-36)
src/shared/types/presenters/legacy.presenters.d.ts (2)
  • MCPToolDefinition (1146-1162)
  • MCPToolResponse (1178-1207)
src/shared/types/core/mcp.ts (2)
  • MCPToolDefinition (3-19)
  • MCPToolResponse (58-71)
src/main/presenter/index.ts (1)
  • presenter (223-223)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build-check (x64)
🔇 Additional comments (3)
src/main/presenter/threadPresenter/types.ts (1)

2-2: Type-only import for PendingToolCall is appropriate; no runtime cycle.

Using import type prevents runtime deps; adding pendingToolCall?: PendingToolCall cleanly extends state without side effects. LGTM.

Also applies to: 15-15

src/main/presenter/threadPresenter/promptBuilder.ts (1)

23-30: Exported PendingToolCall shape looks good.

The added server metadata fields enable richer UI/state sync later. No issues.

src/main/presenter/threadPresenter/index.ts (1)

2834-2879: Good: in-memory state sync prevents DB overwrite after permission.

Copying the updated permission block back into generatingState.message.content avoids stale overwrites. LGTM.

@zerob13 zerob13 merged commit 5b039d0 into dev Oct 29, 2025
2 checks passed
@zerob13 zerob13 deleted the codex/mcp branch November 6, 2025 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants