Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a Table of Contents (ToC) feature for blog posts, providing a sticky sidebar navigation for h2 and h3 headings with scroll-spy functionality. The implementation follows the documented plan in TOC_IMPLEMENTATION_PLAN.md and includes comprehensive test coverage.
Changes:
- Added two custom hooks (
useTableOfContentsanduseActiveHeading) for heading extraction and scroll-spy functionality - Created a
TableOfContentscomponent with automatic ID generation, German umlaut handling, and responsive design - Refactored
Post.tsxlayout to use a 3-column grid with centered content and right sidebar for ToC - Added comprehensive test suites for all new hooks and components
- Extended global test setup with IntersectionObserver mock
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| website/hooks/useTableOfContents.ts | New hook that extracts h2/h3 headings from DOM and generates URL-safe IDs with German umlaut support |
| website/hooks/useActiveHeading.ts | New hook implementing scroll-spy with IntersectionObserver to track visible heading |
| website/components/TableOfContents.tsx | New component rendering sticky ToC sidebar with active state highlighting and smooth scrolling |
| website/components/Post.tsx | Refactored to use 3-column grid layout and integrate ToC sidebar with content ref passing |
| website/layouts/styles.ts | Added ToC styles and 3-column article layout with responsive breakpoints |
| website/test/useTableOfContents.test.ts | Comprehensive tests for heading extraction, ID generation, and edge cases |
| website/test/useActiveHeading.test.ts | Tests for scroll-spy functionality with IntersectionObserver mocking |
| website/test/TableOfContents.test.tsx | Component integration tests for rendering and click handling |
| website/test/setup.ts | Added global IntersectionObserver mock for test environment |
| website/TOC_IMPLEMENTATION_PLAN.md | Documentation of implementation phases and requirements |
website/hooks/useActiveHeading.ts
Outdated
| return () => { | ||
| observer.disconnect(); | ||
| }; | ||
| }, [headingIds, activeId]); |
There was a problem hiding this comment.
Including activeId in the dependency array causes the effect to re-run every time the active heading changes. This recreates the IntersectionObserver unnecessarily and defeats the purpose of the if (!activeId && headingIds.length > 0) check on line 60, which is meant to only set the initial value. Remove activeId from the dependency array to prevent unnecessary re-initialization of the observer.
| }, [headingIds, activeId]); | |
| }, [headingIds]); |
| */ | ||
| export function TableOfContents({ contentRef, minHeadings = 2, title = "On this page" }: TableOfContentsProps) { | ||
| const headings = useTableOfContents(contentRef); | ||
| const { activeId, setActiveId } = useActiveHeading(headings.map((h) => h.id)); |
There was a problem hiding this comment.
The headings.map((h) => h.id) expression creates a new array on every render, causing useActiveHeading's effect to re-run unnecessarily. Wrap this in a useMemo to maintain referential equality when the heading IDs haven't actually changed. Example: const headingIds = useMemo(() => headings.map((h) => h.id), [headings]);
website/hooks/useActiveHeading.ts
Outdated
| return () => { | ||
| observer.disconnect(); | ||
| }; | ||
| }, [headingIds, activeId]); |
There was a problem hiding this comment.
The dependency array includes headingIds which is an array. Since arrays are compared by reference in React, this could cause unnecessary re-runs of the effect even when the heading IDs haven't changed. Consider using a more stable approach such as converting the array to a string for comparison, or using a deep equality check with a custom hook like useDeepCompareEffect.
1 participant
No description provided.