✨ qs.js 6.15.3 parity#61
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR tightens ChangesList limit enforcement
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Decoder as _parseQueryStringValues
participant ListParser as _parseListValue
participant Promoter as _promoteCommaListIfNeeded
participant Utils as Utils.combine/merge
Decoder->>ListParser: parse value with isFlatListValue flag
ListParser->>ListParser: count commas, check listLimit
alt limit exceeded & throwOnLimitExceeded
ListParser-->>Decoder: throw RangeError
else within limit
ListParser-->>Decoder: parsed list value
end
Decoder->>Promoter: promote comma list if needed
Promoter->>Utils: combine(listLimit, throwOnLimitExceeded)
Decoder->>Utils: combine duplicate keys (throwOnLimitExceeded)
alt overflow & throwOnLimitExceeded
Utils-->>Decoder: throw RangeError
else overflow, no throw
Utils-->>Decoder: overflow map
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
🟢 Coverage 96.88% diff coverage · +0.04% coverage variation
Metric Results Coverage variation ✅ +0.04% coverage variation (-1.00%) Diff coverage ✅ 96.88% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (4ee0278) 1423 1391 97.75% Head commit (5793a48) 1452 (+29) 1420 (+29) 97.80% (+0.04%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#61) 64 62 96.88% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61 +/- ##
==========================================
+ Coverage 97.75% 97.79% +0.04%
==========================================
Files 20 20
Lines 1423 1452 +29
==========================================
+ Hits 1391 1420 +29
Misses 32 32 ☔ View full report in Codecov by Harness. 🚀 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 (2)
lib/src/extensions/decode.dart (1)
352-368: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
Utils.combinecall bodies for the two switch cases.The
(true, Duplicates.combine)and(true, _) when bracketSuffixcases are byte-for-byte identical. Dart's switch-statement fallthrough (stacking case labels with no code between them) lets you share the body without changing the guard semantics:♻️ Proposed fix using switch fallthrough
switch ((existing, options.duplicates)) { case (true, Duplicates.combine): - // Existing key + `combine` policy: merge old/new values. - obj[key] = Utils.combine( - obj[key], - val, - listLimit: options.listLimit, - throwOnLimitExceeded: options.throwOnLimitExceeded, - ); - break; - case (true, _) when bracketSuffix: + case (true, _) when bracketSuffix: // `qs` always combines duplicate bracket-notation keys, even when - // the duplicates option is `first` or `last`. + // the duplicates option is `first` or `last` (and the `combine` + // policy always merges regardless of notation). obj[key] = Utils.combine( obj[key], val, listLimit: options.listLimit, throwOnLimitExceeded: options.throwOnLimitExceeded, ); break;This PR itself needed to update both blocks identically, which is a good sign this duplication should be collapsed to prevent future divergence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/extensions/decode.dart` around lines 352 - 368, The `(true, Duplicates.combine)` and `(true, _) when bracketSuffix` branches in the switch both perform the same Utils.combine update on obj[key], so collapse them into a single shared body using switch fallthrough while preserving the bracketSuffix guard behavior. Keep the logic in decode.dart centered on the same unique symbols (Utils.combine, bracketSuffix, and the switch on duplicates) so future changes only need to be made once.lib/src/utils.dart (1)
68-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnify the list-limit error text
lib/src/utils.dartandlib/src/extensions/decode.dartboth build the sameList parsing is disabled/List limit exceededmessage. Move that into one internal helper so wording and pluralisation stay in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/utils.dart` around lines 68 - 75, The list-limit error message is duplicated between _throwListLimitExceeded in utils.dart and the matching logic in decode.dart, so centralize the wording in one internal helper and have both call it. Update _throwListLimitExceeded to own the full message construction, then refactor the decode-side list parsing path to reuse that helper so the disabled/limit-exceeded text and pluralisation stay identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/src/extensions/decode.dart`:
- Around line 352-368: The `(true, Duplicates.combine)` and `(true, _) when
bracketSuffix` branches in the switch both perform the same Utils.combine update
on obj[key], so collapse them into a single shared body using switch fallthrough
while preserving the bracketSuffix guard behavior. Keep the logic in decode.dart
centered on the same unique symbols (Utils.combine, bracketSuffix, and the
switch on duplicates) so future changes only need to be made once.
In `@lib/src/utils.dart`:
- Around line 68-75: The list-limit error message is duplicated between
_throwListLimitExceeded in utils.dart and the matching logic in decode.dart, so
centralize the wording in one internal helper and have both call it. Update
_throwListLimitExceeded to own the full message construction, then refactor the
decode-side list parsing path to reuse that helper so the
disabled/limit-exceeded text and pluralisation stay identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e3ec32d5-999d-4301-a056-31f61dd0bd96
📒 Files selected for processing (5)
CHANGELOG.mdlib/src/extensions/decode.dartlib/src/utils.darttest/unit/decode_test.darttest/unit/utils_test.dart
There was a problem hiding this comment.
Pull request overview
This PR tightens query-string decoding behavior to better match Node qs 6.15.3, focusing on stricter list-limit enforcement (including strict-mode exceptions) and expanding regression coverage for tricky parsing and encoding edge cases.
Changes:
- Centralizes strict list-limit failures via
Utils.throwListLimitExceeded()and threads strict enforcement through list merging/combining paths. - Adjusts decode list-growth and comma-split behaviors to enforce cumulative list limits more consistently across duplicate keys and mixed notation.
- Adds regression tests for unbalanced bracket keys, list-limit edge cases, cyclic compaction, and surrogate-pair chunk-boundary encoding.
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 |
|---|---|
| test/unit/utils_test.dart | Adds coverage for Utils.merge/Utils.combine list-limit overflow vs strict-throw behavior, plus cyclic compaction and surrogate-boundary encoding tests. |
| test/unit/decode_test.dart | Adds unbalanced-bracket regression cases and qs 6.15.3 list-limit parity tests (strict throws vs overflow objects). |
| lib/src/utils.dart | Introduces throwListLimitExceeded and enforces listLimit during merge/combine flows (including strict-mode throwing). |
| lib/src/extensions/decode.dart | Unifies strict list-limit error behavior and adds a pre-check for oversized flat comma-separated values. |
| CHANGELOG.md | Documents the parity fixes and newly added regression coverage under 1.8.1-dev. |
…improved readability
This pull request focuses on improving how list limits and error handling are enforced during query string (
qs) decoding, ensuring stricter compliance with Nodeqs6.15.3 behavior. It also adds regression test coverage for edge cases and unbalanced bracket keys. The main changes include stricter enforcement of list limits (with exceptions thrown when exceeded in strict mode), unification of error handling logic, and expanded test cases to match Nodeqsbehavior.List limit enforcement and error handling:
Utils.throwListLimitExceeded()to standardize throwing aRangeErrorwhen the list limit is exceeded, and replaced all previous ad hoc error messages and throws with this utility. [1] [2] [3] [4]Utils.combine()and all internal merging logic to throw immediately (whenthrowOnLimitExceededis enabled) instead of returning an overflow map, for both flat comma lists and incremental list growth. [1] [2] [3] [4] [5]qscumulative enforcement. [1] [2]Behavioral and compatibility improvements:
qs6.15.3. [1] [2] [3] [4] [5] [6] [7] [8]Regression test coverage:
qs6.15.3 regression coverage.Documentation and housekeeping:
CHANGELOG.mdto reflect the bug fixes, new strict error handling, and new regression test coverage.