feat: grouped asset by primary asset #10380
Conversation
📝 WalkthroughWalkthroughImplements primary-asset grouping and related-asset support across search/lists/tables, adds grouped/expandable rows and long-press actions, migrates many UIs to a unified AssetList/AssetRow (showPrice/showRelatedAssets/handleLongPress), updates selectors and worker protocol to propagate related-asset mappings, and adjusts virtualization behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as AssetList / AssetRow
participant Store as Redux Selectors
participant Worker as assetSearch.worker
participant Lib as filterAssetsByChainSupport
User->>UI: Open/search assets
UI->>Store: selectPrimaryAssets + selectRelatedAssetIdsByAssetIdInclusive
Store-->>UI: assets + relatedAssetIdsById
UI->>Worker: postMessage(search, { query, activeChainId, walletConnectedChainIds })
Worker->>Lib: filterAssetsByChainSupport(assets, {activeChainId, walletConnectedChainIds, relatedAssetIdsById})
Lib-->>Worker: filtered assets (includes related-chain matches)
Worker-->>UI: search results
UI->>UI: Render AssetRow or GroupedAssetRow
User->>UI: tap -> expand row (show per-chain balances)
User->>UI: long-press -> open asset actions
sequenceDiagram
autonumber
participant Table as Markets/Accounts Table
participant Select as Selectors
participant MD as MarketDataFetcher
Table->>Select: request visible asset ids (onVisibleRowsChange)
Select-->>Table: asset ids
Table->>MD: fetch market data for visible asset ids
MD-->>Table: prices & change
Table->>Table: render rows (expandable grouped rows if applicable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -65,6 +71,10 @@ const buildRowTitle = (asset: Asset, postFix?: string, showAssetSymbol?: boolean | |||
| return `${asset.name} ${postFix}` | |||
| } | |||
|
|
|||
| if (isGrouped) { | |||
| return asset.name.split(' on ')[0] ?? asset.name | |||
There was a problem hiding this comment.
If it's grouped, AssetCell is used only at the level 1 asset so we effectively know we need to display the name without chain
I couldn't find any different heuristic without a big lift for this as we would need to query all related assets and get the one without the chain in the name
There was a problem hiding this comment.
Don't love this but agree it's probably the best we've got without major changes. Potentially we could run a common prefix function over the related asset names? Not sure if that's going to be more or less reliable
There was a problem hiding this comment.
I might prefer keeping this a display time, running the function would mutate the assets, I don't really mind, not sure it will change anything considering we might not touch this part for a long time if it works as expected
There was a problem hiding this comment.
Don't love this but agree it's probably the best we've got without major changes. Potentially we could run a common prefix function over the related asset names? Not sure if that's going to be more or less reliable
Great minds @premiumjibles - may or may not be worth doing in this PR, but as mentioned in #10380 (comment), could do something where we keep the asset names without related name x on y as name, and then adding an additional composite name when upserting in the store, this way we could leverage both as-needed (may be a huge lift to tackle across the board and could bring regressions though, so happy to have this or similar in a follow-up, agree hacky approach isn't too bad to minimise already huge diff)
Or yeah alternatively run composite method as-needed! Feels like we'll end up with more and more places where the name with network may not make sense anymore, as we generalise the concept of primary/related assets and representing an asset as a single entity across the app.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/Explore/Explore.tsx (1)
86-93: Add missing useFeatureFlag importAdd the following to
src/pages/Explore/Explore.tsxwith the other imports to restore the build:import { useAppSelector } from '@/state/store' +import { useFeatureFlag } from '@/hooks/useFeatureFlag/useFeatureFlag'src/components/AssetSearch/components/AssetRow.tsx (1)
200-210: Don't disable custom-asset rows — otherwise Import becomes unreachableIf
disableUnsupportedis true and no wallet is connected or the wallet can't support the asset, the entire row becomes disabled, which also disables the Import button for custom assets. Gate the disabled state on non-custom assets.Apply:
- isDisabled={!isSupported && disableUnsupported} + isDisabled={!isCustomAsset && disableUnsupported && !isSupported}
♻️ Duplicate comments (1)
src/components/StakingVaults/Cells.tsx (1)
56-62: Make grouped title stripping safer (avoid false positives).
Current split(' on ') will truncate legitimate names containing " on ". Prefer stripping only a trailing " on " suffix.-const buildRowTitle = ( - asset: Asset, - postFix?: string, - showAssetSymbol?: boolean, - isGrouped?: boolean, -): string => { +const buildRowTitle = ( + asset: Asset, + postFix?: string, + showAssetSymbol?: boolean, + isGrouped?: boolean, +): string => { … - if (isGrouped) { - return asset.name.split(' on ')[0] ?? asset.name - } + if (isGrouped) { + // strip only a trailing " on <Chain>" pattern + const m = asset.name.match(/^(.*)\s+on\s+[^()]+$/) + return m ? m[1] : asset.name + } return asset.name }If this heuristic was previously discussed and accepted as-is, ignore and punt to a follow-up.
Also applies to: 74-77
🧹 Nitpick comments (18)
src/pages/Explore/components/PortalAssetRow.tsx (2)
26-33: Remove redundant portalsAssets prop from PortalAssetRowProps.
You’re sourcingportalsAssetsfromdata, so this top-level prop is dead and may confuse future readers.-type PortalAssetRowProps = { +type PortalAssetRowProps = { asset: Asset index: number data: AssetData showNetworkIcon?: boolean - portalsAssets?: PortalsAssets } & ButtonProps
101-101: Double-check default for showNetworkIcon.
If Explore should show chain context by default, consider defaulting totruewhen prop is undefined.-<AssetIcon assetId={asset.assetId} size='sm' showNetworkIcon={showNetworkIcon} /> +<AssetIcon assetId={asset.assetId} size='sm' showNetworkIcon={showNetworkIcon ?? true} />src/components/ReactTable/InfiniteTable.tsx (2)
221-223: Avoid custom tab order with row.index.
Setting tabIndex to an arbitrary index can create confusing navigation. Prefer 0 (focusable in DOM order) or -1.- tabIndex={row?.index} + tabIndex={0}
227-236: Guard data-test prefix to avoid “undefined-…” attributes.
Only set data-test when both key and prefix are provided.- {...(row && rowDataTestKey + {...(row && rowDataTestKey && rowDataTestPrefix ? { 'data-test': `${rowDataTestPrefix}-${String( row.original?.[rowDataTestKey] ?? '', ) .split(' ') .join('-') .toLowerCase()}`, } : {})}src/pages/Explore/components/CategoryCard.tsx (1)
175-193: Avoid leaking unknown prop to DOM: drop top-level portalsAssets on PortalAssetRowPortalAssetRow consumes portalsAssets via its data prop; passing portalsAssets at the top level will be forwarded to the underlying Button and may cause “unknown DOM prop” warnings.
Apply:
{filteredAssets.map((asset, index) => ( <PortalAssetRow key={asset.assetId} asset={asset} data={assetSearchRowData} index={index} // We are not virtualizing so we don't use this prop but reuse the component for simplicity/reusability style={emptyStyle} color={assetTitleColor} showNetworkIcon={false} - portalsAssets={portalsAssets} /> ))}src/state/slices/portfolioSlice/selectors.ts (1)
739-741: Document new grouping fields for downstream consumersAdd brief JSDoc on isGrouped and relatedAssetIds to clarify semantics (e.g., when set, ordering, and whether relatedAssetIds includes self).
Example:
export type AccountRowData = { name: string icon: string | undefined symbol: string fiatAmount: string cryptoAmount: string assetId: AssetId allocation: number price: string priceChange: number - isGrouped?: boolean - relatedAssetIds?: string[] + /** True when this row represents an aggregate across multiple chains for the same primary asset */ + isGrouped?: boolean + /** Ordered list of related per-chain AssetIds composing the group (exclude the primary if not needed) */ + relatedAssetIds?: string[] }src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx (2)
253-266: Guard grouped mode to avoid collapsing results when relatedAssetKey isn’t present.AssetList groups by asset.relatedAssetKey when shouldDisplayRelatedAssets is true. If results lack this key, grouping may collapse to a single item.
Apply a conditional enablement:
- <AssetList - assets={filteredAssets} - handleClick={handleRowClick} - handleLongPress={handleRowLongPress} - portalsAssets={portalsAssets} - rowComponent={ - selectedCategory === MarketsCategories.OneClickDefi ? PortalAssetRow : undefined - } - disableUnsupported={false} - height='100vh' - showPrice - shouldDisplayRelatedAssets - /> + <AssetList + assets={filteredAssets} + handleClick={handleRowClick} + handleLongPress={handleRowLongPress} + portalsAssets={portalsAssets} + rowComponent={ + selectedCategory === MarketsCategories.OneClickDefi ? PortalAssetRow : undefined + } + disableUnsupported={false} + height='100vh' + showPrice + shouldDisplayRelatedAssets={Boolean(filteredAssets?.some(a => (a as any)?.relatedAssetKey))} + />Additionally, there appears to be a typo in AssetList: renderRow uses
row_componentinstead ofrowComponent. If present in your branch, fix it or runtime will fail:- const renderRow = useCallback( - (index: number) => { - const asset = uniqueAssets[index] - const RowComponent = row_component + const renderRow = useCallback( + (index: number) => { + const asset = uniqueAssets[index] + const RowComponent = rowComponent ... - [uniqueAssets, itemData, row_component, showPrice, onImportClick, shouldDisplayRelatedAssets], + [uniqueAssets, itemData, rowComponent, showPrice, onImportClick, shouldDisplayRelatedAssets], )
162-178: setBuyAsset payload type is correct; Mixpanel prop rename recommended
ThesetBuyAssetaction is defined as(state, action: PayloadAction<Asset>), so passing the fullAssetobject is correct. For clarity in analytics, rename the Mixpanel property key fromassettoassetSymbolwhen sendingasset.symbol.src/pages/Dashboard/components/AccountList/GroupedAssetsSubComponent.tsx (3)
36-40: Avoid forging react-table Row; confirm consumer expectations.You’re constructing
{ original: relatedRow } as Row<AccountRowData>. If downstream code accesses other Row props (id, values, index), this will break.Prefer changing the callback types to accept
AccountRowData(or introduce a lightweightRowLike), or ensure consumers only ever read.original.
21-22: Use AssetId type for clarity.Prop uses
string; preferAssetIdfor stronger typing.-import type { Row } from 'react-table' +import type { Row } from 'react-table' +import type { AssetId } from '@shapeshiftoss/caip' ... -type RelatedAssetRowProps = { - assetId: string +type RelatedAssetRowProps = { + assetId: AssetId
60-63: Truncation is fine; consider consistency.Six-char truncation is consistent; if UX requests longer symbols later, wire a constant.
Also applies to: 79-81
src/components/Layout/Header/GlobalSearch/AssetResults/AssetResults.tsx (1)
16-21: Guard grouped mode in search results; consider memo.
- If results lack relatedAssetKey, enabling grouping may collapse to one item.
- Minor: wrap the component in memo as per project guidance.
Apply:
-export const AssetResults: React.FC<AssetResultsProps> = ({ results, onClick, searchQuery }) => { +export const AssetResults: React.FC<AssetResultsProps> = ({ results, onClick, searchQuery }) => { const translate = useTranslate() const renderItems = useMemo(() => { - return <AssetList assets={results} handleClick={onClick} shouldDisplayRelatedAssets /> + const shouldGroup = Boolean(results?.some(a => (a as any)?.relatedAssetKey)) + return ( + <AssetList + assets={results} + handleClick={onClick} + shouldDisplayRelatedAssets={shouldGroup} + /> + ) }, [results, onClick])Optionally wrap with memo:
-import { useMemo } from 'react' +import { memo, useMemo } from 'react' ... -export const AssetResults: React.FC<AssetResultsProps> = ({ results, onClick, searchQuery }) => { +export const AssetResults: React.FC<AssetResultsProps> = memo( +({ results, onClick, searchQuery }) => { ... -} +})src/pages/Dashboard/components/AccountList/AccountTable.tsx (1)
70-101: Prefer using relatedAssetKey for grouping instead of parsing namesSplitting
asset.nameon' on 'is brittle. If available, grouping byrelatedAssetKey(or falling back toassetId) will align the dashboard with AssetList/AssetRow grouping.Sketch:
- const grouped = groupBy(data, row => { - const asset = assets[row.assetId] - if (!asset) return row.assetId - const baseAssetName = asset.name.split(' on ')[0] - return `${baseAssetName}_${asset.symbol}` - }) + const grouped = groupBy(data, row => { + const asset = assets[row.assetId] + return asset?.relatedAssetKey ?? row.assetId + })src/components/AssetSearch/components/GroupedAssetRow.tsx (5)
109-115: Memoize static row data onceAvoid recreating the data object per row to reduce renders; you already disabled the lint rule below—no need if we memoize once.
const handleAssetClick = useCallback( (asset: Asset) => { handleClick(asset) }, [handleClick], ) + + const assetRowData = useMemo( + () => ({ + handleClick: handleAssetClick, + disableUnsupported, + hideZeroBalanceAmounts, + }), + [handleAssetClick, disableUnsupported, hideZeroBalanceAmounts], + )
254-269: Use memoized data; drop unused propsindex={0} looks vestigial and passing assets in data doesn’t match AssetRow’s usage. Use the memoized data and remove the eslint disable.
- {relatedAssets.map(asset => ( + {relatedAssets.map(asset => ( <AssetRow key={asset.assetId} asset={asset} - index={0} py={8} - // eslint-disable-next-line react-memo/require-usememo - data={{ - assets: [asset], - handleClick: handleAssetClick, - disableUnsupported, - hideZeroBalanceAmounts, - }} + data={assetRowData} /> ))}
46-55: Stabilize memo dependenciesFilter depends only on asset.assetId. Depending on the whole asset object may cause unnecessary recalcs.
- const relatedAssetIdsFilter = useMemo( - () => ({ - assetId: asset.assetId, - // We want all related assetIds, and conditionally mark the disconnected/unsupported ones as - // disabled in the UI. This allows users to see our product supports more assets than they - // have connected chains for. - onlyConnectedChains: false, - }), - [asset], - ) + const relatedAssetIdsFilter = useMemo( + () => ({ + assetId: asset.assetId, + // Show all related assetIds regardless of connection state. + onlyConnectedChains: false, + }), + [asset.assetId], + )
67-75: Naming nit: variable suggests the oppositeassetWithoutChainSuffix actually matches a name including ' on ' or the fee asset. Consider primaryAssetCandidate for clarity.
- const primaryAsset = useMemo(() => { - const assetWithoutChainSuffix = relatedAssets.find( + const primaryAsset = useMemo(() => { + const primaryAssetCandidate = relatedAssets.find( relatedAsset => relatedAsset?.name.includes(' on ') || feeAsset?.assetId === relatedAsset?.assetId, ) - return assetWithoutChainSuffix || asset + return primaryAssetCandidate || asset }, [relatedAssets, feeAsset, asset])
171-185: Minor a11y: expose state to assistive techConsider adding aria-expanded and aria-controls linking the Button to the Collapse panel. Not blocking—can be a follow-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
package.json(0 hunks)src/components/AssetSearch/AssetSearch.tsx(2 hunks)src/components/AssetSearch/components/AssetList.tsx(6 hunks)src/components/AssetSearch/components/AssetRow.tsx(6 hunks)src/components/AssetSearch/components/GroupedAssetRow.tsx(1 hunks)src/components/Layout/Header/GlobalSearch/AssetResults/AssetResult.tsx(0 hunks)src/components/Layout/Header/GlobalSearch/AssetResults/AssetResults.tsx(1 hunks)src/components/Layout/Header/GlobalSearch/AssetSearchResults.tsx(2 hunks)src/components/Layout/Header/GlobalSearch/GlobalSearchModal.tsx(0 hunks)src/components/Modals/AssetSearch/AssetSearchModal.tsx(2 hunks)src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx(5 hunks)src/components/ReactTable/InfiniteTable.tsx(4 hunks)src/components/StakingVaults/Cells.tsx(6 hunks)src/components/Table/Table.theme.ts(3 hunks)src/components/TradeAssetSearch/components/DefaultAssetList.tsx(2 hunks)src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsx(3 hunks)src/components/TradeAssetSearch/components/GroupedAssetList/components/GroupedAssetRow.tsx(0 hunks)src/components/TradeAssetSearch/components/SearchTermAssetList.tsx(3 hunks)src/pages/Dashboard/components/AccountList/AccountTable.tsx(8 hunks)src/pages/Dashboard/components/AccountList/GroupedAssetsSubComponent.tsx(1 hunks)src/pages/Explore/Explore.tsx(1 hunks)src/pages/Explore/ExploreCategory.tsx(2 hunks)src/pages/Explore/components/CategoryCard.tsx(3 hunks)src/pages/Explore/components/PortalAssetRow.tsx(3 hunks)src/state/slices/portfolioSlice/selectors.ts(3 hunks)
💤 Files with no reviewable changes (4)
- package.json
- src/components/TradeAssetSearch/components/GroupedAssetList/components/GroupedAssetRow.tsx
- src/components/Layout/Header/GlobalSearch/GlobalSearchModal.tsx
- src/components/Layout/Header/GlobalSearch/AssetResults/AssetResult.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
src/components/AssetSearch/AssetSearch.tsxsrc/components/AssetSearch/components/GroupedAssetRow.tsxsrc/components/Layout/Header/GlobalSearch/AssetSearchResults.tsxsrc/pages/Explore/components/CategoryCard.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/pages/Dashboard/components/AccountList/GroupedAssetsSubComponent.tsxsrc/components/AssetSearch/components/AssetRow.tsxsrc/state/slices/portfolioSlice/selectors.tssrc/pages/Explore/ExploreCategory.tsxsrc/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsxsrc/components/Table/Table.theme.tssrc/components/Layout/Header/GlobalSearch/AssetResults/AssetResults.tsxsrc/pages/Explore/Explore.tsxsrc/components/TradeAssetSearch/components/DefaultAssetList.tsxsrc/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsxsrc/pages/Explore/components/PortalAssetRow.tsxsrc/components/Modals/AssetSearch/AssetSearchModal.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/components/StakingVaults/Cells.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.tsx: ALWAYS wrap components in error boundaries
ALWAYS provide user-friendly fallback components in error boundaries
ALWAYS log errors for debugging in error boundaries
ALWAYS use useErrorToast hook for displaying errors
ALWAYS provide translated error messages in error toasts
ALWAYS handle different error types appropriately in error toasts
Missing error boundaries in React components is an anti-pattern
**/*.tsx: ALWAYS use PascalCase for React component names
ALWAYS use descriptive names that indicate the component's purpose
ALWAYS match the component name to the file name
Flag components without PascalCase
Flag default exports for components
Files:
src/components/AssetSearch/AssetSearch.tsxsrc/components/AssetSearch/components/GroupedAssetRow.tsxsrc/components/Layout/Header/GlobalSearch/AssetSearchResults.tsxsrc/pages/Explore/components/CategoryCard.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/pages/Dashboard/components/AccountList/GroupedAssetsSubComponent.tsxsrc/components/AssetSearch/components/AssetRow.tsxsrc/pages/Explore/ExploreCategory.tsxsrc/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsxsrc/components/Layout/Header/GlobalSearch/AssetResults/AssetResults.tsxsrc/pages/Explore/Explore.tsxsrc/components/TradeAssetSearch/components/DefaultAssetList.tsxsrc/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsxsrc/pages/Explore/components/PortalAssetRow.tsxsrc/components/Modals/AssetSearch/AssetSearchModal.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/components/StakingVaults/Cells.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
src/components/AssetSearch/AssetSearch.tsxsrc/components/AssetSearch/components/GroupedAssetRow.tsxsrc/components/Layout/Header/GlobalSearch/AssetSearchResults.tsxsrc/pages/Explore/components/CategoryCard.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/pages/Dashboard/components/AccountList/GroupedAssetsSubComponent.tsxsrc/components/AssetSearch/components/AssetRow.tsxsrc/state/slices/portfolioSlice/selectors.tssrc/pages/Explore/ExploreCategory.tsxsrc/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsxsrc/components/Table/Table.theme.tssrc/components/Layout/Header/GlobalSearch/AssetResults/AssetResults.tsxsrc/pages/Explore/Explore.tsxsrc/components/TradeAssetSearch/components/DefaultAssetList.tsxsrc/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsxsrc/pages/Explore/components/PortalAssetRow.tsxsrc/components/Modals/AssetSearch/AssetSearchModal.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/components/StakingVaults/Cells.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
**/*.{tsx,jsx}: ALWAYS useuseMemofor expensive computations, object/array creations, and filtered data
ALWAYS useuseMemofor derived values and computed properties
ALWAYS useuseMemofor conditional values and simple transformations
ALWAYS useuseCallbackfor event handlers and functions passed as props
ALWAYS useuseCallbackfor any function that could be passed as a prop or dependency
ALWAYS include all dependencies inuseEffect,useMemo,useCallbackdependency arrays
NEVER use// eslint-disable-next-line react-hooks/exhaustive-depsunless absolutely necessary
ALWAYS explain why dependencies are excluded if using eslint disable
ALWAYS use named exports for components
NEVER use default exports for components
KEEP component files under 200 lines when possible
BREAK DOWN large components into smaller, reusable pieces
EXTRACT complex logic into custom hooks
USE local state for component-level state
LIFT state up when needed across multiple components
USE Context for avoiding prop drilling
ALWAYS wrap components in error boundaries for production
ALWAYS handle async errors properly
ALWAYS provide user-friendly error messages
ALWAYS use virtualization for lists with 100+ items
ALWAYS implement proper key props for list items
ALWAYS lazy load heavy components
ALWAYS use React.lazy for code splitting
Components receiving props are wrapped withmemo
Files:
src/components/AssetSearch/AssetSearch.tsxsrc/components/AssetSearch/components/GroupedAssetRow.tsxsrc/components/Layout/Header/GlobalSearch/AssetSearchResults.tsxsrc/pages/Explore/components/CategoryCard.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/pages/Dashboard/components/AccountList/GroupedAssetsSubComponent.tsxsrc/components/AssetSearch/components/AssetRow.tsxsrc/pages/Explore/ExploreCategory.tsxsrc/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsxsrc/components/Layout/Header/GlobalSearch/AssetResults/AssetResults.tsxsrc/pages/Explore/Explore.tsxsrc/components/TradeAssetSearch/components/DefaultAssetList.tsxsrc/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsxsrc/pages/Explore/components/PortalAssetRow.tsxsrc/components/Modals/AssetSearch/AssetSearchModal.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/components/StakingVaults/Cells.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
src/components/AssetSearch/AssetSearch.tsxsrc/components/AssetSearch/components/GroupedAssetRow.tsxsrc/components/Layout/Header/GlobalSearch/AssetSearchResults.tsxsrc/pages/Explore/components/CategoryCard.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/pages/Dashboard/components/AccountList/GroupedAssetsSubComponent.tsxsrc/components/AssetSearch/components/AssetRow.tsxsrc/state/slices/portfolioSlice/selectors.tssrc/pages/Explore/ExploreCategory.tsxsrc/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsxsrc/components/Table/Table.theme.tssrc/components/Layout/Header/GlobalSearch/AssetResults/AssetResults.tsxsrc/pages/Explore/Explore.tsxsrc/components/TradeAssetSearch/components/DefaultAssetList.tsxsrc/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsxsrc/pages/Explore/components/PortalAssetRow.tsxsrc/components/Modals/AssetSearch/AssetSearchModal.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/components/StakingVaults/Cells.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
🧠 Learnings (17)
📓 Common learnings
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
📚 Learning: 2025-08-08T15:00:49.887Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Applied to files:
src/components/Layout/Header/GlobalSearch/AssetSearchResults.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/AssetSearch/components/AssetRow.tsxsrc/components/Layout/Header/GlobalSearch/AssetResults/AssetResults.tsxsrc/pages/Explore/Explore.tsxsrc/components/TradeAssetSearch/components/DefaultAssetList.tsxsrc/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsxsrc/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-08-08T14:59:40.422Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/pages/Explore/ExploreCategory.tsx:231-238
Timestamp: 2025-08-08T14:59:40.422Z
Learning: In src/pages/Explore/ExploreCategory.tsx, for the PageHeader filter trigger, NeOMakinG considers changing a clickable Chakra Icon to IconButton too nitpicky for this PR and prefers to keep the current Icon-based trigger; such minor a11y/UI nitpicks should be deferred to a follow-up if needed.
Applied to files:
src/pages/Explore/components/CategoryCard.tsxsrc/pages/Explore/ExploreCategory.tsxsrc/pages/Explore/Explore.tsxsrc/pages/Explore/components/PortalAssetRow.tsx
📚 Learning: 2025-08-06T11:02:09.501Z
Learnt from: NeOMakinG
PR: shapeshift/web#10191
File: src/pages/Explore/Explore.tsx:54-72
Timestamp: 2025-08-06T11:02:09.501Z
Learning: In src/pages/Explore/Explore.tsx, NeOMakinG indicated that memoizing the ExploreCard component would be unnecessary, questioning "why would we memoize that", suggesting that like CategoryCard, this component doesn't benefit from memoization despite receiving props, likely due to its specific usage patterns in the carousel with static content.
Applied to files:
src/pages/Explore/components/CategoryCard.tsx
📚 Learning: 2025-08-06T09:46:50.860Z
Learnt from: NeOMakinG
PR: shapeshift/web#10191
File: src/pages/Explore/components/CategoryCard.tsx:27-196
Timestamp: 2025-08-06T09:46:50.860Z
Learning: In src/pages/Explore/components/CategoryCard.tsx, NeOMakinG indicated that wrapping the CategoryCard component with React.memo would be "quite useless in this case", suggesting that memoization is not beneficial for this particular component despite it receiving props, likely due to its specific usage patterns or context.
Applied to files:
src/pages/Explore/components/CategoryCard.tsx
📚 Learning: 2025-08-05T23:36:13.214Z
Learnt from: premiumjibles
PR: shapeshift/web#10187
File: src/state/slices/preferencesSlice/selectors.ts:21-25
Timestamp: 2025-08-05T23:36:13.214Z
Learning: The AssetId type from 'shapeshiftoss/caip' package is a string type alias, so it can be used directly as a return type for cache key resolvers in re-reselect selectors without needing explicit string conversion.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-05T22:41:35.473Z
Learnt from: premiumjibles
PR: shapeshift/web#10187
File: src/pages/Assets/Asset.tsx:1-1
Timestamp: 2025-08-05T22:41:35.473Z
Learning: In the shapeshift/web codebase, component imports use direct file paths like '@/components/ComponentName/ComponentName' rather than barrel exports. The AssetAccountDetails component should be imported as '@/components/AssetAccountDetails/AssetAccountDetails', not from a directory index.
Applied to files:
src/components/AssetSearch/components/AssetRow.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts accepts a filter object with an assetId property (signature: (state, { assetId })), not a raw AssetId string. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/AssetSearch/components/AssetRow.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts accepts a filter object with an assetId property, not a raw AssetId string. The selector signature is (state: ReduxState, filter) where filter is expected to have an assetId property. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/AssetSearch/components/AssetRow.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts expects a filter object with an assetId property, not a raw AssetId string. The selector signature is (state: ReduxState, filter) where filter should have an assetId property. This pattern is consistent across portfolio selectors that use selectAssetIdParamFromFilter. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/AssetSearch/components/AssetRow.tsxsrc/state/slices/portfolioSlice/selectors.ts
📚 Learning: 2025-08-03T22:10:38.426Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/react-best-practices.mdc:0-0
Timestamp: 2025-08-03T22:10:38.426Z
Learning: Applies to **/*.{tsx,jsx} : ALWAYS use virtualization for lists with 100+ items
Applied to files:
src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsxsrc/components/ReactTable/InfiniteTable.tsx
📚 Learning: 2025-07-30T20:57:48.176Z
Learnt from: NeOMakinG
PR: shapeshift/web#10148
File: src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx:88-91
Timestamp: 2025-07-30T20:57:48.176Z
Learning: In src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx, NeOMakinG prefers keeping the hardcoded overscan value (40) over dynamic calculation based on viewport height, prioritizing code simplicity over precision when the current approach works effectively.
Applied to files:
src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsx
📚 Learning: 2025-08-06T09:47:29.865Z
Learnt from: NeOMakinG
PR: shapeshift/web#10191
File: src/pages/Explore/Explore.tsx:174-178
Timestamp: 2025-08-06T09:47:29.865Z
Learning: In src/pages/Explore/Explore.tsx, NeOMakinG prefers keeping complex height calculations inline rather than extracting them to useMemo, prioritizing perceived performance/execution speed over code readability, even when the calculation would only run when the dependency (isSearching) changes.
Applied to files:
src/pages/Explore/Explore.tsx
📚 Learning: 2025-08-14T17:54:32.563Z
Learnt from: gomesalexandre
PR: shapeshift/web#10276
File: src/pages/ThorChainLP/components/ReusableLpStatus/ReusableLpStatus.tsx:97-108
Timestamp: 2025-08-14T17:54:32.563Z
Learning: In ReusableLpStatus component (src/pages/ThorChainLP/components/ReusableLpStatus/ReusableLpStatus.tsx), the txAssets dependency is stable from first render because poolAsset, baseAsset, actionSide, and action are all defined first render, making the current txAssetsStatuses initialization pattern safe without needing useEffect synchronization.
Applied to files:
src/components/TradeAssetSearch/components/DefaultAssetList.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-08-10T21:09:25.643Z
Learnt from: premiumjibles
PR: shapeshift/web#10215
File: src/components/MultiHopTrade/hooks/useGetTradeRateInput.ts:65-67
Timestamp: 2025-08-10T21:09:25.643Z
Learning: In the MultiHopTrade components, `selectInputBuyAsset` and `selectInputSellAsset` selectors from `tradeInputSlice` always return defined values because they have default values in the initial state (BTC for buyAsset, ETH for sellAsset, with fallback to defaultAsset). Null checks for these assets are unnecessary when using these selectors.
Applied to files:
src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx
📚 Learning: 2025-08-08T11:40:55.734Z
Learnt from: NeOMakinG
PR: shapeshift/web#10234
File: src/components/MultiHopTrade/components/TradeConfirm/TradeConfirm.tsx:41-41
Timestamp: 2025-08-08T11:40:55.734Z
Learning: In MultiHopTrade confirm flow (src/components/MultiHopTrade/components/TradeConfirm/TradeConfirm.tsx and related hooks), there is only one active trade per flow. Because of this, persistent (module/Redux) dedupe for QuotesReceived in useTrackTradeQuotes is not necessary; the existing ref-based dedupe is acceptable.
Applied to files:
src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx
📚 Learning: 2025-08-10T22:06:16.818Z
Learnt from: premiumjibles
PR: shapeshift/web#10216
File: src/pages/Dashboard/components/AccountList/AccountTable.tsx:183-189
Timestamp: 2025-08-10T22:06:16.818Z
Learning: In the ShapeShift web codebase, haptic feedback for long press interactions (via `pulseAndroid()`) is handled centrally within the table components (`InfiniteTable`, `ReactTable`, `ReactTableNoPager`, `MarketsTableVirtualized`) using the `useLongPress` hook. Parent components that pass `onRowLongPress` callbacks should not add their own haptic feedback calls as this would cause duplicate vibrations.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
🧬 Code graph analysis (16)
src/components/AssetSearch/components/GroupedAssetRow.tsx (10)
src/state/slices/assetsSlice/assetsSlice.ts (1)
assets(42-64)src/state/store.ts (3)
useAppSelector(143-143)useSelectorWithArgs(144-147)store(137-137)src/state/slices/assetsSlice/selectors.ts (3)
selectAssets(39-42)selectFeeAssetById(64-71)selectFeeAssetByChainId(58-62)src/state/slices/related-assets-selectors.ts (1)
selectRelatedAssetIdsInclusiveSorted(70-91)src/state/slices/marketDataSlice/selectors.ts (1)
selectMarketDataByAssetIdUserCurrency(66-72)src/state/slices/portfolioSlice/selectors.ts (1)
selectPortfolioUserCurrencyBalanceByAssetId(199-209)src/state/slices/common-selectors.ts (1)
selectPortfolioCryptoPrecisionBalanceByFilter(119-134)src/components/LazyLoadAvatar.tsx (1)
LazyLoadAvatar(10-45)src/components/AssetIcon.tsx (1)
AssetIcon(112-172)src/components/AssetSearch/components/AssetRow.tsx (1)
AssetRow(41-239)
src/components/Layout/Header/GlobalSearch/AssetSearchResults.tsx (1)
src/components/Layout/Header/GlobalSearch/AssetResults/AssetResults.tsx (1)
AssetResults(16-29)
src/pages/Explore/components/CategoryCard.tsx (2)
src/pages/Explore/components/PortalAssetRow.tsx (1)
PortalAssetRow(34-139)src/components/AssetSearch/components/AssetRow.tsx (1)
AssetRow(41-239)
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx (2)
src/components/Text/Text.tsx (1)
Text(19-83)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(43-164)
src/pages/Dashboard/components/AccountList/GroupedAssetsSubComponent.tsx (4)
src/state/slices/portfolioSlice/selectors.ts (2)
AccountRowData(729-741)selectPortfolioAccountRows(743-792)src/lib/vibrate.ts (1)
vibrate(6-16)src/constants/longPress.ts (1)
defaultLongPressConfig(3-3)src/components/StakingVaults/Cells.tsx (1)
AssetCell(81-177)
src/components/AssetSearch/components/AssetRow.tsx (7)
src/components/AssetSearch/components/AssetList.tsx (1)
AssetData(16-32)src/state/store.ts (1)
useAppSelector(143-143)src/state/slices/assetsSlice/selectors.ts (1)
selectAssetById(16-20)src/state/slices/marketDataSlice/selectors.ts (1)
selectMarketDataByAssetIdUserCurrency(66-72)src/lib/math.ts (1)
firstNonZeroDecimal(36-38)src/components/AssetSearch/components/GroupedAssetRow.tsx (1)
GroupedAssetRow(37-273)src/components/AssetIcon.tsx (1)
AssetIcon(112-172)
src/pages/Explore/ExploreCategory.tsx (1)
src/pages/Explore/components/PortalAssetRow.tsx (1)
PortalAssetRow(34-139)
src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsx (1)
src/components/AssetSearch/components/AssetList.tsx (2)
AssetList(43-164)INCREASE_VIEWPORT_BY(41-41)
src/components/Layout/Header/GlobalSearch/AssetResults/AssetResults.tsx (1)
src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(43-164)
src/components/TradeAssetSearch/components/DefaultAssetList.tsx (2)
src/components/Text/Text.tsx (1)
Text(19-83)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(43-164)
src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx (2)
src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(43-164)src/pages/Explore/components/PortalAssetRow.tsx (1)
PortalAssetRow(34-139)
src/pages/Explore/components/PortalAssetRow.tsx (2)
src/components/AssetSearch/components/AssetList.tsx (1)
AssetData(16-32)src/pages/Markets/hooks/usePortalsAssetsQuery.ts (1)
PortalsAssets(15-19)
src/components/AssetSearch/components/AssetList.tsx (1)
src/pages/Markets/hooks/usePortalsAssetsQuery.ts (1)
PortalsAssets(15-19)
src/components/StakingVaults/Cells.tsx (1)
src/components/AssetIcon.tsx (1)
AssetIcon(112-172)
src/pages/Dashboard/components/AccountList/AccountTable.tsx (6)
src/state/slices/assetsSlice/selectors.ts (1)
selectAssets(39-42)src/hooks/useInfiniteScroll/useInfiniteScroll.tsx (1)
useInfiniteScroll(11-49)src/state/slices/portfolioSlice/selectors.ts (2)
RowProps(727-727)AccountRowData(729-741)src/pages/Dashboard/components/AccountList/GroupedAssetsSubComponent.tsx (1)
GroupedAssetsSubComponent(93-111)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(43-164)src/components/ReactTable/InfiniteTable.tsx (1)
InfiniteTable(46-299)
src/components/ReactTable/InfiniteTable.tsx (2)
src/lib/vibrate.ts (1)
vibrate(6-16)src/constants/longPress.ts (2)
defaultLongPressConfig(3-3)longPressSx(5-14)
🪛 Biome (2.1.2)
src/components/ReactTable/InfiniteTable.tsx
[error] 158-165: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ 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). (1)
- GitHub Check: Call / Static
🔇 Additional comments (34)
src/components/StakingVaults/Cells.tsx (3)
44-45: Prop addition looks good; naming matches conventions.
isGrouped?: booleanis clear and consistent with boolean naming guidelines.
108-109: Row title derivation wiring LGTM.
PropagatingisGroupedintobuildRowTitlekeeps logic centralized.
127-128: Correctly hiding the network icon in grouped mode.
showNetworkIcon={!isGrouped}matches the grouped design.src/pages/Explore/components/PortalAssetRow.tsx (2)
34-39: Rename/export LGTM.
Public API shift to PortalAssetRow is clean and backwards-compat handled via call sites in this PR.
60-66: APY/volume right-side content: verify units and deps.
- Ensure
portalAsset.metrics.apyis a percent (e.g., 5 for 5%). If it’s already a fraction, drop thetimes('0.01').- Dependency list looks correct; nice.
Do you want me to scan the portals assets schema to confirm APY units and adjust accordingly?
Also applies to: 67-72, 74-79, 81-82
src/components/Table/Table.theme.ts (1)
195-207: Expanded cell radius rules LGTM.
Thetd.*.expandedand expansion container radii align with the virtualized expanded row UI.src/components/ReactTable/InfiniteTable.tsx (1)
26-41: scrollableTarget removal verified – no occurrences found in the codebase.src/components/AssetSearch/AssetSearch.tsx (1)
29-30: Prop plumbing for related assets looks goodNew shouldDisplayRelatedAssets prop is correctly typed, destructured, and forwarded to AssetList without behavior changes.
Also applies to: 36-37, 139-140
src/pages/Explore/ExploreCategory.tsx (2)
30-30: PortalAssetRow import aligns with OneClickDeFi pathImport is correct and scoped to the conditional usage below.
276-279: Confirm rowComponent prop on AssetList; flags usage LGTM
AssetListProps declaresrowComponent?(defaulting toAssetRow), and bothshowPriceandshouldDisplayRelatedAssetsalign with its API.src/pages/Explore/components/CategoryCard.tsx (3)
10-10: Swapping to AssetRow and adding PortalAssetRow is consistent with the refactorImports reflect the new rendering paths and keep OneClickDeFi isolated to PortalAssetRow.
Also applies to: 17-17
198-207: Price display in AssetRow is correctly enabledshowPrice is the right flag for this context; the rest props mirror virtualized-row signatures without side effects.
220-221: Dependency array fixAdding category prevents stale renders when the category changes.
src/state/slices/portfolioSlice/selectors.ts (1)
19-19: Type-only Row import and RowProps alias are fineType-only import avoids runtime cost; RowProps improves downstream typing for tables.
Also applies to: 727-727
src/pages/Explore/Explore.tsx (1)
203-205: Search results: enabling price + related-assets is correctFlags align with AssetList’s updated API for grouped rendering in Explore search.
src/components/TradeAssetSearch/components/DefaultAssetList.tsx (2)
28-31: LGTM on allAssets memoizationConcise and stable; concat order ensures portfolio versions win when grouped by relatedAssetKey.
33-41: Verify UX: should this scope show prices?Trade asset pickers sometimes display prices; AssetList defaults to not showing them. Confirm product intent; if needed, pass showPrice.
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx (2)
168-186: LGTM: unified list + loading statesHeader + AssetList replacement and loading OR-combination look correct and consistent with the new API.
178-186: Confirm whether search results should show priceAcceptance notes often expect price/24h change in search. If yes, add showPrice to AssetList here.
<AssetList assets={searchTermAssets} handleClick={handleAssetClick} hideZeroBalanceAmounts={true} onImportClick={onImportClick} shouldDisplayRelatedAssets isLoading={isLoadingCustomTokens || isAssetListLoading || workerSearchState.isSearching} height='70vh' + showPrice />src/components/Layout/Header/GlobalSearch/AssetSearchResults.tsx (1)
17-29: LGTM: prop simplification and AssetResults wiringSimpler API aligns with the consolidated AssetList flow in AssetResults.
src/components/Modals/AssetSearch/AssetSearchModal.tsx (2)
34-35: LGTM: plumbed shouldDisplayRelatedAssets into modal propsMatches new AssetSearch public API.
57-58: LGTM: forwarding the flagPropagates related-assets grouping into the search component as intended.
src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsx (2)
14-20: Wrapper Item is finePadding via Item component is a clean way to avoid Fragment limitations with Virtuoso components.
1-1: Remove unused GroupedAssetList component
No external references to GroupedAssetList were found—delete src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsx (and its containing folder) to clean up dead code and reduce bundle size.src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx (5)
14-14: Switch to AssetList: LGTM.Import looks correct and aligns with the new shared list API.
24-24: PortalAssetRow integration: LGTM.Correctly enables the OneClickDeFi row override via AssetList’s rowComponent prop.
183-186: Long-press → asset actions: LGTM.Opens the drawer with the correct assetId and aligns with the new AssetList callbacks.
279-279: Correct dependency inclusion: LGTM.Adding portalsAssets to the memo deps prevents stale props.
294-294: Layout adjustment: LGTM.maxWidth='100%' avoids unintended clipping in narrower parents.
src/pages/Dashboard/components/AccountList/GroupedAssetsSubComponent.tsx (2)
30-35: Selector usage: LGTM.Memoized lookup keeps per-row work low; selector returns deep-equal stable arrays.
93-111: Sub-component structure: LGTM.Early return guard and stable keys look good.
src/components/Layout/Header/GlobalSearch/AssetResults/AssetResults.tsx (1)
8-8: AssetList adoption: LGTM.Keeps GlobalSearch aligned with the shared list renderer.
src/pages/Dashboard/components/AccountList/AccountTable.tsx (1)
291-300: Mobile list: confirm grouping parity with desktopMobile path enables
shouldDisplayRelatedAssets. Good. Confirm search and dashboard use the same grouping key (relatedAssetKey) to avoid discrepancies between mobile and desktop.If desktop keeps name-based grouping, users may see different groupings across breakpoints. Consider aligning both to
relatedAssetKey.src/components/AssetSearch/components/GroupedAssetRow.tsx (1)
204-223: AC check: should expanded rows show per-chain price & 24h change?Acceptance criteria mention current price and 24h change per chain on expand. Here expanded AssetRow renders fiat+crypto totals only (showPrice=false). Confirm product intent; if needed, pass showPrice to child rows and ensure the list layout accommodates dynamic height.
src/components/TradeAssetSearch/components/DefaultAssetList.tsx
Outdated
Show resolved
Hide resolved
src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsx
Outdated
Show resolved
Hide resolved
src/pages/Dashboard/components/AccountList/GroupedAssetsSubComponent.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Overall looks to work well in all the places it's implemented 👌 . Much more intuitive.
Blocking: Long press to get the context menu on mobile view doesn't seem to be working any more (at least on web view mobile).
Maybe blocking: Are we supposed to have the grouping on the assets route?
For some reason when I do a global search i'm getting different results between prd and this branch. Confirmed if i'm on local develop it matches prod, so something related to how this branch is doing search results.
I'd be inclined to bump this one up to high risk and get a second review. Just by nature the surface area here is massive (which also means high chance of a million different edge cases) and is touching some important stuff.
Code wise there are some code rabbit suggestions that look reasonable or worth looking into but I don't have many specific nitpicks. Looks sane.
From a product perspective I do wonder if rendering all these icons is a little bit hectic but I don't feel strongly. I am slightly concerned on rendering performance but we'll see how it goes in a built env.
Thanks for the catches, it should be addressed now I don't know why the research was borken, but it's working fine now, probably because of sorting things we had, I removed an extra step which was not needed and took care of other issues, fixed sorting... Changed to high and I'll invoke gomes for a big review as I know his eyes will catch a lot of weird things for sure |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/components/ReactTable/InfiniteTable.tsx (1)
164-192: Add explicit keys to header cells to silence Biome (no wrapper needed).Keep returning bare for TableVirtuoso, but add
key={column.id}to each header cell.Apply this diff:
- <Th + <Th + key={column.id} {...column.getHeaderProps(column.getSortByToggleProps())}
🧹 Nitpick comments (14)
src/pages/Assets/Assets.tsx (4)
29-40: Remove any and simplify search filterTighten types and short‑circuit on empty trimmed queries.
- const filterRowsBySearchTerm = useCallback((rows: Asset[], filterValue: any) => { - if (!filterValue) return rows - if (typeof filterValue !== 'string') { - return [] - } - const search = filterValue.trim().toLowerCase() - const matchedAssets = matchSorter(rows, search, { - keys: ['name', 'symbol'], - threshold: matchSorter.rankings.CONTAINS, - }) - return matchedAssets - }, []) + const filterRowsBySearchTerm = useCallback((rows: Asset[], filterValue: string) => { + const search = filterValue.trim().toLowerCase() + if (!search) return rows + return matchSorter(rows, search, { + keys: ['name', 'symbol'], + threshold: matchSorter.rankings.CONTAINS, + }) + }, [])
26-26: Avoid work on whitespace-only queriesTrim before length check to prevent unnecessary match-sorter runs.
- const isSearching = useMemo(() => searchQuery.length > 0, [searchQuery]) + const isSearching = useMemo(() => searchQuery.trim().length > 0, [searchQuery])
45-52: Guard against missing assetId before navigatingPrevents navigating to ''.
const handleRowClick = useCallback( (row: Row<Asset>) => { const { assetId } = row.original - const url = assetId ? `/assets/${assetId}` : '' - navigate(url) + if (!assetId) return + navigate(`/assets/${assetId}`) }, [navigate], )
54-60: Guard long‑press action when assetId is absentAvoids opening the drawer with invalid payloads.
const handleRowLongPress = useCallback( (row: Row<Asset>) => { const { assetId } = row.original - assetActionsDrawer.open({ assetId }) + if (!assetId) return + assetActionsDrawer.open({ assetId }) }, [assetActionsDrawer], )src/components/ReactTable/InfiniteTable.tsx (2)
68-69: Make expansion state stable across sorting/filtering (avoid using row.id).
row.idcan change with sorting/filters, causing mismatched open states and contributing to the flicker you noted. Prefer a stable key (e.g.,row.original.assetIdfallback torow.id).Apply this diff:
- const [expandedItems, setExpandedItems] = useState<Set<string>>(new Set()) + const [expandedItems, setExpandedItems] = useState<Set<string>>(new Set()) + const getRowKey = useCallback((row: Row<T>) => { + const original: any = row.original ?? {} + return String(original.assetId ?? row.id) + }, []) - const handleRowToggle = useCallback((rowId: string) => { + const handleRowToggle = useCallback((rowKey: string) => { setExpandedItems(prev => { const newSet = new Set(prev) - if (newSet.has(rowId)) { - newSet.delete(rowId) + if (newSet.has(rowKey)) { + newSet.delete(rowKey) } else { - newSet.add(rowId) + newSet.add(rowKey) } return newSet }) }, []) - const isExpanded = expandedItems.has(row.id) + const isExpanded = expandedItems.has(getRowKey(row))- const isExpanded = row ? expandedItems.has(row.id) : false + const isExpanded = row ? expandedItems.has(getRowKey(row)) : false- row.toggleRowExpanded?.() - handleRowToggle(row.id) + row.toggleRowExpanded?.() + handleRowToggle(getRowKey(row))And add
getRowKeyto the deps where needed (e.g.,renderRowContent,tableComponents).Also applies to: 111-121, 135-142, 222-237
75-83: Skeleton cell key prop is unnecessary.
key={index}inside theCellrenderer has no effect and can be removed to reduce noise.Apply this diff:
- ? columns.map((column, index) => ({ + ? columns.map(column => ({ ...column, - Cell: () => <Skeleton key={index} height='16px' />, + Cell: () => <Skeleton height='16px' />, }))src/pages/Dashboard/components/AccountList/GroupedAccounts.tsx (1)
36-40: Avoid fabricating a partial Row on click, or relax the handler type.Constructing
{ original: relatedRow } as Row<AccountRowData>can bite if callers rely on otherRowAPIs. Either (a) change the prop types to acceptAccountRowDatadirectly, or (b) consistently pass a Row-shaped object in both click and long‑press as in the diff above.src/components/AssetSearch/components/AssetRow.tsx (1)
196-206: Confirm sub-rows should omit price view.GroupedAssetRow renders per-chain rows using AssetRow with default showPrice=false. The linked issue suggests per-chain rows include current price and 24h change; if that’s required here, pass showPrice to the inner AssetRow.
Apply if needed:
- {relatedAssets.map(asset => ( + {relatedAssets.map(asset => ( <AssetRow key={asset.assetId} asset={asset} index={0} py={8} + showPrice // eslint-disable-next-line react-memo/require-usememo data={{ assets: [asset], handleClick: handleAssetClick, disableUnsupported, hideZeroBalanceAmounts, handleLongPress: onLongPress, }} /> ))}src/components/MarketTableVirtualized/GroupedAssets.tsx (2)
8-8: Use the local AssetCell wrapper and pass symbol to avoid undefined subText.The MarketTable AssetCell expects symbol; importing from StakingVaults may bypass the wrapper and props. Import the local wrapper and pass symbol.
-import { AssetCell } from '@/components/StakingVaults/Cells' +import { AssetCell } from '@/components/MarketTableVirtualized/AssetCell'- <AssetCell assetId={asset.assetId} /> + <AssetCell assetId={asset.assetId} symbol={asset.symbol} />Also applies to: 57-57
24-26: Hover style equals container bg — no visible feedback.Use a hover token distinct from the container background for clearer affordance.
-const relatedAssetRowHoverStyles = { - bg: 'background.surface.raised.base', -} +const relatedAssetRowHoverStyles = { + bg: 'background.surface.raised.hover', +}src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (4)
18-21: DRY the AssetWithRelatedAssetIds type — import the single source of truth.This type is already exported from GroupedAssets.tsx. Avoid duplicating it here.
-import { GroupedAssets } from '@/components/MarketTableVirtualized/GroupedAssets' +import { GroupedAssets } from '@/components/MarketTableVirtualized/GroupedAssets' +import type { AssetWithRelatedAssetIds } from '@/components/MarketTableVirtualized/GroupedAssets'-type AssetWithRelatedAssetIds = Asset & { - isGrouped?: boolean - relatedAssetIds?: string[] -} +// (removed – use imported AssetWithRelatedAssetIds)Also applies to: 31-34
152-161: Make the toggle icon interactive (explicit expand/collapse).Currently it only renders an icon; add an onClick to toggle expansion for grouped rows. If InfiniteTable injects a toggler, call it; otherwise delegate to onRowClick.
{ Header: '', id: 'toggle', width: 50, Cell: ({ row }: { row: Row<AssetWithRelatedAssetIds> }) => { if (!row.original.isGrouped) return null - - return row.isExpanded ? <ChevronUpIcon /> : <ChevronDownIcon /> + const Icon = row.isExpanded ? ChevronUpIcon : ChevronDownIcon + return ( + <Box + as='button' + onClick={e => { + e.stopPropagation() + // Prefer table-provided toggler if available + // @ts-expect-error: depends on InfiniteTable row API + row.toggleRowExpanded?.(!row.isExpanded) ?? null + }} + aria-label={row.isExpanded ? 'Collapse' : 'Expand'} + cursor='pointer' + > + <Icon /> + </Box> + ) }, },Please verify that row.toggleRowExpanded (or the equivalent API from InfiniteTable) exists and works as expected on desktop.
183-199: Remove eslint disable and memoize mobile handlers.Wrap the mobile click/long‑press mappers with useCallback to stabilize props and drop the disable.
- <AssetList - assets={rows} - // eslint-disable-next-line react-memo/require-usememo - handleClick={asset => onRowClick({ original: asset } as Row<Asset>)} - handleLongPress={ - onRowLongPress - ? asset => onRowLongPress({ original: asset } as Row<Asset>) - : undefined - } + <AssetList + assets={rows} + handleClick={handleMobileClick} + handleLongPress={handleMobileLongPress} height='100vh' shouldDisplayRelatedAssets showPrice />Add above the return (near other handlers):
const toRow = useCallback((asset: Asset) => ({ original: asset } as Row<Asset>), []) const handleMobileClick = useCallback((asset: Asset) => onRowClick(toRow(asset)), [onRowClick, toRow]) const handleMobileLongPress = useCallback( onRowLongPress ? (asset: Asset) => onRowLongPress(toRow(asset)) : undefined, [onRowLongPress, toRow], )
66-71: Ensure price/market data loads for expanded sub‑rows.Only visible parent rows drive useFetchFiatAssetMarketData; expanded related assets may lack price/change data. Consider fetching for groupedRow.relatedAssetIds on expand.
Option: augment handleVisibleRowsChange or renderSubComponent to dispatch market data loads for a grouped row’s relatedAssetIds when it expands.
Also applies to: 174-181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
src/components/AssetSearch/components/AssetList.tsx(6 hunks)src/components/AssetSearch/components/AssetRow.tsx(6 hunks)src/components/AssetSearch/components/GroupedAssetRow.tsx(1 hunks)src/components/MarketTableVirtualized/AssetCell.tsx(1 hunks)src/components/MarketTableVirtualized/GroupedAssets.tsx(1 hunks)src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx(7 hunks)src/components/ReactTable/InfiniteTable.tsx(4 hunks)src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsx(0 hunks)src/components/TradeAssetSearch/components/GroupedAssetList/components/GroupedAssetRowLoading.tsx(0 hunks)src/pages/Assets/Assets.tsx(1 hunks)src/pages/Dashboard/components/AccountList/AccountTable.tsx(8 hunks)src/pages/Dashboard/components/AccountList/GroupedAccounts.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- src/components/TradeAssetSearch/components/GroupedAssetList/components/GroupedAssetRowLoading.tsx
- src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/AssetSearch/components/GroupedAssetRow.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
src/components/MarketTableVirtualized/AssetCell.tsxsrc/pages/Assets/Assets.tsxsrc/components/MarketTableVirtualized/GroupedAssets.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/pages/Dashboard/components/AccountList/GroupedAccounts.tsxsrc/components/ReactTable/InfiniteTable.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/components/AssetSearch/components/AssetRow.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.tsx: ALWAYS wrap components in error boundaries
ALWAYS provide user-friendly fallback components in error boundaries
ALWAYS log errors for debugging in error boundaries
ALWAYS use useErrorToast hook for displaying errors
ALWAYS provide translated error messages in error toasts
ALWAYS handle different error types appropriately in error toasts
Missing error boundaries in React components is an anti-pattern
**/*.tsx: ALWAYS use PascalCase for React component names
ALWAYS use descriptive names that indicate the component's purpose
ALWAYS match the component name to the file name
Flag components without PascalCase
Flag default exports for components
Files:
src/components/MarketTableVirtualized/AssetCell.tsxsrc/pages/Assets/Assets.tsxsrc/components/MarketTableVirtualized/GroupedAssets.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/pages/Dashboard/components/AccountList/GroupedAccounts.tsxsrc/components/ReactTable/InfiniteTable.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/components/AssetSearch/components/AssetRow.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
src/components/MarketTableVirtualized/AssetCell.tsxsrc/pages/Assets/Assets.tsxsrc/components/MarketTableVirtualized/GroupedAssets.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/pages/Dashboard/components/AccountList/GroupedAccounts.tsxsrc/components/ReactTable/InfiniteTable.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/components/AssetSearch/components/AssetRow.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
**/*.{tsx,jsx}: ALWAYS useuseMemofor expensive computations, object/array creations, and filtered data
ALWAYS useuseMemofor derived values and computed properties
ALWAYS useuseMemofor conditional values and simple transformations
ALWAYS useuseCallbackfor event handlers and functions passed as props
ALWAYS useuseCallbackfor any function that could be passed as a prop or dependency
ALWAYS include all dependencies inuseEffect,useMemo,useCallbackdependency arrays
NEVER use// eslint-disable-next-line react-hooks/exhaustive-depsunless absolutely necessary
ALWAYS explain why dependencies are excluded if using eslint disable
ALWAYS use named exports for components
NEVER use default exports for components
KEEP component files under 200 lines when possible
BREAK DOWN large components into smaller, reusable pieces
EXTRACT complex logic into custom hooks
USE local state for component-level state
LIFT state up when needed across multiple components
USE Context for avoiding prop drilling
ALWAYS wrap components in error boundaries for production
ALWAYS handle async errors properly
ALWAYS provide user-friendly error messages
ALWAYS use virtualization for lists with 100+ items
ALWAYS implement proper key props for list items
ALWAYS lazy load heavy components
ALWAYS use React.lazy for code splitting
Components receiving props are wrapped withmemo
Files:
src/components/MarketTableVirtualized/AssetCell.tsxsrc/pages/Assets/Assets.tsxsrc/components/MarketTableVirtualized/GroupedAssets.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/pages/Dashboard/components/AccountList/GroupedAccounts.tsxsrc/components/ReactTable/InfiniteTable.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/components/AssetSearch/components/AssetRow.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
src/components/MarketTableVirtualized/AssetCell.tsxsrc/pages/Assets/Assets.tsxsrc/components/MarketTableVirtualized/GroupedAssets.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/pages/Dashboard/components/AccountList/GroupedAccounts.tsxsrc/components/ReactTable/InfiniteTable.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/components/AssetSearch/components/AssetRow.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
🧠 Learnings (18)
📓 Common learnings
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
📚 Learning: 2025-08-08T14:59:40.422Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/pages/Explore/ExploreCategory.tsx:231-238
Timestamp: 2025-08-08T14:59:40.422Z
Learning: In src/pages/Explore/ExploreCategory.tsx, for the PageHeader filter trigger, NeOMakinG considers changing a clickable Chakra Icon to IconButton too nitpicky for this PR and prefers to keep the current Icon-based trigger; such minor a11y/UI nitpicks should be deferred to a follow-up if needed.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
📚 Learning: 2025-09-02T08:34:08.130Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/pages/Dashboard/components/AccountList/AccountTable.tsx:60-0
Timestamp: 2025-09-02T08:34:08.130Z
Learning: NeOMakinG prefers code review comments to focus only on actual PR changes, not pre-existing code issues, unless there are critical security or correctness concerns directly related to the new functionality.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-08-27T09:51:36.901Z
Learnt from: NeOMakinG
PR: shapeshift/web#10376
File: packages/contracts/src/viemClient.ts:39-40
Timestamp: 2025-08-27T09:51:36.901Z
Learning: NeOMakinG confirmed that issues flagged by CodeRabbit that predate the PR changes and are unrelated to the PR's core objective should be addressed separately, not within the current PR scope.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
📚 Learning: 2025-08-08T15:00:22.321Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx:14-14
Timestamp: 2025-08-08T15:00:22.321Z
Learning: In shapeshift/web reviews for NeOMakinG, avoid nitpicks to change deep-relative imports to '@/…' alias paths within feature/non-refactor PRs; defer such style-only changes to a dedicated follow-up refactor unless they fix an issue.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
📚 Learning: 2025-08-08T15:00:49.887Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsxsrc/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-08-22T13:00:44.879Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-08-27T09:47:06.275Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/components/ButtonWalletPredicate/ButtonWalletPredicate.tsx:7-7
Timestamp: 2025-08-27T09:47:06.275Z
Learning: In shapeshift/web project, NeOMakinG consistently prefers to defer UI/UX improvements and refactoring work (like the Drawer.Close hack fix in ButtonWalletPredicate.tsx) to follow-up PRs rather than expanding the scope of feature PRs, even when the improvements would enhance robustness.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
📚 Learning: 2025-08-08T11:41:22.794Z
Learnt from: NeOMakinG
PR: shapeshift/web#10234
File: src/components/MultiHopTrade/hooks/useGetTradeQuotes/hooks/useTrackTradeQuotes.ts:42-86
Timestamp: 2025-08-08T11:41:22.794Z
Learning: NeOMakinG prefers not to include refactors in move-only PRs; such suggestions should be deferred to follow-up issues instead of being applied within the same PR.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-09-02T12:38:46.898Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/Table/Table.theme.ts:177-180
Timestamp: 2025-09-02T12:38:46.898Z
Learning: NeOMakinG prefers to defer technical debt and CSS correctness issues (like improper hover selectors) to follow-up PRs when the current PR is already large and focused on major feature implementation, even when the issues are valid from a usability/technical perspective.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-08-25T12:59:43.842Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/Fox/components/RFOXSection.tsx:234-248
Timestamp: 2025-08-25T12:59:43.842Z
Learning: NeOMakinG considers suggestions to refactor working modal state management from useState to useDisclosure as too nitpicky, preferring to keep such cosmetic code improvements out of feature PRs unless they address actual issues.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-08-10T22:06:16.818Z
Learnt from: premiumjibles
PR: shapeshift/web#10216
File: src/pages/Dashboard/components/AccountList/AccountTable.tsx:183-189
Timestamp: 2025-08-10T22:06:16.818Z
Learning: In the ShapeShift web codebase, haptic feedback for long press interactions (via `pulseAndroid()`) is handled centrally within the table components (`InfiniteTable`, `ReactTable`, `ReactTableNoPager`, `MarketsTableVirtualized`) using the `useLongPress` hook. Parent components that pass `onRowLongPress` callbacks should not add their own haptic feedback calls as this would cause duplicate vibrations.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-03T22:10:38.426Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/react-best-practices.mdc:0-0
Timestamp: 2025-08-03T22:10:38.426Z
Learning: Applies to **/*.{tsx,jsx} : ALWAYS use virtualization for lists with 100+ items
Applied to files:
src/components/ReactTable/InfiniteTable.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-07-30T20:57:48.176Z
Learnt from: NeOMakinG
PR: shapeshift/web#10148
File: src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx:88-91
Timestamp: 2025-07-30T20:57:48.176Z
Learning: In src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx, NeOMakinG prefers keeping the hardcoded overscan value (40) over dynamic calculation based on viewport height, prioritizing code simplicity over precision when the current approach works effectively.
Applied to files:
src/components/ReactTable/InfiniteTable.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-02T12:37:37.565Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/AssetSearch/components/AssetList.tsx:0-0
Timestamp: 2025-09-02T12:37:37.565Z
Learning: NeOMakinG in project shapeshift/web prefers solutions that preserve sorting order when implementing asset grouping/deduplication functionality, avoiding approaches like lodash groupBy that would reorganize the array structure.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts accepts a filter object with an assetId property (signature: (state, { assetId })), not a raw AssetId string. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/AssetSearch/components/AssetRow.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts accepts a filter object with an assetId property, not a raw AssetId string. The selector signature is (state: ReduxState, filter) where filter is expected to have an assetId property. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/AssetSearch/components/AssetRow.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts expects a filter object with an assetId property, not a raw AssetId string. The selector signature is (state: ReduxState, filter) where filter should have an assetId property. This pattern is consistent across portfolio selectors that use selectAssetIdParamFromFilter. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/AssetSearch/components/AssetRow.tsx
🧬 Code graph analysis (7)
src/components/MarketTableVirtualized/GroupedAssets.tsx (4)
src/state/slices/assetsSlice/selectors.ts (1)
selectAssets(39-42)src/lib/vibrate.ts (1)
vibrate(6-16)src/constants/longPress.ts (1)
defaultLongPressConfig(3-3)src/components/MarketTableVirtualized/AssetCell.tsx (1)
AssetCell(17-25)
src/pages/Dashboard/components/AccountList/AccountTable.tsx (4)
src/state/slices/assetsSlice/selectors.ts (1)
selectAssets(39-42)src/hooks/useInfiniteScroll/useInfiniteScroll.tsx (1)
useInfiniteScroll(11-49)src/pages/Dashboard/components/AccountList/GroupedAccounts.tsx (1)
GroupedAccounts(93-109)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(42-176)
src/pages/Dashboard/components/AccountList/GroupedAccounts.tsx (4)
src/state/slices/portfolioSlice/selectors.ts (2)
AccountRowData(729-741)selectPortfolioAccountRows(743-792)src/lib/vibrate.ts (1)
vibrate(6-16)src/constants/longPress.ts (1)
defaultLongPressConfig(3-3)src/components/MarketTableVirtualized/AssetCell.tsx (1)
AssetCell(17-25)
src/components/ReactTable/InfiniteTable.tsx (2)
src/lib/vibrate.ts (1)
vibrate(6-16)src/constants/longPress.ts (2)
defaultLongPressConfig(3-3)longPressSx(5-14)
src/components/AssetSearch/components/AssetList.tsx (1)
src/pages/Markets/hooks/usePortalsAssetsQuery.ts (1)
PortalsAssets(15-19)
src/components/AssetSearch/components/AssetRow.tsx (7)
src/components/AssetSearch/components/AssetList.tsx (1)
AssetData(15-31)src/state/store.ts (1)
useAppSelector(143-143)src/state/slices/assetsSlice/selectors.ts (1)
selectAssetById(16-20)src/lib/vibrate.ts (1)
vibrate(6-16)src/constants/longPress.ts (1)
defaultLongPressConfig(3-3)src/state/slices/marketDataSlice/selectors.ts (1)
selectMarketDataByAssetIdUserCurrency(66-72)src/components/AssetSearch/components/GroupedAssetRow.tsx (1)
GroupedAssetRow(47-301)
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (3)
src/components/MarketTableVirtualized/GroupedAssets.tsx (2)
AssetWithRelatedAssetIds(13-16)GroupedAssets(69-85)src/state/apis/fiatRamps/hooks.ts (1)
useFetchFiatAssetMarketData(8-20)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(42-176)
🪛 Biome (2.1.2)
src/components/ReactTable/InfiniteTable.tsx
[error] 169-176: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ 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). (1)
- GitHub Check: Call / Static
🔇 Additional comments (8)
src/pages/Assets/Assets.tsx (2)
79-84: Confirm grouping behavior on Assets routeProduct asked whether grouping is desired here; this usage doesn’t pass any grouping hint/prop. Please confirm expected UX for this route and adjust
MarketsTableVirtualizedprops accordingly if grouping should apply.
7-7: Collected output will confirm whether v8 (@tanstack/react-table) is in use. If no v8 imports or dependencies are found, then codebase consistently uses v7 (react-table), and no mismatch exists. Conversely, if v8 usage is detected, we should update allRowimports to@tanstack/react-table. I'll review the results once available.src/components/MarketTableVirtualized/AssetCell.tsx (1)
14-23: Prop plumbing looks correct.
isGroupedis added to the props and correctly forwarded; truncation behavior remains unchanged.src/components/AssetSearch/components/AssetList.tsx (2)
56-74: Dedup preserves ordering – good choice.The Set-based filtering keeps upstream sort order and correctly handles missing relatedAssetKey. LGTM.
157-165: Small-list path: container height/overflow logic is reasonable.Using maxHeight with overflow auto avoids forcing virtualization for tiny lists. Looks good.
src/pages/Dashboard/components/AccountList/AccountTable.tsx (1)
187-196: Chevron toggle UI — synced with expansion state.The dedicated toggle column that reflects row.isExpanded is clear and accessible; with the upstream sync fix in InfiniteTable, this is good.
src/components/AssetSearch/components/AssetRow.tsx (2)
92-101: Price/percent change rendering looks solid.Good use of color mode, icons, and memoization; dependency arrays are correct.
Also applies to: 117-136, 138-181
70-90: Custom-asset import: minimal, safe.Import button behavior, stopPropagation, and optional handler are correct. LGTM.
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
Outdated
Show resolved
Hide resolved
NeOMakinG
force-pushed
the
grouped-assets
branch
from
d25ba0f to
69ca5df
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (1)
31-34: Group by a stable relatedAssetKey instead of parsing name.Splitting on " on " will misgroup assets with similar names/symbols. Use a stable key if present and fall back only when absent.
type AssetWithRelatedAssetIds = Asset & { isGrouped?: boolean relatedAssetIds?: string[] + relatedAssetKey?: string } @@ - const grouped = groupBy(rows, asset => { - const baseAssetName = asset.name.split(' on ')[0] - return `${baseAssetName}_${asset.symbol}` - }) + const grouped = groupBy(rows, asset => { + const baseAssetName = asset.name?.split?.(' on ')[0] + return asset.relatedAssetKey ?? `${asset.symbol}|${baseAssetName}` + })Also applies to: 45-48
🧹 Nitpick comments (5)
src/pages/Explore/Explore.tsx (1)
146-152: Add haptic feedback on long‑press for parity with clickClick handler vibrates; mirror that on long‑press for UX consistency.
const handleAssetLongPress = useCallback( (asset: Asset) => { - const { assetId } = asset + vibrate('heavy') + const { assetId } = asset assetActionsDrawer.open({ assetId }) }, [assetActionsDrawer], )src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (4)
66-71: Also fetch market data for grouped children when a grouped row is visible.Currently only top-level assetIds are fetched; expanded child rows won’t have prefetched data. Include relatedAssetIds of grouped rows.
- const handleVisibleRowsChange = useCallback((visibleRows: Asset[]) => { - const assetIds = visibleRows.map(row => row.assetId) + const handleVisibleRowsChange = useCallback((visibleRows: AssetWithRelatedAssetIds[]) => { + const assetIds = visibleRows.flatMap(row => + row.relatedAssetIds && row.relatedAssetIds.length > 0 ? row.relatedAssetIds : [row.assetId], + ) setVisibleAssetIds(new Set(assetIds)) }, [])Alternatively, call
useFetchFiatAssetMarketData(row.original.relatedAssetIds ?? [])insideGroupedAssetsupon mount/expand.
112-115: Avoid showing per-chain price/change/volume on grouped parent rows.Spec calls for per-chain details in the expanded view; showing a single assetId’s price on the parent can mislead. Hide these cells for grouped parents.
{ id: 'price', Header: () => <Text ml='auto' translation='dashboard.portfolio.price' />, - Cell: ({ row }: { row: Row<AssetWithRelatedAssetIds> }) => ( - <PriceCell assetId={row.original.assetId} /> - ), + Cell: ({ row }: { row: Row<AssetWithRelatedAssetIds> }) => + row.original.isGrouped ? null : <PriceCell assetId={row.original.assetId} />, }, @@ { id: 'change', Header: () => <Text translation='dashboard.portfolio.priceChange' />, - Cell: ({ row }: { row: Row<AssetWithRelatedAssetIds> }) => ( - <ChangeCell assetId={row.original.assetId} /> - ), + Cell: ({ row }: { row: Row<AssetWithRelatedAssetIds> }) => + row.original.isGrouped ? null : <ChangeCell assetId={row.original.assetId} />, }, @@ { id: 'volume', Header: () => <Text translation='dashboard.portfolio.volume' />, - Cell: ({ row }: { row: Row<AssetWithRelatedAssetIds> }) => ( - <VolumeCell assetId={row.original.assetId} /> - ), + Cell: ({ row }: { row: Row<AssetWithRelatedAssetIds> }) => + row.original.isGrouped ? null : <VolumeCell assetId={row.original.assetId} />, },Also applies to: 119-124, 130-135
153-156: Minor: empty string Header can confuse screen readers.Prefer a null render to avoid announcing an empty header cell.
- Header: '', + Header: () => null,
36-42: Ensure this component is within an error boundary.Per project guidelines, wrap TSX components in error boundaries with user-friendly fallbacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/components/AssetSearch/components/GroupedAssetRow.tsx(1 hunks)src/components/MarketTableVirtualized/GroupedAssets.tsx(1 hunks)src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx(7 hunks)src/pages/Assets/Assets.tsx(1 hunks)src/pages/Explore/Explore.tsx(4 hunks)src/pages/Explore/ExploreCategory.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pages/Assets/Assets.tsx
- src/components/MarketTableVirtualized/GroupedAssets.tsx
- src/components/AssetSearch/components/GroupedAssetRow.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
src/pages/Explore/ExploreCategory.tsxsrc/pages/Explore/Explore.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.tsx: ALWAYS wrap components in error boundaries
ALWAYS provide user-friendly fallback components in error boundaries
ALWAYS log errors for debugging in error boundaries
ALWAYS use useErrorToast hook for displaying errors
ALWAYS provide translated error messages in error toasts
ALWAYS handle different error types appropriately in error toasts
Missing error boundaries in React components is an anti-pattern
**/*.tsx: ALWAYS use PascalCase for React component names
ALWAYS use descriptive names that indicate the component's purpose
ALWAYS match the component name to the file name
Flag components without PascalCase
Flag default exports for components
Files:
src/pages/Explore/ExploreCategory.tsxsrc/pages/Explore/Explore.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
src/pages/Explore/ExploreCategory.tsxsrc/pages/Explore/Explore.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
**/*.{tsx,jsx}: ALWAYS useuseMemofor expensive computations, object/array creations, and filtered data
ALWAYS useuseMemofor derived values and computed properties
ALWAYS useuseMemofor conditional values and simple transformations
ALWAYS useuseCallbackfor event handlers and functions passed as props
ALWAYS useuseCallbackfor any function that could be passed as a prop or dependency
ALWAYS include all dependencies inuseEffect,useMemo,useCallbackdependency arrays
NEVER use// eslint-disable-next-line react-hooks/exhaustive-depsunless absolutely necessary
ALWAYS explain why dependencies are excluded if using eslint disable
ALWAYS use named exports for components
NEVER use default exports for components
KEEP component files under 200 lines when possible
BREAK DOWN large components into smaller, reusable pieces
EXTRACT complex logic into custom hooks
USE local state for component-level state
LIFT state up when needed across multiple components
USE Context for avoiding prop drilling
ALWAYS wrap components in error boundaries for production
ALWAYS handle async errors properly
ALWAYS provide user-friendly error messages
ALWAYS use virtualization for lists with 100+ items
ALWAYS implement proper key props for list items
ALWAYS lazy load heavy components
ALWAYS use React.lazy for code splitting
Components receiving props are wrapped withmemo
Files:
src/pages/Explore/ExploreCategory.tsxsrc/pages/Explore/Explore.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
src/pages/Explore/ExploreCategory.tsxsrc/pages/Explore/Explore.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
🧠 Learnings (7)
📓 Common learnings
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
📚 Learning: 2025-08-08T14:59:40.422Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/pages/Explore/ExploreCategory.tsx:231-238
Timestamp: 2025-08-08T14:59:40.422Z
Learning: In src/pages/Explore/ExploreCategory.tsx, for the PageHeader filter trigger, NeOMakinG considers changing a clickable Chakra Icon to IconButton too nitpicky for this PR and prefers to keep the current Icon-based trigger; such minor a11y/UI nitpicks should be deferred to a follow-up if needed.
Applied to files:
src/pages/Explore/ExploreCategory.tsxsrc/pages/Explore/Explore.tsx
📚 Learning: 2025-08-08T15:00:49.887Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Applied to files:
src/pages/Explore/ExploreCategory.tsxsrc/pages/Explore/Explore.tsx
📚 Learning: 2025-08-06T09:47:29.865Z
Learnt from: NeOMakinG
PR: shapeshift/web#10191
File: src/pages/Explore/Explore.tsx:174-178
Timestamp: 2025-08-06T09:47:29.865Z
Learning: In src/pages/Explore/Explore.tsx, NeOMakinG prefers keeping complex height calculations inline rather than extracting them to useMemo, prioritizing perceived performance/execution speed over code readability, even when the calculation would only run when the dependency (isSearching) changes.
Applied to files:
src/pages/Explore/Explore.tsx
📚 Learning: 2025-07-30T20:57:48.176Z
Learnt from: NeOMakinG
PR: shapeshift/web#10148
File: src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx:88-91
Timestamp: 2025-07-30T20:57:48.176Z
Learning: In src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx, NeOMakinG prefers keeping the hardcoded overscan value (40) over dynamic calculation based on viewport height, prioritizing code simplicity over precision when the current approach works effectively.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-03T22:10:38.426Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/react-best-practices.mdc:0-0
Timestamp: 2025-08-03T22:10:38.426Z
Learning: Applies to **/*.{tsx,jsx} : ALWAYS use virtualization for lists with 100+ items
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-10T22:06:16.818Z
Learnt from: premiumjibles
PR: shapeshift/web#10216
File: src/pages/Dashboard/components/AccountList/AccountTable.tsx:183-189
Timestamp: 2025-08-10T22:06:16.818Z
Learning: In the ShapeShift web codebase, haptic feedback for long press interactions (via `pulseAndroid()`) is handled centrally within the table components (`InfiniteTable`, `ReactTable`, `ReactTableNoPager`, `MarketsTableVirtualized`) using the `useLongPress` hook. Parent components that pass `onRowLongPress` callbacks should not add their own haptic feedback calls as this would cause duplicate vibrations.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
🧬 Code graph analysis (3)
src/pages/Explore/ExploreCategory.tsx (2)
src/hooks/useModal/useModal.tsx (1)
useModal(6-24)src/pages/Explore/components/PortalAssetRow.tsx (1)
PortalAssetRow(34-139)
src/pages/Explore/Explore.tsx (1)
src/hooks/useModal/useModal.tsx (1)
useModal(6-24)
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (3)
src/state/apis/fiatRamps/hooks.ts (1)
useFetchFiatAssetMarketData(8-20)src/components/MarketTableVirtualized/AssetCell.tsx (1)
AssetCell(17-25)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(42-176)
⏰ 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). (1)
- GitHub Check: Call / Static
🔇 Additional comments (9)
src/pages/Explore/ExploreCategory.tsx (4)
29-31: Modal + row imports look correctImports align with the new long‑press flow and OneClickDeFi row specialization.
50-50: Confirm modal props contract for assetActionsDrawerEnsure Modals['assetActionsDrawer'] expects a props object with an assetId: string to avoid runtime typing mismatches.
222-229: Long‑press wiring to open asset actions drawer — LGTMCallback is memoized and correctly passes assetId.
286-290: AssetList grouping flags and custom row — verify intended behaviorPassing showPrice + shouldDisplayRelatedAssets with a custom row for OneClickDeFi is fine; confirm showPrice is only relevant for the default row and intentionally ignored by PortalAssetRow.
src/pages/Explore/Explore.tsx (3)
37-39: Import updates look goodFeature flag and modal hook imports are correct.
92-92: Modal handle initialization — LGTMuseModal('assetActionsDrawer') usage is consistent with the drawer flow.
214-217: Grouped display + long‑press props — verify interactionsProps set look right; please sanity‑check on devices:
- Long‑press opens the drawer without interfering with list scroll/virtualization.
- Grouped rows expand/collapse correctly and preserve scroll position.
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (2)
191-203: Mobile path looks good.Passing
shouldDisplayRelatedAssetsdelegates deduping to AssetList; long-press is preserved. Height and price display make sense.
25-34: Prop signature change validated. All MarketsTableVirtualized call sites now pass callbacks receiving Asset (norow/.originalusage remains).
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
Outdated
Show resolved
Hide resolved
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/MarketTableVirtualized/GroupedAssets.tsx (1)
38-40: Fix long‑press payload: pass Row context instead of Asset (aligns with table consumers).This still forwards an Asset as the long‑press context. Consumers in table flows typically expect Row and read row.original. Also good that local haptics were removed for long‑press—keep it that way per central handling.
- onRowLongPress?: (asset: Asset) => void + onRowLongPress?: (row: Row<Asset>) => void @@ - const longPressHandlers = useLongPress((_, { context: asset }) => { - onRowLongPress?.(asset as Asset) - }, defaultLongPressConfig) + const longPressHandlers = useLongPress<Row<Asset>>((_, { context: row }) => { + onRowLongPress?.(row) + }, defaultLongPressConfig) @@ - {...longPressHandlers(asset)} + {...longPressHandlers({ original: asset } as Row<Asset>)} @@ - onRowLongPress?: (asset: Asset) => void + onRowLongPress?: (row: Row<Asset>) => voidAlso applies to: 53-53, 21-21, 65-65
🧹 Nitpick comments (11)
src/components/ReactTable/InfiniteTable.tsx (6)
165-193: Add stable keys to header cells to satisfy Biome (no wrapping).Keep returning bare for TableVirtuoso, but add explicit keys from getHeaderProps to silence the linter and avoid unnecessary re-renders.
Apply this diff:
- return headerGroups.map(headerGroup => - headerGroup.headers.map(column => ( - <Th - {...column.getHeaderProps(column.getSortByToggleProps())} + return headerGroups.map(headerGroup => + headerGroup.headers.map(column => { + const headerProps = column.getHeaderProps(column.getSortByToggleProps()) + return ( + <Th + key={headerProps.key ?? column.id} + {...headerProps} color='text.subtle' textAlign={column.textAlign} display={column.display} // eslint-disable-next-line react-memo/require-usememo _hover={{ color: column.canSort ? hoverColor : 'text.subtle' }} - > + > <Flex justifyContent={column.justifyContent} alignItems={column.alignItems}> {column.render('Header')} <Flex alignItems='center'> {column.isSorted ? ( column.isSortedDesc ? ( <ArrowDownIcon ml={2} aria-label={translate('common.table.sortedDesc')} /> ) : ( <ArrowUpIcon ml={2} aria-label={translate('common.table.sortedAsc')} /> ) ) : null} </Flex> </Flex> - </Th> - )), + </Th> + ) + }), )
145-157: Use react-table’s generated cell keys to avoid collisions.getCellProps() provides a stable key; don’t override it with column.id. This prevents potential duplicate keys across grouped/expanded tables.
Apply this diff:
- {row.cells.map(cell => ( - <Td - {...cell.getCellProps()} + {row.cells.map(cell => { + const cellProps = cell.getCellProps() + const cellKey = (cellProps as any).key ?? cell.column.id + return ( + <Td + key={cellKey} + {...cellProps} {...(typeof cell.column.Header === 'string' ? { 'data-label': cell.column.Header } : undefined)} display={cell.column.display} - key={cell.column.id} className={isExpanded ? 'expanded' : ''} borderBottomRadius={isExpanded ? 0 : undefined} > {cell.render('Cell')} </Td> - ))} + )})}
239-249: Guard data-test prefix to avoid “undefined-” attributes.If rowDataTestPrefix is not provided, the attribute currently becomes “undefined-…”.
Apply this diff:
- {...(row && rowDataTestKey + {...(row && rowDataTestKey ? { - 'data-test': `${rowDataTestPrefix}-${String( + 'data-test': `${(rowDataTestPrefix ?? 'row')}-${String( row.original?.[rowDataTestKey] ?? '', ) .split(' ') .join('-') .toLowerCase()}`, } : {})}
68-70: Expansion state: consider syncing or resetting on data changes.You maintain both react-table expansion and a local Set. That’s fine, but stale ids can persist after data resort/filter. Optionally reset expandedItems when data identity changes.
Apply this minimal reset (keeps current UX predictable):
import { Fragment, useCallback, useMemo, useRef, useState } from 'react' +import { useEffect } from 'react' … const [expandedItems, setExpandedItems] = useState<Set<string>>(new Set()) +useEffect(() => { + setExpandedItems(new Set()) +}, [data])If preserving expansion across minor data updates is required, derive row ids from a stable key (e.g., assetId) and skip the reset.
Also applies to: 112-122, 124-134, 253-259
210-218: Type safety for Virtuoso components mapping.Avoid any to retain prop inference and catch regressions.
Apply this diff:
-import { TableVirtuoso } from 'react-virtuoso' +import { TableVirtuoso, type TableComponents } from 'react-virtuoso' … - const tableComponents = useMemo( - () => ({ + const tableComponents = useMemo<TableComponents<Row<T>>>( + () => ({ - Table: ({ style, ...props }: any) => ( + Table: ({ style, ...props }) => ( <Table ref={tableRef} variant={variant} size={tableSize} style={style} className='infinite-table' {...props} /> ), TableHead: Thead, TableBody: Tbody, TableFoot: Tfoot, - TableRow: ({ children, item, context, ...props }: any) => { + TableRow: ({ children, item, ...props }) => {Note: adjust the generic if your Row type differs.
Also applies to: 223-265
231-236: Tab order nit: avoid numeric tabIndex for rows.tabIndex={row.index} can create surprising keyboard order. Prefer 0 (focusable in DOM order) or -1 (programmatic only).
- tabIndex={row?.index} + tabIndex={0}src/components/MarketTableVirtualized/GroupedAssets.tsx (2)
72-83: Precompute fiat map and add deterministic tiebreaker to sort.Avoid repeated Array.find calls during sort and stabilize order for equal/unknown fiat amounts.
- const sortedAssetIds = useMemo(() => { - if (!groupedRow.relatedAssetIds) return [] - - return [...groupedRow.relatedAssetIds].sort((a, b) => { - const aRow = rowData.find(r => r.assetId === a) - const bRow = rowData.find(r => r.assetId === b) - - if (!aRow || !bRow) return 0 - - return Number(bRow.fiatAmount) - Number(aRow.fiatAmount) - }) - }, [groupedRow.relatedAssetIds, rowData]) + const sortedAssetIds = useMemo(() => { + const ids = groupedRow.relatedAssetIds ?? [] + // Build fiat lookup once + const fiatByAssetId = new Map<string, number>() + for (const r of rowData) fiatByAssetId.set(r.assetId, Number(r.fiatAmount)) + + return [...ids].sort((a, b) => { + const aFiat = fiatByAssetId.get(a) ?? -Infinity + const bFiat = fiatByAssetId.get(b) ?? -Infinity + if (aFiat !== bFiat) return bFiat - aFiat + // Deterministic tie-breaker + return a.localeCompare(b) + }) + }, [groupedRow.relatedAssetIds, rowData])
18-22: Reduce store subscriptions: pass asset from parent instead of selecting per row.One selector per RelatedAssetRow scales with group size. Lifting the lookup to the parent keeps a single subscription and simplifies the child.
type RelatedAssetRowProps = { - assetId: string - onRowClick: (asset: Asset) => void - onRowLongPress?: (asset: Asset) => void + asset: Asset | undefined + onRowClick: (asset: Asset) => void + onRowLongPress?: (row: Row<Asset>) => void } @@ -const RelatedAssetRow = memo<RelatedAssetRowProps>(({ assetId, onRowClick, onRowLongPress }) => { - const assets = useSelector(selectAssets) - const asset = assets[assetId] +const RelatedAssetRow = memo<RelatedAssetRowProps>(({ asset, onRowClick, onRowLongPress }) => { @@ export const GroupedAssets = memo<GroupedAssetsProps>(({ row, onRowClick, onRowLongPress }) => { const groupedRow = row.original as AssetWithRelatedAssetIds - const rowData = useSelector(selectPortfolioAccountRows) + const rowData = useSelector(selectPortfolioAccountRows) + const assets = useSelector(selectAssets) @@ - {sortedAssetIds.map(assetId => ( + {sortedAssetIds.map(assetId => ( <RelatedAssetRow - key={assetId} - assetId={assetId} + key={assetId} + asset={assets[assetId]} onRowClick={onRowClick} onRowLongPress={onRowLongPress} /> ))}Also applies to: 28-31, 70-71, 90-95
src/components/AssetSearch/components/AssetList.tsx (2)
151-151: Unnecessary optional chaining on a non-optional valueuniqueAssets is always defined from useMemo. You can drop the optional chain.
- if (uniqueAssets?.length === 0) { + if (uniqueAssets.length === 0) {
172-176: Overscan may be excessive on low-end devicesoverscan={1000} can increase memory/CPU on large lists. Consider 200–400 or making it responsive to row height.
- overscan={1000} + overscan={300}src/pages/Dashboard/components/AccountList/AccountTable.tsx (1)
60-66: Prefer a stable grouping key when availableIf assets[row.assetId]?.relatedAssetKey exists, use it to group; it’s more robust than splitting on “ on ”.
- const grouped = groupBy(rowData, row => { - const asset = assets[row.assetId] - if (!asset) return row.assetId - const baseAssetName = asset.name.split(' on ')[0] - return `${baseAssetName}_${asset.symbol}` - }) + const grouped = groupBy(rowData, row => { + const asset = assets[row.assetId] + if (!asset) return row.assetId + return asset.relatedAssetKey ?? `${asset.name.split(' on ')[0]}_${asset.symbol}` + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/components/AssetSearch/components/AssetList.tsx(6 hunks)src/components/MarketTableVirtualized/GroupedAssets.tsx(1 hunks)src/components/ReactTable/InfiniteTable.tsx(4 hunks)src/pages/Dashboard/components/AccountList/AccountTable.tsx(9 hunks)src/pages/Dashboard/components/AccountList/GroupedAccounts.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Dashboard/components/AccountList/GroupedAccounts.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
src/components/MarketTableVirtualized/GroupedAssets.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.tsx: ALWAYS wrap components in error boundaries
ALWAYS provide user-friendly fallback components in error boundaries
ALWAYS log errors for debugging in error boundaries
ALWAYS use useErrorToast hook for displaying errors
ALWAYS provide translated error messages in error toasts
ALWAYS handle different error types appropriately in error toasts
Missing error boundaries in React components is an anti-pattern
**/*.tsx: ALWAYS use PascalCase for React component names
ALWAYS use descriptive names that indicate the component's purpose
ALWAYS match the component name to the file name
Flag components without PascalCase
Flag default exports for components
Files:
src/components/MarketTableVirtualized/GroupedAssets.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
src/components/MarketTableVirtualized/GroupedAssets.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
**/*.{tsx,jsx}: ALWAYS useuseMemofor expensive computations, object/array creations, and filtered data
ALWAYS useuseMemofor derived values and computed properties
ALWAYS useuseMemofor conditional values and simple transformations
ALWAYS useuseCallbackfor event handlers and functions passed as props
ALWAYS useuseCallbackfor any function that could be passed as a prop or dependency
ALWAYS include all dependencies inuseEffect,useMemo,useCallbackdependency arrays
NEVER use// eslint-disable-next-line react-hooks/exhaustive-depsunless absolutely necessary
ALWAYS explain why dependencies are excluded if using eslint disable
ALWAYS use named exports for components
NEVER use default exports for components
KEEP component files under 200 lines when possible
BREAK DOWN large components into smaller, reusable pieces
EXTRACT complex logic into custom hooks
USE local state for component-level state
LIFT state up when needed across multiple components
USE Context for avoiding prop drilling
ALWAYS wrap components in error boundaries for production
ALWAYS handle async errors properly
ALWAYS provide user-friendly error messages
ALWAYS use virtualization for lists with 100+ items
ALWAYS implement proper key props for list items
ALWAYS lazy load heavy components
ALWAYS use React.lazy for code splitting
Components receiving props are wrapped withmemo
Files:
src/components/MarketTableVirtualized/GroupedAssets.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
src/components/MarketTableVirtualized/GroupedAssets.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
🧠 Learnings (21)
📓 Common learnings
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
📚 Learning: 2025-08-10T22:06:16.818Z
Learnt from: premiumjibles
PR: shapeshift/web#10216
File: src/pages/Dashboard/components/AccountList/AccountTable.tsx:183-189
Timestamp: 2025-08-10T22:06:16.818Z
Learning: In the ShapeShift web codebase, haptic feedback for long press interactions (via `pulseAndroid()`) is handled centrally within the table components (`InfiniteTable`, `ReactTable`, `ReactTableNoPager`, `MarketsTableVirtualized`) using the `useLongPress` hook. Parent components that pass `onRowLongPress` callbacks should not add their own haptic feedback calls as this would cause duplicate vibrations.
Applied to files:
src/components/MarketTableVirtualized/GroupedAssets.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-08-08T15:00:49.887Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Applied to files:
src/components/MarketTableVirtualized/GroupedAssets.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
📚 Learning: 2025-08-08T15:00:22.321Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx:14-14
Timestamp: 2025-08-08T15:00:22.321Z
Learning: In shapeshift/web reviews for NeOMakinG, avoid nitpicks to change deep-relative imports to '@/…' alias paths within feature/non-refactor PRs; defer such style-only changes to a dedicated follow-up refactor unless they fix an issue.
Applied to files:
src/components/MarketTableVirtualized/GroupedAssets.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
📚 Learning: 2025-08-27T09:47:06.275Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/components/ButtonWalletPredicate/ButtonWalletPredicate.tsx:7-7
Timestamp: 2025-08-27T09:47:06.275Z
Learning: In shapeshift/web project, NeOMakinG consistently prefers to defer UI/UX improvements and refactoring work (like the Drawer.Close hack fix in ButtonWalletPredicate.tsx) to follow-up PRs rather than expanding the scope of feature PRs, even when the improvements would enhance robustness.
Applied to files:
src/components/MarketTableVirtualized/GroupedAssets.tsxsrc/components/AssetSearch/components/AssetList.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
📚 Learning: 2025-08-27T09:51:36.901Z
Learnt from: NeOMakinG
PR: shapeshift/web#10376
File: packages/contracts/src/viemClient.ts:39-40
Timestamp: 2025-08-27T09:51:36.901Z
Learning: NeOMakinG confirmed that issues flagged by CodeRabbit that predate the PR changes and are unrelated to the PR's core objective should be addressed separately, not within the current PR scope.
Applied to files:
src/components/MarketTableVirtualized/GroupedAssets.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
📚 Learning: 2025-09-02T12:37:37.565Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/AssetSearch/components/AssetList.tsx:0-0
Timestamp: 2025-09-02T12:37:37.565Z
Learning: NeOMakinG in project shapeshift/web prefers solutions that preserve sorting order when implementing asset grouping/deduplication functionality, avoiding approaches like lodash groupBy that would reorganize the array structure.
Applied to files:
src/components/AssetSearch/components/AssetList.tsxsrc/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts expects a filter object with an assetId property, not a raw AssetId string. The selector signature is (state: ReduxState, filter) where filter should have an assetId property. This pattern is consistent across portfolio selectors that use selectAssetIdParamFromFilter. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-08-22T12:59:01.702Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/components/Layout/Header/ActionCenter/components/RewardDistributionActionCard.tsx:37-53
Timestamp: 2025-08-22T12:59:01.702Z
Learning: In RewardDistributionActionCard component (src/components/Layout/Header/ActionCenter/components/RewardDistributionActionCard.tsx), NeOMakinG confirmed that runeAsset is expected to always be defined when the component renders, so defensive guards against undefined runeAsset are not needed.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-08-22T12:58:36.070Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/components/Layout/Header/ActionCenter/components/Notifications/RewardDistributionNotification.tsx:33-55
Timestamp: 2025-08-22T12:58:36.070Z
Learning: In RewardDistributionNotification component (src/components/Layout/Header/ActionCenter/components/Notifications/RewardDistributionNotification.tsx), NeOMakinG confirmed that runeAsset is expected to always be defined when the component renders, so defensive guards against undefined runeAsset are not needed.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-08-05T22:41:35.473Z
Learnt from: premiumjibles
PR: shapeshift/web#10187
File: src/pages/Assets/Asset.tsx:1-1
Timestamp: 2025-08-05T22:41:35.473Z
Learning: In the shapeshift/web codebase, component imports use direct file paths like '@/components/ComponentName/ComponentName' rather than barrel exports. The AssetAccountDetails component should be imported as '@/components/AssetAccountDetails/AssetAccountDetails', not from a directory index.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-07-29T10:22:27.037Z
Learnt from: NeOMakinG
PR: shapeshift/web#10136
File: src/lib/asset-service/service/encodedRelatedAssetIndex.json:1-1
Timestamp: 2025-07-29T10:22:27.037Z
Learning: PRs with titles starting with "feat: regenerate asset data" are routine daily asset updates that don't need detailed code analysis. Users prefer to skip automated reviews for these maintenance PRs using coderabbitai ignore.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-08-25T23:32:13.876Z
Learnt from: premiumjibles
PR: shapeshift/web#10361
File: src/pages/Markets/components/CardWithSparkline.tsx:83-92
Timestamp: 2025-08-25T23:32:13.876Z
Learning: In shapeshift/web PR #10361, premiumjibles considered the nested button accessibility issue (ChartErrorFallback retry Button inside Card rendered as Button in CardWithSparkline.tsx) out of scope for the error boundaries feature PR, consistent with deferring minor a11y improvements to follow-up PRs.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-08-08T14:59:40.422Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/pages/Explore/ExploreCategory.tsx:231-238
Timestamp: 2025-08-08T14:59:40.422Z
Learning: In src/pages/Explore/ExploreCategory.tsx, for the PageHeader filter trigger, NeOMakinG considers changing a clickable Chakra Icon to IconButton too nitpicky for this PR and prefers to keep the current Icon-based trigger; such minor a11y/UI nitpicks should be deferred to a follow-up if needed.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/ReactTable/InfiniteTable.tsx
📚 Learning: 2025-09-02T08:34:08.130Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/pages/Dashboard/components/AccountList/AccountTable.tsx:60-0
Timestamp: 2025-09-02T08:34:08.130Z
Learning: NeOMakinG prefers code review comments to focus only on actual PR changes, not pre-existing code issues, unless there are critical security or correctness concerns directly related to the new functionality.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-08-22T13:00:44.879Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-08-08T11:41:22.794Z
Learnt from: NeOMakinG
PR: shapeshift/web#10234
File: src/components/MultiHopTrade/hooks/useGetTradeQuotes/hooks/useTrackTradeQuotes.ts:42-86
Timestamp: 2025-08-08T11:41:22.794Z
Learning: NeOMakinG prefers not to include refactors in move-only PRs; such suggestions should be deferred to follow-up issues instead of being applied within the same PR.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-09-02T12:38:46.898Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/Table/Table.theme.ts:177-180
Timestamp: 2025-09-02T12:38:46.898Z
Learning: NeOMakinG prefers to defer technical debt and CSS correctness issues (like improper hover selectors) to follow-up PRs when the current PR is already large and focused on major feature implementation, even when the issues are valid from a usability/technical perspective.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-08-25T12:59:43.842Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/Fox/components/RFOXSection.tsx:234-248
Timestamp: 2025-08-25T12:59:43.842Z
Learning: NeOMakinG considers suggestions to refactor working modal state management from useState to useDisclosure as too nitpicky, preferring to keep such cosmetic code improvements out of feature PRs unless they address actual issues.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-08-03T22:10:38.426Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/react-best-practices.mdc:0-0
Timestamp: 2025-08-03T22:10:38.426Z
Learning: Applies to **/*.{tsx,jsx} : ALWAYS use virtualization for lists with 100+ items
Applied to files:
src/components/ReactTable/InfiniteTable.tsx
📚 Learning: 2025-07-30T20:57:48.176Z
Learnt from: NeOMakinG
PR: shapeshift/web#10148
File: src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx:88-91
Timestamp: 2025-07-30T20:57:48.176Z
Learning: In src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx, NeOMakinG prefers keeping the hardcoded overscan value (40) over dynamic calculation based on viewport height, prioritizing code simplicity over precision when the current approach works effectively.
Applied to files:
src/components/ReactTable/InfiniteTable.tsx
🧬 Code graph analysis (4)
src/components/MarketTableVirtualized/GroupedAssets.tsx (3)
src/state/slices/assetsSlice/selectors.ts (1)
selectAssets(39-42)src/lib/vibrate.ts (1)
vibrate(6-16)src/constants/longPress.ts (1)
defaultLongPressConfig(3-3)
src/components/AssetSearch/components/AssetList.tsx (1)
src/pages/Markets/hooks/usePortalsAssetsQuery.ts (1)
PortalsAssets(15-19)
src/pages/Dashboard/components/AccountList/AccountTable.tsx (5)
src/state/slices/assetsSlice/selectors.ts (1)
selectAssets(39-42)src/hooks/useInfiniteScroll/useInfiniteScroll.tsx (1)
useInfiniteScroll(11-49)src/state/slices/portfolioSlice/selectors.ts (2)
RowProps(727-727)AccountRowData(729-741)src/pages/Dashboard/components/AccountList/GroupedAccounts.tsx (1)
GroupedAccounts(93-124)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(45-179)
src/components/ReactTable/InfiniteTable.tsx (2)
src/lib/vibrate.ts (1)
vibrate(6-16)src/constants/longPress.ts (2)
defaultLongPressConfig(3-3)longPressSx(5-14)
🪛 Biome (2.1.2)
src/components/ReactTable/InfiniteTable.tsx
[error] 170-177: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ 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). (1)
- GitHub Check: Call / Static
🔇 Additional comments (11)
src/components/ReactTable/InfiniteTable.tsx (4)
34-35: Visible range callback: inclusive end index handled correctly.onVisibleRowsChange addition and the slice(endIndex + 1) fix are correct.
Also applies to: 103-110
300-310: Overscan 2000px: verify perf on low-end mobile.Large overscan smooths scroll but can cost memory/CPU with rich rows. Suggest testing on mid/low devices; if jank appears, try 800–1200.
279-296: Loading skeletons fit the new virtualization flow.Good UX: preserves table structure and avoids layout shift.
70-74: Long-press regression check.Hook wiring and context look correct. Please re-verify on mobile web: press-and-hold opens the menu, and a short tap still triggers handleRowClick.
Also applies to: 223-236
src/components/MarketTableVirtualized/GroupedAssets.tsx (1)
88-97: Verify expanded rows show per‑chain metrics per spec (name, crypto, fiat, price, 24h).This renders only AssetCell. Confirm other per‑chain columns are visible in the expanded area (via AssetCell or adjacent cells). If not, we’ll need to add them to meet #10339.
src/components/AssetSearch/components/AssetList.tsx (3)
21-28: Row renderer type surface looks goodExpanding rowComponent props to include showPrice/onImportClick/shouldDisplayRelatedAssets aligns with how you invoke it. No further action.
Also applies to: 31-33
54-58: Prop defaults are sensibleshowPrice default false and shouldDisplayRelatedAssets default false are appropriate; good to keep behavior opt‑in.
59-77: Dedup preserves sort order — niceSet-based relatedAssetKey filtering avoids groupBy reordering and handles undefined keys correctly. Solid approach.
src/pages/Dashboard/components/AccountList/AccountTable.tsx (3)
183-192: Chevron relies on row.isExpandedAssuming InfiniteTable now syncs react-table expansion, this is fine. If you see stale icons, verify the toggle sync remains in place.
233-236: No-op click for grouped rows is correctDelegating expansion to the table avoids double toggles. LGTM.
279-293: Mobile dataset source is correctFeeding AssetList from full grouped list (uniqueRowsAll) ensures complete results; filtering with isSome avoids undefined assets. Nice.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/TradeAssetSearch/TradeAssetSearch.tsx (1)
121-127: Prop/state sync: update activeChainId when selectedChainId prop changesWithout syncing, parent-driven chain changes won’t reflect here after mount.
Add this effect near the state init:
useEffect(() => { setActiveChainId(selectedChainId) }, [selectedChainId])
♻️ Duplicate comments (2)
src/components/AssetSearch/components/GroupedAssetRow.tsx (2)
112-120: Avoid recreating the data prop; memoize common parts.Pass a stable base object and only vary the asset to reduce avoidable re-renders.
- // eslint-disable-next-line react-memo/require-usememo - data={{ - assets: [relatedAsset], - handleClick: handleAssetClick, - disableUnsupported, - hideZeroBalanceAmounts, - handleLongPress: onLongPress, - }} + data={{ ...rowDataBase, assets: [relatedAsset] }}Add outside this range:
const rowDataBase = useMemo( () => ({ handleClick: handleAssetClick, disableUnsupported, hideZeroBalanceAmounts, handleLongPress: onLongPress }), [handleAssetClick, disableUnsupported, hideZeroBalanceAmounts, onLongPress], )
74-96: Subscribe instead of using store.getState; add stable keys.Direct store.getState breaks reactivity; also key can be undefined. Select fee assets via useAppSelector and fallback the key.
- const networksIcons = useMemo(() => { - return groupedAssetBalances?.relatedAssets.map(asset => { - const feeAsset = selectFeeAssetByChainId( - store.getState(), - assets[asset.assetId]?.chainId ?? '', - ) - return ( - <Box - key={feeAsset?.chainId} - w={2} - borderRadius='full' - display='flex' - alignItems='center' - justifyContent='center' - fontSize='xs' - color='white' - fontWeight='bold' - > - <LazyLoadAvatar src={feeAsset?.networkIcon ?? feeAsset?.icon} boxSize={4} /> - </Box> - ) - }) - }, [groupedAssetBalances?.relatedAssets, assets]) + const networksIcons = useMemo(() => { + if (!groupedAssetBalances?.relatedAssets) return null + return groupedAssetBalances.relatedAssets.map(asset => { + const feeAsset = selectFeeAssetByChainId( + // will be read reactively via useAppSelector below + store.getState(), + assets[asset.assetId]?.chainId ?? '', + ) + return ( + <Box + key={feeAsset?.chainId ?? asset.assetId} + w={2} + borderRadius='full' + display='flex' + alignItems='center' + justifyContent='center' + fontSize='xs' + color='white' + fontWeight='bold' + > + <LazyLoadAvatar src={feeAsset?.networkIcon ?? feeAsset?.icon} boxSize={4} /> + </Box> + ) + }) + }, [groupedAssetBalances?.relatedAssets, assets])Add this snippet above to make it reactive:
// subscribe to fee assets reactively const feeAssets = useAppSelector(state => groupedAssetBalances?.relatedAssets.map( a => selectFeeAssetByChainId(state, assets[a.assetId]?.chainId ?? ''), ) ?? [], )
🧹 Nitpick comments (27)
src/pages/Dashboard/components/DashboardHeader/WalletBalance.tsx (2)
50-61: Confirm TooltipWithTouch forwards layout props; if not, wrap it to ensure positioning appliesIf TooltipWithTouch doesn’t spread unknown props onto its root wrapper,
position/right/topwon’t take effect. Safe alternative:- <TooltipWithTouch - label={translate('defi.loadingAccounts', { - portfolioAccountsLoaded: Object.keys(walletAccounts).length, - })} - position='absolute' - right={-8} - top={1} - > - <Spinner color='blue.500' size='sm' /> - </TooltipWithTouch> + <Box position='absolute' right={-8} top={1}> + <TooltipWithTouch + label={translate('defi.loadingAccounts', { + portfolioAccountsLoaded: Object.keys(walletAccounts).length, + })} + > + <Spinner color='blue.500' size='sm' /> + </TooltipWithTouch> + </Box>
56-56: Prefer logical CSS property for RTL safetyUse
insetInlineEndinstead ofright.- right={-8} + insetInlineEnd={-8}src/components/ReactTable/InfiniteTable.tsx (3)
232-241: Use accessible tabIndex (avoid index-based tab order).tabIndex should be 0 (focusable in natural order) or -1 (programmatic focus only). Index-based values create erratic tab order.
- tabIndex={row?.index} + tabIndex={0}
244-254: Guard data-test prefix to avoid “undefined-” keys.Prevent emitting unstable/invalid test ids when rowDataTestPrefix is not provided.
- {...(row && rowDataTestKey - ? { - 'data-test': `${rowDataTestPrefix}-${String( - row.original?.[rowDataTestKey] ?? '', - ) - .split(' ') - .join('-') - .toLowerCase()}`, - } - : {})} + {...(row && rowDataTestKey + ? { + 'data-test': `${rowDataTestPrefix ? `${rowDataTestPrefix}-` : ''}${String( + row.original?.[rowDataTestKey] ?? '', + ) + .split(' ') + .join('-') + .toLowerCase()}`, + } + : {})}
124-134: Avoid dual expansion states (single source of truth).You manage expansion via expandedItems; calling row.toggleRowExpanded adds a second, unused state. Remove it to prevent divergence.
const handleRowClick = useCallback( (row: Row<T>) => { if (renderSubComponent) { - row.toggleRowExpanded?.() handleRowToggle(row.id) } onRowClick?.(row) },src/components/AssetSearch/components/GroupedAssetRow.tsx (1)
208-213: Minor: avoid redundant toString().fiatAmount is already a string; pass it directly.
- <Amount.Fiat - color='var(--chakra-colors-chakra-body-text)' - value={groupedAssetBalances?.primaryAsset.fiatAmount.toString()} - /> + <Amount.Fiat color='var(--chakra-colors-chakra-body-text)' value={groupedAssetBalances?.primaryAsset.fiatAmount} />src/components/TradeAssetSearch/hooks/useAssetSearchWorker.ts (2)
73-80: Use structured logging instead of console.error.Adopt the app’s logger (with context) to align with logging standards.
- worker.onerror = error => { - console.error('Worker error:', error) + worker.onerror = error => { + logger.error({ error }, 'assetSearch worker error') setWorkerSearchState(prev => ({ ...prev, workerState: 'failed' })) } } catch (error) { - console.error('Failed to initialize worker:', error) + logger.error({ error }, 'assetSearch worker init failed') setWorkerSearchState(prev => ({ ...prev, workerState: 'failed' })) }If no logger exists here, import your shared logger utility.
90-105: Optional: batch worker updates into a single message.Sending assets and related ids together reduces message churn and race windows.
- workerRef.current.postMessage({ - type: 'updateAssets', - payload: { assets: assets.map(a => pick(a, ['assetId', 'name', 'symbol', 'chainId'])) }, - }) - workerRef.current.postMessage({ - type: 'updateRelatedAssetIds', - payload: { relatedAssetIdsById }, - }) + workerRef.current.postMessage({ + type: 'updateIndex', + payload: { + assets: assets.map(a => pick(a, ['assetId', 'name', 'symbol', 'chainId'])), + relatedAssetIdsById, + }, + })Note: requires the worker to support a unified updateIndex message.
src/lib/assetSearch/types.ts (1)
23-28: Reuse shared type alias for related-asset mapping to avoid drift.Use the central alias from types (AssetsByIdRelatedAssetKey) instead of repeating
Record<AssetId, AssetId[]>.Apply:
-import type { AssetId, ChainId } from '@shapeshiftoss/caip' +import type { AssetId, ChainId } from '@shapeshiftoss/caip' +import type { AssetsByIdRelatedAssetKey } from '@shapeshiftoss/types' @@ - relatedAssetIdsById: Record<AssetId, AssetId[]> + relatedAssetIdsById: AssetsByIdRelatedAssetKeysrc/pages/Explore/Explore.tsx (2)
208-217: Avoid double scroll by inheriting container height.
height='100vh'inside a container with its own computed height can cause nested scrolling on mobile. Preferheight='100%'.Apply:
- height='100vh' + height='100%'
171-174: Avoid synthetic event casting for clearing search.Casting a plain object to
ChangeEvent<HTMLInputElement>is brittle. Prefer exposing asetSearchString('')orhandleReset()from the hook.Example (if you add a setter in the hook):
- const handleClearSearch = useCallback(() => { - handleSearchChange({ target: { value: '' } } as ChangeEvent<HTMLInputElement>) - }, [handleSearchChange]) + const handleClearSearch = useCallback(() => setSearchString(''), [setSearchString])src/components/TradeAssetSearch/hooks/useGetPopularAssetsQuery.tsx (2)
14-22: Type return of queryFn explicitly for clarity.Add an explicit return type to align with TS guidelines.
Apply:
-export const queryFn = async () => { +export const queryFn = async (): Promise<Record<ChainId | 'All', Asset[]>> => {
35-49: Guard against duplicates across chains and in “All”.Pushing the same primary asset into multiple buckets can create duplicates. Deduplicate per bucket.
Apply:
- if (!result[relatedAssetChainId]) result[relatedAssetChainId] = [] - - result[relatedAssetChainId].push(asset) + if (!result[relatedAssetChainId]) result[relatedAssetChainId] = [] + if (!result[relatedAssetChainId].some(a => a.assetId === asset.assetId)) { + result[relatedAssetChainId].push(asset) + } @@ - result[chainId].push(asset) - result.All.push(asset) + if (!result[chainId].some(a => a.assetId === asset.assetId)) result[chainId].push(asset) + if (!result.All.some(a => a.assetId === asset.assetId)) result.All.push(asset)src/components/TradeAssetSearch/workers/assetSearch.worker.ts (2)
52-55: Defensive default when updating related-asset mapGuard against undefined payloads to avoid retaining stale mappings.
- case 'updateRelatedAssetIds': { - RELATED_ASSET_IDS_BY_ASSET_ID = data.payload.relatedAssetIdsById + case 'updateRelatedAssetIds': { + RELATED_ASSET_IDS_BY_ASSET_ID = data.payload.relatedAssetIdsById ?? {} break }
57-65: Log failures with context in the worker catchSilent catch makes diagnosing search failures hard. Log with structured context.
- case 'search': { - try { - handleSearch(data) - } catch { - postMessage({ + case 'search': { + try { + handleSearch(data) + } catch (error) { + // non-fatal: return empty and log for diagnostics + // eslint-disable-next-line no-console + console.debug('assetSearch.worker: search failed', { + requestId: data.requestId, + error, + }) + postMessage({ type: 'searchResult', requestId: data.requestId, payload: { assetIds: [] }, }) } break }src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsx (2)
76-99: Restore long-press support and drop unused data fieldPass a long-press handler through to AssetRow, and remove the unused assets array from itemData.
- const itemData = { - assets: [asset], - handleClick: onAssetClick, - disableUnsupported: false, - hideZeroBalanceAmounts, - onImportClick, - } + const itemData = { + handleClick: onAssetClick, + disableUnsupported: false, + hideZeroBalanceAmounts, + onImportClick, + // handleLongPress?: add when available in props + } @@ - <AssetRow + <AssetRow asset={asset} index={index} // eslint-disable-next-line react-memo/require-usememo data={itemData} onImportClick={onImportClick} showRelatedAssets />If this list should support long-press, add an optional onLongPress prop to GroupedAssetListProps and thread it into itemData as handleLongPress.
102-110: Virtualization tuning: consider aligning overscan with AssetListAssetList uses overscan={1000}. Using 200 here may cause more re-mounts and contribute to the image flicker noted in the PR. Consider increasing to match AssetList or make it configurable.
src/lib/assetSearch/utils.ts (1)
22-30: Option type could be looser for convenienceConsider typing relatedAssetIdsById as Partial<Record<AssetId, readonly AssetId[]>> to reflect sparse maps and allow readonly arrays.
src/components/AssetSearch/components/AssetList.tsx (1)
145-151: Provide stable item keys to prevent row remounts/flickerVirtuoso currently keys by index; when the data reorders, rows can remount (icon flicker). Provide a stable key using assetId.
Apply:
return ( <Virtuoso data={assets} itemContent={renderRow} style={virtuosoStyle} overscan={1000} + itemKey={(index) => assets[index]?.assetId ?? index} increaseViewportBy={INCREASE_VIEWPORT_BY} /> )src/pages/Dashboard/components/AccountList/AccountTable.tsx (2)
150-159: Hide toggle when a group has a single related assetThe toggle icon shows for all non-unique rows, even when the subcomponent renders nothing (single related asset). Optionally gate the icon on “has >1 related assets” to reduce visual noise.
Example (if you have a selector handy):
- Cell: ({ row }: { row: RowProps }) => { - if (isUniqueAsset(row.original.relatedAssetKey)) return null - return row.isExpanded ? <ChevronUpIcon /> : <ChevronDownIcon /> - }, + Cell: ({ row }: { row: RowProps }) => { + if (isUniqueAsset(row.original.relatedAssetKey)) return null + // optionally check related count and early-return null when === 1 + return row.isExpanded ? <ChevronUpIcon /> : <ChevronDownIcon /> + },
250-259: Mobile height calc: duplicate safe-area termenv(safe-area-inset-top) is subtracted twice. Trim the duplicate to avoid undersizing on some devices.
Apply:
- height='calc(100vh - var(--mobile-header-offset) - var(--safe-area-inset-bottom) - var(--safe-area-inset-top) - env(safe-area-inset-bottom) - env(safe-area-inset-top) - env(safe-area-inset-top) - var(--mobile-nav-offset) - 54px)' + height='calc(100vh - var(--mobile-header-offset) - var(--safe-area-inset-bottom) - var(--safe-area-inset-top) - env(safe-area-inset-bottom) - env(safe-area-inset-top) - var(--mobile-nav-offset) - 54px)'src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (1)
61-62: Remove debug log.Leftover
console.log('rows', rows)is noisy and risks leaking PII in production logs.- console.log('rows', rows)src/components/AssetSearch/components/AssetRow.tsx (1)
200-210: Prevent parent props from accidentally overriding onClick; keep long-press handlers last.Spread
...propsbefore controlled props, and keep...longPressHandlers(asset)last so handlers win when both are present.return ( <Button variant='ghost' - onClick={handleOnClick} justifyContent='space-between' isDisabled={!isSupported && disableUnsupported} _focus={focus} width='100%' - py={8} - {...props} - {...longPressHandlers(asset)} + {...props} + onClick={handleOnClick} + py={8} + {...longPressHandlers(asset)} >src/state/slices/portfolioSlice/selectors.ts (1)
809-826: Guard against missing groups and simplify accumulation.
- Default
allRelatedAssetIdsto at least[primaryAssetId]to avoid edge-caseundefined.- Drop the always-truthy check around
cryptoBalance(it's a string like'0').- const primaryAccountRowsWithAggregatedBalances = primaryAccountRows.reduce<AccountRowData[]>( + const primaryAccountRowsWithAggregatedBalances = primaryAccountRows.reduce<AccountRowData[]>( (acc, { assetId: primaryAssetId }) => { const primaryAsset = assets[primaryAssetId] - const allRelatedAssetIds = relatedAssetIdsByAssetId[primaryAssetId] + const allRelatedAssetIds = relatedAssetIdsByAssetId[primaryAssetId] ?? [primaryAssetId] let totalCryptoBalance = bnOrZero(0) Object.values(accountBalancesById).forEach(accountBalances => { allRelatedAssetIds.forEach(relatedAssetId => { const relatedAsset = assets[relatedAssetId] const balance = accountBalances[relatedAssetId] const cryptoBalance = fromBaseUnit(bnOrZero(balance), relatedAsset?.precision ?? 0) - - if (cryptoBalance) { - totalCryptoBalance = totalCryptoBalance.plus(bnOrZero(cryptoBalance)) - } + totalCryptoBalance = totalCryptoBalance.plus(bnOrZero(cryptoBalance)) }) })src/state/slices/common-selectors.ts (2)
95-107: Consider memoization strategy for the zero-inclusive selectorThe new
selectPortfolioAssetBalancesBaseUnitIncludingZeroBalancesselector computes aggregated balances for all assets. Since this is used as an input toselectPortfolioAssetBalancesBaseUnit, consider whether the deep equality check is sufficient or if additional memoization might be needed for large portfolios.
370-387: Optimize chain filtering logic for primary assetsBoth
selectPortfolioPrimaryAssetsByChainandselectPrimaryAssetsByChainuse similar filtering logic. Consider extracting a shared utility function to reduce duplication and ensure consistency.Consider extracting a utility function:
const filterAssetsByChain = ( assets: Asset[], relatedAssetIdsById: AssetsByIdRelatedAssetKey, chainId: ChainId | 'All' ): Asset[] => { if (chainId === 'All') return assets return assets.filter(asset => { if (asset.chainId === chainId) return true return ( relatedAssetIdsById[asset.assetId]?.some( relatedAssetId => fromAssetId(relatedAssetId).chainId === chainId, ) ?? false ) }) }src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx (1)
294-294: Consider constraining the maxWidth valueThe
maxWidth='100%'on the outer Flex container might cause layout issues on very wide screens. Consider using a responsive value or a max pixel width to maintain proper content boundaries.- <Flex flexDir='column' height='100%' flex='1' minHeight={0} maxWidth='100%'> + <Flex flexDir='column' height='100%' flex='1' minHeight={0} maxWidth={{ base: '100%', lg: '600px' }}>
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
Outdated
Show resolved
Hide resolved
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
Outdated
Show resolved
Hide resolved
NeOMakinG
force-pushed
the
grouped-assets
branch
from
9631f44 to
2447eb5
Compare
|
thanks for the big review @gomesalexandre as always Should be good now hopefully, I fixed everything noticed minus the flashing of row opening on account/assets page as I took ilke 3-4 hours trying to find a fix and couldn't find one, I would love to give this a try as a follow up later |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/lib/assetSearch/utils.ts (1)
55-66: Fix chain filter bug: don't gate related-asset match on walletConnectedChainIdsWhen activeChainId is set, inclusion should depend only on the presence of a related asset on that activeChainId. The prior guard already rejects unsupported activeChainId when allowWalletUnsupportedAssets is false. Remove the walletConnectedChainIds check to avoid false negatives. This was previously requested and remains unfixed.
- if ( - activeChainId && - relatedAssetIdsById[asset.assetId]?.some(relatedAssetId => { - const relatedAssetChainId = fromAssetId(relatedAssetId).chainId - - return ( - walletConnectedChainIds.includes(relatedAssetChainId) && - activeChainId === relatedAssetChainId - ) - }) - ) - return true + if ( + activeChainId && + (relatedAssetIdsById[asset.assetId]?.some( + relatedAssetId => fromAssetId(relatedAssetId).chainId === activeChainId, + ) ?? false) + ) + return truesrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (2)
123-132: Chevron should appear on primary rows (group roots), not be hidden.Render the toggle only for primary rows so users can discover expansion.
- Cell: ({ row }: { row: Row<Asset> }) => { - if (isUniqueAsset(row.original.relatedAssetKey)) return null - - return row.isExpanded ? <ChevronUpIcon /> : <ChevronDownIcon /> - }, + Cell: ({ row }: { row: Row<Asset> }) => { + if (!isPrimaryAsset(row.original.relatedAssetKey, row.original.assetId)) return null + return row.isExpanded ? <ChevronUpIcon /> : <ChevronDownIcon /> + },
137-143: Fix: Clicking primary (grouped) rows should not navigate.Clicking a primary row both expands (handled by InfiniteTable) and navigates due to this guard. Use the shared primary predicate.
- const handleRowClick = useCallback( - (row: Row<Asset>) => { - if (!isUniqueAsset(row.original.relatedAssetKey)) return - onRowClick(row.original) - }, - [onRowClick], - ) + const handleRowClick = useCallback( + (row: Row<Asset>) => { + if (isPrimaryAsset(row.original.relatedAssetKey, row.original.assetId)) return + onRowClick(row.original) + }, + [onRowClick], + )
🧹 Nitpick comments (3)
src/lib/assetSearch/utils.ts (1)
28-29: Tighten type of relatedAssetIdsById to readonlyWe never mutate this mapping; using readonly prevents accidental writes and communicates intent.
- relatedAssetIdsById: Record<AssetId, AssetId[]> + relatedAssetIdsById: Readonly<Record<AssetId, ReadonlyArray<AssetId>>>src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (2)
61-62: Remove leftover debug log.Avoid noisy console output in production.
- console.log('rows', rows)
37-44: Type visibleAssetIds as Set for accuracy.Keeps types aligned end-to-end with useFetchFiatAssetMarketData.
- const [visibleAssetIds, setVisibleAssetIds] = useState<Set<string>>(new Set()) + const [visibleAssetIds, setVisibleAssetIds] = useState<Set<AssetId>>(new Set())Also update the import:
-import type { Asset } from '@shapeshiftoss/types' +import type { Asset, AssetId } from '@shapeshiftoss/types'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx(7 hunks)src/components/TradeAssetSearch/workers/assetSearch.worker.ts(5 hunks)src/lib/assetSearch/utils.ts(2 hunks)src/state/slices/common-selectors.ts(8 hunks)src/state/slices/portfolioSlice/selectors.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/TradeAssetSearch/workers/assetSearch.worker.ts
- src/state/slices/common-selectors.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsxsrc/state/slices/portfolioSlice/selectors.tssrc/lib/assetSearch/utils.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.tsx: ALWAYS wrap components in error boundaries
ALWAYS provide user-friendly fallback components in error boundaries
ALWAYS log errors for debugging in error boundaries
ALWAYS use useErrorToast hook for displaying errors
ALWAYS provide translated error messages in error toasts
ALWAYS handle different error types appropriately in error toasts
Missing error boundaries in React components is an anti-pattern
**/*.tsx: ALWAYS use PascalCase for React component names
ALWAYS use descriptive names that indicate the component's purpose
ALWAYS match the component name to the file name
Flag components without PascalCase
Flag default exports for components
Files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsxsrc/state/slices/portfolioSlice/selectors.tssrc/lib/assetSearch/utils.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
**/*.{tsx,jsx}: ALWAYS useuseMemofor expensive computations, object/array creations, and filtered data
ALWAYS useuseMemofor derived values and computed properties
ALWAYS useuseMemofor conditional values and simple transformations
ALWAYS useuseCallbackfor event handlers and functions passed as props
ALWAYS useuseCallbackfor any function that could be passed as a prop or dependency
ALWAYS include all dependencies inuseEffect,useMemo,useCallbackdependency arrays
NEVER use// eslint-disable-next-line react-hooks/exhaustive-depsunless absolutely necessary
ALWAYS explain why dependencies are excluded if using eslint disable
ALWAYS use named exports for components
NEVER use default exports for components
KEEP component files under 200 lines when possible
BREAK DOWN large components into smaller, reusable pieces
EXTRACT complex logic into custom hooks
USE local state for component-level state
LIFT state up when needed across multiple components
USE Context for avoiding prop drilling
ALWAYS wrap components in error boundaries for production
ALWAYS handle async errors properly
ALWAYS provide user-friendly error messages
ALWAYS use virtualization for lists with 100+ items
ALWAYS implement proper key props for list items
ALWAYS lazy load heavy components
ALWAYS use React.lazy for code splitting
Components receiving props are wrapped withmemo
Files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsxsrc/state/slices/portfolioSlice/selectors.tssrc/lib/assetSearch/utils.ts
🧠 Learnings (29)
📓 Common learnings
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/lib/utils/asset.ts:3-5
Timestamp: 2025-09-04T13:00:47.724Z
Learning: In shapeshift/web, primary assets are assets that represent a group of related assets across multiple chains. An asset with undefined relatedAssetKey doesn't participate in grouping and is not considered primary. Only assets with null relatedAssetKey or where relatedAssetKey === assetId are considered primary assets.
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
📚 Learning: 2025-07-30T20:57:48.176Z
Learnt from: NeOMakinG
PR: shapeshift/web#10148
File: src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx:88-91
Timestamp: 2025-07-30T20:57:48.176Z
Learning: In src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx, NeOMakinG prefers keeping the hardcoded overscan value (40) over dynamic calculation based on viewport height, prioritizing code simplicity over precision when the current approach works effectively.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-08T15:00:49.887Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsxsrc/lib/assetSearch/utils.ts
📚 Learning: 2025-08-27T09:47:06.275Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/components/ButtonWalletPredicate/ButtonWalletPredicate.tsx:7-7
Timestamp: 2025-08-27T09:47:06.275Z
Learning: In shapeshift/web project, NeOMakinG consistently prefers to defer UI/UX improvements and refactoring work (like the Drawer.Close hack fix in ButtonWalletPredicate.tsx) to follow-up PRs rather than expanding the scope of feature PRs, even when the improvements would enhance robustness.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsxsrc/lib/assetSearch/utils.ts
📚 Learning: 2025-08-08T15:00:22.321Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx:14-14
Timestamp: 2025-08-08T15:00:22.321Z
Learning: In shapeshift/web reviews for NeOMakinG, avoid nitpicks to change deep-relative imports to '@/…' alias paths within feature/non-refactor PRs; defer such style-only changes to a dedicated follow-up refactor unless they fix an issue.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-02T12:38:46.940Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/Table/Table.theme.ts:177-180
Timestamp: 2025-09-02T12:38:46.940Z
Learning: NeOMakinG prefers to defer technical debt and CSS correctness issues (like improper hover selectors) to follow-up PRs when the current PR is already large and focused on major feature implementation, even when the issues are valid from a usability/technical perspective.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-22T13:00:44.879Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-27T09:51:36.901Z
Learnt from: NeOMakinG
PR: shapeshift/web#10376
File: packages/contracts/src/viemClient.ts:39-40
Timestamp: 2025-08-27T09:51:36.901Z
Learning: NeOMakinG confirmed that issues flagged by CodeRabbit that predate the PR changes and are unrelated to the PR's core objective should be addressed separately, not within the current PR scope.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsxsrc/lib/assetSearch/utils.ts
📚 Learning: 2025-09-03T21:17:27.681Z
Learnt from: gomesalexandre
PR: shapeshift/web#10418
File: src/Routes/RoutesCommon.tsx:231-267
Timestamp: 2025-09-03T21:17:27.681Z
Learning: gomesalexandre prefers to keep PR diffs focused and reasonable in size, deferring tangential improvements (like Mixpanel privacy enhancements) to separate efforts rather than expanding the scope of feature PRs.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-02T08:34:08.157Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/pages/Dashboard/components/AccountList/AccountTable.tsx:60-0
Timestamp: 2025-09-02T08:34:08.157Z
Learning: NeOMakinG prefers code review comments to focus only on actual PR changes, not pre-existing code issues, unless there are critical security or correctness concerns directly related to the new functionality.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-25T12:59:43.842Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/Fox/components/RFOXSection.tsx:234-248
Timestamp: 2025-08-25T12:59:43.842Z
Learning: NeOMakinG considers suggestions to refactor working modal state management from useState to useDisclosure as too nitpicky, preferring to keep such cosmetic code improvements out of feature PRs unless they address actual issues.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-08T14:59:40.422Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/pages/Explore/ExploreCategory.tsx:231-238
Timestamp: 2025-08-08T14:59:40.422Z
Learning: In src/pages/Explore/ExploreCategory.tsx, for the PageHeader filter trigger, NeOMakinG considers changing a clickable Chakra Icon to IconButton too nitpicky for this PR and prefers to keep the current Icon-based trigger; such minor a11y/UI nitpicks should be deferred to a follow-up if needed.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-04T17:35:57.546Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/common-selectors.ts:164-178
Timestamp: 2025-09-04T17:35:57.546Z
Learning: In shapeshift/web, while the Asset type allows relatedAssetKey to be AssetId | null | undefined, the actual asset generation and processing system ensures relatedAssetKey is never undefined in practice. All asset creation utilities explicitly set it to either a specific AssetId (for grouped assets) or null (for primary/unique assets). The only undefined assignments occur during intermediate processing phases and get resolved during the final asset generation step.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsxsrc/lib/assetSearch/utils.ts
📚 Learning: 2025-09-04T17:35:57.545Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/common-selectors.ts:164-178
Timestamp: 2025-09-04T17:35:57.545Z
Learning: In shapeshift/web, while the Asset type allows relatedAssetKey to be AssetId | null | undefined, the actual asset generation and processing system ensures relatedAssetKey is never undefined in practice. All asset creation utilities explicitly set it to either a specific AssetId (for grouped assets) or null (for primary/unique assets). The only exception is temporary undefined assignment during portal asset processing, which gets resolved later in the pipeline.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-04T13:00:47.724Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/lib/utils/asset.ts:3-5
Timestamp: 2025-09-04T13:00:47.724Z
Learning: In shapeshift/web, primary assets are assets that represent a group of related assets across multiple chains. An asset with undefined relatedAssetKey doesn't participate in grouping and is not considered primary. Only assets with null relatedAssetKey or where relatedAssetKey === assetId are considered primary assets.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-04T17:27:33.287Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/TradeAssetSearch/components/SearchTermAssetList.tsx:88-94
Timestamp: 2025-09-04T17:27:33.287Z
Learning: In shapeshift/web, NFTs never have related assets in the grouping system. The related assets feature is designed for fungible tokens that exist across multiple chains (like USDC on different chains), not for NFTs which are unique. Therefore, checking isNft() for related asset IDs is unnecessary since NFT asset IDs won't be present in the relatedAssetIdsById mapping.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsxsrc/lib/assetSearch/utils.ts
📚 Learning: 2025-09-04T13:22:35.355Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/assetsSlice/selectors.ts:45-51
Timestamp: 2025-09-04T13:22:35.355Z
Learning: In shapeshift/web, the isPrimaryAsset function in src/lib/utils/asset.ts is intentionally designed to only treat null relatedAssetKey as primary (not undefined). The asset generation process specifically sets relatedAssetKey to null for primary assets, while undefined means the asset doesn't participate in grouping and should not be considered primary. Only assets with null relatedAssetKey or where relatedAssetKey === assetId are primary assets.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-25T23:32:13.876Z
Learnt from: premiumjibles
PR: shapeshift/web#10361
File: src/pages/Markets/components/CardWithSparkline.tsx:83-92
Timestamp: 2025-08-25T23:32:13.876Z
Learning: In shapeshift/web PR #10361, premiumjibles considered the nested button accessibility issue (ChartErrorFallback retry Button inside Card rendered as Button in CardWithSparkline.tsx) out of scope for the error boundaries feature PR, consistent with deferring minor a11y improvements to follow-up PRs.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-03T20:56:35.789Z
Learnt from: gomesalexandre
PR: shapeshift/web#10418
File: src/components/Layout/Header/NavBar/ShapeShiftMenu.tsx:1-15
Timestamp: 2025-09-03T20:56:35.789Z
Learning: gomesalexandre dismisses accessibility improvements with "meh accessibility", preferring to keep existing implementations over making elements properly accessible in feature PRs.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-04T17:29:59.454Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/TradeAssetSearch/hooks/useGetPopularAssetsQuery.tsx:28-33
Timestamp: 2025-09-04T17:29:59.454Z
Learning: In shapeshift/web, the useGetPopularAssetsQuery function in src/components/TradeAssetSearch/hooks/useGetPopularAssetsQuery.tsx intentionally uses primaryAssets[assetId] instead of falling back to assets[assetId]. The design distributes primary assets across chains by iterating through their related assets and adding the primary asset to each related asset's chain. This ensures primary assets appear in all chains where they have related assets, supporting the grouped asset system.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsxsrc/lib/assetSearch/utils.ts
📚 Learning: 2025-09-04T17:26:19.399Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/PriceChangeTag/PriceChangeTag.tsx:25-36
Timestamp: 2025-09-04T17:26:19.399Z
Learning: In the shapeshift/web project, NeOMakinG prefers to hide 0% price changes in the PriceChangeTag component rather than displaying "0.00%", so the falsy check `if (!changePercent24Hr) return null` is intentional design behavior.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-10T22:06:16.818Z
Learnt from: premiumjibles
PR: shapeshift/web#10216
File: src/pages/Dashboard/components/AccountList/AccountTable.tsx:183-189
Timestamp: 2025-08-10T22:06:16.818Z
Learning: In the ShapeShift web codebase, haptic feedback for long press interactions (via `pulseAndroid()`) is handled centrally within the table components (`InfiniteTable`, `ReactTable`, `ReactTableNoPager`, `MarketsTableVirtualized`) using the `useLongPress` hook. Parent components that pass `onRowLongPress` callbacks should not add their own haptic feedback calls as this would cause duplicate vibrations.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-03T22:10:38.426Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/react-best-practices.mdc:0-0
Timestamp: 2025-08-03T22:10:38.426Z
Learning: Applies to **/*.{tsx,jsx} : ALWAYS use virtualization for lists with 100+ items
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts expects a filter object with an assetId property, not a raw AssetId string. The selector signature is (state: ReduxState, filter) where filter should have an assetId property. This pattern is consistent across portfolio selectors that use selectAssetIdParamFromFilter. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/state/slices/portfolioSlice/selectors.tssrc/lib/assetSearch/utils.ts
📚 Learning: 2025-09-04T10:18:34.113Z
Learnt from: gomesalexandre
PR: shapeshift/web#10427
File: src/hooks/useActionCenterSubscribers/useSwapActionSubscriber.tsx:40-40
Timestamp: 2025-09-04T10:18:34.113Z
Learning: In the shapeshift/web codebase, src/state/slices/selectors.ts uses wildcard exports (`export * from './[sliceName]/selectors'`) to re-export all selectors from individual slice selector files, making them available through the barrel import. This means selectors like selectTxByFilter from txHistorySlice/selectors are properly accessible via '@/state/slices/selectors' even though they don't appear in explicit named exports.
Applied to files:
src/state/slices/portfolioSlice/selectors.ts
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts accepts a filter object with an assetId property, not a raw AssetId string. The selector signature is (state: ReduxState, filter) where filter is expected to have an assetId property. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/state/slices/portfolioSlice/selectors.tssrc/lib/assetSearch/utils.ts
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts accepts a filter object with an assetId property (signature: (state, { assetId })), not a raw AssetId string. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/state/slices/portfolioSlice/selectors.ts
📚 Learning: 2025-08-10T21:09:25.643Z
Learnt from: premiumjibles
PR: shapeshift/web#10215
File: src/components/MultiHopTrade/hooks/useGetTradeRateInput.ts:65-67
Timestamp: 2025-08-10T21:09:25.643Z
Learning: In the MultiHopTrade components, `selectInputBuyAsset` and `selectInputSellAsset` selectors from `tradeInputSlice` always return defined values because they have default values in the initial state (BTC for buyAsset, ETH for sellAsset, with fallback to defaultAsset). Null checks for these assets are unnecessary when using these selectors.
Applied to files:
src/state/slices/portfolioSlice/selectors.ts
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Verify chain ID filtering in filterAssetIdsBySellable and filterBuyAssetsBySellAssetId methods.
Applied to files:
src/lib/assetSearch/utils.ts
🧬 Code graph analysis (3)
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (6)
packages/types/src/base.ts (1)
Asset(69-89)src/state/apis/fiatRamps/hooks.ts (1)
useFetchFiatAssetMarketData(8-20)src/lib/utils/asset.ts (2)
isUniqueAsset(7-9)isPrimaryAsset(3-5)src/components/MarketTableVirtualized/GroupedAssets.tsx (1)
GroupedAssets(65-85)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(39-153)src/components/ReactTable/InfiniteTable.tsx (1)
InfiniteTable(47-317)
src/state/slices/portfolioSlice/selectors.ts (8)
src/state/selector-utils.ts (1)
createDeepEqualOutputSelector(5-7)src/state/slices/assetsSlice/selectors.ts (1)
selectAssets(40-43)src/state/slices/marketDataSlice/selectors.ts (1)
selectMarketDataUserCurrency(19-55)src/state/slices/common-selectors.ts (3)
selectPortfolioAssetBalancesBaseUnitIncludingZeroBalances(95-107)selectPortfolioAccountBalancesBaseUnit(86-93)selectRelatedAssetIdsByAssetIdInclusive(164-178)src/state/slices/preferencesSlice/preferencesSlice.ts (1)
preferences(239-369)src/lib/math.ts (1)
fromBaseUnit(10-24)src/state/reducer.ts (1)
ReduxState(194-194)packages/caip/src/assetId/assetId.ts (1)
AssetId(17-17)
src/lib/assetSearch/utils.ts (1)
packages/caip/src/assetId/assetId.ts (2)
AssetId(17-17)fromAssetId(143-178)
⏰ 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). (1)
- GitHub Check: Install and Cache
🔇 Additional comments (9)
src/lib/assetSearch/utils.ts (1)
31-36: Default activeChainId to 'All' to avoid filtering everything when undefinedIf callers omit activeChainId, the current final return filters out all assets. Defaulting avoids surprise behavior and matches the "All" semantics.
- const { - activeChainId, + const { + activeChainId = 'All', allowWalletUnsupportedAssets, walletConnectedChainIds, relatedAssetIdsById, } = optionssrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (1)
162-175: Mobile path looks good.Using AssetList with showRelatedAssets + showPrice is aligned with the grouped-asset UX on small screens.
src/state/slices/portfolioSlice/selectors.ts (7)
19-19: Type-only import is fine.No issues with bringing in
Rowfromreact-table.
26-26: Good reuse of common selectors.Importing
selectPortfolioAssetBalancesBaseUnitIncludingZeroBalancesandselectRelatedAssetIdsByAssetIdInclusivematches the grouping requirements.Also applies to: 29-29
57-57: Utility import looks appropriate.
isSomeis used correctly for filtering nullable rows.
741-742: AccountRowData: relatedAssetKey addition is correct.Type
string | null | undefinedaligns with primary/related semantics in this repo.
744-747: GroupedAssetBalance type LGTM.Clear contract for grouped primary + related assets.
859-937: Apply essential refactor for grouping precision, performance, and non-primary support
- Build a
MapfromaccountRowsto avoid repeated.findcalls.- Normalize the input ID to its group key (
relatedAssetKey ?? assetId) so non-primary IDs return their full group.- Preserve full precision on crypto totals by removing the fixed “2” decimals.
export const selectGroupedAssetBalances = createCachedSelector( selectPortfolioAccountRows, selectRelatedAssetIdsByAssetIdInclusive, selectAssets, selectMarketDataUserCurrency, (_state: ReduxState, primaryAssetId: AssetId) => primaryAssetId, ( accountRows, relatedAssetIdsByAssetId, assetsById, marketData, primaryAssetId, ): GroupedAssetBalance | null => { - const primaryAsset = assetsById[primaryAssetId] - const primaryRow = accountRows.find(row => row.assetId === primaryAssetId) ?? { + const rowsByAssetId = new Map(accountRows.map(r => [r.assetId, r])) + const primaryAsset = assetsById[primaryAssetId] + const groupKey = primaryAsset?.relatedAssetKey ?? primaryAssetId + const primaryRow = rowsByAssetId.get(groupKey) ?? { assetId: primaryAssetId, - name: primaryAsset?.name ?? '', - icon: primaryAsset?.icon ?? '', - symbol: primaryAsset?.symbol ?? '', + name: (assetsById[groupKey] ?? primaryAsset)?.name ?? '', + icon: (assetsById[groupKey] ?? primaryAsset)?.icon ?? '', + symbol: (assetsById[groupKey] ?? primaryAsset)?.symbol ?? '', fiatAmount: '0', cryptoAmount: '0', allocation: 0, - price: marketData[primaryAssetId]?.price ?? '0', - priceChange: marketData[primaryAssetId]?.changePercent24Hr ?? 0, - relatedAssetKey: assetsById[primaryAssetId]?.relatedAssetKey ?? null, + price: marketData[groupKey]?.price ?? '0', + priceChange: marketData[groupKey]?.changePercent24Hr ?? 0, + relatedAssetKey: assetsById[groupKey]?.relatedAssetKey ?? null, } - const allRelatedAssetIds = relatedAssetIdsByAssetId[primaryAssetId] || [] + const allRelatedAssetIds = relatedAssetIdsByAssetId[groupKey] ?? [primaryAssetId] const relatedAssets = allRelatedAssetIds .map(assetId => { - const row = accountRows.find(row => row.assetId === assetId) + const row = rowsByAssetId.get(assetId) const asset = assetsById[assetId] if (!row && !asset) return null @@ - const totalFiatBalance = allRelatedAssetIds - .reduce((sum, assetId) => { - const row = accountRows.find(row => row.assetId === assetId) - return sum.plus(row?.fiatAmount ?? '0') - }, bnOrZero(0)) - .toFixed(2) + const totalFiatBalance = allRelatedAssetIds + .reduce((sum, assetId) => sum.plus(rowsByAssetId.get(assetId)?.fiatAmount ?? '0'), bnOrZero(0)) + .toFixed(2) - const totalCryptoBalance = allRelatedAssetIds - .reduce((sum, assetId) => { - const row = accountRows.find(row => row.assetId === assetId) - return sum.plus(row?.cryptoAmount ?? '0') - }, bnOrZero(0)) - .toFixed(2) + const totalCryptoBalance = allRelatedAssetIds + .reduce((sum, assetId) => sum.plus(rowsByAssetId.get(assetId)?.cryptoAmount ?? '0'), bnOrZero(0)) + .toFixed() return { primaryAsset: { ...primaryRow, fiatAmount: totalFiatBalance, cryptoAmount: totalCryptoBalance, }, relatedAssets, } }, )((_state: ReduxState, primaryAssetId: AssetId) => primaryAssetId)Also verify that all current call sites (e.g. GroupedAccounts, GroupedAssets, GroupedAssetRow) pass an ID that, when normalized via its
relatedAssetKey, yields the intended primary group.
729-729: Keep RowProps alias – it’s imported and used in src/pages/Dashboard/components/AccountList/AccountTable.tsx (e.g.import type { AccountRowData, RowProps } from '@/state/slices/selectors'), so it isn’t dead.
| } | ||
|
|
||
| export const selectPortfolioAccountRows = createDeepEqualOutputSelector( | ||
| selectAssets, | ||
| selectMarketDataUserCurrency, | ||
| selectPortfolioAssetBalancesBaseUnit, | ||
| selectPortfolioAssetBalancesBaseUnitIncludingZeroBalances, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard allocation against divide-by-zero.
If total portfolio fiat is 0, current math risks NaN/Infinity; default allocation to 0.
Apply:
- const allocation = bnOrZero(userCurrencyAmount.toFixed(2))
- .div(bnOrZero(totalPortfolioUserCurrencyBalance))
- .times(100)
- .toNumber()
+ const total = bnOrZero(totalPortfolioUserCurrencyBalance)
+ const allocation = total.gt(0)
+ ? bnOrZero(userCurrencyAmount.toFixed(2)).div(total).times(100).toNumber()
+ : 0Also applies to: 754-754, 778-779
🤖 Prompt for AI Agents
In src/state/slices/portfolioSlice/selectors.ts around lines 752, 754 and
778-779, the allocation math can divide by total portfolio fiat which may be 0;
update the selectors to guard against divide-by-zero by checking if
totalPortfolioFiat is 0 (or <= 0) and return 0 for the allocation in that case,
otherwise compute allocation as before; apply the same guard to the other
occurrences on lines 754 and 778-779 so any division uses a conditional (if
total === 0 return 0) or a safe denominator before dividing.
5917175 to
8974452
Compare
packages/types/src/base.ts
Outdated
| @@ -90,6 +90,7 @@ export type Asset = { | |||
|
|
|||
| export type AssetsById = Record<AssetId, Asset> | |||
| export type AssetsByIdPartial = PartialRecord<AssetId, Asset> | |||
| export type AssetsByIdRelatedAssetKey = Record<AssetId, AssetId[]> | |||
There was a problem hiding this comment.
Since this is only consumed in web, probably should live there, WDYT?
Also confused by the name and what this is supposed to represent, should probably be something like RelatedAssetIdsByAssetId or RelatedAssetIdsById to match selectRelatedAssetIdsByAssetIdInclusive
| @@ -723,6 +726,8 @@ export const selectAccountIdsByAssetIdAboveBalanceThresholdByFilter = createDeep | |||
| : accountIdsAboveThreshold, | |||
| ) | |||
|
|
|||
| export type RowProps = Row<AccountRowData> | |||
There was a problem hiding this comment.
Can we name this AccountRowProps and give it a better, less generic home?
src/lib/utils/asset.ts
Outdated
| @@ -0,0 +1,9 @@ | |||
| import type { AssetId } from '@shapeshiftoss/caip' | |||
|
|
|||
| export const isPrimaryAsset = (relatedAssetKey: AssetId | null | undefined, assetId: AssetId) => { | |||
There was a problem hiding this comment.
Wondering if this could also live in an Asset intersection in store @NeOMakinG ? Not a huge deal to have this but same rationale here, more atomic to have this in the extended Asset type
There was a problem hiding this comment.
Added to both account rows and Asset as we already have things like isPool, it's quite convenient so we don't have to add conditions everywhere and selector is based only on isPrimary
in 09cab7e
There was a problem hiding this comment.
Looking very very g now @NeOMakinG ! Will test at runtime after #10419 gets in
NeOMakinG
force-pushed
the
grouped-assets
branch
from
d97b84c to
e7280c7
Compare
| @@ -85,12 +85,13 @@ export type Asset = { | |||
| explorerTxLink: string | |||
| explorerAddressLink: string | |||
| relatedAssetKey: AssetId | null | undefined | |||
| isPrimary?: boolean | |||
| isChainSpecific?: boolean | |||
There was a problem hiding this comment.
Asset that is only one one chain, it's needed to discriminate as primary assets without related assets might have relatedAssetKey as null
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx (1)
27-36: Wire long-press through to AssetList (fixes reported context-menu breakage)Props don’t expose/passthrough long-press; AssetList supports it. This likely explains the blocking bug noted in review.
export type SearchTermAssetListProps = { isLoading: boolean activeChainId: ChainId | 'All' searchString: string allowWalletUnsupportedAssets: boolean | undefined assetFilterPredicate?: (assetId: AssetId) => boolean onAssetClick: (asset: Asset) => void onImportClick: (asset: Asset) => void + onAssetLongPress?: (asset: Asset) => void workerSearchState: WorkerSearchState } @@ onAssetClick: handleAssetClick, onImportClick, + onAssetLongPress, workerSearchState, }: SearchTermAssetListProps) => { @@ <AssetList assets={searchTermAssets} handleClick={handleAssetClick} + handleLongPress={onAssetLongPress} hideZeroBalanceAmounts={true} onImportClick={onImportClick} showRelatedAssets isLoading={isLoadingCustomTokens || isAssetListLoading || workerSearchState.isSearching} height='70vh' />Also applies to: 199-207
♻️ Duplicate comments (2)
src/state/slices/portfolioSlice/selectors.ts (2)
756-768: Guard allocation against divide‑by‑zeroIf totalPortfolioUserCurrencyBalance is 0, allocation becomes Infinity/NaN.
Apply:
- const allocation = bnOrZero(userCurrencyAmount.toFixed(2)) - .div(bnOrZero(totalPortfolioUserCurrencyBalance)) - .times(100) - .toNumber() + const total = bnOrZero(totalPortfolioUserCurrencyBalance) + const allocation = total.gt(0) + ? bnOrZero(userCurrencyAmount.toFixed(2)).div(total).times(100).toNumber() + : 0
793-861: Aggregate via existing rows to avoid O(accounts × assets) and price divergenceCurrent logic loops over all accounts per primary and multiplies summed crypto by the primary price, which can diverge from selectGroupedAssetBalances (sums fiat per related asset) and is heavier.
Reuse portfolioAccountRows indexed by assetId to sum fiat/crypto across related IDs, then sort. This aligns totals with the grouped selector and reduces complexity.
🧹 Nitpick comments (14)
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (4)
36-43: Stabilize visible rows to avoid redundant market-data fetches and rendersSorting and guarding state updates prevents churn when the visible set hasn’t actually changed.
- const [visibleAssetIds, setVisibleAssetIds] = useState<Set<string>>(new Set()) + const [visibleAssetIds, setVisibleAssetIds] = useState<Set<string>>(new Set()) + const visibleAssetIdsArray = useMemo(() => Array.from(visibleAssetIds).sort(), [visibleAssetIds]) - useFetchFiatAssetMarketData(Array.from(visibleAssetIds)) + useFetchFiatAssetMarketData(visibleAssetIdsArray) - const handleVisibleRowsChange = useCallback((visibleRows: Asset[]) => { - const assetIds = visibleRows.map(row => row.assetId) - setVisibleAssetIds(new Set(assetIds)) - }, []) + const handleVisibleRowsChange = useCallback((visibleRows: Asset[]) => { + const next = new Set(visibleRows.map(row => row.assetId)) + setVisibleAssetIds(prev => { + if (prev.size === next.size && Array.from(next).every(id => prev.has(id))) return prev + return next + }) + }, [])
60-61: Remove stray console.log before mergeAvoid noisy logs in production.
- console.log('rows', rows)
122-131: Show chevron only for expandable (primary) rows to match expansion behaviorUsing isPrimary keeps UI consistent with renderSubComponent and prevents chevrons on non-expandable rows.
- Cell: ({ row }: { row: Row<Asset> }) => { - if (row.original.isChainSpecific) return null - - return row.isExpanded ? <ChevronUpIcon /> : <ChevronDownIcon /> - }, + Cell: ({ row }: { row: Row<Asset> }) => { + if (!row.original.isPrimary) return null + return row.isExpanded ? <ChevronUpIcon /> : <ChevronDownIcon /> + },
176-188: Avoid expanding non-primary rows (prevents empty detail rows/gaps)InfiniteTable toggles expansion for every row when renderSubComponent is provided, even if it returns null for a specific row. This can produce empty “expanded” rows and styling changes on non-primary items. Consider adding an
isRowExpandable: (row) => booleanprop to InfiniteTable and guarding both the toggle and details row with it. If preferred, I can draft that patch in a follow-up.src/state/slices/assetsSlice/types.ts (2)
1-1: Use root CAIP import to avoid ESM/CJS path couplingImporting from dist/cjs can break bundlers and types resolution. Prefer the package root.
-import type { AssetId } from '@shapeshiftoss/caip/dist/cjs' +import type { AssetId } from '@shapeshiftoss/caip'
3-3: Type name LGTM; consider immutabilityAlias reads well and matches selector naming. Optionally freeze the shape to prevent accidental mutation.
-export type RelatedAssetIdsById = Record<AssetId, AssetId[]> +export type RelatedAssetIdsById = Readonly<Record<AssetId, ReadonlyArray<AssetId>>>packages/types/src/base.ts (1)
88-89: Clarify/strengthen new flagsFlags align with grouping semantics. If these are always set by decoders/builders, consider making them non‑optional to simplify consumers; otherwise add brief doc comments stating derivation rules (null => chain‑specific primary; self key => multi‑chain primary; undefined => not grouped).
- isPrimary?: boolean - isChainSpecific?: boolean + /** true when relatedAssetKey is null or equals assetId */ + isPrimary?: boolean + /** true only when relatedAssetKey is null (single-chain primary) */ + isChainSpecific?: booleansrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx (1)
88-95: Avoid hot-path fromAssetId parsing during filteringUse a cheap prefix check; NFTs never appear in relatedAssetIdsById, so no extra isNft needed here.
- return _assets.filter( - asset => - (asset.chainId === activeChainId && !isNft(asset.assetId)) || - relatedAssetIdsById[asset.assetId]?.some( - relatedAssetId => fromAssetId(relatedAssetId).chainId === activeChainId, - ), - ) + return _assets.filter(asset => { + if (asset.chainId === activeChainId && !isNft(asset.assetId)) return true + const relatedIds = relatedAssetIdsById[asset.assetId] + if (!relatedIds?.length) return false + const chainPrefix = `${activeChainId}/` + return relatedIds.some(id => id.startsWith(chainPrefix)) + })src/pages/Dashboard/components/AccountList/AccountTable.tsx (4)
156-165: Show chevron only for expandable (multi‑chain primary) rowsGate the chevron on the actual expandability predicate to avoid displaying it on non-expandable rows.
Apply:
Cell: ({ row }: { row: AccountRowProps }) => { - if (row.original.isChainSpecific) return null - - return row.isExpanded ? <ChevronUpIcon /> : <ChevronDownIcon /> + const isExpandable = row.original.relatedAssetKey === row.original.assetId + if (!isExpandable) return null + return row.isExpanded ? <ChevronUpIcon /> : <ChevronDownIcon /> },
129-131: Comparator should handle equality (return 0) for stable sortingCurrent comparator never returns 0. Use comparedTo for clarity and correctness.
Apply:
- sortType: (a: AccountRowProps, b: AccountRowProps): number => - bnOrZero(a.original.priceChange).gt(bnOrZero(b.original.priceChange)) ? 1 : -1, + sortType: (a: AccountRowProps, b: AccountRowProps): number => { + const cmp = bnOrZero(a.original.priceChange).comparedTo(bnOrZero(b.original.priceChange)) + return cmp === 0 ? 0 : cmp > 0 ? 1 : -1 + },
131-144: Hide 0% change and avoid “red 0%”Aligns with the project’s preference to hide zero changes; otherwise 0 renders as a red decrease.
Apply:
- Cell: ({ value }: { value: number }) => ( - <Stat> - <StatNumber - fontSize='md' - display='flex' - lineHeight='shorter' - alignItems='center' - color={value > 0 ? 'green.500' : 'red.500'} - > - <StatArrow ml={1} type={value > 0 ? 'increase' : 'decrease'} /> - <Amount.Percent value={value * 0.01} /> - </StatNumber> - </Stat> - ), + Cell: ({ value }: { value: number }) => { + if (!value) return null + const isUp = value > 0 + return ( + <Stat> + <StatNumber + fontSize='md' + display='flex' + lineHeight='shorter' + alignItems='center' + color={isUp ? 'green.500' : 'red.500'} + > + <StatArrow ml={1} type={isUp ? 'increase' : 'decrease'} /> + <Amount.Percent value={value * 0.01} /> + </StatNumber> + </Stat> + ) + },
259-265: Nit: de-duplicate safe-area vars in height calcThe expression repeats safe-area insets; consider centralizing this height in a theme token or helper to avoid drift.
Example:
- height='calc(100vh - var(--mobile-header-offset) - var(--safe-area-inset-bottom) - var(--safe-area-inset-top) - env(safe-area-inset-bottom) - env(safe-area-inset-top) - env(safe-area-inset-top) - var(--mobile-nav-offset) - 54px)' + height='var(--dashboard-mobile-list-height)'(Define --dashboard-mobile-list-height once in theme.)
src/state/slices/portfolioSlice/selectors.ts (2)
729-730: Decouple selectors from react-table typesExporting AccountRowProps = Row from selectors couples state to a UI library. Move this alias to the UI layer (e.g., AccountTable types) or a dedicated view-types module.
Apply:
-import type { Row } from 'react-table' ... -export type AccountRowProps = Row<AccountRowData>Then re-declare AccountRowProps where the table is defined.
893-934: Micro-opt: avoid repeated .find lookupsYou repeatedly scan accountRows to resolve each related asset. Index once by assetId to reduce O(n×m) lookups.
Apply:
- const relatedAssets = allRelatedAssetIds - .map(assetId => { - const row = accountRows.find(row => row.assetId === assetId) + const rowsByAssetId = new Map(accountRows.map(r => [r.assetId, r])) + const relatedAssets = allRelatedAssetIds + .map(assetId => { + const row = rowsByAssetId.get(assetId) const asset = assetsById[assetId] ... }) .filter(isSome) .sort((a, b) => bnOrZero(b.fiatAmount).minus(bnOrZero(a.fiatAmount)).toNumber()) - const totalFiatBalance = allRelatedAssetIds - .reduce((sum, assetId) => { - const row = accountRows.find(row => row.assetId === assetId) + const totalFiatBalance = allRelatedAssetIds + .reduce((sum, assetId) => { + const row = rowsByAssetId.get(assetId) return sum.plus(row?.fiatAmount ?? '0') }, bnOrZero(0)) .toFixed(2) - const totalCryptoBalance = allRelatedAssetIds - .reduce((sum, assetId) => { - const row = accountRows.find(row => row.assetId === assetId) + const totalCryptoBalance = allRelatedAssetIds + .reduce((sum, assetId) => { + const row = rowsByAssetId.get(assetId) return sum.plus(row?.cryptoAmount ?? '0') }, bnOrZero(0)) .toFixed(2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
package.json(0 hunks)packages/types/src/base.ts(1 hunks)packages/utils/src/assetData/decodeAssetData.ts(1 hunks)packages/utils/src/makeAsset/makeAsset.ts(1 hunks)src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx(7 hunks)src/components/TradeAssetSearch/components/SearchTermAssetList.tsx(6 hunks)src/pages/Dashboard/components/AccountList/AccountTable.tsx(11 hunks)src/state/slices/assetsSlice/selectors.ts(4 hunks)src/state/slices/assetsSlice/types.ts(1 hunks)src/state/slices/common-selectors.ts(9 hunks)src/state/slices/portfolioSlice/selectors.ts(6 hunks)
💤 Files with no reviewable changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/state/slices/assetsSlice/selectors.ts
- src/state/slices/common-selectors.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
packages/utils/src/makeAsset/makeAsset.tssrc/state/slices/assetsSlice/types.tspackages/utils/src/assetData/decodeAssetData.tspackages/types/src/base.tssrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/state/slices/portfolioSlice/selectors.tssrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
packages/utils/src/makeAsset/makeAsset.tssrc/state/slices/assetsSlice/types.tspackages/utils/src/assetData/decodeAssetData.tspackages/types/src/base.tssrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/state/slices/portfolioSlice/selectors.tssrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
packages/utils/src/makeAsset/makeAsset.tssrc/state/slices/assetsSlice/types.tspackages/utils/src/assetData/decodeAssetData.tspackages/types/src/base.tssrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/state/slices/portfolioSlice/selectors.tssrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.tsx: ALWAYS wrap components in error boundaries
ALWAYS provide user-friendly fallback components in error boundaries
ALWAYS log errors for debugging in error boundaries
ALWAYS use useErrorToast hook for displaying errors
ALWAYS provide translated error messages in error toasts
ALWAYS handle different error types appropriately in error toasts
Missing error boundaries in React components is an anti-pattern
**/*.tsx: ALWAYS use PascalCase for React component names
ALWAYS use descriptive names that indicate the component's purpose
ALWAYS match the component name to the file name
Flag components without PascalCase
Flag default exports for components
Files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
**/*.{tsx,jsx}: ALWAYS useuseMemofor expensive computations, object/array creations, and filtered data
ALWAYS useuseMemofor derived values and computed properties
ALWAYS useuseMemofor conditional values and simple transformations
ALWAYS useuseCallbackfor event handlers and functions passed as props
ALWAYS useuseCallbackfor any function that could be passed as a prop or dependency
ALWAYS include all dependencies inuseEffect,useMemo,useCallbackdependency arrays
NEVER use// eslint-disable-next-line react-hooks/exhaustive-depsunless absolutely necessary
ALWAYS explain why dependencies are excluded if using eslint disable
ALWAYS use named exports for components
NEVER use default exports for components
KEEP component files under 200 lines when possible
BREAK DOWN large components into smaller, reusable pieces
EXTRACT complex logic into custom hooks
USE local state for component-level state
LIFT state up when needed across multiple components
USE Context for avoiding prop drilling
ALWAYS wrap components in error boundaries for production
ALWAYS handle async errors properly
ALWAYS provide user-friendly error messages
ALWAYS use virtualization for lists with 100+ items
ALWAYS implement proper key props for list items
ALWAYS lazy load heavy components
ALWAYS use React.lazy for code splitting
Components receiving props are wrapped withmemo
Files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
🧠 Learnings (35)
📓 Common learnings
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
📚 Learning: 2025-09-04T13:22:35.355Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/assetsSlice/selectors.ts:45-51
Timestamp: 2025-09-04T13:22:35.355Z
Learning: In shapeshift/web, the isPrimaryAsset function in src/lib/utils/asset.ts is intentionally designed to only treat null relatedAssetKey as primary (not undefined). The asset generation process specifically sets relatedAssetKey to null for primary assets, while undefined means the asset doesn't participate in grouping and should not be considered primary. Only assets with null relatedAssetKey or where relatedAssetKey === assetId are primary assets.
Applied to files:
packages/utils/src/makeAsset/makeAsset.tssrc/state/slices/assetsSlice/types.tspackages/utils/src/assetData/decodeAssetData.tspackages/types/src/base.tssrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-04T17:29:59.454Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/TradeAssetSearch/hooks/useGetPopularAssetsQuery.tsx:28-33
Timestamp: 2025-09-04T17:29:59.454Z
Learning: In shapeshift/web, the useGetPopularAssetsQuery function in src/components/TradeAssetSearch/hooks/useGetPopularAssetsQuery.tsx intentionally uses primaryAssets[assetId] instead of falling back to assets[assetId]. The design distributes primary assets across chains by iterating through their related assets and adding the primary asset to each related asset's chain. This ensures primary assets appear in all chains where they have related assets, supporting the grouped asset system.
Applied to files:
packages/utils/src/makeAsset/makeAsset.tssrc/state/slices/assetsSlice/types.tspackages/utils/src/assetData/decodeAssetData.tspackages/types/src/base.tssrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-04T13:00:47.724Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/lib/utils/asset.ts:3-5
Timestamp: 2025-09-04T13:00:47.724Z
Learning: In shapeshift/web, primary assets are assets that represent a group of related assets across multiple chains. An asset with undefined relatedAssetKey doesn't participate in grouping and is not considered primary. Only assets with null relatedAssetKey or where relatedAssetKey === assetId are considered primary assets.
Applied to files:
packages/utils/src/makeAsset/makeAsset.tspackages/utils/src/assetData/decodeAssetData.tspackages/types/src/base.tssrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Verify chain ID filtering in filterAssetIdsBySellable and filterBuyAssetsBySellAssetId methods.
Applied to files:
packages/utils/src/makeAsset/makeAsset.tspackages/utils/src/assetData/decodeAssetData.tssrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-09-04T17:35:57.546Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/common-selectors.ts:164-178
Timestamp: 2025-09-04T17:35:57.546Z
Learning: In shapeshift/web, while the Asset type allows relatedAssetKey to be AssetId | null | undefined, the actual asset generation and processing system ensures relatedAssetKey is never undefined in practice. All asset creation utilities explicitly set it to either a specific AssetId (for grouped assets) or null (for primary/unique assets). The only undefined assignments occur during intermediate processing phases and get resolved during the final asset generation step.
Applied to files:
src/state/slices/assetsSlice/types.tspackages/utils/src/assetData/decodeAssetData.tspackages/types/src/base.tssrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-04T17:35:57.545Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/common-selectors.ts:164-178
Timestamp: 2025-09-04T17:35:57.545Z
Learning: In shapeshift/web, while the Asset type allows relatedAssetKey to be AssetId | null | undefined, the actual asset generation and processing system ensures relatedAssetKey is never undefined in practice. All asset creation utilities explicitly set it to either a specific AssetId (for grouped assets) or null (for primary/unique assets). The only exception is temporary undefined assignment during portal asset processing, which gets resolved later in the pipeline.
Applied to files:
src/state/slices/assetsSlice/types.tspackages/utils/src/assetData/decodeAssetData.tspackages/types/src/base.tssrc/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-05T23:36:13.214Z
Learnt from: premiumjibles
PR: shapeshift/web#10187
File: src/state/slices/preferencesSlice/selectors.ts:21-25
Timestamp: 2025-08-05T23:36:13.214Z
Learning: The AssetId type from 'shapeshiftoss/caip' package is a string type alias, so it can be used directly as a return type for cache key resolvers in re-reselect selectors without needing explicit string conversion.
Applied to files:
src/state/slices/assetsSlice/types.tssrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-09-04T17:27:33.287Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/TradeAssetSearch/components/SearchTermAssetList.tsx:88-94
Timestamp: 2025-09-04T17:27:33.287Z
Learning: In shapeshift/web, NFTs never have related assets in the grouping system. The related assets feature is designed for fungible tokens that exist across multiple chains (like USDC on different chains), not for NFTs which are unique. Therefore, checking isNft() for related asset IDs is unnecessary since NFT asset IDs won't be present in the relatedAssetIdsById mapping.
Applied to files:
src/state/slices/assetsSlice/types.tssrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-08T14:59:40.422Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/pages/Explore/ExploreCategory.tsx:231-238
Timestamp: 2025-08-08T14:59:40.422Z
Learning: In src/pages/Explore/ExploreCategory.tsx, for the PageHeader filter trigger, NeOMakinG considers changing a clickable Chakra Icon to IconButton too nitpicky for this PR and prefers to keep the current Icon-based trigger; such minor a11y/UI nitpicks should be deferred to a follow-up if needed.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-02T08:34:08.157Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/pages/Dashboard/components/AccountList/AccountTable.tsx:60-0
Timestamp: 2025-09-02T08:34:08.157Z
Learning: NeOMakinG prefers code review comments to focus only on actual PR changes, not pre-existing code issues, unless there are critical security or correctness concerns directly related to the new functionality.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-27T09:51:36.901Z
Learnt from: NeOMakinG
PR: shapeshift/web#10376
File: packages/contracts/src/viemClient.ts:39-40
Timestamp: 2025-08-27T09:51:36.901Z
Learning: NeOMakinG confirmed that issues flagged by CodeRabbit that predate the PR changes and are unrelated to the PR's core objective should be addressed separately, not within the current PR scope.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-08T15:00:22.321Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx:14-14
Timestamp: 2025-08-08T15:00:22.321Z
Learning: In shapeshift/web reviews for NeOMakinG, avoid nitpicks to change deep-relative imports to '@/…' alias paths within feature/non-refactor PRs; defer such style-only changes to a dedicated follow-up refactor unless they fix an issue.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-08T15:00:49.887Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-22T13:00:44.879Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-27T09:47:06.275Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/components/ButtonWalletPredicate/ButtonWalletPredicate.tsx:7-7
Timestamp: 2025-08-27T09:47:06.275Z
Learning: In shapeshift/web project, NeOMakinG consistently prefers to defer UI/UX improvements and refactoring work (like the Drawer.Close hack fix in ButtonWalletPredicate.tsx) to follow-up PRs rather than expanding the scope of feature PRs, even when the improvements would enhance robustness.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-08T11:41:22.794Z
Learnt from: NeOMakinG
PR: shapeshift/web#10234
File: src/components/MultiHopTrade/hooks/useGetTradeQuotes/hooks/useTrackTradeQuotes.ts:42-86
Timestamp: 2025-08-08T11:41:22.794Z
Learning: NeOMakinG prefers not to include refactors in move-only PRs; such suggestions should be deferred to follow-up issues instead of being applied within the same PR.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-09-02T12:38:46.940Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/Table/Table.theme.ts:177-180
Timestamp: 2025-09-02T12:38:46.940Z
Learning: NeOMakinG prefers to defer technical debt and CSS correctness issues (like improper hover selectors) to follow-up PRs when the current PR is already large and focused on major feature implementation, even when the issues are valid from a usability/technical perspective.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-25T12:59:43.842Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/Fox/components/RFOXSection.tsx:234-248
Timestamp: 2025-08-25T12:59:43.842Z
Learning: NeOMakinG considers suggestions to refactor working modal state management from useState to useDisclosure as too nitpicky, preferring to keep such cosmetic code improvements out of feature PRs unless they address actual issues.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-05T16:39:58.598Z
Learnt from: gomesalexandre
PR: shapeshift/web#10191
File: src/pages/Explore/Explore.tsx:56-56
Timestamp: 2025-08-05T16:39:58.598Z
Learning: In the ShapeShift web codebase, the established pattern for handling floating point numbers is to use BigNumber operations (bnOrZero, bn) for calculations and convert to strings using .toString() before passing to UI components like Amount.Fiat, Amount.Crypto, and Amount.Percent. This prevents JavaScript floating point precision issues and maintains consistency across the application.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsx
📚 Learning: 2025-09-04T17:26:19.399Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/PriceChangeTag/PriceChangeTag.tsx:25-36
Timestamp: 2025-09-04T17:26:19.399Z
Learning: In the shapeshift/web project, NeOMakinG prefers to hide 0% price changes in the PriceChangeTag component rather than displaying "0.00%", so the falsy check `if (!changePercent24Hr) return null` is intentional design behavior.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-10T22:06:16.818Z
Learnt from: premiumjibles
PR: shapeshift/web#10216
File: src/pages/Dashboard/components/AccountList/AccountTable.tsx:183-189
Timestamp: 2025-08-10T22:06:16.818Z
Learning: In the ShapeShift web codebase, haptic feedback for long press interactions (via `pulseAndroid()`) is handled centrally within the table components (`InfiniteTable`, `ReactTable`, `ReactTableNoPager`, `MarketsTableVirtualized`) using the `useLongPress` hook. Parent components that pass `onRowLongPress` callbacks should not add their own haptic feedback calls as this would cause duplicate vibrations.
Applied to files:
src/pages/Dashboard/components/AccountList/AccountTable.tsxsrc/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-14T17:54:32.563Z
Learnt from: gomesalexandre
PR: shapeshift/web#10276
File: src/pages/ThorChainLP/components/ReusableLpStatus/ReusableLpStatus.tsx:97-108
Timestamp: 2025-08-14T17:54:32.563Z
Learning: In ReusableLpStatus component (src/pages/ThorChainLP/components/ReusableLpStatus/ReusableLpStatus.tsx), the txAssets dependency is stable from first render because poolAsset, baseAsset, actionSide, and action are all defined first render, making the current txAssetsStatuses initialization pattern safe without needing useEffect synchronization.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts expects a filter object with an assetId property, not a raw AssetId string. The selector signature is (state: ReduxState, filter) where filter should have an assetId property. This pattern is consistent across portfolio selectors that use selectAssetIdParamFromFilter. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/state/slices/portfolioSlice/selectors.ts
📚 Learning: 2025-08-17T21:53:03.806Z
Learnt from: 0xApotheosis
PR: shapeshift/web#10290
File: scripts/generateAssetData/color-map.json:41-47
Timestamp: 2025-08-17T21:53:03.806Z
Learning: In the ShapeShift web codebase, native assets (using CAIP-19 slip44 namespace like eip155:1/slip44:60, bip122:.../slip44:..., cosmos:.../slip44:...) are manually hardcoded and not generated via the automated asset generation script. Only ERC20/BEP20 tokens go through the asset generation process. The validation scripts should only validate generated assets, not manually added native assets.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts accepts a filter object with an assetId property (signature: (state, { assetId })), not a raw AssetId string. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/state/slices/portfolioSlice/selectors.ts
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts accepts a filter object with an assetId property, not a raw AssetId string. The selector signature is (state: ReduxState, filter) where filter is expected to have an assetId property. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/state/slices/portfolioSlice/selectors.ts
📚 Learning: 2025-08-10T21:09:25.643Z
Learnt from: premiumjibles
PR: shapeshift/web#10215
File: src/components/MultiHopTrade/hooks/useGetTradeRateInput.ts:65-67
Timestamp: 2025-08-10T21:09:25.643Z
Learning: In the MultiHopTrade components, `selectInputBuyAsset` and `selectInputSellAsset` selectors from `tradeInputSlice` always return defined values because they have default values in the initial state (BTC for buyAsset, ETH for sellAsset, with fallback to defaultAsset). Null checks for these assets are unnecessary when using these selectors.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsxsrc/state/slices/portfolioSlice/selectors.ts
📚 Learning: 2025-09-04T10:18:34.113Z
Learnt from: gomesalexandre
PR: shapeshift/web#10427
File: src/hooks/useActionCenterSubscribers/useSwapActionSubscriber.tsx:40-40
Timestamp: 2025-09-04T10:18:34.113Z
Learning: In the shapeshift/web codebase, src/state/slices/selectors.ts uses wildcard exports (`export * from './[sliceName]/selectors'`) to re-export all selectors from individual slice selector files, making them available through the barrel import. This means selectors like selectTxByFilter from txHistorySlice/selectors are properly accessible via '@/state/slices/selectors' even though they don't appear in explicit named exports.
Applied to files:
src/state/slices/portfolioSlice/selectors.ts
📚 Learning: 2025-08-21T22:16:55.203Z
Learnt from: kaladinlight
PR: shapeshift/web#10326
File: src/hooks/useActionCenterSubscribers/useFetchBasePortfolio.ts:33-35
Timestamp: 2025-08-21T22:16:55.203Z
Learning: In the ShapeShift web codebase Base portfolio management hack, the accountsById[accountId] check in upsertBasePortfolio serves as a security safeguard to ensure only user-associated accounts get balance updates, not just an existence check - this boundary should be maintained even during degraded Base state.
Applied to files:
src/state/slices/portfolioSlice/selectors.ts
📚 Learning: 2025-07-30T20:57:48.176Z
Learnt from: NeOMakinG
PR: shapeshift/web#10148
File: src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx:88-91
Timestamp: 2025-07-30T20:57:48.176Z
Learning: In src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx, NeOMakinG prefers keeping the hardcoded overscan value (40) over dynamic calculation based on viewport height, prioritizing code simplicity over precision when the current approach works effectively.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-03T21:17:27.681Z
Learnt from: gomesalexandre
PR: shapeshift/web#10418
File: src/Routes/RoutesCommon.tsx:231-267
Timestamp: 2025-09-03T21:17:27.681Z
Learning: gomesalexandre prefers to keep PR diffs focused and reasonable in size, deferring tangential improvements (like Mixpanel privacy enhancements) to separate efforts rather than expanding the scope of feature PRs.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-25T23:32:13.876Z
Learnt from: premiumjibles
PR: shapeshift/web#10361
File: src/pages/Markets/components/CardWithSparkline.tsx:83-92
Timestamp: 2025-08-25T23:32:13.876Z
Learning: In shapeshift/web PR #10361, premiumjibles considered the nested button accessibility issue (ChartErrorFallback retry Button inside Card rendered as Button in CardWithSparkline.tsx) out of scope for the error boundaries feature PR, consistent with deferring minor a11y improvements to follow-up PRs.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-09-03T20:56:35.789Z
Learnt from: gomesalexandre
PR: shapeshift/web#10418
File: src/components/Layout/Header/NavBar/ShapeShiftMenu.tsx:1-15
Timestamp: 2025-09-03T20:56:35.789Z
Learning: gomesalexandre dismisses accessibility improvements with "meh accessibility", preferring to keep existing implementations over making elements properly accessible in feature PRs.
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
📚 Learning: 2025-08-03T22:10:38.426Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/react-best-practices.mdc:0-0
Timestamp: 2025-08-03T22:10:38.426Z
Learning: Applies to **/*.{tsx,jsx} : ALWAYS use virtualization for lists with 100+ items
Applied to files:
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx
🧬 Code graph analysis (5)
src/state/slices/assetsSlice/types.ts (1)
packages/caip/src/assetId/assetId.ts (1)
AssetId(17-17)
src/pages/Dashboard/components/AccountList/AccountTable.tsx (6)
src/state/slices/portfolioSlice/selectors.ts (3)
selectPrimaryPortfolioAccountRowsSortedByBalance(793-861)AccountRowData(731-744)AccountRowProps(729-729)src/state/slices/assetsSlice/selectors.ts (1)
selectAssets(39-42)src/hooks/useInfiniteScroll/useInfiniteScroll.tsx (1)
useInfiniteScroll(11-49)packages/types/src/base.ts (1)
Asset(69-91)src/pages/Dashboard/components/AccountList/GroupedAccounts.tsx (1)
GroupedAccounts(87-106)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(39-153)
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx (7)
src/state/store.ts (1)
useAppSelector(143-143)src/state/slices/common-selectors.ts (2)
selectRelatedAssetIdsByAssetIdInclusive(164-178)selectPortfolioUserCurrencyBalances(144-162)src/state/slices/assetsSlice/selectors.ts (1)
selectPrimaryAssets(44-50)packages/caip/src/assetId/assetId.ts (1)
fromAssetId(143-178)packages/types/src/base.ts (1)
Asset(69-91)src/components/Text/Text.tsx (1)
Text(19-83)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(39-153)
src/state/slices/portfolioSlice/selectors.ts (7)
src/state/selector-utils.ts (1)
createDeepEqualOutputSelector(5-7)src/state/slices/assetsSlice/selectors.ts (1)
selectAssets(39-42)src/state/slices/marketDataSlice/selectors.ts (1)
selectMarketDataUserCurrency(19-55)src/state/slices/common-selectors.ts (3)
selectPortfolioAssetBalancesBaseUnitIncludingZeroBalances(95-107)selectPortfolioAccountBalancesBaseUnit(86-93)selectRelatedAssetIdsByAssetIdInclusive(164-178)src/lib/math.ts (1)
fromBaseUnit(10-24)src/state/reducer.ts (1)
ReduxState(194-194)packages/caip/src/assetId/assetId.ts (1)
AssetId(17-17)
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (5)
packages/types/src/base.ts (1)
Asset(69-91)src/state/apis/fiatRamps/hooks.ts (1)
useFetchFiatAssetMarketData(8-20)src/components/MarketTableVirtualized/GroupedAssets.tsx (1)
GroupedAssets(65-85)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(39-153)src/components/ReactTable/InfiniteTable.tsx (1)
InfiniteTable(47-317)
⏰ 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). (1)
- GitHub Check: Install and Cache
🔇 Additional comments (11)
src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx (4)
136-142: Confirm predicate for navigation vs. expansionCurrent guard uses isChainSpecific to allow navigation; elsewhere expansion hinges on isPrimary. Please confirm there are no rows that are non-primary yet not chain-specific (which would then block navigation unintentionally). If such rows can exist, consider guarding with isPrimary instead for clarity.
144-150: Long‑press handling is correctBlocking long‑press on primary rows avoids accidental context actions on groups; integrates cleanly with central haptics in InfiniteTable.
152-159: Subcomponent rendering aligned with groupingRendering details only for isPrimary keeps the expanded area coherent with the grouping model.
161-174: Verify 100vh mobile list doesn’t cause double-scroll/clippingOn some devices with insets/headers, 100vh can clip or create nested scroll. If you observe this, consider using
height='calc(var(--app-height, 100vh))'or a container-based height.packages/utils/src/makeAsset/makeAsset.ts (1)
52-54: Defaulting runtime-created assets to primary + chain-specific makes senseThis matches “custom tokens aren’t grouped.” If any runtime-created asset can later join an existing group, ensure an update path flips these flags when relatedAssetKey is set.
packages/utils/src/assetData/decodeAssetData.ts (1)
88-90: Derivation logic LGTMPrimary/chain-specific inference from relatedAssetId matches the intended semantics (null -> chain-specific primary; self -> multi-chain primary).
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx (1)
49-54: Are we sure this list is primary-only? (summary mismatch)AI summary says we switched to primary assets, but this still selects the full sorted assets list. If non-primary rows slip in, you’ll get duplicates despite showRelatedAssets. Verify; if needed, derive assets from selectPrimaryAssets.
src/pages/Dashboard/components/AccountList/AccountTable.tsx (3)
204-218: Correct navigation vs. expansion behaviorEarly return on multi‑chain primary rows keeps navigation off expandable rows; navigation flows for single‑chain rows look correct.
59-63: Paging after grouping looks correctFeeding useInfiniteScroll with already‑grouped/sorted selector output avoids the previous paging aggregation issues.
253-256: Mobile data source LGTMMapping primary rows to assets for AssetList is consistent with grouped semantics, and filters safely with isSome.
src/state/slices/portfolioSlice/selectors.ts (1)
813-814: Confirm related IDs presence or add safe fallbackAssumes relatedAssetIdsByAssetId[primaryAssetId] always exists. If this invariant could break, default to [primaryAssetId] to avoid crashes.
Would you like a small guard added, or is the invariant guaranteed by asset generation?
| const getAssetBalance = (asset: Asset) => { | ||
| if (asset.isChainSpecific) | ||
| return bnOrZero(portfolioUserCurrencyBalances[asset.assetId]).toNumber() | ||
|
|
||
| const primaryAssetTotalBalance = relatedAssetIdsById[asset.assetId]?.reduce( | ||
| (acc, relatedAssetId) => { | ||
| return acc.plus(bnOrZero(portfolioUserCurrencyBalances[relatedAssetId])) | ||
| }, | ||
| bnOrZero(0), | ||
| ) | ||
|
|
||
| return primaryAssetTotalBalance.toNumber() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing related mapping to prevent runtime TypeError
If a non‑chain‑specific asset ever lacks an entry in relatedAssetIdsById, primaryAssetTotalBalance is undefined and .toNumber() will throw.
- const getAssetBalance = (asset: Asset) => {
- if (asset.isChainSpecific)
- return bnOrZero(portfolioUserCurrencyBalances[asset.assetId]).toNumber()
-
- const primaryAssetTotalBalance = relatedAssetIdsById[asset.assetId]?.reduce(
- (acc, relatedAssetId) => {
- return acc.plus(bnOrZero(portfolioUserCurrencyBalances[relatedAssetId]))
- },
- bnOrZero(0),
- )
-
- return primaryAssetTotalBalance.toNumber()
- }
+ const getAssetBalance = (asset: Asset) => {
+ const related = relatedAssetIdsById[asset.assetId]
+ if (asset.isChainSpecific || !related?.length) {
+ return bnOrZero(portfolioUserCurrencyBalances[asset.assetId]).toNumber()
+ }
+ const total = related.reduce(
+ (acc, relatedAssetId) => acc.plus(bnOrZero(portfolioUserCurrencyBalances[relatedAssetId])),
+ bnOrZero(0),
+ )
+ return total.toNumber()
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getAssetBalance = (asset: Asset) => { | |
| if (asset.isChainSpecific) | |
| return bnOrZero(portfolioUserCurrencyBalances[asset.assetId]).toNumber() | |
| const primaryAssetTotalBalance = relatedAssetIdsById[asset.assetId]?.reduce( | |
| (acc, relatedAssetId) => { | |
| return acc.plus(bnOrZero(portfolioUserCurrencyBalances[relatedAssetId])) | |
| }, | |
| bnOrZero(0), | |
| ) | |
| return primaryAssetTotalBalance.toNumber() | |
| } | |
| const getAssetBalance = (asset: Asset) => { | |
| const related = relatedAssetIdsById[asset.assetId] | |
| if (asset.isChainSpecific || !related?.length) { | |
| return bnOrZero(portfolioUserCurrencyBalances[asset.assetId]).toNumber() | |
| } | |
| const total = related.reduce( | |
| (acc, relatedAssetId) => | |
| acc.plus(bnOrZero(portfolioUserCurrencyBalances[relatedAssetId])), | |
| bnOrZero(0), | |
| ) | |
| return total.toNumber() | |
| } |
🤖 Prompt for AI Agents
In src/components/TradeAssetSearch/components/SearchTermAssetList.tsx around
lines 159 to 171, the reduce uses relatedAssetIdsById[asset.assetId] which can
be undefined causing primaryAssetTotalBalance to be undefined and .toNumber() to
throw; fix by guarding the lookup (use a default empty array when mapping is
missing or check for undefined first), run reduce on that safe array (starting
with bnOrZero(0)) so primaryAssetTotalBalance is always a BigNumber-like value,
then call .toNumber() safely; ensure types reflect the possible undefined and/or
update the lookup to coalesce to [] before reduce.
| if (!isLargerThanMd) { | ||
| return ( | ||
| <AssetList | ||
| assets={accountsAssets} | ||
| handleClick={handleAssetClick} | ||
| handleLongPress={handleAssetLongPress} | ||
| height='calc(100vh - var(--mobile-header-offset) - var(--safe-area-inset-bottom) - var(--safe-area-inset-top) - env(safe-area-inset-bottom) - env(safe-area-inset-top) - env(safe-area-inset-top) - var(--mobile-nav-offset) - 54px)' | ||
| showRelatedAssets | ||
| /> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mobile: pass loading state to AssetList to prevent “No results” flash
Without isLoading, mobile can momentarily show an empty state while data is loading.
Apply:
<AssetList
assets={accountsAssets}
handleClick={handleAssetClick}
handleLongPress={handleAssetLongPress}
height='calc(100vh - var(--mobile-header-offset) - var(--safe-area-inset-bottom) - var(--safe-area-inset-top) - env(safe-area-inset-bottom) - env(safe-area-inset-top) - env(safe-area-inset-top) - var(--mobile-nav-offset) - 54px)'
showRelatedAssets
+ isLoading={loading}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!isLargerThanMd) { | |
| return ( | |
| <AssetList | |
| assets={accountsAssets} | |
| handleClick={handleAssetClick} | |
| handleLongPress={handleAssetLongPress} | |
| height='calc(100vh - var(--mobile-header-offset) - var(--safe-area-inset-bottom) - var(--safe-area-inset-top) - env(safe-area-inset-bottom) - env(safe-area-inset-top) - env(safe-area-inset-top) - var(--mobile-nav-offset) - 54px)' | |
| showRelatedAssets | |
| /> | |
| ) | |
| if (!isLargerThanMd) { | |
| return ( | |
| <AssetList | |
| assets={accountsAssets} | |
| handleClick={handleAssetClick} | |
| handleLongPress={handleAssetLongPress} | |
| height='calc(100vh - var(--mobile-header-offset) - var(--safe-area-inset-bottom) - var(--safe-area-inset-top) - env(safe-area-inset-bottom) - env(safe-area-inset-top) - env(safe-area-inset-top) - var(--mobile-nav-offset) - 54px)' | |
| showRelatedAssets | |
| isLoading={loading} | |
| /> | |
| ) | |
| } |
🤖 Prompt for AI Agents
In src/pages/Dashboard/components/AccountList/AccountTable.tsx around lines 257
to 266, the mobile branch renders AssetList without forwarding the loading state
which can cause a transient "No results" flash; add the isLoading prop to the
AssetList (e.g., isLoading={isLoading}) so the component receives and uses the
current loading state, and ensure the isLoading variable is in scope or rename
to the correct loading flag used in this component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/utils/src/assetData/assetData.test.ts (2)
7-7: Prefer UPPER_SNAKE_CASE for test constantsMatches project naming guidance for constants and improves readability.
-const mockGeneratedAssetData: AssetsById = { +const MOCK_GENERATED_ASSET_DATA: AssetsById = { -const mockSortedAssetIds = [ +const MOCK_SORTED_ASSET_IDS = [ - const encodedAssetData = encodeAssetData(mockSortedAssetIds, mockGeneratedAssetData) - const { assetData, sortedAssetIds } = decodeAssetData(encodedAssetData) - expect(assetData).toEqual(mockGeneratedAssetData) - expect(sortedAssetIds).toEqual(mockSortedAssetIds) + const encodedAssetData = encodeAssetData(MOCK_SORTED_ASSET_IDS, MOCK_GENERATED_ASSET_DATA) + const { assetData, sortedAssetIds } = decodeAssetData(encodedAssetData) + expect(assetData).toEqual(MOCK_GENERATED_ASSET_DATA) + expect(sortedAssetIds).toEqual(MOCK_SORTED_ASSET_IDS)Also applies to: 121-130, 134-137
24-38: Add a non-primary sample to cover the false branchAll samples are primary; add one secondary (relatedAssetKey points to a different assetId) to exercise
isPrimary: falseandisChainSpecific: false.'eip155:1/erc20:0xf073bac22dab7faf4a3dd6c6189a70d54110525c': { @@ relatedAssetKey: 'eip155:1/erc20:0xf073bac22dab7faf4a3dd6c6189a70d54110525c', isPrimary: true, isChainSpecific: false, }, + // Secondary (non-primary) variant to exercise isPrimary === false + 'eip155:42161/erc20:0xf073bac22dab7faf4a3dd6c6189a70d54110525c': { + assetId: 'eip155:42161/erc20:0xf073bac22dab7faf4a3dd6c6189a70d54110525c', + chainId: 'eip155:42161', + name: 'Inception Restaked ETH on Arbitrum', + precision: 18, + color: '#D6D1E4', + icon: 'https://assets.coingecko.com/coins/images/34127/large/inETH.png?1715036464', + symbol: 'INETH', + explorer: 'https://arbiscan.io', + explorerAddressLink: 'https://arbiscan.io/address/', + explorerTxLink: 'https://arbiscan.io/tx/', + // Points to the Ethereum primary, so this entry is non-primary + relatedAssetKey: 'eip155:1/erc20:0xf073bac22dab7faf4a3dd6c6189a70d54110525c', + isPrimary: false, + isChainSpecific: false, + }, @@ const mockSortedAssetIds = [ 'eip155:1/erc20:0xeeecd285f60e802ecb6d8d8d37790c887f9a4b33', // big tom 'eip155:1/erc20:0xeeee2a2e650697d2a8e8bc990c2f3d04203be06f', // forgotten playland 'eip155:1/erc20:0xeed3ae7b0f8b5b9bb8c035a9941382b1822671cd', // everycoin 'eip155:1/erc20:0xf073bac22dab7faf4a3dd6c6189a70d54110525c', // Inception Restaked ETH on Ethereum + 'eip155:42161/erc20:0xf073bac22dab7faf4a3dd6c6189a70d54110525c', // Inception Restaked ETH on Arbitrum (secondary) 'eip155:1/erc20:0xeee0fe52299f2de8e2ed5111cd521ab67dcf0faf', // the qwan 'eip155:1/erc20:0xeeda34a377dd0ca676b9511ee1324974fa8d980d', // Curve PUFETH/WSTETH Pool 'bip122:000000000019d6689c085ae165831e93/slip44:0', // BTC ]Also applies to: 121-130
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/utils/src/assetData/assetData.test.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
packages/utils/src/assetData/assetData.test.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
packages/utils/src/assetData/assetData.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
packages/utils/src/assetData/assetData.test.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/lib/utils/asset.ts:3-5
Timestamp: 2025-09-04T13:00:47.724Z
Learning: In shapeshift/web, primary assets are assets that represent a group of related assets across multiple chains. An asset with undefined relatedAssetKey doesn't participate in grouping and is not considered primary. Only assets with null relatedAssetKey or where relatedAssetKey === assetId are considered primary assets.
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
📚 Learning: 2025-09-04T13:22:35.355Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/assetsSlice/selectors.ts:45-51
Timestamp: 2025-09-04T13:22:35.355Z
Learning: In shapeshift/web, the isPrimaryAsset function in src/lib/utils/asset.ts is intentionally designed to only treat null relatedAssetKey as primary (not undefined). The asset generation process specifically sets relatedAssetKey to null for primary assets, while undefined means the asset doesn't participate in grouping and should not be considered primary. Only assets with null relatedAssetKey or where relatedAssetKey === assetId are primary assets.
Applied to files:
packages/utils/src/assetData/assetData.test.ts
📚 Learning: 2025-09-04T17:29:59.454Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/TradeAssetSearch/hooks/useGetPopularAssetsQuery.tsx:28-33
Timestamp: 2025-09-04T17:29:59.454Z
Learning: In shapeshift/web, the useGetPopularAssetsQuery function in src/components/TradeAssetSearch/hooks/useGetPopularAssetsQuery.tsx intentionally uses primaryAssets[assetId] instead of falling back to assets[assetId]. The design distributes primary assets across chains by iterating through their related assets and adding the primary asset to each related asset's chain. This ensures primary assets appear in all chains where they have related assets, supporting the grouped asset system.
Applied to files:
packages/utils/src/assetData/assetData.test.ts
📚 Learning: 2025-09-04T13:00:47.724Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/lib/utils/asset.ts:3-5
Timestamp: 2025-09-04T13:00:47.724Z
Learning: In shapeshift/web, primary assets are assets that represent a group of related assets across multiple chains. An asset with undefined relatedAssetKey doesn't participate in grouping and is not considered primary. Only assets with null relatedAssetKey or where relatedAssetKey === assetId are considered primary assets.
Applied to files:
packages/utils/src/assetData/assetData.test.ts
📚 Learning: 2025-09-04T17:35:57.545Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/common-selectors.ts:164-178
Timestamp: 2025-09-04T17:35:57.545Z
Learning: In shapeshift/web, while the Asset type allows relatedAssetKey to be AssetId | null | undefined, the actual asset generation and processing system ensures relatedAssetKey is never undefined in practice. All asset creation utilities explicitly set it to either a specific AssetId (for grouped assets) or null (for primary/unique assets). The only exception is temporary undefined assignment during portal asset processing, which gets resolved later in the pipeline.
Applied to files:
packages/utils/src/assetData/assetData.test.ts
📚 Learning: 2025-09-04T17:35:57.546Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/common-selectors.ts:164-178
Timestamp: 2025-09-04T17:35:57.546Z
Learning: In shapeshift/web, while the Asset type allows relatedAssetKey to be AssetId | null | undefined, the actual asset generation and processing system ensures relatedAssetKey is never undefined in practice. All asset creation utilities explicitly set it to either a specific AssetId (for grouped assets) or null (for primary/unique assets). The only undefined assignments occur during intermediate processing phases and get resolved during the final asset generation step.
Applied to files:
packages/utils/src/assetData/assetData.test.ts
⏰ 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). (1)
- GitHub Check: Install and Cache
🔇 Additional comments (7)
packages/utils/src/assetData/assetData.test.ts (7)
21-22: isPrimary/isChainSpecific correct for null relatedAssetKeyValues align with the grouping contract (null => primary and chain-specific). LGTM.
36-37: Self-referential relatedAssetKey handled correctlyPrimary is true and chain-specific is false for a self-referential key; matches the intended semantics.
51-52: Correct flags for standalone assetNull relatedAssetKey → primary and chain-specific. Looks good.
71-72: Pool asset flags are consistentEven with isPool: true, flags follow relatedAssetKey rules. LGTM.
86-87: Consistent flagsNull relatedAssetKey → primary/chain-specific. LGTM.
101-102: Consistent flagsSame correctness as above; no issues.
116-117: Consistent flagsFinal entry also correctly reflects the contract.
There was a problem hiding this comment.
Tested locally, LGTM assuming tests fixed
https://jam.dev/c/77e79a95-1ef2-439d-a392-be1a26afb81f
Two suggestions here:
- Give a min/height of sorts to the search to make it more sane so that we don't have a flash of search modal while it resizes
https://jam.dev/c/f9aeb50b-a033-4f8d-801b-92b7d435fa96
- Fiat ramps are not leveraging this just yet, is that expected?
Fixed the height issue, also added custom scrollbars For the fiat ramps, would love to do that in a follow up! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx (1)
161-173: Fix: .toNumber() on possibly undefined when related mapping is missingWhen relatedAssetIdsById[asset.assetId] is undefined, primaryAssetTotalBalance becomes undefined and .toNumber() throws. Guard and coalesce.
- const getAssetBalance = (asset: Asset) => { - if (asset.isChainSpecific) - return bnOrZero(portfolioUserCurrencyBalances[asset.assetId]).toNumber() - - const primaryAssetTotalBalance = relatedAssetIdsById[asset.assetId]?.reduce( - (acc, relatedAssetId) => { - return acc.plus(bnOrZero(portfolioUserCurrencyBalances[relatedAssetId])) - }, - bnOrZero(0), - ) - - return primaryAssetTotalBalance.toNumber() - } + const getAssetBalance = (asset: Asset) => { + const related = relatedAssetIdsById[asset.assetId] + if (asset.isChainSpecific || !related?.length) { + return bnOrZero(portfolioUserCurrencyBalances[asset.assetId]).toNumber() + } + const total = related.reduce( + (acc, relatedAssetId) => + acc.plus(bnOrZero(portfolioUserCurrencyBalances[relatedAssetId])), + bnOrZero(0), + ) + return total.toNumber() + }
🧹 Nitpick comments (2)
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx (1)
78-85: Hot path: replace fromAssetId() parsing with a cheap prefix check; always exclude NFTs in “All”Parsing every relatedAssetId with fromAssetId on each filter pass is unnecessary. CAIP-19 allows a fast chain-prefix check. Also, in the All branch, returning _assets when allowWalletUnsupportedAssets is true can surface NFTs in trade search.
Apply:
@@ - if (activeChainId === 'All') { - if (allowWalletUnsupportedAssets) return _assets - return _assets.filter( - asset => walletConnectedChainIds.includes(asset.chainId) && !isNft(asset.assetId), - ) - } + if (activeChainId === 'All') { + if (allowWalletUnsupportedAssets) + return _assets.filter(asset => !isNft(asset.assetId)) + return _assets.filter( + asset => walletConnectedChainIds.includes(asset.chainId) && !isNft(asset.assetId), + ) + } @@ - return _assets.filter( - asset => - (asset.chainId === activeChainId && !isNft(asset.assetId)) || - relatedAssetIdsById[asset.assetId]?.some( - relatedAssetId => fromAssetId(relatedAssetId).chainId === activeChainId, - ), - ) + const chainPrefix = `${activeChainId}/` + return _assets.filter(asset => { + if (asset.chainId === activeChainId && !isNft(asset.assetId)) return true + const related = relatedAssetIdsById[asset.assetId] + if (!related?.length) return false + return related.some(relatedAssetId => relatedAssetId.startsWith(chainPrefix)) + })Also applies to: 90-96
src/components/AssetSearch/components/AssetList.tsx (1)
136-150: Small-list fallback: consider not enforcing minH=50vhFor <=10 items, forcing minH='50vh' can introduce excessive empty space in constrained containers (drawers/modals). Let parent control height or reduce minH.
- <Box - maxHeight={height} - overflow='auto' - height='auto' - minH='50vh' - className='scroll-container' - > + <Box maxHeight={height} overflow='auto' height='auto' className='scroll-container'>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/components/AssetSearch/components/AssetList.tsx(6 hunks)src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsx(2 hunks)src/components/TradeAssetSearch/components/SearchTermAssetList.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/TradeAssetSearch/components/GroupedAssetList/GroupedAssetList.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{ts,tsx}: ALWAYS use Result<T, E> pattern for error handling in swappers and APIs
ALWAYS use Ok() and Err() from @sniptt/monads for monadic error handling
ALWAYS use custom error classes from @shapeshiftoss/errors
ALWAYS provide meaningful error codes for internationalization
ALWAYS include relevant details in error objects
ALWAYS wrap async operations in try-catch blocks
ALWAYS use AsyncResultOf utility for converting promises to Results
ALWAYS provide fallback error handling
ALWAYS use timeoutMonadic for API calls
ALWAYS provide appropriate timeout values for API calls
ALWAYS handle timeout errors gracefully
ALWAYS validate inputs before processing
ALWAYS provide clear validation error messages
ALWAYS use early returns for validation failures
ALWAYS log errors for debugging
ALWAYS use structured logging for errors
ALWAYS include relevant context in error logs
Throwing errors instead of using monadic patterns is an anti-pattern
Missing try-catch blocks for async operations is an anti-pattern
Generic error messages without context are an anti-pattern
Not handling specific error types is an anti-pattern
Missing timeout handling is an anti-pattern
No input validation is an anti-pattern
Poor error logging is an anti-pattern
Using any for error types is an anti-pattern
Missing error codes for internationalization is an anti-pattern
No fallback error handling is an anti-pattern
Console.error without structured logging is an anti-pattern
**/*.{ts,tsx}: ALWAYS use camelCase for variables, functions, and methods
ALWAYS use descriptive names that explain the purpose for variables and functions
ALWAYS use verb prefixes for functions that perform actions
ALWAYS use PascalCase for types, interfaces, and enums
ALWAYS use descriptive names that indicate the structure for types, interfaces, and enums
ALWAYS use suffixes like Props, State, Config, Type when appropriate for types and interfaces
ALWAYS use UPPER_SNAKE_CASE for constants and configuration values
ALWAYS use d...
Files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.tsx: ALWAYS wrap components in error boundaries
ALWAYS provide user-friendly fallback components in error boundaries
ALWAYS log errors for debugging in error boundaries
ALWAYS use useErrorToast hook for displaying errors
ALWAYS provide translated error messages in error toasts
ALWAYS handle different error types appropriately in error toasts
Missing error boundaries in React components is an anti-pattern
**/*.tsx: ALWAYS use PascalCase for React component names
ALWAYS use descriptive names that indicate the component's purpose
ALWAYS match the component name to the file name
Flag components without PascalCase
Flag default exports for components
Files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-conventions.mdc)
**/*: ALWAYS use appropriate file extensions
Flag files without kebab-case
Files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
**/*.{tsx,jsx}: ALWAYS useuseMemofor expensive computations, object/array creations, and filtered data
ALWAYS useuseMemofor derived values and computed properties
ALWAYS useuseMemofor conditional values and simple transformations
ALWAYS useuseCallbackfor event handlers and functions passed as props
ALWAYS useuseCallbackfor any function that could be passed as a prop or dependency
ALWAYS include all dependencies inuseEffect,useMemo,useCallbackdependency arrays
NEVER use// eslint-disable-next-line react-hooks/exhaustive-depsunless absolutely necessary
ALWAYS explain why dependencies are excluded if using eslint disable
ALWAYS use named exports for components
NEVER use default exports for components
KEEP component files under 200 lines when possible
BREAK DOWN large components into smaller, reusable pieces
EXTRACT complex logic into custom hooks
USE local state for component-level state
LIFT state up when needed across multiple components
USE Context for avoiding prop drilling
ALWAYS wrap components in error boundaries for production
ALWAYS handle async errors properly
ALWAYS provide user-friendly error messages
ALWAYS use virtualization for lists with 100+ items
ALWAYS implement proper key props for list items
ALWAYS lazy load heavy components
ALWAYS use React.lazy for code splitting
Components receiving props are wrapped withmemo
Files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react-best-practices.mdc)
USE Redux only for global state shared across multiple places
Files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
🧠 Learnings (25)
📓 Common learnings
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/lib/utils/asset.ts:3-5
Timestamp: 2025-09-04T13:00:47.724Z
Learning: In shapeshift/web, primary assets are assets that represent a group of related assets across multiple chains. An asset with undefined relatedAssetKey doesn't participate in grouping and is not considered primary. Only assets with null relatedAssetKey or where relatedAssetKey === assetId are considered primary assets.
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/pages/RFOX/components/Stake/components/StakeSummary.tsx:112-114
Timestamp: 2025-08-22T13:00:44.879Z
Learning: NeOMakinG prefers to keep PR changes minimal and focused on the core objectives, avoiding cosmetic or defensive code improvements that aren't directly related to the PR scope, even when they would improve robustness.
📚 Learning: 2025-08-08T15:00:49.887Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/AssetSearch/components/AssetList.tsx:2-2
Timestamp: 2025-08-08T15:00:49.887Z
Learning: Project shapeshift/web: NeOMakinG prefers avoiding minor a11y/UI nitpicks (e.g., adding aria-hidden to decorative icons in empty states like src/components/AssetSearch/components/AssetList.tsx) within feature PRs; defer such suggestions to a follow-up instead of blocking the PR.
Applied to files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-09-04T17:35:57.546Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/common-selectors.ts:164-178
Timestamp: 2025-09-04T17:35:57.546Z
Learning: In shapeshift/web, while the Asset type allows relatedAssetKey to be AssetId | null | undefined, the actual asset generation and processing system ensures relatedAssetKey is never undefined in practice. All asset creation utilities explicitly set it to either a specific AssetId (for grouped assets) or null (for primary/unique assets). The only undefined assignments occur during intermediate processing phases and get resolved during the final asset generation step.
Applied to files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-09-04T17:35:57.545Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/common-selectors.ts:164-178
Timestamp: 2025-09-04T17:35:57.545Z
Learning: In shapeshift/web, while the Asset type allows relatedAssetKey to be AssetId | null | undefined, the actual asset generation and processing system ensures relatedAssetKey is never undefined in practice. All asset creation utilities explicitly set it to either a specific AssetId (for grouped assets) or null (for primary/unique assets). The only exception is temporary undefined assignment during portal asset processing, which gets resolved later in the pipeline.
Applied to files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-09-04T13:22:35.355Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/state/slices/assetsSlice/selectors.ts:45-51
Timestamp: 2025-09-04T13:22:35.355Z
Learning: In shapeshift/web, the isPrimaryAsset function in src/lib/utils/asset.ts is intentionally designed to only treat null relatedAssetKey as primary (not undefined). The asset generation process specifically sets relatedAssetKey to null for primary assets, while undefined means the asset doesn't participate in grouping and should not be considered primary. Only assets with null relatedAssetKey or where relatedAssetKey === assetId are primary assets.
Applied to files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-09-02T12:37:37.608Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/AssetSearch/components/AssetList.tsx:0-0
Timestamp: 2025-09-02T12:37:37.608Z
Learning: NeOMakinG in project shapeshift/web prefers solutions that preserve sorting order when implementing asset grouping/deduplication functionality, avoiding approaches like lodash groupBy that would reorganize the array structure.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-09-04T17:29:59.454Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/TradeAssetSearch/hooks/useGetPopularAssetsQuery.tsx:28-33
Timestamp: 2025-09-04T17:29:59.454Z
Learning: In shapeshift/web, the useGetPopularAssetsQuery function in src/components/TradeAssetSearch/hooks/useGetPopularAssetsQuery.tsx intentionally uses primaryAssets[assetId] instead of falling back to assets[assetId]. The design distributes primary assets across chains by iterating through their related assets and adding the primary asset to each related asset's chain. This ensures primary assets appear in all chains where they have related assets, supporting the grouped asset system.
Applied to files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-09-04T13:00:47.724Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/lib/utils/asset.ts:3-5
Timestamp: 2025-09-04T13:00:47.724Z
Learning: In shapeshift/web, primary assets are assets that represent a group of related assets across multiple chains. An asset with undefined relatedAssetKey doesn't participate in grouping and is not considered primary. Only assets with null relatedAssetKey or where relatedAssetKey === assetId are considered primary assets.
Applied to files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts expects a filter object with an assetId property, not a raw AssetId string. The selector signature is (state: ReduxState, filter) where filter should have an assetId property. This pattern is consistent across portfolio selectors that use selectAssetIdParamFromFilter. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-22T12:59:01.702Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/components/Layout/Header/ActionCenter/components/RewardDistributionActionCard.tsx:37-53
Timestamp: 2025-08-22T12:59:01.702Z
Learning: In RewardDistributionActionCard component (src/components/Layout/Header/ActionCenter/components/RewardDistributionActionCard.tsx), NeOMakinG confirmed that runeAsset is expected to always be defined when the component renders, so defensive guards against undefined runeAsset are not needed.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-08-22T12:58:36.070Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/components/Layout/Header/ActionCenter/components/Notifications/RewardDistributionNotification.tsx:33-55
Timestamp: 2025-08-22T12:58:36.070Z
Learning: In RewardDistributionNotification component (src/components/Layout/Header/ActionCenter/components/Notifications/RewardDistributionNotification.tsx), NeOMakinG confirmed that runeAsset is expected to always be defined when the component renders, so defensive guards against undefined runeAsset are not needed.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-08-05T22:41:35.473Z
Learnt from: premiumjibles
PR: shapeshift/web#10187
File: src/pages/Assets/Asset.tsx:1-1
Timestamp: 2025-08-05T22:41:35.473Z
Learning: In the shapeshift/web codebase, component imports use direct file paths like '@/components/ComponentName/ComponentName' rather than barrel exports. The AssetAccountDetails component should be imported as '@/components/AssetAccountDetails/AssetAccountDetails', not from a directory index.
Applied to files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-08T15:00:22.321Z
Learnt from: NeOMakinG
PR: shapeshift/web#10231
File: src/components/MultiHopTrade/components/TradeInput/components/HighlightedTokens.tsx:14-14
Timestamp: 2025-08-08T15:00:22.321Z
Learning: In shapeshift/web reviews for NeOMakinG, avoid nitpicks to change deep-relative imports to '@/…' alias paths within feature/non-refactor PRs; defer such style-only changes to a dedicated follow-up refactor unless they fix an issue.
Applied to files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-27T09:47:06.275Z
Learnt from: NeOMakinG
PR: shapeshift/web#10323
File: src/components/ButtonWalletPredicate/ButtonWalletPredicate.tsx:7-7
Timestamp: 2025-08-27T09:47:06.275Z
Learning: In shapeshift/web project, NeOMakinG consistently prefers to defer UI/UX improvements and refactoring work (like the Drawer.Close hack fix in ButtonWalletPredicate.tsx) to follow-up PRs rather than expanding the scope of feature PRs, even when the improvements would enhance robustness.
Applied to files:
src/components/AssetSearch/components/AssetList.tsxsrc/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-07-29T10:22:27.037Z
Learnt from: NeOMakinG
PR: shapeshift/web#10136
File: src/lib/asset-service/service/encodedRelatedAssetIndex.json:1-1
Timestamp: 2025-07-29T10:22:27.037Z
Learning: PRs with titles starting with "feat: regenerate asset data" are routine daily asset updates that don't need detailed code analysis. Users prefer to skip automated reviews for these maintenance PRs using coderabbitai ignore.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-07-30T20:57:48.176Z
Learnt from: NeOMakinG
PR: shapeshift/web#10148
File: src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx:88-91
Timestamp: 2025-07-30T20:57:48.176Z
Learning: In src/components/MarketTableVirtualized/MarketsTableVirtualized.tsx, NeOMakinG prefers keeping the hardcoded overscan value (40) over dynamic calculation based on viewport height, prioritizing code simplicity over precision when the current approach works effectively.
Applied to files:
src/components/AssetSearch/components/AssetList.tsx
📚 Learning: 2025-07-24T09:43:11.699Z
Learnt from: CR
PR: shapeshift/web#0
File: .cursor/rules/swapper.mdc:0-0
Timestamp: 2025-07-24T09:43:11.699Z
Learning: Applies to packages/swapper/src/swappers/*/{*.ts,endpoints.ts} : Verify chain ID filtering in filterAssetIdsBySellable and filterBuyAssetsBySellAssetId methods.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-09-04T17:27:33.287Z
Learnt from: NeOMakinG
PR: shapeshift/web#10380
File: src/components/TradeAssetSearch/components/SearchTermAssetList.tsx:88-94
Timestamp: 2025-09-04T17:27:33.287Z
Learning: In shapeshift/web, NFTs never have related assets in the grouping system. The related assets feature is designed for fungible tokens that exist across multiple chains (like USDC on different chains), not for NFTs which are unique. Therefore, checking isNft() for related asset IDs is unnecessary since NFT asset IDs won't be present in the relatedAssetIdsById mapping.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-17T21:53:03.806Z
Learnt from: 0xApotheosis
PR: shapeshift/web#10290
File: scripts/generateAssetData/color-map.json:41-47
Timestamp: 2025-08-17T21:53:03.806Z
Learning: In the ShapeShift web codebase, native assets (using CAIP-19 slip44 namespace like eip155:1/slip44:60, bip122:.../slip44:..., cosmos:.../slip44:...) are manually hardcoded and not generated via the automated asset generation script. Only ERC20/BEP20 tokens go through the asset generation process. The validation scripts should only validate generated assets, not manually added native assets.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts accepts a filter object with an assetId property (signature: (state, { assetId })), not a raw AssetId string. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-15T07:51:16.374Z
Learnt from: gomesalexandre
PR: shapeshift/web#10278
File: src/components/AssetHeader/hooks/useQuickBuy.ts:97-99
Timestamp: 2025-08-15T07:51:16.374Z
Learning: The selectPortfolioUserCurrencyBalanceByAssetId selector in src/state/slices/portfolioSlice/selectors.ts accepts a filter object with an assetId property, not a raw AssetId string. The selector signature is (state: ReduxState, filter) where filter is expected to have an assetId property. Passing a filter object like { assetId: someAssetId } is the correct usage pattern.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-10T21:09:25.643Z
Learnt from: premiumjibles
PR: shapeshift/web#10215
File: src/components/MultiHopTrade/hooks/useGetTradeRateInput.ts:65-67
Timestamp: 2025-08-10T21:09:25.643Z
Learning: In the MultiHopTrade components, `selectInputBuyAsset` and `selectInputSellAsset` selectors from `tradeInputSlice` always return defined values because they have default values in the initial state (BTC for buyAsset, ETH for sellAsset, with fallback to defaultAsset). Null checks for these assets are unnecessary when using these selectors.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-07-24T11:07:20.536Z
Learnt from: gomesalexandre
PR: shapeshift/web#10073
File: src/features/defi/providers/fox-farming/components/FoxFarmingManager/Claim/Claim.tsx:77-84
Timestamp: 2025-07-24T11:07:20.536Z
Learning: In fox farming components, the `opportunity?.rewardsCryptoBaseUnit?.amounts` property has a well-defined type signature that is always an array (never undefined), but can be empty: `readonly [] | readonly [string, string, string] | readonly [string, string] | readonly [string]`. Using optional chaining on the `amounts` property itself is unnecessary since it's always defined, though accessing `amounts[0]` on an empty array returns undefined which bnOrZero() handles safely.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-05T23:36:13.214Z
Learnt from: premiumjibles
PR: shapeshift/web#10187
File: src/state/slices/preferencesSlice/selectors.ts:21-25
Timestamp: 2025-08-05T23:36:13.214Z
Learning: The AssetId type from 'shapeshiftoss/caip' package is a string type alias, so it can be used directly as a return type for cache key resolvers in re-reselect selectors without needing explicit string conversion.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
📚 Learning: 2025-08-14T17:54:32.563Z
Learnt from: gomesalexandre
PR: shapeshift/web#10276
File: src/pages/ThorChainLP/components/ReusableLpStatus/ReusableLpStatus.tsx:97-108
Timestamp: 2025-08-14T17:54:32.563Z
Learning: In ReusableLpStatus component (src/pages/ThorChainLP/components/ReusableLpStatus/ReusableLpStatus.tsx), the txAssets dependency is stable from first render because poolAsset, baseAsset, actionSide, and action are all defined first render, making the current txAssetsStatuses initialization pattern safe without needing useEffect synchronization.
Applied to files:
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx
🧬 Code graph analysis (2)
src/components/AssetSearch/components/AssetList.tsx (2)
src/components/AssetSearch/components/AssetRow.tsx (1)
AssetRowData(35-42)src/pages/Markets/hooks/usePortalsAssetsQuery.ts (1)
PortalsAssets(15-19)
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx (5)
src/theme/theme.tsx (1)
breakpoints(37-44)src/state/slices/common-selectors.ts (3)
selectAssetsSortedByMarketCapUserCurrencyBalanceCryptoPrecisionAndName(270-332)selectRelatedAssetIdsByAssetIdInclusive(164-178)selectPortfolioUserCurrencyBalances(144-162)src/state/slices/assetsSlice/selectors.ts (1)
selectPrimaryAssets(44-50)src/components/Text/Text.tsx (1)
Text(19-83)src/components/AssetSearch/components/AssetList.tsx (1)
AssetList(34-162)
⏰ 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). (1)
- GitHub Check: Install and Cache
🔇 Additional comments (5)
src/components/TradeAssetSearch/components/SearchTermAssetList.tsx (2)
13-15: Primary-assets wiring and responsive height look goodUsing selectPrimaryAssets + selectRelatedAssetIdsByAssetIdInclusive and responsive 50vh/70vh via theme breakpoints is consistent with the grouping model and UX.
Also applies to: 24-27, 49-56
191-211: UI glue LGTMHeader copy, showRelatedAssets flag, dynamic height, and loading state composition are coherent here.
src/components/AssetSearch/components/AssetList.tsx (3)
22-28: Row typing and prop forwarding are correctTyping rowComponent as FC and forwarding showPrice/onImportClick/showRelatedAssets keeps custom rows type-safe.
Also applies to: 83-94
32-32: Overscan 1000px may increase work on low-end devices — verify before shipIncreasing overscan to 1000 with extra viewport padding improves scroll smoothness but can render many offscreen rows. Validate perf on mobile and wallet drawer contexts.
Suggested quick check:
- Measure frame timings while rapidly scrolling the Trade search and Assets page on low-end Android/iOS.
- If jank observed, try overscan 400–600 or tune increaseViewportBy first.
Also applies to: 154-159
99-107: Loading/empty states LGTMSkeleton padding and empty state copy are consistent with the rest of AssetSearch.
Also applies to: 127-133
Description
Introducing grouping of assets everywhere it's useful (Trade search, global search, account page, assets page, explore page on mobile, wallet mobile page)
AssetRowalmost everywhere we can, except for portal assets which is quite different-- Improve assetrow to support showing prices or import button if it's a custom asset
Issue (if applicable)
closes #10339
Risk
High considering first reviewer suggestion
Testing
-- All those parts should include the grouped asset including the total balance OR price depending on the scope of this
Engineering
Operations
Screenshots (if applicable)
Summary by CodeRabbit
New Features
Improvements
Chores