fix(e2e): resolve Keplr popup race condition in trade tests #4218
Conversation
- Add explicit 15s timeout to waitForEvent('page') calls
- Reorder operations to listen for popup BEFORE clicking confirm button
- Increase UI settle time from 200ms to 500ms
- Add try-catch wrapper with clear error messages
- Apply fix to both buyAndGetWalletMsg() and sellAndGetWalletMsg()
This addresses intermittent test failures where the Keplr approval popup
context closes prematurely, causing 'Target page, context or browser has
been closed' errors and 60s timeouts.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
4 Skipped Deployments
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds retry-capable trade operations and single-shot approvals with configurable slippage and retries; exposes new TradePage methods for buy/sell/swap and wallet-message retrieval; adds cancel-limit-order in TransactionsPage; makes extension unzip asynchronous; and applies formatting/locator normalizations across E2E tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Test
participant TradePage
participant KeplrPopup as Keplr Popup
participant Blockchain
Test->>TradePage: buyAndGetWalletMsg(context, options?)
activate TradePage
Note over TradePage: check balances → open review modal → set slippage (if provided)
TradePage->>TradePage: attempt confirm (button may be disabled)
par Popup attach (optional)
TradePage->>KeplrPopup: attachTo(timeout)
and Retry loop
TradePage->>TradePage: retry with delays/logging
end
alt Popup appeared
KeplrPopup->>KeplrPopup: approve
KeplrPopup-->>TradePage: return msgContentAmount
TradePage->>Blockchain: wait for tx confirmation (≤40s)
TradePage-->>Test: return msgContentAmount
else 1-click / no popup
Note over TradePage: return undefined msgContentAmount (tx submitted/preapproved)
TradePage-->>Test: return undefined
end
deactivate TradePage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/e2e/pages/trade-page.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
📚 Learning: 2025-09-03T10:03:23.597Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-12-13T09:12:42.643Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T12:43:19.973Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:205-208
Timestamp: 2024-11-15T12:43:19.973Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, within the `setTransaction1CTParams` function, it's acceptable to return the previous parameters if `initialTransaction1CTParams` or `nextParams` are undefined, as this situation is temporary and reusing the previous state is valid.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T13:02:05.236Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:741-747
Timestamp: 2024-11-15T13:02:05.236Z
Learning: In `packages/web/modals/review-order.tsx`, when toggling one-click trading, if `transaction1CTParams` is undefined due to not retrieving default parameters from `api.local.oneClickTrading.getParameters`, we should keep `transaction1CTParams` undefined rather than providing a default state.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T09:50:28.017Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:324-325
Timestamp: 2024-11-15T09:50:28.017Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, it's intentional to throw an error in the state updater function of `OneClickTradingSettings` component when `params` is undefined in standalone mode.
Applied to files:
packages/e2e/pages/trade-page.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: preview-swap-usdc-tests
- GitHub Check: Analyze (javascript)
- GitHub Check: Summary
- GitHub Check: preview-pools-and-select-pair-tests
- Add retry loop with 2 retries (3 total attempts) to buyAndGetWalletMsg and sellAndGetWalletMsg
- Keep 15-second timeout for waitForEvent('page') to handle slow popup appearances
- Add detailed logging with attempt numbers and error messages
- Add 2-second wait between retry attempts to let UI settle
- Throw clear error messages on final failure after all retries
This addresses intermittent 'Target page, context or browser has been closed'
errors in CI environments by combining timeout handling, reordered operations,
and retry logic. Maximum execution time: ~49 seconds across all attempts.
Implements coderabbit's suggestion from PR review.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/e2e/pages/trade-page.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
📚 Learning: 2025-09-03T10:03:23.597Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-12-13T09:12:42.643Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T12:43:19.973Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:205-208
Timestamp: 2024-11-15T12:43:19.973Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, within the `setTransaction1CTParams` function, it's acceptable to return the previous parameters if `initialTransaction1CTParams` or `nextParams` are undefined, as this situation is temporary and reusing the previous state is valid.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T09:50:28.017Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:324-325
Timestamp: 2024-11-15T09:50:28.017Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, it's intentional to throw an error in the state updater function of `OneClickTradingSettings` component when `params` is undefined in standalone mode.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-19T17:28:47.846Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-params.ts:126-144
Timestamp: 2024-11-19T17:28:47.846Z
Learning: In the React TypeScript file `packages/web/hooks/one-click-trading/use-one-click-trading-params.ts`, within the `setTransaction1CTParamsWithChanges` function of the `useOneClickTradingParams` hook, resetting is a safety measure and recovery method, not an error path. Therefore, adding extra logs when resetting is unnecessary.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T13:02:05.236Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:741-747
Timestamp: 2024-11-15T13:02:05.236Z
Learning: In `packages/web/modals/review-order.tsx`, when toggling one-click trading, if `transaction1CTParams` is undefined due to not retrieving default parameters from `api.local.oneClickTrading.getParameters`, we should keep `transaction1CTParams` undefined rather than providing a default state.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T10:08:23.742Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:132-136
Timestamp: 2024-11-15T10:08:23.742Z
Learning: In `packages/web/modals/review-order.tsx`, the calculation of `wouldExceedSpendLimit` in the `ReviewOrder` component handles optional chaining and null values correctly, and does not require providing default values to avoid potential `NaN` results.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-05T08:46:24.743Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3913
File: packages/trpc/src/assets.ts:444-446
Timestamp: 2024-11-05T08:46:24.743Z
Learning: In the codebase, it's acceptable for errors to propagate to the client from the `getUserDustAssets` procedure in `packages/trpc/src/assets.ts`, so adding try-catch blocks for handling potential price fetching errors is not required.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-23T16:36:41.685Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/alert/prettify.ts:43-44
Timestamp: 2024-11-23T16:36:41.685Z
Learning: In `packages/web/components/alert/prettify.ts`, when handling overspend error messages related to `uusdc`, assuming 6 decimal places is acceptable because `uusdc` uses 6 decimals.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-22T11:02:18.859Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-session.ts:89-92
Timestamp: 2024-11-22T11:02:18.859Z
Learning: In the `useOneClickTradingSession` hook within `packages/web/hooks/one-click-trading/use-one-click-trading-session.ts`, storing the expired flag in the store is not desirable as it introduces unnecessary persistence without real usage. The `isExpired` status should be calculated rather than stored.
Applied to files:
packages/e2e/pages/trade-page.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Analyze (javascript)
- GitHub Check: test (22.x, proto-codecs)
- GitHub Check: test (22.x, unit)
- GitHub Check: test (22.x, tx)
- GitHub Check: test (22.x, bridge)
- GitHub Check: wait-for-deployment
- GitHub Check: test (22.x, web)
- GitHub Check: test (22.x, stores)
- GitHub Check: test (22.x, utils)
- GitHub Check: test (22.x, pools)
- GitHub Check: test (22.x, math)
- GitHub Check: test (22.x, web)
- GitHub Check: test (22.x, server)
- GitHub Check: lint-workspace
- GitHub Check: test (22.x, server)
- GitHub Check: test (22.x, utils)
- GitHub Check: test (22.x, math)
- GitHub Check: test (22.x, stores)
- GitHub Check: test (22.x, bridge)
- GitHub Check: test (22.x, pools)
- GitHub Check: Summary
🔇 Additional comments (2)
packages/e2e/pages/trade-page.ts (2)
506-527: Well-implemented utility method.The
setSlippageTolerancemethod is properly structured with clear documentation, appropriate waits, value validation, and good error handling.
529-589: Excellent retry logic for handling race conditions.The retry mechanism properly handles the race condition where quote refreshes temporarily disable the swap button. The use of
justApproveIfNeeded(which already handles optional popups per the learnings) combined with specific error detection and informative logging makes this robust.Based on learnings
|
@JohnnyWyles I have turned this PR into a draft - I am reviewing as I need to be 100% sure I'm not breaking any other tests. This is taking longer than I thought it would however it is critical that I have a clear understanding of these tests, especially since the monitoring tests use the same files (the ones that run every 30 minutes) and are harder to test being that they can only be tested on stage. Also, the latest preview push test failed because it does a SwapExactAmountOut, and it is giving an estimate error which needs investigation: |
- Add graceful timeout handling for 1-click trading scenarios
- Add fail-fast insufficient balance checks before retry loops
- Return { msgContentAmount: undefined } when popup doesn't appear
- Check insufficient balance once before retries to prevent wasted time
- Prevents ~43s timeout when 1-click is enabled
- Prevents ~94s wasted retries when balance is insufficient
…alidation - Increase isTransactionSuccesful timeout from 7s to 15s across all tests - Increase global Playwright timeout from 60s to 90s - Make msgContentAmount validation conditional for 1-click trading - Prevents 'Timed out 7000ms waiting for Transaction Successful' errors - Prevents worker teardown timeout errors - Accommodates longer blockchain confirmation times for split route swaps This fixes test failures where split route swap transactions take longer than 7 seconds to complete on-chain, particularly with the increased gas multiplier (3.0) for MsgSplitRouteSwapExactAmountIn transactions.
…-to-keplr-popup-context-closure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/e2e/pages/trade-page.ts (1)
329-418: Buy/Sell retry + 1‑click handling look correct; consider de‑duplication and narrower timeout classificationThe new
buyAndGetWalletMsg/sellAndGetWalletMsgflows do what you want: balance pre‑check outside the loop, bounded retries with clear logging, and treating the KeplrwaitForEvent("page")timeout as the 1‑click / pre‑approval path viamsgContentAmount: undefined. Callers intrade.wallet.spec.tsnow handle that shape correctly.Two follow‑ups you may want (can be done later):
- Both methods are ~identical apart from the button and log strings; factoring the core logic into a small helper (e.g.
performTradeWithKeplrPopup({ kind: "buy" | "sell", limit })) would avoid future drift and make changing timeouts or logging less error‑prone.- Right now any
TimeoutError(or message containing "Timeout"/"timeout") from the whole inner block—includingwaitForLoadStateorgetByText(...).textContent()—is treated as “no popup” and returns{ msgContentAmount: undefined }. If you ever need sharper diagnosis, you could restrict the “no popup” success path to only thewaitForEvent("page")timeout and let later timeouts behave as retriable failures.Overall, the behavior is solid and aligns with the flaky Keplr popup + 1‑click trading requirements.
Also applies to: 420-509
packages/e2e/tests/monitoring.swap.wallet.spec.ts (1)
55-56: Slippage and success‑timeout choices for monitoring swap testsWiring through
swapAndApprove(context, { slippagePercent: '1' })andisTransactionSuccesful(15)is consistent with the new TradePage API, but 1% slippage is quite strict for a monitoring suite that runs against live routes and liquidity. If you still see intermittent “estimate” or routing errors here, consider relaxing this to match the 3% you use in the OSMO tests or making it configurable per test scenario rather than hard‑coding1.packages/e2e/tests/monitoring.limit.wallet.spec.ts (1)
52-52: Align monitoring limit tests’ success timeout with shared constantUsing
isTransactionSuccesful(15)here is consistent with the updated TradePage API, but since this value is now duplicated across several suites, it would be safer to route it through a shared constant (and likely bump it alongside the trade tests) so monitoring and functional tests stay in sync when you adjust for real‑world confirmation times.Also applies to: 69-69
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/e2e/pages/trade-page.ts(1 hunks)packages/e2e/playwright.config.ts(1 hunks)packages/e2e/tests/monitoring.limit.wallet.spec.ts(2 hunks)packages/e2e/tests/monitoring.swap.wallet.spec.ts(1 hunks)packages/e2e/tests/swap.osmo.wallet.spec.ts(3 hunks)packages/e2e/tests/trade.wallet.spec.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/e2e/playwright.config.ts
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
📚 Learning: 2024-12-13T09:12:42.643Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
Applied to files:
packages/e2e/tests/trade.wallet.spec.tspackages/e2e/tests/monitoring.swap.wallet.spec.tspackages/e2e/tests/swap.osmo.wallet.spec.tspackages/e2e/tests/monitoring.limit.wallet.spec.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2025-09-03T10:03:23.597Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Applied to files:
packages/e2e/tests/trade.wallet.spec.tspackages/e2e/tests/monitoring.swap.wallet.spec.tspackages/e2e/tests/swap.osmo.wallet.spec.tspackages/e2e/tests/monitoring.limit.wallet.spec.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-19T17:28:47.846Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-params.ts:126-144
Timestamp: 2024-11-19T17:28:47.846Z
Learning: In the React TypeScript file `packages/web/hooks/one-click-trading/use-one-click-trading-params.ts`, within the `setTransaction1CTParamsWithChanges` function of the `useOneClickTradingParams` hook, resetting is a safety measure and recovery method, not an error path. Therefore, adding extra logs when resetting is unnecessary.
Applied to files:
packages/e2e/tests/trade.wallet.spec.tspackages/e2e/tests/monitoring.swap.wallet.spec.tspackages/e2e/tests/monitoring.limit.wallet.spec.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T13:02:05.236Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:741-747
Timestamp: 2024-11-15T13:02:05.236Z
Learning: In `packages/web/modals/review-order.tsx`, when toggling one-click trading, if `transaction1CTParams` is undefined due to not retrieving default parameters from `api.local.oneClickTrading.getParameters`, we should keep `transaction1CTParams` undefined rather than providing a default state.
Applied to files:
packages/e2e/tests/trade.wallet.spec.tspackages/e2e/tests/swap.osmo.wallet.spec.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T12:43:19.973Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:205-208
Timestamp: 2024-11-15T12:43:19.973Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, within the `setTransaction1CTParams` function, it's acceptable to return the previous parameters if `initialTransaction1CTParams` or `nextParams` are undefined, as this situation is temporary and reusing the previous state is valid.
Applied to files:
packages/e2e/tests/trade.wallet.spec.tspackages/e2e/tests/monitoring.swap.wallet.spec.tspackages/e2e/tests/swap.osmo.wallet.spec.tspackages/e2e/tests/monitoring.limit.wallet.spec.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-23T16:36:41.685Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/alert/prettify.ts:43-44
Timestamp: 2024-11-23T16:36:41.685Z
Learning: In `packages/web/components/alert/prettify.ts`, when handling overspend error messages related to `uusdc`, assuming 6 decimal places is acceptable because `uusdc` uses 6 decimals.
Applied to files:
packages/e2e/tests/trade.wallet.spec.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T10:08:23.742Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:132-136
Timestamp: 2024-11-15T10:08:23.742Z
Learning: In `packages/web/modals/review-order.tsx`, the calculation of `wouldExceedSpendLimit` in the `ReviewOrder` component handles optional chaining and null values correctly, and does not require providing default values to avoid potential `NaN` results.
Applied to files:
packages/e2e/tests/trade.wallet.spec.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T09:50:28.017Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:324-325
Timestamp: 2024-11-15T09:50:28.017Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, it's intentional to throw an error in the state updater function of `OneClickTradingSettings` component when `params` is undefined in standalone mode.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-05T08:46:24.743Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3913
File: packages/trpc/src/assets.ts:444-446
Timestamp: 2024-11-05T08:46:24.743Z
Learning: In the codebase, it's acceptable for errors to propagate to the client from the `getUserDustAssets` procedure in `packages/trpc/src/assets.ts`, so adding try-catch blocks for handling potential price fetching errors is not required.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-22T11:02:18.859Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-session.ts:89-92
Timestamp: 2024-11-22T11:02:18.859Z
Learning: In the `useOneClickTradingSession` hook within `packages/web/hooks/one-click-trading/use-one-click-trading-session.ts`, storing the expired flag in the store is not desirable as it introduces unnecessary persistence without real usage. The `isExpired` status should be calculated rather than stored.
Applied to files:
packages/e2e/pages/trade-page.ts
🧬 Code graph analysis (1)
packages/e2e/tests/trade.wallet.spec.ts (1)
packages/e2e/pages/transactions-page.ts (1)
TransactionsPage(11-177)
🪛 GitHub Actions: Preview frontend E2E tests
packages/e2e/pages/trade-page.ts
[error] 251-251: Timed out waiting for the Transaction Successful element to be visible ( waited 15s ) during test execution. This caused the Trade feature test to fail.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (2)
packages/e2e/tests/swap.osmo.wallet.spec.ts (1)
60-63: OSMO swap tests now correctly leverage retry + slippage optionsUsing
swapAndApprove(context, { maxRetries: 3, slippagePercent: "3" })for the OSMO↔ATOM flows lines up with the new TradePage API and should mitigate the previous estimate/split‑route flakiness while still keeping assertions simple. The change looks good as‑is.Also applies to: 71-75
packages/e2e/tests/trade.wallet.spec.ts (1)
49-55: Conditional msgContent assertions correctly handle 1‑click / no‑popup pathsGating the msgContent checks behind
if (msgContentAmount)in the buy/sell and limit tests is consistent with the new{ msgContentAmount | undefined }contract frombuyAndGetWalletMsg/sellAndGetWalletMsg. This avoids penalizing 1‑click trading scenarios while still validating the on‑chain message when a Keplr popup actually appears.Also applies to: 65-71, 85-95, 113-123
There was a problem hiding this comment.
Pull Request Overview
This PR addresses intermittent e2e test failures caused by race conditions in Keplr popup handling during trade operations. The changes implement retry logic, improved timeout handling, and support for 1-click trading scenarios to improve test reliability.
Key changes:
- Added retry logic (3 attempts) with explicit 15-second timeouts for Keplr popup detection
- Implemented graceful handling of 1-click trading scenarios where no popup appears
- Increased transaction success timeout from 7s to 15s to accommodate split route swaps
- Added fail-fast balance checks before retry loops
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/e2e/pages/trade-page.ts | Implemented retry logic with timeout handling for buyAndGetWalletMsg() and sellAndGetWalletMsg(), including support for 1-click trading and fail-fast balance validation |
| packages/e2e/tests/trade.wallet.spec.ts | Updated to conditionally validate message content (only when popup appears) and increased transaction timeout to 15s |
| packages/e2e/tests/swap.osmo.wallet.spec.ts | Increased transaction success timeout from 7s to 15s for all swap operations |
| packages/e2e/tests/monitoring.swap.wallet.spec.ts | Reduced slippage tolerance from 3% to 1% for stablecoin swaps and increased transaction timeout to 15s |
| packages/e2e/tests/monitoring.limit.wallet.spec.ts | Increased transaction success timeout from 7s to 15s for limit order operations |
| packages/e2e/playwright.config.ts | Updated comment to clarify the 90s timeout is increased from the default 60s value |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ); | ||
|
|
||
| // Wait before retry to let things settle | ||
| await this.page.waitForTimeout(2000); |
There was a problem hiding this comment.
Potential issue: When a retry occurs after clicking sellBtn and confirmSwapBtn, the UI may be left in an inconsistent state (e.g., confirmation modal still open). Before retrying, consider resetting the UI state by either:
- Closing any open modals/dialogs
- Navigating back to a known good state
- Checking if the sell flow needs to be restarted from scratch
Without proper cleanup between retries, subsequent attempts may fail due to unexpected UI state rather than the original race condition.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/e2e/pages/trade-page.ts (1)
329-418: Return type inconsistency creates breaking change.
buyAndGetWalletMsgnow returns{ msgContentAmount }(lines 380, 389), but this differs fromswapAndGetWalletMsgat line 184, which returns the rawmsgContentstring. This breaks the established API pattern and could cause failures in test code expecting a string.While the retry logic and timeout handling improvements are excellent, the return type should remain consistent. Callers currently check/log the message content directly as a string.
Apply this diff to restore consistency:
if (attempt > 0) { console.log(`✓ Buy operation succeeded after ${attempt} retry(ies)`); } - return { msgContentAmount }; + return msgContentAmount ?? undefined; } catch (error: any) { // Gracefully handle timeout - popup didn't appear (1-click trading enabled or pre-approved) if (error.name === "TimeoutError" || error.message?.includes("Timeout") || error.message?.includes("timeout")) { console.log("✓ Keplr approval popup did not appear within 15s; assuming 1-click trading is enabled or transaction was pre-approved."); await this.page.waitForTimeout(2000); if (attempt > 0) { console.log(`✓ Buy operation succeeded after ${attempt} retry(ies)`); } - return { msgContentAmount: undefined }; + return undefined; }Also apply the same change to
sellAndGetWalletMsgat lines 472 and 481.
🧹 Nitpick comments (2)
packages/e2e/pages/trade-page.ts (2)
329-510: Consider extracting common retry logic to reduce ~180 lines of duplication.
buyAndGetWalletMsgandsellAndGetWalletMsgcontain nearly identical retry loops, timeout handling, and error messaging. This duplication makes maintenance harder—any bug fix or enhancement must be applied twice.Consider extracting shared logic into a private helper:
private async executeTradeWithRetry( context: BrowserContext, actionBtn: Locator, actionType: 'buy' | 'sell', limit: boolean ): Promise<string | undefined> { // Common retry loop, timeout handling, and popup approval }Then both methods become thin wrappers:
async buyAndGetWalletMsg(context: BrowserContext, limit = false) { return this.executeTradeWithRetry(context, this.buyBtn, 'buy', limit); }
392-393: Add fallback for undefined error messages.Error logging at multiple points accesses
error.messagewithout a fallback, which will log "undefined" as a string if the property doesn't exist.Apply this pattern consistently:
-console.error("Failed to get Keplr approval popup:", error.message); +console.error("Failed to get Keplr approval popup:", error.message ?? "Unknown error");Also applies to lines 484 and 491 in error logging blocks.
Also applies to: 484-484, 491-491
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/e2e/pages/trade-page.ts(1 hunks)packages/e2e/tests/monitoring.swap.wallet.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
📚 Learning: 2024-12-13T09:12:42.643Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
Applied to files:
packages/e2e/tests/monitoring.swap.wallet.spec.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2025-09-03T10:03:23.597Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Applied to files:
packages/e2e/tests/monitoring.swap.wallet.spec.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T12:43:19.973Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:205-208
Timestamp: 2024-11-15T12:43:19.973Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, within the `setTransaction1CTParams` function, it's acceptable to return the previous parameters if `initialTransaction1CTParams` or `nextParams` are undefined, as this situation is temporary and reusing the previous state is valid.
Applied to files:
packages/e2e/tests/monitoring.swap.wallet.spec.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-19T17:28:47.846Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-params.ts:126-144
Timestamp: 2024-11-19T17:28:47.846Z
Learning: In the React TypeScript file `packages/web/hooks/one-click-trading/use-one-click-trading-params.ts`, within the `setTransaction1CTParamsWithChanges` function of the `useOneClickTradingParams` hook, resetting is a safety measure and recovery method, not an error path. Therefore, adding extra logs when resetting is unnecessary.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T13:02:05.236Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:741-747
Timestamp: 2024-11-15T13:02:05.236Z
Learning: In `packages/web/modals/review-order.tsx`, when toggling one-click trading, if `transaction1CTParams` is undefined due to not retrieving default parameters from `api.local.oneClickTrading.getParameters`, we should keep `transaction1CTParams` undefined rather than providing a default state.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T09:50:28.017Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:324-325
Timestamp: 2024-11-15T09:50:28.017Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, it's intentional to throw an error in the state updater function of `OneClickTradingSettings` component when `params` is undefined in standalone mode.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T10:08:23.742Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:132-136
Timestamp: 2024-11-15T10:08:23.742Z
Learning: In `packages/web/modals/review-order.tsx`, the calculation of `wouldExceedSpendLimit` in the `ReviewOrder` component handles optional chaining and null values correctly, and does not require providing default values to avoid potential `NaN` results.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-05T08:46:24.743Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3913
File: packages/trpc/src/assets.ts:444-446
Timestamp: 2024-11-05T08:46:24.743Z
Learning: In the codebase, it's acceptable for errors to propagate to the client from the `getUserDustAssets` procedure in `packages/trpc/src/assets.ts`, so adding try-catch blocks for handling potential price fetching errors is not required.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-23T16:36:41.685Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/alert/prettify.ts:43-44
Timestamp: 2024-11-23T16:36:41.685Z
Learning: In `packages/web/components/alert/prettify.ts`, when handling overspend error messages related to `uusdc`, assuming 6 decimal places is acceptable because `uusdc` uses 6 decimals.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-22T11:02:18.859Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-session.ts:89-92
Timestamp: 2024-11-22T11:02:18.859Z
Learning: In the `useOneClickTradingSession` hook within `packages/web/hooks/one-click-trading/use-one-click-trading-session.ts`, storing the expired flag in the store is not desirable as it introduces unnecessary persistence without real usage. The `isExpired` status should be calculated rather than stored.
Applied to files:
packages/e2e/pages/trade-page.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: staging
- GitHub Check: wait-for-deployment
- GitHub Check: test (22.x, web)
- GitHub Check: test (22.x, utils)
- GitHub Check: test (22.x, utils)
- GitHub Check: test (22.x, bridge)
- GitHub Check: test (22.x, server)
- GitHub Check: test (22.x, pools)
- GitHub Check: test (22.x, web)
- GitHub Check: test (22.x, math)
- GitHub Check: test (22.x, stores)
- GitHub Check: lint-workspace
- GitHub Check: Analyze (javascript)
- GitHub Check: Summary
🔇 Additional comments (3)
packages/e2e/pages/trade-page.ts (2)
512-575: Well-structured additions with proper error handling.The new
buyAndApprove,sellAndApprove, andsetSlippageTolerancemethods integrate cleanly with the enhanced flow. Good use of:
- Optional configuration via options object
- Clear error messages with context
- Verification that slippage was actually set (line 568)
577-637: Robust race condition handling for quote refresh scenarios.The retry logic in
swapAndApproveproperly addresses the race condition where route/quote recalculation temporarily disables the swap button. The error type detection (lines 613-614) correctly distinguishes retriable errors from genuine failures, and logging the exchange rate (line 627) aids debugging.packages/e2e/tests/monitoring.swap.wallet.spec.ts (1)
55-61: Good documentation of slippage rationale and appropriate timeout increase.The comment explaining why 1% slippage is suitable for stablecoin pairs (tighter spreads, predictable pricing) along with the trade-off warning about volatility is helpful for future maintainers. The 15-second timeout (line 61) aligns with the PR objective to accommodate split-route swaps that require longer confirmation times.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
…pattern - Refactor all transaction methods to start success listener BEFORE UI interactions - Move success promise handling INSIDE retry loops for fresh 40s timeout per attempt - Each retry now gets independent timeout instead of accumulating wait time - Transactions wait for blockchain confirmation internally before returning - Add JSDoc documentation to 6 transaction methods for better maintainability - Update inline comments: "transaction can complete immediately (1-click trading) or after Keplr popup approval flow" - Fix Keplr extension race condition by awaiting decompress operation in unzip-extension.ts - Fix "manifest missing" errors when running multiple tests concurrently Benefits: - Fresh 40s timeout per retry attempt (total 120s across 3 attempts) - Success listener runs in parallel with popup handling (faster 1-click tests) - Tests are simpler and more readable - More resilient to varying environment / confirmation times Affected files: - pages: trade-page.ts (5 methods), transactions-page.ts (1 method) - setup: unzip-extension.ts, setup-keplr.ts - tests: 6 test files
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/e2e/pages/trade-page.ts (1)
579-597: Normalize timeout detection insellAndGetWalletMsgIn
sellAndGetWalletMsg, timeout detection currently relies on string matching:if ( error.name === "TimeoutError" || error.message?.includes("Timeout") || error.message?.includes("timeout") )whereas
buyAndGetWalletMsgalready uses a more robust, case-insensitive regex with aninstanceof Errorcheck.For consistency and fewer edge cases, consider mirroring the buy logic here:
- if ( - error.name === "TimeoutError" || - error.message?.includes("Timeout") || - error.message?.includes("timeout") - ) { + if ( + error.name === "TimeoutError" || + (error instanceof Error && /timeout/i.test(error.message)) + ) {This keeps behavior aligned between buy/sell flows and avoids depending on exact error message casing.
🧹 Nitpick comments (5)
packages/e2e/tests/monitoring.limit.wallet.spec.ts (1)
7-77: Consider closing the browser context inafterAllfor consistencyThis suite sets up a persistent
BrowserContextinbeforeAllbut never callscontext.close(), unlike the other monitoring tests. Closing it inafterAllwould align patterns and avoid leaking a Playwright context if more suites are added in the same worker.packages/e2e/tests/swap.usdc.wallet.spec.ts (1)
24-38: Balance check behavior no longer matches the “fail fast” comment
ensureBalancesis now called with{ warnOnly: true }, so insufficient balances will only log warnings instead of failing the suite, while the comment still says “fail fast if insufficient”. Please either:
- Keep
warnOnly: truebut update the comment to reflect warn-only semantics, or- Drop
warnOnlyif you still want this suite to fail early on underfunded wallets.packages/e2e/pages/trade-page.ts (1)
56-58: Tighten theslippageInputlocator to avoid future mis-targeting
this.slippageInput = page.locator('input[type="text"][inputmode="decimal"]').first();relies on the slippage field being the first decimal text input in the DOM. If another decimal input is added before it (e.g., a new numeric field in the modal),setSlippageTolerancecould start writing to the wrong element.Consider scoping the locator to the review modal or a data-testid, for example:
this.slippageInput = page .locator('[data-testid="slippage-input"] input[type="text"][inputmode="decimal"]') .first();(or any equivalent modal-scoped selector that uniquely identifies the slippage control).
packages/e2e/tests/trade.wallet.spec.ts (2)
99-99: Clean up commented-out assertion or add explanation.The commented-out expect statement lacks context. If the assertion is no longer valid due to message format changes, remove it. If it's temporarily disabled pending a fix, add a TODO comment explaining why.
Apply this diff to remove the commented code:
// Only validate message content if Keplr popup appeared (not 1-click trading) if (msgContentAmount) { - //expect(msgContentAmount).toContain(amount + " ATOM (Cosmos Hub/channel-0)"); expect(msgContentAmount).toContain("place_limit"); expect(msgContentAmount).toContain('"order_direction": "ask"'); }
125-125: Clean up commented-out assertion or add explanation.Similar to line 99, this commented-out expect statement needs cleanup. Either remove it if obsolete or document why it's disabled.
Apply this diff to remove the commented code:
// Only validate message content if Keplr popup appeared (not 1-click trading) if (msgContentAmount) { - //expect(msgContentAmount).toContain(`${amount} OSMO`); expect(msgContentAmount).toContain("place_limit"); expect(msgContentAmount).toContain('"order_direction": "ask"'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/e2e/pages/trade-page.ts(7 hunks)packages/e2e/pages/transactions-page.ts(1 hunks)packages/e2e/setup-keplr.ts(1 hunks)packages/e2e/tests/monitoring.limit.wallet.spec.ts(1 hunks)packages/e2e/tests/monitoring.market.wallet.spec.ts(1 hunks)packages/e2e/tests/monitoring.swap.wallet.spec.ts(1 hunks)packages/e2e/tests/swap.osmo.wallet.spec.ts(1 hunks)packages/e2e/tests/swap.usdc.wallet.spec.ts(2 hunks)packages/e2e/tests/trade.wallet.spec.ts(1 hunks)packages/e2e/unzip-extension.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/e2e/tests/swap.osmo.wallet.spec.ts
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
📚 Learning: 2025-09-03T10:03:23.597Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Applied to files:
packages/e2e/pages/trade-page.tspackages/e2e/tests/monitoring.market.wallet.spec.tspackages/e2e/pages/transactions-page.tspackages/e2e/setup-keplr.tspackages/e2e/tests/swap.usdc.wallet.spec.tspackages/e2e/tests/trade.wallet.spec.tspackages/e2e/tests/monitoring.limit.wallet.spec.tspackages/e2e/tests/monitoring.swap.wallet.spec.ts
📚 Learning: 2024-12-13T09:12:42.643Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
Applied to files:
packages/e2e/pages/trade-page.tspackages/e2e/tests/monitoring.market.wallet.spec.tspackages/e2e/pages/transactions-page.tspackages/e2e/tests/swap.usdc.wallet.spec.tspackages/e2e/tests/trade.wallet.spec.tspackages/e2e/tests/monitoring.limit.wallet.spec.tspackages/e2e/tests/monitoring.swap.wallet.spec.ts
📚 Learning: 2024-11-15T12:43:19.973Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:205-208
Timestamp: 2024-11-15T12:43:19.973Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, within the `setTransaction1CTParams` function, it's acceptable to return the previous parameters if `initialTransaction1CTParams` or `nextParams` are undefined, as this situation is temporary and reusing the previous state is valid.
Applied to files:
packages/e2e/pages/trade-page.tspackages/e2e/pages/transactions-page.tspackages/e2e/tests/trade.wallet.spec.tspackages/e2e/tests/monitoring.swap.wallet.spec.ts
📚 Learning: 2024-11-15T13:02:05.236Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:741-747
Timestamp: 2024-11-15T13:02:05.236Z
Learning: In `packages/web/modals/review-order.tsx`, when toggling one-click trading, if `transaction1CTParams` is undefined due to not retrieving default parameters from `api.local.oneClickTrading.getParameters`, we should keep `transaction1CTParams` undefined rather than providing a default state.
Applied to files:
packages/e2e/pages/trade-page.tspackages/e2e/pages/transactions-page.tspackages/e2e/tests/monitoring.swap.wallet.spec.ts
📚 Learning: 2024-11-19T17:28:47.846Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-params.ts:126-144
Timestamp: 2024-11-19T17:28:47.846Z
Learning: In the React TypeScript file `packages/web/hooks/one-click-trading/use-one-click-trading-params.ts`, within the `setTransaction1CTParamsWithChanges` function of the `useOneClickTradingParams` hook, resetting is a safety measure and recovery method, not an error path. Therefore, adding extra logs when resetting is unnecessary.
Applied to files:
packages/e2e/pages/trade-page.tspackages/e2e/pages/transactions-page.tspackages/e2e/tests/monitoring.limit.wallet.spec.ts
📚 Learning: 2024-11-15T09:50:28.017Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:324-325
Timestamp: 2024-11-15T09:50:28.017Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, it's intentional to throw an error in the state updater function of `OneClickTradingSettings` component when `params` is undefined in standalone mode.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T10:08:23.742Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:132-136
Timestamp: 2024-11-15T10:08:23.742Z
Learning: In `packages/web/modals/review-order.tsx`, the calculation of `wouldExceedSpendLimit` in the `ReviewOrder` component handles optional chaining and null values correctly, and does not require providing default values to avoid potential `NaN` results.
Applied to files:
packages/e2e/pages/trade-page.tspackages/e2e/pages/transactions-page.ts
📚 Learning: 2024-11-05T08:46:24.743Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3913
File: packages/trpc/src/assets.ts:444-446
Timestamp: 2024-11-05T08:46:24.743Z
Learning: In the codebase, it's acceptable for errors to propagate to the client from the `getUserDustAssets` procedure in `packages/trpc/src/assets.ts`, so adding try-catch blocks for handling potential price fetching errors is not required.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-23T16:36:41.685Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/alert/prettify.ts:43-44
Timestamp: 2024-11-23T16:36:41.685Z
Learning: In `packages/web/components/alert/prettify.ts`, when handling overspend error messages related to `uusdc`, assuming 6 decimal places is acceptable because `uusdc` uses 6 decimals.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-22T11:02:18.859Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-session.ts:89-92
Timestamp: 2024-11-22T11:02:18.859Z
Learning: In the `useOneClickTradingSession` hook within `packages/web/hooks/one-click-trading/use-one-click-trading-session.ts`, storing the expired flag in the store is not desirable as it introduces unnecessary persistence without real usage. The `isExpired` status should be calculated rather than stored.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-10-01T12:11:49.471Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 3862
File: .github/workflows/monitoring-limit-geo-e2e-tests.yml:36-36
Timestamp: 2024-10-01T12:11:49.471Z
Learning: The wallet associated with `TEST_PRIVATE_KEY_1` has sufficient `uosmo` balance for EU tests.
Applied to files:
packages/e2e/tests/swap.usdc.wallet.spec.ts
🧬 Code graph analysis (7)
packages/e2e/tests/monitoring.market.wallet.spec.ts (2)
packages/e2e/setup-keplr.ts (1)
SetupKeplr(7-44)packages/e2e/utils/balance-checker.ts (1)
ensureBalances(282-390)
packages/e2e/pages/transactions-page.ts (1)
packages/e2e/pages/base-page.ts (1)
BasePage(3-79)
packages/e2e/setup-keplr.ts (1)
packages/e2e/unzip-extension.ts (1)
UnzipExtension(5-29)
packages/e2e/tests/swap.usdc.wallet.spec.ts (1)
packages/e2e/utils/balance-checker.ts (1)
ensureBalances(282-390)
packages/e2e/tests/trade.wallet.spec.ts (4)
packages/e2e/pages/trade-page.ts (1)
TradePage(10-857)packages/e2e/setup-keplr.ts (1)
SetupKeplr(7-44)packages/e2e/utils/balance-checker.ts (1)
ensureBalances(282-390)packages/e2e/pages/transactions-page.ts (1)
TransactionsPage(11-202)
packages/e2e/tests/monitoring.limit.wallet.spec.ts (3)
packages/e2e/pages/trade-page.ts (1)
TradePage(10-857)packages/e2e/setup-keplr.ts (1)
SetupKeplr(7-44)packages/e2e/utils/balance-checker.ts (1)
ensureBalances(282-390)
packages/e2e/tests/monitoring.swap.wallet.spec.ts (2)
packages/e2e/setup-keplr.ts (1)
SetupKeplr(7-44)packages/e2e/utils/balance-checker.ts (1)
ensureBalances(282-390)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: wait-for-deployment
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript)
- GitHub Check: Summary
- GitHub Check: test (22.x, stores)
- GitHub Check: test (22.x, utils)
- GitHub Check: test (22.x, web)
- GitHub Check: lint-workspace
- GitHub Check: test (22.x, utils)
- GitHub Check: test (22.x, pools)
- GitHub Check: test (22.x, tx)
- GitHub Check: test (22.x, server)
- GitHub Check: test (22.x, math)
- GitHub Check: test (22.x, web)
- GitHub Check: test (22.x, unit)
- GitHub Check: test (22.x, proto-codecs)
🔇 Additional comments (10)
packages/e2e/setup-keplr.ts (1)
8-13: Awaiting extension unzip is correctAwaiting
getPathToExtension()before launching the persistent context is the right fix; it guarantees the Keplr extension is fully extracted before Playwright starts using it while keeping the cleanup path intact.packages/e2e/unzip-extension.ts (1)
6-16: Async unzip + await decompress looks goodMaking
getPathToExtensionasync and awaitingdecompressgives callers a fully ready extension path and matches howsetup-keplrnow uses it.packages/e2e/tests/monitoring.swap.wallet.spec.ts (1)
7-67: Stablecoin monitoring swaps wired correctly to new helpersThe monitoring suite now cleanly wires through
SetupKeplr, balance checks in warn-only mode, andswapAndApprovewith documented 1% slippage, plus propercontext.close()inafterAll. Looks good.packages/e2e/tests/monitoring.market.wallet.spec.ts (1)
7-90: Market monitoring tests align with new buy/sell helpersThe monitoring market suite now consistently uses
SetupKeplr, warn-only balance checks, and the newbuyAndApprove/sellAndApproveflows with slippage, plus proper teardown. No functional issues spotted.packages/e2e/tests/trade.wallet.spec.ts (6)
22-30: LGTM: Balance pre-checks with monitoring-friendly warnOnly mode.The
ensureBalancescall withwarnOnly: trueproperly validates wallet balances before tests run while allowing monitoring suites to proceed even with insufficient funds. The inline comments documenting the required amounts for each test are helpful.
53-62: LGTM: Proper retry logic and 1-click trading support.The test correctly uses
buyAndGetWalletMsgwithmaxRetries: 2for resilience and conditionally validatesmsgContentAmountonly when the Keplr popup appeared. This pattern properly handles both traditional approval flows and 1-click trading scenarios.
71-80: LGTM: Consistent sell flow with proper conditional validation.The sell test follows the same robust pattern as the buy test: retry logic via
maxRetries, custom slippage, and conditional message validation for 1-click trading compatibility.
93-107: LGTM: Proper limit order flow with cleanup.The test correctly uses the
limit: trueflag forsellAndGetWalletMsg, performs conditional validation, and cleans up by canceling the order. The cleanup ensures no lingering orders affect subsequent test runs.
110-134: LGTM: Comprehensive limit order test coverage.The OSMO limit sell test provides good coverage of limit orders with a different asset and price change percentage, following the same reliable pattern with retry logic and proper cleanup.
1-135: Past review concern about 15s timeout has been addressed.The previous review flagged hard-coded
isTransactionSuccesful(15)calls as causing CI flakiness. The current implementation has resolved this by:
- Moving transaction success handling into
buyAndGetWalletMsg()andsellAndGetWalletMsg()methods- Using longer timeouts (40s) internally within these methods
- Eliminating direct
isTransactionSuccesful()calls from test codeThis refactoring addresses the flakiness concern raised in the past review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
packages/e2e/tests/monitoring.market.wallet.spec.ts:12
- Unused variable TRX_SUCCESS_TIMEOUT.
const TRX_SUCCESS_TIMEOUT = 10000;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
- this was missed in the previous commit but is already used in another method in a previous commit.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/e2e/pages/trade-page.ts (4)
56-58:slippageInputlocator may be too generic/brittleUsing
input[type="text"][inputmode="decimal"]with.first()can accidentally hit the wrong field if multiple decimal inputs exist (amount, price, slippage, etc.), and DOM order changes are hard to reason about. If possible, anchor this locator on a label or dedicated test id for the slippage control (e.g., viagetByLabelordata-testid) to reduce flakiness.
331-484:buyAndGetWalletMsg: robust retry + popup/1‑click handling, with a small refinement opportunityThe structure here is solid: you fail fast on insufficient balance, set up a fresh
trxSuccessfullistener per attempt (40s), startwaitForEvent("page")before any clicks to avoid races, support slippage configuration, and treat Keplr not opening within 20s as a valid 1‑click/pre‑approved path returning{ msgContentAmount: undefined }. Logging and final error messages are also clear, and the retry loop is well‑scoped.One improvement to consider: the inner
try/catchcurrently treats anyTimeoutErrorin that block (waitForEvent,waitForLoadState,getByText,expect(approveBtn)) as “no popup needed” and falls back to the 1‑click path. That can blur genuine Keplr UI issues (e.g., popup opened but never fully loaded orApprovebutton never enabled) into the 1‑click case, only surfacing later via the 40s success timeout. Splittingawait pageApprovePromiseinto its own try/catch (mapping only that timeout to the 1‑click branch and letting later timeouts rethrow to the outer loop) would keep logs and diagnosis sharper without changing behaviour when the popup truly never appears. Based on learnings
486-640:sellAndGetWalletMsgmirrors buy flow correctly; keep it in sync with any refinementThe sell path mirrors
buyAndGetWalletMsgnicely: same balance pre-check, retry loop, per-attempttrxSuccessfullistener (40s), Keplr popup handling with a 20s wait plus 1‑click fallback, and structured logging/errors. That symmetry makes the API predictable for tests. If you adopt the narrower timeout-handling refinement suggested for the buy method, it’s worth applying the same split here so buy/sell behave identically in edge cases.
740-763:setSlippageTolerancewiring looks good; failures are intentionally hardWaiting for
slippageInputto become visible, then assertingtoHaveValueafter filling, is a good pattern for de-flaking slippage changes in the review modal, and the method integrates cleanly into swap/buy/sell flows. Since any failure here is logged and rethrown, these tests will now fail hard on slippage‑UI regressions; if at some point you decide slippage should be best-effort, you could downgrade this to warning-only without rethrowing, but the current strictness is reasonable for catching bugs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/e2e/pages/trade-page.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
📚 Learning: 2025-09-03T10:03:23.597Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-12-13T09:12:42.643Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T12:43:19.973Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:205-208
Timestamp: 2024-11-15T12:43:19.973Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, within the `setTransaction1CTParams` function, it's acceptable to return the previous parameters if `initialTransaction1CTParams` or `nextParams` are undefined, as this situation is temporary and reusing the previous state is valid.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-19T17:28:47.846Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-params.ts:126-144
Timestamp: 2024-11-19T17:28:47.846Z
Learning: In the React TypeScript file `packages/web/hooks/one-click-trading/use-one-click-trading-params.ts`, within the `setTransaction1CTParamsWithChanges` function of the `useOneClickTradingParams` hook, resetting is a safety measure and recovery method, not an error path. Therefore, adding extra logs when resetting is unnecessary.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T09:50:28.017Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:324-325
Timestamp: 2024-11-15T09:50:28.017Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, it's intentional to throw an error in the state updater function of `OneClickTradingSettings` component when `params` is undefined in standalone mode.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T13:02:05.236Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:741-747
Timestamp: 2024-11-15T13:02:05.236Z
Learning: In `packages/web/modals/review-order.tsx`, when toggling one-click trading, if `transaction1CTParams` is undefined due to not retrieving default parameters from `api.local.oneClickTrading.getParameters`, we should keep `transaction1CTParams` undefined rather than providing a default state.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T10:08:23.742Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:132-136
Timestamp: 2024-11-15T10:08:23.742Z
Learning: In `packages/web/modals/review-order.tsx`, the calculation of `wouldExceedSpendLimit` in the `ReviewOrder` component handles optional chaining and null values correctly, and does not require providing default values to avoid potential `NaN` results.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-05T08:46:24.743Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3913
File: packages/trpc/src/assets.ts:444-446
Timestamp: 2024-11-05T08:46:24.743Z
Learning: In the codebase, it's acceptable for errors to propagate to the client from the `getUserDustAssets` procedure in `packages/trpc/src/assets.ts`, so adding try-catch blocks for handling potential price fetching errors is not required.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-23T16:36:41.685Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/alert/prettify.ts:43-44
Timestamp: 2024-11-23T16:36:41.685Z
Learning: In `packages/web/components/alert/prettify.ts`, when handling overspend error messages related to `uusdc`, assuming 6 decimal places is acceptable because `uusdc` uses 6 decimals.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-22T11:02:18.859Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-session.ts:89-92
Timestamp: 2024-11-22T11:02:18.859Z
Learning: In the `useOneClickTradingSession` hook within `packages/web/hooks/one-click-trading/use-one-click-trading-session.ts`, storing the expired flag in the store is not desirable as it introduces unnecessary persistence without real usage. The `isExpired` status should be calculated rather than stored.
Applied to files:
packages/e2e/pages/trade-page.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: test (22.x, tx)
- GitHub Check: test (22.x, unit)
- GitHub Check: test (22.x, bridge)
- GitHub Check: test (22.x, pools)
- GitHub Check: test (22.x, proto-codecs)
- GitHub Check: test (22.x, web)
- GitHub Check: test (22.x, server)
- GitHub Check: test (22.x, math)
- GitHub Check: test (22.x, stores)
- GitHub Check: test (22.x, stores)
- GitHub Check: test (22.x, pools)
- GitHub Check: lint-workspace
- GitHub Check: test (22.x, bridge)
- GitHub Check: test (22.x, web)
- GitHub Check: test (22.x, server)
- GitHub Check: test (22.x, math)
- GitHub Check: test (22.x, utils)
- GitHub Check: wait-for-deployment
- GitHub Check: Summary
🔇 Additional comments (5)
packages/e2e/pages/trade-page.ts (5)
169-187: Swap button timeout increase inswapAndGetWalletMsglooks reasonableBumping the
toBeEnabledtimeout forswapBtnto 15s aligns with the longer transaction/confirmation windows you’ve introduced and should help with slow quote/route computation without materially impacting runtime.
202-245: Additional visibility waits inselectPairimprove stabilityAdding
waitFor({ state: "visible" })before clicking the filtered from/to token locators is a good safeguard against slow filtering and DOM updates, and should reduce intermittent “not visible” click errors in asset selection flows.
642-687:sellAndApprove: simple non-retrying path looks appropriateThis helper cleanly covers the “fire once and wait” case: it waits for the sell button to be enabled, starts a 40s
trxSuccessfullistener, optionally sets slippage, then usesjustApproveIfNeededto handle both popup and 1‑click scenarios before awaiting confirmation. The docstring clearly positions this as the simpler alternative tosellAndGetWalletMsg.
689-733:buyAndApprove: consistent counterpart to the sell helper
buyAndApprovemirrorssellAndApprove(enablement check, per-call 40s success wait, optional slippage,justApproveIfNeeded, then confirmation), which keeps the TradePage API symmetric and straightforward for tests that only care about success, not wallet message contents.
765-855:swapAndApprove: retry strategy and success listener per attempt look soundThe enhanced JSDoc plus the retry loop that only re-attempts on “disabled”/
toBeEnablederrors matches the goal of handling quote-refresh races without masking other failures. Creating a freshtrxSuccessfulvisibility promise for each attempt (40s), wiring slippage viasetSlippageTolerance, and continuing to usejustApproveIfNeededfor Keplr keep this flow aligned with the new buy/sell helpers while preserving the existing semantics for non-retryable errors.
Add comprehensive "IMPORTANT:" comments across all transaction methods in trade-page.ts and transactions-page.ts to explain the promise pattern for future developers. Comments explain: - Why success promise starts BEFORE button clicks (catch immediate confirmations for 1-click trading) - Why waitForEvent needs explicit timeout (prevent hanging indefinitely) - Why timeout errors are gracefully handled (1-click trading is expected behavior, not error) - Why we await actual blockchain confirmation (instead of arbitrary waits) - Why each retry gets fresh timeout (avoid timeout exhaustion across retries) Files updated: - trade-page.ts: buyAndGetWalletMsg, sellAndGetWalletMsg, swapAndApprove, buyAndApprove, sellAndApprove - transactions-page.ts: cancelLimitOrder, claimAndCloseAny, claimAll This improves code maintainability and helps onboard new developers to the test suite patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/e2e/pages/trade-page.ts (1)
809-812: Move insufficient balance check outside retry loop for consistencyThe insufficient balance check is currently inside the retry loop, which means it's re-checked on every attempt. For consistency with
buyAndGetWalletMsg(line 363) andsellAndGetWalletMsg(line 524), and to enable fail-fast behavior, consider moving this check before theforloop starts (line 806).Apply this diff:
const maxRetries = options?.maxRetries ?? 3; const slippagePercent = options?.slippagePercent; + + // Check for insufficient balance BEFORE retry loop - no point retrying if balance is low + expect( + await this.isInsufficientBalanceForSwap(), + "Insufficient balance for the swap!" + ).toBeFalsy(); // Retry logic to handle race conditions where quote refreshes and temporarily disables swap button for (let attempt = 0; attempt <= maxRetries; attempt++) { try { - // Make sure to have sufficient balance and swap button is enabled - expect( - await this.isInsufficientBalanceForSwap(), - "Insufficient balance for the swap!" - ).toBeFalsy(); console.log("Swap and Sign now..");packages/e2e/pages/transactions-page.ts (1)
19-33: Pre-existing XPath issue notedLine 23 uses an invalid XPath predicate
'//a/span["View on explorer"]/..'that doesn't actually filter by text content. While this should eventually be fixed to'//a/span[.="View on explorer"]/..', this is pre-existing and outside the scope of this PR's race condition fixes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/e2e/pages/trade-page.ts(7 hunks)packages/e2e/pages/transactions-page.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
📚 Learning: 2024-12-13T09:12:42.643Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4012
File: packages/e2e/tests/swap.usdc.wallet.spec.ts:64-64
Timestamp: 2024-12-13T09:12:42.643Z
Learning: In `packages/e2e/tests/swap.usdc.wallet.spec.ts`, it is acceptable to call `tradePage.isTransactionSuccesful()` without additional error handling.
Applied to files:
packages/e2e/pages/transactions-page.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2025-09-03T10:03:23.597Z
Learnt from: yury-dubinin
Repo: osmosis-labs/osmosis-frontend PR: 4167
File: packages/e2e/pages/trade-page.ts:129-147
Timestamp: 2025-09-03T10:03:23.597Z
Learning: In wallet approval flows for e2e tests in packages/e2e/pages/trade-page.ts, approval popups may or may not appear depending on wallet state. The pattern of waiting a fixed timeout and then checking if context.pages() has additional pages is appropriate because it gracefully handles both scenarios (popup appears vs popup doesn't appear), whereas using context.waitForEvent("page") would fail when no popup is needed.
Applied to files:
packages/e2e/pages/transactions-page.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T10:08:23.742Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:132-136
Timestamp: 2024-11-15T10:08:23.742Z
Learning: In `packages/web/modals/review-order.tsx`, the calculation of `wouldExceedSpendLimit` in the `ReviewOrder` component handles optional chaining and null values correctly, and does not require providing default values to avoid potential `NaN` results.
Applied to files:
packages/e2e/pages/transactions-page.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-19T17:28:47.846Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-params.ts:126-144
Timestamp: 2024-11-19T17:28:47.846Z
Learning: In the React TypeScript file `packages/web/hooks/one-click-trading/use-one-click-trading-params.ts`, within the `setTransaction1CTParamsWithChanges` function of the `useOneClickTradingParams` hook, resetting is a safety measure and recovery method, not an error path. Therefore, adding extra logs when resetting is unnecessary.
Applied to files:
packages/e2e/pages/transactions-page.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T13:02:05.236Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/modals/review-order.tsx:741-747
Timestamp: 2024-11-15T13:02:05.236Z
Learning: In `packages/web/modals/review-order.tsx`, when toggling one-click trading, if `transaction1CTParams` is undefined due to not retrieving default parameters from `api.local.oneClickTrading.getParameters`, we should keep `transaction1CTParams` undefined rather than providing a default state.
Applied to files:
packages/e2e/pages/transactions-page.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T12:43:19.973Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:205-208
Timestamp: 2024-11-15T12:43:19.973Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, within the `setTransaction1CTParams` function, it's acceptable to return the previous parameters if `initialTransaction1CTParams` or `nextParams` are undefined, as this situation is temporary and reusing the previous state is valid.
Applied to files:
packages/e2e/pages/transactions-page.tspackages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-15T09:50:28.017Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/one-click-trading/one-click-trading-settings.tsx:324-325
Timestamp: 2024-11-15T09:50:28.017Z
Learning: In `packages/web/components/one-click-trading/one-click-trading-settings.tsx`, it's intentional to throw an error in the state updater function of `OneClickTradingSettings` component when `params` is undefined in standalone mode.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-05T08:46:24.743Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3913
File: packages/trpc/src/assets.ts:444-446
Timestamp: 2024-11-05T08:46:24.743Z
Learning: In the codebase, it's acceptable for errors to propagate to the client from the `getUserDustAssets` procedure in `packages/trpc/src/assets.ts`, so adding try-catch blocks for handling potential price fetching errors is not required.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-23T16:36:41.685Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/components/alert/prettify.ts:43-44
Timestamp: 2024-11-23T16:36:41.685Z
Learning: In `packages/web/components/alert/prettify.ts`, when handling overspend error messages related to `uusdc`, assuming 6 decimal places is acceptable because `uusdc` uses 6 decimals.
Applied to files:
packages/e2e/pages/trade-page.ts
📚 Learning: 2024-11-22T11:02:18.859Z
Learnt from: greg-nagy
Repo: osmosis-labs/osmosis-frontend PR: 3954
File: packages/web/hooks/one-click-trading/use-one-click-trading-session.ts:89-92
Timestamp: 2024-11-22T11:02:18.859Z
Learning: In the `useOneClickTradingSession` hook within `packages/web/hooks/one-click-trading/use-one-click-trading-session.ts`, storing the expired flag in the store is not desirable as it introduces unnecessary persistence without real usage. The `isExpired` status should be calculated rather than stored.
Applied to files:
packages/e2e/pages/trade-page.ts
🧬 Code graph analysis (1)
packages/e2e/pages/transactions-page.ts (1)
packages/e2e/pages/base-page.ts (1)
BasePage(3-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Summary
- GitHub Check: test (22.x, pools)
- GitHub Check: test (22.x, bridge)
- GitHub Check: test (22.x, math)
- GitHub Check: test (22.x, unit)
- GitHub Check: test (22.x, utils)
- GitHub Check: test (22.x, bridge)
- GitHub Check: test (22.x, stores)
- GitHub Check: test (22.x, proto-codecs)
- GitHub Check: test (22.x, tx)
- GitHub Check: test (22.x, server)
- GitHub Check: test (22.x, server)
- GitHub Check: test (22.x, utils)
- GitHub Check: test (22.x, pools)
- GitHub Check: test (22.x, stores)
- GitHub Check: test (22.x, math)
- GitHub Check: test (22.x, web)
- GitHub Check: lint-workspace
- GitHub Check: wait-for-deployment
🔇 Additional comments (8)
packages/e2e/pages/trade-page.ts (5)
56-58: LGTM - Improved slippage input locatorThe updated locator is more specific and uses
.first()to handle multiple matches, which is a good defensive pattern.
331-490: Excellent implementation of retry-safe buy flowThe method correctly implements:
- Fail-fast balance check before retry loop (lines 363-366)
- Success listener started before UI interactions to catch immediate confirmations (lines 383-385)
- Graceful timeout handling for 1-click trading scenarios (lines 428-435)
- Robust timeout detection using both error.name and regex pattern (lines 429-430)
- Fresh 40s success timeout per attempt to avoid timeout exhaustion (line 456)
- Comprehensive JSDoc documentation
Based on learnings
492-652: Consistent implementation mirrors buy flowThe sell method correctly applies the same patterns as
buyAndGetWalletMsg, maintaining consistency across the codebase. All the same robust timeout handling and retry logic is present.Based on learnings
654-749: Well-designed single-shot approval methodsBoth
sellAndApproveandbuyAndApprovecorrectly:
- Start success listener before UI interactions (lines 683-685, 731-733)
- Wait for actual blockchain confirmation instead of arbitrary timeouts (lines 700, 748)
- Document the no-retry behavior in JSDoc, directing users to retry-capable alternatives
756-779: Robust slippage tolerance setter with validationThe implementation correctly:
- Waits for the input to be visible before interacting (line 761)
- Validates the value was actually set before proceeding (lines 770-772)
- Provides helpful error logging if something fails
packages/e2e/pages/transactions-page.ts (3)
82-171: Excellent implementation of cancel flow with fail-fast patternThe method correctly implements:
- Fail-fast button visibility check to avoid hanging promises (lines 106-111)
- Success listener started before button click to catch immediate confirmations (lines 116-120)
- Explicit timeout on popup wait to handle 1-click scenarios (lines 126-128)
- Graceful timeout handling that proceeds to wait for blockchain confirmation (lines 146-165)
- Message validation when popup appears (line 144)
- Comprehensive JSDoc documentation (lines 82-97)
Based on learnings
182-256: Consistent claim and close implementationThe method correctly applies the same robust patterns:
- Optional execution when button not visible (lines 183-189)
- Success listener started before interactions (lines 194-198)
- Graceful timeout handling for 1-click scenarios (lines 230-250)
- Validates both claim_limit and cancel_limit when popup appears (lines 228-229)
Based on learnings
258-311: Clean claim all implementationThe method correctly implements the same robust patterns as other transaction methods. The simpler implementation (no message validation) is appropriate since the "Transaction Successful" confirmation provides sufficient verification.
Based on learnings
|
@JohnnyWyles I'm happy with this now! |
JohnnyWyles
deleted the
jason/fe-1539-intermittent-test-failure-due-to-keplr-popup-context-closure
branch
Fix Intermittent E2E Test Failures with Enhanced Timeout and Retry Logic
Summary
Comprehensive fix for intermittent E2E test failures caused by Keplr popup race conditions, insufficient timeouts, and 1-click trading scenarios. Introduces robust retry logic, optimized promise handling, and proper timeout management across all transaction tests.
Linear Task
FE-1539: Intermittent test failure due to Keplr popup context closure
Changes
🔧 Keplr Popup Race Condition Fix (Initial)
Problem: Intermittent "Target page, context or browser has been closed" errors when Keplr popup appeared.
Solution:
waitForEvent('page')callsbuyAndGetWalletMsg()andsellAndGetWalletMsg()🔄 Retry Logic for Transaction Operations
Problem: Transient failures in CI environments due to timing issues and network conditions.
Solution:
Affected methods:
buyAndGetWalletMsg()sellAndGetWalletMsg()swapAndApprove()buyAndApprove()sellAndApprove()🎯 1-Click Trading Support
Problem: Tests would timeout waiting for Keplr popup that never appears when 1-click trading is enabled.
Solution:
{ msgContentAmount: undefined }when popup doesn't appearmsgContentAmountvalidation conditional in tests⏱️ Increased Transaction Success Timeouts
Problem: "Timed out 7000ms waiting for Transaction Successful" errors for split route swaps.
Solution:
isTransactionSuccesfultimeout from 7s → 15s across all testsplaywright.config.ts⚡ Fail-Fast Balance Checks
Problem: Tests would waste ~94s retrying transactions when balance was already insufficient.
Solution:
insufficientBalErrorvisibility once to fail fast🎯 Tx Success Promise Pattern (Latest)
Problem: Successful transaction listener could be blocked by the wallet popup listener, especially if 1-ct is enabled.
Solution:
Architecture change:
Benefits:
📝 Updated Page Object Methods (6 methods)
trade-page.ts:
buyAndGetWalletMsg()- Now waits for success internally with 40s timeout per retrysellAndGetWalletMsg()- Now waits for success internally with 40s timeout per retryswapAndApprove()- Now waits for success internally with 40s timeoutbuyAndApprove()- Now waits for success internally with 40s timeoutsellAndApprove()- Now waits for success internally with 40s timeouttransactions-page.ts:
cancelLimitOrder()- Now waits for success internally with 40s timeout📚 JSDoc Documentation Added
Added comprehensive JSDoc comments to 6 transaction methods documenting:
🐛 Keplr Extension Race Condition Fix
Problem: When running all tests together,
decompress()wasn't awaited, causing some tests to start before Keplr extension extraction completed → "manifest missing" error.Solution:
UnzipExtension.getPathToExtension()asyncawaittodecompress()call inunzip-extension.tsSetupKeplr.setupWalletKeplr()to await extension extractionTesting
✅ Verified Locally
Local playwright test run sanity check:
📊 Performance & Reliability Impact
Timeout Strategy:
1-Click Trading:
Fail-Fast Balance Checks:
Risk Assessment
Low Risk - Changes improve test reliability without modifying application code:
Additional Notes