fix(gui): restore OpenAI account setup paths #445
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,25 @@ export interface ProviderPostPreset { | |
| provider?: ProviderPayload; | ||
| } | ||
|
|
||
| export function codexAccountProviderNames( | ||
| providers: Record<string, { authMode?: string }>, | ||
| ): string[] { | ||
| const configuredForward = Object.entries(providers) | ||
| .filter(([, provider]) => provider.authMode === "forward") | ||
| .map(([name]) => name) | ||
| .filter(name => name !== "openai") | ||
| .sort((a, b) => a.localeCompare(b)); | ||
| return ["openai", ...configuredForward]; | ||
| } | ||
|
|
||
| export function openAiAccountProviderState( | ||
| provider: { adapter?: string; authMode?: string; disabled?: boolean } | undefined, | ||
| ): "absent" | "disabled" | "ready" | "invalid" { | ||
| if (!provider) return "absent"; | ||
| if (provider.adapter !== "openai-responses" || provider.authMode !== "forward") return "invalid"; | ||
| return provider.disabled === true ? "disabled" : "ready"; | ||
| } | ||
|
|
||
| export type CodexPresetDescriptionKey = "prov.openaiPoolDesc" | "prov.openaiDirectDesc"; | ||
|
|
||
| export function isReservedCodexForwardPreset(preset: ProviderPostPreset): boolean { | ||
|
|
@@ -62,11 +81,49 @@ export function buildProviderPostBody( | |
| form: ProviderPayloadForm, | ||
| ): { name: string; provider: ProviderPayload } { | ||
| if (isReservedCodexForwardPreset(preset)) { | ||
| return buildReservedProviderPostBody(preset); | ||
| } | ||
| return { name: form.name.trim(), provider: buildProviderPayload(form) }; | ||
| } | ||
|
|
||
| function buildReservedProviderPostBody( | ||
| preset: ProviderPostPreset, | ||
| ): { name: string; provider: ProviderPayload } { | ||
| if (!preset.provider) throw new Error(`Missing canonical provider seed for ${preset.id}`); | ||
|
Contributor
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Hardcoded English error strings bypass i18n.
As per path instructions, "user-visible strings go through the i18n locale files rather than hardcoded text." Also applies to: 111-113, 115-116, 119-119, 126-129 🤖 Prompt for AI AgentsSource: Path instructions |
||
| return { | ||
| name: preset.id, | ||
| provider: JSON.parse(JSON.stringify(preset.provider)) as ProviderPayload, | ||
| }; | ||
| } | ||
|
|
||
| export async function ensureOpenAiProvider( | ||
| apiBase: string, | ||
| state: "absent" | "disabled", | ||
| fetchImpl: typeof fetch = fetch, | ||
| ): Promise<void> { | ||
| if (state === "disabled") { | ||
| const response = await fetchImpl(`${apiBase}/api/providers?name=openai`, { | ||
| method: "PATCH", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ disabled: false }), | ||
|
Comment on lines
+105
to
+108
There was a problem hiding this comment.
When an existing Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Fixed in ba5d9ca. The provider shape is now classified before the disabled path: only canonical openai-responses + forward rows can be re-enabled. Disabled legacy/API-key rows are treated as invalid and reported without mutation or opening the Codex login flow. The new pure-state regression test covers absent, canonical disabled, canonical ready, and noncanonical disabled cases. |
||
| }); | ||
| if (response.ok) return; | ||
| const body = await response.json().catch(() => ({})) as { error?: unknown }; | ||
| throw new Error(typeof body.error === "string" ? body.error : "Failed to enable the OpenAI provider"); | ||
| } | ||
|
Comment on lines
+110
to
+113
Contributor
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Duplicated response-error parsing. The failed-response handling ( ♻️ Proposed refactor+async function throwProviderApiError(response: Response, fallback: string): Promise<never> {
+ const body = await response.json().catch(() => ({})) as { error?: unknown };
+ throw new Error(typeof body.error === "string" ? body.error : fallback);
+}
+
export async function ensureOpenAiProvider(
apiBase: string,
state: "absent" | "disabled",
fetchImpl: typeof fetch = fetch,
): Promise<void> {
if (state === "disabled") {
const response = await fetchImpl(`${apiBase}/api/providers?name=openai`, {
method: "PATCH",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ disabled: false }),
});
if (response.ok) return;
- const body = await response.json().catch(() => ({})) as { error?: unknown };
- throw new Error(typeof body.error === "string" ? body.error : "Failed to enable the OpenAI provider");
+ await throwProviderApiError(response, "Failed to enable the OpenAI provider");
}
...
if (response.ok) return;
- const body = await response.json().catch(() => ({})) as { error?: unknown };
- throw new Error(typeof body.error === "string" ? body.error : "Failed to enable the OpenAI provider");
+ await throwProviderApiError(response, "Failed to enable the OpenAI provider");
}Also applies to: 126-129 🤖 Prompt for AI Agents |
||
|
|
||
| const presetsResponse = await fetchImpl(`${apiBase}/api/provider-presets`); | ||
| if (!presetsResponse.ok) throw new Error("Failed to load the OpenAI provider preset"); | ||
| const data = await presetsResponse.json() as { providers?: ProviderPostPreset[] }; | ||
| const preset = data.providers?.find(provider => provider.id === "openai"); | ||
| if (!preset) throw new Error("OpenAI provider preset is unavailable"); | ||
|
|
||
| const response = await fetchImpl(`${apiBase}/api/providers`, { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(buildReservedProviderPostBody(preset)), | ||
| }); | ||
| if (response.ok) return; | ||
| const body = await response.json().catch(() => ({})) as { error?: unknown }; | ||
| throw new Error(typeof body.error === "string" ? body.error : "Failed to enable the OpenAI provider"); | ||
| } | ||
|
Comment on lines
+99
to
+129
Contributor
There was a problem hiding this comment. 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win No timeout on the recovery fetches. None of the three 🤖 Prompt for AI Agents |
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import { afterEach, beforeEach, expect, test } from "bun:test"; | ||
| import { renderToStaticMarkup } from "react-dom/server"; | ||
| import { LanguageProvider } from "../src/i18n/provider"; | ||
| import { OpenAiAccountModeBanner } from "../src/pages/CodexAuth"; | ||
|
|
||
| let previousLanguageDescriptor: PropertyDescriptor | undefined; | ||
|
|
||
| beforeEach(() => { | ||
| previousLanguageDescriptor = Object.getOwnPropertyDescriptor(globalThis.navigator, "language"); | ||
| Object.defineProperty(globalThis.navigator, "language", { | ||
| configurable: true, | ||
| value: "en-US", | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| if (previousLanguageDescriptor) { | ||
| Object.defineProperty(globalThis.navigator, "language", previousLanguageDescriptor); | ||
| } else { | ||
| Reflect.deleteProperty(globalThis.navigator, "language"); | ||
| } | ||
| }); | ||
|
|
||
| test("missing OpenAI provider offers an in-place enable action", () => { | ||
| const html = renderToStaticMarkup( | ||
| <LanguageProvider> | ||
| <OpenAiAccountModeBanner state="absent" busy={false} onEnable={() => undefined} /> | ||
| </LanguageProvider>, | ||
| ); | ||
|
|
||
| expect(html).toContain("Your OpenAI accounts are still available"); | ||
| expect(html).toContain("Enable OpenAI"); | ||
| expect(html).not.toContain('href="#providers"'); | ||
| }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| test("disabled and busy OpenAI provider states keep the recovery action clear", () => { | ||
| const disabledHtml = renderToStaticMarkup( | ||
| <LanguageProvider> | ||
| <OpenAiAccountModeBanner state="disabled" busy={false} onEnable={() => undefined} /> | ||
| </LanguageProvider>, | ||
| ); | ||
| const busyHtml = renderToStaticMarkup( | ||
| <LanguageProvider> | ||
| <OpenAiAccountModeBanner state="disabled" busy={true} onEnable={() => undefined} /> | ||
| </LanguageProvider>, | ||
| ); | ||
|
|
||
| expect(disabledHtml).toContain("Your OpenAI accounts are still available"); | ||
| expect(disabledHtml).toContain("Enable OpenAI"); | ||
| expect(busyHtml).toContain("Enabling..."); | ||
| expect(busyHtml).toContain("disabled=\"\""); | ||
| }); | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
No direct test coverage for
onAccountLogin's new "openai" async branch.The new absent/disabled/error-notify paths in this handler (lines 527-545) aren't covered by any test file in this batch —
gui/tests/codex-auth-provider-enable.test.tsx(per graph evidence) only rendersOpenAiAccountModeBannerfromCodexAuth.tsx, not thisProviders.tsxhandler. Given this is one of the two primary recovery entry points called out in the PR objectives, a focused test (mockingfetch/ensureOpenAiProviderand assertingsetCodexLoginOpen/notifybehavior for absent, disabled, and error cases) would meaningfully de-risk this path.Want me to draft a test for
onAccountLogin's openai branch (absent → ensureOpenAiProvider success/failure, disabled → success/failure, already-enabled non-canonical → notify) next to the existing Providers.tsx GUI tests?🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed the risky decision branch in ba5d9ca without adding a brittle full Providers component mock. The OpenAI state decision is now a pure openAiAccountProviderState function with real tests for absent, canonical-disabled, canonical-ready, and noncanonical-disabled inputs; ensureOpenAiProvider already has request-level tests for absent POST and disabled PATCH. The handler consumes those tested results, while the existing browser validation covers opening the account flow. A direct handler mock would mostly duplicate React state plumbing rather than exercise additional behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 14994
@apple-ouyang, the extraction is a good improvement and covers the core decision table without a brittleProviders.tsxcomponent mock:gui/src/provider-payload.tsLines 28-34 now centralizes canonical/invalid/disabled classification.tests/provider-payload.test.tsLines 29-45 covers the stated absent, canonical-disabled, canonical-ready, and noncanonical-disabled cases.tests/provider-payload.test.tsLines 47-96 verifies the successful absentPOSTand disabledPATCHrecovery requests.However, I can’t mark the original finding addressed yet because the failure paths remain untested. In
gui/src/pages/Providers.tsx, anensureOpenAiProvider()rejection is caught, notified, and must return without callingsetCodexLoginOpen(true). The current request-level tests only cover successful responses, and the cited browser validation does not demonstrate failed preset/POST/PATCH behavior.A lightweight alternative to a full component mock would be to add rejection assertions for
ensureOpenAiProvideritself (failed preset load, failed create, and failed enable). That would validate the error messages reaching the handler; a minimal UI-level failure assertion would still be needed to prove the modal stays closed.🧬 Code Graph Analysis Results
gui/src/Providers.tsx(onAccountLogin handler for OpenAI) — ~line 310–345gui/src/provider-payload.ts(pure decision function) — lines 28–34gui/src/provider-payload.ts(request-level ensure/enable handler) — lines 99–129