refactor: remove default value for custom field in har models#5
Merged
refactor: remove default value for custom field in har models#5
Conversation
Updated various HAR model classes to remove default value assignment for the 'custom' field, allowing for more flexible initialization.
Updated multiple toJson methods across various classes to remove the includeNulls parameter, simplifying the JSON serialization process.
Updated tests to remove writing parsed JSON to output files and ensured that the parsed JSON is not empty.
- Removed default values for request and response in HarEntry. - Cleaned up imports in har_entry.dart.
Removed the redundant comment about deserialization from the DevToolsHarLog class to enhance code clarity.
- Improved clarity in the refactor issue template description. - Removed the enable-beta-ecosystems flag from dependabot configuration. - Cleaned up spelling dictionaries by removing misspelled terms. - Removed a redundant analyzer rule from analysis options.
📝 WalkthroughWalkthroughThis PR refactors HAR (HTTP Archive) model constructors across multiple files by removing default values for the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
==========================================
+ Coverage 88.19% 88.75% +0.55%
==========================================
Files 27 27
Lines 644 640 -4
==========================================
Hits 568 568
+ Misses 76 72 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Changed version from 0.1.0 to 0.1.1
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/src/models/base/har_request.dart (1)
9-16:⚠️ Potential issue | 🟡 MinorStale class doc comment after making constructor parameters optional
The doc comment at lines 14–15 still reads: "All seven top-level fields (method, url, httpVersion, cookies, headers, queryString, headersSize, bodySize) are required by the spec. Only [postData] and [comment] are optional."
After this PR, only
url,headersSize, andbodySizeremainrequired;method,httpVersion,cookies,headers, andqueryStringnow have defaults.📝 Suggested update
-/// All seven top-level fields (method, url, httpVersion, cookies, -/// headers, queryString, headersSize, bodySize) are required by the -/// spec. Only [postData] and [comment] are optional. +/// [url], [headersSize], and [bodySize] are required. The remaining +/// spec-required fields ([method], [httpVersion], [cookies], [headers], +/// [queryString]) have safe defaults and may be omitted at the call site. +/// Only [postData] and [comment] are truly optional per the spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/base/har_request.dart` around lines 9 - 16, Update the class doc comment in HarRequest (file: har_request.dart) to reflect the current constructor parameter requirements: note that only url, headersSize, and bodySize are required by the spec in this implementation, while method, httpVersion, cookies, headers, and queryString now have defaults and are no longer required; edit the descriptive paragraph to remove the outdated claim about seven required top-level fields and list the actual required and optional/defaulted fields (including postData and comment still optional).lib/src/models/devtools/devtools_har_entry.dart (1)
143-154:⚠️ Potential issue | 🟠 MajorSame
includeNullspropagation gap asHarRoot.toJson.
super.toJson()is called without forwardingincludeNulls. All inheritedHarEntryfields (request, response, cache, etc.) will have their nulls stripped by the base class before the outerapplyNullPolicy(includeNulls: includeNulls)on line 153 sees the map. See the same issue raised inhar_root.dart.🔧 Restore propagation or remove the parameter
- ...super.toJson(), + ...super.toJson(includeNulls: includeNulls),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/devtools/devtools_har_entry.dart` around lines 143 - 154, The toJson method in DevtoolsHarEntry calls super.toJson() without forwarding includeNulls, so inherited fields get nulls stripped prematurely; update the call in Json toJson({bool includeNulls = false}) => ... to invoke super.toJson(includeNulls: includeNulls) (or alternatively remove the includeNulls parameter entirely if you prefer not to support it), ensuring the includeNulls flag is propagated to the base HarEntry serialization before HarUtils.applyNullPolicy is applied.lib/src/models/devtools/devtools_har_timings.dart (1)
107-114:⚠️ Potential issue | 🟠 Major
super.toJson()does not forwardincludeNulls.Same propagation gap as
HarRoot.toJsonandDevToolsHarEntry.toJson. All inheritedHarTimingsnull fields (blocked,dns,connect,ssl) are stripped bysuper.toJson()before the outerapplyNullPolicyon line 113.🔧 Restore propagation
- ...super.toJson(), + ...super.toJson(includeNulls: includeNulls),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/devtools/devtools_har_timings.dart` around lines 107 - 114, The toJson in DevToolsHarTimings calls super.toJson() without passing includeNulls so inherited HarTimings null fields get stripped prematurely; update the call to forward the includeNulls flag (i.e., call super.toJson(includeNulls: includeNulls) or first capture super.toJson(includeNulls: includeNulls) into a Map and then merge with the DevTools-specific keys) so that HarTimings' null-handling is preserved before HarUtils.applyNullPolicy is applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/ISSUE_TEMPLATE/refactor.md:
- Line 10: Replace the typo "any" with "and" in the sentence so it reads
correctly, and optionally shorten the phrase "in order to help prioritize" to
"to help prioritize" to improve conciseness; search for the exact strings "any"
(in the phrase) and "in order to help prioritize" to locate and update them in
the template.
In `@lib/src/models/base/har_cache.dart`:
- Around line 54-61: The public HarCache.toJson(includeNulls: bool) currently
forwards includeNulls only to applyNullPolicy, causing nested serializers
(afterRequest?.toJson(), beforeRequest?.toJson(), commonJson()) to use their
default and drop nulls; update HarCache.toJson (and the other affected model
toJson methods such as HarCacheEntry.toJson, HarPage.toJson, HarEntry.toJson,
HarRequest.toJson, HarResponse.toJson, HarLog.toJson, HarPageTimings.toJson) to
propagate the flag into nested calls (e.g., afterRequest?.toJson(includeNulls:
includeNulls), beforeRequest?.toJson(includeNulls: includeNulls),
commonJson(includeNulls: includeNulls)) or alternatively remove the includeNulls
parameter from the public signatures if you intend top-level-only behavior—pick
one consistent approach across all model toJson methods and apply it everywhere.
In `@lib/src/models/base/har_name_version.dart`:
- Line 23: In HarNameVersion (lib/src/models/base/har_name_version.dart) remove
the redundant explicit default on super.custom (it already defaults to const {}
in HarObject) and update the toJson/commonJson call to propagate the
includeNulls flag: change the map construction that currently uses
...commonJson() to use ...commonJson(includeNulls: includeNulls) so
applyNullPolicy (used inside commonJson) receives the same includeNulls value as
toJson; ensure you update the same pattern wherever commonJson is spread (e.g.,
maps containing kName and kVersion) so null-inclusion behavior is consistent
with toJson(includeNulls: true).
In `@lib/src/models/base/har_post_data.dart`:
- Around line 22-23: The serialized output always includes an empty "params"
array because HarPostData defines params = const [] and the serializer always
emits kParams: params.map(...).toList(); change the serializer (where kParams is
added) to only include kParams when params is non-null and not empty (e.g., if
(params?.isNotEmpty ?? false) add kParams: params.map(...).toList()); this
preserves the current constructor but prevents emitting "params": [] for
instances that originally had no params; reference HarPostData, the params
field, and the kParams key in the toJson/serialization code.
- Around line 102-110: HarPostData.toJson is not forwarding the includeNulls
flag to nested serializers so HarParam.toJson and commonJson() still use their
default (false); update HarPostData.toJson to pass the includeNulls argument
into params.map(...) by calling e.toJson(includeNulls: includeNulls) and also
call commonJson(includeNulls: includeNulls) so the null-inclusion policy is
consistently propagated to nested HarParam and shared fields.
In `@lib/src/models/base/har_query_param.dart`:
- Around line 60-63: The toJson method in HarQueryParam (Json toJson) calls
commonJson() without forwarding the includeNulls flag, so comment/custom fields
always strip nulls; update the call to pass the flag (e.g.,
commonJson(includeNulls: includeNulls)) so commonJson receives and applies the
same includeNulls policy used by HarQueryParam::toJson.
In `@lib/src/models/base/har_response.dart`:
- Line 6: HarResponse currently imports HarRequest only to read
HarRequest.kDefaultHttpVersion, creating an unnecessary dependency; remove the
import of "har_request.dart" and add a static const String kDefaultHttpVersion
inside the HarResponse class (matching the existing value used from
HarRequest.kDefaultHttpVersion), then update any references within this file to
use HarResponse.kDefaultHttpVersion instead of HarRequest.kDefaultHttpVersion so
the peer models are no longer coupled.
- Line 36: The _fromJson path for HarResponse uses an empty string fallback for
httpVersion causing divergence from the constructor default; update the
httpVersion assignment in HarResponse._fromJson (the expression currently using
json[kHttpVersion]?.toString() ?? '') to instead fallback to
HarRequest.kDefaultHttpVersion (or call the HarResponse constructor default) so
both construction paths produce the same default HTTP version; ensure you
reference kHttpVersion and HarRequest.kDefaultHttpVersion in the change.
In `@lib/src/models/base/har_root.dart`:
- Around line 32-35: HarRoot.toJson currently calls log.toJson() and
commonJson() without forwarding the includeNulls flag so nested nulls are
stripped; similarly HarLog.toJson calls browser?.toJson(), creator.toJson(), and
commonJson() without the flag. Fix by propagating the parameter: pass
includeNulls to all nested toJson(...) and commonJson(...) calls (e.g.,
HarRoot.log.toJson(includeNulls: includeNulls), HarRoot.commonJson(includeNulls:
includeNulls), HarLog.browser?.toJson(includeNulls: includeNulls),
HarLog.creator.toJson(includeNulls: includeNulls), etc.) so
HarUtils.applyNullPolicy can operate on full raw maps; alternatively, if you
intend to deprecate the parameter, add a deprecation notice to toJson and adjust
callers accordingly.
In `@lib/src/models/devtools/devtools_har_cookie.dart`:
- Around line 79-82: The toJson method currently calls super.toJson() without
forwarding includeNulls, causing inconsistent null handling between base-class
fields and kSameSite; change Json toJson({bool includeNulls = false}) to call
super.toJson(includeNulls: includeNulls) so the base map preserves null entries
when requested, then let HarUtils.applyNullPolicy(..., includeNulls:
includeNulls) perform the single consistent null-filtering pass for kSameSite
(sameSite?.value) and all inherited fields.
In `@lib/src/models/devtools/devtools_har_response.dart`:
- Around line 102-106: The DevToolsHarResponse.toJson implementation fails to
forward includeNulls to the base class so base-class null fields get stripped;
change the call to super.toJson to return everything (e.g., call
super.toJson(includeNulls: true) or otherwise obtain the full base map) and then
call HarUtils.applyNullPolicy once at this level with includeNulls passed
through, keeping kTransferSize and kError merged into that full map so
null-handling is applied uniformly.
In `@test/src/models/devtools/devtools_har_parser_test.dart`:
- Line 14: The test's assertion using parsedJson and isNotEmpty is too weak
because jsonEncode({}) yields "{}" which is non-empty; instead assert a
meaningful round-trip by comparing structure or specific fields: after parsing
the HAR round-trip check parsedJson contains expected keys (e.g.,
parsedJson['log'] and parsedJson['log']['creator']['name']) or assert key counts
match the original map, or compare the parsedJson map to the original decoded
map to ensure fidelity; update the assertion referencing parsedJson and the
jsonEncode/parsed variables accordingly.
---
Outside diff comments:
In `@lib/src/models/base/har_request.dart`:
- Around line 9-16: Update the class doc comment in HarRequest (file:
har_request.dart) to reflect the current constructor parameter requirements:
note that only url, headersSize, and bodySize are required by the spec in this
implementation, while method, httpVersion, cookies, headers, and queryString now
have defaults and are no longer required; edit the descriptive paragraph to
remove the outdated claim about seven required top-level fields and list the
actual required and optional/defaulted fields (including postData and comment
still optional).
In `@lib/src/models/devtools/devtools_har_entry.dart`:
- Around line 143-154: The toJson method in DevtoolsHarEntry calls
super.toJson() without forwarding includeNulls, so inherited fields get nulls
stripped prematurely; update the call in Json toJson({bool includeNulls =
false}) => ... to invoke super.toJson(includeNulls: includeNulls) (or
alternatively remove the includeNulls parameter entirely if you prefer not to
support it), ensuring the includeNulls flag is propagated to the base HarEntry
serialization before HarUtils.applyNullPolicy is applied.
In `@lib/src/models/devtools/devtools_har_timings.dart`:
- Around line 107-114: The toJson in DevToolsHarTimings calls super.toJson()
without passing includeNulls so inherited HarTimings null fields get stripped
prematurely; update the call to forward the includeNulls flag (i.e., call
super.toJson(includeNulls: includeNulls) or first capture
super.toJson(includeNulls: includeNulls) into a Map and then merge with the
DevTools-specific keys) so that HarTimings' null-handling is preserved before
HarUtils.applyNullPolicy is applied.
---
Duplicate comments:
In `@lib/src/models/base/har_content.dart`:
- Line 23: The constructor for HarContent and its use of super.custom should
guard against null like in HarNameVersion: change the constructor parameter
usage from "super.custom" to "super.custom ?? const {}" (or otherwise ensure a
non-null Map) and update commonJson() handling to avoid null propagation—use
null-aware access or default an empty map before calling .forEach (e.g.,
(commonJson() ?? {}).forEach(...) or commonJson()?.forEach(...) with safe
handling). This ensures HarContent's constructor and its commonJson() usage
won't NPE when custom or commonJson() is null.
In `@lib/src/models/base/har_cookie.dart`:
- Line 26: The HarCookie constructor is reusing the same problematic
default/propagation pattern as HarNameVersion: remove or avoid duplicating
super.custom as a default and make the commonJson() call null-safe when
building/merging custom maps. Update the HarCookie class constructor (and any
place calling commonJson()) to not pass super.custom twice; instead initialize
custom by merging (if non-null) e.g. set localCustom = commonJson() ?? {} then
merge with provided custom only when not null, and ensure any commonJson() calls
are guarded with null-propagation so you never assume it returns non-null.
In `@lib/src/models/base/har_header.dart`:
- Line 20: HarHeader's constructor currently forwards comment via super.custom
which can be lost if commonJson() filters nulls; update HarHeader::toJson (and
constructor usage of super.custom) to explicitly pass the comment field and
ensure commonJson/includeNulls is propagated — call commonJson(includeNulls:
includeNulls) (or add an includeNulls argument) so that when
toJson(includeNulls: true) the comment property is preserved, and replace or
supplement super.custom with an explicit "comment: comment" entry so the comment
isn't dropped by null-propagation.
In `@lib/src/models/base/har_timings.dart`:
- Line 48: Constructor and JSON merging for HarTimings repeat the same issues as
HarNameVersion: remove or avoid using a hard default for super.custom in the
HarTimings constructor and make commonJson() null-safe when merging maps. Update
the HarTimings constructor (the call site that passes super.custom) to not
supply a default empty map blindly, and change commonJson() in HarTimings to use
null-aware access/merging (e.g., treat custom as possibly null and merge with ??
{} or use ?. operators) so you don't force non-null defaults or risk
null-propagation errors—follow the same pattern/fix you applied to
HarNameVersion for consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 1-3: Update the CHANGELOG.md entry for version 0.1.1 to replace
the stale "- Initial version." line with a concise summary of the actual changes
introduced in 0.1.1 (e.g., removal of default values on the custom field,
cleanup/removal of includeNulls behavior, and the version bump/refactor notes);
ensure the entry clearly lists the notable breaking or behavior-changing items
so consumers understand what changed in 0.1.1.
- Line 1: The changelog starts with an h2 ("## 0.1.1") which violates MD041;
update CHANGELOG.md to include a top-level heading and brief preamble: insert a
leading "# Changelog" (or similar top-level title) and optionally a short
introductory line before the existing "## 0.1.1" entry so the file begins with
an h1; ensure the new top-level heading appears above the current "## 0.1.1"
heading to satisfy the linter.
- Modified toJson methods across multiple classes to pass includeNulls parameter to commonJson and other toJson calls, ensuring consistent null handling in JSON output.
Updated HarRequest and HarResponse to use HarObject.kDefaultHttpVersion for consistency. Removed unnecessary import of HarRequest in HarResponse.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Dart HAR (HTTP Archive) model layer to reduce boilerplate around custom fields and to make many constructor parameters optional via safe defaults, while also cleaning up tests and project configuration around releases and CI tooling.
Changes:
- Removed repeated
custom = const {}defaults from many model constructors and centralized shared defaults (e.g., HTTP version) onHarObject. - Made several previously-required HAR model constructor parameters optional with defaults (notably in request/response/log models) and aligned JSON parsing defaults accordingly.
- Removed file-write side effects from parser tests; updated versioning/docs/CI configs (pubspec/changelog/README/templates/spell-check/dependabot/analyzer rules).
Reviewed changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/src/models/devtools/devtools_har_parser_test.dart | Removes file output side effect; adds assertion that serialized JSON is non-empty. |
| test/src/models/base/har_parser_test.dart | Removes file output side effect; adds assertion that serialized JSON is non-empty. |
| pubspec.yaml | Bumps package version to 0.1.1. |
| lib/src/models/har_utils.dart | Makes includeNulls optional with a default for null-filtering utility. |
| lib/src/models/har_object.dart | Adds shared kDefaultHttpVersion; continues to centralize custom default. |
| lib/src/models/devtools/devtools_har_timings.dart | Removes redundant default for custom super-parameter. |
| lib/src/models/devtools/devtools_har_root.dart | Simplifies constructor; removes redundant custom default. |
| lib/src/models/devtools/devtools_har_response.dart | Makes cookies/headers optional at the DevTools layer; removes redundant custom default. |
| lib/src/models/devtools/devtools_har_request.dart | Makes several super-parameters optional; removes redundant custom default. |
| lib/src/models/devtools/devtools_har_log.dart | Defaults entries to empty list; removes redundant custom default; updates doc wording. |
| lib/src/models/devtools/devtools_har_entry.dart | Removes redundant custom default. |
| lib/src/models/devtools/devtools_har_cookie.dart | Removes redundant custom default. |
| lib/src/models/base/har_timings.dart | Removes redundant custom default. |
| lib/src/models/base/har_response.dart | Defaults headers/cookies/httpVersion; aligns parsing fallback to default HTTP version. |
| lib/src/models/base/har_request.dart | Defaults method/httpVersion/cookies/headers/queryString; aligns parsing fallback to default HTTP version; updates docs. |
| lib/src/models/base/har_query_param.dart | Removes redundant custom default. |
| lib/src/models/base/har_post_data.dart | Defaults params list; makes text optional; removes redundant custom default; updates docs. |
| lib/src/models/base/har_page.dart | Changes missing/invalid datetime fallback to Unix epoch; removes redundant custom default; updates docs. |
| lib/src/models/base/har_name_version.dart | Removes redundant custom default. |
| lib/src/models/base/har_log.dart | Defaults entries to empty list; removes redundant custom default; updates docs. |
| lib/src/models/base/har_header.dart | Removes redundant custom default. |
| lib/src/models/base/har_entry.dart | Removes unused import; simplifies fallback request/response creation; removes stale toJson comment. |
| lib/src/models/base/har_cookie.dart | Removes redundant custom default; updates fromJson doc wording. |
| lib/src/models/base/har_content.dart | Removes redundant custom default. |
| lib/src/models/base/har_cache.dart | Removes redundant custom default. |
| example/lib/main.dart | Simplifies example usage to rely on new defaults; reduces ignore directives; streamlines entry printing. |
| analysis_options.yaml | Removes deprecated/obsolete analyzer rule configuration. |
| README.md | Updates example snippets and removes the architecture diagram block. |
| CHANGELOG.md | Bumps version header to 0.1.1. |
| .github/workflows/config/spell-check/nicknames_dictionary.txt | Minor formatting cleanup in spell-check dictionary. |
| .github/workflows/config/spell-check/http_dictionary.txt | Removes misspelled/invalid dictionary entry. |
| .github/workflows/config/spell-check/dart_dictionary.txt | Removes misspelled entries from Dart spell-check dictionary. |
| .github/dependabot.yaml | Removes top-level beta ecosystems flag. |
| .github/ISSUE_TEMPLATE/revert.md | Fixes wording (“follow-on”). |
| .github/ISSUE_TEMPLATE/refactor.md | Fixes grammar (“and why”). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/models/har_object.dart`:
- Around line 11-12: The kDefaultHttpVersion constant is defined on the generic
HarObject base but only used by HarRequest/HarResponse; move it closer to its
consumers by removing kDefaultHttpVersion from HarObject and either (a) add the
constant to HarRequest (or HarResponse) where it's used, or (b) create a small
shared har_constants.dart and export kDefaultHttpVersion from there, then update
references in HarRequest/HarResponse to import the new constant; ensure no other
Har* subclasses depend on HarObject.kDefaultHttpVersion before removing it.
---
Duplicate comments:
In `@lib/src/models/base/har_post_data.dart`:
- Around line 102-110: The toJson in HarPostData is always emitting kParams
because params.map(...).toList() yields an empty list; update HarPostData.toJson
so kParams is only included when params.isNotEmpty (or when params is not null)
instead of unconditionally adding kParams: map params to JSON only when
params.isNotEmpty and otherwise omit the kParams entry before calling
HarUtils.applyNullPolicy; reference HarPostData, params, toJson, kParams, and
HarUtils.applyNullPolicy when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Summary by CodeRabbit
New Features
Refactor
Chores