From 2ad554fc6129d7e67698546594482d9c25dec93f Mon Sep 17 00:00:00 2001 From: Santo Shakil Date: Thu, 9 Jul 2026 01:13:32 +0600 Subject: [PATCH] fix(mobile): update timeline args on layout constraint changes (#29653) * fix(mobile): update timeline args on layout constraint changes * chore(mobile): temporary blank timeline diagnostics * chore(mobile): error logs when the timeline goes blank * chore(mobile): remove blank timeline debug logging --------- Co-authored-by: Alex --- .../widgets/timeline/timeline.state.dart | 11 +- .../widgets/timeline/timeline.widget.dart | 71 +++---- .../infrastructure/timeline.provider.dart | 8 +- .../widgets/timeline/timeline_args_test.dart | 190 ++++++++++++++++++ 4 files changed, 240 insertions(+), 40 deletions(-) create mode 100644 mobile/test/presentation/widgets/timeline/timeline_args_test.dart diff --git a/mobile/lib/presentation/widgets/timeline/timeline.state.dart b/mobile/lib/presentation/widgets/timeline/timeline.state.dart index 8dd87f9868..b39c431b43 100644 --- a/mobile/lib/presentation/widgets/timeline/timeline.state.dart +++ b/mobile/lib/presentation/widgets/timeline/timeline.state.dart @@ -86,13 +86,14 @@ class TimelineStateNotifier extends Notifier { // This provider watches the buckets from the timeline service & args and serves the segments. // It should be used only after the timeline service and timeline args provider is overridden final timelineSegmentProvider = StreamProvider.autoDispose>((ref) async* { - final args = ref.watch(timelineArgsProvider); - final columnCount = args.columnCount; - final spacing = args.spacing; - final availableTileWidth = args.maxWidth - (spacing * (columnCount - 1)); + // maxHeight is left out on purpose, a height-only change must not restart the bucket stream + final (maxWidth, columnCount, spacing, groupByArg) = ref.watch( + timelineArgsProvider.select((args) => (args.maxWidth, args.columnCount, args.spacing, args.groupBy)), + ); + final availableTileWidth = maxWidth - (spacing * (columnCount - 1)); final tileExtent = math.max(0, availableTileWidth) / columnCount; - final groupBy = args.groupBy ?? ref.watch(appConfigProvider.select((config) => config.timeline.groupAssetsBy)); + final groupBy = groupByArg ?? ref.watch(appConfigProvider.select((config) => config.timeline.groupAssetsBy)); final timelineService = ref.watch(timelineServiceProvider); yield* timelineService.watchBuckets().map((buckets) { diff --git a/mobile/lib/presentation/widgets/timeline/timeline.widget.dart b/mobile/lib/presentation/widgets/timeline/timeline.widget.dart index c13725ba4c..3f05edc2a4 100644 --- a/mobile/lib/presentation/widgets/timeline/timeline.widget.dart +++ b/mobile/lib/presentation/widgets/timeline/timeline.widget.dart @@ -29,7 +29,7 @@ import 'package:immich_mobile/widgets/common/immich_sliver_app_bar.dart'; import 'package:immich_mobile/widgets/common/mesmerizing_sliver_app_bar.dart'; import 'package:immich_mobile/widgets/common/selection_sliver_app_bar.dart'; -class Timeline extends StatelessWidget { +class Timeline extends ConsumerWidget { const Timeline({ super.key, this.topSliverWidget, @@ -62,35 +62,40 @@ class Timeline extends StatelessWidget { final Widget? loadingWidget; @override - Widget build(BuildContext context) { + Widget build(BuildContext context, WidgetRef ref) { + final columnCount = ref.watch(appConfigProvider.select((config) => config.timeline.tilesPerRow)); return LayoutBuilder( - builder: (_, constraints) => ProviderScope( - overrides: [ - timelineArgsProvider.overrideWith( - (ref) => TimelineArgs( - maxWidth: constraints.maxWidth, - maxHeight: constraints.maxHeight, - columnCount: ref.watch(appConfigProvider.select((config) => config.timeline.tilesPerRow)), - showStorageIndicator: showStorageIndicator, - withStack: withStack, - groupBy: groupBy, + builder: (_, constraints) { + return ProviderScope( + overrides: [ + // overrideWithValue keeps the scoped args in sync with the latest constraints on rebuilds, + // a function override would stay locked to the first frame's constraints for the whole session + timelineArgsProvider.overrideWithValue( + TimelineArgs( + maxWidth: constraints.maxWidth, + maxHeight: constraints.maxHeight, + columnCount: columnCount, + showStorageIndicator: showStorageIndicator, + withStack: withStack, + groupBy: groupBy, + ), ), + if (readOnly) readonlyModeProvider.overrideWith(() => _AlwaysReadOnlyNotifier()), + ], + child: _SliverTimeline( + topSliverWidget: topSliverWidget, + topSliverWidgetHeight: topSliverWidgetHeight, + bottomSliverWidget: bottomSliverWidget, + appBar: appBar, + bottomSheet: bottomSheet, + withScrubber: withScrubber, + persistentBottomBar: persistentBottomBar, + snapToMonth: snapToMonth, + maxWidth: constraints.maxWidth, + loadingWidget: loadingWidget, ), - if (readOnly) readonlyModeProvider.overrideWith(() => _AlwaysReadOnlyNotifier()), - ], - child: _SliverTimeline( - topSliverWidget: topSliverWidget, - topSliverWidgetHeight: topSliverWidgetHeight, - bottomSliverWidget: bottomSliverWidget, - appBar: appBar, - bottomSheet: bottomSheet, - withScrubber: withScrubber, - persistentBottomBar: persistentBottomBar, - snapToMonth: snapToMonth, - maxWidth: constraints.maxWidth, - loadingWidget: loadingWidget, - ), - ), + ); + }, ); } } @@ -169,13 +174,11 @@ class _SliverTimelineState extends ConsumerState<_SliverTimeline> with WidgetsBi void didUpdateWidget(covariant _SliverTimeline oldWidget) { super.didUpdateWidget(oldWidget); if (widget.maxWidth != oldWidget.maxWidth) { - final asyncSegments = ref.read(timelineSegmentProvider); - asyncSegments.whenData((segments) { - final index = _getCurrentAssetIndex(segments); - // Refresh to wait for new segments to be generated with the updated width before restoring the scroll position - final _ = ref.refresh(timelineArgsProvider); - _restoreAssetIndex = index; - }); + // The updated args already regenerate the segments, only remember the scroll position to restore it afterwards + final segments = ref.read(timelineSegmentProvider).valueOrNull; + if (segments != null && _scrollController.hasClients) { + _restoreAssetIndex = _getCurrentAssetIndex(segments); + } } } diff --git a/mobile/lib/providers/infrastructure/timeline.provider.dart b/mobile/lib/providers/infrastructure/timeline.provider.dart index b22c693033..a67b8dd822 100644 --- a/mobile/lib/providers/infrastructure/timeline.provider.dart +++ b/mobile/lib/providers/infrastructure/timeline.provider.dart @@ -1,3 +1,4 @@ +import 'package:collection/collection.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/domain/services/timeline.service.dart'; import 'package:immich_mobile/infrastructure/repositories/timeline.repository.dart'; @@ -39,5 +40,10 @@ final timelineUsersProvider = StreamProvider>((ref) { return Stream.value([]); } - return ref.watch(timelineRepositoryProvider).watchTimelineUserIds(currentUserId); + // Drift re-emits a fresh but content-identical list on unrelated table updates, + // which would dispose and rebuild the timeline service mid-load + return ref + .watch(timelineRepositoryProvider) + .watchTimelineUserIds(currentUserId) + .distinct(const ListEquality().equals); }); diff --git a/mobile/test/presentation/widgets/timeline/timeline_args_test.dart b/mobile/test/presentation/widgets/timeline/timeline_args_test.dart new file mode 100644 index 0000000000..4eb49bd3d6 --- /dev/null +++ b/mobile/test/presentation/widgets/timeline/timeline_args_test.dart @@ -0,0 +1,190 @@ +import 'dart:async'; + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:hooks_riverpod/hooks_riverpod.dart'; +import 'package:immich_mobile/domain/models/config/app_config.dart'; +import 'package:immich_mobile/domain/models/timeline.model.dart'; +import 'package:immich_mobile/domain/services/timeline.service.dart'; +import 'package:immich_mobile/presentation/widgets/timeline/timeline.state.dart'; +import 'package:immich_mobile/presentation/widgets/timeline/timeline.widget.dart'; +import 'package:immich_mobile/providers/infrastructure/settings.provider.dart'; +import 'package:immich_mobile/providers/infrastructure/timeline.provider.dart'; + +// A first fetch that never delivers - the state a suspended or storm-starved +// bucket watch is stuck in when the timeline mounts on a zero-sized first frame +class _FrozenBucketService implements TimelineService { + final _ctrl = StreamController>.broadcast(); + + @override + Stream> Function() get watchBuckets => + () => _ctrl.stream; + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +class _EmptyBucketService implements TimelineService { + @override + Stream> Function() get watchBuckets => + () => Stream.value(const []); + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +// Counts how many times the bucket stream is subscribed. Each subscription is a +// fresh photo query, so the count is our proxy for "did the relayout re-run the +// segment query for an input it does not use". +class _CountingBucketService implements TimelineService { + int watchCount = 0; + final _ctrl = StreamController>.broadcast(); + + @override + Stream> Function() get watchBuckets => () { + watchCount++; + return _ctrl.stream; + }; + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +void main() { + testWidgets('timeline args follow constraints after a zero-sized first frame while buckets are still loading', ( + tester, + ) async { + tester.view.physicalSize = Size.zero; + tester.view.devicePixelRatio = 3.0; + addTearDown(tester.view.reset); + + TimelineArgs? probed; + final probe = Consumer( + builder: (_, ref, __) { + probed = ref.watch(timelineArgsProvider); + return const SizedBox.shrink(); + }, + ); + + await tester.pumpWidget( + ProviderScope( + overrides: [ + timelineServiceProvider.overrideWithValue(_FrozenBucketService()), + appConfigProvider.overrideWithValue(const AppConfig()), + ], + child: MaterialApp(home: Timeline(withScrubber: false, readOnly: true, loadingWidget: probe)), + ), + ); + await tester.pump(); + + expect(probed, isNotNull); + expect(probed!.maxWidth, 0.0); + + tester.view.physicalSize = const Size(1206, 2622); + await tester.pump(); + await tester.pump(); + + expect( + probed!.maxWidth, + 402.0, + reason: 'args locked to the zero-sized first frame leave the timeline blank for the whole session', + ); + }); + + testWidgets('timeline args follow constraints after a zero-sized first frame once buckets resolve', (tester) async { + tester.view.physicalSize = Size.zero; + tester.view.devicePixelRatio = 3.0; + addTearDown(tester.view.reset); + + TimelineArgs? probed; + final probe = SliverToBoxAdapter( + child: Consumer( + builder: (_, ref, __) { + probed = ref.watch(timelineArgsProvider); + return const SizedBox.shrink(); + }, + ), + ); + + await tester.pumpWidget( + ProviderScope( + overrides: [ + timelineServiceProvider.overrideWithValue(_EmptyBucketService()), + appConfigProvider.overrideWithValue(const AppConfig()), + ], + child: MaterialApp( + home: Timeline( + withScrubber: false, + readOnly: true, + appBar: const SliverToBoxAdapter(child: SizedBox.shrink()), + topSliverWidget: probe, + ), + ), + ), + ); + await tester.pump(); + await tester.pump(); + + tester.view.physicalSize = const Size(1206, 2622); + await tester.pump(); + await tester.pump(); + await tester.pump(); + + expect(probed, isNotNull); + expect(probed!.maxWidth, 402.0); + }); + + testWidgets('a height-only relayout (multiselect app bar toggle) keeps the timeline, a width change refreshes it', ( + tester, + ) async { + final service = _CountingBucketService(); + tester.view.devicePixelRatio = 3.0; + tester.view.physicalSize = const Size(1206, 2622); + addTearDown(tester.view.reset); + + TimelineArgs? probed; + final probe = Consumer( + builder: (_, ref, __) { + probed = ref.watch(timelineArgsProvider); + return const SizedBox.shrink(); + }, + ); + + await tester.pumpWidget( + ProviderScope( + overrides: [ + timelineServiceProvider.overrideWithValue(service), + appConfigProvider.overrideWithValue(const AppConfig()), + ], + child: MaterialApp(home: Timeline(withScrubber: false, readOnly: true, loadingWidget: probe)), + ), + ); + await tester.pump(); + + final initialSubscriptions = service.watchCount; + final initialWidth = probed!.maxWidth; + final initialHeight = probed!.maxHeight; + expect(initialSubscriptions, greaterThan(0)); + + // toggling multiselect changes the app bar, so only the available height moves + tester.view.physicalSize = const Size(1206, 2000); + await tester.pump(); + await tester.pump(); + + expect(probed!.maxHeight, isNot(initialHeight), reason: 'the height should have actually changed'); + expect(probed!.maxWidth, initialWidth); + expect( + service.watchCount, + initialSubscriptions, + reason: 'a height-only change must not re-run the bucket query for an input the segments do not use', + ); + + // a real width change (rotation, fold, split screen) should refresh the tiles + tester.view.physicalSize = const Size(1000, 2000); + await tester.pump(); + await tester.pump(); + + expect(probed!.maxWidth, lessThan(initialWidth)); + expect(service.watchCount, greaterThan(initialSubscriptions)); + }); +}