[ECO-5116] Added new fields to the ARTMessage.
#1998
Conversation
WalkthroughThe changes in this pull request involve enhancements to the Changes
Assessment against linked issues
Possibly related 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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/1998/features
github-actions
bot
temporarily deployed
to
staging/pull/1998/jazzydoc
maratal
changed the title
ARTMessage.ARTMessage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
Source/include/Ably/ARTMessage.h (1)
7-16: Add documentation for the enum and its values.While the enum is well-structured, it would benefit from documentation explaining:
- The purpose and usage of
ARTMessageAction- The meaning and appropriate use case for each action value
- Any constraints or relationships between actions
Add documentation like this:
+/** + * Represents the type of action associated with a message in the V2 Chat API. + */ NS_SWIFT_SENDABLE typedef NS_ENUM(NSUInteger, ARTMessageAction) { + /// Indicates an unset or invalid action state ARTMessageUnset, + /// Represents a message creation action ARTMessageCreate, // ... document other values similarly };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Source/ARTJsonLikeEncoder.m(1 hunks)Source/include/Ably/ARTMessage.h(2 hunks)
🔇 Additional comments (3)
Source/include/Ably/ARTMessage.h (1)
Line range hint 7-33: Verify V2 Chat API migration impact.
As these changes are part of the V2 Chat API migration:
- Ensure backward compatibility is maintained or document breaking changes
- Consider adding migration guide for users upgrading from V1
- Verify integration with related components
Let's check for related changes and potential impacts:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
V2 Chat API changes appear to be part of a broader protocol upgrade with proper implementation.
The changes to ARTMessage are part of the Realtime protocol v2 implementation (issue #1494) and show:
- The new properties (
actionandserial) are properly integrated into the existing message handling system as evidenced by their usage inARTJsonLikeEncoderand other core components - The changes maintain backward compatibility as they're additive and don't modify existing behavior
- The implementation is consistent across the codebase with proper message action handling in transport, presence, and channel components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related V2 API changes and documentation
echo "Checking for related V2 API changes..."
rg -l "V2.*Chat.*API|ARTMessage.*V2" --type md
echo "Checking for potential usage of affected properties..."
rg -l "\.action\s*=|\.serial\s*=" --type=objc
echo "Checking for tests covering new properties..."
rg -l "test.*ARTMessage.*action|test.*ARTMessage.*serial" --type=objc
Length of output: 640
Script:
#!/bin/bash
# Let's check the actual usage of ARTMessageAction and ARTMessage properties
echo "Checking ARTMessageAction usage..."
rg -A 5 "ARTMessageAction" --type=objc
echo "Checking message action and serial property usage..."
rg -B 2 -A 2 "\.action\s*=|\.serial\s*=" --type=objc
echo "Checking for migration guides or documentation..."
fd -e md | xargs rg -l "migration|upgrade|breaking.?change"
echo "Checking test coverage..."
fd -e m | xargs rg -l "ARTMessage.*test"
Length of output: 8878
Script:
#!/bin/bash
# Let's check the changelog for any breaking changes or migration notes
echo "Checking CHANGELOG.md for V2 changes..."
rg -B 2 -A 5 "v2|breaking.?change|migration" CHANGELOG.md
# Let's check the actual implementation of ARTMessage
echo "Checking ARTMessage implementation..."
fd "ARTMessage.m" | xargs cat
# Let's look for any tests specifically related to these new properties
echo "Checking for specific test cases..."
rg -A 5 "ARTMessageAction|\.action\s*=|\.serial\s*=" --type=m Spec/ Test/
Length of output: 7046
Source/ARTJsonLikeEncoder.m (2)
278-279: LGTM! The new fields are properly extracted from the dictionary.
The implementation correctly handles the new action and serial fields, with appropriate default value for action and null safety through the artNumber: and artString: helpers.
278-279: Verify the usage of new message fields across the codebase.
The addition of new fields to ARTMessage might affect existing code that processes messages.
✅ Verification successful
Changes to message fields are consistent with existing usage
The verification shows that:
- The
actionfield is a standard part of theARTMessageinterface and is used consistently across the codebase for protocol message handling and presence state management - The
serialfield is properly defined in theARTMessageinterface and is only used during message encoding/decoding inARTJsonLikeEncoder.m - Both fields are properly documented in the interface definition
- The changes in
ARTJsonLikeEncoder.mmaintain consistency with the existing message protocol
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of ARTMessage action and serial fields
# Check for message action usage
echo "Checking message action usage..."
rg -l "message\.action" --type objc --type h
# Check for message serial usage
echo "Checking message serial usage..."
rg -l "message\.serial" --type objc --type h
# Check for potential test coverage
echo "Checking test coverage..."
rg -l "ARTMessage.*action.*serial" --type objc --glob "*Test*"
Length of output: 503
Script:
#!/bin/bash
# Let's examine the actual usage of these fields in the identified files
echo "Checking ARTMessage action usage in ARTRealtime.m..."
rg "message\.action" Source/ARTRealtime.m -B 2 -A 2
echo -e "\nChecking ARTMessage action usage in ARTRealtimePresence.m..."
rg "message\.action" Source/ARTRealtimePresence.m -B 2 -A 2
echo -e "\nChecking ARTMessage action usage in ARTRealtimeChannel.m..."
rg "message\.action" Source/ARTRealtimeChannel.m -B 2 -A 2
echo -e "\nChecking ARTMessage serial usage in ARTJsonLikeEncoder.m..."
rg "message\.serial" Source/ARTJsonLikeEncoder.m -B 2 -A 2
# Let's also check the ARTMessage class definition
echo -e "\nChecking ARTMessage class definition..."
rg "^@interface ARTMessage" --type objc -A 10
Length of output: 4902
maratal
force-pushed
the
feature/ECO-5116-chat-support
branch
from
8812dfc to
f071709
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1998/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
Source/ARTBaseMessage.m (3)
24-24: Consider using explicit copy forextraspropertyFor consistency with other complex properties and to prevent potential shared state issues, consider using explicit copy:
- message->_extras = self.extras; + message->_extras = [self.extras copy];
Line range hint
63-64: Improve error handling and performance forextrassize calculationThe current implementation has several potential issues:
- No error handling for JSON conversion of
extras- No nil check before accessing
extras- Potentially inefficient due to JSON string conversion
Consider this safer implementation:
NSInteger finalResult = 0; - finalResult += [[self.extras toJSONString] lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; + if (self.extras) { + NSError *error = nil; + NSJSONWritingOptions options = 0; + if (@available(macOS 10.15, iOS 13.0, tvOS 13.0, *)) { + options = NSJSONWritingWithoutEscapingSlashes; + } + NSData *extrasData = [NSJSONSerialization dataWithJSONObject:self.extras + options:options + error:&error]; + if (!error && extrasData) { + finalResult += [extrasData length]; + } + }This implementation:
- Adds nil safety
- Uses consistent JSON serialization approach
- Matches the error handling pattern used for
dataproperty
Line range hint
62-62: Add documentation for message size calculation rulesThe
messageSizemethod lacks documentation about its calculation rules. Consider adding a detailed comment explaining:
- Which properties contribute to the total size
- How different data types are handled
- Any size limits or constraints
Add documentation like:
/// Calculates the total size of the message in bytes. /// @discussion The size includes: /// - extras (as JSON) /// - clientId (UTF8 encoded) /// - data (varies by type): /// - String: UTF8 encoded length /// - NSData: byte length /// - Other: JSON serialized length /// @return The total size in bytes - (NSInteger)messageSize {Source/ARTMessage.m (1)
Line range hint
64-71: Consider security implications of exposing cipher details in error messages.The error messages include cipher information in the prepended text:
"Decoder can't be created with cipher: %@". This could potentially expose sensitive information about the encryption mechanism.Consider replacing with a more generic message:
- prepend:[NSString stringWithFormat:@"Decoder can't be created with cipher: %@", options.cipher]]; + prepend:@"Failed to initialize message decoder"];Also applies to: 94-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Source/ARTBaseMessage.m(1 hunks)Source/ARTJsonLikeEncoder.m(1 hunks)Source/ARTMessage.m(1 hunks)Source/include/Ably/ARTMessage.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Source/include/Ably/ARTMessage.h
🔇 Additional comments (4)
Source/ARTMessage.m (2)
Line range hint 62-86: LGTM: Robust error handling implementation.
The error handling in both methods is well-implemented:
- Proper error propagation
- Consistent pattern across methods
- Clear error context
- Appropriate use of nil returns on errors
Also applies to: 92-116
43-44: Verify property declarations for memory management.
The copying of new properties looks correct. However, please ensure that the property declarations in the header file use appropriate memory management attributes (e.g., copy, strong, assign) based on their types.
Let's check the property declarations:
✅ Verification successful
Property declarations are correctly defined with appropriate memory management attributes
The property declarations in ARTMessage.h are properly defined:
actionis declared asnonatomicwithoutcopyorstrongwhich is appropriate for an enum type (ARTMessageAction)serialis declared asnullable,nonatomicfor anNSStringwhich automatically impliesstrongsemantics in ARC
The memory management in the copyWithZone: implementation aligns well with these declarations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check property declarations in the header file
rg -A 2 "@property.*(?:action|serial)" --type objc
Length of output: 905
Source/ARTJsonLikeEncoder.m (2)
278-279: Deserialization looks good, but serialization is missing.
The deserialization of action and serial fields is implemented correctly. However, these fields are not being serialized back in the messageToDictionary: method.
The previous review comment about adding serialization to messageToDictionary: is still valid:
- (NSDictionary *)messageToDictionary:(ARTMessage *)message {
NSMutableDictionary *output = [NSMutableDictionary dictionary];
+
+ [output setObject:@(message.action) forKey:@"action"];
+
+ if (message.serial) {
+ [output setObject:message.serial forKey:@"serial"];
+ }
+
if (message.id) {
[output setObject:message.id forKey:@"id"];
}278-279: Verify the impact of new message fields.
Let's ensure the new fields are handled consistently across the codebase.
✅ Verification successful
Message fields are properly integrated and handled consistently
The verification shows that both action and serial fields are:
- Properly declared as properties in
ARTMessage.h - Consistently serialized/deserialized in
ARTJsonLikeEncoder - Appropriately handled in message processing across the codebase:
actionfield is used in switch statements for protocol handling- Both fields are properly copied in message copying operations
- Fields are documented in the interface definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of action and serial fields in ARTMessage
# Check for message serialization/deserialization points
echo "Checking message serialization/deserialization points..."
rg -l "messageToDictionary|messageFromDictionary" --type objc
# Check for direct usage of action field
echo -e "\nChecking action field usage..."
rg "message\.action|ARTMessageCreate" --type objc
# Check for direct usage of serial field
echo -e "\nChecking serial field usage..."
rg "message\.serial" --type objc
# Check for protocol documentation mentioning these fields
echo -e "\nChecking protocol documentation..."
fd -e md -e txt . | xargs rg -l "message.*action|message.*serial"
Length of output: 2733
Script:
#!/bin/bash
# Let's check the ARTMessage class definition and usage patterns
# Check ARTMessage class definition
echo "=== ARTMessage Class Definition ==="
rg -A 10 "^@interface ARTMessage" --type objc
# Check ARTMessage property declarations
echo -e "\n=== ARTMessage Properties ==="
rg "^@property.*serial|^@property.*action" --type objc
# Check message initialization patterns
echo -e "\n=== Message Initialization Patterns ==="
ast-grep --pattern 'message = [[ARTMessage alloc] init];'
# Check protocol message handling
echo -e "\n=== Protocol Message Handling ==="
rg -B 2 -A 2 "switch.*message\.action" --type objc
Length of output: 3932
github-actions
bot
temporarily deployed
to
staging/pull/1998/jazzydoc
maratal
force-pushed
the
feature/ECO-5116-chat-support
branch
from
f071709 to
1e312ee
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1998/features
github-actions
bot
temporarily deployed
to
staging/pull/1998/jazzydoc
maratal
force-pushed
the
feature/ECO-5116-chat-support
branch
from
1e312ee to
eaa3662
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1998/features
github-actions
bot
temporarily deployed
to
staging/pull/1998/jazzydoc
|
@lawrence-forooghian For now I took docstings from the link you've provided - https://github.com/ably/ably-js/blob/18a255948c38d1e60715c8f5d6173369b57cb8d6/ably.d.ts#L2337-L2429 |
maratal
force-pushed
the
feature/ECO-5116-chat-support
branch
from
eaa3662 to
ab4583a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1998/features
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h (1)
33-34: Consider protocol versioning strategy.As this change introduces protocol-specific handling into the encoder, consider implementing a protocol version abstraction layer to handle future protocol versions more elegantly. This could help maintain cleaner separation of concerns between message encoding and protocol-specific logic.
Some suggestions:
- Consider introducing a protocol version strategy interface
- Document the protocol version dependencies in the class documentation
- Consider extracting protocol-specific logic into separate handlers for better maintainability
Source/ARTJsonLikeEncoder.m (2)
269-275: Add input validation for version generation.The version string generation looks correct, but consider adding validation for the index value to ensure it's non-negative and within reasonable bounds.
- (nullable NSString *)versionFromInput:(NSDictionary *)input withProtocolMessage:(nullable ARTProtocolMessage *)protocolMessage { if (protocolMessage.channelSerial == nil) { return nil; } int index = [[input artNumber:@"_index"] intValue]; + if (index < 0 || index > 999) { + return nil; + } return [NSString stringWithFormat:@"%@:%03d", protocolMessage.channelSerial, index]; // TM2p <channelSerial>:<padded_index> }
766-775: Consider optimizing the message index tracking.While the current implementation works, the comment about time constraints suggests room for improvement. Consider:
- Avoiding multiple array copies
- Using NSMutableArray's enumerateObjectsUsingBlock for better performance
- NSMutableArray *messages = [[input objectForKey:@"messages"] mutableCopy]; - - // There is probably a better way to do this, but I have limited time to implement TM2p - for (int i = 0; i < messages.count; i++) { - NSMutableDictionary *msgDict = [messages[i] mutableCopy]; - msgDict[@"_index"] = @(i); - messages[i] = msgDict; - } - message.messages = [self messagesFromArray:messages protocolMessage:message]; + NSArray *inputMessages = [input objectForKey:@"messages"]; + NSMutableArray *messages = [NSMutableArray arrayWithCapacity:inputMessages.count]; + [inputMessages enumerateObjectsUsingBlock:^(NSDictionary *msg, NSUInteger idx, BOOL *stop) { + NSMutableDictionary *msgDict = [msg mutableCopy]; + msgDict[@"_index"] = @(idx); + [messages addObject:msgDict]; + }]; + message.messages = [self messagesFromArray:messages protocolMessage:message];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Source/ARTBaseMessage.m(1 hunks)Source/ARTJsonLikeEncoder.m(4 hunks)Source/ARTMessage.m(3 hunks)Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h(1 hunks)Source/include/Ably/ARTMessage.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Source/ARTBaseMessage.m
🧰 Additional context used
📓 Learnings (1)
Source/ARTJsonLikeEncoder.m (1)
Learnt from: maratal
PR: ably/ably-cocoa#1998
File: Source/ARTJsonLikeEncoder.m:278-279
Timestamp: 2024-11-22T10:11:05.305Z
Learning: In `ARTMessage`, the `action` and `serial` fields are set by the realtime service and should not be serialized in the `messageToDictionary:` method in `Source/ARTJsonLikeEncoder.m`.
🔇 Additional comments (6)
Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h (1)
33-34: LGTM! Verify implementation consistency.
The method signature changes look good and follow proper Objective-C conventions. The nullable annotation is correctly used for the new protocolMessage parameter.
Let's verify that all callers have been updated and the implementation handles the new parameter correctly:
✅ Verification successful
Implementation verified and consistent
The implementation is correctly handling the new protocolMessage parameter:
- All callers have been updated to include the new parameter (mostly passing
nilfor backward compatibility) - The implementation in
ARTJsonLikeEncoder.mproperly uses the parameter for version handling viaversionFromInput:withProtocolMessage: - The changes maintain consistency across the codebase with proper nullable handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for method calls and implementation
# Search for method calls to ensure they've been updated with the new parameter
rg -t objc -t h "messageFromDictionary:|messagesFromArray:" --no-heading
# Search for the implementation of these methods
ast-grep --pattern 'messageFromDictionary:protocolMessage:|messagesFromArray:protocolMessage:'
Length of output: 1518
Script:
#!/bin/bash
# Let's check the implementation details in ARTJsonLikeEncoder.m to see how the protocolMessage parameter is used
rg -t objc "messageFromDictionary:.*protocolMessage:" -A 20 Source/ARTJsonLikeEncoder.m
Length of output: 2782
Source/include/Ably/ARTMessage.h (2)
7-40: LGTM! Well-structured enum with proper documentation.
The ARTMessageAction enum is well-implemented with:
- Clear documentation for each action
- Proper prefix convention (
ARTMessageAction) - Swift concurrency support via
NS_SWIFT_SENDABLE
59-60: Verify the implementation of serial population algorithm.
Based on the discussion in issue #2000, there's a complex algorithm for populating the serial field. While keeping this PR focused on the Chat SDK integration, we should ensure the implementation aligns with TM2k behavior.
Let's verify the current implementation:
✅ Verification successful
Let me search for more context about the serial field implementation, particularly focusing on the JsonLikeEncoder which seems to handle the core logic.
Let me try a different approach to search for the serial implementation and tests.
Based on the findings, I can now provide a conclusive response about the serial implementation.
Serial implementation aligns with TM2k behavior
The implementation in ARTJsonLikeEncoder.m shows the correct serial population logic:
- Serial is primarily populated from the input's "serial" field
- For message creation (ARTMessageActionCreate), if serial is not set, it falls back to the message version (marked with TM2k comment)
This matches the expected behavior discussed in issue #2000, with proper fallback mechanisms in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for serial population logic in the implementation files
rg -A 10 "serial.*=" Source/
Length of output: 6564
Script:
#!/bin/bash
# Search for ARTJsonLikeEncoder class definition and implementation
ast-grep --pattern 'interface ARTJsonLikeEncoder' Source/
ast-grep --pattern 'implementation ARTJsonLikeEncoder' Source/
# Search for any tests related to message serial
rg -l "test.*[Ss]erial" Source/
Length of output: 158
Script:
#!/bin/bash
# Search for message serial related tests and implementation
rg -A 5 "message\.serial" Source/
rg -A 5 "@property.*serial" Source/
# Look for test files
fd "Test.*\.m" Source/
Length of output: 1346
Source/ARTMessage.m (2)
101-101: Ensure consistent behavior with fromEncoded: method
The protocolMessage parameter is set to nil here as well. To maintain consistency:
- The handling of
protocolMessageshould align with the behavior infromEncoded:. - Any future changes to handle non-nil protocol messages should be applied to both methods.
71-71: Clarify the purpose of protocolMessage parameter
The protocolMessage parameter is being set to nil. If this parameter is intended to support the v2 chat protocol:
- Should it receive a non-nil value in certain cases?
- What's the expected behavior when it's nil?
✅ Verification successful
Based on the search results, I can now generate a final response. The code shows that the protocolMessage parameter is consistently set to nil in both fromEncoded methods, and there are no other usages of this parameter in ARTMessage.m. Let me check the implementation of messageFromDictionary:protocolMessage: to understand its purpose.
Now I can provide a final response based on the code context. The protocolMessage parameter's purpose is clear from the implementation.
Setting protocolMessage to nil is correct in this context
The protocolMessage parameter is used specifically for TM2p protocol feature to generate message versions using channel serial numbers. When decoding messages from JSON in fromEncoded:, passing nil is correct because:
- The version should already be present in the encoded message if needed
- The
versionFromInput:withProtocolMessage:method is only used as a fallback when no explicit version is provided - For locally decoded messages (via
fromEncoded:), there is no protocol message context to reference
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of messageFromDictionary:protocolMessage: to understand the expected values
ast-grep --pattern 'messageFromDictionary:$_ protocolMessage:$_'
Length of output: 66
Script:
#!/bin/bash
# Search for messageFromDictionary method declaration and implementation
ast-grep --pattern 'messageFromDictionary:$_ protocolMessage:$_'
# Search for any references to protocolMessage in the codebase
rg "protocolMessage" -A 3 -B 3
# Search for the ARTMessage class implementation
rg "^@implementation ARTMessage" -A 10
Length of output: 119373
Script:
#!/bin/bash
# Search for messageFromDictionary method implementation
rg -A 20 "messageFromDictionary:protocolMessage:" Source/
# Search for ARTMessage class implementation and its properties
rg -A 20 "@implementation ARTMessage" Source/
# Search for any references to protocolMessage in ARTMessage.m
rg "protocolMessage" Source/ARTMessage.m
Length of output: 2845
Script:
#!/bin/bash
# Search for messageFromDictionary:protocolMessage: implementation in ARTJsonLikeEncoder
rg -A 30 "messageFromDictionary:protocolMessage:" Source/ARTJsonLikeEncoder.m
Length of output: 77
Script:
#!/bin/bash
# Search for messageFromDictionary method in ARTJsonLikeEncoder with more context
rg -B 2 -A 30 "- \(ARTMessage \*\)messageFromDictionary:" Source/ARTJsonLikeEncoder.m
# Search for any references to versionFromInput method which might explain the protocolMessage usage
rg -B 2 -A 10 "versionFromInput:" Source/ARTJsonLikeEncoder.m
Length of output: 2083
Source/ARTJsonLikeEncoder.m (1)
286-291: LGTM! Implementation aligns with TM2 spec requirements.
The changes correctly implement:
- Default action to CREATE (TM2k)
- Serial fallback to version for CREATE actions (TM2k)
- CreatedAt fallback to timestamp for CREATE actions (TM2o)
Also applies to: 296-299
github-actions
bot
temporarily deployed
to
staging/pull/1998/jazzydoc
maratal
force-pushed
the
feature/ECO-5116-chat-support
branch
from
ab4583a to
deb9483
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1998/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Source/ARTJsonLikeEncoder.m (1)
766-774: Consider improving the message index tracking implementation.While the current implementation works, it could be improved:
- Create a new array instead of modifying the input array
- Use array enumeration with index instead of a C-style for loop
Consider this alternative implementation:
- NSMutableArray *messages = [[input objectForKey:@"messages"] mutableCopy]; - - // There is probably a better way to do this, but I have limited time to implement TM2p - for (int i = 0; i < messages.count; i++) { - NSMutableDictionary *msgDict = [messages[i] mutableCopy]; - msgDict[@"_index"] = @(i); - messages[i] = msgDict; - } + NSArray *inputMessages = [input objectForKey:@"messages"]; + NSMutableArray *messages = [NSMutableArray arrayWithCapacity:inputMessages.count]; + [inputMessages enumerateObjectsUsingBlock:^(NSDictionary *msg, NSUInteger idx, BOOL *stop) { + NSMutableDictionary *msgDict = [msg mutableCopy]; + msgDict[@"_index"] = @(idx); + [messages addObject:msgDict]; + }];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Source/ARTBaseMessage.m(1 hunks)Source/ARTJsonLikeEncoder.m(4 hunks)Source/ARTMessage.m(3 hunks)Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h(1 hunks)Source/include/Ably/ARTMessage.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Source/ARTBaseMessage.m
- Source/ARTMessage.m
- Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h
- Source/include/Ably/ARTMessage.h
🧰 Additional context used
📓 Learnings (1)
Source/ARTJsonLikeEncoder.m (1)
Learnt from: maratal
PR: ably/ably-cocoa#1998
File: Source/ARTJsonLikeEncoder.m:278-279
Timestamp: 2024-11-22T10:11:05.305Z
Learning: In `ARTMessage`, the `action` and `serial` fields are set by the realtime service and should not be serialized in the `messageToDictionary:` method in `Source/ARTJsonLikeEncoder.m`.
🔇 Additional comments (3)
Source/ARTJsonLikeEncoder.m (3)
269-275: LGTM! Well-structured version string generation.
The helper method correctly implements the version string format <channelSerial>:<padded_index> with proper null checks and 3-digit padding for the index.
286-291: LGTM! Proper implementation of v2 chat protocol fields.
The method correctly:
- Sets default action to
ARTMessageActionCreate - Handles version from input or generates it using the helper method
- Sets serial from input or version for create actions
- Sets createdAt from input or timestamp for create actions
Also applies to: 296-299
Line range hint 306-318: LGTM! Consistent propagation of protocol message.
The method correctly passes the protocol message to each message's deserialization, maintaining proper error handling.
github-actions
bot
temporarily deployed
to
staging/pull/1998/jazzydoc
maratal
force-pushed
the
feature/ECO-5116-chat-support
branch
from
deb9483 to
30df383
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1998/features
github-actions
bot
temporarily deployed
to
staging/pull/1998/jazzydoc
maratal
force-pushed
the
feature/ECO-5116-chat-support
branch
from
30df383 to
9f1cbd9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
Test/Tests/UtilitiesTests.swift (4)
489-525: Good test coverage, consider adding edge cases.The test effectively verifies the core functionality of serial field derivation from version for create actions. Consider enhancing it with:
- More descriptive variable names than 'pm' (e.g., 'protocolMessage')
- Edge cases like empty or invalid version strings
- Additional action types besides just 'unset'
- var pm = try jsonEncoder.decodeProtocolMessage(data) + var protocolMessage = try jsonEncoder.decodeProtocolMessage(data)
528-552: Consider additional validation for version field generation.The test effectively verifies version field generation for sequential messages. Consider adding:
- Validation of channelSerial format constraints
- Edge cases with empty or malformed channelSerial
- Verification of sequence number padding (e.g., "001" vs "1")
- Test cases for maximum sequence number handling
555-590: Enhance timestamp handling test cases.Consider adding test cases for:
- Invalid timestamp formats
- Timestamp format consistency validation
- Different timezone representations
- Empty or null timestamp values
487-590: Consider additional test coverage areas.The new tests provide good coverage for the v2 chat protocol message handling. Consider adding tests for:
- Error handling for malformed messages
- Protocol version compatibility checks
- Message size limits with the new fields
- Integration tests with the actual transport layer
Test/Tests/RestClientChannelTests.swift (2)
571-571: LGTM! Consider adding protocol version test cases.The addition of the
protocolMessageparameter withnilvalue maintains backward compatibility while supporting the v2 chat protocol.Consider adding test cases that explicitly verify message encoding with different protocol versions to ensure proper handling of both v1 and v2 messages.
Line range hint
1-1200: Consider adding explicit protocol version transition tests.While the test suite covers both v1 and v2 protocol functionality, it would be valuable to add specific test cases that verify:
- Protocol version negotiation
- Graceful handling of mixed protocol version messages
- Version upgrade/downgrade scenarios
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
Source/ARTBaseMessage.m(1 hunks)Source/ARTJsonLikeEncoder.m(4 hunks)Source/ARTMessage.m(3 hunks)Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h(1 hunks)Source/include/Ably/ARTMessage.h(2 hunks)Test/Tests/RestClientChannelTests.swift(1 hunks)Test/Tests/UtilitiesTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Source/ARTBaseMessage.m
- Source/ARTMessage.m
- Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h
- Source/include/Ably/ARTMessage.h
🧰 Additional context used
📓 Learnings (1)
Source/ARTJsonLikeEncoder.m (1)
Learnt from: maratal
PR: ably/ably-cocoa#1998
File: Source/ARTJsonLikeEncoder.m:278-279
Timestamp: 2024-11-22T10:11:05.305Z
Learning: In `ARTMessage`, the `action` and `serial` fields are set by the realtime service and should not be serialized in the `messageToDictionary:` method in `Source/ARTJsonLikeEncoder.m`.
🔇 Additional comments (4)
Source/ARTJsonLikeEncoder.m (3)
269-275: LGTM: Version string generation follows TM2p specification.
The implementation correctly generates version strings in the format <channelSerial>:<padded_index> with proper null checks and 3-digit padding.
286-291: LGTM: Message field handling follows TM2 specifications.
The implementation correctly:
- Sets default action to ARTMessageActionCreate
- Handles version and serial fields per TM2p/TM2k specs
- Sets createdAt field per TM2o spec
Also applies to: 296-299
306-313: LGTM: Consistent propagation of protocolMessage parameter.
The method correctly passes the protocolMessage to each message during deserialization.
Test/Tests/RestClientChannelTests.swift (1)
Line range hint 1-1200: LGTM! Well-structured test suite.
The test suite demonstrates:
- Clear test case organization and naming
- Comprehensive coverage of functionality
- Proper error handling and edge cases
- Consistent patterns for setup, execution and verification
Added new fields to the
ARTMessageto fit into new v2 chat protocol.Docstrings PR
A par of a bigger task - #2000
Summary by CodeRabbit
Summary by CodeRabbit
New Features
action,serial,version, andcreatedAtin theARTMessageclass for enhanced message identification and versioning.ARTMessageActionto define various message action types.protocolMessage, improving message processing._extrasproperty during message duplication.ARTJsonLikeEncoderto ensure proper decoding of protocol messages based on action types.Bug Fixes