Repository navigation
Fix stale "no timeline event analysis data" message in Frame Analysis tab - #10050
ishaquehassan wants to merge 2 commits into
Conversation
… tab The selected frame is published before its timeline events are loaded. When the events have to be pulled from the VM they are assigned to the same FlutterFrame instance later, so selectedFrame never fires again and the Frame Analysis tab keeps showing the no-data message. Notify selectedFrame listeners again when events are assigned to the selected frame. Fixes flutter#9729
![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 pull request introduces a custom _SelectedFrameNotifier to trigger listener notifications when timeline event data is assigned to an already selected FlutterFrame, fixing a bug where the Frame Analysis tab would not update. It also adds a unit test and updates the release notes. The review feedback correctly identifies a potential crash if notifyListeners() is called after the notifier is disposed, and suggests adding a disposal guard.
| class _SelectedFrameNotifier extends ValueNotifier<FlutterFrame?> { | ||
| _SelectedFrameNotifier(super.value); | ||
|
|
||
| @override | ||
| // This override is needed to change visibility of the method. | ||
| void notifyListeners() { | ||
| super.notifyListeners(); | ||
| } | ||
| } |
There was a problem hiding this comment.
[MUST-FIX] Calling notifyListeners() on a disposed ValueNotifier will throw an assertion error or exception. Since assignEventToFrame can be called asynchronously (e.g., when a pending timeline poll completes after the controller has been disposed), we should guard notifyListeners() to ensure it is not called after dispose().
| class _SelectedFrameNotifier extends ValueNotifier<FlutterFrame?> { | |
| _SelectedFrameNotifier(super.value); | |
| @override | |
| // This override is needed to change visibility of the method. | |
| void notifyListeners() { | |
| super.notifyListeners(); | |
| } | |
| } | |
| class _SelectedFrameNotifier extends ValueNotifier<FlutterFrame?> { | |
| _SelectedFrameNotifier(super.value); | |
| bool _disposed = false; | |
| @override | |
| void dispose() { | |
| _disposed = true; | |
| super.dispose(); | |
| } | |
| @override | |
| // This override is needed to change visibility of the method. | |
| void notifyListeners() { | |
| if (!_disposed) { | |
| super.notifyListeners(); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where the Frame Analysis tab displays a "No timeline event analysis data available" message if a frame is selected before its timeline events are fetched. It introduces a custom _SelectedFrameNotifier to allow notifying listeners when events are assigned to an already selected frame, and adds corresponding unit tests. The review feedback suggests formatting the release notes entry to keep the PR link on the same line as the text and ensuring it points to the pull request rather than the issue.
| /// Timeline event data can be assigned to a [FlutterFrame] after the frame has | ||
| /// been selected, so listeners need to be notified even when the selected | ||
| /// [FlutterFrame] instance itself has not changed. | ||
| class _SelectedFrameNotifier extends ValueNotifier<FlutterFrame?> { |
There was a problem hiding this comment.
I don't understand why this class is introduced. It only contains a controller which passes the value to super, and overrides notifyListeners to call super. On the surface, it looks like a complete no-op from just using ValueNotifier<FlutterFrame?>.
There was a problem hiding this comment.
yeah fair, by itself it does look like a no-op. notifyListeners is @protected and @visibleForTesting on ChangeNotifier, so calling it on a plain ValueNotifier from the controller trips invalid_use_of_protected_member and analyze fails. the override was only there to re-expose it (same as ListValueNotifier), but agree that was too hidden, so i swapped it for a named notifyFrameDataChanged() and put the reason in the doc comment 👍
Replace the notifyListeners visibility override in _SelectedFrameNotifier with notifyFrameDataChanged, and document why the subclass is needed. Also link the release note entry to the PR.
Fixes #9729
The Frame Analysis tab renders
frame.frameAnalysis, which stays null until timeline event data has been assigned to thatFlutterFrame.FlutterFramesController.handleSelectedFramepublishes the selected frame right away and then loads the timeline events in the background, so when the events for the frame still have to be pulled from the VM, they land on the frame object after the tab has already been built. The frame instance itself never changes, soselectedFramedoes not fire again and the tab keeps showing "No timeline event analysis data available for this frame." Selecting a different frame and coming back changes the notifier value, the tab rebuilds, and the analysis shows up, which is why the message is gone on the second visit.This PR notifies
selectedFramelisteners again fromassignEventToFramewhen the events are being assigned to the frame that is currently selected. I put it at the assignment site rather than afterTimelineEventsController.handleSelectedFramecompletes, because that method'sprocessMoreEventsOrExitHelperis a fire-and-forgetvoid ... asynchelper, so its returned future resolves before the forced refresh has produced any events. To re-notify without changing the value,_selectedFrameNotifieris now a small privateValueNotifiersubclass with anotifyFrameDataChanged()method, becausenotifyListenersis@protectedand@visibleForTestingonChangeNotifierand calling it on a plainValueNotifierfrom the controller is an analyzer warning. The current listeners are the performance tab view and the frames chart (both justsetState) andRebuildStatsView(recomputes its metrics), so the extra notification only causes a rebuild with the same selection.The new test in
flutter_frames_controller_test.dartselects a frame that has no timeline events, checks thatframeAnalysisis null, then assigns a UI event and checks that the notifier fired once and the analysis is now available. It also checks that assigning an event to a frame that is not selected does not notify. With the controller change reverted, it fails withExpected: <1> Actual: <0>.flutter test test/screens/performance/passes 111/111, anddart analyzeanddart formatare clean on both changed Dart files.Pre-launch Checklist
General checklist
///).Issues checklist
contributions-welcomeorgood-first-issuelabel.contributions-welcomeorgood-first-issuelabel. I understand this means my PR might take longer to be reviewed.Tests checklist
AI-tooling checklist
Feature-change checklist
release-notes-not-requiredlabel or left a comment requesting the label be added.packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md.No screenshots since the layout doesn't change: the Frame Analysis tab already renders the analysis correctly once it rebuilds, the bug is only that the rebuild never happened. I haven't checked this against a running app, so for now the unit test above is what covers the behavior.
If you need help, consider asking for help on Discord.