fix: unify extension LLM config UI and require model before run#205
Merged
achuvyas-kv merged 1 commit intoJul 18, 2026
Merged
Conversation
WalkthroughThe extension popup now presents provider, API key, base URL, and model controls as a unified configuration flow. Provider-specific UI wiring was simplified, and Run-button eligibility now requires a selected model in addition to existing prerequisites. ChangesLLM configuration flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant PopupControls
participant ProviderLogic
participant ModelState
participant RunButton
User->>PopupControls: select provider or enter API key
PopupControls->>ProviderLogic: apply provider configuration
ProviderLogic->>ModelState: load or update model selection
ModelState->>RunButton: refresh eligibility
RunButton-->>User: enabled only when required fields are ready
Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@runners/extension/popup.html`:
- Around line 2240-2268: Update the Base URL guidance associated with the
baseUrl field and the corresponding applyProvider() logic so Azure users receive
Azure-appropriate text while OpenAI-compatible providers retain their existing
guidance, or use provider-neutral wording for both. Keep the label and field
behavior unchanged.
In `@runners/extension/popup.js`:
- Line 2799: Reset model eligibility in both fetch failure handlers: update
fetchModelsFromBaseUrl() at runners/extension/popup.js:2799-2799 and
fetchModelsForSimpleProvider() at runners/extension/popup.js:2853-2853 to clear
state.model, reset the model dropdown value, and call updateRunButton().
- Around line 2970-2978: Update the apiKey input handler to invalidate
simple-provider model state when credentials change: clear state.model and the
provider’s model options, mark models for re-fetch, and disable Run until the
refreshed models are loaded. Preserve the existing OpenAI-compatible
resetCompatModels behavior and avoid applying the simple-provider clearing logic
to that provider.
- Around line 2738-2742: Update updateRunButton and the related apiKeyOptional
UI logic so OpenAI-compatible providers can run without an API key when marked
optional; otherwise remove the optional-key messaging. Ensure Run eligibility
and the displayed optional hint use the same provider-aware authentication
requirement.
- Around line 442-447: Update updateRunButton() to compute a provider-aware
missingBaseUrl condition for Azure and OpenAI-compatible providers, requiring a
non-empty Base URL before enabling Run while preserving existing eligibility
checks. Also invoke updateRunButton() in the Base URL input listener so button
state updates immediately when the field changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 20befe22-8dfe-4606-824f-5b1555878388
📒 Files selected for processing (2)
runners/extension/popup.htmlrunners/extension/popup.js
Comment on lines
+2240
to
+2268
| <!-- Base URL (Azure + OpenAI-compatible only) --> | ||
| <div class="llm-field" id="baseUrlField" style="display: none"> | ||
| <div class="endpoint-step-label"> | ||
| Base URL | ||
| <span | ||
| class="info-tip" | ||
| data-tooltip="Root URL of your OpenAI-compatible server, e.g. http://localhost:11434/v1" | ||
| > | ||
| <svg | ||
| width="12" | ||
| height="12" | ||
| viewBox="0 0 24 24" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| stroke-width="2" | ||
| stroke-linecap="round" | ||
| stroke-linejoin="round" | ||
| > | ||
| <circle cx="12" cy="12" r="10" /> | ||
| <line x1="12" y1="8" x2="12" y2="12" /> | ||
| <line x1="12" y1="16" x2="12.01" y2="16" /> | ||
| </svg> | ||
| </span> | ||
| </div> | ||
| <input | ||
| id="baseUrl" | ||
| type="text" | ||
| placeholder="https://api.openai.com/v1" | ||
| autocomplete="off" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use provider-neutral Base URL guidance.
This row is also shown for Azure, but its tooltip and placeholder describe an OpenAI-compatible endpoint. Use neutral text or update both values in applyProvider() to avoid directing Azure users to the wrong URL format.
🤖 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 `@runners/extension/popup.html` around lines 2240 - 2268, Update the Base URL
guidance associated with the baseUrl field and the corresponding applyProvider()
logic so Azure users receive Azure-appropriate text while OpenAI-compatible
providers retain their existing guidance, or use provider-neutral wording for
both. Keep the label and field behavior unchanged.
Comment on lines
442
to
+447
| function updateRunButton() { | ||
| const missingKey = !state.apiKey.trim(); | ||
| const missingModel = !state.model.trim(); | ||
| const missingEvals = state.selectedEvaluators.size === 0; | ||
| const missingSuite = !state.suiteId || !state.catalog; | ||
| $("runBtn").disabled = missingKey || missingEvals || missingSuite; | ||
| $("runBtn").disabled = missingKey || missingModel || missingEvals || missingSuite; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include required Base URL in Run eligibility.
Azure and OpenAI-compatible providers can enable Run with an empty Base URL, although the downstream configuration contract rejects it. Add a provider-aware missingBaseUrl check and call updateRunButton() from the Base URL listener.
Proposed fix
function updateRunButton() {
const missingKey = !state.apiKey.trim();
const missingModel = !state.model.trim();
+ const missingBaseUrl =
+ PROVIDERS_NEEDING_BASE_URL.has(state.provider) && !state.baseUrl.trim();
const missingEvals = state.selectedEvaluators.size === 0;
const missingSuite = !state.suiteId || !state.catalog;
- $("runBtn").disabled = missingKey || missingModel || missingEvals || missingSuite;
+ $("runBtn").disabled =
+ missingKey || missingModel || missingBaseUrl || missingEvals || missingSuite;
}🤖 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 `@runners/extension/popup.js` around lines 442 - 447, Update updateRunButton()
to compute a provider-aware missingBaseUrl condition for Azure and
OpenAI-compatible providers, requiring a non-empty Base URL before enabling Run
while preserving existing eligibility checks. Also invoke updateRunButton() in
the Base URL input listener so button state updates immediately when the field
changes.
Comment on lines
+2738
to
+2742
| $("baseUrlField").style.display = needsBaseUrl ? "" : "none"; | ||
| $("apiKeyHint").style.display = isCompatible ? "inline-flex" : "none"; | ||
| $("apiKeyOptional").style.display = isCompatible ? "" : "none"; | ||
| $("refreshModelsBtn").style.display = isCompatible ? "" : "none"; | ||
|
|
||
| // Standalone API key — simple providers only | ||
| $("standaloneApiKey").style.display = isSimple ? "" : "none"; | ||
| $("apiKey").placeholder = apiKeyPlaceholder(state.provider); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the optional-key UI with Run eligibility.
OpenAI-compatible servers are marked as accepting an optional key, but updateRunButton() always disables Run when the key is empty. Make key eligibility provider-aware, or remove the optional messaging if authentication is actually mandatory.
🤖 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 `@runners/extension/popup.js` around lines 2738 - 2742, Update updateRunButton
and the related apiKeyOptional UI logic so OpenAI-compatible providers can run
without an API key when marked optional; otherwise remove the optional-key
messaging. Ensure Run eligibility and the displayed optional hint use the same
provider-aware authentication requirement.
| state.model = keep; | ||
| modelDD?.setValue(keep); | ||
| saveModelAndKey(); | ||
| updateRunButton(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset model eligibility on every model-fetch failure.
Both fetch implementations clear dropdown options after an error without clearing state.model, allowing Run to remain enabled with a stale model.
runners/extension/popup.js#L2799-L2799: infetchModelsFromBaseUrl()failure handling, clearstate.model, reset the dropdown value, and callupdateRunButton().runners/extension/popup.js#L2853-L2853: apply the same failure-state reset infetchModelsForSimpleProvider().
📍 Affects 1 file
runners/extension/popup.js#L2799-L2799(this comment)runners/extension/popup.js#L2853-L2853
🤖 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 `@runners/extension/popup.js` at line 2799, Reset model eligibility in both
fetch failure handlers: update fetchModelsFromBaseUrl() at
runners/extension/popup.js:2799-2799 and fetchModelsForSimpleProvider() at
runners/extension/popup.js:2853-2853 to clear state.model, reset the model
dropdown value, and call updateRunButton().
Comment on lines
+2970
to
+2978
| // API key (single field, shared by every provider) | ||
| $("apiKey").addEventListener("input", (e) => { | ||
| state.apiKey = e.target.value; | ||
| $("apiKeyCard").value = e.target.value; | ||
| saveModelAndKey(); | ||
| updateRunButton(); | ||
| setModelHint(""); | ||
| _compatModelsLoaded = false; | ||
| // Reset loaded models when the key changes so the next open re-fetches. | ||
| if (state.provider === PROVIDERS.OPENAI_COMPATIBLE) resetCompatModels(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Invalidate simple-provider models when credentials change.
For simple providers, changing the key marks models for re-fetch but retains the previous model, so Run can remain enabled before that refresh occurs. Clear state.model and the options, then disable Run until models are fetched for the new key.
🤖 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 `@runners/extension/popup.js` around lines 2970 - 2978, Update the apiKey input
handler to invalidate simple-provider model state when credentials change: clear
state.model and the provider’s model options, mark models for re-fetch, and
disable Run until the refreshed models are loaded. Preserve the existing
OpenAI-compatible resetCompatModels behavior and avoid applying the
simple-provider clearing logic to that provider.
achuvyas-kv
approved these changes
Jul 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Users reported several issues with the extension's LLM config panel:
providers), so it wasn't clear a key was required.
Solution
Reworked the panel into one consistent, labeled layout for every provider, and
gated the Run button on both a key and a model being present.
Changes
Packages/files affected:
runners/extensionpopup.html— renamed the section to "Model selection"; every provider nowshows the same labeled rows (Provider / API Key / Base URL / Model), with
Base URL shown only when required; removed the "Configure endpoint" accordion
and the duplicate key input; shortened the key placeholder to "Paste key here"
and fixed its font (was inheriting the mono + letter-spacing meant for the
typed key value).
popup.js— collapsed the two synced API-key inputs into one; simplifiedapplyProvider(no more DOM reparenting);updateRunButtonnow also requiresa model, and the model select/fetch/reset paths refresh the button.
Issue
N/A
How to test
runners/extensionas an unpacked Chrome extension; open the side panel.show; Base URL appears only for Azure and Custom (OpenAI-compatible).
model → Run enables.
Screenshots
Summary by CodeRabbit