From ef2aa1ae38fadcc0bc22b8bf6ed96b35933146be Mon Sep 17 00:00:00 2001 From: Tom Brow Date: Sat, 19 Sep 2026 11:00:47 -0700 Subject: [PATCH] fix(mobile): preserve thread replies through refresh failures (#7757) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary A failed mobile thread refresh could briefly remove replies and show “0 replies” even though Riverpod still retained the previous result. Read the retained reply value directly and merge it with pending local replies through errors and retries. When a reopened thread has no retained result, show loading or failure instead of a false zero count. Error status remains visible alongside retained replies. No additional cache or retention is introduced. Current main intentionally disposes thread queries on close; reopening still fetches fresh data. The recorded symptom is reproduced by the failed-refresh test, but the recording alone does not establish its network trigger. ### Related issue Related to #4831 (thread refresh and disposal behavior), incorporated into main through #6691. No duplicate for this rendering defect found. ### Testing - Reproduced disappearing cached replies in widget tests before the fix; the new retry/recovery, terminal-error, reopen/loading, initial-error, and empty-result tests now pass. - Provider coverage verifies cached and pending local replies survive a failed refresh and confirmation still clears the local overlay. - All five new widget regressions, the existing hydration/scroll-position regression, all nine thread-provider tests, Flutter analysis, and the final-head full mobile pre-push gate passed (2,261 tests passed; four skipped). - Phone-width and large-text rendering checked with synthetic messages. [Before/after screenshots](https://github.com/block/buzz/pull/7757#issuecomment-5743469556). - All final-head GitHub checks passed, including mobile tests, Android debug APK, iOS simulator app, and unsigned iOS release builds. - Codex code review and security review are clean for `4e5f606ae`; the earlier accessibility finding is fixed and resolved. Builderbot review was requested but has not returned a result. - Broad local `just ci` could not finish: unchanged ACP timing tests failed, including `keepalive_resets_idle_past_deadline` on the lower-concurrency retry. The separate ACP run passed all 941 tests. Initial parallel mobile runs also hit timing flakes; the final-head full mobile gate passed. No unrelated native code was changed. --------- Signed-off-by: Tom Brow --- .../features/channels/thread_detail_page.dart | 18 +- .../thread_detail_page/message_list.dart | 34 ++- .../channels/thread_replies_provider.dart | 9 +- .../channels/channel_detail_page_test.dart | 3 + .../thread_replies_provider_test.dart | 47 +++++ .../channels/thread_reply_refresh_cases.dart | 195 ++++++++++++++++++ 6 files changed, 286 insertions(+), 20 deletions(-) create mode 100644 mobile/test/features/channels/thread_reply_refresh_cases.dart diff --git a/mobile/lib/features/channels/thread_detail_page.dart b/mobile/lib/features/channels/thread_detail_page.dart index 154193b3d2..0bb4cedb34 100644 --- a/mobile/lib/features/channels/thread_detail_page.dart +++ b/mobile/lib/features/channels/thread_detail_page.dart @@ -128,14 +128,15 @@ class ThreadDetailPage extends HookConsumerWidget { final liveChannelEvents = ref.watch(channelMessagesProvider(channelId)).value ?? const []; - final replyMessages = repliesState.whenData((events) { - return formatTimeline( - mergeThreadEvents(events, liveChannelEvents), - currentPubkey: currentPubkey, - ); - }); - - final fetchedReplies = replyMessages.value; + // Loading/error states can still carry replies from the last query. + // whenData drops that retained value during retries. + final replyEvents = repliesState.value; + final fetchedReplies = replyEvents == null + ? null + : formatTimeline( + mergeThreadEvents(replyEvents, liveChannelEvents), + currentPubkey: currentPubkey, + ); final hasFetchedReplies = fetchedReplies != null; // A terminal query error cannot produce a more authoritative list. Keep // loading states provisional, but let the hydrated route snapshot drive @@ -892,6 +893,7 @@ class ThreadDetailPage extends HookConsumerWidget { itemPositionsListener: itemPositionsListener, bottomInset: timelineBottomInset, replies: replies, + relayReplyState: relayReplyState, localSendAnimations: localSendAnimations, trackActiveScrollPosition: trackActiveScrollPosition, headIsDeleted: liveDeletionHidesHead, diff --git a/mobile/lib/features/channels/thread_detail_page/message_list.dart b/mobile/lib/features/channels/thread_detail_page/message_list.dart index 90e227ab7d..a33aa9a4ee 100644 --- a/mobile/lib/features/channels/thread_detail_page/message_list.dart +++ b/mobile/lib/features/channels/thread_detail_page/message_list.dart @@ -9,6 +9,7 @@ class _ThreadMessageList extends StatelessWidget { final ItemPositionsListener itemPositionsListener; final double bottomInset; final List replies; + final AsyncValue> relayReplyState; final Map localSendAnimations; final Widget Function(Widget child) trackActiveScrollPosition; final bool headIsDeleted; @@ -34,6 +35,7 @@ class _ThreadMessageList extends StatelessWidget { required this.itemPositionsListener, required this.bottomInset, required this.replies, + required this.relayReplyState, required this.localSendAnimations, required this.trackActiveScrollPosition, required this.headIsDeleted, @@ -51,6 +53,18 @@ class _ThreadMessageList extends StatelessWidget { required this.childrenByParent, }); + String get _replySummary { + if (!relayReplyState.hasValue && replies.isEmpty) { + return relayReplyState.isLoading + ? 'Loading replies…' + : 'Couldn’t load replies'; + } + final count = + '${replies.length} ${replies.length == 1 ? 'reply' : 'replies'}'; + if (!relayReplyState.hasError) return count; + return '$count · ${relayReplyState.isLoading ? 'Retrying…' : 'Couldn’t refresh'}'; + } + @override Widget build(BuildContext context) { const headIndex = 0; @@ -136,13 +150,19 @@ class _ThreadMessageList extends StatelessWidget { ), child: Row( children: [ - Text( - '${replies.length} ${replies.length == 1 ? 'reply' : 'replies'}', - style: context.textTheme.labelMedium - ?.copyWith( - color: context.colors.onSurfaceVariant, - fontWeight: FontWeight.w600, - ), + Flexible( + child: Semantics( + liveRegion: true, + child: Text( + _replySummary, + style: context.textTheme.labelMedium + ?.copyWith( + color: + context.colors.onSurfaceVariant, + fontWeight: FontWeight.w600, + ), + ), + ), ), const SizedBox(width: Grid.xxs), Expanded( diff --git a/mobile/lib/features/channels/thread_replies_provider.dart b/mobile/lib/features/channels/thread_replies_provider.dart index 1fb19432f1..c49461834e 100644 --- a/mobile/lib/features/channels/thread_replies_provider.dart +++ b/mobile/lib/features/channels/thread_replies_provider.dart @@ -130,11 +130,10 @@ final threadRepliesWithLocalProvider = Provider.autoDispose } } if (localReplies.isEmpty) return relayReplies; - return relayReplies.when( - data: (events) => AsyncData(_mergeReplies(events, localReplies)), - loading: () => AsyncData(localReplies), - error: (error, stackTrace) => AsyncData(localReplies), - ); + // This provider supplies display events; the original query remains + // the source of loading/error status. Preserve its retained value when + // merging optimistic replies during a failed refresh or retry. + return AsyncData(_mergeReplies(authoritative ?? const [], localReplies)); }); /// Union two event lists by id, newest-wins, in timeline order. diff --git a/mobile/test/features/channels/channel_detail_page_test.dart b/mobile/test/features/channels/channel_detail_page_test.dart index 7159fb2fd1..23a83ddab3 100644 --- a/mobile/test/features/channels/channel_detail_page_test.dart +++ b/mobile/test/features/channels/channel_detail_page_test.dart @@ -64,6 +64,8 @@ import 'package:buzz/shared/widgets/masked_avatar_badge.dart'; import 'package:buzz/shared/widgets/skeleton.dart'; import 'package:shared_preferences/shared_preferences.dart'; +part 'thread_reply_refresh_cases.dart'; + const _channelId = '11111111-2222-4333-8444-555555555555'; const _huddleChannelId = '8d764100-fd8f-44cf-9c98-6d8fbd739b8c'; const _otherChannelId = '22222222-3333-4444-8555-666666666666'; @@ -471,6 +473,7 @@ double? effectiveFontSizeForText( } void main() { + threadReplyRefreshTests(); setUp(() async { SharedPreferences.setMockInitialValues({}); _testPrefs = await SharedPreferences.getInstance(); diff --git a/mobile/test/features/channels/thread_replies_provider_test.dart b/mobile/test/features/channels/thread_replies_provider_test.dart index cdfc16fe81..419bd1e533 100644 --- a/mobile/test/features/channels/thread_replies_provider_test.dart +++ b/mobile/test/features/channels/thread_replies_provider_test.dart @@ -215,6 +215,53 @@ void main() { }, ); + test( + 'local replies preserve cached authoritative replies after refresh failure', + () async { + final cached = _reply('cached', 1000); + final local = _reply('local', 1001); + final session = _FakeRelaySession()..replies = [cached]; + final container = ProviderContainer( + retry: (_, _) => null, + overrides: [relaySessionProvider.overrideWith(() => session)], + ); + addTearDown(container.dispose); + final combined = container.listen( + threadRepliesWithLocalProvider(args), + (_, _) {}, + ); + await container.read(threadRepliesProvider(args).future); + container.read(threadLocalRepliesProvider(args).notifier).add(local); + await container.pump(); + final refresh = Completer>(); + session.nextQueryGate = refresh; + container.invalidate(threadRepliesProvider(args)); + await container.pump(); + expect(combined.read().value?.map((event) => event.id), [ + 'cached', + 'local', + ]); + refresh.completeError(Exception('Refresh failed')); + await container.pump(); + expect(container.read(threadRepliesProvider(args)).hasError, isTrue); + expect(combined.read().value?.map((event) => event.id), [ + 'cached', + 'local', + ]); + expect(container.read(threadLocalRepliesProvider(args)), [local]); + session.replies = [cached, local]; + container.invalidate(threadRepliesProvider(args)); + await container.read(threadRepliesProvider(args).future); + await container.pump(); + expect(combined.read().value?.map((event) => event.id), [ + 'cached', + 'local', + ]); + await Future.delayed(Duration.zero); + expect(container.read(threadLocalRepliesProvider(args)), isEmpty); + }, + ); + test('reopening a disposed thread performs a fresh load', () async { final (container, fakeSession, subscription) = makeHarness([ _reply('r1', 1000), diff --git a/mobile/test/features/channels/thread_reply_refresh_cases.dart b/mobile/test/features/channels/thread_reply_refresh_cases.dart new file mode 100644 index 0000000000..c788731fc5 --- /dev/null +++ b/mobile/test/features/channels/thread_reply_refresh_cases.dart @@ -0,0 +1,195 @@ +part of 'channel_detail_page_test.dart'; + +void threadReplyRefreshTests() { + const args = ThreadRepliesArgs(channelId: _channelId, rootId: 'refresh-root'); + final root = _textMsg( + id: args.rootId, + pubkey: 'alice', + content: 'Refresh root', + ); + final reply = _textMsg( + id: 'cached-reply', + pubkey: 'bob', + content: 'Cached reply', + createdAt: 1001, + extraTags: [ + ['e', args.rootId, '', 'reply'], + ], + ); + + Future<(NavigatorState, ProviderContainer)> mount( + WidgetTester tester, + Future> Function() load, { + bool retry = false, + TextScaler textScaler = TextScaler.noScaling, + }) async { + await tester.pumpWidget( + _buildTestable( + messages: [root], + textScaler: textScaler, + threadReplyLoaders: {args.rootId: load}, + providerRetry: (_, _) => + retry ? const Duration(milliseconds: 200) : null, + ), + ); + await tester.pumpAndSettle(); + final context = tester.element(find.byType(ChannelDetailPage)); + return (Navigator.of(context), ProviderScope.containerOf(context)); + } + + void open(NavigatorState navigator) { + final head = formatTimeline([root]).single; + navigator.push( + MaterialPageRoute( + builder: (_) => ThreadDetailPage( + threadHead: head, + allMessages: [head], + channelId: _channelId, + currentPubkey: 'self', + isMember: true, + isArchived: false, + ), + ), + ); + } + + testWidgets( + 'thread refresh keeps cached replies through retry and recovery', + (tester) async { + var calls = 0; + final refresh = Completer>(); + final retry = Completer>(); + final (navigator, container) = await mount(tester, () { + calls++; + return switch (calls) { + 1 => Future.value([reply]), + 2 => refresh.future, + _ => retry.future, + }; + }, retry: true); + open(navigator); + await tester.pumpAndSettle(); + expect(find.text('1 reply'), findsOneWidget); + container.invalidate(threadRepliesProvider(args)); + await tester.pump(); + expect(find.text('1 reply'), findsOneWidget); + refresh.completeError(Exception('Transient refresh failure')); + await tester.pump(); + for (var frame = 0; frame < 20; frame++) { + await tester.pump(const Duration(milliseconds: 16)); + expect(find.text('0 replies'), findsNothing, reason: 'frame $frame'); + expect( + find.byKey(const ValueKey('thread-message-group-cached-reply')), + findsOneWidget, + ); + expect(container.read(threadRepliesProvider(args)).value, [reply]); + } + expect(calls, 3); + retry.complete([ + reply, + _textMsg( + id: 'fresh-reply', + pubkey: 'bob', + content: 'Fresh reply', + createdAt: 1002, + extraTags: [ + ['e', args.rootId, '', 'reply'], + ], + ), + ]); + await tester.pumpAndSettle(); + expect(find.text('2 replies'), findsOneWidget); + expect( + find.byKey(const ValueKey('thread-message-group-fresh-reply')), + findsOneWidget, + ); + }, + ); + + testWidgets('thread refresh error keeps cached replies visible', ( + tester, + ) async { + final semantics = tester.ensureSemantics(); + var calls = 0; + final refresh = Completer>(); + tester.view.physicalSize = const Size(320, 844); + tester.view.devicePixelRatio = 1; + addTearDown(tester.view.resetPhysicalSize); + addTearDown(tester.view.resetDevicePixelRatio); + final (navigator, container) = await mount( + tester, + () => ++calls == 1 ? Future.value([reply]) : refresh.future, + textScaler: const TextScaler.linear(2), + ); + open(navigator); + await tester.pumpAndSettle(); + container.invalidate(threadRepliesProvider(args)); + await tester.pump(); + refresh.completeError(Exception('Refresh failed')); + await tester.pumpAndSettle(); + expect( + find.byKey(const ValueKey('thread-message-group-cached-reply')), + findsOneWidget, + ); + expect(find.text('0 replies'), findsNothing); + expect(find.text('1 reply · Couldn’t refresh'), findsOneWidget); + expect( + tester + .getSemantics(find.text('1 reply · Couldn’t refresh')) + .getSemanticsData() + .flagsCollection + .isLiveRegion, + isTrue, + ); + expect(tester.takeException(), isNull); + expect(container.read(threadRepliesProvider(args)).hasError, isTrue); + semantics.dispose(); + }); + + testWidgets('thread reopen shows loading until the fresh query completes', ( + tester, + ) async { + var calls = 0; + final reopened = Completer>(); + final (navigator, _) = await mount( + tester, + () => ++calls == 1 ? Future.value([reply]) : reopened.future, + ); + open(navigator); + await tester.pumpAndSettle(); + navigator.pop(); + await tester.pumpAndSettle(); + open(navigator); + await tester.pumpAndSettle(); + expect(calls, 2); + expect(find.text('0 replies'), findsNothing); + expect(find.text('Loading replies…'), findsOneWidget); + reopened.complete([reply]); + await tester.pumpAndSettle(); + expect(find.text('1 reply'), findsOneWidget); + expect(find.text('Loading replies…'), findsNothing); + }); + + testWidgets('thread initial error is distinct from an empty thread', ( + tester, + ) async { + final query = Completer>(); + final (navigator, _) = await mount(tester, () => query.future); + open(navigator); + await tester.pumpAndSettle(); + query.completeError(Exception('Initial load failed')); + await tester.pumpAndSettle(); + expect(find.text('0 replies'), findsNothing); + expect(find.text('Couldn’t load replies'), findsOneWidget); + }); + testWidgets('thread successful empty query displays zero replies', ( + tester, + ) async { + final (navigator, _) = await mount(tester, () async => []); + open(navigator); + await tester.pumpAndSettle(); + expect(find.text('0 replies'), findsOneWidget); + expect(find.text('Loading replies…'), findsNothing); + expect(find.text('Couldn’t load replies'), findsNothing); + }); +}