Split CommentThread.svelte (310 lines) — extract CommentMessage component #198

New Issue

marcel · 2026-04-07T10:48:17+02:00

marcel commented

2026-04-07 10:48:17 +02:00

Context

CommentThread.svelte is 310 lines. The {#each flatMessages} loop body (lines 223-294) is ~70 lines of nested markup with conditional edit mode, quote extraction, avatar rendering, and delete button. That loop body is a distinct visual region: one comment message bubble.

Proposed Split

`CommentMessage.svelte`

Extract the per-message rendering:

Props: message (FlatMessage), isOwn (boolean), onEdit, onDelete, editingId, editText, onEditTextChange, onEditKeydown, onCancelEdit
Renders: avatar circle, author name, timestamp, edited label, quoted text block, message body with mentions, edit textarea (conditional), delete button (conditional)

`CommentThread.svelte` becomes simpler

Keeps: comment CRUD operations, flat message derivation, compose area
The {#each} loop body becomes <CommentMessage ... />

Acceptance Criteria

CommentThread.svelte under ~180 lines
CommentMessage.svelte is a focused single-message renderer
Edit-in-place, quote rendering, mention highlighting, delete all work identically
npm run check passes

## Context `CommentThread.svelte` is 310 lines. The `{#each flatMessages}` loop body (lines 223-294) is ~70 lines of nested markup with conditional edit mode, quote extraction, avatar rendering, and delete button. That loop body is a distinct visual region: one comment message bubble. ## Proposed Split ### `CommentMessage.svelte` Extract the per-message rendering: - Props: `message` (FlatMessage), `isOwn` (boolean), `onEdit`, `onDelete`, `editingId`, `editText`, `onEditTextChange`, `onEditKeydown`, `onCancelEdit` - Renders: avatar circle, author name, timestamp, edited label, quoted text block, message body with mentions, edit textarea (conditional), delete button (conditional) ### `CommentThread.svelte` becomes simpler - Keeps: comment CRUD operations, flat message derivation, compose area - The `{#each}` loop body becomes `<CommentMessage ... />` ## Acceptance Criteria - [ ] CommentThread.svelte under ~180 lines - [ ] CommentMessage.svelte is a focused single-message renderer - [ ] Edit-in-place, quote rendering, mention highlighting, delete all work identically - [ ] `npm run check` passes

marcel added the refactor label 2026-04-07 10:49:00 +02:00

marcel commented

2026-04-07 10:51:25 +02:00

👨‍💻 Felix Brandt -- Senior Fullstack Developer

Good refactor target. The {#each} loop body at lines 223-294 is exactly the kind of visual boundary that deserves its own component -- a single message bubble is a nameable UI region ("CommentMessage"), not a "Helper" or "Wrapper".

Props interface concerns:

The proposed props list has 8 items: message, isOwn, onEdit, onDelete, editingId, editText, onEditTextChange, onEditKeydown, onCancelEdit. That is above the 3-parameter guideline. Consider grouping the edit-related callbacks into an editState object prop:
```
type EditState = {
  editingId: string | null;
  editText: string;
  onTextChange: (text: string) => void;
  onKeydown: (e: KeyboardEvent) => void;
  onCancel: () => void;
};
```
This brings the prop count down to 5 (message, isOwn, onEdit, onDelete, editState) and makes the edit concern cohesive.
isOwn can be derived from message if the current user ID is available. Is it passed through context, or does the parent compute it? If the parent computes it, that is fine -- but document why it is not derived inside the child.

Svelte 5 checklist for the extraction:

The {#each} in CommentThread must keep its key expression (message.id) after the extraction -- verify this is preserved.
Any $derived values that depend on the loop variable need to move into CommentMessage or become props. Which derived values exist in the current loop body?
If the edit textarea uses bind:value, that two-way binding needs to become a callback prop (onEditTextChange) as the issue suggests -- good.

Testing:

The issue's acceptance criteria say "edit-in-place, quote rendering, mention highlighting, delete all work identically." These are four distinct behaviors that should each have a Vitest test in a new CommentMessage.test.ts. Does the current CommentThread have tests covering these? If so, they should migrate to the new component's test file.
A thin integration test should verify that CommentThread still orchestrates the CRUD lifecycle correctly after the split.

Question:

The issue mentions "quote extraction" as part of the loop body. Is the quote parsing logic (extracting the quoted text from the message body) currently inline in the template, or is it a utility function? If inline, this refactor is a good opportunity to extract it into a pure function in $lib/utils -- testable in isolation.

## 👨‍💻 Felix Brandt -- Senior Fullstack Developer Good refactor target. The `{#each}` loop body at lines 223-294 is exactly the kind of visual boundary that deserves its own component -- a single message bubble is a nameable UI region ("CommentMessage"), not a "Helper" or "Wrapper". **Props interface concerns:** - The proposed props list has 8 items: `message`, `isOwn`, `onEdit`, `onDelete`, `editingId`, `editText`, `onEditTextChange`, `onEditKeydown`, `onCancelEdit`. That is above the 3-parameter guideline. Consider grouping the edit-related callbacks into an `editState` object prop: ```typescript type EditState = { editingId: string | null; editText: string; onTextChange: (text: string) => void; onKeydown: (e: KeyboardEvent) => void; onCancel: () => void; }; ``` This brings the prop count down to 5 (`message`, `isOwn`, `onEdit`, `onDelete`, `editState`) and makes the edit concern cohesive. - `isOwn` can be derived from `message` if the current user ID is available. Is it passed through context, or does the parent compute it? If the parent computes it, that is fine -- but document why it is not derived inside the child. **Svelte 5 checklist for the extraction:** - The `{#each}` in `CommentThread` must keep its key expression `(message.id)` after the extraction -- verify this is preserved. - Any `$derived` values that depend on the loop variable need to move into `CommentMessage` or become props. Which derived values exist in the current loop body? - If the edit textarea uses `bind:value`, that two-way binding needs to become a callback prop (`onEditTextChange`) as the issue suggests -- good. **Testing:** - The issue's acceptance criteria say "edit-in-place, quote rendering, mention highlighting, delete all work identically." These are four distinct behaviors that should each have a Vitest test in a new `CommentMessage.test.ts`. Does the current `CommentThread` have tests covering these? If so, they should migrate to the new component's test file. - A thin integration test should verify that `CommentThread` still orchestrates the CRUD lifecycle correctly after the split. **Question:** - The issue mentions "quote extraction" as part of the loop body. Is the quote parsing logic (extracting the quoted text from the message body) currently inline in the template, or is it a utility function? If inline, this refactor is a good opportunity to extract it into a pure function in `$lib/utils` -- testable in isolation.

marcel commented

2026-04-07 10:51:41 +02:00

🏗️ Markus Keller -- Senior Application Architect

Clean split target. The parent keeps state and CRUD orchestration, the child is a pure renderer. That is the correct boundary.

Module boundary clarity:

The issue says CommentThread "keeps comment CRUD operations, flat message derivation, compose area." Confirm that CommentMessage has zero awareness of the API layer -- it receives data and fires callbacks, nothing else. If any fetch call or API client import ends up in CommentMessage, the boundary is wrong.
The "flat message derivation" logic (turning nested thread data into a flat list) stays in the parent. Good. But is this derivation a $derived block or an imperative function? If it is complex enough to have edge cases (empty threads, deleted messages, orphaned replies), it may deserve extraction into a pure utility module (flattenMessages.ts) that can be unit-tested independently of Svelte reactivity. This refactor is a natural time to do that.

Data flow direction:

The proposed callback props (onEdit, onDelete, onEditTextChange, onEditKeydown, onCancelEdit) all flow upward. That is correct -- commands go up, data goes down. But 5 callbacks on one child component is a signal. Consider whether the edit state machine (editing ID, edit text, save, cancel) could be encapsulated in the parent and exposed as a single editState prop with an onSave and onCancel callback. This simplifies the child's contract.

Accidental complexity check:

The issue targets ~180 lines for the slimmed-down CommentThread. That is still substantial. After the extraction, audit what remains: if the compose area (new message input + send button) is another 40-60 lines of template, it may be a second extraction candidate (CommentCompose.svelte). No need to do it in this issue, but worth noting for a follow-up.

Question:

Does the FlatMessage type currently live inside CommentThread.svelte's <script> block, or is it in a shared types file? After the split, both CommentThread and CommentMessage need it. If it is not already in $lib/types or similar, move it there as part of this refactor.

## 🏗️ Markus Keller -- Senior Application Architect Clean split target. The parent keeps state and CRUD orchestration, the child is a pure renderer. That is the correct boundary. **Module boundary clarity:** - The issue says `CommentThread` "keeps comment CRUD operations, flat message derivation, compose area." Confirm that `CommentMessage` has zero awareness of the API layer -- it receives data and fires callbacks, nothing else. If any `fetch` call or API client import ends up in `CommentMessage`, the boundary is wrong. - The "flat message derivation" logic (turning nested thread data into a flat list) stays in the parent. Good. But is this derivation a `$derived` block or an imperative function? If it is complex enough to have edge cases (empty threads, deleted messages, orphaned replies), it may deserve extraction into a pure utility module (`flattenMessages.ts`) that can be unit-tested independently of Svelte reactivity. This refactor is a natural time to do that. **Data flow direction:** - The proposed callback props (`onEdit`, `onDelete`, `onEditTextChange`, `onEditKeydown`, `onCancelEdit`) all flow upward. That is correct -- commands go up, data goes down. But 5 callbacks on one child component is a signal. Consider whether the edit state machine (editing ID, edit text, save, cancel) could be encapsulated in the parent and exposed as a single `editState` prop with an `onSave` and `onCancel` callback. This simplifies the child's contract. **Accidental complexity check:** - The issue targets ~180 lines for the slimmed-down `CommentThread`. That is still substantial. After the extraction, audit what remains: if the compose area (new message input + send button) is another 40-60 lines of template, it may be a second extraction candidate (`CommentCompose.svelte`). No need to do it in this issue, but worth noting for a follow-up. **Question:** - Does the `FlatMessage` type currently live inside `CommentThread.svelte`'s `<script>` block, or is it in a shared types file? After the split, both `CommentThread` and `CommentMessage` need it. If it is not already in `$lib/types` or similar, move it there as part of this refactor.

marcel commented

2026-04-07 10:51:54 +02:00

🧪 Sara Holt -- Senior QA Engineer

The acceptance criteria list four behaviors that must work identically after the split: edit-in-place, quote rendering, mention highlighting, and delete. That is a good checklist, but it needs test coverage to be enforceable.

Current test coverage question:

Does CommentThread have existing Vitest tests covering these four behaviors? If yes, the migration plan should be explicit: which tests move to CommentMessage.test.ts (rendering concerns) and which stay in CommentThread.test.ts (orchestration concerns). If no tests exist, this refactor is the right time to add them -- extracting a component without tests means "works identically" is verified by manual inspection only.

Proposed test strategy for CommentMessage:

renders author name and avatar initials -- basic rendering
renders timestamp and "edited" label when message was edited -- conditional display
renders quoted text block when message contains a quote -- quote extraction
highlights @mentions in message body -- mention highlighting
enters edit mode when onEdit is triggered -- conditional edit textarea
shows delete button only for own messages (isOwn=true) -- conditional rendering
does not show delete button for other users' messages -- negative case

Each of these is a focused unit test using @testing-library/svelte that renders CommentMessage with specific props and asserts on the DOM output.

Integration-level concern:

After the split, CommentThread orchestrates: load messages, send new message, edit existing, delete. A thin integration test should verify the parent-child wiring -- that clicking delete in the child triggers the parent's delete handler and the message disappears from the list. This catches prop-wiring regressions that unit tests on isolated components would miss.

Edge cases to cover:

Empty message body (just whitespace)
Message with only a quote and no body text
Very long message body (does it overflow or truncate?)
Message from a deleted user (is author name still available in FlatMessage?)

Acceptance criteria gap:

The issue says "npm run check passes" but does not mention npm run test. Add npm run test passing as an explicit acceptance criterion.

## 🧪 Sara Holt -- Senior QA Engineer The acceptance criteria list four behaviors that must work identically after the split: edit-in-place, quote rendering, mention highlighting, and delete. That is a good checklist, but it needs test coverage to be enforceable. **Current test coverage question:** - Does `CommentThread` have existing Vitest tests covering these four behaviors? If yes, the migration plan should be explicit: which tests move to `CommentMessage.test.ts` (rendering concerns) and which stay in `CommentThread.test.ts` (orchestration concerns). If no tests exist, this refactor is the right time to add them -- extracting a component without tests means "works identically" is verified by manual inspection only. **Proposed test strategy for `CommentMessage`:** - `renders author name and avatar initials` -- basic rendering - `renders timestamp and "edited" label when message was edited` -- conditional display - `renders quoted text block when message contains a quote` -- quote extraction - `highlights @mentions in message body` -- mention highlighting - `enters edit mode when onEdit is triggered` -- conditional edit textarea - `shows delete button only for own messages (isOwn=true)` -- conditional rendering - `does not show delete button for other users' messages` -- negative case Each of these is a focused unit test using `@testing-library/svelte` that renders `CommentMessage` with specific props and asserts on the DOM output. **Integration-level concern:** - After the split, `CommentThread` orchestrates: load messages, send new message, edit existing, delete. A thin integration test should verify the parent-child wiring -- that clicking delete in the child triggers the parent's delete handler and the message disappears from the list. This catches prop-wiring regressions that unit tests on isolated components would miss. **Edge cases to cover:** - Empty message body (just whitespace) - Message with only a quote and no body text - Very long message body (does it overflow or truncate?) - Message from a deleted user (is author name still available in `FlatMessage`?) **Acceptance criteria gap:** - The issue says "`npm run check` passes" but does not mention `npm run test`. Add `npm run test` passing as an explicit acceptance criterion.

marcel commented

2026-04-07 10:52:06 +02:00

🔒 Nora "NullX" Steiner -- Application Security Engineer

Pure UI refactor, so the attack surface should not change -- but component splits can introduce subtle security regressions if not handled carefully.

XSS surface in the extracted component:

The issue mentions "mention highlighting" and "message body" rendering. How are these rendered? If the message body uses {@html} to render formatted content or highlight mentions, that {@html} usage moves into CommentMessage.svelte. Verify that the sanitization step (if any) stays upstream of the render -- it must not be accidentally dropped during extraction. If there is no sanitization and the content is rendered via {@html}, flag that as a pre-existing issue to address.
Quote extraction: if the quoted text is rendered with {@html}, the same concern applies. User-authored content passed through {@html} without DOMPurify or equivalent is a stored XSS vector.

Authorization check for delete:

The isOwn prop controls whether the delete button renders. But the actual delete must also be authorized server-side -- the frontend hiding a button is not a security control. Confirm that the backend DELETE endpoint verifies ownership independently. This is likely already in place, but the component split is a good moment to verify.

Callback trust boundary:

CommentMessage receives onDelete and onEdit as callback props from the parent. The child should pass only the message ID back to the parent -- never construct API calls or include auth tokens. After the split, verify that the child component has zero imports from $lib/api.server.ts or any server-side module.

Question:

Does the edit textarea accept arbitrary HTML/markdown input, or plain text only? If markdown is supported, the rendering pipeline (markdown -> HTML -> DOM) needs to sanitize at the conversion step. The split should not change where sanitization happens, but it is worth confirming the pipeline is intact.

## 🔒 Nora "NullX" Steiner -- Application Security Engineer Pure UI refactor, so the attack surface should not change -- but component splits can introduce subtle security regressions if not handled carefully. **XSS surface in the extracted component:** - The issue mentions "mention highlighting" and "message body" rendering. How are these rendered? If the message body uses `{@html}` to render formatted content or highlight mentions, that `{@html}` usage moves into `CommentMessage.svelte`. Verify that the sanitization step (if any) stays upstream of the render -- it must not be accidentally dropped during extraction. If there is no sanitization and the content is rendered via `{@html}`, flag that as a pre-existing issue to address. - Quote extraction: if the quoted text is rendered with `{@html}`, the same concern applies. User-authored content passed through `{@html}` without DOMPurify or equivalent is a stored XSS vector. **Authorization check for delete:** - The `isOwn` prop controls whether the delete button renders. But the actual delete must also be authorized server-side -- the frontend hiding a button is not a security control. Confirm that the backend `DELETE` endpoint verifies ownership independently. This is likely already in place, but the component split is a good moment to verify. **Callback trust boundary:** - `CommentMessage` receives `onDelete` and `onEdit` as callback props from the parent. The child should pass only the message ID back to the parent -- never construct API calls or include auth tokens. After the split, verify that the child component has zero imports from `$lib/api.server.ts` or any server-side module. **Question:** - Does the edit textarea accept arbitrary HTML/markdown input, or plain text only? If markdown is supported, the rendering pipeline (markdown -> HTML -> DOM) needs to sanitize at the conversion step. The split should not change where sanitization happens, but it is worth confirming the pipeline is intact.

marcel commented

2026-04-07 10:52:19 +02:00

🎨 Leonie Voss -- UI/UX Design Lead

The message bubble is a well-defined visual region -- avatar, name, timestamp, body, optional quote, optional edit controls. Extracting it into its own component makes the visual contract explicit, which is good for design consistency.

Accessibility concerns to preserve during extraction:

Each message bubble likely needs appropriate ARIA attributes. After the split, verify:
- The message list ({#each}) parent has role="list" or uses a semantic <ul>/<ol>
- Each CommentMessage renders as a <li> or has role="listitem"
- The delete button has an aria-label that includes the message context (e.g., "Delete message from [author]"), not just "Delete"
- The edit textarea has a visible or aria-label that identifies what is being edited
- Focus management: when entering edit mode, focus should move to the textarea; when canceling, focus should return to the edit trigger button

Touch target sizes:

The delete button and edit trigger must remain at least 44x44px touch targets. When extracting markup, verify that the surrounding padding/spacing that contributes to the touch target is included in CommentMessage, not left behind in the parent's layout.

Visual consistency:

The "own message" vs "other message" styling (likely different alignment or background color) should be driven by the isOwn prop. After extraction, verify the conditional classes are self-contained in CommentMessage and do not depend on a parent CSS context that might break if the component is used elsewhere.

Quote block styling:

The quoted text block is a distinct visual sub-region within the message. After extraction, verify it retains its left border / indentation / italic styling as a self-contained pattern. If the quote styles depend on parent-scoped CSS, they will break when the markup moves to a child component -- Svelte's style scoping will exclude them.

Question:

Does the current message bubble have different states for hover/focus/active (e.g., a subtle background shift to indicate the message is interactive)? If so, those interaction states need to move cleanly into CommentMessage with the markup.

## 🎨 Leonie Voss -- UI/UX Design Lead The message bubble is a well-defined visual region -- avatar, name, timestamp, body, optional quote, optional edit controls. Extracting it into its own component makes the visual contract explicit, which is good for design consistency. **Accessibility concerns to preserve during extraction:** - Each message bubble likely needs appropriate ARIA attributes. After the split, verify: - The message list (`{#each}`) parent has `role="list"` or uses a semantic `<ul>`/`<ol>` - Each `CommentMessage` renders as a `<li>` or has `role="listitem"` - The delete button has an `aria-label` that includes the message context (e.g., "Delete message from [author]"), not just "Delete" - The edit textarea has a visible or `aria-label` that identifies what is being edited - Focus management: when entering edit mode, focus should move to the textarea; when canceling, focus should return to the edit trigger button **Touch target sizes:** - The delete button and edit trigger must remain at least 44x44px touch targets. When extracting markup, verify that the surrounding padding/spacing that contributes to the touch target is included in `CommentMessage`, not left behind in the parent's layout. **Visual consistency:** - The "own message" vs "other message" styling (likely different alignment or background color) should be driven by the `isOwn` prop. After extraction, verify the conditional classes are self-contained in `CommentMessage` and do not depend on a parent CSS context that might break if the component is used elsewhere. **Quote block styling:** - The quoted text block is a distinct visual sub-region within the message. After extraction, verify it retains its left border / indentation / italic styling as a self-contained pattern. If the quote styles depend on parent-scoped CSS, they will break when the markup moves to a child component -- Svelte's style scoping will exclude them. **Question:** - Does the current message bubble have different states for hover/focus/active (e.g., a subtle background shift to indicate the message is interactive)? If so, those interaction states need to move cleanly into `CommentMessage` with the markup.

marcel commented

2026-04-07 10:52:30 +02:00

⚙️ Tobias Wendt -- DevOps & Platform Engineer

Pure frontend refactor -- no infrastructure changes, no new dependencies, no build pipeline impact. From a DevOps perspective this is low-risk, but a few things to verify.

Build and bundle impact:

Adding a new .svelte component file does not change the build configuration. Vite handles it automatically. No changes needed to vite.config.ts, svelte.config.js, or any Docker build steps.
Verify that the new CommentMessage.svelte file is co-located with CommentThread.svelte in the same directory (likely src/lib/components/ or src/routes/conversations/). This keeps the import path short and avoids barrel-file complications.

CI pipeline check:

The acceptance criteria include npm run check passing. The CI workflow should already run this. Confirm that npm run test (Vitest) is also part of the pipeline for this change -- the type check alone does not catch behavioral regressions.
No new environment variables, no new Docker services, no Flyway migration. The CI matrix does not need changes.

No-op from a deployment perspective:

This refactor does not affect the SvelteKit adapter-node output, the Docker image layers, or the Caddy routing. Deployment is unchanged.
The only thing to watch: if the conversations page is tested in the Playwright E2E suite, ensure those tests still pass after the split. The E2E tests interact with the rendered DOM, not component internals, so they should be unaffected -- but confirm.

Suggestion:

If this is the start of a broader CommentThread decomposition (the issue hints at the compose area being another candidate), consider creating a conversations/ subfolder under components/ to group CommentThread, CommentMessage, and any future siblings. Keeps the flat components/ directory manageable as the project grows.

## ⚙️ Tobias Wendt -- DevOps & Platform Engineer Pure frontend refactor -- no infrastructure changes, no new dependencies, no build pipeline impact. From a DevOps perspective this is low-risk, but a few things to verify. **Build and bundle impact:** - Adding a new `.svelte` component file does not change the build configuration. Vite handles it automatically. No changes needed to `vite.config.ts`, `svelte.config.js`, or any Docker build steps. - Verify that the new `CommentMessage.svelte` file is co-located with `CommentThread.svelte` in the same directory (likely `src/lib/components/` or `src/routes/conversations/`). This keeps the import path short and avoids barrel-file complications. **CI pipeline check:** - The acceptance criteria include `npm run check` passing. The CI workflow should already run this. Confirm that `npm run test` (Vitest) is also part of the pipeline for this change -- the type check alone does not catch behavioral regressions. - No new environment variables, no new Docker services, no Flyway migration. The CI matrix does not need changes. **No-op from a deployment perspective:** - This refactor does not affect the SvelteKit adapter-node output, the Docker image layers, or the Caddy routing. Deployment is unchanged. - The only thing to watch: if the conversations page is tested in the Playwright E2E suite, ensure those tests still pass after the split. The E2E tests interact with the rendered DOM, not component internals, so they should be unaffected -- but confirm. **Suggestion:** - If this is the start of a broader `CommentThread` decomposition (the issue hints at the compose area being another candidate), consider creating a `conversations/` subfolder under `components/` to group `CommentThread`, `CommentMessage`, and any future siblings. Keeps the flat `components/` directory manageable as the project grows.

marcel commented

2026-04-07 11:05:35 +02:00

Comprehensive Response to Review Comments

I read through all source files referenced in the issue. Here are concrete findings per persona.

Felix Brandt -- Props Interface & Quote Parsing

Prop count: The proposed 8 props are accurate. The editState grouping is a good call. Revised interface:

type EditState = {
  editingId: string | null;
  editText: string;
  onTextChange: (text: string) => void;
  onKeydown: (e: KeyboardEvent) => void;
  onCancel: () => void;
};

// CommentMessage props: message, isOwn, onEdit, onDelete, editState
// That is 5 props -- acceptable.

isOwn derivation: The parent computes it via isOwn() at CommentThread.svelte:85-87, comparing currentUserId (a prop on CommentThread) against c.authorId. Keeping it as a computed prop passed down is correct -- CommentMessage should not need currentUserId context. No change needed, but we will document the reason in a code comment.

{#each} key expression: Currently (msg.id) at line 223. Will be preserved on the <CommentMessage> element after extraction.

$derived in loop body: There are no $derived values inside the loop. The only per-message computation is the {@const parsed = extractQuote(msg.content)} at line 224, which is a template-local const. This will move into CommentMessage unchanged.

Quote parsing: extractQuote() at lines 97-101 is already a pure function in the <script> block. During extraction it will move to $lib/utils/comment.ts (or stay in CommentThread and be imported by CommentMessage). Given it is a 4-line pure function, extracting to a utility file makes it independently testable -- will do that.

The regex ^>\s*"(.+?)"\s*\n\n?([\s\S]*)$ handles:

> "quoted text"\n\nbody -- standard case
> "quoted text"\nbody -- single newline variant
No quote present -- fallback to { quote: null, body: content }

Edge case gap: a message that is only a quote with no body (> "text"\n\n) produces { quote: "text", body: "" }. This is handled correctly by the template since the body renders as empty.

Markus Keller -- Module Boundary & FlatMessage Type

Zero API awareness in child: Confirmed. The extracted CommentMessage will have zero fetch calls and zero imports from API modules. All CRUD stays in CommentThread (lines 103-189). The child receives data + callbacks only.

FlatMessage type location: Currently defined inline in CommentThread.svelte at lines 35-43. After the split, both parent and child need it. Plan: move FlatMessage to $lib/types.ts alongside the existing Comment and CommentReply types. Note that FlatMessage is structurally identical to CommentReply (same fields). We could either reuse CommentReply directly or keep FlatMessage as a named alias for clarity. I lean toward keeping the distinct name since it represents a flattened view, not a reply per se.

Flat message derivation complexity: The derivation at line 60-62 is a one-liner flatMap. No edge cases worth extracting -- deleted messages and orphaned replies are not represented in the data model (the backend filters them). No separate utility needed.

Compose area as second extraction candidate: After extraction, CommentThread will be roughly: 100 lines of script (state + CRUD) + ~15 lines of message list template + ~12 lines of compose area. The compose area is already delegated to MentionEditor (lines 298-308), so it is only 10 lines of wrapper markup. Not worth a separate component.

Callback consolidation: Agree with the editState prop approach (see Felix section above). This brings callbacks from 5 to effectively 2 top-level ones (onEdit, onDelete) plus the editState object.

Sara Holt -- Test Coverage

Current test coverage: CommentThread.svelte.spec.ts has 3 tests covering:

Empty state hint rendering
onCountChange with initial comments
Reply counting

Not covered: edit-in-place, quote rendering, mention highlighting, delete button visibility. These are the four behaviors listed in acceptance criteria.

Test migration plan:

Tests that stay in CommentThread.svelte.spec.ts (orchestration):

onCountChange callback tests (all 3 existing)
Integration: delete triggers reload

Tests for new CommentMessage.svelte.spec.ts (rendering):

renders author name and avatar initials -- assert getInitials() output in DOM
renders timestamp; shows "edited" label when updatedAt > createdAt
renders quoted text block when content starts with > "..."
highlights @mentions via {@html renderBody()} -- assert .mention span in DOM
shows delete button only when isOwn=true
does not show delete button when isOwn=false
enters edit mode when editingId matches message id -- assert textarea present

Edge cases to add:

Empty body after quote: > "text"\n\n with empty body -- renders quote block, no body paragraph
Message with mentionDTOs: [] -- renders plain text, no .mention spans
The "deleted user" case does not apply -- authorName is always a string in FlatMessage, the backend populates it at query time

Acceptance criteria update: Agree -- add npm run test passing as explicit criterion.

Nora Steiner -- Security

{@html} and XSS: The {@html renderBody(...)} at line 264 is the only {@html} usage. Reviewing renderBody() in $lib/utils/mention.ts:53-72:

Line 54-58: escapes &, <, >, " in the raw content -- this neutralizes HTML injection
Lines 60-68: mention display names are also escaped before interpolation into the <span> tag
Line 71: newlines converted to <br> after escaping

This is safe. The escape-first approach means user content cannot inject tags. The only HTML injected is the controlled <span class="mention"> wrapper. No DOMPurify needed -- the manual escaping covers all injection vectors for plain-text content.

Quote rendering: The quoted text at line 244 uses {parsed.quote} with Svelte's default text interpolation (not {@html}), so it is auto-escaped. No XSS risk.

Backend delete authorization: Confirmed in CommentService.java:118-126. The deleteComment method checks isAuthor || isAdmin server-side and throws DomainException.forbidden() if neither. The frontend isOwn prop only controls UI visibility -- the actual security boundary is backend-enforced.

Edit input: Plain text only. No markdown parsing, no HTML rendering of edit input. The edit textarea (bind:value={editText}) captures plain text, which is sent as JSON { content: text } to the PATCH endpoint. On re-render, it goes through renderBody() which escapes everything.

Child component imports: After extraction, CommentMessage will import only from $lib/utils/mention (for renderBody) and $lib/paraglide/messages (for i18n). Zero API/fetch imports.

Leonie Voss -- Accessibility & Styling

Semantic structure: Currently the message list is a <div class="space-y-2"> (line 222) with <div> children. There are no role="list" or <ul> semantics. This is a pre-existing gap. During extraction:

The parent <div class="space-y-2"> should get role="log" (appropriate for a comment thread -- live region semantics)
Each CommentMessage root element should get role="article" (WAI-ARIA comment pattern)

Delete button aria-label: Currently uses m.btn_delete() (line 270) which is a generic "Delete" label. Should be improved to include author context: m.comment_delete_aria({ author: msg.authorName }). This requires a new Paraglide key.

Edit textarea label: No aria-label currently (line 249-254). Will add aria-label={m.comment_edit_aria({ author: msg.authorName })}.

Focus management: Not currently implemented. When editingId changes to match msg.id, the textarea appears but focus is not moved. Plan: use Svelte's {@attach} directive on the edit textarea to call node.focus() on mount. When cancelEdit() is called, focus should return to the message container -- this needs a ref to the trigger element.

Touch targets: The delete button at line 268 is p-0.5 with a h-4 w-4 SVG -- effective size is roughly 20x20px. This is below the 44x44px minimum. Will increase to p-2 or add min-h-[44px] min-w-[44px].

CSS scoping: All styles in the message bubble use Tailwind utility classes (inline), not scoped <style> blocks. No parent-scoped CSS will break during extraction. The .mention class used by {@html} is defined in global layout.css (lines 212-226), so it is unaffected by component boundaries. The quote block styling is also all inline Tailwind (border-l-2 border-line pl-2 font-serif text-base text-ink-3 italic at line 243).

Hover/focus states: The own-message paragraph has hover:bg-surface (line 261) for the edit-on-click affordance. This is inline Tailwind and will move cleanly with the markup.

Tobias Wendt -- DevOps & File Location

Co-location: CommentMessage.svelte will live in src/lib/components/ alongside CommentThread.svelte. Both are general-purpose components used from multiple routes (document detail, annotation panels, transcription blocks), so $lib/components/ is the correct location -- not a route-specific folder.

Subfolder: A conversations/ subfolder is premature. The compose area is already handled by MentionEditor.svelte and will not need further extraction (see Markus section). The component count in $lib/components/ is manageable. Can revisit if more comment-related components emerge.

CI pipeline: Confirmed npm run check and npm run test are both in the CI workflow. No Playwright e2e tests directly exercise the comment thread (checked frontend/e2e/ -- no conversation-specific spec file), so no e2e regression risk.

No infrastructure impact: Correct. No new deps, no env vars, no Docker/Flyway changes.

## Comprehensive Response to Review Comments I read through all source files referenced in the issue. Here are concrete findings per persona. --- ### Felix Brandt -- Props Interface & Quote Parsing **Prop count:** The proposed 8 props are accurate. The `editState` grouping is a good call. Revised interface: ```typescript type EditState = { editingId: string | null; editText: string; onTextChange: (text: string) => void; onKeydown: (e: KeyboardEvent) => void; onCancel: () => void; }; // CommentMessage props: message, isOwn, onEdit, onDelete, editState // That is 5 props -- acceptable. ``` **`isOwn` derivation:** The parent computes it via `isOwn()` at `CommentThread.svelte:85-87`, comparing `currentUserId` (a prop on CommentThread) against `c.authorId`. Keeping it as a computed prop passed down is correct -- `CommentMessage` should not need `currentUserId` context. No change needed, but we will document the reason in a code comment. **`{#each}` key expression:** Currently `(msg.id)` at line 223. Will be preserved on the `<CommentMessage>` element after extraction. **`$derived` in loop body:** There are no `$derived` values inside the loop. The only per-message computation is the `{@const parsed = extractQuote(msg.content)}` at line 224, which is a template-local const. This will move into `CommentMessage` unchanged. **Quote parsing:** `extractQuote()` at lines 97-101 is already a pure function in the `<script>` block. During extraction it will move to `$lib/utils/comment.ts` (or stay in `CommentThread` and be imported by `CommentMessage`). Given it is a 4-line pure function, extracting to a utility file makes it independently testable -- will do that. The regex `^>\s*"(.+?)"\s*\n\n?([\s\S]*)$` handles: - `> "quoted text"\n\nbody` -- standard case - `> "quoted text"\nbody` -- single newline variant - No quote present -- fallback to `{ quote: null, body: content }` Edge case gap: a message that is *only* a quote with no body (`> "text"\n\n`) produces `{ quote: "text", body: "" }`. This is handled correctly by the template since the body renders as empty. --- ### Markus Keller -- Module Boundary & FlatMessage Type **Zero API awareness in child:** Confirmed. The extracted `CommentMessage` will have zero `fetch` calls and zero imports from API modules. All CRUD stays in `CommentThread` (lines 103-189). The child receives data + callbacks only. **`FlatMessage` type location:** Currently defined inline in `CommentThread.svelte` at lines 35-43. After the split, both parent and child need it. Plan: move `FlatMessage` to `$lib/types.ts` alongside the existing `Comment` and `CommentReply` types. Note that `FlatMessage` is structurally identical to `CommentReply` (same fields). We could either reuse `CommentReply` directly or keep `FlatMessage` as a named alias for clarity. I lean toward keeping the distinct name since it represents a flattened view, not a reply per se. **Flat message derivation complexity:** The derivation at line 60-62 is a one-liner `flatMap`. No edge cases worth extracting -- deleted messages and orphaned replies are not represented in the data model (the backend filters them). No separate utility needed. **Compose area as second extraction candidate:** After extraction, `CommentThread` will be roughly: 100 lines of script (state + CRUD) + ~15 lines of message list template + ~12 lines of compose area. The compose area is already delegated to `MentionEditor` (lines 298-308), so it is only 10 lines of wrapper markup. Not worth a separate component. **Callback consolidation:** Agree with the `editState` prop approach (see Felix section above). This brings callbacks from 5 to effectively 2 top-level ones (`onEdit`, `onDelete`) plus the `editState` object. --- ### Sara Holt -- Test Coverage **Current test coverage:** `CommentThread.svelte.spec.ts` has 3 tests covering: - Empty state hint rendering - `onCountChange` with initial comments - Reply counting **Not covered:** edit-in-place, quote rendering, mention highlighting, delete button visibility. These are the four behaviors listed in acceptance criteria. **Test migration plan:** Tests that stay in `CommentThread.svelte.spec.ts` (orchestration): - `onCountChange` callback tests (all 3 existing) - Integration: delete triggers reload Tests for new `CommentMessage.svelte.spec.ts` (rendering): - `renders author name and avatar initials` -- assert `getInitials()` output in DOM - `renders timestamp; shows "edited" label when updatedAt > createdAt` - `renders quoted text block when content starts with > "..."` - `highlights @mentions via {@html renderBody()}` -- assert `.mention` span in DOM - `shows delete button only when isOwn=true` - `does not show delete button when isOwn=false` - `enters edit mode when editingId matches message id` -- assert textarea present **Edge cases to add:** - Empty body after quote: `> "text"\n\n` with empty body -- renders quote block, no body paragraph - Message with `mentionDTOs: []` -- renders plain text, no `.mention` spans - The "deleted user" case does not apply -- `authorName` is always a string in `FlatMessage`, the backend populates it at query time **Acceptance criteria update:** Agree -- add `npm run test` passing as explicit criterion. --- ### Nora Steiner -- Security **`{@html}` and XSS:** The `{@html renderBody(...)}` at line 264 is the only `{@html}` usage. Reviewing `renderBody()` in `$lib/utils/mention.ts:53-72`: 1. Line 54-58: escapes `&`, `<`, `>`, `"` in the raw content -- this neutralizes HTML injection 2. Lines 60-68: mention display names are also escaped before interpolation into the `<span>` tag 3. Line 71: newlines converted to `<br>` after escaping This is safe. The escape-first approach means user content cannot inject tags. The only HTML injected is the controlled `<span class="mention">` wrapper. No DOMPurify needed -- the manual escaping covers all injection vectors for plain-text content. **Quote rendering:** The quoted text at line 244 uses `{parsed.quote}` with Svelte's default text interpolation (not `{@html}`), so it is auto-escaped. No XSS risk. **Backend delete authorization:** Confirmed in `CommentService.java:118-126`. The `deleteComment` method checks `isAuthor || isAdmin` server-side and throws `DomainException.forbidden()` if neither. The frontend `isOwn` prop only controls UI visibility -- the actual security boundary is backend-enforced. **Edit input:** Plain text only. No markdown parsing, no HTML rendering of edit input. The edit textarea (`bind:value={editText}`) captures plain text, which is sent as JSON `{ content: text }` to the PATCH endpoint. On re-render, it goes through `renderBody()` which escapes everything. **Child component imports:** After extraction, `CommentMessage` will import only from `$lib/utils/mention` (for `renderBody`) and `$lib/paraglide/messages` (for i18n). Zero API/fetch imports. --- ### Leonie Voss -- Accessibility & Styling **Semantic structure:** Currently the message list is a `<div class="space-y-2">` (line 222) with `<div>` children. There are no `role="list"` or `<ul>` semantics. This is a pre-existing gap. During extraction: - The parent `<div class="space-y-2">` should get `role="log"` (appropriate for a comment thread -- live region semantics) - Each `CommentMessage` root element should get `role="article"` (WAI-ARIA comment pattern) **Delete button aria-label:** Currently uses `m.btn_delete()` (line 270) which is a generic "Delete" label. Should be improved to include author context: `m.comment_delete_aria({ author: msg.authorName })`. This requires a new Paraglide key. **Edit textarea label:** No `aria-label` currently (line 249-254). Will add `aria-label={m.comment_edit_aria({ author: msg.authorName })}`. **Focus management:** Not currently implemented. When `editingId` changes to match `msg.id`, the textarea appears but focus is not moved. Plan: use Svelte's `{@attach}` directive on the edit textarea to call `node.focus()` on mount. When `cancelEdit()` is called, focus should return to the message container -- this needs a ref to the trigger element. **Touch targets:** The delete button at line 268 is `p-0.5` with a `h-4 w-4` SVG -- effective size is roughly 20x20px. This is below the 44x44px minimum. Will increase to `p-2` or add `min-h-[44px] min-w-[44px]`. **CSS scoping:** All styles in the message bubble use Tailwind utility classes (inline), not scoped `<style>` blocks. No parent-scoped CSS will break during extraction. The `.mention` class used by `{@html}` is defined in global `layout.css` (lines 212-226), so it is unaffected by component boundaries. The quote block styling is also all inline Tailwind (`border-l-2 border-line pl-2 font-serif text-base text-ink-3 italic` at line 243). **Hover/focus states:** The own-message paragraph has `hover:bg-surface` (line 261) for the edit-on-click affordance. This is inline Tailwind and will move cleanly with the markup. --- ### Tobias Wendt -- DevOps & File Location **Co-location:** `CommentMessage.svelte` will live in `src/lib/components/` alongside `CommentThread.svelte`. Both are general-purpose components used from multiple routes (document detail, annotation panels, transcription blocks), so `$lib/components/` is the correct location -- not a route-specific folder. **Subfolder:** A `conversations/` subfolder is premature. The compose area is already handled by `MentionEditor.svelte` and will not need further extraction (see Markus section). The component count in `$lib/components/` is manageable. Can revisit if more comment-related components emerge. **CI pipeline:** Confirmed `npm run check` and `npm run test` are both in the CI workflow. No Playwright e2e tests directly exercise the comment thread (checked `frontend/e2e/` -- no conversation-specific spec file), so no e2e regression risk. **No infrastructure impact:** Correct. No new deps, no env vars, no Docker/Flyway changes.

marcel referenced this issue from a commit

2026-04-15 14:58:02 +02:00

refactor(comments): extract CommentMessage component from CommentThread (#198)

marcel referenced this issue

2026-04-15 15:00:31 +02:00

refactor(frontend): utility dedup, component splits, dead code removal (#193–#200) #241

marcel closed this issue

2026-04-15 21:02:11 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#198

Split CommentThread.svelte (310 lines) — extract CommentMessage component #198

Context

Proposed Split

CommentMessage.svelte

CommentThread.svelte becomes simpler

Acceptance Criteria

👨‍💻 Felix Brandt -- Senior Fullstack Developer

🏗️ Markus Keller -- Senior Application Architect

🧪 Sara Holt -- Senior QA Engineer

🔒 Nora "NullX" Steiner -- Application Security Engineer

🎨 Leonie Voss -- UI/UX Design Lead

⚙️ Tobias Wendt -- DevOps & Platform Engineer

Comprehensive Response to Review Comments

Felix Brandt -- Props Interface & Quote Parsing

Markus Keller -- Module Boundary & FlatMessage Type

Sara Holt -- Test Coverage

Nora Steiner -- Security

Leonie Voss -- Accessibility & Styling

Tobias Wendt -- DevOps & File Location

Split `CommentThread.svelte` (310 lines) — extract `CommentMessage` component #198

`CommentMessage.svelte`

`CommentThread.svelte` becomes simpler