-
Notifications
You must be signed in to change notification settings - Fork 160
feat(widget): option to disable bridging #6730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR relocates Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/widget-lib/src/types.ts (1)
261-265: Consider clarifying interaction with related parameters.The new
disableCrossChainSwapparameter is well-documented and follows naming conventions. However, consider documenting how it interacts withtargetChainId(line 211) andhideBridgeInfo(line 301).For example, if a user sets
disableCrossChainSwap: truebut also provides atargetChainId, what's the expected behavior? ShouldtargetChainIdbe ignored, or should this combination be validated?💡 Suggested documentation enhancement
/** * Disables cross-chain swaps (bridging) + * When enabled, targetChainId will be ignored and cross-chain functionality will be unavailable. * Defaults to false. */ disableCrossChainSwap?: boolean
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cowswap-frontend/src/modules/application/containers/App/Updaters.tsx(1 hunks)apps/cowswap-frontend/src/modules/bridge/index.ts(1 hunks)apps/cowswap-frontend/src/modules/bridge/updaters/BridgingEnabledUpdater.ts(1 hunks)libs/widget-lib/src/types.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 5443
File: apps/cowswap-frontend/src/modules/swap/containers/ConfirmSwapModalSetup/index.tsx:71-71
Timestamp: 2025-02-20T15:59:33.749Z
Learning: The swap module in apps/cowswap-frontend/src/modules/swap/ is marked for deletion in PR #5444 as part of the swap widget unification effort.
📚 Learning: 2025-09-25T08:49:32.256Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/entities/bridgeProvider/useBridgeSupportedNetworks.test.tsx:62-67
Timestamp: 2025-09-25T08:49:32.256Z
Learning: In the cowswap-frontend codebase, when testing hooks that use multiple bridge providers, both providers are always properly mocked as complete jest.Mocked<BridgeProvider<BridgeQuoteResult>> objects with all required methods stubbed, ensuring no undefined returns that could break the hook logic.
Applied to files:
apps/cowswap-frontend/src/modules/bridge/updaters/BridgingEnabledUpdater.ts
📚 Learning: 2025-02-20T15:59:33.749Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 5443
File: apps/cowswap-frontend/src/modules/swap/containers/ConfirmSwapModalSetup/index.tsx:71-71
Timestamp: 2025-02-20T15:59:33.749Z
Learning: The swap module in apps/cowswap-frontend/src/modules/swap/ is marked for deletion in PR #5444 as part of the swap widget unification effort.
Applied to files:
apps/cowswap-frontend/src/modules/bridge/updaters/BridgingEnabledUpdater.tslibs/widget-lib/src/types.tsapps/cowswap-frontend/src/modules/application/containers/App/Updaters.tsx
📚 Learning: 2025-09-19T11:38:59.206Z
Learnt from: fairlighteth
Repo: cowprotocol/cowswap PR: 6232
File: apps/cowswap-frontend/src/modules/tokensList/pure/ChainsSelector/index.tsx:199-200
Timestamp: 2025-09-19T11:38:59.206Z
Learning: The makeBuildClickEvent function in apps/cowswap-frontend/src/modules/tokensList/pure/ChainsSelector/index.tsx takes five parameters: defaultChainId, contextLabel, mode, isSwapMode, and chainsCount. The chainsCount parameter is used to determine the CrossChain flag in analytics events.
Applied to files:
apps/cowswap-frontend/src/modules/bridge/updaters/BridgingEnabledUpdater.ts
📚 Learning: 2025-08-12T05:57:08.021Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6138
File: libs/hook-dapp-lib/src/hookDappsRegistry.ts:1-1
Timestamp: 2025-08-12T05:57:08.021Z
Learning: The matchHooksToDapps function in libs/hook-dapp-lib/src/utils.ts provides backward compatibility for permit hooks through function selector detection (EIP_2612_PERMIT_SELECTOR and DAI_PERMIT_SELECTOR) rather than dappId matching, making it robust against dappId changes.
Applied to files:
apps/cowswap-frontend/src/modules/bridge/updaters/BridgingEnabledUpdater.ts
📚 Learning: 2025-10-10T20:28:16.565Z
Learnt from: fairlighteth
Repo: cowprotocol/cowswap PR: 6347
File: apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.tsx:49-49
Timestamp: 2025-10-10T20:28:16.565Z
Learning: In apps/cowswap-frontend/src/modules/trade, TradeConfirmation follows a two-layer architecture: TradeConfirmationView (pure/stateless) in pure/TradeConfirmation/index.tsx renders the UI, while TradeConfirmation (container) in containers/TradeConfirmation/index.tsx wraps it to freeze props during pending trades (via useStableTradeConfirmationProps), wire in signing state (useSigningStep), and inject trade confirmation state (useTradeConfirmState). Consuming modules should import the container TradeConfirmation from 'modules/trade' to preserve this stateful behavior.
<!-- [add_learning]
When reviewing component refactoring in apps/cowswap-frontend/src/modules/trade, recognize the pattern where a pure view component (e.g., TradeConfirmationView) is separated from a stateful container (e.g., TradeConfirmation) that wraps it. The container adds runtime state management (prop stabilization, signing state, etc.) while the view remains testable and composable. Do not flag usages that import th...
Applied to files:
apps/cowswap-frontend/src/modules/bridge/updaters/BridgingEnabledUpdater.tsapps/cowswap-frontend/src/modules/application/containers/App/Updaters.tsx
📚 Learning: 2025-06-23T07:03:50.760Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 5859
File: apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts:76-82
Timestamp: 2025-06-23T07:03:50.760Z
Learning: In the useTradeQuotePolling hook, there are two useLayoutEffect hooks that work together: one resets the counter to 0 when the confirmation modal closes, and another automatically triggers pollQuote(false, true) whenever the counter reaches 0. This creates an intentional chain reaction for immediate quote updates.
Applied to files:
apps/cowswap-frontend/src/modules/bridge/updaters/BridgingEnabledUpdater.ts
📚 Learning: 2025-07-24T10:00:45.353Z
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6009
File: apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/hooks/useHighFeeWarning.ts:36-36
Timestamp: 2025-07-24T10:00:45.353Z
Learning: In the CowSwap frontend, when there's a bridgeFee present in the transaction, the isSell flag is always true for business reasons. This means bridge transactions are always structured as sell operations, which ensures consistent currency handling in fee percentage calculations.
Applied to files:
libs/widget-lib/src/types.ts
📚 Learning: 2025-11-19T10:18:23.717Z
Learnt from: limitofzero
Repo: cowprotocol/cowswap PR: 6537
File: apps/cowswap-frontend/src/modules/trade/pure/PartnerFeeRow/index.tsx:33-35
Timestamp: 2025-11-19T10:18:23.717Z
Learning: In apps/cowswap-frontend/src/modules/trade/pure/PartnerFeeRow/index.tsx, when there is no partner fee (amount is null/undefined, bps is missing, or amount equals 0), FreeFeeRow is rendered with withTimelineDot={false} hardcoded. This is intentional design—free fee rows should not show the timeline dot regardless of what the parent component passes, as they have a distinct visual treatment from actual fee rows.
Applied to files:
libs/widget-lib/src/types.ts
🧬 Code graph analysis (1)
apps/cowswap-frontend/src/modules/bridge/updaters/BridgingEnabledUpdater.ts (3)
apps/cowswap-frontend/src/modules/trade/hooks/useTradeTypeInfo.ts (1)
useTradeTypeInfo(6-8)libs/common-hooks/src/useIsBridgingEnabled.ts (1)
useSetIsBridgingEnabled(9-11)apps/cowswap-frontend/src/common/constants/routes.ts (1)
Routes(10-45)
⏰ 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). (2)
- GitHub Check: Setup
- GitHub Check: Cypress
🔇 Additional comments (5)
apps/cowswap-frontend/src/modules/application/containers/App/Updaters.tsx (1)
18-18: LGTM! Clean refactoring to move BridgingEnabledUpdater to the bridge module.The import path update correctly reflects the relocation of
BridgingEnabledUpdaterfromcommon/updaterstomodules/bridge, which improves code organization by grouping bridge-related updaters together.apps/cowswap-frontend/src/modules/bridge/index.ts (1)
12-12: LGTM! Public API correctly expanded.The export properly exposes
BridgingEnabledUpdaterfrom the bridge module, consistent with the existing pattern forPendingBridgeOrdersUpdater.apps/cowswap-frontend/src/modules/bridge/updaters/BridgingEnabledUpdater.ts (2)
5-5: LGTM! Feature implementation is correct.The updater now properly respects the
disableCrossChainSwapwidget parameter:
- Defaults to
false(bridging enabled by default) for backward compatibility- Bridging is enabled only when on the SWAP route AND cross-chain swaps are not disabled
- The logic correctly implements the intended behavior
Also applies to: 13-15
8-8: LGTM! Import path improvement.Using the centralized routes constant from
common/constants/routesinstead of a relative path improves maintainability.libs/widget-lib/src/types.ts (1)
180-180: LGTM! Formatting cleanup.Removing the trailing semicolon is a style improvement with no semantic impact.
elena-zh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works, thank you!
Tested with this Json:
{"baseUrl": "https://swap-dev-git-feat-disable-bridge-in-widget-cowswap-dev.vercel.app/",
"disableCrossChainSwap": true}
| /** | ||
| * Disables cross-chain swaps (bridging) | ||
| * Defaults to false. | ||
| */ | ||
| disableCrossChainSwap?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add it to the docs as well? https://docs.cow.fi/cow-protocol/tutorials/widget#cowswapwidgetparams
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Added it: cowprotocol/docs#581
Summary
To Test
{"disableCrossChainSwap": true}to Raw JSON paramsSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.