fix(bridge): hide stale price impact while a new quote is loading#7711
fix(bridge): hide stale price impact while a new quote is loading#7711tenderdeve wants to merge 10 commits into
Conversation
When the destination token is changed in swap-and-bridge mode, the price impact briefly rendered a huge/scary value derived from the previous quote while the new bridge quote was still being fetched. `PriceImpactIndicator` already received the `loading` flag but rendered the (stale) percentage *and* the spinner side by side. Gate the percentage on `!priceImpactLoading` so only the spinner shows while a fresh quote is being computed, matching the rest of the bridge-quote loading UX. Closes cowprotocol#7429
|
@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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesPrice Impact Loading Suppression
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
elena-zh
added
the
trigger-preview
|
Cloudflare Pages preview mirror Preview branch URL: https://github.com/cowprotocol/cowswap/tree/cf-preview/pr-7711 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.
Hey @tenderdeve , thank you, but not fixed: the issue is reproducible with price impact in one chain.
The previous fix only stopped \`PriceImpactIndicator\` from rendering a
stale percentage during USD-loading. The reporter still saw a scary
value on same-chain quote refreshes: when the destination token
changes, the trade form keeps the previous \`outputCurrencyAmount\` until
the new quote lands, and during that window \`useFiatValuePriceImpact\`
happily divides the new input USD by the old output USD, producing a
huge nonsense %.
Pull \`isLoading\` / \`hasParamsChanged\` out of \`useTradeQuote()\` and
fold them into the price-impact loading flag. While that combined flag
is true, return \`{ priceImpact: undefined, isLoading: true }\` so every
consumer falls back to the loading state (spinner) instead of rendering
the stale value.
Closes cowprotocol#7429
|
@elena-zh updated in bad39f2 — the previous fix only suppressed the value while USD prices were loading. Same-chain repro hits a different window: the trade form keeps the stale |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/legacy/hooks/usePriceImpact/useFiatValuePriceImpact.ts`:
- Line 40: The timeout effect that manages `hasLoadingTimedOut` only resets this
flag when `inputToken` or `outputToken` change, but for amount-only quote
refreshes (no token change), the flag can remain `true` after 15 seconds,
bypassing stale-value suppression. To fix this, modify the dependency array and
logic of the timeout effect (around lines 43-54) to also reset
`hasLoadingTimedOut` when `quoteParamsChanged` is true, ensuring the timeout is
reset whenever a new quote request is triggered regardless of whether the tokens
changed. This prevents the stale-value bypass window for amount-only refreshes
without resetting the timer on every keystroke.
🪄 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: 7ace58b7-f587-4b6b-be06-6a920d75320c
📒 Files selected for processing (2)
apps/cowswap-frontend/src/common/pure/PriceImpactIndicator/index.tsxapps/cowswap-frontend/src/legacy/hooks/usePriceImpact/useFiatValuePriceImpact.ts
…quote-loading-price-impact
| // While a fresh quote is loading, don't expose the stale value at all — consumers | ||
| // hide the percentage when `priceImpact` is undefined, leaving just the spinner. | ||
| if (stillLoading) { | ||
| return { priceImpact: undefined, isLoading: true } |
There was a problem hiding this comment.
While a fresh quote is loading, don't expose the stale value at all —
This never happens because after the first quote is being loaded, the second quote (the fresh quote as you say) has hasLoadingTimedOut === true so stillLoading is never true again, after the first quote.
So this code and comment are incorrect
…n change The stale-value suppression relied on `hasLoadingTimedOut`, whose timeout only reset when the token pair changed. After the first 15s it stayed true, so later same-pair repricing exposed the stale price impact immediately instead of hiding it while the fresh quote loaded. Add `quoteParamsChanged` to the timeout effect so it re-arms for every new quote, while plain loading flicker still can't restart it (a stuck quote keeps timing out). Adds a regression test.
|
@kernelwhisperer you're right — the guard was dead after the first timeout. The 15s timeout only reset on a token-pair change, so once Fixed in 6354b1a: the timeout effect now also keys off |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
fairlighteth
left a comment
fairlighteth
left a comment
There was a problem hiding this comment.
⚠️ AI Review (Codex GPT-5, worked 21m): follow-up still misses an already-true quote-change state
Rechecked
- Author response:
6354b1ae6re-arms the timeout by addingquoteParamsChangedto the timeout effect dependency. - Code path:
apps/cowswap-frontend/src/legacy/hooks/usePriceImpact/useFiatValuePriceImpact.ts:59. - Test coverage:
useFiatValuePriceImpact.test.tscoversquoteParamsChanged: false -> true, but not a new same-pair quote while it is alreadytrue.
Result: Still broken. The fix re-arms the timeout when quoteParamsChanged flips to true, but useTradeQuotePolling can start another changed-params quote with pollQuoteRef.current(true) while the previous changed-params quote is still loading/timed out. In that path setLoading(true, ...) writes the same boolean again, the effect does not re-run, hasLoadingTimedOut stays true, and stale price impact can render immediately.
Suggested next step
- Reset from a quote request key / amount-change signal, or include the relevant same-pair quote params in the timeout reset condition.
- Add a regression test for the
quoteParamsChanged: true -> truecase.
🤖 Prompt for AI agents
Verify this follow-up finding against current PR code. The current timeout reset depends on quoteParamsChanged, so it only re-arms when the boolean changes. Check the case where hasLoadingTimedOut is true while quoteParamsChanged is already true, then another same-pair changed-params quote starts and setLoading(true, ...) writes the same boolean again. If still valid, reset the timeout from a quote request/params key or amount-change signal, and add a regression test for quoteParamsChanged staying true across a second same-pair quote request.
Generated using the pr-review skill from the CoW Protocol skills repo.
4 participants
Closes #7429.
When the destination token is switched in swap-and-bridge mode, the price impact field briefly flashes a huge / scary percentage derived from the previous quote while the new bridge quote is still being fetched.
PriceImpactIndicatoralready received theloadingflag, but rendered the (stale) percentage and the spinner side by side. Gate the percentage on!priceImpactLoadingso only the spinner shows while a fresh quote is being computed, matching the rest of the bridge-quote loading UX.File
apps/cowswap-frontend/src/common/pure/PriceImpactIndicator/index.tsxTesting steps
Summary by CodeRabbit