From b453c13bae9f0710017b3d7f94a48a9e27a6f987 Mon Sep 17 00:00:00 2001 From: Marcel Date: Tue, 2 Jun 2026 19:22:27 +0200 Subject: [PATCH] refactor(transcription): lift @mention dropdown lifecycle into a single controller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pulls mountedDropdown / requestId / debouncedSearch / dropdownState ownership out of Tiptap's suggestion.render() closure into one createMentionController(). render() becomes a thin adapter: onStart→open, onUpdate→update, onExit→close. This is the single-owner structure #628 needs for the AC-6 single-dropdown invariant — the upcoming pencil re-edit affordance opens via the same controller.open() instead of racing the suggestion plugin over module state. open() now also bumps the request token so an open-A→open-B sequence discards A's in-flight fetch (preserved increment-on-open semantics). No behaviour change for the fresh-@ path — existing browser suite is the regression guard. Co-Authored-By: Claude Opus 4.8 --- .../discussion/PersonMentionEditor.svelte | 241 ++++++++++-------- 1 file changed, 140 insertions(+), 101 deletions(-) diff --git a/frontend/src/lib/shared/discussion/PersonMentionEditor.svelte b/frontend/src/lib/shared/discussion/PersonMentionEditor.svelte index 39261606..98e19632 100644 --- a/frontend/src/lib/shared/discussion/PersonMentionEditor.svelte +++ b/frontend/src/lib/shared/discussion/PersonMentionEditor.svelte @@ -63,6 +63,133 @@ type DropdownExports = { onKeyDown: (event: KeyboardEvent) => boolean; }; +type CommitFn = (item: Person) => void; +type RectGetter = (() => DOMRect | null) | null; + +// Tiptap's SuggestionProps types `command` against the default MentionNodeAttrs +// (id/label). Our custom Mention extension uses personId/displayName, so the +// render() adapter casts the renderProps to this looser shape locally. +type LooseRenderProps = { + items: unknown; + command: (props: { personId: string; displayName: string }) => void; + query: string; + clientRect?: (() => DOMRect | null) | null; +}; + +// Single owner of the dropdown lifecycle. Both entry points route through one +// controller — Tiptap's fresh-`@` suggestion (via the render() adapter below) +// and (#628) the pencil re-edit affordance — so at most one dropdown is ever +// mounted (the AC-6 single-dropdown invariant). open() closes any prior +// dropdown first; render() is a thin adapter over open()/update()/close(). +type MentionController = { + open: (clientRect: RectGetter, query: string, commit: CommitFn) => void; + update: (clientRect: RectGetter, query: string, commit: CommitFn) => void; + close: () => void; + onKeyDown: (event: KeyboardEvent) => boolean; +}; + +function createMentionController(): MentionController { + // Request-token guard: every search AND every open bumps `requestId`; + // runSearch captures the id active when its fetch starts and discards the + // response if a newer search/open has happened since. Without this, a late + // response can repopulate a dropdown the user already moved on from (e.g. + // open A → open B → A's stale response). Sara on PR #629. + let requestId = 0; + let exports: DropdownExports | null = null; + + const runSearch = async (query: string) => { + const id = requestId; + try { + // Defensive client-side cap — server-side enforcement is tracked + // separately. Markus on PR #629. + const res = await fetch( + `/api/persons?review=true&q=${encodeURIComponent(query)}&size=${SEARCH_RESULT_LIMIT}` + ); + if (id !== requestId) return; + if (!res.ok) { + dropdownState.items = []; + return; + } + const body = (await res.json()) as { items?: Person[] }; + if (id !== requestId) return; + dropdownState.items = (body.items ?? []).slice(0, SEARCH_RESULT_LIMIT); + } catch { + if (id !== requestId) return; + dropdownState.items = []; + } + }; + + const debouncedSearch = debounce(runSearch, SEARCH_DEBOUNCE_MS); + // Hoisted so onDestroy can cancel any pending fetch even after the editor is + // gone — a trailing debounced search would otherwise pollute later tests. + cancelPendingSearch = () => debouncedSearch.cancel(); + + const onSearch = (query: string) => { + requestId++; + if (query.trim() === '') { + debouncedSearch.cancel(); + dropdownState.items = []; + return; + } + debouncedSearch(query); + }; + + const close = () => { + debouncedSearch.cancel(); + if (mountedDropdown) { + unmount(mountedDropdown); + mountedDropdown = null; + exports = null; + } + }; + + // Clip the query once so the dropdown's editor-mirror sees the capped value + // (CWE-400 amplification — Nora #1 / Felix #3 on PR #629). The commit closure + // is supplied by the caller, so the fresh-insert path keeps clipping the + // inserted displayName while the relink path (#628) preserves the stored + // displayName by construction. + const writeState = (clientRect: RectGetter, query: string, commit: CommitFn) => { + dropdownState.command = commit; + dropdownState.clientRect = clientRect; + dropdownState.editorQuery = query.slice(0, MAX_QUERY_LENGTH); + }; + + const open = (clientRect: RectGetter, query: string, commit: CommitFn) => { + // Single-dropdown invariant: tear down any open dropdown before mounting a + // new one, and bump the request token so a previous open's in-flight fetch + // cannot repopulate this dropdown. + close(); + requestId++; + dropdownState.items = []; + writeState(clientRect, query, commit); + const mounted = mount(MentionDropdown, { + target: document.body, + props: { + model: dropdownState, + // MentionDropdown reads `editorQuery` off the shared state proxy via + // this getter — Svelte 5's mount() does not expose settable prop + // accessors, so we route through the proxy (same pattern as items). + get editorQuery() { + return dropdownState.editorQuery; + }, + onSearch + } + }); + mountedDropdown = mounted as object; + exports = mounted as unknown as DropdownExports; + }; + + const update = (clientRect: RectGetter, query: string, commit: CommitFn) => { + writeState(clientRect, query, commit); + }; + + const onKeyDown = (event: KeyboardEvent) => exports?.onKeyDown(event) ?? false; + + return { open, update, close, onKeyDown }; +} + +const controller = createMentionController(); + onMount(() => { // Custom Mention node: uses personId / displayName instead of the // default id / label attribute names so the mentionSerializer can @@ -172,120 +299,32 @@ onMount(() => { ]) .run(); }, + // Thin adapter over the single mention controller. The fresh-`@` + // commit clips the typed query into the inserted displayName + // (AC-1, #380); the controller owns the dropdown lifecycle and the + // debounced/request-token search. render() { - let exports: DropdownExports | null = null; - - // Tiptap's SuggestionProps types `command` against the default - // MentionNodeAttrs (id/label). Our custom Mention extension uses - // personId/displayName, so we cast the renderProps locally. - type LooseRenderProps = { - items: unknown; - command: (props: { personId: string; displayName: string }) => void; - query: string; - clientRect?: (() => DOMRect | null) | null; + const buildFreshCommit = (loose: LooseRenderProps): CommitFn => { + const clippedQuery = loose.query.slice(0, MAX_QUERY_LENGTH); + return (item: Person) => + loose.command({ personId: item.id, displayName: clippedQuery }); }; - - // Request-token guard: every onSearch invocation bumps `requestId`; - // runSearch captures the id active when its fetch starts and discards - // the response if a newer onSearch has fired since. Without this, a - // late response can repopulate the dropdown after the user cleared - // the search input. Sara on PR #629. - let requestId = 0; - const runSearch = async (query: string) => { - const id = requestId; - try { - // Defensive client-side cap — server-side enforcement is tracked - // separately. Markus on PR #629. - const res = await fetch( - `/api/persons?review=true&q=${encodeURIComponent(query)}&size=${SEARCH_RESULT_LIMIT}` - ); - if (id !== requestId) return; - if (!res.ok) { - dropdownState.items = []; - return; - } - const body = (await res.json()) as { items?: Person[] }; - if (id !== requestId) return; - dropdownState.items = (body.items ?? []).slice(0, SEARCH_RESULT_LIMIT); - } catch { - if (id !== requestId) return; - dropdownState.items = []; - } - }; - const debouncedSearch = debounce(runSearch, SEARCH_DEBOUNCE_MS); - cancelPendingSearch = () => debouncedSearch.cancel(); - const onSearch = (query: string) => { - requestId++; - if (query.trim() === '') { - debouncedSearch.cancel(); - dropdownState.items = []; - return; - } - debouncedSearch(query); - }; - - const updateState = (renderProps: LooseRenderProps) => { - // Clip once here so both the inserted displayName and the - // dropdown's editor-mirror see the same value. The dropdown - // already clips the mirror (Nora #1 CWE-400), but without - // clipping at the command boundary an unclipped query would - // still flow through as the inserted displayName — visible - // UI divergence between "what I searched" and "what was - // inserted". Felix #3 on PR #629. - const clippedQuery = renderProps.query.slice(0, MAX_QUERY_LENGTH); - // AC-1: pass typed query as displayName, not person.displayName - dropdownState.command = (item: Person) => - renderProps.command({ - personId: item.id, - displayName: clippedQuery - }); - dropdownState.clientRect = renderProps.clientRect ?? null; - dropdownState.editorQuery = clippedQuery; - }; - return { onStart(renderProps) { const loose = renderProps as unknown as LooseRenderProps; - updateState(loose); - // MentionDropdown reads `editorQuery` off the shared state - // proxy via its `editorQuery` prop binding below — this is - // the same pattern as `model.items`. We do not pass it as a - // separate prop because Svelte 5's mount() does not expose - // settable prop accessors, so we route through the proxy. - const mounted = mount(MentionDropdown, { - target: document.body, - props: { - model: dropdownState, - get editorQuery() { - return dropdownState.editorQuery; - }, - onSearch - } - }); - mountedDropdown = mounted as object; - exports = mounted as unknown as DropdownExports; + controller.open(loose.clientRect ?? null, loose.query, buildFreshCommit(loose)); }, onUpdate(renderProps) { - updateState(renderProps as unknown as LooseRenderProps); + const loose = renderProps as unknown as LooseRenderProps; + controller.update(loose.clientRect ?? null, loose.query, buildFreshCommit(loose)); }, onKeyDown({ event }) { // Escape is handled by the suggestion plugin itself. if (event.key === 'Escape') return false; - return exports?.onKeyDown(event) ?? false; + return controller.onKeyDown(event); }, onExit() { - // Cancel any pending debounce so a closed dropdown's trailing - // runSearch cannot fire against the *next* dropdown's state. - // The hoisted `cancelPendingSearch` would be overwritten by - // the next render()'s onStart before the trailing call fires, - // so we cancel locally via the closure-scoped debouncedSearch. - // Felix #1 on PR #629. - debouncedSearch.cancel(); - if (mountedDropdown) { - unmount(mountedDropdown); - mountedDropdown = null; - exports = null; - } + controller.close(); } }; }