Repository navigation
fix(trade): preserve URL recipient when chainId initializes - #7465
alfetopito merged 11 commits into
Conversation
When a user navigates to a URL containing ?recipient=vitalik.eth, the recipient was being cleared on page load because the chain change effect in useResetRecipient fired as the wallet's chainId went from undefined to its initial value. Add hasRecipientInUrl as a guard so the chain change reset is skipped when the recipient was explicitly provided in the URL. Fixes cowprotocol#7323
|
@tenderdeve is attempting to deploy a commit to the cow-dev Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRecipient reset logic now detects URL-provided recipients synchronously from query params and preserves them across reset effects; chain-change and hook-exit effects are gated on that detection. The chain-change hook replaces mount-skip logic with an initialization ref that ignores the first undefined→defined hydration. ChangesRecipient URL Prefill Preservation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey @tenderdeve , thank you for the contribution!
Unfortunately, this does not fix the issue:
5c190c9 to
618c26c
Compare
The previous guard derived hasRecipientInUrl from the tradeStateFromUrl atom, which is only populated one render after useResetRecipient first runs. On the first commit the reset effects therefore saw an empty value and cleared a recipient that was explicitly provided in the URL. - Derive hasRecipientInUrl synchronously from location.search so it is correct on the first render, and apply the guard to every reset effect rather than only the chainId-change one. - In useResetRecipientOnChainChange, skip the initial `undefined -> chainId` transition so app init is not treated as a user-driven chain change. Fixes cowprotocol#7323
|
@elena-zh hey, Thanks for catching that — you were right, the first patch didn't fix it. fixed and Tested locally with ?recipient=vitalik.eth, ?recipient=0x…, and no param — recipient is preserved in the first two, stays empty in the last, no console errors. One known limitation worth flagging: targetChainId comes from the buy token, so a chainId → undefined → chainId sequence (buy token cleared then reselected) also gets skipped. Rare in practice, but happy to tighten the guard to track "first real chain seen". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/trade/hooks/useResetRecipient.ts (1)
66-79: ⚡ Quick winRemove unused dependency
isNativeIn.The
isNativeInvariable is included in the dependency array (line 76) but is not referenced anywhere in the effect body. This causes the effect to re-run unnecessarily wheneverisNativeInchanges, even though it doesn't affect the effect's logic.♻️ Remove extraneous dependency
}, [ recipient, postHooksRecipientOverride, prevPostHooksRecipientOverride, - isNativeIn, hasRecipientInUrl, onChangeRecipient, ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cowswap-frontend/src/modules/trade/hooks/useResetRecipient.ts` around lines 66 - 79, The useEffect contains an unused dependency isNativeIn causing unnecessary re-runs; update the effect declared with useEffect so its dependency array only includes the values actually referenced in the body (recipient, postHooksRecipientOverride, prevPostHooksRecipientOverride, hasRecipientInUrl, onChangeRecipient) and remove isNativeIn, ensuring the effect logic around recipientOverrideWasRemoved and onChangeRecipient(null) remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/cowswap-frontend/src/modules/trade/hooks/useResetRecipient.ts`:
- Around line 66-79: The useEffect contains an unused dependency isNativeIn
causing unnecessary re-runs; update the effect declared with useEffect so its
dependency array only includes the values actually referenced in the body
(recipient, postHooksRecipientOverride, prevPostHooksRecipientOverride,
hasRecipientInUrl, onChangeRecipient) and remove isNativeIn, ensuring the effect
logic around recipientOverrideWasRemoved and onChangeRecipient(null) remains
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 64d0a732-c986-4c4e-8bcf-8afc4123741a
📒 Files selected for processing (2)
apps/cowswap-frontend/src/modules/trade/hooks/useResetRecipient.tsapps/cowswap-frontend/src/modules/trade/hooks/useResetRecipientOnChainChange.ts
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Hey, thank you for the 2nd try to fix it. But still, it does not work:
https://swap-dev-git-fork-tenderdeve-fix-7323-prefil-8a6b53-cowswap-dev.vercel.app/#/1/swap/WETH/COW?recipient=vitalik.eth
|
@elena-zh Hey I tried to test it on the localhost and it is working. Maybe i am not following the correct way to reproduce the bug, can you please help me out with the steps to reproduce the bugs that you are following. |
Hey @tenderdeve , I've provided you the test link in my comment above #7465 (review) |
|
@elena-zh, Hey did we redeploy the code after my second fix. I think we are still testing either with old commit or the deployment is not reflecting the latest change. because i have tried to test it on localhost and it works correctly, as you can see in the video i have attached in the previous comment. |
Hey @tenderdeve , could you please sigh the CLA? |
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
@elena-zh, Hey did we redeploy the code after my second fix. I think we are still testing either with old commit or the deployment is not reflecting the latest change. because i have tried to test it on localhost and it works correctly, as you can see in the video i have attached in the previous comment.
I've redeployed the changes: now it LGTM.
Will approve the PR once you sign CLA.
|
I have read the CLA Document and I hereby sign the CLA |
fairlighteth
left a comment
fairlighteth
left a comment
There was a problem hiding this comment.
Thank you for this PR. Please review these AI findings:
-
P2 apps/cowswap-frontend/src/modules/trade/hooks/useResetRecipientOnChainChange.ts:30
The new if (prev === undefined) return skips every undefined -> chainId transition, not just app initialization. If the buy token is cleared or temporarily unset, then the user selects BTC/SOL, recipient validation is skipped and an incompatible existing recipient can remain. Track the one initial hydration skip separately, then validate later undefined -> chainId transitions. -
Nit apps/cowswap-frontend/src/modules/trade/hooks/useResetRecipient.ts:76
CodeRabbit’s unused isNativeIn dependency comment is still valid. It is not a blocker, but removing it avoids unnecessary effect reruns.
Skip only the initial hydration (undefined -> chainId at app start) once, then validate every subsequent target chain change. Previously the undefined-guard skipped all undefined -> chainId transitions, so clearing the buy token then selecting a non-EVM chain (BTC/SOL) left an incompatible recipient in place. Also drop the unused isNativeIn dependency from the hook-removal effect.
|
@fairlighteth hey, did those changes as well. |
fairlighteth
left a comment
fairlighteth
left a comment
There was a problem hiding this comment.
✅ Manual verification: recipient URL and reset flows pass
- Tested
/#/1/swap/WETH/COW?recipient=vitalik.eth: recipient stays prefilled after load and wallet connect. - Tested no-recipient URL: recipient stays empty.
- Tested manual recipient plus wallet/network chain switch: recipient is cleared.
- Tested EVM recipient, cleared buy token, then selected BTC/SOL output: incompatible recipient is cleared.
- Result: all checks passed.
✅ AI Review (Codex GPT-5, worked 12m): no new findings
Review completed. I found no new non-duplicate comments worth posting.
Related context checked
apps/cowswap-frontend/src/modules/trade/hooks/useResetRecipientOnChainChange.ts: @fairlighteth's prior P2 is addressed.apps/cowswap-frontend/src/modules/trade/hooks/useResetRecipient.ts: CodeRabbit/@fairlighteth's unusedisNativeIndependency note is fixed.apps/cowswap-frontend/src/modules/trade/hooks/useResetRecipient.ts: the original?recipient=vitalik.ethflow is now guarded synchronously fromuseLocation().search.
|
@tenderdeve there's a merge conflict now. Could you review and resolve this? If all good we'd happy to merge. |
…-recipient-from-url # Conflicts: # apps/cowswap-frontend/src/modules/trade/hooks/useResetRecipient.ts
@fairlighteth Resolved conflicts. |
When a URL contains a
?recipient=parameter (e.g./#/1/swap/WETH/COW?recipient=vitalik.eth), the recipient field was cleared on page load before the user could interact with it.The chain-change effect in
useResetRecipientfires as the wallet'schainIdinitializes fromundefinedto its real value, unconditionally callingonChangeRecipient(null)and wiping the URL-provided recipient before it could stick.Adding
hasRecipientInUrlas a guard prevents the reset when recipient was provided via URL.Fixes #7323
Summary
Fixes #7323
Added
!hasRecipientInUrlcondition to the chain-change reset effect inuseResetRecipientso URL-provided recipients are preserved on page load.To Test
/#/1/swap/WETH/COW?recipient=vitalik.ethvitalik.ethon page loadchainIdinitializesBackground
useResetRecipienthas a dedicated effect that resets the recipient wheneverchainIdchanges. On initial page load, the wallet'schainIdtransitions fromundefinedto its real value, which triggers this effect. At that pointhasRecipientInUrlis alreadytrue(parsed from the URL atom), but the effect had no guard for it — soonChangeRecipient(null)fired unconditionally, clearing the field.The fix adds
hasRecipientInUrlto both the condition and the dependency array of that specific effect. The initial-load effect already had this guard; only the chain-change effect was missing itSummary by CodeRabbit