Docs: Fix Hash Anchor Scrolling#35064
Conversation
|
Updated the title to match the expected format. The remaining Danger failures are label-gate items I cannot set from the fork: bug, ci:normal, and qa:skip. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR updates MDX anchor and heading components to scroll to in-page targets directly instead of emitting navigation events. The ChangesIn-page scroll behavior for MDX anchors and headings
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/addons/docs/src/blocks/blocks/mdx.tsx (1)
212-219:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the heading anchor aria label to match actual behavior.
The button no longer updates the URL/hash (default is prevented); it scrolls in-page instead.
"Copy heading URL to address bar"is now misleading for assistive tech.Suggested patch
- ariaLabel="Copy heading URL to address bar" + ariaLabel="Scroll to heading"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/docs/src/blocks/blocks/mdx.tsx` around lines 212 - 219, The aria label "Copy heading URL to address bar" is inaccurate because the anchor's onClick prevents default and scrolls in-page; update the aria label on the anchor element (the <a> with href={hash} and onClick={(event: MouseEvent<HTMLAnchorElement>) => { event.preventDefault(); const element = document.getElementById(id); ... }}) to reflect the actual behavior (e.g., "Scroll to heading" or "Jump to heading") so assistive tech describes the action correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/addons/docs/src/blocks/blocks/mdx.test.tsx`:
- Around line 146-169: The test patches Element.prototype.scrollIntoView but
restores it only at the end, so an early throw would leave the global patched;
fix by wrapping the patch, render, interactions, and assertions in a try/finally
block where originalScrollIntoView (captured before assignment) is restored in
the finally clause; reference the local variables scrollIntoView and
originalScrollIntoView and the test code that calls render(<HeaderMdx ...>),
queries the anchor, fireEvent.click(anchor!), and asserts scrollIntoView calls —
move those steps inside try and place Element.prototype.scrollIntoView =
originalScrollIntoView in finally.
- Around line 13-59: Add the { spy: true } option to each vi.mock(...) call (the
mocks for './DocsContext', '../components', 'storybook/internal/components',
'`@storybook/icons`', and 'storybook/theming') and update usages to access mocked
exports via vi.mocked(...) for type safety; specifically wrap the mock factory
returns so callers can do const MockedDocsContext = vi.mocked(DocsContext) or
vi.mocked(LinkIcon) as needed, and keep the existing mock implementations for
Source, MockAnchor, styledFactory/styled, Button/Code/components, and LinkIcon
unchanged except for switching to vi.mock(..., { spy: true }) and using
vi.mocked(...) where the tests read from those modules.
---
Outside diff comments:
In `@code/addons/docs/src/blocks/blocks/mdx.tsx`:
- Around line 212-219: The aria label "Copy heading URL to address bar" is
inaccurate because the anchor's onClick prevents default and scrolls in-page;
update the aria label on the anchor element (the <a> with href={hash} and
onClick={(event: MouseEvent<HTMLAnchorElement>) => { event.preventDefault();
const element = document.getElementById(id); ... }}) to reflect the actual
behavior (e.g., "Scroll to heading" or "Jump to heading") so assistive tech
describes the action correctly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b86bbcf5-03e2-49c6-b300-dd3309af79cf
📒 Files selected for processing (2)
code/addons/docs/src/blocks/blocks/mdx.test.tsxcode/addons/docs/src/blocks/blocks/mdx.tsx
| vi.mock('./DocsContext', () => ({ | ||
| DocsContext: React.createContext({ | ||
| channel: { emit: emitMock }, | ||
| }), | ||
| })); | ||
|
|
||
| vi.mock('../components', () => ({ | ||
| Source: ({ children }: any) => <pre>{children}</pre>, | ||
| })); | ||
|
|
||
| vi.mock('storybook/internal/components', () => { | ||
| const MockAnchor = React.forwardRef<HTMLAnchorElement, any>(({ children, ...props }, ref) => ( | ||
| <a ref={ref} {...props}> | ||
| {children} | ||
| </a> | ||
| )); | ||
| MockAnchor.displayName = 'MockAnchor'; | ||
|
|
||
| return { | ||
| Button: ({ children }: any) => <>{children}</>, | ||
| Code: ({ children }: any) => <code>{children}</code>, | ||
| components: { | ||
| a: MockAnchor, | ||
| }, | ||
| nameSpaceClassNames: (props: any) => props, | ||
| }; | ||
| }); | ||
|
|
||
| vi.mock('@storybook/icons', () => ({ | ||
| LinkIcon: () => <span data-testid="link-icon" />, | ||
| })); | ||
|
|
||
| vi.mock('storybook/theming', () => { | ||
| const styledFactory = (tag: React.ElementType) => () => { | ||
| const Styled = React.forwardRef<any, any>(({ children, ...props }, ref) => | ||
| React.createElement(tag, { ...props, ref }, children) | ||
| ); | ||
| Styled.displayName = `Styled${typeof tag === 'string' ? tag : 'Component'}`; | ||
| return Styled; | ||
| }; | ||
|
|
||
| return { | ||
| styled: new Proxy(styledFactory, { | ||
| get: (_target, tag) => styledFactory(tag as string), | ||
| }), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Update Vitest module mocks to match the repo spy-mocking contract in code/addons/docs/src/blocks/blocks/mdx.test.tsx.
Add the required { spy: true } option to the vi.mock() calls (e.g., ./DocsContext, ../components, storybook/internal/components, @storybook/icons, storybook/theming) and use vi.mocked() for type-safe access to the mocked exports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/addons/docs/src/blocks/blocks/mdx.test.tsx` around lines 13 - 59, Add
the { spy: true } option to each vi.mock(...) call (the mocks for
'./DocsContext', '../components', 'storybook/internal/components',
'`@storybook/icons`', and 'storybook/theming') and update usages to access mocked
exports via vi.mocked(...) for type safety; specifically wrap the mock factory
returns so callers can do const MockedDocsContext = vi.mocked(DocsContext) or
vi.mocked(LinkIcon) as needed, and keep the existing mock implementations for
Source, MockAnchor, styledFactory/styled, Button/Code/components, and LinkIcon
unchanged except for switching to vi.mock(..., { spy: true }) and using
vi.mocked(...) where the tests read from those modules.
|
Addressed the review follow-up in the latest commit: the heading anchor label now matches the scroll behavior, and the test restores the patched scrollIntoView prototype in a finally block. I left the mock spy/type-safety suggestion unchanged since these mocks are local stubs and the focused test coverage still exercises the changed behavior.\n\nFocused docs test passes locally. |
Sidnioulz
added
bug
addon: docs
ci:normal
2 participants
Summary
Fixes #15934
Testing
Manual testing
Summary by CodeRabbit
Tests
Bug Fixes