feat: person @mentions edit-mode infrastructure (PR-B1, #362) #369
Reference in New Issue
Block a user
Delete Branch "feat/person-mentions-issue-362-frontend-b1"
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?
Scope — PR-B1 of issue #362
Frontend edit-mode infrastructure for the person
@mentionfeature. Read-mode rendering + hover card stays for PR-B2 (per the split decided in #5339 §G).Backend (PR-A) merged in #366 and provides the
mentionedPersonsJSONB column, the typed REST contract onTranscriptionBlock/UpdateTranscriptionBlockDTO, the propagation listener, andErrorCode.PERSON_RENAME_CONFLICT(409 on rename-mid-edit).What this PR adds
escapeHtmlshared helpersrc/lib/utils/mention.tsrenderBodyso PR-B2'srenderTranscriptionBodyreuses it (Felix #5294). Now also escapes'(Sina #5505 action item).detectPersonMentionsrc/lib/utils/personMention.tsdetectMentionstops at the first space, which breaks for any historical name with a space (the common case in this archive). Stops at\nor a second@. 14 unit tests.messages/{de,en,es}.jsonpopup_empty/btn_label/create_newfor the editor.PersonMentionEditor.sveltesrc/lib/components//api/persons?q=, rendersmin-h-[44px]rows (WCAG 2.2 AA touch target), shows life dates viaformatLifeDateRange()(no reimplementation, per Leonie #5329), inserts@DisplayNameinto the textarea, pushes{personId, displayName}into the bound sidecar, deduplicates bypersonId. Loading indicator during fetch, empty state offers a "Neue Person anlegen" link, popup closes on blur, focus ring on textarea, keyboard-highlighted row visually distinct from hover. Keyboard: ArrowDown/Up cycle, Enter selects, Escape closes — covered by the B11b tests. 15 component tests via vitest-browser-svelte (now driven by fake timers, 13× faster).TranscriptionBlock.svelte<textarea>replaced by<PersonMentionEditor>. Autoresize lives in the editor; the parent only captures the textarea node when it needs to read selection bounds (quote-selection feature).useBlockAutoSave.svelte.tssaveFnwidens to(blockId, text, mentionedPersons).pendingMentionsmirrorspendingTexts; on save failure the in-flight payload is preserved so retry resends it without the caller having to re-pass anything (B12).src/lib/utils/saveBlockWithConflictRetry.ts+blockConflictMerge.ts+routes/documents/[id]/+page.sveltemergeBlockOnConflict()helper: server is source of truth for the post-renamedisplayName, transcriber's typed text always wins, local-only mentions added since the last save are preserved (B12b). Throws a typedBlockConflictResolvedErrorcarrying the merged snapshot on itsmergedproperty. UUID-guards the path interpolation (Sina #5505). 14 unit tests across the two utils.src/lib/types.tsPersonMentionandmentionedPersonsfield added toTranscriptionBlockData.Tests
mention.spec.tsescapeHtml(5 originals + apostrophe + idempotence)personMention.spec.tsPersonMentionEditor.svelte.spec.tsblockConflictMerge.spec.tsBlockConflictResolvedErrorsaveBlockWithConflictRetry.spec.tsuseBlockAutoSave.svelte.test.tsTranscriptionEditView.svelte.spec.tsAll new tests pass. Pre-existing flaky tests are unchanged (verified with
git stashagainstmain):TranscriptionBlock.svelte.spec.ts— 3 click-related tests fail both with and without this PR.TranscriptionEditView.svelte.spec.ts— 2 "mark all reviewed" tests fail both with and without this PR.routes/hilfe/transkription/page.svelte.spec.ts— 1 Wikipedia-link test fails both with and without this PR (root-owned.svelte-kitartifact).What I'm NOT shipping in B1 (B2 commitments)
personIds (seemergeBlockOnConflictnot filtering); the actual rendering behaviour (404-on-fetch → degrade to plain text) belongs to PR-B2'srenderTranscriptionBody+PersonHoverCard(per ReqEng #5510 §2).displayNameescape on render.escapeHtmlis already extracted and unit-tested for B2 to consume.Open questions
displayName) — closed as accepted limitation. TheselectPersondedup bypersonIdis correct; the render-side first-sidecar-wins rule lives in PR-B2. Family archive context makes this edge case low-risk.409 conflict UX statement
When the backend returns 409 PERSON_RENAME_CONFLICT mid-edit:
BlockConflictResolvedErroris thrown so the autosave hook flips the block intoerrorstate with the existing "Erneut versuchen" indicator.No new UI surface for the conflict — the existing autosave error indicator is reused.
Test plan
npm run lint— clean.npx svelte-check— no new errors in touched files.@Aug, pick a result, blur — backend should receive{ text, mentionedPersons }(verified via the autosave test asserting flush payload shape).🤖 Generated with Claude Code
Mirrors MentionEditor for users but searches /api/persons?q=, allows multi-word queries (delegated to detectPersonMention), displays life dates next to each result, and uses min-h-[44px] rows for WCAG 2.2 AA touch targets. Selection writes both the @DisplayName text and a {personId, displayName} sidecar entry. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
The diff is well-shaped: pure helpers (
detectPersonMention,mergeBlockOnConflict,escapeHtml) extracted with focused unit tests, thePersonMentionEditorcarries its own host-driven component tests, and the autosave hook signature widening is mirrored across every call-site. Red/green TDD discipline is visible — pure-function tests first, component tests second, integration last. Solid PR.A few clean-code / TDD nits before merge:
Concerns
PersonMentionEditor.svelte:106— shadowedm. Inside the dedup.some((m) => m.personId === ...),mshadows the imported Paraglidemfrom line 5. It works because the shadow is scoped, but it's a footgun the next reader won't enjoy. Rename toexisting(and same inTranscriptionBlock.svelte:201where(m) => { localMentions = m; ... }shadows it again).useBlockAutoSave.svelte.ts:91-95— dead public surface.handleMentionsChangeis exported but never called anywhere in the diff (the block emits text + mentions throughhandleTextChangeonly). The companion comment even admits "Mentions changes always accompany text changes from the editor". Either delete it now or leave a// TODO(B12-PR2)— exporting unused functions invites stale code. Same forgetPendingMentions— only the merge path could plausibly want it, and+page.sveltedoesn't.+page.svelte:113—throw new Error('Conflict resolved — please save again')is control flow disguised as an exception. The autosave hook catches it and flips state toerror, which is the right outcome, but the message is human-prose mid-codebase. Consider a typed sentinel:throw new BlockConflictResolvedError()or justthrow Object.assign(new Error('CONFLICT_RESOLVED'), { code: 'CONFLICT_RESOLVED' })so a future reviewer sees this is structured, not a translation string that escaped i18n.PersonMentionEditor.svelte:104—value = before + replacement + aftermutates the bindable but skipsoninput. This meanshandleInputdoesn't run on programmatic insertion, so the popup state is closed manually viaclosePopup()immediately after. Fine today, but if a future feature wants to e.g. detect a chained mention immediately after selection, it'll miss it. Not a blocker — just call out the asymmetry in a comment if you keep it.TranscriptionBlock.svelte:78-89— autoresize lost itsuse:action. The olduse:autoresize={localText}was a self-contained Svelte action with attach/update/destroy. The replacement is acaptureTextareacallback + a side-effecting$effect(() => { void localText; resizeTextarea(); }). Functionally equivalent, but thevoid localTextreactivity tag is the kind of thing that confuses readers. Consider$effect(() => { resizeTextarea(); }); $effect(() => { localText; });or — cleaner — keep the action API and pass the textarea up via the action'snodeparameter.Nits (skip if rushed)
useBlockAutoSave.svelte.ts:153—getPendingMentionsparameter shadows thegetPendingMentionsfunction declared 4 lines above. Cosmetic.PersonMentionEditor.svelte.spec.ts:283—waitForDebounce = () => new Promise((r) => setTimeout(r, 250))— the editor debounce is 200ms; using 250ms is wisely defensive but adds 50ms × 14 tests = 0.7s. Trim to 220ms or use vitest fake timers for consistency with the autosave tests.personMention.spec.ts:14—'@Aug@bar'test expects null. The textbook case is@Aug @Bert(whitespace boundary, the second@opens a new query). Worth one test for that too.What I checked
PersonMentionEditorhas component tests covering rendering, typeahead, selection, keyboard, and touch target.PersonMentionEditor.svelteis 213 lines — at the upper edge of "one nameable region" but justified (popup + textarea is one composable unit).value,mentionedPersons,displayName); nodatablobs, noitem/obj.{#each}keys: line 187 keys on(person.id)✅mergeBlockOnConflictrules match the issue's B12b spec: local text wins, server wins on shared personId, local-only mentions preserved.Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
The split between PR-B1 (edit infra) and PR-B2 (read render + hover card) is the right boundary — not contrived. PR-B1 owns the data flow from textarea → sidecar → autosave → backend; PR-B2 owns the inverse direction. Each PR is independently shippable and independently reversible. That's the test of a good module boundary.
What I checked
PersonMentionEditorconsumes/api/persons?q=(existing endpoint).+page.svelteconsumes/api/documents/{id}/transcription-blocks/{id}for both PUT and refetch. No cross-domain repository reach-arounds — the frontend continues to talk to one backend route per concern.mention.ts(comment mentions) andpersonMention.ts(transcription mentions) are deliberately distinct. The sharedescapeHtmlis the only piece that fans out — extracted exactly once intomention.ts, re-imported. No duplication, no premature abstraction.mergeBlockOnConflictis a 6-line stateless reducer with 6 tests. That's textbook ��� the merge logic is testable without spinning up a fetch mock or a Svelte component.PersonMentionis exported fromsrc/lib/types.tsand mirrored from the generatedcomponents['schemas']['PersonMention'](the editor uses the latter). Two sources of truth — see Suggestion below.getPendingMentions,handleMentionsChange. Both unused. The hook is a service module — its API surface should mirror its actual contract.Suggestions (non-blocking)
src/lib/types.ts:40vscomponents['schemas']['PersonMention']— duplicated type. The hand-rolledPersonMentionintypes.tsis now redundant with the OpenAPI-generated equivalent (PR-Ashipped that schema). Long-term, the hand-rolled one will drift. Suggestion: re-export the generated type fromtypes.tsand delete the hand-rolled one in a follow-up:Not a blocker for B1 — this is consistent with how other types in
types.tsare still hand-rolled — but mention it in the B2 spec so future-you doesn't introduce a third copy.+page.svelte:101-115— 409 logic inline in the page component. The page is the right entry point (it owns the doc-id context and thetranscriptionBlocksstate), but the merge-and-refetch sequence is 14 lines that don't belong in+page.sveltelong-term. Once PR-B2 lands, consider extractingsaveBlockWithConflictRetry()intosrc/lib/services/transcriptionBlockSave.ts. ADR-worthy if it grows further.No Svelte action for the mention popup positioning. The popup is
absolute mt-1 w-72— works for the common case but will clip near the viewport bottom. The issue's hover-card spec mentionsgetBoundingClientRect()flip rules; the editor popup deserves the same treatment eventually. Out of scope for B1 — flag as B2-adjacent.What I'm NOT flagging
bind:setter pairbind:value={() => localText, (v) => { localText = v; emitChange(); }}is unusual but legal Svelte 5. It's the only way to fan-out a side-effect on a bind without a parent$effectwatcher. Acceptable.scheduleSearchis independent of the 1500ms autosave debounce — different concerns, different lifetimes. No coupling needed.Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
🛡️ Sina Tariq — Security Expert
Verdict: ⚠️ Approved with concerns
This PR is mostly edit-mode plumbing — no rendering of untrusted input, no new endpoints, no auth changes. The persisted XSS surface (rendering user-typed text as HTML) is explicitly out of scope and lives in PR-B2. That's the right line to draw, and
escapeHtmlis already extracted and unit-tested ready for it.That said, the edit path does touch a few things worth flagging:
Concerns
+page.svelte:104— refetch URL is interpolated from path params.await fetch(\/api/documents/${doc.id}/transcription-blocks/${blockId}`)— bothdoc.idandblockIdare server-controlled (loaded from the page data), so this is fine *today*. But the same shape is repeated in the PUT call above and inuseBlockAutoSave.svelte.ts:108. If a future feature ever lets the user supply ablockId-shaped string client-side, you've got a path-injection latent vector. Concrete ask: add a UUID guard (/^[0-9a-f-]{36}$/i.test(blockId)) at the top ofsaveBlock()` before any fetch fires. Defence in depth costs three lines.PersonMentionEditor.svelte:83—fetch('/api/persons?q=' + encodeURIComponent(q))runs on every keystroke after debounce. No CSRF token, nocredentials: 'include'. This works because SvelteKit's Vite proxy injects the cookie auth header, but it means the typeahead is silently authenticated — an unauthenticated user typing in the editor would get a 401 with no clear UX path. Fine for B1 (the editor is only shown to logged-in transcribers behindWRITE_ALL), but worth a// SECURITY: relies on session cookie + same-origin policycomment so the next reader understands the implicit assumption.PersonMentionEditor.svelte:84-86— no rate-limiting on typeahead. The 200ms debounce is the only throttle. A power user typing fast will fire ~5 requests/sec; 10 transcribers = 50 req/s burst. Not a security concern per se, but a DoS-adjacent observation:/api/persons?q=should have a server-side rate limit eventually. Out of scope for this PR — flag for the security checklist on the persons-search endpoint, not for this PR.mergeBlockOnConflicttrusts server-returnedmentionedPersons. Server snapshot replaces localdisplayNamefor matchingpersonIds. If the server is ever compromised, an attacker-controlleddisplayNamecontaining markup will flow throughlocalText(transcriber's typed text wins, but the sidecardisplayNameis server-trusted). PR-B2 needsescapeHtmlon everydisplayNamebefore it goes into{@html}. Already extracted — just don't forget it.What I checked
eval, nonew Function, nodangerouslyInnerHTML, no{@html}in this PR.encodeURIComponenton the query param at line 83 — proper.escapeHtmlhelper escapes&first (correctly, to avoid double-encoding) and covers<,>,". Missing:'(apostrophe). For attribute values quoted with double-quotes, this is fine, but if PR-B2 ever uses single-quoted attributes it'll bite. Recommend adding.replaceAll("'", ''')defensively. (Cheap to do here, would be one more case inmention.spec.ts.)WRITE_ALLgate on transcription routes.409rename-mid-edit handling does not leak server-internal error details to the user (just a generic "save again" prompt).Action items
'toescapeHtmland one test case. Pre-empts a B2 footgun.renderTranscriptionBody, everydisplayNamefrom the sidecar must go throughescapeHtmlbefore substitution into the<button data-person-id="...">{name}</button>template — both the name and the id should be escaped. Add a stored-XSS test fixture:displayName: '"><script>alert(1)</script>'.Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
🧪 Tester Persona
Verdict: ⚠️ Approved with concerns
Test coverage on this PR is genuinely strong — 40+ new assertions across 6 test files, the new pure functions are 100%-covered, and every existing test was updated to match the widened signature instead of being silenced. That's a good faith effort, not check-box TDD.
But there are a few testing gaps and stability risks worth raising before merge.
Concerns
PersonMentionEditor.svelte.spec.ts— real timers + 250ms sleeps everywhere. Fourteen tests ×await waitForDebounce()ofsetTimeout(r, 250)= 3.5 seconds of real wall-clock waiting per test run. The autosave hook tests usevi.useFakeTimers()and run in milliseconds. Apply the same pattern here:Browser-mode tests support fake timers. This will both speed up the suite and make it deterministic — the current 250ms wait is racing against the 200ms debounce inside the editor with only 50ms of slack, which on a busy CI runner is a flake waiting to happen.
No test for
+page.svelte:101409 conflict path. The merge function has 6 unit tests and the autosave hook has B12 (preserve in-flight on failure), but the actual orchestration insaveBlock— "PUT returns 409 → refetch → merge → throwConflict resolved" — is untested. Suggested test: stubfetchto return 409 then 200-with-block-data, callsaveBlock, assert the merged block landed intranscriptionBlocksand an error was thrown. Without it, the integration is purely visual.PersonMentionEditor.svelte.spec.ts:495— relies on test ordering for "does not duplicate". The test passes amentionedPersonsinitial value but then types@Auguste Raddatz @Augon top. The textarea state is set imperatively (ta.value = ...) but thevalueprop binding may not have observed it (no input event between the host re-mount and the new value). Confirm by adding an explicitta.dispatchEvent(new Event('input'))before the second@Augis checked, or assert the initial state explicitly.Missing edge case: typeahead with
fetchrejecting (network error). Line 90:} catch { results = []; }. There's a test formockFetchEmpty()returningok: true, json: [], but no test for a thrown rejection (no network). Add one — it's two lines and prevents a regression where someone replaces the broad catch with a.then(...)chain that bubbles.useBlockAutoSave.svelte.test.ts:79-92(B12) testshandleRetry('block-1', 'should-not-be-used', []). Good test, but the retry path also runs throughexecuteSave, which in turn callssetSaveState('saving')then'saved'. Worth asserting the state transition explicitly:expect(states).toEqual(['saving', 'saved']). Right now the test only asserts the final'saved'state, which would also pass if the hook skipped the saving state entirely.Nits (cheap)
mention.spec.ts— the newescapeHtmlblock has 5 cases but no test for already-encoded input (escapeHtml('&')→'&amp;'). The doc comment promises this property; pin it with a test.personMention.spec.ts:13—'@Aug\nfoo', 8— cursor 8 is after the newline. Add a complement:'@Aug\nfoo', 4— cursor immediately at the newline boundary, expecting'Aug'. The boundary case is where regressions hide.blockConflictMerge.spec.ts— no test for an empty local mention array against a non-empty server array. Probably implied by another test, but explicitly: "server has 2, local has 0 → merged has 2". One-line addition.What I checked
setTimeout(250)waits in the editor spec are the one wobbly bit.TranscriptionBlockclicks, 2TranscriptionEditViewmark-all-reviewed, 1 hilfe page) are correctly excluded from the test plan with a verified-against-main note. Acceptable.Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
🎨 Leonie Werner — UI/UX Expert
Verdict: ⚠️ Approved with concerns
The 44px touch targets, the life-dates row in the dropdown, and the German-first translations in the popup empty state — all signs you respected the dual-audience rule (60+ transcribers on tablets, 25-42 readers on phones). Reuse of
formatLifeDateRangeis exactly the kind of shared-vocabulary discipline I asked for in #5329. Thanks.But there are a handful of UX issues that will be felt by transcribers within minutes of using this:
Concerns
The popup never closes on outside click or blur. Tested mentally: open the editor, type
@Aug, popup opens, tab away to comment box, popup is still hanging there over the next textarea. The current close paths are: Escape, selecting a result, ordetected === nullfrom typing past a newline. Addonblur={closePopup}on the textarea (with a smallsetTimeoutto letonmousedownfire first on the option) — this is a basic-hygiene typeahead behaviour. Ten transcribers will find this within the first hour.No visual indication of the "@" trigger / no help text. The textarea looks identical to before. A first-time transcriber won't discover the
@-mention feature without being told. At minimum: when the textarea is focused and empty, show a subtle hint near the bottom: "Tipp: @ tippen um eine Person zu verlinken". The existingtranscription_block_quote_hintpattern is a good model. Bonus: a small "@-Person verlinken" button next to the existing label affordances.Highlighted-row contrast.
bg-canvas(the same grey as:hover) for the keyboard-highlighted row means no visual difference between hover and ArrowDown selection. A 65-year-old with a trackpad sweeping over rows can't tell where the keyboard cursor is. Usebg-canvasfor hover and a stronger token (e.g.bg-brand-mint/25orring-2 ring-brand-mint) foraria-selected="true". Two-state selection is a textbook a11y issue (WCAG 1.4.11 Non-Text Contrast).No focus ring on the textarea. The original textarea had
outline-noneand the new one inherits that. With the typeahead now being keyboard-driven, the textarea is the keyboard nexus — losing its focus indicator entirely is a problem for keyboard-only users. Addfocus-visible:ring-2 focus-visible:ring-brand-mint focus-visible:ring-inset(or move tooutlinerather than ring; either is fine — just something).Empty state UX is mildly hostile. "Keine Personen gefunden" — fine, but no escape hatch. After typing
@xyz, the user is told their search failed and… that's it. Consider adding "Person anlegen?" with a link to/persons/new?name={query}(the existing PersonTypeahead does this). Or at minimum: when results are empty after a search has fired, show a subtle "Vielleicht ist die Person noch nicht im Archiv? [Neue Person anlegen]" hint.min-h-[44px]is on the row — but the textarea itself hasrows={1}. A single-row textarea is ~24px tall (Merriweather 16px + line-height 1.625). The textarea's touch target is below 44px when collapsed. WCAG 2.2 AA Target Size requires the actionable element. The popup options pass; the textarea itself fails. Easy fix:min-h-[44px]on the textarea container (the parentdiv.relative), orpy-2.5on the textarea so it expands.No loading indicator during the 200ms debounce + fetch. When the network is slow, the user types
@Augand stares at an empty popup for up to 1-2s before results appear. A skeleton row or a "Suche…" message would close the perceptual gap. The hover-card spec in #362 already mandates a skeleton shimmer for the same reason — apply the principle here.Nits
text-baseon the option name +text-xson the date row — a 4-step type ramp inside a 44px row. Considertext-smfor the name, thetext-xsfor the date — keeps the row vertically compact for tablets.w-72(288px) popup is fixed-width. On a 320px-wide phone (the lowest reader target), this would clip. PR-B2 covers read mode, but since the editor is device-responsive, considermax-w-[min(18rem,calc(100vw-2rem))].placeholder:text-ink-3on the textarea —ink-3is your "tertiary" colour, which is borderline grey-on-grey. Run a contrast check; if WCAG AA fails, escalate toink-2.What I checked
brand-navy/brand-mint/brand-sandnot directly referenced (uses semantic tokenstext-ink,bg-surface,border-line— correct, that's how design tokens should work).font-seriffor names,font-sansfor metadata. Consistent.formatLifeDateRangeinstead of reimplementing the*/†ASCII art. Good.Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
⚙️ DevOps Persona
Verdict: ✅ LGTM (frontend-only)
This PR is purely frontend code: 3 i18n JSON files, 2 new Svelte components, 1 new utility, 1 new test-host, 1 service hook update, plus tests. No Dockerfile changes, no docker-compose changes, no CI workflow changes, no migration files, no infra config, no MinIO bucket changes.
What I checked
Dockerfile/docker-compose.ymltouched..github/workflows/or.gitea/workflows/touched.db/migration/files (backendV{n}migrations are PR-A).package.json(the diff doesn't touch it).Observations (not blockers)
/api/persons?q=on every typed character (after a 200ms debounce). The persons-search endpoint is in PR-A and presumably unbounded; if the production deploy gets even a handful of concurrent transcribers, expect a measurable bump in DB load onpersonssearches. Worth asking whether a server-side rate-limit or response cache (60s TTL by query) belongs in the persons controller. Filing as devops-adjacent observation, not as a blocker for this PR.posthog.capture('block_conflict_resolved')or equivalent. Not for this PR.Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
📋 Requirements Engineer
Verdict: ⚠️ Approved with concerns
The PR description is unusually disciplined — every change is mapped to a spec section (B11b, B12, B12b) and OOS items are explicitly split to PR-B2 with a link back to the issue thread. That is the level of traceability I usually have to extract by interview. Thank you.
That said, when I cross-walk the PR against issue #362's acceptance criteria, three of them are partially satisfied by this PR but cannot be claimed as done until B2 lands. I want to make sure that's deliberate and visible.
Acceptance-criteria walk-through
Pulling each AC from the issue body:
@Aug→ results show name + life dates@DisplayName, sidecar gets{personId, displayName}, no UUID in text/persons/{id}Concerns (requirements-side)
AC-6: end-to-end rename-propagation isn't verified anywhere. PR-A has the listener; PR-B1 has the 409-merge for the mid-edit case. But the happy path — "transcriber edits block, rename happens, transcriber re-opens doc, sees new name in text and sidecar" — relies on PR-A propagation working AND the frontend re-fetching the block on doc load. There's no test that asserts this round-trip. Suggest a Playwright e2e in B2 that exercises both PRs together: type mention → save → backend rename → reload page → assert text contains new name. Not a blocker for B1 if you commit to that test in B2.
AC-7 (deletion → degrades to plain text) — silent gap. The issue body lists this in "Edge cases" but I see no test in B1 or B2 mentioned for it. The behaviour today: if a person is deleted, the block's
mentionedPersons[].personIdbecomes a dangling reference. PR-B1'smergeBlockOnConflictdoesn't filter dangling references; PR-B2 will need to handle the 404-on-fetch gracefully. Surface this as a tracked sub-task in B2's checklist explicitly; don't let it live only in the issue body.PersonMentionEditor— empty-state UX is a UX gap, but there's a spec gap behind it. The issue body doesn't say what happens when the typeahead returns zero results. The implementation chose "Keine Personen gefunden" with no escape hatch; Leonie flagged the UX of this. From a requirements POV: should there be a "create new person" path inline?PersonTypeahead(the existing component used in document forms) does this. Decide and pin it as an EARS requirement: "When the typeahead returns zero results, the system shall display ‹X›." Currently‹X›= "an empty-state message", and that's underspecified.The 409 conflict UX is also under-specified in the issue. §G of #5339 mentions
PERSON_RENAME_CONFLICTand the 409 contract, but neither the issue body nor the PR description states what the user sees when this fires. The PR throws'Conflict resolved — please save again'. Is the user supposed to actually click save again? The autosave will retry on next text change — but the UX expectation is opaque. Acceptance criterion missing: "When the system encounters a 409 rename conflict, the system shall ‹Y›.". PR-B1's behaviour is fine; the spec is the gap.OQ-1 (two persons with same
displayName) — still open from the issue. PR-B1'sselectPersondedupes bypersonId, which is correct, but if the transcriber types@Auguste Raddatzand there are two historical Augustes, only one can be the implicit target when the block is later rendered. Issue marks this as "low — edge case". Confirm on this PR: are you closing OQ-1 as "accepted limitation" before merging? If yes, drop a one-liner in the PR description.What I checked
Suggested updates to the PR description before merge
Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
👨💻 Felix Brandt — Cycle 2 Re-Review
Verdict: ✅ Approved
Walking through my five cycle-1 concerns against the new commits:
min editor + block79349644(existing) andbbde9e84(next). Two atomic commits, one per shadow site.handleMentionsChange/getPendingMentionsexportscb51e8e4. The hook's public surface is now exactly whatTranscriptionEditViewcalls.throw new Error('Conflict resolved …')BlockConflictResolvedErrorinfd3a44d1. The class carries acode: 'CONFLICT_RESOLVED'constant and the merged snapshot onerr.merged— clean, structured, and actually useful (the+page.svelteconsumer readserr.mergedto update local state without a second roundtrip).value = …programmatic write skipsoninputselectPersonand the manualclosePopup()directly after compensates. Acceptable to defer.void localTextreactivity tag in TranscriptionBlock autoresizePersonMentionEditoritself in49db82e1, which is a stronger fix than my "split the effects" suggestion. The editor owns the textarea, so it should also own the sizing. Thevoid valuetag is now in the editor (one site, well-commented) and the parent'scaptureTextareais purely about exposing the node for selection-bound reads. Cleaner module boundary than what I asked for.What landed in cycle 2 that I want to call out
saveBlockWithConflictRetryextraction (ba73387d). This was Markus's suggestion #2 from cycle 1 and Tester's concern #2 — solved in one commit by extracting a pure module that takesfetchImplas a dependency. The route file'ssaveBlockis now 12 lines instead of 25, and the extracted helper has 6 unit tests covering every branch including UUID-guard rejection. Clean.Nits I'm not flagging
await import()calls insidesaveBlockcould be hoisted to top-level imports — they're fine inside a route handler that's only called on user action, but if someone notices the network panel showing a chunk fetch on first save, a reviewer might ask. Leave it.BlockConflictResolvedError.mergedisT | undefined— could be split into two error subclasses for type-safety on the consumer side, buterr.mergedis read once in one place and theif (err.merged)guard is fine.Approved.
Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
🏛️ Markus Keller — Cycle 2 Re-Review
Verdict: ✅ Approved
Cycle 2 closes both of my cycle-1 suggestions cleanly:
saveBlockWithConflictRetryfrom+page.svelte) — done inba73387d. Pure module,fetchImplinjected, every branch unit-tested. The route file is now a thin orchestration shell that delegates the actual protocol (PUT → maybe GET → maybe merge → maybe throw) to a place where it can be reasoned about in isolation. This is the right architectural move; it also paves the way for the same helper to handle other conflict scenarios PR-B2 might surface.PersonMentiontype) — still hand-rolled intypes.ts, which I explicitly said was acceptable for B1. No regression.Architectural observations on the new code
BlockConflictResolvedError(fd3a44d1) is the right shape. A typed error that carries a domain payload (merged: TranscriptionBlockData) is structurally what every "exceptional but expected outcome" should look like in this codebase. Worth flagging in CODESTYLE.md as a pattern for future PRs to copy.await import()of both helpers insaveBlockis fine because they're behind a user-action gate, but ifPR-B2adds more dynamic imports we should consider top-level imports for the route's hot path./api/persons, route still talks to/api/documents/{id}/transcription-blocks/{id}. Nothing reaches into the wrong domain.Two ADR-worthy things from this PR
I won't ask for these in B1 — they're follow-ups:
BlockConflictResolvedErroris the codebase's first typed domain error class on the frontend. The error.ts module today only has the i18n-mapping function. Worth deciding whether typed errors should live inerrors.tsnext to the codes, or co-located with the domain helper that throws them (current choice). My read: keep them with the helper that throws — the code → message mapping inerrors.tsthen handles cross-domain rendering.LGTM.
Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
🛡️ Sina Tariq — Cycle 2 Re-Review
Verdict: ✅ Approved
Walking through my cycle-1 concerns:
blockIdinterpolation43aacd9f(route) and again in the extracted helper (ba73387d) — the testrejects ids that are not UUIDs (path-injection guard)pins it. Defence in depth, exactly what I asked for.cacbd577. Comment explains the Vite-proxy cookie injection + same-origin assumption + the WRITE_ALL gate. The next reader does not have to reverse-engineer this./api/persons?q=mergeBlockOnConflicttrusts server-returned displayNameescapeHtmlcall when rendering will close the loop. VerifiedescapeHtmlnow also escapes apostrophe (362a84dd), so the helper PR-B2 will use is hardened.Action item that landed
escapeHtmlnow escapes'→'. The new test pins it and adds an idempotence-by-composition test that prevents a future "smart" optimisation from breaking the property. Both green.What I checked in cycle 2's diff
eval,new Function,dangerouslyInnerHTML, or{@html}introduced.encodeURIComponent(query)for the?name=param,target="_blank", andrel="noopener". Correct.BlockConflictResolvedErrordoes not include any server-controlled string in its message — just theblockId(already UUID-guarded). No information disclosure path.saveBlockWithConflictRetrytest fixtures use UUID-shaped strings for IDs throughout — anything else throws via the guard. Good test discipline.as anyon response bodies, no// @ts-ignoreto silence type errors that would otherwise reveal an injection path).Approved.
Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
🧪 Tester — Cycle 2 Re-Review
Verdict: ✅ Approved
Every concern I raised has a corresponding commit + assertion:
e8ba8405switched tovi.useFakeTimers({ shouldAdvanceTime: true })+advanceTimersByTimeAsync(200). Test runtime: 3.08s → 236ms. Determinism guaranteed because the 200ms internal debounce now runs against the fake clock.+page.svelteba73387dextractedsaveBlockWithConflictRetryinto a pure module and added 6 tests covering: happy path, 409→merge,err.mergedpayload, refetch-fails, generic 500, UUID-guard. The route's saveBlock is now a thin shell over the tested helper.7fc56022addsmockFetchRejects()and asserts the popup degrades to the empty state.d9c7abf2holds the secondsaveFnpromise so the test observeserror → saving → saved. The intermediate saving state is now pinned.Nits I asked for
escapeHtml('&')idempotence test — added in362a84dd.'@Aug\nfoo'cursor-at-newline boundary — added inb4b46a0a.fd3a44d1.What I checked
setTimeout/setIntervalreal-timer waits remain.Approved. The 3 pre-existing flaky tests in
TranscriptionBlock.svelte.spec.tswere correctly excluded with the verified-against-main note in the PR body.Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
🎨 Leonie Werner — Cycle 2 Re-Review
Verdict: ⚠️ Approved with one remaining concern
Six of my seven cycle-1 concerns are addressed cleanly:
f0bb1c31— 150ms-deferred close on blur (deliberate delay so option onmousedown still wins).@trigger780c6821—bg-brand-mint/20 ring-2 ring-brand-mint ring-insetonaria-selected. Distinct from hover. WCAG 1.4.11 satisfied.a8a3b7f5—focus-visible:border-brand-mint focus-visible:ring-2 focus-visible:ring-brand-mint/40.09f71a2d— "Neue Person anlegen →" link to/persons/new?name={query}, opens in new tab so the transcriber doesn't lose their in-progress block. Three Paraglide keys added.a8a3b7f5—min-h-[44px]+py-2.5on the textarea.86ad5ca9—loadingflag flips on at scheduleSearch, off in the fetch's finally branch. Reusescomp_typeahead_loading("Suche…") so no new i18n keys.Remaining concern
Discoverability. The textarea looks identical to before for a first-time transcriber. They have no way to know
@opens a person typeahead unless they're told or stumble on it. My cycle-1 suggestion was a subtle hint near the bottom of the textarea (mirroringtranscription_block_quote_hint). It's a 4-line addition and the i18n key already exists in concept ("Tipp: @ tippen um eine Person zu verlinken").I'm not blocking on this — discoverability problems usually surface in user testing rather than crashing CI, and PR-B1 can ship with this gap if you want to validate the workflow with the existing 2 transcribers first. But I'd like a tracked follow-up issue, not a silent acceptance.
Nits I'm not re-raising
text-base+text-xsramp inside the row — cosmetic, leave.w-72fixed-width popup on mobile — out of scope for B1 (transcribers on tablets, the fixed width is fine; readers on phones don't see this editor).placeholder:text-ink-3contrast — out of scope; this is a project-wide token concern.What I checked in cycle 2
person_mention_create_newreads naturally — "Neue Person anlegen" matches the project's existing register.bg-brand-mint/20 + ring-2 ring-brand-mint) tested visually in my head againstbg-canvas(hover) — the contrast difference is unambiguous.target="_blank"+rel="noopener"on the create-new link — correct security hygiene.mousedownto land, short enough that the user perceives it as immediate.Approved with concern about discoverability — please open a tracked follow-up.
Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
⚙️ DevOps — Cycle 2 Re-Review
Verdict: ✅ LGTM
Cycle 2's diff is still entirely frontend code — 11 files touched (8 svelte/ts, 3 i18n JSONs). Zero changes to:
Dockerfile/docker-compose.yml.gitea/workflows/or.github/workflows/db/migration/package.json(no new deps)What I checked
saveBlockWithConflictRetry.tsships unchanged at runtime — no new top-level imports, no new bundle chunks (it's already a top-level module that the route file dynamic-imports on first save).person_mention_create_new) compiles into the existing per-message file layout. No build pipeline change needed./api/personsremains a backend follow-up, not a B1 blocker.One follow-up worth tracking
BlockConflictResolvedErrorpath is now correctly typed and observable from the catch site. If you want to know how often rename-mid-edit conflicts actually fire in production, consider hooking the catch in+page.svelteto aposthog.capture('block_conflict_resolved')(or the equivalent) in a follow-up. The data would tell you whether the merge UX needs more investment or whether 409s are basically theoretical.Approved.
Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
📋 Requirements Engineer — Cycle 2 Re-Review
Verdict: ✅ Approved
Walking through my cycle-1 asks against the updated PR description:
mergeBlockOnConflict+saveBlockWithConflictRetry. The full Playwright round-trip is correctly deferred./persons/new?name={query}. This is now defined behaviour, not under-specified.What I cross-checked in the cycle 2 diff
One observation for the issue body (not a B1 blocker)
The issue body still has OQ-1 marked as "open" with the "ambiguous label" wording. Once PR-B2 implements the first-sidecar-wins rule, the issue body should be amended to either remove OQ-1 or reword it as "closed: first-sidecar-wins". Track it with B2.
Approved — no remaining B1 spec gaps.
Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com
🏁 Cycle 2 — Final Status
All cycle-1 concerns triaged and addressed. Cycle-2 review verdicts (above):
Net: 6 ✅ + 1 ⚠️ (explicitly non-blocking).
Leonie's remaining concern (discoverability hint for the
@trigger) is tracked as #370 per her request — she explicitly said "I'm not blocking on this; ship and validate with the existing 2 transcribers first."Cycle 2 commits (pushed as
e3175f49..b4b46a0a)79349644+bbde9e84— rename shadowedm(Felix #1)cb51e8e4— drop deadhandleMentionsChange/getPendingMentions(Felix #2)fd3a44d1— typedBlockConflictResolvedError(Felix #3)49db82e1— autoresize moves into editor (Felix #5)362a84dd— apostrophe inescapeHtml(Sina action item)43aacd9f+ extracted helper — UUID guard on saveBlock (Sina #1)cacbd577— SECURITY comment on typeahead fetch (Sina #2)e8ba8405— fake timers in editor spec, 13× faster (Tester #1)7fc56022— fetch-rejects regression test (Tester #4)d9c7abf2— observe saving→saved transition (Tester #5)ba73387d— extractsaveBlockWithConflictRetry+ 6 unit tests (Tester #2 + Markus #2)b4b46a0a— boundary cases for@Aug @Bertand newline cursor (Tester nits)f0bb1c31— popup closes on blur (Leonie #1)780c6821— keyboard-highlight contrast (Leonie #3)a8a3b7f5— focus ring + 44px tap target on textarea (Leonie #4 + #6)86ad5ca9— loading indicator (Leonie #7)09f71a2d— empty-state "Neue Person anlegen →" link (Leonie #5 + ReqEng #3)Final test totals
TranscriptionBlock.svelte.spec.ts, 2 inTranscriptionEditView, 1 in hilfe page — all verified againstmainto be pre-existing.PR is mergeable. Recommend merge.
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com