deckhearth/.convoys/fix-layout-default-user.md
Randall Stillwell f666a885b9 chore(convoys): mark shipped convoys and refresh ship-readiness
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>
2026-06-02 08:48:42 -05:00

24 KiB
Raw Blame History

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".
role-design-system-auditor
role-ia-architect
browser-smoke
shipped 2026-05-23 ship-readiness P0
bump-next-js (shipped)
fix-auth-bypass (shipped)
drop-public-setup (shipped)

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 the Layout({ children, user = {...} }) default to user = null. Add a logged-out rendering branch (navbar / profile dropdown / mobile menu) that handles user === null cleanly — typically "Sign in" CTA replacing the user avatar + dropdown.
  • 17 pages in pages/** that import Layout (confirmed via grep):
    pages/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.js
    
    For each: confirm it passes user explicitly 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-name convoy (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 queued single-auth-provider convoy (P1 #9).
  • God-component splitcomponents/Layout.js is 700+ lines. Splitting is the queued god-component-split convoy (P2 #13). Touch only the user- prop default and the logged-out rendering branch in THIS convoy.
  • MobileNavigation — receives user from 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.md Gotcha #8 — will be marked RESOLVED in the post-convoy doc-writer pass; do not pre-emptively edit AGENTS.md here.

Architect's questions

  1. 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?
  2. Page audit triage. For each of the 17 pages, three buckets:

    • Always-authenticated (dashboard, my-cards, profile, settings, scanner, admin/*) — must pass user explicitly; pages without it should add it via useAuth().
    • 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.

  3. 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).

  4. 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 Layout renders the logged-out shape when user === undefined and user === null would 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. Mocks next/router (useRouter{ pathname: '/' }) and lib/theme-context (useTheme{ theme: 'light', toggleTheme: vi.fn() }). Asserts:
    1. <Layout>page</Layout> (no user prop) renders a tree whose textContent does not include me@randallstillwell.com. (Direct regression on the bug shape.)
    2. <Layout user={null}>page</Layout> — same.
    3. <Layout user={null}>page</Layout> renders a "Sign in" affordance (getByText('Sign in')).
    4. <Layout user={{ email: 'foo@bar.com', role: 'user' }}>page</Layout> renders foo@bar.com in the navbar.
    5. (Negative regression — explicit) <Layout>page</Layout>'s textContent does not include Guest (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 in test/lib/auth-secret.test.js, 5 in test/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_token in localStorage (logged out), visit /scanner, /deck-builder, /decks, /deck/[any-id], /profile, /settings, /card/[any-id]. Confirm: navbar shows "Sign in" instead of me@randallstillwell.com, no "Profile / Settings / Logout" dropdown, public nav items still visible.
    • With auth_token set (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=anything and /invite/decline?token=anything while logged out. Confirm: layout renders "Sign in", no maintainer email, no crashes (these pages explicitly pass user={null} and rely on the new branch).

Risks

  • R1 — Loading-flash UX regression (low). useAuth() returns loading: true with user === null until /api/auth/verify resolves. 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 that ProtectedRoute and AdminProtected already produce; their loading branches render <Layout user={null}> today (see components/ProtectedRoute.js line 29, components/AdminProtected.js line 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, and pages/card/[id].js initialize a hardcoded user object. Replacing the initializer with null means any sync code that reads user.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.js and settings.js initialize role: '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 — UserProfileDropdown Sign-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 inside UserProfileDropdown (a single function) — both call sites pick up the new behavior automatically.
  • R5 — vitest test fragility (low). The test asserts on rendered text. If a future change moves "Sign in" to an icon-only aria-label, getByText will fail. Mitigation: Test against getByRole('link', { name: /sign in/i }) so the assertion survives icon-only refactors. Brief 1 specifies this query shape.
  • R6 — jsdom + @testing-library/react adds devDeps (low). Two new packages (~25 transitive deps; devDep only). Mitigation: Brief 1 declares the additions explicitly in files: and acceptance criteria; CI's existing test: job will pick up the new test automatically (the glob already includes test/**/*.test.js).
  • R7 — Three parallel auth providers (existing, not introduced). 4 of the 7 pages we touch import useAuth from lib/auth-context.js (the legacy provider) instead of the canonical lib/use-auth.js. Brief 2 explicitly says "do not change the auth import — keep the existing useAuth source." Migrating these to lib/use-auth.js is the queued single-auth-provider convoy's job. Mitigation: Brief 2 acceptance criteria includes a grep verification that pages/scanner.js, pages/deck-builder.js, pages/deck/[id].js, pages/decks.js still import from lib/auth-context.
  • R8 — MobileNavigation receives user but ignores it (cosmetic, deferred). components/MobileNavigation.js accepts { user, onMenuOpen } but does not access any field of user (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) Fixconst { 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 unused user prop. The component accepts { user, onMenuOpen } but never reads user.* (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 lists MobileNavigation as out-of-scope-or-fold-in territory and the right call here is to defer: a dead prop is harmless, and removing it touches Layout.js (the call site) plus MobileNavigation.js, expanding the diff. Follow-up convoy: cleanup-mobile-nav-dead-props (P3 polish; can fold into god-component-split if that lands first).
  • Three parallel auth providers. Brief 2 leaves pages/scanner.js, pages/deck-builder.js, pages/deck/[id].js, pages/decks.js importing useAuth from lib/auth-context (the legacy provider) instead of lib/use-auth. The convoy file explicitly defers this to single-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.js and pages/settings.js loading state design. Both pages render <Layout user={user}> while loading === true. After Brief 2, this means <Layout user={null}> during loading → "Sign in" briefly visible to a logged-in user reloading the page. This matches ProtectedRoute's existing loading UX and is acceptable. If product wants to suppress the flash, that's a separate UX convoy (auth-loading-skeleton).
  • AGENTS.md Gotcha #8. The convoy file says it will be marked RESOLVED in the post-convoy doc-writer pass. Do not edit AGENTS.md here.
  • .cursor/rules/ui-and-theming.mdc § Component conventions. The rule already documents the intent: "Avoid hardcoded default values for user props … default to null and render a logged-out state." No rule update needed.