-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add table of contents for comments and headings #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| "use client" | ||
|
|
||
| import { Suspense, useState } from "react" | ||
| import { RelativeTime } from "@/components/relative-time" | ||
| import { UserAvatar } from "@/components/user-avatar" | ||
| import type { AuthorInfo } from "./comment-thread" | ||
| import { useToc } from "./toc-context" | ||
|
|
||
| type CommentTocItem = { | ||
| id: string | ||
| commentNumber: string | ||
| author: AuthorInfo | ||
| createdAt: number | ||
| isRoot: boolean | ||
| } | ||
|
|
||
| type CommentsTocProps = { | ||
| items: CommentTocItem[] | ||
| } | ||
|
|
||
| export function CommentsToc({ items }: CommentsTocProps) { | ||
| const [isHovered, setIsHovered] = useState(false) | ||
| const { activeCommentId } = useToc() | ||
|
|
||
| if (items.length === 0) return null | ||
|
|
||
| function scrollToComment(commentNumber: string) { | ||
| const el = document.getElementById(commentNumber) | ||
| if (el) { | ||
| el.scrollIntoView({ behavior: "smooth", block: "start" }) | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <nav | ||
| className="flex flex-col gap-0.5" | ||
| onMouseEnter={() => setIsHovered(true)} | ||
| onMouseLeave={() => setIsHovered(false)} | ||
| > | ||
| {items.map((item) => { | ||
| const isActive = activeCommentId === item.commentNumber | ||
|
|
||
| if (isHovered) { | ||
| return ( | ||
| <button | ||
| className={`group flex items-center gap-2 rounded px-1.5 py-1 text-left transition-colors ${ | ||
| isActive ? "bg-muted/30" : "hover:bg-muted/20" | ||
| }`} | ||
| key={item.id} | ||
| onClick={() => scrollToComment(item.commentNumber)} | ||
| type="button" | ||
| > | ||
| <div className="shrink-0"> | ||
| <UserAvatar | ||
| size={18} | ||
| src={item.author.image} | ||
| username={item.author.username} | ||
| /> | ||
| </div> | ||
| <div className="min-w-0 flex-1"> | ||
| <div | ||
| className={`truncate text-xs ${isActive ? "font-medium text-bright" : "text-dim group-hover:text-bright"}`} | ||
| > | ||
| {item.author.name} | ||
| </div> | ||
| <div className="text-faint text-xs"> | ||
| <Suspense> | ||
| <RelativeTime timestamp={item.createdAt} /> | ||
| </Suspense> | ||
| </div> | ||
| </div> | ||
| </button> | ||
| ) | ||
| } | ||
|
|
||
| return ( | ||
| <button | ||
| className="group flex items-center py-1.5" | ||
| key={item.id} | ||
| onClick={() => scrollToComment(item.commentNumber)} | ||
| type="button" | ||
| > | ||
| <div | ||
| className={`h-px transition-all ${ | ||
| isActive | ||
| ? "w-6 bg-bright" | ||
| : "w-3 bg-faint group-hover:w-4 group-hover:bg-muted" | ||
| }`} | ||
| /> | ||
| </button> | ||
| ) | ||
| })} | ||
| </nav> | ||
| ) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| "use client" | ||
|
|
||
| import { useToc } from "./toc-context" | ||
|
|
||
| export function HeadingsToc() { | ||
| const { headings, activeHeadingId, activeCommentId } = useToc() | ||
|
|
||
| const commentHeadings = headings.filter((h) => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When activeCommentId is null, the filter uses "null-" as a string prefix, which is a logic bug despite accidentally working correctly View Details📝 Patch Detailsdiff --git a/app/[owner]/[repo]/[postNumber]/headings-toc.tsx b/app/[owner]/[repo]/[postNumber]/headings-toc.tsx
index 7f5d0e8..6a381c6 100644
--- a/app/[owner]/[repo]/[postNumber]/headings-toc.tsx
+++ b/app/[owner]/[repo]/[postNumber]/headings-toc.tsx
@@ -5,9 +5,9 @@ import { useToc } from "./toc-context"
export function HeadingsToc() {
const { headings, activeHeadingId, activeCommentId } = useToc()
- const commentHeadings = headings.filter((h) =>
- h.id.startsWith(`${activeCommentId}-`)
- )
+ const commentHeadings = activeCommentId
+ ? headings.filter((h) => h.id.startsWith(`${activeCommentId}-`))
+ : []
if (commentHeadings.length === 0) return null
diff --git a/package.json b/package.json
index 95574f4..c063982 100644
--- a/package.json
+++ b/package.json
@@ -71,7 +71,7 @@
"atmn": "^0.0.30",
"drizzle-kit": "^0.31.8",
"tailwindcss": "^4",
- "typescript": "^5",
+ "typescript": "^5.9.3",
"ultracite": "7.0.5"
}
}
AnalysisBug ExplanationThe issue occurs in const commentHeadings = headings.filter((h) =>
h.id.startsWith(`${activeCommentId}-`)
)When Heading IDs are formatted as Why This Is a Bug
The FixChanged to explicitly check if const commentHeadings = activeCommentId
? headings.filter((h) => h.id.startsWith(`${activeCommentId}-`))
: []This:
|
||
| h.id.startsWith(`${activeCommentId}-`) | ||
| ) | ||
|
|
||
| if (commentHeadings.length === 0) return null | ||
|
|
||
| function scrollToHeading(id: string) { | ||
| const el = document.getElementById(id) | ||
| if (el) { | ||
| el.scrollIntoView({ behavior: "smooth", block: "start" }) | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <nav className="flex flex-col gap-0.5"> | ||
| <span className="mb-2 text-faint text-xs uppercase tracking-wide"> | ||
| On this comment | ||
| </span> | ||
| {commentHeadings.map((heading) => { | ||
| const isActive = activeHeadingId === heading.id | ||
| const indent = (heading.level - 1) * 12 | ||
|
|
||
| return ( | ||
| <button | ||
| className={`group flex items-center py-0.5 text-left text-sm transition-colors ${ | ||
| isActive | ||
| ? "font-medium text-bright" | ||
| : "text-muted hover:text-dim" | ||
| }`} | ||
| key={heading.id} | ||
| onClick={() => scrollToHeading(heading.id)} | ||
| style={{ paddingLeft: indent }} | ||
| type="button" | ||
| > | ||
| <span className="truncate">{heading.text}</span> | ||
| </button> | ||
| ) | ||
| })} | ||
| </nav> | ||
| ) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Text extraction from heading children fails when headings contain React elements (code, em, strong, etc), resulting in missing heading IDs and incomplete TOC entries
View Details
📝 Patch Details
Analysis
Bug Details
The original code at lines 37-41 used a simplistic approach to extract text from heading children:
This only extracted string-type children and ignored React elements, causing failures in these scenarios:
<h2><code>example</code></h2>→ extracted "" instead of "example"<h2>Getting <code>Started</code></h2>→ extracted "Getting " instead of "Getting Started"<h2><strong>Bold</strong> <em>italic</em></h2>→ extracted "" instead of "Bold italic"The issue manifests when:
Started"When text extraction fails (returns empty string), the heading ID becomes empty after slugification, and the heading is not properly registered in the table of contents.
Fix Implementation
Added a recursive
extractTextFromChildrenfunction that:childrenproperty directly on the object, or looks forprops.children(typical React element structure)The fix properly handles nested React elements like:
<code>,<strong>,<em>,<a>and other semantic elementsThe solution is type-safe (uses
unknownand runtime checks) and prevents errors from null/undefined values.