diff --git a/.claude/agent-memory/e2e-test-engineer/MEMORY.md b/.claude/agent-memory/e2e-test-engineer/MEMORY.md index d8696067a..ed299364a 100644 --- a/.claude/agent-memory/e2e-test-engineer/MEMORY.md +++ b/.claude/agent-memory/e2e-test-engineer/MEMORY.md @@ -15,6 +15,7 @@ - [document-linking-and-photos-e2e.md](document-linking-and-photos-e2e.md) — document-linking (Paperless), photo picker/upload flows, orientations. - [photo-annotator-e2e.md](photo-annotator-e2e.md) — Konva canvas annotator (post-SVG-migration rewrite, touch/pointer-event handling). - [diary-e2e.md](diary-e2e.md) — Construction Diary feature: drafts, forms, list/detail, mode filters, UAT-fix history. +- [issue-2060-diary-infinite-scroll.md](issue-2060-diary-infinite-scroll.md) — Diary pager → IntersectionObserver infinite scroll: footer DOM shape/render conditions, POM additions, 11-scenario test breakdown. - [print-and-i18n.md](print-and-i18n.md) — print-mode E2E and i18n locale-switch testing. - [milestones-e2e.md](milestones-e2e.md) — Milestones feature POM and selectors. - [searchpicker-mobile-1708.md](searchpicker-mobile-1708.md) — SearchPicker mobile dropdown-anchor regression (Issue #1708). diff --git a/.claude/agent-memory/e2e-test-engineer/diary-e2e.md b/.claude/agent-memory/e2e-test-engineer/diary-e2e.md index 18a532cfd..18b46d6a9 100644 --- a/.claude/agent-memory/e2e-test-engineer/diary-e2e.md +++ b/.claude/agent-memory/e2e-test-engineer/diary-e2e.md @@ -5,6 +5,13 @@ metadata: type: project --- +## Diary pager removed → infinite scroll (Issue #2060, 2026-09-04) — `diary-list.spec.ts` + +`prevPageButton`/`nextPageButton` are GONE from `DiaryPage` POM (`?page=` URL param no longer read at +all, even as a legacy bookmark). Replaced by `loadMoreButton`/`endOfListMessage`/ +`infiniteScrollSentinel`/`footerError` + `scrollToLoadMore()`. Full detail, DOM shape, and the +11-scenario test breakdown: see [issue-2060-diary-infinite-scroll.md](issue-2060-diary-infinite-scroll.md). + ## Diary default filter mode = 'manual' (fix/1781, 2026-06-22) — `diary-r2-uat.spec.ts`, `diary-list.spec.ts` - Default mode chip changed from `all` → `manual`. Test renamed: `'"Manual" mode chip is aria-pressed=true by default (no filterMode URL param)'`. Assertions flipped: `allChip` → `false`, `manualChip` → `true`, `automaticChip` → `false`. diff --git a/.claude/agent-memory/e2e-test-engineer/flake-patterns.md b/.claude/agent-memory/e2e-test-engineer/flake-patterns.md index 553368915..885fa05be 100644 --- a/.claude/agent-memory/e2e-test-engineer/flake-patterns.md +++ b/.claude/agent-memory/e2e-test-engineer/flake-patterns.md @@ -11,6 +11,19 @@ When interacting with a Konva `` (or any element centered inside a flex `test.slow()` triples the project-level `expect.timeout` (e.g. 15s → 45s), but an explicit `{ timeout: 15_000 }` override on an individual `expect(...).toBeVisible()` _negates_ that tripling, capping the wait at the literal value. Under heavy parallel CI load this causes intermittent failures even though the app and API are correct. When a test calls `test.slow()`, either omit per-assertion timeout overrides (let the tripled budget apply) or set them to the full tripled value (e.g. `45_000`). Prefer awaiting the gating network response (`waitForResponse` registered _before_ the triggering click) over a fixed wall-clock timeout. +## Multiple equivalent triggers racing each other (IntersectionObserver + button) + +When a hook exposes one action reachable via two independent triggers (e.g. `useInfiniteScroll`'s +`loadMore()`, callable both by an `IntersectionObserver` auto-firing and by a "Load more" button's own +click/keypress — Issue #2060), a test that wants to isolate and prove ONE trigger path deterministically +should stub out the OTHER trigger's browser API via `page.addInitScript()` before navigating, rather than +trying to out-race it with careful sequencing. `IntersectionObserver` in particular evaluates its +target's geometry the instant `observe()` is called — it does not need an actual scroll event, so a +short/minimal mocked page (or an off-screen-focus auto-scroll-into-view) can make it fire before a +slower Playwright action (like `.focus()`, which does its own actionability/scroll-into-view work) even +completes, deterministically unmounting whatever the successful fetch causes to disappear. Full +incident + fix in [issue-2060-diary-infinite-scroll.md](issue-2060-diary-infinite-scroll.md). + ## Locale timing after page reload Do not use `page.waitForResponse(GET /api/users/me/preferences)` to gate assertions after `page.reload()` — the response may fire before `reload()` is called (from React's async post-load mounts), leaving the locale update unobserved. Use `await page.waitForLoadState('networkidle')` after `reload()` instead to ensure all async requests, including the preferences fetch, have settled before asserting on locale-dependent UI. diff --git a/.claude/agent-memory/e2e-test-engineer/issue-2060-diary-infinite-scroll.md b/.claude/agent-memory/e2e-test-engineer/issue-2060-diary-infinite-scroll.md new file mode 100644 index 000000000..5dc339863 --- /dev/null +++ b/.claude/agent-memory/e2e-test-engineer/issue-2060-diary-infinite-scroll.md @@ -0,0 +1,177 @@ +--- +name: issue-2060-diary-infinite-scroll +description: Diary pager replaced by IntersectionObserver infinite scroll (Issue #2060) — hook/component DOM shape, footer render conditions, POM additions, dedupe/error/filter-reset test patterns. +metadata: + type: project +--- + +## What changed (frontend-developer + ux-designer, PR for #2060) + +- New shared hook `client/src/hooks/useInfiniteScroll.ts` (generic ``) — owns `IntersectionObserver` + (600px `rootMargin`, constant `INFINITE_SCROLL_LOOKAHEAD_PX`), `status: 'idle'|'loading'|'error'|'done'` + state machine, in-flight dedupe (`inFlightRef`), `resetKey`-driven reset effect. +- New shared component `client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.tsx` (+ css) — + purely presentational, driven by `status`/`hasMore`/`sentinelRef`/`onLoadMore`/`onRetry`. +- `DiaryPage.tsx`: pager fully removed (`handlePageChange`, `?page=` URL param, prev/next buttons all + gone). No `searchParams.get('page')` reference exists anywhere anymore — a stale `/diary?page=3` + bookmark is silently ignored, not stripped from the URL, not read. +- `pagination.*` i18n keys already removed from `client/src/i18n/en/diary.json`; new `infiniteScroll.*` + block added (see keys below). + +## DOM shape / render conditions (verified against actual source, not just the spec) + +- `InfiniteScrollFooter` is rendered by `DiaryPage` **only when `entries.length > 0`** — for a + zero-item response there is no footer, no sentinel, no button, no end-of-list message at all + (confirms AC9 "empty state, no footer" behavior structurally, not just visually). +- Inside the footer, in DOM order: sentinel (`data-testid="diary-sentinel"`, 0×0, `aria-hidden`) → + `FormError` banner (`role="alert"`, only when `status==='error'`) → **either** the button + (`data-testid="diary-load-more-button"`) **or** the end-of-list row + (`data-testid="diary-end-of-list"`) — button and end-of-list are mutually exclusive + (`status === 'done' ? endOfList : button`), but the error banner and the button coexist + simultaneously on `status==='error'` (button relabels to "Retry"). +- **PR follow-up for #2060 genericized `InfiniteScrollFooter` with a `testIdPrefix` prop** + (`DiaryPage` passes `testIdPrefix="diary"`, default `'infinite-scroll'`) so the component can be + reused by other list views later. All testids become `${testIdPrefix}-*`. `-load-more-button`, + `-end-of-list` happened to keep the exact same literal strings under the `diary` prefix, but the + **root footer container's testid changed from the old hardcoded `"diary-infinite-scroll-footer"` + to `"diary-footer"`**, and the sentinel from `"infinite-scroll-sentinel"` to `"diary-sentinel"`. + This was NOT what the coordinator's follow-up message described (it only flagged the sentinel + rename, and asserted the footer testid was "preserved exactly") — caught the footer-id + discrepancy by reading `InfiniteScrollFooter.tsx`'s actual `git diff` rather than trusting the + summary, since `DiaryPage.ts`'s `footerError` locator was scoped through that exact testid + (`getByTestId('diary-infinite-scroll-footer').getByRole('alert')`) and would have silently found + zero elements in every error/retry test otherwise. **Lesson: verify testid claims in ANY + handoff message (coordinator, dev-team-lead, frontend-developer) against the actual component + diff before updating a POM — a summary that's right about most of a rename can still be wrong + about one string.** Same standalone-loading-status-row removal check (`styles.statusRow` deleted, + now only the button's own inline `Spinner` indicates loading) was a true no-op for this spec — + confirmed via grep that no test ever referenced `statusRow` or asserted on a second loading + indicator; every loading-adjacent assertion in `diary-list.spec.ts` goes through + `diaryPage.loadMoreButton` itself. +- **The button is the SAME DOM node across idle/loading/error** — never unmounted until `status` + becomes `'done'`. It self-labels: idle→"Load more", loading→disabled+inline spinner, error→"Retry". + This means `diaryPage.loadMoreButton` is a stable locator through an entire failure→retry cycle — + no need to re-query after a state change. +- Root testid on the whole footer container: `data-testid="diary-infinite-scroll-footer"` — use this + to scope `getByRole('alert')` so it never collides with the page-level `shared.bannerError` + (top-of-page banner, only shown when `error && entries.length === 0`, i.e. FIRST batch fails — + a completely different code path/DOM node than the footer's error, which only ever appears once at + least one batch has already succeeded). +- `hasMore` prop is passed to `InfiniteScrollFooter` but not read inside it (component only reads + `status`) — `hasMore` is consumed by the parent for other purposes; don't assert on it directly via + DOM, assert on `status`-derived visible elements instead. + +## POM additions (`e2e/pages/DiaryPage.ts`) + +`loadMoreButton`, `endOfListMessage`, `infiniteScrollSentinel`, `footerError` (scoped via +`getByTestId('diary-infinite-scroll-footer').getByRole('alert')`), plus a `scrollToLoadMore()` helper +mirroring the existing `waitForLoaded()` `Promise.race` pattern (races a generic diary-entries response +against the end-of-list locator — does NOT filter by status 200, since it must also resolve correctly +when the triggered batch fails). Removed: `prevPageButton`/`nextPageButton` (verified via repo-wide +grep that no OTHER page object shares these field names on `DiaryPage` — the identically-named fields +on `VendorsPage`/`HouseholdItemsPage`/`InvoicesPage`/`WorkItemsPage` are unrelated `DataTable`-based +pager locators, explicitly out of scope per the issue and untouched). + +## Test file (`e2e/tests/diary/diary-list.spec.ts`, "Infinite scroll (Scenario 7)") + +11 tests replacing the old 2-test "Pagination (Scenario 7)" block: + +1. Auto-load on scroll (`@smoke @responsive` — the smoke tag matters: this is the fast regression + guard for the whole rework, and `@responsive` means it re-runs unmodified on tablet+mobile + projects, which is the literal regression test for the original "nothing happens on mobile" bug — + no separate per-viewport test needed, the existing project/grep matrix does it for free). +2. Keyboard-only "Load more" (focus + Enter, no scroll) + focus-visible `box-shadow !== 'none'` check + in both light and dark mode (pattern copied from `reportWizardEditableContent.spec.ts` Scenario 13 + — `.focus()` + `getComputedStyle(el).boxShadow`, not `outlineStyle`, since `:focus-visible` here + uses `box-shadow: var(--shadow-focus)` like every other `shared.module.css` button). +3. Full pager removal — `page.getByTestId('prev-page-button')`/`next-page-button` `.toHaveCount(0)`. +4. Fast repeated scroll dedupe (best-effort, explicitly labeled as such) — realistically this passes + because `IntersectionObserver` only fires its callback on a threshold crossing, not per scroll + frame, so repeated identical `scrollTo(bottom)` calls naturally coalesce into one callback + regardless of the hook's own `inFlightRef` guard; the artificial 500ms route delay + request + counter just makes the assertion meaningful/timeable, it does not really stress the dedupe guard + itself. Don't oversell this test's rigor if revisiting it. +5. End-of-list immediately on a single-page dataset, exactly one request total. +6. Empty dataset → no footer/sentinel/button/end-of-list at all (structural, not just visual). +7. Filter change mid-scroll discards old batch and loads disjoint new set + updates header count — + route handler dispatches on the `type` query param (`work_item_status` → automatic single-item + set, else → manual 2-page set) since clicking `mode-filter-automatic` changes `type`, not a + dedicated "mode" query param. +8. Search reset — URL gains `q=` and never gains `page=`, checked both after loading page 2 AND + after the search. +9. Batch failure → footer `role="alert"` + button relabels to "Retry"; scrolling again while errored + does NOT re-issue the request (asserted via a before/after route-hit counter, not just "eventually + consistent" waiting); retry re-fetches the same page and succeeds with the entry testid appearing + with `toHaveCount(1)` (dedup proof) and end-of-list following since it was the last page. +10. Legacy `/diary?page=3` bookmark — asserted via mocked route capturing the actual `page` query + param sent to the backend (`requestedPages[0] === '1'`), NOT by checking the URL bar, since the + app never strips `?page=3` from the address bar — it just never reads it. Don't assert the URL is + "cleaned"; that's not a real behavior and would be an assertion-that-passes-on-nothing risk if + written against a `.not.toContain('page=3')` check that happens to also pass because of an + unrelated bug. +11. Dark mode — one combined test walking error→retry→done inside a single `data-theme="dark"` + session (cheaper than 3 separate dark-mode tests), asserting `role="alert"` visibility, the + button's className contains `btnSecondary` (proves the shared button style class applied — CSS + Modules classnames retain the literal source name as a substring in this codebase's build, same + convention as the pervasive `[class*="..."]` locator idiom used everywhere else in this suite), + and no horizontal overflow at each state transition. + +## CI-deterministic failure fixed: `IntersectionObserver` auto-fire races a keyboard-only test + +**Symptom**: "Load more button loads the next batch via keyboard alone, with no scroll" failed on +both attempt and retry — `expect(locator).toBeFocused()` → "element(s) not found" for +`diary-load-more-button`, immediately after `.focus()` succeeded on the same locator. + +**Root cause (reasoned from source, no live browser available to confirm empirically — see +[sandbox-live-verification.md](sandbox-live-verification.md))**: the button can only vanish via +`status === 'done'` unmounting it. Two independent mechanisms can each cause that to happen between +`.focus()` and the very next assertion, given the test's undelayed 2-page mock: (1) `IntersectionObserver` +evaluates its target's geometry immediately when `observer.observe()` runs in the hook's effect — it +does not require an actual scroll event to fire, only that the sentinel is already within the 600px +`rootMargin` at observe-time, which 25 minimal-content mock cards can easily satisfy on a 1920×1080 +viewport; and/or (2) focusing (or Tab-ing to) an off-screen element causes the browser/Playwright to +scroll it into view as a normal part of standard focus handling, which can itself bring the adjacent +sentinel into the observer's bounds. Either path calls the identical `loadMore()` the hook exposes, +resolves the undelayed mock instantly, flips `hasMore` false (2-page mock), and unmounts the button — +all before the test's own `toBeFocused()` assertion round-trips over CDP. This is **not** a production +bug: both the auto-scroll path and the button's own activation intentionally share one `loadMore()` +function per the ux-designer spec, so racing is an accepted (if here, test-inconvenient) consequence of +that design, not a defect to file against `useInfiniteScroll`/`InfiniteScrollFooter`. + +**Fix (test-only)**: `page.addInitScript()` before `diaryPage.goto()` to replace `window.IntersectionObserver` +with a no-op stub class (full interface implemented — `root`/`rootMargin`/`thresholds`/`disconnect`/ +`observe`/`unobserve`/`takeRecords` — so no `@ts-expect-error` needed) for the duration of that one test. +This runs before any page script (CDP `addScriptToEvaluateOnNewDocument` semantics), so the hook's +`useEffect` constructs the stub instead of a real observer, and `loadMore()` can **only** be triggered by +the button's own click/Enter handler for the rest of the test — cleanly isolating the keyboard-activation +code path from the auto-scroll path instead of trying to out-race it. General pattern: whenever a test +needs to prove one trigger path of a hook that exposes multiple equivalent triggers (observer + button, +in this case), stub the OTHER trigger's browser API rather than fighting timing/geometry assumptions +about mock content height or CI viewport specifics. + +**Left un-hardened (deliberately, not currently failing)**: "Auto-load on scroll" (Scenario 7) and +"Dark mode" (Scenario 7) both assert page-2-entry-count `toHaveCount(0)` or similar _before_ their own +explicit `scrollTo`/`scrollToLoadMore()` call — in principle exposed to the same "observer already fired +at mount" race described above, since nothing prevents the sentinel from already being in view before +those tests' own trigger runs. Not touched because they are not currently reported failing (their +`waitForLoaded()` → immediate-next-assertion gap is apparently narrow enough in practice to not lose the +race, unlike `.focus()`'s slower actionability-check path) and rewriting a passing test carries its own +regression risk. If either ever starts failing with the same "element(s) not found"/"expected count 0, +got N" signature, apply the same `IntersectionObserver` stub (but only where the test intends to drive +the fetch via explicit action, not for tests whose whole point IS the auto-scroll path itself). + +## i18n keys (`client/src/i18n/en/diary.json`, `infiniteScroll` namespace) + +`loadMoreButton` ("Load more"), `loadingMore` ("Loading more entries…"), `loadingMoreAriaLabel`, +`retryButton` ("Retry"), `errorMessage` ("Failed to load more entries."), `endOfList`, +`endOfListAnnouncement`, `batchAppendedAnnouncement` ("{{count}} more entries loaded"), +`batchAppendedAndEndAnnouncement`. All `pagination.*` keys were already gone by the time E2E work +started — no dead-key cleanup needed on the E2E side. + +## Playwright project mechanics relevant here + +`tablet`/`mobile` projects (`e2e/playwright.config.ts`) only run tests matching `grep: /@responsive/`; +`desktop` runs everything. Tagging a test `@responsive` is sufficient to get it repeated verbatim on +all 3 viewports — no manual `page.setViewportSize()` loop needed, and this is the established pattern +throughout `diary-list.spec.ts` (Scenarios 1, 2, 4, 6, 10, 11 all do this already). `npm run +test:e2e:smoke` = `--grep @smoke --project desktop` only. diff --git a/.claude/agent-memory/product-architect/MEMORY.md b/.claude/agent-memory/product-architect/MEMORY.md index c704f9737..3c33cb444 100644 --- a/.claude/agent-memory/product-architect/MEMORY.md +++ b/.claude/agent-memory/product-architect/MEMORY.md @@ -2,7 +2,7 @@ ## Topic Files -- [Recurring patterns & traps](recurring-patterns.md) — polymorphic FK cleanup, XOR CHECK vs SET NULL, forked-function drift, test smells, cross-layer contract drift, ajv `anyOf`, N+1 sites, async writes surviving state resets, the revert test for fixes that only relax an invariant — re-run it yourself on round 2 (#1968/PR #2002), cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), reinstated-producer-vs-negative-guards (#1965), AC reversal by a polish issue (#1959), amount-threshold booleans narrowing status-existence booleans (#1897), prettier is not CI-gated, single-occurrence delimiter guard tests + German ordinals vs list markers + pre-validating regex fix specs (#1952), `Pick<>` is not a forcing function + caller-supplied monotonic seq reintroduces the ref + cascade tables smuggle behaviour changes + neutralised-trigger-left-in-code (#1947), tier factory only forces the cases that spread it (#1988), regex mirroring a third-party grammar + `parseInt` trailing garbage + env vars documented in four places (#1970, PR #1989), guard-deleted-because-it-looked-like-the-bug + rate-limit identity-check gate + `request.ip` nullability types-lie + CVE test needs a negative control (#1995, PR #1998), prettier config resolution is path-based so /tmp baseline checks lie + wiki is not prettier-ignored + document the invariant not the absence-of-code (#1998 wiki pass), comment-refreshed-but-assertion-left-behind + contract inversion makes pre-existing negatives unconditional + surgical tagging misses read-only value nodes (#1910, PR #2004 r2), the-prop-landed-is-not-the-prop-is-wired + redundant-tag-a-test-asserts + `aria-label` cannot be language-tagged (#1910, PR #2004 r3), `count >= 1` + all-match is a per-instance assertion masquerading as coverage — revert each call site individually, use `toBe(N)` (#1910, PR #2004 r4), untyped E2E route fixtures drift from shared contracts + consumer early-return masks an incomplete fixture + duplicate Playwright route globs are an ordering dependency (#2005, PR #2006), widen-then-`as`-narrow defeats union exhaustiveness + a hardening PR falsifies its own ADR in four predictable places + key-echo fixtures are non-discriminating (#2001, PR #2007), a revert test can prove a _different_ proposition than the one it licenses + implementing a documented rule for the first time is when you learn the rule is wrong + three forked `collectAllStrings` copies (#2003, PR #2008), **a spec's own "purely additive, no E2E changes needed" claim is the tell that existing tests encoded the OLD derivation — a derivation change is never purely additive** + re-seeding a fixture without re-deriving its arithmetic expectation + `toContainText(' (label)')` breaks the moment a badge is rendered between them, so assert the note locator not sibling-node adjacency (#1911, PR #2015), **the two-command env-var drift sweep (`getValue(` read-set vs `^| \`VAR\`` doc-set, then grep the enablement sentences separately — a stated variable *count* is a second drift surface the name diff cannot see) + wiki tables are char-width-padded so measure with python `len()` not `awk length()` (em-dashes are 3 bytes) + two open findings: the `BACKUP_DIR` default/gate is wrong on Architecture.md and API-Contract.md's `splitKind` table is a latent `format:check` failure (#1992, wiki `e14bcbe`)**, **operator-facing prose is a behavioural claim a validator must back — hyperlinking `vercel/ms` while enforcing a regex subset, a caution box falsified by `parseInt` leniency, and `trustProxy: 1` being a hop count not "trust all proxies" (#1990, PR #2027)**, **fuzz the verbatim ports when a doc comment carries an induction proof (a hand-trace only re-reads the author's argument) + a safety argument phrased as a *ratio* is falsified by any clamp in the chain (#1940, PR #2032)**, **flex `gap` + child `margin` are additive not collapsing (bit twice in one PR — code AND the spec reviewing it) + a cohesive prop group modelled as N independent optionals + "leaves N chars for X" comments invite a guard test that pins a fiction: check whether X is bounded at all and whether the consumer clips or paginates (#1941, PR #2033)**, **a mutation count is not evidence of independent coverage — diff a new test's assertion body against its neighbours before trusting its title, esp. a negative-dependency title whose body never names the dependency + when a structural guard would have to re-encode the coupling under removal, a reason-carrying comment IS the right tool (#1953, PR #2035)** +- [Recurring patterns & traps](recurring-patterns.md) — polymorphic FK cleanup, XOR CHECK vs SET NULL, forked-function drift, test smells, cross-layer contract drift, ajv `anyOf`, N+1 sites, async writes surviving state resets, the revert test for fixes that only relax an invariant — re-run it yourself on round 2 (#1968/PR #2002), cross-reference rot in documented-bound comments (#1939), usePreferences per-instance store + serialized-write-queue review (#1955), capability-retained-but-producer-removed (#1959), reinstated-producer-vs-negative-guards (#1965), AC reversal by a polish issue (#1959), amount-threshold booleans narrowing status-existence booleans (#1897), prettier is not CI-gated, single-occurrence delimiter guard tests + German ordinals vs list markers + pre-validating regex fix specs (#1952), `Pick<>` is not a forcing function + caller-supplied monotonic seq reintroduces the ref + cascade tables smuggle behaviour changes + neutralised-trigger-left-in-code (#1947), tier factory only forces the cases that spread it (#1988), regex mirroring a third-party grammar + `parseInt` trailing garbage + env vars documented in four places (#1970, PR #1989), guard-deleted-because-it-looked-like-the-bug + rate-limit identity-check gate + `request.ip` nullability types-lie + CVE test needs a negative control (#1995, PR #1998), prettier config resolution is path-based so /tmp baseline checks lie + wiki is not prettier-ignored + document the invariant not the absence-of-code (#1998 wiki pass), comment-refreshed-but-assertion-left-behind + contract inversion makes pre-existing negatives unconditional + surgical tagging misses read-only value nodes (#1910, PR #2004 r2), the-prop-landed-is-not-the-prop-is-wired + redundant-tag-a-test-asserts + `aria-label` cannot be language-tagged (#1910, PR #2004 r3), `count >= 1` + all-match is a per-instance assertion masquerading as coverage — revert each call site individually, use `toBe(N)` (#1910, PR #2004 r4), untyped E2E route fixtures drift from shared contracts + consumer early-return masks an incomplete fixture + duplicate Playwright route globs are an ordering dependency (#2005, PR #2006), widen-then-`as`-narrow defeats union exhaustiveness + a hardening PR falsifies its own ADR in four predictable places + key-echo fixtures are non-discriminating (#2001, PR #2007), a revert test can prove a _different_ proposition than the one it licenses + implementing a documented rule for the first time is when you learn the rule is wrong + three forked `collectAllStrings` copies (#2003, PR #2008), **a spec's own "purely additive, no E2E changes needed" claim is the tell that existing tests encoded the OLD derivation — a derivation change is never purely additive** + re-seeding a fixture without re-deriving its arithmetic expectation + `toContainText(' (label)')` breaks the moment a badge is rendered between them, so assert the note locator not sibling-node adjacency (#1911, PR #2015), **the two-command env-var drift sweep (`getValue(` read-set vs `^| \`VAR\``doc-set, then grep the enablement sentences separately — a stated variable *count* is a second drift surface the name diff cannot see) + wiki tables are char-width-padded so measure with python`len()`not`awk length()`(em-dashes are 3 bytes) + two open findings: the`BACKUP_DIR`default/gate is wrong on Architecture.md and API-Contract.md's`splitKind`table is a latent`format:check`failure (#1992, wiki`e14bcbe`)**, **operator-facing prose is a behavioural claim a validator must back — hyperlinking `vercel/ms`while enforcing a regex subset, a caution box falsified by`parseInt`leniency, and`trustProxy: 1`being a hop count not "trust all proxies" (#1990, PR #2027)**, **fuzz the verbatim ports when a doc comment carries an induction proof (a hand-trace only re-reads the author's argument) + a safety argument phrased as a *ratio* is falsified by any clamp in the chain (#1940, PR #2032)**, **flex`gap`+ child`margin`are additive not collapsing (bit twice in one PR — code AND the spec reviewing it) + a cohesive prop group modelled as N independent optionals + "leaves N chars for X" comments invite a guard test that pins a fiction: check whether X is bounded at all and whether the consumer clips or paginates (#1941, PR #2033)**, **a mutation count is not evidence of independent coverage — diff a new test's assertion body against its neighbours before trusting its title, esp. a negative-dependency title whose body never names the dependency + when a structural guard would have to re-encode the coupling under removal, a reason-carrying comment IS the right tool (#1953, PR #2035)**, **a staleness guard protects only the value it returns, never the side effects the injected callback performs on the way there — so the contract must SAY side effects inside`fetchPage`are unguarded + a reset effect clears the data but not the counters describing it (diff`useState`decls against the reset's setters) + a shared component born inside one feature keeps that feature's i18n namespace and testids, and mixed generality among its own identifiers is the tell (#2060, PR #2063)**, **r2 of #2063: prescribe the REQUIREMENT not the implementation — my "do NOT widen the page type" forbade the better fix (generic`M`+`onPageApplied`inside the epoch guard), and prop-injection de-features a component better than a`common:`namespace does + hand-rolled`Singular`/`Plural` `t()`suffixes vs native`_one`/`_other` (no live bug in en/de, but a call-site binary split can't express a >2-form CLDR locale, and a dynamic key defeats every static audit) + a precedent inside the file you're editing is weaker evidence than the convention across sibling namespaces** - [Dual-rail aggregation](dual-rail-aggregation.md) — Rail A/B tagged-deposit invariants (#1891/PR #1894), residual-denominator rule, isSplit UNION - [Source-report split inference](source-report-split-inference.md) — budgetLines[]/deposits[] are this-source-scoped so array-shape gates are proxies; **`splitKind` SHIPPED #1911/PR #2015** incl. the ≠S-per-arm predicate, the residual arithmetic proving `(less deposit)` in both directions, the UNION-dedup/`COUNT(*)` trap, and why `isSplit` must be retained as an independent cross-check; pdfmake `'2*'` width trap; wiki + shared type JSDoc fixed (#1914, #1917/PR #1994) - [Story reviews](story-reviews.md) — per-story and per-PR review log diff --git a/.claude/agent-memory/product-architect/recurring-patterns.md b/.claude/agent-memory/product-architect/recurring-patterns.md index 2560ea5e8..02776402b 100644 --- a/.claude/agent-memory/product-architect/recurring-patterns.md +++ b/.claude/agent-memory/product-architect/recurring-patterns.md @@ -1307,7 +1307,7 @@ next agent away from it. Same page, same round: a quoted constant reference (`MA → per-subset `usageChunkChars`) and a "this function **hangs** on `maxChars <= 0`" claim that a prior fix had already turned into a throw. -**How to apply:** on any PR that *removes* a limitation, grep the wiki for the limitation's own +**How to apply:** on any PR that _removes_ a limitation, grep the wiki for the limitation's own statement — not just for the API/schema surface the diff touches. Constraint prose lives in ADR Consequences and "sharp edge" sections that no schema/contract diff would ever point you at. Bonus tell: if the issue body cites a wiki constraint as its motivation, that exact paragraph is the one @@ -1333,13 +1333,12 @@ filter, which converts a silent omission into a compile error. Both auth env-var tables documented `OIDC_REDIRECT_URI`; `server/src/plugins/config.ts` never reads it. The gate is three vars (`config.ts:142`), and the redirect URI is built per request at -`server/src/routes/oidc.ts:45` as `externalUrl || \`${request.protocol}://${request.host}\`` + -`/api/auth/oidc/callback`. Fixed wiki-only. +`server/src/routes/oidc.ts:45` as `externalUrl || \`${request.protocol}://${request.host}\``+`/api/auth/oidc/callback`. Fixed wiki-only. **The cheap sweep** (run it whenever you touch an env-var table, it is two commands): `grep -oE "getValue\('[A-Z0-9_]+'\)" server/src/plugins/config.ts` gives the authoritative read-set; -`grep -oE '^\| \`[A-Z][A-Z0-9_]+\`' wiki/.md` gives the documented set; `comm -23` the sorted -pair. Then `grep -n "enabled when\|If unset\|If either is missing"` — **any sentence that states a +`grep -oE '^\| \`[A-Z][A-Z0-9_]+\`' wiki/.md`gives the documented set;`comm -23`the sorted +pair. Then`grep -n "enabled when\|If unset\|If either is missing"` — **any sentence that states a variable count or an enablement gate is a second, independent drift surface** that the name-level diff cannot see. That is how the "all four OIDC variables" sentence survived. @@ -1360,13 +1359,13 @@ cannot see. That is how the "all four OIDC variables" sentence survived. callback URL differently: `oidc.ts:45` uses `externalUrl || request-origin`, `oidc.ts:106` uses the request origin unconditionally. openid-client sends the **token-request** `redirect_uri` derived from the URL you hand `authorizationCodeGrant` (`index.js:974`, `redirectUri = stripParams(currentUrl)`), - so with `EXTERNAL_URL` set and `TRUST_PROXY` unset the two legs send *different* `redirect_uri` + so with `EXTERNAL_URL` set and `TRUST_PROXY` unset the two legs send _different_ `redirect_uri` values and the provider rejects the exchange with `invalid_grant` (RFC 6749 §4.1.3 requires them to be identical). **Backend fix, not a wiki fix** — the wiki paragraph deliberately documents only the login leg, because documenting leg 2's derivation as intended behaviour would enshrine the bug. **Wiki table mechanics (bit me, cost two rounds):** these tables are prettier-padded so every row is -the *same character width* (auth tables 131; API-Contract Deviation Log 2153 = cells 10/74/780/1276). +the _same character width_ (auth tables 131; API-Contract Deviation Log 2153 = cells 10/74/780/1276). Measure with python `len()`, **never `awk length()`** — awk counts bytes here, and the em-dashes that are everywhere in this wiki make a correctly-padded row read 2 bytes long per dash, which looks like a padding bug and isn't. Editing a Deviation Log cell means re-padding the cell to its exact column @@ -1376,12 +1375,12 @@ width, not just swapping the sentence. Reviewed a docs-site-only PR for technical accuracy (no architecture surface). Everything structural checked out — defaults, the startup-failure chain, the setup route's hardcoded 5/15min. The defects -were all in the gap between *what the prose promises* and *what the code validates*: +were all in the gap between _what the prose promises_ and _what the code validates_: - **Hyperlinking a third-party library while enforcing a strict subset of it.** Copy said "`AUTH_RATE_LIMIT_WINDOW` in [`ms`](vercel/ms) duration format". `config.ts:364` is a hand-rolled regex that rejects `1y`, bare `900000`, `.5h`, `1 msec` — all valid `ms` input shown in that README. - Paired with a caution box promising a hard startup failure, the link *invites* the crash it warns + Paired with a caution box promising a hard startup failure, the link _invites_ the crash it warns about (and the library is named `ms`, so "ms format" reads as "milliseconds" to many). This is the reader-facing twin of the #1970 "regex mirroring a third-party grammar" trap: **when docs link the upstream spec, the regex's subset becomes a documentation bug, not just a code smell.** Same loose @@ -1398,11 +1397,11 @@ were all in the gap between *what the prose promises* and *what the code validat - **The rate-limit key is per-/64 for IPv6**, not per-address (`rateLimitPlugin.ts` `IPV6_SUBNET = 64`), so "keys on the client's IP" is imprecise and shared-bucket guidance isn't NAT-only. -**Method that worked:** execute the validator's regex against every example the prose gives *and* +**Method that worked:** execute the validator's regex against every example the prose gives _and_ against examples the linked upstream spec gives — the second set is where the mismatch lives. Verify a "fails at startup" claim by walking to the entrypoint (`server.ts` had no try/catch around -`buildApp()`; the only `try` wrapped `app.listen`) and confirming intermediate `catch` blocks *push -onto* the error array rather than swallow. +`buildApp()`; the only `try` wrapped `app.listen`) and confirming intermediate `catch` blocks _push +onto_ the error array rather than swallow. ## A guard applied to 1 of N sites of the same hazard — check the finding's own file first (#1912, PR #2028, 2026-08-06) @@ -1425,7 +1424,7 @@ removed two functions above. Same `AttachmentType` union is also interpolated at - **Internal asymmetry is the tell:** the same PR argued (correctly, for the `toBcp47Locale` item) that "a fix covering two of six would not achieve the finding's stated purpose", then shipped 1 of 2 for the key-map item. When one item in a batch widens scope on that reasoning, apply the reasoning - to the *other* items before approving. + to the _other_ items before approving. - **Don't ask for 15 hand-written `Record` maps.** The generalisation is one small generic (`unionKeyMap(prefix, Record)`); leave the shape to the follow-up. - **Residual gap the `Record` does NOT close:** union↔map parity is enforced, map↔locale-JSON parity @@ -1438,16 +1437,16 @@ removed two functions above. Same `AttachmentType` union is also interpolated at Making `reportFormatters` required on `buildReportContent` (deleting six dead silent fallbacks) is line 230's "when a hazard is enforced only by convention, remove the channel, not the individual call", applied to the formatters channel exactly as #2001 applied it to `TFunction`. A runtime throw -would be worse than *both* alternatives — it converts a silently-degraded bank PDF into an +would be worse than _both_ alternatives — it converts a silently-degraded bank PDF into an export-time crash with no compile-time signal either way. **ADR-034 line 248 already documented the 6-arg signature with no optionality marker, so the change moved code toward the ADR — no Deviation -Log row.** The ADR now *under-claims* (invariant 1 at line 206 describes injection as convention where +Log row.** The ADR now _under-claims_ (invariant 1 at line 206 describes injection as convention where it is now compiler-enforced at the `buildReportContent` boundary). Under-claiming is the benign direction: note it for the next ADR-034 pass, don't request changes. **`toBcp47Locale` placement (the "is `formatters.ts` a grab-bag?" question).** Kept it there: all six consumers feed the tag straight into `Intl`-backed calls (`getMonthName`/`getDayName`/ -`formatDateForAria` in `calendarUtils`, `formatWeekdayMonthDay`, `createFormatters`), so it *is* the +`formatDateForAria` in `calendarUtils`, `formatWeekdayMonthDay`, `createFormatters`), so it _is_ the boundary `formatters.ts` owns. `GanttHeader`/`CalendarView` already imported it; only `MonthGrid`/ `WeekGrid` are new importers. The file is mildly grab-baggy already (`computeActualDuration`/ `computeWorkDuration` are arithmetic, not formatting) and the long-term shape is @@ -1460,14 +1459,14 @@ only mapper. **Checks worth repeating on refactor-only PRs:** `ReturnType` grep before approving a named return interface (proves nothing depended on the structural-only relation); `composes:` must be the first declaration in the rule and the composed class must not share properties with the composer -(source order decides, both being single-class selectors); grep the *old* CSS-module class name across +(source order decides, both being single-class selectors); grep the _old_ CSS-module class name across `e2e/` — a POM `[class*="step4Body"]` locator survives a rename as a zero-match locator with Jest green. ## Fuzz the verbatim ports when a doc comment carries a proof (#1940, PR #2032) When an AC's whole correctness rests on an induction argument written in a doc comment, a hand-trace (mine, plus the dev-team-lead's) is two reads of the same reasoning, not two independent checks. -Copy the functions verbatim into a throwaway `.mjs` and fuzz the *stated postconditions* across a +Copy the functions verbatim into a throwaway `.mjs` and fuzz the _stated postconditions_ across a parameter space that includes the degenerate guards — 400k cases took under a minute and covered the cascade, the mid-list runt, the hard-split path, and the meta-segment boundary at once. @@ -1477,7 +1476,7 @@ author did not consider. Only randomized inputs do that. Write the ports, assert the postconditions, `rm` the file before committing. Note that the harness must be created with `Write` (the Bash tool refuses heredoc redirects inside a worktree session). -## A safety argument phrased as a *ratio* is falsified by any clamp in the chain (#1940) +## A safety argument phrased as a _ratio_ is falsified by any clamp in the chain (#1940) The #1940 ux spec argued the merge stays safe across all 96 subsets because "the threshold-to-ceiling ratio stays roughly constant." False: `usageChunkCharsForWidth`'s **one-sided clamp** pins the @@ -1518,7 +1517,7 @@ optional object prop (`lengthLimit?: { max, hint, overHint?, reachedAnnouncement becomes a single discriminant and the compiler enforces the group. **Why:** same family as "the-prop-landed-is-not-the-prop-is-wired" (#1910/PR #2004), but arriving through -the *type system* instead of a call site. Optional props that are only correct together are a latent +the _type system_ instead of a call site. Optional props that are only correct together are a latent contract, not a flexible API. **How to apply:** when a shared component gains >1 optional prop for ONE feature, ask whether any subset is legal. If not, make it one object. Note this is NOT an argument for the component calling @@ -1531,7 +1530,7 @@ should be kept; only the grouping is wrong. `MAX_SAFE_USAGE_CHUNK_CHARS` (650). Three things wrong with that framing: (1) 650 is **not a cliff** — `packUsageCellRows` splits the whole cell stream losslessly, so exceeding it costs a continuation row, not content; (2) the 150 is **unenforceable** — `areaText` is aggregate-unbounded and `attachmentsNote` has no -`maxLength` at all, which is *why* the bound was moved to the whole cell; (3) since #1973 the budget is +`maxLength` at all, which is _why_ the bound was moved to the whole cell; (3) since #1973 the budget is **computed** (`usageChunkCharsForWidth`), pinned at 650 only by a one-sided clamp against the narrowest subset. #1940's `'… '` marker is orthogonal: it's applied post-packing to rows `i >= 1` only, so a single-row cell never gets one and it cannot consume headroom. @@ -1552,26 +1551,26 @@ Two findings from reviewing the split of `LETTER_SUBJECT_FONT_SIZE` out of `SUBH **(a) A new test whose assertions duplicate an existing test, under a title that claims more.** `pageGeometry.test.ts:98-101` was assertion-for-assertion identical to the pre-existing test at lines -160-167 (`toBe(93)` + `toBe(Math.ceil(headerFootprint() + 15))`, order swapped) but titled *"PAGE_TOP_MARGIN -does not depend on letterSubject.fontSize"* — a proposition its body never references. Zero added +160-167 (`toBe(93)` + `toBe(Math.ceil(headerFootprint() + 15))`, order swapped) but titled _"PAGE_TOP_MARGIN +does not depend on letterSubject.fontSize"_ — a proposition its body never references. Zero added discrimination; it catches exactly the older test's mutation set. -**Why:** QA's mutation evidence *corroborated* rather than exposed it — "SUBHEADER 12→11 fails 4 tests" +**Why:** QA's mutation evidence _corroborated_ rather than exposed it — "SUBHEADER 12→11 fails 4 tests" reads as strong coverage, but two of the four are the duplicated pair. **A mutation count is not evidence of independent coverage; it counts assertions, not propositions.** Compare each new test's failing-mutation set against the existing suite's, not against zero. Same family as the PR #2008 "revert test proves a different proposition than the one it licenses" and the PR #2004 r4 `count >= 1` finding. **How to apply:** when a new test lands next to an existing one in the same file, diff the assertion bodies -before reading the titles. A title asserting a *negative dependency* ("X does not depend on Y") whose body +before reading the titles. A title asserting a _negative dependency_ ("X does not depend on Y") whose body never mentions Y is the tell. **(b) When a comment is genuinely the right guard — the argument, not the shrug.** Two adjacent `expect(...).toBe(12)` assertions protected only by a "do NOT deduplicate these" comment is -the right shape here. Not because no machinery exists, but because: (1) what is guarded is a *test's own -discrimination* — collapsing it loses coverage, it does not regress production, since the production split +the right shape here. Not because no machinery exists, but because: (1) what is guarded is a _test's own +discrimination_ — collapsing it loses coverage, it does not regress production, since the production split and its comment stand regardless; and (2) **any structural guard would have to encode the coupling you just removed** — "these two `number`s must be permitted to differ" is not expressible in TS, and its closest -approximation is exactly what already exists: two identifiers, two literals. The production split *is* the +approximation is exactly what already exists: two identifiers, two literals. The production split _is_ the structural guard. Rejected strengthenings, both costing more than the comment: asserting the constant through its role in @@ -1580,5 +1579,137 @@ own header comment warns against, from #1929); mirroring the `TABLE_SMALL_FONT_S needs a module-private constant exported purely to be read by a test. **How to apply:** before proposing machinery for a test-integrity concern, ask what the failure mode actually costs (coverage loss vs regression) and whether the enforcement would re-express the coupling -under removal. If both answers are "yes", a comment naming the *reason* is the correct tool — and say so +under removal. If both answers are "yes", a comment naming the _reason_ is the correct tool — and say so affirmatively rather than as an absence of alternatives. + +## A staleness guard protects only the value it returns — not the side effects on the way there (#2060, PR #2063) + +`useInfiniteScroll` added an `epochRef` generation counter to fix a real stale-response race (#2061): +each fetch's completion handler checks `epoch !== epochRef.current` before applying `setItems`/`setStatus`. +Correct, well tested. But the consumer's `fetchPage` implementation (`DiaryPage.fetchDiaryPage`) called +`setTotalItems(...)` and `setError('')` **inside** the injected function, i.e. after the `await` but before +control returned to the guarded handler. So the hook discarded the superseded batch's _items_ while the +page had already committed that batch's _metadata_ — header total reading 100 over a 5-entry filtered list. +The bug the PR fixed, reproduced one layer up, in the same PR. + +**Why it generalises:** a callback-injection contract (`fetchPage`, `onLoad`, a `loader` prop) draws its +staleness boundary at the _return value_. Anything the callback does to shared state on its own authority +is outside that boundary by construction, and no amount of guarding in the hook can reach it. + +**How to apply:** whenever reviewing a hook/service that guards against superseded async results, do not +stop at "the guard is correct." Read the _injected_ function too and ask which of its statements execute +unconditionally. Two follow-through obligations: + +1. The consumer guards its own side effects (capture the key at fetch-start, compare against a live ref). +2. **The contract must say so.** The hook's JSDoc for the injected function has to state that side effects + inside it are not covered — otherwise consumer #2 repeats the defect and the review that catches it is + luck. This is the "document the invariant, not the absence of code" rule applied to an injection seam. + +Do NOT fix it by widening the page/result type with a metadata passthrough: that grows a shared contract +to carry one consumer's header count. + +## A reset effect that clears "the data" but not the counters _describing_ the data (#2060, PR #2063) + +Same hook: the `resetKey` effect cleared `items`/`hasMore`/`status` and left `fetchSequence` and +`lastBatchCount` untouched — while `fetchSequence`'s own JSDoc claimed it "distinguishes first batch +(=== 1) from appended batch (> 1)". After any filter change the freshly _replaced_ first batch carried +sequence > 1, so the consumer announced "5 more entries loaded" for a list that had just been discarded, +and the `initialLoadAnnouncement` key (added by its own bug fix, #2062) became unreachable for the rest of +the component's life. + +**The tell:** a reset path enumerating state to clear is a _list_, and lists acquire members later than the +reset that consumes them. `fetchSequence` and `lastBatchCount` were added for a11y announcements _after_ +the reset effect was written. Diff the `useState` declarations against the reset effect's setters — any +state variable not in both is either deliberately persistent (rare, and should carry a comment) or a bug. + +**How to apply:** the documented meaning of the field is the contract; when they disagree, the +implementation is what is wrong. Also worth checking: no test pinned the buggy behaviour here, so the fix +cost nothing — but had one existed, weakening it would have been the wrong move (source-of-truth hierarchy). + +## A shared component born inside one feature keeps that feature's namespace and testids (#2060, PR #2063) + +`client/src/components/InfiniteScrollFooter/` correctly satisfied the "must be a reusable shared component" +AC by _location_, then read `t('diary:infiniteScroll.*')` and emitted `data-testid="diary-load-more-button"`. +Structurally reusable, practically not: consumer #2 must either duplicate the key block into its own +namespace or be labelled out of the diary namespace. + +**The tell is usually inside the file itself** — here the sentinel was already generic +(`infinite-scroll-sentinel`) while its two siblings were `diary-`prefixed. Mixed generality in one +component's own identifiers means the extraction stopped halfway. + +**The precedent to cite:** every shared component in `client/src/components/` uses `useTranslation('common')` +— `Modal`, `SearchPicker`, all seven `DataTable*` files — and `DataTable`, the direct analogue (shared list +infrastructure with its own pager), keeps its strings at `common:dataTable.pagination.*`. That makes this a +convention deviation with a named comparator, not an architect's taste. + +**How to apply:** on any new component under `client/src/components/`, grep its own `t(` calls and testids +for a feature prefix. Split the keys: generic UI copy → `common.json`, feature-worded copy (a11y +announcements naming "diary entries") stays with the consumer. Cheap before consumer #2 exists, expensive +after. Also check the mirror case — a _required prop the component never reads_ (`hasMore` here, dead in the +interface while every decision derived from `status`): a brand-new shared contract that mandates dead work at +every future call site, and one the component's own tests won't catch because the prop factory supplies it. + +**ROUND-2 OUTCOME (2026-09-04, commit `d77524a6`) — the fix chose a _different, better_ route than the one I +prescribed, twice.** Worth recording because "they didn't do what I said" was the wrong reflex both times: + +1. On the staleness seam I explicitly wrote "do NOT widen `InfiniteScrollPage` with a metadata + passthrough." They widened it anyway — to `InfiniteScrollPage` with optional `meta`, + plus `onPageApplied(meta, page)` / `onPageFailed(err, page)` fired **inside** the epoch check. That is + strictly better than my consumer-side ref guard: the default type parameter costs metadata-free consumers + nothing, and the invariant now lives in the hook instead of being re-derived at every call site. **My + objection had been to the _shape_ (a shared contract carrying one consumer's field) when the real + requirement was the _location of the guard_.** State the requirement, not the implementation, or you will + forbid the better fix. +2. On the diary-namespace finding I prescribed relocating keys to `common:infiniteScroll.*`. They instead + made the component copy-agnostic — seven required label props, no `useTranslation` at all, plus a + `testIdPrefix` prop (default `'infinite-scroll'`, `DiaryPage` passes `"diary"`). Also better: consumer #2 + controls copy _and_ testids without a shared namespace to collide in. **A shared namespace is only one of + two ways to de-feature a component; prop injection is the other, and it is the stronger one when the copy + is genuinely per-consumer.** + +**New round-2 finding — hand-rolled `Singular`/`Plural` key suffixes instead of i18next's native +`_one`/`_other`.** `t('infiniteScroll.initialLoadAnnouncement' + suffix, { count })` with +`suffix = count === 1 ? 'Singular' : 'Plural'`. Works today (i18next probes `…Singular_one`, misses, falls +back to the base key) and en/de both have exactly 2 plural forms, so no live bug — which is exactly why it +survives CI and green tests. Three reasons it is still a blocking convention deviation: +`dashboard.json`/`budget.json` already use `_one`/`_other` with the same `{{count}}` (named comparator); a +call-site binary split **cannot express** a >2-category CLDR locale (pl/ru/cs/ar), and CLAUDE.md documents +an explicit add-a-locale path, so the constraint is real and the later fix touches call sites not JSON; and +a dynamically-built `t()` key defeats every static key audit — `i18n.parity.test.ts` compares en/de key +_sets_ and does no usage scan, so nothing in the repo covers it. **The trap:** a local precedent existed in +the same file (`page.entryCountSingular`/`Plural`, pre-existing on beta), which is what made the deviation +feel sanctioned. A precedent inside the file you are editing is weaker evidence than the convention across +the other namespace files — check both before calling something "consistent with the codebase." + +**Also: CLAUDE.md's "Component Reuse Policy" shared-component list is a normative registry that nobody +maintains** — it still reads Badge/SearchPicker/Modal/Skeleton/EmptyState/FormError while `Spinner` and the +seven `DataTable*` files exist unlisted. New shared components must be added there (CLAUDE.md's own +Cross-Team Convention says so), but rate it LOW and say the list is already incomplete, or you are enforcing +a standard the repo demonstrably does not uphold. + +### Verifying an i18n pluralization fix (#2060, PR #2063 round 3, 2026-09-04 — APPROVED) + +Renaming `*Singular`/`*Plural` keys to `_one`/`_other` and deleting the suffix-building code **looks** self- +evidently correct in a diff, but the diff alone proves nothing: the suffix format is a runtime contract with +the i18next version, not a naming style. Three checks make the verification discriminating, and all three are +cheap: + +1. **Version + config**: `_one`/`_other` is the v4 JSON format. Confirm the pinned `i18next` major supports + it (26.3.6 here) **and** that `client/src/i18n/index.ts` sets no `compatibilityJSON` override — a v3 + override would silently reinstate `_plural` and break every renamed key with no type error. +2. **Convention, not invention**: grep `_one"` across `client/src/i18n/en/*.json` — budget.json (10) and + dashboard.json (5) already use it, so the fix converges on house style rather than adding a third dialect. +3. **The assertion must be rendered text, not a key**: `DiaryPage.test.tsx` asserts `'1 more entry loaded'` / + `'2 entries loaded'`. If the suffix format were wrong, i18next falls back and these fail. A test that + asserts the *key name* (or that the key exists in JSON) passes under a broken format and is worthless as a + guard. `i18n.parity.test.ts` compares en/de key **sets** only — it cannot catch a wrong suffix format + either, since a consistently-wrong rename stays in parity. + +**Recurrence of comment cross-reference rot** (see the #1939 entry above): the assertions were updated but +`DiaryPage.test.tsx:175` and `:564` still named the deleted `initialLoadAnnouncementSingular`/`Plural` in +prose. Renaming a key is a repo-wide grep, not a call-site edit — grep the old identifier in comments too. + +**Round-3 discipline**: after two full rounds, a comment-only nit and a pre-existing same-shape anti-pattern +in an adjacent line (`page.entryCountSingular`, confirmed on `origin/beta`) are both explicitly non-blocking. +Say "do not respin for this" out loud in the verdict — otherwise a LOW finding reads as a fourth round, and +naming the pre-existing one without the beta provenance check invites scope creep into someone else's diff. diff --git a/.claude/agent-memory/product-owner/MEMORY.md b/.claude/agent-memory/product-owner/MEMORY.md index 03764c121..9bb301be5 100644 --- a/.claude/agent-memory/product-owner/MEMORY.md +++ b/.claude/agent-memory/product-owner/MEMORY.md @@ -35,7 +35,7 @@ Detail in [standalone-bugs-and-stories.md](standalone-bugs-and-stories.md) (budg - Budget/invoice batches: #1369-#1373, #1389-#1390, #1401, #1421-#1425, #1439-#1441, #1553 - Auto-itemize (standalone, no parent epic): #1545-#1547 mini-epic, #1600, #1833 duplicate budget lines on commit retry -- Diary: #1426 critical photo data loss | Photo: #1723 lightbox picker UX +- Diary: #1426 critical photo data loss | **#2060 pager→infinite scroll, In Progress, 23 ACs — PR #2063 APPROVED round 2 (all 5 R1 findings closed); spin-off bugs #2061/#2062 bundled and still open; `Refs #2060` not `Fixes`, stays In Progress → UAT; deferred follow-up #2065** | Photo: #1723 lightbox picker UX - **#1970 auth rate limits** — PR #1989 approved R3, all 7 ACs met, Done on merge; follow-ups #1990/#1991/#1992 open. See [auth-rate-limits-1970.md](auth-rate-limits-1970.md) - **#1955 DataTable two-column toggle race** (Should Have, Backlog) — fast clicking is the SAFE case; #1920's E2E-only fix makes CI green without fixing production. See [datatable-column-preference-race.md](datatable-column-preference-race.md) - **#1957 E2E test-isolation hazard** (shared-admin `user_preferences` writes under `fullyParallel`) — Should Have, Backlog; an audit + per-spec sweep, not a single-file fix. See [e2e-shared-admin-preference-hazard.md](e2e-shared-admin-preference-hazard.md) @@ -80,24 +80,28 @@ All rulings, contract facts, per-PR review outcomes and filed follow-ups live in Full derivations and the incidents behind each are in [pr-review-patterns.md](pr-review-patterns.md) and [bank-report-wizard.md](bank-report-wizard.md). -- **Merge is a code gate; Done is an acceptance gate.** An unverifiable AC *with* a substitute assertion = documented deviation; *without* one → UAT, reopen on failure. +- **Search the tracker before filing anything described as a symptom.** A request phrased as a fresh bug report can already be an In-Progress story — #2060 was nearly duplicated on its own filing day. One `gh issue list --search " in:title"` costs a call; a duplicate splits ACs across two issues and lets an implementer spec from the weaker one. When the existing issue is better, correct _it_ and record the duplicate-check on it. +- **Merge is a code gate; Done is an acceptance gate.** An unverifiable AC _with_ a substitute assertion = documented deviation; _without_ one → UAT, reopen on failure. +- **A generation/epoch guard inside a hook does not protect state the consumer writes inside the injected callback** — enumerate every `setState` on the other side of a race guard (#2063). And when 3+ conditions each suppress a render region, ask what state satisfies all of them at once: the answer is a blank page. +- **A shared component in the shared directory is not automatically shareable** — check an AC's purpose against its letter whenever the deliverable is "make this reusable"; directory placement alone satisfies the letter (#2063). The cheap proof is a `testIdPrefix`-style param plus a test asserting the _default_ IDs are absent under a custom prefix. +- **Put consumer-derived state on the safe side of a race guard with an opaque `meta` passthrough + post-guard callbacks**, not by "moving the write earlier" — keeps the hook domain-free (#2063 R2). And **a shared control that disables itself on its own activation drops keyboard focus to ``** — check it on every such component (#2065). - **A finding that defeats the PR's own AC belongs in that PR, not a follow-up.** Conversely: **a green PR is not reopened to absorb non-blocking findings — file, don't expand.** -- **Closed/released ACs get a dated supersession comment, never a rewrite.** When a body *is* rewritten, always report "body rewritten, numbering reassigned" — omitting that let two agents spec from a stale revision. +- **Closed/released ACs get a dated supersession comment, never a rewrite.** When a body _is_ rewritten, always report "body rewritten, numbering reassigned" — omitting that let two agents spec from a stale revision. - **Wiki Deviation Log: the Observation column of a dated entry is immutable; corrections go forward in that entry's Resolution.** Ruled 2026-08-06 on PR #2022 (#1992). Same shape as the AC supersession rule. Rationale + boundaries (spurious entries are withdrawn not deleted; lead a correction with "Correction to the observation above:", never a trailing parenthetical) in [pr-review-patterns.md](pr-review-patterns.md). -- **Operator-facing docs never carry a "known issue" pointer to an open bug** — docs ship with releases, the tracker doesn't, and nothing forces the line's removal when the fix lands. Put the *workaround* in as a plain requirement instead: prescriptive copy ages into harmlessness, diagnostic copy ages into lies. Ruled 2026-08-06 on PR #2027 (#1990) re #2026. +- **Operator-facing docs never carry a "known issue" pointer to an open bug** — docs ship with releases, the tracker doesn't, and nothing forces the line's removal when the fix lands. Put the _workaround_ in as a plain requirement instead: prescriptive copy ages into harmlessness, diagnostic copy ages into lies. Ruled 2026-08-06 on PR #2027 (#1990) re #2026. - **Docs-only PRs make `Quality Gates`/`E2E Gates` green by vacuity** — `Detect Changes` skips every real job, and the `onBrokenAnchors: 'throw'` docs build runs only on release. Run `npm run docs:build` yourself when reviewing a docs PR that adds anchors. -- **An item whose every claim is machine-checkable against source has no UAT surface** — the PO review *is* the acceptance gate; close on merge (#1992). UAT is for rendered artefacts and operator-observable behaviour. +- **An item whose every claim is machine-checkable against source has no UAT surface** — the PO review _is_ the acceptance gate; close on merge (#1992). UAT is for rendered artefacts and operator-observable behaviour. - **ACs that misdescribe reality fail correct implementations at UAT** (seen 4×). Amend the text; don't design around it. **Shipped error copy that misdescribes an operator's own input is a worse instance than an AC that misdescribes code** — the AC is read once by someone who can check the source; the message is read at 2am with nothing else (#1991). -- **When an AC names a mechanism (a regex, an algorithm) as if it were the contract, the contract is the *outcome* AC.** The mechanism gives way. Check first whether the strict form is even *satisfiable* alongside the guard AC — #1991's `/^\d+$/` required modifying tests that AC6 said must pass unmodified, so it wasn't a tension to balance, it was unsatisfiable. +- **When an AC names a mechanism (a regex, an algorithm) as if it were the contract, the contract is the _outcome_ AC.** The mechanism gives way. Check first whether the strict form is even _satisfiable_ alongside the guard AC — #1991's `/^\d+$/` required modifying tests that AC6 said must pass unmodified, so it wasn't a tension to balance, it was unsatisfiable. - **"The downstream check will catch it" is true seven times out of eight — enumerate all eight.** Verifying every one of #1991's call sites had a lower bound turned a plausible argument into a safety proof, and revealed the disputed behaviour was observable at only 3 of 8 sites (the other 5 collapse both branches into one message). -- **An input cap on a field whose baseline is *derived* must clear what the system itself legally generates**, or the AC's exception state becomes the routine state and a near-limit affordance turns into permanent furniture. Find the floor before picking the number (#1941: `usageText` 500, not the suggested 150). -- **Before routing a "rendering capacity" question to an architect, check whether the renderer has a container at all.** Flowing content with no table/`dontBreakRows`/fixed height has no capacity ceiling — an over-long value just makes more pages, so the question was a *product* one all along (#1941 `coverLetter.body`). Saves a routing round. -- **Before calling a missing mechanism an accessibility gap, check which mechanism already carries the fact.** A two-mode component can legitimately use the accessible *name* in one mode and the *description* in the other; "fixing" the omission then double-announces (#1941 `EditableField` dense vs labelled mode). Separate the enabling refactor (in scope) from the behaviour change (rejected). -- **Chasing an AC's vacuity often exposes a defect in its neighbour** — #1941's AC5 (no server round-trip) proved AC4's "existing *saved* value" fixture unconstructible. Also: a vacuous AC needs an explicit *prohibition*, not just a note, when the literal reading invites the inverse mistake. -- **An issue filed by another agent is a snapshot of that round's codebase** — re-verify the *mechanism*, not just the defect, and correct the mechanism while keeping the story. -- **Premises go stale at *pickup*, not at filing — check the dates before blaming filing diligence.** #1941 and #1950 were both correct when written and invalidated 1–2 days later by *other queue items reworking the same files*; no care at filing could have prevented it. When a queue holds several items touching one area, re-verify every premise against `HEAD` when the item is picked up. Both were caught by implementers who flagged instead of fabricating — the control working, not failing. +- **An input cap on a field whose baseline is _derived_ must clear what the system itself legally generates**, or the AC's exception state becomes the routine state and a near-limit affordance turns into permanent furniture. Find the floor before picking the number (#1941: `usageText` 500, not the suggested 150). +- **Before routing a "rendering capacity" question to an architect, check whether the renderer has a container at all.** Flowing content with no table/`dontBreakRows`/fixed height has no capacity ceiling — an over-long value just makes more pages, so the question was a _product_ one all along (#1941 `coverLetter.body`). Saves a routing round. +- **Before calling a missing mechanism an accessibility gap, check which mechanism already carries the fact.** A two-mode component can legitimately use the accessible _name_ in one mode and the _description_ in the other; "fixing" the omission then double-announces (#1941 `EditableField` dense vs labelled mode). Separate the enabling refactor (in scope) from the behaviour change (rejected). +- **Chasing an AC's vacuity often exposes a defect in its neighbour** — #1941's AC5 (no server round-trip) proved AC4's "existing _saved_ value" fixture unconstructible. Also: a vacuous AC needs an explicit _prohibition_, not just a note, when the literal reading invites the inverse mistake. +- **An issue filed by another agent is a snapshot of that round's codebase** — re-verify the _mechanism_, not just the defect, and correct the mechanism while keeping the story. +- **Premises go stale at _pickup_, not at filing — check the dates before blaming filing diligence.** #1941 and #1950 were both correct when written and invalidated 1–2 days later by _other queue items reworking the same files_; no care at filing could have prevented it. When a queue holds several items touching one area, re-verify every premise against `HEAD` when the item is picked up. Both were caught by implementers who flagged instead of fabricating — the control working, not failing. - **An AC satisfiable only by undoing a shipped decision is a bug in the acceptance record, not an AC** (#1950's AC 1.5 required reinstating the constant its own guard exists to keep from returning). Strike it; don't let an implementer comply. -- **Before adding a replacement assertion for a struck AC, check whether the invariant is already covered.** Split the question: *drift* is a test's job, *deliberate change* is a doc comment's job. If both are covered, record the invariant as prose and add nothing (#1950). +- **Before adding a replacement assertion for a struck AC, check whether the invariant is already covered.** Split the question: _drift_ is a test's job, _deliberate change_ is a doc comment's job. If both are covered, record the invariant as prose and add nothing (#1950). - **When an AC's correct predicate is one plausible misreading away from a no-op, write the misreading into the AC** and demand a test pinning that shape. - **Check the other direction of any reported boolean defect** — the mirror case is often live too. - **`Refs #N`, not `Fixes #N`, for any issue in a parent-less cluster carrying a UAT disposition** — `/epic-close` (the only skill with a UAT step) never runs without an epic, so standalone `/release` would auto-close it unvalidated. **Before trusting "the lifecycle protects this", check the item actually enters that lifecycle.** The acceptance gate is the board status, which I set — not open/closed, which GitHub sets. @@ -107,7 +111,7 @@ Full derivations and the incidents behind each are in [pr-review-patterns.md](pr - **"Dominated by an existing measurement, do not re-measure" is as valuable as demanding the measurement.** Rank flagged risks against each other instead of treating every new co-occurrence as equally alarming. - **When a fix removes a mutual exclusion, ask what combination just became reachable** that never rendered before. - **A documented measurement (glossary space budget, column width) can close a wording debate before it starts.** -- **"Real render" ≠ "measured."** An unmocked render whose assertions read the *input* content tree, or read a quantity fixed by construction (e.g. a table width that is `printableWidth()` for any input), is still a vacuous assertion. Ask what *varies* when the guarded content changes — not whether a render happened. +- **"Real render" ≠ "measured."** An unmocked render whose assertions read the _input_ content tree, or read a quantity fixed by construction (e.g. a table width that is `printableWidth()` for any input), is still a vacuous assertion. Ask what _varies_ when the guarded content changes — not whether a render happened. - **Check that the issue a routing rule points at is still open before restating the rule.** - **Answer boundary/privacy questions about the artifact that leaves the system, not only about the API.** - **A finding's severity is capped by my own enumeration failure** — if I missed it in earlier rounds, it can't be blocking now. End mirror-image review cycles by **stating the enumeration as exhaustive**. diff --git a/.claude/agent-memory/product-owner/pr-review-patterns.md b/.claude/agent-memory/product-owner/pr-review-patterns.md index 51d330720..b2814a3b8 100644 --- a/.claude/agent-memory/product-owner/pr-review-patterns.md +++ b/.claude/agent-memory/product-owner/pr-review-patterns.md @@ -139,29 +139,50 @@ When a PR adds a mobile card list beside a desktop table, re-check rather than a - **`max` and `timeWindow` are independent branches in `mergeParams()`** (`node_modules/@fastify/rate-limit/index.js:163-175`), and route options merge over `globalParams` via `Object.assign`. So an assertion on `x-ratelimit-limit` proves **only** `max`; deleting the route's `timeWindow` line silently inherits the global window with that assertion still green. PR #1989 round 2 shipped exactly that and its commit message claimed it closed the window gap — it did not. **When an author reports a verification gap as fixed, re-derive the mutation yourself** ("delete the wiring line — does this specific assertion fail?"); a plausible-sounding fix to an assertion gap is the easiest thing to wave through twice. - **Hand-rolled regex duplicating a library's grammar drifts in BOTH directions.** The `ms`-format regex accepted `0.5ms` (useless) and rejected `1y` (valid `ms`). Prefer "call the library, require a positive finite result" — one check instead of a guard beside a duplicated grammar. - **A "house convention" ruling still deserves a tracked owner when three reviewers independently trip on it.** Round 2 filed **#1991** (tech-debt, Could Have, Backlog) for uniform integer parsing across the eight `parseInt` call sites in `loadConfig()` — the ruling stays "out of scope for #1970", but `product-architect` (Medium) and `security-engineer` (Low) both raised it, so leaving it purely as a review comment guarantees a fourth reviewer raises it again. Same shape as the #1950 ruling: bounded-and-quantified earns a tracked owner. -- **Before flagging leniency, check whether it is the house convention.** `parseInt` + `isNaN || <= 0` lets `20abc`→20 and `1e9`→1 through, but `BACKUP_RETENTION`, `LLM_MAX_TOKENS`, and `LLM_REQUEST_TIMEOUT_MS` in `config.ts` all use the identical form. Tightening one of four makes the file *less* consistent → ruled explicitly out of scope and labelled informational. Grep the sibling cases in the same file before writing a finding; distinguish "misparse still yields a working control" from "yields no control". +- **Before flagging leniency, check whether it is the house convention.** `parseInt` + `isNaN || <= 0` lets `20abc`→20 and `1e9`→1 through, but `BACKUP_RETENTION`, `LLM_MAX_TOKENS`, and `LLM_REQUEST_TIMEOUT_MS` in `config.ts` all use the identical form. Tightening one of four makes the file _less_ consistent → ruled explicitly out of scope and labelled informational. Grep the sibling cases in the same file before writing a finding; distinguish "misparse still yields a working control" from "yields no control". - **"Asserted by a test that observes the effective limit" means: would this test fail if the wiring line were deleted?** #1989 proved `max` end-to-end (set to 3, 4th request 429s) but nothing proved `timeWindow` reached the route — deleting it would fall back to the global `1 minute` with every test still green. `x-ratelimit-reset` = `Math.ceil(ttl/1000)` makes the window observable (`30s` → ~30 vs global default ~60). Same family as the "assertions that pass on nothing" pattern: the header test asserted only `toBeDefined()`. -- **Run the mutation, don't read the assertion.** Round 3 (`5446b29a`) closed the gap with `expect(response.headers['x-ratelimit-reset']).toBe('900')`. I verified it by *deleting* `timeWindow` from `auth.ts:147` locally, running the single test file (`Expected: "900" / Received: "60"`), then `git checkout -- server/src/routes/auth.ts`. Reading a specific-looking assertion cannot distinguish load-bearing from decorative — round 2 is proof, since I nearly waved through an assertion that looked equally specific. Mutate + revert stays inside PO boundaries: it is verification, not authoring. Do this whenever an AC's evidence is of the form "this test proves X reached Y". -- **Before accepting a numeric-header probe, ask which request in the window it observes.** `x-ratelimit-reset` is exact (`900`) only on the **first** request of a fresh window — `LocalStore.incr` sets `ttl: timeWindow` verbatim (`store/LocalStore.js:17`); on any later request it is `timeWindow - elapsed` (`:38`), where the same equality assertion would be a flake. #1989 is safe because each test builds its own app → fresh in-memory store. A derived-value assertion can be both meaningful *and* flaky; check determinism separately from meaningfulness. +- **Run the mutation, don't read the assertion.** Round 3 (`5446b29a`) closed the gap with `expect(response.headers['x-ratelimit-reset']).toBe('900')`. I verified it by _deleting_ `timeWindow` from `auth.ts:147` locally, running the single test file (`Expected: "900" / Received: "60"`), then `git checkout -- server/src/routes/auth.ts`. Reading a specific-looking assertion cannot distinguish load-bearing from decorative — round 2 is proof, since I nearly waved through an assertion that looked equally specific. Mutate + revert stays inside PO boundaries: it is verification, not authoring. Do this whenever an AC's evidence is of the form "this test proves X reached Y". +- **Before accepting a numeric-header probe, ask which request in the window it observes.** `x-ratelimit-reset` is exact (`900`) only on the **first** request of a fresh window — `LocalStore.incr` sets `ttl: timeWindow` verbatim (`store/LocalStore.js:17`); on any later request it is `timeWindow - elapsed` (`:38`), where the same equality assertion would be a flake. #1989 is safe because each test builds its own app → fresh in-memory store. A derived-value assertion can be both meaningful _and_ flaky; check determinism separately from meaningfulness. - **An AC of the form "documented in CLAUDE.md AND on the docs site (file a request if needed)" is satisfiable by filing the request** — so file it during review instead of blocking on it. Filed **#1990** (docs-writer, Todo, blocked-by #1970), carrying forward #1970's Notes requirement to cross-reference `TRUST_PROXY` (it decides whether the limit buckets on the real client IP or the proxy's — the shared-IP operator needs both settings). - **`gh pr review` cannot be used when the PR author is the token owner** (the user's own PRs). Post the verdict via `gh pr comment` with an explicit `## Verdict:` line — same workaround already recorded for #1909. ## Wiki-only doc PRs: the Deviation Log convention (PR #2022, #1992) — APPROVED -- **RULING: the Observation column of a dated Deviation Log entry is immutable; a wrong claim in it is corrected forward in that same entry's Resolution.** The `2026-08-04` API-Contract entry claimed the OIDC discrepancy "spans the CLAUDE.md env-var table"; it never did. `product-architect` corrected forward. Ruled correct. **Why:** the log carries two payloads — what is true now (amendable) and *what we believed and how we got it wrong* (the reason the log exists at all). Rewriting the Observation destroys the second and makes the log unauditable: a reader can no longer tell whether a claim was ever made, so an instrument whose purpose is catching recurring drift starts erasing its own misses. Same shape as the AC supersession rule. **Boundaries:** typos/re-padding are editable; a wholly spurious entry is *withdrawn* in its Resolution (`Withdrawn — no deviation existed`), never deleted, or the phantom gets rediscovered; a forward correction must name the wrong claim, state the truth, and cite the source of truth. **Shape nit worth repeating:** lead with `Correction to the observation above:` — a trailing parenthetical reads as scope info, not a retraction, and gets skimmed past. Asked product-architect to put the convention in both pages' Deviation Log preamble. -- **Wiki-only PRs are reviewed against source, not against the issue text.** With `product-architect` as author its own review is skipped, and a wiki PR touches no `client/src/` or security files → PO is the *sole* reviewer. Verify each claim by grepping the cited symbols (`config.ts:142` `oidcEnabled`, `oidc.ts:45` derivation, the full `getValue(` read-set) rather than trusting the commit message; the commit message is the artefact most likely to be right for the wrong reason. -- **"State how X is determined" ACs invite adjacent-variable prose — that is completion, not creep, when the variable is already a row on the same page.** #1992's AC4 got `TRUST_PROXY`/`X-Forwarded-Proto` and the registered-redirect-URI requirement thrown in. Ruled in-scope: the AC's *purpose* was to leave no unanswered question, and "falls back to the request host" begs "which host, as seen by whom". +- **RULING: the Observation column of a dated Deviation Log entry is immutable; a wrong claim in it is corrected forward in that same entry's Resolution.** The `2026-08-04` API-Contract entry claimed the OIDC discrepancy "spans the CLAUDE.md env-var table"; it never did. `product-architect` corrected forward. Ruled correct. **Why:** the log carries two payloads — what is true now (amendable) and _what we believed and how we got it wrong_ (the reason the log exists at all). Rewriting the Observation destroys the second and makes the log unauditable: a reader can no longer tell whether a claim was ever made, so an instrument whose purpose is catching recurring drift starts erasing its own misses. Same shape as the AC supersession rule. **Boundaries:** typos/re-padding are editable; a wholly spurious entry is _withdrawn_ in its Resolution (`Withdrawn — no deviation existed`), never deleted, or the phantom gets rediscovered; a forward correction must name the wrong claim, state the truth, and cite the source of truth. **Shape nit worth repeating:** lead with `Correction to the observation above:` — a trailing parenthetical reads as scope info, not a retraction, and gets skimmed past. Asked product-architect to put the convention in both pages' Deviation Log preamble. +- **Wiki-only PRs are reviewed against source, not against the issue text.** With `product-architect` as author its own review is skipped, and a wiki PR touches no `client/src/` or security files → PO is the _sole_ reviewer. Verify each claim by grepping the cited symbols (`config.ts:142` `oidcEnabled`, `oidc.ts:45` derivation, the full `getValue(` read-set) rather than trusting the commit message; the commit message is the artefact most likely to be right for the wrong reason. +- **"State how X is determined" ACs invite adjacent-variable prose — that is completion, not creep, when the variable is already a row on the same page.** #1992's AC4 got `TRUST_PROXY`/`X-Forwarded-Proto` and the registered-redirect-URI requirement thrown in. Ruled in-scope: the AC's _purpose_ was to leave no unanswered question, and "falls back to the request host" begs "which host, as seen by whom". - **Check whether a caveat covers both halves of the thing it qualifies.** The new prose attached the `TRUST_PROXY` dependency to `request.protocol` only, while asserting the host/scheme fallback "reflects whatever the proxy forwarded" unconditionally — but `request.host` is equally gated. Low finding; the wording let a reader conclude the fallback is safe without `EXTERNAL_URL`, which is the exact failure the paragraph exists to prevent. - **Two legs of one flow can derive the same value differently.** `oidc.ts:45` (login) uses `externalUrl || request-origin`; `oidc.ts:106` (callback) uses the request origin unconditionally. Documenting only the leg the issue named reproduces the defect class being fixed. Ask "is this derivation used anywhere else?" whenever a doc PR pins one down. - **Agent memory shipped in a doc PR needs the same staleness check as the doc.** The architect's `recurring-patterns.md` said two sweep findings were "not fixed, not filed" — they were, as #2023/#2024. A memory instruction to file duplicates is worth a review finding. ## Docs-site PRs: the operator-facing surface (PR #2027, #1990) — APPROVED, 5/5 ACs -- **RULING: operator-facing docs never carry a "known issue" pointer to an open bug.** Asked whether the new rate-limit copy should reference #2026 (OIDC `redirect_uri` divergence making `EXTERNAL_URL` + `TRUST_PROXY=true` a de facto required pair). **Why:** (1) docs are versioned and deployed with stable releases, the tracker is not — the line becomes false the moment the fix ships and *nothing in the release pipeline forces its removal*, so it is a defect with a delayed fuse in the one artifact we cannot assert against; (2) it exports internal work-tracking IDs to a reader with no standing to act on them; (3) it inverts ownership — the tracker holds an open defect's status *because* status changes. **The exception:** when the bug has an operator-visible symptom AND a workaround the operator must apply, put the *workaround* in the docs as a plain requirement, no bug reference, no framing as a bug. **Prescriptive copy ages into harmlessness; diagnostic copy ages into lies.** Here both pages already prescribed both variables (`configuration.md:38`, `oidc-setup.md:45-47`), so nothing was owed. -- **Docs-only PRs make `Quality Gates` / `E2E Gates` green by vacuity.** `Detect Changes` skips Static Analysis, Test, Docker and every E2E shard; the wrapper gates still report SUCCESS. The `onBrokenAnchors: 'throw'` docs build does **not** run on the PR — it runs on the release workflow, so a broken anchor merges to `beta` and blocks a *release*. Run `npm run docs:build` yourself when reviewing a docs PR that adds anchors (~1 min; it works in this worktree despite the docs-writer's older "build is broken in worktrees" note). -- **"Add a link and cover the gap" beats rewriting a correct sentence.** #1990's AC5 said "reconcile — no contradictory or duplicated guidance". `configuration.md:35`'s existing clause makes a *spoofing-resistance* claim; the new copy makes a *bucket-membership* claim. Different propositions about one variable = neither duplicate nor contradiction. Rewriting would have traded a true narrow claim for a longer one with more surface to go stale. **Test for "duplicated": are these the same proposition, or two propositions about the same symbol?** -- **When judging "is this page also affected", the page that already prescribes the fix needs no pointer — the page with reassuring prose does.** `oidc-setup.md` looked like the obvious gap (OIDC-behind-a-proxy *is* the shared-IP population) but it already sets `TRUST_PROXY=true` in its compose block and reference table, so a reader lands correctly. `docker-setup.md:53` was the real gap: it discusses rate limiting under `TRUST_PROXY` and closes "no configuration change is required to benefit from this" — true of the spoofing fix, but it is the page's only rate-limiting prose and leaves a complete-and-wrong model. **Rank candidate pages by what a reader believes on exit, not by topical adjacency.** -- **Two tuning bullets keyed on different axes can both match one deployment.** #1990's AC4 named "behind one NAT" (egress) and "internet-exposed, no proxy" (ingress); a home LAN behind a port-forward — the commonest self-hosted shape — matches both and is told to raise *and* lower. AC met (both named shapes described correctly) but the overlap has no tie-breaker. **Whenever guidance branches on deployment shape, check the axes are the same axis; if not, the intersection needs an explicit ruling.** -- **A doc that links a library as "the format" inherits that library's whole grammar as a promise.** `AUTH_RATE_LIMIT_WINDOW` links `vercel/ms`, but `config.ts:364` validates a *narrower* hand-rolled regex (unit suffix and leading digit both required); `100` and `.5h` are valid `ms` and crash the server at boot. Same hand-rolled-regex-drifts-from-library defect already recorded for #1970 — it has now leaked into operator-facing copy. **Check a linked spec against the actual validator, in both directions.** +- **RULING: operator-facing docs never carry a "known issue" pointer to an open bug.** Asked whether the new rate-limit copy should reference #2026 (OIDC `redirect_uri` divergence making `EXTERNAL_URL` + `TRUST_PROXY=true` a de facto required pair). **Why:** (1) docs are versioned and deployed with stable releases, the tracker is not — the line becomes false the moment the fix ships and _nothing in the release pipeline forces its removal_, so it is a defect with a delayed fuse in the one artifact we cannot assert against; (2) it exports internal work-tracking IDs to a reader with no standing to act on them; (3) it inverts ownership — the tracker holds an open defect's status _because_ status changes. **The exception:** when the bug has an operator-visible symptom AND a workaround the operator must apply, put the _workaround_ in the docs as a plain requirement, no bug reference, no framing as a bug. **Prescriptive copy ages into harmlessness; diagnostic copy ages into lies.** Here both pages already prescribed both variables (`configuration.md:38`, `oidc-setup.md:45-47`), so nothing was owed. +- **Docs-only PRs make `Quality Gates` / `E2E Gates` green by vacuity.** `Detect Changes` skips Static Analysis, Test, Docker and every E2E shard; the wrapper gates still report SUCCESS. The `onBrokenAnchors: 'throw'` docs build does **not** run on the PR — it runs on the release workflow, so a broken anchor merges to `beta` and blocks a _release_. Run `npm run docs:build` yourself when reviewing a docs PR that adds anchors (~1 min; it works in this worktree despite the docs-writer's older "build is broken in worktrees" note). +- **"Add a link and cover the gap" beats rewriting a correct sentence.** #1990's AC5 said "reconcile — no contradictory or duplicated guidance". `configuration.md:35`'s existing clause makes a _spoofing-resistance_ claim; the new copy makes a _bucket-membership_ claim. Different propositions about one variable = neither duplicate nor contradiction. Rewriting would have traded a true narrow claim for a longer one with more surface to go stale. **Test for "duplicated": are these the same proposition, or two propositions about the same symbol?** +- **When judging "is this page also affected", the page that already prescribes the fix needs no pointer — the page with reassuring prose does.** `oidc-setup.md` looked like the obvious gap (OIDC-behind-a-proxy _is_ the shared-IP population) but it already sets `TRUST_PROXY=true` in its compose block and reference table, so a reader lands correctly. `docker-setup.md:53` was the real gap: it discusses rate limiting under `TRUST_PROXY` and closes "no configuration change is required to benefit from this" — true of the spoofing fix, but it is the page's only rate-limiting prose and leaves a complete-and-wrong model. **Rank candidate pages by what a reader believes on exit, not by topical adjacency.** +- **Two tuning bullets keyed on different axes can both match one deployment.** #1990's AC4 named "behind one NAT" (egress) and "internet-exposed, no proxy" (ingress); a home LAN behind a port-forward — the commonest self-hosted shape — matches both and is told to raise _and_ lower. AC met (both named shapes described correctly) but the overlap has no tie-breaker. **Whenever guidance branches on deployment shape, check the axes are the same axis; if not, the intersection needs an explicit ruling.** +- **A doc that links a library as "the format" inherits that library's whole grammar as a promise.** `AUTH_RATE_LIMIT_WINDOW` links `vercel/ms`, but `config.ts:364` validates a _narrower_ hand-rolled regex (unit suffix and leading digit both required); `100` and `.5h` are valid `ms` and crash the server at boot. Same hand-rolled-regex-drifts-from-library defect already recorded for #1970 — it has now leaked into operator-facing copy. **Check a linked spec against the actual validator, in both directions.** - **A "fails at startup" claim is only as recognisable as the string the operator greps.** AC2's stated purpose was "so operators recognise the failure mode"; the copy says "fail at startup with a configuration error" but never quotes the literal `Configuration validation failed:` prefix, and under a typical `restart: unless-stopped` compose file the observable symptom is a **crash-looping container**, not a one-shot error. Met, but ask for the literal string and the observable symptom whenever an AC's purpose is recognition. - **Verify a startup-failure claim through the whole registration chain, not just the throw.** `config.ts:377` throws → `configPlugin` body calls `loadConfig` → `app.ts:84` `await app.register(configPlugin)` inside `buildApp()` → `server.ts:4` `await buildApp()` in a `try` whose `catch` does `process.exit(1)`. A throw inside an `fp()` plugin only aborts startup because nothing between it and the entrypoint catches it — check that, since a single `try` anywhere on the path turns "fails at startup" into "starts degraded". + +## Diary infinite scroll (PR #2063, #2060/#2061/#2062) — REQUEST_CHANGES round 1 + +- **RULING: a generation/epoch guard inside a hook does not protect state the _consumer_ writes inside the injected fetch callback.** `useInfiniteScroll` checks `epoch !== epochRef.current` after `await fetchPageRef.current(page)`, so `items`/`hasMore`/`status` are safe — but `DiaryPage`'s `fetchDiaryPage` calls `setTotalItems()` and `setError('')` _inside_ the promise, before returning. A superseded batch (Load more, then change the filter) still applies them: the header total describes the previous filter and never self-corrects, because no further fetch is issued. **How to apply:** whenever a race fix lands as a guard at one layer, enumerate every write that happens on the _other_ side of that guard — the injected callback's own `setState` calls are the ones nobody looks at. Fixing #2061's items half while leaving the total half live is the same PR "fixing" its own bug report incompletely. +- **The mirror case is often the worse one.** The same unguarded `setError('')` gives a stale success permission to clear an error the _new_ fetch raised: banner gone (`error` empty), empty state suppressed (`status !== 'error'`), footer suppressed (`entries.length === 0`) → a completely blank page with no retry. Three independently reasonable render guards intersect at "render nothing". **Whenever three or more conditions each suppress a region, ask what state satisfies all of them at once.** +- **A shared component in the shared directory is not automatically shareable.** `InfiniteScrollFooter` lives in `client/src/components/` but reads all six strings from the `diary:` namespace and hardcodes `diary-*` test IDs. AC21 ("logic lives in a reusable hook/component") passes _literally_ — the observer logic is in the hook — while the issue's assumption 5 ("other list views must be able to adopt it later") fails. **Check the AC's purpose against the AC's letter when the deliverable is "make this reusable"; the letter is satisfiable by directory placement alone.** +- **Two loading affordances is a real finding, not a nit.** `status === 'loading'` renders the footer's `statusRow` _and_ the Load more button's loading state — same string, two spinners, stacked. An AC saying "a loading affordance" (singular) is violated by rendering it twice; both branches read correctly in isolation, which is why it survived two dev-team-lead rounds. +- **`{{count}}` without `_one`/`_other` is a live defect where the same namespace already solves it.** The three new `infiniteScroll.*` announcements produce "1 entries loaded"; `page.entryCountSingular`/`Plural` sit two lines above in the same render, and `dashboard.json`/`budget.json` use the `_one`/`_other` convention. **Grep the sibling keys in the same namespace before rating a plural finding — precedent inside the file turns Low-and-arguable into Low-and-settled.** +- **`it.failing` tripwires must be promoted, not left.** #2061's reproduction was kept as `it.failing` while the bug was open; the PR correctly converted it to a real `it`. Worth checking explicitly — a fixed bug under `it.failing` turns CI red for "unexpectedly passing", so the _absence_ of red CI is not evidence it was handled. +- **Verdict mechanics:** author was the token owner → `--request-changes` rejected (`Review Can not request changes on your own pull request`); posted via `gh pr comment` with `VERDICT: REQUEST_CHANGES` as the literal first line (CLAUDE.md > Reviewer Verdict Policy's mechanical exception), agent prefix on line 3. +- **`Refs #2060`, not `Fixes`** — 7 UAT scenarios + AC4 (position doesn't jump) / AC19 (focus indicator, both themes) / AC22 (mobile) are appearance-and-feel claims, and #2060 is standalone so `/epic-close` never runs. Also corrected the PR body's claim that #2061/#2062 were "closed": both open, and a squash into `beta` closes nothing (GitHub honours the keyword only on a merge into the default branch). + +### Round 2 — APPROVED at `d77524a6` (all 5 findings closed, 1 deferred to #2065) + +- **The generic fix for "consumer state on the wrong side of a race guard": an opaque `meta` passthrough plus post-guard callbacks.** `InfiniteScrollPage` gained `meta?: M`; the hook gained `onPageApplied(meta, page)` (called after `if (epoch !== epochRef.current) return`) and `onPageFailed(err, page)` (called inside the same guard as `setStatus('error')`). `fetchDiaryPage` now returns data and writes nothing. **How to apply:** when a hook races and the consumer must derive state from the response, do not ask for the write to be "moved earlier" — ask for a passthrough so consumer-derived state _inherits_ the guard. Keeps the hook domain-free, which was the other finding on the same PR. +- **Pairing the two writes inside one guard is what closes the mirror case.** `setStatus('error')` and `onPageFailed` now cannot desynchronise, so the "three render guards intersect at blank page" state became unreachable rather than merely unlikely. Prefer a fix that makes the bad state unconstructible over one that makes it rare. +- **A stale-response test is a real revert test only if the stale value is distinctive and asserted absent.** The accepted test resolves the superseded request with `totalItems: 999` and asserts the subtitle both `toHaveTextContent(/7\s*entries/)` **and** `not.toHaveTextContent(/999/)`. The positive assertion alone would pass if the component simply never re-rendered. +- **i18next: a `…Singular`/`…Plural` key pair with `count` passed still resolves.** i18next appends the plural suffix and pops candidates from the end, falling back to the base key when `_one`/`_other` are absent — so the repo's local `entryCountSingular`/`Plural` convention is safe to extend. Confirmed empirically here by tests asserting the exact rendered strings, which is the check to demand rather than reasoning about the resolver. +- **`testIdPrefix` with a default is the cheap reusability proof.** The accepted test renders under `widget-*` and asserts the default `infinite-scroll-*` IDs are _absent_ — a second consumer is demonstrated, not asserted. Ask for the negative half. +- **DEFERRED, filed #2065 — `disabled` on the button the keyboard user is standing on.** Activating "Load more" sets `disabled={status === 'loading'}` on the same DOM node → focus drops to ``, and the next batch needs tabbing back through every appended card. **Not blocked, on two grounds worth reusing:** (1) it was present at round 1 and I missed it, so my own capping rule applies; (2) the remedy (`aria-disabled` + click guard) changes disabled-button semantics app-wide and `wiki/Style-Guide.md` is `ux-designer`'s page — a genuine ownership reason, not a convenience. **General pattern: an in-place async button that disables itself is an a11y defect on every consumer of a shared component; check it whenever a shared control changes state in response to its own activation.** The existing E2E keyboard test asserted the batch loads and stopped before re-asserting focus — the assertion gap is what let it through both rounds. +- **End of a fix loop: state the enumeration as exhaustive.** Round 2 said plainly that nothing further was being held back, which is what stops a mirror-image third round on findings I could have raised earlier. diff --git a/.claude/agent-memory/product-owner/standalone-diary-bugs.md b/.claude/agent-memory/product-owner/standalone-diary-bugs.md index 3853f3e4e..c2c51533e 100644 --- a/.claude/agent-memory/product-owner/standalone-diary-bugs.md +++ b/.claude/agent-memory/product-owner/standalone-diary-bugs.md @@ -9,13 +9,15 @@ EPIC-13 (#446, Construction Diary / Bautagebuch) closed and released. New diary **Why:** the natural parent is closed; we accept ungrouped stories rather than re-opening a closed epic. See [[standalone-bugs-and-stories.md]] for the same pattern in budget/invoice. -**How to apply:** when triaging new diary user-reported improvements, check this list — if it grows (≥4 items), propose a new diary epic at the next planning cycle. If a new diary epic is opened, link these as sub-issues. +**How to apply:** when triaging new diary user-reported improvements, check this list — if it grows (≥4 items), propose a new diary epic at the next planning cycle. If a new diary epic is opened, link these as sub-issues. **Search the tracker before filing** — a same-day duplicate of #2060 was nearly filed on 2026-09-04 because the requester described the symptom rather than checking; `gh issue list --search "diary in:title"` found it in one call. ## Items - **#1426** — BUG: Diary photos lost on upload failure; replace local-stage flow with auto-draft + immediate upload (Closed, shipped). Multi-story mini-epic, all stories merged. Introduced auto-draft on first interaction + immediate photo upload + status column on diary_entries. Surfaced three follow-on UX rough edges (see #1435). - **#1435** — BUG: Diary UX rough edges after #1426 (Todo, 2026-05-17). Three client-only fixes batched in one issue: (a) auto-draft on type-card click instead of intermediate `step === 'form'` state on DiaryEntryCreatePage; (b) photo grid refresh on PhotoUpload.onUpload (current `onUpload={() => {}}` no-op comment hides the bug — usePhotos.refresh() exists and is the simplest fix); (c) replace standalone three-chip status row on DiaryPage with a "Hide drafts" toggle inside DiaryFilterBar. All client-only, no API/schema work. Likely batchable in a single PR. - **#1781** — BUG: Diary list should default to "Manual" filter instead of "All" (Todo, 2026-06-22). Default-value change only on `client/src/pages/DiaryPage/DiaryPage.tsx` line 48 (`filterMode || 'all'` → `'manual'`); filter UI + URL-param handling unchanged. Manual types: daily_log, site_visit, delivery, issue, general_note. Automatic types: work_item_status, invoice_status, invoice_created, milestone_delay, budget_breach, auto_reschedule, subsidy_status. Flagged risk: E2E tests assuming All on initial load may need updates. Now ≥4 standalone diary items — propose diary v2 epic at next planning cycle. +- **#2060** — STORY: Replace broken numbered pager with scroll-driven batch loading (user-story, Should Have, **In Progress** 2026-09-04). 23 ACs + 7 UAT scenarios. Root cause: the debounced-search-sync effect (`DiaryPage.tsx` ~70-83) lists `searchParams`, which `useSearchParams()` re-identifies on every URL change → unconditionally re-runs `newParams.set('page','1')`, clobbering every pager click; direct `?page=N` links work only because `isFirstSearchSyncRef` skips the first pass on remount. Decision: **replace, don't repair** — pager + `?page=` param removed, no page-size selector, scroll loader built as shared component/hook (no `IntersectionObserver` anywhere in `client/src/` today). Endpoint is **`GET /api/diary-entries`** (`server/src/app.ts:268`), NOT `/api/diary` — the body originally said the latter; corrected in place 2026-09-04, no AC touched. PR #2063: CHANGES REQUESTED R1 (5 findings), **APPROVED R2** 2026-09-04 at `d77524a6`, all gates green incl. `E2E Gates`. R2 fix shape worth reusing: the consumer's own state writes moved out of `fetchPage` into new epoch-guarded `onPageApplied(meta, page)` / `onPageFailed(err, page)` callbacks on `useInfiniteScroll`, with `InfiniteScrollPage` carrying an opaque `meta` — the generic way to put consumer-derived state on the safe side of a race guard without domain-leaking the hook. Merge as `Refs #2060` (+ `Fixes #2061`/`Fixes #2062`); #2060 stays In Progress until UAT-1…UAT-7. +- **#2065** — STORY: Load more button loses keyboard focus while its batch loads (Should Have, Backlog, blocked-by #2060, 7 ACs). `InfiniteScrollFooter` sets `disabled` on the same DOM node the keyboard user is standing on → focus drops to ``, next batch needs tabbing back through every appended card. Deferred from PR #2063 R2 because (a) not a R2 regression — present at R1 and I missed it, (b) the remedy (`aria-disabled` + click guard) is a Style-Guide call owned by `ux-designer`. Affects every future consumer of the shared component, not just diary. - **#1688** — BUG: Diary overview filters + search unusable on mobile (Todo, 2026-06-15). CSS-only root cause: `DiaryFilterBar.module.css` mobile breakpoint (`@media max-width:767px`) positions `.filters` panel `absolute; top:100%` but `.filterBar` has no `position: relative` (the `position:relative` at line 25 belongs to `.mobileToggle`). Panel anchors to off-screen `.liveRegion`/viewport → "🔍 Filters" toggle appears dead, mode chips (all/manual/automatic) + search unreachable. Fix: give `.filterBar` a positioning context. Now ≥3 standalone diary items — getting close to proposing a diary v2 epic. ## Key code references for diary draft work diff --git a/.claude/agent-memory/translator/MEMORY.md b/.claude/agent-memory/translator/MEMORY.md index a8af4f471..324db5956 100644 --- a/.claude/agent-memory/translator/MEMORY.md +++ b/.claude/agent-memory/translator/MEMORY.md @@ -110,6 +110,10 @@ New `sourceReports.expand.*` (chevron-expand sub-tables for budget lines + depos - [Non-mandatory register](non-mandatory-register-1941.md) — no semicolon anywhere in de locale files, use en dash "–" as clause joiner instead; avoid "Grenze"/negated "müssen" when spec bans error-blame framing; sr-only strings have no pixel budget +## Singular/Plural Key Splits + +- [Singular/plural announcement split #2060](singular-plural-announcement-split-2060.md) — German strong-declension rule ("weiterer Eintrag" vs "weitere Einträge") when splitting a count+adjective+noun string into *Singular/*Plural pairs + ## Audit Protocol History - [Audit pitfalls](audit-pitfalls.md) — incident history behind the mandatory 4-step full-coverage audit protocol: a parity-only audit missed 13 code-referenced keys (Area UI raw-key bug); loose substring greps flagged 52 false positives diff --git a/.claude/agent-memory/translator/singular-plural-announcement-split-2060.md b/.claude/agent-memory/translator/singular-plural-announcement-split-2060.md new file mode 100644 index 000000000..69dc4593e --- /dev/null +++ b/.claude/agent-memory/translator/singular-plural-announcement-split-2060.md @@ -0,0 +1,57 @@ +--- +name: singular-plural-announcement-split-2060 +description: German declension pattern for splitting a single {{count}}-interpolated string into i18next _one/_other pairs (diary infiniteScroll, Issue #2060) +metadata: + type: project +--- + +**Update (same day, PR #2063 review round 2)**: frontend-developer renamed the English keys again, +from `*Singular`/`*Plural` to i18next's native `_one`/`_other` plural suffix convention (the +"final" naming — matches how i18next's default pluralization resolves German, which also just +uses `_one`/`_other`, same two-category split as English). German text content unchanged, key +names only: `initialLoadAnnouncement_one`/`_other`, `batchAppendedAnnouncement_one`/`_other`, +`batchAppendedAndEndAnnouncement_one`/`_other`. If asked to add further pluralized diary/infinite- +scroll keys, use `_one`/`_other` directly, not `*Singular`/`*Plural` — the latter was an interim +naming that got superseded. Note: this repo's other pluralization keys (`page.entryCountSingular`/ +`entryCountPlural`) still use the older explicit-suffix style — `_one`/`_other` is new to this +codebase as of #2060, introduced specifically because i18next requires it for its `count`-driven +automatic plural resolution (`t('key', { count })`) to work, whereas `entryCountSingular`/`Plural` +are picked manually in code, not through i18next's plural engine. + +**Concurrent-worktree gotcha**: mid-verification, `en/diary.json`'s key names changed under me +between an Edit call and the following diff-script call (frontend-developer's rename landed in the +same shared worktree in that window) — the flattened parity diff briefly showed 6 "missing in de" / +6 "missing in en" that were pure timing noise, not a real gap. Re-running the same diff script +immediately after confirmed 334/334 with zero mismatches. Same lesson as the `closingLabel` +incident in MEMORY.md: always re-check immediately before finalizing rather than trusting one read, +in a shared worktree. + +--- + +Issue #2060 (diary infinite scroll) PR review caught an English grammar bug ("1 entries loaded") +in three `{{count}}`-interpolated `infiniteScroll` announcement keys and split each into +`*Singular`/`*Plural` pairs (superseded by `_one`/`_other`, see update above). The pre-existing German translations had the same latent bug (always +used the plural noun "Einträge" even for count=1) but it was masked because German doesn't +mark this in the same visible way English does — still wrong once split explicitly. + +**Pattern applied** (`client/src/i18n/de/diary.json` `infiniteScroll.*`): + +- Base noun switches "Eintrag" (singular) / "Einträge" (plural) — matches sibling + `page.entryCountSingular`/`entryCountPlural`. +- Where English prepends "more" (`batchAppended*`), the German adjective "weiter-" takes **strong + declension** since no article precedes it and count is a bare numeral: singular is + `"{{count}} weiterer Eintrag geladen"` (masculine nominative singular, `-er` ending), plural is + `"{{count}} weitere Einträge geladen"` (`-e` ending). Do not use "weitere Eintrag" (wrong, + mismatched adjective/noun number) or "weiterer Einträge" (wrong, mismatched the other way) — + this is the easy mistake when splitting a English count/plural pair mechanically without + checking German adjective agreement. + +General rule for future singular/plural key splits: when English source has an adjective before +the noun (more, another, additional, etc.), check German strong/weak declension for that adjective +against the noun's number and case, don't just toggle the noun. + +Verification method used (Jest unrunnable in this worktree — known `ts-node` gap, see +[[audit-pitfalls]]): a throwaway Node script flattening both `en/diary.json` and `de/diary.json` +to dotted-path key sets and diffing both directions — confirmed 334/334 parity, 0 missing either +way. Preferred over trusting Edit tool success alone — see MEMORY.md's "Always verify persistence" +rule, re-confirmed here via `git diff --stat` showing the expected 6-line insert / 3-line delete. diff --git a/.claude/agent-memory/ux-designer/MEMORY.md b/.claude/agent-memory/ux-designer/MEMORY.md index 5565a349a..8f7d6b66c 100644 --- a/.claude/agent-memory/ux-designer/MEMORY.md +++ b/.claude/agent-memory/ux-designer/MEMORY.md @@ -31,11 +31,12 @@ - For `client/src/lib/reportPdf/` PRs (pdfmake document generation, no CSS/components/dark-mode in scope): don't judge column widths/wrapping/page-break behavior from font-metric arithmetic — render the real pipeline to a throwaway `/tmp/*.pdf` via a scratch Jest test and rasterize with `pdftoppm` for visual inspection, then delete the scratch file. See [pdfmake-rendering-verification.md](pdfmake-rendering-verification.md) for the exact technique and a confirmed `dontBreakRows` row-split bug found this way. - pdfmake: an empty-string text node (`{ text: '' }`) reserves the SAME line-height as a non-empty node — never worry it'll collapse to zero, no nbsp workaround needed for an "always reserve this line's space" AC. Also: at 11pt/1.4 line-height, a real rendered line is ~18pt, not the naive `fontSize*lineHeight=15.4pt` — render-and-measure, don't hand-compute. See [pdfmake-rendering-verification.md](pdfmake-rendering-verification.md), Issue #1932. - When an AC complains a UI panel's caption/label "mixes languages" with its value, check whether the real bug is inline concatenation (`"Label: value"` on one line) before translating the caption into the value's language — if sibling captions in the same panel are all interface-language chrome, translating just one caption breaks panel consistency instead of fixing it. Fix by visually separating caption from value (label-above-value, matching the panel's existing label recipe) instead. See Issue #1932 in [feature-spec-history.md](feature-spec-history.md). -- **Playwright browser download is blocked in this sandbox** (`cdn.playwright.dev`/`playwright.download.prss.microsoft.com` both 403 by default-deny network policy), and there is no installable system Chromium (`chromium-browser` here is a snap-transitional stub, `apt-get install chromium` no-ops). For CSS-only component review (no pdfmake involved) when no pixel render is available: build a static HTML harness using the *actual* `.module.css` file contents verbatim (literal, unhashed class names — safe because you write matching literal-class markup yourself, sidestepping the webpack-hashing problem) plus real `tokens.css`, with hand-written markup mirroring the exact JSX tree, then verify by direct CSS-rule/box-model comparison (e.g. "is this property byte-for-byte identical to that sibling's rule", "does `align-self` override the flex container's default stretch") rather than a screenshot. This is real, defensible verification for deterministic box-model questions (button stretch/sizing) but not a substitute for an actual render on subjective spacing/contrast calls — say so explicitly in the review rather than presenting it as equivalent. See PR #1951 in [pr-review-findings.md](pr-review-findings.md). +- **Playwright browser download is blocked in this sandbox** (`cdn.playwright.dev`/`playwright.download.prss.microsoft.com` both 403 by default-deny network policy), and there is no installable system Chromium (`chromium-browser` here is a snap-transitional stub, `apt-get install chromium` no-ops). For CSS-only component review (no pdfmake involved) when no pixel render is available: build a static HTML harness using the _actual_ `.module.css` file contents verbatim (literal, unhashed class names — safe because you write matching literal-class markup yourself, sidestepping the webpack-hashing problem) plus real `tokens.css`, with hand-written markup mirroring the exact JSX tree, then verify by direct CSS-rule/box-model comparison (e.g. "is this property byte-for-byte identical to that sibling's rule", "does `align-self` override the flex container's default stretch") rather than a screenshot. This is real, defensible verification for deterministic box-model questions (button stretch/sizing) but not a substitute for an actual render on subjective spacing/contrast calls — say so explicitly in the review rather than presenting it as equivalent. See PR #1951 in [pr-review-findings.md](pr-review-findings.md). - **`:global(.foo)` in a `.module.css` file only matches a DOM element whose class is the literal unhashed string `"foo"`.** If a component applies another module's class via plain `className={otherModuleStyles.foo}` (not `composes:`), that class resolves to a hashed string (`foo_a1b2c` per `webpack.config.cjs`'s `localIdentName`) in real builds — `:global(.foo)` selectors written to "layer styles on top of" that class never match, silently killing the entire rule block. This is invisible in Jest: `jest.config.ts` maps `.module.css` → `identity-obj-proxy`, which resolves classes to their literal key name, so `:global(.foo)` _does_ match in tests. The correct cross-module extension technique is `composes: foo from '../other/Other.module.css';` inside a locally-scoped class, then apply _that_ local class — this actually merges the real hashed class list onto the element. Check for this specific pattern (`:global(...)` selectors targeting another module's class name, applied via plain className not `composes`) whenever a new component's CSS "doesn't seem to apply" or a spec's described states (hover/focus/at-rest tint/indicator dots) go missing — see PR #1909 in [pr-review-findings.md](pr-review-findings.md). -- A component that renders its own internal `` (e.g. `TriStateCheckbox`), when wrapped in an *outer* `