Skip to content

fix: use ohlcv data to calculate advanced chart percentage value#28574

Open
sahar-fehri wants to merge 2 commits intomainfrom
fix/fix-percentage-display-token-details-advanced-charts
Open

fix: use ohlcv data to calculate advanced chart percentage value#28574
sahar-fehri wants to merge 2 commits intomainfrom
fix/fix-percentage-display-token-details-advanced-charts

Conversation

@sahar-fehri
Copy link
Copy Markdown
Contributor

@sahar-fehri sahar-fehri commented Apr 8, 2026

Description

Fixes a bug where the Advanced Chart's percentage value remained static when users switched timeframes. The percentage now correctly updates to reflect the selected time range by calculating it from OHLCV data instead of legacy props.

Problem
When users switched timeframes in the Advanced Chart (e.g., from "Today" to "Past hour"), the descriptive text updated correctly but the percentage value remained static. This occurred because the percentage was calculated from legacy prices data that was based on a different time period than the Advanced Chart's selected time range.

Solution
Modified the Advanced Chart to calculate its own percentage from OHLCV data instead of using legacy props. This ensures the percentage always matches the selected time range.

Calculation Logic
Formula:

dynamicComparePrice = ohlcvData[0].close  // First candle's close price (start of time range)
dynamicPriceDiff = currentPrice - dynamicComparePrice
percentage = (dynamicPriceDiff / dynamicComparePrice) * 100

Changelog

CHANGELOG entry: fixed percentage display on advanced charts

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/ASSETS-3031

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Low Risk
UI-only calculation change scoped to the token overview advanced price header, with added test coverage; minimal risk beyond potential edge cases when OHLCV data is missing or reordered.

Overview
Fixes the Advanced Chart header change/percentage so it recomputes per selected timeframe by deriving comparePrice/priceDiff from the first OHLCV candle (instead of relying on legacy priceDiff/comparePrice props).

This removes priceDiff/comparePrice from PriceAdvancedProps, updates Price to pass only currentPrice/currentCurrency into PriceAdvanced, and adds tests covering OHLCV-based percentage calculation, empty-data behavior, and updates when the OHLCV dataset changes.

Reviewed by Cursor Bugbot for commit d4b6d20. Bugbot is set up for automated code reviews on this repo. Configure here.

@sahar-fehri sahar-fehri requested a review from a team as a code owner April 8, 2026 21:23
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

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.

@github-actions github-actions bot added size-M risk-low Low testing needed · Low bug introduction risk labels Apr 8, 2026
Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 50810bc. Configure here.

@github-actions github-actions bot added risk-low Low testing needed · Low bug introduction risk and removed risk-low Low testing needed · Low bug introduction risk labels Apr 9, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

🔍 Smart E2E Test Selection

  • Selected E2E tags: None (no tests recommended)
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: low
  • AI Confidence: 85%
click to see 🤖 AI reasoning details

E2E Test Selection:
The changes are confined to the AssetOverview/Price component area:

  1. Price.tsx: Minor prop forwarding change — currentPrice and currentCurrency are now explicitly destructured and passed to child components instead of being spread via ...rest. This is a safe refactor with no behavioral change.

  2. Price.advanced.tsx: Internal refactor removing priceDiff and comparePrice props from the interface. The component now derives these values dynamically from OHLCV chart data (first candle open price). This is a self-contained change within the advanced chart component.

  3. Price.advanced.test.tsx: Unit test updates matching the new interface, with new tests for the dynamic calculation behavior.

Why no E2E tags are needed:

  • These are UI component-level changes with no impact on core wallet flows (accounts, confirmations, swaps, networks, identity)
  • No E2E smoke tests directly test the token price chart display or price diff calculation
  • The changes are well-covered by the updated unit tests
  • No navigation, modal, or shared infrastructure components are affected
  • The TokenDetails view that uses this component is not directly tested in any smoke test suite
  • No controller, Engine, or state management changes

The risk is low — this is a self-contained UI refactor with proper unit test coverage.

Performance Test Selection:
The changes are a calculation refactor (moving price diff computation from props to derived from OHLCV data) within the Price component. This does not introduce new rendering cycles, additional API calls, or list rendering changes that would impact performance metrics. The OHLCV data is already fetched by the component; the change only adds two useMemo computations which are negligible. No performance tests are warranted.

View GitHub Actions results

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 9, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk-low Low testing needed · Low bug introduction risk size-M team-assets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants