Merge pull request #1454 from misrasamuelisiguzor-oss/feat/api-client-1332-1336

feat(api): import-boundary guard, path-encoding, cache-invalidation, pagination (#1332 #1333 #1335 #1336)

Author: hman38705

docs/pr/misrasamuelisiguzor-oss-1332-1336.md

@@ -0,0 +1,79 @@
+# API Client: import boundary guard, path-encoding, cache-invalidation, pagination
+
+Four `frontend/src/lib/api/` issues. Three are "regression trap" issues whose fix already
+landed in an earlier commit - the delta here is the guard test that keeps it from
+regressing, plus one missed call site. #1336 is a new helper.
+
+## What changed and why
+
+### #1332 - public/admin client split guard
+The split (`public-client.ts` / `admin-client.ts`, commit `895748e`) and an *export*
+boundary test (`public-client.test.ts`) already exist. Added the missing *import* boundary
+guard: `client-import-boundary.test.ts` statically scans `src/app/**` and fails if any route
+outside `src/app/admin/**` (and the privileged `markets/<id>/resolve` route) imports
+`admin-client` directly. It also re-asserts that `public-client.ts` never imports
+`admin-client` and exposes no `/api/v1/admin`, `/api/v1/audit`, or `/api/v1/email` path.
+
+### #1333 - centralize path-parameter encoding
+`fillPath()` was the intended single encoder (commit `dd027a9`) but lived in
+`public-client.ts` and three `src/lib/api/` call sites still called `encodeURIComponent`
+directly (`admin-client.ts` email preview, `tts-client.ts` job status + audio).
+
+- Moved `fillPath` (plus a new `fillPathParams` for multi-segment templates) into a
+  dedicated `paths.ts`; `public-client.ts` re-exports it so existing importers are
+  unaffected.
+- Routed the three stray call sites through `fillPath`.
+- `path-encoding.test.ts`: (a) a `market_id` / `tx_hash` containing `/`, `?`, `#`
+  round-trips as a single encoded path segment (asserted against a mocked `fetch`);
+  (b) `fillPath` encodes exactly once; (c) a grep guard - no `src/lib/api/*.ts` file
+  except `paths.ts` calls `encodeURIComponent`.
+- Out of scope: raw `fetch()` calls in app pages/components that never used the client -
+  a broader refactor with its own issues.
+
+### #1335 - invalidate cache tags only on mutation success
+The `succeeded` guard (a 200 body with `success: false` must not bust the cache, commit
+`4a15eda`) already exists in both request helpers. Added
+`cache-invalidation-on-success.test.ts`: a POST returning `{ success: false }` leaves the
+tagged entry untouched; `{ success: true }` and non-envelope bodies invalidate as before.
+(The guard lives in the request helper, not `cache.ts`, because that is where the response
+body is parsed.)
+
+### #1336 - offset/cursor pagination helper (new `pagination.ts`)
+- `buildPaginationParams({ mode: 'offset' | 'cursor', limit?, offset?/cursor? })` builds the
+  query params for either mode. `limit` defaults to 20 and **throws `RangeError` before the
+  request is sent** when it exceeds 100, mirroring the server's documented 400 message.
+- The cursor is passed through verbatim - never parsed or mutated client-side.
+- `CursorPager` holds the opaque cursor for cursor-mode paging; `setSort(key)` drops the
+  stale cursor when the sort order actually changes, so the next page restarts from the top.
+
+## How to test
+
+```
+cd frontend
+npm ci --legacy-peer-deps
+./node_modules/.bin/jest src/lib/api
+```
+
+- `src/lib/api` Jest suite: **135 pre-existing + 21 new tests pass**.
+- `tsc --noEmit`: the new/changed files add no errors over the repo's pre-existing count.
+- `npm run build` (`generate-client && next build`) not run here - needs the full monorepo
+  build; nothing in this change touches build config.
+
+## Breaking changes
+
+None. `fillPath` keeps its `public-client` export; the request/cache behaviour is unchanged.
+
+## Related issues
+
+Closes #1332
+Closes #1333
+Closes #1335
+Closes #1336
+
+## PR Checklist
+
+- [x] Branch is up to date with `main`
+- [x] Commit messages follow Conventional Commits
+- [x] Tests added for the change
+- [x] Documentation updated if behaviour changed (n/a - behaviour preserved)
+- [x] No secrets or credentials committed

frontend/src/lib/api/__tests__/cache-invalidation-on-success.test.ts

@@ -0,0 +1,69 @@
+/**
+ * #1335 — cache tags are invalidated only when a mutation actually succeeded.
+ *
+ * Some endpoints return HTTP 200 with `{ success: false, message: '...' }` to signal a
+ * business-logic failure (regression trap, commit `4a15eda`). Such a response must leave
+ * the existing cache entry untouched. The guard lives in the request helper (that is
+ * where the response body is parsed), shared by both the public and admin clients.
+ */
+import { apiCache, CACHE_TTL } from '../cache';
+import { CacheTag } from '../public-client';
+
+const BASE_URL = process.env.NEXT_PUBLIC_API_URL!.replace(/\/$/, '');
+const STATS_KEY = `${BASE_URL}/api/v1/statistics`;
+
+function mockJsonResponse(body: unknown) {
+  return {
+    ok: true,
+    status: 200,
+    headers: { get: () => null },
+    text: async () => JSON.stringify(body),
+    json: async () => body,
+  };
+}
+
+describe('cache invalidation is gated on mutation success (#1335)', () => {
+  const originalFetch = global.fetch;
+
+  beforeEach(() => {
+    apiCache.clear();
+    apiCache.set(STATS_KEY, { totalMarkets: 7 }, CACHE_TTL.MEDIUM, [CacheTag.STATISTICS]);
+  });
+  afterEach(() => {
+    global.fetch = originalFetch;
+    apiCache.clear();
+  });
+
+  it('a 200 response whose body reports failure does NOT invalidate the tag', async () => {
+    global.fetch = jest.fn(async () =>
+      mockJsonResponse({ success: false, message: 'Email already subscribed' }),
+    ) as unknown as typeof fetch;
+
+    const { api } = await import('../public-client');
+    const result = await api.newsletterSubscribe({ email: 'a@b.co' });
+
+    expect(result.success).toBe(false);
+    // The statistics cache entry is still present and unchanged.
+    expect(apiCache.get<{ totalMarkets: number }>(STATS_KEY)).toEqual({ totalMarkets: 7 });
+  });
+
+  it('a 200 response reporting success DOES invalidate the tag', async () => {
+    global.fetch = jest.fn(async () =>
+      mockJsonResponse({ success: true, message: 'Check your inbox' }),
+    ) as unknown as typeof fetch;
+
+    const { api } = await import('../public-client');
+    await api.newsletterSubscribe({ email: 'a@b.co' });
+
+    expect(apiCache.get(STATS_KEY)).toBeNull();
+  });
+
+  it('a non-envelope 200 body (no `success` field) invalidates as before', async () => {
+    global.fetch = jest.fn(async () => mockJsonResponse({ id: 'sub_1' })) as unknown as typeof fetch;
+
+    const { api } = await import('../public-client');
+    await api.newsletterSubscribe({ email: 'a@b.co' });
+
+    expect(apiCache.get(STATS_KEY)).toBeNull();
+  });
+});

frontend/src/lib/api/__tests__/client-import-boundary.test.ts

@@ -0,0 +1,53 @@
+import { readdirSync, readFileSync, statSync } from 'fs';
+import { join, relative, sep } from 'path';
+
+const SRC_DIR = join(__dirname, '..', '..', '..');
+const APP_DIR = join(SRC_DIR, 'app');
+
+/**
+ * Routes allowed to import the admin client directly:
+ *   - anything under `src/app/admin/**` (the admin area)
+ *   - the privileged `markets/<id>/resolve` route (market resolution is an
+ *     admin/guardian action; it lives outside `admin/` only because of its URL shape)
+ */
+function isAllowed(relPath: string): boolean {
+  const p = relPath.split(sep).join('/');
+  return p.startsWith('app/admin/') || /^app\/markets\/[^/]+\/resolve\//.test(p);
+}
+
+function walk(dir: string, out: string[] = []): string[] {
+  for (const entry of readdirSync(dir)) {
+    const full = join(dir, entry);
+    if (statSync(full).isDirectory()) walk(full, out);
+    else if (/\.(ts|tsx)$/.test(entry)) out.push(full);
+  }
+  return out;
+}
+
+const ADMIN_CLIENT_IMPORT = /from\s+['"][^'"]*\/api\/admin-client['"]/;
+
+describe('module boundary: admin-client import sites (#1332)', () => {
+  it('no non-admin route under src/app imports admin-client directly', () => {
+    const offenders: string[] = [];
+    for (const file of walk(APP_DIR)) {
+      const relPath = relative(SRC_DIR, file);
+      if (isAllowed(relPath)) continue;
+      if (ADMIN_CLIENT_IMPORT.test(readFileSync(file, 'utf8'))) {
+        offenders.push(relPath.split(sep).join('/'));
+      }
+    }
+    expect(offenders).toEqual([]);
+  });
+
+  it('the public client never imports the admin client', () => {
+    const publicSrc = readFileSync(join(__dirname, '..', 'public-client.ts'), 'utf8');
+    expect(publicSrc).not.toMatch(/from\s+['"]\.\/admin-client['"]/);
+  });
+
+  it('the public client exposes no admin-only path prefixes', () => {
+    const publicSrc = readFileSync(join(__dirname, '..', 'public-client.ts'), 'utf8');
+    for (const prefix of ['/api/v1/admin', '/api/v1/audit', '/api/v1/email']) {
+      expect(publicSrc).not.toContain(prefix);
+    }
+  });
+});

frontend/src/lib/api/__tests__/pagination.test.ts

@@ -0,0 +1,74 @@
+import {
+  buildPaginationParams,
+  CursorPager,
+  DEFAULT_LIMIT,
+  MAX_LIMIT,
+} from '../pagination';
+
+describe('buildPaginationParams', () => {
+  it('defaults limit to 20 in offset mode', () => {
+    expect(buildPaginationParams({ mode: 'offset' })).toEqual({ limit: String(DEFAULT_LIMIT) });
+  });
+
+  it('emits offset only when non-zero', () => {
+    expect(buildPaginationParams({ mode: 'offset', offset: 0 })).toEqual({ limit: '20' });
+    expect(buildPaginationParams({ mode: 'offset', limit: 50, offset: 100 })).toEqual({
+      limit: '50',
+      offset: '100',
+    });
+  });
+
+  it('passes an opaque cursor through unchanged', () => {
+    const cursor = 'eyJpZCI6NDJ9';
+    expect(buildPaginationParams({ mode: 'cursor', cursor })).toEqual({ limit: '20', cursor });
+  });
+
+  it('omits the cursor param on the first cursor-mode page', () => {
+    expect(buildPaginationParams({ mode: 'cursor' })).toEqual({ limit: '20' });
+  });
+
+  it('throws before sending when limit exceeds the server maximum', () => {
+    expect(() => buildPaginationParams({ mode: 'offset', limit: MAX_LIMIT + 1 })).toThrow(RangeError);
+    expect(() => buildPaginationParams({ mode: 'cursor', limit: 500 })).toThrow(
+      /exceeds the maximum allowed value of 100/,
+    );
+  });
+
+  it('rejects a non-positive limit and a negative offset', () => {
+    expect(() => buildPaginationParams({ mode: 'offset', limit: 0 })).toThrow(RangeError);
+    expect(() => buildPaginationParams({ mode: 'offset', limit: 1.5 })).toThrow(RangeError);
+    expect(() => buildPaginationParams({ mode: 'offset', offset: -1 })).toThrow(RangeError);
+  });
+});
+
+describe('CursorPager', () => {
+  it('advances through cursors and reports position', () => {
+    const pager = new CursorPager('created_at');
+    expect(pager.atStart).toBe(true);
+    expect(pager.params()).toEqual({ limit: '20' });
+
+    pager.advance('cursor-page-2');
+    expect(pager.atStart).toBe(false);
+    expect(pager.params(50)).toEqual({ limit: '50', cursor: 'cursor-page-2' });
+
+    pager.advance(null); // end of list
+    expect(pager.atStart).toBe(true);
+  });
+
+  it('discards the stale cursor when the sort order changes', () => {
+    const pager = new CursorPager('created_at');
+    pager.advance('cursor-mid-list');
+    expect(pager.atStart).toBe(false);
+
+    pager.setSort('volume'); // opaque cursor is no longer valid for the new order
+    expect(pager.atStart).toBe(true);
+    expect(pager.params()).toEqual({ limit: '20' });
+  });
+
+  it('keeps the cursor when setSort is called with the same key', () => {
+    const pager = new CursorPager('created_at');
+    pager.advance('cursor-mid-list');
+    pager.setSort('created_at');
+    expect(pager.atStart).toBe(false);
+  });
+});

frontend/src/lib/api/__tests__/path-encoding.test.ts

@@ -0,0 +1,73 @@
+import { readdirSync, readFileSync } from 'fs';
+import { join } from 'path';
+import { fillPath, fillPathParams } from '../paths';
+
+const API_DIR = join(__dirname, '..');
+
+describe('fillPath', () => {
+  it('URI-encodes the substituted value', () => {
+    expect(fillPath('/api/v1/markets/{market_id}/resolve', 'market_id', 'a/b?c#d')).toBe(
+      '/api/v1/markets/a%2Fb%3Fc%23d/resolve',
+    );
+  });
+
+  it('encodes exactly once (no double-encoding through a chain)', () => {
+    const once = fillPath('/x/{id}', 'id', 'a b');
+    expect(once).toBe('/x/a%20b');
+    // Feeding an already-filled path back in is a no-op (nothing left to replace).
+    expect(fillPath(once, 'id', 'ignored')).toBe(once);
+  });
+
+  it('fillPathParams substitutes every placeholder', () => {
+    expect(
+      fillPathParams('/u/{user}/tx/{hash}', { user: 'a/b', hash: 'x#y' }),
+    ).toBe('/u/a%2Fb/tx/x%23y');
+  });
+});
+
+describe('path values containing / ? # round-trip through a request', () => {
+  const originalFetch = global.fetch;
+
+  beforeEach(() => {
+    global.fetch = jest.fn().mockResolvedValue({
+      ok: true,
+      text: async () => JSON.stringify({ ok: true }),
+    });
+  });
+  afterEach(() => {
+    global.fetch = originalFetch;
+  });
+
+  const requestedUrl = () => (global.fetch as jest.Mock).mock.calls[0][0] as string;
+
+  it('a market id with special characters lands in the path, not as a new segment or query', async () => {
+    const { api } = await import('../public-client');
+    await api.getBlockchainMarket('m/1?x#y');
+
+    const url = new URL(requestedUrl());
+    // The whole value is one encoded path segment - no stray `/`, `?`, or `#`.
+    expect(url.pathname).toBe('/api/v1/blockchain/markets/m%2F1%3Fx%23y');
+    expect(url.search).toBe('');
+  });
+
+  it('a tx hash with a slash does not escape the path', async () => {
+    const { api } = await import('../public-client');
+    await api.getTransactionStatus('deadbeef/../secret');
+
+    const url = new URL(requestedUrl());
+    expect(url.pathname).toBe('/api/v1/blockchain/tx/deadbeef%2F..%2Fsecret');
+  });
+});
+
+describe('encodeURIComponent is centralized in paths.ts', () => {
+  it('no other src/lib/api module calls encodeURIComponent directly', () => {
+    const offenders: string[] = [];
+    for (const entry of readdirSync(API_DIR, { withFileTypes: true })) {
+      if (!entry.isFile() || !entry.name.endsWith('.ts')) continue;
+      if (entry.name === 'paths.ts' || entry.name.endsWith('.d.ts')) continue;
+      const source = readFileSync(join(API_DIR, entry.name), 'utf8');
+      if (source.includes('encodeURIComponent')) offenders.push(entry.name);
+    }
+    expect(offenders).toEqual([]);
+  });
+});

frontend/src/lib/api/admin-client.ts

@@ -16,7 +16,8 @@ export {
   CacheTag,
 } from './public-client';
 
-import { api as publicApi, CacheTag, fillPath } from './public-client';
+import { api as publicApi, CacheTag } from './public-client';
+import { fillPath } from './paths';
 import { apiCache, CACHE_TTL } from './cache';
 import { getEnvConfig } from '../env';
 import type { paths, components } from './schema';
@@ -337,7 +338,7 @@ export const api = {
     }),
 
   emailPreview: (templateName: string, signal?: AbortSignal) =>
-    request<Record<string, unknown>>("GET", `/api/v1/email/preview/${encodeURIComponent(templateName)}`, {
+    request<Record<string, unknown>>("GET", fillPath("/api/v1/email/preview/{template_name}", 'template_name', templateName), {
       cacheTtl: CACHE_TTL.LONG,
       cacheTags: [CacheTag.EMAIL],
       signal,