Conversation
WalkthroughAdds a path-segment percent-encoding utility and URL-building helpers in ChatAPI to escape Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant ChatAPI
participant Escaper as "String.encodePathSegment()"
participant Builder as "roomUrl / messageUrl"
participant HTTP as "HTTP Client"
participant Ably as "Ably REST"
App->>ChatAPI: invoke(action, roomName, serial?, body)
ChatAPI->>Escaper: escapePathSegment(roomName)
note right of Escaper #DDEBF7: percent-encode path segment
Escaper-->>ChatAPI: encodedRoomName
alt serial present
ChatAPI->>Escaper: escapePathSegment(serial)
note right of Escaper #FFF4E6: percent-encode serial
Escaper-->>ChatAPI: encodedSerial
ChatAPI->>Builder: messageUrl(encodedRoomName, encodedSerial, suffix)
else
ChatAPI->>Builder: roomUrl(encodedRoomName, suffix)
end
Builder-->>ChatAPI: pathWithEscapedSegments
ChatAPI->>HTTP: request(method, pathWithEscapedSegments, body)
HTTP->>Ably: REST call (encoded identifiers)
Ably-->>HTTP: response
HTTP-->>ChatAPI: result
ChatAPI-->>App: result / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used📓 Path-based instructions (3)**/*.swift📄 CodeRabbit inference engine (CLAUDE.md)
Files:
Sources/**/*.swift📄 CodeRabbit inference engine (CLAUDE.md)
Files:
Tests/**/*.swift📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (2)Tests/AblyChatTests/DefaultMessagesTests.swift (4)
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Sources/AblyChat/ChatAPI.swift (1)
18-20: Consider error handling when encoding fails.The fallback
?? roomNamereturns the original unencoded room name ifaddingPercentEncodingfails. While encoding failures are rare, returning an unencoded room name could lead to malformed API paths when room names contain reserved characters. Consider whether it's safer to throw an error when encoding fails, ensuring that problematic room names are caught early rather than potentially causing subtle API failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
Sources/AblyChat/ChatAPI.swift(7 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(4 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(9 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Tests/AblyChatTests/DefaultMessagesTests.swift (2)
Sources/AblyChat/DefaultMessages.swift (1)
subscribe(39-115)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (1)
Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: SPM (Xcode 26.0)
- GitHub Check: SPM,
releaseconfiguration (Xcode 26.0)
🔇 Additional comments (6)
Sources/AblyChat/ChatAPI.swift (2)
18-20: LGTM! Correct character set choice for encoding room names.The use of
urlHostAllowedis correct because it excludes '/' from the allowed characters, ensuring that '/' in room names gets percent-encoded to%2F. The comment clearly explains whyurlPathAllowedwould be incorrect (it includes '/' as a valid path character, which would break the REST paths when room names contain '/').
114-114: Verify if message serials require percent-encoding.The PR title mentions "Encode room name and serial," but only room names are currently being encoded. Message serials appear directly in paths without encoding at these lines:
- Line 114:
updateMessage-messages/\(modifiedMessage.serial)- Line 153:
deleteMessage-messages/\(message.serial)/delete- Line 195:
sendReactionToMessage-messages/\(messageSerial)/reactions- Line 213:
deleteReactionFromMessage-messages/\(messageSerial)/reactionsIf serials are server-controlled and guaranteed to be URL-safe (e.g., only alphanumeric), then the current implementation is correct. However, if serials can contain special characters that need encoding, they should be escaped similar to room names.
Please clarify whether the omission of serial encoding is intentional. If serials are guaranteed URL-safe, consider updating the PR title to reflect that only room names require encoding. If serials can contain special characters, they should also be encoded.
Also applies to: 153-153, 195-195, 213-213
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)
15-15: LGTM! Test correctly verifies room name encoding.The test properly validates that room names containing '/' are percent-encoded in API paths. The room name "basket/ball" is correctly encoded to "basket%2Fball" in the expected request paths for both POST and DELETE reactions operations.
Also applies to: 24-24, 36-36, 50-50
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
12-12: LGTM! Comprehensive test coverage for room name encoding.The tests properly validate that room names containing '/' are percent-encoded across all message operations (send, update, delete, and get). The room name "basket/ball" is consistently encoded to "basket%2Fball" in the expected API request paths, verifying the correct behavior of the
escapeRoomNamehelper.Also applies to: 37-37, 51-51, 57-57, 83-83, 101-101, 107-107, 132-132, 148-148, 224-224, 237-237, 244-244
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (2)
9-9: LGTM! Test correctly verifies occupancy endpoint encoding.The test properly validates that the room name "basket/ball" is percent-encoded to "basket%2Fball" in the occupancy API endpoint path. The added assertion using
realtime.callRecorder.hasRecordprovides clear verification of the encoded path in the HTTP request.Also applies to: 28-28, 43-52
158-159: Good catch fixing the incorrect await usage.Removing
awaitfrom the#expectcall is correct sincedoIt()invokes the synchronousdefaultOccupancy.current()method. The previous code likely hadawaitincorrectly. This is a minor cleanup that improves code correctness, even though it's unrelated to the main PR objective.
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
a58c6c8 to
3122c33
Compare
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
7d03929 to
e97f785
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Sources/AblyChat/ChatAPI.swift (3)
18-25: Make the allowed character set explicit and avoid shadowing the parameter.Using urlHostAllowed works to encode
/, but it’s implicit. Prefer urlPathAllowed minus/and avoid reusing theroomNameidentifier for the encoded value.Apply:
- private func escapeRoomName(_ roomName: String) -> String { - // Using `NSCharacterSet.URLHostAllowedCharacterSet`, because it doesn't include '/', which can be used in room names across other platforms. - // Whereas `URLPathAllowedCharacterSet` includes '/' as a valid path character, so when a room name contains it, it is not encoded and the `REST` path becomes broken. - guard let roomName = roomName.addingPercentEncoding(withAllowedCharacters: CharacterSet.urlHostAllowed) else { - fatalError("Room name '\(roomName)' can't be percent encoded.") - } - return roomName - } + private func escapeRoomName(_ roomName: String) -> String { + // Explicitly disallow "/" so it is percent-encoded in a path segment. + var allowed = CharacterSet.urlPathAllowed + allowed.remove(charactersIn: "/") + guard let encoded = roomName.addingPercentEncoding(withAllowedCharacters: allowed) else { + fatalError("Room name '\(roomName)' can't be percent encoded.") + } + return encoded + }
27-32: Encode “:” in serials explicitly and avoid shadowing the parameter.Tests expect “:” to be encoded. Make this explicit by removing both “/” and “:” from the allowed set, and rename the local.
Apply:
- private func escapeSerial(_ serial: String) -> String { - guard let serial = serial.addingPercentEncoding(withAllowedCharacters: CharacterSet.urlPathAllowed) else { - fatalError("Serial '\(serial)' can't be percent encoded.") - } - return serial - } + private func escapeSerial(_ serial: String) -> String { + var allowed = CharacterSet.urlPathAllowed + allowed.remove(charactersIn: "/:") + guard let encoded = serial.addingPercentEncoding(withAllowedCharacters: allowed) else { + fatalError("Serial '\(serial)' can't be percent encoded.") + } + return encoded + }
18-25: Avoid fatalError in library code; consider throwing instead.fatalError will crash the host app on an unexpected encoding issue. Prefer a thrown InternalError (e.g., ChatError.invalidURLComponentEncoding) and plumb it through callers, which already throw.
If desired, I can draft the change to make these helpers
throwsand adjust the call sites.Also applies to: 27-32
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
12-12: Optional: add coverage for more reserved characters.Consider adding a test case with room names containing spaces or “?#[]” to guard against regressions in percent-encoding.
Also applies to: 226-227
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
Sources/AblyChat/ChatAPI.swift(7 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(6 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(9 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(3 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Tests/AblyChatTests/ChatAPITests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
- Tests/AblyChatTests/DefaultMessageReactionsTests.swift
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/AblyChatTests/DefaultMessagesTests.swift (2)
Sources/AblyChat/DefaultMessages.swift (3)
update(134-140)delete(142-148)subscribe(39-115)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)
🔇 Additional comments (13)
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
61-64: Mock serial update aligns with encoded serial expectations.Consistent with API path encoding tests. LGTM.
Sources/AblyChat/ChatAPI.swift (8)
36-36: getMessages path now encodes roomName.Correct usage of escapeRoomName. LGTM.
90-90: sendMessage path now encodes roomName.Looks good. LGTM.
126-126: updateMessage encodes roomName and serial.Matches spec/test expectations (slashes and “:” encoded). LGTM.
165-165: deleteMessage encodes roomName and serial.Correct and consistent. LGTM.
196-196: getOccupancy encodes roomName.Consistent with other endpoints. LGTM.
207-207: sendReactionToMessage encodes roomName and serial.Path construction looks correct. LGTM.
225-225: deleteReactionFromMessage encodes roomName and serial.Consistent with sendReaction. LGTM.
34-37: All room and serial path interpolations use helpers
No unescapedroomNameorserialendpoints found.Tests/AblyChatTests/DefaultMessagesTests.swift (4)
12-12: Send message tests updated for encoded room path.Using “basket/ball” and asserting “basket%2Fball” is correct. LGTM.
Also applies to: 37-37, 51-52
57-59: Update message tests cover both roomName and serial encoding.New serial format and encoded “:” in path are asserted. LGTM.
Also applies to: 67-67, 85-86, 94-94, 102-103
108-110: Delete message tests reflect encoded roomName and serial.Expectations match the new path construction. LGTM.
Also applies to: 118-119, 121-121, 134-136, 142-143, 150-151
226-227: Subscription history uses encoded room path.GET path assertion with “basket%2Fball” is correct. LGTM.
Also applies to: 239-239, 246-247
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
e97f785 to
2816294
Compare
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
2816294 to
40e169a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
107-150: Serial in URL path requires encoding.Same issue as the update test: the expected path on line 148 contains the unencoded serial
123456789-000@123456789:000. The reserved characters@and:must be percent-encoded.Expected:
/chat/v4/rooms/basket%2Fball/messages/123456789-000%40123456789%3A000/delete
🧹 Nitpick comments (1)
Sources/AblyChat/ChatAPI.swift (1)
18-24: Clarify fatalError message for general path segments.
SinceescapePathSegmenthandles both room names and serials, the crash text mentioning only “Serial” can mislead when the failing input is a room name. Please switch to a neutral noun like “Segment” for accuracy.- guard let escaped = segment.addingPercentEncoding(withAllowedCharacters: characterSet) else { - fatalError("Serial '\(segment)' can't be percent encoded.") + guard let escaped = segment.addingPercentEncoding(withAllowedCharacters: characterSet) else { + fatalError("Segment '\(segment)' can't be percent encoded.") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
Sources/AblyChat/ChatAPI.swift(7 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(6 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(9 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(3 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/AblyChatTests/ChatAPITests.swift
🧰 Additional context used
🧬 Code graph analysis (4)
Tests/AblyChatTests/IntegrationTests.swift (4)
Tests/AblyChatTests/Helpers/Sandbox.swift (1)
createAPIKey(30-49)Sources/AblyChat/Rooms.swift (3)
get(67-70)get(174-281)release(335-373)Sources/AblyChat/Room.swift (6)
onStatusChange(150-164)onStatusChange(167-169)onStatusChange(398-401)attach(372-378)detach(381-387)release(389-394)Sources/AblyChat/DefaultMessages.swift (2)
subscribe(39-115)send(126-132)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)
Tests/AblyChatTests/DefaultRoomTests.swift (1)
channel(9-19)
Tests/AblyChatTests/DefaultMessagesTests.swift (3)
Sources/AblyChat/DefaultMessages.swift (3)
update(134-140)delete(142-148)subscribe(39-115)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)Sources/AblyChat/Subscription.swift (1)
historyBeforeSubscribe(94-109)
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (1)
Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Xcode, macOS (Xcode 26.0)
🔇 Additional comments (3)
Tests/AblyChatTests/DefaultMessagesTests.swift (3)
12-53: LGTM! Room name encoding is correctly verified.The test properly validates that room names containing "/" are percent-encoded to "%2F" in the API path. The spec annotation correctly links to CHA-RST6.
224-246: LGTM! Subscription history request correctly uses encoded room name.The test properly validates that room names are encoded in history API requests. The serial appears as a query parameter (
fromSerial), not in the path, so the encoding rules differ.
57-103: Original suggestion to encode@and:is incorrect
TheescapePathSegmenthelper intentionally percent-encodes only/(removing it fromurlPathAllowed);@and:are valid in path segments per RFC 3986 and should remain unencoded. No changes needed.Likely an incorrect or invalid review comment.
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
40e169a to
833bab3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
Sources/AblyChat/ChatAPI.swift(7 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(6 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(9 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(3 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/AblyChatTests/IntegrationTests.swift
- Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
🧰 Additional context used
🧬 Code graph analysis (2)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)
Tests/AblyChatTests/DefaultRoomTests.swift (1)
channel(9-19)
Tests/AblyChatTests/DefaultMessagesTests.swift (3)
Sources/AblyChat/DefaultMessages.swift (3)
update(134-140)delete(142-148)subscribe(39-115)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)Sources/AblyChat/Subscription.swift (1)
historyBeforeSubscribe(94-109)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: SPM (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: SPM,
releaseconfiguration (Xcode 26.0) - GitHub Check: check-documentation
- GitHub Check: lint
🔇 Additional comments (18)
Sources/AblyChat/ChatAPI.swift (8)
27-33: Well-structured URL construction helpers.The extraction of common path prefixes into helper methods (
roomUrl,messageUrl) addresses the feedback from the past review comments and improves code maintainability. The spec reference (CHA-RST6) is also appropriately included.Based on past review comments suggesting helper extraction similar to the JS implementation.
37-37: LGTM!Correctly uses the
roomUrlhelper for constructing the messages endpoint with proper escaping.
91-91: LGTM!Correctly uses the
roomUrlhelper for constructing the send message endpoint with proper escaping.
127-127: LGTM!Correctly uses the
messageUrlhelper for constructing the update message endpoint with proper escaping of both room name and serial.
166-166: LGTM!Correctly uses the
messageUrlhelper for constructing the delete message endpoint with proper escaping of both room name and serial.
197-197: LGTM!Correctly uses the
escapePathSegmenthelper for encoding the room name in the occupancy endpoint.
208-208: LGTM!Correctly uses the
messageUrlhelper for constructing the reaction endpoint with proper escaping of both room name and serial.
226-226: LGTM!Correctly uses the
messageUrlhelper for constructing the delete reaction endpoint with proper escaping of both room name and serial.Tests/AblyChatTests/ChatAPITests.swift (1)
48-54: Test expectations correctly updated.The expected serial values have been updated to match the new format "123456789-000@123456789:000" used in the mock responses, ensuring test consistency.
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
61-63: Mock data correctly updated.The serial format has been updated to "123456789-000@123456789:000" to match the expected format used throughout the tests, ensuring consistency between mocks and test expectations.
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (4)
15-15: Good addition of spec reference.The spec marker
@specOneOf(5/6) CHA-RST6correctly identifies this as one of the tests covering the escaping requirement.
24-24: Effective test case for escaping.Changing the room name to "basket/ball" specifically tests the escaping behavior by introducing a forbidden character ("/") that must be percent-encoded.
36-36: Correct path assertions for encoded values.The expected paths correctly show the encoded room name "basket%2Fball" (slash encoded as %2F) and the new serial format "123456789-000@123456789:000", verifying that the escaping is working as intended.
Also applies to: 50-50
357-357: Verify why these tests don't use the escaped room name.These tests use "basketball" (no slash) instead of "basket/ball". While this is likely intentional (testing default reaction types without needing to verify escaping), the inconsistency should be verified.
The paths also don't show the encoded room name, which is correct for "basketball", but the serial format is updated to the new format.
Is this intentional that only the first test (
sendAndDeleteReactionForMessage) uses "basket/ball" to test escaping, while the remaining tests use "basketball"? This appears to be an appropriate test design decision (testing escaping in one place and other features separately), but please confirm this is the intended pattern.Also applies to: 396-396
Tests/AblyChatTests/DefaultMessagesTests.swift (4)
12-12: Comprehensive spec marker coverage.The spec markers
@specOneOf(1/6),@specOneOf(2/6),@specOneOf(3/6), and@specOneOf(4/6)for CHA-RST6 correctly identify multiple test cases covering the escaping requirement across different message operations (send, update, delete, get).Also applies to: 57-57, 107-107, 224-224
37-37: Consistent use of test room name with slash.The room name "basket/ball" is consistently used in tests that verify escaping behavior, ensuring the forbidden character "/" is properly percent-encoded in the API paths.
Also applies to: 83-83, 132-132, 237-237
51-51: Correctly encoded paths in test assertions.The expected paths properly show:
- Encoded room name: "basket%2Fball" (slash as %2F)
- Encoded serial: "123456789-000@123456789:000"
This verifies that the escaping helpers are working correctly across different message operations.
Also applies to: 101-101, 148-148, 244-244
161-161: Intentional test pattern: mixing escaped and non-escaped test cases.These tests use "basketball" (no special characters) rather than "basket/ball". This appears to be an intentional design where:
- Some tests specifically verify escaping behavior (using "basket/ball")
- Other tests focus on different aspects (error handling) without needing to test escaping
This is a reasonable test organization strategy, but ensure that the integration test mentioned in past review comments is added to verify end-to-end behavior with ably-cocoa.
As per the past review comments, an integration test should be added in
IntegrationTests.swiftto confirm that ably-cocoa is not doing any double-escaping with the path. Please verify that this integration test has been added or is planned to be added.Also applies to: 184-184, 208-208
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
Sources/AblyChat/ChatAPI.swift(7 hunks)Sources/AblyChat/Extensions/String+Extensions.swift(1 hunks)Tests/AblyChatTests/StringEncodeTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/AblyChat/ChatAPI.swift
🧰 Additional context used
🧬 Code graph analysis (1)
Tests/AblyChatTests/StringEncodeTests.swift (1)
Sources/AblyChat/Extensions/String+Extensions.swift (1)
encodeURIComponent(23-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: SPM (Xcode 26.0)
- GitHub Check: SPM,
releaseconfiguration (Xcode 26.0)
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
6a09775 to
9b08196
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/367/AblyChat
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Sources/AblyChat/ChatAPI.swift (1)
18-23: Previous critical issues remain unresolved.Two critical issues flagged in past review comments are still present:
fatalError will crash the app: Using
fatalErrorwhen percent-encoding fails will crash the application. While encoding failure is unlikely with valid UTF-8 strings, it should still be handled gracefully by throwing an error or returning a safe fallback.Misleading error message: The error message specifically mentions "Serial" but this function is also used for room names (see lines 26, 30, 125), making the message incorrect and misleading.
Both issues were previously flagged in past review comments and remain unaddressed.
🧹 Nitpick comments (2)
Sources/AblyChat/ChatAPI.swift (1)
125-125: Consider using roomUrl helper for consistency.The
getOccupancyendpoint constructs the path inline rather than using theroomUrlhelper. While this works, using the helper would improve consistency across the codebase.Apply this diff to use the helper:
- let endpoint = "\(apiVersionV4)/rooms/\(escapePathSegment(roomName))/occupancy" + let endpoint = roomUrl(roomName: roomName, suffix: "/occupancy")Tests/AblyChatTests/StringEncodeTests.swift (1)
1-161: Review encoding function in ChatAPI
ChatAPI.swift usesencodePathSegment(extraUnescaped: ":@"), which permits additional characters (e.g.$&'()*+,;=) compared toencodeURIComponent. Confirm that path-segment encoding is intended; if not, switch toencodeURIComponentor update the tests accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
Sources/AblyChat/ChatAPI.swift(7 hunks)Sources/AblyChat/Extensions/String+Extensions.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(6 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(11 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(3 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(1 hunks)Tests/AblyChatTests/StringEncodeTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Tests/AblyChatTests/DefaultMessageReactionsTests.swift
- Tests/AblyChatTests/IntegrationTests.swift
- Sources/AblyChat/Extensions/String+Extensions.swift
- Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
🧰 Additional context used
🧬 Code graph analysis (3)
Tests/AblyChatTests/DefaultMessagesTests.swift (3)
Sources/AblyChat/DefaultMessages.swift (3)
update(134-140)delete(142-148)subscribe(39-115)Sources/AblyChat/Subscription.swift (1)
historyBeforeSubscribe(94-109)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)
Sources/AblyChat/ChatAPI.swift (1)
Sources/AblyChat/Extensions/String+Extensions.swift (1)
encodePathSegment(15-25)
Tests/AblyChatTests/StringEncodeTests.swift (1)
Sources/AblyChat/Extensions/String+Extensions.swift (1)
encodeURIComponent(39-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: SPM (Xcode 26.0)
- GitHub Check: SPM,
releaseconfiguration (Xcode 26.0)
🔇 Additional comments (6)
Sources/AblyChat/ChatAPI.swift (2)
25-31: LGTM! Well-designed helper functions.The
roomUrlandmessageUrlhelpers provide clean abstractions for building API endpoints with properly escaped path segments. This centralizes the encoding logic and ensures consistency across all REST endpoint constructions.
35-35: Endpoint updates look good.All message-related endpoints now properly use the
roomUrlandmessageUrlhelpers for consistent path encoding. The changes align with the PR objectives to encode room names and serials in API calls.Also applies to: 61-61, 80-80, 108-108, 136-136, 154-154
Tests/AblyChatTests/ChatAPITests.swift (1)
49-49: LGTM! Test data updated to reflect realistic serial format.The serial values have been updated from "3446456" to "123456789-000@123456789:000", which includes
:and@characters. This more realistic format aligns with the encoding behavior being tested and is consistent with mock response updates across the PR.Also applies to: 55-55
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
62-62: LGTM! Mock response data updated consistently.The serial values in the mock response have been updated to "123456789-000@123456789:000", which is consistent with the test data changes in ChatAPITests.swift and supports testing of the encoding behavior with special characters in serials.
Also applies to: 68-68
Tests/AblyChatTests/DefaultMessagesTests.swift (2)
12-12: Excellent test coverage for room name encoding.The tests have been updated to validate encoding behavior with room names containing special characters (e.g., "basket/ball" → "basket%2Fball"). The changes include:
- Room name changed to "basket/ball" to test encoding of
/character- Expected API paths updated to use encoded form "basket%2Fball"
- Proper spec annotations added (CHA-RST6) to document escaping requirements
specOneOfmarkers track coverage across different API operations (send, update, delete, get)The test data changes are consistent across all message operations and properly validate the encoding behavior.
Also applies to: 39-39, 56-56, 62-62, 93-93, 115-115, 121-121, 150-150, 169-169
71-71: Serial format updated consistently.The serial values have been updated from simple numbers ("0", "1") to the realistic format "123456789-000@123456789:000" which includes
:and@characters. This validates that the encoding implementation properly handles these special characters in message serials.Also applies to: 95-95, 152-152
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
9b08196 to
c2dbc5d
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/367/AblyChat
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
c2dbc5d to
b86d53f
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/367/AblyChat
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/AblyChat/ChatAPI.swift (1)
18-23: Replace fatalError with throwing; fix misleading message.Crashing on encoding failure is unacceptable and the message incorrectly mentions "Serial" even for room names. Make this function throw and propagate.
Apply this change:
- private func escapePathSegment(_ segment: String) -> String { - guard let escaped = segment.encodePathSegment(extraUnescaped: ":@") else { - fatalError("Serial '\(segment)' can't be percent encoded.") - } - return escaped - } + private func escapePathSegment(_ segment: String) throws -> String { + guard let escaped = segment.encodePathSegment(extraUnescaped: ":@") else { + throw ChatError.percentEncodingFailed + } + return escaped + }Also add a new error (in ChatError) and update call sites to use
tryand propagate via existingthrows(InternalError)methods:// Add to ChatError: case percentEncodingFailed // Examples of call-site updates: let endpoint = try roomUrl(roomName: roomName, suffix: "/messages") let endpoint = try messageUrl(roomName: roomName, serial: modifiedMessage.serial) let endpoint = "\(apiVersionV4)/rooms/\(try escapePathSegment(roomName))/occupancy"I can provide a full patch touching all call sites if desired.
🧹 Nitpick comments (2)
Sources/AblyChat/ChatAPI.swift (1)
124-126: Use roomUrl for consistency.Minor consistency: build occupancy path via roomUrl.
- let endpoint = "\(apiVersionV4)/rooms/\(escapePathSegment(roomName))/occupancy" + let endpoint = roomUrl(roomName: roomName, suffix: "/occupancy")Sources/AblyChat/Extensions/String+Extensions.swift (1)
10-15: Doc nit: clarify pct-encoded.Update comment to reflect “pct-encoded = '%' HEXDIG HEXDIG”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
Sources/AblyChat/ChatAPI.swift(7 hunks)Sources/AblyChat/Extensions/String+Extensions.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(6 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(11 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(3 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(1 hunks)Tests/AblyChatTests/StringEncodeTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Tests/AblyChatTests/ChatAPITests.swift
- Tests/AblyChatTests/IntegrationTests.swift
- Tests/AblyChatTests/DefaultMessageReactionsTests.swift
- Tests/AblyChatTests/StringEncodeTests.swift
🧰 Additional context used
🧬 Code graph analysis (3)
Sources/AblyChat/ChatAPI.swift (1)
Sources/AblyChat/Extensions/String+Extensions.swift (1)
encodePathSegment(15-25)
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (1)
Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)
Tests/AblyChatTests/DefaultMessagesTests.swift (3)
Sources/AblyChat/DefaultMessages.swift (3)
update(134-140)delete(142-148)subscribe(39-115)Sources/AblyChat/Subscription.swift (1)
historyBeforeSubscribe(94-109)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: SPM,
releaseconfiguration (Xcode 26.0) - GitHub Check: SPM (Xcode 26.0)
- GitHub Check: lint
🔇 Additional comments (9)
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
62-62: LGTM: mock serials updated to new format.Consistent with tests expecting unescaped ":" and "@" in serial path segments.
Also applies to: 68-68
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (1)
9-9: Good coverage of encoded room path.Test now exercises "/" encoding and asserts the exact encoded path. Looks good.
Also applies to: 28-28, 43-52
Sources/AblyChat/ChatAPI.swift (2)
25-31: Helper extraction for path building looks good.Centralizes path construction and reduces duplication.
35-36: Endpoints now use encoded room/serial segments.Correct application of helpers across endpoints.
Please confirm this encoding policy (leaving ":" and "@" unescaped in serials) matches the final spec decision so we don’t need to flip behavior later (encodeURIComponent vs RFC 3986 pchar).
Also applies to: 61-61, 80-81, 108-109, 136-137, 154-155
Sources/AblyChat/Extensions/String+Extensions.swift (1)
40-107: encodeURIComponent implementation looks correct.UTF‑16 iteration, surrogate handling, and UTF‑8 percent-encoding align with ECMAScript. Nice work.
Tests/AblyChatTests/DefaultMessagesTests.swift (4)
12-13: Send: asserts encoded room path correctly.Good use of “basket/ball” and expectation for “basket%2Fball”.
Also applies to: 39-39, 56-56
63-116: Update: serial shape and path assertion updated appropriately.Tests align with keeping ":" and "@" unescaped in serial path segments.
Please confirm this matches the spec decision so we don’t need to rework tests if encodeURIComponent semantics are chosen.
121-171: Delete: encoded room path and serial expectations updated.Assertions look consistent with the new path helpers.
245-246: History (before subscribe): encoded room path asserted.Correct verification of GET to “/rooms/basket%2Fball/messages”.
Also applies to: 258-259, 265-266
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
b86d53f to
22517ea
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/367/AblyChat
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/AblyChat/Extensions/String+Extensions.swift (1)
15-25: Past concern about "%" has been addressed.The previous review flagged including "%" in the allowed characters, but the current implementation at line 16 correctly omits it. However, note that this function will double-encode any pre-encoded input (e.g.,
"room%2Fname"becomes"room%252Fname").Verify that all callers pass raw (non-encoded) room names and serials:
#!/bin/bash # Search for calls to encodePathSegment rg -nP '\bencodePathSegment\b' -A3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
Sources/AblyChat/ChatAPI.swift(7 hunks)Sources/AblyChat/Extensions/String+Extensions.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(6 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(11 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(3 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(1 hunks)Tests/AblyChatTests/StringEncodeTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift
- Tests/AblyChatTests/StringEncodeTests.swift
- Sources/AblyChat/ChatAPI.swift
🧰 Additional context used
🧬 Code graph analysis (3)
Tests/AblyChatTests/IntegrationTests.swift (5)
Tests/AblyChatTests/Helpers/Sandbox.swift (1)
createAPIKey(30-49)Sources/AblyChat/Rooms.swift (3)
get(60-63)get(164-271)release(325-363)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (6)
get(224-240)get(242-258)get(454-457)attach(366-380)detach(382-396)release(459-461)Sources/AblyChat/Room.swift (3)
attach(372-378)detach(381-387)release(389-394)Sources/AblyChat/DefaultMessages.swift (2)
subscribe(39-115)send(126-132)
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (1)
Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)
Tests/AblyChatTests/DefaultMessagesTests.swift (3)
Sources/AblyChat/DefaultMessages.swift (3)
update(134-140)delete(142-148)subscribe(39-115)Sources/AblyChat/Subscription.swift (1)
historyBeforeSubscribe(94-109)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: SPM,
releaseconfiguration (Xcode 26.0) - GitHub Check: SPM (Xcode 26.0)
🔇 Additional comments (10)
Sources/AblyChat/Extensions/String+Extensions.swift (3)
3-7: LGTM!The
URIErrorenum is appropriately scoped and provides a clear error case for handling unpaired surrogates during encoding operations.
115-144: LGTM!The
codePointAthelper correctly implements ECMAScript's CodePointAt abstract operation with proper surrogate pair detection and code point calculation per the Unicode standard.
40-107: Clarify encodeURIComponent usage
- encodeURIComponent is only referenced in tests and has no production callers. If it’s meant to be part of the public API, add its usage where necessary; otherwise remove the unused function (and its tests).
- Decide whether room names and serials should be encoded with encodePathSegment or encodeURIComponent.
Tests/AblyChatTests/ChatAPITests.swift (1)
49-55: LGTM!The updated serial values align with the new mock response format and are consistent with the encoding changes throughout the test suite.
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (1)
9-9: LGTM!The test correctly verifies that room names containing "/" are percent-encoded in the API path. The encoded path
basket%2Fballproperly represents the slash character, and the spec annotation clearly documents the test coverage for CHA-RST6.Also applies to: 28-28, 43-52
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)
15-15: LGTM!The test updates properly verify that reaction endpoints encode both room names and message serials. The encoded paths (
basket%2Fballand123456789-000@123456789:000) align with the escaping requirements for CHA-RST6.Also applies to: 24-24, 36-36, 50-50
Tests/AblyChatTests/DefaultMessagesTests.swift (4)
12-12: LGTM!The send message test correctly verifies encoded room name in the POST path. The spec annotation documents coverage for CHA-RST6 escaping requirements.
Also applies to: 39-39, 56-56
62-62: LGTM!The update message test properly verifies encoded room name and serial in the PUT path. The updated serial format
123456789-000@123456789:000aligns with the new mock response structure.Also applies to: 71-71, 93-93, 95-95, 103-103, 115-115
121-121: LGTM!The delete message test correctly verifies encoded room name and serial in the DELETE path. The serial updates are consistent with the new mock response format.
Also applies to: 130-130, 133-133, 150-150, 152-152, 158-159, 169-169
245-245: LGTM!The subscription point test properly verifies encoded room name in the history GET path. The spec annotation documents coverage for CHA-RST6 escaping in subscription operations.
Also applies to: 258-258, 265-265
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
22517ea to
a6f0cb3
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/367/AblyChat
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
a6f0cb3 to
8bd03c4
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/367/AblyChat
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Tests/AblyChatTests/IntegrationTests.swift (1)
516-580: Extract shared helper to remove duplication with basicIntegrationTest.This test repeats setup/attach/send/verify/detach/release flow. Factor common steps into a private helper and call it with roomName/log labels. This mirrors the earlier review guidance.
🧹 Nitpick comments (3)
Sources/AblyChat/Extensions/String+Extensions.swift (2)
15-22: encodePathSegment: RFC comment vs implementation (missing ":" and "@").RFC 3986 pchar includes ":" and "@", but the allowed set omits them; they’ll be percent‑encoded. Either include them or adjust the comment to reflect a stricter policy. If you intend to allow pchar fully, update as below.
- let characters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~!$&'()*+,;=" + extraUnescaped + // pchar = unreserved / pct-encoded / sub-delims / ":" / "@" + let characters = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~!$&'()*+,;=:@" + extraUnescaped
37-51: Prevent raw “%” via extraUnescaped in encodeURIComponent.If callers pass "%" in extraUnescaped, raw percent escapes slip through. Sanitize extraUnescaped to drop "%".
- // 4. Let unescapedSet be the string-concatenation of alwaysUnescaped and extraUnescaped. - let unescapedSet = Set(alwaysUnescaped + extraUnescaped) + // 4. Let unescapedSet be the string-concatenation of alwaysUnescaped and extraUnescaped, + // but never allow raw "%" to be unescaped. + let sanitizedExtra = extraUnescaped.filter { $0 != "%" } + let unescapedSet = Set(alwaysUnescaped + sanitizedExtra)Tests/AblyChatTests/StringEncodeTests.swift (1)
5-161: Add tests for encodePathSegment and raw percent handling.Consider:
- encodePathSegment: assert "/" encoded, and decide whether ":" and "@" should be unescaped (align tests with the chosen policy in String+Extensions.swift).
- Ensure "%" is encoded (to prevent raw percent leakage).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
Sources/AblyChat/ChatAPI.swift(7 hunks)Sources/AblyChat/Extensions/String+Extensions.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessageReactionsTests.swift(6 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(11 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(3 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(1 hunks)Tests/AblyChatTests/StringEncodeTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift
- Sources/AblyChat/ChatAPI.swift
- Tests/AblyChatTests/ChatAPITests.swift
- Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
🧰 Additional context used
🧬 Code graph analysis (3)
Tests/AblyChatTests/DefaultMessagesTests.swift (3)
Sources/AblyChat/DefaultMessages.swift (3)
update(134-140)delete(142-148)subscribe(39-115)Sources/AblyChat/Subscription.swift (1)
historyBeforeSubscribe(94-109)Tests/AblyChatTests/Helpers/Helpers.swift (1)
hasRecord(152-163)
Tests/AblyChatTests/IntegrationTests.swift (4)
Tests/AblyChatTests/Helpers/Sandbox.swift (1)
createAPIKey(30-49)Sources/AblyChat/Rooms.swift (3)
get(60-63)get(164-271)release(325-363)Sources/AblyChat/Room.swift (6)
onStatusChange(150-164)onStatusChange(167-169)onStatusChange(398-401)attach(372-378)detach(381-387)release(389-394)Sources/AblyChat/DefaultMessages.swift (2)
subscribe(39-115)send(126-132)
Tests/AblyChatTests/StringEncodeTests.swift (1)
Sources/AblyChat/Extensions/String+Extensions.swift (1)
encodeURIComponent(37-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: SPM (Xcode 26.0)
- GitHub Check: SPM,
releaseconfiguration (Xcode 26.0)
🔇 Additional comments (5)
Tests/AblyChatTests/DefaultMessageReactionsTests.swift (1)
24-29: Paths correctly encode room name and preserve serial punctuation.Good: room "basket/ball" → "basket%2Fball", and serial keeps "@" and ":" unencoded in the path as pchar.
Also applies to: 32-37, 47-51
Tests/AblyChatTests/DefaultMessagesTests.swift (4)
39-57: LGTM: room name is correctly percent‑encoded in send path.basket/ball → basket%2Fball and body/params unchanged.
91-116: LGTM: update path encodes room name and preserves serial punctuation.Path uses basket%2Fball and leaves "@", ":" in serial unencoded as intended.
148-170: LGTM: delete path encodes room name and keeps serial punctuation.Matches the new encoding strategy for room segments.
258-266: LGTM: history path encodes room segment.GET to /rooms/basket%2Fball/messages with expected params.
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
8bd03c4 to
d7508fe
Compare
…ete message reactions and getting occupancy.
maratal
force-pushed
the
360-escape-roomName-and-serial
branch
from
d7508fe to
334a395
Compare
lawrence-forooghian
left a comment
lawrence-forooghian
left a comment
There was a problem hiding this comment.
LGTM, just waiting to come to a conclusion on the spec (have asked people to take another look)
2 participants
Closes #360
Due to ongoing discussion here I've introduced two functions:
encodePathSegment(extraUnescaped:)takes explicit character set from here https://datatracker.ietf.org/doc/html/rfc3986#section-3.3 (deciphered by Lawrence in his comment)encodeURIComponent(extraUnescaped:)implemented by Claude by feeding formal description into it taken from this Paddy's comment@lawrence-forooghian Please chose one when reviewing.
Summary by CodeRabbit
New Features
Bug Fixes
Tests