fix(ai): operations on blocks containing comments - #2953
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR fixes Yjs thread selection serialization by using the current binding. It also enables cumulative ProseMirror step mapping for HTML updates that affect commented blocks, with regression tests for comment-anchor preservation. ChangesYjs thread position resolution
Commented HTML block rebasing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Editor
participant RESTYjsThreadStore
participant YjsBinding
participant Fetch
Editor->>RESTYjsThreadStore: addThreadToDocument(selection)
RESTYjsThreadStore->>YjsBinding: resolve selection positions
RESTYjsThreadStore->>Fetch: submit serialized relative positions
Fetch-->>Editor: return thread request
sequenceDiagram
participant HTMLUpdate
participant RebaseTool
participant Mapping
participant Transaction
HTMLUpdate->>RebaseTool: provide HTML diff steps
RebaseTool->>Mapping: map steps cumulatively
Mapping-->>RebaseTool: return mapped steps
RebaseTool->>Transaction: apply mapped steps
Transaction-->>HTMLUpdate: preserve comment anchors
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Comment marks are tagged `blocknoteIgnore`, so they're deliberately not part of the BlockNote document model. `blocksToHTMLLossy` therefore drops them, and parsing the HTML back produces a block without them. `createHTMLRebaseTool` round-trips the target block through exactly that path and threw `html diff` if the result differed from the document — so every AI operation on a block containing a comment failed before the LLM's response could be applied. Apply the round-trip difference to the projection instead of throwing, which is what the rebase tool's invert map is for, and what `createMDRebaseTool` already does for markdown. Comment anchors outside the rewritten range are preserved. Also fix `RESTYjsThreadStore.addThreadToDocument`, which passed the `ySync` plugin state where a binding was expected. The plugin state has no `mapping`, so adding a thread threw `Cannot read properties of undefined (reading 'get')` as soon as the position walk reached the first non-text node — which, for a BlockNote document, is immediately.
64f2a12 to
25b93b2
Compare
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
|
Summary
Fixes #2947
Running the AI on a block that contains a comment fails with
Error: html diff.Rationale
Comment marks are tagged
blocknoteIgnore(packages/core/src/comments/mark.ts), so they're deliberately excluded from the BlockNote document model.blocksToHTMLLossytherefore emits<p>Hello, world!</p>with nospan.bn-thread-mark, and parsing that back gives a block without the mark.createHTMLRebaseToolround-trips the target block through exactly that path and threw if the result differed from the document. For a block containing a comment it always differs, so the operation failed before the LLM's response could be applied. Blocks are only affected if they carry a comment themselves — editing a sibling block worked, which matches the video in the issue.The rebase tool exists precisely to handle formats that can't express everything in the document: apply the operation to a "clean" projection, then rebase the result onto the real document through the invert map.
createMDRebaseToolalready does this for markdown; the HTML version asserted losslessness instead of building the projection.Changes
createHTMLRebaseTool: apply the round-trip difference to the projection rather than throwing, matchingcreateMDRebaseTool. Theblocks.length !== 1guard is unchanged, and an unmappable step still throws.RESTYjsThreadStore.addThreadToDocument: use theySyncbinding'stype/mappinginstead of the plugin state.Impact
The second fix is a separate bug found while tracing the first, in the same area (comments + Yjs).
addThreadToDocumentpassed theySyncplugin state where a binding was expected. The plugin state has atypebut nomapping, so passingundefinedas the mapping threw as soon asabsolutePositionToRelativePositionreached a non-XmlTextnode:A BlockNote document starts with a
blockGroupelement, so this threw on every call — the Yjs positions were never actually sent. It can be dropped from this PR if you'd rather keep the two separate.Testing
commentedContent.test.ts: building a rebase tool for a commented block, updating a commented block, and updating a sibling while another block has a comment. The first two fail onmainwithhtml diff. Asserts the comment stays anchored to"Hello"after"Hello, world!"is rewritten to"Hello, universe!", so the fix isn't just "stops throwing". Runs fully offline — tool calls are fed straight intoStreamToolExecutor, no LLM.RESTYjsThreadStore.test.ts: stubsfetchand asserts the emitted relative positions resolve back to the positions passed in, so a silently-wrong mapping fails it too.packages/core716 passed / 9 skipped,packages/xl-ai204 passed / 260 skipped (skips are the API-key-gated model suites, unchanged).Checklist
Summary by CodeRabbit
Bug Fixes
Tests