Add infinite scroll and pull to refresh#499
Conversation
- refactor DRY widget - delete redundant provider - fix bad number formatting locale
…into beast/language-support
- use intl from sdk - import legacy provider
Setting already cleared in substarte service logout
- implement infinite scroll - implement pull to refresh
…into beast/language-support
…etwork/quantus-apps into beast/integrate-infinite-scroll-and-refresh-to-activity-screen
|
I've done a thorough review of PR #499. Here's my analysis. PR #499 Review — Add infinite scroll and pull to refreshOverall this is a clean, well-scoped UX improvement. The core mechanics (shared Behavior issues1. Pull-to-refresh wipes the list to skeletons (UX regression). 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 Pick one of:
2. 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, 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 API / semantics4. 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 5. if (pagination.isFetching && pagination.otherTransfers.isEmpty) {
return const AsyncValue.loading();
}You added Minor / style6. Logging inconsistency. Providers now use 7. Naming: 8. Duplicated 9. 10. Things I think are good
RecommendationApprove with the following changes requested:
The rest (naming, DRY of 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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Re-reviewed against the two new commits ( Re-reviewAddressed nicely
Still outstanding from the previous review#1 Pull-to-refresh still flashes skeletons. 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, #2 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 ( if (pos.maxScrollExtent <= 0) return;#5 Loading gate inconsistent with error gate. Both transactions providers still gate loading on if (pagination.isFetching && pagination.otherTransfers.isEmpty) {
return const AsyncValue.loading();
}The error gate uses New issue introduced by
|
|
|
||
| Future<void> fetchMore() async { | ||
| print('UnifiedPaginationController: Fetch more'); | ||
| quantusDebugPrint('UnifiedPaginationController: Fetch more'); |
There was a problem hiding this comment.
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)
Reviewed by Cursor Bugbot for commit f283500. Configure here.
| clearError: true, | ||
| ); | ||
| } catch (e, st) { | ||
| print('Silent refresh failed: $e, $st'); |
There was a problem hiding this comment.
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.
| if (!_scrollController.hasClients) return; | ||
|
|
||
| final pos = _scrollController.position; | ||
| if (pos.maxScrollExtent <= 0 || pos.pixels < pos.maxScrollExtent - _loadMoreThreshold) return; |
There was a problem hiding this comment.
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.
| }; | ||
| Future<void> _refresh() async { | ||
| final pagination = ref.read(activeAccountPaginationProvider(_filterOption)); | ||
| if (pagination == null || pagination.isFetching) return; |
There was a problem hiding this comment.
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.
| final pagination = ref.read(activeAccountPaginationProvider(_filterOption)); | ||
| if (pagination == null || pagination.isFetching) return; | ||
|
|
||
| await readActiveAccountPaginationNotifier(ref, _filterOption)?.silentRefresh(); |
There was a problem hiding this comment.
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.
|
Re-review (head
|
|
some minor cleanups see above then GTG |
|
Here's my review of PR #499 (head SummaryThis is good to merge. Across the long review thread, all the high-priority issues have been addressed in the last batch of commits ( What's solid now
Future<void> silentRefresh() async {
quantusDebugPrint('UnifiedPaginationController: Silent refresh called');
if (state.isFetching) return;
final targetAccountIds = _getAccountIds();
if (targetAccountIds.isEmpty) return;
state = state.copyWith(isFetching: true);
try {
await _silentFetchFirstPage(targetAccountIds);
} finally {
state = state.copyWith(isFetching: false);
}
}
Remaining items — none blocking
Future<void> silentRefresh() async {
if (state.isFetching) return;
if (!ref.read(isOnlineProvider)) return;
...
}
Future<void> _refresh() async {
final pagination = ref.read(activeAccountPaginationProvider(_filterOption));
if (pagination == null || pagination.isFetching) return;
await readActiveAccountPaginationNotifier(ref, _filterOption)?.silentRefresh();
}
RecommendationApprove. Optional 10-LOC polish pass for items 1–3 before merge, leave 4–6 as follow-ups. Per your rules I won't comment on GitHub or push anything — want me to draft the polish patches locally (without touching your current checkout) so you can apply them when ready? |
n13
left a comment
n13
left a comment
There was a problem hiding this comment.
cleaned up the dead code
approved
* fix: prevent from creating account name using whitespace only * feat: remove redeem button
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 7 total unresolved issues (including 5 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 4e21cd4. Configure here.
| ), | ||
| error: (e, _) => Center( | ||
| child: Text(l10n.activityError(e.toString()), style: text.detail?.copyWith(color: colors.textError)), | ||
| ), |
There was a problem hiding this comment.
Error state lacks pull refresh
Medium Severity
When the first chain fetch fails, txAsync shows a centered error without RefreshIndicator, so pull-to-refresh cannot retry. Empty and populated lists use _buildRefreshableContent, but the error branch does not, blocking the main recovery path this PR adds.
Reviewed by Cursor Bugbot for commit 4e21cd4. Configure here.
| final pagination = ref.read(activeAccountPaginationProvider(_filterOption)); | ||
| if (pagination == null || pagination.isFetching || !pagination.hasMore) return; | ||
|
|
||
| readActiveAccountPaginationNotifier(ref, _filterOption)?.fetchMore(); |
There was a problem hiding this comment.
Scroll retriggers failed load more
Medium Severity
_onScroll calls fetchMore when near the bottom if hasMore and not isFetching, but ignores pagination.error. After a load-more failure, isFetching becomes false while hasMore often stays true, so staying at the bottom retriggers fetchMore on every scroll update and can spam the history API.
Reviewed by Cursor Bugbot for commit 4e21cd4. Configure here.
2 participants
Summary
Continuation of this PR #493 , closing that one because of broken git history.
Note
Medium Risk
Changes transaction list loading, error surfacing, and scroll/refresh behavior across shared pagination providers; regressions could affect empty states, load-more, or stale data after failed fetches.
Overview
Adds pull-to-refresh and infinite scroll on the v2 Activity screen, wired through new
activeAccountPaginationProvider/readActiveAccountPaginationNotifierhelpers so the UI can callsilentRefresh()andfetchMore()on the filtered pagination controller.Pagination gains
isLoading,hasLoadedChainData, andcopyWith(clearError: …)so providers distinguish first-load vs load-more failures and keep showing cached history when a later page fails.UnifiedPaginationControllerupdates loading/error flags accordingly and wrapssilentRefreshinisFetchingguards.Smaller UI tweaks: create/edit account buttons disable on empty trimmed names; activity filter chips are removed (fixed
TransactionFilter.all); mining redeem bottom bar is commented out pending implementation; analyzer excludesbuild/**. Unit tests coverPaginationState.copyWitherror semantics.Reviewed by Cursor Bugbot for commit 4e21cd4. Bugbot is set up for automated code reviews on this repo. Configure here.