-
Notifications
You must be signed in to change notification settings - Fork 625
fix: resolve model list synchronization issue across tabs #763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughRemoves 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
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
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(No out-of-scope functional changes identified.) Possibly related PRs
Suggested reviewers
Poem
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 unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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 twicesetupProviderListener() 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 conditionsrefreshProviderModels() 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 setupProviderListenersetupProviderListener() 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 listenerscleanup() 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 modelupdateModelStatus() 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 logThe 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 logconsole.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.
📒 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.tssrc/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.tssrc/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.tssrc/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.tssrc/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 windowsSending 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_CHANGEDUpdating 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 presentThe
MODEL_LIST_CHANGEDlistener insrc/renderer/src/stores/settings.ts(around line 662) remains intact, and the main process continues to emit this event viaeventBus.sendToRenderer(...)in multiple presenters (e.g.,configPresenterandllmProviderPresenter) 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 howlistOllamaModels,model_info, andcapabilitiesare defined and used in your codebase. Once you have the output, we can verify whether the optional chaining andArray.isArray(...).includes(...)changes are necessary or if the existing code already guards against missing properties.
close #760
CleanShot.2025-08-21.at.15.33.46.mp4
Summary by CodeRabbit