refactor: generate order summary while render instead of caching #6767
Conversation
TODO: decompose useRecentActivity.ts file
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR removes precomputed/stored order summaries and shifts summary computation to runtime where needed; it also extracts CancellationModal styled components to a new module and updates types and callers to no longer include a stored Changes
Sequence Diagram(s)(Skipped — changes are refactors and plumbing adjustments without a new multi-component control-flow warranting a sequence diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (4)📚 Learning: 2025-10-10T20:28:16.565ZApplied to files:
📚 Learning: 2025-10-13T19:41:31.440ZApplied to files:
📚 Learning: 2025-08-05T14:27:05.023ZApplied to files:
📚 Learning: 2025-08-08T13:56:18.009ZApplied to files:
🧬 Code graph analysis (1)apps/cowswap-frontend/src/modules/twap/hooks/useTwapOrdersExecutions.ts (1)
⏰ 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)
🔇 Additional comments (1)
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 |
|
|
||
| let activity: EnhancedTransactionDetails | Order, type: ActivityType | ||
|
|
||
| let id: string, |
There was a problem hiding this comment.
I hate let, mutable code is evil
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTableContainer/orders.mock.ts (1)
107-119: Double cast in mock data.The
as never as EnrichedOrderpattern is used to force a type that the object doesn't fully satisfy. While this is acceptable for test mocks, consider whether the mock could be made more complete to avoid the double cast.Alternative: Create a typed mock helper
If this pattern appears elsewhere, consider creating a helper function for mock data:
function createMockEnrichedOrder(partial: Partial<EnrichedOrder>): EnrichedOrder { return partial as never as EnrichedOrder }This makes the intent clearer and centralizes the type workaround.
apps/cowswap-frontend/src/common/pure/CancellationModal/RequestCancellationModal.tsx (1)
70-71: TODO comment noted for tracking.The
TODO: ORDER_SUMMARY_ENTRYcomment indicates this is preparatory work for issue #6748. The current implementation correctly displays the summary when provided.Would you like me to open an issue to track this TODO, or is this already covered by issue #6748?
apps/cowswap-frontend/src/modules/account/containers/Transaction/ActivityDetails.tsx (1)
590-595: Inline comments may be slightly misleading.The fallback logic is correct, but the inline comments could be clearer. The comment
// Transactionon line 590 applies to the entire else branch (non-order activities), while// Orderon line 592 refers to the order fallback computation within that branch. Consider clarifying:🔎 Suggested comment clarification
) : ( - // Transaction - (activityDerivedState.summary ?? - // Order - (order ? computeOrderSummary({ orderFromStore: order, orderFromApi: order.apiAdditionalInfo }) : null) ?? - id) + // Non-order transaction summary fallback chain: + // 1. Use transaction summary if available + // 2. Compute order summary dynamically if order exists + // 3. Fall back to activity id + (activityDerivedState.summary ?? + (order ? computeOrderSummary({ orderFromStore: order, orderFromApi: order.apiAdditionalInfo }) : null) ?? + id) )}apps/cowswap-frontend/src/legacy/hooks/useRecentActivity.ts (1)
140-152: Minor: Unreachablereturn nullat the end.After the early return on line 127, at least one of
txororderis truthy. Since both branches (lines 129-138 and 140-150) return, line 152 is unreachable. The code is correct but could be slightly cleaner.🔎 Suggested simplification
if (tx) { return { id: tx.hash, activity: tx, summary: tx.summary, status: getTxActivityStatus(tx), type: ActivityType.TX, date: new Date(tx.addedTime), fillability: orderFillability, } } - - return null + + // TypeScript cannot infer this is unreachable; added for exhaustiveness + throw new Error('Unreachable: either tx or order must be defined') }Alternatively, keep the current form if TypeScript requires a terminal return for type safety—this is purely stylistic.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
apps/cowswap-frontend/src/common/containers/CancellationModal/index.tsxapps/cowswap-frontend/src/common/hooks/useCancelOrder/index.tsapps/cowswap-frontend/src/common/hooks/useCancelOrder/state.tsapps/cowswap-frontend/src/common/pure/CancellationModal/RequestCancellationModal.tsxapps/cowswap-frontend/src/common/pure/CancellationModal/index.cosmos.tsxapps/cowswap-frontend/src/common/pure/CancellationModal/index.tsxapps/cowswap-frontend/src/common/pure/CancellationModal/styled.tsxapps/cowswap-frontend/src/common/updaters/orders/OrdersFromApiUpdater.tsapps/cowswap-frontend/src/common/updaters/orders/utils.tsapps/cowswap-frontend/src/legacy/hooks/useRecentActivity.tsapps/cowswap-frontend/src/legacy/state/orders/actions.tsapps/cowswap-frontend/src/legacy/state/orders/mocks.tsapps/cowswap-frontend/src/legacy/state/orders/orderPrice.test.tsapps/cowswap-frontend/src/legacy/state/orders/reducer.tsapps/cowswap-frontend/src/legacy/utils/trade.tsapps/cowswap-frontend/src/mocks/orderMock.tsapps/cowswap-frontend/src/modules/account/containers/Transaction/ActivityDetails.tsxapps/cowswap-frontend/src/modules/ethFlow/services/ethFlow/index.tsapps/cowswap-frontend/src/modules/limitOrders/services/safeBundleFlow/index.tsapps/cowswap-frontend/src/modules/limitOrders/services/tradeFlow/index.tsapps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTableContainer/orders.mock.tsapps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleApprovalFlow.tsapps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.tsapps/cowswap-frontend/src/modules/tradeFlow/services/swapFlow/index.tsapps/cowswap-frontend/src/modules/twap/utils/mapPartOrderToStoreOrder.tsapps/cowswap-frontend/src/modules/twap/utils/mapTwapOrderToStoreOrder.ts
💤 Files with no reviewable changes (4)
- apps/cowswap-frontend/src/mocks/orderMock.ts
- apps/cowswap-frontend/src/legacy/state/orders/mocks.ts
- apps/cowswap-frontend/src/legacy/utils/trade.ts
- apps/cowswap-frontend/src/common/updaters/orders/OrdersFromApiUpdater.ts
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
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...
📚 Learning: 2025-04-02T09:58:29.374Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 5549
File: apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.ts:152-152
Timestamp: 2025-04-02T09:58:29.374Z
Learning: In the `safeBundleEthFlow` function, `account` is guaranteed to be truthy based on the type system (`PostOrderParams` defines it as a required string) and the context in which the function is called, so additional runtime checks are unnecessary.
Applied to files:
apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.tsapps/cowswap-frontend/src/modules/limitOrders/services/safeBundleFlow/index.tsapps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleApprovalFlow.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/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.tsapps/cowswap-frontend/src/modules/ethFlow/services/ethFlow/index.tsapps/cowswap-frontend/src/common/containers/CancellationModal/index.tsxapps/cowswap-frontend/src/common/pure/CancellationModal/index.cosmos.tsxapps/cowswap-frontend/src/legacy/state/orders/reducer.tsapps/cowswap-frontend/src/common/pure/CancellationModal/index.tsxapps/cowswap-frontend/src/modules/limitOrders/services/tradeFlow/index.tsapps/cowswap-frontend/src/modules/limitOrders/services/safeBundleFlow/index.tsapps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleApprovalFlow.tsapps/cowswap-frontend/src/modules/twap/utils/mapPartOrderToStoreOrder.tsapps/cowswap-frontend/src/common/hooks/useCancelOrder/index.tsapps/cowswap-frontend/src/common/pure/CancellationModal/styled.tsxapps/cowswap-frontend/src/modules/twap/utils/mapTwapOrderToStoreOrder.tsapps/cowswap-frontend/src/common/hooks/useCancelOrder/state.tsapps/cowswap-frontend/src/modules/tradeFlow/services/swapFlow/index.tsapps/cowswap-frontend/src/common/pure/CancellationModal/RequestCancellationModal.tsxapps/cowswap-frontend/src/modules/ordersTable/pure/OrdersTableContainer/orders.mock.tsapps/cowswap-frontend/src/modules/account/containers/Transaction/ActivityDetails.tsx
📚 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/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.tsapps/cowswap-frontend/src/modules/limitOrders/services/tradeFlow/index.tsapps/cowswap-frontend/src/modules/limitOrders/services/safeBundleFlow/index.tsapps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleApprovalFlow.tsapps/cowswap-frontend/src/modules/tradeFlow/services/swapFlow/index.ts
📚 Learning: 2025-07-24T16:43:47.639Z
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6009
File: apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/highFeeWarningHelpers.ts:18-20
Timestamp: 2025-07-24T16:43:47.639Z
Learning: In apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/highFeeWarningHelpers.ts, the formatFeePercentage function uses ROUND_DOWN with toSignificant(2) for "at least X%" messaging. This ensures the displayed percentage is never higher than the actual fee, making the "at least" phrasing accurate. For example, if the actual fee is 25.4%, displaying "at least 25%" is correct, but "at least 26%" would be misleading.
Applied to files:
apps/cowswap-frontend/src/legacy/state/orders/orderPrice.test.ts
📚 Learning: 2025-02-20T15:59:33.749Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 5443
File: apps/cowswap-frontend/src/modules/swap/containers/ConfirmSwapModalSetup/index.tsx:71-71
Timestamp: 2025-02-20T15:59:33.749Z
Learning: The swap module in apps/cowswap-frontend/src/modules/swap/ is marked for deletion in PR #5444 as part of the swap widget unification effort.
Applied to files:
apps/cowswap-frontend/src/modules/limitOrders/services/tradeFlow/index.tsapps/cowswap-frontend/src/modules/tradeFlow/services/swapFlow/index.tsapps/cowswap-frontend/src/common/pure/CancellationModal/RequestCancellationModal.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/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleApprovalFlow.ts
📚 Learning: 2025-08-05T14:27:05.023Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5992
File: libs/wallet/src/web3-react/utils/switchChain.ts:36-38
Timestamp: 2025-08-05T14:27:05.023Z
Learning: In libs/wallet/src/web3-react/utils/switchChain.ts, the team prefers using Record<SupportedChainId, string | null> over Partial<Record<SupportedChainId, string>> for WALLET_RPC_SUGGESTION to enforce that all supported chain IDs have explicit values set, even if some might be null. This ensures compile-time completeness checking.
Applied to files:
apps/cowswap-frontend/src/common/updaters/orders/utils.ts
🧬 Code graph analysis (3)
apps/cowswap-frontend/src/common/containers/CancellationModal/index.tsx (2)
apps/cowswap-frontend/src/common/updaters/orders/utils.ts (1)
computeOrderSummary(25-83)apps/cowswap-frontend/src/common/pure/CancellationModal/index.cosmos.tsx (1)
onDismiss(26-28)
apps/cowswap-frontend/src/legacy/hooks/useRecentActivity.ts (2)
apps/cowswap-frontend/src/common/hooks/useCategorizeRecentActivity.ts (2)
isCreating(22-22)isPending(14-18)apps/cowswap-frontend/src/legacy/state/enhancedTransactions/reducer.ts (1)
EnhancedTransactionDetails(22-65)
apps/cowswap-frontend/src/modules/account/containers/Transaction/ActivityDetails.tsx (1)
apps/cowswap-frontend/src/common/updaters/orders/utils.ts (1)
computeOrderSummary(25-83)
⏰ 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 (25)
apps/cowswap-frontend/src/modules/tradeFlow/services/swapFlow/index.ts (2)
14-14: LGTM: Import cleanup aligns with the refactoring.The removal of
getOrderSubmitSummaryis consistent with the PR's goal of moving summary generation closer to rendering components.
188-196: LGTM: Summary removed from order mapping.The removal of summary computation during order creation aligns with the refactoring objective. Summary will now be computed at render time where needed.
apps/cowswap-frontend/src/common/updaters/orders/utils.ts (1)
3-3: LGTM: Type refinement to accept nullish values.The change from
EnrichedOrder | nulltoNullish<EnrichedOrder>makes the parameter more permissive (acceptingundefinedin addition tonull). The existing falsy check at line 32 already handles both cases correctly.Also applies to: 30-30
apps/cowswap-frontend/src/modules/twap/utils/mapTwapOrderToStoreOrder.ts (1)
26-39: LGTM: Simplified mapping function.The refactoring removes the intermediate
storeOrdervariable and summary computation, directly returning the order object. This aligns with the PR's goal of computing summaries at render time rather than during order mapping.apps/cowswap-frontend/src/modules/twap/utils/mapPartOrderToStoreOrder.ts (1)
27-42: LGTM: Consistent refactoring with mapTwapOrderToStoreOrder.Similar to the sibling file, this change removes the intermediate variable and summary computation, simplifying the function. All necessary order fields are preserved in the returned object.
apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.ts (1)
12-12: LGTM: Consistent summary removal across flow services.The changes mirror those in
swapFlow/index.ts, removing summary precomputation from the safe bundle ETH flow. This maintains consistency across different trade flow implementations.Also applies to: 125-133
apps/cowswap-frontend/src/common/hooks/useCancelOrder/state.ts (1)
23-33: LGTM: Summary removed from cancellation modal context.Removing the
summaryfield from the cancellation modal context aligns with the refactoring objective. The summary will now be computed within theCancellationModalcomponent at render time rather than being stored in the context.apps/cowswap-frontend/src/modules/limitOrders/services/tradeFlow/index.ts (1)
11-11: LGTM - Import removal aligns with refactoring.The removal of
getOrderSubmitSummaryimport is consistent with the PR's goal to eliminate precomputed order summaries.apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleApprovalFlow.ts (1)
10-10: LGTM - Consistent refactoring pattern.Import removal follows the same pattern as other flow files in this PR.
apps/cowswap-frontend/src/modules/ethFlow/services/ethFlow/index.ts (1)
11-11: LGTM - Ethflow aligned with refactoring.Removal of precomputed summary is consistent across all flow files.
apps/cowswap-frontend/src/common/pure/CancellationModal/index.cosmos.tsx (1)
30-30: LGTM - Fixture updated for new prop.The hardcoded
orderSummaryis appropriate for Cosmos visual testing fixtures.apps/cowswap-frontend/src/legacy/state/orders/orderPrice.test.ts (1)
59-59: LGTM - Test data aligned with new order shape.The removal of the
summaryfield from test orders is consistent with the refactoring objectives.apps/cowswap-frontend/src/common/hooks/useCancelOrder/index.ts (1)
84-84: LGTM - Refactoring moves summary computation to render time.The removal of summary computation from the cancellation hook aligns with the PR objectives. The error handling simplification (removing
: anytype annotation) is acceptable.apps/cowswap-frontend/src/common/containers/CancellationModal/index.tsx (1)
23-26: The current usage is correct. The code passesorder(Redux order object) fororderFromStoreandorder.apiAdditionalInfo(the nested EnrichedOrder data) fororderFromApi—these are different objects with different data shapes. The function is designed to accept a Redux order alongside its embedded API enrichment data to leverage properties from either source.apps/cowswap-frontend/src/legacy/state/orders/reducer.ts (1)
383-383: No action needed—this assignment is type-safe and fully compatible with downstream consumers.The field
apiAdditionalInfois typed asEnrichedOrder(line 60 of actions.ts), and theordervariable in thefulfillOrdersBatchhandler is of typeEnrichedOrder. All downstream accesses use properties that exist inEnrichedOrder(executedBuyAmount, executedSellAmount, executedFeeAmount, fullAppData, ethflowData, etc.), and defensive patterns like optional chaining are already employed throughout the codebase. There is no structure incompatibility introduced by this change.Likely an incorrect or invalid review comment.
apps/cowswap-frontend/src/modules/limitOrders/services/safeBundleFlow/index.ts (2)
11-11: LGTM! Import cleanup aligns with refactor.The removal of
getOrderSubmitSummaryimport is consistent with the PR's objective to move summary computation from order creation time to render time.
108-116: LGTM! Summary removal from order mapping.The
mapUnsignedOrderToOrdercall no longer includes asummaryfield. Verification confirms all six call sites across the codebase are consistent in omitting thesummaryparameter, and the function definition no longer expects or uses it. The refactor removing pre-computed summaries from the order storage path is complete.apps/cowswap-frontend/src/common/pure/CancellationModal/RequestCancellationModal.tsx (1)
5-18: LGTM! Clean extraction of styled components.The styled components are now imported from a dedicated module, improving maintainability and separation of concerns.
apps/cowswap-frontend/src/modules/account/containers/Transaction/ActivityDetails.tsx (2)
17-17: LGTM! Import updates for dynamic summary computation.The addition of
computeOrderSummaryimport and consolidation of other imports properly supports the new render-time summary generation approach.Also applies to: 27-27, 48-48
237-238: LGTM! Removal of pre-computed summary from destructuring.The
summaryfield is no longer destructured fromactivityDerivedState, as summaries are now computed dynamically when needed.apps/cowswap-frontend/src/common/pure/CancellationModal/index.tsx (2)
17-27: LGTM! Clean prop-based summary injection.The refactor from context-embedded summary to an explicit
orderSummaryprop follows a clear pattern. The caller now controls summary computation, enabling dynamic generation at render time.
49-49: LGTM! Consistent usage of orderSummary throughout the component.The
orderSummaryprop is correctly:
- Displayed in the pending cancellation content (line 49)
- Passed to
RequestCancellationModal(line 61)- Included in the
useMemodependency array (line 77)Also applies to: 61-61, 77-77
apps/cowswap-frontend/src/common/pure/CancellationModal/styled.tsx (1)
1-61: LGTM! Well-structured styled components extraction.The styled components are cleanly extracted with:
- Proper use of transient prop convention (
isOnChain$) to prevent DOM attribute leakage- Consistent use of UI theme variables
- Clear component naming that matches their purpose
apps/cowswap-frontend/src/legacy/hooks/useRecentActivity.ts (2)
155-179: LGTM! Status derivation logic is clear.The helper correctly prioritizes statuses (cancelling > failed > pending > etc.) and defaults to EXPIRED for any unmatched states. The logic for determining cancelling state only applies to PENDING and CREATING orders, which aligns with the documented behavior.
Minor: The inline comment at lines 160-165 duplicates the JSDoc at lines 181-188. Consider removing one to reduce maintenance burden.
189-191: LGTM!The guard correctly restricts the "cancelling" visual state to orders that are actively pending, preventing misleading UI for orders that have already reached a terminal state.
| return null | ||
| } | ||
|
|
||
| function getOrderActivityStatus(order: Order): ActivityStatus { |
There was a problem hiding this comment.
good refactoring, thank you!
| fullAppData: order.fullAppData, | ||
| signingScheme: order.signingScheme, | ||
| } | ||
| orderObject.order.apiAdditionalInfo = order |
There was a problem hiding this comment.
question: function is called - reducer, should it be { ...order } instead of using the same ref?
There was a problem hiding this comment.
I think so, but I'm afraid of touching this legacy.
maybe @crutch12 will help us and refactor it to jotai atoms...
Summary
Preparation to fix #6748
While investigating #6748 issue, I found out that the problem is in
computeOrderSummary. That function doesn't take cross-chain swap orders into account. Obviously, I need to put bridge order data inside of the function to fix the issue.Then I started looking for a place, where I can do that, and it turned out that there is a long chain of mappings of the value. But ultimately, there are only two places where it's used:
What is a result of
computeOrderSummary? It's a string likeSWAP 1 ETH for 5000 DAI.From my point of view, it should be generated while rendering a component, no need to pregenerate it and save into store. At least because the result depends on order status, while order is pending we show one amounts, once it's filled, we show other ones.
Because of that, I decided to move
computeOrderSummarycalls as close as possible to components which render the summary.To do that, I started the refactoring.
Please review this PR commit by commit:
createActivityDescriptor. No functional changes here, just got rid ofeslint-disable-next-line. 405295esummaryproperty inBaseOrder/Orderinterface, because this model is stored in redux store with localStorage persistence.Order.summarywas used in only one place -ActivityDetails. So I could easily movecreateActivityDescriptorcall to that component and safely removeOrder.summary. 448ac0e#diff-81fd97ea8b16aa886f6d6225e2182045d2dfcd8fd5458c74f8d0d02fc9ef52d9RequestCancellationModalfile, just extracted styled elements to a separate file, no functional changes here. 9fad34a#diff-3df799ccd7ff9b27678cd609f3a616cec967ff9698a52ad1cf1af309ea9a5e8acomputeOrderSummarycall fromuseCancelOrderto the component where it's used -CancellationModal. 533ac14#diff-08f5b5aa18eedeb547f357891010c04806d6e9a3d0fcf8e173a17ea6e3bdafbfHaving that refactoring we can easily extend
computeOrderSummarylogic and even convert it to a React component instead of being just a string. We couldn't do that, because the value was stored in redux store with localStorage persistence.To Test
No need to test so far, there will be another PR with actual fix of #6748
Summary by CodeRabbit
New Features
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.