fix: tabs in blogs & hydration error fixes#832
Conversation
✅ Deploy Preview for tanstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe changes implement a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/DocFeedbackProvider.tsx (1)
397-414:⚠️ Potential issue | 🟠 MajorPortal containers are not cleaned up on unmount, causing DOM element leaks.
When
BlockButtonunmounts (e.g., mouse leaves the block), the dynamically created portal container remains in the DOM. Similarly forNotePortalandCreatingFeedbackPortal. Over time, repeated interactions will accumulate orphaned DOM elements.Add cleanup logic to remove the portal container when the component unmounts.
🐛 Proposed fix for BlockButton
return ReactDOM.createPortal( <DocFeedbackFloatingButton onAddNote={onAddNote} onAddFeedback={onAddFeedback} hasNote={hasNote} hasImprovement={hasImprovement} onShowNote={onShowNote} isMenuOpen={isMenuOpen} onMenuOpenChange={onMenuOpenChange} />, portalContainer, ) + + // Add useEffect for cleanup }Apply a similar pattern to add cleanup - consider using a
useEffectthat returns a cleanup function:// Inside BlockButton, after creating portalContainer: React.useEffect(() => { return () => { const container = block.querySelector(`[data-button-portal="${blockId}"]`) container?.remove() } }, [block, blockId])Apply the same cleanup pattern to
NotePortal(lines 463-470) andCreatingFeedbackPortal(lines 525-532).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DocFeedbackProvider.tsx` around lines 397 - 414, BlockButton, NotePortal, and CreatingFeedbackPortal create dynamic portalContainer elements (data-button-portal="${blockId}") but never remove them, leaking DOM nodes; add a cleanup that removes the created container on unmount: after creating portalContainer in each component (BlockButton, NotePortal, CreatingFeedbackPortal) wrap the creation in a React.useEffect (or add a componentWillUnmount equivalent) that returns a cleanup function which queries for the container by data-button-portal using the blockId and calls .remove() — make the effect depend on block and blockId so the container is removed when the component unmounts or dependencies change.
🧹 Nitpick comments (2)
src/components/DocFeedbackProvider.tsx (2)
178-179: Consider moving state reset after the new effect initializes to avoid potential flicker.Clearing
blockElementsin cleanup means there's a brief window where the state is empty before the new effect repopulates it. During this window, portals won't render since they depend onblockElements.get(blockId)returning a value.This may cause a brief visual flicker when
childrenchanges. If this becomes noticeable, consider using a ref to track the "old" blocks for cleanup instead of clearing state synchronously.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DocFeedbackProvider.tsx` around lines 178 - 179, The cleanup in DocFeedbackProvider currently clears blockElements immediately (return () => setBlockElements(new Map())), causing a brief empty state/flicker; instead, avoid wiping state in the cleanup — either keep cleanup empty and compute the new blockElements in the effect before calling setBlockElements, or use a ref (e.g., prevBlockElementsRef) to hold the prior map and only replace state once the new map is ready; update the effect that populates blockElements (inside DocFeedbackProvider) to build the next Map first and then call setBlockElements(nextMap) so portals never see an empty map during transitions.
337-350: Consider adding user feedback when block element is unavailable.If
blockElements.get(creatingState.blockId)returns undefined,CreatingFeedbackPortalsilently returns null. This could happen during the brief window whenblockElementsis being repopulated. The user might have clicked to create feedback but see nothing happen.This is a minor edge case, but consider either preventing the creating state from being set when the block isn't available, or showing a brief loading indicator.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DocFeedbackProvider.tsx` around lines 337 - 350, The CreatingFeedbackPortal can silently render null when blockElements.get(creatingState.blockId) is undefined; update the render logic for CreatingFeedbackPortal so it checks the resolved block first and shows a short loading state or spinner when undefined (e.g., render a Loading/Placeholder component) instead of nothing, or alternatively prevent setting creatingState until blockElements contains the block (ensure the creator flow that sets creatingState verifies blockElements.has(blockId)). Target the JSX around CreatingFeedbackPortal and the flow that sets creatingState so the UI either displays a Loading indicator while blockElements is being repopulated or never enters the creatingState until the block is present; keep handleCloseCreating behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/markdown/processor.rsc.tsx`:
- Around line 173-177: The anchor link properties currently set ariaHidden: true
and tabIndex: -1 which prevents keyboard and screen-reader access; update the
properties object for the anchor elements (the entry with className
['anchor-heading', 'anchor-heading-link']) by removing the ariaHidden and
tabIndex keys so the links remain focusable and exposed to assistive
technologies, and ensure the element still has an accessible label (add or
verify an aria-label or descriptive text elsewhere if needed).
---
Outside diff comments:
In `@src/components/DocFeedbackProvider.tsx`:
- Around line 397-414: BlockButton, NotePortal, and CreatingFeedbackPortal
create dynamic portalContainer elements (data-button-portal="${blockId}") but
never remove them, leaking DOM nodes; add a cleanup that removes the created
container on unmount: after creating portalContainer in each component
(BlockButton, NotePortal, CreatingFeedbackPortal) wrap the creation in a
React.useEffect (or add a componentWillUnmount equivalent) that returns a
cleanup function which queries for the container by data-button-portal using the
blockId and calls .remove() — make the effect depend on block and blockId so the
container is removed when the component unmounts or dependencies change.
---
Nitpick comments:
In `@src/components/DocFeedbackProvider.tsx`:
- Around line 178-179: The cleanup in DocFeedbackProvider currently clears
blockElements immediately (return () => setBlockElements(new Map())), causing a
brief empty state/flicker; instead, avoid wiping state in the cleanup — either
keep cleanup empty and compute the new blockElements in the effect before
calling setBlockElements, or use a ref (e.g., prevBlockElementsRef) to hold the
prior map and only replace state once the new map is ready; update the effect
that populates blockElements (inside DocFeedbackProvider) to build the next Map
first and then call setBlockElements(nextMap) so portals never see an empty map
during transitions.
- Around line 337-350: The CreatingFeedbackPortal can silently render null when
blockElements.get(creatingState.blockId) is undefined; update the render logic
for CreatingFeedbackPortal so it checks the resolved block first and shows a
short loading state or spinner when undefined (e.g., render a
Loading/Placeholder component) instead of nothing, or alternatively prevent
setting creatingState until blockElements contains the block (ensure the creator
flow that sets creatingState verifies blockElements.has(blockId)). Target the
JSX around CreatingFeedbackPortal and the flow that sets creatingState so the UI
either displays a Loading indicator while blockElements is being repopulated or
never enters the creatingState until the block is present; keep
handleCloseCreating behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2305b0a-7fbe-4dfa-82d7-1b9f41891919
📒 Files selected for processing (7)
src/components/DocFeedbackProvider.tsxsrc/components/ThemeProvider.tsxsrc/components/markdown/MdComponents.tsxsrc/styles/app.csssrc/utils/blog.functions.tssrc/utils/markdown/processor.rsc.tsxsrc/utils/markdown/renderRsc.tsx
| properties: { | ||
| className: ['anchor-heading'], | ||
| ariaHidden: true, | ||
| className: ['anchor-heading', 'anchor-heading-link'], | ||
| tabIndex: -1, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -200 src/utils/markdown/processor.rsc.tsx | tail -50Repository: TanStack/tanstack.com
Length of output: 1402
🏁 Script executed:
rg -i 'anchor-heading' --type css --type scss -A 3 -B 1Repository: TanStack/tanstack.com
Length of output: 93
🏁 Script executed:
rg 'anchor-heading' -A 3 -B 1 | head -50Repository: TanStack/tanstack.com
Length of output: 2189
🏁 Script executed:
rg -i 'a11y|accessibility|aria' --type json --type ts --type tsx | head -30Repository: TanStack/tanstack.com
Length of output: 92
🏁 Script executed:
rg 'a11y|accessibility|aria' -i | head -20Repository: TanStack/tanstack.com
Length of output: 2080
Remove ariaHidden and tabIndex to make anchor links accessible via keyboard and screen readers.
The ariaHidden: true attribute hides these links from assistive technologies, and tabIndex: -1 removes keyboard access. This blocks users who rely on keyboard navigation or screen readers from using section permalinks.
Suggested fix
.use(rehypeAutolinkHeadings, {
behavior: 'append',
content: {
type: 'text',
value: '#',
},
properties: {
- ariaHidden: true,
className: ['anchor-heading', 'anchor-heading-link'],
- tabIndex: -1,
+ ariaLabel: 'Link to section',
},
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| properties: { | |
| className: ['anchor-heading'], | |
| ariaHidden: true, | |
| className: ['anchor-heading', 'anchor-heading-link'], | |
| tabIndex: -1, | |
| }, | |
| properties: { | |
| className: ['anchor-heading', 'anchor-heading-link'], | |
| ariaLabel: 'Link to section', | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/markdown/processor.rsc.tsx` around lines 173 - 177, The anchor link
properties currently set ariaHidden: true and tabIndex: -1 which prevents
keyboard and screen-reader access; update the properties object for the anchor
elements (the entry with className ['anchor-heading', 'anchor-heading-link']) by
removing the ariaHidden and tabIndex keys so the links remain focusable and
exposed to assistive technologies, and ensure the element still has an
accessible label (add or verify an aria-label or descriptive text elsewhere if
needed).
2 participants
Summary by CodeRabbit
Release Notes
Improvements
Bug Fixes