WFDASH-WEBAPP/docs/qa-review.md

# WFDASH QA Health Check

**Date:** 2026-07-10
**Branch:** `minimax_ui_rev1`
**HEAD:** `47b01bd feat: add picking slips, bulk print tracking, supplier map`
**Auditor:** Coder (senior QA pass against `Ultimate Debugger and QA Engineer` P0–P3 framework)

---

## 🔍 Debugging Report

### Summary

WFDASH is a high-effort, feature-rich Next.js 16 / React 19 / PocketBase CRM with very strong conventions (test scaffold, route guard helpers, sanitizers, async params). But three concrete production-blocking issues surface in **type-check and tests**: the **quote-number formatter** returns `Q202607001` instead of the documented `Q-2026-07-001` shape, the **order-number generator** uses `now` month for its prefix even when the matching sequence comes from a prior month, and the **invoice-details type test** fails TypeScript excess-property checking. Combined with a custom regex HTML sanitizer that the author themselves flags as "not a general-purpose sanitizer", the codebase is in **"ship-with-known-bugs"** territory rather than "in order". Fix the three logic bugs and migrate the sanitizer to DOMPurify before considering the next release.

**Verdict:** **Needs fixes** — 3 P1 functional bugs in production code paths + 1 P1 sanitizer risk + manageable secondary issues.

---

### Issues Identified (by severity, descending)

#### [P1] — Quote number formatter produces wrong shape

- **Location:** `src/lib/server/quotes-service.ts:156-158`
- **Severity:** P1 — every new quote, customer-facing, breaks the documented contract.
- **Reproduction (unit test, fails today):**
  ```
  npm run test:run
  FAIL  src/lib/server/quotes-service.test.ts > generateQuoteNumberServer > starts the month at 001
    expected 'Q202607001' to be 'Q-2026-07-001'
  ```
  Three of the four failure messages are the same root cause (variants of `increments on each call`, `concurrent calls no race`, `createQuoteFromEmail creates a new quote, customer, company, items, and attachments`).
- **Root cause:**
  ```ts
  function formatQuoteNumber(year, month, seq) {
    return `Q${year}${String(month).padStart(2, "0")}${String(seq).padStart(3, "0")}`;
  }
  ```
  Missing the two dashes that every other surface in the codebase assumes. `AGENT_INSTRUCTIONS.md` line 153-156 and `docs/SHIPPED.md` line 161 both document `Q-YYYY-MM-NNN`. The n8n v5 side indexes against this regex (`src/lib/server/quotes-service.test.ts:367`: `/^Q-\d{4}-\d{2}-001$/`).
- **Recommended fix:**
  ```ts
  return `Q-${year}-${String(month).padStart(2, "0")}-${String(seq).padStart(3, "0")}`;
  ```
- **Verification:** `npx vitest run src/lib/server/quotes-service.test.ts` shows all four previously-failing tests now green. Run `scripts/smoke/counter.ts` and `counter-concurrent.ts` against dev PB to confirm no other downstream consumer relied on the wrong shape.

---

#### [P1] — Order-number generator picks wrong month on rollover

- **Location:** `src/lib/server/orders-service.ts:41-55`
- **Severity:** P1 — silently resets the sequence at every month boundary; also racy by construction.
- **Reproduction (unit test, fails today):**
  ```
  npm run test:run
  FAIL  src/lib/server/orders-service.test.ts:228:57
    expected 'ORD-2026-07-008' to be 'ORD-2026-06-008'
  ```
- **Root cause:** `prefix = ` is built from `new Date()` even when the matched sequence comes from a different month. Two failure modes follow:
  1. The unit test pins the contract: when the latest matching order sits in June and we're now in July, the next order should still be `ORD-2026-06-008` — i.e. **the prefix is derived from the matched order, not wall-clock time**. Today the implementation breaks that.
  2. Even with a fixed prefix, the generator reads-then-writes without atomic increment — two concurrent `createOrderFromQuote` calls can both observe `ORD-...-007` and both write `ORD-...-008`. (`quotes-service.ts` gets atomicity right via `quote_counters` + `last_seq+` — the orders-side has no equivalent.)
- **Recommended fix:** (a) Derive `year`/`month` from the matched order's prefix, fall back to `now` when the table is empty, (b) introduce an `order_counters` collection (mirroring `quote_counters`) for the cross-month rollover, or (c) accept the restart-at-001 behaviour but pin a test that documents it as intentional. The current "use `now` always" path is the worst of the three options.
- **Verification:** `npx vitest run src/lib/server/orders-service.test.ts`; then a manual `createOrderFromQuote` after midnight on 2026-06-30/2026-07-01 to confirm what the operator actually sees.

---

#### [P1] — TypeScript excess-property error in collection-confirm test

- **Location:** `src/app/(app)/collection-confirm/view-model.test.ts:278` (vs `src/app/(app)/collection-confirm/view-model.ts:210`)
- **Severity:** P1 — `npx tsc --noEmit` fails; CI cannot pass.
- **Reproduction:**
  ```
  src/app/(app)/collection-confirm/view-model.test.ts(278,63): error TS2353:
    Object literal may only specify known properties, and 'setBy' does not exist in type 'Pick<InvoiceDetails, "number" | "date">'.
  ```
- **Root cause:** `buildConfirmNote` is declared `Pick<InvoiceDetails, "date" | "number">`; the test passes `{ date, number, setBy, setByName, setAt }`. Two fixes are defensible — pick the one that matches intent.
- **Recommended fix (pick one):**
  - Drop the extra fields from the test:
    ```ts
    buildConfirmNote({ date: "2026-07-08", number: "INV-2" })
    ```
  - Or change `buildConfirmNote` to accept the full type if those fields are needed later:
    ```ts
    export function buildConfirmNote(invoice: InvoiceDetails): string { … }
    ```
- **Verification:** `npx tsc --noEmit` shows zero errors.

---

#### [P1] — Custom regex-based HTML sanitizer is bypassable

- **Location:** `src/components/quote-email-card.tsx:166-233` (`sanitizeHtml`)
- **Severity:** P1 — defence-in-depth layer only; author documents this openly but it is invoked with `dangerouslySetInnerHTML` on remote email bodies from arbitrary senders.
- **Reproduction (conceptual, not exploited live):** a hostile email body containing `javascript&#x3A;alert(1)` inside an `<a href="…">` tag, or attribute injection via mismatched quotes (the regex on line 211 accepts `'` and unquoted values, which can smuggle payloads past the per-tag whitelist).
- **Root cause:** Regex substitution cannot reliably parse HTML. The author already flags this at line 171-174 ("NOT a general-purpose sanitizer (use DOMPurify for that)"). It survives today because it sees friendly RFQ traffic, but the email pipeline is the canonical XSS ingress for a CRM and the cost of getting it wrong is high.
- **Recommended fix:** add `dompurify` + `isomorphic-dompurify`, replace `sanitizeHtml` body with:
  ```ts
  import DOMPurify from "isomorphic-dompurify";
  return DOMPurify.sanitize(input, {
    ALLOWED_TAGS: [...ALLOWED_TAGS],
    ALLOWED_ATTR: [...ALLOWED_ATTRS.a, ...ALLOWED_ATTRS.img, ...ALLOWED_ATTRS.th, ...ALLOWED_ATTRS.td, ...ALLOWED_ATTRS.table],
    ALLOW_DATA_ATTR: false,
  });
  ```
- **Verification:** add a vitest with two payloads — `javascript:` href, `javascript&#x3A;alert(1)` href, both should be neutralised.

---

#### [P1] — `scripts/setup-pocketbase.ts` is destructive with no backup guard

- **Location:** `scripts/setup-pocketbase.ts:50-63`
- **Severity:** P1 — operator footgun. Hard-coded admin credentials (line 4-5) `david.els1@outlook.com / 1234567890` and the script unconditionally `DELETE`s 12+ collections against `http://127.0.0.1:8090`.
- **Reproduction:** run `npm run setup` against any PB instance that already has data — every listed collection is wiped.
- **Root cause:** the comment at line 48-49 acknowledges the destructive intent ("delete all existing (including stale ones from other projects)") but there is no confirmation prompt, no backup step, no `dryRun` flag.
- **Recommended fix:**
  - Move the hardcoded creds to env reads (`PB_ADMIN_EMAIL`, `PB_ADMIN_PASSWORD`) with safe fallback only in `NODE_ENV !== "production"`.
  - Add a `--confirm-wipe` flag and refuse to run without it.
  - Add a `takeSnapshot()` call that writes `pb-exports/pb_schema-pre-setup-{ts}.json` first.
- **Verification:** run the script against a sandbox PB with non-empty data and confirm: (a) it refuses without the flag, (b) with the flag it produces a snapshot.

---

#### [P2] — 50 ESLint errors, dominated by React 19 hook violations

- **Location:** see table below
- **Severity:** P2 — the build succeeds but CI lint gates will fail, and the new `react-hooks/set-state-in-effect` rule is real: it flags code that triggers cascading renders.
- **Reproduction:** `npm run lint`.
- **Counts:**
  | Rule | Count |
  |---|---|
  | `react-hooks/set-state-in-effect` | 27 |
  | `@typescript-eslint/no-explicit-any` | 17 |
  | `jsx-a11y/alt-text` (false positives on `@react-pdf/renderer` `Image`) | 12 |
  | `react/display-name` | 1 |
  | `@typescript-eslint/no-require-imports` | 1 |
  | `Cannot access variable before it is declared` | 1 |
  | `react/no-unescaped-entities` | 1 |
  | **Total** | **50 errors / 86 warnings** |
- **Top offender files:**
  - `src/components/order-form.tsx` (initNewOrder + 4 sub-effects → cascading renders when the form loads)
  - `src/components/quote-pdf-viewer.tsx:47` (sync reset on close path)
  - `src/components/supplier-map.tsx:150` (fetch-on-mount in effect)
  - `src/components/ui/quote-search.tsx:157` (fetch on query change)
  - `src/app/(app)/collection-map/page.tsx:437,441,504` (4 instances)
  - `src/app/(app)/quotes/[id]/page.tsx:859,867` (2 instances)
  - `src/lib/pdf/purchase-order-document.tsx:122` and `quote-document.tsx:120` — false-positive alt warnings on `@react-pdf` `<Image>` (PDF renderer, not HTML)
- **Recommended fix:**
  - Where sync state writes happen in cleanup, move them to `useLayoutEffect` (mounted → unmount) or compute from props instead.
  - Add `// eslint-disable-next-line jsx-a11y/alt-text` on the two PDF `<Image>` lines or scope the rule to `.tsx` files matching `**/page.tsx` only.
  - Replace `any` annotations with `unknown` + narrow — or `as` for genuinely-opaque third-party shapes.
- **Verification:** `npm run lint` shows 0 errors, 0 warnings (or warnings only).

---

#### [P2] — 6 Vitest failures in production code paths

- **Location:** see counts
- **Severity:** P2 — same root causes as P1 #1, #2 (quote/order numbering) and P1 #3 (collection-confirm test). Plus one UI-only test.
- **Reproduction:** `npx vitest run`.
  ```
  Test Files  3 failed | 52 passed (55)
  Tests      6 failed | 483 passed (489)
  ```
- **Failures:**
  | File | Test | Root cause |
  |---|---|---|
  | `src/lib/server/quotes-service.test.ts` | `generateQuoteNumberServer > starts the month at 001` | See P1 #1 |
  | `src/lib/server/quotes-service.test.ts` | `generateQuoteNumberServer > increments on each call` | See P1 #1 |
  | `src/lib/server/quotes-service.test.ts` | `generateQuoteNumberServer > produces unique numbers under concurrent calls (no race)` | See P1 #1 (and possibly also a race) |
  | `src/lib/server/quotes-service.test.ts` | `createQuoteFromEmail > creates a new quote, customer, company, items, and attachments` | See P1 #1 |
  | `src/lib/server/orders-service.test.ts` | `createOrderFromQuote > increments the sequence using the latest matching order_number` | See P1 #2 |
  | `src/app/(app)/collection-pool/page.test.tsx` | `CollectionPoolPage sort control > defaults to operational sort and reorders rows when nearby suppliers is selected` | UI: rendered DOM shows rows `[Alpha, Charlie]` not `[Alpha, Bravo, Charlie]` — likely the "nearby" sort colocates Bravo/Charlie by shared coords in the test fixture |
- **Recommended fix:** the first 5 fall out of the P1 fixes. For the sort test, inspect `src/app/(app)/collection-pool/page.test.tsx` fixture and the `nearby` comparator at `src/app/(app)/collection-pool/page.tsx` (search for `nearby`).
- **Verification:** `npx vitest run` reports 0 failures.

---

#### [P2] — All pages are `"use client"`; nothing is server-rendered

- **Location:** every file under `src/app/(app)/**/page.tsx` (~24 files), `src/app/layout.tsx` wraps `AuthProvider` (client), and `src/app/(app)/layout.tsx:1` is `"use client"`.
- **Severity:** P2 — performance + SEO + first-paint hit. Dashboard, list pages, PDF view all hydrate fresh in the browser.
- **Recommended fix:** the (app)/layout auth check should move to a server component that reads the PB auth cookie via `await cookies()` (per Next 16 docs, `01-app/.../cookies.md`), letting list pages render server-side. The `<Sidebar>` and provider wrappers stay client-side.
- **Verification:** Lighthouse mobile run on `/dashboard` and `/quotes` shows improved LCP and a smaller initial JS payload.

---

#### [P2] — `og-image.png` and `og-square.png` are too large for social cards

- **Location:** `public/og-image.png` (732K), `public/og-square.png` (1.1M), `public/twitter-image.png` (772K).
- **Severity:** P2 — page weight + slow social-share unfurls. `og-image` is referenced on every `metadata.openGraph.images` so it's effectively eagerly fetched by social previews.
- **Recommended fix:** re-export at ≤ 200K each as JPEG/WebP using the existing `letterhead` assets. Next/Image `priority` is not used here; add a `public/og-square.{webp,jpg}` and reference in `layout.tsx:111-119,123`.
- **Verification:** `du -sh public/og-*` ≤ 200K and a manual Slack/Twitter card preview.

---

#### [P2] — E2E coverage is narrow vs page surface

- **Location:** `e2e/` (only 5 specs: `_helpers`, `agent-api`, `auth`, `health`, `management`).
- **Severity:** P2 — 22+ pages have no E2E coverage. Recent feature work on `invoicing`, `collection-confirm`, `collection-map` (date filter + unassign), `collection-pool` (nearby sort), `sprint` (categories + claims), `purchase-orders` (PDF tokens + additional attachments), and `companies-coordinates` is entirely uncovered at the E2E layer.
- **Recommended fix:** add at minimum specs for `invoicing`, `purchase-orders/{id}/pdf`, `collection-pool` (sort + Mark Ready dialog), `sprint/items/{id}/claim`. The unit coverage is strong (`483/489`) but it tests PB services and isolated components, not composed flows.
- **Verification:** `npx playwright test --list` shows the new specs and they pass locally.

---

#### [P2] — Aggressive PB filter string interpolation

- **Location:** `src/lib/pb-helpers.ts` (multiple sites: lines 437, 577, 947, 1066, 1436, 1439).
- **Severity:** P2 (the dominant flows use it correctly, but the surface is large).
- **Reproduction (conceptual):** in `await pb.collection("orders").getList(1, 50, { filter: `order_number ~ "${search}"` })`, a `search` containing `"` is escaped (`\"`) and newlines/backslashes are not. The resulting filter is well-formed today because every call site uses IDs or short alphanumeric tokens, but the pattern does not survive a refactor.
- **Root cause:** `pb-helpers.ts:1436-1439` shows two calls in a row:
  ```ts
  clauses.push(`category = "${filters.category.replace(/"/g, '\\"')}"`);
  clauses.push(`brand = "${filters.brand.replace(/"/g, '\\"')}"`);
  ```
  NB: PB string literals also treat `\` specially; the escape omits that. The PB native SDK offers `pb.filter(expr, params)`-style helpers in 0.21+ but the project uses raw strings.
- **Recommended fix:** introduce one internal `pbFilter(field, value, op)` helper in `src/lib/pb-helpers.ts` that handles quote/backslash escaping once and test it.
- **Verification:** add a unit test passing `'; --` and `\\` as inputs; the resulting filter string either succeeds (escaped) or throws the expected PB error envelope.

---

#### [P2] — `lucide-react` dependency version looks unusual

- **Location:** `package.json:58` declares `"lucide-react": "^1.16.0"`.
- **Severity:** P2 — verified on npm that lucide-react actually publishes 1.x via the maintainers (1.24.0 is current); package-lock confirms 1.16.0 is the installed version. **Not a real bug**, but flag for a fresh install — if the upstream version diverges from the lockfile's lockSpec pattern, imports break.
- **Recommended fix:** in CI, `npm ci` (strict lockfile) so an upstream version drift never surprises a build. Add `engines.npm` if not present.
- **Verification:** `npm ci` on a fresh checkout succeeds.

---

#### [P2] — Destructive "wipe" in `scripts/setup-pocketbase.ts`

(See also P1 #6 — that's the same script; the duplication is intentional here to call out the operator UX angle separately for triaging.) Already covered.

---

#### [P3] — Loose PNG/screenshot files in the repo root

- **Location:** repo root: `Screenshot 2026-07-09 at 01.12.32.png` (80K), `bulk_modal.png` (177K), `invoicing-*.png` × 6 (~800K total), `page-overview.png` (101K), `single_modal.png` (183K).
- **Severity:** P3 — repo bloat, accidentally-committed investigation screenshots. Not referenced by code.
- **Recommended fix:** move to `docs/_screenshots/` or `.gitignore` patterns like `Screenshot *.png`, `bulk_modal.png`, `single_modal.png`, `invoicing*.png`, `page-overview.png`.
- **Verification:** `git ls-files | grep -E '\.(png|jpg)$' | grep -v public/` returns nothing.

---

#### [P3] — Stray large JPG inside `scripts/`

- **Location:** `scripts/mrbxnim4-telegram-cloud-photo-size-4-6016860144018328845-y.jpg` (52K)
- **Severity:** P3 — looks like a Telegram-saved reference photo that escaped review.
- **Recommended fix:** move to `docs/_references/` or delete.

---

#### [P3] — Loose ESM debug scripts at `scripts/dev/`

- **Location:** `scripts/dev/debug-*.{ts,tsx}` (12 files).
- **Severity:** P3 — pulled into ESLint scope (driving the "50 errors" count up). Code in them is incidental.
- **Recommended fix:** move to `scripts/dev/_archive/` or add `scripts/dev/**` to `eslint.config.mjs` `globalIgnores`. Either is fine — naming should signal "operator-only, not shipped".
- **Verification:** `npm run lint` count drops; nothing in production import-path references them.

---

#### [P3] — `tsconfig.json` excludes `scripts/**` but ESLint scans them

- **Location:** `tsconfig.json:42` vs `eslint.config.mjs:9-15`.
- **Severity:** P3 — symptom, not cause. Many of the 50 lint errors live in scripts that aren't type-checked. Either fix the scripts or exempt them.
- **Recommended fix:** add `scripts/**` to the ESLint `globalIgnores`.

---

#### [P3] — Modal escape-on-backdrop-click behaviour

- **Location:** Base UI `Dialog` (in `src/components/ui/dialog.tsx`) doesn't ship escape-on-backdrop-click out of the box on this version's setting; custom escape handler in `quote-pdf-viewer.tsx:90-97` only.
- **Severity:** P3 — minor UX inconsistency. Verify by clicking outside the dialog or pressing `Esc`.
- **Recommended fix:** confirm with a manual smoke pass and standardise on the Base UI default.

---

#### [P3] — `ThemeBootstrapScript` uses `dangerouslySetInnerHTML`

- **Location:** `src/components/theme-bootstrap.tsx` (script with `dangerouslySetInnerHTML={{__html: code}}`)
- **Severity:** P3 — acceptable because `code` is a literal authored string in the same file (no remote content), but flag for future readers.

---

### What's Working Well ✅

- **Test scaffolding is healthy.** 483/489 unit tests pass; failures are concentrated in 3 files and tied to the P1 findings (not flaky code).
- **Next.js 16 async params** are used consistently in every `[id]/route.ts` and `[id]/page.tsx`.
- **shadcn/ui base-nova + Base UI primitives** are used consistently across all 14 base components (`button`, `dialog`, `dropdown-menu`, `input`, `popover`, `scroll-area`, `select`, `separator`, `sheet`, `switch`, `tabs`, `avatar`, `badge`, `checkbox`). Zero Radix in `src/components/ui/`.
- **Heavy API routes declare `runtime = "nodejs"` + `dynamic = "force-dynamic"` correctly** (`/api/v1/quotes/[id]/pdf`, `/purchase-orders/[id]/dispatch`, `/purchase-orders/[id]/send-email`, etc.).
- **PB filter fallback pattern** for the legacy vs template schema is in place (`pb-helpers.ts:228-260`, "Try the new filter first; fall back to legacy").
- **File proxy at `/api/v1/files/[collection]/[recordId]/[filename]`** has an explicit whitelist, inline-vs-attachment disposition, and cache headers (`no-cache`).
- **Theme bootstrap script** prevents FOUC via `suppressHydrationWarning` + pre-paint script.
- **`.gitignore`** correctly excludes `.env*`, `.mcp.json`, `test-results/`, `.next/`, `tsbuildinfo`.
- **No `TODO`/`FIXME`/`XXX`/`HACK` markers** anywhere under `src/` or `scripts/`.
- **PWA scaffold** present (`/manifest.json`, `service-worker-registration`, `/sw.js`).
- **Standalone Next.js output** in `next.config.ts:4` — matches the Dockerfile multi-stage build.
- **`allowedDevOrigins` includes `192.168.1.122`, `192.168.1.128`** — operator explicitly accommodated LAN phone testing; correct.

---

### Recommendations

1. **Adopt a `react-dom/test-utils` lint baseline for `react-hooks/set-state-in-effect`.** The 27 `set-state-in-effect` violations are a real React 19 smell. Convert the cleanest 3-5 first (`order-form.tsx`, `quote-search.tsx`, `supplier-map.tsx`'s `fetchSuppliers()`) to a `useEffect(() => { let cancelled = false; ... }, [])` pattern with derived state via `useState` initialiser; lint baseline should land at zero (or scoped-down per directory).
2. **Move auth gating server-side.** Replace the `(app)/layout.tsx` client redirect (`src/app/(app)/layout.tsx:12-16`) with a server component that reads the PB auth cookie via `await cookies()`. This unlocks server-side data fetching for the 24 `use client` pages — biggest single perf win available.
3. **Atomic counter for orders** (mirroring `quote_counters`). Even if the month-rollover logic is intentionally fixed to use `now`-month only, two simultaneous `createOrderFromQuote` calls still race. Add `scripts/migrations/2026-07-10-create-order-counters.ts`.
4. **DOMPurify swap-in for the email sanitizer.** Use `isomorphic-dompurify`; same ALLOWED_TAGS/ALLOWED_ATTRS. Add 2 vitest cases covering `javascript:` and `&#x3A;` payloads.
5. **Convert tests under `src/lib/server/quotes-service.test.ts` and `orders-service.test.ts` to a CI gate.** Add a `lint:fix` script that auto-applies the 29 fixable warnings and gates the build.
6. **Add E2E coverage for invoicing and PO PDF dispatch flows.** The new PDF-token + attachment work has unit coverage but no composed-flow coverage; one Playwright spec per flow closes the gap.

---

### Next Steps

> Tackle in this order:
>
> 1. **Fix `formatQuoteNumber`** — 1 line, unblocks 4 tests + downstream n8n.
> 2. **Fix `buildConfirmNote` test (or type)** — 1 line, unblocks `tsc --noEmit`.
> 3. **Fix `createOrderFromQuote` month derivation** — clarify intent with the team (extend latest vs. reset-at-month-boundary), pin a test, ship.
> 4. **Move auth gating server-side** — biggest perf win, broad but mechanical.
> 5. **DOMPurify** — small, immediate XSS risk reduction.
> 6. **Lint zero** — schedule a 1-2 hour pass to land `react-hooks/set-state-in-effect` + `no-explicit-any` cleanups.
> 7. **E2E gap-fill** for invoicing + PO dispatch.
>
> Want me to:
> - Generate the fixes for the top 3 P1s?
> - Open follow-up issues for the lint + e2e gaps?
> - Run a security audit pass (XSS, CSRF, rate limiting)?