feat(documents): paginate /documents search so first paint isn't 1500 rows #315

Closed
opened 2026-04-23 22:40:42 +02:00 by marcel · 6 comments
Owner

Context

/documents (the home page) calls GET /api/documents/search which today returns every matching row in a single response — ~1500 documents with no page/size support. The backend additionally runs full enrichment (FTS match snippets, completion stats, contributor stacks) on every row before responding, and the frontend renders all 1500 DocumentRow components on first paint with no virtualization. Initial load is visibly slow.

The fastest win is classical offset pagination — server returns one page at a time, enrichment happens only for that page, and the user navigates via URL params (?page=). This matches the convention already in place for GET /api/notifications (the only other paged endpoint in the project).

  • Default page size 50, max 100.
  • URL param: ?page=0 (0-indexed, matches Spring Data PageRequest).
  • Existing filters (q, from, to, senderId, receiverId, tag[], tagOp, tagQ, sort, dir) are preserved unchanged; page is an orthogonal param.
  • Any filter change resets page to 0 on the frontend.
  • The year-group / sender-group / receiver-group headers keep working — they just group within the current page. Per-year total counts are dropped for now (they'd require a separate aggregate endpoint; out of scope).

Not pursued:

  • Infinite scroll / intersection-observer: nicer UX but larger surface — requires a scroll sentinel, virtualization for large groups, and non-shareable state. Ship classic pagination first, upgrade later if needed.
  • Cursor pagination: only pays off past ~100k rows; 1500 rows doesn't justify the complexity.

Backend changes

DocumentController.search (lines 243–263)

Add two query params:

@RequestParam(defaultValue = "0") @Min(0) int page,
@RequestParam(defaultValue = "50") @Min(1) @Max(100) int size,

Switch the return type from ResponseEntity<DocumentSearchResult> to ResponseEntity<PagedDocumentSearchResult> (a thin wrapper with content, totalElements, totalPages, number, size — matching the Spring Page<T> shape emitted for notifications so the openapi-typescript consumer already understands it).

DocumentService.searchDocuments (lines 358–406)

Current flow returns a full List<DocumentSearchItem> + enrichment. New flow, minimal churn:

  1. FTS / Spec filter — unchanged (returns the full ID set or ranked IDs).
  2. Sort:
    • DATE / TITLE / UPLOAD_DATE / RELEVANCE — push sort + PageRequest into the repository call (findAll(Specification, Pageable)).
    • SENDER / RECEIVER — keep the in-memory sort (it handles null parties) but page-slice the sorted list with subList(page*size, min((page+1)*size, total)).
  3. Enrich only the current pagebuildResult runs searchMatchData, completion stats, and contributors against the 50-item slice, not the full 1500.
  4. Return the PagedDocumentSearchResult.

Why this ordering wins: the hot path (DATE sort, default) now skips loading every row into memory AND skips enrichment on rows the user will never see on this page. Even the slow-path in-memory sorts (SENDER/RECEIVER) get cheap enrichment.

DocumentRepository

The existing findAll(Specification<Document> spec, Pageable pageable) is already inherited from JpaSpecificationExecutor — no repo change needed for the fast path.

Tests

  • DocumentControllerTest: assert that ?page=0&size=50 returns 50 items and totalElements equals the full match count.
  • DocumentControllerTest: assert that ?page=2&size=50 returns items 100–149.
  • DocumentServiceTest: lock in that enrichment is only called for page items (verify mock call counts).
  • DocumentServiceTest: SENDER-sort + page=1 returns the correct slice of a 120-doc fixture.

Frontend changes

frontend/src/routes/documents/+page.server.ts

  • Read page from the URL (Number(url.searchParams.get('page') ?? '0'), clamp to ≥ 0).
  • Pass page + a constant size = 50 to api.GET('/api/documents/search', ...).
  • Return { items, total, totalPages, page, size, ...existingFilters }.

frontend/src/routes/documents/+page.svelte

  • Pass totalPages + page to a new <Pagination> control rendered under DocumentList.
  • In the existing triggerSearch() function that builds URL params, reset page=0 whenever any filter changes.

frontend/src/lib/components/Pagination.svelte (new, ~40 lines)

  • Props: page: number, totalPages: number, makeHref: (p: number) => string.
  • Renders: « Zurück | Seite X von Y | Weiter » — three links. Prev/Next disabled at the bounds.
  • 44px touch targets (senior-accessibility floor), aria-label on each link, aria-current="page" on the current number.
  • i18n: add pagination_prev / pagination_next / pagination_page_of Paraglide keys in de/en/es.

DocumentList.svelte

  • No structural change. The component already renders whatever items it receives — it doesn't know (or care) that the slice is paginated. The empty-state message stays sensible.
  • Drop the "N Briefe" count in group headers for now (each group only sees current-page items). Keep the top-level total count in the status line since the backend still returns totalElements.

i18n

Add to frontend/messages/{de,en,es}.json:

pagination_prev      → « Zurück / « Previous / « Anterior
pagination_next      → Weiter » / Next » / Siguiente »
pagination_page_of   → Seite {page} von {total} / Page {page} of {total} / Página {page} de {total}

Tests

  • frontend/src/lib/components/Pagination.svelte.spec.ts: renders prev disabled at page 0, next disabled at page === totalPages-1, href builder called with expected numbers, "Page 3 of 10" text present.
  • Update the existing documents page.svelte.spec (if present) to mount at a non-zero page and assert the pagination footer rendered.

OpenAPI regeneration

Run npm run generate:api in frontend/ after the backend response shape changes so DocumentSearchResultPagedDocumentSearchResult propagates to the typed client.


Critical files

Backend

  • backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java — add params, new return type
  • backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java — paginate + enrich slice only
  • backend/src/main/java/org/raddatz/familienarchiv/dto/PagedDocumentSearchResult.javanew (or reuse Spring Page<T>)
  • backend/src/test/java/org/raddatz/familienarchiv/controller/DocumentControllerTest.java
  • backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceTest.java

Frontend

  • frontend/src/routes/documents/+page.server.ts — read page param, pass through
  • frontend/src/routes/documents/+page.svelte — wire pagination component
  • frontend/src/lib/components/Pagination.sveltenew
  • frontend/src/lib/components/Pagination.svelte.spec.tsnew
  • frontend/src/routes/DocumentList.svelte — drop per-group counts
  • frontend/messages/{de,en,es}.json — three new keys
  • frontend/src/lib/generated/api.ts — regenerated

Verification

  1. Backend unit + slice tests:
    cd backend && ./mvnw test -Dtest=DocumentControllerTest
    cd backend && ./mvnw test -Dtest=DocumentServiceTest
    
  2. Integration smoke: with the stack running, curl 'http://localhost:8080/api/documents/search?size=50&page=0' returns 50 items and totalElements ≥ 1500; curl '…?page=29' returns the tail slice.
  3. Frontend component test: cd frontend && npx vitest run src/lib/components/Pagination.svelte.spec.ts.
  4. Manual smoke:
    • Load /documents, confirm first paint is ~50 rows not 1500. Measure with DevTools Performance (TTI < 1s on localhost).
    • Click "Weiter »", confirm URL updates to ?page=1 and the second slice renders.
    • Change the search query — confirm it resets to page 0.
    • Open /documents?q=foo&page=5 in a new tab — state is driven from the URL.
  5. Visual smoke: pagination control renders at mobile (375px), tablet, desktop without layout break. Touch target ≥ 44px.

Acceptance criteria

  • /documents first paint shows ≤ 50 rows regardless of how many match.
  • ?page=N preserves every existing filter and is shareable via URL.
  • Changing any filter resets to page 0.
  • Backend enrichment runs only for the current-page slice (verified by mock call counts in DocumentServiceTest).
  • i18n covers de/en/es for the three new pagination strings.
  • No regression in DocumentControllerTest / DocumentServiceTest / page.svelte.spec / a11y sweep.
## Context `/documents` (the home page) calls `GET /api/documents/search` which today returns **every matching row** in a single response — ~1500 documents with no `page`/`size` support. The backend additionally runs full enrichment (FTS match snippets, completion stats, contributor stacks) on every row before responding, and the frontend renders all 1500 `DocumentRow` components on first paint with no virtualization. Initial load is visibly slow. The fastest win is classical offset pagination — server returns one page at a time, enrichment happens only for that page, and the user navigates via URL params (`?page=`). This matches the convention already in place for `GET /api/notifications` (the only other paged endpoint in the project). ## Recommended approach — classic page/size pagination - Default page size **50**, max **100**. - URL param: `?page=0` (0-indexed, matches Spring Data `PageRequest`). - Existing filters (`q`, `from`, `to`, `senderId`, `receiverId`, `tag[]`, `tagOp`, `tagQ`, `sort`, `dir`) are preserved unchanged; `page` is an orthogonal param. - Any filter change resets `page` to 0 on the frontend. - The year-group / sender-group / receiver-group headers keep working — they just group *within the current page*. Per-year total counts are dropped for now (they'd require a separate aggregate endpoint; out of scope). Not pursued: - **Infinite scroll / intersection-observer**: nicer UX but larger surface — requires a scroll sentinel, virtualization for large groups, and non-shareable state. Ship classic pagination first, upgrade later if needed. - **Cursor pagination**: only pays off past ~100k rows; 1500 rows doesn't justify the complexity. --- ## Backend changes ### `DocumentController.search` (lines 243–263) Add two query params: ```java @RequestParam(defaultValue = "0") @Min(0) int page, @RequestParam(defaultValue = "50") @Min(1) @Max(100) int size, ``` Switch the return type from `ResponseEntity<DocumentSearchResult>` to `ResponseEntity<PagedDocumentSearchResult>` (a thin wrapper with `content`, `totalElements`, `totalPages`, `number`, `size` — matching the Spring `Page<T>` shape emitted for notifications so the openapi-typescript consumer already understands it). ### `DocumentService.searchDocuments` (lines 358–406) Current flow returns a full `List<DocumentSearchItem>` + enrichment. New flow, minimal churn: 1. **FTS / Spec filter** — unchanged (returns the full ID set or ranked IDs). 2. **Sort**: - `DATE` / `TITLE` / `UPLOAD_DATE` / `RELEVANCE` — push sort + `PageRequest` into the repository call (`findAll(Specification, Pageable)`). - `SENDER` / `RECEIVER` — keep the in-memory sort (it handles null parties) but page-slice the sorted list with `subList(page*size, min((page+1)*size, total))`. 3. **Enrich only the current page** — `buildResult` runs `searchMatchData`, completion stats, and contributors against the 50-item slice, not the full 1500. 4. Return the `PagedDocumentSearchResult`. **Why this ordering wins**: the hot path (DATE sort, default) now skips loading every row into memory AND skips enrichment on rows the user will never see on this page. Even the slow-path in-memory sorts (SENDER/RECEIVER) get cheap enrichment. ### `DocumentRepository` The existing `findAll(Specification<Document> spec, Pageable pageable)` is already inherited from `JpaSpecificationExecutor` — no repo change needed for the fast path. ### Tests - `DocumentControllerTest`: assert that `?page=0&size=50` returns 50 items and `totalElements` equals the full match count. - `DocumentControllerTest`: assert that `?page=2&size=50` returns items 100–149. - `DocumentServiceTest`: lock in that enrichment is only called for page items (verify mock call counts). - `DocumentServiceTest`: SENDER-sort + page=1 returns the correct slice of a 120-doc fixture. --- ## Frontend changes ### `frontend/src/routes/documents/+page.server.ts` - Read `page` from the URL (`Number(url.searchParams.get('page') ?? '0')`, clamp to ≥ 0). - Pass `page` + a constant `size = 50` to `api.GET('/api/documents/search', ...)`. - Return `{ items, total, totalPages, page, size, ...existingFilters }`. ### `frontend/src/routes/documents/+page.svelte` - Pass `totalPages` + `page` to a new `<Pagination>` control rendered under `DocumentList`. - In the existing `triggerSearch()` function that builds URL params, **reset `page=0` whenever any filter changes**. ### `frontend/src/lib/components/Pagination.svelte` (new, ~40 lines) - Props: `page: number`, `totalPages: number`, `makeHref: (p: number) => string`. - Renders: `« Zurück | Seite X von Y | Weiter »` — three links. Prev/Next disabled at the bounds. - 44px touch targets (senior-accessibility floor), `aria-label` on each link, `aria-current="page"` on the current number. - i18n: add `pagination_prev` / `pagination_next` / `pagination_page_of` Paraglide keys in de/en/es. ### `DocumentList.svelte` - No structural change. The component already renders whatever `items` it receives — it doesn't know (or care) that the slice is paginated. The empty-state message stays sensible. - Drop the "N Briefe" count in group headers for now (each group only sees current-page items). Keep the top-level total count in the status line since the backend still returns `totalElements`. ### i18n Add to `frontend/messages/{de,en,es}.json`: ``` pagination_prev → « Zurück / « Previous / « Anterior pagination_next → Weiter » / Next » / Siguiente » pagination_page_of → Seite {page} von {total} / Page {page} of {total} / Página {page} de {total} ``` ### Tests - `frontend/src/lib/components/Pagination.svelte.spec.ts`: renders prev disabled at page 0, next disabled at page === totalPages-1, href builder called with expected numbers, "Page 3 of 10" text present. - Update the existing documents page.svelte.spec (if present) to mount at a non-zero page and assert the pagination footer rendered. --- ## OpenAPI regeneration Run `npm run generate:api` in `frontend/` after the backend response shape changes so `DocumentSearchResult` → `PagedDocumentSearchResult` propagates to the typed client. --- ## Critical files **Backend** - `backend/src/main/java/org/raddatz/familienarchiv/controller/DocumentController.java` — add params, new return type - `backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java` — paginate + enrich slice only - `backend/src/main/java/org/raddatz/familienarchiv/dto/PagedDocumentSearchResult.java` — **new** (or reuse Spring `Page<T>`) - `backend/src/test/java/org/raddatz/familienarchiv/controller/DocumentControllerTest.java` - `backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceTest.java` **Frontend** - `frontend/src/routes/documents/+page.server.ts` — read `page` param, pass through - `frontend/src/routes/documents/+page.svelte` — wire pagination component - `frontend/src/lib/components/Pagination.svelte` — **new** - `frontend/src/lib/components/Pagination.svelte.spec.ts` — **new** - `frontend/src/routes/DocumentList.svelte` — drop per-group counts - `frontend/messages/{de,en,es}.json` — three new keys - `frontend/src/lib/generated/api.ts` — regenerated --- ## Verification 1. **Backend unit + slice tests**: ```bash cd backend && ./mvnw test -Dtest=DocumentControllerTest cd backend && ./mvnw test -Dtest=DocumentServiceTest ``` 2. **Integration smoke**: with the stack running, `curl 'http://localhost:8080/api/documents/search?size=50&page=0'` returns 50 items and `totalElements` ≥ 1500; `curl '…?page=29'` returns the tail slice. 3. **Frontend component test**: `cd frontend && npx vitest run src/lib/components/Pagination.svelte.spec.ts`. 4. **Manual smoke**: - Load `/documents`, confirm first paint is ~50 rows not 1500. Measure with DevTools Performance (TTI < 1s on localhost). - Click "Weiter »", confirm URL updates to `?page=1` and the second slice renders. - Change the search query — confirm it resets to page 0. - Open `/documents?q=foo&page=5` in a new tab — state is driven from the URL. 5. **Visual smoke**: pagination control renders at mobile (375px), tablet, desktop without layout break. Touch target ≥ 44px. --- ## Acceptance criteria - `/documents` first paint shows ≤ 50 rows regardless of how many match. - `?page=N` preserves every existing filter and is shareable via URL. - Changing any filter resets to page 0. - Backend enrichment runs only for the current-page slice (verified by mock call counts in `DocumentServiceTest`). - i18n covers de/en/es for the three new pagination strings. - No regression in `DocumentControllerTest` / `DocumentServiceTest` / `page.svelte.spec` / a11y sweep.
marcel added the featureui labels 2026-04-23 22:40:48 +02:00
Author
Owner

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

  • The issue reads like a design doc, not a ticket — that's what I want to see. TDD plan is concrete.
  • One scope-correctness note: the AC says "Drop the 'N Briefe' count in group headers." Looking at frontend/src/routes/DocumentList.svelte:112-117, the group header already only renders group.label — there is no per-group count to drop. The top-level count at line 94 (m.docs_result_count({ count: total })) is the only one, and it should stay backed by totalElements. Just remove that AC line; nothing to drop.
  • NotificationController.java:49-58 already uses Page<NotificationDTO> directly as the return type. The issue recommends a dedicated PagedDocumentSearchResult DTO instead — agreed, that's the safer call. Raw Spring Page<T> leaks pageable.sort.sorted/unsorted and pageable.unpaged fields into the TypeScript types, which openapi-typescript renders as Record<string, never> noise and diverges from what DocumentSearchResult consumers already expect.

Recommendations

  • TDD order, red/green each step. Controller first:
    1. search_returns_50_items_and_totalElements_when_page0_size50_given_full_fixture
    2. search_returns_empty_content_with_correct_totalElements_when_page_beyond_last
    3. search_rejects_size_101 (400) — this is what @Valid on the param annotation buys you.
      Then service:
    4. searchDocuments_enrichesOnlyCurrentPageSlice — verify searchMatchData mock called with 50 ids, not 1500. This is the perf invariant; lock it.
    5. searchDocuments_SENDER_sort_page1_returnsCorrectSlice_of120DocFixture.
  • Naming: PagedDocumentSearchResult vs. just adding fields to DocumentSearchResult. I'd pick the latter — add pageNumber, pageSize, totalPages next to the existing items/total. One record type, one codegen shape, no downstream renames. DocumentSearchResult already carries total (line 11), which is the same as totalElements — rename it to totalElements for Spring-Page parity and you're done.
  • Guard clause for the in-memory slice. In searchDocuments, after sortBySender / sortByFirstReceiver, you need one line: List<Document> slice = sorted.subList(Math.min(page*size, sorted.size()), Math.min((page+1)*size, sorted.size())); — extract into a private pageSlice(List<T>, int, int) helper so the edge math lives once. Tested once in a utility test, reused twice.
  • Frontend: triggerSearch change is one line. Inside triggerSearch() (+page.svelte:38-51), simply do not carry page into the new params object. Any filter call already rebuilds from local state (q, from, to, etc.) and page is NOT in that set — so you're implicitly at page 0 already. But do add a page param when the Pagination component calls goto.

Open Decisions

(none — the shape is clear and the one genuine ambiguity was the DTO naming, which I've recommended above)

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - The issue reads like a design doc, not a ticket — that's what I want to see. TDD plan is concrete. - One scope-correctness note: the AC says "Drop the 'N Briefe' count in group headers." Looking at `frontend/src/routes/DocumentList.svelte:112-117`, the group header already only renders `group.label` — there is no per-group count to drop. The top-level count at line 94 (`m.docs_result_count({ count: total })`) is the only one, and it should stay backed by `totalElements`. Just remove that AC line; nothing to drop. - `NotificationController.java:49-58` already uses `Page<NotificationDTO>` directly as the return type. The issue recommends a dedicated `PagedDocumentSearchResult` DTO instead — agreed, that's the safer call. Raw Spring `Page<T>` leaks `pageable.sort.sorted/unsorted` and `pageable.unpaged` fields into the TypeScript types, which openapi-typescript renders as `Record<string, never>` noise and diverges from what `DocumentSearchResult` consumers already expect. ### Recommendations - **TDD order, red/green each step.** Controller first: 1. `search_returns_50_items_and_totalElements_when_page0_size50_given_full_fixture` 2. `search_returns_empty_content_with_correct_totalElements_when_page_beyond_last` 3. `search_rejects_size_101` (400) — this is what `@Valid` on the param annotation buys you. Then service: 4. `searchDocuments_enrichesOnlyCurrentPageSlice` — verify `searchMatchData` mock called with 50 ids, not 1500. This is the perf invariant; lock it. 5. `searchDocuments_SENDER_sort_page1_returnsCorrectSlice_of120DocFixture`. - **Naming: `PagedDocumentSearchResult` vs. just adding fields to `DocumentSearchResult`.** I'd pick the latter — add `pageNumber`, `pageSize`, `totalPages` next to the existing `items`/`total`. One record type, one codegen shape, no downstream renames. `DocumentSearchResult` already carries `total` (line 11), which is the same as `totalElements` — rename it to `totalElements` for Spring-Page parity and you're done. - **Guard clause for the in-memory slice.** In `searchDocuments`, after `sortBySender` / `sortByFirstReceiver`, you need one line: `List<Document> slice = sorted.subList(Math.min(page*size, sorted.size()), Math.min((page+1)*size, sorted.size()));` — extract into a private `pageSlice(List<T>, int, int)` helper so the edge math lives once. Tested once in a utility test, reused twice. - **Frontend: `triggerSearch` change is one line.** Inside `triggerSearch()` (+page.svelte:38-51), simply do not carry `page` into the new params object. Any filter call already rebuilds from local state (`q`, `from`, `to`, etc.) and `page` is NOT in that set — so you're implicitly at page 0 already. But **do** add a `page` param when the Pagination component calls `goto`. ### Open Decisions _(none — the shape is clear and the one genuine ambiguity was the DTO naming, which I've recommended above)_
Author
Owner

🏛️ Markus Keller — Senior Application Architect

Observations

  • Classic offset pagination is the right boring choice for 1500 rows. Cursor pagination's sweet spot starts at ~100k rows with deep-paging attacks or write-heavy timelines — neither applies here. Good call on rejecting it.
  • Shape is consistent with NotificationController.java:50 which already returns Page<NotificationDTO>. Convention established; extending it doesn't require any new architectural concept.
  • DocumentRepository already inherits from JpaSpecificationExecutor so findAll(Specification, Pageable) is free — no repo changes. Confirmed by reading DocumentService.searchDocuments at backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java:358-406.

Recommendations

  • Pass Pageable into the service method, not int page, int size. searchDocuments(..., Pageable pageable) is the Spring idiom — it bundles page/size/sort and lets you pass a PageImpl through the in-memory sort paths. Then the controller does PageRequest.of(page, size) at the edge, same pattern as NotificationController.java:57.
  • Return Page<DocumentSearchItem> internally, map to a domain DTO at the controller boundary. Same pattern as NotificationService.getNotifications (returns Page<NotificationDTO>) + a thin controller that just relays it. Keeps the service's return type honest about the pagination contract.
  • Scaling ceiling worth noting in the code comment. The SENDER/RECEIVER in-memory sort already flagged at DocumentService.java:378-380 is a memory trap past ~10k docs (loads all rows, sorts, slices). At 1500 rows, not a concern. At 15k it starts to bite. I'd add one comment line: "In-memory sort cost scales linearly with match count; acceptable while documents stays under ~10k. Past that, replace with SQL-level LEFT JOIN sort."
  • No ADR needed. Adding query params + a DTO wrapper is not an architectural decision — it's plumbing within an existing convention.

Open Decisions

(none)

## 🏛️ Markus Keller — Senior Application Architect ### Observations - Classic offset pagination is the right boring choice for 1500 rows. Cursor pagination's sweet spot starts at ~100k rows with deep-paging attacks or write-heavy timelines — neither applies here. Good call on rejecting it. - Shape is consistent with `NotificationController.java:50` which already returns `Page<NotificationDTO>`. Convention established; extending it doesn't require any new architectural concept. - `DocumentRepository` already inherits from `JpaSpecificationExecutor` so `findAll(Specification, Pageable)` is free — no repo changes. Confirmed by reading `DocumentService.searchDocuments` at `backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java:358-406`. ### Recommendations - **Pass `Pageable` into the service method, not `int page, int size`.** `searchDocuments(..., Pageable pageable)` is the Spring idiom — it bundles page/size/sort and lets you pass a `PageImpl` through the in-memory sort paths. Then the controller does `PageRequest.of(page, size)` at the edge, same pattern as `NotificationController.java:57`. - **Return `Page<DocumentSearchItem>` internally, map to a domain DTO at the controller boundary.** Same pattern as `NotificationService.getNotifications` (returns `Page<NotificationDTO>`) + a thin controller that just relays it. Keeps the service's return type honest about the pagination contract. - **Scaling ceiling worth noting in the code comment.** The SENDER/RECEIVER in-memory sort already flagged at `DocumentService.java:378-380` is a memory trap past ~10k docs (loads all rows, sorts, slices). At 1500 rows, not a concern. At 15k it starts to bite. I'd add one comment line: "In-memory sort cost scales linearly with match count; acceptable while `documents` stays under ~10k. Past that, replace with SQL-level LEFT JOIN sort." - **No ADR needed.** Adding query params + a DTO wrapper is not an architectural decision — it's plumbing within an existing convention. ### Open Decisions _(none)_
Author
Owner

🔒 Nora Steiner — Application Security Engineer

Observations

  • Missing @Validated on DocumentController is a blocker, not a nit. The issue spec proposes @Min(0) @Max(100) int size, but these Jakarta Bean Validation annotations only fire when the controller class (or method) is annotated @Validated. NotificationController.java:32 has @Validated. DocumentController does not. Without it, a client sending ?size=999999 silently bypasses validation and the server happily processes all 1500 rows + enrichment — which is the exact DoS vector pagination is supposed to eliminate.
  • No new auth surface — pagination doesn't change visibility rules, only slicing.
  • Offset pagination itself isn't a DoS vector at this scale. With max size 100 and 1500 rows, worst-case enrichment load is 100 items/request. Safe.

Recommendations

  • Add @Validated to DocumentController at the class level, matching NotificationController.java:32. One-line change, enables every existing @Min/@Max/@Pattern across every endpoint. Add a controller-level test that ?size=101 returns 400 — without this test, someone could silently remove @Validated later and reopen the DoS window.
  • Bound page too, not just size. The issue has @Min(0) on page but no upper bound. A client sending ?page=2147483647 (INT_MAX) creates a PageRequest with offset = 2147483647 × 50 which overflows and findAll may produce undefined behavior (Hibernate may translate to negative OFFSET; postgres will fail). Either @Max(100000) or check page <= totalPages server-side. Low probability, zero cost to bound.
  • Log rejections. @Valid triggers a MethodArgumentNotValidException → 400 response. If the app's GlobalExceptionHandler doesn't already log these at WARN with the client IP, consider it — pattern-of-failure lookups on rejected size values reveal brute-force attempts.

Open Decisions

(none — the validation gap is a must-fix, not a choice)

## 🔒 Nora Steiner — Application Security Engineer ### Observations - **Missing `@Validated` on `DocumentController` is a blocker, not a nit.** The issue spec proposes `@Min(0) @Max(100) int size`, but these Jakarta Bean Validation annotations only fire when the controller class (or method) is annotated `@Validated`. `NotificationController.java:32` has `@Validated`. `DocumentController` does not. Without it, a client sending `?size=999999` silently bypasses validation and the server happily processes all 1500 rows + enrichment — which is the exact DoS vector pagination is supposed to eliminate. - No new auth surface — pagination doesn't change visibility rules, only slicing. - Offset pagination itself isn't a DoS vector at this scale. With max size 100 and 1500 rows, worst-case enrichment load is 100 items/request. Safe. ### Recommendations - **Add `@Validated` to `DocumentController`** at the class level, matching `NotificationController.java:32`. One-line change, enables every existing `@Min/@Max/@Pattern` across every endpoint. Add a controller-level test that `?size=101` returns 400 — without this test, someone could silently remove `@Validated` later and reopen the DoS window. - **Bound `page` too, not just `size`.** The issue has `@Min(0)` on page but no upper bound. A client sending `?page=2147483647` (INT_MAX) creates a `PageRequest` with offset = 2147483647 × 50 which overflows and `findAll` may produce undefined behavior (Hibernate may translate to negative OFFSET; postgres will fail). Either `@Max(100000)` or check `page <= totalPages` server-side. Low probability, zero cost to bound. - **Log rejections.** `@Valid` triggers a `MethodArgumentNotValidException` → 400 response. If the app's `GlobalExceptionHandler` doesn't already log these at WARN with the client IP, consider it — pattern-of-failure lookups on rejected `size` values reveal brute-force attempts. ### Open Decisions _(none — the validation gap is a must-fix, not a choice)_
Author
Owner

🧪 Sara Holt — QA Engineer

Observations

  • The test plan in the issue covers the happy-path cases and the perf invariant (enrichment mock call count). Good bones, a few gaps worth filling.
  • DocumentList.svelte group headers currently don't show per-group counts (per Felix's scope note), so there's no existing test to update there.
  • No integration test currently asserts "page beyond last returns empty content + correct totalElements" anywhere — this is a common JPA edge-case footgun and worth locking in once.

Recommendations

  • Backend DocumentControllerTest additions beyond the issue's four:

    1. search_returns_400_when_size_exceeds_max?size=101, expect 400. This is the test that guards @Validated from being silently removed.
    2. search_returns_400_when_page_negative?page=-1, expect 400.
    3. search_returns_empty_content_with_correct_totalElements_when_page_beyond_last?page=999&size=50 with a 30-item fixture. Expect 200, content: [], totalElements: 30. Spring's Page does this correctly; lock it.
    4. search_applies_page_and_size_to_filtered_query — combine ?q=foo&page=1&size=10 and assert the FTS ranked-ID slice is respected.
  • Backend DocumentServiceTest — lock the perf invariant Sara cares about most: "enrichment runs only for page items":

    • Use verify(repo, times(1)).findAll(any(Specification.class), any(Pageable.class)) — confirms the fast-path uses Pageable, not List<Document> findAll(Specification).
    • For SENDER/RECEIVER sort, verify buildResult is called with a 50-item list, not 1500.
  • Frontend Pagination.svelte.spec.ts — the issue lists bounds cases. I'd add:

    • Keyboard test: Tab reaches prev, Enter navigates — stable keyboard nav is a Leonie-shared concern but testable here.
    • aria-current="page" rendered on current page number.
    • Touch target assertion (toHaveClass('min-h-[44px]') or computed height ≥ 44px).
  • Frontend page test — add one case: "filter change resets page to 0." Mount ?q=foo&page=5, simulate q='bar' change, assert goto was called without page. Without this test, a later refactor could silently break filter-reset behaviour.

  • Regression safety on typegen. After npm run generate:api, re-run npm run check. If DocumentSearchResultPagedDocumentSearchResult renaming cascades into places I can't easily see, type errors will surface.

Open Decisions

(none)

## 🧪 Sara Holt — QA Engineer ### Observations - The test plan in the issue covers the happy-path cases and the perf invariant (enrichment mock call count). Good bones, a few gaps worth filling. - `DocumentList.svelte` group headers currently don't show per-group counts (per Felix's scope note), so there's no existing test to update there. - No integration test currently asserts "page beyond last returns empty content + correct totalElements" anywhere — this is a common JPA edge-case footgun and worth locking in once. ### Recommendations - **Backend `DocumentControllerTest` additions beyond the issue's four:** 1. `search_returns_400_when_size_exceeds_max` — `?size=101`, expect 400. This is the test that guards `@Validated` from being silently removed. 2. `search_returns_400_when_page_negative` — `?page=-1`, expect 400. 3. `search_returns_empty_content_with_correct_totalElements_when_page_beyond_last` — `?page=999&size=50` with a 30-item fixture. Expect 200, `content: []`, `totalElements: 30`. Spring's `Page` does this correctly; lock it. 4. `search_applies_page_and_size_to_filtered_query` — combine `?q=foo&page=1&size=10` and assert the FTS ranked-ID slice is respected. - **Backend `DocumentServiceTest`** — lock the perf invariant Sara cares about most: "enrichment runs only for page items": - Use `verify(repo, times(1)).findAll(any(Specification.class), any(Pageable.class))` — confirms the fast-path uses `Pageable`, not `List<Document> findAll(Specification)`. - For SENDER/RECEIVER sort, verify `buildResult` is called with a 50-item list, not 1500. - **Frontend `Pagination.svelte.spec.ts`** — the issue lists bounds cases. I'd add: - Keyboard test: `Tab` reaches prev, `Enter` navigates — stable keyboard nav is a Leonie-shared concern but testable here. - `aria-current="page"` rendered on current page number. - Touch target assertion (`toHaveClass('min-h-[44px]')` or computed height ≥ 44px). - **Frontend page test** — add one case: "filter change resets page to 0." Mount `?q=foo&page=5`, simulate `q='bar'` change, assert `goto` was called without `page`. Without this test, a later refactor could silently break filter-reset behaviour. - **Regression safety on typegen.** After `npm run generate:api`, re-run `npm run check`. If `DocumentSearchResult` → `PagedDocumentSearchResult` renaming cascades into places I can't easily see, type errors will surface. ### Open Decisions _(none)_
Author
Owner

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

  • Zero infrastructure change. No new env var, no new service, no new port, no Compose update. ✓
  • CI impact is nil — new backend tests run in the existing unit-test job, new frontend spec in the Vitest browser-mode job.
  • The change is net-positive for production cost: enrichment runs against ~50 rows instead of ~1500, so the per-request CPU footprint drops ~30×. First-paint latency improves for the user and JVM heap churn drops for the server.

Recommendations

  • Measure before and after. If a dashboard exists for /api/documents/search, capture p95 latency before merge and after. This is the kind of change that looks good in a one-line Grafana chart — "latency dropped from 1.4s p95 to 180ms p95 on 2026-04-24" is a useful data point for future capacity conversations.
  • Cache-Control for paged responses. /api/documents/search should NOT set Cache-Control: public — query results vary per user's permissions. private, max-age=0, must-revalidate (or no cache header at all) is the right posture. If the endpoint currently sets anything caching-related, confirm it's either absent or private.
  • No Caddy changes. The endpoint path and content type are unchanged; reverse-proxy config is unaffected.
  • Rate limiting. The project has RateLimitInterceptor registered (per the backend config package). If it isn't already applied to /api/documents/search, consider adding it — 50/min/user would be plenty for human browsing and would throttle any accidental paginator-loop bug on the client.

Open Decisions

(none)

## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations - Zero infrastructure change. No new env var, no new service, no new port, no Compose update. ✓ - CI impact is nil — new backend tests run in the existing unit-test job, new frontend spec in the Vitest browser-mode job. - The change is net-positive for production cost: enrichment runs against ~50 rows instead of ~1500, so the per-request CPU footprint drops ~30×. First-paint latency improves for the user and JVM heap churn drops for the server. ### Recommendations - **Measure before and after.** If a dashboard exists for `/api/documents/search`, capture p95 latency before merge and after. This is the kind of change that looks good in a one-line Grafana chart — "latency dropped from 1.4s p95 to 180ms p95 on 2026-04-24" is a useful data point for future capacity conversations. - **Cache-Control for paged responses.** `/api/documents/search` should NOT set `Cache-Control: public` — query results vary per user's permissions. `private, max-age=0, must-revalidate` (or no cache header at all) is the right posture. If the endpoint currently sets anything caching-related, confirm it's either absent or `private`. - **No Caddy changes.** The endpoint path and content type are unchanged; reverse-proxy config is unaffected. - **Rate limiting.** The project has `RateLimitInterceptor` registered (per the backend config package). If it isn't already applied to `/api/documents/search`, consider adding it — 50/min/user would be plenty for human browsing and would throttle any accidental paginator-loop bug on the client. ### Open Decisions _(none)_
Author
Owner

🎨 Leonie Voss — UX Designer & Accessibility Advocate

Observations

  • 44px touch targets and aria-current="page" are in the spec — exactly right for the senior audience floor. ✓
  • The existing triggerSearch() (+page.svelte:50) uses goto(url, { keepFocus: true, noScroll: true }). For filter changes this is right — the user's typing or tag-picking, no one wants a scroll jump. But for pagination clicks, the same option keeps the user stranded mid-list after the slice changes, which is disorienting especially for seniors.
  • "« Zurück | Seite X von Y | Weiter »" — the arrow glyphs are decorative, but the text labels already carry the semantics. If the «/» live inside the <a> text, screen readers read them; they should be wrapped in <span aria-hidden="true"> or dropped entirely.
  • At 320px width (small phone in bright daylight — the senior-constraint viewport), "Seite 15 von 47" + two button labels rarely fits horizontally. Expect wrapping.

Recommendations

  • Scroll to top on page change. In Pagination's makeHref (or wherever the pagination goto lives), use goto(url, { keepFocus: true }) — drop noScroll: true. SvelteKit's default scroll behavior will put the user at the top of the new page, which is what they expect. Keep noScroll: true ONLY in the existing filter-change triggerSearch path. Mental model: filter change = same results surface being narrowed, don't move me; page change = next surface, take me to the top.
  • aria-hidden on decorative chevrons.
    <a href={hrefFor(page - 1)} aria-label={m.pagination_prev()}>
        <span aria-hidden="true">«</span> {m.pagination_prev()}
    </a>
    
    Screen readers announce "Previous," not "« Previous."
  • Responsive stacking at narrow viewports. At < 480px, lay the control out as: prev button on left, next button on right (both 44px, flex-1), "Seite X von Y" centered on a line above the buttons. At ≥ 480px, inline them. A single sm:flex-row flex-col swap in Tailwind handles it.
  • Focus indicator on Pagination linksfocus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2. Links without visible focus leave keyboard users stranded.
  • Dark mode. The pagination control should work in both themes. Test axe-playwright in light and dark.
  • Future (not for this PR): for 47-page result sets, "Next/Prev only" means users can't jump from page 2 to page 40. A jump-to-page numeric input or "First/Last" buttons would help — but correct to keep out of this issue's scope. Worth filing as a follow-up once the core pagination is in place.

Open Decisions

  • Scroll-to-top vs. preserve-scroll on pagination navigation. I'm recommending scroll-to-top (senior UX, expected behavior). The existing triggerSearch uses noScroll: true for filter changes and that's correct there — this is specifically about the pagination-click path. If you feel strongly that scroll should be preserved (e.g. for power users who want to compare page N and page N+1 via quick toggle), say so — it's the one genuine UX tradeoff in this change.
## 🎨 Leonie Voss — UX Designer & Accessibility Advocate ### Observations - 44px touch targets and `aria-current="page"` are in the spec — exactly right for the senior audience floor. ✓ - The existing `triggerSearch()` (`+page.svelte:50`) uses `goto(url, { keepFocus: true, noScroll: true })`. For filter changes this is right — the user's typing or tag-picking, no one wants a scroll jump. But for **pagination** clicks, the same option keeps the user stranded mid-list after the slice changes, which is disorienting especially for seniors. - "« Zurück | Seite X von Y | Weiter »" — the arrow glyphs are decorative, but the text labels already carry the semantics. If the `«`/`»` live inside the `<a>` text, screen readers read them; they should be wrapped in `<span aria-hidden="true">` or dropped entirely. - At 320px width (small phone in bright daylight — the senior-constraint viewport), "Seite 15 von 47" + two button labels rarely fits horizontally. Expect wrapping. ### Recommendations - **Scroll to top on page change.** In Pagination's `makeHref` (or wherever the pagination `goto` lives), use `goto(url, { keepFocus: true })` — drop `noScroll: true`. SvelteKit's default scroll behavior will put the user at the top of the new page, which is what they expect. Keep `noScroll: true` ONLY in the existing filter-change `triggerSearch` path. Mental model: filter change = same results surface being narrowed, don't move me; page change = next surface, take me to the top. - **`aria-hidden` on decorative chevrons.** ```svelte <a href={hrefFor(page - 1)} aria-label={m.pagination_prev()}> <span aria-hidden="true">«</span> {m.pagination_prev()} </a> ``` Screen readers announce "Previous," not "« Previous." - **Responsive stacking at narrow viewports.** At `< 480px`, lay the control out as: prev button on left, next button on right (both 44px, flex-1), "Seite X von Y" **centered on a line above** the buttons. At ≥ 480px, inline them. A single `sm:flex-row flex-col` swap in Tailwind handles it. - **Focus indicator on Pagination links** — `focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2`. Links without visible focus leave keyboard users stranded. - **Dark mode.** The pagination control should work in both themes. Test `axe-playwright` in light and dark. - **Future (not for this PR):** for 47-page result sets, "Next/Prev only" means users can't jump from page 2 to page 40. A jump-to-page numeric input or "First/Last" buttons would help — but correct to keep out of this issue's scope. Worth filing as a follow-up once the core pagination is in place. ### Open Decisions - **Scroll-to-top vs. preserve-scroll on pagination navigation.** I'm recommending scroll-to-top (senior UX, expected behavior). The existing `triggerSearch` uses `noScroll: true` for filter changes and that's correct there — this is specifically about the pagination-click path. If you feel strongly that scroll should be preserved (e.g. for power users who want to compare page N and page N+1 via quick toggle), say so — it's the one genuine UX tradeoff in this change.
Sign in to join this conversation.
No Label feature ui
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/familienarchiv#315