Merge remote-tracking branch 'origin/main' into convoy/migration-tool
Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # .cursor/rules/no-go-zones.mdc
This commit is contained in:
commit
b99e885a80
29 changed files with 1657 additions and 351 deletions
155
.convoys/cleanup-mobile-nav-dead-props.md
Normal file
155
.convoys/cleanup-mobile-nav-dead-props.md
Normal file
|
|
@ -0,0 +1,155 @@
|
||||||
|
# cleanup-mobile-nav-dead-props (P3 polish — single-prop hygiene)
|
||||||
|
|
||||||
|
**Status:** SHIPPED 2026-05-26 (PR TBD)
|
||||||
|
**Classification:** hygiene
|
||||||
|
**Priority:** P3 polish (not a bug, not a security issue; dead-prop
|
||||||
|
removal is purely a clarity-of-surface cleanup)
|
||||||
|
**Convoy owner:** parent (no architect — single-line prop removal in
|
||||||
|
one component + one caller; surfaced and pre-decided in a sibling
|
||||||
|
convoy)
|
||||||
|
**Opened:** 2026-05-26
|
||||||
|
|
||||||
|
## Background
|
||||||
|
|
||||||
|
`components/MobileNavigation.js` accepts `{ user, onMenuOpen }` but
|
||||||
|
never reads `user.*` — the bottom-bar items (Cards, Decks, Dashboard,
|
||||||
|
Community, More) are static and don't depend on auth state or role.
|
||||||
|
|
||||||
|
This was originally surfaced as **R8** in the `fix-layout-default-user`
|
||||||
|
convoy (see `.convoys/fix-layout-default-user.md` § R8 and § "Anything
|
||||||
|
flagged but not acted on") and deferred there with explicit
|
||||||
|
instructions: *"If the implementer is tempted to delete the prop, they
|
||||||
|
MUST stop — that's god-component-split / single-auth-provider
|
||||||
|
territory."* The deferral was correct for that convoy's scope; it is
|
||||||
|
no longer needed because the prop is genuinely dead at the current
|
||||||
|
static-bar reality, and removing it does not require a wider auth
|
||||||
|
refactor.
|
||||||
|
|
||||||
|
The follow-up was queued as `cleanup-mobile-nav-dead-props` in
|
||||||
|
`.convoys/ship-readiness.md` § Queued convoys, with a note that it
|
||||||
|
may fold into `god-component-split` (P2 #13) if that lands first.
|
||||||
|
God-component-split has not landed; this small hygiene convoy ships
|
||||||
|
first.
|
||||||
|
|
||||||
|
## Audit results
|
||||||
|
|
||||||
|
Pre-edit audit (the spec's "don't blindly trust the queue entry"
|
||||||
|
clause):
|
||||||
|
|
||||||
|
1. **Reading `components/MobileNavigation.js`** — the file is 171
|
||||||
|
lines. Line 5 destructures `{ user, onMenuOpen }`. Lines 6–170 use
|
||||||
|
`onMenuOpen` exactly once (line 39, as the `onClick` for the "More"
|
||||||
|
button). `user` does not appear elsewhere — no `user.email`,
|
||||||
|
`user.role`, `user.id`, no conditional render gated on `user`, no
|
||||||
|
pass-through to a child component. The bottom-bar `navigationItems`
|
||||||
|
array is hardcoded and does not branch on auth state.
|
||||||
|
2. **`rg '\buser\b' components/MobileNavigation.js`** before edit: 1
|
||||||
|
hit (the destructure on line 5). After edit: 0 hits.
|
||||||
|
3. **`rg "MobileNavigation" components/ pages/ --type js`**: two
|
||||||
|
import + JSX-callsite pairs in the codebase:
|
||||||
|
- `components/Layout.js` (active) — line 5 import, line 598-601 JSX
|
||||||
|
call passing `user={user}` and `onMenuOpen={...}`.
|
||||||
|
- `components/Layout.js.backup` (no-go-zone per
|
||||||
|
`.cursor/rules/no-go-zones.mdc` § "Append-only / historical" —
|
||||||
|
"legacy snapshot; delete with a real PR, never edit") — line 5
|
||||||
|
import, line 264 JSX call. Left untouched per the no-go-zone
|
||||||
|
rule; if/when the `.backup` file is eventually deleted, this dead
|
||||||
|
call disappears with it.
|
||||||
|
|
||||||
|
Audit verdict: `user` is genuinely dead. Cleanup is safe.
|
||||||
|
|
||||||
|
## The fix
|
||||||
|
|
||||||
|
Two-file, three-line diff:
|
||||||
|
|
||||||
|
1. **`components/MobileNavigation.js` line 5**: remove `user` from the
|
||||||
|
destructured props.
|
||||||
|
- Before: `export default function MobileNavigation({ user, onMenuOpen }) {`
|
||||||
|
- After: `export default function MobileNavigation({ onMenuOpen }) {`
|
||||||
|
2. **`components/Layout.js` lines 598-601**: remove the `user={user}`
|
||||||
|
JSX attribute from the only active call site.
|
||||||
|
- Before:
|
||||||
|
```
|
||||||
|
<MobileNavigation
|
||||||
|
user={user}
|
||||||
|
onMenuOpen={() => setIsMobileMenuOpen(true)}
|
||||||
|
/>
|
||||||
|
```
|
||||||
|
- After:
|
||||||
|
```
|
||||||
|
<MobileNavigation
|
||||||
|
onMenuOpen={() => setIsMobileMenuOpen(true)}
|
||||||
|
/>
|
||||||
|
```
|
||||||
|
|
||||||
|
No new code. No refactors. No tests added (the component has no
|
||||||
|
direct test coverage; `test/components/Layout.test.js` tests Layout's
|
||||||
|
logged-out branch and does not assert on `MobileNavigation`'s prop
|
||||||
|
shape).
|
||||||
|
|
||||||
|
## Out of scope
|
||||||
|
|
||||||
|
- The unused `import { useState } from 'react'` on
|
||||||
|
`components/MobileNavigation.js` line 3. The hook is imported but
|
||||||
|
not called. This is a pre-existing dead import unrelated to the
|
||||||
|
`user` prop; the convoy spec explicitly forbids "refactor anything
|
||||||
|
else in MobileNavigation.js (this is a single-prop removal)". A
|
||||||
|
future hygiene pass can sweep it (or it'll get caught by an
|
||||||
|
eventual lint-no-unused-imports rule).
|
||||||
|
- `components/Layout.js.backup` — no-go-zone, untouched.
|
||||||
|
|
||||||
|
## Verification plan
|
||||||
|
|
||||||
|
1. `rg '\buser\b' components/MobileNavigation.js` → 0 hits (post-edit
|
||||||
|
confirmation that the prop is truly gone, not just renamed).
|
||||||
|
2. `rg "MobileNavigation" components/ pages/ --type js` → confirm
|
||||||
|
each active call site passes only `onMenuOpen`.
|
||||||
|
3. `npm run lint` → 128 problems baseline preserved (no regression
|
||||||
|
introduced; no new dead-code/unused-var warnings created by the
|
||||||
|
change).
|
||||||
|
4. `npm run test:run` → 21/21 pass. Specifically,
|
||||||
|
`test/components/Layout.test.js` continues to pass — its
|
||||||
|
regression-lock assertions for the logged-out Layout branch
|
||||||
|
(Gotcha #8) do not depend on `MobileNavigation`'s prop shape, so
|
||||||
|
the dead-prop removal is invisible to that suite.
|
||||||
|
5. `npm run build` skipped — relying on Vercel preview CI. Trade-off:
|
||||||
|
single-prop removal in a leaf component is extremely low risk of
|
||||||
|
build-time regression, and the Playwright smoke + visual-diff
|
||||||
|
workflows on the PR will catch any Layout-rendering issue before
|
||||||
|
merge.
|
||||||
|
|
||||||
|
## Risks
|
||||||
|
|
||||||
|
- **R1 — A future feature that wants per-user bottom-bar items would
|
||||||
|
need to re-add the prop.** Hypothetical examples: showing an
|
||||||
|
unread-count badge on a "Notifications" tab gated on `user.id`, or
|
||||||
|
hiding the "Community" tab for unauthenticated visitors. **Accepted.**
|
||||||
|
Re-adding a prop is a one-line change when the feature actually
|
||||||
|
lands; carrying a dead prop "just in case" obscures the current
|
||||||
|
surface and adds nothing. The cleanup is correct for the
|
||||||
|
current static-bar reality; future features pay their own
|
||||||
|
add-the-prop cost.
|
||||||
|
- **R2 — `components/Layout.js.backup` still references the old prop
|
||||||
|
shape.** **Accepted.** The backup is a no-go-zone (per
|
||||||
|
`.cursor/rules/no-go-zones.mdc`) and is dead code by definition.
|
||||||
|
Touching it would violate the rule; leaving it as a stale snapshot
|
||||||
|
is the convention. When the backup is eventually deleted in a
|
||||||
|
separate convoy, this stale call disappears with it.
|
||||||
|
|
||||||
|
## Acceptance criteria
|
||||||
|
|
||||||
|
- 2 files modified, 3 lines net change (1 line edit + 1 attribute
|
||||||
|
removal from a multi-line JSX block).
|
||||||
|
- `npm run lint` exit 1 with 128 problems (baseline preserved).
|
||||||
|
- `npm run test:run` 21/21 pass.
|
||||||
|
- `rg '\buser\b' components/MobileNavigation.js` → 0 hits post-edit.
|
||||||
|
|
||||||
|
## Owns
|
||||||
|
|
||||||
|
Parent (single-prop removal in a leaf component; no architect or
|
||||||
|
implementer subagent required; audit confirms the queue entry's
|
||||||
|
premise).
|
||||||
|
|
||||||
|
## As-shipped
|
||||||
|
|
||||||
|
_To be filled in post-merge._
|
||||||
176
.convoys/lint-against-cjs-in-esm-scripts.md
Normal file
176
.convoys/lint-against-cjs-in-esm-scripts.md
Normal file
|
|
@ -0,0 +1,176 @@
|
||||||
|
---
|
||||||
|
name: lint-against-cjs-in-esm-scripts
|
||||||
|
classification: hygiene
|
||||||
|
success_metric: future helper scripts that re-introduce CJS `require()` calls under `package.json` "type": "module" fail at lint time, not at first execution
|
||||||
|
status: open
|
||||||
|
created: 2026-05-26
|
||||||
|
---
|
||||||
|
|
||||||
|
# lint-against-cjs-in-esm-scripts (P3 polish — parent-owned)
|
||||||
|
|
||||||
|
**Priority:** P3 polish (one-line ESLint rule; no architect required)
|
||||||
|
**Convoy owner:** parent
|
||||||
|
**Opened:** 2026-05-26
|
||||||
|
|
||||||
|
## Background — the recurring bug pattern
|
||||||
|
|
||||||
|
Since `bump-next-js` flipped `package.json` to `"type": "module"`,
|
||||||
|
any helper script under `scripts/` that uses CJS `require()` throws
|
||||||
|
`ReferenceError: require is not defined` on Node 22.x at first run.
|
||||||
|
The same bug has now bitten the repo twice in two convoys:
|
||||||
|
|
||||||
|
1. **`drop-public-setup` Brief 2** (commit `b63b509`, 2026-05-23):
|
||||||
|
`scripts/setup-neon-db.js` was still CJS post-`bump-next-js`; `npm
|
||||||
|
run setup-db` was silently broken until Brief 2 swept it to ESM
|
||||||
|
imports. The convoy retro called this out as "the seed script
|
||||||
|
silently stopped executing after `bump-next-js`."
|
||||||
|
2. **`fix-reset-db-script` Brief 1** (commit `3ab9bf8`, PR #25,
|
||||||
|
2026-05-26): three `require()` calls in `scripts/reset-db.js`
|
||||||
|
(lines 10, 12, 142) — same bug, same blast radius (`npm run
|
||||||
|
reset-db` throws `ReferenceError`), same fix shape (verbatim
|
||||||
|
mirror of post-`drop-public-setup` `setup-neon-db.js`).
|
||||||
|
|
||||||
|
Both bugs were caught at first run, not at lint time. A small
|
||||||
|
ESLint rule scoped to `scripts/**/*.js` would have caught both at
|
||||||
|
PR time and is cheap insurance against a third recurrence.
|
||||||
|
|
||||||
|
## Design decision — `scripts/**` only (NOT all `.js`)
|
||||||
|
|
||||||
|
Two reasonable scopes:
|
||||||
|
|
||||||
|
- **`scripts/**/*.js` (chosen):** matches the actual blast radius —
|
||||||
|
every observed instance of the bug has been in a helper script.
|
||||||
|
Per-file-block override in `eslint.config.mjs` via a second flat-
|
||||||
|
config entry. Zero impact on `pages/api/**` (already correctly
|
||||||
|
ESM-imported throughout) and zero impact on the root `*.config.js`
|
||||||
|
files (which are intentionally CJS-shaped and which the next-config
|
||||||
|
base rules already handle correctly).
|
||||||
|
- **All `.js` files at repo root (rejected):** broader-than-necessary
|
||||||
|
blast radius. `pages/api/**` already uses ESM `import` everywhere
|
||||||
|
(`add-rate-limiting`, `cors-tighten`, and `add-route` skill all
|
||||||
|
verified this in the last three months). A repo-wide ban would
|
||||||
|
produce zero true positives outside `scripts/**` today and would
|
||||||
|
risk breaking config-file shapes that legitimately use CJS
|
||||||
|
(`postcss.config.js`, `tailwind.config.js` are flagged by
|
||||||
|
`import/no-anonymous-default-export` today but read CJS-style
|
||||||
|
exports under the hood — see also Gotcha #9 + #10).
|
||||||
|
|
||||||
|
The scoped rule is a 7-line flat-config block; the broader rule
|
||||||
|
would require explicit allowlist for every config file, which is
|
||||||
|
strictly more code and more maintenance.
|
||||||
|
|
||||||
|
## The fix
|
||||||
|
|
||||||
|
Add a new flat-config block at the end of `eslint.config.mjs` (after
|
||||||
|
the existing `globalIgnores(...)` call, NOT inside the root rules
|
||||||
|
block) targeting only `scripts/**/*.js`:
|
||||||
|
|
||||||
|
```js
|
||||||
|
{
|
||||||
|
files: ['scripts/**/*.js'],
|
||||||
|
rules: {
|
||||||
|
'no-restricted-syntax': ['error', {
|
||||||
|
selector: 'CallExpression[callee.name="require"]',
|
||||||
|
message: 'Use ESM `import` syntax. `package.json` has "type": "module"; require() throws ReferenceError at runtime. See .convoys/fix-reset-db-script.md.',
|
||||||
|
}],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
```
|
||||||
|
|
||||||
|
The error message points at `.convoys/fix-reset-db-script.md` so that
|
||||||
|
the next agent / contributor who triggers the rule gets a 1-click path
|
||||||
|
to the exemplar fix (ESM top-level imports for `dotenv`, `neon`,
|
||||||
|
`bcrypt`) instead of having to re-derive it.
|
||||||
|
|
||||||
|
`scripts/migrations/**` is already in `globalIgnores` (from
|
||||||
|
`pick-a-name` Brief 2's migration script) and stays ignored — the
|
||||||
|
rule does not fire there even though the migration script is ESM and
|
||||||
|
correctly uses `import` (no need to re-lint files already excluded).
|
||||||
|
|
||||||
|
The rule fires on `CallExpression[callee.name="require"]` — the AST
|
||||||
|
shape of a plain `require('foo')` call. It does NOT fire on
|
||||||
|
`createRequire(import.meta.url)` patterns (which use `Module.createRequire`)
|
||||||
|
should one ever be needed; the AST callee is `createRequire`, not
|
||||||
|
`require`. If a future helper script legitimately needs CJS interop,
|
||||||
|
the right path is `await import('foo')` (ESM dynamic import) — the
|
||||||
|
rule will not block that either.
|
||||||
|
|
||||||
|
## Verification plan
|
||||||
|
|
||||||
|
1. `node --check eslint.config.mjs` → exit 0 (config parses).
|
||||||
|
2. `npm run lint` → exit 1 with **128 problems (81 errors, 47
|
||||||
|
warnings)** — verbatim match of the pre-convoy baseline (no
|
||||||
|
regression, no new false positives in the current tree).
|
||||||
|
3. **Negative test (apply, run, revert):** prepend
|
||||||
|
`const x = require('fs');` to `scripts/reset-db.js`, run
|
||||||
|
`npm run lint`, confirm exit 1 with the new rule firing at the
|
||||||
|
expected line/column and the documented message, then revert.
|
||||||
|
4. `npm run test:run` → 21/21 pass (no test surface touched; runs
|
||||||
|
only to confirm vitest is still green).
|
||||||
|
5. `rg "require\(" scripts/ --type js` → 0 hits (sanity check
|
||||||
|
confirming the current tree is clean and the rule has zero
|
||||||
|
positives to silence on day 1).
|
||||||
|
|
||||||
|
## Risks
|
||||||
|
|
||||||
|
- **False positives if anyone legitimately needs `require()` in
|
||||||
|
`scripts/**`.** None today (verified by step 5 — zero `require(`
|
||||||
|
hits across all helper scripts in the current tree after PR #25
|
||||||
|
and the `drop-public-setup` B2 sweep). If a future script
|
||||||
|
legitimately needs CJS interop (e.g. a dependency that only
|
||||||
|
exports CJS without an ESM wrapper), the fix is `await
|
||||||
|
import('foo')` — ESM dynamic import works in any ESM script and
|
||||||
|
is not flagged by the rule. If that's somehow not viable, the
|
||||||
|
escape hatch is a per-line `// eslint-disable-next-line
|
||||||
|
no-restricted-syntax` with a comment explaining why ESM doesn't
|
||||||
|
work; lint baseline tracking will catch the disable directive in
|
||||||
|
review.
|
||||||
|
- **Rule scope drift.** If someone adds a new top-level scripts
|
||||||
|
directory (`tools/`, `cli/`, etc.) the rule won't fire there. Low
|
||||||
|
risk — this repo has consolidated on `scripts/` since inception
|
||||||
|
and there's no signal of a second scripts directory being added.
|
||||||
|
Tracked here so the next refactor that reshapes the helper-script
|
||||||
|
layout knows to extend the `files:` glob.
|
||||||
|
- **ESLint v10 bump.** When `bump-eslint-10` lands (currently
|
||||||
|
upstream-blocked per Gotcha #10), re-verify this rule's selector
|
||||||
|
syntax against the v10 AST behavior. `no-restricted-syntax` is a
|
||||||
|
stable core rule going back to ESLint v1; no v10 deprecation is
|
||||||
|
expected, but the smoke check is cheap.
|
||||||
|
|
||||||
|
## Acceptance criteria
|
||||||
|
|
||||||
|
- `node --check eslint.config.mjs` exit 0
|
||||||
|
- `npm run lint` exit 1 with 128 problems (baseline preserved)
|
||||||
|
- `npm run test:run` 21/21 pass
|
||||||
|
- Negative test passes (rule fires on synthetic `require()` insertion,
|
||||||
|
reverts cleanly to 128 problems after the synthetic edit is
|
||||||
|
removed)
|
||||||
|
- Grep: 0 `require(` occurrences in `scripts/**/*.js` (current tree
|
||||||
|
is clean — rule starts with zero positives to silence)
|
||||||
|
|
||||||
|
## Out of scope
|
||||||
|
|
||||||
|
- Sweeping any other `scripts/**` file — current tree is clean
|
||||||
|
(verified by the grep step above). The rule is preventative,
|
||||||
|
not retroactive.
|
||||||
|
- Broadening the rule to all `.js` files at repo root — see
|
||||||
|
§ Design decision; `pages/api/**` is already correctly ESM and
|
||||||
|
the config files (`postcss.config.js`, `tailwind.config.js`,
|
||||||
|
`next.config.js`) intentionally use CJS-style exports that the
|
||||||
|
next-config base rules handle correctly.
|
||||||
|
- Bumping any deps (ESLint stays at v9 per Gotcha #10; no
|
||||||
|
`typescript-eslint` interaction since the rule is a core rule).
|
||||||
|
- The `purge-weak-creds-from-helpers` follow-up (the
|
||||||
|
`scripts/create-test-users.js` portion remains queued; this
|
||||||
|
convoy only adds the lint rule, not the weak-creds sweep).
|
||||||
|
|
||||||
|
## Owns
|
||||||
|
|
||||||
|
Parent (single-file ESLint config edit; no architect or implementer
|
||||||
|
subagent required — proven-pattern follow-up to PR #25).
|
||||||
|
|
||||||
|
## As-shipped
|
||||||
|
|
||||||
|
_(Stub — doc-writer to fill in post-merge with squash commit SHA,
|
||||||
|
PR URL, observed lint baseline before/after, CI gate results, and
|
||||||
|
any deviations from the planned shape.)_
|
||||||
187
.convoys/purge-weak-creds-from-helpers.md
Normal file
187
.convoys/purge-weak-creds-from-helpers.md
Normal file
|
|
@ -0,0 +1,187 @@
|
||||||
|
# purge-weak-creds-from-helpers (P2 hygiene — final scope close)
|
||||||
|
|
||||||
|
**Status:** IN-FLIGHT 2026-05-26
|
||||||
|
**Priority:** P2 hygiene (not a security blocker; the test-fixture
|
||||||
|
script is dev-only and the documented passwords were never reachable
|
||||||
|
from a production code path — but the bug pattern is the same as the
|
||||||
|
P0-grade weak-creds shape that `drop-public-setup` removed from
|
||||||
|
`setup-neon-db.js`, so closing it brings the helper-script surface to
|
||||||
|
zero weak literals)
|
||||||
|
**Convoy owner:** parent (no architect — proven-pattern mirror;
|
||||||
|
single-script + single-doc fix following two already-shipped
|
||||||
|
applications of the same pattern)
|
||||||
|
**Opened:** 2026-05-26
|
||||||
|
**Classification:** hygiene
|
||||||
|
|
||||||
|
## Background — the multi-convoy history that led here
|
||||||
|
|
||||||
|
The original `purge-weak-creds-from-helpers` convoy was queued in
|
||||||
|
`.convoys/ship-readiness.md` as the umbrella for sweeping every
|
||||||
|
helper-script + manual-QA-doc reference to the legacy `admin123` /
|
||||||
|
`alice123` / `bob123` literals and the legacy `@tcgvault.com` email
|
||||||
|
domain. Its scope has been progressively whittled down by three
|
||||||
|
already-shipped convoys:
|
||||||
|
|
||||||
|
1. **`drop-public-setup`** (squash `ff80753` Brief 1 + `b63b509`
|
||||||
|
Brief 2): replaced the hardcoded `admin123` in
|
||||||
|
`scripts/setup-neon-db.js` with the fail-loud
|
||||||
|
`ADMIN_INITIAL_PASSWORD` env-var gate; converted the script from
|
||||||
|
CJS to ESM so `npm run setup-db` actually runs on Node 22.x. Set
|
||||||
|
the precedent for the env-var + fail-loud + no-echo pattern that
|
||||||
|
the next two convoys (and this one) mirror verbatim.
|
||||||
|
2. **`pick-a-name` Brief 2** (squash `9abbab6`, 2026-05-24): swept
|
||||||
|
every `@tcgvault.com` literal in scripts + docs to `@deckhearth.com`
|
||||||
|
together with the one-shot migration script. **Email half done.**
|
||||||
|
3. **`fix-reset-db-script`** (squash `3ab9bf8`, PR #25, 2026-05-26):
|
||||||
|
second application of the post-`drop-public-setup` pattern, this
|
||||||
|
time to `scripts/reset-db.js`. Removed the second `admin123` literal
|
||||||
|
from the codebase, removed the only remaining `Admin Password:`
|
||||||
|
echo, converted the third CJS-in-ESM script.
|
||||||
|
|
||||||
|
After those three convoys, the remaining weak-credential surface is
|
||||||
|
exactly two files — the alice/bob test-user fixture script and the
|
||||||
|
manual-QA doc that pairs with it. Both are addressed here.
|
||||||
|
|
||||||
|
## Remaining scope (this convoy)
|
||||||
|
|
||||||
|
1. **`scripts/create-test-users.js`** — alice + bob fixtures still
|
||||||
|
hardcode `bcrypt.hash('alice123', 12)` + `bcrypt.hash('bob123', 12)`
|
||||||
|
and echo the literal passwords to stdout (`console.log('✅ Created
|
||||||
|
Alice (alice@deckhearth.com / alice123)')`).
|
||||||
|
2. **`TESTING_GUIDE.md`** — Test Accounts table still documents the
|
||||||
|
literal passwords for admin + alice + bob.
|
||||||
|
|
||||||
|
## The fix shape — single env var, no echo, ESM-already
|
||||||
|
|
||||||
|
The fix is a verbatim mirror of the post-`drop-public-setup`
|
||||||
|
`scripts/setup-neon-db.js` pattern and the post-`fix-reset-db-script`
|
||||||
|
`scripts/reset-db.js` pattern, with one deliberate simplification:
|
||||||
|
|
||||||
|
- **Single env var: `TEST_USERS_PASSWORD`.** Both alice and bob get
|
||||||
|
the same hashed value. Per-user env vars (`ALICE_PASSWORD`,
|
||||||
|
`BOB_PASSWORD`) would be unnecessary sprawl for what is a test
|
||||||
|
fixture surface — these aren't independent identities, they're a
|
||||||
|
collaborator-flow demo pair. Risk R2 below argues this explicitly.
|
||||||
|
- **Fail-loud at the top of `createTestUsers()`.** Reads
|
||||||
|
`process.env.TEST_USERS_PASSWORD`; if unset or whitespace-only,
|
||||||
|
prints an actionable error (names the var, points at `.env.local`,
|
||||||
|
suggests `openssl rand -base64 24`, references README's "First-time
|
||||||
|
admin setup" section) and `process.exit(1)` BEFORE opening any DB
|
||||||
|
connection. Same wording template as `setup-neon-db.js` lines 20-26
|
||||||
|
and `reset-db.js` lines 29-35.
|
||||||
|
- **No password echo to stdout.** The previous file logged the
|
||||||
|
literal `alice123` / `bob123` strings in both the per-user creation
|
||||||
|
line and the final summary block. All four echo lines are deleted;
|
||||||
|
the new summary line documents *where* the password comes from
|
||||||
|
(`(passwords from TEST_USERS_PASSWORD)`) without ever printing the
|
||||||
|
value.
|
||||||
|
- **ESM already.** Unlike `setup-neon-db.js` and `reset-db.js` at the
|
||||||
|
start of their respective convoys, `create-test-users.js` was
|
||||||
|
already top-level ESM (it imports `{ config } from 'dotenv'`,
|
||||||
|
`{ sql } from '@vercel/postgres'`, `bcrypt from 'bcryptjs'` at the
|
||||||
|
top of the file). **No CJS→ESM conversion needed.** This convoy is
|
||||||
|
the first of the three to skip that half of the pattern.
|
||||||
|
- **`ON CONFLICT (email) DO NOTHING`** is preserved (already in the
|
||||||
|
original file at lines 19, 28) — defensive against double-run.
|
||||||
|
|
||||||
|
`TESTING_GUIDE.md`'s Test Accounts table is rewritten to (a) remove
|
||||||
|
the literal passwords from the table, (b) document the env-var source
|
||||||
|
for each user, and (c) point at README's "First-time admin setup"
|
||||||
|
section for the `openssl rand -base64 24` generation tip. The two
|
||||||
|
inline `Password: alice123` / `Password: bob123` snippets later in the
|
||||||
|
workflow are replaced with `Password: <value of TEST_USERS_PASSWORD
|
||||||
|
from .env.local>`.
|
||||||
|
|
||||||
|
## Verification plan (static-grep only — script is destructive)
|
||||||
|
|
||||||
|
This convoy does NOT live-test `create-test-users.js`. The script
|
||||||
|
opens a DB connection and inserts rows; running it against a Neon
|
||||||
|
branch in CI or in the boot-the-brief loop would be a side-effect
|
||||||
|
cost we don't need to incur. The verification surface is entirely
|
||||||
|
static:
|
||||||
|
|
||||||
|
- `node --check scripts/create-test-users.js` → exit 0
|
||||||
|
- `npm run lint` → 128 problems (baseline preserved; no regression)
|
||||||
|
- `npm run test:run` → 21/21 pass
|
||||||
|
- Grep `scripts/ TESTING_GUIDE.md` for `admin123|password123|test123|
|
||||||
|
alice123|bob123` → 0 hits on literal passwords
|
||||||
|
- Grep `scripts/create-test-users.js` for `require(` → 0 hits
|
||||||
|
(mirror-the-pattern preserves ESM-only; trivially satisfied here
|
||||||
|
because the file was already ESM)
|
||||||
|
- Grep `scripts/ TESTING_GUIDE.md` for `TEST_USERS_PASSWORD` →
|
||||||
|
expect 10 hits (5 in script: docstring + const + error message
|
||||||
|
body; 5 in TESTING_GUIDE.md: table + two inline workflow snippets +
|
||||||
|
explanatory paragraph)
|
||||||
|
|
||||||
|
**Live verification deferred to operator.** Optional post-merge
|
||||||
|
action: set `TEST_USERS_PASSWORD` in `.env.local`, run
|
||||||
|
`node scripts/create-test-users.js` against a non-prod Neon branch,
|
||||||
|
verify alice + bob rows insert; then unset the env var and re-run,
|
||||||
|
verify the script exits 1 with the helpful error message before
|
||||||
|
opening the DB connection.
|
||||||
|
|
||||||
|
## Risks
|
||||||
|
|
||||||
|
- **R1 — A CI step or doc dep on the literal passwords.** If
|
||||||
|
`.github/workflows/**` or any other doc (`docs/**`, `TESTING_GUIDE.md`
|
||||||
|
sibling files, the agent-context-pipeline docs) references
|
||||||
|
`alice123` / `bob123` / `admin123` as part of an automated test
|
||||||
|
flow, removing the literal would break it. **Mitigation:** the grep
|
||||||
|
hunt covered `scripts/` + `TESTING_GUIDE.md`. The broader hits in
|
||||||
|
`.convoys/**` and `AGENTS.md` are historical convoy narrative and
|
||||||
|
must NOT be edited (rewriting history). The only live reference
|
||||||
|
outside this convoy's scope is `pages/login.js` — see "Surfaced
|
||||||
|
out-of-scope follow-up" below.
|
||||||
|
- **R2 — Env-var sprawl.** Using a single `TEST_USERS_PASSWORD` for
|
||||||
|
both alice and bob is intentional. These are test-fixture users
|
||||||
|
for the collaboration demo flow in TESTING_GUIDE.md; they aren't
|
||||||
|
modeled as independent identities anywhere in the auth surface, and
|
||||||
|
giving them per-user passwords would (a) double the env-var
|
||||||
|
contract for zero security benefit (anyone running this script
|
||||||
|
already has full DB access) and (b) drift from the
|
||||||
|
ADMIN_INITIAL_PASSWORD shape that the operator is already trained
|
||||||
|
on. If future test-user additions need distinct passwords for
|
||||||
|
realistic concurrency testing, that's a separate concern and a
|
||||||
|
separate convoy.
|
||||||
|
|
||||||
|
## Surfaced out-of-scope follow-up
|
||||||
|
|
||||||
|
- **`pages/login.js` "Quick Login" buttons still hardcode the legacy
|
||||||
|
literals.** Lines 172 + 184 invoke
|
||||||
|
`handleQuickLogin('alice@deckhearth.com', 'alice123')` and
|
||||||
|
`handleQuickLogin('bob@deckhearth.com', 'bob123')`. These are
|
||||||
|
client-side dev convenience buttons that ship to production HTML
|
||||||
|
and reveal the legacy passwords directly to anyone viewing the
|
||||||
|
login page source. **NOT in scope for this convoy** (the convoy
|
||||||
|
spec is "scripts + docs only; do NOT touch `pages/**`"). Queue a
|
||||||
|
follow-up convoy: `purge-quick-login-from-loginpage` (P2 hygiene)
|
||||||
|
to either (a) delete the Quick Login section entirely or
|
||||||
|
(b) gate it behind `process.env.NODE_ENV === 'development'`. The
|
||||||
|
latter still requires a credential source that doesn't ship to
|
||||||
|
prod HTML — likely a `.env.local`-only `NEXT_PUBLIC_DEV_*`
|
||||||
|
convention or a dev-only proxy endpoint. Architect-worth.
|
||||||
|
|
||||||
|
## Operator action required
|
||||||
|
|
||||||
|
- **Pre-merge:** none. No schema change. No new infra.
|
||||||
|
- **Post-merge:** anyone running `node scripts/create-test-users.js`
|
||||||
|
(or `npm run create-test-users` if such a script exists) must add
|
||||||
|
`TEST_USERS_PASSWORD=<value>` to their `.env.local` first.
|
||||||
|
Existing alice + bob rows in already-seeded environments are
|
||||||
|
**not** rotated by re-running this script — `ON CONFLICT (email)
|
||||||
|
DO NOTHING` preserves the old hashes. Any environment that ran
|
||||||
|
`create-test-users.js` before this convoy still has the weak
|
||||||
|
`alice123` / `bob123` hashes in its DB; operators must rotate
|
||||||
|
manually via the app (or drop those rows and re-seed). Same
|
||||||
|
caveat that applies to the `drop-public-setup` admin row —
|
||||||
|
`setup-neon-db.js`'s and `create-test-users.js`'s idempotency
|
||||||
|
means they do NOT rotate; they only seed.
|
||||||
|
|
||||||
|
## Owns
|
||||||
|
|
||||||
|
Parent (single-script + single-doc proven-pattern fix; no architect
|
||||||
|
or implementer subagent required).
|
||||||
|
|
||||||
|
## As-shipped
|
||||||
|
|
||||||
|
(stub — fill in post-merge)
|
||||||
317
.convoys/single-auth-provider.md
Normal file
317
.convoys/single-auth-provider.md
Normal file
|
|
@ -0,0 +1,317 @@
|
||||||
|
# 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
|
||||||
|
|
||||||
|
_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).
|
||||||
434
.convoys/single-sql-client.md
Normal file
434
.convoys/single-sql-client.md
Normal file
|
|
@ -0,0 +1,434 @@
|
||||||
|
---
|
||||||
|
name: single-sql-client
|
||||||
|
classification: quality
|
||||||
|
success_metric: |
|
||||||
|
`lib/database.js` is deleted; every former caller uses
|
||||||
|
`@vercel/postgres` tagged templates with identical query
|
||||||
|
semantics; vitest 21/21 stays green; lint baseline (128
|
||||||
|
problems) is preserved; no remaining `lib/database` import
|
||||||
|
appears anywhere under `pages/` or `lib/`. AGENTS.md Gotcha #1
|
||||||
|
/ `.convoys/ship-readiness.md` P1 #8 flip → RESOLVED.
|
||||||
|
skip:
|
||||||
|
- role-design-system-auditor
|
||||||
|
- role-a11y-auditor
|
||||||
|
- role-ux-reviewer
|
||||||
|
- role-ia-architect
|
||||||
|
status: open
|
||||||
|
created: 2026-05-26
|
||||||
|
parent: ship-readiness
|
||||||
|
addresses: P1 #8 (launch sequence step 8) — "Two SQL clients in parallel"
|
||||||
|
depends_on: []
|
||||||
|
---
|
||||||
|
|
||||||
|
# Convoy: single-sql-client
|
||||||
|
|
||||||
|
Collapse the dual SQL-client problem documented in AGENTS.md
|
||||||
|
Gotcha #1 by deleting `lib/database.js` and migrating its sole
|
||||||
|
production caller (`pages/api/auth-utils.js`) onto the canonical
|
||||||
|
`@vercel/postgres` tagged-template surface. Keep
|
||||||
|
`@neondatabase/serverless` as a runtime dependency for the
|
||||||
|
out-of-scope `scripts/**` helpers that already use `neon()`
|
||||||
|
directly.
|
||||||
|
|
||||||
|
## Background
|
||||||
|
|
||||||
|
`AGENTS.md` Gotcha #1 (and the `.cursor/rules/db-and-schema.mdc`
|
||||||
|
"Clients" section) document a long-standing dual-client problem:
|
||||||
|
|
||||||
|
- `@neondatabase/serverless` is wrapped by `lib/database.js`'s
|
||||||
|
`DatabaseAdapter`, which exposes a `db.query(sqlString,
|
||||||
|
params)` API that **manually interpolates `$1, $2, ...`
|
||||||
|
placeholders into the SQL string** and then calls
|
||||||
|
`sql.unsafe(query)` on the result.
|
||||||
|
- `@vercel/postgres` is used directly by ~30 `pages/api/**`
|
||||||
|
handlers via tagged-template SQL (`` await sql`SELECT … WHERE
|
||||||
|
id = ${id}` ``), which is parameterized natively by the driver.
|
||||||
|
|
||||||
|
The manual-interpolation shape in `lib/database.js` is the
|
||||||
|
documented foot-gun:
|
||||||
|
|
||||||
|
> *Avoid `lib/database.js`'s `db.query(string, params)` API for
|
||||||
|
> new code — it interpolates params into a string and then
|
||||||
|
> calls `sql.unsafe()`, which is a SQL-injection vector.*
|
||||||
|
> — `.cursor/rules/db-and-schema.mdc` § Clients
|
||||||
|
|
||||||
|
The `.convoys/ship-readiness.md` P1 #8 entry tracks the cleanup
|
||||||
|
as launch sequence step 8 ("touches ~3 files based on graph").
|
||||||
|
|
||||||
|
## Decisions
|
||||||
|
|
||||||
|
### D1 — Caller inventory: 2 files in scope (1 source + 1 test), not "~3"
|
||||||
|
|
||||||
|
Architect ran `Grep` against the workspace for `lib/database`
|
||||||
|
imports. Only **2** files touch the module:
|
||||||
|
|
||||||
|
| File | Kind | What it does with `db` |
|
||||||
|
| --- | --- | --- |
|
||||||
|
| `pages/api/auth-utils.js` | source | imports `db`, calls `db.query` in `isAdmin(userId)` + `getUserById(userId)` |
|
||||||
|
| `test/api/auth-utils.test.js` | test | `vi.mock('../../lib/database.js', () => ({ db: { query: vi.fn() } }))` — mock-only; the 5 tests exercise `generateToken` / `verifyToken`, not `isAdmin` / `getUserById`. The mock exists purely to satisfy the import graph |
|
||||||
|
|
||||||
|
The README/AGENTS-quoted "~3 files based on graph" estimate from
|
||||||
|
ship-readiness P1 #8 was loose. Confirmed exact count = 2. No
|
||||||
|
deviation flag needed; the convoy is even smaller than estimated.
|
||||||
|
|
||||||
|
### D2 — `@vercel/postgres` tagged templates, byte-equivalent SQL
|
||||||
|
|
||||||
|
Two call sites in `pages/api/auth-utils.js`:
|
||||||
|
|
||||||
|
**Before:**
|
||||||
|
|
||||||
|
```js
|
||||||
|
import { db } from '../../lib/database.js';
|
||||||
|
|
||||||
|
export async function isAdmin(userId) {
|
||||||
|
try {
|
||||||
|
const result = await db.query(`
|
||||||
|
SELECT role FROM users WHERE id = $1
|
||||||
|
`, [userId]);
|
||||||
|
return result.rows[0]?.role === 'admin';
|
||||||
|
} catch (error) { ... }
|
||||||
|
}
|
||||||
|
|
||||||
|
export async function getUserById(userId) {
|
||||||
|
try {
|
||||||
|
const result = await db.query(`
|
||||||
|
SELECT id, email, role, created_at FROM users WHERE id = $1
|
||||||
|
`, [userId]);
|
||||||
|
return result.rows[0];
|
||||||
|
} catch (error) { ... }
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
**After:**
|
||||||
|
|
||||||
|
```js
|
||||||
|
import { sql } from '@vercel/postgres';
|
||||||
|
|
||||||
|
export async function isAdmin(userId) {
|
||||||
|
try {
|
||||||
|
const result = await sql`
|
||||||
|
SELECT role FROM users WHERE id = ${userId}
|
||||||
|
`;
|
||||||
|
return result.rows[0]?.role === 'admin';
|
||||||
|
} catch (error) { ... }
|
||||||
|
}
|
||||||
|
|
||||||
|
export async function getUserById(userId) {
|
||||||
|
try {
|
||||||
|
const result = await sql`
|
||||||
|
SELECT id, email, role, created_at FROM users WHERE id = ${userId}
|
||||||
|
`;
|
||||||
|
return result.rows[0];
|
||||||
|
} catch (error) { ... }
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
The two queries are SELECT-only, single-table, single-parameter
|
||||||
|
(numeric `userId`). The translated tagged-template form produces
|
||||||
|
**byte-equivalent SQL** with proper parameter binding (instead
|
||||||
|
of string interpolation + `sql.unsafe`). The `result.rows[0]`
|
||||||
|
access pattern works identically — `@vercel/postgres` returns
|
||||||
|
`{ rows, rowCount }` natively, which matches the shape
|
||||||
|
`DatabaseAdapter.query` was already returning. No
|
||||||
|
result-handling change needed at the call sites in `isAdmin` /
|
||||||
|
`getUserById`. No transaction / pool semantics change either:
|
||||||
|
neither `lib/database.js` nor `@vercel/postgres` uses a
|
||||||
|
long-lived pool from these call sites (both create a per-request
|
||||||
|
HTTP connection against Neon's serverless endpoint).
|
||||||
|
|
||||||
|
### D3 — Keep `@neondatabase/serverless` as a dep; SCOPE = `lib/database.js` collapse only
|
||||||
|
|
||||||
|
The convoy seed flagged this explicitly. Architect verified:
|
||||||
|
|
||||||
|
- `@neondatabase/serverless` is imported by **11 files** outside
|
||||||
|
`lib/database.js`:
|
||||||
|
- `scripts/setup-neon-db.js` (the DDL bootstrap)
|
||||||
|
- `scripts/migrations/2026-05-24-rename-admin-email.js` (post-`pick-a-name` admin rename)
|
||||||
|
- `scripts/reset-db.js` (dev reset)
|
||||||
|
- 8 other one-off `scripts/add-*.js` / `scripts/fix-*.js` / `scripts/seed-*.js` historical jobs
|
||||||
|
- None of those go through `lib/database.js`; they all use
|
||||||
|
`neon(POSTGRES_URL)` directly with tagged-template SQL (which
|
||||||
|
is the safe shape that `lib/database.js`'s wrapper subverts).
|
||||||
|
|
||||||
|
**Scope decision: keep `@neondatabase/serverless` in
|
||||||
|
`package.json`.** This convoy is about deleting the
|
||||||
|
`lib/database.js` abstraction, not about eliminating
|
||||||
|
`@neondatabase/serverless` from the dependency tree. The
|
||||||
|
no-go-zones rule explicitly forbids touching
|
||||||
|
`scripts/setup-neon-db.js`, `scripts/migrations/*`, and the
|
||||||
|
`scripts/add-*` / `scripts/fix-*` / `scripts/seed-*` historical
|
||||||
|
jobs. Migrating those to `@vercel/postgres` would also be the
|
||||||
|
wrong call architecturally — `@vercel/postgres` is tuned for
|
||||||
|
Vercel's edge / serverless runtime; one-off scripts run from a
|
||||||
|
developer laptop or CI runner where `@neondatabase/serverless`'s
|
||||||
|
direct `neon()` shape is more appropriate.
|
||||||
|
|
||||||
|
A future convoy could (a) migrate the scripts to a uniform
|
||||||
|
client OR (b) extract a thin `scripts/lib/db.js` that wraps
|
||||||
|
`neon()` once. Either is its own scope; flagged in § Follow-ups.
|
||||||
|
|
||||||
|
### D4 — `sql.unsafe` audit: NOT a real injection vector with current callers (security finding: NO)
|
||||||
|
|
||||||
|
The `lib/database.js` shape is **unsafe-by-default**: it
|
||||||
|
interpolates raw values into a SQL string and calls
|
||||||
|
`sql.unsafe()`. In theory, that's a SQL-injection vector for
|
||||||
|
any caller that passes user-controlled string input.
|
||||||
|
|
||||||
|
Architect audit of the **2 current call sites**:
|
||||||
|
|
||||||
|
- `isAdmin(userId)` — `userId` is sourced from a verified JWT
|
||||||
|
payload (`decoded.userId` after `verifyToken(token)` succeeds
|
||||||
|
in `pages/api/admin/index.js`). It's a number from
|
||||||
|
`users.id` (SERIAL). The `lib/database.js` interpolation path
|
||||||
|
for numbers is `result = await sql\`${sql.unsafe(query)}\``
|
||||||
|
with the number inlined raw — for a numeric SERIAL id, no
|
||||||
|
injection vector exists in practice.
|
||||||
|
- `getUserById(userId)` — not currently called by any handler
|
||||||
|
(`Grep` for `getUserById` returns only the definition site +
|
||||||
|
the public-API comment in AGENTS.md). Same `userId` shape
|
||||||
|
semantics apply if it were called.
|
||||||
|
|
||||||
|
**Conclusion: NO real security finding.** This is a pure
|
||||||
|
refactor + foot-gun-removal convoy. The next convoy that adds a
|
||||||
|
caller passing user-controlled string input to `db.query` would
|
||||||
|
have been the security incident; deleting the unsafe surface
|
||||||
|
prevents that future incident.
|
||||||
|
|
||||||
|
If a future audit surfaces a `lib/database.js`-shaped wrapper
|
||||||
|
re-introduced in another lib (e.g. `lib/db-helper.js`), this
|
||||||
|
convoy's lesson is: kill it on sight. See § Follow-ups for a
|
||||||
|
queued ESLint rule that would catch a `sql.unsafe` re-introduction.
|
||||||
|
|
||||||
|
### D5 — Tests: 1 test file touched, no semantic change
|
||||||
|
|
||||||
|
`test/api/auth-utils.test.js` already mocks `lib/database.js`
|
||||||
|
purely to satisfy the import graph; the 5 tests exercise
|
||||||
|
`generateToken` + `verifyToken`, neither of which touches the
|
||||||
|
DB. **Post-migration:** the mock is no longer needed because
|
||||||
|
`auth-utils.js` no longer imports from `lib/database.js`. Drop
|
||||||
|
the `vi.mock('../../lib/database.js', ...)` call and the now-unused
|
||||||
|
`vi` import. Test count + assertions unchanged: 5/5.
|
||||||
|
|
||||||
|
The other 16 vitest tests (`lib/auth-secret.test.js`,
|
||||||
|
`lib/permission-middleware.test.js`,
|
||||||
|
`components/Layout.test.js`) don't touch `lib/database.js`. No
|
||||||
|
mock drift risk; nothing else to update.
|
||||||
|
|
||||||
|
`@vercel/postgres` does NOT get a new mock in this convoy
|
||||||
|
(deferred to queued `fill-vitest-handler-coverage`); the
|
||||||
|
`isAdmin` / `getUserById` functions are still untested at the
|
||||||
|
unit level, same as pre-migration. The migration is purely a
|
||||||
|
client swap, not a coverage expansion.
|
||||||
|
|
||||||
|
## Caller inventory (verbatim before/after)
|
||||||
|
|
||||||
|
### `pages/api/auth-utils.js` (only source caller)
|
||||||
|
|
||||||
|
**Imports (before):**
|
||||||
|
```js
|
||||||
|
import jwt from 'jsonwebtoken';
|
||||||
|
import { db } from '../../lib/database.js';
|
||||||
|
import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js';
|
||||||
|
```
|
||||||
|
|
||||||
|
**Imports (after):**
|
||||||
|
```js
|
||||||
|
import jwt from 'jsonwebtoken';
|
||||||
|
import { sql } from '@vercel/postgres';
|
||||||
|
import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js';
|
||||||
|
```
|
||||||
|
|
||||||
|
**`isAdmin` body (before):**
|
||||||
|
```js
|
||||||
|
const result = await db.query(`
|
||||||
|
SELECT role FROM users WHERE id = $1
|
||||||
|
`, [userId]);
|
||||||
|
```
|
||||||
|
|
||||||
|
**`isAdmin` body (after):**
|
||||||
|
```js
|
||||||
|
const result = await sql`
|
||||||
|
SELECT role FROM users WHERE id = ${userId}
|
||||||
|
`;
|
||||||
|
```
|
||||||
|
|
||||||
|
**`getUserById` body (before):**
|
||||||
|
```js
|
||||||
|
const result = await db.query(`
|
||||||
|
SELECT id, email, role, created_at FROM users WHERE id = $1
|
||||||
|
`, [userId]);
|
||||||
|
```
|
||||||
|
|
||||||
|
**`getUserById` body (after):**
|
||||||
|
```js
|
||||||
|
const result = await sql`
|
||||||
|
SELECT id, email, role, created_at FROM users WHERE id = ${userId}
|
||||||
|
`;
|
||||||
|
```
|
||||||
|
|
||||||
|
The `try/catch` shape, the `result.rows[0]` access, the
|
||||||
|
`?.role === 'admin'` check, and the `console.error` + `return
|
||||||
|
false`/`return null` error paths are all preserved verbatim.
|
||||||
|
|
||||||
|
### `test/api/auth-utils.test.js` (mock cleanup)
|
||||||
|
|
||||||
|
**Imports (before):**
|
||||||
|
```js
|
||||||
|
import { describe, expect, it, vi } from 'vitest';
|
||||||
|
import jwt from 'jsonwebtoken';
|
||||||
|
|
||||||
|
vi.mock('../../lib/database.js', () => ({
|
||||||
|
db: { query: vi.fn() },
|
||||||
|
}));
|
||||||
|
|
||||||
|
import { JWT_SECRET } from '../../lib/auth-secret.js';
|
||||||
|
import { generateToken, verifyToken } from '../../pages/api/auth-utils.js';
|
||||||
|
```
|
||||||
|
|
||||||
|
**Imports (after):**
|
||||||
|
```js
|
||||||
|
import { describe, expect, it } from 'vitest';
|
||||||
|
import jwt from 'jsonwebtoken';
|
||||||
|
|
||||||
|
import { JWT_SECRET } from '../../lib/auth-secret.js';
|
||||||
|
import { generateToken, verifyToken } from '../../pages/api/auth-utils.js';
|
||||||
|
```
|
||||||
|
|
||||||
|
The 5 test bodies (2 `generateToken` + 3 `verifyToken`) are
|
||||||
|
unchanged.
|
||||||
|
|
||||||
|
### `lib/database.js` (deleted)
|
||||||
|
|
||||||
|
47-line file. No replacement; the abstraction is gone. The two
|
||||||
|
callers go straight to `@vercel/postgres` tagged-template SQL,
|
||||||
|
matching the canonical pattern used by ~30 other `pages/api/**`
|
||||||
|
handlers already in the tree.
|
||||||
|
|
||||||
|
## The fix (per-file diff shape)
|
||||||
|
|
||||||
|
| File | Change | Lines |
|
||||||
|
| --- | --- | --- |
|
||||||
|
| `pages/api/auth-utils.js` | swap `db.query(string, params)` → `sql\`…${userId}\`` in 2 functions; swap import | +5 / -7 |
|
||||||
|
| `test/api/auth-utils.test.js` | drop `vi.mock('../../lib/database.js', ...)` + unused `vi` import | +1 / -5 |
|
||||||
|
| `lib/database.js` | DELETE | 0 / -47 |
|
||||||
|
| `.convoys/single-sql-client.md` | NEW (this file) | +330 / 0 |
|
||||||
|
|
||||||
|
Total: 3 modified files (1 source + 1 test + 1 deletion) + 1
|
||||||
|
new convoy doc.
|
||||||
|
|
||||||
|
## Verification plan
|
||||||
|
|
||||||
|
1. `npm run lint` → 128 problems (baseline preserved, no regression)
|
||||||
|
2. `npm run test:run` → 21/21 pass
|
||||||
|
3. `Grep "lib/database" --type js -l` → 0 hits anywhere
|
||||||
|
(no `pages/**`, no `lib/**`, no `scripts/**`, no `test/**`)
|
||||||
|
4. `Grep "@neondatabase/serverless" --type js -l` → still matches
|
||||||
|
`scripts/setup-neon-db.js`, `scripts/migrations/2026-05-24-rename-admin-email.js`,
|
||||||
|
`scripts/reset-db.js`, and the 8 other `scripts/add-*` / `fix-*` /
|
||||||
|
`seed-*` historical helpers (all out of scope per § D3)
|
||||||
|
5. `node --check pages/api/auth-utils.js` → exit 0 (syntax)
|
||||||
|
|
||||||
|
**Live runtime smoke deferred.** The two migrated functions
|
||||||
|
(`isAdmin`, `getUserById`) are only reachable via
|
||||||
|
`pages/api/admin/index.js` which requires an admin Bearer token
|
||||||
|
+ a populated `users` table in prod Neon. Running a live curl
|
||||||
|
smoke against a local `npm run dev` would require seeding the
|
||||||
|
admin user with a known password (which `setup-neon-db.js`
|
||||||
|
now requires `ADMIN_INITIAL_PASSWORD` to do — operator-only
|
||||||
|
flow) and minting a token. Out-of-band for a convoy
|
||||||
|
that's a pure client swap; the byte-equivalent SQL semantics
|
||||||
|
(D2) plus the same `try/catch` + `result.rows[0]` access shape
|
||||||
|
gives high confidence that the migration is correct. If the
|
||||||
|
post-merge Vercel preview's admin surface 500s on an admin
|
||||||
|
action, the rollback is a single-commit revert of this convoy.
|
||||||
|
|
||||||
|
## Risks
|
||||||
|
|
||||||
|
- **R1 — Byte-equivalence not guaranteed if `lib/database.js`
|
||||||
|
has hidden behavior.** Architect re-read all 47 lines of
|
||||||
|
`lib/database.js` (it's a small file). The only behavior
|
||||||
|
beyond "interpolate, run SQL, return `{ rows, rowCount }`" is
|
||||||
|
the `escapedParams.map` quoting for string params — and both
|
||||||
|
current callers pass numeric `userId`, so the quoting path
|
||||||
|
isn't exercised. The `raw(sqlString, params)` method is just
|
||||||
|
an alias for `query(sqlString, params)`; no caller invokes
|
||||||
|
`raw` (`Grep "\.raw\(" --type js` → zero hits). **Residual
|
||||||
|
risk: very low.** The migration's byte-equivalent SQL plus
|
||||||
|
identical result shape (`{ rows: [...], rowCount: N }`)
|
||||||
|
closes this risk in practice.
|
||||||
|
|
||||||
|
- **R2 — Missed callers.** Mitigated by the post-delete `Grep`
|
||||||
|
sweep in the verification plan (step 3) — if any file still
|
||||||
|
imports `lib/database`, the file no longer exists and the
|
||||||
|
import will throw at module load, failing CI's lint or test
|
||||||
|
job. The `Grep` sweep also catches `require('../../lib/database')`
|
||||||
|
CJS shape (zero hits across the tree at architect time;
|
||||||
|
expected since the repo is `"type": "module"`).
|
||||||
|
|
||||||
|
- **R3 — A future PR re-introduces `lib/database.js` or
|
||||||
|
another `sql.unsafe`-shaped wrapper.** Mitigated by:
|
||||||
|
documentation (this convoy file + AGENTS.md Gotcha #1
|
||||||
|
doc-writer flip → RESOLVED in a follow-up cleanup pass).
|
||||||
|
Stronger mitigation would be an ESLint
|
||||||
|
`no-restricted-imports` rule against `lib/database` or a
|
||||||
|
`no-restricted-syntax` rule against `sql.unsafe(` — surfaced
|
||||||
|
as a follow-up below (`lint-against-lib-database`).
|
||||||
|
|
||||||
|
## As-shipped
|
||||||
|
|
||||||
|
*Stub for doc-writer.* Populate after merge with:
|
||||||
|
- Squash commit SHA + PR number
|
||||||
|
- Diff stats (file count, line counts)
|
||||||
|
- CI gate outcomes (lint, vitest, smoke, forbidden-* gates)
|
||||||
|
- Any operator-action follow-ups
|
||||||
|
- Cross-validation findings (e.g. smoke continuing to pass)
|
||||||
|
- Updates needed to AGENTS.md Gotcha #1 + `.cursor/rules/db-and-schema.mdc` § Clients + ship-readiness P1 #8 entry
|
||||||
|
|
||||||
|
## Follow-ups
|
||||||
|
|
||||||
|
- **`lint-against-lib-database`** (priority: P3 polish). Add an
|
||||||
|
ESLint `no-restricted-imports` rule against `'../**/lib/database'`
|
||||||
|
(or any path resolving to a `lib/database.js` file), so a future
|
||||||
|
PR that re-introduces the unsafe wrapper fails at lint time
|
||||||
|
rather than at runtime. Pairs naturally with the queued
|
||||||
|
`lint-against-cjs-in-esm-scripts` follow-up (both are
|
||||||
|
static-source guards added to `eslint.config.mjs`). Small
|
||||||
|
surface (one entry in the config); could fold into a
|
||||||
|
`harden-eslint-static-guards` convoy if more such guards
|
||||||
|
accumulate.
|
||||||
|
|
||||||
|
- **`purge-neondatabase-serverless-fully`** (priority: P3 polish;
|
||||||
|
blocked on `migration-tool`). Once the `migration-tool` convoy
|
||||||
|
(P1 #11) lands and replaces the ad-hoc `scripts/add-*` /
|
||||||
|
`scripts/fix-*` / `scripts/seed-*` shape with a real migration
|
||||||
|
framework, revisit whether the remaining 11
|
||||||
|
`@neondatabase/serverless` import sites can be collapsed onto
|
||||||
|
`@vercel/postgres`. Caveat: `@vercel/postgres` is tuned for the
|
||||||
|
Vercel edge runtime and may not be the right choice for
|
||||||
|
developer-laptop / CI-runner scripts; the right answer may be
|
||||||
|
"keep the dep, but route all scripts through a single thin
|
||||||
|
helper" rather than "delete the dep entirely". This is its own
|
||||||
|
scope; do NOT bolt onto this convoy.
|
||||||
|
|
||||||
|
- **`add-neon-return-shape-rule`** (priority: P3 polish; surfaced
|
||||||
|
2026-05-25 in PR #24 + restated in `.convoys/fix-reset-db-script.md`
|
||||||
|
§ Out of scope). Codify the `neon()` (returns `[rows]`) vs
|
||||||
|
`@vercel/postgres` (`{ rows: [...] }`) return-shape difference
|
||||||
|
as a `.cursor/rules/db-and-schema.mdc` callout. **Partially
|
||||||
|
satisfied** by this convoy because the dual-client shape is now
|
||||||
|
collapsed for `pages/api/**` — only the 11 `scripts/**` callers
|
||||||
|
still use `neon()` directly, and they all destructure the
|
||||||
|
array return shape correctly today. The rule would defend
|
||||||
|
future contributors who don't already know the difference; queue
|
||||||
|
if a third such bug surfaces.
|
||||||
|
|
||||||
|
## Owns
|
||||||
|
|
||||||
|
- Architect (this convoy file + decisions D1-D5).
|
||||||
|
- Implementer (mechanical: 2 source-file edits + 1 deletion +
|
||||||
|
verification gates). Possibly the same agent — the
|
||||||
|
implementation surface is small enough that the cost of a
|
||||||
|
fresh implementer subagent boot may exceed the cost of doing
|
||||||
|
the work in the architect's own turn. Documented per
|
||||||
|
`fix-reset-db-script` precedent ("Why no architect" / "Why
|
||||||
|
no implementer" — single-file proven-pattern work).
|
||||||
270
.convoys/tighten-visual-diff-path-filter.md
Normal file
270
.convoys/tighten-visual-diff-path-filter.md
Normal file
|
|
@ -0,0 +1,270 @@
|
||||||
|
# tighten-visual-diff-path-filter (P3 polish — single-file YAML tweak)
|
||||||
|
|
||||||
|
**Status:** OPEN 2026-05-26 (this PR)
|
||||||
|
**Priority:** P3 polish (CI-cost / signal-noise; not a security or
|
||||||
|
correctness issue — `Screenshot diff` already swallows its own
|
||||||
|
"snapshot doesn't exist" failure via `continue-on-error: true`, so
|
||||||
|
the false-trigger is wasted runtime + a slightly chatty checks tab,
|
||||||
|
nothing more)
|
||||||
|
**Convoy owner:** parent (no architect — single-file workflow YAML
|
||||||
|
tweak following published GitHub Actions path-filter semantics)
|
||||||
|
**Opened:** 2026-05-26
|
||||||
|
**Branch:** `convoy/tighten-visual-diff-path-filter`
|
||||||
|
|
||||||
|
## Background
|
||||||
|
|
||||||
|
`.github/workflows/visual-diff.yml` is supposed to fire only on UI
|
||||||
|
changes. Its current `paths:` filter:
|
||||||
|
|
||||||
|
```yaml
|
||||||
|
paths:
|
||||||
|
- 'pages/**'
|
||||||
|
- 'components/**'
|
||||||
|
- 'styles/**'
|
||||||
|
- 'tailwind.config.js'
|
||||||
|
- 'postcss.config.js'
|
||||||
|
```
|
||||||
|
|
||||||
|
But `pages/**` matches `pages/api/**` too, and the `tcg-vault` repo's
|
||||||
|
API surface lives entirely under `pages/api/` (Next.js Pages router).
|
||||||
|
That means **API-only PRs trigger the visual-diff workflow** even
|
||||||
|
though they can't possibly move a single rendered pixel. The
|
||||||
|
empirical false-positives:
|
||||||
|
|
||||||
|
- **PR #19 `cors-tighten`** (squash `da50d78`, 2026-05-24) — touched
|
||||||
|
24 files all under `pages/api/**`. Triggered `Screenshot diff`,
|
||||||
|
documented in `.convoys/ship-readiness.md` § P0 #5 As-shipped
|
||||||
|
metrics: *"Screenshot diff — workflow exited 0 because of
|
||||||
|
`continue-on-error: true`, but the actual visual test failed with
|
||||||
|
the documented 'snapshot doesn't exist' error… Triggered on PR #19
|
||||||
|
despite this being API-only because its `paths:` filter is
|
||||||
|
`pages/**` which matches `pages/api/**` too — minor false-positive
|
||||||
|
queued as `tighten-visual-diff-path-filter`."*
|
||||||
|
- **PR #20 `add-rate-limiting`** (squash `708ef45`, 2026-05-24) —
|
||||||
|
touched 6 routes under `pages/api/` (plus `lib/rate-limit.js` and
|
||||||
|
`pages/admin/card-import.js`). Same false-trigger, same swallow.
|
||||||
|
Documented in `.convoys/ship-readiness.md` § P0 #6 As-shipped
|
||||||
|
metrics.
|
||||||
|
|
||||||
|
Cost per false-trigger: **~55s of CI runtime** (the `wait-for-vercel-preview`
|
||||||
|
step hits its 120s budget against the deployed preview, then
|
||||||
|
Playwright `npm ci` + `npx playwright install` + the `visual` project
|
||||||
|
runs — even though the test ultimately can't compare against a
|
||||||
|
non-existent baseline). The `Screenshot diff` job exits 0 because of
|
||||||
|
`continue-on-error: true` (Decision-4 end state of
|
||||||
|
`adopt-playwright-smoke`, until `seed-visual-baselines-on-linux`
|
||||||
|
lands), but it still posts a "Visual Diff — view run" comment and
|
||||||
|
clutters the PR Checks tab with a green-but-meaningless run.
|
||||||
|
|
||||||
|
## Design decision — negated-glob `!pages/api/**`
|
||||||
|
|
||||||
|
GitHub Actions evaluates `paths:` with [minimatch](https://github.com/isaacs/minimatch)
|
||||||
|
and supports `!`-prefixed exclusion patterns per the
|
||||||
|
[official path-filter cheatsheet](https://docs.github.com/en/actions/using-workflows/triggering-a-workflow#patterns-to-match-file-paths).
|
||||||
|
Order matters: a `!pattern` only takes effect if it comes AFTER an
|
||||||
|
include that already matched the path. So the canonical shape is:
|
||||||
|
|
||||||
|
```yaml
|
||||||
|
paths:
|
||||||
|
- 'pages/**'
|
||||||
|
- '!pages/api/**' # must follow 'pages/**' to subtract from it
|
||||||
|
- 'components/**'
|
||||||
|
...
|
||||||
|
```
|
||||||
|
|
||||||
|
**Considered alternatives:**
|
||||||
|
|
||||||
|
1. **Per-feature paths** — replace `pages/**` with explicit
|
||||||
|
subdirectory globs (`pages/!(api)/**` *or* `pages/dashboard.js`,
|
||||||
|
`pages/cards/**`, `pages/decks/**`, …). Rejected: too verbose,
|
||||||
|
needs to be touched every time a top-level page is added,
|
||||||
|
defeats the "trigger on UI changes" intent.
|
||||||
|
2. **Extglob `pages/!(api)/**`** — would work in bash with extglob
|
||||||
|
enabled, but minimatch's default options used by GitHub Actions
|
||||||
|
do NOT enable extglob without a flag we can't set from YAML.
|
||||||
|
The queue entry explicitly flagged this risk; the negated-glob
|
||||||
|
shape is the safer documented path.
|
||||||
|
3. **Move the gate into the `gate:` job** — add a step that diffs
|
||||||
|
`pages/api/**` and sets `should_run=false` if every changed file
|
||||||
|
is API-only. Rejected: more code, more surface, doesn't actually
|
||||||
|
fire faster (the gate job itself spins up a runner). The native
|
||||||
|
`paths:` filter short-circuits BEFORE any runner spins up,
|
||||||
|
which is the cheapest possible exclusion.
|
||||||
|
|
||||||
|
The simple negation is sufficient and matches GitHub's published
|
||||||
|
guidance.
|
||||||
|
|
||||||
|
## The fix
|
||||||
|
|
||||||
|
Single edit in `.github/workflows/visual-diff.yml`. Insert
|
||||||
|
`!pages/api/**` immediately after `pages/**`, with an inline comment
|
||||||
|
explaining the ordering rule and the empirical motivation:
|
||||||
|
|
||||||
|
```yaml
|
||||||
|
paths:
|
||||||
|
- 'pages/**'
|
||||||
|
# Exclude API-only edits — they don't render UI, so they can't move
|
||||||
|
# any visual-diff pixels. Order matters: GitHub Actions evaluates the
|
||||||
|
# `paths:` list with minimatch and applies `!`-prefixed exclusions
|
||||||
|
# only after they've already matched a prior include. Keep this entry
|
||||||
|
# immediately AFTER `pages/**`.
|
||||||
|
# Surfaced by `tighten-visual-diff-path-filter` after PR #19
|
||||||
|
# (cors-tighten) and PR #20 (add-rate-limiting) both falsely
|
||||||
|
# triggered Screenshot diff at ~55s/PR.
|
||||||
|
- '!pages/api/**'
|
||||||
|
- 'components/**'
|
||||||
|
- 'styles/**'
|
||||||
|
- 'tailwind.config.js'
|
||||||
|
- 'postcss.config.js'
|
||||||
|
```
|
||||||
|
|
||||||
|
All five existing entries are preserved; only the one exclusion entry
|
||||||
|
is added.
|
||||||
|
|
||||||
|
### `.github/workflows/preview-smoke.yml` — left untouched
|
||||||
|
|
||||||
|
Verified the sibling workflow's shape:
|
||||||
|
|
||||||
|
```yaml
|
||||||
|
on:
|
||||||
|
pull_request:
|
||||||
|
branches: [main]
|
||||||
|
types: [labeled, opened, synchronize, reopened]
|
||||||
|
```
|
||||||
|
|
||||||
|
`preview-smoke.yml` has **no `paths:` filter at all** — it triggers
|
||||||
|
on every PR targeting `main` (modulo the in-job `gate:` skip via
|
||||||
|
`pipeline: skip smoke` in the PR body). This is intentional: a smoke
|
||||||
|
test that hits the home redirect, the sign-in page, and `/api/health`
|
||||||
|
SHOULD run on every PR including API-only ones, because changes to
|
||||||
|
`pages/api/**` can break those routes too. There is no false-positive
|
||||||
|
shape to fix here. Leaving `preview-smoke.yml` strictly out of scope.
|
||||||
|
|
||||||
|
## Verification plan
|
||||||
|
|
||||||
|
1. **YAML parse** — `python3 -c "import yaml; ..."` confirms the
|
||||||
|
`paths:` list deserializes to the expected 6-entry list with
|
||||||
|
`'!pages/api/**'` at index 1 (immediately after `'pages/**'`).
|
||||||
|
Done at gate time, see § Acceptance criteria.
|
||||||
|
2. **`npm run lint`** — exits 1 with **128 problems** (baseline
|
||||||
|
preserved, no regression). YAML files don't go through ESLint;
|
||||||
|
verification here is just that we didn't accidentally edit a
|
||||||
|
`.js` source file.
|
||||||
|
3. **`npm run test:run`** — **21/21 pass**. YAML changes don't touch
|
||||||
|
any test surface; verification only.
|
||||||
|
4. **Post-merge CI behavior verification — DEFERRED.** The only true
|
||||||
|
verification that the `!pages/api/**` exclusion actually fires
|
||||||
|
the way we expect is observing the **next API-only PR after this
|
||||||
|
merges** and confirming `Screenshot diff` does NOT appear in its
|
||||||
|
Checks tab. We document this explicitly here so the doc-writer
|
||||||
|
pass that closes the convoy can record the next API-only PR's
|
||||||
|
number + a "Screenshot diff: not triggered" line as the
|
||||||
|
as-shipped success criterion (mirroring the
|
||||||
|
`fix-reset-db-script` convoy's Screenshot-diff-not-triggered
|
||||||
|
line in its as-shipped block).
|
||||||
|
|
||||||
|
We do NOT attempt to live-verify the path filter at convoy time
|
||||||
|
(e.g. by pushing a throwaway API-only commit to a sacrificial
|
||||||
|
branch and watching CI). That'd be theater — GitHub's path-filter
|
||||||
|
semantics are documented and stable, and the YAML parse + the
|
||||||
|
syntax match against the published cheatsheet is enough
|
||||||
|
pre-merge confidence for a P3 polish convoy.
|
||||||
|
|
||||||
|
## Acceptance criteria
|
||||||
|
|
||||||
|
- `python3 -c "import yaml; d=yaml.safe_load(open('.github/workflows/visual-diff.yml')); print(d[True]['pull_request']['paths'])"`
|
||||||
|
→ `['pages/**', '!pages/api/**', 'components/**', 'styles/**', 'tailwind.config.js', 'postcss.config.js']`
|
||||||
|
(order-sensitive)
|
||||||
|
- `npm run lint` → exit 1 with 128 problems (baseline preserved)
|
||||||
|
- `npm run test:run` → 21/21 pass
|
||||||
|
- `.github/workflows/preview-smoke.yml` unchanged in this PR's diff
|
||||||
|
- No other workflow files touched
|
||||||
|
|
||||||
|
## Risks
|
||||||
|
|
||||||
|
- **R1 — minimatch syntax compatibility.** GitHub Actions uses
|
||||||
|
minimatch internally; the `!` prefix at the start of a pattern is
|
||||||
|
documented as the canonical exclusion syntax. If for any reason
|
||||||
|
Actions rejects this shape on the next workflow load (unlikely —
|
||||||
|
this exact shape is shown in the published cheatsheet), the
|
||||||
|
workflow would either fail to register OR silently treat the `!`
|
||||||
|
pattern as a literal include. **Fallback:** restructure to
|
||||||
|
per-feature path globs (`pages/dashboard.js`, `pages/cards/**`,
|
||||||
|
`pages/decks/**`, `pages/deck/**`, `pages/deck-builder.js`,
|
||||||
|
`pages/scanner.js`, `pages/profile.js`, `pages/settings.js`,
|
||||||
|
`pages/login.js`, `pages/register.js`, `pages/admin/**`,
|
||||||
|
`pages/collection/**`, `pages/collections.js`, `pages/community/**`,
|
||||||
|
`pages/invite/**`, `pages/my-cards.js`, `pages/card/**`,
|
||||||
|
`pages/_app.js`, `pages/_document.js`, `pages/_error.js`, `pages/index.js`).
|
||||||
|
More verbose, but unambiguously valid. Track as a follow-up convoy
|
||||||
|
ONLY if the post-merge verification step (next API-only PR) shows
|
||||||
|
the exclusion didn't fire.
|
||||||
|
- **R2 — future `pages/<non-api>/<api-like>` subdir.** If someone
|
||||||
|
later adds a directory like `pages/server/**` that contains both
|
||||||
|
API-style endpoints AND visual UI pages, the simple `!pages/api/**`
|
||||||
|
exclusion would not catch it, and visual-diff would fire on
|
||||||
|
changes to that directory. Documented but accepted: the repo
|
||||||
|
convention for the foreseeable future is "all backend lives under
|
||||||
|
`pages/api/**`", and the only realistic alternative ("server
|
||||||
|
components" or similar) would warrant its own paths-filter
|
||||||
|
revisit at that point. R2 is a "watch this space" risk, not a
|
||||||
|
blocker.
|
||||||
|
|
||||||
|
## Scope
|
||||||
|
|
||||||
|
- **In scope:** `.github/workflows/visual-diff.yml` only (plus this
|
||||||
|
convoy planning doc).
|
||||||
|
- **Out of scope:** any other workflow file. Verified
|
||||||
|
`preview-smoke.yml` has no `paths:` filter and intentionally fires
|
||||||
|
on every PR, so no mirror-fix is needed there.
|
||||||
|
|
||||||
|
## Why no architect
|
||||||
|
|
||||||
|
This is a **single-file YAML tweak following published vendor
|
||||||
|
documentation**. No new precedents; no new decisions; the queue
|
||||||
|
entry in `.convoys/ship-readiness.md` § Queued convoys already
|
||||||
|
ratified the negated-glob direction. Parent applies the fix, runs
|
||||||
|
the bounded checks (YAML parse + lint baseline + vitest), opens the
|
||||||
|
PR. If anything surprising surfaces (the YAML doesn't parse,
|
||||||
|
minimatch rejects the syntax), the parent stops and dispatches an
|
||||||
|
architect mid-execution.
|
||||||
|
|
||||||
|
## Out of scope (queued follow-ups)
|
||||||
|
|
||||||
|
- **`seed-visual-baselines-on-linux`** (priority: P3 polish; **was
|
||||||
|
already queued** by `adopt-playwright-smoke`) — once visual
|
||||||
|
baselines are seeded under `tests/visual/__screenshots__/` from a
|
||||||
|
Linux runner (or the documented Playwright Docker container), the
|
||||||
|
`Screenshot diff` job will start posting real visual-diff
|
||||||
|
comparisons and `continue-on-error: true` can be removed. This
|
||||||
|
convoy's path-filter tightening is orthogonal to baseline seeding —
|
||||||
|
both are needed eventually, but neither blocks the other. Surfaced
|
||||||
|
in `AGENTS.md` § 6 Testing.
|
||||||
|
|
||||||
|
## As-shipped
|
||||||
|
|
||||||
|
*(stub — populated by post-merge doc-writer pass)*
|
||||||
|
|
||||||
|
- Squash commit: `<TBD>`
|
||||||
|
- PR: `<TBD>`
|
||||||
|
- Diff stat: `<TBD>` (expected: 2 files, +N / -0 — `visual-diff.yml`
|
||||||
|
+N for the one entry + comment block; this convoy file +M for the
|
||||||
|
full planning doc)
|
||||||
|
- Verification at merge:
|
||||||
|
- YAML parse: paths list includes `!pages/api/**` immediately after
|
||||||
|
`pages/**`
|
||||||
|
- Lint: 128 problems (baseline preserved)
|
||||||
|
- Vitest: 21/21
|
||||||
|
- All pre-existing CI gates green at merge
|
||||||
|
- **Post-merge success criterion** (the deferred verification from
|
||||||
|
§ Verification plan): the next API-only PR after this merges does
|
||||||
|
NOT show `Screenshot diff` in its Checks tab. Doc-writer to record
|
||||||
|
that PR's number + the absence of `Screenshot diff` as the
|
||||||
|
as-shipped success line, mirroring `.convoys/fix-reset-db-script.md`'s
|
||||||
|
*"Screenshot diff: not triggered (script-only PR — `paths:` filter
|
||||||
|
excludes `scripts/**`…)"* line.
|
||||||
|
|
||||||
|
## Owns
|
||||||
|
|
||||||
|
Parent (single-file proven-pattern fix; no architect or implementer
|
||||||
|
subagent required).
|
||||||
|
|
@ -21,12 +21,15 @@ There are three parallel client-side auth implementations and one server-side he
|
||||||
| Client: route protection | `components/ProtectedRoute.js` |
|
| Client: route protection | `components/ProtectedRoute.js` |
|
||||||
| Client: admin route protection | `components/AdminProtected.js` |
|
| Client: admin route protection | `components/AdminProtected.js` |
|
||||||
|
|
||||||
## Legacy (do not extend)
|
## Legacy (deleted by `single-auth-provider`)
|
||||||
|
|
||||||
- `lib/auth-context.js::AuthProvider` + `useAuth` — older context. Still wired in `pages/_app.js`; left in place for compatibility. Don't add new consumers.
|
`lib/auth-context.js` and `lib/admin-auth.js` were the two parallel client-side
|
||||||
- `lib/admin-auth.js::AdminProvider` + `useAdmin` + `useIsAdmin` — parallel admin context. Same story.
|
auth surfaces that lived alongside `lib/use-auth.js`. They were deleted by the
|
||||||
|
`single-auth-provider` convoy (P1). Do **not** reintroduce a `<AuthProvider>`
|
||||||
A convoy is planned to collapse these three into one provider + one hook.
|
or `<AdminProvider>` wrapper in `pages/_app.js` — `useAuth()` from
|
||||||
|
`lib/use-auth.js` is hook-only (reads token from `localStorage` and hits
|
||||||
|
`/api/auth/verify` on mount) and does not require a context provider. The
|
||||||
|
`useIsAdmin` semantic is now `const { user } = useAuth(); const isAdmin = user?.role === 'admin'`.
|
||||||
|
|
||||||
## Token model
|
## Token model
|
||||||
|
|
||||||
|
|
@ -48,7 +51,7 @@ When introducing a new permission tier, update both `checkRolePermission`'s hier
|
||||||
|
|
||||||
## Authentication state on the client
|
## Authentication state on the client
|
||||||
|
|
||||||
`useAuth()` returns `{ user, loading, login, logout, refresh }`. `user === null` means logged out; `loading === true` means token verification in flight. Always render against `loading === false` before deciding to redirect.
|
`useAuth()` returns `{ user, loading, logout, refreshAuth }`. `user === null` means logged out; `loading === true` means token verification in flight. Always render against `loading === false` before deciding to redirect. The login / register flows do **not** go through `useAuth` — `pages/login.js` and `pages/signup.js` `fetch` `/api/auth/{login,register}` directly and write the returned token to `localStorage`; `useAuth()` will pick it up on next mount via its `useEffect` → `/api/auth/verify` roundtrip (or call `refreshAuth()` to re-verify in place).
|
||||||
|
|
||||||
## Server-side authorization patterns
|
## Server-side authorization patterns
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -33,5 +33,5 @@ Do not edit, refactor, or quote as context examples. If you think you need to ch
|
||||||
## Editing rules of thumb
|
## Editing rules of thumb
|
||||||
|
|
||||||
- **Schema changes:** ship as a `node-pg-migrate` migration under `migrations/` at the repo root (post-`migration-tool` convoy, 2026-05-26). Generate via `npm run migrate create <name> -- -j js`, then edit the generated file. Do NOT edit `scripts/setup-neon-db.js` for any **DDL change** (`CREATE TABLE`, `ALTER`, new columns, constraint changes) — the bootstrap script's DDL was relocated to `migrations/1779853647564_initial-schema.js` and `setup-neon-db.js` now owns only env-var validation, the migration-runner spawn, and the admin-row seed. **Operational changes to `setup-neon-db.js` are still allowed** (env-var gating, error-message hardening, module-system fixes) — the `drop-public-setup` convoy set that precedent. The legacy `scripts/migrations/YYYY-MM-DD-<slug>.js` placeholder is preserved for the lone existing pre-tool migration (`2026-05-24-rename-admin-email.js`) and is **not** used for new work; new migrations go in the repo-root `migrations/` directory and are wrapped by the tool.
|
- **Schema changes:** ship as a `node-pg-migrate` migration under `migrations/` at the repo root (post-`migration-tool` convoy, 2026-05-26). Generate via `npm run migrate create <name> -- -j js`, then edit the generated file. Do NOT edit `scripts/setup-neon-db.js` for any **DDL change** (`CREATE TABLE`, `ALTER`, new columns, constraint changes) — the bootstrap script's DDL was relocated to `migrations/1779853647564_initial-schema.js` and `setup-neon-db.js` now owns only env-var validation, the migration-runner spawn, and the admin-row seed. **Operational changes to `setup-neon-db.js` are still allowed** (env-var gating, error-message hardening, module-system fixes) — the `drop-public-setup` convoy set that precedent. The legacy `scripts/migrations/YYYY-MM-DD-<slug>.js` placeholder is preserved for the lone existing pre-tool migration (`2026-05-24-rename-admin-email.js`) and is **not** used for new work; new migrations go in the repo-root `migrations/` directory and are wrapped by the tool.
|
||||||
- **Auth refactors:** `lib/permission-middleware.js`, `pages/api/auth-utils.js`, `lib/auth-context.js`, `lib/admin-auth.js`, and `lib/use-auth.js` form a deliberately documented mess. Tighten them inside a single convoy; don't cherry-pick.
|
- **Auth refactors:** `lib/permission-middleware.js`, `pages/api/auth-utils.js`, `lib/auth-secret.js`, and `lib/use-auth.js` are the four documented auth surfaces. Tighten them inside a single convoy; don't cherry-pick. (The legacy `lib/auth-context.js` and `lib/admin-auth.js` were deleted by `single-auth-provider`; do not resurrect them.)
|
||||||
- **Card-import jobs:** `pages/api/cards/import-*.js` hit external APIs with rate limits. Don't run them ad-hoc against prod data; use staging.
|
- **Card-import jobs:** `pages/api/cards/import-*.js` hit external APIs with rate limits. Don't run them ad-hoc against prod data; use staging.
|
||||||
|
|
|
||||||
|
|
@ -93,7 +93,7 @@ Use Tailwind for layout, spacing, sizing, hover/focus states. Use CSS vars (inli
|
||||||
## Step 6: Check
|
## Step 6: Check
|
||||||
|
|
||||||
- [ ] Auth wrapper chosen (ProtectedRoute / AdminProtected / public).
|
- [ ] Auth wrapper chosen (ProtectedRoute / AdminProtected / public).
|
||||||
- [ ] `useAuth()` from `lib/use-auth.js` (not the legacy `lib/auth-context.js`).
|
- [ ] `useAuth()` from `lib/use-auth.js` (the only client auth hook; `lib/auth-context.js` and `lib/admin-auth.js` were deleted by the `single-auth-provider` convoy).
|
||||||
- [ ] `user` passed to Layout explicitly.
|
- [ ] `user` passed to Layout explicitly.
|
||||||
- [ ] Colors come from theme tokens, not hex.
|
- [ ] Colors come from theme tokens, not hex.
|
||||||
- [ ] All interactive elements have `aria-label` or visible text.
|
- [ ] All interactive elements have `aria-label` or visible text.
|
||||||
|
|
@ -105,5 +105,5 @@ Use Tailwind for layout, spacing, sizing, hover/focus states. Use CSS vars (inli
|
||||||
| --- | --- |
|
| --- | --- |
|
||||||
| Hardcode hex colors | Use CSS variables |
|
| Hardcode hex colors | Use CSS variables |
|
||||||
| Default `user = { … }` to a real email | Default to `null` |
|
| Default `user = { … }` to a real email | Default to `null` |
|
||||||
| Pull from `lib/auth-context` for new code | Use `lib/use-auth` |
|
| Reintroduce `lib/auth-context` or `lib/admin-auth` (deleted) | Use `lib/use-auth` |
|
||||||
| Render Layout twice on the same page | Single `<Layout>` at the top |
|
| Render Layout twice on the same page | Single `<Layout>` at the top |
|
||||||
|
|
|
||||||
2
.github/CODEOWNERS
vendored
2
.github/CODEOWNERS
vendored
|
|
@ -8,8 +8,6 @@
|
||||||
pages/api/auth/** @YOUR-GITHUB-HANDLE
|
pages/api/auth/** @YOUR-GITHUB-HANDLE
|
||||||
pages/api/auth-utils.js @YOUR-GITHUB-HANDLE
|
pages/api/auth-utils.js @YOUR-GITHUB-HANDLE
|
||||||
lib/permission-middleware.js @YOUR-GITHUB-HANDLE
|
lib/permission-middleware.js @YOUR-GITHUB-HANDLE
|
||||||
lib/auth-context.js @YOUR-GITHUB-HANDLE
|
|
||||||
lib/admin-auth.js @YOUR-GITHUB-HANDLE
|
|
||||||
lib/use-auth.js @YOUR-GITHUB-HANDLE
|
lib/use-auth.js @YOUR-GITHUB-HANDLE
|
||||||
components/ProtectedRoute.js @YOUR-GITHUB-HANDLE
|
components/ProtectedRoute.js @YOUR-GITHUB-HANDLE
|
||||||
components/AdminProtected.js @YOUR-GITHUB-HANDLE
|
components/AdminProtected.js @YOUR-GITHUB-HANDLE
|
||||||
|
|
|
||||||
9
.github/workflows/visual-diff.yml
vendored
9
.github/workflows/visual-diff.yml
vendored
|
|
@ -9,6 +9,15 @@ on:
|
||||||
branches: [main]
|
branches: [main]
|
||||||
paths:
|
paths:
|
||||||
- 'pages/**'
|
- 'pages/**'
|
||||||
|
# Exclude API-only edits — they don't render UI, so they can't move
|
||||||
|
# any visual-diff pixels. Order matters: GitHub Actions evaluates the
|
||||||
|
# `paths:` list with minimatch and applies `!`-prefixed exclusions
|
||||||
|
# only after they've already matched a prior include. Keep this entry
|
||||||
|
# immediately AFTER `pages/**`.
|
||||||
|
# Surfaced by `tighten-visual-diff-path-filter` after PR #19
|
||||||
|
# (cors-tighten) and PR #20 (add-rate-limiting) both falsely
|
||||||
|
# triggered Screenshot diff at ~55s/PR.
|
||||||
|
- '!pages/api/**'
|
||||||
- 'components/**'
|
- 'components/**'
|
||||||
- 'styles/**'
|
- 'styles/**'
|
||||||
- 'tailwind.config.js'
|
- 'tailwind.config.js'
|
||||||
|
|
|
||||||
|
|
@ -29,7 +29,7 @@ A web app for managing trading-card-game collections (Magic, Pokémon, Lorcana).
|
||||||
| Pages router views | `pages/*.js` | Public + auth views; uses `components/Layout.js` |
|
| Pages router views | `pages/*.js` | Public + auth views; uses `components/Layout.js` |
|
||||||
| API routes | `pages/api/**/*.js` | Express-style `handler(req, res)`. **30+ handlers depend on `lib/permission-middleware.js::getUserFromRequest`** |
|
| API routes | `pages/api/**/*.js` | Express-style `handler(req, res)`. **30+ handlers depend on `lib/permission-middleware.js::getUserFromRequest`** |
|
||||||
| Shared UI | `components/*.js` | `Layout`, `CardItem`, `CameraScanner`, modal family |
|
| Shared UI | `components/*.js` | `Layout`, `CardItem`, `CameraScanner`, modal family |
|
||||||
| Auth + DB libs | `lib/*.js` | `auth-context`, `admin-auth`, `use-auth` (three parallel auth surfaces), `database`, `permission-middleware` |
|
| Auth + DB libs | `lib/*.js` | `use-auth` (canonical client hook — sole surface post-`single-auth-provider`), `database`, `permission-middleware` |
|
||||||
| Migration scripts | `scripts/*.js` | 27+ one-off "add column" / "seed" scripts. No formal migration tool |
|
| Migration scripts | `scripts/*.js` | 27+ one-off "add column" / "seed" scripts. No formal migration tool |
|
||||||
| Card-import jobs | `pages/api/cards/import-*.js`, `scripts/import-*.js` | Scryfall / Lorcana / Pokémon TCG APIs |
|
| Card-import jobs | `pages/api/cards/import-*.js`, `scripts/import-*.js` | Scryfall / Lorcana / Pokémon TCG APIs |
|
||||||
| Database schema | `scripts/setup-neon-db.js` | Bootstrap SQL DDL — the source of truth until a real migration tool lands |
|
| Database schema | `scripts/setup-neon-db.js` | Bootstrap SQL DDL — the source of truth until a real migration tool lands |
|
||||||
|
|
@ -40,7 +40,7 @@ Code graph is indexed by `user-code-review-graph` MCP (122 files, 628 nodes, 560
|
||||||
## 3. Key conventions
|
## 3. Key conventions
|
||||||
|
|
||||||
- **Auth (server):** `import { getUserFromRequest } from '../../lib/permission-middleware'` → returns `{ userId, email, role }` or `null`. `null` means "send 401" — always early-return when the user is null before doing any work that depends on their identity.
|
- **Auth (server):** `import { getUserFromRequest } from '../../lib/permission-middleware'` → returns `{ userId, email, role }` or `null`. `null` means "send 401" — always early-return when the user is null before doing any work that depends on their identity.
|
||||||
- **Auth (client):** `import { useAuth } from '../lib/use-auth'`. Avoid `lib/auth-context.js` and `lib/admin-auth.js` for new code — they are legacy parallel implementations.
|
- **Auth (client):** `import { useAuth } from '../lib/use-auth'`. Returns `{ user, loading, logout, refreshAuth }`; `user === null` means logged out, `loading === true` means token verification in flight. There is no client-side admin hook — compute `const isAdmin = user?.role === 'admin'` from the same `useAuth()` call. The legacy `lib/auth-context.js` and `lib/admin-auth.js` were deleted by the `single-auth-provider` convoy; do not reintroduce a `<AuthProvider>` / `<AdminProvider>` wrapper in `pages/_app.js`.
|
||||||
- **Layout `user` prop:** pages should pass `user` from `useAuth()` to `<Layout>`. Layout's default is `null` and renders a logged-out "Sign in" CTA when no user is supplied — both paths are valid (some surfaces like `pages/invite/{accept,decline}.js` legitimately render Layout for anonymous visitors). Do not reintroduce a hardcoded user object as a default prop.
|
- **Layout `user` prop:** pages should pass `user` from `useAuth()` to `<Layout>`. Layout's default is `null` and renders a logged-out "Sign in" CTA when no user is supplied — both paths are valid (some surfaces like `pages/invite/{accept,decline}.js` legitimately render Layout for anonymous visitors). Do not reintroduce a hardcoded user object as a default prop.
|
||||||
- **JWT secret + TTL:** `import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js'`. This is the only place either value is defined; do not reintroduce literal fallbacks. `JWT_TOKEN_TTL = '24h'` is canonical.
|
- **JWT secret + TTL:** `import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js'`. This is the only place either value is defined; do not reintroduce literal fallbacks. `JWT_TOKEN_TTL = '24h'` is canonical.
|
||||||
- **Auth helper (token mint / verify / password hash):** `import { ... } from '../../pages/api/auth-utils'` (`generateToken`, `verifyToken`, `hashPassword`, `verifyPassword`). Reads the secret + TTL from `lib/auth-secret.js` under the hood.
|
- **Auth helper (token mint / verify / password hash):** `import { ... } from '../../pages/api/auth-utils'` (`generateToken`, `verifyToken`, `hashPassword`, `verifyPassword`). Reads the secret + TTL from `lib/auth-secret.js` under the hood.
|
||||||
|
|
|
||||||
|
|
@ -2,11 +2,21 @@
|
||||||
|
|
||||||
## 👥 Test Accounts
|
## 👥 Test Accounts
|
||||||
|
|
||||||
| User | Email | Password | Role |
|
| User | Email | Role | Password source |
|
||||||
|------|-------|----------|------|
|
|------|-------|------|-----------------|
|
||||||
| Admin | `admin@deckhearth.com` | `admin123` | Admin |
|
| Admin | `admin@deckhearth.com` | Admin | `ADMIN_INITIAL_PASSWORD` env var (seeded by `npm run setup-db`) |
|
||||||
| Alice | `alice@deckhearth.com` | `alice123` | User |
|
| Alice | `alice@deckhearth.com` | User | `TEST_USERS_PASSWORD` env var (seeded by `node scripts/create-test-users.js`) |
|
||||||
| Bob | `bob@deckhearth.com` | `bob123` | User |
|
| Bob | `bob@deckhearth.com` | User | `TEST_USERS_PASSWORD` env var (seeded by `node scripts/create-test-users.js`) |
|
||||||
|
|
||||||
|
Both env vars must be set in `.env.local` before running the
|
||||||
|
corresponding seed script — each script fails loud (exit 1, no DB
|
||||||
|
connection opened) if its env var is unset. Generate strong values
|
||||||
|
with `openssl rand -base64 24`; see README.md → "First-time admin
|
||||||
|
setup" for the canonical env-var pattern.
|
||||||
|
|
||||||
|
Alice + Bob share a single `TEST_USERS_PASSWORD` value because this is
|
||||||
|
a test-fixture surface; that's intentional and documented in the
|
||||||
|
`purge-weak-creds-from-helpers` convoy.
|
||||||
|
|
||||||
## 🃏 Sample Cards Available
|
## 🃏 Sample Cards Available
|
||||||
|
|
||||||
|
|
@ -21,8 +31,8 @@
|
||||||
|
|
||||||
### 1. **Login as Alice**
|
### 1. **Login as Alice**
|
||||||
```
|
```
|
||||||
Email: alice@deckhearth.com
|
Email: alice@deckhearth.com
|
||||||
Password: alice123
|
Password: <value of TEST_USERS_PASSWORD from .env.local>
|
||||||
```
|
```
|
||||||
|
|
||||||
### 2. **Create a Collection**
|
### 2. **Create a Collection**
|
||||||
|
|
@ -51,7 +61,7 @@ Password: alice123
|
||||||
### 5. **Switch to Bob's Account**
|
### 5. **Switch to Bob's Account**
|
||||||
- Logout and login as Bob
|
- Logout and login as Bob
|
||||||
- Email: `bob@deckhearth.com`
|
- Email: `bob@deckhearth.com`
|
||||||
- Password: `bob123`
|
- Password: `<value of TEST_USERS_PASSWORD from .env.local>`
|
||||||
|
|
||||||
### 6. **Accept Invitation (Simulated)**
|
### 6. **Accept Invitation (Simulated)**
|
||||||
Since we're testing locally, simulate email acceptance:
|
Since we're testing locally, simulate email acceptance:
|
||||||
|
|
|
||||||
|
|
@ -596,7 +596,6 @@ export default function Layout({ children, user = null, showSearch = false }) {
|
||||||
<div className="flex h-screen" style={{ backgroundColor: 'var(--bg-primary)' }}>
|
<div className="flex h-screen" style={{ backgroundColor: 'var(--bg-primary)' }}>
|
||||||
{/* Mobile Navigation - Bottom bar for mobile */}
|
{/* Mobile Navigation - Bottom bar for mobile */}
|
||||||
<MobileNavigation
|
<MobileNavigation
|
||||||
user={user}
|
|
||||||
onMenuOpen={() => setIsMobileMenuOpen(true)}
|
onMenuOpen={() => setIsMobileMenuOpen(true)}
|
||||||
/>
|
/>
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -2,7 +2,7 @@ import { useRouter } from 'next/router';
|
||||||
import Link from 'next/link';
|
import Link from 'next/link';
|
||||||
import { useState } from 'react';
|
import { useState } from 'react';
|
||||||
|
|
||||||
export default function MobileNavigation({ user, onMenuOpen }) {
|
export default function MobileNavigation({ onMenuOpen }) {
|
||||||
const router = useRouter();
|
const router = useRouter();
|
||||||
|
|
||||||
// Navigation items for the bottom bar
|
// Navigation items for the bottom bar
|
||||||
|
|
|
||||||
|
|
@ -11,6 +11,15 @@ const eslintConfig = defineConfig([
|
||||||
'next-env.d.ts',
|
'next-env.d.ts',
|
||||||
'scripts/migrations/**',
|
'scripts/migrations/**',
|
||||||
]),
|
]),
|
||||||
|
{
|
||||||
|
files: ['scripts/**/*.js'],
|
||||||
|
rules: {
|
||||||
|
'no-restricted-syntax': ['error', {
|
||||||
|
selector: 'CallExpression[callee.name="require"]',
|
||||||
|
message: 'Use ESM `import` syntax. `package.json` has "type": "module"; require() throws ReferenceError at runtime. See .convoys/fix-reset-db-script.md.',
|
||||||
|
}],
|
||||||
|
},
|
||||||
|
},
|
||||||
]);
|
]);
|
||||||
|
|
||||||
export default eslintConfig;
|
export default eslintConfig;
|
||||||
|
|
|
||||||
|
|
@ -1,120 +0,0 @@
|
||||||
import { createContext, useContext, useState, useEffect } from 'react';
|
|
||||||
|
|
||||||
// Create admin context
|
|
||||||
const AdminContext = createContext();
|
|
||||||
|
|
||||||
export function AdminProvider({ children }) {
|
|
||||||
const [user, setUser] = useState(null);
|
|
||||||
const [loading, setLoading] = useState(true);
|
|
||||||
|
|
||||||
useEffect(() => {
|
|
||||||
checkAdminAuth();
|
|
||||||
}, []);
|
|
||||||
|
|
||||||
const checkAdminAuth = async () => {
|
|
||||||
try {
|
|
||||||
// Get token from localStorage
|
|
||||||
const token = localStorage.getItem('auth_token');
|
|
||||||
|
|
||||||
const headers = {
|
|
||||||
'Content-Type': 'application/json',
|
|
||||||
};
|
|
||||||
|
|
||||||
// Add authorization header if token exists
|
|
||||||
if (token) {
|
|
||||||
headers.Authorization = `Bearer ${token}`;
|
|
||||||
}
|
|
||||||
|
|
||||||
const response = await fetch('/api/auth/verify', { headers });
|
|
||||||
if (response.ok) {
|
|
||||||
const userData = await response.json();
|
|
||||||
setUser(userData);
|
|
||||||
} else {
|
|
||||||
setUser(null);
|
|
||||||
// Clear invalid token
|
|
||||||
if (token) {
|
|
||||||
localStorage.removeItem('auth_token');
|
|
||||||
}
|
|
||||||
}
|
|
||||||
} catch (error) {
|
|
||||||
console.error('Auth check failed:', error);
|
|
||||||
setUser(null);
|
|
||||||
} finally {
|
|
||||||
setLoading(false);
|
|
||||||
}
|
|
||||||
};
|
|
||||||
|
|
||||||
const isAdmin = () => {
|
|
||||||
return user && user.role === 'admin';
|
|
||||||
};
|
|
||||||
|
|
||||||
const isAuthenticated = () => {
|
|
||||||
return user !== null;
|
|
||||||
};
|
|
||||||
|
|
||||||
const value = {
|
|
||||||
user,
|
|
||||||
loading,
|
|
||||||
isAdmin,
|
|
||||||
isAuthenticated,
|
|
||||||
checkAdminAuth
|
|
||||||
};
|
|
||||||
|
|
||||||
return (
|
|
||||||
<AdminContext.Provider value={value}>
|
|
||||||
{children}
|
|
||||||
</AdminContext.Provider>
|
|
||||||
);
|
|
||||||
}
|
|
||||||
|
|
||||||
export function useAdmin() {
|
|
||||||
const context = useContext(AdminContext);
|
|
||||||
if (!context) {
|
|
||||||
throw new Error('useAdmin must be used within an AdminProvider');
|
|
||||||
}
|
|
||||||
return context;
|
|
||||||
}
|
|
||||||
|
|
||||||
// Simple hook for checking admin status without context
|
|
||||||
export function useIsAdmin() {
|
|
||||||
const [isAdmin, setIsAdmin] = useState(false);
|
|
||||||
const [loading, setLoading] = useState(true);
|
|
||||||
|
|
||||||
useEffect(() => {
|
|
||||||
const checkAdmin = async () => {
|
|
||||||
try {
|
|
||||||
// Get token from localStorage
|
|
||||||
const token = localStorage.getItem('auth_token');
|
|
||||||
|
|
||||||
const headers = {
|
|
||||||
'Content-Type': 'application/json',
|
|
||||||
};
|
|
||||||
|
|
||||||
// Add authorization header if token exists
|
|
||||||
if (token) {
|
|
||||||
headers.Authorization = `Bearer ${token}`;
|
|
||||||
}
|
|
||||||
|
|
||||||
const response = await fetch('/api/auth/verify', { headers });
|
|
||||||
if (response.ok) {
|
|
||||||
const userData = await response.json();
|
|
||||||
setIsAdmin(userData.role === 'admin');
|
|
||||||
} else {
|
|
||||||
setIsAdmin(false);
|
|
||||||
// Clear invalid token
|
|
||||||
if (token) {
|
|
||||||
localStorage.removeItem('auth_token');
|
|
||||||
}
|
|
||||||
}
|
|
||||||
} catch (error) {
|
|
||||||
setIsAdmin(false);
|
|
||||||
} finally {
|
|
||||||
setLoading(false);
|
|
||||||
}
|
|
||||||
};
|
|
||||||
|
|
||||||
checkAdmin();
|
|
||||||
}, []);
|
|
||||||
|
|
||||||
return { isAdmin, loading };
|
|
||||||
}
|
|
||||||
|
|
@ -1,116 +0,0 @@
|
||||||
import { createContext, useContext, useState, useEffect } from 'react';
|
|
||||||
|
|
||||||
const AuthContext = createContext();
|
|
||||||
|
|
||||||
export function AuthProvider({ children }) {
|
|
||||||
const [user, setUser] = useState(null);
|
|
||||||
const [loading, setLoading] = useState(true);
|
|
||||||
|
|
||||||
useEffect(() => {
|
|
||||||
// Check for existing token on app load
|
|
||||||
const token = localStorage.getItem('auth_token');
|
|
||||||
if (token) {
|
|
||||||
// Verify token and set user
|
|
||||||
verifyToken(token);
|
|
||||||
} else {
|
|
||||||
setLoading(false);
|
|
||||||
}
|
|
||||||
}, []);
|
|
||||||
|
|
||||||
const verifyToken = async (token) => {
|
|
||||||
try {
|
|
||||||
const response = await fetch('/api/auth/verify', {
|
|
||||||
headers: {
|
|
||||||
'Authorization': `Bearer ${token}`
|
|
||||||
}
|
|
||||||
});
|
|
||||||
|
|
||||||
if (response.ok) {
|
|
||||||
const userData = await response.json();
|
|
||||||
setUser(userData); // API returns user data directly, not wrapped in .user
|
|
||||||
} else {
|
|
||||||
localStorage.removeItem('auth_token');
|
|
||||||
}
|
|
||||||
} catch (error) {
|
|
||||||
console.error('Token verification failed:', error);
|
|
||||||
localStorage.removeItem('auth_token');
|
|
||||||
} finally {
|
|
||||||
setLoading(false);
|
|
||||||
}
|
|
||||||
};
|
|
||||||
|
|
||||||
const login = async (email, password) => {
|
|
||||||
try {
|
|
||||||
const response = await fetch('/api/auth/login', {
|
|
||||||
method: 'POST',
|
|
||||||
headers: {
|
|
||||||
'Content-Type': 'application/json',
|
|
||||||
},
|
|
||||||
body: JSON.stringify({ email, password }),
|
|
||||||
});
|
|
||||||
|
|
||||||
const data = await response.json();
|
|
||||||
|
|
||||||
if (response.ok) {
|
|
||||||
localStorage.setItem('auth_token', data.token);
|
|
||||||
setUser(data.user);
|
|
||||||
return { success: true };
|
|
||||||
} else {
|
|
||||||
return { success: false, error: data.error };
|
|
||||||
}
|
|
||||||
} catch (error) {
|
|
||||||
return { success: false, error: 'Network error' };
|
|
||||||
}
|
|
||||||
};
|
|
||||||
|
|
||||||
const register = async (email, password) => {
|
|
||||||
try {
|
|
||||||
const response = await fetch('/api/auth/register', {
|
|
||||||
method: 'POST',
|
|
||||||
headers: {
|
|
||||||
'Content-Type': 'application/json',
|
|
||||||
},
|
|
||||||
body: JSON.stringify({ email, password }),
|
|
||||||
});
|
|
||||||
|
|
||||||
const data = await response.json();
|
|
||||||
|
|
||||||
if (response.ok) {
|
|
||||||
localStorage.setItem('auth_token', data.token);
|
|
||||||
setUser(data.user);
|
|
||||||
return { success: true };
|
|
||||||
} else {
|
|
||||||
return { success: false, error: data.error };
|
|
||||||
}
|
|
||||||
} catch (error) {
|
|
||||||
return { success: false, error: 'Network error' };
|
|
||||||
}
|
|
||||||
};
|
|
||||||
|
|
||||||
const logout = () => {
|
|
||||||
localStorage.removeItem('auth_token');
|
|
||||||
setUser(null);
|
|
||||||
};
|
|
||||||
|
|
||||||
const value = {
|
|
||||||
user,
|
|
||||||
loading,
|
|
||||||
login,
|
|
||||||
register,
|
|
||||||
logout,
|
|
||||||
};
|
|
||||||
|
|
||||||
return (
|
|
||||||
<AuthContext.Provider value={value}>
|
|
||||||
{children}
|
|
||||||
</AuthContext.Provider>
|
|
||||||
);
|
|
||||||
}
|
|
||||||
|
|
||||||
export function useAuth() {
|
|
||||||
const context = useContext(AuthContext);
|
|
||||||
if (!context) {
|
|
||||||
throw new Error('useAuth must be used within an AuthProvider');
|
|
||||||
}
|
|
||||||
return context;
|
|
||||||
}
|
|
||||||
|
|
@ -1,47 +0,0 @@
|
||||||
import { neon } from '@neondatabase/serverless';
|
|
||||||
|
|
||||||
// Get the database URL from environment variables
|
|
||||||
const sql = neon(process.env.POSTGRES_URL);
|
|
||||||
|
|
||||||
// Database adapter that works with Neon
|
|
||||||
class DatabaseAdapter {
|
|
||||||
async query(sqlString, params = []) {
|
|
||||||
try {
|
|
||||||
// For Neon, we need to use tagged template literals
|
|
||||||
// This is a simplified approach - in production you'd want more robust parameter handling
|
|
||||||
let result;
|
|
||||||
|
|
||||||
if (params.length === 0) {
|
|
||||||
// No parameters
|
|
||||||
result = await sql`${sql.unsafe(sqlString)}`;
|
|
||||||
} else {
|
|
||||||
// With parameters - this is a simplified approach
|
|
||||||
// In production, you'd want proper parameter escaping
|
|
||||||
const escapedParams = params.map(param =>
|
|
||||||
typeof param === 'string' ? `'${param.replace(/'/g, "''")}'` : param
|
|
||||||
);
|
|
||||||
|
|
||||||
let query = sqlString;
|
|
||||||
for (let i = 0; i < escapedParams.length; i++) {
|
|
||||||
query = query.replace(`$${i + 1}`, escapedParams[i]);
|
|
||||||
}
|
|
||||||
|
|
||||||
result = await sql`${sql.unsafe(query)}`;
|
|
||||||
}
|
|
||||||
|
|
||||||
return {
|
|
||||||
rows: Array.isArray(result) ? result : [result],
|
|
||||||
rowCount: Array.isArray(result) ? result.length : 1
|
|
||||||
};
|
|
||||||
} catch (error) {
|
|
||||||
console.error('Database query error:', error);
|
|
||||||
throw error;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
async raw(sqlString, params = []) {
|
|
||||||
return await this.query(sqlString, params);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
export const db = new DatabaseAdapter();
|
|
||||||
|
|
@ -1,13 +1,10 @@
|
||||||
import '../styles/globals.css';
|
import '../styles/globals.css';
|
||||||
import { AuthProvider } from '../lib/auth-context.js';
|
|
||||||
import { ThemeProvider } from '../lib/theme-context.js';
|
import { ThemeProvider } from '../lib/theme-context.js';
|
||||||
|
|
||||||
export default function App({ Component, pageProps }) {
|
export default function App({ Component, pageProps }) {
|
||||||
return (
|
return (
|
||||||
<ThemeProvider>
|
<ThemeProvider>
|
||||||
<AuthProvider>
|
<Component {...pageProps} />
|
||||||
<Component {...pageProps} />
|
|
||||||
</AuthProvider>
|
|
||||||
</ThemeProvider>
|
</ThemeProvider>
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
@ -1,5 +1,5 @@
|
||||||
import jwt from 'jsonwebtoken';
|
import jwt from 'jsonwebtoken';
|
||||||
import { db } from '../../lib/database.js';
|
import { sql } from '@vercel/postgres';
|
||||||
import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js';
|
import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js';
|
||||||
|
|
||||||
export async function hashPassword(password) {
|
export async function hashPassword(password) {
|
||||||
|
|
@ -34,9 +34,9 @@ export function verifyToken(token) {
|
||||||
|
|
||||||
export async function isAdmin(userId) {
|
export async function isAdmin(userId) {
|
||||||
try {
|
try {
|
||||||
const result = await db.query(`
|
const result = await sql`
|
||||||
SELECT role FROM users WHERE id = $1
|
SELECT role FROM users WHERE id = ${userId}
|
||||||
`, [userId]);
|
`;
|
||||||
return result.rows[0]?.role === 'admin';
|
return result.rows[0]?.role === 'admin';
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.error('Error checking admin status:', error);
|
console.error('Error checking admin status:', error);
|
||||||
|
|
@ -46,9 +46,9 @@ export async function isAdmin(userId) {
|
||||||
|
|
||||||
export async function getUserById(userId) {
|
export async function getUserById(userId) {
|
||||||
try {
|
try {
|
||||||
const result = await db.query(`
|
const result = await sql`
|
||||||
SELECT id, email, role, created_at FROM users WHERE id = $1
|
SELECT id, email, role, created_at FROM users WHERE id = ${userId}
|
||||||
`, [userId]);
|
`;
|
||||||
return result.rows[0];
|
return result.rows[0];
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.error('Error getting user:', error);
|
console.error('Error getting user:', error);
|
||||||
|
|
|
||||||
|
|
@ -1,7 +1,6 @@
|
||||||
import { useState, useEffect } from 'react';
|
import { useState, useEffect } from 'react';
|
||||||
import { useRouter } from 'next/router';
|
import { useRouter } from 'next/router';
|
||||||
import Layout from '../../components/Layout';
|
import Layout from '../../components/Layout';
|
||||||
import { useIsAdmin } from '../../lib/admin-auth';
|
|
||||||
import { useAuth } from '../../lib/use-auth';
|
import { useAuth } from '../../lib/use-auth';
|
||||||
import CollectionSelectionModal from '../../components/CollectionSelectionModal';
|
import CollectionSelectionModal from '../../components/CollectionSelectionModal';
|
||||||
import { ManaCost, ColorIdentity, AdvancedManaCost } from '../../components/ManaSymbols';
|
import { ManaCost, ColorIdentity, AdvancedManaCost } from '../../components/ManaSymbols';
|
||||||
|
|
@ -10,7 +9,7 @@ import ManaSymbolSettings from '../../components/ManaSymbolSettings';
|
||||||
export default function CardDetail() {
|
export default function CardDetail() {
|
||||||
const router = useRouter();
|
const router = useRouter();
|
||||||
const { id } = router.query;
|
const { id } = router.query;
|
||||||
const { user } = useAuth();
|
const { user, loading: authLoading } = useAuth();
|
||||||
|
|
||||||
const [card, setCard] = useState(null);
|
const [card, setCard] = useState(null);
|
||||||
const [loading, setLoading] = useState(true);
|
const [loading, setLoading] = useState(true);
|
||||||
|
|
@ -31,8 +30,8 @@ export default function CardDetail() {
|
||||||
// Mana symbol settings
|
// Mana symbol settings
|
||||||
const [manaSymbolSettings, setManaSymbolSettings] = useState({ useSVG: false });
|
const [manaSymbolSettings, setManaSymbolSettings] = useState({ useSVG: false });
|
||||||
|
|
||||||
// Check admin status
|
const isAdmin = user?.role === 'admin';
|
||||||
const { isAdmin, loading: adminLoading } = useIsAdmin();
|
const adminLoading = authLoading;
|
||||||
|
|
||||||
// Fetch card data from API
|
// Fetch card data from API
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
|
|
|
||||||
|
|
@ -4,7 +4,7 @@ import Link from 'next/link';
|
||||||
import Layout from '../components/Layout';
|
import Layout from '../components/Layout';
|
||||||
import { ManaCost, ColorIdentity, ColorFilterSymbol } from '../components/ManaSymbols';
|
import { ManaCost, ColorIdentity, ColorFilterSymbol } from '../components/ManaSymbols';
|
||||||
import ManaSymbolSettings from '../components/ManaSymbolSettings';
|
import ManaSymbolSettings from '../components/ManaSymbolSettings';
|
||||||
import { useAuth } from '../lib/auth-context';
|
import { useAuth } from '../lib/use-auth';
|
||||||
import { getColorIdentity, getColorSymbol } from '../lib/mana-symbols';
|
import { getColorIdentity, getColorSymbol } from '../lib/mana-symbols';
|
||||||
|
|
||||||
export default function DeckBuilder() {
|
export default function DeckBuilder() {
|
||||||
|
|
|
||||||
|
|
@ -3,7 +3,7 @@ import { useRouter } from 'next/router';
|
||||||
import Link from 'next/link';
|
import Link from 'next/link';
|
||||||
import Layout from '../../components/Layout';
|
import Layout from '../../components/Layout';
|
||||||
import { ManaCost, ColorIdentity } from '../../components/ManaSymbols';
|
import { ManaCost, ColorIdentity } from '../../components/ManaSymbols';
|
||||||
import { useAuth } from '../../lib/auth-context';
|
import { useAuth } from '../../lib/use-auth';
|
||||||
import { getColorIdentity } from '../../lib/mana-symbols';
|
import { getColorIdentity } from '../../lib/mana-symbols';
|
||||||
|
|
||||||
export default function DeckDetail() {
|
export default function DeckDetail() {
|
||||||
|
|
|
||||||
|
|
@ -2,7 +2,7 @@ import { useState, useEffect } from 'react';
|
||||||
import { useRouter } from 'next/router';
|
import { useRouter } from 'next/router';
|
||||||
import Link from 'next/link';
|
import Link from 'next/link';
|
||||||
import Layout from '../components/Layout';
|
import Layout from '../components/Layout';
|
||||||
import { useAuth } from '../lib/auth-context';
|
import { useAuth } from '../lib/use-auth';
|
||||||
|
|
||||||
export default function Decks() {
|
export default function Decks() {
|
||||||
const { user } = useAuth();
|
const { user } = useAuth();
|
||||||
|
|
|
||||||
|
|
@ -1,7 +1,7 @@
|
||||||
import { useState, useEffect } from 'react';
|
import { useState, useEffect } from 'react';
|
||||||
import { useRouter } from 'next/router';
|
import { useRouter } from 'next/router';
|
||||||
import Link from 'next/link';
|
import Link from 'next/link';
|
||||||
import { useAuth } from '../lib/auth-context.js';
|
import { useAuth } from '../lib/use-auth.js';
|
||||||
import AnimatedFireLogo from '../components/AnimatedFireLogo';
|
import AnimatedFireLogo from '../components/AnimatedFireLogo';
|
||||||
|
|
||||||
export default function Home() {
|
export default function Home() {
|
||||||
|
|
|
||||||
|
|
@ -5,7 +5,7 @@ import CameraScanner from '../components/CameraScanner';
|
||||||
import OCRSettings from '../components/OCRSettings';
|
import OCRSettings from '../components/OCRSettings';
|
||||||
import { ManaCost, ColorIdentity } from '../components/ManaSymbols';
|
import { ManaCost, ColorIdentity } from '../components/ManaSymbols';
|
||||||
import ManaSymbolSettings from '../components/ManaSymbolSettings';
|
import ManaSymbolSettings from '../components/ManaSymbolSettings';
|
||||||
import { useAuth } from '../lib/auth-context';
|
import { useAuth } from '../lib/use-auth';
|
||||||
|
|
||||||
export default function Scanner() {
|
export default function Scanner() {
|
||||||
const { user } = useAuth();
|
const { user } = useAuth();
|
||||||
|
|
|
||||||
|
|
@ -1,39 +1,69 @@
|
||||||
#!/usr/bin/env node
|
#!/usr/bin/env node
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Create Test Users Script
|
||||||
|
*
|
||||||
|
* Seeds the alice + bob test-user fixtures used by the manual QA flows
|
||||||
|
* in TESTING_GUIDE.md. Both users share a single password supplied via
|
||||||
|
* the TEST_USERS_PASSWORD environment variable — this is a test-fixture
|
||||||
|
* helper, not a prod auth surface, so per-user env vars would be
|
||||||
|
* unnecessary sprawl.
|
||||||
|
*
|
||||||
|
* Required env (in .env.local):
|
||||||
|
* POSTGRES_URL — Neon connection string
|
||||||
|
* TEST_USERS_PASSWORD — strong password applied to every test user
|
||||||
|
* (generate with `openssl rand -base64 24`)
|
||||||
|
*
|
||||||
|
* Mirrors the post-`drop-public-setup` shape of `setup-neon-db.js`
|
||||||
|
* (commit b63b509) and the post-`fix-reset-db-script` shape of
|
||||||
|
* `reset-db.js` (commit 3ab9bf8) — same ESM imports, same fail-loud
|
||||||
|
* env-var check, same no-password-echo convention. Convoy:
|
||||||
|
* `purge-weak-creds-from-helpers` (2026-05-26).
|
||||||
|
*/
|
||||||
|
|
||||||
import { config } from 'dotenv';
|
import { config } from 'dotenv';
|
||||||
import { sql } from '@vercel/postgres';
|
import { sql } from '@vercel/postgres';
|
||||||
import bcrypt from 'bcryptjs';
|
import bcrypt from 'bcryptjs';
|
||||||
|
|
||||||
// Load environment variables
|
|
||||||
config({ path: '.env.local' });
|
config({ path: '.env.local' });
|
||||||
|
|
||||||
async function createTestUsers() {
|
async function createTestUsers() {
|
||||||
|
const testUsersPassword = process.env.TEST_USERS_PASSWORD;
|
||||||
|
if (!testUsersPassword || !testUsersPassword.trim()) {
|
||||||
|
console.error(
|
||||||
|
'❌ TEST_USERS_PASSWORD environment variable is not set.\n' +
|
||||||
|
'\n' +
|
||||||
|
' Set it in .env.local before running `node scripts/create-test-users.js`.\n' +
|
||||||
|
' Generate a strong password with: openssl rand -base64 24\n' +
|
||||||
|
' See README.md → "First-time admin setup" for the env-var pattern.\n'
|
||||||
|
);
|
||||||
|
process.exit(1);
|
||||||
|
}
|
||||||
|
|
||||||
try {
|
try {
|
||||||
console.log('<27><> Creating test users...\n');
|
console.log('👥 Creating test users...\n');
|
||||||
|
|
||||||
|
const hashedPassword = await bcrypt.hash(testUsersPassword, 12);
|
||||||
|
|
||||||
// Create Alice (collaborator)
|
|
||||||
const alicePassword = await bcrypt.hash('alice123', 12);
|
|
||||||
await sql`
|
await sql`
|
||||||
INSERT INTO users (email, password, role)
|
INSERT INTO users (email, password, role)
|
||||||
VALUES ('alice@deckhearth.com', ${alicePassword}, 'user')
|
VALUES ('alice@deckhearth.com', ${hashedPassword}, 'user')
|
||||||
ON CONFLICT (email) DO NOTHING
|
ON CONFLICT (email) DO NOTHING
|
||||||
`;
|
`;
|
||||||
console.log('✅ Created Alice (alice@deckhearth.com / alice123)');
|
console.log('✅ Created Alice (alice@deckhearth.com)');
|
||||||
|
|
||||||
// Create Bob (collaborator)
|
|
||||||
const bobPassword = await bcrypt.hash('bob123', 12);
|
|
||||||
await sql`
|
await sql`
|
||||||
INSERT INTO users (email, password, role)
|
INSERT INTO users (email, password, role)
|
||||||
VALUES ('bob@deckhearth.com', ${bobPassword}, 'user')
|
VALUES ('bob@deckhearth.com', ${hashedPassword}, 'user')
|
||||||
ON CONFLICT (email) DO NOTHING
|
ON CONFLICT (email) DO NOTHING
|
||||||
`;
|
`;
|
||||||
console.log('✅ Created Bob (bob@deckhearth.com / bob123)');
|
console.log('✅ Created Bob (bob@deckhearth.com)');
|
||||||
|
|
||||||
console.log('\n🎉 Test users created successfully!');
|
console.log('\n🎉 Test users created successfully!');
|
||||||
console.log('\n👥 Available Test Accounts:');
|
console.log('\n👥 Available Test Accounts (passwords from TEST_USERS_PASSWORD):');
|
||||||
console.log(' 1. admin@deckhearth.com / admin123 (Admin)');
|
console.log(' 1. admin@deckhearth.com (Admin — seeded by setup-neon-db.js)');
|
||||||
console.log(' 2. alice@deckhearth.com / alice123 (User)');
|
console.log(' 2. alice@deckhearth.com (User)');
|
||||||
console.log(' 3. bob@deckhearth.com / bob123 (User)');
|
console.log(' 3. bob@deckhearth.com (User)');
|
||||||
|
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.error('❌ Failed to create test users:', error.message);
|
console.error('❌ Failed to create test users:', error.message);
|
||||||
|
|
|
||||||
|
|
@ -1,10 +1,6 @@
|
||||||
import { describe, expect, it, vi } from 'vitest';
|
import { describe, expect, it } from 'vitest';
|
||||||
import jwt from 'jsonwebtoken';
|
import jwt from 'jsonwebtoken';
|
||||||
|
|
||||||
vi.mock('../../lib/database.js', () => ({
|
|
||||||
db: { query: vi.fn() },
|
|
||||||
}));
|
|
||||||
|
|
||||||
import { JWT_SECRET } from '../../lib/auth-secret.js';
|
import { JWT_SECRET } from '../../lib/auth-secret.js';
|
||||||
import { generateToken, verifyToken } from '../../pages/api/auth-utils.js';
|
import { generateToken, verifyToken } from '../../pages/api/auth-utils.js';
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue