diff --git a/.planning/phases/12-initial-setup-wizard/12-07-SUMMARY.md b/.planning/phases/12-initial-setup-wizard/12-07-SUMMARY.md new file mode 100644 index 0000000..f32e1e1 --- /dev/null +++ b/.planning/phases/12-initial-setup-wizard/12-07-SUMMARY.md @@ -0,0 +1,127 @@ +--- +phase: 12-initial-setup-wizard +plan: 07 +subsystem: ui +tags: [react, tanstack-query, react-router, setup-wizard, pwa, oidc] + +# Dependency graph +requires: + - phase: 12-initial-setup-wizard + provides: "SetupPage wizard with alreadyLocked Surface 8; /setup route gate; first-login-claim in upsertUser; SetupBanner self-service onboarding (12-03/12-04)" +provides: + - "/setup route reverse-gated on setupComplete — Surface 8 ('Setup already complete') after completion, never re-mounts the wizard" + - "['me'] freshness fix (staleTime 0) so the post-wizard 'Set up your calendar' banner clears once the claimed credential is in effect" + - "SetupBanner.test.tsx regression coverage for the success-only dismissal contract + stale-cache refetch" +affects: [setup-wizard, onboarding, pwa-app-shell] + +# Tech tracking +tech-stack: + added: [] + patterns: + - "Reverse route-gate: conditional route element keyed on setupComplete/setupLoading mirroring the existing `*`-route gate" + - "staleTime 0 on a boot-critical ['me'] query so authenticated-shell entry always reflects fresh server truth (post-claim)" + +key-files: + created: + - apps/pwa/src/components/SetupBanner.test.tsx + modified: + - apps/pwa/src/App.tsx + - apps/pwa/src/App.test.tsx + - apps/pwa/src/components/SetupBanner.tsx + +key-decisions: + - "D-12-07-GAP6-MECH: gap 6 root cause is mechanism (ii) — ['me'] client-cache staleness, NOT a backend linking gap. upsertUser's first-login claim preserves users.id (where eq(users.id, unclaimed.id)), so the wizard-stored CalDAV credential stays linked and the DB reports needsProviderSetup=false. Fix is client-only." + - "D-12-07-STALE0: ['me'] staleTime set to 0 in both App.tsx (boot) and SetupBanner.tsx so a pre-claim stale entry is refetched on shell entry; success-only dismissal contract preserved (no dismiss/X button added)." + - "D-12-07-LOCKED-PROP: /setup reverse-gate uses SetupPage alreadyLocked prop (Surface 8) rather than Navigate, keeping the operator on /setup with a terminal surface per UAT expectation." + +patterns-established: + - "Reverse-gate a standalone route by swapping its element via the same loading/complete derivation used by the app-shell gate." + +requirements-completed: [SETUP-01, SETUP-04] + +# Metrics +duration: 11min +completed: 2026-06-16 +--- + +# Phase 12 Plan 07: UAT Gap-Closure (gaps 5 & 6) Summary + +**The /setup wizard no longer re-mounts after completion (shows Surface 8 'Setup already complete'), and the '/calendar' setup banner no longer nags the operator after they finish the wizard — fixed by reverse-gating the route and making the ['me'] query fresh on shell entry.** + +## Performance + +- **Duration:** ~11 min +- **Started:** 2026-06-16T21:19Z +- **Completed:** 2026-06-16T21:24Z +- **Tasks:** 2 +- **Files modified:** 4 (3 modified, 1 created) + +## Accomplishments + +- **Gap 5 closed:** `/setup` is now reverse-gated on `setupComplete`. After completion, manually visiting `/setup` renders `SetupPage alreadyLocked` → Surface 8 "Setup already complete" (the backend already 423s setup mutations; this is the matching frontend gate). Loading state renders a no-flash placeholder; `setupComplete===false` still mounts the active wizard. +- **Gap 6 closed:** the "Set up your calendar" banner no longer persists after the operator completes the wizard. Root cause confirmed as a `['me']` cache-staleness gap (mechanism ii), NOT a backend linking gap. Set `['me']` `staleTime` to 0 in both `App.tsx` (boot) and `SetupBanner.tsx` so a pre-claim stale entry is refetched on entry to the authenticated shell — `needsProviderSetup` then reflects the just-claimed credential and the banner hides. +- **Regression coverage added:** new `SetupBanner.test.tsx` (3 tests) + 2 new App reverse-gate tests. Full PWA suite green at 258 tests; typecheck clean. + +## Gap 6 — Root Cause Investigation (Task 2 Step A) + +The UAT `root_cause` was flagged PRELIMINARY. Reading `apps/api/src/auth/user.ts` `upsertUser` confirmed the first-login claim branch (lines ~118-130) updates the unclaimed row via `where(eq(users.id, unclaimed.id))` — it **preserves the same `users.id`**. Because `member_credentials` is keyed on `userId`, the wizard-stored CalDAV credential stays linked to the claimed operator row, so the DB correctly returns `needsProviderSetup=false` after first OIDC login. + +**Conclusion: mechanism (i) (a backend claiming/linking gap) is NOT the cause** — consistent with the 12-03 summary and threat-register disposition `T-12-10 = accept`. The cause is **mechanism (ii)**: `['me']` had `staleTime: 5 * 60 * 1000`, so a cache entry populated before the claim (e.g. a pre-auth visit) served `needsProviderSetup=true` for up to 5 minutes after the operator authenticated post-wizard. No backend change was made; the fix is purely client-side cache freshness. + +## Task Commits + +1. **Task 1: Gate the /setup route on setupComplete (gap 5)** — `fdcb4dc` (feat) + - (also carried the App.tsx `['me']` staleTime → 0 edit, staged together; the SetupBanner-side change + its test landed in Task 2) +2. **Task 2: Diagnose + fix the persistent calendar banner (gap 6)** — `2b3569f` (fix) + +## Files Created/Modified + +- `apps/pwa/src/App.tsx` — reverse-gated `/setup` route element (loading placeholder / `alreadyLocked` Surface 8 / active wizard); boot `['me']` `staleTime` → 0 with mechanism note. +- `apps/pwa/src/components/SetupBanner.tsx` — `['me']` `staleTime` 5min → 0 (gap-6 freshness); no dismiss button added; success-only dismissal contract restated in comments. +- `apps/pwa/src/App.test.tsx` — SetupPage mock now respects `alreadyLocked`; 2 new reverse-gate tests (already-complete surface + active wizard on `/setup`). +- `apps/pwa/src/components/SetupBanner.test.tsx` — NEW: banner absent when `needsProviderSetup=false`, present (no dismiss button) when true, and stale-cache refetch hides the banner on mount (staleTime 0). + +## Decisions Made + +- **D-12-07-GAP6-MECH** — gap 6 is mechanism (ii) `['me']` staleness, not a linking gap (evidence: id-preserving claim in `upsertUser`). +- **D-12-07-STALE0** — `['me']` `staleTime` set to 0 in App.tsx + SetupBanner.tsx; success-only dismissal contract preserved. +- **D-12-07-LOCKED-PROP** — `/setup` reverse-gate uses the `alreadyLocked` prop (Surface 8), not `Navigate`, per the UAT expectation that manual `/setup` navigation shows the "already complete" surface. + +## Deviations from Plan + +None — plan executed as written. The plan's expected gap-6 finding (mechanism ii) was confirmed by the Task 2 Step A investigation; the prescribed staleTime fix was applied. No architectural changes; no backend changes required. + +## Issues Encountered + +- **Pre-existing PWA lint errors (out of scope).** `pnpm lint` in `apps/pwa` reports 22 errors in `src/api/setupClient.contract.test.ts` (`no-unsafe-*`) and `src/routes/SetupPage.test.tsx:152` (`no-unused-vars`). Neither file was touched by this plan; both were last modified in earlier Phase-12 commits. The four files this plan touched lint clean (exit 0). Logged to `.planning/phases/12-initial-setup-wizard/deferred-items.md` and left untouched per the executor SCOPE BOUNDARY rule. Recommend a follow-up lint-cleanup quick task. + +## Verification + +- `apps/pwa` full suite: **258 tests passed (22 files)**; `App.test.tsx` 8 passed; `SetupBanner.test.tsx` 3 passed. +- `pnpm typecheck` (apps/pwa): clean. +- `grep -nE "alreadyLocked" apps/pwa/src/App.tsx` → `/setup` route gated on setupComplete (Surface 8). +- `grep` for new dismiss/X button in `SetupBanner.tsx` → none added (only contract comments). +- Touched-files lint: `eslint` over the 4 files → exit 0. + +## Known Stubs + +None. + +## User Setup Required + +None — no external service configuration required. + +## Next Phase Readiness + +- Both UAT major gaps (5 and 6) are closed in code with regression tests. Ready for re-UAT of the post-completion `/setup` surface and the post-wizard calendar banner. +- Remaining UAT gaps (if any from 12-05/12-06) are tracked in their own gap-closure plans; this plan scoped only gaps 5 & 6. +- Pre-existing PWA lint debt deferred (see deferred-items.md) — does not block this plan's UI behavior. + +## Self-Check: PASSED + +- All 5 created/modified files present on disk. +- All 3 commits (`fdcb4dc`, `2b3569f`, `96c4913`) present in git history. + +--- +*Phase: 12-initial-setup-wizard* +*Completed: 2026-06-16* diff --git a/.planning/phases/12-initial-setup-wizard/deferred-items.md b/.planning/phases/12-initial-setup-wizard/deferred-items.md new file mode 100644 index 0000000..0100354 --- /dev/null +++ b/.planning/phases/12-initial-setup-wizard/deferred-items.md @@ -0,0 +1,12 @@ +# Deferred Items — Phase 12 + +Out-of-scope discoveries logged during execution. Not fixed by the originating plan. + +## 12-07 — Pre-existing PWA lint errors (out of scope) + +Discovered during 12-07 verification (`pnpm lint` in apps/pwa). 22 errors, NOT introduced by 12-07 (the four files 12-07 touched lint clean): + +- `apps/pwa/src/api/setupClient.contract.test.ts` — `@typescript-eslint/no-unsafe-*` (any-typed `res.body` access in contract assertions) +- `apps/pwa/src/routes/SetupPage.test.tsx:152` — `no-unused-vars` (`container` assigned but unused) + +Both files were last modified in earlier Phase-12 commits (e.g. 066b69f), confirming pre-existing. Left untouched per the executor SCOPE BOUNDARY rule (only auto-fix issues directly caused by the current task). Recommend a follow-up lint-cleanup quick task. diff --git a/apps/pwa/src/App.test.tsx b/apps/pwa/src/App.test.tsx index 0acd7aa..496d4ed 100644 --- a/apps/pwa/src/App.test.tsx +++ b/apps/pwa/src/App.test.tsx @@ -25,8 +25,17 @@ vi.mock('./components/CalendarShell.js', () => ({ CalendarShell: () =>
CalendarShell
, })); +// SetupPage mock respects the `alreadyLocked` prop so the reverse-gate test (gap 5) +// can distinguish the active wizard (Step 1 "Welcome to FamilySync Setup") from the +// Surface 8 "Setup already complete" terminal surface. The real SetupPage renders +// these two surfaces based on this exact prop — see routes/SetupPage.tsx. vi.mock('./routes/SetupPage.js', () => ({ - SetupPage: () =>
SetupPage
, + SetupPage: ({ alreadyLocked }: { alreadyLocked?: boolean }) => + alreadyLocked ? ( +
Setup already complete
+ ) : ( +
Welcome to FamilySync Setup
+ ), })); vi.mock('./components/AppNav.js', () => ({ @@ -177,6 +186,41 @@ describe('App — setup-status gate', () => { expect(screen.getByTestId('setup-page')).toBeInTheDocument(); }); }); + + // gap 5 (T-12-04): manually visiting /setup AFTER setup is complete must show the + // "Setup already complete" surface (alreadyLocked), NOT re-mount the active wizard. + it('renders the "already complete" surface (not the wizard) on /setup when setupComplete is true', async () => { + mockFetchSetupStatus.mockResolvedValue({ setupComplete: true }); + // Navigate directly to /setup (URL-isolation pattern — beforeEach reset to /) + window.history.pushState({}, '', '/setup'); + + const queryClient = makeQueryClient(); + renderApp(queryClient); + + await waitFor(() => { + expect(screen.getByTestId('setup-page')).toHaveTextContent('Setup already complete'); + }); + + // The active wizard's Step 1 heading must NOT render when setup is complete + expect(screen.queryByText('Welcome to FamilySync Setup')).toBeNull(); + // Standalone wizard surface — no AppNav shell on /setup + expect(screen.queryByTestId('app-nav')).toBeNull(); + }); + + // gap 5 counterpart: /setup with setupComplete false still mounts the active wizard. + it('renders the active wizard on /setup when setupComplete is false', async () => { + mockFetchSetupStatus.mockResolvedValue({ setupComplete: false }); + window.history.pushState({}, '', '/setup'); + + const queryClient = makeQueryClient(); + renderApp(queryClient); + + await waitFor(() => { + expect(screen.getByTestId('setup-page')).toHaveTextContent('Welcome to FamilySync Setup'); + }); + + expect(screen.queryByText('Setup already complete')).toBeNull(); + }); }); describe('App — setupStatus and route presence', () => { diff --git a/apps/pwa/src/App.tsx b/apps/pwa/src/App.tsx index 1cf1e3f..b641ea5 100644 --- a/apps/pwa/src/App.tsx +++ b/apps/pwa/src/App.tsx @@ -81,11 +81,21 @@ export default function App() { // Fetch current user once at the app shell level so AppNav has member data on // ALL routes. This is the same query key (['me']) used by CalendarShell, so // TanStack Query deduplicates the request — no double fetch. + // + // gap 6 (mechanism (ii) — ['me'] staleness, NOT a linking gap; see SUMMARY): + // the first-login claim in upsertUser (auth/user.ts) preserves the same users.id, + // so the wizard-stored CalDAV credential stays linked → needsProviderSetup is + // correctly FALSE in the DB after the operator authenticates post-wizard. The bug + // was purely client-cache: a ['me'] entry populated BEFORE the claim (e.g. an + // earlier pre-auth visit) served a stale needsProviderSetup=true for up to 5 + // minutes, so the "Set up your calendar" banner kept showing. Set staleTime 0 on + // the boot ['me'] query so the authenticated app shell always refetches member + // status on entry — needsProviderSetup then reflects the just-claimed credential. const meQuery = useQuery({ queryKey: ['me'], queryFn: fetchMe, retry: false, - staleTime: 5 * 60 * 1000, + staleTime: 0, }); // isAdmin from /api/me — used for UX gating only (D-03). Server enforces 403. @@ -135,8 +145,26 @@ export default function App() { return ( - {/* /setup route — standalone wizard, no AppNav/BottomTabBar shell (UI-SPEC §Routing) */} - } /> + {/* /setup route — standalone wizard, no AppNav/BottomTabBar shell (UI-SPEC §Routing). + Reverse gate (gap 5, T-12-04): once setup is complete the wizard must NOT re-mount. + - While setupQuery is loading → render the no-flash placeholder (no wizard before + status resolves), mirroring the `*`-route loading gate below. + - setupComplete === true → render SetupPage with alreadyLocked → Surface 8 + ("Setup already complete"), keeping the operator on /setup with a terminal surface. + - setupComplete === false (or undefined post-load) → active wizard, as before. + The backend already 423s setup mutations; this is the matching frontend reverse-gate. */} +