Offline support for Network page by hrajwade96 · Pull Request #8332 · flutter/devtools

hrajwade96 · 2024-09-21T09:19:46Z

Adding offline support to Network page.

issue link - #4470

List which issues are fixed by this PR.

Please add a note to packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md if your change requires release notes. Otherwise, add the 'release-notes-not-required' label to the PR.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read the Flutter Style Guide recently, and have followed its advice.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or there is a reason for not adding tests.

If you need help, consider asking for help on Discord.

This reverts commit 0c1f414.

kenzieschmoll · 2024-09-30T21:30:15Z

@hrajwade96 is this PR ready for review or is this still a work in progress?

hrajwade96 · 2024-10-01T17:13:00Z

@hrajwade96 is this PR ready for review or is this still a work in progress?

@kenzieschmoll just finishing up few things, I will mark it ready soon

…into network_screen_offline_support

kenzieschmoll · 2025-01-21T17:29:26Z

Sure, so as per your next comment I think you are suggesting to enable the review history (with this PR) and add another flag which gives two options to choose the format for export, and also an import button (in the next PR).

Yes you are correct sorry for the confusion. I meant to update the comment about the flag with my edit. For the next PR (adding the buttons behind a flag), I will make that change since I already have a PR prepared. I'll leave the flag off, and then you can make your change to support full offline support for the network page, which requires exporting the data to a JSON file so that it can be reloaded into DevTools in the existing offline mechanism. The majority of the work for that is done in this PR, but the final piece is supporting the full serialization / deserialization to / from JSON.

Could you elaborate on the key idea behind adding the option to export it as a '.json' file?

This is a requirement of adding full offline support for the Network page (where you can export a file and load it back into DevTools later). The offline framework in DevTools serializes to json and loads files from json.

hrajwade96 · 2025-01-25T08:29:06Z

Yes you are correct sorry for the confusion. I meant to update the comment about the flag with my edit. For the next PR (adding the buttons behind a flag), I will make that change since I already have a PR prepared. I'll leave the flag off, and then you can make your change to support full offline support for the network page, which requires exporting the data to a JSON file so that it can be reloaded into DevTools in the existing offline mechanism. The majority of the work for that is done in this PR, but the final piece is supporting the full serialization / deserialization to / from JSON.

Oh, okay, no problem. Thanks! I had the idea before, but this made it much clearer.

This is a requirement of adding full offline support for the Network page (where you can export a file and load it back into DevTools later). The offline framework in DevTools serializes to json and loads files from json.

I thought we are going to add support to re-import the .har file which we can download (like #4470 (comment)). Is it like har and json both formats will be supported for importing? Or now are you thinking to add support for only json for importing back.

kenzieschmoll · 2025-01-27T17:00:46Z

I thought we are going to add support to re-import the .har file which we can download (like #4470 (comment)). Is it like har and json both formats will be supported for importing? Or now are you thinking to add support for only json for importing back.

From the work in this PR, adding support for re-importing as JSON seems like it is 90% there. Is that correct? If I understand correctly there is not much work left to support the full JSON serialization / deserialization. If this is almost complete, then I think we should push this across the finish line. This is also good for consistency with how offline data is supported for other DevTools screens.

For deserializing the .har files, this work has not started yet, right? At this point, it seems like this would take more work to support, and if the JSON serialization / deserialization is robust and well-tested, then it may not make sense to support importing both types of files.

Bringing my comment from #4470 to this thread for reference:

The more complicated part for the Network page is the fact that we support exporting as a .har file, whereas the offline framework for DevTools only supports importing & exporting to / from a JSON file. So in the current form of the offline framework, if network data is exported as a .har file, it will not be able to be re-imported back into DevTools. Only data exported as a JSON file through the DevTools offline framework can be re-imported back into DevTools.

We can tweak the logic to support loading the .har file for the network page as a special case, but we can do this as a follow up task to adding the full offline support for the Network page.

To re-state in the context of this PR, we could consider adding support for importing the .har file to DevTools as a follow up. However, we may be limited if the .har format doesn't allow us to add metadata that we would have in the JSON format (things like filters or other data specific to the DevTools network profiler), so it may not be worth doing.

As a user of this tool, I see the user journey as such:

If I want to export network data for viewing later in Flutter DevTools, then I should download as a JSON file.
If I want to export network data for viewing in some other tool that supports the .har format, then I should download the .har file.

hrajwade96 · 2025-01-28T15:57:17Z

From the work in this PR, adding support for re-importing as JSON seems like it is 90% there. Is that correct? If I understand correctly there is not much work left to support the full JSON serialization / deserialization. If this is almost complete, then I think we should push this across the finish line. This is also good for consistency with how offline data is supported for other DevTools screens.

Yes correct, I added the remaining piece (disconnected network body) in this PR itself , now we don't need another PR for this. (note: I haven't added OpenSaveButtonGroup for import/export cta since you would be raising a PR with new UI for that, also btw we need to add a call to_fetchFullDataBeforeExport before the json is downloaded just like in case of har).

For deserializing the .har files, this work has not started yet, right? At this point, it seems like this would take more work to support, and if the JSON serialization / deserialization is robust and well-tested, then it may not make sense to support importing both types of files.
Yes not yet started,

Bringing my comment from #4470 to this thread for reference:

The more complicated part for the Network page is the fact that we support exporting as a .har file, whereas the offline framework for DevTools only supports importing & exporting to / from a JSON file. So in the current form of the offline framework, if network data is exported as a .har file, it will not be able to be re-imported back into DevTools. Only data exported as a JSON file through the DevTools offline framework can be re-imported back into DevTools.
We can tweak the logic to support loading the .har file for the network page as a special case, but we can do this as a follow up task to adding the full offline support for the Network page.

To re-state in the context of this PR, we could consider adding support for importing the .har file to DevTools as a follow up. However, we may be limited if the .har format doesn't allow us to add metadata that we would have in the JSON format (things like filters or other data specific to the DevTools network profiler), so it may not be worth doing.

Ahh ok, at that time I interpreted this comment a little differently regarding the follow-up PR on re-importing .har

Hmm, After completing this PR, I'll analyze the approach to add this support and get back to you. Once I've done that, we can discuss whether it's worth pursuing. Does that work?

As a user of this tool, I see the user journey as such:

If I want to export network data for viewing later in Flutter DevTools, then I should download as a JSON file.

If I want to export network data for viewing in some other tool that supports the .har format, then I should download the .har file.

Hmm, yes got it, thanks

kenzieschmoll · 2025-01-28T19:36:08Z

  }
 }

+class DisconnectedNetworkScreenBody extends StatelessWidget {


did you mean to include these changes in this PR? Have you verified that the network data can be exported to JSON and that it can be reloaded into DevTools? If not, we should not add this code yet.

Actually I did not originally intend to include these changes, but thought to add it as it's relatively not a big change.
I had already verified the happy journey (attaching videos), I am fixing one issue in the response data. I will throughly test it and then let you know, otherwise I can raise it as a separate PR as well.

Screen.Recording.2025-01-29.at.10.55.14.PM.mov

clideo_editor_baefe568f22145b2b04a299b8fd94832.mp4

Thank you for the videos, this looks great!

One more comment. You said:

also btw we need to add a call to_fetchFullDataBeforeExport before the json is downloaded just like in case of har

Can that be done as part of this PR?

Thank you for the videos, this looks great!

Yup :)

One more comment. You said:

also btw we need to add a call to_fetchFullDataBeforeExport before the json is downloaded just like in case of har

Can that be done as part of this PR?

Yes, but then, I had added it in the OpenSaveButtonGroup, which is not a part of this PR, as you would be adding the dropdown in a separate one.
(Note: This function was marked private before make it public)

OpenSaveButtonGroup( screenId: ScreenMetaData.network.id, onSave: () async { await widget.controller.fetchFullDataBeforeExport(); widget.controller.exportData(); }, ),

Okay I can apply that change to my follow up PR, thanks for the code pointer.

kenzieschmoll · 2025-01-30T20:05:06Z

Can you merge with the master branch? The benchmark size threshold was increased recently.

kenzieschmoll · 2025-01-31T20:24:08Z

Looks like a legitimate error in the performance benchmarks:

benchmark/devtools_benchmarks_test.dart 43:15  main.<fn>
  Caught browser-side error: Null check operator used on a null value
      at module0.NetworkController._recordingNetworkTraffic inner (http://localhost:9999/main.dart.wasm:wasm-function[19792]:0x3c6bf2)
      at module0.NetworkController._recordingNetworkTraffic (http://localhost:9999/main.dart.wasm:wasm-function[19647]:0x3c295e)
      at module0.NetworkController.startRecording inner (http://localhost:9999/main.dart.wasm:wasm-function[19645]:0x3c27d3)
      at module0.NetworkController._initHelper inner (http://localhost:9999/main.dart.wasm:wasm-function[19637]:0x3c2665)
      at module0.defaultScreens closure at file:///Users/runner/work/devtools/devtools/packages/devtools_app/lib/src/app.dart:662:25 (http://localhost:9999/main.dart.wasm:wasm-function[18664]:0x3a6bf1)
      at module0.closure wrapper at file:///Users/runner/work/devtools/devtools/packages/devtools_app/lib/src/app.dart:662:25 trampoline (http://localhost:9999/main.dart.wasm:wasm-function[18683]:0x3a746e)
      at module0.DevToolsScreen.controllerProvider closure at file:///Users/runner/work/devtools/devtools/packages/devtools_app/lib/src/app.dart:544:15 (http://localhost:9999/main.dart.wasm:wasm-function[26423]:0x45eebc)
      at module0._CreateInheritedProviderState.value (http://localhost:9999/main.dart.wasm:wasm-function[26414]:0x45ec90)
  dart:async                                     _Completer.completeError
  package:web_benchmarks/src/runner.dart 254:25  BenchmarkServer.run.<fn>

hrajwade96 · 2025-02-07T18:36:44Z

Looks like a legitimate error in the performance benchmarks:

Sorry for late response got occupied with something.
I think it was trying to record as offlineDataController.showingOfflineData.value was coming as false, I have pushed one check for this.
Might need to add 2 more checks at other places checking it locally will push it if working.

…into network_screen_offline_support

…in controller, added a util function isConnected()

hrajwade96 · 2025-02-09T10:19:03Z

Looks like a legitimate error in the performance benchmarks:

Sorry for late response got occupied with something. I think it was trying to record as offlineDataController.showingOfflineData.value was coming as false, I have pushed one check for this. Might need to add 2 more checks at other places checking it locally will push it if working.

I have added the necessary handling wherever service connection was being used. Please run the checks once.

kenzieschmoll · 2025-02-10T19:47:16Z


  void _initHelper() async {
-    if (offlineDataController.showingOfflineData.value) {
+    if (!isConnected() || offlineDataController.showingOfflineData.value) {


we do not want the check here for !isConnected(). It is possible for offline data to be showing even when an existing DevTools app connection exists. Please remove.

It is possible for offline data to be showing even when an existing DevTools app connection exists. Please remove.

Yes, actually it would show even when a connection exists as it's added with an OR followed by another check. I had added it to prevent recording in disconnected state and show offline data if available even in connected mode) as it was giving below exception.
But now I realise I should instead wrap
await startRecording(); with a If to only execute when connected, as offlineDataController.showingOfflineData.value is the correct check to show offline data, even in disconnected mode.

Uncaught (in promise) DartError: Unexpected null value. at Object.throw_ [as throw] (errors.dart:307:3) at Object.nullCheck (profile.dart:117:39) at network_controller.dart:351:61 at async_patch.dart:622:19 at async_patch.dart:647:23 at Object._asyncStartSync (async_patch.dart:541:3) at [_recordingNetworkTraffic] (network_controller.dart:347:16) at network_controller.dart:269:35 at async_patch.dart:622:19 at async_patch.dart:647:23 at Object._asyncStartSync (async_patch.dart:541:3) at network_controller.NetworkController.new.startRecording (network_controller.dart:267:16) at network_controller.dart:185:13 at async_patch.dart:622:19 at async_patch.dart:647:23 at Object._asyncStartSync (async_patch.dart:541:3) at [_initHelper] (network_controller.dart:173:8) at new network_controller.NetworkController.new (network_controller.dart:56:5) at app.dart:662:32 at app.dart:545:28...

Hence now I will write it as :

if (offlineDataController.showingOfflineData.value) { await maybeLoadOfflineData( NetworkScreen.id, createData: (json) => OfflineNetworkData.fromJson(json), // This ignore is used because the 'data' parameter can have a dynamic type, // which cannot be explicitly typed here due to its dependency on JSON parsing. // ignore: avoid_dynamic_calls shouldLoad: (data) => !data.isEmpty, loadData: (data) => loadOfflineData(data), ); } if(serviceConnection.serviceManager.connectedState.value.connected){ await startRecording(); }

Note: I am using second if here instead of using else-if intentionally assuming it should still record the traffic in the background in a scenario where connection exists but user is viewing offline data by importing json into Devtools.

Let me know if this looks correct.

… with Object?

added initial implementation

fe11d5b

hrajwade96 requested review from a team, bkonyi and kenzieschmoll as code owners September 21, 2024 09:19

hrajwade96 marked this pull request as draft September 21, 2024 09:19

hrajwade96 changed the title ~~added initial implementation~~ Offline support for Network page Sep 21, 2024

hrajwade96 added 3 commits September 29, 2024 18:20

adding constants file

6c140b4

updated NEXT_RELEASE_NOTES.md

0c1f414

Revert "updated NEXT_RELEASE_NOTES.md"

e7eeecb

This reverts commit 0c1f414.

hrajwade96 and others added 2 commits October 2, 2024 22:46

used constants, added util for header size calc, added params

c5ec6f6

Merge branch 'master' into network_screen_offline_support

131cde6

hrajwade96 marked this pull request as ready for review October 2, 2024 17:19

hrajwade96 added 3 commits October 2, 2024 22:56

param renamed, used _log

2a9c4cb

Merge remote-tracking branch 'origin/network_screen_offline_support' …

4f473be

…into network_screen_offline_support

added copyright header

ae58738

kenzieschmoll reviewed Oct 2, 2024

View reviewed changes

Comment thread packages/devtools_app/lib/src/screens/network/network_screen.dart Outdated

kenzieschmoll reviewed Oct 2, 2024

View reviewed changes

Comment thread packages/devtools_app/lib/src/screens/network/network_controller.dart Outdated

hrajwade96 added 5 commits October 3, 2024 18:35

setup changes implemented

2bfbe1c

put class name in square brackets

857cfbe

added isNullOrEmpty check

7150436

added exit offline cta, hiding other controls, removed json string

da11a67

fix for request selection

941ec88

kenzieschmoll reviewed Oct 7, 2024

View reviewed changes

hrajwade96 added 5 commits October 8, 2024 09:09

comments resolved

852b144

removing getFullRequestData section

433af38

using updateOrAddAll for setting data

4644b00

Wrapped with OfflineAwareControls, removed if-else.

6fd98d0

removed addAutoDisposeListener as we are using OfflineAwareControls

665ea0d

added handing and tests for requests having error

243dda4

kenzieschmoll reviewed Jan 27, 2025

View reviewed changes

hrajwade96 added 2 commits January 28, 2025 19:53

fixed requiresConnection value and import

0b847a3

comments resolved, added disconnected network screen body

70f8a43

kenzieschmoll reviewed Jan 28, 2025

View reviewed changes

Merge branch 'master' into network_screen_offline_support

0454095

hrajwade96 and others added 2 commits February 7, 2025 23:58

added a check for disconnected journey

6bd41ea

Merge branch 'master' into network_screen_offline_support

69186ce

hrajwade96 added 3 commits February 8, 2025 13:15

Merge remote-tracking branch 'origin/network_screen_offline_support' …

c1d04cd

…into network_screen_offline_support

added a check to fetch full data only when connected

04ab572

added handling for disconnected journey for json viewer, added check …

be18d9a

…in controller, added a util function isConnected()

minor fix for copy data

e42fe34

kenzieschmoll reviewed Feb 10, 2025

View reviewed changes

Comment thread packages/devtools_app/lib/src/shared/utils/utils.dart Outdated

kenzieschmoll reviewed Feb 10, 2025

View reviewed changes

Comment thread packages/devtools_app/lib/src/shared/ui/common_widgets.dart Outdated

kenzieschmoll reviewed Feb 10, 2025

View reviewed changes

Comment thread packages/devtools_app/lib/src/shared/ui/common_widgets.dart

kenzieschmoll reviewed Feb 10, 2025

View reviewed changes

Comment thread packages/devtools_app/lib/src/shared/ui/common_widgets.dart

hrajwade96 added 2 commits February 11, 2025 14:05

Modified offline checks, removed function from util, replaced dynamic…

2281232

… with Object?

comment added back

7aaf948

kenzieschmoll merged commit 220eae9 into flutter:master Feb 12, 2025

This was referenced Feb 12, 2025

Add open / save controls to the network screen #8886

Merged

Enable offline support for all DevTools screens #4470

Open

Conversation

hrajwade96 commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

kenzieschmoll commented Sep 30, 2024

Uh oh!

hrajwade96 commented Oct 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kenzieschmoll commented Jan 21, 2025

Uh oh!

hrajwade96 commented Jan 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenzieschmoll commented Jan 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hrajwade96 commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kenzieschmoll Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrajwade96 Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

hrajwade96 Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrajwade96 Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll commented Jan 30, 2025

Uh oh!

kenzieschmoll commented Jan 31, 2025

Uh oh!

hrajwade96 commented Feb 7, 2025

Uh oh!

hrajwade96 commented Feb 9, 2025

Uh oh!

kenzieschmoll Feb 10, 2025

Choose a reason for hiding this comment

Uh oh!

hrajwade96 Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

hrajwade96 commented Sep 21, 2024 •

edited

Loading

hrajwade96 commented Jan 25, 2025 •

edited

Loading

hrajwade96 commented Jan 28, 2025 •

edited

Loading

kenzieschmoll Jan 28, 2025 •

edited

Loading

hrajwade96 Jan 30, 2025 •

edited

Loading

hrajwade96 Jan 30, 2025 •

edited

Loading

hrajwade96 Feb 11, 2025 •

edited

Loading