Skip to content

Conversation

@shoom3301
Copy link
Collaborator

@shoom3301 shoom3301 commented Dec 3, 2025

Summary

Since cross-chain swaps are not possible in TWAP, it doesn't make sense to suggest it when current trade is a cross-chain swap.

image

To Test

  1. When current trade is a cross-chain swap, then "Minimize price impact with TWAP" banner should not be displayed

Summary by CodeRabbit

  • Documentation

    • Updated and refined translation strings for improved messaging clarity.
    • Added new notification labels for accessibility and user interface navigation.
  • Bug Fixes

    • Enhanced banner display logic to prevent duplicate messaging in specific scenarios.

✏️ Tip: You can customize this high-level summary in your review settings.

@shoom3301 shoom3301 requested review from a team December 3, 2025 12:48
@shoom3301 shoom3301 self-assigned this Dec 3, 2025
@vercel
Copy link

vercel bot commented Dec 3, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Updated (UTC)
cowfi Ready Ready Preview Dec 3, 2025 0:59am
explorer-dev Ready Ready Preview Dec 3, 2025 0:59am
sdk-tools Ready Ready Preview Dec 3, 2025 0:59am
swap-dev Ready Ready Preview Dec 3, 2025 0:59am
widget-configurator Ready Ready Preview Dec 3, 2025 0:59am
1 Skipped Deployment
Project Deployment Preview Updated (UTC)
cosmos Ignored Ignored Dec 3, 2025 0:59am

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2025

Walkthrough

Translation updates reactivate and revise localization strings; TWAP suggestion banner is now gated to not render during bridging trades; explicit ReactNode return types added to improve type safety in banner and warnings components.

Changes

Cohort / File(s) Summary
Localization/Translations
apps/cowswap-frontend/src/locales/en-US.po
Rewrote "I just received surplus on" → "I received surplus on"; reactivated deprecated "Bridge costs" entry; added new translations for "Trade alert settings" and "View jobs (opens in a new tab)" in NotificationSidebar component
Type Annotations
apps/cowswap-frontend/src/modules/swap/pure/TwapSuggestionBanner.tsx, apps/cowswap-frontend/src/modules/swap/containers/Warnings/index.tsx
Added explicit ReactNode return type annotations to exported functions; added ReactNode import from React
Bridging State Guard
apps/cowswap-frontend/src/modules/swap/containers/Warnings/index.tsx
Imported useIsCurrentTradeBridging hook and gated TWAP suggestion banner rendering to only display when not in a bridging trade

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Translation updates are straightforward and repetitive
  • Type annotations are non-functional improvements
  • Bridging guard logic is a single conditional check—verify the hook behavior and ensure the condition correctly prevents banner display during bridging operations

Possibly related PRs

Suggested labels

Bridge, Bug

Suggested reviewers

  • elena-zh
  • alfetopito

Poem

🐰 A bridging trade hops high, yet needs no TWAP cheer,
Our banners know to pause when crossing lanes appear,
New words translate the wins, fresh labels bloom so true,
With types now firmly set, the code shines clean and new! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The description includes a summary with context and screenshots, but the 'To Test' section is incomplete and lacks detailed verification checkpoints. Expand the 'To Test' section with detailed steps and checkbox-style verification criteria, and consider adding a 'Background' section for additional context if needed.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: preventing TWAP suggestion for cross-chain swaps, which aligns with all the code modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/do-not-suggest-twap-when-bridge

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dd05bb7 and 8097607.

📒 Files selected for processing (3)
  • apps/cowswap-frontend/src/locales/en-US.po (5 hunks)
  • apps/cowswap-frontend/src/modules/swap/containers/Warnings/index.tsx (2 hunks)
  • apps/cowswap-frontend/src/modules/swap/pure/TwapSuggestionBanner.tsx (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.
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6009
File: apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/HighFeeWarningTooltipContent.tsx:23-33
Timestamp: 2025-07-24T16:42:53.154Z
Learning: In apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/HighFeeWarningTooltipContent.tsx, the use of toFixed(2) for percentage formatting in tooltip content is intentional and differs from the banner message formatting that uses toSignificant(2, undefined, Rounding.ROUND_DOWN). This formatting difference serves different UX purposes and should not be flagged as inconsistent.
📚 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/swap/containers/Warnings/index.tsx
  • apps/cowswap-frontend/src/modules/swap/pure/TwapSuggestionBanner.tsx
  • apps/cowswap-frontend/src/locales/en-US.po
📚 Learning: 2025-07-24T16:42:53.154Z
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6009
File: apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/HighFeeWarningTooltipContent.tsx:23-33
Timestamp: 2025-07-24T16:42:53.154Z
Learning: In apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/HighFeeWarningTooltipContent.tsx, the use of toFixed(2) for percentage formatting in tooltip content is intentional and differs from the banner message formatting that uses toSignificant(2, undefined, Rounding.ROUND_DOWN). This formatting difference serves different UX purposes and should not be flagged as inconsistent.

Applied to files:

  • apps/cowswap-frontend/src/modules/swap/containers/Warnings/index.tsx
  • apps/cowswap-frontend/src/modules/swap/pure/TwapSuggestionBanner.tsx
  • apps/cowswap-frontend/src/locales/en-US.po
📚 Learning: 2025-10-13T19:41:31.440Z
Learnt from: limitofzero
Repo: cowprotocol/cowswap PR: 6351
File: apps/cowswap-frontend/src/modules/erc20Approve/containers/TradeApproveModal/useTradeApproveCallback.ts:87-121
Timestamp: 2025-10-13T19:41:31.440Z
Learning: In apps/cowswap-frontend/src/modules/erc20Approve, useApproveCallback returns Promise<TransactionResponse | undefined> and is distinct from useApproveCurrency, which can return Promise<TransactionReceipt | SafeMultisigTransactionResponse>. When reviewing approval flows, verify which hook is actually being used before flagging Safe wallet concerns.

Applied to files:

  • apps/cowswap-frontend/src/modules/swap/containers/Warnings/index.tsx
📚 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/swap/containers/Warnings/index.tsx
📚 Learning: 2025-06-16T15:58:00.268Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5830
File: apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/index.tsx:1-2
Timestamp: 2025-06-16T15:58:00.268Z
Learning: JSX can be imported as a named export from React in modern React versions (React 17+). The import `import { JSX } from 'react'` is valid and does not cause compilation errors.

Applied to files:

  • apps/cowswap-frontend/src/modules/swap/containers/Warnings/index.tsx
📚 Learning: 2025-06-16T16:01:46.729Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5831
File: apps/cowswap-frontend/src/modules/application/containers/AppMenu/index.tsx:7-9
Timestamp: 2025-06-16T16:01:46.729Z
Learning: React Router v7 restructured packages - NavLink and other core routing components should be imported from 'react-router' (not 'react-router-dom'). In v7, 'react-router-dom' mainly re-exports for backward compatibility, while 'react-router' is the new preferred import path for most components.

Applied to files:

  • apps/cowswap-frontend/src/modules/swap/containers/Warnings/index.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/swap/pure/TwapSuggestionBanner.tsx
📚 Learning: 2025-07-24T16:43:47.639Z
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6009
File: apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/highFeeWarningHelpers.ts:18-20
Timestamp: 2025-07-24T16:43:47.639Z
Learning: In apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/highFeeWarningHelpers.ts, the formatFeePercentage function uses ROUND_DOWN with toSignificant(2) for "at least X%" messaging. This ensures the displayed percentage is never higher than the actual fee, making the "at least" phrasing accurate. For example, if the actual fee is 25.4%, displaying "at least 25%" is correct, but "at least 26%" would be misleading.

Applied to files:

  • apps/cowswap-frontend/src/locales/en-US.po
⏰ 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: Cypress
  • GitHub Check: Setup
🔇 Additional comments (7)
apps/cowswap-frontend/src/locales/en-US.po (5)

34-36: Surplus wording deprecation looks correct

Marking "I just received surplus on" as obsolete while introducing the new wording elsewhere avoids stale copy and keeps catalog clean.


616-618: New surplus copy is consistent and clear

"I received surplus on" with identical msgstr is grammatically correct and matches the intended shorter wording for BenefitComponents.


2209-2212: “Trade alert settings” string is well‑scoped

Label text is concise, matches its NotificationSidebar context, and msgstr mirrors msgid as expected for en-US.


3749-3751: Jobs link copy is explicit and accessible

"View jobs (opens in a new tab)" clearly communicates both action and behavior, which is good for accessibility.


6082-6085: “Bridge costs” label aligns with existing bridge fee messaging

Generic “Bridge costs” label is consistent with other bridge-related strings (e.g., “Bridge costs are at least …”) and will work well as a column/row label.

apps/cowswap-frontend/src/modules/swap/containers/Warnings/index.tsx (1)

1-32: TWAP banner correctly suppressed for cross‑chain swaps

Using useIsCurrentTradeBridging() to add !isCurrentTradeBridging to showTwapSuggestionBanner matches the PR goal: TWAP is only suggested when ADVANCED orders are enabled and the trade is not a bridge. The explicit ReactNode return type for Warnings is also accurate and keeps typing tight without affecting runtime behavior. Please double‑check, per your test plan, that the banner still appears on high price‑impact same‑chain swaps and never appears on “Swap and Bridge” flows.

apps/cowswap-frontend/src/modules/swap/pure/TwapSuggestionBanner.tsx (1)

1-2: Explicit ReactNode return type is appropriate and non‑breaking

Importing ReactNode and typing TwapSuggestionBanner to return ReactNode makes the component signature explicit while preserving the existing behavior of returning either null or JSX. This should remain fully compatible with all current JSX usages.

Also applies to: 41-47


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

msgstr "Signature is undefined!"

#: apps/cowswap-frontend/src/modules/orderProgressBar/pure/BenefitComponents.tsx
msgid "I just received surplus on"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p.s. I didn't update it manully, it was updated automatically

@shoom3301 shoom3301 merged commit 81d42ff into develop Dec 4, 2025
15 checks passed
@shoom3301 shoom3301 deleted the fix/do-not-suggest-twap-when-bridge branch December 4, 2025 10:10
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants