From 85bd0d82e138813d462f44c7bce862a56cea2998 Mon Sep 17 00:00:00 2001 From: Matt Date: Thu, 14 May 2026 22:38:02 +0200 Subject: [PATCH] docs: capture post-audit fix plan from two-round Playwright sweep 24 fixes + 1 new feature, tiered by priority. T0 already shipped in the previous commit; T1-T4 batches sequenced with effort estimates and file pointers. Includes the manual-berth-status catch-up workflow design. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/POST-AUDIT-FIX-PLAN.md | 305 ++++++++++++++++++++++++++++++++++++ 1 file changed, 305 insertions(+) create mode 100644 docs/POST-AUDIT-FIX-PLAN.md diff --git a/docs/POST-AUDIT-FIX-PLAN.md b/docs/POST-AUDIT-FIX-PLAN.md new file mode 100644 index 00000000..5cf8ef53 --- /dev/null +++ b/docs/POST-AUDIT-FIX-PLAN.md @@ -0,0 +1,305 @@ +# Post-Audit Fix Plan + +Generated 2026-05-14 from two rounds of deep Playwright + API audit on `feat/documents-folders` → `main`. + +**Total findings:** 24 fixes + 1 new feature. Grouped by priority. Each entry has impact, file pointer, and effort estimate. + +--- + +## TIER 0 — Already Applied in Working Tree (uncommitted) + +Status: **fixed in code, not yet committed**. Commit + push to ship. + +### F1. `/api/v1/bootstrap/*` proxy allow-list (task #22) + +- **Impact:** Cold-start VPS deploy can't bootstrap its first super-admin. `/setup` page calls `/api/v1/bootstrap/status` which 401s; setup form never renders. +- **File:** `src/proxy.ts` — added to `PUBLIC_PATHS`. +- **Effort:** XS. + +### F2. Interest detail page 500s on every visit (task #25) + +- **Impact:** Sales workflow non-functional. Raw `Date` passed to postgres-js `sql\`${col} >= ${dateVar}\`` template crashes the Bind step. +- **File:** `src/lib/services/interests.service.ts:566` — switched to `gte(col, date)`. +- **Effort:** XS. + +--- + +## TIER 1 — Pre-Deploy Blockers (P1) + +Ship before any real client touches the system. + +### F3. GDPR export 500s — BullMQ rejects job IDs with colons (task #51) + +- **Impact:** GDPR Article 15 right-to-access non-functional. Legal/compliance gate. +- **File:** `src/lib/services/gdpr-export.service.ts:113` — change `jobId: \`gdpr-export:${row.id}\`` → `jobId: \`gdpr-export-${row.id}\``. +- **Effort:** XS (one char). + +### F4. Redis eviction policy is `allkeys-lru` but BullMQ requires `noeviction` (companion to F3) + +- **Impact:** Under memory pressure, Redis will evict BullMQ keys; jobs disappear silently. +- **File:** production Redis config (`maxmemory-policy noeviction`) + the docker-compose redis service. +- **Effort:** XS (config). + +### F5. `deleteBerth()` hard-deletes rows instead of soft-archiving (task #65) + +- **Impact:** Permanent data loss on accidental delete. Junction tables CASCADE-vanish. Audit log points to non-existent rows. Public feed could 404 mid-customer-inquiry. +- **Files:** + - `src/lib/services/berths.service.ts:673-685` — replace `db.delete()` with `set archivedAt = now(), archivedBy = userId, archiveReason = input.reason`. + - Add filter `isNull(berths.archivedAt)` to all default berth queries (recommender, public feed, list, dashboard heat). + - Add restore endpoint `POST /api/v1/berths/[id]/restore` mirroring the interests pattern. + - Require `reason` (min 5 chars) before destructive call. +- **Effort:** M. + +### F6. Weak input validation on `/api/v1/clients` (task #50) + +- **Impact:** Email format not validated (bounces silently); whitespace-only names accepted (blank chips everywhere); XSS payload stored verbatim (depends on every render path being safe). +- **Files:** + - `src/lib/validators/clients.ts` — add `.email()` refinement on contacts where `channel === 'email'`; trim+min(1) on `fullName`; regex-strip control chars + zero-width chars. + - Audit every fullName render path for `dangerouslySetInnerHTML` / pdfme / react-pdf / email template merges and ensure escaping. + - Apply similar hardening to yachts, companies, interests, notes, berths, reminders (audit all string fields). +- **Effort:** S for the obvious zod tweaks, M for the full audit. + +### F7. No rate limiting on login (task #68) + +- **Impact:** Brute force is wide open. 20 wrong-password attempts in a row all returned 401 with no lockout. +- **Files:** + - `src/lib/auth/` — add a `rateLimit` block to the better-auth config: `{ window: 60, max: 5 }` per IP+email. + - Optionally: Redis sliding window via existing ioredis client. + - Optionally: per-user lockout table (`auth_lockouts`) after 5 failures, locked 15min. +- **Effort:** S. + +### F8. postgres-js pool corruption causes CONNECT_TIMEOUT (task #46) + +- **Impact:** During the audit the dev server twice entered a stuck state where every query 500'd with `CONNECT_TIMEOUT` while the DB was healthy (1/100 connections used). Production VPS will hit this under load. +- **Files:** + - `src/lib/db/index.ts` — add `connect_timeout: 5`, `max_lifetime: 60 * 60`, `idle_timeout: 30`. + - Wrap critical-path queries in retry-on-CONNECT_TIMEOUT logic (one retry, then 503). + - Consider pgbouncer in front of postgres for production multi-process deployments. +- **Effort:** S for the postgres-js options, M for full pgbouncer. + +--- + +## TIER 2 — High Impact Architectural / UX + +Not strictly deploy-blocking, but each one breaks the UX in observable ways every day. + +### F9. Layout-wide duplicate mobile/desktop DOM rendering (task #26) + +- **Impact:** Single highest leverage UX bug. EVERY page mounts BOTH responsive layouts; both Radix Tabs providers are concurrently active with `data-state="active"`. Half my click attempts on tabs/filters/popovers went to the wrong layer. Doubled network requests, doubled component state, doubled a11y landmarks. +- **Files:** the responsive shell (likely `src/components/layout/*-shell.tsx` and detail-page wrappers). +- **Fix options:** use `useMediaQuery` to mount only one tree; or hoist `` to a single provider and let both layouts consume context. +- **Effort:** L (architectural refactor across multiple pages). + +### F10. Archiving a client doesn't cascade-archive their interests (task #66) + +- **Impact:** Orphan refs. Archived clients have active interests; active queries surface them with broken breadcrumbs / silent 404s on drill-in. +- **Files:** `src/lib/services/clients.service.ts:archiveClient()` — wrap in transaction, archive open interests too. OR extend `activeInterestsWhere()` to filter on `client.archived_at IS NULL`. +- **Effort:** S. + +--- + +## TIER 3 — Standard Fixes (P3) + +UX polish + missing entry points. Each is small, but the sum matters. + +### F11. "Mark as won" dialog still says "moves to Completed" (task #27) + +- **Impact:** Stale copy from before the 7-stage refactor. Misleads users. +- **File:** `src/components/interests/won-dialog.tsx` (or similar) — update copy to "marks Won; stage stays at ". +- **Effort:** XS. + +### F12. Activity feed + tab count concatenation (task #23) + +- **Impact:** "Test Person 1interest", "Interests0", "Click Test Co.company" — unprofessional. +- **Files:** `src/components/dashboard/activity-feed.tsx` (entity name + type), every detail-page tab count render. Audit log FTS `search_text` should also include entity names. +- **Effort:** S. + +### F13. Bulk-add berths wizard has no UI entry point (task #28) + +- **Impact:** Feature built for new-port setup, but invisible. Operator must know the URL. +- **Files:** Add a "Bulk add" button next to "New berth" on `/[portSlug]/berths`. Add link on `/admin` landing card. +- **Effort:** S. + +### F14. Audit Log page has no UI entry point (task #49) + +- **Impact:** Feature built, no nav link. Discovery requires URL knowledge. +- **Files:** Sidebar Admin section — add "Audit Log" entry under `documents` settings or as its own item, gated by `audit_log.view` permission. +- **Effort:** S. + +### F15. New Yacht dialog only lists clients in owner picker (task #44) + +- **Impact:** Data model supports `'client' | 'company'` ownership; UI only lets you pick clients. Cannot create company-owned yacht via UI. +- **Files:** `src/components/yachts/new-yacht-dialog.tsx` — add owner-type segmented control (Client / Company) above the owner picker; switch data source. +- **Effort:** S. + +### F16. InlineTagEditor "Add tag" focus + create flow (task #45) + +- **Impact:** Typing in the tag widget set the CONTACT LABEL instead. Plus no "Create new tag" affordance for new tag names. +- **Files:** `src/components/shared/inline-tag-editor.tsx`. Fix focus target; surface "Create new: X" as a popover item; orchestrate POST /api/v1/tags then PUT .../tags. +- **Effort:** S. + +### F17. Cross-port (and 404) detail URLs silently render list shell (task #48) + +- **Impact:** User pastes a wrong-port URL → API 404s correctly but UI silently shows the list shell. No explicit "not found" message. +- **Files:** every entity-detail client component — render `` when GET returns 404. Apply to clients, interests, yachts, companies, berths. +- **Effort:** M (apply pattern to each detail page). + +### F18. Recommender `limit` param ignored (task #69) + +- **Impact:** Request with `{"limit": 3}` returned 8 berths. Either param name mismatch or no clamp. +- **Files:** `src/lib/services/berth-recommender.service.ts` + the recommend-berths validator. +- **Effort:** XS. + +--- + +## TIER 4 — Polish & UX Reductions (P4) + +The `UX EFFICIENCY` list (task #24). Each is small, mostly copy/flow improvements. + +### F19. New Client form — primary contact default trap + +- Default-checked "Primary contact" with empty email silently rejects on submit. Either don't pre-add OR drop empty contacts on save. + +### F20. New Interest dialog — redirect to detail page on create + +- Currently returns to the list. Add `router.push('/interests/' + newId)` to land on the workflow page immediately. + +### F21. Stage-transition error toast leaks developer language + +- "yachtId is required before leaving stage=enquiry" → "Yacht is required before leaving the Enquiry stage." +- Audit ALL ValidationError + ConflictError + service error messages for user-readable copy. + +### F22. Stage menu uses unicode emoji `⚑` as prereq-blocked indicator + +- Per user preference (memory: avoid decorative emoji), replace with a Lucide icon (`Lock`, `AlertCircle`, or `FlagOff`). + +### F23. Blocked-stage UX — show prereq picker inline + +- Clicking a blocked stage currently dismisses with a toast. Better: open the prereq picker inline ("Pick a yacht to leave Enquiry" with combobox right there). + +### F24. New Client form — "Country" optional but prominent + +- Drop from quick-path OR move to a "More details" disclosure. + +### F25. Documents Hub — folder navigation doesn't update URL + +- Drilling into a folder updates "Current location" but doesn't change `location.search`. Can't deep-link, browser-back broken, refresh resets to root. + +### F26. "Reopen" outcome action silent — no toast + +- After clicking Reopen, no feedback. Add `toast.success('Outcome cleared')` or similar. + +### F27. Same-stage write returns full body — should be 204 + +- PATCH /stage with same stage = current stage returns 200 + full interest. Should be 204 No Content (no-op). + +### F28. Recommender empty-result UI + +- 300ft yacht returns `data: []` — UI Recommendations tab silently shows blank. Should render "No berths match — try relaxing constraints." + +### F29. Inbox first-load "Loading..." stuck + +- First navigation to /inbox shows "Loading..." indefinitely; subsequent reload renders fine. TanStack Query cache initialization issue. + +### F30. Berths in default queries should filter `archivedAt IS NULL` + +- Companion to F5 — once soft-delete lands, every default list query must filter archived rows. + +--- + +## NEW FEATURE — Manual Berth Status Catch-Up Workflow (task #67) + +User-requested. Foundation already exists (column `berths.status_override_mode` is in schema but never written). + +### Phase 1 — Wire the status_override_mode field + +- `updateBerthStatus()` sets `status_override_mode = 'manual'` when called via the user-facing API. +- `berth-rules-engine.ts` triggers set `status_override_mode = 'automated'`. +- When a backing interest is successfully created and links the berth, clear `status_override_mode` back to null in the same transaction; set `status_last_changed_reason` to "Reconciled via interest [id]". +- **Effort:** S. + +### Phase 2 — Visual indicator + +- On berth list rows: small chip "Manual" next to the status badge when `status_override_mode = 'manual'` AND no active interest is linked. +- On berth detail page header: badge + tooltip showing last reason, user, when. +- On dashboard "Berth Heat" widget: filter or annotate the manual rows. +- **Effort:** S. + +### Phase 3 — Reconciliation Queue page + +- New page `/[portSlug]/admin/berths/reconcile`. +- Lists every berth where `status_override_mode = 'manual'` and no active interest. Sortable by `status_last_modified DESC`. +- Each row links to the catch-up wizard. +- Sidebar Admin section gets a link with the queue count badge. +- **Effort:** S. + +### Phase 4 — Catch-Up Wizard (the core piece) + +- Multi-step modal. Steps: + 1. **Pick or create client** — combobox + inline quick-create (name + email only). + 2. **Pick or create yacht** — optional if pre-EOI; quick-create with name + dimensions. + 3. **Pick the matching stage** — based on current berth status: + - `under_offer` → enquiry / qualified / nurturing / eoi (default eoi) + - `sold` → contract + outcome=won + - Allow override. + 4. **Upload existing docs** — EOI PDF, contract PDF, reservation form. Each auto-filed to the right entity folder. + 5. **Optional payments** — if status=sold, prompt for deposit/full amount. + 6. **Review + submit.** On submit, transaction: + - Create/select client + yacht + - Create interest at chosen stage with `assigned_to = current user` + - Upsert `interest_berths(is_primary=true, is_specific_interest=true, is_in_eoi_bundle=true)` + - Upload + attach files + - Insert payments + - Set `berth.status_override_mode = null` + `status_last_changed_reason = 'Reconciled via interest [id]'` + - Audit log single "reconcile" event linking berth + new interest. +- **Effort:** M (wizard) + S (transaction service) + S (API endpoint). Total M-L. + +### Phase 5 — Entry points + +- Berth list row menu → "Catch up..." +- Berth detail page next to manual badge → "Catch up" +- Dashboard widget "Manual statuses awaiting reconciliation" (count + link) +- Sidebar link +- **Effort:** S. + +### Total feature effort: M-L (2-3 dev days). + +--- + +## What I Tested in Round 2 (15 deep journeys, all passed structural validation) + +| Journey | Result | +| -------------------------------------------- | ------------------------------------------------------------------------------------------- | +| State machine — stage skipping | ✓ Rejects forward/backward jumps with friendly copy + override path | +| Double outcome write | ⚠ Allowed (won→lost flips freely); audit log just says "update" — should tag outcome change | +| Cascade — delete with dependents | ✗ Inconsistent: clients soft-archive, **berths HARD-delete**, companies soft-archive | +| Manual berth status without backing interest | ✗ Foundation column exists, never written | +| Unicode (emoji/RTL/zero-width) | ⚠ Emoji + RTL OK; zero-width chars NOT stripped (search blind spot) | +| Storage / file upload magic-byte | ✓ Rejects JPEG/HTML disguised as PDF | +| Documenso webhook idempotency | ✓ Timing-safe + rate-limited bad-secret check | +| Berth recommender edge cases | ⚠ Empty dims OK; extreme dims return empty; **limit param ignored** | +| Email body XSS via markdown | ✓ Escape-first-then-rules, javascript: URLs stripped | +| Public berth feed correctness | ✓ Port allow-list, archive filter, status enum validation | +| Rate limiting / abuse | ✗ Login: no rate limit; public feed: CDN-cached | +| Health check + dependency probes | ✓ Anonymous minimal payload, secret-mode for website-intake | +| Direct ID enumeration | ✓ Uniform 404 — no leak | +| Cross-port API access | ✓ 404 at API; **silent at UI** | +| CSRF — fake Origin | ✓ Prod-only protection — dev intentionally skips | + +--- + +## Recommended Commit Sequence + +1. **Squash-commit T0 fixes** (F1 + F2) — these are deploy-blockers already applied. Push to main. +2. **T1 batch commit** (F3, F4, F5, F6, F7, F8) — pre-deploy blockers. Single commit per fix for clean review. +3. **T2** (F9, F10) — schedule for next sprint (F9 is architectural). +4. **T3** (F11-F18) — knock out in a few hours. Quick polish wave. +5. **T4** (F19-F30) — UX list. Bundle into a single PR over a few sessions. +6. **NEW FEATURE — Catch-Up Workflow** — 2-3 dev days. Higher business value than T2; prioritize after T1. + +--- + +## Risk Notes + +- The audit polluted the dev DB with test entities: `Smoke Test Client (renamed)`, `Aurora Marine Holdings Ltd`, `Bad Email Test`, `Phone Test`, `Robert'; DROP TABLE clients`, `François 🏄 المعتمد`, `محمد عبد الله`, `CSRF Test`, etc. Also **hard-deleted berth A1 in port-amador** + soft-archived Test Person 1. Consider `pnpm db:reseed:synthetic` before the next clean run. +- The Smoke Test Client interest had `outcome=lost_other` set during the won-then-lost test (R2-B). Audit log preserved both transitions but with action="update" not action="outcome_change".