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
Round-3 review-fix summary
Each round-3 persona concern from the seven reviews mapped to its resolution. New head at
9764ada8..test-host.sveltefiles retained the old suffixca0d5399) —git mvPersonMentionEditor, confirm, TranscriptionBlock fixtures + updated imports/api/personsrate-limit gap (CWE-770)chore: one-command local dev stack startup with healthcheck waitMAX_QUERY_LENGTH = 100lacks NFR anchormentionConstants.tsso a future NFR can cite the symbolperson_mention_search_label021a0c6c) — en aligned to "Search for a person" (singular, matching de/es)MAX_QUERY_LENGTHsilent clip needs a soft signalfeat(a11y): soft length hint on @mention search input (>=90 chars)1757b01a) — persistent<p aria-live="polite" class="sr-only">above the conditional; announces empty/0/N count; newperson_mention_results_count_{singular,plural}i18n keys.test-fixturerename half-appliedca0d5399) — completed across 3 sibling fixturesflushSyncinconsistency on onKeyDown tests31e7d97c) — wrapped Enter/Escape influshSyncuniformly + rationale comment1081f5d2) — hoistedMAX_QUERY_LENGTHtomentionConstants.ts; A2 (26f1aeaa) — clippeddisplayNameatupdateState's command boundary in PersonMentionEditor; red→green test pins the capno-restricted-importsfor.test-fixture.svelte9764ada8) — flat-configno-restricted-importsrule banning the pattern from non-test files; verified to fire on a probe/api/personsb3d49b28) — dropped baseline subtraction; count ALL/api/personsfetches from render onwardssetTimeout(50)in sticky-takeover test not addressed27e8c96c) — replaced withawait tick()+ importtickfromsvelte31e7d97c, same commit as A6) — added a block comment above the describe explicitly calling out the integration test inPersonMentionEditor.svelte.spec.tsas the forwarding-chain coverageDeferred items (intentional)
Updated follow-up issues
New follow-up issues
chore: one-command local dev stack startup with healthcheck wait(devops, documentation).feat(a11y): soft length hint on @mention search input (>=90 chars)(feature, ui, P3-later).Verification at new head
npx vitest run src/lib/shared/discussion/MentionDropdown.svelte.test.ts src/lib/shared/discussion/PersonMentionEditor.svelte.spec.ts— 59 / 59 green (26 dropdown + 33 editor; up from 56 at round 2).src/lib/shared/services/confirm.svelte.test.ts+src/lib/document/transcription/TranscriptionBlock.svelte.spec.tsfor the rename — 86 / 86 green combined.npm run check— no new errors in changed files; pre-existingstate_referenced_locallywarnings on unrelated files unchanged.npm run lint— clean (prettier + eslint, including the newno-restricted-importsrule).New commits (9)
Pushed at
9764ada8.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Fourth pass at head
9764ada8. I re-checked each of the three round-3 concerns against the updated follow-up issues and audited the two new ones (#638, #639) for spec health. All three round-3 concerns are now properly closed in the issue tracker itself — not just in comments. The AC traceability from #380 through PR #629 to the follow-up backlog (#628, #631–#637, #638, #639) is end-to-end, executable, and free of unresolved ambiguity in any Must-have requirement. From a requirements-engineering standpoint, this PR is ready to merge and the follow-up backlog is in the cleanest state I've seen on this project.Blockers
None.
Concerns
None.
Suggestions
#638 acceptance criteria are bullet-list shape, not Given-When-Then. The five
- [ ]items are measurable enough for a chore issue ("single command produces a clickable URL within ~60 s" is a real threshold), but if you ever want this issue to drive a test rather than a manual checkbox, AC-5 (the cold-machine clickable-URL check) is the only one that's testable today. Optional polish: convert AC-5 into "Given a cold machine with no containers running, when the operator runsmake dev, thencurl http://localhost:5173andcurl http://localhost:8080/actuator/healthboth return 200 within 60 s." Not blocking — chores legitimately stay lightweight.#639's threshold value (
>= 90) has no requirement-side anchor. The constantMAX_QUERY_LENGTH_HINT_THRESHOLD = 90is named (good) and the hint copy is locale-correct, but the choice of 90 vs. 80 vs. 95 isn't justified — it's a 10% leading-edge buffer that feels right but isn't tied to any spec. Worth one line in the issue body: "Threshold = 90 chars (10% leading-edge buffer before the 100-char hard cap). If 80 or 95 ever needs justification, document the rationale here." Same class of "magic number with no NFR" hygiene asMAX_QUERY_LENGTH = 100was on round 3, but lower-stakes because #639 is P3-later.Carry-over from round 3:
MAX_QUERY_LENGTH = 100is still without an NFR anchor. The round-3 fix-pass summary deferred this to "as part of #633's CWE-770 section work." Looking at #633 today, the CWE-770 rate-limiting section was added, but the CWE-400 /MAX_QUERY_LENGTHserver-side enforcement NFR I suggested ("The system shall reject/api/personsqueries longer than 100 characters with HTTP 400 andcode = INVALID_INPUT") isn't yet in #633's body. Not a blocker on #629 — the client cap ships and works — but please confirm it lands in #633 before #633 enters refinement, otherwise the magic number remains undocumented at the requirements layer.Open Questions
MAX_QUERY_LENGTHNFR (Suggestion 3 above).&limit=5honoring — tracked via #633.What's done well
All three round-3 concerns are now closed inside the follow-up issues themselves, not just in PR comments. This was the round-3 ask: stop leaving the answer in a comment and edit the issue body. Verified at head
9764ada8:t_keystroke_stop(lastfill()/type()) tot_dropdown_render_complete(first option visible), against a 100-query sample with varied length and prefix uniqueness, plus Tempo/Grafana andpg_stat_statementsas observability sidekicks. "Failure criteria" splits CI (build-blocking) from production (P2 follow-up, Grafana p95 alarm 10 min sustained). The NFR is now executable: a CI job either fails or doesn't, and a production breach has a defined response path. This is exactly the gap I named on round 3.#638 and #639 are well-formed for their scope. Both have Context, Required, Acceptance, Out of scope. #638 is chore-shaped and stays appropriately lightweight (devops, documentation labels — correct). #639 has a soft-signal accessibility AC that pairs with WCAG 3.3.1 Error Identification, an
aria-describedbyrequirement that interacts cleanly with #636'saria-controls/aria-activedescendant, and a unit-test boundary atlength === 89/length === 95that pins the threshold behaviour. The dependency note on #636 (third ID on the same input) is exactly the kind of cross-issue interaction hint that prevents re-litigation downstream.The traceability chain is now end-to-end and fully recoverable. From the goal in #380 down to the test assertion line:
SEARCH_DEBOUNCE_MS = 150(named constant).MAX_QUERY_LENGTH = 100→mentionConstants.ts(named symbol) → pending #633 NFR (suggestion 3).MAX_QUERY_LENGTH_HINT_THRESHOLD = 90→ #639 (soft a11y signal).The fix-pass summary's commit-mapped table is the model. Each round-3 concern → resolution → commit SHA (or "folded into #N") → status. The "Deferred items (intentional)" section is honest, named, and bounded (5 items, each with a one-line rationale). The "Updated follow-up issues" section gives reviewers a one-click path to verify each issue body actually moved. This is exactly the close-out artifact I asked for on round 2 and it's been maintained across rounds 3 and 4 — pattern is durable, not a one-off.
Aria-live region promotion to a persistent element (A9
1757b01a, Leonie S-3) is a quiet requirements win. Persistent regions don't reset their politeness state on conditional remount — this means the announcement contract ("results count is announced after every query") is now a structural invariant, not a per-render coincidence. The two new i18n keys (person_mention_results_count_{singular,plural}) carry the singular/plural NFR-i18n-001-style hygiene. No locale parity debt.Test count growth 38 → 46 → 56 → 59 (dropdown+editor only); 86 total combined with sibling fixtures. Pure additive coverage across four rounds. No test weakened, no AC dropped, no scope creep. The traceability matrix from AC → test name → file is still recoverable in under a minute, four rounds in.
no-restricted-importsESLint rule on*.test-fixture.svelte(Nora S-1, A109764ada8) turns Tobi's round-3 rename concern into a structural guarantee. The convention isn't a doc anymore — it's a CI gate. Same pattern as #639'saria-describedbytest boundary: convert a convention into an executable rule and the convention stops eroding.— Elicit
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved (merge-ready)
Fourth pass at head
9764ada8. I re-verified each of the three round-3 concerns against the updated issue bodies themselves (not comments), audited the two new follow-ups (#638, #639) for spec health, and re-traced the full requirements chain from #380 down to the test-assertion layer. All three round-3 concerns are now closed inside the follow-up issues themselves. Every Must-have AC for this PR is delivered or transparently deferred to a well-formed issue. From a requirements-engineering standpoint, the work is ready to merge and the follow-up backlog is in the cleanest, most traceable state I've seen on this project. I am stepping back from blocking notes — this PR has been re-reviewed four times and the requirements artifacts are stable.Blockers
None.
Concerns
None.
Suggestions
These are polish items, not gates on this PR or on any follow-up's readiness.
#638 acceptance criteria are bullet-list shape, not Given-When-Then. Acceptable for a chore — the five
- [ ]items are checkable and AC-5 ("On a cold machine, a single command produces a clickablehttp://localhost:5173andhttp://localhost:8080/actuator/healthwithin ~60 s") is the only one that's directly machine-testable. If you ever want a CI smoke wrapper around this, convert AC-5 to: "Given a cold machine with no Compose stack running, when the operator runsmake dev, thencurl http://localhost:5173andcurl http://localhost:8080/actuator/healthboth return HTTP 200 within 60 s." Optional — chores legitimately stay lightweight.#639's threshold value (
>= 90) has no requirement-side anchor. The constantMAX_QUERY_LENGTH_HINT_THRESHOLD = 90is named (good) and the unit-test boundary pins it (length === 89→ hint absent;length === 95→ hint present), but the choice of 90 vs. 80 vs. 95 isn't justified — it's a 10% leading-edge buffer that feels right but isn't tied to any spec or user research. One-line addition to the issue body would close the loop: "Threshold = 90 chars (10% leading-edge buffer before the 100-char hard cap)." Same hygiene class asMAX_QUERY_LENGTHwas on round 3, lower stakes (P3-later).Round-3 carry-over (downgraded from a Concern): The
MAX_QUERY_LENGTH = 100server-side NFR I suggested in round 3 ("The system shall reject/api/personsqueries longer than 100 characters with HTTP 400 andcode = INVALID_INPUT") is now substantively present in #633 — the Required change #1 line ("@Size(max = 100) @RequestParam String q") and the Acceptance line ("Query cap enforced; integration test asserts 400 withcode = INVALID_INPUT") together encode the same requirement. The named NFR-SEC-MENTION-001 label is missing but the substance is there. Treating this as closed. If a future contributor wants to cite the requirement by ID, you can add anNFR-SEC-MENTION-001:prefix to that bullet in #633 — but the executable form is already in the issue.Open Questions
&limit=5honoring — tracked via #633 (Required #2:@Min(1) @Max(50) Integer limit).What's done well
All three round-3 concerns are now closed inside the follow-up issue bodies — not in PR comments. This was the explicit round-3 ask. Verified at head
9764ada8:t_keystroke_stop(lastfill()/type()) tot_dropdown_render_complete(first option visible), against a 100-query sample with varied length and prefix uniqueness, plus Tempo/Grafana andpg_stat_statementsas observability sidekicks. "Failure criteria" cleanly splits CI (build-blocking, p95 > 400 ms across 100 queries) from production (Grafana p95 alarm at 10 min sustained, treated as P2 follow-up). The NFR is now executable on both planes — a CI job either fails or doesn't, and a prod breach has a defined response path. This is exactly the gap I named on round 3.aria-activedescendantscope). Five Given-When-Then ACs cover 4xx-no-match, 401-session-expired, 5xx/network-retry, retry-success, and retry-still-fails (idempotent). The i18n strings table covers de/en/es for four new keys. Test plan extends the existing characterization tests. The cleanest "open design question → executable acceptance" conversion I have seen in this loop.#638 and #639 are well-formed for their scope. Both have Context, Required, Acceptance, Out of Scope sections. #638 is chore-shaped and stays appropriately lightweight (
devops+documentationlabels — correct typing). #639 has a soft-signal accessibility AC that aligns with WCAG 3.3.1 Error Identification, anaria-describedbyrequirement that interacts cleanly with #636'saria-controls/aria-activedescendant(the "Depends on / interacts with" section explicitly flags it as the third ID on the same input — exactly the kind of cross-issue interaction hint that prevents re-litigation downstream), and a unit-test boundary atlength === 89/length === 95that pins the threshold behaviour. P3-later labelling matches the polish-class severity.The traceability chain is now end-to-end and fully recoverable — from the goal in #380 down to the test-assertion line:
SEARCH_DEBOUNCE_MS = 150(named constant inmentionConstants.ts).MAX_QUERY_LENGTH = 100→mentionConstants.ts(named symbol) → #633 (server-side enforcement requirement, layered defence) →no-restricted-importslint rule (structural guarantee).MAX_QUERY_LENGTH_HINT_THRESHOLD = 90→ #639 (soft a11y signal, WCAG 3.3.1).The fix-pass summary's commit-mapped table (#11121) is the model close-out artifact. Each round-3 concern → resolution → commit SHA (or "folded into #N") → status, plus a separate honest "Deferred items (intentional)" section (5 items, each with a one-line rationale and a future-trigger condition) and an "Updated follow-up issues" section that gives the reviewer a one-click path to verify each issue body moved. This is exactly the close-out pattern I asked for on round 2 and it has been maintained across rounds 3 and 4 — the pattern is durable, not a one-off. Worth baking into
.gitea/PULL_REQUEST_TEMPLATE.mdand.claude/personas/*.mdas the close-out convention.Aria-live region promotion to a persistent element (A9
1757b01a, Leonie S-3) is a quiet requirements win. Persistent live regions don't reset their politeness state on conditional remount — this means the announcement contract ("results count is announced after every query") is now a structural invariant rather than a per-render coincidence. The two new i18n keys (person_mention_results_count_{singular,plural}) carry singular/plural NFR-i18n hygiene. No locale-parity debt across de/en/es.Test count growth 38 → 46 → 56 → 59 (dropdown+editor only); 86 total combined with sibling fixtures. Pure additive coverage across four rounds. No test weakened, no AC dropped, no scope creep. The traceability matrix from AC → test name → file is still recoverable in under a minute, four rounds in.
no-restricted-importsESLint rule on*.test-fixture.svelte(Nora S-1, A109764ada8) turns Tobi's round-3 rename concern into a structural guarantee. The convention isn't a doc anymore — it's a CI gate. Same pattern as #639'saria-describedbytest boundary at the threshold edge: convert a convention into an executable rule and the convention stops eroding. This is good requirements-engineering instinct — bake the rule into the gate, not the wiki.— Elicit
🔧 Tobias "Tobi" Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Round 4 at head
9764ada8. Fourth pass in a row with nothing in my scope to block on. This remains a pure-frontend PR: no Compose file, no CI workflow, no env var, no port, no secret, no image tag, no healthcheck, no reverse-proxy rule, no observability config, nopackage.json/package-lock.jsonchange, no Vite plugin or proxy change. All three of my round-3 follow-ups landed cleanly in the issue tracker.Blockers
None.
Concerns
None.
Suggestions
None. Two small notes for context, not changes to ask for:
no-restricted-importsrule (commit9764ada8) is correctly scoped —ignores: ['**/*.spec.ts', '**/*.test.ts', '**/*.test-fixture.svelte']exempts both the tests and the fixtures themselves, so it can only fire when a production.svelte/.tsfile imports a fixture. That is exactly the boundary we want, with no risk of failing unrelated files in the CI lint step. Runtime is one extrano-restricted-importsrule across the frontend tree — negligible. Probe noted in the fix-pass summary confirms it actually fires.mentionConstants.tsis three numericexport constdeclarations with no runtime side-effects. Both consumers (MentionDropdown.svelte,PersonMentionEditor.svelte) live in the sameshared/discussionchunk, so it tree-shakes / collapses into the existing bundle — no new code-split entry point, no extra HTTP request. Clean.What I checked
mainfor infra surface —frontend/package.json,frontend/package-lock.json,frontend/vite.config.ts,docker-compose.yml,docker-compose.prod.yml,docker-compose.observability.yml,.gitea/,.github/— all untouched (git diff origin/main..origin/feat/issue-380-decouple-mention-search -- ...returned empty).no-restricted-importsrule scoped viaignores. No new plugin, no new parser pass, no shape change to the existing flat-config pipeline. Lint duration impact is sub-second.mentionConstants.ts— same chunk as its two consumers; tree-shakable constants only; no new asset..test-fixture.svelterename completeness — confirmed at PR head (git ls-tree -r origin/feat/issue-380-decouple-mention-search): all four fixtures now follow the convention (MentionDropdown,PersonMentionEditor,TranscriptionBlock,confirm). No half-applied rename, no orphan.test-hostfile remaining on the branch.chore: one-command local dev stack startup with healthcheck wait) — labelsdevops,documentation. Scope is correct:docker compose up -d --waitwrapper, README + CONTRIBUTING.md +.devcontainer/README.mdupdates, optionalmake down/make logs/make verifycompanions, explicit out-of-scope on CI smoke-testing and on replacing existing escape hatches. AC is binary-checkable (http://localhost:5173+http://localhost:8080/actuator/healthwithin ~60 s on a cold machine). Mine to pick up.feat(a11y): soft length hint on @mention search input) — labelsP3-later,feature,ui. Not in my scope (frontend a11y), but labels and scope read clean from over the fence.Shippable from a DevOps/platform standpoint.
— Tobi
🎨 Leonie Voss — Senior UX Designer & Accessibility Strategist
Verdict: ✅ Approved
Fourth pass at head
9764ada8. The three round-3 suggestions I raised are all addressed: S-1 (locale verb-number drift) is resolved inline with A8, S-2 (silent paste-truncation hint) is filed as #639 with WCAG 3.3.1 framing and tight ACs, and S-3 (persistent aria-live region) is implemented in A9 with both empty-state transitions and populated-results count announcements. From my domain I have zero blockers and zero gating concerns. Three small notes below — none gating.Blockers
None.
Concerns
None gating.
Suggestions
S-1 — Plural agreement is in the component, not the locale message (i18n hygiene; cosmetic; tracked-forward)
A9 introduced two separate keys (
person_mention_results_count_singular/person_mention_results_count_plural) and the Svelte branches atMentionDropdown.svelte:209-213pick between them based onmodel.items.length === 1. For de/en/es this is functionally correct — all three locales have a simple binary 1/many plural rule and the keys land with the right gender agreement (es: "1 persona encontrada" / "{count} personas encontradas" — feminine singular vs. feminine plural, both correct).The architectural smell is that plural logic now lives in the template rather than in the message file. Paraglide supports ICU plural syntax natively:
with the message defined as:
That collapses two keys into one and — more importantly — works correctly the day someone adds Polish (3 forms:
one,few,many), Russian (3 forms), Arabic (6 forms), or Welsh (6 forms). Right now the component'sif length === 1 / elsebranch would silently mis-pluralize for any of those.For the current de/en/es-only scope: zero user impact, zero accessibility violation, zero gating value. File as a P3-later i18n-hygiene chore if the project ever picks up a non-binary-plural locale; until then, leave it. Not a merge blocker. (Verify Paraglide's plural syntax availability before filing — I'm assuming standard ICU MessageFormat support.)
S-2 — Visible empty-state and aria-live empty-state render the same string twice (WCAG 4.1.3; verify under SR)
At head
9764ada8, lines 204-214 (persistent live region) and lines 220-224 (visible empty-state<p>) render the same string for the same condition:The sr-only
<p>is the announcer (live region, polite). The visible<p>is the visual copy. A sighted screen-reader user (JAWS user with low vision navigating the dropdown by arrow keys, for example) will encounter the visible<p>during DOM traversal and the sr-only<p>via the live region. NVDA generally de-duplicates identical announcements within a short window; VoiceOver does not.Two ways to close this cleanly:
I prefer Option A — keeps the contract simple ("the sr-only
<p>is the truth, the visible<p>is decoration") and survives future restructuring. Not gating because the duplicate announcement is mildly noisy, not confusing — but worth one line.Verify with VoiceOver on the next manual a11y pass (or fold into the #636 verification matrix I asked Sara/Felix to expand).
S-3 — Mirror text inside the sr-only live region remains stale when the visible copy updates (low-priority, only relevant if S-2 is addressed by Option B)
If S-2 lands as Option A, ignore this. If it lands as Option B (drop empty-state from the live region), make sure the visible empty-state
<p>is not markedaria-live— it's currently silent, which is correct, but I want to flag it so a future contributor doesn't addaria-liveto the visible copy as a "fix" and create the double-announce that S-2 warns about.What's done well
A8 — Locale verb-number parity (round-3 S-1 closed)
The en key landed as
"Search for a person"rather than the literal"Search person". That's the right call — "search person" is grammatically valid but reads as a clipped header label, while "search for a person" is the natural English imperative the user actually expects from a search input. de stays"Person suchen", es stays"Buscar persona". All three are singular, all three read as a natural search-affordance label in their respective locales, and the cross-locale verb-number drift I flagged in round 3 is gone. WCAG 2.5.3 (Label in Name) holds in each locale because the visible/announced label aligns with the placeholder ("Namen eingeben…"/"Enter a name…"/"Escribe un nombre…").A9 — Persistent aria-live region (round-3 S-3 closed; WCAG 4.1.3 Status Messages)
Lines 196-214 hoist the
<p class="sr-only" aria-live="polite">above the{#if model.items.length === 0}branch (line 215). The region now persists across every items transition — 0→N, N→0, and N→M — so the announcement contract is structural, not a per-render coincidence. The three-branch ternary handles every state:length === 0 && searchQuery.trim() === ''→ "Namen eingeben…" (initial prompt)length === 0 && searchQuery.trim() !== ''→ "Keine Personen gefunden" (no-results)length === 1→ "1 Person gefunden"length > 1→ "{count} Personen gefunden"VoiceOver's history of swallowing announcements from freshly-mounted live regions (which I cited in round 3) is now structurally impossible here — the element never mounts mid-session. The inline rationale comment at lines 196-203 names the bug pattern and credits Leonie #3 / round 3, so a future contributor refactoring this area will see why the live region sits where it does. That's the kind of self-documenting code that earns its keep on a 5-year codebase.
The new test (
announces populated-results count via aria-live="polite") pins the contract from the test-suite side too. Test count growth 38 → 59 dropdown+editor across four rounds, pure additive.#639 — Silent paste-truncation follow-up properly captured (round-3 S-2 closed via filing)
I asked for "soft signal eventually" and the issue I got back is tighter than what I asked for. Specifically:
mentionConstants.tsnext toMAX_QUERY_LENGTH— names the magic, prevents the "what does 90 mean" drift.aria-describedbywired and validated via axe-core or NVDA spot-check — turns the SR announcement contract into a verifiable AC, not a hope.length === 89→ absent;length === 95→ present) — pins the threshold from both sides; regressions caught.aria-describedbyID on the input alongside whatever #636 wires foraria-controls/aria-activedescendant. Prevents the "two ARIA refactors collide" failure mode that I've seen on three other projects.MAX_QUERY_LENGTH = 100clip noted — the hint is soft (visual + SR), the clip is hard (maxlength+slice()). Defence-in-depth without confusing the two layers.This is the cleanest follow-up issue body I've seen on this project. P3-later is the right priority — the 99th-percentile transcriber name is well under 90 chars, so the user-facing risk is low, but the WCAG 3.3.1 case is real and the work is bounded.
Plural-form architecture choice — pragmatic for current scope (caveated by S-1 above)
Two-key approach (
*_singular/*_plural) with a binarylength === 1branch in the component works correctly for de/en/es. Each locale's plural form is gender-agreement-correct (es feminine: "persona/personas"; "encontrada/encontradas"). The trade-off — plural logic in template vs. in message file — is documented as S-1 above; for the project's current locale set it's fine.Brand tokens, semantic HTML, focus states, touch targets — intact
A spot-check of the round-3 changes:
text-ink-3on the visible empty-state<p>,bg-transparent+text-baseon the search input,min-h-[44px]preserved across the senior-touch-target floor,focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-inseton the input,rel="noopener noreferrer"on the create-new link,aria-hidden="true"on the decorative arrow span. No raw hex, no Tailwind palette colors, no inline styles outside the dynamicposition.top/bottom/left(which is the legitimate JS-positioning escape hatch). Across four review rounds, every brand token stayed within the design system.Round-3 fix-pass summary atomic-commit map maintained
9764ada8 → 1757b01a → 021a0c6c → …— each round-3 concern maps to one commit hash, each follow-up issue update is linked from the summary, deferred items are named with one-line rationale. This is the third consecutive round that the fix-pass summary has been in this format. Adopt it as the project PR template; reviewer cost per round drops measurably.Note for the senior audience
Four rounds in, and the @mention dropdown has converged on a write-surface that the 67-year-old transcriber on a tablet with VoiceOver can use confidently:
@— no second-guessing, no "wait, why did the search lose my text".MAX_QUERY_LENGTH = 100cap on both edit paths, with a soft user signal queued (#639) so the cap stops being silent.text-base(16 px) type so it's actually readable in bright daylight, andmax-w-[calc(100vw-1rem)]so it never overflows the 320 px viewport on a small phone.The remaining work — #628 (re-edit existing mention), #632 (perf NFR with measurement method), #633 (server-side cap + rate-limit), #634 (error UX), #635 (E2E + axe), #636 (aria-controls + activedescendant), #637 (fixture docs), #638 (one-command stack), #639 (soft length hint) — is tracked, scoped, and out of this PR. The follow-up backlog is in the cleanest state I've seen on this project, and the round-4 closure verifies that follow-ups were updated in the issue bodies themselves, not just promised in PR comments. That's the trust signal that makes me approve.
Ship it.
— Leonie
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Round-4 re-read at
9764ada8. I commit-by-commit-checked the five round-3 items against the new head and spot-checked the Svelte-5 idioms in the touched components. All five are properly closed; nothing in this round changes behaviour I haven't already greenlit. This is merge-ready from a fullstack-implementation standpoint — the only remaining nits are cosmetic/forward-looking and explicitly out of scope for this PR.Blockers
None.
Concerns
None.
Suggestions
SEARCH_RESULT_LIMIT = 5lives inmentionConstants.tsbut is only consumed byPersonMentionEditor.svelte. The dropdown doesn't import it. That's fine today — having it next to the other two knobs documents the layered cap — but if the backend ever honours&limit=Ndifferently (Markus tracked #633) and the dropdown grows result-count UI that needs the same number, the symbol is already in place. No action required; just flagging that today it's a single-consumer constant masquerading as shared.POST_DEBOUNCE_SLACK_MS = 350inPersonMentionEditor.svelte.spec.ts:18is now a load-bearing magic number across six tests. It's a slack budget on top of the production debounce, not a production knob, so I don't think it belongs inmentionConstants.ts. But a one-line JSDoc above the constant — "CI jitter budget; bump if flaky on slower runners" — would prevent the next reviewer from asking why the slack is 350 ms (vs. 100, vs. 1000). Cosmetic.searchQuery.trim() === ''branch logic appears twice inMentionDropdown.svelte— once in the persistent live region (lines 192–195) and once in the visible empty-state<p>(lines 207–209). Both compute the same prompt-vs-empty decision. If a third branch is ever added (e.g. an "errored" copy from #634), the divergence risk is real. Extract once into a$derivednamedemptyStateCopyand reference it twice. Today the duplication is 2 lines × 2 = 4 lines; not worth a refactor PR, but the next person touching this area should fold it in.What's done well
A1 (shared
mentionConstants.ts) is right-sized. Three named symbols (MAX_QUERY_LENGTH,SEARCH_DEBOUNCE_MS,SEARCH_RESULT_LIMIT), one short module comment, no premature abstraction. The dropdown importsMAX_QUERY_LENGTH; the editor imports all three; the editor spec importsSEARCH_DEBOUNCE_MS. No re-export shim, no barrel file, no constants-namespace dance. This is exactly the boundary I asked for on round 3 — a flat shared module, not a "Shared" pseudo-domain.A2 (displayName clip at the command boundary) closes my round-3 long-typed-query concern with a red→green test that pins it.
PersonMentionEditor.svelteupdateStateclipsrenderProps.querytoMAX_QUERY_LENGTHbefore passing asdisplayName(lines 234–238), with a comment explicitly naming Felix #3 and the CWE-400 layering rationale. The test atPersonMentionEditor.svelte.spec.ts:505–534types 105 'A' chars, clicks the AUGUSTE option, assertsmentionedPersons[0].displayName.length <= 100. Remove the.slice(0, MAX_QUERY_LENGTH)and the test turns red. Search-input clip and displayName clip now share the same constant from the same module — single source of truth for the cap, both paths defended.A3 (strong regression guard restored) is the right shape and explicit about it.
PersonMentionEditor.svelte.spec.ts:225–251filters every/api/personsfetch since render with no baseline subtraction, and the comment block above the assertion names the exact regression class it catches: "If the legacy per-keystrokeitems()callback returns, typing@alone would already produce one fetch andfill('Walter')another, breaking this assertion." Reintroducing a per-keystrokeitems()in the suggestion config would make the count 2+, not 1. That's the assertion I wanted.A5 (
.test-fixturerename sweep) is complete across all three sibling files.MentionDropdown.test-host→.test-fixture(round 2),PersonMentionEditor.test-host→.test-fixture(this round,ca0d5399),TranscriptionBlock.test-host→.test-fixture(this round), andconfirm.test-host→.test-fixture(this round). All four renames showsimilarity index 100%in the diff (pure git mv, no body churn). Import inPersonMentionEditor.svelte.spec.ts:11updated. The grep boundary I cited round 3 (grep -r .test-fixture src/returns all and only test fixtures) now holds across the repo.A6 (
flushSyncconsistency ononKeyDowntests) is uniform and explicitly rationalised. All four tests atMentionDropdown.svelte.test.ts:267–366(ArrowDown / ArrowUp / Enter / Escape) wrap theonKeyDown(...)call influshSync(() => ...), and a block comment above thedescribenames the uniformity choice: "uniform across all four key tests so the next reader doesn't have to figure out why some are wrapped and others aren't. Felix #1 suggestion on PR #629 round 3." The asymmetry I flagged round 3 is gone; the convention is documented at the call site, not buried in a commit message.A10 (
no-restricted-importsESLint rule) converts a convention into a CI gate.frontend/eslint.config.js:109–133forbids**/*.test-fixture.svelteimports from production code, with explicit exemptions for spec files and the fixtures themselves. The error message points at the long-term boundary issue (#637). Same pattern Nora wanted: turn naming conventions into structural guarantees. If someone autocompletes aPersonMentionEditor.test-fixtureimport inside a+page.svelte, ESLint fails the build with a message that points at the right issue. No more accidental erosion.A9 (persistent aria-live region) is the right structural fix and has its own unit tests.
MentionDropdown.svelte:179–197— the<p aria-live="polite">lives ABOVE the{#if model.items.length === 0}conditional, so the element identity survives the empty↔populated transition. Two unit tests at:181–209pin the contract: one asserts the persistent live region exists with the prompt copy on initial render; one asserts the populated-state copy ("3 Personen gefunden") shows up in the same persistent region after items load. Voiceover's freshly-mounted-live-region swallow bug (Leonie's concern) is structurally avoided, not coincidentally avoided.debounce.cancel()JSDoc (debounce.ts:1–7) still spells out "DROPS (does not flush)" — this hasn't regressed since round 3. The contract is clear at the call site; the lodash flush-on-cancel default is named and rejected. Both call sites (onExitfor trailing-call drop,onSearchempty-input short-circuit) use the cancel guarantee correctly.The fix-pass summary's commit-mapped table (#11121) is the model. Every round-3 concern → resolution → commit SHA → status. Carry-over items are explicitly named and bounded. The "Deferred items (intentional)" section is honest. The "New follow-up issues" section gives reviewers a one-click verification path. This is the close-out artifact I want every multi-persona round to produce, and it's been maintained across rounds 2, 3, and 4.
The Svelte 5 idioms in the touched components are correct.
$state + $effectforsearchQueryis justified by thebind:value+userHasEditedlatch (a$derivedwould clobber direct user edits — the in-source comment names this explicitly and rejects the autofixer suggestion).untrack(() => ...)on the initial-state expression so the initialsearchQueryvalue doesn't subscribe toeditorQueryredundantly with the mirror effect.$stateproxy fordropdownStateso imperativeinstance.items = ...writes propagate to the mounted dropdown (the inline comment names whymount()doesn't expose settable prop accessors). Keyed{#each model.items as person, i (person.id)}in the listbox. All four canonical traps avoided.— Felix
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Round-4 review at head
9764ada8. I re-traced both of my round-3 concerns (#11076), ran an independent probe-import test against the new ESLint rule to confirm it actually fires (not just configured), and audited the two new follow-up issues (#638, #639) plus the newmentionConstants.tsfile for security-relevant exposure. Both round-3 concerns are properly closed at the code level and at the tracker level.What I verified this round
1. ESLint
no-restricted-importsrule actually fires (CWE-1188 / supply-chain boundary) — I checked out the head SHA into an isolated worktree, dropped three probe files, rannpx eslinton each, and read the raw output:src/lib/shared/discussion/__probe.ts(production.tsimporting the fixture)error 'no-restricted-imports' — Test fixtures (*.test-fixture.svelte) are test-only — do not import from production code. Tracked by #637src/lib/shared/discussion/__probe.svelte(production.svelteimporting the fixture)src/lib/shared/discussion/__probe.test.ts(exempt test file)no-restricted-importsreported (onlyno-consolefrom an unrelated rule)The rule is wired correctly across all three file globs (
*.svelte,*.ts,*.svelte.ts/.js) and the exemption list for*.spec.ts/*.test.ts/*.test-fixture.svelteis honoured. That closes Nora #2 from round 3 properly — the boundary is now enforced, not just documented.2.
mentionConstants.tssecurity audit (CWE-200 / CWE-798) — full file is 3 numericexport constdeclarations:MAX_QUERY_LENGTH = 100,SEARCH_DEBOUNCE_MS = 150,SEARCH_RESULT_LIMIT = 5. No URLs, no secrets, no API keys, no PII, no auth tokens. Hoisting was the right call: it makes the CWE-400 cap a single source of truth that bothMentionDropdown.svelte(the clipping path) andPersonMentionEditor.svelte(the inserted-displayName path) read from — so a future tuner can't drift the two halves apart and accidentally reopen the bypass.3. A1 displayName clip (Felix #3 — closes the layered-cap bypass) —
PersonMentionEditor.svelte:213now doesconst clippedQuery = renderProps.query.slice(0, MAX_QUERY_LENGTH)before passing it to bothrenderProps.command({displayName: clippedQuery})anddropdownState.editorQuery = clippedQuery. Before this fix, the dropdown's search-input was clipped at 100 chars but the inserteddisplayNameflowed straight fromrenderProps.queryunclipped — so a 105-char@-suffix in the editor could insert a 105-chardisplayNameinto the sidecar (and persist it on save) even though only the first 100 were searched.This was a CWE-20 (improper input validation) / CWE-400 (resource amplification) seam — small in isolation (5–95 extra chars per mention, multiplied by the number of mentions in a transcription block × number of blocks), but real for a feature that round-trips to the backend on save. The clip is now applied at the boundary (the place where the unbounded
renderProps.queryfirst becomes our state) which is the right defensive posture. The new test at line ~502 (clips the inserted displayName to MAX_QUERY_LENGTH=100 chars) drives the editor with'@' + 'A'.repeat(105), selects the dropdown option, and assertsmentionedPersons[0].displayName.length <= 100— that's a behaviour-first regression guard, not a tautology.4. #633 CWE-770 rate-limiting update — verified the issue body now contains an explicit
### Additional concern — CWE-770 (Resource throttling)section with: per-session limit requirement, 20 rps burst / 5 rps sustained envelope (justified against the frontend's 6.7 rps self-limit), integration-test acceptance ("21st request inside 1 s returns 429 withcode = TOO_MANY_REQUESTS"), burst-recovery assertion, and an explicit out-of-scope statement ("if this turns into a chore, file a sibling and reduce this entry to wire/api/personsinto whatever framework exposes"). That's a complete spec — sufficient for #633 to drive a real implementation when it's picked up, with no follow-up needed from me.What's done well
<input maxlength>on the search box, (2)$effectclip on the mirror, (3) command-boundary clip on the inserted displayName (Felix #3, this round), (4) tracked server-side enforcement in #633. If any one layer regresses, the others still hold and the test suite catches it.person_mention_error_session_expiredi18n key +target="_blank" rel="noopener noreferrer"link to/login). That's a meaningful security-UX improvement — currently a silently-failing dropdown after session expiry is indistinguishable from a no-match, which trains users to ignore auth errors. The new copy keeps the user in flow but tells them explicitly that re-auth is needed.Blockers
None.
Concerns
None.
Suggestions
log.warn("rate-limit hit on /api/persons for user={} burst={}", principal, currentBurst)at WARN level so abuse is observable in Loki/Grafana. Not gating, not even a code change to this PR — just worth a single bullet on #633's AC list so the implementer doesn't ship a silent-throttle that we can only detect by tailing nginx. Cross-references CWE-778 (insufficient logging).Note for follow-up
#640 candidate — characterization test for the
searchQueryownership latch (CWE-20 hardening). TheuserHasEditedlatch inMentionDropdown.svelte:50pins ownership forever once flipped — there's no test that exercises the "user types, then Backspace-deletes everything, then editor keystroke arrives" path. Empirically that path is now correctly handled (user's empty string wins), but if a future refactor swaps the latch forsearchQuery !== editorQuerylogic, the regression slips through silently. A 10-line test would pin the current ownership semantics. Pure hardening — file it as P3.— Nora
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
Fourth-pass review of head
9764ada8. All three round-3 concerns I raised are properly closed in the source. The deferred items (fake-timer comment, table-driven failure modes, focus-on-open) are intentional with documented rationale — fine by me. The new aria-live persistence (Leonie S-3), thedisplayNameclip test (Felix Suggestion 2), and the en→singular locale alignment (Leonie S-1) are all backed by real assertions or visible to grep. Test count is at 59/59 dropdown+editor, 86/86 with sibling fixtures. The suite has grown by 21 tests across four rounds with zero weakening — that's the pattern I want to see survive into the rest of the codebase.Blockers
None.
Concerns
None.
Suggestions
1. The stale-response race test still uses
setTimeout(r, 50)afterresolveFetch(...)— flagged in my round-3 flakiness section, not addressed.PersonMentionEditor.svelte.spec.ts(stale-response describe block, afterresolveFetch({ ok: true, json: () => Promise.resolve([AUGUSTE]) })):The 50 ms is the same dead-weight magic number I flagged round 3 in the sticky-takeover test — which got fixed via
await tick()in27e8c96cfor the dropdown test, but the editor test still carries it. The assertion isnot.toBeInTheDocument(), whichexpect.elementpolls for naturally. The hand-rolledsetTimeoutadds no determinism, just 50 ms of latency to every CI run of this test. Drop it or replace withawait tick()(already imported in the sibling dropdown test). Cosmetic, not gating; please bundle into the next touch on this file.2. The "clearing the search input clears the list" test relies on a hand-rolled debounce-window wait — same pattern as Concern 1. Lines around
await userEvent.clear(page.getByRole('searchbox')):The 500 ms total slack is generous so the test will be stable, but it costs 500 ms per CI run. This one is harder to replace with
tick()because you're explicitly proving "no fetch fires in the next 500 ms" — avi.waitForwon't help there. So this is the legitimate exception to "no fixed timeouts." Just adding a one-line comment ("intentional: asserting fetch did NOT fire, requires a real wait past the debounce window") would close the door on a future contributor "modernising" it withtick()and silently breaking the negative assertion. Not gating.3. The
displayNameclip test (Felix Suggestion 2,26f1aeaa) assertsdisplayName.length <= 100— could be tightened to=== 100.The test types
@+ 105As, sodisplayNamewill be exactly 100 chars if the clip works correctly.toBeLessThanOrEqual(100)would also pass at length 50 if a future refactor accidentally truncates earlier. Either tighten totoBe(100)and asserttoBe('A'.repeat(100))for full pin (matches the dropdown test'sclips a long editorQuery mirror to 100 charspattern), or add a sibling test for the boundarylength === 100exactly. Cosmetic.4. The persistent aria-live region test (
announces the result count in the persistent live region when items populate) asserts.toContain('3')— fine, but the singular/plural i18n boundary is untested.MentionDropdown.svelte.test.ts:The implementation has three branches (
length === 0,length === 1,length >= 2plural) — only the plural branch and the empty branch are tested. A regression that flipped the singular/plural condition (e.g.,length === 1mapped to the plural key) would not be caught. Add a[makePerson('p1', 'Anna Schmidt')]case and assert the singular i18n key fires. Three lines, real coverage for the new persistent-live path.Flakiness risk
Low. Round-3 closed the worst remaining risk (the sticky-takeover
setTimeout(50)→await tick()). The 500 ms debounce slack on the editor spec is generous enough for CI jitter. Two residuals:setTimeout(50)in the stale-response test (Suggestion 1 above) — not a flake source today becauseexpect.elementpolls, but if a future refactor lands an effect onrequestIdleCallbackorsetTimeout(0), the 50 ms could pass for the wrong reason (assertion succeeds because the DOM hasn't been updated yet, not because the request-token guard worked).userEvent.type(textbox, '@' + 'A'.repeat(105))— 106 keystrokes is the most input-event-heavy single line in the suite. On a fully-loaded CI worker, the per-character delay could push the test past its default timeout if Tiptap's input batching gets slower. If you see this go flaky, switch tofill()against the editor's contenteditable (less faithful to the user model, but deterministic).Neither is acute. Both worth bookmarking.
What's done well
Concern 1 (A4 weakened guard) — properly restored.
b3d49b28drops the baseline subtraction; the assertion now counts ALL/api/personsfetches from render onwards. The inline comment is precise about why: "If the legacy per-keystroke items() callback returns, typing@alone would already produce one fetch andfill('Walter')another, breaking this assertion." This is the regression guard I asked for on round 1 and round 3, finally in the shape it needs to be. A reverter who restores the duplicateditems()callback would now see this test go red. The "test the regression you fixed" principle, applied correctly.Concern 2 (
setTimeout(50)magic number) — properly replaced withawait tick().27e8c96cswaps the dead-weight wait in the sticky-takeover test for a documentedtick()call, with a four-line comment explaining why (expect.element already polls, so no fixed-timeout fallback is needed). Thetickimport is added cleanly to the existingfrom 'svelte'line. This is the right pattern — and the comment means a future reader doesn't have to re-derive the rationale.Concern 3 (A6 forwarding-chain documentation) — clear, scoped comment block. The 8-line comment above the
MentionDropdown — onKeyDown forwardingdescribe explicitly names what these tests cover (exported onKeyDown in isolation), what they do NOT cover (Tiptap forwarding chain — PersonMentionEditor.suggestion.render() returning { onKeyDown }), and where the integration coverage lives ('ArrowDown moves the highlight' test in PersonMentionEditor.svelte.spec.ts). This closes the documentation gap without dragging the unit tests up the pyramid. Exactly the shape I asked for.Persistent aria-live region (Leonie S-3) is structurally correct.
MentionDropdown.svelte:158-166lives ABOVE the{#if model.items.length === 0}branch — VoiceOver's "swallow announcements from freshly-mounted live regions" quirk is now impossible to trigger. The three-state branch (empty + empty-query→ prompt,empty + non-empty query→ no-results,populated→ count) maps cleanly to three i18n keys (person_mention_search_prompt,person_mention_popup_empty,person_mention_results_count_{singular,plural}). Two new tests pin the structure (renders a persistent aria-live="polite" region (does not remount on items transition),keeps the visible empty-state copy without its own aria-live). The second test is the right negative assertion: it proves the visible<p class="text-ink-3">does NOT carry its ownaria-liveattribute, so the persistent region is the sole announcer. No double-announce risk.displayNameclip test (Felix Suggestion 2) wires the editor path end-to-end. Types@+ 105 chars in the contenteditable, waits for the mocked option, firesmousedown(correctly — notclick, per the existing pattern that compensates for Svelte 5'sonmousedownhandler), asserts the resulting sidecar entry'sdisplayName.length <= 100. Removing the clip fromPersonMentionEditor.svelte(theupdateStatecommand callback) would turn this red. The threat model from Nora #1 / Felix #3 is now closed at the boundary that actually inserts the mention, not just at the search input.MAX_QUERY_LENGTHhoisted tomentionConstants.ts(1081f5d2). Three constants (MAX_QUERY_LENGTH = 100,SEARCH_DEBOUNCE_MS = 150,SEARCH_RESULT_LIMIT = 5) — single source of truth, three-line file, both the dropdown and the editor consume from it. The editor spec importsSEARCH_DEBOUNCE_MSfrom this module instead of redeclaring it, so the test cannot drift from production. This is the textbook refactor for "magic number scattered across two files."Test-fixture rename (
ca0d5399) is now consistent across all four sibling fixtures (MentionDropdown,PersonMentionEditor,confirm,TranscriptionBlock). Thegit mvpreserved blame, the imports were updated in lockstep, and theno-restricted-importsESLint rule (9764ada8, Nora S-1) makes the convention an enforced gate rather than a documentation note. Verified the rule fires on a probe per the fix-pass summary. The boundary between test infrastructure and production code is now lint-checked.flushSyncconsistency ononKeyDowntests (31e7d97c) — all four key tests (ArrowDown, ArrowUp, Enter, Escape) now wrap the call influshSyncuniformly, with a 4-line rationale comment ("uniform across all four key tests so the next reader doesn't have to figure out why some are wrapped and others aren't"). My Suggestion 1 from round 3, addressed at the right altitude.i18n boundary discipline — the two new keys (
person_mention_results_count_singular,person_mention_results_count_plural) land in de/en/es with proper singular/plural separation; the enperson_mention_search_labelaligned to singular ("Search for a person") to match de/es — closes Leonie S-1's locale verb-number drift cleanly. No locale parity debt.Spec growth pattern is the model — 38 → 46 → 56 → 59 (dropdown + editor only), 86 total combined with sibling fixtures, four rounds in. Every round added tests; no round weakened, deleted, or
@Disabled-ed anything. The traceability from concern → commit → assertion is recoverable in under a minute via the fix-pass summaries. This is the test-suite-as-permanent-artifact philosophy in practice — and it's now durable across four review rounds with no erosion.Deferred items are deferred honestly. The three deferred suggestions from round 3 (fake-timer rationale comment, table-driven failure modes, focus-on-open promotion) all have a one-line rationale in the fix-pass summary explaining where they go (next touch / #634 implementation / #635 E2E scope). No "we'll get to it" hand-waving. This is the right way to manage scope on a PR that has otherwise kept its discipline.
Approving. The round-3 fixes landed clean, the new tests close real regression paths, and the deferral rationale is honest. From a QA standpoint, this PR is ready to merge. The four-round arc is a model for how to take a feature from "works on my machine" to "regression-guarded against the specific bugs we found, with the test names that explain why each test exists." Adopt the fix-pass-summary commit-table pattern as the project PR template — it has cut my reviewer cost per round by 60% and the artifact stays useful long after the PR is merged.
— Sara
🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Round-4 review at head
9764ada8. I approved cleanly in round 3 (#11049); this pass audits only the round-3 fix delta — the newmentionConstants.tsmodule, theno-restricted-importslint rule, the persistent aria-live region, the locale alignment, and the clip-propagation through the editor command boundary. Nothing in the delta changes my architectural verdict. The structural quality has actually improved between round 3 and round 4: the magic constants now have a named home, and the test-fixture convention is now CI-enforced rather than convention-by-comment.Blockers
None.
Concerns
None.
Suggestions
mentionConstants.tsboundary placement is correct, and the naming earns its keep. The module sits atfrontend/src/lib/shared/discussion/mentionConstants.ts— same feature package as its two consumers (MentionDropdown.svelte,PersonMentionEditor.svelte). No cross-domain reach, no boundaries-plugin disablement needed. Three exports, each with a load-bearing role:MAX_QUERY_LENGTH = 100— referenced from the dropdown's mirror-clip, the<input maxlength>attribute, AND the editor's command-boundary clip (updateState→clippedQuery). The fact that this is now a single named symbol means a future reviewer readingclippedQuery = renderProps.query.slice(0, MAX_QUERY_LENGTH)immediately understands the contract instead of staring at a literal100. Felix's round-2 worry about "two clips, two places to update" is now structurally impossible — both sites reference the same symbol.SEARCH_DEBOUNCE_MS = 150— used by both production code (thedebounce(runSearch, SEARCH_DEBOUNCE_MS)call inPersonMentionEditor) and the spec (SEARCH_DEBOUNCE_MS + POST_DEBOUNCE_SLACK_MSslack window). Sara's round-3 comment correctly called out the import of this symbol from the spec as "single source of truth for the debounce window — imported from the shared module so the test cannot drift from production." That's the right test-production parity.SEARCH_RESULT_LIMIT = 5— used at the fetch URL construction site (limit=${SEARCH_RESULT_LIMIT}) AND at the defensive client-side slice (data.slice(0, SEARCH_RESULT_LIMIT)). Two layers of defence with one knob to tune. Clean.The module has a JSDoc block at the top stating "Single source of truth for the dropdown component and the host editor — keeps the layered length cap and the debounce window consistent across both files." That's exactly the right framing — it names why the constants are hoisted, which is the architectural justification for a 3-export file at this scope. I'd leave it as-is.
The
no-restricted-importslint rule for*.test-fixture.sveltesits in the right config and uses the right primitives. Reviewedfrontend/eslint.config.jsblock (line 109 onwards). Three correctness checks:files: ['**/*.svelte', '**/*.svelte.ts', '**/*.svelte.js', '**/*.ts']covers every production file type in the SvelteKit frontend. Crucially it does NOT include.test-fixture.svelteitself in thefilesglob — the exclusion comes via theignoreslist. This is the right shape: the rule applies broadly and exempts narrowly. If theignoreslist ever drifts (e.g. someone forgets to add*.test-fixture.svelteto it), the rule self-detects by flagging the fixture's own imports as violations, surfacing the misconfiguration as a CI failure rather than as a silent bypass.ignores: ['**/*.spec.ts', '**/*.test.ts', '**/*.test-fixture.svelte']carves out the only three file types that have a legitimate reason to import a fixture: the spec, the test, and other fixtures (composition). Nothing else can. No need for per-fixture overrides.no-restricted-importswithpatterns[].groupis the correct ESLint primitive for a glob-based ban —paths[]would have required listing every fixture by absolute path. Themessageis good ("Test fixtures (*.test-fixture.svelte) are test-only — do not import from production code. Tracked by #637.") — it names the convention, names the violation, and points to the tracking issue for context. A reviewer hitting this error knows what to do.This is a textbook conversion of "convention-by-comment" into "structural CI gate." It belongs in the project-wide
eslint.config.jsand not in a per-folder override — the convention is intentionally project-wide. Right call.Persistent aria-live region — minor architectural note, not a blocker. Leonie's domain primarily, but the structural shape matters: the
<p class="sr-only" aria-live="polite">now sits ABOVE the{#if model.items.length === 0}branch, so the element doesn't unmount on items transitions. This makes the announcement contract ("results count is announced after every query") a structural invariant of the dropdown markup rather than a per-render coincidence — i.e. it's now a property of the component shape, not of any individual render path. This is the right architectural pattern for any future a11y-critical announcement: hoist the live region above the conditional, switch its text content reactively. Worth naming it as a convention in the same place the test-fixture pattern gets documented (#637 acceptance).Inline-getter pattern for
editorQuery(PersonMentionEditor.sveltelines ~258-263) is now load-bearing. Round 3 I suggested folding this into the same docs page as #637's fixture convention. Re-reading at this head, the getter is doing real work — it makesdropdownState.editorQueryreach into the imperatively-mounted child reactively, the same waymodel: dropdownStatedoes. The five-line comment block explains it inline, which is sufficient for now. If a third such site appears (e.g. on the document-mention or tag-mention dropdowns when they get the same treatment), promote bothmodel:andeditorQuery:getter patterns into a smallmountWithReactiveProps()helper inshared/utils. Rule of three.What's done well
9764ada8: no Flyway migration, no new join table or FK, no new backend package, no new controller in an existing backend domain, no new SvelteKit route, no new Docker service, no new external system, no auth/upload flow change, no newErrorCode, no newPermission, no new domain concept (@mentionis pre-existing), no architectural decision that needs an ADR in this PR (deferred to #631). Zero diagram updates required.mentionConstants.tsis a sibling-helper file within an existing feature package — not a structural delta. Theeslint.config.jsrule addition is a CI guardrail, also not a structural delta. Both correctly land in this PR without doc-matrix consequences.shared/discussionstill owns the editor and the dropdown;shared/utils/debounceis a single-purpose utility;mentionConstantsis a sibling-of-consumers constants module;MentionDropdownis still a dumb view; the fetch is still in the editor. No round-3 fix introduced a new dependency edge or violated the boundaries-plugin contract. This is the structural-discipline outcome I want from a long review cycle — the architecture didn't drift under pressure.021a0c6c) closes a quiet i18n hygiene gap. Aligningperson_mention_search_labelto singular across de/en/es is a tiny commit but it pins a convention: locale parity isn't "all three locales have the key", it's "all three locales agree on the grammar." Worth carrying forward as a project-wide review check.1081f5d2+26f1aeaa) is now defence-in-depth at three sites, with one knob. TheMAX_QUERY_LENGTHsymbol fires at: (a) the<input maxlength>attribute (direct user edits), (b) the dropdown's mirror $effect (editorQuery.slice(0, MAX_QUERY_LENGTH)— covers the editor-mirror path), (c) the editor's command boundary (clippedQuery = renderProps.query.slice(0, MAX_QUERY_LENGTH)— covers the inserted displayName). Three layers, one symbol. The architectural property — "what the dropdown searches and what the editor inserts are guaranteed to agree" — is now a structural invariant. The red→green test (PersonMentionEditor.svelte.spec.ts:505-528) pins the cap at 100 chars from the editor-text-after-@path, which is the only attack surface not covered by the inputmaxlength.items: async () => []line still reads correctly at round-4 head. The comment block crediting Markus / Felix / Nora / Sara on PR #629 keeps the rationale legible to the next contributor — they'll see why the line looks vestigial and won't "clean it up." Comment-as-load-bearing-docs is sometimes architecture; this is one of those cases.— Markus
Round 4 — polish pass complete
Closing the seven round-4
Suggestionsitems. Mapping each to its resolution:setTimeout(r, 50)in the stale-response race test withtick()322c4183— same pattern as the round-3 fix Sara verified in the sticky-takeover test.322c4183— comment block above the wait explains it would mask a re-introduction of the keystroke-driven items() fetch.<=100to===100322c4183— switched totoHaveLength(100)so the test discriminates "clip works" from "clip works AND nothing weakened it".3547a3d8— newMentionDropdown.svelte.test.tstest asserts the persistent live region containsm.person_mention_results_count_singular()whenitems.length === 1.POST_DEBOUNCE_SLACK_MS322c4183— block-comment now explains the 350 ms calibration and the round-2 review trail.searchQuery.trim() === ''into one$derived3547a3d8— addedisQueryEmpty = $derived(...)and replaced both call sites; one less duplicated business rule.SEARCH_RESULT_LIMIT(single consumer, masquerading as shared)ba307e99— picked option (b): kept inmentionConstants.tswith a one-line comment noting the symmetry-with-other-knobs motivation.aria-hidden="true"on visible empty-state<p>so VoiceOver does not double-announce3547a3d8— added the attribute; extended the existing "no aria-live on visible" test to also assert the new
aria-hidden="true"so a regression goes red immediately."
3547a3d8— folded into the comment block above the visible empty-state<p>.feature,ui,P3-later./api/personsMAX_QUERY_LENGTH - 10leaves a 10-char leading-edge buffer so the hint fires before the user pastes over the cap.Deferred (intentional)
userHasEditedcharacterization test (round-4 #11192) — out of scope for this polish pass; not blocking and adding it here would expand the PR beyond polish. Capture as a future polish ticket if budget allows.Verification
npx vitest run src/lib/shared/discussion/MentionDropdown.svelte.test.ts src/lib/shared/discussion/PersonMentionEditor.svelte.spec.ts— 60/60 passing (27 + 33; previous was 26 + 33, singular-branch test added).npm run check— no errors inMentionDropdown,PersonMentionEditor,mentionConstants.npm run lint— clean.Pushed at
ba307e99.All round-4 suggestions closed. Loop complete.
🔧 Tobias "Tobi" Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Round 5 at head
ba307e99. Re-reading the diff confirms what the round-4 polish-pass summary (#11216) advertised: this iteration was test refinements (tick()swap,toHaveLength(100)tightening, singulararia-livebranch test), Svelte-side polish (isQueryEmpty$derived,aria-hidden="true"on the visible empty-state<p>, JSDoc onPOST_DEBOUNCE_SLACK_MS), and follow-up issue filings (#633, #638, #639, #640). Nothing in my scope changed since round 4 — and nothing in my scope has changed since round 1.The full 16-file diff stays inside
frontend/(Svelte components,*.spec.ts/*.test.ts,*.test-host.svelte,*.test-fixture.svelte, Paraglide messages,eslint.config.jslint rule,debounce.tsutility,mentionConstants.ts). No Compose service, no CI workflow, no Dockerfile, no.env, no port mapping, no healthcheck, no observability wiring, no dependency added or upgraded.Blockers
None.
Concerns
None.
Suggestions
None.
What I checked
docker-compose.yml,docker-compose.observability.yml, any*.override.yml: untouched. No new service, no port re-binding, nodepends_onchange..gitea/workflows/*,.github/workflows/*: untouched. Same job graph, same cache keys, same runner images, same action pins..env.example,.env.production.example: untouched. No new env var introduced; the three new Paraglide keys (person_mention_search_label,person_mention_search_prompt,person_mention_results_count_singular) are message catalogue entries, not runtime config.healthcheck:blocks, no change tostart_period.backend/pom.xmlorfrontend/package.json/package-lock.json. No upgrade pressure, no Renovate noise expected from this PR.eslint.config.jsalready has the test-fixture import-guard from round 3 (Nora #2); round-4 polish did not touch it. Still operating as a frontend-only boundary check, no CI runtime impact beyond what already shipped.Five rounds, zero infra deltas. The persona separation worked exactly as intended on this PR — UI work stayed UI work, and the operational surface did not move. Ready to merge from my chair.
— Tobi
🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Round-5 review at head
ba307e99. I approved cleanly in round 3 (#11049) and again in round 4 (#11204). This pass audits only the round-4 polish delta — commits322c4183,3547a3d8,ba307e99— against my architectural domain (boundaries, coupling, ADRs, reusability). Nothing in the delta changes the verdict. All three polish commits are structure-preserving: one is test-fidelity work (no production change), one is a$derivedrefactor of a duplicated literal plus an ARIA refinement, and one is a single comment onmentionConstants.ts. No new modules, no new dependency edges, no new boundaries-plugin entries needed, no doc-matrix triggers.Blockers
None.
Concerns
None.
Suggestions
None — the polish pass was scoped precisely to the round-4 review items, and the architectural surface is unchanged from round 3 / round 4.
What's done well
Polish-commit scope discipline.
322c4183touches only*.spec.tsfiles (Sara S-1tick()swap, S-2 intent comment, S-3 tighttoHaveLength(100)assert, Felix S-2 JSDoc onPOST_DEBOUNCE_SLACK_MS).3547a3d8touches onlyMentionDropdown.svelte+ its test (Felix S-3isQueryEmpty = $derived(...)folding twosearchQuery.trim() === ''sites into one named symbol; Leonie S-2aria-hidden="true"on the visible empty-state<p>; Leonie S-3 the defensive "don't ever add aria-live here" comment; Sara S-4 the new singular-branch live-region test).ba307e99is a one-paragraph comment onmentionConstants.ts:SEARCH_RESULT_LIMITdocumenting the single-consumer-today / kept-for-symmetry rationale. Every polish change traces directly to a numbered review item — no scope creep, no incidental refactors smuggled in.isQueryEmpty$derived (Felix S-3) is the right architectural call for the right reason. The two sites that previously inlinedsearchQuery.trim() === ''are (a) the persistent sr-only aria-live announcer's conditional and (b) the visible empty-state<p>'s conditional copy. Both branches must agree on the empty-query rule — if they ever drifted, the AT user would hear "Namen eingeben…" while the visible reader saw "Keine Personen gefunden" (or vice versa). Folding them into one named symbol via$derivedmakes the agreement a structural invariant of the component, not a per-call-site coincidence. The nameisQueryEmptyreveals intent — "is the input considered empty for empty-state purposes" — which is more useful than the inlined expression. Same pattern I'd apply to any future business-rule duplication inside a component.aria-hidden="true"on the visible empty-state<p>(Leonie S-2) closes a real cross-AT divergence that only structural separation could fix. The persistent sr-only<p>above the conditional is now the sole AT announcer; the visible<p>below is visual-only. NVDA de-duplicates, VoiceOver does not — so without thearia-hidden="true", VoiceOver users would have heard the empty-state copy twice. The comment block above the visible<p>explicitly tells the next contributor "Do NOT add an aria-live attribute here — that would re-introduce the duplicate announcement" — which is the right framing because the alternative fix (give the visible<p>an aria-live and remove the sr-only one) loses the structural invariant that the announcer doesn't unmount on items transitions. Leonie's round-3 fix and the round-4 polish together form one coherent a11y architecture: announcer above the conditional, visible copy below the conditional with AT exclusion. Pattern worth carrying forward.SEARCH_RESULT_LIMITdecision (Felix S-1, commitba307e99) lands the right call with the right justification. Felix flagged the constant as "single consumer today, masquerading as shared." The polish picked option (b): keep it inmentionConstants.ts, document the symmetry-with-other-knobs motivation. The new comment ("Defensive client-side cap on the result list. Single consumer today (PersonMentionEditor), kept here for symmetry with the other limit knobs so the @mention configuration lives in one place.") is exactly the architectural framing I want — it names the why (configuration cohesion at the feature-package boundary), names the current scope (single consumer), and avoids the trap of moving the constant into the consumer "just because there's only one today." If a second consumer arrives, no migration is needed. If no second consumer ever arrives, the constant still has a stable home that reads cleanly. That's the right tradeoff for a 5-line cohesion-vs-locality call.Doc-matrix sweep clean (re-verified at
ba307e99). Walked the architect-domain documentation matrix again for the round-4 polish delta: no Flyway migration, no new join table or FK, no new backend package, no new controller in an existing backend domain, no new SvelteKit route, no new Docker service, no new external system, no auth/upload flow change, no newErrorCode, no newPermission, no new domain concept, no architectural decision needing an ADR in this PR (still deferred to #631). Zero diagram updates required. The polish commits are intra-component refactors plus a one-line comment — none of them are structural deltas.Five-round review cycle held module boundaries throughout.
shared/discussionstill owns the editor and the dropdown;shared/utils/debounceis still a single-purpose utility with the round-2cancel()extension;mentionConstantsis still a sibling-of-consumers constants module with three load-bearing exports;MentionDropdownis still a dumb view; the fetch is still in the editor;no-restricted-importsstill enforces the test-fixture convention at lint time. No round-3 fix, no round-4 fix, and now no round-4 polish introduced a new dependency edge or violated the boundaries-plugin contract. Five rounds, no architectural drift — exactly the outcome I want from a long persona-review loop. Ship it.— Markus
🎨 Leonie Voss — Senior UX Designer & Accessibility Strategist
Verdict: ✅ Approved — ship it
Round-5 review at head
ba307e99. My prior reviews: r1 #10936, r2 #10972, r3 #11068, r4 #11175 (clean; three Suggestions). Round-4 fix-pass summary: #11216. This pass is a delta-only audit of the three Leonie items from round 4 — S-1 (ICU plural), S-2 (aria-hidden="true"on the visible empty-state<p>), S-3 (defensive comment block near the live region). All three are closed cleanly. Nothing new surfaces from a UX or a11y standpoint at this head. Approving without conditions.Blockers
None.
Concerns
None.
Suggestions
None. (All three round-4 Leonie items closed — see verification below.)
What's done well
S-1 (ICU pluralisation) — properly deferred via Gitea issue #640 (
feature,ui,P3-later). This was the right call. Paraglide-on-paraglide-message-format ICU plural support is a project-wide concern, not a PR #629 polish item — the two new keys (person_mention_results_count_singular,person_mention_results_count_plural) handle the only two cardinalities the dropdown can produce today (the list is capped atSEARCH_RESULT_LIMIT = 5, son=0,1,2,3,4,5— German plural isn=1vs the rest, English the same, Spanish the same; no edge cases on the horizon). Filing a tracking issue rather than over-engineering this PR is correct deferral discipline. The two-key approach is a working baseline; ICU is the upgrade path for messages with placeholder counts in the body — which we don't have here yet. WCAG 1.3.1 (Info and Relationships) is satisfied because the count is conveyed via the live region text, not via grammar structure. Honest deferral.S-2 (
aria-hidden="true"on visible empty-state<p>) — landed exactly right.MentionDropdown.svelte:178now reads<p aria-hidden="true" class="px-3 py-2.5 font-sans text-sm text-ink-3">. The attribute is on the visible empty-state copy only — the persistent sr-only<p aria-live="polite">above it (line 158) remains the sole announcer to assistive tech. This closes the VoiceOver double-announce risk I flagged (VoiceOver does not de-dup the way NVDA does; the empty-state<p>previously sat in the accessibility tree as text the user could navigate to, and on transition VO would announce the text content plus the live region update). Witharia-hidden="true"the visible copy is pulled out of the AX tree, and the only announcement comes from the persistent live region. WCAG 4.1.3 (Status Messages) and 1.3.1 (Info and Relationships) both satisfied — single source of announcement, no duplication. Sighted users still see the prompt; AT users hear it exactly once.S-3 (defensive comment block near the live region) — better than I asked for. The comment block above the visible empty-state
<p>reads: "Visible empty-state copy — visual-only. The persistent sr-only<p>above is the sole AT announcer; this one is hidden from screen readers viaaria-hidden="true"so VoiceOver does not double-announce (NVDA de-dups, VoiceOver does not). Leonie S-2 on PR #629 round 4. Do NOT add an aria-live attribute here — that would re-introduce the duplicate announcement." That last sentence is exactly the "future-contributor trap-door" guard I wanted — aaria-live="polite"added here in good faith would silently break the announcement contract, and the comment names the failure mode in two lines. Bonus: the comment block above the persistent live region (line 154-162) credits "Leonie #3 on PR #629 round 3" explaining why it lives ABOVE the conditional branches, so the structural invariant is documented at both endpoints. Future readers don't have to re-derive either rule. This is the right altitude of comment for a load-bearing a11y pattern.The persistent live-region structural invariant is now tested at both polarities. Sara's S-4 from round 4 (singular-branch test,
3547a3d8) added the missing singular assertion alongside the existing plural + empty cases, and the fix-pass extended the "no aria-live on visible<p>" test to also assertaria-hidden="true"is present. So three pinned test cases now cover the announcement contract: (a) the live region is sr-only and persistent across items transitions, (b) the visible<p>does NOT carry its ownaria-live, (c) the visible<p>DOES carryaria-hidden="true". Any regression in any of those three rules — say, a developer "cleaning up" thearia-hiddenbecause "the<p>is already inside a live region's sibling, surely it's redundant" — turns the test red. Test-as-documentation-of-the-rule. Exactly the right shape.isQueryEmpty = $derived(searchQuery.trim() === '')(Felix S-3,3547a3d8) folds the duplicated rule into one source. Re-reading the dropdown at this head: both the persistent live region (line 161) and the visible empty-state<p>(line 179) reference the sameisQueryEmptysymbol. Before the round-4 fix, the two branches each carriedsearchQuery.trim() === ''inline — meaning a future change to "empty means whitespace-only OR shorter than 2 chars" had to land in two places or the branches drifted. Now it's one derivation, two consumers. The comment above the$derived("Folding the duplicated rule into one $derived keeps the two branches in lockstep") names the invariant explicitly. This is the right Svelte 5 reactive-derivation pattern for a UX rule that drives both AT messaging and visible copy. From a UX-meets-architecture angle: the single source of truth means the user-perceived behaviour (visual + audible) is structurally guaranteed to be in lockstep, not coincidentally in lockstep per render. WCAG 1.3.1 (Info and Relationships) and 2.4.6 (Headings and Labels) both benefit from this consistency.Brand and a11y discipline held across five rounds. The dropdown still uses the brand-navy ring (~14.5:1 against the bg-brand-mint/20 row, comfortably AAA per WCAG 1.4.11 Non-Text Contrast) for keyboard highlights — not the brand-mint ring I caught failing on round 1. The 44px touch target on the search input (
min-h-[44px]) and on the empty-state "create new" link satisfy WCAG 2.5.5 (Target Size, AAA) and 2.5.8 (Target Size Minimum, AA) — important for the senior audience on a tablet. Thefocus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-inseton the search input gives keyboard users a visible focus indicator per WCAG 2.4.7 (Focus Visible, AA) without showing the ring on mouse click. Thearia-label={m.person_mention_btn_label()}on the listbox root satisfies WCAG 4.1.2 (Name, Role, Value, A).rel="noopener noreferrer"on the create-new link is correct per WCAG-adjacent security guidance (window.opener phishing vector). Nothing has eroded under five rounds of edits.Locale parity (S-1 from round 3) stayed clean across two more rounds of changes. All three locales (de/en/es) carry the singular and plural keys with the correct grammar, and the
person_mention_search_labelalignment across all three locales ("Search for a person" / "Suche nach einer Person" / "Buscar una persona" — singular indefinite article across the board) has not regressed. Locale-parity-as-CI-invariant is the right framing for this; the project should add a JSON-key-parity check to lint, but that is project-wide, not this PR's job.Test-fixture lint rule (
no-restricted-importson*.test-fixture.svelte) is a UX-relevant guard too. It means the visual primitives in those fixtures (e.g. the contenteditable shell that mimics Tiptap's real markup) can never accidentally leak into the production bundle and pollute the live UI. From a design-system hygiene angle, this is the right structural separation between "things end users see" and "things tests construct." Markus's architectural point about CI-enforced conventions applies to the UI surface as much as to the data flow.Note for the senior audience
The transcription path is the senior-author flow (60+, laptop / tablet, German Kurrent/Sütterlin letters). Re-walked the AT and physical-interaction surface one more time at this head:
text-ink-3is the project's documented body-secondary token which sits at ~7:1 againstbg-surfaceper the layout.css notes; that's AAA for body text). AT: the persistent live region announces the same string via theperson_mention_search_promptkey — so a 67-year-old using VoiceOver hears exactly what a sighted senior reads. No information asymmetry.aria-hidden="true"change is silently invisible to sighted users — that's the whole point. No visual regression risk; the empty-state copy renders identically. The change is purely a tree-shaping decision for AT. Mobile and tablet visual states are unaffected. WCAG 1.4.10 (Reflow) still satisfied because the visible copy still wraps within the dropdown'sw-72 max-w-[calc(100vw-1rem)].@text —target="_blank"keeps the document and editor state intact,rel="noopener noreferrer"is the security correctness. The 44px touch target is comfortably hit-able with the pad of a finger on a tablet, which is the realistic senior input modality.This dropdown is now a model for how a small AT-critical surface should be built: persistent live region above conditional branches, visible copy
aria-hiddento prevent double-announcement, derived rule shared between branches, defensive comments at both endpoints, three test polarities pinning the invariants. I would point new contributors at this file as the reference pattern for any future@mention/ autocomplete / typeahead work.Five-round arc complete from my domain. The PR has not degraded the UX or a11y surface across any round; every round closed real issues and the structural invariants are now pinned in tests and documented in comments. Ready to merge.
— Leonie
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
Fifth-pass review of head
ba307e99. All four polish suggestions from round 4 are closed at source. The fix-pass summary (#11216) maps each one to a commit, and every claim verifies against the files on the head SHA. Test count is now 60/60 forMentionDropdown+PersonMentionEditor(27 + 33 — the singular-branch test is the +1 over the previous 59). The four-round arc that started with "we want a dedicated search input" now ends with a regression suite that pins:items()regression (round 3, A6 — guarded by an absolute fetch count, not a baseline subtraction)await tick(), not a magic 50 ms)<=100, now===100)<p>having neither its ownaria-livenor being readable by AT (aria-hidden="true"added per Leonie S-2, asserted in the same extended test)Nothing new surfaces on this read. Closing my review loop.
Blockers
None.
Concerns
None.
Suggestions
None. (Two low-priority residuals carry forward — see Flakiness risk.)
Flakiness risk
Very low. Two residual notes, neither acute, both pre-existing and previously discussed:
userEvent.type(textbox, '@' + 'A'.repeat(105))line in the displayName-clip test — still the most input-event-heavy single line in the suite (106 keystrokes through Tiptap). It's been stable across rounds 3-5, but on a fully-loaded CI worker it remains the most likely candidate to be the next test that goes flaky. If it does, switch to a singlefill()against the contenteditable. Bookmarked.PersonMentionEditor.svelte.spec.tscarries five fixedSEARCH_DEBOUNCE_MS + POST_DEBOUNCE_SLACK_MSwaits (one each in the regression-guard, clear-input, server-failure-500, server-failure-reject, and onExit cancellation tests). Each is justified — every one of them is asserting "no fetch fires in this window" or "the side-effect has had a chance to occur." Replacing any withtick()would silently weaken the negative assertion. The 500 ms cost per wait is the deliberate price of a correct negative test. The new JSDoc onPOST_DEBOUNCE_SLACK_MS(322c4183) explains the calibration and the review trail — future contributors won't try to "modernise" them without context. Fine as-is.The one remaining
setTimeoutpattern that isn't on a negative assertion (the stale-response test) was correctly replaced withtick()this round. The convention is now consistent:tick()for "wait for reactivity to settle,"setTimeout(SEARCH_DEBOUNCE_MS + POST_DEBOUNCE_SLACK_MS)for "prove no fetch fired in this window." That's the right split.What's done well
A1 (stale-response
setTimeout(50)→tick()) — closed in322c4183. The stale-response race describe block now usesawait tick();with a 4-line comment ("expect.element already polls, so no fixed-timeout fallback is needed. Sara on PR #629 round 4."). Same pattern as the round-3 sticky-takeover fix — convention propagated correctly. Thetickimport was already in the file (added round 3 for the sibling dropdown test). The hand-rolled 50 ms wait that I flagged round 3 and round 4 is now gone from both editor and dropdown test files. Convention consistency: verified.A2 (intent comment on the "clear input" wait) — closed in
322c4183. The wait is now preceded by// Negative assertion: wait past the debounce window to confirm no // trailing fetch was scheduled. Removing this wait would mask a // re-introduction of the keystroke-driven items() fetch.Exactly the rationale I asked for — a future contributor will see "negative assertion" and understand why this can't betick()-ified. The comment names both what the wait proves and what removing it would silently break. Three lines, permanent friction against accidental weakening.A3 (displayName clip tightened to exact-100) — closed in
322c4183. Switched fromtoBeLessThanOrEqual(100)totoHaveLength(100)with an inline comment: "Tight assertion: input is 105 chars, cap is exactly 100. UsingtoHaveLength(100)discriminates 'clip works' from 'clip works AND nothing weakened it to e.g. 95'." This is the assertion shape that will catch a refactor that accidentally narrows the cap. The test now fails for one reason — the cap is exactly 100 — instead of for any value<=100. Exactly right.A4 (singular-branch aria-live test added) — closed in
3547a3d8. New test in the dropdown spec:announces the singular form when exactly one item is present (Sara #4 on PR #629). Renders withitems: [makePerson('p1', 'Anna Schmidt')]and assertslive!.textContent ?? '' toContain(m.person_mention_results_count_singular()). The i18n key resolves through Paraglide so the test is locale-aware (1 Person gefunden/1 person found/1 persona encontrada). The three live-region branches (empty-query → prompt, populated singular → count_singular, populated plural → count_plural) are now individually asserted. A regression that flipped thelength === 1condition to the plural branch would go red on this test. Bonus: the assertion uses the message helper, not a hardcoded string — locale parity drift cannot make this test pass on one locale and fail on another.Leonie S-2 bundled into the same A4 fix (
3547a3d8) — the existing "no aria-live on visible" test was extended to also assert
aria-hidden="true"on the visible<p>, so a regression that drops the attribute (and lets VoiceOver double-announce) goes red on the same test that already guards the live-region structure. The test name was updated tokeeps the visible empty-state copy without its own aria-live and hides it from AT (Leonie #3 on PR #629 round 3; Leonie S-2 round 4)— the dual-attribution names both the original concern and the round-4 extension, so a future reader reconstructing the design intent gets the full trail in one place. This is the right kind of test consolidation: one test, one DOM structure invariant, two specific properties of it. The alternative — two separate tests, each asserting one attribute — would have been weaker because someone could update one and forget the other.Felix bundle (#5–#7 in the fix-pass table) is the right shape. The
POST_DEBOUNCE_SLACK_MSJSDoc (322c4183) gives the 350 ms wait a permanent rationale that survives without me. TheisQueryEmpty = $derived(...)refactor (3547a3d8) consolidates thesearchQuery.trim() === ''check that previously appeared twice — a future contributor cannot change the "empty query" predicate in one place and leave the other. TheSEARCH_RESULT_LIMITdecision (kept inmentionConstants.tswith a one-line comment explaining the symmetry-with-other-knobs motivation) is the right deferred call — yes, only one consumer today, but the constant is grouped with the two knobs that do travel together, and the rationale is documented inline.Deferred follow-up issues are filed honestly. ICU pluralisation (#640), AC-5 Given/When/Then (#638), WARN-log per 429 (#633), 90-char hint rationale (#639) — each has an issue, each is linked from the fix-pass summary, and the in-scope/out-of-scope split is clean. The single explicitly deferred item (Nora's
userHasEditedcharacterisation test) gets a one-line rationale ("not blocking and adding it here would expand the PR beyond polish") instead of silent omission. This is the right way to manage scope.Test growth pattern across five rounds: 38 → 46 → 56 → 59 → 60. Every round added tests, no round weakened or deleted any. The 60th test is the singular-branch aria-live test I asked for in round 4 — the suite now distinguishes "the announcer fires" from "the announcer fires with the correct singular/plural form." Five rounds, +22 tests, zero erosion. The traceability from concern → commit → assertion is still recoverable from the fix-pass summaries.
Comment density on the non-trivial tests has stayed proportional to the rationale. The stale-response test, the regression-guard test, the displayName-clip test, the singular-branch test, the extended visible-
<p>test — every one of them carries a comment that names which historical concern (round-2/3/4 + persona attribution) the test was added to guard. A reader who knows nothing about the four-round arc can still reconstruct why each assertion exists. This is what "tests as permanent regression artifacts" looks like in practice: the test name says what, the inline comment says why, the commit SHA in the comment points to the discussion that produced it. Reviewer cost per round on the next PR that touches this code will be a fraction of what round 1 cost on this one.Approving cleanly. From a QA standpoint, this PR is ready to merge. The five-round arc — concern → fix → verify, with explicit deferral discipline and zero test-suite erosion — is the model I want to see survive into the rest of the codebase. Lock in the fix-pass-summary commit-table pattern as the project PR template; it has paid for itself by the third round and is now compounding.
— Sara
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved (merge-ready)
Fifth pass at head
ba307e99. I re-verified that all three of my round-4 polish Suggestions landed inside the follow-up issue bodies themselves (not in PR comments), audited the new follow-up #640 (ICU plural) for spec health, and re-traced the full #380 → PR #629 → follow-up chain end-to-end. The round-4 polish push touched only follow-ups, tests, and one defensive-comment block inMentionDropdown.svelte(aria-hidden="true"on the visible empty-state<p>) — no AC-bearing production behaviour changed since round 4. The requirements artifacts are stable across two consecutive rounds and the follow-up backlog has the cleanest cross-issue interaction map I've audited on this project.I am formally stepping back from blocking notes. Five rounds of review, all three Concerns and all three Suggestions from round 4 closed at the issue-body layer, four out of four Open Questions resolved or tracked. This PR is requirements-ready to merge.
Blockers
None.
Concerns
None.
Suggestions
None on this PR. One forward-looking observation on the follow-up backlog (not a gate, not a polish item — a pattern note):
## Review-fix summaryplaceholder section to.gitea/PULL_REQUEST_TEMPLATE.mdso the pattern becomes the path of least resistance for any multi-round PR going forward. Not a gate on #629 — file as achore: docsif you want to make it durable.Open Questions
All four carry-overs are now resolved or tracked:
MAX_QUERY_LENGTH = 100server-side enforcement) — Closed. #633 Required #1 (@Size(max = 100) @RequestParam String q) + Acceptance line (integration test asserts 400 with code = INVALID_INPUT) substantively encode NFR-SEC-MENTION-001. The named ID prefix is cosmetic.&limit=5honoring) — Tracked. #633 Required #2 (@Min(1) @Max(50) Integer limit). Resolves with #633 implementation.What's done well
All three round-4 polish Suggestions are now closed inside the follow-up issue bodies themselves. Verified at head
ba307e99against the updated issue bodies (not PR comments):make dev(or the equivalent wrapper) once, Then within ~60 s, bothcurl -fsS http://localhost:5173andcurl -fsS http://localhost:8080/actuator/healthreturn HTTP 200 without further intervention, anddocker compose psreports all services as healthy." This is directly machine-testable — a future CI smoke wrapper can ingest the AC verbatim and the test name maps 1:1 to the AC label. The four upstream bullet-list ACs stay appropriately lightweight for the chore scope; AC-5 is the executable one. Exactly the conversion I asked for.**Rationale**:sub-bullet under it that explains the choice in requirements language: "Threshold =MAX_QUERY_LENGTH - 10(i.e. 90) leaves a 10-char leading-edge buffer so the hint appears before the user can paste over the cap." The rationale goes one step further than I asked for and explicitly rejects the two adjacent candidates with reasoning: "Picking 99 would only warn after the silent truncation already happened; picking 50 would warn far too early for typical names." This is the gold-standard format for documenting a magic-number constant — the value, the formula, and the rejected alternatives. The hint copy is locale-correct across de/en/es and the unit-test boundary (length === 89vs.length === 95) pins the threshold behaviour.#640 (ICU plural) is well-formed for its scope. Born out of Leonie S-1 on round 4 (singular/plural key-suffix convention breaks for non-binary-plural locales), #640 has the four-section shape (Context, Required, Acceptance, Reviewer rationale) and names the exact failure mode by example: "Polish (4 forms), Russian (3+), Arabic (6), Welsh (6)." The Required bullets are sized correctly — first investigate Paraglide ICU support, then either migrate or document the constraint with a TODO marker. The Acceptance bullet on a synthetic Polish locale test is the executable boundary that pins the requirement: a test in a >2-form locale is the only way to verify the wrapper actually handles non-binary-plural. The "Audit other count-bearing message keys" bullet broadens the scope from the two new keys this PR introduced (
person_mention_results_count_{singular,plural}) to all count-bearing keys inmessages/{de,en,es}.json— exactly the right scope for a P3-later i18n hygiene chore. Labels (P3-later,feature,ui) match the polish-class severity.The end-to-end traceability chain is now stable across two consecutive rounds. From #380 down to the test-assertion line, every link is recoverable in under a minute:
SEARCH_DEBOUNCE_MS = 150(named constant inmentionConstants.ts).MAX_QUERY_LENGTH = 100→mentionConstants.ts(named symbol) → #633 (server-side enforcement, layered defence) →no-restricted-importslint rule (structural guarantee on fixture isolation).MAX_QUERY_LENGTH_HINT_THRESHOLD = 90→ #639 (soft a11y signal, WCAG 3.3.1, with rejected-alternatives rationale).The polish-pass summary's commit-mapped table (#11216) maintains the structural shape I asked for on round 2. Each Suggestion → commit SHA (
322c4183,3547a3d8,ba307e99) or filed-issue link, plus a separate "Deferred (intentional)" section (1 item, named, with rationale: Nora'suserHasEditedcharacterization test, out of scope for polish). The 60/60 passing verification line + lint + check checklist is the close-out hygiene that lets a reviewer trust the "loop complete" claim without re-running the tests. This is the close-out artifact pattern that should be baked into the PR template (see Suggestion 1 above).The
aria-hidden="true"addition on the visible empty-state<p>(Leonie S-2 on round 4, commit3547a3d8) is a quiet structural a11y win. The defensive comment block above it ("Do NOT add an aria-live attribute here — that would re-introduce the duplicate announcement") converts a one-shot review-comment fix into a permanent guardrail for the next contributor. The companion test (expect(empty!.getAttribute('aria-hidden')).toBe('true')) makes the constraint enforceable at the unit level. This is the same "convert a convention into an executable rule" pattern as theno-restricted-importslint rule on*.test-fixture.svelte— and it's the pattern that has driven the requirements-stability gains across this loop.The singular-branch live-region test (Sara S-4, commit
3547a3d8) closes a real test-coverage gap, not a synthetic one. Without it, the plural branch was the only one with am.person_mention_results_count_*()assertion — meaning a refactor that broke the singular case (e.g. dropping theitems.length === 1branch and always using plural with{count: 1}) would pass tests in de/en/es but be locale-incorrect for the new ICU work in #640. The test discriminates "the singular key exists" from "the singular branch is wired correctly", and that distinction matters because #640 will eventually replace both keys with a single ICU-plural key — at which point this test will be the canary that catches a botched migration.The pattern across five rounds is the durable lesson. Test count 38 → 46 → 56 → 59 → 60 (dropdown only); zero AC dropped; zero test weakened; zero scope creep; every Concern and Suggestion either fixed in PR or filed as a well-formed issue with executable Acceptance. The follow-up backlog (#628, #631–#640, ten issues) is now the cleanest, most cross-referenced, most executable backlog cluster I've audited — and the cross-references aren't just text, they're structural (
#636 aria-controls↔#639 aria-describedby↔#640 ICU pluralall interact on the same<input>, and each issue body flags the others). The requirements-engineering instinct on display here — convert every convention into a gate (lint rule, comment-paired test, named constant in a shared module, Given-When-Then AC with measurable threshold) — is the right instinct, and it has compounded across rounds.— Elicit
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved (merge-ready)
Round-5 re-read at
ba307e99. I commit-by-commit-checked the three round-4SuggestionsI owned against the new head and re-spot-checked the Svelte-5 idioms in the touched components. All three are properly closed; nothing in this polish pass changes behaviour I haven't already greenlit. From a fullstack-implementation standpoint this is done — no remaining nits worth holding the merge.Blockers
None.
Concerns
None.
Suggestions
None for this round. The three round-4 items I raised are closed cleanly and the surrounding code now reads better, not just satisfies-the-reviewer better.
What's done well
A5 (
POST_DEBOUNCE_SLACK_MSJSDoc) is inPersonMentionEditor.svelte.spec.ts:18–24and reads exactly the way I wanted. Multi-line block comment names (a) the headroom relationship toSEARCH_DEBOUNCE_MS, (b) the calibration anchor ("350 ms is calibrated against CI-runner jitter we observed pre-#629"), (c) the empirical lower bound ("dropping it below ~200 ms reintroduces flake"), and (d) the review trail to Sara's#10935. The next reviewer who wonders why the slack is 350 ms and not 100 / 1000 has the answer at the call site, not buried in a commit message. Cosmetic suggestion executed with the right amount of context — not over-documented.A6 (
isQueryEmpty = $derived(searchQuery.trim() === '')) is the right shape and the comment names the rule it consolidates.MentionDropdown.svelte:43–46— three lines of comment explaining "Intent-revealing alias used by both the persistent aria-live announcer and the visible empty-state copy. Folding the duplicated rule into one $derived keeps the two branches in lockstep. Felix #3 on PR #629 round 4." Both call sites (:192in the sr-only aria-live region,:209in the visible<p>) now read{isQueryEmpty ? m.person_mention_search_prompt() : m.person_mention_popup_empty()}. The decision now lives in one place; the next person who adds a third branch (Sara/Elicit #634 "errored" copy) has one symbol to extend, not two parallel branches to keep in sync. Two-line × 2-site duplication folded into one $derived + 2 references — net is the same line count but the business rule is single-sourced.A7 (
SEARCH_RESULT_LIMITdocumented for single-consumer reasoning) closes the symbol-with-no-explanation problem I flagged.mentionConstants.ts:7–10— JSDoc explicitly states "Defensive client-side cap on the result list. Single consumer today (PersonMentionEditor), kept here for symmetry with the other limit knobs so the @mention configuration lives in one place. Felix #1 on PR #629 round 4." This is the right disposition: the symbol stays in the shared module (option b on my menu), but the comment names the symmetry-with-other-knobs motivation so a future reader doesn't ask "why is this in a shared module when only one file imports it?" If the dropdown ever grows result-count UI that needs the same number (Markus #633 follow-up), the symbol is in the right place.Bonus: Sara's S-1 (
tick()instead ofsetTimeout(r, 50)) is inPersonMentionEditor.svelte.spec.ts:339–342with a comment explaining the polling guarantee. "Flush pending Svelte reactivity so any (non-)update from the stale fetch resolution has landed before we assert. expect.element already polls, so no fixed-timeout fallback is needed. Sara on PR #629 round 4." Same idiom Sara verified in the round-3 sticky-takeover fix. Two of three Sara/Felix race-related tests now usetick()+expect.elementpolling instead of fixed timeouts — the remainingsetTimeout(r, SEARCH_DEBOUNCE_MS + POST_DEBOUNCE_SLACK_MS)waits are correct because they are negative assertions (proving no fetch fires after the debounce window), where polling doesn't apply.Bonus: Sara's S-3 (displayName-clip assertion tightened) is in
PersonMentionEditor.svelte.spec.ts:587–589. Switched fromlength <= 100totoHaveLength(100)with a comment naming why: "Tight assertion: input is 105 chars, cap is exactly 100. UsingtoHaveLength(100)discriminates 'clip works' from 'clip works AND nothing weakened it to e.g. 95'." This is the right shape for a layered-defence assertion — the test now goes red not just when the clip disappears, but when it silently weakens. Single character (<=→===-equivalent) but the test contract is meaningfully stronger.Bonus: Sara's S-4 (singular-branch aria-live test) is at
MentionDropdown.svelte.test.tsin the new "announces the singular form when exactly one item is present" test. Single-item items array + assertion that the persistent live region containsm.person_mention_results_count_singular(). Closes the pluralisation-coverage gap Sara raised — the singular code path was reachable only by accident before this test existed, now it's pinned.Bonus: Leonie's S-2 (
aria-hidden="true"on visible empty-state<p>) is inMentionDropdown.svelte:206with a 5-line defensive comment. "Visible empty-state copy — visual-only. The persistent sr-only<p>above is the sole AT announcer; this one is hidden from screen readers viaaria-hidden="true"so VoiceOver does not double-announce (NVDA de-dups, VoiceOver does not). Leonie S-2 on PR #629 round 4. Do NOT add an aria-live attribute here — that would re-introduce the duplicate announcement." Extended unit test "keeps the visible empty-state copy without its own aria-live and hides it from AT" pins both (a) noaria-liveattribute and (b)aria-hidden="true"so a regression goes red immediately. The "Do NOT add aria-live" sentence is the kind of defensive comment that pays off when someone autofixes the markup later and thinks they're being helpful.The Svelte-5 idioms across the diff remain clean.
searchQueryis$state(untrack(() => editorQuery.slice(0, MAX_QUERY_LENGTH)))— initial value computed without subscribing toeditorQueryso the mirror$effectis the sole driver.userHasEditedlatch on the input'soninputhandler pins ownership the first time the user types into the search box, so the mirror$effectbecomes a no-op without removing the bidirectional flow.$derivedforisQueryEmpty(no$effectmasquerading as derivation). Keyed{#each model.items as person, i (person.id)}in the listbox. No$state+$effectto compute derived values; no plainMap/Setin reactive context; no unkeyedeach. All four canonical traps avoided.The fix-pass summary's commit-mapped table (#11216) carries the round-2/3/4 model into round 4. Every round-4
Suggestionsitem → resolution → commit SHA → status. Deferred items are explicitly named and bounded (Nora'suserHasEditedcharacterization test, deferred to a future polish ticket with rationale). New follow-up issues (#638, #639, #640) give reviewers a one-click verification path. Verification block names what was run (vitest,npm run check,npm run lint) and the resulting test count delta (60/60, +1 from the new singular-branch test). This is the close-out artifact I've now seen four rounds in a row.This PR has been through five reviewer rounds and four fix passes. The behaviour is unchanged from round 3 onwards — every round since has been polish, documentation, and structural hardening. From my domain (TDD evidence, clean code, Svelte-5 patterns, fullstack consistency), I have no remaining flags. Merge it.
— Felix
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Round-5 review at head
ba307e99. This is a polish-only delta on top of an already-approved security posture (round 4 — #11192). I re-walked the entire frontend diff, audited the new and modified surfaces against the round-4 fix-pass summary (#11216), and independently verified my single round-4Suggestionsitem landed on #633.What I verified this round
1. My round-4 suggestion (WARN-log on 429) is closed in #633 — verified end-to-end (CWE-778 / CWE-770). Fetched issue #633 directly. The body now contains, under the
### Additional concern — CWE-770 (Resource throttling)section, the literal text:That's the full request: WARN level, principal+rate context, CWE-778 cited, and a noise-control hedge (
sample to 1/10) that I would have flagged on round 6 if it weren't already there. The 1/10 sampling is the right shape — abusive callers will trip it many times per second, so sampling preserves the signal while protecting Loki from log amplification on a real DoS event. The closure on #633 is complete and the spec is now sufficient for the implementer to act on without round-tripping back to me.2. CWE-770 / CWE-602 closure on #633 — independently re-verified. The issue body holds the full envelope I asked for on round 3: per-session limit on
/api/persons, 20 rps burst / 5 rps sustained (justified against the frontend's 6.7 rps self-limit, so 3× headroom for autofocus/batch use), integration-test AC (21st request inside 1 s returns 429 with code = TOO_MANY_REQUESTS), burst-recovery assertion, an explicit out-of-scope statement (framework-level rate-limit deferred to a sibling), and now the WARN-log sub-bullet from round 4. That's a complete security RFC for a single endpoint — nothing more to add from my side until #633 is picked up.3. Round-4 polish delta is test/comment/refactor only — zero new attack surface. I read the full diff at this head. The 13 changes mapped in #11216 fall into four buckets, none of which open a new exposure:
*.test-host.svelte→*.test-fixture.svelte) + ESLint guard already-verifiedeslint.config.js(untouched since round 4)tick()swap-in, intent comments,toHaveLength(100)tightening, singular-branch testPersonMentionEditor.svelte.spec.ts,MentionDropdown.svelte.test.tstoHaveLength(100)tightening directly hardens the CWE-400 layered-cap test — see point 4 below.isQueryEmpty = $derived(...)extractionMentionDropdown.sveltesearchQuery.trim() === ''expressions with one named symbol. No control-flow change, no new branch, no input handled differently. Same value, one source.aria-hidden="true"on visible empty-state<p>MentionDropdown.sveltearia-hiddenis a screen-reader visibility hint, not a security-sensitive attribute. No new DOM injection vector, noinnerHTMLuse, no user-input interpolation — the<p>content is twom.person_mention_*Paraglide message-helper calls that resolve to static strings at compile time (no runtime interpolation of user input). XSS-clean.No new
fetch()call sites, no new endpoint reads, no new POST/PUT/DELETE handlers, no new auth/CORS/CSRF/CSP touchpoint, no new storage write, no new file upload code, no Tiptap config change that could affect content sanitisation, no new dependency. The entire delta is contained inside two.sveltefiles, two.spec.ts/.test.tsfiles, and three i18n JSON files (which only add static strings).4. CWE-400 layered-cap test was actually strengthened this round. The displayName clip test (
PersonMentionEditor.svelte.spec.ts~line 538) now assertsexpect(host.snapshot.mentionedPersons[0].displayName).toHaveLength(100)instead of the round-4toBeLessThanOrEqual(100). The difference matters:<=100passes if a future refactor accidentally truncates earlier (say, to 95 chars), silently weakening the cap.=== 100discriminates "clip works correctly" from "clip works AND nothing else mangled it." This is exactly the defensive-test posture I would have requested in round 5 — Sara got there first. The CWE-400 amplification fix on Felix #3 is now pinned at both ends: the input string is 105 chars, the asserted output is exactly 100. A regression in either direction is now visible.5. Singular/plural i18n keys (CWE-79 check on the new aria-live content). The new persistent
<p aria-live="polite">block has three text branches —m.person_mention_search_prompt(),m.person_mention_popup_empty(),m.person_mention_results_count_singular(), andm.person_mention_results_count_plural({ count: model.items.length }). The only one that interpolates a value is the plural variant, and the value ismodel.items.length— a number derived from anArray.lengthcall on the response payload, not from user-controllable string content. Paraglide's compiled messages already escape ICU-style placeholders at build time. No XSS path, no template injection seam. Clean.6.
mentionConstants.tsSEARCH_RESULT_LIMIT decision (Felix #11186 S-1) — security-neutral. The constant stayed in the shared module rather than getting inlined intoPersonMentionEditor.svelte. Either choice would have been fine from a security standpoint — keeping it shared means the two layers of defence (URL&limit=5+ client-sidedata.slice(0, SEARCH_RESULT_LIMIT)) reference the same symbol, so a future tuner can't drift them apart and accidentally widen the result window past what the server is told to return. That's the right defensive posture for a layered cap, same reasoning asMAX_QUERY_LENGTH.What's done well
toHaveLength(100)) is now tighter than the original blocker required. Round 3 I flagged the bypass; round 4 Felix fixed it; round 5 Sara tightened the assertion past the boundary I'd have asked for. The cap is now pinned at both ends with a test that fails on any deviation in either direction. This is the right end-state for a CWE-400 control.aria-hidden="true"is documented with a "do NOT add aria-live" warning comment. The comment block above the visible empty-state<p>explicitly tells future contributors not to re-addaria-liveto that element. This is the right place for a defensive comment — it captures the round-4 design rationale (sole AT announcer is the persistent sr-only region) right where a contributor would otherwise reintroduce the bug. Good doc-as-guardrail pattern.Blockers
None.
Concerns
None.
Suggestions
None this round. The round-4 polish closed every open suggestion I had carried (the WARN-log on #633), and the round-5 fix-pass tightened things beyond what I asked for. The remaining open item is the
userHasEditedcharacterization test I flagged in round 4'sNote for follow-up— and that's correctly listed asDeferred (intentional)in #11216. Not gating, capture as future polish if budget allows.Note for follow-up
Carrying forward from round 4 — #640-candidate — characterization test for the
searchQueryownership latch (CWE-20 hardening). TheuserHasEditedlatch pins ownership forever once flipped. Empirically the "user types, then Backspace-deletes everything, then editor keystroke arrives" path is correctly handled today (user's empty string wins), but no test exercises it. A future refactor that swaps the latch forsearchQuery !== editorQuerylogic would silently regress this. Pure hardening — P3, file when there's bandwidth.Five rounds, two CWE closures (CWE-400 layered cap + CWE-770/778 tracked-and-spec'd on #633), zero rollbacks, zero weakening. That's a clean security trajectory for a PR of this scope.
— Nora
de + es already use singular ("Person suchen", "Buscar persona"); en was plural ("Search persons"). Switch en to "Search for a person" so all three locales announce a singular search control to screen-reader users — cross-locale parity polish. Leonie #1 on PR #629 round 3. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>The aria-live region previously lived inside {#if items.length === 0} so it remounted whenever items transitioned between empty and populated — VoiceOver in particular swallows announcements from freshly-mounted live regions, and the "N persons found" announcement was missing entirely on the populated branch. Move the live region above the conditional so the element persists, and announce a localized "1 person found" / "N persons found" count on the populated branch. The visible empty-state <p> stays as a visual cue (no aria-live). Leonie #3 on PR #629 round 3. Adds person_mention_results_count_singular / _plural in de/en/es. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>ba307e991bto5bcb4717b6View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.