Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
sahar-fehri wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
841559b
chore: refetch chart data on pull down to refresh
sahar-fehri May 4, 2026
53ed368
chore: fix duplicate call
sahar-fehri May 4, 2026
91b9fd0
chore: add ut
sahar-fehri May 4, 2026
4f1a30d
Merge branch 'main' into fix/fix-pull-down-to-refresh-chart
sahar-fehri May 5, 2026
c3da206
Merge branch 'main' into fix/fix-pull-down-to-refresh-chart
sahar-fehri May 5, 2026
2f0f9bd
chore: nit
sahar-fehri May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions app/components/UI/AssetOverview/Price/Price.advanced.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ const ohlcvPaddingThree = [
{ time: 300, open: 91, high: 92, low: 90, close: 92, volume: 1 },
];

const mockRefetch = jest.fn();
const mockUseOHLCVChart = jest.fn().mockReturnValue({
ohlcvData: [
...ohlcvPaddingThree,
Expand All @@ -74,6 +75,7 @@ const mockUseOHLCVChart = jest.fn().mockReturnValue({
hasMore: false,
nextCursor: null,
hasEmptyData: false,
refetch: mockRefetch,
});

jest.mock('../../Charts/AdvancedChart/useOHLCVChart', () => ({
Expand Down Expand Up @@ -175,6 +177,7 @@ describe('PriceAdvanced', () => {
hasMore: false,
nextCursor: null,
hasEmptyData: false,
refetch: mockRefetch,
});
});

Expand Down Expand Up @@ -781,6 +784,84 @@ describe('PriceAdvanced', () => {
});
});

describe('chartRefreshKey pull-to-refresh', () => {
it('does not call refetch on initial render when chartRefreshKey is 0', () => {
render(<PriceAdvanced {...baseProps} chartRefreshKey={0} />);

expect(mockRefetch).not.toHaveBeenCalled();
});

it('does not call refetch when chartRefreshKey is undefined', () => {
render(<PriceAdvanced {...baseProps} />);

expect(mockRefetch).not.toHaveBeenCalled();
});

it('calls refetch when chartRefreshKey changes from 0 to 1', () => {
const { rerender } = render(
<PriceAdvanced {...baseProps} chartRefreshKey={0} />,
);

rerender(<PriceAdvanced {...baseProps} chartRefreshKey={1} />);

expect(mockRefetch).toHaveBeenCalledTimes(1);
});

it('calls refetch again when chartRefreshKey increments from 1 to 2', () => {
const { rerender } = render(
<PriceAdvanced {...baseProps} chartRefreshKey={0} />,
);

rerender(<PriceAdvanced {...baseProps} chartRefreshKey={1} />);
expect(mockRefetch).toHaveBeenCalledTimes(1);

rerender(<PriceAdvanced {...baseProps} chartRefreshKey={2} />);
expect(mockRefetch).toHaveBeenCalledTimes(2);
});

it('does not call refetch redundantly when refetch identity changes but chartRefreshKey stays the same', () => {
const refetchA = jest.fn();
const refetchB = jest.fn();

mockUseOHLCVChart.mockReturnValueOnce({
ohlcvData: [
...ohlcvPaddingThree,
{ time: 1000, open: 100, high: 101, low: 99, close: 100, volume: 1 },
{ time: 2000, open: 100, high: 106, low: 100, close: 105, volume: 1 },
],
isLoading: false,
error: undefined,
hasMore: false,
nextCursor: null,
hasEmptyData: false,
refetch: refetchA,
});

const { rerender } = render(
<PriceAdvanced {...baseProps} chartRefreshKey={1} />,
);
refetchA.mockClear();

mockUseOHLCVChart.mockReturnValueOnce({
ohlcvData: [
...ohlcvPaddingThree,
{ time: 1000, open: 100, high: 101, low: 99, close: 100, volume: 1 },
{ time: 2000, open: 100, high: 106, low: 100, close: 105, volume: 1 },
],
isLoading: false,
error: undefined,
hasMore: false,
nextCursor: null,
hasEmptyData: false,
refetch: refetchB,
});

rerender(<PriceAdvanced {...baseProps} chartRefreshKey={1} />);

expect(refetchB).not.toHaveBeenCalled();
});
});

describe('touch gesture handling', () => {
it('sets isChartBeingTouched to true on touch start', () => {
const { getByTestId } = render(<PriceAdvanced {...baseProps} />);
Expand Down
13 changes: 13 additions & 0 deletions app/components/UI/AssetOverview/Price/Price.advanced.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ export interface PriceAdvancedProps {
timePeriod?: TimePeriod;
chartNavigationButtons?: TimePeriod[];
setTimePeriod?: (period: TimePeriod) => void;
/** Monotonically increasing counter to trigger chart data re-fetch on pull-to-refresh. */
chartRefreshKey?: number;
}

const PriceAdvanced = ({
Expand All @@ -93,6 +95,7 @@ const PriceAdvanced = ({
timePeriod = '1d',
chartNavigationButtons = [],
setTimePeriod,
chartRefreshKey,
}: PriceAdvancedProps) => {
const dispatch = useDispatch();
const { trackEvent, createEventBuilder } = useAnalytics();
Expand Down Expand Up @@ -213,13 +216,23 @@ const PriceAdvanced = ({
hasMore,
nextCursor,
hasEmptyData,
refetch: refetchChart,
} = useOHLCVChart({
assetId,
timePeriod: config.timePeriod,
interval: config.interval,
vsCurrency: currentCurrency,
});

const refetchChartRef = useRef(refetchChart);
refetchChartRef.current = refetchChart;

useEffect(() => {
if (chartRefreshKey && chartRefreshKey > 0) {
refetchChartRef.current();
}
}, [chartRefreshKey]);
Comment thread
sahar-fehri marked this conversation as resolved.

const ohlcvPagination = useMemo(
() => ({
nextCursor,
Expand Down
4 changes: 4 additions & 0 deletions app/components/UI/AssetOverview/Price/Price.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ export type PriceProps = PriceSharedProps & {
timePeriod: TimePeriod;
chartNavigationButtons?: TimePeriod[];
setTimePeriod?: (period: TimePeriod) => void;
/** Monotonically increasing counter to trigger chart data re-fetch on pull-to-refresh. */
chartRefreshKey?: number;
};

const Price = (props: PriceProps) => {
Expand All @@ -43,6 +45,7 @@ const Price = (props: PriceProps) => {
setTimePeriod,
currentPrice,
currentCurrency,
chartRefreshKey,
...rest
} = props;

Expand All @@ -57,6 +60,7 @@ const Price = (props: PriceProps) => {
isLoading={isLoading}
currentPrice={currentPrice}
currentCurrency={currentCurrency}
chartRefreshKey={chartRefreshKey}
{...rest}
/>
);
Expand Down
12 changes: 11 additions & 1 deletion app/components/UI/Charts/AdvancedChart/useOHLCVChart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ export interface UseOHLCVChartResult {
nextCursor: string | null;
/** True if the API returned an empty data array (asset not supported for OHLCV) */
hasEmptyData: boolean;
/** Force a re-fetch of the initial data (e.g. on pull-to-refresh). */
refetch: () => void;
}

const mapCandle = (candle: OHLCVApiCandle): OHLCVBar => ({
Expand Down Expand Up @@ -148,5 +150,13 @@ export const useOHLCVChart = ({
return () => abortRef.current?.abort();
}, [loadInitial]);

return { ohlcvData, isLoading, error, hasMore, nextCursor, hasEmptyData };
return {
ohlcvData,
isLoading,
error,
hasMore,
nextCursor,
hasEmptyData,
refetch: loadInitial,
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.

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

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.

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

@sahar-fehri sahar-fehri May 6, 2026

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

@sahar-fehri sahar-fehri May 6, 2026

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

@sahar-fehri sahar-fehri May 6, 2026

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

@sahar-fehri sahar-fehri May 6, 2026

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 •
edited
Loading

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

@sahar-fehri sahar-fehri May 6, 2026

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

@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.

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

@sahar-fehri sahar-fehri May 6, 2026

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

};
};
8 changes: 8 additions & 0 deletions app/components/UI/TokenDetails/Views/TokenDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,11 @@ const TokenDetails: React.FC<{
const navigation = useNavigation();
const [isInsightsDisclaimerVisible, setIsInsightsDisclaimerVisible] =
useState(false);
const [chartRefreshKey, setChartRefreshKey] = useState(0);
const handleChartRefresh = useCallback(
() => setChartRefreshKey((k) => k + 1),
[],
);

const caip19AssetId = useMemo((): CaipAssetType | null => {
try {
Expand Down Expand Up @@ -244,6 +249,7 @@ const TokenDetails: React.FC<{
securityData={securityData}
isSecurityDataLoading={isSecurityDataLoading}
hasSecurityDataError={Boolean(securityDataError)}
chartRefreshKey={chartRefreshKey}
///: BEGIN:ONLY_INCLUDE_IF(tron)
stakedTrxAsset={stakedTrxAsset}
inLockPeriodBalance={inLockPeriodBalance}
Expand Down Expand Up @@ -282,6 +288,7 @@ const TokenDetails: React.FC<{
enableRefresh
showDisclaimer
location={TransactionDetailLocation.AssetDetails}
onRefresh={handleChartRefresh}
/>
) : (
<Transactions
Expand All @@ -301,6 +308,7 @@ const TokenDetails: React.FC<{
skipScrollOnClick
hideEmptyState
location={TransactionDetailLocation.AssetDetails}
onRefresh={handleChartRefresh}
/>
)}
{!txLoading && (
Expand Down
5 changes: 5 additions & 0 deletions app/components/UI/TokenDetails/components/AssetOverviewContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ export interface AssetOverviewContentProps {
isSecurityDataLoading?: boolean;
/** Whether the security data fetch failed. Hides the card when true. */
hasSecurityDataError?: boolean;

/** Monotonically increasing counter to trigger chart data re-fetch on pull-to-refresh. */
chartRefreshKey?: number;
}

/**
Expand Down Expand Up @@ -230,6 +233,7 @@ const AssetOverviewContent: React.FC<AssetOverviewContentProps> = ({
securityData,
isSecurityDataLoading = false,
hasSecurityDataError = false,
chartRefreshKey,
}) => {
const { styles } = useStyles(styleSheet, {});
const navigation = useNavigation();
Expand Down Expand Up @@ -693,6 +697,7 @@ const AssetOverviewContent: React.FC<AssetOverviewContentProps> = ({
currentPrice={currentPrice}
comparePrice={comparePrice}
isLoading={isLoading}
chartRefreshKey={chartRefreshKey}
/>
{!isTokenTradingOpen(token as BridgeToken) && (
<View style={styles.marketClosedActionButtonContainer}>
Expand Down
5 changes: 5 additions & 0 deletions app/components/UI/Transactions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,10 @@ class Transactions extends PureComponent {
hideAwaitingConfirmation: PropTypes.func,
showHardwareWalletError: PropTypes.func,
}),
/**
* Optional extra callback invoked alongside the built-in transaction refresh.
*/
onRefresh: PropTypes.func,
};

static defaultProps = {
Expand Down Expand Up @@ -400,6 +404,7 @@ class Transactions extends PureComponent {
onRefresh = async () => {
this.setState({ refreshing: true });

this.props.onRefresh?.();
await updateIncomingTransactions();

this.setState({ refreshing: false });
Expand Down
8 changes: 7 additions & 1 deletion app/components/Views/MultichainTransactionsView/MultichainTransactionsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ interface MultichainTransactionsViewProps {
* Location context for analytics tracking (home or asset_details)
*/
location?: TransactionDetailLocation;
/**
* Optional extra callback invoked alongside the built-in transaction refresh.
*/
onRefresh?: () => void;
}

const MultichainTransactionsView = ({
Expand All @@ -86,6 +90,7 @@ const MultichainTransactionsView = ({
showDisclaimer = false,
onScroll,
location,
onRefresh: onRefreshProp,
}: MultichainTransactionsViewProps) => {
const { colors } = useTheme();
const style = styles();
Expand Down Expand Up @@ -128,13 +133,14 @@ const MultichainTransactionsView = ({

setRefreshing(true);
try {
onRefreshProp?.();
await updateIncomingTransactions();
} catch (error) {
console.warn('Error refreshing transactions:', error);
} finally {
setRefreshing(false);
}
}, [enableRefresh]);
}, [enableRefresh, onRefreshProp]);

const renderEmptyList = () => (
<View style={style.emptyContainer}>
Expand Down
Loading