Conversation
WalkthroughRefactors the QS encoder into a frame-and-phase-driven DFS, introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EncoderStack as "EncodeFrame Stack"
participant PhaseMachine as "Phase Machine"
participant PathBuilder as "Path Builder"
Client->>EncoderStack: push(root value, phase: start)
loop process frames
EncoderStack->>PhaseMachine: inspect current phase
alt start
PhaseMachine->>PathBuilder: prepare objKeys / seqList
PathBuilder-->>EncoderStack: objKeys / seqList
PhaseMachine-->>EncoderStack: transition -> iterate
else iterate
PhaseMachine->>EncoderStack: if has next child
EncoderStack->>EncoderStack: push(child frame, phase: start)
EncoderStack-->>EncoderStack: child returns fragment(s)
EncoderStack->>EncoderStack: collect child fragments into values[]
PhaseMachine-->>EncoderStack: index++
else exhausted
PhaseMachine->>PathBuilder: materialise path & assemble fragment
PathBuilder-->>EncoderStack: fragment appended to payload
EncoderStack-->>Client: propagate fragment or final payload
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50 +/- ##
==========================================
+ Coverage 97.51% 98.03% +0.51%
==========================================
Files 18 18
Lines 1247 1322 +75
==========================================
+ Hits 1216 1296 +80
+ Misses 31 26 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
CHANGELOG.md (1)
3-7: Consider adding issue/PR links per bullet for traceability.The entries are clear, but linking each bullet to its issue/PR would make regression tracking easier across releases.
Based on learnings: PRs should link issues, state behavior changes, and mention any parity deviations from Node qs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 3 - 7, Update the CHANGELOG.md bullets to include explicit issue or PR links and brief traceability notes: for each bullet (e.g., the encode fast-path/guarded single-key linear map, the encoder traversal refactor, KeyPathNode.materialize(), segment-string reuse/top-level fragment assembly, and tool/perf_snapshot.dart) append the corresponding issue/PR link and, where relevant, a one-line note about any behavior changes or parity deviations from Node qs; ensure each entry cites the PR/issue number (or “N/A” if none) and a short parenthetical like “(PR `#1234`)” or “(issue `#567`)” so future readers can trace regressions.lib/src/enums/encode_phase.dart (1)
4-7: Rename enum members to PascalCase to match repository rules.
start,iterate, andawaitChildshould follow the project’s enum-member naming convention.As per coding guidelines: Enum members should use PascalCase naming convention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/enums/encode_phase.dart` around lines 4 - 7, Rename the enum members in EncodePhase from start, iterate, awaitChild to PascalCase (Start, Iterate, AwaitChild) to comply with the project convention; update all usages of EncodePhase.start, EncodePhase.iterate, and EncodePhase.awaitChild throughout the codebase to the new names (EncodePhase.Start, EncodePhase.Iterate, EncodePhase.AwaitChild) to avoid compilation errors.tool/perf_snapshot.dart (1)
28-30: Avoid direct console output in non encode/decode paths.This benchmarking tool currently performs direct
As per coding guidelines: Avoid printing and side-effects outside encode/decode operations.
Also applies to: 55-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/perf_snapshot.dart` around lines 28 - 30, The two direct print calls in perf_snapshot.dart (the 'qs.dart perf snapshot...' and 'Encode (encode=false, deep nesting):' lines) introduce IO side-effects; refactor by extracting the benchmark logic into a pure helper (e.g., collectPerfSnapshot or generatePerfMetrics) that returns a structured result object or Map with fields for title, test name, and median timings, and move all console output to the top-level runner (e.g., main) which reads that result and prints. Also remove or refactor any other prints at lines 55-59 to follow the same pattern so all printing is isolated to the explicit IO path.lib/src/extensions/encode.dart (3)
458-463: Exception safety: tracked objects may leak on unexpected errors.If an exception other than the intentional
RangeError(e.g., fromconfig.formatterorconfig.serializeDate) is thrown within the loop,cleanupTracked()is not invoked. While this likely has minimal impact sincesideChannelis scoped per-encode call, consider wrapping the loop in a try-finally for robustness.🛡️ Proposed try-finally wrapper
final List<Object> tracked = []; - void cleanupTracked() { - for (int i = tracked.length - 1; i >= 0; i--) { - sideChannel.remove(tracked[i]); - } - tracked.clear(); - } - - while (true) { + try { + while (true) { // ... existing loop body ... + } + } finally { + for (int i = tracked.length - 1; i >= 0; i--) { + sideChannel.remove(tracked[i]); + } }🤖 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 458 - 463, The removal loop that iterates tracked and calls sideChannel.remove may throw from user formatters, so wrap the loop in a try { ... } finally { tracked.clear(); } (or call the existing cleanupTracked() in the finally) to guarantee tracked is cleared and sideChannel entries are removed on any exception; update the cleanup area around the tracked/sideChannel removal in encode.dart (referencing cleanupTracked(), tracked, and sideChannel) so removal and tracked.clear() always run in finally.
346-354: Consider narrowing the catch clause or adding a debug assertion.The catch-all
catch (_)silently swallows all exceptions when accessing a dynamic indexer, which could mask unexpected errors (e.g.,OutOfMemoryError,StackOverflowError). While the intent to handle missing indexers gracefully is clear, consider catching onlyNoSuchMethodErroror logging in debug mode to aid troubleshooting.🔧 Proposed narrower catch
} else { // Best-effort dynamic indexer for user-defined classes that expose `operator []`. // If it throws (no indexer / wrong type), we fall through to the catch and mark undefined. value = (frame.object as dynamic)[key]; valueUndefined = false; } - } catch (_) { + } on NoSuchMethodError catch (_) { value = null; valueUndefined = true; }🤖 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 346 - 354, The current catch-all in the dynamic indexer block (around the use of (frame.object as dynamic)[key]) swallows all exceptions; change it to only catch expected errors (e.g., NoSuchMethodError and TypeError) and treat others as rethrows, and add a debug-only assertion or logging to surface unexpected exceptions during development; update the try/catch around the operator [] access (the block that sets value and valueUndefined) to catch specific exceptions and rethrow or assert on anything else so OutOfMemoryError/StackOverflowError aren’t silently ignored.
409-419: Minor optimisation opportunity for list concatenation.When
encodedis aList<String>, usingaddAllwith a filtered view would be slightly more efficient than iterating element-by-element. However, this is a micro-optimisation and the current code is clear.♻️ Optional: use addAll for list results
case EncodePhase.awaitChild: final dynamic encoded = lastResult; - if (encoded is Iterable) { - for (final dynamic e in encoded) { - if (e != null) { - frame.values.add(e as String); - } - } + if (encoded is List<String>) { + frame.values.addAll(encoded.where((e) => e != null)); + } else if (encoded is Iterable) { + frame.values.addAll(encoded.whereType<String>()); } else if (encoded != null) { frame.values.add(encoded as String); }🤖 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 409 - 419, The loop in the EncodePhase.awaitChild branch can be replaced with a single addAll call to avoid element-by-element iteration: when lastResult (local variable encoded) is an Iterable, cast it to Iterable and call frame.values.addAll((encoded as Iterable).whereType<String>()) so only non-null Strings are added; keep the existing else branch for a single non-null encoded value. Reference symbols: EncodePhase.awaitChild, lastResult/encoded, and frame.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 `@CHANGELOG.md`:
- Around line 3-7: Update the CHANGELOG.md bullets to include explicit issue or
PR links and brief traceability notes: for each bullet (e.g., the encode
fast-path/guarded single-key linear map, the encoder traversal refactor,
KeyPathNode.materialize(), segment-string reuse/top-level fragment assembly, and
tool/perf_snapshot.dart) append the corresponding issue/PR link and, where
relevant, a one-line note about any behavior changes or parity deviations from
Node qs; ensure each entry cites the PR/issue number (or “N/A” if none) and a
short parenthetical like “(PR `#1234`)” or “(issue `#567`)” so future readers can
trace regressions.
In `@lib/src/enums/encode_phase.dart`:
- Around line 4-7: Rename the enum members in EncodePhase from start, iterate,
awaitChild to PascalCase (Start, Iterate, AwaitChild) to comply with the project
convention; update all usages of EncodePhase.start, EncodePhase.iterate, and
EncodePhase.awaitChild throughout the codebase to the new names
(EncodePhase.Start, EncodePhase.Iterate, EncodePhase.AwaitChild) to avoid
compilation errors.
In `@lib/src/extensions/encode.dart`:
- Around line 458-463: The removal loop that iterates tracked and calls
sideChannel.remove may throw from user formatters, so wrap the loop in a try {
... } finally { tracked.clear(); } (or call the existing cleanupTracked() in the
finally) to guarantee tracked is cleared and sideChannel entries are removed on
any exception; update the cleanup area around the tracked/sideChannel removal in
encode.dart (referencing cleanupTracked(), tracked, and sideChannel) so removal
and tracked.clear() always run in finally.
- Around line 346-354: The current catch-all in the dynamic indexer block
(around the use of (frame.object as dynamic)[key]) swallows all exceptions;
change it to only catch expected errors (e.g., NoSuchMethodError and TypeError)
and treat others as rethrows, and add a debug-only assertion or logging to
surface unexpected exceptions during development; update the try/catch around
the operator [] access (the block that sets value and valueUndefined) to catch
specific exceptions and rethrow or assert on anything else so
OutOfMemoryError/StackOverflowError aren’t silently ignored.
- Around line 409-419: The loop in the EncodePhase.awaitChild branch can be
replaced with a single addAll call to avoid element-by-element iteration: when
lastResult (local variable encoded) is an Iterable, cast it to Iterable and call
frame.values.addAll((encoded as Iterable).whereType<String>()) so only non-null
Strings are added; keep the existing else branch for a single non-null encoded
value. Reference symbols: EncodePhase.awaitChild, lastResult/encoded, and
frame.values.
In `@tool/perf_snapshot.dart`:
- Around line 28-30: The two direct print calls in perf_snapshot.dart (the
'qs.dart perf snapshot...' and 'Encode (encode=false, deep nesting):' lines)
introduce IO side-effects; refactor by extracting the benchmark logic into a
pure helper (e.g., collectPerfSnapshot or generatePerfMetrics) that returns a
structured result object or Map with fields for title, test name, and median
timings, and move all console output to the top-level runner (e.g., main) which
reads that result and prints. Also remove or refactor any other prints at lines
55-59 to follow the same pattern so all printing is isolated to the explicit IO
path.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.mdlib/src/enums/encode_phase.dartlib/src/extensions/encode.dartlib/src/models/encode_frame.dartlib/src/models/key_path_node.dartlib/src/qs.darttest/unit/encode_test.darttest/unit/models/key_path_node_test.darttool/perf_snapshot.dart
There was a problem hiding this comment.
Pull request overview
This PR refactors the core query-string encoder traversal to reduce allocations and improve deep-structure encode performance, while adding targeted regression tests and a repeatable perf snapshot tool.
Changes:
- Reworked iterative encoder traversal to a phase-driven
EncodeFrame/EncodePhasemodel and introduced a guarded deep single-key map-chain fast path. - Optimized top-level fragment assembly in
QS.encodeto reduce intermediate list allocations. - Improved
KeyPathNode.materialize()deep-chain behavior and added deep-chain regression/perf infrastructure (unit tests + perf snapshot script).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/src/extensions/encode.dart |
Major traversal refactor (phase-driven stack), MRU segment reuse, and _tryEncodeLinearChain fast path. |
lib/src/models/encode_frame.dart |
Updates frame state to support phase-driven traversal (removes callback, adds phase + iteration state). |
lib/src/enums/encode_phase.dart |
Adds internal phase enum used by the new traversal model. |
lib/src/qs.dart |
Switches fragment assembly to StringBuffer with incremental delimiter writes. |
lib/src/models/key_path_node.dart |
Reworks materialize() to build the path string in one pass for deep chains. |
test/unit/encode_test.dart |
Adds deep-chain and fast-path fallback/regression tests for encode=false and option interactions. |
test/unit/models/key_path_node_test.dart |
Adds very-deep materialize() regression coverage. |
tool/perf_snapshot.dart |
Adds a local script for repeatable deep-encode performance snapshots. |
CHANGELOG.md |
Documents the performance refactor and new tooling/tests for the next version. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7a0c48fcd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
@codex review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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". |
|
@coderabbitai resume |
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 `@lib/src/extensions/encode.dart`:
- Around line 118-120: The EncodePhase enum members are using lowerCamelCase;
rename the enum members to PascalCase (e.g., start -> Start, iterate -> Iterate,
awaitChild -> AwaitChild) in the enum declaration and update every usage: change
references like EncodePhase.start, EncodePhase.iterate, EncodePhase.awaitChild,
and switch arms in functions/methods that pattern-match on EncodePhase (such as
the switch handling frame.phase and any other comparisons or assignments found
later in this file) to their PascalCase equivalents to satisfy the repository
naming rules.
- Around line 157-163: The code currently emits raw materialized keys (via
materializedPath()) in the strict-null/empty-list branches which bypasses the
normal key encoding/formatting pipeline; update the branches that call
finishFrame(frame, keyOnly) so they first run the key through the same
encoder/formatting logic used elsewhere (respecting config.encoder and
config.encodeValuesOnly) instead of using materializedPath()
directly—specifically adjust the logic around materializedPath(), the
construction of keyOnly, and the calls to finishFrame() so the produced key is
encoder!(...) or formatted exactly like other paths; apply the same change to
the analogous empty-list branch that currently emits materializedPath() at the
other location.
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
lib/src/extensions/encode.dart (2)
118-120:⚠️ Potential issue | 🟡 MinorEnum case names should follow PascalCase.
EncodePhase.start,EncodePhase.iterate, andEncodePhase.awaitChildviolate the repo enum naming rule and should be updated consistently with the enum declaration and all references.#!/bin/bash # Verify enum declaration and all lowerCamelCase member usages. rg -nP --type=dart 'enum\s+EncodePhase\b' -C6 rg -nP --type=dart '\bEncodePhase\.(start|iterate|awaitChild)\b' -C2As per coding guidelines:
Enum members should use PascalCase naming convention.Also applies to: 310-312, 412-425
🤖 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 118 - 120, The enum member usages EncodePhase.start, EncodePhase.iterate, and EncodePhase.awaitChild (and their other occurrences around lines noted) violate the PascalCase rule; update the enum declaration to use PascalCase names (e.g., Start, Iterate, AwaitChild) and then rename all references to those members (places like the switch on frame.phase and any equality checks or case labels) to the new PascalCase identifiers so the declaration and all usages remain consistent (search for EncodePhase.start/iterate/awaitChild and replace with EncodePhase.Start/Iterate/AwaitChild).
157-163:⚠️ Potential issue | 🟠 MajorKey-only emissions still bypass the standard key formatting pipeline.
At Line 159 and Line 297, emitted key-only fragments skip
config.formatter(...), which can produce inconsistent output versus normal key handling (for example formatter-driven space handling).Suggested fix
@@ - String? pathText; - String materializedPath() => pathText ??= frame.path.materialize(); + String? pathText; + String materializedPath() => pathText ??= frame.path.materialize(); + String formatKeyOnly(String rawPath) { + final String key = config.encoder != null && !config.encodeValuesOnly + ? config.encoder!(rawPath) + : rawPath; + return config.formatter(key); + } @@ - final String keyOnly = - config.encoder != null && !config.encodeValuesOnly - ? config.encoder!(materializedPath()) - : materializedPath(); - finishFrame(frame, keyOnly); + finishFrame(frame, formatKeyOnly(materializedPath())); continue; @@ - if (config.allowEmptyLists && seqList != null && seqList.isEmpty) { - finishFrame(frame, adjustedPath.append('[]').materialize()); + if (config.allowEmptyLists && seqList != null && seqList.isEmpty) { + finishFrame( + frame, + formatKeyOnly(adjustedPath.append('[]').materialize()), + ); continue; }Also applies to: 296-297
🤖 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 157 - 163, The key-only emission bypasses the standard formatter: when building the key-only string (using materializedPath() and optional config.encoder) before calling finishFrame (in the strictNullHandling branch and the analogous branch later), pass the computed key through config.formatter(...) (respecting encodeValuesOnly and encoder logic) so the same formatting pipeline is applied; update the key-only construction used in finishFrame calls to use config.formatter(keyOnly) when config.formatter is set.
🤖 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 118-120: The enum member usages EncodePhase.start,
EncodePhase.iterate, and EncodePhase.awaitChild (and their other occurrences
around lines noted) violate the PascalCase rule; update the enum declaration to
use PascalCase names (e.g., Start, Iterate, AwaitChild) and then rename all
references to those members (places like the switch on frame.phase and any
equality checks or case labels) to the new PascalCase identifiers so the
declaration and all usages remain consistent (search for
EncodePhase.start/iterate/awaitChild and replace with
EncodePhase.Start/Iterate/AwaitChild).
- Around line 157-163: The key-only emission bypasses the standard formatter:
when building the key-only string (using materializedPath() and optional
config.encoder) before calling finishFrame (in the strictNullHandling branch and
the analogous branch later), pass the computed key through config.formatter(...)
(respecting encodeValuesOnly and encoder logic) so the same formatting pipeline
is applied; update the key-only construction used in finishFrame calls to use
config.formatter(keyOnly) when config.formatter is set.
techouse
force-pushed
the
chore/optimizations-2
branch
from
b371481 to
97ea7ca
Compare
This pull request introduces significant performance optimizations and a major refactor of the iterative encoder traversal logic in the query string serialization library. The changes include a new phase-driven stack frame model that eliminates per-child callback allocations, a guarded fast path for deep single-key map chains, improved key-path materialization, and lower-allocation fragment assembly. Additionally, new internal types and utility classes are introduced to support these improvements, and test/performance infrastructure is enhanced.
Performance optimizations and encoder traversal refactor:
EncodePhase), removing per-child callback allocations and preserving existing cycle/null/comma semantics. This significantly reduces allocation overhead and improves traversal efficiency. [1] [2] [3] [4]_tryEncodeLinearChain) for deep single-key map chains under strict option constraints, enabling linear-time encoding and reducing allocations for these workloads. [1] [2]KeyPathNode.materialize()to be linear-time by building the string once at the end instead of per-step.Internal types and utility classes:
_ValueSentinelclass to distinguish synthetic "value" entries (e.g., comma-list joins) from normal keys during traversal. [1] [2]Test and performance infrastructure:
tool/perf_snapshot.dartfor repeatable deep encode performance snapshots and expanded encode/key-path regression coverage for deep chains and fast-path fallback scenarios.Changelog:
CHANGELOG.mdto document all performance improvements, refactors, and new test infrastructure.Dart: main vs PR head
9385664)97ea7ca)main / PR)PR head vs other ports