feat: add offset & limit params to getTrades #6567
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
All contributors have signed the CLA ✍️ ✅ |
kernelwhisperer
force-pushed
the
feat/trades-pagination
branch
from
969cb27 to
117d3bf
Compare
kernelwhisperer
force-pushed
the
feat/trades-pagination
branch
from
7b38e00 to
2709351
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/explorer/src/components/orders/OrderDetails/OrderDetails.fixture.tsx (1)
393-405: Fixture pagination props are wired correctlyCentralizing pagination props in
mockTablePropsand spreading them into allOrderDetailsfixtures is a clean way to adapt to the new API. The no‑op handlers andtableStateshape align with the component’s expectations;totalResults: 0is a reasonable placeholder until/if the backend exposes real totals.Also applies to: 411-577, 581-727
apps/explorer/src/hooks/useOperatorTrades.ts (1)
11-16: Pagination logic in useOrderTrades is correct and matches the “limit+1” patternThe hook changes look coherent:
- Accepting
(order, offset, limit)and passing{ offset, limit: limit + 1 }togetTradesis the right way to detect a next page.- Computing
hasNextfromtrades.length > limitand then slicing tolimitgives consumers exactly one page plus a reliablehasNextPageflag.- Including
offsetandlimitin thefetchTradescallback deps ensures refetch on page/size changes.If you ever want to simplify state,
hasNextPagecould be derived fromrawTradesandlimitinside the effect instead of being stored separately, but what you have is already fine.Also applies to: 51-57, 69-84, 118-124, 144-147
apps/explorer/src/explorer/components/OrderWidget/index.tsx (1)
9-12: OrderWidget pagination wiring is coherent; consider avoiding direct state mutationUsing
useTablefor{ pageOffset, pageSize }and feeding those intouseOrderTrades(withRESULTS_PER_PAGE = 10) gives you a clear pagination flow, and passingtableStateand handlers down toOrderDetailsvia props is a good separation of concerns.The only smell is:
// eslint-disable-next-line react-hooks/immutability tableState['hasNextPage'] = hasNextPageMutating the
stateobject coming from a hook is generally discouraged. It works here because other state changes trigger re‑renders and consumers only readhasNextPage, but longer‑term it would be cleaner to haveuseTableexpose a setter (e.g.setHasNextPage) or accepthasNextPageas part of its own state update function.Also applies to: 17-24, 31-37, 38-40, 51-62
📜 Review details
Configuration used: CodeRabbit 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/explorer/cosmos.config.json(1 hunks)apps/explorer/src/api/operator/operatorApi.ts(2 hunks)apps/explorer/src/api/operator/types.ts(1 hunks)apps/explorer/src/components/orders/OrderDetails/OrderDetails.fixture.tsx(26 hunks)apps/explorer/src/components/orders/OrderDetails/index.tsx(3 hunks)apps/explorer/src/explorer/components/OrderWidget/index.tsx(2 hunks)apps/explorer/src/hooks/useOperatorTrades.ts(6 hunks)package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6285
File: apps/cowswap-frontend/src/modules/swap/updaters/index.tsx:37-39
Timestamp: 2025-09-24T14:51:43.087Z
Learning: SwapUpdaters component doesn't have pagination functionality - it processes a fixed number (10) of the most recent pending orders, not paginated results.
📚 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/explorer/src/explorer/components/OrderWidget/index.tsxapps/explorer/src/components/orders/OrderDetails/index.tsx
📚 Learning: 2025-09-24T14:51:43.087Z
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6285
File: apps/cowswap-frontend/src/modules/swap/updaters/index.tsx:37-39
Timestamp: 2025-09-24T14:51:43.087Z
Learning: SwapUpdaters component doesn't have pagination functionality - it processes a fixed number (10) of the most recent pending orders, not paginated results.
Applied to files:
apps/explorer/src/explorer/components/OrderWidget/index.tsx
📚 Learning: 2025-09-25T08:48:53.495Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6299
File: apps/cowswap-frontend/src/entities/bridgeProvider/useBridgeSupportedNetworks.test.tsx:58-60
Timestamp: 2025-09-25T08:48:53.495Z
Learning: In the cowswap-frontend codebase, when writing SWR tests, the team prefers maximum test isolation by using `provider: () => new Map()` in SWRConfig wrappers, even if it recreates cache on every render, to ensure tests don't share any state.
Applied to files:
apps/explorer/src/components/orders/OrderDetails/OrderDetails.fixture.tsx
📚 Learning: 2025-09-25T08:49:32.256Z
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.
Applied to files:
apps/explorer/src/components/orders/OrderDetails/OrderDetails.fixture.tsx
🧬 Code graph analysis (3)
apps/explorer/src/hooks/useOperatorTrades.ts (3)
apps/explorer/src/types/index.ts (1)
UiError(17-20)apps/explorer/src/api/operator/types.ts (2)
Trade(86-100)RawTrade(81-81)apps/explorer/src/api/operator/operatorApi.ts (1)
getTrades(56-79)
apps/explorer/src/explorer/components/OrderWidget/index.tsx (2)
apps/explorer/src/hooks/useOperatorOrder.ts (1)
useOrderAndErc20s(119-150)apps/explorer/src/hooks/useOperatorTrades.ts (1)
useOrderTrades(51-148)
apps/explorer/src/components/orders/OrderDetails/index.tsx (1)
apps/explorer/src/state/network/hooks.ts (1)
useNetworkId(6-10)
🔇 Additional comments (5)
package.json (1)
81-81: cow-sdk bump to 7.1.6 looks aligned with new getTrades usageThe version bump is consistent with introducing paginated
getTradescalls; no issues spotted here. Please just confirm this SDK version indeed supports{ offset, limit }ongetTradesand that no other breaking changes affect explorer.apps/explorer/cosmos.config.json (1)
25-27: Updated Vite config path to .mtsPointing Cosmos at
vite.config.mtsmakes sense with an ESM Vite config. Just verify that this file exists inapps/explorerand thatnx run explorer:cosmos:runstill works as expected.apps/explorer/src/api/operator/types.ts (1)
125-130: GetTradesParams pagination fields are consistent with usageAdding optional
offsetandlimithere matches howgetTradesanduseOrderTradesnow work; the shape looks correct and backward‑compatible for existing callers that don’t pass these fields.apps/explorer/src/api/operator/operatorApi.ts (1)
3-3: getTrades now correctly exposes paginated params to the SDKSwitching to
GetTradesParamsand forwarding{ owner, orderUid, offset, limit }toorderBookSDK.getTrades(for both PROD and BARN) aligns the API layer with the new hook and types. The logging changes are also helpful for debugging pagination.Please double‑check against the cow‑sdk typings/docs that
offsetandlimithave the same semantics you expect (especially when combining PROD + staging results) and that leaving themundefinedfor legacy callers remains valid.Also applies to: 56-79
apps/explorer/src/components/orders/OrderDetails/index.tsx (1)
14-15: OrderDetails now cleanly consumes external table pagination stateImporting
TableState, extendingPropswithtableStateand pagination callbacks, and passing them throughFillsTableContextis a solid refactor: pagination logic lives in the container (OrderWidget) while OrderDetails stays focused on rendering and tab behavior. The tab selection and redirect logic remain unchanged, so this should be a non‑breaking UI change aside from enabling pagination.Also applies to: 35-46, 140-156, 216-225
|
The backend is live in prod on all networks. Before merging we need a new cow-sdk deployment. Waiting on cowprotocol/cow-sdk#752 |
|
@kernelwhisperer , tested on this order on Mainnet: also looks good As for other chains, not sure how to find orders with so many fills.. |
kernelwhisperer
force-pushed
the
feat/trades-pagination
branch
from
5de7c29 to
3a7d1bf
Compare
|
@kernelwhisperer the SDK changes were released to NPM |
kernelwhisperer
force-pushed
the
feat/trades-pagination
branch
from
dc34f4b to
3a7d1bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/explorer/src/hooks/useOperatorTrades.ts (1)
118-123: Minor inefficiency: timestamps fetched for the extra record.The
fetchTradesTimestampscall on line 90 processes allrawTradesincluding thelimit+1record that gets discarded. This results in one unnecessarygetBlockcall per page when more results exist. Consider slicing before fetching timestamps if this becomes a performance concern.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/explorer/cosmos.config.jsonapps/explorer/src/api/operator/operatorApi.tsapps/explorer/src/api/operator/types.tsapps/explorer/src/components/orders/OrderDetails/FillsTable.tsxapps/explorer/src/components/orders/OrderDetails/OrderDetails.fixture.tsxapps/explorer/src/components/orders/OrderDetails/index.tsxapps/explorer/src/explorer/components/OrderWidget/index.tsxapps/explorer/src/hooks/useOperatorTrades.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/explorer/src/api/operator/types.ts
- apps/explorer/src/components/orders/OrderDetails/OrderDetails.fixture.tsx
- apps/explorer/src/components/orders/OrderDetails/FillsTable.tsx
- apps/explorer/src/api/operator/operatorApi.ts
- apps/explorer/cosmos.config.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6285
File: apps/cowswap-frontend/src/modules/swap/updaters/index.tsx:37-39
Timestamp: 2025-09-24T14:51:43.087Z
Learning: SwapUpdaters component doesn't have pagination functionality - it processes a fixed number (10) of the most recent pending orders, not paginated results.
📚 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/explorer/src/explorer/components/OrderWidget/index.tsxapps/explorer/src/components/orders/OrderDetails/index.tsx
📚 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/explorer/src/components/orders/OrderDetails/index.tsx
📚 Learning: 2025-11-19T10:18:23.717Z
Learnt from: limitofzero
Repo: cowprotocol/cowswap PR: 6537
File: apps/cowswap-frontend/src/modules/trade/pure/PartnerFeeRow/index.tsx:33-35
Timestamp: 2025-11-19T10:18:23.717Z
Learning: In apps/cowswap-frontend/src/modules/trade/pure/PartnerFeeRow/index.tsx, when there is no partner fee (amount is null/undefined, bps is missing, or amount equals 0), FreeFeeRow is rendered with withTimelineDot={false} hardcoded. This is intentional design—free fee rows should not show the timeline dot regardless of what the parent component passes, as they have a distinct visual treatment from actual fee rows.
Applied to files:
apps/explorer/src/components/orders/OrderDetails/index.tsx
🧬 Code graph analysis (3)
apps/explorer/src/hooks/useOperatorTrades.ts (3)
apps/explorer/src/types/index.ts (1)
UiError(17-20)apps/explorer/src/api/operator/types.ts (2)
Trade(86-100)RawTrade(81-81)apps/explorer/src/api/operator/operatorApi.ts (1)
getTrades(56-79)
apps/explorer/src/explorer/components/OrderWidget/index.tsx (3)
apps/explorer/src/explorer/components/OrdersTableWidget/useTable.tsx (1)
useTable(32-71)apps/explorer/src/hooks/useOperatorOrder.ts (1)
useOrderAndErc20s(119-150)apps/explorer/src/hooks/useOperatorTrades.ts (1)
useOrderTrades(51-148)
apps/explorer/src/components/orders/OrderDetails/index.tsx (2)
apps/explorer/src/explorer/components/OrdersTableWidget/useTable.tsx (1)
TableState(5-10)apps/explorer/src/components/orders/OrdersUserDetailsTable/index.tsx (1)
Props(57-62)
⏰ 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 (10)
apps/explorer/src/hooks/useOperatorTrades.ts (3)
51-56: LGTM - Pagination parameters and state properly added.The hook signature correctly defaults
offsetto 0 andlimitto 10, and the newhasNextPagestate is appropriately initialized.
68-84: LGTM - "Limit plus one" pattern correctly implemented.The
limit + 1fetch strategy is a standard, efficient approach for detecting additional pages without requiring a separate count query. The callback dependencies are correctly updated.
144-147: LGTM - Return value properly memoized with new dependency.The
hasNextPageis correctly included in the memoization dependencies and returned result.apps/explorer/src/explorer/components/OrderWidget/index.tsx (2)
17-36: LGTM - Pagination state setup and hook integration.The table state initialization with
pageOffset: 0andpageSize: RESULTS_PER_PAGEis correct, and the hook is properly invoked with the pagination parameters.
9-9: No action needed. The import path is correct. OrderWidget requiressetPageOffset, which is only exported byTokensTableWidget/useTable. TheOrdersTableWidget/useTablevariant does not export this function, making the current import the only viable option.apps/explorer/src/components/orders/OrderDetails/index.tsx (5)
41-45: LGTM - Props interface properly extended for external pagination control.The new props correctly expose the pagination state and handlers needed for external state management.
75-81: LGTM - hasMultipleTrades properly integrated.The
hasMultipleTradesparameter is correctly used to determine bothshowFillslogic andgetOrderWithTxHashbehavior.
138-143: LGTM - getOrderWithTxHash logic correctly updated.The condition
trades.length === 1 && !hasMultipleTradescorrectly ensurestxHashis only attached when there's truly a single trade across all pages, preventing the edge case where pagination would incorrectly assign the first trade's hash.
246-246: LGTM - Edge case for hasMultipleTrades correctly addressed.The condition now includes
!!tableState.hasNextPage, which handles the scenario where there's only one trade visible but more exist on subsequent pages. This addresses the concern from the previous review.
223-232: LGTM - Context provider correctly wired with external state.The
FillsTableContext.Providerproperly passes all pagination controls from the parent, enabling child components to access consistent pagination state.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
kernelwhisperer
force-pushed
the
feat/trades-pagination
branch
from
3a7d1bf to
1b4f953
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/explorer/src/api/operator/operatorApi.ts (1)
62-78: Critical: PROD/BARN merge breaks "limit plus one" pagination strategy.The current implementation passes the same
offsetandlimitto both PROD and BARN environments, then merges the results. This breaks the "limit plus one" strategy used inuseOrderTrades:The problem:
useOrderTradesrequestslimit + 1trades (e.g., 11 when limit=10)- This function requests 11 from PROD → returns up to 11 trades
- This function requests 11 from BARN → returns up to 11 trades
- Merged result: up to 22 trades
- Hook checks
trades.length (22) > limit (10)→ true- Hook sets
hasNextPage = trueand slices to 10 trades- Result: 12 trades are lost, and
hasNextPagemay be incorrectly trueImpact:
- Trades from one environment could be systematically hidden
- Page navigation will skip records
hasNextPagelogic is unreliable when both environments return dataPotential solutions:
- Apply pagination post-merge (fetch all from both, merge, then paginate)
- Fetch limit+1 from PROD, check length; if < limit+1, fetch remaining from BARN
- Choose primary environment for pagination and treat BARN as supplementary
- Have backend handle the merge with pagination
🧹 Nitpick comments (1)
apps/explorer/src/components/orders/OrderDetails/FillsTable.tsx (1)
19-19: Consider removing unusedtableStateprop.The
tableStateprop is defined in the type but never destructured or used in the component. Since pagination is now managed externally, this prop appears to be vestigial.🔎 Proposed cleanup
type FillsTableProps = SimpleTableProps & { trades: Trade[] | undefined order: Order | null - tableState: TableState isPriceInverted: boolean invertPrice: Command }Also applies to: 25-25
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/explorer/cosmos.config.jsonapps/explorer/src/api/operator/operatorApi.tsapps/explorer/src/api/operator/types.tsapps/explorer/src/components/orders/OrderDetails/FillsTable.tsxapps/explorer/src/components/orders/OrderDetails/OrderDetails.fixture.tsxapps/explorer/src/components/orders/OrderDetails/index.tsxapps/explorer/src/explorer/components/OrderWidget/index.tsxapps/explorer/src/hooks/useOperatorTrades.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/explorer/src/explorer/components/OrderWidget/index.tsx
- apps/explorer/src/api/operator/types.ts
- apps/explorer/src/components/orders/OrderDetails/OrderDetails.fixture.tsx
- apps/explorer/cosmos.config.json
🧰 Additional context used
🧠 Learnings (3)
📚 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/explorer/src/components/orders/OrderDetails/FillsTable.tsxapps/explorer/src/components/orders/OrderDetails/index.tsx
📚 Learning: 2025-11-19T10:18:23.717Z
Learnt from: limitofzero
Repo: cowprotocol/cowswap PR: 6537
File: apps/cowswap-frontend/src/modules/trade/pure/PartnerFeeRow/index.tsx:33-35
Timestamp: 2025-11-19T10:18:23.717Z
Learning: In apps/cowswap-frontend/src/modules/trade/pure/PartnerFeeRow/index.tsx, when there is no partner fee (amount is null/undefined, bps is missing, or amount equals 0), FreeFeeRow is rendered with withTimelineDot={false} hardcoded. This is intentional design—free fee rows should not show the timeline dot regardless of what the parent component passes, as they have a distinct visual treatment from actual fee rows.
Applied to files:
apps/explorer/src/components/orders/OrderDetails/FillsTable.tsxapps/explorer/src/components/orders/OrderDetails/index.tsx
📚 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/explorer/src/components/orders/OrderDetails/index.tsx
🧬 Code graph analysis (4)
apps/explorer/src/api/operator/operatorApi.ts (2)
apps/explorer/src/api/operator/types.ts (2)
GetTradesParams(125-130)RawTrade(81-81)apps/explorer/src/cowSdk.ts (1)
orderBookSDK(11-14)
apps/explorer/src/hooks/useOperatorTrades.ts (3)
apps/explorer/src/types/index.ts (1)
UiError(17-20)apps/explorer/src/api/operator/types.ts (2)
Trade(86-100)RawTrade(81-81)apps/explorer/src/api/operator/operatorApi.ts (1)
getTrades(56-79)
apps/explorer/src/components/orders/OrderDetails/FillsTable.tsx (1)
apps/explorer/src/components/orders/OrderDetails/FillsTableRow.tsx (1)
FillsTableRow(30-93)
apps/explorer/src/components/orders/OrderDetails/index.tsx (2)
apps/explorer/src/explorer/components/OrdersTableWidget/useTable.tsx (1)
TableState(5-10)apps/explorer/src/api/operator/types.ts (2)
Order(38-76)Trade(86-100)
⏰ 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 (11)
apps/explorer/src/components/orders/OrderDetails/FillsTable.tsx (1)
29-37: LGTM - Pagination externalization complete.The component now correctly renders the trades array directly without internal pagination logic, delegating pagination control to the parent component as intended by this PR.
apps/explorer/src/api/operator/operatorApi.ts (1)
3-3: LGTM - Type extraction and parameter handling.The refactoring to use
GetTradesParamsimproves maintainability, and the logging correctly includes the new pagination parameters.Also applies to: 57-60
apps/explorer/src/hooks/useOperatorTrades.ts (4)
11-16: LGTM - Pagination parameters and return type.The addition of
hasNextPageto the result and default pagination parameters (offset=0, limit=10) are well-designed and consistent with standard pagination patterns.Also applies to: 51-51
62-84: "Limit plus one" strategy implemented correctly.The fetch logic correctly requests
limit + 1records to detect if more pages exist (Line 69). The dependency array (Line 83) appropriately includesoffsetandlimitto trigger refetch when pagination parameters change.Note: This implementation assumes
getTradesreturns properly paginated results. The PROD/BARN merge issue flagged inoperatorApi.tsmay affect the correctness of this strategy.
118-123: LGTM - hasNextPage detection and result memoization.The "limit plus one" detection logic is correctly implemented:
- Checks if
trades.length > limitto determine if more pages exist (Line 119)- Conditionally slices to
limitwhen there are more results (Line 122)- Properly memoizes the result including
hasNextPage(Lines 144-147)Also applies to: 144-147
111-117: Client-side sorting only affects the current page; verify backend returns trades in consistent order.The
getTradesAPI call passes no sort parameters, and each paginated batch is sorted independently on the client (lines 111-117). If the backend doesn't guarantee a consistent sort order across all results, users could see trades in different chronological order when navigating pages.Consider either:
- Passing an
orderByparameter to the backend (if supported by the API)- Documenting the expected sort order behavior
- Removing client-side sort if backend already returns sorted results
apps/explorer/src/components/orders/OrderDetails/index.tsx (5)
35-46: LGTM - Pagination props structure.The addition of
tableStateand pagination handlers (setPageSize,setPageOffset,handleNextPage,handlePreviousPage) follows a clear external pagination pattern, properly typing all new props.
138-143: LGTM - Correct handling of single trade with pagination.The updated logic correctly handles the edge case where the current page shows only one trade, but other trades exist on different pages. By checking both
trades.length === 1and!hasMultipleTrades, it ensurestxHashis only attached when there's truly a single trade for the entire order.
75-81: LGTM - Consistent hasMultipleTrades usage.The
hasMultipleTradesparameter is properly threaded throughtabItemsand used consistently in:
getOrderWithTxHashcall (Line 77)showFillscondition (Line 81)This ensures the Fills tab and order details rendering respect pagination state.
246-246: LGTM - hasMultipleTrades computation correctly handles pagination edge cases.The boolean expression correctly determines if multiple trades exist across all pages by checking:
trades.length > 1- Multiple trades on current pagetableState.pageIndex > 1- Previous pages exist with tradestableState.hasNextPage- Subsequent pages exist with tradesThis addresses the edge case previously flagged where a single trade on the current page but more on other pages would incorrectly set
hasMultipleTrades = false.
223-232: LGTM - Context provider properly wired with pagination state.The
FillsTableContext.Providercorrectly supplies all pagination state and handlers to child components, enabling the external pagination pattern implemented in this PR.
Summary
Fixes https://linear.app/cowswap/issue/COW-120
Related PR: cowprotocol/cow-sdk#713
The downside to this implementation is that you can't see the number of pages. (totalResults missing)
To Test
The backend change has been merged tobase-staging, but Ilya searched the database and the highest number of fills in a single order is 3.The change is live in production.
Example orders with many fills:
Base prod:
0x3c8a1b22e804935157767226acf931dc7443b5df4d98b4ee1a1a3017c90d0b51711429b3fdf0e76cf7288e8e4078dfdc5366026467d39719Background
The API latency spikes when this explorer page is loaded, the solution is to use pagination.
Break-down
useTablecall: from OrderDetails to the parent OrderWidgetuseOrderTrades, similar to accountOrderUtils.tscow-sdkto fix a type error when building the explorer:Summary by CodeRabbit
New Features
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.