fix(providers): manage default provider safely - #810
Merged
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:
📝 WalkthroughWalkthroughThe provider management API now supports default-provider selection, reassignment during deletion, and protection against deleting the final enabled provider. The GUI exposes these actions, localizes related errors, updates confirmation dialogs, documents the behavior, and adjusts rail hover styling. ChangesProvider default management
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderDetails
participant Providers
participant useProvidersCrud
participant providerRoutes
participant ProviderConfig
ProviderDetails->>Providers: Select enabled non-default provider
Providers->>useProvidersCrud: setDefaultProvider(name)
useProvidersCrud->>providerRoutes: PATCH with setDefault: true
providerRoutes->>ProviderConfig: Persist selected default
ProviderConfig-->>providerRoutes: Return updated default
providerRoutes-->>useProvidersCrud: Return success or structured error
useProvidersCrud-->>Providers: Refresh configuration and notify result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@gui/src/components/provider-workspace/types.ts`:
- Around line 89-90: Update ProviderUpdatePatch to use a discriminated union
with separate editable and standalone setDefault variants. Define the
setDefault: true variant with all other patch fields marked never, and define
the editable variant without setDefault and without apiKey. Preserve the
management route contract so setDefault requests cannot include additional
fields.
In `@gui/src/i18n/ko.ts`:
- Around line 313-316: Update the Korean locale entries prov.removedDefault,
prov.removeLastProvider, and prov.removeHasDependentCombos to use the existing
“프로바이더” terminology instead of “제공자”, including the additional affected entry,
while preserving the messages’ meaning and placeholders.
In `@gui/src/pages/Providers.tsx`:
- Line 347: Update the default-provider removal flow around
removeDefaultProvider to choose the first enabled remaining provider using fresh
server data, matching the server’s disabled-provider filtering rule rather than
relying on stale config.providers ordering. Reconcile the selected UI state with
the DELETE response, and add coverage for stale ordering and a disabled first
remaining provider.
In `@gui/tests/rail-hover-delete.test.ts`:
- Around line 80-83: Update the hover-style assertions in
rail-hover-delete.test.ts to use the existing rule() helper and verify that the
.pws-rail-row-wrap:hover .providers-workspace-rail-trail rule specifically
contains opacity: 0;. Do not use a stylesheet-wide opacity assertion that can
match unrelated declarations.
In `@src/server/management/provider-routes.ts`:
- Around line 437-466: Update the fallback selection in the provider deletion
flow around fallbackDefault to choose the first remaining provider whose
disabled flag is false or absent, matching the existing disabled-provider guard.
Reject deletion with a 409 conflict and the localized last-provider error code
when no enabled fallback remains, and add an integration test covering deletion
of the enabled provider while only a disabled provider remains.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fe37f463-adf4-426c-96ec-936d72840085
📒 Files selected for processing (16)
gui/src/components/provider-workspace/ProviderDetails.tsxgui/src/components/provider-workspace/ProviderDialogs.tsxgui/src/components/provider-workspace/types.tsgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Providers.tsxgui/src/pages/providers-page-modals.tsxgui/src/pages/use-providers-crud.tsgui/src/styles/provider-workspace-shell.cssgui/tests/rail-hover-delete.test.tssrc/server/management/provider-routes.tstests/management-provider-validation.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cf32f5689
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
DELETE of the current default now picks the first remaining enabled provider (or 409), matching setDefault/disable guards. Restore actionable API errors in the GUI and document the dashboard set-default flow.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/i18n/zh.ts (1)
1322-1322: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the existing provider term in the workspace.
Nearby
pws.*strings use提供商, but this confirmation uses提供方twice. Replace both occurrences with提供商so the provider workspace uses one term.🤖 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 `@gui/src/i18n/zh.ts` at line 1322, Update the pws.removeDefaultConfirmBody translation in zh.ts by replacing both occurrences of 提供方 with 提供商, preserving the existing placeholders and message structure.
🤖 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 `@docs-site/src/content/docs/guides/web-dashboard.md`:
- Line 43: Update all eight documented sites to state that only enabled
providers may be selected as the default, and that deletion is refused when no
enabled replacement exists. In the English API sections, explicitly document the
enabled-only PATCH rule and the 409 response with code "last_provider", while
adding the equivalent dashboard and API behavior in Japanese, Korean, and
Russian at docs-site/src/content/docs/guides/web-dashboard.md:43-43 and 132-132,
docs-site/src/content/docs/ja/guides/web-dashboard.md:35-35 and 108-108,
docs-site/src/content/docs/ko/guides/web-dashboard.md:35-35 and 110-110, and
docs-site/src/content/docs/ru/guides/web-dashboard.md:35-35 and 114-114.
In `@docs-site/src/content/docs/zh-cn/guides/web-dashboard.md`:
- Line 34: Update both provider-management entries in
docs-site/src/content/docs/zh-cn/guides/web-dashboard.md at lines 34 and 102:
document that deleting the current default reassigns it to the first remaining
enabled provider, but deletion is refused and the current default is preserved
when no enabled replacement exists.
In `@tests/management-provider-validation.test.ts`:
- Around line 1003-1006: Remove the duplicate providers member from the response
type literal used by the saved assignment in the management-provider validation
test, keeping a single providers declaration with its existing Record<string, {
disabled?: boolean }> shape.
---
Outside diff comments:
In `@gui/src/i18n/zh.ts`:
- Line 1322: Update the pws.removeDefaultConfirmBody translation in zh.ts by
replacing both occurrences of 提供方 with 提供商, preserving the existing placeholders
and message structure.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2f56b568-7f11-4ee2-99f7-e002524c3c65
📒 Files selected for processing (16)
docs-site/src/content/docs/guides/web-dashboard.mddocs-site/src/content/docs/ja/guides/web-dashboard.mddocs-site/src/content/docs/ko/guides/web-dashboard.mddocs-site/src/content/docs/ru/guides/web-dashboard.mddocs-site/src/content/docs/zh-cn/guides/web-dashboard.mdgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Providers.tsxgui/src/pages/use-providers-crud.tsgui/tests/rail-hover-delete.test.tssrc/server/management/provider-routes.tstests/management-provider-validation.test.ts
Clarify that set-default requires an enabled provider and that deleting the default is refused when no enabled replacement remains.
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 `@docs-site/src/content/docs/guides/web-dashboard.md`:
- Line 132: Document the actual distinction between provider mutation paths: in
docs-site/src/content/docs/guides/web-dashboard.md:132-132,
docs-site/src/content/docs/ja/guides/web-dashboard.md:108-108,
docs-site/src/content/docs/ko/guides/web-dashboard.md:110-110,
docs-site/src/content/docs/ru/guides/web-dashboard.md:114-114, and
docs-site/src/content/docs/zh-cn/guides/web-dashboard.md:102-102, explain that
POST accepts setDefault alongside provider creation/replacement and PATCH uses
it as a standalone action, explicitly distinguishing enabled and disabled
provider behavior according to provider-routes.ts.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30bb4c73-0b70-4095-accf-fa1f24ff7245
📒 Files selected for processing (5)
docs-site/src/content/docs/guides/web-dashboard.mddocs-site/src/content/docs/ja/guides/web-dashboard.mddocs-site/src/content/docs/ko/guides/web-dashboard.mddocs-site/src/content/docs/ru/guides/web-dashboard.mddocs-site/src/content/docs/zh-cn/guides/web-dashboard.md
Match PATCH: refuse creating/replacing a disabled provider as default before mutating config, and document POST vs PATCH setDefault in the dashboard API table.
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 `@src/server/management/provider-routes.ts`:
- Around line 121-123: Update the provider creation validation and
default-assignment checks to use strict boolean validation: require
body.setDefault === true rather than relying on truthiness, matching the PATCH
contract checks and rejecting values such as the string "false".
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6dabcf12-f0ba-4bcd-98a8-392ec51a9035
📒 Files selected for processing (7)
docs-site/src/content/docs/guides/web-dashboard.mddocs-site/src/content/docs/ja/guides/web-dashboard.mddocs-site/src/content/docs/ko/guides/web-dashboard.mddocs-site/src/content/docs/ru/guides/web-dashboard.mddocs-site/src/content/docs/zh-cn/guides/web-dashboard.mdsrc/server/management/provider-routes.tstests/management-provider-validation.test.ts
Reject non-boolean setDefault values and only promote when the flag is exactly true, matching the PATCH contract.
Collaborator
[shipping-github] Verdict: approve-commentPR: UsefulnessReal dashboard gap: set an enabled provider as default, and delete the current default without a broken config. Fixes the prior “cannot delete default” / missing set-default UX, with six-locale notices and a rail hover polish so the delete control does not crop the default star. Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
Gatenone Bottom lineShip. Default-provider set/delete is correct against the enabled-provider invariant, localized, documented, and CI-clean on tip. |
Collaborator
[shipping-github] Merge readyPR: Reviews
Tip freshness
Checks
Residualnone for this PR. Optional later: rename Ready to merge. |
Cold Bun starts of `provider --help` exceeded the 5s default on contended windows-latest (status null). Use SPAWN_BUDGET_MS for the suite and give spawnSync a matching deadline.
Collaborator
|
Thanks @eachann1024 — merging this. Why it helps: the Providers dashboard can now set an enabled provider as the default and delete the current default without leaving routing broken — Ship it. |
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.
Summary
flowchart LR A["Set provider as default"] --> B["PATCH /api/providers?setDefault"] B --> C["Persist defaultProvider"] D["Delete current default"] --> E{"Another provider exists?"} E -- Yes --> F["Switch to first remaining provider"] --> G["Delete provider"] E -- No --> H["Keep config valid and show localized error"]Validation
bun test tests/management-provider-validation.test.ts -t "switches the default"bun test ./gui/tests/use-providers-crud-update.test.tsx ./gui/tests/rail-hover-delete.test.ts ./gui/tests/rail-hover-delete-dom.test.tsxbun run lint/bun run lint:i18n/bun run buildingui/bun run typecheckbun run privacy:scanNotes
Summary by CodeRabbit