Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
4 Skipped Deployments
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
WalkthroughAdds a helper method Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/e2e/pages/base-page.ts`:
- Around line 68-77: The dismissVariantsPopupIfPresent method currently waits
4000ms and uses an unscoped getByRole which causes long blind waits and can
dismiss unrelated "Dismiss" buttons; change dismissVariantsPopupIfPresent (and
callers like goto() and connectWallet()) to use a much shorter probe timeout
(<=1000 ms) and scope the locator to the Variants Detected modal (e.g., locate
the dialog/container first then call getByRole or use a text match for "Variants
Detected") so only the intended popup is targeted, and update the catch to only
swallow the expected timeout/no-element errors (rethrow any other exceptions)
instead of silencing all errors.
|
@JoseRFelix Tested this locally with
All swap tests pass locally now (2 passed, 1 skipped). Updated the PR description with details. R4R when you get a chance. 😌🙏 |
…irmation detection failures The Osmosis frontend shows a Variants Detected alert modal after wallet connection and balance load, which overlays the page and prevents Playwright from detecting the Transaction Successful toast. This adds a dismissVariantsPopupIfPresent() method to BasePage and calls it from connectWallet() and every page goto() so the modal is cleared before test assertions run. Co-authored-by: Cursor <cursoragent@cursor.com>
jasbanza
force-pushed
the
jason/mer-122-e2e-dismiss-variants-detected-modal-to-prevent-transaction
branch
from
9293430 to
5170d25
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| // PopUp page is auto-closed | ||
| // Handle Pop-up page <- | ||
| await this.getWalletBalance() | ||
| await this.dismissVariantsPopupIfPresent() |
There was a problem hiding this comment.
Dismiss called after getWalletBalance causing timeout failures
High Severity
dismissVariantsPopupIfPresent() is called after getWalletBalance() in connectWallet(), but the PR description explicitly identifies this ordering as the cause of 9-second timeout failures — the modal overlay prevents the balance element from becoming visible. The dismiss call needs to be before getWalletBalance() to unblock it.
| async dismissVariantsPopupIfPresent() { | ||
| try { | ||
| const dismissBtn = this.page.getByRole('button', { name: 'Dismiss' }) | ||
| await dismissBtn.waitFor({ state: 'visible', timeout: 4000 }) |
There was a problem hiding this comment.
Timeout is 4s instead of intended 1s
Medium Severity
The waitFor timeout is 4000ms, but the PR description says it was reduced to 1s because the modal either appears within ~500ms or not at all. With calls in both connectWallet() and every page's goto(), this adds up to 8+ seconds of unnecessary wait time per test in the common no-modal path.
| console.log('Dismissed "Variants Detected" popup.') | ||
| } catch { | ||
| // Modal not present — continue normally | ||
| } |
There was a problem hiding this comment.
Bare catch swallows all errors silently
Medium Severity
The catch block swallows all errors indiscriminately. The PR description says the catch clause was updated to only swallow timeout errors and re-throw anything else, but the code uses a bare catch {}. This silences real failures (e.g., page crashes, unexpected Playwright errors) making debugging harder.
|
|
||
| async dismissVariantsPopupIfPresent() { | ||
| try { | ||
| const dismissBtn = this.page.getByRole('button', { name: 'Dismiss' }) |
There was a problem hiding this comment.
Dismiss button locator not scoped to Variants modal
Medium Severity
The locator getByRole('button', { name: 'Dismiss' }) matches any button named "Dismiss" on the page, not just the one inside the Variants Detected alert. The PR description says this was fixed to scope to getByRole('alert').filter({ hasText: 'Variants Detected' }) first, then find the dismiss button within that. Without scoping, this could click unrelated dismiss buttons or trigger a Playwright strict-mode violation if multiple matches exist — which would be silently swallowed by the bare catch.
2 participants
Summary
dismissVariantsPopupIfPresent()method toBasePagethat safely dismisses the "Variants Detected" alert modal (1s timeout, no-op if absent)connectWallet()and every page'sgoto()(TradePage,SwapPage,PoolsPage,PortfolioPage,TransactionsPage) so the modal is cleared before test assertions runLinear Issue
Resolves MER-122
Changes
Initial implementation
dismissVariantsPopupIfPresent()toBasePageconnectWallet()andgoto()inTradePage,SwapPage,PoolsPage,PortfolioPageFixed while testing locally
Running
npx playwright test swap.osmo.walletlocally revealed three issues:Wrong ARIA role -- The "Variants Detected" popup uses
role="alert", notrole="alertdialog". The scoped locator was targeting the wrong element. Fixed togetByRole('alert').filter({ hasText: 'Variants Detected' }).Dismiss called too late in
connectWallet()-- The modal appears after wallet approval but before the balance loads. The original code calleddismissVariantsPopupIfPresent()aftergetWalletBalance(), so the modal overlay prevented the balance element from becoming visible, causing a 9s timeout failure. Fixed by moving the dismiss call beforegetWalletBalance().Dismiss called too late in
PoolsPage.goto()-- The dismiss was at the end ofgoto(), afterpoolsLink.click()andhover(). If the modal appeared afterpage.goto('/'), those interactions would fail before the dismiss was ever reached. Fixed by moving it right after the initial page load.Missing from
TransactionsPage.open()-- Added dismiss call for consistency with all other page navigation methods.Timeout reduced from 4s to 1s -- The modal either appears within ~500ms of page load or not at all. 4s added unnecessary dead wait in the common (no-modal) path. With calls in both
connectWallet()andgoto(), this was up to 8s of wasted time per test.Catch clause now discriminates errors -- Only swallows timeout errors; re-throws anything else so real failures aren't silenced.
Test plan
npx playwright test swap.osmo.walletlocally -- modal dismissed, 2 passed, 1 skippedNote
Low Risk
Test-only change that adds a defensive UI dismissal path; main risk is masking a real unexpected dialog or timing issues if the locator/timeout is wrong.
Overview
Reduces e2e flakiness by adding
BasePage.dismissVariantsPopupIfPresent()to detect and click the modal’sDismissbutton when present (no-op when absent).Hooks this dismissal into
connectWallet()and multiple pagegoto()flows (SwapPage,TradePage,PoolsPage,PortfolioPage) so transient overlays don’t block interactions like balance visibility checks or transaction confirmation assertions.Written by Cursor Bugbot for commit 5170d25. This will update automatically on new commits. Configure here.