Skip to content

Conversation

@kernelwhisperer
Copy link
Contributor

@kernelwhisperer kernelwhisperer commented Dec 30, 2025

Summary

Fixes #4127

The issue was that the form validation was not taking into account the isBalancesLoading state.

To Test

As per the issue:

Connect to a wallet (using WC or open the app inside Safe --> most likely you will immediately reproduce the issue)

AR: each time when app is loading after changing a network/connection, 'Couldn't load balances' message appears and is displayed up to 20 seconds

Summary by CodeRabbit

  • Bug Fixes
    • Improved trade form validation to correctly handle balance loading states, preventing premature error messages while balance data is being fetched.

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

@vercel
Copy link

vercel bot commented Dec 30, 2025

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

Project Deployment Review Updated (UTC)
explorer-dev Ready Ready Preview Dec 30, 2025 3:58pm
sdk-tools Ready Ready Preview Dec 30, 2025 3:58pm
swap-dev Ready Ready Preview Dec 30, 2025 3:58pm
widget-configurator Ready Ready Preview Dec 30, 2025 3:58pm
1 Skipped Deployment
Project Deployment Review Updated (UTC)
cosmos Ignored Ignored Dec 30, 2025 3:58pm

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 30, 2025

Walkthrough

The PR integrates balance loading state tracking into trade form validation to prevent false "Couldn't load balances" errors during the asset balance fetching phase. It adds an isBalancesLoading flag to the validation context and updates the balance validation logic to suppress the error message while balances are actively loading.

Changes

Cohort / File(s) Summary
Balance Loading State Integration
apps/cowswap-frontend/src/modules/tradeFormValidation/hooks/useTradeFormValidationContext.ts
Added useTokensBalances import and destructured isBalancesLoading from the hook. Exposed isBalancesLoading in the returned validation context object and included it in memoization dependencies.
Validation Context Type
apps/cowswap-frontend/src/modules/tradeFormValidation/types.ts
Added isBalancesLoading: boolean field to TradeFormValidationCommonContext interface to track balance-fetching state.
Balance Validation Logic
apps/cowswap-frontend/src/modules/tradeFormValidation/services/validateTradeForm.ts
Updated balance validation condition: BalancesNotLoaded error is now only triggered when canPlaceOrderWithoutBalance is false AND isBalancesLoading is false, preventing error display during active balance loading.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested reviewers

  • elena-zh
  • shoom3301
  • alfetopito

Poem

🐰 A balance-loading tale so fine,
No false errors shall cross the line,
While assets fetch from far and wide,
We wait with patience, side by side,
Until the coins align, all mine!

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: hiding the 'couldn't load balances' message when balances are loading, which directly addresses the primary issue in the PR.
Description check ✅ Passed The description identifies the linked issue (#4127), explains the problem, and provides testing steps. However, the 'To Test' section lacks detailed checkboxes/verification steps compared to the template.
Linked Issues check ✅ Passed The code changes add isBalancesLoading tracking to the validation context and prevent the BalancesNotLoaded error during loading, directly addressing issue #4127's requirement to suppress the message while balances load.
Out of Scope Changes check ✅ Passed All changes are scoped to the trade form validation module and directly support the issue objective: tracking balance loading state and suppressing validation errors during load.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c35a88e and 9a92f75.

📒 Files selected for processing (3)
  • apps/cowswap-frontend/src/modules/tradeFormValidation/hooks/useTradeFormValidationContext.ts
  • apps/cowswap-frontend/src/modules/tradeFormValidation/services/validateTradeForm.ts
  • apps/cowswap-frontend/src/modules/tradeFormValidation/types.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/tradeFormValidation/types.ts
  • apps/cowswap-frontend/src/modules/tradeFormValidation/hooks/useTradeFormValidationContext.ts
  • apps/cowswap-frontend/src/modules/tradeFormValidation/services/validateTradeForm.ts
📚 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/tradeFormValidation/hooks/useTradeFormValidationContext.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). (2)
  • GitHub Check: Cypress
  • GitHub Check: Setup
🔇 Additional comments (3)
apps/cowswap-frontend/src/modules/tradeFormValidation/types.ts (1)

75-75: LGTM! Balance loading state added to validation context.

The addition of isBalancesLoading to the validation context interface is correctly typed and enables the validation logic to distinguish between "balances are still loading" and "balances failed to load."

apps/cowswap-frontend/src/modules/tradeFormValidation/services/validateTradeForm.ts (1)

30-30: LGTM! Validation now correctly suppresses balance errors during loading.

The updated condition !canPlaceOrderWithoutBalance && !isBalancesLoading ensures that balance validation (lines 147-153) only runs after balances have finished loading. This directly resolves the false positive "Couldn't load balances" message that appeared during the ~20 second loading period described in issue #4127.

Also applies to: 146-154

apps/cowswap-frontend/src/modules/tradeFormValidation/hooks/useTradeFormValidationContext.ts (1)

3-3: No issues found. The useTokensBalances hook is properly exported from @cowprotocol/balances-and-allowances with the expected signature (): BalancesState, which includes the isLoading: boolean property. The integration in useTradeFormValidationContext.ts is correct: the hook is called at the top level with no arguments, the isLoading property is correctly destructured and aliased, and it's properly included in the memoized context and dependencies.


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.

@kernelwhisperer kernelwhisperer self-assigned this Dec 30, 2025
@kernelwhisperer kernelwhisperer requested review from a team December 30, 2025 15:58
}

if (!canPlaceOrderWithoutBalance) {
if (!canPlaceOrderWithoutBalance && !isBalancesLoading) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should add another TradeFormValidation state and display Fetching balances <Loader/>?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

'Couldn't load balances' message is displayed after a wallet connection

3 participants