Skip to content

Preserve future API mode defaults after customization#1009

Open
PeterDaveHello wants to merge 1 commit into
ChatGPTBox-dev:masterfrom
PeterDaveHello:preserve-future-api-mode-defaults
Open

Preserve future API mode defaults after customization#1009
PeterDaveHello wants to merge 1 commit into
ChatGPTBox-dev:masterfrom
PeterDaveHello:preserve-future-api-mode-defaults

Conversation

@PeterDaveHello

@PeterDaveHello PeterDaveHello commented Jul 16, 2026

Copy link
Copy Markdown
Member

Summary

  • Track the API mode default baseline for customized profiles so future defaults can be appended without restoring defaults that users removed.
  • Preserve compatibility for legacy configuration migration and imported backups, including deterministic cleanup of stale API mode state.
  • Persist API mode lists, selection state, and baseline metadata atomically, with rollback-safe handling when persistence fails.
  • Preserve built-in Azure OpenAI and Ollama rows while materializing and reconciling defaults.

Validation

  • npm run lint
  • npm test (714 tests)
  • npm run build

Rollout

This is phase 1 of the future-default rollout. The API mode reorder UI is intentionally deferred until this PR is accepted.

Summary by CodeRabbit

  • New Features
    • Added materialized API mode default handling, including deterministic default IDs and schema-v2 initialization.
    • Introduced selection-aware API mode config expansion/update helpers for consistent active/custom list + selection behavior.
  • Bug Fixes
    • Hardened schema-v2 migration/import cleanup to sanitize malformed API-mode fields and prevent stale defaults from returning.
    • Improved reconciliation to avoid duplicates while preserving custom/provider-specific variants.
  • Tests
    • Expanded unit tests for config migration behavior, API mode update/rollback, import cleanup, and reconciliation edge cases.

@coderabbitai

coderabbitai Bot commented Jul 16, 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

Schema version 2 introduces materialized API mode defaults, migration reconciliation, atomic popup updates, and import normalization for API mode fields and known default identifiers.

Changes

API mode defaults and migration

Layer / File(s) Summary
Default identifiers and reconciliation
src/config/index.mjs, src/utils/model-name-convert.mjs, tests/unit/utils/model-name-convert.test.mjs
Default API mode IDs are centralized, default rows are materialized and de-duplicated, and built-in preset rows are normalized and tested.
Schema v2 migration and storage cleanup
src/config/index.mjs, tests/unit/config/migrate-user-config.test.mjs
Migration distinguishes live defaults from customized profiles, reconciles known defaults, conditionally persists fields, and removes obsolete keys after successful writes.
Atomic popup API mode updates
src/popup/api-mode-config-utils.mjs, src/popup/Popup.jsx, src/popup/sections/ApiModes.jsx, tests/unit/popup/api-mode-config-utils.test.mjs
API mode edits, selection changes, toggles, and deletions use shared atomic update payloads with optimistic concurrency and rollback tests.
Import normalization
src/popup/sections/import-data-cleanup.mjs, tests/unit/popup/import-data-cleanup.test.mjs
Imported API mode fields are cleared or canonicalized together, including knownApiModeDefaultIds, with migration coverage for stale storage state.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

Configuration migration

sequenceDiagram
  participant getUserConfig
  participant migrateUserConfig
  participant reconcileMaterializedApiModeDefaults
  participant BrowserStorage
  getUserConfig->>migrateUserConfig: migrate stored configuration
  migrateUserConfig->>reconcileMaterializedApiModeDefaults: reconcile known defaults
  reconcileMaterializedApiModeDefaults-->>migrateUserConfig: return modes and known IDs
  migrateUserConfig-->>getUserConfig: return migrated config and keys
  getUserConfig->>BrowserStorage: persist migration
  BrowserStorage-->>getUserConfig: confirm successful write
  getUserConfig->>BrowserStorage: remove obsolete keys
Loading

Popup API mode updates

sequenceDiagram
  participant ApiModes
  participant api_mode_config_utils
  participant PopupUpdateConfig
  participant BrowserStorage
  ApiModes->>api_mode_config_utils: build atomic API mode update
  api_mode_config_utils-->>ApiModes: return normalized bundle
  ApiModes->>PopupUpdateConfig: submit expanded patch
  PopupUpdateConfig->>BrowserStorage: persist update
  BrowserStorage-->>PopupUpdateConfig: return success or failure
  PopupUpdateConfig-->>ApiModes: apply update or rollback
Loading

Possibly related PRs

Suggested labels: Review effort 4/5

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.88% 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: preserving future API mode defaults after users customize profiles.
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.
✨ 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.

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Preserve future API mode defaults after profile customization

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Track a default-baseline for customized API mode lists to append future defaults safely.
• Migrate legacy and imported configs deterministically, cleaning stale API mode storage keys.
• Persist API mode list + selection state atomically with rollback-safe handling on write failure.
Diagram

graph TD
  popup["Popup UI"] --> api_utils["api-mode-config-utils"] --> popup_cfg["popup-config-utils"] --> storage[("Browser storage.local")]
  storage --> config["getUserConfig / migrateUserConfig"] --> storage
  config --> convert["model-name-convert (reconcile)"] --> config
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Track removed default IDs instead of a baseline
  • ➕ Smaller stored metadata (only removals).
  • ➕ Directly encodes user intent to hide specific defaults.
  • ➖ Harder to reason about across canonicalization/renames of IDs.
  • ➖ Does not naturally capture 'which defaults existed when customization happened' for append-only semantics.
2. Versioned default list with migration per version bump
  • ➕ Simpler runtime logic; all complexity isolated to migrations.
  • ➕ Easy to test per schema version transitions.
  • ➖ Migration code grows with every new default addition/reorder.
  • ➖ More fragile for imported backups spanning multiple versions.
3. Compute identity-based merge without storing any baseline
  • ➕ No extra persisted field; purely derives from current state.
  • ➕ Avoids long-lived metadata drift.
  • ➖ Cannot distinguish 'user deleted a default' vs 'default never existed yet'.
  • ➖ Risk of re-adding removed defaults when they reappear as candidates.

Recommendation: Keep the PR’s baseline approach (knownApiModeDefaultIds) plus identity-based reconciliation. It cleanly supports the core requirement—append future defaults without restoring removed ones—while remaining robust to legacy snapshots/imports and to ID canonicalization. The additional atomic write bundling in the popup complements this by preventing partial persistence/rollback inconsistencies.

Files changed (10) +859 / -63

Enhancement (5) +275 / -49
index.mjsAdd default API mode baseline and schema v2 migration for materialized lists +144/-28

Add default API mode baseline and schema v2 migration for materialized lists

• Introduces a centralized defaultApiModeIds list, adds knownApiModeDefaultIds to config, and bumps schema to v2. Updates migration to distinguish live-default profiles vs materialized profiles, materialize legacy activeApiModes into customApiModes, reconcile future defaults against a stored baseline, and remove stale storage keys only after successful persistence.

src/config/index.mjs

Popup.jsxExpand API mode config updates into atomic list+selection bundles +3/-1

Expand API mode config updates into atomic list+selection bundles

• Routes popup config writes through expandApiModeListConfigUpdate so API mode list updates always include the full set of related keys. This supports rollback/optimistic update behavior without leaving partial API-mode state in memory or storage.

src/popup/Popup.jsx

api-mode-config-utils.mjsNew helpers for atomic API mode list persistence and baseline updates +38/-0

New helpers for atomic API mode list persistence and baseline updates

• Adds utilities to build a complete API mode update payload (customApiModes + knownApiModeDefaultIds, with activeApiModes cleared) and to expand partial list updates into the full atomic bundle. Ensures the baseline accumulates current defaultApiModeIds while preserving prior baseline entries.

src/popup/api-mode-config-utils.mjs

ApiModes.jsxPersist API mode edits/toggles/deletes with baseline-aware atomic updates +27/-18

Persist API mode edits/toggles/deletes with baseline-aware atomic updates

• Switches list mutations to use buildApiModeListConfigUpdate and adds selectionPatch handling when disabling/removing a currently-selected mode. Ensures UI operations always persist customApiModes, clear activeApiModes, and advance knownApiModeDefaultIds together.

src/popup/sections/ApiModes.jsx

model-name-convert.mjsReconcile materialized API mode defaults using stable identity matching +63/-2

Reconcile materialized API mode defaults using stable identity matching

• Hardens normalizeApiMode for non-string customName, adjusts filtering to keep built-in Azure/Ollama rows even when unnamed, and adds reconcileMaterializedApiModeDefaults. The reconciliation appends unseen defaults in order without reactivating equivalent inactive rows and avoids duplicating provider variants.

src/utils/model-name-convert.mjs

Bug fix (1) +16 / -0
import-data-cleanup.mjsNormalize imported API mode list state with null sentinels and baseline canonicalization +16/-0

Normalize imported API mode list state with null sentinels and baseline canonicalization

• When any API-mode list key is present in an import payload, fills missing sibling keys with null to force atomic replacement of stale state. Also canonicalizes knownApiModeDefaultIds to trimmed, valid model keys to keep baseline deterministic.

src/popup/sections/import-data-cleanup.mjs

Tests (4) +568 / -14
migrate-user-config.test.mjsAdd extensive migration tests for baseline, live-default profiles, and write failures +254/-14

Add extensive migration tests for baseline, live-default profiles, and write failures

• Updates expectations for schema v2 and adds coverage for live-default profiles, imported null sentinels, legacy active-only materialization, baseline validation/repair, default appends, deterministic retry behavior after storage.set failures, and non-restoration of removed defaults.

tests/unit/config/migrate-user-config.test.mjs

api-mode-config-utils.test.mjsNew unit tests for atomic API mode update helpers and rollback ownership +104/-0

New unit tests for atomic API mode update helpers and rollback ownership

• Validates baseline accumulation in buildApiModeListConfigUpdate, ensures expandApiModeListConfigUpdate injects the full bundle, and tests ownership/rollback interactions so newer list writes supersede or fully roll back earlier optimistic updates.

tests/unit/popup/api-mode-config-utils.test.mjs

import-data-cleanup.test.mjsTest atomic import cleanup for API mode list state and baseline normalization +50/-0

Test atomic import cleanup for API mode list state and baseline normalization

• Adds coverage that old backups trigger null sentinels for missing API mode keys, baseline canonicalization drops invalid entries, and import replaces stale stored API-mode state before running legacy migrations.

tests/unit/popup/import-data-cleanup.test.mjs

model-name-convert.test.mjsAdd tests for default reconciliation and built-in Azure/Ollama row retention +160/-0

Add tests for default reconciliation and built-in Azure/Ollama row retention

• Introduces test cases for reconcileMaterializedApiModeDefaults behavior (ordering, idempotence, duplicates, provider variants, inactive preservation, Azure/Ollama materialization) and asserts getApiModesFromConfig keeps nameless built-in Azure/Ollama preset rows.

tests/unit/utils/model-name-convert.test.mjs

@kilo-code-bot

kilo-code-bot Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No New Issues Found (incremental) | Recommendation: Merge

Incremental diff since 15257cc5 touched only src/config/index.mjs (persistence cleanup in getUserConfig, lines ~1741-1800) plus tests. The change replaces the swallowed .catch(() => {}) on Browser.storage.local.set with an explicit try/catch whose behavior is identical: live-default sentinels (activeApiModes/knownApiModeDefaultIds) are removed only after set succeeds, and on remove failure the sentinels remain non-authoritative and are retried on the next read. The accompanying test validates this correctly.

Prior CRITICAL/WARNING findings on lines 266/283/150/1630/1638/1589/45/95/158/27 are outside this incremental diff and were previously marked fixed, resolved, or not-adopted by the author; no new issues are raised here.

Files Reviewed (2 files)
  • src/config/index.mjs
  • tests/unit/config/migrate-user-config.test.mjs
Previous Review Summaries (5 snapshots, latest commit cbfaec7)

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

Previous review (commit cbfaec7)

Status: No New Issues Found (incremental) | Recommendation: Merge

Incremental diff since 15257cc5 touched only src/config/index.mjs (persistence cleanup in getUserConfig, lines ~1741-1800) plus tests. The change replaces the swallowed .catch(() => {}) on Browser.storage.local.set with an explicit try/catch whose behavior is identical: live-default sentinels (activeApiModes/knownApiModeDefaultIds) are removed only after set succeeds, and on remove failure the sentinels remain non-authoritative and are retried on the next read. The accompanying test validates this correctly.

Prior CRITICAL/WARNING findings on lines 266/283/150/1630/1638/1589/45/95/158/27 are outside this incremental diff and were previously marked fixed, resolved, or not-adopted by the author; no new issues are raised here.

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

Previous review (commit 15257cc)

Status: No New Issues Found (incremental) | Recommendation: Merge

Incremental diff since f53bcf1 touched only src/config/index.mjs (error-handling cleanup in getUserConfig, lines 1793-1799) and added a test. The change replaces a swallowed .catch(() => {}) with an explicit try/catch whose behavior is identical (live-default sentinels remain and are retried on the next read on remove failure). The accompanying test validates this correctly.

Prior CRITICAL/WARNING findings on lines 266/150/283/1589/1630/1638 are outside this incremental diff and were previously marked fixed or not-adopted; no new issues are raised here.

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

Previous review (commit f53bcf1)

Status: 6 Issues Found (verified carry-forward) | Recommendation: Address before merge

Incremental review: prior review commit da25801 is not reachable in this clone (current HEAD f53bcf1). Findings were re-verified against current code on f53bcf1. import-data-cleanup.mjs:27 was confirmed resolved (orphaned-selection cleanup now at lines 24-26) and is dropped below. No new issues beyond the carried findings were identified in the changed files.

Overview

Severity Count
CRITICAL 1
WARNING 4
SUGGESTION 1
Issue Details (click to expand)

CRITICAL

File Line Issue
src/utils/model-name-convert.mjs 266 reconcileMaterializedApiModeDefaults special-cases defaultId === 'customModel' by only adding it to knownApiModeDefaultIds and continue-ing without materializing a row. But getApiModesFromConfig (line 220) drops customModel from activeApiModes and a customApiModelKeys row with empty customName is filtered out (lines 147-151, test line 327-352), so the built-in Custom Model option disappears for materialized/customized profiles. Materialize it like any other default instead of skipping it.

WARNING

File Line Issue
src/utils/model-name-convert.mjs 283 getApiModeDefaultIdentity is keyed on itemName; a user-configured built-in Azure/Ollama row (customName set, itemName empty) normalizes itemName to the preset name but isCustom becomes true, while the materialized default has isCustom:false. The dedup check at line 280 then misses the match and appends a duplicate default row. Normalize isCustom consistently for built-in preset groups before stringifying.
src/utils/model-name-convert.mjs 150 The AlwaysCustomGroups filter admits any non-custom row with a truthy itemName; a malformed { groupName: 'customApiModelKeys', itemName: 'customApiModelKeys', isCustom: false } row passes. Narrow the non-custom exception to only the intended built-in Azure/Ollama preset rows (isBuiltInAlwaysCustomPreset).
src/config/index.mjs 1630 The reconcile branch materializes defaults via reconcileMaterializedApiModeDefaults; combined with the customModel special-case (CRITICAL above) this is where the Custom Model default is silently dropped. Ensure the reconcile path produces a complete default set.
src/config/index.mjs 1638 knownApiModeDefaultIds is rebuilt by filtering non-string/empty entries, but only after the rebuild; if migrated.knownApiModeDefaultIds contains a stale customModel entry it is kept while the materialized set lacks the row, leaving the baseline and customApiModes inconsistent.

SUGGESTION

File Line Issue
src/config/index.mjs 1589 currentDefaultIds.length === 0 is redundant because currentDefaultIds derives from the non-empty constant defaultApiModeIds. Simplify to rawKnownApiModeDefaultIds.length > 0.
Resolved / Verified
  • src/popup/sections/import-data-cleanup.mjs:27 — When an API-mode list key is present but a selection key is omitted, the selection key is now removed (lines 24-26), preventing storageArea.set from retaining an orphaned selection. Marked resolved.
Files Reviewed (10 files)
  • src/config/index.mjs
  • src/popup/Popup.jsx
  • src/popup/api-mode-config-utils.mjs
  • src/popup/sections/ApiModes.jsx
  • src/popup/sections/import-data-cleanup.mjs
  • src/utils/model-name-convert.mjs
  • tests/unit/config/migrate-user-config.test.mjs
  • tests/unit/popup/api-mode-config-utils.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/utils/model-name-convert.test.mjs

Fix these issues in Kilo Cloud

Previous review (commit da25801)

Status: 4 Issues Found (verified carry-forward) | Recommendation: Address before merge

Incremental review: PR HEAD (da25801) is identical to the previous review commit, so no new diff was introduced. All findings below were raised by other reviewers and re-verified against the current code on this commit. One prior finding was already fixed in da25801.

Overview

Severity Count
CRITICAL 1
WARNING 2
SUGGESTION 1
Issue Details (click to expand)

CRITICAL

File Line Issue
src/utils/model-name-convert.mjs 253 defaultId === 'customModel' special-case marks the id known but never materializes 'Custom Model' into customApiModes. Since defaultApiModeIds includes 'customModel', the custom model default will never appear for customized profiles when reconciled. Remove the special-case so it is treated like any other default.

WARNING

File Line Issue
src/utils/model-name-convert.mjs 270 getApiModeDefaultIdentity includes itemName; legacy Azure/Ollama rows with empty itemName mismatch the canonical identity, so reconcileMaterializedApiModeDefaults can append a duplicate default row. Normalize empty itemName for those built-in preset groups before stringifying.
src/utils/model-name-convert.mjs 142 AlwaysCustomGroups filter keeps any non-custom row with a truthy itemName; a malformed { groupName: 'customApiModelKeys', itemName: 'customApiModelKeys', isCustom: false } row is admitted. Narrow the non-custom exception to only the intended built-in Azure/Ollama preset rows.

SUGGESTION

File Line Issue
src/config/index.mjs 1582 currentDefaultIds.length === 0 is redundant because currentDefaultIds derives from the non-empty constant defaultApiModeIds. Simplify to rawKnownApiModeDefaultIds.length > 0.
Resolved / Verified
  • src/popup/sections/import-data-cleanup.mjs:27 — Stale selection-state finding is addressed in da25801: when an API-mode list key is present but a selection key is omitted, the selection key is now removed (lines 24-26), preventing storageArea.set merge from retaining an orphaned selection.
Files Reviewed (10 files)
  • src/config/index.mjs
  • src/popup/Popup.jsx
  • src/popup/api-mode-config-utils.mjs
  • src/popup/sections/ApiModes.jsx
  • src/popup/sections/import-data-cleanup.mjs
  • src/utils/model-name-convert.mjs
  • tests/unit/config/migrate-user-config.test.mjs
  • tests/unit/popup/api-mode-config-utils.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/utils/model-name-convert.test.mjs

Previous review (commit d99a2c6)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (10 files)
  • src/config/index.mjs
  • src/popup/Popup.jsx
  • src/popup/api-mode-config-utils.mjs
  • src/popup/sections/ApiModes.jsx
  • src/popup/sections/import-data-cleanup.mjs
  • src/utils/model-name-convert.mjs
  • tests/unit/config/migrate-user-config.test.mjs
  • tests/unit/popup/api-mode-config-utils.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/utils/model-name-convert.test.mjs

Reviewed by hy3:free · Input: 66.3K · Output: 1.9K · Cached: 226.1K

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request implements a significant refactor of the API mode configuration system, introducing a versioned schema (upgraded to version 2) and a more robust mechanism for materializing default API modes. It introduces 'knownApiModeDefaultIds' to track and reconcile default modes, ensuring that user configurations remain stable across updates while allowing for the seamless addition of new defaults. The changes include updates to the migration logic, the introduction of utility functions for building and expanding configuration updates, and comprehensive unit tests to verify the migration paths and default reconciliation behavior.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@PeterDaveHello
PeterDaveHello requested a review from Copilot July 16, 2026 19:25

@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/popup/sections/import-data-cleanup.mjs`:
- Around line 13-23: Update prepareImportData so replacing any API-mode list
also resets missing apiMode and modelName fields to null, preventing storage
merging from retaining stale selections. Use the existing list-replacement
detection and list metadata, and extend the import integration test to cover
omitted selection fields and stale persisted values.
🪄 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: 21afd85f-7b39-49f4-a85d-d2e85332c7cd

📥 Commits

Reviewing files that changed from the base of the PR and between c2106cc and d99a2c6.

📒 Files selected for processing (10)
  • src/config/index.mjs
  • src/popup/Popup.jsx
  • src/popup/api-mode-config-utils.mjs
  • src/popup/sections/ApiModes.jsx
  • src/popup/sections/import-data-cleanup.mjs
  • src/utils/model-name-convert.mjs
  • tests/unit/config/migrate-user-config.test.mjs
  • tests/unit/popup/api-mode-config-utils.test.mjs
  • tests/unit/popup/import-data-cleanup.test.mjs
  • tests/unit/utils/model-name-convert.test.mjs

Comment thread src/popup/sections/import-data-cleanup.mjs

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces a robust migration system for API mode configurations, transitioning from a simple list to a versioned, materialized baseline approach. It adds a new schema version (v2), introduces knownApiModeDefaultIds to track default API modes, and implements reconciliation logic to ensure user configurations remain consistent across updates. The changes include utility functions for building and expanding configuration updates, as well as comprehensive unit tests to verify the migration and reconciliation behavior. I have kept the critical feedback regarding the inconsistent handling of 'customModel' and the suggestion to simplify the baseline check condition.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/utils/model-name-convert.mjs
Comment thread src/config/index.mjs
@qodo-code-review

qodo-code-review Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

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

Grey Divider


Action required

1. Selection rollback lost ✓ Resolved 🐞 Bug ≡ Correctness
Description
Popup.updateConfig now expands API-mode list writes to always include modelName/apiMode and then
marks those keys as owned by the list request, even when the caller didn’t intend to change
selection. If an earlier selection-only write fails, its rollback won’t apply (ownership was
overwritten) and the later list write can persist the uncommitted selection state.
Code

src/popup/Popup.jsx[R91-95]

+    const rawValue = value && typeof value === 'object' ? value : {}
+    const nextValue = expandApiModeListConfigUpdate(latestConfigRef.current, rawValue)
    const { propagateError = false } = options && typeof options === 'object' ? options : {}
    const requestId = ++updateConfigRequestIdRef.current
    for (const key of Object.keys(nextValue)) {
Evidence
The issue is caused by (1) list-update payload expansion injecting selection keys, (2) updateConfig
recording ownership for all expanded keys, and (3) rollback only applying to keys still owned by the
failing requestId—so a later list update can steal ownership and prevent rollback of a failed
selection-only write.

src/popup/Popup.jsx[88-104]
src/popup/api-mode-config-utils.mjs[24-37]
src/popup/popup-config-utils.mjs[14-30]

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

### Issue description
`Popup.updateConfig` uses per-key ownership to decide what to rollback on write failure. After this PR, `expandApiModeListConfigUpdate()` injects `modelName` and `apiMode` into any payload that touches the API-mode list, and `updateConfig` then records those injected keys as “touched” by the list request.

This can suppress rollback of a *previous* selection-only request that fails (because the selection keys’ ownership got overwritten), and the subsequent list write can accidentally persist the earlier optimistic (but failed) selection.

### Issue Context
- `Popup.updateConfig` marks **every** key in the (expanded) payload as owned by the current request and uses that ownership map for rollback.
- `expandApiModeListConfigUpdate` currently injects `modelName` and `apiMode` for any API-mode list update, even when selection wasn’t part of the caller’s request.

### Fix Focus Areas
- src/popup/Popup.jsx[88-104]
- src/popup/api-mode-config-utils.mjs[24-37]
- src/popup/popup-config-utils.mjs[14-30]

### Suggested fix approach
Pick one of these (A is simplest and least risky):

**A) Only expand list fields, not selection fields**
- Change `expandApiModeListConfigUpdate` to fill only `{activeApiModes, customApiModes, knownApiModeDefaultIds}`.
- Keep selection changes explicit via callers’ `selectionPatch` (already used in `ApiModes.jsx`).

**B) Expand selection fields only when explicitly requested**
- In `expandApiModeListConfigUpdate`, include `modelName`/`apiMode` only if `value` already has `modelName` or `apiMode` as own properties.

If you keep persisting selection “atomically” even when not requested, you’ll also need an ownership strategy that doesn’t overwrite selection key ownership (e.g., track ownership based on the *raw* caller payload rather than the expanded payload), otherwise earlier failed selection writes can be incorrectly committed by later list writes.

### Tests
Add a unit test that simulates:
1) selection-only update A (apiMode/modelName) queued,
2) list update B queued before A resolves,
3) A fails,
4) B succeeds,
…and assert that selection is rolled back (or at minimum not persisted) unless B explicitly requested selection changes.

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


2. Default dedupe misses legacy ✓ Resolved 🐞 Bug ≡ Correctness
Description
reconcileMaterializedApiModeDefaults dedupes using an identity that includes itemName, but
legacy Azure/Ollama rows can have an empty itemName; equivalent rows will not match and a
duplicate default row can be appended during future-default reconciliation.
Code

src/utils/model-name-convert.mjs[R225-270]

+function getApiModeDefaultIdentity(apiMode) {
+  const normalized = normalizeApiMode(apiMode)
+  if (!normalized) return ''
+  return JSON.stringify({
+    groupName: normalized.groupName,
+    itemName: normalized.itemName,
+    isCustom: normalized.isCustom,
+    customName: normalized.customName.trim(),
+    providerId: normalized.providerId,
+  })
+}
+
+export function reconcileMaterializedApiModeDefaults(config, defaultIds, knownDefaultIds) {
+  const currentModes = Array.isArray(config.customApiModes)
+    ? config.customApiModes.map((apiMode) => ({ ...apiMode }))
+    : []
+  const knownIds = Array.isArray(knownDefaultIds) ? [...knownDefaultIds] : []
+  const knownIdSet = new Set(knownIds)
+  let changed = false
+
+  for (const defaultId of Array.isArray(defaultIds) ? defaultIds : []) {
+    if (knownIdSet.has(defaultId)) continue
+
+    if (defaultId === 'customModel') {
+      knownIds.push(defaultId)
+      knownIdSet.add(defaultId)
+      changed = true
+      continue
+    }
+
+    const materializedModes = getApiModesFromConfig(
+      {
+        ...config,
+        activeApiModes: [defaultId],
+        customApiModes: [],
+      },
+      false,
+    )
+    const materializedMode = materializedModes[0]
+    if (!materializedMode) continue
+
+    const identity = getApiModeDefaultIdentity(materializedMode)
+    const alreadyExists = currentModes.some(
+      (apiMode) => getApiModeDefaultIdentity(apiMode) === identity,
+    )
+    if (!alreadyExists) currentModes.push({ ...materializedMode, active: true })
Evidence
The reconciliation dedupe identity includes itemName, but the same module acknowledges
Azure/Ollama legacy rows may have empty itemName and need canonicalization in other paths; tests
confirm such legacy rows exist. Therefore, reconciliation can fail to recognize an equivalent legacy
row and append a duplicate.

src/utils/model-name-convert.mjs[225-274]
src/utils/model-name-convert.mjs[158-175]
tests/unit/utils/model-name-convert.test.mjs[549-563]

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

### Issue description
`reconcileMaterializedApiModeDefaults()` deduplicates existing rows by comparing a JSON identity that includes `itemName`. For Azure/Ollama legacy rows, `itemName` may be empty (a supported legacy format), so the identity mismatches the newly materialized default row (which has canonical `itemName`), and reconciliation can append a duplicate row.

### Issue Context
The codebase already contains explicit logic to repair legacy Azure/Ollama rows with missing `itemName` when they are matched through `activeApiModes` (`applyCanonicalLegacyItemName`). The reconciliation path compares identities without applying that repair.

### Fix Focus Areas
- src/utils/model-name-convert.mjs[225-274]
- src/utils/model-name-convert.mjs[158-175]
- tests/unit/utils/model-name-convert.test.mjs[549-590]

### Recommended fix
1. Adjust `getApiModeDefaultIdentity()` so legacy Azure/Ollama rows normalize to the canonical `itemName` before stringifying, OR ignore `itemName` for `AlwaysCustomGroups` entirely.
  - Example normalization:
    - if `groupName === 'azureOpenAiApiModelKeys'` and `itemName` is empty -> treat as `'azureOpenAi'`
    - if `groupName === 'ollamaApiModelKeys'` and `itemName` is empty -> treat as `'ollamaModel'`
2. Add a unit test covering reconciliation when `customApiModes` contains a legacy Azure/Ollama row with `itemName: ''` and `knownApiModeDefaultIds` is missing that default ID, asserting reconciliation does **not** append a duplicate row.

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



Remediation recommended

3. Default IDs shared reference ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
migrateUserConfig can end up assigning defaultApiModeIds by reference into
migrated.knownApiModeDefaultIds because canonicalizeModelKeyArray returns the original array
when unchanged. Any later in-place mutation of knownApiModeDefaultIds would then corrupt the
module-level defaults and make future migrations non-deterministic.
Code

src/config/index.mjs[R1569-1662]

+  const currentDefaultIds = canonicalizeModelKeyArray(defaultApiModeIds)
+  const migrationConfig = {
+    ...defaultConfig,
+    ...migrated,
+    customApiModes,
+    customOpenAIProviders,
+    providerSecrets,
+  }
+  const configuredCustomApiModes = getApiModesFromConfig(
+    {
+      ...migrationConfig,
+      activeApiModes: [],
+    },
+    false,
+  )
+  const hasStoredActiveApiModes = Array.isArray(migrated.activeApiModes)
+  const rawKnownApiModeDefaultIds = migrated.knownApiModeDefaultIds
+  const hasCurrentBaseline =
+    Number(migrated.configSchemaVersion) >= 2 &&
+    Array.isArray(rawKnownApiModeDefaultIds) &&
+    (currentDefaultIds.length === 0 || rawKnownApiModeDefaultIds.length > 0) &&
+    rawKnownApiModeDefaultIds.every(
+      (modelName) => typeof modelName === 'string' && Boolean(modelName.trim()),
+    )
+  const isLiveDefaultProfile =
+    !hasStoredActiveApiModes && !hasCurrentBaseline && configuredCustomApiModes.length === 0
+
+  if (isLiveDefaultProfile) {
+    if (Object.hasOwn(options, 'activeApiModes')) {
+      delete migrated.activeApiModes
+      storageKeysToRemove.push('activeApiModes')
+      dirty = true
+    }
+    if (Object.hasOwn(options, 'knownApiModeDefaultIds')) {
+      delete migrated.knownApiModeDefaultIds
+      storageKeysToRemove.push('knownApiModeDefaultIds')
+      dirty = true
+    }
+  } else {
+    const activeApiModesForMaterialization = hasStoredActiveApiModes
+      ? migrated.activeApiModes
+      : hasCurrentBaseline
+      ? []
+      : [...currentDefaultIds]
+
+    if (!hasStoredActiveApiModes && hasCurrentBaseline) {
+      migrated.activeApiModes = []
+      dirty = true
+    }
+
+    if (!hasCurrentBaseline || activeApiModesForMaterialization.length > 0) {
+      customApiModes = getApiModesFromConfig(
+        {
+          ...migrationConfig,
+          activeApiModes: activeApiModesForMaterialization,
+          customApiModes,
+        },
+        false,
+      )
+      migrated.activeApiModes = []
+      dirty = true
+    }
+
+    let knownApiModeDefaultIds = hasCurrentBaseline
+      ? canonicalizeModelKeyArray(
+          migrated.knownApiModeDefaultIds
+            .filter((modelName) => typeof modelName === 'string')
+            .map((modelName) => modelName.trim())
+            .filter(Boolean),
+        )
+      : currentDefaultIds
+
+    if (hasCurrentBaseline) {
+      const reconciled = reconcileMaterializedApiModeDefaults(
+        {
+          ...migrationConfig,
+          activeApiModes: [],
+          customApiModes,
+        },
+        currentDefaultIds,
+        knownApiModeDefaultIds,
+      )
+      customApiModes = reconciled.customApiModes
+      knownApiModeDefaultIds = reconciled.knownApiModeDefaultIds
+      if (reconciled.changed) dirty = true
+    }
+
+    if (
+      JSON.stringify(migrated.knownApiModeDefaultIds) !== JSON.stringify(knownApiModeDefaultIds)
+    ) {
+      dirty = true
+    }
+    migrated.knownApiModeDefaultIds = knownApiModeDefaultIds
+  }
Evidence
canonicalizeModelKeyArray explicitly returns the original input array when no changes occur, so
currentDefaultIds can alias defaultApiModeIds; the migration then uses currentDefaultIds as
the baseline value assigned into the migrated config.

src/config/model-key-migrations.mjs[118-137]
src/config/index.mjs[662-678]
src/config/index.mjs[1569-1662]

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

### Issue description
`canonicalizeModelKeyArray(defaultApiModeIds)` can return the same array instance as `defaultApiModeIds` when no canonicalization/dedupe changes are needed. That array is later assigned into `migrated.knownApiModeDefaultIds`, creating an alias to the exported defaults array. If any subsequent code mutates `knownApiModeDefaultIds` in-place (e.g., `push`, `sort`, `splice`), it will mutate `defaultApiModeIds` globally.

### Issue Context
This PR introduces baseline tracking via `knownApiModeDefaultIds` and computes `currentDefaultIds` from `defaultApiModeIds`. Because `canonicalizeModelKeyArray` sometimes returns its input array unchanged, this can produce shared references.

### Fix Focus Areas
- src/config/index.mjs[1569-1662]
- src/config/model-key-migrations.mjs[118-137]

### Suggested fix
Make `currentDefaultIds` (and any baseline assignment using it) a fresh array:
- `const currentDefaultIds = canonicalizeModelKeyArray([...defaultApiModeIds])`
- and/or when seeding the baseline: `knownApiModeDefaultIds = [...currentDefaultIds]`
Optionally freeze the exported constant: `export const defaultApiModeIds = Object.freeze([...])`.

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


4. Live-default misclassification ✗ Dismissed 🐞 Bug ≡ Correctness
Description
migrateUserConfig only treats a profile as “live defaults” when activeApiModes is missing; if a
legacy snapshot has activeApiModes persisted but equal to the current defaults, it will be
migrated into materialized customApiModes and a persisted knownApiModeDefaultIds baseline. This
changes the profile semantics (from live defaults to materialized defaults) even though the
user-visible state was still default-valued.
Code

src/config/index.mjs[R1584-1630]

+  const hasStoredActiveApiModes = Array.isArray(migrated.activeApiModes)
+  const rawKnownApiModeDefaultIds = migrated.knownApiModeDefaultIds
+  const hasCurrentBaseline =
+    Number(migrated.configSchemaVersion) >= 2 &&
+    Array.isArray(rawKnownApiModeDefaultIds) &&
+    (currentDefaultIds.length === 0 || rawKnownApiModeDefaultIds.length > 0) &&
+    rawKnownApiModeDefaultIds.every(
+      (modelName) => typeof modelName === 'string' && Boolean(modelName.trim()),
+    )
+  const isLiveDefaultProfile =
+    !hasStoredActiveApiModes && !hasCurrentBaseline && configuredCustomApiModes.length === 0
+
+  if (isLiveDefaultProfile) {
+    if (Object.hasOwn(options, 'activeApiModes')) {
+      delete migrated.activeApiModes
+      storageKeysToRemove.push('activeApiModes')
+      dirty = true
+    }
+    if (Object.hasOwn(options, 'knownApiModeDefaultIds')) {
+      delete migrated.knownApiModeDefaultIds
+      storageKeysToRemove.push('knownApiModeDefaultIds')
+      dirty = true
+    }
+  } else {
+    const activeApiModesForMaterialization = hasStoredActiveApiModes
+      ? migrated.activeApiModes
+      : hasCurrentBaseline
+      ? []
+      : [...currentDefaultIds]
+
+    if (!hasStoredActiveApiModes && hasCurrentBaseline) {
+      migrated.activeApiModes = []
+      dirty = true
+    }
+
+    if (!hasCurrentBaseline || activeApiModesForMaterialization.length > 0) {
+      customApiModes = getApiModesFromConfig(
+        {
+          ...migrationConfig,
+          activeApiModes: activeApiModesForMaterialization,
+          customApiModes,
+        },
+        false,
+      )
+      migrated.activeApiModes = []
+      dirty = true
+    }
Evidence
The migration’s “live default” gate requires activeApiModes to be absent, so any stored array
(even if it matches defaults) is routed through materialization and baseline persistence. Backups
are exported from raw storage keys, so any previously persisted activeApiModes will be carried
through export/import and continue to trigger this behavior.

src/config/index.mjs[1569-1630]
src/config/index.mjs[1632-1662]
src/popup/sections/GeneralPart.jsx[987-996]

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

## Issue description
`migrateUserConfig` treats any array-valued `activeApiModes` as evidence of a customized/materialized profile (`hasStoredActiveApiModes`), even when that stored list is exactly the current default set. In that case it will enter the materialization path and persist a baseline, converting a default-valued legacy snapshot into a materialized/baselined profile.

## Issue Context
This shows up most often when:
- a legacy version persisted default `activeApiModes`, and/or
- a user exports/imports a backup created from older storage state (export uses `Browser.storage.local.get(null)` which will include any previously persisted `activeApiModes`).

## Fix Focus Areas
- src/config/index.mjs[1569-1662]

### Suggested change
Refine `isLiveDefaultProfile` detection to also treat *stored* `activeApiModes` as live-default when:
- there is no current baseline (`hasCurrentBaseline` is false),
- there are no configured custom modes (`configuredCustomApiModes.length === 0`), and
- the canonicalized stored `activeApiModes` equals the canonical current default id list.

In that case, delete `migrated.activeApiModes` and enqueue `activeApiModes` for removal (same as the current `isLiveDefaultProfile` cleanup), instead of materializing into `customApiModes`.

### Test
Add a unit test covering a legacy snapshot like:
- `configSchemaVersion: 1` (or missing),
- `activeApiModes: defaultApiModeIds`,
- no valid custom API modes,
and assert the migration removes the stored `activeApiModes` key and keeps the profile on live defaults (no baseline persisted).

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


5. Unsanitized activeApiModes entries 🐞 Bug ☼ Reliability
Description
migrateUserConfig now passes stored activeApiModes into getApiModesFromConfig for
materialization; a truthy non-string element (e.g., 1 or {}) can throw inside string-based
model-name helpers and abort config migration/loading.
Code

src/config/index.mjs[R1601-1623]

+    const activeApiModesForMaterialization = hasStoredActiveApiModes
+      ? migrated.activeApiModes
+      : hasCurrentBaseline
+      ? []
+      : [...currentDefaultIds]
+
+    if (!hasStoredActiveApiModes && hasCurrentBaseline) {
+      migrated.activeApiModes = []
+      dirty = true
+    }
+
+    if (!hasCurrentBaseline || activeApiModesForMaterialization.length > 0) {
+      customApiModes = getApiModesFromConfig(
+        {
+          ...migrationConfig,
+          activeApiModes: activeApiModesForMaterialization,
+          customApiModes,
+        },
+        false,
+      )
+      migrated.activeApiModes = []
+      dirty = true
+    }
Evidence
The migration now materializes activeApiModes through getApiModesFromConfig, but the repo’s
canonicalization/import paths do not guarantee activeApiModes is a string-only array; downstream
helpers treat truthy values as strings and may throw on non-strings.

src/config/index.mjs[1562-1623]
src/config/model-key-migrations.mjs[91-137]
src/popup/sections/import-data-cleanup.mjs[48-61]
src/utils/model-name-convert.mjs[60-63]

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

### Issue description
`migrateUserConfig()` materializes `activeApiModes` through `getApiModesFromConfig()`, but `activeApiModes` can contain non-string values because canonicalization/import normalization does not filter them out. Truthy non-string values can crash when downstream helpers assume strings (e.g., calling `.includes()`), preventing `getUserConfig()` from completing.

### Issue Context
- `canonicalizeModelKeyArray()` preserves non-string items, so malformed arrays can survive migration.
- `prepareImportData()` canonicalizes `activeApiModes` but does not filter to strings.
- `getApiModesFromConfig()` ultimately reaches model-name helpers that assume strings.

### Fix Focus Areas
- Filter/sanitize `activeApiModes` (and any other "model key arrays" that are expected to be string IDs) before calling `getApiModesFromConfig()` during migration.
- Consider also filtering during import normalization so malformed backups don’t enter storage.

#### Suggested approach
- In `migrateUserConfig`, when computing `activeApiModesForMaterialization`, ensure the array is `string[]` (e.g., `filter((x) => typeof x === 'string' && x.trim())`) before passing into `getApiModesFromConfig`.
- Optionally, add similar filtering in `prepareImportData` for `activeApiModes`.

### Fix Focus Areas (code references)
- src/config/index.mjs[1562-1623]
- src/popup/sections/import-data-cleanup.mjs[48-61]
- src/config/model-key-migrations.mjs[118-137]
- src/utils/model-name-convert.mjs[60-67]

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



Informational

6. Config update lacks guards ✗ Dismissed 🐞 Bug ☼ Reliability
Description
expandApiModeListConfigUpdate dereferences currentConfig.* without validating currentConfig is
a non-null object with those keys. While Popup.jsx currently passes a full config object, any
future reuse (or accidental partial caller) could throw or generate an atomic bundle containing
undefined values that overwrite valid stored state.
Code

src/popup/api-mode-config-utils.mjs[R24-44]

+export function expandApiModeListConfigUpdate(currentConfig, value) {
+  const nextValue = value && typeof value === 'object' ? value : {}
+  if (!API_MODE_LIST_CONFIG_KEYS.some((key) => Object.hasOwn(nextValue, key))) {
+    return nextValue
+  }
+
+  const includesSelectionUpdate =
+    Object.hasOwn(nextValue, 'modelName') || Object.hasOwn(nextValue, 'apiMode')
+
+  return {
+    activeApiModes: currentConfig.activeApiModes,
+    customApiModes: currentConfig.customApiModes,
+    knownApiModeDefaultIds: currentConfig.knownApiModeDefaultIds,
+    ...(includesSelectionUpdate
+      ? {
+          modelName: currentConfig.modelName,
+          apiMode: currentConfig.apiMode,
+        }
+      : {}),
+    ...nextValue,
+  }
Evidence
The helper directly dereferences currentConfig.* (no guard), and the only in-repo call site relies
on latestConfigRef.current being initialized to defaultConfig to avoid a crash.

src/popup/api-mode-config-utils.mjs[24-45]
src/popup/Popup.jsx[66-105]

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

### Issue description
`expandApiModeListConfigUpdate(currentConfig, value)` assumes `currentConfig` is a fully populated config object and unconditionally reads `currentConfig.activeApiModes`, `currentConfig.customApiModes`, `currentConfig.knownApiModeDefaultIds`, and optionally `currentConfig.modelName/apiMode`. If `currentConfig` is ever nullish (or missing these fields), this can throw or produce an update payload that unintentionally writes `undefined` into storage.

### Issue Context
Today, `Popup.jsx` initializes `latestConfigRef` with `defaultConfig`, so the current call path is safe. This is still a brittle exported helper and can become a sharp edge during refactors or reuse.

### Fix Focus Areas
- src/popup/api-mode-config-utils.mjs[24-45]
- src/popup/Popup.jsx[66-105]

### Suggested fix
Harden the helper input:
- Normalize `currentConfig` at the top: `const base = currentConfig && typeof currentConfig === 'object' ? currentConfig : {}`
- Use null-safe reads with defaults: `activeApiModes: Array.isArray(base.activeApiModes) ? base.activeApiModes : []`, etc.
- (Optional) If you want to enforce a strict contract instead, throw a clear error when `currentConfig` is invalid (rather than a TypeError).

ⓘ 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 ed87e8d

Results up to commit d99a2c6


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


Action required
1. Default dedupe misses legacy ✓ Resolved 🐞 Bug ≡ Correctness
Description
reconcileMaterializedApiModeDefaults dedupes using an identity that includes itemName, but
legacy Azure/Ollama rows can have an empty itemName; equivalent rows will not match and a
duplicate default row can be appended during future-default reconciliation.
Code

src/utils/model-name-convert.mjs[R225-270]

+function getApiModeDefaultIdentity(apiMode) {
+  const normalized = normalizeApiMode(apiMode)
+  if (!normalized) return ''
+  return JSON.stringify({
+    groupName: normalized.groupName,
+    itemName: normalized.itemName,
+    isCustom: normalized.isCustom,
+    customName: normalized.customName.trim(),
+    providerId: normalized.providerId,
+  })
+}
+
+export function reconcileMaterializedApiModeDefaults(config, defaultIds, knownDefaultIds) {
+  const currentModes = Array.isArray(config.customApiModes)
+    ? config.customApiModes.map((apiMode) => ({ ...apiMode }))
+    : []
+  const knownIds = Array.isArray(knownDefaultIds) ? [...knownDefaultIds] : []
+  const knownIdSet = new Set(knownIds)
+  let changed = false
+
+  for (const defaultId of Array.isArray(defaultIds) ? defaultIds : []) {
+    if (knownIdSet.has(defaultId)) continue
+
+    if (defaultId === 'customModel') {
+      knownIds.push(defaultId)
+      knownIdSet.add(defaultId)
+      changed = true
+      continue
+    }
+
+    const materializedModes = getApiModesFromConfig(
+      {
+        ...config,
+        activeApiModes: [defaultId],
+        customApiModes: [],
+      },
+      false,
+    )
+    const materializedMode = materializedModes[0]
+    if (!materializedMode) continue
+
+    const identity = getApiModeDefaultIdentity(materializedMode)
+    const alreadyExists = currentModes.some(
+      (apiMode) => getApiModeDefaultIdentity(apiMode) === identity,
+    )
+    if (!alreadyExists) currentModes.push({ ...materializedMode, active: true })
Evidence
The reconciliation dedupe identity includes itemName, but the same module acknowledges
Azure/Ollama legacy rows may have empty itemName and need canonicalization in other paths; tests
confirm such legacy rows exist. Therefore, reconciliation can fail to recognize an equivalent legacy
row and append a duplicate.

src/utils/model-name-convert.mjs[225-274]
src/utils/model-name-convert.mjs[158-175]
tests/unit/utils/model-name-convert.test.mjs[549-563]

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

### Issue description
`reconcileMaterializedApiModeDefaults()` deduplicates existing rows by comparing a JSON identity that includes `itemName`. For Azure/Ollama legacy rows, `itemName` may be empty (a supported legacy format), so the identity mismatches the newly materialized default row (which has canonical `itemName`), and reconciliation can append a duplicate row.

### Issue Context
The codebase already contains explicit logic to repair legacy Azure/Ollama rows with missing `itemName` when they are matched through `activeApiModes` (`applyCanonicalLegacyItemName`). The reconciliation path compares identities without applying that repair.

### Fix Focus Areas
- src/utils/model-name-convert.mjs[225-274]
- src/utils/model-name-convert.mjs[158-175]
- tests/unit/utils/model-name-convert.test.mjs[549-590]

### Recommended fix
1. Adjust `getApiModeDefaultIdentity()` so legacy Azure/Ollama rows normalize to the canonical `itemName` before stringifying, OR ignore `itemName` for `AlwaysCustomGroups` entirely.
  - Example normalization:
    - if `groupName === 'azureOpenAiApiModelKeys'` and `itemName` is empty -> treat as `'azureOpenAi'`
    - if `groupName === 'ollamaApiModelKeys'` and `itemName` is empty -> treat as `'ollamaModel'`
2. Add a unit test covering reconciliation when `customApiModes` contains a legacy Azure/Ollama row with `itemName: ''` and `knownApiModeDefaultIds` is missing that default ID, asserting reconciliation does **not** append a duplicate row.

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


Results up to commit da25801


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


Remediation recommended
1. Unsanitized activeApiModes entries 🐞 Bug ☼ Reliability
Description
migrateUserConfig now passes stored activeApiModes into getApiModesFromConfig for
materialization; a truthy non-string element (e.g., 1 or {}) can throw inside string-based
model-name helpers and abort config migration/loading.
Code

src/config/index.mjs[R1601-1623]

+    const activeApiModesForMaterialization = hasStoredActiveApiModes
+      ? migrated.activeApiModes
+      : hasCurrentBaseline
+      ? []
+      : [...currentDefaultIds]
+
+    if (!hasStoredActiveApiModes && hasCurrentBaseline) {
+      migrated.activeApiModes = []
+      dirty = true
+    }
+
+    if (!hasCurrentBaseline || activeApiModesForMaterialization.length > 0) {
+      customApiModes = getApiModesFromConfig(
+        {
+          ...migrationConfig,
+          activeApiModes: activeApiModesForMaterialization,
+          customApiModes,
+        },
+        false,
+      )
+      migrated.activeApiModes = []
+      dirty = true
+    }
Evidence
The migration now materializes activeApiModes through getApiModesFromConfig, but the repo’s
canonicalization/import paths do not guarantee activeApiModes is a string-only array; downstream
helpers treat truthy values as strings and may throw on non-strings.

src/config/index.mjs[1562-1623]
src/config/model-key-migrations.mjs[91-137]
src/popup/sections/import-data-cleanup.mjs[48-61]
src/utils/model-name-convert.mjs[60-63]

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

### Issue description
`migrateUserConfig()` materializes `activeApiModes` through `getApiModesFromConfig()`, but `activeApiModes` can contain non-string values because canonicalization/import normalization does not filter them out. Truthy non-string values can crash when downstream helpers assume strings (e.g., calling `.includes()`), preventing `getUserConfig()` from completing.

### Issue Context
- `canonicalizeModelKeyArray()` preserves non-string items, so malformed arrays can survive migration.
- `prepareImportData()` canonicalizes `activeApiModes` but does not filter to strings.
- `getApiModesFromConfig()` ultimately reaches model-name helpers that assume strings.

### Fix Focus Areas
- Filter/sanitize `activeApiModes` (and any other "model key arrays" that are expected to be string IDs) before calling `getApiModesFromConfig()` during migration.
- Consider also filtering during import normalization so malformed backups don’t enter storage.

#### Suggested approach
- In `migrateUserConfig`, when computing `activeApiModesForMaterialization`, ensure the array is `string[]` (e.g., `filter((x) => typeof x === 'string' && x.trim())`) before passing into `getApiModesFromConfig`.
- Optionally, add similar filtering in `prepareImportData` for `activeApiModes`.

### Fix Focus Areas (code references)
- src/config/index.mjs[1562-1623]
- src/popup/sections/import-data-cleanup.mjs[48-61]
- src/config/model-key-migrations.mjs[118-137]
- src/utils/model-name-convert.mjs[60-67]

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


Results up to commit f53bcf1


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


Remediation recommended
1. Live-default misclassification ✗ Dismissed 🐞 Bug ≡ Correctness
Description
migrateUserConfig only treats a profile as “live defaults” when activeApiModes is missing; if a
legacy snapshot has activeApiModes persisted but equal to the current defaults, it will be
migrated into materialized customApiModes and a persisted knownApiModeDefaultIds baseline. This
changes the profile semantics (from live defaults to materialized defaults) even though the
user-visible state was still default-valued.
Code

src/config/index.mjs[R1584-1630]

+  const hasStoredActiveApiModes = Array.isArray(migrated.activeApiModes)
+  const rawKnownApiModeDefaultIds = migrated.knownApiModeDefaultIds
+  const hasCurrentBaseline =
+    Number(migrated.configSchemaVersion) >= 2 &&
+    Array.isArray(rawKnownApiModeDefaultIds) &&
+    (currentDefaultIds.length === 0 || rawKnownApiModeDefaultIds.length > 0) &&
+    rawKnownApiModeDefaultIds.every(
+      (modelName) => typeof modelName === 'string' && Boolean(modelName.trim()),
+    )
+  const isLiveDefaultProfile =
+    !hasStoredActiveApiModes && !hasCurrentBaseline && configuredCustomApiModes.length === 0
+
+  if (isLiveDefaultProfile) {
+    if (Object.hasOwn(options, 'activeApiModes')) {
+      delete migrated.activeApiModes
+      storageKeysToRemove.push('activeApiModes')
+      dirty = true
+    }
+    if (Object.hasOwn(options, 'knownApiModeDefaultIds')) {
+      delete migrated.knownApiModeDefaultIds
+      storageKeysToRemove.push('knownApiModeDefaultIds')
+      dirty = true
+    }
+  } else {
+    const activeApiModesForMaterialization = hasStoredActiveApiModes
+      ? migrated.activeApiModes
+      : hasCurrentBaseline
+      ? []
+      : [...currentDefaultIds]
+
+    if (!hasStoredActiveApiModes && hasCurrentBaseline) {
+      migrated.activeApiModes = []
+      dirty = true
+    }
+
+    if (!hasCurrentBaseline || activeApiModesForMaterialization.length > 0) {
+      customApiModes = getApiModesFromConfig(
+        {
+          ...migrationConfig,
+          activeApiModes: activeApiModesForMaterialization,
+          customApiModes,
+        },
+        false,
+      )
+      migrated.activeApiModes = []
+      dirty = true
+    }
Evidence
The migration’s “live default” gate requires activeApiModes to be absent, so any stored array
(even if it matches defaults) is routed through materialization and baseline persistence. Backups
are exported from raw storage keys, so any previously persisted activeApiModes will be carried
through export/import and continue to trigger this behavior.

src/config/index.mjs[1569-1630]
src/config/index.mjs[1632-1662]
src/popup/sections/GeneralPart.jsx[987-996]

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

## Issue description
`migrateUserConfig` treats any array-valued `activeApiModes` as evidence of a customized/materialized profile (`hasStoredActiveApiModes`), even when that stored list is exactly the current default set. In that case it will enter the materialization path and persist a baseline, converting a default-valued legacy snapshot into a materialized/baselined profile.

## Issue Context
This shows up most often when:
- a legacy version persisted default `activeApiModes`, and/or
- a user exports/imports a backup created from older storage state (export uses `Browser.storage.local.get(null)` which will include any previously persisted `activeApiModes`).

## Fix Focus Areas
- src/config/index.mjs[1569-1662]

### Suggested change
Refine `isLiveDefaultProfile` detection to also treat *stored* `activeApiModes` as live-default when:
- there is no current baseline (`hasCurrentBaseline` is false),
- there are no configured custom modes (`configuredCustomApiModes.length === 0`), and
- the canonicalized stored `activeApiModes` equals the canonical current default id list.

In that case, delete `migrated.activeApiModes` and enqueue `activeApiModes` for removal (same as the current `isLiveDefaultProfile` cleanup), instead of materializing into `customApiModes`.

### Test
Add a unit test covering a legacy snapshot like:
- `configSchemaVersion: 1` (or missing),
- `activeApiModes: defaultApiModeIds`,
- no valid custom API modes,
and assert the migration removes the stored `activeApiModes` key and keeps the profile on live defaults (no baseline persisted).

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


Results up to commit 15257cc


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


Action required
1. Selection rollback lost ✓ Resolved 🐞 Bug ≡ Correctness
Description
Popup.updateConfig now expands API-mode list writes to always include modelName/apiMode and then
marks those keys as owned by the list request, even when the caller didn’t intend to change
selection. If an earlier selection-only write fails, its rollback won’t apply (ownership was
overwritten) and the later list write can persist the uncommitted selection state.
Code

src/popup/Popup.jsx[R91-95]

+    const rawValue = value && typeof value === 'object' ? value : {}
+    const nextValue = expandApiModeListConfigUpdate(latestConfigRef.current, rawValue)
    const { propagateError = false } = options && typeof options === 'object' ? options : {}
    const requestId = ++updateConfigRequestIdRef.current
    for (const key of Object.keys(nextValue)) {
Evidence
The issue is caused by (1) list-update payload expansion injecting selection keys, (2) updateConfig
recording ownership for all expanded keys, and (3) rollback only applying to keys still owned by the
failing requestId—so a later list update can steal ownership and prevent rollback of a failed
selection-only write.

src/popup/Popup.jsx[88-104]
src/popup/api-mode-config-utils.mjs[24-37]
src/popup/popup-config-utils.mjs[14-30]

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

### Issue description
`Popup.updateConfig` uses per-key ownership to decide what to rollback on write failure. After this PR, `expandApiModeListConfigUpdate()` injects `modelName` and `apiMode` into any payload that touches the API-mode list, and `updateConfig` then records those injected keys as “touched” by the list request.

This can suppress rollback of a *previous* selection-only request that fails (because the selection keys’ ownership got overwritten), and the subsequent list write can accidentally persist the earlier optimistic (but failed) selection.

### Issue Context
- `Popup.updateConfig` marks **every** key in the (expanded) payload as owned by the current request and uses that ownership map for rollback.
- `expandApiModeListConfigUpdate` currently injects `modelName` and `apiMode` for any API-mode list update, even when selection wasn’t part of the caller’s request.

### Fix Focus Areas
- src/popup/Popup.jsx[88-104]
- src/popup/api-mode-config-utils.mjs[24-37]
- src/popup/popup-config-utils.mjs[14-30]

### Suggested fix approach
Pick one of these (A is simplest and least risky):

**A) Only expand list fields, not selection fields**
- Change `expandApiModeListConfigUpdate` to fill only `{activeApiModes, customApiModes, knownApiModeDefaultIds}`.
- Keep selection changes explicit via callers’ `selectionPatch` (already used in `ApiModes.jsx`).

**B) Expand selection fields only when explicitly requested**
- In `expandApiModeListConfigUpdate`, include `modelName`/`apiMode` only if `value` already has `modelName` or `apiMode` as own properties.

If you keep persisting selection “atomically” even when not requested, you’ll also need an ownership strategy that doesn’t overwrite selection key ownership (e.g., track ownership based on the *raw* caller payload rather than the expanded payload), otherwise earlier failed selection writes can be incorrectly committed by later list writes.

### Tests
Add a unit test that simulates:
1) selection-only update A (apiMode/modelName) queued,
2) list update B queued before A resolves,
3) A fails,
4) B succeeds,
…and assert that selection is rolled back (or at minimum not persisted) unless B explicitly requested selection changes.

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


Results up to commit cbfaec7


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


Remediation recommended
1. Default IDs shared reference ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
migrateUserConfig can end up assigning defaultApiModeIds by reference into
migrated.knownApiModeDefaultIds because canonicalizeModelKeyArray returns the original array
when unchanged. Any later in-place mutation of knownApiModeDefaultIds would then corrupt the
module-level defaults and make future migrations non-deterministic.
Code

src/config/index.mjs[R1569-1662]

+  const currentDefaultIds = canonicalizeModelKeyArray(defaultApiModeIds)
+  const migrationConfig = {
+    ...defaultConfig,
+    ...migrated,
+    customApiModes,
+    customOpenAIProviders,
+    providerSecrets,
+  }
+  const configuredCustomApiModes = getApiModesFromConfig(
+    {
+      ...migrationConfig,
+      activeApiModes: [],
+    },
+    false,
+  )
+  const hasStoredActiveApiModes = Array.isArray(migrated.activeApiModes)
+  const rawKnownApiModeDefaultIds = migrated.knownApiModeDefaultIds
+  const hasCurrentBaseline =
+    Number(migrated.configSchemaVersion) >= 2 &&
+    Array.isArray(rawKnownApiModeDefaultIds) &&
+    (currentDefaultIds.length === 0 || rawKnownApiModeDefaultIds.length > 0) &&
+    rawKnownApiModeDefaultIds.every(
+      (modelName) => typeof modelName === 'string' && Boolean(modelName.trim()),
+    )
+  const isLiveDefaultProfile =
+    !hasStoredActiveApiModes && !hasCurrentBaseline && configuredCustomApiModes.length === 0
+
+  if (isLiveDefaultProfile) {
+    if (Object.hasOwn(options, 'activeApiModes')) {
+      delete migrated.activeApiModes
+      storageKeysToRemove.push('activeApiModes')
+      dirty = true
+    }
+    if (Object.hasOwn(options, 'knownApiModeDefaultIds')) {
+      delete migrated.knownApiModeDefaultIds
+      storageKeysToRemove.push('knownApiModeDefaultIds')
+      dirty = true
+    }
+  } else {
+    const activeApiModesForMaterialization = hasStoredActiveApiModes
+      ? migrated.activeApiModes
+      : hasCurrentBaseline
+      ? []
+      : [...currentDefaultIds]
+
+    if (!hasStoredActiveApiModes && hasCurrentBaseline) {
+      migrated.activeApiModes = []
+      dirty = true
+    }
+
+    if (!hasCurrentBaseline || activeApiModesForMaterialization.length > 0) {
+      customApiModes = getApiModesFromConfig(
+        {
+          ...migrationConfig,
+          activeApiModes: activeApiModesForMaterialization,
+          customApiModes,
+        },
+        false,
+      )
+      migrated.activeApiModes = []
+      dirty = true
+    }
+
+    let knownApiModeDefaultIds = hasCurrentBaseline
+      ? canonicalizeModelKeyArray(
+          migrated.knownApiModeDefaultIds
+            .filter((modelName) => typeof modelName === 'string')
+            .map((modelName) => modelName.trim())
+            .filter(Boolean),
+        )
+      : currentDefaultIds
+
+    if (hasCurrentBaseline) {
+      const reconciled = reconcileMaterializedApiModeDefaults(
+        {
+          ...migrationConfig,
+          activeApiModes: [],
+          customApiModes,
+        },
+        currentDefaultIds,
+        knownApiModeDefaultIds,
+      )
+      customApiModes = reconciled.customApiModes
+      knownApiModeDefaultIds = reconciled.knownApiModeDefaultIds
+      if (reconciled.changed) dirty = true
+    }
+
+    if (
+      JSON.stringify(migrated.knownApiModeDefaultIds) !== JSON.stringify(knownApiModeDefaultIds)
+    ) {
+      dirty = true
+    }
+    migrated.knownApiModeDefaultIds = knownApiModeDefaultIds
+  }
Evidence
canonicalizeModelKeyArray explicitly returns the original input array when no changes occur, so
currentDefaultIds can alias defaultApiModeIds; the migration then uses currentDefaultIds as
the baseline value assigned into the migrated config.

src/config/model-key-migrations.mjs[118-137]
src/config/index.mjs[662-678]
src/config/index.mjs[1569-1662]

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

### Issue description
`canonicalizeModelKeyArray(defaultApiModeIds)` can return the same array instance as `defaultApiModeIds` when no canonicalization/dedupe changes are needed. That array is later assigned into `migrated.knownApiModeDefaultIds`, creating an alias to the exported defaults array. If any subsequent code mutates `knownApiModeDefaultIds` in-place (e.g., `push`, `sort`, `splice`), it will mutate `defaultApiModeIds` globally.

### Issue Context
This PR introduces baseline tracking via `knownApiModeDefaultIds` and computes `currentDefaultIds` from `defaultApiModeIds`. Because `canonicalizeModelKeyArray` sometimes returns its input array unchanged, this can produce shared references.

### Fix Focus Areas
- src/config/index.mjs[1569-1662]
- src/config/model-key-migrations.mjs[118-137]

### Suggested fix
Make `currentDefaultIds` (and any baseline assignment using it) a fresh array:
- `const currentDefaultIds = canonicalizeModelKeyArray([...defaultApiModeIds])`
- and/or when seeding the baseline: `knownApiModeDefaultIds = [...currentDefaultIds]`
Optionally freeze the exported constant: `export const defaultApiModeIds = Object.freeze([...])`.

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



Informational
2. Config update lacks guards ✗ Dismissed 🐞 Bug ☼ Reliability
Description
expandApiModeListConfigUpdate dereferences currentConfig.* without validating currentConfig is
a non-null object with those keys. While Popup.jsx currently passes a full config object, any
future reuse (or accidental partial caller) could throw or generate an atomic bundle containing
undefined values that overwrite valid stored state.
Code

src/popup/api-mode-config-utils.mjs[R24-44]

+export function expandApiModeListConfigUpdate(currentConfig, value) {
+  const nextValue = value && typeof value === 'object' ? value : {}
+  if (!API_MODE_LIST_CONFIG_KEYS.some((key) => Object.hasOwn(nextValue, key))) {
+    return nextValue
+  }
+
+  const includesSelectionUpdate =
+    Object.hasOwn(nextValue, 'modelName') || Object.hasOwn(nextValue, 'apiMode')
+
+  return {
+    activeApiModes: currentConfig.activeApiModes,
+    customApiModes: currentConfig.customApiModes,
+    knownApiModeDefaultIds: currentConfig.knownApiModeDefaultIds,
+    ...(includesSelectionUpdate
+      ? {
+          modelName: currentConfig.modelName,
+          apiMode: currentConfig.apiMode,
+        }
+      : {}),
+    ...nextValue,
+  }
Evidence
The helper directly dereferences currentConfig.* (no guard), and the only in-repo call site relies
on latestConfigRef.current being initialized to defaultConfig to avoid a crash.

src/popup/api-mode-config-utils.mjs[24-45]
src/popup/Popup.jsx[66-105]

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

### Issue description
`expandApiModeListConfigUpdate(currentConfig, value)` assumes `currentConfig` is a fully populated config object and unconditionally reads `currentConfig.activeApiModes`, `currentConfig.customApiModes`, `currentConfig.knownApiModeDefaultIds`, and optionally `currentConfig.modelName/apiMode`. If `currentConfig` is ever nullish (or missing these fields), this can throw or produce an update payload that unintentionally writes `undefined` into storage.

### Issue Context
Today, `Popup.jsx` initializes `latestConfigRef` with `defaultConfig`, so the current call path is safe. This is still a brittle exported helper and can become a sharp edge during refactors or reuse.

### Fix Focus Areas
- src/popup/api-mode-config-utils.mjs[24-45]
- src/popup/Popup.jsx[66-105]

### Suggested fix
Harden the helper input:
- Normalize `currentConfig` at the top: `const base = currentConfig && typeof currentConfig === 'object' ? currentConfig : {}`
- Use null-safe reads with defaults: `activeApiModes: Array.isArray(base.activeApiModes) ? base.activeApiModes : []`, etc.
- (Optional) If you want to enforce a strict contract instead, throw a clear error when `currentConfig` is invalid (rather than a TypeError).

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


Qodo Logo

Comment thread src/utils/model-name-convert.mjs

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 updates how API mode lists are persisted for customized profiles by introducing a “known default baseline” (knownApiModeDefaultIds). The goal is to let the extension append newly introduced default API modes in future releases without reintroducing defaults that a user previously removed, while keeping migrations/backups deterministic and rollback-safe.

Changes:

  • Introduces defaultApiModeIds + knownApiModeDefaultIds and migration logic to distinguish “live default” profiles from materialized/customized ones, and to reconcile future defaults deterministically.
  • Ensures API mode list + selection state are persisted as an atomic bundle in popup updates (including rollback-safe behavior on write failure).
  • Adds import cleanup behavior to clear stale/missing API mode fields for older backups, plus broad unit test coverage for the new behaviors.

Reviewed changes

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

Show a summary per file
File Description
tests/unit/utils/model-name-convert.test.mjs Adds unit tests for default reconciliation, dedupe behavior, and Azure/Ollama preset handling.
tests/unit/popup/import-data-cleanup.test.mjs Adds tests ensuring imported backups deterministically clear/normalize API mode state.
tests/unit/popup/api-mode-config-utils.test.mjs Adds tests for atomic API mode list update/rollback helpers used by the popup.
tests/unit/config/migrate-user-config.test.mjs Extends migration tests for schema v2 baseline behavior and deterministic retry after write failure.
src/utils/model-name-convert.mjs Adds default reconciliation helper and adjusts filtering logic for AlwaysCustomGroups.
src/popup/sections/import-data-cleanup.mjs Normalizes imported API mode fields (including baseline) and ensures missing fields are cleared atomically.
src/popup/sections/ApiModes.jsx Switches API mode edits/toggles/deletes to use atomic list+selection update helper.
src/popup/Popup.jsx Ensures API mode list updates are expanded into a complete atomic bundle before queueing persistence.
src/popup/api-mode-config-utils.mjs Introduces helpers to build/expand atomic API mode list config updates.
src/config/index.mjs Adds defaultApiModeIds, bumps schema version, and implements baseline-aware migration + safe persistence/removal ordering.

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

Comment thread src/utils/model-name-convert.mjs
@PeterDaveHello
PeterDaveHello force-pushed the preserve-future-api-mode-defaults branch from d99a2c6 to da25801 Compare July 16, 2026 19:38

Copy link
Copy Markdown
Member Author

/agentic_review

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces a robust migration and materialization system for API modes, upgrading the configuration schema to version 2. It centralizes default API mode management, improves configuration consistency by introducing knownApiModeDefaultIds, and adds utility functions to handle the reconciliation of materialized defaults. The changes also include comprehensive unit tests to verify the migration logic and ensure backward compatibility. I have included a suggestion to extract the array sanitization logic into a shared utility function to improve maintainability and reduce code duplication.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/config/index.mjs
Comment thread src/config/index.mjs
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

@PeterDaveHello
PeterDaveHello force-pushed the preserve-future-api-mode-defaults branch from da25801 to f53bcf1 Compare July 16, 2026 20:13

Copy link
Copy Markdown
Member Author

/agentic_review

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request updates the user configuration schema from version 1 to 2, introducing a more robust mechanism for handling default and customized API modes. It adds defaultApiModeIds and knownApiModeDefaultIds to allow untouched profiles to dynamically read live defaults while appending future defaults to customized profiles. The migration logic in migrateUserConfig has been updated to manage these keys and clean up stale storage keys. Additionally, the popup configuration update flow has been refactored, and extensive unit tests have been added. Feedback on these changes highlights that silently ignoring errors during Browser.storage.local.remove could leave the storage in an inconsistent state if a removal fails after a successful write, potentially corrupting subsequent configuration migrations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/config/index.mjs Outdated
Comment thread src/config/index.mjs
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

@PeterDaveHello
PeterDaveHello force-pushed the preserve-future-api-mode-defaults branch from f53bcf1 to 15257cc Compare July 16, 2026 20:31

Copy link
Copy Markdown
Member Author

/agentic_review

Copy link
Copy Markdown
Member Author

/gemini 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: 15257cc5e6

ℹ️ 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

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request updates the configuration schema to version 2, introducing a baseline tracking system via knownApiModeDefaultIds to manage default API modes dynamically. It materializes default API modes into customApiModes for customized profiles while allowing untouched profiles to read live defaults, and updates the popup components and import utilities to handle these updates atomically. The feedback suggests simplifying state management in ApiModes.jsx by deriving model names directly from the apiModes state rather than relying on a redundant state variable, and using optional chaining in expandApiModeListConfigUpdate to safely handle potentially null or undefined configurations.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/popup/sections/ApiModes.jsx
Comment thread src/popup/api-mode-config-utils.mjs
Comment thread src/popup/Popup.jsx
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 15257cc

@PeterDaveHello
PeterDaveHello force-pushed the preserve-future-api-mode-defaults branch from 15257cc to cbfaec7 Compare July 16, 2026 20:59

Copy link
Copy Markdown
Member Author

/agentic_review

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request refactors API mode configuration management by bumping the schema version to 2 and introducing a default baseline tracking mechanism (knownApiModeDefaultIds). This ensures untouched profiles use live defaults while customized profiles safely append future defaults. The changes include atomic config update helpers, import/export cleanup, and extensive unit tests. The review feedback focuses on enhancing robustness, specifically recommending keeping modelName and apiMode in sync during edits, guarding against potential TypeErrors when accessing properties of potentially null or undefined objects (data and currentConfig), and ensuring array checks are performed before canonicalizing model keys.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/popup/sections/ApiModes.jsx
Comment thread src/popup/sections/import-data-cleanup.mjs
Comment thread src/config/index.mjs
Comment thread src/popup/api-mode-config-utils.mjs
Comment thread src/config/index.mjs Outdated
Comment thread src/popup/api-mode-config-utils.mjs
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

Track the default API mode IDs seen by materialized profiles so newly
introduced defaults can be appended without restoring modes users
previously removed.

Migrate legacy list states, keep import recovery deterministic, and
persist list and selection changes as one rollback-safe update.
@PeterDaveHello
PeterDaveHello force-pushed the preserve-future-api-mode-defaults branch from cbfaec7 to ed87e8d Compare July 16, 2026 21:25

Copy link
Copy Markdown
Member Author

/agentic_review

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request upgrades the configuration schema to version 2, introducing a mechanism to manage API mode defaults and customizations. Untouched profiles now read live defaults dynamically, while customized profiles materialize their settings into customApiModes and track them using knownApiModeDefaultIds. The update includes migration logic, popup utility enhancements, import data cleanup, and comprehensive unit tests. The review feedback recommends adding defensive checks, specifically validating the input data object in prepareImportData and using optional chaining on currentConfig in expandApiModeListConfigUpdate to prevent potential runtime crashes.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/popup/sections/import-data-cleanup.mjs
Comment thread src/popup/api-mode-config-utils.mjs
@qodo-code-review

Copy link
Copy Markdown
Contributor

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

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