Conversation
|
@codex review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAdds structured-key detection and selective merging for string inputs; extracts comma/list‑splitting helpers; threads list‑growth/list‑limit semantics through parsing; replaces slice with substring; replaces internal encode sentinel with ValueSentinel; expands tests and adds a decode benchmark script. Changes
Sequence DiagramsequenceDiagram
participant Client as Input (query string)
participant QS as QS.decode()
participant Scan as StructuredKeyScan
participant Parser as _parseKeys/_parseObject
participant ListParser as _parseListValue
participant Utils as Utils.decode()
participant Result as Decoded map
Client->>QS: decode(input, options)
alt input is String
QS->>Parser: parse into tempObj
QS->>Scan: scanStructuredKeys(tempObj, options)
Scan-->>QS: StructuredKeyScan (hasAnyStructuredSyntax, roots, keys)
alt hasAnyStructuredSyntax == false
QS->>Result: return compact(tempObj)
else
QS->>Parser: _parseKeys(decodeFromString=true, scan)
Parser->>ListParser: parse list segments (pass isListGrowthPath)
ListParser->>Utils: decode parts (uses '+'/'%' guards)
ListParser-->>Parser: parts/lists (enforce remaining capacity/listLimit)
Parser->>QS: merge respecting structuredRoots and list-growth
QS->>Result: return final decoded map
end
else
QS->>Parser: _parseKeys(decodeFromString=false)
Parser->>Result: return merged object
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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 #51 +/- ##
==========================================
- Coverage 98.03% 97.71% -0.32%
==========================================
Files 18 20 +2
Lines 1322 1402 +80
==========================================
+ Hits 1296 1370 +74
- Misses 26 32 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Ports decode hot-path optimizations into qs_dart to improve performance on flat and mixed flat/structured query workloads while preserving Node qs semantics.
Changes:
- Adds flat-query and mixed flat/structured fast paths in
QS.decodeto bypass key parsing/merge when safe. - Optimizes query tokenization and comma-list splitting to reduce allocations and enforce limits efficiently.
- Adds targeted unit tests and a
tool/benchmark snapshot script for decode performance regression tracking.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tool/decode_perf_snapshot.dart | Adds a standalone decode performance snapshot harness aligned to C1/C2/C3 benchmark shapes. |
| test/unit/decode_test.dart | Adds tests covering flat fast path, mixed merge behavior, delimiter edge cases, and parameter-limit counting. |
| lib/src/utils.dart | Adds a no-op fast path to Utils.decode when no decoding work is needed (+/% absent). |
| lib/src/qs.dart | Introduces structured-syntax detection, structured-root protection, and fast paths in QS.decode. |
| lib/src/extensions/decode.dart | Refactors tokenization/comma splitting into capped collectors and replaces hot slice usage with substring. |
Comments suppressed due to low confidence (1)
lib/src/extensions/decode.dart:236
partsis built via_collectNonEmptyParts(...), which already filters out empty segments, so thisif (part.isEmpty) continue;branch should be unreachable. Removing it would simplify the loop and avoid implying that empty parts can still occur here.
final String part = parts[i];
if (part.isEmpty) continue;
final int bracketEqualsPos = part.indexOf(']=');
|
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/src/qs.dart (1)
37-103: Consider moving structured-key helper logic intoextensions/decode.dart.
_hasStructuredSyntax,_leadingStructuredRoot, and_computeStructuredRootsare decode-only concerns; relocating them would keepQSfocused on orchestration.Based on learnings: Implementation should delegate heavy lifting to private part files (extensions/decode.dart & extensions/encode.dart) rather than monolithic functions in src/qs.dart.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/qs.dart` around lines 37 - 103, Move the decode-only helper functions _hasStructuredSyntax, _leadingStructuredRoot, and _computeStructuredRoots out of src/qs.dart into the private extensions/decode.dart part file; implement them there (keeping their signatures and using DecodeOptions and _$Decode._splitKeyIntoSegments) and export or make them available to the main QS code via the library/part directive so callers in src/qs.dart call the relocated helpers instead of the originals; update imports/part declarations and any references (e.g., usages that iterate tempObj or call _leadingStructuredRoot) so compilation and behavior remain unchanged.
🤖 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/decode.dart`:
- Around line 53-75: The strict listLimit check can be bypassed when an existing
scalar value is converted into a list because currentListLength is only computed
for List instances; update the logic that reads currentListLength (used where
options.throwOnLimitExceeded is checked and where _splitCommaValue is called) to
treat a non-list existing value as length 1 (i.e., if the existing entry is not
a List but is non-null, count it as one element) before comparing against
options.listLimit and before computing remaining; apply the same fix in both
places referenced (the scalar→list transition path around _splitCommaValue and
the incremental growth guard where currentListLength is checked) and keep using
_listLimitExceededMessage when throwing.
---
Nitpick comments:
In `@lib/src/qs.dart`:
- Around line 37-103: Move the decode-only helper functions
_hasStructuredSyntax, _leadingStructuredRoot, and _computeStructuredRoots out of
src/qs.dart into the private extensions/decode.dart part file; implement them
there (keeping their signatures and using DecodeOptions and
_$Decode._splitKeyIntoSegments) and export or make them available to the main QS
code via the library/part directive so callers in src/qs.dart call the relocated
helpers instead of the originals; update imports/part declarations and any
references (e.g., usages that iterate tempObj or call _leadingStructuredRoot) so
compilation and behavior remain unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/extensions/decode.dartlib/src/qs.dartlib/src/utils.darttest/unit/decode_test.darttool/decode_perf_snapshot.dart
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/decode.dart`:
- Around line 321-329: The code misdetects list-growth when checking
part.contains('[]=') (it inspects the full "key=value" token); change the check
to inspect the parsed key only (e.g. use key.contains('[]') or
key.endsWith('[]') as appropriate) when computing listGrowthFromKey so
expressions like "a=x[]=y" are not treated as list growth; update the
listGrowthFromKey assignment that references part to use the parsed key variable
instead while leaving existingKey, combiningDuplicates, and currentListLength
logic unchanged.
abbb3b5 to
d2085a5
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/extensions/decode.dart (1)
430-435:⚠️ Potential issue | 🟡 MinorUpdate the
_parseObjectcontract comment to match new strict[]growth checks.
_parseObjectnow forwards list-growth context into_parseListValue(Line 434), which can throw onthrowOnLimitExceeded. The nearby method docs still state that this path is not enforced, so the comment is now misleading.📝 Suggested doc-only fix
- /// - A negative `listLimit` disables numeric‑index parsing (bracketed numbers become map keys). - /// Empty‑bracket pushes (`[]`) still create lists here; this method does not enforce - /// `throwOnLimitExceeded` for that path. Comma‑split growth (if any) has already been - /// handled by `_parseListValue`. + /// - A negative `listLimit` disables numeric‑index parsing (bracketed numbers become map keys). + /// Empty‑bracket pushes (`[]`) still create lists here. + /// When `throwOnLimitExceeded` is true, list-growth checks are enforced via `_parseListValue` + /// for list-growth paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/decode.dart` around lines 430 - 435, Update the comment/contract for _parseObject to reflect that it now forwards list-growth context into _parseListValue (via the chain '[]' check) and therefore enforces strict list growth checks which can trigger throwOnLimitExceeded; explicitly document that callers should expect _parseListValue to potentially throw when options.parseLists and throwOnLimitExceeded are enabled, and note which parameters (chain and options) convey this behavior so reviewers can locate the relevant code paths in _parseObject and _parseListValue.
🧹 Nitpick comments (1)
tool/decode_perf_snapshot.dart (1)
108-109: Prefer decoupling benchmark output from directThis harness currently emits console output directly. Consider returning structured rows (or injecting a reporter callback) so measurement logic stays side-effect-light and reusable.
As per coding guidelines, "Avoid printing and side-effects outside encode/decode operations".
Also applies to: 148-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tool/decode_perf_snapshot.dart` around lines 108 - 109, The direct print calls emitting benchmark text (e.g., the string literals 'qs.dart decode perf snapshot (median of 7 samples)' and 'Decode (public API):' and the prints around lines 148-154) should be removed and the harness refactored to return structured measurement rows (e.g., a list/map of results) or accept an injected reporter callback parameter; update the function that runs the decode benchmark in decode_perf_snapshot.dart to collect results into a data structure and either return it or call the provided reporter with each row, ensuring no direct console printing occurs inside the measurement logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/src/extensions/decode.dart`:
- Around line 430-435: Update the comment/contract for _parseObject to reflect
that it now forwards list-growth context into _parseListValue (via the chain
'[]' check) and therefore enforces strict list growth checks which can trigger
throwOnLimitExceeded; explicitly document that callers should expect
_parseListValue to potentially throw when options.parseLists and
throwOnLimitExceeded are enabled, and note which parameters (chain and options)
convey this behavior so reviewers can locate the relevant code paths in
_parseObject and _parseListValue.
---
Nitpick comments:
In `@tool/decode_perf_snapshot.dart`:
- Around line 108-109: The direct print calls emitting benchmark text (e.g., the
string literals 'qs.dart decode perf snapshot (median of 7 samples)' and 'Decode
(public API):' and the prints around lines 148-154) should be removed and the
harness refactored to return structured measurement rows (e.g., a list/map of
results) or accept an injected reporter callback parameter; update the function
that runs the decode benchmark in decode_perf_snapshot.dart to collect results
into a data structure and either return it or call the provided reporter with
each row, ensuring no direct console printing occurs inside the measurement
logic.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/extensions/decode.dartlib/src/qs.dartlib/src/utils.darttest/unit/decode_test.darttool/decode_perf_snapshot.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/utils.dart
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
lib/src/extensions/decode.dart:435
_parseObjectonly treats a list-growth path aschain.length >= 2 && chain.last == '[]'. WhenQS.decodeis called with a pre-parsedMap<String, dynamic>(sovaluesParsed == false), a root key of just'[]'will not be considered a growth path and therefore will not enforcethrowOnLimitExceeded/listLimit(notablylistLimit < 0should throw on any list growth perDecodeOptionsdocs). This makes strict-limit behavior differ between String input and Map input.
Consider treating chain.last == '[]' && options.parseLists as a growth path even when chain.length == 1 (and keep currentListLength as 0 for the root case), so strict list-limit enforcement is consistent across input types.
// Lazily parse comma-lists once per leaf unless the caller already did.
dynamic leaf = valuesParsed
? val
: _parseListValue(
val,
options,
currentListLength,
chain.length >= 2 && chain.last == '[]' && options.parseLists,
);
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/src/extensions/decode.dart (1)
305-306: Remove redundant empty-part guard in the hot loop.Line 305 is now defensive-only, because
_collectNonEmptyPartsalready filters empty tokens before iteration. Dropping this branch simplifies the hot path slightly.♻️ Proposed micro-refactor
- if (part.isEmpty) continue; final int bracketEqualsPos = part.indexOf(']=');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/extensions/decode.dart` around lines 305 - 306, Remove the redundant empty-part guard inside the hot loop: delete the `if (part.isEmpty) continue;` check (the variable `part` and subsequent `final int bracketEqualsPos = part.indexOf(']=');` are in the same loop) because `_collectNonEmptyParts` already filters out empty tokens; ensure no other code relies on that guard and run tests to confirm behavior unchanged.
🤖 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/decode.dart`:
- Around line 305-306: Remove the redundant empty-part guard inside the hot
loop: delete the `if (part.isEmpty) continue;` check (the variable `part` and
subsequent `final int bracketEqualsPos = part.indexOf(']=');` are in the same
loop) because `_collectNonEmptyParts` already filters out empty tokens; ensure
no other code relies on that guard and run tests to confirm behavior unchanged.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
lib/src/extensions/decode.dart:189
_collectNonEmptyPartsstill callsinput.split(...)forRegExpand genericPatterndelimiters. In Dart,String.spliteagerly allocates the full list of parts, somaxParts(used forparameterLimittruncation/overflow probing) doesn’t prevent work/allocation for large inputs with non-String delimiters. Consider implementing a capped splitter forRegExp/Patternas well (iterate matches and stop atmaxParts), so the optimization and DoS protection apply consistently across delimiter types.
static List<String> _collectNonEmptyParts(
String input,
Pattern delimiter, {
int? maxParts,
}) {
return switch (delimiter) {
String d => _collectNonEmptyStringParts(input, d, maxParts: maxParts),
RegExp r => _collectNonEmptyIterableParts(
input.split(r),
maxParts: maxParts,
),
_ => _collectNonEmptyIterableParts(
input.split(delimiter),
maxParts: maxParts,
),
};
lib/src/qs.dart:83
- In
QS.decode(string path),_hasStructuredSyntaxis computed in an initial scan oftempObj.keys, and then recomputed again for each entry during the merge loop (plus_computeStructuredRootsdoes its own scanning). For structured queries this adds extra passes over the same keys; consider folding the initial scan into the per-entry logic (e.g., trackhasAnyStructuredSyntaxwhile buildingstructuredRootsand/or while iterating entries) to avoid redundant key scans.
if (decodeFromString && (tempObj?.isNotEmpty ?? false)) {
bool hasAnyStructuredSyntax = false;
for (final String key in tempObj!.keys) {
if (_hasStructuredSyntax(key, options.allowDots)) {
hasAnyStructuredSyntax = true;
break;
}
}
if (!hasAnyStructuredSyntax) {
return Utils.compact(tempObj);
}
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/decode.dart`:
- Around line 326-327: The detection of list-growth via "[]=" is currently done
unconditionally in the local variable listGrowthFromKey (uses
part.contains('[]=') || combiningDuplicates) which can trigger a RangeError
before honoring DecodeOptions.parseLists; change this logic to only consider
part.contains('[]=') when DecodeOptions.parseLists is true (but still allow
combiningDuplicates to force list growth), i.e. gate the '[]=' check behind the
parseLists option in the decode routine where listGrowthFromKey is computed; add
a regression test in test/unit/decode_test.dart that decodes "a[]=1" with
DecodeOptions(parseLists: false, throwOnLimitExceeded: true) to assert no
premature RangeError and correct behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
lib/src/qs.dart:76
- The structured-key scan / flat fast-path doesn’t consider
options.depth <= 0. Whendepthis 0 (or negative),_splitKeyIntoSegmentswill treat all keys as flat (no bracket/dot structure), so thestructuredKeyScanwork and the subsequent_parseKeys+mergeloop are unnecessary. You can skip_scanStructuredKeysentirely (and returnUtils.compact(tempObj)directly) whenoptions.depth <= 0to keep the flat-query optimization effective for that configuration.
final bool decodeFromString = input is String;
final _StructuredKeyScan structuredKeyScan =
decodeFromString && (tempObj?.isNotEmpty ?? false)
? _scanStructuredKeys(tempObj!, options)
: const _StructuredKeyScan.empty();
if (decodeFromString && !structuredKeyScan.hasAnyStructuredSyntax) {
return Utils.compact(tempObj!);
}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/decode_test.dart (1)
3226-3228: Consider simplifying the type cast.The cast to
Iterable<dynamic>followed bytoList()could be simplified if the result is already aList. However, this approach is safe and handles potential edge cases where the implementation might return different iterable types.💡 Optional simplification
- final Iterable<dynamic> iterable = result['a'] as Iterable; - expect(iterable.toList(), equals(['1', '2'])); + expect(result['a'], equals(['1', '2']));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/decode_test.dart` around lines 3226 - 3228, The test unnecessarily casts result['a'] to Iterable<dynamic> and then calls toList(); instead, cast directly to List<String> (or List<dynamic> if types vary) to simplify and make intent explicit—replace the line using the local variable iterable (from result['a'] as Iterable) with a direct typedList assignment from result['a'] (e.g., final list = result['a'] as List<String>) and assert on list, keeping the expect(...) equals(['1','2']) assertion unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/decode_test.dart`:
- Around line 3226-3228: The test unnecessarily casts result['a'] to
Iterable<dynamic> and then calls toList(); instead, cast directly to
List<String> (or List<dynamic> if types vary) to simplify and make intent
explicit—replace the line using the local variable iterable (from result['a'] as
Iterable) with a direct typedList assignment from result['a'] (e.g., final list
= result['a'] as List<String>) and assert on list, keeping the expect(...)
equals(['1','2']) assertion unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/src/extensions/decode.darttest/unit/decode_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/extensions/decode.dart
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/decode.dart`:
- Around line 267-286: The list-growth check currently gates bracket-detected
arrays behind options.parseLists, which lets inputs like a[]=1 bypass strict
list limits; update the computation of listGrowthFromKey in decode.dart (the
variable used before calling _parseListValue) to remove the options.parseLists
condition so it becomes combiningDuplicates || rawKey.endsWith('[]'), ensuring
bracket notation always signals potential list growth and that _parseListValue's
strict-limit checks (and RangeError behavior) run regardless of
options.parseLists; keep existing uses of Duplicates.combine, currentListLength,
and the call into _parseListValue unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/src/extensions/decode.dartlib/src/extensions/encode.dartlib/src/models/structured_key_scan.dartlib/src/models/value_sentinel.dartlib/src/qs.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/qs.dart
|
@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 |
ecdeb6c to
794724f
Compare
This PR ports key decode performance ideas from
qs-kotlinPRs #77/#78 intoqs.dart, focused on the decode hot path.Main goal: remove the large decode bottleneck on flat query workloads (the C1/C2/C3 benchmark shapes), while preserving existing semantics and test parity.
What Changed
1. Flat-query fast path in
QS.decode[,.,%2E/%2ewithallowDots) and early return for fully flat queries.Utils.compact(tempObj)directly, skipping key parsing + merge.File:
lib/src/qs.dart2. Mixed flat/structured bypass with structured-root protection
structuredRootscomputation._parseKeys+Utils.merge.This keeps merge behavior for cases like
a=1&a[b]=2.File:
lib/src/qs.dart3. Tokenization optimization in
_parseQueryStringValuessplit + take + toListflow with capped non-empty collectors.limit + 1probe in strict mode).ArgumentError).File:
lib/src/extensions/decode.dart4. Comma value split optimization
split(',')with manual capped splitting.remaining + 1to detect overflow without over-allocating.remainingcap directly.listLimitsemantics (including negativelistLimitbehavior currently covered by tests).File:
lib/src/extensions/decode.dart5. Decode hot-path micro-optimizations
sliceusage withsubstringwhere indices are known.Utils.decodewhen token has no+and no%.Files:
lib/src/extensions/decode.dartlib/src/utils.dart6. Benchmark harness added
File:
tool/decode_perf_snapshot.dart7. Targeted decode tests added
Added coverage for:
a=b&&c=d, trailing delimiter)File:
test/unit/decode_test.dartBenchmark Results
Measured on the same machine before vs after this PR using equivalent C1/C2/C3 shapes.
count=100, comma=false, utf8=false, len=8)count=1000, comma=false, utf8=false, len=40)count=1000, comma=true, utf8=true, len=40)Validation
Ran successfully:
make analyzemake check_formatmake testmake suredart run tool/decode_perf_snapshot.dartCompatibility / API Impact