fix(chronik): surface REPLY events in Für-dich feed via youParticipated #297
Reference in New Issue
Block a user
Delete Branch "feat/issue-295-you-participated"
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?
Summary
youParticipatedflag toActivityFeedItemDTO, computed via a correlatedEXISTSsubquery against thenotificationstable (no schema migration needed)fuer-dichfilter in the Chronik to includeyouParticipated, soCOMMENT_ADDEDevents now appear for thread participants — consistent with the notification bell's REPLY behaviourfilterFeed()to a pure function inchronik/feedFilters.tsfor testabilityCloses #295
Changes
audit/ActivityFeedRow.java+isYouParticipated()audit/AuditLogQueryRepository.java+you_participatedEXISTS subquery;+payloadviaMIN(payload::text)::jsonbin aggregated CTEdashboard/ActivityFeedItemDTO.java+youParticipatedfield with@Schema(requiredMode = REQUIRED)dashboard/DashboardService.javarow.isYouParticipated()into DTO constructorfrontend/src/lib/generated/api.ts+youParticipated: booleanonActivityFeedItemDTOfrontend/src/routes/chronik/feedFilters.tsfilterFeed()with widened fuer-dich predicateAuditLogQueryRepositoryRolledUpTest.javayouParticipatedtrue/false + retroactiveyouMentionedcoveragefrontend/.../feedFilters.test.tsfrontend/src/routes/chronik/+page.sveltefilterFeed()Test plan
./mvnw test— 1196 tests, 0 failuresnpm run test(server project) — 318 tests, 0 failuresCOMMENT_ADDEDeventMapper populates uploadedAt from Document.createdAt so the dashboard enrichment block can show a relative-time meta line ("vor 2 Min.") per issue #296. LocalDateTime matches the convention used by NotificationDTO, DocumentVersionSummary and InviteListItemDTO. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
What I checked
Domain boundary crossing, CTE payload aggregation correctness, migration necessity, layering.
Blockers
None. The deliberate cross-domain query decision was signed off in issue #295 and the SQL comment names the contract:
That is exactly what I asked for. ✅
Suggestions
1.
MIN(payload::text)::jsonbneeds a WHY comment —AuditLogQueryRepository.javaThe query carries payload through the
aggregatedCTE usingMIN(payload::text)::jsonb. This is semantically correct becauseCOMMENT_ADDEDandMENTION_CREATEDare never rolled up (each hasis_new_session = 1unconditionally), so each session contains exactly one event andMINis the identity. However, that invariant is not stated anywhere near the SQL. A future developer who touches the session-marking logic could break this silently.Without this comment, the correctness of
youParticipateddepends on a non-obvious invariant in a different part of the same query.2. The PR diff contains out-of-scope changes
The diff includes changes to
DocumentController.java,IncompleteDocumentDTO.java,DocumentService.java, and their tests. These are from issue #296, already merged to main. Because this branch was cut from a point slightly before those commits, they appear in the PR diff.This is a cosmetic issue for review purposes (the changes are already in main and will merge cleanly), but it makes the diff harder to reason about. Before merging: confirm
git merge-base HEAD mainand verify no actual regressions are introduced. A quick rebase onto current main tip would clean this up.What's done well
CAST(:currentUserId AS uuid)instead of::uuidsuffix is the correct workaround for Spring Data JPA parameter parsing. Good catch.@Schema(requiredMode = REQUIRED)consistent with the rest of the record.DashboardService.getActivity()stays a pure mapper — no business logic leak.🔒 Nora Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
What I checked
IDOR risk on
youParticipated, SQL injection in native query, scoping correctness, cross-user data leakage.Blockers
None. The primary risk I was watching for — user A seeing
youParticipated = truefor a comment thread they are not part of — is correctly prevented:n.recipient_id = CAST(:currentUserId AS uuid)binds the EXISTS subquery to the querying user. The CAST() approach also avoids parameter injection via the::uuidsuffix workaround. Parameterized query throughout. ✅Concerns (not blockers, but should be addressed)
1. Missing cross-user scoping regression test —
AuditLogQueryRepositoryRolledUpTest.javaThe current test suite covers:
youParticipated_is_true_when_user_has_reply_notification_for_comment— user has a REPLY notification ✅youParticipated_is_false_for_comment_with_no_reply_notification— no notification exists ✅What is not tested: a notification exists in the
notificationstable for the same comment but belonging to a different user. This is the exact IDOR case. The subquery should not let that notification bleed through.This test is worth adding as a permanent regression. The current
recipient_id = CAST(:currentUserId AS uuid)predicate correctly prevents the leak, but without this test there is no machine-readable proof it does.2. Hand-rolled JSON in test helper —
AuditLogQueryRepositoryRolledUpTest.javaThis is test code, not production, so no exploitable surface. But if a future test passes a value containing a double quote (e.g.
"comment's \"text\"") the resulting JSON is malformed and the test will fail with a confusing JSONB parse error rather than a clear assertion failure. Use Jackson:This is already a dependency in the test classpath.
What's done well
recipient_id = CAST(:currentUserId AS uuid)correctly scopes to the requesting user.reference_id = (ag.payload->>'commentId')::uuidcasts the JSON text extraction to UUID — no text comparison of UUID values (which can differ in case).CAST()syntax for the parameter avoids Spring Data JPA treating::uuidas part of the named parameter, which would have caused silent failures.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
What I checked
TDD evidence, test coverage at all layers, test helper quality, edge cases.
Blockers
None.
Concerns
1. Missing cross-user scoping test —
AuditLogQueryRepositoryRolledUpTest.javaSee also Nora's comment. From a test coverage standpoint:
youParticipated_is_false_for_comment_with_no_reply_notificationproves the negative when no notification exists. But the more important negative is: a REPLY notification exists in the table forOTHER_USER_IDon the same comment —USER_ID's feed should still returnyouParticipated = false. Without this test, the scoping constraint is not machine-verified.2.
insertAuditEventpayload JSON is hand-rolled —AuditLogQueryRepositoryRolledUpTest.javaThis will silently produce malformed JSON if any value contains a
",\, or control character. Tests using this helper would fail with a JSONB parse error from PostgreSQL rather than an assertion failure — confusing diagnostic. UseObjectMapper.writeValueAsString(payload)instead, which is already available via the Jackson dependency.3. No test for
youParticipatedwhen payload has nocommentIdkey — suggestionWhat happens when
COMMENT_ADDEDis inserted without acommentIdin the payload?ag.payload->>'commentId'returns NULL,(NULL)::uuidis NULL, andn.reference_id = NULLevaluates to UNKNOWN (never true). This is the correct behavior and it meansyouParticipatedstaysfalse— but it is not tested. A one-line test would document this graceful-degradation behavior:What's done well
youParticipatedtrue/false + retroactiveyouMentionedtrue/false coverage closes a pre-existing gap.feedFilters.test.tscover all 5 filter values, positive and negative cases. ThemakeItem()factory follows the project's pattern exactly.DashboardServiceTestmock was correctly updated to implement the new interface method.@Transactionalon the test class ensures no cross-test state leakage — correct use of the test infrastructure.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
What I checked
TDD discipline, naming, function size, SQL invariant comments, frontend patterns.
Blockers
None.
Concerns
1.
MIN(payload::text)::jsonbhas no invariant comment —AuditLogQueryRepository.javaThe correctness of
youParticipateddepends onMIN(payload::text)::jsonbcollapsing to the single payload of aCOMMENT_ADDEDevent. This works becauseCOMMENT_ADDEDevents are always their own session (unconditionalis_new_session = 1insessions_marked). That invariant is invisible from the aggregation line alone.Without a comment, a developer who later changes the session-marking logic for comment events would not know they are about to break
youParticipated. One line:This is the "why" comment that the code style guide explicitly calls for — not explaining what
MINdoes, but explaining the invariant that makes it correct.2. Hand-rolled JSON serialization in test helper —
AuditLogQueryRepositoryRolledUpTest.javaTest code, but fragile. A UUID value like
uuid-with-no-special-charsis fine; but if someone passes a map entry whose value contains a"or\, the produced JSON is invalid and the failure from PostgreSQL will be a parse error, not an assertion failure. Jackson is already on the classpath:What's done well
filterFeed()extraction from+page.svelteis exactly right: pure function, clear name, zero DOM dependencies, trivially testable. The$derived(filterFeed(...))call in the page is one line. Clean.feedFilters.tshas no business logic in template markup. ✅ActivityFeedRowinterface change is the minimal prerequisite for everything else — correct place to start the TDD chain.isYouParticipated()andisYouMentioned()are consistent boolean naming with the Spring projection interface convention.AopInvocation Null return valueerror (before the SQL was added) confirms the test was genuinely red before going green.⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
Migration necessity, infrastructure changes, CI impact, index implications.
LGTM notes
notificationstable (created in V16) and itsrecipient_id/reference_idcolumns are already present. Theidx_notifications_recipientindex on(recipient_id, read, created_at DESC)covers therecipient_idfilter in the correlated subquery at family archive scale. ✅api.tsupdated manually (consistent with whatgenerate:apiwould produce). ✅One note for future consideration
The subquery filters
notificationsbyrecipient_idANDreference_id. The current index is on(recipient_id, read, created_at DESC)—reference_idis not indexed. At current family archive scale (tens of notifications per user), therecipient_idprefix reduces the scan to one user's rows and the sequential scan over those rows is fast.If notification volume grows past ~500 per user, the partial index proposed in issue #295 would become worthwhile:
Not a blocker — flagging as a known future improvement when/if the scale requires it.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
What I checked
UI impact of the data fix, rendering of COMMENT_ADDED events in the new "Für dich" context, brand compliance, accessibility.
LGTM notes
This PR is a data correctness fix — no new components, no layout changes, no color or typography changes. The
COMMENT_ADDEDevent type already renders in the Chronik (alleandkommentarefilters), so the only UX impact is that these events now also appear underfuer-dich. The existingChronikTimelinerow component handles them correctly: actor avatar, document title link, timestamp. No new visual treatment is needed. ✅The
filterFeed()function extraction infeedFilters.tsimproves the code structure without any user-visible change. ✅One minor point to verify manually
Before merge, do a quick manual check that a
COMMENT_ADDEDrow in thefuer-dichcontext renders acceptably at 320px. The document title could be longer than typical in this filter (since reply events can appear for any document), and theChronikTimelinecomponent's title truncation should handle it. This was flagged in the original issue review as a "quick check, not a new concern" and the existing styling should handle it — just worth a visual confirmation.Review concerns addressed
All four concerns from the previous review round have been resolved in commits
2104658c–47f20c17:insertAuditEvent2104658cObjectMapper.writeValueAsString(payload)— already on classpath8726b964youParticipated_is_false_when_reply_notification_belongs_to_other_user— inserts REPLY notification forOTHER_USER_ID, assertsUSER_ID's feed returnsfalseba54cd6dyouParticipated_is_false_when_comment_added_has_no_commentId_in_payload— confirms NULLpayload->>'commentId'yieldsfalsevia SQL NULL semanticsMIN(payload::text)::jsonbhas no invariant comment47f20c17is_new_session=1invariant that makesMINsafeNote on the SQL comment: an initial version used
event's(apostrophe) which caused Spring Data JPA's text-block parser to reject the query at startup (IllegalArgumentException: The string <WITH events AS ...>). Fixed by removing the apostrophe from the comment text.Test counts after these changes:
./mvnw test)vitest run --project=server)🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
What I checked
Domain boundary correctness, CTE payload aggregation invariant, diff scope, layering.
Previous concerns — resolved
1.
MIN(payload::text)::jsonbinvariant comment ✅The comment is now in place:
A future developer changing session-marking logic for comment events will see this immediately. The
event'sapostrophe failure was caught and fixed — good catch on the Spring Data JPA text-block parser limitation.2. Out-of-scope diff (issue #296 changes) — still cosmetic
The
DocumentController,IncompleteDocumentDTO,DocumentService, and related test changes are still in the diff. This is a base-branch calculation artifact (branch cut fromb8ffd81b, those commits were already at758c7087on main). No regressions are introduced — all those changes are already merged. The diff just doesn't match the code review scope cleanly.This is the same cosmetic note from the previous round. It's not a blocker and cannot be fixed without a rebase. Consider rebasing onto current main before merge to produce a clean diff that matches the PR description exactly.
What's done well
youParticipatedunknowingly.CAST(:currentUserId AS uuid)remains the correct Spring Data JPA workaround — the comment in the SQL correctly documents the contract it relies on.DashboardService.getActivity()remains a pure mapper with no business logic leak.🔒 Nora Steiner — Application Security Engineer
Verdict: ✅ Approved
What I checked
IDOR scoping, SQL injection, cross-user notification leakage, JSON serialization safety in tests.
Previous concerns — resolved
1. Missing cross-user IDOR scoping test ✅
The regression test is now in place:
This test inserts a REPLY notification for
OTHER_USER_IDagainst the samecommentId, then queries asUSER_IDand assertsisYouParticipated()returnsfalse. Therecipient_id = CAST(:currentUserId AS uuid)predicate is now machine-verified against the specific IDOR pattern I was watching for.2. Hand-rolled JSON serialization ✅
The
insertAuditEventhelper now usesObjectMapper.writeValueAsString(payload):A
RuntimeExceptionwraps the checkedJsonProcessingException— this is the correct pattern for a test helper whereJsonProcessingExceptionis truly unexpected.Security posture summary
recipient_id = CAST(:currentUserId AS uuid)correctly scopesyouParticipatedto the querying user. Machine-verified by three negative tests.CAST(:currentUserId AS uuid),:limit).youParticipatedis a boolean; no notification content is exposed to the feed consumer.reference_id = (ag.payload->>'commentId')::uuidcast ensures UUID comparison is type-safe, not string comparison.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ✅ Approved
What I checked
TDD coverage, test helper quality, all three previously raised concerns, test naming, edge case coverage.
Previous concerns — resolved
1. Cross-user scoping test ✅
youParticipated_is_false_when_reply_notification_belongs_to_other_user— the key negative case is now in the suite. Arrangement is clear: insert notification forOTHER_USER_ID, assertUSER_ID's feed returns false.2. Hand-rolled JSON in test helper ✅
ObjectMapper.writeValueAsString(payload)replaces string concatenation. Thetry/catchwrappingJsonProcessingExceptionintoRuntimeExceptionis the right choice for a test helper — test failures will be assertion failures, not confusing JSONB parse errors from PostgreSQL.3. No-commentId graceful degradation test ✅
youParticipated_is_false_when_comment_added_has_no_commentId_in_payload— the empty payload case is now documented. SQL NULL semantics (ag.payload->>'commentId'→ NULL →reference_id = NULL→ UNKNOWN → EXISTS returns false) are verified by a real database test.Final test count
AuditLogQueryRepositoryRolledUpTest: 12 tests, all named as sentences, all passing.feedFilters.test.ts: 13 tests covering all 5 filter values with positive and negative cases.DashboardServiceTest: mock correctly implements the newisYouParticipated()interface method.The test suite now covers: true positive, false negative (no notification), false negative (notification for wrong user), and false negative (missing payload key). That is complete behavioral coverage for this feature.
No concerns remaining.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
What I checked
TDD discipline, SQL invariant comment, JSON helper, naming, function size, frontend patterns.
Previous concerns — resolved
1.
MINinvariant comment ✅The comment reads:
This explains the WHY (the is_new_session invariant), not the WHAT (MIN is a min function). Exactly what the code style guide calls for. The Spring Data JPA apostrophe issue was also caught and fixed before committing — good discipline.
2. Jackson ObjectMapper in test helper ✅
Static final
MAPPERinstance,writeValueAsStringcalled per-invocation,JsonProcessingExceptionwrapped inRuntimeException. Clean, correct, no state shared across tests.Clean code notes
filterFeed()extraction remains the highlight of this PR. One line in the component ($derived(filterFeed(...))), a pure function in a dedicated file, 13 unit tests. This is exactly the "extract business logic from template markup" principle in action.isYouParticipated()andisYouMentioned()are consistent boolean naming conventions for the projection interface.youParticipated_is_false_when_comment_added_has_no_commentId_in_payloadleaves no ambiguity about what is being proven.LGTM. Ready to merge.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
Migration necessity, infrastructure impact, CI, index implications, test counts.
LGTM notes
notifications,audit_log) and their columns are already present. ✅youParticipated: booleaninapi.tsmatches whatgenerate:apiwould produce. ✅The
idx_notifications_recipientindex on(recipient_id, read, created_at DESC)continues to cover therecipient_idfilter in the correlated subquery. At family archive scale, no additional indexing is needed. The future(recipient_id, reference_id) WHERE type = 'REPLY'index remains a valid deferred improvement if notification volume grows. No action needed now.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
What I checked
UI impact, brand compliance, accessibility, rendering of newly surfaced
COMMENT_ADDEDevents infuer-dichcontext.LGTM notes
No UI changes in this round of commits. The new commits (
2104658c–47f20c17) are all backend test and SQL changes — no frontend components, no markup, no styles.The earlier approval from the first review round stands:
COMMENT_ADDEDevents already render correctly inChronikTimeline(actor avatar, document title link, timestamp).filterFeed()extraction produced no visual change.fuer-dichpredicate widening is a data correctness fix, not a UI change.Manual check suggestion from the first round (320px rendering of
COMMENT_ADDEDinfuer-dich) is still worth a quick pass before merge, but no code change is required — the existingChronikTimelinetitle truncation handles it.