Bug/74325 inserting hash inside text removes content after cursor#122
Bug/74325 inserting hash inside text removes content after cursor#122ihordubas99 wants to merge 3 commits intodevfrom
Conversation
judithroth
left a comment
judithroth
left a comment
There was a problem hiding this comment.
The code works, there is no more content removed when inserting with #.
However, I find the code very hard to understand. Also, this is using TipTap API directly and BlockNote might not support that for ever. If somehow possible, please try to solve the problem with BlockNote "materials on board".
| const originalBlockId = originalBlockIdRef.current ?? currentBlockId; | ||
| const savedSelection = savedSelectionRef.current; | ||
|
|
||
| pendingSizeRef.current = "xxs"; |
There was a problem hiding this comment.
Shouldn't this be covered by const pendingSizeRef = useRef<InlineWpSize>("xxs"); already?
| clearTriggerText(editor); | ||
| insertWpChipIntoBlock(editor, blockId, wp, size); | ||
| if (savedSelection) { | ||
| const tiptap = (editor as any)._tiptapEditor; |
There was a problem hiding this comment.
Nick from BlockNote wrote that they are not sure if they will keep the tiptap APIs (https://matrix.to/#/!yWsguKHvTVyakhzlui:openproject.org/$namCWPK10P-jifVJIjDTaj7bYnnw19XbSC2pSjO8JkY?via=openproject.org&via=matrix.org) and that we should not rely on them.
Is it possible to achieve this without tiptap? If not, why?
There was a problem hiding this comment.
Good news: I removed the TipTap dependency from the UI component (HashWpMenu.tsx). By keeping focus on the editor during a click, we no longer need TipTap to manage the cursor there.
However, for clearTriggerText, using TipTap remains the most reliable approach at this stage. Standard browser tools (DOM) get confused by our custom WorkPackage chips: they count every letter inside the chip, while BlockNote's internal model sees the whole chip as a single item. This "offset mismatch" makes it very risky to delete the # trigger using native methods, as it often leads to deleting the wrong text or data loss.
TipTap is currently the most stable way to find the exact cursor position within BlockNote's structure and ensure we only delete the trigger text
There was a problem hiding this comment.
Thanks for removing the TipTap dependency from HashWpMenu.tsx!
Standard browser tools (DOM) get confused by our custom WorkPackage chips: they count every letter inside the chip, while BlockNote's internal model sees the whole chip as a single item. This "offset mismatch" makes it very risky to delete the # trigger using native methods, as it often leads to deleting the wrong text or data loss.
TipTap is currently the most stable way to find the exact cursor position within BlockNote's structure and ensure we only delete the trigger text
Okay, I see the problem but I am not convinced we necessarily have this problem 🙂
See my comment here: https://github.com/opf/op-blocknote-extensions/pull/122/changes#r3189104054
judithroth
left a comment
judithroth
left a comment
There was a problem hiding this comment.
This looks already a lot better! The code is at lot easier to understand now 👍 thanks for improving this!
And it works as expected and does not delete more than necessary ✨
However, it still relies on using TipTap in some places and I am not (yet?) convinced we really need that. I'd also like to have one real integration (browser) test for the insertion via "#" notation (one with text before and after where the link is inserted, so we can test that both are not removed accidentally).
|
|
||
| // BlockNote's default "Enter" behavior splits the block. | ||
| // If the block ID changed, it was a keyboard Enter, so we remove the new empty block. | ||
| const isKeyboardEnter = currentBlock && currentBlock.id !== currentBlockId; |
There was a problem hiding this comment.
Could we simplify this by using something like event.preventDefault and event.stopPropagation?
| const text = triggerNode.text; | ||
| const hashIndex = text.search(/#/); | ||
| const textBefore = hashIndex > 0 ? text.slice(0, hashIndex) : null; | ||
| const textBefore = $from.parent.textBetween( |
There was a problem hiding this comment.
Instead of using TipTap here, would it be possible to use BlockNote's setTextCursorPosition to set the position right before the newly inserted block (which I assume is inserted right after the search text the user entered), then you can delete backwards until the "#" sign, without having to worry about having a BlockNote block messing with the counting - since the search via "#" only is text, there can be no other block than the one that was just created.
If that is not possible I am okay with keeping TipTap, but at the moment I don't see why this approach should cause problems and why we necessarily need TipTap here 😅
| it("does nothing if no block", () => { | ||
| const editor = { getTextCursorPosition: () => null, updateBlock: vi.fn() }; | ||
| const editor = { | ||
| _tiptapEditor: null, |
There was a problem hiding this comment.
Instead of using TipTap, please try to create an empty BlockNote editor or just remove the contents from the one you create with createTestEditor instead.
There was a problem hiding this comment.
Thanks for adding all those tests!
Please add at least one real integration test that interacts with a browser. I see value in these unit tests here since they cover edge cases like having multiple texts with "#" in a line and seeing that it is deleted only until the first # (from right) 👍
However, integration tests often uncover problems the developer or AI hasn't thought of yet. And those are typically very hard to test in unit tests :)
2 participants
Ticket
https://community.openproject.org/projects/communicator-stream/work_packages/74325
What are you trying to accomplish?
Fix a data loss bug where inserting # in the middle of a text block would silently delete all content after the cursor position.
What approach did you choose and why?
The root cause was in clearTriggerText: the old implementation searched for the trigger node in BlockNote's content tree and sliced everything from that index onward - which discarded all content after the cursor.
The fix rewrites clearTriggerText to operate directly on the TipTap transaction layer. Instead of manipulating the BlockNote content tree, it now reads the 50 characters before the cursor, matches the trailing #+\S* pattern, and issues a precise tr.delete for exactly those characters. This leaves the rest of the block untouched.
As a secondary cleanup, size and blockId are now captured at interaction time (via refs) rather than at render time, eliminating stale closure values on keyboard selection. insertWpChipIntoBlock was simplified to a thin wrapper around insertWpChip since both mouse and keyboard paths now share the same cursor-based insertion logic.
Considered keeping the BlockNote content-tree approach and patching the slice logic, but the tree manipulation is inherently fragile when the cursor is mid-block - TipTap transactions are the correct abstraction for precise text deletion.
Merge checklist