From 2342d35834893e7b24fe8e114a8760fc4dd4ac81 Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Fri, 4 Sep 2026 13:17:57 +0200 Subject: [PATCH 1/7] feat(diary): replace numbered pager with infinite scroll Fixes #2060 Co-Authored-By: Claude dev-team-lead Co-Authored-By: Claude frontend-developer Co-Authored-By: Claude qa-integration-tester Co-Authored-By: Claude e2e-test-engineer --- .../agent-memory/e2e-test-engineer/MEMORY.md | 1 + .../e2e-test-engineer/diary-e2e.md | 7 + .../issue-2060-diary-infinite-scroll.md | 114 ++++ .claude/agent-memory/product-owner/MEMORY.md | 29 +- .../product-owner/standalone-diary-bugs.md | 3 +- .../InfiniteScrollFooter.module.css | 48 ++ .../InfiniteScrollFooter.test.tsx | 128 ++++ .../InfiniteScrollFooter.tsx | 66 ++ client/src/hooks/useInfiniteScroll.test.tsx | 409 ++++++++++++ client/src/hooks/useInfiniteScroll.ts | 145 +++++ client/src/i18n/en/diary.json | 14 +- .../src/pages/DiaryPage/DiaryPage.module.css | 24 - client/src/pages/DiaryPage/DiaryPage.test.tsx | 437 ++++++++++--- client/src/pages/DiaryPage/DiaryPage.tsx | 211 +++---- e2e/pages/DiaryPage.ts | 49 +- e2e/tests/diary/diary-list.spec.ts | 587 +++++++++++++++++- 16 files changed, 2009 insertions(+), 263 deletions(-) create mode 100644 .claude/agent-memory/e2e-test-engineer/issue-2060-diary-infinite-scroll.md create mode 100644 client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.module.css create mode 100644 client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx create mode 100644 client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.tsx create mode 100644 client/src/hooks/useInfiniteScroll.test.tsx create mode 100644 client/src/hooks/useInfiniteScroll.ts 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/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..a4553e5d1 --- /dev/null +++ b/.claude/agent-memory/e2e-test-engineer/issue-2060-diary-infinite-scroll.md @@ -0,0 +1,114 @@ +--- +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="infinite-scroll-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"). +- **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. + +## 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-owner/MEMORY.md b/.claude/agent-memory/product-owner/MEMORY.md index 03764c121..573e24701 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** | 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,25 @@ 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 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 +108,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/standalone-diary-bugs.md b/.claude/agent-memory/product-owner/standalone-diary-bugs.md index 3853f3e4e..b5bb7ffa4 100644 --- a/.claude/agent-memory/product-owner/standalone-diary-bugs.md +++ b/.claude/agent-memory/product-owner/standalone-diary-bugs.md @@ -9,13 +9,14 @@ 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. - **#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/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.module.css b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.module.css new file mode 100644 index 000000000..6ab801ce5 --- /dev/null +++ b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.module.css @@ -0,0 +1,48 @@ +.footer { + display: flex; + flex-direction: column; +} + +.sentinel { + width: 0; + height: 0; +} + +.statusRow { + display: flex; + align-items: center; + justify-content: center; + gap: var(--spacing-2); + min-height: var(--spacing-12); + color: var(--color-text-muted); + font-size: var(--font-size-sm); +} + +.endOfList { + border-top: 1px solid var(--color-border); + padding: var(--spacing-6) var(--spacing-3); + color: var(--color-text-muted); + font-size: var(--font-size-sm); + text-align: center; + min-height: var(--spacing-12); +} + +.loadMoreButton { + display: flex; + align-items: center; + justify-content: center; + gap: var(--spacing-2); + margin: 0 auto; + min-height: var(--spacing-12); +} + +@media (max-width: 767px) { + .loadMoreButton { + width: 100%; + min-height: 44px; + } + + .endOfList { + padding: var(--spacing-4) var(--spacing-3); + } +} diff --git a/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx new file mode 100644 index 000000000..cdc4c5aca --- /dev/null +++ b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx @@ -0,0 +1,128 @@ +/** + * @jest-environment jsdom + * + * Unit tests for InfiniteScrollFooter (Issue #2060 — Diary infinite-scroll rework). + * + * Real i18n is initialized (via import of app i18n setup) so useTranslation() resolves + * actual copy from client/src/i18n/en/diary.json, matching the established pattern in + * PhotoUpload.test.tsx. + */ +import { jest, describe, it, expect } from '@jest/globals'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import '../../i18n/index.js'; +import { InfiniteScrollFooter } from './InfiniteScrollFooter.js'; +import styles from './InfiniteScrollFooter.module.css'; +import type { InfiniteScrollFooterProps } from './InfiniteScrollFooter.js'; + +function renderFooter(overrides: Partial = {}) { + const onLoadMore = jest.fn(); + const onRetry = jest.fn(); + const sentinelRef = jest.fn(); + const props: InfiniteScrollFooterProps = { + status: 'idle', + hasMore: true, + sentinelRef, + onLoadMore, + onRetry, + ...overrides, + }; + const view = render(); + return { ...view, onLoadMore, onRetry, sentinelRef }; +} + +describe('InfiniteScrollFooter', () => { + // ─── idle ──────────────────────────────────────────────────────────────────── + + it('idle: renders the sentinel (aria-hidden), and an enabled button labeled "Load more"', () => { + renderFooter({ status: 'idle' }); + + const sentinel = screen.getByTestId('infinite-scroll-sentinel'); + expect(sentinel).toBeInTheDocument(); + expect(sentinel).toHaveAttribute('aria-hidden', 'true'); + + const button = screen.getByTestId('diary-load-more-button'); + expect(button).toBeEnabled(); + expect(button).toHaveTextContent('Load more'); + }); + + it('idle: clicking the button calls onLoadMore, not onRetry', async () => { + const user = userEvent.setup(); + const { onLoadMore, onRetry } = renderFooter({ status: 'idle' }); + + await user.click(screen.getByTestId('diary-load-more-button')); + + expect(onLoadMore).toHaveBeenCalledTimes(1); + expect(onRetry).not.toHaveBeenCalled(); + }); + + // ─── loading ───────────────────────────────────────────────────────────────── + + it('loading: the button is disabled and shows a spinner + "Loading more entries…" inside it', () => { + renderFooter({ status: 'loading' }); + + const button = screen.getByTestId('diary-load-more-button'); + expect(button).toBeDisabled(); + expect(button).toHaveTextContent('Loading more entries…'); + }); + + it('loading: a separate status row also renders a spinner + "Loading more entries…"', () => { + renderFooter({ status: 'loading' }); + + // Two occurrences of the loading copy: one inside the button, one in the status row. + expect(screen.getAllByText('Loading more entries…')).toHaveLength(2); + // The status-row spinner carries the dedicated aria-label; the button's spinner + // uses the Spinner default label ("Loading"). + expect(screen.getByRole('img', { name: 'Loading more diary entries' })).toBeInTheDocument(); + }); + + // ─── error ─────────────────────────────────────────────────────────────────── + + it('error: renders a FormError banner with role="alert" and the exact error copy', () => { + renderFooter({ status: 'error' }); + + const banner = screen.getByRole('alert'); + expect(banner).toHaveTextContent('Failed to load more entries.'); + }); + + it('error: the button is enabled and labeled "Retry"', () => { + renderFooter({ status: 'error' }); + + const button = screen.getByTestId('diary-load-more-button'); + expect(button).toBeEnabled(); + expect(button).toHaveTextContent('Retry'); + }); + + it('error: clicking the button calls onRetry, not onLoadMore', async () => { + const user = userEvent.setup(); + const { onLoadMore, onRetry } = renderFooter({ status: 'error' }); + + await user.click(screen.getByTestId('diary-load-more-button')); + + expect(onRetry).toHaveBeenCalledTimes(1); + expect(onLoadMore).not.toHaveBeenCalled(); + }); + + // ─── done ──────────────────────────────────────────────────────────────────── + + it('done: the button is absent', () => { + renderFooter({ status: 'done', hasMore: false }); + + expect(screen.queryByTestId('diary-load-more-button')).not.toBeInTheDocument(); + }); + + it('done: renders the end-of-list row with the exact copy and testid', () => { + renderFooter({ status: 'done', hasMore: false }); + + const endOfList = screen.getByTestId('diary-end-of-list'); + expect(endOfList).toHaveTextContent("You've reached the end — no more entries to load."); + }); + + // ─── sentinel styling ──────────────────────────────────────────────────────── + + it('the sentinel div has zero width/height via its CSS module class', () => { + renderFooter({ status: 'idle' }); + + expect(screen.getByTestId('infinite-scroll-sentinel')).toHaveClass(styles.sentinel); + }); +}); diff --git a/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.tsx b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.tsx new file mode 100644 index 000000000..9c5c6fc27 --- /dev/null +++ b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.tsx @@ -0,0 +1,66 @@ +import { useTranslation } from 'react-i18next'; +import { Spinner } from '../Spinner/Spinner.js'; +import { FormError } from '../FormError/FormError.js'; +import shared from '../../styles/shared.module.css'; +import styles from './InfiniteScrollFooter.module.css'; + +export interface InfiniteScrollFooterProps { + status: 'idle' | 'loading' | 'error' | 'done'; + hasMore: boolean; + sentinelRef: (node: HTMLDivElement | null) => void; + onLoadMore: () => void; + onRetry: () => void; +} + +export function InfiniteScrollFooter({ + status, + sentinelRef, + onLoadMore, + onRetry, +}: InfiniteScrollFooterProps) { + const { t } = useTranslation(); + + return ( +
+ + ); +} + +export default InfiniteScrollFooter; diff --git a/client/src/hooks/useInfiniteScroll.test.tsx b/client/src/hooks/useInfiniteScroll.test.tsx new file mode 100644 index 000000000..da5d609fa --- /dev/null +++ b/client/src/hooks/useInfiniteScroll.test.tsx @@ -0,0 +1,409 @@ +/** + * @jest-environment jsdom + * + * Unit tests for useInfiniteScroll (Issue #2060 — Diary infinite-scroll rework). + * + * Covers the state machine (idle/loading/error/done), page-counter bookkeeping, + * dedupe of concurrent loadMore() calls, retry-same-page semantics, IntersectionObserver + * wiring/teardown, and the resetKey-driven reset-and-refetch behavior. + * + * jsdom does not implement IntersectionObserver. Following the established local-mock + * pattern used in PhotoAnnotator.test.tsx for ResizeObserver, a small MockIntersectionObserver + * class is defined here and installed/restored per test rather than touching the shared + * setupTests.ts. + */ +import { jest, describe, it, expect, beforeEach, afterEach } from '@jest/globals'; +import { act, render, renderHook, waitFor } from '@testing-library/react'; +import type { InfiniteScrollPage } from './useInfiniteScroll.js'; +import { useInfiniteScroll } from './useInfiniteScroll.js'; + +// ─── IntersectionObserver mock ────────────────────────────────────────────── + +class MockIntersectionObserver { + static instances: MockIntersectionObserver[] = []; + callback: IntersectionObserverCallback; + constructor(cb: IntersectionObserverCallback) { + this.callback = cb; + MockIntersectionObserver.instances.push(this); + } + observe = jest.fn(); + disconnect = jest.fn(); + unobserve = jest.fn(); + trigger(isIntersecting: boolean) { + this.callback( + [{ isIntersecting } as IntersectionObserverEntry], + this as unknown as IntersectionObserver, + ); + } +} + +let originalIntersectionObserver: typeof IntersectionObserver | undefined; + +beforeEach(() => { + MockIntersectionObserver.instances = []; + originalIntersectionObserver = globalThis.IntersectionObserver; + (globalThis as unknown as { IntersectionObserver: unknown }).IntersectionObserver = + MockIntersectionObserver; +}); + +afterEach(() => { + (globalThis as unknown as { IntersectionObserver: unknown }).IntersectionObserver = + originalIntersectionObserver; +}); + +// ─── Helpers ───────────────────────────────────────────────────────────────── + +function page(items: T[], hasMore: boolean): InfiniteScrollPage { + return { items, hasMore }; +} + +/** Mounts the hook attached to a real sentinel
, so the IntersectionObserver + * effect (which only attaches once `sentinelNode` is non-null) actually runs. */ +function renderWithSentinel(options: { + fetchPage: (page: number) => Promise>; + resetKey: string; +}) { + const results: ReturnType>[] = []; + function Harness(props: { resetKey: string }) { + const hook = useInfiniteScroll({ fetchPage: options.fetchPage, resetKey: props.resetKey }); + results.push(hook); + return
; + } + const view = render(); + return { + ...view, + latest: () => results[results.length - 1]!, + }; +} + +describe('useInfiniteScroll', () => { + // ─── Mount / initial load ─────────────────────────────────────────────────── + + it('calls fetchPage(1) exactly once on mount and transitions idle -> loading -> idle when hasMore is true', async () => { + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['a', 'b'], true)); + + const { result } = renderHook(() => useInfiniteScroll({ fetchPage, resetKey: 'k' })); + + expect(result.current.status).toBe('loading'); + + await waitFor(() => expect(result.current.status).toBe('idle')); + + expect(fetchPage).toHaveBeenCalledTimes(1); + expect(fetchPage).toHaveBeenCalledWith(1); + expect(result.current.items).toEqual(['a', 'b']); + expect(result.current.hasMore).toBe(true); + }); + + it('transitions to "done" on mount when the first page reports hasMore=false', async () => { + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['only'], false)); + + const { result } = renderHook(() => useInfiniteScroll({ fetchPage, resetKey: 'k' })); + + await waitFor(() => expect(result.current.status).toBe('done')); + expect(result.current.hasMore).toBe(false); + expect(result.current.items).toEqual(['only']); + }); + + // ─── loadMore ──────────────────────────────────────────────────────────────── + + it('loadMore() while idle fetches the next page and appends items, preserving order with no duplication', async () => { + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['a', 'b'], true)); + fetchPage.mockResolvedValueOnce(page(['c', 'd'], true)); + + const { result } = renderHook(() => useInfiniteScroll({ fetchPage, resetKey: 'k' })); + await waitFor(() => expect(result.current.status).toBe('idle')); + + act(() => result.current.loadMore()); + + await waitFor(() => expect(fetchPage).toHaveBeenCalledTimes(2)); + expect(fetchPage).toHaveBeenNthCalledWith(2, 2); + + await waitFor(() => expect(result.current.status).toBe('idle')); + expect(result.current.items).toEqual(['a', 'b', 'c', 'd']); + }); + + // ─── Dedupe ────────────────────────────────────────────────────────────────── + + it('dedupes two loadMore() calls fired back-to-back while the first is still in flight', async () => { + let resolveSecondFetch: ((v: InfiniteScrollPage) => void) | undefined; + const deferred = new Promise>((resolve) => { + resolveSecondFetch = resolve; + }); + + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['a'], true)); // initial mount, page 1 + fetchPage.mockImplementationOnce(() => deferred); // page 2, held open + + const { result } = renderHook(() => useInfiniteScroll({ fetchPage, resetKey: 'k' })); + await waitFor(() => expect(result.current.status).toBe('idle')); + + act(() => { + result.current.loadMore(); + result.current.loadMore(); + }); + + // Only one call for page 2 despite two loadMore() invocations in the same tick. + expect(fetchPage).toHaveBeenCalledTimes(2); + expect(fetchPage).toHaveBeenNthCalledWith(2, 2); + + await act(async () => { + resolveSecondFetch?.(page(['b'], true)); + await deferred; + }); + + await waitFor(() => expect(result.current.status).toBe('idle')); + // Still exactly one call for page 2 — no follow-up call was queued by the dupe. + expect(fetchPage).toHaveBeenCalledTimes(2); + expect(result.current.items).toEqual(['a', 'b']); + }); + + // ─── IntersectionObserver wiring ───────────────────────────────────────────── + + it('firing the intersection observer (isIntersecting=true) while idle triggers the same loadMore path as the button', async () => { + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['a'], true)); + fetchPage.mockResolvedValueOnce(page(['b'], true)); + + const view = renderWithSentinel({ fetchPage, resetKey: 'k' }); + await waitFor(() => expect(view.latest().status).toBe('idle')); + + expect(MockIntersectionObserver.instances).toHaveLength(1); + act(() => { + MockIntersectionObserver.instances[0]!.trigger(true); + }); + + await waitFor(() => expect(fetchPage).toHaveBeenCalledTimes(2)); + expect(fetchPage).toHaveBeenNthCalledWith(2, 2); + await waitFor(() => expect(view.latest().items).toEqual(['a', 'b'])); + }); + + it('firing the intersection observer while status is "error" is a no-op', async () => { + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['a'], true)); + fetchPage.mockRejectedValueOnce(new Error('boom')); + + const view = renderWithSentinel({ fetchPage, resetKey: 'k' }); + await waitFor(() => expect(view.latest().status).toBe('idle')); + + act(() => view.latest().loadMore()); + await waitFor(() => expect(view.latest().status).toBe('error')); + expect(fetchPage).toHaveBeenCalledTimes(2); + + act(() => { + MockIntersectionObserver.instances[0]!.trigger(true); + }); + + // No additional fetchPage call — the observer callback no-ops on error. + expect(fetchPage).toHaveBeenCalledTimes(2); + expect(view.latest().status).toBe('error'); + }); + + it('disconnects the IntersectionObserver on unmount', async () => { + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['a'], true)); + + const view = renderWithSentinel({ fetchPage, resetKey: 'k' }); + await waitFor(() => expect(view.latest().status).toBe('idle')); + + expect(MockIntersectionObserver.instances).toHaveLength(1); + const instance = MockIntersectionObserver.instances[0]!; + expect(instance.disconnect).not.toHaveBeenCalled(); + + view.unmount(); + + expect(instance.disconnect).toHaveBeenCalledTimes(1); + }); + + // ─── retry() ───────────────────────────────────────────────────────────────── + + it('retry() re-issues the SAME page that failed, and does not advance the page counter on failure', async () => { + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['a'], true)); // mount: page 1 succeeds + fetchPage.mockRejectedValueOnce(new Error('network down')); // loadMore: page 2 fails + fetchPage.mockResolvedValueOnce(page(['b'], true)); // retry: page 2 succeeds + + const { result } = renderHook(() => useInfiniteScroll({ fetchPage, resetKey: 'k' })); + await waitFor(() => expect(result.current.status).toBe('idle')); + + act(() => result.current.loadMore()); + await waitFor(() => expect(result.current.status).toBe('error')); + expect(fetchPage).toHaveBeenNthCalledWith(2, 2); + // The failed fetch must not have mutated items. + expect(result.current.items).toEqual(['a']); + + act(() => result.current.retry()); + await waitFor(() => expect(result.current.status).toBe('idle')); + + // Retry used the SAME page number (2) as the failed attempt — not 3. + expect(fetchPage).toHaveBeenNthCalledWith(3, 2); + // No gap or duplicate: exactly the first batch followed by the retried batch. + expect(result.current.items).toEqual(['a', 'b']); + }); + + it('retry() is a no-op unless status is "error"', async () => { + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['a'], true)); + + const { result } = renderHook(() => useInfiniteScroll({ fetchPage, resetKey: 'k' })); + await waitFor(() => expect(result.current.status).toBe('idle')); + + act(() => result.current.retry()); + + // Still just the one mount call — retry() did nothing while idle. + expect(fetchPage).toHaveBeenCalledTimes(1); + expect(result.current.status).toBe('idle'); + }); + + // ─── resetKey ──────────────────────────────────────────────────────────────── + + it('changing resetKey clears items, resets to page 1, and issues a fresh fetchPage(1) call', async () => { + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['a'], true)); + fetchPage.mockResolvedValueOnce(page(['x', 'y'], false)); + + const { result, rerender } = renderHook( + ({ resetKey }: { resetKey: string }) => useInfiniteScroll({ fetchPage, resetKey }), + { initialProps: { resetKey: 'k1' } }, + ); + await waitFor(() => expect(result.current.status).toBe('idle')); + expect(result.current.items).toEqual(['a']); + + rerender({ resetKey: 'k2' }); + + await waitFor(() => expect(result.current.status).toBe('done')); + expect(fetchPage).toHaveBeenNthCalledWith(2, 1); + expect(result.current.items).toEqual(['x', 'y']); + }); + + // ─── FINDING: mid-flight reset race ────────────────────────────────────────── + // If resetKey changes while a loadMore() fetch (page >= 2) from the PREVIOUS + // resetKey is still in flight, runFetch(1) issued by the reset effect is + // swallowed by the `inFlightRef` guard (it is still `true` from the pending + // call), so no fresh page-1 request is ever made. When the stale in-flight + // promise later resolves, its handler still closes over the OLD page number + // and unconditionally does `setItems((prev) => [...prev, ...result.items])` + // (since that closed-over page !== 1), appending the stale batch onto the + // freshly-cleared (now empty) `items` array — and `pageRef.current` is set to + // that stale page + 1, corrupting the counter for the new resetKey's session. + // + // This test documents the CORRECT expected behavior per the hook's contract + // (a resetKey change must always win and reflect only the new key's page-1 + // data). Originally filed as BUG-2060-1 (github.com/steilerDev/cornerstone + // issues/2061) and kept as `it.failing` while that bug was open — confirmed + // failing against the pre-fix implementation (fetchPage was never called a + // 3rd time; the reset's runFetch(1) call was swallowed by the inFlightRef + // guard). frontend-developer's fix adds an `epochRef` generation counter: + // bumped on every resetKey change, checked by each fetch's completion + // handler before it applies any state, and the reset effect now forces + // `inFlightRef.current = false` so its own runFetch(1) is never swallowed by + // a still-in-flight stale fetch. Converted back to a normal `it` — verified + // passing against the fixed hook (see qa-integration-tester's handback + // report for the run output). + it('a resetKey change while a stale page>=2 fetch is in flight must not let the stale response leak into the new result set', async () => { + let resolveStalePage2: ((v: InfiniteScrollPage) => void) | undefined; + const staleDeferred = new Promise>((resolve) => { + resolveStalePage2 = resolve; + }); + + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['old-a'], true)); // mount under resetKey 'k1', page 1 + fetchPage.mockImplementationOnce(() => staleDeferred); // loadMore under 'k1', page 2 (held open) + fetchPage.mockResolvedValueOnce(page(['new-a'], false)); // expected fresh page 1 under 'k2' + + const { result, rerender } = renderHook( + ({ resetKey }: { resetKey: string }) => useInfiniteScroll({ fetchPage, resetKey }), + { initialProps: { resetKey: 'k1' } }, + ); + await waitFor(() => expect(result.current.status).toBe('idle')); + + act(() => result.current.loadMore()); // page 2 under k1 now in flight, never resolves yet + await waitFor(() => expect(fetchPage).toHaveBeenCalledTimes(2)); + + // Change filters before the in-flight page-2 fetch resolves. + rerender({ resetKey: 'k2' }); + + // The reset issues a fresh fetchPage(1) call for the new key. + await waitFor(() => expect(fetchPage).toHaveBeenCalledTimes(3)); + await waitFor(() => expect(result.current.items).toEqual(['new-a'])); + // The new key's own page-1 result (hasMore: false) must be reflected — + // proves this isn't just an items-array coincidence. + expect(result.current.status).toBe('done'); + expect(result.current.hasMore).toBe(false); + + // Now let the stale k1/page-2 response resolve. It must NOT be appended to + // the new result set, and must not clobber hasMore/status derived from the + // new key's own (already-applied) result. + await act(async () => { + resolveStalePage2?.(page(['stale-b'], true)); + await staleDeferred; + }); + + expect(result.current.items).toEqual(['new-a']); + expect(result.current.status).toBe('done'); + expect(result.current.hasMore).toBe(false); + }); + + // ─── fetchSequence / lastBatchCount ────────────────────────────────────────── + + it('increments fetchSequence once per successful fetch and reports lastBatchCount for the most recent batch', async () => { + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['a', 'b'], true)); + fetchPage.mockResolvedValueOnce(page(['c'], false)); + + const { result } = renderHook(() => useInfiniteScroll({ fetchPage, resetKey: 'k' })); + + await waitFor(() => expect(result.current.fetchSequence).toBe(1)); + expect(result.current.lastBatchCount).toBe(2); + + act(() => result.current.loadMore()); + + await waitFor(() => expect(result.current.fetchSequence).toBe(2)); + expect(result.current.lastBatchCount).toBe(1); + }); + + it('does not increment fetchSequence on a failed fetch', async () => { + const fetchPage = jest.fn<(p: number) => Promise>>(); + fetchPage.mockResolvedValueOnce(page(['a'], true)); + fetchPage.mockRejectedValueOnce(new Error('boom')); + + const { result } = renderHook(() => useInfiniteScroll({ fetchPage, resetKey: 'k' })); + await waitFor(() => expect(result.current.fetchSequence).toBe(1)); + + act(() => result.current.loadMore()); + await waitFor(() => expect(result.current.status).toBe('error')); + + expect(result.current.fetchSequence).toBe(1); + }); + + // ─── fetchPage identity stability ──────────────────────────────────────────── + + it('a new fetchPage function identity on re-render (same resetKey) does not trigger a new fetch', async () => { + const fetchPageA = jest.fn<(p: number) => Promise>>(); + fetchPageA.mockResolvedValue(page(['a'], true)); + + const { result, rerender } = renderHook( + ({ fetchPage }: { fetchPage: (p: number) => Promise> }) => + useInfiniteScroll({ fetchPage, resetKey: 'k' }), + { initialProps: { fetchPage: fetchPageA } }, + ); + await waitFor(() => expect(result.current.status).toBe('idle')); + expect(fetchPageA).toHaveBeenCalledTimes(1); + + const fetchPageB = jest.fn<(p: number) => Promise>>(); + fetchPageB.mockResolvedValue(page(['z'], true)); + + rerender({ fetchPage: fetchPageB }); + + // Give any (incorrect) effect a tick to fire before asserting it didn't. + await act(async () => { + await Promise.resolve(); + }); + + expect(fetchPageB).not.toHaveBeenCalled(); + expect(fetchPageA).toHaveBeenCalledTimes(1); + expect(result.current.status).toBe('idle'); + }); +}); diff --git a/client/src/hooks/useInfiniteScroll.ts b/client/src/hooks/useInfiniteScroll.ts new file mode 100644 index 000000000..55f4afd6d --- /dev/null +++ b/client/src/hooks/useInfiniteScroll.ts @@ -0,0 +1,145 @@ +import { useCallback, useEffect, useRef, useState } from 'react'; + +/** Distance (in px) below the viewport bottom at which the next batch starts loading. */ +const INFINITE_SCROLL_LOOKAHEAD_PX = 600; + +export type InfiniteScrollStatus = 'idle' | 'loading' | 'error' | 'done'; + +export interface InfiniteScrollPage { + items: T[]; + /** true if more pages remain after this one */ + hasMore: boolean; +} + +export interface UseInfiniteScrollOptions { + /** + * Fetches one batch for the given 1-based page number. Must reject (throw) + * on failure. Read via a ref internally so a new function identity each + * render does NOT retrigger a fetch — only `resetKey` changes do. + */ + fetchPage: (page: number) => Promise>; + /** + * Changing this value resets to a fresh first batch: clears items, resets + * the page counter to 1, and re-fetches. Derive it from the active + * filters/search (e.g. a `|`-joined string of the filter primitives). + */ + resetKey: string; +} + +export interface UseInfiniteScrollResult { + items: T[]; + status: InfiniteScrollStatus; + hasMore: boolean; + /** Count of items in the most recently successful fetch (for a11y announcements). */ + lastBatchCount: number; + /** Increments once per successful fetch (including the first). Distinguishes "first batch" (sequence === 1) from "appended batch" (sequence > 1). */ + fetchSequence: number; + /** Ref-callback — attach to the sentinel `
`. */ + sentinelRef: (node: HTMLDivElement | null) => void; + /** Called by both the observer and the "Load more" button. No-ops while loading/error/done. */ + loadMore: () => void; + /** Re-issues the SAME page that just failed. No-ops unless status === 'error'. */ + retry: () => void; +} + +export function useInfiniteScroll({ + fetchPage, + resetKey, +}: UseInfiniteScrollOptions): UseInfiniteScrollResult { + const [items, setItems] = useState([]); + const [status, setStatus] = useState('loading'); + const [hasMore, setHasMore] = useState(true); + const [lastBatchCount, setLastBatchCount] = useState(0); + const [fetchSequence, setFetchSequence] = useState(0); + const [sentinelNode, setSentinelNode] = useState(null); + + const fetchPageRef = useRef(fetchPage); + fetchPageRef.current = fetchPage; + + const pageRef = useRef(1); + const inFlightRef = useRef(false); + const statusRef = useRef('loading'); + statusRef.current = status; + // Bumped on every resetKey change. Lets a fetch's completion handler detect it was + // started under a since-superseded resetKey generation and discard its result instead + // of corrupting the freshly-reset state (see #2061). + const epochRef = useRef(0); + + const runFetch = useCallback(async (page: number, epoch: number) => { + if (inFlightRef.current) return; + inFlightRef.current = true; + try { + const result = await fetchPageRef.current(page); + if (epoch !== epochRef.current) return; // superseded by a resetKey change — discard + setItems((prev) => (page === 1 ? result.items : [...prev, ...result.items])); + setHasMore(result.hasMore); + setStatus(result.hasMore ? 'idle' : 'done'); + pageRef.current = page + 1; + setLastBatchCount(result.items.length); + setFetchSequence((prev) => prev + 1); + } catch { + if (epoch === epochRef.current) { + setStatus('error'); + } + } finally { + inFlightRef.current = false; + } + }, []); + + const loadMore = useCallback(() => { + if (statusRef.current !== 'idle') return; + setStatus('loading'); + void runFetch(pageRef.current, epochRef.current); + }, [runFetch]); + + const retry = useCallback(() => { + if (statusRef.current !== 'error') return; + setStatus('loading'); + void runFetch(pageRef.current, epochRef.current); + }, [runFetch]); + + /* eslint-disable @eslint-react/set-state-in-effect -- resetKey change (filters/search) must + synchronously clear the accumulated list and page counter before the first batch of the + new result set is fetched; runFetch/fetchPage are read via refs so they are intentionally + excluded from the dependency array. */ + useEffect(() => { + epochRef.current += 1; + const epoch = epochRef.current; + pageRef.current = 1; + // Bypass the dedupe guard: a fetch still in flight under the previous resetKey must not + // swallow this reset's own fetchPage(1) call (see #2061). + inFlightRef.current = false; + setItems([]); + setHasMore(true); + setStatus('loading'); + void runFetch(1, epoch); + }, [resetKey, runFetch]); + /* eslint-enable @eslint-react/set-state-in-effect */ + + const sentinelRef = useCallback((node: HTMLDivElement | null) => { + setSentinelNode(node); + }, []); + + useEffect(() => { + if (!sentinelNode) return; + const observer = new IntersectionObserver( + (entries) => { + if (entries[0]?.isIntersecting) loadMore(); + }, + { rootMargin: `0px 0px ${INFINITE_SCROLL_LOOKAHEAD_PX}px 0px` }, + ); + observer.observe(sentinelNode); + return () => observer.disconnect(); + }, [sentinelNode, loadMore]); + + return { + items, + status, + hasMore, + lastBatchCount, + fetchSequence, + sentinelRef, + loadMore, + retry, + }; +} diff --git a/client/src/i18n/en/diary.json b/client/src/i18n/en/diary.json index 8b96b553d..de7a4762b 100644 --- a/client/src/i18n/en/diary.json +++ b/client/src/i18n/en/diary.json @@ -12,10 +12,16 @@ }, "loading": "Loading entries...", "error": "Failed to load diary entries. Please try again.", - "pagination": { - "previous": "Previous", - "next": "Next", - "pageInfo": "Page {{currentPage}} of {{totalPages}}" + "infiniteScroll": { + "loadingMore": "Loading more entries…", + "loadingMoreAriaLabel": "Loading more diary entries", + "loadMoreButton": "Load more", + "retryButton": "Retry", + "errorMessage": "Failed to load more entries.", + "endOfList": "You've reached the end — no more entries to load.", + "initialLoadAnnouncement": "{{count}} entries loaded", + "batchAppendedAnnouncement": "{{count}} more entries loaded", + "batchAppendedAndEndAnnouncement": "{{count}} more entries loaded. You've reached the end." }, "filterBar": { "search": "Search entries...", diff --git a/client/src/pages/DiaryPage/DiaryPage.module.css b/client/src/pages/DiaryPage/DiaryPage.module.css index ab64fbd33..2d2a73287 100644 --- a/client/src/pages/DiaryPage/DiaryPage.module.css +++ b/client/src/pages/DiaryPage/DiaryPage.module.css @@ -69,21 +69,6 @@ overflow: hidden; } -.pagination { - display: flex; - align-items: center; - justify-content: center; - gap: var(--spacing-4); - margin-top: var(--spacing-6); -} - -.pageInfo { - font-size: var(--font-size-sm); - color: var(--color-text-secondary); - min-width: 150px; - text-align: center; -} - /* Responsive */ @media (max-width: 767px) { .page { @@ -120,15 +105,6 @@ width: 100%; min-height: 44px; } - - .pagination { - flex-direction: column; - gap: var(--spacing-2); - } - - .pageInfo { - min-width: auto; - } } @media (prefers-reduced-motion: reduce) { diff --git a/client/src/pages/DiaryPage/DiaryPage.test.tsx b/client/src/pages/DiaryPage/DiaryPage.test.tsx index 7e19e7c48..10dca40bb 100644 --- a/client/src/pages/DiaryPage/DiaryPage.test.tsx +++ b/client/src/pages/DiaryPage/DiaryPage.test.tsx @@ -2,13 +2,34 @@ * @jest-environment jsdom */ import { jest, describe, it, expect, beforeEach, afterEach } from '@jest/globals'; -import { screen, waitFor, render } from '@testing-library/react'; +import { act, fireEvent, screen, waitFor, render } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import { MemoryRouter } from 'react-router-dom'; +import { MemoryRouter, useLocation } from 'react-router-dom'; import type * as DiaryApiTypes from '../../lib/diaryApi.js'; import type { DiaryEntryListResponse, DiaryEntrySummary } from '@cornerstone/shared'; import type React from 'react'; +/** Renders the current URL's search string into the DOM so tests can assert on + * which query params are present/absent without reaching into router internals. */ +function LocationDisplay() { + const location = useLocation(); + return
{location.search}
; +} + +// ── IntersectionObserver stub ──────────────────────────────────────────────── +// jsdom does not implement IntersectionObserver, and useInfiniteScroll (used by +// DiaryPage) attaches one to its sentinel unconditionally on mount. None of these +// page-level tests drive the observer directly (that's covered in +// useInfiniteScroll.test.tsx) — this is just a no-op stub so mounting doesn't throw. +class NoopIntersectionObserver { + observe = jest.fn(); + disconnect = jest.fn(); + unobserve = jest.fn(); + constructor(_cb: IntersectionObserverCallback) {} +} + +let originalIntersectionObserver: typeof IntersectionObserver | undefined; + // ── API mock ────────────────────────────────────────────────────────────────── const mockListDiaryEntries = jest.fn(); @@ -106,15 +127,21 @@ describe('DiaryPage', () => { DiaryPage = mod.default; } mockListDiaryEntries.mockReset(); + originalIntersectionObserver = globalThis.IntersectionObserver; + (globalThis as unknown as { IntersectionObserver: unknown }).IntersectionObserver = + NoopIntersectionObserver; }); afterEach(() => { localStorage.clear(); + (globalThis as unknown as { IntersectionObserver: unknown }).IntersectionObserver = + originalIntersectionObserver; }); const renderPage = (initialEntries = ['/diary']) => render( + , ); @@ -133,9 +160,12 @@ describe('DiaryPage', () => { mockListDiaryEntries.mockResolvedValueOnce( makeListResponse([makeSummary('1'), makeSummary('2')]), ); - renderPage(); + // Scoped to the .subtitle element specifically: the infinite-scroll live region + // also announces "Loaded 2 entries" on the same page, which would otherwise + // collide with a bare /2 entries/i text query. + const { container } = renderPage(); await waitFor(() => { - expect(screen.getByText(/2 entries/i)).toBeInTheDocument(); + expect(container.querySelector('.subtitle')).toHaveTextContent(/2\s*entries/i); }); }); @@ -245,94 +275,351 @@ describe('DiaryPage', () => { }); }); - // ─── Pagination ────────────────────────────────────────────────────────────── + // ─── Infinite scroll (Issue #2060) ─────────────────────────────────────────── - it('shows pagination controls when there are multiple pages', async () => { - mockListDiaryEntries.mockResolvedValueOnce({ - items: [makeSummary('de-1')], - pagination: { page: 1, pageSize: 25, totalPages: 3, totalItems: 60 }, + describe('infinite scroll', () => { + it('renders the first batch on mount, and the load-more button is present when hasMore', async () => { + mockListDiaryEntries.mockResolvedValueOnce({ + items: [makeSummary('p1-1'), makeSummary('p1-2')], + pagination: { page: 1, pageSize: 25, totalPages: 3, totalItems: 60 }, + }); + renderPage(); + + await waitFor(() => { + expect(screen.getByTestId('diary-card-p1-1')).toBeInTheDocument(); + }); + expect(screen.getByTestId('diary-card-p1-2')).toBeInTheDocument(); + expect(screen.getByTestId('diary-load-more-button')).toBeInTheDocument(); }); - renderPage(); - await waitFor(() => { - expect(screen.getByTestId('next-page-button')).toBeInTheDocument(); - expect(screen.getByTestId('prev-page-button')).toBeInTheDocument(); + + it('does not render the load-more button and shows the end-of-list row when there is only one page', async () => { + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('de-1')], 1)); + renderPage(); + + await waitFor(() => { + expect(screen.getByTestId('diary-card-de-1')).toBeInTheDocument(); + }); + expect(screen.queryByTestId('diary-load-more-button')).not.toBeInTheDocument(); + expect(screen.getByTestId('diary-end-of-list')).toBeInTheDocument(); }); - }); - it('does not show pagination when there is only one page', async () => { - mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('de-1')])); - renderPage(); - await waitFor(() => { - expect(screen.queryByTestId('next-page-button')).not.toBeInTheDocument(); + it('changing the search query re-fetches from page 1 and discards previously-appended entries; the URL keeps q but never gains a page param', async () => { + const user = userEvent.setup(); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('old-1')])); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('new-1')])); + + renderPage(); + await waitFor(() => { + expect(screen.getByTestId('diary-card-old-1')).toBeInTheDocument(); + }); + + await user.type(screen.getByTestId('diary-search-input'), 'foo'); + + await waitFor( + () => { + expect(screen.getByTestId('diary-card-new-1')).toBeInTheDocument(); + }, + { timeout: 2000 }, + ); + expect(screen.queryByTestId('diary-card-old-1')).not.toBeInTheDocument(); + + const lastCall = mockListDiaryEntries.mock.calls[mockListDiaryEntries.mock.calls.length - 1]; + expect(lastCall?.[0]?.q).toBe('foo'); + expect(lastCall?.[0]?.page).toBe(1); + + const search = screen.getByTestId('location-search').textContent ?? ''; + expect(search).toContain('q=foo'); + expect(search).not.toContain('page='); }); - }); - it('disables the Previous button on the first page', async () => { - mockListDiaryEntries.mockResolvedValueOnce({ - items: [makeSummary('de-1')], - pagination: { page: 1, pageSize: 25, totalPages: 3, totalItems: 60 }, + // Regression test for a finding surfaced alongside BUG-2060-1/2060-2: the + // debounced-search-sync effect only wrote/deleted the `q` param and did not + // delete a stale `page` param already present in the URL (e.g. from a + // pre-rework bookmark/shared link), unlike every other filter-change + // handler in this file, which all do `newParams.delete('page')`. Fixed by + // adding the same `newParams.delete('page')` to that effect. + it('typing a search query when the URL already has a stale page param removes it', async () => { + const user = userEvent.setup(); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('old-1')])); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('new-1')])); + + renderPage(['/diary?page=3']); + await waitFor(() => { + expect(screen.getByTestId('diary-card-old-1')).toBeInTheDocument(); + }); + + await user.type(screen.getByTestId('diary-search-input'), 'foo'); + + await waitFor( + () => { + expect(screen.getByTestId('diary-card-new-1')).toBeInTheDocument(); + }, + { timeout: 2000 }, + ); + + const search = screen.getByTestId('location-search').textContent ?? ''; + expect(search).toContain('q=foo'); + expect(search).not.toContain('page='); }); - renderPage(); - await waitFor(() => { - expect(screen.getByTestId('prev-page-button')).toBeDisabled(); + + it('clicking the load-more button in idle state fetches page 2 and appends its items below the first batch', async () => { + const user = userEvent.setup(); + mockListDiaryEntries.mockResolvedValueOnce({ + items: [makeSummary('p1-1')], + pagination: { page: 1, pageSize: 25, totalPages: 2, totalItems: 30 }, + }); + mockListDiaryEntries.mockResolvedValueOnce({ + items: [makeSummary('p2-1')], + pagination: { page: 2, pageSize: 25, totalPages: 2, totalItems: 30 }, + }); + + renderPage(); + await waitFor(() => { + expect(screen.getByTestId('diary-card-p1-1')).toBeInTheDocument(); + }); + + await user.click(screen.getByTestId('diary-load-more-button')); + + await waitFor(() => { + expect(screen.getByTestId('diary-card-p2-1')).toBeInTheDocument(); + }); + expect(screen.getByTestId('diary-card-p1-1')).toBeInTheDocument(); + + expect(mockListDiaryEntries).toHaveBeenCalledTimes(2); + expect(mockListDiaryEntries.mock.calls[1]?.[0]?.page).toBe(2); }); - }); - it('disables the Next button on the last page', async () => { - mockListDiaryEntries.mockResolvedValueOnce({ - items: [makeSummary('de-1')], - pagination: { page: 3, pageSize: 25, totalPages: 3, totalItems: 60 }, + it('shows the full-page error banner when listDiaryEntries rejects on the first call (no entries yet)', async () => { + mockListDiaryEntries.mockRejectedValueOnce(new Error('network down')); + renderPage(); + + await waitFor(() => { + expect(screen.getByText(/failed to load diary entries/i)).toBeInTheDocument(); + }); + expect(screen.getByText(/failed to load diary entries/i)).toHaveClass('bannerError'); }); - // Render with URL param page=3 - render( - - - , - ); - await waitFor(() => { - expect(screen.getByTestId('next-page-button')).toBeDisabled(); + + it('a failed append fetch shows the footer error banner (not the full-page banner) and keeps the first batch of cards', async () => { + const user = userEvent.setup(); + mockListDiaryEntries.mockResolvedValueOnce({ + items: [makeSummary('p1-1')], + pagination: { page: 1, pageSize: 25, totalPages: 2, totalItems: 30 }, + }); + mockListDiaryEntries.mockRejectedValueOnce(new Error('network blip')); + + renderPage(); + await waitFor(() => { + expect(screen.getByTestId('diary-card-p1-1')).toBeInTheDocument(); + }); + + await user.click(screen.getByTestId('diary-load-more-button')); + + await waitFor(() => { + expect(screen.getByRole('alert')).toHaveTextContent('Failed to load more entries.'); + }); + // The full-page banner must not render — entries are still present. + expect(screen.queryByText(/failed to load diary entries/i)).not.toBeInTheDocument(); + expect(screen.getByTestId('diary-card-p1-1')).toBeInTheDocument(); }); - }); - // ─── useDebounce migration (#1816): page param not reset on mount ───────── - // Regression test for the `isFirstSearchSync` guard around the debounced - // search-sync effect. Without it, mounting with both `q` and `page` in the - // URL would fire the search-sync effect on mount (since useDebounce returns - // its initial value synchronously) and reset `page` back to '1', discarding - // the user's pagination position on page load/refresh. - - it('does not reset the page URL param to 1 on initial mount when the URL has both q and page', async () => { - // NOTE: mounting with a `page` URL param already produces two fetches by - // design, unrelated to this guard: `currentPage` state initializes to 1, - // then a separate effect syncs it from the `page` URL param once `urlPage` - // is read, triggering a second fetch. That's pre-existing behavior. What - // this test guards against is a THIRD, spurious fetch/URL-rewrite from the - // debounced-search-sync effect resetting `page` back to '1' on mount - // (since useDebounce returns its initial value synchronously, that effect - // would otherwise treat the initial `q` value as a "change"). - mockListDiaryEntries.mockResolvedValue({ - items: [makeSummary('de-1')], - pagination: { page: 3, pageSize: 25, totalPages: 5, totalItems: 120 }, + it('visiting with ?page=3&q=foo in the URL loads page-1 semantics, honors q=foo, and does not error', async () => { + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('foo-1')])); + + render( + + + + , + ); + + await waitFor(() => { + expect(screen.getByTestId('diary-card-foo-1')).toBeInTheDocument(); + }); + + const callArg = mockListDiaryEntries.mock.calls[0]?.[0]; + expect(callArg?.page).toBe(1); + expect(callArg?.q).toBe('foo'); + expect(screen.queryByText(/failed to load/i)).not.toBeInTheDocument(); }); - render( - - - , - ); + it('sets aria-busy=true on the timeline only while an append fetch is loading, and the timeline does not exist during the very first load', async () => { + let resolveFirst: ((v: DiaryEntryListResponse) => void) | undefined; + const firstPromise = new Promise((resolve) => { + resolveFirst = resolve; + }); + let resolveSecond: ((v: DiaryEntryListResponse) => void) | undefined; + const secondPromise = new Promise((resolve) => { + resolveSecond = resolve; + }); - // Final rendered state must reflect page 3, not a reset to page 1. - await waitFor(() => { - expect(screen.getByText('Page 3 of 5')).toBeInTheDocument(); + mockListDiaryEntries.mockImplementationOnce(() => firstPromise); + mockListDiaryEntries.mockImplementationOnce(() => secondPromise); + + renderPage(); + + // Initial load: entries.length === 0, so the timeline isn't rendered yet. + expect(screen.queryByRole('feed')).not.toBeInTheDocument(); + + await act(async () => { + resolveFirst?.({ + items: [makeSummary('p1-1')], + pagination: { page: 1, pageSize: 25, totalPages: 2, totalItems: 30 }, + }); + await firstPromise; + }); + + await waitFor(() => { + expect(screen.getByTestId('diary-card-p1-1')).toBeInTheDocument(); + }); + const timeline = screen.getByRole('feed'); + expect(timeline).toHaveAttribute('aria-busy', 'false'); + + const user = userEvent.setup(); + await user.click(screen.getByTestId('diary-load-more-button')); + + await waitFor(() => { + expect(timeline).toHaveAttribute('aria-busy', 'true'); + }); + + await act(async () => { + resolveSecond?.({ + items: [makeSummary('p2-1')], + pagination: { page: 2, pageSize: 25, totalPages: 2, totalItems: 30 }, + }); + await secondPromise; + }); + + await waitFor(() => { + expect(timeline).toHaveAttribute('aria-busy', 'false'); + }); + }); + + it('announces via the "more entries loaded" copy (not the end-of-list copy) when hasMore remains true after an append', async () => { + const user = userEvent.setup(); + mockListDiaryEntries.mockResolvedValueOnce({ + items: [makeSummary('p1-1')], + pagination: { page: 1, pageSize: 25, totalPages: 3, totalItems: 60 }, + }); + mockListDiaryEntries.mockResolvedValueOnce({ + items: [makeSummary('p2-1')], + pagination: { page: 2, pageSize: 25, totalPages: 3, totalItems: 60 }, + }); + + renderPage(); + await waitFor(() => { + expect(screen.getByTestId('diary-card-p1-1')).toBeInTheDocument(); + }); + + await user.click(screen.getByTestId('diary-load-more-button')); + + await waitFor(() => { + expect(screen.getByRole('status')).toHaveTextContent('1 more entries loaded'); + }); + // Still more pages after this batch — must not use the end-of-list announcement. + expect(screen.getByRole('status')).not.toHaveTextContent(/reached the end/i); + }); + + it('toggling an entry type chip while in manual mode restricts the query to the manual/type intersection and removes the page param', async () => { + const user = userEvent.setup(); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('m-1')])); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('m-2')])); + + renderPage(['/diary']); // default filterMode is 'manual' + await waitFor(() => { + expect(screen.getByTestId('diary-card-m-1')).toBeInTheDocument(); + }); + + await user.click(screen.getByTestId('type-filter-daily_log')); + + await waitFor(() => { + expect(screen.getByTestId('diary-card-m-2')).toBeInTheDocument(); + }); + const lastCall = mockListDiaryEntries.mock.calls[mockListDiaryEntries.mock.calls.length - 1]; + expect(lastCall?.[0]?.type).toBe('daily_log'); + + const search = screen.getByTestId('location-search').textContent ?? ''; + expect(search).toContain('types=daily_log'); + expect(search).not.toContain('page='); }); - // The last API call must have requested page 3 with the search query intact - // — if the isFirstSearchSync guard were missing, the debounced-search-sync - // effect would have rewritten the URL's `page` param back to '1' on mount, - // and this final call/render would show page 1 instead. - const lastCall = mockListDiaryEntries.mock.calls[mockListDiaryEntries.mock.calls.length - 1]; - expect(lastCall?.[0]?.page).toBe(3); - expect(lastCall?.[0]?.q).toBe('foo'); + it('toggling an entry type chip while in automatic mode restricts the query to the automatic/type intersection', async () => { + const user = userEvent.setup(); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('a-1')])); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('a-2')])); + + renderPage(['/diary?filterMode=automatic']); + await waitFor(() => { + expect(screen.getByTestId('diary-card-a-1')).toBeInTheDocument(); + }); + + await user.click(screen.getByTestId('type-filter-work_item_status')); + + await waitFor(() => { + expect(screen.getByTestId('diary-card-a-2')).toBeInTheDocument(); + }); + const lastCall = mockListDiaryEntries.mock.calls[mockListDiaryEntries.mock.calls.length - 1]; + expect(lastCall?.[0]?.type).toBe('work_item_status'); + }); + + it('changing the date-from filter removes the page param and re-fetches from page 1', async () => { + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('d-1')])); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('d-2')])); + + renderPage(); + await waitFor(() => { + expect(screen.getByTestId('diary-card-d-1')).toBeInTheDocument(); + }); + + fireEvent.change(screen.getByTestId('diary-date-from'), { target: { value: '2026-01-01' } }); + + await waitFor(() => { + expect(screen.getByTestId('diary-card-d-2')).toBeInTheDocument(); + }); + const lastCall = mockListDiaryEntries.mock.calls[mockListDiaryEntries.mock.calls.length - 1]; + expect(lastCall?.[0]?.dateFrom).toBe('2026-01-01'); + const search = screen.getByTestId('location-search').textContent ?? ''; + expect(search).not.toContain('page='); + }); + + it('changing the date-to filter removes the page param and re-fetches from page 1', async () => { + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('d-1')])); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('d-2')])); + + renderPage(); + await waitFor(() => { + expect(screen.getByTestId('diary-card-d-1')).toBeInTheDocument(); + }); + + fireEvent.change(screen.getByTestId('diary-date-to'), { target: { value: '2026-02-01' } }); + + await waitFor(() => { + expect(screen.getByTestId('diary-card-d-2')).toBeInTheDocument(); + }); + const lastCall = mockListDiaryEntries.mock.calls[mockListDiaryEntries.mock.calls.length - 1]; + expect(lastCall?.[0]?.dateTo).toBe('2026-02-01'); + const search = screen.getByTestId('location-search').textContent ?? ''; + expect(search).not.toContain('page='); + }); + + it('clicking a filter-mode chip removes the page param, updates the URL, and re-fetches', async () => { + const user = userEvent.setup(); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('fm-1')])); + mockListDiaryEntries.mockResolvedValueOnce(makeListResponse([makeSummary('fm-2')])); + + renderPage(); + await waitFor(() => { + expect(screen.getByTestId('diary-card-fm-1')).toBeInTheDocument(); + }); + + await user.click(screen.getByTestId('mode-filter-automatic')); + + await waitFor(() => { + expect(screen.getByTestId('diary-card-fm-2')).toBeInTheDocument(); + }); + const search = screen.getByTestId('location-search').textContent ?? ''; + expect(search).toContain('filterMode=automatic'); + expect(search).not.toContain('page='); + }); }); // ─── Filter mode changes call API ────────────────────────────────────────── diff --git a/client/src/pages/DiaryPage/DiaryPage.tsx b/client/src/pages/DiaryPage/DiaryPage.tsx index 68905dc69..4c17a228f 100644 --- a/client/src/pages/DiaryPage/DiaryPage.tsx +++ b/client/src/pages/DiaryPage/DiaryPage.tsx @@ -1,4 +1,4 @@ -import { useState, useEffect, useRef, useMemo } from 'react'; +import { useEffect, useRef, useState, useMemo } from 'react'; import { useSearchParams, Link } from 'react-router-dom'; import { useTranslation } from 'react-i18next'; import type { @@ -10,8 +10,10 @@ import type { import { listDiaryEntries } from '../../lib/diaryApi.js'; import { ApiClientError } from '../../lib/apiClient.js'; import { useDebounce } from '../../hooks/useDebounce.js'; +import { useInfiniteScroll, type InfiniteScrollPage } from '../../hooks/useInfiniteScroll.js'; import { DiaryFilterBar } from '../../components/diary/DiaryFilterBar/DiaryFilterBar.js'; import { DiaryDateGroup } from '../../components/diary/DiaryDateGroup/DiaryDateGroup.js'; +import { InfiniteScrollFooter } from '../../components/InfiniteScrollFooter/InfiniteScrollFooter.js'; import shared from '../../styles/shared.module.css'; import styles from './DiaryPage.module.css'; @@ -29,18 +31,14 @@ const MANUAL_TYPES = new Set([ 'general_note', ]); +const PAGE_SIZE = 25; + export default function DiaryPage() { const { t } = useTranslation('diary'); const [searchParams, setSearchParams] = useSearchParams(); - const [entries, setEntries] = useState([]); - const [isLoading, setIsLoading] = useState(true); - const [error, setError] = useState(''); - - const [currentPage, setCurrentPage] = useState(1); - const [totalPages, setTotalPages] = useState(1); const [totalItems, setTotalItems] = useState(0); - const pageSize = 25; + const [error, setError] = useState(''); // Filter state from URL const searchQuery = searchParams.get('q') || ''; @@ -52,17 +50,10 @@ export default function DiaryPage() { ? (typeFilterStr.split(',') as DiaryEntryType[]) : []; const statusFilter = (searchParams.get('status') as DiaryEntryStatus | null) || null; - const urlPage = parseInt(searchParams.get('page') || '1', 10); const [searchInput, setSearchInput] = useState(searchQuery); const announcementRef = useRef(null); - /* eslint-disable @eslint-react/set-state-in-effect -- synchronously sync URL page to component state on page param change */ - useEffect(() => { - if (urlPage !== currentPage) setCurrentPage(urlPage); - }, [urlPage, currentPage]); - /* eslint-enable @eslint-react/set-state-in-effect */ - // Debounced search with URL sync const debouncedSearchInput = useDebounce(searchInput, 300); const isFirstSearchSyncRef = useRef(true); @@ -72,50 +63,45 @@ export default function DiaryPage() { isFirstSearchSyncRef.current = false; return; } - const newParams = new URLSearchParams(searchParams); - if (debouncedSearchInput) { - newParams.set('q', debouncedSearchInput); - } else { - newParams.delete('q'); + setSearchParams((prev) => { + const newParams = new URLSearchParams(prev); + if (debouncedSearchInput) { + newParams.set('q', debouncedSearchInput); + } else { + newParams.delete('q'); + } + newParams.delete('page'); + return newParams; + }); + }, [debouncedSearchInput, setSearchParams]); + + const fetchDiaryPage = async (page: number): Promise> => { + // Determine which types to query based on filter mode + let queriableTypes: DiaryEntryType[] = activeTypes; + if (filterMode === 'manual') { + queriableTypes = + activeTypes.length > 0 + ? activeTypes.filter((type) => MANUAL_TYPES.has(type as ManualDiaryEntryType)) + : (Array.from(MANUAL_TYPES) as DiaryEntryType[]); + } else if (filterMode === 'automatic') { + queriableTypes = + activeTypes.length > 0 + ? activeTypes.filter((type) => !MANUAL_TYPES.has(type as ManualDiaryEntryType)) + : ([ + 'work_item_status', + 'invoice_status', + 'invoice_created', + 'milestone_delay', + 'budget_breach', + 'auto_reschedule', + 'subsidy_status', + ] as const as unknown as DiaryEntryType[]); } - newParams.set('page', '1'); - setSearchParams(newParams); - }, [debouncedSearchInput, searchParams, setSearchParams]); - useEffect(() => { - void loadEntries(); - // eslint-disable-next-line @eslint-react/exhaustive-deps -- loadEntries is defined in the component body; the filter primitives are the intended deps - }, [searchQuery, dateFrom, dateTo, filterMode, typeFilterStr, statusFilter, currentPage]); - - const loadEntries = async () => { - setIsLoading(true); - setError(''); try { - // Determine which types to query based on filter mode - let queriableTypes: DiaryEntryType[] = activeTypes; - if (filterMode === 'manual') { - queriableTypes = - activeTypes.length > 0 - ? activeTypes.filter((t) => MANUAL_TYPES.has(t as ManualDiaryEntryType)) - : (Array.from(MANUAL_TYPES) as DiaryEntryType[]); - } else if (filterMode === 'automatic') { - queriableTypes = - activeTypes.length > 0 - ? activeTypes.filter((t) => !MANUAL_TYPES.has(t as ManualDiaryEntryType)) - : ([ - 'work_item_status', - 'invoice_status', - 'invoice_created', - 'milestone_delay', - 'budget_breach', - 'auto_reschedule', - 'subsidy_status', - ] as const as unknown as DiaryEntryType[]); - } - const response = await listDiaryEntries({ - page: currentPage, - pageSize, + page, + pageSize: PAGE_SIZE, q: searchQuery || undefined, dateFrom: dateFrom || undefined, dateTo: dateTo || undefined, @@ -123,25 +109,47 @@ export default function DiaryPage() { status: statusFilter || undefined, }); - setEntries(response.items); - setTotalPages(response.pagination.totalPages); setTotalItems(response.pagination.totalItems); - - // Announce update - if (announcementRef.current) { - announcementRef.current.textContent = `Loaded ${response.items.length} entries`; - } + setError(''); + return { items: response.items, hasMore: page < response.pagination.totalPages }; } catch (err) { - if (err instanceof ApiClientError) { - setError(err.error.message); - } else { - setError(t('error')); - } - } finally { - setIsLoading(false); + setError(err instanceof ApiClientError ? err.error.message : t('error')); + throw err; } }; + const resetKey = `${searchQuery}|${dateFrom}|${dateTo}|${filterMode}|${typeFilterStr}|${statusFilter}`; + + const { + items: entries, + status, + hasMore, + lastBatchCount, + fetchSequence, + sentinelRef, + loadMore, + retry, + } = useInfiniteScroll({ fetchPage: fetchDiaryPage, resetKey }); + + const showInitialLoading = status === 'loading' && entries.length === 0; + + useEffect(() => { + if (fetchSequence === 0 || !announcementRef.current) return; + if (fetchSequence === 1) { + announcementRef.current.textContent = t('infiniteScroll.initialLoadAnnouncement', { + count: lastBatchCount, + }); + } else if (!hasMore) { + announcementRef.current.textContent = t('infiniteScroll.batchAppendedAndEndAnnouncement', { + count: lastBatchCount, + }); + } else { + announcementRef.current.textContent = t('infiniteScroll.batchAppendedAnnouncement', { + count: lastBatchCount, + }); + } + }, [fetchSequence, hasMore, lastBatchCount, t]); + const groupedEntries = useMemo(() => { const grouped: GroupedEntries = {}; entries.forEach((entry) => { @@ -165,7 +173,7 @@ export default function DiaryPage() { } else { newParams.delete('dateFrom'); } - newParams.set('page', '1'); + newParams.delete('page'); setSearchParams(newParams); }; @@ -176,7 +184,7 @@ export default function DiaryPage() { } else { newParams.delete('dateTo'); } - newParams.set('page', '1'); + newParams.delete('page'); setSearchParams(newParams); }; @@ -187,7 +195,7 @@ export default function DiaryPage() { } else { newParams.delete('types'); } - newParams.set('page', '1'); + newParams.delete('page'); setSearchParams(newParams); }; @@ -195,7 +203,7 @@ export default function DiaryPage() { const newParams = new URLSearchParams(searchParams); newParams.set('filterMode', mode); newParams.delete('types'); - newParams.set('page', '1'); + newParams.delete('page'); setSearchParams(newParams); }; @@ -208,7 +216,7 @@ export default function DiaryPage() { } else { newParams.set('status', 'saved'); } - newParams.set('page', '1'); + newParams.delete('page'); setSearchParams(newParams); }; @@ -219,13 +227,6 @@ export default function DiaryPage() { setSearchParams(newParams); }; - const handlePageChange = (page: number) => { - const newParams = new URLSearchParams(searchParams); - newParams.set('page', page.toString()); - setSearchParams(newParams); - window.scrollTo({ top: 0, behavior: 'smooth' }); - }; - const sortedDates = Object.keys(groupedEntries).sort().reverse(); return ( @@ -249,7 +250,7 @@ export default function DiaryPage() {
- {error &&
{error}
} + {error && entries.length === 0 &&
{error}
} - {isLoading &&
{t('loading')}
} + {showInitialLoading &&
{t('loading')}
} - {!isLoading && entries.length === 0 && ( + {!showInitialLoading && entries.length === 0 && status !== 'error' && (
)} - {!isLoading && entries.length > 0 && ( -
+ {entries.length > 0 && ( +
{sortedDates.map((date) => ( ))} @@ -303,31 +309,14 @@ export default function DiaryPage() { aria-atomic="true" /> - {/* Pagination */} - {!isLoading && totalPages > 1 && ( -
- - - {t('pagination.pageInfo', { currentPage, totalPages })} - - -
+ {entries.length > 0 && ( + )}
); diff --git a/e2e/pages/DiaryPage.ts b/e2e/pages/DiaryPage.ts index 6caffa131..84ab08c0c 100644 --- a/e2e/pages/DiaryPage.ts +++ b/e2e/pages/DiaryPage.ts @@ -10,8 +10,17 @@ * - A timeline of DiaryDateGroup sections (data-testid="date-group-{date}"), each containing * DiaryEntryCard links (data-testid="diary-card-{id}") * - An empty state (class emptyState from shared.module.css) with a "Create your first entry" link - * - A live region (role="status") that announces loaded entry count - * - Pagination: "Previous"/"Next" buttons (data-testid: prev-page-button / next-page-button) + * - A live region (role="status") that announces loaded entry count and batch-append/end-of-list + * status (Issue #2060) + * - Infinite scroll (Issue #2060): the numbered pager is gone entirely (no `?page=` URL param, no + * prev/next buttons). `InfiniteScrollFooter` (data-testid="diary-infinite-scroll-footer") is + * rendered only when `entries.length > 0`, containing (in DOM order): a 0-dimension + * `aria-hidden` sentinel (data-testid="infinite-scroll-sentinel") observed via + * IntersectionObserver to auto-trigger the next batch; a `FormError` banner (role="alert") on + * `status === 'error'`; and either the always-present "Load more"/"Retry" button + * (data-testid="diary-load-more-button", same DOM node across idle/loading/error states, only + * unmounted once `status === 'done'`) or the end-of-list message + * (data-testid="diary-end-of-list") once done. * * Key DOM observations from source: * - h1 has class styles.title (CSS module), not a data-testid; use role heading @@ -22,7 +31,6 @@ * - Search input: data-testid="diary-search-input" (also id="diary-search") * - Type chips: data-testid="type-filter-{type}" * - Clear filters: data-testid="clear-filters-button" - * - Pagination buttons: data-testid="prev-page-button" / data-testid="next-page-button" * - Draft badge on entry card: data-testid="draft-badge-{id}" * - Draft entries link to /diary/:id/edit (not /diary/:id) * - Drafts chip: data-testid="status-filter-drafts" with aria-pressed="true" (default, shows all) @@ -60,9 +68,11 @@ export class DiaryPage { // Error banner readonly errorBanner: Locator; - // Pagination - readonly prevPageButton: Locator; - readonly nextPageButton: Locator; + // Infinite scroll footer (Issue #2060) — replaces the numbered pager + readonly loadMoreButton: Locator; + readonly endOfListMessage: Locator; + readonly infiniteScrollSentinel: Locator; + readonly footerError: Locator; // "Drafts" toggle chip — data-testid="status-filter-drafts" (added in #1446) // aria-pressed="true" → all entries shown (default) @@ -102,8 +112,13 @@ export class DiaryPage { this.errorBanner = page.locator('[class*="bannerError"]'); - this.prevPageButton = page.getByTestId('prev-page-button'); - this.nextPageButton = page.getByTestId('next-page-button'); + // Infinite scroll footer (Issue #2060) — replaces the numbered pager entirely. + // Rendered only when entries.length > 0; footerError is scoped to the footer's own + // role="alert" banner so it never matches the page-level errorBanner above. + this.loadMoreButton = page.getByTestId('diary-load-more-button'); + this.endOfListMessage = page.getByTestId('diary-end-of-list'); + this.infiniteScrollSentinel = page.getByTestId('infinite-scroll-sentinel'); + this.footerError = page.getByTestId('diary-infinite-scroll-footer').getByRole('alert'); } /** @@ -139,6 +154,24 @@ export class DiaryPage { ]); } + /** + * Scroll to the bottom of the page to trigger the infinite-scroll sentinel + * (Issue #2060) and wait for either a new /api/diary-entries response or the + * end-of-list message to appear — whichever happens first. Mirrors the + * waitForLoaded() race pattern above. Does not assert anything itself; callers + * that need to inspect the specific response (e.g. asserting a page number or + * status code) should register their own `page.waitForResponse` before calling + * this, since this helper's own listener is not exposed. + * No explicit timeout — uses project-level actionTimeout. + */ + async scrollToLoadMore(): Promise { + const responsePromise = this.page + .waitForResponse((resp) => resp.url().includes('/api/diary-entries')) + .catch(() => null); + await this.page.evaluate(() => window.scrollTo(0, document.body.scrollHeight)); + await Promise.race([responsePromise, this.endOfListMessage.waitFor({ state: 'visible' })]); + } + /** * Get all entry card locators currently rendered in the timeline. */ diff --git a/e2e/tests/diary/diary-list.spec.ts b/e2e/tests/diary/diary-list.spec.ts index 5156197d9..7cb4fb7c9 100644 --- a/e2e/tests/diary/diary-list.spec.ts +++ b/e2e/tests/diary/diary-list.spec.ts @@ -10,7 +10,9 @@ * 4. Entry created via API appears in the timeline * 5. Date grouping — entries on different dates render separate date headers * 6. Search filter finds a specific entry - * 7. "Next" pagination button fetches page 2 (mock API) + * 7. Infinite scroll replaces the numbered pager (Issue #2060) — auto-load on scroll, + * keyboard-only "Load more", full pager removal, dedupe under fast scroll, end-of-list, + * empty state, filter/search reset, error+retry, legacy ?page= bookmarks, dark mode * 8. Entry card click navigates to the detail page * 9. Type switcher filters to manual-only entries (mock API) * 10. Responsive — no horizontal scroll on current viewport (@responsive) @@ -360,77 +362,610 @@ test.describe('Search filter (Scenario 6)', { tag: '@responsive' }, () => { }); // ───────────────────────────────────────────────────────────────────────────── -// Scenario 7: "Next" pagination button fetches page 2 (mock API) +// Scenario 7: Infinite scroll replaces the numbered pager (Issue #2060) +// +// The pager (prev/next buttons, ?page= URL param) is gone entirely. Older entries now +// load via IntersectionObserver-driven auto-append (useInfiniteScroll) plus an always +// -present keyboard-reachable "Load more"/"Retry" button (InfiniteScrollFooter). Both +// paths call the same loadMore()/retry() functions from the hook, so there is exactly +// one code path per action. See client/src/hooks/useInfiniteScroll.ts and +// client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.tsx. // ───────────────────────────────────────────────────────────────────────────── -test.describe('Pagination (Scenario 7)', () => { - test('Pagination controls are visible when totalPages > 1', async ({ page }) => { - const diaryPage = new DiaryPage(page); +test.describe('Infinite scroll (Scenario 7)', () => { + test.describe('Auto-load on scroll', { tag: '@responsive' }, () => { + test( + 'Scrolling near the bottom automatically loads and appends the next batch', + { tag: '@smoke' }, + async ({ page }) => { + const diaryPage = new DiaryPage(page); + + const page1Entries = Array.from({ length: 25 }, (_, i) => + makeMockEntry({ + id: `is-p1-${i}`, + title: `Batch 1 Entry ${String(i + 1).padStart(2, '0')}`, + }), + ); + const page2Entries = Array.from({ length: 25 }, (_, i) => + makeMockEntry({ + id: `is-p2-${i}`, + title: `Batch 2 Entry ${String(i + 1).padStart(2, '0')}`, + }), + ); + + await page.route('**/api/diary-entries*', async (route) => { + if (route.request().method() !== 'GET') { + await route.continue(); + return; + } + const requestedPage = new URL(route.request().url()).searchParams.get('page'); + const body = + requestedPage === '2' + ? makePaginatedResponse(page2Entries, { page: 2, totalItems: 50, totalPages: 2 }) + : makePaginatedResponse(page1Entries, { totalItems: 50, totalPages: 2 }); + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify(body), + }); + }); - // Return a multi-page response so the pagination bar renders - const entries = Array.from({ length: 25 }, (_, i) => - makeMockEntry({ - id: `pag-entry-${i}`, - title: `Paginated Entry ${String(i + 1).padStart(2, '0')}`, - }), + try { + await diaryPage.goto(); + await diaryPage.waitForLoaded(); + + await expect(diaryPage.entryCard('is-p1-0')).toBeVisible(); + await expect(diaryPage.entryCard('is-p2-0')).toHaveCount(0); + + const page2ResponsePromise = page.waitForResponse( + (resp) => + resp.url().includes('/api/diary-entries') && + new URL(resp.url()).searchParams.get('page') === '2', + ); + await page.evaluate(() => window.scrollTo(0, document.body.scrollHeight)); + await page2ResponsePromise; + + // The original page-1 cards must remain mounted — appending never replaces them. + await expect(diaryPage.entryCard('is-p1-0')).toBeVisible(); + await expect(diaryPage.entryCard('is-p2-0')).toBeVisible(); + await expect(diaryPage.endOfListMessage).toBeVisible(); + + // No pagination-style URL state is ever introduced. + expect(new URL(page.url()).searchParams.has('page')).toBe(false); + } finally { + await page.unroute('**/api/diary-entries*'); + } + }, ); + }); + + test('"Load more" button loads the next batch via keyboard alone, with no scroll', async ({ + page, + }) => { + const diaryPage = new DiaryPage(page); + + const page1Entries = Array.from({ length: 25 }, (_, i) => makeMockEntry({ id: `kbd-p1-${i}` })); + const page2Entries = Array.from({ length: 25 }, (_, i) => makeMockEntry({ id: `kbd-p2-${i}` })); + + await page.route('**/api/diary-entries*', async (route) => { + if (route.request().method() !== 'GET') { + await route.continue(); + return; + } + const requestedPage = new URL(route.request().url()).searchParams.get('page'); + const body = + requestedPage === '2' + ? makePaginatedResponse(page2Entries, { page: 2, totalItems: 50, totalPages: 2 }) + : makePaginatedResponse(page1Entries, { totalItems: 50, totalPages: 2 }); + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify(body), + }); + }); + + try { + await diaryPage.goto(); + await diaryPage.waitForLoaded(); + + await expect(diaryPage.loadMoreButton).toBeVisible(); + await diaryPage.loadMoreButton.focus(); + await expect(diaryPage.loadMoreButton).toBeFocused(); + + // A visible focus outline (box-shadow ring, per shared.btnSecondary:focus-visible) must be + // present in both light and dark mode — never a plain/missing outline. + const lightFocusBoxShadow = await diaryPage.loadMoreButton.evaluate( + (el) => getComputedStyle(el).boxShadow, + ); + expect(lightFocusBoxShadow).not.toBe('none'); + + await page.evaluate(() => document.documentElement.setAttribute('data-theme', 'dark')); + const darkFocusBoxShadow = await diaryPage.loadMoreButton.evaluate( + (el) => getComputedStyle(el).boxShadow, + ); + expect(darkFocusBoxShadow).not.toBe('none'); + await page.evaluate(() => document.documentElement.removeAttribute('data-theme')); + + // No mouse/scroll interaction at all — just focus + Enter. + const page2ResponsePromise = page.waitForResponse( + (resp) => + resp.url().includes('/api/diary-entries') && + new URL(resp.url()).searchParams.get('page') === '2', + ); + await page.keyboard.press('Enter'); + await page2ResponsePromise; + + await expect(diaryPage.entryCard('kbd-p2-0')).toBeVisible(); + } finally { + await page.unroute('**/api/diary-entries*'); + } + }); + + test('No pagination controls remain anywhere on the page — full removal, not just hidden', async ({ + page, + }) => { + const diaryPage = new DiaryPage(page); + + await diaryPage.goto(); + await diaryPage.waitForLoaded(); + + await expect(page.getByTestId('prev-page-button')).toHaveCount(0); + await expect(page.getByTestId('next-page-button')).toHaveCount(0); + }); + + test('Fast repeated scrolling does not issue duplicate requests for the same batch (best-effort)', async ({ + page, + }) => { + const diaryPage = new DiaryPage(page); + + const page1Entries = Array.from({ length: 25 }, (_, i) => makeMockEntry({ id: `dd-p1-${i}` })); + const page2Entries = Array.from({ length: 25 }, (_, i) => makeMockEntry({ id: `dd-p2-${i}` })); + let pageTwoRequestCount = 0; + + await page.route('**/api/diary-entries*', async (route) => { + if (route.request().method() !== 'GET') { + await route.continue(); + return; + } + const requestedPage = new URL(route.request().url()).searchParams.get('page'); + if (requestedPage === '2') { + pageTwoRequestCount += 1; + // Artificial delay so multiple rapid scroll triggers land while the fetch is in flight. + await new Promise((resolve) => setTimeout(resolve, 500)); + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify( + makePaginatedResponse(page2Entries, { page: 2, totalItems: 50, totalPages: 2 }), + ), + }); + return; + } + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify( + makePaginatedResponse(page1Entries, { totalItems: 50, totalPages: 2 }), + ), + }); + }); + + try { + await diaryPage.goto(); + await diaryPage.waitForLoaded(); + + const page2ResponsePromise = page.waitForResponse( + (resp) => + resp.url().includes('/api/diary-entries') && + new URL(resp.url()).searchParams.get('page') === '2', + ); + + // Fire several scroll-to-bottom events in quick succession, well within the 500ms + // in-flight window for the page-2 request. + for (let i = 0; i < 5; i++) { + await page.evaluate(() => window.scrollTo(0, document.body.scrollHeight)); + await page.waitForTimeout(50); + } + + await page2ResponsePromise; + expect(pageTwoRequestCount).toBe(1); + await expect(diaryPage.entryCard('dd-p2-0')).toBeVisible(); + } finally { + await page.unroute('**/api/diary-entries*'); + } + }); + + test('A dataset smaller than one batch reaches end-of-list immediately with no second request', async ({ + page, + }) => { + const diaryPage = new DiaryPage(page); + let requestCount = 0; + + await page.route('**/api/diary-entries*', async (route) => { + if (route.request().method() !== 'GET') { + await route.continue(); + return; + } + requestCount += 1; + const entries = Array.from({ length: 5 }, (_, i) => makeMockEntry({ id: `small-${i}` })); + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify(makePaginatedResponse(entries, { totalItems: 5, totalPages: 1 })), + }); + }); + + try { + await diaryPage.goto(); + await diaryPage.waitForLoaded(); + + await expect(diaryPage.endOfListMessage).toBeVisible(); + await expect(diaryPage.loadMoreButton).toHaveCount(0); + expect(requestCount).toBe(1); + } finally { + await page.unroute('**/api/diary-entries*'); + } + }); + + test('Zero matching entries renders the empty state with no footer, sentinel, or load-more control', async ({ + page, + }) => { + const diaryPage = new DiaryPage(page); await page.route('**/api/diary-entries*', async (route) => { if (route.request().method() === 'GET') { await route.fulfill({ status: 200, contentType: 'application/json', - body: JSON.stringify(makePaginatedResponse(entries, { totalItems: 50, totalPages: 2 })), + body: JSON.stringify(makePaginatedResponse([])), }); } else { await route.continue(); } }); + try { + await diaryPage.goto(); + + await expect(diaryPage.emptyState).toBeVisible(); + await expect(diaryPage.loadMoreButton).toHaveCount(0); + await expect(diaryPage.infiniteScrollSentinel).toHaveCount(0); + await expect(diaryPage.endOfListMessage).toHaveCount(0); + } finally { + await page.unroute('**/api/diary-entries*'); + } + }); + + test('Changing a filter mid-scroll discards the old batches and loads a fresh first batch', async ({ + page, + }) => { + const diaryPage = new DiaryPage(page); + + const manualPage1 = Array.from({ length: 25 }, (_, i) => + makeMockEntry({ id: `flt-man1-${i}` }), + ); + const manualPage2 = Array.from({ length: 25 }, (_, i) => + makeMockEntry({ id: `flt-man2-${i}` }), + ); + const automaticEntries = [makeMockEntry({ id: 'flt-auto-0', entryType: 'work_item_status' })]; + + await page.route('**/api/diary-entries*', async (route) => { + if (route.request().method() !== 'GET') { + await route.continue(); + return; + } + const url = new URL(route.request().url()); + const typeParam = url.searchParams.get('type') ?? ''; + const requestedPage = url.searchParams.get('page'); + + if (typeParam.includes('work_item_status')) { + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify( + makePaginatedResponse(automaticEntries, { totalItems: 1, totalPages: 1 }), + ), + }); + return; + } + + const body = + requestedPage === '2' + ? makePaginatedResponse(manualPage2, { page: 2, totalItems: 50, totalPages: 2 }) + : makePaginatedResponse(manualPage1, { totalItems: 50, totalPages: 2 }); + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify(body), + }); + }); + try { await diaryPage.goto(); await diaryPage.waitForLoaded(); + await diaryPage.scrollToLoadMore(); + + await expect(diaryPage.entryCard('flt-man1-0')).toBeVisible(); + await expect(diaryPage.entryCard('flt-man2-0')).toBeVisible(); - await expect(diaryPage.prevPageButton).toBeVisible(); - await expect(diaryPage.nextPageButton).toBeVisible(); + await diaryPage.openFiltersIfCollapsed(); + const automaticResponsePromise = page.waitForResponse( + (resp) => + resp.url().includes('/api/diary-entries') && + new URL(resp.url()).searchParams.get('type')?.includes('work_item_status') === true, + ); + await page.getByTestId('mode-filter-automatic').click(); + await automaticResponsePromise; + await diaryPage.waitForLoaded(); - // Previous button disabled on page 1 - await expect(diaryPage.prevPageButton).toBeDisabled(); + // Old manual-mode batches are discarded, not just visually hidden. + await expect(diaryPage.entryCard('flt-man1-0')).toHaveCount(0); + await expect(diaryPage.entryCard('flt-man2-0')).toHaveCount(0); + await expect(diaryPage.entryCard('flt-auto-0')).toBeVisible(); - // Next button enabled on page 1 - await expect(diaryPage.nextPageButton).toBeEnabled(); + const count = await diaryPage.getEntryCount(); + expect(count).toBe(1); } finally { await page.unroute('**/api/diary-entries*'); } }); - test('Pagination is not shown when all entries fit on one page', async ({ page }) => { + test('Typing a search query resets the list and the URL never gains a page param', async ({ + page, + }) => { const diaryPage = new DiaryPage(page); + const page1Entries = Array.from({ length: 25 }, (_, i) => + makeMockEntry({ id: `srch-p1-${i}` }), + ); + const page2Entries = Array.from({ length: 25 }, (_, i) => + makeMockEntry({ id: `srch-p2-${i}` }), + ); + await page.route('**/api/diary-entries*', async (route) => { - if (route.request().method() === 'GET') { + if (route.request().method() !== 'GET') { + await route.continue(); + return; + } + const requestedPage = new URL(route.request().url()).searchParams.get('page'); + const body = + requestedPage === '2' + ? makePaginatedResponse(page2Entries, { page: 2, totalItems: 50, totalPages: 2 }) + : makePaginatedResponse(page1Entries, { totalItems: 50, totalPages: 2 }); + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify(body), + }); + }); + + try { + await diaryPage.goto(); + await diaryPage.waitForLoaded(); + await diaryPage.scrollToLoadMore(); + + expect(new URL(page.url()).searchParams.has('page')).toBe(false); + + await diaryPage.search('mock search query'); + + expect(page.url()).toContain('q='); + expect(new URL(page.url()).searchParams.has('page')).toBe(false); + } finally { + await page.unroute('**/api/diary-entries*'); + } + }); + + test('A failed batch shows an error with retry at the footer, and retry appends the batch exactly once', async ({ + page, + }) => { + const diaryPage = new DiaryPage(page); + + const page1Entries = Array.from({ length: 25 }, (_, i) => makeMockEntry({ id: `err-p1-${i}` })); + const page2Entries = Array.from({ length: 25 }, (_, i) => makeMockEntry({ id: `err-p2-${i}` })); + let pageTwoRequestCount = 0; + + await page.route('**/api/diary-entries*', async (route) => { + if (route.request().method() !== 'GET') { + await route.continue(); + return; + } + const requestedPage = new URL(route.request().url()).searchParams.get('page'); + if (requestedPage === '2') { + pageTwoRequestCount += 1; + if (pageTwoRequestCount === 1) { + await route.fulfill({ + status: 500, + contentType: 'application/json', + body: JSON.stringify({ error: { code: 'INTERNAL_ERROR', message: 'Server error' } }), + }); + return; + } await route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify( - makePaginatedResponse([makeMockEntry()], { totalItems: 1, totalPages: 1 }), + makePaginatedResponse(page2Entries, { page: 2, totalItems: 50, totalPages: 2 }), ), }); - } else { - await route.continue(); + return; } + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify( + makePaginatedResponse(page1Entries, { totalItems: 50, totalPages: 2 }), + ), + }); }); try { await diaryPage.goto(); await diaryPage.waitForLoaded(); - // Pagination buttons are not rendered when totalPages === 1 - await expect(diaryPage.prevPageButton).not.toBeVisible(); - await expect(diaryPage.nextPageButton).not.toBeVisible(); + const failedResponsePromise = page.waitForResponse( + (resp) => + resp.url().includes('/api/diary-entries') && + new URL(resp.url()).searchParams.get('page') === '2', + ); + await page.evaluate(() => window.scrollTo(0, document.body.scrollHeight)); + await failedResponsePromise; + + await expect(diaryPage.footerError).toBeVisible(); + // Every already-loaded entry remains on screen. + await expect(diaryPage.entryCard('err-p1-0')).toBeVisible(); + + // Scrolling again while errored must not re-issue the request (AC15). + const requestsBeforeExtraScroll = pageTwoRequestCount; + await page.evaluate(() => window.scrollTo(0, document.body.scrollHeight)); + await page.waitForTimeout(300); + expect(pageTwoRequestCount).toBe(requestsBeforeExtraScroll); + + // Retry re-requests the same batch; on success it appends with no duplicates. + const retryResponsePromise = page.waitForResponse( + (resp) => + resp.url().includes('/api/diary-entries') && + new URL(resp.url()).searchParams.get('page') === '2' && + resp.status() === 200, + ); + await diaryPage.loadMoreButton.click(); + await retryResponsePromise; + + await expect(diaryPage.entryCard('err-p1-0')).toBeVisible(); + await expect(diaryPage.entryCard('err-p2-0')).toHaveCount(1); + await expect(diaryPage.endOfListMessage).toBeVisible(); + expect(pageTwoRequestCount).toBe(2); } finally { await page.unroute('**/api/diary-entries*'); } }); + + test('An old /diary?page=3 bookmark loads normally from the first batch with no error', async ({ + page, + }) => { + const diaryPage = new DiaryPage(page); + const entries = Array.from({ length: 5 }, (_, i) => makeMockEntry({ id: `bm-${i}` })); + const requestedPages: (string | null)[] = []; + + await page.route('**/api/diary-entries*', async (route) => { + if (route.request().method() !== 'GET') { + await route.continue(); + return; + } + requestedPages.push(new URL(route.request().url()).searchParams.get('page')); + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify(makePaginatedResponse(entries, { totalItems: 5, totalPages: 1 })), + }); + }); + + try { + await page.goto(`${DIARY_ROUTE}?page=3`); + await diaryPage.heading.waitFor({ state: 'visible' }); + await diaryPage.waitForLoaded(); + + await expect(diaryPage.heading).toBeVisible(); + await expect(diaryPage.errorBanner).not.toBeVisible(); + await expect(diaryPage.entryCard('bm-0')).toBeVisible(); + + // The internal batch counter always starts at 1, regardless of a stale ?page= value. + expect(requestedPages[0]).toBe('1'); + } finally { + await page.unroute('**/api/diary-entries*'); + } + }); + + test.describe('Dark mode', { tag: '@responsive' }, () => { + test('Loading-error and end-of-list footer states render correctly in dark mode', async ({ + page, + }) => { + const diaryPage = new DiaryPage(page); + + const page1Entries = Array.from({ length: 25 }, (_, i) => + makeMockEntry({ id: `dm-p1-${i}` }), + ); + const page2Entries = Array.from({ length: 25 }, (_, i) => + makeMockEntry({ id: `dm-p2-${i}` }), + ); + let pageTwoAttempts = 0; + + await page.route('**/api/diary-entries*', async (route) => { + if (route.request().method() !== 'GET') { + await route.continue(); + return; + } + const requestedPage = new URL(route.request().url()).searchParams.get('page'); + if (requestedPage === '2') { + pageTwoAttempts += 1; + if (pageTwoAttempts === 1) { + await route.fulfill({ + status: 500, + contentType: 'application/json', + body: JSON.stringify({ error: { code: 'INTERNAL_ERROR', message: 'Server error' } }), + }); + return; + } + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify( + makePaginatedResponse(page2Entries, { page: 2, totalItems: 50, totalPages: 2 }), + ), + }); + return; + } + await route.fulfill({ + status: 200, + contentType: 'application/json', + body: JSON.stringify( + makePaginatedResponse(page1Entries, { totalItems: 50, totalPages: 2 }), + ), + }); + }); + + try { + await page.goto(DIARY_ROUTE); + await page.evaluate(() => { + document.documentElement.setAttribute('data-theme', 'dark'); + }); + await diaryPage.heading.waitFor({ state: 'visible' }); + await diaryPage.waitForLoaded(); + + const failedResponsePromise = page.waitForResponse( + (resp) => + resp.url().includes('/api/diary-entries') && + new URL(resp.url()).searchParams.get('page') === '2', + ); + await page.evaluate(() => window.scrollTo(0, document.body.scrollHeight)); + await failedResponsePromise; + + await expect(diaryPage.footerError).toBeVisible(); + const loadMoreClass = await diaryPage.loadMoreButton.getAttribute('class'); + expect(loadMoreClass).toContain('btnSecondary'); + + let hasHorizontalScroll = await page.evaluate( + () => document.documentElement.scrollWidth > window.innerWidth, + ); + expect(hasHorizontalScroll).toBe(false); + + const successResponsePromise = page.waitForResponse( + (resp) => + resp.url().includes('/api/diary-entries') && + new URL(resp.url()).searchParams.get('page') === '2' && + resp.status() === 200, + ); + await diaryPage.loadMoreButton.click(); + await successResponsePromise; + + await expect(diaryPage.endOfListMessage).toBeVisible(); + hasHorizontalScroll = await page.evaluate( + () => document.documentElement.scrollWidth > window.innerWidth, + ); + expect(hasHorizontalScroll).toBe(false); + } finally { + await page.unroute('**/api/diary-entries*'); + } + }); + }); }); // ───────────────────────────────────────────────────────────────────────────── From d386dd1ca0c1ca214604ad0182becc35a27c984d Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Fri, 4 Sep 2026 13:31:27 +0200 Subject: [PATCH 2/7] fix(diary): resolve CI typecheck failure in InfiniteScrollFooter test noUncheckedIndexedAccess makes CSS-module index access string | undefined; non-null assert to match the existing DateRangePicker.test.tsx pattern for toHaveClass(styles.). Co-Authored-By: Claude dev-team-lead Co-Authored-By: Claude qa-integration-tester --- .../InfiniteScrollFooter/InfiniteScrollFooter.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx index cdc4c5aca..2671113c9 100644 --- a/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx +++ b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx @@ -123,6 +123,6 @@ describe('InfiniteScrollFooter', () => { it('the sentinel div has zero width/height via its CSS module class', () => { renderFooter({ status: 'idle' }); - expect(screen.getByTestId('infinite-scroll-sentinel')).toHaveClass(styles.sentinel); + expect(screen.getByTestId('infinite-scroll-sentinel')).toHaveClass(styles.sentinel!); }); }); From 5c90bcd07a31c8d21077bcda6700b9860200ddfd Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Fri, 4 Sep 2026 13:55:08 +0200 Subject: [PATCH 3/7] fix(diary): add German infiniteScroll translations, isolate keyboard-activation E2E test from observer race Adds the missing German infiniteScroll.* keys (and removes the stale pagination block) in client/src/i18n/de/diary.json to restore i18n parity, resolving the Quality Gates Jest failure. Also stubs IntersectionObserver in the keyboard-only "Load more" E2E test so its own click/keypress activation path is isolated from the auto-scroll trigger, which could otherwise race and unmount the button before the test's focus assertion ran. Co-Authored-By: Claude dev-team-lead Co-Authored-By: Claude translator Co-Authored-By: Claude e2e-test-engineer --- .../e2e-test-engineer/flake-patterns.md | 13 ++++++ .../issue-2060-diary-infinite-scroll.md | 44 +++++++++++++++++++ client/src/i18n/de/diary.json | 14 ++++-- e2e/tests/diary/diary-list.spec.ts | 28 +++++++++++- 4 files changed, 94 insertions(+), 5 deletions(-) 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 index a4553e5d1..498f3eae5 100644 --- 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 @@ -97,6 +97,50 @@ pager locators, explicitly out of scope per the issue and untouched). 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`, diff --git a/client/src/i18n/de/diary.json b/client/src/i18n/de/diary.json index 89d8110e0..8dd3e4118 100644 --- a/client/src/i18n/de/diary.json +++ b/client/src/i18n/de/diary.json @@ -12,10 +12,16 @@ }, "loading": "Einträge werden geladen...", "error": "Fehler beim Laden von Tagebucheinträgen. Bitte versuchen Sie es erneut.", - "pagination": { - "previous": "Zurück", - "next": "Weiter", - "pageInfo": "Seite {{currentPage}} von {{totalPages}}" + "infiniteScroll": { + "loadingMore": "Weitere Einträge werden geladen…", + "loadingMoreAriaLabel": "Weitere Tagebucheinträge werden geladen", + "loadMoreButton": "Mehr laden", + "retryButton": "Erneut versuchen", + "errorMessage": "Weitere Einträge konnten nicht geladen werden.", + "endOfList": "Sie haben das Ende erreicht – keine weiteren Einträge vorhanden.", + "initialLoadAnnouncement": "{{count}} Einträge geladen", + "batchAppendedAnnouncement": "{{count}} weitere Einträge geladen", + "batchAppendedAndEndAnnouncement": "{{count}} weitere Einträge geladen. Sie haben das Ende erreicht." }, "filterBar": { "search": "Einträge durchsuchen...", diff --git a/e2e/tests/diary/diary-list.spec.ts b/e2e/tests/diary/diary-list.spec.ts index 7cb4fb7c9..9c38add58 100644 --- a/e2e/tests/diary/diary-list.spec.ts +++ b/e2e/tests/diary/diary-list.spec.ts @@ -443,6 +443,31 @@ test.describe('Infinite scroll (Scenario 7)', () => { }) => { const diaryPage = new DiaryPage(page); + // Stub out IntersectionObserver for this test only. Without this, the sentinel can already + // be within the observer's 600px rootMargin the instant it mounts (page-1's mocked entries are + // short, and focusing/tabbing to an off-screen element also scrolls it into view as a normal + // part of browser/Playwright focus handling) — either way, the auto-scroll loadMore() path can + // fire and complete (the mock has no delay) before this test's own focus/keyboard assertions + // run, unmounting diary-load-more-button once hasMore flips to false and turning + // `toBeFocused()` into a deterministic "element(s) not found". This test's whole point is to + // prove the button's OWN click/keypress activation works independent of the auto-scroll + // observer, so isolate that path entirely rather than trying to out-race it. + await page.addInitScript(() => { + class StubIntersectionObserver implements IntersectionObserver { + readonly root: Element | Document | null = null; + readonly rootMargin = ''; + readonly thresholds: ReadonlyArray = []; + constructor(_callback: IntersectionObserverCallback, _options?: IntersectionObserverInit) {} + disconnect(): void {} + observe(): void {} + unobserve(): void {} + takeRecords(): IntersectionObserverEntry[] { + return []; + } + } + window.IntersectionObserver = StubIntersectionObserver; + }); + const page1Entries = Array.from({ length: 25 }, (_, i) => makeMockEntry({ id: `kbd-p1-${i}` })); const page2Entries = Array.from({ length: 25 }, (_, i) => makeMockEntry({ id: `kbd-p2-${i}` })); @@ -485,7 +510,8 @@ test.describe('Infinite scroll (Scenario 7)', () => { expect(darkFocusBoxShadow).not.toBe('none'); await page.evaluate(() => document.documentElement.removeAttribute('data-theme')); - // No mouse/scroll interaction at all — just focus + Enter. + // No mouse/scroll interaction at all — just focus + Enter. With IntersectionObserver + // stubbed above, this Enter press is the ONLY thing that can trigger the fetch below. const page2ResponsePromise = page.waitForResponse( (resp) => resp.url().includes('/api/diary-entries') && From d77524a6d0896842f409b9721508fa0b5606046a Mon Sep 17 00:00:00 2001 From: Frank Steiler Date: Fri, 4 Sep 2026 14:34:41 +0200 Subject: [PATCH 4/7] =?UTF-8?q?fix(diary):=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20epoch-gated=20metadata,=20reusable=20footer=20props?= =?UTF-8?q?,=20pluralized=20announcements?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - useInfiniteScroll: epoch-gated batch metadata, fetchSequence/lastBatchCount reset on resetKey change - InfiniteScrollFooter: reusable props instead of diary-specific assumptions, removed duplicate loading indicator, dropped dead hasMore prop - DiaryPage: wiring updates for the above - i18n: singular/plural announcement keys in en/de diary.json (screen-reader announcements now grammatically correct for count === 1) - e2e: updated sentinel/footer testids in DiaryPage POM to match Co-Authored-By: Claude dev-team-lead Co-Authored-By: Claude frontend-developer Co-Authored-By: Claude qa-integration-tester Co-Authored-By: Claude e2e-test-engineer Co-Authored-By: Claude translator --- .../InfiniteScrollFooter.module.css | 10 -- .../InfiniteScrollFooter.test.tsx | 75 ++++++--- .../InfiniteScrollFooter.tsx | 48 +++--- client/src/hooks/useInfiniteScroll.test.tsx | 149 ++++++++++++++++++ client/src/hooks/useInfiniteScroll.ts | 38 ++++- client/src/i18n/de/diary.json | 9 +- client/src/i18n/en/diary.json | 9 +- client/src/pages/DiaryPage/DiaryPage.test.tsx | 97 +++++++++++- client/src/pages/DiaryPage/DiaryPage.tsx | 67 +++++--- e2e/pages/DiaryPage.ts | 25 +-- 10 files changed, 422 insertions(+), 105 deletions(-) diff --git a/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.module.css b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.module.css index 6ab801ce5..20d812847 100644 --- a/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.module.css +++ b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.module.css @@ -8,16 +8,6 @@ height: 0; } -.statusRow { - display: flex; - align-items: center; - justify-content: center; - gap: var(--spacing-2); - min-height: var(--spacing-12); - color: var(--color-text-muted); - font-size: var(--font-size-sm); -} - .endOfList { border-top: 1px solid var(--color-border); padding: var(--spacing-6) var(--spacing-3); diff --git a/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx index 2671113c9..7a5b39fed 100644 --- a/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx +++ b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.test.tsx @@ -3,25 +3,32 @@ * * Unit tests for InfiniteScrollFooter (Issue #2060 — Diary infinite-scroll rework). * - * Real i18n is initialized (via import of app i18n setup) so useTranslation() resolves - * actual copy from client/src/i18n/en/diary.json, matching the established pattern in - * PhotoUpload.test.tsx. + * The component takes plain string label/message props (no useTranslation of its own — + * callers translate and pass the resolved strings), so no i18n bootstrap is needed here. */ import { jest, describe, it, expect } from '@jest/globals'; import { render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import '../../i18n/index.js'; import { InfiniteScrollFooter } from './InfiniteScrollFooter.js'; import styles from './InfiniteScrollFooter.module.css'; import type { InfiniteScrollFooterProps } from './InfiniteScrollFooter.js'; +const LABELS = { + loadingLabel: 'Loading more entries…', + loadingAriaLabel: 'Loading more diary entries', + loadMoreLabel: 'Load more', + retryLabel: 'Retry', + errorMessage: 'Failed to load more entries.', + endOfListMessage: "You've reached the end — no more entries to load.", +}; + function renderFooter(overrides: Partial = {}) { const onLoadMore = jest.fn(); const onRetry = jest.fn(); const sentinelRef = jest.fn(); const props: InfiniteScrollFooterProps = { status: 'idle', - hasMore: true, + ...LABELS, sentinelRef, onLoadMore, onRetry, @@ -41,7 +48,7 @@ describe('InfiniteScrollFooter', () => { expect(sentinel).toBeInTheDocument(); expect(sentinel).toHaveAttribute('aria-hidden', 'true'); - const button = screen.getByTestId('diary-load-more-button'); + const button = screen.getByTestId('infinite-scroll-load-more-button'); expect(button).toBeEnabled(); expect(button).toHaveTextContent('Load more'); }); @@ -50,7 +57,7 @@ describe('InfiniteScrollFooter', () => { const user = userEvent.setup(); const { onLoadMore, onRetry } = renderFooter({ status: 'idle' }); - await user.click(screen.getByTestId('diary-load-more-button')); + await user.click(screen.getByTestId('infinite-scroll-load-more-button')); expect(onLoadMore).toHaveBeenCalledTimes(1); expect(onRetry).not.toHaveBeenCalled(); @@ -58,21 +65,16 @@ describe('InfiniteScrollFooter', () => { // ─── loading ───────────────────────────────────────────────────────────────── - it('loading: the button is disabled and shows a spinner + "Loading more entries…" inside it', () => { + it('loading: the button is disabled and shows a spinner + loadingLabel inside it, with no separate status row', () => { renderFooter({ status: 'loading' }); - const button = screen.getByTestId('diary-load-more-button'); + const button = screen.getByTestId('infinite-scroll-load-more-button'); expect(button).toBeDisabled(); expect(button).toHaveTextContent('Loading more entries…'); - }); - - it('loading: a separate status row also renders a spinner + "Loading more entries…"', () => { - renderFooter({ status: 'loading' }); - // Two occurrences of the loading copy: one inside the button, one in the status row. - expect(screen.getAllByText('Loading more entries…')).toHaveLength(2); - // The status-row spinner carries the dedicated aria-label; the button's spinner - // uses the Spinner default label ("Loading"). + // Exactly one occurrence of the loading copy — the standalone status row + // (duplicated rendering) was removed; only the button's own label remains. + expect(screen.getAllByText('Loading more entries…')).toHaveLength(1); expect(screen.getByRole('img', { name: 'Loading more diary entries' })).toBeInTheDocument(); }); @@ -88,7 +90,7 @@ describe('InfiniteScrollFooter', () => { it('error: the button is enabled and labeled "Retry"', () => { renderFooter({ status: 'error' }); - const button = screen.getByTestId('diary-load-more-button'); + const button = screen.getByTestId('infinite-scroll-load-more-button'); expect(button).toBeEnabled(); expect(button).toHaveTextContent('Retry'); }); @@ -97,7 +99,7 @@ describe('InfiniteScrollFooter', () => { const user = userEvent.setup(); const { onLoadMore, onRetry } = renderFooter({ status: 'error' }); - await user.click(screen.getByTestId('diary-load-more-button')); + await user.click(screen.getByTestId('infinite-scroll-load-more-button')); expect(onRetry).toHaveBeenCalledTimes(1); expect(onLoadMore).not.toHaveBeenCalled(); @@ -106,15 +108,15 @@ describe('InfiniteScrollFooter', () => { // ─── done ──────────────────────────────────────────────────────────────────── it('done: the button is absent', () => { - renderFooter({ status: 'done', hasMore: false }); + renderFooter({ status: 'done' }); - expect(screen.queryByTestId('diary-load-more-button')).not.toBeInTheDocument(); + expect(screen.queryByTestId('infinite-scroll-load-more-button')).not.toBeInTheDocument(); }); it('done: renders the end-of-list row with the exact copy and testid', () => { - renderFooter({ status: 'done', hasMore: false }); + renderFooter({ status: 'done' }); - const endOfList = screen.getByTestId('diary-end-of-list'); + const endOfList = screen.getByTestId('infinite-scroll-end-of-list'); expect(endOfList).toHaveTextContent("You've reached the end — no more entries to load."); }); @@ -125,4 +127,31 @@ describe('InfiniteScrollFooter', () => { expect(screen.getByTestId('infinite-scroll-sentinel')).toHaveClass(styles.sentinel!); }); + + // ─── testIdPrefix ──────────────────────────────────────────────────────────── + + describe('testIdPrefix', () => { + it('defaults to "infinite-scroll" when omitted', () => { + renderFooter({ status: 'idle' }); + + expect(screen.getByTestId('infinite-scroll-footer')).toBeInTheDocument(); + expect(screen.getByTestId('infinite-scroll-sentinel')).toBeInTheDocument(); + expect(screen.getByTestId('infinite-scroll-load-more-button')).toBeInTheDocument(); + + renderFooter({ status: 'done' }); + expect(screen.getByTestId('infinite-scroll-end-of-list')).toBeInTheDocument(); + }); + + it('applies a custom prefix to all four testids, proving the component is reusable by a second consumer', () => { + renderFooter({ status: 'idle', testIdPrefix: 'widget' }); + + expect(screen.getByTestId('widget-footer')).toBeInTheDocument(); + expect(screen.getByTestId('widget-sentinel')).toBeInTheDocument(); + expect(screen.getByTestId('widget-load-more-button')).toBeInTheDocument(); + expect(screen.queryByTestId('infinite-scroll-footer')).not.toBeInTheDocument(); + + renderFooter({ status: 'done', testIdPrefix: 'widget' }); + expect(screen.getByTestId('widget-end-of-list')).toBeInTheDocument(); + }); + }); }); diff --git a/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.tsx b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.tsx index 9c5c6fc27..9dc51aef0 100644 --- a/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.tsx +++ b/client/src/components/InfiniteScrollFooter/InfiniteScrollFooter.tsx @@ -1,43 +1,49 @@ -import { useTranslation } from 'react-i18next'; +import type { InfiniteScrollStatus } from '../../hooks/useInfiniteScroll.js'; import { Spinner } from '../Spinner/Spinner.js'; import { FormError } from '../FormError/FormError.js'; import shared from '../../styles/shared.module.css'; import styles from './InfiniteScrollFooter.module.css'; export interface InfiniteScrollFooterProps { - status: 'idle' | 'loading' | 'error' | 'done'; - hasMore: boolean; + status: InfiniteScrollStatus; + loadingLabel: string; + loadingAriaLabel: string; + loadMoreLabel: string; + retryLabel: string; + errorMessage: string; + endOfListMessage: string; sentinelRef: (node: HTMLDivElement | null) => void; onLoadMore: () => void; onRetry: () => void; + /** Prefix for all data-testid attributes. Defaults to 'infinite-scroll'. */ + testIdPrefix?: string; } export function InfiniteScrollFooter({ status, + loadingLabel, + loadingAriaLabel, + loadMoreLabel, + retryLabel, + errorMessage, + endOfListMessage, sentinelRef, onLoadMore, onRetry, + testIdPrefix = 'infinite-scroll', }: InfiniteScrollFooterProps) { - const { t } = useTranslation(); - return ( -
+