feat(chronik): add commentPreview to ActivityFeedItemDTO (#454) #475
Reference in New Issue
Block a user
Delete Branch "worktree-feat+issue-454-comment-preview"
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
commentPreviewfield toActivityFeedItemDTO— plain-text preview of the comment body, HTML-stripped server-side via Jsoup, truncated to 120 charsfindAnnotationIdsByIds+ hypothetical preview fetch) into a singlefindDataByIds()call, halving DB round-trips for every Chronik page loadChronikRow.sveltewith{item.commentPreview ?? '„…"'}(plain text binding, no{@html}); addsaria-labelon the<a>wrapper for screen readersMENTION_CREATEDrows are out of scope — no preview in thefor-youvariantChanges
Backend
pom.xml— adds Jsoup 1.18.1 dependencyCommentService— newCommentDatarecord,findDataByIds(),stripAndTruncate();findAnnotationIdsByIdsnow delegates tofindDataByIdsActivityFeedItemDTO— new optionalcommentPreviewfield (@Nullable,NOT_REQUIREDin OpenAPI schema)DashboardService— usesfindDataByIds, populatescommentPreviewin activity rowsFrontend
ChronikRow.svelte— renders{item.commentPreview ?? '„…"'}in comment rows;aria-labelon link for screen readersChronikRow.svelte.spec.ts— 4 new tests for preview rendering, placeholder, non-comment variant, and aria-labelapi.ts— manually updated to reflect the new optionalcommentPreview?: stringfield (propergenerate:apionce backend is running)Test plan
./mvnw test -Dtest=CommentServiceTest— 9 new tests pass./mvnw test -Dtest=DashboardServiceTest— 3 new tests pass, 3 updated mocks compile and pass./mvnw test— all backend tests greencd frontend && npm run lint— passescd frontend && npm run check— no TypeScript errorsCloses #454
🤖 Generated with Claude Code
Replace the „…" placeholder with {item.commentPreview ?? '„…"'}. Plain-text binding — no {@html} — as specified in the security note from issue #285. Adds aria-label to the <a> wrapper for COMMENT_ADDED rows that carry a preview, giving screen reader users the full context in one announcement. Generated api.ts updated manually to include commentPreview?:string; will be regenerated by npm run generate:api once the backend is running. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
The overall design is solid.
CommentDatarecord as a co-located projection type, the singlefindDataByIds()call halving DB round-trips, and keeping the preview generation server-side are all sound decisions. One structural concern and one missing doc update.Blockers
1.
CommentDatais a public inner type leakingdashboard's internal concern intocomment's API surfaceCommentDataexposes bothannotationIdandpreviewas a bundle becauseDashboardServiceneeds them both in one DB call. That is a dashboard performance concern, not a domain concept inCommentService. As a result,DashboardServicenow importsCommentService.CommentData— a cross-module type that couples the dashboard package to the comment service's internal projection type.This is a modest boundary leak, but the
findAnnotationIdsByIdswrapper now performs double work (callsfindDataByIds, then discards the preview) for any future caller that only needs annotation IDs. A cleaner boundary would be to keepfindAnnotationIdsByIdsdoing direct work (as before), and add a dedicatedfindDataByIdsthat dashboard calls when it genuinely needs the combined projection. The namingCommentDatais also ambiguous — it sounds like an entity, not a read-projection.Suggestion: rename to
CommentPreviewDataorCommentProjection, and add a Javadoc explaining it is a read-only projection for the Chronik feed.2. Missing required doc update
Per the doc-update matrix: a new field on an existing DTO (
ActivityFeedItemDTO) that surfaces in the public API does not trigger a PlantUML diagram update, but theCLAUDE.mdnote aboutcommentPreviewand thedocs/GLOSSARY.mdshould be checked — "comment preview" is a new domain concept introduced in this PR. This is a minor doc omission, not a blocking one, but the glossary entry for "Chronik" or "ActivityFeedItem" should reference the new field.Suggestions
stripAndTruncateis a private static helper that does not depend on any instance state. It could beprivate staticto make that clear (currently it is an instance method that readsPREVIEW_MAX_CHARS). SincePREVIEW_MAX_CHARSis alreadystatic final, making the methodstaticis a straightforward improvement.findAnnotationIdsByIdswrapper now callsfindDataByIdswhich callscommentRepository.findAllById— and thenfindAnnotationIdsByIdsalso filters out nulls. Any external caller offindAnnotationIdsByIdsnow pays for a Jsoup parse per comment that they throw away. The wrapper should not have existed post-refactor; if no external caller uses it, it should be removed or marked@Deprecated.What's done well
Jsoup.parse().text()is the idiomatic plain-text extraction approach.commentIds.isEmpty()prevents needless DB calls and matches the existing pattern.@Nullable+NOT_REQUIREDon the DTO field is correct for an optional field.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
The logic is correct and the code reads clearly. Two things need fixing before merge.
Blockers
1.
findAnnotationIdsByIdsis now dead weight — and it does double workDashboardServiceno longer calls this method — the diff shows it was replaced withfindDataByIds. So this method now:commentRepository.findAllById(DB hit + Jsoup parse per comment)Any future caller that only wants annotation IDs pays for HTML stripping they didn't ask for. This is the "functions doing two things" smell applied at the call graph level. Either:
grep -r findAnnotationIdsByIds), or2.
aria-labelconstruction inChronikRow.svelteduplicates the actor nameactorNameis prepended manually, butm.chronik_comment_addedalready receivesactor: ''. This means the message template renders "Felix hat einen Kommentar zu [doc] hinzugefügt" with a leading space (becauseactor: ''produces an empty slot where the actor name normally goes), and thenactorNameis prepended separately.This works accidentally if the i18n template is
{actor} hat einen Kommentar …— the actor slot just produces' hat einen Kommentar …'and the prependedactorNamefills the gap. But it is fragile: if the Spanish or English template puts the actor name elsewhere in the sentence, the aria-label will be grammatically broken.The correct approach is to pass
actor: actorNameto the message function and remove the manual prepend:Suggestions
stripAndTruncatesplits at exactly 120 characters which may cut in the middle of a Unicode surrogate pair. For a family archive with 19th-century German names (umlauts, ß) and potential Kurrent transcription, acodePoint-aware truncation (text.codePoints().limit(120)) would be more correct. Not a blocker for Latin scripts, but worth noting.findDataByIds_does_not_truncate_at_exactly_120_charsinCommentServiceTestis misleading — it tests that a 120-char string stays at 120 chars (no truncation), but the name reads like it is testing truncation behaviour. Rename tofindDataByIds_does_not_truncate_when_content_is_exactly_120_chars.What's done well
$derivedforvariant— single expression, no$effect.{item.commentPreview ?? '„…"'}fallback is idiomatic Svelte 5 and exactly what the TODO comment asked for.data-testid="chronik-comment-preview"on the paragraph enables precise spec targeting without coupling to CSS classes.CommentServiceTestcovering null, blank, HTML stripping, truncation boundary — solid TDD coverage.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This PR does the right things from a security perspective. The historical TODO comment explicitly called out two security requirements — no
{@html}, and server-side stripping — and both are correctly implemented.Security controls verified
1. No XSS — plain text binding confirmed
This is a Svelte text interpolation, not
{@html}. Svelte escapes all text bindings. An attacker-controlled comment body like<img src=x onerror=alert(1)>will be rendered as the literal characters<img src=x onerror=alert(1)>in the DOM — the Jsoup strip is a belt, and Svelte's escaping is the suspenders. Both layers working independently means either one failing alone does not produce XSS.2. Jsoup strip is correct for this use case
Jsoup.parse().text()returns the concatenated text nodes with HTML decoded —&becomes&,<becomes<, etc. This is appropriate here because the plain text is then bound into a Svelte text node (not{@html}), so the re-decoded<and>are safe. If this preview were ever used in an{@html}binding, it would be a stored XSS. TheSECURITYcomment from the old TODO was removed — I'd recommend a brief comment onstripAndTruncatenoting that its output is safe for text-node binding but must never be used as raw HTML.3.
aria-labeldoes not introduce injection surfaceThe
aria-labelis bound via Svelte attribute syntax (notinnerHTML), so it is DOM-escaped by Svelte. A comment preview containing">or<script>will not break the attribute.4. No new endpoints, no new permissions
This PR adds no new API endpoints and no new permission checks. The existing access control on the dashboard feed endpoint is unaffected.
Minor concern (not a blocker)
The
commentPreviewfield surfaces comment body text to any user who can see the activity feed. If the feed is accessible to users with onlyREAD_ALLpermission, they will now see the first 120 characters of any comment. This was presumably true before (they could navigate to the document and read the full comment), so there is no privilege escalation. But it is worth confirming explicitly: the activity feed's authorization check is on the feed endpoint, not per-item. If the feed already had correct access control, this is fine.Recommended addition (non-blocking)
Add a one-line comment to
stripAndTruncate:This is a maintenance signal: if someone copies this method to a context where
{@html}is used, they need to know the contract.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Test coverage is above average for this kind of feature addition — 9 backend unit tests + 3 updated DashboardService tests + 4 Vitest component specs. One naming issue and one missing coverage scenario stand out.
Blockers
1. Misleading test name in
CommentServiceTestThis test creates a 120-character string and asserts
hasSize(120)— meaning "a string at exactly the limit does not get truncated". But the name reads as "this test is verifying that truncation does NOT happen at 120 characters" which is the opposite of what a reader expects. The paired testfindDataByIds_truncates_at_exactly_120_charsis clear; this one should be:When this test fails in CI, the developer has to read the body to understand which direction the failure went.
Suggestions
2. No test for multi-whitespace / only-tags HTML
The
stripAndTruncatepath is covered for null, blank string, and<p><strong>text</strong></p>. A case worth adding: HTML that contains only whitespace after stripping (e.g.<p> </p>or<br/>). Jsoup's.text()on<p> </p>returnsisBlank()does not catch this in Java. The truncated result would be a single non-breaking space appearing as an invisible placeholder. Consider:This is a nice-to-have, not a blocker, but worth tracking.
3. Frontend spec uses
document.querySelectorinstead ofgetByRole/getByTestIdThe project's
vitest-browser-sveltesetup providesgetByTestIdviarender. Usingdocument.querySelectorbypasses the scoped query and can match elements from a previous test if cleanup is incomplete. Prefer:Not a blocker since
data-testidis unique enough in a component test, but consistency with the rest of the spec file matters.What's done well
DashboardServiceTesttests specifically coveringcommentPreviewpopulated, null for non-comment rows, and null for deleted comments — these are the three most important service-layer cases.aria-labeltest verifying the label contains the preview text — accessibility is tested, not just assumed.findDataByIds_omits_deleted_comments_from_result_map) — this is the silent failure mode that would show stale previews after a comment delete, and it's covered.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
The preview renders correctly in context and the accessibility intent is good. One blocker on the
aria-labelconstruction.Blockers
1.
aria-labelbreaks the i18n contract for non-German localesThe message
m.chronik_comment_added({ actor: '', doc: docTitle })is called withactor: ''— an empty string — then.trim()is applied, andactorNameis prepended manually. In German, if the template is"{actor} hat zu „{doc}" einen Kommentar hinzugefügt", passingactor: ''produces" hat zu „Titel" einen Kommentar hinzugefügt"— a leading space that.trim()catches only if the actor name appears first in the sentence.In English or Spanish, the actor slot may not appear first. Spanish subject-verb order may be
"Se añadió un comentario de {actor} en {doc}"— in that case.trim()does nothing useful, and prependingactorNameproduces a grammatically broken label like"María Se añadió un comentario de en Brief an Oma".Fix:
Pass the actor name into the message function directly. Let Paraglide handle the word order per locale.
Suggestions
2. Preview is
italic— consider whether this is the right typographic signal for quoted contentitalicis traditionally used for titles, foreign words, or emphasis — not for quoted text from another user. The preview is essentially a blockquote excerpt. The German typographic convention for quotations uses„…"(which the placeholder already uses correctly). Adding a subtle left border or a slightly different background might better signal "this is what they wrote" versus "this is a label", especially for the 60+ audience.This is a design suggestion, not a WCAG issue. The current italic is not wrong — it is consistent with the existing
for-youvariant style — but it is worth considering.3. Line clamp on mobile (
line-clamp-1) may be too aggressive for seniorsline-clamp-1on small screens shows a single line of italic serif text attext-sm(14px). The senior audience (60+, primary transcriber persona) typically needs 16–18px minimum for comfortable reading. The preview is supplementary, so clamping is acceptable — but attext-sm+line-clamp-1, a meaningful preview may be nearly invisible to someone with mild visual impairment.Consider
text-base(16px) on the preview paragraph, or at minimumsm:text-basefor the mobile breakpoint.What's done well
{@html}is correctly avoided — the comment text is rendered as a text node, so no risk of HTML injection even if the backend preview were to leak a tag.aria-labelbeing conditionallyundefinedwhen there is no preview is correct —undefinedremoves the attribute from the DOM, falling back to the visible text content as the accessible name.focus-visible:ring-2 focus-visible:ring-focus-ringon the<a>wrapper is already present from before — the PR does not regress keyboard accessibility.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
This PR adds one new Maven dependency and modifies application code only. No Docker Compose changes, no new services, no new environment variables, no CI workflow changes.
Checked
pom.xml— Jsoup 1.18.1: version is pinned (not a range like[1.18,)). Renovate will pick this up and auto-create a bump PR when 1.19 lands. ✅One observation (non-blocking)
The
owasp-java-html-sanitizerdependency was already present (visible inpom.xmlcontext around the new Jsoup entry). OWASP sanitizer bundles a subset of HTML parsing via Guava. Jsoup is a second, independent HTML parser. For a preview feature, this is acceptable — Jsoup's.text()extraction is simpler than wiring the sanitizer to produce plain text. But if the dependency list grows, it is worth noting that two HTML parsers exist in the JAR.Not raising this as a concern — just noting it for the next dependency audit.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
The feature as delivered matches the stated requirements from issue #454. Reviewing from a requirements completeness perspective.
Requirements satisfied
commentPreviewis rendered inChronikRow.svelteforvariant === 'comment'{item.commentPreview ?? '„…"'}handles null/undefinedJsoup.parse(html).text()instripAndTruncatePREVIEW_MAX_CHARS = 120{@html}binding ✅ — confirmed in templatefindDataByIds()replacesfindAnnotationIdsByIds()commentPreviewis null for non-comment rowsGap: no acceptance criterion for "deleted comment" UX
The PR correctly returns
commentPreview: nullwhen a comment has been deleted (the comment ID exists in the audit log but the comment row is gone from the DB). The frontend then shows„…"— the same placeholder as "no preview available yet".From a user perspective, there is no visible distinction between:
This is arguably fine (the clickthrough to the document will reveal the deletion), but it was not captured as an explicit acceptance criterion in the original issue. Worth logging as a known gap for a future issue rather than blocking this PR.
Recommendation (post-merge)
Create a follow-up issue: "Chronik: distinguish deleted-comment placeholder from loading placeholder". The current
„…"is an acceptable placeholder for MVP — but the UX gap is documented for future consideration.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
What I checked
TDD evidence, naming, function responsibility, Svelte 5 patterns, dead code.
Findings
Positive
stripAndTruncate()is a single-responsibility private method — exactly the right shape. Under 10 lines, guard clause at top, name reveals intent.findDataByIds()is clean: early return on empty input, no nested conditionals, single loop.CommentDatarecord is a well-typed value object. Keeps the map typed rather than returningObject[].{item.commentPreview ?? '„…"'}is plain text (not{@html}) — correct.aria-labelcorrectly uses the derivedactorName(not a raw empty string as in the previous bug).textContent,getAttribute), not component internals.Blockers
None.
Suggestions
DashboardService.javaline 149: the ternarycommentData != null && !commentData.preview().isEmpty() ? commentData.preview() : nullcould be extracted to a one-liner named methodpreviewOrNull(CommentData)for clarity, but this is a minor readability preference — not a blocker.ChronikRow.svelte.spec.tsusesdocument.querySelectorrather thangetByRole/getByTestIdfromvitest-browser-svelte. The existing test suite uses this pattern too, so it is consistent — but worth noting for future test hygiene.Clean implementation overall. The removal of the dead TODO comment block was the right call.
🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
What I checked
Layering, doc currency, new dependencies, module boundaries.
Blockers
1.
CommentDatarecord is a public inner type ofCommentService— it leaks service internalsDashboardServiceimportsCommentService.CommentDatadirectly:This is a cross-domain type dependency.
DashboardServicenow couples to an internal implementation type of thecommentsub-domain rather than consuming a published interface. TheCommentDatarecord should be a top-level package-private or public class in thedocument.commentpackage, not a nested class of the service.Fix: Move
CommentDatatodocument/comment/CommentData.javaas a top-level record. The service keeps its method signature; the import inDashboardServicebecomesimport org.raddatz.familienarchiv.document.comment.CommentData;— clean published API surface.Documentation Gaps (per the doc-currency table)
No doc changes are required for this PR because:
The Jsoup dependency addition in
pom.xmlis a library addition, not a new external system, so no C4 L1 update is needed.Positive
findAllByIdpass is a good performance win.PREVIEW_MAX_CHARS = 120as a named constant is the right call — avoids a magic number buried in the truncation logic.Summary
The inner-type coupling is a real boundary issue, not a style preference. Move
CommentDatato a top-level class indocument/comment/before merging.🔒 Nora "NullX" Steiner — Security Engineer
Verdict: ✅ Approved
What I checked
XSS / HTML injection, output encoding, new library supply chain, API exposure surface.
Findings
Positive: No XSS vector introduced
The PR explicitly binds
{item.commentPreview ?? '„…"'}as a Svelte text interpolation (not{@html}). Svelte's compiler escapes all text bindings — user-supplied comment content cannot inject HTML into the DOM. This is exactly correct and consistent with the TODO comment it replaces (which warned: "SECURITY: once item.commentPreview lands, render via {text}, never {@html}").Jsoup dependency: CWE-829 supply chain check
Jsoup 1.18.1 is the current stable release (no known CVEs as of this review). The library is used only for
Jsoup.parse(html).text()— a read-only operation that extracts plain text from HTML. There is no rendering or sanitization configuration involved, so the threat surface is minimal: a crafted HTML string can at worst produce unexpected whitespace in the preview text.stripAndTruncateis safesubstring(0, 120)on a Java String truncates at a character boundary — there is no byte/multibyte encoding issue here since Java strings are UTF-16 internally. Emoji/multi-codepoint sequences could theoretically be cut mid-surrogate-pair if they fall exactly at position 120, producing a corrupted preview. This is a display edge case, not a security issue.aria-labelcontentThe
aria-labelis constructed fromactorName(server-side display name) andcommentPreview(server-side stripped text). Neither goes through{@html}— screen reader injection is not a risk here.Blockers
None.
Minor observation
If Jsoup is ever upgraded to parse untrusted HTML for rendering (not just text extraction), that context requires the
Safelistconfiguration. For the current read-only.text()call, no action needed.🧪 Sara Holt — QA Engineer
Verdict: ✅ Approved
What I checked
Test completeness, naming, coverage of edge cases and error paths, test-layer placement.
Findings
Positive
CommentServiceTestcovers 9 distinct cases: empty input, HTML stripping, truncation at 121, preservation at 120, blank content, null content, deleted comment (key absent), annotation ID preservation, null annotation ID. This is comprehensive boundary coverage.DashboardServiceTestadds 3 new tests covering: populated preview, null preview for non-comment kind, and null preview when comment is deleted (empty map return). These exercise theDashboardServiceintegration path, not just theCommentServiceunit.findDataByIds_strips_html_and_extracts_plain_text,getActivity_leaves_commentPreview_null_for_TEXT_SAVED_rows— readable in CI failure reports.data-testidselectors consistently with the existing test suite.One gap: no test for
commentPreviewvisibility whenvariant === 'for-you'The PR description states "MENTION_CREATED rows are out of scope — no preview in the
for-youvariant." The{#if variant === 'comment'}block correctly excludesfor-you. But there is no Vitest spec asserting that afor-youitem (with a non-nullcommentPreview) does NOT render the preview paragraph. The third test (does not render preview paragraph for non-comment variants) useskind: 'TEXT_SAVED'— a good proxy, butfor-youis a separate variant derived fromyouMentioned, not fromkind. Iffor-youbehavior ever changes, this gap would miss it.Suggestion (non-blocker): Add a test:
Blockers
None. The existing coverage is solid. The gap above is a suggestion for future completeness.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
What I checked
Accessibility (ARIA, keyboard nav), brand compliance, typography, mobile behavior, senior audience considerations.
Findings
Positive
{item.commentPreview ?? '„…"'}— the fallback placeholder uses proper German quotation marks and an ellipsis. This reads naturally for German-speaking family members and signals "content exists but isn't loaded" without breaking the layout.font-serif text-sm text-ink-2 italicon the preview paragraph — correct brand typography for quoted/preview content.text-ink-2has sufficient contrast againstbg-surface.line-clamp-1 sm:line-clamp-2— clamping to 1 line on mobile and 2 on tablet+ is the right responsive choice. Prevents overflow on 320px screens.aria-labelon the<a>wrapper is a meaningful addition. Screen readers will announce the full context ("Max added a comment on Letter from Grandmother — Hello family, great letter!") instead of reading the fragmented visual text.One concern:
aria-labelsuppresses inner text for screen readersWhen
aria-labelis set on the<a>element, it completely overrides the inner text content for screen readers. This means the document title, actor name, and timestamp rendered inside the anchor are no longer announced. Thearia-labelstring currently includes the action + preview but omits the timestamp (timeLabel). Senior users navigating by screen reader lose the time context.Recommendation (non-blocker): Append the time to the
aria-label:For rows without a preview,
aria-labelisundefined, which means the inner text is read — that is correct. The issue only affects the populated preview case.Touch targets
The
<a>usesp-3(12px padding). On mobile, the total tap area depends on content height. Comment rows withline-clamp-2on the preview will be tall enough. Single-line rows might be borderline at 44px. Not flagging as a blocker since the existing row pattern is unchanged.Blockers
None. The
aria-labeltimestamp suggestion is a polish item for a follow-up.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
New dependencies (supply chain, pinning), docker-compose impact, CI pipeline impact, infrastructure surface changes.
Findings
Jsoup 1.18.1 — pinned, current, no CVEs
pom.xmlpins Jsoup to1.18.1explicitly. No floating range, noLATEST. This is reproducible. Renovate will pick up future patch bumps automatically. Good.No infrastructure changes
No new Docker services, no new environment variables, no changes to
docker-compose.yml, no new ports exposed, no new volumes. The PR is purely application code + a Maven dependency. Zero operational impact.No CI pipeline impact
The Jsoup JAR is a transitive download that Maven caches. The
maven_cachekey already coverspom.xmlchanges viahashFiles('backend/pom.xml')— CI cache will invalidate once and repopulate. Normal.Backend JAR size
Jsoup 1.18.1 adds ~520KB to the fat JAR. Negligible for a Spring Boot application.
Blockers
None. Clean from a DevOps perspective.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
What I checked
Requirements coverage vs. issue #454, acceptance criteria completeness, edge cases documented, scope creep.
Findings
Requirements traceability
Issue #454 requested:
commentPreviewfield, backend strip+truncate, frontend rendering)„…")MENTION_CREATED/for-youvariant — ✅ out-of-scope documented in PR descriptionEdge cases covered in spec/tests
One open question for the product owner (non-blocker)
The truncation is at 120 characters. If a comment begins with a mention like
@Oma Gretland the actual message content starts at character 12, users see a preview that starts with the mention handle rather than the message. Is this acceptable, or should mentions be stripped/replaced before truncation? No current test covers this scenario. Suggest tracking as a follow-up issue if family testers find it confusing.Scope discipline
The PR is well-scoped. It does not add comment editing previews, it does not change the
for-youpanel, and it does not modify notification behavior. The PR description correctly calls out what is out of scope.Blockers
None.
🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
What I checked
DashboardServicecallsCommentService.findDataByIds()— correct, no repository cross-access.CommentDatarecord lives indocument/comment/— correct package ownership.ErrorCodeorPermissionvalues — documentation trigger table has no hits for this PR.commentPreviewis computed on the fly from existingcontentcolumn. Correct call.CommentDatarecord (annotationId + preview) collapses two concerns into one DB round-trip. That's the right trade-off at this scale.Suggestions (non-blocking)
stripAndTruncateis a private method that calls a third-party library. If Jsoup is ever replaced, this is the only place to change. The isolation is already good. No action needed.commentPreviewtruncates at a character boundary. For a family archive with German text (ß, ä, ö, ü), truncating mid-word is acceptable — the context is a feed preview, not a canonical display. Fine as-is.Clean PR. Module boundaries respected. No architectural concerns.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
1.
DashboardService.getActivity—commentPreviewnull-check uses!commentData.preview().isEmpty()instead of!commentData.preview().isBlank()backend/src/main/java/org/raddatz/familienarchiv/dashboard/DashboardService.java(line ~153):stripAndTruncatealready returns""for blank/null content, and Jsoup's.text()strips whitespace. ButisEmpty()vsisBlank()is a semantic inconsistency: if for any reasonstripAndTruncatereturned" ", this check would pass an all-whitespace preview to the frontend.isBlank()is the correct guard here, matching the intent of "no meaningful preview content". Small but a real edge case.2.
CommentData.java— record fields have no@Nullableannotation onannotationIdbackend/src/main/java/org/raddatz/familienarchiv/document/comment/CommentData.java:annotationIdis intentionally nullable (comments without an associated annotation exist), but there's no@Nullableannotation. Given the rest of the codebase marks nullable fields explicitly (seeActivityFeedItemDTO), this is an inconsistency. The field should be annotated@Nullable UUID annotationId.Suggestions (non-blocking)
ChronikRow.svelte: Thearia-labelconcatenateschronik_comment_added(...)+—+commentPreview. The em-dash separator is hardcoded. Consider whether this separator should be i18n-aware (in German:—, in Spanish:—). Low priority for a family app but worth noting for completeness.findDataByIdsinCommentServiceuses aforloop +HashMappattern. A streams approach would be idiomatic Java 21:Not a blocker — the current code is readable and correct.
previewfield inCommentDatacould be@NonNull(sincestripAndTruncatealways returns a non-null string) to make the contract explicit.Overall this is clean work. The two items above are genuine correctness concerns worth fixing before merge.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
What I audited
XSS — the critical path for this PR
The whole point of this feature is to display user-generated HTML comment content in the feed. I verified the full data flow:
Jsoup.parse(html).text()— this correctly extracts plain text from HTML, discarding all tags. Jsoup's.text()method does not preserve any markup. ✅commentPreviewis a plainStringinActivityFeedItemDTO, typed asstring?in the generated TypeScript types. No HTML encoding needed at this layer. ✅{item.commentPreview ?? '„…"'}— Svelte's{expression}syntax uses DOMtextContent, notinnerHTML. No{@html}in sight. The old TODO comment explicitly warned about this, and the implementation correctly followed the instruction. ✅The previous TODO comment in
ChronikRow.svelteexplicitly identified the XSS risk and the correct mitigation. Both were followed.OWASP Top 10 scan
commentPreviewexposes comment content to feed viewers. Comments are only indexed in the activity feed if they were legitimately posted (the audit log row exists). No new data exposure surface beyond what the existing feed already provides.Jsoup.parse()is used for parsing only, not for constructing any query or command. No injection risk.Suggestions (non-blocking)
Safelist). This PR uses it purely for text extraction, not sanitization. That's fine —Jsoup.parse(html).text()is the correct idiom for plain-text extraction and is safe for this use case. Worth a one-line comment instripAndTruncateexplaining the intent: "plain-text extraction, not sanitization."No security blockers. This is a textbook safe implementation of user-generated content in a feed.
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
Test coverage review
Backend — CommentServiceTest
9 new tests covering: empty input, HTML stripping, truncation at 121 chars, preservation at exactly 120 chars, blank content, null content, deleted comment omission, annotation ID preservation, and null annotation ID. Boundary conditions are well covered —
text.length() > PREVIEW_MAX_CHARSmeans 121 chars truncates and 120 chars does not, and both cases are explicitly tested. ✅Backend — DashboardServiceTest
3 new tests:
commentPreviewpopulated for COMMENT_ADDED rows, null for non-comment rows (TEXT_SAVED), and null when the comment is deleted (empty map returned). These cover the three meaningful states ofcommentPreviewat the service layer. The two existing tests that usedfindAnnotationIdsByIdsare correctly updated to usefindDataByIdswith aCommentDatastub. ✅Frontend — ChronikRow.svelte.spec.ts
4 new tests: preview text rendered, placeholder ellipsis on null preview, no preview paragraph for non-comment variants, and
aria-labelcontains preview text. All usedocument.querySelectorwithdata-testidselectors rather than implementation internals. ✅Concerns (non-blocking)
Missing edge case: commentPreview exactly at 120 chars with trailing whitespace.
stripAndTruncatecalls.trim()after Jsoup's.text(), and.text()itself strips leading/trailing whitespace. So a 120-char string with trailing spaces would be trimmed to fewer than 120 chars and not truncated. This is correct behaviour but not explicitly tested. Low risk for a preview field.Missing test: preview truncates mid-character for multibyte input (e.g., Kurrent transcriptions containing ä, ö, ü, ß, em-dashes). Java's
substring(0, 120)is char-index based. German characters in the BMP (ä=U+00E4 etc.) are single chars, so this is safe. Em-dashes (U+2014) also single char. No supplementary-plane characters expected in family archive content. Not a practical risk.Frontend test uses
commentPreview: undefinedto simulate null. The OpenAPI spec marks this field asNOT_REQUIREDso it will beundefinedwhen absent in the TypeScript type. This is the correct simulation. ✅What I didn't find
No
Thread.sleep(), no disabled tests, no snapshot-only coverage, no tests that mock what should be real. The unit tests are at the right layer. The component tests test user-visible behaviour.Solid test suite. No blockers.
🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
Blockers
1.
aria-labelis only set whencommentPreviewis present — keyboard/screen-reader users get no label for comment rows without a previewfrontend/src/lib/activity/ChronikRow.svelte:When
commentPreviewisundefined(deleted comment, or MENTION_CREATED row), the<a>has noaria-label. Screen readers will then read the full visible text content of the link — which includes the verb phrase, the doc title, the preview paragraph text („…"), and the timestamp. That is coherent, so this is not catastrophic, but the experience is inconsistent: rows with a preview get a clean, structured aria-label; rows without a preview fall back to raw inner text. The placeholder„…"will be announced as literal quote characters.Recommendation: Always set an
aria-labelon comment rows, using the preview when available and omitting the preview portion when not:This way MENTION_CREATED and non-comment variants still fall back to inner text (fine), but comment rows always have a structured label.
Suggestions (non-blocking)
Typography: Preview uses
font-serif text-sm text-ink-2 italic— this is the correct brand style for quoted/secondary content. Tinos italic at 14px is legible at desktop. On mobile (line-clamp-1at 320px), this renders at roughly one line of ~50 characters. Acceptable.Placeholder characters:
„…"(U+201E, U+2026, U+201C) — these are the correct German typographic quote characters. They render well in Tinos. ✅Touch target: The entire row is the
<a>element withp-3padding. Row height will be well above 44px in practice. ✅Focus indicator:
focus-visible:ring-2 focus-visible:ring-focus-ring focus-visible:outline-noneis present on the<a>. ✅The one blocker is real accessibility impact for screen reader users on comment rows without a preview. Fix is a one-liner.
🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
pom.xml— Jsoup dependencyVersion is pinned — reproducible builds. Jsoup 1.18.1 is a recent stable release. No known CVEs. Adds ~470KB to the JAR — negligible. ✅
docker-compose.yml— marked as modified in git statusI see
docker-compose.ymlis modified in the working tree (from the git status). This PR's diff does not include it, which means it's a local change that isn't part of this PR's changeset. Not a concern for this PR, but worth confirming separately that the compose change is intentional and tracked.No new services, no new volumes, no new infrastructure components — this PR touches only application code and one Maven dependency. Zero operational impact. ✅
CI impact: Jsoup is a compile-scoped dependency. It will be downloaded and cached on first CI run. Maven cache key is
maven-${{ hashFiles('backend/pom.xml') }}— pom.xml changed, so the cache will miss once and rebuild. Normal behaviour. ✅Suggestions (non-blocking)
No DevOps concerns with this PR.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Requirements coverage
The issue (#454) asks for a plain-text preview of the comment body in the Chronik feed row. Reviewing the implementation against the described acceptance criteria:
FR-PREVIEW-01: Preview is plain text (no HTML) — ✅ Jsoup
.text()extraction, no{@html}in frontend.FR-PREVIEW-02: Preview is truncated server-side to avoid unbounded payloads — ✅ 120 chars, enforced in
stripAndTruncate, tested.FR-PREVIEW-03: Deleted comments show no preview — ✅
commentDataByComment.get(commentId)returnsnullwhen the comment is not found;commentPreviewisnullin the DTO; frontend renders„…"placeholder.FR-PREVIEW-04: MENTION_CREATED rows are out of scope and correctly excluded — the PR description explicitly calls this out and no preview is wired for that variant. ✅
NFR scope note: The PR description notes that
api.tswas manually updated because the backend wasn't running. This is explicitly flagged as a temporary workaround with a note to re-rungenerate:apionce the backend is running. The manual diff looks correct (optionalcommentPreview?: stringon the right DTO type). This is an acceptable process workaround for a solo developer.Open question (non-blocking)
The 120-char truncation limit is implemented but not traced to a requirements decision. This is a design constant that affects UX (how much context users get) and payload size. Worth a brief comment in the code or a Gitea comment on the original issue noting why 120 was chosen (one line of feed text at ~sm breakpoint?), so the next person who touches it has context. Not a blocker — the value is reasonable and consistent with common feed patterns.
Requirements coverage is complete. No missing acceptance criteria.
🏗️ Markus Keller — Senior Application Architect — Round 2
Verdict: ✅ Approved
Re-reviewed after the fix commit. Nothing changed that touches module boundaries or architecture concerns. The three fixes (isBlank, @Nullable, aria-label) are all implementation-level corrections. No new architectural issues introduced. My previous approval stands.
👨💻 Felix Brandt — Senior Fullstack Developer — Round 2
Verdict: ✅ Approved
Both blockers from my previous review are resolved:
DashboardService.java:!commentData.preview().isBlank()— correct. ✅CommentData.java:@Nullable UUID annotationIdwithjakarta.annotation.Nullable— consistent with the codebase convention used inActivityFeedItemDTO. ✅The fix commit is clean. No new issues introduced. LGTM.
🔒 Nora "NullX" Steiner — Application Security Engineer — Round 2
Verdict: ✅ Approved
Re-checked the fix commit. No security-relevant changes in this round — the fixes are annotation hygiene (
@Nullable) and a guard logic improvement (isBlank). The XSS mitigations I verified in Round 1 remain intact. No new attack surface. My previous approval stands.🧪 Sara Holt — Senior QA Engineer — Round 2
Verdict: ✅ Approved
The fix commit adds one new frontend test:
This correctly covers the new
aria-labelbehavior for comment rows without a preview. The test asserts both that the<a>has anaria-labelattribute AND that it is not null — which verifies the fallback path (m.chronik_comment_added(...)without the preview suffix) is exercised. ✅49 backend tests passing, 5 frontend spec tests for the preview behavior. Test suite is solid. LGTM.
🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist — Round 2
Verdict: ✅ Approved
My blocker is resolved. The updated
aria-labelinChronikRow.svelte:This now correctly:
Screen reader users will now always get a clean, meaningful label on comment rows regardless of whether the comment still exists. ✅
The corresponding test also verifies this. Well done — accessibility as a quality gate, not an afterthought.
🛠️ Tobias Wendt — DevOps & Platform Engineer — Round 2
Verdict: ✅ Approved
No infrastructure changes in the fix commit. The three fixes are pure application code. CI impact is nil beyond a normal recompile. My previous approval stands.
📋 Elicit — Requirements Engineer — Round 2
Verdict: ✅ Approved
The fix commit addresses the accessibility gap that Leonie raised — screen reader users can now access a meaningful label on comment rows even when the comment has been deleted. This maps to the implicit NFR of WCAG 2.1 AA compliance for the Chronik feed.
No requirements gaps remain. All acceptance criteria from issue #454 are met. The PR is ready to merge.