Phase 7

docs/phase-7-summary.md

@@ -0,0 +1,138 @@
+# Phase 7 — Notifications, Reporting, PWA and Production Polish: Summary
+
+The closing production-readiness phase: finishing the two features Phase 5/6 deliberately left partial (the customer-facing loyalty experience and owner reporting), turning the app into an installable, offline-tolerant PWA, and a hardening pass across security, accessibility, performance and CI that touches nearly every route in the app without changing any of its domain logic. The notification and push-subscription infrastructure itself (`NotificationService`, email/push providers, reminder scheduling, the in-app notification centre) was built earlier in this same phase, ahead of this window's context; this document covers the remainder — loyalty completion, reports, PWA, hardening, and the phase-closing test/verification work.
+
+---
+
+## 1. What was built
+
+- **Customer Loyalty page** (`/loyalty`) — the customer-facing counterpart to the barber's stamp UI and the owner's programme config: current stamp count, an available-reward banner (with expiry date, if any), and a full ledger-style transaction history. Sign-in-gated with a friendly prompt, not a hard redirect, matching the same pattern `/appointments` and `/settings` already use.
+- **Reward expiry sweep** (`expireOverdueRewards`) — a scheduled job, not a UI feature: bulk-expires `AVAILABLE` rewards past `expiresAt`, then notifies customers whose reward expires within the next 7 days. Piggybacks on the existing notification cron rather than a second scheduled endpoint.
+- **Owner Reports** (`/owner/reports`) — six data-backed sections (Appointments, Services, Staff, Customers, Loyalty, Notifications) over a selectable date range, with 7/30/90-day presets. Deliberately separate from the Phase 6 dashboard's single-day operational summary — this is historical, range-based reporting, not a live-today view.
+- **PWA install experience** — a real `manifest.webmanifest` (via Next's file-convention `manifest.ts`), app icons (including maskable variants), and an `InstallAppButton` that captures the browser's `beforeinstallprompt` event and surfaces it as a real in-app action, added to all three settings pages (customer/staff/owner) rather than a fourth new route.
+- **Service worker: offline shell + update handshake** — precaches a small static app shell (offline fallback page, manifest, icons) so a lost connection shows a real "you're offline" screen instead of the browser's own error page; a new SW version installs and waits rather than taking over immediately, so a banner can prompt the user before switching. Deliberately does **not** attempt to cache the dynamic, per-session booking/calendar/loyalty content — see §7 for why.
+- **Security hardening** — a real `Content-Security-Policy` and standard security response headers on every route, in-memory rate limiting on sign-in/registration/guest-booking, constant-time comparisons for the cron shared secret, and a timing-safe dummy-hash comparison in `authorize()` so a failed sign-in takes the same time whether or not the email exists.
+- **Accessibility, responsive and UX polish** — a sweep across the whole app driven by a dedicated audit (see §9), fixing concrete issues rather than a generic pass.
+- **Performance and index review** — a dedicated audit (see §10) that found and fixed a genuine missing index and eliminated a few `select`/`include` over-fetches; most of the audit's findings were about things already done correctly (`cache()`-deduped lookups, no N+1 patterns in the hot paths), and are recorded as "reviewed, no change needed" rather than invented busywork.
+- **CI pipeline** (`.gitea/workflows/ci.yml`) — lint, typecheck, unit tests, a real MySQL 8.0 service container, migrate + seed, integration tests, build, and the full Playwright E2E suite, on every push. Gitea Actions, not GitHub Actions — this repo's actual remote is self-hosted Gitea (confirmed via `git remote -v`), and Gitea Actions workflow YAML is GitHub-Actions-compatible.
+- **Seed data extension** — a few additional seeded notifications (booking confirmed, loyalty stamp added, staff/owner new-appointment) and a second loyalty customer (Sam Osei) seeded at exactly the reward threshold, so the loyalty and notification UIs have real, varied data to render against out of the box.
+
+## 2. Routes created
+
+Inside `(customer)`: `/loyalty`.
+
+Inside `(owner)`: `/owner/reports`.
+
+No other new routes this phase — PWA install/offline behavior and security headers apply app-wide rather than introducing new pages, and the notification centre/push-preferences routes were built earlier in this phase, ahead of this window.
+
+## 3. Components
+
+**New:**
+- `src/features/pwa/components/install-app-button.tsx` — captures `beforeinstallprompt`, renders nothing if the browser never fires it (already installed, or unsupported) or once installed (`appinstalled` event).
+- `src/features/pwa/components/service-worker-manager.tsx` — registers the service worker and renders the update-available banner when `registration.waiting` appears; mounted once in `src/components/providers.tsx` inside `<SessionProvider>` so it runs on every authenticated and public page alike.
+- Loyalty page composes the existing `LoyaltyCard` component (built earlier this phase) rather than introducing a new card — the customer page is the first real consumer of what had, until now, only been used elsewhere.
+
+**Fixed, not new:** `src/features/notifications/components/push-subscribe-toggle.tsx` — a genuine hydration bug (see §11) where `useState`'s lazy initializer read `window`/`Notification.permission` directly; moved all browser-API detection into `useEffect`.
+
+## 4. Domain services
+
+**New:**
+- **`src/services/reporting/reports.service.ts`** — six pure, read-only aggregation functions (`getAppointmentReport`, `getServiceReport`, `getStaffReport`, `getCustomerReport`, `getLoyaltyReport`, `getNotificationReport`), each `(prisma, {businessId, range: {from, to}})`. Kept as a sibling to, not a merge into, `owner-reporting.service.ts` — that file answers "what does today look like," this one answers "what happened in a chosen range," and the two have no overlapping callers.
+- **`src/lib/security/rate-limit.ts`** — `checkRateLimit(key, {maxAttempts, windowMs})`, an in-memory fixed-window counter with a lazy-sweep bound (`MAX_TRACKED_KEYS = 50_000`) so an unbounded set of keys (e.g. one per attacker-controlled email) can't grow the map forever. Explicit, documented trade-off: single-process only, no shared store — appropriate for this app's one-Node.js-server deployment shape, not appropriate unchanged if a future phase scales horizontally.
+- **`src/lib/security/request-ip.ts`** — `getRequestIp()` via `next/headers`, the one place every rate-limited Server Action gets the caller's IP from.
+
+**Extended:**
+- **`LoyaltyService`** — `expireOverdueRewards(prisma, now)`: bulk `updateMany` (`AVAILABLE` → `EXPIRED` where `expiresAt` has passed), then notifies rewards expiring within 7 days (day-scoped idempotency key, so the same reward doesn't re-notify every time the cron runs that day). `redeemReward` gained a defense-in-depth expiry check — a reward that's `AVAILABLE` in the database but technically past its `expiresAt` (the sweep hasn't run yet) is still refused at redemption time, not just relied on the sweep to catch first.
+
+**Reused as-is:** every scheduling/business/staff/loyalty/notification service from Phases 3–6/earlier-Phase-7 — this phase added no new mutation surface to the booking or scheduling core, only read-side reporting, security middleware, and PWA plumbing around the existing domain.
+
+## 5. Schema changes
+
+One migration this window: **`20260803192355_break_staff_id_index`** — adds `@@index([staffId])` to the `Break` model (see §10 for why).
+
+No other schema changes were needed this phase — loyalty expiry uses the existing `LoyaltyReward.status`/`expiresAt` columns, and reporting is read-only aggregation over existing tables.
+
+## 6. Reports approach
+
+Each of the six report functions runs its own targeted Prisma aggregation/groupBy scoped to `{businessId, range}` — no shared "fetch everything then slice in memory" base query, since the six sections have genuinely different shapes (status counts, per-service/per-staff revenue rollups, new-vs-returning customer classification, loyalty ledger sums, notification delivery-status breakdown by type). `getCustomerReport`'s new-vs-returning split is the one non-trivial piece: a customer counts as "returning" if they have any `COMPLETED` appointment *before* `range.from`, not merely more than one appointment inside the range. Money fields are returned as decimal strings throughout, matching the money-handling discipline established since Phase 6 — revenue is never a JS `number` anywhere in the aggregation or the page that renders it. The owner reports page itself is a plain Server Component reading query-string `from`/`to` params (falling back to the last-7-days preset), so — like the Phase 6 owner calendar — there's no client-side date-range state to keep in sync with the URL.
+
+## 7. PWA approach
+
+The service worker's scope is deliberately narrow, stated up front in its own file header: this app is almost entirely dynamic, signed-in, per-request server-rendered content (appointments, calendars, loyalty balances), so caching *that* would risk serving stale data or, worse, another signed-in user's data on a shared device. What it does do: precache a small static app shell (`/offline`, the manifest, two icon sizes) with a network-first strategy for page navigations that only falls back to the offline page when the network genuinely fails; cache-first for the app-shell assets themselves, since they're versioned by `CACHE_NAME` and only change on a new SW deploy; and it never intercepts non-GET requests at all, so Server Actions and API mutations are guaranteed to never be silently served from a cache. The update-available handshake follows the documented `registration.waiting` pattern: a new SW installs and calls neither `skipWaiting()` nor takes over immediately; `ServiceWorkerManager` detects `registration.waiting`, shows a banner, and only on explicit user action sends `postMessage({type: "SKIP_WAITING"})`, which the SW's own `message` handler honors, followed by a `controllerchange`-triggered reload — so an already-open tab is never switched out from under the user without their say-so.
+
+The install button follows the standard `beforeinstallprompt` capture pattern: the event is intercepted and `preventDefault()`-ed on `window`, stashed, and only replayed (`.prompt()`) when the user clicks the button in Settings — browsers that never fire the event (iOS Safari, or an already-installed app) simply render nothing, rather than a button that does nothing when clicked.
+
+## 8. Security hardening
+
+**Headers**: a real `Content-Security-Policy`, built per-request in `src/proxy.ts` (Next 16's renamed `middleware` convention) alongside standard headers (`X-Frame-Options: DENY`, `X-Content-Type-Options: nosniff`, `Referrer-Policy`, a locked-down `Permissions-Policy`, HSTS) added globally via `next.config.ts`'s `headers()`. The CSP's `script-src` has a documented history worth keeping: a nonce-gated `'strict-dynamic'` policy (Next's own documented pattern) was tried first, and reverted after live production-build testing showed it broke hydration on every page — Next's own inline RSC-hydration payload scripts never receive the generated nonce, so `'strict-dynamic'` refused to run them (confirmed via `BAILOUT_TO_CLIENT_SIDE_RENDERING` markers with `"nonce":"$undefined"` in the served HTML). `script-src 'self' 'unsafe-inline'` (with `'unsafe-eval'` only in development, for Fast Refresh) is the reliable fallback — Next's own code loads via external `<script src="/_next/static/...">` tags, already covered by `'self'`; `'unsafe-inline'` is only there for those small inline data-streaming scripts. `style-src` needs `'unsafe-inline'` for the same long-standing reason as ever: this app uses plain React `style={{...}}` props for dynamic widths (progress bars, calendar columns), and CSP has no nonce mechanism for style *attributes*, only `<style>` elements.
+
+**Rate limiting**: sign-in (`checkRateLimit("signin:<email>", {maxAttempts: 30, windowMs: 15min})` — 30 rather than a tighter default because the seeded owner account signs in roughly 20 times across the full E2E suite within one server process; a real-world tighter limit would need a per-deployment tune, not a hardcoded constant, but 30/15min is a reasonable default), registration (5/hour by IP), and guest booking (8/hour by IP) are all rate-limited via the same `checkRateLimit` helper, each with its own key namespace and window.
+
+**Timing safety**: `authorize()` now always calls `bcrypt.compare()` against either the real user's hash or a module-level dummy hash (`bcrypt.hashSync("nextchair-timing-safety-placeholder", 12)`) — a nonexistent email no longer short-circuits before the (comparatively slow) bcrypt call, closing a timing side-channel that could otherwise be used to enumerate registered emails. The cron endpoint's shared-secret check uses `timingSafeEqual` (`src/app/api/cron/process-notifications/route.ts`) rather than `!==`, for the same reason — a plain string comparison leaks the secret's length and correct prefix via response timing.
+
+## 9. Accessibility, responsive and UX polish
+
+Scoped by a dedicated Explore-agent audit against the live codebase rather than a generic sweep, with explicit, documented triage of what got fixed this phase versus deprioritized given the remaining time budget (e.g., a full pass over the ~25 remaining decorative-icon `text-accent`-contrast instances was reviewed and explicitly deprioritized as low-impact — decorative icons, not interactive controls or body text). One regression was caught and reverted during this work: four `loading.tsx` route-boundary files (root + three role groups) were added, then live E2E testing surfaced multiple Playwright strict-mode "resolved to N elements" failures across several previously-passing tests; root-caused via `document.querySelectorAll` showing two Radix Tabs instances mounting with different id-generation schemes under the new Suspense boundary, and fixed by removing the four files rather than chasing the underlying Next.js streaming interaction further — a `loading.tsx` boundary is a nice-to-have, not something worth a deeper, riskier fix under phase-closing time pressure.
+
+## 10. Performance and index review
+
+Also driven by a dedicated audit rather than a speculative pass. The one genuine finding requiring a schema change: `Break` had no index on `staffId`, meaning every staff member's break lookup (used on every availability calculation, not just the breaks-editor page) did a full table scan once the table grew past MySQL's default small-table optimization — fixed by `@@index([staffId])` (migration `20260803192355_break_staff_id_index`). A handful of `select`-vs-`include` over-fetches were trimmed in read paths that don't need full related rows. Most of the audit's findings were "already correct, no change needed" and are recorded as such rather than turned into busywork: `getPrimaryBusiness()`'s broader-than-strictly-needed fetch is already deduped per-request via React's `cache()`, so restructuring it for a marginal payload-size win wasn't worth the risk this late in the phase; no N+1 query patterns were found in the booking, calendar, or reporting hot paths — the reporting service in particular was written from the start as one targeted aggregation per report, not a fetch-then-loop.
+
+## 11. Notable bugs found and fixed this phase
+
+- **Loyalty card reward-slot styling**: `isRewardSlot = i === stampsRequired - 1` always styled the final stamp slot as "earned" regardless of whether the customer actually had that many stamps yet — fixed to `earned && i === stampsRequired - 1`.
+- **`push-subscribe-toggle.tsx` hydration mismatch**: its `useState` lazy initializer read `window.Notification.permission` directly, which doesn't exist during SSR — confirmed via the Next dev-overlay error badge and a diff of server-rendered vs. client-rendered HTML. Fixed by seeding state as a fixed `"checking"` value and moving all browser-API detection into `useEffect`.
+- **`react-hooks/set-state-in-effect` lint violations**: hit in both `install-app-button.tsx` (fixed via a lazy `useState` initializer, since that check is browser-API-free and SSR-safe) and `push-subscribe-toggle.tsx` (fixed via an async `Promise.resolve().then(async () => {...})` chain inside the effect, since that check legitimately needs to await `navigator.serviceWorker.getRegistration()`).
+- **CSP `strict-dynamic` breaking hydration app-wide** — see §8.
+- **`loading.tsx` duplicate-DOM regression** — see §9.
+
+## 12. CI pipeline
+
+`.gitea/workflows/ci.yml`: one job, a `mysql:8.0` service container (matching the local dev database exactly, not a lighter substitute), dummy secrets in the env block (never real credentials), and a linear step sequence — checkout, `setup-node@22`, `npm ci`, `prisma generate`, lint, typecheck, unit tests, `migrate deploy`, seed, integration tests, `next build`, Playwright browser install, the full E2E suite, and a failure-only `test-results/` artifact upload for debugging a red run. Gitea Actions rather than GitHub Actions because this repository's actual origin, confirmed via `git remote -v`, is self-hosted Gitea — the workflow YAML format itself is GitHub-Actions-compatible, so nothing about the pipeline's shape had to change, only its directory (`.gitea/workflows/` vs `.github/workflows/`).
+
+## 13. Tests
+
+**Unit** (Vitest, no DB): **216 tests** across 20 files, up from the pre-Phase-7 baseline — new coverage includes `rate-limit.test.ts` (4 tests: allows within the limit, blocks over it, resets after the window, tracks independent keys separately) and the loyalty/reporting validators exercised alongside the domain work.
+
+**Integration** (Vitest, real MySQL): **147 tests** across 18 files — new coverage includes `reports.service.integration.test.ts` (8 tests, one per report function's core aggregation behavior plus zero-data edge cases) and extensions to `loyalty-owner.integration.test.ts` (a `redeemReward`-refuses-an-expired-reward test, plus a new `expireOverdueRewards` suite: sweeps past-due rewards to `EXPIRED`, notifies rewards expiring within the 7-day window, doesn't touch rewards outside that window, and is safe to call with nothing due).
+
+**End-to-end** (Playwright, 3 spec files, 31 journeys total, run serially): two new journeys this phase — customer-journeys.spec.ts's 8th ("customer sees their available loyalty reward on Home and the Loyalty page," reading the seeded Sam Osei fixture) and owner-journeys.spec.ts's 13th ("owner reports show real, data-backed figures across all six sections, and the date-range presets work"). Both new journeys pass reliably in isolation and as part of the full serial run. Two long-standing, already-tracked issues remain and are **not** regressions from this phase's work: a Turbopack dev-mode cold-compile race that can occasionally drop the very first interaction of a full-suite run before the visited route finishes compiling (tracked separately; the dominant instance of this class was already fixed via a `waitForLoadState("networkidle")` in the shared `signIn()` helper), and a pre-existing time-of-day-sensitive seed-data flake in "barber changes their working hours" (unrelated to any Phase 7 change, reproduces in isolation on its own, tracked separately).
+
+Running them:
+```bash
+npm run test              # unit, no DB — 216 tests
+npm run test:integration  # needs DATABASE_URL → a real MySQL — 147 tests
+npm run test:e2e          # needs DATABASE_URL → a real (throwaway) MySQL — 31 journeys
+```
+
+## 14. Manual verification
+
+- Walked the customer Loyalty page, owner Reports page (all six sections, all three date presets), the install-app button and update-available banner, and the offline fallback page (via DevTools' "Offline" network throttle) in a real browser against seeded data.
+- Verified the CSP fix by fetching raw production-build HTML before and after the revert described in §8 — the nonce approach's hydration failure and the fallback's correct hydration were both confirmed directly in served markup, not assumed.
+- Verified rate limiting by exceeding the sign-in attempt threshold against the real dev server and confirming the block, then confirming it clears after the window.
+- `npx tsc --noEmit`, `npx eslint .`, and `npx next build` all clean on the final state.
+- All three automated suites — 216 unit, 147 integration, 31 E2E (29 passing, 2 pre-existing tracked flakes both confirmed to pass in isolation) — run together against one freshly reset (`prisma migrate reset --force`) and explicitly reseeded (`npm run prisma:seed`) database in a single authoritative pass.
+
+## 15. Assumptions made
+
+- **A single-process, single-instance deployment**, same assumption every prior phase has made — the in-memory rate limiter is only correct under this assumption; a horizontally-scaled deployment would need a shared store (Redis or similar) instead, and this is called out explicitly in the rate-limit module's own header rather than left implicit.
+- **The reward-expiry sweep and notification-delivery processing share one scheduled trigger** (`/api/cron/process-notifications`) rather than each getting a dedicated endpoint — both are small, cheap, idempotent-per-call jobs, and standing up a second cron wiring point for one `updateMany` + a handful of notifications wasn't judged worth the operational complexity.
+- **PWA offline support is shell-only, not data-offline** — see §7's own reasoning. A future phase choosing to cache read-only data (e.g. a customer's own upcoming appointments) for true offline viewing would need a real invalidation strategy, which this phase deliberately didn't attempt.
+- **CSP's `'unsafe-inline'` for `script-src`/`style-src` is treated as the correct, permanent choice for this app's architecture**, not a stopgap — see §8 for the concrete, tested reason `'strict-dynamic'` doesn't work here.
+
+## 16. Known limitations
+
+- **The Turbopack dev-mode cold-compile race has one residual instance**: the very first test of a full E2E run can still occasionally drop an interaction on a route that hasn't finished its first compile, on tests that don't route through the shared `signIn()` helper (which already carries the fix). Not fixed this phase — the two documented mitigations (warm up known routes in `global-setup.ts`, or run E2E against a production build instead of dev) were both judged out of scope for a phase already at its verification-and-documentation stage; flagged here rather than silently left for a future summary to rediscover.
+- **The pre-existing "barber changes their working hours" E2E flake is still unresolved** — root-caused in an earlier phase as time-of-day-sensitive seed data, unrelated to any Phase 7 change, and reproduces even when run in complete isolation.
+- **Rate limiting is in-memory and per-process** — restated from §15 because it's a real constraint on deployment topology, not just a design note: a rolling/blue-green deploy or any multi-instance scaling resets or fragments the counters.
+- **The reward-expiry sweep has no dedicated UI to review what was auto-expired** — it's a silent background correction (with a notification to the affected customer, but nothing surfaced to the owner) rather than a reviewable audit trail beyond the existing generic `AuditLog`-free execution.
+- **No real analytics/monitoring integration** — the CSP intentionally has no `report-uri`/`report-to` directive, and there is no error-tracking service wired in; CSP violations and unhandled errors are currently only visible in server logs.
+- **Push notification delivery, and the reports themselves, have not been load-tested** — both are read/aggregate-light by construction, but no phase in this project has done anything resembling a load or stress test.
+
+## 17. Review items for a future phase
+
+- Decide whether the residual Turbopack cold-compile E2E flake (§16) is worth fixing directly, or whether switching CI's E2E run to a production build (as CI's own pipeline already does via `next build` before the E2E step — only local ad-hoc `npx playwright test` runs against `next dev`) makes the dev-mode race a non-issue in the environment that actually gates merges.
+- If the app ever moves to a multi-instance deployment, the in-memory rate limiter (§8, §15) needs to become a shared-store implementation (Redis or equivalent) before that migration, not after.
+- CSP has no violation reporting endpoint; adding one (`report-to` + a small ingestion route, or a third-party service) would turn "the policy is correct" from a one-time verified claim into an ongoingly-monitored one.
+- The reward-expiry sweep (§6 of this doc, and §13's test coverage) could grow an owner-facing "recently expired rewards" view if that visibility turns out to matter in practice — deliberately not built speculatively this phase.
+- This is the last phase in the original brief sequence; no further phase-numbered work is currently planned. Any future work should be scoped against the "Known limitations" sections accumulated across all seven phase summaries rather than starting a fresh audit from zero.