feat: add model preferences#302
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @KagurazakaSakuya, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
|
|
Thanks for your first pull request to TouchAI. 感谢你向 TouchAI 提交第一条 Pull Request。 A few quick checks before review:
Helpful links: Thanks for contributing. |
github-actions
Bot
added
area:tauri
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a scenario-based model preference system: users configure scenario-to-model mappings (e.g., "frontend → Claude Sonnet") stored in a new database table; the system injects these preferences into the agent prompt so the AI can intelligently route tasks via an enhanced ChangesModel preferences and settings
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Settings UI
participant Store as Settings Store
participant DB as Database
participant Prompt as Prompt Builder
participant Agent as Agent Service
participant Tool as UpgradeTool
User->>UI: Create scenario preference<br/>(frontend → Claude Sonnet)
UI->>DB: createModelPreference()
DB-->>UI: saved
Note over Agent: On new conversation
Prompt->>DB: listModelPreferences()
Prompt->>DB: findModelRoleWithProvider(entry/fast/general)
DB-->>Prompt: preferences + role models
Prompt-->>Agent: build markdown table
Agent->>Agent: inject preferences in system prompt
Note over Agent,Tool: During conversation
User->>Agent: "Help me write a React component"
Agent->>Agent: recognize matches frontend scenario
Agent->>Tool: upgrade_model({ scenario: "frontend" })
Tool->>DB: findModelPreferenceByName("frontend")
DB-->>Tool: Claude Sonnet
Tool-->>Agent: switched to Claude Sonnet
Agent->>Agent: respond with Claude Sonnet
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/components/AlertMessage.vue (1)
4-36: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding JSDoc documentation for the ownership mechanism.
The module-level subscription mechanism is correct and handles concurrent instance lifecycle correctly. However, JSDoc comments explaining the ownership protocol would improve maintainability—particularly documenting:
- Why no locks are needed (JS single-threaded execution)
- The notification flow when the owner unmounts
- Expected behavior when no instances remain
📚 Example documentation
+/** + * Callback invoked when the Sonner owner state changes (e.g., current owner releases). + */ type AlertSonnerOwnerListener = () => void; +/** + * UID of the component instance currently owning the Sonner toast renderer. + * Only one instance should render <Toaster /> to avoid duplicate toast containers. + */ let sonnerOwnerUid: number | null = null; + +/** + * Registered listeners that will be notified when ownership is released. + */ const alertSonnerOwnerListeners = new Set<AlertSonnerOwnerListener>(); +/** + * Notifies all subscribed listeners that the Sonner owner has changed. + * Uses a defensive copy to allow listeners to modify the set during iteration. + */ const notifyAlertSonnerOwnerChanged = (): void => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/components/AlertMessage.vue` around lines 4 - 36, Add JSDoc comments to the top-level ownership mechanism describing the protocol and notification flow: document the purpose of sonnerOwnerUid and alertSonnerOwnerListeners, explain that claimAlertSonnerOwner returns true if it acquires or already holds ownership, describe releaseAlertSonnerOwner clearing ownership and calling notifyAlertSonnerOwnerChanged to wake subscribers, and explain subscribeAlertSonnerOwner's contract (returns unsubscribe) and why explicit locks aren’t needed in JS single-threaded execution and what happens when no instances remain; attach these JSDoc blocks to the functions/variables claimAlertSonnerOwner, releaseAlertSonnerOwner, subscribeAlertSonnerOwner, notifyAlertSonnerOwnerChanged, and the sonnerOwnerUid variable for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/database/drizzle/0002_model_preferences.sql`:
- Line 16: The schema currently creates a non-unique index
model_preferences_name_idx on table model_preferences, allowing duplicate
scenario names; change the migration to create a UNIQUE INDEX on the name column
(replace CREATE INDEX ... with CREATE UNIQUE INDEX `model_preferences_name_idx`
ON `model_preferences` (`name`)) and also update the corresponding schema
definition in schema.ts to use uniqueIndex for the model_preferences.name field
so the uniqueness is enforced both in the DB migration and in the application
schema.
In `@apps/desktop/src/database/queries/models.ts`:
- Around line 138-145: The listModelPreferences query orders by id so preference
priority is ignored; change the ORDER BY to sort first by
modelPreferences.priority (ascending or descending per app semantics—use
ascending here) and then by modelPreferences.id as a stable tiebreaker (e.g.,
orderBy(asc(modelPreferences.priority), asc(modelPreferences.id))). Apply the
same change to the corresponding name-lookup query referenced in the review (the
other query that currently orders by id around lines 150-157) so both
listModelPreferences and the name lookup use priority then id for deterministic
routing.
In `@apps/desktop/src/i18n/messages.ts`:
- Line 605: The i18n entry uses a literal Chinese string as the key ("模型已是目标模型")
which breaks the project's namespacing; replace that key with a namespaced key
(e.g., "conversation.approval.modelAlreadyTarget") while leaving the Chinese
text as the value, and update any other occurrences of the literal key (the
other occurrence at the second instance) to use the new namespaced key so
consumers reference a stable key.
In `@apps/desktop/src/services/AgentService/prompt/modelPreferences.ts`:
- Around line 7-9: In escapeMarkdownCell, backslashes are not escaped before
replacing pipe characters, so user input like "\" can break markdown tables;
update the function (escapeMarkdownCell) to first replace backslashes with
"\\\\" (escape them), then replace pipes with "\\|" and newlines with a space,
and finally trim the result so all backslashes are preserved safely before cell
escaping.
In
`@apps/desktop/src/services/BuiltInToolService/tools/upgradeModel/constants.ts`:
- Around line 34-38: The UPGRADE_MODEL_TOOL_INPUT_SCHEMA in constants.ts
currently allows any string for the scenario field while runtime validation
(upgradeModelArgsSchema) rejects empty or whitespace-only strings; update the
input schema's scenario definition (the scenario property in
UPGRADE_MODEL_TOOL_INPUT_SCHEMA) to disallow empty/whitespace-only values —
e.g., keep type: ['string','null'] and add a JSON Schema pattern like
'^(?!\\s*$).+' (or equivalent non-empty/non-whitespace constraint) so the tool
input and upgradeModelArgsSchema remain aligned.
In `@apps/desktop/src/services/BuiltInToolService/tools/upgradeModel/index.ts`:
- Around line 164-176: The new hard-coded English messages in
resolveScenarioTarget (and the other message locations referenced) should be
wrapped with the app's i18n helper (tt(...)) instead of plain strings; update
the thrown Error messages in resolveScenarioTarget and the switch summary/output
lines around the referenced ranges (including the checks for missing preference,
null model_id, unavailable model/provider, and the messages at lines ~192-195,
214-215, 280-292) to call tt(...) with the same text (or appropriate translation
keys) so all user-facing strings use i18n consistently; search for
resolveScenarioTarget, the switch summary function, and the surrounding message
constants and replace plain English literals with tt(...) calls.
- Around line 236-248: The current selection logic in upgradeModel uses
parsedArgs from parseUpgradeModelArgs and silently prioritizes restore, then
role, then scenario; change it to detect conflicting selectors and fail fast: if
more than one of parsedArgs.restore === true, parsedArgs.role (truthy), or
parsedArgs.scenario being a non-empty string is present, throw or return a clear
validation error indicating conflicting target selectors; otherwise keep the
existing single-target resolution flow by calling resolveRestoreTarget,
resolveRoleTarget(parsedArgs.role), or
resolveScenarioTarget(parsedArgs.scenario).
In
`@apps/desktop/src/views/SettingsView/components/General/components/ModelPreferenceDialog.vue`:
- Around line 143-146: The initializeProvider function can coerce null to 0 via
Number(null) when providerOptions is empty; update initializeProvider to avoid
calling Number on null by using the explicit providerOptions presence check: use
selectedModel.value?.provider_id as first choice, else if providerOptions.value
has an element use Number(providerOptions.value[0].value), otherwise set
selectedProviderId.value to null (or undefined) to avoid the invalid 0
selection; adjust references to selectedProviderId, providerOptions and
selectedModel accordingly.
In `@apps/desktop/src/views/SettingsView/components/General/index.vue`:
- Around line 364-371: The save handler saveAllowModelAutoSwitch flips the
toggle optimistically but on persistence failure only logs and shows an error;
capture the previous value before calling
settingsStore.updateAllowModelAutoSwitch, then in the catch restore
settings.value.allowModelAutoSwitch to that previous value and show the error
via alertMessage.value?.error; make the same change to the analogous save
handler at the other occurrence (lines ~672-675) so both revert UI state on
failure (references: saveAllowModelAutoSwitch,
settingsStore.updateAllowModelAutoSwitch, settings.value.allowModelAutoSwitch,
alertMessage).
---
Outside diff comments:
In `@apps/desktop/src/components/AlertMessage.vue`:
- Around line 4-36: Add JSDoc comments to the top-level ownership mechanism
describing the protocol and notification flow: document the purpose of
sonnerOwnerUid and alertSonnerOwnerListeners, explain that claimAlertSonnerOwner
returns true if it acquires or already holds ownership, describe
releaseAlertSonnerOwner clearing ownership and calling
notifyAlertSonnerOwnerChanged to wake subscribers, and explain
subscribeAlertSonnerOwner's contract (returns unsubscribe) and why explicit
locks aren’t needed in JS single-threaded execution and what happens when no
instances remain; attach these JSDoc blocks to the functions/variables
claimAlertSonnerOwner, releaseAlertSonnerOwner, subscribeAlertSonnerOwner,
notifyAlertSonnerOwnerChanged, and the sonnerOwnerUid variable for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 040219b9-19e4-4a9d-8a36-0b48b2845441
📒 Files selected for processing (41)
.prettierignoreapps/desktop/src-tauri/resources/rtk/manifest.jsonapps/desktop/src/components/AlertMessage.vueapps/desktop/src/components/appIconMap.tsapps/desktop/src/database/drizzle/0002_model_preferences.sqlapps/desktop/src/database/drizzle/meta/_journal.jsonapps/desktop/src/database/queries/models.tsapps/desktop/src/database/schema.tsapps/desktop/src/database/types/index.tsapps/desktop/src/i18n/messages.tsapps/desktop/src/i18n/textMap.tsapps/desktop/src/services/AgentService/prompt/composer.tsapps/desktop/src/services/AgentService/prompt/modelPreferences.tsapps/desktop/src/services/BuiltInToolService/tools/upgradeModel/constants.tsapps/desktop/src/services/BuiltInToolService/tools/upgradeModel/helper.tsapps/desktop/src/services/BuiltInToolService/tools/upgradeModel/index.tsapps/desktop/src/services/EventService/types.tsapps/desktop/src/stores/settings.tsapps/desktop/src/views/SettingsView/components/AiServices/components/ModelCard.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ModelGroup.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ModelList.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ProviderCard.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ProviderList.vueapps/desktop/src/views/SettingsView/components/AiServices/index.vueapps/desktop/src/views/SettingsView/components/General/components/ModelPreferenceDialog.vueapps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vueapps/desktop/src/views/SettingsView/components/General/index.vueapps/desktop/src/views/SettingsView/components/ModelPreferences/index.vueapps/desktop/src/views/SettingsView/index.vueapps/desktop/src/views/SettingsView/settingsNavigation.tsapps/desktop/tests/SettingsView/ai-services-i18n.test.tsapps/desktop/tests/SettingsView/model-card-i18n.test.tsapps/desktop/tests/SettingsView/navigation-sidebar-i18n.test.tsapps/desktop/tests/services/AgentService/prompt/model-preferences.test.tsapps/desktop/tests/services/BuiltInToolService/tools/upgradeModel/i18n.test.tsapps/desktop/tests/views/SettingsView/modelGroupBehavior.test.tsapps/desktop/tests/views/SettingsView/modelListEmptyState.test.tsapps/desktop/tests/views/SettingsView/settingsAiServicesLayout.test.tsapps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.tsapps/desktop/tests/views/SettingsView/settingsNavigation.test.tseslint.config.js
💤 Files with no reviewable changes (1)
- apps/desktop/src/views/SettingsView/components/AiServices/components/ProviderList.vue
📜 Review details
⏰ 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). (5)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (rust)
- GitHub Check: Frontend Tests
- GitHub Check: Rust Checks
- GitHub Check: Desktop E2E Smoke (Windows)
🧰 Additional context used
🪛 GitHub Check: CodeQL
apps/desktop/src/services/AgentService/prompt/modelPreferences.ts
[failure] 8-8: Incomplete string escaping or encoding
This does not escape backslash characters in the input.
🔇 Additional comments (39)
apps/desktop/src/database/drizzle/meta/_journal.json (1)
19-24: LGTM!apps/desktop/src/database/schema.ts (1)
293-319: LGTM!Also applies to: 615-617
apps/desktop/src/database/types/index.ts (1)
256-285: LGTM!apps/desktop/src/database/queries/models.ts (1)
3-31: LGTM!Also applies to: 63-77, 127-136, 160-227, 312-367
.prettierignore (1)
55-56: LGTM!eslint.config.js (1)
63-66: LGTM!apps/desktop/src-tauri/resources/rtk/manifest.json (1)
8-8: ⚡ Quick winClarify the impact of the Windows
binary_digestchange
apps/desktop/src-tauri/build.rsusesbinary_digest(when non-empty) as the embedded bundled-binary SHA256 (BUNDLED_RTK_SHA256) and for cache reuse of the extractedrtk.exe. Thedigestfield is the SHA256 of the downloaded archive bytes, so updating onlybinary_digestchanges the integrity hash for the extracted binary even if the archive digest stays the same.Please confirm the PR’s intent: did the bundled/extracted Windows
rtk.exeactually change (or was the manifest regenerated automatically), and is the Windows-onlybinary_digestupdate required or should it be reverted?apps/desktop/tests/SettingsView/ai-services-i18n.test.ts (1)
185-202: LGTM!Also applies to: 211-212, 236-237, 317-317
apps/desktop/tests/SettingsView/model-card-i18n.test.ts (1)
85-85: LGTM!Also applies to: 101-102, 122-137, 146-146
apps/desktop/tests/services/AgentService/prompt/model-preferences.test.ts (1)
1-101: LGTM!apps/desktop/tests/services/BuiltInToolService/tools/upgradeModel/i18n.test.ts (1)
15-38: LGTM!Also applies to: 85-85, 206-315
apps/desktop/tests/views/SettingsView/modelGroupBehavior.test.ts (1)
67-67: LGTM!Also applies to: 74-75, 104-104, 112-113, 125-125, 139-139
apps/desktop/tests/SettingsView/navigation-sidebar-i18n.test.ts (1)
59-59: LGTM!apps/desktop/tests/views/SettingsView/modelListEmptyState.test.ts (1)
29-29: LGTM!apps/desktop/tests/views/SettingsView/settingsAiServicesLayout.test.ts (1)
239-239: LGTM!Also applies to: 291-292, 295-299, 314-325
apps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts (1)
190-190: LGTM!apps/desktop/tests/views/SettingsView/settingsNavigation.test.ts (1)
18-18: LGTM!Also applies to: 34-38
apps/desktop/src/views/SettingsView/components/AiServices/components/ModelCard.vue (1)
15-15: LGTM!Also applies to: 32-33
apps/desktop/src/views/SettingsView/components/AiServices/components/ModelGroup.vue (1)
20-20: LGTM!Also applies to: 44-44, 46-46, 49-49, 108-108
apps/desktop/src/views/SettingsView/components/AiServices/components/ModelList.vue (1)
17-17: LGTM!Also applies to: 59-59, 61-61, 72-73, 81-82, 111-112, 122-123, 136-137, 176-176, 319-319
apps/desktop/src/views/SettingsView/components/AiServices/components/ProviderCard.vue (1)
29-29: LGTM!apps/desktop/src/views/SettingsView/components/AiServices/index.vue (1)
77-78: LGTM!Also applies to: 173-174, 286-289, 298-301, 432-434, 488-497, 665-665
apps/desktop/src/services/EventService/types.ts (1)
73-78: LGTM!apps/desktop/src/stores/settings.ts (1)
27-39: LGTM!Also applies to: 41-53, 132-135, 168-170, 195-197, 228-240, 258-262, 276-277, 385-387, 440-440
apps/desktop/src/views/SettingsView/components/General/index.vue (1)
645-661: LGTM!Also applies to: 664-671, 677-687
apps/desktop/src/components/appIconMap.ts (1)
11-12: LGTM!Also applies to: 41-42, 53-54, 79-80
apps/desktop/src/views/SettingsView/settingsNavigation.ts (1)
5-8: LGTM!Also applies to: 53-63
apps/desktop/src/i18n/messages.ts (1)
123-124: LGTM!Also applies to: 156-167, 246-247, 282-320, 884-887, 922-934, 1015-1017, 1057-1097
apps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vue (1)
3-386: LGTM!apps/desktop/src/views/SettingsView/components/ModelPreferences/index.vue (1)
1-16: LGTM!apps/desktop/src/views/SettingsView/index.vue (1)
27-29: LGTM!Also applies to: 46-51, 262-277
apps/desktop/src/i18n/textMap.ts (1)
704-704: LGTM!apps/desktop/src/components/AlertMessage.vue (4)
4-13: LGTM!
27-27: LGTM!Also applies to: 31-36
55-65: LGTM!
67-72: LGTM!apps/desktop/src/services/AgentService/prompt/modelPreferences.ts (1)
37-100: LGTM!apps/desktop/src/services/AgentService/prompt/composer.ts (1)
15-15: LGTM!Also applies to: 95-109
apps/desktop/src/services/BuiltInToolService/tools/upgradeModel/helper.ts (1)
11-15: LGTM!Also applies to: 49-50
KagurazakaSakuya
force-pushed
the
feat/issue-139-model-preferences
branch
from
46a584f to
6035ef7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
apps/desktop/src/database/drizzle/0002_model_preferences.sql (1)
16-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce unique scenario names at the database layer.
Scenario resolution is name-based; a non-unique
nameallows ambiguous matches and nondeterministic routing targets. Makenameunique (and mirror it inschema.tswithuniqueIndex).Suggested migration adjustment
-CREATE INDEX `model_preferences_name_idx` ON `model_preferences` (`name`); +CREATE UNIQUE INDEX `model_preferences_name_uidx` ON `model_preferences` (`name`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/database/drizzle/0002_model_preferences.sql` at line 16, The migration currently creates a non-unique index model_preferences_name_idx on model_preferences; change the migration to enforce uniqueness by replacing that index with a UNIQUE index on the name column (i.e., CREATE UNIQUE INDEX or ALTER TABLE ... ADD CONSTRAINT ... UNIQUE for model_preferences.name) and update the schema definition in schema.ts to use uniqueIndex on the name field so the database constraint and drizzle schema remain in sync (adjust any tests or seed data that assume duplicate names).apps/desktop/src/views/SettingsView/components/General/index.vue (1)
364-372:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRevert optimistic toggle state when persistence fails.
The toggle is flipped before saving, but failure handling only shows an error. This can leave UI state out of sync with stored settings until reload.
Proposed fix
-const saveAllowModelAutoSwitch = async () => { +const saveAllowModelAutoSwitch = async (previousValue: boolean) => { try { await settingsStore.updateAllowModelAutoSwitch(settings.value.allowModelAutoSwitch); alertMessage.value?.success(t('common.saved'), 2000); } catch (error) { + settings.value.allowModelAutoSwitch = previousValue; console.error('Failed to save allow_model_auto_switch setting:', error); alertMessage.value?.error(t('settings.general.saveSettingsFailed'), 3000); } };Call-site adjustment at lines 672-675:
`@click`=" + const previousValue = settings.allowModelAutoSwitch; settings.allowModelAutoSwitch = !settings.allowModelAutoSwitch; - saveAllowModelAutoSwitch(); + saveAllowModelAutoSwitch(previousValue); "🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/views/SettingsView/components/General/index.vue` around lines 364 - 372, The saveAllowModelAutoSwitch handler currently assumes persistence succeeds but doesn't revert the UI when it fails; capture the previous value of settings.value.allowModelAutoSwitch before attempting the optimistic change, and in saveAllowModelAutoSwitch's catch block restore that previous value (e.g., settings.value.allowModelAutoSwitch = previousValue) before calling alertMessage.value?.error(...); keep the existing console.error and success path unchanged and reference the saveAllowModelAutoSwitch function and settingsStore.updateAllowModelAutoSwitch to locate where to add the revert logic.apps/desktop/src/database/queries/models.ts (1)
138-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
priorityfor preference ordering and deterministic lookup.Both
listModelPreferencesand the name lookup currently sort byid, so the configuredpriorityfield never affects routing precedence. Order bypriority(withidas a stable tiebreaker) in both queries.Proposed fix
export const listModelPreferences = async (): Promise<ModelPreferenceWithModel[]> => (await db .select(modelPreferenceWithModelSelection) .from(modelPreferences) .leftJoin(models, eq(models.id, modelPreferences.model_id)) .leftJoin(providers, eq(providers.id, models.provider_id)) - .orderBy(asc(modelPreferences.id)) + .orderBy(asc(modelPreferences.priority), asc(modelPreferences.id)) .all()) as ModelPreferenceWithModel[];Apply the same fix to
findModelPreferenceByNameat lines 150-157:.where(eq(modelPreferences.name, name.trim())) - .orderBy(asc(modelPreferences.id)) + .orderBy(asc(modelPreferences.priority), asc(modelPreferences.id)) .limit(1)As per coding guidelines, past review comments flagged this same issue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/database/queries/models.ts` around lines 138 - 145, listModelPreferences and findModelPreferenceByName are currently ordering by modelPreferences.id, so the configured priority field is ignored; change both queries to order first by modelPreferences.priority (ascending) and then by modelPreferences.id (ascending) as a stable tiebreaker so routing respects priority. Locate listModelPreferences and findModelPreferenceByName in models.ts and update the .orderBy(...) call to use asc(modelPreferences.priority), asc(modelPreferences.id) instead of asc(modelPreferences.id).apps/desktop/src/views/SettingsView/components/General/components/ModelPreferenceDialog.vue (1)
143-146:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid coercing an empty provider fallback to
0.Line 145 can set
selectedProviderIdto0when no providers exist (Number(null) === 0), producing an invalid selected state.Proposed fix
function initializeProvider() { const providerId = selectedModel.value?.provider_id ?? null; - selectedProviderId.value = providerId ?? Number(providerOptions.value[0]?.value ?? null); + const firstProviderId = providerOptions.value[0]?.value; + selectedProviderId.value = + providerId ?? (firstProviderId !== undefined ? Number(firstProviderId) : null); }As per coding guidelines, past review comments flagged this same issue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/views/SettingsView/components/General/components/ModelPreferenceDialog.vue` around lines 143 - 146, initializeProvider currently coerces null to 0 via Number(providerOptions.value[0]?.value ?? null), which can set selectedProviderId to 0 when no providers exist; update the assignment in initializeProvider (referencing selectedProviderId, selectedModel, providerOptions) to only parse/convert the first provider option when it actually exists (e.g., check providerOptions.value[0]?.value before Number/parseInt) and otherwise leave selectedProviderId.value as null (or undefined) so an empty provider list does not produce an invalid 0 selection.apps/desktop/src/i18n/messages.ts (1)
605-605:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a namespaced i18n key instead of a literal Chinese key.
The message key
模型已是目标模型breaks the project's namespacing convention. Consumers expect a stable, namespaced key such asconversation.approval.modelAlreadyTarget.Proposed fix
- 模型已是目标模型: '模型已是目标模型', + 'conversation.approval.modelAlreadyTarget': '模型已是目标模型',And for the English locale at line 1400:
- 模型已是目标模型: 'Model is already the target model', + 'conversation.approval.modelAlreadyTarget': 'Model is already the target model',As per coding guidelines, past review comments flagged this same issue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/i18n/messages.ts` at line 605, Replace the literal Chinese i18n key "模型已是目标模型" in the messages.ts object with a namespaced key (e.g. conversation.approval.modelAlreadyTarget) and keep the existing Chinese string as the value; then add/update the corresponding English locale entry to use the same namespaced key with the English copy ("Model is already the target model") so both locales share the stable key; update any usages that referenced the old literal key to reference conversation.approval.modelAlreadyTarget (search for the string "模型已是目标模型" to find all call sites).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/services/BuiltInToolService/tools/upgradeModel/helper.ts`:
- Around line 11-15: The exported type UpgradeModelArgs is currently handwritten
and can drift from the runtime schema; change it to be derived from the Zod
schema by replacing the manual declaration with a z.infer of
upgradeModelArgsSchema (i.e., export type UpgradeModelArgs = z.infer<typeof
upgradeModelArgsSchema>) and ensure z is imported where UpgradeModelArgs is
defined so the type and schema remain in sync.
---
Duplicate comments:
In `@apps/desktop/src/database/drizzle/0002_model_preferences.sql`:
- Line 16: The migration currently creates a non-unique index
model_preferences_name_idx on model_preferences; change the migration to enforce
uniqueness by replacing that index with a UNIQUE index on the name column (i.e.,
CREATE UNIQUE INDEX or ALTER TABLE ... ADD CONSTRAINT ... UNIQUE for
model_preferences.name) and update the schema definition in schema.ts to use
uniqueIndex on the name field so the database constraint and drizzle schema
remain in sync (adjust any tests or seed data that assume duplicate names).
In `@apps/desktop/src/database/queries/models.ts`:
- Around line 138-145: listModelPreferences and findModelPreferenceByName are
currently ordering by modelPreferences.id, so the configured priority field is
ignored; change both queries to order first by modelPreferences.priority
(ascending) and then by modelPreferences.id (ascending) as a stable tiebreaker
so routing respects priority. Locate listModelPreferences and
findModelPreferenceByName in models.ts and update the .orderBy(...) call to use
asc(modelPreferences.priority), asc(modelPreferences.id) instead of
asc(modelPreferences.id).
In `@apps/desktop/src/i18n/messages.ts`:
- Line 605: Replace the literal Chinese i18n key "模型已是目标模型" in the messages.ts
object with a namespaced key (e.g. conversation.approval.modelAlreadyTarget) and
keep the existing Chinese string as the value; then add/update the corresponding
English locale entry to use the same namespaced key with the English copy
("Model is already the target model") so both locales share the stable key;
update any usages that referenced the old literal key to reference
conversation.approval.modelAlreadyTarget (search for the string "模型已是目标模型" to
find all call sites).
In
`@apps/desktop/src/views/SettingsView/components/General/components/ModelPreferenceDialog.vue`:
- Around line 143-146: initializeProvider currently coerces null to 0 via
Number(providerOptions.value[0]?.value ?? null), which can set
selectedProviderId to 0 when no providers exist; update the assignment in
initializeProvider (referencing selectedProviderId, selectedModel,
providerOptions) to only parse/convert the first provider option when it
actually exists (e.g., check providerOptions.value[0]?.value before
Number/parseInt) and otherwise leave selectedProviderId.value as null (or
undefined) so an empty provider list does not produce an invalid 0 selection.
In `@apps/desktop/src/views/SettingsView/components/General/index.vue`:
- Around line 364-372: The saveAllowModelAutoSwitch handler currently assumes
persistence succeeds but doesn't revert the UI when it fails; capture the
previous value of settings.value.allowModelAutoSwitch before attempting the
optimistic change, and in saveAllowModelAutoSwitch's catch block restore that
previous value (e.g., settings.value.allowModelAutoSwitch = previousValue)
before calling alertMessage.value?.error(...); keep the existing console.error
and success path unchanged and reference the saveAllowModelAutoSwitch function
and settingsStore.updateAllowModelAutoSwitch to locate where to add the revert
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0995bd45-8c33-4621-b9c2-7adc7e33ec72
📒 Files selected for processing (41)
.prettierignoreapps/desktop/src-tauri/resources/rtk/manifest.jsonapps/desktop/src/components/AlertMessage.vueapps/desktop/src/components/appIconMap.tsapps/desktop/src/database/drizzle/0002_model_preferences.sqlapps/desktop/src/database/drizzle/meta/_journal.jsonapps/desktop/src/database/queries/models.tsapps/desktop/src/database/schema.tsapps/desktop/src/database/types/index.tsapps/desktop/src/i18n/messages.tsapps/desktop/src/i18n/textMap.tsapps/desktop/src/services/AgentService/prompt/composer.tsapps/desktop/src/services/AgentService/prompt/modelPreferences.tsapps/desktop/src/services/BuiltInToolService/tools/upgradeModel/constants.tsapps/desktop/src/services/BuiltInToolService/tools/upgradeModel/helper.tsapps/desktop/src/services/BuiltInToolService/tools/upgradeModel/index.tsapps/desktop/src/services/EventService/types.tsapps/desktop/src/stores/settings.tsapps/desktop/src/views/SettingsView/components/AiServices/components/ModelCard.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ModelGroup.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ModelList.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ProviderCard.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ProviderList.vueapps/desktop/src/views/SettingsView/components/AiServices/index.vueapps/desktop/src/views/SettingsView/components/General/components/ModelPreferenceDialog.vueapps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vueapps/desktop/src/views/SettingsView/components/General/index.vueapps/desktop/src/views/SettingsView/components/ModelPreferences/index.vueapps/desktop/src/views/SettingsView/index.vueapps/desktop/src/views/SettingsView/settingsNavigation.tsapps/desktop/tests/SettingsView/ai-services-i18n.test.tsapps/desktop/tests/SettingsView/model-card-i18n.test.tsapps/desktop/tests/SettingsView/navigation-sidebar-i18n.test.tsapps/desktop/tests/services/AgentService/prompt/model-preferences.test.tsapps/desktop/tests/services/BuiltInToolService/tools/upgradeModel/i18n.test.tsapps/desktop/tests/views/SettingsView/modelGroupBehavior.test.tsapps/desktop/tests/views/SettingsView/modelListEmptyState.test.tsapps/desktop/tests/views/SettingsView/settingsAiServicesLayout.test.tsapps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.tsapps/desktop/tests/views/SettingsView/settingsNavigation.test.tseslint.config.js
💤 Files with no reviewable changes (1)
- apps/desktop/src/views/SettingsView/components/AiServices/components/ProviderList.vue
📜 Review details
⏰ 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). (4)
- GitHub Check: Desktop E2E Smoke (Windows)
- GitHub Check: Rust Checks
- GitHub Check: Frontend Tests
- GitHub Check: CodeQL (rust)
🔇 Additional comments (44)
.prettierignore (1)
55-56: LGTM!apps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts (1)
190-190: LGTM!apps/desktop/tests/views/SettingsView/modelListEmptyState.test.ts (1)
29-29: LGTM!apps/desktop/src/views/SettingsView/components/ModelPreferences/index.vue (1)
1-15: LGTM!apps/desktop/tests/SettingsView/navigation-sidebar-i18n.test.ts (1)
59-59: LGTM!apps/desktop/tests/views/SettingsView/modelGroupBehavior.test.ts (1)
67-67: LGTM!Also applies to: 74-74, 104-104, 112-112, 125-125, 139-139
apps/desktop/src-tauri/resources/rtk/manifest.json (1)
8-8: LGTM!eslint.config.js (1)
63-66: LGTM!apps/desktop/src/i18n/textMap.ts (1)
704-704: LGTM!apps/desktop/tests/SettingsView/model-card-i18n.test.ts (2)
85-85: LGTM!Also applies to: 101-101, 128-128, 146-146
122-138: LGTM!apps/desktop/tests/services/AgentService/prompt/model-preferences.test.ts (1)
1-119: LGTM!apps/desktop/tests/views/SettingsView/settingsAiServicesLayout.test.ts (1)
239-325: LGTM!apps/desktop/tests/SettingsView/ai-services-i18n.test.ts (1)
185-202: LGTM!Also applies to: 204-227, 229-246, 303-322
apps/desktop/src/views/SettingsView/index.vue (2)
27-29: LGTM!Also applies to: 46-46, 50-50
262-277: LGTM!apps/desktop/src/services/AgentService/prompt/modelPreferences.ts (2)
7-9: Backslash escaping is already handled here.Line 8 escapes
\\before pipes/newlines, addressing the prior CodeQL/escaping finding. No further change needed.
3-3: ⚡ Quick winConfirm
findModelRoleWithProviderandlistModelPreferencesare exported from@database/queries. The repo doesn’t contain adatabase/queries/directory at that path, so ensure the@database/queriesalias resolves to the actual barrel/index file that re-exports those exact names to avoid a runtime import/export mismatch.apps/desktop/src/services/EventService/types.ts (1)
77-77: LGTM!apps/desktop/tests/services/BuiltInToolService/tools/upgradeModel/i18n.test.ts (1)
15-315: LGTM!apps/desktop/tests/views/SettingsView/settingsNavigation.test.ts (1)
18-39: LGTM!apps/desktop/src/services/AgentService/prompt/composer.ts (1)
95-109: LGTM!apps/desktop/src/views/SettingsView/settingsNavigation.ts (1)
7-63: LGTM!apps/desktop/src/components/appIconMap.ts (1)
11-11: LGTM!Also applies to: 41-41, 53-53, 79-79
apps/desktop/src/stores/settings.ts (1)
31-31: LGTM!Also applies to: 45-45, 132-135, 168-169, 195-196, 228-228, 239-239, 258-261, 276-276, 385-387, 440-440
apps/desktop/src/services/BuiltInToolService/tools/upgradeModel/constants.ts (1)
34-38: ⚡ Quick winKeep
input_schemaaligned with runtime validation.
upgradeModelArgsSchemarejects empty/whitespace-onlyscenario, but the exposedinput_schemastill permits any string, which can surface as tool-call validation failures.Proposed fix
scenario: { type: ['string', 'null'], + pattern: '^(?!\\s*$).+', description: 'Optional scenario name from the configured model preferences. Use null to restore the entry model.', },apps/desktop/src/services/BuiltInToolService/tools/upgradeModel/index.ts (2)
236-255: ⚡ Quick winReject conflicting target selectors instead of silently prioritizing.
When callers pass multiple selectors (e.g.
role+scenario), precedence silently picks one, risking an unintended switch. Fail fast when more than one ofrestore/scenario===null,role, and a non-emptyscenariois present.
280-293: ⚡ Quick winWrap new user-facing strings with
tt(...).The switch-summary actions ("Restored entry model", "Switched model role/scenario", "Current model", "Target model", trailing sentence) and the thrown errors in
resolveScenarioTarget/resolveRestoreTarget/resolveRoleTargetare hardcoded English, while adjacent output usestt(...). This yields mixed-language responses under non-English locales.Also applies to: 167-175, 195, 214.
apps/desktop/src/components/AlertMessage.vue (1)
4-72: LGTM!apps/desktop/src/views/SettingsView/components/AiServices/components/ModelGroup.vue (1)
18-114: LGTM!apps/desktop/src/database/drizzle/meta/_journal.json (1)
19-25: LGTM!apps/desktop/src/views/SettingsView/components/AiServices/components/ModelCard.vue (1)
13-48: LGTM!apps/desktop/src/views/SettingsView/components/AiServices/index.vue (1)
77-78: LGTM!Also applies to: 165-189, 285-303, 427-442, 480-504, 662-673
apps/desktop/src/database/types/index.ts (1)
256-286: LGTM!apps/desktop/src/views/SettingsView/components/AiServices/components/ProviderCard.vue (1)
28-30: LGTM!apps/desktop/src/database/schema.ts (2)
293-319: LGTM!
615-617: LGTM!apps/desktop/src/views/SettingsView/components/AiServices/components/ModelList.vue (1)
14-21: LGTM!Also applies to: 59-177, 319-319
apps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vue (1)
1-261: LGTM!apps/desktop/src/i18n/messages.ts (1)
123-124: LGTM!Also applies to: 156-156, 164-167, 246-246, 282-319
apps/desktop/src/views/SettingsView/components/General/components/ModelPreferenceDialog.vue (1)
54-63: LGTM!Also applies to: 65-84, 86-110, 112-142, 148-177, 179-201, 203-423
apps/desktop/src/database/queries/models.ts (3)
3-3: LGTM!Also applies to: 8-8, 25-30, 63-77
127-136: LGTM!Also applies to: 160-202, 204-227
312-366: LGTM!
KagurazakaSakuya
force-pushed
the
feat/issue-139-model-preferences
branch
from
6035ef7 to
5cc3a92
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/services/AgentService/prompt/modelPreferences.ts`:
- Line 85: The guidance string in modelPreferences.ts incorrectly recommends
using `{ "scenario": null }` for restore which conflicts with string-based
scenario routing; update the message (the string containing 'Use
`builtin__upgrade_model` with ...') to remove the `{ "scenario": null }`
suggestion and only mention using `{ "restore": true }` and `{ "role": "entry"
}` for returning to the entry model so it matches the `builtin__upgrade_model`
argument contract.
In
`@apps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vue`:
- Around line 212-218: The current validation for selectedModel (using modelId,
selectedModel and models.value) accepts models even if they or their provider
are disabled; update the selection check to reject disabled models by ensuring
the found model exists AND model.enabled !== false AND (model.provider?.enabled
!== false) (or the equivalent flags your model objects use), and if any of those
checks fail keep the existing
alert.error(t('settings.general.modelPreferences.modelRequired')) and return;
locate the logic around modelId/selectedModel in ModelPreferences.vue and add
those enabled checks to prevent persisting disabled targets.
In `@apps/desktop/src/views/SettingsView/components/General/index.vue`:
- Around line 364-379: The toggle can trigger overlapping saves; add an
in-flight guard (e.g., isSavingAllowModelAutoSwitch ref/boolean) to
saveAllowModelAutoSwitch and toggleAllowModelAutoSwitch so clicks while a save
is pending are ignored or queued: in toggleAllowModelAutoSwitch, return early if
isSavingAllowModelAutoSwitch is true, set it to true before calling
settingsStore.updateAllowModelAutoSwitch in saveAllowModelAutoSwitch, and always
set it back to false in finally; ensure previousValue handling remains the same
and use the guard in both saveAllowModelAutoSwitch and the other similar block
mentioned (lines ~670-680) to prevent out-of-order persistence.
- Around line 670-680: The auto-switch toggle button lacks an accessible name;
update the button (the element using settings.allowModelAutoSwitch and
`@click`="toggleAllowModelAutoSwitch", with
data-testid="settings-allow-model-auto-switch-toggle") to include an explicit
accessible label—add an aria-label (or aria-labelledby pointing to a visible
label) that describes the control (e.g., "Enable model auto-switch" or use your
i18n string key) so screen readers can announce its purpose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9e38ccbf-0960-415b-8534-77474e71d0da
📒 Files selected for processing (40)
.prettierignoreapps/desktop/src-tauri/resources/rtk/manifest.jsonapps/desktop/src/components/AlertMessage.vueapps/desktop/src/components/appIconMap.tsapps/desktop/src/database/drizzle/0002_model_preferences.sqlapps/desktop/src/database/drizzle/meta/_journal.jsonapps/desktop/src/database/queries/models.tsapps/desktop/src/database/schema.tsapps/desktop/src/database/types/index.tsapps/desktop/src/i18n/messages.tsapps/desktop/src/services/AgentService/prompt/composer.tsapps/desktop/src/services/AgentService/prompt/modelPreferences.tsapps/desktop/src/services/BuiltInToolService/tools/upgradeModel/constants.tsapps/desktop/src/services/BuiltInToolService/tools/upgradeModel/helper.tsapps/desktop/src/services/BuiltInToolService/tools/upgradeModel/index.tsapps/desktop/src/services/EventService/types.tsapps/desktop/src/stores/settings.tsapps/desktop/src/views/SettingsView/components/AiServices/components/ModelCard.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ModelGroup.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ModelList.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ProviderCard.vueapps/desktop/src/views/SettingsView/components/AiServices/components/ProviderList.vueapps/desktop/src/views/SettingsView/components/AiServices/index.vueapps/desktop/src/views/SettingsView/components/General/components/ModelPreferenceDialog.vueapps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vueapps/desktop/src/views/SettingsView/components/General/index.vueapps/desktop/src/views/SettingsView/components/ModelPreferences/index.vueapps/desktop/src/views/SettingsView/index.vueapps/desktop/src/views/SettingsView/settingsNavigation.tsapps/desktop/tests/SettingsView/ai-services-i18n.test.tsapps/desktop/tests/SettingsView/model-card-i18n.test.tsapps/desktop/tests/SettingsView/navigation-sidebar-i18n.test.tsapps/desktop/tests/services/AgentService/prompt/model-preferences.test.tsapps/desktop/tests/services/BuiltInToolService/tools/upgradeModel/i18n.test.tsapps/desktop/tests/views/SettingsView/modelGroupBehavior.test.tsapps/desktop/tests/views/SettingsView/modelListEmptyState.test.tsapps/desktop/tests/views/SettingsView/settingsAiServicesLayout.test.tsapps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.tsapps/desktop/tests/views/SettingsView/settingsNavigation.test.tseslint.config.js
💤 Files with no reviewable changes (1)
- apps/desktop/src/views/SettingsView/components/AiServices/components/ProviderList.vue
📜 Review details
⏰ 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). (2)
- GitHub Check: CodeQL (rust)
- GitHub Check: Desktop E2E Smoke (Windows)
🔇 Additional comments (45)
apps/desktop/src/components/AlertMessage.vue (1)
4-5: LGTM!Also applies to: 7-23, 37-37, 41-46, 65-65, 67-75, 78-78
.prettierignore (1)
55-56: LGTM!eslint.config.js (1)
63-66: LGTM!apps/desktop/src-tauri/resources/rtk/manifest.json (1)
8-8: Confirm Windowsbinary_digestmatches upstream artifact
apps/desktop/src-tauri/resources/rtk/manifest.jsonline 8: the updatedbinary_digest(3f8786f4bc120a10dd83728d4e481e74460fede13e811c9d8849098a48d0b33e) matches the SHA256 ofrtk.exeextracted fromv0.40.0/rtk-x86_64-pc-windows-msvc.zipfromrtk-ai/rtk(and the archive SHA256 also matches the expected value), indicating an intentional correction rather than manifest drift.apps/desktop/src/database/drizzle/0002_model_preferences.sql (1)
1-20: LGTM!apps/desktop/src/database/drizzle/meta/_journal.json (1)
18-24: LGTM!apps/desktop/src/database/schema.ts (1)
293-319: LGTM!Also applies to: 615-617
apps/desktop/src/database/types/index.ts (1)
256-285: LGTM!apps/desktop/src/database/queries/models.ts (1)
3-31: LGTM!Also applies to: 63-77, 127-227, 312-367
apps/desktop/src/services/AgentService/prompt/modelPreferences.ts (1)
7-84: LGTM!Also applies to: 86-100
apps/desktop/src/services/AgentService/prompt/composer.ts (1)
15-15: LGTM!Also applies to: 95-95, 106-109
apps/desktop/tests/SettingsView/ai-services-i18n.test.ts (1)
185-197: LGTM!Also applies to: 211-212, 236-237, 317-318
apps/desktop/tests/SettingsView/model-card-i18n.test.ts (1)
85-86: LGTM!Also applies to: 101-102, 122-137, 146-147
apps/desktop/tests/views/SettingsView/modelGroupBehavior.test.ts (1)
67-68: LGTM!Also applies to: 74-75, 104-105, 112-113, 125-126, 139-140
apps/desktop/tests/views/SettingsView/modelListEmptyState.test.ts (1)
29-30: LGTM!apps/desktop/tests/views/SettingsView/settingsAiServicesLayout.test.ts (1)
239-240: LGTM!Also applies to: 291-299, 314-325
apps/desktop/tests/SettingsView/navigation-sidebar-i18n.test.ts (1)
59-59: LGTM!apps/desktop/tests/services/AgentService/prompt/model-preferences.test.ts (1)
1-118: LGTM!apps/desktop/tests/services/BuiltInToolService/tools/upgradeModel/i18n.test.ts (1)
15-29: LGTM!Also applies to: 32-37, 85-85, 206-333
apps/desktop/tests/views/SettingsView/settingsGeneralComponent.test.ts (1)
17-17: LGTM!Also applies to: 29-29, 157-159, 194-194, 236-250
apps/desktop/tests/views/SettingsView/settingsNavigation.test.ts (1)
18-18: LGTM!Also applies to: 34-39
apps/desktop/src/views/SettingsView/components/AiServices/components/ModelCard.vue (1)
15-15: LGTM!Also applies to: 32-34
apps/desktop/src/views/SettingsView/components/AiServices/components/ModelGroup.vue (1)
20-20: LGTM!Also applies to: 44-50, 108-108
apps/desktop/src/views/SettingsView/components/AiServices/components/ModelList.vue (1)
17-17: LGTM!Also applies to: 59-59, 61-61, 72-74, 81-82, 111-113, 122-124, 136-138, 176-176, 319-319
apps/desktop/src/views/SettingsView/components/AiServices/components/ProviderCard.vue (1)
29-29: LGTM!apps/desktop/src/views/SettingsView/components/AiServices/index.vue (1)
77-78: LGTM!Also applies to: 173-174, 286-301, 432-435, 488-497, 665-665
apps/desktop/src/components/appIconMap.ts (1)
11-11: LGTM!Also applies to: 41-41, 53-53, 79-79
apps/desktop/src/views/SettingsView/settingsNavigation.ts (1)
7-7: LGTM!Also applies to: 54-54, 58-63
apps/desktop/src/services/BuiltInToolService/tools/upgradeModel/helper.ts (2)
11-15: DeriveUpgradeModelArgsfromupgradeModelArgsSchemato avoid schema/type drift.The manual type definition duplicates the Zod schema structure. Zod 4 supports
z.infer<typeof schema>to keep the type and runtime validation in sync.
49-51: LGTM!apps/desktop/src/services/BuiltInToolService/tools/upgradeModel/constants.ts (3)
8-14: LGTM!
19-20: LGTM!
25-48: LGTM!apps/desktop/src/services/BuiltInToolService/tools/upgradeModel/index.ts (12)
3-14: LGTM!
46-52: LGTM!
54-61: LGTM!
63-70: LGTM!
164-196: LGTM!
198-215: LGTM!
237-270: LGTM!
279-310: LGTM!
328-337: LGTM!
369-428: LGTM!
449-457: LGTM!
217-235: ⚡ Quick win{role: "entry"} and {restore: true} should resolve to the same entry model—confirm shared lookup logic.
There are explicit
if (role === 'entry')branches inapps/desktop/src/database/queries/models.ts, but the implementation offindDefaultModelWithProvider/findEffectiveModelRoleWithProviderwasn’t found in the current code search results, so it’s unclear thatresolveRestoreTargetandresolveRoleTarget('entry')always pick the identical model. Ensure both paths use the same “default entry model” source of truth.
…39-model-preferences # Conflicts: # apps/desktop/src/database/drizzle/meta/_journal.json # apps/desktop/src/database/queries/models.ts # apps/desktop/tests/views/SettingsView/modelGroupBehavior.test.ts # apps/desktop/tests/views/SettingsView/modelListEmptyState.test.ts # apps/desktop/tests/views/SettingsView/settingsNavigation.test.ts
There was a problem hiding this comment.
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 (1)
apps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vue (1)
255-284:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid flickering the scenario list on every role save/refresh.
loadData()always setsloading = true, and it's invoked after everysaveModelRole,savePreference, andremovePreference. Sinceloadinggates the scenario-preferences list (templatev-if="loading"), changing a role model briefly flashes an unrelated section to "loading…" on each save. Gate the spinner to the initial load only.Proposed fix
- async function loadData() { - loading.value = true; + async function loadData({ showLoading = true }: { showLoading?: boolean } = {}) { + if (showLoading) { + loading.value = true; + } try {Then pass
{ showLoading: false }from the post-mutation refresh calls (saveModelRole,savePreference,removePreference), keepingonMountedon the default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vue` around lines 255 - 284, loadData() currently always sets loading.value = true which causes the spinner to flash on every post-mutation refresh; change loadData to accept an options param (e.g., loadData({ showLoading = true } = {})) and only set loading.value = true when showLoading is true, then update post-mutation callers (saveModelRole, savePreference, removePreference) to call loadData({ showLoading: false }) while leaving the onMounted call as loadData() so initial load still shows the spinner; ensure the new signature is used consistently and types/defaults are handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vue`:
- Around line 70-79: Multiple components duplicate the same provider-logo
globbing and path resolution logic (rawProviderLogos, providerLogos and
resolveProviderLogoPath). Extract a shared helper (e.g., a composable or util
like useProviderLogos or resolveProviderLogoPathUtil) that runs
import.meta.glob<{ default: string }>('`@assets/logos/providers/`*', { eager: true
}), builds the providerLogos map, and exposes a resolveProviderLogoPath(name)
function; then replace the duplicated code in ModelPreferences.vue,
ModelPreferenceDialog.vue, BadgedLogo.vue, AddProviderDialog.vue,
EditProviderDialog.vue and UpgradeModelToolConfig.vue to import and use that
shared helper. Ensure the helper returns the same record shape
(Record<string,string>) and the same behavior when a file is missing so callers
remain compatible.
---
Outside diff comments:
In
`@apps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vue`:
- Around line 255-284: loadData() currently always sets loading.value = true
which causes the spinner to flash on every post-mutation refresh; change
loadData to accept an options param (e.g., loadData({ showLoading = true } =
{})) and only set loading.value = true when showLoading is true, then update
post-mutation callers (saveModelRole, savePreference, removePreference) to call
loadData({ showLoading: false }) while leaving the onMounted call as loadData()
so initial load still shows the spinner; ensure the new signature is used
consistently and types/defaults are handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dce24e21-1c95-4c53-aba2-8541ed69c056
📒 Files selected for processing (8)
apps/desktop/src/components/appIconMap.tsapps/desktop/src/database/drizzle/0003_model_preferences.sqlapps/desktop/src/database/drizzle/meta/_journal.jsonapps/desktop/src/database/queries/models.tsapps/desktop/src/database/schema.tsapps/desktop/src/i18n/messages.tsapps/desktop/src/stores/settings.tsapps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vue
💤 Files with no reviewable changes (1)
- apps/desktop/src/database/drizzle/0003_model_preferences.sql
📜 Review details
🔇 Additional comments (24)
apps/desktop/src/database/schema.ts (2)
293-319: LGTM!Also applies to: 624-626
560-580: LGTM!apps/desktop/src/database/queries/models.ts (9)
25-30: LGTM!
63-77: LGTM!
78-102: LGTM!
152-170: LGTM!
172-183: LGTM!
185-227: LGTM!
229-252: LGTM!
337-391: LGTM!
418-512: LGTM!apps/desktop/src/views/SettingsView/components/General/components/ModelPreferences.vue (5)
111-120: LGTM!
210-244: LGTM!
323-372: LGTM!
379-660: LGTM!
145-148:provider_driverandprovider_logoare non-null onModelWithProvider
ModelWithProviderdefinesprovider_driver: DbProviderDriverandprovider_logo: string, and the backingproviderstable columnsdriverandlogoare both.notNull(), so the"undefined"/"null"interpolation concern doesn’t apply to the real data contract.apps/desktop/src/stores/settings.ts (4)
31-31: LGTM!Also applies to: 45-45
117-124: LGTM!
141-144: LGTM!Also applies to: 177-178, 204-205
237-237: LGTM!Also applies to: 248-248, 267-270, 285-285, 322-329, 400-402, 455-455
apps/desktop/src/i18n/messages.ts (2)
123-124: LGTM!Also applies to: 156-156, 164-167, 914-916, 952-952, 960-963
282-319: LGTM!Also applies to: 293-293, 427-444, 623-635, 1087-1127, 1100-1100, 1243-1260, 1448-1464
apps/desktop/src/components/appIconMap.ts (2)
28-28: Summary inconsistency: arrow icons not mentioned.The AI-generated summary claims only
cloudandroutewere added, but the diff showsarrow-leftandarrow-rightwere also added (lines 28, 37, 50-51). The implementation is correct; the summary is incomplete.Also applies to: 37-37, 50-51
11-11: LGTM!Also applies to: 28-28, 37-37, 43-43, 50-51, 57-57, 83-83
…39-model-preferences # Conflicts: # apps/desktop/src/database/queries/models.ts
3 participants
Summary
Add model settings for role-based and scenario-based model switching.
This PR lets users configure:
The assistant receives the configured model settings in the system prompt and can use the built-in model switching tool to move to a better-fit model when the task clearly matches a configured role or scenario.
User-facing impact:
Related issue or RFC
Link the issue or RFC:
For code changes, link the tracking issue. Only documentation wording, link fixes, or comment-only cleanups may skip the issue-first flow.
AI assistance disclosure
If AI tools materially assisted this contribution, disclose that here or point to the relevant commit trailer.
Testing evidence
List the commands you ran and the results you observed.
pnpm test:pr(includes type check, lint, format, all tests, and frontend coverage)pnpm test:coverage:rust(requires cargo-tarpaulin, or rely on CI)pnpm test:e2eDid you follow TDD (test-first) for feature and fix work? Strongly recommended. See docs/testing/testing.md.
Results:
corepack pnpm test:pr passed locally before the final rebase onto the latest branch head.
After rebase conflict resolution, corepack pnpm --filter @touchai/desktop type:check passed.
lint:check reported warnings only, with 0 errors.
Risk notes
AgentService, runtime, MCP, or schema impact: Adds model settings prompt injection and extends built-in model switching behavior. MCP integration is not directly changed.model_preferencestable migration and related model preference query APIs.Screenshots or recordings
Checklist
[WIP]or similar title prefixes.AgentService, runtime, MCP, or schema boundaries, there is an accepted RFC.pnpm test:prfor this code PR, or this is a docs-only change.pnpm test:coverage:rustor relied on CI coverage evidence.pnpm test:e2elocally or documented why CI is the first valid proof.