Skip to content

implemented har feature#7970

Merged
kenzieschmoll merged 56 commits into
flutter:masterfrom
hrajwade96:export_har_feature
Jul 30, 2024
Merged

implemented har feature#7970
kenzieschmoll merged 56 commits into
flutter:masterfrom
hrajwade96:export_har_feature

Conversation

@hrajwade96

@hrajwade96 hrajwade96 commented Jun 23, 2024

Copy link
Copy Markdown
Contributor
  • Export har feature added.
  • includes serialisation and de-serialisation (from and to har).
  • test cases added.

This PR addresses below issue -
#3806

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.

build.yaml badge

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

@hrajwade96 hrajwade96 requested review from a team, bkonyi and kenzieschmoll as code owners June 23, 2024 07:58
@hrajwade96

Copy link
Copy Markdown
Contributor Author

@kenzieschmoll @exaby73
When I disconnect the app loader keeps showing #7968

@bkonyi bkonyi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! I've got mostly minor comments so far.

Comment thread packages/devtools_app/lib/src/screens/network/network_controller.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/network_controller.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/network_controller.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/network_controller.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/network_controller.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/network_controller.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/network_controller.dart Outdated
@hrajwade96 hrajwade96 requested a review from bkonyi June 24, 2024 17:32

@bkonyi bkonyi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the comments!

Comment thread packages/devtools_app/lib/src/screens/network/har_builder.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/har_builder.dart Outdated
};
}).toList();

// Assemble the final HAR object

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ubernit: indent

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry didn't get you

Comment thread packages/devtools_app/lib/src/screens/network/har_builder.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/har_builder.dart
Comment thread packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart Outdated
Comment thread packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart Outdated
Comment thread packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart Outdated
static const onContentLoad = -1;
static const onLoad = -1;
static const httpVersion = 'HTTP/1.1';
static const responseHttpVersion = 'http/2.0';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be hardcoded?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this field doesn't seem to be available in the 'DartIOHttpRequestData'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put some placeholder like "Unknown" instead? Maybe @brianquinlan knows if this information is available somewhere.

@hrajwade96 hrajwade96 requested a review from bkonyi June 27, 2024 08:14
@kenzieschmoll

Copy link
Copy Markdown
Member

When I disconnect the app loader keeps showing #7968

This issue should be resolved if you merge with the master branch. Fix was here: #7992

@hrajwade96

hrajwade96 commented Jul 6, 2024

Copy link
Copy Markdown
Contributor Author

@kenzieschmoll as I observed the offline functionality works in these ways :

  1. When the app gets disconnected, view offline data.
  2. Before connecting to the app, few screens support importing a file.
  3. While the app is connected user can still import a saved file, it disconnects the app and in goes in offline mode where dev can view the data.

I have implemented the 1st one (changes yet to be pushed as of now).
Can you clarify which of these is req for network screen?
this is my demo pr for 1st point from my forked repo - hrajwade96#1
let me know your inputs

@hrajwade96

Copy link
Copy Markdown
Contributor Author

@bkonyi can you re-check the comments and mark them as resolved if they are addressed

@bkonyi

bkonyi commented Jul 8, 2024

Copy link
Copy Markdown
Contributor

@bkonyi can you re-check the comments and mark them as resolved if they are addressed

Sorry for the delay, I'll take a look.

@bkonyi

bkonyi commented Jul 8, 2024

Copy link
Copy Markdown
Contributor

All of my comments are basically addressed and this LGTM overall. I'll let @kenzieschmoll give final say on the PR. Thanks for your contribution!

@hrajwade96

Copy link
Copy Markdown
Contributor Author

All of my comments are basically addressed and this LGTM overall. I'll let @kenzieschmoll give final say on the PR. Thanks for your contribution!

Looking forward to contributing more in future, after this one is done!

@kenzieschmoll kenzieschmoll left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize, I had a pending review that I never hit submit on. Here are my recommendations.

Comment thread packages/devtools_app/lib/src/screens/network/har_builder.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/har_builder.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/network_controller.dart Outdated
Comment thread packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart Outdated
Comment thread packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/network_screen.dart
Comment thread packages/devtools_app/lib/src/shared/analytics/constants/_network_constants.dart Outdated
@hrajwade96 hrajwade96 requested a review from kenzieschmoll July 19, 2024 17:59
Comment thread packages/devtools_app/lib/src/screens/network/har_network_data.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/har_network_data.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/har_network_data.dart Outdated
@hrajwade96 hrajwade96 requested a review from kenzieschmoll July 20, 2024 05:25
Comment thread packages/devtools_app/lib/src/screens/network/har_data_entry.dart
Comment thread packages/devtools_app/lib/src/shared/http/http_request_data.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/har_data_entry.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/har_data_entry.dart Outdated
Comment thread packages/devtools_app/lib/src/screens/network/har_data_entry.dart
Comment thread packages/devtools_app/lib/src/screens/network/har_data_entry.dart
Comment thread packages/devtools_app/lib/src/screens/network/har_data_entry.dart Outdated
@hrajwade96 hrajwade96 requested a review from kenzieschmoll July 25, 2024 12:09
Comment thread packages/devtools_app/lib/src/screens/network/har_data_entry.dart Outdated

@kenzieschmoll kenzieschmoll left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once all tests pass. Thanks for the feature!

@hrajwade96

Copy link
Copy Markdown
Contributor Author

LGTM once all tests pass. Thanks for the feature!

Thank you for the review! I'll ensure all tests pass. Appreciate the feedback and support!

@hrajwade96 hrajwade96 requested a review from kenzieschmoll July 26, 2024 18:16
Comment thread packages/devtools_app/lib/src/shared/diagnostics/inspector_service.dart Outdated
@hrajwade96 hrajwade96 requested a review from kenzieschmoll July 29, 2024 17:38
@hrajwade96

Copy link
Copy Markdown
Contributor Author

LGTM once all tests pass. Thanks for the feature!

@kenzieschmoll All the tests have passed can this be merged now? Or Anything else remaining?

@kenzieschmoll kenzieschmoll merged commit fae408a into flutter:master Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants