Automatically generate table of contents in text #2213
Conversation
|
just a heads up, I probably won't get around to deeply reviewing this until next week. at a glance it looks good though. |
There was a problem hiding this comment.
It passed my stress test, leaving behind just a nitpick.
Your code is high quality, especially buildToc which addresses the complexity of the problem in an elegant way.
There's only the problem of {:toc} rendering in comments, but even then, we're talking about UX that can be fixed easily.
I also talked about the duplication issue, but that doesn't depend from your code, nor was in scope for this PR/issue, just something to keep in mind.
It works really well, nice job! ^^
|
|
||
| return toc | ||
| }, [text]) | ||
| const toc = useMemo(() => extractHeadings(text), [text]) |
There was a problem hiding this comment.
I noticed that we don't give IDs to nodes inside comments, only on full posts:
stacker.news/components/text.js
Line 116 in 4620160
Because of this, the table of contents won't work if used in comments. I personally think that the ToC doesn't make that much sense in comments, so we can just disable {:toc} for them with topLevel awareness. What do you think? ^^
There was a problem hiding this comment.
I agree, it wouldn't make sense to use Toc in comments, so I changed it so that remarkToc is only processed for topLevel items
| const str = toString(node) | ||
| headings.push({ | ||
| heading: str, | ||
| slug: slug(str.replace(/[^\w\-\s]+/gi, '')), |
There was a problem hiding this comment.
note for the future, unrelated to review:
it seems that we don't handle duplicate headings, because we use slug instead of the GithubSlugger class.
Probably because it's faster as GithubSlugger would have instead tracked headings in memory and checked every heading against the previous ones to count 🤔.
There was a problem hiding this comment.
We don't use GithubSlugger because of #1405
There was a problem hiding this comment.
Ok, will leave this as is for now
| @@ -49,6 +54,9 @@ export function SearchText ({ text }) { | |||
|
|
|||
| // this is one of the slowest components to render | |||
| export default memo(function Text ({ rel = UNKNOWN_LINK_REL, imgproxyUrls, children, tab, itemId, outlawed, topLevel }) { | |||
| // include remarkToc if topLevel | |||
| const remarkPlugins = topLevel ? [...baseRemarkPlugins, remarkToc] : baseRemarkPlugins | |||
There was a problem hiding this comment.
Unfortunately, topLevel cannot be used to exclude this plugin from comments because topLevel is also true for comments when they are the root of a page:
2025-07-25.02-08-24.mp4
not sure how to best deal with this
There was a problem hiding this comment.
Interesting. One possibility is to remove the topLevel check for giving headings node ids:
stacker.news/components/text.js
Line 116 in 4620160
I'm not sure why that check is there and if doing so would break anything.
I suppose another possibility is to only process {:toc} if the item has no parent, right? Will topLevel always be true for posts? (and thus headings always get assigned node ids in posts)
| const str = toString(node) | ||
| headings.push({ | ||
| heading: str, | ||
| slug: slug(str.replace(/[^\w\-\s]+/gi, '')), |
There was a problem hiding this comment.
Was just copying from line 22 of the old components/table-of-contents.js, which uses the same pattern:
But is it redundant since we're already passing the string to slug? I will usually copy the old code whenever I can to break as few things as possible
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| while (stack.length && depth <= stack[stack.length - 1].depth) { | ||
| stack.pop() | ||
| } | ||
| let parent = stack[stack.length - 1].node |
There was a problem hiding this comment.
Bug: Stack Underflow in buildToc Function
The buildToc function is vulnerable to a stack underflow. If a heading with a depth of 0 or less is encountered in the AST, the while loop will pop all elements from the stack. This causes stack[stack.length - 1] to become undefined, leading to a TypeError when its .node property is accessed. While standard markdown headings have depths 1-6, this edge case can crash the application if malformed AST data is processed.
Locations (1)
Description
Closes #2208
Writing
{:toc}on its own line will automatically render a table of contents as a bulleted list, using the headings (#,##, etc) contained in the post.Screenshots
auto-toc.mov
Additional Context
n/a
Checklist
Are your changes backwards compatible? Please answer below:
yes
On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
{:toc}somewhere it could render in unexpected ways.The behavior of links that start with a hash (i.e.
#section-1) has also changed, but I can't see why anyone would've wanted such links to open in a new tab.For frontend changes: Tested on mobile, light and dark mode? Please answer below:
n/a
Did you introduce any new environment variables? If so, call them out explicitly here:
no