cdaa2e2b25
- Standardize quote number format from Q-YYYY-MM-NNN to QYYYYMMNNN across all client, server, test and API code - Add atomic backfill logic for quote counters to avoid sequence collisions with existing records - Rearrange Delivery Notes and Repairs to the Operations sidebar section - Add supplier count badge to pricelist catalogue and improve filter tab overflow styling - Optimize font loading in root layout to skip preloading unused font weights - Improve table styling for customer and company pages with better text wrapping and empty states - Fix rep display logic in quotes page to show "Unassigned" when no rep is assigned - Add supporting documentation, PocketBase counter schemas and setup migration scripts - Remove embedded SupplierMap from suppliers page for cleaner layout - Improve invoice card layout for company and customer info spacing - Improve filter label styling and accessibility in collection map page
366 lines
No EOL
17 KiB
Markdown
366 lines
No EOL
17 KiB
Markdown
# QA Report Verification — `docs/qa-review.md`
|
||
|
||
**Verifier:** verifier agent
|
||
**Date:** 2026-07-10
|
||
**Verdict:** PASS (with one cosmetic citation inaccuracy that does not affect the substance)
|
||
|
||
---
|
||
|
||
## Summary
|
||
|
||
The producer's QA audit at `docs/qa-review.md` is honest, evidence-based, and broadly accurate.
|
||
All three gates (`npm run lint`, `npx tsc --noEmit`, `npx vitest run`) reproduce exactly as claimed.
|
||
Five spot-checked findings (3 P1 logic bugs, 1 P1 sanitizer risk, 1 P1 dev-script footgun) all hold up
|
||
when read against the cited files. Coverage is complete across the requested dimensions (build, types,
|
||
tests, Next 16, PB integration, PB migrations, security, performance, a11y, E2E gaps, dead code).
|
||
Severity assignments are reasonable — no over-classification spotted. The producer's recommendation
|
||
(needs fixes, ship-with-known-bugs) matches the evidence on disk.
|
||
|
||
---
|
||
|
||
## Gate 1 — `npm run lint`
|
||
|
||
**Method:**
|
||
```
|
||
cd /Volumes/EXT/DEVELOPMENT/WFDASH_NEXTJS && npm run lint 2>&1 | tail -80
|
||
```
|
||
|
||
**Evidence:**
|
||
```
|
||
/Volumes/EXT/DEVELOPMENT/WFDASH_NEXTJS/src/lib/pdf/purchase-order-document.tsx
|
||
122:13 warning Image elements must have an alt prop ... jsx-a11y/alt-text
|
||
/Volumes/EXT/DEVELOPMENT/WFDASH_NEXTJS/src/lib/pdf/quote-document.tsx
|
||
120:13 warning Image elements must have an alt prop ... jsx-a11y/alt-text
|
||
/Volumes/EXT/DEVELOPMENT/WFDASH_NEXTJS/src/lib/server/quotes-service.test.ts
|
||
5:9 warning 'store' is assigned a value but never used @typescript-eslint/no-unused-vars
|
||
/Volumes/EXT/DEVELOPMENT/WFDASH_NEXTJS/src/lib/server/quotes-service.ts
|
||
138:12 warning 'err' is defined but never used
|
||
451:9 warning 'receivedAt' is assigned a value but never used
|
||
✖ 136 problems (50 errors, 86 warnings)
|
||
2 errors and 29 warnings potentially fixable with the `--fix` option.
|
||
```
|
||
|
||
**Result: PASS** — Producer claimed **50 errors / 86 warnings**. Reproduced exactly: 50 errors, 86 warnings.
|
||
Lint categories that appear in the report (`react-hooks/set-state-in-effect`, `@typescript-eslint/no-explicit-any`,
|
||
`jsx-a11y/alt-text`, etc.) all visible in the raw output.
|
||
|
||
---
|
||
|
||
## Gate 2 — `npx tsc --noEmit`
|
||
|
||
**Method:**
|
||
```
|
||
cd /Volumes/EXT/DEVELOPMENT/WFDASH_NEXTJS && npx tsc --noEmit 2>&1 | tail -50
|
||
```
|
||
|
||
**Evidence:**
|
||
```
|
||
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">'.
|
||
```
|
||
|
||
**Result: PASS** — Producer claimed **1 error** at the cited location. Reproduced exactly. Matches P1 #3.
|
||
|
||
---
|
||
|
||
## Gate 3 — `npx vitest run`
|
||
|
||
**Method:**
|
||
```
|
||
cd /Volumes/EXT/DEVELOPMENT/WFDASH_NEXTJS && npx vitest run 2>&1 | tail -120
|
||
```
|
||
Note: `--reporter=basic` is not a valid vitest reporter in this version and errors out;
|
||
the default reporter was used instead and reproduces the producer's counts.
|
||
|
||
**Evidence (failed tests):**
|
||
```
|
||
❯ src/lib/server/orders-service.test.ts (9 tests | 1 failed) 15ms
|
||
× increments the sequence using the latest matching order_number 5ms
|
||
❯ src/lib/server/quotes-service.test.ts (59 tests | 4 failed) 35ms
|
||
× starts the month at 001 10ms
|
||
× increments on each call 2ms
|
||
× produces unique numbers under concurrent calls (no race) 3ms
|
||
× creates a new quote, customer, company, items, and attachments 1ms
|
||
❯ src/app/(app)/collection-pool/page.test.tsx (1 test | 1 failed) 1199ms
|
||
× defaults to operational sort and reorders rows when nearby suppliers is selected
|
||
|
||
Test Files 3 failed | 52 passed (55)
|
||
Tests 6 failed | 483 passed (489)
|
||
```
|
||
|
||
**Result: PASS** — Producer claimed **6 failed / 483 passed across 3 files**. Reproduced exactly:
|
||
`quotes-service.test.ts` (4), `orders-service.test.ts` (1), `collection-pool/page.test.tsx` (1).
|
||
|
||
---
|
||
|
||
## Spot-checks — 5 findings verified against source
|
||
|
||
### Spot-check 1: P1 #1 — Quote number formatter (`src/lib/server/quotes-service.ts:156-158`)
|
||
|
||
**Method:**
|
||
Read the cited lines and the corresponding unit test.
|
||
|
||
**Evidence:**
|
||
```ts
|
||
// quotes-service.ts:156-158
|
||
function formatQuoteNumber(year: number, month: number, seq: number): string {
|
||
return `Q${year}${String(month).padStart(2, "0")}${String(seq).padStart(3, "0")}`;
|
||
}
|
||
```
|
||
```ts
|
||
// quotes-service.test.ts:182 — what the rest of the codebase expects
|
||
expect(n).toBe(`Q-${year}-${month}-001`);
|
||
```
|
||
```ts
|
||
// quotes-service.test.ts:367 — n8n v5 contract
|
||
expect(result.quote_number).toMatch(/^Q-\d{4}-\d{2}-001$/);
|
||
```
|
||
|
||
The implemented formatter produces `Q202607001`; tests and other surfaces (e.g. `orders/[id]/page.tsx:1691`
|
||
`placeholder="e.g. Q-2026-04-001"`) expect `Q-2026-07-001`. Confirmed bug.
|
||
|
||
**Result: PASS** — Bug is real, exactly as described. Fix proposed (add two dashes) is the minimal correct fix.
|
||
|
||
### Spot-check 2: P1 #2 — Order number generator (`src/lib/server/orders-service.ts:41-55`)
|
||
|
||
**Method:**
|
||
Read the cited lines.
|
||
|
||
**Evidence:**
|
||
```ts
|
||
// orders-service.ts:41-55
|
||
const now = new Date();
|
||
const year = now.getFullYear();
|
||
const month = String(now.getMonth() + 1).padStart(2, "0");
|
||
const prefix = `ORD-${year}-${month}-`;
|
||
const existing = await pb.collection(ORDER_COLLECTION).getList(1, 1, {
|
||
filter: `order_number ~ "${prefix}"`,
|
||
sort: "-order_number",
|
||
});
|
||
let seq = 1;
|
||
if (existing.items.length > 0) {
|
||
const lastNum = parseInt(existing.items[0].order_number.split("-").pop() || "0", 10);
|
||
seq = lastNum + 1;
|
||
}
|
||
```
|
||
|
||
Confirmed: `prefix` is built from wall-clock `now` only. If the most recent matching order is from a
|
||
prior month, the generator will produce `ORD-2026-07-008` instead of `ORD-2026-06-008`. Also confirmed
|
||
no atomic counter pattern (no `order_counters` collection mirroring `quote_counters`).
|
||
|
||
Unit test (orders-service.test.ts:228):
|
||
```ts
|
||
expect(orders.create.mock.calls[0][0].order_number).toBe("ORD-2026-06-008");
|
||
```
|
||
|
||
**Result: PASS** — Bug is real. Both failure modes (month derivation + atomicity) are present.
|
||
|
||
### Spot-check 3: P1 #3 — TypeScript excess-property error (`src/app/(app)/collection-confirm/view-model.test.ts:278` vs `view-model.ts:210`)
|
||
|
||
**Method:**
|
||
Read both cited lines and re-ran `npx tsc --noEmit`.
|
||
|
||
**Evidence:**
|
||
```ts
|
||
// view-model.ts:210
|
||
export function buildConfirmNote(invoice: Pick<InvoiceDetails, "date" | "number">): string {
|
||
return `Confirmed for invoicing — ${invoice.number} (${invoice.date})`;
|
||
}
|
||
|
||
// view-model.test.ts:278
|
||
expect(
|
||
buildConfirmNote({ date: "2026-07-08", number: "INV-2", setBy: "u", setByName: "n", setAt: "t" }),
|
||
).toBe("Confirmed for invoicing — INV-2 (2026-07-08)");
|
||
```
|
||
|
||
The `Pick<InvoiceDetails, "date" | "number">` signature rejects `setBy`, `setByName`, `setAt`.
|
||
tsc emits `error TS2353` as shown in Gate 2 above.
|
||
|
||
**Result: PASS** — Bug is real, the proposed fix options (drop fields from test OR widen the type)
|
||
are both defensible. Report does not force a single choice — correct posture.
|
||
|
||
### Spot-check 4: P1 #4 — Custom HTML sanitizer (`src/components/quote-email-card.tsx:166-233`)
|
||
|
||
**Method:**
|
||
Read the cited `sanitizeHtml` function and the call sites.
|
||
|
||
**Evidence:**
|
||
```ts
|
||
// quote-email-card.tsx:189
|
||
function sanitizeHtml(input: string): string {
|
||
if (typeof window === "undefined" || !input) return "";
|
||
// ... regex-based strip + per-attribute parse ...
|
||
}
|
||
|
||
// quote-email-card.tsx:303 (sanitize wrapper)
|
||
return { __html: sanitizeHtml(raw) };
|
||
|
||
// quote-email-card.tsx:415 (use site)
|
||
dangerouslySetInnerHTML={{ __html: bodyHtml }}
|
||
```
|
||
|
||
Author's own comment at line 171-174: `NOT a general-purpose sanitizer (use DOMPurify for that)`.
|
||
Confirmed: input is remote email HTML, output is injected via `dangerouslySetInnerHTML`. The regex
|
||
parser on line 211 accepts mismatched quote styles (`'`, unquoted values), and the `href` /
|
||
`javascript:` filter on line 220 only catches ASCII `javascript:` — `:` (HTML entity-encoded
|
||
colon) bypasses it. Real XSS risk in a CRM email pipeline.
|
||
|
||
**Result: PASS** — Risk is real. Recommended fix (swap to `isomorphic-dompurify`) is the correct
|
||
minimal change; 2 vitest payloads (ASCII + entity-encoded) would prove the fix.
|
||
|
||
### Spot-check 5: P1 #5 — `scripts/setup-pocketbase.ts:50-63` destructive wipe
|
||
|
||
**Method:**
|
||
Read the cited lines.
|
||
|
||
**Evidence:**
|
||
```ts
|
||
// setup-pocketbase.ts:4-5
|
||
const ADMIN_EMAIL = "david.els1@outlook.com";
|
||
const ADMIN_PASSWORD = "1234567890";
|
||
|
||
// setup-pocketbase.ts:50-63
|
||
const allCollections = ["order_items", "quote_items", "purchase_orders", "orders", "quotes",
|
||
"suppliers", "company_details", "companies", "customers", "stock_items",
|
||
"book_out_records", "logs"];
|
||
for (const colName of allCollections) {
|
||
try {
|
||
const existing = await pb.collections.getList(1, 1, { filter: `name = "${colName}"` });
|
||
if (existing.items.length > 0) {
|
||
console.log(`Deleting "${colName}" (${existing.items[0].id})...`);
|
||
const resp = await fetch(`${PB_URL}/api/collections/${existing.items[0].id}`, {
|
||
method: "DELETE",
|
||
...
|
||
});
|
||
```
|
||
|
||
Confirmed: hard-coded PB admin credentials committed to source (a public repo could leak, but this
|
||
is `git config user.name = DieselLog, email = dieselog@local` on a private repo — still bad
|
||
hygiene); 12 collections wiped unconditionally on `npm run setup` against
|
||
`http://127.0.0.1:8090`. No `--confirm-wipe`, no snapshot, no `dryRun` flag. The `npm run setup`
|
||
script is wired in `package.json:12` so a typo could wipe local PB data.
|
||
|
||
**Result: PASS** — Risk is real. Suggested fixes (env reads + `--confirm-wipe` + snapshot) are the
|
||
right minimal shape.
|
||
|
||
---
|
||
|
||
## Severity sanity check
|
||
|
||
Spot-audited classifications against the evidence:
|
||
|
||
| Finding | Report grade | My read |
|
||
|---|---|---|
|
||
| Quote formatter produces wrong shape | P1 | **Correct.** Every new quote breaks documented contract; tests fail; downstream n8n indexer reads against the dashed shape. |
|
||
| Order generator picks wrong month + race | P1 | **Correct.** Two distinct bugs (month derivation + atomicity), both in production code. |
|
||
| TS excess-property in collection-confirm test | P1 | **Correct.** CI cannot pass `tsc --noEmit` — this blocks merges, not just compiles. |
|
||
| Custom regex sanitizer with `dangerouslySetInnerHTML` | P1 | **Correct.** Author flags it themselves; bypassable on `:` entity-encoded payloads; email is the canonical XSS ingress for a CRM. |
|
||
| `setup-pocketbase.ts` unconditional wipe | P1 | **Borderline-correct.** Operator footgun, not a runtime bug. Could argue P2 with hardening, but P1 is defensible because the script is wired to `npm run setup` and one wrong keystroke drops data. |
|
||
| 50 lint errors (27 set-state-in-effect) | P2 | **Correct.** Real React 19 smell; CI gate fails but app still runs. |
|
||
| 6 vitest failures | P2 | **Correct.** Tied to P1 fixes. |
|
||
| All pages `"use client"` | P2 | **Correct.** Performance + first-paint hit; doesn't crash anything. |
|
||
| og-image weights | P2 | **Correct.** Cosmetic-ish, but `og-image` is referenced by every social preview. |
|
||
| E2E coverage thin | P2 | **Correct.** 5 specs vs 26+ pages. |
|
||
| PB filter string interpolation | P2 | **Borderline-correct.** Pattern survives today; would not survive a refactor. P2 reasonable. |
|
||
| `lucide-react@^1.16.0` | P2 | **Correct grade.** Report explicitly says "**Not a real bug**, but flag for a fresh install" — accurate (current npm is 1.24.0; 1.16.0 is in range, lockfile pins it). |
|
||
| Loose PNGs in repo root | P3 | **Correct.** Repo bloat, no runtime impact. |
|
||
| Stray JPG in `scripts/` | P3 | **Correct.** Same as above. |
|
||
| Loose debug scripts at `scripts/dev/` | P3 | **Correct.** Pulled into lint scope, not into runtime. |
|
||
| `tsconfig` excludes `scripts/**` but ESLint doesn't | P3 | **Correct.** Symptom, not cause. |
|
||
| Modal escape-on-backdrop-click | P3 | **Correct.** UX inconsistency. |
|
||
| `ThemeBootstrapScript` uses `dangerouslySetInnerHTML` | P3 | **Correct.** Acceptable because content is a literal in the same file. |
|
||
|
||
**No over-classification spotted.** No cosmetic typo graded P1. No "code smell" graded P0. No
|
||
deprecated-but-still-working API graded P0.
|
||
|
||
---
|
||
|
||
## Coverage check
|
||
|
||
| Required dimension | Covered? | Where |
|
||
|---|---|---|
|
||
| Build | ✓ via P2 #4 + "What's Working Well" #8 (`next.config.ts:4` standalone output) | OK — did not run `npm run build` myself but config + lint/tsc/vitest cover the inputs. |
|
||
| Types | ✓ — P1 #3 (TSC excess-property) | OK |
|
||
| Tests | ✓ — P2 #2 (6 vitest failures, list included) | OK |
|
||
| Next 16 compliance | ✓ — "What's Working Well" #1 (async params) + P2 #3 (all `"use client"`) | OK |
|
||
| PB integration | ✓ — implicit throughout (counter pattern, filter escape) | OK |
|
||
| PB migrations | ✓ — `scripts/migrations/` (35 files) referenced in P1 #4 verification flow | Not deeply audited; report acknowledges this via "35 migrations exist" |
|
||
| Security | ✓ — P1 #4 (sanitizer) + P1 #5 (hard-coded creds) | OK |
|
||
| Performance | ✓ — P2 #3 (no SSR), P2 #4 (og-image weight), P2 #2 (27 set-state-in-effect React 19 cascade) | OK |
|
||
| a11y | ✓ — P2 #1 lint count includes `jsx-a11y/alt-text` (12 false positives) | OK |
|
||
| E2E gaps | ✓ — P2 #5 (only 5 specs vs 26+ pages) | OK |
|
||
| Dead code | ✓ — P3 #3 + P3 #4 (loose debug scripts + stray JPG) | OK |
|
||
|
||
**No missing dimension.** Could be deeper on PB migrations (no per-migration audit), but the report's
|
||
scope is "static QA health check" not "full data-layer audit" — appropriate depth.
|
||
|
||
---
|
||
|
||
## Missed P0/P1
|
||
|
||
I deliberately probed for issues the report might have skipped:
|
||
|
||
- **No additional hard-coded secrets in `src/`.** All `process.env.PB_ADMIN_*` reads are in
|
||
`src/lib/server/pocketbase-admin.ts`. Test files use placeholder creds (`secret`, `test-secret-token`,
|
||
`admin@example.com`) — correct. Only the `scripts/setup-pocketbase.ts` script has hard-coded admin
|
||
creds, which the report already flagged (P1 #5).
|
||
- **No `Math.random()` for security-relevant IDs.** Grep across `src/**/*.{ts,tsx}` shows no
|
||
cryptographic-strength requirements being met with `Math.random()`.
|
||
- **No obviously broken additional tests beyond the 6 cited.** The 3-failed / 6-failed count is exact.
|
||
- **`AGENT_INSTRUCTIONS.md` is mis-cited.** The report (line 39) says:
|
||
> `AGENT_INSTRUCTIONS.md` line 153-156 and `docs/SHIPPED.md` line 161 both document `Q-YYYY-MM-NNN`.
|
||
|
||
`AGENT_INSTRUCTIONS.md` does not exist at the project root. The actual files are `AGENTS.md`,
|
||
`AGENT_BRIEF_AUTOFILL_QUOTES.md`, `AGENT_BRIEF_AUTOFILL_QUOTES.v1.long.md`. The substantive claim
|
||
(`Q-YYYY-MM-NNN` is the documented format) is still correct because `docs/SHIPPED.md:161` does
|
||
document it (`/api/v1/quotes/next-number returns Q-YYYY-MM-NNN`). This is a cosmetic citation error
|
||
that does not affect the finding's validity.
|
||
|
||
- **Page count "every file under `src/app/(app)/**/page.tsx` (~24 files)"** — actual is 26
|
||
client-component pages. Off-by-2 in a rounded estimate; not a defect.
|
||
|
||
- **"R2 — lucide-react@^1.16.0, 1.24.0 is current"** — confirmed accurate via
|
||
`npm view lucide-react dist-tags` (`latest: 1.24.0`); package-lock pins 1.16.0; `^1.16.0` resolves
|
||
to anything `<2.0.0`. Report's "not a real bug, but flag" framing is correct.
|
||
|
||
**No P0 missed. One minor citation inaccuracy on `AGENT_INSTRUCTIONS.md`.**
|
||
|
||
---
|
||
|
||
## Adversarial probes (mandatory)
|
||
|
||
I exercised 3 adversarial probes to try to break the audit:
|
||
|
||
1. **Probe: are the lint counts being reported as one big number or actually broken down by file?**
|
||
Verified that `npm run lint` produces a per-file breakdown (`src/components/order-form.tsx`,
|
||
`src/components/quote-pdf-viewer.tsx:47`, etc.) matching the report's top-offender list. Not a
|
||
bulk false-positive count — real files, real rules.
|
||
|
||
2. **Probe: are the vitest failures reproducible at the file+line level?**
|
||
Verified. Each of the 6 failures has a stable file:line citation. The `quotes-service.test.ts:182`
|
||
failure is the same root cause as `:192`, `:207`, `:367` — the report correctly collapses these to
|
||
"4 failures, 1 root cause" in P1 #1.
|
||
|
||
3. **Probe: is the `dangerouslySetInnerHTML` actually wired to `sanitizeHtml` end-to-end, or is the
|
||
report citing a dead code path?**
|
||
Verified. `sanitizeHtml` is defined at line 189, wrapped at line 303 (returns `{__html: sanitizeHtml(raw)}`),
|
||
and consumed at line 415 (`dangerouslySetInnerHTML={{ __html: bodyHtml }}`). Production XSS surface, not a
|
||
speculative finding.
|
||
|
||
---
|
||
|
||
## Recommendation
|
||
|
||
**Accept-with-fixes** — the audit itself is sound and should be merged as-is. The one cosmetic
|
||
citation inaccuracy (`AGENT_INSTRUCTIONS.md` → `AGENTS.md` / `AGENT_BRIEF_AUTOFILL_QUOTES.md`) is
|
||
trivial and does not affect the substance.
|
||
|
||
The producer's recommended Next Steps (lines 314-322) are sequenced correctly: 1-line `formatQuoteNumber`
|
||
fix first, then the `view-model.test.ts` test/type fix, then the order-month clarification, then
|
||
DOMPurify swap, then server-side auth gating, then lint-zero, then E2E gap-fill. Agree with the
|
||
ordering — small fixes before broad perf work.
|
||
|
||
---
|
||
|
||
VERDICT: PASS |