feat(diary): replace numbered pager with infinite scroll

[steilerDev]

Summary

• Root cause of the broken Next/Previous pager: the debounced-search-sync effect in DiaryPage.tsx listed searchParams in its dependency array — useSearchParams() re-identifies that object on every URL change, so the effect unconditionally re-ran newParams.set('page', '1') and clobbered every pager click back to page 1.
• Decision: replace, don't repair. The numbered pager and ?page= URL param are removed entirely (no page-size selector). In their place, a new shared useInfiniteScroll hook (IntersectionObserver-based, dedupe-safe, idle/loading/error/done state machine) and a new shared InfiniteScrollFooter presentational component drive scroll-triggered batch loading on the diary entries list.
• New i18n keys added under infiniteScroll.* in client/src/i18n/en/diary.json; the now-removed pagination.* keys were deleted.

Fixes #2060
Fixes #2061
Fixes #2062

Notes

• This PR's QA process surfaced and fixed two additional bugs along the way during round-1 review (filed and closed as BUG-2060-1: useInfiniteScroll drops fresh page-1 fetch and leaks stale data on resetKey race #2061 and BUG-2060-2: DiaryPage initial-load a11y announcement is hardcoded English (not i18n) #2062); their fixes are included in this same commit rather than shipped separately, so both are marked as fixed by this PR for traceability.
• Went through dev-team-lead review twice: round 1 returned CHANGES_REQUIRED (3 fixes, including the stale searchParams dependency finding), round 2 returned APPROVED after verification.

Test plan

• Unit tests pass (95%+ coverage) — new useInfiniteScroll.test.tsx, InfiniteScrollFooter.test.tsx, updated DiaryPage.test.tsx
• Integration tests pass
• E2E: updated e2e/pages/DiaryPage.ts POM and e2e/tests/diary/diary-list.spec.ts
• CI Quality Gates pass (typecheck, tests, build, audit)

Co-Authored-By: Claude dev-team-lead noreply@anthropic.com
Co-Authored-By: Claude frontend-developer noreply@anthropic.com
Co-Authored-By: Claude qa-integration-tester noreply@anthropic.com
Co-Authored-By: Claude e2e-test-engineer noreply@anthropic.com

[steilerDev]

[ux-designer] Reviewed the diff against wiki/Style-Guide.md and client/src/styles/tokens.css. Scope: InfiniteScrollFooter (new shared component), useInfiniteScroll (new shared hook), and the DiaryPage.tsx/.module.css changes that consume them.

Token adherence — Clean. Every color/spacing/font-size/radius value in InfiniteScrollFooter.module.css resolves to an existing token (--spacing-2/3/4/6/12, --color-text-muted, --color-border, --font-size-sm). No hardcoded hex or px values apart from 44px/767px, both of which match the codebase's established (non-tokenized) touch-target and mobile-breakpoint conventions used identically across ~15 other components (CalendarView, Sidebar, PhotoMetadataModal, etc.) — not a new deviation.

Dark mode — All colors route through semantic (Layer 2) tokens (--color-text-muted, --color-border) that already flip under [data-theme="dark"]; no Layer 1 palette token referenced directly. Verified --color-gray-500→--color-slate-300 and --color-gray-200→--color-slate-500 mappings exist and keep sufficient contrast.

Component reuse — Correctly built as new shared artifacts in the right locations (client/src/components/InfiniteScrollFooter/, client/src/hooks/) rather than page-local one-offs — this is a genuinely new pattern (grepped for any prior IntersectionObserver/load-more usage in client/src, found none) so a new shared component is justified, not a duplication. Within it, correctly reuses shared.btnSecondary (inherits the correct box-shadow: var(--shadow-focus) focus-visible treatment, not outline), Spinner, and FormError instead of reinventing button/spinner/error-banner styling.

Interactive states / accessibility — Native <button> gives keyboard activation (Enter/Space) for free; same DOM node persists across idle→loading→error so focus/locator stability holds. FormError banner correctly gets role="alert" only in error state. Sentinel div is aria-hidden (correctly excluded from the accessibility tree — it's a scroll trigger, not content). role="feed" gains aria-busy={status === 'loading'}, a nice touch for AT users during batch appends. i18n keys (infiniteScroll.*) are present and symmetric between en/de.

Animation — The button's inline Spinner already respects prefers-reduced-motion (pre-existing in Spinner.module.css); no new unconditional animation introduced.

Responsive — Mobile media query widens the button to 100% and sets min-height: 44px, matching the app's touch-target convention.

No blocking findings.

Verdict: APPROVED

[steilerDev]

[ux-designer] Note: could not use gh pr review --approve (GitHub blocks approving a PR authored under this session's own gh identity, per the repo's own PR authorship regardless of displayed author). Posting the review as a comment with an explicit verdict instead — see above.

Verdict: APPROVED

[steilerDev]

VERDICT: REQUEST_CHANGES

[product-owner] Round 1 requirements review of PR #2063 against #2060 (23 ACs / 7 UAT scenarios), #2061 and #2062.

gh pr review --request-changes is rejected on a PR authored by this token, so the verdict is carried on the first line per CLAUDE.md > Reviewer Verdict Policy.

What I verified

The replace-don't-repair decision is executed faithfully and most of the acceptance record is met. Confirmed against the diff and the branch head:

• AC1, AC12, AC20 — pager, Page X of Y, prev-page-button/next-page-button and the ?page= read are gone. Every filter handler now does newParams.delete('page'), and handleClearAll builds a fresh URLSearchParams, so no path can re-introduce page. A legacy /diary?page=3&q=foo honours q and loads batch 1.
• The root cause is actually fixed. The debounced-search effect no longer lists searchParams; it uses the setSearchParams((prev) => …) functional form with [debouncedSearchInput, setSearchParams] deps. That is the defect from the issue's root-cause section, closed at the source rather than worked around.
• AC3–AC8 — IntersectionObserver with a 600px bottom lookahead; append via the page === 1 ? replace : [...prev, ...] branch; window.scrollTo removed (AC4); inFlightRef plus the statusRef.current !== 'idle' gate dedupe concurrent triggers (AC6); hasMore = page < totalPages drives done so a single-page dataset reaches end-of-list with no second request (AC8).
• AC9 — footer is gated on entries.length > 0, so a zero-match filter renders the empty state with no end-of-list row, no sentinel and no Load more. Covered in E2E.
• AC13–AC15 — footer-local FormError + retry, top banner suppressed via error && entries.length === 0 so loaded cards survive a failed append; retry() reuses pageRef.current (not advanced on failure), and loadMore no-ops in error so the observer cannot loop.
• AC16, AC21, AC23 — keyboard-reachable Load more is always rendered outside done; scroll-observer logic lives entirely in client/src/hooks/useInfiniteScroll.ts with zero observer code left in DiaryPage.tsx; the footer CSS is fully tokenised.
• BUG-2060-2: DiaryPage initial-load a11y announcement is hardcoded English (not i18n) #2062 is fixed — initialLoadAnnouncement added to en/de and the hardcoded template literal is gone. The it.failing tripwire for BUG-2060-1: useInfiniteScroll drops fresh page-1 fetch and leaks stale data on resetKey race #2061 has been promoted to a real it, and the epoch guard in runFetch is the right shape for it.
• CI is green including E2E Gates, and the E2E suite covers the three-viewport requirement of AC22 and the dark-mode footer states.

Findings — all fix-in-session

1. fix-in-session — High — AC2 and AC10: a superseded fetch can overwrite the header total (residual half of #2061)

fetchDiaryPage performs two state writes of its own — setTotalItems(response.pagination.totalItems) and setError('') — inside the promise, before it returns to runFetch. The epoch guard added for #2061 sits in runFetch, after fetchPageRef.current(page) has already resolved, so it protects items/hasMore/status but not the consumer's own writes.

The reset effect deliberately clears inFlightRef so two fetches from different generations are genuinely concurrent, which makes this reachable:

1. Filter A loaded, total 100. Click Load more → page-2 request for filter A in flight.
2. Change the filter (or type a search term) before it resolves → epoch bumps, fresh page-1 request for filter B in flight.
3. Filter B's page 1 resolves first: setTotalItems(20), list shows filter B.
4. Filter A's page 2 resolves: setTotalItems(100) runs unguarded; only its items are discarded by the epoch check.

The subtitle then reads "100 entries" over filter B's 20 entries and never self-corrects, because no further fetch is issued. That breaks AC2 ("the subtitle shows the total number of entries matching the current filters") and AC10 ("the header total updates").

The same unguarded setError('') produces a worse mirror case: if filter B's page 1 fails and the stale filter-A request then succeeds, error is cleared while status stays 'error' — the banner disappears, the empty state is suppressed by status !== 'error', and the footer is suppressed by entries.length === 0. The page renders nothing at all below the filter bar, with no retry affordance and no way out but a reload.

The fix belongs on the same side of the epoch guard as the items — e.g. let InfiniteScrollPage<T> carry an opaque per-batch passthrough that the hook surfaces only for non-superseded responses, so consumer-derived state inherits the guard. I am not prescribing the mechanism; product-architect should confirm whichever shape is chosen keeps the hook generic. Please add a unit test that pins the interleaving above — the existing #2061 test asserts the items are discarded but says nothing about the total.

2. fix-in-session — Medium — InfiniteScrollFooter is a shared component hardcoded to the diary domain

The component sits in the shared client/src/components/ tree, but every string is read from the diary namespace (t('diary:infiniteScroll.loadingMore') and five siblings) and all three hooks are diary-named: data-testid="diary-infinite-scroll-footer", diary-end-of-list, diary-load-more-button.

AC21 passes on its literal wording — the scroll logic is in the hook — but #2060 assumption 5 is explicit that "other list views must be able to adopt it later", and CLAUDE.md > Component Reuse Policy rule 3 requires new components to be genuinely reusable. As written, the second consumer inherits diary copy and diary test IDs. Moving the six infiniteScroll.* keys to a shared namespace (or accepting labels/namespace as props) and parameterising the test-ID prefix is cheap now and a rename-across-two-pages later. Note this makes the keys a translator change as well as a frontend one.

3. fix-in-session — Medium — AC5: "Loading more entries…" renders twice during an append

While status === 'loading' the footer renders both the statusRow (spinner + infiniteScroll.loadingMore) and the Load more button in its loading state (spinner + the same infiniteScroll.loadingMore). Two identical labels and two spinners stack vertically. AC5 asks for "a loading affordance", singular. Pick one — either the status row or the in-button state — and drop the other. Deferring to ux-designer on which; I only require that one of the two goes.

4. fix-in-session — Low — the three new {{count}} announcement keys are not pluralised

initialLoadAnnouncement, batchAppendedAnnouncement and batchAppendedAndEndAnnouncement interpolate count with no _one/_other variants, so i18next falls back to the base key and a single entry announces "1 entries loaded" / "1 more entries loaded" (and the German equivalents). The diary namespace already solves this exact problem two lines above in the same render — the subtitle uses page.entryCountSingular / page.entryCountPlural — and dashboard.json / budget.json use the _one/_other convention throughout. Screen-reader-only copy, hence Low, but it is a two-key edit in each locale.

5. fix-in-session — Low — dead hasMore prop

InfiniteScrollFooterProps declares hasMore: boolean and DiaryPage passes it, but the component never destructures or reads it — status === 'done' already encodes it. ESLint cannot see it because it is never destructured. Remove it from the interface and the call site.

Merge instruction — Refs #2060, not Fixes #2060

#2060 carries 7 UAT scenarios, plus AC4 ("my reading position does not jump"), AC19 (visible focus indicator in light and dark) and AC22 (the mobile interaction that "previously did nothing"). Those are rendered-appearance and feel claims — the automated suite raises confidence but is not the acceptance gate. #2060 is standalone (EPIC-13 is closed), so /epic-close — the only workflow with a UAT step — never runs for it, and a standalone /release promotion would auto-close it unvalidated the moment a Fixes #2060 line reached main.

• Squash body: Refs #2060, and Fixes #2061 / Fixes #2062 (both machine-checkable against the source, no UAT surface).
• Diary: replace the broken numbered pager with scroll-driven batch loading #2060 stays In Progress on the board after merge and moves to Done only after UAT-1 … UAT-7 pass. I will reopen it rather than file a follow-up if UAT fails.
• Minor correction to the PR description: it states BUG-2060-1: useInfiniteScroll drops fresh page-1 fetch and leaks stale data on resetKey race #2061 and BUG-2060-2: DiaryPage initial-load a11y announcement is hardcoded English (not i18n) #2062 were "filed and closed" — both are currently open, and a squash into beta will not close them (GitHub only honours the keyword on a merge into the default branch). They close on promotion.

Scope

In scope and clean. No page-size selector was added (assumption 2 honoured), the DataTable pages are untouched (out-of-scope list respected), no backend or pagination-contract change was made (assumption 6), and the removed pagination.* keys were deleted from both locales as the issue's Notes asked. Bundling #2061 and #2062 into this PR is correct — both were found in this PR's own new code and neither is separable from it.

Board status for #2060 is In Progress, as required. No other reviewer verdicts are on the PR yet; reviews run in parallel with CI under the current policy, so that is not a finding — the orchestrator still gates the merge on product-architect and ux-designer (client-only change, so security-engineer does not apply).

Nothing is deferred, so no follow-up issues are filed. Re-request review once findings 1–5 are addressed.

[steilerDev]

[product-architect]

Verdict: CHANGES REQUESTED

The architecture of this rework is right, and the #2061 fix is genuinely well done — the epoch counter is the correct mechanism and its regression test is a real revert test (it asserts fetchPage reaches 3 calls and uses a hasMore: false discriminator so it cannot pass by items-array coincidence). All 4 blocking findings below are mechanical, low-effort edits: fix-in-session, no re-spec needed.

Verified

• AC 21 (reuse) — scroll logic lives in client/src/hooks/useInfiniteScroll.ts + client/src/components/InfiniteScrollFooter/, zero observer logic inline in DiaryPage.tsx. Correct shape.
• Pager removal — no searchParams.get('page') anywhere; styles.pagination/pageButton/pageInfo all removed from DiaryPage.module.css with no orphan references; prevPageButton/nextPageButton fully gone from e2e/pages/DiaryPage.ts with no dangling consumers. Diary pagination.* i18n keys deleted; the surviving pagination.* hits are common:dataTable.pagination.* (DataTablePagination.tsx), a different namespace — correctly untouched.
• AC 6/7/15 (no duplicate request, no runaway, no error loop) — loadMore() gates on statusRef.current !== 'idle' (cross-render) and runFetch gates on inFlightRef (same-tick). The two guards are complementary and together close both windows; 'done' and 'error' both make the observer callback a no-op.
• AC 14 (retry same page) — pageRef.current advances only in the success path, so retry() re-issues the failed page. Pinned by a dedicated unit test.
• fetchPage read via fetchPageRef — a new function identity per render does not retrigger a fetch, and there is a test pinning exactly that. The right call, given fetchDiaryPage is redefined every render.
• i18n — en/de key sets match exactly (9 keys each), {{count}} interpolation present in both. No orphan keys: endOfListAnnouncement is referenced in agent memory but exists in neither file, so nothing dead shipped.
• CI fully green (Quality Gates, all 6 test shards, all 16 E2E shards, Trailer Check).
• No API, schema, or migration surface — no wiki update is owed by this PR. Correct that wiki/ is untouched.

---

Blocking

1. HIGH — fetchDiaryPage's side effects sit outside the epoch guard, so a superseded batch clobbers the header total (AC 2, AC 10)

client/src/pages/DiaryPage/DiaryPage.tsx, in fetchDiaryPage:

const response = await listDiaryEntries({ page, pageSize: PAGE_SIZE, ... });

setTotalItems(response.pagination.totalItems);   // <-- unguarded
setError('');                                    // <-- unguarded
return { items: response.items, hasMore: page < response.pagination.totalPages };

Both setState calls run before control returns to runFetch, which is where the epoch !== epochRef.current check lives. So the hook correctly discards the items of a superseded batch while the consumer has already committed that batch's metadata.

Failure scenario — the exact one #2061 describes:

1. Filter A loaded (100 matching entries), user scrolls → fetchPage(2) for A in flight.
2. User changes the filter to B → epoch bumps, fresh fetchPage(1) for B.
3. B's page 1 resolves first: header shows B's total (say 5), list shows B's entries.
4. A's page 2 resolves: items discarded by the epoch check, but setTotalItems(100) already fired. The header now reads "100 entries" over a 5-entry list for filter B, and stays wrong until the next fetch.

setError('') has the mirror problem in the failure direction: a superseded batch that rejects calls setError(...) and is then discarded by the epoch check (so status never becomes 'error'). The page-level banner is gated on error && entries.length === 0, which is exactly true during the reset window — so a spurious banner appears for a filter whose own fetch has not failed. If the new filter legitimately returns zero items, the banner and the empty state render simultaneously and the banner never clears.

This is the same bug class the PR just fixed, one layer up. Fix — capture the key at fetch start and guard both writes:

const liveResetKeyRef = useRef(resetKey);
liveResetKeyRef.current = resetKey;

const fetchDiaryPage = async (page: number) => {
  const startedUnder = resetKey;               // closure value at fetch-start
  try {
    const response = await listDiaryEntries({ ... });
    if (liveResetKeyRef.current === startedUnder) {
      setTotalItems(response.pagination.totalItems);
      setError('');
    }
    return { items: response.items, hasMore: page < response.pagination.totalPages };
  } catch (err) {
    if (liveResetKeyRef.current === startedUnder) {
      setError(err instanceof ApiClientError ? err.error.message : t('error'));
    }
    throw err;
  }
};

Deliberately not widening InfiniteScrollPage<T> with a metadata passthrough — the hook should not grow a channel for one consumer's header count.

Also required: document this sharp edge on the hook, because the next consumer will hit it. Add to fetchPage's JSDoc in useInfiniteScroll.ts:

fetchPage may be invoked for a batch that is later superseded by a resetKey change; the hook discards such a result. Side effects performed inside fetchPage are not covered by that guard — consumers must guard them against supersession themselves.

Please add a unit test for the race: mount, hold loadMore() open, change the search param, let the new page 1 resolve, then resolve the stale page 2, and assert the subtitle still shows the new total.

2. MEDIUM — the new shared component is hardcoded to the diary namespace and diary-* testids, so no other list view can adopt it (AC 21 / issue assumption 5)

InfiniteScrollFooter.tsx lives in client/src/components/ but reads t('diary:infiniteScroll.errorMessage'), t('diary:infiniteScroll.loadMoreButton') etc., and emits data-testid="diary-load-more-button" / "diary-end-of-list" / "diary-infinite-scroll-footer".

Every other shared component in this repo takes useTranslation('common') — Modal, SearchPicker, and all seven DataTable* files. DataTable is the direct analogue here (shared list infrastructure with its own pager) and its strings live at common:dataTable.pagination.*. A second consumer of InfiniteScrollFooter would have to either duplicate the infiniteScroll.* block into its own namespace or be labelled out of the diary namespace.

The giveaway is inside this one file: the sentinel is already correctly generic (data-testid="infinite-scroll-sentinel") while its two siblings are diary-prefixed. Cheap now, expensive once a second list view depends on the names.

Fix:

• Move the six UI keys — loadingMore, loadingMoreAriaLabel, loadMoreButton, retryButton, errorMessage, endOfList — from diary.json to common.json under infiniteScroll.*, en and de (the German strings already exist verbatim; relocate, don't retranslate). Generalise loadingMoreAriaLabel's copy, which currently says "diary entries" / "Tagebucheinträge".
• Keep the three announcement keys (initialLoadAnnouncement, batchAppendedAnnouncement, batchAppendedAndEndAnnouncement) in diary.json — they are diary-worded and owned by DiaryPage, not by the shared component.
• useTranslation() → useTranslation('common') and drop the diary: prefixes.
• Rename the testids to infinite-scroll-load-more / infinite-scroll-end-of-list / infinite-scroll-footer. 24 references across 5 files (InfiniteScrollFooter.tsx, its test, DiaryPage.test.tsx, e2e/pages/DiaryPage.ts, diary-list.spec.ts) — mechanical.

3. MEDIUM — the reset effect does not reset fetchSequence, contradicting the field's own documented contract and mislabelling the post-filter-change announcement

useInfiniteScroll.ts documents the field as:

/** Increments once per successful fetch (including the first). Distinguishes "first batch" (sequence === 1) from "appended batch" (sequence > 1). */

but the reset effect clears items/hasMore/status and leaves fetchSequence (and lastBatchCount) alone. So fetchSequence === 1 is true only for the very first fetch of the component's entire lifetime. After any filter or search change, the freshly-replaced first batch has sequence > 1, and DiaryPage's announcement effect falls into the append branch:

"5 more entries loaded" — for a list that was just discarded and replaced.

That is wrong copy for a screen-reader user (AC 17 scopes the append wording to appends), and it makes initialLoadAnnouncement — the key #2062 was filed to add — unreachable after the first load. The documented meaning of fetchSequence is the contract; the implementation is what is wrong here. One line, inside the block that already carries the set-state-in-effect disable:

setFetchSequence(0);
setLastBatchCount(0);

No existing test pins the current behaviour, so nothing has to be weakened. Please add one asserting fetchSequence returns to 1 for the first batch after a resetKey change, plus a DiaryPage test that a filter change announces the initialLoadAnnouncement copy rather than the append copy.

4. LOW — InfiniteScrollFooter's hasMore is a required prop that is never read

export interface InfiniteScrollFooterProps {
  status: 'idle' | 'loading' | 'error' | 'done';
  hasMore: boolean;          // <-- declared
  ...
}

export function InfiniteScrollFooter({ status, sentinelRef, onLoadMore, onRetry }: InfiniteScrollFooterProps) {

hasMore is not destructured and appears nowhere in the body — every rendering decision derives from status, and status === 'done' already is !hasMore as the hook computes it. The unit tests pass hasMore in their prop factory, so they don't catch it either.

A required-but-ignored prop on a brand-new shared contract mandates dead work at every future call site and implies a behaviour that does not exist. Remove it from the interface, the DiaryPage call site, and the test prop factory.

---

Non-blocking (no action required in this PR)

• runFetch's finally clears a flag it may not own. A superseded fetch's finally { inFlightRef.current = false } fires even when a different, still-in-flight fetch set the flag. It is harmless today only because statusRef covers the cross-render window while inFlightRef only needs to cover the same-tick window — a genuine but entirely undocumented two-guard invariant. Worth one comment above inFlightRef saying which window each guard owns, so that a later "simplification" dropping one does not silently reopen AC 6.
• First-batch failure has no retry affordance — the footer, and hence the only retry control, is gated on entries.length > 0. Not a regression (beta had no retry either) and outside AC 13/14, which both presume ≥1 loaded batch. Filed as Diary: a first-batch load failure offers no retry affordance (footer is gated on entries.length > 0) #2064 with two suggested directions and an acceptance sketch.
• min-height: 44px hardcoded in InfiniteScrollFooter.module.css's mobile media query rather than a token. Consistent with existing touch-target precedent in this codebase; flagging for the ux-designer's call, not mine.
• 600px lookahead vs a 25-item batch: on a tall viewport with short cards the sentinel can already be inside rootMargin at observe() time, so several batches chain-load with no user scroll at all. Self-limiting (stops at done) and standard for lookahead infinite scroll, but it does mean PAGE_SIZE = 25 is now load-bearing for perceived behaviour, not just request size. Note only.

[steilerDev]

[ux-designer] Re-confirming approval on the removal of the standalone `.statusRow` (item 2/6 of my original spec on #2060).

I reviewed InfiniteScrollFooter.tsx/.module.css at the current HEAD and agree with product-owner/product-architect: the coexisting row was the wrong call on my part, not a case of "reinforcement." Rationale for reversing it:

1. No accessibility loss. DiaryPage.tsx has its own dedicated role="status" aria-live="polite" aria-atomic="true" live region (announcementRef) that already announces initial-load / batch-appended / end-of-list text, fully independent of InfiniteScrollFooter. Removing .statusRow doesn't remove any non-visual signal — it removes a second, redundant visual+DOM copy of "Loading more entries…" that duplicated what the live region was already saying. My original framing of that as intentional reinforcement undersold the real cost: two same-text nodes in one footer region is exactly the kind of duplicate-announcement risk PO/architect flagged.
2. Visual prominence is preserved. The load-more button keeps its shared.btnSecondary styling and min-height: var(--spacing-12) (44px on mobile) regardless of state, so the loading indicator doesn't appear in a smaller/collapsed element — it's the full content swap of the same button box the user's scroll (via the IntersectionObserver sentinel sitting just above it) naturally lands on. There's no reflow or button-size change between idle/loading states, so nothing shifts underneath the user's viewport when loading kicks in.
3. One minor, non-blocking observation: the removed row used Spinner size="md" (24px) while the button's inline spinner is size="sm" (16px) — the smaller spinner is now the only visible loading affordance. Given point 2 above (the button is already where the user's attention/scroll position is), I don't consider this a blocker, but if there's an easy follow-up, bumping the button's spinner to size="md" would give a slightly stronger visual cue for low-vision users. Not required for this PR.

Verdict: approved — the single inline indicator inside the button is sufficient, and this is a net improvement over my original spec.

[steilerDev]

VERDICT: APPROVE

[product-owner] Round 2 requirements review of PR #2063 against #2060 (23 ACs / 7 UAT scenarios), #2061 and #2062. Verified at head d77524a6.

gh pr review --approve is rejected on a PR authored by this token, so the verdict is carried on the first line per CLAUDE.md > Reviewer Verdict Policy.

All five round-1 findings are closed

1 (High) — superseded fetch overwriting the header total: fixed at the right layer. fetchDiaryPage no longer writes state of its own; it returns meta: { totalItems } and the consumer's writes moved into the new onPageApplied / onPageFailed callbacks, which runFetch invokes after if (epoch !== epochRef.current) return. setTotalItems and setError('') therefore now sit on the same side of the epoch guard as items/hasMore/status, which is exactly what the finding asked for, and the generic passthrough keeps the hook domain-free. The setError mirror case — banner cleared while status stays 'error', leaving a blank page with no retry affordance — is closed by the same change: setStatus('error') and onPageFailed are now inside one epoch guard, so they can no longer be desynchronised.

The demanded test exists and is a real revert test, not a vacuous one. DiaryPage.test.tsx's "when a slow first (pre-filter-change) response resolves after a fast second..." holds the mount fetch open, changes the search, asserts the subtitle reads 7, then resolves the stale request with totalItems: 999 and asserts the subtitle is still 7 and never contains 999. Under the round-1 code that final assertion fails, which is the property I wanted pinned. Two hook-level tests back it (onPageApplied not called for a stale resolve; onPageFailed not called for a stale reject, with the current key's own outcome asserted unaffected).

2 (Medium) — footer hardcoded to the diary domain: fixed via the props route. useTranslation is gone from the component; six label/message props and a testIdPrefix (defaulting to infinite-scroll) replace the diary-namespace t() calls and the three diary-* test IDs. DiaryPage passes testIdPrefix="diary", so the E2E surface is unchanged in spirit and the POM was updated in step. The "applies a custom prefix to all four testids" test renders under widget-* and asserts the default prefix is absent — a second consumer is now demonstrably supported rather than merely asserted.

3 (Medium) — AC5 double loading affordance: fixed. The standalone .statusRow and its CSS block are removed; the in-button spinner + label is the single affordance, with the aria label moved onto it so nothing was lost. expect(screen.getAllByText('Loading more entries…')).toHaveLength(1) pins it and fails if the row returns.

4 (Low) — pluralisation: fixed, following the local convention. Six …Singular/…Plural keys in both en and de, selected on lastBatchCount === 1. This matches page.entryCountSingular/entryCountPlural two lines away in the same namespace, which is the convention I cited. The German forms are correctly declined (weiterer Eintrag / weitere Einträge), and the DiaryPage tests assert the exact rendered strings, so the i18next fallback path (a suffixed key with count present but no _one/_other variants resolving to the base key) is empirically confirmed rather than assumed.

5 (Low) — dead hasMore prop: removed from both the interface and the call site. hasMore is still consumed from the hook by the announcement effect, which is correct.

Round-2 regression check

I re-checked what the round-2 edits made newly reachable:

• setFetchSequence(0) / setLastBatchCount(0) in the reset effect is a genuine improvement, not just bookkeeping: a filter change now announces the new result set as "N entries loaded" rather than "N more entries loaded". Pinned by the new "changing filters announces the new filter's first batch via initial-load phrasing" test, which asserts both the positive copy and the absence of the appended copy.
• Clearing inFlightRef on reset still lets a stale fetch's finally release the dedupe flag while a current fetch is in flight, but loadMore's statusRef.current !== 'idle' gate covers that window, so AC6 holds. No change from round 1.
• No AC regressed: AC1/AC12/AC20 (pager and ?page= fully gone), AC2/AC10 (total now correct under interleaving), AC3–AC9, AC13–AC15, AC16/AC21/AC23 all re-verified at this head. Static Analysis, Quality Gates and E2E Gates are all green.

I consider my enumeration of this PR complete across both rounds — nothing further is being held back.

Deferred, filed as #2065 — Medium, not blocking

Load more loses keyboard focus while its batch loads. The button carries disabled={status === 'loading'} on the same DOM node across states, so activating it by keyboard blurs the element the user is standing on; focus drops to <body> and the next batch requires tabbing back through every newly appended card.

Deferred rather than fixed in session for two reasons: it is not a round-2 regression (the attribute was present at round 1 and I did not flag it), and the remedy — aria-disabled plus a click guard, keeping the node focusable — is a design-system call on wiki/Style-Guide.md that belongs to ux-designer, since it changes the semantics of disabled buttons app-wide. #2065 carries 7 ACs including the E2E gap: the existing keyboard test asserts the batch loads but stops before re-asserting focus. Filed as Should Have, Backlog, blocked-by #2060.

AC16's letter is met — one keyboard activation loads one batch — which is why this does not block.

Merge instruction (unchanged from round 1)

• Squash body: Refs #2060, plus Fixes #2061 and Fixes #2062. Diary: replace the broken numbered pager with scroll-driven batch loading #2060 carries 7 UAT scenarios and rendered-appearance claims (AC4 reading position, AC19 focus indicator in both themes, AC22 the mobile interaction that "previously did nothing") that the automated suite raises confidence in but does not settle. It is standalone, so /epic-close — the only workflow with a UAT step — never runs for it, and a Fixes #2060 line reaching main via a standalone /release would auto-close it unvalidated.
• Diary: replace the broken numbered pager with scroll-driven batch loading #2060 stays In Progress on the board after merge and moves to Done only once UAT-1 … UAT-7 pass. I reopen it rather than file a follow-up if UAT fails.
• The PR description still says BUG-2060-1: useInfiniteScroll drops fresh page-1 fetch and leaks stale data on resetKey race #2061 and BUG-2060-2: DiaryPage initial-load a11y announcement is hardcoded English (not i18n) #2062 were "filed and closed" — both are open as of this review, and a squash into beta will not close them. They close on promotion to main. Worth correcting the body before merge for the change inventory's sake.

Scope

Still clean. No page-size selector, DataTable pages untouched, no backend or pagination-contract change, removed pagination.* keys gone from both locales. Round 2 added no scope beyond the five findings and their tests.

[steilerDev]

VERDICT: REQUEST_CHANGES

[product-architect]

Verdict: CHANGES REQUESTED (round 2)

All four round-1 blockers are genuinely resolved, and two of the three fixes are better than what I proposed. Both findings below are mechanical and contained to this PR's files — fix-in-session, one commit. Flagging up front that this is round 2 of the capped review loop: if the orchestrator judges the cap binding, item 1 is the one that must not be deferred silently, and item 2 can go to a filed issue.

---

Round-1 blockers — verified fixed

1 (HIGH, unguarded side effects in fetchDiaryPage) — fixed, better than proposed. I asked for a consumer-side ref guard and explicitly warned against widening InfiniteScrollPage<T>. The implementation widened it anyway — to InfiniteScrollPage<T, M = undefined> with an optional meta, plus onPageApplied(meta, page) / onPageFailed(error, page) fired inside the epoch check in runFetch. That is the better call and I withdraw the objection: the default type parameter keeps the contract free for consumers that don't need metadata, and it moves the invariant into the hook instead of asking every future consumer to re-derive a ref guard. setTotalItems and setError('') now sit in onPageApplied; setError(...) in onPageFailed. fetchDiaryPage is side-effect-free.

The JSDoc obligation is met — onPageApplied reads "Use this — not side effects inside fetchPage itself — for any state a stale, discarded batch must not be allowed to affect." That is the invariant stated affirmatively, which is what the next consumer needs.

Test coverage is real, not decorative. does not call onPageApplied for a stale fetch that resolves after a resetKey change holds the mount fetch open on a deferred promise, resolves k2's page 1, then resolves the stale k1 batch with different meta ({ n: 999 }) and asserts toHaveBeenCalledTimes(1). Moving onPageAppliedRef.current?.(...) above the epoch !== epochRef.current line makes it 2. The mirror test does the same for a stale rejection. And the page-level test asserts .subtitle still reads 7 entries and not.toHaveTextContent(/999/) after the stale response lands — the exact failure scenario from the finding, pinned end to end.

2 (MEDIUM, diary-hardcoded namespace/testids) — fixed via a different route, also acceptable. Rather than relocating keys to common:infiniteScroll.*, the component was made copy-agnostic: seven required label props, no useTranslation at all, and a testIdPrefix prop defaulting to 'infinite-scroll' with DiaryPage passing "diary". That satisfies the reuse requirement more completely than my proposal did — consumer #2 controls both copy and testids without touching a shared namespace. The diary-worded loadingMoreAriaLabel is now correctly the consumer's string, not the component's. e2e/pages/DiaryPage.ts was updated in step (diary-footer, diary-sentinel) and its header comment documents the prefix mechanism.

3 (MEDIUM, fetchSequence not reset) — fixed. setFetchSequence(0) / setLastBatchCount(0) added to the reset effect. Pinned by resets fetchSequence to 1 (not accumulated)... (asserts toBe(1), not toBe(2)) and by the DiaryPage test asserting a filter change announces 2 entries loaded and not.toHaveTextContent(/more entr(y|ies) loaded/i). Reverting either line fails both.

4 (LOW, dead hasMore prop) — fixed. Gone from the interface, the call site, and the test factory. The hook still returns hasMore and DiaryPage genuinely reads it in the announcement effect, so nothing became dead in the process.

Also verified: no orphan references to the removed diary-infinite-scroll-footer testid or the removed .statusRow class; infinite-scroll-sentinel survives only in the component's own tests exercising the default prefix, which is correct. en/de infiniteScroll key sets match (12 each). CI fully green on d77524a6 — Quality Gates, all 6 unit shards, all 16 E2E shards, E2E Gates, Static Analysis.

---

Blocking

1. MEDIUM — hand-rolled Singular/Plural key suffixes bypass i18next's native pluralization and hardcode a two-form plural assumption

client/src/pages/DiaryPage/DiaryPage.tsx:

const suffix = lastBatchCount === 1 ? 'Singular' : 'Plural';
...
t(`infiniteScroll.initialLoadAnnouncement${suffix}`, { count: lastBatchCount });

against six new key pairs in en/diary.json and de/diary.json (initialLoadAnnouncementSingular/Plural, batchAppendedAnnouncement*, batchAppendedAndEndAnnouncement*).

This is functionally correct today and I am not claiming a live bug — count is passed, i18next probes …Singular_one, misses, and falls back to the base key, which the tests confirm renders 1 more entry loaded / 2 entries loaded. English and German both have exactly two plural forms, so the binary split is currently expressible.

The problem is that it is a convention deviation with named in-repo comparators, and it bakes a constraint into new code:

• The repo already does this natively, extensively. client/src/i18n/en/dashboard.json (summaryItems_other, summarySources_other, ariaChartItems_other, program_other) and client/src/i18n/en/budget.json (areaLineCount_other, selectedCount_other, movingCount_other, claimedWarningHeading_other, successToast_other) all use i18next's _one/_other suffixes with the same {{count}} interpolation. This PR is the odd one out.
• It is not extensible to the locales CLAUDE.md tells us how to add. CLAUDE.md documents an explicit locale-extension path ("To add a locale: update glossary.json _meta.locales, create client/src/i18n/{locale}/…"). A locale with more than two plural categories — Polish, Russian, Czech, Arabic — cannot be expressed by a Singular/Plural pair chosen at the call site. i18next's native suffixes resolve the CLDR category automatically and need no call-site change at all. Fixing this after a third locale exists means editing call sites, not just JSON.
• The dynamically-built key defeats every form of static key analysis — grep, the t() key audits QA runs, and any future extraction tooling. i18n.parity.test.ts catches en/de set mismatches but does no usage scan, so nothing else in the repo covers this.
• Passing {{count}} and hand-suffixing reads like native pluralization while not being it, which is the specific way the next person gets this wrong.

Fix (mechanical, ~12 key renames + one deleted line):

• Rename in both en/diary.json and de/diary.json: …Singular → …_one, …Plural → …_other, for all three announcement families. Strings are unchanged — relocate, don't retranslate.
• Delete the suffix computation and call the base keys directly:

  t('infiniteScroll.initialLoadAnnouncement', { count: lastBatchCount })
  t('infiniteScroll.batchAppendedAndEndAnnouncement', { count: lastBatchCount })
  t('infiniteScroll.batchAppendedAnnouncement', { count: lastBatchCount })

• The existing DiaryPage.test.tsx assertions ('1 more entry loaded', '2 entries loaded') should pass unchanged — if one does not, that is a real resolution problem worth surfacing rather than papering over.

Out of scope, noted only: page.entryCountSingular / page.entryCountPlural (used by the subtitle) has the same shape and predates this PR on beta. It is the local precedent this change followed, and it is why I am calling this Medium rather than High. Please do not widen the PR to fix it — but do not extend it either.

2. LOW — a new shared component and hook are not registered in CLAUDE.md's Component Reuse Policy

InfiniteScrollFooter is now a first-class shared component under client/src/components/, and useInfiniteScroll is a shared hook, but CLAUDE.md's "Shared components (must be used instead of creating alternatives)" list still reads Badge / SearchPicker / Modal / Skeleton / EmptyState / FormError. CLAUDE.md's own Cross-Team Convention says a new shared pattern must be documented there, and this PR is precisely the "reusable list infrastructure" case a future list view should be routed to rather than reinventing.

One bullet:

• InfiniteScrollFooter — sentinel + load-more/retry button + end-of-list message for infinite-scrolling lists; pair with the useInfiniteScroll hook (client/src/hooks/useInfiniteScroll.ts), which owns the IntersectionObserver, paging, and stale-response epoch guard. Copy is supplied by the consumer as label props; testids are namespaced via testIdPrefix.

I acknowledge the list is already incomplete (Spinner and the DataTable* family are missing), so this is not a standard the repo has upheld rigorously — hence LOW, and the one item here I would accept as a filed follow-up issue rather than an in-session fix.

---

Non-blocking (no action in this PR)

• Seven independent required label props is a cohesive group modelled as N separate parameters. Required-ness means none can be silently omitted, so there is no live hazard, but a single labels: { loading, loadingAria, loadMore, retry, error, endOfList } object would make the group additive-safe: today, adding a seventh piece of copy is a breaking signature change at every call site. Worth revisiting when consumer EPIC-02: Application Shell & Infrastructure #2 arrives, not before.
• onPageApplied?: (meta: M | undefined, page: number) forces if (meta) at a call site that always supplies meta, so a future omission degrades silently to a stale total rather than a type error. Tightening meta to required when M is instantiated is possible but costs more type machinery than it saves at one call site.
• The two-guard invariant remains undocumented (statusRef owns the cross-render window, inFlightRef the same-tick window; a superseded fetch's finally clears a flag it may not own). Carried over unchanged from round 1 as non-blocking — still worth one comment above inFlightRef so a later simplification dropping either guard does not silently reopen AC 6.
• First-batch failure still has no retry affordance — footer gated on entries.length > 0. Filed as Diary: a first-batch load failure offers no retry affordance (footer is gated on entries.length > 0) #2064, unchanged.

No API, schema, or migration surface — no wiki page is owed by this PR, and wiki/ being untouched is correct.

[steilerDev]

[product-architect] Round 3 re-review (final) — PR #2063 / issue #2060

Both round-2 findings verified as genuinely resolved, not merely claimed. Checked at commit bd8bd1f8.

Finding 1 (round 2, MEDIUM) — hand-rolled Singular/Plural suffixes → native i18next pluralization: RESOLVED

Verified beyond the diff:

• en/diary.json and de/diary.json both carry exactly 3 base keys × _one/_other = 6 keys, en/de parity intact; i18n.parity.test.ts already covers the diary namespace, so drift is guarded.
• client/package.json pins i18next 26.3.6 and client/src/i18n/index.ts sets no compatibilityJSON override — so the v4 JSON plural format (_one/_other, Intl.PluralRules-driven) is the native resolution path, not a coincidence of naming.
• The convention is confirmed as the house style, not invented here: budget.json (10 _one keys) and dashboard.json (5) already use it.
• DiaryPage.tsx:145-160: the suffix variable is gone; all three branches call the base key with { count: lastBatchCount }. Effect deps unchanged and still correct (fetchSequence, hasMore, lastBatchCount, t).
• Most importantly, the guard is discriminating: DiaryPage.test.tsx:520 and :565 assert rendered text ('1 more entry loaded', '2 entries loaded'), not key names. If the suffix format were wrong for i18next 26, i18next would fall back and those assertions would fail. This proves runtime plural selection, not just key existence — which is exactly what my round-2 note asked for.

The round-2 concern about a call-site binary split being unable to express a >2-form CLDR locale is now structurally fixed: adding e.g. Polish or Arabic is a pure translation-file change with no code edit.

Finding 2 (round 2, LOW) — register InfiniteScrollFooter/useInfiniteScroll in CLAUDE.md: RESOLVED

The bullet's claims check out against the tree: client/src/components/InfiniteScrollFooter/ exists (component + module CSS + test), client/src/hooks/useInfiniteScroll.ts exists, and testIdPrefix?: string with a 'infinite-scroll' default is real (InfiniteScrollFooter.tsx:19,33). No aspirational documentation.

---

New findings — both NON-BLOCKING, safe to defer

LOW — stale key names in test comments. DiaryPage.test.tsx:175 and :564 still name the now-deleted keys initialLoadAnnouncementSingular / initialLoadAnnouncementPlural in explanatory comments. The assertions themselves were correctly updated; only the prose cross-reference rotted. Zero behavioural impact, but it is a comment pointing at a symbol that no longer exists anywhere in the repo. Do not respin the PR for this — fold it into the next commit that touches this file, or drop it entirely.

INFORMATIONAL — same anti-pattern still live one file over, but pre-existing. DiaryPage.tsx:249 renders totalItems === 1 ? t('page.entryCountSingular') : t('page.entryCountPlural') — the exact construction I asked to be removed from the announcement path. I confirmed against origin/beta that entryCountSingular/entryCountPlural predate this PR, so it is genuinely out of scope here and must not expand this PR's diff. Worth a small follow-up issue to converge the whole namespace on native pluralization.

Neither finding meets the bar for another round after three passes.

VERDICT: APPROVE

(Posted as a comment rather than gh pr review --approve because this session's token authored the PR.)

[github-actions]

🎉 This PR is included in version 2.15.0-beta.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀