Skip to content

fix(charts): distinguish offline from loading; make retry reachable [PERA-4581]#1013

Merged
fmsouza merged 10 commits into
mainfrom
fmsouza/pera-4581
Jul 24, 2026
Merged

fix(charts): distinguish offline from loading; make retry reachable [PERA-4581]#1013
fmsouza merged 10 commits into
mainfrom
fmsouza/pera-4581

Conversation

@fmsouza

@fmsouza fmsouza commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Description

Charts previously collapsed every failure mode into isPending ? spinner : isError ? error : …. A query paused offline reports isPending: true forever, so true offline meant an eternal spinner with an unreachable retry, and chart data was never persisted, so cold offline starts had nothing to show. This PR makes the chart stack paused-aware and cache-friendly:

  • New useBalanceLineChart hook (colocated with BalanceLineChart) implementing the epic's data-first state contract: chart (fresh or stale) → offline → error → loading → empty. Stale data always beats state screens; a paused query renders a localized offline state (common.offline_mode / common.offline_refresh_body — no new i18n strings) instead of the spinner.
  • Offline-guarded retry: the retry button reads useNetworkStatusStore.getState().hasInternet at press time and short-circuits while offline instead of dispatching a doomed 30 s request; an errored query on a known-offline device also collapses into the offline surface. Refetch auto-resumes on reconnect via the existing onlineManager wiring.
  • isPaused threaded from the raw query results through WealthChart, AssetPriceChart, and AssetWealthChart; PriceTrend logic extracted into a colocated usePriceTrend hook that hides the trend (instead of showing a fake 0.00%) when paused with no data.
  • Persistence allowlist: shouldDehydrateQuery extracted from QueryProvider into a testable apps/mobile/src/providers/query-persistence.ts; successful ['accounts','balance-history',…] and ['assets','prices','history',…] snapshots are now dehydrated (network-only, no SQLite table backs them, no PII) so last-known charts survive restarts within reactQueryPersistenceAge. New key predicates isAccountBalancesHistoryQuery / isAssetPriceHistoryQuery live in their packages; the per-account asset-history key is deliberately NOT allowlisted (ticket scope).
  • CHART_QUERY_TIMEOUT_MS kept at 30 s — decision documented in docs/OFFLINE_PAUSED_STATE.md: no production latency data to justify lowering it, the endpoints are documented as routinely exceeding ky's 10 s default, and the offline short-circuit removes the doomed-wait UX that motivated reconsidering it.
  • Online behavior unchanged: LineChart props, intervals, ranges, and pointer interactions untouched; chart queries stay pure-network (networkMode untouched, per the paused-state contract doc).

Related Issues

Checklist

  • Have you tested your changes locally? (pnpm pre-push green; full pnpm test green — 549 files / 3937 mobile tests; every change built TDD red→green)
  • Have you reviewed the code for any potential issues?
  • Have you documented any necessary changes in the project's documentation? (docs/OFFLINE_PAUSED_STATE.md: chart adoption of the paused-state contract, persistence allowlist rationale, timeout decision)
  • Have you added any necessary tests for your changes? (hook specs for useBalanceLineChart + usePriceTrend, component branch tests incl. paused/offline-retry, query-persistence eligibility spec, package-level key-predicate tests)
  • Have you updated any relevant dependencies? (N/A — no dependency changes)

Additional Notes

  • Design/QA glance suggested (offline-only visual nit): when PriceTrend hides offline, the AssetMarkets trend row's space-between layout left-aligns the lone date text.
  • AssetWealthChart got isPaused threading (same shared component, same eternal-spinner bug) but its query key is intentionally outside the persistence allowlist, matching the ticket's explicit scope.

🤖 Generated with Claude Code

@fmsouza
fmsouza requested a review from a team as a code owner July 23, 2026 11:40

@yasin-ce yasin-ce left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🔴 Blocking

  • common.offline_refresh_body no longer exists on main — the offline body will render the raw key after rebase. (BalanceLineChart.tsx:96)

    The PR branch is 3 commits behind main and its en.json still carries this key via an inherited pre-revert merge — but #1014 (75f5fe049, the PERA-4586 revert) removed it from main, and no commit unique to this PR re-adds it. So on the branch it works, but once you bring main in (which you must to merge), t('common.offline_refresh_body') falls through to the literal string and the user sees common.offline_refresh_body as the offline body.

    The PR description's "reuses existing common.offline_mode / common.offline_refresh_body" is only true against the stale base — on current main, offline_mode survives (it predates #986) but offline_refresh_body does not.

    Fix: add common.offline_refresh_body to en.json in this PR. (Rebase first — the tests import real i18n, so a rebased branch would surface this immediately.)

🟡 Should-fix

Nothing blocking-adjacent. The one real item is above.

⚪ Nits

  • period ?? 'one-week' in usePriceTrend.ts:749 is dead — UsePriceTrendParams.period is the required HistoryPeriod, so the fallback can't fire. It's faithfully carried over from the old inline code, so not a regression; either drop the ?? 'one-week' or make the param period?: if callers can actually pass undefined.
  • A couple of the intent comments run long (the renderState and allowlist blocks). They explain real gotchas so I'd keep them, but we've been trimming comment verbosity lately — could tighten.

🔵 Questions / awareness

  • Retry button in the offline state is visible but no-ops while offline (useBalanceLineChart.ts:412-419). The guard is sound (skip the doomed 30s request), but should we show a button that silently does nothing when tapped, given the copy already promises an auto-refresh on reconnect? Not blocking — the same button works the instant connectivity returns — just worth a UX confirm.
  • usePriceTrend's isHidden = isPaused && !chartData treats an empty array as "has data" (![] is false), so a paused query resolved with [] would render a 0.00% trend rather than hiding. Edge-y (a paused query rarely carries data: []) — flag for awareness, not a change.
  • The shouldDehydrateQuery allowlist correctly runs ahead of the exclusion block (a balance-history key also matches isAccountQuery, so order matters) — nicely done and tested. Confirm the two predicates stay prefix-only ([0]/[1], and [2] for price history) if the query-key shapes ever change; the negative tests guard exactly this.

Comment on lines +128 to +155
) : renderState === 'offline' ? (
// Offline must not masquerade as loading: a paused query
// reports isPending forever, so without this branch the
// spinner never yields and retry is unreachable (PERA-4581).
<EmptyView
title={t('common.offline_mode')}
body={t('common.offline_refresh_body')}
button={retryButton}
/>
) : renderState === 'error' ? (
// A failed request must not masquerade as "no history" — show a
// distinct error state so the user can retry rather than assume
// there's nothing to display.
<EmptyView
title={t('common.error.title')}
body={errorBody ?? t('common.error.body')}
button={retryButton}
/>
) : renderState === 'loading' ? (
<LoadingView
variant='circle'
size='lg'
/>
) : (
<EmptyView
title=''
body={emptyBody}
/>

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.

nit: this nested ternary thing is super hard to read - might be worth a renderEmptyView() function or something to do it with some slightly saner branching or switches

fmsouza added a commit that referenced this pull request Jul 24, 2026
…ne retry [PERA-4580]

Align the Transaction Details screen with the PERA-4581 charts contract
(PR #1013): a device that goes offline carrying a prior non-offline error
(e.g. a timeout) now shows the offline surface via
`isPaused || (isError && !hasInternet)` instead of a timeout screen with a
Retry that just re-pauses. The retry also short-circuits while offline so it
no longer fires a doomed request, matching the charts retry.

Addresses review feedback on #1008.
fmsouza added 10 commits July 24, 2026 13:29
- Re-add common.offline_refresh_body to en.json (removed from main by
  #1014's revert; the branch inherited it from a stale base, so it would
  fall through to the raw key after rebase)
- Drop dead 'period ?? one-week' fallback in usePriceTrend (period is a
  required HistoryPeriod)
- Hide the price trend when paused with an empty series, not just a
  missing one, matching the hook's documented contract
- Replace the nested ternary in BalanceLineChart with a switch on
  renderState
Pressing the chart retry button while offline previously did nothing —
a button that silently no-ops is confusing. Instead, open the standard
informational bottom sheet (ConfirmActionContent) explaining that the
device is offline and the chart will refresh on reconnect, reusing the
existing common.offline_mode / common.offline_refresh_body / common.ok
strings (no new i18n). Online retry is unchanged.

Renames useBalanceLineChart.ts -> .tsx since the hook now builds the
sheet's content element, matching useSettingsScreen/useInfoButton.
@fmsouza
fmsouza force-pushed the fmsouza/pera-4581 branch from fd46f8c to 17f74a6 Compare July 24, 2026 12:49
@fmsouza
fmsouza merged commit cb4da31 into main Jul 24, 2026
9 checks passed
@fmsouza
fmsouza deleted the fmsouza/pera-4581 branch July 24, 2026 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants