SD-3227 - fix: enhance anchor navigation logic in TOC#3514
Conversation
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
chittolinag
requested review from
artem-harbour,
caio-pizzol,
luccas-harbour and
tupizz
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
No issues found across 4 files
Linked issue analysis
Linked issue: SD-3227: BUG: TOC section links fail because TC-embedded _Toc bookmarks are not imported
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Preserve/import _Toc bookmarks embedded inside TC field instructions by hoisting w:bookmarkStart / w:bookmarkEnd out of the synthesized sd:tableOfContentsEntry so the bookmark name is registered for navigation. | tc-preprocessor now extracts bookmarkStart/bookmarkEnd runs into siblings and builds the sd:tableOfContentsEntry from the remaining nodes, so bookmark nodes are no longer trapped inside an atom; unit tests verify the hoist behavior. |
| ✅ | Resolve bookmarks whose PM position sits just past a paragraph's visible runs to that paragraph (prefer the PM-closer neighbour; ties preserve prior behaviour of choosing the next fragment). | AnchorNavigation updated to track previous fragment and choose the neighbour with smaller PM-distance, with tie behavior kept to next fragment; several new test cases cover closer-neighbour, tie, and the SD-3227 scenario. |
| ✅ | Add regression/unit tests that reproduce and prevent recurrence of SD-3227 (hoist + navigation behavior together result in TOC anchors for section bookmarks resolving correctly). | The PR adds co-located tests for the tc-preprocessor hoist and new AnchorNavigation cases including the SD-3227 scenario, demonstrating the combined fix is covered by CI-level tests. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ecff25dd1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (node?.name === 'w:bookmarkStart') { | ||
| leadingBookmarks.push(node); | ||
| } else if (node?.name === 'w:bookmarkEnd') { | ||
| trailingBookmarks.push(node); |
There was a problem hiding this comment.
Do not bucket all w:bookmarkStart nodes before the synthetic entry and all w:bookmarkEnd nodes after it, because this rewrites bookmark ordering and can corrupt bookmark spans. For example, adjacent pairs like start1,end1,start2,end2 become start1,start2,entry,end1,end2, which changes which content each bookmark covers and can produce invalid/crossed ranges on import/export. The hoist should keep each marker’s original relative order (or pair by w:id) instead of globally reordering starts and ends.
Useful? React with 👍 / 👎.
…ecause-tc-embedded-_toc
3 participants
Issue
TOC entries whose
_Toc…target bookmark lives inside a heading's hiddenTCfield didn't navigate, while entries whose bookmark wrapped visible heading text worked. The embedded bookmark was being trapped inside the synthesizedsd:tableOfContentsEntryPM atom, so its name was never indexed for navigation. See SD-3227.Proposed solution
tc-preprocessor.js: hoist embeddedw:bookmarkStart/w:bookmarkEndruns out of the TC entry as paragraph-level siblings, so the bookmark name gets registered in the bookmark map.AnchorNavigation.ts: when no fragment contains the bookmark position, pick the PM-closer neighbour (ties still go to the next fragment) so a bookmark sitting just past a paragraph's visible runs lands on that paragraph, not the next one.tc-preprocessor.test.jsand newAnchorNavigation.test.tscases covering the hoist, the closer-neighbour fallback, and the SD-3227 regression.