From 8106ba0e4e42f3b89c327fee9f9d0400373424e5 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Thu, 6 Aug 2026 19:29:56 +0200 Subject: [PATCH] chore: update implementation checklist with lessons learned Adds recurring patterns surfaced during this release's reviews: - Money is stored in major units; the /100 cents idiom and bare Math.round both corrupt currency values (#1916) - :global() cannot style another CSS module's hashed class, and Jest's identity-obj-proxy hides the failure (#1909) - New editor controls may never reach the export pipeline (#1959/#1973) - Test titles must be read against their assertions (#1916) - A skipped CI dependency is not a passed one (#2043) - New section for LLM/prompt-assembly defects (#1916, #1932, #1952) Sourced from product-owner, product-architect, and ux-designer review memory for the beta->main range promoted in #2041. --- .claude/checklists/implementation-checklist.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.claude/checklists/implementation-checklist.md b/.claude/checklists/implementation-checklist.md index b399354aa..8e3cddfca 100644 --- a/.claude/checklists/implementation-checklist.md +++ b/.claude/checklists/implementation-checklist.md @@ -15,6 +15,7 @@ This checklist is updated after each epic's lessons-learned sync (see `/epic-clo - [ ] **Empty states**: Every list/table view must use the shared `EmptyState` component when data is empty. Never show a blank page or raw "No data" text. - [ ] **Loading states**: Every async data fetch must show the shared `Skeleton` component during loading. Never show a blank container or raw "Loading..." text. - [ ] **Fraction-to-percent display**: Decimal-fraction constants (e.g. `CONFIDENCE_MARGINS` where `0.2` means 20%) must be multiplied by 100 before percentage display. This is a recurring finding flagged independently by the architect and product owner (e.g. PR #401). +- [ ] **`Intl.NumberFormat` groups thousands; `toFixed()` never did**: When replacing a `toFixed()` call or manual string-building with a locale formatter, verify the old-vs-new output for values >= 1000, not just per-locale correctness. Grouping separators are an easy-to-miss regression in the *English* output too (PR #1845). ## Frontend — Forms & Validation @@ -31,6 +32,7 @@ This checklist is updated after each epic's lessons-learned sync (see `/epic-clo - [ ] **Semantic token usage**: Use tokens for their intended purpose. Hover backgrounds must use `var(--color-bg-hover)`, never `var(--color-border)` or other non-bg tokens as background values. - [ ] **Dark mode**: All color properties must use CSS custom properties that switch in `[data-theme="dark"]`. Verify no hardcoded `#hex` or `rgb()` values. - [ ] **No colors in inline style props**: Never put color values (`color-mix()`, `backgroundColor: 'var(--token)'`, etc.) in inline `style` props — inline styles bypass stylelint's token enforcement entirely (recurring: PR #792, PR #1681). Use a CSS-module class, or a `data-*` attribute with a CSS attribute selector for dynamic variants. +- [ ] **Never use `:global(.foo)` to style another module's class**: `:global(.foo)` matches only a literal, unhashed class string. A class applied via plain `className={otherStyles.foo}` resolves to a hashed name (`foo_a1b2c`) in real webpack builds, so the entire rule block silently never applies. **This is invisible in Jest** — `identity-obj-proxy` resolves classes to their literal key name, so the selector *does* match in tests and the suite stays green. The correct cross-module technique is `composes: foo from '../other/Other.module.css';` inside a locally-scoped class, then apply that local class (PR #1909). Suspect this whenever spec'd states (hover/focus/at-rest tint/indicator dots) go missing in the real app but pass in tests. ## Frontend — Shared Components @@ -43,6 +45,8 @@ This checklist is updated after each epic's lessons-learned sync (see `/epic-clo - [ ] **FormError usage**: Error display must use the shared `FormError` component. - [ ] **No one-off components**: Every new UI component must be designed as a reusable shared component in `client/src/components/`. - [ ] **Compose shared CSS classes**: CSS utility classes (buttons, modals, loading, empty states, sr-only) must use `composes:` from `client/src/styles/shared.module.css`. Never duplicate shared class definitions. +- [ ] **A new control in an editor must actually reach the export/generation pipeline**: Grep the control's state symbol *outside* its own component before assuming the value is consumed. PR #1959 shipped column-visibility checkboxes that changed only the on-screen preview and never reached the generated PDF; wiring them through was a separate story (#1973). For column show/hide specifically, `DataTable/DataTableColumnSettings.tsx` already exists — check it before building a bespoke checkbox row. +- [ ] **Preview components must match the exporter's constants, not just the design tokens**: When a component previews an exported artifact (PDF, print view), compare each style against the *exporter's* constants. A preview can be fully token-compliant and still not match what the export renders — PR #1959 shipped two grey annotations at two sizes, only one matching the PDF. ## Frontend — React Hooks @@ -68,12 +72,16 @@ This checklist is updated after each epic's lessons-learned sync (see `/epic-clo - [ ] **Parameterized queries**: All database queries must use parameterized values. Never interpolate user input into SQL strings. - [ ] **Wiki documentation**: When adding or changing API endpoints, fields, or query parameters, update `wiki/API-Contract.md` and `wiki/Schema.md` accordingly. This is a recurring architect review finding. - [ ] **Named error codes from acceptance criteria**: When an AC specifies a custom error code (e.g. `ACCOUNT_DEACTIVATED`, `MUTUALLY_EXCLUSIVE_BUDGET_LINK`), return that exact code via `AppError` with the code set explicitly. Do not use a `ValidationError` subclass that emits a generic `VALIDATION_ERROR` (recurring: PR #56, PR #414). +- [ ] **Reused error codes need feature-neutral copy**: When a second feature reuses an existing `ErrorCode`, re-read its message text. `LLM_NOT_CONFIGURED` read "Auto-itemization is not configured" while surfacing in the report wizard, and the `LLM_*` family all said "The extraction service ..." (PR #1916). Written for the first consumer, wrong for the second. +- [ ] **A wiki edit is not published until it is pushed**: `git -C wiki status --short` showing `M API-Contract.md` while `git -C wiki log -1` still equals `origin/master` means the page is written but unpublished — a "documented on the wiki" acceptance criterion is **not** satisfied. Check this on every story carrying a wiki documentation criterion. ## Backend — Data Handling - [ ] **snake_case in DB, camelCase in TS**: Database columns use snake_case; TypeScript code uses camelCase. ORM mapping handles conversion. - [ ] **Cascade deletes**: When deleting parent entities, ensure child records are cleaned up (via FK cascades or explicit deletion). - [ ] **Transaction safety**: Multi-step mutations that must be atomic should use database transactions. +- [ ] **Money is stored in MAJOR units, not cents**: `real` columns and types like `SourceReportInvoice.allocatedAmount` hold `250` meaning EUR 250.00, and feed `Intl` currency formatting directly. The minor-units idiom `(amount / 100).toFixed(2)` understates every figure by 100x — it reached a bank-facing cover letter in PR #1916. For the same reason, `Math.round(x)` on major units rounds to whole currency units, not cents: the correct form is `Math.round(x * 100) / 100`. Treat a `// Round to nearest cent` comment sitting above a bare `Math.round(x)` as a defect. +- [ ] **Derived aggregates and their per-item components must agree**: When an exclusion rule adjusts an aggregate, verify the per-item values sent alongside it were adjusted by the same rule. PR #1916 subtracted excluded portions from `totalAmount` but emitted each invoice's raw `allocatedAmount`, so the parts visibly summed to more than the stated total. ## Shared — TypeScript Conventions @@ -95,6 +103,16 @@ This checklist is updated after each epic's lessons-learned sync (see `/epic-clo - [ ] **E2E text locators after label changes**: When a production PR renames a UI label, update all E2E test locators that match that text. Regex locators like `/hide linked/i` silently break when the label changes to "Hide already-linked documents" (no contiguous match). Prefer `data-testid` attributes for stability; when using text regex, keep the pattern broad enough to survive minor rewording (e.g. `/hide.*linked/i`). - [ ] **E2E modal interception**: When a feature wraps an existing user interaction inside a new modal (e.g. file selection → "Add photo details" modal before upload), ALL existing E2E tests that exercise the downstream behavior (upload queue, photo card appearance) must be updated to dismiss the modal first. The dev-team-lead must flag this in `[MODE: review]` if affected E2E tests are not updated in the same PR. - [ ] **E2E flake-avoidance patterns**: Timing-sensitive E2E work (canvas coordinates, `test.slow()` timeouts, post-reload locale waits, shard redistribution, stale cache-warmup CI) must follow the patterns in `.claude/agent-memory/e2e-test-engineer/flake-patterns.md`. +- [ ] **Read test titles *against* their assertions**: A test whose title states the contract but whose expectation encodes what the code currently does is a defect, not coverage. `reportContentGenerationService.test.ts` scenario 5 was titled "rounded to the nearest cent" and asserted `733` for a true `733.335` (PR #1916). When verifying a fix round, read the **deleted** test lines too — a relaxed assertion and a legitimate cleanup have the same diff shape. +- [ ] **Skipped is not passed**: When a CI gate aggregates job results, a `skipped` dependency must not be read as success unless the skip reason is "nothing to test". A failed upstream job also skips its dependents — this let `E2E Gates` report green with all 16 shards skipped on promotion PR #2041 (Issue #2043). +- [ ] **Confirm an E2E failure is a regression before attributing it to the PR**: Compare against the previous run on the same branch (`gh run list --branch `, then `gh run view --json jobs`) before treating a shard failure as caused by the change under review. + +## LLM & Prompt Assembly + +- [ ] **A constraint stated only in the prompt is not a guarantee**: For every behavioural rule the prompt asserts, identify what fails if the model ignores it. `prompts.test.ts`-style tests pin that the *instruction exists* — that is real coverage of the instruction and zero coverage of the outcome. #1932's plain-prose rule had no enforced counterpart and the render path is literal, so `**bold**` reached a bank-facing PDF (#1952). +- [ ] **Prompt content is usually untested**: Whenever money or other formatted numerics reach an LLM, assert the *rendered value* in the prompt, not merely that a label appears. Existing tests asserted `toContain('Invoice ID: inv-1')` and never a single amount (PR #1916). +- [ ] **Match the coerce-vs-reject policy the field already has**: If a validator truncates rather than throws for a field, a stricter failure mode for a *milder* violation is incoherent. One generation often yields several unrelated outputs — rejecting the whole response over a cosmetic defect in one field discards correct, expensive output and may fail identically on retry (#1952 ruled strip, not reject). +- [ ] **When the hardening is a text transform, false positives are the risk**: Domain punctuation collides with markup characters (`Pos. 3 - Dachstuhl`, `Rechnung #2024-117`, `Beträge < 500 EUR`, footnote `*`). A mangled reference number is worse than the markup, because nothing signals a character went missing. Write as many byte-identical-passthrough cases as stripping cases, plus "if the transform empties a non-empty value, keep the original". ## i18n — Translations