Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBumps a Linux‑only dependency, normalizes Obj‑C bridge option values (charset, dot handling, limits), implements non‑throwing comma‑list overflow fallback and overflow decoding, adds iterative depth‑bounded encoding fallback and iterative dictionary merge to avoid recursion, adds strong‑to‑strong NSMapTable for Linux, and expands tests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Encoder
participant IterativeEncoder
participant Stack
Client->>Encoder: encode(input, options, depth)
alt depth >= iterativeFallbackDepth and canUseIterativeDeepFallback
Encoder->>IterativeEncoder: encodeIterativeDeepFallback(root, charset, options)
IterativeEncoder->>Stack: push(root task)
loop traverse tasks
IterativeEncoder->>Stack: pop(task)
alt container node
IterativeEncoder->>IterativeEncoder: containerIdentity() -> detect cycle
IterativeEncoder->>Stack: push(leave task)
IterativeEncoder->>Stack: push(child tasks)
else scalar/leaf
IterativeEncoder->>IterativeEncoder: describe(value, charset)
IterativeEncoder->>Stack: append result to parent
end
end
IterativeEncoder-->>Encoder: composed result
else
Encoder->>Encoder: use recursive encoding path
end
Encoder-->>Client: return encoded output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 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 |
|
@codex review |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
+ Coverage 93.37% 95.63% +2.25%
==========================================
Files 54 54
Lines 2823 3158 +335
==========================================
+ Hits 2636 3020 +384
+ Misses 187 138 -49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ 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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@Sources/QsSwift/Internal/Decoder`+ParseList.swift:
- Around line 26-31: The call in parseObjectValue is relying on parseListValue's
default isFirstOccurrence=true which can wrongly trigger the overflow-map
branch; update the call in the parseObjectValue function to pass the actual
isFirstOccurrence boolean from its scope into parseListValue(_:, options:,
currentListLength:, isFirstOccurrence:) so that parseListValue receives the
correct occurrence flag rather than the default.
In `@Sources/QsSwift/Internal/Encoder.swift`:
- Around line 470-495: The swiftlint:disable:next function_parameter_count
comment only applies to canUseIterativeDeepFallback, leaving
encodeIterativeDeepFallback still triggering the rule; fix by either moving the
swiftlint suppression so it applies to both declarations (e.g., place a single
swiftlint:disable function_parameter_count before the
canUseIterativeDeepFallback declaration and re-enable after
encodeIterativeDeepFallback) or reduce parameters by introducing a parameter
object (e.g., a config/Options struct used by encodeIterativeDeepFallback and
canUseIterativeDeepFallback) and update calls to use that struct; reference the
functions canUseIterativeDeepFallback and encodeIterativeDeepFallback when
making the change.
🧹 Nitpick comments (7)
Sources/QsObjC/QsBridge+DecodeConvenience.swift (1)
35-35: Inconsistent brace placement style within the file.Lines 35 and 62 now place the opening brace at the end of the function signature, while similar functions at lines 77-79 and 89-93 still place it on a new line. Consider aligning the brace placement style across all functions in this file for consistency.
🔧 Option to align with lines 77-79, 89-93 style
- public static func decodeOrNil(_ input: Any?, options: DecodeOptionsObjC?) -> NSDictionary? { + public static func decodeOrNil(_ input: Any?, options: DecodeOptionsObjC?) + -> NSDictionary? + {- public static func decodeOrEmpty(_ input: Any?, options: DecodeOptionsObjC?) -> NSDictionary { + public static func decodeOrEmpty(_ input: Any?, options: DecodeOptionsObjC?) + -> NSDictionary + {Also applies to: 62-62
Sources/QsSwift/Internal/Decoder+ParseQuery.swift (1)
135-142: Consider extracting the overflow-aware length computation into a small helper.The
currentLenclosure computes list length from three different representations ([Any],[Any?], overflow map). Since this pattern of "get virtual array length" might be reused elsewhere (e.g., inUtils.combineor other merge logic), a tiny static helper likeUtils.effectiveListLength(_:)could reduce duplication and make the intent clearer.Sources/QsObjC/Models/DecodeOptionsObjC.swift (1)
129-136: Normalization logic looks good overall;normalizedDecodeDotInKeysis tautological.Line 130:
decodeDotInKeys && normalizedAllowDotsis always equivalent to justdecodeDotInKeys, because wheneverdecodeDotInKeysistrue,normalizedAllowDots(line 129) is alsotrue. The&&guard is therefore redundant. This is harmless but slightly misleading.Sources/QsSwift/Internal/NSMapTable+Linux.swift (1)
54-61: Silent no-op when key doesn't conform toNSObjectin weak mode.Lines 55 and 71: if the key can't be cast to
NSObject, the method silently returns (no insertion, no error). This matches the previous behavior but is worth documenting, as it could silently drop entries on Linux if non-NSObjectkeys are used.Sources/QsSwift/Internal/Utils+Merge.swift (1)
355-355: SwiftLint: remove explicit= nilinitialization.Static analysis flags
implicit_optional_initialization— optional variables are implicitlynil.Proposed fix
- var completed: [AnyHashable: Any]? = nil + var completed: [AnyHashable: Any]?Sources/QsSwift/Internal/Encoder.swift (1)
67-92: Iterative fallback gate looks solid; key-ordering diverges from the recursive path for dictionaries.When
encoder != nil, the recursive path atdepth > 0partitions dictionary keys (primitives before containers) and sorts each half. The iterative fallback iterates keys in arbitrary/insertion order regardless. At depth ≥ 512 with a non-nil encoder, the output order could differ from what the recursive path would produce for the same subtree.This is unlikely to matter in practice (queries that deep are extreme edge cases, and
canUseIterativeDeepFallbackis already very conservative), but worth noting if strict ordering parity with the recursive path is a goal.Tests/QsSwiftTests/EncodeTests.swift (1)
3815-3849: 250K-element array inencoder_iterativeFallback_largeAcyclic_noFalseCyclemay slow CI.The test allocates
250_001empty arrays to verify no false-positive cycle detection. While correct, this could be slow on resource-constrained CI runners. Consider whether a smaller count (e.g., 10K–50K) would still exercise the hash-set growth path adequately.
There was a problem hiding this comment.
Pull request overview
This pull request introduces several robustness and compatibility improvements to the QsSwift query string encoder/decoder library. The changes focus on handling deeply nested structures without stack overflow, maintaining parity with the upstream JavaScript qs library (version 6.14.2), and ensuring proper charset validation and Data encoding.
Changes:
- Added iterative fallback encoding/merging to prevent stack overflow with deeply nested structures (depth >= 512)
- Implemented qs@6.14.2 parity for comma list overflow behavior, falling back to indexed-object representation on first occurrence
- Enhanced options validation in Objective-C bridge layers to normalize charset values and enforce minimum limits
- Fixed Data encoding to respect charset parameter instead of using debug string representation
- Updated dependencies (ReerKit 1.2.2 → 1.2.5, qs 6.14.1 → 6.14.2)
- Various code style improvements (trailing commas, line breaks, pattern matching consistency)
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Package.swift | Updated ReerKit dependency to 1.2.5 for Linux builds |
| Package@swift-5.10.swift | Updated ReerKit dependency to 1.2.5 for Swift 5.10 compatibility |
| Tools/QsSwiftComparison/js/package.json | Updated qs reference library to 6.14.2 |
| Tools/QsSwiftComparison/js/package-lock.json | Updated qs package lock with new version and integrity hash |
| Sources/QsSwift/Internal/Encoder.swift | Added iterative deep fallback encoder, fixed Data charset handling in describe functions |
| Sources/QsSwift/Internal/Decoder+ParseList.swift | Added overflow fallback logic for comma lists on first occurrence |
| Sources/QsSwift/Internal/Decoder+ParseQuery.swift | Enhanced currentListLength calculation to include overflow maps, added isFirstOccurrence tracking |
| Sources/QsSwift/Internal/Utils+Merge.swift | Replaced recursive dictionary merge with iterative stack-based implementation |
| Sources/QsSwift/Internal/Utils.swift | Updated case pattern matching style for consistency |
| Sources/QsSwift/Internal/NSMapTable+Linux.swift | Added strongToStrongObjects support with enum-based storage mode dispatch |
| Sources/QsSwift/Models/DecodeOptions.swift | Reformatted typealias for better readability |
| Sources/QsSwift/Models/EncodeOptions.swift | Reformatted typealias for better readability |
| Sources/QsSwift/Qs+Encode.swift | Minor line break formatting for better readability |
| Sources/QsObjC/Models/DecodeOptionsObjC.swift | Added normalization for charset, parameterLimit, depth, and dot-key handling |
| Sources/QsObjC/Models/EncodeOptionsObjC.swift | Added charset normalization to default invalid values to utf8 |
| Sources/QsObjC/QsBridge+DecodeConvenience.swift | Formatting cleanup for function signatures |
| Tests/QsSwiftTests/DecodeTests.swift | Added tests for comma overflow fallback and deep merge scenarios |
| Tests/QsSwiftTests/EncodeTests.swift | Added tests for Data encoding, deep nesting, iterative fallback branches, and strictNullHandling |
| Tests/QsSwiftTests/UtilsTests.swift | Added deep merge test, minor formatting fixes (spacing, trailing commas) |
| Tests/QsSwiftTests/Fixtures/DummyEnum.swift | Removed trailing blank line |
| Tests/QsObjCTests/ObjCModelCoverageTests.swift | Added sanitization tests, formatting improvements |
| Tests/QsObjCTests/ObjCExampleTests.swift | Whitespace cleanup |
| Tests/QsObjCTests/ObjCEncodeTests.swift | Added test for Data encoding with encode=false |
| Tests/QsObjCTests/ObjCDecodeTests.swift | Added tests for comma overflow fallback behavior |
| Tests/QsObjCTests/ObjCBridgeTests.swift | Formatting improvements (trailing commas, line breaks) |
Files not reviewed (1)
- Tools/QsSwiftComparison/js/package-lock.json: Language not supported
![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: 776961a94e
ℹ️ 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".
…onary ordering in encoding
…ng fallback and custom decoders
|
@codex review |
…ictionariesIterative
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06df6aefd0
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Tests/QsSwiftTests/EncodeTests.swift`:
- Around line 4068-4108: The test
encoder_iterativeFallback_preservesDictionaryOrderingParity uses a plain
[String: Any] fixture (data) but performs an order-sensitive equality check
between recursiveParts and fallbackParts; replace data with an OrderedDictionary
preserving insertion order or change the final assertion to an order-insensitive
comparison (e.g., compare sets or sorted arrays) so the parity check does not
depend on unspecified Dictionary ordering; update references in encodeParts,
recursiveParts, and fallbackParts accordingly to keep the test deterministic.
…xtract logic to effectiveListLength function
…r and add corresponding tests
|
@codex review |
…in describeData function
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Sources/QsSwift/Internal/Utils`+Merge.swift:
- Around line 357-362: When assigning a nested merge result into a parent frame
(inside the while loop that pops stack frames and uses frame.pendingKey,
frame.target and completed), also update the parent overflow max if the parent
is an overflow target: detect when frame.target supports overflowMax (the
overflow dict type/property) and when childResult is either an overflow dict
(has its own overflowMax) or a plain dict with integer-string keys; compute the
child's max index (use child.overflowMax if present, otherwise parse integer
keys and take max) and set frame.target.overflowMax =
max(frame.target.overflowMax, childMax) before clearing frame.pendingKey and
completed so the parent’s stored max stays current when nested dictionaries are
merged.
|
Codex Review: Didn't find any major issues. More of your lovely PRs 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". |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/QsSwift/Internal/Encoder.swift (1)
758-765: Remove unnecessary Array allocation in UTF-8 decoding.
String(decoding: Array(data), as: UTF8.self)creates an unnecessary copy in a hot path.Datais aCollection<UInt8>and can be passed directly to the initializer.♻️ Suggested change
- return String(decoding: Array(data), as: UTF8.self) + return String(decoding: data, as: UTF8.self)
…tests for list-of-lists semantics
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Sources/QsSwift/Internal/Decoder`+ParseQuery.swift:
- Around line 287-299: The function overflowElementsAsArray currently allocates
an array of size maxIndex+1 based on comma-split indices which can be huge;
modify overflowElementsAsArray to guard against unbounded allocation by checking
the computed size against the parser's listLimit (or a sensible threshold) and
if maxIndex+1 exceeds that limit, avoid allocating the large array and instead
return the original sparse overflow map (or an empty/safe fallback) so
parseListValue can continue using the overflow form; reference
overflowElementsAsArray, Utils.intIndex, Utils.isOverflowKey and ensure the
check uses the same listLimit used by parseListValue so you cap or bypass
allocation when maxIndex+1 > listLimit.
🧹 Nitpick comments (4)
Sources/QsSwift/Internal/Encoder.swift (1)
757-767: Address SwiftLintoptional_data_string_conversionwarning on line 764.The use of
String(decoding:as:)here is intentional — it's the non-failable initializer that replaces malformed sequences with U+FFFD, keeping the payload visible. The failableString(bytes:encoding:)is already used on line 759. Consider adding an inline disable to silence the lint warning and document the intent:Proposed fix
if charset == .utf8 { // Keep payload visible for malformed UTF-8 instead of collapsing to empty. + // swiftlint:disable:next optional_data_string_conversion return String(decoding: data, as: UTF8.self) }Tests/QsSwiftTests/EncodeTests.swift (2)
4989-4993: Force-unwrap in custom encoder closure could crash if contract changes.Line 4992 force-unwraps
value!after checkingvalue == nilon Line 4991. In a test this is low-risk, but the== nilcheck onAny?can behave unexpectedly withOptional<Optional<…>>(double-wrapped optionals). A safer pattern isguard let v = value else { ... }.🔧 Safer unwrap
- if value == nil { return "null_token" } - return "v_\(String(describing: value!))" + guard let v = value else { return "null_token" } + return "v_\(String(describing: v))"
4831-4864: Large acyclic input test allocates 250 001 empty arrays — verify this doesn't slow CI.
Array(repeating: [Any](), count: 250_001)is intentionally large to test false-cycle detection, but this may cause noticeable test-suite slowdowns on constrained CI runners. Consider adding a comment noting the intentional size, or reducing to a still-large-but-cheaper count if the threshold being tested allows it.Tests/QsObjCTests/ObjCConvenienceTests.swift (1)
111-121:!Thread.isMainThreadassertion could be flaky in edge cases.Line 115 asserts
!Thread.isMainThread, butDispatchQueue.global().asyncdoesn't guarantee off-main execution in all runtime configurations (e.g., if the main thread happens to service the global queue). This is extremely unlikely in practice but worth noting. ThedecodeAsyncOnMaintest'sThread.isMainThreadassertion is safe because it explicitly dispatches toDispatchQueue.main.
…ommands, coding style, testing, and commit practices
…ions for large sparse maps
… improve value handling
1 participant
This pull request introduces several improvements and fixes to the Swift and Objective-C bridging layers for query string encoding and decoding. The most significant changes are improved normalization and validation of options, enhanced handling of list limits and overflow cases during decoding, and the addition of a deep iterative fallback for encoding very deeply nested structures. The update also ensures charset handling is consistent and robust, and upgrades a package dependency for Linux builds.
Decoding Improvements:
DecodeOptionsObjC, ensuring only supported charsets are used and limits are enforced with sensible minimums. [1] [2]Encoding Improvements:
EncodeOptionsObjC, defaulting to.utf8if an unsupported charset is provided. [1] [2]DatatoString, ensuring correct encoding in all cases. [1] [2] [3] [4]General Code Quality:
Dependency Update:
ReerKitpackage version for Linux builds from 1.2.2 to 1.2.5 in bothPackage.swiftandPackage@swift-5.10.swift. [1] [2]Summary by CodeRabbit
New Features
Bug Fixes
Chores