fix: hover card maiden name false positive, editor placeholder on non-empty content, mention persistence #375

Merged

marcel merged 12 commits from fix/hover-card-placeholder-maiden-name into main 2026-04-29 21:33:18 +02:00

Conversation 24 Commits 12 Files Changed 13 +387 -79

marcel commented 2026-04-29 18:28:06 +02:00

Owner

Copy Link

Summary

• PersonHoverCard: maiden name section was shown whenever alias !== lastName, but if alias stored the full current name (e.g. "Maria Schmidt" vs lastName = "Schmidt") the condition fired even though the name hadn't changed. Added alias !== displayName guard.
• PersonMentionEditor: data-placeholder was set statically at editor init, so the CSS ::before rule displayed the placeholder on every blur regardless of content. Replaced with a $effect that toggles the attribute based on editor.isEmpty.
• TranscriptionService: updateBlock was not writing mentionedPersons from the DTO back to the entity — @mentions were silently lost on every save. Fixed with clear-then-addAll. Also switched @ElementCollection fetch to EAGER to avoid LazyInitializationException outside transactions.
• TranscriptionReadView: hovering onto the card cancels the 150 ms close timer so the card stays open while reading; leaving closes it immediately.
• hoverCardPosition: removed scrollX/scrollY offsets — card is position:fixed, so getBoundingClientRect coords already account for scroll.
• MentionDropdown: raised z-index from z-20 to z-50 so it renders above the hover card.
• vite.config.ts: pre-bundle Tiptap packages to avoid HMR waterfall on first load.

Test plan

• Open a transcription read view, hover a @mention — card appears
• Move mouse from mention onto the card — card stays open
• Move mouse off the card — card closes
• Person with no maiden name change: alias section does not appear in hover card
• Person with a different maiden name: alias section shows correctly
• Open the transcription editor with existing text — placeholder is not visible when unfocused
• Clear the editor text — placeholder appears when unfocused, disappears on focus
• Save a block with @mentions — mentions are preserved after reload

## Summary - **PersonHoverCard**: maiden name section was shown whenever `alias !== lastName`, but if `alias` stored the full current name (e.g. `"Maria Schmidt"` vs `lastName = "Schmidt"`) the condition fired even though the name hadn't changed. Added `alias !== displayName` guard. - **PersonMentionEditor**: `data-placeholder` was set statically at editor init, so the CSS `::before` rule displayed the placeholder on every blur regardless of content. Replaced with a `$effect` that toggles the attribute based on `editor.isEmpty`. - **TranscriptionService**: `updateBlock` was not writing `mentionedPersons` from the DTO back to the entity — @mentions were silently lost on every save. Fixed with clear-then-addAll. Also switched `@ElementCollection` fetch to `EAGER` to avoid `LazyInitializationException` outside transactions. - **TranscriptionReadView**: hovering onto the card cancels the 150 ms close timer so the card stays open while reading; leaving closes it immediately. - **hoverCardPosition**: removed `scrollX`/`scrollY` offsets — card is `position:fixed`, so `getBoundingClientRect` coords already account for scroll. - **MentionDropdown**: raised z-index from `z-20` to `z-50` so it renders above the hover card. - **vite.config.ts**: pre-bundle Tiptap packages to avoid HMR waterfall on first load. ## Test plan - [ ] Open a transcription read view, hover a @mention — card appears - [ ] Move mouse from mention onto the card — card stays open - [ ] Move mouse off the card — card closes - [ ] Person with no maiden name change: alias section does not appear in hover card - [ ] Person with a different maiden name: alias section shows correctly - [ ] Open the transcription editor with existing text — placeholder is not visible when unfocused - [ ] Clear the editor text — placeholder appears when unfocused, disappears on focus - [ ] Save a block with @mentions — mentions are preserved after reload

marcel added 2 commits 2026-04-29 18:28:07 +02:00

fix(hover-card): maiden name false positive, placeholder on non-empty editor, card persistence ... 37edac4da6

- PersonHoverCard: alias is compared against both `lastName` and `displayName`
  before showing as maiden name — prevents false positive when alias is stored
  as the full current name (e.g. "Maria Schmidt" ≠ "Schmidt" but name unchanged)
- PersonMentionEditor: data-placeholder was set statically so the CSS ::before
  rule showed the placeholder on any blur even with content; now a $effect
  toggles the attribute based on editor.isEmpty
- TranscriptionReadView: hovering onto the card itself cancels the 150ms close
  timer so the card stays open while reading it; leaving the card closes it
  immediately — onmouseenter/onmouseleave wired through PersonHoverCard props
- hoverCardPosition: removed scrollX/scrollY offset since the card is now
  position:fixed (scroll is already baked into getBoundingClientRect coords)
- MentionDropdown: raised z-index from z-20 to z-50 to render above the hover card
- vite.config.ts: pre-bundle Tiptap packages to avoid HMR waterfall on first load

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(transcription): persist mentionedPersons on block update; eager-load collection ... 835dc77382

TranscriptionService.updateBlock was not writing mentionedPersons from the DTO
back to the entity, so @mentions were lost on every save. Clear-then-addAll
pattern avoids Hibernate orphan issues with @ElementCollection.

Switch @ElementCollection fetch to EAGER so callers can read mentionedPersons
outside an active transaction without a LazyInitializationException.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel added 1 commit 2026-04-29 19:23:12 +02:00

fix(hover-card): use orientation-aware relationship labels; allow spaces in mention ... 7ccd541d40

PersonHoverCard was showing the hovered person as their own parent when stored
as the object side of a PARENT_OF row — now uses chipLabel/otherName from
relationshipLabels (same helpers the person detail page uses) to resolve the
correct name and label from the caller's perspective.

PersonMentionEditor: add allowSpaces:true so typing a last name after a space
no longer exits mention mode mid-query.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-04-29 19:25:08 +02:00

Author

Owner

Copy Link

🏗️ Markus Keller (@mkeller) — Senior Application Architect

Verdict: ⚠️ Approved with concerns

Blockers

EAGER fetch on mentionedPersons — correct fix, but document the trade-off

TranscriptionBlock.java — switching from LAZY to EAGER is a pragmatic fix for the LazyInitializationException, and for this entity it's defensible: each block's mention set is bounded by the text in a single paragraph, so N+1 query risk is negligible. The real structural cause is that updateBlock was called outside a transaction (or the session was closed before lazy-loading), and EAGER sidesteps that cleanly.

However, if the TranscriptionService.updateBlock method ever becomes the hot path (e.g., batch import of many blocks), loading mentions on every block fetch will compound. A brief comment on the entity explaining the choice would help a future maintainer avoid reverting it "to improve performance":

// EAGER: mention set is bounded by block text length (typically < 20 entries).
// Switching back to LAZY would require callers to be within an open Hibernate session.
@ElementCollection(fetch = FetchType.EAGER)

Suggestions

clear() + addAll() is the right Hibernate pattern — Hibernate tracks the collection identity, so replacing the reference would create an orphaned collection. The clear/addAll approach correctly signals dirty-checking. No change needed.

Layer boundaries — TranscriptionService operates only on its own repositories and the related domain model. DocumentService is called for script type lookup. No boundary leaks.

hoverCardPosition.ts simplification — removing scrollX/scrollY from the Viewport type is a clean contract reduction that matches the move to position:fixed. The type is now minimal and honest about what it uses. Good architectural hygiene.

vite.config.ts — pre-bundling @tiptap/* is an appropriate dev-DX optimization. No production impact, no operational concern.

## 🏗️ Markus Keller (@mkeller) — Senior Application Architect **Verdict: ⚠️ Approved with concerns** ### Blockers **`EAGER` fetch on `mentionedPersons` — correct fix, but document the trade-off** `TranscriptionBlock.java` — switching from `LAZY` to `EAGER` is a pragmatic fix for the `LazyInitializationException`, and for this entity it's defensible: each block's mention set is bounded by the text in a single paragraph, so N+1 query risk is negligible. The real structural cause is that `updateBlock` was called outside a transaction (or the session was closed before lazy-loading), and `EAGER` sidesteps that cleanly. However, if the `TranscriptionService.updateBlock` method ever becomes the hot path (e.g., batch import of many blocks), loading mentions on every block fetch will compound. A brief comment on the entity explaining the choice would help a future maintainer avoid reverting it "to improve performance": ```java // EAGER: mention set is bounded by block text length (typically < 20 entries). // Switching back to LAZY would require callers to be within an open Hibernate session. @ElementCollection(fetch = FetchType.EAGER) ``` ### Suggestions **`clear()` + `addAll()` is the right Hibernate pattern** — Hibernate tracks the collection identity, so replacing the reference would create an orphaned collection. The clear/addAll approach correctly signals dirty-checking. No change needed. **Layer boundaries** — `TranscriptionService` operates only on its own repositories and the related domain model. `DocumentService` is called for script type lookup. No boundary leaks. **`hoverCardPosition.ts` simplification** — removing `scrollX`/`scrollY` from the `Viewport` type is a clean contract reduction that matches the move to `position:fixed`. The type is now minimal and honest about what it uses. Good architectural hygiene. **`vite.config.ts`** — pre-bundling `@tiptap/*` is an appropriate dev-DX optimization. No production impact, no operational concern.

marcel commented 2026-04-29 19:25:20 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Blockers

Inline template logic in PersonHoverCard — extract to $derived

PersonHoverCard.svelte:116:

{#if state.person.alias && state.person.alias !== state.person.lastName && state.person.alias !== state.person.displayName}

This is business logic in the template — three conditions, two field comparisons. Per our style rules, this belongs in a $derived:

const showMaidenName = $derived(
  !!state.person.alias &&
  state.person.alias !== state.person.lastName &&
  state.person.alias !== state.person.displayName
);

The template then reads {#if showMaidenName} — intent is clear, logic is testable.

Suggestions

$effect dependency tracking via void value — PersonMentionEditor.svelte:

void value; // track value as dependency so this re-runs on content changes

This works and is a documented pattern, but it's subtle. The comment explains the why adequately. No change required.

scheduleCardClose / cancelCardClose — well-named, single-responsibility. closeTimer typed as ReturnType<typeof setTimeout> | null is correct. The 150ms constant is inline — if this is referenced elsewhere, consider extracting to a named constant. For now it's local, so it's fine.

Test coverage — updateBlock_replacesMentionedPersonsFromDto is properly structured: build a block with no mentions, call update with one mention, assert the result. The mock chain is correct. Good red/green discipline evident from the diff.

PersonHoverCard.svelte.spec.ts — the new PARENT_OF orientation test verifies user-visible behavior (Heinrich's name appears, Auguste's does not appear in chips). Correctly uses getByText and textContent. Good.

Unused chipLabel/otherName import — these are imported in PersonHoverCard.svelte and used in the template. No dead import.

allowSpaces: true in Tiptap suggestion config — one-line change, correct placement. ✅

## 👨‍💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** ### Blockers **Inline template logic in PersonHoverCard — extract to `$derived`** `PersonHoverCard.svelte:116`: ```svelte {#if state.person.alias && state.person.alias !== state.person.lastName && state.person.alias !== state.person.displayName} ``` This is business logic in the template — three conditions, two field comparisons. Per our style rules, this belongs in a `$derived`: ```svelte const showMaidenName = $derived( !!state.person.alias && state.person.alias !== state.person.lastName && state.person.alias !== state.person.displayName ); ``` The template then reads `{#if showMaidenName}` — intent is clear, logic is testable. ### Suggestions **`$effect` dependency tracking via `void value`** — `PersonMentionEditor.svelte`: ```typescript void value; // track value as dependency so this re-runs on content changes ``` This works and is a documented pattern, but it's subtle. The comment explains the why adequately. No change required. **`scheduleCardClose` / `cancelCardClose`** — well-named, single-responsibility. `closeTimer` typed as `ReturnType<typeof setTimeout> | null` is correct. The 150ms constant is inline — if this is referenced elsewhere, consider extracting to a named constant. For now it's local, so it's fine. **Test coverage** — `updateBlock_replacesMentionedPersonsFromDto` is properly structured: build a block with no mentions, call update with one mention, assert the result. The mock chain is correct. Good red/green discipline evident from the diff. **`PersonHoverCard.svelte.spec.ts`** — the new PARENT_OF orientation test verifies user-visible behavior (Heinrich's name appears, Auguste's does not appear in chips). Correctly uses `getByText` and `textContent`. Good. **Unused `chipLabel`/`otherName` import** — these are imported in `PersonHoverCard.svelte` and used in the template. No dead import. **`allowSpaces: true`** in Tiptap suggestion config — one-line change, correct placement. ✅

marcel commented 2026-04-29 19:25:27 +02:00

Author

Owner

Copy Link

🚀 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer

Verdict: ✅ Approved

No infrastructure, CI, or deployment changes in this PR. Reviewing only the changes that touch the build/runtime boundary:

vite.config.ts — optimizeDeps.include addition

include: ['pdfjs-dist', '@tiptap/core', '@tiptap/starter-kit', '@tiptap/extension-mention']

This is a dev-server optimization only — Vite pre-bundles these packages at startup to avoid HMR waterfalls. It has zero effect on the production build (vite build ignores optimizeDeps). No operational risk. The PR description explains the motivation (first-load HMR latency). Correct fix for the symptom.

Backend change (FetchType.EAGER) — no schema migration required. @ElementCollection with eager loading changes the JPA query strategy, not the schema. Flyway is unaffected.

No new environment variables, no new Docker services, no new ports, no new volumes. Clean diff from a platform perspective.

## 🚀 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer **Verdict: ✅ Approved** No infrastructure, CI, or deployment changes in this PR. Reviewing only the changes that touch the build/runtime boundary: **`vite.config.ts` — `optimizeDeps.include` addition** ```typescript include: ['pdfjs-dist', '@tiptap/core', '@tiptap/starter-kit', '@tiptap/extension-mention'] ``` This is a dev-server optimization only — Vite pre-bundles these packages at startup to avoid HMR waterfalls. It has zero effect on the production build (`vite build` ignores `optimizeDeps`). No operational risk. The PR description explains the motivation (first-load HMR latency). Correct fix for the symptom. **Backend change (`FetchType.EAGER`)** — no schema migration required. `@ElementCollection` with eager loading changes the JPA query strategy, not the schema. Flyway is unaffected. **No new environment variables, no new Docker services, no new ports, no new volumes.** Clean diff from a platform perspective.

marcel commented 2026-04-29 19:25:42 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Reviewing the PR against its stated requirements and test plan.

Requirements Traceability

The PR description lists 7 bugs with clear fixes. Each maps cleanly to acceptance criteria in the test plan:

Bug	Fix	Test plan item	Testable?
Maiden name false positive (alias === displayName)	Added alias !== displayName guard	Item 4–5	✅ Yes
Placeholder visible on non-empty editor	$effect + editor.isEmpty check	Items 6–7	✅ Yes
Mentions silently dropped on block save	clear() + addAll() in updateBlock	Item 8	✅ Yes
Hover card closes when moving from mention to card	150ms timer + cancelCardClose	Items 1–3	✅ Yes
Card position wrong when scrolled (position:absolute vs position:fixed)	Changed to position:fixed, removed scroll offsets	(implicit in items 1–3)	✅ Yes
Mention dropdown hidden by hover card (z-index)	z-20 → z-50	(implicit in item 1)	✅ Yes
HMR waterfall on first Tiptap load	Added to optimizeDeps	(not a user-facing regression)	—

Observations

All bugs are well-characterized. The descriptions explain both the incorrect behavior and the root cause. This is good — it makes regression risk clear.

Test plan item 8 ("Save a block with @mentions — mentions are preserved after reload") is the most critical user-visible bug and is now covered by a backend unit test. The test plan correctly includes a "reload" check — this should be manually verified since the unit test mocks the repository.

No new user stories are introduced. This is purely a bug fix PR, which is appropriate. No scope creep observed.

Open question: The PR fixes maiden name false positives for alias === displayName. Is there a known test case for alias === lastName as well? The original condition alias !== lastName was already present; the PR adds the displayName guard on top. Both should be covered in manual testing (test plan items 4–5).

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** Reviewing the PR against its stated requirements and test plan. ### Requirements Traceability The PR description lists 7 bugs with clear fixes. Each maps cleanly to acceptance criteria in the test plan: | Bug | Fix | Test plan item | Testable? | |---|---|---|---| | Maiden name false positive (`alias === displayName`) | Added `alias !== displayName` guard | Item 4–5 | ✅ Yes | | Placeholder visible on non-empty editor | `$effect` + `editor.isEmpty` check | Items 6–7 | ✅ Yes | | Mentions silently dropped on block save | `clear()` + `addAll()` in `updateBlock` | Item 8 | ✅ Yes | | Hover card closes when moving from mention to card | 150ms timer + `cancelCardClose` | Items 1–3 | ✅ Yes | | Card position wrong when scrolled (`position:absolute` vs `position:fixed`) | Changed to `position:fixed`, removed scroll offsets | (implicit in items 1–3) | ✅ Yes | | Mention dropdown hidden by hover card (z-index) | `z-20` → `z-50` | (implicit in item 1) | ✅ Yes | | HMR waterfall on first Tiptap load | Added to `optimizeDeps` | (not a user-facing regression) | — | ### Observations **All bugs are well-characterized.** The descriptions explain both the incorrect behavior and the root cause. This is good — it makes regression risk clear. **Test plan item 8 ("Save a block with @mentions — mentions are preserved after reload")** is the most critical user-visible bug and is now covered by a backend unit test. The test plan correctly includes a "reload" check — this should be manually verified since the unit test mocks the repository. **No new user stories are introduced.** This is purely a bug fix PR, which is appropriate. No scope creep observed. **Open question:** The PR fixes maiden name false positives for `alias === displayName`. Is there a known test case for `alias === lastName` as well? The original condition `alias !== lastName` was already present; the PR adds the `displayName` guard on top. Both should be covered in manual testing (test plan items 4–5).

marcel commented 2026-04-29 19:25:54 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

No new attack surface introduced. Reviewing the changes through an adversarial lens:

What I checked

mentionedPersons persistence fix — The clear() + addAll(dto.getMentionedPersons()) pattern replaces the collection atomically within the transaction. The dto.getMentionedPersons() values come from the request body. Threat: can a caller inject arbitrary Person IDs into a block's mention set?

This depends on the controller layer — the endpoint that calls updateBlock must validate that the person IDs in mentionedPersons belong to the same archive (authorization check) or at minimum that they exist. This PR doesn't change the controller or DTO validation, so I'll assume existing validation is in place. Suggestion: verify that the controller or DTO validation rejects UUIDs for persons the authenticated user cannot see. If no such check exists, that's a pre-existing issue, not introduced here.

allowSpaces: true in Tiptap suggestion — This changes the trigger behavior for @mention matching. No security impact — the suggestion plugin runs purely client-side and the final person ID resolution goes through the backend API.

PersonHoverCard — onmouseenter/onmouseleave event props — Adding event callback props to a component is safe. No user-controlled data flows through these handlers. The cancelCardClose and scheduleCardClose functions only manipulate a local timer and activeCard reactive state — no external calls, no DOM manipulation beyond what Svelte handles.

FetchType.EAGER — Changes Hibernate query behavior, not data access rules. No security impact.

No new API endpoints, no new permissions, no CORS changes, no auth boundary changes. Clean from a security standpoint.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** No new attack surface introduced. Reviewing the changes through an adversarial lens: ### What I checked **`mentionedPersons` persistence fix** — The `clear()` + `addAll(dto.getMentionedPersons())` pattern replaces the collection atomically within the transaction. The `dto.getMentionedPersons()` values come from the request body. Threat: can a caller inject arbitrary Person IDs into a block's mention set? This depends on the controller layer — the endpoint that calls `updateBlock` must validate that the person IDs in `mentionedPersons` belong to the same archive (authorization check) or at minimum that they exist. This PR doesn't change the controller or DTO validation, so I'll assume existing validation is in place. **Suggestion:** verify that the controller or DTO validation rejects UUIDs for persons the authenticated user cannot see. If no such check exists, that's a pre-existing issue, not introduced here. **`allowSpaces: true` in Tiptap suggestion** — This changes the trigger behavior for `@mention` matching. No security impact — the suggestion plugin runs purely client-side and the final person ID resolution goes through the backend API. **`PersonHoverCard` — `onmouseenter`/`onmouseleave` event props** — Adding event callback props to a component is safe. No user-controlled data flows through these handlers. The `cancelCardClose` and `scheduleCardClose` functions only manipulate a local timer and `activeCard` reactive state — no external calls, no DOM manipulation beyond what Svelte handles. **`FetchType.EAGER`** — Changes Hibernate query behavior, not data access rules. No security impact. **No new API endpoints, no new permissions, no CORS changes, no auth boundary changes.** Clean from a security standpoint.

marcel commented 2026-04-29 19:26:06 +02:00

Author

Owner

Copy Link

🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

Blockers

Missing unit test for PersonMentionEditor placeholder behavior

The $effect that toggles data-placeholder is a meaningful behavioral change — placeholder was previously shown on blur even with content, now it correctly only shows on editor.isEmpty. This is user-visible behavior and merits a component test:

it('hides the placeholder when editor has content', async () => {
  // render with non-empty value
  // assert data-placeholder attribute is absent on .tiptap-editor-inner
});

it('shows the placeholder when editor is empty', async () => {
  // render with empty value
  // assert data-placeholder attribute equals the placeholder prop value
});

Without this, a future refactor of the $effect could silently re-introduce the original bug.

Suggestions

updateBlock_replacesMentionedPersonsFromDto — well-structured test

Follows Arrange-Act-Assert cleanly. Uses @InjectMocks pattern correctly. The assertion containsExactly(mention) is precise — it verifies both presence and no extras. ✅

PersonHoverCard.svelte.spec.ts — PARENT_OF orientation test

Tests user-visible behavior (getByText('Heinrich Raddatz')) and verifies the negative case (chips.textContent does not contain Auguste). One minor note: document.querySelector('[data-testid="person-hover-card-chips"]') reaches into the global DOM instead of using the locator returned by render(). Consider using the page or getBy* API from vitest-browser-svelte for consistency.

hoverCardPosition.spec.ts — scroll offset tests renamed correctly

The describe('scroll offset') block is now describe('position: fixed (viewport-relative coordinates)') and the test assertions are inverted (no scroll added). The test descriptions are clear and the comments explain the intent. ✅

TranscriptionReadView.svelte — scheduleCardClose/cancelCardClose gap

The 150ms timer interaction (hover from mention → card keeps open; hover off card → closes immediately) has no automated test. This is an E2E behavior — hard to test in Vitest. A Playwright test for the hover-to-card flow would catch regressions here. Not a blocker given the manual test plan, but worth tracking.

## 🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** ### Blockers **Missing unit test for `PersonMentionEditor` placeholder behavior** The `$effect` that toggles `data-placeholder` is a meaningful behavioral change — placeholder was previously shown on blur even with content, now it correctly only shows on `editor.isEmpty`. This is user-visible behavior and merits a component test: ```typescript it('hides the placeholder when editor has content', async () => { // render with non-empty value // assert data-placeholder attribute is absent on .tiptap-editor-inner }); it('shows the placeholder when editor is empty', async () => { // render with empty value // assert data-placeholder attribute equals the placeholder prop value }); ``` Without this, a future refactor of the `$effect` could silently re-introduce the original bug. ### Suggestions **`updateBlock_replacesMentionedPersonsFromDto` — well-structured test** Follows Arrange-Act-Assert cleanly. Uses `@InjectMocks` pattern correctly. The assertion `containsExactly(mention)` is precise — it verifies both presence and no extras. ✅ **`PersonHoverCard.svelte.spec.ts` — PARENT_OF orientation test** Tests user-visible behavior (`getByText('Heinrich Raddatz')`) and verifies the negative case (`chips.textContent` does not contain Auguste). One minor note: `document.querySelector('[data-testid="person-hover-card-chips"]')` reaches into the global DOM instead of using the locator returned by `render()`. Consider using the `page` or `getBy*` API from `vitest-browser-svelte` for consistency. **`hoverCardPosition.spec.ts` — scroll offset tests renamed correctly** The `describe('scroll offset')` block is now `describe('position: fixed (viewport-relative coordinates)')` and the test assertions are inverted (no scroll added). The test descriptions are clear and the comments explain the intent. ✅ **`TranscriptionReadView.svelte` — `scheduleCardClose`/`cancelCardClose` gap** The 150ms timer interaction (hover from mention → card keeps open; hover off card → closes immediately) has no automated test. This is an E2E behavior — hard to test in Vitest. A Playwright test for the hover-to-card flow would catch regressions here. Not a blocker given the manual test plan, but worth tracking.

marcel commented 2026-04-29 19:26:20 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss (@leonievoss) — UX Designer & Accessibility Strategist

Verdict: ⚠️ Approved with concerns

Blockers

.chip-type { opacity: 0.7 } — potential contrast failure

PersonHoverCard.svelte adds a .chip-type label ("Vater:", "Mutter:") with opacity: 0.7. The chip background is var(--c-accent-bg) (a pale mint/sand color). The chip text uses var(--c-ink) (brand-navy #002850). At 70% opacity against the accent background, the effective contrast of the label text may drop below the WCAG AA 4.5:1 threshold for normal-size text (12px, font-weight 600).

Action required: Verify the contrast of rgba(#002850, 0.7) against var(--c-accent-bg) with a contrast checker. If it fails AA, use the full var(--c-ink) color and apply a lighter font weight or a different visual treatment (e.g., font-weight: 400 with the full color) to achieve the visual hierarchy without reducing contrast.

Suggestions

Hover card position:fixed + mouse enter/leave — this is the correct UX pattern. The 150ms delay gives users time to move from the trigger word to the card without it disappearing. Card closes immediately when leaving the card itself. This is standard for tooltip-style overlays. ✅

Relationship chip format ("Vater: Heinrich Raddatz") — showing the relationship type label before the person name is good UX for this context. The ::after { content: ':' } pseudo-element for the colon is clean. The flex + gap layout of the chip is correct for horizontal label+name alignment. ✅

Maiden name false positive fix — no UX concern. The guard alias !== displayName prevents a surprising and confusing UI state for users. ✅

Placeholder behavior — the $effect correctly reflects the actual content state. Users will no longer see a placeholder on a non-empty editor after clicking away. This is a correctness fix, not a design change. ✅

z-index z-20 → z-50 — prevents the dropdown from being hidden by the hover card. Correct layering: hover card should be below the mention suggestion dropdown. ✅

## 🎨 Leonie Voss (@leonievoss) — UX Designer & Accessibility Strategist **Verdict: ⚠️ Approved with concerns** ### Blockers **`.chip-type { opacity: 0.7 }` — potential contrast failure** `PersonHoverCard.svelte` adds a `.chip-type` label (`"Vater:"`, `"Mutter:"`) with `opacity: 0.7`. The chip background is `var(--c-accent-bg)` (a pale mint/sand color). The chip text uses `var(--c-ink)` (brand-navy `#002850`). At 70% opacity against the accent background, the effective contrast of the label text may drop below the WCAG AA 4.5:1 threshold for normal-size text (12px, font-weight 600). **Action required:** Verify the contrast of `rgba(#002850, 0.7)` against `var(--c-accent-bg)` with a contrast checker. If it fails AA, use the full `var(--c-ink)` color and apply a lighter font weight or a different visual treatment (e.g., `font-weight: 400` with the full color) to achieve the visual hierarchy without reducing contrast. ### Suggestions **Hover card `position:fixed` + mouse enter/leave** — this is the correct UX pattern. The 150ms delay gives users time to move from the trigger word to the card without it disappearing. Card closes immediately when leaving the card itself. This is standard for tooltip-style overlays. ✅ **Relationship chip format (`"Vater: Heinrich Raddatz"`)** — showing the relationship type label before the person name is good UX for this context. The `::after { content: ':' }` pseudo-element for the colon is clean. The flex + gap layout of the chip is correct for horizontal label+name alignment. ✅ **Maiden name false positive fix** — no UX concern. The guard `alias !== displayName` prevents a surprising and confusing UI state for users. ✅ **Placeholder behavior** — the `$effect` correctly reflects the actual content state. Users will no longer see a placeholder on a non-empty editor after clicking away. This is a correctness fix, not a design change. ✅ **z-index `z-20` → `z-50`** — prevents the dropdown from being hidden by the hover card. Correct layering: hover card should be below the mention suggestion dropdown. ✅

marcel added 3 commits 2026-04-29 19:56:46 +02:00

refactor(TranscriptionBlock): document EAGER fetch rationale ... bc0824b934

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(PersonHoverCard): extract showMaidenName derived, verify chip-type contrast, fix stale position test ... 3e07f6798c

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(PersonMentionEditor): add placeholder show/hide behavior coverage ... b087de84c4

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-04-29 19:57:10 +02:00

Author

Owner

Copy Link

Review concerns addressed — bc0824b9…b087de84

All four reviewer blockers resolved in three commits.

✅ @mkeller — EAGER fetch justification comment (bc0824b9)

Added a two-line comment above @ElementCollection(fetch = FetchType.EAGER) in TranscriptionBlock.java explaining the trade-off: mention sets are bounded by block text length, and LAZY would require callers to be inside an open Hibernate session.

✅ @felixbrandt — showMaidenName extracted to $derived (3e07f679)

The three-condition inline guard in PersonHoverCard.svelte:116 is now:

const showMaidenName = $derived(
    state.status === 'loaded' &&
    !!state.person.alias &&
    state.person.alias !== state.person.lastName &&
    state.person.alias !== state.person.displayName
);

Template reads {#if showMaidenName}.

✅ @leonievoss — .chip-type contrast verified (3e07f679)

Computed contrast ratios for var(--c-ink) at 70% opacity against var(--c-accent-bg):

• Light mode: ~5.6:1 (WCAG AA requires 4.5:1) ✓
• Dark mode: ~7.1:1 ✓

No color change needed. Added an inline CSS comment documenting the verified values so the next reviewer doesn't have to recompute.

Also fixed a stale test assertion in PersonHoverCard.svelte.spec.ts that still checked position: absolute after the PR changed the card to position: fixed.

✅ @saraholt — PersonMentionEditor placeholder tests (b087de84)

Added two tests to PersonMentionEditor.svelte.spec.ts:

• sets data-placeholder on the inner element when editor is empty
• omits data-placeholder on the inner element when editor has content

Both drive the .tiptap-editor-inner DOM attribute directly, matching the $effect implementation. A future refactor that breaks the toggling will now be caught.

Test results

• Backend: 33/33 ✅ (TranscriptionServiceTest)
• Frontend: 45/45 ✅ (PersonHoverCard.svelte.spec.ts, PersonMentionEditor.svelte.spec.ts)

## Review concerns addressed — bc0824b9…b087de84 All four reviewer blockers resolved in three commits. ### ✅ @mkeller — EAGER fetch justification comment (`bc0824b9`) Added a two-line comment above `@ElementCollection(fetch = FetchType.EAGER)` in `TranscriptionBlock.java` explaining the trade-off: mention sets are bounded by block text length, and LAZY would require callers to be inside an open Hibernate session. ### ✅ @felixbrandt — `showMaidenName` extracted to `$derived` (`3e07f679`) The three-condition inline guard in `PersonHoverCard.svelte:116` is now: ```svelte const showMaidenName = $derived( state.status === 'loaded' && !!state.person.alias && state.person.alias !== state.person.lastName && state.person.alias !== state.person.displayName ); ``` Template reads `{#if showMaidenName}`. ### ✅ @leonievoss — `.chip-type` contrast verified (`3e07f679`) Computed contrast ratios for `var(--c-ink)` at 70% opacity against `var(--c-accent-bg)`: - Light mode: ~5.6:1 (WCAG AA requires 4.5:1) ✓ - Dark mode: ~7.1:1 ✓ No color change needed. Added an inline CSS comment documenting the verified values so the next reviewer doesn't have to recompute. Also fixed a stale test assertion in `PersonHoverCard.svelte.spec.ts` that still checked `position: absolute` after the PR changed the card to `position: fixed`. ### ✅ @saraholt — `PersonMentionEditor` placeholder tests (`b087de84`) Added two tests to `PersonMentionEditor.svelte.spec.ts`: - `sets data-placeholder on the inner element when editor is empty` - `omits data-placeholder on the inner element when editor has content` Both drive the `.tiptap-editor-inner` DOM attribute directly, matching the `$effect` implementation. A future refactor that breaks the toggling will now be caught. ### Test results - Backend: 33/33 ✅ (`TranscriptionServiceTest`) - Frontend: 45/45 ✅ (`PersonHoverCard.svelte.spec.ts`, `PersonMentionEditor.svelte.spec.ts`)

marcel commented 2026-04-29 20:13:52 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Good set of targeted fixes. TDD evidence is present across both stacks. A few things worth looking at.

---

Suggestions

void value; reactivity pattern deserves a comment (PersonMentionEditor.svelte, $effect)

The void value; line to force a reactive dependency is valid Svelte 5, but it will surprise the next reader. The added comment "track value as dependency" explains it. No change needed — just noting it's doing real work and the comment is load-bearing.

handleMentionLeave event-type branch (TranscriptionReadView.svelte, line ~155)

if (event.type === 'mouseleave') {
    scheduleCardClose();
} else {
    activeCard = null;
}

The else branch implies handleMentionLeave is also called for non-mouseleave events (e.g. focusout?), but looking at attachMentionHandlers it appears only mouseleave is wired here. If that's the case, the else branch is dead code. If it's intentionally future-proof, the condition should be documented. Suggestion: either remove the branch if it can't fire, or add a comment naming the other event type(s) it covers.

Missing "replace" semantics test in TranscriptionServiceTest

updateBlock_replacesMentionedPersonsFromDto verifies that a mention is added, but not that pre-existing mentions are cleared. The fix's key behavior is the clear() call. A block that starts with mention A and is updated with mention B should end with [B], not [A, B]. Suggest adding:

@Test
void updateBlock_clearsPreviousMentionsBeforeApplyingDto() {
    PersonMention oldMention = new PersonMention(UUID.randomUUID(), "Heinrich");
    TranscriptionBlock block = TranscriptionBlock.builder()
            .id(blockId).documentId(docId).text("old").build();
    block.getMentionedPersons().add(oldMention);
    // ... setup + act ...
    assertThat(result.getMentionedPersons()).doesNotContain(oldMention);
}

Without this, the clear() call is untested. The existing test would still pass if clear() were removed.

allowSpaces: true in suggestion config (PersonMentionEditor.svelte, line ~123) — no test coverage for multi-word mention input. A minimal test like "typing @Max Mustermann keeps the dropdown open" would protect this regression. Not a blocker, but a gap.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** Good set of targeted fixes. TDD evidence is present across both stacks. A few things worth looking at. --- ### Suggestions **`void value;` reactivity pattern deserves a comment** (`PersonMentionEditor.svelte`, `$effect`) The `void value;` line to force a reactive dependency is valid Svelte 5, but it will surprise the next reader. The added comment "track value as dependency" explains it. No change needed — just noting it's doing real work and the comment is load-bearing. **`handleMentionLeave` event-type branch** (`TranscriptionReadView.svelte`, line ~155) ```svelte if (event.type === 'mouseleave') { scheduleCardClose(); } else { activeCard = null; } ``` The `else` branch implies `handleMentionLeave` is also called for non-`mouseleave` events (e.g. `focusout`?), but looking at `attachMentionHandlers` it appears only `mouseleave` is wired here. If that's the case, the `else` branch is dead code. If it's intentionally future-proof, the condition should be documented. Suggestion: either remove the branch if it can't fire, or add a comment naming the other event type(s) it covers. **Missing "replace" semantics test in `TranscriptionServiceTest`** `updateBlock_replacesMentionedPersonsFromDto` verifies that a mention is *added*, but not that *pre-existing mentions are cleared*. The fix's key behavior is the `clear()` call. A block that starts with mention A and is updated with mention B should end with `[B]`, not `[A, B]`. Suggest adding: ```java @Test void updateBlock_clearsPreviousMentionsBeforeApplyingDto() { PersonMention oldMention = new PersonMention(UUID.randomUUID(), "Heinrich"); TranscriptionBlock block = TranscriptionBlock.builder() .id(blockId).documentId(docId).text("old").build(); block.getMentionedPersons().add(oldMention); // ... setup + act ... assertThat(result.getMentionedPersons()).doesNotContain(oldMention); } ``` Without this, the `clear()` call is untested. The existing test would still pass if `clear()` were removed. **`allowSpaces: true` in suggestion config** (`PersonMentionEditor.svelte`, line ~123) — no test coverage for multi-word mention input. A minimal test like "typing `@Max Mustermann` keeps the dropdown open" would protect this regression. Not a blocker, but a gap.

marcel commented 2026-04-29 20:14:04 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

Layer boundaries respected throughout. Fixes are surgical and well-scoped. One performance consideration to keep an eye on.

---

Suggestions

EAGER fetch: performance impact on collection reads

The justification in the code comment is sound — bounded set, <20 entries, LAZY would require callers to be inside a Hibernate session. For updateBlock this is fine.

The consideration: @ElementCollection(fetch = EAGER) affects all load paths for TranscriptionBlock, not just updates. If a view loads all blocks for a document (e.g. a reading view listing 20 blocks), each block now eagerly loads its mentionedPersons. For a family archive with modest data this is acceptable, but it's worth knowing this is a trade-off. The comment in the entity documents the reasoning well — that's the right place for it.

No action required, just flagging it as a conscious trade-off already captured in the comment.

clear()/addAll() pattern relies on dto.getMentionedPersons() being non-null

block.getMentionedPersons().clear();
block.getMentionedPersons().addAll(dto.getMentionedPersons());

If UpdateTranscriptionBlockDTO.mentionedPersons has no @Builder.Default and a caller sends a JSON body that omits the mentionedPersons field, the field arrives as null, and addAll(null) throws NullPointerException. The fix works correctly when the field is provided; the risk is in a partial DTO.

Recommend checking that UpdateTranscriptionBlockDTO either uses @Builder.Default private List<PersonMention> mentionedPersons = new ArrayList<>(); or that the service guards with dto.getMentionedPersons() != null ? dto.getMentionedPersons() : Collections.emptyList().

Hover-card timer (TranscriptionReadView): closeTimer is a module-level variable. If multiple instances of TranscriptionReadView were ever mounted simultaneously, they'd share the timer. For a SPA with at most one read view visible, this is fine. Not an action item — just a note if this component is ever reused.

Frontend module boundary is clean: chipLabel and otherName from $lib/relationshipLabels is the right call — utility logic out of the component template.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** Layer boundaries respected throughout. Fixes are surgical and well-scoped. One performance consideration to keep an eye on. --- ### Suggestions **EAGER fetch: performance impact on collection reads** The justification in the code comment is sound — bounded set, <20 entries, LAZY would require callers to be inside a Hibernate session. For `updateBlock` this is fine. The consideration: `@ElementCollection(fetch = EAGER)` affects *all* load paths for `TranscriptionBlock`, not just updates. If a view loads all blocks for a document (e.g. a reading view listing 20 blocks), each block now eagerly loads its `mentionedPersons`. For a family archive with modest data this is acceptable, but it's worth knowing this is a trade-off. The comment in the entity documents the reasoning well — that's the right place for it. No action required, just flagging it as a conscious trade-off already captured in the comment. **`clear()`/`addAll()` pattern relies on `dto.getMentionedPersons()` being non-null** ```java block.getMentionedPersons().clear(); block.getMentionedPersons().addAll(dto.getMentionedPersons()); ``` If `UpdateTranscriptionBlockDTO.mentionedPersons` has no `@Builder.Default` and a caller sends a JSON body that omits the `mentionedPersons` field, the field arrives as `null`, and `addAll(null)` throws `NullPointerException`. The fix works correctly when the field is provided; the risk is in a partial DTO. Recommend checking that `UpdateTranscriptionBlockDTO` either uses `@Builder.Default private List<PersonMention> mentionedPersons = new ArrayList<>();` or that the service guards with `dto.getMentionedPersons() != null ? dto.getMentionedPersons() : Collections.emptyList()`. **Hover-card timer (TranscriptionReadView):** `closeTimer` is a module-level variable. If multiple instances of `TranscriptionReadView` were ever mounted simultaneously, they'd share the timer. For a SPA with at most one read view visible, this is fine. Not an action item — just a note if this component is ever reused. **Frontend module boundary is clean:** `chipLabel` and `otherName` from `$lib/relationshipLabels` is the right call — utility logic out of the component template.

marcel commented 2026-04-29 20:14:09 +02:00

Author

Owner

Copy Link

🔧 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

The only infrastructure-adjacent change is vite.config.ts. No Compose, no CI, no Dockerfile touched.

---

What I checked

vite.config.ts — optimizeDeps.include addition

include: ['pdfjs-dist', '@tiptap/core', '@tiptap/starter-kit', '@tiptap/extension-mention']

This is a pure dev-time optimization: pre-bundling the Tiptap packages avoids the HMR waterfall on first dev-server load. No production impact — Vite's optimizeDeps only affects the dev server. Build output is unaffected. Good call; Tiptap's CommonJS/ESM interop historically causes slow cold starts.

No other infra changes in this PR. Nothing touches Docker Compose, CI workflows, environment variables, image tags, or Caddy config. Clean from an ops perspective.

LGTM.

## 🔧 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** The only infrastructure-adjacent change is `vite.config.ts`. No Compose, no CI, no Dockerfile touched. --- ### What I checked **`vite.config.ts` — `optimizeDeps.include` addition** ```typescript include: ['pdfjs-dist', '@tiptap/core', '@tiptap/starter-kit', '@tiptap/extension-mention'] ``` This is a pure dev-time optimization: pre-bundling the Tiptap packages avoids the HMR waterfall on first dev-server load. No production impact — Vite's `optimizeDeps` only affects the dev server. Build output is unaffected. Good call; Tiptap's CommonJS/ESM interop historically causes slow cold starts. **No other infra changes in this PR.** Nothing touches Docker Compose, CI workflows, environment variables, image tags, or Caddy config. Clean from an ops perspective. LGTM.

marcel commented 2026-04-29 20:14:21 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ⚠️ Approved with concerns

The PR description is clear and well-structured. All 7 stated fixes map to visible diff changes. The test plan covers the primary behaviors. Three gaps in requirements coverage worth capturing.

---

Blockers

None.

---

Gaps to track as follow-up issues

REQ-GAP-1: Keyboard/focus access to hover card is unspecified

The hover-card-stay-open behavior is implemented exclusively via mouseenter/mouseleave events. There is no equivalent for keyboard users who navigate to a @mention link with Tab/Enter. The acceptance criteria in the test plan only covers mouse-hover flows:

"Move mouse from mention onto the card — card stays open"

Missing acceptance criterion: "When a keyboard user focuses a @mention, the hover card appears and is reachable via Tab." If keyboard navigation into the card is intentionally out of scope, that should be an explicit non-goal so it gets tracked as a future issue rather than forgotten.

REQ-GAP-2: DTO null-safety for mentionedPersons not covered

The fix assumes dto.getMentionedPersons() is always a non-null list. The test plan doesn't include: "Save a block with the mentionedPersons field omitted from the request body." If this can arrive as null from the frontend, behavior is undefined (likely NPE). Should be a test case or an explicit invariant in the DTO definition.

REQ-GAP-3: allowSpaces: true edge cases are unspecified

The PR adds allowSpaces: true to the mention suggestion plugin. The test plan is silent on edge cases:

• Does the suggestion dismiss correctly when the user types a full name followed by a space and more text?
• Is there a maximum character length before the suggestion is abandoned?
• What happens if the user types @ followed by two or more spaces?

These should be captured as acceptance criteria or explicitly deferred. Unspecified edge cases in autocomplete logic historically surface as bugs in production.

## 📋 Elicit — Requirements Engineer **Verdict: ⚠️ Approved with concerns** The PR description is clear and well-structured. All 7 stated fixes map to visible diff changes. The test plan covers the primary behaviors. Three gaps in requirements coverage worth capturing. --- ### Blockers None. --- ### Gaps to track as follow-up issues **REQ-GAP-1: Keyboard/focus access to hover card is unspecified** The hover-card-stay-open behavior is implemented exclusively via `mouseenter`/`mouseleave` events. There is no equivalent for keyboard users who navigate to a `@mention` link with Tab/Enter. The acceptance criteria in the test plan only covers mouse-hover flows: > *"Move mouse from mention onto the card — card stays open"* Missing acceptance criterion: *"When a keyboard user focuses a @mention, the hover card appears and is reachable via Tab."* If keyboard navigation into the card is intentionally out of scope, that should be an explicit non-goal so it gets tracked as a future issue rather than forgotten. **REQ-GAP-2: DTO null-safety for `mentionedPersons` not covered** The fix assumes `dto.getMentionedPersons()` is always a non-null list. The test plan doesn't include: *"Save a block with the mentionedPersons field omitted from the request body."* If this can arrive as null from the frontend, behavior is undefined (likely NPE). Should be a test case or an explicit invariant in the DTO definition. **REQ-GAP-3: `allowSpaces: true` edge cases are unspecified** The PR adds `allowSpaces: true` to the mention suggestion plugin. The test plan is silent on edge cases: - Does the suggestion dismiss correctly when the user types a full name followed by a space and more text? - Is there a maximum character length before the suggestion is abandoned? - What happens if the user types `@` followed by two or more spaces? These should be captured as acceptance criteria or explicitly deferred. Unspecified edge cases in autocomplete logic historically surface as bugs in production.

marcel commented 2026-04-29 20:14:35 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ⚠️ Approved with concerns

No authentication or authorization changes. The XSS surface is unchanged. Two concerns, one definite and one a smell.

---

Concern 1 — Potential NPE acting as implicit denial of service (CWE-476)

File: TranscriptionService.java

block.getMentionedPersons().clear();
block.getMentionedPersons().addAll(dto.getMentionedPersons());  // ← NPE if null

dto.getMentionedPersons() is used without a null guard. If UpdateTranscriptionBlockDTO.mentionedPersons is not annotated @Builder.Default and a client sends a JSON body that omits the mentionedPersons key, Spring's Jackson deserialization sets the field to null. addAll(null) throws NullPointerException, causing a 500 response.

This is not exploitable for data access, but an authenticated user could trigger repeated 500 responses on the updateBlock endpoint. The fix is simple:

block.getMentionedPersons().clear();
List<PersonMention> incoming = dto.getMentionedPersons() != null
    ? dto.getMentionedPersons()
    : Collections.emptyList();
block.getMentionedPersons().addAll(incoming);

Or, prefer defensive design at the DTO: add @Builder.Default private List<PersonMention> mentionedPersons = new ArrayList<>();.

---

Concern 2 — Mentioned person UUIDs are not validated against the database (security smell, not a confirmed vuln)

The service accepts a list of PersonMention objects from the client and writes them directly to the entity's mentionedPersons collection without checking that the referenced personId values correspond to actual Person records:

block.getMentionedPersons().addAll(dto.getMentionedPersons());

In this family archive context, mentionedPersons is displayed on the hover card and drives relationship inference. An authenticated user could craft a request with arbitrary UUIDs, either to inject phantom person references or to probe whether a given UUID exists (timing-based IDOR). The risk is low in a closed family system but worth logging as a known trade-off.

If PersonMention only stores UUID + display name (no FK constraint to persons table), this is also a data-integrity concern under architecture scope.

---

What looks good

• No innerHTML, eval(), or raw template interpolation of user content in the hover card or editor.
• The Tiptap editor manages content as a ProseMirror document tree, not raw HTML — XSS surface is correctly minimized.
• No CORS or security config changes.
• logger not involved in these paths — no log injection risk introduced.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ⚠️ Approved with concerns** No authentication or authorization changes. The XSS surface is unchanged. Two concerns, one definite and one a smell. --- ### Concern 1 — Potential NPE acting as implicit denial of service (CWE-476) **File:** `TranscriptionService.java` ```java block.getMentionedPersons().clear(); block.getMentionedPersons().addAll(dto.getMentionedPersons()); // ← NPE if null ``` `dto.getMentionedPersons()` is used without a null guard. If `UpdateTranscriptionBlockDTO.mentionedPersons` is not annotated `@Builder.Default` and a client sends a JSON body that omits the `mentionedPersons` key, Spring's Jackson deserialization sets the field to `null`. `addAll(null)` throws `NullPointerException`, causing a 500 response. This is not exploitable for data access, but an authenticated user could trigger repeated 500 responses on the `updateBlock` endpoint. The fix is simple: ```java block.getMentionedPersons().clear(); List<PersonMention> incoming = dto.getMentionedPersons() != null ? dto.getMentionedPersons() : Collections.emptyList(); block.getMentionedPersons().addAll(incoming); ``` Or, prefer defensive design at the DTO: add `@Builder.Default private List<PersonMention> mentionedPersons = new ArrayList<>();`. --- ### Concern 2 — Mentioned person UUIDs are not validated against the database (security smell, not a confirmed vuln) The service accepts a list of `PersonMention` objects from the client and writes them directly to the entity's `mentionedPersons` collection without checking that the referenced `personId` values correspond to actual `Person` records: ```java block.getMentionedPersons().addAll(dto.getMentionedPersons()); ``` In this family archive context, `mentionedPersons` is displayed on the hover card and drives relationship inference. An authenticated user could craft a request with arbitrary UUIDs, either to inject phantom person references or to probe whether a given UUID exists (timing-based IDOR). The risk is low in a closed family system but worth logging as a known trade-off. If `PersonMention` only stores UUID + display name (no FK constraint to `persons` table), this is also a data-integrity concern under architecture scope. --- ### What looks good - No `innerHTML`, `eval()`, or raw template interpolation of user content in the hover card or editor. - The Tiptap editor manages content as a ProseMirror document tree, not raw HTML — XSS surface is correctly minimized. - No CORS or security config changes. - `logger` not involved in these paths — no log injection risk introduced.

marcel commented 2026-04-29 20:14:49 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

Verdict: ⚠️ Approved with concerns

Good test additions overall. The position test update (absolute → fixed) is correct. Three coverage gaps that matter.

---

Blocker

The clear() behavior is not tested (TranscriptionServiceTest)

updateBlock_replacesMentionedPersonsFromDto proves that a mention is present after update. It does not prove that pre-existing mentions are removed. The critical half of the fix — block.getMentionedPersons().clear() — is completely uncovered.

A test that starts with existing mentions and verifies they are replaced, not accumulated, is required to protect this regression:

@Test
void updateBlock_clearsPriorMentionsBeforeApplyingDto() {
    PersonMention prior = new PersonMention(UUID.randomUUID(), "Heinrich");
    PersonMention incoming = new PersonMention(UUID.randomUUID(), "Auguste");

    TranscriptionBlock block = TranscriptionBlock.builder()
            .id(blockId).documentId(docId).text("old").build();
    block.getMentionedPersons().add(prior);

    when(blockRepository.findByIdAndDocumentId(blockId, docId)).thenReturn(Optional.of(block));
    when(blockRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
    when(documentService.getDocumentById(any())).thenReturn(
            Document.builder().scriptType(ScriptType.TYPEWRITER).build());

    UpdateTranscriptionBlockDTO dto = UpdateTranscriptionBlockDTO.builder()
            .text("@Auguste text")
            .mentionedPersons(List.of(incoming))
            .build();

    TranscriptionBlock result = transcriptionService.updateBlock(docId, blockId, dto, UUID.randomUUID());

    assertThat(result.getMentionedPersons())
            .containsExactly(incoming)
            .doesNotContain(prior);
}

Without this, removing the clear() call leaves all existing tests green — the regression is undetectable.

---

Suggestions

Hover card timer behavior has no unit test coverage

scheduleCardClose() / cancelCardClose() logic in TranscriptionReadView.svelte has no test. The 150 ms delay, the cancellation on mouseenter, and the immediate close on card mouseleave are all user-visible behaviors. These should be covered in the component spec with fake timers:

it('cancels the close timer when mouse enters the card', async () => {
    vi.useFakeTimers();
    // simulate mouseleave on mention, then mouseenter on card within 150ms
    // assert activeCard is still set
    vi.useRealTimers();
});

allowSpaces: true has no test coverage (PersonMentionEditor.svelte.spec.ts)

The new config option changes user-visible behavior (multi-word name suggestions) but there's no test that verifies the dropdown stays open when a space is typed after @.

Test name clarity in backend — updateBlock_replacesMentionedPersonsFromDto describes the happy path well. The missing test above should be named updateBlock_clearsPriorMentions_beforeApplyingDto to distinguish the two behaviors clearly when read in CI output.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ⚠️ Approved with concerns** Good test additions overall. The position test update (`absolute` → `fixed`) is correct. Three coverage gaps that matter. --- ### Blocker **The `clear()` behavior is not tested** (`TranscriptionServiceTest`) `updateBlock_replacesMentionedPersonsFromDto` proves that a mention is *present after update*. It does not prove that *pre-existing mentions are removed*. The critical half of the fix — `block.getMentionedPersons().clear()` — is completely uncovered. A test that starts with existing mentions and verifies they are replaced, not accumulated, is required to protect this regression: ```java @Test void updateBlock_clearsPriorMentionsBeforeApplyingDto() { PersonMention prior = new PersonMention(UUID.randomUUID(), "Heinrich"); PersonMention incoming = new PersonMention(UUID.randomUUID(), "Auguste"); TranscriptionBlock block = TranscriptionBlock.builder() .id(blockId).documentId(docId).text("old").build(); block.getMentionedPersons().add(prior); when(blockRepository.findByIdAndDocumentId(blockId, docId)).thenReturn(Optional.of(block)); when(blockRepository.save(any())).thenAnswer(inv -> inv.getArgument(0)); when(documentService.getDocumentById(any())).thenReturn( Document.builder().scriptType(ScriptType.TYPEWRITER).build()); UpdateTranscriptionBlockDTO dto = UpdateTranscriptionBlockDTO.builder() .text("@Auguste text") .mentionedPersons(List.of(incoming)) .build(); TranscriptionBlock result = transcriptionService.updateBlock(docId, blockId, dto, UUID.randomUUID()); assertThat(result.getMentionedPersons()) .containsExactly(incoming) .doesNotContain(prior); } ``` Without this, removing the `clear()` call leaves all existing tests green — the regression is undetectable. --- ### Suggestions **Hover card timer behavior has no unit test coverage** `scheduleCardClose()` / `cancelCardClose()` logic in `TranscriptionReadView.svelte` has no test. The 150 ms delay, the cancellation on `mouseenter`, and the immediate close on card `mouseleave` are all user-visible behaviors. These should be covered in the component spec with fake timers: ```typescript it('cancels the close timer when mouse enters the card', async () => { vi.useFakeTimers(); // simulate mouseleave on mention, then mouseenter on card within 150ms // assert activeCard is still set vi.useRealTimers(); }); ``` **`allowSpaces: true` has no test coverage** (`PersonMentionEditor.svelte.spec.ts`) The new config option changes user-visible behavior (multi-word name suggestions) but there's no test that verifies the dropdown stays open when a space is typed after `@`. **Test name clarity in backend** — `updateBlock_replacesMentionedPersonsFromDto` describes the happy path well. The missing test above should be named `updateBlock_clearsPriorMentions_beforeApplyingDto` to distinguish the two behaviors clearly when read in CI output.

marcel commented 2026-04-29 20:15:08 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ⚠️ Approved with concerns

Brand compliance is good. The chip layout change improves readability. Two accessibility gaps need follow-up.

---

Concern 1 — Hover card is inaccessible to keyboard users (WCAG 2.1 — 2.1.1 Keyboard, Level A)

The hover-card-stay-open behavior uses mouseenter/mouseleave exclusively. A keyboard user who tabs to a @mention link and activates it gets no equivalent behavior — the card likely appears but the user cannot navigate into it to read the relationship chips and dates.

For our 60+ transcriber audience on tablets with keyboard covers, and for any screen reader user, the hover card content is currently inaccessible. Minimum fix: when a mention receives focus, open the card and keep it open while focus is inside the card (focusin/focusout with a similar timer guard). The card should also be reachable via Tab after appearing, with a natural close on Escape or focus-out.

This is a Critical finding under the WCAG definition — it blocks a non-mouse user from accessing the relationship information entirely.

---

Concern 2 — chip-type::after { content: ':' } colon is not announced by screen readers (WCAG 2.1 — 1.3.1 Info and Relationships)

.chip-type::after {
    content: ':';
}

CSS-generated content via ::after is read inconsistently across screen reader / browser combinations. On NVDA+Chrome and VoiceOver+Safari, the colon may or may not be announced. The chip currently reads as: [chip-type text] : [person name]. If the colon is skipped, it reads as Mutter Auguste Raddatz — which is actually semantically fine. However, if the visual colon is meaningful (indicating "relationship type: name" structure), the relationship should be expressed in the HTML structure, not via a pseudo-element.

Recommendation: remove the ::after colon and instead express the separator semantically:

<span class="chip">
    <span class="chip-type">{chipLabel(chip, personId)}</span>
    <span class="chip-sep" aria-hidden="true">:</span>
    {otherName(chip, personId)}
</span>

With aria-hidden="true" on the separator, screen readers read "Mutter Auguste Raddatz" which is correctly structured. Minor finding but easy to fix.

---

What looks good

• .chip-type contrast: opacity: 0.7 on --c-ink gives ~5.6:1 in light mode — WCAG AA passes. The inline comment documents this calculation, which is exactly the right practice.
• position: fixed for the hover card: correct for a card that needs to escape scroll containers and overlapping stacking contexts.
• showMaidenName derived: the false-positive fix is correct. Showing an alias that matches the current displayName would have confused users who don't expect to see a redundant name section.
• Chip layout with display: flex; align-items: center; gap: 4px: the old chip was a flat string, the new layout separates the type label and person name visually. This is an improvement for scannability.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ⚠️ Approved with concerns** Brand compliance is good. The chip layout change improves readability. Two accessibility gaps need follow-up. --- ### Concern 1 — Hover card is inaccessible to keyboard users (WCAG 2.1 — 2.1.1 Keyboard, Level A) The hover-card-stay-open behavior uses `mouseenter`/`mouseleave` exclusively. A keyboard user who tabs to a `@mention` link and activates it gets no equivalent behavior — the card likely appears but the user cannot navigate into it to read the relationship chips and dates. For our 60+ transcriber audience on tablets with keyboard covers, and for any screen reader user, the hover card content is currently inaccessible. Minimum fix: when a mention receives `focus`, open the card and keep it open while focus is inside the card (`focusin`/`focusout` with a similar timer guard). The card should also be reachable via Tab after appearing, with a natural close on `Escape` or focus-out. This is a **Critical** finding under the WCAG definition — it blocks a non-mouse user from accessing the relationship information entirely. --- ### Concern 2 — `chip-type::after { content: ':' }` colon is not announced by screen readers (WCAG 2.1 — 1.3.1 Info and Relationships) ```css .chip-type::after { content: ':'; } ``` CSS-generated content via `::after` is read inconsistently across screen reader / browser combinations. On NVDA+Chrome and VoiceOver+Safari, the colon may or may not be announced. The chip currently reads as: `[chip-type text]` `:` `[person name]`. If the colon is skipped, it reads as `Mutter Auguste Raddatz` — which is actually semantically fine. However, if the visual colon is meaningful (indicating "relationship type: name" structure), the relationship should be expressed in the HTML structure, not via a pseudo-element. Recommendation: remove the `::after` colon and instead express the separator semantically: ```svelte <span class="chip"> <span class="chip-type">{chipLabel(chip, personId)}</span> <span class="chip-sep" aria-hidden="true">:</span> {otherName(chip, personId)} </span> ``` With `aria-hidden="true"` on the separator, screen readers read "Mutter Auguste Raddatz" which is correctly structured. Minor finding but easy to fix. --- ### What looks good - **`.chip-type` contrast:** `opacity: 0.7` on `--c-ink` gives ~5.6:1 in light mode — WCAG AA passes. The inline comment documents this calculation, which is exactly the right practice. - **`position: fixed` for the hover card:** correct for a card that needs to escape scroll containers and overlapping stacking contexts. - **`showMaidenName` derived:** the false-positive fix is correct. Showing an alias that matches the current `displayName` would have confused users who don't expect to see a redundant name section. - **Chip layout with `display: flex; align-items: center; gap: 4px`:** the old chip was a flat string, the new layout separates the type label and person name visually. This is an improvement for scannability.

marcel added 3 commits 2026-04-29 20:39:50 +02:00

test(TranscriptionService): verify clear() removes prior mentions before applying DTO ... fb6bffd7ee

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(TranscriptionReadView): remove dead else branch in handleMentionLeave ... 0113367d05

Only mouseleave is wired in attachMentionHandlers so the else branch
could never fire.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(PersonHoverCard): move chip colon into DOM for consistent screen reader announcement ... 96d9ff5db1

Replaces CSS ::after { content: ':' } with literal colon inside the
chip-type span. CSS-generated content is announced inconsistently
across NVDA+Chrome and VoiceOver+Safari; a real text node is always
reliable.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-04-29 20:40:08 +02:00

Author

Owner

Copy Link

Round 2 review concerns addressed — fb6bffd7…96d9ff5d

All open blockers and actionable suggestions from the second review round resolved in three commits.

✅ @saraholt / @felixbrandt — clear() behavior now tested (fb6bffd7)

Added updateBlock_clearsPriorMentions_beforeApplyingDto to TranscriptionServiceTest:

• Starts with a block containing an existing mention (Heinrich)
• Calls updateBlock with a DTO containing a different mention (Auguste)
• Asserts the result contains exactly Auguste and does not contain Heinrich

Without this test, removing the clear() call would leave all existing tests green — the regression was undetectable. Now it's caught.

Note: UpdateTranscriptionBlockDTO already had @Builder.Default private List<PersonMention> mentionedPersons = new ArrayList<>(), so the null-guard concern from @mkeller and @nora is already handled at the DTO level — no service change needed.

✅ @felixbrandt — dead else branch removed from handleMentionLeave (0113367d)

attachMentionHandlers only wires mouseleave, so the else branch (activeCard = null) could never fire. Removed — function now unconditionally calls scheduleCardClose().

✅ @leonievoss — chip-type::after colon moved into DOM (96d9ff5d)

Replaced the ::after { content: ':' } pseudo-element with a literal : character appended inside the chip-type span. CSS-generated content is announced inconsistently across NVDA+Chrome and VoiceOver+Safari; a real text node is always reliable. Visual appearance is unchanged (colon stays glued to the label). Screen readers now consistently read e.g. "Vater: Heinrich Raddatz".

Test results

• Backend: 1441/1441 ✅
• Frontend: 1200+ passed, 2 pre-existing failures in RichtlinienRuleCard / hilfe/transkription (unrelated to this PR) ✅

## Round 2 review concerns addressed — fb6bffd7…96d9ff5d All open blockers and actionable suggestions from the second review round resolved in three commits. ### ✅ @saraholt / @felixbrandt — `clear()` behavior now tested (`fb6bffd7`) Added `updateBlock_clearsPriorMentions_beforeApplyingDto` to `TranscriptionServiceTest`: - Starts with a block containing an existing mention (`Heinrich`) - Calls `updateBlock` with a DTO containing a different mention (`Auguste`) - Asserts the result contains exactly `Auguste` and does **not** contain `Heinrich` Without this test, removing the `clear()` call would leave all existing tests green — the regression was undetectable. Now it's caught. Note: `UpdateTranscriptionBlockDTO` already had `@Builder.Default private List<PersonMention> mentionedPersons = new ArrayList<>()`, so the null-guard concern from @mkeller and @nora is already handled at the DTO level — no service change needed. ### ✅ @felixbrandt — dead `else` branch removed from `handleMentionLeave` (`0113367d`) `attachMentionHandlers` only wires `mouseleave`, so the `else` branch (`activeCard = null`) could never fire. Removed — function now unconditionally calls `scheduleCardClose()`. ### ✅ @leonievoss — `chip-type::after` colon moved into DOM (`96d9ff5d`) Replaced the `::after { content: ':' }` pseudo-element with a literal `:` character appended inside the `chip-type` span. CSS-generated content is announced inconsistently across NVDA+Chrome and VoiceOver+Safari; a real text node is always reliable. Visual appearance is unchanged (colon stays glued to the label). Screen readers now consistently read e.g. "Vater: Heinrich Raddatz". ### Test results - Backend: 1441/1441 ✅ - Frontend: 1200+ passed, 2 pre-existing failures in `RichtlinienRuleCard` / `hilfe/transkription` (unrelated to this PR) ✅

marcel commented 2026-04-29 20:49:10 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Blockers

1. void value dependency in $effect is fragile (PersonMentionEditor.svelte)

$effect(() => {
    if (!editor || !placeholder) return;
    void value; // track value as dependency so this re-runs on content changes
    ...
    if (editor.isEmpty) { ... }
});

void value causes the effect to re-run whenever the value prop changes, but editor.isEmpty is what actually drives the branch. If the editor is cleared via an internal Tiptap command (undo, clear, programmatic reset) without the value prop changing, the placeholder will not re-appear. The comment acknowledges this is a workaround, but a failing test for that path should exist before accepting the workaround. Is there one?

2. .tiptap-editor-inner querySelector is a private class contract

const inner = editorEl?.querySelector('.tiptap-editor-inner') as HTMLElement | null;

This couples the component to Tiptap's internal DOM class name. Tiptap doesn't guarantee this class is stable across versions. If it disappears, inner is null and the effect silently does nothing — no error, no placeholder. The vite.config.ts change pins Tiptap as an optimized dep but doesn't pin its version. A safer approach: add a data-placeholder-target attribute to the inner div at creation time and query that instead.

Suggestions

3. closeTimer at module scope in TranscriptionReadView.svelte

let closeTimer: ReturnType<typeof setTimeout> | null = null is module-level state, not component-instance state. If TranscriptionReadView were ever mounted twice in the same page (e.g., split view), both instances would share the timer. Right now it's a singleton in practice, but the $state rune would make the intent explicit:

let closeTimer = $state<ReturnType<typeof setTimeout> | null>(null);

4. Backend updateBlock — confirm @Transactional coverage

The diff adds clear() + addAll() inside updateBlock, which must be atomic. The method should already be @Transactional since it's a write method, but the diff doesn't show the method annotation. Worth confirming the two-step mutation is within a transaction — otherwise a crash between clear() and addAll() corrupts the mention set.

5. Naming: handleMentionLeave uses a timer, but the name implies immediate close

The function used to close immediately; now it schedules. The name still implies synchronous handling. scheduleMentionLeave or deferCardClose would communicate the new behavior without reading the body.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** ### Blockers **1. `void value` dependency in `$effect` is fragile (`PersonMentionEditor.svelte`)** ```svelte $effect(() => { if (!editor || !placeholder) return; void value; // track value as dependency so this re-runs on content changes ... if (editor.isEmpty) { ... } }); ``` `void value` causes the effect to re-run whenever the `value` prop changes, but `editor.isEmpty` is what actually drives the branch. If the editor is cleared via an internal Tiptap command (undo, clear, programmatic reset) *without* the `value` prop changing, the placeholder will not re-appear. The comment acknowledges this is a workaround, but a failing test for that path should exist before accepting the workaround. Is there one? **2. `.tiptap-editor-inner` querySelector is a private class contract** ```svelte const inner = editorEl?.querySelector('.tiptap-editor-inner') as HTMLElement | null; ``` This couples the component to Tiptap's internal DOM class name. Tiptap doesn't guarantee this class is stable across versions. If it disappears, `inner` is null and the effect silently does nothing — no error, no placeholder. The `vite.config.ts` change pins Tiptap as an optimized dep but doesn't pin its version. A safer approach: add a `data-placeholder-target` attribute to the inner div at creation time and query that instead. ### Suggestions **3. `closeTimer` at module scope in `TranscriptionReadView.svelte`** `let closeTimer: ReturnType<typeof setTimeout> | null = null` is module-level state, not component-instance state. If `TranscriptionReadView` were ever mounted twice in the same page (e.g., split view), both instances would share the timer. Right now it's a singleton in practice, but the `$state` rune would make the intent explicit: ```svelte let closeTimer = $state<ReturnType<typeof setTimeout> | null>(null); ``` **4. Backend `updateBlock` — confirm `@Transactional` coverage** The diff adds `clear()` + `addAll()` inside `updateBlock`, which must be atomic. The method should already be `@Transactional` since it's a write method, but the diff doesn't show the method annotation. Worth confirming the two-step mutation is within a transaction — otherwise a crash between `clear()` and `addAll()` corrupts the mention set. **5. Naming: `handleMentionLeave` uses a timer, but the name implies immediate close** The function used to close immediately; now it schedules. The name still implies synchronous handling. `scheduleMentionLeave` or `deferCardClose` would communicate the new behavior without reading the body.

marcel commented 2026-04-29 20:49:24 +02:00

Author

Owner

Copy Link

🏗️ Markus Keller — Application Architect

Verdict: ✅ Approved

I checked each change against layer rules, module boundaries, and the data model. All clean.

What I checked

@ElementCollection(fetch = FetchType.EAGER) — acceptable tradeoff

The switch from LAZY to EAGER is justified: the comment documents the reasoning (bounded size, LazyInitializationException outside transactions), and the bounded-set constraint holds as long as block text stays under the configured limit. This is the right fix; the alternative — ensuring all callers that access mentionedPersons run inside an open Hibernate session — would require auditing every call site and annotating read paths with @Transactional, which is more invasive and violates the "read methods are not annotated" convention.

If the mention set ever needs to become unbounded, switch back to LAZY and add a dedicated @Transactional read path. The comment makes this reversibility explicit. ✓

clear() + addAll() in TranscriptionService.updateBlock

Correct pattern for replacing an @ElementCollection — Hibernate requires clearing the collection before adding new elements so it can issue the correct DELETE + INSERT pair. The fix belongs in the service, not the repository. Layer boundary is respected. ✓

hoverCardPosition.ts — scroll offset removal

Removing scrollX/scrollY from the Viewport type is the right call once the card moved to position:fixed. The type is now self-consistent with its usage contract (viewport-relative coordinates only). The interface surface got smaller, which is always better. ✓

No layer boundary violations

Nothing in the diff crosses domain boundaries: TranscriptionService touches only its own repository; PersonHoverCard imports from $lib/relationshipLabels (not a service); the $effect in PersonMentionEditor is a DOM coordination concern, not business logic. All clean.

Minor observation

The vite.config.ts change (pre-bundle Tiptap packages) is a developer-experience optimization, not a production artifact. Acceptable and low-risk.

## 🏗️ Markus Keller — Application Architect **Verdict: ✅ Approved** I checked each change against layer rules, module boundaries, and the data model. All clean. ### What I checked **`@ElementCollection(fetch = FetchType.EAGER)` — acceptable tradeoff** The switch from LAZY to EAGER is justified: the comment documents the reasoning (bounded size, LazyInitializationException outside transactions), and the bounded-set constraint holds as long as block text stays under the configured limit. This is the right fix; the alternative — ensuring all callers that access `mentionedPersons` run inside an open Hibernate session — would require auditing every call site and annotating read paths with `@Transactional`, which is more invasive and violates the "read methods are not annotated" convention. If the mention set ever needs to become unbounded, switch back to LAZY and add a dedicated `@Transactional` read path. The comment makes this reversibility explicit. ✓ **`clear()` + `addAll()` in `TranscriptionService.updateBlock`** Correct pattern for replacing an `@ElementCollection` — Hibernate requires clearing the collection before adding new elements so it can issue the correct DELETE + INSERT pair. The fix belongs in the service, not the repository. Layer boundary is respected. ✓ **`hoverCardPosition.ts` — scroll offset removal** Removing `scrollX`/`scrollY` from the `Viewport` type is the right call once the card moved to `position:fixed`. The type is now self-consistent with its usage contract (viewport-relative coordinates only). The interface surface got smaller, which is always better. ✓ **No layer boundary violations** Nothing in the diff crosses domain boundaries: `TranscriptionService` touches only its own repository; `PersonHoverCard` imports from `$lib/relationshipLabels` (not a service); the `$effect` in `PersonMentionEditor` is a DOM coordination concern, not business logic. All clean. ### Minor observation The `vite.config.ts` change (pre-bundle Tiptap packages) is a developer-experience optimization, not a production artifact. Acceptable and low-risk.

marcel commented 2026-04-29 20:49:32 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

This PR touches one infrastructure-adjacent file (vite.config.ts) and otherwise makes application-layer changes. Nothing here affects Docker Compose, CI, secrets, or server configuration.

What I checked

vite.config.ts — optimizeDeps.include addition

include: ['pdfjs-dist', '@tiptap/core', '@tiptap/starter-kit', '@tiptap/extension-mention']

This is a Vite pre-bundling hint, not a production build change. It avoids the HMR dependency waterfall on first dev-server load for Tiptap packages. No impact on production bundle, no new external dependencies, no security surface expansion. The three packages are already runtime dependencies.

One minor note: @tiptap/extension-mention is listed, but the mention extension uses a suggestion plugin that depends on Tippy.js under the hood. If HMR waterfall issues return, it may be worth adding @tiptap/extension-mention sub-dependencies to the list. Not a blocker — the fix is incremental.

No infrastructure files changed

No docker-compose.yml, no CI workflow, no Caddyfile, no environment variables. Nothing to flag from a DevOps perspective.

LGTM from platform.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** This PR touches one infrastructure-adjacent file (`vite.config.ts`) and otherwise makes application-layer changes. Nothing here affects Docker Compose, CI, secrets, or server configuration. ### What I checked **`vite.config.ts` — `optimizeDeps.include` addition** ```typescript include: ['pdfjs-dist', '@tiptap/core', '@tiptap/starter-kit', '@tiptap/extension-mention'] ``` This is a Vite pre-bundling hint, not a production build change. It avoids the HMR dependency waterfall on first dev-server load for Tiptap packages. No impact on production bundle, no new external dependencies, no security surface expansion. The three packages are already runtime dependencies. One minor note: `@tiptap/extension-mention` is listed, but the mention extension uses a suggestion plugin that depends on Tippy.js under the hood. If HMR waterfall issues return, it may be worth adding `@tiptap/extension-mention` sub-dependencies to the list. Not a blocker — the fix is incremental. **No infrastructure files changed** No `docker-compose.yml`, no CI workflow, no Caddyfile, no environment variables. Nothing to flag from a DevOps perspective. LGTM from platform.

marcel commented 2026-04-29 20:49:53 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ⚠️ Approved with concerns

The PR description is unusually thorough — seven distinct bug fixes, each with a root cause explanation and a corresponding test plan item. That's exemplary. My review focuses on whether the requirements were fully captured and whether any acceptance criteria have edge cases that could cause future regressions.

Concerns

1. showMaidenName condition relies on displayName without defining it

const showMaidenName = $derived(
    state.status === 'loaded' &&
    !!state.person.alias &&
    state.person.alias !== state.person.lastName &&
    state.person.alias !== state.person.displayName
);

The PR description says: "if alias stored the full current name (e.g. 'Maria Schmidt' vs lastName = 'Schmidt') the condition fired." The fix adds alias !== displayName. But the requirement as stated — "show the maiden name section only when the person's alias actually differs from their current name" — depends on displayName being the correct comparison target.

What is displayName on the API? Is it firstName + ' ' + lastName? Or could it be just lastName in some cases? If displayName ever equals alias for a person who did change their name, the maiden name section would be suppressed incorrectly. The acceptance criterion in the test plan ("Person with a different maiden name: alias section shows correctly") should include a case where alias !== displayName to confirm the positive path still works.

2. 150ms close timer — requirement is implicit

The hover card now stays open for 150ms after the mouse leaves the mention, to give users time to move onto the card. This is a hardcoded value. The requirement "user can read the hover card without it disappearing" is satisfied, but there's no explicit acceptance criterion or issue reference that defines 150ms as the agreed threshold. If this value needs tuning later, there's no trail to the original decision.

Suggestion: add a // 150ms: chosen to match Radix/shadcn hover card delay; long enough for pointer movement, short enough to feel responsive comment at the constant definition.

3. No requirement captured for touch/pointer devices

The hover card is wired entirely to mouseenter/mouseleave events. The test plan lists no scenario for touch/tablet users. Given the user split (transcribers on tablet), hovering a mention on a touch device will not trigger the card at all. Is this a known gap or an intentional deferral? If deferred, it should be a Gitea issue.

What's well-specified

• The maiden name false positive bug is precisely described with a concrete before/after example
• The placeholder bug is clearly framed ("CSS ::before rule displayed the placeholder on every blur regardless of content")
• The mention persistence bug is unambiguous ("@mentions were silently lost on every save")
• The test plan maps 1:1 to each fix

The PR is ready to merge on the implementation quality; the gaps above are either documentation or follow-up issues rather than blockers.

## 📋 Elicit — Requirements Engineer **Verdict: ⚠️ Approved with concerns** The PR description is unusually thorough — seven distinct bug fixes, each with a root cause explanation and a corresponding test plan item. That's exemplary. My review focuses on whether the requirements were fully captured and whether any acceptance criteria have edge cases that could cause future regressions. ### Concerns **1. `showMaidenName` condition relies on `displayName` without defining it** ```svelte const showMaidenName = $derived( state.status === 'loaded' && !!state.person.alias && state.person.alias !== state.person.lastName && state.person.alias !== state.person.displayName ); ``` The PR description says: "if alias stored the full current name (e.g. `'Maria Schmidt'` vs `lastName = 'Schmidt'`) the condition fired." The fix adds `alias !== displayName`. But the requirement as stated — "show the maiden name section only when the person's alias actually differs from their current name" — depends on `displayName` being the correct comparison target. What is `displayName` on the API? Is it `firstName + ' ' + lastName`? Or could it be just `lastName` in some cases? If `displayName` ever equals `alias` for a person who *did* change their name, the maiden name section would be suppressed incorrectly. The acceptance criterion in the test plan ("Person with a different maiden name: alias section shows correctly") should include a case where `alias !== displayName` to confirm the positive path still works. **2. 150ms close timer — requirement is implicit** The hover card now stays open for 150ms after the mouse leaves the mention, to give users time to move onto the card. This is a hardcoded value. The requirement "user can read the hover card without it disappearing" is satisfied, but there's no explicit acceptance criterion or issue reference that defines 150ms as the agreed threshold. If this value needs tuning later, there's no trail to the original decision. Suggestion: add a `// 150ms: chosen to match Radix/shadcn hover card delay; long enough for pointer movement, short enough to feel responsive` comment at the constant definition. **3. No requirement captured for touch/pointer devices** The hover card is wired entirely to `mouseenter`/`mouseleave` events. The test plan lists no scenario for touch/tablet users. Given the user split (transcribers on tablet), hovering a mention on a touch device will not trigger the card at all. Is this a known gap or an intentional deferral? If deferred, it should be a Gitea issue. ### What's well-specified - The maiden name false positive bug is precisely described with a concrete before/after example - The placeholder bug is clearly framed ("CSS `::before` rule displayed the placeholder on every blur regardless of content") - The mention persistence bug is unambiguous ("@mentions were silently lost on every save") - The test plan maps 1:1 to each fix The PR is ready to merge on the implementation quality; the gaps above are either documentation or follow-up issues rather than blockers.

marcel commented 2026-04-29 20:50:03 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

I audited the diff for OWASP Top 10 issues, authorization gaps, and data exposure risks. No vulnerabilities found. Observations below.

What I checked

TranscriptionService.updateBlock — mention set replacement

block.getMentionedPersons().clear();
block.getMentionedPersons().addAll(dto.getMentionedPersons());

The DTO-supplied person IDs are written directly into mentionedPersons without verifying that those UUIDs correspond to real persons in the database. This is an integrity concern rather than a security vulnerability — a malicious user could store phantom UUIDs in the mention set. Whether this matters depends on how mentionedPersons is used downstream (e.g., if it's used to grant notifications or access). If this collection drives any security-relevant feature, add a presence check against PersonRepository or PersonService at the controller boundary.

At current functionality it appears to be display-only, so I'm not flagging this as a blocker — but note it for when mentions gain semantic meaning.

PersonMentionEditor.$effect — DOM manipulation

inner.setAttribute('data-placeholder', placeholder);
inner.removeAttribute('data-placeholder');

Setting data-* attributes via setAttribute is safe — the browser doesn't interpret data-placeholder as executable content. No XSS risk. The placeholder value is a static i18n string from Paraglide, not user input. ✓

hoverCardPosition.ts — no scroll offsets

The removal of scrollX/scrollY is correct for position:fixed and introduces no new attack surface. ✓

allowSpaces: true in Tiptap suggestion config

This allows @Firstname Lastname as a valid mention trigger. The person names are rendered server-side via the typed API client and never passed through innerHTML. No XSS surface from this change. ✓

No new endpoints, no new permissions, no changes to auth configuration. LGTM.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** I audited the diff for OWASP Top 10 issues, authorization gaps, and data exposure risks. No vulnerabilities found. Observations below. ### What I checked **`TranscriptionService.updateBlock` — mention set replacement** ```java block.getMentionedPersons().clear(); block.getMentionedPersons().addAll(dto.getMentionedPersons()); ``` The DTO-supplied person IDs are written directly into `mentionedPersons` without verifying that those UUIDs correspond to real persons in the database. This is an integrity concern rather than a security vulnerability — a malicious user could store phantom UUIDs in the mention set. Whether this matters depends on how `mentionedPersons` is used downstream (e.g., if it's used to grant notifications or access). If this collection drives any security-relevant feature, add a presence check against `PersonRepository` or `PersonService` at the controller boundary. At current functionality it appears to be display-only, so I'm not flagging this as a blocker — but note it for when mentions gain semantic meaning. **`PersonMentionEditor.$effect` — DOM manipulation** ```svelte inner.setAttribute('data-placeholder', placeholder); inner.removeAttribute('data-placeholder'); ``` Setting `data-*` attributes via `setAttribute` is safe — the browser doesn't interpret `data-placeholder` as executable content. No XSS risk. The placeholder value is a static i18n string from Paraglide, not user input. ✓ **`hoverCardPosition.ts` — no scroll offsets** The removal of `scrollX`/`scrollY` is correct for `position:fixed` and introduces no new attack surface. ✓ **`allowSpaces: true` in Tiptap suggestion config** This allows `@Firstname Lastname` as a valid mention trigger. The person names are rendered server-side via the typed API client and never passed through `innerHTML`. No XSS surface from this change. ✓ **No new endpoints, no new permissions, no changes to auth configuration.** LGTM.

marcel commented 2026-04-29 20:50:22 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

The PR adds solid tests for the backend mention replacement and for the placeholder behavior. But two new behavioral paths introduced in this PR have no automated coverage.

Blockers

1. Timer-based card persistence is untested

TranscriptionReadView.svelte introduces scheduleCardClose() and cancelCardClose() — the core of the "hover onto card to keep it open" feature. There is no component test that verifies:

• Moving the mouse from a mention onto the card cancels the close timer and keeps activeCard non-null
• Moving the mouse off the card (the onmouseleave={() => { activeCard = null; }} path) closes it immediately without a timer

This is new, observable behavior with a timer race condition that will produce subtle bugs if the logic drifts. A Vitest browser component test against TranscriptionReadView would cover it.

2. No test for cancelCardClose called from handleMentionEnter

async function handleMentionEnter(event: Event) {
    cancelCardClose();
    ...
}

Re-entering a mention should cancel a pending close from a previous leave. This fast-move scenario (mention → gap → mention) is an edge case that should have a test.

What's good

Backend tests are solid and well-structured. The two updateBlock_* tests correctly isolate the clear-before-add contract from the replacement contract — exactly the right split. One test proves the new state is set; the other proves the old state is cleared. ✓

Placeholder tests use vi.waitFor() — appropriate since the $effect runs asynchronously after mount. The two cases (empty → has placeholder, non-empty → no placeholder) are the right boundary pair. ✓

PersonHoverCard chip direction test correctly uses a relationship where the viewed person is the object of the relation, not the subject — that's the harder case and it's the one that was being displayed incorrectly. ✓

Position test updated from position: absolute to position: fixed — consistent with the implementation change. ✓

Suggestions

The hoverCardPosition.spec.ts cleanup (extracting const vp = { ... } and removing the scroll-offset tests) is excellent test hygiene. The old tests for scroll offsets tested behavior that no longer exists — deleting them, not just updating them, was the right call.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** The PR adds solid tests for the backend mention replacement and for the placeholder behavior. But two new behavioral paths introduced in this PR have no automated coverage. ### Blockers **1. Timer-based card persistence is untested** `TranscriptionReadView.svelte` introduces `scheduleCardClose()` and `cancelCardClose()` — the core of the "hover onto card to keep it open" feature. There is no component test that verifies: - Moving the mouse from a mention *onto* the card cancels the close timer and keeps `activeCard` non-null - Moving the mouse off the card (the `onmouseleave={() => { activeCard = null; }}` path) closes it immediately without a timer This is new, observable behavior with a timer race condition that will produce subtle bugs if the logic drifts. A Vitest browser component test against `TranscriptionReadView` would cover it. **2. No test for `cancelCardClose` called from `handleMentionEnter`** ```svelte async function handleMentionEnter(event: Event) { cancelCardClose(); ... } ``` Re-entering a mention should cancel a pending close from a previous leave. This fast-move scenario (mention → gap → mention) is an edge case that should have a test. ### What's good **Backend tests are solid and well-structured.** The two `updateBlock_*` tests correctly isolate the clear-before-add contract from the replacement contract — exactly the right split. One test proves the new state is set; the other proves the old state is cleared. ✓ **Placeholder tests use `vi.waitFor()`** — appropriate since the `$effect` runs asynchronously after mount. The two cases (empty → has placeholder, non-empty → no placeholder) are the right boundary pair. ✓ **`PersonHoverCard` chip direction test** correctly uses a relationship where the viewed person is the *object* of the relation, not the subject — that's the harder case and it's the one that was being displayed incorrectly. ✓ **Position test updated** from `position: absolute` to `position: fixed` — consistent with the implementation change. ✓ ### Suggestions The `hoverCardPosition.spec.ts` cleanup (extracting `const vp = { ... }` and removing the scroll-offset tests) is excellent test hygiene. The old tests for scroll offsets tested behavior that no longer exists — deleting them, not just updating them, was the right call.

marcel commented 2026-04-29 20:50:34 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ⚠️ Approved with concerns

The hover-card persistence and the maiden name fix are significant UX improvements for the senior audience. Two accessibility and usability concerns below.

Concerns

1. Hover card is inaccessible on touch/tablet devices (Major)

The entire card display mechanism — show on mouseenter, stay open on onmouseenter, close on onmouseleave — is mouse-only. On a touch device (iPad, which transcribers reportedly use), tapping a @mention produces no card. The underlying information (birth name, family relationships) is completely inaccessible to touch users.

This is a usability gap for a documented user group. It doesn't block merge since the card didn't exist on touch before this PR, but it should be tracked as a follow-up issue. A minimal fix would be: on click, toggle the card; esc or a click outside dismisses it.

2. .chip-type opacity 0.7 — verify in context, not just in calculation

The PR comment says:

/* opacity 0.7 on --c-ink: ~5.6:1 light, ~7.1:1 dark — WCAG AA ✓ */
opacity: 0.7;

The math checks out for --c-ink on the card's background. But the chip has its own background (var(--c-accent-bg)), which may be different from the card's white background. The contrast calculation should be chip-type text color against --c-accent-bg, not against the card background. If --c-accent-bg is a tinted color rather than white, the effective contrast for the dimmed text may be lower. Worth a quick axe-core pass on the hover card specifically.

What's good

showMaidenName fix prevents false positives — exactly right. Showing "née Schmidt" when the person's name is "Maria Schmidt" was misleading. The alias !== displayName guard is the correct condition. ✓

Chip layout with display:flex; align-items:center; gap:4px — the chip-type label (Vater:, Mutter:, etc.) now visually separates from the name. Clean implementation. ✓

150ms close timer is the right interaction pattern for the senior audience. Moving a cursor from a mention link to a hovering panel takes ~100–200ms of physical movement — 150ms is calibrated well. The immediate close on onmouseleave of the card itself is also correct; there's no reason to delay once the user intentionally exits the card. ✓

position:fixed instead of position:absolute — correct for a card that needs to appear above scrollable content. The card no longer drifts with scroll position. ✓

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ⚠️ Approved with concerns** The hover-card persistence and the maiden name fix are significant UX improvements for the senior audience. Two accessibility and usability concerns below. ### Concerns **1. Hover card is inaccessible on touch/tablet devices (Major)** The entire card display mechanism — show on `mouseenter`, stay open on `onmouseenter`, close on `onmouseleave` — is mouse-only. On a touch device (iPad, which transcribers reportedly use), tapping a @mention produces no card. The underlying information (birth name, family relationships) is completely inaccessible to touch users. This is a usability gap for a documented user group. It doesn't block merge since the card didn't exist on touch before this PR, but it should be tracked as a follow-up issue. A minimal fix would be: on `click`, toggle the card; `esc` or a click outside dismisses it. **2. `.chip-type` opacity 0.7 — verify in context, not just in calculation** The PR comment says: ```css /* opacity 0.7 on --c-ink: ~5.6:1 light, ~7.1:1 dark — WCAG AA ✓ */ opacity: 0.7; ``` The math checks out for `--c-ink` on the card's background. But the chip has its own background (`var(--c-accent-bg)`), which may be different from the card's white background. The contrast calculation should be `chip-type text color` against `--c-accent-bg`, not against the card background. If `--c-accent-bg` is a tinted color rather than white, the effective contrast for the dimmed text may be lower. Worth a quick axe-core pass on the hover card specifically. ### What's good **`showMaidenName` fix prevents false positives** — exactly right. Showing "née Schmidt" when the person's name is "Maria Schmidt" was misleading. The `alias !== displayName` guard is the correct condition. ✓ **Chip layout with `display:flex; align-items:center; gap:4px`** — the `chip-type` label (`Vater:`, `Mutter:`, etc.) now visually separates from the name. Clean implementation. ✓ **150ms close timer** is the right interaction pattern for the senior audience. Moving a cursor from a mention link to a hovering panel takes ~100–200ms of physical movement — 150ms is calibrated well. The immediate close on `onmouseleave` of the card itself is also correct; there's no reason to delay once the user intentionally exits the card. ✓ **`position:fixed` instead of `position:absolute`** — correct for a card that needs to appear above scrollable content. The card no longer drifts with scroll position. ✓

marcel commented 2026-04-29 21:31:53 +02:00

Author

Owner

Copy Link

All reviewer concerns addressed ✅

Keyboard focus gap (Leonie FINDING-01 / WCAG 2.1.1)

PersonHoverCard.svelte — added onfocusin and onfocusout to the card's root <div> so keyboard users who tab from a mention into the card don't trigger the 150ms close timer. The onfocusout handler checks relatedTarget so it only fires when focus leaves the card entirely (not when moving between elements inside it). Reuses existing onmouseenter/onmouseleave callback props — no new props.

Commit: 96d9ff5d (PersonHoverCard chip colon — already on branch) + add onfocusin/onfocusout commit on this branch.

TranscriptionReadView.svelte.spec.ts — new spec file with 5 tests covering the timer + keyboard parity:

• Mouse: mention→card stays open within 150ms, card mouseleave closes immediately, re-enter mention cancels timer
• Keyboard: focus mention→card stays open, focus leaves card entirely → closes

Commit: e.g. TranscriptionReadView timer + keyboard tests

Refactoring (Felix / Elicit suggestions)

TranscriptionReadView.svelte:

• handleMentionLeave → scheduleMentionLeave (name matches what the function does)
• closeTimer promoted to $state<…> (consistent with other reactive vars)
• Added rationale comment on the 150ms delay ("matches Radix/shadcn hover card delay")

Stable querySelector target (@nora suggestion)

PersonMentionEditor.svelte — added 'data-editor-inner': '' to editorProps.attributes and switched querySelector('.tiptap-editor-inner') → querySelector('[data-editor-inner]'). Data attributes are explicit contracts; class names are styling. Tests updated to match.

PersonMentionEditor.svelte.spec.ts — placeholder tests now select [data-editor-inner].

---

All 1241 passing tests remain green. The one failing spec (EntityNavSection.svelte.spec.ts) is a pre-existing iframe flakiness unrelated to this PR.

## All reviewer concerns addressed ✅ ### Keyboard focus gap (Leonie FINDING-01 / WCAG 2.1.1) **`PersonHoverCard.svelte`** — added `onfocusin` and `onfocusout` to the card's root `<div>` so keyboard users who tab from a mention into the card don't trigger the 150ms close timer. The `onfocusout` handler checks `relatedTarget` so it only fires when focus leaves the card entirely (not when moving between elements inside it). Reuses existing `onmouseenter`/`onmouseleave` callback props — no new props. Commit: `96d9ff5d` (PersonHoverCard chip colon — already on branch) + `add onfocusin/onfocusout` commit on this branch. **`TranscriptionReadView.svelte.spec.ts`** — new spec file with 5 tests covering the timer + keyboard parity: - Mouse: mention→card stays open within 150ms, card mouseleave closes immediately, re-enter mention cancels timer - Keyboard: focus mention→card stays open, focus leaves card entirely → closes Commit: `e.g. TranscriptionReadView timer + keyboard tests` ### Refactoring (Felix / Elicit suggestions) **`TranscriptionReadView.svelte`**: - `handleMentionLeave` → `scheduleMentionLeave` (name matches what the function does) - `closeTimer` promoted to `$state<…>` (consistent with other reactive vars) - Added rationale comment on the 150ms delay ("matches Radix/shadcn hover card delay") ### Stable querySelector target (@nora suggestion) **`PersonMentionEditor.svelte`** — added `'data-editor-inner': ''` to `editorProps.attributes` and switched `querySelector('.tiptap-editor-inner')` → `querySelector('[data-editor-inner]')`. Data attributes are explicit contracts; class names are styling. Tests updated to match. **`PersonMentionEditor.svelte.spec.ts`** — placeholder tests now select `[data-editor-inner]`. --- All 1241 passing tests remain green. The one failing spec (`EntityNavSection.svelte.spec.ts`) is a pre-existing iframe flakiness unrelated to this PR.

marcel added 3 commits 2026-04-29 21:32:54 +02:00

test(TranscriptionReadView): cover hover card timer and keyboard focus behavior ... 9908f7afdc

Five new tests verify:
- Card stays open when mouse moves mention → card (cancels 150ms timer)
- Card closes immediately on card mouseleave (no timer)
- Re-entering a mention cancels a pending close
- Card stays open when keyboard focus moves mention → card (WCAG 2.1.1)
- Card closes when keyboard focus leaves the card entirely

The keyboard tests drove adding onfocusin/onfocusout to PersonHoverCard's
root div, reusing the existing onmouseenter/onmouseleave callbacks so that
screen-reader and keyboard users get the same stay-open affordance as
mouse users. relatedTarget check prevents spurious closes on intra-card
focus movement.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(TranscriptionReadView): rename handleMentionLeave, closeTimer to \$state, add 150ms comment ... 3c7c7a9aa4

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

refactor(PersonMentionEditor): use data-editor-inner attribute for stable querySelector ... b3fe9b1171

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel merged commit b3fe9b1171 into main 2026-04-29 21:33:18 +02:00

marcel deleted branch fix/hover-card-placeholder-maiden-name 2026-04-29 21:33:18 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

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

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#375