Skip to content

chore: refetch chart data on pull down to refresh#29661

Draft
sahar-fehri wants to merge 6 commits into
mainfrom
fix/fix-pull-down-to-refresh-chart
Draft

chore: refetch chart data on pull down to refresh#29661
sahar-fehri wants to merge 6 commits into
mainfrom
fix/fix-pull-down-to-refresh-chart

Conversation

@sahar-fehri
Copy link
Copy Markdown
Contributor

@sahar-fehri sahar-fehri commented May 4, 2026

Description

Pull-to-refresh on the asset details page previously only refreshed the transaction list. The chart data (OHLCV candles) was never re-fetched, the spinner appeared but the chart remained stale.

This PR hooks the existing pull-to-refresh gesture into the OHLCV chart data fetch, so pulling down now also re-fetches fresh candlestick/line chart data from the Price API.

Changes

  • useOHLCVChart.ts: Exposes the existing loadInitial function as refetch in the hook's return value, allowing consumers to imperatively trigger a re-fetch.
  • Transactions/index.js: Accepts an optional onRefresh prop, called alongside the existing transaction refresh.
  • MultichainTransactionsView.tsx: Same as above for non-EVM transaction lists.
  • TokenDetails.tsx: Introduces a chartRefreshKey counter. Passes handleChartRefresh to both list components and chartRefreshKey down to the chart.
  • AssetOverviewContent.tsx, Price.tsx: Thread chartRefreshKey through to PriceAdvanced.
  • Price.advanced.tsx: Watches chartRefreshKey via useEffect; when it increments above 0, calls refetchChart() to re-fetch OHLCV data. Skips initial mount (key starts at 0).

Changelog

CHANGELOG entry: Refetch chart data on pull down to refresh on token details page.

Related issues

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

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

Screen.Recording.2026-05-04.at.16.05.40.mov

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

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

Medium Risk
Moderate risk because it changes the useOHLCVChart hook contract and wires pull-to-refresh to trigger additional network fetches, which could impact loading/abort behavior and refresh frequency.

Overview
Pull-to-refresh on the token details screen now refreshes both the transactions list and the advanced price chart data.

This exposes a refetch function from useOHLCVChart, threads a new chartRefreshKey prop from TokenDetails through AssetOverviewContent/Price into PriceAdvanced, and triggers refetch when the key increments (skipping the initial render and guarding against refetch function identity changes). Transactions refresh components (Transactions and MultichainTransactionsView) now accept an optional onRefresh callback to piggyback on their existing refresh behavior, and tests were added to validate the chartRefreshKey semantics.

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

@sahar-fehri sahar-fehri requested review from a team as code owners May 4, 2026 14:04
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 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 the pr-not-ready-for-e2e Skip E2E and block merging. Remove this label once the PR is ready to run the E2E tests. label May 4, 2026
@github-actions github-actions Bot added the size-S label May 4, 2026
Comment thread app/components/UI/AssetOverview/Price/Price.advanced.tsx Outdated
Comment thread app/components/UI/AssetOverview/Price/Price.advanced.tsx
juanmigdr
juanmigdr previously approved these changes May 4, 2026
Copy link
Copy Markdown
Member

@juanmigdr juanmigdr left a comment

Choose a reason for hiding this comment

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

LGTM althouh are we also refreshing the price and price change when ppulling down to refresh?

@sahar-fehri
Copy link
Copy Markdown
Contributor Author

re we also refreshing the price and price c

I think we are not, but i believe that is out of scope of this PR 🙏 let's track the balance and price refresh on a separate PR

Copy link
Copy Markdown
Contributor

@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 c3da206. Configure here.

Comment thread app/components/UI/AssetOverview/Price/Price.tsx Outdated
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 6, 2026

hasMore,
nextCursor,
hasEmptyData,
refetch: loadInitial,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

future - we can refactor to use tanstack query, has built in refetch mechanisms

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmmm debating if we want to move to tanstack now actually.

That way we can remove the refreshCounter logic and the state nice and clean.

Places that want to perform a refresh can easily invoke our hook and call the .refresh from the useQuery.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree with the broader vision, we have talked about this today; this comment here on a previous PR explains at a high level the alternative approach;
I thought we would do that when we cleanup the feature flag for the feature 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thinking that perhaps even with tanstack we would still need the counter logic 👀

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah right it wont be needed actually.
Hmm we could keep this bug open until we do the proper cleanup for the feature flag?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The 7.76.0 will be out to users tomorrow, we should be able to have it at 100% to users by next week then we can remove feature flag safely?

Copy link
Copy Markdown
Contributor

@Prithpal-Sooriya Prithpal-Sooriya May 6, 2026

Choose a reason for hiding this comment

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

Hmm good call out.
How critical is this bug?

If this critical/needs to be cherry-picked, then happy to get this merged and we can plan a refactor.

If not critical, then I don't mind we wait until a cleanup.

I'll try to propose a tanstack approach for this (refreshing can be done by the built in refresh mechanic, or by queryCache invalidation)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is a sev1. We can wait for a proper fix with tanstack once we clean up the FF.

The idea is to have all entry points that land on Token Details use tanstack query to cache security data; instead of passing it through nav params where shape mismatches can silently break things (which is exactly what's happening with the swap token selector today).

Token Details would then own its own data: check the query cache first, fetch on-demand if it's not there. On pull-to-refresh, we just call queryClient.invalidateQueries() for the relevant query key.. tanstack handles the refetch, deduplication, and cache update automatically. No counter approach needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Amaaazing, sorry to block this.

If you feel like this does need to get in later, then happy to get merged and resolve as a fast follow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not at all, good callout, if we block it now, this means less cleanup for later; unless someone reports this as a sev1, let's keep it open for now, and fix it properly with FF cleanup

@sahar-fehri sahar-fehri marked this pull request as draft May 6, 2026 15:06
@sahar-fehri
Copy link
Copy Markdown
Contributor Author

We decided to put this on hold for now

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

Labels

pr-not-ready-for-e2e Skip E2E and block merging. Remove this label once the PR is ready to run the E2E tests. size-M team-assets

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants