[ECO-5116] Switch to V2 Chat API #122
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request includes updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Looks good. Only things to add would be:
|
|
yeah that's right. I'd also get rid of the actual functions itself e.g. |
maratal
force-pushed
the
ECO-5116-V2-Chat-API
branch
from
5fa1257 to
6c1bb45
Compare
|
@umair-ably I've pushed draft version of all requested changes (apart from those in cocoa). You can preview it now. |
maratal
force-pushed
the
ECO-5116-V2-Chat-API
branch
4 times, most recently
from
7979e8f to
2044aaf
Compare
maratal
force-pushed
the
ECO-5116-V2-Chat-API
branch
4 times, most recently
from
891a63b to
84bd031
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
Sources/AblyChat/Events.swift (1)
6-21: Consider enhancing the fromAction method implementation.While the implementation is functionally correct, there are a few improvements that could make it more robust and maintainable:
- The implicit return on line 9 could be more explicit
- The comment about ignoring other actions could be moved to documentation
- Consider adding logging for ignored actions in development
internal static func fromAction(_ action: ARTMessageAction) -> Self? { + /// Currently only supporting chat.message events, other actions are ignored switch action { case .create: - .new + return .new // ignore any other actions except `chat.message` for now case .unset, .update, .delete, .annotationCreate, .annotationDelete, .metaOccupancy: + #if DEBUG + print("Ignored message action: \(action)") + #endif nil @unknown default: + #if DEBUG + print("Unknown message action: \(action)") + #endif nil } }Tests/AblyChatTests/MessageSubscriptionTests.swift (2)
29-29: Consider improving test data meaningfulnessWhile the Message initialization correctly uses the new parameters, consider these improvements:
- Use meaningful test values instead of empty strings for
serialandclientID- Consider adding test cases with different
latestActionvalues to ensure proper handling- Message(serial: "", latestAction: .new, clientID: "", roomID: "", text: text, createdAt: .init(), metadata: [:], headers: [:]) + Message( + serial: "test-serial-\(text.lowercased())", + latestAction: .new, + clientID: "test-client-1", + roomID: "test-room-1", + text: text, + createdAt: .init(), + metadata: ["testKey": "testValue"], + headers: ["X-Test": "true"] + )
Line range hint
32-65: Enhance test coverage for V2 Chat API changesThe test suite should be expanded to cover the new V2 Chat API functionality:
- Add test cases for different
latestActionvalues- Add tests for message ordering using
serialcomparisonConsider adding these test cases:
@Test func messageOrdering() async { let messages = [ Message(serial: "1", latestAction: .new, text: "First"), Message(serial: "2", latestAction: .new, text: "Second") ] #expect(messages[0].serial < messages[1].serial) } @Test func differentLatestActions() async { let subscription = MessageSubscription(bufferingPolicy: .unbounded) { _ in fatalError("Not implemented") } let message = Message(serial: "1", latestAction: .edited, text: "Edited") subscription.emit(message) #expect(await subscription.first?.latestAction == .edited) }Tests/AblyChatTests/MessageTests.swift (1)
Line range hint
1-89: Consider adding edge case testsWhile the basic comparison tests are good, consider adding tests for:
- Messages with identical timestamps but different suffixes
- Messages with different prefixes
- Boundary cases for timestamp values
Would you like me to help create additional test cases for these scenarios?
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (2)
Line range hint
64-69: Fix incorrect status code in successSendMessage mockThe mock is named
successSendMessagebut has a status code of 500 (Server Error). This contradicts the success scenario and would cause thesuccessproperty to returnfalse.static let successSendMessage = MockHTTPPaginatedResponse( items: [ [ "serial": "3446456", "createdAt": 1_631_840_000_000, "text": "hello", ], ], - statusCode: 500, + statusCode: 200, headers: [:] )
Line range hint
134-150: Update nextPage mock to match V2 Chat API formatThe
nextPagemock still uses the old "timeserial" key and is missing the "latestAction" field, making it inconsistent with the V2 Chat API format.static let nextPage = MockHTTPPaginatedResponse( items: [ [ - "timeserial": "3446450", + "serial": "3446450", + "latestAction": "chat.message", "roomId": "basketball::$chat::$chatMessages", "text": "previous message", "metadata": [:], "headers": [:], ], [ - "timeserial": "3446451", + "serial": "3446451", + "latestAction": "chat.message", "roomId": "basketball::$chat::$chatMessages", "text": "previous response", "metadata": [:], "headers": [:], ], ], statusCode: 200, headers: [:] )Tests/AblyChatTests/ChatAPITests.swift (1)
Time serial comparison functions still exist in MessageTests.swift
The PR objective to remove time serial comparison functions is not fully implemented. Found comparison functions in:
Tests/AblyChatTests/MessageTests.swift:
isBefore_WhenMessageIsBefore_ReturnsTrue()isBefore_WhenMessageIsNotBefore_ReturnsFalse()isAfter_whenMessageIsAfter_ReturnsTrue()isAfter_whenMessageIsNotAfter_ReturnsFalse()isEqual_whenMessageIsEqual_ReturnsTrue()isEqual_whenMessageIsNotEqual_ReturnsFalse()These comparison functions should be removed and replaced with direct operators as per PR objectives.
🔗 Analysis chain
Line range hint
1-143: Verify removal of time serial comparison functions.Per PR objectives, time serial comparison functions (isBefore, isAfter, isEqual) should be removed in favor of direct operators.
Let's verify if any comparison functions still exist:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for time serial comparison functions ast-grep --pattern 'func isBefore($$$) { $$$ }' ast-grep --pattern 'func isAfter($$$) { $$$ }' ast-grep --pattern 'func isEqual($$$) { $$$ }'Length of output: 153
Script:
#!/bin/bash # Search for any comparison functions or methods in Swift files rg -t swift "func (isBefore|isAfter|isEqual)" -g "!vendor/*" # Search for potential comparison method calls rg -t swift "\.(isBefore|isAfter|isEqual)\(" -g "!vendor/*" # Look for files that might contain Message or Serial related types fd -e swift -x cat {} \; | rg "class.*(Message|Serial)|struct.*(Message|Serial)|extension.*(Message|Serial)" -A 5Length of output: 4956
Sources/AblyChat/ChatAPI.swift (1)
6-6: Document the V1 to V2 migration strategyWhile the TODO comment indicates a plan to remove V1 after full transition, it would be helpful to document:
- The migration timeline
- Why
getOccupancystill uses V1- Any breaking changes between V1 and V2
Consider adding a comment block explaining the migration strategy and any compatibility considerations.
Sources/AblyChat/DefaultMessages.swift (3)
Line range hint
84-93: Consider grouping related message parametersThe message construction parameters could be organized better by grouping related fields:
let message = Message( - serial: serial, - latestAction: action, - clientID: clientID, - roomID: self.roomID, - text: text, - createdAt: message.timestamp, - metadata: metadata ?? .init(), - headers: headers ?? .init() + // Identity fields + serial: serial, + clientID: clientID, + roomID: self.roomID, + // Content fields + text: text, + metadata: metadata ?? .init(), + // State fields + latestAction: action, + createdAt: message.timestamp, + headers: headers ?? .init() )
201-201: Update error message terminologyThe error message still refers to "timeserial" while the codebase has moved to "serial" terminology.
-logger.log(message: "Resolving timeserial on channel attach", level: .debug) +logger.log(message: "Resolving serial on channel attach", level: .debug)
V2 API compatibility verification complete - issues found
The codebase shows several areas that need attention for V2 API compatibility:
Legacy terminology is still present in the codebase:
- References to
timeserialin comments and variable names- Mixed usage of
channelSerialandattachSerialfor message sequencingV2 API integration is incomplete:
- Only basic V2 endpoints are implemented (
/chat/v2/rooms/{roomId}/messages)- TODO comment indicates partial V2 transition: "remove v1 after full transition to v2"
Error handling needs standardization:
- Mix of different error codes (40000, 50000) without clear V2 API alignment
- Some error messages still use legacy terminology
🔗 Analysis chain
Line range hint
1-254: Verify V2 Chat API compatibility requirementsSeveral TODOs remain in the code that might affect V2 API integration:
- Issue Revisit the need for MainActor for DefaultMessages #83: Understanding of @mainactor usage
- Issue Find a way to clean up arrays of subscriptions #36: Handle unsubscribing
- Issue Revisit the errors thrown by library #32: Error handling review
Please ensure these issues are addressed as part of the V2 API migration.
Run the following to find any remaining V2 API compatibility issues:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for V2 API related TODOs and potential compatibility issues echo "Searching for remaining TODOs..." rg -n "TODO|FIXME" --type swift echo "Checking for old API references..." rg -n "timeserial|created\.rawValue" --type swift echo "Verifying message event handling..." ast-grep --pattern 'MessageEvent.$_' --type swiftLength of output: 22813
Script:
#!/bin/bash # Let's verify the V2 API compatibility more thoroughly # Check for V2 API specific patterns echo "Checking V2 API patterns..." rg -n "chat/v2|apiVersionV2" --type swift # Check message event handling echo "Checking message event handling..." rg -n "MessageEvent\." --type swift # Check for any remaining old API references echo "Checking for old API references..." rg -n "created\.rawValue|timeserial|channelSerial|attachSerial" --type swift -C 2 # Check for unhandled error cases echo "Checking error handling..." rg -n "throw (ARTErrorInfo|MessagesError)" --type swift -C 2Length of output: 20686
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)Example/AblyChatExample/ContentView.swift(1 hunks)Example/AblyChatExample/Mocks/Misc.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Package.swift(1 hunks)Sources/AblyChat/ChatAPI.swift(3 hunks)Sources/AblyChat/DefaultMessages.swift(9 hunks)Sources/AblyChat/Events.swift(1 hunks)Sources/AblyChat/Message.swift(2 hunks)Sources/AblyChat/Timeserial.swift(0 hunks)Tests/AblyChatTests/ChatAPITests.swift(4 hunks)Tests/AblyChatTests/MessageSubscriptionTests.swift(1 hunks)Tests/AblyChatTests/MessageTests.swift(5 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(3 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyChat/Timeserial.swift
🧰 Additional context used
📓 Learnings (1)
Package.swift (1)
Learnt from: umair-ably
PR: ably-labs/ably-chat-swift#45
File: Package.swift:23-23
Timestamp: 2024-11-12T15:07:39.465Z
Learning: In the Ably Chat Swift project, it's acceptable to use a branch-based dependency ('branch: "main"') for the 'ably-cocoa' package because the team owns that SDK and the code isn't going into production soon.
🔇 Additional comments (28)
Sources/AblyChat/Events.swift (2)
3-3: LGTM! Verify the public API exposure.
The visibility change to public is appropriate for the V2 Chat API migration. The protocol conformance to Codable and Sendable supports serialization and thread safety.
✅ Verification successful
Making MessageEvent public is appropriate and well-integrated
The visibility change to public is justified as the enum is already used in public interfaces:
- Used as a public property
latestActionin Message struct - Used in public initializer parameters
- Integrated with Ably channel subscription system in DefaultMessages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if this enum is referenced in public interfaces
# Expected: Find references in public API declarations
# Search for public declarations using MessageEvent
ast-grep --pattern 'public $_ $_: MessageEvent'
# Search for public function parameters using MessageEvent
ast-grep --pattern 'public func $_($_: MessageEvent)'
Length of output: 105
Script:
#!/bin/bash
# Let's try a broader search to understand the usage of MessageEvent
# Find all files containing MessageEvent
rg "MessageEvent" -l
# Show the context of MessageEvent usage
rg "MessageEvent" -B 2 -A 2
# Find Swift files that might be using this enum
fd ".swift$" --exec grep -l "MessageEvent" {}
Length of output: 2373
4-4: Verify event subscription updates across the codebase.
The change from created = "message.created" to new = "chat.message" represents a breaking change in the event naming convention. While this aligns with the V2 Chat API, it requires careful migration of existing event subscriptions.
Sources/AblyChat/Message.swift (4)
20-22: LGTM! Initializer changes are consistent
The initializer has been properly updated to include the new parameters and maintains a clean parameter order matching the property declarations.
32-33: LGTM! CodingKeys maintain consistent naming convention
The CodingKeys enum has been properly updated to include the new properties while maintaining the established naming convention for JSON serialization.
Line range hint 1-40: Verify removal of CHA-prefixed components
The file contains a "CHA-M2" comment. Based on PR comments about removing "CHA-M5j", we should verify if other CHA-prefixed components need removal.
9-9: Verify that lexicographical ordering of serial maintains message sequence
The change from timeserial to serial for ID computation looks good, but we should verify that lexicographical comparison of serial strings maintains the correct message ordering.
✅ Verification successful
Lexicographical ordering of serial is verified and maintains correct message sequence
Based on the evidence from the codebase:
- The
serialformat follows a timestamp-based pattern (e.g., "ABC123@1631840000000-5:2") which ensures lexicographical ordering matches temporal ordering - Tests explicitly verify this ordering behavior:
MessageTests.swiftconfirmsearlierMessage.serial < laterMessage.serial- Multiple test cases validate the correct ordering relationship
- The V2 Chat API provides these serials, ensuring consistency with Ably's message ordering guarantees
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any custom sorting or comparison logic that might depend on message ordering
rg -g '*.swift' -A 3 'sort|compare'
Length of output: 5139
Script:
#!/bin/bash
# Search for message ordering or serial-related code
rg -g '*.swift' -A 5 'serial|message.*order'
# Also check for any array operations on messages
ast-grep --pattern 'messages.$$$sort($$$)'
Length of output: 21787
AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
9-10: Caution: Using feature branch in production code
Using a feature branch (feature/ECO-5116-chat-support) as a dependency source can be risky in production code:
- Feature branches are typically temporary and may be deleted
- They can be rebased or force-pushed, breaking your builds
- Makes it harder to track which version of the API you're using
Consider one of these approaches:
- Wait for the V2 Chat API changes to be merged into main
- Fork the repository and maintain your own stable branch
- Pin to a specific tag once the V2 Chat API is officially released
Package.swift (2)
23-24: Track technical debt for switching back to tag-based versioning.
The TODO comment indicates this branch-based dependency is temporary. While this is acceptable for now (as confirmed by previous feedback), we should ensure this technical debt is properly tracked.
Consider creating a reminder or milestone to switch back to tag-based versioning once the V2 Chat API changes are stable. This will ensure better reproducibility of builds in the future.
✅ Verification successful
Technical debt is already tracked in issue #80
The TODO comment references issue #80 which exists and is currently open with the title "Switch back to using a tag of ably-cocoa". This confirms that the technical debt is being properly tracked in the issue tracker.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if issue #80 exists and is still open
gh issue view 80 --repo ably-labs/ably-chat-swift --json state,title
Length of output: 138
24-24: LGTM! Verify branch existence.
The branch change aligns with the PR objective to switch to V2 Chat API.
✅ Verification successful
Branch exists and is accessible ✓
The feature branch feature/ECO-5116-chat-support exists in the ably-cocoa repository and is accessible, making it safe to use as a dependency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the feature branch exists in the ably-cocoa repository
gh repo clone ably/ably-cocoa /tmp/ably-cocoa 2>/dev/null || true
cd /tmp/ably-cocoa
git fetch origin feature/ECO-5116-chat-support
git branch -r --contains origin/feature/ECO-5116-chat-support
Length of output: 327
Tests/AblyChatTests/MessageSubscriptionTests.swift (1)
Line range hint 1-65: Verify removal of CHA-M5j related test code
Based on the PR comments about removing the CHA-M5j component, we should verify if any related test code needs to be removed from this file.
Tests/AblyChatTests/MessageTests.swift (4)
56-57: LGTM! Consistent use of comparison operators
The changes maintain consistency with the isBefore tests by using direct comparison operators.
Also applies to: 62-63
42-43: LGTM! Direct comparison operators improve readability
The switch from isBefore function to direct < operator aligns with the PR objectives and simplifies the code.
Let's verify all comparison functions have been removed:
Also applies to: 48-49
✅ Verification successful
LGTM! All comparison functions have been correctly replaced with operators
Looking at the test file, all the test methods now use direct comparison operators (<, >, ==, !=) instead of function calls. The only remaining function with "isEqual" is in ChatClient.swift but it's unrelated (for test purposes only). The changes align with the PR objectives of simplifying comparisons.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining comparison function implementations
ast-grep --pattern 'func isBefore($$$) { $$$ }'
ast-grep --pattern 'func isAfter($$$) { $$$ }'
ast-grep --pattern 'func isEqual($$$) { $$$ }'
Length of output: 147
Script:
#!/bin/bash
# Search for any remaining comparison function declarations or calls
rg -i "func (isbefore|isafter|isequal)" -g "*.swift"
# Search for any method calls of these comparison functions
rg -i "\.(isbefore|isafter|isequal)\(" -g "*.swift"
# Let's also check the test file specifically to understand the context better
cat Tests/AblyChatTests/MessageTests.swift
Length of output: 3197
71-81: Verify intended equality comparison behavior
The tests now compare only the serial field for equality. Please confirm if this is the intended behavior, as messages with different content but same serial are considered equal.
Let's check the previous implementation:
Consider documenting this behavior in the Message class if it hasn't been done already, as it affects message identity semantics.
Also applies to: 86-87
✅ Verification successful
Message equality based on serial is the correct behavior
The implementation is correct and intentional. The Message struct conforms to Equatable protocol without a custom implementation, which means Swift synthesizes the equality operator (==) comparing all stored properties. However, the struct also conforms to Identifiable with id explicitly defined to return the serial property, indicating that serial is intended to be the unique identifier for messages. The test is correctly verifying this design choice, as two messages in the same channel cannot have the same timeserial (as documented in the code comments).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for previous message equality implementation
git log -p -- "**/Message.swift" | rg -B 5 -A 5 "func.*[iI]s[eE]qual"
Length of output: 0
Script:
#!/bin/bash
# Let's try a different approach to find equality implementation
# First, let's find Message.swift files
fd Message.swift
# Then search for equality implementation in current files
rg -B 5 -A 5 "(?:Equatable|==|[iI]s[eE]qual)" $(fd Message.swift)
# Also search for any tests related to message equality
rg -B 5 -A 5 "(?:test.*[eE]qual|XCTAssert.*[eE]qual)" $(fd Message.swift)
Length of output: 802
Script:
#!/bin/bash
# Let's check the Message.swift file content to understand the equality implementation
cat Sources/AblyChat/Message.swift
# Also check if there are any recent changes to Message.swift
git diff HEAD~5 HEAD -- Sources/AblyChat/Message.swift
Length of output: 1587
6-7: Consider adding serial format validation tests
The serial format appears to follow a specific pattern (prefix@timestamp-suffix), but there are no tests validating this format. The invalidMessage with "invalid" serial suggests format validation might be important.
Let's check if there are any format validation tests elsewhere:
Would you like me to help create test cases for serial format validation?
Also applies to: 17-18, 28-29
Example/AblyChatExample/Mocks/Misc.swift (2)
Line range hint 1-116: Verify test coverage for V2 API changes
Since this is a mock implementation used in tests, ensure that all V2 API scenarios are properly covered, including edge cases with different serial values and action states.
14-15: Verify V2 Chat API message initialization
The changes align with the V2 API transition by replacing timeserial with serial and adding latestAction. However, let's verify if there are any other required parameters for V2 API compliance.
✅ Verification successful
Based on the comprehensive search results, I can now provide a final response regarding the V2 Chat API message initialization verification:
Message initialization is correctly implemented with required V2 API parameters
The Message struct in Sources/AblyChat/Message.swift shows that all required fields are present and properly initialized:
serial: String identifier (replacing timeserial)latestAction: MessageEvent typeclientID: String for client identificationroomID: String for room identificationtext: String contentcreatedAt: Optional Datemetadata: MessageMetadata (type alias for Headers)headers: MessageHeaders (type alias for Headers)
The mock implementation correctly uses these fields and the V2 API transition is properly handled throughout the codebase, including the ChatAPI, DefaultMessages, and test implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Message initialization patterns to ensure consistency
ast-grep --pattern 'Message(
$$$
serial: $_,
latestAction: $_,
$$$
)'
Length of output: 79
Script:
#!/bin/bash
# Search for Message type declarations and initializations with a broader pattern
rg -U "Message.*\{[\s\S]*?\}" -A 10
# Also search for any Message-related type definitions
rg "class Message|struct Message|typealias Message"
Length of output: 74681
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
101-102: LGTM! Changes align with V2 Chat API
The renaming of "timeserial" to "serial" and addition of "latestAction" field are consistent with the V2 Chat API migration.
Also applies to: 111-112
Tests/AblyChatTests/ChatAPITests.swift (3)
93-94: LGTM! Consistent Message structure across test cases.
The Message initialization is consistent across test cases, properly using sequential serials ("3446456", "3446457") and maintaining the new V2 API structure.
Also applies to: 103-104
116-116: LGTM! Improved variable naming.
The rename from getMessages to getMessagesResult enhances test readability by avoiding name collision with the method being tested and better describing the variable's purpose.
Also applies to: 119-119
43-44: LGTM! Verify V2 API Message structure.
The changes correctly reflect the new Message structure for V2 Chat API with serial and latestAction properties.
Let's verify the Message structure matches V2 API specification:
✅ Verification successful
Message structure correctly reflects V2 Chat API changes
The codebase analysis confirms that:
- The Message struct in
Sources/AblyChat/Message.swifthas been properly updated withserialandlatestActionproperties - The changes are consistently applied across all test files and implementations
- The migration from
timeserialtoserialis complete, with proper handling in the DefaultMessages implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Message struct definition to confirm structure
ast-grep --pattern 'struct Message {
$$$
serial: $_
latestAction: $_
$$$
}'
Length of output: 85
Script:
#!/bin/bash
# Let's try a broader search for Message struct definition in Swift files
fd -e swift | xargs rg -l "struct Message"
# Then search for serial and latestAction properties
fd -e swift | xargs rg "serial|latestAction" -A 2 -B 2
Length of output: 19795
Sources/AblyChat/ChatAPI.swift (2)
14-14: LGTM: Clean V2 endpoint update
The endpoint update maintains the existing interface while transitioning to V2.
48-49: Verify Message struct changes and latestAction enum
The Message initialization includes two changes:
- Using
serialinstead oftimeserial - Adding
latestAction: .new
Let's verify the Message struct definition and the latestAction enum.
✅ Verification successful
Let me gather more information about the MessageEvent enum since we see it's used as the type for latestAction.
Message struct and latestAction enum changes are properly defined
The verification confirms:
Messagestruct properly includes thelatestActionproperty of typeMessageEventMessageEventenum is well-defined with.newcase that maps to "chat.message"- The usage is consistent across tests and implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Message struct definition and latestAction enum
echo "Checking Message struct definition:"
ast-grep --pattern 'struct Message {
$$$
}'
echo -e "\nChecking latestAction enum/property definition:"
rg "latestAction" -B 2 -A 2 --type swift
Length of output: 7568
Script:
#!/bin/bash
# Search for MessageEvent enum definition
echo "Checking MessageEvent enum definition:"
ast-grep --pattern 'enum MessageEvent {
$$$
}'
# Also search for any other references to MessageEvent
echo -e "\nChecking MessageEvent references:"
rg "MessageEvent" -B 2 -A 2 --type swift
Length of output: 2332
Example/AblyChatExample/Mocks/MockClients.swift (2)
106-107: LGTM! Changes align with V2 Chat API migration
The changes to use serial instead of timeserial and addition of latestAction: .new are consistent with the V2 Chat API requirements.
132-133: LGTM! Consistent implementation with createSubscription
The changes mirror those in createSubscription, maintaining consistency across the mock implementation.
Sources/AblyChat/DefaultMessages.swift (2)
3-7: LGTM: Clean rename from timeserial to serial
The rename maintains the structure's purpose while aligning with V2 Chat API conventions.
56-56: Verify message handling behavior with V2 API
The changes introduce two significant modifications:
- Switch from
MessageEvent.createdtoMessageEvent.new - Silent return when action can't be mapped to a MessageEvent
Consider logging discarded messages at DEBUG level to aid troubleshooting.
guard let action = MessageEvent.fromAction(message.action) else {
+ logger.log(message: "Discarding message with unknown action: \(message.action)", level: .debug)
return
}Also applies to: 80-82
Example/AblyChatExample/ContentView.swift (2)
Line range hint 1-4: Verify dependencies on removed components
Based on PR comments, please verify:
- Any dependencies on the CHA-M5j component that needs to be removed
- Any usage of time serial comparison functions (isBefore, isAfter, isEqual) that should be replaced with direct operators
Let's check for these dependencies:
✅ Verification successful
No dependencies found on removed components
Based on the search results:
- No occurrences of CHA-M5j component were found in the codebase
- The time comparison functions (
isBefore,isAfter,isEqual) found in tests are unrelated to message comparisons, as they are used for testing client options equality - The
AblyChatimports are valid and required for the chat functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for CHA-M5j component usage and time comparison functions
# Search for CHA-M5j component
echo "Checking for CHA-M5j component usage:"
rg -l "CHA-M5j"
# Check imports for potential dependencies
echo "Checking imports that might need updating:"
rg "^import.*Chat"
# Look for any message comparison usage
echo "Checking for message comparison usage:"
rg -A 2 "\.(isBefore|isAfter|isEqual)"
Length of output: 1647
192-192: LGTM! Verify complete migration from timeserial to serial
The change from timeserial to serial is consistent with the V2 Chat API migration.
Let's verify the complete migration across the codebase:
Also applies to: 200-200
✅ Verification successful
Remaining timeserial references need to be addressed
The migration from timeserial to serial is incomplete. Found references that need attention:
Sources/AblyChat/DefaultMessages.swift: Contains comments and logging references to timeserialSources/AblyChat/Message.swift: Contains a comment about timeserial in message identificationTests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift: Contains mock data using timeserial
Let's verify if these are just documentation artifacts or actual code dependencies:
Migration from timeserial to serial is complete, only documentation needs updating
The codebase scan reveals that all actual code usage has been migrated to serial. The remaining timeserial references are only in:
- Comments in
Message.swiftexplaining theidproperty - Debug logging messages in
DefaultMessages.swift - Test mock data that needs updating
No structural definitions or actual code dependencies on timeserial were found. The migration is functionally complete, but documentation should be updated for consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining timeserial references that need to be updated
# Search for any remaining timeserial references
echo "Checking for remaining timeserial references:"
rg "timeserial"
# Search for any comparison functions that might need updating
echo "Checking for message comparison functions:"
ast-grep --pattern 'func is(Before|After|Equal).*Message.*Bool'
Length of output: 1103
Script:
#!/bin/bash
# Check the actual code usage in these files
echo "Checking DefaultMessages.swift implementation:"
rg -A 2 "timeserial" Sources/AblyChat/DefaultMessages.swift
echo "\nChecking Message.swift implementation:"
rg -A 2 "timeserial" Sources/AblyChat/Message.swift
echo "\nChecking struct/class definitions for timeserial property:"
ast-grep --pattern 'struct $_ {
$$$
timeserial: $_
$$$
}'
ast-grep --pattern 'class $_ {
$$$
timeserial: $_
$$$
}'
Length of output: 1334
maratal
force-pushed
the
ECO-5116-V2-Chat-API
branch
from
fac5ac1 to
14310b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)Example/AblyChatExample/ContentView.swift(1 hunks)Example/AblyChatExample/Mocks/Misc.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Package.swift(1 hunks)Sources/AblyChat/ChatAPI.swift(3 hunks)Sources/AblyChat/DefaultMessages.swift(9 hunks)Sources/AblyChat/Events.swift(1 hunks)Sources/AblyChat/Message.swift(2 hunks)Sources/AblyChat/Timeserial.swift(0 hunks)Tests/AblyChatTests/ChatAPITests.swift(4 hunks)Tests/AblyChatTests/MessageSubscriptionTests.swift(1 hunks)Tests/AblyChatTests/MessageTests.swift(5 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(4 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyChat/Timeserial.swift
🚧 Files skipped from review as they are similar to previous changes (12)
- AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved
- Example/AblyChatExample/ContentView.swift
- Example/AblyChatExample/Mocks/Misc.swift
- Example/AblyChatExample/Mocks/MockClients.swift
- Package.swift
- Sources/AblyChat/ChatAPI.swift
- Sources/AblyChat/DefaultMessages.swift
- Sources/AblyChat/Message.swift
- Tests/AblyChatTests/ChatAPITests.swift
- Tests/AblyChatTests/MessageSubscriptionTests.swift
- Tests/AblyChatTests/MessageTests.swift
- Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift
🔇 Additional comments (3)
Sources/AblyChat/Events.swift (3)
3-4: LGTM! API changes look appropriate.
The changes to make MessageEvent public with Codable and Sendable conformance are well-considered. The case rename from 'created' to 'create' aligns with V2 Chat API conventions.
24-25: LGTM! Message filtering implementation looks clean.
The new MessageFilter enum with the 'new' case provides a clear way to filter chat messages. The Sendable conformance ensures thread safety.
6-21: Verify the intentional ignoring of other message actions.
The comment indicates that other actions are being ignored intentionally. Please confirm if this aligns with the V2 Chat API requirements and if there are plans to handle these actions in the future.
✅ Verification successful
Intentional ignoring of message actions is verified and aligned with current implementation
Based on the codebase analysis:
- The code explicitly handles only the
createaction for messages, with a clear comment indicating this is intentional ("ignore any other actions exceptchat.messagefor now") - The implementation in
DefaultMessages.swiftfilters out all other message actions by returning early whenMessageEvent.fromAction()returns nil - No other parts of the codebase are attempting to use the ignored message actions
- The comment's "for now" suggests this is a deliberate temporary limitation, likely part of an incremental development approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of ignored message actions in the codebase
rg -l "ARTMessageAction\.(update|delete|annotationCreate|annotationDelete|metaOccupancy)"
Length of output: 91
Script:
#!/bin/bash
# Let's first check for any documentation or comments about message actions
rg -i "message.*action" -A 5 -B 5
# Also search for ARTMessageAction enum usage
rg "ARTMessageAction" -A 5 -B 5
# Look for any TODOs or FIXMEs related to message actions
rg -i "(todo|fixme).*message.*action"
# Check if there are any test files related to message actions
fd -e swift test | xargs rg "ARTMessageAction"
Length of output: 13394
maratal
force-pushed
the
ECO-5116-V2-Chat-API
branch
from
14310b9 to
8129e9c
Compare
|
@coderabbitai pause |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
Example/AblyChatExample/Mocks/MockClients.swift (1)
106-106: Consider extracting serial generation logicThe timestamp-based serial generation is duplicated and could potentially lead to collisions in rapid succession. Consider extracting this into a shared utility method that implements a more robust serial generation strategy.
actor MockMessages: Messages { + private func generateUniqueSerial() -> String { + // Using nanoseconds and a counter to ensure uniqueness + let timestamp = Date().timeIntervalSince1970 + let nanos = UInt64(timestamp * 1_000_000_000) + return String(format: "%d", nanos) + } + private func createSubscription() -> MockSubscription<Message> { let subscription = MockSubscription<Message>(randomElement: { Message( - serial: "\(Date().timeIntervalSince1970)", + serial: generateUniqueSerial(), latestAction: .create, // ... ) }, interval: 3) } func send(params: SendMessageParams) async throws -> Message { let message = Message( - serial: "\(Date().timeIntervalSince1970)", + serial: generateUniqueSerial(), latestAction: .create, // ... ) } }Also applies to: 132-132
Sources/AblyChat/DefaultMessages.swift (3)
168-173: Consider standardizing error message capitalizationThe channel attachment handling has been properly updated for the V2 API. However, there's a minor inconsistency in error message capitalization:
- Line 208: "Error resolving serial on channel attach"
- Other error messages start with capital letters
- logger.log(message: "Error resolving serial on channel attach: \(error)", level: .error) + logger.log(message: "Error resolving Serial on channel attach: \(error)", level: .error)Also applies to: 199-208
Line range hint
216-250: Address TODO comment regarding error handlingThere's a TODO comment referencing issue #32 regarding error handling review. Consider addressing this as part of the V2 API migration to ensure comprehensive error handling.
Would you like me to help create a more detailed error handling strategy for the channel attachment states?
Line range hint
1-250: Consider implementing a message versioning strategyAs you're migrating to the V2 Chat API, consider implementing a versioning strategy for message handling to make future API version migrations smoother. This could involve:
- Creating a protocol for message version handling
- Implementing version-specific message processors
- Adding a factory pattern for creating appropriate message handlers
This would make the codebase more maintainable and future-proof for subsequent API versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)Example/AblyChatExample/ContentView.swift(1 hunks)Example/AblyChatExample/Mocks/Misc.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Package.swift(1 hunks)Sources/AblyChat/ChatAPI.swift(3 hunks)Sources/AblyChat/DefaultMessages.swift(9 hunks)Sources/AblyChat/Events.swift(1 hunks)Sources/AblyChat/Message.swift(2 hunks)Sources/AblyChat/Timeserial.swift(0 hunks)Tests/AblyChatTests/ChatAPITests.swift(4 hunks)Tests/AblyChatTests/MessageSubscriptionTests.swift(1 hunks)Tests/AblyChatTests/MessageTests.swift(5 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(4 hunks)
💤 Files with no reviewable changes (1)
- Sources/AblyChat/Timeserial.swift
🚧 Files skipped from review as they are similar to previous changes (10)
- AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved
- Example/AblyChatExample/ContentView.swift
- Example/AblyChatExample/Mocks/Misc.swift
- Package.swift
- Sources/AblyChat/Events.swift
- Sources/AblyChat/Message.swift
- Tests/AblyChatTests/ChatAPITests.swift
- Tests/AblyChatTests/MessageSubscriptionTests.swift
- Tests/AblyChatTests/MessageTests.swift
- Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift
🔇 Additional comments (10)
Sources/AblyChat/ChatAPI.swift (3)
14-14: LGTM: Correctly updated to V2 endpoint
The endpoint update aligns with the V2 API migration while maintaining the existing method signature.
30-30: Verify the Message.latestAction implementation
The addition of latestAction: .create suggests a new message state tracking mechanism. Let's verify the implementation and usage of this property across the codebase.
Also applies to: 48-49
✅ Verification successful
Let me gather more information about the MessageEvent type to understand the complete implementation.
Let me check one more thing to understand the complete MessageEvent implementation and its action mapping.
latestAction implementation is correctly used and follows the expected pattern
The implementation is well-structured and consistent:
MessageEventis properly defined as an enum with acreatecase- The property is correctly used in the Message struct initialization
- The conversion from Ably's message actions to
MessageEventis handled viafromAction - Currently only handles
createactions, explicitly ignoring others as per the comment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Message.latestAction implementation and usage
echo "Checking latestAction implementation:"
ast-grep --pattern 'struct Message {
$$$
latestAction: $_
$$$
}'
echo -e "\nChecking latestAction usage:"
rg "latestAction" --type swift -A 2
Length of output: 4833
Script:
#!/bin/bash
# Check MessageEvent implementation
echo "Checking MessageEvent implementation:"
ast-grep --pattern 'enum MessageEvent {
$$$
}'
echo -e "\nChecking MessageEvent usage patterns:"
rg "MessageEvent\." --type swift -A 2
Length of output: 510
Script:
#!/bin/bash
# Find MessageEvent definition and fromAction implementation
echo "Checking MessageEvent complete implementation:"
rg "MessageEvent" --type swift -B 2 -A 5
echo -e "\nChecking action related code:"
rg "action.*chat\.message" --type swift -A 2
Length of output: 2619
Line range hint 59-62: Document why getOccupancy remains on V1
While other endpoints have been migrated to V2, getOccupancy still uses V1. Please document the reason for this inconsistency or update it to use V2 if appropriate.
Example/AblyChatExample/Mocks/MockClients.swift (2)
106-107: LGTM: Changes align with V2 Chat API migration
The changes correctly implement the V2 Chat API requirements by:
- Using
serialinstead oftimeserial - Adding the new
latestActionparameter with.createdefault value
132-133: LGTM: Consistent implementation in send method
The changes maintain consistency with the createSubscription implementation, ensuring uniform behavior across the mock client.
Sources/AblyChat/DefaultMessages.swift (5)
3-7: LGTM: Clean migration from timeserial to serial
The rename from timeserial to serial is consistent with the V2 Chat API migration. The implementation remains clean and well-documented.
19-21: LGTM: Subscription handling properly updated
The subscription points map and creation logic have been correctly updated to use the new serial terminology while maintaining the robust error handling and initialization flow.
Also applies to: 41-52
69-71: LGTM: Error handling properly updated
The error handling for the serial field is comprehensive and provides clear error messages.
117-124: LGTM: Subscription point handling properly updated
The subscription point access and error handling have been correctly updated to use the new serial terminology.
56-56: Verify the new message filter and action handling implementation
The switch to MessageFilter.new and the addition of action handling represent significant changes in the V2 Chat API integration. Please ensure:
- The
MessageFilter.newconstant is properly defined and matches the V2 API specification - All possible message actions are handled appropriately
Also applies to: 80-86
✅ Actions performedReviews paused. |
maratal
force-pushed
the
ECO-5116-V2-Chat-API
branch
from
8129e9c to
7692200
Compare
|
actually ignore that... this wasn't in the doc as it's part of edit/delete but from looking at ably-js we're going to have a new type which also sounds confusingly similar... we don't need it for this PR but the solution will grow into something like this: 
We should just follow the same naming conventions now to avoid both confusion and the headache of coming up with our own names that make sense |
maratal
force-pushed
the
ECO-5116-V2-Chat-API
branch
from
7692200 to
05167f2
Compare
There was a problem hiding this comment.
CI failing is due to https://ably-real-time.slack.com/archives/C03JDBVM5MY/p1732287002273979 and not this PR... approving but wait until CI passes again... Should CI fail again from next week due to expected timeserial changes, then contact @AndyTWF for how to resolve them in line with expected behaviour
maratal
force-pushed
the
ECO-5116-V2-Chat-API
branch
from
05167f2 to
6f81e5f
Compare
maratal
force-pushed
the
ECO-5116-V2-Chat-API
branch
2 times, most recently
from
3773ca4 to
db03309
Compare
maratal
force-pushed
the
ECO-5116-V2-Chat-API
branch
from
0f12ac3 to
780be56
Compare
I've temporarily switched to the branch of ably-cocoa PR It should be merged before this PR so I could remove tmp commit.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores