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

feat(dashboard): add kinds CSV query param to /api/dashboard/activity #293

New Issue
Closed
opened 2026-04-20 18:13:43 +02:00 by marcel · 14 comments
marcel commented 2026-04-20 18:13:43 +02:00
Owner
Copy Link

Background

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

The #285 issue body lists as backend scope: "Add kinds query param to GET /api/dashboard/activity (CSV of AuditKind, default = 6 kinds)."

PR #288 ships client-side filtering in /chronik (the 5 filter pills filter the already-loaded 40 items in-browser). That's acceptable for v1 but means narrow filters may show fewer items than available — e.g., switching to "Hochgeladen" with 2 uploads in the last 40 rows shows 2 rows even if the user has 50 uploads further back in time.

Concern

Scope drift between issue spec and shipped code. Moving the filter to the server lets each pill request its own set of kinds and get up to 40 matching rows.

Scope

  • Add kinds query param to DashboardController.getActivity (CSV parsed to Set<AuditKind>, default = all 6 rollable kinds).
  • Thread through to AuditLogQueryRepository.findRolledUpActivityFeed — parameterize the WHERE kind IN (...) clause.
  • Frontend: replace client-side filter with server-side request per pill change (re-load via goto?filter=... already triggers the server load).
  • Integration test: repo method returns only rows matching the provided kinds set.

Reference

## Background Deferred from PR #288 during review cycle 1 (Markus Keller). The #285 issue body lists as backend scope: *"Add `kinds` query param to `GET /api/dashboard/activity` (CSV of AuditKind, default = 6 kinds)."* PR #288 ships client-side filtering in `/chronik` (the 5 filter pills filter the already-loaded 40 items in-browser). That's acceptable for v1 but means narrow filters may show fewer items than available — e.g., switching to "Hochgeladen" with 2 uploads in the last 40 rows shows 2 rows even if the user has 50 uploads further back in time. ## Concern Scope drift between issue spec and shipped code. Moving the filter to the server lets each pill request its own set of kinds and get up to 40 matching rows. ## Scope - Add `kinds` query param to `DashboardController.getActivity` (CSV parsed to `Set<AuditKind>`, default = all 6 rollable kinds). - Thread through to `AuditLogQueryRepository.findRolledUpActivityFeed` — parameterize the `WHERE kind IN (...)` clause. - Frontend: replace client-side filter with server-side request per pill change (re-load via `goto?filter=...` already triggers the server load). - Integration test: repo method returns only rows matching the provided kinds set. ## Reference - PR: http://heim-nas:3005/marcel/familienarchiv/pulls/288 - Parent issue: #285
marcel commented 2026-04-20 19:10:01 +02:00
Author
Owner
Copy Link

🏛️ Markus Keller — Senior Application Architect

Observations

  • findRolledUpActivityFeed hardcodes WHERE a.kind IN ('TEXT_SAVED','FILE_UPLOADED','ANNOTATION_CREATED','BLOCK_REVIEWED','COMMENT_ADDED','MENTION_CREATED') in the innermost CTE events. The filter needs to move outward (or be parameterized in place) without breaking the partial covering index's applicability.
  • The V49 partial index WHERE kind IN (six values) requires the query predicate to be a subset of those six. A query for kind IN ('TEXT_SAVED') still uses the index because the Postgres planner recognizes the subset relationship. That's the right shape here.
  • Frontend pill → kind sets are currently defined in +page.svelte:95-116. Moving the mapping to the server means the API contract is "which kinds do you want"; the presentation label ("Hochgeladen", "Transkription") stays frontend.
  • "Für dich" is special — merged.filter((i) => i.kind === 'MENTION_CREATED' || i.youMentioned). The second disjunct uses the youMentioned flag derived from :currentUserId. Pure kinds filtering cannot express this.

Recommendations

  • Parameterize the WHERE kind IN (...) at the events CTE level, not at the outer aggregation. Filtering at events means fewer rows enter the LAG/session CTEs — cheaper and keeps the index predicate subset-valid: 
    WHERE a.kind IN (:kinds)   -- Spring Data binds Collection<String> to IN list
  AND a.document_id IS NOT NULL
  • Do not repurpose kinds to express "Für dich". That filter composes kind=MENTION_CREATED OR youMentioned=TRUE. Keep it as a separate forYou: boolean query param (or encode it in the existing filter alias if you want a single knob). kinds stays a pure kind-set filter; forYou is its own predicate. Conflating them makes the API ambiguous.
  • Frontend pill mapping lives in +page.server.ts, not +page.svelte. The pill value ("hochgeladen") is a UI token; mapping it to an AuditKind set is a server concern. That keeps the +page.svelte focused on rendering and makes the filter spec testable at the server load layer.
  • Default when kinds is absent: all 6. Back-compat with dashboard side-rail callers that don't know about the new param.

Open Decisions

  • "Für dich" filter shape. Option A: separate forYou: boolean query param alongside kinds. Option B: dedicated filter: "fuer-dich" enum value at the controller that translates internally to kinds=MENTION_CREATED + forYou=true. Option C: keep "Für dich" as client-side-only filter (since the live SSE singleton already drives the real for-you box). Recommend C — "Für dich" is the only pill that pulls from notifications, not the activity feed, and the existing client-side predicate already works for the timeline row highlighting.
## 🏛️ Markus Keller — Senior Application Architect ### Observations - `findRolledUpActivityFeed` hardcodes `WHERE a.kind IN ('TEXT_SAVED','FILE_UPLOADED','ANNOTATION_CREATED','BLOCK_REVIEWED','COMMENT_ADDED','MENTION_CREATED')` in the innermost CTE `events`. The filter needs to move outward (or be parameterized in place) without breaking the partial covering index's applicability. - The V49 partial index `WHERE kind IN (six values)` requires the query predicate to be a **subset** of those six. A query for `kind IN ('TEXT_SAVED')` still uses the index because the Postgres planner recognizes the subset relationship. That's the right shape here. - Frontend pill → kind sets are currently defined in `+page.svelte:95-116`. Moving the mapping to the server means the *API contract* is "which kinds do you want"; the *presentation label* ("Hochgeladen", "Transkription") stays frontend. - "Für dich" is special — `merged.filter((i) => i.kind === 'MENTION_CREATED' || i.youMentioned)`. The second disjunct uses the `youMentioned` flag derived from `:currentUserId`. Pure `kinds` filtering cannot express this. ### Recommendations - **Parameterize the `WHERE kind IN (...)` at the events CTE level**, not at the outer aggregation. Filtering at `events` means fewer rows enter the LAG/session CTEs — cheaper and keeps the index predicate subset-valid: ```sql WHERE a.kind IN (:kinds) -- Spring Data binds Collection<String> to IN list AND a.document_id IS NOT NULL ``` - **Do not repurpose `kinds` to express "Für dich".** That filter composes `kind=MENTION_CREATED OR youMentioned=TRUE`. Keep it as a separate `forYou: boolean` query param (or encode it in the existing `filter` alias if you want a single knob). `kinds` stays a pure kind-set filter; `forYou` is its own predicate. Conflating them makes the API ambiguous. - **Frontend pill mapping lives in `+page.server.ts`, not `+page.svelte`.** The pill value (`"hochgeladen"`) is a UI token; mapping it to an `AuditKind` set is a server concern. That keeps the `+page.svelte` focused on rendering and makes the filter spec testable at the server load layer. - **Default when `kinds` is absent: all 6.** Back-compat with dashboard side-rail callers that don't know about the new param. ### Open Decisions - **"Für dich" filter shape.** Option A: separate `forYou: boolean` query param alongside `kinds`. Option B: dedicated `filter: "fuer-dich"` enum value at the controller that translates internally to `kinds=MENTION_CREATED + forYou=true`. Option C: keep "Für dich" as client-side-only filter (since the live SSE singleton already drives the real for-you box). Recommend C — "Für dich" is the only pill that pulls from notifications, not the activity feed, and the existing client-side predicate already works for the timeline row highlighting.
marcel commented 2026-04-20 19:10:25 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

  • Spring's @RequestParam binds Set<AuditKind> directly when the param is CSV (?kinds=TEXT_SAVED,FILE_UPLOADED) — no custom converter needed. Enum name validation happens automatically; a bad value returns 400.
  • The filter mapping table today lives at +page.svelte:95-116. Lifting it to +page.server.ts removes reactive filter state from the component — activeFilter can then be a simple $derived(data.filter) without the $state + $effect + eslint-disable dance I flagged in PR #288 review.
  • ActivityFeedItemDTO.kind is typed as the full AuditKind union. Existing frontend filter predicates (i.kind === 'FILE_UPLOADED') compile fine against the same DTO shape post-change.

Recommendations

  • Controller signature: 
    @GetMapping("/activity")
public List<ActivityFeedItemDTO> getActivity(
        Authentication auth,
        @RequestParam(defaultValue = "7") int limit,
        @RequestParam(required = false) Set<AuditKind> kinds) {
    UUID userId = SecurityUtils.requireUserId(auth, userService);
    Set<AuditKind> effectiveKinds = (kinds == null || kinds.isEmpty())
        ? AuditKind.ROLLUP_ELIGIBLE
        : kinds;
    return dashboardService.getActivity(userId, Math.min(limit, 40), effectiveKinds);
}
    Introduce a constant AuditKind.ROLLUP_ELIGIBLE (or similar) as the single source of truth for the six-kind default. Today that list is duplicated in the SQL WHERE + V49 index + frontend filter — promoting to a Java constant trims one of the duplications.
  • Red tests in order (repo layer first):
    1. rolledUpFeed_filters_to_provided_kinds_only — insert TEXT_SAVED + FILE_UPLOADED, call with kinds=[FILE_UPLOADED], assert only the upload row.
    2. rolledUpFeed_returns_empty_when_kinds_set_is_empty — decide behavior: treat as "no match" or as "default all"? Felix vote: empty set = no match (returns empty list); only null means "use default". That's explicit and avoids ambiguous signaling.
    3. rolledUpFeed_still_uses_partial_index_for_kind_subsets — not a unit test per se; add an EXPLAIN ANALYZE check to the PR description or a quick @Test @Disabled with the plan assertion.
    4. Controller test: activity_parses_kinds_csv_into_set + activity_rejects_invalid_enum_value_with_400.
  • Frontend simplification: 
    <!-- +page.server.ts -->
const kindsForFilter: Record<FilterValue, AuditKind[] | undefined> = {
    alle: undefined, // all 6 server default
    'fuer-dich': ['MENTION_CREATED'],
    hochgeladen: ['FILE_UPLOADED'],
    transkription: ['TEXT_SAVED', 'BLOCK_REVIEWED', 'ANNOTATION_CREATED'],
    kommentare: ['COMMENT_ADDED', 'MENTION_CREATED']
};
// ...
const result = await api.GET('/api/dashboard/activity', {
    params: { query: { limit: 40, kinds: kindsForFilter[filter]?.join(',') } }
});
    Delete the displayFeed client-side switch in +page.svelte — the server-loaded data.activityFeed is already filtered. That also removes the whole activeFilter = $state + $effect block, since the filter is now pure URL → load → render.
  • Regenerate OpenAPI types after the DTO/endpoint change — npm run generate:api. Miss this and the Chronik load function has type errors on the kinds param.

Open Decisions

  • Empty set semantics. Option A: ?kinds= (empty) returns empty list — explicit, maps to "I asked for nothing, I got nothing." Option B: empty set is equivalent to omitting the param, returns default. Option C: empty set is a 400 — "malformed request." Recommend A; it has one less special case than B, and 400 for empty is user-hostile.
## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - Spring's `@RequestParam` binds `Set<AuditKind>` directly when the param is CSV (`?kinds=TEXT_SAVED,FILE_UPLOADED`) — no custom converter needed. Enum name validation happens automatically; a bad value returns 400. - The filter mapping table today lives at `+page.svelte:95-116`. Lifting it to `+page.server.ts` removes reactive filter state from the component — `activeFilter` can then be a simple `$derived(data.filter)` without the `$state + $effect + eslint-disable` dance I flagged in PR #288 review. - `ActivityFeedItemDTO.kind` is typed as the full `AuditKind` union. Existing frontend filter predicates (`i.kind === 'FILE_UPLOADED'`) compile fine against the same DTO shape post-change. ### Recommendations - **Controller signature:** ```java @GetMapping("/activity") public List<ActivityFeedItemDTO> getActivity( Authentication auth, @RequestParam(defaultValue = "7") int limit, @RequestParam(required = false) Set<AuditKind> kinds) { UUID userId = SecurityUtils.requireUserId(auth, userService); Set<AuditKind> effectiveKinds = (kinds == null || kinds.isEmpty()) ? AuditKind.ROLLUP_ELIGIBLE : kinds; return dashboardService.getActivity(userId, Math.min(limit, 40), effectiveKinds); } ``` Introduce a constant `AuditKind.ROLLUP_ELIGIBLE` (or similar) as the single source of truth for the six-kind default. Today that list is duplicated in the SQL WHERE + V49 index + frontend filter — promoting to a Java constant trims one of the duplications. - **Red tests in order (repo layer first):** 1. `rolledUpFeed_filters_to_provided_kinds_only` — insert `TEXT_SAVED` + `FILE_UPLOADED`, call with `kinds=[FILE_UPLOADED]`, assert only the upload row. 2. `rolledUpFeed_returns_empty_when_kinds_set_is_empty` — decide behavior: treat as "no match" or as "default all"? Felix vote: empty set = no match (returns empty list); only `null` means "use default". That's explicit and avoids ambiguous signaling. 3. `rolledUpFeed_still_uses_partial_index_for_kind_subsets` — not a unit test per se; add an `EXPLAIN ANALYZE` check to the PR description or a quick `@Test @Disabled` with the plan assertion. 4. Controller test: `activity_parses_kinds_csv_into_set` + `activity_rejects_invalid_enum_value_with_400`. - **Frontend simplification:** ```svelte <!-- +page.server.ts --> const kindsForFilter: Record<FilterValue, AuditKind[] | undefined> = { alle: undefined, // all 6 server default 'fuer-dich': ['MENTION_CREATED'], hochgeladen: ['FILE_UPLOADED'], transkription: ['TEXT_SAVED', 'BLOCK_REVIEWED', 'ANNOTATION_CREATED'], kommentare: ['COMMENT_ADDED', 'MENTION_CREATED'] }; // ... const result = await api.GET('/api/dashboard/activity', { params: { query: { limit: 40, kinds: kindsForFilter[filter]?.join(',') } } }); ``` Delete the `displayFeed` client-side switch in `+page.svelte` — the server-loaded `data.activityFeed` is already filtered. That also removes the whole `activeFilter = $state + $effect` block, since the filter is now pure URL → load → render. - **Regenerate OpenAPI types after the DTO/endpoint change** — `npm run generate:api`. Miss this and the Chronik load function has type errors on the `kinds` param. ### Open Decisions - **Empty set semantics.** Option A: `?kinds=` (empty) returns empty list — explicit, maps to "I asked for nothing, I got nothing." Option B: empty set is equivalent to omitting the param, returns default. Option C: empty set is a 400 — "malformed request." Recommend A; it has one less special case than B, and 400 for empty is user-hostile.
marcel commented 2026-04-20 19:10:38 +02:00
Author
Owner
Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

  • AuditKind is a plain enum — Spring's @RequestParam Set<AuditKind> binding returns 400 on any value that isn't a member. No special validation needed on the controller, and no string ever reaches SQL directly.
  • The existing rollup query uses :currentUserId as a named parameter — safe. Adding :kinds as a second named parameter via Spring Data's Collection binding stays in the parameterized path. No string concatenation anywhere.
  • /api/dashboard/activity stays gated by @RequirePermission(Permission.READ_ALL) — the kinds filter narrows what the requester sees, never expands access.

Recommendations

  • Type the controller param as Set<AuditKind>, not String or List<String>. The enum binding is the validation. A String-typed param means hand-rolling the CSV parse + validation + lowercasing, and the first bug lands in the regex.
  • Log the rejected enum values when 400 fires, but parameterized. If a client sends ?kinds=TEXT_SAVED,evil_value, log: 
    logger.info("Rejected activity request: invalid AuditKind '{}'", badValue);
    Never logger.info("... '" + badValue + "'") — that's the Log4Shell path. SLF4J {} is injection-proof.
  • No rate-limit change needed. The kinds filter doesn't widen the reachable data set; it narrows it. An attacker already with READ_ALL cannot use kinds to extract anything beyond what they can get with the default.
  • Cap the cardinality of the kinds set at the controller at the enum size (6). A request with ?kinds=X,X,X,X,X,X,X,X,... (10k repeats) would be trimmed by the Set contract (duplicates dropped) — but Spring parses the CSV first, which could still allocate a long intermediate list. Not worth a custom validator; the URL parser caps at ~8KB anyway. Just flagging for awareness.

Open Decisions

  • None.
## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - `AuditKind` is a plain enum — Spring's `@RequestParam Set<AuditKind>` binding returns 400 on any value that isn't a member. No special validation needed on the controller, and no string ever reaches SQL directly. - The existing rollup query uses `:currentUserId` as a named parameter — safe. Adding `:kinds` as a second named parameter via Spring Data's `Collection` binding stays in the parameterized path. No string concatenation anywhere. - `/api/dashboard/activity` stays gated by `@RequirePermission(Permission.READ_ALL)` — the kinds filter narrows what the requester sees, never expands access. ### Recommendations - **Type the controller param as `Set<AuditKind>`, not `String` or `List<String>`.** The enum binding is the validation. A `String`-typed param means hand-rolling the CSV parse + validation + lowercasing, and the first bug lands in the regex. - **Log the rejected enum values when 400 fires, but parameterized.** If a client sends `?kinds=TEXT_SAVED,evil_value`, log: ```java logger.info("Rejected activity request: invalid AuditKind '{}'", badValue); ``` Never `logger.info("... '" + badValue + "'")` — that's the Log4Shell path. SLF4J `{}` is injection-proof. - **No rate-limit change needed.** The kinds filter doesn't widen the reachable data set; it narrows it. An attacker already with `READ_ALL` cannot use `kinds` to extract anything beyond what they can get with the default. - **Cap the cardinality of the `kinds` set at the controller** at the enum size (6). A request with `?kinds=X,X,X,X,X,X,X,X,... (10k repeats)` would be trimmed by the `Set` contract (duplicates dropped) — but Spring parses the CSV first, which could still allocate a long intermediate list. Not worth a custom validator; the URL parser caps at ~8KB anyway. Just flagging for awareness. ### Open Decisions - None.
marcel commented 2026-04-20 19:10:53 +02:00
Author
Owner
Copy Link

🧪 Sara Holt — Senior QA Engineer

Observations

  • AuditLogQueryRepositoryRolledUpTest already has 6 tests — the 7th for kinds filtering slots in without infrastructure changes.
  • DashboardControllerTest has @WebMvcTest with mocked DashboardService — perfect for parsing/validation tests that don't need a real DB.
  • Frontend: chronik/page.server.spec.ts exists and mocks the API client. Adding "the load function passes the right kinds CSV per filter" is mechanical.

Recommendations

  • Backend integration tests: 
    @Test void rolledUpFeed_with_single_kind_returns_only_that_kind();
@Test void rolledUpFeed_with_multiple_kinds_returns_union();
@Test void rolledUpFeed_with_default_returns_all_six_eligible_kinds();
@Test void rolledUpFeed_excludes_rows_whose_kind_is_not_in_the_filter_set();
@Test void rolledUpFeed_rollup_still_works_when_kind_set_is_filtered_to_single_rollable_kind();
    The last one is the boundary test Felix cares about: filter to TEXT_SAVED only, insert 10 TEXT_SAVED events within 2h, assert one rollup row with count=10. Proves the session grouping still composes with the kinds filter.
  • Controller parse/validation tests: 
    @Test void activity_parses_kinds_csv_into_set();
@Test void activity_returns_400_when_kinds_contains_invalid_enum_value();
@Test void activity_uses_default_set_when_kinds_param_absent();
// Felix open decision: empty = empty result, not default fallback
@Test void activity_returns_empty_list_when_kinds_is_empty_string();
  • Frontend load function tests: 
    it('passes no kinds param when filter=alle', ...);
it('passes kinds=FILE_UPLOADED when filter=hochgeladen', ...);
it('passes kinds=TEXT_SAVED,BLOCK_REVIEWED,ANNOTATION_CREATED when filter=transkription', ...);
it('passes kinds=COMMENT_ADDED,MENTION_CREATED when filter=kommentare', ...);
// "fuer-dich" is client-only if Markus's Option C wins — add an assertion then too
  • Delete the obsolete client-side filter tests once +page.svelte stops filtering. No sense maintaining parallel coverage at both layers.
  • Regression risk: the +page.svelte.spec.ts file does not exist yet (flagged in #290 too). This issue is a second chance to add it — wire the filter assertion in the same spec file #290 creates.

Open Decisions

  • None.
## 🧪 Sara Holt — Senior QA Engineer ### Observations - `AuditLogQueryRepositoryRolledUpTest` already has 6 tests — the 7th for `kinds` filtering slots in without infrastructure changes. - `DashboardControllerTest` has `@WebMvcTest` with mocked `DashboardService` — perfect for parsing/validation tests that don't need a real DB. - Frontend: `chronik/page.server.spec.ts` exists and mocks the API client. Adding "the load function passes the right kinds CSV per filter" is mechanical. ### Recommendations - **Backend integration tests:** ```java @Test void rolledUpFeed_with_single_kind_returns_only_that_kind(); @Test void rolledUpFeed_with_multiple_kinds_returns_union(); @Test void rolledUpFeed_with_default_returns_all_six_eligible_kinds(); @Test void rolledUpFeed_excludes_rows_whose_kind_is_not_in_the_filter_set(); @Test void rolledUpFeed_rollup_still_works_when_kind_set_is_filtered_to_single_rollable_kind(); ``` The last one is the boundary test Felix cares about: filter to `TEXT_SAVED` only, insert 10 TEXT_SAVED events within 2h, assert one rollup row with count=10. Proves the session grouping still composes with the kinds filter. - **Controller parse/validation tests:** ```java @Test void activity_parses_kinds_csv_into_set(); @Test void activity_returns_400_when_kinds_contains_invalid_enum_value(); @Test void activity_uses_default_set_when_kinds_param_absent(); // Felix open decision: empty = empty result, not default fallback @Test void activity_returns_empty_list_when_kinds_is_empty_string(); ``` - **Frontend load function tests:** ```ts it('passes no kinds param when filter=alle', ...); it('passes kinds=FILE_UPLOADED when filter=hochgeladen', ...); it('passes kinds=TEXT_SAVED,BLOCK_REVIEWED,ANNOTATION_CREATED when filter=transkription', ...); it('passes kinds=COMMENT_ADDED,MENTION_CREATED when filter=kommentare', ...); // "fuer-dich" is client-only if Markus's Option C wins — add an assertion then too ``` - **Delete the obsolete client-side filter tests once `+page.svelte` stops filtering.** No sense maintaining parallel coverage at both layers. - **Regression risk:** the `+page.svelte.spec.ts` file does not exist yet (flagged in #290 too). This issue is a second chance to add it — wire the filter assertion in the same spec file #290 creates. ### Open Decisions - None.
marcel commented 2026-04-20 19:11:03 +02:00
Author
Owner
Copy Link

🏗️ Tobias Wendt — DevOps & Platform Engineer

Observations

  • No infrastructure surface touched — no new containers, env vars, ports, or secrets.
  • Metric cardinality note (ties into #291): once the kinds query param ships, Micrometer's default URI-template tagger still templates this as /api/dashboard/activity (query params are stripped before the label is computed). Good — no explosion of per-kind-combination metrics.

Recommendations

  • EXPLAIN ANALYZE with single-kind filter before merge to confirm the V49 partial index is still chosen when the query predicate is a subset of the six indexed kinds. Postgres should recognize this; verify once rather than assume. A quick check: 
    EXPLAIN (ANALYZE, BUFFERS) SELECT ...  -- the rewritten query
   WHERE a.kind IN ('FILE_UPLOADED') AND a.document_id IS NOT NULL;
    Want to see Index Scan using idx_audit_log_rollup, not Seq Scan on audit_log.
  • Once #291 lands, the pill click rate will be visible on the Grafana panel — each filter change is a new /api/dashboard/activity request. The dashboard side-rail continues to hit the default (all 6 kinds) at page load. Total traffic doesn't explode, but the request pattern broadens.
  • Renovate will not break this. Spring Boot enum binding has been stable since 3.x. No dependency upgrade risk.

Open Decisions

  • None.
## 🏗️ Tobias Wendt — DevOps & Platform Engineer ### Observations - No infrastructure surface touched — no new containers, env vars, ports, or secrets. - Metric cardinality note (ties into #291): once the `kinds` query param ships, Micrometer's default URI-template tagger still templates this as `/api/dashboard/activity` (query params are stripped before the label is computed). Good — no explosion of per-kind-combination metrics. ### Recommendations - **EXPLAIN ANALYZE with single-kind filter before merge** to confirm the V49 partial index is still chosen when the query predicate is a subset of the six indexed kinds. Postgres *should* recognize this; verify once rather than assume. A quick check: ```sql EXPLAIN (ANALYZE, BUFFERS) SELECT ... -- the rewritten query WHERE a.kind IN ('FILE_UPLOADED') AND a.document_id IS NOT NULL; ``` Want to see `Index Scan using idx_audit_log_rollup`, not `Seq Scan on audit_log`. - **Once #291 lands**, the pill click rate will be visible on the Grafana panel — each filter change is a new `/api/dashboard/activity` request. The dashboard side-rail continues to hit the default (all 6 kinds) at page load. Total traffic doesn't explode, but the request pattern broadens. - **Renovate will not break this.** Spring Boot enum binding has been stable since 3.x. No dependency upgrade risk. ### Open Decisions - None.
marcel commented 2026-04-20 19:11:20 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UX/Design Lead

Observations

  • The pill UX is locked in PR #288 — role=radiogroup, arrow key navigation, 44 px min-height. This issue changes the data-fetch layer, not the pill component itself.
  • The user-visible difference is: filtering to "Hochgeladen" now returns 40 upload rows (if available), not "however many of the top 40 activity rows happen to be uploads." That's a meaningful UX win for anyone looking for a specific class of activity.
  • "Für dich" filter is the only pill that blends notifications (personal mentions) with activity rows marked youMentioned. If that filter moves server-side, the youMentioned predicate goes with it — meaning the for-you timeline rows become server-filtered, while the for-you box at the top stays driven by the live SSE notifications singleton. Two different sources, one visual affordance.

Recommendations

  • Loading state during filter change. When a user clicks a pill, the server round-trip takes ~100-500ms (cached index, small result set). That's fast but not instantaneous. Add a subtle loading state on the timeline area — a aria-busy="true" on the <ChronikTimeline> wrapper during navigation, possibly with a 3-row skeleton after 200ms (debounced so fast navigations don't flash). The spec's "Mehr laden" skeleton pattern applies here by analogy.
  • Empty-state copy parity. Once server-side filtering ships, the filter-empty variant needs to make sense: "Nichts in dieser Ansicht" works for both client-side-empty and server-side-empty. No copy change needed — just confirm the empty state still renders when the server returns [] for a narrow filter.
  • Announce filter change via aria-live. When the filter pill is clicked and the server responds with N rows, announce "X Einträge gefiltert" (or similar) to the polite-live region. Pill clicks without feedback are invisible to keyboard/screen-reader users. This is a standalone a11y improvement, not strictly in this issue's scope but worth piggybacking.
  • Keep "Für dich" filter semantics consistent with the existing user model. If Markus's Option C wins (client-side Für-dich filter stays), that's the right call — the pill filters what the user sees in the timeline; the Für-dich box is the canonical unread list driven by notifications. Two channels, clear roles.

Open Decisions

  • None from the UX side — Markus's Option C (Für-dich stays client-side) preserves the current design model cleanly.
## 🎨 Leonie Voss — UX/Design Lead ### Observations - The pill UX is locked in PR #288 — `role=radiogroup`, arrow key navigation, 44 px min-height. This issue changes the data-fetch layer, not the pill component itself. - The user-visible difference is: filtering to "Hochgeladen" now returns 40 *upload* rows (if available), not "however many of the top 40 activity rows happen to be uploads." That's a meaningful UX win for anyone looking for a specific class of activity. - "Für dich" filter is the only pill that blends notifications (personal mentions) with activity rows marked `youMentioned`. If that filter moves server-side, the `youMentioned` predicate goes with it — meaning the for-you *timeline rows* become server-filtered, while the for-you *box at the top* stays driven by the live SSE notifications singleton. Two different sources, one visual affordance. ### Recommendations - **Loading state during filter change.** When a user clicks a pill, the server round-trip takes ~100-500ms (cached index, small result set). That's fast but not instantaneous. Add a subtle loading state on the timeline area — a `aria-busy="true"` on the `<ChronikTimeline>` wrapper during navigation, possibly with a 3-row skeleton after 200ms (debounced so fast navigations don't flash). The spec's "Mehr laden" skeleton pattern applies here by analogy. - **Empty-state copy parity.** Once server-side filtering ships, the `filter-empty` variant needs to make sense: "Nichts in dieser Ansicht" works for both client-side-empty and server-side-empty. No copy change needed — just confirm the empty state still renders when the server returns `[]` for a narrow filter. - **Announce filter change via `aria-live`.** When the filter pill is clicked and the server responds with N rows, announce "X Einträge gefiltert" (or similar) to the polite-live region. Pill clicks without feedback are invisible to keyboard/screen-reader users. This is a standalone a11y improvement, not strictly in this issue's scope but worth piggybacking. - **Keep "Für dich" filter semantics consistent** with the existing user model. If Markus's Option C wins (client-side Für-dich filter stays), that's the right call — the pill filters what the user sees in the timeline; the Für-dich *box* is the canonical unread list driven by notifications. Two channels, clear roles. ### Open Decisions - None from the UX side — Markus's Option C (Für-dich stays client-side) preserves the current design model cleanly.
marcel commented 2026-04-20 19:11:35 +02:00
Author
Owner
Copy Link

🗳️ Decision Queue — Action Required

2 decisions need your input before implementation starts.

Architecture

  • "Für dich" filter shape.
    • Option A: Separate forYou: boolean query param alongside kinds.
    • Option B: Dedicated filter: "fuer-dich" enum at the controller that translates to kinds=MENTION_CREATED + forYou=true internally.
    • Option C (recommended): Keep "Für dich" as a client-side-only filter — the Für-dich box at the top is already SSE-driven, and the timeline predicate i.youMentioned is a display flag not a permission boundary.
    • Tradeoff: C keeps the API surface minimal and clean; A/B add a param that duplicates an already-working client-side predicate.
    • (Raised by: Markus)

API Design

  • Empty kinds set semantics.
    • Option A (recommended): ?kinds= (empty CSV) returns an empty list — "I asked for nothing, I got nothing."
    • Option B: Empty set treated as missing param, returns default all-6 — implicit signaling.
    • Option C: Empty set returns 400 — strict validation.
    • Tradeoff: A has one less special case than B and is less user-hostile than C.
    • (Raised by: Felix)
## 🗳️ Decision Queue — Action Required _2 decisions need your input before implementation starts._ ### Architecture - **"Für dich" filter shape.** - **Option A:** Separate `forYou: boolean` query param alongside `kinds`. - **Option B:** Dedicated `filter: "fuer-dich"` enum at the controller that translates to `kinds=MENTION_CREATED + forYou=true` internally. - **Option C (recommended):** Keep "Für dich" as a client-side-only filter — the Für-dich box at the top is already SSE-driven, and the timeline predicate `i.youMentioned` is a display flag not a permission boundary. - _Tradeoff:_ C keeps the API surface minimal and clean; A/B add a param that duplicates an already-working client-side predicate. - _(Raised by: Markus)_ ### API Design - **Empty `kinds` set semantics.** - **Option A (recommended):** `?kinds=` (empty CSV) returns an empty list — "I asked for nothing, I got nothing." - **Option B:** Empty set treated as missing param, returns default all-6 — implicit signaling. - **Option C:** Empty set returns 400 — strict validation. - _Tradeoff:_ A has one less special case than B and is less user-hostile than C. - _(Raised by: Felix)_
marcel commented 2026-04-21 19:10:02 +02:00
Author
Owner
Copy Link

🏛️ Markus Keller — Senior Application Architect (Round 2)

The first-round review covered the SQL parameterization and "Für dich" shape well. One structural gap was missed.

Observations

  • AuditLogQueryService.findActivityFeed() is called from two places, not one. DashboardService.getActivity() at line 112 should receive the kinds filter. DashboardService.getPulse() at line 92 also calls auditLogQueryService.findActivityFeed(userId, 50) — that call must continue to receive all 6 rollable kinds regardless of any filter param.
  • If the AuditLogQueryService.findActivityFeed() signature is changed to (UUID, int, Set<AuditKind>) without an overload, the pulse call site breaks at compile time. If the issue scope only changes getActivity, the pulse path must still pass an explicit default.
  • AuditLogQueryService is a thin delegation layer — it does no filtering logic itself. The kinds set should flow through it unchanged, just like limit does today.

Recommendations

  • Add an overloaded findActivityFeed in AuditLogQueryService so the existing no-kinds signature used by getPulse keeps working without change: 
    // existing — called by getPulse, unchanged
public List<ActivityFeedRow> findActivityFeed(UUID currentUserId, int limit) {
    return queryRepository.findRolledUpActivityFeed(
        currentUserId.toString(), limit, toKindStrings(AuditKind.ROLLUP_ELIGIBLE));
}

// new — called by getActivity after this issue
public List<ActivityFeedRow> findActivityFeed(UUID currentUserId, int limit, Set<AuditKind> kinds) {
    return queryRepository.findRolledUpActivityFeed(
        currentUserId.toString(), limit, toKindStrings(kinds));
}
    DashboardService.getPulse() at line 92 calls the zero-kinds overload and is not touched. DashboardService.getActivity() calls the new overload. Clean separation.
  • The repository method signature needs a third param for the kinds collection. Spring Data binds Collection<String> to a native IN (:kinds) clause — pass Set<String> of enum names.
  • Confirm the pulse path is still tested after the refactor. The existing AuditLogQueryRepositoryRolledUpTest covers the repo directly. Add a DashboardServicePulseTest asserting the pulse path still calls findActivityFeed with all 6 kinds if one does not already exist.
## 🏛️ Markus Keller — Senior Application Architect _(Round 2)_ The first-round review covered the SQL parameterization and "Für dich" shape well. One structural gap was missed. ### Observations - **`AuditLogQueryService.findActivityFeed()` is called from two places**, not one. `DashboardService.getActivity()` at line 112 should receive the `kinds` filter. `DashboardService.getPulse()` at line 92 also calls `auditLogQueryService.findActivityFeed(userId, 50)` — that call must continue to receive all 6 rollable kinds regardless of any filter param. - If the `AuditLogQueryService.findActivityFeed()` signature is changed to `(UUID, int, Set<AuditKind>)` without an overload, the pulse call site breaks at compile time. If the issue scope only changes `getActivity`, the pulse path must still pass an explicit default. - `AuditLogQueryService` is a thin delegation layer — it does no filtering logic itself. The `kinds` set should flow through it unchanged, just like `limit` does today. ### Recommendations - **Add an overloaded `findActivityFeed` in `AuditLogQueryService`** so the existing no-kinds signature used by `getPulse` keeps working without change: ```java // existing — called by getPulse, unchanged public List<ActivityFeedRow> findActivityFeed(UUID currentUserId, int limit) { return queryRepository.findRolledUpActivityFeed( currentUserId.toString(), limit, toKindStrings(AuditKind.ROLLUP_ELIGIBLE)); } // new — called by getActivity after this issue public List<ActivityFeedRow> findActivityFeed(UUID currentUserId, int limit, Set<AuditKind> kinds) { return queryRepository.findRolledUpActivityFeed( currentUserId.toString(), limit, toKindStrings(kinds)); } ``` `DashboardService.getPulse()` at line 92 calls the zero-kinds overload and is not touched. `DashboardService.getActivity()` calls the new overload. Clean separation. - **The repository method signature** needs a third param for the kinds collection. Spring Data binds `Collection<String>` to a native `IN (:kinds)` clause — pass `Set<String>` of enum names. - **Confirm the pulse path is still tested** after the refactor. The existing `AuditLogQueryRepositoryRolledUpTest` covers the repo directly. Add a `DashboardServicePulseTest` asserting the pulse path still calls `findActivityFeed` with all 6 kinds if one does not already exist.
marcel commented 2026-04-21 19:10:14 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer (Round 2)

The first round covered the controller signature, ROLLUP_ELIGIBLE constant, and frontend simplification. Two dead-code deletion issues and one existing-test-breakage were not flagged.

Observations

  • feedFilters.ts and feedFilters.test.ts become fully dead code after this PR. feedFilters.ts exports filterFeed() which is currently called from +page.svelte. Once the server handles filtering, filterFeed is unused. Both files should be deleted, not just abandoned.
    feedFilters.test.ts has 9 tests covering filterFeed — they will still pass but test behavior that no longer exists in production. Dead passing tests are a maintenance trap.

  • The existing page.server.spec.ts test will silently verify wrong behavior after the change. Line 31-34 asserts:

    expect(mockApi.GET).toHaveBeenCalledWith('/api/dashboard/activity', {
    params: { query: { limit: 40 } }
});

    Once the load function passes kinds per filter, this assertion will fail for non-alle filters and will need updating. Don't add tests for the new behavior alongside the old test — rewrite the existing test to assert the correct call shape for each filter value.

  • DashboardController current default is limit=7 (line 38), not 40. The chronik load function overrides this by always passing limit=40 explicitly. The controller default only matters for callers that omit the param — the dashboard side-rail, presumably.

Recommendations

  • Delete feedFilters.ts and feedFilters.test.ts in the same commit that removes the displayFeed client-side switch from +page.svelte. One commit: remove client-side filtering, delete dead files. Makes the PR diff easier to review.
  • Rewrite page.server.spec.ts to cover the per-filter API call shape: 
    it('passes kinds=FILE_UPLOADED when filter=hochgeladen', async () => {
    mockApi.GET.mockImplementation(...);
    await load({ fetch, url: buildUrl('?filter=hochgeladen') } as never);
    expect(mockApi.GET).toHaveBeenCalledWith('/api/dashboard/activity', {
        params: { query: { limit: 40, kinds: 'FILE_UPLOADED' } }
    });
});
it('omits kinds param when filter=alle', async () => { ... });
// etc. per filter
  • Keep +page.svelte usage of data.activityFeed direct — no client-side filterFeed() call. The server-loaded feed is already filtered; the component just renders it.
## 👨‍💻 Felix Brandt — Senior Fullstack Developer _(Round 2)_ The first round covered the controller signature, `ROLLUP_ELIGIBLE` constant, and frontend simplification. Two dead-code deletion issues and one existing-test-breakage were not flagged. ### Observations - **`feedFilters.ts` and `feedFilters.test.ts` become fully dead code** after this PR. `feedFilters.ts` exports `filterFeed()` which is currently called from `+page.svelte`. Once the server handles filtering, `filterFeed` is unused. Both files should be **deleted**, not just abandoned. `feedFilters.test.ts` has 9 tests covering `filterFeed` — they will still pass but test behavior that no longer exists in production. Dead passing tests are a maintenance trap. - **The existing `page.server.spec.ts` test will silently verify wrong behavior** after the change. Line 31-34 asserts: ```ts expect(mockApi.GET).toHaveBeenCalledWith('/api/dashboard/activity', { params: { query: { limit: 40 } } }); ``` Once the load function passes `kinds` per filter, this assertion will fail for non-`alle` filters and will need updating. Don't add tests for the new behavior alongside the old test — **rewrite the existing test** to assert the correct call shape for each filter value. - **`DashboardController` current default is `limit=7`** (line 38), not 40. The chronik load function overrides this by always passing `limit=40` explicitly. The controller default only matters for callers that omit the param — the dashboard side-rail, presumably. ### Recommendations - **Delete `feedFilters.ts` and `feedFilters.test.ts` in the same commit that removes the `displayFeed` client-side switch from `+page.svelte`.** One commit: remove client-side filtering, delete dead files. Makes the PR diff easier to review. - **Rewrite `page.server.spec.ts`** to cover the per-filter API call shape: ```ts it('passes kinds=FILE_UPLOADED when filter=hochgeladen', async () => { mockApi.GET.mockImplementation(...); await load({ fetch, url: buildUrl('?filter=hochgeladen') } as never); expect(mockApi.GET).toHaveBeenCalledWith('/api/dashboard/activity', { params: { query: { limit: 40, kinds: 'FILE_UPLOADED' } } }); }); it('omits kinds param when filter=alle', async () => { ... }); // etc. per filter ``` - **Keep `+page.svelte` usage of `data.activityFeed` direct** — no client-side `filterFeed()` call. The server-loaded feed is already filtered; the component just renders it.
marcel commented 2026-04-21 19:10:25 +02:00
Author
Owner
Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer (Round 2)

The first round covered the critical points: type the param as Set<AuditKind>, parameterized SQL binding, and SLF4J logging. No new vulnerabilities introduced. One clarification worth adding.

Observations

  • AuditLogQueryService is a thin pass-through — no security surface added there. The enum binding validation happens at the controller layer before anything reaches the service or repo.
  • youMentioned and youParticipated flags are computed inside the SQL query based on :currentUserId, not based on the kinds filter. Adding a kinds parameter to findRolledUpActivityFeed does not affect the privacy boundary for these flags — they remain correctly scoped to the authenticated user regardless of which kinds are requested.
  • The fuer-dich filter staying client-side (Markus's Option C from Round 1) means youMentioned and youParticipated are still returned in all API responses and filtered in the browser. This is acceptable — both flags are already personal to the requesting user, not cross-user data. No additional exposure.

Recommendations

  • No changes to the security model needed beyond what Round 1 specified. @RequirePermission(Permission.READ_ALL) on the class-level in DashboardController already gates the endpoint; kinds narrows but never widens that gate.
  • One test worth adding to DashboardControllerTest: verify that a user with only READ_ALL can call ?kinds=TEXT_SAVED and get 200, and a user without READ_ALL gets 403. The kinds param should not create a privilege escalation path — confirm this explicitly. 
    @Test void activity_with_kinds_returns_403_when_user_lacks_READ_ALL() { ... }
    This is a "permissions still work with the new param" regression guard, not a new security requirement.
## 🔒 Nora "NullX" Steiner — Application Security Engineer _(Round 2)_ The first round covered the critical points: type the param as `Set<AuditKind>`, parameterized SQL binding, and SLF4J logging. No new vulnerabilities introduced. One clarification worth adding. ### Observations - **`AuditLogQueryService` is a thin pass-through** — no security surface added there. The enum binding validation happens at the controller layer before anything reaches the service or repo. - **`youMentioned` and `youParticipated` flags are computed inside the SQL query** based on `:currentUserId`, not based on the kinds filter. Adding a `kinds` parameter to `findRolledUpActivityFeed` does not affect the privacy boundary for these flags — they remain correctly scoped to the authenticated user regardless of which kinds are requested. - **The `fuer-dich` filter staying client-side** (Markus's Option C from Round 1) means `youMentioned` and `youParticipated` are still returned in all API responses and filtered in the browser. This is acceptable — both flags are already personal to the requesting user, not cross-user data. No additional exposure. ### Recommendations - **No changes to the security model needed** beyond what Round 1 specified. `@RequirePermission(Permission.READ_ALL)` on the class-level in `DashboardController` already gates the endpoint; `kinds` narrows but never widens that gate. - **One test worth adding to `DashboardControllerTest`**: verify that a user with only `READ_ALL` can call `?kinds=TEXT_SAVED` and get 200, and a user without `READ_ALL` gets 403. The `kinds` param should not create a privilege escalation path — confirm this explicitly. ```java @Test void activity_with_kinds_returns_403_when_user_lacks_READ_ALL() { ... } ``` This is a "permissions still work with the new param" regression guard, not a new security requirement.
marcel commented 2026-04-21 19:10:36 +02:00
Author
Owner
Copy Link

🧪 Sara Holt — Senior QA Engineer (Round 2)

Round 1 covered repo integration tests and controller parse/validation tests well. Two test lifecycle issues were missed.

Observations

  • feedFilters.test.ts (9 tests) covers filterFeed() — a function that will be deleted. These tests will continue to pass after this PR because the function file still exists in isolation. But they test dead production behavior. Leaving them creates the illusion of coverage for code that no longer runs. They must be deleted in the same commit as feedFilters.ts.

  • page.server.spec.ts line 31 asserts { params: { query: { limit: 40 } } } — no kinds key. After the load function is updated to pass kinds per filter, any test that verifies the alle case will still pass (no kinds param is correct for alle), but any test for non-alle filters will fail or be missing. The current test suite has NO test for non-default filter behavior — the new tests need to be added and the old test for alle updated to confirm kinds is absent (not just unasserted).

  • DashboardService.getPulse() calls findActivityFeed with no kinds filter — this path has no test coverage for the overloaded signature introduced by this issue. Add one test asserting that getPulse() still receives all 6 rollable kinds even when the new overload exists.

Recommendations

  • Delete feedFilters.test.ts atomically with feedFilters.ts — same commit. The test file name should appear in the PR diff as a deletion, not remain as a green-but-dead suite.
  • Add to page.server.spec.ts (per filter): 
    it('passes kinds=FILE_UPLOADED when filter=hochgeladen');
it('passes kinds=TEXT_SAVED,BLOCK_REVIEWED,ANNOTATION_CREATED when filter=transkription');
it('passes kinds=COMMENT_ADDED,MENTION_CREATED when filter=kommentare');
it('omits kinds param when filter=alle');
it('applies client-side fuer-dich filter without passing kinds to the API');
  • Add DashboardServiceTest.pulse_uses_all_rollup_eligible_kinds() to guard the getPulse path after the overloaded signature lands.
## 🧪 Sara Holt — Senior QA Engineer _(Round 2)_ Round 1 covered repo integration tests and controller parse/validation tests well. Two test lifecycle issues were missed. ### Observations - **`feedFilters.test.ts` (9 tests) covers `filterFeed()` — a function that will be deleted.** These tests will continue to pass after this PR because the function file still exists in isolation. But they test dead production behavior. Leaving them creates the illusion of coverage for code that no longer runs. They must be deleted in the same commit as `feedFilters.ts`. - **`page.server.spec.ts` line 31** asserts `{ params: { query: { limit: 40 } } }` — no `kinds` key. After the load function is updated to pass `kinds` per filter, any test that verifies the `alle` case will still pass (no `kinds` param is correct for `alle`), but any test for non-`alle` filters will fail or be missing. The current test suite has NO test for non-default filter behavior — the new tests need to be added and the old test for `alle` updated to confirm `kinds` is absent (not just unasserted). - **`DashboardService.getPulse()` calls `findActivityFeed`** with no kinds filter — this path has no test coverage for the overloaded signature introduced by this issue. Add one test asserting that `getPulse()` still receives all 6 rollable kinds even when the new overload exists. ### Recommendations - **Delete `feedFilters.test.ts` atomically with `feedFilters.ts`** — same commit. The test file name should appear in the PR diff as a deletion, not remain as a green-but-dead suite. - **Add to `page.server.spec.ts`** (per filter): ```ts it('passes kinds=FILE_UPLOADED when filter=hochgeladen'); it('passes kinds=TEXT_SAVED,BLOCK_REVIEWED,ANNOTATION_CREATED when filter=transkription'); it('passes kinds=COMMENT_ADDED,MENTION_CREATED when filter=kommentare'); it('omits kinds param when filter=alle'); it('applies client-side fuer-dich filter without passing kinds to the API'); ``` - **Add `DashboardServiceTest.pulse_uses_all_rollup_eligible_kinds()`** to guard the getPulse path after the overloaded signature lands.
marcel commented 2026-04-21 19:10:47 +02:00
Author
Owner
Copy Link

🏗️ Tobias Wendt — DevOps & Platform Engineer (Round 2)

Round 1 noted no infrastructure surface changes and flagged the EXPLAIN ANALYZE check. One compilation-safety note for the service layer.

Observations

  • No new infrastructure, config, or deployment surface introduced by this issue. Confirmed: no new env vars, no port changes, no container changes.
  • The AuditLogQueryService.findActivityFeed() signature change affects DashboardService.getPulse() at compile time if implemented naively. The Maven build (./mvnw clean package -DskipTests) will catch this — but only if someone runs it before the PR is merged. If the overloaded method approach is used (as Markus recommends), the build is safe. Worth verifying: run a full backend build as part of the PR checklist.
  • Metric cardinality remains safe: confirmed in Round 1. The /api/dashboard/activity URI template is what Micrometer records; query params don't become label values. No Prometheus explosion risk.

Recommendations

  • Add ./mvnw clean package -DskipTests to the PR checklist before opening review. The getPulse-vs-getActivity call site split is a compile-time risk — a full build catches it in seconds and prevents a CI failure from a caller that's easy to miss during code review.
  • EXPLAIN ANALYZE (repeated from Round 1 for completeness): run it on the filtered query with a single-kind IN clause before merge to confirm the V49 partial index is still chosen. This is a one-time manual check, not a CI gate.
  • No Renovate or dependency changes required — Spring Data's Collection binding for native IN clauses has been stable since Spring Data 2.x.
## 🏗️ Tobias Wendt — DevOps & Platform Engineer _(Round 2)_ Round 1 noted no infrastructure surface changes and flagged the EXPLAIN ANALYZE check. One compilation-safety note for the service layer. ### Observations - **No new infrastructure, config, or deployment surface** introduced by this issue. Confirmed: no new env vars, no port changes, no container changes. - **The `AuditLogQueryService.findActivityFeed()` signature change affects `DashboardService.getPulse()`** at compile time if implemented naively. The Maven build (`./mvnw clean package -DskipTests`) will catch this — but only if someone runs it before the PR is merged. If the overloaded method approach is used (as Markus recommends), the build is safe. Worth verifying: run a full backend build as part of the PR checklist. - **Metric cardinality remains safe**: confirmed in Round 1. The `/api/dashboard/activity` URI template is what Micrometer records; query params don't become label values. No Prometheus explosion risk. ### Recommendations - **Add `./mvnw clean package -DskipTests` to the PR checklist** before opening review. The getPulse-vs-getActivity call site split is a compile-time risk — a full build catches it in seconds and prevents a CI failure from a caller that's easy to miss during code review. - **EXPLAIN ANALYZE** (repeated from Round 1 for completeness): run it on the filtered query with a single-kind `IN` clause before merge to confirm the V49 partial index is still chosen. This is a one-time manual check, not a CI gate. - **No Renovate or dependency changes** required — Spring Data's `Collection` binding for native `IN` clauses has been stable since Spring Data 2.x.
marcel commented 2026-04-21 19:11:00 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UX/Design Lead (Round 2)

Round 1 covered loading state, empty-state copy, and aria-live on filter change. One URL canonicalization detail affects the default filter UX.

Observations

  • The alle filter must produce a clean /chronik URL, not /chronik?kinds= or /chronik?filter=alle. Looking at +page.server.ts, the load function currently reads url.searchParams.get('filter') and defaults to 'alle'. The navigation already works via goto?filter=... — when the user returns to alle, the URL should drop the filter param entirely (or the pill should navigate to /chronik rather than /chronik?filter=alle). This keeps the canonical URL clean and avoids a jarring visual difference between a freshly loaded page and a "reset to all" click.
  • The fuer-dich filter staying client-side (Markus Option C) has a subtle UX implication: after a hard reload on /chronik?filter=fuer-dich, the server returns all 40 items (no kinds filter sent), and the client-side filterFeed applies the youMentioned/youParticipated predicate. If the user has few mentions, the page may flash a full list then snap to a short one. This is cosmetically tolerable but worth calling out so the implementation is intentional, not accidental.
  • Empty state when server returns [] for a narrow filter: Round 1 confirmed the copy "Nichts in dieser Ansicht" still works. Verify the empty state is rendered from data.activityFeed.length === 0 directly — not from filterFeed(data.activityFeed, filter).length === 0. After this change, the server result is already filtered; no client-side predicate should gate the empty state for server-filtered pills.

Recommendations

  • For alle, navigate to /chronik (no query param) rather than /chronik?filter=alle. The pill component's href for alle should be "/chronik" not "/chronik?filter=alle". The load function already defaults to 'alle' when the param is absent — no server change needed.
  • Document the fuer-dich flash consciously: add a brief comment in the load function or the pill component noting that fuer-dich is intentionally client-side filtered because it relies on youMentioned/youParticipated flags. This prevents a future developer from "fixing" it by adding a server-side path that can't express the same predicate.
  • No pill component or design system changes needed — the interaction model from PR #288 is unchanged; only the data source behind each pill changes.
## 🎨 Leonie Voss — UX/Design Lead _(Round 2)_ Round 1 covered loading state, empty-state copy, and aria-live on filter change. One URL canonicalization detail affects the default filter UX. ### Observations - **The `alle` filter must produce a clean `/chronik` URL**, not `/chronik?kinds=` or `/chronik?filter=alle`. Looking at `+page.server.ts`, the load function currently reads `url.searchParams.get('filter')` and defaults to `'alle'`. The navigation already works via `goto?filter=...` — when the user returns to `alle`, the URL should drop the `filter` param entirely (or the pill should navigate to `/chronik` rather than `/chronik?filter=alle`). This keeps the canonical URL clean and avoids a jarring visual difference between a freshly loaded page and a "reset to all" click. - **The `fuer-dich` filter staying client-side** (Markus Option C) has a subtle UX implication: after a hard reload on `/chronik?filter=fuer-dich`, the server returns all 40 items (no kinds filter sent), and the client-side `filterFeed` applies the `youMentioned/youParticipated` predicate. If the user has few mentions, the page may flash a full list then snap to a short one. This is cosmetically tolerable but worth calling out so the implementation is intentional, not accidental. - **Empty state when server returns `[]` for a narrow filter**: Round 1 confirmed the copy `"Nichts in dieser Ansicht"` still works. Verify the empty state is rendered from `data.activityFeed.length === 0` directly — not from `filterFeed(data.activityFeed, filter).length === 0`. After this change, the server result is already filtered; no client-side predicate should gate the empty state for server-filtered pills. ### Recommendations - **For `alle`, navigate to `/chronik` (no query param)** rather than `/chronik?filter=alle`. The pill component's `href` for `alle` should be `"/chronik"` not `"/chronik?filter=alle"`. The load function already defaults to `'alle'` when the param is absent — no server change needed. - **Document the `fuer-dich` flash consciously**: add a brief comment in the load function or the pill component noting that `fuer-dich` is intentionally client-side filtered because it relies on `youMentioned/youParticipated` flags. This prevents a future developer from "fixing" it by adding a server-side path that can't express the same predicate. - **No pill component or design system changes needed** — the interaction model from PR #288 is unchanged; only the data source behind each pill changes.
marcel commented 2026-04-21 20:46:32 +02:00
Author
Owner
Copy Link

🎯 Discussion Resolutions

After reviewing the persona feedback with the user, here are the agreed decisions:

Theme 1 — Service layer: AuditLogQueryService dual-caller split

  • Decision: Add an overloaded findActivityFeed in AuditLogQueryService. The existing two-arg signature (UUID, int) stays in place for getPulse — untouched. A new three-arg signature (UUID, int, Set<AuditKind>) is used by getActivity.
  • Rationale: getPulse must always receive all 6 rollable kinds; getActivity receives whatever the filter pill requests. Overloading keeps both call sites clean without special-casing.
  • Checklist: Run ./mvnw clean package -DskipTests before opening the PR to catch any compile-time breakage.
  • Test: Add DashboardServiceTest.pulse_uses_all_rollup_eligible_kinds() to guard the getPulse path.

Theme 2 — SQL: WHERE clause parameterization and index safety

  • Decision: Add WHERE a.kind IN (:kinds) at the events CTE level (innermost), not the outer aggregation.
  • Rationale: Filtering at events means fewer rows enter the LAG/session CTEs. The V49 partial index remains applicable — Postgres recognizes a subset predicate.
  • Checklist: Run EXPLAIN ANALYZE with a single-kind IN clause before merge to confirm idx_audit_log_rollup is chosen, not a seq scan.

Theme 3 — Controller API design

  • Decision: Controller param typed as @RequestParam(required = false) Set<AuditKind> kinds. Introduce AuditKind.ROLLUP_ELIGIBLE as a Set<AuditKind> constant of the 6 eligible kinds. When kinds is null, default to ROLLUP_ELIGIBLE.
  • Rationale: Spring's enum binding validates values automatically (400 on unknown). ROLLUP_ELIGIBLE eliminates one of the three duplications of the six-kind list. Null default keeps existing callers (dashboard side-rail) working without changes.
  • Logging: Log rejected enum values with SLF4J {} placeholder — never string concatenation.

Theme 4 — Empty kinds set semantics

  • Decision: Empty kinds (e.g. ?kinds=) is treated the same as a missing param — returns default all-6.
  • Rationale: The frontend never sends an empty set; this is purely a robustness decision. Returning all-6 is more forgiving than an empty list and has no practical downside.

Theme 5 — "Für dich" filter shape

  • Decision: Keep "Für dich" as a client-side-only filter (Option C). No kinds param sent to the server; the server returns all 40 rows; the client applies youMentioned || youParticipated predicate.
  • Rationale: The predicate relies on user-scoped flags, not kind values — it cannot be expressed as a pure kinds filter.
  • Note: Add a short comment in the load function or pill component explaining this is intentional, so a future developer doesn't try to move it server-side.

Theme 6 — Dead code deletion

  • Decision: Delete feedFilters.ts and feedFilters.test.ts in the same commit that removes the filterFeed() call from +page.svelte.
  • Rationale: Once the server handles filtering, filterFeed() is unused. feedFilters.test.ts (9 tests) would continue passing but test behavior that no longer exists in production — a maintenance trap.

Theme 7 — Frontend +page.server.ts simplification

  • Decision: Kinds mapping lives in +page.server.ts (server concern, not rendering concern). Confirmed mapping:
    • alle → omit kinds param (server defaults to all 6)
    • hochgeladen → ['FILE_UPLOADED']
    • transkription → ['TEXT_SAVED', 'BLOCK_REVIEWED', 'ANNOTATION_CREATED']
    • kommentare → ['COMMENT_ADDED', 'MENTION_CREATED']
    • fuer-dich → omit kinds param (client-side predicate)
  • Remove activeFilter = $state + $effect block from +page.svelte — filter is pure URL → load → render.
  • "alle" pill href is /chronik (no query param) for a clean canonical URL.

Theme 8 — Existing test breakage

  • Decision: Rewrite the existing page.server.spec.ts test (line 31) rather than adding alongside it. Explicitly assert kinds is absent for alle; add one test per non-alle filter asserting the exact kinds CSV.

Theme 9 — New test coverage

Backend repo (AuditLogQueryRepositoryRolledUpTest):

  • rolledUpFeed_with_single_kind_returns_only_that_kind
  • rolledUpFeed_with_multiple_kinds_returns_union
  • rolledUpFeed_with_default_returns_all_six_eligible_kinds
  • rolledUpFeed_excludes_rows_not_in_filter_set
  • rolledUpFeed_rollup_still_works_when_kind_set_is_filtered_to_single_rollable_kind

Backend controller (DashboardControllerTest):

  • activity_parses_kinds_csv_into_set
  • activity_returns_400_when_kinds_contains_invalid_enum_value
  • activity_uses_default_set_when_kinds_param_absent
  • activity_treats_empty_kinds_as_default
  • activity_with_kinds_returns_403_when_user_lacks_READ_ALL

Backend service (DashboardServiceTest):

  • pulse_uses_all_rollup_eligible_kinds

Frontend (page.server.spec.ts): per-filter kinds assertions (alle, hochgeladen, transkription, kommentare, fuer-dich).

Theme 10 — UX/A11y

  • Decision: Add aria-busy="true" on the timeline wrapper during SvelteKit navigation; optional 3-row skeleton after 200ms (debounced). Empty state rendered from data.activityFeed.length === 0 directly (not from filterFeed result). Add aria-live="polite" announcement on filter change.

Theme 11 — Operational

  • No infrastructure, config, or deployment changes. Metric cardinality safe. Renovate safe.

These resolutions now act as the authoritative design for implementation. The implement skill will read this comment alongside the original issue.

# 🎯 Discussion Resolutions After reviewing the persona feedback with the user, here are the agreed decisions: ## Theme 1 — Service layer: `AuditLogQueryService` dual-caller split - **Decision**: Add an overloaded `findActivityFeed` in `AuditLogQueryService`. The existing two-arg signature `(UUID, int)` stays in place for `getPulse` — untouched. A new three-arg signature `(UUID, int, Set<AuditKind>)` is used by `getActivity`. - **Rationale**: `getPulse` must always receive all 6 rollable kinds; `getActivity` receives whatever the filter pill requests. Overloading keeps both call sites clean without special-casing. - **Checklist**: Run `./mvnw clean package -DskipTests` before opening the PR to catch any compile-time breakage. - **Test**: Add `DashboardServiceTest.pulse_uses_all_rollup_eligible_kinds()` to guard the getPulse path. ## Theme 2 — SQL: WHERE clause parameterization and index safety - **Decision**: Add `WHERE a.kind IN (:kinds)` at the `events` CTE level (innermost), not the outer aggregation. - **Rationale**: Filtering at `events` means fewer rows enter the LAG/session CTEs. The V49 partial index remains applicable — Postgres recognizes a subset predicate. - **Checklist**: Run EXPLAIN ANALYZE with a single-kind IN clause before merge to confirm `idx_audit_log_rollup` is chosen, not a seq scan. ## Theme 3 — Controller API design - **Decision**: Controller param typed as `@RequestParam(required = false) Set<AuditKind> kinds`. Introduce `AuditKind.ROLLUP_ELIGIBLE` as a `Set<AuditKind>` constant of the 6 eligible kinds. When `kinds` is null, default to `ROLLUP_ELIGIBLE`. - **Rationale**: Spring's enum binding validates values automatically (400 on unknown). `ROLLUP_ELIGIBLE` eliminates one of the three duplications of the six-kind list. Null default keeps existing callers (dashboard side-rail) working without changes. - **Logging**: Log rejected enum values with SLF4J `{}` placeholder — never string concatenation. ## Theme 4 — Empty `kinds` set semantics - **Decision**: Empty `kinds` (e.g. `?kinds=`) is treated the same as a missing param — returns default all-6. - **Rationale**: The frontend never sends an empty set; this is purely a robustness decision. Returning all-6 is more forgiving than an empty list and has no practical downside. ## Theme 5 — "Für dich" filter shape - **Decision**: Keep "Für dich" as a client-side-only filter (Option C). No `kinds` param sent to the server; the server returns all 40 rows; the client applies `youMentioned || youParticipated` predicate. - **Rationale**: The predicate relies on user-scoped flags, not kind values — it cannot be expressed as a pure `kinds` filter. - **Note**: Add a short comment in the load function or pill component explaining this is intentional, so a future developer doesn't try to move it server-side. ## Theme 6 — Dead code deletion - **Decision**: Delete `feedFilters.ts` and `feedFilters.test.ts` in the same commit that removes the `filterFeed()` call from `+page.svelte`. - **Rationale**: Once the server handles filtering, `filterFeed()` is unused. `feedFilters.test.ts` (9 tests) would continue passing but test behavior that no longer exists in production — a maintenance trap. ## Theme 7 — Frontend `+page.server.ts` simplification - **Decision**: Kinds mapping lives in `+page.server.ts` (server concern, not rendering concern). Confirmed mapping: - `alle` → omit `kinds` param (server defaults to all 6) - `hochgeladen` → `['FILE_UPLOADED']` - `transkription` → `['TEXT_SAVED', 'BLOCK_REVIEWED', 'ANNOTATION_CREATED']` - `kommentare` → `['COMMENT_ADDED', 'MENTION_CREATED']` - `fuer-dich` → omit `kinds` param (client-side predicate) - Remove `activeFilter = $state + $effect` block from `+page.svelte` — filter is pure URL → load → render. - "alle" pill href is `/chronik` (no query param) for a clean canonical URL. ## Theme 8 — Existing test breakage - **Decision**: Rewrite the existing `page.server.spec.ts` test (line 31) rather than adding alongside it. Explicitly assert `kinds` is absent for `alle`; add one test per non-alle filter asserting the exact kinds CSV. ## Theme 9 — New test coverage **Backend repo** (`AuditLogQueryRepositoryRolledUpTest`): - `rolledUpFeed_with_single_kind_returns_only_that_kind` - `rolledUpFeed_with_multiple_kinds_returns_union` - `rolledUpFeed_with_default_returns_all_six_eligible_kinds` - `rolledUpFeed_excludes_rows_not_in_filter_set` - `rolledUpFeed_rollup_still_works_when_kind_set_is_filtered_to_single_rollable_kind` **Backend controller** (`DashboardControllerTest`): - `activity_parses_kinds_csv_into_set` - `activity_returns_400_when_kinds_contains_invalid_enum_value` - `activity_uses_default_set_when_kinds_param_absent` - `activity_treats_empty_kinds_as_default` - `activity_with_kinds_returns_403_when_user_lacks_READ_ALL` **Backend service** (`DashboardServiceTest`): - `pulse_uses_all_rollup_eligible_kinds` **Frontend** (`page.server.spec.ts`): per-filter kinds assertions (alle, hochgeladen, transkription, kommentare, fuer-dich). ## Theme 10 — UX/A11y - **Decision**: Add `aria-busy="true"` on the timeline wrapper during SvelteKit navigation; optional 3-row skeleton after 200ms (debounced). Empty state rendered from `data.activityFeed.length === 0` directly (not from `filterFeed` result). Add `aria-live="polite"` announcement on filter change. ## Theme 11 — Operational - No infrastructure, config, or deployment changes. Metric cardinality safe. Renovate safe. --- These resolutions now act as the authoritative design for implementation. The `implement` skill will read this comment alongside the original issue.
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#293
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?