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

As a user I want to @mention other users in comments so they are notified and linked to their profile #72

New Issue
Closed
opened 2026-03-25 19:27:36 +01:00 by marcel · 4 comments
marcel commented 2026-03-25 19:27:36 +01:00
Owner
Copy Link

Background

When writing a comment, users want to draw a specific family member's attention to it. Typing @ triggers a search popup that lets them pick a registered user. The mention is saved as a structured reference, rendered as a clickable link to that user's profile, and triggers a notification.

Depends on: #71 (notification system)

Decisions

  • contenteditable div (not <textarea>) — allows inline styled chips that delete as one unit
  • Only AppUser is searched — not Person/sender/receiver records, since only real accounts can receive notifications
  • No migration of pre-existing comments — there are none, so old-format handling is not needed

User Journey

User is writing a comment. They type @H and a small popup appears listing users whose name contains "H". As they keep typing the list narrows. They click a name or press Enter. The @H they typed is replaced with a styled chip: @Hans Mueller. The rest of the comment continues normally.

On submit:

  • Comment body is saved as plain text with @Hans Mueller inline
  • A separate mentionedUserIds list is sent alongside the body
  • Hans receives an in-app notification; if he has email mentions enabled, also an email

Readers see @Hans Mueller rendered as a link to his profile.

E2E Scenarios

Scenario: @ triggers the mention popup
  Given I am writing a comment
  When I type "@"
  Then a user search popup appears

Scenario: Popup filters as I type
  Given the mention popup is open
  When I type "Ha"
  Then only users whose name contains "Ha" are shown

Scenario: Selecting a user inserts a chip
  Given the mention popup is showing "Hans Mueller"
  When I click "Hans Mueller"
  Then a styled "@Hans Mueller" chip appears in the editor
  And the popup closes
  And the cursor is positioned after the chip

Scenario: Chip deletes as one unit
  Given a "@Hans Mueller" chip is in the editor
  When I press Backspace once with the cursor immediately after the chip
  Then the entire chip is removed

Scenario: Mention renders as a link after posting
  Given a comment contains a mention of Hans Mueller
  When I view the comment
  Then "@Hans Mueller" is a link to his profile

Scenario: Mentioned user receives a notification
  Given Hans has an account
  When I post a comment mentioning @Hans Mueller
  Then Hans has an unread notification about the mention

Implementation — contenteditable editor

The comment textarea is replaced with a contenteditable div. Key pieces:

@ detection (on every input event)
Scan backwards from the cursor through the current text node. If there is an @ with no space between it and the cursor, extract the query string and open the popup. A space or another @ closes it.

Popup positioning
Use Range.getBoundingClientRect() on the range covering the @query text to anchor the popup to the exact cursor position.

Chip insertion (on user select)

  1. Delete the @query text from the current text node
  2. Insert <span contenteditable="false" data-mention-id="{uuid}" class="mention-chip">@Hans Mueller</span>
  3. Insert a plain space text node directly after the span
  4. Move the cursor to after the space

contenteditable="false" on the span is what makes Backspace delete it as a single unit — this is native browser behaviour, no extra code needed.

Keyboard navigation in popup
↑/↓ to move, Enter/Tab to select, Escape to dismiss. Prevent default on these keys when the popup is open so they don't move the cursor.

Content extraction (on submit)
Walk childNodes of the editor div:

  • Node.TEXT_NODE → append textContent to body string
  • SPAN[data-mention-id] → append @Name to body string, push mentionId to mentionedUserIds[]
  • BR → append \n

Paste normalization
paste event: e.preventDefault(), extract text/plain from clipboardData, insert via document.execCommand('insertText'). Prevents pasted HTML from breaking the chip detection logic.

Svelte integration
Use bind:this to hold the editor element ref. Do not use bind:innerHTML — Svelte's reconciler would fight with the manual DOM chip insertions. Popup state (query, users, selectedIndex, anchorRect) lives in normal $state. The editor content itself is owned by the DOM.

Backend implementation notes

Frontend implementation notes

  • MentionEditor.svelte — self-contained component; emits { body, mentionedUserIds } on submit
  • MentionPopup.svelte — floating div with role="listbox", positioned via position: fixed + getBoundingClientRect()
  • Rendered mentions in CommentThread: scan the body for @Name strings that match known user names and wrap them in <a href="/users/{id}"> — or store the rendered form server-side if a lookup table is available

Related: #71 (notification delivery), #70 (help page — document @mention syntax)

## Background When writing a comment, users want to draw a specific family member's attention to it. Typing `@` triggers a search popup that lets them pick a registered user. The mention is saved as a structured reference, rendered as a clickable link to that user's profile, and triggers a notification. Depends on: #71 (notification system) ## Decisions - **`contenteditable` div** (not `<textarea>`) — allows inline styled chips that delete as one unit - **Only `AppUser` is searched** — not `Person`/sender/receiver records, since only real accounts can receive notifications - **No migration of pre-existing comments** — there are none, so old-format handling is not needed ## User Journey User is writing a comment. They type `@H` and a small popup appears listing users whose name contains "H". As they keep typing the list narrows. They click a name or press Enter. The `@H` they typed is replaced with a styled chip: **@Hans Mueller**. The rest of the comment continues normally. On submit: - Comment body is saved as plain text with `@Hans Mueller` inline - A separate `mentionedUserIds` list is sent alongside the body - Hans receives an in-app notification; if he has email mentions enabled, also an email Readers see `@Hans Mueller` rendered as a link to his profile. ## E2E Scenarios ``` Scenario: @ triggers the mention popup Given I am writing a comment When I type "@" Then a user search popup appears Scenario: Popup filters as I type Given the mention popup is open When I type "Ha" Then only users whose name contains "Ha" are shown Scenario: Selecting a user inserts a chip Given the mention popup is showing "Hans Mueller" When I click "Hans Mueller" Then a styled "@Hans Mueller" chip appears in the editor And the popup closes And the cursor is positioned after the chip Scenario: Chip deletes as one unit Given a "@Hans Mueller" chip is in the editor When I press Backspace once with the cursor immediately after the chip Then the entire chip is removed Scenario: Mention renders as a link after posting Given a comment contains a mention of Hans Mueller When I view the comment Then "@Hans Mueller" is a link to his profile Scenario: Mentioned user receives a notification Given Hans has an account When I post a comment mentioning @Hans Mueller Then Hans has an unread notification about the mention ``` ## Implementation — contenteditable editor The comment textarea is replaced with a `contenteditable` div. Key pieces: **@ detection (on every `input` event)** Scan backwards from the cursor through the current text node. If there is an `@` with no space between it and the cursor, extract the query string and open the popup. A space or another `@` closes it. **Popup positioning** Use `Range.getBoundingClientRect()` on the range covering the `@query` text to anchor the popup to the exact cursor position. **Chip insertion (on user select)** 1. Delete the `@query` text from the current text node 2. Insert `<span contenteditable="false" data-mention-id="{uuid}" class="mention-chip">@Hans Mueller</span>` 3. Insert a plain space text node directly after the span 4. Move the cursor to after the space `contenteditable="false"` on the span is what makes Backspace delete it as a single unit — this is native browser behaviour, no extra code needed. **Keyboard navigation in popup** `↑`/`↓` to move, `Enter`/`Tab` to select, `Escape` to dismiss. Prevent default on these keys when the popup is open so they don't move the cursor. **Content extraction (on submit)** Walk `childNodes` of the editor div: - `Node.TEXT_NODE` → append `textContent` to body string - `SPAN[data-mention-id]` → append `@Name` to body string, push `mentionId` to `mentionedUserIds[]` - `BR` → append `\n` **Paste normalization** `paste` event: `e.preventDefault()`, extract `text/plain` from `clipboardData`, insert via `document.execCommand('insertText')`. Prevents pasted HTML from breaking the chip detection logic. **Svelte integration** Use `bind:this` to hold the editor element ref. Do not use `bind:innerHTML` — Svelte's reconciler would fight with the manual DOM chip insertions. Popup state (`query`, `users`, `selectedIndex`, `anchorRect`) lives in normal `$state`. The editor content itself is owned by the DOM. ## Backend implementation notes - Extend comment POST/PUT body to accept `mentionedUserIds: UUID[]` alongside `body: string` - Store mentions in a `comment_mentions` join table (commentId → userId) — parse-free, query-friendly - `NotificationService.notifyMention(mentionedUserId, commentId)` — delegates to #71 - New `GET /api/users/search?q=...` endpoint — returns `[{ id, firstName, lastName }]` for authenticated users only ## Frontend implementation notes - `MentionEditor.svelte` — self-contained component; emits `{ body, mentionedUserIds }` on submit - `MentionPopup.svelte` — floating div with `role="listbox"`, positioned via `position: fixed` + `getBoundingClientRect()` - Rendered mentions in `CommentThread`: scan the body for `@Name` strings that match known user names and wrap them in `<a href="/users/{id}">` — or store the rendered form server-side if a lookup table is available Related: #71 (notification delivery), #70 (help page — document @mention syntax)
marcel commented 2026-03-25 19:50:43 +01:00
Author
Owner
Copy Link

Implementation plan finalised

See .agent/current-plan.md (Phases 2 + 4) for the full step-by-step plan. Summary of decisions locked in:

Mention rendering is server-side. Backend returns mentionDTOs: [{id, firstName, lastName}] alongside every comment. Frontend maps over that list to replace @Name substrings with <a> links — no fragile text parsing, no name-collision ambiguity.

contenteditable approach. The <textarea> in CommentThread is replaced with MentionEditor.svelte. The editor div is owned entirely by the DOM (bind:this, no bind:innerHTML). Only popup state (query, users, selectedIndex, anchorRect) lives in Svelte $state. Chip insertion uses contenteditable="false" spans — Backspace deletes the entire chip natively.

Storage. Mentions are stored in a comment_mentions join table (comment_id → user_id). The CreateCommentDTO gains a mentionedUserIds: List<UUID> field.

Serialization. DocumentComment.mentions (JPA collection) is @JsonIgnore. A @Transient List<MentionDTO> mentionDTOs field is populated by the service and serialized instead, exposing only {id, firstName, lastName}.

User search. GET /api/users/search?q=... — max 10 results, authenticated only, searches AppUser (not Person records), matches against full name and username.

No pre-existing comment migration needed — there are no comments in the system yet.

Branch — feat/71-72-notifications-and-mentions (shared with #71)

## Implementation plan finalised See `.agent/current-plan.md` (Phases 2 + 4) for the full step-by-step plan. Summary of decisions locked in: **Mention rendering is server-side.** Backend returns `mentionDTOs: [{id, firstName, lastName}]` alongside every comment. Frontend maps over that list to replace `@Name` substrings with `<a>` links — no fragile text parsing, no name-collision ambiguity. **`contenteditable` approach.** The `<textarea>` in `CommentThread` is replaced with `MentionEditor.svelte`. The editor div is owned entirely by the DOM (`bind:this`, no `bind:innerHTML`). Only popup state (`query`, `users`, `selectedIndex`, `anchorRect`) lives in Svelte `$state`. Chip insertion uses `contenteditable="false"` spans — Backspace deletes the entire chip natively. **Storage.** Mentions are stored in a `comment_mentions` join table (comment_id → user_id). The `CreateCommentDTO` gains a `mentionedUserIds: List<UUID>` field. **Serialization.** `DocumentComment.mentions` (JPA collection) is `@JsonIgnore`. A `@Transient List<MentionDTO> mentionDTOs` field is populated by the service and serialized instead, exposing only `{id, firstName, lastName}`. **User search.** `GET /api/users/search?q=...` — max 10 results, authenticated only, searches `AppUser` (not `Person` records), matches against full name and username. **No pre-existing comment migration needed** — there are no comments in the system yet. **Branch** — `feat/71-72-notifications-and-mentions` (shared with #71)
marcel added the notification label 2026-03-25 20:30:06 +01:00
marcel commented 2026-03-27 18:03:33 +01:00
Author
Owner
Copy Link

Architecture review — @mkeller

Two concerns, one hard blocker.

🔴 Blocker: {@html renderBody()} is an XSS injection point

The plan says:

Use {@html renderBody(comment)} in the template

renderBody() replaces @Name substrings in the comment body with <a href> tags and then hands the result to {@html}. The problem: the raw body string comes from user input. If a commenter writes <script>alert(1)</script> or <img src=x onerror=...>, it gets injected straight into the DOM.

The fix — escape first, inject links second:

function renderBody(comment: DocumentComment): string {
    // 1. Escape the raw body — no user content touches the DOM unescaped
    let safe = comment.body
        .replace(/&/g, '&amp;')
        .replace(/</g, '&lt;')
        .replace(/>/g, '&gt;')
        .replace(/"/g, '&quot;');

    // 2. Replace @Name placeholders with anchor tags in the now-safe string
    for (const m of comment.mentionDTOs) {
        const name = `@${m.firstName} ${m.lastName}`;
        // name is trusted data from our own backend — safe to inject
        safe = safe.replace(
            name,
            `<a href="/users/${m.id}" class="mention-link">${name}</a>`
        );
    }
    return safe;
}

Order matters: escape → inject. Not the other way around. This is non-negotiable before the feature ships. Note that issue #116 (CSP headers) is still open, so there is currently no second line of defence.

🟡 Consider: contenteditable vs. plain textarea

The chip-based mention editor is well-specified, but contenteditable is notoriously hostile territory — cross-browser selection quirks, IME composition events, paste normalization edge cases, Safari. You will spend more time debugging the editor than the rest of the feature combined.

Ask yourself: does a family archive actually need visual chips that delete as a unit? The functional value is the notification trigger, not the inline chip styling.

The simpler alternative — textarea with regex detection:

  • Keep the plain <textarea>
  • Detect @query via regex on input event to trigger the popup (same UX)
  • On user select from popup: replace @query in the textarea value with @Name plain text
  • On submit: scan textarea value with regex to extract mentioned names → resolve against the mentionDTOs returned from /api/users/search
  • extractContent() becomes a one-liner regex scan — no DOM walking, no childNodes traversal

This gives identical functional behavior: popup appears, mention is inserted, notification is triggered, @Name renders as a link in the thread. The only loss is that Backspace doesn't delete the chip as a unit. For a family commenting tool, that is an acceptable trade-off.

If you proceed with contenteditable, the spec is thorough enough to work from — just budget the extra debugging time honestly.

## Architecture review — @mkeller Two concerns, one hard blocker. --- ### 🔴 Blocker: `{@html renderBody()}` is an XSS injection point The plan says: > Use `{@html renderBody(comment)}` in the template `renderBody()` replaces `@Name` substrings in the comment body with `<a href>` tags and then hands the result to `{@html}`. The problem: the raw `body` string comes from user input. If a commenter writes `<script>alert(1)</script>` or `<img src=x onerror=...>`, it gets injected straight into the DOM. **The fix — escape first, inject links second:** ```typescript function renderBody(comment: DocumentComment): string { // 1. Escape the raw body — no user content touches the DOM unescaped let safe = comment.body .replace(/&/g, '&amp;') .replace(/</g, '&lt;') .replace(/>/g, '&gt;') .replace(/"/g, '&quot;'); // 2. Replace @Name placeholders with anchor tags in the now-safe string for (const m of comment.mentionDTOs) { const name = `@${m.firstName} ${m.lastName}`; // name is trusted data from our own backend — safe to inject safe = safe.replace( name, `<a href="/users/${m.id}" class="mention-link">${name}</a>` ); } return safe; } ``` Order matters: escape → inject. Not the other way around. This is non-negotiable before the feature ships. Note that issue #116 (CSP headers) is still open, so there is currently no second line of defence. --- ### 🟡 Consider: `contenteditable` vs. plain textarea The chip-based mention editor is well-specified, but `contenteditable` is notoriously hostile territory — cross-browser selection quirks, IME composition events, paste normalization edge cases, Safari. You will spend more time debugging the editor than the rest of the feature combined. Ask yourself: does a family archive actually need visual chips that delete as a unit? The functional value is the notification trigger, not the inline chip styling. **The simpler alternative — textarea with regex detection:** - Keep the plain `<textarea>` - Detect `@query` via regex on `input` event to trigger the popup (same UX) - On user select from popup: replace `@query` in the textarea value with `@Name` plain text - On submit: scan textarea value with regex to extract mentioned names → resolve against the `mentionDTOs` returned from `/api/users/search` - `extractContent()` becomes a one-liner regex scan — no DOM walking, no `childNodes` traversal This gives identical functional behavior: popup appears, mention is inserted, notification is triggered, `@Name` renders as a link in the thread. The only loss is that Backspace doesn't delete the chip as a unit. For a family commenting tool, that is an acceptable trade-off. If you proceed with `contenteditable`, the spec is thorough enough to work from — just budget the extra debugging time honestly.
marcel commented 2026-03-27 19:03:50 +01:00
Author
Owner
Copy Link

UX Review — @leonievoss

The architect has already flagged the XSS blocker and the contenteditable question. I want to add the UX and accessibility layer to both.

🔴 Blocker — contenteditable has no ARIA role in the spec

The architect's comment correctly raises contenteditable as an implementation risk. From an accessibility standpoint, it is also a silent failure: a contenteditable div has no implicit ARIA role. Screen readers announce it as a generic region — users cannot tell it's an editable field.

If contenteditable is used, these attributes are required on the editor div:

<div
  contenteditable="true"
  role="textbox"
  aria-multiline="true"
  aria-label="Kommentar schreiben"
  aria-autocomplete="list"
  aria-haspopup="listbox"
  aria-expanded={popupOpen}
  aria-controls="mention-popup"
  aria-activedescendant={activeOptionId}
  bind:this={editorEl}
/>

And MentionPopup needs id="mention-popup" role="listbox", each row role="option" aria-selected={i === selectedIndex}.

Without the full combobox ARIA pattern, this feature is invisible to assistive technology.

🔴 Blocker — Mobile keyboard stability is unspecified

On iOS Safari and Chrome Android, programmatic DOM mutations during chip insertion frequently collapse the virtual keyboard. This is not a theoretical risk — it will happen. The implementation notes must specify that chip insertion is tested on both platforms before merge. This is a showstopper for a family app where a significant portion of users are on mobile.

🟡 Design recommendation — take the textarea path

The architect raises this as a consideration. I'm making it a design recommendation, based on the combined accessibility and implementation risk picture:

contenteditable chips Textarea + @Name plain text
Chip deletes as one unit ✅ ❌
Works on iOS Safari ⚠️ fragile ✅
Screen reader navigable ⚠️ full ARIA combobox required ✅ native textarea ARIA
Mobile virtual keyboard ⚠️ DOM mutations can collapse it ✅ stable
Senior usability ⚠️ chip behavior non-obvious ✅ familiar
Implementation scope High Low

The only functional advantage of contenteditable for this use case is "Backspace deletes the chip as a unit". For a family commenting tool, that is not worth the accessibility and implementation cost.

The MentionEditor.svelte component interface (onsubmit: { body, mentionedUserIds }) stays identical regardless of which editor is used — it's a clean swap-in if chips are revisited later.

Recommendation: implement the textarea path first.

🟡 On the XSS fix — one addition

The architect's escape-first fix is correct and non-negotiable. One addition: String.replace() only replaces the first occurrence. If the same person is mentioned twice in a comment, the second instance won't be linked:

// Use replaceAll(), not replace()
safe = safe.replaceAll(name, `<a href="/users/${m.id}" class="mention-link">${name}</a>`);

🟡 Medium — @ trigger is non-discoverable for seniors

A 68-year-old typing a comment will not know to type @. Add a small people-icon button adjacent to the submit button that inserts @ at the cursor position and opens the popup. Power users still type @; the button serves users who don't know the convention. This applies equally to both the contenteditable and textarea implementations.

🟡 Medium — Chip deletion must be announced to screen readers

If contenteditable is implemented: when Backspace removes a chip, the browser removes the DOM node silently. Screen readers will not announce the deletion. Add a visually-hidden aria-live="polite" announcer div and push "@Hans Mueller entfernt" to it on each deletion.

✅ What's good

  • Server-side mention rendering (backend returns mentionDTOs) — eliminates fragile client-side name parsing. Correct call.
  • comment_mentions join table — parse-free, query-friendly, clean.
  • GET /api/users/search scoped to AppUser only (not Person records) — correct, only real accounts can receive notifications.
  • Keyboard navigation spec for the popup (↑↓ Enter Tab Escape) is complete.
## UX Review — @leonievoss The architect has already flagged the XSS blocker and the `contenteditable` question. I want to add the UX and accessibility layer to both. ### 🔴 Blocker — `contenteditable` has no ARIA role in the spec The architect's comment correctly raises `contenteditable` as an implementation risk. From an accessibility standpoint, it is also a **silent failure**: a `contenteditable` div has no implicit ARIA role. Screen readers announce it as a generic region — users cannot tell it's an editable field. If `contenteditable` is used, these attributes are required on the editor div: ```svelte <div contenteditable="true" role="textbox" aria-multiline="true" aria-label="Kommentar schreiben" aria-autocomplete="list" aria-haspopup="listbox" aria-expanded={popupOpen} aria-controls="mention-popup" aria-activedescendant={activeOptionId} bind:this={editorEl} /> ``` And `MentionPopup` needs `id="mention-popup"` `role="listbox"`, each row `role="option"` `aria-selected={i === selectedIndex}`. Without the full combobox ARIA pattern, this feature is invisible to assistive technology. ### 🔴 Blocker — Mobile keyboard stability is unspecified On iOS Safari and Chrome Android, programmatic DOM mutations during chip insertion frequently collapse the virtual keyboard. This is not a theoretical risk — it will happen. The implementation notes must specify that chip insertion is tested on both platforms before merge. This is a showstopper for a family app where a significant portion of users are on mobile. ### 🟡 Design recommendation — take the textarea path The architect raises this as a consideration. I'm making it a design recommendation, based on the combined accessibility and implementation risk picture: | | `contenteditable` chips | Textarea + `@Name` plain text | |---|---|---| | Chip deletes as one unit | ✅ | ❌ | | Works on iOS Safari | ⚠️ fragile | ✅ | | Screen reader navigable | ⚠️ full ARIA combobox required | ✅ native textarea ARIA | | Mobile virtual keyboard | ⚠️ DOM mutations can collapse it | ✅ stable | | Senior usability | ⚠️ chip behavior non-obvious | ✅ familiar | | Implementation scope | High | Low | The only functional advantage of `contenteditable` for this use case is "Backspace deletes the chip as a unit". For a family commenting tool, that is not worth the accessibility and implementation cost. The `MentionEditor.svelte` component interface (`onsubmit: { body, mentionedUserIds }`) stays identical regardless of which editor is used — it's a clean swap-in if chips are revisited later. **Recommendation: implement the textarea path first.** ### 🟡 On the XSS fix — one addition The architect's escape-first fix is correct and non-negotiable. One addition: `String.replace()` only replaces the **first** occurrence. If the same person is mentioned twice in a comment, the second instance won't be linked: ```typescript // Use replaceAll(), not replace() safe = safe.replaceAll(name, `<a href="/users/${m.id}" class="mention-link">${name}</a>`); ``` ### 🟡 Medium — `@` trigger is non-discoverable for seniors A 68-year-old typing a comment will not know to type `@`. Add a small people-icon button adjacent to the submit button that inserts `@` at the cursor position and opens the popup. Power users still type `@`; the button serves users who don't know the convention. This applies equally to both the `contenteditable` and textarea implementations. ### 🟡 Medium — Chip deletion must be announced to screen readers If `contenteditable` is implemented: when Backspace removes a chip, the browser removes the DOM node silently. Screen readers will not announce the deletion. Add a visually-hidden `aria-live="polite"` announcer div and push `"@Hans Mueller entfernt"` to it on each deletion. ### ✅ What's good - Server-side mention rendering (backend returns `mentionDTOs`) — eliminates fragile client-side name parsing. Correct call. - `comment_mentions` join table — parse-free, query-friendly, clean. - `GET /api/users/search` scoped to `AppUser` only (not `Person` records) — correct, only real accounts can receive notifications. - Keyboard navigation spec for the popup (↑↓ Enter Tab Escape) is complete.
marcel commented 2026-03-27 19:13:06 +01:00
Author
Owner
Copy Link

QA Review — @saraholt

The architect and Leonie have flagged the two biggest risks. I want to add the test coverage dimension to both.

🔴 Blocking — XSS fix needs a regression test before anything else ships

Markus's renderBody fix is correct and non-negotiable. A fix without a test is a fix that will regress. This test must exist before merge:

// renderBody.test.ts
describe('renderBody', () => {
  it('escapes raw HTML in comment body before injecting mention links', () => {
    const comment = {
      body: '<script>alert(1)</script> hello @Hans Mueller',
      mentionDTOs: [{ id: 'uuid-1', firstName: 'Hans', lastName: 'Mueller' }]
    };
    const result = renderBody(comment);
    expect(result).not.toContain('<script>');
    expect(result).toContain('&lt;script&gt;');
    expect(result).toContain('<a href="/users/uuid-1"');
  });

  it('replaces all occurrences of the same mention, not just the first', () => {
    const comment = {
      body: 'Hey @Hans Mueller, thanks @Hans Mueller',
      mentionDTOs: [{ id: 'uuid-1', firstName: 'Hans', lastName: 'Mueller' }]
    };
    const result = renderBody(comment);
    expect((result.match(/href="\/users\/uuid-1"/g) || []).length).toBe(2);
  });

  it('handles a body with no mentions', () => {
    const comment = { body: 'plain comment', mentionDTOs: [] };
    expect(renderBody(comment)).toBe('plain comment');
  });
});

The second test encodes Leonie's replaceAll fix — it will fail if replace() is used instead.

🔴 Blocking — contenteditable makes unit tests very hard; decide before writing any code

Markus raises this as a consideration. I'm making it a hard requirement to decide before implementation starts, because it affects the test strategy:

contenteditable chip insertion depends on execCommand, Range, and Selection APIs that are not implemented in jsdom. This means extractContent and detectMention cannot be tested in Vitest without browser shims. You either:

  • Write those tests in Playwright component test mode (extra CI infrastructure not in the plan), or
  • Accept that the core editor logic has no unit tests

The textarea path avoids this entirely. detectMention and extractContent become pure string/regex functions — zero DOM setup, trivially testable in Vitest:

it('detectMention returns query string after @', () => {
  expect(detectMention('Hello @Ha', 9)).toEqual({ query: 'Ha', start: 6 });
});

it('detectMention returns null when @ is followed by a space', () => {
  expect(detectMention('Hello @ world', 7)).toBeNull();
});

it('extractContent returns body and mentionedUserIds from value', () => {
  const result = extractContent('Hey @Hans Mueller, see you', [
    { id: 'uuid-1', firstName: 'Hans', lastName: 'Mueller' }
  ]);
  expect(result.body).toBe('Hey @Hans Mueller, see you');
  expect(result.mentionedUserIds).toEqual(['uuid-1']);
});

The MentionEditor component interface (onsubmit: { body, mentionedUserIds }) is identical regardless of implementation — it's a clean swap-in if chips are revisited later. Recommendation: implement the textarea path and keep the chip approach as a future enhancement.

🟡 Missing unit tests regardless of which editor is chosen

Test Type
GET /api/users/search?q=Hans returns max 10 results @WebMvcTest
GET /api/users/search with empty q returns empty list @WebMvcTest
Unauthenticated GET /api/users/search returns 401 @WebMvcTest (already in plan ✅)
CommentService.postComment stores mention records in comment_mentions @DataJpaTest with Testcontainers
CommentService populates mentionDTOs on every returned comment Unit test with Mockito

🟡 E2E — verify XSS payload does not execute

Once #116 (CSP headers) ships, add to the E2E suite:

test('XSS payload in comment body is not executed', async ({ page }) => {
  // Post a comment with a script tag via the API directly
  // Then navigate to the document and verify no alert fired
  let alertFired = false;
  page.on('dialog', () => { alertFired = true; });
  await page.goto(`/documents/${docId}`);
  expect(alertFired).toBe(false);
});

Until CSP is in place this is defense-in-depth; after #116 it becomes a regression guard for both layers.

✅ What's solid

  • Server-side mention rendering (mentionDTOs from backend) eliminates fragile client-side name parsing — correct call
  • comment_mentions join table is parse-free and query-friendly
  • GET /api/users/search scoped to AppUser only (not Person records) — only real accounts receive notifications
  • Keyboard navigation spec for the popup (↑↓ Enter Tab Escape) is complete and testable
## QA Review — @saraholt The architect and Leonie have flagged the two biggest risks. I want to add the test coverage dimension to both. --- ### 🔴 Blocking — XSS fix needs a regression test before anything else ships Markus's `renderBody` fix is correct and non-negotiable. A fix without a test is a fix that will regress. This test must exist before merge: ```typescript // renderBody.test.ts describe('renderBody', () => { it('escapes raw HTML in comment body before injecting mention links', () => { const comment = { body: '<script>alert(1)</script> hello @Hans Mueller', mentionDTOs: [{ id: 'uuid-1', firstName: 'Hans', lastName: 'Mueller' }] }; const result = renderBody(comment); expect(result).not.toContain('<script>'); expect(result).toContain('&lt;script&gt;'); expect(result).toContain('<a href="/users/uuid-1"'); }); it('replaces all occurrences of the same mention, not just the first', () => { const comment = { body: 'Hey @Hans Mueller, thanks @Hans Mueller', mentionDTOs: [{ id: 'uuid-1', firstName: 'Hans', lastName: 'Mueller' }] }; const result = renderBody(comment); expect((result.match(/href="\/users\/uuid-1"/g) || []).length).toBe(2); }); it('handles a body with no mentions', () => { const comment = { body: 'plain comment', mentionDTOs: [] }; expect(renderBody(comment)).toBe('plain comment'); }); }); ``` The second test encodes Leonie's `replaceAll` fix — it will fail if `replace()` is used instead. --- ### 🔴 Blocking — `contenteditable` makes unit tests very hard; decide before writing any code Markus raises this as a consideration. I'm making it a hard requirement to **decide before implementation starts**, because it affects the test strategy: `contenteditable` chip insertion depends on `execCommand`, `Range`, and `Selection` APIs that are not implemented in jsdom. This means `extractContent` and `detectMention` cannot be tested in Vitest without browser shims. You either: - Write those tests in Playwright component test mode (extra CI infrastructure not in the plan), or - Accept that the core editor logic has no unit tests The textarea path avoids this entirely. `detectMention` and `extractContent` become pure string/regex functions — zero DOM setup, trivially testable in Vitest: ```typescript it('detectMention returns query string after @', () => { expect(detectMention('Hello @Ha', 9)).toEqual({ query: 'Ha', start: 6 }); }); it('detectMention returns null when @ is followed by a space', () => { expect(detectMention('Hello @ world', 7)).toBeNull(); }); it('extractContent returns body and mentionedUserIds from value', () => { const result = extractContent('Hey @Hans Mueller, see you', [ { id: 'uuid-1', firstName: 'Hans', lastName: 'Mueller' } ]); expect(result.body).toBe('Hey @Hans Mueller, see you'); expect(result.mentionedUserIds).toEqual(['uuid-1']); }); ``` The `MentionEditor` component interface (`onsubmit: { body, mentionedUserIds }`) is identical regardless of implementation — it's a clean swap-in if chips are revisited later. **Recommendation: implement the textarea path and keep the chip approach as a future enhancement.** --- ### 🟡 Missing unit tests regardless of which editor is chosen | Test | Type | |---|---| | `GET /api/users/search?q=Hans` returns max 10 results | `@WebMvcTest` | | `GET /api/users/search` with empty `q` returns empty list | `@WebMvcTest` | | Unauthenticated `GET /api/users/search` returns 401 | `@WebMvcTest` (already in plan ✅) | | `CommentService.postComment` stores mention records in `comment_mentions` | `@DataJpaTest` with Testcontainers | | `CommentService` populates `mentionDTOs` on every returned comment | Unit test with Mockito | --- ### 🟡 E2E — verify XSS payload does not execute Once #116 (CSP headers) ships, add to the E2E suite: ```typescript test('XSS payload in comment body is not executed', async ({ page }) => { // Post a comment with a script tag via the API directly // Then navigate to the document and verify no alert fired let alertFired = false; page.on('dialog', () => { alertFired = true; }); await page.goto(`/documents/${docId}`); expect(alertFired).toBe(false); }); ``` Until CSP is in place this is defense-in-depth; after #116 it becomes a regression guard for both layers. --- ### ✅ What's solid - Server-side mention rendering (`mentionDTOs` from backend) eliminates fragile client-side name parsing — correct call - `comment_mentions` join table is parse-free and query-friendly - `GET /api/users/search` scoped to `AppUser` only (not `Person` records) — only real accounts receive notifications - Keyboard navigation spec for the popup (↑↓ Enter Tab Escape) is complete and testable
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 notification
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#72
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?