Rename fields #427
Rename fields #427
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces significant updates to the documentation and implementation of the Ably Chat SDK, focusing on message handling. Key changes include the adoption of versioning for message updates, replacing previous action-based logic. The Changes
Possibly related PRs
Suggested reviewers
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 (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
vladvelici
force-pushed
the
rename-fields-as-agreed
branch
2 times, most recently
from
94bb83d to
31967fa
Compare
vladvelici
force-pushed
the
rename-fields-as-agreed
branch
from
31967fa to
91a7a1f
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
src/core/message.ts (1)
217-221: Document theoperationParameter in ConstructorThe
operationparameter in theDefaultMessageconstructor is used but not explicitly documented. Providing documentation will enhance code readability and maintainability.Add JSDoc comments for the
operationparameter:/** * @param operation - Details of the latest operation that updated the message. */src/core/message-parser.ts (1)
52-54: Improve Error Messaging for MissingversionFieldThe error message when
message.versionis missing could be more informative to aid in debugging.Consider including the
message.serialor other identifying information in the error message:- throw new Ably.ErrorInfo(`received incoming message without version`, 50000, 500); + throw new Ably.ErrorInfo(`Message with serial ${message.serial} is missing the 'version' field`, 50000, 500);test/react/hooks/use-messages.integration.test.tsx (1)
319-319: Consider consolidating persistence wait times into a shared helper function.There are multiple instances of waiting for persistence with the comment "wait for persistence - this will not be necessary in the future". While this is noted as temporary, consider consolidating these waits into a shared helper function to make future removal easier.
+const waitForPersistence = () => new Promise((resolve) => setTimeout(resolve, 3000)); + // Usage: -await new Promise((resolve) => setTimeout(resolve, 3000)); // wait for persistence +await waitForPersistence(); // wait for persistenceAlso applies to: 391-391, 436-436, 456-456, 528-528, 548-549
test/core/messages.test.ts (1)
Line range hint
92-118: Align mock response with actual API response structure.Based on previous feedback, the mock response for
chatApi.deleteMessageshould only includeversionandtimestamp, as these are the only fields returned by the actual API.Simplify the mock response to match the actual API:
vi.spyOn(chatApi, 'deleteMessage').mockResolvedValue({ version: '01672531200001-123@abcdefghij:0', timestamp: deleteTimestamp, - text: 'hello there', - clientId: 'clientId', - createdAt: sendTimestamp, - roomId: context.room.roomId, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
README.md(2 hunks)demo/src/containers/Chat/Chat.tsx(1 hunks)src/core/action-metadata.ts(1 hunks)src/core/chat-api.ts(2 hunks)src/core/index.ts(1 hunks)src/core/message-parser.ts(3 hunks)src/core/message.ts(3 hunks)src/core/messages.ts(2 hunks)test/core/chat.integration.test.ts(2 hunks)test/core/helpers.test.ts(2 hunks)test/core/message-parser.test.ts(4 hunks)test/core/message.test.ts(14 hunks)test/core/messages.integration.test.ts(18 hunks)test/core/messages.test.ts(12 hunks)test/helper/expectations.ts(3 hunks)test/react/hooks/use-messages.integration.test.tsx(7 hunks)test/react/hooks/use-messages.test.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
test/core/messages.test.ts (1)
Learnt from: vladvelici
PR: ably/ably-chat-js#378
File: test/core/messages.test.ts:0-0
Timestamp: 2024-11-12T07:31:58.636Z
Learning: In `test/core/messages.test.ts`, the mocked response for `chatApi.deleteMessage` in the test case `'should be able to delete a message and get it back from response'` should only include `serial` and `deletedAt`, as the real `deleteMessage` API response does not provide additional properties like `text`, `clientId`, `createdAt`, or `roomId`.
🔇 Additional comments (22)
src/core/chat-api.ts (1)
34-52: Consolidate Response Interfaces for Message Operations
The introduction of the MessageOperationResponse interface consolidates the response structures for both delete and update message operations, improving code maintainability and reducing duplication. This is a positive change that enhances consistency across the API.
test/core/messages.integration.test.ts (2)
118-127: Ensure Consistent Validation of Deleted Message Properties
The assertions for the deleted message properties are thorough. Ensure that all relevant properties, such as operation, are consistently validated to catch potential issues early.
270-289: Ensure Tests Reflect Current Backend Behavior
The comments mention that the history API does not currently materialize deleted or updated messages. If this behavior changes in the backend, the tests might fail unexpectedly.
Double-check whether the backend now supports materialized deleted and updated messages in history. If so, update the tests accordingly.
src/core/action-metadata.ts (1)
Line range hint 2-10: Update Documentation to Reflect Changes in Metadata Usage
The comment now correctly refers to the "operations" field instead of "latestActionDetails," ensuring that the documentation accurately reflects the current codebase.
src/core/message-parser.ts (1)
72-86: Ensure All Required Fields Are Included in DefaultMessage Construction
When constructing the DefaultMessage, all necessary fields are correctly included, and the mapping aligns with the updated Message interface.
src/core/index.ts (1)
32-36: Verify backward compatibility of the Operation type alias
The renaming from MessageActionDetails to Operation with an alias maintains backward compatibility. However, we should ensure all consumers are updated to use the new Operation type.
✅ Verification successful
Let me search for usages of the new Operation type to understand the impact better.
Type renaming is correctly implemented with no compatibility issues
The verification shows that MessageActionDetails is only used as an alias in the export statement, while the new Operation type is properly used throughout the codebase in message.ts, message-parser.ts, and other core files. The alias successfully maintains backward compatibility while the codebase has been updated to use the new type name.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usages of MessageActionDetails
rg "MessageActionDetails" --type ts
Length of output: 90
Script:
#!/bin/bash
# Search for usages of Operation type
rg "Operation[^a-zA-Z]" --type ts -A 2
Length of output: 4624
test/helper/expectations.ts (2)
4-5: LGTM! Function simplification improves clarity
The renaming from extractCommonKeys to extractExpectedKeys and the simplification of its logic makes the code more focused and easier to understand.
43-43: LGTM! Enhanced error message improves debugging
The addition of the received type in the error message will help developers quickly identify type mismatches during testing.
test/core/helpers.test.ts (2)
16-16: LGTM: Version field replaces serial field.
The change from serial to version aligns with the new message versioning strategy.
50-51: Verify timestamp handling in DefaultMessage constructor.
The constructor now takes two date parameters. Ensure that both timestamps are properly utilized in the message lifecycle.
✅ Verification successful
Let me analyze the usage of timestamps in DefaultMessage constructor based on the results.
Based on the gathered information, I can now provide a final response:
Both timestamps are properly handled in DefaultMessage implementation
The DefaultMessage constructor correctly accepts and stores both timestamps:
createdAt: Original message creation timestamptimestamp: Current operation timestamp
All instances of DefaultMessage across the codebase consistently provide both timestamps, with proper differentiation in update/delete operations where timestamp reflects the operation time while createdAt preserves the original creation time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of both timestamps in DefaultMessage
# Look for any instances where DefaultMessage is instantiated to ensure consistent usage
ast-grep --pattern 'new DefaultMessage($$$)'
Length of output: 14710
Script:
#!/bin/bash
# Let's examine the DefaultMessage class implementation to understand how these timestamps are used
ast-grep --pattern 'class DefaultMessage {
$$$
}'
Length of output: 53
Script:
#!/bin/bash
# Let's try a different approach to find the DefaultMessage class implementation
rg "class DefaultMessage" -A 10
Length of output: 1304
Script:
#!/bin/bash
# Let's see the complete DefaultMessage class implementation
rg "class DefaultMessage implements Message" -A 20
Length of output: 1186
test/core/message-parser.test.ts (2)
84-95: LGTM: Proper validation of version field.
The test case correctly validates that messages without a version field are rejected.
156-157: LGTM: Updated assertions for action and operation fields.
The test now properly verifies the new message structure with action and operation fields.
test/core/message.test.ts (2)
Line range hint 176-220: LGTM: Comprehensive version comparison tests.
The test cases properly validate version comparison behavior and error handling. The error messages are clear and descriptive.
Line range hint 226-294: LGTM: Well-structured parametrized tests.
The test structure using describe.each provides good coverage of version comparison scenarios while maintaining code readability.
test/react/hooks/use-messages.test.tsx (2)
119-129: LGTM: Message object structure updated correctly.
The message object structure has been properly updated to include the new version-based properties (version, versionBefore, versionAfter, versionEqual) and metadata properties (deletedBy, updatedBy, deletedAt, updatedAt).
186-187: LGTM: Timestamp properties added correctly.
The message object now includes both createdAt and updatedAt timestamps using the same value, which is appropriate for a new message in the test.
demo/src/containers/Chat/Chat.tsx (1)
41-41: LGTM: Message version comparison updated correctly.
The message comparison logic has been properly updated to use versionBefore instead of actionBefore, aligning with the new version-based message update system.
test/react/hooks/use-messages.integration.test.tsx (1)
202-204: LGTM: Message update assertions updated correctly.
The test assertions have been properly updated to verify the new message structure:
- Checking
actionproperty instead oflatestAction - Verifying the
operationobject's properties
src/core/messages.ts (2)
514-533: LGTM! Well-structured message update implementation.
The changes properly implement message versioning and operation metadata handling. The debug logging will be helpful for troubleshooting.
541-561: LGTM! Robust message deletion implementation.
The changes properly implement message deletion with versioning and operation metadata. Using this._roomId instead of message.roomId is a good practice for reliability.
README.md (1)
307-317: LGTM! Clear and comprehensive documentation.
The documentation effectively explains the new message versioning system and provides clear examples for handling out-of-order updates.
test/core/messages.test.ts (1)
Line range hint 187-201: LGTM! Comprehensive test coverage.
The test changes properly verify:
- Operation metadata handling
- Message versioning
- Client ID assertions
Also applies to: 267-289, 291-296
vladvelici
force-pushed
the
rename-fields-as-agreed
branch
from
91a7a1f to
df76919
Compare
vladvelici
force-pushed
the
rename-fields-as-agreed
branch
from
df76919 to
b428369
Compare
Upon our integration tests starting to fail, we noticed that Realtime seems to have renamed the `latestAction` property to `action`. This change has not yet been reflected in the spec, but has been in JS [1]. I’ve created an issue to get the spec updated [2] but since this test failure is stopping us from merging anything else I am going to rename this property until we get a more definitive answer. Part of #169. [1] ably/ably-chat-js#427 [2] ably/specification#254
Rename Message fields and fix tests for history. Enable materialised history tests.
Jira: CHA-734
Summary by CodeRabbit
Release Notes
Documentation
New Features
version,operation, and improved error handling.Bug Fixes
Tests