-
Notifications
You must be signed in to change notification settings - Fork 328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Offline support for Network page #8332
base: master
Are you sure you want to change the base?
Offline support for Network page #8332
Conversation
@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
packages/devtools_app/lib/src/screens/network/network_screen.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
shouldLoad: (data) => !data.isEmpty, | ||
loadData: (data) => loadOfflineData(data), | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code after this if statement should be put in an else
block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not using else was an oversight, added else block now, and also now there is no code outside.
packages/devtools_app/lib/src/screens/network/network_controller.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/offline_network_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/network_screen.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/offline_network_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/offline_network_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/shared/http/http_request_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/shared/http/http_request_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/shared/http/http_request_data.dart
Outdated
Show resolved
Hide resolved
SocketJsonKey.id.name: id, | ||
SocketJsonKey.startTime.name: startTime, | ||
SocketJsonKey.endTime.name: endTime, | ||
//TODO verify if these timings are in correct format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please address this TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While testing I noticed 2 issues :
- The timings in socket stats are having discrepancy in online and offline mode - I thought this could be due to _timelineMicrosBase but I tried adding/ removing it there was still a discrepancy. Is it due to some time zone or formatting issue? I am trying to figure out.
- The Request and Response data is missing in offline mode - I think it's because full fetch is not called, if we want to call where can we ideally call it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another point is that in offline mode data of socket and http requests is grouped when it's populated.
I think we should show them in their original order as the requests came in? Perhaps we can pack them in a combined way instead, or other way is to reorder before rendering based on timestamps (once we fix the timings) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually past 2 weeks I've been on vacation, and then unwell so got very little time to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table should already sort the data by timestamp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, just checked it thanks, it was just due to the issue in timings it was looking grouped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kenzieschmoll can you help with first 2 points? also point 3) on page refresh when in offline mode, this is how it is looking like currently, it's clearing the list, and also not showing exit cta
what is the expected behaviour here?
Do we need to persist the data in storage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 1st point is due to value of _timelineMicrosBase being reset when entering offline mode, causing a discrepancy in timing calculation with this function :
int timelineMicrosecondsSinceEpoch(int micros) { return _timelineMicrosBase + micros; }
I fixed it by adding timelineMicrosOffset in offline data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2nd and 3rd points are remaining,
for (2) - I tried fetching full data with periodic timer
void startPeriodicExport() {
_periodicExportTimer = Timer.periodic(const Duration(milliseconds: 500), (
_,
) async {
try {
unawaited(_fetchFullDataBeforeExport());
debugPrint('Export data fetched successfully');
} catch (e) {
debugPrint('Error during periodic export fetch: $e');
}
});
}
void stopPeriodicExport() {
_periodicExportTimer?.cancel();
_periodicExportTimer = null;
}
so that full data is always available before entering in offline mode. Or alternatively we can fetch each time new request data is added in the current list. Is this fine? or any better ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (3) is there any simple way to add anther check for showing exit offline cta?
such as whenever it is disconnected, current check doesn't seem to work after page refresh
packages/devtools_app/lib/src/screens/network/offline_network_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/offline_network_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/offline_network_data.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/screens/network/offline_network_data.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test coverage for this change. Thanks!
expect(offlineData.socketData.first.id, '105553123901536'); | ||
expect(offlineData.socketData.first.socketType, 'tcp'); | ||
expect(offlineData.socketData.first.port, 443); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we can remove these expectations since they are already tested above. Fine to leave the socketData.length check above on line 123 though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
final firstSocket = socketData.first as Map<String, Object?>; | ||
final socketDetails = firstSocket['socket'] as Map<String, Object?>; | ||
|
||
expect(socketDetails['id'], '105553123901536'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here: we already verified the socket data above, so these can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple more comments. Thank you for all your work on this change!
Looks like there are several failing checks. Please also add an entry to NEXT_RELEASE_NOTES.md so that the release notes check passes. |
You will likely have several merge conflicts after pulling in the tip of the master branch. A change just landed that upgraded the Flutter version used in DevTools, which updated to a new version of the dart formatter. Here's what I recommend to make this merge simpler for you:
|
…fline_support # Conflicts: # packages/devtools_app/lib/src/screens/network/har_data_entry.dart # packages/devtools_app/lib/src/screens/network/network_screen.dart # packages/devtools_app/lib/src/shared/http/http_request_data.dart
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
///
).If you need help, consider asking for help on Discord.