Vitest: Reset playwright cursor position to avoid hover bug#34765
Vitest: Reset playwright cursor position to avoid hover bug#34765Sidnioulz wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Vitest plugin command ChangesMouse Position Reset Integration
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant Setup as resetMousePositionBeforeTests
participant BrowserCommands as vitest/browser.commands
participant PlaywrightPage as Playwright.page
TestRunner->>Setup: beforeEach()
Setup->>BrowserCommands: import commands
alt commands.resetMousePosition is function
Setup->>BrowserCommands: await commands.resetMousePosition()
BrowserCommands->>PlaywrightPage: page.mouse.move(-1000, -1000)
else module missing
Setup->>@vitest/browser/context.commands: import commands (fallback)
alt fallback.commands.resetMousePosition is function
Setup->>@vitest/browser/context.commands: await commands.resetMousePosition()
`@vitest/browser/context.commands-`>>PlaywrightPage: page.mouse.move(-1000, -1000)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![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/addons/vitest/templates/vitest.config.template.ts (1)
12-17: ⚡ Quick winUse a zero-argument
BrowserCommandsignature here.Line 12 declares
BrowserCommand<[number, number]>, but the command implementation takes onlyctxand is called with no arguments (seecommands.resetMousePosition()in the setup-file). The generic should beBrowserCommand<[]>to match the actual payload.♻️ Proposed fix
-const resetMousePosition: BrowserCommand<[number, number]> = async (ctx) => { +const resetMousePosition: BrowserCommand<[]> = async (ctx) => { if (ctx.provider.name !== 'playwright') throw new Error('resetMousePosition requires the Playwright provider'); const frame = await ctx.frame(); await frame.page().mouse.move(-1000, -1000); };🤖 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/vitest/templates/vitest.config.template.ts` around lines 12 - 17, The BrowserCommand generic for resetMousePosition is incorrect: change its type from BrowserCommand<[number, number]> to BrowserCommand<[]> so the command signature matches its zero-argument implementation (resetMousePosition(ctx) is invoked with no payload). Update the declaration of resetMousePosition to use BrowserCommand<[]> and ensure any references or exports that rely on its type reflect the new generic.
🤖 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/addons/vitest/templates/vitest.config.template.ts`:
- Around line 12-17: The BrowserCommand generic for resetMousePosition is
incorrect: change its type from BrowserCommand<[number, number]> to
BrowserCommand<[]> so the command signature matches its zero-argument
implementation (resetMousePosition(ctx) is invoked with no payload). Update the
declaration of resetMousePosition to use BrowserCommand<[]> and ensure any
references or exports that rely on its type reflect the new generic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b2d4a15-b49f-4dd5-9901-25328eb6b62a
📒 Files selected for processing (4)
code/addons/vitest/src/vitest-plugin/setup-file.tscode/addons/vitest/templates/vitest.config.3.2.template.tscode/addons/vitest/templates/vitest.config.4.template.tscode/addons/vitest/templates/vitest.config.template.ts
There was a problem hiding this comment.
Pull request overview
This PR aims to make Vitest Browser (Playwright) runs more reliable on Chromium/Linux by forcing the initial mouse cursor position away from (0,0), preventing unintended :hover recalculations that can skew axe-core a11y results.
Changes:
- Added a
resetMousePositionPlaywright browser command to Vitest config templates. - Invoked
commands.resetMousePosition()from the addon’s internal Vitest setup file in abeforeEachhook.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| code/addons/vitest/templates/vitest.config.template.ts | Adds a Playwright browser command and registers it in test.browser.commands. |
| code/addons/vitest/templates/vitest.config.4.template.ts | Same as above for the Vitest 4 template variant. |
| code/addons/vitest/templates/vitest.config.3.2.template.ts | Same as above for the Vitest 3.2 template variant. |
| code/addons/vitest/src/vitest-plugin/setup-file.ts | Adds a beforeEach hook that conditionally calls the new browser command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sidnioulz
force-pushed
the
sidnioulz/reset-vitest-cursor-position
branch
from
53fcb3e to
ab58c52
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/addons/vitest/src/vitest-plugin/setup-file.ts (1)
40-53: ⚡ Quick winRemove unnecessary error handling and simplify the defensive check
Line 41's
inoperator is safe. Thecommandsexport is guaranteed to exist in both Vitest 3.x and 4.x, so the destructuring on Line 40 will always succeed. The catch block checking for"Cannot destructure property 'commands'"is dead code and can be removed. Simplify to safely check for theresetMousePositionmethod:Proposed fix
export const resetMousePositionBeforeTests = async () => { try { const { commands } = await import('vitest/browser'); - if ('resetMousePosition' in commands && isFunction(commands.resetMousePosition)) { + if (isFunction(commands.resetMousePosition)) { await commands.resetMousePosition(); } } catch (error) { // Ignore when vitest/browser is not found (Vitest 3) if (error instanceof Error && error.message.includes("Cannot find module 'vitest/browser'")) { return; } - // Ignore when commands is not exported by vitest/browser (Vitest 3) - if ( - error instanceof Error && - error.message.includes("Cannot destructure property 'commands'") - ) { - return; - }🤖 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/vitest/src/vitest-plugin/setup-file.ts` around lines 40 - 53, Remove the dead error check that looks for the "Cannot destructure property 'commands'" message in the catch block: keep the current dynamic import of 'vitest/browser' and the defensive runtime check for commands.resetMousePosition (the in operator and isFunction(commands.resetMousePosition)), but delete the entire conditional that tests error.message.includes("Cannot destructure property 'commands'") so the catch only ignores missing-module errors and otherwise rethrows or handles real errors; target the try/catch surrounding the import and the symbols commands and resetMousePosition.
🤖 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/addons/vitest/src/vitest-plugin/setup-file.ts`:
- Around line 40-53: Remove the dead error check that looks for the "Cannot
destructure property 'commands'" message in the catch block: keep the current
dynamic import of 'vitest/browser' and the defensive runtime check for
commands.resetMousePosition (the in operator and
isFunction(commands.resetMousePosition)), but delete the entire conditional that
tests error.message.includes("Cannot destructure property 'commands'") so the
catch only ignores missing-module errors and otherwise rethrows or handles real
errors; target the try/catch surrounding the import and the symbols commands and
resetMousePosition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12bbd42f-ce37-4159-ba2b-0b26502e97db
📒 Files selected for processing (2)
code/addons/vitest/src/vitest-plugin/setup-file.test.tscode/addons/vitest/src/vitest-plugin/setup-file.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/addons/vitest/src/vitest-plugin/setup-file.test.ts
AriPerkkio
left a comment
AriPerkkio
left a comment
There was a problem hiding this comment.
Looks good!
Test cases mock too much but that's out of scope of this PR. 🥹
| // Ignore when vitest/browser is not found not found (Vitest 3) | ||
| if (error instanceof Error && error.message.includes("Cannot find module 'vitest/browser'")) { | ||
| return; |
There was a problem hiding this comment.
Should it retry from @vitest/browser/context in this case?
There was a problem hiding this comment.
Ah, didn't know about that. Sure, I'll add this before merging.
There was a problem hiding this comment.
@copilot add a dynamic import using the vitest v3 import path (v3.vitest.dev/guide/browser/context) when the v4 import fails, and then fall back to the coded pathway. Add a unit test only if the code change is testable without major refactors of the test file.
There was a problem hiding this comment.
Implemented in 17754d5. The setup now retries with @vitest/browser/context when importing vitest/browser fails with module-not-found, then keeps the existing no-op fallback behavior for non-browser mode/missing-module cases. I also added a unit test covering the v3 fallback path.
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/vitest/src/vitest-plugin/setup-file.test.ts`:
- Line 119: The test currently uses a broad await
expect(resetMousePositionBeforeTests()).rejects.toThrow(); update it to assert
the specific error or message thrown by resetMousePositionBeforeTests() to avoid
false positives — for example, change the assertion to await
expect(resetMousePositionBeforeTests()).rejects.toThrowError(<expected message
or regex>) or toThrowError(<ExpectedErrorClass>) and, if needed, adjust the test
stub/mocked dependency that causes the throw so the expected message/class is
known; target the resetMousePositionBeforeTests() call when making this change.
- Around line 82-138: Add a brief inline comment in the test suite above the
per-test vi.doMock() usages explaining that vi.doMock() is intentionally used
per-test as an exception to the spy-mocking rules because the implementation
under test uses dynamic await import() with error handling and needs per-case
module resolution/behavior (reference the vi.doMock calls and the
resetMousePositionBeforeTests tests); mention that hoisted vi.mock(spy: true)
cannot express these per-test variations, and suggest documenting or opening a
discussion in the spy-mocking guidelines if desired.
🪄 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: 2861e223-dba4-4fd7-acea-894d018c1b54
📒 Files selected for processing (2)
code/addons/vitest/src/vitest-plugin/setup-file.test.tscode/addons/vitest/src/vitest-plugin/setup-file.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/addons/vitest/src/vitest-plugin/setup-file.ts
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| vi.doUnmock('vitest/browser'); | ||
| vi.doUnmock('@vitest/browser/context'); | ||
| }); | ||
|
|
||
| it('should reset the mouse position when the browser command exists', async () => { | ||
| const resetMousePosition = vi.fn().mockResolvedValue(undefined); | ||
|
|
||
| vi.doMock('vitest/browser', () => ({ | ||
| commands: { | ||
| resetMousePosition, | ||
| }, | ||
| })); | ||
|
|
||
| await resetMousePositionBeforeTests(); | ||
|
|
||
| expect(resetMousePosition).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('should do nothing when resetMousePosition is not callable', async () => { | ||
| vi.doMock('vitest/browser', () => ({ | ||
| commands: { | ||
| resetMousePosition: 'not-a-function', | ||
| }, | ||
| })); | ||
|
|
||
| await expect(resetMousePositionBeforeTests()).resolves.toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should rethrow unexpected errors', async () => { | ||
| const error = new Error('boom'); | ||
|
|
||
| vi.doMock('vitest/browser', () => { | ||
| throw error; | ||
| }); | ||
|
|
||
| await expect(resetMousePositionBeforeTests()).rejects.toThrow(); | ||
| }); | ||
|
|
||
| it('should fallback to vitest v3 browser context when vitest/browser is not found', async () => { | ||
| const resetMousePosition = vi.fn().mockResolvedValue(undefined); | ||
|
|
||
| vi.doMock('vitest/browser', () => { | ||
| const browser = {}; | ||
| Object.defineProperty(browser, 'commands', { | ||
| get: () => { | ||
| throw new Error("Cannot find module 'vitest/browser'"); | ||
| }, | ||
| }); | ||
| return browser; | ||
| }); | ||
| vi.doMock('@vitest/browser/context', () => ({ | ||
| commands: { | ||
| resetMousePosition, | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files "code/addons/vitest/src/vitest-plugin/setup-file.test.ts"Repository: storybookjs/storybook
Length of output: 121
🏁 Script executed:
cat -n code/addons/vitest/src/vitest-plugin/setup-file.test.tsRepository: storybookjs/storybook
Length of output: 5023
🏁 Script executed:
find . -name "*spy*" -o -name "*mocking*" | head -20Repository: storybookjs/storybook
Length of output: 772
🏁 Script executed:
cat -n code/addons/vitest/src/vitest-plugin/setup-file.tsRepository: storybookjs/storybook
Length of output: 3877
🏁 Script executed:
head -100 .cursor/rules/spy-mocking.mdcRepository: storybookjs/storybook
Length of output: 1586
🏁 Script executed:
# Check the first part of spy-mocking.mdc to understand requirements
head -200 .cursor/rules/spy-mocking.mdcRepository: storybookjs/storybook
Length of output: 1586
🏁 Script executed:
# Check if there's a pattern of vi.doMock usage in test files
rg "vi\.doMock" code/ --type ts --type tsx -c | sort | uniq -cRepository: storybookjs/storybook
Length of output: 93
🏁 Script executed:
# Look at the actual usage pattern - does the source code's dynamic import help justify this?
rg "import\(" code/addons/vitest/src/vitest-plugin/setup-file.ts -B 2 -A 2Repository: storybookjs/storybook
Length of output: 618
🏁 Script executed:
# Check if any other tests in the codebase use vi.doMock - is this a precedent?
rg "vi\.doMock" code/ --type ts --type tsx -lRepository: storybookjs/storybook
Length of output: 93
🏁 Script executed:
# Look at other test files to see if vi.doMock is used elsewhere
rg "vi\.doMock" code/ -t ts -lRepository: storybookjs/storybook
Length of output: 322
🏁 Script executed:
# Search for vi.mock usage to understand the standard pattern
rg "vi\.mock\(" code/ -t ts -B 1 -A 3 | head -80Repository: storybookjs/storybook
Length of output: 6357
🏁 Script executed:
# See how many tests use vi.doMock in that file
rg "vi\.doMock" code/core/src/common/js-package-manager/vite-plus-versions.test.ts -B 2 -A 5Repository: storybookjs/storybook
Length of output: 1495
Document why vi.doMock() is used here as an exception to the spy-mocking rules.
This test suite uses vi.doMock() inline per test case, which violates the spy-mocking guidelines requiring vi.mock() with spy: true at the top level and behaviors in beforeEach. However, this pattern is necessary because the source code uses await import() with error handling for different module resolution scenarios. The hoisted mock pattern cannot support per-test variations in module availability and behavior.
Add a comment in the suite explaining this intentional exception, or consider opening a discussion to clarify in the spy-mocking guidelines when vi.doMock() is appropriate versus the standard pattern.
🤖 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/vitest/src/vitest-plugin/setup-file.test.ts` around lines 82 -
138, Add a brief inline comment in the test suite above the per-test vi.doMock()
usages explaining that vi.doMock() is intentionally used per-test as an
exception to the spy-mocking rules because the implementation under test uses
dynamic await import() with error handling and needs per-case module
resolution/behavior (reference the vi.doMock calls and the
resetMousePositionBeforeTests tests); mention that hoisted vi.mock(spy: true)
cannot express these per-test variations, and suggest documenting or opening a
discussion in the spy-mocking guidelines if desired.
| throw error; | ||
| }); | ||
|
|
||
| await expect(resetMousePositionBeforeTests()).rejects.toThrow(); |
There was a problem hiding this comment.
Make the rethrow assertion specific to avoid false positives.
rejects.toThrow() passes on any thrown error; this test should assert the expected error/message so it truly verifies rethrow behavior.
Suggested tweak
- await expect(resetMousePositionBeforeTests()).rejects.toThrow();
+ await expect(resetMousePositionBeforeTests()).rejects.toThrow('boom');📝 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.
| await expect(resetMousePositionBeforeTests()).rejects.toThrow(); | |
| await expect(resetMousePositionBeforeTests()).rejects.toThrow('boom'); |
🤖 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/vitest/src/vitest-plugin/setup-file.test.ts` at line 119, The
test currently uses a broad await
expect(resetMousePositionBeforeTests()).rejects.toThrow(); update it to assert
the specific error or message thrown by resetMousePositionBeforeTests() to avoid
false positives — for example, change the assertion to await
expect(resetMousePositionBeforeTests()).rejects.toThrowError(<expected message
or regex>) or toThrowError(<ExpectedErrorClass>) and, if needed, adjust the test
stub/mocked dependency that causes the throw so the expected message/class is
known; target the resetMousePositionBeforeTests() call when making this change.
See internal bug report.
What I did
beforeEachto force the initial cursor position away from 0, 0This should address a bug on Chromium in Linux where the
:hoverstate gets recomputed without any actual mouse input events, and applied to elements at 0, 0. The fact that:hovergets applied causesaxe-coreresults to be unreliable, as the default state of a component is no longer tested.This fix causes user config changes, which we likely need to discuss. Do we want this only for new projects? Do we want an automigration? Do we have a more elegant way to trigger the same behaviour with less user config footprint?
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
I'm open to suggestions here.
Manual testing
git clone git@github.com:Sidnioulz/mealdrop.gitcd mealdrop && git checkout tree/telemetry-qa-3yarn && yarn storybookDocumentation
@jonniebigodes @yannbf do you think we need to document this in troubleshooting for the Vitest addon? Or should we add a JSDoc explanation for the browser command in the templates?
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 pull request has been released as version
0.0.0-pr-34765-sha-ab58c528. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34765-sha-ab58c528 sandboxor in an existing project withnpx storybook@0.0.0-pr-34765-sha-ab58c528 upgrade.More information
0.0.0-pr-34765-sha-ab58c528sidnioulz/reset-vitest-cursor-positionab58c5281778511056)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=34765Summary by CodeRabbit
Bug Fixes
Tests