fix(models): show single provider card per provider#1609
Conversation
bmahabirbu
requested review from
benoitf and
gastoner
and removed request for
a team
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR modifies ChangesProvider-Level Aggregation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 `@extensions/ramalama/src/manager/inference-model-manager.spec.ts`:
- Around line 138-145: Extend the test around
inferenceModelManager.refreshRegistrationOfModels so after mocking
createOpenAICompatible to throw you assert the pre-existing connection was not
torn down: set up a prior registered connection, spy on its
dispose/unregister/close method, call refreshRegistrationOfModels (which
throws), then assert the spy was not called and that the manager still lists the
connection (e.g., via the manager's registeredConnections or a
getRegisteredModels method) to prove the old connection remains active. Use the
existing inferenceModelManager and createOpenAICompatible symbols from the diff
to locate the relevant setup and assertions.
In `@extensions/ramalama/src/manager/inference-model-manager.ts`:
- Around line 73-95: Do not dispose this.#connectionDisposable before you
successfully create and register the new provider: first call
createCompositeSDK(models) and attempt to register with
this.ramaLamaProvider.registerInferenceProviderConnection, and only if
registration succeeds replace/dispose the old this.#connectionDisposable (or
swap atomically). Ensure any thrown errors during SDK creation or register keep
the existing this.#connectionDisposable intact and propagate/log the error so
RamaLama remains connected until the new connection is confirmed.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb5d61b9-5c27-4804-873d-68e2a8cc865f
📒 Files selected for processing (2)
extensions/ramalama/src/manager/inference-model-manager.spec.tsextensions/ramalama/src/manager/inference-model-manager.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / macos-15
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: linter, formatters
- GitHub Check: typecheck
- GitHub Check: Windows
- GitHub Check: macOS
- GitHub Check: Linux
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases (e.g.,'/@/plugin/provider-registry.js') instead of relative paths (e.g.,'../plugin/provider-registry.js') for imports outside the current directory's module group. Relative imports are only used for sibling modules within the same directory.
Files:
extensions/ramalama/src/manager/inference-model-manager.spec.tsextensions/ramalama/src/manager/inference-model-manager.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.{ts,tsx}: Usetest()instead ofit()for test cases in Vitest unit tests
Usevi.mock(import('...'))for auto-mocking modules in Vitest. Avoid manual mock factories (vi.mock('...', () => ({...}))) when possible
Usevi.resetAllMocks()inbeforeEachfor resetting mocks in Vitest unit tests, notvi.clearAllMocks()
When an auto-mocked function or class method needs a real implementation in Vitest, usevi.mocked(...). For class methods, use the prototype pattern:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)
Files:
extensions/ramalama/src/manager/inference-model-manager.spec.ts
bmahabirbu
force-pushed
the
fix/ramalama-single-provider-card
branch
from
67118dd to
43000dd
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
jeffmaury
left a comment
jeffmaury
left a comment
There was a problem hiding this comment.
This PR is a wrong way of fixing the issue. The ramalama extension has a single provider and it's perfectly fine for an extension to have a single provider to register several connections. So should be fixed on the provider card so keep a single instance of each provider
|
Right its a ui fix let me update this quickly |
bmahabirbu
force-pushed
the
fix/ramalama-single-provider-card
branch
from
43000dd to
40959a8
Compare
bmahabirbu
changed the title
bmahabirbu
force-pushed
the
fix/ramalama-single-provider-card
branch
from
40959a8 to
5bab935
Compare
|
@jeffmaury done its a simple fix now |
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)
packages/renderer/src/lib/models/models-utils.spec.ts (1)
76-99: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd branch coverage for representative connection selection.
Current assertions validate aggregation, but they don’t exercise the two selection branches introduced in Line 69-70 (
startednot first, and nostartedfallback). Adding those cases will better protect this regression fix.Suggested test additions
+test('getInferenceConnectionSummaries prefers a started connection even when it is not first', () => { + const provider = { + id: 'openai', + name: 'OpenAI', + internalId: 'internal-1', + inferenceConnections: [ + { name: 'conn-stopped', type: 'self-hosted', status: 'stopped', models: [{ label: 'm1' }] }, + { name: 'conn-started', type: 'cloud', status: 'started', models: [{ label: 'm2' }] }, + ], + } as unknown as ProviderInfo; + + const [summary] = getInferenceConnectionSummaries([provider]); + expect(summary.connectionName).toBe('conn-started'); + expect(summary.status).toBe('started'); + expect(summary.modelCount).toBe(2); +}); + +test('getInferenceConnectionSummaries falls back to first connection when none are started', () => { + const provider = { + id: 'openai', + name: 'OpenAI', + internalId: 'internal-1', + inferenceConnections: [ + { name: 'conn-1', type: 'self-hosted', status: 'stopped', models: [{ label: 'm1' }] }, + { name: 'conn-2', type: 'cloud', status: 'error', models: [{ label: 'm2' }] }, + ], + } as unknown as ProviderInfo; + + const [summary] = getInferenceConnectionSummaries([provider]); + expect(summary.connectionName).toBe('conn-1'); + expect(summary.status).toBe('stopped'); + expect(summary.modelCount).toBe(2); +});🤖 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 `@packages/renderer/src/lib/models/models-utils.spec.ts` around lines 76 - 99, Add two unit tests for getInferenceConnectionSummaries to cover both selection branches: (1) a provider whose inferenceConnections has a 'started' connection not at index 0 to ensure the function picks the started one (e.g., connections order: stopped, started), and (2) a provider with no 'started' connections to verify the fallback selection logic (e.g., picks first or the expected fallback). Use distinct provider objects and assert the returned summary fields (connectionName, connectionType, status, modelCount, creationDisplayName) to match the chosen connection in each case so both branches introduced around lines 69-70 are exercised.
🤖 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 `@packages/renderer/src/lib/models/models-utils.spec.ts`:
- Around line 76-99: Add two unit tests for getInferenceConnectionSummaries to
cover both selection branches: (1) a provider whose inferenceConnections has a
'started' connection not at index 0 to ensure the function picks the started one
(e.g., connections order: stopped, started), and (2) a provider with no
'started' connections to verify the fallback selection logic (e.g., picks first
or the expected fallback). Use distinct provider objects and assert the returned
summary fields (connectionName, connectionType, status, modelCount,
creationDisplayName) to match the chosen connection in each case so both
branches introduced around lines 69-70 are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 632bbd85-072b-4cd6-b87f-6ecf2d0bd2c3
📒 Files selected for processing (2)
packages/renderer/src/lib/models/models-utils.spec.tspackages/renderer/src/lib/models/models-utils.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: smoke-e2e-tests (dev) / ubuntu-24.04 (ollama)
- GitHub Check: smoke-e2e-tests (prod) / ubuntu-24.04 (ollama)
- GitHub Check: unit tests / macos-15
- GitHub Check: linter, formatters
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: Windows
- GitHub Check: unit tests / windows-2025
- GitHub Check: typecheck
- GitHub Check: Linux
- GitHub Check: macOS
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
/@/path aliases (e.g.,'/@/plugin/provider-registry.js') instead of relative paths (e.g.,'../plugin/provider-registry.js') for imports outside the current directory's module group. Relative imports are only used for sibling modules within the same directory.
Files:
packages/renderer/src/lib/models/models-utils.tspackages/renderer/src/lib/models/models-utils.spec.ts
packages/renderer/src/**/*.{ts,tsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
External URLs in the renderer process require user confirmation (handled via
setupSecurityRestrictionsOnLinks)
Files:
packages/renderer/src/lib/models/models-utils.tspackages/renderer/src/lib/models/models-utils.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.spec.{ts,tsx}: Usetest()instead ofit()for test cases in Vitest unit tests
Usevi.mock(import('...'))for auto-mocking modules in Vitest. Avoid manual mock factories (vi.mock('...', () => ({...}))) when possible
Usevi.resetAllMocks()inbeforeEachfor resetting mocks in Vitest unit tests, notvi.clearAllMocks()
When an auto-mocked function or class method needs a real implementation in Vitest, usevi.mocked(...). For class methods, use the prototype pattern:vi.mocked(MyClass.prototype.myMethod).mockImplementation(...)
Files:
packages/renderer/src/lib/models/models-utils.spec.ts
🧠 Learnings (2)
📚 Learning: 2026-04-15T08:51:08.199Z
Learnt from: MarsKubeX
Repo: openkaiden/kaiden PR: 1336
File: packages/renderer/src/lib/guided-setup/GuidedSetup.spec.ts:64-69
Timestamp: 2026-04-15T08:51:08.199Z
Learning: For Svelte component test files under `packages/renderer/src/**` with names ending in `.spec.ts` that use `render` from `testing-library/svelte`, call `vi.useFakeTimers({ shouldAdvanceTime: true })` inside a `beforeEach`. Keep it in `beforeEach` for these tests to avoid flakiness from Svelte’s internal tick scheduling; do not remove or flag it based on the presence/absence of explicit timer manipulation in individual tests.
Applied to files:
packages/renderer/src/lib/models/models-utils.spec.ts
📚 Learning: 2026-04-22T02:58:52.726Z
Learnt from: bmahabirbu
Repo: openkaiden/kaiden PR: 1379
File: packages/renderer/src/lib/models/ModelsCatalogEmptyScreen.spec.ts:32-38
Timestamp: 2026-04-22T02:58:52.726Z
Learning: In tests under packages/renderer/src/lib/models/**/*.spec.ts, it’s an intentional Testing Library convention to assert user-facing copy using exact string matches with screen.getByText(...). Do not flag these exact-string getByText assertions as “brittle” or suggest replacing them with regex matchers or data-testid in these specific test files, because the goal is to fail when the wording changes.
Applied to files:
packages/renderer/src/lib/models/models-utils.spec.ts
🔇 Additional comments (1)
packages/renderer/src/lib/models/models-utils.ts (1)
69-81: Single-summary aggregation logic looks correct.This change cleanly implements one summary per provider while preserving representative connection metadata and aggregating model counts across connections.
|
@CodeRabbit resolve |
✅ Actions performedComments resolved and changes approved. |
Group inference connections by provider in getInferenceConnectionSummaries so providers like RamaLama that register multiple connections show one card instead of duplicates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Brian <bmahabir@bu.edu>
bmahabirbu
force-pushed
the
fix/ramalama-single-provider-card
branch
from
5bab935 to
e3b4491
Compare
|
LGTM, 2 ramalama servers => 1 ramalama card. e2e tests fail on some MCP test, I just restarted them |
3 participants
Summary
getInferenceConnectionSummariesin the renderer to group connections by provider, producing one card per provider with an aggregated model countFixes: #1602
#1610 is related
Test plan
🤖 Generated with Claude Code