refactor(webview): canonicalize provider settings identifiers - #1143
refactor(webview): canonicalize provider settings identifiers#1143WebMad wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProvider settings now use centralized ChangesProvider identifier migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (1)
webview-ui/src/components/settings/providers/__tests__/Requesty.spec.tsx (1)
45-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the Requesty callback route.
Line 45 only checks that the URL contains
callback_url. Inwebview-ui/src/oauth/urls.ts, Lines 3-5 add this parameter for every provider. The test passes if the callback route uses another provider. Parse the URL and assert that the callback route ends in/${providerIdentifiers.requesty}.Proposed test update
- expect(screen.getByRole("link")).toHaveAttribute("href", expect.stringContaining("callback_url=")) + const callbackUrl = new URL(screen.getByRole("link").getAttribute("href") ?? "").searchParams.get( + "callback_url", + ) + expect(callbackUrl ?? "").toMatch(new RegExp(`/${providerIdentifiers.requesty}$`))🤖 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 `@webview-ui/src/components/settings/providers/__tests__/Requesty.spec.tsx` at line 45, Strengthen the link assertion in the Requesty test by parsing the generated href and verifying its callback route ends with `/${providerIdentifiers.requesty}`. Keep the existing `callback_url` check if useful, but ensure the assertion specifically confirms Requesty rather than merely any provider callback.
🤖 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 `@webview-ui/src/components/settings/providers/LiteLLM.tsx`:
- Around line 64-65: Update the Save-handler invalidation calls using
queryClient.invalidateQueries for the LiteLLM and "all" routerModels keys so
their returned promises are explicitly handled; either await both invalidations
together or mark each intentional non-blocking call with void, preserving the
existing invalidation keys.
In `@webview-ui/src/components/settings/providers/OpenAICodex.tsx`:
- Line 3: Update the OpenAICodex component tests to remove calls to getByRole
for the deleted “Speed” combobox and instead assert that the
OpenAICodexSpeedSelector is absent in both affected tests. Leave the
service-tier compatibility tests in the API provider suite unchanged.
---
Nitpick comments:
In `@webview-ui/src/components/settings/providers/__tests__/Requesty.spec.tsx`:
- Line 45: Strengthen the link assertion in the Requesty test by parsing the
generated href and verifying its callback route ends with
`/${providerIdentifiers.requesty}`. Keep the existing `callback_url` check if
useful, but ensure the assertion specifically confirms Requesty rather than
merely any provider callback.
🪄 Autofix
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 Plus
Run ID: 5a7309f4-7f15-4c4c-886e-f0419ee0f25f
📒 Files selected for processing (17)
webview-ui/src/components/settings/providers/Kenari.tsxwebview-ui/src/components/settings/providers/KimiCode.tsxwebview-ui/src/components/settings/providers/LiteLLM.tsxwebview-ui/src/components/settings/providers/Moonshot.tsxwebview-ui/src/components/settings/providers/OpenAICodex.tsxwebview-ui/src/components/settings/providers/OpenCodeGo.tsxwebview-ui/src/components/settings/providers/Poe.tsxwebview-ui/src/components/settings/providers/Requesty.tsxwebview-ui/src/components/settings/providers/Unbound.tsxwebview-ui/src/components/settings/providers/VercelAiGateway.tsxwebview-ui/src/components/settings/providers/ZooGateway.tsxwebview-ui/src/components/settings/providers/__tests__/CanonicalProviderIdentifiers.spec.tsxwebview-ui/src/components/settings/providers/__tests__/KimiCode.spec.tsxwebview-ui/src/components/settings/providers/__tests__/LiteLLM.spec.tsxwebview-ui/src/components/settings/providers/__tests__/Moonshot.spec.tsxwebview-ui/src/components/settings/providers/__tests__/Poe.spec.tsxwebview-ui/src/components/settings/providers/__tests__/Requesty.spec.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
|
A note on the two LiteLLM invalidations: there are currently two independent React Query entries for router models. |
c6cac6d to
a38deb9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
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 `@webview-ui/src/components/settings/providers/LiteLLM.tsx`:
- Around line 61-62: Update the refresh handler in LiteLLM to invalidate both
the provider-scoped ["routerModels", providerIdentifiers.litellm] cache used by
useSelectedModel() and the shared ["routerModels", "all"] cache used by
ApiOptions, preserving explicit void handling. Extend LiteLLM.spec.tsx to assert
that both invalidations occur after a successful refresh.
🪄 Autofix
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 Plus
Run ID: 5820d26f-bf4b-48ff-b6c0-1e105545943a
📒 Files selected for processing (2)
webview-ui/src/components/settings/providers/LiteLLM.tsxwebview-ui/src/components/settings/providers/__tests__/LiteLLM.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- webview-ui/src/components/settings/providers/tests/LiteLLM.spec.tsx
| // Refresh the shared cache used by ApiOptions without invalidating unrelated queries. | ||
| void queryClient.invalidateQueries({ queryKey: ["routerModels", "all"] }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Invalidate both LiteLLM router-model caches.
LiteLLM uses two independent React Query entries. The provider-scoped entry ["routerModels", providerIdentifiers.litellm] serves useSelectedModel(). The shared entry ["routerModels", "all"] serves ApiOptions. This handler invalidates only the shared entry, so the selected-model path can retain stale models after a successful refresh.
Invalidate both keys. Keep the explicit void handling, and extend LiteLLM.spec.tsx to assert both invalidations.
Suggested fix
+ void queryClient.invalidateQueries({
+ queryKey: ["routerModels", providerIdentifiers.litellm],
+ })
void queryClient.invalidateQueries({ queryKey: ["routerModels", "all"] })📝 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.
| // Refresh the shared cache used by ApiOptions without invalidating unrelated queries. | |
| void queryClient.invalidateQueries({ queryKey: ["routerModels", "all"] }) | |
| // Refresh the shared cache used by ApiOptions without invalidating unrelated queries. | |
| void queryClient.invalidateQueries({ | |
| queryKey: ["routerModels", providerIdentifiers.litellm], | |
| }) | |
| void queryClient.invalidateQueries({ queryKey: ["routerModels", "all"] }) |
🤖 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 `@webview-ui/src/components/settings/providers/LiteLLM.tsx` around lines 61 -
62, Update the refresh handler in LiteLLM to invalidate both the provider-scoped
["routerModels", providerIdentifiers.litellm] cache used by useSelectedModel()
and the shared ["routerModels", "all"] cache used by ApiOptions, preserving
explicit void handling. Extend LiteLLM.spec.tsx to assert that both
invalidations occur after a successful refresh.
a55d21e to
a38deb9
Compare
1 participant
Summary
Extracts the provider-settings portion of #1141 into a focused pull request:
providerIdentifiersinwebview-ui/src/components/settings/providersSerialized provider values and runtime behavior remain unchanged.
Validation
pnpm check-typesinwebview-ui--prune-suppressions --max-warnings=0for all changed filesgit diff --checkRelated to #944. Extracted from #1141.
Summary by CodeRabbit
Bug Fixes
Tests