Docs: Add test proving scroll is reset across nav#34973
Conversation
Sidnioulz
added
addon: docs
build
|
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 (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdded a Playwright E2E test that scrolls the docs preview iframe, navigates to another docs story via the sidebar, and verifies the iframe's scroll position resets to zero. ChangesDocs Preview Scroll Reset Verification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end regression test to confirm that navigating between Docs pages resets the preview iframe’s scroll position (covering the behavior reported in #12497).
Changes:
- Add Playwright e2e test that scrolls a long Docs page, navigates to another Docs entry via the sidebar, and asserts scroll resets to top.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 74 | 74 | 0 |
| Self size | 20.43 MB | 20.40 MB | 🎉 -26 KB 🎉 |
| Dependency size | 36.65 MB | 36.65 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 205 | 205 | 0 |
| Self size | 908 KB | 908 KB | 🎉 -144 B 🎉 |
| Dependency size | 89.16 MB | 89.13 MB | 🎉 -26 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 198 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +36 B 🚨 |
| Dependency size | 87.65 MB | 87.62 MB | 🎉 -26 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 75 | 75 | 0 |
| Self size | 1.08 MB | 1.08 MB | 0 B |
| Dependency size | 57.08 MB | 57.05 MB | 🎉 -26 KB 🎉 |
| Bundle Size Analyzer | node | node |
Sidnioulz
force-pushed
the
sidnioulz/e2e-test-scroll-reset
branch
from
4066755 to
28c385f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/e2e-sandbox/addon-docs.spec.ts (1)
293-293: ⚡ Quick winRemove redundant page navigation.
The
beforeEachhook at line 14 already navigates tostorybookUrl. This duplicate navigation is unnecessary and adds latency to the test. Other tests in this file (e.g., lines 28-29, 49-50, 76-77) create a newSbPageinstance without re-navigating.♻️ Proposed fix
test('should reset scroll position between pages', async ({ page }) => { - await page.goto(storybookUrl); const sbPage = new SbPage(page, expect);🤖 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/e2e-sandbox/addon-docs.spec.ts` at line 293, Remove the redundant navigation call by deleting the await page.goto(storybookUrl); statement in the test; rely on the existing beforeEach navigation to storybookUrl and instantiate SbPage (or use the existing page variable) without re-calling page.goto — locate the statement referencing page.goto(storybookUrl) in the test and remove it so the test uses the prior beforeEach setup.
🤖 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.
Nitpick comments:
In `@code/e2e-sandbox/addon-docs.spec.ts`:
- Line 293: Remove the redundant navigation call by deleting the await
page.goto(storybookUrl); statement in the test; rely on the existing beforeEach
navigation to storybookUrl and instantiate SbPage (or use the existing page
variable) without re-calling page.goto — locate the statement referencing
page.goto(storybookUrl) in the test and remove it so the test uses the prior
beforeEach setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bef3855e-3870-42fd-bd5a-7969e113394f
📒 Files selected for processing (1)
code/e2e-sandbox/addon-docs.spec.ts
Confirms #12497 is fixed
What I did
Added e2e test proving that when you navigate to a new docs page, scroll position gets reset.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Run e2e test suite or wait for CI.
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit