diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0973b72..a3ff181 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -164,6 +164,29 @@ jobs: # that break soroban-env-host's testutils. run: cargo test --workspace --locked + # ── Test shared packages ───────────────────────────────────────────────────── + # Added because nothing outside apps/api and the contracts was ever run here. + # `@useroutr/types` now owns the API base-URL contract that the dashboard and + # checkout both depend on; when the two disagreed about whether + # NEXT_PUBLIC_API_URL included `/v1`, four features 404'd in production and no + # job in this file could have noticed. + test-packages: + name: Test Packages + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: ${{ env.NODE_VERSION }} + cache: npm + + - run: npm ci + + - name: Run tests + working-directory: packages/types + run: npm test + # ── Build ──────────────────────────────────────────────────────────────────── build: name: Build diff --git a/apps/checkout/components/BankInstructions.tsx b/apps/checkout/components/BankInstructions.tsx index f514fb0..6e78e2d 100644 --- a/apps/checkout/components/BankInstructions.tsx +++ b/apps/checkout/components/BankInstructions.tsx @@ -63,7 +63,7 @@ export function BankInstructions() { try { const result = await api.post( - `/v1/payments/${paymentId}/bank-session`, + `/payments/${paymentId}/bank-session`, ); if (cancelled) return; setSession(result.session); @@ -93,7 +93,7 @@ export function BankInstructions() { try { const result = await api.post( - `/v1/payments/${paymentId}/bank-session/regenerate`, + `/payments/${paymentId}/bank-session/regenerate`, ); setSession(result.session); setExpired(Boolean(result.expired)); @@ -117,7 +117,7 @@ export function BankInstructions() { setError(null); try { - await api.post(`/v1/payments/${paymentId}/bank-sent`); + await api.post(`/payments/${paymentId}/bank-sent`); router.push(`/${paymentId}/confirm`); } catch { setError("Unable to mark transfer as sent. Please try again."); diff --git a/apps/checkout/components/CardForm.tsx b/apps/checkout/components/CardForm.tsx index 8b8385d..b3a0db7 100644 --- a/apps/checkout/components/CardForm.tsx +++ b/apps/checkout/components/CardForm.tsx @@ -166,7 +166,7 @@ export function CardForm({ try { const session = await api.post( - `/v1/payments/${paymentId}/card-session` + `/payments/${paymentId}/card-session` ); const result = await stripe.confirmCardPayment(session.clientSecret, { diff --git a/apps/checkout/hooks/useInvoiceCheckout.ts b/apps/checkout/hooks/useInvoiceCheckout.ts index 71be8a4..bab46a5 100644 --- a/apps/checkout/hooks/useInvoiceCheckout.ts +++ b/apps/checkout/hooks/useInvoiceCheckout.ts @@ -47,7 +47,7 @@ export interface InvoiceCheckoutData { export function useInvoiceCheckout(invoiceId: string) { return useQuery({ queryKey: ["invoice-checkout", invoiceId], - queryFn: () => api.get(`/v1/invoices/${invoiceId}/checkout`), + queryFn: () => api.get(`/invoices/${invoiceId}/checkout`), enabled: !!invoiceId, retry: false, staleTime: 30_000, @@ -56,7 +56,6 @@ export function useInvoiceCheckout(invoiceId: string) { export function useInitiateInvoicePayment() { return useMutation<{ paymentId: string }, Error, string>({ - mutationFn: (invoiceId) => - api.post(`/v1/invoices/${invoiceId}/pay`), + mutationFn: (invoiceId) => api.post(`/invoices/${invoiceId}/pay`), }); } diff --git a/apps/checkout/lib/api.ts b/apps/checkout/lib/api.ts index 8b68213..acfbf9f 100644 --- a/apps/checkout/lib/api.ts +++ b/apps/checkout/lib/api.ts @@ -1,4 +1,17 @@ -const BASE_URL = process.env.NEXT_PUBLIC_API_URL ?? "http://localhost:3333/v1"; +import { assertVersionlessPath, resolveApiBaseUrl } from "@useroutr/types"; + +/** + * The version prefix lives here, not at call sites — the same rule the + * dashboard follows. This file used to read `NEXT_PUBLIC_API_URL` expecting it + * to *include* `/v1` while the dashboard read the same variable expecting it + * not to, so no single deployment value could satisfy both. Meanwhile four + * call sites in this app wrote `/v1/payments/...` on top of a base that + * already ended in `/v1`, and requested `/v1/v1/payments/...`. + */ +const BASE_URL = resolveApiBaseUrl( + process.env.NEXT_PUBLIC_API_URL, + "http://localhost:3333", +); interface RequestOptions { params?: Record; @@ -33,8 +46,11 @@ async function request( path: string, options: RequestOptions & { body?: unknown } = {}, ): Promise { + // Fails loudly in development if a caller re-adds the version prefix. + assertVersionlessPath(path.startsWith("/") ? path : `/${path}`); + // Use string concat rather than `new URL(path, BASE_URL)` so an absolute - // `path` like `/v1/links/abc` doesn't replace BASE_URL's pathname when + // `path` like `/links/abc` doesn't replace BASE_URL's pathname when // BASE_URL itself carries a path prefix (e.g. behind an ingress). const queryString = options.params ? "?" + diff --git a/apps/dashboard/src/hooks/__tests__/usePayouts.test.tsx b/apps/dashboard/src/hooks/__tests__/usePayouts.test.tsx index 52e7ef3..501a1ee 100644 --- a/apps/dashboard/src/hooks/__tests__/usePayouts.test.tsx +++ b/apps/dashboard/src/hooks/__tests__/usePayouts.test.tsx @@ -61,7 +61,7 @@ describe('usePayouts', () => { await waitFor(() => expect(result.current.isSuccess).toBe(true)) - expect(api.get).toHaveBeenCalledWith('/v1/payouts', { + expect(api.get).toHaveBeenCalledWith('/payouts', { params: { limit: 20, offset: 0 }, }) expect(result.current.data).toEqual(mockResponse) @@ -88,7 +88,7 @@ describe('usePayouts', () => { }) await waitFor(() => { - expect(api.get).toHaveBeenCalledWith('/v1/payouts', { params: filters }) + expect(api.get).toHaveBeenCalledWith('/payouts', { params: filters }) }) }) }) @@ -114,7 +114,7 @@ describe('useRetryPayout', () => { await result.current.mutateAsync('payout-1') - expect(api.post).toHaveBeenCalledWith('/v1/payouts/payout-1/retry') + expect(api.post).toHaveBeenCalledWith('/payouts/payout-1/retry') }) }) @@ -139,6 +139,6 @@ describe('useCancelPayout', () => { await result.current.mutateAsync('payout-1') - expect(api.post).toHaveBeenCalledWith('/v1/payouts/payout-1/cancel') + expect(api.post).toHaveBeenCalledWith('/payouts/payout-1/cancel') }) }) diff --git a/apps/dashboard/src/lib/api.ts b/apps/dashboard/src/lib/api.ts index 18843dc..fdecb32 100644 --- a/apps/dashboard/src/lib/api.ts +++ b/apps/dashboard/src/lib/api.ts @@ -1,3 +1,4 @@ +import { assertVersionlessPath, resolveApiBaseUrl } from "@useroutr/types"; import { getToken, refreshAccessToken, @@ -6,14 +7,16 @@ import { } from "./auth"; /** - * All HTTP calls hit the versioned surface (`/v1/*`). The env var points - * at the origin only — we append the prefix here so callers can keep - * writing `api.get("/auth/me")` and not think about versioning. + * All HTTP calls hit the versioned surface (`/v1/*`). `resolveApiBaseUrl` owns + * the prefix so callers keep writing `api.get("/auth/me")` — and so this app + * and checkout agree on what `NEXT_PUBLIC_API_URL` means, which they did not + * before. It accepts the env var with or without a trailing `/v1`. */ -const API_ORIGIN = ( - process.env.NEXT_PUBLIC_API_URL ?? "http://localhost:3333" -).replace(/\/$/, ""); -const BASE_URL = `${API_ORIGIN}/v1`; +const BASE_URL = resolveApiBaseUrl( + process.env.NEXT_PUBLIC_API_URL, + "http://localhost:3333", +); +const API_ORIGIN = BASE_URL.replace(/\/v1$/, ""); function getApiConnectionErrorMessage() { return `Cannot reach the Useroutr API at ${API_ORIGIN}. Start it with \`npm run start:api\` or set NEXT_PUBLIC_API_URL.`; @@ -91,6 +94,9 @@ async function request( // pathname replacement, which would silently strip the "/v1" version // prefix from BASE_URL. The string-join approach is unambiguous. const normalizedPath = path.startsWith("/") ? path : `/${path}`; + // Fails loudly in development if a caller re-adds the version. Nineteen call + // sites here had, and every one of them 404'd silently in the browser. + assertVersionlessPath(normalizedPath); const url = new URL(`${BASE_URL}${normalizedPath}`); if (options.params) { @@ -169,9 +175,9 @@ async function request( } if (!retryRes.ok) { - const retryErrorBody = await parseResponse( - retryRes, - ).catch(() => ({}) as ApiErrorBody); + const retryErrorBody = await parseResponse(retryRes).catch( + () => ({}) as ApiErrorBody, + ); throw new Error( extractErrorMessage( retryErrorBody, diff --git a/apps/dashboard/src/lib/auth.ts b/apps/dashboard/src/lib/auth.ts index bd4aa14..e1bcef1 100644 --- a/apps/dashboard/src/lib/auth.ts +++ b/apps/dashboard/src/lib/auth.ts @@ -1,15 +1,17 @@ +import { resolveApiBaseUrl } from "@useroutr/types"; + const TOKEN_KEY = "useroutr-token"; const REFRESH_KEY = "useroutr-refresh-token"; const VERIFICATION_EMAIL_KEY = "useroutr-verification-email"; -// Origin only (no path) — we append `/v1` ourselves so this file stays -// in sync with lib/api.ts's BASE_URL construction. Fallback is the local -// API port (:3333), NOT the marketing site (:3000) — getting that wrong -// makes every refresh hit a 404 and silently log the user out. -const API_ORIGIN = ( - process.env.NEXT_PUBLIC_API_URL ?? "http://localhost:3333" -).replace(/\/$/, ""); -const BASE_URL = `${API_ORIGIN}/v1`; +// Shares `resolveApiBaseUrl` with lib/api.ts rather than re-deriving the base, +// so the two cannot drift apart. Fallback is the local API port (:3333), NOT +// the marketing site (:3000) — getting that wrong makes every refresh hit a +// 404 and silently log the user out. +const BASE_URL = resolveApiBaseUrl( + process.env.NEXT_PUBLIC_API_URL, + "http://localhost:3333", +); interface JwtPayload { exp?: number; diff --git a/package-lock.json b/package-lock.json index 31d244b..8c18921 100644 --- a/package-lock.json +++ b/package-lock.json @@ -39146,7 +39146,8 @@ "zod": "^4.3.6" }, "devDependencies": { - "typescript": "^5.9.3" + "typescript": "^5.9.3", + "vitest": "^1" } }, "packages/ui": { diff --git a/packages/types/package.json b/packages/types/package.json index eb8fe12..3736388 100644 --- a/packages/types/package.json +++ b/packages/types/package.json @@ -3,16 +3,20 @@ "version": "0.0.1", "main": "dist/index.js", "types": "dist/index.d.ts", - "files": ["dist"], + "files": [ + "dist" + ], "scripts": { "build": "tsc -p tsconfig.build.json", "clean": "rm -rf dist", - "prepare": "npm run build" + "prepare": "npm run build", + "test": "vitest run" }, "dependencies": { "zod": "^4.3.6" }, "devDependencies": { - "typescript": "^5.9.3" + "typescript": "^5.9.3", + "vitest": "^1" } } diff --git a/packages/types/src/api-url.test.ts b/packages/types/src/api-url.test.ts new file mode 100644 index 0000000..bd8fb99 --- /dev/null +++ b/packages/types/src/api-url.test.ts @@ -0,0 +1,97 @@ +import { describe, expect, it } from 'vitest'; +import { API_VERSION, assertVersionlessPath, resolveApiBaseUrl } from './api-url'; + +/** + * These cases are the bug that prompted this module, written down. + * + * Both spellings of the origin have to work, because the two apps that read + * `NEXT_PUBLIC_API_URL` used to disagree about which one it was, and the + * variable is set once per deployment. + */ +describe('resolveApiBaseUrl', () => { + const fallback = 'http://localhost:3333'; + + it('appends the version to a bare origin', () => { + expect(resolveApiBaseUrl('https://api.useroutr.com', fallback)).toBe( + 'https://api.useroutr.com/v1', + ); + }); + + it('leaves an origin that already carries the version alone', () => { + expect(resolveApiBaseUrl('https://api.useroutr.com/v1', fallback)).toBe( + 'https://api.useroutr.com/v1', + ); + }); + + it('tolerates trailing slashes on either spelling', () => { + expect(resolveApiBaseUrl('https://api.useroutr.com/', fallback)).toBe( + 'https://api.useroutr.com/v1', + ); + expect(resolveApiBaseUrl('https://api.useroutr.com/v1/', fallback)).toBe( + 'https://api.useroutr.com/v1', + ); + }); + + // Someone who has already been bitten by the doubled prefix may well "fix" + // it by pasting the doubled value into the env var. Repair it rather than + // faithfully reproduce it. + it('collapses an origin that already doubled the version', () => { + expect(resolveApiBaseUrl('https://api.useroutr.com/v1/v1', fallback)).toBe( + 'https://api.useroutr.com/v1', + ); + }); + + it('falls back when the variable is unset, empty or whitespace', () => { + expect(resolveApiBaseUrl(undefined, fallback)).toBe(`${fallback}/v1`); + expect(resolveApiBaseUrl(null, fallback)).toBe(`${fallback}/v1`); + expect(resolveApiBaseUrl('', fallback)).toBe(`${fallback}/v1`); + expect(resolveApiBaseUrl(' ', fallback)).toBe(`${fallback}/v1`); + }); + + it('preserves a path prefix in front of the version, for ingress setups', () => { + expect(resolveApiBaseUrl('https://useroutr.com/api', fallback)).toBe( + 'https://useroutr.com/api/v1', + ); + }); + + it('never returns a base that lacks exactly one version segment', () => { + for (const input of [ + 'https://x.com', + 'https://x.com/', + 'https://x.com/v1', + 'https://x.com/v1/', + 'https://x.com/v1/v1', + ]) { + const matches = resolveApiBaseUrl(input, fallback).match( + new RegExp(`/${API_VERSION}`, 'g'), + ); + expect(matches).toHaveLength(1); + } + }); +}); + +describe('assertVersionlessPath', () => { + it('rejects a path that re-adds the version', () => { + expect(() => assertVersionlessPath('/v1/payouts')).toThrow(/already includes it/); + }); + + it('names the path the caller should have written', () => { + expect(() => assertVersionlessPath('/v1/invoices/abc')).toThrow( + /Pass "\/invoices\/abc" instead/, + ); + }); + + it('rejects the bare version segment', () => { + expect(() => assertVersionlessPath('/v1')).toThrow(); + }); + + it('allows ordinary resource paths', () => { + expect(() => assertVersionlessPath('/payouts')).not.toThrow(); + expect(() => assertVersionlessPath('/invoices/abc/pdf')).not.toThrow(); + }); + + // The guard must not fire on a resource whose name merely starts with "v1". + it('does not confuse a resource prefixed with the version string', () => { + expect(() => assertVersionlessPath('/v1beta/experiments')).not.toThrow(); + }); +}); diff --git a/packages/types/src/api-url.ts b/packages/types/src/api-url.ts new file mode 100644 index 0000000..29285f1 --- /dev/null +++ b/packages/types/src/api-url.ts @@ -0,0 +1,76 @@ +/** + * One definition of where the API lives and who owns the version prefix. + * + * There used to be three, and they disagreed. `NEXT_PUBLIC_API_URL` meant + * "origin, I will add /v1" in the dashboard and "origin including /v1" in + * checkout. Both were self-consistent, so both looked fine in isolation — but + * the variable is set once per deployment, so whichever value you picked, one + * app was wrong: set `https://api.useroutr.com` and checkout dropped the + * version; set `https://api.useroutr.com/v1` and the dashboard requested + * `/v1/v1/...`. + * + * The rule now: **the base URL owns the version, call sites never write it.** + * A caller passes `/payments/abc`, not `/v1/payments/abc`. The version belongs + * to the API surface as a whole, not to each individual call, which is also + * what makes a future `/v2` a one-line change here rather than a sweep through + * every hook. + */ + +export const API_VERSION = 'v1'; + +/** + * Builds the versioned base URL, accepting an origin written either way. + * + * Tolerating both spellings is deliberate: this value is typed into deployment + * dashboards by people who cannot see this file, and a trailing `/v1` is an + * entirely reasonable thing to write. Normalising is friendlier than a boot + * error, and far friendlier than the 404s the mismatch used to cause. + * + * resolveApiBaseUrl('https://api.useroutr.com') → 'https://api.useroutr.com/v1' + * resolveApiBaseUrl('https://api.useroutr.com/v1') → 'https://api.useroutr.com/v1' + * resolveApiBaseUrl('https://api.useroutr.com/v1/') → 'https://api.useroutr.com/v1' + * resolveApiBaseUrl(undefined, 'http://localhost:3333') + * → 'http://localhost:3333/v1' + */ +export function resolveApiBaseUrl( + rawOrigin: string | undefined | null, + fallbackOrigin: string, +): string { + const raw = (rawOrigin ?? '').trim() || fallbackOrigin; + + // Strip trailing slashes, then any number of trailing `/v1` segments, then + // any slashes those left behind. Repeated rather than single so an origin + // that already carries the doubled prefix is repaired instead of preserved. + let origin = raw.replace(/\/+$/, ''); + while (new RegExp(`/${API_VERSION}$`).test(origin)) { + origin = origin.slice(0, -(API_VERSION.length + 1)).replace(/\/+$/, ''); + } + + return `${origin}/${API_VERSION}`; +} + +/** + * Guards the other half of the contract: that call sites do not add the + * version themselves. + * + * This is the failure the base URL cannot fix on its own. `api.get('/v1/x')` + * against a correctly versioned base produces `/v1/v1/x`, which 404s at + * runtime and nowhere else — no type error, no lint error, and the mocked + * hook tests assert the wrong string right along with it. Twenty call sites + * accumulated this way before anyone opened a network panel. + * + * Throws rather than warns, and only outside production: a 404 in a deployed + * checkout is worse than a loud failure in development, but a hard throw in + * front of a paying customer over a path string is worse still. + */ +export function assertVersionlessPath(path: string): void { + if (process.env.NODE_ENV === 'production') return; + + if (new RegExp(`^/?${API_VERSION}(/|$)`).test(path)) { + throw new Error( + `API path "${path}" starts with "/${API_VERSION}", but the client's base URL ` + + `already includes it — this would request /${API_VERSION}/${API_VERSION}/… ` + + `Pass "${path.replace(new RegExp(`^/?${API_VERSION}`), '')}" instead.`, + ); + } +} diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index d0a94d1..b6ce760 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -1,3 +1,4 @@ +export * from './api-url'; export * from './chain.types'; export * from './chain-detector'; export * from './payment.types'; diff --git a/packages/types/tsconfig.build.json b/packages/types/tsconfig.build.json index e96656a..43ce29e 100644 --- a/packages/types/tsconfig.build.json +++ b/packages/types/tsconfig.build.json @@ -11,5 +11,8 @@ "moduleResolution": "node", "skipLibCheck": true }, - "include": ["src"] + "include": ["src"], + // Tests are source, not product — without this they compile into dist and + // ship to every consumer, dragging a vitest import along with them. + "exclude": ["src/**/*.test.ts"] }