Close stale convoy frontmatter for merged scanner, lint, and hygiene work; record P1 #11.5 and queued follow-ups as RESOLVED with PR references. Co-authored-by: Cursor <cursoragent@cursor.com>
24 KiB
| name | classification | success_metric | skip | status | created | parent | addresses | depends_on | ||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| fix-layout-default-user | convoy | components/Layout.js's user prop defaults to null. Every page in pages/** that renders Layout either passes a user prop explicitly OR relies on the new logged-out rendering branch. No real email address remains in any component default-prop. Manual smoke: load an unauthenticated session on a page that previously impersonated the maintainer; confirm the navbar / profile dropdown reads "Sign in" rather than "me@randallstillwell.com". |
|
shipped | 2026-05-23 | ship-readiness | P0 |
|
Fix Layout default user
As-shipped: PR #15, squash ca302a8 (2026-05-24). Closes P0 #7.
Close P0 #7 from .convoys/ship-readiness.md (the last remaining P0
ship-blocker). components/Layout.js line 562 defaults the user prop to
{ email: 'me@randallstillwell.com', role: 'user' } — any page that renders
Layout without passing user explicitly displays the maintainer's real
email and impersonates them as the logged-in user.
Scope (verbatim from ship-readiness P0 #7)
components/Layout.js— change theLayout({ children, user = {...} })default touser = null. Add a logged-out rendering branch (navbar / profile dropdown / mobile menu) that handlesuser === nullcleanly — typically "Sign in" CTA replacing the user avatar + dropdown.- 17 pages in
pages/**that import Layout (confirmed via grep):
For each: confirm it passespages/scanner.js pages/collection/[identifier].js pages/card/[id].js pages/my-cards.js pages/cards.js pages/deck-builder.js pages/deck/[id].js pages/decks.js pages/dashboard.js pages/community/collections.js pages/collections.js pages/settings.js pages/profile.js pages/invite/decline.js pages/invite/accept.js pages/admin/card-import.js pages/admin/card-editor.jsuserexplicitly OR triage that it should use the new logged-out branch (e.g. public pages like card/[id].js, community/collections.js may legitimately render Layout for anonymous visitors).
Out of scope
- Branding — Layout still renders "Deck Hearth" / "DH" while the rest of
the repo says "TCG Vault". That mismatch is the queued
pick-a-nameconvoy (P1 #12). Don't fix branding here. - Three parallel auth providers — Layout reads from one of
lib/auth-context.js/lib/admin-auth.js/lib/use-auth.js. Collapsing them is the queuedsingle-auth-providerconvoy (P1 #9). - God-component split —
components/Layout.jsis 700+ lines. Splitting is the queuedgod-component-splitconvoy (P2 #13). Touch only the user- prop default and the logged-out rendering branch in THIS convoy. MobileNavigation— receivesuserfrom Layout. May need a similar default-prop fix if it has the same anti-pattern. Audit during architect pass and decide whether to fold in or queue separately.AGENTS.mdGotcha #8 — will be marked RESOLVED in the post-convoy doc-writer pass; do not pre-emptively edit AGENTS.md here.
Architect's questions
-
Logged-out rendering branch design. When
user === null, what should Layout render?- Q1a: Navbar / profile dropdown — replace the user avatar + email
with a "Sign in" link to
/login? - Q1b: Mobile bottom-nav — same treatment, or hide the user-only items entirely?
- Q1c: Authenticated-only nav items (admin, settings, profile) —
hide them, or show but link to
/login?
- Q1a: Navbar / profile dropdown — replace the user avatar + email
with a "Sign in" link to
-
Page audit triage. For each of the 17 pages, three buckets:
- Always-authenticated (dashboard, my-cards, profile, settings,
scanner, admin/*) — must pass
userexplicitly; pages without it should add it viauseAuth(). - Public-or-authenticated (cards, card/[id], collection/[identifier],
community/collections, deck/[id], collections, decks) — currently
show different views based on auth; the Layout user prop should
come from
useAuth()either way. - Anonymous-allowed (invite/decline, invite/accept) — may legitimately render Layout without a user; rely on the new logged-out branch.
The architect should produce the exact bucket assignment per page and the brief should give the implementer the per-page instruction.
- Always-authenticated (dashboard, my-cards, profile, settings,
scanner, admin/*) — must pass
-
Brief decomposition. Three options:
- Single brief, one PR. All 18 files (Layout + 17 pages) in one diff. Reviewable but big.
- Two briefs, one PR. Brief 1: Layout change + logged-out rendering. Brief 2: page audit (depends on Brief 1). Both ship together.
- Fan-out by page bucket. Brief 1: Layout change. Brief 2: always-auth pages. Brief 3: public-or-auth pages. Brief 4: anonymous-allowed pages. Multitask-friendly via worktrees.
Recommend two briefs in one PR for size + reviewability balance, unless the page audit reveals >10 files needing real changes (in which case fan-out makes sense).
-
Test coverage. Should this convoy add vitest tests that exercise Layout's logged-out branch? The fix-auth-bypass convoy added 16 auth tests (
test/lib/permission-middleware.test.js); a similar lock-in for the user-prop default could prevent regression.Recommend yes — a single test that asserts
Layoutrenders the logged-out shape whenuser === undefinedanduser === nullwould catch any future regression that reintroduces the maintainer-email default. Trivial to write; high value.
Expected size
1-2 briefs, ~18 files total (1 component, 17 pages). Single PR likely.
Architecture
File plan
| File | Action | Purpose |
|---|---|---|
components/Layout.js |
modified | Default user to null. Replace UserProfileDropdown's avatar+email+dropdown with a "Sign in" CTA (inline) when user === null. The other null-safe paths (NavigationContent's authenticatedNavigation, myCollectionNavigation, adminNavigation) already do the right thing today; do not touch them. |
vitest.config.js |
modified | No global change to environment; the new test file uses a per-file // @vitest-environment jsdom directive. The only edit here is bumping include: ['test/**/*.test.js'] if the implementer wants test files under test/components/ (already covered by the existing glob — verify only). |
package.json |
modified | Add jsdom and @testing-library/react to devDependencies so the new component test can render the JSX tree. No runtime deps. |
package-lock.json |
modified | Regenerated by npm install. |
test/components/Layout.test.js |
new | Locks in the contract: <Layout> (no prop) and <Layout user={null}> MUST NOT render me@randallstillwell.com; MUST render a "Sign in" affordance; <Layout user={…}> MUST render the supplied email. Negative regression against the maintainer-email default. |
pages/scanner.js |
modified | Pass user={user} to <Layout> (line 333). useAuth from lib/auth-context is already imported. |
pages/deck-builder.js |
modified | Pass user={user} to all four <Layout> calls. useAuth from lib/auth-context already imported. |
pages/deck/[id].js |
modified | Pass user={user} to all three <Layout> calls. useAuth from lib/auth-context already imported. |
pages/decks.js |
modified | Pass user={user} to all three <Layout> calls. useAuth from lib/auth-context already imported. |
pages/profile.js |
modified | Page-level useState initializer at line 10 hardcodes email: 'me@randallstillwell.com', role: 'admin'. Replace the initial value with null. Wrap user.* reads in ?. (the page already runs after loadUserProfile() resolves; the few sync reads need null-guards). Add useAuth() from lib/use-auth.js only if the implementer prefers a single source — but the simpler fix is useState(null) since the API call already overwrites the state. |
pages/settings.js |
modified | Same shape as profile.js: replace useState({ email: 'me@…', role: 'admin' }) with useState(null). The component already redirects to /login if no token (line 53-55), so the null window is the loading flash. |
pages/card/[id].js |
modified | Replace the hardcoded const user = { email: 'me@…', role: 'user' } (lines 13-16) with const { user } = useAuth() from lib/use-auth.js. This is a public-or-authenticated page (community card detail), so user === null is a legitimate state. |
The 10 remaining pages (pages/dashboard.js, pages/my-cards.js, pages/cards.js, pages/collection/[identifier].js, pages/community/collections.js, pages/collections.js, pages/invite/accept.js, pages/invite/decline.js, pages/admin/card-import.js, pages/admin/card-editor.js) already pass user correctly; no changes there. They are listed in the per-bucket assignment below for the audit record.
API surface
None. This is a frontend component change.
Schema diff
None.
Test plan
- Unit (new):
test/components/Layout.test.js(Decision D = D2). Per-file// @vitest-environment jsdom. Mocksnext/router(useRouter→{ pathname: '/' }) andlib/theme-context(useTheme→{ theme: 'light', toggleTheme: vi.fn() }). Asserts:<Layout>page</Layout>(nouserprop) renders a tree whosetextContentdoes not includeme@randallstillwell.com. (Direct regression on the bug shape.)<Layout user={null}>page</Layout>— same.<Layout user={null}>page</Layout>renders a "Sign in" affordance (getByText('Sign in')).<Layout user={{ email: 'foo@bar.com', role: 'user' }}>page</Layout>rendersfoo@bar.comin the navbar.- (Negative regression — explicit)
<Layout>page</Layout>'stextContentdoes not includeGuest(so we don't accidentally ship a "Guest" placeholder where Decision A says "Sign in" should live).
- No new tests for the page audit changes. The Layout test catches the fix at the component boundary; verifying every page individually would duplicate that contract. Manual smoke covers the page-level fixes.
- Existing tests (16 in
test/lib/permission-middleware.test.js, 3 intest/lib/auth-secret.test.js, 5 intest/api/auth-utils.test.js) must remain green. None of them touch UI; this convoy will not regress them. - Manual smoke (in PR description):
- With no
auth_tokeninlocalStorage(logged out), visit/scanner,/deck-builder,/decks,/deck/[any-id],/profile,/settings,/card/[any-id]. Confirm: navbar shows "Sign in" instead ofme@randallstillwell.com, no "Profile / Settings / Logout" dropdown, public nav items still visible. - With
auth_tokenset (logged in as a non-admin), visit each of the same pages. Confirm: navbar shows the real user's email, dropdown opens, "Profile / Settings / Logout" links visible, "Admin Panel" hidden. - Log in as admin. Confirm: "Admin Panel" link visible.
- Visit
/invite/accept?token=anythingand/invite/decline?token=anythingwhile logged out. Confirm: layout renders "Sign in", no maintainer email, no crashes (these pages explicitly passuser={null}and rely on the new branch).
- With no
Risks
- R1 — Loading-flash UX regression (low).
useAuth()returnsloading: truewithuser === nulluntil/api/auth/verifyresolves. During that flash (< 200ms in dev, typically faster in prod), Layout shows "Sign in" before swapping to the authenticated shape. Mitigation: This is the same flash thatProtectedRouteandAdminProtectedalready produce; their loading branches render<Layout user={null}>today (seecomponents/ProtectedRoute.jsline 29,components/AdminProtected.jsline 55). The fix keeps the existing UX contract; document in Brief 1 so the implementer doesn't try to "improve" it with a loading skeleton (out of scope). - R2 — Page-level useState initializer regressions (medium).
pages/profile.js,pages/settings.js, andpages/card/[id].jsinitialize a hardcoded user object. Replacing the initializer withnullmeans any sync code that readsuser.first_name,user.email,user.role, etc. before the API resolves now hits a null-deref. Mitigation: Brief 2 enumerates the sync reads per page and adds?./?? defaults. Vitest can't catch this at render time without a full mock harness; manual smoke is the gate. The implementer MUST exercise both the loading state and the loaded state on each of those three pages. - R3 —
user.role === 'admin'defaults flip (low).profile.jsandsettings.jsinitializerole: 'admin'. Today, a non-admin user briefly sees admin chrome (Admin Panel link) during the load flash. After the fix, that flash shows "Sign in" (R1) until the API resolves, then the correct role takes over. The "ghost admin" was a worse UX bug already; the fix improves it. No mitigation needed. - R4 —
UserProfileDropdownSign-in branch breaks the desktop+mobile-shared component (low). The dropdown is rendered twice (desktop sidebar + mobile drawer). Both must show "Sign in" when logged out. Mitigation: Brief 1 makes the change insideUserProfileDropdown(a single function) — both call sites pick up the new behavior automatically. - R5 —
vitesttest fragility (low). The test asserts on rendered text. If a future change moves "Sign in" to an icon-onlyaria-label,getByTextwill fail. Mitigation: Test againstgetByRole('link', { name: /sign in/i })so the assertion survives icon-only refactors. Brief 1 specifies this query shape. - R6 —
jsdom+@testing-library/reactadds devDeps (low). Two new packages (~25 transitive deps; devDep only). Mitigation: Brief 1 declares the additions explicitly infiles:and acceptance criteria; CI's existingtest:job will pick up the new test automatically (the glob already includestest/**/*.test.js). - R7 — Three parallel auth providers (existing, not introduced). 4 of the 7 pages we touch import
useAuthfromlib/auth-context.js(the legacy provider) instead of the canonicallib/use-auth.js. Brief 2 explicitly says "do not change the auth import — keep the existinguseAuthsource." Migrating these tolib/use-auth.jsis the queuedsingle-auth-providerconvoy's job. Mitigation: Brief 2 acceptance criteria includes a grep verification thatpages/scanner.js,pages/deck-builder.js,pages/deck/[id].js,pages/decks.jsstill import fromlib/auth-context. - R8 —
MobileNavigationreceivesuserbut ignores it (cosmetic, deferred).components/MobileNavigation.jsaccepts{ user, onMenuOpen }but does not access any field ofuser(the bottom-bar items are static). The prop is dead. Decision: leave it alone in this convoy; flag as a tiny cleanup follow-up. If the implementer is tempted to delete the prop, they MUST stop — that's god-component-split / single-auth-provider territory.
Decomposition
| Brief # | Title | Files | Depends on | Estimated PR size |
|---|---|---|---|---|
| 1 | Layout: default user to null + logged-out branch + vitest lock-in | components/Layout.js, test/components/Layout.test.js (new), package.json, package-lock.json |
none | ~80 LOC source change in Layout; ~60 LOC new test; package.json adds 2 devDeps |
| 2 | Pages: pass user explicitly + drop page-level maintainer-email defaults | pages/scanner.js, pages/deck-builder.js, pages/deck/[id].js, pages/decks.js, pages/profile.js, pages/settings.js, pages/card/[id].js |
1 | ~30-40 LOC across 7 files |
Brief 2 depends on Brief 1 because the page changes assume the new logged-out branch exists. Both ship in the same PR. (Decision C = C2.)
Slice dependencies (multitask-ready)
slice_dependencies:
- brief: 1
depends_on: []
files:
- components/Layout.js
- test/components/Layout.test.js
- package.json
- package-lock.json
- brief: 2
depends_on: [1]
files:
- pages/scanner.js
- pages/deck-builder.js
- pages/deck/[id].js
- pages/decks.js
- pages/profile.js
- pages/settings.js
- pages/card/[id].js
Briefs 1 and 2 do not share any files. Brief 2 depends on Brief 1 only because its acceptance criteria reference the new logged-out branch. The Conductor MUST sequence them (no /multitask parallel fan-out for this convoy).
Decisions
Decision A — Logged-out rendering branch design (was Q1)
A1 — replace user avatar + email + dropdown with a "Sign in" link to /login. Hide auth-only nav (already done by existing optional-chains). Public nav items still visible.
NavigationContent is already null-safe: authenticatedNavigation (line 140-142), myCollectionNavigation (line 145-157), and adminNavigation (line 167) all gate on user. The only piece that leaks the maintainer email is UserProfileDropdown's avatar (line 100-104), email/role labels (line 107-110), and dropdown menu items (line 11-18 — Profile/Settings/Admin/Logout linking to authenticated routes). Brief 1 short-circuits UserProfileDropdown with a <Link href="/login"> containing a "Sign in" label and a sign-in icon when user === null; the rest of the layout continues to work.
A2 (elaborate logged-out CTA with marketing copy) is out of scope per the convoy file ("Branding" → pick-a-name; "landing-page-rework" not yet queued). A3 (hide layout chrome entirely) breaks anonymous viewing on pages/invite/{accept,decline}.js, pages/cards.js (PublicCardsView), and any other public surface that legitimately renders Layout for non-authenticated users.
Decision B — Page audit triage (was Q2)
Per-page bucket assignment (17 pages):
| # | Page | Bucket | useAuth source |
Already passes user? |
Action |
|---|---|---|---|---|---|
| 1 | pages/scanner.js |
1 (Always-auth) | lib/auth-context |
No (<Layout> line 333) |
Fix — pass user={user} |
| 2 | pages/dashboard.js |
1 | lib/use-auth |
Yes | None |
| 3 | pages/my-cards.js |
1 | lib/use-auth |
Yes | None |
| 4 | pages/profile.js |
1 | none (uses local useState with maintainer-email init) |
Yes (passes the leaky useState user) | Fix — replace useState({email:'me@…'}) with useState(null) |
| 5 | pages/settings.js |
1 | none (same pattern as profile) | Yes (leaky) | Fix — same as profile |
| 6 | pages/deck-builder.js |
1 | lib/auth-context |
No (<Layout> ×4) |
Fix — pass user={user} to all four |
| 7 | pages/decks.js |
1 | lib/auth-context |
No (<Layout> ×3) |
Fix — pass user={user} to all three |
| 8 | pages/admin/card-import.js |
1 (admin) | none (gets user from AdminProtected render-prop) |
Yes | None |
| 9 | pages/admin/card-editor.js |
1 (admin) | none (same pattern as card-import) | Yes | None |
| 10 | pages/collections.js |
1 | lib/use-auth |
Yes | None |
| 11 | pages/cards.js |
2 (Public-or-auth) | lib/use-auth |
Yes (PublicCardsView passes user={null}; AuthenticatedCards passes user={user}) |
None |
| 12 | pages/card/[id].js |
2 | none (hardcoded const user = { email: 'me@…' }) |
Yes (passes the hardcoded const) | Fix — const { user } = useAuth() from lib/use-auth |
| 13 | pages/collection/[identifier].js |
2 | lib/use-auth |
Yes | None |
| 14 | pages/community/collections.js |
2 | lib/use-auth |
Yes | None |
| 15 | pages/deck/[id].js |
2 | lib/auth-context |
No (<Layout> ×3) |
Fix — pass user={user} to all three |
| 16 | pages/invite/accept.js |
3 (Anonymous-allowed) | none | Yes (user={null}) |
None |
| 17 | pages/invite/decline.js |
3 | none | Yes (user={null}) |
None |
Net: 7 pages need code changes (Brief 2). 10 pages are already correct.
A page-audit grep also surfaced 3 page-level maintainer-email leaks that the convoy file did not enumerate explicitly: pages/profile.js, pages/settings.js, and pages/card/[id].js initialize their user state/const with email: 'me@randallstillwell.com'. These are the same bug shape as the Layout default — the convoy's success metric ("No real email address remains in any component default-prop") is satisfied only if these are fixed too. Brief 2 covers them.
Decision C — Brief decomposition (was Q3)
C2 — two briefs, one PR.
Brief 1 ships the Layout change + vitest lock-in (the actual fix). Brief 2 ships the page audit (the cleanup that proves the fix is complete). Brief 2 depends on Brief 1 because its acceptance criteria reference the new logged-out branch.
C1 (single brief, 12 files) makes the diff harder to review — the Layout change is the conceptually interesting piece; lumping it with 7 mechanical page edits buries it. C3 (fan-out by bucket) creates 4 briefs without parallelization benefit, since Brief 2's pages don't share files with one another but DO all depend on Brief 1, so /multitask gives 4× the orchestration cost for the same wall-clock time.
Decision D — vitest coverage (was Q4)
D2 — yes, add test/components/Layout.test.js with the four assertions enumerated in the test plan.
The bug was a default-prop value that nobody caught for ~12 months. A single test that asserts "the rendered tree does not contain me@randallstillwell.com" makes the regression impossible to reintroduce silently. Cost: two new devDeps (jsdom, @testing-library/react), one ~60-LOC test file, per-file // @vitest-environment jsdom directive (no global vitest config change). Pattern matches test/lib/permission-middleware.test.js's negative-regression-against-old-shape style (the test for the synthetic-admin shape locks in P0 #1's fix).
D1 (no tests, manual smoke only) is what the original bug had. Manual smoke is human-attention-bottlenecked and not a regression gate.
Anything flagged but not acted on
MobileNavigation's unuseduserprop. The component accepts{ user, onMenuOpen }but never readsuser.*(the bottom-bar items are static — Cards, Decks, Dashboard, Community, More — none gated on auth state or role). The prop is dead. Removing it is a 2-line cleanup, but the convoy spec listsMobileNavigationas out-of-scope-or-fold-in territory and the right call here is to defer: a dead prop is harmless, and removing it touchesLayout.js(the call site) plusMobileNavigation.js, expanding the diff. Follow-up convoy:cleanup-mobile-nav-dead-props(P3 polish; can fold intogod-component-splitif that lands first).- Three parallel auth providers. Brief 2 leaves
pages/scanner.js,pages/deck-builder.js,pages/deck/[id].js,pages/decks.jsimportinguseAuthfromlib/auth-context(the legacy provider) instead oflib/use-auth. The convoy file explicitly defers this tosingle-auth-provider. Follow-up convoy:single-auth-provider(P1 #9, already queued in.convoys/ship-readiness.md). - Default branding in Layout headers. Lines 593-596 (mobile drawer header) and 686-689 (desktop sidebar header) render "DH" + "Deck Hearth". The user-prop fix does not touch branding. Follow-up convoy:
pick-a-name(P1 #12, already queued). pages/profile.jsandpages/settings.jsloadingstate design. Both pages render<Layout user={user}>whileloading === true. After Brief 2, this means<Layout user={null}>during loading → "Sign in" briefly visible to a logged-in user reloading the page. This matchesProtectedRoute's existing loading UX and is acceptable. If product wants to suppress the flash, that's a separate UX convoy (auth-loading-skeleton).AGENTS.mdGotcha #8. The convoy file says it will be marked RESOLVED in the post-convoy doc-writer pass. Do not editAGENTS.mdhere..cursor/rules/ui-and-theming.mdc§ Component conventions. The rule already documents the intent: "Avoid hardcoded default values foruserprops … default tonulland render a logged-out state." No rule update needed.