feat: add token chart to trending assets section #1987
Conversation
|
Warning Rate limit exceeded@Nick-1979 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a TokenChart React component with CoinGecko sparkline fetching and modal charting; wires click-to-open from TrendingAssets; introduces an Alert portal and corresponding DOM root; updates chart-related dependencies; and adds an English translation key for failed resets. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TrendingAssets
participant Asset
participant TokenChart
participant CoinGecko
participant AlertPortal
User->>Asset: click asset
Asset->>TrendingAssets: onClick(symbol)
TrendingAssets->>TrendingAssets: setChartToken(symbol)
TrendingAssets->>TokenChart: render(coinId, vsCurrency)
TokenChart->>CoinGecko: fetch sparkline(coinId, vsCurrency) (with timeout)
CoinGecko-->>TokenChart: sparkline data (7d)
TokenChart->>TokenChart: build time-series, render Line chart
TokenChart-->>User: display interactive modal chart
TokenChart->>AlertPortal: show error alert (on fetch failure)
Note right of TokenChart: auto-refresh interval runs periodically
User->>TokenChart: close
TokenChart->>TrendingAssets: onClose()
TrendingAssets->>TrendingAssets: clear chartToken (unmount)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (1)
134-153: Consider adding safety checks for scale access.The code accesses
chart.scales['x']andchart.scales['y']without verifying they exist. While Chart.js typically initializes these, adding optional chaining would improve robustness.Apply this diff:
chart.tooltip?.setActiveElements( [ { datasetIndex: 0, index: maxIndex } ], - { x: chart.scales['x'].getPixelForValue(maxIndex), y: chart.scales['y'].getPixelForValue(priceData[maxIndex].value) } + { x: chart.scales['x']?.getPixelForValue(maxIndex) ?? 0, y: chart.scales['y']?.getPixelForValue(priceData[maxIndex].value) ?? 0 } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
package.json(1 hunks)packages/extension-polkagate/src/fullscreen/components/LineChart.tsx(1 hunks)packages/extension-polkagate/src/fullscreen/home/TrendingAssets.tsx(6 hunks)packages/extension/public/locales/en/translation.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (1)
packages/extension-polkagate/src/fullscreen/components/DraggableModal.tsx (1)
DraggableModal(30-172)
packages/extension-polkagate/src/fullscreen/home/TrendingAssets.tsx (2)
packages/extension-polkagate/src/util/types.ts (1)
PriceValue(638-643)packages/extension-polkagate/src/components/contexts.tsx (1)
CurrencyContext(41-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Analyze (javascript)
🔇 Additional comments (12)
packages/extension/public/locales/en/translation.json (1)
825-825: LGTM!The new translation key follows the established pattern in this file.
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (6)
1-14: LGTM!Proper Chart.js setup with time-scale support via the date-fns adapter.
16-26: LGTM!Clear interface definitions with appropriate types.
70-79: LGTM!Proper setup and cleanup of the auto-refresh interval with correct dependency array.
81-102: LGTM!The chart data configuration properly maps price data to visual elements with dynamic segment coloring based on price movement.
104-132: LGTM!Well-configured chart options with appropriate time-scale settings and formatted axis labels.
155-166: LGTM!Clean render using the DraggableModal wrapper with the Chart.js Line component.
packages/extension-polkagate/src/fullscreen/home/TrendingAssets.tsx (5)
18-18: LGTM!Proper import of the new TokenChart component.
59-66: LGTM!The Asset component is now properly wired to report clicks via the onClick prop.
81-81: LGTM!Proper click handler and cursor styling for interactive asset cards.
Also applies to: 88-88
124-124: LGTM!The state management and price ID lookup logic correctly maps the selected asset symbol to its CoinGecko price ID.
Also applies to: 131-137
184-184: LGTM!Correctly passes the state setter to enable asset selection.
| "better-react-spinkit": "^2.0.4", | ||
| "chance": "^1.1.11", | ||
| "chart.js": "^4.2.1", | ||
| "chartjs-adapter-date-fns": "^3.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify dependency compatibility and security advisories.
The new chart-related dependencies enable the time-scale functionality for the TokenChart component. Please verify:
- Compatibility between
chartjs-adapter-date-fns^3.0.0,date-fns^4.1.0, and the existingchart.js^4.2.1 - Any known security vulnerabilities in these versions
Run the following script to check for security advisories:
Also applies to: 87-87, 92-92
🏁 Script executed:
#!/bin/bash
# Check for security vulnerabilities in the new chart dependencies
echo "=== Checking chartjs-adapter-date-fns ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "chartjs-adapter-date-fns") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
echo -e "\n=== Checking date-fns ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "date-fns") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
echo -e "\n=== Checking react-chartjs-2 ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "react-chartjs-2") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 250
[/web_search]
🌐 Web query:
chartjs-adapter-date-fns 3.0.0 compatibility chart.js 4.2.1
💡 Result:
Short answer: Yes — [email protected] works with Chart.js 4.2.1, but watch how you import it.
Notes and references:
- The adapter's README states it supports Chart.js 2.8.0 or later (i.e. v3 and v4 are supported). [1]
- npm / package listings show v3.0.0 is a released adapter version. [2]
- Practical caveat: when using bundlers (webpack/CRA/etc.) import the adapter's ESM build so it registers the same Chart class instance Chart.js uses — some users had to ensure the package.json entrypoints point to the ESM file (or patch resolve) to avoid the adapter attaching to a different Chart instance (issue/discussion). [3]
If you want, I can give the exact import/snippet for your bundler (webpack, Vite, CRA) to avoid the common ESM/CJS mismatch.
Sources:
[1] chartjs-adapter-date-fns README (GitHub). [2] chartjs-adapter-date-fns npm / package info. [3] Chart.js discussion about CJS/ESM import mismatch and adapter registration.
🌐 Web query:
date-fns 4.1.0 chartjs-adapter-date-fns peer dependency
💡 Result:
Short answer: No — chartjs-adapter-date-fns does not list date-fns v4.1.0 as its peer; the adapter targets date-fns 2.x (older 0.1.x releases targeted date-fns 1.x). See the adapter README (requires date-fns >= 2.0.0) and npm package info. [1][2]
Sources:
- chartjs-adapter-date-fns README (GitHub) — requires date-fns 2.0.0 or later. [1]
- chartjs-adapter-date-fns npm listing / release notes (shows older 0.1.x required date-fns 1.x; later versions require 2.x). [2]
If you need compatibility with date-fns 4.x, tell me which Chart.js and adapter version you're using and I can check the adapter's package.json or open an issue/PR upstream.
🌐 Web query:
react-chartjs-2 5.3.0 peer dependencies
💡 Result:
For react-chartjs-2 v5.3.0 the peerDependencies are:
- chart.js: ^4.0.0. [1][2]
- react: ^16.8.0 || ^17.0.0 || ^18.0.0. [2][3]
Sources: npm package info and docs. [1][2][3]
Critical: date-fns version incompatibility with chartjs-adapter-date-fns.
Security advisories check passed for all three packages. However, there is a version incompatibility:
chartjs-adapter-date-fns^3.0.0 targetsdate-fns2.x (requires >= 2.0.0), not 4.x- The PR specifies
date-fns^4.1.0, which is incompatible with the adapter
Either downgrade date-fns to ^2.0.0 or upgrade chartjs-adapter-date-fns to a version supporting date-fns 4.x (if available).
Note: chart.js ^4.2.1 and react-chartjs-2 ^5.3.0 are compatible with each other.
🤖 Prompt for AI Agents
In package.json around line 84, chartjs-adapter-date-fns is set to ^3.0.0 which
targets date-fns v2.x but the PR uses date-fns ^4.1.0, causing an
incompatibility; fix by either downgrading date-fns to a 2.x range (e.g., ^2.0.0
or a current 2.x patch) or by upgrading chartjs-adapter-date-fns to a release
that explicitly supports date-fns 4.x (verify the adapter’s changelog/registry
for the correct version), then update package.json accordingly and run npm/yarn
install and tests to ensure no breakage.
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (2)
107-107: Use standardsetIntervalinstead ofwindow.setInterval.While
window.setIntervalworks, the standard convention in JavaScript/TypeScript is to usesetIntervaldirectly (the global reference is implicit).Apply this diff:
- const id = window.setInterval(fetchPriceData, intervalSec * 1000); + const id = setInterval(fetchPriceData, intervalSec * 1000);
178-192: Be aware of potential fragility with Chart.js internal APIs.Directly manipulating
chart.tooltipandchart.scalesaccesses internal Chart.js APIs that could change in future versions. While the defensive checks help, consider monitoring Chart.js release notes for breaking changes to these APIs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx(1 hunks)packages/extension-polkagate/src/partials/Alert.tsx(3 hunks)packages/extension-ui/src/Popup/contexts/AlertProvider.tsx(1 hunks)packages/extension/public/index.html(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension-ui/src/Popup/contexts/AlertProvider.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
packages/extension-polkagate/src/partials/Alert.tsx (1)
packages/extension-polkagate/src/util/constants.ts (1)
TIME_TO_REMOVE_ALERT(314-314)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (3)
packages/extension-polkagate/src/components/translate.ts (1)
useTranslation(8-10)packages/extension-polkagate/src/hooks/useAlerts.ts (1)
useAlerts(12-31)packages/extension-polkagate/src/fullscreen/components/DraggableModal.tsx (1)
DraggableModal(30-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (10)
packages/extension/public/index.html (1)
29-29: LGTM! Clean portal root addition.The
alert-rootdiv provides a proper mounting point for portal-based alert rendering, correctly positioned outside the main application root.packages/extension-polkagate/src/partials/Alert.tsx (1)
58-125: LGTM! Portal-based alert rendering implemented correctly.The alert is properly wrapped in
AlertPortalwith appropriate fixed positioning and styling. The expandediconMappingcovers all severity types, and the progress animation is preserved within the new structure.packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (8)
1-17: LGTM!The imports are well-organized and all necessary Chart.js components are properly registered for the line chart with time scale functionality.
19-29: LGTM!Type definitions are clear and correctly structured. The use of optional properties with sensible defaults is appropriate.
31-51: LGTM!The timeout implementation using
AbortControlleris correct and robust. It properly cleans up the timer in both success and error paths, addressing the API reliability concerns from previous reviews.
54-54: Consider a longer default refresh interval to avoid rate limiting.The 60-second default refresh interval may trigger CoinGecko's rate limits, especially if multiple charts are opened. Consider increasing to 300 seconds (5 minutes) or implementing exponential backoff.
114-135: LGTM!The chart data structure is well-designed with dynamic segment coloring based on price movement. The memoization is properly configured with the correct dependencies.
157-157: Verify time unit alignment with data granularity.The time scale uses
unit: 'day'but the data generation at line 87 implies hourly intervals. This mismatch could cause improper time axis rendering or unexpected grouping.
195-209: LGTM!The render implementation is clean and properly structured. The CoinGecko attribution is good practice and complies with their API terms of use.
85-90: No changes needed — assumption is correct.CoinGecko's sparkline_in_7d returns 168 hourly values (one per hour for 7 days), so the code's hourly interval assumption (60 * 60 * 1000) is accurate and properly aligned with the API specification. The existing implementation is correct.
Likely an incorrect or invalid review comment.
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (1)
126-136: Strengthen data validation checks.The validation logic still has the issues flagged in previous reviews:
- Line 126's
if (!data)check won't catch an empty array response ([]), leading to potential undefined access at line 132- Line 132 uses optional chaining after the check, indicating the flow is unclear
- Line 134 silently returns without notifying the user when sparkline data is unavailable
Apply this diff to consolidate and strengthen the validation:
- if (!data) { + if (!data || !Array.isArray(data) || data.length === 0) { notify(t('Something went wrong while fetching token data!'), 'info'); return; } - const sparkLinePrices = data[0]?.sparkline_in_7d?.price as number[]; + const sparkLinePrices = data[0].sparkline_in_7d?.price as number[]; - if (!sparkLinePrices) { + if (!sparkLinePrices || !Array.isArray(sparkLinePrices) || sparkLinePrices.length === 0) { + notify(t('Sparkline data not available for this token.'), 'info'); return; }
🧹 Nitpick comments (2)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (2)
160-167: Consider increasing the default refresh interval.The 60-second default refresh interval may be aggressive for CoinGecko's rate limits, especially if multiple users are viewing charts simultaneously. Consider increasing it to 5 minutes (300 seconds) or implementing exponential backoff on rate limit errors.
Apply this diff to use a more conservative default:
- intervalSec = 60, + intervalSec = 300,
197-206: Clarify comment about tick limit.The comment at line 203 states "force exactly 7 labels," but
maxTicksLimitonly sets a maximum, not an exact count. Chart.js may display fewer labels depending on available space and data.Apply this diff to clarify:
- maxTicksLimit: 7, // force exactly 7 labels + maxTicksLimit: 7, // limit to maximum 7 labels
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (3)
packages/extension-polkagate/src/components/translate.ts (1)
useTranslation(8-10)packages/extension-polkagate/src/hooks/useAlerts.ts (1)
useAlerts(12-31)packages/extension-polkagate/src/fullscreen/components/DraggableModal.tsx (1)
DraggableModal(30-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (9)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (9)
1-17: LGTM!The imports and Chart.js setup are well-organized and appropriate for the component's functionality.
19-29: LGTM!Type definitions are clear and well-documented.
31-51: LGTM!The timeout implementation using
AbortControlleris modern and robust, properly handling cleanup in all code paths. This effectively addresses the API timeout concern from previous reviews.
53-106: LGTM!The gradient fill plugin is well-implemented with appropriate safety checks and proper canvas context management.
112-116: LGTM!Hooks and state are properly initialized and organized.
138-146: Verify timestamp calculation for CoinGecko data.The code assumes hourly spacing between sparkline data points (line 142), but CoinGecko's actual intervals may vary. While the comment acknowledges this is "approximate," it could lead to x-axis label misalignment if the actual data spacing differs significantly.
Consider verifying that this approximation is acceptable for your use case. If precision matters, you might need to either:
- Use CoinGecko's
/coins/{id}/market_chartendpoint which includes timestamps- Document that the x-axis represents an approximate timeline rather than exact timestamps
148-158: LGTM!Error handling is well-implemented with appropriate user feedback and severity levels for different error types.
217-243: LGTM!The automatic tooltip activation on the maximum price point is a nice UX touch, and the implementation properly checks for all required chart elements before attempting to manipulate them.
247-262: LGTM!The render structure is clean, and including CoinGecko attribution is good practice.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (3)
109-116: Reduce rate-limit risk and avoid overlapping fetches.
- Default to a slower poll (5 min).
- Gate concurrent requests with a ref and release in finally.
- Add small jitter to desynchronize clients.
-const TokenChart: React.FC<TokenChartProps> = ({ coinId, - intervalSec = 60, +const TokenChart: React.FC<TokenChartProps> = ({ coinId, + intervalSec = 300, onClose, vsCurrency = 'usd' }) => { const theme = useTheme(); const { t } = useTranslation(); const chartRef = useRef<ChartJS<'line'>>(null); const { notify } = useAlerts(); + const isFetchingRef = useRef(false); @@ - const fetchPriceData = useCallback(async () => { + const fetchPriceData = useCallback(async () => { + if (isFetchingRef.current) { + return; + } + isFetchingRef.current = true; try { const res = await fetchWithTimeout( `https://api.coingecko.com/api/v3/coins/markets?vs_currency=${vsCurrency}&ids=${coinId.toLowerCase()}&sparkline=true` ); @@ - notify(message, isTimeOut ? 'warning' : 'error'); + notify(message, isTimeOut ? 'warning' : 'error'); + } finally { + isFetchingRef.current = false; } }, [coinId, notify, t, vsCurrency]); @@ - const id = window.setInterval(fetchPriceData, intervalSec * 1000); + const jitterMs = Math.floor(Math.random() * 5000); + const id = window.setInterval(fetchPriceData, intervalSec * 1000 + jitterMs);Also applies to: 119-159, 161-164
139-147: Avoid assuming hourly sampling; prefer endpoint with timestamps.
sparkline_in_7d.pricelacks timestamps; spacing may vary. Consider/coins/{id}/market_chart?vs_currency=...&days=7&interval=hourlyand map returned[timestamp, price]pairs for accurate x-axis.Example:
const res = await fetchWithTimeout( `https://api.coingecko.com/api/v3/coins/${coinId.toLowerCase()}/market_chart?vs_currency=${vsCurrency}&days=7&interval=hourly&precision=2` ); if (!res.ok) { /* handle */ return; } const { prices } = await res.json() as { prices: [number, number][] }; setPriceData(prices.map(([time, value]) => ({ time, value })));Is CoinGecko's /coins/markets sparkline_in_7d.price guaranteed hourly? Which documented endpoint returns 7‑day prices with timestamps?
127-137: Strengthen data validation and surface errors to users.Cover empty array/non-array responses and missing sparkline; always notify.
- if (!data) { + if (!data || !Array.isArray(data) || data.length === 0) { notify(t('Something went wrong while fetching token data!'), 'info'); return; } - const sparkLinePrices = data[0]?.sparkline_in_7d?.price as number[]; + const sparkLinePrices = data[0].sparkline_in_7d?.price as number[] | undefined; - if (!sparkLinePrices) { - return; - } + if (!Array.isArray(sparkLinePrices) || sparkLinePrices.length === 0) { + notify(t('Sparkline data not available for this token.'), 'info'); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (3)
packages/extension-polkagate/src/components/translate.ts (1)
useTranslation(8-10)packages/extension-polkagate/src/hooks/useAlerts.ts (1)
useAlerts(12-31)packages/extension-polkagate/src/fullscreen/components/DraggableModal.tsx (1)
DraggableModal(30-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (1)
packages/extension-polkagate/src/fullscreen/components/LineChart.tsx (1)
31-51: Nice: timeout with AbortController.Good addition; prevents hung requests and enables clean error handling.
Summary by CodeRabbit
New Features
Improvements
Chores
Documentation