Keep callout markers on the line they were written on - #445
Conversation
✅ Deploy Preview for docs-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
kbatuigas
left a comment
There was a problem hiding this comment.
Just one finding, but I'll approve:
[P2] Preserve repeated callout numbers — [11-editable-placeholders.js](
)docs-ui/src/js/11-editable-placeholders.js
Lines 91 to 100 in 3b971a3
The observer treats every
i.conumwith the samedata-valueas a duplicate and removes all but the first wheneveraddConumSpansrewrites the block. However, explicitly repeating a callout number on multiple lines is valid AsciiDoc—for example, two<1>markers can reference the same annotation. [Asciidoctor documents this use case](https://docs.asciidoctor.org/asciidoc/latest/verbatim/callouts/#mixed-numbering). Such a block will still lose its later markers. Now that the generated<b>(N)</b>fallbacks are removed before processing, deduplication should preserve legitimate repeated markers, and the browser test should include that case.
The Connect Helm chart quickstart rendered the callouts in its Hello world example as "4 1 3" on one line. Three bugs stacked up to produce that. 17-bloblang-yaml.js: every process* function rebuilds its token from token.textContent, which flattens child elements away, then assigns innerHTML. Any conum or editable placeholder inside the token is destroyed, and processMultilineMapping additionally deletes the continuation siblings it folds in. Callouts <1> and <3> sat inside "mapping: |" block scalars, so both were wiped. Added a hasPreservedMarkup guard at the dispatch point and over the continuation nodes. 11-editable-placeholders.js: the conum MutationObserver repaired removals with mutation.target.appendChild(node), which sends the marker to the end of the block. That is what turned a silent deletion into a visible scramble, and a callout pointing at the wrong line reads as fact. It now restores the position from the siblings MutationRecord captured. Same file, addConumSpans: the literal-(N) regexes matched the text inside Asciidoctor's hidden <b>(1)</b> fallback and minted a second, nested conum for every callout on the page. Those duplicates also defeat the observer's own duplicate check. The fallback element is now dropped first, which keeps "(N)" out of the copy button's output too. The observer fix alone is not enough. When a block scalar contains a blank line Prism splits it, the continuation nodes carrying the position anchors are deleted, and the restore falls back to appendChild: two callouts collapse onto one line. The guard is required. Its cost is that a block scalar containing callouts no longer gets Bloblang colouring, which is a better trade than markers against the wrong lines. tests/callout-preservation runs the real scripts in a real browser, since the server HTML and a cold DOM both look correct and the bug needs Prism, keep-markup and the Bloblang pass to have all run over the same block. Reverting the two source files fails it. generate:prism runs first because prism-core.js is generated, not tracked.
The suites in validate-build.yml run with PUPPETEER_SKIP_DOWNLOAD, so a browser test belongs in the Bloblang workflow next to the other puppeteer ones. Its path filter already covered 17-bloblang-yaml.js but not 11-editable-placeholders.js or the new test, so a change to either would not have run it.
The restoration observer removed every conum past the first with a given data-value anywhere in the code block. That was written to guard against the <b>(N)</b> fallback minting an adjacent duplicate, but repeating a callout number on separate lines is valid AsciiDoc mixed numbering, and the blanket check silently deleted the second, genuine marker. Narrow the check to an actual adjacent sibling with the same value -- the shape the fallback bug produced -- so a second callout elsewhere in the block survives. Added block F to the callout-preservation fixture and its browser-test assertions; reverting the observer change reproduces the loss (3 nodes -> 2) and confirms the new checks catch it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3b971a3 to
fcf276f
Compare

The Connect Helm chart quickstart rendered its Hello world callouts as "4 1 3" on one line. Three bugs stacked up:
17-bloblang-yaml.jsrebuilds each token fromtextContentand reassignsinnerHTML, destroying any callout marker or editable placeholder inside it. Now guarded.11-editable-placeholders.jsrepaired those removals withappendChild, so the orphans landed at the end of the block. Now restores the original position.addConumSpansmatched the text inside Asciidoctor's hidden<b>(1)</b>fallback and minted a duplicate nested conum per callout. The fallback is now dropped first.The observer fix alone is not enough: a block scalar with a blank line in it loses the position anchors, so the guard is required. It costs Bloblang colouring inside a block scalar that contains callouts, which beats markers against the wrong lines.
npm run test:callout-preservationcovers this in a real browser, which is necessary because the server HTML and a cold DOM both look correct. Reverting the two source files fails it.The page itself is fixed separately in rp-connect-docs PR 520, since this needs a release to reach the site.