Repository navigation
fix(model-picker): eliminate O(n²) catalog rebuild lag in /model - #2078
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
⚙️ CodeRabbit configuration file
Files:
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (3)
📝 WalkthroughWalkthroughThe change adds prefix validation for switch-profile values and reuses catalog duplicate detection and route lookup state. New tests cover profile switching, malformed values, custom IDs, aliases, scoped caches, and large catalogs. ChangesModel option flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/utils/model/modelOptions.ts`:
- Around line 941-966: The catalog option changes around
getDuplicateCatalogApiNames and getCatalogOptionValue require focused regression
tests in src/utils/model/modelOptions.ts:941-966 covering case/whitespace
duplicates, direct/ID/alias matching, insertion paths, ordinary model IDs, and
large catalogs. Add focused switch-profile detection tests for
src/components/ModelPicker.tsx:499-509 covering ordinary IDs, valid
switch-profile values, and invalid prefixed values; no direct implementation
change is required at either site beyond adding or updating the corresponding
tests.
- Around line 1020-1042: Update getModelOptions and hasOptionValue to compute
the route-catalog lookup context, including duplicate API-name Map/Set data,
once per options build and pass it through all additional-model and
catalog-entry checks. Avoid calling getRouteCatalogModelOption or rebuilding
duplicate context for each option, while preserving existing matching behavior.
Verify performance with a large provider catalog and many additional options as
required by AGENTS.md.
🪄 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 Plus
Run ID: 2648f991-323f-41ac-83da-1167d0f7261c
📒 Files selected for processing (2)
src/components/ModelPicker.tsxsrc/utils/model/modelOptions.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/model/modelOptions.tssrc/components/ModelPicker.tsx
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/model/modelOptions.tssrc/components/ModelPicker.tsx
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/model/modelOptions.tssrc/components/ModelPicker.tsx
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/utils/model/modelOptions.ts
src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI components under
src/components/.
Files:
src/components/ModelPicker.tsx
🔇 Additional comments (2)
src/utils/model/modelOptions.ts (1)
977-979: LGTM!src/components/ModelPicker.tsx (1)
14-14: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/components/ModelPicker.switchProfile.test.ts`:
- Around line 196-201: Update the test helper that imports the nonce-suffixed
model-options module to return that mocked module instance, then reuse its
getModelOptions result in the assertion near the switchOption lookup. Ensure the
import uses the same specifier instance already bound to ModelPicker, rather
than performing a second dynamic import with a different nonce and separator.
In `@src/utils/model/modelOptions.catalogDedup.test.ts`:
- Around line 56-70: Fix both fresh-import helpers so their process-wide mocks
are gated and fall through to real implementations when inactive. In
src/utils/model/modelOptions.catalogDedup.test.ts lines 56-70, gate the
./providers.js mock, spread realModelProviders, reset its flag in beforeEach and
afterEach, and remove the ./model.js mock if the nonce import is sufficient; in
src/components/ModelPicker.switchProfile.test.ts lines 80-90, gate the
../utils/model/modelOptions.js mock on activeProfilesOverride so it resolves to
the real module when null.
🪄 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 Plus
Run ID: 465fde48-13f9-428a-b3d9-53979d0a10ce
📒 Files selected for processing (3)
src/components/ModelPicker.switchProfile.test.tssrc/components/ModelPicker.tsxsrc/utils/model/modelOptions.catalogDedup.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (22)
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/utils/model/modelOptions.catalogDedup.test.tssrc/components/ModelPicker.switchProfile.test.tssrc/components/ModelPicker.tsx
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/utils/model/modelOptions.catalogDedup.test.tssrc/components/ModelPicker.switchProfile.test.tssrc/components/ModelPicker.tsx
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/model/modelOptions.catalogDedup.test.tssrc/components/ModelPicker.switchProfile.test.tssrc/components/ModelPicker.tsx
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/utils/model/modelOptions.catalogDedup.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/model/modelOptions.catalogDedup.test.tssrc/components/ModelPicker.switchProfile.test.ts
src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI components under
src/components/.
Files:
src/components/ModelPicker.tsx
🪛 ast-grep (0.45.0)
src/utils/model/modelOptions.catalogDedup.test.ts
[warning] 65-65: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(insecure-random-security-token-typescript)
src/components/ModelPicker.switchProfile.test.ts
[warning] 83-83: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(insecure-random-security-token-typescript)
🔇 Additional comments (4)
src/utils/model/modelOptions.catalogDedup.test.ts (2)
116-145: LGTM!
280-308: LGTM!src/components/ModelPicker.tsx (1)
506-511: LGTM!src/components/ModelPicker.switchProfile.test.ts (1)
235-246: LGTM!
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/components/ModelPicker.switchProfile.test.ts (1)
82-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the nonce separator consistently when importing modelOptions.js.
The test at line 204 still imports
../utils/model/modelOptions.js?switchProfile-...; this bypassesimportFreshModelPicker’smock.module(... 'modelOptions.js' ...)and can load a separate module instance. Use the same query-param nonce style.🤖 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 `@src/components/ModelPicker.switchProfile.test.ts` around lines 82 - 97, Update the direct modelOptions.js import in the test around the switch-profile case to use the same `?switchProfile=${nonce}` query-parameter format as `importFreshModelPicker`. Keep the nonce-based cache busting intact so the import resolves through the existing mock.module setup instead of creating a separate module instance.
🤖 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.
Outside diff comments:
In `@src/components/ModelPicker.switchProfile.test.ts`:
- Around line 82-97: Update the direct modelOptions.js import in the test around
the switch-profile case to use the same `?switchProfile=${nonce}`
query-parameter format as `importFreshModelPicker`. Keep the nonce-based cache
busting intact so the import resolves through the existing mock.module setup
instead of creating a separate module instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b0271e54-fb3f-4659-b057-f867824a8eca
📒 Files selected for processing (2)
src/components/ModelPicker.switchProfile.test.tssrc/utils/model/modelOptions.catalogDedup.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: smoke-and-tests (22)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/components/ModelPicker.switchProfile.test.tssrc/utils/model/modelOptions.catalogDedup.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/components/ModelPicker.switchProfile.test.tssrc/utils/model/modelOptions.catalogDedup.test.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/components/ModelPicker.switchProfile.test.tssrc/utils/model/modelOptions.catalogDedup.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/components/ModelPicker.switchProfile.test.tssrc/utils/model/modelOptions.catalogDedup.test.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/utils/model/modelOptions.catalogDedup.test.ts
🔇 Additional comments (2)
src/utils/model/modelOptions.catalogDedup.test.ts (1)
14-14: LGTM!Also applies to: 24-24, 33-63, 96-108, 147-147, 162-162
src/components/ModelPicker.switchProfile.test.ts (1)
11-11: LGTM!Also applies to: 20-20, 82-97
|
@coderabbitai full review |
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
|
Risk surface assessment: This PR touches provider-route catalog lookup and switch-profile value The catalog change only reuses lookup context within a single The switch-profile change only short-circuits ordinary model IDs that cannot Focused regression tests cover catalog aliases, duplicate API names, Based on those constraints and the passing test suite, no remaining routing or |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Remove the duplicate release repair from this PR
web/src/data/releases.ts:20
This is a hand-maintained release record, not output from the release bot. The same baseline repair is already tracked by open maintainer PR #2075, which adds the 0.27.0 entry and its regression coverage. Leaving this separate change here creates a merge conflict if #2075 lands first; landing this PR first instead supersedes the dedicated repair with a divergent, untested release record. Drop this hunk and rebase on the canonical repair (or explicitly coordinate which PR supersedes it). -
[P2] Stop leaking the fresh
model.jsmodule into later test suites
src/utils/model/modelOptions.catalogDedup.test.ts:89
mock.module('./model.js', ...)is process-wide andmock.restore()does not unregister module mocks, but this mock has no active-test gate or real-module fallback. After this suite runs, later imports of the canonical./model.jsreceive the last nonce-suffixed test instance, making module identity/state depend on test order. Remove the override if it is unnecessary, or gate it and fall through to a snapshot of the real module when no catalog test is active.
894629a to
1eb46a6
Compare
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/utils/model/modelOptions.ts (1)
1030-1036: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNon-blocking: the per-lookup catalog scan is still O(entries) and runs several times per build.
findRouteCatalogOptionscanscontext.entrieswith.findon every call.getModelOptionscalls it once per scoped additional option (Line 1109) and again throughhasOptionValue(Line 1115), so a build withmadditional options costs2·m·nnormalizations on ann-entry catalog. The shared context removed the duplicate-set rebuild, but not this scan.Build a normalized lookup
Maponce insidegetRouteCatalogContextand reuse it. Keep first-entry-wins insertion so the resolved entry matches the current.findresult, and keep the apiName → id → alias precedence.⚡ Sketch: precompute the normalized index in the context
type RouteCatalogContext = { entries: ReturnType<typeof getCatalogEntriesForRoute> duplicateApiNames: Set<string> + entriesByNormalizedKey: Map<string, ReturnType<typeof getCatalogEntriesForRoute>[number]> }const entries = getCatalogEntriesForRoute(routeId) + // apiName, then id, then aliases — first entry wins, matching the + // precedence and ordering of the previous linear `.find`. + const entriesByNormalizedKey = new Map<string, (typeof entries)[number]>() + for (const keyOf of [ + (e: (typeof entries)[number]) => [e.apiName], + (e: (typeof entries)[number]) => [e.id], + (e: (typeof entries)[number]) => e.aliases ?? [], + ]) { + for (const entry of entries) { + for (const raw of keyOf(entry)) { + const key = normalizeRouteModelOptionKey(raw) + if (key && !entriesByNormalizedKey.has(key)) { + entriesByNormalizedKey.set(key, entry) + } + } + } + } return { entries, duplicateApiNames: getDuplicateCatalogApiNames(entries), + entriesByNormalizedKey, }- const catalogEntry = context.entries.find(entry => - normalizeRouteModelOptionKey(entry.apiName) === normalizedValue || - normalizeRouteModelOptionKey(entry.id) === normalizedValue || - (entry.aliases ?? []).some( - alias => normalizeRouteModelOptionKey(alias) === normalizedValue, - ), - ) + const catalogEntry = context.entriesByNormalizedKey.get(normalizedValue)Note the precedence difference: the current
.findis entry-major (an early entry's alias beats a later entry's apiName), the sketch above is field-major. Pick the one that matches intent and add a test for the collision case.🤖 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 `@src/utils/model/modelOptions.ts` around lines 1030 - 1036, Update getRouteCatalogContext to build and retain a normalized lookup Map once, inserting each entry’s apiName, id, and aliases in entry-major order with first-entry-wins semantics. Refactor findRouteCatalogOption to resolve through this Map instead of scanning context.entries, preserving the existing normalization and apiName → id → alias checks for each entry and adding coverage for an early alias versus a later apiName collision.
🤖 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 `@src/components/ModelPicker.switchProfile.test.ts`:
- Around line 167-173: Update the ordinary-model test around
isGenuineSwitchProfileValue to track calls to getModelOptions through the gated
mock binding used by ModelPicker, then assert it is not called for an ordinary
id. Preserve the existing false-result assertions while explicitly verifying the
prefix short-circuit.
In `@src/utils/model/modelOptions.catalogDedup.test.ts`:
- Around line 272-303: Remove the overwritten OPENAI_BASE_URL and OPENAI_MODEL
environment assignments from the test setup around getRouteCatalogModelOptions,
while preserving OPENAI_API_KEY if needed by the exercised path. Keep the test
focused on the activeCacheScopeOverride and the helper’s own environment
behavior.
---
Outside diff comments:
In `@src/utils/model/modelOptions.ts`:
- Around line 1030-1036: Update getRouteCatalogContext to build and retain a
normalized lookup Map once, inserting each entry’s apiName, id, and aliases in
entry-major order with first-entry-wins semantics. Refactor
findRouteCatalogOption to resolve through this Map instead of scanning
context.entries, preserving the existing normalization and apiName → id → alias
checks for each entry and adding coverage for an early alias versus a later
apiName collision.
🪄 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 Plus
Run ID: 08413e7a-673c-499c-9c59-6c7f084a9db7
📒 Files selected for processing (5)
src/components/ModelPicker.switchProfile.test.tssrc/components/ModelPicker.tsxsrc/utils/model/modelOptions.catalogDedup.test.tssrc/utils/model/modelOptions.tsweb/src/data/releases.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: smoke-and-tests (24.11.x)
- GitHub Check: typecheck
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/components/ModelPicker.tsxsrc/utils/model/modelOptions.catalogDedup.test.tssrc/components/ModelPicker.switchProfile.test.tsweb/src/data/releases.tssrc/utils/model/modelOptions.ts
src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI components under
src/components/.
Files:
src/components/ModelPicker.tsx
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/components/ModelPicker.tsxsrc/utils/model/modelOptions.catalogDedup.test.tssrc/components/ModelPicker.switchProfile.test.tsweb/src/data/releases.tssrc/utils/model/modelOptions.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/components/ModelPicker.tsxsrc/utils/model/modelOptions.catalogDedup.test.tssrc/components/ModelPicker.switchProfile.test.tsweb/src/data/releases.tssrc/utils/model/modelOptions.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/utils/model/modelOptions.catalogDedup.test.tssrc/utils/model/modelOptions.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/model/modelOptions.catalogDedup.test.tssrc/components/ModelPicker.switchProfile.test.ts
web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
When changing the web application, run
bun run web:typecheckandbun run web:build.
Files:
web/src/data/releases.ts
web/**
⚙️ CodeRabbit configuration file
web/**: Review browser extension changes for content-script isolation, message validation, cross-origin assumptions, permission surfaces, and failures that could leak prompts or credentials.
Files:
web/src/data/releases.ts
🪛 ast-grep (0.45.0)
src/utils/model/modelOptions.catalogDedup.test.ts
[warning] 93-93: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(insecure-random-security-token-typescript)
src/components/ModelPicker.switchProfile.test.ts
[warning] 85-85: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(insecure-random-security-token-typescript)
🔇 Additional comments (8)
web/src/data/releases.ts (2)
20-31: LGTM!
20-31: 📐 Maintainability & Code QualityConfirm the required web checks.
Because
web/src/data/releases.tschanged, confirm thatbun run web:typecheckandbun run web:buildpassed.Source: Path instructions
src/utils/model/modelOptions.ts (2)
992-1015: LGTM!Also applies to: 1078-1090
1056-1069: 🗄️ Data Integrity & IntegrationCheck the removed
optionMatchesModelimplementation before merging.
hasOptionValuenow uses strict equality against both option values and the first matched catalog value. If the old comparison normalized case/whitespace or matched a[1m]suffix, existing persisted custom models can fall through and be appended again. Use the original call-site behavior to decide whether the strict comparison is safe.src/utils/model/modelOptions.catalogDedup.test.ts (2)
39-102: LGTM!Also applies to: 149-182
317-345: LGTM!src/components/ModelPicker.switchProfile.test.ts (1)
30-98: LGTM!Also applies to: 132-165
src/components/ModelPicker.tsx (1)
506-511: 🎯 Functional CorrectnessNo change needed: switch marker construction is consistent.
Production model options set
switchToProfileIdonly ingetInactiveProviderProfileOptions, and that path always writesvalue: encodeSwitchProfileValue(...). Existing tests cover bare values with the prefix but no marker.
getModelOptions() ran an O(n²) optionMatchesModel loop (catalog scan per option) plus an O(n²) duplicate-apiName filter, costing ~43ms per call on catalogs with hundreds of models (e.g. Fireworks' ~280 entries). The picker also rebuilt the full options list on every keystroke via isGenuineSwitchProfileValue, so arrow-key navigation lagged badly. - hoist catalog lookup out of the per-option loop (hasOptionValue) - precompute duplicate apiNames into a Set - short-circuit isGenuineSwitchProfileValue for non-switch values getModelOptions(): 43.5ms -> 2.0ms on a 277-entry catalog
…ns checks getRouteCatalogModelOption re-resolved the active route, fetched the catalog entries, and rebuilt the duplicate-apiName set on every call — getModelOptions() invoked it up to 3x per build (env custom model, each scoped additional option, active custom model + fallback), so large catalogs (Fireworks ~280 entries) paid O(n) context rebuilds repeatedly. Build the RouteCatalogContext lazily once per options build and pass it through findRouteCatalogOption/hasOptionValue. Behavior unchanged; the catalog-miss path measures 6.1ms -> 4.1ms on a 277-entry catalog.
…dup helper OPENAI_BASE_URL and OPENAI_MODEL assigned in the scoped-cache test were immediately overwritten by getRouteCatalogModelOptions, so they never affected the exercised path. Remove the dead assignments; keep OPENAI_API_KEY to preserve the helper's auth path.
isGenuineSwitchProfileValue short-circuits on the switch-profile prefix, skipping the getModelOptions() rebuild for ordinary model ids. Track the gated getModelOptions binding ModelPicker captures (opt-in call-through mock in importFreshModelPicker) and assert it is never invoked for non-prefixed ids, alongside the existing false-result assertions.
0272a35 to
a69458c
Compare
|
Both review findings are addressed:
Branch rebased and pushed ( |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/components/ModelPicker.switchProfile.test.ts`:
- Around line 96-104: Update the tracked spy setup in the test around
getModelOptionsSpy so its mock delegates to the nonce-fresh modelOptions module
instance imported for gatedModelOptionsModule, rather than realModelOptions.
Keep the existing conditional module replacement intact and ensure tracked calls
use the fresh instance’s provider, auth, and providerProfiles bindings without
leaking global or configuration state.
In `@src/utils/model/modelOptions.catalogDedup.test.ts`:
- Around line 69-73: Update the catalog mock around getCatalogEntriesForRoute to
increment a catalogFetchCount on each fetch, reset that counter in beforeEach,
and assert the expected small fixed fetch bound in the large-catalog
options-build test. Keep the existing option-value assertions while verifying
the shared RouteCatalogContext eliminates per-lookup rescans.
In `@src/utils/model/modelOptions.ts`:
- Around line 1056-1069: Add regression coverage for hasOptionValue through
getModelOptions using a cached additional option whose option.value is the
catalog alias while the lookup uses the canonical value. Assert the canonical
lookup matches the alias-backed option and does not append a duplicate model,
specifically exercising the catalogOption.value comparison path.
🪄 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 Plus
Run ID: afe01071-a602-497b-a424-403d8d327584
📒 Files selected for processing (4)
src/components/ModelPicker.switchProfile.test.tssrc/components/ModelPicker.tsxsrc/utils/model/modelOptions.catalogDedup.test.tssrc/utils/model/modelOptions.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: smoke-and-tests (22)
- GitHub Check: typecheck
- GitHub Check: smoke-and-tests (24.11.x)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
TypeScript code in this repository must use strict mode and ESM imports.
**/*.{ts,tsx}: Add or update tests when a TypeScript or TSX change affects behavior.
Run the relevant TypeScript validation checks for changed code, includingbun run typecheckand, when applicable,bun run typecheck:type-tests.
Files:
src/components/ModelPicker.tsxsrc/utils/model/modelOptions.tssrc/components/ModelPicker.switchProfile.test.tssrc/utils/model/modelOptions.catalogDedup.test.ts
src/components/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use React + Ink for terminal UI components under
src/components/.
Files:
src/components/ModelPicker.tsx
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Preserve existing repository patterns unless intentionally refactoring them.
Keep changes small, readable, and focused; avoid broad rewrites or unrelated cleanup.
Do not reformat unrelated files, and keep comments useful and concise.
Update documentation when setup, commands, or user-facing behavior changes.
Review AI-generated changes for correctness, style consistency, unnecessary noise, and adherence to project architecture before submitting them.
Provider changes must follow the documented integration patterns indocs/integrations/overview.mdand the focused guides underdocs/integrations/how-to/.
When changing provider behavior, avoid breaking third-party providers and test the exact provider/model path changed when possible.
Provider pull requests must explicitly identify affected providers, limitations, and follow-up work.
Run the narrowest meaningful validation command for the touched area, and ensure relevant CI checks pass before merging.
Usebun installto install dependencies and the repository's Bun scripts for building, testing, smoke testing, and development.
Dependency changes require a concrete project benefit such as a bug fix, security issue, or approved feature; preference alone is insufficient.
Do not change the project's language, core runtime, or dependency stack, or introduce a new runtime, without prior maintainer agreement.
Keep each pull request focused on one issue or clearly scoped improvement and avoid bundling unrelated fixes, features, or refactors.
Files:
src/components/ModelPicker.tsxsrc/utils/model/modelOptions.tssrc/components/ModelPicker.switchProfile.test.tssrc/utils/model/modelOptions.catalogDedup.test.ts
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/components/ModelPicker.tsxsrc/utils/model/modelOptions.tssrc/components/ModelPicker.switchProfile.test.tssrc/utils/model/modelOptions.catalogDedup.test.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/utils/model/modelOptions.tssrc/utils/model/modelOptions.catalogDedup.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/components/ModelPicker.switchProfile.test.tssrc/utils/model/modelOptions.catalogDedup.test.ts
🪛 ast-grep (0.45.0)
src/components/ModelPicker.switchProfile.test.ts
[warning] 88-88: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(insecure-random-security-token-typescript)
src/utils/model/modelOptions.catalogDedup.test.ts
[warning] 93-93: Do not use Math.random() to generate security-sensitive values such as tokens, secrets, passwords, API keys, salts, nonces, OTPs, or session IDs. Math.random() is not cryptographically secure and is predictable. Use crypto.randomBytes()/crypto.randomUUID() (Node) or crypto.getRandomValues() (Web Crypto) instead.
Context: Math.random()
Note: [CWE-330] Use of Insufficiently Random Values.
(insecure-random-security-token-typescript)
🔇 Additional comments (5)
src/utils/model/modelOptions.ts (2)
941-962: LGTM!Also applies to: 976-979
1078-1096: LGTM!Also applies to: 1109-1115, 1133-1133, 1173-1176
src/utils/model/modelOptions.catalogDedup.test.ts (1)
93-116: LGTM!Also applies to: 149-182, 184-313
src/components/ModelPicker.tsx (1)
14-14: LGTM!Also applies to: 499-511
src/components/ModelPicker.switchProfile.test.ts (1)
30-75: LGTM!Also applies to: 108-117, 151-184, 186-276
3 participants
Summary
Fixes the severe lag in the
/modelpicker when a provider exposes many models (100+). Closes #2077.Root cause — two O(n²) hot spots compounding per keystroke:
getModelOptions()was O(n²) for catalog-backed routes:optionMatchesModelran a full catalog scan (getRouteCatalogModelOption) inside anoptions.some(...)loop, andgetCatalogOptionValuere-filtered the whole catalog per entry. A single call measured ~43ms on the 277-entry Fireworks catalog.isGenuineSwitchProfileValue→getModelOptions()on every keystroke, even for ordinary model ids that can never be cross-profile switch entries.Changes:
hasOptionValuehoists the catalog lookup out of the per-option loop.Set(getDuplicateCatalogApiNames).isGenuineSwitchProfileValueshort-circuits for values that don't start with the__switch_profile__:prefix (semantics preserved, including the documented literal-prefixed-id edge case).Measured:
getModelOptions()on the 100+ entry NVIDIA NIM catalog went from 43.5ms → 2.0ms (~22x), and no option-list rebuild happens per keystroke anymore.Impact
/modelarrow-key navigation is responsive again with large model catalogs (NVIDIA NIM, OpenRouter, big OpenAI-compatible endpoints).Testing
bun run buildbun run smokebun run check(2 pre-existing flaky failures also present onmainwithout this change)bun run typecheckbun test src/utils/model/ src/commands/model/ src/components/ModelPicker.test.tsx(236 pass)Notes
Summary by CodeRabbit
Bug Fixes
Tests