fix(ai-settings): test a Claude Code route under its own slug - #5993
fix(ai-settings): test a Claude Code route under its own slug#5993Guykaganovsky1 wants to merge 2 commits into
Conversation
The custom-routing dialog built its test-call provider string as `ollama:<model>` for every non-cloud source, so pressing Test on a Claude Code route asked Ollama for a model it has never heard of — `ollama:claude-fable-5-1` — while the failure banner named claude-code as the provider that rejected it. `registrySlug`, three lines above, already mapped the three source kinds correctly (cloud → its slug, local → ollama, claude-code → claude-code). The test string now reuses it, so the call names the same slug the save persists. Local routes are unaffected: `registrySlug` yields `ollama` for them, exactly as before. Adds a regression test covering both the claude-code and the cloud case.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
How this change flows1 changed behaviour across 10 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 39 further behaviours left out to keep the diagram readable. flowchart LR
n0["CustomRoutingDialog<br/>changed"]:::changed
n1["ProviderModelPickerDialog"]:::impacted
n2["handleTest"]:::impacted
n3["GlobalOwnModelSelector"]:::impacted
n4["CustomDialogSource"]:::impacted
n5["AIPanel"]:::impacted
n6["currentProviderString"]:::impacted
n0 -->|uses| n1
n0 -->|calls| n2
n0 -->|uses| n4
n0 -->|uses| n6
n1 -->|uses| n4
n2 -->|uses| n6
n3 -->|uses| n1
n3 -->|uses| n4
n5 -->|uses| n0
n5 -->|uses| n3
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthrough
ChangesProvider routing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The Test action now uses the provider registry slug so Claude Code routes are tested through the correct provider, with regression coverage for Claude Code and cloud routes. No concrete merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
Summary
ollama:<model>instead ofclaude-code:<model>.ollama:claude-fable-5-1), while the failure banner named claude-code as the provider that rejected it — pointing the user at the wrong thing to fix.registrySlug, which already mapped all three source kinds correctly, so the test call names the same slug the save persists.Problem
CustomRoutingDialogderived two slugs independently.registrySlug(used for the per-model vision flag) handled all three kinds — cloud → its own slug, local →ollama, claude-code →claude-code.currentProviderString(used for the test call) had only a cloud branch and an else, and the else assumed local:A saved Claude Code route is fine —
handleSavehas a properclaude-codebranch — so the route works while its own Test button reports it as broken. That is the worst shape for this bug: the diagnostic tool is the only thing lying.Solution
Derive the test string from
registrySlug, the mapping that was already correct, and bail out when it isnull(the managed source, which has nothing to test against). Local routes are unchanged:registrySlugyieldsollamafor them exactly as the old else-branch did.Submission Checklist
app/src/components/settings/panels/ai/__tests__/CustomRoutingDialog.test.tsx: the claude-code case (the regression) and the cloud case (the path that must not change).N/A: bug fix to existing behaviour, no feature row added, removed or renamed## Related—N/A: no matrix feature row covers this dialog's test buttontestProviderModel; no network.N/A: no release-cut surface changesCloses #NNN—N/A: no filed issue; found while wiring the Claude Code CLI providerImpact
Desktop UI only, one expression in one dialog. No runtime, RPC, config or persistence change; nothing about how a route is saved or resolved moves. The user-visible effect is that Test now exercises the route the user selected.
Related
N/A — no linked issue.
Summary by CodeRabbit
Bug Fixes
Tests