From 8885f033d5cd5e5acdeb5613a79a0fa7cba04dfd Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Wed, 9 Sep 2026 22:15:45 -0700 Subject: [PATCH 1/2] Fixes https://github.com/flutter/devtools/issues/4172 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. --- .../timeline_events_controller.dart | 7 +- .../performance/performance_model.dart | 5 +- .../import_export/import_export.dart | 20 ++- .../lib/src/shared/offline/offline_data.dart | 33 +++- .../release_notes/NEXT_RELEASE_NOTES.md | 7 +- .../performance/performance_model_test.dart | 21 +++ .../test/shared/import_export_test.dart | 39 +++++ .../shared/offline/offline_data_test.dart | 146 ++++++++++++++++++ 8 files changed, 263 insertions(+), 15 deletions(-) create mode 100644 packages/devtools_app/test/shared/offline/offline_data_test.dart diff --git a/packages/devtools_app/lib/src/screens/performance/panes/timeline_events/timeline_events_controller.dart b/packages/devtools_app/lib/src/screens/performance/panes/timeline_events/timeline_events_controller.dart index c37291140d6..4d1e8e1382c 100644 --- a/packages/devtools_app/lib/src/screens/performance/panes/timeline_events/timeline_events_controller.dart +++ b/packages/devtools_app/lib/src/screens/performance/panes/timeline_events/timeline_events_controller.dart @@ -498,11 +498,12 @@ class TimelineEventsController extends PerformanceFeatureController @override Future setOfflineData(OfflinePerformanceData offlineData) async { - if (offlineData.perfettoTraceBinary != null) { + if (offlineData.perfettoTraceBinary != null && + offlineData.perfettoTraceBinary!.isNotEmpty) { _updatePerfettoTrace(offlineData.perfettoTraceBinary!); + processTrackEvents(); + await loadPerfettoTrace(); } - processTrackEvents(); - await loadPerfettoTrace(); if (offlineData.selectedFrame != null) { perfettoController.scrollToTimeRange( diff --git a/packages/devtools_app/lib/src/screens/performance/performance_model.dart b/packages/devtools_app/lib/src/screens/performance/performance_model.dart index 89c310747ff..2b6059b98b6 100644 --- a/packages/devtools_app/lib/src/screens/performance/performance_model.dart +++ b/packages/devtools_app/lib/src/screens/performance/performance_model.dart @@ -65,7 +65,10 @@ class OfflinePerformanceData { /// tab they exported from. final int selectedTab; - bool get isEmpty => perfettoTraceBinary == null; + bool get isEmpty => + (perfettoTraceBinary == null || perfettoTraceBinary!.isEmpty) && + frames.isEmpty && + rebuildCountModel == null; Map toJson() => { traceBinaryKey: perfettoTraceBinary, diff --git a/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart b/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart index eed968af961..2c07da1e21c 100644 --- a/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart +++ b/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart @@ -69,6 +69,14 @@ class ImportController { final devToolsOfflineData = _DevToolsOfflineData(json); // TODO(kenz): support imports for more than one screen at a time. final activeScreenId = devToolsOfflineData.activeScreenId; + if (activeScreenId == null) { + notificationService.push( + 'The imported file is not a valid DevTools snapshot because it does ' + 'not contain an activeScreenId field.', + ); + return; + } + if (expectedScreenId != null && activeScreenId != expectedScreenId) { notificationService.push( 'Expected a data file for screen \'$expectedScreenId\' but received one' @@ -89,6 +97,14 @@ class ImportController { } } + if (!devToolsOfflineData.json.containsKey(activeScreenId) || + devToolsOfflineData.json[activeScreenId] == null) { + notificationService.push( + 'The imported file does not contain data for screen \'$activeScreenId\'.', + ); + return; + } + final connectedApp = OfflineConnectedApp.parse( devToolsOfflineData.connectedApp, ); @@ -106,8 +122,8 @@ extension type _DevToolsOfflineData(Map json) { return connectedApp == null ? {} : connectedApp.cast(); } - String get activeScreenId => - json[DevToolsExportKeys.activeScreenId.name] as String; + String? get activeScreenId => + json[DevToolsExportKeys.activeScreenId.name] as String?; } enum ExportFileType { diff --git a/packages/devtools_app/lib/src/shared/offline/offline_data.dart b/packages/devtools_app/lib/src/shared/offline/offline_data.dart index a7c6f48ff91..94b6af7a2fe 100644 --- a/packages/devtools_app/lib/src/shared/offline/offline_data.dart +++ b/packages/devtools_app/lib/src/shared/offline/offline_data.dart @@ -10,6 +10,7 @@ import 'dart:async'; import 'package:devtools_app_shared/service.dart'; import 'package:devtools_app_shared/utils.dart'; import 'package:flutter/foundation.dart'; +import 'package:logging/logging.dart'; import '../config_specific/import_export/import_export.dart'; import '../framework/routing.dart'; @@ -131,6 +132,8 @@ class OfflineDataController { /// ), /// } /// ``` +final _log = Logger('offline_data'); + mixin OfflineScreenControllerMixin on DevToolsScreenController, AutoDisposeControllerMixin { final _exportController = ExportController(); @@ -168,19 +171,33 @@ mixin OfflineScreenControllerMixin required FutureOr Function(T data) loadData, }) async { if (offlineDataController.shouldLoadOfflineData(screenId)) { - // TODO(kenz): investigate this line of code. Do we need to be creating a - // second copy of the Map from offlineDataController.offlineDataJson or - // can we use it directly to save this `Map.of` call? - final json = Map.of( - (offlineDataController.offlineDataJson[screenId] as Map) - .cast(), - ); - final screenData = createData(json); + final T screenData; + try { + // TODO(kenz): investigate this line of code. Do we need to be creating a + // second copy of the Map from offlineDataController.offlineDataJson or + // can we use it directly to save this `Map.of` call? + final json = Map.of( + (offlineDataController.offlineDataJson[screenId] as Map) + .cast(), + ); + screenData = createData(json); + } catch (e, st) { + _log.shout('Error parsing offline data for $screenId', e, st); + notificationService.push( + 'Failed to load offline data for screen \'$screenId\': $e', + ); + return false; + } + if (shouldLoad(screenData)) { _loadingOfflineData.value = true; await loadData(screenData); _loadingOfflineData.value = false; return true; + } else { + notificationService.push( + 'The imported file does not contain any data for screen \'$screenId\'.', + ); } } return false; diff --git a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md index 01ad0a7d9e2..792b22f8543 100644 --- a/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md +++ b/packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md @@ -18,6 +18,9 @@ To learn more about DevTools, check out the * Fixed unreadable text in the release notes panel, where blockquotes were drawn on a hard coded light blue background in the dark theme. [#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) ## Inspector updates @@ -25,7 +28,9 @@ TODO: Remove this section if there are not any updates. ## Performance updates -TODO: Remove this section if there are not any updates. +* Fixed an issue where importing performance data with Flutter frames but no + timeline trace would treat the data as empty. + [TODO](https://github.com/flutter/devtools/pull/TODO) ## CPU profiler updates diff --git a/packages/devtools_app/test/screens/performance/performance_model_test.dart b/packages/devtools_app/test/screens/performance/performance_model_test.dart index 7dade0fd72f..23a0cccb6a0 100644 --- a/packages/devtools_app/test/screens/performance/performance_model_test.dart +++ b/packages/devtools_app/test/screens/performance/performance_model_test.dart @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. +import 'dart:typed_data'; + import 'package:devtools_app/devtools_app.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -19,6 +21,25 @@ void main() { expect(offlineData.selectedTab, 0); }); + test('isEmpty', () { + expect(OfflinePerformanceData().isEmpty, isTrue); + expect( + OfflinePerformanceData(perfettoTraceBinary: Uint8List(0)).isEmpty, + isTrue, + ); + expect( + OfflinePerformanceData( + perfettoTraceBinary: Uint8List.fromList([1, 2, 3]), + ).isEmpty, + isFalse, + ); + expect(OfflinePerformanceData(frames: [testFrame0]).isEmpty, isFalse); + expect( + OfflinePerformanceData(rebuildCountModel: RebuildCountModel()).isEmpty, + isFalse, + ); + }); + test('init from parse', () { OfflinePerformanceData offlineData = OfflinePerformanceData.fromJson({}); expect(offlineData.frames, isEmpty); diff --git a/packages/devtools_app/test/shared/import_export_test.dart b/packages/devtools_app/test/shared/import_export_test.dart index 409531bfd3a..f52355c771f 100644 --- a/packages/devtools_app/test/shared/import_export_test.dart +++ b/packages/devtools_app/test/shared/import_export_test.dart @@ -88,6 +88,29 @@ void main() { equals(attemptingToImportMessage('example')), ); }); + + test('importData pushes notification when activeScreenId is missing', () { + importController.importData(devToolsFileJsonWithoutActiveScreenId); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + equals( + 'The imported file is not a valid DevTools snapshot because it does ' + 'not contain an activeScreenId field.', + ), + ); + }); + + test('importData pushes notification when screen data is missing', () { + importController.importData(devToolsFileJsonWithoutScreenData); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + equals( + 'The imported file does not contain data for screen \'example\'.', + ), + ); + }); }); } @@ -110,3 +133,19 @@ final devToolsFileJson = DevToolsJsonFile( 'example': {'title': 'example custom tools'}, }, ); +final devToolsFileJsonWithoutActiveScreenId = DevToolsJsonFile( + name: 'devToolsFileJsonWithoutActiveScreenId', + lastModifiedTime: DateTime.fromMicrosecondsSinceEpoch(3000), + data: { + 'devToolsSnapshot': true, + 'example': {'title': 'example custom tools'}, + }, +); +final devToolsFileJsonWithoutScreenData = DevToolsJsonFile( + name: 'devToolsFileJsonWithoutScreenData', + lastModifiedTime: DateTime.fromMicrosecondsSinceEpoch(4000), + data: { + 'devToolsSnapshot': true, + 'activeScreenId': 'example', + }, +); diff --git a/packages/devtools_app/test/shared/offline/offline_data_test.dart b/packages/devtools_app/test/shared/offline/offline_data_test.dart new file mode 100644 index 00000000000..38df026081e --- /dev/null +++ b/packages/devtools_app/test/shared/offline/offline_data_test.dart @@ -0,0 +1,146 @@ +// Copyright 2026 The Flutter Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd. + +import 'package:devtools_app/devtools_app.dart'; +import 'package:devtools_app/src/shared/config_specific/import_export/import_export.dart'; +import 'package:devtools_app_shared/utils.dart'; +import 'package:devtools_test/devtools_test.dart'; +import 'package:flutter_test/flutter_test.dart'; + +class _TestScreenController extends DevToolsScreenController + with + AutoDisposeControllerMixin, + OfflineScreenControllerMixin> { + Map? loadedData; + + @override + String get screenId => 'test_screen'; + + Future initOfflineData(String screenId, {bool shouldLoad = true}) { + return maybeLoadOfflineData( + screenId, + createData: (json) { + if (json.containsKey('throw')) { + throw Exception('Corrupted data'); + } + return json; + }, + shouldLoad: (data) => shouldLoad && data.isNotEmpty, + loadData: (data) { + loadedData = data; + }, + ); + } + + @override + OfflineScreenData prepareOfflineScreenData() => + OfflineScreenData(screenId: 'test_screen', data: loadedData ?? {}); +} + +void main() { + group('OfflineScreenControllerMixin', () { + late _TestScreenController controller; + late NotificationService notifications; + + setUp(() { + notifications = NotificationService(); + setGlobal(NotificationService, notifications); + setGlobal(OfflineDataController, OfflineDataController()); + setGlobal(ServiceConnectionManager, FakeServiceConnectionManager()); + controller = _TestScreenController(); + }); + + test('loads offline data when data is valid and non-empty', () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'test_screen', + 'test_screen': {'key': 'value'}, + }; + + final result = await controller.initOfflineData('test_screen'); + expect(result, isTrue); + expect(controller.loadedData, equals({'key': 'value'})); + expect(notifications.activeMessages, isEmpty); + }); + + test('notifies when screen data is empty', () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'test_screen', + 'test_screen': {}, + }; + + final result = await controller.initOfflineData('test_screen'); + expect(result, isFalse); + expect(controller.loadedData, isNull); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + equals( + 'The imported file does not contain any data for screen \'test_screen\'.', + ), + ); + }); + + test('notifies when shouldLoad returns false', () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'test_screen', + 'test_screen': {'key': 'value'}, + }; + + final result = await controller.initOfflineData( + 'test_screen', + shouldLoad: false, + ); + expect(result, isFalse); + expect(controller.loadedData, isNull); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + equals( + 'The imported file does not contain any data for screen \'test_screen\'.', + ), + ); + }); + + test('notifies when creating/loading data throws an error', () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'test_screen', + 'test_screen': {'throw': true}, + }; + + final result = await controller.initOfflineData('test_screen'); + expect(result, isFalse); + expect(controller.loadedData, isNull); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + contains('Failed to load offline data for screen \'test_screen\':'), + ); + }); + + test( + 'returns false without notifying if screen is not in offlineDataJson', + () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'other_screen', + 'other_screen': {'key': 'value'}, + }; + + final result = await controller.initOfflineData('test_screen'); + expect(result, isFalse); + expect(controller.loadedData, isNull); + expect(notifications.activeMessages, isEmpty); + }, + ); + }); +} From 58c83a4a0db0d2dd8c509d86c8bf556687252183 Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Thu, 10 Sep 2026 07:34:03 -0700 Subject: [PATCH 2/2] feedback --- .../import_export/import_export.dart | 6 ++-- .../lib/src/shared/offline/offline_data.dart | 14 ++++++-- .../test/shared/import_export_test.dart | 26 ++++++++++++++ .../shared/offline/offline_data_test.dart | 34 ++++++++++++++++++- 4 files changed, 74 insertions(+), 6 deletions(-) diff --git a/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart b/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart index 2c07da1e21c..2f3f0e949fa 100644 --- a/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart +++ b/packages/devtools_app/lib/src/shared/config_specific/import_export/import_export.dart @@ -122,8 +122,10 @@ extension type _DevToolsOfflineData(Map json) { return connectedApp == null ? {} : connectedApp.cast(); } - String? get activeScreenId => - json[DevToolsExportKeys.activeScreenId.name] as String?; + String? get activeScreenId { + final value = json[DevToolsExportKeys.activeScreenId.name]; + return value is String ? value : null; + } } enum ExportFileType { diff --git a/packages/devtools_app/lib/src/shared/offline/offline_data.dart b/packages/devtools_app/lib/src/shared/offline/offline_data.dart index 94b6af7a2fe..7b17521f6ba 100644 --- a/packages/devtools_app/lib/src/shared/offline/offline_data.dart +++ b/packages/devtools_app/lib/src/shared/offline/offline_data.dart @@ -191,9 +191,17 @@ mixin OfflineScreenControllerMixin if (shouldLoad(screenData)) { _loadingOfflineData.value = true; - await loadData(screenData); - _loadingOfflineData.value = false; - return true; + try { + await loadData(screenData); + return true; + } catch (e, st) { + _log.shout('Error loading offline data for $screenId', e, st); + notificationService.push( + 'Failed to load offline data for screen \'$screenId\': $e', + ); + } finally { + _loadingOfflineData.value = false; + } } else { notificationService.push( 'The imported file does not contain any data for screen \'$screenId\'.', diff --git a/packages/devtools_app/test/shared/import_export_test.dart b/packages/devtools_app/test/shared/import_export_test.dart index f52355c771f..c723bb89e42 100644 --- a/packages/devtools_app/test/shared/import_export_test.dart +++ b/packages/devtools_app/test/shared/import_export_test.dart @@ -101,6 +101,23 @@ void main() { ); }); + test( + 'importData pushes notification when activeScreenId is not a String', + () { + importController.importData( + devToolsFileJsonWithNonStringActiveScreenId, + ); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + equals( + 'The imported file is not a valid DevTools snapshot because it does ' + 'not contain an activeScreenId field.', + ), + ); + }, + ); + test('importData pushes notification when screen data is missing', () { importController.importData(devToolsFileJsonWithoutScreenData); expect(notifications.activeMessages.length, equals(1)); @@ -141,6 +158,15 @@ final devToolsFileJsonWithoutActiveScreenId = DevToolsJsonFile( 'example': {'title': 'example custom tools'}, }, ); +final devToolsFileJsonWithNonStringActiveScreenId = DevToolsJsonFile( + name: 'devToolsFileJsonWithNonStringActiveScreenId', + lastModifiedTime: DateTime.fromMicrosecondsSinceEpoch(3500), + data: { + 'devToolsSnapshot': true, + 'activeScreenId': 12345, + 'example': {'title': 'example custom tools'}, + }, +); final devToolsFileJsonWithoutScreenData = DevToolsJsonFile( name: 'devToolsFileJsonWithoutScreenData', lastModifiedTime: DateTime.fromMicrosecondsSinceEpoch(4000), diff --git a/packages/devtools_app/test/shared/offline/offline_data_test.dart b/packages/devtools_app/test/shared/offline/offline_data_test.dart index 38df026081e..d0f08a810ba 100644 --- a/packages/devtools_app/test/shared/offline/offline_data_test.dart +++ b/packages/devtools_app/test/shared/offline/offline_data_test.dart @@ -17,7 +17,11 @@ class _TestScreenController extends DevToolsScreenController @override String get screenId => 'test_screen'; - Future initOfflineData(String screenId, {bool shouldLoad = true}) { + Future initOfflineData( + String screenId, { + bool shouldLoad = true, + bool throwOnLoad = false, + }) { return maybeLoadOfflineData( screenId, createData: (json) { @@ -28,6 +32,9 @@ class _TestScreenController extends DevToolsScreenController }, shouldLoad: (data) => shouldLoad && data.isNotEmpty, loadData: (data) { + if (throwOnLoad) { + throw Exception('Failed during loadData'); + } loadedData = data; }, ); @@ -126,6 +133,31 @@ void main() { ); }); + test( + 'resets loadingOfflineData and notifies when loadData throws', + () async { + offlineDataController + ..startShowingOfflineData(offlineApp: MockConnectedApp()) + ..offlineDataJson = { + DevToolsExportKeys.activeScreenId.name: 'test_screen', + 'test_screen': {'key': 'value'}, + }; + + expect(controller.loadingOfflineData.value, isFalse); + final result = await controller.initOfflineData( + 'test_screen', + throwOnLoad: true, + ); + expect(result, isFalse); + expect(controller.loadingOfflineData.value, isFalse); + expect(notifications.activeMessages.length, equals(1)); + expect( + notifications.activeMessages.first.text, + contains('Failed to load offline data for screen \'test_screen\':'), + ); + }, + ); + test( 'returns false without notifying if screen is not in offlineDataJson', () async {