fix(swap): fix quote double loading and prevent re-fetch quote loop (#6675) #6771
Conversation
|
@crutch12 is attempting to deploy a commit to the cow Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds a cached suggested slippage to tradeQuote state, reads smart slippage from that cache, throttles/refuses rapid slippage-only quote re-fetches (~1.5s), converts quote-deadline math/tests to millisecond precision, introduces a useSyncedRef hook, and adds a utility to detect slippage-only param changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI / Hooks
participant Poll as useTradeQuotePolling
participant BFF as BFF Client
participant State as tradeQuoteAtom
participant Smart as useSmartSlippageFromQuote
rect rgb(240,248,255)
Note over UI,Poll: Initial quote request (normal flow)
UI->>Poll: requestQuote(quoteParams)
Poll->>BFF: fetchQuote(quoteParams)
BFF-->>Poll: quote + suggestedSlippageBps
Poll->>State: updateTradeQuote(quote, suggestedSlippageBps)
State-->>State: cache suggestedSlippageBps, set localQuoteTimestamp
end
rect rgb(255,250,240)
Note over UI,Poll: Rapid user slippage adjustments (throttled)
UI->>Poll: requestQuote(newQuoteParams)
Poll->>Poll: compare prevQuoteParams (checkOnlySlippageBpsChanged)
alt only slippage changed AND lastQuoteAge < 1.5s
Poll-->>Poll: skip fetch (throttle)
else
Poll->>BFF: fetchQuote(newQuoteParams)
BFF-->>State: quote + suggestedSlippageBps
State->>State: update cached suggestedSlippageBps (if applicable)
end
end
rect rgb(245,255,250)
Note over Smart,State: Smart slippage usage (consumer)
Smart->>State: read suggestedSlippageBps (cached)
Smart-->>UI: return bounded slippage value
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.test.ts (2)
48-59: Test inconsistency:localQuoteTimestampunits don't match implementation.The test uses
localQuoteTimestamp = 1713167232which appears to be in seconds (a Unix epoch timestamp), while the implementation now expectslocalQuoteTimestampin milliseconds (fromDate.now()). Additionally,validForfrom the API is in seconds, not milliseconds.With the current implementation (
Math.ceil(localQuoteTimestamp / 1000) + validFor), whenlocalQuoteTimestamp = 1713167232:
expectedValidTo = Math.ceil(1713167232 / 1000) + 60000 = 1713168 + 60000 = 1773168- But
quoteValidTo = 1713167232 + 60000 = 1713227232These don't match, so the test would fail. The
localQuoteTimestampin this test should be in milliseconds (e.g.,1713167232000) andvalidForshould remain in seconds (e.g.,60).🔎 Proposed fix
it('When expected validTo and quote validTo are the same, then should return 0', () => { - const validFor = 60 * ONE_SEC // 1 minute - const localQuoteTimestamp = 1713167232 + const validFor = 60 // 1 minute in seconds (as returned by API) + const localQuoteTimestamp = 1713167232000 // ms expect( getQuoteTimeOffset( getQuoteState({ validFor, localQuoteTimestamp, - quoteValidTo: localQuoteTimestamp + validFor, + quoteValidTo: Math.ceil(localQuoteTimestamp / 1000) + validFor, }), ), ).toEqual(0) })
161-170: Same unit inconsistency pattern inisQuoteExpiredtests.These tests have the same issue:
deadlineis multiplied byONE_SECmaking it milliseconds, butvalidForin the API response is seconds. ThequoteValidTocalculation also incorrectly adds millisecond values.This pattern repeats across all
isQuoteExpiredtests (lines 161, 178, 193, 214, 227, 230). Given the PR mentions "TODO:yarn testcurrently fails", these unit mismatches are likely the cause.
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useQuoteParams.ts (1)
47-53: Consider simplifying the redundant conditional.Both
'user'and'smart'branches returntradeSlippage.value, making the nested ternary redundant.🔎 Proposed simplification
- // Slippage value for quote params: - // - User slippage: always include (re-quotes when user changes it) - // - Smart slippage: always include (re-quotes when user changes amount, etc.) - const slippageBps = - tradeSlippage.type === 'user' - ? tradeSlippage.value - : tradeSlippage.type === 'smart' - ? tradeSlippage.value - : undefined + // Slippage value for quote params: + // - User slippage: always include (re-quotes when user changes it) + // - Smart slippage: always include (re-quotes when user changes amount, etc.) + const slippageBps = + tradeSlippage.type === 'user' || tradeSlippage.type === 'smart' + ? tradeSlippage.value + : undefined
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
apps/cowswap-frontend/src/common/services/bff/cowBffClient.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useQuoteParams.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useSmartSlippageFromQuote.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.tsapps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.tsapps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.test.tsapps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.tsapps/cowswap-frontend/src/modules/tradeQuote/utils/quoteParamsChanges.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5830
File: apps/cowswap-frontend/src/modules/tradeQuote/hooks/useSmartSlippageFromQuote.ts:5-9
Timestamp: 2025-06-16T15:58:57.402Z
Learning: In the cowswap system, 0 bps will never be suggested as a slippage value, so falsy checks on suggested slippage values are safe and won't drop valid 0 values.
📚 Learning: 2025-06-16T15:58:57.402Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5830
File: apps/cowswap-frontend/src/modules/tradeQuote/hooks/useSmartSlippageFromQuote.ts:5-9
Timestamp: 2025-06-16T15:58:57.402Z
Learning: In the cowswap system, 0 bps will never be suggested as a slippage value, so falsy checks on suggested slippage values are safe and won't drop valid 0 values.
Applied to files:
apps/cowswap-frontend/src/common/services/bff/cowBffClient.tsapps/cowswap-frontend/src/modules/tradeQuote/utils/quoteParamsChanges.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useSmartSlippageFromQuote.ts
📚 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/tradeQuote/hooks/useQuoteParams.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useSmartSlippageFromQuote.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts
📚 Learning: 2025-09-25T08:46:43.815Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/modules/tradeQuote/services/fetchAndProcessQuote.test.ts:376-385
Timestamp: 2025-09-25T08:46:43.815Z
Learning: In fetchAndProcessQuote.ts, when bridgingSdk.getBestQuote() returns null, no loading state reset is needed because loading state management is handled through onQuoteResult() callback for results and processQuoteError() for errors. The null case is intentionally designed not to trigger any manager methods.
Applied to files:
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useQuoteParams.tsapps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useSmartSlippageFromQuote.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.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/tradeQuote/hooks/useQuoteParams.tsapps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.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/tradeQuote/hooks/useQuoteParams.ts
📚 Learning: 2025-08-08T13:56:18.009Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6125
File: libs/tokens/src/updaters/TokensListsUpdater/index.tsx:29-31
Timestamp: 2025-08-08T13:56:18.009Z
Learning: In libs/tokens/src/updaters/TokensListsUpdater/index.tsx, the project’s current Jotai version requires using `unstable_getOnInit` (not `getOnInit`) in atomWithStorage options; keep `{ unstable_getOnInit: true }` until Jotai is upgraded.
Applied to files:
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts
📚 Learning: 2025-03-25T10:03:35.157Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5532
File: libs/common-hooks/src/useOnScroll.tsx:22-38
Timestamp: 2025-03-25T10:03:35.157Z
Learning: In React useEffect hooks, refs (RefObjects) should generally be included in dependency arrays even though changes to ref.current don't trigger effects. This is because the ref object itself could change between renders (especially in custom hooks where refs are passed as parameters), and it follows React's best practice of including all external values used in the effect.
Applied to files:
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts
🧬 Code graph analysis (3)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteParamsChanges.ts (1)
apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts (1)
TradeQuoteState(12-23)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.test.ts (1)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.ts (1)
getOrderValidTo(62-72)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (1)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteParamsChanges.ts (1)
checkOnlySlippageBpsChanged(7-21)
⏰ 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 (9)
apps/cowswap-frontend/src/common/services/bff/cowBffClient.ts (1)
56-59: LGTM - Debug test line preserved as documentation.The commented-out random slippage line serves as useful documentation for reproducing the re-fetch loop issue during future debugging. The implementation correctly returns the slippageBps from the API response.
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.ts (2)
29-29: LGTM - Timestamp unit alignment.The change correctly uses
localQuoteTimestampdirectly since it's now stored in milliseconds (viaDate.now()), eliminating the redundant* 1000multiplication.
54-54: LGTM - Proper unit conversion for time offset calculation.The
Math.ceil(localQuoteTimestamp / 1000)correctly converts the ms timestamp to seconds before addingvalidFor(which is in seconds), maintaining proper unit consistency withquoteValidTo(also in seconds).apps/cowswap-frontend/src/modules/tradeQuote/hooks/useSmartSlippageFromQuote.ts (1)
14-15: LGTM - Core fix for the re-fetch loop.Reading from the cached
slippageBpsfield instead ofquote.quoteResults.suggestedSlippageBpsbreaks the update cycle. The falsy check with|| nullis safe since, based on learnings, 0 bps is never suggested as a slippage value.apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteParamsChanges.ts (1)
7-21: LGTM - Clean utility for detecting slippage-only changes.The logic correctly identifies when only
slippageBpshas changed between quote params by:
- Ensuring not in loading state
- Confirming slippageBps values differ
- Verifying all other fields (excluding
signerreference) are equal via deep comparisonapps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (2)
56-59: LGTM - Correct ref update timing for comparison.Using
useEffectto updateprevQuoteParamsRefensures the ref holds the previous value whenuseLayoutEffectruns (sinceuseLayoutEffectexecutes beforeuseEffect). This timing is intentional and correct for the comparison logic.
86-101: LGTM - Effective throttle for slippage-only changes.The throttling logic correctly:
- Detects when only
slippageBpschanged using the new utility- Skips re-fetch if the last quote was received within 1.5 seconds
- Falls through to normal polling for other parameter changes
This breaks the re-fetch loop described in the PR objectives by preventing cascading updates when slippage adjusts from the quote response.
apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts (2)
21-22: LGTM - New cached slippage field.The
slippageBpsfield correctly caches the suggested slippage from the quote response, enabling the throttling mechanism in the polling hook.Also applies to: 34-34
59-68: LGTM - Proper slippage caching with FAST quote handling.The logic correctly:
- Preserves the previous quote when
nextState.quoteis undefined (partial updates)- Updates
localQuoteTimestamptoDate.now()only when a new quote arrives- Caches
slippageBpsfrom optimal quotes while ignoring FAST quotes (which return a default value)The comment on line 66 helpfully documents why FAST quote slippage is ignored.
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.test.ts
Show resolved
Hide resolved
crutch12
force-pushed
the
fix/swap-bridge-quote-loaded-twice
branch
from
685b982 to
434cb44
Compare
crutch12
force-pushed
the
fix/swap-bridge-quote-loaded-twice
branch
from
434cb44 to
eac18d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.test.ts (1)
47-92: Critical: Multiple tests have unit mismatches.Several other tests in this file have similar unit issues:
Lines 47-92 (
getQuoteTimeOffsettests):
validForis multiplied byONE_SEC(milliseconds)localQuoteTimestampis a plain number like1713167232(appears to be seconds)quoteValidTois calculated aslocalQuoteTimestamp + validFor, mixing seconds + millisecondsLines 157-240 (
isQuoteExpiredtests):
deadlineis multiplied byONE_SEC(milliseconds)localQuoteTimestampis set toNOW_TIME(milliseconds)quoteValidTois calculated aslocalQuoteTimestamp + deadline(both milliseconds), but should be in secondsThe implementation expects:
validFor: seconds (see line 54 in quoteDeadline.ts)quoteValidTo: Unix timestamp in secondslocalQuoteTimestamp: milliseconds (see line 29 in quoteDeadline.ts)🔎 Recommended approach to fix all tests
For
getQuoteTimeOffsettests (lines 47-92):
- Keep
validForin seconds (remove* ONE_SEC)- Keep
localQuoteTimestampin seconds- Calculate
quoteValidToin secondsFor
isQuoteExpiredtests (lines 157-240):
- Keep
deadline(which isvalidFor) in seconds (remove* ONE_SEC)- Keep
localQuoteTimestampasNOW_TIME(milliseconds)- Calculate
quoteValidToasMath.ceil(localQuoteTimestamp / 1000) + deadline(seconds)Update all expectations to match the seconds-based return values.
Example for lines 47-60:
it('When expected validTo and quote validTo are the same, then should return 0', () => { - const validFor = 60 * ONE_SEC // 1 minute - const localQuoteTimestamp = 1713167232 + const validFor = 60 // 1 minute in seconds + const localQuoteTimestamp = 1713167232000 // milliseconds expect( getQuoteTimeOffset( getQuoteState({ validFor, localQuoteTimestamp, - quoteValidTo: localQuoteTimestamp + validFor, + quoteValidTo: Math.ceil(localQuoteTimestamp / 1000) + validFor, }), ), ).toEqual(0) })Also applies to: 157-240
♻️ Duplicate comments (1)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.test.ts (1)
112-123: Critical: Test uses incorrect units despite past review.The test multiplies
deadlineandoffsetbyONE_SEC(converting to milliseconds), but the implementation expects these values in seconds:
- Line 54 in
quoteDeadline.ts:expectedValidTo = Math.ceil(localQuoteTimestamp / 1000) + validFor—validFormust be in seconds- Line 67-68 in
getOrderValidTo:const now = Date.now() / 1000— returns seconds- Line 118:
quoteValidTo: localQuoteTimestamp + deadline + offsetmixes milliseconds (localQuoteTimestamp) with millisecond-converted deadline/offset, butquoteValidToshould be a Unix timestamp in seconds- Line 122: Expects
NOW_TIME + deadline + offset(milliseconds), butgetOrderValidToreturns secondsThis was flagged in a previous review as "✅ Addressed" but the fix was not applied.
🔎 Proposed fix to use correct units
it('ValidTo should be now + deadline + timeOffset', () => { - const deadline = 5400 * ONE_SEC // 1.5 hours - const offset = 3600 * ONE_SEC // 1 hour + const deadline = 5400 // 1.5 hours in seconds + const offset = 3600 // 1 hour in seconds const localQuoteTimestamp = NOW_TIME const quoteDeadlineParams = { validFor: deadline, - quoteValidTo: localQuoteTimestamp + deadline + offset, + quoteValidTo: Math.ceil(localQuoteTimestamp / 1000) + deadline + offset, localQuoteTimestamp: localQuoteTimestamp, } - expect(getOrderValidTo(deadline, getQuoteState(quoteDeadlineParams))).toEqual(NOW_TIME + deadline + offset) + const expectedSeconds = Math.floor(deadline + NOW_TIME / 1000 - offset) + expect(getOrderValidTo(deadline, getQuoteState(quoteDeadlineParams))).toEqual(expectedSeconds) })
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (1)
26-26: Optional: Consider removing the eslint-disable directive.The
max-lines-per-functionrule is disabled for the entire function. If possible, consider refactoring to reduce function size rather than disabling the rule. However, this can be addressed in a future PR if needed.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/cowswap-frontend/src/common/services/bff/cowBffClient.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useQuoteParams.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useSmartSlippageFromQuote.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.tsapps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.tsapps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.test.tsapps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.tsapps/cowswap-frontend/src/modules/tradeQuote/utils/quoteParamsChanges.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/cowswap-frontend/src/modules/tradeQuote/hooks/useSmartSlippageFromQuote.ts
- apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteParamsChanges.ts
- apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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.
📚 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/tradeQuote/hooks/useTradeQuotePolling.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useQuoteParams.ts
📚 Learning: 2025-08-08T13:56:18.009Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6125
File: libs/tokens/src/updaters/TokensListsUpdater/index.tsx:29-31
Timestamp: 2025-08-08T13:56:18.009Z
Learning: In libs/tokens/src/updaters/TokensListsUpdater/index.tsx, the project’s current Jotai version requires using `unstable_getOnInit` (not `getOnInit`) in atomWithStorage options; keep `{ unstable_getOnInit: true }` until Jotai is upgraded.
Applied to files:
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts
📚 Learning: 2025-03-25T10:03:35.157Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5532
File: libs/common-hooks/src/useOnScroll.tsx:22-38
Timestamp: 2025-03-25T10:03:35.157Z
Learning: In React useEffect hooks, refs (RefObjects) should generally be included in dependency arrays even though changes to ref.current don't trigger effects. This is because the ref object itself could change between renders (especially in custom hooks where refs are passed as parameters), and it follows React's best practice of including all external values used in the effect.
Applied to files:
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts
📚 Learning: 2025-09-25T08:46:43.815Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/modules/tradeQuote/services/fetchAndProcessQuote.test.ts:376-385
Timestamp: 2025-09-25T08:46:43.815Z
Learning: In fetchAndProcessQuote.ts, when bridgingSdk.getBestQuote() returns null, no loading state reset is needed because loading state management is handled through onQuoteResult() callback for results and processQuoteError() for errors. The null case is intentionally designed not to trigger any manager methods.
Applied to files:
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.tsapps/cowswap-frontend/src/modules/tradeQuote/hooks/useQuoteParams.ts
📚 Learning: 2025-06-16T15:58:57.402Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5830
File: apps/cowswap-frontend/src/modules/tradeQuote/hooks/useSmartSlippageFromQuote.ts:5-9
Timestamp: 2025-06-16T15:58:57.402Z
Learning: In the cowswap system, 0 bps will never be suggested as a slippage value, so falsy checks on suggested slippage values are safe and won't drop valid 0 values.
Applied to files:
apps/cowswap-frontend/src/common/services/bff/cowBffClient.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/tradeQuote/hooks/useQuoteParams.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/tradeQuote/hooks/useQuoteParams.ts
🧬 Code graph analysis (2)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.test.ts (1)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.ts (1)
getOrderValidTo(62-72)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (1)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteParamsChanges.ts (1)
checkOnlySlippageBpsChanged(7-21)
⏰ 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 (4)
apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteDeadline.ts (1)
29-29: LGTM! Timestamp calculations now correctly use millisecond precision.The changes properly align
localQuoteTimestamphandling to use milliseconds throughout:
- Line 29 removes the
* 1000multiplication sincelocalQuoteTimestampis already in milliseconds- Line 54 converts milliseconds to seconds before adding
validForThis fixes the timing drift mentioned in the PR objectives.
Also applies to: 54-54
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (2)
56-59: LGTM! Tracking previous quote params enables throttling logic.The new effect correctly captures the previous
quoteParamsfor comparison in the throttling logic below.
86-101: LGTM! Throttling breaks the slippage re-fetch loop.The throttling logic effectively prevents the quote → slippage → quote loop by:
- Detecting when only
slippageBpschanged (line 86-90)- Checking if the last quote was fetched within 1.5 seconds (line 93-98)
- Skipping the fetch if both conditions are met
The timestamp arithmetic is correct (both
Date.now()andlocalQuoteTimestampare in milliseconds), and the 1.5s interval aligns with the PR objective.apps/cowswap-frontend/src/modules/tradeQuote/hooks/useQuoteParams.ts (1)
47-53: LGTM! Smart slippage now always included, aligned with throttling strategy.Removing the loading check for smart slippage simplifies the logic and works correctly with the throttling mechanism in
useTradeQuotePolling.ts. The 1.5s throttle prevents rapid re-fetches when slippage updates, making it safe to always include the smart slippage value.
| return { | ||
| slippageBps: data.slippageBps, | ||
| // slippageBps: data.slippageBps + Math.floor(Math.random() * 25), // uncomment to test smart slippage re-fetch quote loop problem (fixed) (#6675) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug comment before merging.
The commented-out line 58 is for testing the re-fetch loop scenario. This debug code should be removed before merging to prevent accidental activation in production.
🔎 Proposed cleanup
log(`Retrieved slippage tolerance from API: ${data.slippageBps} BPS`)
- return {
- slippageBps: data.slippageBps,
- // slippageBps: data.slippageBps + Math.floor(Math.random() * 25), // uncomment to test smart slippage re-fetch quote loop problem (fixed) (#6675)
- }
+ return { slippageBps: data.slippageBps }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| slippageBps: data.slippageBps, | |
| // slippageBps: data.slippageBps + Math.floor(Math.random() * 25), // uncomment to test smart slippage re-fetch quote loop problem (fixed) (#6675) | |
| } | |
| return { slippageBps: data.slippageBps } |
🤖 Prompt for AI Agents
In apps/cowswap-frontend/src/common/services/bff/cowBffClient.ts around lines 56
to 59, remove the commented-out debug line that mutates slippageBps ("//
slippageBps: data.slippageBps + Math.floor(Math.random() * 25)...") so the
returned object only uses the real value (slippageBps: data.slippageBps); delete
the comment entirely to avoid accidental activation in production and keep the
code clean.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (1)
28-28: Consider refactoring to reduce function complexity.The eslint-disable indicates the function has grown large. As noted in the PR description, the current throttling approach is a workaround. Consider the suggested alternative of tracking intent in
tradeQuotesAtomto better separate concerns and reduce this function's complexity.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.tslibs/common-hooks/src/index.tslibs/common-hooks/src/useSyncedRef.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 Learning: 2025-03-25T10:03:35.157Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5532
File: libs/common-hooks/src/useOnScroll.tsx:22-38
Timestamp: 2025-03-25T10:03:35.157Z
Learning: In React useEffect hooks, refs (RefObjects) should generally be included in dependency arrays even though changes to ref.current don't trigger effects. This is because the ref object itself could change between renders (especially in custom hooks where refs are passed as parameters), and it follows React's best practice of including all external values used in the effect.
Applied to files:
libs/common-hooks/src/useSyncedRef.ts
📚 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/tradeQuote/hooks/useTradeQuotePolling.ts
📚 Learning: 2025-08-08T13:56:18.009Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6125
File: libs/tokens/src/updaters/TokensListsUpdater/index.tsx:29-31
Timestamp: 2025-08-08T13:56:18.009Z
Learning: In libs/tokens/src/updaters/TokensListsUpdater/index.tsx, the project’s current Jotai version requires using `unstable_getOnInit` (not `getOnInit`) in atomWithStorage options; keep `{ unstable_getOnInit: true }` until Jotai is upgraded.
Applied to files:
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.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/tradeQuote/hooks/useTradeQuotePolling.ts
🧬 Code graph analysis (1)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (3)
libs/common-hooks/src/useSyncedRef.ts (1)
useSyncedRef(9-23)apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useIsSmartSlippageApplied.ts (1)
useIsSmartSlippageApplied(3-5)apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteParamsChanges.ts (1)
checkOnlySlippageBpsChanged(7-21)
⏰ 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 (6)
libs/common-hooks/src/useSyncedRef.ts (1)
9-23: LGTM! Clean implementation of synchronized ref.The implementation correctly provides an immutable ref wrapper that always reflects the latest value. The direct assignment to
ref.currentduring render (line 12) is safe since refs don't trigger re-renders, and the frozen wrapper withuseMemo([], ...)ensures a stable object identity across renders.libs/common-hooks/src/index.ts (1)
26-26: LGTM!Export is correctly added and consistent with the existing pattern.
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (4)
2-2: LGTM! Necessary imports for throttling feature.All new imports are appropriately used in the throttling implementation to prevent the re-fetch loop in smart slippage mode.
Also applies to: 4-4, 9-9, 22-22, 26-26
58-61: LGTM! Correct pattern for tracking previous params.The ref initialization and useEffect update correctly preserve the previous
quoteParamsvalue for comparison in the useLayoutEffect. The execution order ensures the useLayoutEffect sees the old value before the useEffect updates it.
63-63: LGTM! Appropriate use of useSyncedRef.Using
useSyncedRefhere is the right choice—it lets the effect access the latest smart slippage state whenquoteParamschanges without addingisSmartSlippageAppliedto the dependencies (which would trigger unnecessary effect executions).
90-107: Throttling logic is correct and timestamp precision is consistent.Verification confirms
localQuoteTimestampis consistently set in milliseconds viaDate.now()(tradeQuoteAtom.ts line 65) and used throughout with millisecond-based arithmetic (ONE_SEC = 1000 in tests). The comparison at line 103 correctly subtracts millisecond timestamps and compares against a millisecond-based throttle interval.The implementation properly:
- Isolates slippage-only changes via
checkOnlySlippageBpsChanged- Safely handles missing timestamps (skips throttling)
- Uses refs to access latest state without dependency churn
- Returns early to prevent redundant fetches
- Uses consistent millisecond precision for all timestamp operations
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| : undefined | ||
| // in "smart" slippage mode slippageBps updates on every fetch /quote response | ||
| // so we should throttle duplicated additional requests caused by following slippageBps updates to prevent re-fetch loop (#6675) | ||
| if (typeof quoteTimestampDiff === 'number' && quoteTimestampDiff < QUOTE_SLIPPAGE_CHANGE_THROTTLE_INTERVAL) { |
There was a problem hiding this comment.
Another suggestion: instead of throttling by time, we can compare new/prev values and if the difference is less than N% - skip following quote fetch
(ofc only for "smart" slippage mode)
But... sometimes suggestedSlippageBps may jump from 0.5% to 1.25% to 4.75% just one after another 🫠
crutch12
force-pushed
the
fix/swap-bridge-quote-loaded-twice
branch
from
0558792 to
9024f60
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (2)
112-112: Optional:isSmartSlippageApplieddependency is not strictly necessary.Since
useSyncedRefreturns a stable frozen object, includingisSmartSlippageAppliedin the dependency array doesn't cause re-runs and has no functional impact. The effect correctly accesses.currentto read the latest value.You may optionally remove it from the dependency array, though keeping it could aid readability for future maintainers.
63-63: Throttling logic correctly breaks the re-fetch loop.The implementation effectively prevents the
quote↔suggestedSlippageBpscycle by:
- Detecting slippage-only changes via
checkOnlySlippageBpsChanged- Throttling rapid re-fetches within 1.5s of the last quote
- Only applying throttling in smart slippage mode
- Allowing non-slippage changes to bypass throttling
The
useSyncedRefpattern provides stable access to the current smart slippage state without causing effect re-runs.The two-hook chain (lines 68–78 resets counter; lines 117–121 triggers pollQuote) creates the intended effect sequence for immediate quote updates. Consider adding an explicit test case for the throttling behavior to document the expected 1.5s window for slippage-only changes, as current tests cover only the basic quote-fetching flow.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.tslibs/common-hooks/src/index.tslibs/common-hooks/src/useSyncedRef.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/common-hooks/src/index.ts
- libs/common-hooks/src/useSyncedRef.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
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.
📚 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/tradeQuote/hooks/useTradeQuotePolling.ts
📚 Learning: 2025-06-16T15:58:57.402Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5830
File: apps/cowswap-frontend/src/modules/tradeQuote/hooks/useSmartSlippageFromQuote.ts:5-9
Timestamp: 2025-06-16T15:58:57.402Z
Learning: In the cowswap system, 0 bps will never be suggested as a slippage value, so falsy checks on suggested slippage values are safe and won't drop valid 0 values.
Applied to files:
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts
📚 Learning: 2025-08-08T13:56:18.009Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6125
File: libs/tokens/src/updaters/TokensListsUpdater/index.tsx:29-31
Timestamp: 2025-08-08T13:56:18.009Z
Learning: In libs/tokens/src/updaters/TokensListsUpdater/index.tsx, the project’s current Jotai version requires using `unstable_getOnInit` (not `getOnInit`) in atomWithStorage options; keep `{ unstable_getOnInit: true }` until Jotai is upgraded.
Applied to files:
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.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/tradeQuote/hooks/useTradeQuotePolling.ts
🧬 Code graph analysis (1)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (3)
libs/common-hooks/src/useSyncedRef.ts (1)
useSyncedRef(9-23)apps/cowswap-frontend/src/modules/tradeSlippage/hooks/useIsSmartSlippageApplied.ts (1)
useIsSmartSlippageApplied(3-5)apps/cowswap-frontend/src/modules/tradeQuote/utils/quoteParamsChanges.ts (1)
checkOnlySlippageBpsChanged(7-21)
⏰ 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 (1)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (1)
58-61: Correct pattern for tracking previous values.The
prevQuoteParamsRefcorrectly captures the previousquoteParamsvalue for comparison in the throttling logic.
|
Hey @crutch12 , the build is crashing. |
|
@elena-zh sorry, didn't mention fixed build errors This PR is still in progress, there is a problem with slippage manual/auto changing (updated description - TODO) |
| */ | ||
| if (isConfirmOpen) return | ||
|
|
||
| if (isSmartSlippageApplied.current) { |
There was a problem hiding this comment.
Maybe it should be checked inside quoteUsingSameParameters
|
I've fixed the main found functionality problems. Other: UPD |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts (1)
42-42: Consider refactoring to reduce complexity.The addition of
eslint-disable-next-line complexityindicates the function has grown complex. Given the PR description mentions this is a "workaround" approach and suggests an alternative of tracking intent intradeQuotesAtom, consider refactoring this function into smaller, focused helpers (e.g., separate functions for race condition checks, timestamp updates, and cache preservation).
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteManager.tsapps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 Learning: 2025-09-25T08:46:43.815Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/modules/tradeQuote/services/fetchAndProcessQuote.test.ts:376-385
Timestamp: 2025-09-25T08:46:43.815Z
Learning: In fetchAndProcessQuote.ts, when bridgingSdk.getBestQuote() returns null, no loading state reset is needed because loading state management is handled through onQuoteResult() callback for results and processQuoteError() for errors. The null case is intentionally designed not to trigger any manager methods.
Applied to files:
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteManager.tsapps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts
📚 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/tradeQuote/hooks/useTradeQuoteManager.tsapps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.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/tradeQuote/hooks/useTradeQuoteManager.tsapps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts
🔇 Additional comments (4)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteManager.ts (1)
76-82: LGTM: Correct conditional caching of suggestedSlippageBps for optimal quotes.The logic correctly sets
suggestedSlippageBpsonly whenpriceQualityis OPTIMAL, which aligns with the comment on line 79 explaining that the SDK returns a default value for FAST quotes. This prevents stale or default slippage values from overwriting the cached suggested slippage.apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts (3)
21-22: LGTM: Clear field addition for caching suggested slippage.The new
suggestedSlippageBpsfield is well-named, appropriately typed (number | null), and properly initialized in the default state. The comment clearly explains its purpose.Also applies to: 34-34
48-57: LGTM: Correct race condition handling for FAST vs OPTIMAL quotes.The logic correctly prevents FAST quotes from overwriting OPTIMAL quotes when they belong to the same request (matched by
fetchStartTimestamp). The use of optional chaining safely handles cases whereprevQuote.fetchParamsmight be null, and the early return prevents unnecessary state updates.
64-67: LGTM: Correct caching pattern for suggestedSlippageBps.The conditional logic correctly preserves the cached
suggestedSlippageBpswhennextState.suggestedSlippageBpsisundefined, and updates it (including tonull) when explicitly provided. This pattern enables the intended behavior: OPTIMAL quotes set the value, and FAST quotes preserve it.
| ...nextState, | ||
| quote: typeof nextState.quote === 'undefined' ? prevQuote.quote : nextState.quote, | ||
| localQuoteTimestamp: nextState.quote ? Math.ceil(Date.now() / 1000) : null, | ||
| localQuoteTimestamp: nextState.quote ? Date.now() : null, |
There was a problem hiding this comment.
Fix timestamp preservation when quote is preserved.
When nextState.quote is undefined, line 62 correctly preserves prevQuote.quote, but line 63 sets localQuoteTimestamp to null because nextState.quote is falsy. This breaks the invariant that a quote and its timestamp should stay synchronized.
Example scenario:
- Caller passes
{ isLoading: true }(no quote field) - Expected: preserve existing quote and its timestamp
- Actual: preserve quote but reset timestamp to null
🔎 Proposed fix
- localQuoteTimestamp: nextState.quote ? Date.now() : null,
+ localQuoteTimestamp: typeof nextState.quote === 'undefined'
+ ? prevQuote.localQuoteTimestamp
+ : (nextState.quote ? Date.now() : null),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| localQuoteTimestamp: nextState.quote ? Date.now() : null, | |
| localQuoteTimestamp: typeof nextState.quote === 'undefined' | |
| ? prevQuote.localQuoteTimestamp | |
| : (nextState.quote ? Date.now() : null), |
🤖 Prompt for AI Agents
In apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteAtom.ts around
line 63, localQuoteTimestamp is being set based on the truthiness of
nextState.quote which causes the timestamp to be nulled when nextState.quote is
undefined even if prevQuote.quote is preserved; change the logic to set
localQuoteTimestamp to Date.now() only when nextState.quote is explicitly
provided (e.g., nextState.quote !== undefined), otherwise preserve
prevQuote.localQuoteTimestamp so the quote and its timestamp remain
synchronized.
2 participants
Summary
quote->suggestedSlippageBps->quote->suggestedSlippageBps... (Quote is loaded twice or more times #6762)Since
quoteandsuggestedSlippageBpsupdate each other, we have to break the re-fetch loop that happens ifsuggestedSlippageBpschanges on the nextquoteresponse.The easiest workaround: throttle (1.5s)
quotecalls in case when onlysuggestedSlippageBpswas changed.TODO
yarn testfailsquoterequestTo Test
getSlippageTolerancereturn value as comment says. it will allow to 100% catch re-fetch loop problem/quoterequests (fastandoptimal)Background
I don't really like this throttle solution, maybe it's possible to keep needed state in
tradeQuotesAtomto distinguish intended and unintendedquoteParamsupdates 🤷Summary by CodeRabbit
Bug Fixes
Improvements
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.