From afc3afcdd53af3d4b65aabf53d54ebebf59a9c0d Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 26 Aug 2026 17:47:33 +1000 Subject: [PATCH 1/2] fix(desktop): keep the draft space when typing right after a mention pick Desktop Smoke E2E (2) failed on mentions.spec.ts ("typing a mention before existing text does not interleave spaces"): picking a suggestion mid-draft and typing immediately could swallow the draft's space ("hello @bob abcworld"). Chromium sometimes rewrites the whole whitespace run around the caret as replace(" " -> "\u00A0a"), and the settling-time text-input redirect only recognized a caret or a selected trailing space, so the rewrite fell through to the destructive default replacement. Teach the redirect to recognize a space-run rewrite (space or NBSP) and insert only the typed remainder after the mention's trailing space. The race pre-dates this branch (reproduces on main at ~5-25% per local run); 300+ consecutive passes after the fix. (cherry picked from commit 05302ca879197cbe7ab1abcdffc51772f7071fe8) Co-Authored-By: Claude Fable 5 Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Matt Toohey --- .../lib/mentionHighlightExtension.test.mjs | 55 +++++++++++++----- .../messages/lib/mentionHighlightExtension.ts | 57 +++++++++++++------ 2 files changed, 81 insertions(+), 31 deletions(-) diff --git a/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs b/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs index 6c7055c20d..b01ce65929 100644 --- a/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs +++ b/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs @@ -10,9 +10,9 @@ import { buildHighlightPatterns, createMentionCaretSettlement, findHighlightMatches, - insertPosForMentionTextInput, + insertionForMentionTextInput, MentionHighlightExtension, - mentionTextInputInsertPos, + mentionTextInputInsertion, positionAfterArrowLeftThroughMentionSpace, selectionAfterMentionTrailingSpace, shouldAdvanceMentionCaret, @@ -252,35 +252,60 @@ test("createMentionCaretSettlement keeps two editors independent", () => { assert.equal(composerB.peek(), 12); }); -test("insertPosForMentionTextInput redirects a caret at the chip edge", () => { +test("insertionForMentionTextInput redirects a caret at the chip edge", () => { const doc = document(paragraph(text("@quinn "))); const spacePos = 1 + "@quinn".length; + assert.deepEqual(insertionForMentionTextInput(doc, spacePos, spacePos, "x"), { + insertAt: spacePos + 1, + text: "x", + }); assert.equal( - insertPosForMentionTextInput(doc, spacePos, spacePos), - spacePos + 1, - ); - assert.equal( - insertPosForMentionTextInput(doc, spacePos + 1, spacePos + 1), + insertionForMentionTextInput(doc, spacePos + 1, spacePos + 1, "x"), null, ); }); -test("insertPosForMentionTextInput keeps a selected trailing space", () => { +test("insertionForMentionTextInput keeps a selected trailing space", () => { const doc = document(paragraph(text("@quinn "))); const spacePos = 1 + "@quinn".length; + assert.deepEqual( + insertionForMentionTextInput(doc, spacePos, spacePos + 1, "x"), + { insertAt: spacePos + 1, text: "x" }, + ); +}); + +test("insertionForMentionTextInput keeps the draft space in a whitespace-run rewrite", () => { + // Chromium can rewrite the whole space run when typing between the + // mention's trailing space and a pre-existing draft space, emitting + // replace(" " → " a") — usually with a non-breaking space. The draft's + // space must survive either way. + const doc = document(paragraph(text("hello @bob world"))); + const spacePos = 1 + "hello @bob".length; + assert.deepEqual( + insertionForMentionTextInput(doc, spacePos, spacePos + 2, " a"), + { insertAt: spacePos + 1, text: "a" }, + ); + assert.deepEqual( + insertionForMentionTextInput(doc, spacePos, spacePos + 2, "\u00A0a"), + { insertAt: spacePos + 1, text: "a" }, + ); + // A rewrite that is not space-led is a real replacement — leave it alone. assert.equal( - insertPosForMentionTextInput(doc, spacePos, spacePos + 1), - spacePos + 1, + insertionForMentionTextInput(doc, spacePos, spacePos + 2, "x"), + null, ); }); -test("mentionTextInputInsertPos honors a deliberate caret after settlement", () => { +test("mentionTextInputInsertion honors a deliberate caret after settlement", () => { const doc = document(paragraph(text("@bob "))); const spacePos = 1 + "@bob".length; - assert.equal(mentionTextInputInsertPos(doc, spacePos, spacePos, false), null); assert.equal( - mentionTextInputInsertPos(doc, spacePos, spacePos, true), - spacePos + 1, + mentionTextInputInsertion(doc, spacePos, spacePos, "x", false), + null, + ); + assert.deepEqual( + mentionTextInputInsertion(doc, spacePos, spacePos, "x", true), + { insertAt: spacePos + 1, text: "x" }, ); }); diff --git a/desktop/src/features/messages/lib/mentionHighlightExtension.ts b/desktop/src/features/messages/lib/mentionHighlightExtension.ts index 3d1d5d6ef7..56b6615b78 100644 --- a/desktop/src/features/messages/lib/mentionHighlightExtension.ts +++ b/desktop/src/features/messages/lib/mentionHighlightExtension.ts @@ -58,23 +58,43 @@ export function shouldAdvanceMentionCaret({ return next !== from && settling; } +export type MentionTextInsertion = { + insertAt: number; + text: string; +}; + /** - * Where to insert typed text when the caret (or a one-character selection) - * sits on the trailing space after an `@name` / `#channel` token. - * A selected trailing space would otherwise be replaced, producing - * `@bobhello`. + * Where (and what) to insert when typed text arrives at the trailing space + * after an `@name` / `#channel` token. + * + * - Caret on the space: insert after it, so the next keystroke lands after + * the token (`@bobhello` fix). + * - One-character selection of the space: insert after it — replacing the + * selected space would produce `@bobhello`. + * - Whitespace-run rewrite: when typing between the mention's trailing + * space and a pre-existing draft space, Chromium may re-emit the whole + * run as `replace(" " → " a")` — usually with a non-breaking space. + * Applying that verbatim deletes the draft's space + * (`hello @bob abcworld`), so keep the document's spaces and insert only + * the typed remainder after the trailing space. */ -export function insertPosForMentionTextInput( +export function insertionForMentionTextInput( doc: ProseMirrorNode, from: number, to: number, -): number | null { + text: string, +): MentionTextInsertion | null { const next = selectionAfterMentionTrailingSpace(doc, from); if (from === to) { - return next === from ? null : next; + return next === from ? null : { insertAt: next, text }; } - if (to === next && next === from + 1) { - return next; + if (next !== from + 1) return null; + if (to === next) { + return { insertAt: next, text }; + } + const replaced = doc.textBetween(from, to, "\n", "\0"); + if (/^[ \u00A0]+$/.test(replaced) && /^[ \u00A0]/.test(text)) { + return { insertAt: next, text: text.replace(/^[ \u00A0]+/, "") }; } return null; } @@ -84,14 +104,15 @@ export function insertPosForMentionTextInput( * deliberate ArrowLeft or chip click, honor the caret so `x` lands in the * token (`@bobx`) instead of after the space (`@bob x`). */ -export function mentionTextInputInsertPos( +export function mentionTextInputInsertion( doc: ProseMirrorNode, from: number, to: number, + text: string, settling: boolean, -): number | null { +): MentionTextInsertion | null { if (!settling) return null; - return insertPosForMentionTextInput(doc, from, to); + return insertionForMentionTextInput(doc, from, to, text); } /** Caret just after a mention trailing space: ArrowLeft lands on the token end. */ @@ -396,18 +417,22 @@ export const MentionHighlightExtension = Extension.create({ return this.getState(state) ?? DecorationSet.empty; }, handleTextInput(view, from, to, text) { - const insertAt = mentionTextInputInsertPos( + const insertion = mentionTextInputInsertion( view.state.doc, from, to, + text, settlement.peek() !== null, ); - if (insertAt == null) { + if (insertion == null) { settlement.cancel(); return false; } - const tr = view.state.tr.insertText(text, insertAt); - const caret = tr.mapping.map(insertAt, 1); + const tr = view.state.tr.insertText( + insertion.text, + insertion.insertAt, + ); + const caret = tr.mapping.map(insertion.insertAt, 1); tr.setSelection(TextSelection.create(tr.doc, caret)); view.dispatch(tr); settlement.cancel(); From 272257967f67f6cf80653fa23e35ff0e40c07acc Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 26 Aug 2026 21:35:51 +1000 Subject: [PATCH 2/2] fix(desktop): keep the draft space for either whitespace-run anchor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review 5028139009 on PR #6850 reproduced "hello @bob abcworld" at the head that was meant to fix it. It was right, and the gap is deterministic rather than a stale-build artifact: the redirect only recognized a rewrite anchored at the chip edge. Chromium also anchors it one position later, replacing just the draft's own space, and selectionAfterMentionTrailingSpace returns `from` unchanged there (its lookbehind sees "hello @bob " ending in a space), so the `next !== from + 1` guard bailed and the destructive default ate the separator, giving "hello @bob aworld" and then "hello @bob abcworld". Replace the shape matching with the invariant it was approximating. While autocomplete is settling the user cannot have selected anything — a selection cancels settlement — so a whitespace-only replacement next to a mention's trailing space is always the browser normalizing whitespace, never an intentional delete. Keep the document's spaces and insert only the typed characters after the token, whichever edge the rewrite is anchored at. The evidence is now deterministic instead of racy. The new unit test drives the plugin's real handleTextInput prop through all four anchors and falls back to ProseMirror's default when it declines, exactly as the browser does with the return value; against the previous production code it fails with the reviewed value ("hello @bob aworld"). Browser runs: mentions.spec.ts 73/73, messaging.spec.ts 91/91, and the reviewer's parallel --repeat-each repro 85/85 — against a rebuilt bundle after evicting a stale port-4173 server that was serving another worktree's dist, the known confound in this harness. This branch also carries out the review's primary ask: the composer fix now lands on its own instead of riding the sidebar-geometry PR. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Matt Toohey --- .../lib/mentionHighlightExtension.test.mjs | 114 +++++++++++++++++- .../messages/lib/mentionHighlightExtension.ts | 62 +++++++--- 2 files changed, 152 insertions(+), 24 deletions(-) diff --git a/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs b/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs index b01ce65929..106b036934 100644 --- a/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs +++ b/desktop/src/features/messages/lib/mentionHighlightExtension.test.mjs @@ -12,6 +12,7 @@ import { findHighlightMatches, insertionForMentionTextInput, MentionHighlightExtension, + mentionHighlightKey, mentionTextInputInsertion, positionAfterArrowLeftThroughMentionSpace, selectionAfterMentionTrailingSpace, @@ -277,21 +278,40 @@ test("insertionForMentionTextInput keeps a selected trailing space", () => { test("insertionForMentionTextInput keeps the draft space in a whitespace-run rewrite", () => { // Chromium can rewrite the whole space run when typing between the // mention's trailing space and a pre-existing draft space, emitting - // replace(" " → " a") — usually with a non-breaking space. The draft's - // space must survive either way. + // replace(" " -> " a") — usually with a non-breaking space, and anchored + // at either edge of the run. The draft's space must survive every shape. const doc = document(paragraph(text("hello @bob world"))); const spacePos = 1 + "hello @bob".length; + const kept = { insertAt: spacePos + 1, text: "a" }; + // Anchored at the chip edge, replacing the whole run. assert.deepEqual( insertionForMentionTextInput(doc, spacePos, spacePos + 2, " a"), - { insertAt: spacePos + 1, text: "a" }, + kept, ); assert.deepEqual( insertionForMentionTextInput(doc, spacePos, spacePos + 2, "\u00A0a"), - { insertAt: spacePos + 1, text: "a" }, + kept, ); - // A rewrite that is not space-led is a real replacement — leave it alone. - assert.equal( + // Anchored past the trailing space, rewriting only the draft's own space. + // This shape used to fall through to the destructive default and produce + // "hello @bob abcworld" — the failure CI caught. + assert.deepEqual( + insertionForMentionTextInput(doc, spacePos + 1, spacePos + 2, "\u00A0a"), + kept, + ); + // Trailing whitespace is the run being re-emitted on the other side. + assert.deepEqual( + insertionForMentionTextInput(doc, spacePos, spacePos + 2, "a\u00A0"), + kept, + ); + // Whatever the shape, replaced whitespace is never dropped while settling. + assert.deepEqual( insertionForMentionTextInput(doc, spacePos, spacePos + 2, "x"), + { insertAt: spacePos + 1, text: "x" }, + ); + // Replacing something other than whitespace is a real edit — leave it. + assert.equal( + insertionForMentionTextInput(doc, spacePos, spacePos + 3, " a"), null, ); }); @@ -403,3 +423,85 @@ test("typing after a completed mention keeps the separator intact", () => { const typed = typeAt(state, 1 + "@quinn world".length, "!"); assert.equal(typed.doc.textContent, "@quinn world!"); }); + +// ── the browser branch: Chromium's whitespace-run rewrite ───────────── +// +// The helper tests above model the payload. These drive the plugin's real +// `handleTextInput` prop and fall back to ProseMirror's default insertion +// when it declines, exactly as the browser does with the return value — so +// the assertion is sensitive to the production branch itself rather than to +// winning a timing race in a headless browser. + +/** Apply an autocomplete pick: replace the typed token and settle the caret. */ +function pickMentionAt(state, tokenFrom, tokenTo, inserted) { + const tr = state.tr.insertText(inserted, tokenFrom, tokenTo); + tr.setSelection(TextSelection.create(tr.doc, tokenFrom + inserted.length)); + tr.setMeta(mentionHighlightKey, true); + return state.apply(tr); +} + +/** Route a text-input event through the plugin, then the default handling. */ +function textInput(state, from, to, text) { + let current = state; + const view = { + get state() { + return current; + }, + dispatch(tr) { + current = current.apply(tr); + }, + domAtPos: () => ({ node: {}, offset: 0 }), + root: undefined, + }; + const handled = current.plugins.some( + (plugin) => plugin.props?.handleTextInput?.(view, from, to, text) === true, + ); + return handled + ? current + : current.apply(current.tr.insertText(text, from, to)); +} + +test("a whitespace-run rewrite after a mention pick keeps the draft space", () => { + // The CI failure: with "hello world" drafted, the caret placed after + // "hello", " @bo" typed and the "bob" suggestion picked, the document is + // "hello @bob world" — the mention's trailing space followed by the + // draft's own space. The next keystroke arrives as one of these shapes + // depending on how Chromium reconciles that whitespace run, and all of + // them have to keep both spaces. + const tokenFrom = 1 + "hello ".length; + const spacePos = 1 + "hello @bob".length; + const shapes = [ + { name: "caret at the chip edge", from: spacePos, to: spacePos, text: "a" }, + { + name: "caret past the trailing space", + from: spacePos + 1, + to: spacePos + 1, + text: "a", + }, + { + name: "run rewritten from the chip edge", + from: spacePos, + to: spacePos + 2, + text: "\u00A0a", + }, + { + name: "draft space rewritten on its own", + from: spacePos + 1, + to: spacePos + 2, + text: "\u00A0a", + }, + ]; + + for (const shape of shapes) { + // A fresh editor per shape: settlement is per-plugin closure state. + const picked = pickMentionAt( + editorStateWithMentionHighlight("hello @bo world", ["bob"]), + tokenFrom, + tokenFrom + "@bo".length, + "@bob ", + ); + assert.equal(picked.doc.textContent, "hello @bob world", shape.name); + const typed = textInput(picked, shape.from, shape.to, shape.text); + assert.equal(typed.doc.textContent, "hello @bob a world", shape.name); + } +}); diff --git a/desktop/src/features/messages/lib/mentionHighlightExtension.ts b/desktop/src/features/messages/lib/mentionHighlightExtension.ts index 56b6615b78..e55c79c9a3 100644 --- a/desktop/src/features/messages/lib/mentionHighlightExtension.ts +++ b/desktop/src/features/messages/lib/mentionHighlightExtension.ts @@ -63,20 +63,51 @@ export type MentionTextInsertion = { text: string; }; +const SPACE_RUN = /^[ \u00A0]+$/; +const OUTER_SPACES = /^[ \u00A0]+|[ \u00A0]+$/g; + +/** + * Position just after the trailing space of the mention token that `pos` is + * adjacent to, or `null` when `pos` is nowhere near one. + * + * `pos` may sit at the token end (before the space) or already past the + * space: when Chromium rewrites the whitespace run around the caret it + * anchors the replacement at either edge, and both mean the same boundary. + */ +function mentionTrailingSpaceBoundary( + doc: ProseMirrorNode, + pos: number, +): number | null { + const afterSpace = selectionAfterMentionTrailingSpace(doc, pos); + if (afterSpace !== pos) return afterSpace; + if (pos > 0 && selectionAfterMentionTrailingSpace(doc, pos - 1) === pos) { + return pos; + } + return null; +} + /** * Where (and what) to insert when typed text arrives at the trailing space * after an `@name` / `#channel` token. * * - Caret on the space: insert after it, so the next keystroke lands after * the token (`@bobhello` fix). - * - One-character selection of the space: insert after it — replacing the - * selected space would produce `@bobhello`. - * - Whitespace-run rewrite: when typing between the mention's trailing - * space and a pre-existing draft space, Chromium may re-emit the whole - * run as `replace(" " → " a")` — usually with a non-breaking space. - * Applying that verbatim deletes the draft's space - * (`hello @bob abcworld`), so keep the document's spaces and insert only - * the typed remainder after the trailing space. + * - Whitespace replaced next to that space: keep every space the document + * already has and insert only the typed characters after the token's + * trailing space. + * + * The second rule matters because typing between the mention's trailing + * space and a pre-existing draft space makes Chromium re-emit the whole + * whitespace run — `replace(" " -> " a")`, usually with a non-breaking + * space, and anchored at either edge of the run. Applying any of those + * verbatim deletes the draft's space (`hello @bob abcworld`). + * + * Only whitespace is ever redirected, and only while autocomplete is + * settling — a window in which the user cannot have selected anything, + * because a selection cancels settlement. So a replacement arriving here + * is the browser normalizing whitespace, never an intentional delete, and + * preserving the document's spaces is the whole invariant. Recognizing one + * specific rewrite shape instead is what left the draft space exposed. */ export function insertionForMentionTextInput( doc: ProseMirrorNode, @@ -84,19 +115,14 @@ export function insertionForMentionTextInput( to: number, text: string, ): MentionTextInsertion | null { - const next = selectionAfterMentionTrailingSpace(doc, from); if (from === to) { + const next = selectionAfterMentionTrailingSpace(doc, from); return next === from ? null : { insertAt: next, text }; } - if (next !== from + 1) return null; - if (to === next) { - return { insertAt: next, text }; - } - const replaced = doc.textBetween(from, to, "\n", "\0"); - if (/^[ \u00A0]+$/.test(replaced) && /^[ \u00A0]/.test(text)) { - return { insertAt: next, text: text.replace(/^[ \u00A0]+/, "") }; - } - return null; + const boundary = mentionTrailingSpaceBoundary(doc, from); + if (boundary === null) return null; + if (!SPACE_RUN.test(doc.textBetween(from, to, "\n", "\0"))) return null; + return { insertAt: boundary, text: text.replace(OUTER_SPACES, "") }; } /**