fix(gui): add Pool/Direct account mode switch to OpenAI provider settings - #935
Conversation
…ings The Providers workspace Settings tab for the canonical openai provider now renders a Pool/Direct selector backed by the existing dedicated PATCH (/api/providers?name=openai with codexAccountMode alone). The control applies immediately, shows per-mode descriptions and save state, and refreshes the config so the badge and banner stay in sync.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe provider settings UI lets eligible canonical OpenAI providers switch between ChangesOpenAI account-mode settings
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderSettings
participant useProvidersCrud
participant ProviderAPI
participant codexPool
ProviderSettings->>useProvidersCrud: save codexAccountMode
useProvidersCrud->>ProviderAPI: PATCH openai provider
ProviderAPI-->>useProvidersCrud: return update result
useProvidersCrud->>codexPool: refresh account and quota state
codexPool-->>ProviderSettings: return refresh result
ProviderSettings-->>ProviderSettings: display localized status
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 |
![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
🤖 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/ProviderSettings.tsx`:
- Around line 169-181: Update applyAccountMode so that after onUpdateProvider
confirms the account-mode change succeeded, it updates accountMode from the
confirmed response value (next). Leave the failure path unchanged, ensuring the
controlled select and the guard use the newly saved mode without waiting for
item prop synchronization.
- Around line 67-70: Update the provider settings reset effect around
setMsg(null) to also call setModeMsg(null), clearing stale account-mode feedback
whenever the saved provider fields change. Keep the existing reset behavior and
dependency list unchanged.
🪄 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: 1f1e050b-f66d-45ba-989d-e35a2c42b67c
📒 Files selected for processing (10)
gui/src/components/provider-workspace/ProviderSettings.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/provider-workspace/catalog.tsgui/src/styles/provider-workspace-settings.css
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33c3d956bd
ℹ️ 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".
| setMsg(null); | ||
| queueMicrotask(() => setEndpointChoice(matchChoiceId(baseUrlChoices, item.baseUrl))); | ||
| }, [item.adapter, item.baseUrl, item.defaultModel, item.authMode, item.apiKeyTransport, item.keyOptional, item.note, item.allowPrivateNetwork, item.liveModels, baseUrlChoices]); | ||
| }, [item.adapter, item.baseUrl, item.defaultModel, item.authMode, item.apiKeyTransport, item.keyOptional, item.note, item.allowPrivateNetwork, item.liveModels, item.codexAccountMode, baseUrlChoices]); |
There was a problem hiding this comment.
When a user edits another field such as the note or default model and then switches Pool/Direct, onUpdateProvider refreshes the config and changes item.codexAccountMode; because that value is now a dependency of this reset effect, the effect reloads every form field from item and silently discards the unsaved edits. The same loss occurs for edits made while the mode PATCH is in flight. Synchronize accountMode separately without resetting the main draft, or prevent mode changes while the form is dirty and lock the other fields during the request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[GD] Fixed in e9b370e0: item.codexAccountMode is out of the form-reset effect and syncs in its own effect, so the config refresh after a mode PATCH no longer wipes an unsaved draft. Covered by a test that edits the note, refreshes the item with the new mode, and asserts the draft and sticky bar survive.
| setModeSaving(true); | ||
| setModeMsg(null); | ||
| try { | ||
| const res = await onUpdateProvider("openai", { codexAccountMode: next }); |
There was a problem hiding this comment.
A successful mode switch only goes through useProvidersCrud.updateProvider, which refreshes /api/config but never bumps fetchProviderQuotas(true). This matters because fetchChatGptForwardQuota reports the active pooled account in Pool mode but the main account in Direct mode, while ProviderWorkspaceShell re-fetches quotas only when its explicit quota revision changes. Consequently, after switching modes, the Overview, Usage, and dashboard quota bars continue showing the previous account's quota until another quota-invalidating action or a page remount; route this mutation through the existing forced quota refresh path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[GD] Fixed in f26edc8e: after a successful codexAccountMode PATCH, updateProvider now awaits fetchProviderQuotas(true) and codexPool.load(true) (the shared Codex account controller) before reporting success. Tests assert both calls and that non-mode patches skip them.
| </span> | ||
| {modeSaving && <span className="muted text-label">{t("pws.accountSwitching")}</span>} | ||
| {modeMsg && ( | ||
| <span className={modeMsg.ok ? "pwi-settings-mode-msg pwi-settings-mode-msg--ok" : "pwi-settings-mode-msg pwi-settings-mode-msg--err"}> |
There was a problem hiding this comment.
When the PATCH fails, the controlled select returns to its previous value and the only explanation is this dynamically inserted visual span. Because it has neither an alert/status role nor an aria-live region, screen-reader users receive no indication that the requested mode was rejected; expose failures as role="alert" and successes/progress through an appropriate polite status region.
AGENTS.md reference: gui/AGENTS.md:L31-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[GD] Fixed in f26edc8e: the account-mode message now carries role="alert" on failure and role="status" on success, and the select is disabled during ordinary settings saves too. The failure path is asserted in the new test.
Set accountMode from the successful PATCH response instead of relying only on the config refresh, and keep the just-applied mode message through the item refresh while clearing it on unrelated provider-field updates.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ProviderSettings.tsx`:
- Around line 178-189: Update applyAccountMode to catch rejected
onUpdateProvider calls, set the localized account-mode failure message, and
reset modeSavedRef.current when the save does not succeed. Keep the existing
success handling and finally block intact so modeSaving is always cleared and
rejected promises are handled.
- Around line 53-54: Prevent regular provider saves from running while an
account-mode save is in progress: update the save flow around save(),
modeSaving, and updateProvider() so save() is disabled or returns without
starting another update during modeSaving. Ensure the account-mode PATCH and its
subsequent fetchConfig() refresh cannot be overwritten by a concurrently started
regular-save request.
🪄 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: fdb0247a-fd9f-472a-9aa4-01462171018d
📒 Files selected for processing (1)
gui/src/components/provider-workspace/ProviderSettings.tsx
Block the regular settings save while an account-mode save is in flight (and vice versa) so a stale config refresh cannot overwrite the freshly applied mode, and show the localized failure message when the mode PATCH rejects.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
gui/src/components/provider-workspace/ProviderSettings.tsx (2)
279-286: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDisable the selector during general settings saves.
applyAccountModereturns immediately whensavingis true at Line 175, but the selector is disabled only whenmodeSavingis true. A user can select another mode during a general save. The handler discards the event, so the controlled select remains on the previous value.Update the control to use
disabled={saving || modeSaving}.As per path instructions, GUI state changes must stay consistent with management API responses.
🤖 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/components/provider-workspace/ProviderSettings.tsx` around lines 279 - 286, Update the account-mode select in the isCanonicalOpenAi settings block to use saving || modeSaving for its disabled state, preventing changes while general or account-mode saves are active and keeping the controlled value consistent with applyAccountMode.Source: Path instructions
69-74: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve general settings while the account-mode refresh is active.
When
onUpdateProviderrefreshesitemafter the account-mode PATCH, this reset effect also restoresadapter,baseUrl,defaultModel,authMode,note,allowPrivateNetwork, andliveModels. Those controls remain editable whilemodeSavingis true, butsave()and the Save button are blocked. If the user edits a general setting during the mode request, the refresh can erase the edit and cleardirtybefore the edit is saved.Disable the general settings controls while
modeSaving, or preserve dirty local values when the refresh changes onlycodexAccountMode.As per path instructions, GUI state changes must stay consistent with management API responses.
Also applies to: 174-177
🤖 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/components/provider-workspace/ProviderSettings.tsx` around lines 69 - 74, Update the ProviderSettings reset effect and related general-setting controls so an account-mode refresh cannot overwrite unsaved edits: either disable those controls while modeSaving is true, or preserve dirty local values when only codexAccountMode changes. Keep local state synchronized with management API responses after normal provider updates, and ensure dirty edits are not cleared during the account-mode PATCH refresh.Source: Path instructions
♻️ Duplicate comments (1)
gui/src/components/provider-workspace/ProviderSettings.tsx (1)
178-189: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear
modeSavedRefbefore each account-mode request.A successful request sets
modeSavedRef.currenttotrueat Line 182. If a retry starts before the success refresh runs and then returnsok: falseor rejects, Lines 185-189 updatemodeMsgbut leave the ref set. The next unrelated item refresh can preserve that failure message as if it belonged to the successful save.Reset
modeSavedRef.currentbefore enteringtry, and set it totrueonly after a successful response.🤖 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/components/provider-workspace/ProviderSettings.tsx` around lines 178 - 189, Reset modeSavedRef.current to false before the try block in the account-mode update handler. Keep setting it to true only after onUpdateProvider returns a successful response, so failed or rejected retries cannot retain the previous success state.Source: Path instructions
🤖 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.
Outside diff comments:
In `@gui/src/components/provider-workspace/ProviderSettings.tsx`:
- Around line 279-286: Update the account-mode select in the isCanonicalOpenAi
settings block to use saving || modeSaving for its disabled state, preventing
changes while general or account-mode saves are active and keeping the
controlled value consistent with applyAccountMode.
- Around line 69-74: Update the ProviderSettings reset effect and related
general-setting controls so an account-mode refresh cannot overwrite unsaved
edits: either disable those controls while modeSaving is true, or preserve dirty
local values when only codexAccountMode changes. Keep local state synchronized
with management API responses after normal provider updates, and ensure dirty
edits are not cleared during the account-mode PATCH refresh.
---
Duplicate comments:
In `@gui/src/components/provider-workspace/ProviderSettings.tsx`:
- Around line 178-189: Reset modeSavedRef.current to false before the try block
in the account-mode update handler. Keep setting it to true only after
onUpdateProvider returns a successful response, so failed or rejected retries
cannot retain the previous success state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e7ef4d8-8f43-4ca5-923f-ad57d6eb5414
📒 Files selected for processing (1)
gui/src/components/provider-workspace/ProviderSettings.tsx
lidge-jun
left a comment
lidge-jun
left a comment
There was a problem hiding this comment.
The shape is right: one canonical surface (Provider Settings only, Codex Auth stays read-only), the dedicated PATCH /api/providers?name=openai with {codexAccountMode} alone rather than a whole-object round-trip, no optimistic mutation, and the select disabled during its own request. That is the hard part done correctly.
Two blockers, both about what happens around the switch rather than the switch itself.
Dependent state goes stale
gui/src/components/provider-workspace/ProviderSettings.tsx:174-190 refreshes only /api/config after a successful mode change. But the backend clears quota caches and thread affinity on that PATCH, so provider quotas and the shared Codex account controller keep showing pre-switch state until some unrelated poll or navigation happens.
The user switches Pool → Direct, sees it succeed, and the quota numbers next to it are describing the world before the switch. Give the mode action fetchProviderQuotas(true) and the shared Codex account refresh, await them, and assert those calls in a test.
No confirmation for a destructive change
:174-180 fires the PATCH the moment the selection changes. Switching modes clears thread affinity — active conversations get rebound — and changes how quota is accounted. The descriptions explain which accounts each mode uses, which is useful, but they do not say that flipping this will move running threads.
This wants an explicit confirmation naming the two consequences, then the PATCH. A select that silently reroutes live traffic on a single click is the kind of control people hit by accident while browsing settings.
Smaller
No focused test for the new interaction at all. Worth covering: exact PATCH body, confirm and cancel paths, the dependent refreshes above, failure rollback, and the concurrency disable.
The error is rendered but not announced — role="alert" or a live region would make the failure reachable for screen-reader users. And the select stays enabled during an ordinary settings save while its handler silently refuses the action; disabling it there matches what the code already intends.
Verified on 91883da: typecheck, GUI lint, production build, and seven adjacent GUI tests all pass.
This closes #586, which has been open a while — good to see it get a real control rather than another JSON-editing instruction. Fix the refresh and add the confirmation and I will merge it.
The Pool/Direct select now asks for confirmation before the mode PATCH, because switching rebinds running threads and changes quota accounting; a cancelled confirmation leaves the select snapped back. After a successful mode PATCH, provider quotas and the shared Codex account controller refresh before the action reports success, so both tabs never show pre-switch state. The select is disabled during ordinary settings saves too, and mode errors are announced via role=alert. Adds focused tests for the exact PATCH body, confirm/cancel paths, failure rollback, the dependent refreshes, and the concurrency disable.
The form-reset effect used to depend on item.codexAccountMode, so the config refresh that follows a successful mode PATCH wiped any in-progress draft (e.g. an edited note). Account mode now syncs in its own effect, and modeSavedRef is removed because the reset effect no longer runs on mode changes.
|
Addressed the review (
Validation: |
2 participants
Summary
codexAccountModeselector to the Providers workspace Settings tab for the canonical OpenAI (Codex login) provider, so the dashboard no longer requires the JSON editor to change the account mode.PATCH /api/providers?name=openaiendpoint withcodexAccountModealone, applies immediately, shows the description of the current mode, and refreshes the provider config so badges and the Codex Auth banner stay in sync.openaiforward provider; other providers keep the existing settings form unchanged.Validation
bun run typecheck— passbun run lint:gui— passbun run build:gui— passbun run privacy:scan— passbun run test— 7574 pass, 7 skip, 14 fail; the 14 failures are pre-existing Windows host-environment artifacts (symlink creation without Developer Mode, and a globally installedcodex.cmd), all in files untouched by this diff; upstreamdevCI is green on the same test setGET /api/providers), the per-mode hint updates, and the save message appears. New copy uses existingcodexAuth.*keys plus two newpws.accountMode*keys in all six locales.Review notes
codexAccountModewith other fields, satisfying the endpoint's mutual-exclusion rule.Fixes #586
Summary by CodeRabbit
New Features
Bug Fixes
Style
Localization