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 savedSelectionRef = useRef<any>(null); | ||
|
|
||
| const currentSize = getSizeFromCurrentBlock(editor); | ||
| const currentBlockId = editor.getTextCursorPosition()?.block?.id; |
There was a problem hiding this comment.
For what reason do we need pendingSizeRef and currentSize and what's the difference?
Same for originalBlockIdRef, savedSelectionRef, currentBlockId
There was a problem hiding this comment.
You're right, having both was confusing. I've completely removed the redundant useRef hooks (pendingSizeRef, originalBlockIdRef, savedSelectionRef).
The logic is now much simpler: we rely on the closure variables (currentSize, currentBlockId) from the render cycle. This makes the code cleaner and eliminates the need for manual state resets
| 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
| await insertInlineChipViaSlashMenu(); | ||
|
|
||
| await expect.element(page.getByText('#123')).toBeVisible(); | ||
| await expect.element(page.getByText('#123').first()).toBeVisible(); |
There was a problem hiding this comment.
I am wondering why this change is necessary?
There was a problem hiding this comment.
I initially added .first() to avoid strict-mode errors because the menu and the new chip were present at the same time. However, after simplifying the focus logic and refactoring the component, the tests now pass reliably without it. I've reverted this change to keep the tests clean
| setTextCursorPosition: vi.fn(), | ||
| }; | ||
|
|
||
| editor._tiptapEditor = makeFakeTiptap(block); |
There was a problem hiding this comment.
Let's try to use a real BlockNote editor for the tests. Using a fake TipTap here could make these tests useless in the future - if BlockNote decides to remove some TipTap functionality, these tests could still be green but the functionality be broken in the "real world", because the tests are using TipTap directly instead of BlockNote.
| const chip = inserted.find( | ||
| (c): c is { type: string; props: { size: string } } => | ||
| c.type === "openProjectWorkPackageInline" && c.props?.size | ||
| (item: any): item is { type: string; props: { size: string } } => |
There was a problem hiding this comment.
Thanks for no longer using abbreviations here 🤩
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