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) <noreply@anthropic.com>
This commit is contained in:
305
docs/POST-AUDIT-FIX-PLAN.md
Normal file
305
docs/POST-AUDIT-FIX-PLAN.md
Normal file
@@ -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 `<Tabs>` 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 <current>".
|
||||
- **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 `<EmptyState title="Not found" />` 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".
|
||||
Reference in New Issue
Block a user