refactor(model): derive selectable model source - #1575
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes chat model selection into new chat-selectable model groups and resolver utilities, extracts ACP chat-status UI/state into a composable, updates stores/components to consume the new APIs, and adds provider cleanup and agent-scoped agent-change routing/notifications. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatStatusBar
participant Composable as useChatStatusBarAcpConfig
participant ModelStore
participant ConfigClient
Note over User,ChatStatusBar: User opens ACP options or selects a model
User->>ChatStatusBar: open / select option
ChatStatusBar->>Composable: request options / update option
Composable->>ConfigClient: listAgents / setAcpSessionConfigOption
ConfigClient-->>Composable: agents / update ack
Composable->>ModelStore: query chatSelectableModelGroups / resolve model
ModelStore-->>Composable: model groups / resolved model
Composable-->>ChatStatusBar: updated ACP state / resolved {providerId,modelId}
ChatStatusBar-->>User: reflect new selection/state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/stores/modelStore.ts (1)
350-366:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent removed providers from being re-materialized by late refresh writes.
pruneModelState(Line 385) removes deleted providers, but an in-flight refresh can still finish later and re-add them viaupdateCustomModelState/updateAllProviderState(Line 350 / Line 359), because those writers don’t validate provider existence.Suggested guard to keep state aligned with current provider catalog
const updateCustomModelState = (providerId: string, models: RENDERER_MODEL_META[]) => { + if (!getProviderState(providerId)) { + customModels.value = customModels.value.filter((item) => item.providerId !== providerId) + return + } const customIndex = customModels.value.findIndex((item) => item.providerId === providerId) if (customIndex !== -1) { customModels.value[customIndex].models = models } else { customModels.value.push({ providerId, models }) } } const updateAllProviderState = (providerId: string, models: RENDERER_MODEL_META[]) => { + if (!getProviderState(providerId)) { + allProviderModels.value = allProviderModels.value.filter((item) => item.providerId !== providerId) + return + } const idx = allProviderModels.value.findIndex((item) => item.providerId === providerId) if (idx !== -1) { allProviderModels.value[idx].models = models } else { allProviderModels.value.push({ providerId, models }) } }Also applies to: 385-393, 1178-1213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/stores/modelStore.ts` around lines 350 - 366, The updateCustomModelState and updateAllProviderState functions can re-add providers removed by pruneModelState; before updating or pushing into customModels or allProviderModels, verify the provider still exists in the current provider catalog (e.g., check providers.value for an entry with matching providerId) and bail out if not found; apply the same guard pattern to any other writers that mutate provider lists (locations referenced by pruneModelState and the block around 1178-1213) so late refreshes cannot re-materialize deleted providers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/renderer/components/ChatStatusBar.test.ts`:
- Around line 305-311: The current findChatSelectableModel implementation only
searches the first group returned by getChatSelectableModelGroups() for a
matching providerId, so if a providerId appears in multiple groups it can miss
valid models; update findChatSelectableModel to search all groups with
providerId (e.g., iterate/filter all entries from getChatSelectableModelGroups()
where entry.providerId === providerId and then find the model with id ===
modelId) and return the providerName from the specific group that contained the
model, returning null only if no group contains that model.
In `@test/renderer/stores/modelStore.test.ts`:
- Around line 63-66: The test setup currently forces every provider to apiType:
'openai' by unconditionally overriding apiType in the providerRecords.map used
to build sortedProviders; change that map so it preserves an existing
provider.apiType and only defaults to 'openai' when apiType is missing (i.e.,
spread provider then set apiType to provider.apiType ?? 'openai' or equivalent),
so providerRecords and the derived sortedProviders retain provider-specific
apiType values for tests that rely on them.
---
Outside diff comments:
In `@src/renderer/src/stores/modelStore.ts`:
- Around line 350-366: The updateCustomModelState and updateAllProviderState
functions can re-add providers removed by pruneModelState; before updating or
pushing into customModels or allProviderModels, verify the provider still exists
in the current provider catalog (e.g., check providers.value for an entry with
matching providerId) and bail out if not found; apply the same guard pattern to
any other writers that mutate provider lists (locations referenced by
pruneModelState and the block around 1178-1213) so late refreshes cannot
re-materialize deleted providers.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 282e8939-1117-45ed-be9c-5a60bac0d49a
📒 Files selected for processing (9)
src/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/mcpSampling.tssrc/renderer/src/stores/modelStore.tstest/renderer/components/ChatStatusBar.test.tstest/renderer/components/NewThreadPage.test.tstest/renderer/pages/NewThreadPage.test.tstest/renderer/stores/mcpSampling.test.tstest/renderer/stores/modelStore.test.ts
* refactor(model): unify selection resolver * refactor(chat): extract acp status bar state
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/configPresenter/index.ts`:
- Around line 1990-1992: The success path that sets agent install state
(getAgentRepositoryOrThrow().setAgentInstallState(registryAgent.id,
installedState)) only calls notifyAcpAgentsChanged([registryAgent.id]) and never
updates the live ACP provider; after install you should call
refreshAcpProviderAgents to ensure the enabled agent appears immediately. Modify
the success branch in the same block (after setAgentInstallState and
notifyAcpAgentsChanged) to also invoke refreshAcpProviderAgents() (or
refreshAcpProviderAgents([registryAgent.id]) if a targeted refresh overload
exists) so the ACP provider is refreshed right after installation completes.
In `@src/main/presenter/configPresenter/modelStatusHelper.ts`:
- Around line 235-250: deleteProviderModelStatuses currently skips removing
persisted keys if candidate.store (raw snapshot) is unavailable; change it to
fall back to another snapshot source and still remove persisted keys: inside
deleteProviderModelStatuses, after computing prefix and candidate, if rawStore
is falsy obtain a key list from the ElectronStore instance (e.g., use
Object.keys((this.store as any).store) or call a safe accessor on this.store to
get all keys), then iterate those keys and call this.store.delete for keys that
startWith(prefix), and finally call clearProviderModelStatusCache(providerId);
reference deleteProviderModelStatuses, MODEL_STATUS_KEY_PREFIX, candidate.store
and clearProviderModelStatusCache.
In `@src/renderer/src/components/chat/composables/useChatStatusBarAcpConfig.ts`:
- Around line 73-75: hasAcpConfigStateData currently returns false for valid
AcpConfigState objects with an empty options array; update it to treat an
AcpConfigState with options: [] as loaded by checking that state is
non-null/undefined and that state.options is an array (e.g.,
Array.isArray(state.options)) rather than checking truthiness of options.length;
apply the same fix to the other similar checks referenced (the isAcpConfigState
usage and the checks around the process fetch path) so empty option lists clear
loading state instead of falling back to cached data.
- Around line 91-92: The ACP config cache is currently keyed only by agent
(acpConfigCacheByAgent) but acpConfigRequestKey includes workspace
(process:${agentId}::${workdir}), so change the cache to use the same scoped
request key string for both reads and writes: derive a single key (reuse
acpConfigRequestKey) and use it when setting/getting acpConfigCacheByAgent and
when returning the cached fallback in syncAcpConfigOptions; update all places
that read/write the cache (including inside syncAcpConfigOptions and any other
spots that reference acpConfigCacheByAgent or seed from it) so the cache is
workspace-scoped and not collapsed across workdirs, leaving acpConfigSyncToken
logic unchanged.
- Around line 199-208: The status label helper getAcpOptionDisplayValue
currently returns option.currentValue for select options before
getAcpOptionCurrentLabel can provide a human-readable label; update
getAcpOptionDisplayValue to check option.type === 'select' and return
getAcpOptionCurrentLabel(option) ?? '' before falling back to returning a raw
string currentValue, keep the boolean branch as-is, and only use the raw
currentValue for non-select/string options so the status bar displays the select
option's label instead of internal/provider value.
In `@src/renderer/src/stores/ui/agent.ts`:
- Around line 138-139: Replace the hardcoded English error messages assigned to
error.value in the refresh logic (the lines setting error.value = `Failed to
refresh ${agentType} agents: ${e}` and the other similar assignment around the
refresh function) with vue-i18n keys and parameters: import or use the existing
i18n translator (e.g., useI18n() or i18n.t) and set error.value =
t('agents.refresh_failed', { type: agentType, error: e?.toString() }) (or the
project's chosen key name), ensuring you add the corresponding key to
src/renderer/src/i18n and pass agentType and error as params; update both
occurrences that reference error.value so all user-facing strings use i18n keys
instead of inline English text.
In `@src/shared/contracts/routes/config.routes.ts`:
- Line 18: AgentSchema currently uses z.custom<Agent>() which performs no
runtime validation; replace it with a proper Zod object schema by either
importing and reusing the existing AgentBootstrapItemSchema from common.ts or by
constructing a z.object(...) schema that matches the Agent shape, then use that
schema where AgentSchema is referenced (e.g., in the config.listAgents route) so
payloads are validated at runtime. Ensure the new AgentSchema has the same
required fields/types as AgentBootstrapItemSchema and update any imports/exports
accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68a11379-7d54-4848-8285-a04cbf40cd55
📒 Files selected for processing (21)
src/main/presenter/configPresenter/index.tssrc/main/presenter/configPresenter/modelStatusHelper.tssrc/main/presenter/configPresenter/providerHelper.tssrc/main/presenter/configPresenter/providerModelHelper.tssrc/main/routes/config/configRouteHandler.tssrc/main/routes/legacyTypedEventBridge.tssrc/renderer/api/ConfigClient.tssrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/components/chat/composables/useChatStatusBarAcpConfig.tssrc/renderer/src/lib/chatModelSelection.tssrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/mcpSampling.tssrc/renderer/src/stores/ui/agent.tssrc/shared/contracts/events/config.events.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/config.routes.tstest/main/presenter/configPresenter/modelStatusHelper.test.tstest/main/presenter/configPresenter/providerHelper.test.tstest/main/presenter/configPresenter/providerModelHelper.test.tstest/renderer/lib/chatModelSelection.test.tstest/renderer/stores/agentStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/src/pages/NewThreadPage.vue
- src/renderer/src/components/chat/ChatStatusBar.vue
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/stores/modelStore.ts (1)
138-154:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftInvalidate in-flight refreshes when a provider is removed.
purgeRemovedProviderState()clears the materialized groups, but an already-runningrefreshStandardModels/refreshCustomModelscan still resume afterwards and write that provider straight back intoallProviderModels,customModels, orenabledModels. Since the removal watch only fires on the catalog change itself, that resurrected state can stick until the next provider update. Please add a provider-generation/tombstone check so refresh code bails before writing state for a removed provider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/stores/modelStore.ts` around lines 138 - 154, purgeRemovedProviderState currently clears stored groups but doesn't prevent concurrently-running refreshStandardModels/refreshCustomModels from re-writing a removed provider; add a tombstone/generation mechanism: update purgeRemovedProviderState to record a removal marker (e.g., increment a providerGeneration entry or add providerId to a removedProviders set), and modify refreshStandardModels and refreshCustomModels to check that marker/generation before committing any writes to allProviderModels, customModels, or enabledModels (and abort the write if the provider is marked removed or generation mismatches). Use the existing providerId keys and the functions providerModelQueries/customModelQueries/enabledModelQueries to locate where writes occur so you can gate them with the tombstone/generation check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/renderer/src/stores/modelStore.ts`:
- Around line 138-154: purgeRemovedProviderState currently clears stored groups
but doesn't prevent concurrently-running
refreshStandardModels/refreshCustomModels from re-writing a removed provider;
add a tombstone/generation mechanism: update purgeRemovedProviderState to record
a removal marker (e.g., increment a providerGeneration entry or add providerId
to a removedProviders set), and modify refreshStandardModels and
refreshCustomModels to check that marker/generation before committing any writes
to allProviderModels, customModels, or enabledModels (and abort the write if the
provider is marked removed or generation mismatches). Use the existing
providerId keys and the functions
providerModelQueries/customModelQueries/enabledModelQueries to locate where
writes occur so you can gate them with the tombstone/generation check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ae5431d-fc49-4d7f-96dc-f0985c84dd72
📒 Files selected for processing (6)
src/main/presenter/configPresenter/index.tssrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/modelStore.tstest/renderer/components/ChatStatusBar.test.tstest/renderer/stores/modelStore.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/presenter/configPresenter/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/renderer/stores/modelStore.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary\n- derive chat-selectable model groups from the current provider catalog inside modelStore\n- keep model state aligned when providers are enabled, disabled, or removed\n- switch NewThreadPage, MCP sampling, and ChatStatusBar to the shared selectable model source\n\n## Testing\n- commit hook ran oxfmt and typecheck automatically during git commit\n
Summary by CodeRabbit
Refactor
New Features
Improvements
Bug Fixes
Tests