fix guard ScrollController with positions check #4029
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This review identified a potential memory leak due to an undisposed _scrollController and a critical race condition in the scrollDown method. These findings highlight important areas for improving the stability and performance of the application.
| void scrollDown() async { | ||
| if (_scrollController.hasClients) { | ||
| await Future.delayed(const Duration(milliseconds: 250)); | ||
| _scrollController.animateTo( | ||
| _scrollController.position.maxScrollExtent, | ||
| duration: const Duration(milliseconds: 200), | ||
| curve: Curves.easeOut, | ||
| ); | ||
| } |
There was a problem hiding this comment.
There is a race condition here. The _scrollController.hasClients check is performed before an await. After the Future.delayed completes, the widget might have been disposed, and _scrollController might no longer have clients. Accessing _scrollController.position would then throw an exception. The checks for mounted and hasClients (or positions.isNotEmpty) should be performed after any await calls to ensure the state is still valid before using the controller.
| void dispose() { | ||
| _scrollController.dispose(); | ||
| _questionAnimationController.dispose(); | ||
| super.dispose(); | ||
| } |
There was a problem hiding this comment.
The _scrollController is not being disposed of in the dispose method. This can lead to memory leaks. It should be disposed of by calling _scrollController.dispose() to release its resources when the State object is removed from the tree permanently.
|
so _scrollController.hasClients does not work? |
hasClients only checks that the controller is attached at that moment. Since scrollDown waits before running, the list can detach in between so accessing position will crash |
omi/app/lib/pages/speech_profile/page.dart
Line 85 in ef7ad36
ScrollController had no attached position because the ListView was not built yet, that’s whyu position became invalid
closes #4023