Fix performance when JSON is invalid - #10005
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 pull request improves the robustness of offline data importing in DevTools by adding validation checks, error handling, and user-facing notifications for missing or malformed import data. It also fixes an issue where performance data with Flutter frames but no timeline trace was incorrectly treated as empty. The review feedback highlights two important improvements: safely checking the type of activeScreenId to avoid runtime TypeErrors, and wrapping the offline data loading call in a try-finally block to prevent the UI from getting stuck in a loading state if an exception occurs.
srawlins
changed the title
1. **Fixed performance_model.dart:68-71**: * Updated isEmpty to verify that there is no trace binary, no frames, and no rebuild model. 2. **Guarded Perfetto trace loading in timeline_events_controller.dart:501-506**: * Avoided attempting to process track events or load a trace into Perfetto when perfettoTraceBinary is null or empty, allowing offline Flutter frames to display cleanly. 3. **Added validation in import_export.dart:72-106**: * Added null-safety check for activeScreenId with user-facing notification if missing from the snapshot. * Added a check verifying that the JSON contains data for activeScreenId (json.containsKey(activeScreenId) && json[activeScreenId] != null), preventing navigation to a broken snapshot screen and immediately notifying the user. 4. **Added error handling and notifications in offline_data.dart:173-200**: * Pushes a notification if the file data for that screen is empty (!shouldLoad(screenData)). * Wraps deserialization and loading in a try / catch block to log and display an error message if parsing fails. 5. Tests & Release Notes: * Added tests in import_export_test.dart for missing activeScreenId and missing screen data. * Added unit tests in performance_model_test.dart for OfflinePerformanceData.isEmpty. * Created offline_data_test.dart testing OfflineScreenControllerMixin loading, empty payload notifications, and error handling. * Documented the changes in NEXT_RELEASE_NOTES.md.
2c75337 to
58c83a4
Compare
| if (offlineData.perfettoTraceBinary != null && | ||
| offlineData.perfettoTraceBinary!.isNotEmpty) { | ||
| _updatePerfettoTrace(offlineData.perfettoTraceBinary!); |
There was a problem hiding this comment.
nit: we can avoid two null assertions by putting this in a var.
| if (offlineData.perfettoTraceBinary != null && | |
| offlineData.perfettoTraceBinary!.isNotEmpty) { | |
| _updatePerfettoTrace(offlineData.perfettoTraceBinary!); | |
| final perfettoTraceBinary = offlineData.perfettoTraceBinary; | |
| if (perfettoTraceBinary != null && perfettoTraceBinary.isNotEmpty) { | |
| _updatePerfettoTrace(perfettoTraceBinary); |
| /// ), | ||
| /// } | ||
| /// ``` | ||
| final _log = Logger('offline_data'); |
There was a problem hiding this comment.
All of the above dartdoc goes with OfflineScreenControllerMixin, so this _log needs to move above the dartdoc
| [#9957](https://github.com/flutter/devtools/pull/9957) | ||
| * Added user-facing error notifications when importing data files that are | ||
| missing required fields or contain no data for the screen. | ||
| [TODO](https://github.com/flutter/devtools/pull/TODO) |
There was a problem hiding this comment.
update this link and the one below
| notificationService.push( | ||
| 'The imported file does not contain any data for screen \'$screenId\'.', |
There was a problem hiding this comment.
if shouldLoad is true, does this always mean that the data was empty? can a screen return false for some other reason than the data being empty?
kenzieschmoll
left a comment
kenzieschmoll
left a comment
There was a problem hiding this comment.
some comments but overall lgtm
2 participants
Fixes #4172
Fixed
performance_model.dart:Guarded Perfetto trace loading in
timeline_events_controller.dart:perfettoTraceBinaryis null or empty, allowing offline Flutter frames to display cleanly.Added validation in
import_export.dart:activeScreenIdwith user-facing notification if missing from the snapshot.activeScreenId(json.containsKey(activeScreenId) && json[activeScreenId] != null), preventing navigation to a broken snapshot screen and immediately notifying the user.Added error handling and notifications in
offline_data.dart:!shouldLoad(screenData)).Tests & Release Notes:
import_export_test.dartfor missingactiveScreenIdand missing screen data.performance_model_test.dartforOfflinePerformanceData.isEmpty.offline_data_test.darttestingOfflineScreenControllerMixinloading, empty payload notifications, and error handling.NEXT_RELEASE_NOTES.md.