feat: add OpenRouter model selector and configurable fallback provider#2014
Open
luiggibcn wants to merge 34 commits into
Open
feat: add OpenRouter model selector and configurable fallback provider#2014luiggibcn wants to merge 34 commits into
luiggibcn wants to merge 34 commits into
Conversation
|
|
Contributor
|
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:
📝 WalkthroughWalkthroughAdds OpenRouter model discovery and caching, a provider-aware ProviderModelCombobox with virtualized combobox rendering, configurable settings-driven fallback provider for slash-form model IDs, IPC/preload/type updates for OpenRouter listing, provider-detection/auth adjustment, and sanitization of assistant messages for non-Anthropic providers. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer UI
participant Combobox as ProviderModelCombobox
participant Hook as useOpenRouterModels
participant Preload as Preload (electronAPI)
participant Main as Main Process
participant OR as OpenRouter API
UI->>Combobox: mount / user opens picker
Combobox->>Hook: preloadOpenRouterModels() (if provider == openrouter)
Hook->>Preload: window.electronAPI.listOpenRouterModels()
Preload->>Main: ipcRenderer.invoke(OPENROUTER_LIST_MODELS)
Main->>OR: HTTP GET /api/v1/models
OR-->>Main: return models [{id,name},...]
Main-->>Preload: IPCResult { success: true, data:{models} }
Preload-->>Hook: models
Hook-->>Combobox: cached options + isLoading/isError
Combobox-->>UI: render options (virtualized)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a more flexible model selection system, specifically adding support for slash-format model IDs (e.g., 'provider/model') and a configurable fallback provider. Key changes include the implementation of a virtualized ProviderModelCombobox component that supports dynamic fetching for OpenRouter and freeform input, as well as updates to the provider detection logic to utilize application settings. Review feedback identifies a regression where Ollama models are no longer dynamically listed in the new selector, potential performance issues due to synchronous disk I/O when retrieving settings, and a recommendation for more robust string handling in the OpenRouter model hook.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/main/agent/agent-manager.ts (1)
189-195: 🧹 Nitpick | 🔵 TrivialSemantically incorrect context passed to
resolveAuthfor non-Anthropic providers.In the legacy fallback path (lines 189-195), when a slash-form
requestedModellike"openai/gpt-4o"is used,detectProviderFromModelreturns a non-Anthropic provider (typically OpenRouter), yetconfigDiris still sourced from the active Claude profile. Passing an Anthropic-specificconfigDirfor a non-Anthropic provider is semantically inconsistent, even thoughconfigDiris not currently consumed for non-Anthropic providers (the OAuth resolver guards against this).To improve clarity, either:
- Null out
configDirwhen the resolved provider is not'anthropic'- Or skip passing
configDirin this fallback path and let each resolver default to undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/agent/agent-manager.ts` around lines 189 - 195, The legacy fallback currently grabs configDir from getClaudeProfileManager()->activeProfile and passes it into resolveAuth even when detectProviderFromModel(requestedModel) yields a non-'anthropic' provider; update the block so configDir is only passed when provider === 'anthropic' (e.g., set const configDir = provider === 'anthropic' ? activeProfile?.configDir : undefined or omit configDir from the resolveAuth call for non-anthropic providers), leaving detectProviderFromModel and resolveAuth usage unchanged.apps/desktop/src/main/ai/providers/factory.ts (1)
234-252:⚠️ Potential issue | 🟡 MinorJSDoc for
detectProviderFromModelis now attached togetConfiguredFallbackProvider.The block at lines 234–240 (
Detects the provider for a model ID based on its prefix...@parammodelId...@returns...) was originally documentingdetectProviderFromModel, but the newgetConfiguredFallbackProviderfunction was inserted directly under it. The JSDoc now describes a function that takes nomodelIdand returns a hardcodedSupportedProvider. Move the JSDoc to immediately precededetectProviderFromModel(line 254) and add a brief doc forgetConfiguredFallbackProvider.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/ai/providers/factory.ts` around lines 234 - 252, The JSDoc block currently above getConfiguredFallbackProvider belongs to detectProviderFromModel; move that existing JSDoc so it immediately precedes detectProviderFromModel (the function named detectProviderFromModel) and then add a short, accurate JSDoc above getConfiguredFallbackProvider describing that it reads settings to return the configured fallback provider or defaults to SupportedProvider.OpenRouter, including mention that it may swallow errors; keep both comments concise and aligned with each function signature.apps/desktop/src/renderer/components/AgentProfileSelector.tsx (1)
281-318:⚠️ Potential issue | 🟠 MajorExtend
ProviderModelComboboxto fetch Ollama installed models.Currently, when
provider === 'ollama', the combobox renders without a model list sinceALL_AVAILABLE_MODELScontains no Ollama entries. Users must type model names manually instead of selecting from installed models. The IPC infrastructure (window.electronAPI.listOllamaModels) exists and is already in use byOllamaModelManagerandMultiProviderModelSelect; apply the same pattern here so Ollama users in agent profile configuration get the same model discovery as in settings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/AgentProfileSelector.tsx` around lines 281 - 318, ProviderModelCombobox currently doesn't load Ollama-installed models, so update ProviderModelCombobox to detect when props.provider === 'ollama' and asynchronously fetch installed models using the existing IPC call (window.electronAPI.listOllamaModels) or reuse the pattern in OllamaModelManager/MultiProviderModelSelect; merge the returned model names into the component's options (alongside ALL_AVAILABLE_MODELS entries), manage local state for the dynamic options, handle loading/error states and cleanup on unmount, and ensure the fetched list is used for both the agent profile usages in AgentProfileSelector and the custom model combobox so Ollama users can select installed models instead of typing them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/ai/config/__tests__/types.test.ts`:
- Around line 68-71: The test silently passes when no matching prefix is found;
remove the conditional short-circuit and assert the prefix exists first, e.g.
replace the `if` with an assertion like `expect(prefix).toBeDefined()` and then
assert `MODEL_PROVIDER_MAP[prefix]` equals 'anthropic' so the test fails when
the `claude-` mapping is missing (referencing the MODEL_PROVIDER_MAP lookup and
the test case that computes `prefix` for 'claude-sonnet-4-6').
In `@apps/desktop/src/main/ai/providers/__tests__/factory.test.ts`:
- Around line 169-185: Add tests that exercise getConfiguredFallbackProvider by
mocking readSettingsFile from ../../settings-utils: call detectProviderFromModel
without passing the explicit fallbackProvider so it uses
getConfiguredFallbackProvider; assert that when readSettingsFile returns a
settings object with a valid fallbackProviderId the returned provider matches
it, when readSettingsFile returns an invalid id the provider defaults to
'openrouter', and when readSettingsFile throws the helper also defaults to
'openrouter'. Use the same model inputs as existing tests and reference
detectProviderFromModel and getConfiguredFallbackProvider when locating code to
update.
- Around line 170-184: The tests pass raw strings as the fallback provider;
replace those string literals with the SupportedProvider enum to ensure
type-safety and resilience to renames—update calls to detectProviderFromModel in
this file (e.g., the three it() blocks) to use SupportedProvider.OPENROUTER,
SupportedProvider.ANTHROPIC, SupportedProvider.OPENAI (or the matching enum
members) instead of 'openrouter', 'anthropic', 'openai' so the test signature
matches the function and uses the enum everywhere.
In `@apps/desktop/src/main/ai/providers/factory.ts`:
- Around line 244-247: The code silently ignores an invalid
settings.fallbackProviderId by returning the default provider; update the check
around settings?.fallbackProviderId and SupportedProvider so that when an id
exists but is not in Object.values(SupportedProvider) you record the
misconfiguration (use Sentry: add a breadcrumb or Sentry.captureMessage/error
with the invalid id and context; avoid console.log) and then fall back to the
default (e.g., OpenRouter) as before; reference the variables/settings:
settings, fallbackProviderId, SupportedProvider and the return path that casts
id as SupportedProvider so you can locate and modify that branch.
- Around line 241-252: getConfiguredFallbackProvider performs a synchronous disk
read and is invoked from detectProviderFromModel when model IDs use
slash-format; avoid this by either (A) memoizing the fallback value inside
getConfiguredFallbackProvider so it reads settings once and returns cached value
thereafter, or (B) thread the already-loaded fallbackProvider from higher-level
resolver code (e.g., resolveAuth) into detectProviderFromModel so
detectProviderFromModel no longer calls getConfiguredFallbackProvider
synchronously; update detectProviderFromModel signature to accept an optional
fallbackProvider and change callers to pass the preloaded value where available.
In `@apps/desktop/src/renderer/components/settings/ProviderAccountsList.tsx`:
- Around line 204-224: The visible label for the fallback Select is a plain <p>
and not programmatically associated with the Select, so screen readers won’t
announce it; update the Select markup by giving the SelectTrigger a stable id
(e.g. "fallback-provider-trigger") and add aria-labelledby on the Select (or the
SelectTrigger depending on your Select API) pointing to that id, or replace the
<p> label with the existing Label component and reference its id; update the
Select/SelectTrigger usage around fallbackProviderId and
handleFallbackProviderChange and ensure PROVIDER_REGISTRY mapping remains
unchanged.
- Around line 196-200: handleFallbackProviderChange currently awaits
saveSettings but swallows errors so users get no feedback; update the function
to wrap the saveSettings call in try/catch, and on error call the same
destructive toast used elsewhere (e.g., accounts.toast.settingsUpdateFailed with
tryAgain) to inform the user and preserve UI state. Locate
handleFallbackProviderChange and modify it to catch exceptions from
saveSettings, call the toast on failure, and keep the fallbackProviderId logic
unchanged.
In
`@apps/desktop/src/renderer/components/ui/__tests__/ProviderModelCombobox.test.tsx`:
- Around line 46-52: The test title and comment are inconsistent: when
ProviderModelCombobox is rendered with provider={undefined} it falls back to
AVAILABLE_MODELS and renders the static <Select> branch, not a freeform
combobox; update the test description and any inline comment to reflect that it
asserts the static Select branch (e.g., "renders a Select when provider is
undefined (falls back to AVAILABLE_MODELS)"), while keeping the render call to
ProviderModelCombobox and the existing assertion using
screen.getByRole('combobox') intact; refer to ProviderModelCombobox and
AVAILABLE_MODELS when renaming the test to avoid confusion.
In `@apps/desktop/src/renderer/components/ui/combobox.tsx`:
- Around line 67-78: The focusedIndex state (referenced as focusedIndex and
setFocusedIndex) can become stale when the user edits the search string (search)
and filteredOptions changes, so add a useEffect that watches trimmedSearch or
filteredOptions and resets focusedIndex to a safe value (e.g., 0 or -1) whenever
the filteredOptions result changes; update the logic used when opening (existing
open handling) to remain compatible with this reset so keyboard navigation
(Arrow keys/Enter) always targets the currently visible options.
- Around line 252-353: The component renders hardcoded user strings "Loading…"
and "Use" inside the Combobox rendering block (see isLoading branch and the
custom entry block) which must be internationalized; add two new props to the
Combobox component (e.g., loadingMessage and customValuePrefix) and use those
instead of the literals (falling back to sensible defaults), update all call
sites such as ProviderModelCombobox to pass translated values via
t('common:loading','Loading…') and t('common:combobox.use','Use'), and add the
corresponding keys to both en/*.json and fr/*.json translation files so
translations are available.
In `@apps/desktop/src/renderer/components/ui/ProviderModelCombobox.tsx`:
- Around line 39-43: ProviderModelCombobox currently calls useOpenRouterModels()
unconditionally which registers a subscriber on every mount; extract the
OpenRouter-specific logic into a small wrapper component (e.g.,
OpenRouterModelOptions) that calls useOpenRouterModels() and returns { options,
isLoading } and only render that wrapper when isOpenRouter is true; keep
preloadOpenRouterModels() call in the existing useEffect (or move it into the
wrapper if appropriate), and replace the original const { options:
openRouterOptions, isLoading: openRouterLoading } = useOpenRouterModels(); with
a conditional render/prop that uses the wrapper so hooks are only invoked in the
OpenRouter branch and other ProviderModelCombobox mounts won’t subscribe.
- Line 60: In ProviderModelCombobox replace the template-string class
composition on SelectTrigger (currently `${sizeClass} ${className ?? ''}`) with
the project's cn() helper to merge classes correctly (e.g. cn(sizeClass,
className)) so Tailwind merge and clsx behavior are preserved; update the same
pattern where SelectContent/other SelectTrigger usages appear (the similar
occurrence around the later Select instance at the other spot) to use cn() as
well, referencing the SelectTrigger component and the sizeClass and className
props to locate the changes.
- Line 35: ProviderModelCombobox.tsx uses the i18n keys "select.model",
"search.models", and "empty.models" via useTranslation in the 'common' namespace
but those keys are missing; add these keys to both en/common.json and
fr/common.json (under the "select", "search", and "empty" objects) with
appropriate translations so the calls in ProviderModelCombobox.tsx resolve
correctly for English and French users.
In `@apps/desktop/src/renderer/hooks/useOpenRouterModels.ts`:
- Around line 48-72: Add a refresh path to useOpenRouterModels so consumers can
clear error/stale cache and retry: implement a refresh() that sets cacheState =
'idle', clears cachedOptions = null, and then calls preloadOpenRouterModels()
(or the underlying fetchModels()) to re-run the load; expose refresh in the hook
return value alongside options/isLoading/isError and ensure
subscribers/rerendering behavior remains the same so UI updates after clearing
and reloading.
- Around line 14-41: The fetchModels function currently swallows errors in the
bare catch and duplicates the inline model type; update fetchModels to catch
(err) and log the error (e.g., using console.error or a logger) before setting
cacheState = 'error' and calling notify(), and extract the repeated inline type
({ id: string; name: string }) into a named type (e.g., OpenRouterModel) used in
the sort/map operations; reference the async function fetchModels, cacheState,
cachedOptions and the call window.electronAPI.listOpenRouterModels when making
these changes.
In `@apps/desktop/src/shared/types/settings.ts`:
- Around line 293-294: The fallbackProviderId field is currently a plain string;
tighten its type to the runtime-validated provider union by changing
fallbackProviderId?: string to fallbackProviderId?: BuiltinProvider (or the
SupportedProvider union) and update imports if needed; ensure places that
write/read this setting (e.g., ProviderAccountsList.tsx) use the new type and
that getConfiguredFallbackProvider()’s validation remains compatible with the
updated type.
---
Outside diff comments:
In `@apps/desktop/src/main/agent/agent-manager.ts`:
- Around line 189-195: The legacy fallback currently grabs configDir from
getClaudeProfileManager()->activeProfile and passes it into resolveAuth even
when detectProviderFromModel(requestedModel) yields a non-'anthropic' provider;
update the block so configDir is only passed when provider === 'anthropic'
(e.g., set const configDir = provider === 'anthropic' ? activeProfile?.configDir
: undefined or omit configDir from the resolveAuth call for non-anthropic
providers), leaving detectProviderFromModel and resolveAuth usage unchanged.
In `@apps/desktop/src/main/ai/providers/factory.ts`:
- Around line 234-252: The JSDoc block currently above
getConfiguredFallbackProvider belongs to detectProviderFromModel; move that
existing JSDoc so it immediately precedes detectProviderFromModel (the function
named detectProviderFromModel) and then add a short, accurate JSDoc above
getConfiguredFallbackProvider describing that it reads settings to return the
configured fallback provider or defaults to SupportedProvider.OpenRouter,
including mention that it may swallow errors; keep both comments concise and
aligned with each function signature.
In `@apps/desktop/src/renderer/components/AgentProfileSelector.tsx`:
- Around line 281-318: ProviderModelCombobox currently doesn't load
Ollama-installed models, so update ProviderModelCombobox to detect when
props.provider === 'ollama' and asynchronously fetch installed models using the
existing IPC call (window.electronAPI.listOllamaModels) or reuse the pattern in
OllamaModelManager/MultiProviderModelSelect; merge the returned model names into
the component's options (alongside ALL_AVAILABLE_MODELS entries), manage local
state for the dynamic options, handle loading/error states and cleanup on
unmount, and ensure the fetched list is used for both the agent profile usages
in AgentProfileSelector and the custom model combobox so Ollama users can select
installed models instead of typing them.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b16ed81-144b-4903-ac22-044f2a3a8aaa
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (19)
apps/desktop/src/main/agent/agent-manager.tsapps/desktop/src/main/ai/config/__tests__/types.test.tsapps/desktop/src/main/ai/providers/__tests__/factory.test.tsapps/desktop/src/main/ai/providers/factory.tsapps/desktop/src/preload/api/project-api.tsapps/desktop/src/renderer/components/AgentProfileSelector.tsxapps/desktop/src/renderer/components/settings/ProviderAccountsList.tsxapps/desktop/src/renderer/components/ui/ProviderModelCombobox.tsxapps/desktop/src/renderer/components/ui/__tests__/ProviderModelCombobox.test.tsxapps/desktop/src/renderer/components/ui/combobox.tsxapps/desktop/src/renderer/hooks/index.tsapps/desktop/src/renderer/hooks/useOpenRouterModels.tsapps/desktop/src/renderer/lib/browser-mock.tsapps/desktop/src/shared/constants/ipc.tsapps/desktop/src/shared/i18n/locales/en/settings.jsonapps/desktop/src/shared/i18n/locales/fr/settings.jsonapps/desktop/src/shared/types/ipc.tsapps/desktop/src/shared/types/settings.tsapps/desktop/tsconfig.json
Author
|
I have read the CLA Document and I hereby sign the CLA |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/desktop/src/main/ai/providers/factory.ts (1)
247-256:⚠️ Potential issue | 🟡 MinorInvalid
fallbackProviderIdand read failures are still silently swallowed.Both the
catchblock (line 254-256) and theidvalidation branch (line 250) silently fall through to OpenRouter without surfacing the misconfiguration. This was previously raised; flagging again because the current implementation does not record the invalid id or the read failure. Per coding guidelines, use Sentry (e.g.,Sentry.captureMessage/ breadcrumb) so misconfigurations and transient read errors are observable in production.As per coding guidelines: "Use Sentry for error tracking in production; reserve
console.logfor development only."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/ai/providers/factory.ts` around lines 247 - 256, The code swallows both settings read failures and invalid fallbackProviderId values; update the try/catch around readSettingsFile() and the validation branch for id to report issues to Sentry (e.g., call Sentry.captureMessage or add a breadcrumb) with a clear message including the raw id and the error, then fall back to the default as before; specifically, in the catch for readSettingsFile() call Sentry.captureMessage("Failed to read settings file", { extra: { error } }) (or similar) and in the branch where id is present but not in Object.values(SupportedProvider) call Sentry.captureMessage("Invalid fallbackProviderId", { extra: { fallbackProviderId: id } }) before falling through, while still using _fallbackProviderCache when id is valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/ai/providers/factory.ts`:
- Around line 244-259: The settings:save handler fails to invalidate the
process-lifetime fallback provider cache, so getConfiguredFallbackProvider() can
return a stale value (or the default cached after a transient readSettingsFile()
error); fix it by invoking resetFallbackProviderCache() inside the settings:save
handler whenever the update includes fallbackProviderId (mirror how
resetMemoryService() is called for memory-related updates), ensuring settings
saves triggered from saveSettings() / ProviderAccountsList.tsx clear the cached
value so subsequent calls to getConfiguredFallbackProvider() will read the new
setting.
- Around line 275-277: detectProviderFromModel currently treats any modelId
containing '/' as a fallback provider (returning fallbackProvider ??
getConfiguredFallbackProvider()), which breaks Ollama matching for slash-named
local models; update the detection so that the slash-format fallback is only
applied when intended by callers—either by adding an optional flag (e.g.,
allowSlashFallback = true) to detectProviderFromModel and have
resolveAuthFromQueue call it with allowSlashFallback = false, or by changing
resolveAuthFromQueue to detect only explicit native prefixes (claude-, gpt-,
etc.) instead of relying on detectProviderFromModel's slash behavior; ensure
symbols involved are detectProviderFromModel, resolveAuthFromQueue,
fallbackProvider, and getConfiguredFallbackProvider so Ollama (slash-named)
models resolve to undefined/inconclusive in the resolver branch.
In `@apps/desktop/src/renderer/components/ui/ProviderModelCombobox.tsx`:
- Line 5: Replace the relative import of useOpenRouterModels and
preloadOpenRouterModels with the project path-alias import; in
ProviderModelCombobox.tsx update the import that currently references
'../../hooks/useOpenRouterModels' to use the alias '@hooks/useOpenRouterModels'
so the symbols useOpenRouterModels and preloadOpenRouterModels are imported via
the tsconfig path alias.
---
Duplicate comments:
In `@apps/desktop/src/main/ai/providers/factory.ts`:
- Around line 247-256: The code swallows both settings read failures and invalid
fallbackProviderId values; update the try/catch around readSettingsFile() and
the validation branch for id to report issues to Sentry (e.g., call
Sentry.captureMessage or add a breadcrumb) with a clear message including the
raw id and the error, then fall back to the default as before; specifically, in
the catch for readSettingsFile() call Sentry.captureMessage("Failed to read
settings file", { extra: { error } }) (or similar) and in the branch where id is
present but not in Object.values(SupportedProvider) call
Sentry.captureMessage("Invalid fallbackProviderId", { extra: {
fallbackProviderId: id } }) before falling through, while still using
_fallbackProviderCache when id is valid.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 678e0a88-5fe9-4d06-92cd-2eebc29218b4
📒 Files selected for processing (3)
apps/desktop/src/main/agent/agent-manager.tsapps/desktop/src/main/ai/providers/factory.tsapps/desktop/src/renderer/components/ui/ProviderModelCombobox.tsx
Contributor
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/main/ai/providers/__tests__/factory.test.ts (1)
175-219: 🧹 Nitpick | 🔵 TrivialOptional: reset the fallback cache at suite scope to harden against future test pollution.
_fallbackProviderCacheis module-level state populated by the precedinggetConfiguredFallbackProviderdescribe. Current tests in this block all pass an explicitfallbackProvider, so cache contents are bypassed via??. However, any future test added here that omits the explicit fallback would silently inherit cached state from a prior describe and become order-dependent. Consider hoistingresetFallbackProviderCache()(andmockReadSettingsFile.mockReset()) into a top-levelbeforeEachfor the file.♻️ Proposed harden-up
+beforeEach(() => { + resetFallbackProviderCache(); + mockReadSettingsFile.mockReset(); +}); + describe('createProvider', () => {…and the per-describe
beforeEachat lines 149-152 can then be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/ai/providers/__tests__/factory.test.ts` around lines 175 - 219, Tests rely on module-level _fallbackProviderCache populated by getConfiguredFallbackProvider; to avoid order-dependent failures, add a top-level beforeEach in the test file that calls resetFallbackProviderCache() and mockReadSettingsFile.mockReset() so every test starts with a clean cache and mock state, and remove the now-redundant per-describe beforeEach blocks that reset the same state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/components/ui/__tests__/ProviderModelCombobox.test.tsx`:
- Around line 87-88: Remove the redundant global stub and ensure proper test
isolation: delete the vi.stubGlobal('electronAPI', ...) call and rely only on
Object.defineProperty(window, 'electronAPI', { value: { listOllamaModels },
writable: true }); or, if you prefer stubbing globals, call
vi.unstubAllGlobals() in an afterEach to restore state (or enable
unstubAllGlobals in config). Also make the assertion robust by awaiting the
async effect update with waitFor(() =>
expect(listOllamaModels).toHaveBeenCalled()) after render to avoid React act
warnings related to the component's IIFE effect and subsequent setOllamaLoading
state update.
---
Outside diff comments:
In `@apps/desktop/src/main/ai/providers/__tests__/factory.test.ts`:
- Around line 175-219: Tests rely on module-level _fallbackProviderCache
populated by getConfiguredFallbackProvider; to avoid order-dependent failures,
add a top-level beforeEach in the test file that calls
resetFallbackProviderCache() and mockReadSettingsFile.mockReset() so every test
starts with a clean cache and mock state, and remove the now-redundant
per-describe beforeEach blocks that reset the same state.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a4be53a-fa59-48c9-b67a-c31d75383afc
📒 Files selected for processing (2)
apps/desktop/src/main/ai/providers/__tests__/factory.test.tsapps/desktop/src/renderer/components/ui/__tests__/ProviderModelCombobox.test.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/ipc-handlers/memory-handlers.ts`:
- Around line 577-597: The OpenRouter IPC handler
(IPC_CHANNELS.OPENROUTER_LIST_MODELS) currently registered inline in
memory-handlers.ts should be moved into a new domain module file (e.g.,
openrouter-handlers.ts) and exposed via an exported function
registerOpenRouterHandlers(); copy the ipcMain.handle(...) block that performs
the fetch/JSON parsing/error handling into that module, export
registerOpenRouterHandlers which registers the handler, then remove the original
ipcMain.handle(...) from memory-handlers.ts and call
registerOpenRouterHandlers() from the same initialization site where
registerMemoryHandlers() is invoked so the handler is registered alongside other
domain handlers.
- Around line 587-588: The parsing of the OpenRouter response blindly assumes
json.data is an array and then calls .map, which can throw if the shape is
different; update the handler that defines json and models so it validates the
shape (e.g., use Array.isArray(json.data)) before mapping — if json.data is not
an array, treat it as an empty array or throw a meaningful Error so the existing
catch produces a clear message; ensure you update the creation of models (the
variable named models) to use the guarded array instead of json.data directly.
- Around line 581-597: The handler registered for
IPC_CHANNELS.OPENROUTER_LIST_MODELS uses fetch without a timeout and can hang
indefinitely; update the async handler to use an AbortController with a bounded
timeout (create or reuse a timeout constant like OPENROUTER_TIMEOUT_MS or
OLLAMA_TIMEOUT_MS), pass controller.signal into
fetch('https://openrouter.ai/api/v1/models', { signal }), and clear the timeout
when complete; ensure the catch handles AbortError distinctly (or returns the
same failure shape) so the IPC call always resolves even on timeout.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f47ab0b3-ba48-4acd-b150-d94ad3659467
📒 Files selected for processing (1)
apps/desktop/src/main/ipc-handlers/memory-handlers.ts
luiggibcn
pushed a commit
to luiggibcn/Aperant
that referenced
this pull request
Apr 25, 2026
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add ProviderModelCombobox component for selecting OpenRouter models with search - Add useOpenRouterModels hook to fetch available models from OpenRouter API - Add fallbackProviderId setting: user-configurable provider for ambiguous slash-format model IDs (e.g. "provider/model"), defaults to openrouter - Remove hardcoded slash-prefix entries from MODEL_PROVIDER_MAP; detection is now generic via detectProviderFromModel() with user-controlled fallback - Add fallback provider selector UI in the Accounts settings section - Add i18n keys (en/fr) for the new fallback provider setting - Update tests to reflect new MODEL_PROVIDER_MAP structure and slash routing Closes AndyMik90#1950 Closes AndyMik90#1988 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Only pass configDir to resolveAuth when provider is anthropic - Cache fallback provider read from disk (avoid sync I/O on every call) - Export resetFallbackProviderCache() for settings change invalidation - Restore Ollama dynamic model listing in ProviderModelCombobox Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Test getConfiguredFallbackProvider via detectProviderFromModel: valid provider id, invalid id, readSettingsFile throws, no id set - Test ProviderModelCombobox Ollama: renders combobox, calls listOllamaModels on mount, skips call for non-Ollama providers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Registers the ipcMain handler for OPENROUTER_LIST_MODELS that fetches the public model list from openrouter.ai/api/v1/models and returns it to the renderer for the ProviderModelCombobox dynamic list. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add sanitizeReasoningFromMessages() in session/runner.ts: strips reasoning/redacted-reasoning content parts from message history before sending to non-Claude providers, preventing BadRequestError when task switches from Claude (extended thinking) to Fireworks, Together, or any provider that rejects thinking blocks (AndyMik90#1988) - Add JSDoc to useOpenRouterModels, fetchModels, notify functions - Add JSDoc to AgentProfileSelector and ProviderAccountsList components Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Non-Claude provider: strips reasoning parts, preserves text parts - Claude provider: passes reasoning parts through unchanged - redacted-reasoning parts stripped for non-Claude providers - String content messages unaffected regardless of provider Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
luiggibcn
force-pushed
the
feat/openrouter-model-selector
branch
from
148b5a5 to
cbd331a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/ai/session/runner.ts`:
- Around line 662-671: The current messages.map transform replaces assistant
turns that only contain 'reasoning'/'redacted-reasoning' parts with { type:
'text', text: '' }, which some providers reject; update the logic in the
messages.map handler in runner.ts so that when (msg.role === 'assistant' &&
Array.isArray(msg.content)) and filtered.length === 0 you either (A) omit the
message entirely from the returned array (e.g., map -> filter pattern: return
null for that msg and then filter(Boolean)), or (B) replace it with a non-empty
placeholder text (e.g., { type: 'text', text: '[redacted]' }) to ensure
downstream providers accept it; adjust the return to use filtered when
filtered.length > 0 and apply the chosen omission/placeholder behavior
consistently.
- Around line 337-343: The Anthropic detection using isAnthropicModel =
modelId?.startsWith('claude-') incorrectly treats Claude models routed via
Bedrock/OpenRouter/Vertex as non-Anthropic and causes
sanitizeReasoningFromMessages to strip reasoning blocks; update session
detection to rely on a provider/capability flag instead of modelId prefix by
adding a resolvedProvider or supportsReasoningBlocks field to SessionConfig
(propagate it from the auth layer) and change the logic in runner.ts to check
that flag (replace references to isAnthropicModel/modelId with the new
SessionConfig.resolvedProvider or SessionConfig.supportsReasoningBlocks) so
reasoning/redacted-reasoning parts are preserved for supported providers and
avoid creating empty-text fallbacks that some providers may reject.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5efee793-d336-4058-95f2-646c8ef292ff
⛔ Files ignored due to path filters (4)
apps/desktop/docs/screenshots/pr-2014/after_new_task.pngis excluded by!**/*.pngapps/desktop/docs/screenshots/pr-2014/after_settings_accounts.pngis excluded by!**/*.pngapps/desktop/docs/screenshots/pr-2014/before_new_task.pngis excluded by!**/*.pngapps/desktop/docs/screenshots/pr-2014/before_settings_accounts.pngis excluded by!**/*.png
📒 Files selected for processing (4)
apps/desktop/src/main/ai/session/runner.tsapps/desktop/src/renderer/components/AgentProfileSelector.tsxapps/desktop/src/renderer/components/settings/ProviderAccountsList.tsxapps/desktop/src/renderer/hooks/useOpenRouterModels.ts
Some providers reject messages with empty content arrays. Replace the empty text fallback with '[redacted]' when all content parts in an assistant message are reasoning/redacted-reasoning blocks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move the stray JSDoc from above getConfiguredFallbackProvider to the correct position above detectProviderFromModel. Add proper JSDoc to getConfiguredFallbackProvider describing its caching behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace hardcoded 'Select model…', 'Search models…', 'No models found' strings with proper i18n keys (common:modelCombobox.*) in en and fr. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Change fallbackProviderId from string to BuiltinProvider in Settings type - Wrap saveSettings call in try/catch with destructive toast on failure - Add settings:errors.saveFailed i18n key (en/fr) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…romModel tests - Replace raw string literals with SupportedProvider enum values - Add beforeEach that resets fallback provider cache and settings mock to prevent cross-suite contamination from module-level cache state Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract the OPENROUTER_LIST_MODELS handler from memory-handlers.ts into its own registerOpenRouterHandlers() function in openrouter-handlers.ts. Register it alongside other handlers in ipc-handlers/index.ts. Add OpenRouterModel interface for the parsed API response shape. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Exposes a refresh() function that clears the module-level cache and retries the fetch — useful when the user wants to retry after a network error without reloading the app. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace template string class concatenation with cn(sizeClass, className) for consistent Tailwind class merging across the component. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The conditional if(prefix) silently passed when no matching prefix was found, masking regressions. Replace with explicit toBeDefined() assertion so the test fails loudly if the claude- mapping is ever removed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Author
|
@coderabbitai review |
Contributor
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
Guard json.data with Array.isArray before calling .map to avoid a confusing TypeError if OpenRouter returns an unexpected envelope. Filter out entries with a missing id field for extra safety. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dels - Surface the underlying error in the catch block via console.error instead of silently swallowing it - Guard against empty providerSlug (e.g. model ID starts with '/') by falling back to 'Other' group label Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Log a console.warn when fallbackProviderId is set to an unknown SupportedProvider value, and when readSettingsFile throws, instead of silently ignoring both cases. Falls back to openrouter in both paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add id="fallback-provider-label" to the <p> label and aria-labelledby on the SelectTrigger so screen readers announce the correct label when the control receives focus. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Rename 'renders a combobox when provider is undefined (freeform)' to
accurately reflect that undefined falls back to the anthropic static
Select, not a freeform Combobox
- Remove vi.stubGlobal('electronAPI', ...) which set globalThis.electronAPI
instead of window.electronAPI; the Object.defineProperty call below it
was already setting the correct target
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Prevents the IPC handler from hanging indefinitely if the openrouter.ai API is slow or unresponsive. Uses AbortController + clearTimeout in finally to avoid timer leaks on both success and error paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ox wrapper Extract OpenRouterCombobox so useOpenRouterModels() is only called when provider === 'openrouter', preventing unnecessary subscriber registrations on Anthropic/Ollama/unknown phase selectors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion key Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… translation key" This reverts commit 44541ab.
…anslation internally Keeps the generic UI primitive free of i18n concerns — consistent with how emptyMessage, placeholder and searchPlaceholder are already handled as caller-provided strings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…st resolution vitest had @/ → src/ while tsconfig has @/* → src/renderer/*. Aligning them allows @/hooks/useOpenRouterModels to resolve correctly in tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
factory.ts statically imported settings-utils, which imports { app } from
'electron'. Worker threads don't have access to app, so module evaluation
failed with "does not provide an export named 'app'" whenever a task moved
to in_progress and spawned a worker.
Replace the static import with an injectable _settingsReader (default no-op,
safe in any runtime). The main process wires it up via configureSettingsReader
at startup; worker threads keep the no-op because they already receive the
provider resolved in workerData.
Also:
- FactorySettings type instead of Record<string,unknown> — catches typos at
compile time and removes the 'as string' cast
- isMainThread canary: warns if the main process forgets to call
configureSettingsReader before the slash-format path is exercised
- Tests: global beforeEach resets cache + reader before every test in the
file; describe-level beforeEach only injects the mock it needs
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_fallbackProviderCache was never reset after the user changed fallbackProviderId in the UI — the new value only took effect after restarting the app. Call resetFallbackProviderCache() in the SETTINGS_SAVE handler whenever fallbackProviderId is part of the incoming payload. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Replace hardcoded "Use" string with a customValueLabel prop and pass the translated value from ProviderModelCombobox. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
3 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.
Base Branch
Description
Adds a searchable combobox for selecting OpenRouter models from the live API, and replaces the fragile hardcoded slash-prefix list in `MODEL_PROVIDER_MAP` with a user-configurable fallback provider setting.
Also addresses:
Related Issue
Closes #1988
Ref #1950 (bundling fix already in develop via 35bb86d)
Type of Change
Area
Commit Message Format
Follow conventional commits: `: `
Types: feat, fix, docs, style, refactor, test, chore
Example: `feat: add user authentication system`
AI Disclosure
Tool(s) used: Claude Code
Testing level:
Untested -- AI output not yet verified
Lightly tested -- ran the app / spot-checked key paths
Fully tested -- all tests pass, manually verified behavior
I understand what this PR does and how the underlying code works
Checklist
Platform Testing Checklist
CI/Testing Requirements
Screenshots
New Task — Model Selector
Settings → Accounts
Feature Toggle
Breaking Changes
Breaking: No
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores