Repository navigation
fix(ui): whitespace-only and block-boundary selections no longer throw or create unanchorable annotations - #1584
Merged
Conversation
A double-click at a block boundary leaves a non-collapsed selection whose whole string is the break between two blocks. web-highlighter's own pointer-end listener then either paints an invisible highlight and opens the toolbar on it — producing an `originalText: "\n"` annotation no reload can re-anchor, still counted and still exported — or, when the browser puts the range's end on an element past its last child (the last block in the document), throws `Cannot read properties of undefined (reading 'parentNode')` out of a listener we do not own. Guard both in packages/ui: the capture-phase pointer-end handler collapses a blank or unserializable selection before the library reads it, the CREATE handler drops a blank quote (covering every other producer, including a quote the exclusion repair emptied), createAnnotationFromSource refuses one last time, our own fromRange calls are caught and logged once, and restore skips a stored blank quote instead of painting it somewhere arbitrary.
…the blank bail The #881 regression pin covers a text-node-bounded selection only, so a predicate narrowed to text boundaries still passes it while dropping the everyday Chromium shapes (multi-block drag, triple-click) that end on an element boundary inside childNodes. Pin one of those, and let the per-test error listener go with its test. The blank-quote bail in CREATE now clears the pending range runs it leaves behind, like every other bail out of that path.
1 task
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.
Problem
Closes #881.
A double-click at a block boundary in a markdown plan or annotate session — just past the end of a heading, paragraph, list item, fence or blockquote — leaves a non-collapsed selection whose whole string is the break between two blocks (
"\n"). web-highlighter's own pointer-end listener acts on it, and which of two symptoms you get depends only on where the browser put the range's end:originalText: "\n"— invisible, counted in the annotation total, exported to the agent, and unanchorable on the next load.TypeError. When the end boundary is an element withendOffset >= childNodes.length(what Chromium reports at the end of the last block), the library resolves it tochildNodes[offset]—undefined— andCannot read properties of undefined (reading 'parentNode')escapes its listener.Reproduction (before)
Built from this branch's base (
origin/main9bea44e) and driven in headless Chromium against a real annotate session (plannotator annotate fixture.md, fixture with paragraph / list / heading / fence / blockquote / table adjacent). 44 real mouse gestures: double-click just past the end of each block's last text run (dx 1/4/10 px), double-click below each block's text baseline, double-click in each inter-block gap; plus a deterministic whitespace-selection-then-Delete flow.page.on('pageerror')captured, and the live selection recorded atmouseupon the capture phase.TypeErrorsGET /api/draft{"type":"DELETION","originalText":"\n",…}The three throwers were
dblclick-past-end-P-block-8-dx{1,4,10}(the last paragraph), stack:Evidence (screenshots, per-gesture JSON, drivers) in the scratchpad:
881/before-final/(incl.persist-b-after.png: the panel showingAnnotations 1/ aDeletioncard with an empty quote, anddblclick-past-end-P-block-8-dx*.png),881/after-final/,881/repro6.mjs(the sweep),881/repro3.mjs,881/isolate-block5.mjs,881/fixture.md.Root cause
All in
@plannotator/web-highlighter@0.8.1, reached from our own call sites:src/model/range/selection.tsgetDomRange()refuses only a collapsed selection, so a whitespace-only one goes through.src/model/range/dom.tsformatDomNode()returns{ $node: n.$node.childNodes[n.offset], offset: 0 }with no bounds check;offset === childNodes.lengthis legal in the DOM, so$nodeisundefined.getDomMeta → getOriginParentthen reads$node.parentNodeand throws, from inside the bubble-phasemouseuplistenerHighlighter.run()registers on$root— i.e. as an uncaught error, with no annotation created either way.applyAnnotationsInternal,packages/ui/hooks/useAnnotationHighlighter.ts) does tryfromStoreinside atry, but a blankoriginalTextstill "succeeds": the stored positions resolve, the content check compares whitespace against whitespace, and an invisible mark is painted on a spot the reviewer never chose — or the text fallback searches the DOM for"\n".The fix belongs in this repo rather than the fork so it does not wait on a publish; the fork could additionally bounds-check
formatDomNodeand trim ingetDomRange, and these guards would stay correct if it did.Change
packages/ui/hooks/useAnnotationHighlighter.tsonly (+109 lines, no behavior change for valid selections):compactText:isBlankQuote(whitespace-only, nbsp included) andisSerializableRange/boundaryResolves(a non-text boundary the library would resolve toundefined).handlePointerEndCapture: a blank or unserializable selection is collapsed and dropped. This runs on the capture phase of the same element, registered beforehighlighter.run(), so the library's own handler sees a collapsed selection and no-ops. It is deliberately placed ahead of the existing containment check: the library does not check containment either, and the last-block shape has its common ancestor in the scroll wrapper outside the article — bailing on containment first is what let those three throws through in my first pass.source.text(after the existing exclusion-aware quote repair, which can itself empty it) removes the painted highlight and returns — no toolbar, no composer, no quick-label, previous pending untouched. This covers every producer: the library's listener, the touchselectionchangebridge, vim and pinpoint ranges.createAnnotationFromSource: refuses a blank quote as a last line.fromRangecalls (highlightRange, the touch bridge) are wrapped intry/catchwith a once-per-loadconsole.warn; state is left untouched.originalTextis skipped beforeattempted— neither painted nor reported, the same treatment adiagramAnchorrow gets, so a legacy draft carrying one does not raise an "Unanchored" chip and a toast on every load.Valid selections keep their exact anchoring and quote (pinned below); share/draft round-trips are untouched.
Tests
New DOM-gated file
packages/ui/hooks/useAnnotationHighlighter.blankSelection.test.tsx(6 tests), added to theDOM_TESTSstep in.github/workflows/test.yml(scripts/dom-test-allowlist.test.tspasses). It drives the real library over a realmouseupin happy-dom, where both shapes reproduce, and captures uncaught errors viawindow.addEventListener('error'):<mark>(before: two empty marks + composer);originalText: "\n");endOffset === childNodes.length→ no uncaught error (before: theparentNodeTypeError);originalText,blockId, offsets, and web-highlighter'sstartMeta/endMeta— and paints the same mark;originalText: "\n"row is skipped (not attempted, not unanchored, not painted) while a valid neighbour restores normally.With the source change reverted, 5 of the 6 fail; the one that passes is the regression pin.
Counts on this branch:
bun test packages/ui packages/editor→ 1261 pass, 905 skip, 0 fail (2166 tests / 225 files)DOM_TESTS=1 bun test packages/ui packages/editor→ 2154 pass, 1 skip, 4 fail — the 4 are the known pre-existingDocBadges open-in placementfailures, unchanged from mainpackages/guide-viewerreportsTS2688: Cannot find type definition file for 'bun'in my sandbox both with and without this change — an artifact of invokingtscdirectly there, not from this PR.)After
Rebuilt (
bun run --cwd apps/review build && bun run build:hook) and re-ran the identical 44-gesture sweep against a fresh annotate server: 0 uncaught errors, 0 blank-quote highlights, the same 15 valid toolbars, and the whitespace-selection-then-Delete flow leaves the panel at "No annotations yet" with an untouched draft (881/after-final/persist-b-after.pngvs881/before-final/persist-b-after.png).One gesture in the after run still shows a toolbar with a blank selection string: verified in isolation (
881/isolate-block5.mjs) to be the unrelated code-block click feature — it creates acodeblock-…annotation quoting the fence's real source, and painted no selection highlight.