Skip to content

msglist: Fix the double-fetch glitch #2162

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

Open
sm-sayedi wants to merge 4 commits into
base: main
Choose a base branch
from
Open

msglist: Fix the double-fetch glitch #2162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39dba13
msglist [nfc]: Point the TODO about the double-fetch glitch to an issue
sm-sayedi Jan 6, 2026
e9655b8
msglist test: Reproduce the double-fetch glitch
sm-sayedi Jan 6, 2026
da816dc
msglist [nfc]: Extract the "fetch more messages" code in a new method
sm-sayedi Jan 27, 2026
acc3046
msglist: Fix the double-fetch glitch
sm-sayedi Jan 27, 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
31 changes: 20 additions & 11 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,15 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
?? GlobalStoreWidget.settingsOf(context).markReadOnScrollForNarrow(widget.narrow);
}

void _fetchMoreIfNeeded(ScrollMetrics scrollMetrics) {
if (scrollMetrics.extentBefore < kFetchMessagesBufferPixels) {
model.fetchOlder();
}
if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) {
model.fetchNewer();
}
}

void _handleScrollMetrics(ScrollMetrics scrollMetrics) {
if (_effectiveMarkReadOnScroll()) {
_markReadFromScroll();
Expand All @@ -1038,17 +1047,17 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
_scrollToBottomVisible.value = true;
}

if (scrollMetrics.extentBefore < kFetchMessagesBufferPixels) {
// TODO: This ends up firing a second time shortly after we fetch a batch.
// The result is that each time we decide to fetch a batch, we end up
// fetching two batches in quick succession. This is basically harmless
// but makes things a bit more complicated to reason about.
// The cause seems to be that this gets called again with maxScrollExtent
// still not yet updated to account for the newly-added messages.
model.fetchOlder();
}
if (scrollMetrics.extentAfter < kFetchMessagesBufferPixels) {
model.fetchNewer();
if (SchedulerBinding.instance.schedulerPhase == .transientCallbacks) {
SchedulerBinding.instance.addPostFrameCallback((_) {
if (scrollController.hasClients) {
// From the `transientCallbacks` phase to `postFrameCallbacks` phase,
// `scrollMetrics` can become stale; so we use the fresh value
// from `scrollController`.
_fetchMoreIfNeeded(scrollController.position);
}
});
} else {
_fetchMoreIfNeeded(scrollMetrics);
}
}

Expand Down
33 changes: 16 additions & 17 deletions test/widgets/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,35 +841,34 @@ void main() {
check(itemCount(tester)).equals(401);
});

testWidgets('observe double-fetch glitch', (tester) async {
testWidgets('no double-fetch glitch', (tester) async {
// Regression test for: https://github.com/zulip/zulip-flutter/issues/2104
await setupMessageListPage(tester, foundOldest: false,
messages: List.generate(100, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser)));
check(itemCount(tester)).equals(101);
messages: List.generate(300, (i) => eg.streamMessage(id: 950 + i, sender: eg.selfUser)));
connection.takeRequests();
check(itemCount(tester)).equals(301);

// Fling-scroll upward...
await tester.fling(find.byType(MessageListPage), const Offset(0, 300), 8000);
await tester.pump();

// ... and we fetch more messages as we go.
connection.prepare(json: eg.olderGetMessagesResult(anchor: 950, foundOldest: false,
messages: List.generate(100, (i) => eg.streamMessage(id: 850 + i, sender: eg.selfUser))).toJson());
connection.prepare(delay: Duration(milliseconds: 1),
json: eg.olderGetMessagesResult(anchor: 950, foundOldest: false,
messages: List.generate(100, (i) => eg.streamMessage(id: 850 + i, sender: eg.selfUser))).toJson());
for (int i = 0; i < 30; i++) {
// Find the point in the fling where the fetch starts.
await tester.pump(const Duration(milliseconds: 100));
if (itemCount(tester)! > 101) break; // The loading indicator appeared.
if (connection.takeRequests().isNotEmpty) break;
}
await tester.pump(Duration.zero); // Allow a frame for the response to arrive.
check(itemCount(tester)).equals(201);

// On the next frame, we promptly fetch *another* batch.
// This is a glitch and it'd be nicer if we didn't.
connection.prepare(json: eg.olderGetMessagesResult(anchor: 850, foundOldest: false,
messages: List.generate(100, (i) => eg.streamMessage(id: 750 + i, sender: eg.selfUser))).toJson());
await tester.pump(const Duration(milliseconds: 1));
await tester.pump(Duration.zero);
check(itemCount(tester)).equals(301);
}, skip: true); // TODO this still reproduces manually, still needs debugging,
// but has become harder to reproduce in a test.
// Allow a delayed frame for the response to arrive.
await tester.pump(Duration(milliseconds: 1));
check(itemCount(tester)).equals(401);
await tester.pumpAndSettle();
// Check there is no additional request made to cause double-fetch glitch.
check(connection.takeRequests()).isEmpty();
});

testWidgets("avoid getting distracted by nested viewports' metrics", (tester) async {
// Regression test for: https://github.com/zulip/zulip-flutter/issues/507
Expand Down