Conversation
- Add copilot option to runner <select> so existing copilot-runner agents preserve their value on save - Add data-original attribute to capture the runner value at page load - Add #copilot-runner-warning hidden banner shown by checkCopilotRunnerWarning() when user changes away from copilot before saving - Add checkCopilotRunnerWarning() JS function called from onchange handler - Add copilot model list to updateModels() in partials.html AC: 1. Warning shown when runner changed from copilot to another value ✓ 2. Warning text: 'Warning: changing the runner from copilot may break this agent.' ✓ 3. Client-side JS fires before save (onchange on <select>) ✓ 4. No warning shown when copilot runner is unchanged ✓ 5. go build ./... and go test ./... pass ✓
📝 WalkthroughWalkthroughThe changes add support for a "Copilot" runner option in the agent detail template. When users select or switch away from the Copilot runner, a warning displays and associated model options (Claude, GPT-5.x, Gemini) populate automatically through updated client-side logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/templates/partials.html (1)
105-114: Centralize runner/model definitions to prevent driftThis adds another hardcoded model list in JS while canonical runner/model data already exists in
internal/models/models.go(RunnerModels). A future backend model update can desync the UI silently. Consider emitting runner→models from the server and consuming that map inupdateModels().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/templates/partials.html` around lines 105 - 114, The hardcoded model list in internal/templates/partials.html should be replaced by a single source of truth emitted from the server (use the existing RunnerModels in internal/models/models.go) so the UI cannot drift; change the server to serialize RunnerModels into the page (or expose an endpoint) and update the client-side updateModels() to read that emitted JSON map instead of the inline array, removing the hardcoded entries (claude-sonnet-4.6, gpt-5.4, etc.) and ensuring the runner→models mapping is produced from RunnerModels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/templates/agent_detail.html`:
- Around line 192-194: The initial render only calls
updateModels('edit-runner','edit-model') so the copilot warning logic isn't
evaluated until a change; update the DOMContentLoaded handler to also check the
select element(s) with data-original="copilot" (or call the same function that
the change handler uses) and run the copilot-warning display logic immediately
if the current select value differs from its data-original. Locate the
initialization in the DOMContentLoaded block and invoke the copilot check for
the 'edit-model' (and/or 'edit-runner') select(s) so the warning is shown on
first render when needed.
---
Nitpick comments:
In `@internal/templates/partials.html`:
- Around line 105-114: The hardcoded model list in
internal/templates/partials.html should be replaced by a single source of truth
emitted from the server (use the existing RunnerModels in
internal/models/models.go) so the UI cannot drift; change the server to
serialize RunnerModels into the page (or expose an endpoint) and update the
client-side updateModels() to read that emitted JSON map instead of the inline
array, removing the hardcoded entries (claude-sonnet-4.6, gpt-5.4, etc.) and
ensuring the runner→models mapping is produced from RunnerModels.
🪄 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: 45596999-a85a-486a-886f-06dc7c27e52c
📒 Files selected for processing (2)
internal/templates/agent_detail.htmlinternal/templates/partials.html
| document.addEventListener('DOMContentLoaded', () => { | ||
| updateModels('edit-runner', 'edit-model'); | ||
| }); |
There was a problem hiding this comment.
Initialize copilot warning on first render
At Line 193 only models are initialized. If data-original="copilot" and the current select value is already different on load, the warning remains hidden until a new change event.
Suggested fix
document.addEventListener('DOMContentLoaded', () => {
updateModels('edit-runner', 'edit-model');
+ checkCopilotRunnerWarning('edit-runner', 'copilot-runner-warning');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| document.addEventListener('DOMContentLoaded', () => { | |
| updateModels('edit-runner', 'edit-model'); | |
| }); | |
| document.addEventListener('DOMContentLoaded', () => { | |
| updateModels('edit-runner', 'edit-model'); | |
| checkCopilotRunnerWarning('edit-runner', 'copilot-runner-warning'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/templates/agent_detail.html` around lines 192 - 194, The initial
render only calls updateModels('edit-runner','edit-model') so the copilot
warning logic isn't evaluated until a change; update the DOMContentLoaded
handler to also check the select element(s) with data-original="copilot" (or
call the same function that the change handler uses) and run the copilot-warning
display logic immediately if the current select value differs from its
data-original. Locate the initialization in the DOMContentLoaded block and
invoke the copilot check for the 'edit-model' (and/or 'edit-runner') select(s)
so the warning is shown on first render when needed.
![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 introduces support for a new "copilot" runner, adding it as an option in the agent detail page and defining its associated models in the updateModels JavaScript function. It also implements a warning mechanism that displays if an agent originally configured with the "copilot" runner is changed to another runner. The review highlights that the model list for the "copilot" runner is hardcoded in the frontend, duplicating backend logic, and suggests fetching this dynamically for consistency. Additionally, it recommends refactoring the onchange event handler on the runner select element to use programmatic event listeners for improved maintainability.
| } else if (runner === 'copilot') { | ||
| models = [ | ||
| {val: 'claude-sonnet-4.6', label: 'Claude Sonnet 4.6'}, | ||
| {val: 'claude-opus-4.6', label: 'Claude Opus 4.6'}, | ||
| {val: 'claude-haiku-4.5', label: 'Claude Haiku 4.5'}, | ||
| {val: 'gpt-5.4', label: 'GPT-5.4'}, | ||
| {val: 'gpt-5.1', label: 'GPT-5.1'}, | ||
| {val: 'gpt-5-mini', label: 'GPT-5 Mini'}, | ||
| {val: 'gemini-2.5-pro', label: 'Gemini 2.5 Pro'} | ||
| ]; |
There was a problem hiding this comment.
The list of models for the 'copilot' runner is hardcoded in this JavaScript function and duplicates the definition in internal/models/models.go. To ensure consistency and easier updates, the frontend should ideally fetch the available models dynamically from the backend (which should be the single source of truth) rather than hardcoding them. This prevents potential discrepancies between the backend and frontend if model lists change.
| <div> | ||
| <label class="text-[11px] text-ink3/50">Runner</label> | ||
| <select id="edit-runner" name="runner" onchange="updateModels('edit-runner', 'edit-model')" class="w-full bg-sf border border-bd rounded-md px-2.5 py-1.5 text-xs text-ink outline-none focus:ring-1 focus:ring-ac"> | ||
| <select id="edit-runner" name="runner" data-original="{{.Agent.Runner}}" onchange="updateModels('edit-runner', 'edit-model'); checkCopilotRunnerWarning('edit-runner', 'copilot-runner-warning');" class="w-full bg-sf border border-bd rounded-md px-2.5 py-1.5 text-xs text-ink outline-none focus:ring-1 focus:ring-ac"> |
There was a problem hiding this comment.
The onchange attribute on the <select> element is becoming quite long and handles multiple concerns. For better separation of concerns and maintainability, consider attaching event listeners programmatically in JavaScript rather than using inline onchange attributes. This makes the HTML cleaner and the JavaScript easier to manage and test.
| <select id="edit-runner" name="runner" data-original="{{.Agent.Runner}}" onchange="updateModels('edit-runner', 'edit-model'); checkCopilotRunnerWarning('edit-runner', 'copilot-runner-warning');" class="w-full bg-sf border border-bd rounded-md px-2.5 py-1.5 text-xs text-ink outline-none focus:ring-1 focus:ring-ac"> | |
| <select id="edit-runner" name="runner" data-original="{{.Agent.Runner}}" class="w-full bg-sf border border-bd rounded-md px-2.5 py-1.5 text-xs text-ink outline-none focus:ring-1 focus:ring-ac"> |
1 participant
Summary
Fixes the silent data-loss bug where editing a copilot-runner agent would overwrite the runner value without any warning.
Changes
internal/templates/agent_detail.htmlcopilotas an explicit<option>in the runner<select>(selected when current runner is copilot)data-originalattribute to capture the original value at page loadonchangeto also callcheckCopilotRunnerWarning()#copilot-runner-warningshown when user selects a different runnercheckCopilotRunnerWarning()JS functioninternal/templates/partials.htmlcopilotbranch toupdateModels()with all 7 copilot models frommodels.goAcceptance Criteria
Closes SO-22