[two_dimensional_scrollables] Activate leak testing and fix memory leaks by ValentinVignal · Pull Request #11653 · flutter/packages

ValentinVignal · 2026-05-06T09:32:04Z

The package manipulates disposable objects. This PR activates leak testing to make sure disposable objects are correctly disposed.

It also fixes the memory leak warnings from the tests

See the documentation: https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md

Pre-Review Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩²

ValentinVignal · 2026-05-06T09:32:19Z

cc @polina-c

ValentinVignal · 2026-05-06T09:32:54Z

+  static Widget builder({
+    Key? key,
+    bool? primary,


I'm not sure whether that is okay to change from a constructor to a static method. Would you have a better design in mind?

ValentinVignal · 2026-05-06T09:33:33Z

+        'merged column that exceeds metrics will assert',
+        // The build throws an assertion error which prevents the table from
+        // properly disposing the elements.
+        experimentalLeakTesting: LeakTesting.settings.withIgnoredAll(),


Is it acceptable to have memory leaks when the build asserts?

gemini-code-assist

Code Review

This pull request updates the two_dimensional_scrollables package to version 0.5.2, focusing on resolving memory leaks by refactoring TableView and TreeView to properly manage the lifecycle of their delegates and animations. It introduces leak_tracker_flutter_testing to the test suite and updates existing tests to ensure proper resource disposal. Review feedback identified several critical issues: missing parameter propagation in the new _TableViewBuilder and _TableViewList widgets, a missing didUpdateWidget implementation in _TableViewListState, and concerns regarding the breaking nature of converting constructors to static methods.

gemini-code-assist · 2026-05-06T09:39:21Z

-    super.dragStartBehavior,
-    super.keyboardDismissBehavior,
-    super.clipBehavior,
+  static Widget builder({


Changing TableView.builder from a constructor to a static method that returns a Widget is a breaking change. This changes the return type from TableView to Widget, which may break existing code that relies on the specific type or uses it in a context where a TableView instance is expected. Since this is a patch version bump (0.5.1 to 0.5.2), breaking changes should generally be avoided. Consider if a minor version bump is more appropriate or if there is a non-breaking way to manage the delegate's lifecycle.

ValentinVignal · 2026-05-06T10:14:02Z

                ),
              )
            : TableView.builder(
+                key: ValueKey(_selectionMode),


I'm doing this to force the widget to rebuild (and re-run _buildCell). Because now, when we toggle from "Single-cell" to "Disable" selection, the delegate doesn't change, and _buildCell is not rebuilt. That makes the test Selection SegmentedButton control works in packages/two_dimensional_scrollables/example/test/table_view/simple_table_test.dart:60 fail.

Ideally, I think we'd need some kind of notifier that the widget built in _buildCell would listen to. But since it is only an example, I'd thought this would be enough?

What do you think?

Will other users have to apply this change to their existing code? This makes me wonder if this is a breaking change.. might there be performance concerns as well?

I think I have an idea to mitigate this - instead of caching delegate parameters in TableView and conditionally updating the delegate in TreeView , we should simplify the lifecycle management to avoid correctness bugs (e.g., method tear-offs evaluating to == and blocking cell updates):

Constructors: Instantiate the delegates directly within the TableView constructors and track if they are internally managed with a final bool _isInternalDelegate flag.

Disposal: In _TableViewState.didUpdateWidget and dispose , simply check if _isInternalDelegate is true and dispose of the delegate.

Rebuilding: Remove the parameter caching classes (_TableCellDelegateParameters and subclasses) completely. Always recreate the delegates on rebuilds (including in _TreeViewState.didUpdateWidget ) to ensure parent state changes are correctly propagated to the viewport.

Revert the key: ValueKey(_selectionMode) workaround in the example application since parent updates will now propagate naturally.

WDYT?

I'm good with that. What do you think of Do not cache deleguate ? :)

Piinks · 2026-06-02T22:49:28Z

Hey @ValentinVignal thanks for this PR! Sorry I have not finished reviewing it yet. It looks like several checks are failing, can you take a look?

Piinks

Hi @ValentinVignal, thanks again for working on this! Fixing these memory leaks is super important, but I have some concerns about the breaking changes introduced in this PR:

Changing the constructors TableView.builder and TableView.list to static methods returning Widget (instead of TableView) is a breaking change. It breaks code that explicitly types variables as TableView (e.g., TableView table = TableView.builder(...)) and widget tests querying TableView by key.

Instead, I think we can make TableView itself a StatefulWidget that wraps a private stateless _TableViewWidget extending TwoDimensionalScrollView. The state class can manage the lifecycle of internally-created delegates, avoiding any breaking changes or type changes.

Since TreeView is already a StatefulWidget, we can manage the TreeRowBuilderDelegate directly inside the existing _TreeViewState instead of introducing the new private stateful _TreeView widget wrapper.

What do you think about going with this alternative? It allows us to land leak safety without breaking changes or class bloat.

ValentinVignal · 2026-06-08T10:44:40Z

@Piinks I updated the PR as per your suggestion. Let me know what you think about it :)

Piinks

Thanks for the updates @ValentinVignal! It looks like two things still need to be addressed:

Revert TableView.builder and TableView.list back to class constructors/factories returning TableView by making TableView itself a StatefulWidget.
Move TreeRowBuilderDelegate lifecycle management directly into _TreeViewState.

CI appears to be a bit unhappy as well.

ValentinVignal · 2026-06-24T08:17:59Z

Revert TableView.builder and TableView.list back to class constructors/factories returning TableView by making TableView itself a StatefulWidget.

Isn't it what I did already?

Move TreeRowBuilderDelegate lifecycle management directly into _TreeViewState.

Oops, sorry about that, I missed that one. This should be good not

CI appears to be a bit unhappy as well.

It seems like it is missing some python packages when trying to install the android emulators. Are those failure coming from the code changes of this PR ?

  Downloading https://us-python.pkg.dev/chrome-python-ar/chrome-python-ar/pyfakefs/pyfakefs-3.7.2-py3-none-any.whl (177 kB)
ERROR: Could not find a version that satisfies the requirement pylsqpack==0.3.12
ERROR: No matching distribution found for pylsqpack==0.3.12

If you see this, it means some python packages are missing from Artifact Registry or installation failed.
Please report this issue at https://crbug.com/492362903

ValentinVignal · 2026-08-20T08:45:30Z

@Piinks There are some merge conflicts if I rebase; I'm trying a merge first to see if it is enough :)

Piinks · 2026-08-20T15:09:09Z

@Piinks There are some merge conflicts if I rebase; I'm trying a merge first to see if it is enough :)

I understand. Unfortunately we need to rebase this because the base commit is old, which would prevent it from landing.

# Conflicts: # packages/two_dimensional_scrollables/lib/src/table_view/table.dart

# Conflicts: # packages/two_dimensional_scrollables/CHANGELOG.md

ValentinVignal · 2026-08-26T08:51:07Z

@Piinks I have rebased the branch :)

Piinks

Thank you so much! So excited for this to land!

…er#191965) flutter/packages@bd3cbc1...cd4cdd0 2026-08-28 21270878+elliette@users.noreply.github.com [material_ui] Add all M3 templates and generated code to `temporarily_excluded/` before migration (flutter/packages#12661) 2026-08-28 engine-flutter-autoroll@skia.org Roll Flutter from 15d8908 to e8dca90 (58 revisions) (flutter/packages#12660) 2026-08-28 bkonyi@google.com [various] Update pigeon dev_dependency to ^27.3.2 (flutter/packages#12615) 2026-08-27 269567208+reidbaker-agent@users.noreply.github.com [camera_android_camerax] Enforce CHANGELOG backticks and add eval commit author check to pre-push-skill (flutter/packages#12624) 2026-08-27 imcusg@gmail.com [material_ui] Prevent stale async suggestions in SearchAnchor (flutter/packages#12478) 2026-08-27 bkonyi@google.com [go_router_builder] Support analyzer 14 (flutter/packages#12614) 2026-08-27 srawlins@google.com [cupertino_ui] Use super parameters in more places (flutter/packages#12459) 2026-08-27 32538273+ValentinVignal@users.noreply.github.com [material_ui] Remove no-shuffle from progress indicator test (flutter/packages#12505) 2026-08-27 32538273+ValentinVignal@users.noreply.github.com [two_dimensional_scrollables] Activate leak testing and fix memory leaks (flutter/packages#11653) 2026-08-27 21270878+elliette@users.noreply.github.com [material_ui] Add helper methods in gen_defaults template (flutter/packages#12637) 2026-08-27 brackenavaron@gmail.com [cupertino_ui] fix CupertinoIcons font not being included in examples and fix TextEditingController leaks (flutter/packages#12228) 2026-08-27 269567208+reidbaker-agent@users.noreply.github.com [camera_android_camerax] Check Git hooks configuration in check-readiness skill (flutter/packages#12628) 2026-08-27 47866232+chunhtai@users.noreply.github.com [ci] sync back pr for branch release only run when release succeeds (flutter/packages#12581) 2026-08-27 stuartmorgan@google.com [google_maps_flutter] Convert overlay controllers to Swift (flutter/packages#12638) 2026-08-27 47866232+chunhtai@users.noreply.github.com [go_router_builder] Fixes text golden test to ignore platform specific newline (flutter/packages#12652) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…aks (flutter#11653) The package manipulates disposable objects. This PR activates leak testing to make sure disposable objects are correctly disposed. It also fixes the memory leak warnings from the tests See the documentation: https://github.com/dart-lang/leak_tracker/blob/main/doc%2Fleak_tracking%2FDETECT.md ## Pre-Review Checklist **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.

flutter-dashboard Bot added the CICD Run CI/CD label May 6, 2026

ValentinVignal requested a review from Piinks May 6, 2026 09:32

github-actions Bot added p: two_dimensional_scrollables Issues pertaining to the two_dimensional_scrollables package triage-framework Should be looked at in framework triage labels May 6, 2026

ValentinVignal commented May 6, 2026

View reviewed changes

gemini-code-assist Bot reviewed May 6, 2026

View reviewed changes

github-actions Bot removed the CICD Run CI/CD label May 6, 2026

ValentinVignal commented May 6, 2026

View reviewed changes

ValentinVignal force-pushed the tow-dimensional-scrollables/fix-memory-leaks branch from c3f33bf to 6741208 Compare May 12, 2026 08:34

flutter-dashboard Bot added the CICD Run CI/CD label May 12, 2026

ValentinVignal force-pushed the tow-dimensional-scrollables/fix-memory-leaks branch from 6741208 to 6d98ff2 Compare May 21, 2026 10:02

github-actions Bot removed the CICD Run CI/CD label May 21, 2026

ValentinVignal force-pushed the tow-dimensional-scrollables/fix-memory-leaks branch from 6d98ff2 to 97a29c0 Compare May 30, 2026 03:57

flutter-dashboard Bot added the CICD Run CI/CD label May 30, 2026

ValentinVignal force-pushed the tow-dimensional-scrollables/fix-memory-leaks branch from 97a29c0 to 5defacb Compare June 3, 2026 09:32

github-actions Bot removed the CICD Run CI/CD label Jun 3, 2026

flutter-dashboard Bot added the CICD Run CI/CD label Jun 4, 2026

github-actions Bot removed the CICD Run CI/CD label Jun 4, 2026

Piinks reviewed Jun 4, 2026

View reviewed changes

flutter-dashboard Bot added the CICD Run CI/CD label Jun 8, 2026

ValentinVignal requested a review from Piinks June 8, 2026 10:44

github-actions Bot removed the CICD Run CI/CD label Jun 19, 2026

flutter-dashboard Bot added the CICD Run CI/CD label Jun 22, 2026

Piinks requested changes Jun 23, 2026

View reviewed changes

ValentinVignal force-pushed the tow-dimensional-scrollables/fix-memory-leaks branch from f4d60fa to 9b7f1ce Compare June 24, 2026 08:17

github-actions Bot removed the CICD Run CI/CD label Jun 24, 2026

ValentinVignal added 16 commits August 26, 2026 16:06

Activate leak testing

1f52bf0

fix memory leaks

ba1cc6a

# Conflicts: # packages/two_dimensional_scrollables/lib/src/table_view/table.dart

Update tests

2671d1b

Update version number

ab10f86

# Conflicts: # packages/two_dimensional_scrollables/CHANGELOG.md

Add memory leak test in example

d6fe6e7

Gemini comments

04d87b9

Fix tests

dad6877

Fix more memory leaks in tests

844f858

Fix breaking changes

42787c3

Store delegate in _TreeViewState

f59512c

Clean code

5a95ac1

Do not cache deleguate

d9acf86

Fix version number

da02382

Always recreate _treeRowBuilderDeleguate

3b9e17d

Add a regression test

a6d314f

Delete copy paste extra

ad019f1

ValentinVignal force-pushed the tow-dimensional-scrollables/fix-memory-leaks branch from ac32d2c to ad019f1 Compare August 26, 2026 08:15

ValentinVignal added 2 commits August 26, 2026 16:15

Fix formating

b822679

Remove key

619ff14

Piinks approved these changes Aug 26, 2026

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2026

auto-submit Bot merged commit 93e477e into flutter:main Aug 27, 2026
13 checks passed

This was referenced Aug 28, 2026

Roll Packages from bd3cbc1b8c3f to 0bab4169c5ba (14 revisions) flutter/flutter#191961

Closed

Roll Packages from bd3cbc1b8c3f to cd4cdd0a044b (15 revisions) flutter/flutter#191965

Merged

Conversation

ValentinVignal commented May 6, 2026

Pre-Review Checklist

Footnotes

Uh oh!

ValentinVignal commented May 6, 2026

Uh oh!

ValentinVignal May 6, 2026

Choose a reason for hiding this comment

Uh oh!

ValentinVignal May 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot May 6, 2026

Choose a reason for hiding this comment

Uh oh!

ValentinVignal May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Piinks Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

Piinks Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

ValentinVignal Aug 3, 2026

Choose a reason for hiding this comment

Uh oh!

Piinks commented Jun 2, 2026

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

ValentinVignal commented Jun 8, 2026

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

ValentinVignal commented Jun 24, 2026

Uh oh!

ValentinVignal commented Aug 20, 2026

Uh oh!

Piinks commented Aug 20, 2026

Uh oh!

ValentinVignal commented Aug 26, 2026

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants