fix: improve optimistic toggles in settings store #1049
Conversation
WalkthroughReworked the settings store's model status update flow to propagate changes across standard provider models, custom models, and enabled models using direct mutation and rollback-on-failure logic. Introduced state snapshots for atomic operations and a helper to query model enabled state across all sources. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant updateModelStatus
participant Local State
participant Backend
Caller->>updateModelStatus: Request status change
updateModelStatus->>Local State: Snapshot current state
updateModelStatus->>Local State: Apply local mutation
updateModelStatus->>Backend: Send update request
alt Success
Backend-->>updateModelStatus: Confirm
updateModelStatus-->>Caller: Return success
else Failure
Backend-->>updateModelStatus: Error
updateModelStatus->>Local State: Rollback to snapshot
updateModelStatus-->>Caller: Return error
end
sequenceDiagram
participant Caller
participant updateLocalModelStatus
participant Provider Models
participant Custom Models
participant Enabled Models
Caller->>updateLocalModelStatus: Enable/disable model
updateLocalModelStatus->>Provider Models: Update/initialize entry
updateLocalModelStatus->>Custom Models: Check and update
updateLocalModelStatus->>Enabled Models: Sync changes
updateLocalModelStatus->>Enabled Models: Normalize and assign
updateLocalModelStatus-->>Caller: Complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve refactoring core state mutation logic across multiple functions with rollback mechanisms and multi-source synchronization, but remain localized to a single file with consistent patterns. Requires verification of state consistency guarantees, error handling paths, and the correctness of the rollback and snapshot logic. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
🧹 Nitpick comments (1)
src/renderer/src/stores/settings.ts (1)
764-826: Consider immutable array operations for clarity.The function directly mutates the
modelsarray (lines 806, 808, 812) withpushandsplice. While Vue 3's Proxy-based reactivity tracks these mutations correctly, creating new array references would make the update pattern more explicit and consistent with the immutable updates at lines 786, 816, and 821.Example refactor for the enabled branch:
if (enabled) { const sourceModel = providerModel ?? customModel ?? models[modelIndex] if (sourceModel) { const normalizedModel: RENDERER_MODEL_META = { ...sourceModel, enabled: true, vision: sourceModel.vision ?? false, functionCall: sourceModel.functionCall ?? false, reasoning: sourceModel.reasoning ?? false, type: sourceModel.type ?? ModelType.Chat } if (modelIndex === -1) { - models.push(normalizedModel) + enabledProvider.models = [...models, normalizedModel] } else { - models[modelIndex] = normalizedModel + enabledProvider.models = [ + ...models.slice(0, modelIndex), + normalizedModel, + ...models.slice(modelIndex + 1) + ] } } } else if (modelIndex !== -1) { - models.splice(modelIndex, 1) + enabledProvider.models = models.filter((_, i) => i !== modelIndex) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/renderer/src/stores/settings.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-setup.mdc)
**/*.{js,jsx,ts,tsx}: 使用 OxLint 进行代码检查
Log和注释使用英文书写
Files:
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/renderer/src/stores/settings.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-logging.mdc)
**/*.{ts,tsx}: 始终使用 try-catch 处理可能的错误
提供有意义的错误信息
记录详细的错误日志
优雅降级处理
日志应包含时间戳、日志级别、错误代码、错误描述、堆栈跟踪（如适用）、相关上下文信息
日志级别应包括 ERROR、WARN、INFO、DEBUG
不要吞掉错误
提供用户友好的错误信息
实现错误重试机制
避免记录敏感信息
使用结构化日志
设置适当的日志级别
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.Use Pinia for frontend state management (do not introduce alternative state libraries)
Files:
src/renderer/src/stores/settings.ts
**/*.{ts,tsx,js,vue}
📄 CodeRabbit inference engine (CLAUDE.md)
Use English for all logs and comments
Files:
src/renderer/src/stores/settings.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/stores/settings.ts
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/stores/settings.ts
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/stores/settings.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/renderer/src/stores/settings.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/stores/settings.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/renderer/src/stores/settings.ts (2)
828-847: LGTM! Well-structured helper function.The helper correctly queries enabled state across all three model sources and handles the semantic difference between
enabledModels(where existence implies enabled) and the other sources (where the.enabledproperty must be checked).
1002-1039: Excellent rollback implementation for provider status updates.The function properly snapshots both the provider state and timestamp, performs optimistic updates, and comprehensively rolls back on failure. The immutable update pattern at lines 1007-1012 is well done.
Minor edge case: If
optimizeProviderOrdersucceeds butupdateProviderConfigfails, the provider order changes are not rolled back. This is a complex scenario that would requireoptimizeProviderOrderto return its previous state for rollback. Given the scope of this PR, the current implementation is acceptable.
| const updateModelStatus = async (providerId: string, modelId: string, enabled: boolean) => { | ||
| const previousState = getLocalModelEnabledState(providerId, modelId) | ||
| updateLocalModelStatus(providerId, modelId, enabled) | ||
|
|
||
| try { | ||
| await llmP.updateModelStatus(providerId, modelId, enabled) | ||
| // 调用成功后，刷新该 provider 的模型列表 | ||
| await refreshProviderModels(providerId) | ||
| } catch (error) { | ||
| console.error('Failed to update model status:', error) | ||
| if (previousState !== null && previousState !== enabled) { | ||
| updateLocalModelStatus(providerId, modelId, previousState) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider providing user-friendly error feedback.
The rollback logic is correct, but the error handling only logs to console without providing user-friendly error messages or structured logging with timestamps and error codes.
As per coding guidelines:
- "提供有意义的错误信息"
- "日志应包含时间戳、日志级别、错误代码、错误描述、堆栈跟踪（如适用）、相关上下文信息"
Based on coding guidelines
Consider adding user-visible error notification or toast, and enriching the log with context:
} catch (error) {
- console.error('Failed to update model status:', error)
+ console.error('[Settings Store] Failed to update model status', {
+ timestamp: new Date().toISOString(),
+ providerId,
+ modelId,
+ enabled,
+ error
+ })
if (previousState !== null && previousState !== enabled) {
updateLocalModelStatus(providerId, modelId, previousState)
}
+ // TODO: Show user-friendly error notification
+ throw error
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateModelStatus = async (providerId: string, modelId: string, enabled: boolean) => { | |
| const previousState = getLocalModelEnabledState(providerId, modelId) | |
| updateLocalModelStatus(providerId, modelId, enabled) | |
| try { | |
| await llmP.updateModelStatus(providerId, modelId, enabled) | |
| // 调用成功后，刷新该 provider 的模型列表 | |
| await refreshProviderModels(providerId) | |
| } catch (error) { | |
| console.error('Failed to update model status:', error) | |
| if (previousState !== null && previousState !== enabled) { | |
| updateLocalModelStatus(providerId, modelId, previousState) | |
| } | |
| } | |
| } | |
| const updateModelStatus = async (providerId: string, modelId: string, enabled: boolean) => { | |
| const previousState = getLocalModelEnabledState(providerId, modelId) | |
| updateLocalModelStatus(providerId, modelId, enabled) | |
| try { | |
| await llmP.updateModelStatus(providerId, modelId, enabled) | |
| // 调用成功后，刷新该 provider 的模型列表 | |
| await refreshProviderModels(providerId) | |
| } catch (error) { | |
| console.error('[Settings Store] Failed to update model status', { | |
| timestamp: new Date().toISOString(), | |
| providerId, | |
| modelId, | |
| enabled, | |
| error | |
| }) | |
| if (previousState !== null && previousState !== enabled) { | |
| updateLocalModelStatus(providerId, modelId, previousState) | |
| } | |
| // TODO: Show user-friendly error notification | |
| throw error | |
| } | |
| } |
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68fb8869d578832cbf166e159b5bc639
Summary by CodeRabbit