From a62a333d09d2bd25a3a25311618aa6b5a5005653 Mon Sep 17 00:00:00 2001
From: Marcel <marcel@familienarchiv>
Date: Tue, 2 Jun 2026 20:08:43 +0200
Subject: [PATCH] fix(transcription): harden re-edit pencil hit-testing +
 disable sync (#628 review)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Addresses the clean-agent review of PR #717:

- C1: the hidden pencil was opacity-0 only, which still hit-tests; its 44px box
  overhangs adjacent text, so a click in the gap between two mentions could land
  on the invisible button and spuriously open the dropdown (AC-8 hole). Add
  pointer-events-none while hidden, re-enabled with the opacity reveal on
  hover/focus.
- C2/N1: editor.setEditable() emits "update", not a ProseMirror transaction, so
  the NodeView's 'transaction' listener missed a mid-session disable flip (stale
  aria-disabled/tabindex; the comment was wrong). Listen on 'update' instead —
  which also skips selection-only changes, so it fires far less often.
- N2: track the node across update() so the pencil opens with the live
  displayName (hardening; relink only swaps personId today).

Tests: structural guard that the hidden pencil is pointer-events-none + reveals,
and a mid-session disable-flip test (fixture gains an onReady setDisabled hook).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 .../PersonMentionEditor.svelte.spec.ts        | 51 +++++++++++++++++++
 .../PersonMentionEditor.test-fixture.svelte   | 15 ++++--
 .../lib/shared/discussion/mentionNodeView.ts  | 29 ++++++++---
 3 files changed, 85 insertions(+), 10 deletions(-)

diff --git a/frontend/src/lib/shared/discussion/PersonMentionEditor.svelte.spec.ts b/frontend/src/lib/shared/discussion/PersonMentionEditor.svelte.spec.ts
index 195da335..ae15e31d 100644
--- a/frontend/src/lib/shared/discussion/PersonMentionEditor.svelte.spec.ts
+++ b/frontend/src/lib/shared/discussion/PersonMentionEditor.svelte.spec.ts
@@ -1333,3 +1333,54 @@ describe('PersonMentionEditor — #628 editing announce', () => {
 		});
 	});
 });
+
+// ─── #628 review fixes: hidden-pencil hit-testing + mid-session disable ───────
+
+describe('PersonMentionEditor — #628 review fixes', () => {
+	it('keeps the hidden pencil out of hit-testing (pointer-events-none until revealed)', async () => {
+		// opacity-0 alone still hit-tests, and the 44px button overhangs adjacent
+		// text — so a click in the gap between mentions would otherwise land on the
+		// invisible pencil and open the dropdown (AC-8). It must be pointer-events-none
+		// while hidden and only become clickable together with the opacity reveal.
+		renderHost({ value: '@Aug ', mentionedPersons: [{ personId: 'p-aug', displayName: 'Aug' }] });
+		await vi.waitFor(() =>
+			expect(document.querySelector('[data-type="mention"] button')).not.toBeNull()
+		);
+		const btn = document.querySelector('[data-type="mention"] button') as HTMLElement;
+		expect(btn.className).toContain('pointer-events-none');
+		expect(btn.className).toContain('group-hover/mention:pointer-events-auto');
+		expect(btn.className).toContain('group-focus-within/mention:pointer-events-auto');
+	});
+
+	it('re-syncs the pencil to inert when the editor is disabled mid-session', async () => {
+		// editor.setEditable() emits "update", not a transaction — the NodeView must
+		// listen on "update" or a runtime disable flip leaves the pencil stale.
+		let setDisabled!: (value: boolean) => void;
+		render(PersonMentionEditorHost, {
+			initialValue: '@Aug ',
+			initialMentions: [{ personId: 'p-aug', displayName: 'Aug' }],
+			disabled: false,
+			onChange: () => {},
+			onReady: (api: { setDisabled: (value: boolean) => void }) => {
+				setDisabled = api.setDisabled;
+			}
+		});
+
+		await vi.waitFor(() => {
+			const btn = document.querySelector(
+				'[data-type="mention"] button'
+			) as HTMLButtonElement | null;
+			expect(btn).not.toBeNull();
+			expect(btn!.disabled).toBe(false);
+		});
+
+		setDisabled(true);
+
+		await vi.waitFor(() => {
+			const btn = document.querySelector('[data-type="mention"] button') as HTMLButtonElement;
+			expect(btn.disabled).toBe(true);
+			expect(btn.getAttribute('aria-disabled')).toBe('true');
+			expect(btn.tabIndex).toBe(-1);
+		});
+	});
+});
diff --git a/frontend/src/lib/shared/discussion/PersonMentionEditor.test-fixture.svelte b/frontend/src/lib/shared/discussion/PersonMentionEditor.test-fixture.svelte
index cdc31df0..d59daa42 100644
--- a/frontend/src/lib/shared/discussion/PersonMentionEditor.test-fixture.svelte
+++ b/frontend/src/lib/shared/discussion/PersonMentionEditor.test-fixture.svelte
@@ -1,5 +1,5 @@
 <script lang="ts">
-import { untrack } from 'svelte';
+import { onMount, untrack } from 'svelte';
 import PersonMentionEditor from './PersonMentionEditor.svelte';
 import type { components } from '$lib/generated/api';
 
@@ -11,6 +11,9 @@ type Props = {
 	placeholder?: string;
 	disabled?: boolean;
 	onChange: (snapshot: { value: string; mentionedPersons: PersonMention[] }) => void;
+	/** Hands the test a setter so it can flip `disabled` after mount — used to
+	 *  exercise the editor's reactive `disabled` $effect (mid-session toggle). */
+	onReady?: (api: { setDisabled: (value: boolean) => void }) => void;
 };
 
 let {
@@ -18,13 +21,19 @@ let {
 	initialMentions = [],
 	placeholder,
 	disabled = false,
-	onChange
+	onChange,
+	onReady
 }: Props = $props();
 
 // initial* props seed mount-time state; reading them inside untrack signals
 // the intentional one-shot capture and silences state_referenced_locally.
 let value = $state(untrack(() => initialValue));
 let mentionedPersons = $state<PersonMention[]>(untrack(() => [...initialMentions]));
+let disabledState = $state(untrack(() => disabled));
+
+onMount(() => {
+	onReady?.({ setDisabled: (next: boolean) => (disabledState = next) });
+});
 
 $effect(() => {
 	onChange({ value, mentionedPersons: [...mentionedPersons] });
@@ -35,5 +44,5 @@ $effect(() => {
 	bind:value={value}
 	bind:mentionedPersons={mentionedPersons}
 	placeholder={placeholder}
-	disabled={disabled}
+	disabled={disabledState}
 />
diff --git a/frontend/src/lib/shared/discussion/mentionNodeView.ts b/frontend/src/lib/shared/discussion/mentionNodeView.ts
index 733dc8d5..eb6d658d 100644
--- a/frontend/src/lib/shared/discussion/mentionNodeView.ts
+++ b/frontend/src/lib/shared/discussion/mentionNodeView.ts
@@ -57,6 +57,11 @@ function buildPencilIcon(): SVGSVGElement {
  */
 export function createMentionNodeView(requestRelink: RelinkRequest) {
 	return ({ node, editor, getPos }: NodeViewArgs) => {
+		// Tracks the node's current attrs across update()s so the pencil always
+		// opens with the live displayName (relink only swaps personId today, but
+		// this keeps openRelink honest if displayName ever becomes mutable).
+		let currentNode = node;
+
 		const dom = document.createElement('span');
 		dom.className = 'mention-token group/mention';
 		dom.setAttribute('data-type', 'mention');
@@ -81,11 +86,18 @@ export function createMentionNodeView(requestRelink: RelinkRequest) {
 		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.
+		// While hidden the button must be pointer-events-none — opacity-0 alone
+		// still hit-tests, and the 44px box overhangs the adjacent text, so a click
+		// in the gap between two mentions would otherwise land on this invisible
+		// button and spuriously open the dropdown (AC-8). Pointer events are
+		// re-enabled together with the opacity reveal on hover/focus.
 		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',
+			'pointer-events-none opacity-0 transition-none',
+			'group-hover/mention:pointer-events-auto group-hover/mention:opacity-100',
+			'group-focus-within/mention:pointer-events-auto group-focus-within/mention:opacity-100',
+			'focus:pointer-events-auto focus:opacity-100',
 			'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-brand-navy',
 			'disabled:cursor-not-allowed'
 		].join(' ');
@@ -101,7 +113,7 @@ export function createMentionNodeView(requestRelink: RelinkRequest) {
 			if (!editor.isEditable) return;
 			const pos = getPos();
 			if (pos === undefined) return;
-			requestRelink(() => text.getBoundingClientRect(), node.attrs.displayName ?? '', pos);
+			requestRelink(() => text.getBoundingClientRect(), currentNode.attrs.displayName ?? '', pos);
 		};
 
 		// Prevent mousedown from moving the editor selection before the click
@@ -124,9 +136,11 @@ export function createMentionNodeView(requestRelink: RelinkRequest) {
 			}
 		};
 		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);
+		// editor.setEditable() emits "update" (not a ProseMirror transaction), so
+		// we listen on "update" to re-sync the pencil's inert state when `disabled`
+		// flips mid-session. "update" also skips selection-only changes, so this
+		// fires far less often than a "transaction" listener would.
+		editor.on('update', syncEditable);
 
 		return {
 			dom,
@@ -139,13 +153,14 @@ export function createMentionNodeView(requestRelink: RelinkRequest) {
 			stopEvent: (event: Event) => button.contains(event.target as Node),
 			update(updatedNode: ProseMirrorNode) {
 				if (updatedNode.type.name !== 'mention') return false;
+				currentNode = updatedNode;
 				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);
+				editor.off('update', syncEditable);
 			}
 		};
 	};