fix(make-pdf): assign body heading ids so --toc anchors resolve (#1689)#1789
Open
0xDevNinja wants to merge 1 commit into
Open
fix(make-pdf): assign body heading ids so --toc anchors resolve (#1689)#17890xDevNinja wants to merge 1 commit into
0xDevNinja wants to merge 1 commit into
Conversation
- buildTocBlock minted `toc-N` ids for its `#toc-N` anchors and `data-toc-target` spans, but no body heading ever received those ids, so every TOC link was a dead fragment and Paged.js had no target to count page numbers against - Add annotateHeadingIds: walk H1-H3 in document order, inject `id="toc-N"` on headings that lack one, preserve any heading that already declares an id and point the TOC at it - Share the resolved heading/id list between body and TOC so anchors and targets always agree - 3 regression tests: anchor-resolves-to-heading, existing-id preserved, no id injection when toc is off Fixes garrytan#1689.
Contributor
|
Collision check: #1690 is already open for the same issue and same files. It was opened on 2026-05-25 by the issue author, also targets #1689, touches This patch looks directionally correct, but functionally duplicate. I would treat #1690 as the canonical PR unless maintainers prefer this branch. If this one continues, it should explicitly explain why #1690 is being superseded. |
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.
Summary
make-pdf --tocbuilt a table of contents whose<a href="#toc-N">anchors and<span data-toc-target="toc-N">page-number spans referenced element ids (toc-0,toc-1, ...) that no body heading ever carried.buildTocBlockmintedtoc-${i}purely for its own markup; the body headings came straight frommarked(<h1>One</h1>, no id) andwrapChaptersByH1never assigned them. So every TOC link was a dead in-page fragment and Paged.js had no target element to resolve page numbers against.Fixes #1689.
Fix
New
annotateHeadingIds(html)pass:id="toc-N"injected (N = document-order index).{level, text, id}list, whichbuildTocBlocknow consumes — so anchors and targets are guaranteed to agree with the body.The id annotation only runs when
--tocis requested (the ids exist solely for the TOC), so non-TOC renders are byte-for-byte unchanged.Before / after (issue repro)
Before:
anchors: ["#toc-0","#toc-1","#toc-2"] heading ids: []After:
anchors: ["#toc-0","#toc-1","#toc-2"] heading ids: ["toc-0","toc-1","toc-2"]Tests
3 new cases in
make-pdf/test/render.test.ts:data-toc-targetall resolve to a real body heading id.id="toc-injection whentocis off.