feat(e2e): test selectors and e2e mocks - #8036
Merged
Merged
Conversation
…l/cowswap into feat/e2e-playwright-3
…/e2e-playwright-3
…l/cowswap into feat/e2e-playwright-2
…l/cowswap into feat/e2e-playwright-3
…l/cowswap into feat/e2e-playwright-2
…l/cowswap into feat/e2e-playwright-3
…l/cowswap into feat/e2e-playwright-3
…l/cowswap into feat/e2e-playwright-3
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
This was referenced Aug 25, 2026
shoom3301
marked this pull request as ready for review
August 25, 2026 11:02
Danziger
reviewed
Aug 25, 2026
| // `useLayoutEffect` still runs synchronously before the browser paints (no visible flicker, | ||
| // unlike a plain `useEffect`), but as a commit-phase effect rather than a render-phase one, it's | ||
| // safe to update other components from. | ||
| useLayoutEffect(() => { |
Danziger
reviewed
Aug 25, 2026
|
|
||
| return ( | ||
| <Wrapper className={className}> | ||
| <Wrapper className={'collapsible-bridge-route' + (className ?? '')}> |
Contributor
There was a problem hiding this comment.
Missing a space here. In any case, I've added clsx recently, so you can use that here: clsx(className, 'collapsible-bridge-route')
Danziger
reviewed
Aug 25, 2026
| const isSigning = tab.id === 'signing' | ||
| return ( | ||
| <styledEl.TabButton | ||
| className="orders-table_tab" |
Danziger
reviewed
Aug 25, 2026
| const searchParams = new URLSearchParams(location.search) | ||
| const targetChainId = searchParams.get('targetChainId') | ||
| const recipient = searchParams.get('recipient') | ||
| /** |
Contributor
There was a problem hiding this comment.
Maybe not on this stack of PRs, but you might want to take a look at apps/cowswap-frontend/src/entities/routes/routes.atom.ts later.
Collaborator
Author
There was a problem hiding this comment.
Thanks! I really forgot about it!
Danziger
reviewed
Aug 25, 2026
| ref={ref} | ||
| id={id} | ||
| className={className} | ||
| className={'trade-form-blank-button ' + className} |
Contributor
There was a problem hiding this comment.
Not missing a space here, but you could also use clsx.
Danziger
approved these changes
Aug 25, 2026
Danziger
left a comment
Danziger
left a comment
Contributor
There was a problem hiding this comment.
A few minor comments, but approving already.
azebuado
approved these changes
Aug 25, 2026
Collaborator
There was a problem hiding this comment.
✅ Browser QA passed: sell-amount stability across currency switch, and the NEAR Intents bypass is confirmed absent from the shipped bundle
Outcome
- ✅ Passed: sell amount survives currency switching (CS-59) — typed "123" as sell amount, then switched the buy token 5 times in rapid succession (DAI → USDT → WBTC → COW → USDC, ~300ms apart, no settle time between switches). Sell amount stayed
123and the sell token stayedWETHon every switch — never reset to the "1 unit" default and never reverted to "Select a token". - ✅ Passed (primary security-relevant claim): the NEAR Intents signature-bypass branch is not just inert but entirely absent from the production bundle. Downloaded and grepped all JS chunks the preview loads:
recoverDepositAddressappears exactly twice (the SDK's real method definition + its one legitimate call site) — zero assignment-style overrides. The literal stringNODE_ENVappears exactly once in the whole bundle, and it's unrelated Vite env metadata, not theprocess.env.NODE_ENV !== 'production'guard. Terser fully dead-code-eliminated the bypass block, matching the PR's own claim. - Not checked: the
OrdersFromApiUpdater/PendingOrdersUpdaterpending→fulfilled race — per the PR's own reviewer note this only manifests under fast polling relative to fill time and isn't reliably reproducible through a preview URL; relying on the added unit test as the PR states. - Minor observation, non-blocking: the GTM/LaunchDarkly no-op swap (
window.__COWSWAP_E2E__) has noNODE_ENVguard, unlike the NEAR bypass. Not a security issue — both reads happen at module-load time before any page script could set the flag, so it's not externally triggerable — just noting the asymmetry with the NEAR check in case it's not intentional.
Run details
- Source: PR head
7902defvia the PR-linked previewswap-dev-git-e2e-tests-p2-cowswap-dev.vercel.app(base branche2e-tests/p1, stacked PR) - Environment: Linux; Chromium 151.0.7922.34 (Playwright), locale forced to
en-US - Wallet:
disconnected(both checked claims are wallet-independent) - AI assistance: Claude orchestrated Playwright browser execution, downloaded and grepped the deployed JS bundles for the bundle-inspection check, and drafted this note from the observed results.
How to retest
- Open the preview → Swap → enter a sell amount → switch the buy token a few times quickly → confirm the sell amount and sell token don't reset.
- For the NEAR guard:
curlthe app'sstatic/index-*.jschunk(s) and confirm no.recoverDepositAddress=assignment and no liveNODE_ENV !== 'production'branch survive minification.
Not checked / follow-up
⚠️ OrdersFromApiUpdater/PendingOrdersUpdaterrace (surplus-modal timing) — not browser-reproducible per the PR's own note; covered by unit test only.⚠️ getTokenFromMappingaddress-normalization change — exercised incidentally by normal token selection above (WETH/USDC/DAI/USDT/WBTC/COW all resolved correctly) but not independently stress-tested.
Commands + setup
- Preview:
https://swap-dev-git-e2e-tests-p2-cowswap-dev.vercel.app - Playwright context:
locale: 'en-US'(sandbox host locale is POSIX, which otherwise breaks the app'sIntlcalls — unrelated to the PR) - Bundle check: collected all same-origin
.jsresponse URLs loaded on initial page load, downloaded each,grep'd forCOWSWAP_E2E,recoverDepositAddress, the hardcoded attestor address, andNODE_ENV
Generated using the pr-qa skill.
Collaborator
There was a problem hiding this comment.
AI Review (Claude Sonnet 5, worked 9m)
Review completed. I found no new non-duplicate comments worth posting.
I went deep on the four risk areas this PR touches (render-phase-write races, the OrdersFromApiUpdater/PendingOrdersUpdater surplus-modal interaction, the address-key normalization change, and the NEAR Intents bypass guard) rather than stopping at "looks reasonable":
HydrateAtom.tsx,useSetupTradeStateFromUrl.ts: both move a render-phase atom write touseLayoutEffect. Checked the one call site ofuseSetupTradeStateFromUrl's new return value (useSetupTradeState.ts) — correctly consumes it, no other callers.OrdersFromApiUpdater.ts:_getOrdersToQueueForSurplusModal'sUiOrderType.SWAP && !getIsBridgeOrder(order)filter matchesPendingOrdersUpdater's existing eligibility check exactly (same file, line ~450-452). Traced whether both updaters could double-push the same order intosurplusModalAtom(itspush()has no dedup) —PendingOrdersUpdater'spendingRef.currentis resynced from store state every render, so it drops an order from its own pending bucket before it could re-fire once this updater has already marked it fulfilled; and even in the worst case,removeSurplusOrderAtomfilters out all matching IDs on dismiss, not just one. No user-visible double-modal.getTokenFromMapping.ts: swappingviem'sgetAddress(checksum) forgetAddressKey(lowercase) looked like a possible regression at first glance if the token maps were checksum-keyed. Checked — they aren't:useTokensByAddressMapForChain.tsanduseTokensForOrdersList.tsalready key everyTokensByAddressmap withgetAddressKey, so the old checksum-first lookup was actually the mismatched one (it only worked via thetokens[address]fallback, and only when callers happened to pass an already-lowercased address). This change makes the lookup consistent with how the maps are actually built, and as a side effect it also drops a latent crash:getAddress()throws on a malformed address,getAddressKey()doesn't.bridgingSdk.tsNEAR Intents bypass: confirmed by downloading and grepping the deployed preview's JS bundles (see the QA pass on this same PR) — theNODE_ENV !== 'production'branch is fully dead-code-eliminated in the production build, not just runtime-inert.
Review scope and related context
- Existing thread on
CollapsibleBridgeRoute/index.tsx:42,OrdersTabs.pure.tsx:60, andTradeFormBlankButton/index.tsx:115(all reviewer nitpicks about spacing/clsx) — not repeated here; PR is already approved and these are non-blocking style notes, not correctness issues. useNavigateOnCurrencySelection.ts's sticky-ref fix for CS-104 (sell token reverting to "Select a token" after picking the buy token) — same pattern as the twouseLayoutEffectfixes above. Checked the asymmetry betweeninputCurrencyId(always reads the ref) andoutputCurrencyId(prefers fresh state, falls back to the ref) — both converge to the same value in the non-race case since the ref is kept in sync every render; not a bug, just a stylistic inconsistency not worth a nitpick on its own.- The interval-wrapping changes (
getUpdaterIntervalinallowancesAtom.ts,PriorityTokensUpdater.tsx,useTokenAllowance.ts,legacy/state/orders/consts.ts) are mechanical and low-risk. window.__COWSWAP_E2E__/__COWSWAP_E2E_FEATURE_FLAGS__gate GTM/LaunchDarkly/feature-flags without theNODE_ENVguard the NEAR bypass has — already flagged as a non-blocking observation in the QA pass on this PR (module-load-time read only, not settable by an external page script, so not exploitable).
Generated using the pr-review skill from the CoW Protocol skills repo.
… e2e-tests/p1 # Conflicts: # pnpm-lock.yaml
…into e2e-tests/p2
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
window.__COWSWAP_E2E__runtime flag: under it, GTM analytics and the LaunchDarkly provider are swapped for no-ops, feature flags are read fromwindow.__COWSWAP_E2E_FEATURE_FLAGS__instead, and several update-interval consts (order-book polling, allowances, priority tokens, limit/expired-order checks) are compressed to ~2s via a newgetUpdaterInterval()helper incommon-const.NODE_ENV !== 'production'and the e2e flag is set — dead code in every deployed build (prod, staging, and Vercel previews all run the production webpack build), needed because e2e's mocked NEAR quote/attestation fixtures can't carry a real signature.data-testid/id/className) to several components so tests don't rely on style-derived selectors:CurrencyInputPanelfiat amount,PriceImpactIndicator,TradeDetailsAccordion,EthFlowStepper/Step,OrdersTable,AccountDetailsactivity list, approve-modeToggle,CollapsibleBridgeRoute,SnackbarPopup,ConfirmDetailsItem/ReviewOrderModalAmountRow,TradeFormBlankButton.HydrateAtomwrote to its atom during render, which could update an already-mounted sibling mid-render (React warns "Cannot update a component while rendering a different component") and the write could be silently dropped — observed as the sell token intermittently reverting to "Select a token". Moved the write intouseLayoutEffect.OrdersFromApiUpdaterandPendingOrdersUpdaterboth poll and write order status independently. IfOrdersFromApiUpdaterwroteFULFILLEDfirst, the order leftPendingOrdersUpdater's locally-tracked pending bucket before it could detect the transition, and the "Transaction completed" surplus modal never appeared.OrdersFromApiUpdaternow detects the pending→fulfilled transition itself and queues the modal.useSetupTradeAmountsFromUrl's "has an amount ever been set" ref was overwritten every render instead of staying sticky, so switching the buy/sell currency (which transiently reads the amount back asnullfor one render) could stomp a real typed sell amount with the "1 unit" default — the flakyenterSellAmount('1000')behavior tracked as CS-59.getTokenFromMappingnow normalizes addresses withgetAddressKeyfrom@cowprotocol/cow-sdkinstead of viem'sgetAddress, per the repo's address-handling convention.Why
QA Testing
Preview URL QA:
Developer verification:
smokeCI check passed on this PR.OrdersFromApiUpdater.test.ts) and the URL-driven amount defaulting (useSetupTradeAmountsFromUrl.test.ts,useNavigateOnCurrencySelection.test.tsx).Reviewer note:
OrdersFromApiUpdater/PendingOrdersUpdaterrace only manifests when polling is fast relative to order fill time (production's default 30s order-book poll vs. e2e's compressed ~2s), so it isn't reliably reproducible through a preview URL — coverage relies on the added unit test.window.__COWSWAP_E2E__branches and the NEAR Intents signature bypass are inert in any real deployment (guarded byNODE_ENV !== 'production', which every deployed build fails) — worth confirming that guard specifically since it's the one security-relevant change in this PR.Preview URLs