chore: relax-price-api-condition-legacy-chart and update to time based index#33204
Conversation
Lower MIN_COVERAGE_RATIO from 0.95 to 0.50 so the legacy price chart only shows the no-data overlay when less than 50% of the expected time period is covered by the API response. Previously, the chart would show the overlay when coverage was below 95%, which was too aggressive for newly-listed tokens or tokens with limited price history. Chart library audit: react-native-svg-charts AreaChart uses index-based x-axis positioning (not timestamps), so it already begins painting at the first data point without padding or stretching to fill a fixed time range. No chart library changes are needed. Co-authored-by: sahar-fehri <sahar.fehri@consensys.net>
github-actions
Bot
added
the
pr-not-ready-for-e2e
metamask-ci
Bot
added
team-assets
INVALID-PR-TEMPLATE
PR template — items to address before "Ready for review"Warnings — informational, address before merging:
See docs/readme/ready-for-review.md for the full Definition of Ready for Review. |
Switch PriceChart from index-based to time-based x-axis so partial data (e.g. 14h of data in a 24h window) renders at its correct temporal position within the full time range. Changes: - PriceChart.tsx: add timePeriodMs prop; use xAccessor/yAccessor to extract timestamp and price from TokenPrice tuples; set xMin/xMax to span the full time window (Date.now()-timePeriodMs .. Date.now()); rewrite updatePosition touch logic with pixel→timestamp→binary-search; fix Tooltip and EndDot to use timestamp-based x positioning; fix stale PanResponder closure by routing through refs. - Price.legacy.tsx: pass TIME_PERIOD_MS[timePeriod] as timePeriodMs prop to PriceChart (null for 'all' falls back to index-based). - tokenOverviewChart.constants.ts: add TIME_PERIOD_MS mapping (TimePeriod → milliseconds, null for 'all'). Co-authored-by: sahar-fehri <sahar.fehri@consensys.net>
With time-based x-axis, partial data renders correctly (gap on left). Only show no-data overlay for extremely sparse responses (<20%). Co-authored-by: Cursor <cursoragent@cursor.com>
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
sahar-fehri
removed
the
pr-not-ready-for-e2e
…ta-conditions-a279
…MetaMask/metamask-mobile into cursor/price-chart-data-conditions-a279 Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # app/components/UI/AssetOverview/Price/tokenOverviewChart.constants.ts
github-actions
Bot
added
size-M
risk:high
| */ | ||
| const HOURS = 3_600_000; | ||
| const DAYS = 24 * HOURS; | ||
| export const TIME_PERIOD_MS: Record<TimePeriod, number | null> = { |
There was a problem hiding this comment.
TIME_PERIOD_MS tells PriceChart how wide the x-axis should be in real time.
Without it, the chart only knows the data points it received — it has no idea what "1D" or "1W" means. So when the API returns 14 hours of data, it would stretch those points across the full width (the old behavior).
| * Binary-search for the data point whose timestamp is closest to `target`. | ||
| * Assumes `sortedPrices` is sorted ascending by timestamp. | ||
| */ | ||
| function findNearestIndex(sortedPrices: TokenPrice[], target: number): number { |
There was a problem hiding this comment.
Before, the chart was index-based — the x-axis was just array positions (0, 1, 2, 3...), not timestamps. So the touch mapping was trivial:
Pixel → divide by spacing → you get the exact array index. No searching needed because the data points are evenly spaced across the chart width by definition.
With time-based x-axis, data points are not evenly spaced — they sit at their real timestamps. Two points might be 5 minutes apart, then the next one 30 minutes later. So a pixel position no longer maps cleanly to an array index. You need the binary search to find which actual data point is closest to the timestamp the user is pointing at.
Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
risk:medium
🧪 Flaky unit test detectionRun history flaky detectionHistorical failure rate is a hint, not proof — review each suggestion in context. See the flaky-test-detection skill for the full pattern reference and manual audit workflow. Failures / runs sampled per window:
AI-detected flaky patterns
|
sahar-fehri
changed the title
….legacy Cover findNearestIndex binary search (empty, single, exact, between, before/after), time-based rendering, time-based touch/scrub, EndDot with time-based x, and the timePeriodMs pass-through (including the "all" → undefined fallback) to close SonarCloud coverage gaps. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Addresses flaky test detection bot J7 pattern (non-deterministic data). Co-authored-by: Cursor <cursoragent@cursor.com>
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection: Performance Test Selection: |
|
| export function findNearestIndex( | ||
| sortedPrices: TokenPrice[], | ||
| target: number, | ||
| ): number { | ||
| if (sortedPrices.length === 0) return -1; | ||
| let lo = 0; | ||
| let hi = sortedPrices.length - 1; | ||
| while (lo < hi) { | ||
| const mid = (lo + hi) >> 1; | ||
| if (Number(sortedPrices[mid][0]) < target) { | ||
| lo = mid + 1; | ||
| } else { | ||
| hi = mid; | ||
| } | ||
| } | ||
| if (lo > 0) { | ||
| const diffLo = Math.abs(Number(sortedPrices[lo][0]) - target); | ||
| const diffPrev = Math.abs(Number(sortedPrices[lo - 1][0]) - target); | ||
| if (diffPrev < diffLo) return lo - 1; | ||
| } | ||
| return lo; | ||
| } |
There was a problem hiding this comment.
Hmm do we need a custom binary search impl here?
There was a problem hiding this comment.
Claude is recommending d3-array - which we already pull in as a dep.
import { bisector } from 'd3-array';
const tokenPriceBisector = bisector((p: TokenPrice) => Number(p[0]));
...
const idx = tokenPriceBisector.center(prices, targetTs);
onActiveIndexChange(idx);There was a problem hiding this comment.
Hey! d3-array isn't actually a direct dependency.. it's only pulled in transitively via react-native-svg-charts. Adding it as a direct dep just for one bisector call felt like overkill, and the custom binary search is only ~15 lines. Happy to add it as a direct dep if you feel strongly though, lmk!
There was a problem hiding this comment.
yeeeah fair enough... I see you added some tests to prove this logic. I think its okay to keep for now.
| const now = Date.now(); | ||
| return { chartXMin: now - timePeriodMs, chartXMax: now }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isTimeBased, timePeriodMs, prices]); |
There was a problem hiding this comment.
how come we need this eslint rule disable?
There was a problem hiding this comment.
The memo uses Date.now() to set the chart's right edge to "now." Without prices in the dep array, the memo wouldn't re-run when fresh data arrives — so if the user stays on screen and a new fetch happens, the x-domain would stay anchored to the original Date.now() and the latest data points could fall outside the visible window. Adding prices as a dep forces a recalculation of the time anchor whenever new data comes in.
| if (isTimeBased && chartXMin != null && chartXMax != null) { | ||
| const rangeMax = chartWidth - endDotInsetRight; | ||
| const clamped = Math.max(0, Math.min(pixelX, rangeMax)); | ||
| const fraction = rangeMax > 0 ? clamped / rangeMax : 0; | ||
| const targetTs = chartXMin + fraction * (chartXMax - chartXMin); | ||
| const idx = findNearestIndex(prices, targetTs); | ||
| onActiveIndexChange(idx); | ||
| } else { | ||
| const xDistance = chartWidth / priceList.length; | ||
| const clamped = Math.max(0, Math.min(pixelX, chartWidth)); | ||
| let value = Number((clamped / xDistance).toFixed(0)); | ||
| if (value >= priceList.length - 1) { | ||
| value = priceList.length - 1; | ||
| } | ||
| onActiveIndexChange(value); |
There was a problem hiding this comment.
math heavy... but looks like this is unavoidable.
metamask-ci
Bot
added
the
release-8.4.0
3 participants
Description
Purpose
This PR improves the legacy price chart on the Token Details Page (TDP) with two changes:
MIN_COVERAGE_RATIOfrom 0.95 to 0.20. With the time-based x-axis, partial data is no longer misleading, so the no-data overlay only triggers for extremely sparse responses (<20% of the requested period).Changes
app/components/UI/AssetOverview/PriceChart/PriceChart.tsxtimePeriodMs?: numberpropxAccessor/yAccessor/xScalewith d3scaleTimefor time-based positioningxMin = Date.now() - timePeriodMsandxMax = Date.now()x(timestamp)instead ofx(index)app/components/UI/AssetOverview/Price/Price.legacy.tsxTIME_PERIOD_MS[timePeriod]astimePeriodMsto PriceChartapp/components/UI/AssetOverview/Price/tokenOverviewChart.constants.tsTIME_PERIOD_MSmapping (TimePeriod → number | null)app/components/hooks/useTokenHistoricalPrices.tsMIN_COVERAGE_RATIOfrom 0.95 to 0.20Scope
Only the Token Details Page (TDP) is affected.
TraderPriceChart.tsxand other consumers are not modified.Changelog
CHANGELOG entry: null
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/ASSETS-3618
Manual testing steps
Screenshots/Recordings
Before
Chart stretches partial data (14h) across full width, making it look like 24h of data.
After
Chart correctly shows a gap on the left, with data starting at its actual first timestamp.
Screen.Recording.2026-07-13.at.18.39.24.mov
Pre-merge author checklist
Performance checks (if applicable)
Pre-merge reviewer checklist
Note
Medium Risk
Chart rendering and touch-selection behavior change on token details; scope is UI-only but users may misread partial history if the new gap layout is wrong.
Overview
The legacy token overview PriceChart can use a time-based x-axis when
timePeriodMsis passed fromPrice.legacyvia newTIME_PERIOD_MSconstants. Fixed windows anchor fromnow - periodtonow, so sparse history (e.g. 14h in 1D) draws at the correct time with a left gap instead of stretching across the full width. ALL still omitstimePeriodMsand keeps index-based layout.Touch/scrub maps pixels to timestamps and uses exported
findNearestIndex; tooltip, end dot, andAreaChartaccessors switch between timestamp and index. PanResponder handlers go through refs to avoid stale closures.hasInsufficientTimeCoveragethreshold drops from 95% to 20% (MIN_COVERAGE_RATIO), so the no-data overlay only appears for very sparse API responses; docs/comments align with the 50% boundary behavior. Tests cover pass-through, time-based touch,findNearestIndex, and updated coverage cases.Reviewed by Cursor Bugbot for commit dd06940. Bugbot is set up for automated code reviews on this repo. Configure here.