feat(trade): dis 8000 play quote refetch indicator #5859
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
|
""" WalkthroughThe trade quote polling logic in the CowSwap frontend was refactored to delegate polling and update responsibilities to a new Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as useTradeQuotePolling
participant PollHook as usePollQuoteCallback
participant Service as doQuotePolling
participant Fetch as fetchAndProcessQuote
Hook->>PollHook: get pollQuote callback
PollHook->>Service: doQuotePolling(context)
alt Needs to fetch quote
Service->>Fetch: fetchAndProcessQuote(params)
Fetch-->>Service: (updates state)
end
Service-->>PollHook: (no return value)
PollHook-->>Hook: pollQuote result (boolean)
sequenceDiagram
participant TradeWidget as TradeWidgetForm
participant QuoteProgress as QuotePolingProgress
participant Settings as SettingsWidget
TradeWidget->>QuoteProgress: render if !isPriceStatic
TradeWidget->>Settings: render if !lockScreen
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
743a548 to
614ba7a
Compare
There was a problem hiding this comment.
Hey @shoom3301 , works as described.
However, I have some concerns:
- on the 'Confirm' modal we show that a quote is updated once per 30 seconds. IIMO, it is a reasonable time for a quote update instead of 1 minute
- Quote for regular (not eth-flow) orders started to be updated in 1 minute. However, now the time is 30 seconds, so I'd leave this time as it is.
WDYT?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/cowswap-frontend/src/modules/trade/containers/QuotePolingProgress/index.tsx (2)
10-10: Fix component name typo.The component name has a typo: "QuotePolingProgress" should be "QuotePollingProgress" (missing 'l' in "Polling").
-export function QuotePolingProgress(): ReactNode { +export function QuotePollingProgress(): ReactNode {
12-12: Consider UX implications of progress direction.The current calculation makes the progress bar start full and empty as time passes, which might be counterintuitive to users who typically expect progress bars to fill up over time.
Consider if this countdown-style progress bar aligns with user expectations, or if it should be inverted to show elapsed time instead:
- const percent = Math.ceil((counter * 100) / QUOTE_POLLING_INTERVAL) + const percent = Math.ceil(((QUOTE_POLLING_INTERVAL - counter) * 100) / QUOTE_POLLING_INTERVAL)This would make the progress bar start empty and fill up as time elapses, which might be more intuitive.
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (1)
17-17: Consider using the existing ms macro for consistency.The hardcoded
ONE_SEC = 1000constant works, but consider usingms\1s`` for consistency with other timing constants in the codebase.-const ONE_SEC = 1000 +const ONE_SEC = ms`1s`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/cowswap-frontend/src/modules/limitOrders/containers/LimitOrdersConfirmModal/index.tsx(2 hunks)apps/cowswap-frontend/src/modules/limitOrders/containers/LimitOrdersWidget/index.tsx(2 hunks)apps/cowswap-frontend/src/modules/swap/containers/SwapConfirmModal/index.tsx(1 hunks)apps/cowswap-frontend/src/modules/trade/containers/QuotePolingProgress/index.tsx(1 hunks)apps/cowswap-frontend/src/modules/trade/containers/TradeConfirmModal/index.cosmos.tsx(0 hunks)apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/TradeWidgetForm.tsx(3 hunks)apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/styled.tsx(1 hunks)apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/types.ts(1 hunks)apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/CountDown.tsx(1 hunks)apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.cosmos.tsx(0 hunks)apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.tsx(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/consts.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/usePollQuoteCallback.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteCounter.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/index.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteCounterAtom.ts(1 hunks)apps/cowswap-frontend/src/modules/twap/containers/TwapConfirmModal/index.tsx(0 hunks)libs/ui/src/index.ts(1 hunks)libs/ui/src/pure/CircleProgress/index.tsx(1 hunks)
💤 Files with no reviewable changes (3)
- apps/cowswap-frontend/src/modules/trade/containers/TradeConfirmModal/index.cosmos.tsx
- apps/cowswap-frontend/src/modules/twap/containers/TwapConfirmModal/index.tsx
- apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.cosmos.tsx
✅ Files skipped from review due to trivial changes (8)
- apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/types.ts
- libs/ui/src/index.ts
- apps/cowswap-frontend/src/modules/limitOrders/containers/LimitOrdersConfirmModal/index.tsx
- apps/cowswap-frontend/src/modules/swap/containers/SwapConfirmModal/index.tsx
- apps/cowswap-frontend/src/modules/tradeQuote/index.ts
- apps/cowswap-frontend/src/modules/tradeQuote/consts.ts
- apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/styled.tsx
- apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteCounterAtom.ts
🧰 Additional context used
🧬 Code Graph Analysis (7)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteCounter.ts (2)
apps/cowswap-frontend/src/modules/tradeQuote/index.ts (1)
useTradeQuoteCounter(9-9)apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteCounterAtom.ts (1)
tradeQuoteCounterAtom(5-5)
libs/ui/src/pure/CircleProgress/index.tsx (2)
libs/ui/src/pure/Loader/index.tsx (1)
Loader(32-43)libs/common-hooks/src/useTheme.ts (1)
styled(26-26)
apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.tsx (1)
apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/CountDown.tsx (1)
QuoteCountdown(11-31)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/usePollQuoteCallback.ts (9)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useQuoteParams.ts (1)
QuoteParams(24-28)apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteInputAtom.ts (1)
tradeQuoteInputAtom(13-13)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuote.ts (1)
useTradeQuote(14-29)libs/wallet/src/api/hooks.ts (1)
useWalletInfo(16-18)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteManager.ts (1)
useTradeQuoteManager(28-80)libs/common-utils/src/getCurrencyAddress.ts (1)
getCurrencyAddress(6-8)apps/cowswap-frontend/src/modules/tradeQuote/types.ts (1)
TradeQuoteFetchParams(3-7)apps/cowswap-frontend/src/modules/tradeQuote/services/fetchAndProcessQuote.ts (1)
fetchAndProcessQuote(27-80)apps/cowswap-frontend/src/modules/tradeQuote/services/doQuotePolling.ts (2)
QuoteUpdateContext(15-25)doQuotePolling(27-63)
apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/CountDown.tsx (2)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteCounter.ts (1)
useTradeQuoteCounter(6-8)apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/styled.tsx (1)
QuoteCountdownWrapper(87-106)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (9)
apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteInputAtom.ts (1)
tradeQuoteInputAtom(13-13)apps/cowswap-frontend/src/modules/tradeQuote/state/tradeQuoteCounterAtom.ts (1)
tradeQuoteCounterAtom(5-5)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteCounter.ts (1)
useResetQuoteCounter(10-12)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuote.ts (1)
useTradeQuote(14-29)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useQuoteParams.ts (1)
useQuoteParams(30-110)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteManager.ts (1)
useTradeQuoteManager(28-80)libs/common-utils/src/getCurrencyAddress.ts (1)
getCurrencyAddress(6-8)apps/cowswap-frontend/src/modules/tradeQuote/hooks/usePollQuoteCallback.ts (1)
usePollQuoteCallback(18-82)apps/cowswap-frontend/src/modules/tradeQuote/consts.ts (1)
QUOTE_POLLING_INTERVAL(3-3)
apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/TradeWidgetForm.tsx (1)
apps/cowswap-frontend/src/modules/trade/containers/QuotePolingProgress/index.tsx (1)
QuotePolingProgress(10-27)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup
- GitHub Check: Cypress
🔇 Additional comments (22)
apps/cowswap-frontend/src/modules/limitOrders/containers/LimitOrdersWidget/index.tsx (2)
64-64: LGTM: ESLint rule adjustment is appropriate.Removing the
max-lines-per-functionESLint rule is reasonable given the existing TODO comment about breaking down the large function.
241-241: LGTM: Static price configuration is correct for limit orders.Setting
isPriceStatic: trueis appropriate for limit orders since they should have static prices and don't require dynamic quote polling UI elements.apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuoteCounter.ts (1)
1-12: LGTM: Clean and well-structured hook implementation.The hooks properly utilize Jotai atoms with good separation of concerns - one for reading the counter value and one for resetting it.
apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/TradeWidgetForm.tsx (3)
42-42: LGTM: Import added correctly.The import for the new
QuotePolingProgresscomponent is properly added.
88-88: LGTM: Parameter addition is well-implemented.The
isPriceStaticparameter is properly added with an appropriate default value offalse.
200-203: LGTM: Conditional rendering logic is correct.The
HeaderRightwrapper appropriately groups the quote polling progress and settings widget, with correct conditional rendering based onisPriceStaticandlockScreenstates.libs/ui/src/pure/CircleProgress/index.tsx (1)
1-49: LGTM: Solid circular progress component implementation.The SVG-based progress component is well-implemented with:
- Proper circle positioning and sizing calculations
- Correct stroke-dasharray/stroke-dashoffset usage for progress visualization
- Appropriate rotation to start progress from the top
- Clean TypeScript interface with all necessary styling props
apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.tsx (2)
37-37: Good addition of isPriceStatic prop for conditional UI control.The new
isPriceStaticprop provides appropriate control over when quote-related UI elements should be displayed, aligning with the centralized polling state management approach.
138-138: Excellent simplification of QuoteCountdown rendering logic.The removal of the
refreshIntervalprop and addition of theisPriceStaticcondition simplifies the component while maintaining proper conditional rendering. The QuoteCountdown component now uses the centralizeduseTradeQuoteCounterhook for timing.apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/CountDown.tsx (3)
1-1: Good import cleanup and addition of centralized counter hook.The updated imports reflect the refactoring from internal state management to using the centralized
useTradeQuoteCounterhook.Also applies to: 5-5
11-11: Proper return type annotation added.Adding explicit
ReactNodereturn type improves type safety.
14-14: Excellent refactoring to use centralized counter state.The component is significantly simplified by:
- Using
useTradeQuoteCounter()instead of internal interval management- Simplifying the blink trigger logic to check
counter === 0- Using
setTimeoutfor the blink effect instead of complex state management- Directly displaying
counter / ONE_SECfor the countdownThis makes the component more predictable and reduces complexity while maintaining the same user experience.
Also applies to: 17-17, 20-20, 24-24, 28-28
apps/cowswap-frontend/src/modules/tradeQuote/hooks/usePollQuoteCallback.ts (5)
18-21: Well-designed hook interface.The function signature clearly indicates the two key parameters needed for polling: confirmation modal state and quote parameters. The return type as a callback function provides a clean interface for triggering polling.
22-36: Proper dependency setup and ref usage.Good use of refs to avoid stale closures, especially for
tradeQuoteRefandisOnlineRef. The dependency gathering is comprehensive and well-organized.
38-47: Robust validation logic with appropriate early returns.The validation checks handle all the necessary edge cases:
- Missing trade quote manager or parameters
- Unsupported tokens
- Zero amount case with proper cleanup
The early returns prevent unnecessary processing and the
tradeQuoteManager.reset()call for zero amounts is appropriate.
49-70: Clean delegation to the doQuotePolling service.The context object creation is comprehensive and the delegation to
doQuotePollingprovides good separation of concerns. The inlinefetchQuotefunction properly captures the closure variables.
71-81: Comprehensive dependency array ensures proper memoization.The dependency array includes all the relevant values that could affect the callback behavior, ensuring proper re-memoization when needed.
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (5)
1-1: Good import updates for the refactored architecture.The updated imports reflect the new centralized state management approach using atoms and the extracted polling callback hook.
Also applies to: 7-7, 10-10, 13-16
20-22: Excellent refactoring to use centralized state and extracted callback.The hook is significantly simplified by:
- Using
tradeQuoteCounterAtomfor centralized counter state- Delegating complex polling logic to
usePollQuoteCallback- Removing internal complexity while maintaining the same interface
This improves maintainability and testability.
Also applies to: 31-31, 38-38
58-61: Clean parameter change handling.The logic properly triggers immediate polling when parameters change and resets the counter, which ensures users get fresh quotes quickly when they modify trade parameters.
66-70: Simplified polling trigger when counter reaches zero.The refactored logic cleanly triggers polling with
forceUpdate: truewhen the counter reaches zero, maintaining the periodic refresh behavior.
75-93: Verify counter timing precision and confirm modal behavior.The counter tick logic looks correct, but consider these aspects:
- The
setIntervalwith 1-second precision should align with the expected polling behavior- Resetting the counter when
isConfirmOpenchanges ensures proper state management- The counter reset to
QUOTE_POLLING_INTERVALwhen it goes below 0 maintains the cycleThe logic appears sound for the 30-second polling interval mentioned in the PR objectives.
Let me verify that the polling interval aligns with the 1-minute quote expiration cap mentioned in the PR objectives:
#!/bin/bash # Description: Verify the QUOTE_POLLING_INTERVAL constant value # Expected: Should be 30 seconds or less to support 1-minute expiration cap # Search for QUOTE_POLLING_INTERVAL definition rg -A 2 "QUOTE_POLLING_INTERVAL.*=" --type ts
… fix/quote-caching
|
@elena-zh thanks! |
|
Hey @shoom3301 , great!
Confusing, because it should be synced, IMO.
So, there are 2 options:
Also, there is a quote update on limit orders page. No need to fix it, as it is OK, but just letting you know :) |
|
@elena-zh thank you!
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (2)
67-71: Clarify the purpose of theresetPollingparameter.The second parameter
truepassed topollQuote(false, true)suggests it's a polling reset flag, but this isn't immediately clear from the context. Consider adding a comment or using named parameters for better readability.- pollQuote(false, true) + pollQuote(false, true) // Reset polling state after interval
17-17: Consider using a more descriptive constant name.While
ONE_SEC = 1000is correct, consider a more descriptive name that indicates its purpose in the polling context.-const ONE_SEC = 1000 +const POLLING_TICK_INTERVAL = 1000Then update the usage:
- }, ONE_SEC) + }, POLLING_TICK_INTERVAL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/TradeWidgetForm.tsx(3 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/usePollQuoteCallback.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts(1 hunks)apps/cowswap-frontend/src/modules/tradeQuote/services/doQuotePolling.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/TradeWidgetForm.tsx
- apps/cowswap-frontend/src/modules/tradeQuote/hooks/usePollQuoteCallback.ts
- apps/cowswap-frontend/src/modules/tradeQuote/services/doQuotePolling.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cypress
- GitHub Check: Setup
🔇 Additional comments (2)
apps/cowswap-frontend/src/modules/tradeQuote/hooks/useTradeQuotePolling.ts (2)
19-22: LGTM: Clean state management with Jotai atoms.The refactor to use Jotai atoms for state management improves the architecture by centralizing the polling counter state.
58-62: ```shell
#!/bin/bashLocate and inspect the implementation of usePollQuoteCallback
FILE=$(fd usePollQuoteCallback.ts)
echo "Inspecting $FILE"
sed -n '1,200p' "$FILE"</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary
As we recently discovered,expirationin quote response might be too far in the future, so we have to cap it.I've capped it with
1 minute, from my perspective this is a valid limit.There are two commits:1. Just a refactoring in order to simplifyuseTradeQuotePolling, no logic changes: 52c3e712. Actual fix.isQuoteExpirednow caps theexpirationTimewith1 minute: 743a548After Elenas comment, I decided to simplify logic and make it clear.
Now quote refereshes every 30 seconds, that's all.
Along with that logic I added an indicator of the quote interval counter. If you think it's excessive - please let me know, I can delete it.
quote-counter-demo.mov
To Test
Summary by CodeRabbit
Summary by CodeRabbit