test: harden false-positive e2e assertions (#1229)#1303
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
bot
added
the
size:M
📝 WalkthroughWalkthroughThis change fixes three e2e tests producing false positives by strengthening assertions and verification logic. Updates include: enforcing download verification in Ctrl+S test, comparing device Y coordinates before/after ArrowUp movement, verifying canvas transform changes after Focus action, and updating the dragDeviceToRack helper signature to accept positioning parameters. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/rack-context-menu-focus.spec.ts`:
- Around line 50-55: The test dispatches a programmatic "contextmenu" MouseEvent
without clientX/clientY which can default to 0 and break handlers; replace the
manual dispatch (the rack.dispatchEvent(new MouseEvent("contextmenu", ...))
call) with Playwright's native right-click on the element (e.g., await
page.locator('.rack-svg').click({ button: 'right' }) or await
page.click('.rack-svg', { button: 'right' })) so the browser will generate
proper coordinates and a faithful contextmenu event.
- Around line 66-77: Extract the duplicated expect.poll logic around
getPanzoomTransform into a reusable async helper (e.g.,
waitForPanzoomTransformChange) declared at the top of the spec file; the helper
should accept the Playwright Page and the prior transform (or compute
transformBefore inside) and return a boolean or await until transform.scale/x/y
changes by using the same polling logic currently duplicated, then replace both
duplicated expect.poll(...) usages with calls to
waitForPanzoomTransformChange(page, transformBefore) (or
waitForPanzoomTransformChange(page) as implemented) to keep behavior identical
while removing duplication.
- Around line 20-27: The test captures transformBefore too early (it reads the
CSS transform set by setPanzoomTransform but panzoom's RAF loop may overwrite it
before the Focus click), so move the transformBefore capture to immediately
before the focus click to ensure you're comparing the current CSS at the moment
of the click: in the spec's two tests that use setPanzoomTransform, remove the
earlier transformBefore reads and instead call page.evaluate(() =>
document.querySelector(".panzoom-container")?.getAttribute("style") or read
.style.transform) right before invoking focusItem.click() (and similarly before
the second test's focusItem.click()), keeping the rest of the assertions intact.
| async function setPanzoomTransform(page: Page, transform: string) { | ||
| await page.evaluate((nextTransform) => { | ||
| const panzoomContainer = document.querySelector(".panzoom-container"); | ||
| if (panzoomContainer) { | ||
| (panzoomContainer as HTMLElement).style.transform = nextTransform; | ||
| } | ||
| }, transform); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find panzoom in package.json and check for any panzoom imports
rg "panzoom" package.json package-lock.json 2>/dev/null | head -20Repository: RackulaLives/Rackula
Length of output: 290
🏁 Script executed:
#!/bin/bash
# Locate the test file and examine setPanzoomTransform and getPanzoomTransform
fd "rack-context-menu-focus.spec.ts" --type fRepository: RackulaLives/Rackula
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Search for panzoom usage across the codebase
rg -n "panzoom" src/ e2e/ --type ts --type tsx -A 2 | head -40Repository: RackulaLives/Rackula
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Read the test file to understand the structure and helpers
cat -n e2e/rack-context-menu-focus.spec.ts | head -150Repository: RackulaLives/Rackula
Length of output: 5310
🏁 Script executed:
#!/bin/bash
# Search for getPanzoomTransform definition
rg -n "getPanzoomTransform|setPanzoomTransform" e2e/ --type ts -A 5Repository: RackulaLives/Rackula
Length of output: 6337
🏁 Script executed:
#!/bin/bash
# Search for panzoom initialization and focus implementation
rg -n "import.*panzoom|from.*panzoom|new.*Panzoom|panzoom\(" src/ --type ts -A 3 | head -60Repository: RackulaLives/Rackula
Length of output: 3767
🏁 Script executed:
#!/bin/bash
# Look for focusRack or focus-related functions
rg -n "focusRack|focus.*function|const.*focus" src/ --type ts -A 5 | head -80Repository: RackulaLives/Rackula
Length of output: 4256
🏁 Script executed:
#!/bin/bash
# Read the focusRack function implementation
sed -n '289,350p' src/lib/stores/canvas.svelte.tsRepository: RackulaLives/Rackula
Length of output: 2554
🏁 Script executed:
#!/antml:bash
# Also check how panzoom is initialized and if RAF is used
rg -n "new Panzoom|panzoom\(" src/lib --type ts -B 2 -A 5Repository: RackulaLives/Rackula
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Look at panzoom mock to understand expected API
cat -n src/tests/mocks/panzoom.ts | head -70Repository: RackulaLives/Rackula
Length of output: 2161
🏁 Script executed:
#!/bin/bash
# Continue reading focusRack to see panzoom API calls
sed -n '289,400p' src/lib/stores/canvas.svelte.tsRepository: RackulaLives/Rackula
Length of output: 4070
🏁 Script executed:
#!/bin/bash
# Search for smoothZoomAbs or moveTo calls in focusRack
rg -n "smoothZoomAbs|moveTo|zoomAbs" src/lib/stores/canvas.svelte.ts -A 2 -B 2Repository: RackulaLives/Rackula
Length of output: 1851
🏁 Script executed:
#!/bin/bash
# Verify that panzoom internal state is separate from CSS
# Check if there's any initialization code that might set different defaults
rg -n "new Panzoom|panzoom(" src/lib --type ts -B 3 -A 10 | head -60Repository: RackulaLives/Rackula
Length of output: 154
Move transformBefore capture immediately before the Focus click to avoid false positives from panzoom's RAF loop.
setPanzoomTransform writes only to the CSS style.transform; it does not update the panzoom library's internal state. The multiple async operations between capturing transformBefore (line 42) and clicking Focus (line 64)—dispatching the contextmenu event, waiting for .context-menu-content, finding and asserting focusItem—create await points where panzoom's RAF loop continuously synchronizes the CSS to panzoom's actual internal state. This can overwrite the manually set CSS before Focus fires.
If Focus is broken and doesn't update panzoom's internal state, the test will still pass because transformBefore (captured earlier) now holds a stale value that differs from the current CSS, making the poll succeed despite the failure.
Fix: Move `transformBefore` capture to immediately before the click action
In Test 1 (lines 40–64), move the capture block:
// Force a non-focused transform so Focus must change it.
await setPanzoomTransform(page, "matrix(0.4, 0, 0, 0.4, 0, 0)");
- const transformBefore = await getPanzoomTransform(page);
- expect(transformBefore).toBeTruthy();
- expect(transformBefore?.scale).toBe(0.4);
// Open the canvas context menu directly on the rack element.
await page.evaluate(() => {
const rack = document.querySelector(".rack-svg");
if (!rack) throw new Error("Could not find rack svg");
rack.dispatchEvent(new MouseEvent("contextmenu", { bubbles: true, cancelable: true }));
});
await expect(page.locator(".context-menu-content")).toBeVisible();
const focusItem = page.locator('.context-menu-item:has-text("Focus")');
await expect(focusItem).toBeVisible();
+
+ // Capture the actual current transform immediately before clicking,
+ // ensuring it reflects the true CSS state at the moment Focus fires.
+ const transformBefore = await getPanzoomTransform(page);
+ expect(transformBefore).toBeTruthy();
+ expect(transformBefore?.scale).toBe(0.4);
await focusItem.click();Apply the same pattern to Test 2 (lines 84–110): move the capture block to immediately before line 110's focusItem.click().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/rack-context-menu-focus.spec.ts` around lines 20 - 27, The test captures
transformBefore too early (it reads the CSS transform set by setPanzoomTransform
but panzoom's RAF loop may overwrite it before the Focus click), so move the
transformBefore capture to immediately before the focus click to ensure you're
comparing the current CSS at the moment of the click: in the spec's two tests
that use setPanzoomTransform, remove the earlier transformBefore reads and
instead call page.evaluate(() =>
document.querySelector(".panzoom-container")?.getAttribute("style") or read
.style.transform) right before invoking focusItem.click() (and similarly before
the second test's focusItem.click()), keeping the rest of the assertions intact.
| rack.dispatchEvent( | ||
| new MouseEvent("contextmenu", { | ||
| bubbles: true, | ||
| cancelable: true, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Programmatic contextmenu event is missing mouse coordinates.
The event is dispatched without clientX, clientY, screenX, or screenY (all default to 0). If the context menu handler uses event coordinates for positioning or hit-testing (e.g., "was the right-click inside the rack?"), the menu may appear at the top-left corner or the handler may reject the event. Using Playwright's native click({ button: 'right' }) on .rack-svg would be more faithful and avoid this fragility.
🛠️ Proposed fix — use Playwright's built-in right-click
- // Open the canvas context menu directly on the rack element.
- await page.evaluate(() => {
- const rack = document.querySelector(".rack-svg");
- if (!rack) throw new Error("Could not find rack svg");
- rack.dispatchEvent(
- new MouseEvent("contextmenu", {
- bubbles: true,
- cancelable: true,
- }),
- );
- });
+ // Open the canvas context menu via a native right-click on the rack.
+ await page.locator(".rack-svg").first().click({ button: "right" });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/rack-context-menu-focus.spec.ts` around lines 50 - 55, The test
dispatches a programmatic "contextmenu" MouseEvent without clientX/clientY which
can default to 0 and break handlers; replace the manual dispatch (the
rack.dispatchEvent(new MouseEvent("contextmenu", ...)) call) with Playwright's
native right-click on the element (e.g., await page.locator('.rack-svg').click({
button: 'right' }) or await page.click('.rack-svg', { button: 'right' })) so the
browser will generate proper coordinates and a faithful contextmenu event.
| await expect | ||
| .poll(async () => { | ||
| const transformAfter = await getPanzoomTransform(page); | ||
| if (!transformBefore || !transformAfter) return false; | ||
|
|
||
| return ( | ||
| transformAfter.scale !== transformBefore.scale || | ||
| transformAfter.x !== transformBefore.x || | ||
| transformAfter.y !== transformBefore.y | ||
| ); | ||
| }) | ||
| .toBe(true); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate polling logic — extract into a shared helper.
The expect.poll(...) block at lines 66-77 is copy-pasted verbatim at lines 112-123. Extract it into a named helper at the top of the file for DRY and maintainability.
♻️ Proposed refactor
+async function assertTransformChanged(
+ page: Page,
+ transformBefore: { scale: number; x: number; y: number },
+) {
+ await expect
+ .poll(
+ async () => {
+ const transformAfter = await getPanzoomTransform(page);
+ if (!transformAfter) return false;
+ return (
+ transformAfter.scale !== transformBefore.scale ||
+ transformAfter.x !== transformBefore.x ||
+ transformAfter.y !== transformBefore.y
+ );
+ },
+ { timeout: 5000 },
+ )
+ .toBe(true);
+}Then replace both polling blocks with:
- await expect
- .poll(async () => {
- const transformAfter = await getPanzoomTransform(page);
- if (!transformBefore || !transformAfter) return false;
- return (
- transformAfter.scale !== transformBefore.scale ||
- transformAfter.x !== transformBefore.x ||
- transformAfter.y !== transformBefore.y
- );
- })
- .toBe(true);
+ await assertTransformChanged(page, transformBefore!);📝 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 | |
| .poll(async () => { | |
| const transformAfter = await getPanzoomTransform(page); | |
| if (!transformBefore || !transformAfter) return false; | |
| return ( | |
| transformAfter.scale !== transformBefore.scale || | |
| transformAfter.x !== transformBefore.x || | |
| transformAfter.y !== transformBefore.y | |
| ); | |
| }) | |
| .toBe(true); | |
| async function assertTransformChanged( | |
| page: Page, | |
| transformBefore: { scale: number; x: number; y: number }, | |
| ) { | |
| await expect | |
| .poll( | |
| async () => { | |
| const transformAfter = await getPanzoomTransform(page); | |
| if (!transformAfter) return false; | |
| return ( | |
| transformAfter.scale !== transformBefore.scale || | |
| transformAfter.x !== transformBefore.x || | |
| transformAfter.y !== transformBefore.y | |
| ); | |
| }, | |
| { timeout: 5000 }, | |
| ) | |
| .toBe(true); | |
| } | |
| // Then at lines 66-77: | |
| await assertTransformChanged(page, transformBefore!); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/rack-context-menu-focus.spec.ts` around lines 66 - 77, Extract the
duplicated expect.poll logic around getPanzoomTransform into a reusable async
helper (e.g., waitForPanzoomTransformChange) declared at the top of the spec
file; the helper should accept the Playwright Page and the prior transform (or
compute transformBefore inside) and return a boolean or await until
transform.scale/x/y changes by using the same polling logic currently
duplicated, then replace both duplicated expect.poll(...) usages with calls to
waitForPanzoomTransformChange(page, transformBefore) (or
waitForPanzoomTransformChange(page) as implemented) to keep behavior identical
while removing duplication.
1 participant
User description
Summary
Ctrl+Skeyboard test require a real download event (removed swallowed timeout path)Test Plan
npx playwright test --config playwright.dev.config.ts e2e/keyboard.spec.ts e2e/rack-context-menu-focus.spec.ts --grep "Ctrl\+S|Arrow keys move device|Focus option"npm run test:e2e(blocked locally: production build startup exceeds Playwright webServer 60s timeout and WebKit browser is not installed)npm run lint(fails on pre-existing unrelated repo lint errors)Closes #1229
CodeAnt-AI Description
Harden e2e keyboard and rack-focus tests to avoid false positives
What Changed
Impact
✅ Fewer false-positive e2e failures✅ Fewer flaky keyboard tests✅ Fewer flaky focus/context-menu tests💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.