feat(person-mention): PR-B2 — read-mode rendering + hover card (issue #362) #371
Reference in New Issue
Block a user
Delete Branch "feat/person-mentions-issue-362-frontend-b2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements PR-B2 of issue #362 — read-mode rendering of person mentions and the hover card. Builds on PR-A (backend, merged) and PR-B1 (edit-mode infrastructure, merged).
What this PR does
mention.tsrenderTranscriptionBody(text, mentions)— escapes HTML, replaces every@DisplayNamewith<a href="/persons/{id}" class="person-mention" data-person-id="{id}">DisplayName</a>. Strips the@trigger from the rendered link text per spec.mention.ts@Hansno longer eats@HansMüller. Longest-displayName-first + first-sidecar-wins make rendering deterministic for the OQ-1 collision case (#5339 §C4).routes/layout.css.person-mentionglobal rule next to.mention. Underline at rest (WCAG AA), focus-visible 2px box-shadow ring on--c-ink.PersonHoverCard.svelteprefers-reduced-motion: reduce), generic error message, loaded card (navy header with name, life-date range,geb. <alias>; family-only relationship chipsPARENT_OF/SPOUSE_OF/SIBLING_OF; notes excerpt capped at 120 chars; footerZur Person →+ hover hint). Hidden via@media (hover: none)on touch devices.role="region"+aria-live="polite"so SR announces loaded content.TranscriptionReadView.sveltesplitByMarkers+renderTranscriptionBody— markers stay as<em data-marker>siblings (B19b). Mounts the hover card onmouseenterwith manualgetBoundingClientRect()flip (default below-right; flip up if mention <200px from bottom or in bottom 30% of viewport; flip left if <300px from right). 404 → marks anchordata-person-deleted="true", suppresses card and click. Per-pageSvelteMap<personId, Promise>dedup cache (B15.5). Click →goto('/persons/{id}'). eslint-disable comment mirrorsCommentMessage.svelte:88-89.e2e/person-mention-read.spec.tspage.hover()mounts the card, mouseleave unmounts. B21: Pixel 7 device context withhasTouch: true—page.tap()navigates without ever showing the card.Stored-XSS hardening (Sina #5505 / Nora PR-A review)
displayNameflows from sidecar intoblock.textunescaped on the backend — PR-B2 escapes both block text anddisplayNameon render.<script>,<img onerror>, HTML-entity-already-encoded payloads, and quote characters indisplayName(would otherwise break thehrefattribute).Tests
renderTranscriptionBody(XSS, mention rendering, longest-first, first-sidecar-wins, word boundary, no-double-encoding).PersonHoverCard(skeleton/error/loaded states, family-chip filter, 120-char truncation, optional sections, ARIA, positioning).TranscriptionReadView(mention rendering, B19b marker composition, fetch dedup B15.5, hover card mount/unmount, 404 degrade).Total new tests: 47. All passing. The single unrelated failing unit test on
main(hilfe/transkription/page.svelte.spec.ts) is pre-existing and verified not introduced by this PR.Coordination notes
frontend/src/lib/types.ts'sPersonMentiontype (added in PR-A regen).person_mention_open_link,person_mention_hover_hint,person_mention_load_error(added in PR-B1).formatLifeDateRange()reused frompersonLifeDates.ts(PR-B1).Test plan
npm run test— confirms all PR-B2 unit tests pass (47/47).npx playwright test person-mention-read.spec.tsagainst the running stack — confirms B20/B21 pass.@mentionin the transcription panel — verifies skeleton appears within 200ms, then loaded card.Closes the read-mode half of #362 once merged together with PR-A and PR-B1.
🤖 Generated with Claude Code
Replaces every @DisplayName in a transcription block's text with an anchor link to /persons/{personId}, sourced from the mentionedPersons sidecar. The @ prefix is stripped from the rendered link text per spec — it is an editor affordance, not part of the historical text. Stored-XSS hardening: HTML-escapes block text, displayName, and personId before injection. Word-boundary lookahead avoids prefix collisions (@Hans vs @HansMüller). Longest-displayName-first + first-sidecar-wins make rendering deterministic for the OQ-1 collision case (#5339). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Underline-at-rest (WCAG AA) so the link affordance does not depend on colour alone. focus-visible uses a 2px box-shadow ring on --c-ink with a 2px border-radius — the same focus-ring shape as the comment .mention chip but rectangular instead of pill, since the anchor sits in flowing text. Lives next to the existing .mention rule because Svelte scoped styles do not reach the HTML injected by {@html …} in TranscriptionReadView. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>The card has three render states: - loading → 320×180 skeleton with three pulse-animated bars; respects prefers-reduced-motion (animation disabled, opacity dimmed) - error → generic load-error message in the body; the footer link still navigates (click works regardless of fetch outcome) - loaded → navy header with name, life-date range, and "geb. <alias>"; family-only relationship chips (PARENT_OF / SPOUSE_OF / SIBLING_OF) — non-family types are filtered out; notes excerpt capped at 120 chars with ellipsis; footer with "Zur Person →" + hover hint aria-live="polite" on the card root so screen readers announce loaded content when the fetch resolves; the host's id is the cardId so the parent anchor can use aria-describedby. The card is hidden via @media (hover: none) on touch devices — tap navigates directly per spec. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Composes splitByMarkers + renderTranscriptionBody so [unleserlich] markers render as <em data-marker> siblings of the mention anchor — neither nested inside the other (B19b). Hover card lifecycle on each .person-mention anchor: mouseenter → set aria-describedby, place card via getBoundingClientRect (default below-right; flip up if <200px from bottom or mention is in bottom 30% of viewport; flip left if <300px from right), fire fetch, mount card with skeleton state resolved → swap card to loaded state with person + family relationships (PARENT_OF / SPOUSE_OF / SIBLING_OF only) 404 → degrade: mark anchor with data-person-deleted="true", unmount card, suppress future hovers/clicks network → swap card to error state — link still navigates mouseleave → drop aria-describedby, unmount card Per-page SvelteMap<personId, Promise> cache (B15.5) so a sweep across N mentions of the same person fires the backend once. Click handler calls goto() so SvelteKit handles routing without a full reload. Event listeners are attached once per article via a Svelte action because the anchor HTML is injected via {@html ...} and would not receive declarative bindings. The eslint-disable comment mirrors the rationale on CommentMessage.svelte:88-89. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Creates a Person, document, annotation, and transcription block with mentionedPersons sidecar, then exercises the read-mode link in two contexts: - Desktop: page.hover() mounts the hover card; mouseleave unmounts. - Touch (Pixel 7 device): page.tap() navigates to /persons/{id} without the card ever mounting (tap opens the page directly). Tests are sequential because they share a single document/person via beforeAll/afterAll. The touch test spins up a separate browser context with hasTouch=true reusing the stored auth state. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Markus Keller — Senior Application Architect
Verdict: Approved with concerns
This is a well-bounded frontend-only change. The module boundary between the rendering helper (
mention.ts), the presentational component (PersonHoverCard), and the orchestrator (TranscriptionReadView) is clean — each layer owns one concern, and dependencies flow inward. No service-to-repository leaks, no shared DTOs creeping across feature modules. Reuse offormatLifeDateRangeand the existing Paraglide keys is exactly the kind of low-friction reuse I expect.Concerns (worth addressing, not blockers)
{@html ...}boundary needs a single, named gateway.TranscriptionReadView.svelte:203andCommentMessage.svelte:88-89botheslint-disablethe same Svelte rule with effectively the same justification. Two call sites today, three or four next quarter, and the safety invariant ("the renderer always escapes first") is now distributed knowledge. Consider extracting a tiny<SafeHtml html={...} />wrapper or a typedRenderedHtmlbrand type returned fromrenderTranscriptionBody/renderBody. The boundary becomes auditable in one place — exactly the centralisation principle that wins for security/audit code.Module placement of
LoadState.LoadStateis exported fromPersonHoverCard.svelte(frontend/src/lib/components/PersonHoverCard.svelte:9-12) and re-imported by the orchestrator. That is fine, but the type now belongs to two layers: the view's contract and the orchestrator's state machine. If a third caller wants the same shape (admin previews, briefwechsel cards), this becomes a circular import waiting to happen. ApersonHoverCard.types.tsnext topersonLifeDates.tskeeps the type module-stable.Two DOM selector strategies in one file.
attachMentionHandlers(line 161) hand-rolls capture-phase listeners onmouseenter/mouseleavebecause they don't bubble — fine, this is the right call for delegated handling on{@html}-injected anchors. But the selectora.person-mentionis a magic string repeated four times here and once more in CSS, e2e, and unit tests. Extract aPERSON_MENTION_SELECTOR = 'a.person-mention'constant alongside the renderer so the contract between renderer and orchestrator is a single import, not a string-literal coincidence.In-memory cache lifetime.
hoverCacheis per-component instance (good, scoped to one document view). ButdeletedPersonIdsis also per-component — if the same person is deleted by an admin in another tab while this view is open, the cache will still serve the loaded card on next hover. For PR-B2 that's acceptable (404 is rare in this read-only view), but I'd add a comment naming the trade-off so a future reviewer doesn't try to "fix" it by reaching for a global store.No ADR for the read-mode rendering pipeline. The sequencing —
splitByMarkers→ per-segmentrenderTranscriptionBody→ join — is non-obvious enough to merit a short note indocs/adr/(or whatever the project uses). The B19b composition decision and the OQ-1 first-sidecar-wins rule are exactly the kind of "why is the code shaped like this" decision that gets reverse-engineered six months from now.Things done well
mention.tsare testable without DOM, exactly as the testable-architecture rule demands.PersonHoverCard) — not "Manager" or "Helper".SvelteMap/SvelteSetcorrectly used for reactive collections.Felix Brandt — Senior Fullstack Developer
Verdict: Approved with concerns
Read every changed line. TDD evidence is solid — 47 tests with named, sentence-style assertions;
mention.spec.tscovers the security cases (XSS, quote-in-href, double-encoding, word boundary, longest-first, first-sidecar-wins) before the implementation got smarter than areplaceAll. That's the discipline I expect.Blockers
None.
Suggestions (in priority order)
fetchHoverDatadoes two things — split it.frontend/src/lib/components/TranscriptionReadView.svelte:57-76handles cache lookup and the actual fetch and relationship merging and 404 normalisation. That's four jobs. Extract:The cache helper is now 5 lines and the fetch logic gets its own test boundary.
handleMentionEnterhas too many states for one function. Same file, lines 102-138 — six early-returns interleaved with two state assignments. Hard to scan. Split into:prepareCard(link, personId)— derivescardId,position, sets aria-describedby, returns initial{status: 'loading'}activeCardapplyResult(personId, data)— updates activeCard if it's still ourshandleMentionEnter— orchestrates the twoThe current inline pattern works, but each helper would be testable on its own and the orchestrator becomes 5 lines.
computeCardPositionmagic numbers.vh * 0.7,vw - rect.left < 300,CARD_WIDTH,CARD_HEIGHT(line 87-93). The 30 % bottom-band rule and the 300 px right-flip threshold are spec from Leonie #5329 — pin those numbers in a named constant block at the top of the file (BOTTOM_BAND_RATIO,RIGHT_FLIP_THRESHOLD_PX). If they ever drift, the spec link drifts with them.Test brittleness —
setTimeout(r, 50)inTranscriptionReadView.svelte.test.ts. Lines 283, 311, 352, 365-367, 383. These are racing the microtask queue.await Promise.resolve()(orawait vi.waitFor(() => …)) is deterministic;setTimeout(r, 50)is "I hope this is enough." Sara is going to flag this — get ahead of it.mention.ts:87-95could fold the dedup into the sort. Two passes (dedup then sort) where one would do —Array.from(new Map(mentionedPersons.map(m => [m.displayName, m])).values()).sort(...). Same first-sidecar-wins behaviour, half the LOC, one allocation. Personal taste, not a hill to die on.renderTranscriptionBodyreturns a string the caller must trust. This is the same shape Markus flagged from architecture. From a clean-code lens: the return type isstring, notRenderedHtml. Brand types (type RenderedHtml = string & { __brand: 'rendered' }) cost nothing and make it impossible to pass a raw user string to{@html}by accident. Consider for a follow-up.handleMentionClickignores middle-click and ctrl/cmd+click. Line 146-156 unconditionallyevent.preventDefault()andgoto(...). That breaks "open in new tab" — a real workflow for researchers comparing two persons. Either let modified clicks fall through to the default<a href>behaviour, or document why the SPA-only navigation is intentional.Things done well
handleMentionEnterreads top-down.$derived(not$state+$effect) forfamilyChips,dateRange,notesExcerptin PersonHoverCard — exactly right.SvelteMap/SvelteSetfromsvelte/reactivity— exactly right for reactive collections.{#each ... (chip.id)}and{#each ... (block.id)}.Tobias Wendt — DevOps & Platform Engineer
Verdict: Approved
Frontend-only change. No Compose deltas, no CI workflow changes, no image tags, no volumes, no env vars, no secrets. There is genuinely nothing here in my domain to flag.
What I checked
docker-compose.yml,infra/,.gitea/workflows/— untouched in the diff.package.json— confirmed via the PR description and the diff.e2e/person-mention-read.spec.tsreuses the existingSTORAGE_STATEauth fixture and the existingwebServerconfig inplaywright.config.ts. It does not require new CI infrastructure (E2E isn't run in CI today perfrontend/e2e/CLAUDE.md).Tiny observation, not a blocker
setTimeout(r, 50)waits inTranscriptionReadView.svelte.test.tsare component-test code, not E2E, but the same anti-pattern Sara flagged in the past for our suite. If these slip past her into CI, they'll be the source of the next intermittent failure on a slow runner. Worth fixing before that bites — but that is a Sara/Felix call, not a DevOps blocker.LGTM from the platform side.
Elicit — Senior Requirements Engineer
Verdict: Approved with concerns
I'm in Brownfield mode here, reviewing PR-B2 against issue #362 expectations. This PR-B2 description is dense, traceable (#5339, #5505, #5324, B19b, B15.5, OQ-1), and links each technical decision back to a comment thread or spec ID. That's the kind of issue-driven development I'd hold up as the model. Most of my concerns are about completeness of the acceptance evidence, not about the implementation.
Findings
Missing acceptance criteria for the "404 deletion" path in the e2e suite. The PR description's test plan item 5 is "Reviewer deletes a mentioned Person via admin — refreshes the document — confirms the link degrades to plain text on first hover." The component test covers the 404 →
data-person-deleted="true"path (TranscriptionReadView.svelte.test.ts:373-391), but the e2e suite atfrontend/e2e/person-mention-read.spec.tsdoes not exercise this. The e2e tests cover B20 (hover) and B21 (touch tap) only. Either:Either is fine; what's not fine is having a test plan item with no automated assertion in the suite.
NFR — accessibility coverage gap for the hover card itself.
aria-live="polite"is set, butprefers-reduced-motionis honoured only for the skeleton pulse animation (good). However:role="region"without anaria-labeloraria-labelledby. WCAG 1.3.1: a region landmark without an accessible name is a known axe-core warning. Either name the region (e.g.aria-label={state.person.displayName}when loaded) or downgrade the role.NFR — keyboard reachability. Description says "WCAG AA". Read mode anchors are real
<a href>so they tab naturally. But: when a keyboard user tabs onto the mention, the hover card never appears —mouseenteris the only trigger (TranscriptionReadView.svelte:175). For the senior audience constraint Leonie reminds us about, this is a usability gap. Two options:focusin/focusoutlisteners alongsidemouseenter/mouseleave(small change, would close the gap).The issue body should say which.
Ambiguity — "family-only relationship chips PARENT_OF/SPOUSE_OF/SIBLING_OF". What about
CHILD_OF? The data model historically uses directional relationships (PARENT_OF as a forward edge from parent → child), but if the PR only filters on those three values, a person who appears as a child of someone (i.e., the relationship row says PARENT_OF with the other person in therelatedPersonId) — does the chip render? Unit tests only assert that filtering works for the three named types; they don't test the directionality. OQ-372-01: Should chips also surface inverse relationships (e.g., my parents, even though the row was created from their side)? This may already be handled by the backend's relationship endpoint. Confirm or add a test.Hidden assumption — "120-char excerpt" is unilateral. The notes excerpt cuts at 120 characters with an ellipsis (
PersonHoverCard.svelte:28, 42-48). That's mid-word truncation in German (long compound nouns: "Familienzusammenführung"). For a senior audience this can produce confusing output ("Sie war eine bekannte Familienzu…"). Prefer cutting at the last word boundary. Either fix or note the deferred decision.Open question — caching scope. Per Markus, the cache is per-component instance. Per-document is fine for a read view that lives one mount cycle. But if the user opens transcription, closes it, opens it again on the same doc, the cache is rebuilt and we re-fetch every mentioned person. Is that intentional? OQ-372-02: Should the hover-card cache be per-document (survives panel close/reopen) or per-mount (current)? Either answer is defensible — surface the choice.
Done well
person_mention_*Paraglide keys", "No new deps") is exactly the kind of cross-PR breadcrumb that solo + LLM-driven workflows need.Nora "NullX" Steiner — Application Security Engineer
Verdict: Approved with concerns
The XSS hardening here is unambiguously good.
escapeHtmlis correct (escapes& < > " 'in the right order, ampersand first), andrenderTranscriptionBodyescapes the block text before substituting mention anchors. The testmention.spec.ts:269-304pins the right invariants: script tag in displayName escaped,<img onerror>payload in surrounding text escaped, double-encoding preserved (no silent decode), and — important — quote-in-displayName lands as"so it cannot break out of thehrefattribute. CWE-79 mitigated at the rendering boundary, exactly where it should be.Findings (in OWASP-priority order)
🟡 Medium — Suggestion: rel="noopener noreferrer" on the rendered anchor
File:
frontend/src/lib/utils/mention.ts:102The href is internal (
/persons/{uuid}) sonoopenerisn't a hard requirement today. But the link template doesn't constrain that — ifescapedPersonIdever flowed in from a less-sanitised source (e.g. a future "external person" feature), this template would happily emit a clickable absolute URL. Two options:personIdis a UUID at the renderer boundary (defense in depth).rel="noopener"so the anchor is safe by construction even if the URL pattern ever widens.I'd take option 1: a regex check
/^[0-9a-f-]{36}$/i.test(mention.personId)at the top of the loop, skip the substitution if it fails. CWE-79 partially, CWE-601 (Open Redirect) more directly. Negligible perf cost, large defensive benefit.🟡 Medium —
@htmlboundary trusts a string return typeSame root concern as Markus's #1.
renderTranscriptionBodyreturnsstring— TypeScript cannot tell whether the string came from the renderer or from an attacker. The next refactor that does{@html block.text}instead of{@html renderBlockHtml(block)}is a single typo away from a stored-XSS regression. Brand the return type:Then the
@htmlconsumer requires aSafeHtml. Compile-time enforcement of the escape invariant. Adds 2 lines.🟢 Low —
data-person-deleted="true"set on the link, but click handler still calls preventDefaultFile:
TranscriptionReadView.svelte:146-156When a 404 marks a link as deleted,
handleMentionClickdoesevent.preventDefault()and returns without navigating. Net effect: clicking a tombstoned mention silently does nothing. From a UX-security angle this is a minor "disabled control without feedback" smell. Not a vuln, but a user might think they clicked a bad link. Consider either:/persons/{deletedId}and the existing 404 page handles it cleanly), or🟢 Low — fetch error path swallows non-404, non-OK responses
File:
TranscriptionReadView.svelte:64-65…then in
handleMentionEnterthe catch block sets state to'error'but never logs. A 500 from the persons endpoint becomes a generic "Daten konnten nicht geladen werden" with no observability hook. For a read-only view this is acceptable, but adding aconsole.warn(...)(or a Sentry/GlitchTip breadcrumb if/when that lands) would make production triage possible. Not a security finding per se, but missing-audit-trail is in our threat model for the family archive.✅ Things I checked and they're correct
escapeHtmlorder:&first, then<>"'— no double-encoding bug.escapeRegExpcorrectly escapes the regex metacharacters that Unicode display names could contain (.,*,+,?,^,$,{},(),|,[],\).gu(global, unicode) — required for\p{L}\p{N}lookahead to work on German umlauts and Turkish names.eval, noinnerHTMLwrites, nodocument.writein the diff.data-person-id="${escapedPersonId}"— escaped, attribute-safe, can't break out of the data attribute.(?![\\p{L}\\p{N}])correctly prevents@Hansfrom matching@HansMüllerAND prevents an attacker from confusing the renderer by appending characters that look like part of the displayName.Detection notes for the regression suite (already in the PR)
The XSS tests in
mention.spec.ts:269-304are exactly the permanent regression tests I'd ask for. Keep them. If/when a Semgrep config is added, a rule that flags{@html …}without a typedSafeHtmlconsumer would catch the next round-trip.Sara Holt — Senior QA Engineer
Verdict: Approved with concerns
47 new tests across the right layers (unit on
mention.ts, component onPersonHoverCardandTranscriptionReadView, e2e for B20/B21). Pyramid shape is correct: most assertions live in fast unit tests, only the touch-vs-hover device matrix is at the e2e tier. Test names read as sentences. Factory helpers (AUGUSTE,mentionBlock,POSITION) are extracted, not inlined. This is the test discipline I want to see.Blockers
None.
Concerns
setTimeout(r, N)waits in component tests — five occurrences.frontend/src/lib/components/TranscriptionReadView.svelte.test.tslines 283, 311, 352, 365-367, 383. Every one of these is racing the microtask queue. The 50 ms wait at line 352 will pass on a fast laptop and fail on a loaded CI runner. Replace withvi.waitFor(...):vi.waitForpolls until the assertion passes (default 1000 ms timeout, 10 ms interval). It's deterministic — passes the moment the condition is true, fails the moment the timeout elapses. Auto-graded as@Disabled-equivalent risk in my book.e2e
B21test isolation.frontend/e2e/person-mention-read.spec.ts:133-153opens a second browser context withPixel 7device. The contract is "the tap navigates without ever showing the card". The assertionawait expect(touchPage.getByTestId('person-hover-card')).toHaveCount(0)runs after the navigation has already happened — meaning we're now on/persons/{id}and of course there's no hover card on that page (the hover card only exists on the document detail page). The test is effectively only asserting the URL change. Tighten by checkingtoHaveCount(0)before the tap as well, or check during a smallwaitForTimeoutbetween hover and tap.e2e cleanup in
afterAllwon't run ifbeforeAllthrows. Lines 89-92: ifCreate document failed: 4xxthrows,docIdis undefined and the cleanupif (docId) await request.delete(...)quietly skips. Acceptable for this suite, but the first failure leaks state into subsequent CI runs (mostly fine since the doc has a unique title with timestamps elsewhere — but this title is not timestamped). Add a uniqueifier:title: \E2E Person Mention Read ${Date.now()}``.No accessibility check at the hover-card layer. No
axe-playwrightscan is run on the document detail page with the card mounted. Per our standard: critical-flow pages get axe-core in E2E. Add a check afterawait link.hover()andawait expect(card).toBeVisible(). This will probably surface therole="region"withoutaria-labelissue Nora and Elicit independently noticed.No visual regression at 320px. The hover card is
width: 320px(PersonHoverCard.svelte:105). On a 320 px viewport that's literally the entire screen width. The flip-left logic atTranscriptionReadView.svelte:92-94only fires whenvw - rect.left < 300, which on a 320 px viewport means every hover triggers it. There is no visual regression test verifying the card actually fits and doesn't horizontal-overflow. Add a screenshot test or document that the hover card is not supported below 768 px (consistent with@media (hover: none), since touch-first phones already suppress it). If suppressed, the hover-card sizing concern is moot — but document the policy.Test for the position-flip logic is missing.
computeCardPosition(TranscriptionReadView.svelte:78-100) has 4 branches (default, flip-up, flip-left, both) and 2 thresholds (CARD_HEIGHT + CARD_GAP,vh * 0.7,300). Zero unit tests. The function is pure (rect → {top, left}) — extract tosrc/lib/utils/hoverCardPosition.tsand test every branch. Right now this is the second-most-complex function in the PR and the only one without a test.PersonHoverCard.svelte.spec.ts:209-211weak assertion.122is a magic number (120 + '…'.length === 121, plus a fudge?). Pin the exact length:expect(notes.textContent).toBe('x'.repeat(120) + '…'). The current assertion would still pass if the truncation went to 121 chars or 100 chars — both are bugs.Things done well
mention.spec.ts, 17 tests), e2e is a small focused critical-path layer.AUGUSTE,POSITION,mentionBlock) — exactly the readable-tests rule.renderTranscriptionBodyare excellent — empty, no mentions, longest-first, word boundary, first-sidecar-wins, XSS variants. This is regression coverage that will pay off for years.Leonie Voss — UX Designer & Accessibility Strategist
Verdict: Approved with concerns
The intent is right — underline at rest, not colour alone (passes 8% of red-green colour-blind users); focus-visible ring with 2px box-shadow that doesn't touch the glyphs; touch suppression via
@media (hover: none); reduced-motion override on the skeleton pulse. These are all proper a11y instincts, not afterthoughts. But there are concrete fixes I want before this lands.Critical (blocks accessible use)
FINDING-01 — Hover card is unreachable by keyboard
TranscriptionReadView.svelte:175-177only mounts the card onmouseenter. A keyboard user tabbing through transcribed text reaches the anchor (good —<a href>is naturally focusable) but never sees the rich-context card. They can press Enter to navigate, but the previewing function the card provides is mouse-only.focusin/focusoutlisteners alongsidemouseenter/mouseleave:handleMentionEnter, the samegetBoundingClientRect()works for focused elements.High (degrades experience)
FINDING-02 —
role="region"without an accessible namePersonHoverCard.svelte:55declaresrole="region"but noaria-label/aria-labelledby. Screen readers announce "region" with no name.aria-live="polite"alone for SR announcements (simpler).FINDING-03 — Skeleton bars rendered as
<div>not progress indicator<div class="bar">carry no semantics for assistive tech. A screen-reader user hearing "region" → silence → person name has no signal that the silence was loading. The skeleton is a visual affordance only.role="status"on the skeleton wrapper with text "Lade Person…" (visually-hidden viasr-only), or setaria-busy="true"on the region root while loading and letaria-livehandle the loaded announcement.FINDING-04 — Notes truncation can split mid-word in German
PersonHoverCard.svelte:46-47—notes.slice(0, 120) + '…'. A 130-char note ending in "…war eine bekannte Familienzusammenführungsorganisatorin" truncates to "…Familienzusam…" mid-word.FINDING-05 — Hover card may overflow horizontally at 320px
@media (hover: none)width: 320px. On a viewport of exactly 320px the right-flip logic still tries to position it;computeCardPositionreturnsMath.max(0, ...)forleftbut doesn't clampleft + 320 ≤ vw. Right edge can clip.computeCardPosition, after the flip, clamp:@media (max-width: 480px) { .person-hover-card { display: none; } }next to the existing(hover: none)rule. Pick one and document.Medium
FINDING-06 — Underline color contrast at rest
layout.css—text-decoration-color: color-mix(in srgb, var(--c-accent) 60%, transparent). With--c-accent= brand-mint #A6DAD8 at 60% on white, effective colour is ~#C9E6E5. Against white that's roughly 1.6:1 — fails WCAG 1.4.11 for non-text contrast. WCAG 1.4.11 doesn't apply to text decorations strictly (since the underline isn't load-bearing — the text colour itself isvar(--c-ink)which passes), but the underline is also the only visual signal that this is a link mid-paragraph. If the underline is barely visible at rest, the user might miss the affordance entirely.var(--c-ink)at 50% so the underline is visible against any background.FINDING-07 — Touch suppression via
@media (hover: none)is correct, but document@media (hover: none)rule (PersonHoverCard.svelte:119-123) is exactly right for tablets/phones. But hybrid devices (Surface, Chromebook with touch) match(hover: hover) and (pointer: coarse)and can still trigger card and tap. Probably fine; if you see flickering on those, switch to(hover: hover) and (pointer: fine).Things done well
prefers-reduced-motion: reducehonoured for the skeleton pulse.box-shadowfocus ring withborder-radius: 2pxso the rectangle doesn't touch glyphs — exactly right.min-height: 180pxis calm and not crowded.--c-accent-bgchip background.aria-live="polite"so loaded content is announced — almost there, just needs the region naming and the skeleton status.Markus, Felix, and Nora independently flagged the {@html …} boundary as a distributed-knowledge security risk: today renderBody and renderTranscriptionBody return string, so the next refactor that does {@html block.text} (instead of {@html renderBlockHtml(block)}) is one typo away from a stored-XSS regression. Introduce a SafeHtml brand type (string with a phantom __brand) returned by both renderers and by renderBlockHtml in TranscriptionReadView. Compile-time enforcement of the escape invariant — costs zero runtime, makes the contract auditable in one file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Nora's CWE-601 (Open Redirect) defense-in-depth concern: today the backend emits UUIDs, but renderTranscriptionBody concatenates personId straight into an href. If a future "external person" feature ever flows a non-UUID through the sidecar, the renderer would happily emit `<a href="javascript:…">`. Add a strict UUID regex check before substituting. Non-UUID entries fall through unchanged so the @-trigger remains as plain text — no silent data loss, no clickable redirect. Three new failing→passing tests cover javascript: scheme, absolute URL, and the positive case (well-formed UUID still renders). Existing tests that used synthetic IDs ("p-short", "p-first", etc.) updated to real UUIDs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Leonie FINDING-04 + Elicit E5: notes.slice(0, 120) cuts mid-word, especially ugly in German compound nouns ("…Familienzu…"). Sara #7: the assertion .toBeLessThanOrEqual(122) was a magic number that hid this bug. Add truncateAtWordBoundary(text, max): cut at the last space inside the window unless it'd shrink the excerpt below 70% (single-word fallback). Single-word case still produces hard-cut + ellipsis so a 150-char word shows the first 120 chars + … rather than nothing. Tests pinned to exact strings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Cycle 1 fixes pushed — every persona concern addressed
Pushed 13 atomic commits addressing every actionable concern raised by the seven personas. Branch up to
bc58d77f. Test suites: 113/113 PR-specific tests green; full suite still passes apart from the two pre-existing failures inRichtlinienRuleCard.svelte.spec.tsandhilfe/transkription/page.svelte.spec.ts(unchanged fromorigin/main).Concerns resolved
personHoverCard.ts6a6967d8SafeHtmlbrand type on both renderers +renderBlockHtml488d438426519d02PERSON_MENTION_SELECTORconstant1842e23ccomputeHoverCardPositionextracted with named constants + viewport clamp4a7e8a0cfetchHoverData→loadHoverData+getOrFetchHoverData; cache-lifetime trade-off documented5890bb3a3faac135focusinfor keyboard users (WCAG 2.1.1)3365f584aria-labelon region,aria-busywhile loading,role="status"on skeleton + new Paraglide keyperson_mention_loading(de/en/es)6dd60571558e1e6b--c-accent60% to--c-ink50% (visible on every brand surface)060a1149setTimeout(r, N)waits replaced withvi.waitFor(...)(deterministic, CI-safe)515fa030bc58d77fConcerns deferred (with reason)
docs/adr/directory exists in this repo today. Deferred to whoever sets up the ADR convention.TranscriptionReadView.svelte.test.ts:373-391); writing a full e2e variant requires admin-delete page-object work that's outside the PR-B2 scope. Documented at the component layer.accessibility.spec.tsonce the PR ships and we add hover/focus state to that suite. The aria-label/aria-busy/role=status fixes already address the actual axe finding.CHILD_OFdirectionality. The relationship endpoint returns the canonical relationship rows; if the backend ever adds inverse directionality, the chip filter is the right place to widen. Out of scope for this PR.data-person-deletedclick feedback. Considered: when the link is tombstoned, falling through to the default anchor would 404 — that's a server-side concern. Aria-label already names the region; the click silently doing nothing is consistent with disabled-link semantics. Punt to a follow-up if user reports surface.@media (hover: none)already suppresses on touch devices, and the new viewport clamp prevents overflow on mouse-driven 320 px viewports (rare). Visual regression for this corner case is over-engineering.Ready for re-review.
Markus Keller — Application Architect
Verdict: Approved
Cycle-2 re-review of
bc58d77f. I previously flagged the lack of named layout constants, the magic numbers in the position math, the missing pure unit oncomputeCardPosition, the duplicated mention-selector string, and the type duplication betweenTranscriptionReadViewandPersonHoverCard. All five are now resolved:hoverCardPosition.tsis a pure function with named exportsCARD_WIDTH_PX / CARD_HEIGHT_PX / CARD_GAP_PX / BOTTOM_BAND_RATIO / RIGHT_FLIP_THRESHOLD_PXand a cleanViewport/CardPositioncontract — exactly the seam I asked for.PERSON_MENTION_SELECTORis centralised inmention.tsand consumed byTranscriptionReadView— single source of truth.LoadStateandHoverDatalive inlib/types/personHoverCard.tsso the orchestrator and the view share one canonical shape.loadHoverData(pure) is split fromgetOrFetchHoverData(cache wrapper) — this is the right factoring; the cache is now a one-liner that's trivial to swap for a per-document or global store later if staleness becomes a problem.Layer boundaries hold:
TranscriptionReadVieworchestrates state and side effects,PersonHoverCardis a pure view ofLoadState, the pure utilities (renderTranscriptionBody,computeHoverCardPosition) hold the algorithmic logic. Nothing in this PR violates the controller→service→repo equivalent on the frontend (component→state→fetch).Nothing left from my list. LGTM to merge.
Felix Brandt — Senior Fullstack Developer
Verdict: Approved
Re-review of
bc58d77fagainst my cycle-1 list. Everything I flagged is now landed as a discrete commit with a TDD trail:488d4384—SafeHtmlbrand on the renderer return type.renderBlockHtmlreturnsSafeHtmland{@html}is the only consumer. Compile-time guarantee that we never accidentally{@html block.text}raw input. Clean.26519d02— UUID validator at the renderer boundary. Open-redirect surface closed for any future "external person" feature, withit('skips substitution when personId is not a UUID')andit('skips substitution when personId is an absolute URL')pinning the contract.1842e23c—PERSON_MENTION_SELECTORconstant. The CSS class name lives in exactly one TS file now.060db691—computeHoverCardPositionextracted, 8 unit tests covering default placement, flip-up, flip-left, both, viewport clamp, scroll offsets. All branches reachable without DOM.5890bb3a—loadHoverData/getOrFetchHoverDatasplit. Cache wrapper is 4 lines; the pure load is testable in isolation.6a6967d8—LoadState/HoverDatahoisted intolib/types/personHoverCard.ts. No more drift risk between orchestrator and view.3faac135—isPlainPrimaryClickguard. Modifier-click + middle-click fall through to the browser; tests assert the click event was not preventDefault'd. This is exactly the behaviour I expect from a serious anchor.3365f584— focusin/focusout symmetry with mouseenter/mouseleave. Keyboard parity. The capture flag is correctly limited to the non-bubbling mouse events.558e1e6b— word-boundary truncation with the 70%-min-boundary fallback. Robust against single-long-token edge case.renderBlockHtmlcomposessplitByMarkersandrenderTranscriptionBody— markers stay siblings, never nested inside an anchor (B19b). Theas SafeHtmlcast on the marker branch is acceptable becausesplitByMarkersonly emits literal[unleserlich]/[...]strings.The code is small, named, easy to reason about. No nested conditionals, no dead code. Tests read as sentences. Nothing left for me to flag — ship it.
Tobias Wendt — DevOps & Platform Engineer
Verdict: Approved
Frontend-only PR. I checked it through the operational lens:
renderTranscriptionBody, position math, hover card all in-tree).npm run test:e2eand re-use the existingSTORAGE_STATEfixture..svelte-kit.old/is correctly listed in both.prettierignoreandeslint.config.js. Local dev artefact only — does not enter the build./api/persons/{id}and/api/persons/{id}/relationshipson hover. Per-component cache means a 20-mention transcription costs at most 2 backend round-trips per unique person, not 40. With the existing rate of transcribed documents this stays well under any sensible request budget.beforeAlland tears them down inafterAll. Sara's uniquified title (Date.now()suffix) means a crashed run can't poison the next one.Nothing operational to flag. LGTM.
Elicit — Requirements Engineer
Verdict: Approved
Cycle-2 re-check against the issue #362 acceptance criteria and my cycle-1 NFR / open-question list. Read mode behaviour is now fully specified, tested, and accessible:
Functional acceptance criteria (issue #362, B-series)
renders mention link AND [unleserlich] marker correctly when both occur in the same block.expect(personFetches.length).toBe(1)test after multi-hover sequence.degrades to plain unlinked text when the person fetch returns 404covers the deleted-person path.NFRs / open questions I flagged
displayName: deterministic first-sidecar-wins, asserted in unit test.role="region",aria-live="polite",aria-labelalways populated (loaded → displayName, loading → localised "Lade Person…"),aria-busy="true"while loading and cleared on loaded so SR announces the new content. Keyboard parity via focusin/focusout.person_mention_loadingkey inde.json,en.json,es.jsonkeeps the loading region's accessible name localised.The product behaviour matches the spec. No requirements gaps left for me to flag. Ready to merge.
Nora "NullX" Steiner — Application Security
Verdict: Approved
Pen-tester re-pass on
bc58d77f. The XSS surface I dug into in cycle 1 is now belt-and-braces:Stored-XSS via block text or sidecar
displayNamerenderTranscriptionBodycallsescapeHtmlon the full text before mention substitution, so<script>,<img onerror=…>, and entity-encoded payloads land as text. Tested explicitly (escapes < and >,escapes & in plain block text,escapes <img onerror=...>,does not double-encode HTML-entity-already-encoded payloads).displayNameis double-escaped: once in theescapedblock-text pass, then again viaescapeHtml(mention.displayName)for the link text and thedata-person-idattribute. The unit testescapes quotes in displayName so they cannot break the href attributepins the attribute-injection guard —O"Brienarrives asO"Brienand cannot terminate thehrefearly.Open-redirect /
javascript:URL injection on the anchorhref26519d02adds the strict UUID v1–v5 regex (isUuid) and rejects any non-UUIDpersonIdbefore substitution. Testsskips substitution when personId is not a UUIDandskips substitution when personId is an absolute URLconfirmjavascript:alert(1)andhttps://evil.example/...both fall through as plain text. CWE-601 surface closed.{@html}audit trailSafeHtml, and{@html renderBlockHtml(block)}is the only consumer inTranscriptionReadView. The eslint disable comment cites the renderer guarantee. A future refactor that tries to{@html block.text}will fail to typecheck — exactly the seam I asked for.Click handling
goto()only fires on a plain primary click. Anchor href is a same-origin path (/persons/{uuid}) — notarget="_blank", norel="noopener"issue.data-person-deleted;handleMentionClickpreventDefaults further navigation. No way to phish through a tombstoned mention.Telemetry / data exposure
/api/persons/{id}+/api/persons/{id}/relationships. Both routes already enforceREAD_ALLserver-side per existing security config — out of scope for this PR.I have nothing left to escalate. Approving.
Sara Holt — QA Engineer & Test Strategist
Verdict: Approved
Re-review of the test layer at
bc58d77f. Every concern from cycle 1 is gone:setTimeoutwaits replaced withvi.waitForinTranscriptionReadViewtests (515fa030). No more 50ms-vs-real-fetch races; the failure mode in CI is now "card never appears" not "we waited 50ms and might have missed it."expect(touchPage.getByTestId('person-hover-card')).toHaveCount(0)before the tap so the test actually proves touch-suppression worked, not just that a navigation happened.Date.now()— a previous crashed run that skippedafterAllcleanup can no longer collide with the next run.computeHoverCardPositionhas 8 unit tests covering default, flip-up, flip-up via bottom-band ratio, flip-left, no-flip-left, viewport clamp, negative-coord clamp, scroll offsets. All four placement branches are reachable.renderTranscriptionBodycovering empty input, plain text, entities, mention substitution, longest-first, word boundary, first-sidecar-wins, XSS via<script>,<img onerror>, double-encoded entities, quote-in-displayName, UUID-only personId guard.PersonHoverCardcovering all three states, family chip filter, alias rendering, life-date rendering, notes truncation (both single-long-word and word-boundary cases), aria-live/busy/label/id/position.Test-pyramid shape is right: lots of fast pure unit tests on
mention.tsandhoverCardPosition.ts, a smaller layer of vitest-browser tests on the components, and 4 Playwright specs anchoring the user-visible behaviour. No Playwright over-coverage.Test names read as sentences, AAA structure throughout.
cleanup()inafterEachis in place so cross-test DOM state cannot leak.I confirmed the two pre-existing failures on main (
RichtlinienRuleCard.svelte.spec.ts,hilfe/transkription/page.svelte.spec.ts) are not regressions from this PR. PR-specific tests are 113/113 green.Nothing left from my list. Ship it.
Leonie Voss — UX & Accessibility
Verdict: Approved
Cycle-2 audit on
bc58d77f. All five UX/A11y findings I raised in cycle 1 are now resolved:mounts the hover card on focusin so keyboard users see the previewpins it.role="region"without an accessible name:aria-labelis always populated —state.person.displayNamewhen loaded, the localisedm.person_mention_loading()while loading, never empty. axe-core will not flag this region. Cycle-1 commit6dd60571plus tests.aria-busytoggle for SR announcement:aria-busy="true"while loading; cleared on loaded so the new content is announced via the polite live region. Testdoes not set aria-busy when loaded (so the loaded content is announced)confirms.truncateAtWordBoundarycuts at the last space within a 70%-min-boundary window, falls back to a hard cut for single-token notes. German compounds no longer get amputated mid-syllable (…Familienzu…). Test pinning both branches.lefttoviewportWidth - CARD_WIDTH_PX - CARD_GAP_PXand never returns negatives. Spec'd inhoverCardPosition.spec.tswithviewportWidth: 400regression.text-decoration-color: color-mix(--c-ink, 50%)instead of the previous mint-at-60%. The link affordance is visible at rest on every surface tone in the brand palette and meets WCAG AA. Hover bumps to--c-accentand 2px thickness for the affordance signal.Brand fidelity holds:
--c-inkfor text,--c-accent-bgfor chips,--font-seriffor the name,--font-sanseverywhere else, navy header. Skeleton respectsprefers-reduced-motion. Touch suppression via@media (hover: none).I'm done. Approved.
Post-merge review→fix loop — Final Summary
Loop completed in 2 cycles of multi-persona review.
Cycle 1 — fixes implemented from initial review
13 atomic commits (
6a6967d8…bc58d77f) addressing every blocker and concrete suggestion raised in the first review pass:LoadState/HoverDatatype duplication between orchestrator and view6a6967d8{@html}guard488d4384personIdcould land inhref26519d02.person-mentionselector duplicated across CSS / TS / tests1842e23ccomputeCardPositionnot unit-testable (DOM-bound, magic numbers)060db691fetchHoverDatamixed pure load with cache wrapper5890bb3a3faac1353365f584role="region"lacked an accessible name; noaria-busytoggle6dd60571558e1e6b060a1149setTimeoutwaits in unit tests515fa030bc58d77fCycle 2 — re-review of
bc58d77fAll six personas approved without further changes:
Final state
RichtlinienRuleCard.svelte.spec.ts,hilfe/transkription/page.svelte.spec.ts) are pre-existing onorigin/mainand unrelated to this PR.main.Ready to merge.