deckhearth/.convoys/single-auth-provider.md
Randall Stillwell 3022e80c04 refactor(auth): collapse lib/auth-context.js + lib/admin-auth.js onto lib/use-auth.js
`lib/use-auth.js` is now the sole client-side auth surface (P1 §9 of
`.convoys/ship-readiness.md`). The legacy `lib/auth-context.js`
(`AuthProvider` + `useAuth`) and `lib/admin-auth.js` (`AdminProvider` +
`useAdmin` + `useIsAdmin`) are deleted; every importer is migrated to
the canonical hook. Pre-convoy a worst-case page mount issued THREE
identical `GET /api/auth/verify` requests (one per provider/hook); the
post-convoy floor is one verify per page mount (3 → 1 on
`pages/card/[id].js`, 2 → 1 elsewhere).

Importer inventory swept (7 source files):
- `pages/_app.js` — removed `<AuthProvider>` wrapper; `<ThemeProvider>`
  is now the only top-level provider. `lib/use-auth.js` is hook-only,
  no replacement provider needed.
- `pages/index.js`, `pages/scanner.js`, `pages/decks.js`,
  `pages/deck/[id].js`, `pages/deck-builder.js` — `import { useAuth }`
  path swap from `../lib/auth-context` to `../lib/use-auth`. All five
  pages destructured only `{ user }` or `{ user, loading }`; verified
  no consumer reads `login` / `register` from useAuth (those flows are
  in `pages/login.js` / `pages/signup.js` which call the API directly),
  so no shape-parity gap on `lib/use-auth.js`.
- `pages/card/[id].js` — replaced `useIsAdmin()` (the only consumer of
  `lib/admin-auth.js` anywhere in the tree) with synchronous
  `user?.role === 'admin'` derived from the existing `useAuth()` call.
  Render condition at line 524 stays byte-identical.

Decisions documented in `.convoys/single-auth-provider.md`:
- D1: no extension to `lib/use-auth.js` (zero call sites for `login` /
  `register` from useAuth — those flows are direct fetches in
  `login.js` / `signup.js`).
- D2: `useIsAdmin()` collapses onto `useAuth()`; no separate hook.
- D3: provider tree `<ThemeProvider><AuthProvider>{children}</AuthProvider></ThemeProvider>`
  → `<ThemeProvider>{children}</ThemeProvider>`.
- D4: 3 → 1 verify roundtrip on `card/[id].js`; 2 → 1 on every other
  page-load.
- D5: zero test files modified; the 21-test vitest suite is server-
  side or prop-driven (`Layout.test.js` passes `user` as a prop, never
  imports the legacy hooks).

Doc / config updates so the deletion lands cleanly:
- `.github/CODEOWNERS` — drop the two CODEOWNERS lines for the deleted
  files.
- `AGENTS.md` § 2 architecture row + § 3 "Auth (client)" bullet —
  rewritten for the post-convoy single-surface state.
- `.cursor/rules/auth-and-permissions.mdc` — § "Legacy" reframed to
  "deleted by this convoy"; § "Authentication state on the client"
  updated to the post-convoy `useAuth()` shape and the direct-fetch
  login flow used by `login.js` / `signup.js`.
- `.cursor/rules/no-go-zones.mdc` — auth-refactors bullet drops the
  deleted files from the canonical list.
- `.cursor/skills/add-page/SKILL.md` — checklist + anti-pattern row
  refer to the deletion.

Verification:
- `rg "lib/auth-context|lib/admin-auth" --type js` → 0 hits in source.
- `npm run lint` → 128 → 125 problems (3 fewer errors from the deleted
  unused-import lines; no regression).
- `npm run test:run` → 21/21 pass (including the 5 Layout regression
  locks from `fix-layout-default-user`, which are prop-driven and
  unaffected).
- `npm run build` → all 26 pages compile end-to-end; no SSR / static-
  generation breakage that would have surfaced if a page tried to use
  the legacy context hook unwrapped.
- Manual smoke deferred to operator post-merge per convoy doc.

Risks (full discussion in convoy file):
- R1 shape parity gap — verified zero consumers of legacy-only
  surface; mitigated.
- R2 SSR mismatch from removing `<AuthProvider>` — `useEffect`-
  guarded `localStorage` read; identical SSR shape pre/post; build
  passes.
- R3 missed importer — post-delete grep + build pass would surface
  any miss.
- R5 stale `useAuth` cache across components — pre-existing
  pattern, called out as follow-up rather than addressed here.

Out of scope: any change to `lib/permission-middleware.js` (server-
side; resolved P0 #1), `lib/auth-secret.js` (resolved P0 #2),
`pages/api/**` route handlers, login / register API contracts, or
the seeded admin account flow.

Co-authored-by: Cursor <cursoragent@cursor.com>
2026-05-26 22:54:35 -05:00

16 KiB
Raw Blame History

single-auth-provider (P1 quality — collapse three client auth surfaces onto one)

Status: OPEN 2026-05-26 (this convoy) Priority: P1 quality (launch sequence step 9 — .convoys/ship-readiness.md § P1 entry 9) Convoy owner: parent (architect + implementer rolled together — diff is mechanical once the shape-parity decision is made) Branch: convoy/single-auth-provider Opened: 2026-05-26

Background

The repo carried three parallel client-side auth implementations since the early days of the project. .convoys/ship-readiness.md § P1 entry 9 ("Three parallel client-side auth implementations") is the canonical spec; AGENTS.md § 3 already documented lib/use-auth.js as the canonical surface and instructed new code to avoid the other two. This convoy executes the collapse.

The three surfaces:

  1. lib/use-auth.js::useAuth (the keeper). Hook-only — reads auth_token from localStorage on mount, hits /api/auth/verify, exposes { user, loading, logout, refreshAuth }. No React context, no <Provider> wrapper required.
  2. lib/auth-context.js::{ AuthProvider, useAuth } (legacy). Context provider + consumer hook with the same verify-on-mount semantics, plus login() and register() helpers that pages/login.js / pages/signup.js no longer use (those pages call /api/auth/{login,register} directly and write the token to localStorage themselves). Wired in pages/_app.js as <AuthProvider>.
  3. lib/admin-auth.js::{ AdminProvider, useAdmin, useIsAdmin } (legacy). A redundant context that does the same verify-on-mount roundtrip, plus a hook-only useIsAdmin() that does its own verify roundtrip on top of that. AdminProvider is not wired in _app.js (verified by reading _app.js pre-convoy: only <ThemeProvider> + <AuthProvider>), so useAdmin() would have thrown at runtime if anyone called it — nobody does. Only useIsAdmin() has a live consumer (pages/card/[id].js).

Symptom that drove ranking this P1. When pages/card/[id].js mounts, it calls useAuth() from lib/use-auth.js AND useIsAdmin() from lib/admin-auth.js, each issuing its own GET /api/auth/verify. With AuthProvider mounted on every page via _app.js, that's a third verify roundtrip on the very first page load. Three roundtrips, identical request, serial cost on a cold connection. Post-convoy: 1 roundtrip per page-load.

Decisions

D1 — Shape parity check on lib/use-auth.js. Verdict: no parity gap; do not extend.

lib/auth-context.js::useAuth() exposed { user, loading, login, register, logout }. lib/use-auth.js::useAuth() exposes { user, loading, logout, refreshAuth }.

The apparent gap is login / register. Verified-by-grep: zero call sites invoke useAuth().login(…) or useAuth().register(…) anywhere in pages/** or components/**. The only callers of those flows are pages/login.js and pages/signup.js, both of which fetch /api/auth/{login,register} directly and write the returned token to localStorage. useAuth()'s useEffect then picks up the new token on the next mount (or the page can call refreshAuth() to re-verify in place).

Conclusion: do not add login / register to use-auth.js. The legacy methods were dead code on the consumer surface; preserving them would be cargo-culting and would re-create a non-DRY login flow (one in pages/login.js, one in the hook). auth-and-permissions.mdc § "Authentication state on the client" was updated to document the post-convoy useAuth() shape and to spell out the login.js / signup.js direct-fetch pattern.

D2 — useIsAdmin() migration shape. Verdict: collapse onto the existing useAuth() call.

pages/card/[id].js is the only consumer of useIsAdmin(). The page already called useAuth() from lib/use-auth.js at line 13 (added by fix-layout-default-user Brief 2). The migration is:

// Before
const { user } = useAuth();
// ...
const { isAdmin, loading: adminLoading } = useIsAdmin();
// ... usage at line 524: {isAdmin && !adminLoading && (...)}

// After
const { user, loading: authLoading } = useAuth();
// ...
const isAdmin = user?.role === 'admin';
const adminLoading = authLoading;
// ... usage at line 524 unchanged: {isAdmin && !adminLoading && (...)}

adminLoading is kept as a local alias rather than substituting authLoading directly at the call site, to keep the diff minimal and the rendering condition byte-identical. The loading window from useAuth() covers exactly the same period (/api/auth/verify resolution) that useIsAdmin's own loading covered, so there is no UX regression.

D3 — pages/_app.js provider tree. Before / after.

// Before
<ThemeProvider>
  <AuthProvider>
    <Component {...pageProps} />
  </AuthProvider>
</ThemeProvider>

// After
<ThemeProvider>
  <Component {...pageProps} />
</ThemeProvider>

useAuth() from lib/use-auth.js is hook-only — no Provider needed. The <AuthProvider> wrapper is removed entirely; no replacement Provider is added. <ThemeProvider> stays (out of scope). <AdminProvider> was never in the tree to begin with.

D4 — Token-verify roundtrip count.

Per the spec: pre-convoy a worst-case page mount issued 3 identical GET /api/auth/verify requests:

  1. <AuthProvider> in _app.js calls verifyToken() on mount.
  2. pages/card/[id].js calls useAuth() from lib/use-auth.js, which calls checkAuth() on mount → another verify.
  3. The same page calls useIsAdmin() from lib/admin-auth.js, which calls its inline checkAdmin() on mount → another verify.

Post-convoy:

  1. <AuthProvider> is gone.
  2. pages/card/[id].js calls useAuth() once → 1 verify.
  3. useIsAdmin() call site is gone; admin status is computed synchronously from the same user returned by step 2.

Net: 3 → 1 verify roundtrip on card/[id].js mount. Other pages drop from 2 → 1 (no useIsAdmin involved, but <AuthProvider> was). The 1× pattern is the floor; further reduction would require server-side hydration of the user object, which is a separate architectural conversation (out of scope; see Follow-ups).

D5 — Test impact. Verdict: zero test files modified.

The 21-test vitest suite covers:

  • test/lib/auth-secret.test.js (3) — server-side, untouched by this convoy.
  • test/lib/permission-middleware.test.js (8) — server-side, untouched.
  • test/api/auth-utils.test.js (5) — server-side, untouched.
  • test/components/Layout.test.js (5) — passes user as a prop, not via any hook. The legacy auth-context and admin-auth modules are not imported. Unaffected.

All four files were grep-checked for auth-context|admin-auth|use-auth references — zero hits. No test was written against the legacy hooks themselves; the deletion is risk-free from a test-suite perspective. Vitest stays green at 21/21 post-convoy.

Importer inventory

Generated via rg "from ['\"].*lib/auth-context['\"]" --type js and rg "from ['\"].*lib/admin-auth['\"]" --type js against the worktree (excluding docs / convoys).

Importers of lib/auth-context.js (6 source files)

File Symbol Migration
pages/_app.js AuthProvider Wrapper removed; no replacement (D3)
pages/index.js useAuth Path swap → lib/use-auth.js
pages/scanner.js useAuth Path swap → lib/use-auth
pages/decks.js useAuth Path swap → lib/use-auth
pages/deck/[id].js useAuth Path swap → lib/use-auth (depth ../../)
pages/deck-builder.js useAuth Path swap → lib/use-auth

All 5 page-level useAuth consumers destructured only { user } or { user, loading } (verified by grep). No login / register / other-method consumer found, confirming D1.

Importers of lib/admin-auth.js (1 source file)

File Symbol Migration
pages/card/[id].js useIsAdmin Replaced with user?.role === 'admin' from existing useAuth() (D2)

AdminProvider and useAdmin() had zero importers in the source tree — confirming they were dead exports.

Adjacent doc / config edits

File Change
pages/_app.js Removed import { AuthProvider } from '../lib/auth-context.js' and the wrapper
.github/CODEOWNERS Removed the two CODEOWNERS lines for the deleted files
AGENTS.md § 2 + § 3 Updated the Auth row of the architecture table and the "Auth (client)" convention bullet to describe the post-convoy single-surface state
.cursor/rules/auth-and-permissions.mdc Reframed § "Legacy" to "deleted by this convoy"; updated § "Authentication state on the client" to the post-convoy useAuth() shape and the direct-fetch login flow
.cursor/rules/no-go-zones.mdc Auth-refactors bullet updated to drop the deleted files
.cursor/skills/add-page/SKILL.md Updated checklist bullet + anti-pattern row to refer to the deletion

.convoys/** and .convoys/fix-layout-default-user/** were not edited — those are historical convoy records and are append-only by repo convention. The doc-writer post-convoy sweep will add the as-shipped section at the bottom of this file plus update .convoys/ship-readiness.md § P1 → entry 9 with the squash commit reference.

The fix (per-category translation rules)

Category A — useAuth from auth-contextuseAuth from use-auth

// before
import { useAuth } from '../lib/auth-context';   // or auth-context.js
// after
import { useAuth } from '../lib/use-auth';       // or use-auth.js

The destructure pattern (const { user } = useAuth() / const { user, loading } = useAuth()) stays byte-identical. No call-site changes.

Category B — AuthProvider wrapper in _app.js

// before
import { AuthProvider } from '../lib/auth-context.js';
return (
  <ThemeProvider>
    <AuthProvider>
      <Component {...pageProps} />
    </AuthProvider>
  </ThemeProvider>
);

// after
return (
  <ThemeProvider>
    <Component {...pageProps} />
  </ThemeProvider>
);

Plus delete the import line.

Category C — useIsAdmin in pages/card/[id].js

See D2 for the full diff. Three line-ranges touched: the import block, the useAuth destructure, and the useIsAdmin line block. Usage at line 524 is unchanged.

Category D — useAdmin, AdminProvider

No call sites. No work to do; these symbols disappear when the file is deleted.

Verification plan

  1. rg "lib/auth-context|lib/admin-auth" --type js → expect zero hits in pages/, lib/, components/. Achieved.
  2. npm run lint → baseline 128 problems pre-convoy → 125 problems post-convoy (3 fewer errors, since the deleted files contained 3 unused-import / unused-var lints; no new lint surface introduced). No regression.
  3. npm run test:run → 21/21 pass pre- and post-convoy. Layout test confirmed unaffected.
  4. npm run build → succeeds end-to-end. All 26 pages compile (10 dynamic API routes + 16 pages/** views including card/[id], _app, decks, deck/[id], deck-builder, scanner, index — every file modified by the sweep). No SSR-level breakage; importantly no "useAuth must be used within an AuthProvider" runtime error during static generation, which would have indicated the page tried to use the legacy context hook unwrapped.
  5. Manual smoke: deferred — the build pass + vitest pass + zero-hit grep is the gate for merging; the parent does not have a logged-in admin browser session ready in this conversation. Documenting in As-shipped post-merge once the operator runs npm run dev and exercises dashboard / profile / settings / collections / cards / admin/card-editor.

Risks

  • R1 — Shape parity gap breaks runtime auth state. Mitigated by D1. The grep audit confirmed no consumer reads login / register / any other surface that exists on the legacy hook but not on use-auth. loading and user were preserved with identical semantics.
  • R2 — SSR mismatch from removing <AuthProvider>. Mitigated. lib/use-auth.js reads localStorage inside a useEffect, so SSR sees user === null, loading === true and never touches the browser-only API on the server — same guarded shape as the legacy provider. npm run build confirms no SSR error during static generation. (auth-context.js's useEffect had the same guard, so removing the provider didn't change the SSR surface.)
  • R3 — Missed importer. Mitigated. Post-delete grep over --type js returned zero hits. The deletion would itself surface any missed importer at module-load time during npm run build (Node would throw "Cannot find module"); build succeeded.
  • R4 — Verify-roundtrip dedup creates a regression where a page never re-verifies. Mitigated. Pre-convoy, three providers each ran their own verify on mount but they did not coordinate state — one provider's success had no effect on another's loading flag. Post-convoy we have a single source of truth. Pages that need to re-verify (e.g. after an action that might have invalidated the token) can call refreshAuth() from the same hook; no consumer currently does this, but the surface is preserved for future use.
  • R5 — Stale useAuth cache across components. Out of scope; see Follow-ups. Each useAuth() call site instantiates its own state via useState. Two components on the same page that both call useAuth will issue two verify roundtrips and hold two independent user references. This was true pre-convoy too (the legacy useIsAdmin was already a separate verify). Hoisting state into a shared module-level cache or wrapping useAuth in a context (the very thing we just removed!) is a separate decision — see "Follow-ups".

As-shipped

Stub for doc-writer post-merge:

  • Squash commit: <TBD>
  • PR: #<TBD>
  • Files changed: 13 (2 deletions: lib/auth-context.js, lib/admin-auth.js; 11 modifications: pages/_app.js, pages/index.js, pages/scanner.js, pages/decks.js, pages/deck/[id].js, pages/deck-builder.js, pages/card/[id].js, .github/CODEOWNERS, AGENTS.md, .cursor/rules/auth-and-permissions.mdc, .cursor/rules/no-go-zones.mdc, .cursor/skills/add-page/SKILL.md).
  • Verify roundtrip count: documented 3 → 1 on card/[id].js, 2 → 1 on every other page-load.
  • .convoys/ship-readiness.md § P1 entry 9 to be marked RESOLVED with this convoy's squash SHA.
  • Lint baseline updated 128 → 125 (no regression; 3 fewer errors from deleted unused-import lines).

Follow-ups (out of scope here)

  • Component-level useAuth cache audit. Two components on the same page that both call useAuth() will issue two verify roundtrips. This was the original motivation for the legacy context, and was the one legitimate thing those providers did right. A future convoy should consider either (a) returning a shared module-level state via a small Zustand-style store, (b) reintroducing a thin <AuthProvider> that only hoists state without re-implementing fetch logic, or (c) accepting the duplicate roundtrip as the price of hook-only simplicity. Today's call sites already deduplicate at the page level (one useAuth per page is the prevailing pattern), so this is a soft optimisation, not a correctness fix.
  • Rate-limit-aware re-auth on 429. lib/use-auth.js's checkAuth does not currently back off if /api/auth/verify returns 429 (the rate-limiter from add-rate-limiting would only kick in if a single client exceeded 60 verify calls / minute, which is unrealistic in practice but worth a defensive guard).
  • Server-side hydration of user. The page-mount verify roundtrip is unavoidable in this hook-only shape because the token is only readable on the client. Moving to an HTTP-only cookie + Next.js getServerSideProps hydration would eliminate the round-trip entirely and is a larger architectural conversation that should not piggyback on a quality convoy.
  • Doc-writer cleanup. Update .convoys/ship-readiness.md § P1 → entry 9 with the RESOLVED stamp + squash SHA; trim the "three parallel surfaces" framing from any other doc that still mentions it; refresh the "Auth refactors" no-go-zones bullet if any other files become canonical (none today).