Skip to content

Conversation

@yyhhyyyyyy
Copy link
Collaborator

@yyhhyyyyyy yyhhyyyyyy commented Aug 21, 2025

close #760

CleanShot.2025-08-21.at.15.33.46.mp4

Summary by CodeRabbit

  • Bug Fixes
    • Model enable/disable changes in Settings now reflect immediately and consistently in the UI.
  • Performance
    • Reduced unnecessary background refreshes related to model lists, resulting in smoother interactions and fewer UI redraws.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 21, 2025

Walkthrough

Removes the renderer-side IPC listener for MODEL_LIST_CHANGED and adds a reactive reassignment when updating enabledModels in the settings store. Also reformats a sendToRenderer call in the main config presenter without altering logic or payload.

Changes

Cohort / File(s) Summary of Changes
Renderer settings store reactivity
src/renderer/src/stores/settings.ts
Removed MODEL_LIST_CHANGED listener logic; added forced reactivity by reassigning enabledModels.value to a new array and logging after updates. Adjusts refresh flow to rely on local reactive updates.
Main presenter formatting
src/main/presenter/configPresenter/index.ts
Reformatted eventBus.sendToRenderer(MODEL_STATUS_CHANGED, { providerId, modelId, enabled }) call to single line; no functional changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Settings as Renderer: SettingsStore
  participant UI as Renderer: Chat UI
  participant Main as Main Process

  User->>Settings: Toggle model enabled/disabled
  Settings->>Main: (Existing) sendToMain: setModelStatus(providerId, modelId, enabled)
  Main-->>Settings: (Existing) sendToRenderer: MODEL_STATUS_CHANGED {providerId, modelId, enabled}
  note over Settings: Update enabledModels and reassign array<br/>(forced reactivity)
  Settings-->>UI: Vue reactivity triggers computed/model list updates
  UI->>UI: Refresh available model list

  rect rgb(250, 250, 210)
    note over Settings,UI: Removed prior dependency on MODEL_LIST_CHANGED listener
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Assessment against linked issues

Objective Addressed Explanation
Immediate sync of service provider model changes to Chat (#760) Depends on Chat consuming enabledModels/reactivity; no Chat component changes in this PR.
Model list updates without reopening Chat or toggling provider (#760) Removal of MODEL_LIST_CHANGED listener may suffice if reactive pipeline is wired; not verifiable here.
Update occurs on model enable/disable action in Settings (#760) Forced reactivity suggests updates propagate, but end-to-end link to Chat model selector not shown.

Assessment against linked issues: Out-of-scope changes

(No out-of-scope functional changes identified.)

Possibly related PRs

Suggested reviewers

  • zerob13

Poem

I toggled a switch with a flick of my ear,
Models now hop into Chat, crystal clear.
No more waiting burrows deep,
Arrays re-bound, reactivity leap!
Thump-thump logs, a carrot cheer—
Lists refresh fast, the lag gone dear.
Happy hops in code this year! 🥕🐇

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/model-list-sync-across-tabs

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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: 0

Caution

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

⚠️ Outside diff range comments (4)
src/renderer/src/stores/settings.ts (4)

372-373: Duplicate listener registration → events fire twice

setupProviderListener() is invoked inside initSettings() (Line 372) and again in onMounted() (Lines 1317-1320). This leads to duplicated IPC listeners, double updates, and potential memory leaks.

Fix by removing the duplicate call in onMounted():

 onMounted(async () => {
   await initSettings()
-  await setupProviderListener()
 })

Also applies to: 1317-1320


580-591: Await both refreshes to avoid race conditions

refreshProviderModels() fires refreshCustomModels() and refreshStandardModels() without awaiting them. Subsequent logic (e.g., checkAndUpdateSearchAssistantModel) can run before lists are populated, causing inconsistent UI.

Make both calls concurrent and awaited:

-    // 自定义模型直接从配置存储获取,不需要等待provider实例
-    refreshCustomModels(providerId)
-
-    // 标准模型需要provider实例,可能需要等待实例初始化
-    refreshStandardModels(providerId)
+    // 并发且等待两类刷新,确保状态一致
+    await Promise.all([
+      refreshCustomModels(providerId),
+      refreshStandardModels(providerId)
+    ])

And in the catch block:

-    refreshCustomModels(providerId)
+    await refreshCustomModels(providerId)

688-693: Avoid re-registering unrelated listeners inside setupProviderListener

setupProviderListener() also calls setupContentProtectionListener() and setupCopyWithCotEnabledListener(), both already invoked in initSettings(). This causes duplicate event handlers.

Remove these calls from setupProviderListener():

-    // 添加对投屏保护变更的监听
-    setupContentProtectionListener()
-
-    // 设置拷贝事件监听器
-    setupCopyWithCotEnabledListener()

1323-1327: Cleanup misses core CONFIG event listeners

cleanup() removes OLLAMA and SEARCH_ENGINES_UPDATED listeners but leaves PROVIDER_CHANGED, MODEL_LIST_CHANGED, MODEL_STATUS_CHANGED, CONTENT_PROTECTION_CHANGED, COPY_WITH_COT_CHANGED. Add removals to prevent leaks on hot-reload/navigation:

 const cleanup = () => {
   removeOllamaEventListeners()
   // 清理搜索引擎事件监听器
   window.electron?.ipcRenderer?.removeAllListeners(CONFIG_EVENTS.SEARCH_ENGINES_UPDATED)
+  // 清理 provider/model 事件监听器
+  window.electron?.ipcRenderer?.removeAllListeners(CONFIG_EVENTS.PROVIDER_CHANGED)
+  window.electron?.ipcRenderer?.removeAllListeners(CONFIG_EVENTS.MODEL_LIST_CHANGED)
+  window.electron?.ipcRenderer?.removeAllListeners(CONFIG_EVENTS.MODEL_STATUS_CHANGED)
+  window.electron?.ipcRenderer?.removeAllListeners(CONFIG_EVENTS.CONTENT_PROTECTION_CHANGED)
+  window.electron?.ipcRenderer?.removeAllListeners(CONFIG_EVENTS.COPY_WITH_COT_CHANGED)
 }
🧹 Nitpick comments (3)
src/renderer/src/stores/settings.ts (3)

753-759: Avoid unnecessary full refresh after toggling a single model

updateModelStatus() performs await refreshProviderModels(providerId) after llmP.updateModelStatus(). UI already updates via CONFIG_EVENTS.MODEL_STATUS_CHANGED, so this extra refresh causes churn and latency, especially when enabling/disabling multiple models.

   const updateModelStatus = async (providerId: string, modelId: string, enabled: boolean) => {
     try {
       await llmP.updateModelStatus(providerId, modelId, enabled)
-      // 调用成功后,刷新该 provider 的模型列表
-      await refreshProviderModels(providerId)
+      // UI updates via MODEL_STATUS_CHANGED; skip extra refresh
     } catch (error) {
       console.error('Failed to update model status:', error)
     }
   }

747-751: Force reactivity is fine, but drop noisy log

The slice-spread trick to retrigger reactivity is acceptable here. Please remove the debug log to keep the console clean.

 // 强制触发响应式更新
 enabledModels.value = [...enabledModels.value]
-console.log('enabledModels updated:', enabledModels.value)

656-660: Remove stray debug log

console.log('changed') is leftover noise in production.

 window.electron.ipcRenderer.on(CONFIG_EVENTS.PROVIDER_CHANGED, async () => {
-  console.log('changed')
   providers.value = await configP.getProviders()
   await refreshAllModels()
 })
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 93d9095 and cad539e.

📒 Files selected for processing (2)
  • src/main/presenter/configPresenter/index.ts (1 hunks)
  • src/renderer/src/stores/settings.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
**/*.{ts,tsx,js,jsx,vue}

📄 CodeRabbit inference engine (CLAUDE.md)

Use English for logs and comments

Files:

  • src/main/presenter/configPresenter/index.ts
  • src/renderer/src/stores/settings.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Strict type checking enabled for TypeScript

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

Files:

  • src/main/presenter/configPresenter/index.ts
  • src/renderer/src/stores/settings.ts
src/main/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Main to Renderer: Use EventBus to broadcast events via mainWindow.webContents.send()

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

Files:

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

📄 CodeRabbit inference engine (CLAUDE.md)

One presenter per functional domain

Files:

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

📄 CodeRabbit inference engine (CLAUDE.md)

Centralize configuration in configPresenter/

Files:

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

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

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

Files:

  • src/main/presenter/configPresenter/index.ts
  • src/renderer/src/stores/settings.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/configPresenter/index.ts
  • src/renderer/src/stores/settings.ts
src/main/**/*.{ts,js,tsx,jsx}

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

主进程代码放在 src/main

Files:

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

📄 CodeRabbit inference engine (CLAUDE.md)

src/renderer/src/**/*.{ts,tsx,vue}: Use Pinia for frontend state management
Renderer to Main: Use usePresenter.ts composable for direct presenter method calls

Files:

  • src/renderer/src/stores/settings.ts
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/stores/settings.ts
src/renderer/src/stores/**/*.{vue,ts,tsx,js,jsx}

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

src/renderer/src/stores/**/*.{vue,ts,tsx,js,jsx}: Use modules to organize related state and actions
Implement proper state persistence for maintaining data across sessions
Use getters for computed state properties
Utilize actions for side effects and asynchronous operations
Keep the store focused on global state, not component-specific data

Files:

  • src/renderer/src/stores/settings.ts
src/renderer/**/*.{vue,ts,js,tsx,jsx}

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

渲染进程代码放在 src/renderer

Files:

  • src/renderer/src/stores/settings.ts
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/stores/settings.ts
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/stores/settings.ts
src/renderer/**/*.{vue,ts}

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

Implement lazy loading for routes and components.

Files:

  • src/renderer/src/stores/settings.ts
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.

Files:

  • src/renderer/src/stores/settings.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T01:45:33.790Z
Learning: Applies to src/renderer/src/**/*.{ts,tsx,vue} : Renderer to Main: Use usePresenter.ts composable for direct presenter method calls
📚 Learning: 2025-07-21T01:45:33.790Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T01:45:33.790Z
Learning: Applies to src/main/presenter/configPresenter/**/*.ts : Centralize configuration in configPresenter/

Applied to files:

  • src/main/presenter/configPresenter/index.ts
📚 Learning: 2025-07-21T01:45:33.790Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T01:45:33.790Z
Learning: Applies to src/renderer/src/**/*.{ts,tsx,vue} : Renderer to Main: Use usePresenter.ts composable for direct presenter method calls

Applied to files:

  • src/main/presenter/configPresenter/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 send standardized `STREAM_EVENTS` (`RESPONSE`, `END`, `ERROR`) to the frontend via `eventBus`.

Applied to files:

  • src/main/presenter/configPresenter/index.ts
🧬 Code graph analysis (1)
src/main/presenter/configPresenter/index.ts (3)
src/main/eventbus.ts (1)
  • eventBus (151-151)
src/main/events.ts (1)
  • CONFIG_EVENTS (12-35)
src/renderer/src/events.ts (1)
  • CONFIG_EVENTS (11-24)
🔇 Additional comments (4)
src/main/presenter/configPresenter/index.ts (1)

381-386: Good call: broadcast model-status changes to all windows

Sending CONFIG_EVENTS.MODEL_STATUS_CHANGED with {providerId, modelId, enabled} to SendTarget.ALL_WINDOWS is exactly what we need for instant cross-tab sync. No functional concerns here.

src/renderer/src/stores/settings.ts (3)

676-683: LGTM: fine-grained in-place update on MODEL_STATUS_CHANGED

Updating the specific model in-place and conditionally mutating enabledModels minimizes churn and avoids full list refreshes. This aligns with the cross-tab sync objective.


662-675: Ignore incorrect AI summary: MODEL_LIST_CHANGED listener is present

The MODEL_LIST_CHANGED listener in src/renderer/src/stores/settings.ts (around line 662) remains intact, and the main process continues to emit this event via eventBus.sendToRenderer(...) in multiple presenters (e.g., configPresenter and llmProviderPresenter) as verified by your grep results. The summary stating that the listener was “removed” is therefore inaccurate—no changes are needed to this listener.

Likely an incorrect or invalid review comment.


1156-1170: The scripts above will help uncover where and how listOllamaModels, model_info, and capabilities are defined and used in your codebase. Once you have the output, we can verify whether the optional chaining and Array.isArray(...).includes(...) changes are necessary or if the existing code already guards against missing properties.

@zerob13 zerob13 merged commit 9cacf4d into dev Aug 21, 2025
2 checks passed
@zerob13 zerob13 mentioned this pull request Aug 26, 2025
@zerob13 zerob13 deleted the fix/model-list-sync-across-tabs branch November 23, 2025 13:15
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.

[BUG] Model settings cannot be synchronized immediately 模型设置无法立即同步

3 participants