David/WFDASH-WEBAPP
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions
WFDASH-WEBAPP/docs/qa-review.md
SJCoetz 7df7a8850f feat(item-history): add getItemHistory service with 5 PB queries 
Co-authored-by: Orca <help@stably.ai>
2026-07-29 14:51:58 +02:00

23 KiB
Raw Blame History

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: 
    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: 
    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: 
      buildConfirmNote({ date: "2026-07-08", number: "INV-2" })
    • Or change buildConfirmNote to accept the full type if those fields are needed later: 
      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: 
    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 DELETEs 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: 
    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)?