Repository navigation
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes header ID generation for headers containing inline elements (such as links and images) by delaying the ID generation until after inline parsing of the header's children has completed. This ensures that link text is used instead of the link target, and images are omitted entirely. The review feedback suggests decoupling the needsGeneratedId check from the node.children != null check in document.dart to make the implementation more robust for elements that might not have children but still require an ID.
| } else if (node is Element && node.children != null) { | ||
| _parseInlineContent(node.children!); | ||
| if (node.needsGeneratedId) { | ||
| node.generatedId = BlockSyntax.generateAnchorHash(node); | ||
| } | ||
| } |
There was a problem hiding this comment.
Decoupling the recursive inline parsing of children from the needsGeneratedId check makes the code more robust. If a future change or custom syntax creates an element with needsGeneratedId = true but children = null, the ID will still be generated correctly instead of being silently ignored.
} else if (node is Element) {
if (node.children != null) {
_parseInlineContent(node.children!);
}
if (node.needsGeneratedId) {
node.generatedId = BlockSyntax.generateAnchorHash(node);
}
}References
- When building foundational components, prioritize robust, future-proof implementations over simpler ones that may introduce subtle bugs or rely on unintended behavior.
|
One follow-up thought on this fix: A few ways to harden this; curious which you'd prefer (or if you'd rather leave it as-is):
Happy to push whichever you'd like as a follow-up. |
srawlins
left a comment
srawlins
left a comment
There was a problem hiding this comment.
Thank you!
Just a merge conflict in the CHANGELOG and I think this is good to go.
63d08b3 to
d2143b0
Compare
9abccff to
6a8e35f
Compare
8ce38bf to
ed32851
Compare
HeaderWithIdSyntax/SetextHeaderWithIdSyntax generated ids from the header's raw, un-inline-parsed source, so a header containing a link (e.g. `### [Task](/some/path)`) folded the link target into the id instead of using only the link text. Defer id generation until after inline parsing so it runs against the resolved element tree: link targets are excluded, images contribute nothing (matching GitHub's behavior), and other inline formatting is flattened to plain text. Fixes dart-lang#1339
ed32851 to
dccca02
Compare
|
@srawlins the CHANGELOG conflict is resolved and the branch is rebased on main, so this should be good to merge whenever you get a chance. Thanks! |
Summary
HeaderWithIdSyntax/SetextHeaderWithIdSyntaxgenerated a header's anchor id from the raw, un-inline-parsed markdown source. For a header containing a link (e.g.### [Task](/packages/fpdart/lib/src/task.dart)), that folded the link target into the id (taskpackagesfpdartlibsrctaskdart) instead of using just the link text (task), breaking in-page anchor navigation wherever this pattern appears (e.g. on pub.dev-rendered READMEs).**bold**,`code`, etc.) is flattened to plain text before the id is derived.needsGeneratedIdflag onElementso header syntaxes can mark "this element wants a generated id," andDocumentcomputes the actual id right after resolving that element's inline children.Fixes #1339
Test plan
test/extensions/headers_with_ids.unitandtest/extensions/setext_headers_with_ids.unitreproducing the exact repro from the issue (a header containing a link, and a header containing a link + image) — confirmed these fail against the old code and pass with the fix.dart test(full suite, 2766 tests) passes.dart analyze— no issues.dart format --output=none --set-exit-if-changed— no changes needed.