Add infinite scroll and pull to refresh

[dewabisma]

Summary

Continuation of this PR #493 , closing that one because of broken git history.

---

Note

Medium Risk
Changes how transaction history loading and errors are surfaced (initial vs load-more), which affects core wallet activity UX; logic is localized but easy to regress on edge cases (empty history, offline refresh).

Overview
Adds pull-to-refresh and infinite scroll on the v2 Activity screen by wiring a ScrollController to fetchMore() and silentRefresh() via new activeAccountPaginationProvider / readActiveAccountPaginationNotifier helpers on the active-account filtered pagination controller.

Pagination behavior is refined: PaginationState gains isLoading and hasLoadedChainData, plus copyWith(clearError:) so errors can be cleared explicitly. Transaction list providers only emit AsyncValue.error when the initial chain load fails; load-more failures are logged but existing rows stay visible. Initial loading is keyed off isLoading instead of isFetching && empty.

UnifiedPaginationController updates loading flags through init/fetch/refresh paths and wraps silentRefresh in isFetching so concurrent fetches are guarded. Activity UI drops the send/receive/all filter chips (still fixed to TransactionFilter.all) and shows a bottom loader while loading more pages; empty state remains pull-to-refreshable.

Also adds unit tests for PaginationState.copyWith error semantics, a TransactionFilter l10n extension, quantusDebugPrint usage, and excludes build/** from analyzer.

^{Reviewed by Cursor Bugbot for commit f46603d. Bugbot is set up for automated code reviews on this repo. Configure here.}

[n13]

I've done a thorough review of PR #499. Here's my analysis.

PR #499 Review — Add infinite scroll and pull to refresh

Overall this is a clean, well-scoped UX improvement. The core mechanics (shared ScrollController, threshold-based load-more, AlwaysScrollableScrollPhysics so the empty state still pulls, filter-keyed list, and load-more footer) are all idiomatic Flutter. The new activeAccountPaginationProvider / activeAccountPaginationNotifier helpers are a nice DRY-ing of what previously had to be re-derived ad-hoc. A few things are worth tightening before merge.

Behavior issues

1. Pull-to-refresh wipes the list to skeletons (UX regression). loadingRefresh() does state = PaginationState.initial(); before re-fetching:

    final targetAccountIds = _getAccountIds();
    if (targetAccountIds.isEmpty) {
      state = PaginationState.initial().copyWith(hasMore: false);
      return;
    }

    // Reset to initial state to show loading
    state = PaginationState.initial();
    await _fetchPage(targetAccountIds);

After that, the combined provider returns AsyncValue.loading() (isFetching && otherTransfers.isEmpty), so txAsync.when(loading: …) swaps the whole list for skeletons — and tears down the RefreshIndicator subtree mid-pull. Users will see their list blink to skeletons and the pull spinner vanish.

Pick one of:

• Use silentRefresh() from _refresh (it preserves the existing list and just replaces it on success).
• Or change loadingRefresh to not zero out otherTransfers/scheduledReversibleTransfers — only reset offsets + isFetching and let _fetchPage overwrite on success. RefreshIndicator already shows the spinner; we don't need the skeletons in addition.

2. _onScroll can fire fetchMore when content fits the viewport (Bugbot's "premature fetch"). This is a real-but-mild issue:

  void _onScroll() {
    if (!_scrollController.hasClients) return;
    final pos = _scrollController.position;
    if (pos.pixels < pos.maxScrollExtent - _loadMoreThreshold) return;

    final pagination = ref.read(activeAccountPaginationProvider(_filterOption));
    if (pagination == null || pagination.isFetching || !pagination.hasMore) return;

    activeAccountPaginationNotifier(ref, _filterOption)?.fetchMore();
  }

When the list is shorter than the viewport, maxScrollExtent == 0, so pixels (0) >= -200 is always true. The listener doesn't fire on attach, but with AlwaysScrollableScrollPhysics any drag/overscroll triggers it. Add a pos.maxScrollExtent > 0 guard:

if (pos.maxScrollExtent <= 0) return;
if (pos.pixels < pos.maxScrollExtent - _loadMoreThreshold) return;

If you actually want auto-fill-to-viewport behavior (fetch more until the list fills the screen), do it explicitly in the controller after a fetch completes, not as a side-effect of touching the screen.

3. Bugbot's "filter change keeps scroll offset" — not actually a bug, but worth confirming. You added key: ValueKey(_filterOption) on the non-empty ListView.builder, which causes the previous ScrollPosition to detach and a new one to attach. Combined with PageStorage's default keepScrollOffset: true, switching All → Send → All can actually restore the previous All offset (per-filter scroll memory). If that's the intent — great. If you instead want every filter switch to start at the top, either set keepScrollOffset: false on the controller or jumpTo(0) in _onFilterOptionChanged after setState. The empty-state ListView doesn't have the key, but since it's a different widget instance that's fine.

API / semantics

4. copyWith now replaces error/stackTrace unconditionally.

      hasMore: hasMore ?? this.hasMore,
      isFetching: isFetching ?? this.isFetching,
      error: error,
      stackTrace: stackTrace,

This is the right fix for "can't clear the error", but it changes semantics for every call site: any copyWith that doesn't mention error now wipes the existing error. In this codebase that's mostly fine (and arguably good, since state.copyWith(isFetching: true) in _fetchPage now resets stale errors at fetch start). But it's a foot-gun for future callers, including updateReversibleTransferToExecuted and addTransactionToHistory, which silently drop errors. Consider the Optional<T>/sentinel pattern (a _kUnset const) so you can distinguish "not passed" from "explicitly null", or add a focused clearError() helper and keep ?? semantics. At minimum, add a doc comment on copyWith calling out the new behavior.

5. hasLoadedChainData should probably also gate the loading branch. In both providers:

      if (pagination.isFetching && pagination.otherTransfers.isEmpty) {
        return const AsyncValue.loading();
      }

You added hasLoadedChainData (which checks both otherTransfers and scheduledReversibleTransfers) for the error gate, but the loading gate still only checks otherTransfers.isEmpty. For an account that has only scheduled reversible transfers, the screen will flash skeletons on every refetch. Use !hasLoadedChainData here too for consistency.

Minor / style

6. Logging inconsistency. Providers now use quantusDebugPrint, but unified_pagination_controller.dart still has 13 raw print(...) calls (lines 45, 105, 111, 124, 136, 141, 175, 183, 191, 224, 230, 239, 253). The "properly debug sensitive information" intent of the PR isn't really achieved until the controller (which logs txIds, fetch failures, etc.) is also gated by kDebugMode. Recommend converting them all in this PR.

7. Naming: activeAccountPaginationNotifier looks like a provider but is a top-level function taking WidgetRef. Consider renaming to make it clearly a helper, e.g. readActiveAccountPaginationNotifier(ref, filter), or define it as a Provider.family like activeAccountPaginationProvider. The current name is easy to mistake for a Riverpod provider on a quick scan.

8. Duplicated RefreshIndicator shell. The empty-state branch and the data branch both wrap in RefreshIndicator(onRefresh: _refresh, color: …, backgroundColor: …, child: …). A tiny _buildRefreshable({required Widget child}) wrapper keeps it DRY (and matches the repo's DRY rule).

9. _filterLabel lives on the State. It's pure; can be a top-level function or moved into TransactionFilter (an extension on the enum) so other screens can reuse the localized labels.

10. analysis_options.yaml excluding build/** — fine and standard. No concerns.

Things I think are good

• Splitting activeAccountFilteredParams out as the single source of params construction — clean DRY win.
• LayoutBuilder + ConstrainedBox(minHeight: constraints.maxHeight) to make the empty state pull-to-refreshable without center-collapsing — nice touch.
• key: ValueKey(_filterOption) to give each filter its own scroll state.
• Distinguishing initial-load errors from load-more errors via hasLoadedChainData is the right model; the existing list shouldn't disappear because page 5 failed.
• ScrollController is properly disposed and the listener is removed first — no leaks.

Recommendation

Approve with the following changes requested:

• Fix pull-to-refresh skeleton flash (Rust Bindings #1) — high priority, user-visible.
• Add maxScrollExtent > 0 guard in _onScroll (Feature/dilithium signatures #2) — low effort, addresses Bugbot.
• Use !hasLoadedChainData in the loading gate for parity with the error gate (Refactor screens #5).
• Convert the remaining print calls in unified_pagination_controller.dart to quantusDebugPrint (New design #6).
• Either document or harden the new copyWith semantics (Fix warnings1 #4).

The rest (naming, DRY of RefreshIndicator, _filterLabel location) are nits and can land separately.

Want me to apply any of these as edits, or open follow-up comments on the PR? Per your rules, I won't post to GitHub or push without your go-ahead.

[dewabisma]

I don't know if it worth it fixing #1, #2. It works as intended anyway. I will suggest we ignore this AI suggestion.

[n13]

Re-reviewed against the two new commits (5f4d4fd fix: PR issues, ebfbacb feat: make error clear explicit).

Re-review

Addressed nicely

• copyWith semantics (Fix warnings1 #4) — explicit clearError: false flag, doc comment, and unit tests. Clean. The tests cover all four meaningful combinations.
• quantusDebugPrint everywhere (New design #6) — all 13 print(...) calls in unified_pagination_controller.dart converted.
• Renamed to readActiveAccountPaginationNotifier (Pull actual fee #7) — the read prefix matches Riverpod conventions and makes it obvious this isn't a provider.
• _buildRefreshableContent helper (Send progress should be cooler #8) — DRY win, both branches use it.
• TransactionFilterLabel extension (Get real transaction history from chain #9) — moved out of _ActivityScreenState to a reusable extension.

Still outstanding from the previous review

#1 Pull-to-refresh still flashes skeletons. loadingRefresh() is unchanged:

    final targetAccountIds = _getAccountIds();
    if (targetAccountIds.isEmpty) {
      state = PaginationState.initial().copyWith(hasMore: false);
      return;
    }

    // Reset to initial state to show loading
    state = PaginationState.initial();
    await _fetchPage(targetAccountIds);

So after the user pulls, otherTransfers is emptied, the combined provider returns AsyncValue.loading(), and the screen swaps to the skeleton ListView — tearing down the RefreshIndicator mid-pull. Either drop the state = PaginationState.initial(); (let _fetchPage overwrite on success) or have _refresh call silentRefresh() instead. RefreshIndicator already provides the spinner.

#2 _onScroll premature fetch. Still no maxScrollExtent > 0 guard:

  void _onScroll() {
    if (!_scrollController.hasClients) return;
    final pos = _scrollController.position;
    if (pos.pixels < pos.maxScrollExtent - _loadMoreThreshold) return;

When the loaded page doesn't fill the viewport (maxScrollExtent == 0), an overscroll drag fires _onScroll, the condition is true at pixels == 0, and fetchMore runs. One-line fix:

if (pos.maxScrollExtent <= 0) return;

#5 Loading gate inconsistent with error gate. Both transactions providers still gate loading on otherTransfers.isEmpty:

      if (pagination.isFetching && pagination.otherTransfers.isEmpty) {
        return const AsyncValue.loading();
      }

The error gate uses !hasLoadedChainData. For an account that has only scheduledReversibleTransfers, the screen still skeletons on every refetch. Use !pagination.hasLoadedChainData here for parity.

New issue introduced by 5f4d4fd

Filter-change no longer resets scroll. The commit removed key: ValueKey(_filterOption) from the ListView.builder and added ScrollController(keepScrollOffset: false). These don't compose the way it looks:

    _scrollController = ScrollController(keepScrollOffset: false)..addListener(_onScroll);

                    return _buildRefreshableContent(
                      child: ListView.builder(
                        controller: _scrollController,
                        physics: const AlwaysScrollableScrollPhysics(),
                        padding: EdgeInsets.zero,

keepScrollOffset: false only takes effect when the ScrollPosition is detached and a new one is attached. Without the per-filter key, switching filters reuses the same ListView.builder element — the same ScrollPosition stays attached — so the offset persists. Net effect: Bugbot's original "filter change keeps scroll offset" concern is now actually true again.

Pick one:

• Restore key: ValueKey(_filterOption) (it pairs correctly with keepScrollOffset: false to give a fresh ScrollPosition at 0 on each filter change).
• Or, drop both and add _scrollController.jumpTo(0) inside _onFilterOptionChanged after setState (guard with hasClients).

Minor / lint

• transaction_filter_extension.dart and the touched quantus_sdk/lib/src/models/transaction_filter.dart both end without a trailing newline (\ No newline at end of file in the diff). Easy lint fix.
• _buildRefreshableContent reads context.colors twice. Trivially cleaner to take it as a parameter or compute once, but not worth blocking on.

Verdict

Five of the six "should change" items from the previous round are resolved. Three things still need a touch:

1. Skeleton-flash on pull-to-refresh (still real).
2. maxScrollExtent <= 0 guard in _onScroll (one line).
3. Filter-change scroll reset: restore the key, or jumpTo(0) in _onFilterOptionChanged.

Plus the hasLoadedChainData loading-gate parity is a nice consistency follow-up.

Once those are in, this is good to go. Want me to draft the patches?

[cursor]

Overlapping fetchMore requests

Medium Severity

fetchMore only sets isFetching inside _fetchPage, so two calls can pass the guard at once and issue the same paged request. Offsets can advance twice while the UI dedupes by id, which can skip or reorder history pages.

Additional Locations (1)

• mobile-app/lib/v2/screens/activity/activity_screen.dart#L45-L55

 

^{Reviewed by Cursor Bugbot for commit f283500. Configure here.}

[cursor]

Silent refresh hides PTR errors

Medium Severity

When pull-to-refresh uses silentRefresh, failures in _silentFetchFirstPage are only logged; state keeps prior data and no error, so RefreshIndicator still succeeds and the user is not informed the refresh failed.

 

^{Reviewed by Cursor Bugbot for commit f283500. Configure here.}

[cursor]

Short list blocks load more

Medium Severity

The _onScroll method's check for maxScrollExtent <= 0 prevents fetchMore from being called. This means if the initial list of transactions fits within the viewport, subsequent pages of activity will not load, even when more data is available.

 

^{Reviewed by Cursor Bugbot for commit f283500. Configure here.}

[cursor]

Pull refresh skips while fetching

Medium Severity

_refresh returns immediately when pagination.isFetching is true, so pull-to-refresh completes without awaiting a refresh while load-more is in progress, even though the user expects new data.

 

^{Reviewed by Cursor Bugbot for commit f283500. Configure here.}

[cursor]

Activity PTR bypasses offline guard

Medium Severity

Activity pull-to-refresh calls silentRefresh, which does not check isOnlineProvider, unlike loadingRefresh used on Home and manual global refresh. Offline pulls still hit the network and can finish with unchanged data while the indicator succeeds.

 

^{Reviewed by Cursor Bugbot for commit f283500. Configure here.}

[cursor]

Unused filter label extension

Low Severity

The TransactionFilterLabel extension and its filterLabel method were introduced but became dead code, as the Activity filter chips that would have used them were removed.

 

^{Reviewed by Cursor Bugbot for commit f283500. Configure here.}

[dewabisma]

[n13]

Re-review (head f283500)

This pass is clean. The big simplification of dropping the filter UI removes a whole class of "filter change keeps scroll" worries, and the two outstanding fixes from the previous round are in.

Addressed

• Rust Bindings #1 Pull-to-refresh skeleton flash — _refresh now calls silentRefresh() instead of loadingRefresh(). The list stays mounted and only the RefreshIndicator spinner shows. ✓

    await readActiveAccountPaginationNotifier(ref, _filterOption)?.silentRefresh();

• Feature/dilithium signatures #2 _onScroll premature fetch — maxScrollExtent <= 0 guard added. ✓

    if (!_scrollController.hasClients) return;

    final pos = _scrollController.position;
    if (pos.maxScrollExtent <= 0 || pos.pixels < pos.maxScrollExtent - _loadMoreThreshold) return;

• Filter-change scroll reset — moot now that the filter UI is gone. The key: ValueKey(_filterOption) on the ListView.builder is functionally inert (filter is final and constant), but harmless. ✓
• Trailing newlines on the two extension files. ✓

One real concern: silentRefresh ↔ fetchMore race

This isn't introduced by this PR but is exposed by it now that silentRefresh is wired to a user gesture: silentRefresh / _silentFetchFirstPage never set isFetching = true, while fetchMore's guard is exactly state.isFetching || !state.hasMore. So:

1. User pulls → silentRefresh() starts a fetch from offset = 0.
2. Meanwhile, an end-of-list overscroll fires _onScroll, sees isFetching == false, and calls fetchMore() — which starts a fetch from the stale state.otherOffset (say 60).
3. Silent refresh returns first, replaces otherTransfers with the fresh page and resets otherOffset to 20.
4. The in-flight fetchMore returns and appends results requested from offset 60 to a list whose new offset is 20 → gap and/or duplicates, plus otherOffset ends up at 80 when only 40 rows are loaded.

  Future<void> _silentFetchFirstPage(List<String> targetAccountIds) async {
    try {
      final newTransactions = await ref
          .read(chainHistoryServiceProvider)
          .fetchAllTransactionTypes(accountIds: targetAccountIds, limit: _limit, filter: _filter);
      ...
      state = state.copyWith(
        otherTransfers: newOtherTransfers,
        scheduledReversibleTransfers: newScheduledReversibleTransfers,
        otherOffset: newTransactions.nextOtherOffset,
        scheduledOffset: newTransactions.nextScheduledOffset,
        hasMore: newTransactions.hasMore,
        clearError: true,
      );
    } catch (e, st) {
      quantusDebugPrint('Silent refresh failed: $e, $st');
    }
  }

Same race exists between two concurrent silentRefreshs (e.g., user pull + HistoryPollingManager tick).

Minimal fix: in silentRefresh, also flip isFetching for the duration:

Future<void> silentRefresh() async {
  if (state.isFetching) return;
  state = state.copyWith(isFetching: true);
  try {
    final targetAccountIds = _getAccountIds();
    if (targetAccountIds.isEmpty) return;
    await _silentFetchFirstPage(targetAccountIds);
  } finally {
    state = state.copyWith(isFetching: false);
  }
}

Care needed: isFetching && otherTransfers.isEmpty in the providers will then briefly flip the screen to skeleton on the very first silent refresh of an empty account, which defeats the point of this PR's pull-to-refresh fix. Two options:

• Gate that loading branch on !pagination.hasLoadedChainData (the Refactor screens #5 follow-up I mentioned previously) — this becomes table stakes once silentRefresh sets isFetching.
• Or use a separate isRefreshing flag instead of reusing isFetching, and have fetchMore check both.

Either is fine; my preference is the first because it also closes the still-open consistency gap.

Still outstanding from previous round

• Refactor screens #5 Loading-gate parity — both providers still use pagination.otherTransfers.isEmpty rather than !pagination.hasLoadedChainData. Cheap consistency fix and (per above) probably required if you take the race fix.

      if (pagination.isFetching && pagination.otherTransfers.isEmpty) {
        return const AsyncValue.loading();
      }

Minor / nits

• _filterOption is now a constant final of TransactionFilter.all. You can inline it at the call sites and drop the field entirely, the import of transaction_filter_extension.dart, and the now-meaningless ValueKey(_filterOption). Optional cleanup — current code works.
• ScrollController(keepScrollOffset: false) is no longer doing anything useful (single ListView, single position, no detach/reattach happening). Harmless.
• The activity screen no longer needs the Column/spacing block above the list; it's gone now and the layout is much simpler. Nice cleanup.

Verdict

Approve once the silentRefresh race is closed (and ideally take the !hasLoadedChainData follow-up at the same time). Everything else is optional polish.

Want me to draft the silentRefresh + loading-gate patch?

[n13]

some minor cleanups see above then GTG

[cursor]

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

There are 7 total unresolved issues (including 6 from previous reviews).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit a84cf86. Configure here.}