Skip to content

Commit 31f6edd

Browse files
authored
Fix AnimatedList.separated assert when removing last item mid-removal… (#186389)
## Summary `AnimatedList.separated.removeItem` used the sliver's total child count (`_itemsCount`) to decide whether the just-removed item was the last item in the list. That count still includes children animating out from earlier `removeItem` calls, so while a previous removal was in flight the new tail of the visible list was misclassified as a middle item. The separator removal then targeted the wrong index, and once the item being removed was added to `_outgoingItems` the next `_indexToItemIndex` lookup ran past `_itemsCount` and tripped the `itemIndex >= 0 && itemIndex < _itemsCount` assertion inside `_SliverAnimatedMultiBoxAdaptorState.removeItem`. The fix captures `_itemsCount - _outgoingItems.length` before mutating the sliver and uses that visible count for both the "are there enough items to have a separator?" and "is this the new last item?" checks. When no removal is in flight `_outgoingItems` is empty and behavior is unchanged, so existing `.separated` coverage and the `removeAllItems` regression test from #176452 continue to pass. ## Related issues Fixes #186361. Related prior fix for the non-separated `removeAllItems` variant: #176452 (issue #176362). ## Test plan - [x] Added a regression test in `packages/flutter/test/widgets/animated_list_test.dart` that reproduces the assertion on master and passes with this change: starts a 10s removal on a non-last item, pumps part of the animation, then removes the new last item, and asserts no exception is thrown. - [x] `flutter test packages/flutter/test/widgets/animated_list_test.dart packages/flutter/test/widgets/animated_grid_test.dart` — 34 tests pass, including the existing comprehensive `AnimatedList.separated` test and the `removeAllItems` regression test from #176452. - [x] `dart analyze` on the touched files: no issues. - [x] Manually verified with the reproducer from the issue: before the fix the assertion fires in the debug console; after the fix the second removal animates out cleanly. ## AI disclosure Written with assistance from Claude Code (Anthropic). I reviewed every change, ran the tests, and verified the fix manually before submitting. ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
1 parent d95bae8 commit 31f6edd

2 files changed

Lines changed: 83 additions & 2 deletions

File tree

‎packages/flutter/lib/src/widgets/animated_scroll_view.dart‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -749,10 +749,14 @@ abstract class _AnimatedScrollViewState<T extends _AnimatedScrollView> extends S
749749
_sliverAnimatedMultiBoxKey.currentState!.removeItem(index, builder, duration: duration);
750750
} else {
751751
final int itemIndex = _computeItemIndex(index);
752+
// Children animating out from earlier removeItem calls still count
753+
// towards [_itemsCount], so subtract them to recognize the new tail
754+
// of the visible list while a previous removal is still in flight.
755+
final int visibleItemsCount = _itemsCount - _outgoingItemsCount;
752756
// Remove the item
753757
_sliverAnimatedMultiBoxKey.currentState!.removeItem(itemIndex, builder, duration: duration);
754-
if (_itemsCount > 1) {
755-
if (itemIndex == _itemsCount - 1) {
758+
if (visibleItemsCount > 1) {
759+
if (itemIndex == visibleItemsCount - 1) {
756760
// The item was removed from the end of the list, so the separator to remove is the one at `last index` - 1.
757761
_sliverAnimatedMultiBoxKey.currentState!.removeItem(
758762
itemIndex - 1,
@@ -816,6 +820,8 @@ abstract class _AnimatedScrollViewState<T extends _AnimatedScrollView> extends S
816820

817821
int get _itemsCount => _sliverAnimatedMultiBoxKey.currentState!._itemsCount;
818822

823+
int get _outgoingItemsCount => _sliverAnimatedMultiBoxKey.currentState!._outgoingItems.length;
824+
819825
// Helper method to compute the index for the item to insert or remove considering the separators in between.
820826
int _computeItemIndex(int index) {
821827
if (index == 0) {

‎packages/flutter/test/widgets/animated_list_test.dart‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1204,6 +1204,81 @@ void main() {
12041204
expect(itemsSeparatorsTexts[2].data, 'item 1');
12051205
});
12061206

1207+
// Regression test for https://github.com/flutter/flutter/issues/186361
1208+
testWidgets(
1209+
'AnimatedList.separated can remove the last item while another item is still animating out',
1210+
(WidgetTester tester) async {
1211+
Widget itemBuilder(BuildContext context, int index, Animation<double> animation) {
1212+
return SizedBox(height: 100.0, child: Center(child: Text('item $index')));
1213+
}
1214+
1215+
Widget separatorBuilder(BuildContext context, int index, Animation<double> animation) {
1216+
return SizedBox(height: 10.0, child: Center(child: Text('separator $index')));
1217+
}
1218+
1219+
Widget removedSeparatorBuilder(BuildContext context, int index, Animation<double> animation) {
1220+
return SizedBox(height: 10.0, child: Center(child: Text('removing separator $index')));
1221+
}
1222+
1223+
AnimatedRemovedItemBuilder removedItemBuilder(int index) {
1224+
return (BuildContext context, Animation<double> animation) {
1225+
return SizedBox(height: 100.0, child: Center(child: Text('removing item $index')));
1226+
};
1227+
}
1228+
1229+
final listKey = GlobalKey<AnimatedListState>();
1230+
1231+
await tester.pumpWidget(
1232+
Directionality(
1233+
textDirection: TextDirection.ltr,
1234+
child: AnimatedList.separated(
1235+
key: listKey,
1236+
initialItemCount: 3,
1237+
itemBuilder: itemBuilder,
1238+
separatorBuilder: separatorBuilder,
1239+
removedSeparatorBuilder: removedSeparatorBuilder,
1240+
),
1241+
),
1242+
);
1243+
1244+
// Start removing a non-last item (index 0) with a long animation.
1245+
listKey.currentState!.removeItem(
1246+
0,
1247+
removedItemBuilder(0),
1248+
duration: const Duration(seconds: 10),
1249+
);
1250+
await tester.pump(const Duration(milliseconds: 50));
1251+
1252+
// While the first removal is still animating, remove the new last item.
1253+
// Before the fix this triggered an "itemIndex >= 0 && itemIndex < _itemsCount"
1254+
// assertion inside _SliverAnimatedMultiBoxAdaptorState.removeItem.
1255+
listKey.currentState!.removeItem(
1256+
1,
1257+
removedItemBuilder(2),
1258+
duration: const Duration(milliseconds: 100),
1259+
);
1260+
await tester.pump();
1261+
1262+
expect(tester.takeException(), isNull);
1263+
expect(find.text('removing item 0'), findsOneWidget);
1264+
expect(find.text('removing item 2'), findsOneWidget);
1265+
// The surviving item is the originally-middle item. The itemBuilder
1266+
// rebuilds with its current user-facing index, which is 0 now.
1267+
expect(find.text('item 0'), findsOneWidget);
1268+
expect(find.text('item 1'), findsNothing);
1269+
expect(find.text('item 2'), findsNothing);
1270+
1271+
await tester.pumpAndSettle();
1272+
1273+
// All removal animations finished and the surviving item is alone.
1274+
expect(find.text('removing item 0'), findsNothing);
1275+
expect(find.text('removing item 2'), findsNothing);
1276+
expect(find.text('item 0'), findsOneWidget);
1277+
expect(find.text('item 1'), findsNothing);
1278+
expect(find.text('item 2'), findsNothing);
1279+
},
1280+
);
1281+
12071282
testWidgets('AnimatedList does not crash at zero area', (WidgetTester tester) async {
12081283
tester.view.physicalSize = Size.zero;
12091284
final controller = ScrollController();

0 commit comments

Comments
 (0)