From 327580d00a891a2c1823918e3404497c1935d0c3 Mon Sep 17 00:00:00 2001 From: Bretton Date: Sun, 20 Sep 2026 20:56:42 -0700 Subject: [PATCH] fix(post-detail): focus profile comments reliably Prioritize explicit profile-comment navigation over cached thread positions so the requested comment is not missed by a downward-only scan. Changes: - Start focused visits at the top while preserving normal restoration. - Detect collapsed and depth-limited targets before scrolling. - Fall back to the focused thread without leaving the post at the bottom. - Add widget coverage for cache restoration, focus, and failure paths. --- lib/screens/home/post_detail_screen.dart | 89 +++-- ...post_detail_screen_comment_focus_test.dart | 325 ++++++++++++++++++ 2 files changed, 387 insertions(+), 27 deletions(-) create mode 100644 test/widgets/post_detail_screen_comment_focus_test.dart diff --git a/lib/screens/home/post_detail_screen.dart b/lib/screens/home/post_detail_screen.dart index 7164815..1bf3962 100644 --- a/lib/screens/home/post_detail_screen.dart +++ b/lib/screens/home/post_detail_screen.dart @@ -27,6 +27,27 @@ import '../../widgets/tappable_community.dart'; import '../compose/reply_screen.dart'; import 'focused_thread_screen.dart'; +const _commentThreadMaxDepth = 6; + +bool _isCommentDisplayable( + Iterable comments, + String targetUri, + Set collapsedComments, +) { + bool visit(ThreadViewComment thread, int depth) { + if (thread.comment.uri == targetUri) { + return true; + } + if (depth >= _commentThreadMaxDepth || + collapsedComments.contains(thread.comment.uri)) { + return false; + } + return thread.replies?.any((reply) => visit(reply, depth + 1)) ?? false; + } + + return comments.any((thread) => visit(thread, 0)); +} + /// Post Detail Screen /// /// Displays a full post with its comments. @@ -86,8 +107,8 @@ class PostDetailScreen extends StatefulWidget { } class _PostDetailScreenState extends State { - // ScrollController created lazily with cached scroll position for instant - // restoration + // ScrollController created lazily; explicit comment focus overrides + // restoration. late ScrollController _scrollController; final GlobalKey _commentsHeaderKey = GlobalKey(); @@ -174,7 +195,7 @@ class _PostDetailScreenState extends State { /// /// Called from didChangeDependencies to ensure cached data is available /// for the first build. Creates ScrollController with initialScrollOffset - /// set to cached position for instant scroll restoration without flicker. + /// set to cached position unless an explicit comment focus takes precedence. void _initializeProviderSync() { // Get or create provider from cache final cache = context.read(); @@ -184,18 +205,21 @@ class _PostDetailScreenState extends State { postCid: widget.post.post.cid, ); - // Create scroll controller with cached position for instant restoration - // This avoids the flash: loading → content at top → jump to cached position - final cachedScrollPosition = _commentsProvider.scrollPosition; + // Focus scans downward from the top, so cached scroll restoration must + // not start below the requested comment. + final hasCommentFocus = widget.focusCommentUri != null; + final initialScrollOffset = hasCommentFocus + ? 0.0 + : _commentsProvider.scrollPosition; _scrollController = ScrollController( - initialScrollOffset: cachedScrollPosition, + initialScrollOffset: initialScrollOffset, ); _scrollController.addListener(_onScroll); - if (kDebugMode && cachedScrollPosition > 0) { + if (kDebugMode && initialScrollOffset > 0) { debugPrint( '📍 Created ScrollController with initial offset: ' - '$cachedScrollPosition', + '$initialScrollOffset', ); } @@ -298,8 +322,8 @@ class _PostDetailScreenState extends State { /// Runs once after the first successful thread load. The comment list is /// a lazy sliver, so the target's key has no context until its top-level /// ancestor has been laid out: we page the viewport down until it appears, - /// then `ensureVisible` it. If the comment isn't in the loaded tree at all, - /// its subtree is fetched by rkey and opened as a focused thread. + /// then `ensureVisible` it. If the comment cannot be displayed in the loaded + /// tree, its subtree is fetched by rkey and opened as a focused thread. Future _tryFocusComment() async { final targetUri = widget.focusCommentUri; if (targetUri == null || @@ -325,19 +349,29 @@ class _PostDetailScreenState extends State { final messenger = ScaffoldMessenger.of(context); try { - final inTree = provider.comments.any( - (thread) => thread.findByUri(targetUri) != null, + final isDisplayable = _isCommentDisplayable( + provider.comments, + targetUri, + provider.collapsedComments, ); - if (inTree) { - await _scrollToFocusedComment(); - return; + if (isDisplayable) { + final displayed = await _scrollToFocusedComment(); + if (!mounted || _providerInvalidated || displayed) { + return; + } + if (_scrollController.hasClients) { + _scrollController.jumpTo(0); + } + if (!mounted || _providerInvalidated) { + return; + } } - // Not on the loaded page(s) / past the depth cutoff: fetch its subtree - // and present it focused, with this full thread underneath. + // Missing or not displayable (e.g. collapsed / past the depth cutoff): + // fetch its subtree and present it focused, with this thread underneath. final subtree = await provider.loadMoreReplies(targetUri); - if (!mounted) { + if (!mounted || _providerInvalidated) { return; } if (subtree == null) { @@ -349,7 +383,7 @@ class _PostDetailScreenState extends State { if (kDebugMode) { debugPrint('⚠️ Could not focus comment: $e'); } - if (mounted) { + if (mounted && !_providerInvalidated) { _showFocusFailed(messenger); } } finally { @@ -357,11 +391,11 @@ class _PostDetailScreenState extends State { } } - Future _scrollToFocusedComment() async { + Future _scrollToFocusedComment() async { // Lazy sliver: page down until the keyed card is built, then align it. for (var attempt = 0; attempt < 60; attempt++) { - if (!mounted) { - return; + if (!mounted || _providerInvalidated) { + return false; } final targetContext = _focusedCommentKey.currentContext; if (targetContext != null && targetContext.mounted) { @@ -373,15 +407,15 @@ class _PostDetailScreenState extends State { duration: const Duration(milliseconds: 300), curve: Curves.easeInOut, ); - return; + return mounted && !_providerInvalidated; } if (!_scrollController.hasClients) { - return; + return false; } final position = _scrollController.position; if (position.pixels >= position.maxScrollExtent) { // Reached the end without finding it (it was collapsed or removed). - return; + return false; } _scrollController.jumpTo( (position.pixels + position.viewportDimension).clamp( @@ -391,6 +425,7 @@ class _PostDetailScreenState extends State { ); await WidgetsBinding.instance.endOfFrame; } + return false; } void _showFocusFailed(ScaffoldMessengerState messenger) { @@ -1179,7 +1214,7 @@ class _CommentItem extends StatelessWidget { return CommentThread( thread: comment, currentTime: currentTime, - maxDepth: 6, + maxDepth: _commentThreadMaxDepth, onCommentTap: onCommentTap, collapsedComments: collapsedComments, onCollapseToggle: onCollapseToggle, diff --git a/test/widgets/post_detail_screen_comment_focus_test.dart b/test/widgets/post_detail_screen_comment_focus_test.dart new file mode 100644 index 0000000..c1aa381 --- /dev/null +++ b/test/widgets/post_detail_screen_comment_focus_test.dart @@ -0,0 +1,325 @@ +import 'dart:async'; + +import 'package:coves_flutter/constants/app_theme.dart'; +import 'package:coves_flutter/models/comment.dart'; +import 'package:coves_flutter/models/post.dart'; +import 'package:coves_flutter/providers/auth_provider.dart'; +import 'package:coves_flutter/providers/block_provider.dart'; +import 'package:coves_flutter/providers/comments_provider.dart'; +import 'package:coves_flutter/providers/vote_provider.dart'; +import 'package:coves_flutter/screens/home/focused_thread_screen.dart'; +import 'package:coves_flutter/screens/home/post_detail_screen.dart'; +import 'package:coves_flutter/services/comments_provider_cache.dart'; +import 'package:coves_flutter/widgets/post_action_bar.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:mockito/mockito.dart'; +import 'package:provider/provider.dart'; + +import '../test_helpers/test_mocks.dart'; + +void main() { + const postUri = 'at://did:plc:author/social.coves.community.post/post'; + const postCid = 'post-cid'; + const targetContent = 'Early profile comment target'; + const savedOffset = 4500.0; + const failureMessage = + "Couldn't find that comment. It may have been deleted."; + + late MockAuthProvider auth; + late MockVoteProvider votes; + late MockCovesApiService api; + late BlockProvider blocks; + late CommentsProviderCache cache; + late CommentsProvider comments; + late ThreadViewComment target; + late List threads; + late List requests; + Completer? refresh; + Completer? subtreeFetch; + var failSubtree = false; + + ThreadViewComment thread(String rkey, {List? replies}) { + return ThreadViewComment( + comment: CommentView( + uri: 'at://did:plc:author/social.coves.community.comment/$rkey', + cid: 'cid-$rkey', + record: CommentRecord( + content: rkey == 'target' ? targetContent : 'Comment $rkey', + ), + createdAt: DateTime(2025), + indexedAt: DateTime(2025), + author: AuthorView(did: 'did:plc:author', handle: 'author.test'), + post: CommentRef(uri: postUri, cid: postCid), + stats: CommentStats(replyCount: replies?.length ?? 0), + ), + replies: replies, + ); + } + + CommentsResponse response(List items) => + CommentsResponse(post: null, comments: items); + + ThreadViewComment targetAtDepth(int depth) { + var nested = target; + for (var currentDepth = depth - 1; currentDepth >= 0; currentDepth--) { + nested = thread('depth-$currentDepth', replies: [nested]); + } + return nested; + } + + setUp(() { + auth = MockAuthProvider(); + votes = MockVoteProvider(); + api = MockCovesApiService(); + when(auth.isAuthenticated).thenReturn(false); + when(votes.isLiked(any)).thenReturn(false); + when(votes.getVoteState(any)).thenReturn(null); + when(votes.isPending(any)).thenReturn(false); + when(votes.getAdjustedScore(any, any)) + .thenAnswer((invocation) => invocation.positionalArguments[1] as int); + blocks = BlockProvider(apiService: api, authProvider: auth); + cache = CommentsProviderCache( + authProvider: auth, + voteProvider: votes, + commentService: MockCommentService(), + apiService: api, + ); + requests = []; + refresh = null; + subtreeFetch = null; + failSubtree = false; + target = thread('target'); + threads = [ + target, + ...List.generate(80, (index) => thread('filler-$index')), + ]; + when(api.getComments( + postUri: anyNamed('postUri'), + sort: anyNamed('sort'), + timeframe: anyNamed('timeframe'), + depth: anyNamed('depth'), + limit: anyNamed('limit'), + cursor: anyNamed('cursor'), + parentRkey: anyNamed('parentRkey'), + )).thenAnswer((invocation) async { + final parent = invocation.namedArguments[#parentRkey] as String?; + requests.add(parent); + if (parent != null) { + if (failSubtree) { + throw Exception('Subtree fetch failed'); + } + return subtreeFetch == null + ? response([target]) + : await subtreeFetch!.future; + } + return refresh == null ? response(threads) : await refresh!.future; + }); + }); + + Widget app({String? focusCommentUri}) => MultiProvider( + providers: [ + ChangeNotifierProvider.value(value: auth), + ChangeNotifierProvider.value(value: votes), + ChangeNotifierProvider.value(value: blocks), + Provider.value(value: cache), + ], + child: MaterialApp( + theme: AppTheme.dark, + home: PostDetailScreen( + focusCommentUri: focusCommentUri, + post: FeedViewPost( + post: PostView( + uri: postUri, + cid: postCid, + rkey: 'post', + author: AuthorView( + did: 'did:plc:author', + handle: 'author.test', + ), + community: CommunityRef(did: 'did:plc:community', name: 'test'), + createdAt: DateTime(2025), + indexedAt: DateTime(2025), + record: const PostRecord(content: 'Profile navigation post'), + stats: PostStats( + upvotes: 0, + downvotes: 0, + score: 0, + commentCount: 81, + ), + ), + ), + ), + ), + ); + + ScrollPosition position(WidgetTester tester) => tester + .state(find.descendant( + of: find.byType(CustomScrollView), + matching: find.byType(Scrollable), + ).first) + .position; + + void testCachedVisit( + String description, + Future Function(WidgetTester) body, + ) { + testWidgets(description, (tester) async { + try { + await body(tester); + } finally { + // Dispose the provider's timer before test invariants run, + // and unmount both routes before disposing their shared cache. + await tester.pumpWidget(const SizedBox.shrink()); + if (refresh != null && !refresh!.isCompleted) { + refresh!.complete(response(threads)); + await tester.pump(); + } + if (subtreeFetch != null && !subtreeFetch!.isCompleted) { + subtreeFetch!.complete(response([target])); + await tester.pump(); + } + cache.dispose(); + blocks.dispose(); + } + }); + } + + Future cacheScrolledVisit( + WidgetTester tester, { + bool collapsed = false, + }) async { + if (collapsed) { + threads[0] = thread('ancestor', replies: [target]); + } + await tester.pumpWidget(app()); + await tester.pumpAndSettle(); + comments = cache.peekProvider(postUri)!; + expect(requests, [null]); + if (collapsed) { + comments.toggleCollapsed(threads.first.comment.uri); + await tester.pumpAndSettle(); + } + position(tester).jumpTo(savedOffset); + await tester.pumpAndSettle(); + expect(position(tester).pixels, closeTo(savedOffset, 1)); + // The early target is outside both the viewport and lazy sliver cache. + expect(find.text(targetContent), findsNothing); + expect(comments.scrollPosition, closeTo(savedOffset, 1)); + await tester.pumpWidget(const SizedBox.shrink()); + await tester.pump(); + expect(comments.isStale, isFalse); + requests.clear(); + } + + Future reopenFocused(WidgetTester tester) async { + refresh = Completer(); + await tester.pumpWidget(app(focusCommentUri: target.comment.uri)); + expect(identical(cache.peekProvider(postUri), comments), isTrue); + expect(requests, [null], reason: 'A fresh cache must still force refresh'); + expect(comments.isLoading, isTrue); + refresh!.complete(response(threads)); + // Allow the bounded lazy-sliver scan and route/scroll animations to finish. + // Keep elapsed time below the failure snackbar dismissal duration. + for (var frame = 0; frame < 100; frame++) { + await tester.pump(const Duration(milliseconds: 16)); + } + expect(comments.isLoading, isFalse); + } + + testCachedVisit('no-focus revisit preserves the cached scroll position', + (tester) async { + await cacheScrolledVisit(tester); + await tester.pumpWidget(app()); + await tester.pumpAndSettle(); + + expect(identical(cache.peekProvider(postUri), comments), isTrue); + expect(position(tester).pixels, closeTo(savedOffset, 1)); + expect(find.text(targetContent), findsNothing); + expect(requests, isEmpty); + }); + + testCachedVisit('focused revisit reveals an early target above cached scroll', + (tester) async { + await cacheScrolledVisit(tester); + await reopenFocused(tester); + + expect(find.byType(FocusedThreadScreen), findsNothing); + expect( + requests, + [null], + reason: 'An ordinary in-tree target needs no subtree', + ); + final targetFinder = find.text(targetContent); + expect(targetFinder, findsOneWidget); + final targetRect = tester.getRect(targetFinder); + final viewport = tester.getRect(find.byType(CustomScrollView)); + final actionBar = tester.getRect(find.byType(PostActionBar)); + expect(targetRect.top, greaterThanOrEqualTo(viewport.top + kToolbarHeight)); + expect(targetRect.bottom, lessThanOrEqualTo(actionBar.top)); + expect(targetFinder.hitTestable(), findsOneWidget); + }); + + testCachedVisit('collapsed in-tree target opens the fetched focused subtree', + (tester) async { + await cacheScrolledVisit(tester, collapsed: true); + await reopenFocused(tester); + + expect(comments.isCollapsed(threads.first.comment.uri), isTrue); + expect(find.byType(FocusedThreadScreen), findsOneWidget); + expect(requests.whereType(), contains('target')); + expect(find.text(targetContent).hitTestable(), findsOneWidget); + }); + + testCachedVisit('collapsed target bypasses the underlying scroll scan', + (tester) async { + await cacheScrolledVisit(tester, collapsed: true); + subtreeFetch = Completer(); + await reopenFocused(tester); + + expect(requests.whereType(), contains('target')); + expect(position(tester).pixels, 0); + + subtreeFetch!.complete(response([target])); + await tester.pumpAndSettle(); + expect(find.byType(FocusedThreadScreen), findsOneWidget); + + Navigator.of(tester.element(find.byType(FocusedThreadScreen))).pop(); + await tester.pumpAndSettle(); + expect(position(tester).pixels, 0); + }); + + testCachedVisit('target beyond max depth opens the fetched focused subtree', + (tester) async { + threads = [ + targetAtDepth(7), + ...List.generate(80, (index) => thread('filler-$index')), + ]; + await cacheScrolledVisit(tester); + subtreeFetch = Completer(); + await reopenFocused(tester); + + expect(requests.whereType(), contains('target')); + expect(position(tester).pixels, 0); + + subtreeFetch!.complete(response([target])); + await tester.pumpAndSettle(); + expect(find.byType(FocusedThreadScreen), findsOneWidget); + expect(find.text(targetContent).hitTestable(), findsOneWidget); + + Navigator.of(tester.element(find.byType(FocusedThreadScreen))).pop(); + await tester.pumpAndSettle(); + expect(position(tester).pixels, 0); + }); + + testCachedVisit('collapsed target fetch failure shows the existing snackbar', + (tester) async { + await cacheScrolledVisit(tester, collapsed: true); + failSubtree = true; + await reopenFocused(tester); + + expect(find.byType(FocusedThreadScreen), findsNothing); + expect(find.text(failureMessage), findsOneWidget); + expect(requests.whereType(), contains('target')); + }); +} -- 2.51.2