feat: decouple person-mention display text from person name (#372) #373
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?
Closes #372
Summary
Replaces the textarea-based
PersonMentionEditorwith a Tiptap v3 rich-text editor and decouples the stored mentiondisplayNamefrom the person's DB display name. When a transcriber types@Augand selects "Auguste Raddatz" from the dropdown, the mention now storesdisplayName: 'Aug'(the typed query) — preserving archival fidelity of the original transcription.AC mapping
commandinsertsprops.displayName = renderProps.query. Regression test:PersonMentionEditor.svelte.spec.ts"typed query → sidecar".decoration-ink/50 underline(matches read-mode.person-mention, ≥3:1 contrast per WCAG 1.4.11).MentionDropdown.svelterenders{formatLifeDateRange(...)}line; spec covers "* 1882 – † 1944" rendering.beforeinputinterception needed).MentionDropdown.onKeyDown; specs for ArrowDown highlight, Enter select, Escape close.mentionSerializerlongest-displayName-first matching; explicit "backward compatibility" suite inmentionSerializer.spec.ts.TranscriptionBlockmigrated fromcaptureTextarea+handleTextareaMouseUptoonSelectionChangewired to Tiptap'sselectionUpdate.Backend changes
Deleted the
PersonMentionPropagationListener+PersonDisplayNameChangedEventinfrastructure. Under the new archival model, mentions are never rewritten when a person is renamed — the listener would actively corrupt historical transcription text.PersonMentionPropagationListener,PersonDisplayNameChangedEvent,ErrorCode.PERSON_RENAME_CONFLICT, the publish-event block inPersonService.updatePersonPersonMention.displayNamePersonServiceTest, 1 inPersonControllerTestFrontend changes
mentionSerializer.ts(puredeserialize/serializewith round-trip invariant),MentionDropdown.svelte(Tiptap suggestion-compatible), Tiptap v3.22.5 (core, starter-kit, extension-mention) pinned exact,renovate.json@tiptap/*group rulePersonMentionEditor.svelteas a Tiptap editor with customMentionextension (usespersonId/displayNameattrs, not the defaultid/label);PersonMentionEditor.svelte.spec.ts(old tests useddocument.querySelector('textarea')— dead after migration)TranscriptionBlock.sveltequote selection,PersonHoverCard.sveltegeb.→m.person_born_name_prefix(),errors.tsremovesPERSON_RENAME_CONFLICT, i18n keystranscription_editor_aria_label+person_born_name_prefixin de/en/esCycle 2 review fixes (post-review push)
Addressed every blocker plus most concerns from the seven-persona review:
personMention.ts/spec) + staleerror_person_rename_conflicttranslation keys.editor.setEditable(!disabled)wired reactively,aria-disabledon wrapper — keyboard users can no longer type into a "disabled" editor (WCAG 2.1.1 / 4.1.2).decoration-brand-mint(≈1.7:1) todecoration-ink/50(≈6.4:1); dropdown highlight ring switched fromring-brand-mint(≈2.5:1) toring-brand-navy(≈14.5:1) — WCAG 1.4.11 Non-Text Contrast resolved.MentionDropdown(uses existingm.person_mention_create_new()key, opens in a new tab so the editor state is preserved).PersonMentionEditor.svelte.spec.ts"renders a malicious displayName as text, not as HTML elements") asserting no<img>/<script>element is created from a sidecar payload like<img src=x onerror=alert(1)>.GET /api/personsreturns the fullPersonSummaryDTOincludingnotes. Tracked separately in #374 ("GET /api/persons leaks PersonSummaryDTO.notes to typeahead clients (CWE-200)") so the fix can be done with proper DTO partitioning.Test results
mentionSerializer.spec.ts— 12/12 ✅PersonMentionEditor.svelte.spec.ts— 19/19 ✅ (rewritten for Tiptap; +5 new tests for disabled, XSS, empty-state link)TranscriptionBlock.svelte.spec.ts— 22/22 ✅PersonServiceTest— 49/49 ✅Notable design decisions
$stateproxy: Svelte 5'smount()does NOT return prop accessors — settinginstance.items = newValueis a no-op. The dropdown receives a single reactive$stateobject as amodelprop; mutating fields propagates via Svelte's proxy reactivity. The prop is namedmodel(notstate) because the$staterune name shadows astateidentifier in templates.MentionwithpersonId/displayName(instead of mappingid/labelin the serializer) keeps the Tiptap JSON shape aligned with the storage model and avoids attribute remapping.setEditableeffect guard: the reactive$effectcallseditor.setEditableonly wheneditor.isEditable !== shouldBeEditable. Without the guard, setEditable's own ProseMirror transaction firesonUpdate→ bind:value/mentionedPersons writes → host re-render → effect re-fires, exceedingeffect_update_depth.Bundle size note (Tobias #5623)
Three new dependencies in this PR:
@tiptap/core3.22.5 (~50 KB gzipped, includes ProseMirror)@tiptap/extension-mention3.22.5 (~5 KB gzipped)@tiptap/starter-kit3.22.5 (~30 KB gzipped, pulls multiple extensions)Net add to the transcription page chunk: ~85 KB gzipped. Acceptable for a self-hosted family archive on a Hetzner CX32 with mostly single-digit concurrent users; bandwidth cost negligible. Future bundle audits should baseline against this.
Test plan
mentionSerializerround-trip preserves text exactly (incl. backward compat with old full-name sidecar)@Augand selecting "Auguste Raddatz" storesdisplayName: 'Aug'(regression test for AC-1)/persons/newdisplayName(<img src=x onerror=…>) renders as text only — no HTML element created@Clar, select "Clara Cram" — editor showsClara(underlined, ink-50%), notClara Cram🤖 Generated with Claude Code
Felix Brandt — Senior Fullstack Developer
Verdict: Changes requested
Solid TDD evidence: red/green visible across
mentionSerializer.spec.ts(round-trip invariant with explicit failure-message assertion), the rewrittenPersonMentionEditor.svelte.spec.ts(AC-1 regression), and the cleaned-upPersonServiceTest. The Tiptap migration is well-structured and the serializer is pure (testable, refactor-friendly). Two code-cleanliness blockers and a few suggestions.Blockers
Dead code:
frontend/src/lib/utils/personMention.ts+personMention.spec.ts—detectPersonMentionwas the textarea-era helper. The Tiptap migration replaced it with the suggestion plugin'schar: '@'mechanism; nothing in production importspersonMention.tsanymore (only its own spec does). Per the rule "Dead code is deleted, not commented out": delete both files. The spec running in the suite is misleading — it tests a function nobody calls.Stale i18n keys in
messages/{de,en,es}.json—errors.tsremoved thePERSON_RENAME_CONFLICTmapping anderror_person_rename_conflictswitch case, but the message keys remain at line 554 of all three locale files:These are now orphaned. The error code is gone from the backend (
ErrorCode.java), gone from the frontend type union, gone fromgetErrorMessage. Delete the three JSON entries to keep the i18n surface honest. (Otherwise Paraglide ships unused translation strings — minor bloat plus noise for translators.)Suggestions
PersonMentionEditor.svelte:34—editorElis declared as a non-nullHTMLDivElementwithout$state. It's bound viabind:thisand read insideonMount, which is fine in practice, but consider explicitly typing itHTMLDivElement | undefinedand guarding inonMount— Svelte 5 lifts bind:this into reactive state implicitly, but the explicit type makes the temporal contract obvious.disabledprop is not enforced on the editor itself (line 1937 of the diff). The wrapper setspointer-events-noneandopacity-50, but a keyboard user who tabs into the editor'scontenteditablediv can still type. Tiptap supports this declaratively:And reactively:
This matches the textarea's old
disabledsemantics. Without it, "disabled" is purely visual.onMountarrow function does not return cleanup — theonDestroy(() => editor?.destroy())is correct, but you could collapse both lifecycle hooks into a singleonMountthat returns the cleanup function (return () => editor?.destroy()). One concern instead of two — same lifetime semantics.MentionDropdown.svelte:57— theposition$derived.byreadswindowsynchronously. During SSR this would explode. The component is mounted imperatively inonMountso it never runs server-side in practice — but a comment near thewindow.innerHeightreference would document the assumption. (Or guard withtypeof window !== 'undefined'for defense in depth.)mentionSerializer.ts:39— overlap detection is O(n²) per mention. For typical block sizes (a few mentions, <500 chars) this is fine. If a paragraph ever has 50+ mentions, replace with a sweep-line approach. Document the bound somewhere or leave a TODO.PersonServiceTest.java— the import block changed fromMockito.*star import to explicit imports fornever,verify,when. Good cleanup. ButargThatis imported and never used in the visible diff context. If it's truly unused after the deletion, drop it.PersonMention.java:29— the new comment// Archival: ...is good and documents the why. This is the kind of comment that earns its place. Leave it.LGTM
personId/displayNameattrs (instead of remappingid/labelin the serializer) keeps the storage model and the editor model aligned. Good design call documented in the PR description.mentionSerializer.tsis pure, side-effect-free, and the round-trip invariant is enforced by tests. Textbook example of a refactor-friendly utility.updatePerson_returns409_whenRenameConflicttest removal — correct response to the listener deletion. No orphaned coverage.🤖 Generated with Claude Code
Markus Keller — Senior Application Architect
Verdict: Approved with concerns
Architecturally, this PR is the right call: the rename-propagation listener actively corrupted archival fidelity (the whole point of issue #372), and removing it is a net architectural improvement — fewer moving parts, no cross-domain transactional coupling, no event-publisher mock in the service test. The decision aligns with "push integrity decisions to the lowest trustworthy layer" and recognises that historical mention text is now archival, not derived data. That's a domain-model clarification, not just a code change.
Concerns
Frontend module boundary leak — client-side fetch from a leaf component.
PersonMentionEditor.svelte:118callsfetch('/api/persons?q=...')directly from inside a Tiptap suggestion. The frontend CLAUDE.md is explicit: "Data flows from+page.server.tsvia props — never client-side API fetch." This is one of the rare legitimate exceptions (typeahead in an interactive editor cannot pre-load all persons via SSR), but it deserves to be acknowledged. The security comment in the file mentions the SvelteKit Vite proxy handling auth — true in dev, but production goes through Caddy. Either:+server.tsendpoint (so auth handling stays consistent with the rest of the app), ordocs/adr/) documenting "client-side typeahead exception for editor mention plugin: rationale, threat model, scope of fetched data."Right now this exception is invisible — six months from now, someone will read CLAUDE.md, see
fetch()in a leaf component, and either copy the pattern or rewrite this. ADR captures intent.Listener deletion changes the contract of
PersonService.updatePersonin a non-trivial way. Before: rename propagated to historical text atomically. After: rename leaves history alone (intentional). This is a semantic change visible to API consumers (e.g. anyone readingPersonMention.displayNamewho assumed it tracked the current person name). The PR adds an inline archival comment toPersonMention.java— good, but worth one ADR entry too:The PR description has this rationale but it lives in a Gitea PR — not where future devs read codebase context.
The
MentionDropdown.sveltemodelprop pattern as a Svelte-5-around-mount()workaround is clever, but undocumented at the architecture level. The PR description explains it well; consider an inline comment (not just in the file but infrontend/CLAUDE.mdunder "Imperative Svelte 5 mount pattern") so this becomes a reusable, named pattern instead of a one-off.LGTM
PersonServiceno longer publishes events, but it never reached into transcription internals either way. Domain boundaries clean.ErrorCode.javacleanup is precise — single value removed, no leftover references in backend code.updatePerson_*tests still cover the actual update logic).Minor notes
renovate.jsonat the repo root: confirm the team's chosen Renovate config location (renovate.jsonat root vs.gitea/renovate.jsonvsrenovate.json5). For a self-hosted Gitea Renovate runner this matters — the runner config ininfra/(if present) may already point elsewhere. Tobias to confirm.mentionSerializer.tscould push downstream — verify no editor outside this PR (e.g. a future mass-import preview, an OCR auto-mention) needs the same shape; if so, extract to a sharedlib/transcription/namespace before the second consumer arrives.🤖 Generated with Claude Code
Nora Steiner ("NullX") — Application Security Engineer
Verdict: Approved with concerns
The Tiptap migration introduces three new attack surfaces (renderHTML output, contenteditable input, client-side fetch) and removes one (the regex-based listener that rewrote stored block text — that thing was a textbook injection-vector waiting to happen). Net security: improvement. But the new surfaces deserve explicit assertions.
Concerns (no confirmed vulnerabilities — these are smells worth pinning down)
renderHTMLoutput for the Mention node is array-form, which Tiptap auto-escapes — verify with a test.PersonMentionEditor.svelte:101–112returns a tuple['span', { 'data-display-name': node.attrs.displayName }, '@${node.attrs.displayName}']. ProseMirror's DOM serializer treats the third element as text content and escapes it, so adisplayNameof<script>alert(1)</script>should become<script>alert(1)</script>. The serializer test verifies that the string survives the round-trip — but no test asserts that the rendered DOM does not contain a script tag. Add one (CWE-79):Without this test, a future "let me just use innerHTML for performance" refactor silently regresses.
renderTranscriptionBody(read mode, inPersonHoverCard.svelteand elsewhere) is unchanged but consumes the new short-displayNamesidecar entries. IfdisplayName: 'Aug'is naively interpolated as HTML somewhere, a malicious transcriber typing@<script>into the typeahead and selecting any person storesdisplayName: '<script>'in the sidecar, which then gets reflected in read mode. Trace where the sidecardisplayNameis consumed and confirm every consumer escapes it. The serializer security test correctly says "HTML escaping is the renderer's job" — that places the burden on the renderer. Audit the renderer.Client-side
fetch('/api/persons?q=' + encodeURIComponent(query))— the encoding is correct, but the response is dropped into the dropdown without server-side filtering of which fields are exposed. The component types it asPerson, which (per the OpenAPI schema) includesbirthYear,deathYear, possiblynotes. Notes are private text. ConfirmGET /api/persons?q=returns only the typeahead-safe shape (id, displayName, birthYear, deathYear), not the full Person entity. Ifnotesleak into the dropdown, that's CWE-200 (information exposure). Quick check: read thePersonController.searchmethod or hit the endpoint locally and verify.Disabled state is purely visual (Felix flagged this for usability — I flag it for security). A
disablededitor that still accepts keyboard input means a "read-only" view is not actually read-only. If transcription edit permissions are gated bydisabled={!user.canWrite}, a determined user can tab into the contenteditable and modify content client-side — server-side authorization will (correctly) reject the save, but the UI lies about state. Wireeditor.setEditable(!disabled)reactively as Felix suggested. Defense in depth: the server is the only authority, but the client should not lie.Suggestion plugin's
itemscallback fetches without a CSRF token — fine forGET, but worth noting. GET requests don't need CSRF; the cookie-based auth via Vite proxy / Caddy works. No action needed; just affirming the request method.Backend security — clean
PersonMentionPropagationListenerremoves a regex-based string rewriter operating on user-controlled text. The regex was ReDoS-bounded byPattern.quote, but the concept of "rewrite arbitrary user content based on metadata changes" is a class of vulnerability we don't need.ErrorCode.PERSON_RENAME_CONFLICTremoval is contained — no information leak from the now-impossible 409 response.PersonService.updatePersonno longer catchesOptimisticLockingFailureExceptionfrom the listener path because the listener is gone. The remaining service code path doesn't introduce new exception swallowing. Good.Detection notes
innerHTMLassignment in the transcription render path so future contributors don't accidentally introduce XSS:notes(lock down the API surface).🤖 Generated with Claude Code
Sara Holt — Senior QA Engineer
Verdict: Approved with concerns
The test rewrite is a major improvement: the old
document.querySelector('textarea')+dispatchEvent('input')approach was implementation-coupled and brittle. The newuserEvent.type+vi.waitForflow tests user-visible behavior. AC-1 has explicit regression tests with strong assertions ("typed text becomes displayName, not the DB name"). The serializer round-trip invariant is exactly the kind of property test that catches refactoring regressions. 14/14 + 22/22 + 12/12 + 49/49 — green is documented.Concerns
Test pyramid: no integration coverage for the new client→server typeahead path. The component spec mocks
fetchglobally, so the wire format between Tiptap'sitemscallback andGET /api/persons?q=is never exercised end-to-end. This is acceptable at the unit layer, but I'd want at least one Playwright E2E that types@Auginto a real document's transcription block, sees a real backend response, and selects a person. Without it, a backend change to the search response shape (e.g. wrapping in{items: [...]}) silently breaks the editor and is only caught manually. Add it tofrontend/e2e/transcription/mention-typeahead.spec.tsif it doesn't exist.mentionSerializer.spec.ts— strong start but missing edge cases I'd expect:displayName: ''in sidecar — what happens? Should it be skipped or preserved verbatim?@Annafollowed by@Anna Schmidtin text with both in sidecar — current code sorts by length descending, soAnna Schmidtwins where it appears, but is the position-aware matching correct? Add an explicit test case.@Müllerround-trip — string compares are byte-aware, JS strings handle UTF-16 surrogates. Add'@François'as a round-trip case.'\n'but the JSON model uses separate paragraphs.'a\n\nb'(double newline) — verify it produces 3 paragraphs and round-trips.TranscriptionBlock.svelte.spec.tsquote-selection test (line 2533–2554 of the diff) — uses rawdocument.createRange()+selection.addRange()+ manually dispatchedinputevent. This works because Tiptap listens to selection changes. But the test bypasses Tiptap'sonSelectionUpdateplugin lifecycle — it depends on theinputevent triggering the right callback chain. If a future Tiptap upgrade decouplesinputfrom selection notifications, this test silently passes for the wrong reason. Usevitest-browser'suserEvent.tripleClick(editor)(selects the line) or Tiptap's editor command to drive selection through the official path.Stale test:
personMention.spec.tsstill runs but tests dead code. Felix flagged the source file. The spec file should either be deleted along with the source, or — if the tests are still valuable as documentation of the regex rules — moved into thementionSerializertest alongside thedeserializeboundary tests. As-is, it's coverage padding.Backend: the listener tests were deleted (
PersonMentionPropagationListenerTest.java, full file removal). Correct response to listener deletion. ButPersonServiceTestlost 4 tests coveringupdatePerson_publishesEvent_*paths. Verify these aren't replaced by tests confirming the non-publishing behavior:Wait —
eventPublisherfield was removed from the test class entirely (line 511 of diff:@Mock ApplicationEventPublisher eventPublisher;deleted). That's correct, but it means the negative assertion is now untestable structurally. The architecture decision (no event published) is now enforced by the absence of the dependency, which is a cleaner guarantee. Acceptable.LGTM
displayName, not the DB name. If a future contributor "fixes" the suggestion to insertprops.displayNamefromperson.displayName, this test fails immediately. That's permanent regression value.mentionSerializer.spec.tsis property-style — covers any text/sidecar pair, not just enumerated cases. Good QA hygiene.vi.waitForreplaces fake-timer hacks. Real-time tests in browser mode are slower but more honest about what the user sees. Right call.Quality gate concerns
git checkout main && npm run testthat these fail on main too, then file Gitea issues. Pre-existing failures are tech debt — name them or fix them.open document → click into transcription block → type @ → select person → save → reopen → verify mention persists with typed displayName. This is a critical user journey and the heart of issue #372. Add it.🤖 Generated with Claude Code
Leonie Voss — UX Designer & Accessibility Strategist
Verdict: Approved with concerns
The visual model is preserved (mention pill: brand-mint underline, brand-navy text — matches the read-mode rendering). The dropdown's flip-strategy positioning (
MentionDropdown.sveltelines 53–63 of the diff) is a real UX improvement over the textarea version, which always opened downward and clipped on long blocks. Touch targets stayed at 44px (min-h-[44px]). i18n keys added for de/en/es. Three accessibility issues need fixing before merge.Critical
disabledis visual-only — keyboard users can still type into a "disabled" editor.PersonMentionEditor.svelte:1937wraps the editor div withclass:opacity-50andclass:pointer-events-none, butpointer-events-nonedoes not block keyboard focus. A user pressing Tab will land in the contenteditable and be able to type. This is a WCAG 2.1.1 (Keyboard) and 4.1.2 (Name, Role, Value) issue: the disabled state is announced by neither name nor role, and keyboard users have no way to know the editor is supposed to be inactive.Fix:
And add
aria-disabled="true"to the wrapper when disabled — Tiptap will setcontenteditable="false"on the inner div, and screen readers announce "Transkriptionstext, deaktiviert" instead of "Transkriptionstext, edit text."Major
Mention pill class
text-brand-navy font-medium underline decoration-brand-mint underline-offset-2(line 108) —brand-mint #A6DAD8as an underline on white background has a contrast ratio of ~2.0:1. It fails WCAG 1.4.11 Non-Text Contrast (3:1 minimum). The mint underline communicates "this is a mention" — that's a meaningful visual cue, not decorative. Either thicken todecoration-2(often perceived as more solid) AND darken to adecoration-brand-mint-700token, or use a brand-navy underline with brand-mint as the background highlight (bg-brand-mint/20). Test with the senior-on-bright-screen scenario.MentionDropdown.svelte— keyboard highlight ring isring-2 ring-brand-mint(line 1431 of the diff), same problem as #2. Onbg-brand-mint/20, the brand-mint ring is ~2.5:1 against white. Usering-brand-navy(14.5:1) for the highlighted-row indicator. The screenshot will tell the whole story — please attach one before merging.Minor
MentionDropdown.svelte:32—let highlightedIndex = $state(0)resets to 0 whenevermodel.itemschanges. Result: when the user types@Aug(highlight = first match) then types one more letter@Auge, the highlight jumps back to position 0. If the same result is still in the list at a different index, the previous user expectation breaks. Minor edge case, but for a power user typing fast this is friction. Consider preserving the highlight bypersonIdrather than index.No empty-state "Create new person" link. The old textarea-era component (line 1894 of the diff, deleted) showed "Neue Person anlegen →" when no results matched — that's the
m.person_mention_create_new()translation key. The newMentionDropdown.svelteonly shows "Keine Personen gefunden" with no creation affordance. This is a regression in workflow: if the transcriber types a name that doesn't exist, they have to (a) close the dropdown, (b) navigate to /persons/new, (c) come back, (d) re-type. Restore the link:The
m.person_mention_create_new()key still exists in all three locale files — it's just not rendered.aria-multiline="true"is set on the editor (line 1755 of the diff) — good. But thearia-label="Transkriptionstext"is the only accessible name. For SR users navigating a document with multiple transcription blocks, every block reads as "Transkriptionstext, multiline edit text" with no way to distinguish them. Pass the block's index or label as part of the aria-label:aria-label={\Transkriptionsblock ${blockIndex}: ${m.transcription_editor_aria_label()}`}`. Optional — but improves wayfinding for screen reader users on long letters.LGTM (visual / brand)
font-serifon the editor body,font-sanson the dropdown labels and the empty-state text — matches the brand typography system.flex flex-col gap-1rows withmin-h-[44px]keep touch targets correct for tablet stylus users (the 60+ transcriber persona).font-sans text-xs text-ink-3for the life-date subline is the correct hierarchy.geb.→m.person_born_name_prefix()change inPersonHoverCard.svelte(line 1461) is the right move —geb.is German-only and was broken for en/es viewers. Now it'snée/n.— culturally correct.Test coverage on a11y
axe-playwrightshould run on a document detail page in transcribe mode to catch the contrast issues above automatically. Sara mentioned E2E coverage gaps; an axe pass ontranscription edit modewould fold a11y testing into the same coverage push.🤖 Generated with Claude Code
Tobias Wendt — DevOps & Platform Engineer
Verdict: Approved with concerns
Bundle-size and dependency-management considerations dominate this review. The Tiptap stack is appropriate for the use case but adds non-trivial weight to the frontend; the
renovate.jsonplacement and grouping rule need a quick check against the existing infra. No deployment- or backup-affecting changes — backend is purely a deletion, which is cheap to deploy.Concerns
Renovate config placement & integration. The new
renovate.jsonis at the frontend repo root-... wait, no, it's at the monorepo root (line 3215 of the diff). For a monorepo withfrontend/,backend/,ocr-service/, this is the right location for repo-wide Renovate runs. Verify three things before merge:.gitea/renovate.json? If your runner config (ininfra/) expects the latter, this PR's file is dead config and Renovate keeps using its defaults.matchPackagePatternssyntax is the legacy Renovate config — modern Renovate prefersmatchPackageNameswith regex (/^@tiptap//). Your runner version determines compatibility. The legacy syntax still works but emits a deprecation warning in logs.automerge: falseis correct for@tiptap/*(a richtext editor is too API-surface-heavy for blind automerges). But this is the firstrenovate.jsonin the repo per the diff context — if you already have one elsewhere (e.g..github/renovate.jsonfrom a Gitea fork), confirm one config wins.Bundle size — three new dependencies.
That's ~85 KB gzipped added to the transcription page bundle. For a self-hosted family archive on a Hetzner CX32 with mostly single-digit concurrent users, this is acceptable — the bandwidth cost is negligible. But add a
npm run buildsize diff to the PR description so future bundle audits have a baseline. (Runnpx vite-bundle-visualizeronce and commit a note: "transcription chunk: 142 KB → 227 KB after Tiptap.")yarn.lockwas modified (line 2869 of the diff) butpackage-lock.jsonwas also modified (line 676). Pick one. Mixing both is a smell — npm will use one, yarn the other, and CI runners pick the wrong one half the time. The project's documented installer isnpm installperfrontend/CLAUDE.md, so:yarn.lockyarn.lockto.gitignorenpm ci(notyarn install)If yarn was committed by accident from a contributor's local environment, this is the moment to clean it up. Otherwise every Renovate update will create lockfile drift between the two.
Pinned exact versions (3.22.5 — no caret). Good — reproducible. Renovate will manage updates. But
@tiptap/extension-blockquoteand friends inyarn.lockare caret-ranged, suggesting starter-kit pulls them as transitive deps with their own range. The PR description says "Tiptap 3.22.5 (core, starter-kit, extension-mention) pinned exact" — verify the transitive deps stay locked across reinstalls. Usenpm ci(notnpm install) in CI to enforce lockfile fidelity.No CI workflow changes detected. Adding three dependencies should at least bump the
frontend-buildjob'sactions/cache@v4key (if the cache key includespackage-lock.jsonhash, this is automatic — verify by reading.gitea/workflows/). If the key is hardcoded or hashes onlypackage.json, the new deps cause cache misses but still work; just a missed optimization.LGTM
renovate.jsoneven existing is a positive step — most family/hobby projects skip dependency management until something breaks.Action items before merge
frontend/yarn.lock, confirm npm-only.renovate.json.🤖 Generated with Claude Code
Elicit — Requirements Engineer (Brownfield review)
Verdict: Approved with concerns
This PR resolves issue #372 ("Person mention: decouple display text from person name") with a surgically scoped intervention. The acceptance-criteria mapping in the PR description is excellent — AC-1 (typed text → displayName), AC-4 (mention atomicity), AC-6 (backward compat), AC-7 (quote selection still works) are each linked to a concrete test. AC-2/3/5 are not mentioned; verify they're either out-of-scope for this PR (a follow-up) or implicitly covered.
Open Questions / TBD
OQ-373-01 — AC coverage gaps. The PR description maps AC-1, 4, 6, 7. What about AC-2, 3, 5? If they were retired during refinement, link the comment that retired them. If they're for a follow-up issue (B3?), add a "Out of scope" subsection to the PR body. Reviewers should not have to cross-reference issue #372's full AC list to know what is and isn't shipping.
OQ-373-02 — Manual test plan items unchecked.
These are checkbox items in the test plan that are not yet checked. The behavior they verify is the core promise of issue #372. Are they:
OQ-373-03 — i18n key
transcription_editor_aria_label. "Transkriptionstext" / "Transcription text" / "Texto de transcripción" — these are translations of "transcription text," not of "transcription editor" or "transcription text editor." For screen reader users, the announced name is<value>, edit text, multi line(textbox role + multiline). Compare:Decide which fits the senior persona better; the current text is functional but not optimal.
OQ-373-04 — User story for
@typeahead empty state. The old empty state offered "Neue Person anlegen →" (linked to/persons/new?name=...). The newMentionDropdown.svelteshows only "Keine Personen gefunden". Was this an intentional requirement change ("simplify the empty state — no inline person creation"), or an oversight? The translation keym.person_mention_create_new()is still in all three message files, suggesting the intent was to keep it. Confirm with the product owner; if removed intentionally, drop the orphaned translation key (Felix flagged this elsewhere).NFR Checklist for the changed surface
/documents/[id]did not regress.disablednot enforced; mint underline contrast questionable (Leonie).error_person_rename_conflictkeys not removed (Felix).displayNamein DOM (Nora).LGTM
Definition of Done — gating items
error_person_rename_conflictdeleted or kept with a justification.🤖 Generated with Claude Code
Felix Brandt — Senior Fullstack Developer (cycle 2 re-review)
Verdict: Approved
Both blockers from my cycle-1 review are gone. Dead code (
personMention.ts+ spec) deleted in4ac94b2f; staleerror_person_rename_conflictkeys removed in all three locale files in94d07334. Atomic commits, each with a clear "why" message — exactly the diff hygiene I want. Cycle 2 is eight commits, every one a single concern. No bundling.What I checked again
frontend/src/lib/utils/personMention.ts— gone. Spec file gone. No dangling imports anywhere insrc.messages/{de,en,es}.json:554— the orphaned key is gone in all three files.errors.tsconsistent. No translator confusion.PersonMentionEditor.sveltedisabledsemantics —editor: !disabledat construction +$effect(() => editor.setEditable(!disabled))with the idempotence guardeditor.isEditable !== shouldBeEditable. The follow-up commitba88febcshows TDD-as-truth-discovery: the failing tests caught a re-entry loop that came from setEditable's own ProseMirror transaction → onUpdate → bind:value writes → re-render → effect re-fire. Guard fix is the right shape — compare desired state, only mutate on diff. Same pattern I'd write.fa7b97ac) is a characterization test for existing safe behavior — Tiptap's array-form renderHTML is auto-escaped; the test pins that down so a "let me use innerHTML for performance" refactor breaks immediately. Good regression value.Suggestions (non-blocking)
The XSS test asserts negative against the wrong primary:![]()
" hints at fragility. If a future Tiptap version stops inserting the separator (or changes its attrs), the test still passes but for trivial reasons. Consider also asserting that the
textbox.querySelector('img[onerror]')is correct, but the comment "Tiptap inserts its own ProseMirror-separatordisplayNametext content equals the literal string<img src=x onerror=alert(1)>— that proves the round-trip preserved the payload as text rather than only proving "no element exists with these attrs."docs(PersonMentionEditor): document client-side fetch exception inlineis the right move for now — a deferred ADR is a known pattern. But the comment says "Open follow-up: ADR: client-side fetch exception for editor suggestion plugins" without a tracker reference. Either open the placeholder issue and link it (single line in the comment), or strike the "Open follow-up" line so it doesn't drift into "this comment promised an ADR that never materialised."Empty-state link (commit
e6844c40) — theborder-t border-lineseparates the "Keine Personen gefunden" message from the link, so visually they read as two units. Good. Thetarget="_blank"rationale (preserve editor state) is valid; therel="noopener"defensive nit is correct. The mousedown preventDefault matches the dropdown-row pattern used for option rows. Solid.LGTM
setEditableguard fix as a separate commit referencing the parent commit (Follow-up to 6ef888a1) — that's the cleanest way to record "the test caught a real bug in my implementation." Don't amend, do the new commit.decoration-ink/50choice aligns the editor pill with the read-mode.person-mentionrule. One contrast story across the whole transcription surface — easier to maintain.Cycle-1 blockers all cleared. Approving on this pass.
🤖 Generated with Claude Code
Markus Keller — Senior Application Architect (cycle 2 re-review)
Verdict: Approved with concerns
Cycle 2 addresses the inline-comment piece of my CLAUDE.md "no client-side API fetch" concern (
49443ad1documents the exception in code), and the deferral of the architectural decision (ADR) is captured by reference rather than left implicit. I appreciate that. The new commits are otherwise localised to the editor surface — no service-layer or domain-model changes — so my architectural review of the listener deletion stands unchanged.What I checked
setEditableguard, contrast tweaks, empty-state link, and disabled-state plumbing are all UI-layer changes. No new repository or service couplings introduced.PersonMentionEditor.svelte:124-137flags the persistent client-side fetch but the call shape is unchanged — same endpoint, same params. Issue #374 (DTO-shape audit) is the right scope for the actual fix.$effectdiscipline: the guard pattern (editor.isEditable !== shouldBeEditable) is the correct response to "external library setter triggers reactivity that re-fires the effect." This is a Svelte-5-specific gotcha; capturing the lesson in the inline comment ("Without the guard, setEditable's own ProseMirror transaction fires onUpdate → ...") is exactly the documentation a future contributor needs.Concerns
The "EXCEPTION to frontend/CLAUDE.md" inline comment is well-written, but it now lives only in
PersonMentionEditor.svelte(one file). A future engineer adding a second editor (e.g. a comment editor with its own typeahead) won't see this comment. Two paths:frontend/CLAUDE.mdunder a new "Imperative client-side fetch — sanctioned exceptions" heading, with this case as the first example.Without one of these, the inline comment is local context only. The architectural rule is repo-wide.
The mention-pill class
'mention-token underline decoration-ink/50 underline-offset-2 text-brand-navy font-medium'is a long string in the renderHTML closure. Six classes inline in a custom Tiptap extension is fine here, but if a third reader (e.g. comment mentions reusing the same pattern) ends up with the same class string, you have a duplication. Consider extractingMENTION_PILL_CLASS = '...'as a module-level constant in eithermentionSerializer.tsor a newmentionStyles.ts. Trivial to do in this PR or as a follow-up; non-blocking.Issue #374 (PersonSummaryDTO leaks
notes) is the right deferral, but the labels you applied aresecurity,P1-high,bug— and the AC list is excellent. One architectural addition I'd want: AC for the API surface boundary itself, e.g. "AC-6: Add an OpenAPI spec test or a Pact-style contract test confirming the typeahead response schema doesn't drift back to includingnoteson a future refactor." That converts "lock down the API surface" from a one-time test into a regression contract. (Optional — issue #374 is already well-scoped.)LGTM
editor.isEditable !== shouldBeEditableguard is a textbook example of "make the effect idempotent." Right pattern, right place.Issue #374 is the cleanest possible deferral: scoped, AC-bounded, security-labelled, P1-high. That's the architectural follow-up I wanted. Approving with the same single concern as before — the architectural rule for "client-side fetch exceptions" needs a single canonical location, not just a per-file comment.
🤖 Generated with Claude Code
Nora Steiner ("NullX") — Application Security Engineer (cycle 2 re-review)
Verdict: Approved
All four cycle-1 concerns acknowledged; three resolved in this PR, one deferred to a properly-scoped follow-up issue (#374). The XSS regression test (
fa7b97ac) is the right shape, and thenotesleak is now tracked under a security-labelled, P1-high issue with bounded ACs that include "lock down the API surface" as an acceptance criterion. That's how I want deferred security concerns to be handled.What I verified
CWE-79 regression test (
PersonMentionEditor.svelte.spec.ts:319-355) — asserts that a sidecar entry whosedisplayNameis<img src=x onerror=alert(1)>does NOT produce a real<img>element with anonerrorattribute. The test correctly checks user-controllable attributes (img[onerror],img[src="x"]) rather than the raw element type, which means it survives Tiptap'sProseMirror-separatorinternal<img>injection without false positives. Felix's note about "also assert literal text content" is a good defense-in-depth — already partially covered by thetextContent.toContain(payload)assertion at line 354.disabledenforcement on the contenteditable (6ef888a1) —editor.setEditable(!disabled)flipscontenteditable=falseon the inner DOM. A keyboard user pressing Tab into a disabled editor cannot type. Thearia-disabled="true"on the wrapper +aria-disabledabsent when enabled is the proper ARIA dual-state. Defense-in-depth for the "client UI lies about state" smell I flagged in cycle 1 — server is still the only authority, but the client now matches.Issue #374 (
notesleak) — opened, labelledsecurity,P1-high,bug. AC-4 specifically calls out "Backend test: PersonControllerTest asserts the typeahead response does NOT includenotes(lock down the API surface — Nora explicitly recommended this)." Exactly the framing I wanted. The shape-aware DTO approach (separatePersonTypeaheadDTO) is the architecturally clean fix; field-level filtering on the wide DTO would have been a band-aid. Good decision.Inline doc on the client-side fetch (
49443ad1) — explicit acknowledgement thatGET /api/persons?q=is the lowest-privilege caller and that the response shape is being audited (#374). Future contributors reading this comment will know "this fetch exists, it has a known field-leak ticket, don't expand its trust surface." Threat model is documented in code, which is rare and welcome.Minor note (non-blocking)
The XSS test currently uses
vi.waitForto wait for Tiptap to render. If Tiptap ever changes its initial-render timing, the test could pass before the malicious payload is even attempted — a false negative. Mitigation: also assert that the textbox'stextContentequals (not just contains) the expected escaped payload, so the test fails if Tiptap silently drops the payload during render. As-is it's adequate; this is a polish suggestion.Out of scope, but worth noting
script-src 'self'; object-src 'none'; base-uri 'self') would make XSS regressions in the contenteditable surface much harder to weaponise even if a future renderer regression slipped through. Track separately if not already.LGTM
notesdeferral was the right call — fixing it inside this PR would have meant either a wide-scope DTO partition or a leaky band-aid; opening #374 keeps both PRs reviewable.aria-disabled+setEditablecombination is the textbook fix for "disabled is visual-only."Approved. Merge-blocking concerns from cycle 1 are all addressed; the remaining
notesleak is properly deferred to #374 with the right labels and ACs.🤖 Generated with Claude Code
Sara Holt — Senior QA Engineer (cycle 2 re-review)
Verdict: Approved with concerns
Cycle 2 added five new tests with strong assertions and removed the dead
personMention.spec.tsthat I'd flagged as coverage padding. The TDD evidence in commitba88febc("guard setEditable effect against re-entry loop") is exactly the kind of test-driven discovery I want to see: the failing tests didn't just verify the spec, they caught an implementation bug (effect re-entry) that would otherwise have shipped. That's the value of writing tests first.What I verified
personMention.spec.tsdeleted (commit4ac94b2f). Coverage no longer pads with tests for unused functions.PersonMentionEditor.svelte.spec.ts:PersonMentionEditor.svelte.spec.tspass.TranscriptionBlock.svelte.spec.ts22/22 still green after the editor-effect changes — the consuming spec proves the new$effectdoesn't break consumers.Concerns (carried over from cycle 1, mostly non-fixed in this PR)
No E2E coverage added for the typeahead — my cycle-1 #1 concern stands. The component spec mocks
fetchglobally, so the wire format between Tiptap'sitemscallback andGET /api/persons?q=is still never exercised end-to-end. This is acceptable for a focused review-fix cycle, but it should land in a follow-up before the next major editor change. Suggested test:frontend/e2e/transcription/mention-typeahead.spec.tstypes@Auginto a real document's transcription block, sees a real backend response, selects a person, saves, reopens, and verifies the mention persists with the typeddisplayName.Disabled-state tests are missing one important case: the transition. All three new tests start the component in a fixed disabled state (
trueorfalse). None tests "render withdisabled=false, type some text, flip todisabled=true, verify the typed text remains AND no further input is accepted." That's the realistic flow when transcription auto-saves and locks the editor. The$effectand its idempotence guard exist precisely to handle this transition — and there's no test for it. This is the test that would have caught the re-entry loop first instead of cascading into TranscriptionBlock failures. Add it as part of a follow-up:mentionSerializer edge cases from cycle 1 not added: empty mention
displayName: '', mention overlap (@Annavs@Anna Schmidtmatching at the same position), Unicode displayName (@Müller,@François), double-newline paragraph splits. None of these are addressed in cycle 2. They're not regressions from cycle 2, but they remain coverage gaps that should land before the editor changes again.XSS test's
textContent.toContainis necessary but not sufficient. Felix and Nora both flagged this. The test asserts the payload appears as visible text, but doesn't assert the payload appears only as text — i.e. that no DOM children of the textbox represent the payload structurally. A stricter assertion:expect(textbox.children).toHaveLength(<expected internal structure>)or assert the deep-clone of the textbox contains zeroElementnodes derived from the payload string. Polish, non-blocking.The pre-existing failures I asked about in cycle 1 were never confirmed against
main. PR description says "DocumentRow tag-click and RichtlinienRuleCard fail on the base branch and are unrelated." I didn't see follow-up Gitea issues created for these pre-existing failures. Track them or fix them — leaving unnamed broken tests onmainrots the green-bar signal across the repo.LGTM
main. That's exactly the workflow I want.6ef888a1(red+green for disabled),ba88febc(green correction for the re-entry loop),fa7b97ac(XSS regression). Each commit is a discrete proof.href="/persons/new") and accessibility (getByRole('link')) — not just "an<a>exists in the DOM." Right-shape assertion.Approving subject to my E2E gap (follow-up issue) and the disabled-state-transition test (also follow-up). Both are tech debt, not blockers.
🤖 Generated with Claude Code
Leonie Voss — UX Designer & Accessibility Strategist (cycle 2 re-review)
Verdict: Approved
All three accessibility issues I raised in cycle 1 are addressed. The disabled state is now real (not visual-only), the WCAG 1.4.11 contrast failures are fixed, and the empty-state escape hatch is back. Cycle 2 is a clean a11y catch-up.
What I verified
Critical —
disabledenforcement (WCAG 2.1.1 / 4.1.2) ✅ Resolvededitor: !disabledat construction (PersonMentionEditor.svelte:86) plus$effect(() => editor.setEditable(!disabled))keeps the contenteditable in sync. Tab no longer lands users in a "disabled" editor that still accepts input.aria-disabled="true"on the wrapper announces the deactivated state to screen readers. Combined with the innercontenteditable="false", NVDA/JAWS/VoiceOver will read "Transkriptionstext, deaktiviert" instead of "edit text, multi line."disabled statedescribe block, lines 299-330). Thedisabled=falsebaseline test catches regressions where someone might accidentally hard-codeeditable: false.Major — mention pill underline contrast ✅ Resolved
decoration-brand-mint(≈1.7:1, fails 1.4.11) →decoration-ink/50(≈6.4:1, passes AA Non-Text Contrast for meaningful indicators)..person-mentionrule (layout.css:343-356), which usestext-decoration-color: color-mix(in srgb, var(--c-ink) 50%, transparent). One contrast story across read and edit modes — that's the right design decision, not just a contrast patch.Major — dropdown highlight ring contrast ✅ Resolved
ring-brand-mint(≈2.5:1 onbg-brand-mint/20, fails 1.4.11) →ring-brand-navy(≈14.5:1, passes AAA).Minor — empty-state "Create new person" link ✅ Restored
MentionDropdown.svelte:125-141reintroduces them.person_mention_create_new()link.target="_blank"preserves the editor's transcription state;rel="noopener"prevents reverse-tabnabbing.min-h-[44px]keeps WCAG 2.5.5 (Target Size) AAA — important for the 60+ transcriber persona on tablets/laptops with stylus input.border-t border-lineseparates the empty-state message from the action — visually two units, semantically two distinct elements.Test coverage for a11y —
PersonMentionEditor.svelte.spec.tsadds:aria-disabled="true"assertion when disabledcontenteditable="false"/contenteditable="true"assertion for both statesThat's the testable a11y surface for the unit-spec layer. The remaining a11y wins (axe-playwright on a real page, screen reader user-journey video) belong in E2E and should fold in with Sara's E2E push.
Suggestions (non-blocking, deferred from cycle 1)
highlightedIndexresets to 0 on every list update — still an issue. When the user types@Aug(highlight = first match) then types@Auge, the highlight jumps back to position 0. If the same person is still in the new list at a different index, the user's expected target moves. Edge case for power users; minor friction. Track separately if not already done.aria-label="Transkriptionstext"for every block — when a screen reader user navigates a long letter with multiple transcription blocks, every block reads as "Transkriptionstext, multi line edit text" with no way to distinguish them. Add the block's index to disambiguate:aria-label={${m.transcription_editor_aria_label()} ${blockIndex + 1}}. Optional — but improves wayfinding.The empty-state link uses
target="_blank"— that's the right pragmatic choice (editor state is preserved), but it does break the typical "Back" muscle memory. Acceptable trade-off; the manual test plan should verify "create person → tab back to original tab → mention dropdown still shows the new person after retyping" flows naturally.LGTM (visual / brand)
decoration-ink/50contrast change unifies edit and read modes under one ink-based underline language. Fewer visual exceptions = easier brand maintenance.ring-brand-navyonbg-brand-mint/20is a strong layered indicator: mint says "row," navy says "you're here." Two distinct states a11y-correctly.font-sans text-sm font-medium,hover:bg-canvas). Consistent affordance language.m.person_mention_create_new()isNeue Person anlegen/Create new person/Crear nueva persona. No translation drift.Cycle-1 critical and major a11y issues all closed. The minor
highlightedIndexcarry-over and the screen-reader block-index suggestion are for follow-ups. Approving.🤖 Generated with Claude Code
Tobias Wendt — DevOps & Platform Engineer (cycle 2 re-review)
Verdict: Approved with concerns
Cycle 2 made no infrastructure or build changes — purely UI/i18n/test-surface fixes. My cycle-1 concerns about Renovate config placement, lockfile hygiene, and bundle size are unaddressed in this PR but most are non-blocking deferrals and the bundle-size note now appears in the PR body.
What I checked
package.jsonandyarn.lockwere not touched. Good.yarn.lock) is still correct.Concerns (carried over from cycle 1; partially addressed)
yarn.lockvspackage-lock.jsonmixing — still unresolved. Per the cycle-2 implementation note from the orchestrator, the user explicitly chose to keepyarn.lockas the project's canonical lockfile. I'll defer to that decision, but two consequences need addressing:frontend/CLAUDE.mddocumentsnpm installas the install command (lines 175-181). If the project uses yarn, this is misleading documentation. Either:frontend/CLAUDE.mdto sayyarn install(single line edit), ornpm installis fine because both lockfiles are committed (then explicitly ignorepackage-lock.jsonin.gitignore, or vice versa).yarn.lockis canonical, CI should beyarn install --frozen-lockfile(notnpm ci). Verify this in.gitea/workflows/.This isn't strictly a blocker for cycle 2 (no lockfile changes were made), but every future PR that touches dependencies will reopen the question.
Renovate config
renovate.json— still at the monorepo root. Cycle 1 question stands: does the self-hosted Renovate runner pick uprenovate.jsonat root, or does it look at.gitea/renovate.json? If the latter, this PR's config is dead. Verify before relying on the@tiptap/*group rule. (Probably worth a single-line check: readinfra/renovate*if it exists.)No CI bundle-size budget enforcement — the PR body says ~85 KB. That's a manual measurement at one point in time. If there's no
size-limitorvite-bundle-visualizerbudget in CI, the next "let me add a date picker library" change adds another ~50 KB and nobody notices until users on slow connections complain. Optional follow-up:npx size-limitintegration with a budget per chunk. (Skip if the team's bandwidth threshold tolerance is already lax.)LGTM
src/tree ormessages/JSON. Nothing for the deploy pipeline to worry about.Cycle-1 deployment concerns: still 1 follow-up to do (
yarn.lockvsnpmdocumentation drift, or CI installer audit). Cycle 2 itself adds zero deployment risk. Approving with the same minor lockfile concern from cycle 1.🤖 Generated with Claude Code
Elicit — Requirements Engineer (cycle 2 re-review)
Verdict: Approved
Every cycle-1 OQ has either been resolved in this PR or properly deferred with traceability. The PR description now contains the AC mapping table I asked for (AC-1 through AC-7, each with status and "where covered"), the bundle-size note Tobias requested, the cycle-2 fix log indexed by reviewer comment, and the deferred concerns linked to issue #374. The Definition-of-Done gating items from my cycle-1 review are addressed.
Open Questions — cycle 2 status
OQ-373-01 (AC coverage gaps for AC-2/3/5) ✅ Resolved
PR description Table § "AC mapping" now lists all seven ACs with status and test coverage. AC-2 (mention pill renders distinct), AC-3 (life dates in dropdown), AC-5 (keyboard nav) are explicitly mapped to specs and the relevant component code. Future archeology has a one-table source of truth.
OQ-373-02 (Manual test plan items unchecked) ⚠️ Still pending
Two unchecked manual items remain:
These verify the core promise of issue #372. They cannot be skipped before merge. Per the orchestrator instruction these are explicitly user tasks (left for the human reviewer to verify after deploy-to-staging). Traceability: the items are still listed in the PR test plan, and AC-1 + AC-6 unit tests cover the same invariants in unit form. The manual run is the integration-level check on top of that.
OQ-373-03 (
transcription_editor_aria_labelwording) Unchanged in cycle 2Translation strings still read "Transkriptionstext" / "Transcription text" / "Texto de transcripción" — fine, screen readers will say "Transkriptionstext, edit text, multi line" or similar with the textbox role. Defer if Leonie's team finds it acceptable; track only if a transcriber complains. Non-blocking.
OQ-373-04 (Empty-state "Create new person" link removed) ✅ Resolved
Restored in commit
e6844c40. The translation keym.person_mention_create_new()is no longer orphaned. Empty-state UX regression fixed; spec confirms the link renders withhref="/persons/new".NFR Checklist — cycle 2 update
error_person_rename_conflictremoved in all three locales.personMention.ts) removed. Idempotence guard onsetEditableeffect documented inline.Definition of Done — gating items, cycle 2
error_person_rename_conflictdeleted or kept with a justification. Done — deleted in all three locales (commit94d07334).Two open DoD items: both are "outside this PR's scope by design" (manual user verification on staging; E2E tooling expansion). Neither blocks merge of cycle 2 fixes.
Issue traceability
notesleak. Properly labelledsecurity,P1-high,bugwith bounded ACs.LGTM
#5615,#5616,#5618,#5621,#5623,#5624). Auditable line-by-line.Approving. The remaining open items are user-verifiable (manual tests on staging) and a tracked follow-up (E2E typeahead coverage). Cycle-2 fixes meet the original requirements traceability bar.
🤖 Generated with Claude Code
Cycle-2 implementation summary
Two review/fix cycles run (1 of the 8-cycle budget consumed in cycle 2; cycle 3 was a re-review with zero blockers and is the loop exit).
Per-cycle "what changed"
Cycle 2 — implementation cycle (8 commits):
4ac94b2fpersonMention.ts/spec — orphaned after Tiptap migration94d07334error_person_rename_conflictfrom de/en/es6ef888a1editor.setEditable(!disabled)reactively +aria-disabledon wrapper (WCAG 2.1.1)fa7b97ac<img>/<script>from malicious displayNameba88febceditor.isEditable !== shouldBeEditableto prevent re-entry loopf1932fd5decoration-ink/50, dropdown ringring-brand-navye6844c4049443ad1Plus PR description rewrite with full AC mapping (AC-1 through AC-7), cycle-2 fix log indexed by reviewer comment, bundle-size note, and reference to the new issue #374.
Cycle 3 — re-review: All 7 personas re-reviewed. All cycle-1 blockers and major concerns resolved. Remaining concerns are all non-blocking deferrals (E2E coverage, ADR placement, lockfile audit, mentionSerializer edge cases) tracked as carry-overs.
Items deferred to user (with reason)
notesleak) — deferred per orchestrator instruction ("if it leaks, that's a separate scope and should be deferred to a new issue"). Created withsecurity/P1-high/buglabels and 5 bounded ACs.Final test status
npm run check: zero NEW type errors in files touched by cycle 2 (mentionSerializer.spec.ts has 1 pre-existing error from the original PR'sas PersonMention[]cast on areadonlytuple — unchanged).Final commit count on the branch
8 cycle-2 commits on top of the 16 original PR commits = 24 commits total on
feat/person-mentions-issue-362-frontend-b2.Final verdicts (cycle 3)
Zero "Changes requested" verdicts. All cycle-1 blockers cleared. Remaining "Approved with concerns" verdicts cover non-blocking follow-up tech debt that is either out of scope (Tobias's lockfile/CI audit, Sara's E2E push) or properly tracked elsewhere (Markus's ADR, Nora's #374). The PR is ready for the user's manual verification + merge decision.
🤖 Generated with Claude Code