Repository navigation
fix(account): show correct order amounts in Unfillable edit-approval modal - #7718
Conversation
…modal Two issues on the Account modal's "Unfillable" warning when the user opened "Edit partial approval": 1. The amount preview at the top of the modal pulled from `useDerivedTradeState()`, which reflects the *current* trade form, not the pending order being warned about. With another (e.g. limit) order draft on the form, the preview showed the wrong sell/buy amounts. 2. The token symbol could wrap mid-line in the "Please top up X balance" copy. Thread the order's `outputAmount` from `ActivityDetails` down through `OrderFillabilityWarning` -> `OrderPartialApprove` -> `PartialApproveAmountModal` -> `ChangeApproveAmountModal` / `ChangeApproveAmountModalPure` to `SwapAmountPreview`, which now accepts explicit `inputCurrencyAmount` / `outputCurrencyAmount` overrides and only falls back to the derived trade state when none are provided. Wrap the token symbol in the not-enough-balance copy in a `NoWrap` span so it stays on a single line. Closes cowprotocol#7686
|
@tenderdeve is attempting to deploy a commit to the cow-dev Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAn ChangesPartial Approval Modal Output Amount Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In
`@apps/cowswap-frontend/src/modules/account/containers/OrderPartialApprove/OrderPartialApprove.tsx`:
- Around line 20-25: The OrderPartialApproveProps in the OrderPartialApprove.tsx
file is currently declared as a type alias using export type syntax, but should
be converted to an exported interface instead to follow the coding guidelines.
Replace the type alias declaration with an interface declaration that has the
same name OrderPartialApproveProps and the same properties (amountToApprove,
amountToBuy, isPartialApproveEnabledBySettings, and orderId) with their
respective types, ensuring all properties maintain their optional status where
applicable.
In
`@apps/cowswap-frontend/src/modules/account/pure/OrderFillabilityWarning/index.tsx`:
- Line 12: The styled component NoWrap is currently being imported as a named
import from the styled module, but the codebase follows a namespace import
pattern for styled files. Change the import statement to use namespace style
import (import * as styledEl from the styled file path), then update all
references to NoWrap throughout the file to use the namespace prefix
(styledEl.NoWrap).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f4f18dd6-8b69-4831-8004-457860af43b1
📒 Files selected for processing (8)
apps/cowswap-frontend/src/modules/account/containers/OrderPartialApprove/OrderPartialApprove.tsxapps/cowswap-frontend/src/modules/account/containers/Transaction/ActivityDetails.tsxapps/cowswap-frontend/src/modules/account/pure/OrderFillabilityWarning/index.tsxapps/cowswap-frontend/src/modules/account/pure/OrderFillabilityWarning/styled.tsapps/cowswap-frontend/src/modules/erc20Approve/containers/ChangeApproveAmountModal/ChangeApproveAmountModal.tsxapps/cowswap-frontend/src/modules/erc20Approve/containers/ChangeApproveAmountModal/ChangeApproveAmountModalPure.tsxapps/cowswap-frontend/src/modules/erc20Approve/containers/PartialApproveAmountModal/PartialApproveAmountModal.tsxapps/cowswap-frontend/src/modules/erc20Approve/containers/SwapAmountPreview/SwapAmountPreview.tsx
| export type OrderPartialApproveProps = { | ||
| amountToApprove: CurrencyAmount<Currency> | ||
| amountToBuy?: CurrencyAmount<Currency> | ||
| isPartialApproveEnabledBySettings?: boolean | ||
| orderId?: string | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use an exported interface for component props.
Line 20 exports OrderPartialApproveProps as a type alias in a component file. Please switch this to an exported interface.
As per coding guidelines, component files should export only components and their interfaces.
Suggested change
-export type OrderPartialApproveProps = {
+export interface OrderPartialApproveProps {
amountToApprove: CurrencyAmount<Currency>
amountToBuy?: CurrencyAmount<Currency>
isPartialApproveEnabledBySettings?: boolean
orderId?: string
}🤖 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/account/containers/OrderPartialApprove/OrderPartialApprove.tsx`
around lines 20 - 25, The OrderPartialApproveProps in the
OrderPartialApprove.tsx file is currently declared as a type alias using export
type syntax, but should be converted to an exported interface instead to follow
the coding guidelines. Replace the type alias declaration with an interface
declaration that has the same name OrderPartialApproveProps and the same
properties (amountToApprove, amountToBuy, isPartialApproveEnabledBySettings, and
orderId) with their respective types, ensuring all properties maintain their
optional status where applicable.
Source: Coding guidelines
|
|
||
| import { | ||
| ApproveWrapper, | ||
| NoWrap, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use namespace style imports for styled modules.
Line 12 extends a named import from the style file; this should use a namespace style import pattern.
As per coding guidelines, import styles as import * as styledEl from './X.styled'.
🤖 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/account/pure/OrderFillabilityWarning/index.tsx`
at line 12, The styled component NoWrap is currently being imported as a named
import from the styled module, but the codebase follows a namespace import
pattern for styled files. Change the import statement to use namespace style
import (import * as styledEl from the styled file path), then update all
references to NoWrap throughout the file to use the namespace prefix
(styledEl.NoWrap).
Source: Coding guidelines
elena-zh
added
the
trigger-preview
|
Cloudflare Pages preview mirror Preview branch URL: https://github.com/cowprotocol/cowswap/tree/cf-preview/pr-7718 Source fork branch:
|
github-actions
Bot
removed
the
trigger-preview
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
Works as expected, thank you
fairlighteth
left a comment
fairlighteth
left a comment
There was a problem hiding this comment.
✅ AI Review (Codex GPT-5, worked 4m): no new findings
Review completed. I found no new non-duplicate comments worth posting.
Related context checked:
ActivityDetailsnow passes the orderoutputAmountthrough the unfillable-warning approval path, andSwapAmountPreviewuses the passed order amounts for the Account modal edit-approval preview.- The existing trade-flow partial approval caller still keeps the previous derived trade-state behavior for the buy-side preview.
- Existing open comments already cover the style issues around
OrderPartialApprovePropsand styled-module imports. - Existing human review says the repro scenario works as expected.
Generated using the pr-review skill from the CoW Protocol skills repo.
🧪 Scoped automated browser QA passed: mocked unfillable allowance modal statesOutcome
Run details
Artifacts Primary assertion result JSON: https://github.com/cowprotocol/cowswap/releases/download/pr-qa-assets/prqa-7718-chromium-mocked-approval-modal-33707aa-result.json Inline evidence image, combining the browser screenshot with the mocked inputs and pass assertions: How to retest
Not checked / follow-up
Generated with the |
✅ Security Review (Codex GPT-5): no new PR-introduced finding; DeepSec surfaced pre-existing approval-state riskReview completed on PR head No new PR-introduced security finding worth blocking this change on. DeepSec did surface two reports in the touched approval path, but after code inspection they collapse to one pre-existing root cause already present on DeepSec triage
Suggested follow-up Track or patch separately by scoping pending-order approval modal state to Review scope and related context
Generated using the |
Closes #7686.
Two UI bugs on the Account modal's "Unfillable" warning when opening Edit partial approval:
Fix
Testing steps
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes