marcel/familienarchiv
1
0
Fork 0
Code Issues 117 Pull Requests 1 Actions 1 Packages Projects Releases Wiki Activity

feat(chronik): add cursor/offset pagination to /api/dashboard/activity + wire "Mehr laden" #290

New Issue
Open
opened 2026-04-20 18:13:26 +02:00 by marcel · 7 comments
marcel commented 2026-04-20 18:13:26 +02:00
Owner
Copy Link

Background

Deferred from PR #288 during review cycle 1 (Markus Keller + Sara Holt).

The first iteration of /chronik shipped a "Mehr laden" button that called GET /api/dashboard/activity?limit=40&offset=N, but the backend DashboardController.getActivity only accepts limit — offset was silently ignored, causing every click to re-fetch and duplicate the top-40 rows. The UI was removed from PR #288 rather than ship broken behaviour.

Concern

No pagination past 40 activity items. The spec (docs/specs/chronik-spec.html §02 state 10) shows a "Mehr laden" button with an accompanying loading micro-state (3 static skeleton rows, aria-busy, focus preservation, aria-live="polite" announcement).

Scope

Backend

  • Add offset (or before: OffsetDateTime cursor) query param to DashboardController.getActivity.
  • Thread it through DashboardService.getActivity → AuditLogQueryService.findActivityFeed → AuditLogQueryRepository.findRolledUpActivityFeed.
  • Decide cursor vs offset: cursor (before=timestamp) is more robust against live inserts; offset is simpler. Recommend cursor since the feed is time-ordered DESC.
  • Integration test: insert 50 events, fetch first 40, assert next fetch returns items 40-49 without overlap.

Frontend

  • Reintroduce the Load-more UI in src/routes/chronik/+page.svelte:
    • <button> with aria-busy during fetch, focus preserved after tick().
    • Three static (non-shimmer) skeleton rows while loading.
    • aria-live="polite" region announces "{count} weitere Einträge geladen" (chronik_load_more_announcement key already exists).
    • Pass before (or offset) from the last row's happenedAt (or the current merged feed length).
  • New spec +page.svelte.spec.ts covering: append on success, aria-busy toggling, skeleton render, focus preservation, no-op when already loading.

Acceptance

  • Backend accepts paging param and returns non-overlapping batches.
  • Frontend Load-more button reliably fetches the next 40.
  • Loading micro-state matches spec §02 state 10.

Reference

## Background Deferred from PR #288 during review cycle 1 (Markus Keller + Sara Holt). The first iteration of `/chronik` shipped a "Mehr laden" button that called `GET /api/dashboard/activity?limit=40&offset=N`, but the backend `DashboardController.getActivity` only accepts `limit` — `offset` was silently ignored, causing every click to re-fetch and duplicate the top-40 rows. The UI was removed from PR #288 rather than ship broken behaviour. ## Concern No pagination past 40 activity items. The spec (`docs/specs/chronik-spec.html` §02 state 10) shows a "Mehr laden" button with an accompanying loading micro-state (3 static skeleton rows, `aria-busy`, focus preservation, `aria-live="polite"` announcement). ## Scope ### Backend - Add `offset` (or `before: OffsetDateTime` cursor) query param to `DashboardController.getActivity`. - Thread it through `DashboardService.getActivity` → `AuditLogQueryService.findActivityFeed` → `AuditLogQueryRepository.findRolledUpActivityFeed`. - Decide cursor vs offset: cursor (`before=timestamp`) is more robust against live inserts; offset is simpler. Recommend cursor since the feed is time-ordered DESC. - Integration test: insert 50 events, fetch first 40, assert next fetch returns items 40-49 without overlap. ### Frontend - Reintroduce the Load-more UI in `src/routes/chronik/+page.svelte`: - `<button>` with `aria-busy` during fetch, focus preserved after `tick()`. - Three static (non-shimmer) skeleton rows while loading. - `aria-live="polite"` region announces "{count} weitere Einträge geladen" (`chronik_load_more_announcement` key already exists). - Pass `before` (or `offset`) from the last row's `happenedAt` (or the current merged feed length). - New spec `+page.svelte.spec.ts` covering: append on success, aria-busy toggling, skeleton render, focus preservation, no-op when already loading. ### Acceptance - [ ] Backend accepts paging param and returns non-overlapping batches. - [ ] Frontend Load-more button reliably fetches the next 40. - [ ] Loading micro-state matches spec §02 state 10. ## Reference - PR: http://heim-nas:3005/marcel/familienarchiv/pulls/288 - Parent issue: #285 - Spec: `docs/specs/chronik-spec.html` §02 state 10
marcel commented 2026-04-20 19:05:23 +02:00
Author
Owner
Copy Link

🏛️ Markus Keller — Senior Application Architect

Observations

  • V49__add_audit_log_rollup_index.sql is (actor_id, document_id, kind, happened_at DESC) with a partial WHERE clause matching the rollup kinds. Cursor-based WHERE happened_at < :before queries walk it in index order with zero sort cost.
  • NotificationController.getNotifications at line 51–57 already uses the Spring Data page/size convention with PageRequest.of(...). Using the same pattern here would be consistent — but that pattern is offset-based under the hood and has the live-insert drift problem.
  • The rollup query's outer ORDER BY ag.happened_at DESC LIMIT :limit sits after the CTE aggregation. Adding WHERE ag.happened_at < :before to the final SELECT is the cleanest cursor insertion point — the CTEs don't need to change.
  • Session happened_at is MIN(s.happened_at) (start-of-session). Two sessions with the exact same start timestamp are possible in theory (two actors transcribe the same document in the same millisecond) but vanishingly unlikely in practice. Still: the composite cursor (happened_at, document_id, kind) is tie-break-safe.

Recommendations

  • Use cursor, not offset. The feed is strictly time-ordered DESC, new events arrive at the top, the covering index supports WHERE happened_at < :before ORDER BY happened_at DESC LIMIT :limit as a forward-only range scan. Offset forces a scan+skip that degrades with page depth. Cursor also survives live SSE inserts without skipping/duplicating rows.
  • Composite tie-break cursor: (beforeHappenedAt, beforeDocumentId, beforeKind). Controller accepts three query params; the repo WHERE becomes (happened_at, document_id, kind) < (:beforeHappenedAt, :beforeDocumentId, :beforeKind) using Postgres's row-comparison operator. Trivially supported, no ORDER BY change needed.
  • Keep the endpoint shape. Don't introduce Spring Pageable here — it buys nothing for cursor pagination and couples this controller to the page/size mental model the notifications controller uses. Plain @RequestParam with explicit names documents the contract clearly.
  • Frontend state is lastRowCursor, not offset or page number. After each fetch, store the (happenedAt, documentId, kind) of the last row. Next "Mehr laden" sends those three params.
  • End-of-feed signal: backend returns < limit rows when there's no more. Frontend hides the Load-more button. No need for a separate "totalCount" or hasNextPage field.

Open Decisions

  • Cursor encoding at the API level. Option A: three separate query params (beforeAt, beforeDocId, beforeKind) — explicit, easy to debug, compiles straight to @RequestParam. Option B: single opaque cursor param (base64-encoded tuple) — hides implementation detail, easier to evolve later, harder to tweak via curl. Recommend A for this codebase — it's a family archive, not a public API, and debuggability wins.
## 🏛️ Markus Keller — Senior Application Architect ### Observations - `V49__add_audit_log_rollup_index.sql` is `(actor_id, document_id, kind, happened_at DESC)` with a partial WHERE clause matching the rollup kinds. Cursor-based `WHERE happened_at < :before` queries walk it in index order with zero sort cost. - `NotificationController.getNotifications` at line 51–57 already uses the Spring Data `page`/`size` convention with `PageRequest.of(...)`. Using the same pattern here would be consistent — but that pattern is offset-based under the hood and has the live-insert drift problem. - The rollup query's outer `ORDER BY ag.happened_at DESC LIMIT :limit` sits *after* the CTE aggregation. Adding `WHERE ag.happened_at < :before` to the final SELECT is the cleanest cursor insertion point — the CTEs don't need to change. - Session `happened_at` is `MIN(s.happened_at)` (start-of-session). Two sessions with the exact same start timestamp are possible in theory (two actors transcribe the same document in the same millisecond) but vanishingly unlikely in practice. Still: the composite cursor `(happened_at, document_id, kind)` is tie-break-safe. ### Recommendations - **Use cursor, not offset.** The feed is strictly time-ordered DESC, new events arrive at the top, the covering index supports `WHERE happened_at < :before ORDER BY happened_at DESC LIMIT :limit` as a forward-only range scan. Offset forces a scan+skip that degrades with page depth. Cursor also survives live SSE inserts without skipping/duplicating rows. - **Composite tie-break cursor: `(beforeHappenedAt, beforeDocumentId, beforeKind)`.** Controller accepts three query params; the repo WHERE becomes `(happened_at, document_id, kind) < (:beforeHappenedAt, :beforeDocumentId, :beforeKind)` using Postgres's row-comparison operator. Trivially supported, no ORDER BY change needed. - **Keep the endpoint shape.** Don't introduce Spring `Pageable` here — it buys nothing for cursor pagination and couples this controller to the page/size mental model the notifications controller uses. Plain `@RequestParam` with explicit names documents the contract clearly. - **Frontend state is `lastRowCursor`, not `offset` or page number.** After each fetch, store the `(happenedAt, documentId, kind)` of the last row. Next "Mehr laden" sends those three params. - **End-of-feed signal:** backend returns `< limit` rows when there's no more. Frontend hides the Load-more button. No need for a separate "totalCount" or `hasNextPage` field. ### Open Decisions - **Cursor encoding at the API level.** Option A: three separate query params (`beforeAt`, `beforeDocId`, `beforeKind`) — explicit, easy to debug, compiles straight to `@RequestParam`. Option B: single opaque `cursor` param (base64-encoded tuple) — hides implementation detail, easier to evolve later, harder to tweak via curl. Recommend A for this codebase — it's a family archive, not a public API, and debuggability wins.
marcel commented 2026-04-20 19:05:43 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

  • The current findRolledUpActivityFeed is a 4-CTE pipeline that ends with ORDER BY ag.happened_at DESC LIMIT :limit. Adding a cursor clause means one new WHERE in the final SELECT and three new @Param bindings in the repo method — no other changes.
  • DashboardController.getActivity is Math.min(limit, 40) — that cap stays. The cursor just decides where the 40 starts.
  • ActivityFeedItemDTO already has happenedAt + documentId + kind fields — the frontend can build the cursor for the next request without new DTO surface.

Recommendations

  • Red tests before any implementation, in this order:
    1. rolledUpFeed_cursor_returns_rows_before_the_cursor_timestamp — insert 50 events, fetch first 40 without cursor, then fetch with cursor = last row's (happenedAt, documentId, kind), assert 10 non-overlapping rows.
    2. rolledUpFeed_cursor_returns_empty_when_no_older_rows — fetch with cursor set past the oldest row, assert List.of().
    3. rolledUpFeed_cursor_tie_break_is_stable — two sessions with the same happened_at on different documents. Cursor-with-docId must not skip or duplicate.
    4. Controller test: activity_clamps_limit_to_40 already exists; add activity_threads_before_params_through_to_service.
  • Frontend state with $state + $derived, no $effect loops. 
    let pages = $state<ActivityFeedItemDTO[][]>([[...data.activityFeed]]);
let isLoadingMore = $state(false);
let loadMoreBtn = $state<HTMLButtonElement | null>(null);

const flat = $derived(pages.flat());
const lastRow = $derived(flat.at(-1));
const atEnd = $state(false); // flipped when a fetch returns < 40
    loadMore() is an async function that:
    1. guards on isLoadingMore || atEnd
    2. sets isLoadingMore = true
    3. fetches with lastRow.happenedAt / documentId / kind query params
    4. on < 40 returned: sets atEnd = true
    5. after await tick(), loadMoreBtn?.focus()
  • Co-located +page.svelte.spec.ts with mocked fetch — assert:
    • Pressing Load-more sends the cursor params derived from the last row.
    • On < 40 response, the button is removed from the DOM.
    • Focus lands back on the button (or the next focusable if removed) after success.
    • aria-busy="true" while loading.
  • Skeleton rows: keep the data-testid="chronik-skeleton-row" attribute — reuse the pattern from the original #288 implementation. Three <li> with static height, no shimmer.
  • Singleton rule reminder: bind:this={loadMoreBtn} requires let loadMoreBtn = $state<HTMLButtonElement | null>(null). Forgetting the $state(...) wrapper is the bug that tripped me up in #288; the svelte linter will flag it but CI won't block on warnings — worth writing it correctly first time.

Open Decisions

  • None — the approach is mechanical once the cursor shape is decided (Markus covers that).
## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - The current `findRolledUpActivityFeed` is a 4-CTE pipeline that ends with `ORDER BY ag.happened_at DESC LIMIT :limit`. Adding a cursor clause means one new `WHERE` in the final SELECT and three new `@Param` bindings in the repo method — no other changes. - `DashboardController.getActivity` is `Math.min(limit, 40)` — that cap stays. The cursor just decides *where* the 40 starts. - `ActivityFeedItemDTO` already has `happenedAt` + `documentId` + `kind` fields — the frontend can build the cursor for the next request without new DTO surface. ### Recommendations - **Red tests before any implementation, in this order:** 1. `rolledUpFeed_cursor_returns_rows_before_the_cursor_timestamp` — insert 50 events, fetch first 40 without cursor, then fetch with cursor = last row's `(happenedAt, documentId, kind)`, assert 10 non-overlapping rows. 2. `rolledUpFeed_cursor_returns_empty_when_no_older_rows` — fetch with cursor set past the oldest row, assert `List.of()`. 3. `rolledUpFeed_cursor_tie_break_is_stable` — two sessions with the same `happened_at` on different documents. Cursor-with-docId must not skip or duplicate. 4. Controller test: `activity_clamps_limit_to_40` already exists; add `activity_threads_before_params_through_to_service`. - **Frontend state with `$state` + `$derived`, no `$effect` loops.** ```ts let pages = $state<ActivityFeedItemDTO[][]>([[...data.activityFeed]]); let isLoadingMore = $state(false); let loadMoreBtn = $state<HTMLButtonElement | null>(null); const flat = $derived(pages.flat()); const lastRow = $derived(flat.at(-1)); const atEnd = $state(false); // flipped when a fetch returns < 40 ``` `loadMore()` is an async function that: 1. guards on `isLoadingMore || atEnd` 2. sets `isLoadingMore = true` 3. fetches with `lastRow.happenedAt / documentId / kind` query params 4. on `< 40` returned: sets `atEnd = true` 5. after `await tick()`, `loadMoreBtn?.focus()` - **Co-located `+page.svelte.spec.ts`** with mocked `fetch` — assert: - Pressing Load-more sends the cursor params derived from the last row. - On < 40 response, the button is removed from the DOM. - Focus lands back on the button (or the next focusable if removed) after success. - `aria-busy="true"` while loading. - **Skeleton rows:** keep the `data-testid="chronik-skeleton-row"` attribute — reuse the pattern from the original #288 implementation. Three `<li>` with static height, no shimmer. - **Singleton rule reminder:** `bind:this={loadMoreBtn}` requires `let loadMoreBtn = $state<HTMLButtonElement | null>(null)`. Forgetting the `$state(...)` wrapper is the bug that tripped me up in #288; the svelte linter will flag it but CI won't block on warnings — worth writing it correctly first time. ### Open Decisions - None — the approach is mechanical once the cursor shape is decided (Markus covers that).
marcel commented 2026-04-20 19:05:58 +02:00
Author
Owner
Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

  • The rollup query is parameterized through @Param("currentUserId") + @Param("limit"). Adding @Param("beforeAt") / @Param("beforeDocId") / @Param("beforeKind") continues the pattern — no injection surface.
  • /api/dashboard/activity is gated by @RequirePermission(Permission.READ_ALL) at the DashboardController class level. Cursor params don't change the authorization story.
  • The youMentioned flag is computed from :currentUserId. It's a display hint, not an access control — any user with READ_ALL sees all rows with youMentioned=false for events that mention others. That's existing behavior, not a new concern from this issue.

Recommendations

  • Validate cursor kind against the AuditKind enum at the controller boundary. Spring's enum-as-query-param binding does this by default; just type the param as AuditKind (not String). If a caller sends ?beforeKind=<script>, Spring returns 400 before the query runs. 
    public List<ActivityFeedItemDTO> getActivity(
        @RequestParam(defaultValue = "7") int limit,
        @RequestParam(required = false) OffsetDateTime beforeAt,
        @RequestParam(required = false) UUID beforeDocId,
        @RequestParam(required = false) AuditKind beforeKind,
        Authentication auth) { ... }
    All three must be present together (or all absent). Reject inconsistent combinations with a 400 + structured error code (INVALID_CURSOR).
  • Rate-limit deep pagination. Cursor pagination over the full audit log is cheap per-request but an attacker with valid auth could walk the entire history to scrape. The existing rate limit on /api/dashboard/activity should cover this; confirm it's set conservatively (e.g., 60 req/min per user) and doesn't have a generous "API burst" bucket that lets a scraper drain the feed in minutes.
  • No happenedAt from untrusted sources ever reaches the query as a string. Using Spring's OffsetDateTime binding means malformed dates → 400 before SQL. Do not accept an epoch-millis integer alternative in the same endpoint — one format, one parser.
  • Cursor leakage is not a secret. Don't obfuscate or sign the cursor. The three values are public (returned as DTO fields already), and signing adds a key-management burden for zero benefit on a family archive.

Open Decisions

  • None.
## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - The rollup query is parameterized through `@Param("currentUserId")` + `@Param("limit")`. Adding `@Param("beforeAt")` / `@Param("beforeDocId")` / `@Param("beforeKind")` continues the pattern — no injection surface. - `/api/dashboard/activity` is gated by `@RequirePermission(Permission.READ_ALL)` at the `DashboardController` class level. Cursor params don't change the authorization story. - The `youMentioned` flag is computed from `:currentUserId`. It's a display hint, not an access control — any user with `READ_ALL` sees all rows with `youMentioned=false` for events that mention others. That's existing behavior, not a new concern from this issue. ### Recommendations - **Validate cursor kind against the `AuditKind` enum** at the controller boundary. Spring's enum-as-query-param binding does this by default; just type the param as `AuditKind` (not `String`). If a caller sends `?beforeKind=<script>`, Spring returns 400 before the query runs. ```java public List<ActivityFeedItemDTO> getActivity( @RequestParam(defaultValue = "7") int limit, @RequestParam(required = false) OffsetDateTime beforeAt, @RequestParam(required = false) UUID beforeDocId, @RequestParam(required = false) AuditKind beforeKind, Authentication auth) { ... } ``` All three must be present together (or all absent). Reject inconsistent combinations with a 400 + structured error code (`INVALID_CURSOR`). - **Rate-limit deep pagination.** Cursor pagination over the full audit log is cheap per-request but an attacker with valid auth could walk the entire history to scrape. The existing rate limit on `/api/dashboard/activity` should cover this; confirm it's set conservatively (e.g., 60 req/min per user) and doesn't have a generous "API burst" bucket that lets a scraper drain the feed in minutes. - **No `happenedAt` from untrusted sources ever reaches the query as a string.** Using Spring's `OffsetDateTime` binding means malformed dates → 400 before SQL. Do not accept an epoch-millis integer alternative in the same endpoint — one format, one parser. - **Cursor leakage is not a secret.** Don't obfuscate or sign the cursor. The three values are public (returned as DTO fields already), and signing adds a key-management burden for zero benefit on a family archive. ### Open Decisions - None.
marcel commented 2026-04-20 19:06:15 +02:00
Author
Owner
Copy Link

🧪 Sara Holt — Senior QA Engineer

Observations

  • AuditLogQueryRepositoryRolledUpTest already runs against real postgres:16-alpine via Testcontainers and has 6 rollup tests. Extending it with cursor cases inherits the right infrastructure — no new @Import needed.
  • +page.svelte currently has no co-located spec file (the chronik page composition was covered by component-level specs). This issue is the right time to add +page.svelte.spec.ts.
  • vitest-browser-svelte + vitest/browser are already in the project (see ChronikFuerDichBox.svelte.spec.ts) — load-more assertions like focus preservation work best in real browser mode.

Recommendations

  • Backend test fixture for 50+ rows without copy-paste. Reuse the helper shape I'd written into other tests: 
    private void insertManyAuditEvents(UUID actorId, UUID docId, String kind, Instant base, int count, int gapSeconds) {
    for (int i = 0; i < count; i++) {
        insertAuditEvent(actorId, docId, kind, base.plusSeconds((long) i * gapSeconds));
    }
}
    Test pagination by calling it with count=50, gapSeconds=120 so each event seeds its own session (>120 min gap not triggered, but cursor works on aggregated rows).
  • Edge-case test matrix:
    • Cursor is the very first row → returns 39 rows (one less than limit, since cursor is exclusive).
    • Cursor is past the oldest row → returns [].
    • Cursor references an event that is part of a rolled-up session (not a session start) → should still paginate cleanly because the cursor is applied post-aggregation to session-start timestamps.
    • Two sessions with the same happened_at on different (document_id, kind) → composite cursor disambiguates; neither row is duplicated or skipped across page boundary.
    • Deleted document mid-pagination: page 1 has doc X in the last row. Doc X is deleted. Page 2 fetched with cursor pointing at doc X's happened_at — since audit_log.document_id has ON DELETE CASCADE, the rows are gone entirely. The cursor still paginates the remaining rows correctly. Write this test to lock the behavior.
  • Frontend browser-mode test: 
    it('focuses the Load-more button after a successful fetch', async () => { ... });
it('removes the Load-more button when the response has < 40 items', async () => { ... });
it('sends before-cursor query params derived from the last row', async () => { ... });
it('is a no-op when already loading', async () => { ... });
it('sets aria-busy="true" during the fetch', async () => { ... });
  • Accessibility regression: add /chronik to the existing axe-playwright sweep once the Load-more button is back — the skeleton rows must also pass contrast checks when rendered.

Open Decisions

  • None.
## 🧪 Sara Holt — Senior QA Engineer ### Observations - `AuditLogQueryRepositoryRolledUpTest` already runs against real `postgres:16-alpine` via Testcontainers and has 6 rollup tests. Extending it with cursor cases inherits the right infrastructure — no new `@Import` needed. - `+page.svelte` currently has no co-located spec file (the chronik page composition was covered by component-level specs). This issue is the right time to add `+page.svelte.spec.ts`. - `vitest-browser-svelte` + `vitest/browser` are already in the project (see `ChronikFuerDichBox.svelte.spec.ts`) — load-more assertions like focus preservation work best in real browser mode. ### Recommendations - **Backend test fixture for 50+ rows** without copy-paste. Reuse the helper shape I'd written into other tests: ```java private void insertManyAuditEvents(UUID actorId, UUID docId, String kind, Instant base, int count, int gapSeconds) { for (int i = 0; i < count; i++) { insertAuditEvent(actorId, docId, kind, base.plusSeconds((long) i * gapSeconds)); } } ``` Test pagination by calling it with `count=50, gapSeconds=120` so each event seeds its own session (>120 min gap not triggered, but cursor works on aggregated rows). - **Edge-case test matrix:** - Cursor is the very first row → returns 39 rows (one less than limit, since cursor is exclusive). - Cursor is past the oldest row → returns `[]`. - Cursor references an event that is part of a rolled-up session (not a session start) → should still paginate cleanly because the cursor is applied *post-aggregation* to session-start timestamps. - Two sessions with the same `happened_at` on different `(document_id, kind)` → composite cursor disambiguates; neither row is duplicated or skipped across page boundary. - Deleted document mid-pagination: page 1 has doc X in the last row. Doc X is deleted. Page 2 fetched with cursor pointing at doc X's happened_at — since `audit_log.document_id` has `ON DELETE CASCADE`, the rows are gone entirely. The cursor still paginates the remaining rows correctly. Write this test to lock the behavior. - **Frontend browser-mode test:** ```ts it('focuses the Load-more button after a successful fetch', async () => { ... }); it('removes the Load-more button when the response has < 40 items', async () => { ... }); it('sends before-cursor query params derived from the last row', async () => { ... }); it('is a no-op when already loading', async () => { ... }); it('sets aria-busy="true" during the fetch', async () => { ... }); ``` - **Accessibility regression:** add `/chronik` to the existing axe-playwright sweep once the Load-more button is back — the skeleton rows must also pass contrast checks when rendered. ### Open Decisions - None.
marcel commented 2026-04-20 19:06:30 +02:00
Author
Owner
Copy Link

🏗️ Tobias Wendt — DevOps & Platform Engineer

Observations

  • V49 partial covering index is (actor_id, document_id, kind, happened_at DESC) WHERE kind IN (...). Cursor queries using WHERE happened_at < :before hit the index on the happened_at DESC component directly — index-only range scan, no heap fetch for the tuple comparison.
  • Offset pagination against the same index forces Postgres to materialize and skip — cost grows linearly with offset. At depth 1000 the difference is measurable; at depth 10 000 it's noticeable. For a 50-row dataset it doesn't matter, but for a family archive with years of accumulated audit log rows, cursor wins long-term.
  • No infrastructure changes needed. No new env vars, no new ports, no Caddy rules.

Recommendations

  • Confirm #291 lands first (or in parallel). Once pagination is live, /api/dashboard/activity gets called more often per user session (each Load-more click is another request). The Grafana p95 panel from #291 is the trip-wire that tells us if a bad index or a regression slows it down. Without that panel, a slow cursor query surfaces as a user complaint first.
  • Pagination depth ceiling is a product question, not an infra one — but cap it anyway. Arbitrary-depth walks are cheap per-request but the user experience of "Mehr laden" 200 times is worse than "Show me everything from March 2024 onward" as a future feature. For now: no hard cap in the backend, let the UX naturally bottom out when users stop clicking. Reconsider if a single user session starts burning >1000 requests/minute on this endpoint.
  • No Redis, no caching layer. A 40-row cursor query against an indexed 100k-row table is <10ms. Caching this would save single-digit milliseconds at the cost of cache invalidation complexity on new audit events. Not worth it.
  • EXPLAIN ANALYZE the new cursor query once before shipping. Confirm the plan shows an "Index Scan using idx_audit_log_rollup" with the right boundary condition and a small row count — not a "Seq Scan" or a full sort. A one-line sanity check before merge.

Open Decisions

  • None.
## 🏗️ Tobias Wendt — DevOps & Platform Engineer ### Observations - `V49` partial covering index is `(actor_id, document_id, kind, happened_at DESC)` WHERE kind IN (...). Cursor queries using `WHERE happened_at < :before` hit the index on the `happened_at DESC` component directly — index-only range scan, no heap fetch for the tuple comparison. - Offset pagination against the same index forces Postgres to materialize and skip — cost grows linearly with offset. At depth 1000 the difference is measurable; at depth 10 000 it's noticeable. For a 50-row dataset it doesn't matter, but for a family archive with years of accumulated audit log rows, cursor wins long-term. - No infrastructure changes needed. No new env vars, no new ports, no Caddy rules. ### Recommendations - **Confirm #291 lands first (or in parallel).** Once pagination is live, `/api/dashboard/activity` gets called more often per user session (each Load-more click is another request). The Grafana p95 panel from #291 is the trip-wire that tells us if a bad index or a regression slows it down. Without that panel, a slow cursor query surfaces as a user complaint first. - **Pagination depth ceiling is a product question, not an infra one — but cap it anyway.** Arbitrary-depth walks are cheap per-request but the user experience of "Mehr laden" 200 times is worse than "Show me everything from March 2024 onward" as a future feature. For now: no hard cap in the backend, let the UX naturally bottom out when users stop clicking. Reconsider if a single user session starts burning >1000 requests/minute on this endpoint. - **No Redis, no caching layer.** A 40-row cursor query against an indexed 100k-row table is <10ms. Caching this would save single-digit milliseconds at the cost of cache invalidation complexity on new audit events. Not worth it. - **EXPLAIN ANALYZE the new cursor query once before shipping.** Confirm the plan shows an "Index Scan using idx_audit_log_rollup" with the right boundary condition and a small row count — not a "Seq Scan" or a full sort. A one-line sanity check before merge. ### Open Decisions - None.
marcel commented 2026-04-20 19:06:48 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UX/Design Lead

Observations

  • docs/specs/chronik-spec.html §02 state 10 specifies: button label swaps "Mehr laden" → "Lädt …", aria-busy="true", 3 static skeleton rows below the button, focus stays on the button after load, aria-live="polite" announces chronik_load_more_announcement with count. All of that exists as Paraglide keys already (added in PR #288).
  • The original PR #288 implementation had the layout in place before I asked for pagination removal. Reinstating it is mostly an exercise in putting the original code back with a working backend.
  • "Mehr laden" at 320 px — the button should be full-width on mobile for tap target comfort, constrained width (not full-bleed) at sm: and above. The spec shows a centered pill, not a full-width bar.

Recommendations

  • End-of-feed state: when the response returns < 40 rows, replace the Load-more button with a small static message like „Das war alles — älter als {first visible day}." This is a dignity thing for seniors: an empty button area where something used to be feels broken; an explicit "you've seen everything" is reassuring. Copy can be an i18n key chronik_end_of_feed deferred to this issue's scope.
  • Announcement wording matters. {count} weitere Einträge geladen reads fine in German but at screen-reader speed it's verbose. Consider also announcing the end-of-feed state — "Keine älteren Einträge" — when we transition. Silence when the feed ends is a worse UX than the announcement.
  • Loading button label: "Lädt …" is correct per spec. Don't substitute a spinner glyph — the label + aria-busy combination is readable by both sighted users and assistive tech without extra ARIA plumbing.
  • Focus handling on end-of-feed: when the button is removed after the last page, focus needs to land somewhere — otherwise keyboard users lose their position. Move focus to the last row's link, or to the end-of-feed status text. Don't let it silently reset to <body>.
  • prefers-reduced-motion: skeleton rows are already specified as static (no shimmer) per Marcel's #285 resolution. Keep it — it's the senior-friendly default. No animation gating needed since there's no animation to gate.

Open Decisions

  • End-of-feed copy wording. Option A: „Das war alles" — direct, warm. Option B: „Keine älteren Einträge" — neutral, matches the "Keine neuen Erwähnungen" inbox-zero style. Option C: no message at all, just hide the button. Recommend A — the archive is the family history, finishing the list deserves a human phrasing, not a blank space.
## 🎨 Leonie Voss — UX/Design Lead ### Observations - `docs/specs/chronik-spec.html` §02 state 10 specifies: button label swaps "Mehr laden" → "Lädt …", `aria-busy="true"`, 3 static skeleton rows below the button, focus stays on the button after load, `aria-live="polite"` announces `chronik_load_more_announcement` with count. All of that exists as Paraglide keys already (added in PR #288). - The original PR #288 implementation had the layout in place before I asked for pagination removal. Reinstating it is mostly an exercise in putting the original code back with a working backend. - "Mehr laden" at 320 px — the button should be full-width on mobile for tap target comfort, constrained width (not full-bleed) at `sm:` and above. The spec shows a centered pill, not a full-width bar. ### Recommendations - **End-of-feed state:** when the response returns < 40 rows, replace the Load-more button with a small static message like `„Das war alles — älter als {first visible day}."` This is a dignity thing for seniors: an empty button area where something used to be feels broken; an explicit "you've seen everything" is reassuring. Copy can be an i18n key `chronik_end_of_feed` deferred to this issue's scope. - **Announcement wording matters.** `{count} weitere Einträge geladen` reads fine in German but at screen-reader speed it's verbose. Consider also announcing the end-of-feed state — "Keine älteren Einträge" — when we transition. Silence when the feed ends is a worse UX than the announcement. - **Loading button label:** "Lädt …" is correct per spec. Don't substitute a spinner glyph — the label + aria-busy combination is readable by both sighted users and assistive tech without extra ARIA plumbing. - **Focus handling on end-of-feed:** when the button is removed after the last page, focus needs to land *somewhere* — otherwise keyboard users lose their position. Move focus to the last row's link, or to the end-of-feed status text. Don't let it silently reset to `<body>`. - **`prefers-reduced-motion`:** skeleton rows are already specified as *static* (no shimmer) per Marcel's #285 resolution. Keep it — it's the senior-friendly default. No animation gating needed since there's no animation to gate. ### Open Decisions - **End-of-feed copy wording.** Option A: `„Das war alles"` — direct, warm. Option B: `„Keine älteren Einträge"` — neutral, matches the "Keine neuen Erwähnungen" inbox-zero style. Option C: no message at all, just hide the button. Recommend A — the archive is the family history, finishing the list deserves a human phrasing, not a blank space.
marcel commented 2026-04-20 19:07:05 +02:00
Author
Owner
Copy Link

🗳️ Decision Queue — Action Required

2 decisions need your input before implementation starts.

API Design

  • Cursor encoding at the API level.
    • Option A (recommended): Three separate query params — beforeAt, beforeDocId, beforeKind. Explicit, debuggable via curl, compiles straight to @RequestParam with Spring's built-in type binding (and security validation of beforeKind via the AuditKind enum).
    • Option B: Single opaque cursor param (base64-encoded tuple). Hides implementation detail, easier to evolve server-side without breaking clients.
    • Tradeoff: Option A is debuggable and validates naturally. Option B is slightly more future-proof at the cost of key-management complexity if ever signed.
    • (Raised by: Markus)

UX

  • End-of-feed copy wording.
    • Option A (recommended): „Das war alles" — direct, warm.
    • Option B: „Keine älteren Einträge" — neutral, matches „Keine neuen Erwähnungen" style.
    • Option C: No message, just hide the Load-more button.
    • Tradeoff: C leaves keyboard users without a focus anchor after the last page. A adds dignity to a finished list; B is more consistent with existing copy.
    • (Raised by: Leonie)
## 🗳️ Decision Queue — Action Required _2 decisions need your input before implementation starts._ ### API Design - **Cursor encoding at the API level.** - **Option A (recommended):** Three separate query params — `beforeAt`, `beforeDocId`, `beforeKind`. Explicit, debuggable via curl, compiles straight to `@RequestParam` with Spring's built-in type binding (and security validation of `beforeKind` via the `AuditKind` enum). - **Option B:** Single opaque `cursor` param (base64-encoded tuple). Hides implementation detail, easier to evolve server-side without breaking clients. - _Tradeoff:_ Option A is debuggable and validates naturally. Option B is slightly more future-proof at the cost of key-management complexity if ever signed. - _(Raised by: Markus)_ ### UX - **End-of-feed copy wording.** - **Option A (recommended):** `„Das war alles"` — direct, warm. - **Option B:** `„Keine älteren Einträge"` — neutral, matches `„Keine neuen Erwähnungen"` style. - **Option C:** No message, just hide the Load-more button. - _Tradeoff:_ C leaves keyboard users without a focus anchor after the last page. A adds dignity to a finished list; B is more consistent with existing copy. - _(Raised by: Leonie)_
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
P0-critical
Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.
 P1-high
Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.
 P2-medium
Noticeable improvement; doesn't block anything; schedule opportunistically.
 P3-later
Cosmetic or parking-lot work; fine to stay open indefinitely.
 audit
Read-only audit / assessment work; produces a report rather than changing code
 bug
Something isn't working
 cleanup
Removal of dead code, vague comments, naming violations, and scratch artifacts
 collaboration
We want to extend the family archive to have more collaboration tools
 conversation
descoped
We will do that later
 devops
documentation
README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs
 epic
Marker for epic-level issues that group multiple child issues
 feature
file-upload
legibility
Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction
 notification
person
phase-1: security
Security hygiene — must be done first
 phase-2: container-images
Production-ready multi-stage Docker images
 phase-3: prod-compose
Production compose overlay + Caddy reverse proxy
 phase-4: spring-prod-profile
Spring Boot production configuration profile
 phase-5: backups
Database and object storage backup strategy
 phase-6: deployment-docs
.env.example, DEPLOYMENT.md, runbook
 phase-7: monitoring
Prometheus, Loki, Grafana, Alertmanager
 refactor
Code restructuring without behaviour change
 security
test
ui
UI/UX and accessibility issues
 user
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Jens marcel
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/familienarchiv#290
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?