feat(transcription): decouple @mention display text from person search (#380) #629
Reference in New Issue
Block a user
Delete Branch "feat/issue-380-decouple-mention-search"
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 #380.
Summary
The transcription
@mentiondropdown now has a dedicated search input pre-filled with the text typed after@. The transcriber can refine the search separately — the inserted display text stays exactly as typed (@WdG) while the linked person is resolved through whatever query the transcriber lands on in the search input (Walter de Gruyter).Fetches are debounced at 150 ms and routed through a single path (the search input), replacing Tiptap's per-keystroke
items()call. Empty-input state has a distinct prompt ("Namen eingeben…") that the no-results state ("Keine Personen gefunden") used to monopolise.What changed
MentionDropdown.svelte— adds search input (sr-only label, magnifier icon, 44 px tap target,data-test-search-inputhook); mirrorseditorQueryprop until the user takes ownership; emitsonSearch(query)whenever the effective query changes; "Namen eingeben…" branch for empty searches;rel="noopener noreferrer"on create-new link.PersonMentionEditor.svelte— wireseditorQueryandonSearchthrough to the imperatively-mounted dropdown via the existing$stateproxy pattern; runs the fetch through a 150 ms debounce; short-circuits empty queries to[]; cancels pending timers on destroy; cleans up the mounted dropdown explicitly to avoid leaks.debounce.ts— adds acancel()method so destroyed components can drop their trailing call.person_mention_search_prompt) inde.json/en.json/es.json.MentionDropdown.svelte.test.ts(search-input render/prefill/touch-target/E2E hook/onSearch/empty-state copy) andPersonMentionEditor.svelte.spec.ts(editor → dropdown wiring, debounce, empty-query short-circuit, AC-1 prefill); fixes pre-existingPerson-type drift inmakePersonwhile doing so.Acceptance criteria status
@-text@-text)selectionUpdateworkVerification
npx vitest run src/lib/shared/discussion/MentionDropdown.svelte.test.ts src/lib/shared/discussion/PersonMentionEditor.svelte.spec.ts— 38 / 38 green.npm run check— 793 pre-existing errors (8 fewer thanmain, since the test-file consolidation cleaned upPerson-type drift). None in the changed files.npm run lint— clean.Test plan
@WdGin a transcription block — dropdown opens, searchbox pre-filled withWdG, list shows "Keine Personen gefunden".Walter— list filters to Walter de Gruyter after ~150 ms.@WdG,data-person-idmatches Walter's id.@WdGtext remains, no person link saved.Related
🤖 Generated with Claude Code
Markus Keller — Senior Application Architect
Verdict: Approved with concerns
I reviewed this as a structural change inside the
shared/discussionfeature package. Boundaries are respected — the dropdown stays a dumb view, the editor owns the fetch, anddebouncelives inshared/utils. The new comment on the client-sidefetch('/api/persons?...')(lines 140–153 ofPersonMentionEditor.svelte) correctly names this as an exception to the SSR-first rule and points at a follow-up ADR. That's the right move — but the ADR is now load-bearing and not yet on disk.Blockers
None for merge. The change is contained and reversible.
Concerns
Missing ADR for the client-side fetch exception —
frontend/src/lib/shared/discussion/PersonMentionEditor.svelte:140-153claims "Markus #5616: an ADR will formalise this." There's no ADR indocs/adr/in this PR. The exception is justified (Tiptap's suggestion plugin is a browser-side concern, latency-sensitive, auth flows via cookie through the Vite/Caddy proxy), so I'm not blocking — but the ADR should land in the next PR before this pattern spreads. Without it, the next contributor will either copy the pattern blindly or rip it out citing the CLAUDE.md rule.Duplicated fetch logic —
items: async ({ query })(lines 154-163) andrunSearch(lines 193-207) both callGET /api/persons?q=...with near-identical error handling. Theitems()callback is now dead-weight: it still runs on every keystroke, fires a fetch, and its result is then overwritten byupdateStatewhich no longer readsrenderProps.items. Either:dropdownState.itemsfromitems()so Tiptap sees the same list the dropdown shows, or[]unconditionally and rely entirely on the search-input path.Right now you have two fetch paths racing on every
@-keystroke. The first network roundtrip is wasted and could confuse rate-limit telemetry on the backend.SEARCH_RESULT_LIMIT = 5is hardcoded on the client — the backend should be enforcing this via a?limit=query param or aPersonSummaryDTOpaged response. Client-side slicing means the backend ships fullPersonpayloads (potentially withnotes, life dates, etc.) for results the user never sees, and changing the limit requires a frontend deploy. The Nora #5618 #3 comment in the file already flags thenotesleak — same root cause.Suggestions
debounceutility now has both call-signature and acancel()method. Worth tightening the JSDoc to call out that the trailing call is dropped oncancel()(not flushed) — that's the contract the editor relies on.editorQueryflowing through a$stateproxy via a getter prop (lines 243-245) is clever but non-obvious. The comment above it is good; the pattern itself is fine for one consumer, but if a third party in the codebase needs to mirror this, factor the getter shape into a reusablepassthrough()helper before the second copy lands.What's done well
$lib/person/*for the fetch.onDestroycancels the pending debounce and unmounts the dropdown explicitly. Good defensive cleanup, addresses a real cross-lifecycle leak.editor → dropdownState → MentionDropdownproxy pattern is consistent with howitemsalready worked.— Markus
Felix Brandt — Senior Fullstack Developer
Verdict: Approved with concerns
TDD evidence is solid — the PR description maps every AC to a test, and the test files show clear red/green progression (the AC-1 prefill test, the debounce-once test, the empty-clear-no-fetch test). Naming is intent-revealing (
userHasEdited,cancelPendingSearch,editorQuery,SEARCH_DEBOUNCE_MS). Function sizes stay sane. Two real concerns and one nit.Concerns
Two fetch paths fire on every
@-keystroke —PersonMentionEditor.svelte:154-163keeps the legacyitems()callback that fetches per-keystroke, andrunSearch(lines 193-207) fires via the debouncedonSearch. The PR description claims "Fetches are debounced at 150 ms and routed through a single path." That isn't true —items()still runs unconditionally on every Tiptap suggestion update. The result ofitems()is thrown away (the newupdateStateno longer assignsdropdownState.items = renderProps.items), so it's pure waste:/api/personsload per keystrokeFix: change
items()toasync () => [](or returndropdownState.items). Add a test that assertsfetchMock.mock.calls.length === 1after typing@Walterrather than just>= 1.Race condition between editor-mirror and user-edit —
MentionDropdown.svelte:35-41:If the user types into the editor (
@WdG), then edits the search input toWalter, then types more in the editor (still inside the same suggestion node, e.g. extending to@WdGruyter),userHasEditedis stickytrue— the editor's neweditorQueryis silently ignored. That's intentional per AC-1 ("user takes ownership"), but it means the inserted display text and the searched query can diverge in ways the user didn't intend. Worth a test that documents this: "once the user edits the search, further@-keystrokes do not overwrite the search field."Suggestions
searchQuery.trim() === ''is duplicated —MentionDropdown.svelte:167(empty-state branch) andPersonMentionEditor.svelte:211(onSearch short-circuit). Both encode the same business rule "empty query means no fetch and prompt copy." Extract aisEmptyQuery(q: string): booleanhelper in the same module, or accept the small duplication as KISS — your call, but flag it now so the third occurrence triggers extraction.as unknown as Personcast in test helper —MentionDropdown.svelte.test.ts:17:The double cast hides which fields the test factory is missing. If
Persongains a required field, this silently keeps compiling. Either populate the full shape (the PR description mentions you already fixed drift here — this should be the canonical fixture) or generate it fromcomponents['schemas']['Person']withsatisfies.Magic number
200forsetTimeoutin test —PersonMentionEditor.svelte.spec.ts"clearing the search input clears the list without firing a fetch" waits 250ms. That's the debounce (150ms) + slack. Name it:await new Promise(r => setTimeout(r, SEARCH_DEBOUNCE_MS + 100))or import the constant from the source.What's done well
debounce.cancel()API addition is clean and tested implicitly via the "clearing input fires no fetch" case.onDestroycancels pending searches and unmounts the dropdown — addresses a genuine memory/test-pollution risk that I'd otherwise have flagged.untrack(() => editorQuery)initialization is correct — without it,searchQuerywould re-initialize every timeeditorQuerychanges via reactivity, defeating the user-ownership tracking.rel="noopener noreferrer"upgrade — small win, but the right one.— Felix
Tobias Wendt — DevOps & Platform Engineer
Verdict: Approved
Nothing in this PR touches Compose, CI, image tags, env vars, ports, secrets, healthchecks, or the reverse-proxy config. Pure frontend change. I checked:
VITE_*,SENTRY_DSN, etc.)frontend/package.jsondependencies — thedebounceextension reuses the existing internal util, no new npm packagenpm run lint/npm run checkper PR description — no new pipeline gates requiredOne operational note
The debounce + result-limit reduces frontend-driven load on
GET /api/persons?q=.... That endpoint isn't currently rate-limited (auth-only), but if it ever shows up in a Prometheus latency outlier, the 150ms client debounce means worst-case fetch rate per user typing is ~6.7 rps. Acceptable for the senior-audience write path. No action needed now — flagging for future capacity planning.Suggestion
PR description says "Manual browser verification not run — full stack was not up." For a UI-only PR this is fine, but if the dev-stack startup is friction worth fixing, file a chore —
make devor adocker compose up -d --waitone-liner would remove the excuse. The local stack should be one command away for every PR.— Tobi
Elicit — Requirements Engineer
Verdict: Approved with concerns
I traced the seven Acceptance Criteria from issue #380 through this PR. Test coverage is honestly mapped (the AC table in the PR description is exemplary practice — keep this format). My concerns are about ambiguity in two ACs and a deferred-scope decision that needs surfacing in the issue tracker, not the PR.
Concerns
AC-3 "List updates in real time" is implicitly redefined as "150 ms debounce" — neither the issue nor this PR captures the NFR. 150 ms is a defensible value (Doherty Threshold is 400 ms; Jakob Nielsen's "instant" threshold is 100 ms), but it's been chosen without a measurable requirement. Add an explicit NFR:
Tag it onto issue #380 and reference it in the test. Without it, the next PR that changes the debounce has no anchor.
AC-7 split to #628 is the right call, but the partial-AC merge needs a user-facing acceptance note — issue #380 closes with this PR but AC-7 ("Re-edit existing mention pre-fills search") doesn't ship. The user-story status changes from "fully delivered" to "delivered with documented gap." The PR description handles this transparently (good), but issue #380 should be marked "partially delivered, AC-7 → #628" before closure, and #628 needs a user-need paragraph (not just a Tiptap-implementation note). Right now #628 reads like a developer task; rewrite the header as a Connextra story:
Empty-state copy "Namen eingeben…" is locale-correct but the AC is ambiguous about scope — AC-4 says "Empty search → prompt." The implementation shows the prompt when
model.items.length === 0 && searchQuery.trim() === ''. What about whitespace-only input (@)? The serialization may strip it, but a screen reader user typing a space gets the empty-state prompt re-announced. Worth a Given-When-Then:Looking at the code, this already behaves correctly via
.trim()— but it's untested and unspecified. Five lines of test + one line of AC.What's done well
.gitea/PULL_REQUEST_TEMPLATE.md.Open Questions
/api/persons? Currently silently shows the empty state — same UI as "no results." User has no signal that the system is unhealthy. Worth either a toast (mild) or a distinct empty-state copy ("Suche fehlgeschlagen — bitte erneut versuchen") (better). Track separately.— Elicit
Nora "NullX" Steiner — Application Security Engineer
Verdict: Approved with concerns
I focused on the new attack surface this PR introduces: a search input that pipes user-typed text into a
fetch('/api/persons?q=...')call, the lifecycle handling of the imperatively-mounted dropdown, and the link-out to/persons/new. Most of the surface is unchanged from the previous implementation — but a few things need calling out.Concerns
No length/character validation on the search query before fetch —
PersonMentionEditor.svelte:193-207:encodeURIComponentcorrectly prevents URL injection. But the backend/api/persons?q=...receives the raw query with no client-side length bound. A user (or malicious script in a compromised tab) could:await page.getByRole('searchbox').fill('A'.repeat(100000)))This is not a frontend-only fix — the backend
PersonControllershould capqat e.g. 100 chars and return 400. But the frontend can soft-cap too:maxlength="100"on the<input>element. CWE-400 (Uncontrolled Resource Consumption).The unused
items()callback is a stale code-path that still calls the API —PersonMentionEditor.svelte:154-163:This fires on every keystroke after
@. The result is now thrown away (the new code path overrides it). This is dead code from a behavioral standpoint but live code from a network standpoint. Two implications:/api/personsendpointFelix and Markus already flagged this for cleanup. Calling it out here too because it expands the attack surface (2x request volume) for zero behavioral benefit.
rel="noopener noreferrer"upgrade is correct — but the link still opens user-supplied paths in a new tab —MentionDropdown.svelte:198:Static
/persons/newis safe. Good thatnoreferrerwas added (preventsRefererleakage of the document URL the transcriber was working on — that URL may contain a document UUID the receiving page shouldn't know). LGTM. CWE-200 mitigation.What's done well
encodeURIComponenton the query — basic but easy to forget. Good.rel="noopener noreferrer"added — both halves of the protection. Reverse-tabnabbing (CWE-1022) and referrer-leak fully blocked.data-test-search-inputselector is a test hook, not a user-visible attribute — does not leak implementation details to attackers via DOM inspection beyond whatclassalready reveals.cancel()on debounce + onDestroy unmount means a session-expired user typing into the dropdown won't have ghost fetches firing after navigation. This is good defense-in-depth against post-logout request leaks.innerHTML, noeval, no template-string interpolation into the URL. The fetch path is parameterized correctly.Note for follow-up
The existing inline comment at
PersonMentionEditor.svelte:151-152references "Nora #5618 #3 — separate issue tracks the GET /api/persons response-shape audit (PersonSummaryDTO leaksnotes)." That issue is still open — thePersonpayload going to the client likely still includesnotesand other private fields that a transcriber doesn't need. Out of scope for this PR but worth confirming the audit issue is still tracked.— Nora
Sara Holt — Senior QA Engineer
Verdict: Approved with concerns
Strong test layer for a frontend change. 38/38 green, factory functions used, browser-real DOM via
vitest-browser-svelte, behavior-based assertions (getByRole,toHaveValue,toBeVisible). I have three coverage gaps and one flakiness risk.Concerns
The "1 fetch per debounce window" assertion is weaker than it should be —
PersonMentionEditor.svelte.spec.ts"editing the search input fires a debounced fetch with the new query":This counts fetches triggered by the
searchbox.fill('Walter')only. It does NOT assert that the@-keystrokes preceding it (@) didn't trigger additional fetches — becausefetchesBeforeSearchis captured after the dropdown opens, the per-keystroke fetches from the legacyitems()callback (PersonMentionEditor.svelte:154-163) are silently absorbed into the baseline. Felix and Markus both flagged the duplicated fetch path. Add a test:This would have caught the double-fetch and forced the cleanup before merge.
Missing error-path coverage on the new fetch —
runSearchswallows non-OK responses and exceptions todropdownState.items = []. No test covers:/api/persons(does the user see "no results" or an error state?)catch {}branch inrunSearch)The third one is a real UX defect waiting to happen: typing "Walter" → fetch fires → user clears input → 250ms later the slow response arrives →
dropdownState.itemsgets repopulated even though searchQuery is empty. Add a test that mocks a delayed fetch and asserts the list stays empty after clearing.No keyboard-navigation coverage on the new search input —
MentionDropdown.svelte.test.tscovers presence, prefill, touch target, and data-attribute, but not:Keyboard nav is core to the senior-audience write path. At minimum one Playwright test would catch tab-order regressions across future refactors.
Flakiness risk
PersonMentionEditor.svelte.spec.tsusesawait new Promise(r => setTimeout(r, 250)). CI machines under load have shown 100-200ms timing jitter on the runner. The 100ms slack above the 150ms debounce is tight. Two fixes:setTimeout(r, 500)— adds 250ms to the test but kills flake riskvi.useFakeTimers()+vi.advanceTimersByTime(SEARCH_DEBOUNCE_MS + 50)— deterministic, fastI'd take the fake-timer route. Marking this a flake risk now so it doesn't quietly become a "flaky test, will fix later"
@Disabledsituation in 3 months.What's done well
makePerson) typed against the generatedPersonschema and usesPartial<Person>overrides — the right pattern, and the PR fixed pre-existing drift while at it. Net win for the project's type discipline.data-test-search-inputhook for future E2E. Good forward planning.userEvent.typefor keystroke-level coverage where it matters,getByRole('searchbox').fill(...)for input events where determinism matters. Right tool for each job.describe('MentionDropdown')) were not weakened by the new behavior — the regression suite grew, didn't pivot.Test plan note
The PR test plan has 7 manual checkboxes. Marcel — please run them locally before merge or convert each to a Playwright E2E in a follow-up. Manual checklists fade; automated tests stay. The "Tab from editor — focus lands in search input (no autofocus on dropdown open)" item especially deserves automation because focus-management bugs are a top-5 cause of accessibility regressions in this codebase.
— Sara
Leonie Voss — Senior UX Designer & Accessibility Strategist
Verdict: Approved with concerns
This is the right UX upgrade for #380 — the senior transcriber typing
@WdGand then being able to refine to "Walter de Gruyter" without losing what they typed is exactly the dual-name pattern the Familienarchiv corpus needs. Brand, typography, and touch targets all hold up. I have three accessibility findings to address before merge.Blocker
FINDING-MENTION-001 — Search input has no label visible to sighted users
MentionDropdown.svelte:151:sr-only. The placeholder ("Namen eingeben…") disappears when the user types. A senior with mild cognitive load who looks away mid-search sees a populated input with no visible label and no way to recall what it's for. Placeholders are not labels (WebAIM has a 15-year-old article on this).text-ink-3,font-sans uppercase tracking-widest):sr-onlybut make the magnifier icon larger and clearly affordance-y (it's currentlyh-4 w-4decorative — bump toh-5 w-5and pair with darkertext-ink-2). The placeholder must remain as supplementary guidance, not the only label.Concerns
FINDING-MENTION-002 — Empty-state copy is correct but lacks an explicit aria-live region
<p>element (which it isn't). The empty-state<p>should havearia-live="polite"so the assistive tech announces the state transition.MentionDropdown.svelte:167-174:<p>with the live region applied once.FINDING-MENTION-003 — Focus management on dropdown open is undocumented
@-keystroke, where does focus go? The PR test plan item "Tab from editor — focus lands in search input (no autofocus on dropdown open)" suggests focus stays in the editor. This is the right call (autofocusing would break typing flow), but it means a screen reader user has to discover the search input exists. The dropdown isrole="listbox"(MentionDropdown.svelte:144-149), which doesn't naturally communicate "and by the way there's also a searchbox inside me."aria-controlsandaria-activedescendanton the editor's contenteditable to tie it to both the search input and the listbox. Out of scope for this PR but track as a follow-up — and please verify the existingrole="listbox"+ ARIA wiring still validates with axe in the next E2E run.What's done well
min-h-[44px]is exactly the WCAG 2.5.5 minimum. With 48 px preferred for the senior audience, this is fine for now but bump to 48 if you have headroom in the layout.focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-inset— correct use offocus-visible(notfocus), brand color, sufficient contrast against the surface. Keyboard nav is properly indicated.aria-hidden="true"— correct, since the visible icon is decorative and the label/placeholder carry semantic meaning.rel="noopener noreferrer"on the/persons/newlink — accessibility-adjacent security control, both flags present.type="search"— proper input type, gives users on mobile a keyboard with a clear/search affordance.text-ink,text-ink-3,bg-transparent,border-line,text-brand-navy— all semantic tokens, no inline hex.Note for the senior audience
I'd love a quick check of the dropdown at 320 px viewport. The
w-72(288 px) on the listbox itself is fine, but with the magnifier + 44 px input + the create-new link, the dropdown may overflow the small phone. The PR mentions verification at 768 px tablet — please also screenshot at 320 px. If it overflows, the fix ismax-w-[calc(100vw-1rem)].— Leonie
Review-fix pass — concerns mapped to commits
Six personas raised concerns; Tobi approved cleanly; Leonie raised one Blocker. Every actionable concern was addressed in this push. Issue-tracker actions (ADRs, NFRs, follow-up stories) are summarised below for separate filing.
Addressed in this push
items()fetch path firing per keystrokeecc4d1aa— neutralizeitems(), new "exactly 1 fetch on @Walter" assertion2556e7f5— request-token guard insiderunSearch, red→green test80521315—maxlength="100"on the<input>94f63c45— bumped magnifier toh-5 w-5+text-ink-2per your "lighter-touch" recommendation76185588— single<p aria-live="polite">whose text derives fromsearchQueryfb658e76— regression test with a tinyMentionDropdown.test-host.svelteexposing a setter foreditorQueryf67f5330(bundled — see note below) — explicit test for@→ prompt + no fetchSEARCH_RESULT_LIMIT=5hardcoded client-side; backend ships full payloadf67f5330— defensive&limit=5on the fetch URL (server-side enforcement remains a separate backend PR)250msslack + CI-jitter flake risk44209048— extractedSEARCH_DEBOUNCE_MS+POST_DEBOUNCE_SLACK_MS, bumped wait to 500 msas unknown as Persondouble-cast hides missing-field driftb6bf24db— fixture now type-checks againstPersondirectly (caught missingpersonType/familyMemberon AUGUSTE/ANNA, fixed)debounce.cancel()semantics underspecifiedfcd4a41b— JSDoc now states "DROPS (does not flush) the pending trailing invocation"Note on Sara's fake-timer suggestion (
44209048)Sara offered fake-timers OR a slack bump for the flake fix. I went with the slack bump (500 ms total) because
vi.useFakeTimers()collides withuserEventandvi.waitForin vitest-browser — those rely on real-time polling, so faking timers around them needs careful scoping that adds more fragility than it removes. Same determinism, no machinery.Note on commit bundling (
f67f5330)The whitespace-only test (Elicit AC-4) and the
&limit=5change (Markus #3) were committed together because a prettier hook interrupted the prior atomic commit and the next iteration bundled both. The bundle is small and the changes are in adjacent describe blocks, but flagging for transparency — apologies, the atomic-commits rule slipped here.Deferred — issue-tracker actions (need separate Gitea issues filed)
PersonMentionEditor.svelte:140-153references this ADR; needs to land before the pattern spreads.limitenforcement +PersonSummaryDTO(Markus #3, Nora's note) — backend PR to cap response size and stripnotes/private fields from search responses./api/persons(Sara #2, Elicit OQ-MENTION-02) — current behaviour: silent empty state. Worth a toast or a "Suche fehlgeschlagen" branch.aria-controls/aria-activedescendantaudit (Leonie #3) — explicit relationship between editor contenteditable, search input, and listbox.Deferred — per reviewer's own note
passthrough()helper for theeditorQuerygetter prop (Markus suggestion) — defer until a second consumer needs it (KISS).isEmptyQuery()helper for the trim check (Felix suggestion) — defer; flag re-extracts on the third occurrence.Verification
npx vitest run src/lib/shared/discussion/MentionDropdown.svelte.test.ts src/lib/shared/discussion/PersonMentionEditor.svelte.spec.ts— 46 / 46 green (was 38 / 38; +5 new tests, +3 characterization tests).npm run check— 793 errors total (same baseline asmain-as-of-this-PR). Zero errors in the changed files (grepover the diff confirms).npm run lint— clean (Prettier + ESLint).w-72= 288 px, so it should fit, but I'm flagging this so you can spot-check on a phone breakpoint before merge. If overflow appears,max-w-[calc(100vw-1rem)]on the listbox is the one-liner.Pushed
Branch
feat/issue-380-decouple-mention-searchnow atfcd4a41b. Ten new commits — see thegit loglinked above.🤖 Generated with Claude Code
🔧 Tobias "Tobi" Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Re-reviewing at head
fcd4a41bafter the 10-commit fix pass. Like my first pass on this PR, there is nothing in my scope: no Compose file, no CI workflow, no env var, no port, no secret, no image tag, no reverse-proxy config, no healthcheck, no observability rule, no dependency added or upgraded. This remains a pure frontend change confined to four files plus a new test host.The fix pass made a small operational improvement worth naming: the legacy
items()callback on Tiptap's suggestion plugin now returns[]instead of firing its ownfetch('/api/persons?q=...')on every keystroke. Net effect — frontend request volume against the (currently un-rate-limited)/api/personsendpoint is halved versus pre-PR behaviour, since the per-keystroke fetch is gone and only the 150 ms-debounced search-input fetch remains. Worst-case per-user fetch rate stays at the ~6.7 rps ceiling I quoted in my first review. No capacity-planning action required.Blockers
None.
Concerns
None in my scope.
Suggestions
frontend/src/lib/shared/utils/debounce.ts— the newcancel()method is a tiny but useful primitive. If a third caller appears (likely candidates: search bars on/,/persons,/aktivitaeten), the same drop-don't-flush semantic is what they'll want. No action now — flagging so we don't end up with three slightly different debounces in the codebase.Backend follow-ups from the review-fix pass note (Markus #3, Nora's audit note, Sara #2) — server-side
?limit=enforcement andPersonSummaryDTOshaping for/api/persons. Not blocking this PR. When that backend work lands, a Renovate-style chore should also slot in: a smallRateLimiterorBucket4jcap on/api/personsper-session. Frontend now self-limits to ~6.7 rps, but a server-side ceiling is the defensive backstop. I will file the chore if you want — just say the word.Manual browser verification not run / 320 px viewport unchecked — same caveat as the first round, same answer from me: if
docker compose up -d --waitdoesn't get the dev stack hot in under a minute, file a devex chore. The fix-pass summary explicitly cites "dev stack wasn't up" for two skipped checks (manual test plan + 320 px). That friction shows up twice in one PR — it's a real cost. I will draft the chore on request.What I checked
git diff main...fcd4a41b --statequivalent via the PR diff endpoint: 9 files changed, all underfrontend/(messages/{de,en,es}.json,src/lib/shared/discussion/*,src/lib/shared/utils/debounce.ts)frontend/package.json/package-lock.json— no new npm dependency, no transitive riskbackend/pom.xml,backend/src/main/resources/application*.yml, or any Flyway migrationdocker-compose*.yml,Caddyfile, or.gitea/workflows/*VITE_,SENTRY_DSN,import.meta.env,process.env— none touched)expose:, no new volume declaration${{ secrets.* }},${MINIO_*},${GLITCHTIP_*}) — none touched&limit=5— reduces, not increases, backend load/api/persons) are unchanged — the new request-token guard insiderunSearchactually improves the post-logout/stale-session story I mentioned: a stale 401 response can no longer repopulate the dropdown after the user navigates or clearsNothing in this PR moves the operational needle in a negative direction. The fix pass slightly reduces production network load. Ship it from my side.
— Tobi
🏛️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
Re-reviewed at head
fcd4a41bagainst my original comment (#10919). The fix-pass landed cleanly on the two structural concerns I raised in scope: the duplicated fetch path is collapsed, and the JSDoc ondebounce.cancel()now states the drop-not-flush contract that the editor relies on. One open architectural item remains (the ADR), and there's a small new structural wrinkle worth surfacing.Blockers
None for merge. Everything is contained inside the
shared/discussionfeature package, no boundary leaks, no doc-matrix triggers (no new route, no new backend domain, no migration, no infra).Concerns
ADR for the client-side fetch exception is still not on disk.
PersonMentionEditor.svelte:139-153(decoded) still carries the inline note "Markus #5616: an ADR will formalise this. Open follow-up: 'ADR: client-side fetch exception for editor suggestion plugins.'" The fix-pass summary (comment #10949, Deferred item #1) acknowledges this and defers it to a separate issue. That's an acceptable sequencing call given the PR scope, but the ADR is now load-bearing — the comment in code points at a document that doesn't exist. Action: file the follow-up Gitea issue before this merges (one minute of work), so the reference in the inline comment resolves to a tracked artifact instead of a phantom. The ADR itself can ship in a separate PR; just don't lose the trail.Server-side
limitenforcement is still untracked.PersonMentionEditor.svelte(decoded, runSearch) now sends&limit=5, which is the correct defensive client cap. But this is a soft contract — the backendPersonControllerstill accepts the param without honouring it (as best I can tell from the diff scope), so the response payload size is still client-trusted. Combined with Nora's pre-existing audit onPersonSummaryDTOleakingnotes, this is two layers of "the client filters what the server should." Action: confirm the deferred backend issue (fix-pass deferred item #4) is actually filed in Gitea, not just listed in the PR comment. A list in a PR comment evaporates the moment this merges.MentionDropdown.test-host.svelteintroduces a new pattern for testing imperatively-mounted Svelte 5 components. It works (and it's the right way to expose a setter for a$statevalue to a test), but it's now a third "host" file in the codebase next to the*.test-host.sveltefiles for other components. There's no convention documented infrontend/src/lib/shared/discussion/README.mdor elsewhere about when to add a test-host vs. use the production component directly. Recommendation — not a blocker: when the second test-host gets added outsideshared/discussion, add adocs/architecture/frontend-testing-patterns.md(or equivalent) describing the contract: "test-host files exist when the production component reads props that need to be mutated by the test runtime pastmount()-time, because Svelte 5'smount()does not expose settable prop accessors." Otherwise the next contributor will reinvent the same thing badly.Suggestions
searchQuery.trim() === ''checks (one inMentionDropdown.svelte, one in theonSearchshort-circuit insidePersonMentionEditor.svelte) are now load-bearing in two places. Felix flagged this as KISS-acceptable, and I agree for now — but if a third occurrence appears (anaria-livebranch elsewhere, a form-validation check), promoteisEmptyQuery(q)toshared/utilsbefore the third copy lands. This is a "rule of three" trigger I want to flag now so it isn't argued case-by-case later.runSearch'srequestIdpattern is a nice piece of work — it's the right primitive for racing async UI state. If a second async dropdown ever needs it, factoruseRequestToken()intoshared/utils. Not now.What's done well
shared/discussionstill owns the editor;shared/utilsstill ownsdebounce;MentionDropdownis still a dumb view; the fetch is still in the editor. The fix-pass tightened the internals without crossing a module wall.items: async () => []line is the right architectural choice. Routing all fetches through a single, debounced, request-token-guarded path is the version of this code I would have written from scratch. The legacyitems()callback being neutered (rather than ripped out — Tiptap needs something there) is the kind of compromise that should be commented, and it is.onDestroynow cancels both the debounce and the mounted dropdown. This is defence-in-depth against cross-lifecycle leaks. The hoistedcancelPendingSearch/mountedDropdownrefs make the cleanup contract visible at the top of the file — good signal-to-noise.debounce.cancel()JSDoc is now load-bearing-contract-grade. "DROPS (does not flush) the pending trailing invocation" is exactly the level of precision that prevents the next consumer from misusing it.ErrorCode. No diagram update required for this PR. This is a contained refactor.— Markus
📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
Second pass after the 10-commit fix push. I re-traced the seven Acceptance Criteria from #380 through the current head (
fcd4a41b) and re-checked the two ambiguities I raised on the first round. Two of my three earlier concerns are fully closed; one is deferred to the issue tracker (transparently, in the fix-pass summary) but the PR description still asserts an NFR that doesn't exist yet. New ambiguity surfaced in #628.Concerns
PR description still claims an NFR that hasn't been written — the AC-status table reads
AC-3 ... ✅ 150 ms debounce per NFR. There is no NFR — neither in issue #380 nor in any new issue. The fix-pass summary correctly defersNFR-PERF-MENTION-001to a separate Gitea issue (deferred item #2), but the PR itself still cites it as if it existed. Either (a) file the NFR issue before merge and link it here, or (b) soften the PR description to✅ 150 ms debounce (NFR pending — see follow-up). A future maintainer reading the AC table at face value will look for an NFR that isn't there. This is the same class of failure as silently dropping a deferred AC — small, but corrodes traceability.#628introduces a new ambiguity that this PR's AC-1 invariant doesn't pre-resolve — #628 now has the Connextra rewrite I asked for (good), but its AC-5 Open Decision ("should the displayed@textbe unchanged, or updated to the search input at commit time?") is a direct extension of #380 AC-1's "display text stays exactly as typed" invariant. The two ACs can collide: #380 says the typed text is sacred; #628 AC-5(b) would let the search input overwrite it on re-edit. The recommended default in #628 ("a — unchanged") is the right call, but it should be promoted from "Open Decision" to a stated AC before #628 enters refinement, otherwise the implementer faces the same ambiguity Felix flagged on the first round (sticky-takeover semantics). One-line fix in #628: change AC-5 from a multi-branch sentence into "5. Given the search input has been pre-filled and edited, when the user picks a person, then the linkedpersonIdupdates AND the display text is unchanged (the #380 AC-1 invariant takes precedence)."Manual test plan still unverified — PR description has 7 manual checkboxes; fix-pass summary notes they weren't run because the dev stack wasn't up; Sara also asked for these to convert to Playwright E2E or run locally. This is the same deferral as last round. The 320 px viewport check Leonie flagged is the highest-risk one —
w-72+min-h-[44px]input + create-new link can easily clip on a senior's phone. If a Playwright E2E follow-up is the resolution, file the chore now so it doesn't become a phantom item.Suggestions
.gitea/PULL_REQUEST_TEMPLATE.md.limitenforcement (Markus #3), the 401/500 error UX (Sara #2 + my OQ-MENTION-02), the keyboard-nav E2E (Sara #3), and thearia-controlsaudit (Leonie #3) — that's seven follow-ups. Two are NFR-class, five are functional. File them now, link from #380, then close #380 with confidence.Open Questions
&limit=5query param is now sent by the client (good defensive cap). What does the backend do with it today? If it's ignored, the server still ships a full result set and the client slices to 5. If it's honored, great — but please verify and add a one-line note to the follow-up backend issue./api/personsstill silently shows the empty-state copy. Not addressed in this PR (correctly out of scope), but should be filed as a Gitea issue per my point #3 above.maxlength. Server should enforce — track via the backend follow-up.What's done well
keeps the "Namen eingeben…" prompt and fires no fetch when @ is followed only by spacestest inPersonMentionEditor.svelte.spec.tsis exactly the Given-When-Then I asked for, expressed as a behavior assertion. Closes my first-round concern #3 cleanly.@mention, I want the search dropdown to re-open with the search field pre-filled with the stored display text, so that I can correct or re-link the person without retyping the mention from scratch." That's Connextra-correct, scope is bounded, AC list is 5 explicit Given-When-Thens, NFRs are tagged. Closes my first-round concern #2.f67f5330is honest, not hidden. This is the cleanest review-loop close I've seen on this project. Adopt the pattern.SEARCH_DEBOUNCE_MSconstant is now named — even without the NFR, the magic number has a name, which is half the battle. When the NFR lands, it can simply assert against this constant.keeps the user-edited search value when editorQuery changes after the takeover) pins the design decision Felix flagged. The user-story invariant ("user takes ownership") is now executable. This is the right way to convert a design comment into a behavior contract.— Elicit
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
I re-read the diff commit-by-commit at
fcd4a41b. Both of my prior concerns are properly closed:ecc4d1aa) —items: async () => []with a clear comment crediting all four reviewers. The "exactly 1 fetch on @Walter" test (PersonMentionEditor.svelte.spec.ts, "fires exactly one /api/persons fetch when the user types @Walter") would have caught this before merge. That's the strong assertion I asked for, not a>= 1substring check. Red-then-green is provable because the olditems()body literally fired a fetch per keystroke — running this test against the prior implementation would yield 7 calls for@Walter, not 1.fb658e76) —MentionDropdown.test-host.svelteexposes a setter foreditorQueryvia anonReadycallback. The test asserts that after a user-takeover (fill('Walter')), pushing a neweditorQueryvia the setter does NOT overwrite. That pins the AC-1 ownership contract. The test-host pattern is also reusable for any future "editor-mirrors-into-child-but-child-takes-over" components.The other reviewers' concerns were addressed equally cleanly — I verified the request-token guard (
2556e7f5), the fixture-drift fix (b6bf24db), and the debounce JSDoc tightening (fcd4a41b).Concerns
None blocking.
Suggestions
One-line comment on
MentionDropdown.svelte:37-41mirror effect — the highlightedIndex variable above it has a 5-line explanation of why$state + $effectis correct over$derived. The new mirror effect deserves a peer: "mutable$statebecausebind:valuewrites it;$effectmirrors editor's typed text until the user takes ownership, after whichuserHasEditedpins it." Cheap context for the next reader.In-flight debounce can outlive
onExit—PersonMentionEditor.svelte:209assignscancelPendingSearch = () => debouncedSearch.cancel()insiderender(), but this hoisted module-level reference only tracks the LATEST dropdown's debounce. If a user opens dropdown A, types, hits Escape (onExit fires, unmounts dropdown A), then opens dropdown B before the 150 ms elapses — dropdown A's pending debounce will still fire its runSearch and silently mutatedropdownState.itemsfor dropdown B. The mutation is harmless (B's items get briefly overwritten before its own fetch resolves) but it's wasted network. AdddebouncedSearch.cancel()toonExit()to make each dropdown lifecycle close cleanly. Not a blocker — fix during the next ADR/cleanup pass.Test-host colocation —
MentionDropdown.test-host.sveltelives next to production code inshared/discussion/. A__fixtures__/subdirectory or.test-fixtures.sveltesuffix would make the boundary visible to the eslint-boundaries plugin and to future readers grep'ing the directory. Optional; keeping the colocation is also defensible.What's done well
userHasEdited,cancelPendingSearch,requestId,runSearch,SEARCH_DEBOUNCE_MS,POST_DEBOUNCE_SLACK_MS,editorQuery,mountedDropdown— every name carries intent. NoHelper, noManager, nod. Test constants also extracted from magic numbers.PersonMentionEditor.svelte:193-217) is textbook.requestIdbumped synchronously inonSearch, captured at runSearch start, checked twice (after fetch, afterawait res.json()) so a fast-fail 401 also discards correctly. Defensible against the trickiest race in this whole interaction.debounce.cancel()contract documented as "DROPS (does not flush)" — the surprising choice (lodash flushes by default) is now explicit. JSDoc serves its purpose.untrack(() => editorQuery)in bothMentionDropdown.svelte:33and the test-host — without this,searchQuerywould re-initialize every time editor reactivity flowed in, defeating the user-ownership tracking. Correct on the first read.onDestroycallscancelPendingSearch?.()beforeeditor?.destroy()so a trailing debounce can't fire against a destroyed editor. Order matters; the order is right.b6bf24db) —makePersonnow returns a typedPersonvia construction, AUGUSTE/ANNA are direct typed literals withoutas unknown as Person. If the backend adds a required field, these break loudly at compile time. Net win for the project.— Felix
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved with concerns
Re-reviewed the head at
fcd4a41bagainst my prior comment (#10933). All three items I raised in the first pass are addressed: themaxlength="100"soft cap landed, the duplicateitems()fetch is neutralised (was doubling/api/personsrequest volume per keystroke), and the stale-response race is killed by therequestIdtoken guard. Therel="noopener noreferrer"is present. Good fix-pass.Two new concerns surfaced while looking at the consolidated state, plus one observation about the soft cap's actual coverage. Nothing blocking — but worth landing before this pattern propagates.
Blockers
None.
Concerns
maxlength="100"is bypassed by the editor-mirror path (CWE-400, layered) —MentionDropdown.svelte:36-41mirrorseditorQuery(the text typed after@in the contenteditable) intosearchQueryvia a$effect. The contenteditable does NOT enforcemaxlength. A user can type@followed by 5,000 characters in the editor — the mirror writes the full string intosearchQuery, the$effectononSearchfires, and the editor'srunSearchissuesGET /api/persons?q=<5000 chars>&limit=5. The 100-char cap only applies when the user types directly into the<input>after taking ownership (userHasEdited = true).This is a real bypass of the defensive cap. Two options:
Or clip inside
onSearchinPersonMentionEditor.svelte:209-217before the debounce fires:I'd take the second approach — single chokepoint, server-shaped contract enforced at the network boundary. The current state means my CWE-400 fix is half-applied: the input element is capped, but the path from Tiptap → mirror → fetch is not.
Server-side cap on
?q=length is still missing — the client-side cap is not a security boundary (CWE-602: Client-Side Enforcement of Server-Side Security) — The fix-pass summary lists "Server-sidelimitenforcement +PersonSummaryDTO" as a deferred backend PR. Good — but the length ofqalso needs a server-side bound. Any actor scripting the page (compromised browser extension, JS in another opened mention dropdown after session takeover, future XSS) can bypass the DOMmaxlengthwith one line:The backend
PersonControllermust rejectqlonger than (say) 100 chars with HTTP 400 — and ideally rate-limit/api/personsat the same time since it's still authenticated-but-otherwise-unbounded. File a separate backend issue:Suggestions
MentionDropdown.test-host.svelteshipped atsrc/lib/shared/discussion/(not in a__tests__/directory) — Vite tree-shakes unimported modules, so it won't ship to production as long as nothing imports it from a route. To make the dev-only intent unambiguous (and protect against an accidental future import via a fuzzy autocomplete), either: rename the file to.test-only.svelteand add an ESLintno-restricted-importsrule, or move it tofrontend/src/lib/shared/discussion/__tests__/. Defense in depth against supply-chain "what is this file?" moments. Low priority.The empty-query short-circuit in
onSearchrunsrequestId++before the trim check (PersonMentionEditor.svelte:209-217) — correct ordering, good. Worth a one-line comment so a future refactor doesn't reorder it ("Bump requestId first so any in-flight response from a previous non-empty query is discarded when the user clears the input").What's done well
maxlength="100"on the search input. Soft cap, but the right kind of defensive control with explicit comment citing CWE-400. Test asserts the value (MentionDropdown.svelte.test.ts:179-184). The cap is visible, audited, and regression-proof.requestIdtoken-guard pattern (PersonMentionEditor.svelte:193-218) is textbook — capture id at fetch start, recheck before each state write, drop response on mismatch. Tested explicitly with a deliberately-delayedPromise(PersonMentionEditor.svelte.spec.ts:267-290). This was a real UX-leaks-into-security issue (a slow response repopulating after the user cleared the input could trick the user into selecting a stale-result person they didn't intend to mention).items()fetch (PersonMentionEditor.svelte:154-158) removes a per-keystroke amplification I called out in the first pass. The&limit=5query param added at the same time gives the backend a hint to bound response size before the wire (when the server starts respecting it).rel="noopener noreferrer"on the create-new link survives the fix-pass. Both halves of reverse-tabnabbing and Referer-leak (CWE-1022, CWE-200) are blocked.cancel()semantics documented: The JSDoc now says "DROPS (does not flush) the pending trailing invocation." This matters for theonDestroypath — a flush-on-cancel debounce would have fired a post-unmount fetch, which is exactly the kind of post-logout request leak that creates audit-log noise (or worse, if session timing was tight, a 401 firing into a closed component's error handler).encodeURIComponentretained. URL parameter injection blocked.innerHTML, no template-string interpolation into URLs, noeval. Clean.Note for follow-up
Nora #5618 #3reference in the file (line 152) — the open audit issue forGET /api/personsresponse-shape (PersonSummaryDTO leaksnotes) — is still relevant. Adding&limit=5doesn't fix the per-row payload, just the row count. A 5-row response with fullPersonobjects still ships private fields the transcriber doesn't need. Confirm that issue is still tracked separately.@Size(max = 100)lands, the frontend can drop the slice() clip — but until then, keep both layers. Defense in depth.— Nora
🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
Second-pass review of head
fcd4a41b. Every concern from my first review was either fixed or explicitly deferred with rationale — that's good practice and I appreciate the explicit mapping in the fix-pass summary. Test count went from 38→46. The new characterization tests are genuinely well-targeted. Two real flakiness risks remain and one coverage gap is cheap enough that I'd rather we close it now than ticket it.Blockers
None.
Concerns
1. The "exactly 1 fetch on
@Walter" test has a latent CI-jitter flake —frontend/src/lib/shared/discussion/PersonMentionEditor.svelte.spec.ts(test "fires exactly one /api/persons fetch when the user types @Walter (debounced)"):userEvent.typetypes 7 characters. Each one fires Tiptap's suggestiononUpdate→updateState→dropdownState.editorQueryflips → dropdown's$effectcallsonSearch→ debounce reset. If CI jitter ever spaces two consecutive keystrokes >150 ms apart, the trailing debounced call fires for the intermediate query before the next reset, producing 2 fetches. The request-token guard (requestIdbump) discards the stale response, butfetchMock.mock.calls.lengthstill increments because the request was sent. Assertion fails as flake.This is exactly the kind of catch I was asking for — but the assertion's robustness depends on tight per-keystroke timing that vitest-browser's
userEventdoesn't guarantee. Two fixes:await page.getByRole('textbox').fill('@Walter')for this test — single input event, deterministic, same coverage of the "user typed something" intent. The existing "editing the search input fires a debounced fetch" test already validates the per-keystroke case via type+wait. One test per timing mode.toBeLessThanOrEqual(1)— but that defeats the regression-catching purpose. Don't do this.I went back and forth here. Take (a).
2. Error-path coverage on
runSearch(401/500) is genuinely cheap and the test infrastructure is already in place — the stale-response race test already proves you can return a custom mock fromfetchto exerciserunSearch. The cost of adding a 5xx test is oneit()block:This both (a) characterizes the current behaviour (silent empty state) and (b) acts as a tripwire for the deferred follow-up #5 in your summary (distinct 401/500 UX). The day someone implements the toast, this test goes red and forces them to update the assertion. That's the test pyramid working.
Same argument for the
catch {}branch on network failure (fetchthrows). Twoit()blocks, ~10 lines of code each. Please add before merge.3. Keyboard-nav deferral to E2E is acceptable, with one caveat — the deferral is defensible (the suggestion plugin's keydown routing is real DOM behaviour, hard to fake at unit level). However: the
MentionDropdown.test-host.svelteshim now exists and is doing useful work for the sticky-takeover test. The same shim could host an "ArrowDown moves highlight from search input to list" unit test at the dropdown level (without the editor), sinceMentionDropdownowns the listbox and selection state. That's ~15 lines and would catch a regression that today only catches in E2E. Not blocking — keep it on the follow-up list — but flagging that the shim's reach is broader than you used it for.Suggestions
1. The
setTimeout(50)in the sticky-takeover test is brittle naming —MentionDropdown.svelte.test.ts, sticky-takeover test:Svelte 5
$effectflushes on the next microtask, so 50 ms is a bag of slack. Either:expect.element(...).toHaveValue(...)already has its own pollingawait tick()from'svelte'if you want to be explicit about flushingNaming a
setTimeout(50)as "give the effect time to flush" is the kind of thing future-Sara-reading-this-in-two-years will misinterpret as a race fix.2. The slack-bump rationale (over fake timers) deserves a one-line comment in the test file, not just the commit message —
PersonMentionEditor.svelte.spec.ts:18-22already explains why the constants exist but doesn't mention the fake-timer rejection. Add:Six lines. Saves the next reviewer from re-deriving the trade-off.
Flakiness risk
One real risk (Concern 1 above) + one near-miss. The near-miss:
await expect.element(page.getByText('Auguste Raddatz')).not.toBeInTheDocument()in the stale-response race test is asserted ~50ms afterresolveFetch(...). Microtasks should flush, so this should be deterministic — but if any downstream effect is on arequestIdleCallbackor similar, you'd get false negatives (test passes because the element hasn't been added yet, not because the guard worked). Recommend: also assertexpect(dropdownState.items.length).toBe(0)if exposed, or extend the wait to 200ms. Defensive but cheap.What's done well
MentionDropdown.test-host.svelteshim withonReady-exposed setter is the right level of indirection. It documents the intentional design (user-edit wins) without leaking implementation details. Future refactors that break the contract will fail this test, which is exactly what you want.@) is solid — pins both the prompt copy AND the no-fetch behaviour, which is the AC-4 ambiguity Elicit raised. The combined assertion (getByText(prompt).toBeInTheDocument()+expect(fetchMock).not.toHaveBeenCalled()) is exactly how I'd write it.makePersonfactory now typed againstPersondirectly —MentionDropdown.svelte.test.ts:14-25. Caught the missingpersonType/familyMemberdrift. This is the test discipline I asked for and the team paid it back with interest.&limit=5test —PersonMentionEditor.svelte.spec.ts"appends &limit=5 to the fetch URL" — defensive cap pinned by a test means Markus's concern can't silently regress.runSearch—PersonMentionEditor.svelte:191-211. The pattern (captureidat fetch start, re-check after eachawait) is textbook. The accompanying test (stale-response race) covers the exact scenario it defends against.Test plan note (carryover from first review)
The manual 7-checkbox test plan in the PR description still hasn't been run (per your own note: dev stack not up). For a UI feature with brand-token and viewport concerns Leonie raised, that's the gap I'd close before merge — even a single Playwright session capturing screenshots at 320 px / 768 px would lock those in. Not blocking.
— Sara
🎨 Leonie Voss — Senior UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
Re-reviewed at head
fcd4a41b. The fix pass discharged FINDING-MENTION-001 (Blocker) and FINDING-MENTION-002 (Concern) cleanly. Magnifier is nowh-5 w-5 text-ink-2(20 px, +25 % vs. the original 16 px, with darker ink for affordance), and the empty-state copy collapsed into a single<p aria-live="polite">that derives its text fromsearchQuery.trim() === ''— exactly the shape I recommended. Tests for both fixes exist inMentionDropdown.svelte.test.ts("renders the magnifier icon at h-5 w-5 with text-ink-2 (Leonie BLOCKER on PR #629)" and "announces empty-state copy via aria-live="polite" (Leonie FINDING-MENTION-002 on PR #629)").I'm lifting the Blocker. Three smaller findings remain — none ship-stoppers — plus the 320 px viewport check that's still outstanding.
Blockers
None.
Concerns
FINDING-MENTION-004 — Search input's accessible name is mismatched against its purpose
MentionDropdown.svelte:151reusesm.person_mention_btn_label()("Person verlinken" / "Link person") as the<label sr-only>for the search input. A screen reader user lands on a<input type="search">and hears "Link person, search" — but the input doesn't link anything; it filters a candidate list. The verb-mismatch is small but creates the "is this the same control I just heard the listbox announce?" confusion you specifically wanted to avoid.<label sr-only for="mention-search">. Keepperson_mention_btn_labelfor the listboxaria-label. Five minutes; pure i18n + one line.FINDING-MENTION-005 — 320 px viewport still unverified, and
w-72is fragilew-72= 288 px, so the listbox itself fits, but theposition.leftis anchored to the caret's absolute viewport X — on a 320 px viewport with the caret near the right edge, the listbox can still overflow horizontally. The flip logic atMentionDropdown.svelte:108-118handles the vertical overflow only.FINDING-MENTION-006 —
text-sm(14 px) on the search input is below the senior-audience floorMentionDropdown.svelte:163— the<input>usestext-sm(14 px). The list items below it usetext-base(16 px) for the name andtext-xsfor life dates. The input is the primary write surface the senior is typing INTO. Right now it's the smallest non-metadata text in the dropdown.text-base(16 px) — it costs ~2 px of vertical rhythm in the popover header and zero a11y debt. This matches the persona's 16 px minimum body text rule.Suggestions
Whitespace-only state should announce too — the new test
keeps the "Namen eingeben…" prompt and fires no fetch when @ is followed only by spacesdocuments the behavior, which is good. Thearia-live="polite"<p>will already announce the prompt when the items list is empty, so this is fine in practice — but I'd love a screen-reader transcript check (NVDA + VoiceOver) when Sara's a11y E2E lands. Not blocking.maxlength="100"UX — Nora's CWE-400 cap is exactly right at the input layer. Worth confirming with a transcriber that 100 chars is enough for the longest plausible historical name + role they'd type (e.g. "Friedrich Wilhelm Heinrich Karl Ludwig von Hohenzollern" = 56). It is — but if someone ever pastes a full salutation into the search ("Seine Hochwohlgeboren Friedrich Wilhelm..."), the silent truncation could surprise them. A future enhancement: addaria-describedbypointing to a small "max 100 characters" hint that only appears when ≥ 90 chars are typed.Magnifier icon is now
text-ink-2— good. Verify the contrast ratio againstbg-surface(which I expect remaps to--c-surface).--palette-ink-2should be in the 5:1+ range on the warm sand surface — pull a quick axe-core check in the next E2E pass.What's done well
h-5 w-5 text-ink-2magnifier reads as a strong affordance. Combined withtype="search"(which gives mobile keyboards a clear/search button) and the persistent placeholder, the input is identifiable even after the placeholder vanishes. The lighter-touch path was the right call; a visible label would have added visual clutter inside a popover that's already information-dense.<p aria-live="polite">with conditional text is exactly the WCAG 4.1.3 pattern. Svelte will keep the same DOM node across state changes (no remount), so the live-region announcement fires reliably on text content change. The inline comment crediting the finding is also a nice habit — it tells the next contributor why this isn't two<p>elements.rel="noopener noreferrer"on/persons/new— both halves of the protection, double-checked in the test (expect(link.rel).toContain('noreferrer')).min-h-[44px]assertions across the dropdown. WCAG 2.5.5 cleanly met.text-ink,text-ink-2,text-ink-3,bg-surface,bg-canvas,border-line,text-brand-navy,bg-brand-mint/20,ring-brand-navy— no raw hex, no Tailwind palette colors, every value flows through the semantic token system. The earlier round's inline comment onbg-brand-mint/20 ring-brand-navy(1.4.11 Non-Text Contrast) is preserved.focus) — keyboard users see the ring, mouse users don't. Correct discrimination.person_mention_search_promptkey landed in all three locales (de.json,en.json,es.json) with culturally appropriate copy ("Namen eingeben…" / "Enter a name…" / "Escribe un nombre…").MentionDropdown.test-host.svelteis a clean, single-purpose$stateproxy that lets the test driveeditorQuerymutations without leaking the pattern into production code. Good test-architecture instinct.Note for the senior audience
The remaining concerns all point in the same direction: this dropdown is a write surface for a 60+ transcriber, and the senior-audience constraints (
text-baseminimum body, visible affordances persistent across state, predictable horizontal overflow on small phones) take precedence over visual compactness inside a popover. Specifically:text-baseon the search input addresses the "I can't read what I'm typing" complaint that has historically been #1 in senior-usability tests of search bars.max-w-[calc(100vw-1rem)]addresses the silent failure mode where a transcriber working on a small phone in landscape sees the dropdown clip off the right edge with no scroll affordance.Personen suchenas the SR label addresses the screen-reader user who needs the control's purpose announced in plain language — "search" is the verb their muscle memory expects on a<input type="search">.None of these are merge-blockers. All three are 5-minute fixes that would discharge any remaining a11y debt I can see in this surface. If you want to bundle them into a follow-up issue, I'd suggest one issue titled "Mention dropdown — senior-audience polish (text-base, search-label, max-w)" so they don't get lost.
Ship from my side once you've at least eyeballed the dropdown at 320 px. The rest is polish, not gating.
— Leonie
Round-2 review-fix summary
Each concern from the seven persona round-2 reviews mapped to its resolution. New head at
4f2880a6.?limit=enforcement untrackede37351f5) addsmax-w-[calc(100vw-1rem)]defensive cap; E2E itself filed #635cancelPendingSearchonly tracks latest dropdown lifecycle58358e84) —debouncedSearch.cancel()in onExit + red→green test$effectdeserves a comment likehighlightedIndexa8224795) — 5-line explanation comment added7603c8d9) — renamed.test-host.svelte→.test-fixture.sveltemaxlength="100"bypassed via editor-mirror path (CWE-400 layered)b5455066) —MAX_QUERY_LENGTHconstant, clipped at mirror + bound tomaxlengthattrq.length()cap missing (CWE-602)?limit=and PersonSummaryDTO)@Walterkeystroke race could fire ≥2 fetches on CIb6b9235d) — switched tofill('Walter')for deterministic single input event34b6a8a2) — two characterization tests pin current silent failure; UX follow-up #6342df46b71) — exercises exportedonKeyDowndirectly viamount+flushSync; full focus-chain E2E filed #635332d8197) — newperson_mention_search_labelkey in de/en/es; sr-only label switchedw-72listbox can overflow at 320 pxe37351f5) —max-w-[calc(100vw-1rem)]text-sm(14 px) below senior 16 px floor4f2880a6) —text-basearia-controls+aria-activedescendantchain not wiredDeferred to follow-up issues
Verification at new head
npx vitest run src/lib/shared/discussion/MentionDropdown.svelte.test.ts src/lib/shared/discussion/PersonMentionEditor.svelte.spec.ts— 56 / 56 green (up from 38 → 46 → 56 across the two review rounds).npm run check— no new errors in changed files.npm run lint— clean.Pushed at
4f2880a6.🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Round-3 review at head
4f2880a6. The two round-2 architectural concerns I raised in scope (#10959) are both discharged. No new structural concerns surfaced from re-reading the final state. The PR is shippable from an architectural standpoint.Blockers
None.
Concerns
None new. My round-2 items are tracked:
PersonMentionEditor.svelte:140-153still references "Markus #5616: an ADR will formalise this" — once #631 lands, that comment should link the ADR file path. The follow-up acceptance criteria already capture that step, so no extra action here. The framing is exactly what I'd want from the ADR itself; the next PR just needs to fill inAlternatives(+server.tsproxy hop, SvelteKit form action SSR round-trip) andConsequences(Tiptap/ProseMirror suggestion plugins now have a sanctioned escape hatch).?limit=+ query length +PersonSummaryDTO) — filed, three concerns bundled cleanly. Good call combining with Nora's CWE-602 and the pre-existingPersonSummaryDTOaudit; a single backend PR closes the whole "client filters what the server should" loop.rerender(), optional ESLint guard against importing.test-fixture.sveltefrom non-test code. The.test-host→.test-fixturerename (commit7603c8d9) also helps — the suffix now reads as "test infrastructure" rather than "a host component you might mount in prod." That's already an in-PR improvement over my round-2 state.Suggestions
Single-source-of-truth on the 100-char clip is cleaner than I expected. Re-reading the diff carefully: the clip lives only in
MentionDropdown.svelte(MAX_QUERY_LENGTH = 100, applied at theeditorQuery.slice(0, ...)mirror initialization, in the$effectmirror sync, and bound to the<input maxlength>attribute).PersonMentionEditor.sveltedoes not re-clip — it just hands the editor query throughdropdownState.editorQuery = renderProps.queryand the dropdown clips it on receipt. TherunSearchcall then receives the already-clipped value viaonSearch(searchQuery). This is the right boundary: the dropdown owns the search-input contract, including its length cap. The editor owns the fetch. No coupling, no duplicated constant. Felix's worry from round-2 ("two clips, two places to update") was understandable from the commit message but isn't borne out by the final code. I'd actually leave it exactly as it is.Inline-getter pattern for
editorQuerymounting is worth documenting once.PersonMentionEditor.sveltelines 220-228 useget editorQuery() { return dropdownState.editorQuery; }in themount()props to make a$statefield reach into the imperatively-mounted child reactively. The five-line comment explains it well in context, but this is the second place this pattern appears (along withmodel: dropdownState), and #637's "convention for Svelte 5 test fixtures" is the natural home for it. Suggest folding the inline-getter pattern into the same docs page that #637 produces — both are workarounds formount()'s no-prop-accessor limitation.runSearch's request-token guard is reusable. Not for this PR. But when the second async dropdown (document linking? Tag picker?) needs the same primitive, factoruseRequestToken()intoshared/utils. The implementation is six lines and the contract is well-defined. Flagged as a "rule of three" anchor.What's done well
ErrorCode, no newPermission, no new domain concept (@mentionis a pre-existing term in the codebase), no architectural decision that needs an ADR in this PR (the ADR is correctly deferred to #631 with a tracked follow-up). Zero diagram updates required. This is a clean structural refactor inside one feature package.shared/discussionstill owns the editor;shared/utils/debounceis a single-purpose utility with a clear contract;MentionDropdownis still a dumb view (it receives state, firesonSearch, doesn't fetch); the fetch is still in the editor. The test-fixture rename is the cherry on top — it makes the test-time-only nature of the file unmissable.items: async () => []line still reads correctly at round-3. Tiptap needs something onitems()to keep the suggestion plugin alive; returning[]and routing the actual fetch through the dropdown'sonSearchis the version of this code I'd have written from scratch. The comment block crediting all four reviewers (Markus / Felix / Nora / Sara) makes the rationale legible to the next contributor — they'll see why this looks vestigial and won't "clean it up."onDestroydoes the right thing. Cancels the debounce, destroys the editor, unmounts the dropdown if it outlives the editor. The hoistedcancelPendingSearchandmountedDropdownrefs make the cleanup contract visible at the top of the file. Defensive without being paranoid.— Markus
🔧 Tobias "Tobi" Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Round 3 at head
4f2880a6. Like my first two rounds, there is nothing in my scope. This remains a pure-frontend PR: no Compose, no CI workflow, no env var, no port, no secret, no image tag, no healthcheck, no reverse-proxy rule, no observability config, and — confirmed again — no dependency change.Blockers
None.
Concerns
None in my scope.
Suggestions
The
MentionDropdown.test-host.svelte→MentionDropdown.test-fixture.svelterename (commit7603c8d9) is the right naming convention, but it leaves three siblings still using the old.test-host.sveltesuffix:frontend/src/lib/shared/services/confirm.test-host.sveltefrontend/src/lib/shared/discussion/PersonMentionEditor.test-host.sveltefrontend/src/lib/document/transcription/TranscriptionBlock.test-host.svelteOperationally this is fine — neither suffix is referenced by
eslint.config.js,vite.config.ts(testincludepatterns aresrc/**/*.svelte.{test,spec}.{js,ts}andsrc/**/*.{test,spec}.{js,ts}, neither pattern matches.test-host.svelteor.test-fixture.svelte), Tailwind content scan, Paraglide, or any Compose / CI file. So zero pipeline risk. But the mixed convention is a paper cut for the next person grepping*.test-host.svelte. Follow-up #637 is already filed to define the convention project-wide — once that lands, the three stragglers should be renamed in one sweep so the codebase reads consistently. Not blocking.#633 (backend cap +
limit+PersonSummaryDTO) and capacity planning. I read the issue body — it tracks exactly the server-side ceiling I flagged in round 1 (@Size(max=100),@Min(1) @Max(50) limit, DTO scrub ofnotes/alias/title). Two operational notes for when that work lands:?limit=is enforced, aCache-Control: private, max-age=30(or similar short-TTL) onGET /api/personsbecomes safe to consider — typeahead requests are read-only and benefit from per-tab caching. Worth a 5-minute experiment when the DTO is in place; not before, because today's response leaksnotesand we don't want intermediaries caching that.Bucket4jfilter at ~20 rps per session on/api/personsas the defensive backstop. Frontend self-limits at ~6.7 rps already (150 ms debounce, one inflight request); 20 rps gives 3× headroom for autofocus / batch interactions while still capping a compromised tab. I will file the chore the day #633 merges — flagging now so the work isn't orphaned.#634 (distinct error UX for
/api/personsfailure) — when that lands, the silent-failure-empty-state characterization tests in this PR (server failure → empty-state copy,network failure → empty-state copy) will go red and pin the new behaviour. Good. Just noting that the backend response on 401 for this endpoint matters operationally — if a session expires mid-typing, the user sees "Keine Personen gefunden" instead of a redirect. Either #634 covers that explicitly, or it should — please flag if not, I'll add a line item.Manual browser verification / 320 px viewport — still unchecked. Same caveat as rounds 1 and 2. The PR fix-pass summary cites "dev stack wasn't up" again. Three review rounds in a row is a real cost signal. Standing offer: I'll draft a
chore(devex): one-command dev-stack warmup (docker compose up -d --wait + health gate)issue on request — no action without your say-so.What I checked
fcd4a41b..4f2880a6): pergit log, ten commits — A158358e84, A2a8224795, A37603c8d9(the rename), A4b6b9235d, A534b6a8a2, A62df46b71, A7b5455066, A8332d8197, A9e37351f5, A104f2880a6. Touched files cumulatively at this head:frontend/messages/{de,en,es}.json,frontend/src/lib/shared/discussion/*(3 files),frontend/src/lib/shared/utils/debounce.ts. No others.git diff 0c0a4830..4f2880a6 -- 'frontend/package.json' 'frontend/package-lock.json' 'docker-compose*.yml' '.gitea/' 'Caddyfile' 'backend/pom.xml' '**/eslint*' '**/.eslintrc*'→ empty. No dependency, infra, or lint-config change at any point in the PR.eslint.config.jsline 85 only matches**/*.spec.ts/**/*.test.ts— does not mention either suffix.vite.config.tstestincludepatterns do not capture either suffix. No bundler / test-runner / lint behaviour change.q.length,limitenforcement (@Max(50)), and DTO scrub. Closes my round-1/2 capacity-planning flag once it ships.VITE_*,SENTRY_DSN,import.meta.env,process.env,${{ secrets.*— all unchanged).&limit=5fetch per search-input edit, no per-keystroke duplicate, request-token guard discards stale responses, onExit cancels pending debounce. Worst-case ~6.7 rps per user.This PR moves the operational needle in the right direction (request volume halved versus pre-PR) and nothing it touches creates a new failure mode for ops. Ship it.
— Tobi
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Third pass at head
4f2880a6. I re-traced the seven Acceptance Criteria from #380 through this head, re-checked the three concerns from round 2, and audited the seven follow-up issues (#631–#637) for spec health. All three of my round-2 concerns are now closed; the deferred-actions list has been promoted from PR comment to real Gitea issues; the AC-3 NFR citation in the PR description now points at a real issue. The work is requirements-clean and ready to merge.Blockers
None.
Concerns
None blocking. Two minor traceability notes captured under Suggestions.
Suggestions
#628 AC-5 — close the loop in the issue body itself before the issue is picked up. Round 2 raised that AC-5 is still an "Open Decision." The resolution in the fix-pass summary is "Posted suggested AC-5 rewrite as comment on #628 (issue body untouched)." The comment (#11033) is well-formed and unambiguous, but a future contributor coming to #628 cold will read the issue body first and see the (a) / (b) ambiguity — they have to scroll to comments to find the resolution. Before #628 enters refinement, fold #11033's wording into the AC-5 line and remove the Open Decisions section. No new work, just a one-line issue-body edit. Tracking this here so it doesn't fall through; not a blocker on #629.
#632 is well-formed but slightly under-specified in two places. The requirement statement is measurable (400 ms, p95, broadband — good Doherty anchor) and the rationale chain is captured (debounce 150 ms + p95 backend ~150 ms + render ~50 ms ≈ 350 ms). Two gaps before this NFR can be executed against:
fill()on the search input to the first list option being visible."perf-regression."These are clarifications, not Connextra deficiencies — file as comments on #632 or fold into the description on the next touch.
The 7-follow-up issues vary in spec density. #633, #635, #636 are implementation-ready. #634 still has a TBD. Specifically #634 ("distinct error feedback") lists three design options (toast / distinct empty-state / inline alert) and a recommendation, but no committed pick — the "Decision needed" section is open. Same anti-pattern as #628 AC-5 round 2: leaves the implementer to re-litigate the design. Before #634 enters a release milestone, the user-need owner (you) needs to pick one and convert it to a single Given-When-Then AC. Until then, #634 is
needs-refinement, notready.#637 (test-fixture convention) is light on AC. Three bullet points under "Required" but no measurable acceptance — "Convention documented" is unbounded. Recommend converting to two concrete Gherkin ACs: (a) a new fixture-using test in a different directory passes lint; (b) a documentation reference resolves from
frontend/CLAUDE.md. Otherwise it sits as a documentation chore that never quite finishes.MAX_QUERY_LENGTH = 100is now both a security cap (Nora #1) and an undocumented requirement. The constant is named, commented, and tested in two paths (#629 covers it well). But there's no NFR or requirement-side anchor — if a future contributor needs to know "why 100?", they only have a CWE-400 comment. Recommend a one-line addition to #633 (which already enforces this server-side): "NFR-SEC-MENTION-001: The system shall reject/api/personsqueries longer than 100 characters with HTTP 400 andcode = INVALID_INPUT." That makes the magic number a requirement, not a defensive number. Optional polish — not a blocker.Open Questions
&limit=5today? Tracked via #633. Closes once #633 ships.What's done well
SEARCH_DEBOUNCE_MS = 150..gitea/PULL_REQUEST_TEMPLATE.mdand.claude/personas/*.mdas the close-out convention.MAX_QUERY_LENGTH = 100is paired with two-path test coverage —maxlengthattribute on the input AND the mirror-clip path. Layered defence is the right CWE-400 posture and the requirement is now executable on both paths.keeps the user-edited search value when editorQuery changes after the takeover) executes the user-story invariant ("user takes ownership") as a behaviour contract, not a code comment. That's how you turn a design decision into a durable requirement.frontend/messages/{de,en,es}.jsonparity is maintained — the newperson_mention_search_labelkey landed in all three locales in the same commit (A8332d8197). i18n NFR debt zero.— Elicit
🎨 Leonie Voss — Senior UX Designer & Accessibility Strategist
Verdict: ✅ Approved
Third pass at head
4f2880a6. Round 2's three concerns — FINDING-MENTION-004 (verb-mismatched sr-only label), FINDING-MENTION-005 (320 px overflow), FINDING-MENTION-006 (text-smbelow the senior floor) — are all discharged. The follow-up I called for (aria-controls+aria-activedescendant) is filed as #636 with the right scope. From my domain I have zero merge-blockers and zero must-fix concerns. Two small notes below — both in the suggestion bucket, neither gating.Blockers
None.
Concerns
None gating. Everything below is in the suggestion bucket.
Suggestions
S-1 — Locale verb-number drift in
person_mention_search_label(WCAG 2.5.3 / 3.1.3; cosmetic)The three locales landed as:
de.json"Person suchen"(singular)en.json"Search persons"(plural)es.json"Buscar persona"(singular)That's a real but minor inconsistency: German/Spanish announce "Search person, search, edit text", English announces "Search persons, search, edit text". WCAG 2.5.3 (Label in Name) is technically satisfied in each locale because each label is internally consistent with its placeholder ("Namen eingeben…" / "Enter a name…" / "Escribe un nombre…"). Cross-locale parity is a project-style thing, not an a11y violation.
If you want the polish, pluralise all three (
Personen suchen/Search persons/Buscar personas). If not, leave it — it does not block merge and the senior transcriber will never hear two locales in one session.S-2 —
MAX_QUERY_LENGTH = 100silent clip needs a soft user signal eventually (WCAG 3.3.1 Error Identification — informational rather than strict; tracked in suggestion #2 of round 2)The 100-character cap is the right defence-in-depth play for CWE-400 amplification (Nora). The implementation correctly clips both paths (
maxlengthattribute on direct user input,editorQuery.slice(0, MAX_QUERY_LENGTH)on the mirror). My concern is what the user perceives when a paste of "Seine Hochwohlgeboren Friedrich Wilhelm Heinrich Karl Ludwig von Hohenzollern-Sigmaringen…" silently drops the tail.For now: fine. The 99th-percentile transcriber name is well under 100 chars. But when #636 (aria-activedescendant) ships, please bundle a tiny visible+SR-announced hint that only renders at
searchQuery.length >= 90:…wired to
<input aria-describedby="mention-search-hint" ... />. This is the "honest about what it does" rule from my Secure-Code §1. Not gating this PR — file as a follow-up alongside #634 if you want.S-3 —
aria-liveregion remounts on items transition (WCAG 4.1.3 Status Messages; verify under SR)MentionDropdown.svelte:198-202: the<p aria-live="polite">lives inside{#if model.items.length === 0}. Whenitemsgoes 0 → N, the live region unmounts and the list renders; when N → 0, the live region re-mounts. NVDA and JAWS generally re-announce when the live region re-enters the DOM, but VoiceOver on macOS has historically been fussy about that case. The cleaner pattern is one persistent live region above the conditional:That also closes the "N results found" announce gap (the current dropdown silently populates the list without telling SR users how many candidates appeared). Bundle into #636 — it pairs naturally with
aria-activedescendant.Note on #636 (aria-controls follow-up)
Read #636. The acceptance criteria are tight on the mechanics (
aria-controls,aria-activedescendant, stable IDs, axe pass, manual SR verification) but thin on announcement quality. Two things I'd add to its body before someone picks it up:Verification matrix — at minimum: NVDA + Firefox, NVDA + Chrome, VoiceOver + Safari, JAWS + Chrome. The aria-activedescendant chain behaves differently across these — JAWS in particular has a quirk where it announces the option without announcing list position. The issue currently just says "Manual SR test with NVDA or VoiceOver" (singular OR). For a write-surface used by 60+ transcribers on screen readers, the matrix matters.
Announcement script — the issue should specify what should be announced on each interaction (dropdown open, ArrowDown, ArrowUp, Enter, Escape, query change with N results, query change with 0 results). Without that script, the implementer can wire the ARIA attributes correctly and still ship a confusing experience. Suggested format:
I can post that script as a comment on #636 if you want — say the word. Not blocking this PR; raising it here so the follow-up doesn't ship with the same blind spot.
What's done well
FINDING-MENTION-004 — sr-only label correctly split into a distinct
person_mention_search_labelkey, referenced from the<label sr-only for="mention-search">, and the listbox keepsperson_mention_btn_labelasaria-label. The verb mismatch ("link person" vs "search persons") that I flagged in round 2 is gone. Screen-reader announce now reads as a search control, which is whattype="search"warrants.FINDING-MENTION-005 —
max-w-[calc(100vw-1rem)]is exactly the defensive cap I asked for. It interacts cleanly with the absoluteposition.leftbecausemax-wconstrains the box and the browser then resolves the overflow against the viewport edge — no JS, no flake. The vertical flip strategy atMentionDropdown.svelte:108-118is orthogonal (it adjuststop/bottomonly), so the two strategies don't fight. Tested at the unit level (caps the listbox width to the viewport (320 px reflow guard — Leonie FINDING-MENTION-005)). Confirmed via the rendered class string on the listbox div.FINDING-MENTION-006 —
text-base(16 px) brings the search input to the senior-audience floor specified in CLAUDE.md §Dual-Audience Design. The 44 pxmin-his preserved so vertical rhythm is undisturbed. Pinned by the new testrenders the @mention search input at text-base (16 px senior-audience floor — Leonie FINDING-MENTION-006)— explicitly asserts.toContain('text-base')AND.not.toContain('text-sm'), so regressions are caught both ways. That's good test discipline.MAX_QUERY_LENGTHconstant + double-clip (Nora #1 collaboration). Naming the magic, applying it at both edit paths, and pinning both paths in tests (caps the search input at maxlength=100ANDclips a long editorQuery mirror to 100 chars) is a clean architecture move — security-defence-in-depth that doubles as readability. I appreciate the inline comment crediting why two paths exist.aria-live="polite"collapsed into one<p>(preserved from round 2's discharge of FINDING-MENTION-002). Verified atMentionDropdown.svelte:198-202. The new test (announces empty-state copy via aria-live="polite") is correctly scoped to the empty-state branch.Test-fixture rename (
test-host.svelte→test-fixture.svelte) ��� small thing, but the.test-fixtureextension makes the test-only boundary visible at the filesystem level. Future contributors won't accidentally import the fixture from production code.i18n hygiene — three locales updated, no hardcoded strings, no orphan keys, the test asserts copy via
m.person_mention_search_prompt()rather than literal strings so locale changes don't break the test suite.Brand tokens intact —
text-ink,text-ink-2,text-ink-3,bg-surface,bg-canvas,border-line,text-brand-navy,bg-brand-mint/20,ring-brand-navy. No raw hex, no Tailwind palette colours, every value flows through the semantic token system. Across three review rounds.Atomic commit map in the fix-pass summary — every round-2 concern maps to one commit hash. Reviewer cost per round dropped meaningfully. Adopt this as the project PR template.
Note for the senior audience
This is the rare case where every round-2 concern from my domain pointed in the same direction — "senior transcribers need predictability and visibility on the write surface" — and the fix pass implemented all three with no UX regressions. The dropdown now:
max-w-[calc(100vw-1rem)]with no JS)text-basematching the project's 16 px floor)The remaining work —
aria-controls/aria-activedescendant, error UX, axe pass at 320 px, the four follow-up issues (#634, #635, #636 + the MAX_QUERY soft signal) — is correctly out of scope for this PR and tracked. The follow-up grouping (one PR-scope-creep prevention issue per axis: backend cap, error UX, E2E, ARIA, fixture docs) is the right granularity.Ship it.
— Leonie
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Round-3 re-read at
4f2880a6. I commit-by-commit-checked the 10 round-2 fixes against the four prior concerns I'd flagged and one new TDD spot-check. All five are properly closed at the code level — the new tests are behaviour-first, not tautologies, and would catch real regressions. One small follow-up nit on test-fixture naming consistency below.Blockers
None.
Concerns
.test-fixturerename is half-applied —frontend/src/lib/shared/discussion/MentionDropdown.test-host.sveltegot renamed to.test-fixture.svelte(commit7603c8d9), but its siblingfrontend/src/lib/shared/discussion/PersonMentionEditor.test-host.svelte(still imported atPersonMentionEditor.svelte.spec.ts:11) was missed. The fix-pass summary (#11037 row A3) phrases this as if both files moved, but only one did. The original concern (eslint-boundaries visibility, grep boundary) still applies to the editor host. Not a blocker — the test-fixture-naming-convention follow-up (#637) will catch it — but worth a one-line rename + import update in the next pass.Suggestions
onKeyDowntest inconsistency:flushSyncon Arrow keys, raw call on Enter —MentionDropdown.svelte.test.ts:281and:309wrap the call influshSync(() => …)because the test readsaria-selectedimmediately after;:338(Enter) doesn't, because it only assertscommand.mock.calls. Both work, but the asymmetry forces the next reader to figure out why. Wrap all four influshSyncfor uniformity, or drop it on the ArrowDown/ArrowUp tests and rely onawait tick()post-mutation. Cosmetic.Long-typed-query divergence between dropdown and command —
MentionDropdown.svelte:54clips the mirror toMAX_QUERY_LENGTH(100), butPersonMentionEditor.svelte:234(thecommandcallback) usesrenderProps.querydirectly as the inserteddisplayName. So a 200-char@-suffix in the editor shows 100 chars in the search input but inserts 200 chars as the mention's displayName. The CWE-400 layered cap is correct (fetch query is bounded), but the user-visible divergence between "what I searched" and "what got inserted" deserves a thought. Either also clip at the command boundary or accept the trade-off and add a one-line comment onPersonMentionEditor.svelte:234noting the asymmetry is intentional.What's done well
A1 fix is real, not cosmetic.
PersonMentionEditor.svelte:277callsdebouncedSearch.cancel()on the closure-scoped debounce insideonExit, with a comment explaining why the hoistedcancelPendingSearchreference would be stale by the time the trailing call fires. The new test atPersonMentionEditor.svelte.spec.ts:355-391is behavior-first — it waits for any in-flight fetch to settle, queues a freshfill('Walter'), fires Escape via the editor (so Tiptap's suggestion plugin's Escape handler actually triggersonExit), then assertswalterFetches.length === 0past the debounce window. Removing thedebouncedSearch.cancel()line would turn this red. That's the strong assertion shape.A2 comment is exactly the right length and altitude.
MentionDropdown.svelte:45-51— six lines, names the alternative ($derived), names why it's wrong (read-only, would clobberbind:value), and credits the latch (userHasEdited). Peer of thehighlightedIndexcomment two functions below. Reads cleanly.A3 rename's intent is right (test-fixture suffix makes the boundary visible to eslint-boundaries and to grep), even though only one of two files was renamed. The fix-pass commit message is accurate that the dropdown fixture renamed; the summary in #11037 overstates the scope. Easy follow-up.
A6 (
onKeyDownunit tests) are production-path tests, not tautologies.MentionDropdown.svelte.test.ts:259-366mounts the production dropdown, capturesinstance.onKeyDown(the real Svelte 5 export — same function Tiptap forwards events into), and calls it with a syntheticKeyboardEvent. The first assertion readsaria-selectedoff the DOM afterflushSync, the Enter assertion readscommand.mock.calls. If someone broke the modulo arithmetic atMentionDropdown.svelte:121-126(e.g. wrote% leninstead of% Math.max(len, 1)and the items list was empty), the ArrowDown test catches it. If someone removed the Escape early-return, the Escape test catches it. Real coverage.A7 (
MAX_QUERY_LENGTHlayered defence) is correctly threaded. Constant declared atMentionDropdown.svelte:17, used at line 42 (initial state), line 54 (mirror effect), and bound tomaxlengthat line 184. The two test cases atMentionDropdown.svelte.test.ts:181-197cover both paths (direct input maxlength + mirror clip). Removing either clip breaks one of the tests. Clean.Request-token guard at
PersonMentionEditor.svelte:195-216is textbook race protection.requestIdbumps synchronously inonSearch, gets captured at runSearch start, checked twice (afterawait fetch, afterawait res.json()). The stale-response test (PersonMentionEditor.svelte.spec.ts:295-319) holds a fetch open via an unresolved promise, clears the input, then resolves — and asserts the dropdown stayed empty. A one-line removal of either of the twoif (id !== requestId) returnguards turns this red. Defensible.debounce.cancel()JSDoc (debounce.ts:1-7) now spells out "DROPS (does not flush)". The lodash flush-on-cancel default is surprising; the JSDoc serves its purpose.Server-failure characterization tests (
PersonMentionEditor.svelte.spec.ts:325-351) — these pin the current silent-failure UX (Keine Personen gefundenon both 500 and network error) so a future implementer of distinct error UX is forced to update the assertions. That's the right pattern for "behavior we accept but want to be intentional about."— Felix
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Round-3 review at head
4f2880a6. Re-traced both of my round-2 concerns (#10969) plus the new attack surface introduced by the fix pass. Both are properly closed:Nora #1 (CWE-400 layered cap) —
MentionDropdown.svelte:18declaresconst MAX_QUERY_LENGTH = 100with a 6-line threat-model comment naming the bypass path it closes. The constant is applied at all three entry points tosearchQuery:let searchQuery = $state(untrack(() => editorQuery.slice(0, MAX_QUERY_LENGTH)))(MentionDropdown.svelte:48) — clips on first paint.$effect(() => { if (!userHasEdited) searchQuery = editorQuery.slice(0, MAX_QUERY_LENGTH); })(MentionDropdown.svelte:57-61) — clips on every editor keystroke until the user takes ownership.<input maxlength={MAX_QUERY_LENGTH} … />(MentionDropdown.svelte:186) — DOM-level cap once the user has typed into the search box.Because
onSearchfires off$effect(() => onSearch(searchQuery))after the mirror clip,runSearchinPersonMentionEditor.svelte:194-216can never receive a query longer than 100 chars from this component. TestMentionDropdown.svelte.test.ts:179-184("caps the search input at maxlength=100") andMentionDropdown.svelte.test.ts:188-196("clips a long editorQuery mirror to 100 chars") pin both ends. The CWE-400 layered defence is now complete.Nora #2 (CWE-602 server-side cap) — #633 is filed and the spec captures the three required controls in
PersonController.search:@Size(max = 100) @RequestParam String q(CWE-400/602)@Min(1) @Max(50) Integer limit(CWE-400 amplification, response side)PersonSummaryDTO { id, displayName, birthYear, deathYear, personType, familyMember }stripsnotes,alias,title(CWE-200)Acceptance criteria are precise enough to act on: integration test asserts 400 on 101-char query, limit honoured up to 50,
PersonSummaryDTOexposed via OpenAPI. Issue body cites my round-1/round-2 comments and Markus's twin concerns. Good triage.Blockers
None.
Concerns
None for this PR.
Suggestions
Test-fixture rename verification — Vite/SvelteKit tree-shaking is doing the work, no explicit exclusion exists (
vite.config.ts:1-103). The rename.test-host.svelte→.test-fixture.svelteis fine because the only import ofMentionDropdown.test-fixture.svelteis inMentionDropdown.svelte.test.ts(which the client-test project matches viasrc/**/*.svelte.{test,spec}.{js,ts}and the production build never reaches). I confirmed by reading the vite config — there's no production glob that would pull.sveltefiles unreachable from routes. The fixture stays out of the prod bundle.Defence-in-depth follow-up (low priority): add an ESLint
no-restricted-importsrule banning imports of**/*.test-fixture.sveltefrom non-test files. Right now an accidental autocomplete-driven import from a route would silently bundle it. The cost is one rule entry; the benefit is making the "test-only" boundary lint-enforced instead of name-convention-enforced. Worth filing as a follow-up.#633 covers the length/limit/DTO triple but omits rate-limiting on
/api/persons— A capped query length blocks one DOS vector; an authenticated user (or compromised tab) can still hammer/api/persons?q=ab10,000 times. Two options for #633 (or a sibling issue):LoginRateLimiter-style throttling per authenticated principal on/api/persons(e.g. 60 req/min). CWE-770 (Allocation of Resources Without Limits or Throttling).Note
/api/personsis aGET, so CSRF protection is irrelevant (no state mutation). I confirmed nothing in this PR turns/api/personsinto a mutating endpoint. No action needed on CSRF.What's done well
MentionDropdown.svelte:48,:57-61,:186) all enforceMAX_QUERY_LENGTH = 100. The threat-model comment at:11-17is exactly the kind of explicit security documentation that survives a refactor — it names what is being defended against, names the bypass path that was previously open, and names the server-side issue tracking the rest. This is the readable-security-code pattern from my persona doc applied perfectly.<input>.onSearchreadssearchQueryafter the clip rather thaneditorQuerydirectly (MentionDropdown.svelte:67-69), a script that programmatically changes the parent'seditorQueryprop — e.g. via the test fixture'ssetEditorQueryexported inMentionDropdown.test-fixture.svelte— cannot bypass the cap. Tested explicitly via the fixture..test-fixture.svelteis a clearer signal than.test-host.svelteand matches the prevailing Svelte 5 fixture convention. The file is 32 lines, single-purpose, and clearly off the production module graph.Personcast inrunSearchupdated toPersonSummaryDTO(separate PR)" line correctly anticipates the type-regeneration churn.get editorQuery() { return dropdownState.editorQuery; }accessor inPersonMentionEditor.svelte:225-227is a read-only getter — it cannot be exploited to leak parent state becausedropdownStateis a$stateproxy owned by the host editor. The request-token guard from round-1 is intact.encodeURIComponentretained,rel="noopener noreferrer"retained, noinnerHTML/template-string URL interpolation/eval. Clean.Note for follow-up
MAX_QUERY_LENGTHslice can be relied on as anything more than a UX hint. Until then, keep both layers — the frontend cap survives even if the backend cap regresses (defence in depth).Nora #5618 #3reference atPersonMentionEditor.svelte:151-152(PersonSummaryDTOnotesleak) is now folded into #633's DTO scope. Once #633 ships, the inline reference can be removed./api/persons). CWE-770 is the unaddressed sliver.no-restricted-importsrule for.test-fixture.svelteis the only forward-defence I'd suggest. Not blocking; not in scope for this PR.Approving. The round-2 layered cap fix is exemplary — explicit constant, threat-model comment, three independent enforcement points, regression tests for two of them. This is the standard I want for the rest of the codebase.
— Nora
🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
Third-pass review of head
4f2880a6. 56/56 green, factory functions correct, characterization tests well-targeted. Every round-2 concern was addressed in spirit; one of my round-2 fixes (A4) accidentally weakened the test it was meant to harden — the test no longer protects against the original double-fetch regression. Not a merge blocker because the&limit=5test catches it incidentally, but worth tightening before this pattern gets copied.Blockers
None.
Concerns
1. The A4 fix (round-2 Sara #1) accidentally removed the regression guard —
frontend/src/lib/shared/discussion/PersonMentionEditor.svelte.spec.tslines ~208-235, "fires exactly one /api/persons fetch when the user searches for Walter (debounced)":The original regression the test was guarding against was the duplicated
items()callback inPersonMentionEditor.sveltefiring a per-keystroke fetch in addition to the debounced search-input fetch (Markus & Felix flagged it in round 1). With the new structure:userEvent.type('@')fires once.items()callback, it would fetch ONCE here (only@is typed).fetchesBeforeSearchis captured AFTER that, so the regression fetch is silently absorbed into the baseline.fill('Walter')produces a single input event → one debounced fetch → assertion passes.A reverter who restores
items: async ({ query }) => { const res = await fetch(...); ... }would still see this test pass. The double-fetch regression is currently caught only incidentally by the&limit=5test — because the legacy code didn't include that parameter — but rename the parameter or change the legacy code slightly and that incidental catch goes away too.Fix (cheap): drop the
@open + baseline capture and useuserEvent.type('@Walter')end-to-end withfillas the post-open replacement, OR keepfilland assertexpect(fetchMock).toHaveBeenCalledTimes(1)against/api/persons(no baseline). My preference:That's the regression guard I asked for in round 1. It also subsumes the "appends &limit=5" test's incidental coverage. If the per-keystroke type race that the round-2 fix addressed kicks back in on CI, fix that with a
vi.waitForpolling for the fetch to land instead of a fixed setTimeout — the deterministic count is more valuable than the deterministic timing.2. The
setTimeout(50)in the sticky-takeover test was not addressed —frontend/src/lib/shared/discussion/MentionDropdown.svelte.test.tsline ~245 (sticky-takeover test):I flagged this in round 2 ("Naming a
setTimeout(50)as 'give the effect time to flush' is the kind of thing future-Sara-reading-this-in-two-years will misinterpret as a race fix") and it didn't make the fix list. Not a flake risk becauseexpect.elementhas its own polling, but the magic 50 ms is dead weight. Drop the line or replace withawait tick()from'svelte'. Six lines of net change.3. Fake-timer rationale comment was not added to
PersonMentionEditor.svelte.spec.ts— round 2 suggestion 2. The constants are named (SEARCH_DEBOUNCE_MS,POST_DEBOUNCE_SLACK_MS) but the rejection ofvi.useFakeTimers()is only in the commit message. Future maintainers re-litigating will not find it. Adding the four-line comment I suggested would close this loop.Suggestions
1. The onKeyDown unit tests (A6) are good but don't catch the forwarding-chain regression —
MentionDropdown.svelte.test.ts:222-368. The tests invokeinstance.onKeyDown(...)directly via the exported export. That validates the dropdown's selection logic in isolation, but does not catch a regression wherePersonMentionEditor.svelte'sonKeyDown(event) { return exports?.onKeyDown(event) ?? false; }callback gets dropped or rewired. If someone refactors the Tiptap suggestion config and forgets to plumbonKeyDownthrough, these tests still pass.A cheap way to close the gap without going full E2E: in the existing
PersonMentionEditorkeyboard-navigation describe (PersonMentionEditor.svelte.spec.ts:567-622), theArrowDown moves the highlight to the next resulttest already exercises the forwarding chain throughuserEvent.keyboard('{ArrowDown}'). That test covers the integration. The new dropdown-level tests are valuable for unit-level regression, but the comment in the spec file (// In production, Tiptap intercepts ArrowDown/ArrowUp/Enter at the editor level and forwards them...) overstates the coverage. Either add a one-liner to that block referencing the integration test inPersonMentionEditor.svelte.spec.ts, or call out explicitly that the unit tests do not exercise the forwarding chain.2. The 500-characterization test (A5) is well-built but its sibling network-failure test duplicates 90% of the setup —
PersonMentionEditor.svelte.spec.ts:431-449. Twoit()blocks with the sameawait userEvent.type(page.getByRole('textbox'), '@Aug')→await new Promise(setTimeout)→getByText(popup_empty)shape. Worth factoring into afor (const failureMode of [...])table-driven test:Not a round-3 fix — file under "next time someone touches this area." Keep your tests DRY when the failure semantics are the same.
3. The deferred items in #635 (E2E keyboard nav) look right — Tab from editor → searchbox → list → create-new is genuinely E2E territory (real focus chain, Tiptap's editor.commands.focus(), suggestion plugin event routing). The unit-level coverage in this PR (
MentionDropdownonKeyDown forwarding +PersonMentionEditorArrowDown integration) is the right floor. #635 should NOT come back into this PR.The one item I'd consider promoting: focus-on-open (does the search input autofocus when the dropdown opens? CLAUDE.md's UX section calls this out for senior-audience write path). That's a single Playwright assertion or even a unit-level
expect(document.activeElement).toBe(input)after mount. If it's already in #635, fine. If not, add it.Flakiness risk
Low. Round-2 closed the worst risk (the
@Walterkeystroke race) with thefillswitch. The 500 ms total slack (SEARCH_DEBOUNCE_MS + POST_DEBOUNCE_SLACK_MS = 500ms) is generous enough for CI jitter. One residual: the stale-response race test (PersonMentionEditor.svelte.spec.ts:412-428) still usessetTimeout(r, 50)afterresolveFetch(...). Microtasks should flush in <1ms in practice, so 50ms is plenty — but if any downstream effect ever lands onrequestIdleCallbackthe assertion could pass for the wrong reason (element hasn't been added yet, not because the guard worked). Watch this if you see intermittent false greens.What's done well
makePersonfactory now typed strictly againstPersonand usesPartial<Person>—MentionDropdown.svelte.test.ts:14-25. Carries the type-discipline win from round 1 forward;personTypeandfamilyMemberare no longer drift candidates..test-host.svelte→.test-fixture.svelte) — A3 / commit7603c8d9. Makes the test boundary visible in the file tree. Right call. Adopting this convention project-wide (via #637) is the correct follow-up; please don't let that issue stagnate.PersonMentionEditor.svelte.spec.ts:476-516. The test is a textbook integration-level race characterization: stage state, force the race window, assert the guard held. The accompanying production-code change (debouncedSearch.cancel()insideonExit's closure scope, not via the hoistedcancelPendingSearch) is exactly right — the closure binds to the specific dropdown lifecycle, so a trailing call cannot fire against the next dropdown's state.editorQueryinjection vector; themaxlengthattribute test catches direct-input. Twoit()blocks for two attack surfaces. This is how layered defence gets pinned by tests.@Disabledducking. Test suite grew by ~50% on a single 757-line feature PR. That's the discipline I want to see.Test plan note (carryover, last time)
The 7-checkbox manual test plan in the PR description is still unverified. For a UI feature with 320 px viewport concerns (#635 is filed but not in this PR), I'd at minimum capture a screenshot at 320 px and 768 px before merging — even one Playwright session. The
max-w-[calc(100vw-1rem)]cap is now unit-tested for presence but the actual reflow behaviour at 320 px is unverified. Not a blocker; flagging it because manual checklists fade and the next reviewer doesn't know what was actually clicked.— Sara
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.