Skip to content

Conversation

@zerob13
Copy link
Collaborator

@zerob13 zerob13 commented Oct 25, 2025

Summary

  • merge provider DB models with cached provider-specific models when refreshing the settings store so newly downloaded models appear
  • add MODEL_META typing to normalize stored entries before merging

Testing

  • pnpm run typecheck:web

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

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced how custom AI model configurations are stored and retrieved, ensuring custom settings are properly preserved when loading available models.
    • Improved fallback logic for model availability to merge persisted model definitions with available database models for better reliability.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 25, 2025

Walkthrough

The refreshStandardModels function in the settings store was enhanced to merge persisted provider model definitions with database-derived models. When stored models exist, each is normalized and merged into the in-memory model list, with stored overrides taking precedence, while maintaining backward compatibility through existing fallback paths.

Changes

Cohort / File(s) Summary
Settings Store Model Merging Logic
src/renderer/src/stores/settings.ts
Enhanced refreshStandardModels to normalize and merge persisted provider models with database-aggregated models. Added MODEL_META and RENDERER_MODEL_META imports. Implemented normalization logic to produce normalized metadata (id, name, group, providerId, enabled, isCustom, contextLength, maxTokens, vision, functionCall, reasoning, enableSearch, type) and merge stored overrides into DB-derived models. Extended fallback to fetch from llmP when no models remain after merge.

Sequence Diagram

sequenceDiagram
    participant refreshStandardModels as refreshStandardModels()
    participant storedModels as Persisted Models
    participant dbModels as DB-Aggregated Models
    participant normalization as Normalization Logic
    participant merge as Merge Algorithm
    participant llmProvider as LLM Provider Fallback

    refreshStandardModels->>storedModels: Check if exists & non-empty
    alt Stored models exist
        refreshStandardModels->>storedModels: Iterate each stored model
        storedModels->>normalization: Normalize against DB model or defaults
        normalization-->>normalization: Produce RENDERER_MODEL_META
        normalization->>merge: Provide normalized fields
        merge->>dbModels: Merge into DB-derived models
        merge-->>merge: Prefer stored overrides, preserve DB models
        refreshStandardModels->>merge: Use merged result
    else No merged models
        refreshStandardModels->>llmProvider: Fetch models fallback
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • The normalization logic must be carefully verified to ensure all model metadata fields are correctly transformed and preserved
  • Merge precedence rules (stored overrides vs. DB models) require thorough testing across multiple scenarios
  • Fallback paths need validation to confirm backward compatibility when neither persisted nor DB models are available

Possibly related PRs

Poem

A rabbit hops through model lists so fine, 🐰
Merging persisted and database lines,
Normalizing fields with careful grace,
Stored overrides find their rightful place,
All while fallbacks keep the old ways alive!

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 pull request title "fix: merge stored provider models when refreshing settings" accurately captures the primary change in this changeset. The modification introduces logic to merge persisted provider model definitions with aggregated provider DB models during the refreshStandardModels operation, which is precisely what the title conveys. The title is concise, specific, and avoids vague language, making it clear to developers reviewing the history that this change addresses model merging during the settings refresh process. The use of the "fix:" prefix appropriately indicates this resolves an issue with the prior behavior.
✨ 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/fix-model-fetching-logic-in-settings.ts

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

🧹 Nitpick comments (1)
src/renderer/src/stores/settings.ts (1)

513-513: Use English for comments

Replace the Chinese comment with an English equivalent to match project guidelines.

As per coding guidelines

-      // 若聚合 DB 为空且没有持久化模型,回退到 LLMProviderPresenter 的模型列表
+      // Fallback: if DB models are empty and there are no persisted models, use LLMProviderPresenter model list
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2b4a1f0 and f1b8d1f.

📒 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
🧠 Learnings (2)
📚 Learning: 2025-07-23T00:45:57.322Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: .cursor/rules/vue-shadcn.mdc:0-0
Timestamp: 2025-07-23T00:45:57.322Z
Learning: Applies to src/renderer/stores/**/*.ts : Use Pinia for state management.

Applied to files:

  • src/renderer/src/stores/settings.ts
📚 Learning: 2025-10-14T08:02:59.495Z
Learnt from: CR
PR: ThinkInAIXYZ/deepchat#0
File: AGENTS.md:0-0
Timestamp: 2025-10-14T08:02:59.495Z
Learning: Use TypeScript + Vue 3 Composition API with Pinia for state and Tailwind for styles in the renderer

Applied to files:

  • src/renderer/src/stores/settings.ts
🧬 Code graph analysis (1)
src/renderer/src/stores/settings.ts (2)
src/shared/types/presenters/llmprovider.presenter.d.ts (2)
  • RENDERER_MODEL_META (10-25)
  • MODEL_META (27-42)
src/shared/types/presenters/legacy.presenters.d.ts (2)
  • RENDERER_MODEL_META (550-564)
  • MODEL_META (565-579)
⏰ 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)

3-3: Import extension looks good

Bringing in MODEL_META is appropriate for normalizing persisted entries; no issues.


462-512: Incorrect premise—getProviderModels is synchronous, not async

The review comment's core concern is unfounded. The method signature shows getProviderModels(providerId: string): MODEL_META[] returns a non-Promise array, and the implementation is synchronous. The original code at line 462 correctly does not use await. While defensive coding patterns (like null checks and coalescing) can improve robustness, they should not be framed as fixes for an async/await issue that does not exist.

Likely an incorrect or invalid review comment.

@zerob13
Copy link
Collaborator Author

zerob13 commented Oct 25, 2025

fix #1050

@zerob13 zerob13 merged commit 4002e48 into dev Oct 25, 2025
2 checks passed
@zerob13 zerob13 deleted the codex/fix-model-fetching-logic-in-settings.ts 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