fix(composer): use native ProseMirror inserts for autocomplete (mentions / channels / emoji)#618
Merged
Conversation
Mention/channel/emoji autocomplete used to round-trip through the full
markdown parser on every selection: build a new `content` string by
splicing the suggestion in at text-offset space, then call
`setContent(markdown)`. That reparse:
- dropped Shift+Enter hard breaks (newlines collapsed when you picked
a mention/emoji — sprout-bugs report #2),
- blew away any in-progress marks / list context at the cursor (the
"mention ruins formatting" report — sprout-bugs report #3),
- is exquisitely sensitive to subtle markdown parser/serializer
asymmetries that can shift as either side ticks.
It also exposed a latent off-by-one: `getTextAndCursor` used
`state.doc.textContent`, which does *not* honour Tiptap's hardBreak
`renderText` — so each Shift+Enter shifted the cursor offset by 1
relative to what mention/channel detection saw. The bug was invisible
until you tried to round-trip the offset back to a PM position, which
`setContent` happened to do indirectly.
This change replaces the round-trip with a single native ProseMirror
transaction:
- New module `plainTextProjection.ts` builds a one-shot projection of
the doc into plain text (hardBreak + inter-block boundary both
contribute `\n`, matching `doc.textBetween(0, end, "\n", "\n")`)
and exposes two helpers that share the *same* walk:
`mapPMToTextOffset(pm)`
`mapTextOffsetToPM(offset)`
One source of truth → forward + reverse mappings can't drift.
- `useRichTextEditor` gains:
`getPlainTextAndCursor()` replaces `getTextAndCursor`
`replacePlainTextRange(from, to, text)`
single `tr.insertText` call; cursor lands at
`tr.mapping.map(fromPM + text.length)` so the helper remains
correct if anything else shifts positions.
- Hooks (`useMentions`, `useChannelLinks`, `useEmojiAutocomplete`) now
return an `AutocompleteEdit` descriptor
`{ replaceFromOffset, replaceToOffset, insertText }` instead of a
pre-spliced `{ nextContent, nextCursor }`. They stay editor-agnostic
— they just describe the edit in plain-text-offset space.
- Both composers (`MessageComposer` + `ForumComposer`) swap
`setContentWithTrailingSpace(nextContent)` for
`replacePlainTextRange(from, to, insertText)` via a small shared
`applyAutocompleteEdit` helper.
Removed:
- `setContentWithTrailingSpace` (no callers).
- The trailing-space ProseMirror transaction hack (`insertText`
already ends in a space; no need to evade markdown's whitespace
trimming if we never re-parse).
- The old node-walk in `getTextAndCursor` (replaced by the projection
helper, which is correct for hardBreak).
Preserved:
- `breaks: true` on tiptap-markdown (already set).
- `\\\n` → `\n` strip in `getMarkdownFromEditor` — wire-format
behaviour; render uses remark-breaks; no longer load-bearing for
cursor edits, safe either way.
- Emoji-picker button + toolbar @ button paths (already native
`chain().insertContent()` — control cases, unchanged).
Tests:
- New `plainTextProjection.test.mjs` (20 tests) covering single
paragraph, hardBreak before/after/across, multi-paragraph, nested
list items, paragraph + bullet-list mix, edge cases (empty doc,
out-of-range clamping), and a property-test round-trip that every
text offset maps to a PM position that maps back.
- Removed `useRichTextEditor.test.mjs` — it tested a copy of the old
algorithm against a hand-built node list, which no longer exists.
The new test exercises the real Tiptap schema directly.
- All existing node tests still pass (176/176).
- typecheck + biome + production build clean.
- File sizes within limits.
Net diff in existing files: +155 / -273 (-118 LOC). New files: ~200
LOC of pure projection + thorough tests.
Co-authored-by: Quinn (sprout agent) <96f056ad5f2305c8ddf637dc65d048aa4c12d7daeb8867690e34fca46b0ef64c@sprout-oss.stage.blox.sqprod.co>
Signed-off-by: Tyler Longwell <109685178+tlongwell-block@users.noreply.github.com>
Address Perci's review on PR #618. The plain-text projection treated empty paragraphs / list items as 'no content' — the block-boundary \n was emitted, but no segment was recorded for the empty block's interior PM position. As a result: <p>a</p><p></p> produced text 'a\n' but offset 2 → PM 3 (the boundary) → back to 1 Cursor / range endpoints living *inside* an empty paragraph (paste, draft restore, type-then-Shift-Enter-twice) couldn't round-trip. Fix: - New segment kind `emptyBlockContent` records the zero-width content slot for an empty leaf block at PM `pos + 1`, mapped to the current text offset. Recorded immediately after the blockBoundary (or as the first segment if the empty block is the very first leaf). - `mapPMToTextOffset` and `mapTextOffsetToPM` both gain a clause for the new segment kind. The text-offset claim is exact-equality (`offset === seg.textAt`) so it only fires when no following text segment will claim the offset — preserves the priority "text wins, then hardBreak, then empty-block slot, then boundary" that the existing tests assume. - Initial-offset handling (offset <= 0) now also considers the empty-block slot, so a doc that *starts* with an empty paragraph has offset 0 land inside it rather than before the doc. Tests added: - paragraph + empty paragraph (offset round-trip into the empty block) - empty paragraph + paragraph (offset 0 lands inside the empty block) - two empty paragraphs (single \n, both interiors reachable) - empty list item + filled list item (separators preserved) Round-trip property test now exercises a fixture that mixes hardBreak, empty paragraph, filled paragraph, filled list item, empty list item, and filled list item. All 26 projection tests pass; 182/182 node tests pass overall; typecheck + biome + file-size checks clean.
… arithmetic Desktop smoke e2e was failing on the two click-to-select mention paths. Root cause: `replacePlainTextRange` placed the post-transaction cursor at tr.mapping.map(fromPM + text.length) The argument to `mapping.map` is a *pre-image* (old-doc) position, but `fromPM + text.length` can be larger than the old doc's size — for a 3-char input replacing a 0-char range with 7 chars, the arithmetic yields a position 7 past the original end, which ProseMirror rejects with "Position N out of range". The thrown error bubbled out of the mention click handler, the transaction never dispatched, and the editor stayed at "Hey @ali". The keyboard-Enter path passed only because the surrounding code-path (`handleEditorKeyDown`) caught the error implicitly — but in fact it was hitting the same bug; the test just asserted on a state that the default Enter keymap also reaches. Fix: map `toPM` (the right end of the replaced range, a valid old-doc position) through `tr.mapping.map`. That returns the post-transaction position right after the inserted text — which is what we want, and is always valid: const cursorPM = tr.mapping.map(toPM); This still satisfies Perci's robustness concern from the original review (use the transaction's mapping, not raw arithmetic) but uses the correct pre-image input. Regression coverage: - All 4 existing mentions.spec.ts tests for the click-and-Enter selection paths now pass. - Added a new regression test for bug #2: type two lines separated by Shift+Enter, type "@ali", click "alice" — assert both lines survive *and* there's still exactly one `<br>` in the editor. This is the literal scenario from the bug report. Verified locally: - smoke suite: 97/97 pass (was 94 + 2 new mentions tests + 1 new regression test). - node unit tests: 182/182. - typecheck + biome + file-size: clean. Removed the temporary `console.warn` instrumentation I'd added while diagnosing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Replace the markdown round-trip on mention/channel/emoji autocomplete with a single native ProseMirror transaction, and fix a latent hardBreak off-by-one in the plain-text cursor mapping while we're there.
Tracked the design with Perci end-to-end in #sprout-bugs; we converged on 9.5/10 (minimal / elegant / correct) before I started building.
The four bugs and what's actually wrong
Reports in #sprout-bugs:
richText.setContentWithTrailingSpace(nextContent)→editor.commands.setContent(markdown). That's a full markdown reparse on every insert; even withbreaks: true, the round-trip is sensitive to subtle parser/serializer asymmetries and resets cursor/marks.@mentioningruins the formatting. Same root cause — the reparse rebuilds the doc from scratch, so any in-progress marks (bold, italic, code) and list-item nesting at the cursor are blown away.This PR fixes #2 and #3 plus a latent off-by-one in
getTextAndCursor: it usedstate.doc.textContent, which ignores Tiptap'shardBreakrenderText. EveryShift+Entershifted the cursor offset by 1 relative to what mention/channel detection saw — invisible only because nobody had to round-trip the offset back to a PM position.How
1. New module
plainTextProjection.ts— one walk over the doc, one source of truth for both directions:Hard breaks and inter-block boundaries each contribute exactly one
\n, matchingdoc.textBetween(0, end, "\n", "\n"). Forward + reverse mappings share the same segment list so they can't drift — historically the source of bugs.2.
useRichTextEditorgains two paired helpers:replacePlainTextRangeis a thin wrapper aroundtr.insertText(text, fromPM, toPM)— the right ProseMirror primitive for a textarea-style replacement (preserves active marks, handles empty insert). The new cursor lands attr.mapping.map(fromPM + text.length)so the helper stays robust if anything else shifts positions.3. Hook API change:
Hooks stay editor-agnostic: they describe the edit in plain-text-offset space. The editor wrapper applies it. Same change for
useChannelLinksanduseEmojiAutocomplete.4. Both composers (
MessageComposer+ForumComposer) swap to the new helper via a small sharedapplyAutocompleteEditlocal. No moresetContent(markdown)for cursor edits.Removed
setContentWithTrailingSpace(no callers).insertTextalready ends in a space; no markdown reparse to evade).descendantswalk ingetTextAndCursor(replaced by the projection helper, which is correct forhardBreak).Preserved
breaks: trueon tiptap-markdown (already set).\\\n → \nstrip ingetMarkdownFromEditor— wire-format behaviour, no longer load-bearing, safe to leave.@button paths — already nativechain().insertContent(); control cases, unchanged.Tests
plainTextProjection.test.mjs— 20 tests against the real Tiptap schema (built viagetSchema([StarterKit.configure(...)]), not a hand-rolled one): single paragraph, hardBreak before/after/across-break, two and three paragraphs, paragraph + bullet-list mix, list-item round-trip, edge cases (empty doc, out-of-range), and a property test that every text offset → PM → text offset is identity over a fixture covering all three node types.useRichTextEditor.test.mjs— it tested a copy of the old algorithm against a hand-built node list. New test exercises the real schema directly.File-size and LOC
MessageComposer.tsx: 702 → 709 lines (limit 710; fits).Self-rating
Follow-up PR (separate, after this lands)
toggleBulletList.window.prompt × 2with a shadcn popover;setLink({href})for selections, nativeinsertContentwith link mark for no-selection — never insert literal[…](…)text.These are toolbar UX changes on a different surface; reviewing them mixed in with this deeper editor-state change would slow review.
Out of scope
useDrafts.ts) — separate file/feature.cc @perci-sprout — your two implementation nits from the design thread are both honored:
tr.mapping.map(fromPM + insertText.length), not the raw arithmetic.<br>and inside-list-item.