⚡ improve encode performance for deep object graphs#49
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
WalkthroughReplaces string-prefix frame state with a linked KeyPathNode model and an immutable EncodeConfig propagated via EncodeFrame; swaps WeakMap-based cycle detection for an identity Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Encoder as Encoder
participant KeyPathNode as KeyPathNode
participant EncodeConfig as EncodeConfig
participant Filter as Filter/Serializer
participant SideChannel as SideChannel (Set<Object>)
participant Output as Output
Client->>Encoder: encode(value, options)
Encoder->>EncodeConfig: create root config
Encoder->>KeyPathNode: create root path
Encoder->>SideChannel: create HashSet.identity()
loop traverse frames
Encoder->>KeyPathNode: append segments / materialize()
Encoder->>Filter: apply filter/serializer with path.materialize()
Filter-->>Encoder: possibly transformed value
Encoder->>SideChannel: check/add identity for cycle detection
alt cycle detected
Encoder->>Client: throw RangeError
else
Encoder->>Output: emit encoded pair using path.materialize()
end
end
Encoder-->>Client: return encoded string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
@codex review |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
techouse
changed the title
QS.encode performance for deep object graphs
techouse
changed the title
QS.encode performance for deep object graphsencode performance for deep object graphs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/unit/models/key_path_node_test.dart (1)
4-41: Tests cover core KeyPathNode behaviour well.The identity-based caching assertions (
identical) are a good way to verify the lazy caching semantics. Consider adding a test for deeper chains (depth > 3) to exercise theStringBuffer-based materialisation path inmaterialize(), since the current tests only reach depth 3 which hits the depth-2 optimisation and the general path once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/models/key_path_node_test.dart` around lines 4 - 41, Add a new unit test that builds a deeper KeyPathNode chain (depth > 3) using KeyPathNode.fromMaterialized(...) and repeated append(...) calls so materialize() runs the general StringBuffer path rather than the depth-2 fast path; call materialize() twice and assert the string equals the expected full path and that the two results are identical (to validate caching), and likewise add an asDotEncoded() call for a chain containing dots to ensure encoded caching across calls.lib/src/models/encode_frame.dart (1)
16-38: Consider extracting shared immutable config into a separate object to reduce per-frame allocation cost.Every
EncodeFramenow carries ~15 immutable configuration fields (strictNullHandling,skipNulls,encoder,format, etc.) that are identical across all frames in a single_encodecall. For deep object graphs (the PR targets depth 12000+), this means thousands of redundant field copies per traversal.Extracting these into a shared
EncodeConfigobject that each frame references would reduce per-frame size and allocation pressure noticeably.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/encode_frame.dart` around lines 16 - 38, Extract the repeated immutable options into a new EncodeConfig class (or typedef) that holds strictNullHandling, skipNulls, encoder, format, formatter, encodeValuesOnly, charset, allowDots, encodeDotInKeys, serializeDate, sort, filter, allowEmptyLists, generateArrayPrefix, commaRoundTrip, commaCompactNulls, onResult, etc., then change EncodeFrame to accept a single final EncodeConfig config field instead of the long list of individual option fields; update the EncodeFrame constructor and remove the duplicated option fields, adjust all uses inside EncodeFrame to reference config.<field>, and modify the call site that creates frames in _encode to instantiate one shared EncodeConfig per _encode invocation and pass it to every new EncodeFrame so frames share the same config instance.lib/src/extensions/encode.dart (1)
378-408: Per-frame duplication of immutable config fields.Every child
EncodeFramecopies ~15 identical configuration fields from the parent. For the deep object graphs this PR targets (depth 2000–12000), this is a meaningful amount of redundant allocation. As noted on theEncodeFramemodel, extracting these into a shared config object would reduce per-frame cost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/encode.dart` around lines 378 - 408, The code is copying many immutable config fields into every child EncodeFrame causing redundant allocations; refactor by introducing a shared immutable config holder (e.g., EncodeConfig) that bundles fields like undefined, sideChannel, generateArrayPrefix, commaRoundTrip, commaCompactNulls, allowEmptyLists, strictNullHandling, skipNulls, encodeDotInKeys, serializeDate, sort, filter, allowDots, format, formatter, encodeValuesOnly, charset and any other repeated options, add a config property to EncodeFrame and a constructor overload that accepts this shared EncodeConfig, update creation sites (like the current stack.add(EncodeFrame(...))) to pass frame.config or frame.sharedConfig and only set per-frame mutable fields (object, path, encoder, onResult, seqList, values, etc.), and ensure any logic (e.g., encoder selection using _commaGenerator and frame.seqList) reads from the shared config when appropriate.
🤖 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/key_path_node.dart`:
- Around line 9-16: The _length field on KeyPathNode is unused and should be
removed: delete the declaration "final int _length;" and its computation in the
private constructor KeyPathNode._(this._parent, this._segment) so the
initializer no longer computes (_parent?._length ?? 0) + _segment.length; then
remove any remaining references (e.g., in append() or other KeyPathNode methods)
and adjust code to rely only on _parent, _segment and _depth; ensure
constructors and factory methods compile after removing the _length initializer.
---
Nitpick comments:
In `@lib/src/extensions/encode.dart`:
- Around line 378-408: The code is copying many immutable config fields into
every child EncodeFrame causing redundant allocations; refactor by introducing a
shared immutable config holder (e.g., EncodeConfig) that bundles fields like
undefined, sideChannel, generateArrayPrefix, commaRoundTrip, commaCompactNulls,
allowEmptyLists, strictNullHandling, skipNulls, encodeDotInKeys, serializeDate,
sort, filter, allowDots, format, formatter, encodeValuesOnly, charset and any
other repeated options, add a config property to EncodeFrame and a constructor
overload that accepts this shared EncodeConfig, update creation sites (like the
current stack.add(EncodeFrame(...))) to pass frame.config or frame.sharedConfig
and only set per-frame mutable fields (object, path, encoder, onResult, seqList,
values, etc.), and ensure any logic (e.g., encoder selection using
_commaGenerator and frame.seqList) reads from the shared config when
appropriate.
In `@lib/src/models/encode_frame.dart`:
- Around line 16-38: Extract the repeated immutable options into a new
EncodeConfig class (or typedef) that holds strictNullHandling, skipNulls,
encoder, format, formatter, encodeValuesOnly, charset, allowDots,
encodeDotInKeys, serializeDate, sort, filter, allowEmptyLists,
generateArrayPrefix, commaRoundTrip, commaCompactNulls, onResult, etc., then
change EncodeFrame to accept a single final EncodeConfig config field instead of
the long list of individual option fields; update the EncodeFrame constructor
and remove the duplicated option fields, adjust all uses inside EncodeFrame to
reference config.<field>, and modify the call site that creates frames in
_encode to instantiate one shared EncodeConfig per _encode invocation and pass
it to every new EncodeFrame so frames share the same config instance.
In `@test/unit/models/key_path_node_test.dart`:
- Around line 4-41: Add a new unit test that builds a deeper KeyPathNode chain
(depth > 3) using KeyPathNode.fromMaterialized(...) and repeated append(...)
calls so materialize() runs the general StringBuffer path rather than the
depth-2 fast path; call materialize() twice and assert the string equals the
expected full path and that the two results are identical (to validate caching),
and likewise add an asDotEncoded() call for a chain containing dots to ensure
encoded caching across calls.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/src/extensions/encode.dartlib/src/models/encode_frame.dartlib/src/models/key_path_node.dartlib/src/qs.dartpubspec.yamltest/unit/encode_edge_cases_test.darttest/unit/models/key_path_node_test.dart
💤 Files with no reviewable changes (1)
- pubspec.yaml
There was a problem hiding this comment.
Pull request overview
This PR optimizes QS.encode for deep object graphs by reducing key-path string allocations and switching cycle detection to an O(1) active-path identity set, while also fixing cycle detection to run before filter transformations.
Changes:
- Introduced
KeyPathNodeto lazily materialize/cached key paths during encoding. - Replaced
weak_map-based cycle tracking withHashSet<Object>.identity()active-path tracking and adjusted cycle detection ordering (pre-filter). - Added regression tests for
KeyPathNodecaching and for filter-wrapped cyclic structures.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/extensions/encode.dart | Refactors encoder traversal to use KeyPathNode, generator caching, and identity-set cycle detection (pre-filter). |
| lib/src/models/encode_frame.dart | Updates encoder frame to carry KeyPathNode paths and Set<Object> side-channel state. |
| lib/src/models/key_path_node.dart | Adds lazy/cached key-path representation used by the encoder for deep nesting. |
| lib/src/qs.dart | Removes weak_map, uses identity HashSet side-channel, wires in KeyPathNode support. |
| pubspec.yaml | Drops the weak_map dependency. |
| test/unit/encode_edge_cases_test.dart | Adds regression test ensuring cycles still throw when filters wrap values in fresh containers. |
| test/unit/models/key_path_node_test.dart | Adds unit tests for KeyPathNode append/materialize/dot-encoding caching behavior. |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
🧹 Nitpick comments (5)
lib/src/models/key_path_node.dart (2)
65-68: Depth-2 fast path skips caching the parent's_materialized.if (_depth == 2) { _materialized = '${parent._segment}$_segment'; return _materialized!; }
parent._materializedis never set here. Ifparent.materialize()is called independently afterwards, it recomputes (though cheaply, as it just assigns_segment). Callingparent.materialize()inside the fast path would cache both in one go at zero extra cost:♻️ Proposed fix
if (_depth == 2) { - _materialized = '${parent._segment}$_segment'; + _materialized = '${parent.materialize()}$_segment'; return _materialized!; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/key_path_node.dart` around lines 65 - 68, The depth-2 fast path currently sets only this node's _materialized and returns, leaving parent._materialized unset; update the fast path in materialize()/where _depth == 2 to also populate the parent's cache by either calling parent.materialize() or assigning parent._materialized = parent._segment before constructing this node's _materialized (ensure you still set this node's _materialized = '${parent._segment}$_segment' and return it) so both parent and child are cached with no extra work.
26-50:asDotEncoded()is recursive — consider an iterative implementation for defensiveness.In the encoder's sequential frame-processing loop, parents are always prepared before children, so each parent's
_dotEncodedcache is warm by the time a child callsasDotEncoded(), bounding actual recursion to ≈ 2 frames. However, if this method is ever called directly on a deep uncached chain (e.g., in a future test or internal helper), it would recurse proportionally to the chain depth. Thematerialize()companion already uses an iterative loop for exactly this reason;asDotEncoded()could follow the same pattern.♻️ Sketch of an iterative replacement
KeyPathNode asDotEncoded() { final cached = _dotEncoded; if (cached != null) return cached; // Walk to the first ancestor with a cached encoded form (or root). final List<KeyPathNode> chain = []; KeyPathNode? cursor = this; while (cursor != null && cursor._dotEncoded == null) { chain.add(cursor); cursor = cursor._parent; } // Seed: either a cached encoded ancestor or null (for root). KeyPathNode? encodedParent = cursor?._dotEncoded; // Rebuild encoded nodes from root toward leaf, filling caches. for (int i = chain.length - 1; i >= 0; i--) { final node = chain[i]; final encodedSegment = _replaceDots(node._segment); final KeyPathNode encoded; if (encodedParent == null) { encoded = identical(encodedSegment, node._segment) ? node : KeyPathNode._(null, encodedSegment); } else { encoded = identical(encodedParent, node._parent) && identical(encodedSegment, node._segment) ? node : KeyPathNode._(encodedParent, encodedSegment); } node._dotEncoded = encoded; encodedParent = encoded; } return _dotEncoded!; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/key_path_node.dart` around lines 26 - 50, The asDotEncoded() method currently uses recursion and can overflow on deep uncached chains; rewrite it to be iterative like materialize(): walk up from this following _parent collecting nodes until you find an ancestor with a non-null _dotEncoded (or root), then rebuild encoded nodes from ancestor → leaf using _replaceDots on each node._segment, construct KeyPathNode._(...) only when necessary (compare with identical checks against node._parent and node._segment), set each node._dotEncoded as you build, and finally return this._dotEncoded; reference functions/fields: KeyPathNode, asDotEncoded, _dotEncoded, _parent, _segment, _replaceDots, and materialize() for the pattern to follow.lib/src/models/encode_config.dart (1)
47-68: Consider aligning with thecopyWith/EquatableMixinconvention forsrc/**/*.dartclasses.
withEncoderis the only "copy" factory, covering the only field that changes during traversal. That's intentional, but the coding guideline for option classes insrc/**/*.dartcalls for a fullcopyWithandEquatableMixin. As an@internalclass that's never compared for equality, these can reasonably be deferred — but if the class ever grows new per-frame variants (e.g.,withFilter,withSort), the lack of a generalcopyWithwill require repeated boilerplate.Based on learnings from
.github/copilot-instructions.md: "When adding an option class: ensure const constructor, add to copyWith method, implement equality via EquatableMixin."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/encode_config.dart` around lines 47 - 68, EncodeConfig currently provides only withEncoder which limits extensibility; add a general copyWith method that accepts nullable replacements for all fields (generateArrayPrefix, commaRoundTrip, commaCompactNulls, allowEmptyLists, strictNullHandling, skipNulls, encodeDotInKeys, encoder, serializeDate, sort, filter, allowDots, format, formatter, encodeValuesOnly, charset) and returns a new EncodeConfig using existing values when null, keep withEncoder delegating to copyWith for backward compatibility, make the constructor const if not already, and implement EquatableMixin on EncodeConfig with props listing all fields so instances can be compared for equality and future per-field "withX" helpers are unnecessary boilerplate.lib/src/extensions/encode.dart (1)
410-431:_buildSequenceChildPathfallback materialises then discards the chain.return KeyPathNode.fromMaterialized( generator(adjustedPath.materialize(), encodedKey), );For a custom
generator, the result is wrapped in a new rootKeyPathNode(depth 1), discarding the shared ancestor chain. This means any futureappendon this node will create a fresh depth-2 sub-chain with no relationship to prior nodes. That is semantically correct (the generator owns the full path string), but it forfeits the incremental caching benefitKeyPathNodeprovides for subsequent children. Worth a doc comment to set expectations for custom generator authors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/encode.dart` around lines 410 - 431, The fallback branch in _buildSequenceChildPath currently calls KeyPathNode.fromMaterialized(generator(adjustedPath.materialize(), encodedKey)), which materialises the full path and creates a new root KeyPathNode, discarding the ancestor chain and losing incremental caching benefits; add a concise doc comment above _buildSequenceChildPath explaining that for custom generator callbacks the generator owns the full path string, that using KeyPathNode.fromMaterialized will produce a fresh root (depth 1) with no shared ancestor chain, and note the performance/caching implication so generator authors know this is intentional and can design generators accordingly (reference generator, adjustedPath.materialize(), and KeyPathNode.fromMaterialized in the comment).test/unit/models/key_path_node_test.dart (1)
1-62: Good coverage; consider adding a root-node edge-case test.The suite covers the critical caching and identity invariants well. One gap:
fromMaterializedwith a dot-containing string is only exercised within a chain (e.g.,'a.b'as the root in the deep-chain test), butasDotEncoded()on a bare root node with dots is never tested in isolation. A single extra case would complete the contract:test('asDotEncoded encodes dots in a root node', () { final root = KeyPathNode.fromMaterialized('a.b.c'); expect(root.asDotEncoded().materialize(), equals('a%2Eb%2Ec')); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/models/key_path_node_test.dart` around lines 1 - 62, Add a focused unit test that verifies asDotEncoded on a bare root node with dots: create a root via KeyPathNode.fromMaterialized('a.b.c'), call root.asDotEncoded().materialize(), and assert it equals 'a%2Eb%2Ec' (test name: 'asDotEncoded encodes dots in a root node'); place it alongside the existing KeyPathNode tests to ensure dot-encoding for root nodes is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/src/extensions/encode.dart`:
- Around line 410-431: The fallback branch in _buildSequenceChildPath currently
calls KeyPathNode.fromMaterialized(generator(adjustedPath.materialize(),
encodedKey)), which materialises the full path and creates a new root
KeyPathNode, discarding the ancestor chain and losing incremental caching
benefits; add a concise doc comment above _buildSequenceChildPath explaining
that for custom generator callbacks the generator owns the full path string,
that using KeyPathNode.fromMaterialized will produce a fresh root (depth 1) with
no shared ancestor chain, and note the performance/caching implication so
generator authors know this is intentional and can design generators accordingly
(reference generator, adjustedPath.materialize(), and
KeyPathNode.fromMaterialized in the comment).
In `@lib/src/models/encode_config.dart`:
- Around line 47-68: EncodeConfig currently provides only withEncoder which
limits extensibility; add a general copyWith method that accepts nullable
replacements for all fields (generateArrayPrefix, commaRoundTrip,
commaCompactNulls, allowEmptyLists, strictNullHandling, skipNulls,
encodeDotInKeys, encoder, serializeDate, sort, filter, allowDots, format,
formatter, encodeValuesOnly, charset) and returns a new EncodeConfig using
existing values when null, keep withEncoder delegating to copyWith for backward
compatibility, make the constructor const if not already, and implement
EquatableMixin on EncodeConfig with props listing all fields so instances can be
compared for equality and future per-field "withX" helpers are unnecessary
boilerplate.
In `@lib/src/models/key_path_node.dart`:
- Around line 65-68: The depth-2 fast path currently sets only this node's
_materialized and returns, leaving parent._materialized unset; update the fast
path in materialize()/where _depth == 2 to also populate the parent's cache by
either calling parent.materialize() or assigning parent._materialized =
parent._segment before constructing this node's _materialized (ensure you still
set this node's _materialized = '${parent._segment}$_segment' and return it) so
both parent and child are cached with no extra work.
- Around line 26-50: The asDotEncoded() method currently uses recursion and can
overflow on deep uncached chains; rewrite it to be iterative like materialize():
walk up from this following _parent collecting nodes until you find an ancestor
with a non-null _dotEncoded (or root), then rebuild encoded nodes from ancestor
→ leaf using _replaceDots on each node._segment, construct KeyPathNode._(...)
only when necessary (compare with identical checks against node._parent and
node._segment), set each node._dotEncoded as you build, and finally return
this._dotEncoded; reference functions/fields: KeyPathNode, asDotEncoded,
_dotEncoded, _parent, _segment, _replaceDots, and materialize() for the pattern
to follow.
In `@test/unit/models/key_path_node_test.dart`:
- Around line 1-62: Add a focused unit test that verifies asDotEncoded on a bare
root node with dots: create a root via KeyPathNode.fromMaterialized('a.b.c'),
call root.asDotEncoded().materialize(), and assert it equals 'a%2Eb%2Ec' (test
name: 'asDotEncoded encodes dots in a root node'); place it alongside the
existing KeyPathNode tests to ensure dot-encoding for root nodes is covered.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/src/extensions/encode.dartlib/src/models/encode_config.dartlib/src/models/encode_frame.dartlib/src/models/key_path_node.dartlib/src/qs.darttest/unit/models/key_path_node_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/qs.dart
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/src/models/key_path_node.dart (1)
9-10:_lengthfield removed — past review comment addressed. ✓The constructor now only initialises
_depth, eliminating the unnecessary per-appendallocation that was previously flagged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/key_path_node.dart` around lines 9 - 10, Constructor KeyPathNode._ now only initializes _depth and the previously problematic per-append _length allocation has been removed; ensure the private field _length is deleted from the class and all references to _length (reads, writes, tests) are updated to use _depth or recomputed where needed (look for any usages of _length in KeyPathNode, append methods, equality/hashCode, and unit tests) so the codebase compiles and behavior remains correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/src/models/key_path_node.dart`:
- Around line 9-10: Constructor KeyPathNode._ now only initializes _depth and
the previously problematic per-append _length allocation has been removed;
ensure the private field _length is deleted from the class and all references to
_length (reads, writes, tests) are updated to use _depth or recomputed where
needed (look for any usages of _length in KeyPathNode, append methods,
equality/hashCode, and unit tests) so the codebase compiles and behavior remains
correct.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/extensions/encode.dartlib/src/models/encode_config.dartlib/src/models/key_path_node.darttest/unit/models/encode_config_test.darttest/unit/models/key_path_node_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/models/key_path_node_test.dart
- lib/src/models/encode_config.dart
techouse
force-pushed
the
chore/optimizations
branch
from
d17f7e8 to
eebaf80
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/src/models/key_path_node.dart (1)
9-10:_lengthremoval confirmed.The unused
_lengthfield flagged in the previous review has been removed. ✓🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/key_path_node.dart` around lines 9 - 10, The removed unused field _length has been confirmed and approved; ensure there are no remaining references to _length anywhere in the codebase (search for _length and usages in the KeyPathNode class and callers) and keep the constructor KeyPathNode._(this._parent, this._segment) : _depth = (_parent?._depth ?? 0) + 1; as-is; run tests/analysis to verify no lingering compilation or lint errors after the removal.
🧹 Nitpick comments (3)
lib/src/extensions/encode.dart (1)
283-289: Redundant== trueon a non-nullablebool.
EncodeConfig.commaRoundTripis declared asfinal bool(non-nullable), so comparing it totrueis redundant.♻️ Proposed simplification
- final bool shouldAppendRoundTripMarker = - (config.commaRoundTrip == true) && - seqList != null && + final bool shouldAppendRoundTripMarker = + config.commaRoundTrip && + seqList != null &&🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/encode.dart` around lines 283 - 289, The boolean comparison "config.commaRoundTrip == true" is redundant because EncodeConfig.commaRoundTrip is a non-nullable bool; update the shouldAppendRoundTripMarker expression to use config.commaRoundTrip directly (remove "== true") so the logic in the assignment for shouldAppendRoundTripMarker (which references config.generateArrayPrefix, _commaGenerator, commaEffectiveLength, and seqList) remains the same but with the simpler boolean check.lib/src/models/encode_config.dart (1)
31-31: Usestatic finalinstead ofstatic constfor the_notSetsentinel.In Dart,
const Object()is a canonicalised compile-time constant — everyconst Object()expression in the entire program resolves to the same identical object. This meansidentical(someConst, _notSet)istruefor any otherstatic const Object x = Object()anywhere in the codebase, including test files. A caller that inadvertently passesconst Object()asencoderorfilterwould silently be treated as "not provided."♻️ Proposed fix
- static const Object _notSet = Object(); + static final Object _notSet = Object();A
static finalfield is initialised once at runtime with a freshly-allocatedObject(), guaranteeing a unique sentinel that cannot be aliased from outside this class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/encode_config.dart` at line 31, Replace the sentinel declaration "static const Object _notSet = Object();" with a runtime-unique sentinel by changing it to "static final Object _notSet = Object();" in the EncodeConfig (or the class that defines _notSet) so that identical(const Object(), _notSet) cannot be true for external const Object() instances; this ensures _notSet is a unique runtime object used to detect omitted values for encoder/filter.lib/src/models/key_path_node.dart (1)
83-97: Optional: leverage parent's cached_materializedin the depth-≥3 path.The general path walks from the leaf all the way to the root on every first call, even when an ancestor's
_materializedis already populated. Checking the direct parent first would short-circuit the traversal at a much shallower depth in most encoder walks.♻️ Proposed micro-optimisation
+ final parent = _parent; + if (parent != null) { + final parentMaterialized = parent._materialized ?? parent.materialize(); + return _materialized = '$parentMaterialized$_segment'; + } + final List<String?> parts = List<String?>.filled(_depth, null); KeyPathNode? current = this; for (int i = _depth - 1; i >= 0; i--) { parts[i] = current!._segment; current = current._parent; } final StringBuffer out = StringBuffer(); for (final part in parts) { out.write(part); } _materialized = out.toString(); return _materialized!;This makes the depth-≥3 case just as O(1) amortised as depth-2 once ancestors are warm, and reduces cold-path work in deep graphs where parent nodes were already materialised earlier in the traversal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/key_path_node.dart` around lines 83 - 97, The current leaf-to-root traversal in KeyPathNode.materialization (using _segment, _parent and setting _materialized) always walks to the root; change it to first check the nearest ancestor's cached _materialized and short-circuit when found: walk upward from this node until you encounter an ancestor with non-null _materialized (or root), collect only the intervening _segment values, then build the result by starting from that ancestor's _materialized and appending the collected segments, store into this._materialized and return; update references to KeyPathNode, _materialized, _parent, and _segment accordingly so deep nodes reuse parent caches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/src/models/key_path_node.dart`:
- Around line 9-10: The removed unused field _length has been confirmed and
approved; ensure there are no remaining references to _length anywhere in the
codebase (search for _length and usages in the KeyPathNode class and callers)
and keep the constructor KeyPathNode._(this._parent, this._segment) : _depth =
(_parent?._depth ?? 0) + 1; as-is; run tests/analysis to verify no lingering
compilation or lint errors after the removal.
---
Nitpick comments:
In `@lib/src/extensions/encode.dart`:
- Around line 283-289: The boolean comparison "config.commaRoundTrip == true" is
redundant because EncodeConfig.commaRoundTrip is a non-nullable bool; update the
shouldAppendRoundTripMarker expression to use config.commaRoundTrip directly
(remove "== true") so the logic in the assignment for
shouldAppendRoundTripMarker (which references config.generateArrayPrefix,
_commaGenerator, commaEffectiveLength, and seqList) remains the same but with
the simpler boolean check.
In `@lib/src/models/encode_config.dart`:
- Line 31: Replace the sentinel declaration "static const Object _notSet =
Object();" with a runtime-unique sentinel by changing it to "static final Object
_notSet = Object();" in the EncodeConfig (or the class that defines _notSet) so
that identical(const Object(), _notSet) cannot be true for external const
Object() instances; this ensures _notSet is a unique runtime object used to
detect omitted values for encoder/filter.
In `@lib/src/models/key_path_node.dart`:
- Around line 83-97: The current leaf-to-root traversal in
KeyPathNode.materialization (using _segment, _parent and setting _materialized)
always walks to the root; change it to first check the nearest ancestor's cached
_materialized and short-circuit when found: walk upward from this node until you
encounter an ancestor with non-null _materialized (or root), collect only the
intervening _segment values, then build the result by starting from that
ancestor's _materialized and appending the collected segments, store into
this._materialized and return; update references to KeyPathNode, _materialized,
_parent, and _segment accordingly so deep nodes reuse parent caches.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
lib/src/extensions/encode.dartlib/src/models/encode_config.dartlib/src/models/encode_frame.dartlib/src/models/key_path_node.dartlib/src/qs.dartpubspec.yamltest/unit/encode_edge_cases_test.darttest/unit/models/encode_config_test.darttest/unit/models/key_path_node_test.dart
💤 Files with no reviewable changes (1)
- pubspec.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- test/unit/encode_edge_cases_test.dart
- test/unit/models/encode_config_test.dart
- test/unit/models/key_path_node_test.dart
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/src/models/key_path_node.dart (1)
9-10:_lengthremoval addresses the previous review concern.The unused
_lengthfield and its computation have been correctly removed, leaving only_depth. LGTM.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/key_path_node.dart` around lines 9 - 10, The previous unused _length field has been removed from KeyPathNode; no code changes are required—verify the KeyPathNode._ constructor (KeyPathNode._(this._parent, this._segment) : _depth = (_parent?._depth ?? 0) + 1;) is correct and leave the removal as-is.
🧹 Nitpick comments (3)
lib/src/extensions/encode.dart (2)
426-429:_commaGeneratorarm in_buildSequenceChildPathis dead code.For comma format,
objKeysis always constructed as[{'value': ...}](lines 250–258). This makesisCommaSentinel == truefor every key, and the calling site at line 369–370 takes theframe.adjustedPath!branch unconditionally —_buildSequenceChildPathis never reached whengeneratoris_commaGenerator. The arm at line 427 can be removed.♻️ Proposed simplification
- if (identical(generator, _repeatGenerator) || - identical(generator, _commaGenerator)) { + if (identical(generator, _repeatGenerator)) { return adjustedPath; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/encode.dart` around lines 426 - 429, The _commaGenerator branch in _buildSequenceChildPath is dead code and should be removed: locate the function _buildSequenceChildPath and delete the conditional arm that checks for identical(generator, _commaGenerator) (the clause that returns adjustedPath alongside the _repeatGenerator check), leaving only the _repeatGenerator check (or handle only identical(generator, _repeatGenerator)) so the function no longer contains the unreachable _commaGenerator path; ensure no other references assume that arm exists and run tests to confirm behavior unchanged.
426-429:_commaGeneratorarm in_buildSequenceChildPathis dead code.When
config.generateArrayPrefixis_commaGenerator,objKeysis always built as a single-element[{'value': ...}]map (lines 250–258). The calling site (lines 369–371) then unconditionally takes theframe.adjustedPath!branch (isCommaSentinel && identical(..., _commaGenerator)), so_buildSequenceChildPathis never reached with a comma generator. The arm is harmless but misleading.♻️ Proposed simplification
- if (identical(generator, _repeatGenerator) || - identical(generator, _commaGenerator)) { + if (identical(generator, _repeatGenerator)) { return adjustedPath; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/encode.dart` around lines 426 - 429, The `_commaGenerator` branch in `_buildSequenceChildPath` is dead code because when `config.generateArrayPrefix` is `_commaGenerator` the caller builds `objKeys` as a single-element map and always takes the `frame.adjustedPath!` branch (see `objKeys`, `generateArrayPrefix` and the call site that checks `frame.adjustedPath`), so `_buildSequenceChildPath` is never invoked with `_commaGenerator`; remove the `identical(generator, _commaGenerator)` arm from the `if (identical(generator, _repeatGenerator) || identical(generator, _commaGenerator))` check (leaving only the `_repeatGenerator` check) and run tests, or replace the combined condition with a clear single check for `_repeatGenerator` to avoid the misleading dead code while preserving `_buildSequenceChildPath` behavior for repeat generation.lib/src/models/key_path_node.dart (1)
83-102: Consider caching intermediate nodes in the generalmaterialize()path for consistency withasDotEncoded().
asDotEncoded()populates_dotEncodedon every traversed node (line 57). The general path ofmaterialize()collects only segments, so sibling nodes at depth > 2 each re-walk O(depth) uncached ancestors. The fast path at depth == 2 already caches the parent correctly; extending that behaviour to deeper chains costs one extratoString()per ancestor but eliminates repeat walks for wide object graphs.♻️ Proposed change
- final List<String> suffix = <String>[]; + final List<KeyPathNode> suffix = <KeyPathNode>[]; KeyPathNode? current = this; String base = ''; while (current != null) { final cachedCurrent = current._materialized; if (cachedCurrent != null) { base = cachedCurrent; break; } - suffix.add(current._segment); + suffix.add(current); current = current._parent; } final StringBuffer out = StringBuffer(base); for (int i = suffix.length - 1; i >= 0; i--) { - out.write(suffix[i]); + out.write(suffix[i]._segment); + suffix[i]._materialized = out.toString(); } - _materialized = out.toString(); return _materialized!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/key_path_node.dart` around lines 83 - 102, materialize() currently only sets _materialized on the leaf, causing repeated ancestor walks; change it to cache intermediate nodes similarly to asDotEncoded(): when traversing from this up to a cached ancestor (using KeyPathNode._parent and collecting _segment), after building the full materialized string assign the progressively shorter suffix strings to each traversed node's _materialized (set on the node instances whose cachedCurrent was null) so ancestors are cached and future materialize() calls avoid re-walking; preserve the existing fast path behavior for depth == 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 `@lib/src/extensions/encode.dart`:
- Around line 14-16: The docstring entry for "prefix" is stale: update or remove
the sentence that describes prefix as "current key path being built (e.g.,
`user[address]`)" because prefix is now only used to seed the root KeyPathNode
and path advancement happens via KeyPathNode.append; modify the docstring in
encode.dart to either state that prefix seeds the root KeyPathNode (used to
initialize the path) or drop the example/description entirely to avoid implying
concatenation-based path building.
- Around line 14-16: Update the docstring for the `prefix` parameter in
encode.dart to stop calling it the "current key path being built" and instead
state that it only seeds the root KeyPathNode; note that path advancement is
handled by the KeyPathNode class (see KeyPathNode and any references to `prefix`
around line 106). Edit the docstring entry for `*prefix*` to reflect its new
role (e.g., "initial root key path seed used to construct the root KeyPathNode;
subsequent path advancement is managed by KeyPathNode") or remove the outdated
description entirely so it no longer implies string-concatenation-based path
building.
---
Duplicate comments:
In `@lib/src/models/key_path_node.dart`:
- Around line 9-10: The previous unused _length field has been removed from
KeyPathNode; no code changes are required—verify the KeyPathNode._ constructor
(KeyPathNode._(this._parent, this._segment) : _depth = (_parent?._depth ?? 0) +
1;) is correct and leave the removal as-is.
---
Nitpick comments:
In `@lib/src/extensions/encode.dart`:
- Around line 426-429: The _commaGenerator branch in _buildSequenceChildPath is
dead code and should be removed: locate the function _buildSequenceChildPath and
delete the conditional arm that checks for identical(generator, _commaGenerator)
(the clause that returns adjustedPath alongside the _repeatGenerator check),
leaving only the _repeatGenerator check (or handle only identical(generator,
_repeatGenerator)) so the function no longer contains the unreachable
_commaGenerator path; ensure no other references assume that arm exists and run
tests to confirm behavior unchanged.
- Around line 426-429: The `_commaGenerator` branch in `_buildSequenceChildPath`
is dead code because when `config.generateArrayPrefix` is `_commaGenerator` the
caller builds `objKeys` as a single-element map and always takes the
`frame.adjustedPath!` branch (see `objKeys`, `generateArrayPrefix` and the call
site that checks `frame.adjustedPath`), so `_buildSequenceChildPath` is never
invoked with `_commaGenerator`; remove the `identical(generator,
_commaGenerator)` arm from the `if (identical(generator, _repeatGenerator) ||
identical(generator, _commaGenerator))` check (leaving only the
`_repeatGenerator` check) and run tests, or replace the combined condition with
a clear single check for `_repeatGenerator` to avoid the misleading dead code
while preserving `_buildSequenceChildPath` behavior for repeat generation.
In `@lib/src/models/key_path_node.dart`:
- Around line 83-102: materialize() currently only sets _materialized on the
leaf, causing repeated ancestor walks; change it to cache intermediate nodes
similarly to asDotEncoded(): when traversing from this up to a cached ancestor
(using KeyPathNode._parent and collecting _segment), after building the full
materialized string assign the progressively shorter suffix strings to each
traversed node's _materialized (set on the node instances whose cachedCurrent
was null) so ancestors are cached and future materialize() calls avoid
re-walking; preserve the existing fast path behavior for depth == 2.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/extensions/encode.dartlib/src/models/encode_config.dartlib/src/models/key_path_node.darttest/unit/models/encode_config_test.darttest/unit/models/key_path_node_test.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/src/models/encode_config.dart
- test/unit/models/key_path_node_test.dart
- test/unit/models/encode_config_test.dart
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/src/extensions/encode.dart (1)
14-17: Stale docstring concern already addressed.The
prefixentry previously flagged in review has been updated in this revision — lines 16-17 now correctly describeprefixas seeding the rootKeyPathNode. Nothing further to action here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/encode.dart` around lines 14 - 17, The docstring note about `prefix` has already been fixed in lib/src/extensions/encode.dart: no code change required; confirm the docstring for `prefix` correctly states that it "seeds the root KeyPathNode for traversal" and that references to KeyPathNode.append(...) remain accurate, then mark the stale-comment as resolved.
🧹 Nitpick comments (2)
lib/src/extensions/encode.dart (1)
428-431:_commaGeneratorbranch in_buildSequenceChildPathis unreachable dead code.When
generateArrayPrefixis_commaGeneratorandobjis anIterable, the preparation block collapses the entire list into a single{'value': …}sentinel entry (lines 225-261). When that sentinel is iterated,isCommaSentinelistrue, sokeyPathis assignedframe.adjustedPath!directly (line 373) —_buildSequenceChildPathis never called for the comma format. The_commaGeneratorarm on line 429 is therefore dead.The branch is harmless, but documenting its defensive nature (or removing it) would clarify intent.
♻️ Proposed cleanup with explanatory comment
- if (identical(generator, _repeatGenerator) || - identical(generator, _commaGenerator)) { - return adjustedPath; - } + // Repeat format: every element repeats the same key (e.g. a=1&a=2). + // Note: _commaGenerator never reaches here — comma lists are collapsed to a + // single sentinel entry before key iteration, taking the isCommaSentinel + // fast-path in the caller. + if (identical(generator, _repeatGenerator)) { + return adjustedPath; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/encode.dart` around lines 428 - 431, The branch checking identical(generator, _commaGenerator) inside _buildSequenceChildPath is dead because when generateArrayPrefix is _commaGenerator the iterable is collapsed into a single sentinel (see generateArrayPrefix/_commaGenerator behavior) and isCommaSentinel causes keyPath to be taken from frame.adjustedPath instead of calling _buildSequenceChildPath; remove the redundant `_commaGenerator` arm (or replace it with a one-line comment stating it's intentionally unreachable) in _buildSequenceChildPath to clarify intent and avoid confusion, referencing _buildSequenceChildPath, _commaGenerator, generateArrayPrefix, isCommaSentinel, and frame.adjustedPath when making the change.lib/src/models/encode_frame.dart (1)
42-46: Optional: collapsetracked+trackedObjectinto a single nullable sentinel.
trackedis always set totruein lockstep withtrackedObject = <non-null value>, making the boolean redundant. All six call sites inencode.dartthat checkframe.trackedimmediately useframe.trackedObject, sotrackedObject != nullcould serve as the sole sentinel, removing a field and eliminating the?? frame.objectfallback (which is dead code whentrackedis true).♻️ Proposed simplification
- /// Whether this frame registered a cycle-tracking entry. - bool tracked = false; - - /// The object identity registered in [sideChannel] for cycle tracking. - Object? trackedObject; + /// The object identity registered in [sideChannel] for cycle tracking; + /// non-null iff this frame registered a cycle-tracking entry. + Object? trackedObject;Then in
encode.dartreplace everyif (frame.tracked)/frame.sideChannel.remove(frame.trackedObject ?? frame.object)with:if (frame.trackedObject != null) { frame.sideChannel.remove(frame.trackedObject!); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/encode_frame.dart` around lines 42 - 46, The boolean field tracked is redundant because it is always true exactly when trackedObject is non-null; remove the tracked field from encode_frame.dart and treat trackedObject as the single nullable sentinel, update all code paths that set tracked to instead set trackedObject to the non-null value (and clear it to null where tracked was cleared), and change every call site in encode.dart that does if (frame.tracked) / frame.sideChannel.remove(frame.trackedObject ?? frame.object) to if (frame.trackedObject != null) { frame.sideChannel.remove(frame.trackedObject!); } ensuring you no longer rely on the fallback to frame.object and that sideChannel.remove is only called when trackedObject is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/src/extensions/encode.dart`:
- Around line 14-17: The docstring note about `prefix` has already been fixed in
lib/src/extensions/encode.dart: no code change required; confirm the docstring
for `prefix` correctly states that it "seeds the root KeyPathNode for traversal"
and that references to KeyPathNode.append(...) remain accurate, then mark the
stale-comment as resolved.
---
Nitpick comments:
In `@lib/src/extensions/encode.dart`:
- Around line 428-431: The branch checking identical(generator, _commaGenerator)
inside _buildSequenceChildPath is dead because when generateArrayPrefix is
_commaGenerator the iterable is collapsed into a single sentinel (see
generateArrayPrefix/_commaGenerator behavior) and isCommaSentinel causes keyPath
to be taken from frame.adjustedPath instead of calling _buildSequenceChildPath;
remove the redundant `_commaGenerator` arm (or replace it with a one-line
comment stating it's intentionally unreachable) in _buildSequenceChildPath to
clarify intent and avoid confusion, referencing _buildSequenceChildPath,
_commaGenerator, generateArrayPrefix, isCommaSentinel, and frame.adjustedPath
when making the change.
In `@lib/src/models/encode_frame.dart`:
- Around line 42-46: The boolean field tracked is redundant because it is always
true exactly when trackedObject is non-null; remove the tracked field from
encode_frame.dart and treat trackedObject as the single nullable sentinel,
update all code paths that set tracked to instead set trackedObject to the
non-null value (and clear it to null where tracked was cleared), and change
every call site in encode.dart that does if (frame.tracked) /
frame.sideChannel.remove(frame.trackedObject ?? frame.object) to if
(frame.trackedObject != null) { frame.sideChannel.remove(frame.trackedObject!);
} ensuring you no longer rely on the fallback to frame.object and that
sideChannel.remove is only called when trackedObject is present.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdlib/src/extensions/encode.dartlib/src/models/encode_frame.dartlib/src/qs.dart
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
techouse
force-pushed
the
chore/optimizations
branch
from
2f0cacb to
bf30792
Compare
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/src/models/key_path_node.dart (1)
77-81: Depth-2 fast path has an implicit parent-mutation side effect.
parent._materialized ??= parent._segmentis always correct for a depth-1 root node, but mutates the parent node as a side effect of materialising its child. This is safe and intended, but worth noting for future maintainers: any caller materialising a depth-2 node will warm the parent's cache as a by-product.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/key_path_node.dart` around lines 77 - 81, The depth-2 fast path currently mutates the parent (parent._materialized ??= parent._segment) as a side effect of materialising the child; change the block so it does not update parent._materialized: compute a local parentSegment = parent._materialized ?? parent._segment, then set _materialized = '${parentSegment}$_segment' and return it, leaving parent._materialized untouched (or if the mutation is intended, replace the code with a clear comment documenting that parent._materialized is intentionally warmed when materialising the child).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/src/models/key_path_node.dart`:
- Around line 77-81: The depth-2 fast path currently mutates the parent
(parent._materialized ??= parent._segment) as a side effect of materialising the
child; change the block so it does not update parent._materialized: compute a
local parentSegment = parent._materialized ?? parent._segment, then set
_materialized = '${parentSegment}$_segment' and return it, leaving
parent._materialized untouched (or if the mutation is intended, replace the code
with a clear comment documenting that parent._materialized is intentionally
warmed when materialising the child).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.mdlib/src/extensions/encode.dartlib/src/models/encode_config.dartlib/src/models/encode_frame.dartlib/src/models/key_path_node.dartlib/src/qs.dartpubspec.yamltest/unit/encode_edge_cases_test.darttest/unit/models/encode_config_test.darttest/unit/models/key_path_node_test.dart
💤 Files with no reviewable changes (1)
- pubspec.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- lib/src/qs.dart
- test/unit/models/encode_config_test.dart
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/src/models/key_path_node.dart (2)
3-17: "Immutable" in the docstring is misleading — clarify as logically/observationally immutable.
_dotEncodedand_materializedare mutable cache fields. The structural shape of the node is indeed fixed after construction, but the flat claim "Immutable linked node" would suggest no field ever changes.📝 Suggested doc tweak
-/// Immutable linked node representation of an encoder key path. +/// Logically immutable linked node representation of an encoder key path. +/// +/// Structural fields (_parent, _segment, _depth) are fixed at construction. +/// _dotEncoded and _materialized are mutable lazy-cache fields that are +/// write-once per node and do not affect observable value semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/key_path_node.dart` around lines 3 - 17, The class docstring currently says "Immutable linked node" which is misleading because the instance has mutable cache fields; update the comment for KeyPathNode to describe it as "logically" or "observationally" immutable while noting that _dotEncoded and _materialized are lazily-populated, mutable caches that do not change the node's structural identity; keep references to KeyPathNode._(constructor), _dotEncoded, and _materialized so reviewers can locate and verify the adjusted wording.
83-103: Optional: propagate_materializedcaching to intermediate uncached nodes.The general path collects only
_segmentstrings, so onlythis._materializedis populated at line 101. Ancestor nodes visited during the upward walk remain uncached; a futureancestor.materialize()call must re-traverse from that ancestor to the nearest cached node.To fill caches for every node visited, the suffix list would need to carry node references rather than raw strings, and the downward pass would write a partial
out.toString()into each node's_materialized— at the cost of N intermediatetoString()allocations per call. Whether this trade-off is worthwhile depends on how often intermediate nodes are materialised directly after a leaf materialisation.♻️ Sketch of intermediate-caching variant
- final List<String> suffix = <String>[]; - KeyPathNode? current = this; - String base = ''; - while (current != null) { - final cachedCurrent = current._materialized; - if (cachedCurrent != null) { - base = cachedCurrent; - break; - } - suffix.add(current._segment); - current = current._parent; - } - - final StringBuffer out = StringBuffer(base); - for (int i = suffix.length - 1; i >= 0; i--) { - out.write(suffix[i]); - } - - _materialized = out.toString(); - return _materialized!; + final List<(KeyPathNode, String)> uncached = <(KeyPathNode, String)>[]; + KeyPathNode? current = this; + String base = ''; + while (current != null) { + final cachedCurrent = current._materialized; + if (cachedCurrent != null) { + base = cachedCurrent; + break; + } + uncached.add((current, current._segment)); + current = current._parent; + } + + final StringBuffer out = StringBuffer(base); + for (int i = uncached.length - 1; i >= 0; i--) { + out.write(uncached[i].$2); + uncached[i].$1._materialized = out.toString(); + } + + return _materialized!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/models/key_path_node.dart` around lines 83 - 103, The materialize logic in KeyPathNode.materialize currently only caches this node's _materialized string; to also populate intermediate ancestor caches capture nodes (List<KeyPathNode>) instead of just segments during the upward walk, then on the downward pass build the StringBuffer progressively and assign the partial out.toString() into each visited node's _materialized (e.g., collect nodes into suffixNodes, iterate from last to first writing segment and setting node._materialized), ensuring you still respect an early break when a cached _materialized is found and avoid overwriting existing cached values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/src/models/key_path_node.dart`:
- Around line 3-17: The class docstring currently says "Immutable linked node"
which is misleading because the instance has mutable cache fields; update the
comment for KeyPathNode to describe it as "logically" or "observationally"
immutable while noting that _dotEncoded and _materialized are lazily-populated,
mutable caches that do not change the node's structural identity; keep
references to KeyPathNode._(constructor), _dotEncoded, and _materialized so
reviewers can locate and verify the adjusted wording.
- Around line 83-103: The materialize logic in KeyPathNode.materialize currently
only caches this node's _materialized string; to also populate intermediate
ancestor caches capture nodes (List<KeyPathNode>) instead of just segments
during the upward walk, then on the downward pass build the StringBuffer
progressively and assign the partial out.toString() into each visited node's
_materialized (e.g., collect nodes into suffixNodes, iterate from last to first
writing segment and setting node._materialized), ensuring you still respect an
early break when a cached _materialized is found and avoid overwriting existing
cached values.
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/extensions/encode.dart`:
- Around line 156-165: The key-only emission in the strictNullHandling branch
(config.strictNullHandling) and the allow-empty-lists branch must format and
encode the materialized path the same way as the normal fragment emission: take
materializedPath(), pass it through the configured formatter (e.g.,
config.formatter) and, if !config.encodeValuesOnly, then through config.encoder
before assigning keyOnly or emitting; update the code around materializedPath(),
config.encoder, config.formatter, and the allowEmptyLists handling to reuse the
same formatting/encoding logic as used by the normal fragment path (and still
clear frame.trackedObject via frame.sideChannel.remove when present).
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/src/extensions/decode.dartlib/src/extensions/encode.dartlib/src/models/key_path_node.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/models/key_path_node.dart
techouse
force-pushed
the
chore/optimizations
branch
from
fc1d951 to
77c8663
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/src/extensions/encode.dart (1)
14-17: Docstring updated to reflectKeyPathNode-based path model.The
*prefix*and*sideChannel*entries now accurately describe the new semantics. This addresses the prior review feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/encode.dart` around lines 14 - 17, The docstring in encode.dart contains duplicated documentation for the updated KeyPathNode-based path model (the *prefix* and *sideChannel* entries are repeated); remove the duplicate docstring block so there is a single, clear description referencing KeyPathNode and the semantics of *prefix* and *sideChannel* (ensure references to KeyPathNode.append, *prefix*, and *sideChannel* remain intact and not duplicated).
🧹 Nitpick comments (3)
lib/src/extensions/encode.dart (2)
395-403: Minor: skipcopyWithoverhead when encoder is unchanged.
config.withEncoder(childEncoder)runs a 16-field identity check insidecopyWitheven whenchildEncoderis the same reference asconfig.encoder(the common case for non-comma paths). A short-circuit avoids that overhead in the hot loop.Suggested optimisation
- final EncodeConfig childConfig = config.withEncoder(childEncoder); + final EncodeConfig childConfig = + identical(childEncoder, config.encoder) ? config : config.withEncoder(childEncoder);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/encode.dart` around lines 395 - 403, The hot loop currently always calls config.withEncoder(childEncoder) which triggers a costly 16-field identity check even when childEncoder === config.encoder; change to short-circuit by comparing childEncoder to config.encoder first and only call config.withEncoder(childEncoder) when they differ, otherwise set childConfig = config (referencing the variables childEncoder, config.encoder and the method config.withEncoder).
56-100: Consider constructingEncodeConfigin the caller (qs.dart) and passing it directly.
_encodestill accepts 18 individual parameters, only to immediately bundle them into anEncodeConfigon lines 83–100. Moving theEncodeConfigconstruction up to the call site inqs.dartand accepting a singleEncodeConfigparameter would eliminate the parameter duplication and make the internal API cleaner. This is entirely optional for this PR — the current approach works and minimises blast radius.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/encode.dart` around lines 56 - 100, The _encode function currently takes many encode-related parameters then immediately builds an EncodeConfig; instead, construct an EncodeConfig at the call site in qs.dart and change _encode to accept a single EncodeConfig parameter (e.g., replace the multiple params with EncodeConfig rootConfig), update the qs.dart caller(s) to create and pass that EncodeConfig, and remove the now-unneeded individual parameters from _encode and its call sites while ensuring internal references in _encode use the passed-in EncodeConfig instance.lib/src/models/key_path_node.dart (1)
99-104: Nit:StringBufferallocation could be avoided for depth-3 paths.For the general
materialize()path (depth ≥ 3), aStringBufferis allocated. You already have a depth-2 fast path; a depth-3 fast path using simple string interpolation ('$grandparent$parent$segment') would cover another common bracket-notation case withoutStringBufferoverhead. This is entirely optional — the current implementation is correct and theStringBuffercost is small relative to the deeper graphs this PR targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/src/extensions/encode.dart`:
- Around line 14-17: The docstring in encode.dart contains duplicated
documentation for the updated KeyPathNode-based path model (the *prefix* and
*sideChannel* entries are repeated); remove the duplicate docstring block so
there is a single, clear description referencing KeyPathNode and the semantics
of *prefix* and *sideChannel* (ensure references to KeyPathNode.append,
*prefix*, and *sideChannel* remain intact and not duplicated).
---
Nitpick comments:
In `@lib/src/extensions/encode.dart`:
- Around line 395-403: The hot loop currently always calls
config.withEncoder(childEncoder) which triggers a costly 16-field identity check
even when childEncoder === config.encoder; change to short-circuit by comparing
childEncoder to config.encoder first and only call
config.withEncoder(childEncoder) when they differ, otherwise set childConfig =
config (referencing the variables childEncoder, config.encoder and the method
config.withEncoder).
- Around line 56-100: The _encode function currently takes many encode-related
parameters then immediately builds an EncodeConfig; instead, construct an
EncodeConfig at the call site in qs.dart and change _encode to accept a single
EncodeConfig parameter (e.g., replace the multiple params with EncodeConfig
rootConfig), update the qs.dart caller(s) to create and pass that EncodeConfig,
and remove the now-unneeded individual parameters from _encode and its call
sites while ensuring internal references in _encode use the passed-in
EncodeConfig instance.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
CHANGELOG.mdlib/src/extensions/decode.dartlib/src/extensions/encode.dartlib/src/models/encode_config.dartlib/src/models/encode_frame.dartlib/src/models/key_path_node.dartlib/src/qs.dartpubspec.yamltest/unit/encode_edge_cases_test.darttest/unit/models/encode_config_test.darttest/unit/models/key_path_node_test.dart
💤 Files with no reviewable changes (1)
- pubspec.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGELOG.md
- lib/src/extensions/decode.dart
- test/unit/models/encode_config_test.dart
- test/unit/encode_edge_cases_test.dart
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49 +/- ##
==========================================
+ Coverage 97.27% 97.51% +0.24%
==========================================
Files 16 18 +2
Lines 1136 1247 +111
==========================================
+ Hits 1105 1216 +111
Misses 31 31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This pull request refactors the core encoder logic in the
qs_dartpackage to improve maintainability, performance, and clarity. The main changes revolve around introducing an immutableEncodeConfigto hold encoder options, replacing the previous approach of passing many parameters through each stack frame. Additionally, the cycle detection mechanism is simplified, and key path handling is made more robust and explicit.The most important changes are:
Encoder Configuration Refactor:
EncodeConfigclass to encapsulate all encoder options, which is now passed to eachEncodeFrameinstead of many individual parameters. This reduces parameter bloat and makes the code easier to maintain and reason about. (lib/src/models/encode_config.dart,lib/src/models/encode_frame.dart,lib/src/extensions/encode.dart) [1] [2] [3] [4] [5] [6]Cycle Detection Simplification:
WeakMapfor cycle detection with a simplerSet<Object>to track the active traversal path, providing O(1) cycle checks and reducing dependencies. (lib/src/extensions/encode.dart) [1] [2] [3]Key Path Handling Improvements:
KeyPathNodeabstraction, improving how key paths are constructed and manipulated, especially for arrays and special cases like dot encoding and round-trip markers. (lib/src/extensions/encode.dart) [1] [2] [3]Cleaner Frame Construction:
EncodeFrameto take aKeyPathNodeand anEncodeConfiginstead of many separate fields, and centralized child frame construction logic. (lib/src/models/encode_frame.dart,lib/src/extensions/encode.dart) [1] [2]General Code Cleanups:
lib/src/models/encode_frame.dart,lib/src/extensions/encode.dart) [1] [2] [3] [4] [5]These changes set up the encoder for better extensibility and correctness while making the codebase easier to understand and modify.
Benchmark Snapshot (local)
Workload:
QS.encode(payload, const EncodeOptions(encode: false))Payload: nested
{a: { ... { leaf: 'x' } } }Metric: median ms/op (7 samples)
Benchmark Appendix:
mainvs PR headSetup
main@1952f97chore/optimizations@eebaf80QS.encodemicro-benchmark (4 payload shapes)runs=9,warmups=4us/op(lower is better)ops/s(higher is better)Results
flat_scalarsnested_mixedcomma_large_listdeep_dotted_keysSummary
nested_mixed(~2.9% slower).