fix to get trading and migration data merged#336
Conversation
WalkthroughThe changes update the formatting of points values in leaderboard components by applying a formatting utility for improved readability, specifically adding thousands separators. Corresponding test assertions are updated to match the new formatted output. Additionally, internal logic for merging leaderboard data and migration data is refactored for correctness and efficiency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PointsCard
participant formatAmountDisplay
User->>PointsCard: Render with points value
PointsCard->>formatAmountDisplay: Format floored points value
formatAmountDisplay-->>PointsCard: Formatted points string
PointsCard-->>User: Display formatted points (e.g., "1,234 PX")
sequenceDiagram
participant App
participant getMergeLeaderboardData
participant MigrationDataMap
App->>getMergeLeaderboardData: Provide tradingData, migrationData
getMergeLeaderboardData->>MigrationDataMap: Build address-to-migration map
getMergeLeaderboardData->>getMergeLeaderboardData: For each trading entry, find and merge with migration entry
getMergeLeaderboardData-->>App: Return merged leaderboard data
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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. 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 (
|
Deploying x with
|
| Latest commit: |
3e4b10a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://8a1dc3cb.x-e62.pages.dev |
| Branch Preview URL: | https://feat-px-points-migration-boa.x-e62.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/apps/leaderboard/utils/index.tsx (2)
116-125: Minor: avoid O(n·m) look-ups by populating the map only onceBuilding
migrationMapinsideforEachis fine, but you iterateentry.addressesfor every migration row.
If address arrays are large, consider pre-flattening addresses or storing a Set per entry to reduce nested loops.
Not critical, but worth noting for scalability of >10k addresses.
157-163: Unmatched migration entries are copied but not deep-clonedYou spread the top-level object (
{ ...entry }) but nested mutable fields (addresses) remain by reference.
If later code mutatesaddresses, originals are affected.
Consider deep-cloning or at least cloningaddresseswith[...entry.addresses].src/apps/leaderboard/components/PointsCards/PointsCard.tsx (2)
1-3: Import is fine but consider tree-shaking
formatAmountDisplaypulls inIntl.NumberFormat; ensure this utility is already used widely to avoid duplicating polyfills for older browsers.
40-41: Optional: explicitly suppress decimals
Intl.NumberFormatwon’t add decimals for integers, but being explicit improves readability:- {formatAmountDisplay(Math.floor(points || 0))}{' '} + {formatAmountDisplay(Math.floor(points || 0), 0, 0)}{' '}src/apps/leaderboard/components/PointsCards/OverviewPointsCard.tsx (2)
38-41: Same comment as PointsCard – decimals can be suppressed explicitly- {formatAmountDisplay(Math.floor(myAllTimeMerged.entry?.totalPoints || 0))}{' '} + {formatAmountDisplay(Math.floor(myAllTimeMerged.entry?.totalPoints || 0), 0, 0)}{' '}
85-88: Repeat of previous suggestion for weekly points formattingApply same explicit fraction-digits parameters for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/apps/leaderboard/components/PointsCards/tests/__snapshots__/OverviewPointsCard.test.tsx.snapis excluded by!**/*.snapsrc/apps/leaderboard/components/PointsCards/tests/__snapshots__/PointsCard.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
src/apps/leaderboard/components/PointsCards/OverviewPointsCard.tsx(3 hunks)src/apps/leaderboard/components/PointsCards/PointsCard.tsx(2 hunks)src/apps/leaderboard/components/PointsCards/tests/OverviewPointsCard.test.tsx(1 hunks)src/apps/leaderboard/components/PointsCards/tests/PointsCard.test.tsx(1 hunks)src/apps/leaderboard/utils/index.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/apps/leaderboard/utils/index.tsx (1)
Learnt from: RanaBug
PR: pillarwallet/x#334
File: src/apps/leaderboard/utils/index.tsx:91-94
Timestamp: 2025-06-17T09:20:44.499Z
Learning: In src/apps/leaderboard/utils/index.tsx, the getLastWeekMigrationData function intentionally uses currentWeek (not lastWeek) for the completedSwapWeek lookup. This is correct business logic - when retrieving last week's migration data, the function should check swap completion against the current week while using lastWeek for points and USD calculations.
🧬 Code Graph Analysis (2)
src/apps/leaderboard/components/PointsCards/OverviewPointsCard.tsx (1)
src/utils/number.tsx (1)
formatAmountDisplay(3-33)
src/apps/leaderboard/components/PointsCards/PointsCard.tsx (1)
src/utils/number.tsx (1)
formatAmountDisplay(3-33)
🔇 Additional comments (3)
src/apps/leaderboard/components/PointsCards/tests/PointsCard.test.tsx (1)
48-48: Assertion update LGTMExpectation now matches formatted output with thousands separator. No further action.
src/apps/leaderboard/components/PointsCards/tests/OverviewPointsCard.test.tsx (1)
57-57: Formatted points assertion looks correctTest aligns with UI change; nothing else required.
src/apps/leaderboard/components/PointsCards/OverviewPointsCard.tsx (1)
4-6: Import aligns with other card – good consistency
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
Style
Bug Fixes
Tests