feat(dashboard): add kinds CSV query param to /api/dashboard/activity
#302
Reference in New Issue
Block a user
Delete Branch "feat/issue-293-dashboard-activity-kinds-filter"
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?
Closes #293
Summary
Moves Chronik filter-pill logic from client-side (filtering 40 pre-loaded items) to server-side (each pill fetches up to 40 matching rows of its own kind set).
ROLLUP_ELIGIBLEconstant toAuditKindas the single source of truth for the 6 rollable kindsCollection<String> kindsparam toAuditLogQueryRepository.findRolledUpActivityFeed— filter applied at the innermosteventsCTE so it reduces rows entering the LAG/session CTEsfindActivityFeed(UUID, int, Set<AuditKind>)overload toAuditLogQueryService; existing 2-arg delegates to it withROLLUP_ELIGIBLEsogetPulseis unaffectedDashboardController.getActivityaccepts?kinds=FILE_UPLOADED,TEXT_SAVED(Spring auto-converts toSet<AuditKind>; unknown value → 400; absent/empty → ROLLUP_ELIGIBLE)+page.server.tsmaps eachFilterValueto its kinds set;alleandfuer-dichomit the param (server defaults to all 6)fuer-dichstays client-side:youMentioned || youParticipatedis a user-scoped predicate that cannot be expressed as akindsfilterfeedFilters.tsand its 9 tests (dead code)aria-live="polite"+aria-busy={!!navigating.type}on timeline regionkindsparam visible in the spec- Delete feedFilters.ts and its 9 tests (dead code: server now filters) - Remove activeFilter $state + $effect — read data.filter directly - fuer-dich stays client-side via youMentioned/youParticipated predicate - aria-live="polite" + aria-busy={!!navigating.type} on timeline region Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🏛️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
The architectural direction is correct: moving filter logic to the server eliminates the mismatch between "client fetches 40 items then filters to 3" and "user expects 40 matching items". The
ROLLUP_ELIGIBLEconstant as a single source of truth, the 2-arg/3-arg overload to preservegetPulsebehavior, and theKINDS_FOR_FILTERmapping in+page.server.tsare all clean, maintainable decisions.Concerns
api.ts regeneration silently removed 6 comment-route operations (
getDocumentComments,postDocumentComment,replyToDocumentComment,getAnnotationComments,postAnnotationComment,replyToAnnotationComment). The prior file had a// MANUALLY ADDEDguard specifically because these routes were being dropped on regeneration. Now both the guard and the types are gone. Any frontend component calling these viacreateApiClienthas lost compile-time coverage. The runtime behavior is unchanged (raw fetch still works), but the TypeScript protection is silently missing.Before merging: confirm that no component uses
api.GET('/api/documents/{documentId}/comments')etc. via the typed client, and decide whether to restore the routes or leave a documented known-gap comment.Blockers
AuditLogQueryServiceTest.javalives in thedashboardpackage but testsAuditLogQueryServicefrom theauditdomain. Per this project's package-by-feature convention, it belongs inorg.raddatz.familienarchiv.audit.Suggestions
dashboardpackage now importsAuditKinddirectly (import org.raddatz.familienarchiv.audit.AuditKind). This is fine — the dependency direction is dashboard → audit, which is correct. No issue here, just confirming the boundary is respected.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: 🚫 Changes requested
The backend work is clean: the overload pattern avoids breaking
getPulse,ROLLUP_ELIGIBLEis a proper constant, and the null-guard in the controller prevents an emptyIN ()SQL error. TDD evidence is present throughout.Blockers
1. Behavior regression in "für dich" predicate
The old
feedFilters.tsincludedMENTION_CREATEDitems unconditionally:The new
+page.sveltepredicate is:If a
MENTION_CREATEDrow has both flags false (e.g. the backend doesn't set them for this user), it silently disappears from "für dich". The 9 deletedfeedFilters.test.tstests had explicit coverage for this case. The new Svelte predicate has zero test coverage. Either confirm the backend always setsyouMentioned: trueonMENTION_CREATEDitems (and add a test for it), or preserve thekind === 'MENTION_CREATED'guard.2. api.ts: comment operations removed
The regeneration dropped
getDocumentComments,postDocumentComment,replyToDocumentComment,getAnnotationComments,postAnnotationComment,replyToAnnotationComment. The prior file had:Both the comment and the routes are now gone. This is exactly the scenario the comment warned about. Verify these routes are in the live spec (the backend was running with a specific profile during regeneration — were the comment controllers active?) and restore the missing operations.
Suggestions
3.
AuditLogQueryServiceTestin wrong packagebackend/src/test/java/org/raddatz/familienarchiv/dashboard/AuditLogQueryServiceTest.javatestsorg.raddatz.familienarchiv.audit.AuditLogQueryService. Move toorg.raddatz.familienarchiv.audit.4. Unchecked cast in
DashboardControllerTestany(Set.class)produces an unchecked cast. PreferArgumentMatchers.<Set<AuditKind>>any()for type-safe matching.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Reviewed the new attack surface introduced by the
kindsquery parameter.What I checked
Spring enum binding —
@RequestParam Set<AuditKind> kindsuses Spring's built-inConversionService. An unknown value (e.g.?kinds=DROP_TABLE) causes Spring to return HTTP 400 before the controller method runs. No injection path. TheDashboardControllerTest.activity_returns400_forUnknownKindValuetest confirms this path.SQL IN clause — The
:kindsCollection parameter infindRolledUpActivityFeedis passed as aList<String>through Spring Data JPA's native query parameterization. JPA handles list expansion into positional bind parameters. No string concatenation, no injection vector.Empty list guard —
(kinds == null || kinds.isEmpty()) ? AuditKind.ROLLUP_ELIGIBLE : kindsprevents an emptyIN ()from reaching PostgreSQL (which would throw a syntax error). This is the correct fail-safe default.OpenAPI spec — The
kindsparameter is constrained to the enum values in the spec. This limits what clients can send without needing additional validation in the controller.No new concerns
Existing
@RequirePermission(Permission.READ_ALL)on the dashboard controller is preserved. No new endpoints, no new permission surfaces, no secrets or credentials touched.🧪 Sara Holt — Senior QA Engineer
Verdict: 🚫 Changes requested
The backend test additions are solid. The
AuditLogQueryRepositoryRolledUpTestkinds-filter section covers single-kind, multi-kind, union, exclusion, and rollup-within-filter scenarios — that's the right level of integration coverage. TheDashboardControllerTestnew cases (CSV parsing, invalid kind → 400, default behavior, single-kind) form a complete slice.Blockers
1. "für dich" client-side predicate has no test coverage
feedFilters.test.tscontained 5 tests for theyouMentioned || youParticipatedpredicate (including the MENTION_CREATED edge case). Those 9 tests were deleted and no equivalent tests for the new+page.svelteinline predicate were added. The predicate is now untested:A Vitest test for
+page.svelteis complex, butpage.server.spec.tscould verify the filter value passes through correctly, or a small pure-function helper could be extracted and tested directly. At minimum, the old behaviorkind === 'MENTION_CREATED'should be documented as intentionally removed (with a reason).2. api.ts regression may silently break component tests
If comment-fetching components have Vitest tests that mock
createApiClient, those mocks referencegetDocumentCommentsetc. by operation name. With those operations removed fromapi.ts, the TypeScript layer no longer catches wrong call signatures. Verify withnpm run checkin the worktree.Suggestions
3.
ALL_ELIGIBLE_KINDSduplicatesAuditKind.ROLLUP_ELIGIBLEIn
AuditLogQueryRepositoryRolledUpTest:This is a manually maintained duplicate. If a new kind is added to
ROLLUP_ELIGIBLE, these tests won't catch the gap. Consider:4.
rolledUpFeed_with_default_returns_all_six_eligible_kindsmay be fragileThe 6 events are inserted 60 seconds apart. The rollup window likely groups some of them (TEXT_SAVED + FILE_UPLOADED on the same doc in the same session = 1 row). The test asserts
hasSize(6), which could fail if the session window changes. Verify or add comments explaining the expected rollup behavior.⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR. Checked:
What I noticed
Database performance — The
kindsfilter is applied at the innermosteventsCTE in the native query, which is the right place: it reduces the row set before the LAG-based session grouping and rollup happen. PostgreSQL should be able to use an index scan onaudit_logfor the kind predicate if an appropriate index exists. The PR description mentions EXPLAIN ANALYZE, but I don't see evidence it was run. This is non-blocking for merge but worth doing before the feature is under production load — add it to a follow-up ticket if not already done.No regressions for
getPulse— The 2-arg overload (findActivityFeed(userId, limit)) delegates to ROLLUP_ELIGIBLE, so the home page pulse widget is unaffected.Everything looks operationally sound.
🎨 Leonie Voss — UX & Accessibility Lead
Verdict: ✅ Approved
The frontend changes are an accessibility improvement, not a regression.
What's good
aria-live="polite"+aria-busy={!!navigating.type}on the timeline region — This is exactly right. When a user clicks a filter pill, the browser starts a navigation. Thearia-busyattribute tells assistive technology to hold announcements until the update is complete. Once navigation completes and the new content renders,aria-live="polite"announces the region update without interrupting ongoing speech. WCAG 4.1.3 (Status Messages) is now satisfied for filter changes.aria-atomic="false"is correct — The timeline can have many items;atomic="false"means the screen reader announces individual changes, not the entire region re-read. Right choice for a feed.Filter pill now uses
data.filter(server-authoritative) — The oldactiveFilter$state + $effect pattern created a brief visual desync where the pill showed the new filter before the server confirmed it. Now the pill state tracks server reality. Consistent visual feedback for all users.One non-blocking suggestion
When
navigating.typeis truthy, thearia-busyattribute signals "updating" to screen readers — but sighted users have no equivalent signal. Consider adding a visual loading indicator onChronikFilterPills(a subtle spinner on the active pill, or a progress bar under the header) when navigation is in progress. This benefits keyboard-only and low-vision users who can see but not hear ARIA announcements. Not a blocker, but a good follow-up.Not checked (out of scope for this PR)
Touch target sizes and contrast ratios on
ChronikFilterPills— those were pre-existing and not changed here.Resolution follow-up (3 fix commits pushed:
42cf7715,c0e52b84,ba0f9bb3)Felix & Sara — "für dich" predicate now tested
Extracted the client-side predicate to
frontend/src/routes/chronik/clientFilter.tswith 11 tests inclientFilter.test.ts. The+page.sveltenow callsapplyClientFilter(data.activityFeed, data.filter).The behavior change from the old
feedFilters.tsis intentional and documented in the test:The old
kind === 'MENTION_CREATED'check was wrong: it showed mentions directed at other users in the current user's "für dich" feed. The backend setsyouMentioned: truespecifically for MENTION_CREATED events targeting the current user, so that flag is the correct signal.Felix & Markus — api.ts comment routes
Verified:
getDocumentComments,postDocumentComment, etc. were never backed by real backend routes.CommentController.javaonly has/api/documents/{documentId}/transcription-blocks/{blockId}/comments— document-level and annotation-level comment routes don't exist. The regeneration correctly removed them.CommentThread.svelteuses rawfetch()and is unaffected.The
// MANUALLY ADDEDnote was guarding thecommentId/annotationIdfields onActivityFeedItemDTO. Those fields are now in the backend spec and appear correctly in the newapi.ts, so the guard was no longer needed.Markus — AuditLogQueryServiceTest package
Moved to
org.raddatz.familienarchiv.audit.AuditLogQueryServiceTest(commitc0e52b84).Felix — any(Set.class) unchecked cast
Replaced all occurrences with
any()inDashboardControllerTest(commitba0f9bb3).Sara — ALL_ELIGIBLE_KINDS duplication
ALL_ELIGIBLE_KINDSinAuditLogQueryRepositoryRolledUpTestnow derives fromAuditKind.ROLLUP_ELIGIBLE.stream().map(Enum::name).toList()(commitba0f9bb3).Sara — rolledUpFeed_with_default_returns_all_six_eligible_kinds may be fragile
Verified: the 6 events are on different kinds per row in the rollup, and each is separated by a large enough time gap (7300–7420 seconds apart from the TEXT_SAVED event) to fall outside any single session window. The test asserts
hasSize(6)which is stable as long as these events aren't rolled up together — which they won't be since rollup is per-kind within a time window. The test is safe as-is.🏛️ Markus Keller — Senior Application Architect (Cycle 2)
Verdict: ✅ Approved
All concerns from the first review are resolved.
AuditLogQueryServiceTestis now inorg.raddatz.familienarchiv.audit— correct package placement, boundary respectedROLLUP_ELIGIBLEis the single source of truth throughout: the constant inAuditKind, the test helper derived from it, and the 2-arg overload all convergeThe overall architecture is sound: server-side filtering with a client-side
fuer-dichnarrowing pass is the right model for this use case. The layer dependencies are clean (dashboard → auditis the only cross-domain coupling, and it's the correct direction).👨💻 Felix Brandt — Senior Fullstack Developer (Cycle 2)
Verdict: ✅ Approved
All blockers from cycle 1 are resolved.
clientFilter.ts+clientFilter.test.ts— The extraction is clean. Pure function, single concern, directly testable. The test forMENTION_CREATEDwithout theyouMentionedflag explicitly documents the behavior change with the reasoning in the comment body:This is exactly the right level of documentation for a non-obvious behavior decision.
+page.svelte—const displayFeed = $derived(applyClientFilter(data.activityFeed, data.filter))is a single-expression$derived. Clean.DashboardControllerTest—any()is correct. No unchecked cast.Package placement —
AuditLogQueryServiceTestis now where it belongs.🔒 Nora "NullX" Steiner — Application Security Engineer (Cycle 2)
Verdict: ✅ Approved
No new security surface introduced in the fix commits. The
clientFilter.tsmodule is a pure client-side predicate with no I/O, no external calls, and no user input handling. Nothing to flag from a security perspective.Previous approval on the backend kinds parameter handling stands.
🧪 Sara Holt — Senior QA Engineer (Cycle 2)
Verdict: ✅ Approved
All test coverage concerns from cycle 1 are resolved.
clientFilter.test.ts— 11 tests across 3describeblocks. Coverage includes:youMentioned: trueinclusionyouParticipated: trueinclusionMENTION_CREATEDwithout flags → excluded (behavioral regression risk documented and tested)MENTION_CREATEDwithyouMentioned: true→ includedThat's the complete behavior matrix. Well done.
ALL_ELIGIBLE_KINDSnow derives fromAuditKind.ROLLUP_ELIGIBLE.stream().map(Enum::name).toList()— any future kind addition to the constant automatically propagates to the integration test. Correct fix.rolledUpFeed_with_default_returns_all_six_eligible_kinds— accepted as-is, per the analysis in the resolution comment.⚙️ Tobias Wendt — DevOps & Platform Engineer (Cycle 2)
Verdict: ✅ Approved
No infrastructure changes. No new environment variables. Fix commits are test/refactor only. Previous approval stands.
🎨 Leonie Voss — UX & Accessibility Lead (Cycle 2)
Verdict: ✅ Approved
Fix commits don't touch UI or accessibility. The
aria-live/aria-busyimprovement from cycle 1 is intact. TheapplyClientFilterrefactor is internal JavaScript only — no markup changes.Previous approval stands.