Preserve API mode selections after default list changes#1001
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe API mode dropdown now uses a shared helper to resolve its selected value, supports unmatched configured modes through a disabled fallback option, and removes per-option selection flags. Unit tests cover matched, custom, ambiguous, unmatched, and missing-provider label cases. ChangesAPI mode selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant GeneralPart
participant getSelectedApiModeOptionValue
User->>GeneralPart: open API mode select
GeneralPart->>getSelectedApiModeOptionValue: resolve current selection
getSelectedApiModeOptionValue-->>GeneralPart: return index, custom value, or unmatched sentinel
GeneralPart-->>User: render selected or unmatched option
User->>GeneralPart: choose an API mode option
GeneralPart-->>User: ignore unmatched sentinel or apply selected option
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoPreserve API mode selection when default mode list changes
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the API mode selection in the GeneralPart component to use a controlled <select> element by introducing getSelectedApiModeOptionValue and handling unmatched API modes with a disabled option. It also adds corresponding unit tests. Feedback suggests avoiding the React anti-pattern of synchronizing derived state via useLayoutEffect for apiModes and instead calculating it directly during render using useMemo.
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.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Notes
Previous Review Summary (commit 9cb60e5)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9cb60e5)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by hy3-20260706:free · Input: 59.4K · Output: 6.7K · Cached: 253.6K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/popup/sections/GeneralPart.jsx (1)
495-504: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd a guard for undefined
apiModein theonChangehandler.If
apiModeschanges between render and the change event,apiModes[e.target.value]could returnundefined, leading toupdateConfig({ apiMode: undefined }). The downstreamConversationCardguards against this withif (!selectedApiMode) return. Consider adding the same guard for consistency and safety.🛡️ Proposed fix
if (e.target.value === '-1') { updateConfig({ modelName: 'customModel', apiMode: null }) return } - const apiMode = apiModes[e.target.value] - updateConfig({ apiMode: apiMode }) + const apiMode = apiModes[Number(e.target.value)] + if (!apiMode) return + updateConfig({ apiMode: apiMode })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/popup/sections/GeneralPart.jsx` around lines 495 - 504, Update the onChange handler for selectedApiModeValue to check whether apiModes[e.target.value] exists before calling updateConfig. Return early when apiMode is undefined, while preserving the existing handling for UNMATCHED_API_MODE_VALUE and '-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.
Outside diff comments:
In `@src/popup/sections/GeneralPart.jsx`:
- Around line 495-504: Update the onChange handler for selectedApiModeValue to
check whether apiModes[e.target.value] exists before calling updateConfig.
Return early when apiMode is undefined, while preserving the existing handling
for UNMATCHED_API_MODE_VALUE and '-1'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 197d2380-2414-4d32-a668-d06a6b6365f4
📒 Files selected for processing (3)
src/popup/sections/GeneralPart.jsxsrc/popup/sections/api-modes-provider-utils.mjstests/unit/popup/api-modes-provider-utils.test.mjs
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
There was a problem hiding this comment.
Pull request overview
This PR updates the General settings “API Mode” dropdown to preserve and visibly represent the currently configured mode even when it is no longer part of the active/default-enabled API mode list, preventing the UI from silently falling back to a different selection.
Changes:
- Add
getSelectedApiModeOptionValue+UNMATCHED_API_MODE_VALUEto represent “currently configured but unavailable” selections. - Convert the General settings API mode
<select>to a controlled input and render a disabled “current mode” option when the configured mode is unmatched. - Add unit tests covering unmatched selections (legacy
modelName, removed API modes, ambiguous matches) and missing custom provider label fallback.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/popup/api-modes-provider-utils.test.mjs | Adds regression tests for unmatched/ambiguous API mode selection behavior and missing custom provider label fallback. |
| src/popup/sections/GeneralPart.jsx | Makes the API mode dropdown controlled and preserves unmatched selections via a disabled current-mode option. |
| src/popup/sections/api-modes-provider-utils.mjs | Introduces getSelectedApiModeOptionValue and a sentinel value for representing unmatched selections in UI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Keep the configured API mode visible when it is no longer enabled by default. This prevents the General settings dropdown from appearing to select an unrelated fallback model. Show the current mode as a disabled option until the user explicitly chooses an available mode.
9cb60e5 to
cf2103e
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors the API mode selection logic in the popup's GeneralPart section by introducing a controlled select component and a fallback placeholder for unmatched API modes. It also adds corresponding unit tests. Feedback suggests refactoring the apiModes state to a memoized value using useMemo to prevent a visual flash and double render on initial mount.
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.
|
Code review by qodo was updated up to the latest commit cf2103e |
Summary
This prevents default-list maintenance from making the General settings dropdown appear to select an unrelated fallback model. It does not add models back to the default list.
Follow-up to #1000 (comment).
Validation
npm run prettynpm run lintnpm test: 696 passednpm run buildSummary by CodeRabbit