From f4180b3fa7045682babad4ad55a6b680d6d2ec1c Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Thu, 12 Nov 2026 15:42:07 +0530 Subject: [PATCH 1/3] UN-4313 [FEAT] Add the v1 data layer: one HTTP client, server state and error contract - services/http.ts: one axios client; CSRF on same-origin mutating requests, a request id on every request, ApiError rejections, one redirect for concurrent 401s. - services/context.ts: the org id and session-expiry hook UN-4314 sets. - services/errors.ts and report.ts: one error shape, form field mapping, one reporter; app/query.ts wires it into the v1 QueryClient. - QueryView pattern, page contract, sample adapters service on the demo page. - lint:v1 rejects backend URLs, axios and the client outside services/. Co-Authored-By: Claude Opus 5.5 --- frontend/scripts/check-v1.mjs | 39 +++ frontend/src/v1/README.md | 16 +- frontend/src/v1/app/V1App.tsx | 32 ++- frontend/src/v1/app/VersionSwitch.test.tsx | 10 +- frontend/src/v1/app/query.test.tsx | 239 ++++++++++++++++ frontend/src/v1/app/query.ts | 50 ++++ .../v1/components/patterns/QueryView.test.tsx | 85 ++++++ .../src/v1/components/patterns/QueryView.tsx | 40 +++ frontend/src/v1/pages/demo/index.test.tsx | 28 ++ frontend/src/v1/pages/demo/index.tsx | 34 ++- frontend/src/v1/services/README.md | 94 +++++++ frontend/src/v1/services/adapters.test.ts | 77 ++++++ frontend/src/v1/services/adapters.ts | 90 ++++++ frontend/src/v1/services/context.test.ts | 68 +++++ frontend/src/v1/services/context.ts | 52 ++++ frontend/src/v1/services/errors.test.ts | 261 ++++++++++++++++++ frontend/src/v1/services/errors.ts | 248 +++++++++++++++++ frontend/src/v1/services/http.test.ts | 163 +++++++++++ frontend/src/v1/services/http.ts | 89 ++++++ frontend/src/v1/services/page.ts | 18 ++ frontend/src/v1/services/report.test.ts | 68 +++++ frontend/src/v1/services/report.ts | 42 +++ frontend/src/v1/test/README.md | 5 +- frontend/src/v1/test/fixtures/adapters.ts | 74 +++++ frontend/src/v1/test/providers.tsx | 15 +- .../v1-boundary/boundary.fixtures.mjs | 9 + .../src/v1/hooks/useRun.ts | 1 + .../src/v1/services/users.ts | 13 + .../src/v1/pages/demo/users.ts | 1 + .../src/v1/shell/flags.ts | 1 + .../src/v1/pages/demo/users.test.ts | 5 + .../src/v1/components/patterns/load.ts | 3 + .../src/v1/stores/load.ts | 2 + .../src/v1/pages/demo/load.ts | 3 + 34 files changed, 1948 insertions(+), 27 deletions(-) create mode 100644 frontend/src/v1/app/query.test.tsx create mode 100644 frontend/src/v1/app/query.ts create mode 100644 frontend/src/v1/components/patterns/QueryView.test.tsx create mode 100644 frontend/src/v1/components/patterns/QueryView.tsx create mode 100644 frontend/src/v1/pages/demo/index.test.tsx create mode 100644 frontend/src/v1/services/README.md create mode 100644 frontend/src/v1/services/adapters.test.ts create mode 100644 frontend/src/v1/services/adapters.ts create mode 100644 frontend/src/v1/services/context.test.ts create mode 100644 frontend/src/v1/services/context.ts create mode 100644 frontend/src/v1/services/errors.test.ts create mode 100644 frontend/src/v1/services/errors.ts create mode 100644 frontend/src/v1/services/http.test.ts create mode 100644 frontend/src/v1/services/http.ts create mode 100644 frontend/src/v1/services/page.ts create mode 100644 frontend/src/v1/services/report.test.ts create mode 100644 frontend/src/v1/services/report.ts create mode 100644 frontend/src/v1/test/fixtures/adapters.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-backend-url-deployment/src/v1/hooks/useRun.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-backend-url-services-ok/src/v1/services/users.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-backend-url-string/src/v1/pages/demo/users.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-backend-url-template/src/v1/shell/flags.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-backend-url-test-ok/src/v1/pages/demo/users.test.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-http-client-axios/src/v1/components/patterns/load.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-http-client-dynamic/src/v1/stores/load.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-http-client-module/src/v1/pages/demo/load.ts diff --git a/frontend/scripts/check-v1.mjs b/frontend/scripts/check-v1.mjs index 843425f8ae..f971fd821b 100755 --- a/frontend/scripts/check-v1.mjs +++ b/frontend/scripts/check-v1.mjs @@ -551,6 +551,40 @@ function* notifyEntryPoints(ast, specs) { } } +// P0-T05: backend URLs and the HTTP client only in services/; tests may name URLs for mocks. +const BACKEND_URL_HOMES = new Set(["services", "test"]); +// v1 calls the backend on its own origin, so its URLs are root-relative. +const BACKEND_URL = /^\/(api|deployment)\//; +const HTTP_CLIENT = /^(axios(\/|$)|@v1\/services\/http(\.ts)?$)/; +const BACKEND_URL_MSG = + "Backend URLs appear only in services/; call a typed service function instead."; +const HTTP_CLIENT_MSG = + "Only services/ imports the HTTP client (axios, @v1/services/http); call a typed service function instead."; + +/* The text a string or template starts with, or undefined for other nodes. */ +function literalStart(node) { + if (node.type === "StringLiteral") { + return node.value; + } + return node.type === "TemplateLiteral" + ? node.quasis[0].value.cooked + : undefined; +} + +/* HTTP-client imports and backend URL literals, as { index, msg }. */ +function* backendCalls(ast, specs) { + for (const arg of specs) { + if (HTTP_CLIENT.test(arg.spec ?? "")) { + yield { index: arg.index, msg: HTTP_CLIENT_MSG }; + } + } + for (const node of nodes(ast.program)) { + if (BACKEND_URL.test(literalStart(node) ?? "")) { + yield { index: node.start, msg: BACKEND_URL_MSG }; + } + } +} + /* PropTypes and antd `ant-` class names, as { index, msg }. */ function* bannedUses(ast, src) { for (const node of nodes(ast.program)) { @@ -883,6 +917,11 @@ for (const rel of walk(ROOT)) { report(rel, lineAt(src, index), msg); } } + if (!isTestFile && !BACKEND_URL_HOMES.has(folder)) { + for (const { index, msg } of backendCalls(parsed.ast, specs)) { + report(rel, lineAt(src, index), msg); + } + } for (const { index, msg } of bannedUses(parsed.ast, src)) { report(rel, lineAt(src, index), msg); } diff --git a/frontend/src/v1/README.md b/frontend/src/v1/README.md index 59a7ae7f19..d03b41e4f0 100644 --- a/frontend/src/v1/README.md +++ b/frontend/src/v1/README.md @@ -114,6 +114,18 @@ is convention. - `CodeEditor` uses Monaco from its default CDN, as v0 does. Never call `loader.config()` in v1: the loader is shared with v0. +## Data layer + +- Server state goes through TanStack Query with the typed functions in + `services/`; `services/README.md` has the contract. `V1App` mounts the one + QueryClient (`app/query.ts`), which reports every failure. +- Render a read with `QueryView` (`components/patterns/`): skeleton, inline + error with Retry, then the data. Forms map field errors with + `mapFieldErrors()` from `@v1/services/errors`. +- `lint:v1` rejects a string or template starting `/api/` or `/deployment/`, + and any import of axios or `@v1/services/http`, outside `services/`, + `test/` and test files. + ## Version switch and route registry v0's router (`routes/Router.jsx`) wraps its routes in `VersionSwitch`, which @@ -173,7 +185,7 @@ Every folder may import from itself. Test files (`*.test.ts(x)`, | `components/patterns/` | Composites built from core components | `components/ui/`, `lib/`, `services/`, `stores/`, `hooks/`, `assets/` | | `shell/` | Top bar, rail, context bar, tab bar | `components/ui/`, `components/patterns/`, `services/`, `stores/`, `hooks/`, `lib/`, `assets/` | | `pages//` | One folder per target page | `components/ui/`, `components/patterns/`, `shell/`, `services/`, `stores/`, `hooks/`, `lib/`, `styles/`, `assets/`, `plugins/` — not other pages, `app/`, `test/` or the entry | -| `services/` | Typed service modules; the only place a URL appears | `lib/` | +| `services/` | Typed service modules and the HTTP client; the only place a backend URL appears (see `services/README.md`) | `lib/` | | `stores/` | Zustand stores | `lib/` | | `hooks/` | Shared hooks | `lib/` | | `lib/` | Pure helpers | — | @@ -187,7 +199,7 @@ Every folder may import from itself. Test files (`*.test.ts(x)`, | Script | Checks | | --- | --- | | `bun run typecheck:v1` | TypeScript-only files, styles only in `styles/`, allowed file types only, no symlinks, folder taxonomy; then `tsc` with `src/v1/tsconfig.json` | -| `bun run lint:v1` | Biome on `src/v1/` (warnings fail), then imports in code and in `styles/` CSS (nothing outside v1, folder contract, and v0 reaching v1 only through `@v1`), PropTypes and `ant-` checks, the single notification entry point, then the styling policy (S1 to S8 above), then the icon rules | +| `bun run lint:v1` | Biome on `src/v1/` (warnings fail), then imports in code and in `styles/` CSS (nothing outside v1, folder contract, and v0 reaching v1 only through `@v1`), PropTypes and `ant-` checks, the single notification entry point, backend URLs and the HTTP client only in `services/`, then the styling policy (S1 to S8 above), then the icon rules | | `bun run tokens:v1` | Regenerates `styles/tokens.css` from `styles/tokens.json` (via `styles/tokens.ts`; runs under bun, which imports the TypeScript source directly) | | `bun run test:v1` | v1 unit tests (`vitest.v1.config.mts`, then cloud plugins with `vitest.v1-plugins.config.mts`) with v1's own coverage threshold, then the guardrail fixtures in `test-fixtures/v1-boundary/`, `test-fixtures/v1-harness/` and `test-fixtures/v1-unit-gate/` (the coverage threshold fails a run, and CI runs the rig group's config). The harness is documented in `test/README.md` | | `bun run seam:v1` | Seam guard: v1 work changes v0 (`src/` outside v1, `public/`) only at the files in `scripts/v1-seam-allowlist.json`, compared with `origin/main` | diff --git a/frontend/src/v1/app/V1App.tsx b/frontend/src/v1/app/V1App.tsx index 22ddb35053..9ac04de091 100644 --- a/frontend/src/v1/app/V1App.tsx +++ b/frontend/src/v1/app/V1App.tsx @@ -3,10 +3,13 @@ * guard. Loaded lazily by the version switch, so v1's code reaches the * browser only when a v1 page renders; its styles are in the global * stylesheet (vite.config.js sets cssCodeSplit: false). The Toaster mounts - * here, once, so `notify()` reaches the user on every v1 page. Session, - * guards (P0-T06) and the shell (P2-T01) mount here. + * here, once, so `notify()` reaches the user on every v1 page, and so does + * the one QueryClient. Session, guards (P0-T06) and the shell (P2-T01) mount + * here. */ +import { QueryClientProvider } from "@tanstack/react-query"; +import { createQueryClient } from "@v1/app/query"; import type { RegistryEntry } from "@v1/app/registry/types"; import { ThemeProvider } from "@v1/app/theme/ThemeProvider"; import { Toaster } from "@v1/components/ui/feedback"; @@ -16,21 +19,26 @@ import { PAGE_COMPONENTS } from "./pages"; export type V1AppProps = Readonly<{ entry: RegistryEntry }>; +// One cache for the v1 tree, kept across v1 pages. +const queryClient = createQueryClient(); + export function V1App({ entry }: V1AppProps): JSX.Element { const Page = PAGE_COMPONENTS[entry.page]; if (!Page) { throw new Error(`v1 page ${entry.page} is registered but has no component`); } return ( - - - - {entry.v1Routes.map((path) => ( - } /> - ))} - - - - + + + + + {entry.v1Routes.map((path) => ( + } /> + ))} + + + + + ); } diff --git a/frontend/src/v1/app/VersionSwitch.test.tsx b/frontend/src/v1/app/VersionSwitch.test.tsx index d326254e5a..97a50d7ecd 100644 --- a/frontend/src/v1/app/VersionSwitch.test.tsx +++ b/frontend/src/v1/app/VersionSwitch.test.tsx @@ -6,6 +6,7 @@ import { buildLiveRegistry, type LiveRegistry } from "@v1/app/registry/live"; import { toast } from "@v1/lib/notify"; import { oldVersionStorageKey } from "@v1/lib/oldVersion"; import { useOldVersionStore } from "@v1/stores/oldVersion"; +import { HttpResponse, http as mock, mockApi } from "@v1/test"; import { useEffect } from "react"; import { MemoryRouter, @@ -15,7 +16,7 @@ import { useLocation, useNavigate, } from "react-router-dom"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; import { VersionSwitch } from "./VersionSwitch"; // Two test-only pages beside the real demo ones: one that fails to render on @@ -102,11 +103,18 @@ function renderAt( const where = () => screen.getByTestId("where").textContent; +// Load the lazy v1 modules once, so a test's wait is not spent loading them. +beforeAll(() => Promise.all([import("./V1App"), import("@v1/pages/demo")])); + beforeEach(() => { visited = []; window.localStorage.clear(); window.sessionStorage.clear(); useOldVersionStore.setState({ userId: null, oldVersion: null, chosen: {} }); + // The demo page lists adapters (UN-4313). + mockApi( + mock.get("/api/v1/unstract/:org/adapter/", () => HttpResponse.json([])), + ); }); describe("VersionSwitch", () => { diff --git a/frontend/src/v1/app/query.test.tsx b/frontend/src/v1/app/query.test.tsx new file mode 100644 index 0000000000..f2282bc772 --- /dev/null +++ b/frontend/src/v1/app/query.test.tsx @@ -0,0 +1,239 @@ +import { useMutation, useQuery } from "@tanstack/react-query"; +import { act, waitFor } from "@testing-library/react"; +import { type NotifyInput, setNotifier } from "@v1/lib/notify"; +import { + adapterKeys, + adapterQueries, + listAdapters, +} from "@v1/services/adapters"; +import { setOrganizationId } from "@v1/services/context"; +import { ApiError } from "@v1/services/errors"; +import { + apiPath, + DEFAULT_SESSION, + delay, + HttpResponse, + http as mock, + mockApi, + renderHookV1, +} from "@v1/test"; +import { adapterFixtures, adapterHandlers } from "@v1/test/fixtures/adapters"; +import { isCancel } from "axios"; +import type { JsonBodyType } from "msw"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createQueryClient, retryRead } from "./query"; + +const ADAPTERS = apiPath(DEFAULT_SESSION, "/adapter/"); +const LLM = { adapterType: "LLM" } as const; + +let shown: NotifyInput[]; +let unregister: () => void; + +beforeEach(() => { + setOrganizationId(DEFAULT_SESSION.organization.id); + shown = []; + unregister = setNotifier({ + show: (input) => { + shown.push(input); + return "toast-1"; + }, + dismiss: () => undefined, + }); +}); +afterEach(() => { + unregister(); + setOrganizationId(null); +}); + +/* A handler that answers `status` and counts its calls. */ +function failing(status: number, body: JsonBodyType = {}) { + const calls = { count: 0 }; + mockApi( + mock.all(ADAPTERS, () => { + calls.count += 1; + return HttpResponse.json(body, { + status, + headers: { "X-Request-ID": "req-7" }, + }); + }), + ); + return calls; +} + +describe("retryRead", () => { + it("retries transient failures up to twice", () => { + const network = new ApiError({ kind: "network", message: "m" }); + expect(retryRead(0, network)).toBe(true); + expect(retryRead(1, network)).toBe(true); + expect(retryRead(2, network)).toBe(false); + expect( + retryRead( + 0, + new ApiError({ kind: "not_found", message: "m", status: 404 }), + ), + ).toBe(false); + }); +}); + +describe("reads", () => { + it("retry a gateway error, then report it", async () => { + vi.spyOn(console, "warn").mockImplementation(() => undefined); + const calls = failing(503); + const queryClient = createQueryClient({ queries: { retryDelay: 0 } }); + const { result } = renderHookV1(() => useQuery(adapterQueries.list(LLM)), { + queryClient, + }); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(calls.count).toBe(3); + }); + + it("do not retry a client error", async () => { + vi.spyOn(console, "warn").mockImplementation(() => undefined); + const calls = failing(404); + const queryClient = createQueryClient({ queries: { retryDelay: 0 } }); + const { result } = renderHookV1(() => useQuery(adapterQueries.list(LLM)), { + queryClient, + }); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(calls.count).toBe(1); + }); + + it("with no data are left to the page and logged with the request id", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + failing(500, { error: "Database down" }); + const { result } = renderHookV1(() => useQuery(adapterQueries.list(LLM))); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(shown).toEqual([]); + expect(warn).toHaveBeenCalledWith( + "[v1 api] %s (request %s)", + "Database down", + "req-7", + ); + }); + + it("toast when a refresh fails while data is on screen", async () => { + mockApi(...adapterHandlers(DEFAULT_SESSION)); + const { result } = renderHookV1(() => useQuery(adapterQueries.list(LLM))); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + failing(500, { error: "Database down" }); + await act(() => result.current.refetch()); + expect(shown).toEqual([ + expect.objectContaining({ + title: "Database down", + requestId: "req-7", + id: JSON.stringify(adapterQueries.list(LLM).queryKey), + }), + ]); + }); + + it("are cancelled when the caller unmounts, and report nothing", async () => { + mockApi( + mock.get(ADAPTERS, async () => { + await delay(100); + return HttpResponse.json([]); + }), + ); + let request: Promise | undefined; + const { unmount } = renderHookV1(() => + useQuery({ + queryKey: ["cancel-test"], + queryFn: ({ signal }) => { + request = listAdapters(LLM, { signal }); + return request; + }, + }), + ); + await waitFor(() => expect(request).toBeDefined()); + unmount(); + const error = await request?.catch((e: unknown) => e); + expect(isCancel(error)).toBe(true); + expect(shown).toEqual([]); + }); +}); + +describe("paging", () => { + it("keeps the previous page on screen while the next loads", async () => { + mockApi(...adapterHandlers(DEFAULT_SESSION)); + const name = (data?: { results: readonly { adapter_name: string }[] }) => + data?.results[0]?.adapter_name; + const { result, rerender } = renderHookV1( + ({ page }: { page: number }) => + useQuery(adapterQueries.page({ ...LLM, page, pageSize: 1 })), + { initialProps: { page: 1 } }, + ); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + expect(name(result.current.data)).toBe("OpenAI GPT-4o"); + rerender({ page: 2 }); + expect(result.current.isPlaceholderData).toBe(true); + expect(name(result.current.data)).toBe("OpenAI GPT-4o"); + await waitFor(() => expect(result.current.isPlaceholderData).toBe(false)); + expect(name(result.current.data)).toBe("Claude"); + }); +}); + +describe("invalidation", () => { + it("refetches a resource's queries in this organisation only", async () => { + const seen: URL[] = []; + mockApi(...adapterHandlers(DEFAULT_SESSION, adapterFixtures.llm, seen)); + const otherOrg = ["org-2", "adapters", "list", LLM] as const; + const { result, queryClient } = renderHookV1(() => [ + useQuery(adapterQueries.list(LLM)), + useQuery(adapterQueries.page({ ...LLM, page: 1, pageSize: 10 })), + ]); + await waitFor(() => + expect(result.current.every((query) => query.isSuccess)).toBe(true), + ); + queryClient.setQueryData(otherOrg, []); + await act(() => + queryClient.invalidateQueries({ queryKey: adapterKeys.all() }), + ); + expect(seen).toHaveLength(4); + expect(queryClient.getQueryState(otherOrg)?.isInvalidated).toBe(false); + }); +}); + +describe("mutations", () => { + const save = () => listAdapters(LLM); + + it("toast a failure with its request id", async () => { + failing(409, { detail: "Already exists." }); + const { result } = renderHookV1(() => useMutation({ mutationFn: save })); + await act(() => result.current.mutateAsync().catch(() => undefined)); + expect(shown).toEqual([ + expect.objectContaining({ + kind: "error", + title: "Already exists.", + requestId: "req-7", + }), + ]); + }); + + it("only log when the form shows the error itself", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + failing(400, { type: "validation_error", errors: [] }); + const { result } = renderHookV1(() => + useMutation({ mutationFn: save, meta: { notify: false } }), + ); + await act(() => result.current.mutateAsync().catch(() => undefined)); + expect(shown).toEqual([]); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it("fail on network loss without retrying", async () => { + const calls = { count: 0 }; + mockApi( + mock.get(ADAPTERS, () => { + calls.count += 1; + return HttpResponse.error(); + }), + ); + const { result } = renderHookV1(() => useMutation({ mutationFn: save }), { + queryClient: createQueryClient(), + }); + await act(() => result.current.mutateAsync().catch(() => undefined)); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(result.current.error).toMatchObject({ kind: "network" }); + expect(calls.count).toBe(1); + expect(shown).toHaveLength(1); + }); +}); diff --git a/frontend/src/v1/app/query.ts b/frontend/src/v1/app/query.ts new file mode 100644 index 0000000000..afd98c02e3 --- /dev/null +++ b/frontend/src/v1/app/query.ts @@ -0,0 +1,50 @@ +import { + type DefaultOptions, + type Mutation, + MutationCache, + type Query, + QueryCache, + QueryClient, +} from "@tanstack/react-query"; +import { isTransient } from "@v1/services/errors"; +import { reportApiError } from "@v1/services/report"; + +// Server state for v1; every failure is reported once, here (services/README.md). + +const MAX_READ_RETRIES = 2; + +/** Reads retry only on transient failures; mutations never retry. */ +export function retryRead(failureCount: number, error: unknown): boolean { + return failureCount < MAX_READ_RETRIES && isTransient(error); +} + +function reportQueryError( + error: Error, + query: Query, +): void { + // A failed refresh keeps stale data on screen, so only then a toast. + const notify = query.state.data !== undefined && query.meta?.notify !== false; + reportApiError(error, { notify, id: query.queryHash }); +} + +function reportMutationError( + error: Error, + _variables: unknown, + _onMutateResult: unknown, + mutation: Mutation, +): void { + reportApiError(error, { notify: mutation.meta?.notify !== false }); +} + +/** The v1 QueryClient; `defaults` override the default options (tests). */ +export function createQueryClient(defaults: DefaultOptions = {}): QueryClient { + return new QueryClient({ + queryCache: new QueryCache({ onError: reportQueryError }), + mutationCache: new MutationCache({ onError: reportMutationError }), + defaultOptions: { + ...defaults, + queries: { retry: retryRead, ...defaults.queries }, + mutations: { retry: false, ...defaults.mutations }, + }, + }); +} diff --git a/frontend/src/v1/components/patterns/QueryView.test.tsx b/frontend/src/v1/components/patterns/QueryView.test.tsx new file mode 100644 index 0000000000..cc079267f6 --- /dev/null +++ b/frontend/src/v1/components/patterns/QueryView.test.tsx @@ -0,0 +1,85 @@ +import { useQuery } from "@tanstack/react-query"; +import { screen } from "@testing-library/react"; +import { adapterQueries } from "@v1/services/adapters"; +import { setOrganizationId } from "@v1/services/context"; +import { + apiPath, + DEFAULT_SESSION, + delay, + HttpResponse, + http as mock, + mockApi, + renderV1, +} from "@v1/test"; +import { adapterFixtures, adapterHandlers } from "@v1/test/fixtures/adapters"; +import type { JSX } from "react"; +import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { QueryView } from "./QueryView"; + +const ADAPTERS = apiPath(DEFAULT_SESSION, "/adapter/"); + +function Names(): JSX.Element { + const query = useQuery(adapterQueries.list({ adapterType: "LLM" })); + return ( + + {(rows) =>

{rows.map((row) => row.adapter_name).join(", ")}

} + + ); +} + +beforeEach(() => setOrganizationId(DEFAULT_SESSION.organization.id)); +afterEach(() => setOrganizationId(null)); + +it("shows a skeleton while loading, then the data", async () => { + mockApi( + mock.get(ADAPTERS, async () => { + await delay(20); + return HttpResponse.json(adapterFixtures.llm); + }), + ); + const { container } = renderV1(); + expect(container.querySelector("[data-slot='skeleton-text']")).not.toBeNull(); + expect(await screen.findByText("OpenAI GPT-4o, Claude")).toBeInTheDocument(); +}); + +it("shows the error with its request id, and Retry fetches again", async () => { + vi.spyOn(console, "warn").mockImplementation(() => undefined); + mockApi( + mock.get( + ADAPTERS, + () => + HttpResponse.json( + { error: "Database down" }, + { status: 500, headers: { "X-Request-ID": "req-42" } }, + ), + { once: true }, + ), + ...adapterHandlers(DEFAULT_SESSION), + ); + const { user, container } = renderV1(); + const alert = await screen.findByRole("alert"); + expect(alert).toHaveTextContent("Database down"); + expect(alert).toHaveTextContent("Request ID req-42"); + await expect(container).toHaveNoA11yViolations(); + await user.click(screen.getByRole("button", { name: "Retry" })); + expect(await screen.findByText("OpenAI GPT-4o, Claude")).toBeInTheDocument(); +}); + +it("takes a custom loading view", () => { + mockApi( + mock.get(ADAPTERS, async () => { + await delay(20); + return HttpResponse.json([]); + }), + ); + function Custom(): JSX.Element { + const query = useQuery(adapterQueries.list({ adapterType: "LLM" })); + return ( + Loading adapters

}> + {() => null} + + ); + } + renderV1(); + expect(screen.getByText("Loading adapters")).toBeInTheDocument(); +}); diff --git a/frontend/src/v1/components/patterns/QueryView.tsx b/frontend/src/v1/components/patterns/QueryView.tsx new file mode 100644 index 0000000000..2b999ce18c --- /dev/null +++ b/frontend/src/v1/components/patterns/QueryView.tsx @@ -0,0 +1,40 @@ +import type { UseQueryResult } from "@tanstack/react-query"; +import { InlineCode } from "@v1/components/ui/code"; +import { InlineAlert, SkeletonText } from "@v1/components/ui/feedback"; +import { toApiError } from "@v1/services/errors"; +import type { JSX, ReactNode } from "react"; + +export type QueryViewProps = Readonly<{ + query: UseQueryResult; + /** Shown while loading; default three skeleton lines. */ + loading?: ReactNode; + children: (data: T) => ReactNode; +}>; + +/** + * Renders a query's three states: a skeleton while loading, an InlineAlert + * with Retry when it failed with no data, and `children(data)` otherwise. + */ +export function QueryView({ + query, + loading, + children, +}: QueryViewProps): JSX.Element { + if (query.status === "pending") { + return <>{loading ?? }; + } + if (query.data === undefined) { + const error = toApiError(query.error); + return ( + void query.refetch()}> + {error.requestId ? ( + <> + Request ID{" "} + {error.requestId} + + ) : null} + + ); + } + return <>{children(query.data)}; +} diff --git a/frontend/src/v1/pages/demo/index.test.tsx b/frontend/src/v1/pages/demo/index.test.tsx new file mode 100644 index 0000000000..45e4fb441c --- /dev/null +++ b/frontend/src/v1/pages/demo/index.test.tsx @@ -0,0 +1,28 @@ +import { screen } from "@testing-library/react"; +import { setOrganizationId } from "@v1/services/context"; +import { DEFAULT_SESSION, mockApi, renderV1 } from "@v1/test"; +import { adapterFixtures, adapterHandlers } from "@v1/test/fixtures/adapters"; +import { afterEach, expect, it } from "vitest"; +import { DemoPage } from "./index"; + +const ORG = DEFAULT_SESSION.organization.id; +const at = { route: `/${ORG}/v1-demo`, path: "/:orgName/v1-demo" }; + +afterEach(() => setOrganizationId(null)); + +it("lists the organisation's LLM adapters through the sample service", async () => { + const seen: URL[] = []; + mockApi(...adapterHandlers(DEFAULT_SESSION, adapterFixtures.llm, seen)); + const { container } = renderV1(, at); + expect(await screen.findByText("OpenAI GPT-4o")).toBeInTheDocument(); + expect(screen.getByText("Claude")).toBeInTheDocument(); + expect(seen[0]?.pathname).toBe(`/api/v1/unstract/${ORG}/adapter/`); + expect(seen[0]?.searchParams.get("adapter_type")).toBe("LLM"); + await expect(container).toHaveNoA11yViolations(); +}); + +it("says so when there are none", async () => { + mockApi(...adapterHandlers(DEFAULT_SESSION, [])); + renderV1(, at); + expect(await screen.findByText("No LLM adapters yet.")).toBeInTheDocument(); +}); diff --git a/frontend/src/v1/pages/demo/index.tsx b/frontend/src/v1/pages/demo/index.tsx index c3c873fa49..bcc6f9cdda 100644 --- a/frontend/src/v1/pages/demo/index.tsx +++ b/frontend/src/v1/pages/demo/index.tsx @@ -2,17 +2,47 @@ * Development-only demo pages that prove the version switch (UN-4271). They * ship in every build but are reachable only with VITE_V1_DEV_ROUTES=true. * DemoReviewPage stands in for Human Review: its "Switch to old version" - * control is the shape of the one P6-T07 builds. + * control is the shape of the one P6-T07 builds. DemoPage also lists the + * organisation's LLM adapters, the sample of the data layer (UN-4313). */ +import { useQuery } from "@tanstack/react-query"; +import { QueryView } from "@v1/components/patterns/QueryView"; +import { adapterQueries } from "@v1/services/adapters"; +import { setOrganizationId } from "@v1/services/context"; import { useOldVersionStore } from "@v1/stores/oldVersion"; import type { JSX } from "react"; import { Link, useParams } from "react-router-dom"; +function LlmAdapters(): JSX.Element { + const adapters = useQuery(adapterQueries.list({ adapterType: "LLM" })); + return ( + + {(rows) => + rows.length === 0 ? ( +

No LLM adapters yet.

+ ) : ( +
    + {rows.map((row) => ( +
  • {row.adapter_name}
    • + ))} +
+ ) + } + + ); +} + export function DemoPage(): JSX.Element { + // Stand-in until UN-4314's session: the URL's org name is the org id on OSS only. + setOrganizationId(useParams().orgName ?? null); return ( -
+

v1 demo page

+
+

LLM adapters

+ +
); } diff --git a/frontend/src/v1/services/README.md b/frontend/src/v1/services/README.md new file mode 100644 index 0000000000..936b8b7f1c --- /dev/null +++ b/frontend/src/v1/services/README.md @@ -0,0 +1,94 @@ +# Services: the v1 data layer + +One way to call the backend (P0-T05). Pages read and write server state through +TanStack Query with the typed functions here, and never write their own loading +or error handling. + +## Files + +| File | Holds | +| --- | --- | +| `http.ts` | The one HTTP client. Adds `X-CSRFToken` (from the `csrftoken` cookie) to same-origin POST, PUT, PATCH and DELETE, and a new `X-Request-ID` to every request; rejects with an `ApiError`; treats a 401 as session expiry, once for any number of concurrent 401s. Only `services/` imports it, or axios (`lint:v1`). | +| `context.ts` | What the client takes from the session: `setOrganizationId()` (org-scoped paths) and `onSessionExpired()` (default: v0's `/api/v1/logout` redirect). The session (P0-T06, UN-4314) sets both. | +| `errors.ts` | `ApiError`, `toApiError()`, `isCancelled()`, `mapFieldErrors()`. | +| `report.ts` | `reportApiError()`: a toast through `notify()`, or a log line with the request id. | +| `page.ts` | `Page` (the backend's `{count, next, previous, results}`), `PageParams` and `pageQuery()`. | +| `.ts` | One module per backend resource, e.g. `adapters.ts`. | + +## Writing a service module + +- Backend URLs appear only here (`lint:v1`). Build org-scoped ones with + `orgPath("/adapter/")`, which gives `/api/v1/unstract//adapter/`, and + write the trailing slash exactly as the backend route has it. +- Every function takes typed params and `RequestOptions` (`{ signal }`), passes + the signal to the client, and returns a typed response. Never accept a + free-form query object. +- A paged endpoint takes `PageParams` (`page` from 1, `pageSize`), sends them + with `pageQuery()`, and returns `Page`. Its query sets + `placeholderData: keepPreviousData`, so the previous page stays on screen + while the next loads (see `adapterQueries.page`). +- Export query keys and options per resource. Keys are + `[org, resource, "list" | "page" | "detail", params | id]`, so two + organisations never share a cache entry: + +```ts +export const adapterKeys = { + all: () => [currentOrganizationId(), "adapters"] as const, + list: (params: ListAdaptersParams) => [...adapterKeys.all(), "list", params] as const, +}; +export const adapterQueries = { + list: (params: ListAdaptersParams) => + queryOptions({ + queryKey: adapterKeys.list(params), + queryFn: ({ signal }) => listAdapters(params, { signal }), + }), +}; +``` + +- Invalidate a resource with `queryClient.invalidateQueries({ queryKey: adapterKeys.all() })`. +- Fixtures go in `test/fixtures/.ts`, typed against the module's types + (see `test/fixtures/adapters.ts`). + +## Errors + +Every failure becomes an `ApiError` with `kind`, `message` (for people), +`status`, `fieldErrors`, `formErrors` and `requestId`. A cancelled request +rejects with axios's `CanceledError` instead; check `isCancelled()` first. + +| Kind | When | +| --- | --- | +| `validation` | `type: "validation_error"` (400); fields in `fieldErrors`, the rest in `formErrors` | +| `subscription` | 402, or a `subscription_error` / `subscription_inactive` body | +| `unauthenticated` | 401 | +| `forbidden` | 403 | +| `not_found` | 404 | +| `client` | Any other 4xx | +| `server` | 5xx | +| `network` | No response | +| `unknown` | Anything that is not an HTTP failure | + +## How failures reach the user + +`app/query.ts` reports every failure once, so pages do not: + +| Failure | Shown as | +| --- | --- | +| Mutation | A toast with the request id. Pass `meta: { notify: false }` when the form shows it; it is then logged. | +| Read with no data yet | Inline, by `QueryView` (`components/patterns/`): a skeleton, then an `InlineAlert` with Retry and the request id. Also logged. | +| Read that fails while data is on screen | A toast, replaced rather than stacked while the same query keeps failing. | +| 401 | One redirect to sign in; no toast. | +| Subscription | A toast that stays until dismissed. | +| Cancellation | Nothing. | + +Reads retry twice on a network error, 502, 503 or 504. Mutations never retry. + +## Forms + +```ts +const save = useMutation({ mutationFn: createAdapter, meta: { notify: false } }); +const { fields, form } = mapFieldErrors(save.error, ["adapter_name", "description"]); +``` + +`fields` holds one message per field the form shows. `form` holds everything +else, including errors for fields the form does not show, for one +`InlineAlert` above the form; nothing is dropped. diff --git a/frontend/src/v1/services/adapters.test.ts b/frontend/src/v1/services/adapters.test.ts new file mode 100644 index 0000000000..3df0b5db41 --- /dev/null +++ b/frontend/src/v1/services/adapters.test.ts @@ -0,0 +1,77 @@ +import { DEFAULT_SESSION, makeSession, mockApi } from "@v1/test"; +import { adapterFixtures, adapterHandlers } from "@v1/test/fixtures/adapters"; +import { afterEach, expect, it } from "vitest"; +import { + adapterKeys, + adapterQueries, + listAdapters, + listAdaptersPage, +} from "./adapters"; +import { setOrganizationId } from "./context"; + +afterEach(() => setOrganizationId(null)); + +it("lists the organisation's adapters of one type, with typed params", async () => { + setOrganizationId(DEFAULT_SESSION.organization.id); + const seen: URL[] = []; + mockApi(...adapterHandlers(DEFAULT_SESSION, adapterFixtures.llm, seen)); + const rows = await listAdapters({ adapterType: "LLM", search: "gpt" }); + expect(rows).toEqual(adapterFixtures.llm); + expect(seen[0]?.pathname).toBe("/api/v1/unstract/org-1/adapter/"); + expect(Object.fromEntries(seen[0]?.searchParams ?? [])).toEqual({ + adapter_type: "LLM", + search: "gpt", + }); +}); + +it("sends no search parameter when none is given", async () => { + setOrganizationId(DEFAULT_SESSION.organization.id); + const seen: URL[] = []; + mockApi(...adapterHandlers(DEFAULT_SESSION, [], seen)); + await listAdapters({ adapterType: "OCR" }); + expect(seen[0]?.search).toBe("?adapter_type=OCR"); +}); + +it("fetches one page with typed page params", async () => { + setOrganizationId(DEFAULT_SESSION.organization.id); + const seen: URL[] = []; + mockApi(...adapterHandlers(DEFAULT_SESSION, adapterFixtures.llm, seen)); + const page = await listAdaptersPage({ + adapterType: "LLM", + page: 2, + pageSize: 1, + }); + expect(Object.fromEntries(seen[0]?.searchParams ?? [])).toEqual({ + adapter_type: "LLM", + page: "2", + page_size: "1", + }); + expect(page).toMatchObject({ + count: 2, + next: null, + results: [adapterFixtures.llm[1]], + failing_count: 0, + }); + expect(page.previous).toContain("page=1"); +}); + +it("keys queries by organisation, so two organisations never share a cache entry", () => { + const params = { adapterType: "LLM" } as const; + setOrganizationId("org-1"); + const first = adapterQueries.list(params).queryKey; + setOrganizationId( + makeSession({ organization: { id: "org-2" } }).organization.id, + ); + const second = adapterQueries.list(params).queryKey; + expect(first).toEqual(["org-1", "adapters", "list", params]); + expect(second).toEqual(["org-2", "adapters", "list", params]); + expect(adapterKeys.all()).toEqual(["org-2", "adapters"]); + expect( + adapterQueries.page({ ...params, page: 1, pageSize: 10 }).queryKey, + ).toEqual([ + "org-2", + "adapters", + "page", + { ...params, page: 1, pageSize: 10 }, + ]); +}); diff --git a/frontend/src/v1/services/adapters.ts b/frontend/src/v1/services/adapters.ts new file mode 100644 index 0000000000..dcd6505220 --- /dev/null +++ b/frontend/src/v1/services/adapters.ts @@ -0,0 +1,90 @@ +import { keepPreviousData, queryOptions } from "@tanstack/react-query"; +import { currentOrganizationId, orgPath } from "./context"; +import { http, type RequestOptions } from "./http"; +import { type Page, type PageParams, pageQuery } from "./page"; + +/* Adapters (LLM, embedding, vector DB, text extractor, OCR) in the current organisation. */ + +export const ADAPTER_TYPES = [ + "LLM", + "EMBEDDING", + "VECTOR_DB", + "X2TEXT", + "OCR", +] as const; +export type AdapterType = (typeof ADAPTER_TYPES)[number]; + +/** A row of `GET adapter/`; the fields v1 reads. */ +export type Adapter = Readonly<{ + id: string; + adapter_id: string; + adapter_name: string; + adapter_type: AdapterType; + description: string | null; + icon: string; + created_at: string; + modified_at: string; +}>; + +export type ListAdaptersParams = Readonly<{ + adapterType: AdapterType; + search?: string; +}>; + +export type ListAdaptersPageParams = ListAdaptersParams & PageParams; + +/** A page of adapters; the endpoint adds the failing count across all pages. */ +export type AdapterPage = Page & Readonly<{ failing_count: number }>; + +const filterQuery = (params: ListAdaptersParams) => ({ + adapter_type: params.adapterType, + search: params.search, +}); + +/** `GET /api/v1/unstract//adapter/`, unpaged. */ +export async function listAdapters( + params: ListAdaptersParams, + { signal }: RequestOptions = {}, +): Promise { + const { data } = await http.get(orgPath("/adapter/"), { + params: filterQuery(params), + signal, + }); + return data; +} + +/** The same list, one page at a time. */ +export async function listAdaptersPage( + params: ListAdaptersPageParams, + { signal }: RequestOptions = {}, +): Promise { + const { data } = await http.get(orgPath("/adapter/"), { + params: { ...filterQuery(params), ...pageQuery(params) }, + signal, + }); + return data; +} + +/** Query keys: `[org, "adapters", "list" | "page" | "detail", params | id]`. */ +export const adapterKeys = { + all: () => [currentOrganizationId(), "adapters"] as const, + list: (params: ListAdaptersParams) => + [...adapterKeys.all(), "list", params] as const, + page: (params: ListAdaptersPageParams) => + [...adapterKeys.all(), "page", params] as const, +}; + +export const adapterQueries = { + list: (params: ListAdaptersParams) => + queryOptions({ + queryKey: adapterKeys.list(params), + queryFn: ({ signal }) => listAdapters(params, { signal }), + }), + // The previous page stays on screen while the next one loads. + page: (params: ListAdaptersPageParams) => + queryOptions({ + queryKey: adapterKeys.page(params), + queryFn: ({ signal }) => listAdaptersPage(params, { signal }), + placeholderData: keepPreviousData, + }), +}; diff --git a/frontend/src/v1/services/context.test.ts b/frontend/src/v1/services/context.test.ts new file mode 100644 index 0000000000..44f9cc56ba --- /dev/null +++ b/frontend/src/v1/services/context.test.ts @@ -0,0 +1,68 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + currentOrganizationId, + expireSession, + onSessionExpired, + orgPath, + setOrganizationId, +} from "./context"; + +afterEach(() => { + setOrganizationId(null); +}); + +describe("orgPath", () => { + it("builds the org-scoped path and keeps the trailing slash as written", () => { + setOrganizationId("org-1"); + expect(orgPath("/adapter/")).toBe("/api/v1/unstract/org-1/adapter/"); + expect(orgPath("/adapter/info/a1/")).toBe( + "/api/v1/unstract/org-1/adapter/info/a1/", + ); + expect(orgPath("/flags")).toBe("/api/v1/unstract/org-1/flags"); + }); + + it("encodes the organisation id, so it stays one segment", () => { + setOrganizationId("a/b c"); + expect(orgPath("/x/")).toBe("/api/v1/unstract/a%2Fb%20c/x/"); + }); + + it("throws while no organisation is set", () => { + expect(() => orgPath("/adapter/")).toThrow(/No organisation is set/); + setOrganizationId(""); + expect(() => currentOrganizationId()).toThrow(/No organisation is set/); + }); +}); + +describe("session expiry", () => { + it("sends the browser to the logout URL by default", () => { + const assign = vi + .spyOn(window.location, "assign") + .mockImplementation(() => undefined); + expireSession(); + expect(assign).toHaveBeenCalledWith("/api/v1/logout"); + }); + + it("runs a registered handler instead, until its cleanup", () => { + const assign = vi + .spyOn(window.location, "assign") + .mockImplementation(() => undefined); + const handler = vi.fn(); + const cleanup = onSessionExpired(handler); + expireSession(); + expect(handler).toHaveBeenCalledTimes(1); + expect(assign).not.toHaveBeenCalled(); + cleanup(); + expireSession(); + expect(assign).toHaveBeenCalledTimes(1); + }); + + it("keeps a newer handler when an older one's cleanup runs", () => { + const older = onSessionExpired(vi.fn()); + const newer = vi.fn(); + const cleanup = onSessionExpired(newer); + older(); + expireSession(); + expect(newer).toHaveBeenCalledTimes(1); + cleanup(); + }); +}); diff --git a/frontend/src/v1/services/context.ts b/frontend/src/v1/services/context.ts new file mode 100644 index 0000000000..633fcdbfcd --- /dev/null +++ b/frontend/src/v1/services/context.ts @@ -0,0 +1,52 @@ +// What the HTTP client takes from the session (UN-4314): the org and expiry handling. + +// v0's target too: the backend ends the session and lands on the login page. +const SESSION_EXPIRED_URL = "/api/v1/logout"; + +export type SessionExpiredHandler = () => void; + +const leaveForLogin: SessionExpiredHandler = () => { + window.location.assign(SESSION_EXPIRED_URL); +}; + +let sessionExpiredHandler: SessionExpiredHandler = leaveForLogin; +let organizationId: string | null = null; + +/** Sets the organisation org-scoped paths use. */ +export function setOrganizationId(id: string | null): void { + organizationId = id; +} + +/** The current organisation id; throws while none is set. */ +export function currentOrganizationId(): string { + if (!organizationId) { + throw new Error( + "No organisation is set; the session sets it with setOrganizationId().", + ); + } + return organizationId; +} + +/** `/api/v1/unstract/` + path, the trailing slash kept as written. */ +export function orgPath(path: `/${string}`): string { + const org = encodeURIComponent(currentOrganizationId()); + return `/api/v1/unstract/${org}${path}`; +} + +/** + * Replaces what happens on session expiry (default: v0's logout redirect). + * Returns a cleanup that restores the default if this handler is current. + */ +export function onSessionExpired(handler: SessionExpiredHandler): () => void { + sessionExpiredHandler = handler; + return () => { + if (sessionExpiredHandler === handler) { + sessionExpiredHandler = leaveForLogin; + } + }; +} + +/** Runs the current session-expiry handler; the client calls it once. */ +export function expireSession(): void { + sessionExpiredHandler(); +} diff --git a/frontend/src/v1/services/errors.test.ts b/frontend/src/v1/services/errors.test.ts new file mode 100644 index 0000000000..130ecce439 --- /dev/null +++ b/frontend/src/v1/services/errors.test.ts @@ -0,0 +1,261 @@ +import { CancelledError } from "@tanstack/react-query"; +import { HttpResponse, http as mock, mockApi } from "@v1/test"; +import { CanceledError } from "axios"; +import type { JsonBodyType } from "msw"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { onSessionExpired } from "./context"; +import { + ApiError, + isCancelled, + isTransient, + mapFieldErrors, + REQUEST_ID_HEADER, + toApiError, +} from "./errors"; +import { createHttpClient } from "./http"; + +const URL = "/api/v1/thing/"; +const SENT_ID = "sent-1"; + +/* What the client rejects with when the backend answers `response`. */ +async function failureFor(response: Response | (() => Response)) { + mockApi( + mock.all(URL, typeof response === "function" ? response : () => response), + ); + const headers = { [REQUEST_ID_HEADER]: SENT_ID }; + const error = await createHttpClient() + .post(URL, {}, { headers }) + .catch((e: unknown) => e); + expect(error).toBeInstanceOf(ApiError); + return error as ApiError; +} + +const json = (body: JsonBodyType, status: number, headers?: HeadersInit) => + HttpResponse.json(body, { status, headers }); + +let cleanup: () => void; +beforeEach(() => { + // A 401 would otherwise send the test's window to the logout URL. + cleanup = onSessionExpired(vi.fn()); +}); +afterEach(() => cleanup()); + +describe("each kind, from the backend's own error bodies", () => { + it("validation: field and non-field errors, all in the message", async () => { + const error = await failureFor( + json( + { + type: "validation_error", + errors: [ + { + code: "required", + detail: "This field is required.", + attr: "adapter_name", + }, + { + code: "blank", + detail: "May not be blank.", + attr: "adapter_name", + }, + { + code: "invalid", + detail: "Name already taken.", + attr: "non_field_errors", + }, + ], + }, + 400, + ), + ); + expect(error).toMatchObject({ + kind: "validation", + status: 400, + fieldErrors: { + adapter_name: ["This field is required.", "May not be blank."], + }, + formErrors: ["Name already taken."], + }); + expect(error.message).toBe( + "adapter_name: This field is required. adapter_name: May not be blank. Name already taken.", + ); + }); + + it("subscription: the plugin's 402 body", async () => { + const error = await failureFor( + json( + { + type: "subscription_error", + errors: "Trial period expired. Please contact sales/admin.", + }, + 402, + ), + ); + expect(error).toMatchObject({ + kind: "subscription", + message: "Trial period expired. Please contact sales/admin.", + }); + }); + + it("subscription: a 400 whose error code is subscription_error", async () => { + const error = await failureFor( + json( + { + type: "client_error", + errors: [ + { + code: "subscription_error", + detail: "Selected organization is not active.", + attr: null, + }, + ], + }, + 400, + ), + ); + expect(error).toMatchObject({ + kind: "subscription", + message: "Selected organization is not active.", + }); + }); + + it("unauthenticated: the auth middleware's 401", async () => { + const error = await failureFor(json({ message: "Unauthorized" }, 401)); + expect(error).toMatchObject({ kind: "unauthenticated", status: 401 }); + }); + + it("forbidden: the auth middleware's 403", async () => { + const error = await failureFor( + json({ message: "Organization access denied" }, 403), + ); + expect(error).toMatchObject({ + kind: "forbidden", + message: "Organization access denied", + }); + }); + + it("not found: the resource-named 404", async () => { + const error = await failureFor( + json( + { + type: "client_error", + errors: [ + { code: "not_found", detail: "Adapter not found.", attr: null }, + ], + }, + 404, + ), + ); + expect(error).toMatchObject({ + kind: "not_found", + message: "Adapter not found.", + }); + }); + + it("client: any other 4xx", async () => { + const error = await failureFor(json({ detail: "Already exists." }, 409)); + expect(error).toMatchObject({ kind: "client", message: "Already exists." }); + expect(error.fieldErrors).toEqual({}); + }); + + it("server: a non-JSON 502 gets the default message and the echoed request id", async () => { + const error = await failureFor( + new HttpResponse("Bad Gateway", { + status: 502, + headers: { "Content-Type": "text/html", [REQUEST_ID_HEADER]: "srv-9" }, + }), + ); + expect(error).toMatchObject({ + kind: "server", + status: 502, + message: "Something went wrong on our side. Try again.", + requestId: "srv-9", + }); + }); + + it("server: the SDK's error text", async () => { + const error = await failureFor( + json({ error: "LLM call failed", type: "sdk_error" }, 500), + ); + expect(error).toMatchObject({ kind: "server", message: "LLM call failed" }); + }); + + it("network: no response, with the request id that was sent", async () => { + const error = await failureFor(() => HttpResponse.error()); + expect(error).toMatchObject({ + kind: "network", + status: undefined, + requestId: SENT_ID, + }); + }); + + it("unknown: anything that is not an HTTP failure", () => { + const cause = new TypeError("x is undefined"); + expect(toApiError(cause)).toMatchObject({ + kind: "unknown", + message: "Something went wrong.", + cause, + }); + }); + + it("returns an ApiError as it is", () => { + const error = new ApiError({ kind: "client", message: "m" }); + expect(toApiError(error)).toBe(error); + }); +}); + +describe("isCancelled and isTransient", () => { + it("recognises axios and TanStack cancellations", () => { + expect(isCancelled(new CanceledError())).toBe(true); + expect(isCancelled(new CancelledError())).toBe(true); + expect(isCancelled(new ApiError({ kind: "network", message: "m" }))).toBe( + false, + ); + }); + + it("calls only network and gateway failures transient", () => { + const failure = (kind: ApiError["kind"], status?: number) => + new ApiError({ kind, message: "m", status }); + expect(isTransient(failure("network"))).toBe(true); + expect(isTransient(failure("server", 503))).toBe(true); + expect(isTransient(failure("server", 500))).toBe(false); + expect(isTransient(failure("validation", 400))).toBe(false); + expect(isTransient(new CanceledError())).toBe(false); + }); +}); + +describe("mapFieldErrors", () => { + const validation = new ApiError({ + kind: "validation", + message: "all", + fieldErrors: { adapter_name: ["Required."], metadata: ["Bad JSON."] }, + formErrors: ["Name already taken."], + }); + + it("maps shown fields and moves the rest into the form-level message", () => { + expect(mapFieldErrors(validation, ["adapter_name", "description"])).toEqual( + { + fields: { adapter_name: "Required." }, + form: "Name already taken. metadata: Bad JSON.", + }, + ); + }); + + it("leaves no form-level message when every error has a field", () => { + const fieldsOnly = new ApiError({ + kind: "validation", + message: "m", + fieldErrors: { adapter_name: ["Required."] }, + }); + expect(mapFieldErrors(fieldsOnly, ["adapter_name"])).toEqual({ + fields: { adapter_name: "Required." }, + form: undefined, + }); + }); + + it("puts any other failure in the form-level message", () => { + expect(mapFieldErrors(new TypeError("x"), ["adapter_name"])).toEqual({ + fields: {}, + form: "Something went wrong.", + }); + }); +}); diff --git a/frontend/src/v1/services/errors.ts b/frontend/src/v1/services/errors.ts new file mode 100644 index 0000000000..01fa80433e --- /dev/null +++ b/frontend/src/v1/services/errors.ts @@ -0,0 +1,248 @@ +import { isCancelledError } from "@tanstack/react-query"; +import { type AxiosError, AxiosHeaders, isAxiosError, isCancel } from "axios"; + +// The error contract: every failed request becomes one ApiError (see README.md). + +export const REQUEST_ID_HEADER = "X-Request-ID"; + +export const API_ERROR_KINDS = [ + "validation", + "subscription", + "unauthenticated", + "forbidden", + "not_found", + "client", + "server", + "network", + "unknown", +] as const; +export type ApiErrorKind = (typeof API_ERROR_KINDS)[number]; + +const DEFAULT_MESSAGES: Readonly> = { + validation: "Some fields need attention.", + subscription: "Your plan does not allow this. Contact your admin.", + unauthenticated: "Your session has expired. Sign in again.", + forbidden: "You don't have permission to do this.", + not_found: "Not found.", + client: "The request could not be completed.", + server: "Something went wrong on our side. Try again.", + network: "Can't reach the server. Check your connection and try again.", + unknown: "Something went wrong.", +}; + +export type FieldErrors = Readonly>; + +export type ApiErrorInit = Readonly<{ + kind: ApiErrorKind; + message: string; + status?: number; + fieldErrors?: FieldErrors; + formErrors?: readonly string[]; + requestId?: string; + cause?: unknown; +}>; + +/** One failed request: what kind it is, a message for people, and its request id. */ +export class ApiError extends Error { + readonly kind: ApiErrorKind; + readonly status: number | undefined; + /** Validation errors by field (`attr`), as the backend names it. */ + readonly fieldErrors: FieldErrors; + /** Validation errors not tied to a field. */ + readonly formErrors: readonly string[]; + readonly requestId: string | undefined; + + constructor(init: ApiErrorInit) { + super(init.message, { cause: init.cause }); + this.name = "ApiError"; + this.kind = init.kind; + this.status = init.status; + this.fieldErrors = init.fieldErrors ?? {}; + this.formErrors = init.formErrors ?? []; + this.requestId = init.requestId; + } +} + +type ErrorItem = Readonly<{ code?: string; detail: string; attr?: string }>; + +// drf-standardized-errors, or a `{message}` / `{error}` / `{detail}` body. +type ErrorBody = Readonly<{ + type?: string; + items: readonly ErrorItem[]; + text?: string; +}>; + +const NON_FIELD = "non_field_errors"; +const GATEWAY_STATUSES = new Set([502, 503, 504]); +const SUBSCRIPTION_TYPES = new Set([ + "subscription_error", + "subscription_inactive", +]); + +const isRecord = (value: unknown): value is Record => + typeof value === "object" && value !== null && !Array.isArray(value); + +const text = (value: unknown): string | undefined => + typeof value === "string" && value !== "" ? value : undefined; + +function toItems(value: unknown): ErrorItem[] { + const detail = isRecord(value) ? text(value.detail) : undefined; + if (!isRecord(value) || detail === undefined) { + return []; + } + return [{ code: text(value.code), detail, attr: text(value.attr) }]; +} + +function parseBody(data: unknown): ErrorBody { + if (!isRecord(data)) { + return { items: [] }; + } + const { errors } = data; + return { + type: text(data.type), + items: Array.isArray(errors) ? errors.flatMap(toItems) : [], + text: + text(data.error) ?? + text(data.detail) ?? + text(data.message) ?? + text(errors), + }; +} + +const isSubscription = (status: number, body: ErrorBody): boolean => + status === 402 || + SUBSCRIPTION_TYPES.has(body.type ?? "") || + body.items.some((item) => SUBSCRIPTION_TYPES.has(item.code ?? "")); + +function kindOf(status: number, body: ErrorBody): ApiErrorKind { + if (status === 401) { + return "unauthenticated"; + } + if (isSubscription(status, body)) { + return "subscription"; + } + if (body.type === "validation_error") { + return "validation"; + } + if (status === 403) { + return "forbidden"; + } + if (status === 404) { + return "not_found"; + } + return status >= 500 ? "server" : "client"; +} + +const isFieldItem = (item: ErrorItem): item is ErrorItem & { attr: string } => + item.attr !== undefined && item.attr !== NON_FIELD; + +const describe = (item: ErrorItem): string => + isFieldItem(item) ? `${item.attr}: ${item.detail}` : item.detail; + +function splitItems(items: readonly ErrorItem[]) { + const fieldErrors: Record = {}; + const formErrors: string[] = []; + for (const item of items) { + if (isFieldItem(item)) { + fieldErrors[item.attr] = [...(fieldErrors[item.attr] ?? []), item.detail]; + } else { + formErrors.push(item.detail); + } + } + return { fieldErrors, formErrors }; +} + +function messageOf(kind: ApiErrorKind, body: ErrorBody): string { + if (kind === "validation") { + // Every error, so a toast drops none of them. + const all = body.items.map(describe).join(" "); + return all || body.text || DEFAULT_MESSAGES.validation; + } + return body.text ?? body.items[0]?.detail ?? DEFAULT_MESSAGES[kind]; +} + +// axios hands both request and response headers over as AxiosHeaders. +const requestIdIn = (headers: unknown): string | undefined => + headers instanceof AxiosHeaders + ? text(headers.get(REQUEST_ID_HEADER)) + : undefined; + +/* The id the backend echoed, else the one the client sent. */ +const requestIdOf = (error: AxiosError): string | undefined => + requestIdIn(error.response?.headers) ?? requestIdIn(error.config?.headers); + +function fromAxios(error: AxiosError): ApiError { + const requestId = requestIdOf(error); + if (!error.response) { + const message = DEFAULT_MESSAGES.network; + return new ApiError({ kind: "network", message, requestId, cause: error }); + } + const { status, data } = error.response; + const body = parseBody(data); + const kind = kindOf(status, body); + return new ApiError({ + kind, + status, + message: messageOf(kind, body), + ...(kind === "validation" ? splitItems(body.items) : {}), + requestId, + cause: error, + }); +} + +/** True for a request the caller cancelled; it is not a failure. */ +export function isCancelled(error: unknown): boolean { + return isCancel(error) || isCancelledError(error); +} + +/** Maps any failure to an ApiError. Check isCancelled() first. */ +export function toApiError(error: unknown): ApiError { + if (error instanceof ApiError) { + return error; + } + if (isAxiosError(error)) { + return fromAxios(error); + } + const message = DEFAULT_MESSAGES.unknown; + return new ApiError({ kind: "unknown", message, cause: error }); +} + +/** Failures worth retrying for a read: no response, or a gateway error. */ +export function isTransient(error: unknown): boolean { + if (isCancelled(error)) { + return false; + } + const { kind, status } = toApiError(error); + return kind === "network" || GATEWAY_STATUSES.has(status ?? 0); +} + +export type FormErrors = Readonly<{ + /** One message per field the form shows. */ + fields: Partial>; + /** Everything else, for one message above the form; never dropped. */ + form: string | undefined; +}>; + +/** Splits a failure into the form's field errors and one form-level message for the rest. */ +export function mapFieldErrors( + error: unknown, + fields: readonly F[], +): FormErrors { + const apiError = toApiError(error); + const shown = new Set(fields); + const isShown = (attr: string): attr is F => shown.has(attr); + const mapped: Partial> = {}; + const rest = [...apiError.formErrors]; + for (const [attr, details] of Object.entries(apiError.fieldErrors)) { + if (isShown(attr)) { + mapped[attr] = details.join(" "); + } else { + rest.push(`${attr}: ${details.join(" ")}`); + } + } + if (rest.length > 0) { + return { fields: mapped, form: rest.join(" ") }; + } + const anyField = Object.keys(mapped).length > 0; + return { fields: mapped, form: anyField ? undefined : apiError.message }; +} diff --git a/frontend/src/v1/services/http.test.ts b/frontend/src/v1/services/http.test.ts new file mode 100644 index 0000000000..b3f044c1e0 --- /dev/null +++ b/frontend/src/v1/services/http.test.ts @@ -0,0 +1,163 @@ +import { delay, HttpResponse, http as mock, mockApi } from "@v1/test"; +import { isCancel } from "axios"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { onSessionExpired } from "./context"; +import { ApiError, REQUEST_ID_HEADER } from "./errors"; +import { createHttpClient, http } from "./http"; + +const URL = "/api/v1/echo/"; +const UUID_V4 = + /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/; + +type Echo = { csrf: string | null; requestId: string | null }; + +function mockEcho(): void { + mockApi( + mock.all(URL, ({ request }) => + HttpResponse.json({ + csrf: request.headers.get("X-CSRFToken"), + requestId: request.headers.get(REQUEST_ID_HEADER), + }), + ), + ); +} + +/* The browser's cookie string, as the client reads it. */ +function setCookies(cookies: string): void { + vi.spyOn(document, "cookie", "get").mockReturnValue(cookies); +} + +describe("CSRF header", () => { + beforeEach(() => { + setCookies("sessionid=s1; csrftoken=tok-123; theme=dark"); + mockEcho(); + }); + + it.each([ + "post", + "put", + "patch", + "delete", + ] as const)("is sent on %s", async (method) => { + const { data } = await http.request({ url: URL, method }); + expect(data.csrf).toBe("tok-123"); + }); + + it("is not sent to another origin", async () => { + const other = "https://other.example/api/v1/echo/"; + mockApi( + mock.post(other, ({ request }) => + HttpResponse.json({ + csrf: request.headers.get("X-CSRFToken"), + requestId: null, + }), + ), + ); + const { data } = await http.post(other); + expect(data.csrf).toBeNull(); + }); + + it("is not sent on get", async () => { + const { data } = await http.get(URL); + expect(data.csrf).toBeNull(); + }); + + it("is not sent when there is no csrftoken cookie, or it is malformed", async () => { + setCookies("sessionid=s1"); + expect((await http.post(URL)).data.csrf).toBeNull(); + setCookies("csrftoken=%E0%A4%A"); + expect((await http.post(URL)).data.csrf).toBeNull(); + }); +}); + +describe("request id header", () => { + beforeEach(mockEcho); + + it("is a new UUID on every request", async () => { + const first = (await http.get(URL)).data.requestId; + const second = (await http.get(URL)).data.requestId; + expect(first).toMatch(UUID_V4); + expect(second).toMatch(UUID_V4); + expect(first).not.toBe(second); + }); + + it("is a UUID outside a secure context, where randomUUID is missing", async () => { + const { getRandomValues } = crypto; + vi.stubGlobal("crypto", { getRandomValues: getRandomValues.bind(crypto) }); + const { data } = await http.get(URL); + expect(data.requestId).toMatch(UUID_V4); + }); + + it("keeps an id the caller set", async () => { + const headers = { [REQUEST_ID_HEADER]: "mine" }; + const { data } = await http.get(URL, { headers }); + expect(data.requestId).toBe("mine"); + }); +}); + +describe("failures", () => { + it("reject with an ApiError carrying the request id that was sent", async () => { + mockApi(mock.get(URL, () => HttpResponse.text("boom", { status: 500 }))); + const headers = { [REQUEST_ID_HEADER]: "sent-1" }; + const error = await http.get(URL, { headers }).catch((e: unknown) => e); + expect(error).toBeInstanceOf(ApiError); + expect(error).toMatchObject({ kind: "server", requestId: "sent-1" }); + }); +}); + +describe("401", () => { + it("expires the session once for concurrent 401s, and rejects each", async () => { + mockApi( + mock.get(URL, () => + HttpResponse.json({ message: "Unauthorized" }, { status: 401 }), + ), + ); + const expired = vi.fn(); + const cleanup = onSessionExpired(expired); + const client = createHttpClient(); + const results = await Promise.allSettled([ + client.get(URL), + client.get(URL), + ]); + cleanup(); + expect(expired).toHaveBeenCalledTimes(1); + for (const result of results) { + expect(result).toMatchObject({ + status: "rejected", + reason: { kind: "unauthenticated" }, + }); + } + }); + + it("is not treated as expiry for other statuses", async () => { + mockApi(mock.get(URL, () => HttpResponse.json({}, { status: 403 }))); + const expired = vi.fn(); + const cleanup = onSessionExpired(expired); + await createHttpClient() + .get(URL) + .catch(() => undefined); + cleanup(); + expect(expired).not.toHaveBeenCalled(); + }); +}); + +describe("cancellation", () => { + it("rejects as cancelled, not as an ApiError, when the signal aborts", async () => { + mockApi( + mock.get(URL, async () => { + await delay(50); + return HttpResponse.json({}); + }), + ); + const expired = vi.fn(); + const cleanup = onSessionExpired(expired); + const controller = new AbortController(); + const pending = http.get(URL, { signal: controller.signal }); + controller.abort(); + const error = await pending.catch((e: unknown) => e); + cleanup(); + expect(isCancel(error)).toBe(true); + expect(error).not.toBeInstanceOf(ApiError); + expect(expired).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/v1/services/http.ts b/frontend/src/v1/services/http.ts new file mode 100644 index 0000000000..60130894fc --- /dev/null +++ b/frontend/src/v1/services/http.ts @@ -0,0 +1,89 @@ +import axios, { + type AxiosInstance, + type InternalAxiosRequestConfig, + isCancel, +} from "axios"; +import { expireSession } from "./context"; +import { REQUEST_ID_HEADER, toApiError } from "./errors"; + +// The one HTTP client for v1; only services/ imports it (see README.md). + +const CSRF_COOKIE = "csrftoken"; +const CSRF_HEADER = "X-CSRFToken"; +const MUTATING_METHODS = new Set(["post", "put", "patch", "delete"]); + +/** Options every service function takes. */ +export type RequestOptions = Readonly<{ signal?: AbortSignal }>; + +function readCookie(name: string): string | undefined { + const prefix = `${name}=`; + const entry = document.cookie + .split("; ") + .find((part) => part.startsWith(prefix)); + if (entry === undefined) { + return undefined; + } + try { + return decodeURIComponent(entry.slice(prefix.length)); + } catch { + // A malformed value is no token: the backend refuses the request. + return undefined; + } +} + +/* A v4 UUID; randomUUID exists only in secure contexts (not plain-http on-prem). */ +function newRequestId(): string { + if (typeof crypto.randomUUID === "function") { + return crypto.randomUUID(); + } + const bytes = crypto.getRandomValues(new Uint8Array(16)); + bytes[6] = (bytes[6] & 0x0f) | 0x40; + bytes[8] = (bytes[8] & 0x3f) | 0x80; + const hex = Array.from(bytes, (b) => b.toString(16).padStart(2, "0")).join( + "", + ); + return `${hex.slice(0, 8)}-${hex.slice(8, 12)}-${hex.slice(12, 16)}-${hex.slice(16, 20)}-${hex.slice(20)}`; +} + +// The CSRF token never goes to another origin. +const isSameOrigin = (url: string | undefined): boolean => + new URL(url ?? "", window.location.href).origin === window.location.origin; + +function addHeaders( + config: InternalAxiosRequestConfig, +): InternalAxiosRequestConfig { + if (!config.headers.has(REQUEST_ID_HEADER)) { + config.headers.set(REQUEST_ID_HEADER, newRequestId()); + } + const method = (config.method ?? "get").toLowerCase(); + if (!MUTATING_METHODS.has(method) || !isSameOrigin(config.url)) { + return config; + } + const token = readCookie(CSRF_COOKIE); + if (token !== undefined) { + config.headers.set(CSRF_HEADER, token); + } + return config; +} + +/** A client with its own session-expiry latch; services use `http`. */ +export function createHttpClient(): AxiosInstance { + const client = axios.create(); + let expired = false; + client.interceptors.request.use(addHeaders); + client.interceptors.response.use(undefined, (error: unknown) => { + if (isCancel(error)) { + return Promise.reject(error); + } + const apiError = toApiError(error); + // Concurrent 401s redirect once. + if (apiError.kind === "unauthenticated" && !expired) { + expired = true; + expireSession(); + } + return Promise.reject(apiError); + }); + return client; +} + +export const http: AxiosInstance = createHttpClient(); diff --git a/frontend/src/v1/services/page.ts b/frontend/src/v1/services/page.ts new file mode 100644 index 0000000000..2eadb80ecb --- /dev/null +++ b/frontend/src/v1/services/page.ts @@ -0,0 +1,18 @@ +// The backend's paged-list contract (DRF PageNumberPagination): `?page=&page_size=`. + +/** One page of a list endpoint. */ +export type Page = Readonly<{ + /** Rows across all pages. */ + count: number; + next: string | null; + previous: string | null; + results: readonly T[]; +}>; + +/** Which page to fetch; `page` starts at 1. */ +export type PageParams = Readonly<{ page: number; pageSize: number }>; + +/** The query parameters for a page, as the backend names them. */ +export function pageQuery({ page, pageSize }: PageParams) { + return { page, page_size: pageSize } as const; +} diff --git a/frontend/src/v1/services/report.test.ts b/frontend/src/v1/services/report.test.ts new file mode 100644 index 0000000000..6ba979752d --- /dev/null +++ b/frontend/src/v1/services/report.test.ts @@ -0,0 +1,68 @@ +import { type NotifyInput, setNotifier } from "@v1/lib/notify"; +import { CanceledError } from "axios"; +import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { ApiError } from "./errors"; +import { reportApiError } from "./report"; + +let shown: NotifyInput[]; +let unregister: () => void; + +beforeEach(() => { + shown = []; + unregister = setNotifier({ + show: (input) => { + shown.push(input); + return "toast-1"; + }, + dismiss: () => undefined, + }); +}); +afterEach(() => unregister()); + +const failure = (kind: ApiError["kind"]) => + new ApiError({ kind, message: `${kind} message`, requestId: "req-1" }); + +it("shows a failure as an error toast with its request id", () => { + reportApiError(failure("server"), { id: "q1" }); + expect(shown).toEqual([ + { + kind: "error", + title: "server message", + requestId: "req-1", + persistent: false, + id: "q1", + }, + ]); +}); + +it("keeps a subscription failure until it is dismissed", () => { + reportApiError(failure("subscription")); + expect(shown[0]).toMatchObject({ persistent: true }); +}); + +it("only logs, with the request id, when the caller shows the error", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); + reportApiError(failure("validation"), { notify: false }); + expect(shown).toEqual([]); + expect(warn).toHaveBeenCalledWith( + "[v1 api] %s (request %s)", + "validation message", + "req-1", + ); +}); + +it("reports nothing for a cancellation or an expired session", () => { + reportApiError(new CanceledError()); + reportApiError(failure("unauthenticated")); + expect(shown).toEqual([]); +}); + +it("logs the cause of an unexpected failure and still shows it", () => { + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + const cause = new TypeError("boom"); + reportApiError(cause); + expect(error).toHaveBeenCalledWith("[v1 api] unexpected failure", cause); + expect(shown).toEqual([ + expect.objectContaining({ title: "Something went wrong." }), + ]); +}); diff --git a/frontend/src/v1/services/report.ts b/frontend/src/v1/services/report.ts new file mode 100644 index 0000000000..c0f68b9439 --- /dev/null +++ b/frontend/src/v1/services/report.ts @@ -0,0 +1,42 @@ +import { notify } from "@v1/lib/notify"; +import { isCancelled, toApiError } from "./errors"; + +export type ReportOptions = Readonly<{ + /** False when the caller shows the error itself; it is then only logged. */ + notify?: boolean; + /** Reusing an id replaces the open toast instead of stacking another. */ + id?: string; +}>; + +/** + * Reports a failed request: a toast through notify(), or a log line with its + * request id when the caller shows it. Cancellations and session expiry + * (already redirected) report nothing. + */ +export function reportApiError( + error: unknown, + options: ReportOptions = {}, +): void { + if (isCancelled(error)) { + return; + } + const apiError = toApiError(error); + if (apiError.kind === "unauthenticated") { + return; + } + if (apiError.kind === "unknown") { + console.error("[v1 api] unexpected failure", apiError.cause); + } + if (options.notify === false) { + const requestId = apiError.requestId ?? "none"; + console.warn("[v1 api] %s (request %s)", apiError.message, requestId); + return; + } + notify({ + kind: "error", + title: apiError.message, + requestId: apiError.requestId, + persistent: apiError.kind === "subscription", + id: options.id, + }); +} diff --git a/frontend/src/v1/test/README.md b/frontend/src/v1/test/README.md index f1eec5558e..4f599cd003 100644 --- a/frontend/src/v1/test/README.md +++ b/frontend/src/v1/test/README.md @@ -60,7 +60,8 @@ await user.click(screen.getByRole("button", { name: "Add" })); - `user`: a `userEvent.setup()` instance. Pass `userEventOptions`, for example `{ advanceTimers: vi.advanceTimersByTime }` with fake timers. - `queryClient`: a fresh client per render, with no retries, no refetch on - focus and no garbage collection mid-test. Pass `queryClient` to share one. + focus and no garbage collection mid-test. Pass `queryClient` to share one, + or `createQueryClient()` from `@v1/app/query` for the app's retry policy. - `portal`: the `#v1-portal` container, themed like the root. - `theme`: what the test rendered with. @@ -80,7 +81,7 @@ the app's own provider once its task has landed, and a stand-in until then: | Slot | Today | Replace with, when it lands | | --- | --- | --- | -| query | stand-in: `QueryClientProvider` over `createTestQueryClient()` | the data layer's provider (P0-T05, UN-4313) | +| query | **real**: the app's `createQueryClient()` (P0-T05, UN-4313) with test defaults, so failures are reported as in the app | — | | router | stand-in: `MemoryRouter` (+ `Routes` when `path` is set) | the app router in memory mode, with the route registry (P0-T03, UN-4271) | | theme | **real**: `app/theme/ThemeProvider` (P0-T04, #2399) | — | | session | none: no v1 code reads a session | the session store or provider, when it lands. Add `session` to `ProviderContext` and `RenderV1Options` with it | diff --git a/frontend/src/v1/test/fixtures/adapters.ts b/frontend/src/v1/test/fixtures/adapters.ts new file mode 100644 index 0000000000..29caf8810d --- /dev/null +++ b/frontend/src/v1/test/fixtures/adapters.ts @@ -0,0 +1,74 @@ +/* Fixtures for services/adapters, typed against its contract. */ + +import type { Adapter, AdapterPage } from "@v1/services/adapters"; +import { apiPath, HttpResponse, http, type TestSession } from "@v1/test"; + +export const adapterFixtures = { + llm: [ + { + id: "4f0c2a8e-1d6b-4a51-9a39-6a3c1d2b7e10", + adapter_id: "openai|502ecf49-e47c-445c-9907-6d4b90c5cd17", + adapter_name: "OpenAI GPT-4o", + adapter_type: "LLM", + description: null, + icon: "/icons/adapter-icons/OpenAI.png", + created_at: "2026-11-01T10:00:00Z", + modified_at: "2026-11-02T10:00:00Z", + }, + { + id: "9b7e5c3a-2f4d-4c1b-8e6a-0d9f8c7b6a51", + adapter_id: "anthropic|90ebd4cd-2f19-4cef-a884-9eeb6ac0f203", + adapter_name: "Claude", + adapter_type: "LLM", + description: "Default extraction model", + icon: "/icons/adapter-icons/Anthropic.png", + created_at: "2026-11-03T10:00:00Z", + modified_at: "2026-11-03T10:00:00Z", + }, + ], +} satisfies Record; + +/* The link to `page`, or null when it is out of range. */ +function pageLink(url: URL, page: number, last: number): string | null { + if (page < 1 || page > last) { + return null; + } + const link = new URL(url); + link.searchParams.set("page", String(page)); + return link.href; +} + +/* As the backend pages it: an envelope only when the caller asks for a page. */ +function answer( + url: URL, + rows: readonly Adapter[], +): readonly Adapter[] | AdapterPage { + const page = Number(url.searchParams.get("page") || 0); + const size = Number(url.searchParams.get("page_size") || 0); + if (!page && !size) { + return rows; + } + const current = page || 1; + const perPage = size || 50; + const last = Math.ceil(rows.length / perPage); + return { + count: rows.length, + next: pageLink(url, current + 1, last), + previous: pageLink(url, current - 1, last), + results: rows.slice((current - 1) * perPage, current * perPage), + failing_count: 0, + }; +} + +/** `GET adapter/` answering `rows`, paged on request; records each URL in `seen`. */ +export const adapterHandlers = ( + session: TestSession, + rows: readonly Adapter[] = adapterFixtures.llm, + seen: URL[] = [], +) => [ + http.get(apiPath(session, "/adapter/"), ({ request }) => { + const url = new URL(request.url); + seen.push(url); + return HttpResponse.json(answer(url, rows)); + }), +]; diff --git a/frontend/src/v1/test/providers.tsx b/frontend/src/v1/test/providers.tsx index 296fcead21..2012b0680e 100644 --- a/frontend/src/v1/test/providers.tsx +++ b/frontend/src/v1/test/providers.tsx @@ -1,4 +1,5 @@ -import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { type QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { createQueryClient } from "@v1/app/query"; import { ThemeProvider } from "@v1/app/theme/ThemeProvider"; import { getPortalContainer, PORTAL_CONTAINER_ID } from "@v1/lib/portal"; import { THEME_STORAGE_KEY } from "@v1/lib/theme"; @@ -12,7 +13,7 @@ import type { ThemeMode } from "./session"; * lands. A slot is the app's own provider once its task has merged, and a * stand-in until then: * - * query stand-in P0-T05 data layer (UN-4313) → the app's QueryClient provider + * query real app/query (P0-T05, UN-4313), with test defaults * router stand-in P0-T03 router + registry (UN-4271) → the app's router, in memory * theme real app/theme/ThemeProvider (P0-T04, #2399) * @@ -38,13 +39,11 @@ export type ProviderSlot = ( props: Readonly<{ context: ProviderContext; children: ReactNode }>, ) => JSX.Element; -/** A QueryClient for one test: no retries, no refetch on focus, no GC mid-test. */ +/** The app's QueryClient for one test: no retries, no refetch on focus, no GC mid-test. */ export function createTestQueryClient(): QueryClient { - return new QueryClient({ - defaultOptions: { - queries: { retry: false, gcTime: Infinity, refetchOnWindowFocus: false }, - mutations: { retry: false }, - }, + return createQueryClient({ + queries: { retry: false, gcTime: Infinity, refetchOnWindowFocus: false }, + mutations: { retry: false }, }); } diff --git a/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs b/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs index fcc736bba7..efa82ba621 100644 --- a/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs +++ b/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs @@ -116,6 +116,15 @@ const CASES = parseTable(` # Outside the feedback folder, a static named radix-ui import without Toast is fine. v1-notify-radix-named-ok | lint:v1 | - v1-notify-feedback-ok | lint:v1 | - + # One way to call the backend: URLs and the HTTP client only in services/. + v1-backend-url-string | lint:v1 | Backend URLs appear only in services/ + v1-backend-url-template | lint:v1 | Backend URLs appear only in services/ + v1-backend-url-deployment | lint:v1 | Backend URLs appear only in services/ + v1-http-client-axios | lint:v1 | Only services/ imports the HTTP client + v1-http-client-module | lint:v1 | Only services/ imports the HTTP client + v1-http-client-dynamic | lint:v1 | Only services/ imports the HTTP client + v1-backend-url-services-ok | typecheck:v1 + lint:v1 | - + v1-backend-url-test-ok | lint:v1 | - v1-explicit-any | lint:v1 | noExplicitAny # The v1 override must add to the shared rules, not replace them. v1-base-rules-still-apply | lint:v1 | noDoubleEquals diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-deployment/src/v1/hooks/useRun.ts b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-deployment/src/v1/hooks/useRun.ts new file mode 100644 index 0000000000..812b4f1b8d --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-deployment/src/v1/hooks/useRun.ts @@ -0,0 +1 @@ +export const RUN_URL = "/deployment/api/org-1/invoice/"; diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-services-ok/src/v1/services/users.ts b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-services-ok/src/v1/services/users.ts new file mode 100644 index 0000000000..663f606207 --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-services-ok/src/v1/services/users.ts @@ -0,0 +1,13 @@ +import { orgPath } from "./context"; +import { http, type RequestOptions } from "./http"; + +export type User = Readonly<{ id: string; email: string }>; + +export async function listUsers({ + signal, +}: RequestOptions = {}): Promise { + const { data } = await http.get(orgPath("/users/"), { signal }); + return data; +} + +export const LOGOUT_URL = "/api/v1/logout"; diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-string/src/v1/pages/demo/users.ts b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-string/src/v1/pages/demo/users.ts new file mode 100644 index 0000000000..cbce533dfe --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-string/src/v1/pages/demo/users.ts @@ -0,0 +1 @@ +export const loadUsers = () => fetch("/api/v1/unstract/org-1/users/"); diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-template/src/v1/shell/flags.ts b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-template/src/v1/shell/flags.ts new file mode 100644 index 0000000000..ca393083bb --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-template/src/v1/shell/flags.ts @@ -0,0 +1 @@ +export const flagsUrl = (org: string) => `/api/v1/unstract/${org}/flags/`; diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-test-ok/src/v1/pages/demo/users.test.ts b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-test-ok/src/v1/pages/demo/users.test.ts new file mode 100644 index 0000000000..465eca1ccd --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-test-ok/src/v1/pages/demo/users.test.ts @@ -0,0 +1,5 @@ +import { expect, it } from "vitest"; + +it("names the endpoint it mocks", () => { + expect("/api/v1/unstract/org-1/users/").toContain("/users/"); +}); diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-http-client-axios/src/v1/components/patterns/load.ts b/frontend/test-fixtures/v1-boundary/cases/v1-http-client-axios/src/v1/components/patterns/load.ts new file mode 100644 index 0000000000..e6d67571f3 --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-http-client-axios/src/v1/components/patterns/load.ts @@ -0,0 +1,3 @@ +import axios from "axios"; + +export const load = (url: string) => axios.get(url); diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-http-client-dynamic/src/v1/stores/load.ts b/frontend/test-fixtures/v1-boundary/cases/v1-http-client-dynamic/src/v1/stores/load.ts new file mode 100644 index 0000000000..0e397161aa --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-http-client-dynamic/src/v1/stores/load.ts @@ -0,0 +1,2 @@ +export const load = async (url: string) => + (await import("axios")).default.get(url); diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-http-client-module/src/v1/pages/demo/load.ts b/frontend/test-fixtures/v1-boundary/cases/v1-http-client-module/src/v1/pages/demo/load.ts new file mode 100644 index 0000000000..8bb7140ae2 --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-http-client-module/src/v1/pages/demo/load.ts @@ -0,0 +1,3 @@ +import { http } from "@v1/services/http"; + +export const load = (url: string) => http.get(url); From 74db2e149bdc0eb1780c2bf37665af49b01ac66c Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Thu, 12 Nov 2026 18:14:34 +0530 Subject: [PATCH 2/3] UN-4313 [FEAT] Fix the data layer review findings: typed 401s, offline, org-scoped queries, stricter lint - A typed 401 (a provider's key, a connector's OAuth) is a client error; only the auth middleware's 401 expires the session. - toApiError calls a cancel unknown, not network; mapFieldErrors gives nothing for no error or a cancel. - QueryView shows the alert only when the first fetch failed; a failed refresh keeps the data. - networkMode always: offline fails as network instead of pausing, and a save is never sent later. - Queries fetch for the org in their key; orgPath moves into http.ts with an org option. - lint:v1 matches the client under any spelling and flags fetch, XMLHttpRequest and EventSource outside services/; plugins//services/ may hold them. - Drop the unused query-level meta.notify; document the once-per-page-load expiry and fix the README rows. Co-Authored-By: Claude Opus 5.5 --- frontend/scripts/check-v1.mjs | 58 +++++++++-- frontend/src/v1/README.md | 10 +- frontend/src/v1/app/query.test.tsx | 95 ++++++++++++++++++- frontend/src/v1/app/query.ts | 8 +- .../v1/components/patterns/QueryView.test.tsx | 50 +++++++++- .../src/v1/components/patterns/QueryView.tsx | 5 +- frontend/src/v1/services/README.md | 42 ++++---- frontend/src/v1/services/adapters.test.ts | 11 +++ frontend/src/v1/services/adapters.ts | 21 ++-- frontend/src/v1/services/context.test.ts | 23 +---- frontend/src/v1/services/context.ts | 9 +- frontend/src/v1/services/errors.test.ts | 44 +++++++++ frontend/src/v1/services/errors.ts | 15 ++- frontend/src/v1/services/http.test.ts | 51 +++++++++- frontend/src/v1/services/http.ts | 20 +++- frontend/src/v1/test/README.md | 23 +---- .../v1-boundary/boundary.fixtures.mjs | 6 ++ .../src/v1/services/users.ts | 3 +- .../src/v1/pages/demo/path.ts | 3 + .../src/v1/plugins/billing/Plans.tsx | 1 + .../src/v1/plugins/billing/services/plans.ts | 10 ++ .../src/v1/shell/stream.ts | 1 + .../v1-raw-fetch/src/v1/pages/demo/load.ts | 1 + .../v1-raw-xhr/src/v1/hooks/useUpload.ts | 1 + 24 files changed, 409 insertions(+), 102 deletions(-) create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-http-client-js-ext/src/v1/pages/demo/path.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-plugin-page-backend-url/src/v1/plugins/billing/Plans.tsx create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-plugin-services-ok/src/v1/plugins/billing/services/plans.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-raw-eventsource-global/src/v1/shell/stream.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-raw-fetch/src/v1/pages/demo/load.ts create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-raw-xhr/src/v1/hooks/useUpload.ts diff --git a/frontend/scripts/check-v1.mjs b/frontend/scripts/check-v1.mjs index f971fd821b..646eea2f83 100755 --- a/frontend/scripts/check-v1.mjs +++ b/frontend/scripts/check-v1.mjs @@ -551,15 +551,56 @@ function* notifyEntryPoints(ast, specs) { } } -// P0-T05: backend URLs and the HTTP client only in services/; tests may name URLs for mocks. +// P0-T05: backend URLs and requests only in services/; tests may name URLs for mocks. const BACKEND_URL_HOMES = new Set(["services", "test"]); +// A cloud plugin keeps its own service modules in plugins//services/. +const PLUGIN_SERVICES = /^plugins\/[^/]+\/services\//; // v1 calls the backend on its own origin, so its URLs are root-relative. const BACKEND_URL = /^\/(api|deployment)\//; -const HTTP_CLIENT = /^(axios(\/|$)|@v1\/services\/http(\.ts)?$)/; +const AXIOS = /^axios(\/|$)/; +// The client and orgPath(), under any spelling of its path. +const HTTP_CLIENT_MODULE = "services/http"; +const CODE_SUFFIX = /\.[cm]?[jt]sx?$/; +// Browser APIs that skip the client's CSRF, request id and 401 handling. +const RAW_REQUESTS = new Set(["fetch", "XMLHttpRequest", "EventSource"]); +const GLOBAL_OBJECTS = new Set(["window", "globalThis", "self"]); const BACKEND_URL_MSG = "Backend URLs appear only in services/; call a typed service function instead."; const HTTP_CLIENT_MSG = "Only services/ imports the HTTP client (axios, @v1/services/http); call a typed service function instead."; +const RAW_REQUEST_MSG = + "Only services/ sends requests (fetch, XMLHttpRequest, EventSource); call a typed service function instead."; + +const isBackendHome = (folder, rel) => + BACKEND_URL_HOMES.has(folder) || PLUGIN_SERVICES.test(rel); + +function isHttpClient(spec, fileRel) { + if (AXIOS.test(spec)) { + return true; + } + if (!spec.startsWith("@v1/") && !RELATIVE.test(spec)) { + return false; + } + const target = resolveV1(spec, fileRel).replace(CODE_SUFFIX, ""); + return target === HTTP_CLIENT_MODULE; +} + +/* `fetch(...)`, `new XMLHttpRequest()`, `new window.EventSource(...)` and the like. */ +function isRawRequest(node) { + if (node.type !== "CallExpression" && node.type !== "NewExpression") { + return false; + } + const callee = unwrap(node.callee); + if (callee?.type === "Identifier") { + return RAW_REQUESTS.has(callee.name); + } + return ( + callee?.type === "MemberExpression" && + !callee.computed && + GLOBAL_OBJECTS.has(unwrap(callee.object)?.name) && + RAW_REQUESTS.has(callee.property.name) + ); +} /* The text a string or template starts with, or undefined for other nodes. */ function literalStart(node) { @@ -571,14 +612,17 @@ function literalStart(node) { : undefined; } -/* HTTP-client imports and backend URL literals, as { index, msg }. */ -function* backendCalls(ast, specs) { +/* HTTP-client imports, raw requests and backend URL literals, as { index, msg }. */ +function* backendCalls(ast, specs, fileRel) { for (const arg of specs) { - if (HTTP_CLIENT.test(arg.spec ?? "")) { + if (isHttpClient(arg.spec ?? "", fileRel)) { yield { index: arg.index, msg: HTTP_CLIENT_MSG }; } } for (const node of nodes(ast.program)) { + if (isRawRequest(node)) { + yield { index: node.start, msg: RAW_REQUEST_MSG }; + } if (BACKEND_URL.test(literalStart(node) ?? "")) { yield { index: node.start, msg: BACKEND_URL_MSG }; } @@ -917,8 +961,8 @@ for (const rel of walk(ROOT)) { report(rel, lineAt(src, index), msg); } } - if (!isTestFile && !BACKEND_URL_HOMES.has(folder)) { - for (const { index, msg } of backendCalls(parsed.ast, specs)) { + if (!isTestFile && !isBackendHome(folder, rel)) { + for (const { index, msg } of backendCalls(parsed.ast, specs, rel)) { report(rel, lineAt(src, index), msg); } } diff --git a/frontend/src/v1/README.md b/frontend/src/v1/README.md index d03b41e4f0..8c8b14b9c3 100644 --- a/frontend/src/v1/README.md +++ b/frontend/src/v1/README.md @@ -123,8 +123,9 @@ is convention. error with Retry, then the data. Forms map field errors with `mapFieldErrors()` from `@v1/services/errors`. - `lint:v1` rejects a string or template starting `/api/` or `/deployment/`, - and any import of axios or `@v1/services/http`, outside `services/`, - `test/` and test files. + any import of axios or `@v1/services/http`, and any call to `fetch`, + `XMLHttpRequest` or `EventSource`, outside `services/`, + `plugins//services/`, `test/` and test files. ## Version switch and route registry @@ -140,7 +141,8 @@ only when a v1 page first renders. `REGISTERED_PAGES`. Its v0 URLs then redirect to v1; reverting is that line. v1 renders outside v0's `PersistentLogin` and `RequireAuth`, and its own session and guards come with P0-T06: a page registered before that ships - with no session load, login redirect or org check. + with no session load or org check, and leaves for sign-in only when a + service call gets a 401 (`services/http.ts`). - `app/registry/validate.test.ts` fails CI on an invalid entry: overlapping patterns across pages, a v1 route and a different v0 route of one page that match the same URL (a page may keep its v0 URL), a route without a mapping @@ -199,7 +201,7 @@ Every folder may import from itself. Test files (`*.test.ts(x)`, | Script | Checks | | --- | --- | | `bun run typecheck:v1` | TypeScript-only files, styles only in `styles/`, allowed file types only, no symlinks, folder taxonomy; then `tsc` with `src/v1/tsconfig.json` | -| `bun run lint:v1` | Biome on `src/v1/` (warnings fail), then imports in code and in `styles/` CSS (nothing outside v1, folder contract, and v0 reaching v1 only through `@v1`), PropTypes and `ant-` checks, the single notification entry point, backend URLs and the HTTP client only in `services/`, then the styling policy (S1 to S8 above), then the icon rules | +| `bun run lint:v1` | Biome on `src/v1/` (warnings fail), then imports in code and in `styles/` CSS (nothing outside v1, folder contract, and v0 reaching v1 only through `@v1`), PropTypes and `ant-` checks, the single notification entry point, backend URLs, the HTTP client and raw requests only in `services/`, then the styling policy (S1 to S8 above), then the icon rules | | `bun run tokens:v1` | Regenerates `styles/tokens.css` from `styles/tokens.json` (via `styles/tokens.ts`; runs under bun, which imports the TypeScript source directly) | | `bun run test:v1` | v1 unit tests (`vitest.v1.config.mts`, then cloud plugins with `vitest.v1-plugins.config.mts`) with v1's own coverage threshold, then the guardrail fixtures in `test-fixtures/v1-boundary/`, `test-fixtures/v1-harness/` and `test-fixtures/v1-unit-gate/` (the coverage threshold fails a run, and CI runs the rig group's config). The harness is documented in `test/README.md` | | `bun run seam:v1` | Seam guard: v1 work changes v0 (`src/` outside v1, `public/`) only at the files in `scripts/v1-seam-allowlist.json`, compared with `origin/main` | diff --git a/frontend/src/v1/app/query.test.tsx b/frontend/src/v1/app/query.test.tsx index f2282bc772..6e5477f7ce 100644 --- a/frontend/src/v1/app/query.test.tsx +++ b/frontend/src/v1/app/query.test.tsx @@ -1,4 +1,4 @@ -import { useMutation, useQuery } from "@tanstack/react-query"; +import { onlineManager, useMutation, useQuery } from "@tanstack/react-query"; import { act, waitFor } from "@testing-library/react"; import { type NotifyInput, setNotifier } from "@v1/lib/notify"; import { @@ -20,7 +20,15 @@ import { import { adapterFixtures, adapterHandlers } from "@v1/test/fixtures/adapters"; import { isCancel } from "axios"; import type { JsonBodyType } from "msw"; -import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + type MockInstance, + vi, +} from "vitest"; import { createQueryClient, retryRead } from "./query"; const ADAPTERS = apiPath(DEFAULT_SESSION, "/adapter/"); @@ -45,6 +53,10 @@ afterEach(() => { setOrganizationId(null); }); +/* The data layer's console lines, without React Router's warnings. */ +const v1Lines = (spy: MockInstance) => + spy.mock.calls.filter(([first]) => String(first).startsWith("[v1 ")); + /* A handler that answers `status` and counts its calls. */ function failing(status: number, body: JsonBodyType = {}) { const calls = { count: 0 }; @@ -77,7 +89,7 @@ describe("retryRead", () => { describe("reads", () => { it("retry a gateway error, then report it", async () => { - vi.spyOn(console, "warn").mockImplementation(() => undefined); + const warn = vi.spyOn(console, "warn").mockImplementation(() => undefined); const calls = failing(503); const queryClient = createQueryClient({ queries: { retryDelay: 0 } }); const { result } = renderHookV1(() => useQuery(adapterQueries.list(LLM)), { @@ -85,6 +97,13 @@ describe("reads", () => { }); await waitFor(() => expect(result.current.isError).toBe(true)); expect(calls.count).toBe(3); + expect(v1Lines(warn)).toEqual([ + [ + "[v1 api] %s (request %s)", + "Something went wrong on our side. Try again.", + "req-7", + ], + ]); }); it("do not retry a client error", async () => { @@ -127,6 +146,8 @@ describe("reads", () => { }); it("are cancelled when the caller unmounts, and report nothing", async () => { + const warn = vi.spyOn(console, "warn"); + const error = vi.spyOn(console, "error"); mockApi( mock.get(ADAPTERS, async () => { await delay(100); @@ -145,9 +166,11 @@ describe("reads", () => { ); await waitFor(() => expect(request).toBeDefined()); unmount(); - const error = await request?.catch((e: unknown) => e); - expect(isCancel(error)).toBe(true); + const cancelled = await request?.catch((e: unknown) => e); + expect(isCancel(cancelled)).toBe(true); expect(shown).toEqual([]); + expect(v1Lines(warn)).toEqual([]); + expect(v1Lines(error)).toEqual([]); }); }); @@ -219,6 +242,30 @@ describe("mutations", () => { expect(warn).toHaveBeenCalledTimes(1); }); + it("report nothing for a save the caller cancelled", async () => { + const warn = vi.spyOn(console, "warn"); + const error = vi.spyOn(console, "error"); + mockApi( + mock.get(ADAPTERS, async () => { + await delay(100); + return HttpResponse.json([]); + }), + ); + const controller = new AbortController(); + const { result } = renderHookV1(() => + useMutation({ + mutationFn: () => listAdapters(LLM, { signal: controller.signal }), + }), + ); + act(() => result.current.mutate()); + controller.abort(); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(isCancel(result.current.error)).toBe(true); + expect(shown).toEqual([]); + expect(v1Lines(warn)).toEqual([]); + expect(v1Lines(error)).toEqual([]); + }); + it("fail on network loss without retrying", async () => { const calls = { count: 0 }; mockApi( @@ -237,3 +284,41 @@ describe("mutations", () => { expect(shown).toHaveLength(1); }); }); + +describe("offline", () => { + afterEach(() => onlineManager.setOnline(true)); + + it("a read fails as network instead of waiting for the connection", async () => { + vi.spyOn(console, "warn").mockImplementation(() => undefined); + onlineManager.setOnline(false); + mockApi(mock.get(ADAPTERS, () => HttpResponse.error())); + const queryClient = createQueryClient({ queries: { retryDelay: 0 } }); + const { result } = renderHookV1(() => useQuery(adapterQueries.list(LLM)), { + queryClient, + }); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(result.current.error).toMatchObject({ kind: "network" }); + }); + + it("a save fails instead of being sent when the connection returns", async () => { + onlineManager.setOnline(false); + const calls = { count: 0 }; + mockApi( + mock.get(ADAPTERS, () => { + calls.count += 1; + return HttpResponse.error(); + }), + ); + const { result } = renderHookV1( + () => useMutation({ mutationFn: () => listAdapters(LLM) }), + { queryClient: createQueryClient() }, + ); + act(() => result.current.mutate()); + await waitFor(() => expect(result.current.isError).toBe(true)); + expect(result.current.isPaused).toBe(false); + expect(calls.count).toBe(1); + expect(shown).toEqual([ + expect.objectContaining({ title: expect.stringContaining("connection") }), + ]); + }); +}); diff --git a/frontend/src/v1/app/query.ts b/frontend/src/v1/app/query.ts index afd98c02e3..93415c47e5 100644 --- a/frontend/src/v1/app/query.ts +++ b/frontend/src/v1/app/query.ts @@ -23,7 +23,7 @@ function reportQueryError( query: Query, ): void { // A failed refresh keeps stale data on screen, so only then a toast. - const notify = query.state.data !== undefined && query.meta?.notify !== false; + const notify = query.state.data !== undefined; reportApiError(error, { notify, id: query.queryHash }); } @@ -38,13 +38,15 @@ function reportMutationError( /** The v1 QueryClient; `defaults` override the default options (tests). */ export function createQueryClient(defaults: DefaultOptions = {}): QueryClient { + // "always": offline fails fast as `network` instead of pausing, and a save is never sent later. + const networkMode = "always"; return new QueryClient({ queryCache: new QueryCache({ onError: reportQueryError }), mutationCache: new MutationCache({ onError: reportMutationError }), defaultOptions: { ...defaults, - queries: { retry: retryRead, ...defaults.queries }, - mutations: { retry: false, ...defaults.mutations }, + queries: { retry: retryRead, networkMode, ...defaults.queries }, + mutations: { retry: false, networkMode, ...defaults.mutations }, }, }); } diff --git a/frontend/src/v1/components/patterns/QueryView.test.tsx b/frontend/src/v1/components/patterns/QueryView.test.tsx index cc079267f6..e3d8f0208d 100644 --- a/frontend/src/v1/components/patterns/QueryView.test.tsx +++ b/frontend/src/v1/components/patterns/QueryView.test.tsx @@ -1,5 +1,5 @@ import { useQuery } from "@tanstack/react-query"; -import { screen } from "@testing-library/react"; +import { screen, waitFor } from "@testing-library/react"; import { adapterQueries } from "@v1/services/adapters"; import { setOrganizationId } from "@v1/services/context"; import { @@ -83,3 +83,51 @@ it("takes a custom loading view", () => { renderV1(); expect(screen.getByText("Loading adapters")).toBeInTheDocument(); }); + +it("keeps the data on screen, with no alert, when a refresh fails", async () => { + const toast = vi.spyOn(console, "error").mockImplementation(() => undefined); + mockApi(...adapterHandlers(DEFAULT_SESSION)); + function Refreshable(): JSX.Element { + const query = useQuery(adapterQueries.list({ adapterType: "LLM" })); + return ( + <> +

Status {query.status}

+ + + {(rows) =>

{rows.map((row) => row.adapter_name).join(", ")}

} + + + ); + } + const { user } = renderV1(); + expect(await screen.findByText("OpenAI GPT-4o, Claude")).toBeInTheDocument(); + mockApi(mock.get(ADAPTERS, () => HttpResponse.json({}, { status: 500 }))); + await user.click(screen.getByRole("button", { name: "Refresh" })); + await waitFor(() => + expect(screen.getByText("Status error")).toBeInTheDocument(), + ); + expect(screen.getByText("OpenAI GPT-4o, Claude")).toBeInTheDocument(); + expect(screen.queryByRole("alert")).toBeNull(); + // No notifier is mounted, so the toast falls back to the console. + expect(toast).toHaveBeenCalledTimes(1); +}); + +it("renders a successful query whose select gives undefined, not an alert", async () => { + mockApi(...adapterHandlers(DEFAULT_SESSION)); + function Pick(): JSX.Element { + const query = useQuery({ + ...adapterQueries.list({ adapterType: "LLM" }), + select: (rows) => rows.find((row) => row.adapter_name === "Mistral"), + }); + return ( + + {(row) =>

{row?.adapter_name ?? "No match"}

} + + ); + } + renderV1(); + expect(await screen.findByText("No match")).toBeInTheDocument(); + expect(screen.queryByRole("alert")).toBeNull(); +}); diff --git a/frontend/src/v1/components/patterns/QueryView.tsx b/frontend/src/v1/components/patterns/QueryView.tsx index 2b999ce18c..34042a261e 100644 --- a/frontend/src/v1/components/patterns/QueryView.tsx +++ b/frontend/src/v1/components/patterns/QueryView.tsx @@ -13,7 +13,8 @@ export type QueryViewProps = Readonly<{ /** * Renders a query's three states: a skeleton while loading, an InlineAlert - * with Retry when it failed with no data, and `children(data)` otherwise. + * with Retry when the first fetch failed, and `children(data)` otherwise + * (a failed refresh keeps the data; app/query.ts toasts it). */ export function QueryView({ query, @@ -23,7 +24,7 @@ export function QueryView({ if (query.status === "pending") { return <>{loading ?? }; } - if (query.data === undefined) { + if (query.isLoadingError) { const error = toApiError(query.error); return ( void query.refetch()}> diff --git a/frontend/src/v1/services/README.md b/frontend/src/v1/services/README.md index 936b8b7f1c..ef6c29247a 100644 --- a/frontend/src/v1/services/README.md +++ b/frontend/src/v1/services/README.md @@ -8,8 +8,8 @@ or error handling. | File | Holds | | --- | --- | -| `http.ts` | The one HTTP client. Adds `X-CSRFToken` (from the `csrftoken` cookie) to same-origin POST, PUT, PATCH and DELETE, and a new `X-Request-ID` to every request; rejects with an `ApiError`; treats a 401 as session expiry, once for any number of concurrent 401s. Only `services/` imports it, or axios (`lint:v1`). | -| `context.ts` | What the client takes from the session: `setOrganizationId()` (org-scoped paths) and `onSessionExpired()` (default: v0's `/api/v1/logout` redirect). The session (P0-T06, UN-4314) sets both. | +| `http.ts` | The one HTTP client, and `orgPath()`. Adds `X-CSRFToken` (from the `csrftoken` cookie) to same-origin POST, PUT, PATCH and DELETE, and a new `X-Request-ID` to every request; rejects with an `ApiError`; treats the auth middleware's 401 as session expiry, once per page load, so concurrent 401s redirect once. Only `services/` imports it, or axios, or calls `fetch`, `XMLHttpRequest` or `EventSource` (`lint:v1`). | +| `context.ts` | What the client takes from the session: `setOrganizationId()` (the org `orgPath()` uses) and `onSessionExpired()` (default: v0's `/api/v1/logout` redirect; a handler must leave the page). The session (P0-T06, UN-4314) sets both. | | `errors.ts` | `ApiError`, `toApiError()`, `isCancelled()`, `mapFieldErrors()`. | | `report.ts` | `reportApiError()`: a toast through `notify()`, or a log line with the request id. | | `page.ts` | `Page` (the backend's `{count, next, previous, results}`), `PageParams` and `pageQuery()`. | @@ -18,18 +18,22 @@ or error handling. ## Writing a service module - Backend URLs appear only here (`lint:v1`). Build org-scoped ones with - `orgPath("/adapter/")`, which gives `/api/v1/unstract//adapter/`, and - write the trailing slash exactly as the backend route has it. -- Every function takes typed params and `RequestOptions` (`{ signal }`), passes - the signal to the client, and returns a typed response. Never accept a - free-form query object. + `orgPath("/adapter/", org)`, which gives `/api/v1/unstract//adapter/`, + and write the trailing slash exactly as the backend route has it. A cloud + plugin keeps its service modules in `plugins//services/`. +- Every function takes typed params and `RequestOptions` (`{ signal, org }`), + passes the signal to the client and the org to `orgPath()`, and returns a + typed response. Never accept a free-form query object. - A paged endpoint takes `PageParams` (`page` from 1, `pageSize`), sends them with `pageQuery()`, and returns `Page`. Its query sets `placeholderData: keepPreviousData`, so the previous page stays on screen - while the next loads (see `adapterQueries.page`). + while the next loads (see `adapterQueries.page`). A page past the end is a + 404 ("Invalid page."), so whoever holds the page number steps back to the + last page (`count`) after a delete. - Export query keys and options per resource. Keys are - `[org, resource, "list" | "page" | "detail", params | id]`, so two - organisations never share a cache entry: + `[org, resource, "list" | "page" | "detail", params | id]`, and each query + fetches for the org in its key, so two organisations never share a cache + entry: ```ts export const adapterKeys = { @@ -40,7 +44,7 @@ export const adapterQueries = { list: (params: ListAdaptersParams) => queryOptions({ queryKey: adapterKeys.list(params), - queryFn: ({ signal }) => listAdapters(params, { signal }), + queryFn: ({ queryKey: [org], signal }) => listAdapters(params, { signal, org }), }), }; ``` @@ -53,13 +57,14 @@ export const adapterQueries = { Every failure becomes an `ApiError` with `kind`, `message` (for people), `status`, `fieldErrors`, `formErrors` and `requestId`. A cancelled request -rejects with axios's `CanceledError` instead; check `isCancelled()` first. +rejects with axios's `CanceledError` instead; check `isCancelled()` first +(`toApiError()` calls it `unknown`, "The request was cancelled."). | Kind | When | | --- | --- | | `validation` | `type: "validation_error"` (400); fields in `fieldErrors`, the rest in `formErrors` | | `subscription` | 402, or a `subscription_error` / `subscription_inactive` body | -| `unauthenticated` | 401 | +| `unauthenticated` | The auth middleware's 401 (no `type` in the body); a typed 401, such as a provider rejecting its key, is `client` | | `forbidden` | 403 | | `not_found` | 404 | | `client` | Any other 4xx | @@ -74,13 +79,15 @@ rejects with axios's `CanceledError` instead; check `isCancelled()` first. | Failure | Shown as | | --- | --- | | Mutation | A toast with the request id. Pass `meta: { notify: false }` when the form shows it; it is then logged. | -| Read with no data yet | Inline, by `QueryView` (`components/patterns/`): a skeleton, then an `InlineAlert` with Retry and the request id. Also logged. | -| Read that fails while data is on screen | A toast, replaced rather than stacked while the same query keeps failing. | +| Read whose first fetch fails (also a next page shown with `keepPreviousData`) | Inline, by `QueryView` (`components/patterns/`): a skeleton, then an `InlineAlert` with Retry and the request id. Also logged. | +| Refresh that fails while the query's data is on screen | A toast, replaced rather than stacked while the same query keeps failing. | | 401 | One redirect to sign in; no toast. | -| Subscription | A toast that stays until dismissed. | +| Subscription | A toast that stays until dismissed; inline like any other first-fetch failure. | | Cancellation | Nothing. | Reads retry twice on a network error, 502, 503 or 504. Mutations never retry. +Offline, requests still go out (`networkMode: "always"`) and fail as +`network`; nothing waits for the connection, and a save is never sent later. ## Forms @@ -91,4 +98,5 @@ const { fields, form } = mapFieldErrors(save.error, ["adapter_name", "descriptio `fields` holds one message per field the form shows. `form` holds everything else, including errors for fields the form does not show, for one -`InlineAlert` above the form; nothing is dropped. +`InlineAlert` above the form; nothing is dropped. Before the first submit +(`error` is null) or after a cancel, both are empty. diff --git a/frontend/src/v1/services/adapters.test.ts b/frontend/src/v1/services/adapters.test.ts index 3df0b5db41..26a6d96b59 100644 --- a/frontend/src/v1/services/adapters.test.ts +++ b/frontend/src/v1/services/adapters.test.ts @@ -1,3 +1,4 @@ +import { QueryClient } from "@tanstack/react-query"; import { DEFAULT_SESSION, makeSession, mockApi } from "@v1/test"; import { adapterFixtures, adapterHandlers } from "@v1/test/fixtures/adapters"; import { afterEach, expect, it } from "vitest"; @@ -75,3 +76,13 @@ it("keys queries by organisation, so two organisations never share a cache entry { ...params, page: 1, pageSize: 10 }, ]); }); + +it("fetches for the organisation in the key, even after the current one changes", async () => { + setOrganizationId("org-1"); + const options = adapterQueries.list({ adapterType: "LLM" }); + setOrganizationId("org-2"); + const seen: URL[] = []; + mockApi(...adapterHandlers(DEFAULT_SESSION, adapterFixtures.llm, seen)); + await new QueryClient().fetchQuery(options); + expect(seen[0]?.pathname).toBe("/api/v1/unstract/org-1/adapter/"); +}); diff --git a/frontend/src/v1/services/adapters.ts b/frontend/src/v1/services/adapters.ts index dcd6505220..a1646f3b7a 100644 --- a/frontend/src/v1/services/adapters.ts +++ b/frontend/src/v1/services/adapters.ts @@ -1,6 +1,6 @@ import { keepPreviousData, queryOptions } from "@tanstack/react-query"; -import { currentOrganizationId, orgPath } from "./context"; -import { http, type RequestOptions } from "./http"; +import { currentOrganizationId } from "./context"; +import { http, orgPath, type RequestOptions } from "./http"; import { type Page, type PageParams, pageQuery } from "./page"; /* Adapters (LLM, embedding, vector DB, text extractor, OCR) in the current organisation. */ @@ -44,9 +44,9 @@ const filterQuery = (params: ListAdaptersParams) => ({ /** `GET /api/v1/unstract//adapter/`, unpaged. */ export async function listAdapters( params: ListAdaptersParams, - { signal }: RequestOptions = {}, + { signal, org }: RequestOptions = {}, ): Promise { - const { data } = await http.get(orgPath("/adapter/"), { + const { data } = await http.get(orgPath("/adapter/", org), { params: filterQuery(params), signal, }); @@ -56,16 +56,16 @@ export async function listAdapters( /** The same list, one page at a time. */ export async function listAdaptersPage( params: ListAdaptersPageParams, - { signal }: RequestOptions = {}, + { signal, org }: RequestOptions = {}, ): Promise { - const { data } = await http.get(orgPath("/adapter/"), { + const { data } = await http.get(orgPath("/adapter/", org), { params: { ...filterQuery(params), ...pageQuery(params) }, signal, }); return data; } -/** Query keys: `[org, "adapters", "list" | "page" | "detail", params | id]`. */ +/** Query keys: `[org, "adapters", "list" | "page", params]`. */ export const adapterKeys = { all: () => [currentOrganizationId(), "adapters"] as const, list: (params: ListAdaptersParams) => @@ -74,17 +74,20 @@ export const adapterKeys = { [...adapterKeys.all(), "page", params] as const, }; +// Each query fetches for the org in its key, so rows never land under another org's key. export const adapterQueries = { list: (params: ListAdaptersParams) => queryOptions({ queryKey: adapterKeys.list(params), - queryFn: ({ signal }) => listAdapters(params, { signal }), + queryFn: ({ queryKey: [org], signal }) => + listAdapters(params, { signal, org }), }), // The previous page stays on screen while the next one loads. page: (params: ListAdaptersPageParams) => queryOptions({ queryKey: adapterKeys.page(params), - queryFn: ({ signal }) => listAdaptersPage(params, { signal }), + queryFn: ({ queryKey: [org], signal }) => + listAdaptersPage(params, { signal, org }), placeholderData: keepPreviousData, }), }; diff --git a/frontend/src/v1/services/context.test.ts b/frontend/src/v1/services/context.test.ts index 44f9cc56ba..f6eef940e4 100644 --- a/frontend/src/v1/services/context.test.ts +++ b/frontend/src/v1/services/context.test.ts @@ -3,7 +3,6 @@ import { currentOrganizationId, expireSession, onSessionExpired, - orgPath, setOrganizationId, } from "./context"; @@ -11,25 +10,13 @@ afterEach(() => { setOrganizationId(null); }); -describe("orgPath", () => { - it("builds the org-scoped path and keeps the trailing slash as written", () => { - setOrganizationId("org-1"); - expect(orgPath("/adapter/")).toBe("/api/v1/unstract/org-1/adapter/"); - expect(orgPath("/adapter/info/a1/")).toBe( - "/api/v1/unstract/org-1/adapter/info/a1/", - ); - expect(orgPath("/flags")).toBe("/api/v1/unstract/org-1/flags"); - }); - - it("encodes the organisation id, so it stays one segment", () => { - setOrganizationId("a/b c"); - expect(orgPath("/x/")).toBe("/api/v1/unstract/a%2Fb%20c/x/"); - }); - - it("throws while no organisation is set", () => { - expect(() => orgPath("/adapter/")).toThrow(/No organisation is set/); +describe("currentOrganizationId", () => { + it("gives the organisation that is set, and throws while none is", () => { + expect(() => currentOrganizationId()).toThrow(/No organisation is set/); setOrganizationId(""); expect(() => currentOrganizationId()).toThrow(/No organisation is set/); + setOrganizationId("org-1"); + expect(currentOrganizationId()).toBe("org-1"); }); }); diff --git a/frontend/src/v1/services/context.ts b/frontend/src/v1/services/context.ts index 633fcdbfcd..5c6156d27d 100644 --- a/frontend/src/v1/services/context.ts +++ b/frontend/src/v1/services/context.ts @@ -1,6 +1,6 @@ // What the HTTP client takes from the session (UN-4314): the org and expiry handling. -// v0's target too: the backend ends the session and lands on the login page. +// v0's target too. GET goes when v0 retires (account_v2/views.py), so UN-4314's handler uses the POST sign-out. const SESSION_EXPIRED_URL = "/api/v1/logout"; export type SessionExpiredHandler = () => void; @@ -27,14 +27,9 @@ export function currentOrganizationId(): string { return organizationId; } -/** `/api/v1/unstract/` + path, the trailing slash kept as written. */ -export function orgPath(path: `/${string}`): string { - const org = encodeURIComponent(currentOrganizationId()); - return `/api/v1/unstract/${org}${path}`; -} - /** * Replaces what happens on session expiry (default: v0's logout redirect). + * The handler must leave the page: the client calls it once per page load. * Returns a cleanup that restores the default if this handler is current. */ export function onSessionExpired(handler: SessionExpiredHandler): () => void { diff --git a/frontend/src/v1/services/errors.test.ts b/frontend/src/v1/services/errors.test.ts index 130ecce439..2f575e9b40 100644 --- a/frontend/src/v1/services/errors.test.ts +++ b/frontend/src/v1/services/errors.test.ts @@ -123,6 +123,29 @@ describe("each kind, from the backend's own error bodies", () => { expect(error).toMatchObject({ kind: "unauthenticated", status: 401 }); }); + it.each([ + [ + "a provider rejecting its key", + { error: "Invalid API key.", type: "sdk_error" }, + "Invalid API key.", + ], + [ + "a connector's failed OAuth refresh", + { + type: "client_error", + errors: [{ code: "error", detail: "OAuth failed.", attr: null }], + }, + "OAuth failed.", + ], + ])("client, not session expiry: a typed 401 from %s", async (_, body, message) => { + const expired = vi.fn(); + const restore = onSessionExpired(expired); + const error = await failureFor(json(body, 401)); + restore(); + expect(error).toMatchObject({ kind: "client", status: 401, message }); + expect(expired).not.toHaveBeenCalled(); + }); + it("forbidden: the auth middleware's 403", async () => { const error = await failureFor( json({ message: "Organization access denied" }, 403), @@ -197,6 +220,16 @@ describe("each kind, from the backend's own error bodies", () => { }); }); + it("unknown, not network: a cancelled request", () => { + for (const cause of [new CanceledError(), new CancelledError()]) { + expect(toApiError(cause)).toMatchObject({ + kind: "unknown", + message: "The request was cancelled.", + cause, + }); + } + }); + it("returns an ApiError as it is", () => { const error = new ApiError({ kind: "client", message: "m" }); expect(toApiError(error)).toBe(error); @@ -252,6 +285,17 @@ describe("mapFieldErrors", () => { }); }); + it.each([ + ["an idle mutation (null)", null], + ["no error (undefined)", undefined], + ["a cancelled request", new CanceledError()], + ])("shows nothing for %s", (_, error) => { + expect(mapFieldErrors(error, ["adapter_name"])).toEqual({ + fields: {}, + form: undefined, + }); + }); + it("puts any other failure in the form-level message", () => { expect(mapFieldErrors(new TypeError("x"), ["adapter_name"])).toEqual({ fields: {}, diff --git a/frontend/src/v1/services/errors.ts b/frontend/src/v1/services/errors.ts index 01fa80433e..0672508f90 100644 --- a/frontend/src/v1/services/errors.ts +++ b/frontend/src/v1/services/errors.ts @@ -29,6 +29,7 @@ const DEFAULT_MESSAGES: Readonly> = { network: "Can't reach the server. Check your connection and try again.", unknown: "Something went wrong.", }; +const CANCELLED_MESSAGE = "The request was cancelled."; export type FieldErrors = Readonly>; @@ -115,7 +116,8 @@ const isSubscription = (status: number, body: ErrorBody): boolean => body.items.some((item) => SUBSCRIPTION_TYPES.has(item.code ?? "")); function kindOf(status: number, body: ErrorBody): ApiErrorKind { - if (status === 401) { + // Only the auth middleware's untyped 401 is session expiry; a typed one is a provider's or connector's. + if (status === 401 && body.type === undefined) { return "unauthenticated"; } if (isSubscription(status, body)) { @@ -195,11 +197,16 @@ export function isCancelled(error: unknown): boolean { return isCancel(error) || isCancelledError(error); } -/** Maps any failure to an ApiError. Check isCancelled() first. */ +/** Maps any failure to an ApiError; a cancel is `unknown`. Check isCancelled() first. */ export function toApiError(error: unknown): ApiError { if (error instanceof ApiError) { return error; } + // Before axios: its cancel has no response, so it would read as `network`. + if (isCancelled(error)) { + const message = CANCELLED_MESSAGE; + return new ApiError({ kind: "unknown", message, cause: error }); + } if (isAxiosError(error)) { return fromAxios(error); } @@ -228,6 +235,10 @@ export function mapFieldErrors( error: unknown, fields: readonly F[], ): FormErrors { + // No error yet (an idle mutation), or a cancel: nothing to show. + if (error == null || isCancelled(error)) { + return { fields: {}, form: undefined }; + } const apiError = toApiError(error); const shown = new Set(fields); const isShown = (attr: string): attr is F => shown.has(attr); diff --git a/frontend/src/v1/services/http.test.ts b/frontend/src/v1/services/http.test.ts index b3f044c1e0..5e6adfbbf9 100644 --- a/frontend/src/v1/services/http.test.ts +++ b/frontend/src/v1/services/http.test.ts @@ -1,9 +1,9 @@ import { delay, HttpResponse, http as mock, mockApi } from "@v1/test"; import { isCancel } from "axios"; -import { beforeEach, describe, expect, it, vi } from "vitest"; -import { onSessionExpired } from "./context"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { onSessionExpired, setOrganizationId } from "./context"; import { ApiError, REQUEST_ID_HEADER } from "./errors"; -import { createHttpClient, http } from "./http"; +import { createHttpClient, http, orgPath } from "./http"; const URL = "/api/v1/echo/"; const UUID_V4 = @@ -27,6 +27,34 @@ function setCookies(cookies: string): void { vi.spyOn(document, "cookie", "get").mockReturnValue(cookies); } +describe("orgPath", () => { + afterEach(() => setOrganizationId(null)); + + it("builds the org-scoped path and keeps the trailing slash as written", () => { + setOrganizationId("org-1"); + expect(orgPath("/adapter/")).toBe("/api/v1/unstract/org-1/adapter/"); + expect(orgPath("/adapter/info/a1/")).toBe( + "/api/v1/unstract/org-1/adapter/info/a1/", + ); + expect(orgPath("/flags")).toBe("/api/v1/unstract/org-1/flags"); + }); + + it("uses the organisation given, else the current one", () => { + setOrganizationId("org-1"); + expect(orgPath("/x/", "org-2")).toBe("/api/v1/unstract/org-2/x/"); + expect(orgPath("/x/", undefined)).toBe("/api/v1/unstract/org-1/x/"); + }); + + it("encodes the organisation id, so it stays one segment", () => { + setOrganizationId("a/b c"); + expect(orgPath("/x/")).toBe("/api/v1/unstract/a%2Fb%20c/x/"); + }); + + it("throws while no organisation is set", () => { + expect(() => orgPath("/adapter/")).toThrow(/No organisation is set/); + }); +}); + describe("CSRF header", () => { beforeEach(() => { setCookies("sessionid=s1; csrftoken=tok-123; theme=dark"); @@ -129,6 +157,23 @@ describe("401", () => { } }); + it("expires the session once per page load: the handler leaves the page", async () => { + mockApi( + mock.get(URL, () => + HttpResponse.json({ message: "Unauthorized" }, { status: 401 }), + ), + ); + const expired = vi.fn(); + const cleanup = onSessionExpired(expired); + const client = createHttpClient(); + const first = await client.get(URL).catch((e: unknown) => e); + const second = await client.get(URL).catch((e: unknown) => e); + cleanup(); + expect(expired).toHaveBeenCalledTimes(1); + expect(first).toMatchObject({ kind: "unauthenticated" }); + expect(second).toMatchObject({ kind: "unauthenticated" }); + }); + it("is not treated as expiry for other statuses", async () => { mockApi(mock.get(URL, () => HttpResponse.json({}, { status: 403 }))); const expired = vi.fn(); diff --git a/frontend/src/v1/services/http.ts b/frontend/src/v1/services/http.ts index 60130894fc..061fadd6d7 100644 --- a/frontend/src/v1/services/http.ts +++ b/frontend/src/v1/services/http.ts @@ -3,17 +3,29 @@ import axios, { type InternalAxiosRequestConfig, isCancel, } from "axios"; -import { expireSession } from "./context"; +import { currentOrganizationId, expireSession } from "./context"; import { REQUEST_ID_HEADER, toApiError } from "./errors"; -// The one HTTP client for v1; only services/ imports it (see README.md). +// The one HTTP client for v1 and its backend paths; only services/ imports it (see README.md). const CSRF_COOKIE = "csrftoken"; const CSRF_HEADER = "X-CSRFToken"; const MUTATING_METHODS = new Set(["post", "put", "patch", "delete"]); /** Options every service function takes. */ -export type RequestOptions = Readonly<{ signal?: AbortSignal }>; +export type RequestOptions = Readonly<{ + signal?: AbortSignal; + /** The organisation to call; a query passes its key's, else the current one. */ + org?: string; +}>; + +/** `/api/v1/unstract/` + path, the trailing slash kept as written. */ +export function orgPath( + path: `/${string}`, + org: string = currentOrganizationId(), +): string { + return `/api/v1/unstract/${encodeURIComponent(org)}${path}`; +} function readCookie(name: string): string | undefined { const prefix = `${name}=`; @@ -76,7 +88,7 @@ export function createHttpClient(): AxiosInstance { return Promise.reject(error); } const apiError = toApiError(error); - // Concurrent 401s redirect once. + // Once per page load, so concurrent 401s redirect once; the handler leaves the page. if (apiError.kind === "unauthenticated" && !expired) { expired = true; expireSession(); diff --git a/frontend/src/v1/test/README.md b/frontend/src/v1/test/README.md index 4f599cd003..2a614281e9 100644 --- a/frontend/src/v1/test/README.md +++ b/frontend/src/v1/test/README.md @@ -107,7 +107,7 @@ import { apiPath, DEFAULT_SESSION, HttpResponse, http, mockApi } from "@v1/test" mockApi( http.get(apiPath(DEFAULT_SESSION, "/adapter/"), () => - HttpResponse.json(adapterFixtures.list), + HttpResponse.json(adapterFixtures.llm), ), ); ``` @@ -134,23 +134,10 @@ arguments. A test that leaves fake timers on gets real timers back after it. Each service module in `services/` exports its request and response types. Its fixtures live in `test/fixtures/.ts`, typed against those types -so a contract change breaks the fixture at typecheck: - -```ts -// test/fixtures/adapters.ts -import type { Adapter } from "@v1/services/adapters"; -import { apiPath, HttpResponse, http, type TestSession } from "@v1/test"; - -export const adapterFixtures = { - list: [{ id: "a1", adapter_name: "OpenAI", adapter_type: "LLM" }], -} satisfies Record; - -export const adapterHandlers = (session: TestSession) => [ - http.get(apiPath(session, "/adapter/"), () => - HttpResponse.json(adapterFixtures.list), - ), -]; -``` +with `satisfies`, so a contract change breaks the fixture at typecheck. See +`test/fixtures/adapters.ts`: `adapterFixtures` holds the rows and +`adapterHandlers(session, rows, seen)` answers `GET adapter/` the way the +backend does. ## Accessibility diff --git a/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs b/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs index efa82ba621..76dd8d6162 100644 --- a/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs +++ b/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs @@ -123,7 +123,13 @@ const CASES = parseTable(` v1-http-client-axios | lint:v1 | Only services/ imports the HTTP client v1-http-client-module | lint:v1 | Only services/ imports the HTTP client v1-http-client-dynamic | lint:v1 | Only services/ imports the HTTP client + v1-http-client-js-ext | lint:v1 | Only services/ imports the HTTP client + v1-raw-fetch | lint:v1 | Only services/ sends requests + v1-raw-xhr | lint:v1 | Only services/ sends requests + v1-raw-eventsource-global | lint:v1 | Only services/ sends requests + v1-plugin-page-backend-url | lint:v1 | Backend URLs appear only in services/ v1-backend-url-services-ok | typecheck:v1 + lint:v1 | - + v1-plugin-services-ok | lint:v1 | - v1-backend-url-test-ok | lint:v1 | - v1-explicit-any | lint:v1 | noExplicitAny # The v1 override must add to the shared rules, not replace them. diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-services-ok/src/v1/services/users.ts b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-services-ok/src/v1/services/users.ts index 663f606207..d9189b4dcf 100644 --- a/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-services-ok/src/v1/services/users.ts +++ b/frontend/test-fixtures/v1-boundary/cases/v1-backend-url-services-ok/src/v1/services/users.ts @@ -1,5 +1,4 @@ -import { orgPath } from "./context"; -import { http, type RequestOptions } from "./http"; +import { http, orgPath, type RequestOptions } from "./http"; export type User = Readonly<{ id: string; email: string }>; diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-http-client-js-ext/src/v1/pages/demo/path.ts b/frontend/test-fixtures/v1-boundary/cases/v1-http-client-js-ext/src/v1/pages/demo/path.ts new file mode 100644 index 0000000000..4286a06071 --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-http-client-js-ext/src/v1/pages/demo/path.ts @@ -0,0 +1,3 @@ +import { orgPath } from "@v1/services/http.js"; + +export const usersPath = () => orgPath("/users/"); diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-plugin-page-backend-url/src/v1/plugins/billing/Plans.tsx b/frontend/test-fixtures/v1-boundary/cases/v1-plugin-page-backend-url/src/v1/plugins/billing/Plans.tsx new file mode 100644 index 0000000000..8cfb46c067 --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-plugin-page-backend-url/src/v1/plugins/billing/Plans.tsx @@ -0,0 +1 @@ +export const PLANS_URL = "/api/v1/plans/"; diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-plugin-services-ok/src/v1/plugins/billing/services/plans.ts b/frontend/test-fixtures/v1-boundary/cases/v1-plugin-services-ok/src/v1/plugins/billing/services/plans.ts new file mode 100644 index 0000000000..5a87d2573c --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-plugin-services-ok/src/v1/plugins/billing/services/plans.ts @@ -0,0 +1,10 @@ +import { http, orgPath, type RequestOptions } from "@v1/services/http"; + +export type Plan = Readonly<{ id: string; name: string }>; + +export async function listPlans({ signal }: RequestOptions = {}) { + const { data } = await http.get(orgPath("/plans/"), { signal }); + return data; +} + +export const PLANS_PAGE = "/api/v1/billing/plans/"; diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-raw-eventsource-global/src/v1/shell/stream.ts b/frontend/test-fixtures/v1-boundary/cases/v1-raw-eventsource-global/src/v1/shell/stream.ts new file mode 100644 index 0000000000..09e71f5eb1 --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-raw-eventsource-global/src/v1/shell/stream.ts @@ -0,0 +1 @@ +export const listen = (url: string) => new window.EventSource(url); diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-raw-fetch/src/v1/pages/demo/load.ts b/frontend/test-fixtures/v1-boundary/cases/v1-raw-fetch/src/v1/pages/demo/load.ts new file mode 100644 index 0000000000..9d93a5e636 --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-raw-fetch/src/v1/pages/demo/load.ts @@ -0,0 +1 @@ +export const load = (url: string) => fetch(url, { method: "POST" }); diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-raw-xhr/src/v1/hooks/useUpload.ts b/frontend/test-fixtures/v1-boundary/cases/v1-raw-xhr/src/v1/hooks/useUpload.ts new file mode 100644 index 0000000000..8ac7fa308d --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-raw-xhr/src/v1/hooks/useUpload.ts @@ -0,0 +1 @@ +export const newUpload = () => new XMLHttpRequest(); From f42bbc34c53907d189f3ba30b2dd4083c2de9ed8 Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Fri, 13 Nov 2026 10:49:40 +0530 Subject: [PATCH 3/3] UN-4313 [FEAT] Fix the second data layer review: reconnect refetch, provider 402s, same-org paging - Reads refetch when the connection returns, and queries that cannot reach the server share one toast. - A provider's typed 402 is a client error, not the org's plan; a Blob body no longer hides session expiry. - QueryView decides "first fetch failed" from the cache; a select that throws goes to the error boundary. - Paged queries keep the previous page only within one organisation (previousPageInOrg). - Lint messages and docs name plugins' services/; the E2E harness docs match the demo page's data. - Tests: the org-in-key check covers paged queries; a fixture covers relative imports of the client. Co-Authored-By: Claude Opus 5.5 --- frontend/scripts/check-v1.mjs | 8 +-- frontend/src/v1/README.md | 8 ++- frontend/src/v1/app/query.test.tsx | 53 ++++++++++++++ frontend/src/v1/app/query.ts | 23 +++++- .../v1/components/patterns/QueryView.test.tsx | 70 ++++++++++++++++++- .../src/v1/components/patterns/QueryView.tsx | 6 +- frontend/src/v1/services/README.md | 17 +++-- frontend/src/v1/services/adapters.test.ts | 21 +++++- frontend/src/v1/services/adapters.ts | 21 ++++-- frontend/src/v1/services/errors.test.ts | 28 +++++++- frontend/src/v1/services/errors.ts | 13 +++- frontend/src/v1/services/page.ts | 14 ++++ frontend/src/v1/test/README.md | 4 +- .../v1-boundary/boundary.fixtures.mjs | 25 ++++--- .../src/v1/pages/demo/load.ts | 3 + tests/e2e/ui-v1/README.md | 6 +- tests/e2e/ui-v1/specs/P0-T09.demo.spec.ts | 5 +- tests/e2e/ui-v1/support/state-fixture.ts | 7 +- 18 files changed, 278 insertions(+), 54 deletions(-) create mode 100644 frontend/test-fixtures/v1-boundary/cases/v1-http-client-relative/src/v1/pages/demo/load.ts diff --git a/frontend/scripts/check-v1.mjs b/frontend/scripts/check-v1.mjs index 646eea2f83..4e6f4208f9 100755 --- a/frontend/scripts/check-v1.mjs +++ b/frontend/scripts/check-v1.mjs @@ -551,7 +551,7 @@ function* notifyEntryPoints(ast, specs) { } } -// P0-T05: backend URLs and requests only in services/; tests may name URLs for mocks. +// P0-T05: backend URLs and requests only in services/ (and plugins' services/); tests may name URLs for mocks. const BACKEND_URL_HOMES = new Set(["services", "test"]); // A cloud plugin keeps its own service modules in plugins//services/. const PLUGIN_SERVICES = /^plugins\/[^/]+\/services\//; @@ -565,11 +565,11 @@ const CODE_SUFFIX = /\.[cm]?[jt]sx?$/; const RAW_REQUESTS = new Set(["fetch", "XMLHttpRequest", "EventSource"]); const GLOBAL_OBJECTS = new Set(["window", "globalThis", "self"]); const BACKEND_URL_MSG = - "Backend URLs appear only in services/; call a typed service function instead."; + "Backend URLs appear only in services/ and plugins//services/; call a typed service function instead."; const HTTP_CLIENT_MSG = - "Only services/ imports the HTTP client (axios, @v1/services/http); call a typed service function instead."; + "Only services/ and plugins//services/ import the HTTP client (axios, @v1/services/http); call a typed service function instead."; const RAW_REQUEST_MSG = - "Only services/ sends requests (fetch, XMLHttpRequest, EventSource); call a typed service function instead."; + "Only services/ and plugins//services/ send requests (fetch, XMLHttpRequest, EventSource); call a typed service function instead."; const isBackendHome = (folder, rel) => BACKEND_URL_HOMES.has(folder) || PLUGIN_SERVICES.test(rel); diff --git a/frontend/src/v1/README.md b/frontend/src/v1/README.md index 8c8b14b9c3..b47da9cb2f 100644 --- a/frontend/src/v1/README.md +++ b/frontend/src/v1/README.md @@ -141,8 +141,10 @@ only when a v1 page first renders. `REGISTERED_PAGES`. Its v0 URLs then redirect to v1; reverting is that line. v1 renders outside v0's `PersistentLogin` and `RequireAuth`, and its own session and guards come with P0-T06: a page registered before that ships - with no session load or org check, and leaves for sign-in only when a - service call gets a 401 (`services/http.ts`). + with no session load or org check. Until something sets the org + (`setOrganizationId()`), its queries throw and it shows the error fallback; + it leaves for sign-in only when a service call gets a 401 + (`services/http.ts`). - `app/registry/validate.test.ts` fails CI on an invalid entry: overlapping patterns across pages, a v1 route and a different v0 route of one page that match the same URL (a page may keep its v0 URL), a route without a mapping @@ -187,7 +189,7 @@ Every folder may import from itself. Test files (`*.test.ts(x)`, | `components/patterns/` | Composites built from core components | `components/ui/`, `lib/`, `services/`, `stores/`, `hooks/`, `assets/` | | `shell/` | Top bar, rail, context bar, tab bar | `components/ui/`, `components/patterns/`, `services/`, `stores/`, `hooks/`, `lib/`, `assets/` | | `pages//` | One folder per target page | `components/ui/`, `components/patterns/`, `shell/`, `services/`, `stores/`, `hooks/`, `lib/`, `styles/`, `assets/`, `plugins/` — not other pages, `app/`, `test/` or the entry | -| `services/` | Typed service modules and the HTTP client; the only place a backend URL appears (see `services/README.md`) | `lib/` | +| `services/` | Typed service modules and the HTTP client; with a cloud plugin's `plugins//services/`, the only place a backend URL appears (see `services/README.md`) | `lib/` | | `stores/` | Zustand stores | `lib/` | | `hooks/` | Shared hooks | `lib/` | | `lib/` | Pure helpers | — | diff --git a/frontend/src/v1/app/query.test.tsx b/frontend/src/v1/app/query.test.tsx index 6e5477f7ce..639d8cec1c 100644 --- a/frontend/src/v1/app/query.test.tsx +++ b/frontend/src/v1/app/query.test.tsx @@ -13,6 +13,7 @@ import { DEFAULT_SESSION, delay, HttpResponse, + makeSession, http as mock, mockApi, renderHookV1, @@ -145,6 +146,29 @@ describe("reads", () => { ]); }); + it("that cannot reach the server share one toast", async () => { + mockApi(...adapterHandlers(DEFAULT_SESSION)); + const queryClient = createQueryClient({ queries: { retryDelay: 0 } }); + const { result } = renderHookV1( + () => [ + useQuery(adapterQueries.list(LLM)), + useQuery(adapterQueries.page({ ...LLM, page: 1, pageSize: 10 })), + ], + { queryClient }, + ); + await waitFor(() => + expect(result.current.every((query) => query.isSuccess)).toBe(true), + ); + mockApi(mock.get(ADAPTERS, () => HttpResponse.error())); + await act(() => + Promise.all(result.current.map((query) => query.refetch())), + ); + expect(shown.map((input) => input.id)).toEqual([ + "v1-network", + "v1-network", + ]); + }); + it("are cancelled when the caller unmounts, and report nothing", async () => { const warn = vi.spyOn(console, "warn"); const error = vi.spyOn(console, "error"); @@ -192,6 +216,23 @@ describe("paging", () => { await waitFor(() => expect(result.current.isPlaceholderData).toBe(false)); expect(name(result.current.data)).toBe("Claude"); }); + + it("never keeps another organisation's page on screen", async () => { + const other = makeSession({ organization: { id: "org-2" } }); + mockApi(...adapterHandlers(DEFAULT_SESSION), ...adapterHandlers(other)); + const params = { ...LLM, page: 1, pageSize: 1 }; + const { result, rerender } = renderHookV1( + ({ org }: { org: string }) => { + setOrganizationId(org); + return useQuery(adapterQueries.page(params)); + }, + { initialProps: { org: DEFAULT_SESSION.organization.id } }, + ); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + rerender({ org: other.organization.id }); + expect(result.current.isPlaceholderData).toBe(false); + expect(result.current.data).toBeUndefined(); + }); }); describe("invalidation", () => { @@ -300,6 +341,18 @@ describe("offline", () => { expect(result.current.error).toMatchObject({ kind: "network" }); }); + it("reads refetch when the connection returns", async () => { + const seen: URL[] = []; + mockApi(...adapterHandlers(DEFAULT_SESSION, adapterFixtures.llm, seen)); + const { result } = renderHookV1(() => useQuery(adapterQueries.list(LLM)), { + queryClient: createQueryClient(), + }); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + act(() => onlineManager.setOnline(false)); + act(() => onlineManager.setOnline(true)); + await waitFor(() => expect(seen).toHaveLength(2)); + }); + it("a save fails instead of being sent when the connection returns", async () => { onlineManager.setOnline(false); const calls = { count: 0 }; diff --git a/frontend/src/v1/app/query.ts b/frontend/src/v1/app/query.ts index 93415c47e5..f55682f557 100644 --- a/frontend/src/v1/app/query.ts +++ b/frontend/src/v1/app/query.ts @@ -6,12 +6,14 @@ import { QueryCache, QueryClient, } from "@tanstack/react-query"; -import { isTransient } from "@v1/services/errors"; +import { isTransient, toApiError } from "@v1/services/errors"; import { reportApiError } from "@v1/services/report"; // Server state for v1; every failure is reported once, here (services/README.md). const MAX_READ_RETRIES = 2; +// One toast for every query that cannot reach the server, not one each. +const NETWORK_TOAST_ID = "v1-network"; /** Reads retry only on transient failures; mutations never retry. */ export function retryRead(failureCount: number, error: unknown): boolean { @@ -24,9 +26,17 @@ function reportQueryError( ): void { // A failed refresh keeps stale data on screen, so only then a toast. const notify = query.state.data !== undefined; - reportApiError(error, { notify, id: query.queryHash }); + const network = toApiError(error).kind === "network"; + reportApiError(error, { + notify, + id: network ? NETWORK_TOAST_ID : query.queryHash, + }); } +// Only a `select` that threw (the cached query did not fail): a bug, for the error boundary. +const throwSelectError = (_error: Error, query: Query) => + query.state.status !== "error"; + function reportMutationError( error: Error, _variables: unknown, @@ -40,12 +50,19 @@ function reportMutationError( export function createQueryClient(defaults: DefaultOptions = {}): QueryClient { // "always": offline fails fast as `network` instead of pausing, and a save is never sent later. const networkMode = "always"; + // "always" turns refetch on reconnect off; reads still refresh when the connection returns. + const queries = { + retry: retryRead, + networkMode, + refetchOnReconnect: true, + throwOnError: throwSelectError, + } as const; return new QueryClient({ queryCache: new QueryCache({ onError: reportQueryError }), mutationCache: new MutationCache({ onError: reportMutationError }), defaultOptions: { ...defaults, - queries: { retry: retryRead, networkMode, ...defaults.queries }, + queries: { ...queries, ...defaults.queries }, mutations: { retry: false, networkMode, ...defaults.mutations }, }, }); diff --git a/frontend/src/v1/components/patterns/QueryView.test.tsx b/frontend/src/v1/components/patterns/QueryView.test.tsx index e3d8f0208d..7363706fba 100644 --- a/frontend/src/v1/components/patterns/QueryView.test.tsx +++ b/frontend/src/v1/components/patterns/QueryView.test.tsx @@ -12,7 +12,7 @@ import { renderV1, } from "@v1/test"; import { adapterFixtures, adapterHandlers } from "@v1/test/fixtures/adapters"; -import type { JSX } from "react"; +import { Component, type JSX, type ReactNode } from "react"; import { afterEach, beforeEach, expect, it, vi } from "vitest"; import { QueryView } from "./QueryView"; @@ -131,3 +131,71 @@ it("renders a successful query whose select gives undefined, not an alert", asyn expect(await screen.findByText("No match")).toBeInTheDocument(); expect(screen.queryByRole("alert")).toBeNull(); }); + +it("reports a failed refresh once, as a toast, when select gave undefined", async () => { + const toast = vi.spyOn(console, "error").mockImplementation(() => undefined); + mockApi(...adapterHandlers(DEFAULT_SESSION)); + function Pick(): JSX.Element { + const query = useQuery({ + ...adapterQueries.list({ adapterType: "LLM" }), + select: (rows) => rows.find((row) => row.adapter_name === "Mistral"), + }); + return ( + <> +

Status {query.status}

+ + + {(row) =>

{row?.adapter_name ?? "No match"}

} + + + ); + } + const { user } = renderV1(); + expect(await screen.findByText("No match")).toBeInTheDocument(); + mockApi(mock.get(ADAPTERS, () => HttpResponse.json({}, { status: 500 }))); + await user.click(screen.getByRole("button", { name: "Refresh" })); + await waitFor(() => + expect(screen.getByText("Status error")).toBeInTheDocument(), + ); + expect(screen.getByText("No match")).toBeInTheDocument(); + expect(screen.queryByRole("alert")).toBeNull(); + expect(toast).toHaveBeenCalledTimes(1); +}); + +class Boundary extends Component<{ children: ReactNode }, { error?: Error }> { + override state: { error?: Error } = {}; + + static getDerivedStateFromError(error: Error) { + return { error }; + } + + override render(): ReactNode { + return this.state.error ? ( +

Caught {this.state.error.message}

+ ) : ( + this.props.children + ); + } +} + +it("leaves a select that throws to the error boundary", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); + mockApi(...adapterHandlers(DEFAULT_SESSION)); + function Broken(): JSX.Element { + const query = useQuery({ + ...adapterQueries.list({ adapterType: "LLM" }), + select: (): never => { + throw new TypeError("bad select"); + }, + }); + return {() =>

Rows

}; + } + renderV1( + + + , + ); + expect(await screen.findByText("Caught bad select")).toBeInTheDocument(); +}); diff --git a/frontend/src/v1/components/patterns/QueryView.tsx b/frontend/src/v1/components/patterns/QueryView.tsx index 34042a261e..40846b797c 100644 --- a/frontend/src/v1/components/patterns/QueryView.tsx +++ b/frontend/src/v1/components/patterns/QueryView.tsx @@ -24,7 +24,8 @@ export function QueryView({ if (query.status === "pending") { return <>{loading ?? }; } - if (query.isLoadingError) { + // The first fetch failed: decided on the cache, as app/query.ts does, not on `select`'s result. + if (query.isError && query.dataUpdatedAt === 0) { const error = toApiError(query.error); return ( void query.refetch()}> @@ -37,5 +38,6 @@ export function QueryView({ ); } - return <>{children(query.data)}; + // After a failed refresh, `data` is undefined only if `select` made it so, and T then allows it. + return <>{children(query.data as T)}; } diff --git a/frontend/src/v1/services/README.md b/frontend/src/v1/services/README.md index ef6c29247a..bfba93d158 100644 --- a/frontend/src/v1/services/README.md +++ b/frontend/src/v1/services/README.md @@ -26,10 +26,11 @@ or error handling. typed response. Never accept a free-form query object. - A paged endpoint takes `PageParams` (`page` from 1, `pageSize`), sends them with `pageQuery()`, and returns `Page`. Its query sets - `placeholderData: keepPreviousData`, so the previous page stays on screen - while the next loads (see `adapterQueries.page`). A page past the end is a - 404 ("Invalid page."), so whoever holds the page number steps back to the - last page (`count`) after a delete. + `placeholderData: previousPageInOrg(queryKey)`, so the previous page stays on + screen while the next loads, but never another organisation's (see + `adapterQueries.page`). A page past the end is a 404 ("Invalid page."), so + whoever holds the page number steps back to the last page, + `Math.ceil(count / pageSize)`, after a delete. - Export query keys and options per resource. Keys are `[org, resource, "list" | "page" | "detail", params | id]`, and each query fetches for the org in its key, so two organisations never share a cache @@ -63,7 +64,7 @@ rejects with axios's `CanceledError` instead; check `isCancelled()` first | Kind | When | | --- | --- | | `validation` | `type: "validation_error"` (400); fields in `fieldErrors`, the rest in `formErrors` | -| `subscription` | 402, or a `subscription_error` / `subscription_inactive` body | +| `subscription` | A `subscription_error` / `subscription_inactive` type or code, or a 402 with no `type`; a provider's typed 402 (`sdk_error`) is `client` | | `unauthenticated` | The auth middleware's 401 (no `type` in the body); a typed 401, such as a provider rejecting its key, is `client` | | `forbidden` | 403 | | `not_found` | 404 | @@ -79,8 +80,9 @@ rejects with axios's `CanceledError` instead; check `isCancelled()` first | Failure | Shown as | | --- | --- | | Mutation | A toast with the request id. Pass `meta: { notify: false }` when the form shows it; it is then logged. | -| Read whose first fetch fails (also a next page shown with `keepPreviousData`) | Inline, by `QueryView` (`components/patterns/`): a skeleton, then an `InlineAlert` with Retry and the request id. Also logged. | -| Refresh that fails while the query's data is on screen | A toast, replaced rather than stacked while the same query keeps failing. | +| Read whose first fetch fails (also a next page) | Inline, by `QueryView` (`components/patterns/`): an `InlineAlert` with Retry and the request id, after a skeleton (or, for a next page, the previous page). Also logged. | +| Refresh that fails while the query's data is on screen | A toast, replaced rather than stacked while the same query keeps failing. Every query that cannot reach the server shares one toast. | +| A `select` that throws | The page's error boundary: it is a bug, not a failed request. | | 401 | One redirect to sign in; no toast. | | Subscription | A toast that stays until dismissed; inline like any other first-fetch failure. | | Cancellation | Nothing. | @@ -88,6 +90,7 @@ rejects with axios's `CanceledError` instead; check `isCancelled()` first Reads retry twice on a network error, 502, 503 or 504. Mutations never retry. Offline, requests still go out (`networkMode: "always"`) and fail as `network`; nothing waits for the connection, and a save is never sent later. +Reads refetch when the connection returns. ## Forms diff --git a/frontend/src/v1/services/adapters.test.ts b/frontend/src/v1/services/adapters.test.ts index 26a6d96b59..00052ed89e 100644 --- a/frontend/src/v1/services/adapters.test.ts +++ b/frontend/src/v1/services/adapters.test.ts @@ -77,12 +77,27 @@ it("keys queries by organisation, so two organisations never share a cache entry ]); }); -it("fetches for the organisation in the key, even after the current one changes", async () => { +// Each builds its query now and returns a run that fetches it later. +const queries = { + list: () => { + const options = adapterQueries.list({ adapterType: "LLM" }); + return () => new QueryClient().fetchQuery(options); + }, + page: () => { + const params = { adapterType: "LLM", page: 1, pageSize: 10 } as const; + const options = adapterQueries.page(params); + return () => new QueryClient().fetchQuery(options); + }, +}; + +it.each( + Object.entries(queries), +)("%s fetches for the organisation in its key, even after the current one changes", async (_, build) => { setOrganizationId("org-1"); - const options = adapterQueries.list({ adapterType: "LLM" }); + const run = build(); setOrganizationId("org-2"); const seen: URL[] = []; mockApi(...adapterHandlers(DEFAULT_SESSION, adapterFixtures.llm, seen)); - await new QueryClient().fetchQuery(options); + await run(); expect(seen[0]?.pathname).toBe("/api/v1/unstract/org-1/adapter/"); }); diff --git a/frontend/src/v1/services/adapters.ts b/frontend/src/v1/services/adapters.ts index a1646f3b7a..85d1587367 100644 --- a/frontend/src/v1/services/adapters.ts +++ b/frontend/src/v1/services/adapters.ts @@ -1,7 +1,12 @@ -import { keepPreviousData, queryOptions } from "@tanstack/react-query"; +import { queryOptions } from "@tanstack/react-query"; import { currentOrganizationId } from "./context"; import { http, orgPath, type RequestOptions } from "./http"; -import { type Page, type PageParams, pageQuery } from "./page"; +import { + type Page, + type PageParams, + pageQuery, + previousPageInOrg, +} from "./page"; /* Adapters (LLM, embedding, vector DB, text extractor, OCR) in the current organisation. */ @@ -83,11 +88,13 @@ export const adapterQueries = { listAdapters(params, { signal, org }), }), // The previous page stays on screen while the next one loads. - page: (params: ListAdaptersPageParams) => - queryOptions({ - queryKey: adapterKeys.page(params), + page: (params: ListAdaptersPageParams) => { + const queryKey = adapterKeys.page(params); + return queryOptions({ + queryKey, queryFn: ({ queryKey: [org], signal }) => listAdaptersPage(params, { signal, org }), - placeholderData: keepPreviousData, - }), + placeholderData: previousPageInOrg(queryKey), + }); + }, }; diff --git a/frontend/src/v1/services/errors.test.ts b/frontend/src/v1/services/errors.test.ts index 2f575e9b40..979b39247d 100644 --- a/frontend/src/v1/services/errors.test.ts +++ b/frontend/src/v1/services/errors.test.ts @@ -1,6 +1,6 @@ import { CancelledError } from "@tanstack/react-query"; import { HttpResponse, http as mock, mockApi } from "@v1/test"; -import { CanceledError } from "axios"; +import { AxiosError, type AxiosResponse, CanceledError } from "axios"; import type { JsonBodyType } from "msw"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { onSessionExpired } from "./context"; @@ -118,11 +118,37 @@ describe("each kind, from the backend's own error bodies", () => { }); }); + it("client, not subscription: a provider's 402 for exhausted credit", async () => { + const error = await failureFor( + json({ error: "Insufficient credits", type: "sdk_error" }, 402), + ); + expect(error).toMatchObject({ + kind: "client", + status: 402, + message: "Insufficient credits", + }); + }); + it("unauthenticated: the auth middleware's 401", async () => { const error = await failureFor(json({ message: "Unauthorized" }, 401)); expect(error).toMatchObject({ kind: "unauthenticated", status: 401 }); }); + it("unauthenticated: the middleware's 401 read as a Blob (a download)", () => { + const body = new Blob(['{"message":"Unauthorized"}'], { + type: "application/json", + }); + const response = { status: 401, data: body } as AxiosResponse; + const cause = new AxiosError( + "401", + "ERR_BAD_REQUEST", + undefined, + null, + response, + ); + expect(toApiError(cause)).toMatchObject({ kind: "unauthenticated" }); + }); + it.each([ [ "a provider rejecting its key", diff --git a/frontend/src/v1/services/errors.ts b/frontend/src/v1/services/errors.ts index 0672508f90..ad7fa7342b 100644 --- a/frontend/src/v1/services/errors.ts +++ b/frontend/src/v1/services/errors.ts @@ -80,8 +80,14 @@ const SUBSCRIPTION_TYPES = new Set([ "subscription_inactive", ]); -const isRecord = (value: unknown): value is Record => - typeof value === "object" && value !== null && !Array.isArray(value); +// Plain objects only: a Blob body's `type` is its MIME type, not an error type. +function isRecord(value: unknown): value is Record { + if (typeof value !== "object" || value === null) { + return false; + } + const proto: unknown = Object.getPrototypeOf(value); + return proto === Object.prototype || proto === null; +} const text = (value: unknown): string | undefined => typeof value === "string" && value !== "" ? value : undefined; @@ -110,8 +116,9 @@ function parseBody(data: unknown): ErrorBody { }; } +// A provider's 402 comes typed (`sdk_error`); Unstract's carries a subscription type or code. const isSubscription = (status: number, body: ErrorBody): boolean => - status === 402 || + (status === 402 && body.type === undefined) || SUBSCRIPTION_TYPES.has(body.type ?? "") || body.items.some((item) => SUBSCRIPTION_TYPES.has(item.code ?? "")); diff --git a/frontend/src/v1/services/page.ts b/frontend/src/v1/services/page.ts index 2eadb80ecb..66bff908fd 100644 --- a/frontend/src/v1/services/page.ts +++ b/frontend/src/v1/services/page.ts @@ -1,3 +1,5 @@ +import type { QueryKey } from "@tanstack/react-query"; + // The backend's paged-list contract (DRF PageNumberPagination): `?page=&page_size=`. /** One page of a list endpoint. */ @@ -16,3 +18,15 @@ export type PageParams = Readonly<{ page: number; pageSize: number }>; export function pageQuery({ page, pageSize }: PageParams) { return { page, page_size: pageSize } as const; } + +/** + * A paged query's `placeholderData`: the previous page stays on screen while + * the next loads, but never one from another organisation (keys start with it). + */ +export const previousPageInOrg = + (queryKey: K) => + ( + previous: T | undefined, + previousQuery?: { queryKey: K }, + ): T | undefined => + previousQuery?.queryKey[0] === queryKey[0] ? previous : undefined; diff --git a/frontend/src/v1/test/README.md b/frontend/src/v1/test/README.md index 2a614281e9..39bf5b8bbb 100644 --- a/frontend/src/v1/test/README.md +++ b/frontend/src/v1/test/README.md @@ -136,8 +136,8 @@ Each service module in `services/` exports its request and response types. Its fixtures live in `test/fixtures/.ts`, typed against those types with `satisfies`, so a contract change breaks the fixture at typecheck. See `test/fixtures/adapters.ts`: `adapterFixtures` holds the rows and -`adapterHandlers(session, rows, seen)` answers `GET adapter/` the way the -backend does. +`adapterHandlers(session, rows, seen)` answers `GET adapter/` with those +rows, paged when the request asks for a page (it neither filters nor 404s). ## Accessibility diff --git a/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs b/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs index 76dd8d6162..219612efaf 100644 --- a/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs +++ b/frontend/test-fixtures/v1-boundary/boundary.fixtures.mjs @@ -117,17 +117,20 @@ const CASES = parseTable(` v1-notify-radix-named-ok | lint:v1 | - v1-notify-feedback-ok | lint:v1 | - # One way to call the backend: URLs and the HTTP client only in services/. - v1-backend-url-string | lint:v1 | Backend URLs appear only in services/ - v1-backend-url-template | lint:v1 | Backend URLs appear only in services/ - v1-backend-url-deployment | lint:v1 | Backend URLs appear only in services/ - v1-http-client-axios | lint:v1 | Only services/ imports the HTTP client - v1-http-client-module | lint:v1 | Only services/ imports the HTTP client - v1-http-client-dynamic | lint:v1 | Only services/ imports the HTTP client - v1-http-client-js-ext | lint:v1 | Only services/ imports the HTTP client - v1-raw-fetch | lint:v1 | Only services/ sends requests - v1-raw-xhr | lint:v1 | Only services/ sends requests - v1-raw-eventsource-global | lint:v1 | Only services/ sends requests - v1-plugin-page-backend-url | lint:v1 | Backend URLs appear only in services/ + v1-backend-url-string | lint:v1 | Backend URLs appear only in services/ and plugins//services/ + v1-backend-url-template | lint:v1 | Backend URLs appear only in services/ and plugins//services/ + v1-backend-url-deployment | lint:v1 | Backend URLs appear only in services/ and plugins//services/ + v1-http-client-axios | lint:v1 | Only services/ and plugins//services/ import the HTTP client + v1-http-client-module | lint:v1 | Only services/ and plugins//services/ import the HTTP client + v1-http-client-dynamic | lint:v1 | Only services/ and plugins//services/ import the HTTP client + v1-http-client-js-ext | lint:v1 | Only services/ and plugins//services/ import the HTTP client + # Biome's '../' ban stops lint:v1 first; check-v1 still catches it. + v1-http-client-relative | lint:v1 | never with '../' + v1-http-client-relative | node scripts/check-v1.mjs --code | Only services/ and plugins//services/ import the HTTP client + v1-raw-fetch | lint:v1 | Only services/ and plugins//services/ send requests + v1-raw-xhr | lint:v1 | Only services/ and plugins//services/ send requests + v1-raw-eventsource-global | lint:v1 | Only services/ and plugins//services/ send requests + v1-plugin-page-backend-url | lint:v1 | Backend URLs appear only in services/ and plugins//services/ v1-backend-url-services-ok | typecheck:v1 + lint:v1 | - v1-plugin-services-ok | lint:v1 | - v1-backend-url-test-ok | lint:v1 | - diff --git a/frontend/test-fixtures/v1-boundary/cases/v1-http-client-relative/src/v1/pages/demo/load.ts b/frontend/test-fixtures/v1-boundary/cases/v1-http-client-relative/src/v1/pages/demo/load.ts new file mode 100644 index 0000000000..e549c5b78c --- /dev/null +++ b/frontend/test-fixtures/v1-boundary/cases/v1-http-client-relative/src/v1/pages/demo/load.ts @@ -0,0 +1,3 @@ +import { http } from "../../services/http"; + +export const load = (url: string) => http.get(url); diff --git a/tests/e2e/ui-v1/README.md b/tests/e2e/ui-v1/README.md index fd6b627e4f..8474e3a48c 100644 --- a/tests/e2e/ui-v1/README.md +++ b/tests/e2e/ui-v1/README.md @@ -95,8 +95,10 @@ test.describe("P4-T01 API Deployments", { tag: DIST.both }, () => { (`[data-ui-version="v1"]`, carrying `data-theme`), and the theme modes and storage key from `src/v1/lib/theme.ts`. - `specs/P0-T09.demo.spec.ts` proves the switch on P0-T03's demo pages - (`/:orgName/v1-demo`, `/:orgName/v1-demo-review`). Those pages load no data, - so the five-state helper is proven on `support/state-fixture.ts`. + (`/:orgName/v1-demo`, `/:orgName/v1-demo-review`). Their only data, the demo + page's adapter list, renders none of the state components the five-state + helper looks for (`STATE_TEST_IDS`), so it is proven on + `support/state-fixture.ts`. Still open: diff --git a/tests/e2e/ui-v1/specs/P0-T09.demo.spec.ts b/tests/e2e/ui-v1/specs/P0-T09.demo.spec.ts index ec2f15216c..a36eae8656 100644 --- a/tests/e2e/ui-v1/specs/P0-T09.demo.spec.ts +++ b/tests/e2e/ui-v1/specs/P0-T09.demo.spec.ts @@ -9,8 +9,9 @@ * Part 2 runs on P0-T03's development-only demo pages, which the E2E image * registers (VITE_V1_DEV_ROUTES=true): the version switch and its redirect, * the theme matrix with an enforcing scan, and Human Review's old-version - * round trip on its stand-in. The demo pages load no data, so the five-state - * helper is proven by its self-tests (specs/harness/five-states.spec.ts). + * round trip on its stand-in. DemoPage's adapter list (UN-4313) renders none + * of the state components the five-state helper looks for (STATE_TEST_IDS), so + * the helper is proven by its self-tests (specs/harness/five-states.spec.ts). * * The redirect and the round trip also run in specs/harness/ * registry-routes.spec.ts, once per registered page. They stay here too, at diff --git a/tests/e2e/ui-v1/support/state-fixture.ts b/tests/e2e/ui-v1/support/state-fixture.ts index 49d8e624a3..516e60964b 100644 --- a/tests/e2e/ui-v1/support/state-fixture.ts +++ b/tests/e2e/ui-v1/support/state-fixture.ts @@ -1,9 +1,10 @@ /** * A self-test fixture for the helpers, not an app page. * - * `expectFiveStates` needs a screen that loads data, and P0-T03's demo pages - * (`/:orgName/v1-demo`) render static content. So the helper self-tests serve - * this page instead: `page.route` fulfils `/__v1-harness-fixture` on the + * `expectFiveStates` needs a screen that renders the core state components + * (STATE_TEST_IDS), and P0-T03's demo pages (`/:orgName/v1-demo`) render none. + * So the helper self-tests serve this page instead: `page.route` fulfils + * `/__v1-harness-fixture` on the * frontend origin with a minimal page that renders each state from its API * (`/__v1-harness-fixture/api/items`), using the contract/v1-ui.ts markers. * A green run here proves the helpers; the version switch is proven against