[ECO-5157] [ECO-5158] Edits and Deletes #227
Conversation
WalkthroughThis pull request updates the dependency for ably-cocoa from version 1.2.39 to 1.2.40 in both the Package.resolved and Package.swift files. In addition, the changes enhance the messaging functionality by introducing new properties (version, timestamp, operation) to the Message structure and adjusting various mocks, API endpoints, and tests accordingly. New methods for updating and deleting messages have been added across the ChatAPI, DefaultMessages, and protocol definitions while error handling and initialization checks have been refined. The core public interfaces remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ChatAPI
participant DefaultMessages
participant Server
Client->>ChatAPI: updateMessage(newMessage, description, metadata)
ChatAPI->>DefaultMessages: Validate & process update request
DefaultMessages->>Server: Send update request with version, timestamp, operation info
Server-->>DefaultMessages: Return update response
DefaultMessages-->>ChatAPI: Return updated Message
ChatAPI-->>Client: Deliver updated Message
sequenceDiagram
participant Client
participant ChatAPI
participant DefaultMessages
participant Server
Client->>ChatAPI: deleteMessage(message, params)
ChatAPI->>DefaultMessages: Validate & process delete request
DefaultMessages->>Server: Send delete request with version, timestamp, operation info
Server-->>DefaultMessages: Return delete response
DefaultMessages-->>ChatAPI: Return deleted Message
ChatAPI-->>Client: Deliver deleted Message
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
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 (
|
umair-ably
force-pushed
the
ECO-5157-5158
branch
4 times, most recently
from
434133f to
df2d9e2
Compare
github-actions
bot
temporarily deployed
to
staging/pull/227/AblyChat
![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 (4)
Sources/AblyChat/Messages.swift (1)
49-64: Consider clarifying errors that can be thrown by this method.
The doc block describes the parameters and return type well, but it might be helpful to specify which errors could be thrown (e.g., network issues, invalid parameters, authentication errors) so the user knows what to handle.Example/AblyChatExample/Mocks/MockClients.swift (2)
138-141: Consider including a mocked version in the message.
You're currently assigning an empty string toversion. If you want these mocks to closely reflect real behavior, consider referencing a generated or incremental version.
147-153: These methods are not yet implemented.
Stubs forupdateanddeletewithfatalErrorblock integration testing. Implement or provide a suitable placeholder to allow tests to run.Would you like a sample implementation or an extended test harness to track progress on these methods?
Sources/AblyChat/DefaultMessages.swift (1)
293-304: Well-implemented conversion with proper error handlingThe extension provides a clean conversion from ARTMessageOperation to MessageOperation with appropriate validation of required fields.
Consider enhancing the error message to include more context about which message caused the error, such as the message serial or ID, to aid in debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)Example/AblyChatExample/Mocks/Misc.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(3 hunks)Example/AblyChatExample/Mocks/MockRealtime.swift(1 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/AblyChat/ChatAPI.swift(3 hunks)Sources/AblyChat/ChatClient.swift(1 hunks)Sources/AblyChat/DefaultMessages.swift(4 hunks)Sources/AblyChat/Events.swift(1 hunks)Sources/AblyChat/Message.swift(4 hunks)Sources/AblyChat/Messages.swift(2 hunks)Tests/AblyChatTests/ChatAPITests.swift(3 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/MessageSubscriptionTests.swift(1 hunks)Tests/AblyChatTests/MessageTests.swift(4 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: SPM (Xcode 16)
🔇 Additional comments (39)
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (2)
108-109: Added version and timestamp fields to mock message responseThe mock response now includes version and timestamp properties, which aligns with the enhancements to the Message structure for supporting edit and delete operations.
119-119: Added version field to second mock messageThe version field is correctly added to the second mock message. Note that a timestamp field is included in the first message but not in the second one. This inconsistency might be intentional for testing different scenarios, but it's worth verifying.
Example/AblyChatExample/Mocks/MockRealtime.swift (1)
13-13: Implemented clientId in MockRealtimeThe clientId property now returns "AblyTest" instead of triggering a fatal error. This change supports the new message editing and deletion functionality which likely requires a valid client identifier for ownership verification.
Package.swift (1)
23-23:✅ Verification successful
Updated ably-cocoa dependency version
The dependency on ably-cocoa has been updated from 1.2.39 to 1.2.40. This is likely to incorporate new features or bug fixes that support the new message editing and deletion functionality.
🏁 Script executed:
#!/bin/bash # Description: Check for changelog or release notes for ably-cocoa 1.2.40 # Check for changelog or release notes on GitHub curl -s https://api.github.com/repos/ably/ably-cocoa/releases/tags/1.2.40 | grep -E "body|name|published_at" | head -n 20Length of output: 956
Dependency version update verified and approved
The update in
Package.swiftsuccessfully changes the ably-cocoa version from 1.2.39 to 1.2.40. The release notes confirm the inclusion of the newARTMessageOperationtype, which supports message editing and deletion, with the release published on 2025-02-26. No further changes are required.AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
2-2: Updated Package.resolved for ably-cocoa 1.2.40The Package.resolved file has been updated to reflect the dependency change to ably-cocoa 1.2.40, including the new revision hash and originHash. This is consistent with the change made in Package.swift.
Also applies to: 9-10
Sources/AblyChat/Events.swift (2)
11-12: Nice addition of message update and delete operations.The new
updateanddeletecases in theMessageActionenum align with the PR objectives to enable users to edit and delete their messages.
18-21: Properly mapped Ably's realtime actions to internal action types.The implementation correctly handles the mapping from Ably's realtime action types to the application's action types for the new update and delete operations.
Example/AblyChatExample/Mocks/Misc.swift (1)
21-25: Mock messages updated to match new Message structure.The mock implementation correctly initializes the new properties (
version,timestamp, andoperation) with sensible default values, maintaining consistency with the updated Message struct.Tests/AblyChatTests/MessageTests.swift (1)
13-16: Test messages updated with new properties.The test messages have been properly updated to include the new
versionandtimestampproperties.However, I notice that
earlierMessageandlaterMessagehave the sameversionvalue ("ABC123@1631840000000-5:2") despite having differentserialvalues. Was this intentional or should these be different values?Also applies to: 26-29, 39-42, 85-88
Sources/AblyChat/ChatClient.swift (1)
75-80: Good implementation of clientID property with proper error handling.The implementation correctly retrieves the client ID from the underlying Realtime client and includes appropriate error handling with a clear error message when the client ID is not available.
Package.resolved (1)
2-2: Library dependency update to support edit and delete functionality.The ably-cocoa library has been updated from version 1.2.39 to 1.2.40, which likely includes the necessary changes to support message editing and deletion functionality.
Also applies to: 9-10
Tests/AblyChatTests/MessageSubscriptionTests.swift (1)
29-29: Constructor updated to support new Message properties.The Message constructor has been updated to include the new
versionandtimestampparameters, which aligns with the enhanced message functionality for editing and deleting messages.Tests/AblyChatTests/IntegrationTests.swift (5)
162-165: Well-structured section for testing message editing and deletion.The new test section clearly identifies its purpose and efficiently reuses existing test resources for better integration.
169-177: Message update functionality correctly implemented and tested.The message update functionality is appropriately implemented and tests the copy functionality to create a new message with edited content while preserving necessary properties.
179-194: Comprehensive verification of edited message properties.The test thoroughly verifies all relevant properties of the edited message to ensure consistency and correctness.
205-217: Comprehensive verification of deleted message properties.The test thoroughly verifies all relevant properties of the deleted message to ensure consistency and correctness.
197-203:❓ Verification inconclusive
Follow up on the metadata issue mentioned in the TODO comment.
There's a metadata issue noted in line 201 that needs to be addressed. The comment references issue #226.
🌐 Web query:
What is the issue with non-string metadata values in Ably Chat Swift?💡 Result:
Ably Chat's Swift SDK has a known limitation regarding non-string metadata values in messages. The issue stems from how the SDK handles type conversion and validation for message metadata properties:
Strict Type Enforcement
The Swift implementation requires metadata values to be strictlyStringtypes[7][10]. When developers attempt to use non-string values (like numbers, booleans, or custom objects), the SDK fails to serialize them properly and may throw type mismatch errors during message publication[6][10].Lack of Automatic Conversion
Unlike some other Ably client libraries that automatically convert primitive types to strings, the Swift SDK currently doesn't perform this conversion for metadata values[10]. Developers must manually convert all non-string values before adding them to message metadata[7].Validation Gap
While Ably's documentation explicitly states that metadata should be treated as untrusted user input[7], the Swift SDK doesn't provide built-in validation for metadata types. This can lead to runtime errors when unexpected data types are used[6][10].Recommended Workaround:
// Explicitly convert all metadata values to String type let numericMetadata = 42 let message = Message( text: "Hello", metadata: [ "important": String(true), "value": String(numericMetadata), "timestamp": String(Date().timeIntervalSince1970) ] )This strict typing requirement aligns with Swift's strong type safety features but differs from more flexible implementations in other languages. The issue is particularly noticeable when migrating chat implementations from other platforms that allow dynamic typing in metadata[4][7]. Developers should ensure all metadata values are strings before message publication to avoid serialization errors[7][10].
Citations:
- 1: https://www.ably.io/documentation/general/events
- 2: https://www.npmjs.com/package/@ably%2Fchat
- 3: https://ably.com/docs/api/rest-sdk/authentication
- 4: https://forums.swift.org/t/getting-type-metadata-on-non-darwin-platforms/28540
- 5: https://github.com/ably/docs/blob/main/content/chat/setup.textile
- 6: Improve testing for messages functionality #195
- 7: https://ably.com/docs/chat/rooms/messages
- 8: https://systemsdigest.com/videos/channel-metadata-api-ably-quick-bits
- 9: https://developer.apple.com/documentation/foundation/app_extension_support/supporting_suggestions_in_your_app_s_share_extension
- 10: https://ably.com/docs/api/realtime-sdk/types
ACTION: Verify and update metadata conversion in message deletion.
- The Swift SDK requires that all metadata values be strings. Passing non-string values (numbers, booleans, etc.) triggers a decoding error as documented in issue #226.
- In this test, metadata is temporarily set to
nilto bypass the error. However, for real-world usage, non-string metadata should be explicitly converted to a string (e.g., usingString(describing:)).- Please review and adjust the metadata handling to ensure that any non-string values are converted to strings before being passed to the deletion API.
Tests/AblyChatTests/ChatAPITests.swift (2)
50-52: Updated test expectations for new Message properties.Test expectations have been updated to include the new
versionandtimestampproperties, maintaining test validity with the enhanced Message model.
148-150: Consistent implementation of new Message properties across test cases.The message construction in this test consistently includes the new properties, with appropriate values for each test case. The second message appropriately uses a nil timestamp to test that scenario.
Also applies to: 160-162
Sources/AblyChat/Messages.swift (3)
43-43: Documentation is clear and consistent.
Returning a message with the action set as.createaccurately reflects the behavior. No concerns here.
65-79: Deletion doc block looks good.
The documentation for thedeletemethod is consistent with the rest of the API. No issues noted.
141-189: New params structs are well-defined.
BothUpdateMessageParamsandDeleteMessageParamsmatch the established style. The documentation is thorough, and the initializers make the usage straightforward.Example/AblyChatExample/Mocks/MockClients.swift (2)
18-18: Verify fallback client ID handling.
Defaulting to"AblyTest"can be convenient for tests, but ensure it doesn't mask missing client IDs in production.
111-114: Revisit hardcoded default values.
In test mocks, returning empty values (version: "", timestamp: Date(), operation: nil) is acceptable. However, consider whether you need more realistic values to better simulate production scenarios.Sources/AblyChat/ChatAPI.swift (5)
28-40: Utility struct provides a clean abstraction.
MessageOperationResponseand the accompanying typealiases neatly capture version and timestamp.
64-78: Seamless integration of new fields.
The inclusion ofversionandtimestampin the returnedMessagealigns well with the new REST response format.
80-130: Update message logic looks consistent.
The PUT semantics and overwriting omitted fields align with the specification. Ensure downstream consumers handle missing fields gracefully.
132-170: Delete message flow is well-structured.
Returning the finalMessagewith.deleteaction helps unify the experience between Realtime and REST-based edits.
188-188: Error handling comment is accurate.
All references to thrown errors from the REST API (send, update, delete) are consistent. No further changes needed.Sources/AblyChat/DefaultMessages.swift (4)
79-81: Good addition of version validationAdded guard statement ensures incoming messages have a version, consistent with the updated Message struct requirements.
95-108: Clear comment explaining thetryplacementThe comment effectively explains why the
tryis placed at the Message initialization level rather than directly on the operation conversion, addressing previous review feedback.
138-140: New update method aligns with PR objectiveThe addition of this method enables message editing functionality as described in the PR objectives.
142-144: New delete method aligns with PR objectiveThe addition of this method enables message deletion functionality as described in the PR objectives.
Sources/AblyChat/Message.swift (6)
13-16: Well-documented type aliasClear documentation for the new OperationMetadata type alias.
85-101: Good documentation for new message propertiesThe added properties (version, timestamp, and operation) are well-documented with clear explanations of their purpose, which aligns with the PR objective of enabling message editing and deletion.
102-114: Updated initializer accommodates new propertiesThe initializer has been appropriately updated to include the new properties, with a sensible default for operation.
116-139: Useful helper method with clear documentationThe copy function is well-documented and provides a convenient way to create updated versions of messages for the edit functionality.
142-146: Well-structured operation modelThe MessageOperation struct effectively captures the necessary information for operations that modify messages.
148-171: Updated JSON decoding handles new propertiesThe JSONObjectDecodable implementation has been appropriately updated to handle the new properties, with proper error handling for required fields and optional handling for optional fields.
umair-ably
force-pushed
the
ECO-5157-5158
branch
from
df2d9e2 to
140873c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Sources/AblyChat/ChatAPI.swift (1)
132-170: Complete deleteMessage implementation with proper documentation.The deleteMessage method is well-documented with requirements (CHA-M9) and uses the appropriate POST method. The implementation correctly constructs the Message response with all necessary fields. Consider consolidating common code between updateMessage and deleteMessage to improve maintainability.
You could extract the common code for constructing a response Message into a private helper method to reduce duplication between updateMessage and deleteMessage methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)Example/AblyChatExample/Mocks/Misc.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(3 hunks)Example/AblyChatExample/Mocks/MockRealtime.swift(1 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/AblyChat/ChatAPI.swift(3 hunks)Sources/AblyChat/ChatClient.swift(1 hunks)Sources/AblyChat/DefaultMessages.swift(4 hunks)Sources/AblyChat/Events.swift(1 hunks)Sources/AblyChat/Message.swift(4 hunks)Sources/AblyChat/Messages.swift(2 hunks)Tests/AblyChatTests/ChatAPITests.swift(3 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(2 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)Tests/AblyChatTests/MessageSubscriptionTests.swift(1 hunks)Tests/AblyChatTests/MessageTests.swift(4 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift
- Example/AblyChatExample/Mocks/MockRealtime.swift
- Sources/AblyChat/Events.swift
- Package.swift
- AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved
- Sources/AblyChat/ChatClient.swift
- Tests/AblyChatTests/IntegrationTests.swift
- Example/AblyChatExample/Mocks/Misc.swift
- Tests/AblyChatTests/MessageSubscriptionTests.swift
- Tests/AblyChatTests/ChatAPITests.swift
- Package.resolved
- Sources/AblyChat/Message.swift
- Example/AblyChatExample/Mocks/MockClients.swift
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: SPM (Xcode 16)
🔇 Additional comments (23)
Tests/AblyChatTests/MessageTests.swift (4)
13-15: New properties integrated for message versioning.The
headers,version, andtimestampproperties have been added to theearlierMessageinitialization, which is in line with the PR's objective of adding edit and delete functionality. Theversionproperty matches theserialvalue, which is the expected behavior for message tracking.
26-28: Verify version consistency inlaterMessage.I notice that the
versionproperty uses the same value asearlierMessageeven though theserialproperty is different. Is this intentional? In a real-world scenario, would we expect the version to be different if the serial is different?
39-41: Consistent versioning pattern maintained forinvalidMessage.The
versionproperty correctly uses the same value as theserialproperty for this invalid test case.
85-87: Version property correctly integrated in equality test case.The
versionproperty in the duplicate message correctly matches the version inearlierMessage, which ensures consistent testing of equality between messages.Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)
25-26: ExtendedMessageToEmitstruct to support message operations.The addition of
operationandversionproperties to theMessageToEmitstruct aligns with the PR objectives of enabling message editing and deletion. These properties will track the operation type and message version during these actions.
152-153: Message operation properties properly propagated during subscribe.Setting the
operationandversionfields on the emitted message ensures that mock messages correctly simulate the behavior of real messages with edit/delete operations.Tests/AblyChatTests/DefaultMessagesTests.swift (2)
92-94: Added operation and version properties to mock message.The addition of
operation: nilandversion: ""properties aligns with the changes to theMessageToEmitstruct and ensures that the test remains compatible with the updated message structure.
127-129: Consistent property initialization in metadata extraction test.Similar to the headers extraction test, the metadata extraction test has been updated with the new
operationandversionfields to maintain consistency with the updated message structure.Sources/AblyChat/Messages.swift (5)
43-43: Clarified return value ofsendmethod.The documentation now explicitly states that the returned message will have its action set to
.create, which improves the API clarity.
49-64: Added new method for updating messages.The new
updatemethod is well-documented and follows a consistent pattern with the existingsendmethod. The method clearly describes the parameters and return value, including the note about possible message reception via subscription before the method returns.
65-79: Added new method for deleting messages.The
deletemethod is well-documented and follows the same pattern as theupdatemethod. It provides a clear interface for users to delete messages and includes appropriate documentation about return values and subscription behavior.
141-175: Created well-structured params for message updates.The
UpdateMessageParamsstruct is well-designed with clear documentation for each property. It includes options for description and metadata, which provides flexibility for clients when updating messages.
177-189: Created params struct for message deletion.The
DeleteMessageParamsstruct provides a clean interface for optional parameters when deleting messages. The structure is consistent with other parameter structs in the codebase.Sources/AblyChat/DefaultMessages.swift (5)
79-81: Proper validation added for message version.Adding this guard check ensures that incoming messages have a version field, which is crucial for implementing the edit and delete functionality.
95-108: Message initialization updated with version and operation tracking.The Message initialization now includes version, timestamp, and operation parameters, which are necessary for message versioning and edit/delete capabilities. The comment clarifies why the
tryis placed at the Message initialization rather than the operation conversion.
138-140: New update method added for message edits.This method enables users to update messages by delegating to the ChatAPI. The implementation is clean and follows the pattern established by other methods in this class.
142-144: New delete method added for message removal.This method enables users to delete messages by delegating to the ChatAPI. The implementation is clean and follows the pattern established by other methods in this class.
293-304: Helper extension to convert ARTMessageOperation to ChatOperation.This extension handles the conversion from Ably's operation type to the internal MessageOperation type, with proper validation of the clientId field. The implementation correctly throws an error when clientId is missing, preventing potential issues downstream.
Sources/AblyChat/ChatAPI.swift (5)
28-36: New MessageOperationResponse struct to handle operation responses.This struct provides a common response format for both update and delete operations, containing version and timestamp information. The implementation follows the existing pattern for JSON decoding and is well-structured.
38-39: Type aliases for response types improve code readability.Using type aliases for UpdateMessageResponse and DeleteMessageResponse makes the code more maintainable and self-documenting while reusing the common MessageOperationResponse structure.
64-78: sendMessage now includes version and timestamp in returned Message.The method has been updated to include version (set to serial) and timestamp in the returned Message, consistent with the new message model. The creation of createdAtDate as a separate variable improves code readability.
80-130: Comprehensive updateMessage implementation with proper documentation.The updateMessage method is well-documented with requirements (CHA-M8) and implements the PUT semantics correctly. The implementation handles optional parameters appropriately and constructs a proper Message response with all required fields.
188-189: Updated comment for comprehensive error handling.The comment now correctly references all the methods (send, update, delete) that throw errors from the REST API, ensuring consistent documentation.
Updates the public API to allow users to edit and delete their messages.
Example app will be updated in a following PR. Additional unit tests will be added after #219 is merged. Integration tests have been extended adequately until this is done.
API differs between edit and delete due to this conversation... https://ably-real-time.slack.com/archives/C02NY1VT3LY/p1740391041340959
Summary by CodeRabbit
New Features
Chores