feat(transcription): re-edit @mention via a pencil affordance (#628)
Hosts each mention as a Tiptap NodeView (mentionNodeView.ts) that renders the @displayName token (textContent — never innerHTML) plus a contenteditable=false pencil button in a fixed-width slot, revealed on whole-token hover and keyboard focus (instant opacity swap, no reflow). Activating the pencil (click or Enter/ Space) opens the single mention dropdown via the controller, anchored at the token and pre-filled with the stored displayName. commitRelink swaps ONLY personId in place via setNodeMarkup, sourcing the id solely from the selected Person — the stored displayName is preserved by construction (AC-3), even after the search input is edited (AC-5, the #380 AC-1 invariant). renderHTML/renderText stay for serialization + clipboard. AC-1/AC-2/AC-3/AC-5 + serializer round-trip covered by browser tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -9,6 +9,7 @@ import type { PersonMention } from '$lib/shared/types';
|
||||
import { deserialize, serialize } from '$lib/shared/discussion/mentionSerializer';
|
||||
import { debounce } from '$lib/shared/utils/debounce';
|
||||
import MentionDropdown from './MentionDropdown.svelte';
|
||||
import { createMentionNodeView } from './mentionNodeView';
|
||||
import { MAX_QUERY_LENGTH, SEARCH_DEBOUNCE_MS, SEARCH_RESULT_LIMIT } from './mentionConstants';
|
||||
|
||||
type Person = components['schemas']['Person'];
|
||||
@@ -190,6 +191,37 @@ function createMentionController(): MentionController {
|
||||
|
||||
const controller = createMentionController();
|
||||
|
||||
// Re-link an existing mention in place (#628 AC-3/AC-5). Captures the node's
|
||||
// pos when the pencil opens and swaps ONLY personId via setNodeMarkup, so the
|
||||
// stored displayName is preserved by construction — never round-tripped through
|
||||
// insertContentAt (which would rebind displayName to the search query). The new
|
||||
// personId comes solely from the selected Person (anti-spoof — never from the
|
||||
// reflected data-person-id or the search input).
|
||||
function commitRelink(pos: number): CommitFn {
|
||||
return (item: Person) => {
|
||||
if (!editor) return;
|
||||
editor
|
||||
.chain()
|
||||
.focus()
|
||||
.command(({ tr, state }) => {
|
||||
const node = state.doc.nodeAt(pos);
|
||||
if (!node || node.type.name !== 'mention') return false;
|
||||
tr.setNodeMarkup(pos, undefined, { ...node.attrs, personId: item.id });
|
||||
return true;
|
||||
})
|
||||
.run();
|
||||
controller.close();
|
||||
};
|
||||
}
|
||||
|
||||
// Entry point handed to the mention NodeView's pencil. Opens the one dropdown
|
||||
// (closing any other first — AC-6) anchored at the mention and pre-filled with
|
||||
// the stored displayName.
|
||||
function requestRelink(getRect: () => DOMRect | null, displayName: string, pos: number) {
|
||||
if (!editor || !editor.isEditable) return;
|
||||
controller.open(getRect, displayName, commitRelink(pos));
|
||||
}
|
||||
|
||||
onMount(() => {
|
||||
// Custom Mention node: uses personId / displayName instead of the
|
||||
// default id / label attribute names so the mentionSerializer can
|
||||
@@ -215,6 +247,12 @@ onMount(() => {
|
||||
})
|
||||
}
|
||||
};
|
||||
},
|
||||
// #628: host the mention as a NodeView so each token carries a pencil
|
||||
// re-edit affordance. renderHTML/renderText below stay for serialization
|
||||
// and clipboard; this only governs the live in-editor DOM.
|
||||
addNodeView() {
|
||||
return createMentionNodeView(requestRelink);
|
||||
}
|
||||
});
|
||||
|
||||
|
||||
@@ -771,3 +771,190 @@ describe('PersonMentionEditor — touch target', () => {
|
||||
expect(option.className).toContain('min-h-[44px]');
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #628: re-edit an existing @mention via the pencil affordance ─────────────
|
||||
|
||||
describe('PersonMentionEditor — #628 re-edit pencil', () => {
|
||||
// A saved mention seeded into the editor: text "@Aug " + sidecar. The typed
|
||||
// displayName is "Aug" (short form, #380 AC-1), distinct from the DB name.
|
||||
const SAVED = { personId: 'p-aug', displayName: 'Aug' };
|
||||
|
||||
function editPencil() {
|
||||
return page.getByRole('button', { name: m.person_mention_edit_label() });
|
||||
}
|
||||
|
||||
async function renderSavedMention() {
|
||||
const host = renderHost({ value: '@Aug ', mentionedPersons: [SAVED] });
|
||||
await expect.element(editPencil()).toBeInTheDocument();
|
||||
return host;
|
||||
}
|
||||
|
||||
async function pencilElement(): Promise<HTMLElement> {
|
||||
return (await editPencil().element()) as HTMLElement;
|
||||
}
|
||||
|
||||
function clickPencil(btn: HTMLElement) {
|
||||
// Native dispatch — CDP userEvent.click is unreliable for handlers attached
|
||||
// imperatively inside a ProseMirror NodeView (same rationale as the option
|
||||
// mousedown dispatch elsewhere in this file).
|
||||
btn.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true }));
|
||||
}
|
||||
|
||||
async function pickOption(name: RegExp) {
|
||||
const el = (await page.getByRole('option', { name }).element()) as HTMLElement;
|
||||
el.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, cancelable: true }));
|
||||
}
|
||||
|
||||
// AC-1 ----------------------------------------------------------------------
|
||||
|
||||
it('renders an edit pencil on a saved mention, labelled via aria-label', async () => {
|
||||
await renderSavedMention();
|
||||
await expect.element(editPencil()).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('the pencil is keyboard-focusable (tabindex 0, focus lands on it)', async () => {
|
||||
await renderSavedMention();
|
||||
const btn = await pencilElement();
|
||||
expect(btn.getAttribute('tabindex')).toBe('0');
|
||||
btn.focus();
|
||||
expect(document.activeElement).toBe(btn);
|
||||
});
|
||||
|
||||
it('hides the pencil by default and reveals it on hover + focus-within (opacity swap)', async () => {
|
||||
await renderSavedMention();
|
||||
const btn = await pencilElement();
|
||||
// Reveal is a class-driven instant opacity swap (no Tailwind CSS in the
|
||||
// component test env — assert the mechanism structurally, as the existing
|
||||
// touch-target test does for min-h-[44px]).
|
||||
expect(btn.className).toContain('opacity-0');
|
||||
expect(btn.className).toContain('group-hover/mention:opacity-100');
|
||||
expect(btn.className).toContain('group-focus-within/mention:opacity-100');
|
||||
});
|
||||
|
||||
it('renders the pencil in an always-present fixed-width slot (no reflow)', async () => {
|
||||
await renderSavedMention();
|
||||
const slot = document.querySelector('.mention-edit-slot') as HTMLElement | null;
|
||||
expect(slot).not.toBeNull();
|
||||
expect(slot!.className).toContain('w-4');
|
||||
});
|
||||
|
||||
// AC-2 ----------------------------------------------------------------------
|
||||
|
||||
it('opens the dropdown pre-filled with the stored displayName when the pencil is clicked', async () => {
|
||||
mockFetchEmpty();
|
||||
await renderSavedMention();
|
||||
clickPencil(await pencilElement());
|
||||
|
||||
await vi.waitFor(async () => {
|
||||
await expect.element(page.getByRole('searchbox')).toHaveValue('Aug');
|
||||
});
|
||||
});
|
||||
|
||||
it('opens the dropdown when the pencil is activated by keyboard (Enter)', async () => {
|
||||
mockFetchEmpty();
|
||||
await renderSavedMention();
|
||||
const btn = await pencilElement();
|
||||
btn.focus();
|
||||
btn.dispatchEvent(
|
||||
new KeyboardEvent('keydown', { key: 'Enter', bubbles: true, cancelable: true })
|
||||
);
|
||||
|
||||
await vi.waitFor(async () => {
|
||||
await expect.element(page.getByRole('searchbox')).toHaveValue('Aug');
|
||||
});
|
||||
});
|
||||
|
||||
// AC-3 ----------------------------------------------------------------------
|
||||
|
||||
it('relinks in place: picking a different person swaps data-person-id', async () => {
|
||||
mockFetchWithPersons(); // AUGUSTE (p-aug) + ANNA (p-anna)
|
||||
await renderSavedMention();
|
||||
clickPencil(await pencilElement());
|
||||
|
||||
await vi.waitFor(async () => {
|
||||
await expect.element(page.getByRole('option', { name: /Anna Schmidt/ })).toBeVisible();
|
||||
});
|
||||
await pickOption(/Anna Schmidt/);
|
||||
|
||||
await vi.waitFor(() => {
|
||||
const token = document.querySelector('[data-type="mention"]') as HTMLElement;
|
||||
expect(token.getAttribute('data-person-id')).toBe('p-anna');
|
||||
});
|
||||
});
|
||||
|
||||
it('relink preserves the displayed token text exactly (only personId changes)', async () => {
|
||||
mockFetchWithPersons();
|
||||
const host = await renderSavedMention();
|
||||
clickPencil(await pencilElement());
|
||||
|
||||
await vi.waitFor(async () => {
|
||||
await expect.element(page.getByRole('option', { name: /Anna Schmidt/ })).toBeVisible();
|
||||
});
|
||||
await pickOption(/Anna Schmidt/);
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(host.snapshot.mentionedPersons).toEqual([{ personId: 'p-anna', displayName: 'Aug' }]);
|
||||
});
|
||||
});
|
||||
|
||||
// Serializer regression — text byte-identical, only personId swapped --------
|
||||
|
||||
it('serializer regression: re-link keeps the text byte-identical, swaps only personId', async () => {
|
||||
mockFetchWithPersons();
|
||||
const host = await renderSavedMention();
|
||||
clickPencil(await pencilElement());
|
||||
|
||||
await vi.waitFor(async () => {
|
||||
await expect.element(page.getByRole('option', { name: /Anna Schmidt/ })).toBeVisible();
|
||||
});
|
||||
await pickOption(/Anna Schmidt/);
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(host.snapshot.value).toBe('@Aug ');
|
||||
expect(host.snapshot.mentionedPersons).toEqual([{ personId: 'p-anna', displayName: 'Aug' }]);
|
||||
});
|
||||
});
|
||||
|
||||
// AC-5 — edited-then-picked (highest value): personId updates, text invariant
|
||||
|
||||
it('AC-5: editing the search input then picking sets the new personId', async () => {
|
||||
mockFetchWithPersons();
|
||||
const host = await renderSavedMention();
|
||||
clickPencil(await pencilElement());
|
||||
await vi.waitFor(async () => {
|
||||
await expect.element(page.getByRole('searchbox')).toBeVisible();
|
||||
});
|
||||
|
||||
await page.getByRole('searchbox').fill('Anna');
|
||||
await vi.waitFor(async () => {
|
||||
await expect.element(page.getByRole('option', { name: /Anna Schmidt/ })).toBeVisible();
|
||||
});
|
||||
await pickOption(/Anna Schmidt/);
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(host.snapshot.mentionedPersons[0].personId).toBe('p-anna');
|
||||
});
|
||||
});
|
||||
|
||||
it('AC-5: editing the search input then picking keeps the ORIGINAL displayName', async () => {
|
||||
mockFetchWithPersons();
|
||||
const host = await renderSavedMention();
|
||||
clickPencil(await pencilElement());
|
||||
await vi.waitFor(async () => {
|
||||
await expect.element(page.getByRole('searchbox')).toBeVisible();
|
||||
});
|
||||
|
||||
await page.getByRole('searchbox').fill('Anna');
|
||||
await vi.waitFor(async () => {
|
||||
await expect.element(page.getByRole('option', { name: /Anna Schmidt/ })).toBeVisible();
|
||||
});
|
||||
await pickOption(/Anna Schmidt/);
|
||||
|
||||
await vi.waitFor(() => {
|
||||
// The #380 AC-1 invariant wins over the edited search input.
|
||||
expect(host.snapshot.mentionedPersons[0].displayName).toBe('Aug');
|
||||
expect(host.snapshot.value).toBe('@Aug ');
|
||||
expect(host.snapshot.value).not.toContain('@Anna');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
152
frontend/src/lib/shared/discussion/mentionNodeView.ts
Normal file
152
frontend/src/lib/shared/discussion/mentionNodeView.ts
Normal file
@@ -0,0 +1,152 @@
|
||||
import type { Editor } from '@tiptap/core';
|
||||
import type { Node as ProseMirrorNode } from '@tiptap/pm/model';
|
||||
import { m } from '$lib/paraglide/messages.js';
|
||||
|
||||
const SVG_NS = 'http://www.w3.org/2000/svg';
|
||||
|
||||
/**
|
||||
* Opens the re-edit dropdown for a saved @mention. The host editor supplies
|
||||
* this; the NodeView calls it when the pencil is activated.
|
||||
*
|
||||
* @param getRect anchor for the dropdown — the mention token's on-screen rect
|
||||
* @param displayName the stored display text, used to pre-fill the search input
|
||||
* @param pos the mention node's document position, captured at open time
|
||||
* so the eventual re-link targets exactly this node
|
||||
*/
|
||||
export type RelinkRequest = (
|
||||
getRect: () => DOMRect | null,
|
||||
displayName: string,
|
||||
pos: number
|
||||
) => void;
|
||||
|
||||
type NodeViewArgs = {
|
||||
node: ProseMirrorNode;
|
||||
editor: Editor;
|
||||
getPos: () => number | undefined;
|
||||
};
|
||||
|
||||
// Static developer markup — no user data reaches the DOM here, so building the
|
||||
// glyph element-by-element (never innerHTML) keeps the "user text is textContent
|
||||
// only" rule honest across the whole NodeView.
|
||||
function buildPencilIcon(): SVGSVGElement {
|
||||
const svg = document.createElementNS(SVG_NS, 'svg');
|
||||
svg.setAttribute('viewBox', '0 0 24 24');
|
||||
svg.setAttribute('fill', 'none');
|
||||
svg.setAttribute('stroke', 'currentColor');
|
||||
svg.setAttribute('stroke-width', '2');
|
||||
svg.setAttribute('aria-hidden', 'true');
|
||||
svg.setAttribute('class', 'h-4 w-4');
|
||||
const tip = document.createElementNS(SVG_NS, 'path');
|
||||
tip.setAttribute('d', 'M12 20h9');
|
||||
tip.setAttribute('stroke-linecap', 'round');
|
||||
const body = document.createElementNS(SVG_NS, 'path');
|
||||
body.setAttribute('d', 'M16.5 3.5a2.12 2.12 0 0 1 3 3L7 19l-4 1 1-4 12.5-12.5z');
|
||||
body.setAttribute('stroke-linejoin', 'round');
|
||||
svg.append(tip, body);
|
||||
return svg;
|
||||
}
|
||||
|
||||
/**
|
||||
* Tiptap NodeView for a person @mention. Renders the `@displayName` token text
|
||||
* (as `textContent` — the name is user-influenced, never `innerHTML`) plus a
|
||||
* `contenteditable="false"` pencil button that opens the re-edit dropdown (#628).
|
||||
*
|
||||
* The pencil sits in a fixed-width slot revealed on whole-token hover and
|
||||
* keyboard focus — an instant opacity swap (respects prefers-reduced-motion),
|
||||
* so following words never shift and the caret model stays stable.
|
||||
*/
|
||||
export function createMentionNodeView(requestRelink: RelinkRequest) {
|
||||
return ({ node, editor, getPos }: NodeViewArgs) => {
|
||||
const dom = document.createElement('span');
|
||||
dom.className = 'mention-token group/mention';
|
||||
dom.setAttribute('data-type', 'mention');
|
||||
dom.setAttribute('data-person-id', node.attrs.personId ?? '');
|
||||
dom.setAttribute('data-display-name', node.attrs.displayName ?? '');
|
||||
|
||||
const text = document.createElement('span');
|
||||
text.className = 'underline decoration-ink/50 underline-offset-2 text-brand-navy font-medium';
|
||||
text.textContent = `@${node.attrs.displayName}`;
|
||||
dom.appendChild(text);
|
||||
|
||||
// Fixed-width slot — always present so revealing the pencil never reflows
|
||||
// the following text (NFR no-reflow). Only opacity changes on reveal.
|
||||
const slot = document.createElement('span');
|
||||
slot.className =
|
||||
'mention-edit-slot ml-0.5 inline-flex w-4 shrink-0 items-center justify-center align-middle';
|
||||
|
||||
const button = document.createElement('button');
|
||||
button.type = 'button';
|
||||
button.contentEditable = 'false';
|
||||
button.tabIndex = 0;
|
||||
button.setAttribute('aria-label', m.person_mention_edit_label());
|
||||
// Visible glyph stays ~16px; the 44px tap target comes from padding pulled
|
||||
// back with negative margin so the larger hit box does not reflow the line.
|
||||
button.className = [
|
||||
'mention-edit-btn',
|
||||
'-mx-3 -my-2 inline-flex h-11 w-11 items-center justify-center rounded-sm text-ink-2',
|
||||
'opacity-0 transition-none',
|
||||
'group-hover/mention:opacity-100 group-focus-within/mention:opacity-100 focus:opacity-100',
|
||||
'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-brand-navy',
|
||||
'disabled:cursor-not-allowed'
|
||||
].join(' ');
|
||||
button.appendChild(buildPencilIcon());
|
||||
slot.appendChild(button);
|
||||
dom.appendChild(slot);
|
||||
|
||||
const openRelink = (event: Event) => {
|
||||
event.preventDefault();
|
||||
event.stopPropagation();
|
||||
// AC-7: inert when the editor is disabled — independent of the wrapper's
|
||||
// pointer-events-none, on both the pointer and the keyboard path.
|
||||
if (!editor.isEditable) return;
|
||||
const pos = getPos();
|
||||
if (pos === undefined) return;
|
||||
requestRelink(() => text.getBoundingClientRect(), node.attrs.displayName ?? '', pos);
|
||||
};
|
||||
|
||||
// Prevent mousedown from moving the editor selection before the click
|
||||
// fires — keeps the captured pos valid. click + Enter/Space activate.
|
||||
button.addEventListener('mousedown', (event) => event.preventDefault());
|
||||
button.addEventListener('click', openRelink);
|
||||
button.addEventListener('keydown', (event) => {
|
||||
if (event.key === 'Enter' || event.key === ' ') openRelink(event);
|
||||
});
|
||||
|
||||
const syncEditable = () => {
|
||||
const editable = editor.isEditable;
|
||||
button.disabled = !editable;
|
||||
if (editable) {
|
||||
button.removeAttribute('aria-disabled');
|
||||
button.tabIndex = 0;
|
||||
} else {
|
||||
button.setAttribute('aria-disabled', 'true');
|
||||
button.tabIndex = -1;
|
||||
}
|
||||
};
|
||||
syncEditable();
|
||||
// setEditable (and any doc change) dispatches a transaction — re-sync the
|
||||
// pencil's inert state so a mid-session disable flip takes effect.
|
||||
editor.on('transaction', syncEditable);
|
||||
|
||||
return {
|
||||
dom,
|
||||
// The mention is an atom leaf — its DOM is fully owned here, so PM must
|
||||
// ignore our mutations (token textContent / disabled attr) rather than
|
||||
// trying to read them back into the document.
|
||||
ignoreMutation: () => true,
|
||||
// Pencil events are ours; everything else (caret placement on the token)
|
||||
// flows to ProseMirror.
|
||||
stopEvent: (event: Event) => button.contains(event.target as Node),
|
||||
update(updatedNode: ProseMirrorNode) {
|
||||
if (updatedNode.type.name !== 'mention') return false;
|
||||
text.textContent = `@${updatedNode.attrs.displayName}`;
|
||||
dom.setAttribute('data-person-id', updatedNode.attrs.personId ?? '');
|
||||
dom.setAttribute('data-display-name', updatedNode.attrs.displayName ?? '');
|
||||
return true;
|
||||
},
|
||||
destroy() {
|
||||
editor.off('transaction', syncEditable);
|
||||
}
|
||||
};
|
||||
};
|
||||
}
|
||||
Reference in New Issue
Block a user