Skip to content

Add xAI API provider support#1011

Open
PeterDaveHello wants to merge 1 commit into
ChatGPTBox-dev:masterfrom
PeterDaveHello:add-xai-provider
Open

Add xAI API provider support#1011
PeterDaveHello wants to merge 1 commit into
ChatGPTBox-dev:masterfrom
PeterDaveHello:add-xai-provider

Conversation

@PeterDaveHello

@PeterDaveHello PeterDaveHello commented Jul 17, 2026

Copy link
Copy Markdown
Member

Summary

  • Add xAI as a built-in OpenAI-compatible API provider.
  • Add Grok 4.5 and Grok 4.3 presets, with Grok 4.5 enabled by default.
  • Wire API-key setup, request routing, legacy preset mapping, and focused provider coverage.

Validation

  • npm test (795 tests passed)
  • npm run lint
  • npm run build

Manual browser smoke testing was not run because a browser extension runtime is not available in this environment.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds xAI Grok API models, provider configuration, API-key mapping, OpenAI-compatible session routing, setup URL handling, provider resolution, and unit-test coverage.

Changes

xAI provider integration

Layer / File(s) Summary
xAI model and provider contracts
src/config/index.mjs, src/config/openai-provider-mappings.mjs, src/services/apis/provider-registry.mjs
Registers xAI models, configuration, API-key mappings, builtin provider details, legacy model resolution, and model detection.
xAI provider routing and setup
src/background/index.mjs, src/popup/sections/GeneralPart.jsx
Routes xAI sessions through the OpenAI-compatible API and provides the xAI API-key setup URL.
xAI integration tests
tests/unit/config/config-predicates.test.mjs, tests/unit/services/apis/provider-registry.test.mjs, tests/unit/services/apis/thin-adapters.test.mjs
Validates model detection, provider resolution, XAI endpoint and authorization handling, and assistant output updates.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Session
  participant Background
  participant OpenAICompatibleAPI
  participant XAI
  Session->>Background: Select xAI model session
  Background->>OpenAICompatibleAPI: Generate answers
  OpenAICompatibleAPI->>XAI: Send chat completion request
  XAI-->>OpenAICompatibleAPI: Return assistant output
  OpenAICompatibleAPI-->>Session: Update conversation record
Loading

Possibly related PRs

Suggested reviewers: josstorer

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding xAI API provider support.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kilo-code-bot

kilo-code-bot Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental re-review of PR 1011 against current HEAD (2f937f2). The previous incremental base commit (d5f235ed) was no longer reachable from the PR branch, so this run verified the full current PR diff (8 files, 55 insertions).

All prior P2 findings are confirmed resolved in earlier commits and remain fixed in the current code:

  • Config migration no longer writes sessions and is value-aware for secrets (config/index.mjs).
  • normalizedProviderId redeclaration removed in the fallback branch (provider-registry.mjs:732).
  • URL-aware resolveRenamedProviderId preserves custom-provider IDs across ambiguous legacy IDs.

The new xAI provider wiring — model keys/presets (xaiGrok4_5, xaiGrok4_3), xaiApiKey config field and default config, isUsingXaiApiModel predicate, openai-provider-mappings.mjs legacy-key and group mappings, provider-registry.mjs built-in registration (xai / baseUrl https://api.x.ai/v1) and resolveProviderIdFromLegacyModelName branch, background/index.mjs session routing, and GeneralPart.jsx setup URL — mirrors the existing provider pattern and is covered by the new unit tests (provider-registry, thin-adapters, config-predicates). The GeneralPart.jsx:64 console.x.ai/team/default/api-keys URL is an already-resolved discussion (author confirmed default is a valid xAI Console alias), not an open defect.

Files Reviewed (8 files in current diff)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/GeneralPart.jsx
  • src/services/apis/provider-registry.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs
Previous Review Summaries (12 snapshots, latest commit d5f235e)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit d5f235e)

Status: No Issues Found | Recommendation: Merge

Full re-review of all 17 changed files against current HEAD (90d783a). The previous incremental base (73328bb0) is no longer reachable from the PR branch, so this run re-verified the entire PR diff.

All prior P2 findings are confirmed resolved against current code:

  • Config migration no longer writes sessions (fixed in d541947), eliminating the session write race.
  • Migration is value-aware for secrets and no longer drops the legacy key on rename (fixed in d541947).
  • getOpenAICompatibleRequestDiagnostic uses hasNormalizedProviderIdMatch (provider-registry.mjs:592-601), which checks legacyProviderIds, resolving the legacy-ID diagnostic gap (verified).
  • normalizedProviderId redeclaration removed in the fallback branch (fixed in 90d783a; verified at provider-registry.mjs:774-781).
  • URL-aware resolveRenamedProviderId preserves custom-provider IDs across ambiguous legacy IDs (verified).

New production code — xAI builtin provider registration/presets (provider-registry.mjs:81-88, config/index.mjs), xaiApiKey wiring and model keys (openai-provider-mappings.mjs), areProviderIdsEquivalent/isApiModeSelected/getUniquelySelectedApiModeIndex disambiguation, ensureLeadingSlash/getProviderChatCompletionsUrlForCompare helpers, completedBuiltinProviderIdMigrations bookkeeping, import-data-cleanup marker, and the background/index.mjs / ApiModes.jsx call-site updates — is consistent and well covered by the PR's 841 passing tests. The remaining GeneralPart.jsx:66 console URL item is an already-resolved discussion (author confirmed default is an xAI Console alias), not an open defect.

Files Reviewed (17 files)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/ApiModes.jsx
  • src/popup/sections/GeneralPart.jsx
  • src/popup/sections/api-modes-provider-utils.mjs
  • src/popup/sections/import-data-cleanup.mjs
  • src/services/apis/provider-registry.mjs
  • src/utils/model-name-convert.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/config/migrate-user-config.test.mjs
  • tests/unit/config/migrate-xai-provider.test.mjs
  • tests/unit/popup/api-modes-provider-utils.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs
  • tests/unit/utils/model-name-convert.test.mjs

Previous review (commit 90d783a)

Status: No Issues Found | Recommendation: Merge

Full re-review of all 17 changed files against current HEAD (90d783a). The previous incremental base (73328bb0) is no longer reachable from the PR branch, so this run re-verified the entire PR diff.

All prior P2 findings are confirmed resolved against current code:

  • Config migration no longer writes sessions (fixed in d541947), eliminating the session write race.
  • Migration is value-aware for secrets and no longer drops the legacy key on rename (fixed in d541947).
  • getOpenAICompatibleRequestDiagnostic uses hasNormalizedProviderIdMatch (provider-registry.mjs:592-601), which checks legacyProviderIds, resolving the legacy-ID diagnostic gap (verified).
  • normalizedProviderId redeclaration removed in the fallback branch (fixed in 90d783a; verified at provider-registry.mjs:774-781).
  • URL-aware resolveRenamedProviderId preserves custom-provider IDs across ambiguous legacy IDs (verified).

New production code — xAI builtin provider registration/presets (provider-registry.mjs:81-88, config/index.mjs), xaiApiKey wiring and model keys (openai-provider-mappings.mjs), areProviderIdsEquivalent/isApiModeSelected/getUniquelySelectedApiModeIndex disambiguation, ensureLeadingSlash/getProviderChatCompletionsUrlForCompare helpers, completedBuiltinProviderIdMigrations bookkeeping, import-data-cleanup marker, and the background/index.mjs / ApiModes.jsx call-site updates — is consistent and well covered by the PR's 841 passing tests. The remaining GeneralPart.jsx:66 console URL item is an already-resolved discussion (author confirmed default is an xAI Console alias), not an open defect.

Files Reviewed (17 files)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/ApiModes.jsx
  • src/popup/sections/GeneralPart.jsx
  • src/popup/sections/api-modes-provider-utils.mjs
  • src/popup/sections/import-data-cleanup.mjs
  • src/services/apis/provider-registry.mjs
  • src/utils/model-name-convert.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/config/migrate-user-config.test.mjs
  • tests/unit/config/migrate-xai-provider.test.mjs
  • tests/unit/popup/api-modes-provider-utils.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs
  • tests/unit/utils/model-name-convert.test.mjs

Previous review (commit 73328bb)

Status: No Issues Found | Recommendation: Merge

Full re-review of all 17 changed files against current HEAD (73328bb). The previous incremental base (a109bd9) was no longer reachable, so this run re-verified the entire PR diff.

Prior P2 findings (config migration session write race, duplicate/ambiguous provider-ID mapping, secret drop on rename, diagnostic ignores legacy IDs) were already fixed in earlier commits (58a2730, d541947) and are confirmed resolved against current code:

  • Config migration is value-aware for secrets and no longer writes sessions.
  • getOpenAICompatibleRequestDiagnostic already uses hasNormalizedProviderIdMatch (provider-registry.mjs:592-601), which does check legacyProviderIds, so the legacy-ID diagnostic gap is resolved.
  • URL-aware resolveRenamedProviderId preserves custom-provider IDs across ambiguous legacy IDs.

New production code reviewed — xAI builtin provider registration/presets (provider-registry.mjs, config/index.mjs), xaiApiKey wiring and model keys (openai-provider-mappings.mjs), areProviderIdsEquivalent/isApiModeSelected/getUniquelySelectedApiModeIndex disambiguation, ensureLeadingSlash/getProviderChatCompletionsUrlForCompare helpers, completedBuiltinProviderIdMigrations bookkeeping, import-data-cleanup marker, and the background/index.mjs / ApiModes.jsx call-site updates — is consistent and well covered by 1,400+ lines of focused regression tests (841 tests passing per PR description).

Files Reviewed (17 files)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/ApiModes.jsx
  • src/popup/sections/GeneralPart.jsx
  • src/popup/sections/api-modes-provider-utils.mjs
  • src/popup/sections/import-data-cleanup.mjs
  • src/services/apis/provider-registry.mjs
  • src/utils/model-name-convert.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/config/migrate-user-config.test.mjs
  • tests/unit/config/migrate-xai-provider.test.mjs
  • tests/unit/popup/api-modes-provider-utils.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs
  • tests/unit/utils/model-name-convert.test.mjs

Previous review (commit a109bd9)

Status: No Issues Found | Recommendation: Merge

Full re-review of all 16 changed files against the current HEAD (a109bd9). The previous incremental base commit (ad570ab) is no longer reachable, so this run re-verified the entire PR diff.

Prior P2 findings (config migration session write, duplicate/ambiguous provider-ID mapping, secret drop on rename) were already fixed in earlier commits (58a2730, d541947) and are confirmed resolved against current code. No new bugs, security vulnerabilities, runtime errors, logic errors, or breaking changes were found in the changed lines.

New production code reviewed — xAI builtin provider registration/presets (provider-registry.mjs), xaiApiKey wiring and model keys (config/index.mjs, openai-provider-mappings.mjs), ensureLeadingSlash/getProviderChatCompletionsUrlForCompare helpers, completedBuiltinProviderIdMigrations bookkeeping, URL-aware resolveRenamedProviderId, legacy-ID matching/clearing in applySelectedProviderToApiMode/sanitizeApiModeForSave/getProvidersMatchingSessionProviderId, areProviderIdsEquivalent/isApiModeSelected/getUniquelySelectedApiModeIndex disambiguation, import-data-cleanup marker, and the setup URL in GeneralPart.jsx — is consistent and well covered by 627 lines of focused regression tests (831 tests passing per PR description).

Files Reviewed (16 files)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/ApiModes.jsx
  • src/popup/sections/GeneralPart.jsx
  • src/popup/sections/api-modes-provider-utils.mjs
  • src/popup/sections/import-data-cleanup.mjs
  • src/services/apis/provider-registry.mjs
  • src/utils/model-name-convert.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/config/migrate-xai-provider.test.mjs
  • tests/unit/popup/api-modes-provider-utils.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs
  • tests/unit/utils/model-name-convert.test.mjs

Previous review (commit ad570ab)

Status: No Issues Found | Recommendation: Merge

The previous incremental base commit (90a7795c) is no longer reachable (history was rewritten; current HEAD is ad570ab), so this run performed a full re-review of all changed files against the current diff.

Prior P2 findings are verified resolved in the current HEAD:

  • Config migration session writegetUserConfig no longer reads or writes sessions; historical sessions are resolved at runtime via legacyProviderIds with URL-aware disambiguation instead of mutating persisted snapshots.
  • Duplicate/ambiguous provider ID mappingresolveRenamedProviderId / getProvidersMatchingSessionProviderId / resolveOpenAICompatibleRequest use a URL-aware resolver; ambiguous, empty, and no-match cases stay unmapped.
  • Secret drop on renamemigrateUserConfig uses value-aware normalizeText checks and only deletes raw slots when the captured oldSecret came from them; covered by regression tests.

New production code reviewed — xAI builtin provider registration and presets, xaiApiKey wiring, ensureLeadingSlash/getProviderChatCompletionsUrlForCompare helpers, completedBuiltinProviderIdMigrations bookkeeping, legacy-ID matching/clearing in applySelectedProviderToApiMode/sanitizeApiModeForSave, isApiModeSelected/getUniquelySelectedApiModeIndex URL disambiguation, import-data-cleanup migration marker, and the setup URL in GeneralPart.jsx — no new bugs, security issues, or logic errors found. The xAI setup-URL default alias was already raised and addressed by the author in an existing thread.

Files Reviewed (16 files)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/ApiModes.jsx
  • src/popup/sections/GeneralPart.jsx
  • src/popup/sections/api-modes-provider-utils.mjs
  • src/popup/sections/import-data-cleanup.mjs
  • src/services/apis/provider-registry.mjs
  • src/utils/model-name-convert.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/config/migrate-xai-provider.test.mjs
  • tests/unit/popup/api-modes-provider-utils.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs
  • tests/unit/utils/model-name-convert.test.mjs

Previous review (commit 90a7795)

Status: No Issues Found | Recommendation: Merge

Incremental review collapsed to a full review because the previous commit (58a2730) is no longer reachable (history was rewritten; current head is d541947). All 15 changed files were re-reviewed against the current diff.

Prior P2 findings are verified resolved in the current head:

  • Session write racegetUserConfig no longer reads or writes sessions (confirmed: no sessions read/write in src/config/index.mjs payload building at lines 1846-1879). Historical sessions are resolved at runtime via legacyProviderIds metadata with URL-aware disambiguation, not by mutating persisted session snapshots.
  • Duplicate provider IDresolveRenamedProviderId / getProvidersMatchingSessionProviderId use the same URL-aware resolver as custom API modes; ambiguous/empty/no-match cases stay unmapped.
  • Secret drop on renamemigrateUserConfig now uses value-aware normalizeText checks (rawSecretValue/normalizedSecretValue) and only deletes raw slots when the captured oldSecret came from them; covered by regression tests.

New code reviewed (helpers ensureLeadingSlash/getProviderChatCompletionsUrlForCompare, secret-migration block, resolveOpenAICompatibleRequest legacy-ID matching, applySelectedProviderToApiMode legacy-ID clearing, getReferencedCustomProviderIdsFromSessions/getApiModeDisplayLabel/getConversationAiName disambiguation, import-data-cleanup migration markers, isApiModeSelected/getUniquelySelectedApiModeIndex URL disambiguation, xAI presets/provider registration) — no new bugs, security issues, or logic errors found. No orphaned references to renamed helpers.

Files Reviewed (15 files)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/GeneralPart.jsx
  • src/popup/sections/api-modes-provider-utils.mjs
  • src/popup/sections/import-data-cleanup.mjs
  • src/services/apis/provider-registry.mjs
  • src/utils/model-name-convert.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/config/migrate-xai-provider.test.mjs
  • tests/unit/popup/api-modes-provider-utils.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs
  • tests/unit/utils/model-name-convert.test.mjs

Previous review (commit d541947)

Status: No Issues Found | Recommendation: Merge

Incremental review collapsed to a full review because the previous commit (58a2730) is no longer reachable (history was rewritten; current head is d541947). All 15 changed files were re-reviewed against the current diff.

Prior P2 findings are verified resolved in the current head:

  • Session write racegetUserConfig no longer reads or writes sessions (confirmed: no sessions read/write in src/config/index.mjs payload building at lines 1846-1879). Historical sessions are resolved at runtime via legacyProviderIds metadata with URL-aware disambiguation, not by mutating persisted session snapshots.
  • Duplicate provider IDresolveRenamedProviderId / getProvidersMatchingSessionProviderId use the same URL-aware resolver as custom API modes; ambiguous/empty/no-match cases stay unmapped.
  • Secret drop on renamemigrateUserConfig now uses value-aware normalizeText checks (rawSecretValue/normalizedSecretValue) and only deletes raw slots when the captured oldSecret came from them; covered by regression tests.

New code reviewed (helpers ensureLeadingSlash/getProviderChatCompletionsUrlForCompare, secret-migration block, resolveOpenAICompatibleRequest legacy-ID matching, applySelectedProviderToApiMode legacy-ID clearing, getReferencedCustomProviderIdsFromSessions/getApiModeDisplayLabel/getConversationAiName disambiguation, import-data-cleanup migration markers, isApiModeSelected/getUniquelySelectedApiModeIndex URL disambiguation, xAI presets/provider registration) — no new bugs, security issues, or logic errors found. No orphaned references to renamed helpers.

Files Reviewed (15 files)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/GeneralPart.jsx
  • src/popup/sections/api-modes-provider-utils.mjs
  • src/popup/sections/import-data-cleanup.mjs
  • src/services/apis/provider-registry.mjs
  • src/utils/model-name-convert.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/config/migrate-xai-provider.test.mjs
  • tests/unit/popup/api-modes-provider-utils.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs
  • tests/unit/utils/model-name-convert.test.mjs

Previous review (commit 58a2730)

Status: No Issues Found | Recommendation: Merge

Incremental review against the previous run (commit b71c520658a2730). This iteration refactors getUserConfig so session provider-ID remapping is performed from the latest sessions snapshot read immediately before the single combined storage write, and routes session remapping through the same URL-aware resolveRenamedProviderId resolver used for custom API modes (ambiguous/empty/no-match cases are left unmapped rather than guessed). It adds regression tests covering the latest-snapshot read and all four URL-disambiguation paths.

The prior P2/session-write-race and duplicate-provider-ID findings were confirmed addressed by the author in 58a2730 (CodeRabbit marked the duplicate-ID item resolved). I re-verified the changed lines in src/config/index.mjs (renaming sessionProviderIdRenameLookup/resolveRenamedProviderIdForCustomModeresolveRenamedProviderId, the late sessions read at lines 1872-1878, and the remapSessionProviderIds signature change) and found no new issues: no orphaned references, correct function-guard at line 1038, and consistent URL-matching logic at lines 1193-1211. The existing GeneralPart.jsx:66 xAI console link discussion is outside this diff and unchanged.

Files Reviewed (2 files)
  • src/config/index.mjs
  • tests/unit/config/migrate-xai-provider.test.mjs

Previous review (commit b71c520)

Status: No Issues Found | Recommendation: Merge

Incremental review against the previous run (commit fa8e341ab71c5206). This iteration adds a one-time migration for newly-reserved builtin provider IDs (xai): pre-existing user custom providers whose id collides with the new builtin xai are renamed (e.g. xai-2) and their secrets are moved out of the builtin providerSecrets.xai slot without clobbering a genuine builtin xAI key. It also remaps affected session/apiMode provider IDs (with URL-based disambiguation for ambiguous renames) and reruns the migration on legacy imports via completedBuiltinProviderIdMigrations.

I traced the rename→copy→delete ordering across all changed branches in migrateUserConfig and cross-checked the new migrate-xai-provider tests: secrets are consistently copied to the renamed provider before the old slot is removed, the real builtin key is restored when it does not belong to a custom provider, and ambiguous renames are left unmapped for sessions while custom modes use URL disambiguation. No secret-loss or wrong-key defects found on changed lines.

Files Reviewed (4 files)
  • src/config/index.mjs
  • src/popup/sections/import-data-cleanup.mjs
  • tests/unit/config/migrate-xai-provider.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs

Note: the pre-existing discussion on src/popup/sections/GeneralPart.jsx:66 (xAI API key setup link) is unchanged by this iteration and was left as-is.

Previous review (commit fa8e341)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (8 files)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/GeneralPart.jsx
  • src/services/apis/provider-registry.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs

Incremental review against the previous run (commit b90539c9): the new xAI provider wiring follows the established OpenAI-compatible provider pattern (Google/OpenRouter) consistently across config, provider registry, background routing, popup setup URL, and legacy model-name resolution. Predicates and tests are aligned with the new model keys. No new issues found.

Previous review (commit 19cb00b)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (8 files)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/GeneralPart.jsx
  • src/services/apis/provider-registry.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs

Incremental review against the previous run (commit b90539c9): the new xAI provider wiring follows the established OpenAI-compatible provider pattern (Google/OpenRouter) consistently across config, provider registry, background routing, popup setup URL, and legacy model-name resolution. Predicates and tests are aligned with the new model keys. No new issues found.

Previous review (commit b90539c)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (8 files)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/GeneralPart.jsx
  • src/services/apis/provider-registry.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs

Reviewed by hy3:free · Input: 26.7K · Output: 2.1K · Cached: 144K

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add xAI as an OpenAI-compatible API provider with Grok presets

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Register xAI as a built-in OpenAI-compatible provider and route API-mode sessions to it.
• Add Grok 4.5 and Grok 4.3 model presets plus xAI API-key storage/mappings.
• Extend UI setup links and add unit coverage for predicates, registry resolution, and adapters.
Diagram

graph TD
  UI("API mode UI") --> BG["background/index.mjs"] --> REG["provider-registry.mjs"] --> CLIENT["openai-api.mjs"] --> XAI{{"xAI API"}}
  UI --> CFG["config/index.mjs"] --> REG

  subgraph Legend
    direction LR
    _ui("UI") ~~~ _svc["Runtime/service"] ~~~ _mod["Config/module"] ~~~ _ext{{"External API"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Treat xAI as a custom OpenAI provider only (no builtin)
  • ➕ No builtin registry/mapping changes; relies on existing custom provider UI
  • ➖ Worse UX (no provider preset, no dedicated setup link)
  • ➖ Harder to ensure stable base URL/path defaults
  • ➖ Less confidence without dedicated tests for providerId/secret routing
2. Implement a dedicated xAI adapter (not OpenAI-compatible)
  • ➕ Can expose xAI-specific parameters/features cleanly if they diverge from OpenAI compat
  • ➖ Duplicates the existing OpenAI-compatible request stack
  • ➖ More surface area to maintain and test
3. Alias Grok models into an existing provider group
  • ➕ Minimizes new config groups and predicates
  • ➖ Conflates providers and API key ownership
  • ➖ Makes provider-specific setup links and secrets mapping ambiguous

Recommendation: The PR’s approach (builtin OpenAI-compatible provider + new apiMode group + legacy secret mapping) is the best fit: it reuses the existing compat pipeline, keeps provider identity explicit (for routing and key storage), and adds targeted tests to prevent regressions. Alternatives either degrade UX/confuse ownership (custom-only/aliasing) or increase maintenance cost (dedicated adapter).

Files changed (8) +117 / -0

Enhancement (3) +13 / -0
index.mjsInclude xAI sessions in OpenAI-compatible API routing +2/-0

Include xAI sessions in OpenAI-compatible API routing

• Imports the new xAI model predicate and includes it in the OpenAI-compatible session check. This ensures xAI API-mode sessions flow through the shared compat execution path.

src/background/index.mjs

GeneralPart.jsxAdd xAI API key setup link to provider UI +2/-0

Add xAI API key setup link to provider UI

• Extends provider setup URL resolution to include the xAI console API key page. Improves discoverability for configuring xAI credentials.

src/popup/sections/GeneralPart.jsx

provider-registry.mjsRegister builtin xAI provider and resolve legacy xAI model names +9/-0

Register builtin xAI provider and resolve legacy xAI model names

• Adds xAI to the builtin provider template with baseUrl https://api.x.ai/v1 and chat completions path. Updates legacy model-name-to-provider resolution so Grok presets and the xaiApiModelKeys group map to providerId 'xai'.

src/services/apis/provider-registry.mjs

Tests (3) +84 / -0
config-predicates.test.mjsTest xAI model predicate detection +9/-0

Test xAI model predicate detection

• Adds coverage ensuring isUsingXaiApiModel returns true for all xaiApiModelKeys and false for non-xAI models. Prevents regressions in provider selection logic.

tests/unit/config/config-predicates.test.mjs

provider-registry.test.mjsTest xAI provider registration and request resolution +68/-0

Test xAI provider registration and request resolution

• Verifies xAI is present in the full provider list with expected base URL/path. Adds tests for resolving providerId from legacy names and apiMode, and for resolving requests using both legacy xaiApiKey and providerSecrets[xai].

tests/unit/services/apis/provider-registry.test.mjs

thin-adapters.test.mjsAdd thin-adapter coverage for xAI base URL and Authorization header +7/-0

Add thin-adapter coverage for xAI base URL and Authorization header

• Adds an xAI adapter case to validate the compat layer uses the expected https://api.x.ai/v1 base URL and forwards the provider secret into the Bearer Authorization header.

tests/unit/services/apis/thin-adapters.test.mjs

Other (2) +20 / -0
index.mjsAdd xAI model group, Grok presets, and xaiApiKey config field +18/-0

Add xAI model group, Grok presets, and xaiApiKey config field

• Introduces xaiApiModelKeys, registers the group in ModelGroups, and adds Grok 4.5/4.3 presets to Models. Extends defaultConfig with xaiApiKey and provides an isUsingXaiApiModel predicate wrapper.

src/config/index.mjs

openai-provider-mappings.mjsMap xAI providerId to legacy key field and apiMode group +2/-0

Map xAI providerId to legacy key field and apiMode group

• Adds providerId→legacy key field mapping for xaiApiKey and wires xaiApiModelKeys into the group-to-provider mapping. This enables providerSecrets/legacy-field interop for xAI.

src/config/openai-provider-mappings.mjs

@qodo-code-review

qodo-code-review Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Secret dropped on rename ✓ Resolved 🐞 Bug ≡ Correctness
Description
In migrateUserConfig(), a colliding custom provider can migrate an empty normalized providerSecrets
entry (e.g. providerSecrets['xai']==='') while still deleting the non-empty raw-key entry (e.g.
providerSecrets['XAI']==='real-key'), permanently losing the secret for the renamed provider. This
happens because the collision logic treats “own property exists” as “secret exists”, even when its
value is empty, and schedules the raw slot for deletion regardless of which slot was migrated.
Code

src/config/index.mjs[R1249-1318]

+    const hasRawIdSecret = Object.hasOwn(providerSecretSnapshot, oldRawProviderId)
+    const hasNormalizedIdSecret = Object.hasOwn(providerSecretSnapshot, oldProviderId)
+    const hasDistinctRawId = oldRawProviderId !== oldProviderId
    const usesBuiltinSecretSlot = builtinProviderIds.has(oldProviderId)
-    if (usesBuiltinSecretSlot && !hasRawIdSecret) continue
-    if (!usesBuiltinSecretSlot && !hasRawIdSecret && !hasNormalizedIdSecret) continue
-    const rawIdSecret = hasRawIdSecret ? providerSecrets[oldRawProviderId] : undefined
-    const normalizedIdSecret = hasNormalizedIdSecret ? providerSecrets[oldProviderId] : undefined
-    const oldSecret = usesBuiltinSecretSlot
-      ? rawIdSecret
-      : hasRawIdSecret && rawIdSecret !== ''
-      ? rawIdSecret
-      : hasNormalizedIdSecret
-      ? normalizedIdSecret
-      : rawIdSecret
+    const isPendingBuiltinProviderIdMigration =
+      usesBuiltinSecretSlot && pendingBuiltinProviderIdMigrations.has(oldProviderId)
+    const hasMultipleProviderIdRenames = (providerIdRenameCounts.get(oldProviderId) || 0) > 1
+    const rawIdSecret = hasRawIdSecret ? providerSecretSnapshot[oldRawProviderId] : undefined
+    const normalizedIdSecret = hasNormalizedIdSecret
+      ? providerSecretSnapshot[oldProviderId]
+      : undefined
+    const builtinLegacyKey = LEGACY_API_KEY_FIELD_BY_PROVIDER_ID[oldProviderId]
+    const builtinLegacySecret = builtinLegacyKey ? normalizeText(migrated[builtinLegacyKey]) : ''
+    const normalizedSecretBelongsToBuiltin =
+      isPendingBuiltinProviderIdMigration &&
+      builtinLegacySecret &&
+      builtinLegacySecret === normalizeText(normalizedIdSecret)
+    if (hasDistinctRawId && hasRawIdSecret) {
+      rawProviderSecretIdsToDelete.add(oldRawProviderId)
+    }
+    let hasOldSecret = false
+    let oldSecret
+    let usesNormalizedBuiltinSecretSlot = false
+    if (
+      hasDistinctRawId &&
+      hasRawIdSecret &&
+      (hasMultipleProviderIdRenames ||
+        !isPendingBuiltinProviderIdMigration ||
+        !hasNormalizedIdSecret ||
+        normalizedSecretBelongsToBuiltin)
+    ) {
+      hasOldSecret = true
+      oldSecret =
+        !usesBuiltinSecretSlot && rawIdSecret === '' && hasNormalizedIdSecret
+          ? normalizedIdSecret
+          : rawIdSecret
+    } else if (
+      isPendingBuiltinProviderIdMigration &&
+      hasNormalizedIdSecret &&
+      !normalizedSecretBelongsToBuiltin
+    ) {
+      hasOldSecret = true
+      oldSecret = normalizedIdSecret
+      usesNormalizedBuiltinSecretSlot = true
+    } else if (!usesBuiltinSecretSlot && (hasRawIdSecret || hasNormalizedIdSecret)) {
+      hasOldSecret = true
+      oldSecret =
+        hasRawIdSecret && rawIdSecret !== ''
+          ? rawIdSecret
+          : hasNormalizedIdSecret
+          ? normalizedIdSecret
+          : rawIdSecret
+    }
    if (
-      !Object.hasOwn(providerSecrets, newProviderId) ||
-      providerSecrets[newProviderId] !== oldSecret
+      hasOldSecret &&
+      (!Object.hasOwn(providerSecrets, newProviderId) ||
+        providerSecrets[newProviderId] !== oldSecret)
    ) {
      providerSecrets[newProviderId] = oldSecret
      dirty = true
    }
-    if (hasRawIdSecret && oldRawProviderId !== oldProviderId) {
-      delete providerSecrets[oldRawProviderId]
+    if (usesNormalizedBuiltinSecretSlot) {
+      builtinProviderSecretIdsToDelete.set(oldProviderId, normalizedIdSecret)
+    }
+  }
+  for (const providerId of rawProviderSecretIdsToDelete) {
+    if (Object.hasOwn(providerSecrets, providerId)) {
+      delete providerSecrets[providerId]
+      dirty = true
+    }
Evidence
The migration code schedules deletion of the raw secret slot whenever the raw provider id differs
and exists, but the pending-builtin collision branch can choose normalizedIdSecret as oldSecret
even when that value is empty (because hasNormalizedIdSecret is based on property existence, not
value). Separately, the UI/config utilities can store empty-string providerSecrets values while
keeping the key as an own-property, making this state reachable.

src/config/index.mjs[1240-1318]
src/popup/sections/provider-secret-utils.mjs[241-256]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
During custom provider ID collision migration (e.g. custom provider `XAI` renamed away from newly-reserved builtin `xai`), the migration can select an empty normalized secret slot as the source secret and still delete the distinct raw secret slot. This can drop the only non-empty API key.

### Issue Context
- `providerSecrets` can legitimately contain empty-string values while still being an own-property (e.g. user clears a key).
- The migration logic currently uses `Object.hasOwn(...)` to decide whether a secret exists, without checking whether the value is non-empty.

### Fix Focus Areas
- src/config/index.mjs[1249-1318]

### What to change
1. Introduce value-aware checks, e.g.:
  - `const rawSecretValue = hasRawIdSecret ? normalizeText(rawIdSecret) : ''`
  - `const normalizedSecretValue = hasNormalizedIdSecret ? normalizeText(normalizedIdSecret) : ''`
2. In the `isPendingBuiltinProviderIdMigration && hasNormalizedIdSecret && !normalizedSecretBelongsToBuiltin` branch, only treat the normalized slot as the source if `normalizedSecretValue` is non-empty **or** if the raw slot is empty/absent.
3. Only add `oldRawProviderId` to `rawProviderSecretIdsToDelete` when it is safe:
  - either the raw value is empty, or
  - the migrated/captured `oldSecret` came from the raw slot (or you copied the raw value to the new providerId).
4. Add a unit test case covering:
  - providerSecrets contains `{ XAI: 'real', xai: '' }`
  - customOpenAIProviders contains `{ id: 'XAI', ... }`
  - after migration, renamed provider gets `'real'` and no secret is lost.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Provider reference check too strict 🐞 Bug ≡ Correctness
Description
isProviderReferencedByApiModes() only checks exact trimmed string equality for
apiMode.providerId, so a mode referencing a provider via a canonically equivalent id (e.g.
Proxy_V1 vs proxy-v1) or via recorded legacy ids can be missed and the UI can allow deletion.
This can clear that provider’s secret and remove the provider even though it’s still in use, leaving
the mode orphaned.
Code

src/popup/sections/api-modes-provider-utils.mjs[R28-35]

+export function areProviderIdsEquivalent(firstProviderId, secondProviderId) {
+  const normalizedFirstProviderId = normalizeProviderId(firstProviderId)
+  const normalizedSecondProviderId = normalizeProviderId(secondProviderId)
+  if (!normalizedFirstProviderId || !normalizedSecondProviderId) {
+    return normalizeText(firstProviderId) === normalizeText(secondProviderId)
+  }
+  return normalizedFirstProviderId === normalizedSecondProviderId
+}
Evidence
The PR introduces canonical provider-id equivalence (areProviderIdsEquivalent) but the provider
deletion guard still uses strict equality, even though deletion queues secret clearing/removal. This
mismatch can allow deletion of a provider that is still referenced in a migrated/imported config
state where the mode’s providerId representation differs from the provider’s current id (or only
matches via legacy ids).

src/popup/sections/api-modes-provider-utils.mjs[28-35]
src/popup/sections/api-modes-provider-utils.mjs[435-442]
src/popup/sections/ApiModes.jsx[215-218]
src/popup/sections/ApiModes.jsx[356-380]
src/popup/sections/api-modes-provider-utils.mjs[252-263]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`isProviderReferencedByApiModes()` is used to block deleting a provider that is still referenced by custom API modes, but it only compares `normalizeText(apiMode.providerId) === normalizeText(providerId)`. With this PR, provider IDs can be canonically equivalent (e.g. underscores/case) and API modes/providers can carry `legacyProviderIds`, so the current check can miss real references and allow deletion.

### Issue Context
Provider deletion in `ApiModes.jsx` is gated by `isProviderReferencedByApiModes(...)`, and deletion clears secrets via `applyDeletedProviderSecrets(...)`. This PR adds `areProviderIdsEquivalent()` and introduces/uses `legacyProviderIds` in several places, but the deletion reference check wasn’t updated accordingly.

### Fix Focus Areas
- src/popup/sections/api-modes-provider-utils.mjs[435-442]
- src/popup/sections/api-modes-provider-utils.mjs[28-35]

### Implementation sketch
- Update `isProviderReferencedByApiModes(providerId, apiModes)` to:
 - Compare provider IDs using `areProviderIdsEquivalent(apiMode.providerId, providerId)`.
 - Also treat `apiMode.legacyProviderIds` (if present) as referencing the provider when any legacy id is equivalent to `providerId`.
- Add/extend a unit test demonstrating that deletion is blocked when a mode has `providerId: 'Proxy_V1'` and the provider being deleted has id `proxy-v1` (and/or when the mode only references it through `legacyProviderIds`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Diagnostics ignore legacy IDs ✓ Resolved 🐞 Bug ◔ Observability
Description
Provider resolution now considers legacyProviderIds when determining that a matching provider is
disabled, but getOpenAICompatibleRequestDiagnostic still only checks the provider's current id. When
resolution fails for a legacy-id session due to a disabled provider, the logged diagnostic and
thrown error hint can be misleading (generic instead of "provider exists but is disabled").
Code

src/services/apis/provider-registry.mjs[R763-770]

      const hasDisabledCustomProviderMatch = Array.isArray(config?.customOpenAIProviders)
        ? config.customOpenAIProviders.some(
            (item) =>
-              normalizeProviderId(item?.id) === normalizedProviderId && item?.enabled === false,
+              item?.enabled === false &&
+              (normalizeProviderId(item?.id) === normalizedProviderId ||
+                (Array.isArray(item?.legacyProviderIds) &&
+                  item.legacyProviderIds.map(normalizeProviderId).includes(normalizedProviderId))),
          )
Evidence
The resolver’s disabled-provider check now treats legacy IDs as matches, but the diagnostic used for
the thrown error hint does not, so the hint can be wrong for sessions referencing a legacy provider
ID.

src/services/apis/provider-registry.mjs[761-770]
src/services/apis/provider-registry.mjs[592-615]
src/services/apis/openai-api.mjs[52-64]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`resolveOpenAICompatibleRequest()` now treats `legacyProviderIds` as equivalent IDs for matching/disabled detection, but `getOpenAICompatibleRequestDiagnostic()` does not. This can produce incorrect hints in `buildOpenAICompatibleResolutionErrorMessage()` when resolution fails for sessions that refer to a provider via a legacy ID.

### Issue Context
- The resolver explicitly checks `legacyProviderIds` when deciding whether a disabled custom provider blocks recovery.
- The diagnostic booleans (`hasDisabledMatchingCustomProvider`, `hasMatchingCustomProvider`) are used to decide the user-facing hint when resolution fails.

### Fix Focus Areas
- src/services/apis/provider-registry.mjs[592-624]
- src/services/apis/openai-api.mjs[52-64]

### Suggested fix
Update `getOpenAICompatibleRequestDiagnostic()` to treat a provider as matching/disabled if either:
- `normalizeProviderId(item.id) === normalizedProviderId`, OR
- `item.legacyProviderIds` contains `normalizedProviderId` (after normalizing entries)

This should be applied to both `hasMatchingCustomProvider` and `hasDisabledMatchingCustomProvider` so the hint selection remains accurate for migrated/renamed providers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Session migration write race ✓ Resolved 🐞 Bug ☼ Reliability
Description
getUserConfig() now performs a read-modify-write of the entire sessions array to remap custom
provider IDs; if another flow also rewrites sessions from a stale snapshot around the same time,
the last writer wins and can drop session updates. This risk is increased because getSessions()
and session CRUD operations also rewrite the full sessions array independently.
Code

src/config/index.mjs[R1828-1837]

+  const { migrated, dirty, sessionProviderIdRenameLookup } = migrateUserConfig(options)
+  let sessionMigration = { sessions: undefined, dirty: false }
+  if (sessionProviderIdRenameLookup.size > 0) {
+    const { sessions } = await Browser.storage.local.get('sessions')
+    sessionMigration = remapSessionProviderIds(sessions, sessionProviderIdRenameLookup)
+  }
+  if (dirty || sessionMigration.dirty) {
    const payload = {}
    if (JSON.stringify(options.customApiModes) !== JSON.stringify(migrated.customApiModes)) {
      payload.customApiModes = migrated.customApiModes
Evidence
The PR adds a new non-atomic sessions migration inside getUserConfig() (read sessions → transform
→ write sessions). The repo already has other independent read/overwrite writers for sessions, so
concurrent execution can cause lost updates due to last-write-wins behavior.

src/config/index.mjs[1828-1865]
src/services/local-session.mjs[17-33]
src/services/local-session.mjs[71-77]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`getUserConfig()` now reads `sessions`, remaps provider IDs, and writes the full `sessions` array back. Other code paths (session canonicalization and CRUD) also do full-array writes, so overlapping operations can overwrite each other and lose updates.

### Issue Context
- `getUserConfig()` now conditionally writes `payload.sessions`.
- `getSessions()` in `src/services/local-session.mjs` also migrates and writes `sessions`.
- Session CRUD functions overwrite the full `sessions` array.

### Fix Focus Areas
- src/config/index.mjs[1828-1865]
- src/services/local-session.mjs[17-33]
- src/services/local-session.mjs[71-77]

### Suggested fix approach
Implement one of the following (in descending preference):
1) **Centralize session migrations in the session layer**: remove `payload.sessions` writes from `getUserConfig()` and instead apply providerId remapping inside `getSessions()` (or a dedicated session-migration function) so there is a single canonical migration writer for `sessions`.
2) **Add a shared sessions write mutex/queue**: ensure all writers (config migration + CRUD + session canonicalization) serialize their `storage.local.set({sessions})` operations.
3) **Rebase before commit** (lightweight mitigation): in `getUserConfig()`, re-read latest `sessions` immediately before writing and re-apply `remapSessionProviderIds()` to that latest value; only then write if changed. Optionally loop once if `sessions` changed between reads.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

5. Duplicated provider ID logic ✗ Dismissed 🐞 Bug ⚙ Maintainability
Description
Provider ID normalization/equivalence is implemented separately in model-name-convert.mjs and
api-modes-provider-utils.mjs. This duplication increases the risk of future drift where
selection/matching behavior differs between popup UI and session selection logic.
Code

src/utils/model-name-convert.mjs[R3-24]

+function normalizeProviderId(value) {
+  return String(value || '')
+    .trim()
+    .toLowerCase()
+    .replace(/[^a-z0-9]+/g, '-')
+    .replace(/^-+|-+$/g, '')
+}
+
+function areProviderIdsEquivalent(firstProviderId, secondProviderId) {
+  const normalizedFirstProviderId = normalizeProviderId(firstProviderId)
+  const normalizedSecondProviderId = normalizeProviderId(secondProviderId)
+  if (!normalizedFirstProviderId || !normalizedSecondProviderId) {
+    return String(firstProviderId || '').trim() === String(secondProviderId || '').trim()
+  }
+  return normalizedFirstProviderId === normalizedSecondProviderId
+}
+
+function normalizeProviderEndpointUrl(value) {
+  return String(value || '')
+    .trim()
+    .replace(/\/+$/, '')
+}
Evidence
Both files contain separate implementations of provider-id canonicalization/equivalence logic, which
is a known drift risk as the matching rules evolve.

src/utils/model-name-convert.mjs[1-25]
src/popup/sections/api-modes-provider-utils.mjs[17-39]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`normalizeProviderId()` / `areProviderIdsEquivalent()` are duplicated across modules. While currently similar, future tweaks to one copy can silently desync provider matching behavior across the popup and session selection.

### Issue Context
Two independent implementations now exist:
- `src/utils/model-name-convert.mjs` (used for apiMode selection/session compat)
- `src/popup/sections/api-modes-provider-utils.mjs` (used for popup provider selection/editing)

### Fix Focus Areas
- src/utils/model-name-convert.mjs[1-25]
- src/popup/sections/api-modes-provider-utils.mjs[17-39]

### Suggested fix
Extract `normalizeProviderId()` and `areProviderIdsEquivalent()` into a shared utility module (e.g., `src/utils/provider-id.mjs`) and import it from both places. This keeps provider matching semantics as a single source of truth.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 2f937f2

Results up to commit b71c520


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Session migration write race ✓ Resolved 🐞 Bug ☼ Reliability
Description
getUserConfig() now performs a read-modify-write of the entire sessions array to remap custom
provider IDs; if another flow also rewrites sessions from a stale snapshot around the same time,
the last writer wins and can drop session updates. This risk is increased because getSessions()
and session CRUD operations also rewrite the full sessions array independently.
Code

src/config/index.mjs[R1828-1837]

+  const { migrated, dirty, sessionProviderIdRenameLookup } = migrateUserConfig(options)
+  let sessionMigration = { sessions: undefined, dirty: false }
+  if (sessionProviderIdRenameLookup.size > 0) {
+    const { sessions } = await Browser.storage.local.get('sessions')
+    sessionMigration = remapSessionProviderIds(sessions, sessionProviderIdRenameLookup)
+  }
+  if (dirty || sessionMigration.dirty) {
    const payload = {}
    if (JSON.stringify(options.customApiModes) !== JSON.stringify(migrated.customApiModes)) {
      payload.customApiModes = migrated.customApiModes
Evidence
The PR adds a new non-atomic sessions migration inside getUserConfig() (read sessions → transform
→ write sessions). The repo already has other independent read/overwrite writers for sessions, so
concurrent execution can cause lost updates due to last-write-wins behavior.

src/config/index.mjs[1828-1865]
src/services/local-session.mjs[17-33]
src/services/local-session.mjs[71-77]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`getUserConfig()` now reads `sessions`, remaps provider IDs, and writes the full `sessions` array back. Other code paths (session canonicalization and CRUD) also do full-array writes, so overlapping operations can overwrite each other and lose updates.

### Issue Context
- `getUserConfig()` now conditionally writes `payload.sessions`.
- `getSessions()` in `src/services/local-session.mjs` also migrates and writes `sessions`.
- Session CRUD functions overwrite the full `sessions` array.

### Fix Focus Areas
- src/config/index.mjs[1828-1865]
- src/services/local-session.mjs[17-33]
- src/services/local-session.mjs[71-77]

### Suggested fix approach
Implement one of the following (in descending preference):
1) **Centralize session migrations in the session layer**: remove `payload.sessions` writes from `getUserConfig()` and instead apply providerId remapping inside `getSessions()` (or a dedicated session-migration function) so there is a single canonical migration writer for `sessions`.
2) **Add a shared sessions write mutex/queue**: ensure all writers (config migration + CRUD + session canonicalization) serialize their `storage.local.set({sessions})` operations.
3) **Rebase before commit** (lightweight mitigation): in `getUserConfig()`, re-read latest `sessions` immediately before writing and re-apply `remapSessionProviderIds()` to that latest value; only then write if changed. Optionally loop once if `sessions` changed between reads.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 58a2730


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. Secret dropped on rename ✓ Resolved 🐞 Bug ≡ Correctness
Description
In migrateUserConfig(), a colliding custom provider can migrate an empty normalized providerSecrets
entry (e.g. providerSecrets['xai']==='') while still deleting the non-empty raw-key entry (e.g.
providerSecrets['XAI']==='real-key'), permanently losing the secret for the renamed provider. This
happens because the collision logic treats “own property exists” as “secret exists”, even when its
value is empty, and schedules the raw slot for deletion regardless of which slot was migrated.
Code

src/config/index.mjs[R1249-1318]

+    const hasRawIdSecret = Object.hasOwn(providerSecretSnapshot, oldRawProviderId)
+    const hasNormalizedIdSecret = Object.hasOwn(providerSecretSnapshot, oldProviderId)
+    const hasDistinctRawId = oldRawProviderId !== oldProviderId
    const usesBuiltinSecretSlot = builtinProviderIds.has(oldProviderId)
-    if (usesBuiltinSecretSlot && !hasRawIdSecret) continue
-    if (!usesBuiltinSecretSlot && !hasRawIdSecret && !hasNormalizedIdSecret) continue
-    const rawIdSecret = hasRawIdSecret ? providerSecrets[oldRawProviderId] : undefined
-    const normalizedIdSecret = hasNormalizedIdSecret ? providerSecrets[oldProviderId] : undefined
-    const oldSecret = usesBuiltinSecretSlot
-      ? rawIdSecret
-      : hasRawIdSecret && rawIdSecret !== ''
-      ? rawIdSecret
-      : hasNormalizedIdSecret
-      ? normalizedIdSecret
-      : rawIdSecret
+    const isPendingBuiltinProviderIdMigration =
+      usesBuiltinSecretSlot && pendingBuiltinProviderIdMigrations.has(oldProviderId)
+    const hasMultipleProviderIdRenames = (providerIdRenameCounts.get(oldProviderId) || 0) > 1
+    const rawIdSecret = hasRawIdSecret ? providerSecretSnapshot[oldRawProviderId] : undefined
+    const normalizedIdSecret = hasNormalizedIdSecret
+      ? providerSecretSnapshot[oldProviderId]
+      : undefined
+    const builtinLegacyKey = LEGACY_API_KEY_FIELD_BY_PROVIDER_ID[oldProviderId]
+    const builtinLegacySecret = builtinLegacyKey ? normalizeText(migrated[builtinLegacyKey]) : ''
+    const normalizedSecretBelongsToBuiltin =
+      isPendingBuiltinProviderIdMigration &&
+      builtinLegacySecret &&
+      builtinLegacySecret === normalizeText(normalizedIdSecret)
+    if (hasDistinctRawId && hasRawIdSecret) {
+      rawProviderSecretIdsToDelete.add(oldRawProviderId)
+    }
+    let hasOldSecret = false
+    let oldSecret
+    let usesNormalizedBuiltinSecretSlot = false
+    if (
+      hasDistinctRawId &&
+      hasRawIdSecret &&
+      (hasMultipleProviderIdRenames ||
+        !isPendingBuiltinProviderIdMigration ||
+        !hasNormalizedIdSecret ||
+        normalizedSecretBelongsToBuiltin)
+    ) {
+      hasOldSecret = true
+      oldSecret =
+        !usesBuiltinSecretSlot && rawIdSecret === '' && hasNormalizedIdSecret
+          ? normalizedIdSecret
+          : rawIdSecret
+    } else if (
+      isPendingBuiltinProviderIdMigration &&
+      hasNormalizedIdSecret &&
+      !normalizedSecretBelongsToBuiltin
+    ) {
+      hasOldSecret = true
+      oldSecret = normalizedIdSecret
+      usesNormalizedBuiltinSecretSlot = true
+    } else if (!usesBuiltinSecretSlot && (hasRawIdSecret || hasNormalizedIdSecret)) {
+      hasOldSecret = true
+      oldSecret =
+        hasRawIdSecret && rawIdSecret !== ''
+          ? rawIdSecret
+          : hasNormalizedIdSecret
+          ? normalizedIdSecret
+          : rawIdSecret
+    }
    if (
-      !Object.hasOwn(providerSecrets, newProviderId) ||
-      providerSecrets[newProviderId] !== oldSecret
+      hasOldSecret &&
+      (!Object.hasOwn(providerSecrets, newProviderId) ||
+        providerSecrets[newProviderId] !== oldSecret)
    ) {
      providerSecrets[newProviderId] = oldSecret
      dirty = true
    }
-    if (hasRawIdSecret && oldRawProviderId !== oldProviderId) {
-      delete providerSecrets[oldRawProviderId]
+    if (usesNormalizedBuiltinSecretSlot) {
+      builtinProviderSecretIdsToDelete.set(oldProviderId, normalizedIdSecret)
+    }
+  }
+  for (const providerId of rawProviderSecretIdsToDelete) {
+    if (Object.hasOwn(providerSecrets, providerId)) {
+      delete providerSecrets[providerId]
+      dirty = true
+    }
Evidence
The migration code schedules deletion of the raw secret slot whenever the raw provider id differs
and exists, but the pending-builtin collision branch can choose normalizedIdSecret as oldSecret
even when that value is empty (because hasNormalizedIdSecret is based on property existence, not
value). Separately, the UI/config utilities can store empty-string providerSecrets values while
keeping the key as an own-property, making this state reachable.

src/config/index.mjs[1240-1318]
src/popup/sections/provider-secret-utils.mjs[241-256]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
During custom provider ID collision migration (e.g. custom provider `XAI` renamed away from newly-reserved builtin `xai`), the migration can select an empty normalized secret slot as the source secret and still delete the distinct raw secret slot. This can drop the only non-empty API key.

### Issue Context
- `providerSecrets` can legitimately contain empty-string values while still being an own-property (e.g. user clears a key).
- The migration logic currently uses `Object.hasOwn(...)` to decide whether a secret exists, without checking whether the value is non-empty.

### Fix Focus Areas
- src/config/index.mjs[1249-1318]

### What to change
1. Introduce value-aware checks, e.g.:
  - `const rawSecretValue = hasRawIdSecret ? normalizeText(rawIdSecret) : ''`
  - `const normalizedSecretValue = hasNormalizedIdSecret ? normalizeText(normalizedIdSecret) : ''`
2. In the `isPendingBuiltinProviderIdMigration && hasNormalizedIdSecret && !normalizedSecretBelongsToBuiltin` branch, only treat the normalized slot as the source if `normalizedSecretValue` is non-empty **or** if the raw slot is empty/absent.
3. Only add `oldRawProviderId` to `rawProviderSecretIdsToDelete` when it is safe:
  - either the raw value is empty, or
  - the migrated/captured `oldSecret` came from the raw slot (or you copied the raw value to the new providerId).
4. Add a unit test case covering:
  - providerSecrets contains `{ XAI: 'real', xai: '' }`
  - customOpenAIProviders contains `{ id: 'XAI', ... }`
  - after migration, renamed provider gets `'real'` and no secret is lost.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit ad570ab


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Diagnostics ignore legacy IDs ✓ Resolved 🐞 Bug ◔ Observability
Description
Provider resolution now considers legacyProviderIds when determining that a matching provider is
disabled, but getOpenAICompatibleRequestDiagnostic still only checks the provider's current id. When
resolution fails for a legacy-id session due to a disabled provider, the logged diagnostic and
thrown error hint can be misleading (generic instead of "provider exists but is disabled").
Code

src/services/apis/provider-registry.mjs[R763-770]

      const hasDisabledCustomProviderMatch = Array.isArray(config?.customOpenAIProviders)
        ? config.customOpenAIProviders.some(
            (item) =>
-              normalizeProviderId(item?.id) === normalizedProviderId && item?.enabled === false,
+              item?.enabled === false &&
+              (normalizeProviderId(item?.id) === normalizedProviderId ||
+                (Array.isArray(item?.legacyProviderIds) &&
+                  item.legacyProviderIds.map(normalizeProviderId).includes(normalizedProviderId))),
          )
Evidence
The resolver’s disabled-provider check now treats legacy IDs as matches, but the diagnostic used for
the thrown error hint does not, so the hint can be wrong for sessions referencing a legacy provider
ID.

src/services/apis/provider-registry.mjs[761-770]
src/services/apis/provider-registry.mjs[592-615]
src/services/apis/openai-api.mjs[52-64]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`resolveOpenAICompatibleRequest()` now treats `legacyProviderIds` as equivalent IDs for matching/disabled detection, but `getOpenAICompatibleRequestDiagnostic()` does not. This can produce incorrect hints in `buildOpenAICompatibleResolutionErrorMessage()` when resolution fails for sessions that refer to a provider via a legacy ID.

### Issue Context
- The resolver explicitly checks `legacyProviderIds` when deciding whether a disabled custom provider blocks recovery.
- The diagnostic booleans (`hasDisabledMatchingCustomProvider`, `hasMatchingCustomProvider`) are used to decide the user-facing hint when resolution fails.

### Fix Focus Areas
- src/services/apis/provider-registry.mjs[592-624]
- src/services/apis/openai-api.mjs[52-64]

### Suggested fix
Update `getOpenAICompatibleRequestDiagnostic()` to treat a provider as matching/disabled if either:
- `normalizeProviderId(item.id) === normalizedProviderId`, OR
- `item.legacyProviderIds` contains `normalizedProviderId` (after normalizing entries)

This should be applied to both `hasMatchingCustomProvider` and `hasDisabledMatchingCustomProvider` so the hint selection remains accurate for migrated/renamed providers.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational
2. Duplicated provider ID logic ✗ Dismissed 🐞 Bug ⚙ Maintainability
Description
Provider ID normalization/equivalence is implemented separately in model-name-convert.mjs and
api-modes-provider-utils.mjs. This duplication increases the risk of future drift where
selection/matching behavior differs between popup UI and session selection logic.
Code

src/utils/model-name-convert.mjs[R3-24]

+function normalizeProviderId(value) {
+  return String(value || '')
+    .trim()
+    .toLowerCase()
+    .replace(/[^a-z0-9]+/g, '-')
+    .replace(/^-+|-+$/g, '')
+}
+
+function areProviderIdsEquivalent(firstProviderId, secondProviderId) {
+  const normalizedFirstProviderId = normalizeProviderId(firstProviderId)
+  const normalizedSecondProviderId = normalizeProviderId(secondProviderId)
+  if (!normalizedFirstProviderId || !normalizedSecondProviderId) {
+    return String(firstProviderId || '').trim() === String(secondProviderId || '').trim()
+  }
+  return normalizedFirstProviderId === normalizedSecondProviderId
+}
+
+function normalizeProviderEndpointUrl(value) {
+  return String(value || '')
+    .trim()
+    .replace(/\/+$/, '')
+}
Evidence
Both files contain separate implementations of provider-id canonicalization/equivalence logic, which
is a known drift risk as the matching rules evolve.

src/utils/model-name-convert.mjs[1-25]
src/popup/sections/api-modes-provider-utils.mjs[17-39]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`normalizeProviderId()` / `areProviderIdsEquivalent()` are duplicated across modules. While currently similar, future tweaks to one copy can silently desync provider matching behavior across the popup and session selection.

### Issue Context
Two independent implementations now exist:
- `src/utils/model-name-convert.mjs` (used for apiMode selection/session compat)
- `src/popup/sections/api-modes-provider-utils.mjs` (used for popup provider selection/editing)

### Fix Focus Areas
- src/utils/model-name-convert.mjs[1-25]
- src/popup/sections/api-modes-provider-utils.mjs[17-39]

### Suggested fix
Extract `normalizeProviderId()` and `areProviderIdsEquivalent()` into a shared utility module (e.g., `src/utils/provider-id.mjs`) and import it from both places. This keeps provider matching semantics as a single source of truth.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 90d783a


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Provider reference check too strict 🐞 Bug ≡ Correctness
Description
isProviderReferencedByApiModes() only checks exact trimmed string equality for
apiMode.providerId, so a mode referencing a provider via a canonically equivalent id (e.g.
Proxy_V1 vs proxy-v1) or via recorded legacy ids can be missed and the UI can allow deletion.
This can clear that provider’s secret and remove the provider even though it’s still in use, leaving
the mode orphaned.
Code

src/popup/sections/api-modes-provider-utils.mjs[R28-35]

+export function areProviderIdsEquivalent(firstProviderId, secondProviderId) {
+  const normalizedFirstProviderId = normalizeProviderId(firstProviderId)
+  const normalizedSecondProviderId = normalizeProviderId(secondProviderId)
+  if (!normalizedFirstProviderId || !normalizedSecondProviderId) {
+    return normalizeText(firstProviderId) === normalizeText(secondProviderId)
+  }
+  return normalizedFirstProviderId === normalizedSecondProviderId
+}
Evidence
The PR introduces canonical provider-id equivalence (areProviderIdsEquivalent) but the provider
deletion guard still uses strict equality, even though deletion queues secret clearing/removal. This
mismatch can allow deletion of a provider that is still referenced in a migrated/imported config
state where the mode’s providerId representation differs from the provider’s current id (or only
matches via legacy ids).

src/popup/sections/api-modes-provider-utils.mjs[28-35]
src/popup/sections/api-modes-provider-utils.mjs[435-442]
src/popup/sections/ApiModes.jsx[215-218]
src/popup/sections/ApiModes.jsx[356-380]
src/popup/sections/api-modes-provider-utils.mjs[252-263]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`isProviderReferencedByApiModes()` is used to block deleting a provider that is still referenced by custom API modes, but it only compares `normalizeText(apiMode.providerId) === normalizeText(providerId)`. With this PR, provider IDs can be canonically equivalent (e.g. underscores/case) and API modes/providers can carry `legacyProviderIds`, so the current check can miss real references and allow deletion.

### Issue Context
Provider deletion in `ApiModes.jsx` is gated by `isProviderReferencedByApiModes(...)`, and deletion clears secrets via `applyDeletedProviderSecrets(...)`. This PR adds `areProviderIdsEquivalent()` and introduces/uses `legacyProviderIds` in several places, but the deletion reference check wasn’t updated accordingly.

### Fix Focus Areas
- src/popup/sections/api-modes-provider-utils.mjs[435-442]
- src/popup/sections/api-modes-provider-utils.mjs[28-35]

### Implementation sketch
- Update `isProviderReferencedByApiModes(providerId, apiModes)` to:
 - Compare provider IDs using `areProviderIdsEquivalent(apiMode.providerId, providerId)`.
 - Also treat `apiMode.legacyProviderIds` (if present) as referencing the provider when any legacy id is equivalent to `providerId`.
- Add/extend a unit test demonstrating that deletion is blocked when a mode has `providerId: 'Proxy_V1'` and the provider being deleted has id `proxy-v1` (and/or when the mode only references it through `legacyProviderIds`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds xAI as a built-in OpenAI-compatible API provider so the extension can route chat-completions requests to xAI using the existing OpenAI-compatible infrastructure and store secrets consistently (legacy xaiApiKey + providerSecrets).

Changes:

  • Registered a new built-in provider xai with base URL https://api.x.ai/v1 and /chat/completions path, including legacy-model-name and apiMode-group resolution.
  • Added xaiApiModelKeys model group plus Grok 4.5 / 4.3 model presets, and integrated xAI into OpenAI-compatible session detection.
  • Wired provider-to-secret-field mapping (xai -> xaiApiKey) and added a provider API-key setup link in the popup.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/unit/services/apis/thin-adapters.test.mjs Adds coverage ensuring thin adapter resolution includes xAI with expected base URL and key retrieval.
tests/unit/services/apis/provider-registry.test.mjs Adds focused tests for provider registry inclusion, provider-id resolution, and request resolution for xAI (legacy + providerSecrets).
tests/unit/config/config-predicates.test.mjs Adds predicate coverage to ensure xAI API models are detected correctly.
src/services/apis/provider-registry.mjs Registers built-in xai provider and maps legacy model/apiMode identifiers to provider id xai.
src/popup/sections/GeneralPart.jsx Adds xAI API-key setup URL routing in the popup.
src/config/openai-provider-mappings.mjs Adds provider-id mappings for legacy key field (xaiApiKey) and apiMode group (xaiApiModelKeys).
src/config/index.mjs Introduces xaiApiModelKeys, model group entry, Grok presets, default config key field, and isUsingXaiApiModel.
src/background/index.mjs Includes xAI in OpenAI-compatible API session detection for request routing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
putComment timed out

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 19cb00b

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown
Member Author

/agentic_review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Comment thread src/popup/sections/GeneralPart.jsx
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit fa8e341

Copy link
Copy Markdown
Member Author

/agentic_review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b71c5206a3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/config/index.mjs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/config/index.mjs`:
- Around line 1193-1197: Update the persisted-session remapping flow around
sessionProviderIdRenameLookup so duplicate provider IDs are not removed from the
rename candidates. Preserve the mappings and add session disambiguation based on
each custom session’s URL, matching the existing custom API mode handling, so
legacy sessions resolve to the correct renamed custom provider rather than the
builtin provider.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 01f1a6e8-fd8f-4bfa-9766-ae3514ea27b4

📥 Commits

Reviewing files that changed from the base of the PR and between fa8e341 and b71c520.

📒 Files selected for processing (11)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/GeneralPart.jsx
  • src/popup/sections/import-data-cleanup.mjs
  • src/services/apis/provider-registry.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/config/migrate-xai-provider.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs
🚧 Files skipped from review as they are similar to previous changes (5)
  • tests/unit/services/apis/thin-adapters.test.mjs
  • src/popup/sections/GeneralPart.jsx
  • src/background/index.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs

Comment thread src/config/index.mjs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit ad570ab

Copy link
Copy Markdown
Member Author

/agentic_review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/services/apis/provider-registry.mjs:738

  • matchedConfiguredProviderId is already canonicalized via normalizeProviderId(...), but the lookup compares it against item.id verbatim. If a stored provider ID isn’t already in canonical form (e.g. imported / legacy configs), this label-based recovery path will fail to find the intended provider even when it exists. Compare using canonicalized IDs (or reuse getProviderById).
      const matchedConfiguredProviderId = normalizeProviderId(matchedConfiguredApiMode?.providerId)
      if (matchedConfiguredProviderId) {
        const matchedConfiguredProvider = customProviders.find(
          (item) => item.enabled !== false && item.id === matchedConfiguredProviderId,
        )

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit a109bd9

Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 73328bb

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Comment thread src/utils/model-name-convert.mjs Outdated
Comment thread src/services/apis/provider-registry.mjs

Copy link
Copy Markdown
Member Author

/agentic_review

Comment thread src/popup/sections/api-modes-provider-utils.mjs Outdated
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 90d783a

Copy link
Copy Markdown
Member Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit d5f235e

Register xAI as a built-in OpenAI-compatible chat provider and offer current Grok 4.5 and Grok 4.3 presets.

Wire shared API-key storage, request routing, setup links, and focused coverage so the provider behaves like existing built-in adapters.

Copy link
Copy Markdown
Member Author

/agentic_review

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@tests/unit/services/apis/provider-registry.test.mjs`:
- Around line 2581-2585: Add an assertion in resolveProviderIdForSession’s
legacy xAI model-name test for modelName 'xaiGrok4_3', and verify it resolves to
'xai' alongside the existing Grok 4.5 coverage.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6c524d55-0a97-4c2e-a576-5822f5bf316d

📥 Commits

Reviewing files that changed from the base of the PR and between b71c520 and 2f937f2.

📒 Files selected for processing (8)
  • src/background/index.mjs
  • src/config/index.mjs
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/GeneralPart.jsx
  • src/services/apis/provider-registry.mjs
  • tests/unit/config/config-predicates.test.mjs
  • tests/unit/services/apis/provider-registry.test.mjs
  • tests/unit/services/apis/thin-adapters.test.mjs
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/config/openai-provider-mappings.mjs
  • src/popup/sections/GeneralPart.jsx
  • tests/unit/services/apis/thin-adapters.test.mjs
  • src/background/index.mjs
  • src/services/apis/provider-registry.mjs

Comment thread tests/unit/services/apis/provider-registry.test.mjs
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 2f937f2

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants