From 5fac4983fabfc6fdd4eb4942f3a9888e004be55d Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 22:36:25 -0500 Subject: [PATCH 1/7] test(ui-components): give the package a test harness Copies the setup ui-react got in #92: vitest, jsdom, and globals on so @testing-library/react can register its own cleanup. Turbo needs no change because its test task already declares dependsOn ["^build"]. test-support.tsx carries the scaffolding the component tests share. It gives you two levers. withProvider drives the real AuthProvider, AuthManager and AuthClient stack over an injected fetch and storage, which is how a test proves a component works against the code that ships. stubAuth hands back an AuthContextValue the test owns, for the cases that need the session to change while the component stays mounted. AuthContext is exported from ui-react for exactly that. routedFetch dispatches on "METHOD /path" and 404s anything unrouted, so a missing route shows up as a failed assertion instead of a hung test. It serves /v1/me by default, since AuthManager.initialize fetches the profile before it will report a session at all. --- ui/packages/components/package.json | 8 +- ui/packages/components/src/test-support.tsx | 217 ++++++++++++++++++++ ui/packages/components/vitest.config.ts | 16 ++ ui/pnpm-lock.yaml | 9 + 4 files changed, 248 insertions(+), 2 deletions(-) create mode 100644 ui/packages/components/src/test-support.tsx create mode 100644 ui/packages/components/vitest.config.ts diff --git a/ui/packages/components/package.json b/ui/packages/components/package.json index 6e4cddb3..2d9e95c2 100644 --- a/ui/packages/components/package.json +++ b/ui/packages/components/package.json @@ -29,7 +29,8 @@ "dev": "tsup --watch", "typecheck": "tsc --noEmit", "clean": "rm -rf dist", - "lint": "eslint ." + "lint": "eslint .", + "test": "vitest run" }, "dependencies": { "@authsome/ui-core": "workspace:*", @@ -56,12 +57,15 @@ "react-dom": "^18.0.0 || ^19.0.0" }, "devDependencies": { + "@testing-library/react": "^16.3.0", "@types/react": "^19.2.18", "@types/react-dom": "^19.2.5", + "jsdom": "^28.0.1", "react": "^19.2.8", "react-dom": "^19.2.8", "tailwindcss": "^4.3.3", "tsup": "^8.5.1", - "typescript": "^5.9.3" + "typescript": "^5.9.3", + "vitest": "^4.1.11" } } diff --git a/ui/packages/components/src/test-support.tsx b/ui/packages/components/src/test-support.tsx new file mode 100644 index 00000000..ab9a8aae --- /dev/null +++ b/ui/packages/components/src/test-support.tsx @@ -0,0 +1,217 @@ +/** + * Shared scaffolding for this package's component tests. + * + * Two levers, deliberately: `withProvider` drives the real AuthProvider -> + * AuthManager -> AuthClient stack over an injected fetch and storage, which is + * how a test proves a component works against the shipping code path. + * `stubAuth` hands back an AuthContextValue the test owns outright, for the + * cases that need the session to change while the component stays mounted — + * AuthContext is exported from ui-react for exactly that. + */ + +import { AuthClient, AuthManager } from "@authsome/ui-core"; +import type { + AuthState, + ClientConfig, + Session, + TokenStorage, + User, +} from "@authsome/ui-core"; +import { + AuthContext, + AuthProvider, + type AuthContextValue, +} from "@authsome/ui-react"; +import type { ReactElement, ReactNode } from "react"; + +export const BASE = "https://api.example.test"; + +/** A session far enough from expiry that the manager will not try to refresh. */ +export function makeSession(token = "tok"): Session { + return { + session_token: token, + refresh_token: `${token}-refresh`, + expires_at: new Date(Date.now() + 3_600_000).toISOString(), + }; +} + +export function makeUser(overrides: Partial = {}): User { + return { + id: "usr_1", + app_id: "app_1", + env_id: "env_1", + email: "ada@test", + email_verified: true, + first_name: "Ada", + last_name: "Lovelace", + banned: false, + phone_verified: false, + created_at: new Date(0).toISOString(), + updated_at: new Date(0).toISOString(), + ...overrides, + } as User; +} + +export function json(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }); +} + +/** Handler for one "METHOD /path" route. Return a Response or a plain body. */ +export type Route = (req: { + path: string; + method: string; + token: string | null; +}) => unknown | Promise; + +export interface RoutedFetch { + fetchFn: typeof globalThis.fetch; + /** Every "METHOD /path" the component actually requested, in order. */ + calls: string[]; + /** The same requests with their query strings, for asserting on params. */ + urls: string[]; +} + +/** + * A fetch that dispatches on "METHOD /path". Anything unrouted 404s loudly + * rather than hanging, so a missing route shows up as a failed assertion + * instead of a timeout. + */ +export function routedFetch(routes: Record): RoutedFetch { + const calls: string[] = []; + const urls: string[] = []; + + // AuthManager.initialize() fetches the profile before it will report a + // session, so every component test needs this route. Defaulting it here + // keeps it out of the tests, which care about their own endpoint. A test + // that wants a different profile, or a failing one, just overrides it. + const table: Record = { "GET /v1/me": () => makeUser(), ...routes }; + + const fetchFn = (async ( + input: string | URL | Request, + init?: RequestInit, + ): Promise => { + const url = new URL( + String(input instanceof Request ? input.url : input), + BASE, + ); + const method = (init?.method ?? "GET").toUpperCase(); + const key = `${method} ${url.pathname}`; + calls.push(key); + urls.push(`${method} ${url.pathname}${url.search}`); + + const handler = table[key]; + if (!handler) return json({ error: `no route for ${key}` }, 404); + + const auth = new Headers(init?.headers).get("Authorization"); + // Await it: a handler that holds a request open (to make the in-flight + // state observable) returns a promise, and stringifying that yields "{}". + const result = await handler({ + path: url.pathname, + method, + token: auth?.replace(/^Bearer /, "") ?? null, + }); + return result instanceof Response ? result : json(result); + }) as typeof globalThis.fetch; + + return { fetchFn, calls, urls }; +} + +export function memoryStorage(seed?: Session): TokenStorage { + const store = new Map(); + if (seed) store.set("authsome:session", JSON.stringify(seed)); + return { + getItem: (k) => store.get(k) ?? null, + setItem: (k, v) => void store.set(k, v), + removeItem: (k) => void store.delete(k), + }; +} + +/** Wrap children in a real AuthProvider over an injected fetch and storage. */ +export function withProvider( + children: ReactNode, + opts: { + fetch: typeof globalThis.fetch; + session?: Session | null; + /** Seeds the manager's client config, so useClientConfig has something + * to report without a discovery round trip. */ + clientConfig?: ClientConfig; + }, +): ReactElement { + const session = opts.session === undefined ? makeSession() : opts.session; + return ( + + {children} + + ); +} + +function unsupported(name: string): () => never { + return () => { + throw new Error(`${name} is not wired in this test`); + }; +} + +/** + * A complete AuthContextValue the test controls. Only `client` and `session` + * matter to the components under test here; the rest are present because the + * type demands them and throw if something reaches for them unexpectedly. + */ +export function stubAuth(opts: { + fetch: typeof globalThis.fetch; + session: Session | null; + user?: User | null; + /** + * Reuse a client across values. A real token change comes from the same + * AuthManager and therefore the same client, so a test that swaps the + * session must hold the client identity steady or it is also changing a + * dependency the component keys its fetch on. + */ + client?: AuthClient; +}): AuthContextValue { + const client = + opts.client ?? new AuthClient({ baseURL: BASE, fetch: opts.fetch }); + const user = opts.user === undefined ? makeUser() : opts.user; + const state: AuthState = + opts.session && user + ? { status: "authenticated", user, session: opts.session } + : { status: "unauthenticated" }; + + return { + state, + manager: new AuthManager({ baseURL: BASE, fetch: opts.fetch }), + client, + user: opts.session ? user : null, + session: opts.session, + isAuthenticated: Boolean(opts.session), + isLoading: false, + clientConfig: null, + isConfigLoaded: true, + signIn: unsupported("signIn"), + signUp: unsupported("signUp"), + signOut: unsupported("signOut"), + resendVerification: unsupported("resendVerification"), + submitMFAChallenge: unsupported("submitMFAChallenge"), + submitMFACode: unsupported("submitMFACode"), + submitRecoveryCode: unsupported("submitRecoveryCode"), + sendSMSCode: unsupported("sendSMSCode"), + submitSMSCode: unsupported("submitSMSCode"), + }; +} + +/** Wrap children in a caller-owned context value. */ +export function withAuth( + children: ReactNode, + value: AuthContextValue, +): ReactElement { + return ( + {children} + ); +} diff --git a/ui/packages/components/vitest.config.ts b/ui/packages/components/vitest.config.ts new file mode 100644 index 00000000..790fa0e9 --- /dev/null +++ b/ui/packages/components/vitest.config.ts @@ -0,0 +1,16 @@ +import { defineConfig } from "vitest/config"; + +// Same shape as ui-react's config. These components render, so they need a +// DOM, and several of them read window.location or listen for popstate. +// +// globals is on so @testing-library/react can register its own afterEach +// cleanup. Without it every render stays in the document and the next test +// finds several copies of the same element. Tests still import describe/it/ +// expect explicitly, the same way the other packages do. +export default defineConfig({ + test: { + environment: "jsdom", + globals: true, + include: ["src/**/*.test.{ts,tsx}"], + }, +}); diff --git a/ui/pnpm-lock.yaml b/ui/pnpm-lock.yaml index 158df92d..abd14536 100644 --- a/ui/pnpm-lock.yaml +++ b/ui/pnpm-lock.yaml @@ -158,12 +158,18 @@ importers: specifier: ^3.6.0 version: 3.6.0 devDependencies: + '@testing-library/react': + specifier: ^16.3.0 + version: 16.3.2(@testing-library/dom@10.4.0)(@types/react-dom@19.2.5(@types/react@19.2.18))(@types/react@19.2.18)(react-dom@19.2.8(react@19.2.8))(react@19.2.8) '@types/react': specifier: ^19.2.18 version: 19.2.18 '@types/react-dom': specifier: ^19.2.5 version: 19.2.5(@types/react@19.2.18) + jsdom: + specifier: ^28.0.1 + version: 28.1.0 react: specifier: ^19.2.8 version: 19.2.8 @@ -179,6 +185,9 @@ importers: typescript: specifier: ^5.9.3 version: 5.9.3 + vitest: + specifier: ^4.1.11 + version: 4.1.11(@types/node@26.3.0)(jsdom@28.1.0)(vite@8.2.2(@types/node@26.3.0)(esbuild@0.28.2)(jiti@2.7.0)) packages/core: devDependencies: From 33aa4a975a8d0a79c15cc35a970fabcd2bb8f96d Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 22:36:40 -0500 Subject: [PATCH 2/7] fix(ui-components): keep the spinner on a reload in the three lists DeviceList, SessionList and PasskeyList all called a useCallback that opened with setIsLoading(true) from inside an effect, which is what react-hooks/set-state-in-effect reports. The cheap way out is to move the loading flag past the await so nothing is set synchronously. It passes lint, and it is the bug #94 shipped: isLoading starts true, so the mount case still looks right, and every later load has no spinner at all. Change your session token and you sit looking at the previous account's devices with nothing saying the list is being refetched. So loading is derived here rather than stored. The list is loading whenever what is on screen does not belong to the token it is now being asked about. That is true on the first render and true again the instant the token changes, and no effect has to set it. The refetch a handler triggers keeps its own flag, because a handler may set state synchronously. PasskeyList is worse than the other two under the cheap fix, not better. Its no-token branch settles loading to false, so the pre-hydration pass turns the spinner off before the token ever arrives and it never comes back. Deriving loading also lets that branch settle honestly instead of spinning forever when you are signed out. Each test was checked against a deliberately broken version. The mount test passes under the trap in all three, which is the point: only the token-changes test catches it. Removing the useCallback from PasskeyList also clears the preserve-manual-memoization finding on the same file. --- .../src/components/device-list.test.tsx | 168 ++++++++++++++++++ .../components/src/components/device-list.tsx | 62 ++++++- .../src/components/passkey-list.test.tsx | 123 +++++++++++++ .../src/components/passkey-list.tsx | 88 ++++++--- .../src/components/session-list.test.tsx | 112 ++++++++++++ .../src/components/session-list.tsx | 52 +++++- 6 files changed, 560 insertions(+), 45 deletions(-) create mode 100644 ui/packages/components/src/components/device-list.test.tsx create mode 100644 ui/packages/components/src/components/passkey-list.test.tsx create mode 100644 ui/packages/components/src/components/session-list.test.tsx diff --git a/ui/packages/components/src/components/device-list.test.tsx b/ui/packages/components/src/components/device-list.test.tsx new file mode 100644 index 00000000..6d197592 --- /dev/null +++ b/ui/packages/components/src/components/device-list.test.tsx @@ -0,0 +1,168 @@ +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { AuthClient } from "@authsome/ui-core"; +import { describe, expect, it } from "vitest"; + +import { DeviceList } from "./device-list"; +import { + BASE, + json, + makeSession, + routedFetch, + stubAuth, + withAuth, + withProvider, +} from "../test-support"; + +/** Skeleton is the only thing in this tree with animate-pulse. */ +function showsLoading(container: HTMLElement): boolean { + return container.querySelector(".animate-pulse") !== null; +} + +const laptop = { + id: "dev_1", + name: "Ada's laptop", + browser: "Firefox", + os: "Linux", + last_seen_at: new Date().toISOString(), + trusted: true, + type: "desktop", + created_at: new Date(0).toISOString(), + user_id: "usr_1", + app_id: "app_1", + updated_at: new Date(0).toISOString(), +}; + +describe("DeviceList", () => { + it("shows the loading skeletons until the devices arrive", async () => { + let release!: () => void; + const gate = new Promise((r) => (release = r)); + + const { fetchFn } = routedFetch({ + "GET /v1/devices": async () => { + await gate; + return { devices: [laptop] }; + }, + }); + + const { container } = render( + withProvider(, { fetch: fetchFn }), + ); + + // In flight: skeletons, no device row. + await waitFor(() => expect(showsLoading(container)).toBe(true)); + expect(screen.queryByText("Ada's laptop")).toBeNull(); + + release(); + + await waitFor(() => expect(screen.getByText("Ada's laptop")).toBeTruthy()); + expect(showsLoading(container)).toBe(false); + }); + + it("reports loading again when the session token changes", async () => { + // The regression guard. A rewrite that only sets isLoading after the await + // still passes the mount case, because isLoading starts true. It fails + // here: the second load has to put the component back into loading, or the + // user stares at the previous account's devices with no spinner. + const seen: string[] = []; + let release!: () => void; + let gate = Promise.resolve(); + + const { fetchFn } = routedFetch({ + "GET /v1/devices": async ({ token }) => { + seen.push(token ?? "none"); + await gate; + return { devices: [{ ...laptop, name: `laptop for ${token}` }] }; + }, + }); + + const client = new AuthClient({ baseURL: BASE, fetch: fetchFn }); + const first = stubAuth({ fetch: fetchFn, session: makeSession("tok-1"), client }); + + const { container, rerender } = render( + withAuth(, first), + ); + + await waitFor(() => + expect(screen.getByText("laptop for tok-1")).toBeTruthy(), + ); + expect(showsLoading(container)).toBe(false); + + // Second load, held open so the in-flight state is observable. + gate = new Promise((r) => (release = r)); + const second = stubAuth({ + fetch: fetchFn, + session: makeSession("tok-2"), + client, + }); + rerender(withAuth(, second)); + + await waitFor(() => expect(showsLoading(container)).toBe(true)); + expect(screen.queryByText("laptop for tok-1")).toBeNull(); + + release(); + await waitFor(() => + expect(screen.getByText("laptop for tok-2")).toBeTruthy(), + ); + expect(seen).toEqual(["tok-1", "tok-2"]); + }); + + it("surfaces the server's message when the load fails", async () => { + const { fetchFn } = routedFetch({ + "GET /v1/devices": () => json({ error: "devices unavailable" }, 500), + }); + + const { container } = render( + withProvider(, { fetch: fetchFn }), + ); + + await waitFor(() => + expect(screen.getByRole("alert").textContent).toContain( + "devices unavailable", + ), + ); + // The error clears the loading state rather than leaving both on screen. + expect(showsLoading(container)).toBe(false); + }); + + it("reloads through the loading state after trusting a device, then reports", async () => { + // Pins two things the fix must not disturb: the refetch a handler triggers + // goes through the same loading state as any other load, and onTrust fires + // after that refetch rather than before it. + const order: string[] = []; + let trusted = false; + let release!: () => void; + let gate = Promise.resolve(); + + const { fetchFn } = routedFetch({ + "GET /v1/devices": async () => { + await gate; + order.push("list"); + return { devices: [{ ...laptop, trusted }] }; + }, + "PATCH /v1/devices/dev_1/trust": () => { + order.push("trust"); + trusted = true; + return { ...laptop, trusted: true }; + }, + }); + + const seen: string[] = []; + const { container } = render( + withProvider( seen.push(id)} />, { + fetch: fetchFn, + }), + ); + + await waitFor(() => expect(screen.getByText("Untrusted")).toBeTruthy()); + + gate = new Promise((r) => (release = r)); + fireEvent.click(screen.getByTitle("Trust device")); + + await waitFor(() => expect(showsLoading(container)).toBe(true)); + expect(seen).toEqual([]); + + release(); + await waitFor(() => expect(seen).toEqual(["dev_1"])); + expect(order).toEqual(["list", "trust", "list"]); + }); +}); diff --git a/ui/packages/components/src/components/device-list.tsx b/ui/packages/components/src/components/device-list.tsx index 05b5bc69..62477d55 100644 --- a/ui/packages/components/src/components/device-list.tsx +++ b/ui/packages/components/src/components/device-list.tsx @@ -94,17 +94,67 @@ export function DeviceList({ const { client, session } = useAuth(); const [devices, setDevices] = useState([]); - const [isLoading, setIsLoading] = useState(true); const [error, setError] = useState(null); + const [loadedToken, setLoadedToken] = useState(null); + const [isReloading, setIsReloading] = useState(false); const [actionLoading, setActionLoading] = useState(null); const [deleteTarget, setDeleteTarget] = useState(null); const token = session?.session_token; + // Loading is derived, not stored. The list is loading whenever what is on + // screen does not belong to the token it is now being asked about: true on + // the first render, and true again the instant the token changes. + // + // Deriving it is the whole point. It lets the effect below set no state + // synchronously, which is what react-hooks/set-state-in-effect asks for, + // without losing the spinner on a reload. Moving setIsLoading(true) past the + // await instead satisfies the rule and silently drops the reload spinner, + // because the initial `true` covers the mount and nothing covers the rest. + // That is the regression #94 shipped, and device-list.test.tsx pins it. + const isLoading = + isReloading || + loadedToken === null || + (token !== undefined && loadedToken !== token); + + // A message from the previous attempt does not belong to this one, which is + // what clearing the error at the start of a fetch used to express. + const visibleError = isLoading ? null : error; + + // The automatic load. Runs on mount and whenever the token changes, and + // touches state only after the await. + useEffect(() => { + if (!token || loadedToken === token) return; + + let cancelled = false; + void (async () => { + try { + const response = await client.listDevices(token); + if (cancelled) return; + setDevices((response.devices ?? []) as unknown as Device[]); + setError(null); + } catch (err) { + if (cancelled) return; + setError( + err instanceof Error ? err.message : "Failed to load devices", + ); + } finally { + if (!cancelled) setLoadedToken(token); + } + })(); + + return () => { + cancelled = true; + }; + }, [client, token, loadedToken]); + + // The refetch the action handlers trigger. This one is called from an event + // handler, where setting state synchronously is exactly what React expects, + // so it can raise the loading flag up front the way the old code did. const fetchDevices = useCallback(async () => { if (!token) return; - setIsLoading(true); + setIsReloading(true); setError(null); try { @@ -115,14 +165,10 @@ export function DeviceList({ err instanceof Error ? err.message : "Failed to load devices", ); } finally { - setIsLoading(false); + setIsReloading(false); } }, [client, token]); - useEffect(() => { - void fetchDevices(); - }, [fetchDevices]); - const handleTrust = async (device: Device) => { if (!token) return; @@ -169,7 +215,7 @@ export function DeviceList({ - + {isLoading ? (
diff --git a/ui/packages/components/src/components/passkey-list.test.tsx b/ui/packages/components/src/components/passkey-list.test.tsx new file mode 100644 index 00000000..8583cc8a --- /dev/null +++ b/ui/packages/components/src/components/passkey-list.test.tsx @@ -0,0 +1,123 @@ +import { render, screen, waitFor } from "@testing-library/react"; +import { AuthClient } from "@authsome/ui-core"; +import { describe, expect, it } from "vitest"; + +import { PasskeyList } from "./passkey-list"; +import { + BASE, + json, + makeSession, + routedFetch, + stubAuth, + withAuth, + withProvider, +} from "../test-support"; + +function showsLoading(container: HTMLElement): boolean { + return container.querySelector(".animate-pulse") !== null; +} + +function makeCredential(name: string) { + return { + id: `cred_${name}`, + display_name: name, + transport: ["internal"], + created_at: new Date(0).toISOString(), + }; +} + +describe("PasskeyList", () => { + it("shows the loading skeletons until the passkeys arrive", async () => { + let release!: () => void; + const gate = new Promise((r) => (release = r)); + + const { fetchFn } = routedFetch({ + "GET /v1/passkeys": async () => { + await gate; + return { credentials: [makeCredential("Yubikey")] }; + }, + }); + + const { container } = render( + withProvider(, { fetch: fetchFn }), + ); + + await waitFor(() => expect(showsLoading(container)).toBe(true)); + release(); + + await waitFor(() => expect(screen.getByText("Yubikey")).toBeTruthy()); + expect(showsLoading(container)).toBe(false); + }); + + it("settles on the empty state rather than a spinner when signed out", async () => { + // The branch that makes this component different from the other two + // lists: with no token it stops loading instead of waiting forever. A fix + // that derives loading purely from "have I loaded for this token" would + // spin here for good. + const { fetchFn, calls } = routedFetch({}); + + const { container } = render( + withProvider(, { fetch: fetchFn, session: null }), + ); + + await waitFor(() => + expect(screen.getByText("No passkeys registered")).toBeTruthy(), + ); + expect(showsLoading(container)).toBe(false); + expect(calls).not.toContain("GET /v1/passkeys"); + }); + + it("reports loading again when the session token changes", async () => { + let release!: () => void; + let gate = Promise.resolve(); + + const { fetchFn } = routedFetch({ + "GET /v1/passkeys": async ({ token }) => { + await gate; + return { credentials: [makeCredential(`key-${token}`)] }; + }, + }); + + const client = new AuthClient({ baseURL: BASE, fetch: fetchFn }); + const { container, rerender } = render( + withAuth( + , + stubAuth({ fetch: fetchFn, session: makeSession("tok-1"), client }), + ), + ); + + await waitFor(() => expect(screen.getByText("key-tok-1")).toBeTruthy()); + expect(showsLoading(container)).toBe(false); + + gate = new Promise((r) => (release = r)); + rerender( + withAuth( + , + stubAuth({ fetch: fetchFn, session: makeSession("tok-2"), client }), + ), + ); + + await waitFor(() => expect(showsLoading(container)).toBe(true)); + expect(screen.queryByText("key-tok-1")).toBeNull(); + + release(); + await waitFor(() => expect(screen.getByText("key-tok-2")).toBeTruthy()); + }); + + it("surfaces the server's message when the load fails", async () => { + const { fetchFn } = routedFetch({ + "GET /v1/passkeys": () => json({ error: "passkeys unavailable" }, 500), + }); + + const { container } = render( + withProvider(, { fetch: fetchFn }), + ); + + await waitFor(() => + expect(screen.getByRole("alert").textContent).toContain( + "passkeys unavailable", + ), + ); + expect(showsLoading(container)).toBe(false); + }); +}); diff --git a/ui/packages/components/src/components/passkey-list.tsx b/ui/packages/components/src/components/passkey-list.tsx index 60d0e151..05610120 100644 --- a/ui/packages/components/src/components/passkey-list.tsx +++ b/ui/packages/components/src/components/passkey-list.tsx @@ -1,7 +1,7 @@ "use client"; import * as React from "react"; -import { useEffect, useState, useCallback } from "react"; +import { useEffect, useState } from "react"; import { useAuth } from "@authsome/ui-react"; import { Key, @@ -210,40 +210,70 @@ export function PasskeyList({ const { client, session } = useAuth(); const [credentials, setCredentials] = useState([]); - const [isLoading, setIsLoading] = useState(true); const [error, setError] = useState(null); const [deleteTarget, setDeleteTarget] = useState(null); const [isDeleting, setIsDeleting] = useState(false); - const fetchPasskeys = useCallback(async () => { - if (!session?.session_token) { - setIsLoading(false); - return; - } + const token = session?.session_token; - setError(null); - setIsLoading(true); + // Which token the list on screen belongs to. `undefined` means nothing has + // settled yet, `null` means the signed-out case has settled, a string means + // that token's passkeys are what is rendered. + const [loadedFor, setLoadedFor] = useState( + undefined, + ); - try { - // The codegen collapses multiple Go "ListResponse" structs into one - // TS type and the consents-list shape wins. The real passkey - // endpoint returns {credentials}; cast to the actual shape here. - const { credentials } = (await client.listPasskeys( - session.session_token, - )) as unknown as { credentials: CredentialInfo[] }; - setCredentials(credentials); - } catch (err) { - setError( - err instanceof Error ? err.message : "Failed to load passkeys", - ); - } finally { - setIsLoading(false); - } - }, [client, session?.session_token]); + // Loading is derived rather than stored, so the effect below can avoid + // setting state synchronously (react-hooks/set-state-in-effect) without + // losing the spinner. Deriving it also keeps the signed-out case honest: + // once that settles to null it stops loading instead of spinning forever. + // + // The tempting alternative, moving setIsLoading past the await, is worse + // here than it was in #94: the pre-hydration pass has no token, settles + // loading to false, and the spinner never appears at all. passkey-list.test + // .tsx pins both that and the reload case. + const isLoading = loadedFor !== (token ?? null); + + // A message from the previous attempt does not belong to this one, which is + // what clearing the error at the start of a fetch used to express. + const visibleError = isLoading ? null : error; useEffect(() => { - void fetchPasskeys(); - }, [fetchPasskeys]); + const current = token ?? null; + if (loadedFor === current) return; + + let cancelled = false; + void (async () => { + if (current === null) { + // Nothing to fetch. Settle so the empty state can replace the skeleton. + if (!cancelled) setLoadedFor(null); + return; + } + + try { + // The codegen collapses multiple Go "ListResponse" structs into one + // TS type and the consents-list shape wins. The real passkey + // endpoint returns {credentials}; cast to the actual shape here. + const { credentials } = (await client.listPasskeys( + current, + )) as unknown as { credentials: CredentialInfo[] }; + if (cancelled) return; + setCredentials(credentials); + setError(null); + } catch (err) { + if (cancelled) return; + setError( + err instanceof Error ? err.message : "Failed to load passkeys", + ); + } finally { + if (!cancelled) setLoadedFor(current); + } + })(); + + return () => { + cancelled = true; + }; + }, [client, token, loadedFor]); async function handleDelete(): Promise { if (!deleteTarget || !session?.session_token) { @@ -285,11 +315,11 @@ export function PasskeyList({ )} - + {isLoading && } - {!isLoading && credentials.length === 0 && !error && ( + {!isLoading && credentials.length === 0 && !visibleError && ( )} diff --git a/ui/packages/components/src/components/session-list.test.tsx b/ui/packages/components/src/components/session-list.test.tsx new file mode 100644 index 00000000..58bea3fb --- /dev/null +++ b/ui/packages/components/src/components/session-list.test.tsx @@ -0,0 +1,112 @@ +import { render, screen, waitFor } from "@testing-library/react"; +import { AuthClient } from "@authsome/ui-core"; +import { describe, expect, it } from "vitest"; + +import { SessionList } from "./session-list"; +import { + BASE, + json, + makeSession, + routedFetch, + stubAuth, + withAuth, + withProvider, +} from "../test-support"; + +function showsLoading(container: HTMLElement): boolean { + return container.querySelector(".animate-pulse") !== null; +} + +function makeRow(device: string) { + return { + id: `sess_${device}`, + device, + browser: "Firefox", + ip_address: "10.0.0.1", + last_active: new Date().toISOString(), + created_at: new Date(0).toISOString(), + session_token: "other", + }; +} + +describe("SessionList", () => { + it("shows the loading skeletons until the sessions arrive", async () => { + let release!: () => void; + const gate = new Promise((r) => (release = r)); + + const { fetchFn } = routedFetch({ + "GET /v1/sessions": async () => { + await gate; + return { sessions: [makeRow("Thinkpad")] }; + }, + }); + + const { container } = render( + withProvider(, { fetch: fetchFn }), + ); + + await waitFor(() => expect(showsLoading(container)).toBe(true)); + expect(screen.queryByText(/Thinkpad/)).toBeNull(); + + release(); + + await waitFor(() => expect(screen.getByText(/Thinkpad/)).toBeTruthy()); + expect(showsLoading(container)).toBe(false); + }); + + it("reports loading again when the session token changes", async () => { + // Family A regression guard: see device-list.test.tsx. A fix that only + // sets loading after the await still passes the mount case and fails here. + let release!: () => void; + let gate = Promise.resolve(); + + const { fetchFn } = routedFetch({ + "GET /v1/sessions": async ({ token }) => { + await gate; + return { sessions: [makeRow(`box-${token}`)] }; + }, + }); + + const client = new AuthClient({ baseURL: BASE, fetch: fetchFn }); + const first = stubAuth({ + fetch: fetchFn, + session: makeSession("tok-1"), + client, + }); + + const { container, rerender } = render(withAuth(, first)); + await waitFor(() => expect(screen.getByText(/box-tok-1/)).toBeTruthy()); + expect(showsLoading(container)).toBe(false); + + gate = new Promise((r) => (release = r)); + rerender( + withAuth( + , + stubAuth({ fetch: fetchFn, session: makeSession("tok-2"), client }), + ), + ); + + await waitFor(() => expect(showsLoading(container)).toBe(true)); + expect(screen.queryByText(/box-tok-1/)).toBeNull(); + + release(); + await waitFor(() => expect(screen.getByText(/box-tok-2/)).toBeTruthy()); + }); + + it("surfaces the server's message when the load fails", async () => { + const { fetchFn } = routedFetch({ + "GET /v1/sessions": () => json({ error: "sessions unavailable" }, 500), + }); + + const { container } = render( + withProvider(, { fetch: fetchFn }), + ); + + await waitFor(() => + expect(screen.getByRole("alert").textContent).toContain( + "sessions unavailable", + ), + ); + expect(showsLoading(container)).toBe(false); + }); +}); diff --git a/ui/packages/components/src/components/session-list.tsx b/ui/packages/components/src/components/session-list.tsx index b5e4a6b9..cc289f5a 100644 --- a/ui/packages/components/src/components/session-list.tsx +++ b/ui/packages/components/src/components/session-list.tsx @@ -68,7 +68,8 @@ export function SessionList({ const { client, session } = useAuth(); const [sessions, setSessions] = useState([]); - const [isLoading, setIsLoading] = useState(true); + const [loadedToken, setLoadedToken] = useState(null); + const [isReloading, setIsReloading] = useState(false); const [error, setError] = useState(null); const [actionLoading, setActionLoading] = useState(null); const [revokeTarget, setRevokeTarget] = useState(null); @@ -77,10 +78,49 @@ export function SessionList({ const token = session?.session_token; const activeToken = currentSessionToken ?? token; + // Loading is derived, not stored: the list is loading whenever what is on + // screen does not belong to the token it is now being asked about. See the + // longer note in device-list.tsx — moving setIsLoading(true) past the await + // instead is lint-clean and silently drops the reload spinner. + const isLoading = + isReloading || + loadedToken === null || + (token !== undefined && loadedToken !== token); + + const visibleError = isLoading ? null : error; + + // The automatic load. Touches state only after the await. + useEffect(() => { + if (!token || loadedToken === token) return; + + let cancelled = false; + void (async () => { + try { + const response = await client.listSessions(token); + if (cancelled) return; + setSessions((response.sessions ?? []) as unknown as Session[]); + setError(null); + } catch (err) { + if (cancelled) return; + setError( + err instanceof Error ? err.message : "Failed to load sessions", + ); + } finally { + if (!cancelled) setLoadedToken(token); + } + })(); + + return () => { + cancelled = true; + }; + }, [client, token, loadedToken]); + + // The refetch the revoke handlers trigger. Called from an event handler, so + // it can raise the flag up front the way the old code did. const fetchSessions = useCallback(async () => { if (!token) return; - setIsLoading(true); + setIsReloading(true); setError(null); try { @@ -91,14 +131,10 @@ export function SessionList({ err instanceof Error ? err.message : "Failed to load sessions", ); } finally { - setIsLoading(false); + setIsReloading(false); } }, [client, token]); - useEffect(() => { - void fetchSessions(); - }, [fetchSessions]); - const isCurrentSession = (s: Session): boolean => { return Boolean(activeToken && s.session_token === activeToken); }; @@ -162,7 +198,7 @@ export function SessionList({ - + {isLoading ? (
From 4dcb245439861fc0a6df3dc2fabc69b96b3ec1cb Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 22:36:50 -0500 Subject: [PATCH 3/7] fix(ui-components): read the location as the external store it is useSubPath was duplicated byte for byte in sign-in.tsx and sign-up.tsx, and useCodeFromURL in device-authorization-form.tsx is the same shape over the query string. All three mirrored window.location into state and resynced it from an effect, which is the set-state-in-effect finding on each. The trap here is not the one the list components had. Each effect opens by re-deriving the value the useState initializer already computed, so that line reads as dead code and is the obvious one to delete. Delete it and every mount test still passes. What breaks is the resync when basePath changes, which is what happens when you mount SignIn and SignUp against the same URL. useSyncExternalStore has no such line to lose. A new basePath is a new snapshot function and React re-reads it, and the third argument reports undefined during SSR the way the old window guard did. useSubPath is now one shared module instead of two copies, so it is fixed and tested once. sign-in.test.tsx covers the swap through the component, since the screen you get is picked off the sub-path. --- .../src/components/sign-in.test.tsx | 50 +++++++++++ .../components/src/components/sign-in.tsx | 30 +------ .../components/src/components/sign-up.tsx | 26 +----- ui/packages/components/src/lib/pop-state.ts | 12 +++ .../components/src/lib/use-code-from-url.ts | 29 ++++++ .../components/src/lib/use-sub-path.test.tsx | 88 +++++++++++++++++++ .../components/src/lib/use-sub-path.ts | 44 ++++++++++ 7 files changed, 225 insertions(+), 54 deletions(-) create mode 100644 ui/packages/components/src/components/sign-in.test.tsx create mode 100644 ui/packages/components/src/lib/pop-state.ts create mode 100644 ui/packages/components/src/lib/use-code-from-url.ts create mode 100644 ui/packages/components/src/lib/use-sub-path.test.tsx create mode 100644 ui/packages/components/src/lib/use-sub-path.ts diff --git a/ui/packages/components/src/components/sign-in.test.tsx b/ui/packages/components/src/components/sign-in.test.tsx new file mode 100644 index 00000000..5ba004b8 --- /dev/null +++ b/ui/packages/components/src/components/sign-in.test.tsx @@ -0,0 +1,50 @@ +import { act, render, screen } from "@testing-library/react"; +import { describe, expect, it } from "vitest"; + +import { SignIn } from "./sign-in"; +import { routedFetch, withProvider } from "../test-support"; + +function at(pathname: string): void { + window.history.pushState({}, "", pathname); +} + +function navigate(pathname: string): void { + act(() => { + window.history.pushState({}, "", pathname); + window.dispatchEvent(new PopStateEvent("popstate")); + }); +} + +/** + * SignIn picks its screen off the sub-path. These pin that mapping through the + * component, so the shared useSubPath hook cannot be swapped underneath it + * without the routing being re-checked. + */ +describe("SignIn routing", () => { + const mount = () => { + const { fetchFn } = routedFetch({}); + return render(withProvider(, { fetch: fetchFn, session: null })); + }; + + it("shows the forgot-password screen at /sign-in/forgot-password", () => { + at("/sign-in/forgot-password"); + mount(); + expect(screen.getByText("Forgot password")).toBeTruthy(); + }); + + it("shows the reset-password screen at /sign-in/reset-password", () => { + at("/sign-in/reset-password?token=abc"); + mount(); + // The title and the submit button share this label. + expect(screen.getAllByText("Reset password").length).toBeGreaterThan(0); + }); + + it("leaves the sub-screens when the user navigates back", () => { + at("/sign-in/forgot-password"); + mount(); + expect(screen.getByText("Forgot password")).toBeTruthy(); + + navigate("/sign-in"); + expect(screen.queryByText("Forgot password")).toBeNull(); + }); +}); diff --git a/ui/packages/components/src/components/sign-in.tsx b/ui/packages/components/src/components/sign-in.tsx index 75b3ffe5..5c91cd18 100644 --- a/ui/packages/components/src/components/sign-in.tsx +++ b/ui/packages/components/src/components/sign-in.tsx @@ -3,6 +3,7 @@ import * as React from "react"; import { useAuth } from "@authsome/ui-react"; import { safeRedirectTarget } from "@authsome/ui-core"; +import { useSubPath } from "../lib/use-sub-path"; import { SignInForm } from "./sign-in-form"; import { ForgotPasswordForm } from "./forgot-password-form"; import { ResetPasswordForm } from "./reset-password-form"; @@ -198,32 +199,3 @@ function VerifyEmailRoute({ ); } -/** - * Extracts the sub-path segment after the base path from the current URL. - * E.g. for base="/sign-in" and URL="/sign-in/forgot-password", returns "forgot-password". - */ -function useSubPath(basePath: string): string | undefined { - const [subPath, setSubPath] = React.useState(() => { - if (typeof window === "undefined") return undefined; - return extractSubPath(window.location.pathname, basePath); - }); - - React.useEffect(() => { - setSubPath(extractSubPath(window.location.pathname, basePath)); - - const handler = () => { - setSubPath(extractSubPath(window.location.pathname, basePath)); - }; - window.addEventListener("popstate", handler); - return () => window.removeEventListener("popstate", handler); - }, [basePath]); - - return subPath; -} - -function extractSubPath(pathname: string, basePath: string): string | undefined { - const normalized = basePath.replace(/\/+$/, ""); - if (!pathname.startsWith(normalized)) return undefined; - const rest = pathname.slice(normalized.length).replace(/^\/+/, ""); - return rest || undefined; -} diff --git a/ui/packages/components/src/components/sign-up.tsx b/ui/packages/components/src/components/sign-up.tsx index b90fef8e..ea672349 100644 --- a/ui/packages/components/src/components/sign-up.tsx +++ b/ui/packages/components/src/components/sign-up.tsx @@ -3,6 +3,7 @@ import * as React from "react"; import { useClientConfig } from "@authsome/ui-react"; import { safeRedirectTarget } from "@authsome/ui-core"; +import { useSubPath } from "../lib/use-sub-path"; import { SignUpForm } from "./sign-up-form"; import { EmailVerificationForm } from "./email-verification-form"; import type { AuthCardAlign, AuthCardVariant } from "./auth-card"; @@ -118,28 +119,3 @@ export function SignUp({ ); } -function useSubPath(basePath: string): string | undefined { - const [subPath, setSubPath] = React.useState(() => { - if (typeof window === "undefined") return undefined; - return extractSubPath(window.location.pathname, basePath); - }); - - React.useEffect(() => { - setSubPath(extractSubPath(window.location.pathname, basePath)); - - const handler = () => { - setSubPath(extractSubPath(window.location.pathname, basePath)); - }; - window.addEventListener("popstate", handler); - return () => window.removeEventListener("popstate", handler); - }, [basePath]); - - return subPath; -} - -function extractSubPath(pathname: string, basePath: string): string | undefined { - const normalized = basePath.replace(/\/+$/, ""); - if (!pathname.startsWith(normalized)) return undefined; - const rest = pathname.slice(normalized.length).replace(/^\/+/, ""); - return rest || undefined; -} diff --git a/ui/packages/components/src/lib/pop-state.ts b/ui/packages/components/src/lib/pop-state.ts new file mode 100644 index 00000000..6a98f5bf --- /dev/null +++ b/ui/packages/components/src/lib/pop-state.ts @@ -0,0 +1,12 @@ +/** + * Subscribes to history navigation. + * + * Shared by the hooks that read the current location. They read it through + * useSyncExternalStore rather than mirroring it into state, because the + * location is an external store and treating it as one is what keeps them + * from having to resync it from an effect. + */ +export function subscribeToPopState(onChange: () => void): () => void { + window.addEventListener("popstate", onChange); + return () => window.removeEventListener("popstate", onChange); +} diff --git a/ui/packages/components/src/lib/use-code-from-url.ts b/ui/packages/components/src/lib/use-code-from-url.ts new file mode 100644 index 00000000..736d2909 --- /dev/null +++ b/ui/packages/components/src/lib/use-code-from-url.ts @@ -0,0 +1,29 @@ +import { useSyncExternalStore } from "react"; + +import { subscribeToPopState } from "./pop-state"; + +/** + * Reads user_code or code from the current URL query params. + * Supports both raw codes (`ABCDEFGH`) and dash-formatted (`ABCD-EFGH`). + */ +export function parseCodeFromSearch(search: string): string | undefined { + const params = new URLSearchParams(search); + const raw = params.get("user_code") ?? params.get("code"); + if (!raw) return undefined; + const cleaned = raw.replace(/[^A-Z0-9]/gi, "").toUpperCase(); + return cleaned || undefined; +} + +function getSnapshot(): string | undefined { + return parseCodeFromSearch(window.location.search); +} + +/** + * The device code carried in the current URL, kept in step with history + * navigation. Reads the location as the external store it is, so there is no + * mirrored state for an effect to resync. Reports undefined during SSR, which + * is what the previous useState initializer's window guard did. + */ +export function useCodeFromURL(): string | undefined { + return useSyncExternalStore(subscribeToPopState, getSnapshot, () => undefined); +} diff --git a/ui/packages/components/src/lib/use-sub-path.test.tsx b/ui/packages/components/src/lib/use-sub-path.test.tsx new file mode 100644 index 00000000..3920d88b --- /dev/null +++ b/ui/packages/components/src/lib/use-sub-path.test.tsx @@ -0,0 +1,88 @@ +import { act, render, screen } from "@testing-library/react"; +import { describe, expect, it } from "vitest"; + +import { extractSubPath, useSubPath } from "./use-sub-path"; + +function go(pathname: string): void { + window.history.pushState({}, "", pathname); +} + +function pop(pathname: string): void { + act(() => { + window.history.pushState({}, "", pathname); + window.dispatchEvent(new PopStateEvent("popstate")); + }); +} + +function Probe({ basePath }: { basePath: string }) { + const sub = useSubPath(basePath); + return {sub ?? "(none)"}; +} + +function current(): string { + return screen.getByTestId("sub").textContent ?? ""; +} + +describe("extractSubPath", () => { + it("returns the segment following the base path", () => { + expect(extractSubPath("/sign-in/forgot-password", "/sign-in")).toBe( + "forgot-password", + ); + }); + + it("returns undefined when the path is exactly the base path", () => { + expect(extractSubPath("/sign-in", "/sign-in")).toBeUndefined(); + }); + + it("ignores trailing slashes on the base path", () => { + expect(extractSubPath("/sign-in/verify-email", "/sign-in//")).toBe( + "verify-email", + ); + }); + + it("returns undefined when the path is outside the base path", () => { + expect(extractSubPath("/settings/profile", "/sign-in")).toBeUndefined(); + }); +}); + +describe("useSubPath", () => { + it("reports the sub-path of the current location on first render", () => { + go("/sign-in/forgot-password"); + render(); + expect(current()).toBe("forgot-password"); + }); + + it("follows popstate navigation", () => { + go("/sign-in"); + render(); + expect(current()).toBe("(none)"); + + pop("/sign-in/reset-password"); + expect(current()).toBe("reset-password"); + + pop("/sign-in"); + expect(current()).toBe("(none)"); + }); + + it("resyncs when the base path changes", () => { + // The family B regression guard. The old effect re-derived the value on + // every run, which looks redundant next to the useState initializer and + // is the obvious thing to drop. Dropping it leaves mount behaviour intact + // and breaks exactly this: the same URL read against a new base path. + go("/sign-up/verify-email"); + const { rerender } = render(); + expect(current()).toBe("(none)"); + + rerender(); + expect(current()).toBe("verify-email"); + }); + + it("stops listening once unmounted", () => { + go("/sign-in"); + const { unmount } = render(); + unmount(); + // Would warn about setting state on an unmounted component if the + // subscription outlived the render. + pop("/sign-in/forgot-password"); + }); +}); diff --git a/ui/packages/components/src/lib/use-sub-path.ts b/ui/packages/components/src/lib/use-sub-path.ts new file mode 100644 index 00000000..88735167 --- /dev/null +++ b/ui/packages/components/src/lib/use-sub-path.ts @@ -0,0 +1,44 @@ +import { useCallback, useSyncExternalStore } from "react"; + +import { subscribeToPopState } from "./pop-state"; + +/** + * Extracts the sub-path segment after the base path. + * + * E.g. for base="/sign-in" and pathname="/sign-in/forgot-password", returns + * "forgot-password". + */ +export function extractSubPath( + pathname: string, + basePath: string, +): string | undefined { + const normalized = basePath.replace(/\/+$/, ""); + if (!pathname.startsWith(normalized)) return undefined; + const rest = pathname.slice(normalized.length).replace(/^\/+/, ""); + return rest || undefined; +} + +/** + * The sub-path of the current location relative to `basePath`, kept in step + * with history navigation. + * + * The location is an external store, so this reads it as one rather than + * mirroring it into state and resyncing from an effect. That version had to + * re-derive the value at the top of every effect run, which reads as dead code + * next to the useState initializer and is the obvious line to delete — but + * deleting it silently stops the hook resyncing when `basePath` changes. + * useSyncExternalStore has no such line to lose: a new basePath is a new + * snapshot function, and React re-reads it. + * + * The third argument is the server snapshot. There is no location to read + * during SSR, so it reports undefined, which is what the useState initializer's + * `typeof window === "undefined"` guard did. + */ +export function useSubPath(basePath: string): string | undefined { + const getSnapshot = useCallback( + () => extractSubPath(window.location.pathname, basePath), + [basePath], + ); + + return useSyncExternalStore(subscribeToPopState, getSnapshot, () => undefined); +} From af52c0e2b52c2e0613aaa31d4d2665f069605549 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 22:37:01 -0500 Subject: [PATCH 4/7] fix(ui-components): adopt codes and field defaults without an effect Two places mirrored a prop into state from an effect. DeviceAuthorizationForm copies initialCode, or the code it read from the URL, into the OTP value. SignUpForm seeds the configured defaults for its dynamic fields. Both now use React's documented pattern for adjusting state when a prop changes, comparing against the last value applied, so nothing is set inside an effect. Worth reading before you touch the device form again: doing what exhaustive-deps asks is the wrong move. It wants code and isSubmitting added to the dep list. Add them and every keystroke re-runs the effect, sees the typed value differ from the code in the URL, and overwrites what you just typed. The suppressed dep list was hiding a real reason. SignUpForm's risk runs the other way. Seeding it only on a change compares equal on the mount pass and the defaults never appear at all, so the initial values come from the useState initializer instead. prev still wins the spread, since a default may only fill a field you have not set. Dropping the `as any` on completeDeviceAuthorization goes here too. The shipped AuthClient in ui-core extends the generated one and types that method properly, so the cast was never buying anything. --- .../device-authorization-form.test.tsx | 124 ++++++++++++++++++ .../components/device-authorization-form.tsx | 57 +++----- .../src/components/sign-up-form.test.tsx | 76 +++++++++++ .../src/components/sign-up-form.tsx | 52 +++++--- 4 files changed, 255 insertions(+), 54 deletions(-) create mode 100644 ui/packages/components/src/components/device-authorization-form.test.tsx create mode 100644 ui/packages/components/src/components/sign-up-form.test.tsx diff --git a/ui/packages/components/src/components/device-authorization-form.test.tsx b/ui/packages/components/src/components/device-authorization-form.test.tsx new file mode 100644 index 00000000..45947f5d --- /dev/null +++ b/ui/packages/components/src/components/device-authorization-form.test.tsx @@ -0,0 +1,124 @@ +import { act, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { describe, expect, it } from "vitest"; + +import { DeviceAuthorizationForm } from "./device-authorization-form"; +import { routedFetch, withProvider } from "../test-support"; + +function at(search: string): void { + window.history.pushState({}, "", `/device${search}`); +} + +function navigate(search: string): void { + act(() => { + window.history.pushState({}, "", `/device${search}`); + window.dispatchEvent(new PopStateEvent("popstate")); + }); +} + +function codeInput(container: HTMLElement): HTMLInputElement { + const input = container.querySelector("input"); + if (!input) throw new Error("no code input rendered"); + return input as HTMLInputElement; +} + +describe("DeviceAuthorizationForm", () => { + it("takes the code from ?user_code, stripping the dashes", () => { + at("?user_code=ABCD-EFGH"); + const { fetchFn } = routedFetch({}); + const { container } = render( + withProvider(, { + fetch: fetchFn, + }), + ); + expect(codeInput(container).value).toBe("ABCDEFGH"); + }); + + it("also accepts ?code", () => { + at("?code=WXYZ1234"); + const { fetchFn } = routedFetch({}); + const { container } = render( + withProvider(, { + fetch: fetchFn, + }), + ); + expect(codeInput(container).value).toBe("WXYZ1234"); + }); + + it("follows popstate to a new user_code", () => { + at("?user_code=AAAA1111"); + const { fetchFn } = routedFetch({}); + const { container } = render( + withProvider(, { + fetch: fetchFn, + }), + ); + expect(codeInput(container).value).toBe("AAAA1111"); + + navigate("?user_code=BBBB2222"); + expect(codeInput(container).value).toBe("BBBB2222"); + }); + + it("adopts a new initialCode prop", () => { + at(""); + const { fetchFn } = routedFetch({}); + const { container, rerender } = render( + withProvider( + , + { fetch: fetchFn }, + ), + ); + expect(codeInput(container).value).toBe("AAAA1111"); + + rerender( + withProvider( + , + { fetch: fetchFn }, + ), + ); + expect(codeInput(container).value).toBe("CCCC3333"); + }); + + it("keeps what the user typed when the props have not changed", () => { + // Family C regression guard. Deriving the code from the prop on every + // render, rather than only when the prop changes, silently throws away + // typing the moment anything else re-renders the form. + at("?user_code=AAAA1111"); + const { fetchFn } = routedFetch({}); + const { container, rerender } = render( + withProvider(, { + fetch: fetchFn, + }), + ); + expect(codeInput(container).value).toBe("AAAA1111"); + + fireEvent.change(codeInput(container), { target: { value: "ZZZZ9999" } }); + expect(codeInput(container).value).toBe("ZZZZ9999"); + + rerender( + withProvider(, { + fetch: fetchFn, + }), + ); + expect(codeInput(container).value).toBe("ZZZZ9999"); + }); + + it("auto-submits a complete code from the URL exactly once", async () => { + at("?user_code=ABCD-EFGH"); + const bodies: string[] = []; + const { fetchFn } = routedFetch({ + "POST /v1/oauth/device/complete": () => { + bodies.push("called"); + return { status: "approved" }; + }, + }); + + render( + withProvider(, { fetch: fetchFn }), + ); + + await waitFor(() => + expect(screen.getByText("Device authorized successfully")).toBeTruthy(), + ); + expect(bodies).toHaveLength(1); + }); +}); diff --git a/ui/packages/components/src/components/device-authorization-form.tsx b/ui/packages/components/src/components/device-authorization-form.tsx index 40d926e5..31c1d23f 100644 --- a/ui/packages/components/src/components/device-authorization-form.tsx +++ b/ui/packages/components/src/components/device-authorization-form.tsx @@ -5,6 +5,7 @@ import { useState, useCallback, useEffect, useRef } from "react"; import { useAuth } from "@authsome/ui-react"; import { CheckCircle2 } from "lucide-react"; import { cn } from "../lib/utils"; +import { useCodeFromURL } from "../lib/use-code-from-url"; import { Button } from "../primitives/button"; import { InputOTP, @@ -79,6 +80,7 @@ export function DeviceAuthorizationForm({ const initialCode = initialCodeProp ?? autoCode; const [code, setCode] = useState(initialCode?.toUpperCase() ?? ""); + const [appliedCode, setAppliedCode] = useState(initialCode?.toUpperCase()); const [error, setError] = useState(null); const [isSubmitting, setIsSubmitting] = useState(false); const [isSuccess, setIsSuccess] = useState(false); @@ -102,7 +104,7 @@ export function DeviceAuthorizationForm({ // Use the AuthClient method — routes through the client's baseURL. // Sends Bearer token when available; the method also sets // credentials: "include" so cookies are sent for same-origin setups. - await (client as any).completeDeviceAuthorization( + await client.completeDeviceAuthorization( clean, "approve", token ?? undefined, @@ -126,13 +128,23 @@ export function DeviceAuthorizationForm({ [client, codeLength, isSubmitting, onError, onSubmitProp, onSuccess, token], ); - // Update code if initialCode changes. - useEffect(() => { - const newCode = (initialCodeProp ?? autoCode)?.toUpperCase(); - if (newCode && newCode !== code && !isSubmitting) { - setCode(newCode); + // Adopt a new code from the props or the URL. This is React's documented + // "adjusting state when a prop changes" pattern rather than an effect: it + // compares against the last code it applied, so it fires once when that + // source changes and never on an unrelated re-render. + // + // The effect it replaces listed only [initialCodeProp, autoCode], which is + // what react-hooks/exhaustive-deps reported. Completing that dep list the + // way the rule asks is worse than leaving it: with `code` in the deps, every + // keystroke re-runs the effect, sees the typed value differ from the URL + // code, and overwrites what the user just typed. The test file pins that. + const desiredCode = (initialCodeProp ?? autoCode)?.toUpperCase(); + if (desiredCode !== appliedCode) { + setAppliedCode(desiredCode); + if (desiredCode && !isSubmitting) { + setCode(desiredCode); } - }, [initialCodeProp, autoCode]); + } // Auto-submit when code is pre-filled from URL and is complete. // Wait for auth to finish loading so the token is available. @@ -259,34 +271,3 @@ export function DeviceAuthorizationForm({ ); } - -/** - * Reads user_code or code from the current URL query params. - * Supports both raw codes (`ABCDEFGH`) and dash-formatted (`ABCD-EFGH`). - */ -function useCodeFromURL(): string | undefined { - const [code, setCode] = useState(() => { - if (typeof window === "undefined") return undefined; - return parseCodeFromSearch(window.location.search); - }); - - useEffect(() => { - setCode(parseCodeFromSearch(window.location.search)); - - const handlePopState = () => { - setCode(parseCodeFromSearch(window.location.search)); - }; - window.addEventListener("popstate", handlePopState); - return () => window.removeEventListener("popstate", handlePopState); - }, []); - - return code; -} - -function parseCodeFromSearch(search: string): string | undefined { - const params = new URLSearchParams(search); - const raw = params.get("user_code") ?? params.get("code"); - if (!raw) return undefined; - const cleaned = raw.replace(/[^A-Z0-9]/gi, "").toUpperCase(); - return cleaned || undefined; -} diff --git a/ui/packages/components/src/components/sign-up-form.test.tsx b/ui/packages/components/src/components/sign-up-form.test.tsx new file mode 100644 index 00000000..835dcc57 --- /dev/null +++ b/ui/packages/components/src/components/sign-up-form.test.tsx @@ -0,0 +1,76 @@ +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import type { ClientConfig } from "@authsome/ui-core"; +import { describe, expect, it } from "vitest"; + +import { SignUpForm } from "./sign-up-form"; +import { routedFetch, withProvider } from "../test-support"; + +const withDefaults = { + signup_fields: [ + { + key: "company", + label: "Company", + type: "text", + order: 1, + default: "Acme Inc", + }, + { key: "role", label: "Role", type: "text", order: 2 }, + ], +} as unknown as ClientConfig; + +function field(key: string): HTMLInputElement { + return document.getElementById(`signup-field-${key}`) as HTMLInputElement; +} + +/** The dynamic fields live on the second step, behind the email form. */ +function continuePastEmail(): void { + fireEvent.change(document.getElementById("signup-email")!, { + target: { value: "ada@test" }, + }); + fireEvent.click(screen.getByRole("button", { name: "Continue" })); +} + +describe("SignUpForm dynamic field defaults", () => { + const mount = (clientConfig: ClientConfig) => { + const { fetchFn } = routedFetch({}); + return render( + withProvider(, { + fetch: fetchFn, + session: null, + clientConfig, + }), + ); + }; + + it("pre-fills a configured field default", async () => { + mount(withDefaults); + continuePastEmail(); + + await waitFor(() => expect(field("company")).toBeTruthy()); + expect(field("company").value).toBe("Acme Inc"); + expect(field("role").value).toBe(""); + }); + + it("keeps what the user typed over a default across re-renders", async () => { + // Family C regression guard. Seeding defaults on every render, rather than + // only when the field config changes, throws away the user's edit as soon + // as anything else re-renders the form. + const { rerender } = mount(withDefaults); + continuePastEmail(); + + await waitFor(() => expect(field("company")).toBeTruthy()); + fireEvent.change(field("company"), { target: { value: "Other Co" } }); + expect(field("company").value).toBe("Other Co"); + + const { fetchFn } = routedFetch({}); + rerender( + withProvider(, { + fetch: fetchFn, + session: null, + clientConfig: withDefaults, + }), + ); + + expect(field("company").value).toBe("Other Co"); + }); +}); diff --git a/ui/packages/components/src/components/sign-up-form.tsx b/ui/packages/components/src/components/sign-up-form.tsx index b3fa019b..00fd2804 100644 --- a/ui/packages/components/src/components/sign-up-form.tsx +++ b/ui/packages/components/src/components/sign-up-form.tsx @@ -56,6 +56,17 @@ export interface SignUpFormComponentProps { /** * Renders a single dynamic signup field based on its type. */ +/** The configured default value for each field that declares one. */ +function defaultsFor( + fields: SignupFieldConfig[] | null, +): Record { + const defaults: Record = {}; + for (const f of fields ?? []) { + if (f.default) defaults[f.key] = f.default; + } + return defaults; +} + function DynamicField({ field, value, @@ -242,23 +253,32 @@ export function SignUpForm({ captchaCfg.provider === "turnstile" && !!captchaCfg.site_key; - // Dynamic field values — keyed by field key. - const [fieldValues, setFieldValues] = useState>({}); - - // Initialize defaults when signup fields change. - React.useEffect(() => { - if (signupFields) { - const defaults: Record = {}; - for (const f of signupFields) { - if (f.default && !fieldValues[f.key]) { - defaults[f.key] = f.default; - } - } - if (Object.keys(defaults).length > 0) { - setFieldValues((prev) => ({ ...defaults, ...prev })); - } + // Dynamic field values — keyed by field key. Seeded from the configured + // defaults on the first render rather than from an effect afterwards, so + // the first paint already shows them. + const [fieldValues, setFieldValues] = useState>(() => + defaultsFor(signupFields), + ); + + // Re-seed when the configured fields change. This is React's documented + // "adjusting state when a prop changes" pattern rather than an effect, so + // nothing is set synchronously inside one. + // + // Seeding has to happen on the first render as well as on a change, which is + // what the initializer above covers. Keying only on the identity comparison + // here would compare equal on mount and never apply the defaults at all — + // the failure sign-up-form.test.tsx pins. + // + // `prev` deliberately wins the spread: a default may only fill a field the + // user has not set. + const [appliedFields, setAppliedFields] = useState(signupFields); + if (signupFields !== appliedFields) { + setAppliedFields(signupFields); + const defaults = defaultsFor(signupFields); + if (Object.keys(defaults).length > 0) { + setFieldValues((prev) => ({ ...defaults, ...prev })); } - }, [signupFields]); // eslint-disable-line react-hooks/exhaustive-deps + } const setFieldValue = (key: string, value: string) => { setFieldValues((prev) => ({ ...prev, [key]: value })); From 94a0a6bc1e46fcfffeefe3cedaf1c1349fdf89cf Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 22:37:13 -0500 Subject: [PATCH 5/7] fix(ui-components): send the social login return target as redirect_url This one started as the last no-explicit-any finding and turned out to be a live bug. handleSocialLogin called startOAuth with an object: client.startOAuth(providerId, { redirect_url: window.location.href }) but startOAuth takes frontend_url and redirect_url positionally and puts them in the query string. The object landed in frontend_url, the generated client ran String() over it, and the request went out as frontend_url=%5Bobject+Object%5D with no redirect_url at all. So the place you started from was never sent, and after signing in you did not come back to it. It went unnoticed because frontend_url is validated against the allowlist in plugins/social/plugin.go, "[object Object]" fails that check, and the backend falls back to a trusted Origin. The flow completes. You just land somewhere else. The any and the bug are the same problem. No honest type accepts both an object at the call site and the real (provider, string?, string?) method, which is why any was there. Typing it properly forces the call to be right. frontend_url is left unset on purpose so the backend keeps using the origin it already trusts, rather than a value this function would be asserting. The waitlist form's cast is narrowed to a named shape instead. There is no waitlist method on the generated client and no public accessor for its base URL, so it still reaches for the field, the same way ui-core's own client.ts does for /v1/client-config. Worth a getter on AuthClient at some point. --- .../src/components/waitlist-form.tsx | 7 +- .../components/src/lib/social-login.test.ts | 77 +++++++++++++++++++ .../components/src/lib/social-login.ts | 31 ++++---- 3 files changed, 100 insertions(+), 15 deletions(-) create mode 100644 ui/packages/components/src/lib/social-login.test.ts diff --git a/ui/packages/components/src/components/waitlist-form.tsx b/ui/packages/components/src/components/waitlist-form.tsx index 9772136c..4769acb1 100644 --- a/ui/packages/components/src/components/waitlist-form.tsx +++ b/ui/packages/components/src/components/waitlist-form.tsx @@ -58,8 +58,11 @@ export function WaitlistForm({ setIsSubmitting(true); try { - // Access baseURL from the client instance. - const baseURL = (client as any).baseURL ?? ""; + // There is no waitlist method on the generated client and no public + // accessor for its base URL, so this reads the field directly — the same + // reach-in that ui-core's own client.ts uses for /v1/client-config. Named + // shape rather than `any`, so what is being assumed is written down. + const baseURL = (client as unknown as { baseURL?: string }).baseURL ?? ""; const res = await fetch(baseURL + "/v1/waitlist/join", { method: "POST", headers: { "Content-Type": "application/json" }, diff --git a/ui/packages/components/src/lib/social-login.test.ts b/ui/packages/components/src/lib/social-login.test.ts new file mode 100644 index 00000000..d2169050 --- /dev/null +++ b/ui/packages/components/src/lib/social-login.test.ts @@ -0,0 +1,77 @@ +import { AuthClient } from "@authsome/ui-core"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { handleSocialLogin } from "./social-login"; +import { BASE, routedFetch } from "../test-support"; + +const realOpen = window.open; + +afterEach(() => { + window.open = realOpen; +}); + +/** A popup that never closes, so the completion poll stays parked. */ +function stubPopup(): void { + window.open = vi.fn(() => ({ closed: false }) as unknown as Window); +} + +describe("handleSocialLogin", () => { + it("sends the return target as redirect_url", async () => { + // The backend reads redirect_url (query or body) and validates + // frontend_url against the origin allowlist — see + // plugins/social/plugin.go. Passing the target in the wrong slot means + // the user is not returned where they started after signing in. + stubPopup(); + window.history.pushState({}, "", "/dashboard?next=1"); + + const { fetchFn, urls } = routedFetch({ + "POST /v1/social/github": () => ({ + auth_url: "https://github.test/login/oauth", + }), + }); + const client = new AuthClient({ baseURL: BASE, fetch: fetchFn }); + + await handleSocialLogin(client, "github", () => {}); + + const call = urls.find((u) => u.includes("/v1/social/github")); + expect(call).toBeTruthy(); + const params = new URLSearchParams(call!.split("?")[1] ?? ""); + expect(params.get("redirect_url")).toBe(window.location.href); + }); + + it("never sends a stringified object as frontend_url", async () => { + stubPopup(); + + const { fetchFn, urls } = routedFetch({ + "POST /v1/social/github": () => ({ + auth_url: "https://github.test/login/oauth", + }), + }); + const client = new AuthClient({ baseURL: BASE, fetch: fetchFn }); + + await handleSocialLogin(client, "github", () => {}); + + const call = urls.find((u) => u.includes("/v1/social/github")) ?? ""; + const params = new URLSearchParams(call.split("?")[1] ?? ""); + // The generated client runs String() over whatever it is handed, so an + // object passed into a string slot arrives as "[object Object]" — and + // frontend_url is checked against the origin allowlist. + expect(params.get("frontend_url")).not.toBe("[object Object]"); + }); + + it("reports a failed start through onError", async () => { + stubPopup(); + const { fetchFn } = routedFetch({}); + const client = new AuthClient({ baseURL: BASE, fetch: fetchFn }); + + let seen: unknown = null; + await handleSocialLogin( + client, + "github", + () => {}, + (err) => (seen = err), + ); + + expect(seen).toBeTruthy(); + }); +}); diff --git a/ui/packages/components/src/lib/social-login.ts b/ui/packages/components/src/lib/social-login.ts index 12320724..1c3e72d6 100644 --- a/ui/packages/components/src/lib/social-login.ts +++ b/ui/packages/components/src/lib/social-login.ts @@ -29,17 +29,19 @@ export function openOAuthPopup( * so the caller typically just needs to redirect/reload after completion. */ export async function handleSocialLogin( - // The codegen mis-types this endpoint on both sides: the social-OAuth - // Start{Request,Response} Go structs collide with the phone-auth pair, - // and the wrong shapes win (request gains a required `phone`, response - // loses `auth_url`). The real backend takes {app_id?, redirect_url?} - // and returns {auth_url}. The body is typed as `any` so this function - // accepts the real AuthClient; the response is cast at the assertion - // site below. + // The codegen mis-types this endpoint's response: the social-OAuth + // Start{Request,Response} Go structs collide with the phone-auth pair and + // the wrong shape wins, so the declared response loses `auth_url`. It is + // cast at the assertion site below. + // + // The request is not a body. startOAuth takes frontend_url and redirect_url + // positionally and puts them in the query string, which is what + // plugins/social/plugin.go reads (both are `query:` tagged there). client: { startOAuth: ( provider: string, - body: any, + frontendUrl?: string, + redirectUrl?: string, ) => Promise; }, providerId: string, @@ -47,11 +49,14 @@ export async function handleSocialLogin( onError?: (err: unknown) => void, ): Promise { try { - // The provider is a path parameter (already passed positionally); the - // body only carries the post-auth redirect target. - const res = (await client.startOAuth(providerId, { - redirect_url: window.location.href, - })) as { auth_url: string }; + // redirect_url is the post-auth return target. frontend_url is left unset + // so the backend falls back to the Origin/Referer it already trusts, + // rather than a value this function would be asserting. + const res = (await client.startOAuth( + providerId, + undefined, + window.location.href, + )) as { auth_url: string }; const { auth_url } = res; const popup = openOAuthPopup(auth_url); From 31b41e9ae537de0cb483ac77481487493d49907a Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 22:37:21 -0500 Subject: [PATCH 6/7] ci: run lint and tests for the ui workspace The js job installed, built and typechecked, so pnpm lint had been failing in ui-components for long enough to collect 13 findings and nothing said a word. Turbo stopped at ui-react first, which is why #94 never reached the rest. Tests go in for the same reason. A harness CI never runs is only slightly more visible than a lint CI never runs. Both pass across all five packages today. ui-core still reports nine no-explicit-any warnings and eslint exits 0 on warnings, so the step is green without them being fixed. --- .github/workflows/ci.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5f89f0eb..ae875d87 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -240,6 +240,17 @@ jobs: - name: Typecheck run: cd ui && pnpm run typecheck + # Neither of these ran here before. `pnpm lint` had been failing in + # ui-components for long enough to accumulate 13 findings, and nothing + # reported it, because this job only built and typechecked. The tests are + # added for the same reason: a harness CI never runs is only marginally + # more visible than a lint that CI never runs. + - name: Lint + run: cd ui && pnpm run lint + + - name: Test + run: cd ui && pnpm run test + - name: Build SDK TypeScript run: | cd sdk/typescript From 036fd7f8d9c8fb8441a0eb0b3539a8802d3dc514 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 22:47:43 -0500 Subject: [PATCH 7/7] fix(ui-components): trim the sub-path slashes without a regex CodeQL flagged extractSubPath as js/polynomial-redos, high. The trailing-slash trim used /\/+$/, and basePath is the caller's `path` prop rather than anything this module controls. The bad input is a run of slashes that does not end the string. The engine consumes the run from every start position, fails the $, and backtracks the whole way before shifting along one character. Measured on the old version: 46ms at 10k slashes, 1.0s at 50k, 4.2s at 100k, 16.8s at 200k. Clean quadratic. The scan that replaces it does 200k in 0.11ms. The leading-slash trim was never the problem, since /^\/+/ is anchored and only ever tries one start position, but it goes the same way for consistency. This regex is older than the harness. It was duplicated byte for byte in sign-in.tsx and sign-up.tsx and CodeQL only saw it once it moved into a file of its own. Both copies are gone now, so fixing it here fixes it everywhere. The new test asserts elapsed time rather than waiting for the runner to time out, so a regression says what it is instead of just hanging. --- .../components/src/lib/use-sub-path.test.tsx | 21 +++++++++++++++++++ .../components/src/lib/use-sub-path.ts | 19 ++++++++++++++--- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/ui/packages/components/src/lib/use-sub-path.test.tsx b/ui/packages/components/src/lib/use-sub-path.test.tsx index 3920d88b..a08ebd35 100644 --- a/ui/packages/components/src/lib/use-sub-path.test.tsx +++ b/ui/packages/components/src/lib/use-sub-path.test.tsx @@ -43,6 +43,27 @@ describe("extractSubPath", () => { it("returns undefined when the path is outside the base path", () => { expect(extractSubPath("/settings/profile", "/sign-in")).toBeUndefined(); }); + + it("normalizes a long run of trailing slashes", () => { + const base = "/sign-in" + "/".repeat(50_000); + expect(extractSubPath("/sign-in/verify-email", base)).toBe("verify-email"); + }); + + it("stays linear on a long run of slashes followed by a non-slash", () => { + // basePath is the `path` prop, so it is library input rather than + // something this module controls. Trimming the trailing slashes with + // /\/+$/ is polynomial on this shape: the engine matches \/+ up to the + // "x" from every start position in the run, fails $, and backtracks the + // whole way. Measured on the regex version: 46ms at 10k, 4.2s at 100k, + // 16.8s at 200k. CodeQL reports it as js/polynomial-redos. + const base = "/".repeat(100_000) + "x"; + + const started = performance.now(); + expect(extractSubPath("/unrelated", base)).toBeUndefined(); + const elapsed = performance.now() - started; + + expect(elapsed).toBeLessThan(250); + }); }); describe("useSubPath", () => { diff --git a/ui/packages/components/src/lib/use-sub-path.ts b/ui/packages/components/src/lib/use-sub-path.ts index 88735167..f94d1411 100644 --- a/ui/packages/components/src/lib/use-sub-path.ts +++ b/ui/packages/components/src/lib/use-sub-path.ts @@ -12,10 +12,23 @@ export function extractSubPath( pathname: string, basePath: string, ): string | undefined { - const normalized = basePath.replace(/\/+$/, ""); + // The slashes are trimmed by scanning rather than with a regex. /\/+$/ is + // polynomial on a run of slashes that does not end the string: the engine + // consumes the run from every start position, fails the $, and backtracks + // through the whole run before trying the next one. It cost 46ms at 10k + // slashes and 4.2s at 100k. basePath is the caller's `path` prop, so it is + // input this module does not control, which is why CodeQL reports it as + // js/polynomial-redos. Scanning does the same job in linear time. + let end = basePath.length; + while (end > 0 && basePath[end - 1] === "/") end--; + const normalized = basePath.slice(0, end); + if (!pathname.startsWith(normalized)) return undefined; - const rest = pathname.slice(normalized.length).replace(/^\/+/, ""); - return rest || undefined; + + let start = normalized.length; + while (start < pathname.length && pathname[start] === "/") start++; + + return pathname.slice(start) || undefined; } /**