Skip to content

msglist: Fetch more messages despite previous muted batch #1989

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 8 commits into
base: main
Choose a base branch
from
Open

msglist: Fetch more messages despite previous muted batch #1989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6dce47b
msglist [nfc]: Point the TODO about the double-fetch glitch to an issue
sm-sayedi Jan 6, 2026
9b5ea9a
msglist test: Reproduce the double-fetch glitch
sm-sayedi Jan 6, 2026
ca30293
msglist [nfc]: Extract the "fetch more messages" code in a new method
sm-sayedi Jan 27, 2026
8007e46
msglist: Fix the double-fetch glitch
sm-sayedi Jan 27, 2026
bbac187
msglist [nfc]: Mention fetchNewer in the MessageListView dartdoc
sm-sayedi Nov 8, 2025
a92bb30
msglist test [nfc]: Move a few helpers up to the top, for reusability
sm-sayedi Jan 8, 2026
a3065ec
msglist: Fetch more messages despite previous muted batch
sm-sayedi Nov 13, 2025
7145e19
msglist: Autofocus compose box only when the whole history is empty
sm-sayedi Feb 10, 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
45 changes: 40 additions & 5 deletions lib/model/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,32 @@ mixin _MessageSequence {
/// The corresponding item index is [middleItem].
int middleMessage = 0;

/// The ID of the oldest message fetched so far in this narrow.
///
/// This is used as the anchor for fetching the next batch of older messages
/// and will be `null` if no messages of this narrow have been fetched yet.
///
/// A message with this ID might not appear in [messages]:
/// - The message may be in a muted conversation.
/// - The message may have been moved or deleted after it was fetched.
///
/// See also [newestFetchedMessageId].
int? get oldestFetchedMessageId => _oldestFetchedMessageId;
int? _oldestFetchedMessageId;

/// The ID of the newest message fetched so far in this narrow.
///
/// This is used as the anchor for fetching the next batch of newer messages
/// and will be `null` if no messages of this narrow have been fetched yet.
///
/// A message with this ID might not appear in [messages]:
/// - The message may be in a muted conversation.
/// - The message may have been moved or deleted after it was fetched.
///
/// See also [oldestFetchedMessageId].
int? get newestFetchedMessageId => _newestFetchedMessageId;
int? _newestFetchedMessageId;

/// Whether [messages] and [items] represent the results of a fetch.
///
/// This allows the UI to distinguish "still working on fetching messages"
Expand Down Expand Up @@ -405,6 +431,8 @@ mixin _MessageSequence {
generation += 1;
messages.clear();
middleMessage = 0;
_oldestFetchedMessageId = null;
_newestFetchedMessageId = null;
outboxMessages.clear();
_haveOldest = false;
_haveNewest = false;
Expand Down Expand Up @@ -593,7 +621,7 @@ bool _sameDay(DateTime date1, DateTime date2) {
/// * Add listeners with [addListener].
/// * Fetch messages with [fetchInitial]. When the fetch completes, this object
/// will notify its listeners (as it will any other time the data changes.)
/// * Fetch more messages as needed with [fetchOlder].
/// * Fetch more messages as needed with [fetchOlder] and [fetchNewer].
/// * On reassemble, call [reassemble].
/// * When the object will no longer be used, call [dispose] to free
/// resources on the [PerAccountStore].
Expand Down Expand Up @@ -814,6 +842,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
Future<void> fetchInitial() async {
assert(!fetched && !haveOldest && !haveNewest && !busyFetchingMore);
assert(messages.isEmpty && contents.isEmpty);
assert(oldestFetchedMessageId == null && newestFetchedMessageId == null);

if (narrow case KeywordSearchNarrow(keyword: '')) {
// The server would reject an empty keyword search; skip the request.
Expand All @@ -828,6 +857,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
_setStatus(FetchingStatus.fetchInitial, was: FetchingStatus.unstarted);
// TODO schedule all this in another isolate
final generation = this.generation;
// TODO(#2085): handle request failure
final result = await getMessages(store.connection,
narrow: narrow.apiEncode(),
anchor: anchor,
Expand All @@ -837,6 +867,9 @@ class MessageListView with ChangeNotifier, _MessageSequence {
);
if (this.generation > generation) return;

_oldestFetchedMessageId = result.messages.firstOrNull?.id;
_newestFetchedMessageId = result.messages.lastOrNull?.id;

_adjustNarrowForTopicPermalink(result.messages.firstOrNull);

store.reconcileMessages(result.messages);
Expand Down Expand Up @@ -911,12 +944,13 @@ class MessageListView with ChangeNotifier, _MessageSequence {
if (haveOldest) return;
if (busyFetchingMore) return;
assert(fetched);
assert(messages.isNotEmpty);
assert(oldestFetchedMessageId != null);
await _fetchMore(
anchor: NumericAnchor(messages[0].id),
anchor: NumericAnchor(oldestFetchedMessageId!),
numBefore: kMessageListFetchBatchSize,
numAfter: 0,
processResult: (result) {
_oldestFetchedMessageId = result.messages.firstOrNull?.id ?? oldestFetchedMessageId;
store.reconcileMessages(result.messages);
store.recentSenders.handleMessages(result.messages); // TODO(#824)

Expand All @@ -941,12 +975,13 @@ class MessageListView with ChangeNotifier, _MessageSequence {
if (haveNewest) return;
if (busyFetchingMore) return;
assert(fetched);
assert(messages.isNotEmpty);
assert(newestFetchedMessageId != null);
await _fetchMore(
anchor: NumericAnchor(messages.last.id),
anchor: NumericAnchor(newestFetchedMessageId!),
numBefore: 0,
numAfter: kMessageListFetchBatchSize,
processResult: (result) {
_newestFetchedMessageId = result.messages.lastOrNull?.id ?? newestFetchedMessageId;
store.reconcileMessages(result.messages);
store.recentSenders.handleMessages(result.messages); // TODO(#824)

Expand Down
63 changes: 47 additions & 16 deletions lib/widgets/message_list.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,7 +895,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
model.fetchInitial();
}

bool _prevFetched = false;
bool _hasAutofocused = false;

void _modelChanged() {
// When you're scrolling quickly, our mark-as-read requests include the
Expand Down Expand Up @@ -923,14 +923,36 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
// This method was called because that just changed.
});

if (!_prevFetched && model.fetched && model.messages.isEmpty) {
// If the fetch came up empty, there's nothing to read,
// so opening the keyboard won't be bothersome and could be helpful.
SchedulerBinding.instance.addPostFrameCallback((_) {
if (!model.fetched || !scrollController.hasClients) {
return;
}

// If fetchInitial or fetchOlder/fetchNewer
// haven't filled model.messages with any visible (i.e. unmuted) messages,
// or anyway not enough to fill the screen, fetch again.
// If we're in a long run of muted messages, this has the effect of
// fetching in a loop until we've either fetched the narrow's whole history
// or we've filled the screen with visible messages,
// without needing user scroll input between iterations.
//
// The right time for the "if-needed" check is
// after the current model change has been laid out
// and the scroll metrics have been updated,
// so that when we receive and lay out a screenful of messages,
// we don't fetch again unnecessarily.
// That's why we do it in a post-frame callback.
_fetchMoreIfNeeded(scrollController.position);
});

if (model.messages.isEmpty && model.haveNewest && model.haveOldest && !_hasAutofocused) {
// If there are no messages to show in the whole history,
// opening the keyboard won't be bothersome and could be helpful.
// It's definitely helpful if we got here from the new-DM page.
MessageListPage.ancestorOf(context)
.composeBoxState?.controller.requestFocusIfUnfocused();
_hasAutofocused = true;
}
_prevFetched = model.fetched;
}

/// Find the range of message IDs on screen, as a (first, last) tuple,
Expand Down Expand Up @@ -1042,6 +1064,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 @@ -1053,17 +1084,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((_) {
Comment on lines +1087 to +1088
Copy link
Copy Markdown
Member

@gnprice gnprice Feb 13, 2026

Choose a reason for hiding this comment

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

Can you explain why this solves the problem (the double-fetch glitch)? And why .transientCallbacks; are there potentially any other phases where we should do the same thing?

Copy link
Copy Markdown
Collaborator Author

@sm-sayedi sm-sayedi Feb 19, 2026

Choose a reason for hiding this comment

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

Yeah, so as explained in #2104 description, double-fetch was used to happen whenever the response for a fetch-older request arrived while there was a fling going on in the message list. The fling (which is driven by an animation) causes the scroll position to change, but that change happens in .transientCallbacks phase (the phase where the animations tick, in this case, the animation for the fling). The change in the scroll position eventually calls _handleScrollMetrics (in the transientCallbacks phase) with old scroll metrics (specifically minScrollExtent), which causes a double fetch glitch. Then in the .persistentCallbacks phase, where the message list view is laid out with the newly arrived messages, minScrollExtent will be updated to account for the new messages. The updated minScrollExtent value will then be available in .postFrameCallbacks phase, so that's why we wrap _fetchMoreIfNeeded in addPostFrameCallback. And the check for .transientCallbacks is there because in this specific case, the double-fetch glitch is caused by _handleScrollMetrics being called in .transientCallbacks phase.

are there potentially any other phases where we should do the same thing?

Right now, _handleScrollMetrics is called by _scrollChanged and _handleScrollMetricsNotification.

  • _scrollChanged is called through ScrollPosition.setPixels. Its dartdoc says:
    "This should only be called by the current [ScrollActivity], either during the transient callback phase or in response to user input." So it's called either during .transientCallbacks or .idle (user input handlers are executed here) phase.
  • _handleScrollMetricsNotification is called through NotificationListener.onNotification. Its dartdoc says: "Notifications vary in terms of when they are dispatched. There are two main possibilities: dispatch between frames, and dispatch during layout." So it's called either during .idle or .persistentCallbacks phase. _handleScrollMetricsNotification is specifically called on ScrollMetricsNotification, which in turn is only dispatched in .idle phase — see ScrollPosition.didUpdateScrollMetrics and its call site.

So besides .transientCallbacks, .idle is the only other phase where _handleScrollMetrics could be called. Should we also check for the .idle phase, given that we still haven't experienced any double-fetch glitch caused by it?

if (scrollController.hasClients) {
Copy link
Copy Markdown
Member

@gnprice gnprice Feb 13, 2026

Choose a reason for hiding this comment

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

What's the reasoning for this condition?

Copy link
Copy Markdown
Collaborator Author

@sm-sayedi sm-sayedi Feb 19, 2026

Choose a reason for hiding this comment

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

In this case I think it doesn't really make any difference, but it's a safety check. Maybe there is a possibility that scrollController can lose clients post frame; I'm not sure. The same check is necessary for another addPostFrameCallback in one of the next commits, though, so I thought it would be a good idea to add it here too.

// 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
130 changes: 130 additions & 0 deletions test/model/message_list_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,6 +727,132 @@ void main() {
});
});

group('oldestFetchedMessageId, newestFetchedMessageId', () {
group('fetchInitial', () {
test('visible messages', () async {
await prepare();
check(model)..oldestFetchedMessageId.isNull()..newestFetchedMessageId.isNull();

connection.prepare(json: newestResult(
foundOldest: true,
messages: List.generate(100, (i) => eg.streamMessage(id: 100 + i)),
).toJson());
await model.fetchInitial();

checkNotifiedOnce();
check(model)
..messages.length.equals(100)
..oldestFetchedMessageId.equals(100)..newestFetchedMessageId.equals(199);
});

test('invisible messages', () async {
final mutedUser = eg.user();
await prepare(users: [mutedUser], mutedUserIds: [mutedUser.userId]);
check(model)..oldestFetchedMessageId.isNull()..newestFetchedMessageId.isNull();

connection.prepare(json: newestResult(
foundOldest: true,
messages: List.generate(100,
(i) => eg.dmMessage(id: 100 + i, from: eg.selfUser, to: [mutedUser])),
).toJson());
await model.fetchInitial();

checkNotifiedOnce();
check(model)
..messages.isEmpty()
..oldestFetchedMessageId.equals(100)..newestFetchedMessageId.equals(199);
});

test('no messages found', () async {
await prepare();
check(model)..oldestFetchedMessageId.isNull()..newestFetchedMessageId.isNull();

connection.prepare(json: newestResult(
foundOldest: true,
messages: [],
).toJson());
await model.fetchInitial();

checkNotifiedOnce();
check(model)
..messages.isEmpty()
..oldestFetchedMessageId.isNull()..newestFetchedMessageId.isNull();
});
});

group('fetching more', () {
test('visible messages', () async {
await prepare(anchor: AnchorCode.firstUnread);
check(model)..oldestFetchedMessageId.isNull()..newestFetchedMessageId.isNull();

await prepareMessages(
foundOldest: false, foundNewest: false,
anchorMessageId: 250,
messages: List.generate(100, (i) => eg.streamMessage(id: 200 + i)));
check(model)
..messages.length.equals(100)
..oldestFetchedMessageId.equals(200)..newestFetchedMessageId.equals(299);

connection.prepare(json: olderResult(
anchor: 200, foundOldest: true,
messages: List.generate(100, (i) => eg.streamMessage(id: 100 + i)),
).toJson());
await model.fetchOlder();
checkNotified(count: 2);
check(model)
..messages.length.equals(200)
..oldestFetchedMessageId.equals(100)..newestFetchedMessageId.equals(299);

connection.prepare(json: newerResult(
anchor: 299, foundNewest: true,
messages: List.generate(100, (i) => eg.streamMessage(id: 300 + i)),
).toJson());
await model.fetchNewer();
checkNotified(count: 2);
check(model)
..messages.length.equals(300)
..oldestFetchedMessageId.equals(100)..newestFetchedMessageId.equals(399);
});

test('invisible messages', () async {
final mutedUser = eg.user();
await prepare(anchor: AnchorCode.firstUnread,
users: [mutedUser], mutedUserIds: [mutedUser.userId]);
check(model)..oldestFetchedMessageId.isNull()..newestFetchedMessageId.isNull();

await prepareMessages(
foundOldest: false, foundNewest: false,
anchorMessageId: 250,
messages: List.generate(100, (i) => eg.streamMessage(id: 200 + i)));
check(model)
..messages.length.equals(100)
..oldestFetchedMessageId.equals(200)..newestFetchedMessageId.equals(299);

connection.prepare(json: olderResult(
anchor: 200, foundOldest: true,
messages: List.generate(100,
(i) => eg.dmMessage(id: 100 + i, from: eg.selfUser, to: [mutedUser])),
).toJson());
await model.fetchOlder();
checkNotified(count: 2);
check(model)
..messages.length.equals(100)
..oldestFetchedMessageId.equals(100)..newestFetchedMessageId.equals(299);

connection.prepare(json: newerResult(
anchor: 299, foundNewest: true,
messages: List.generate(100,
(i) => eg.dmMessage(id: 300 + i, from: eg.selfUser, to: [mutedUser])),
).toJson());
await model.fetchNewer();
checkNotified(count: 2);
check(model)
..messages.length.equals(100)
..oldestFetchedMessageId.equals(100)..newestFetchedMessageId.equals(399);
});
});
});

// TODO(#1569): test jumpToEnd

group('MessageEvent', () {
Expand Down Expand Up @@ -3270,6 +3396,8 @@ void checkInvariants(MessageListView model) {
check(model)
..messages.isEmpty()
..outboxMessages.isEmpty()
..oldestFetchedMessageId.isNull()
..newestFetchedMessageId.isNull()
..haveOldest.isFalse()
..haveNewest.isFalse()
..busyFetchingMore.isFalse();
Expand Down Expand Up @@ -3458,6 +3586,8 @@ extension MessageListViewChecks on Subject<MessageListView> {
Subject<List<MessageListItem>> get items => has((x) => x.items, 'items');
Subject<int> get middleItem => has((x) => x.middleItem, 'middleItem');
Subject<bool> get fetched => has((x) => x.fetched, 'fetched');
Subject<int?> get oldestFetchedMessageId => has((x) => x.oldestFetchedMessageId, 'oldestFetchedMessageId');
Subject<int?> get newestFetchedMessageId => has((x) => x.newestFetchedMessageId, 'newestFetchedMessageId');
Subject<bool> get haveOldest => has((x) => x.haveOldest, 'haveOldest');
Subject<bool> get haveNewest => has((x) => x.haveNewest, 'haveNewest');
Subject<bool> get busyFetchingMore => has((x) => x.busyFetchingMore, 'busyFetchingMore');
Expand Down
8 changes: 4 additions & 4 deletions test/model/message_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ void main() {

test('outbox messages get unique localMessageId', () async {
await prepare(stream: stream);
await prepareMessages([]);
await prepareMessages([], foundOldest: true);

for (int i = 0; i < 10; i++) {
connection.prepare(json: SendMessageResult(id: 1).toJson());
Expand Down Expand Up @@ -566,7 +566,7 @@ void main() {
test('takeOutboxMessage', () async {
final stream = eg.stream();
await prepare(stream: stream);
await prepareMessages([]);
await prepareMessages([], foundOldest: true);

for (int i = 0; i < 10; i++) {
connection.prepare(apiException: eg.apiBadRequest());
Expand Down Expand Up @@ -1745,7 +1745,7 @@ void main() {

// The actual message hasn't been fetched by a message list;
// we want to test [MessageStore.starredMessages] in isolation.
await prepareMessages([]);
await prepareMessages([], foundOldest: true);

check(store).starredMessages.single.equals(message.id);
await store.handleEvent(eg.deleteMessageEvent([message]));
Expand Down Expand Up @@ -1806,7 +1806,7 @@ void main() {

test('all: true; we don\'t know about any messages', () async {
await prepare();
await prepareMessages([]);
await prepareMessages([], foundOldest: true);
await store.handleEvent(mkAddEvent(MessageFlag.read, [], all: true));
checkNotNotified();
});
Expand Down
Loading