feat: Person @mentions in transcription blocks with hover card #362
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Overview
Transcribers can tag historical persons directly in transcription block text using an
@mentiontypeahead (analogous to the existing user mention in comments). In read mode, tagged names render as underlined links. On desktop, hovering shows a rich popup card (Wikipedia-style); clicking — or tapping on touch — navigates to the person detail page.Interaction model
@/api/persons?q=...)@Display Nameinserted into text; UUID stored in sidecar (never in text)/persons/{id}/persons/{id}directly (no hover card on touch)The
@prefix is stripped in read mode — the link reads "Auguste Raddatz", not "@Auguste Raddatz", since the@is an editor affordance, not part of the historical text.Data model
New JSONB column on
transcription_blocks:Each entry in the array:
displayNameis whatrenderTranscriptionBody()scans the block text for.personIdis what the link and hover card resolve to. The UUID never appears inblock.text.The GIN index supports the future "show all documents where person X is mentioned" query on the person detail page (
WHERE mentioned_persons @> '[{"personId":"..."}]'). A normalised join table can be added at that point if queries get complex — not needed now (YAGNI).Backend changes
1.
TranscriptionBlockentityNew value object (no table, no repository):
2. Save/update DTO
CreateTranscriptionBlockDTO(and any update DTO) gets:3.
PersonServicename-change hookWhen a person's
displayNamechanges,PersonServicecallsTranscriptionBlockService.propagateDisplayNameChange(UUID personId, String oldName, String newName):mentioned_persons @> [{"personId": "..."}](GIN-indexed)text.replace("@" + oldName, "@" + newName)displayNamein the JSON entry@Transactional4. No new endpoints
Hover card data uses two existing endpoints already in the codebase:
GET /api/persons/{id}— name, birthYear, deathYear, notes, nameAliasesGET /api/persons/{id}/relationships— direct relationships (filtered client-side toPARENT_OF,SPOUSE_OF,SIBLING_OFonly; no inferred/derived relationships)Frontend — edit mode
New
PersonMentionEditor.svelte— thin variant ofMentionEditor.svelte, three differences:/api/persons?q=...(not/api/users/search?q=...)selectPerson()writes@DisplayNameinto the textarea text and pushes{personId, displayName}into the boundmentionedPersonsarray (analogous tomentionCandidatesinMentionEditor)The textarea remains a plain
<textarea>— nocontenteditable, consistent with the decision made for comment mentions. Users never see a UUID at any point.Frontend — render mode
renderTranscriptionBody(text, mentionedPersons)New function alongside the existing
renderBody()inmention.ts. For each entry inmentionedPersons, replaces@DisplayNamein the text with:Names typed manually without the typeahead are not in the sidecar and remain as plain text. This is intentional — you must explicitly use the typeahead to create a mention.
PersonHoverCard.sveltemouseenterPARENT_OF,SPOUSE_OF,SIBLING_OF), omitted section if none@media (hover: none)) — tap navigates directlyTranscriptionReadView.svelterenderTranscriptionBody()output as{@html ...}(same pattern as current comment rendering)mouseenter/mouseleavehandlers to.person-mentionbuttons to mount/unmountPersonHoverCardclickhandler on.person-mention→goto('/persons/{personId}')Edge cases
@Nameremains visible in the block as a historical trace@Namewithout typeaheadAcceptance criteria
Given a transcriber is editing a transcription block and types
@AugWhen results appear in the typeahead
Then each result shows the person's display name and life dates, and the matched characters are highlighted
Given a transcriber selects a person from the typeahead
When the block is saved
Then
block.textcontains@Auguste Raddatz(no UUID) andblock.mentionedPersonscontains{personId, displayName}for that personGiven a reader views a transcription block in read mode on desktop
When they hover over a person mention
Then a hover card appears within 200ms showing name, life dates, maiden name (if any), family relationship chips (family-type only), and a notes excerpt (if any)
Given a reader clicks a person mention
When on any device
Then they are navigated to
/persons/{id}Given a reader views a transcription block on a touch device
When they tap a person mention
Then they are navigated directly to
/persons/{id}(no hover card shown)Given a person's display name is updated in the system
When the update is saved
Then all transcription block texts containing
@OldNamefor that person are updated to@NewName, and thedisplayNamein each block'smentionedPersonssidecar is updated atomicallyGiven the person referenced by a mention has been deleted
When a reader hovers over the mention
Then no hover card appears and the text renders as plain unlinked text
Open questions
displayNamementioned in the same block — rendering is ambiguous. Acceptable limitation for now?🏗️ Markus Keller — Senior Application Architect
Observations
Cross-domain coupling is inverted. The spec has
PersonService.propagateDisplayNameChange()callTranscriptionBlockService. In the existing architecture, Document depends on Person — that direction is natural (document needs person metadata). But Person depending on Transcription inverts the dependency arrow: Person domain now has a runtime dependency on the Transcription domain. Person records are agnostic of where they are used; they should not import a Transcription service.@Type(JsonType.class)requireshypersistence-utils-hibernate63, which is not in pom.xml. Before adding a new library, note thatUserGroup.javaalready uses@ElementCollection(fetch = FetchType.EAGER)for itsSet<String> permissions— native JPA, no external library.PersonMentioncan be modeled the same way with a@Embeddablevalue object and@ElementCollection. The schema result is similar (a child tabletranscription_block_mentioned_persons), avoids the dependency, and is already an established pattern in this codebase.JSONB vs join table design divergence.
DocumentCommenttracks user mentions via a@ManyToMany comment_mentionsjoin table. This spec uses JSONB for person mentions. The divergence is defensible — you will needWHERE mentioned_persons @> [{personId:...}]queries for the person detail page, which JSONB + GIN handles well. But the schema design choice should be documented in the migration comment, since future devs will see two completely different patterns for "this entity has mentions."GIN index is correct forward planning. The
WHERE mentioned_persons @> '[{"personId":"..."}]'query on the person detail page is the natural follow-on, and the GIN index makes it O(log n). Keep it.Recommendations
Decouple via
ApplicationEvent.PersonServicepublishes aPersonDisplayNameChangedEvent(personId, oldName, newName)viaApplicationEventPublisher. APersonMentionPropagationListenerin the transcription package handles it with@EventListener @Transactional. Person domain has zero runtime dependency on Transcription. Spring's event bus is synchronous by default, so the whole operation still commits atomically in one transaction.Prefer
@ElementCollectionover JSONB + hypersistence-utils unless there is a concrete reason to store JSON in PostgreSQL now (e.g., you need arbitrary nested structure).@Embeddable PersonMention { UUID personId; String displayName; }with@ElementCollectionon the block entity works without a new dependency. If the future "query all blocks mentioning person X" query turns out to need a GIN index, add the hypersistence dependency at that point with full justification.Add a migration comment explaining why this table uses JSONB/ElementCollection rather than a join table — specifically the "future bidirectional query" motivation. Future devs reading the schema need to understand why this differs from
comment_mentions.👨💻 Felix Brandt — Senior Fullstack Developer
Observations
CRITICAL:
detectMention()breaks on multi-word person names. I checkedfrontend/src/lib/utils/mention.ts:23:Once the user types a space, mention detection is killed. For user mentions (firstName only), this is fine —
@Maxtriggers and the full name is inserted. For person mentions, display names ARE multi-word:@Auguste Raddatz,@Marie von Braun. The user can type@Augand get results, but if they type@Auguste(with space, wanting to narrow by last name), detection stops and the dropdown disappears.PersonMentionEditormust use a customdetectMention()variant that allows spaces in the query — e.g., stop only at@@or at the start of a new word preceded by a non-name character.@Type(JsonType.class)dependency is missing.hypersistence-utils-hibernate63is not inpom.xml. The codebase already uses@ElementCollectioninUserGroup.javafor a similar need (a set of embedded values). Use@Embeddable PersonMention { UUID personId; String displayName; }+@ElementCollection— same semantics, no new library, established pattern.renderTranscriptionBody()must HTML-escapedisplayNamebefore interpolating into HTML.renderBody()atmention.ts:54-69shows the correct pattern: escape the full content first, then also escapedisplayNameseparately before building the span. If a person'sdisplayNameis<script>alert(1)</script>(set by any WRITE_ALL user), every reader viewing that block gets XSS. The fix is one additionalreplaceAllpass ondisplayName— identical to whatrenderBody()already does.OQ-1 has a concrete implementation problem. Two persons with the same
displayNamein one block:renderTranscriptionBody()with simplereplaceAllwould map ALL occurrences of@Auguste Raddatzto the firstpersonIdin the sidecar. The second sidecar entry never links to anything. The spec marks this as acceptable, but the implementation should be explicit about the tie-breaking rule (first entry wins) so it's not accidentally "fixed" later in a way that breaks the semantics.Recommendations
Write a
detectPersonMention()variant in a newpersonMention.tsutility (or parameterise the existing function) that allows spaces in the query: stop detection at@boundaries or newlines only. Test it with:@Auguste Raddatz,@M(single char),@Auguste(trailing space), and@alone.Use
@ElementCollection+@EmbeddableforPersonMention. LookupUserGroup.javafor the exact annotation pattern already in use.In
renderTranscriptionBody(), escapedisplayNameexactly likerenderBody()does for its display names (lines 62–66 ofmention.ts). Copy that four-line block verbatim.In
renderTranscriptionBody(), use<a href="/persons/${mention.personId}" class="person-mention">instead of<button>. This gives browser-native affordances (right-click → open in new tab, middle-click) and semantically correct role=link for screen readers. Thehrefcan be pre-computed at render time since allpersonIdsare in the sidecar.🔐 Nora "NullX" Steiner — Application Security Engineer
Observations
XSS in
renderTranscriptionBody(). The spec generates<button class="person-mention" data-person-id="{uuid}">{DisplayName}</button>where{DisplayName}comes from the sidecar (persisted in the database by the saving client). Any user withWRITE_ALLpermission can create a person named<img src=x onerror=alert(document.cookie)>. Every reader who views any transcription block mentioning that person then executes attacker-controlled JavaScript. The existingrenderBody()function (mention.ts:54–69) already shows the exact correct pattern: escape the full content string first, then separately escapedisplayNamebefore interpolating it. The new function must follow the same pattern.mentionedPersonsDTO input validation is unspecified. The spec addsList<PersonMention> mentionedPersonsto the DTO but does not constrain it. A client can submit: (a) malformed/non-existent UUIDs aspersonId, (b) extremely longdisplayNamestrings. Recommend:@Size(max = 200)ondisplayName(matches thePerson.displayNamefield length) and@NotNullonpersonId. Validating that the UUID references a real person at save time is optional but prevents dangling sidecar entries.Historical text mutation in
propagateDisplayNameChange(). This method doestext.replace("@" + oldName, "@" + newName)on stored historical transcription data. A bug here (wrong match, partial name collision) permanently corrupts original source material. The@Transactionalwrapper correctly ensures atomic rollback on failure. Confirm: the transaction includes both thementionedPersonsJSONB update AND thetextfield update in a single commit — a partial update where text changes but sidecar doesn't (or vice versa) would break the rendering invariant.Client-side hover card fetches.
PersonHoverCard.sveltefetches/api/persons/{id}and/api/persons/{id}/relationshipsfrom the browser. The Vite proxy (vite.config.ts) injects theauth_tokencookie header automatically for/apicalls in dev. In production (behind Caddy), the session cookie is forwarded normally. No authentication bypass issue here, but confirm thatGET /api/persons/{id}requires at minimumREAD_ALL(the existingPersonControllerlikely gates on authentication already — worth verifying in the controller source).Recommendations
Copy the four-line HTML escape block from
renderBody()(mention.ts:62–65) intorenderTranscriptionBody()fordisplayName. Zero tolerance for this class of bug — it affects every user viewing the page.Add to
PersonMentionvalue object (DTO layer):@NotNull UUID personIdand@Size(max = 200) String displayName.Add a security regression test: save a transcription block with
mentionedPersons: [{personId: ..., displayName: "<script>alert(1)</script>"}]. Render it withrenderTranscriptionBody(). Assert the output contains<script>(escaped), not<script>(raw).Confirm
PersonController.GET /api/persons/{id}is authentication-gated. If not, the hover card would leak person details (names, birthdates, notes) to unauthenticated callers who fetch the endpoint directly.🧪 Sara Holt — Senior QA Engineer
Observations
propagateDisplayNameChange()has no test plan in the spec, and it is the highest-risk method in this feature. It mutates stored historical transcription text across an arbitrary number of rows. A bug here silently corrupts primary source material. This method needs integration tests with Testcontainers before any production deploy.String.replace("@" + oldName, "@" + newName)matches manually-typed occurrences too. The spec explicitly states: "Names typed manually without the typeahead are not in the sidecar and remain as plain text." But propagation uses a text search that finds all@OldNameoccurrences regardless of whether they came from the typeahead. So renaming "Auguste Raddatz" → "Augusta Raddatz" updates manually-typed@Auguste Raddatztoo — contradicting the "only explicit typeahead mentions are links" invariant.detectMention()space restriction breaks multi-word name queries. The function returnsnullas soon as the query contains a space (mention.ts:23). This is a functional gap for person names. If not discovered in testing before release, transcribers will find that typing@Auguste R(partial last name) makes the dropdown disappear. This needs a specific test case.Missing acceptance criteria:
MentionEditorsupports this; the spec is silent on whetherPersonMentionEditorinherits it.@SamePersonin one block text — are all linked? Just the first?Recommendations
Minimum test plan for
propagateDisplayNameChange():@OldName— verify all texts and all sidecar entries updated atomically.@OldNametwice — verify both replaced.For
detectMention(): Either (a) write aPersonMentionEditor-specific detector that allows spaces and stops only at paragraph breaks/double-spaces, or (b) define the UX constraint explicitly in the spec: "To search by last name, transcribers must type the full first name first and select from the filtered list." Add an AC covering this.Define hover card network error behavior (probably: silently show no card, do not navigate). Add to edge cases table and cover in
PersonHoverCardunit tests.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Observations
<button>for navigation is semantically wrong. The spec renders mention links as<button class="person-mention" data-person-id="{uuid}">. Buttons are for actions. Navigation is the job of<a href>. Screen readers announce<button>as "Auguste Raddatz, button" — confusing, because nothing is being submitted. An<a href="/persons/{id}">announces "Auguste Raddatz, link" — exactly what the interaction is. Additional benefit: native browser affordances (right-click → open in new tab, middle-click, Ctrl+click) work without extra code. SincepersonIdis available in the sidecar at render time, the href can be computed inrenderTranscriptionBody()directly.Hover card viewport overflow is unspecified. "Positioned absolutely near the mention button" — if the mention is in the last line of a block near the bottom edge, or at the right margin, the card clips off-screen. This is especially bad for the senior audience on 1280px screens with the reading pane centered. The spec should define a fallback: prefer below-right, flip to above-right if <200px from viewport bottom, flip to left if <300px from viewport right-edge.
"Klick öffnet Seite" and "Zur Person →" are hardcoded German. The app supports de/en/es via Paraglide. These two strings need i18n keys:
m.person_mention_hover_footer_hint()andm.person_mention_hover_open_link()(or equivalent). Missing keys means English and Spanish readers see German.No focus ring specified for mention links. Keyboard users Tab through the page and must see focus position. Every mention link needs
focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2 outline-none rounded-sm. Since these links are rendered via{@html}, the focus ring styling must come from a CSS class inmention.cssorglobal.cssthat targets.person-mention:focus-visible.The 200ms AC is ambiguous. "Then a hover card appears within 200ms" — does this mean the skeleton appears, or the loaded card? With a lazy fetch, the loaded card could take 500–2000ms. The skeleton should appear within 200ms of
mouseenter; the loaded content appears when the fetch resolves. The AC should say "the skeleton state appears within 200ms" to be testable.Recommendations
Change
<button>→<a href="/persons/{id}" class="person-mention">inrenderTranscriptionBody(). No onclick handler needed; the href handles navigation. For the hover card, attach themouseenter/mouseleaveto the anchor element instead.Add smart card positioning: use
getBoundingClientRect()on the trigger element and flip the card if it would overflow. Floating UI (@floating-ui/dom, already a SvelteKit-compatible library) handles this with two lines of code. Alternatively, specify the flip rules explicitly so the developer can implement them without a library.Add Paraglide keys for both hover card strings before the i18n lock. Add to
messages/de.json,en.json,es.json.Add to
app.cssor equivalent:.person-mention:focus-visible { outline: none; ring: 2px solid var(--color-ink); }(or the Tailwind@apply focus-visible:ring-2equivalent).🚀 Tobias Wendt — DevOps & Platform Engineer
Observations
No infrastructure changes required. This feature is entirely application-level — no new Docker services, no new environment variables, no Compose changes. The existing PostgreSQL instance, MinIO, and Spring Boot container are all unchanged.
hypersistence-utils-hibernate63is not inpom.xml. If the JSONB /@Type(JsonType.class)approach is chosen, this library must be added with a pinned version (e.g.,3.9.0). Renovate will pick it up for future bump PRs automatically once it is pinned. If the@ElementCollectionalternative is used instead (no JSONB), this step is skipped entirely.Flyway migration safety. The proposed migration adds
mentioned_persons JSONB NOT NULL DEFAULT '[]'::jsonbtotranscription_blocksand creates a GIN index. Adding aNOT NULL DEFAULTcolumn to a table with existing rows is safe on PostgreSQL 11+ — it's a metadata-only operation (no full table rewrite). Creating a GIN index on a new (empty) column is near-instant. Both operations are production-safe as written.OpenAPI regeneration is a required step after merge. The
mentionedPersonsfield is added to the entity and at least one DTO. After merging the backend changes, the frontend developer must runnpm run generate:api(with the backend running with--spring.profiles.active=dev) before writing any typed frontend code. If this step is skipped, the TypeScript types will be stale and type errors will appear only at compile time.Propagation latency under load.
propagateDisplayNameChange()runs synchronously within thePUT /api/persons/{id}request, holding the transaction open for the full duration. At family-archive scale (hundreds to low thousands of transcription blocks), this is fine — GIN-indexed JSONB lookup is fast. Worth noting if the archive grows significantly.Recommendations
If
hypersistence-utilsis added: pin to a specific version tag inpom.xml, add a comment explaining the choice (JSONB querying for future person-detail page feature), and verify that Renovate's Maven configuration covers it.Add a comment to the Flyway migration file:
-- Adding GIN index is safe on new column (zero existing rows); NOT NULL DEFAULT is metadata-only on PG 11+.This spares the next person from re-checking migration safety.No other DevOps action items on this feature. Clean ticket from an infra perspective.
📋 Elicit — Requirements Engineer
Observations
detectMention()space constraint is an unspecified UX gap. The existingmention.tsfunction kills detection as soon as the query contains a space. For user mentions, this is fine (first name only). For persons named "Auguste Raddatz", "Marie von Braun", etc., the transcriber CAN type@Augand get suggestions, but CANNOT type@Auguste Rto narrow by last name — the dropdown disappears on the space. This constraint is not in the spec and has no AC. Either it is an accepted limitation (add to the edge-cases table with explicit behavior), orPersonMentionEditorneeds a modified detector (add to backend changes / frontend changes section).OQ-1 describes a limitation but not the actual behavior. "Two different persons with the same displayName mentioned in the same block — rendering is ambiguous. Acceptable limitation for now?" This is marked Low and acceptable, but it leaves the behavior unspecified for the implementer. With
renderTranscriptionBody()usingreplaceAll, the rendering outcome is deterministic: the first entry inmentionedPersonswith that displayName links all occurrences; subsequent entries with the same name produce dead sidecar data. This should be stated explicitly rather than left as "ambiguous."Missing NFR for person rename latency.
propagateDisplayNameChange()runs synchronously in the rename request. No NFR defines what "acceptable" response time is. Without this constraint, the implementation has no target and future optimizations have no baseline.Missing AC for save failure with pending mentions. The spec covers the happy save path and the deleted-person degradation case, but not: "Given a transcriber has selected a person from the typeahead, when saving the block fails (network error or conflict), then the editor retains the pending mention and displays an error." This failure path is important because losing a sidecar entry silently means the mention degrades to plain text with no visible change.
"Appears within 200ms" is ambiguous in AC. The acceptance criterion reads: "Then a hover card appears within 200ms." With a lazy fetch, the loaded card could take seconds. The intent is clearly that the skeleton state appears within 200ms. The AC should read: "Then a hover card skeleton state appears within 200ms of hover."
Recommendations
Add to the edge-cases table:
@Name typed after space to narrow by surname → detection stops, dropdown disappears; transcriber must retype from @. Or add toPersonMentionEditorspecification: support multi-word queries.Replace OQ-1 with a deterministic behavioral statement: "Where two sidecar entries share the same displayName, the first sidecar entry takes precedence for all text occurrences; the second entry produces no link."
Add NFR: "The person rename operation (including mention propagation across all blocks) must complete within 3 seconds for archives with up to 2,000 transcription blocks."
Add AC: "Given a transcriber selects a person from the typeahead and the block save fails, When the error is shown, Then the mention remains in the editor's pending state so the transcriber can retry without re-searching."
Amend the 200ms hover card AC to specify "skeleton state appears" rather than "hover card appears."
🗳️ Decision Queue — Action Required
5 decisions need your input before implementation starts.
Architecture
Domain coupling: direct call vs. ApplicationEvent.
PersonServicecallingTranscriptionBlockService.propagateDisplayNameChange()directly creates a Person→Transcription dependency. Alternative:PersonServicepublishes aPersonDisplayNameChangedEvent; a listener in the transcription package handles propagation. The event approach keeps Person domain agnostic of Transcription; both are synchronous and transactional by default. Cost of direct call: adds a compile-time dependency, Person service must be updated whenever transcription block storage changes. Cost of event: one extra class (the listener), slightly less obvious call chain. (Raised by: Markus)JSONB +
hypersistence-utilsvs.@ElementCollection+@Embeddable. The spec assumes@Type(JsonType.class)which requires addinghypersistence-utils-hibernate63(not currently in pom.xml). The codebase already uses@ElementCollectioninUserGroup.javafor an embedded value collection — same pattern works forPersonMention. Cost of JSONB: new library dependency, simpler future GIN-query for person-detail page. Cost of@ElementCollection: child table instead of JSONB column, GIN index cannot be applied directly (needs a join query instead), no new library. If the "show all documents where person X is mentioned" query is a near-term feature, JSONB wins. If it's speculative,@ElementCollectionwins. (Raised by: Markus, Felix, Tobias)UX / Frontend
<button>vs.<a href>for rendered mention links. The spec uses<button class="person-mention">for click-to-navigate. Semantically, navigation belongs on<a href="/persons/{id}">(role=link, browser affordances, screen reader announcement). Using<a>withhrefpre-computed at render time requires passingmentionedPersonstorenderTranscriptionBody()— which is already the case. No open implementation cost for switching; the only constraint is thatrenderTranscriptionBody()generates<a href>instead of<button>. (Raised by: Leonie, Felix)Behavioral / Requirements
Multi-word name detection: accept limitation or fix it?
detectMention()inmention.ts:23returnsnullthe moment the query contains a space. A transcriber typing@Auguste R(partial last name) loses the dropdown. Options: (a) accept — add to edge-cases table ("type the first name or a fragment, select from results; you cannot narrow by last name mid-query"); (b) fix — write adetectPersonMention()variant that allows spaces and stops only at@@or newline. Cost of accepting: some transcribers will be confused when the dropdown disappears. Cost of fixing: ~20 lines of TypeScript, one utility test. (Raised by: Felix, Sara, Elicit)Propagation scope for manually-typed
@OldName.propagateDisplayNameChange()usesString.replace("@" + oldName, "@" + newName), which matches ALL occurrences of@OldNamein the text — including ones typed manually (not through the typeahead). The spec says manually-typed names are plain text and not links, but this propagation would still rename them. Options: (a) accept — propagation renames all@OldNameoccurrences, linked or not (simpler implementation); (b) restrict — only rename occurrences that have a matching sidecar entry (requires offset tracking or a more complex algorithm). Cost of accepting: manual text containing@OldNameis silently renamed. Cost of restricting: significantly more complex propagation logic. For a family archive with controlled authorship, (a) is likely acceptable. (Raised by: Sara, Elicit)🏗️ Markus Keller — Architecture Discussion Follow-up
Four open architectural items discussed and resolved. Decisions below supersede the open options in the Decision Queue for the architecture-relevant items.
✅ Item 1 — Domain coupling: ApplicationEvent approach adopted
Decision:
PersonServicepublishesPersonDisplayNameChangedEvent(personId, oldName, newName)viaApplicationEventPublisher. APersonMentionPropagationListenerin the transcription package handles propagation with@EventListener @Transactional.Person domain has zero compile-time dependency on Transcription. Spring's default synchronous event dispatch means the entire operation (rename + propagation) still commits in one transaction — same atomicity guarantee as a direct call, with clean domain boundaries.
✅ Item 2 — Storage approach:
@ElementCollection+@EmbeddableDecision: Use
@Embeddable PersonMention { UUID personId; String displayName; }with@ElementCollectionon the block entity. Nohypersistence-utils-hibernate63dependency.Even with "show all documents where person X is mentioned" coming as the next feature, a join on an indexed
person_idB-tree column is equally fast as a GIN containment check — and avoids a new library to vet and maintain. The establishedUserGroup.javapattern applies directly.✅ Item 3 — Package placement: follow existing by-layer structure
Decision: New classes go into the existing layer packages —
model/PersonMention.java,service/PersonMentionPropagationListener.java,model/PersonDisplayNameChangedEvent.java. Do not introduce feature packages as an island within this feature. A migration to package-by-feature is a separate initiative. Worth noting a comment onPersonMentionPropagationListenermarking it as transcription-domain logic, so the intent survives a future restructure.✅ Item 4 — Synchronous propagation: correct choice at this scale
Decision: Keep propagation synchronous within the rename transaction (Spring
@EventListener, not@Async). Rename + propagation commit atomically or not at all — the right guarantee for historical text where silent partial corruption is unacceptable. At family-archive scale (worst case: ~150–200 block updates) this adds at most 100–300ms to the rename response. Async propagation would introduce partial-failure scenarios requiring retry/reconciliation logic that this scale does not justify. If the archive ever grows to tens of thousands of blocks, switch to@TransactionalEventListener(phase = AFTER_COMMIT)with@Async— a one-annotation change.Overall: the feature spec is sound. The three changes above (event decoupling, ElementCollection storage, synchronous propagation) make the implementation simpler and more consistent with the existing codebase — not more complex.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
UI/accessibility discussion — five open items from my earlier review, all resolved.
✅ Item 1 —
<button>→<a href>for rendered mention linksDecision:
renderTranscriptionBody()generates<a href="/persons/{id}" class="person-mention">instead of<button>. Thehrefis pre-computed from thementionedPersonssidecar at render time — no onclick handler needed.mouseenter/mouseleavefor the hover card attaches to the anchor element.Screen readers announce "Auguste Raddatz, link" (correct) instead of "Auguste Raddatz, button" (confusing). Browser-native affordances (Ctrl+click, middle-click, right-click → open in new tab) work without extra code.
✅ Item 2 — Hover card viewport overflow handling
Decision: Manual positioning with
getBoundingClientRect()— no new library dependency. Flip rules:✅ Item 3 — i18n keys for hover card footer strings
Decision: Add Paraglide keys before implementation — not after. Two keys required:
person_mention_open_link→ "Zur Person" (de), equivalent in en/esperson_mention_hover_hint→ "Klick öffnet Seite" (de), equivalent in en/esAdd to
messages/de.json,en.json, andes.jsonas part of this feature.✅ Item 4 — Focus ring and underline for
.person-mentionlinksDecision: Since mention links are injected via
{@html}, Tailwind utility classes cannot be applied inline. Two rules go in the global stylesheet (alongside any existing{@html}content styles):Underline in resting state is required: colour alone as the sole link differentiator fails WCAG AA for colour-blind users (8% of men).
✅ Item 5 — "200ms" acceptance criterion reworded
Decision: Replace:
With:
The original wording was untestable — with a lazy fetch the loaded card takes 500–2000ms. The intent is that the skeleton shimmer appears immediately; content follows asynchronously.
Overall: the feature interaction model is solid. With
<a href>links, explicit flip rules, and global focus styles, this will work for keyboard users and our senior audience on all viewport sizes. No blockers from the UX side.👨💻 Felix Brandt — Senior Fullstack Developer
Developer discussion follow-up — six implementation-level items resolved.
✅ Item 1 — Multi-word name detection: fix it
Decision: Write
detectPersonMention()in a newpersonMention.tsutility. Spaces are allowed inside the query; detection stops only at a newline or a second@. The existingdetectMention()inmention.ts(which stops at the first space) remains unchanged for user mentions.Test cases required:
@Auguste,@Auguste(trailing space),@Auguste Raddatz,@M,@alone.Accepting the limitation (option a) would silently break the typeahead for any archive with multiple persons sharing a first name — exactly the common case.
✅ Item 2 — HTML escaping in
renderTranscriptionBody()Decision: Extract a shared
escapeHtml(str: string): stringhelper inmention.ts. BothrenderBody()and the newrenderTranscriptionBody()call it. No duplication; one test covers both functions.renderBody()gets the fix retroactively — its current inline escaping is correct but fragile.✅ Item 3 ��� Propagation scope for manually-typed
@OldNameDecision:
propagateDisplayNameChange()replaces all@OldNameoccurrences in block text — linked or not. This is the defined behavior, not a bug. Stated explicitly here so it is not accidentally "fixed" later. For a family archive with controlled authorship, renaming a manually-typed@Auguste Raddatzwhen the person is renamed is the correct outcome.✅ Item 4 — OQ-1 tie-breaking rule
Decision: Replace the "ambiguous" label with a deterministic rule: where two sidecar entries share the same
displayName, the first sidecar entry takes precedence for all text occurrences; the second entry produces no link. This is the natural outcome ofreplaceAlland should be documented as intentional.✅ Item 5 — Hover card network error state
Decision: Three distinct states for
PersonHoverCard:mouseenterThe mention link navigates to
/persons/{id}in all states — the card is informational, the link always works.Add Paraglide key
person_mention_load_errortomessages/de.json,en.json, andes.json.✅ Item 6 — Save failure with pending mentions
Decision: On save failure, the editor retains both the textarea text and the
mentionedPersonsarray unchanged. The transcriber sees the error, fixes the cause, and retries without re-searching. No mention data is lost on a failed save.Overall: the spec is implementable with these six points locked. Architecture (Markus) and UX (Leonie) decisions from earlier comments stand. No blockers from the dev side.
👨💻 Felix Brandt — Implementation Plan
Decisions consolidated from the four discussion follow-ups (Markus, Leonie, Felix-dev, Nora). Plan is split into PR A — Backend then PR B — Frontend so the OpenAPI types regenerate cleanly between them. Branch and uncommitted-work cleanup happens before any code starts.
Pre-flight
feat/stammbaum-issue-358(RelationshipService.java + CreateRelationshipRequest.java) — commit or discard before branching for #362.git pull origin mainas a separate explicit step.git checkout -b feat/issue-362-person-mentions-backend.PR A — Backend
Each task = one failing test → minimum implementation → atomic commit.
V56__add_transcription_block_mentioned_persons.sql— block_id + person_id + display_name table, B-tree onperson_id. No FK onperson_idso deleted persons degrade to plain text per specPersonMention@Embeddablevalue object —UUID personId(@NotNull),String displayName(@Size(max=200)), Lombok +@Schema(REQUIRED)List<PersonMention> mentionedPersonstoTranscriptionBlockvia@ElementCollection(fetch = LAZY)+@CollectionTablePersonDisplayNameChangedEvent(personId, oldName, newName)inmodel/packageApplicationEventPublisherintoPersonService; inupdatePersoncapture olddisplayName, save, compare, publish if changedPersonServicepublishes event when display name changesPersonServicedoes not publish event when display name unchanged (e.g. notes-only edit)TranscriptionBlockRepository.findByMentionedPersons_PersonId(UUID)derived queryPersonMentionPropagationListener(transcription package, doc-commented as transcription-domain logic per Markus Item 3);@EventListener @Transactionaldoestext.replace("@" + old, "@" + new)and updates each matchingPersonMention.displayName, thensaveAll@Marieand@Marie Braunboth rewritten — intentional per Felix Item 3)CreateTranscriptionBlockDTO+UpdateTranscriptionBlockDTOacceptmentionedPersons; controller methods get@ValidTranscriptionService.createBlock/updateBlockpersist the sidecarupdateBlockpersistsmentionedPersonsfrom DTO@WebMvcTestslice — controller rejects oversizeddisplayName(>200) with 400./mvnw clean package -DskipTestsand./mvnw testgreenfrontend/, commitapi.tsfeat: person @mention sidecar in transcription blocks (backend)PR B — Frontend (after PR A merges)
escapeHtmlhelper inmention.ts;renderBodyuses it (Felix Item 2)personMention.ts—detectPersonMention(allows spaces, stops at\n/ second@) andrenderTranscriptionBody(longest-displayName-first,<a href="/persons/{id}" class="person-mention">DisplayName</a>, no@prefix)detectPersonMentioncases:@Auguste,@Auguste,@Auguste Raddatz,@M,@,@Aug\nfoo,@Aug@barrenderTranscriptionBodyHTML-escapes hostiledisplayName(XSS regression)@from rendered link textdisplayName(OQ-1 deterministic rule)person_mention_open_link,person_mention_hover_hint,person_mention_load_error(de/en/es).person-mentionglobal CSS — underline +:focus-visiblering inroutes/layout.cssPersonMentionEditor.svelte— mirrorsMentionEditor, hits/api/persons?q=…, dropdown shows name + life dates, bindablementionedPersons@DisplayNameand pushes sidecar entrymentionedPersons(Felix Item 6)<textarea>inTranscriptionBlock.svelte(lines 185–194) withPersonMentionEditor; threadmentionedPersonsthrough autosave toPUT /api/.../transcription-blocks/{blockId}mentionedPersonsPersonHoverCard.svelte— three states (loading skeleton ≤200ms, loaded with header/dates/maiden/relationship chips/notes excerpt/footer, generic error); 404 doesn't mount; manual flip viagetBoundingClientRect()(below-right default; flip up <200px from bottom; flip left <300px from right); hidden via@media (hover: none)PersonHoverCardloaded statePersonHoverCardnon-404 error statePersonHoverCarddoes not mount on 404TranscriptionReadView.svelte— render viarenderTranscriptionBody, compose with existing marker rendering, attach delegated hover handlers on.person-mentionnpm run check+npm run lint+npm run testfeat: person @mention typeahead, hover card, render integration (frontend)Risks I'm watching
renderTranscriptionBodyhas to coexist withsplitByMarkers([unleserlich],[…]→<em data-marker>). Naive ordering means either markers get HTML-escaped or mention HTML gets re-escaped. Implementation must split into marker-segments first and run mention substitution +escapeHtmlonly on text segments before re-joining.@ElementCollectionwith@Embeddableis new ground in this codebase (UserGrouponly uses it withSet<String>). The repository test (A9) and the listener integration tests are the safety net.personRepository.saveand within the rename transaction so the listener observes new state. Spring's default synchronous dispatch handles it; worth a sanity-check assertion in the integration test.Awaiting green light to proceed once #358 is finalised.
🏗️ Markus Keller — Plan Review
Reviewing the implementation plan in #5321 against the architectural decisions locked in #5290.
Observations
Person.displayNameis a computed@Transientfield, not a column. I checkedPerson.java:63-67—getDisplayName()derives fromDisplayNameFormatter.format(title, firstName, lastName). Plan task A5 says "inupdatePersoncapture olddisplayName, save, compare, publish if changed." That's not literally how the data flows: there is nothing to "capture" from the DTO — you must callperson.getDisplayName()before mutatingsetTitle/setFirstName/setLastName, then compare againstgetDisplayName()after save. This isn't a blocker, just imprecise wording in the plan that could mislead implementation.Event infrastructure is brand new. I grepped the entire backend — the only
@EventListenerin production code isOcrTrainingService.java:234listening to Spring'sApplicationReadyEvent. There are zero custom domain events published viaApplicationEventPublisher. Plan task A4-A5 introduces this pattern from scratch. That's fine — it's still the right call from #5290 — but worth noting in the migration comment / package-info that this is the first custom domain event in the codebase. It will set the convention for every future cross-domain decoupling decision.TranscriptionBlockhas@Version int version(optimistic locking, line 54-57). The synchronous propagation listener (A10) callssaveAllacross all matching blocks in the same transaction as the person rename. If a transcriber autosave commits between the rename's read and write phase, JPA throwsOptimisticLockExceptionand the entire rename + propagation transaction rolls back. At family-archive scale (single-digit concurrent transcribers) this is rare but not impossible. The behavior is correct (atomic rollback over silent partial corruption), but the user-facing failure should map to a clearErrorCode.PERSON_RENAME_CONFLICTso the frontend can show a "another transcriber edited a referenced block — please retry" message rather than a generic 500.@CollectionTablename must match the migration. Plan A1 creates the migration; A3 adds@ElementCollectionon the entity. JPA's default join table name is derived from the entity field name (transcription_block_mentioned_persons). The migration must declare the same name explicitly via@CollectionTable(name="...")or rely on naming-strategy defaults — flag this in A3 so it isn't discovered at integration-test time.By-layer placement holds (#5290 Item 3 confirmed). Plan correctly puts
PersonMentioninmodel/,PersonMentionPropagationListenerinservice/. ✓Recommendations
Refine A5 wording. Rephrase the task as: "In
PersonService.updatePerson, before mutating the entity, snapshotoldDisplayName = person.getDisplayName(). AfterpersonRepository.save(person), computenewDisplayName = saved.getDisplayName(). If they differ, publishPersonDisplayNameChangedEvent." This is the only correct sequence — anything else either races with the mutation or reads from the DTO (which doesn't carry a precomputed name).Add task A10b: declare the conflict ErrorCode. Add
ErrorCode.PERSON_RENAME_CONFLICTand havePersonMentionPropagationListenercatchOptimisticLockException(or let it propagate and map at the controller advice). The@Transactionalrollback is correct; the user message must be specific.Add A3.5: explicit
@CollectionTable(name = "transcription_block_mentioned_persons")on the field. Match the migration's table name byte-for-byte. Without this, JPA's naming-strategy default could diverge from the migration on certain Hibernate versions.Plan is otherwise architecturally sound. Two PRs (backend-then-frontend with OpenAPI regen between them) is the right cut — splits review surface area cleanly and keeps the API-contract change atomic.
Open Decisions (none — all architecture concerns have a concrete remedy above)
👨💻 Felix Brandt — Plan Review (Self-Review)
Stepping back to review my own implementation plan with fresh eyes.
Observations
A5 oversimplifies
displayNamechange detection.Person.getDisplayName()(Person.java:65-67) is@Transientand computed fromtitle + firstName + lastNameviaDisplayNameFormatter. The plan reads as ifdisplayNamewere a column. Implementation must readoldName = person.getDisplayName()before the setter calls, save, then computenewName = saved.getDisplayName()and compare. A6 ("publishes event when display name changes") needs explicit coverage of the case where onlytitlechanges (e.g. "Auguste" → "Dr. Auguste") — that is a display-name change and must fire propagation.renderBodyis already used via{@html}inCommentMessage.svelte:88-89with aneslint-disable-next-line svelte/no-at-html-tagscomment justifying that the function escapes its inputs. Plan B19 doesn't mention the equivalent comment forrenderTranscriptionBody. Without it, the Svelte ESLint rule will flag the call. Add to B19: "Include the sameeslint-disable-next-line svelte/no-at-html-tags -- renderTranscriptionBody escapes…comment used by CommentMessage."Marker composition (Risk #1) has no test. B3-B7 cover
renderTranscriptionBodyin isolation, but the actual integration inTranscriptionReadView.svelte(B19) coexists with the existingsplitByMarkersflow. The realistic failure mode is text like"Hallo @Auguste Raddatz [unleserlich] Marie"— must render with the mention link AND the marker<em>. No test in B16-B21 explicitly covers this composition.@Validcascade on List. A16 says "controller methods get@Valid," but JSR-303 doesn't recurse into list elements unless the field itself is annotated@Valid. The DTO must be@Valid List<PersonMention> mentionedPersonsfor@Size(max=200)and@NotNullonPersonMentionto fire. A19 (oversize displayName → 400) will silently pass with broken validation otherwise.The
firstNamecolumn on Person is nullable (Person.java:27). A person can legitimately have onlylastName, makinggetDisplayName()return just the last name (e.g. "Raddatz"). With multi-word allowed in the typeahead query (B2),@Raddatzis unambiguous, but B7 (first-sidecar-wins on duplicate) becomes likelier — single-name persons collide more often. Worth a sentence in B7's test description.Branch name diverges from existing convention. Current in-flight branch is
feat/stammbaum-issue-358. Plan proposesfeat/issue-362-person-mentions-backend. Existing convention seems to befeat/<topic>-issue-<n>. Switch tofeat/person-mentions-issue-362-backendfor consistency.Recommendations
Reword A5 explicitly: snapshot
oldDisplayName = person.getDisplayName()before setter calls, then compare withsaved.getDisplayName()afterpersonRepository.save. Add A6b: "publishes event when onlytitlechanges."Add to B19: the eslint-disable comment, copying CommentMessage's existing pattern verbatim with the rationale phrasing changed for
renderTranscriptionBody.Add B19b — composition integration test: render a block with text containing both a mention AND a
[unleserlich]marker. Assert: anchor tag for the mention,<em data-marker>for the marker, no double-escaping, no marker text inside the link's text content.Strengthen A2 —
@Validcascade: explicitly noteprivate @Valid List<PersonMention> mentionedPersonsonCreateTranscriptionBlockDTOandUpdateTranscriptionBlockDTO, not just@Validon the controller method.Rename branches to
feat/person-mentions-issue-362-backend/…-frontendto match existing convention.Open Decisions (none)
🔐 Nora "NullX" Steiner — Plan Review
XSS escaping is well-handled in B1/B4. Two new issues from the plan that the earlier review didn't surface.
Observations
GET /api/persons/{id}andGET /api/personsare NOT permission-gated in the currentPersonController.java:36-44. Only POST/PUT/DELETE carry@RequirePermission. Reads rely on Spring Security's default.anyRequest().authenticated(). The hover card (B15) and the typeahead (B10) both call these endpoints. Functionally fine — but if anyone ever adds a "public preview" route in the future, hover-card data would inadvertently follow. Add@RequirePermission(Permission.READ_ALL)to both read endpoints as part of this PR (one-line change on each@GetMapping). Defense in depth, costs nothing.@Size(max=200)validation cascade is fragile (per Felix's review). Plan A2 declares the constraints onPersonMention, plan A16 says "controller methods get@Valid." Without@Validon the field itself (@Valid List<PersonMention> mentionedPersons), JSR-303 does NOT descend into list elements. Result: A19's "rejects oversized displayName" test would pass for the wrong reason (no validation runs at all). A regression where someone removes the@Validfrom the DTO field would silently letdisplayName: <50KB string>through — at which point both DB storage and HTML escaping become DoS vectors. Make this explicit in A2 and A16, and add a test case asserting that a 201-characterdisplayNamereturns 400 (so the test would fail if validation cascade breaks).Migration without FK is correct for the spec, but creates dangling-reference attack surface. Plan A1 says
block_id + person_id + display_name table, **No FK onperson_id**. Spec says deleted persons → mention degrades to plain text. Confirmed correct. Threat-model implication: a malicious admin can delete a person while leaving staledisplayName: <legitimate name>sidecar entries pointing to a UUID that no longer resolves. The render-mode anchor<a href="/persons/{id}">would 404 on click — no security impact, just orphan UX. Worth adding A11.5: "Listener pins the documented behaviour: when no person is found by id during propagation, the sidecar entry is left untouched (it's already orphaned and will degrade per spec)."Hover card lazy fetch + concurrent hover. B15 fetches
/api/persons/{id}and/api/persons/{id}/relationshipsonmouseenter. A user moving their cursor across a paragraph with 20 mentions can fire 40 concurrent backend calls in a single second. No rate limit on these endpoints. At family-archive scale this is fine — but cache the responses client-side perpersonIdfor the page lifetime to prevent unnecessary load and reduce the attack surface for any future slowloris-style probing. OneMap<UUID, Promise<HoverData>>does it.Recommendations
Add
@RequirePermission(Permission.READ_ALL)toPersonController.getPerson()andPersonController.getPersons()as part of this PR (PR-A scope). One commit.Add to A16/A19: the test must assert 400 status for
displayNamelength 201. If validation cascade breaks (someone drops@Validfrom the field), this test fails. Same forpersonId: null.Add B15.5 — hover card fetch deduplication: per-page
Map<UUID, Promise<HoverData>>cache. Hovering the same mention twice fires one fetch. Hovering 10 mentions for the same person fires one fetch.Document the orphaned-sidecar behavior in A10/A14 test descriptions: propagation on a non-existent person is a no-op (defensive — should never happen, but a malformed event from a future async refactor shouldn't crash the listener).
Open Decisions (none)
🧪 Sara Holt — Plan Review
The test plan is comprehensive (24 tasks across 47 items, 21 are tests). Six gaps I'd add before merge.
Observations
No optimistic-lock test.
TranscriptionBlockhas@Version int version(TranscriptionBlock.java:54-57). The synchronous propagation listener (A10) doessaveAllon N matching blocks in the same transaction as the rename. If a transcriber autosaves a referenced block between the listener's read and write, JPA throwsOptimisticLockExceptionand the entire rename rolls back. A11-A15 don't cover this scenario. Add A15b — concurrent edit during rename triggers OptimisticLockException, rename + propagation rolls back atomically. Use@DataJpaTest+ Testcontainers + a second JdbcTemplate instance to bump the version mid-test.No marker-composition integration test. B3-B7 test
renderTranscriptionBodyin isolation; B19 wires it intoTranscriptionReadView.sveltealongside the existingsplitByMarkers. The realistic failure mode is"Hallo @Auguste Raddatz [unleserlich] Marie"— both transformations must compose without HTML re-escaping or marker text leaking into anchor text. Felix's risk #1 names this; the plan doesn't add a test. Add B19b — block with mention + marker renders both correctly, no double-escape, no marker text inside anchor tag.No keyboard-navigation test for
PersonMentionEditor. B11 covers selection-by-click. The existingMentionEditor.svelte:117-154supports ArrowDown/ArrowUp/Enter/Escape. The new editor must too. Add B11b —ArrowDowncycles highlight,Enterselects highlighted,Escapecloses dropdown without inserting. Without this, sighted-keyboard transcribers (40% of the senior audience per Leonie's brief) cannot use the typeahead.No test for the case "mention's display name was renamed since the block was last opened." Sequence: A opens a block with
@Auguste Raddatz, B renames Auguste to Augusta in another tab (propagation runs), A saves their pending edits. Does A overwrite B's rename? Should A's save reload + replay? Block has@Version, so A's save throwsOptimisticLockException— the front-end retry behavior (Felix Item 6, #5294) preserves the textarea, but does it also reload the latest sidecar from the server? Add B12b — save fails with 409 Conflict (optimistic lock), client refetches block and merges, transcriber sees the updated text but their unsaved input is preserved.No load test data for propagation latency. Tobias's earlier comment noted "at family-archive scale this is fine — worst case ~150–200 block updates." There's no NFR or test fixture establishing what "fine" means in milliseconds. Add A11c — propagation across 200 blocks completes in <2s on Testcontainers. This is the floor, not a benchmark — if it ever drops below, the merge is blocked until investigated.
Browser test assertions in B20/B21 are vague. "Hover mounts card, click navigates" / "Touch device skips card, click still navigates" — what's the exact assertion?
await expect(page.getByTestId('person-hover-card')).toBeVisible()afterawait page.hover(...). Touch device emulation needsawait context.setHasTouch(true)andawait page.tap(...). Spell these out in the task description so the test is reviewable.Recommendations
page.hover(),page.tap(),context.setHasTouch(true), and the exact selectors / data-testid attributes the test relies on.Open Decisions (none)
🎨 Leonie Voss — Plan Review
The earlier UX decisions (#5291) are well-reflected in the plan. Five gaps from the user-experience side.
Observations
B9 puts focus styles in
routes/layout.css. I checked — that file is the right place; line 313 already documentsrenderBody()-injected mention styles forCommentMessage. Add the new.person-mentionblock alongside existing mention styles, not in a separate file. Keeps{@html}-rendered content styling colocated.PersonHoverCardrendering states aren't all WCAG-checked. B15 specifies loading skeleton, loaded content, and error state. None of B16-B18 mentionaria-livefor the loaded-content state. A keyboard-only user tabbing through mentions hears nothing when the card mounts because the focus didn't move into it. Addaria-live="polite"to the card root so the announce-on-mount happens for assistive tech users. Or — better, since the card is contextual to the focus target — usearia-describedbyon the anchor pointing to the card's content id.Hover-card flip rule in #5291 is below-right default. What about the first line of a paragraph near the top of the viewport? Below-right works there, but the user's eye is anchored on the mention — flipping above means they have to look up. The flip rule should also handle "if mention is in the bottom 30% of the viewport, prefer above-right; otherwise below-right." Otherwise reading flow at the bottom of a long block is interrupted. Add a test viewport at 320×600 (small phone, short page) to verify: hover at y=100 → below-right; hover at y=550 → above-right.
B10 dropdown "name + life dates" doesn't specify the visual hierarchy. With name in
font-serifand dates in smallerfont-sans(project convention), the existingformatLifeDateRange()fromfrontend/src/lib/utils/personLifeDates.tsalready produces* 1882 – † 1944. Use it directly — don't re-implement. The dropdown row pattern: seriftext-base text-inkfor name, sanstext-xs text-ink-3for dates, gap-1, single line, ellipsis on overflow. Add this to B10 description.Touch target on dropdown rows is unspecified. The current
MentionEditor.svelte:183-196usespx-3 py-2— about 36px tall. WCAG 2.2 AA requires 44×44px for primary interactive elements; for the senior audience this matters. Usemin-h-[44px] px-3 py-2.5or equivalent to bump dropdown rows to compliance. Bonus: smaller fingers/cursors don't accidentally dismiss the dropdown trying to scroll on a tablet.Skeleton state visual not specified. B15 says "skeleton shimmer appears within 200ms" — what does it look like? At minimum: same width and height as the loaded card, animated
bg-mutedpulse for 3 stripes (header, dates line, notes line). Without a spec, every developer will build a different skeleton and the testable AC ("appears within 200ms") becomes "something appears within 200ms." Add to B15: "skeleton matches the loaded card's footprint (320px × ~180px), three pulse-animated bars inbg-muted, respectsprefers-reduced-motion: reduceby removing the pulse animation."Recommendations
routes/layout.cssnext to the existing mention styles (line 313).aria-describedby(anchor → card id) oraria-live="polite"to the hover card root in B15.formatLifeDateRangefrompersonLifeDates.ts. Specify the row pattern exactly.min-h-[44px]for WCAG 2.2 AA touch target.Open Decisions (none)
🚀 Tobias Wendt — Plan Review
Earlier review (#5266) confirmed no infra changes. The plan now drops
hypersistence-utils(good — no new dep to vet). Two operational items the plan misses.Observations
Pre-flight bullet 1 ("finalise the unrelated changes on
feat/stammbaum-issue-358") is the actual current state.git statusshowsRelationshipService.javaandCreateRelationshipRequest.javamodified plus four test files, and the active branch isfeat/stammbaum-issue-358. There's no PR yet for #358 (verified by absence of upstream tracking on the branch in the git context). The plan's pre-flight is correct but understated — the real path is: open the PR for #358, get it merged, thengit pull origin main, then branch for #362. Otherwise the OpenAPI-types regen in A22 will diff against #358 changes and bundle them into PR-A. Spell that out: "PR for #358 must be merged to main beforegit pull origin main."OpenAPI regeneration step (A22) belongs in PR-A, not PR-B. The plan correctly puts it after backend changes. But: the regenerated
frontend/src/lib/generated/api.tsis committed in PR-A, then PR-B uses those types. If PR-A merges and PR-B is reviewed days later, a third unrelated PR could regenerateapi.tsin the meantime, causing merge conflicts. Two mitigations: (a) keep PR-A and PR-B chained — once PR-A merges, immediately rebase PR-B on main; (b) freeze other backend work during the gap. Note this in the plan.No
@CollectionTable(name = "...")explicit declaration noted. Per Hibernate's default naming strategy,@ElementCollection List<PersonMention>onTranscriptionBlockwould auto-generatetranscription_block_mentioned_persons. The Flyway migration A1 must use the same name. If the naming strategy ever changes (project convention shift, Hibernate upgrade), the auto-generated name diverges from the migration and Flyway validation breaks at startup. Always declare it explicitly:@CollectionTable(name = "transcription_block_mentioned_persons", joinColumns = @JoinColumn(name = "block_id")). Otherwise this is a time-bomb for whoever does the next Hibernate major upgrade.Migration safety reconfirmed:
CREATE TABLEfor an@ElementCollectionjoin table is a brand-new table — zero existing rows, zero locking impact. Index creation on a new (empty) table is instantaneous. No Flyway concerns. Plan A20's rationale-comment is good — keep it.No CI runtime estimate. PR-A adds Testcontainers integration tests (A11-A15, A18, A19) — roughly +60s on Java suite. PR-B adds vitest-browser tests (B11/B12, B16-B18, B20-B21) — roughly +90s on TypeScript suite. Within current budget (backend < 4min, frontend < 5min) but worth noting in the PR description so reviewers don't get blindsided.
Recommendations
Pre-flight, restated: (1) finalise/PR/merge #358 on its own; (2)
git pull origin mainstandalone; (3) branch for #362. Don't mix #358 cleanup into the #362 branch — that's exactly the situationfeedback_atomic_commitswarns against.Explicit
@CollectionTable(name = "transcription_block_mentioned_persons")in task A3. One line, zero cost, removes a category of migration-naming bug forever.Add note to PR-A description: "PR-B (#362-frontend) is chained to this PR. Please merge promptly to avoid OpenAPI-type drift."
Add to A21: record total Java test runtime delta. If A11/A15 add more than 90s, investigate Testcontainers reuse.
Open Decisions (none)
📋 Elicit — Plan Review
Plan-level requirements check. The earlier review (#5267) flagged five spec-level gaps, four of which were resolved in #5294 (multi-word detection, OQ-1 deterministic rule, network error states, save-failure pending state). One remains unaddressed AND three new requirements gaps surface from the plan itself.
Observations
NFR-PERF-001 (rename latency) is still missing. Earlier review asked for "person rename completes within X seconds for archives with up to N transcription blocks." Plan still has no NFR. Sara's review proposes a Testcontainers floor of 2s for 200 blocks — that's a test target, not a published NFR. The two are different: the NFR is a contract for whoever operates the application; the test is a regression guard. Add NFR-PERF-001 to the issue body or a follow-up acceptance criterion.
Plan task A5 reads as if
displayNamewere a column. Implementer-facing task wording "capture olddisplayName" is ambiguous given thatPerson.getDisplayName()is@Transient(computed). Two implementations both pass A5 as written: (a) snapshot before mutation (correct); (b) compare DTO's pre-update field values to entity's post-update field values (also correct but more brittle). The wording leaves the call to interpretation. From a requirements standpoint, A6/A7 should specify which input field changes count — at minimum: title, firstName, lastName. If alias changes, does propagation fire? (Spec says no — alias isn't part ofgetDisplayName()— but A7 should explicitly assert that an alias-only change does NOT publish the event.)Acceptance criterion for "person renamed mid-edit" is missing. Sara's B12b case (transcriber holds an unsaved edit, person rename runs, transcriber saves) maps to a concrete user-facing scenario:
Telemetry / observability NFR. No task mentions logging the propagation event (
log.info("Propagated person rename {} → {} across {} blocks", oldName, newName, count)). For a feature that mutates historical text, an audit trail is essential. SLF4J parameterised log on the listener, ≤1 line per rename. Without it, "did the propagation actually run?" becomes unanswerable in production.Branch flow conflict with
feedback_pull_before_branch. The plan's pre-flight sequencing is correct, but condenses three separate, important steps into a single bullet ("Finalise the unrelated changes onfeat/stammbaum-issue-358— commit or discard before branching for #362"). Per project guidance, finalising #358 (commit/PR/merge) is itself a separate work item that should not be silently bundled into the prep for #362. Surface as a blocker: "Issue #362 implementation BLOCKED until #358 is merged."Recommendations
Add NFR-PERF-001: "The person rename operation, including mention propagation across all transcription blocks, shall complete within 3 seconds for archives with up to 2,000 transcription blocks containing the renamed person, measured at the response of
PUT /api/persons/{id}."Add AC-PROP-04 (rename-mid-edit) to the acceptance criteria in the issue body, exactly as worded above. Implementation maps to Sara's B12b.
Add A10b — propagation logging: structured info-level log on the listener, including
oldName,newName, and affected block count.Sequence #358 explicitly: rewrite pre-flight as three separate work items: (1)
[blocked-by]= #358 merge to main; (2) sync local main; (3) branch for #362. Don't pretend #358 cleanup is a one-line prep step.Tighten A5/A6/A7 task wording so the implementer cannot ship behaviour-(b) by accident. Plain language: "Snapshot
oldDisplayName = person.getDisplayName()before any setter call. After save, computenewDisplayName = saved.getDisplayName(). Publish the event iff they differ. A6/A7: assert title-only change DOES publish; assert notes-only or alias-only change does NOT publish."Open Decisions (none)
📌 Pinned summary — read this instead of all comments above
A. Locked architectural decisions (Markus follow-up #5290)
PersonServicepublishesPersonDisplayNameChangedEvent(personId, oldName, newName)viaApplicationEventPublisher;PersonMentionPropagationListener(transcription package) handles it with@EventListener @Transactional. Why: Person domain stays free of any compile-time dependency on Transcription. Spring's default synchronous dispatch keeps rename + propagation atomic in one transaction — same guarantee as a direct call, with cleaner boundaries.@ElementCollection+@Embeddable PersonMention { UUID personId; String displayName; }. Nohypersistence-utils-hibernate63dependency. Why: The future "show all blocks mentioning person X" join on an indexed B-treeperson_idis equally fast as JSONB GIN containment, avoids a new library, and matches the establishedUserGrouppattern.model/PersonMention.java,model/PersonDisplayNameChangedEvent.java,service/PersonMentionPropagationListener.java. Doc-comment the listener as transcription-domain logic. No feature-package island.@EventListener(not@Async). Why: Historical text mutation must commit atomically — silent partial corruption is unacceptable. ~150–200 blocks worst case = 100–300ms latency. If archive grows to tens of thousands of blocks later, switch to@TransactionalEventListener(AFTER_COMMIT) + @Async(one annotation).B. Locked UX/A11y decisions (Leonie follow-up #5291)
<a href="/persons/{id}" class="person-mention">— not<button>. Computed at render from sidecar. Why: Correct semantics ("link" not "button"), browser-native affordances (Ctrl/middle-click → new tab), correct screen-reader announcement.mouseenter/mouseleaveattaches to the anchor.getBoundingClientRect().person_mention_open_link("Zur Person"),person_mention_hover_hint("Klick öffnet Seite"),person_mention_load_error..person-mentionglobal CSS inroutes/layout.css(next to existingrenderBodymention styles, line 313):text-decoration: underlineresting state (WCAG AA — color alone fails 8% of men colour-blind),:focus-visible { outline: none; box-shadow: 0 0 0 2px var(--color-ink); border-radius: 2px; }.C. Locked dev/behavioral decisions (Felix follow-up #5294)
detectPersonMention()inpersonMention.tsallows spaces; stops only at newline or second@. OriginaldetectMention()for user mentions stays as-is. Why: Accepting the limitation breaks the typeahead for any archive with multiple persons sharing a first name (the common case).escapeHtml(str)helper inmention.ts. BothrenderBody()andrenderTranscriptionBody()call it.renderBodyrefactored retroactively.@OldNametext occurrences — including manually-typed ones. Defined behaviour, not a bug. Why: Family archive with controlled authorship; renaming the person should rename their textual mentions everywhere.displayName, first sidecar entry wins for all text occurrences; second produces no link. (Replaces the "ambiguous" label.)mentionedPersonsarray — transcriber retries without re-searching. No mention data lost on failed save.D. Implementation plan (Felix #5321) — 23 backend + 24 frontend tasks
Pre-flight (per Tobias #5330 + Elicit #5332 — explicit blocking):
[blocked-by]#358 must be PR'd, reviewed, and merged to main (uncommitted onfeat/stammbaum-issue-358:RelationshipService.java,CreateRelationshipRequest.java, 3 test files).git pull origin mainstandalone.feat/person-mentions-issue-362-backend(per Felix self-review #5324: matchesfeat/<topic>-issue-<n>convention).PR-A (Backend):
person_id, no GIN.PersonMention @Embeddable,@NotNull personId,@Size(max=200) displayName.@ElementCollection(LAZY)onTranscriptionBlockwith explicit@CollectionTable(name = "transcription_block_mentioned_persons", joinColumns = @JoinColumn(name = "block_id"))(Markus #5323, Tobias #5330 — match migration name byte-for-byte; survives Hibernate naming-strategy changes).PersonDisplayNameChangedEventinmodel/.PersonService.updatePerson— correct sequence (Markus/Felix/Elicit): snapshotoldDisplayName = person.getDisplayName()before any setter (getDisplayName()is@Transient, computed viaDisplayNameFormatter.format(title, firstName, lastName), Person.java:63-67),personRepository.save(person), thennewDisplayName = saved.getDisplayName(), publish iff differ.getDisplayName()).findByMentionedPersons_PersonId(UUID).PersonMentionPropagationListener:text.replace("@"+old,"@"+new)+ sidecar update +saveAll. Tests: happy path / no-op / partial-name guard / multiple occurrences. A10b — SLF4J info log (Elicit):oldName,newName, affected block count — audit trail mandatory for historical-text mutation.@DataJpaTest+ Testcontainers + second JdbcTemplate to bumpversionmid-rename → assertOptimisticLockException, full rollback of rename + propagation.ErrorCode.PERSON_RENAME_CONFLICT(Markus #5323):TranscriptionBlockhas@Version(line 54-57); concurrent autosave during rename →OptimisticLockException→ whole rename rolls back. Frontend needs the specific code to render "another transcriber edited a referenced block — please retry" rather than generic 500.@Validcascade (Felix self-review + Nora):@Valid List<PersonMention> mentionedPersonson the field, not just on the controller method. JSR-303 doesn't recurse into list elements without it. Add regression tests asserting 400 fordisplayNamelength 201 and 400 forpersonId: null— fail loudly if anyone drops@Valid.@RequirePermission(Permission.READ_ALL)onPersonController.getPerson()andgetPersons()(Nora #5326). Currently only POST/PUT/DELETE are gated (PersonController.java:36-44). Defense in depth — required by the hover card and typeahead.api.tsdrift from a third unrelated PR).PR-B (Frontend, after PR-A merges):
escapeHtmlextraction inmention.ts.personMention.ts:detectPersonMention(allow spaces; stop at\nor second@; test cases@Auguste,@Auguste,@Auguste Raddatz,@M,@,@Aug\nfoo,@Aug@bar);renderTranscriptionBody(longest-displayName-first,<a href="/persons/{id}" class="person-mention">DisplayName</a>, no@prefix in rendered text)..person-mentionCSS inroutes/layout.css:313.PersonMentionEditor.svelte: mirrorMentionEditor, hits/api/persons?q=, dropdown shows name + life dates viaformatLifeDateRange()fromfrontend/src/lib/utils/personLifeDates.ts(Leonie #5329 — don't reimplement). Row pattern: seriftext-base text-inkfor name, sanstext-xs text-ink-3for dates, gap-1, ellipsis on overflow,min-h-[44px] px-3 py-2.5(Leonie — WCAG 2.2 AA touch target; currentMentionEditor:183-196usespx-3 py-2≈ 36px).MentionEditor.svelte:117-154supports it; new editor must too.)mentionedPersons. On 409 Conflict (rename-mid-edit), client refetches block, merges latest sidecar/text from server, transcriber's unsaved input preserved.TranscriptionBlock.sveltelines 185–194; threadmentionedPersonsthrough autosave.PersonHoverCard.svelte: 3 states; manual flip viagetBoundingClientRect()(default below-right; flip up <200px from bottom OR mention in bottom 30% of viewport; flip left <300px from right; Leonie test viewport 320×600: hover y=100→below-right; y=550→above-right); hidden via@media (hover: none). Skeleton spec (Leonie #5329): match loaded-card footprint (~320×180px), 3 pulse-animated bars inbg-muted, respectprefers-reduced-motion: reduce.aria-describedbyon anchor → card content id (oraria-live="polite"on card root) for SR announce-on-mount.Map<UUID, Promise<HoverData>>cache. Cursor sweeping a paragraph with 20 mentions otherwise fires 40 backend calls/sec.TranscriptionReadView.svelte: render viarenderTranscriptionBody, compose with existingsplitByMarkers— split into marker segments first, run mention substitution +escapeHtmlon text segments only, then re-join. Addeslint-disable-next-line svelte/no-at-html-tagscomment matchingCommentMessage.svelte:88-89rationale (Felix self-review)."Hallo @Auguste Raddatz [unleserlich] Marie"must render mention link AND<em data-marker>correctly with no double-escape and no marker text inside the anchor.page.hover(),page.tap(),context.setHasTouch(true), exactdata-testidselectors.Person.firstNameis nullable (Person.java:27); single-name persons (last-name-only) valid → first-sidecar-wins collisions become more likely. Mention in test description.Felix's risks tracked:
@ElementCollection+@Embeddableis new ground (UserGrouponly usesSet<String>). Repo test + listener integration tests are the safety net.personRepository.saveand within rename transaction; sanity-check assertion in integration test.First custom domain event in this codebase (Markus #5323). Only existing
@EventListenerisOcrTrainingService.java:234listening to Spring'sApplicationReadyEvent. Worth a package-info / migration comment — sets convention for future cross-domain decoupling.CI delta estimate (Tobias): backend +60s (Testcontainers), frontend +90s (vitest-browser). Within budget. Note in PR descriptions.
E. Issue-body amendments still required (Elicit #5267 / #5332)
Add to the issue body before implementation starts:
PUT /api/persons/{id}."F. Status
Open decisions: none. Every persona's plan-review section closes with "(none)".
Blocking: PR for #358 must merge to main before #362 work starts.
Ready to start once: issue body amendments above are added, and #358 is merged.
G. Update — 2026-04-28: PR-B split (post-review of PR-A #366)
The original plan put the entire frontend (~24 tasks) into a single PR-B. PR-A's six-persona review (Tobias, Sara) recommended splitting frontend work in two for reviewability. Adopted:
PR-B1 — Edit-mode infrastructure
escapeHtmlextraction inmention.ts(refactor; shared with B2)personMention.ts:detectPersonMention(allow spaces; stop at\nor second@) + testsPersonMentionEditor.svelte+ keyboard-nav tests (mirror existingMentionEditorkeyboard behaviour)person_mention_open_link,person_mention_hover_hint,person_mention_load_errorformatLifeDateRangereuse frompersonLifeDates.tsTranscriptionBlock.sveltelines 185–194; threadmentionedPersonsthrough autosavePR-B2 — Read-mode rendering + hover card (depends on PR-B1 being merged)
renderTranscriptionBodyinmention.ts(longest-displayName-first, no@prefix in rendered text)displayNameflows from sidecar intoblock.textunescaped on the backend (PR-A) — PR-B2 must HTML-escapedisplayNameand any block-text segment on render with explicit unit tests for<script>,<img onerror>, and HTML-entity-already-encoded payloads. Nora #1 / Leonie review (PR #366)..person-mentionglobal CSS inroutes/layout.css:313 (underline resting state for WCAG AA, focus-visible ring)PersonHoverCard.sveltewith three states (skeleton ≤ 200ms, 404 unmounted, non-404 error in card) +getBoundingClientRect()flip rulesTranscriptionReadView.svelteintegration withsplitByMarkers+ B19b marker-composition testaria-describedby/aria-liveon hover cardeslint-disable-next-line svelte/no-at-html-tagson{@html ...}(mirrorCommentMessage.svelte:88-89pattern)Why split: B1 is editor-side, fully testable in isolation, no read-path coupling. B2 requires B1's typeahead inserting sidecar entries before the read-path can render anything. Splitting halves each review's surface area, surfaces XSS-on-render concerns in their own commit boundary, and lets B1 land while B2's hover-card design iterates.
Per-PR review-cycle expectation: 1 cycle each (no fixes anticipated for B1; B2 may require touch-ups once Leonie sees the hover card on a real device).
PR-A (Backend) opened — #366
http://heim-nas:3005/marcel/familienarchiv/pulls/366
Implements the backend half per the locked plan in #362#issuecomment-5339. 21 atomic commits, 1446/1446 backend tests green.
Summary of what landed
e833d1f7PersonMention@Embeddablea6c8db22PersonDisplayNameChangedEventrecordb435fd69TranscriptionBlock.mentionedPersonsfield + round-trip test0f3e0003,7805da52@Validcascade + 400 tests for length / null80ddfb47,4e8df66a,1db0f38fPersonService.updatePersonpublishes event + 3 publish/no-publish tests08e79870,28112e1dPersonMentionPropagationListener+ 5 tests (happy / no-op / partial / multi / orphan)4d288589,a2c633c5,29a1df5d,e94ffde0,e0212613,bd175321ErrorCode.PERSON_RENAME_CONFLICT(backend + frontend mirror + de/en/es)4bc4267e404d874b,221a6af8PersonController@RequirePermission(READ_ALL)on the two GET endpoints5ebe1f1afrontend/src/lib/generated/api.tsregen8b498665Spec deviation
Per A15b: end-to-end JPA optimistic-lock race reproduction inside
@DataJpaTestis impractical (no window for an external version bump within a single transaction's listener load + save). The translation logic is unit-tested inPersonServiceTestwith a stubbed publisher; Hibernate's own suite covers the@Versionmechanism. If a stronger e2e check is wanted later, a@SpringBootTestwith explicit thread coordination is the right home.Blocking on PR-A merging
PR-B (frontend, ~24 tasks) cannot start until #366 lands on main, because every PR-B task imports from the regenerated
api.ts.🤖 Generated with Claude Code
PR-B1 (Frontend, edit-mode infrastructure) opened — #369
http://heim-nas:3005/marcel/familienarchiv/pulls/369
Implements the PR-B1 scope from the pinned summary (#5339 §G):
escapeHtmlextraction inmention.ts(refactor; PR-B2 will reuse).personMention.ts—detectPersonMention(multi-word; stops at\nor second@) + 12 unit tests.PersonMentionEditor.sveltemirroringMentionEditor:/api/persons?q=,min-h-[44px]rows (WCAG 2.2 AA), life dates viaformatLifeDateRange(), keyboard nav (B11b), 14 component tests.popup_empty/btn_label.TranscriptionBlock.sveltelines 185–194 →<PersonMentionEditor>. Quote-selection still works via acaptureTextareacallback exposing the underlying node.useBlockAutoSave.saveFnwidens to(blockId, text, mentionedPersons), threadsmentionedPersonsthrough autosave +flushOnUnload. Pending payload preserved across save failure for retry (B12, with regression test).routes/documents/[id]/+page.sveltevia the new pure helpermergeBlockOnConflict()(6 unit tests).PR-B2 (read-mode rendering, hover card, stored-XSS guard, Playwright hover/tap) follows next.
Next step: an automated
/review-pr → /implementloop on #369 until all personas approve.PR-B2 (Frontend, read-mode + hover card) opened — #371
http://heim-nas:3005/marcel/familienarchiv/pulls/371
Implements the PR-B2 scope from the consolidated plan in #362#issuecomment-5339 (§G — split adopted post PR-A review).
Tasks delivered
renderTranscriptionBodyinmention.ts(longest-first, first-sidecar-wins, word boundary,@stripped from rendered text)<script>,<img onerror>, already-encoded entities, quote payloads).person-mentionglobal CSS inroutes/layout.css(underline at rest WCAG AA, focus-visible ring)PersonHoverCard.svelte— three states (skeleton ≤200ms / error / loaded); navy header, family-only chips, 120-char notes excerptaria-describedby(set on the anchor at hover) +aria-live="polite"on the cardTranscriptionReadViewcomposessplitByMarkers+renderTranscriptionBody; manualgetBoundingClientRect()flip; eslint-disable mirrorsCommentMessage.svelte:88-89Hallo @Auguste Raddatz [unleserlich] Marierenders link AND<em data-marker>correctly, no nesting, no double-escapeSvelteMap<personId, Promise<HoverData>>dedup cachehasTouch=true) e2e testsdata-person-deleted="true", card unmounts, click suppressed (degrades to plain unlinked text)Tests: 47 new tests (17 utility, 18 hover-card, 8 read-view component, 4 e2e). All passing.
Following the loop now:
/review-pr→/implementuntil all reviewer concerns are resolved.