fix: ensure unique TOC slugs for repeated headings(Examples) #4593
Conversation
Sourya07
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughTOC item interface now accepts an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as TOC Component
participant Item as TOC Item
participant Slug as Slug Generator
participant DOM as Anchor/Heading
UI->>Item: read item.content and item.seen
Note right of Slug `#DDEBF7`: sanitize content\n(strip special chars, spaces→hyphens, lowercase)
Item->>Slug: provide content
Slug-->>Slug: compute baseSlug
alt item.seen > 0
Slug-->>Slug: append "-<seen>" suffix
end
Slug->>DOM: emit `slugWithATag` for id/href
DOM->>UI: render anchor/heading with computed slug
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4593--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/TOC.tsx (1)
41-53: Slug uniqueness logic is solid; consider typingseenand extracting a helperThe new
slugWithATaglogic correctly ensures uniqueness by appending-<seen>when the item has been seen before, which should address the duplicate TOC anchor issue for repeated headings.Two small improvements you might consider:
- Type safety: instead of
(item as any).seen, extend the TOC item shape to include an optionalseen?: numberand drop theanycast:- toc: { - lvl: number; - content: string; - slug: string; - }[]; + toc: { + lvl: number; + content: string; + slug: string; + seen?: number; + }[];and then:
- if (typeof (item as any).seen === 'number' && (item as any).seen > 0) { - return `${base}-${(item as any).seen}`; - } + if (typeof item.seen === 'number' && item.seen > 0) { + return `${base}-${item.seen}`; + }
- Reuse/consistency: if the same slugging rules exist wherever the actual heading
ids are generated, extracting this IIFE into a shared helper (e.g.,slugFromHeading(content, seen?)) would reduce the risk of the TOC getting out of sync with the DOM IDs over time.Overall, the behavior change looks good; these are just type-safety and maintainability tweaks.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/TOC.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/TOC.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: cypress-run
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Lighthouse CI
🔇 Additional comments (1)
components/TOC.tsx (1)
68-72: Heading className formatting change is safeThe updated template string for the
<h5>classes preserves the previous Tailwind behavior (only whitespace/newline changes plus the same conditional xl vs lg classes). No functional issues here.
|
Hi @princerajpoot20, |
|
@Sourya07 Can you please have a look at the nitpick comments mentioned above |
|
@princerajpoot20 |
Co-authored-by: Prince Rajpoot <[email protected]>
princerajpoot20
left a comment
princerajpoot20
left a comment
There was a problem hiding this comment.
Added a small suggestion. The rest looks good.
Now it has been fixed . |
|
Pipeline is failing. Please have a look. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4593 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 799 799
Branches 146 146
=========================================
Hits 799 799 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
All the checks has been cleared |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/TOC.tsx (2)
7-15: Document and stabilize theseensemantics on TOC itemsThe
seen?: numberaddition is a clean way to support disambiguating duplicate headings, and the rest of the code treats it as an occurrence counter (> 0meaning “duplicate”). To make this easier to use correctly from callers, consider briefly documenting the expected semantics (e.g., “0 for first occurrence, 1 for second, …”) where the TOC items are constructed or in a short JSDoc on the interface.This is non-blocking but will help future maintainers wire the counter consistently.
60-63: TightentwMergeusage to avoid"undefined"classes and improve readabilityUsing
twMergehere is fine, but interpolatingclassNamedirectly into a template string can still produce a literal"undefined"class whenclassNameis not provided. It also makes it a bit harder to see which classes are conditional vs static.Consider restructuring to pass arguments directly to
twMerge, and guardingclassNameseparately:- <div - className={twMerge( - `${className} ${tocItems.length ? '' : 'hidden'} - ${cssBreakingPoint === 'xl' ? 'xl:block' : 'lg:block'} md:top-24 md:max-h-(screen-14) mb-4 z-20` - )} + <div + className={twMerge( + className, + tocItems.length ? '' : 'hidden', + cssBreakingPoint === 'xl' ? 'xl:block' : 'lg:block', + 'md:top-24 md:max-h-(screen-14) mb-4 z-20' + )}You can apply the same pattern to the
<h5>classNameblock to keep conditional bits clearer and avoid accidental"undefined"there as well. This is a readability/robustness improvement, not a blocker.Also applies to: 71-77
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/TOC.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
📚 Learning: 2024-10-11T07:38:35.745Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/newsroom/FeaturedBlogPost.tsx:90-92
Timestamp: 2024-10-11T07:38:35.745Z
Learning: In Next.js, nested `<a>` tags cause hydration issues due to invalid HTML. To fix this, avoid nesting `<a>` tags by replacing inner `<a>` tags with non-interactive elements like `<span>`, and use click handlers to maintain interactivity if needed.
Applied to files:
components/TOC.tsx
📚 Learning: 2024-10-11T10:46:58.882Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/Avatar.tsx:35-44
Timestamp: 2024-10-11T10:46:58.882Z
Learning: In Next.js, when avoiding nested `<a>` tags due to hydration issues, use accessible non-link elements like `<button>` or `<span>` with appropriate roles, `tabIndex`, and event handlers to maintain accessibility and SEO.
Applied to files:
components/TOC.tsx
📚 Learning: 2024-10-11T11:32:30.226Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3262
File: components/navigation/BlogPostItem.tsx:95-119
Timestamp: 2024-10-11T11:32:30.226Z
Learning: In the `BlogPostItem` component (`components/navigation/BlogPostItem.tsx`), nesting `<a>` tags inside the parent `Link` component leads to hydration issues; therefore, we should avoid nesting `<a>` tags inside `Link` components in this component.
Applied to files:
components/TOC.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (1)
components/TOC.tsx (1)
31-56: Slug generation withseensuffix looks correct and robustThe
minLevelcalculation andtocItemsmapping look fine, and the newslugWithATagIIFE:
- Builds a normalized
baseslug fromitem.content.- Appends
-seenonly whenseenis a number and strictly greater than 0.- Leaves the first occurrence unsuffixed, which matches the examples → examples-1 → examples-2 behavior described in the PR.
The explicit
typeof item.seen === 'number'guard also makes this resilient to any unexpected runtime data.No changes needed here from my side.
Co-authored-by: Prince Rajpoot <[email protected]>
Co-authored-by: Prince Rajpoot <[email protected]>
|
@sambhavgupta0705 PTAL |
|
Thanks for reviewing this, @princerajpoot20 and @sambhavgupta0705 🙏 |
|
Hey @sambhavgupta0705, |
Sourya07
changed the title
|
@princerajpoot20 |
4 participants
Description
• Fixes an issue where multiple headings with the same name (e.g: Examples) generated identical slugs in the TOC.
• This caused incorrect navigation behavior in the right sidebar (TOC), especially on pages like the Numbers Style Guide, where several “Examples” sections exist
• The fix appends a numeric suffix to duplicate headings (based on seen count), ensuring each TOC entry has a unique slugWithATag
• This resolves broken anchor navigation and scrollspy highlighting.
fixes this ->#4591
What this PR changes
Summary by CodeRabbit
New Features
Bug Fixes
Style