refactor: use Array.reduce instead of Object.groupBy #1652
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx (1)
164-167: Add safety check for empty asset groupsThe code assumes that
groupedAssets[index]is non-empty. While this might be guaranteed by the logic above, it's better to be explicit.Consider adding a safety check:
- const assetSample = groupedAssets[index][0]; + const assets = groupedAssets[index]; + if (!assets.length) { + return null; + } + const assetSample = assets[0];Also, consider renaming
accumulatedPricePerAssettoaccumulatedBalanceas it represents the sum of balances, not prices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx(1 hunks)
🔇 Additional comments (1)
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/TotalBalancePieChart.tsx (1)
152-162: LGTM! Clean implementation of grouping logic
The refactoring from Object.groupBy to Array.reduce is well-implemented. The composite key creation is clean, and the accumulator type is properly defined.
| @@ -146,19 +144,27 @@ function TotalBalancePieChart ({ hideNumbers, setGroupedAssets }: Props): React. | |||
| Object.keys(balances).forEach((address) => { | |||
| Object.keys(balances?.[address]).forEach((genesisHash) => { | |||
| if (!TEST_NETS.includes(genesisHash)) { | |||
| // @ts-ignore | |||
| allAccountsAssets = allAccountsAssets.concat(balances[address][genesisHash]); | |||
| allAccountsAssets = allAccountsAssets.concat(balances[address][genesisHash] as unknown as AssetsWithUiAndPrice[]); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing the double type assertion
The double type assertion as unknown as AssetsWithUiAndPrice[] bypasses TypeScript's type checking completely. This could hide potential type mismatches and make the code less type-safe.
Consider properly typing the balances structure or using a type guard instead:
interface AccountBalances {
[address: string]: {
[genesisHash: string]: AssetsWithUiAndPrice[];
};
}
Close: #1651
Summary by CodeRabbit
New Features
Bug Fixes