Polling Bug

[dewabisma]

Summary

Fixed the bug of polling visited accounts even though not active.

---

Note

Medium Risk
Changes how often history, balances, and pending/reversible state refresh run; incorrect scoping could leave inactive-account pending txs stale, though reconciliation still includes senders/receivers.

Overview
Fixes background polling and refresh fan-out so work targets the active wallet account (and transaction parties where needed), not every account the user has opened.

A new polling_refresh_scope module centralizes scoped balance invalidation, filtered pagination refresh, account-switch handling, and reconciliation account selection. Global history polling now silently refreshes only the active account; pending-tx reconciliation reads per-account filtered history and skips inactive accounts unless their pagination provider already exists. Reversible-transfer monitoring subscribes to the active account’s transaction list instead of wallet-wide history, and event-driven refreshes use deduplicated per-account targets instead of refreshing all accounts.

Across the mobile app, raw print calls are replaced with quantusDebugPrint for consistent debug logging. Unit tests cover refresh-target and reconciliation account scoping.

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

[n13]

I have enough context. Here's my review:

PR #502 Review: Polling Bug Fix

The refactor is a solid direction — narrowing polling to the active account avoids the wasteful fan-out across every visited wallet, and the new polling_refresh_scope.dart cleanly centralizes the scoping logic. The unit tests on accountRefreshTargets / reconciliationAccountIds are a nice touch. That said, there are a few real issues that should be addressed before merging.

Critical: Double-scheduled timer on activeId == null

Cursor Bugbot's finding is correct, and now actually reachable.

    try {
      final activeId = activeAccountId(_ref);
      if (activeId == null) {
        _scheduleNextPoll();   // ← inner schedule
        return;
      }
      ...
    } catch (e) {
      ...
    } finally {
      if (_isPolling) {
        _scheduleNextPoll();   // ← finally also schedules → orphans the first timer
      }
    }

The finally block runs on return, so when activeId == null you assign _pollingTimer twice; the first Timer is lost but still fires, effectively doubling poll cadence each time it triggers. The same shape existed before with the accountsState empty check, but that branch was guarded by the accountsProvider listener that calls stopPolling(). The new activeId == null branch is much more reachable (account switching, transient activeAccountProvider loading), so what was latent is now an actual bug.

Fix: drop the inner call; the finally already handles it.

if (activeId == null) return;

Concern: receive notifications won't fire for non-active accounts

notification_integration_service.dart listens to the wallet-wide paginationControllerProvider to detect newly received transfers and fire _notifyTokenReceived:

    _ref.listen(paginationControllerProvider, (previous, next) {
      if (previous != null && previous.otherTransfers.isNotEmpty) {
        ...
        final newReceiveTxs = newTxs.where((tx) {
          if (tx is! TransferEvent) return false;
          return accountIds.contains(tx.to) && !accountIds.contains(tx.from);
        });

        for (final receiveTokenTx in newReceiveTxs) {
          _notifyTokenReceived(receiveTokenTx as TransferEvent);
        }
      }
    });

The PR removes every paginationControllerProvider.silentRefresh() call from polling, so this global controller will no longer be ticked by the timer. That means an incoming transfer to an account that isn't currently selected won't drive this listener — i.e. no "you received X" notification — until the user switches to that account. Worth confirming whether that's acceptable, or whether paginationControllerProvider still needs a periodic silent refresh (or whether the notification listener should move to a different signal).

Concern: reconciliation can silently skip the active account

In the reconciliation rewrite:

for (final accountId in accountIds) {
  await refreshAccountsPagination(
    _ref,
    accountIds: [accountId],
    action: (notifier) => notifier.silentRefresh(),
    onlyIfAlive: accountId == activeId,
  );
}
final confirmedTransactions = _loadConfirmedTransactions(txService, accountIds);

…and:

final pagination = _ref.read(filteredPaginationControllerProviderFamily(params));
if (!pagination.hasLoadedChainData) continue;

Two things compound:

1. For the active account we use onlyIfAlive: true. If the user is on a screen that doesn't currently keep that filtered pagination provider alive (e.g., settings, send flow), the refresh is skipped — and then _loadConfirmedTransactions reads the freshly instantiated provider, sees hasLoadedChainData == false, and silently skips it. So the active-account pending tx won't be reconciled until that screen is reopened. The previous code used allTransactionsProvider, which is referenced by other consumers and was reliably kept up to date.
2. hasLoadedChainData is otherTransfers.isNotEmpty || scheduledReversibleTransfers.isNotEmpty — for a brand-new wallet account with no history, it's permanently false. Not a real regression (nothing to match anyway), just be aware the per-account skip can mask "no data yet".

Suggested fix: don't gate the active account on onlyIfAlive, or perform a forced silentRefresh on the active account regardless before calling _loadConfirmedTransactions.

Minor: triggerSilentHistoryRefresh no longer adds the new tx to the global pagination controller

Old code did mainController.addTransactionToHistory(newTransaction); mainController.silentRefresh(); against the wallet-wide controller. The new code only updates per-account filtered controllers. Combined with the notification-service point above, anything still reading paginationControllerProvider directly will be stale until something else refreshes it.

Minor: DRY between refreshAccountsPagination and updatePaginationFiltersFor

Both iterate over filters and invoke a callback per (accountIds, filter) notifier. The new helper adds onlyIfAlive and async support; the old one is still sync with Reader. The filters param added to updatePaginationFiltersFor doesn't appear to have any callers passing it. Consider either consolidating into a single API or dropping the unused filters param to keep this from drifting.

Nits

• print('Performing global history poll for active account...') — most of the controller code already uses quantusDebugPrint. Mixed-mode logging is preexisting, but worth noting.
• refreshAccountsPagination is missing a doc comment while its peers have them.
• In the activeAccountProvider listener you skip when previousId == null, which means the first account assignment after app launch never triggers refreshActiveAccountOnSwitch. Likely intentional (handled by the initial bring-up), but worth a short comment explaining why.

Positives

• Active-account-only scoping is a meaningful efficiency win.
• Awaiting reconcilePendingTransactions() and the manual refresh now produces sane ordering.
• ProviderSubscription lifecycle in ReversibleTransferMonitoringService is properly closed on lifecycle changes — good resource hygiene.
• accountRefreshTargets correctly dedupes active vs affected.
• Switching the reversible monitor to listen to activeAccountPaginationProvider(TransactionFilter.all) is a clean fix.

Recommendation

Request changes — primarily for the timer double-scheduling bug (small fix) and the reconciliation/notification regressions (need a decision on whether paginationControllerProvider still needs to be ticked, or whether the notification listener should move).

PR link: Polling Bug #502

[dewabisma]

Concern: receive notifications won't fire for non-active accounts

We already have remote notification. So this is intentional to not send notification for non active account.

Concern: reconciliation can silently skip the active account

This is nonsense. We don't skip. We don't do recon if there is nothing to recon.

[n13]

I've reviewed the new commits and the additional Bugbot/author comments. Here's my updated review:

PR #502 — Re-review (after a9ba276 and 62e6719)

Resolved since last review

• Double-scheduled timer: fixed via a defensive _pollingTimer?.cancel() inside _scheduleNextPoll() — different approach than I suggested, but functionally correct. ✅
  • Minor: this makes the _scheduleNextPoll(); return; in the activeId == null branch redundant (the finally will reschedule anyway). It's harmless now, but if (activeId == null) return; would be cleaner.
• One print → quantusDebugPrint migration landed. (Most of the file still uses print, but the trend is right.)

Author response triage

• "Remote notifications cover non-active accounts" — Fair, accepting this. The in-app paginationControllerProvider listener regression is intentional. This concern is closed on my end.
• "We don't skip — we don't recon if there's nothing to recon" — Respectfully, this isn't quite right, and the Bugbot finding below explains why. The cache being empty isn't because there's nothing to reconcile; it's because we never fetched it. The interaction between the inverted onlyIfAlive and the hasLoadedChainData skip means the active account's pending txs can be left unreconciled until the user opens the activity screen. See below.

New Bugbot findings — all valid

1. Inverted onlyIfAlive in reconciliation (Medium) — agree

      for (final accountId in accountIds) {
        await refreshAccountsPagination(
          _ref,
          accountIds: [accountId],
          action: (notifier) => notifier.silentRefresh(),
          onlyIfAlive: accountId == activeId,
        );
      }

Recall the helper:

Future<void> refreshAccountsPagination(
  Ref ref, {
  required List<String> accountIds,
  required Future<void> Function(UnifiedPaginationController notifier) action,
  Iterable<TransactionFilter> filters = _backgroundPollFilters,
  bool onlyIfAlive = false,
}) async {
  if (accountIds.isEmpty) return;

  final cachedIds = AccountIdListCache.get(accountIds);

  for (final filter in filters) {
    final params = FilteredTransactionsParams(accountIds: cachedIds, filter: filter);
    if (onlyIfAlive && !ref.exists(filteredPaginationControllerProviderFamily(params))) {
      continue;
    }
    ...
  }
}

The current call gives the opposite of the PR's stated goal:

• Active account → onlyIfAlive: true → skipped if its provider isn't alive (e.g., user is on settings/send/scan screens during the 1-minute poll tick).
• Non-active counterparties → onlyIfAlive: false → unconditionally instantiates their providers, which is exactly the fan-out this PR is trying to eliminate.

This also amplifies the "active account skip" concern: after the inverted call skips the active account, _loadConfirmedTransactions reads its provider, sees !hasLoadedChainData, and continues — so the active account's stale pending txs never get matched.

Fix: onlyIfAlive: accountId != activeId.

It's also worth re-thinking whether the active account's data should be loaded unconditionally before reconciliation runs (since the global poll may now be the only thing keeping it warm), e.g., by performing a non-onlyIfAlive refresh on the active account explicitly before the loop.

2. _txSubscription not closed in dispose() (Medium) — agree

Confirmed by reading the current file:

  void dispose() {
    for (final timer in _timers.values) {
      timer.cancel();
    }
    for (final poller in _executionPollers.values) {
      poller.cancel();
    }
    _timers.clear();
    _executionPollers.clear();
  }

Coupled with:

final reversibleTransferMonitoringServiceProvider = Provider<ReversibleTransferMonitoringService>((ref) {
  final service = ReversibleTransferMonitoringService(ref);

  ref.onDispose(service.dispose);

  return service;
});

The lifecycle handler closes _txSubscription correctly when the app backgrounds, but the ref.onDispose(service.dispose) path leaks it. Fix: add _txSubscription?.close(); _txSubscription = null; to dispose(). (Bonus: the explicit _txSubscription close in the lifecycle handler can then collapse into just dispose().)

3. Silent refresh duplicated in triggerSilentRefresh (Low) — agree

Future<void> triggerSilentRefresh() async {
  print('History polling manager: Silent Refresh!');
  _refreshBalance(showLoading: false);
  await silentRefreshActiveAccount(_ref);                       // 1st
  await _reversibleMonitor.forceCheckAllMonitoredTransfers();   // calls silentRefreshActiveAccount internally if pollers exist → 2nd
}

Minor, but easy to fix. Two reasonable options:

• Drop the call from forceCheckAllMonitoredTransfers() (callers that need a refresh + monitor check already do them in this order), or
• Drop the direct call here and let forceCheckAllMonitoredTransfers() always refresh — but its current contract is "only if pollers exist", so option 1 is cleaner.

Still outstanding from my prior review

• refreshAccountsPagination is still missing a doc comment.
• The filters parameter added to updatePaginationFiltersFor still has no callers passing a non-default value — consider dropping it or migrating callers.
• The previousId == null early-return in the new activeAccountProvider listener silently skips the first assignment after launch; if that's intentional, a one-line comment would help future readers.
• Mixed print / quantusDebugPrint is now even more inconsistent in global_history_polling_service.dart after the partial migration. Cheap to finish in this PR while the file is hot.

Recommendation

Still request changes, primarily for:

1. Inverted onlyIfAlive (functional regression for active-account reconciliation + still leaks fan-out to non-active accounts).
2. Subscription leak in dispose().

The double-schedule fix and the author's note on remote notifications are good. Once #1 and #2 are addressed and the low-severity duplicate is cleaned up, this should be in good shape to merge.

PR link: Polling Bug #502

[n13]

I have the latest state. Let me provide the updated review.

---

PR #502 — Third Review (after commits through 487404ea)

Current PR head: 487404ea ("feat: remove duplicate refresh")

What the author has addressed since the last review

Issue	Status	Notes
Double-scheduled timer on activeId == null	✅ Fixed	Defensive _pollingTimer?.cancel() inside _scheduleNextPoll(). Different approach than suggested but effective.
Inverted onlyIfAlive in reconciliation	✅ Fixed	Renamed parameter to isAccountInactive, logic flipped to accountId != activeId. Active account now always refreshed; inactive accounts only if provider already alive.
_txSubscription leak in dispose()	✅ Fixed	Subscription now closed + nulled in dispose().
Duplicate silentRefreshActiveAccount call in silent refresh path	✅ Fixed	HistoryPollingManager.triggerSilentRefresh() no longer calls _reversibleMonitor.forceCheckAllMonitoredTransfers(). The monitor's forceCheck... still does a conditional refresh when pollers exist, but the double-tap in the common path is gone.
Debug print hygiene	Partial	One print → quantusDebugPrint in global poller; file still has mixed usage.

Remaining open finding (from Bugbot on 487404ea)

Low severity: triggerManualRefresh uses silentRefresh() instead of loadingRefresh()

Current code in GlobalHistoryPollingService.triggerManualRefresh():

await refreshAccountsPagination(
  _ref,
  accountIds: [active.account.accountId],
  action: (notifier) => notifier.silentRefresh(),  // was loadingRefresh
);

The method is documented as "Manually trigger a history refresh (useful for pull-to-refresh)". The previous behavior (and the old code the diff removes) used loadingRefresh() which shows loading indicators.

However, this method is not actually called from the real pull-to-refresh gestures:

• Home screen _refresh directly calls loadingRefresh() on the filtered controller.
• Activity screen _refresh directly calls silentRefresh() on the notifier.

HistoryPollingManager.triggerManualRefresh() (which delegates to the global poller) even has a comment: // This is not called from anywhere!

So the user-visible behavior of pull-to-refresh is unchanged — the screens bypass these service methods entirely. The finding is a documentation/implementation mismatch on a low-traffic code path, not a regression in the shipping UX.

Recommendation on this one: Either:

• Update the implementation to call loadingRefresh() (preserving old documented behavior for any future callers), or
• Update the doc comment to clarify this is a "background-triggered manual refresh" that intentionally stays silent.

Either is fine; it's a low-priority cleanup.

Other nits still outstanding (unchanged from prior review)

• refreshAccountsPagination still lacks a doc comment (the new helper at the heart of the scoping logic).
• The filters parameter added to updatePaginationFiltersFor still has no non-default callers — either migrate callers or drop the now-dead param.
• The activeAccountProvider listener still silently skips the first assignment after launch (previousId == null). A one-line comment would help.
• Mixed print/quantusDebugPrint remains in the touched files.

Positives (reiterated)

• Active-account scoping is now consistently applied across polling, reconciliation, reversible monitoring, and event-driven refreshes.
• Timer safety, subscription hygiene, and duplicate refresh elimination are all solid fixes.
• The parameter rename (isAccountInactive) makes the intent much clearer than the original onlyIfAlive boolean.
• Unit tests landed for the scoping helpers.

Overall assessment

This PR has meaningfully improved since the initial review. The three medium-severity Bugbot findings from the previous round are all resolved. The remaining open item is a low-severity documentation-vs-implementation nit on a method that isn't even wired to the actual pull-to-refresh UX.

Verdict: LGTM with optional low-priority cleanup. If the author addresses the triggerManualRefresh doc/impl mismatch (or just clarifies it), this is ready to merge. The core correctness and resource-leak issues that blocked earlier are gone.

PR: #502

[n13]

GTG!

the AI is kinda confused about the UX for refreshing, lets see how it looks later ;)

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

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

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

[cursor]

Reconciliation creates providers that refresh intentionally skipped

Medium Severity

_loadConfirmedTransactions calls _ref.read(filteredPaginationControllerProviderFamily(params)) for every account ID (including external addresses from tx.from/tx.to), which creates providers that refreshAccountsPagination intentionally skipped via ref.exists(...). The refresh step avoids instantiating controllers for inactive/external accounts, but _loadConfirmedTransactions then instantiates them anyway by calling _ref.read(...). Adding a ref.exists(...) guard before reading would maintain consistency with the optimization.

Additional Locations (1)

• mobile-app/lib/services/pending_transaction_reconciliation_service.dart#L51-L59

 

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

[cursor]

Manual refresh defaults to background-only filter scope

Low Severity

triggerManualRefresh calls refreshAccountsPagination without specifying the filters parameter, so it defaults to _backgroundPollFilters which is only [TransactionFilter.all]. A manual refresh conceptually covers all filter views (all, send, receive), matching what the old code did via updatePaginationFiltersFor iterating TransactionFilter.values. If this method is ever invoked for a user-initiated refresh, only the "all" tab updates.

 

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