diff --git a/devlog/_plan/260825_oauth_login_ux/020_wp3_first_add_parity.md b/devlog/_plan/260825_oauth_login_ux/020_wp3_first_add_parity.md index f6103ae1278..f3f14c86d9f 100644 --- a/devlog/_plan/260825_oauth_login_ux/020_wp3_first_add_parity.md +++ b/devlog/_plan/260825_oauth_login_ux/020_wp3_first_add_parity.md @@ -30,23 +30,28 @@ That is the whole bug. It is not "the modal cannot show a link" — it is presets, usageRank, presetsLoading, initialTier, onSelectPreset, onSelectCustom, accountRows, accountStatus, busyProvider, -+ loginHint, -+ onSubmitLoginCode, ++ loginHint = null, ++ paste, onLogin, onCancelLogin, onLogout, onManage, }: { + /** Hint for the account row whose login is in flight; ignored for other rows. */ -+ loginHint?: { provider: string; url?: string; instructions?: string; deviceCode?: string } | null; -+ onSubmitLoginCode?: (provider: string, input: string) => Promise<{ ok: boolean; error?: string }>; ++ loginHint?: CatalogLoginHint | null; ++ /** Paste state, owned by the modal so the catalog stays presentational. */ ++ paste?: { ++ value: string; busy: boolean; message: string; ok: boolean; ++ onChange: (value: string) => void; ++ onSubmit: (provider: string) => void; ++ }; ``` -Inside the account-row map (`:148-212`), when -`busyProvider === row.id && loginHint?.provider === row.id`, render the WP2 -`` **below** the row rather than inside its badge strip — the -strip is a horizontal flex of buttons and a URL block belongs on its own line. +A controlled paste field needs its value and status, not just a submit +callback — an `onSubmitLoginCode` alone could not render one. The catalog +passes `row.id` back on submit so the modal posts to the right provider. -The row is currently a `
` with two children. It -becomes a wrapper with the row on top and the hint underneath, so the -non-busy layout is byte-identical. +Inside the account-row map (`:148-212`), render the WP2 `` +**below** the row rather than inside its badge strip: the strip is a +horizontal flex of buttons and a URL block belongs on its own line. See +section 5 for why that needs CSS, not just markup. ### 2. Paste state lives in the modal, not the catalog @@ -55,19 +60,79 @@ so). The paste value, busy flag, and message belong in `AddProviderModal`, which already owns exactly those fields in its reducer for the preset pane (`manualCode`, `manualCodeBusy`, `manualCodeMsg`, `manualCodeOk`). -`AddProviderModal` passes them down; the catalog renders them. One reducer, -two panes, no duplicated state. +Reusing them is sound because `/api/oauth/login/code` is **provider-keyed**: +the modal's `submitManualCode` can complete a login the *page* started. The +catalog receives a `paste` prop and calls `onSubmit(row.id)`. -### 3. `Providers.tsx` threads the hint into the modal +A caveat the first draft of this doc oversold: this is not "no duplicated +state". Hint state genuinely exists twice — page-level `loginInfo` for account +rows, reducer `oauthUrl`/`oauthDeviceCode` for the preset pane. The two panes +cannot be mounted at once (the catalog renders only while `preset === null`), +so they cannot disagree on screen, but the duplication is real and this doc +should not pretend otherwise. -`loginInfo` already lives in `Providers.tsx:39`. It is passed to -`ProvidersPageModals` alongside `addModalAccountRows` and -`accountLoginStatus`, which the modal already receives. One more prop. +### 3. The hint comes from the PAGE, not the modal's reducer + +This is the load-bearing fact of the phase, and getting it backwards would +produce a hint that is permanently empty. + +An Accounts-tab login does **not** run `useAddProviderOAuth`: + +``` +ProviderCatalog onLogin(row.id) + → AddProviderModal onAccountLogin + → ProvidersPageModals onAccountLogin + → Providers.onAccountLogin + → codex / forward rows: opens AddCodexAccountModal + → oauth rows: requestLoginOAuth → useProvidersOAuth.loginOAuth + → setLoginInfo({ provider, url, instructions, deviceCode }) +``` + +`useAddProviderOAuth` runs only from the preset OAuth pane, after a preset is +chosen. For an account row it never runs, and `set-oauth-url` no-ops anyway +while `preset` is null. So the hint must be threaded from `Providers.tsx`'s +`loginInfo` — through `ProvidersPageModals` **and** `AddProviderModal`, which +is a hop the first draft of this doc skipped — down to the catalog. ### 4. Cancel already works -`onCancelLogin` is wired at `ProviderCatalog.tsx:200-204` and hits -`cancelLoginFlow`. No change. +`onCancelLogin` is already wired on the account row and reaches the page's +`cancelLoginOAuth`, which POSTs `/api/oauth/login/cancel`. No change. + +### 5. The row has to grow a second line + +`.list-row` is `display: flex; align-items: center; justify-content: +space-between`, so a hint added as a third child lands on the **badge axis**, +beside the buttons. The row needs a head wrapper plus a waiting-state modifier: + +```css +.provider-catalog-account-row-head { display: flex; align-items: center; + justify-content: space-between; gap: 10px; width: 100%; } +.list-row.provider-catalog-account-row--waiting { flex-direction: column; + align-items: stretch; gap: 10px; cursor: default; } +``` + +The waiting modifier **must** be qualified with `.list-row`. This stylesheet +is `@import`ed at the top of `styles.css` while `.list-row` is declared far +below it, and the two selectors have the same specificity — so source order +decides, and the unqualified modifier silently loses `align-items` and +`cursor`. The hint then renders as a shrink-wrapped centered column instead of +a full-width second line, which looks *almost* right and is easy to miss in a +screenshot. + +The head wrapper is always present, so a non-waiting row is not *byte*-identical +markup — it is one extra div reproducing the same flex rules, and renders the +same. The earlier claim of byte-identical layout was wrong and is corrected +here. + +### 6. A Codex row must never show an OAuth hint + +`kind: "codex"` rows do not log in through `/api/oauth` at all: they open the +Codex account modal, and the page sets `busy = "openai"` while enabling the +OpenAI provider first. A provider-match check alone would let a stale +`loginInfo.provider === "openai"` paint an authorization URL onto a row whose +real flow is somewhere else, so the predicate excludes non-OAuth kinds +explicitly rather than relying on the ids never colliding. ## What this phase must not do @@ -81,15 +146,17 @@ two panes, no duplicated state. ## Test -`tests/oauth-first-add-hint.test.ts` (new): a pure-function test over the -row-visibility predicate extracted from the JSX, asserting that a hint renders -only for the row whose provider matches and only while that provider is busy. -Extract it as `shouldShowLoginHint(row, busyProvider, hint)` in -`provider-presets.ts` so it is testable without a DOM. +`tests/oauth-first-add-hint.test.ts` (new): a pure-function test over +`shouldShowLoginHint(row, busyProvider, hint)`, asserting that a hint renders +only for an OAuth row whose provider matches and only while that provider is +busy. + +The predicate lives in a new `provider-catalog/login-hint-visibility.ts`, not +in `provider-presets.ts` — that module is the preset DTO / tier / search owner +and declares itself free of React concerns; login chrome does not belong in it. -The cross-provider case is the one that matters: a hint for `anthropic` must -never render on the `xai` row, which is the same class of leak the -`oauthUrlProvider` tag guards against in the preset pane. +Two cases matter: a hint for `anthropic` must never render on the `xai` row, +and a `codex` row must show nothing even while the page is busy on it. ## Acceptance diff --git a/gui/src/components/AddProviderModal.tsx b/gui/src/components/AddProviderModal.tsx index d308c8da4c3..835a84b22bf 100644 --- a/gui/src/components/AddProviderModal.tsx +++ b/gui/src/components/AddProviderModal.tsx @@ -14,6 +14,7 @@ import OAuthTosWarningModal from "./OAuthTosWarningModal"; import ProviderCatalog from "./provider-catalog/ProviderCatalog"; import type { AccountLoginRow, AccountLoginStatus } from "./provider-catalog/ProviderCatalog"; import type { CatalogPreset } from "./provider-catalog/provider-presets"; +import type { CatalogLoginHint } from "./provider-catalog/login-hint-visibility"; import { baseUrlForChoice, matchChoiceId, resolvedBaseUrlForChoice } from "../base-url-choice"; import { AddProviderOAuthPane } from "./add-provider-oauth-pane"; import { AddProviderFormPane } from "./add-provider-form-pane"; @@ -29,7 +30,8 @@ type Preset = CatalogPreset; export default function AddProviderModal({ apiBase, existingNames, onClose, onAdded, initialTier, initialCustom = false, - accountRows, accountStatus, accountBusy, onAccountLogin, onAccountCancelLogin, onAccountLogout, onAccountManage, onOpen, + accountRows, accountStatus, accountBusy, accountLoginHint = null, + onAccountLogin, onAccountCancelLogin, onAccountLogout, onAccountManage, onOpen, }: { apiBase: string; existingNames: string[]; @@ -40,6 +42,8 @@ export default function AddProviderModal({ accountRows?: AccountLoginRow[]; accountStatus?: Record; accountBusy?: string | null; + /** Login hint for an Accounts-tab login in flight, owned by the providers page. */ + accountLoginHint?: CatalogLoginHint | null; onAccountLogin?: (provider: string, addAccount?: boolean) => void; onAccountCancelLogin?: (provider: string) => void; onAccountLogout?: (provider: string) => void; @@ -257,6 +261,15 @@ export default function AddProviderModal({ onCancelLogin={onAccountCancelLogin} onLogout={onAccountLogout} onManage={onAccountManage} + loginHint={accountLoginHint} + paste={{ + value: manualCode, + busy: manualCodeBusy, + message: manualCodeMsg, + ok: manualCodeOk, + onChange: code => dispatch({ type: "set-manual-code", code }), + onSubmit: providerId => { void submitManualCode(providerId); }, + }} /> ) : form && ( preset.auth === "oauth" && form.authMode === "oauth" ? ( diff --git a/gui/src/components/provider-catalog/ProviderCatalog.tsx b/gui/src/components/provider-catalog/ProviderCatalog.tsx index ab980b0eacf..14973d44f95 100644 --- a/gui/src/components/provider-catalog/ProviderCatalog.tsx +++ b/gui/src/components/provider-catalog/ProviderCatalog.tsx @@ -11,6 +11,8 @@ import { filterPresets, type CatalogPreset, } from "./provider-presets"; +import { shouldShowLoginHint, type CatalogLoginHint } from "./login-hint-visibility"; +import { LoginHint } from "../login-url-block"; export type AccountLoginStatus = { loggedIn: boolean; email?: string; error?: string; needsReauth?: boolean }; export type AccountLoginRow = { @@ -38,6 +40,8 @@ export default function ProviderCatalog({ accountRows = EMPTY_ACCOUNT_ROWS, accountStatus = EMPTY_ACCOUNT_STATUS, busyProvider = null, + loginHint = null, + paste, onLogin, onCancelLogin, onLogout, @@ -53,6 +57,17 @@ export default function ProviderCatalog({ accountRows?: AccountLoginRow[]; accountStatus?: Record; busyProvider?: string | null; + /** Authorization URL / device code for the account-row login in flight. */ + loginHint?: CatalogLoginHint | null; + /** Paste-a-redirect-or-code state, owned by the modal so the catalog stays presentational. */ + paste?: { + value: string; + busy: boolean; + message: string; + ok: boolean; + onChange: (value: string) => void; + onSubmit: (provider: string) => void; + }; onLogin?: (provider: string, addAccount?: boolean) => void; onCancelLogin?: (provider: string) => void; onLogout?: (provider: string) => void; @@ -152,8 +167,13 @@ export default function ProviderCatalog({ const statusText = loggedIn ? (status?.email ?? row.statusLabel ?? t("modal.accountLoggedIn")) : (status?.error ?? row.statusLabel ?? t("modal.accountLoggedOut")); + // A first-time add is the one moment the operator has no other way in: + // the provider has no workspace panel yet, so without this the + // authorization URL is computed and never drawn. + const showHint = shouldShowLoginHint(row, busyProvider, loginHint); return ( -
+
+
{row.label}
{statusText}
@@ -208,6 +228,24 @@ export default function ProviderCatalog({ onLogin && )}
+
+ {showHint && loginHint && ( + paste.onSubmit(row.id), + }, + } + : {})} + /> + )}
); })} diff --git a/gui/src/components/provider-catalog/login-hint-visibility.ts b/gui/src/components/provider-catalog/login-hint-visibility.ts new file mode 100644 index 00000000000..24ff14c9afd --- /dev/null +++ b/gui/src/components/provider-catalog/login-hint-visibility.ts @@ -0,0 +1,43 @@ +/** + * provider-catalog/login-hint-visibility.ts + * + * One predicate, extracted from the catalog's JSX so its failure cases are + * testable without a DOM. It lives beside the catalog rather than in + * `provider-presets.ts`, which is the preset DTO / tier / search module and + * has no business knowing about login chrome. + */ + +/** Login hint carried by the providers page while an account-row login is in flight. */ +export type CatalogLoginHint = { + provider: string; + url?: string; + instructions?: string; + deviceCode?: string; +}; + +/** The account-row kinds the catalog renders; only OAuth rows own a login hint. */ +export type CatalogRowKind = "oauth" | "key" | "codex"; + +/** + * Whether an account row should render the in-flight login hint. + * + * Two failure cases this exists to prevent: + * + * 1. **Cross-provider paint.** The page holds ONE hint for whichever login is + * in flight, and the Accounts tab renders many rows from it. A login started + * for one provider must never show its authorization URL under another. + * 2. **Wrong surface entirely.** A `codex` row does not log in through + * `/api/oauth` at all — it opens the Codex account modal, and the page marks + * itself busy while enabling the OpenAI provider. A stale hint must not paint + * an OAuth URL onto a row whose real flow is somewhere else. + */ +export function shouldShowLoginHint( + row: { id: string; kind: CatalogRowKind }, + busyProvider: string | null, + hint: CatalogLoginHint | null | undefined, +): boolean { + if (!hint) return false; + if (row.kind !== "oauth") return false; + if (busyProvider !== row.id) return false; + return hint.provider === row.id; +} diff --git a/gui/src/pages/Providers.tsx b/gui/src/pages/Providers.tsx index 3c0c6c10b8a..158497a36cf 100644 --- a/gui/src/pages/Providers.tsx +++ b/gui/src/pages/Providers.tsx @@ -394,6 +394,7 @@ export default function Providers({ apiBase }: { apiBase: string }) { busy={busy} addModalAccountRows={addModalAccountRows} accountLoginStatus={accountLoginStatus} + accountLoginHint={loginInfo} removeConfirmName={removeConfirmName} removeDefaultProvider={removeConfirmName === config.defaultProvider ? Object.entries(config.providers).find(([name, provider]) => name !== removeConfirmName && provider.disabled !== true)?.[0] ?? null diff --git a/gui/src/pages/providers-page-modals.tsx b/gui/src/pages/providers-page-modals.tsx index 243dd125aeb..051c3d2ee6f 100644 --- a/gui/src/pages/providers-page-modals.tsx +++ b/gui/src/pages/providers-page-modals.tsx @@ -16,6 +16,7 @@ export function ProvidersPageModals({ busy, addModalAccountRows, accountLoginStatus, + accountLoginHint, removeConfirmName, removeDefaultProvider, codexLoginOpen, @@ -46,6 +47,7 @@ export function ProvidersPageModals({ busy: string | null; addModalAccountRows: AccountLoginRow[]; accountLoginStatus: Record; + accountLoginHint?: { provider: string; url?: string; instructions?: string; deviceCode?: string } | null; removeConfirmName: string | null; removeDefaultProvider: string | null; codexLoginOpen: boolean; @@ -82,6 +84,7 @@ export function ProvidersPageModals({ accountRows={addModalAccountRows} accountStatus={accountLoginStatus} accountBusy={busy} + accountLoginHint={accountLoginHint ?? null} onAccountLogin={onAccountLogin} onAccountCancelLogin={onAccountCancelLogin} onAccountLogout={onAccountLogout} diff --git a/gui/src/styles/provider-catalog.css b/gui/src/styles/provider-catalog.css index e12d5b199a4..c6c5618a412 100644 --- a/gui/src/styles/provider-catalog.css +++ b/gui/src/styles/provider-catalog.css @@ -60,6 +60,32 @@ text-overflow: ellipsis; } +/* An account row is normally one horizontal line (`.list-row` is a row flex). + While a login for that row is in flight it grows a second line for the + authorization URL, device code, and paste field, so the head keeps the + original layout and the hint sits underneath it rather than being squeezed + into the badge strip. A row with no login in flight is untouched. */ +.provider-catalog-account-row-head { + display: flex; + align-items: center; + justify-content: space-between; + gap: 10px; + width: 100%; +} + +/* Specificity, not source order: this file is @imported at the TOP of + styles.css while `.list-row` is declared far below it, so a bare + `.provider-catalog-account-row--waiting` would lose `align-items` and + `cursor` to the later equal-specificity `.list-row` and the hint would + render as a shrink-wrapped centered column instead of a full-width second + line. Qualifying with `.list-row` wins the cascade regardless of order. */ +.list-row.provider-catalog-account-row--waiting { + flex-direction: column; + align-items: stretch; + gap: 10px; + cursor: default; +} + .provider-catalog-footer { display: flex; gap: 8px; diff --git a/tests/oauth-first-add-hint.test.ts b/tests/oauth-first-add-hint.test.ts new file mode 100644 index 00000000000..da66f7a9f7e --- /dev/null +++ b/tests/oauth-first-add-hint.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, test } from "bun:test"; +import { shouldShowLoginHint } from "../gui/src/components/provider-catalog/login-hint-visibility"; + +/** + * A first-time provider add is the one moment the operator has no other way in. + * Once a provider exists its workspace panel shows the authorization URL with a + * copy button; during the first login no panel is mounted, so the hint has to + * render inside the add-provider dialog or it renders nowhere — which is what + * used to happen: the URL was fetched, stored in page state, and never drawn. + * + * The predicate is extracted from the JSX so the leak case is testable without + * a DOM. That case is not hypothetical: the page holds ONE hint for whichever + * login is in flight, and the Accounts tab renders many rows from it. + */ + +const hint = { provider: "xai", url: "https://accounts.x.ai/oauth/authorize?x=1", deviceCode: "WDJB-MJHT" }; +const oauthRow = (id: string) => ({ id, kind: "oauth" as const }); + +describe("first-add login hint visibility", () => { + test("renders on the row whose login is in flight", () => { + expect(shouldShowLoginHint(oauthRow("xai"), "xai", hint)).toBe(true); + }); + + test("never renders another provider's authorization URL", () => { + // The busy row is anthropic, but the hint belongs to xai: a late or + // superseded response must not paint under the wrong provider. + expect(shouldShowLoginHint(oauthRow("anthropic"), "anthropic", hint)).toBe(false); + // And xai's own row stays quiet while a different provider is the busy one. + expect(shouldShowLoginHint(oauthRow("xai"), "anthropic", hint)).toBe(false); + }); + + test("renders nothing when no login is in flight", () => { + expect(shouldShowLoginHint(oauthRow("xai"), null, hint)).toBe(false); + expect(shouldShowLoginHint(oauthRow("xai"), "xai", null)).toBe(false); + expect(shouldShowLoginHint(oauthRow("xai"), "xai", undefined)).toBe(false); + }); + + test("a Codex row never shows an OAuth hint, even while the page is busy on it", () => { + // A codex row opens the Codex account modal instead of /api/oauth, and the + // page marks itself busy while enabling the OpenAI provider. A stale hint + // must not paint an authorization URL onto a row whose flow is elsewhere. + const stale = { provider: "openai", url: "https://auth.openai.com/authorize?x=1" }; + expect(shouldShowLoginHint({ id: "openai", kind: "codex" }, "openai", stale)).toBe(false); + // Key rows have no login button at all. + expect(shouldShowLoginHint({ id: "xai", kind: "key" }, "xai", hint)).toBe(false); + }); +});