chore(eslint): restrict modules imports inside common dir #6592
Conversation
|
@crutch12 is attempting to deploy a commit to the cow-dev Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds file-level ESLint disable comments for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
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 ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
![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
♻️ Duplicate comments (11)
apps/cowswap-frontend/src/common/hooks/useSwapResultsContext.ts (1)
1-1: Same note on file-wideno-restricted-importsdisableAs in
common/pure/ReceiveAmount/index.tsx, a file-level disable will also exempt any futuremodules/*imports added here. If you later tighten this up, consider per-importeslint-disable-next-line no-restricted-importscomments instead.apps/cowswap-frontend/src/common/containers/OrderHooksDetails/index.tsx (1)
1-1: File-wide suppression continues to allow newmodules/*importsThis follows the same pattern as other
common/*files: the top-of-file disable will also cover any newmodules/*imports added here. If/when you start paying down the TODOs in this component, it may be worth scoping the disable to just the offending imports.apps/cowswap-frontend/src/common/updaters/orders/CancelledOrdersUpdater.ts (1)
1-1: Legacymodules/*dependency masked for the whole fileThe file-level
no-restricted-importsdisable correctly unblocks the existingmodules/ordersusage, but it will also hide any new restricted imports added later in this updater. Same consideration as othercommon/*files: if you want stricter enforcement over time, narrowing the disable to just themodules/*import lines would help.apps/cowswap-frontend/src/common/updaters/orders/UnfillableOrdersUpdater/hooks/useUpdateIsUnfillableFlag.ts (1)
1-1: Consistent with other legacy exceptions formodules/*importsThis file-wide suppression matches the approach in other
common/*files. Just be aware it will also cover any newmodules/*imports added here until the TODO is addressed.apps/cowswap-frontend/src/common/pure/ConfirmationPendingContent/ConfirmationPendingContentShell.tsx (1)
1-1: File-levelno-restricted-importsdisable for existingmodules/accountusageSame as other
common/*files: this unblocks the currentmodules/accountimport but will also hide any futuremodules/*imports in this file until it’s refactored.apps/cowswap-frontend/src/common/hooks/useGetSurplusFiatValue.ts (1)
1-1: Legacymodules/*import exemption mirrors other surplus/bridge hooksThis file-wide disable keeps the existing
modules/usdAmountdependency working but will also allow any additionalmodules/*imports in this file without lint errors until you tackle the TODO.apps/cowswap-frontend/src/common/pure/CurrencyInputPanel/CurrencyInputPanel.tsx (1)
1-1: Same note about scoping theno-restricted-importsdisableThis mirrors the pattern in
useEnhancedActivityDerivedState: if feasible, prefer an inline// eslint-disable-next-line no-restricted-importson themodules/*imports instead of disabling the rule for the whole file, so other restricted imports remain enforced.apps/cowswap-frontend/src/common/updaters/WidgetTokensUpdater.tsx (1)
1-1: Scoped disable would keep otherno-restricted-importsprotections activeAs with the other
commonfiles, you might eventually want to replace the file-level/* eslint-disable no-restricted-imports */with inline// eslint-disable-next-line no-restricted-importson themodules/*imports, so the rule still guards against other restricted imports in this file.apps/cowswap-frontend/src/common/updaters/BridgingEnabledUpdater.ts (1)
1-1: File-levelno-restricted-importsdisable mirrors othercommonfilesSame comment as above: for long-term cleanliness, consider scoping the disable to the
modules/tradeimport line rather than disablingno-restricted-importsfor the entire file.apps/cowswap-frontend/src/common/updaters/orders/PendingOrdersUpdater.ts (1)
1-1: Top-levelno-restricted-importsdisable is consistent with the rest of the PRMatches the pattern in other
commonfiles. When you eventually refactor awaymodules/ordershere, consider also tightening this to an inline// eslint-disable-next-line no-restricted-importson that import (or removing it entirely) to restore full lint coverage.apps/cowswap-frontend/src/common/containers/CoWAmmBanner/index.tsx (1)
1-1: Same scoped-disable consideration forCoWAmmBannerGiven the multiple
modules/*imports here, the file-wide disable is understandable; just keep in mind it also disables otherno-restricted-importschecks. Once these imports are moved out ofcommon, you can drop or narrow this directive.
🧹 Nitpick comments (9)
apps/cowswap-frontend/src/common/utils/doesOrderHavePermit.ts (2)
1-1: Improve the TODO comment for clarity and actionability.The ESLint disable approach aligns with the PR's enforcement strategy, but the TODO could be more specific to guide the refactoring effort.
Consider updating the TODO to be more actionable:
-/* eslint-disable no-restricted-imports */ // TODO: Don't use 'modules' import +/* eslint-disable no-restricted-imports */ // TODO: Refactor to pass hooks as parameter or move getAppDataHooks to common/
4-4: Verify that the modules/appData dependency cannot be restructured.This file violates the documented architectural rule that
common/must not import frommodules/. While this PR intentionally suppresses existing violations, you should evaluate if the dependency can be eliminated or moved to thecommon/directory.Possible refactoring approaches:
- Pass
hooksorappDataas parameters to these functions instead of computing them internally.- Move the
getAppDataHookslogic intocommon/if it is general-purpose utility code.- Create an abstraction in
common/that these functions depend on, withmodules/providing the implementation separately.apps/cowswap-frontend/src/common/pure/ExecutionPrice/hooks/useExecutionPriceFiat.ts (1)
1-1: Temporary suppression acknowledged — offer to help refactor.This eslint-disable comment aligns with the PR's objective to temporarily suppress architectural violations. However, this file imports
useUsdAmountfrommodules/usdAmount(line 5), violating the documented rule thatcommon/must not depend onmodules/.Would you like me to help refactor this file to remove the
modulesdependency? I can propose a solution that moves the USD amount conversion logic to avoid the architectural violation.apps/cowswap-frontend/src/common/pure/ReceiveAmount/index.tsx (1)
1-1: Scopeno-restricted-importsdisable more narrowly if you want stricter enforcementDisabling
no-restricted-importsfor the whole file means any newmodules/*imports added here later will also be silently allowed. If you want the new rule to catch regressions even in legacy files, consider switching to// eslint-disable-next-line no-restricted-importsjust on the specificmodules/*import lines when you next touch this file, and keep the TODO as a reminder to refactor them away.apps/cowswap-frontend/src/common/pure/ConfirmationPendingContent/ConfirmationPendingContentShell.tsx (1)
27-45: Remove redundant{modalMode}child for clarityInside the JSX,
{modalMode}(Line 29) doesn’t render anything (booleans are ignored by React), so it’s effectively a no-op and a bit confusing to readers. You can safely remove that expression.apps/cowswap-frontend/src/common/hooks/useEnhancedActivityDerivedState.ts (1)
1-1: Scope theno-restricted-importsdisable to the offending import instead of the whole fileThe file-wide disable is understandable for now, but it also turns off all other
no-restricted-importsprotections here. Consider narrowing it to just themodules/*import (e.g.,// eslint-disable-next-line no-restricted-importson that line) so future restricted imports in this file still get caught.apps/cowswap-frontend/src/common/hooks/useGetExecutedBridgeSummary.ts (1)
1-12: Lint suppression plus executed summary selection both look reasonable
- The added
getExecutedSummaryDataWithSurplusTokenusage inside theuseMemolooks consistent: it’s only called whenintermediateTokenexists and dependencies[order, intermediateToken]are correct.- Same as other
commonfiles, the file-level/* eslint-disable no-restricted-imports */is fine as a temporary workaround, but longer term you may want to restrict it to just themodules/tradeimport so other restricted imports still get caught.apps/cowswap-frontend/src/common/hooks/useRateInfoParams.ts (1)
1-1: File-wideno-restricted-importsdisable is acceptable as a temporary escape hatchThis aligns with the PR goal of unblocking existing
modulesimports fromcommon. As follow‑up, consider narrowing the scope later (e.g.,eslint-disable-next-lineon the specificmodulesimport) so new violations in this file don’t slip through unnoticed.Please double‑check that this file is listed among the known temporary exceptions you plan to refactor away from
modulesimports.apps/cowswap-frontend/src/common/hooks/useCancelOrder/onChainCancellation.ts (1)
1-1: Temporaryno-restricted-importsdisable is consistent with the migration strategyUsing a file-level disable here matches the PR’s approach to existing
modulesimports fromcommon. As with other files, it would be good to eventually either:
- remove this directive once the
modulesimport is eliminated, or- narrow it to an inline
eslint-disable-next-lineon that specific import to avoid hiding new violations.Confirm that this file is tracked in whatever list/issue you’re using for cleaning up
modulesimports fromcommon.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
apps/cowswap-frontend/src/common/containers/CoWAmmBanner/index.tsx(1 hunks)apps/cowswap-frontend/src/common/containers/CoWAmmBanner/types.ts(1 hunks)apps/cowswap-frontend/src/common/containers/OrderHooksDetails/HookItem/index.tsx(1 hunks)apps/cowswap-frontend/src/common/containers/OrderHooksDetails/index.tsx(1 hunks)apps/cowswap-frontend/src/common/hooks/useCancelOrder/onChainCancellation.ts(4 hunks)apps/cowswap-frontend/src/common/hooks/useCancelOrder/useGetOnChainCancellation.ts(1 hunks)apps/cowswap-frontend/src/common/hooks/useCancelOrder/useSendOnChainCancellation.ts(1 hunks)apps/cowswap-frontend/src/common/hooks/useConvertUsdToTokenValue.ts(1 hunks)apps/cowswap-frontend/src/common/hooks/useEnhancedActivityDerivedState.ts(1 hunks)apps/cowswap-frontend/src/common/hooks/useGetExecutedBridgeSummary.ts(1 hunks)apps/cowswap-frontend/src/common/hooks/useGetMarketDimension.ts(1 hunks)apps/cowswap-frontend/src/common/hooks/useGetSurplusFiatValue.ts(1 hunks)apps/cowswap-frontend/src/common/hooks/useOnSelectNetwork.tsx(1 hunks)apps/cowswap-frontend/src/common/hooks/useRateInfoParams.ts(3 hunks)apps/cowswap-frontend/src/common/hooks/useSwapAndBridgeContext.ts(1 hunks)apps/cowswap-frontend/src/common/hooks/useSwapAndBridgeOverview.ts(1 hunks)apps/cowswap-frontend/src/common/hooks/useSwapResultsContext.ts(1 hunks)apps/cowswap-frontend/src/common/pure/CoWAmmBannerContent/index.tsx(1 hunks)apps/cowswap-frontend/src/common/pure/ConfirmationPendingContent/ConfirmationPendingContentShell.tsx(1 hunks)apps/cowswap-frontend/src/common/pure/CurrencyInputPanel/CurrencyInputPanel.tsx(1 hunks)apps/cowswap-frontend/src/common/pure/CurrencyInputPanel/types.ts(1 hunks)apps/cowswap-frontend/src/common/pure/ExecutionPrice/hooks/useExecutionPriceFiat.ts(2 hunks)apps/cowswap-frontend/src/common/pure/ReceiveAmount/index.tsx(1 hunks)apps/cowswap-frontend/src/common/pure/ReceiveAmountInfo/index.tsx(1 hunks)apps/cowswap-frontend/src/common/updaters/BridgingEnabledUpdater.ts(1 hunks)apps/cowswap-frontend/src/common/updaters/SentryUpdater.ts(1 hunks)apps/cowswap-frontend/src/common/updaters/WidgetTokensUpdater.tsx(1 hunks)apps/cowswap-frontend/src/common/updaters/orders/CancelledOrdersUpdater.ts(1 hunks)apps/cowswap-frontend/src/common/updaters/orders/OrdersFromApiUpdater.ts(1 hunks)apps/cowswap-frontend/src/common/updaters/orders/PendingOrdersUpdater.ts(1 hunks)apps/cowswap-frontend/src/common/updaters/orders/SpotPricesUpdater.ts(1 hunks)apps/cowswap-frontend/src/common/updaters/orders/UnfillableOrdersUpdater/hooks/useUpdateIsUnfillableFlag.ts(1 hunks)apps/cowswap-frontend/src/common/updaters/orders/UnfillableOrdersUpdater/updaters/UnfillableOrderUpdater.ts(1 hunks)apps/cowswap-frontend/src/common/utils/doesOrderHavePermit.ts(1 hunks)eslint.config.js(1 hunks)libs/wallet-provider/.eslintrc.json(1 hunks)
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 6234
File: libs/tokens/src/index.ts:1-4
Timestamp: 2025-09-11T08:25:51.460Z
Learning: In the cowprotocol/cowswap project, there is currently no SSR (Server-Side Rendering) support, so localStorage access at module import time does not cause SSR-related issues.
📚 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/common/pure/ReceiveAmountInfo/index.tsxapps/cowswap-frontend/src/common/hooks/useCancelOrder/useGetOnChainCancellation.tsapps/cowswap-frontend/src/common/hooks/useGetMarketDimension.tsapps/cowswap-frontend/src/common/updaters/orders/CancelledOrdersUpdater.tsapps/cowswap-frontend/src/common/containers/CoWAmmBanner/index.tsxapps/cowswap-frontend/src/common/hooks/useOnSelectNetwork.tsxapps/cowswap-frontend/src/common/hooks/useSwapAndBridgeContext.tsapps/cowswap-frontend/src/common/hooks/useSwapResultsContext.tsapps/cowswap-frontend/src/common/updaters/orders/PendingOrdersUpdater.tsapps/cowswap-frontend/src/common/containers/OrderHooksDetails/index.tsxapps/cowswap-frontend/src/common/hooks/useEnhancedActivityDerivedState.tsapps/cowswap-frontend/src/common/hooks/useSwapAndBridgeOverview.tsapps/cowswap-frontend/src/common/containers/OrderHooksDetails/HookItem/index.tsxapps/cowswap-frontend/src/common/hooks/useCancelOrder/useSendOnChainCancellation.tsapps/cowswap-frontend/src/common/pure/ConfirmationPendingContent/ConfirmationPendingContentShell.tsxapps/cowswap-frontend/src/common/pure/CurrencyInputPanel/CurrencyInputPanel.tsx
📚 Learning: 2025-06-16T15:58:00.268Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 5830
File: apps/cowswap-frontend/src/modules/trade/containers/TradeWidget/index.tsx:1-2
Timestamp: 2025-06-16T15:58:00.268Z
Learning: JSX can be imported as a named export from React in modern React versions (React 17+). The import `import { JSX } from 'react'` is valid and does not cause compilation errors.
Applied to files:
apps/cowswap-frontend/src/common/pure/ReceiveAmountInfo/index.tsx
📚 Learning: 2025-07-24T16:42:53.154Z
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6009
File: apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/HighFeeWarningTooltipContent.tsx:23-33
Timestamp: 2025-07-24T16:42:53.154Z
Learning: In apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/HighFeeWarningTooltipContent.tsx, the use of toFixed(2) for percentage formatting in tooltip content is intentional and differs from the banner message formatting that uses toSignificant(2, undefined, Rounding.ROUND_DOWN). This formatting difference serves different UX purposes and should not be flagged as inconsistent.
Applied to files:
apps/cowswap-frontend/src/common/hooks/useConvertUsdToTokenValue.tsapps/cowswap-frontend/src/common/pure/CurrencyInputPanel/CurrencyInputPanel.tsxapps/cowswap-frontend/src/common/pure/ExecutionPrice/hooks/useExecutionPriceFiat.tsapps/cowswap-frontend/src/common/hooks/useRateInfoParams.ts
📚 Learning: 2025-09-11T08:25:51.460Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 6234
File: libs/tokens/src/index.ts:1-4
Timestamp: 2025-09-11T08:25:51.460Z
Learning: In the cowprotocol/cowswap project, there is currently no SSR (Server-Side Rendering) support, so localStorage access at module import time does not cause SSR-related issues.
Applied to files:
apps/cowswap-frontend/src/common/hooks/useConvertUsdToTokenValue.tsapps/cowswap-frontend/src/common/hooks/useCancelOrder/useGetOnChainCancellation.tsapps/cowswap-frontend/src/common/hooks/useGetMarketDimension.tsapps/cowswap-frontend/src/common/updaters/SentryUpdater.tsapps/cowswap-frontend/src/common/updaters/orders/CancelledOrdersUpdater.tsapps/cowswap-frontend/src/common/pure/CoWAmmBannerContent/index.tsxapps/cowswap-frontend/src/common/containers/CoWAmmBanner/index.tsxapps/cowswap-frontend/src/common/hooks/useOnSelectNetwork.tsxapps/cowswap-frontend/src/common/updaters/WidgetTokensUpdater.tsxapps/cowswap-frontend/src/common/hooks/useGetSurplusFiatValue.tsapps/cowswap-frontend/src/common/hooks/useSwapAndBridgeContext.tsapps/cowswap-frontend/src/common/hooks/useSwapResultsContext.tsapps/cowswap-frontend/src/common/pure/ReceiveAmount/index.tsxapps/cowswap-frontend/src/common/updaters/BridgingEnabledUpdater.tsapps/cowswap-frontend/src/common/utils/doesOrderHavePermit.tsapps/cowswap-frontend/src/common/containers/OrderHooksDetails/index.tsxapps/cowswap-frontend/src/common/hooks/useEnhancedActivityDerivedState.tsapps/cowswap-frontend/src/common/hooks/useSwapAndBridgeOverview.tsapps/cowswap-frontend/src/common/containers/OrderHooksDetails/HookItem/index.tsxapps/cowswap-frontend/src/common/hooks/useCancelOrder/useSendOnChainCancellation.tsapps/cowswap-frontend/src/common/pure/CurrencyInputPanel/CurrencyInputPanel.tsx
📚 Learning: 2025-09-19T11:38:59.206Z
Learnt from: fairlighteth
Repo: cowprotocol/cowswap PR: 6232
File: apps/cowswap-frontend/src/modules/tokensList/pure/ChainsSelector/index.tsx:199-200
Timestamp: 2025-09-19T11:38:59.206Z
Learning: The makeBuildClickEvent function in apps/cowswap-frontend/src/modules/tokensList/pure/ChainsSelector/index.tsx takes five parameters: defaultChainId, contextLabel, mode, isSwapMode, and chainsCount. The chainsCount parameter is used to determine the CrossChain flag in analytics events.
Applied to files:
apps/cowswap-frontend/src/common/hooks/useConvertUsdToTokenValue.tsapps/cowswap-frontend/src/common/hooks/useOnSelectNetwork.tsxapps/cowswap-frontend/src/common/hooks/useSwapAndBridgeContext.tsapps/cowswap-frontend/src/common/hooks/useEnhancedActivityDerivedState.tsapps/cowswap-frontend/src/common/hooks/useSwapAndBridgeOverview.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/hooks/useConvertUsdToTokenValue.tsapps/cowswap-frontend/src/common/hooks/useOnSelectNetwork.tsxapps/cowswap-frontend/src/common/hooks/useEnhancedActivityDerivedState.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/common/updaters/orders/UnfillableOrdersUpdater/updaters/UnfillableOrderUpdater.tsapps/cowswap-frontend/src/common/updaters/orders/UnfillableOrdersUpdater/hooks/useUpdateIsUnfillableFlag.tsapps/cowswap-frontend/src/common/updaters/WidgetTokensUpdater.tsxapps/cowswap-frontend/src/common/updaters/orders/OrdersFromApiUpdater.tsapps/cowswap-frontend/src/common/updaters/BridgingEnabledUpdater.tsapps/cowswap-frontend/src/common/updaters/orders/PendingOrdersUpdater.tsapps/cowswap-frontend/src/common/updaters/orders/SpotPricesUpdater.ts
📚 Learning: 2025-08-08T13:55:17.528Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6125
File: libs/tokens/src/state/tokens/allTokensAtom.ts:78-78
Timestamp: 2025-08-08T13:55:17.528Z
Learning: In libs/tokens/src/state/tokens/allTokensAtom.ts (TypeScript/Jotai), the team prefers to wait for token lists to initialize (listsStatesListAtom non-empty) before returning tokens. No fallback to favorites/user-added/native tokens should be used when listsStatesList is empty.
Applied to files:
apps/cowswap-frontend/src/common/updaters/orders/UnfillableOrdersUpdater/updaters/UnfillableOrderUpdater.tsapps/cowswap-frontend/src/common/updaters/orders/UnfillableOrdersUpdater/hooks/useUpdateIsUnfillableFlag.tsapps/cowswap-frontend/src/common/updaters/orders/OrdersFromApiUpdater.tsapps/cowswap-frontend/src/common/updaters/orders/PendingOrdersUpdater.tsapps/cowswap-frontend/src/common/updaters/orders/SpotPricesUpdater.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/common/pure/CoWAmmBannerContent/index.tsx
📚 Learning: 2025-08-12T05:57:08.021Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6138
File: libs/hook-dapp-lib/src/hookDappsRegistry.ts:1-1
Timestamp: 2025-08-12T05:57:08.021Z
Learning: The matchHooksToDapps function in libs/hook-dapp-lib/src/utils.ts provides backward compatibility for permit hooks through function selector detection (EIP_2612_PERMIT_SELECTOR and DAI_PERMIT_SELECTOR) rather than dappId matching, making it robust against dappId changes.
Applied to files:
apps/cowswap-frontend/src/common/hooks/useOnSelectNetwork.tsxapps/cowswap-frontend/src/common/utils/doesOrderHavePermit.tsapps/cowswap-frontend/src/common/containers/OrderHooksDetails/HookItem/index.tsx
📚 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/common/updaters/WidgetTokensUpdater.tsxapps/cowswap-frontend/src/common/hooks/useGetSurplusFiatValue.tsapps/cowswap-frontend/src/common/hooks/useSwapAndBridgeContext.tsapps/cowswap-frontend/src/common/hooks/useSwapResultsContext.tsapps/cowswap-frontend/src/common/hooks/useSwapAndBridgeOverview.ts
📚 Learning: 2025-08-12T06:33:19.348Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6137
File: libs/tokens/src/state/tokens/allTokensAtom.ts:34-65
Timestamp: 2025-08-12T06:33:19.348Z
Learning: In libs/tokens/src/utils/parseTokenInfo.ts, the parseTokenInfo() function returns a new instance of TokenInfo using object spread syntax ({ ...token, ... }), making it safe to mutate properties like lpTokenProvider on the returned object without side effects.
Applied to files:
apps/cowswap-frontend/src/common/updaters/WidgetTokensUpdater.tsxapps/cowswap-frontend/src/common/containers/CoWAmmBanner/types.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/common/hooks/useGetSurplusFiatValue.tsapps/cowswap-frontend/src/common/hooks/useSwapResultsContext.tsapps/cowswap-frontend/src/common/hooks/useRateInfoParams.ts
📚 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/cowswap-frontend/src/common/hooks/useSwapAndBridgeContext.tsapps/cowswap-frontend/src/common/hooks/useSwapResultsContext.tsapps/cowswap-frontend/src/common/containers/OrderHooksDetails/index.tsxapps/cowswap-frontend/src/common/hooks/useSwapAndBridgeOverview.tsapps/cowswap-frontend/src/common/containers/OrderHooksDetails/HookItem/index.tsxapps/cowswap-frontend/src/common/hooks/useGetExecutedBridgeSummary.ts
📚 Learning: 2025-06-05T08:31:01.284Z
Learnt from: fairlighteth
Repo: cowprotocol/cowswap PR: 5782
File: apps/cow-fi/app/(learn)/learn/topics/page.tsx:1-1
Timestamp: 2025-06-05T08:31:01.284Z
Learning: Next.js App Router official documentation states that Client Components marked with 'use client' should NOT be declared as async functions, as this can lead to infinite loops or application hangs. The recommended pattern for async operations in Client Components is to use useEffect hooks. Server Components (without 'use client') can be async, which might be a source of confusion.
Applied to files:
apps/cowswap-frontend/src/common/updaters/BridgingEnabledUpdater.ts
📚 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/cowswap-frontend/src/common/updaters/orders/PendingOrdersUpdater.ts
📚 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/cowswap-frontend/src/common/containers/OrderHooksDetails/index.tsxapps/cowswap-frontend/src/common/containers/OrderHooksDetails/HookItem/index.tsx
📚 Learning: 2025-08-12T06:33:19.348Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 6137
File: libs/tokens/src/state/tokens/allTokensAtom.ts:34-65
Timestamp: 2025-08-12T06:33:19.348Z
Learning: In libs/tokens/src/state/tokens/allTokensAtom.ts, the parseTokenInfo() function returns a new instance of TokenInfo each time, making it safe to mutate properties like lpTokenProvider on the returned object without side effects.
Applied to files:
apps/cowswap-frontend/src/common/containers/CoWAmmBanner/types.ts
🔇 Additional comments (24)
apps/cowswap-frontend/src/common/updaters/orders/OrdersFromApiUpdater.ts (1)
1-1: ESLint disable directive correctly placed to suppress existing violations.The file-wide
eslint-disablecomment is appropriately positioned and will suppress the newno-restricted-importsrule for this file, which currently imports frommodules/(lines 14–15)—a violation of the documented architectural constraint thatcommon/must be leaf-only. The TODO comment clearly flags this as technical debt for future refactoring.This aligns with the PR's pragmatic approach to enforce the rule prospectively while deferring fixes for the ~33 existing violations across the codebase.
apps/cowswap-frontend/src/common/pure/ReceiveAmountInfo/index.tsx (1)
1-1: ESLint suppression matches new import restriction policyThe file‑level
no-restricted-importsdisable with a TODO is consistent with the new rule and safely preserves existingmodules/tradeimports until this component can be refactored off them.apps/cowswap-frontend/src/common/updaters/orders/UnfillableOrdersUpdater/updaters/UnfillableOrderUpdater.ts (1)
1-1: Lint-only change to grandfather existingmodulesimportThe added
no-restricted-importsdisable cleanly exempts this updater from the new rule while it still depends onmodules/orders, with no runtime impact.apps/cowswap-frontend/src/common/hooks/useConvertUsdToTokenValue.ts (1)
1-1: File-level exception for existingmodules-based hook is fineThis suppression correctly isolates the new
no-restricted-importsrule from the currentmodules/usdAmountdependency until it can be relocated.apps/cowswap-frontend/src/common/hooks/useGetMarketDimension.ts (1)
1-1: Temporary opt-out for trade-relatedmodulesimportsGiven the current reliance on
modules/tradehooks/types, this top-of-fileno-restricted-importsdisable is a reasonable intermediate step toward enforcing the common/ isolation rule.apps/cowswap-frontend/src/common/hooks/useCancelOrder/useGetOnChainCancellation.ts (1)
1-1: Grandfatheringmodules/twapusage under new lint ruleThe added
no-restricted-importsdisable correctly prevents new lint errors while preserving the existing on-chain cancellation flow.apps/cowswap-frontend/src/common/containers/CoWAmmBanner/types.ts (1)
1-1: Lint suppression for yieldmodulestype importThe file-level
no-restricted-importsdisable is appropriate here to keepPoolInfousable frommodules/yielduntil the CoWAmm banner types can be decoupled.apps/cowswap-frontend/src/common/hooks/useOnSelectNetwork.tsx (1)
1-1: Accepting file-wide exception for walletmodulesimportThis
no-restricted-importsdisable safely preservesmodules/walletusage inuseOnSelectNetworkunder the new ESLint policy, with no behavioral impact.apps/cowswap-frontend/src/common/hooks/useCancelOrder/useSendOnChainCancellation.ts (1)
1-1: ESLint opt-out for TWAPmodulesdependencyThe new
no-restricted-importsdisable is an appropriate temporary workaround souseSendOnChainCancellationcan keep usingmodules/twapwhile the architectural cleanup is pending.apps/cowswap-frontend/src/common/updaters/SentryUpdater.ts (1)
1-1: Appropriate lint suppression for existing violation.The file-wide suppression correctly covers the
modules/trade/hooks/useTradeStateimport on line 10. The TODO comment tracks this as technical debt to be addressed later.apps/cowswap-frontend/src/common/pure/CurrencyInputPanel/types.ts (1)
1-1: Appropriate lint suppression for existing violation.The suppression correctly covers the
modules/trade/typesimport on line 8. This type-only import is a good candidate for future refactoring—consider movingReceiveAmountInfoto a shared location to break the dependency.apps/cowswap-frontend/src/common/containers/OrderHooksDetails/HookItem/index.tsx (1)
1-1: Appropriate lint suppression for existing violation.The suppression correctly covers the
modules/tenderly/hooks/useSimulationDataimport on line 10. This component's dependency on the Tenderly module hook is the architectural violation to address in a future refactor.apps/cowswap-frontend/src/common/hooks/useSwapAndBridgeOverview.ts (1)
1-1: Appropriate lint suppression for existing violation.The suppression correctly covers the
modules/bridge/typesimport on line 11. The return typeSwapAndBridgeOverviewcomes from the bridge module, which is the architectural violation to address.apps/cowswap-frontend/src/common/pure/CoWAmmBannerContent/index.tsx (1)
1-1: Appropriate lint suppression for existing violation.The suppression correctly covers the
modules/yield/typesimport on line 13. TheVampireAttackContexttype dependency is the architectural violation to address in future work.apps/cowswap-frontend/src/common/hooks/useSwapAndBridgeContext.ts (1)
1-1: Appropriate lint suppression for existing violations.The suppression correctly covers the multiple
modules/bridge/typesimports (lines 18-25). This hook has significant coupling to the bridge module—six type imports in one block—making it a higher-priority candidate for refactoring when addressing this architectural debt.apps/cowswap-frontend/src/common/updaters/orders/SpotPricesUpdater.ts (1)
1-1: Appropriate lint suppression for existing violations.The suppression correctly covers the three module imports on lines 13-15 (
modules/orders/state/spotPricesAtom,modules/usdAmount, andmodules/usdAmount/hooks/useUsdPrice). This updater's coupling to multiple modules makes it a good candidate for future refactoring.libs/wallet-provider/.eslintrc.json (1)
2-2: Confirm that extending../../eslint.config.jsfrom.eslintrc.jsonbehaves as expectedPointing
"extends"at../../eslint.config.jsmakes sense given the repo-wide move, but depending on your ESLint version and whethereslint.config.jsis using the new flat-config format,.eslintrcfiles may be ignored or may not be able to extend it. Please double-check thatlibs/wallet-provideris actually picking up the intended rules (e.g., by running ESLint with--print-configon a file in this package).eslint.config.js (1)
229-249: Verify ESLint rule override impact oncommon/**directory restrictionsThe technical claim about ESLint flat config rule overrides is correct: later matching configs override earlier ones rather than merge rule options. However, I cannot verify whether this new
no-restricted-importsblock actually overrides earlier restrictions, as I cannot access the full config file to confirm whether earlierno-restricted-importsrules apply toapps/cowswap-frontend/src/common/**files.To complete this review:
- Check if earlier
no-restricted-importsrules exist (e.g., forapps/cowswap-frontend/**/*or**/*.{ts,tsx,js,jsx}) that should also apply tocommon/**files.- If they do, verify the intent: Is narrowing
common/**to only themodulesrestriction intentional, or should all prior restrictions (ethers, styled-components, etc.) still apply?- Confirm whether
'cowswap-frontend/modules'imports are actually used in the codebase or ifpatterns: ['modules/*']alone suffices.apps/cowswap-frontend/src/common/hooks/useRateInfoParams.ts (3)
16-19: Signature trailing comma is fineAdding a trailing comma to the second parameter is purely stylistic and keeps diffs cleaner; no behavioral change or typing issue here.
24-31:useCallbackdependencies remain correctThe callback only uses
activeRate, and the dependency array still includes justactiveRate. The added trailing comma is formatting only and does not affect memoization behavior.
33-39: Formatting-only change arounduseTradeUsdAmountsThe added trailing comma on the second argument is consistent with the rest of the file and doesn’t affect the call semantics. No issues spotted with the
tryParseCurrencyAmountusage in either argument.apps/cowswap-frontend/src/common/hooks/useCancelOrder/onChainCancellation.ts (3)
27-30: Trailing comma ingetEthFlowCancellationsignature is harmlessThe added comma after
order: Orderis a style-only change and does not alter the function type or its usage.
43-48: Trailing comma inlogTradeFlowErrorcall (ETH flow path)Only a formatting tweak: passing
erroras the last argument remains unchanged; the extra comma is syntactically valid and has no runtime impact.
72-77: Trailing comma inlogTradeFlowErrorcall (GPv2 path)Same as above: this is strictly a formatting change, and the error logging behavior is preserved.
shoom3301
left a comment
shoom3301
left a comment
There was a problem hiding this comment.
This is amazing! Thank you!
3 participants
Summary
I've checked your https://github.com/cowprotocol/cowswap/blob/develop/CONTRIBUTING.md file and it looks great!
And it says:
But, as I can see, a lot of
cowswap-frontend/commonfiles (33) do imports fromcowswap-frontend/modules.Therefore I wrote some eslint rules (
no-restricted-imports) to restrict such imports in future.To Test
cowswap-frontend/common/**.ts file (e.g.apps/cowswap-frontend/src/common/utils/fetch.ts)'modules'(e.g.import { useAccountModalState } from 'modules/account')'modules/account' import is restricted from being used by a pattern. eslint(no-restricted-imports).If you see an error, the rule works correctly.
Background
Also I added
eslint-disable no-restricted-importsmarks in current files (33) to ignore this eslint error for now.But rewrite those files would be better ofc. (is it a good idea?)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.