deckhearth/.convoys/single-auth-provider.md
Randall Stillwell a941d4db1f docs: post-convoy cleanup for 7-convoy 2026-05-26 wave
Updates ship-readiness.md, AGENTS.md, and 7 convoy files to reflect
the as-shipped state of the 2026-05-26 7-convoy multitask wave:

- PR #26 tighten-visual-diff-path-filter (P3)
- PR #27 purge-weak-creds-from-helpers (P2, closes the umbrella)
- PR #28 cleanup-mobile-nav-dead-props (P3)
- PR #29 lint-against-cjs-in-esm-scripts (P3, surfaced by PR #25)
- PR #30 single-sql-client (P1 #8 RESOLVED)
- PR #31 single-auth-provider (P1 #9 RESOLVED)
- PR #32 migration-tool (P1 #11 RESOLVED)

Milestone: 5 of 6 P1 quality items RESOLVED. Only fix-lint-baseline
(P1 #11.5) remains in the P1 lane.

Newly queued follow-ups:
- purge-quick-login-from-loginpage (surfaced by PR #27)
- purge-neondatabase-serverless-fully (surfaced by PR #30,
  unblocked by PR #32's migration tool adoption)

Co-authored-by: Cursor <cursoragent@cursor.com>
2026-05-26 23:12:26 -05:00

432 lines
23 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# 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:
```js
// 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.
```jsx
// 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-context` → `useAuth` from `use-auth`
```js
// 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`
```jsx
// 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
Single squash commit `0668b0c` (PR #31, merged 2026-05-27T03:58:08Z
UTC / local 2026-05-26). Parent-owned end-to-end per the "Convoy owner"
line — no architect, no implementer subagent dispatched. Mirror-the-pattern
fix exactly as planned; no mid-execution surprises. **AGENTS.md § 2
Architecture quick reference + § 3 Conventions ("Auth (client)") +
`.cursor/rules/auth-and-permissions.mdc` swept to describe the
post-convoy single-surface state in the same wave.**
**Diff: 15 files, +341 / -263.** 2 file deletions
(`lib/auth-context.js`, `lib/admin-auth.js`); 12 file modifications
(7 source pages + `.github/CODEOWNERS` + 4 docs / rules / skills);
1 new convoy planning file (`.convoys/single-auth-provider.md`).
**The collapse shipped exactly as designed:**
1. **`lib/auth-context.js` + `lib/admin-auth.js` deleted.** No
replacement; `lib/use-auth.js`'s hook-only `useAuth()` is the sole
client auth surface.
2. **6 importers of `lib/auth-context.js` swept** per the inventory
table — path swap `'../lib/auth-context'``'../lib/use-auth'`
(each file's relative depth preserved). Destructure pattern
(`const { user } = useAuth()` / `const { user, loading } = useAuth()`)
stays byte-identical. Affected files: `pages/_app.js`,
`pages/index.js`, `pages/scanner.js`, `pages/decks.js`,
`pages/deck/[id].js`, `pages/deck-builder.js`.
3. **`<AuthProvider>` wrapper removed from `pages/_app.js`.** Per D3:
`useAuth()` from `lib/use-auth.js` is hook-only, no Provider
needed. `<ThemeProvider>` stays. `<AdminProvider>` was never in
the tree to begin with (confirmed by reading `_app.js` pre-convoy
— only `<ThemeProvider>` + `<AuthProvider>`).
4. **`useIsAdmin()`'s lone consumer inlined.** `pages/card/[id].js`
was the only consumer; replaced `const { isAdmin, loading:
adminLoading } = useIsAdmin()` with `const isAdmin = user?.role
=== 'admin'; const adminLoading = authLoading;` from the existing
`useAuth()` call. Rendering condition at line 524 (`{isAdmin &&
!adminLoading && (...)}`) unchanged byte-for-byte; the
`adminLoading` alias is kept rather than substituting
`authLoading` directly to keep the diff minimal.
5. **`AdminProvider` and `useAdmin()` had ZERO importers** in the
source tree — confirming they were dead exports (only `useIsAdmin`
had a live consumer). Deleted together with `lib/admin-auth.js`;
no per-file sweep needed for them.
6. **Verify roundtrip count reduced 3 → 1** on `pages/card/[id].js`
mount, and 2 → 1 on every other page-load. Pre-convoy worst case
was `<AuthProvider>` verify + `useAuth()` verify + `useIsAdmin()`
verify (3 identical `GET /api/auth/verify` requests, serial cost
on cold connection). Post-convoy: single `useAuth()` verify per
page; admin status computed synchronously from the same `user`.
7. **`.github/CODEOWNERS`** lines for the two deleted files removed.
8. **Doc surface updated atomically** (the 4 docs / rules / skills
modifications in the diff stat): `AGENTS.md` § 2 (Auth + DB libs
row reframed to the post-convoy single-surface state) + § 3
("Auth (client)" convention bullet rewritten); `.cursor/rules/auth-and-permissions.mdc`
(§ "Legacy" reframed to "deleted by this convoy"; § "Authentication
state on the client" rewritten to the post-convoy `useAuth()` shape
+ the direct-fetch login flow from `pages/login.js` /
`pages/signup.js`); `.cursor/rules/no-go-zones.mdc` (auth-refactors
bullet trimmed of the deleted files); `.cursor/skills/add-page/SKILL.md`
(checklist + anti-pattern row updated to refer to the deletion).
**Verification (all gates green at merge):**
- `rg "lib/auth-context|lib/admin-auth" --type js` → 0 hits in
`pages/`, `lib/`, `components/` post-edit (R3 mitigation —
confirms no missed importer; any missed importer would also have
surfaced at module-load time during `npm run build` as "Cannot
find module", which did not happen).
- `npm run lint`**128 → 125 problems** (3 fewer errors; the
deleted files contained 3 unused-import / unused-var lints; no new
lint surface introduced). **This is the new lint baseline** for
subsequent convoys.
- `npm run test:run` → 21/21 pass pre- and post-convoy. The 4 test
files don't import any of the deleted modules (`grep`-confirmed
pre-convoy); Layout test confirmed unaffected.
- `npm run build` → succeeds end-to-end. All 26 pages compile (10
dynamic API routes + 16 `pages/**` views including every file
modified by the sweep). **No "useAuth must be used within an
AuthProvider" runtime error during static generation**, which
confirms `<AuthProvider>` removal is safe — no page tried to use
the legacy context hook unwrapped.
- CI on PR #31: Lint ✓ (125 problems baseline) | Vitest 21/21 ✓ |
Playwright smoke 3/3 ✓ | `forbidden-endpoints` ✓ |
`forbidden-cors-headers` ✓ | Vercel preview deploy ✓ | Aggregate
gate ✓
- `Screenshot diff`: triggered (PR #31 touches `pages/**` non-API +
some adjacent surface that the visual-diff path filter matches) —
`continue-on-error: true` swallow per the documented Decision-4
end state of `adopt-playwright-smoke` (no baseline committed yet).
**Manual smoke deferred** per § Verification plan step 5 — the build
pass + vitest pass + zero-hit grep is the gate for merging; the
parent did not have a logged-in admin browser session ready in this
conversation. Optional post-merge operator sequence: `npm run dev` +
exercise dashboard / profile / settings / collections / cards /
admin/card-editor to confirm no runtime regression.
**Cross-validation finding (continues the lineage).** `Playwright
smoke` 3/3 PASS on a 15-file sweep confirms the deployed preview is
unaffected by the 2-deletion auth-surface collapse. Specifically,
smoke test 2 (`'sign-in page renders'`) continues to assert against
the `<Link href="/login">Sign in</Link>` CTA introduced by
`fix-layout-default-user` (PR #15) — that CTA is unchanged here, and
its render path doesn't depend on `<AuthProvider>`. **Ninth
consecutive convoy** where the same 3-test smoke spec defends the
auth surface (PR #15#19#20#21#25#32#27#30
this PR).
**Operator action required going forward:** **none.** No env vars,
no schema, no infra changes. The single client auth surface is
`lib/use-auth.js::useAuth()`; the direct-fetch login flow in
`pages/login.js` / `pages/signup.js` is preserved verbatim (no
client-side credential handling moved).
**Spec deviation (documented as the as-shipped reality):** the
pre-merge importer estimate was ~30 in `.convoys/ship-readiness.md`
P1 #9; actual was 7. This is not a real spec deviation — it's a
loose estimate that was correct at the time the estimate was made
but became stale once `fix-layout-default-user` (PR #15) migrated
most of the tree to `lib/use-auth.js`. The post-flip ship-readiness
P1 #9 entry records the actual count.
**Cross-convoy follow-up (R5 still open).** § Risks R5 — two
components on the same page that both call `useAuth()` will issue
two verify roundtrips and hold two independent `user` references —
is intentionally out of scope. This was true pre-convoy too (the
legacy `useIsAdmin` was already a separate verify). Hoisting state
into a shared module-level cache or reintroducing a thin
`<AuthProvider>` that only hoists state is a separate decision; see
§ Follow-ups in this convoy file.
## 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).