feat(transcription): re-edit existing @mention by pre-filling the search input (#628) #717
Reference in New Issue
Block a user
Delete Branch "feat/issue-628-mention-reedit"
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 #628.
Adds an explicit re-edit affordance to saved
@mentions: each rendered mention carries a pencil that opens the search dropdown pre-filled with the storeddisplayName, letting a transcriber re-link the person without retyping.What changed
createMentionControllerinPersonMentionEditor.svelte): the dropdown lifecycle (mountedDropdown/requestId/debouncedSearch/dropdownState) is lifted out of Tiptap'ssuggestion.render()closure into one owner withopen/update/close/onKeyDown.render()is now a thin adapter. This is the single owner the AC-6 invariant needs;requestIdincrement-on-open is preserved (+ bumped on open so a superseded open's in-flight fetch is discarded).mentionNodeView.ts): hosts each mention as a Tiptap NodeView rendering the@displayNametoken text astextContent(neverinnerHTML) plus acontenteditable=falsepencil in a fixed-width slot — instant opacity reveal on whole-token hover + keyboard focus, no reflow.renderHTML/renderTextstay for serialization/clipboard.commitRelink: swaps onlypersonIdin place viasetNodeMarkup, so the storeddisplayNameis preserved by construction (never round-tripped throughinsertContentAt).personIdis sourced solely from the selectedPerson— never the reflecteddata-person-idor the clipped search text.MentionDropdown.svelte): a visible × control (shared with the fresh-@ path) + Escape; the re-edit path (no suggestion plugin to forward keys) focuses the search input on open and handles Arrow/Enter/Escape itself.aria-liveregion (editingDisplayName) — no second live region.person_mention_edit_label,person_mention_editing_announce,person_mention_dismiss_labelinmessages/{de,en,es}.json.Acceptance criteria
AC-1…AC-9 + NFRs (no-reflow, a11y announce,
MAX_QUERY_LENGTHclip, anti-spoof provenance) covered by browser tests inPersonMentionEditor.svelte.spec.ts. The serializer round-trip (textbyte-identical, onlypersonIdswapped) is pinned.Deviation — flagging for review
The
justCommittedlatch from the Implementation Notes is not added: it guards theselectionUpdate-driven auto-open that Resolved Decision 1 removed, so there's no vector to suppress in the explicit-gesture design, and an unexercised latch risked blocking a legitimate immediate re-edit. Trivial to add back if preferred.Testing
vitest-browser-svelte) — validated by the--project=clientCI lane.npm run check+eslintclean for all changed files;prettierenforced by the pre-commit hook.Docs
No migration / route / permission /
ErrorCode/ domain term → no doc-table updates (per the issue's Docs section).🤖 Generated with Claude Code
Passes editingDisplayName into MentionDropdown; the persistent aria-live region announces person_mention_editing_announce({displayName}) on re-edit open and falls back to the prompt/empty/count copy once the user edits or results arrive. Routed through the SAME sr-only region as the result count — no second live region (avoids the double-announce bug Leonie S-2 fixed). Fresh-@ passes an empty editingDisplayName, so its announcements are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>Clean-agent review → fixes applied (commit
a62a333d)A fresh-eyes reviewer (no prior context) checked the diff against #628's ACs. Verdict: approve-with-nits, no blockers. Two real correctness issues were confirmed and fixed:
opacity-0alone doesn't remove an element from hit-testing, and the 44px button overhangs adjacent text — so a click in the gap between two mentions could land on the invisible pencil and open the dropdown (an AC-8 hole, not caught by the render-time-only AC-8 test). Fix:pointer-events-nonewhile hidden, re-enabled with the opacity reveal on hover/focus.setEditable()emits"update", not a transaction, so the NodeView's'transaction'listener missed a runtimedisabledflip (stalearia-disabled/tabindex; the runtime open-guard still held, so no security impact). The comment was also wrong. Fix: listen on'update'(also skips selection-only changes → fires far less often, resolving nit N1).update()so the pencil opens with the livedisplayName.New tests: structural guard that the hidden pencil is
pointer-events-none+ reveals, and a mid-session disable-flip test (fixture gained anonReady/setDisabledhook).Confirmed sound by the reviewer (no change needed): the single-controller refactor preserves the fresh-@ path, the
requestIdstale-fetch guard, and the omission of thejustCommittedlatch (the auto-open vector it guarded was removed by Resolved Decision 1).Re-checked:
npm run check+eslintclean for all changed files. Browser tests run on the--project=clientCI lane.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
This is the structure I'd want. The single-controller refactor (
createMentionController) is the right move — it makes AC-6 fall out of one owner instead of two code paths racing over module state, andrender()reads as a thin adapter (onStart→open,onUpdate→update,onExit→close). Two named commit functions (commitFreshInsert/commitRelink) instead of a boolean branch — exactly right, andcommitRelinkpreservingdisplayNameby construction viasetNodeMarkupis the clean way to honour the #380 invariant.What I checked
$effect-computes-state anti-patterns introduced;mount()/unmount()lifecycle owned in one place withonDestroycleanup intact.textContentfor the user-influenced name,createElementNSfor the static glyph,destroy()removes the listener.currentNodetracking added in the review pass is the correct hardening.requestRelink,dismiss,writeState,syncEditableall reveal intent. Guard-first inopenRelink/requestRelink.Suggestions (non-blocking)
PersonMentionEditor.svelteis now ~370 lines. Still cohesive (one editor, one controller), and you correctly deferred extracting amentionController.svelte.tsuntil a second call site appears — agreed. Just flagging the line count for the next person.as object/as unknown as DropdownExportscasts aroundmount()are unavoidable given Svelte's mount typing; fine.TDD note: most new behaviour has a test, and the red/green discipline is visible. The single soft spot is the small set of behaviours that were locked in after the controller refactor rather than test-first — but they're genuinely covered now.
🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
Frontend-only, no module-boundary or transport implications. The judgment calls land where I'd put them.
What I checked
PersonMentionEditor.svelterather than being lifted into a.svelte.tsmodule — correct for one editor / one consumer. Extract when a second call site appears, not before.mentionNodeView.tsas a pure module (DOM-building, no runes) is a clean seam: testable, framework-light, and it leavesrenderHTML/renderTextin place for serialization/clipboard. Good separation of "live editing DOM" vs "serialized form".// eslint-disable boundaries/dependenciesforpersonLifeDatesinMentionDropdownis pre-existing and unchanged — not this PR's concern.Documentation gate
Ran the doc-table: no Flyway migration, no new route/controller/service, no
ErrorCode/Permission, no new domain term. New i18n keys are not a doc-table trigger. No doc updates required — matches the PR's own Docs section. Nothing to block on.Note (not a blocker)
The client-side-fetch ADR follow-up referenced in
PersonMentionEditor("ADR: client-side fetch exception for editor suggestion plugins") is still open — but that's inherited from #629, not introduced here. This PR reuses the same justified exception; fine to leave the ADR to its own ticket.🛡️ Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
The user-influenced data paths are handled correctly, and there's a regression test for each. No new attack surface.
What I checked
mentionNodeView.tsbuilds the token astextContentand the pencil glyph viacreateElementNSelement-by-element. NoinnerHTML, nodocument.write, no string-interpolated markup anywhere adisplayNameflows.data-person-id/data-display-namego throughsetAttribute, which escapes. The XSS test (<img src=x onerror=...>payload) still asserts no real element materialises. ✅MAX_QUERY_LENGTHis preserved inwriteState, and there's an explicit test that a 150-char storeddisplayNameclips the search input to 100 while leaving the node text full-length. The clip applies to the query only — correct, the stored text must not be truncated. ✅commitRelinksourcespersonIdsolely from the selectedPersonobject (item.id), never from the reflecteddata-person-idor the search text. The security test picks a person whose id differs from the one in the DOM to prove it. ✅encodeURIComponent(query)on the fetch URL; same/api/persons?review=trueendpoint and cookie-via-proxy auth as before. No change to the network surface.Nothing to fix. The "every fix gets a test" discipline is visible — the clip and the provenance both have permanent regression tests.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
The suite is genuinely well-built — descriptive names, factory helpers, AAA, and the stale-fetch test is deterministic (promise-controlled
requestIdordering, nosleep). That's the hard one done right. My concerns are about the gap between what's asserted and what runs locally, not about correctness.Concerns (should address)
className.toContain('pointer-events-none'). That pins the mechanism, but the actual AC-8 risk — a real pointer click landing in the gap between two adjacent mentions — has no behavioural test. A syntheticdispatchEvent('click')won't catch it (it ignorespointer-events). The honest behavioural test needs a realuserEvent.clickat a gap coordinate, which is fiddly to target. Acceptable to ship the structural guard, but please log a follow-up for a coordinate-based gap-click test (ideally Playwright E2E wherepointer-eventsis enforced).setTimeoutsleep in the AC-7 "no dropdown mounts" test (SEARCH_DEBOUNCE_MS + POST_DEBOUNCE_SLACK_MS). I normally reject sleeps — but this is a negative assertion (proving an event never happens), where there's nothing toawait. It reuses the calibrated slack constant, so I'll allow it. Don't let this pattern spread to positive assertions.What I verified will hold in CI (reasoning, since the
--project=clientlane can't run locally)dispatchEvent('mousedown')for option selection matches the dropdown's actual handler — consistent with the pre-existing #629 tests, sound.fill()over per-keystroketype()to dodge debounce jitter — correct.focusOnMountand mid-session-disable assertions are wrapped invi.waitFor, so async focus/setEditable→updatepropagation won't flake.onReady/setDisabledhook is the right way to exercise the reactivedisabledpath — and it's the test that would have caught the original'transaction'-listener bug. Good.Note
Per project convention these run only in CI. Watch the
clientlane on this PR before merge — that's the real gate, and I can't sign off on green I haven't seen. Everything I can reason about looks deterministic.🎨 Leonie Voss — UX & Accessibility Lead
Verdict: ⚠️ Approved with concerns
The affordance respects the senior-first constraints I care about, and the a11y wiring is mostly textbook. Two things to verify/track before I'm fully happy.
What's right
h-11 w-11), glyph kept small (~16px) — meets WCAG 2.2 AA target size while keeping the inline visual light. ✅tabindex 0,focus-visible:ring-2 ring-brand-navy; the × button has the same ring. Keyboard focus reveals the pencil (group-focus-within), so it's not pointer-only. ✅aria-labelvia the new i18n key on the icon-only button. ✅transition-none— an instant opacity swap, no slide/fade. Exactly right, costs nothing. ✅pointer-events-none-while-hidden fix also means the invisible target can't hijack a caret click. ✅aria-liveregion — no double-announce. ✅Concerns
text-ink-2onbg-surface. A meaningful icon needs ≥3:1.ink-2is mid-tone — please confirm the exact ratio againstsurface; if it's under 3:1, bump totext-ink(the token text already uses the darker ink). This is the same class of finding I raised on the dropdown rows in #629, so worth a quick check rather than an assumption.:hoveron touch, the pencil only appears on focus. Resolved Decision 2 scoped this to laptop-primary transcribing and accepts native caret precision, so I'm not blocking — but for the record, a touch transcriber won't discover the pencil by hovering. If touch ever becomes a real path for the author flow, revisit with a tap-to-reveal affordance.Both are quick. Fix #1 if the ratio misses; #2 is documented-and-accepted.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Traceability is clean — every acceptance criterion maps to at least one named test, and the one deviation is surfaced, not buried.
AC → test traceability
data-person-idswap + text unchangedOpen-decision register
justCommittedlatch deliberately omitted. Correctly logged in the PR body with a sound rationale (the auto-open vector was removed by Resolved Decision 1). This is the right way to handle a spec instruction you're consciously not following — surfaced for the PO, reversible in one commit. No action needed unless the PO wants the belt-and-suspenders guard.One requirements-completeness note (not a blocker)
AC-8's most important behaviour — "no dropdown when the caret lands between two adjacent mentions" — is currently proven only structurally (the
pointer-events-noneclass), not by a real gap-click. The requirement is satisfied by the implementation, but the evidence is one level removed. Suggest a logged follow-up for a behavioural gap-click test (Sara flagged the same). Scope is otherwise tight — no creep, AC-9's keyboard parity is justified by AC-9 itself.⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved (nothing in my domain)
Checked for anything operational and found none — as expected for a frontend feature PR.
docker-compose*.yml, Dockerfile, or service-topology changes.--project=clientlane.LGTM from an infra standpoint. The only operational reminder is the one everyone's already noted: this lands on the client test lane in CI, so let that go green before merge.
CI: server coverage gate fixed (commit
e6a0fbc9)The "unit and component tests with coverage" step failed on the coverage threshold (functions 78.68%, branches 78.48% < 80%) — not on any test (785/785 passed).
Cause:
mentionNodeView.tsis a Tiptap NodeView that builds live ProseMirror DOM — it only runs in the browser editor, so it's exercised by the client lane, not the node coverage run. But it's a plain.tsunder the coverageincludeglobs, soall-files counted it at 0% and dragged the global below 80%.Fix: exclude it from the node coverage config, alongside the
.svelte/ browser-only UI files that config already measures around (it states it measures "utility and server-side logic only"). Verified locally on theserverproject:Note on the
RollupError: PARSE_ERROR pos 26log: it's non-fatal (the build exits 0 with it present) and pre-existing — it comes from the v8 coverage provider raw-parsing an unrelated uncovered file (testHelpers.ts/mentionConstants.ts), not from this PR, and it was already in the original run. Left as-is (out of scope).Still to watch: the client browser-test lane (the ~37 new
PersonMentionEditor.svelte.spec.tscases) runs in a separate step — that's the behavioural gate for this feature.