Add update message capability in the SDK, react hooks, and demo app #378
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 messaging functionality within the chat application. Key changes include the addition of message editing and deletion capabilities, modifications to the Changes
Assessment against linked issues
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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
demo/src/main.tsx (2)
36-41: Consider removing commented-out code and documenting configuration options.The commented-out Ably client initialization contains valuable information about alternative configuration options. However, keeping commented code in the codebase can lead to confusion and maintenance issues over time.
Consider the following actions:
- Remove the commented-out code.
- Document the configuration options in a separate file or in the project's README.
- If these options are still relevant, implement them as part of the conditional setup suggested earlier.
Example documentation:
## Ably Client Configuration The Ably Realtime client can be configured with the following options: - `authUrl`: URL for token authentication - `restHost`: Custom REST host (set via VITE_ABLY_HOST environment variable) - `realtimeHost`: Custom Realtime host (set via VITE_ABLY_HOST environment variable) - `clientId`: Unique identifier for the client For local development, additional options are available: - `port`: 8081 - `environment`: 'local' - `tls`: false
30-42: Summary: Local development configuration added with room for improvement.The changes in this file focus on configuring the Ably Realtime client for local development. While this is beneficial for testing, there are a few points to consider:
- The local development configuration (lines 30-32) should be applied conditionally to prevent unintended use in production.
- The commented-out code (lines 36-41) contains valuable information about alternative configurations, which should be documented properly rather than left as comments.
- The overall structure of the client initialization could be improved to handle different environments more elegantly.
These changes align with the PR objective of enhancing the SDK and demo app capabilities, particularly in terms of facilitating local development and testing of new features like message editing.
Consider refactoring the Ably client initialization to use a configuration object that can be easily switched between environments (local, staging, production). This would improve maintainability and make it easier to add or modify environment-specific settings in the future.
demo/src/components/MessageComponent/MessageComponent.tsx (1)
33-35: Consider optimizinguseCallbackdependenciesThe
iddependency in theuseCallbackhook forhandleMessageClickmight be unnecessary now that we're using themessageobject. Consider removing it to optimize performance:const handleMessageClick = useCallback(() => { onMessageClick?.(message); -}, [id, onMessageClick]); +}, [message, onMessageClick]);This change could prevent unnecessary re-renders when only the
idchanges but themessageobject remains the same.src/core/message-parser.ts (1)
Line range hint
1-99: Overall assessment of changesThe modifications to
parseMessagefunction align well with the PR objectives of enhancing message handling capabilities. The addition of new cases for empty string, undefined, and null actions improves the robustness of the function.However, there are opportunities to enhance code quality and maintainability:
- Consider adding logging or telemetry for unexpected action types to aid in debugging and monitoring.
- Ensure consistent use of braces in switch cases for better readability and maintainability.
- Prefer type guards or type narrowing over type assertions for improved type safety.
These changes will contribute to a more robust and maintainable codebase, aligning with the overall goals of the chat system enhancements.
Consider implementing a more extensible pattern for handling message actions, such as a strategy pattern or a map of action handlers. This could improve maintainability as new action types are added in the future.
🧰 Tools
🪛 GitHub Check: lint
[failure] 94-94:
Missing braces in case clause
[failure] 96-96:
Missing braces in case clause
[failure] 98-98:
Missing braces in case clause
[failure] 98-98:
Useundefinedinstead ofnullsrc/core/chat-api.ts (3)
68-69: Clarify property name to avoid confusionBoth
message.metadataandmetadataexist inUpdateMessageParams, which might be confusing. Consider renaming themetadatafield tooperationMetadatato distinguish it from the message's metadata.Apply this diff to improve clarity:
interface UpdateMessageParams { ... - /** Metadata of the update operation */ - metadata?: Record<string, string>; + /** Metadata of the update operation */ + operationMetadata?: Record<string, string>; }
149-152: Consistent parameter naming for serial identifiersIn the
deleteMessagemethod, the parameter is namedtimeserial, whereas inupdateMessage, it's namedserial. For consistency and clarity, consider using the same parameter name across methods.Option 1: Rename
serialtotimeserialinupdateMessage:- async updateMessage(roomId: string, serial: string, params: UpdateMessageParams): Promise<UpdateMessageResponse> { - const encodedSerial = encodeURIComponent(serial); + async updateMessage(roomId: string, timeserial: string, params: UpdateMessageParams): Promise<UpdateMessageResponse> { + const encodedSerial = encodeURIComponent(timeserial);Option 2: Rename
timeserialtoserialindeleteMessage:- async deleteMessage( - roomId: string, - timeserial: string, - params?: DeleteMessageParams, - ): Promise<DeleteMessageResponse> { - const encodedSerial = encodeURIComponent(timeserial); + async deleteMessage( + roomId: string, + serial: string, + params?: DeleteMessageParams, + ): Promise<DeleteMessageResponse> { + const encodedSerial = encodeURIComponent(serial);Choose the option that best fits your naming conventions.
149-152: Add error handling for theupdateMessagemethodCurrently, the
updateMessagemethod relies on_makeAuthorizedRequest, which throws an exception on failure. Consider adding error handling or logging withinupdateMessageto provide more context in case of failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- demo/src/components/MessageComponent/MessageComponent.tsx (2 hunks)
- demo/src/containers/Chat/Chat.tsx (5 hunks)
- demo/src/main.tsx (1 hunks)
- src/core/chat-api.ts (3 hunks)
- src/core/message-parser.ts (1 hunks)
- src/core/messages.ts (3 hunks)
- src/react/hooks/use-messages.ts (3 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
src/core/message-parser.ts
[failure] 94-94:
Missing braces in case clause
[failure] 96-96:
Missing braces in case clause
[failure] 98-98:
Missing braces in case clause
[failure] 98-98:
Useundefinedinstead ofnullsrc/core/messages.ts
[failure] 147-147:
An interface declaring no members is equivalent to its supertypesrc/react/hooks/use-messages.ts
[warning] 109-109:
React Hook useCallback received a function whose dependencies are unknown. Pass an inline function instead
🔇 Additional comments (13)
demo/src/main.tsx (2)
30-32: Verify local development configuration and consider conditional setup.The new parameters (
port: 8081,environment: 'local',tls: false) appear to be intended for local development. While this is useful for testing, it's important to ensure that these settings are not used in production.Consider implementing a conditional setup based on the environment:
const isLocalDev = process.env.NODE_ENV === 'development'; const realtimeClient = new Ably.Realtime({ authUrl: `/api/ably-token-request?clientId=${clientId}`, ...(isLocalDev ? { port: 8081, environment: 'local', tls: false, } : {}), clientId, });This approach ensures that the local development settings are only applied in the development environment.
42-42: LGTM: Improved code readability.The addition of an empty line improves code readability by clearly separating different sections of the configuration.
demo/src/components/MessageComponent/MessageComponent.tsx (3)
21-21: Improved prop type foronMessageClickThe change from
(id: string) => voidto(message: Message) => voidfor theonMessageClickprop is a good improvement. It allows passing the entire message object to the click handler, providing more context and flexibility for operations like message editing.
Line range hint
1-105: Summary of changesThe modifications to
MessageComponent.tsxsuccessfully implement the necessary changes to support message editing functionality. The updates to theonMessageClickprop type and the correspondinghandleMessageClickfunction improve the component's flexibility and align with the PR objectives. These changes lay the groundwork for implementing message editing in the demo app.A minor optimization suggestion was made regarding the
useCallbackdependencies. Overall, the changes are well-implemented and consistent with the project's goals.
34-34: UpdatedhandleMessageClickto pass message objectThe change in
handleMessageClickto passmessageinstead ofidis consistent with the updatedonMessageClickprop type. This ensures that the entire message object is available in the click handler, which is beneficial for operations like message editing.To ensure this change is consistently applied throughout the codebase, let's verify the usage of
onMessageClick:src/core/message-parser.ts (1)
94-99:⚠️ Potential issueImprove robustness and code style in switch statement
While handling additional cases improves robustness, there are some concerns:
- Empty case blocks might hide potential issues.
- Type assertions (
as Ably.MessageAction) could be unsafe.- Inconsistent use of braces in case clauses.
- Use of
nullas a type is discouraged in TypeScript.Consider the following improvements:
switch (message.action) { case ChatMessageActions.MessageCreate: break; case '': case undefined: case null: console.warn(`Received message with unexpected action: ${message.action}`); break; // ... other cases ... }Also, consider using a type guard or type narrowing instead of type assertions:
if (message.action === '' || message.action == null) { console.warn(`Received message with unexpected action: ${message.action}`); break; }To ensure this change doesn't introduce regressions, please run the following command:
✅ Verification successful
Review Comment Verified: No Other
message.actionSwitch Statements FoundThe shell script confirmed that the only switch statement handling
message.actionis insrc/core/message-parser.tsat line 90.No further action is required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other switch statements handling message.action rg -p "switch.*message.action" --type tsLength of output: 100
🧰 Tools
🪛 GitHub Check: lint
[failure] 94-94:
Missing braces in case clause
[failure] 96-96:
Missing braces in case clause
[failure] 98-98:
Missing braces in case clause
[failure] 98-98:
Useundefinedinstead ofnullsrc/react/hooks/use-messages.ts (3)
30-34: LGTM: Addition ofupdatemethod toUseMessagesResponseinterfaceThe new
updatemethod is well-documented and correctly typed. This addition aligns with the PR objectives to implement message editing functionality.
161-161: LGTM: Addition ofupdatemethod to hook's return objectThe
updatemethod is correctly added to the object returned by theuseMessageshook, making it accessible to consumers of the hook. This change is consistent with the interface modifications and aligns with the PR objectives.
Line range hint
1-173: Summary of changes inuse-messages.tsThe changes in this file successfully implement the message editing functionality as per the PR objectives. The
updatemethod has been added to theUseMessagesResponseinterface and theuseMessageshook, allowing consumers to update messages.Key points:
- The interface and documentation changes are well-implemented and clear.
- The
updatemethod is correctly added to the hook's return object.- A suggestion has been made to refactor the
updatemethod implementation to improve performance and address a static analysis warning.Overall, the changes are good, with room for a minor improvement in the
updatemethod implementation.src/core/chat-api.ts (1)
124-124: Ensure consistent encoding of serial identifiersYou've introduced
encodedSerialby encodingtimeserialusingencodeURIComponent. Ensure that all methods constructing URLs withtimeserialorserialencode these values consistently to handle special characters properly.Run the following script to verify the encoding in other methods:
Expected: All URLs should use
encodeURIComponentontimeserialorserialparameters.demo/src/containers/Chat/Chat.tsx (1)
221-221: EnsureonMessageClickprop aligns withMessageComponentexpectationsVerify that the
onMessageClickprop inMessageComponentaccepts a function that matches theupdateMessagesignature. This ensures that message updates are handled correctly when a message is clicked.src/core/messages.ts (2)
274-274: Method Signature Looks CorrectThe
updatemethod is appropriately added to theMessagesinterface with clear documentation and correct parameter definitions.
557-579: Implementation ofupdateMethod Appears CorrectThe
updatemethod correctly calls_chatApi.updateMessagewith the appropriate parameters and constructs a newDefaultMessageinstance with updated properties. The handling of optional parameters and default values is appropriate.
src/core/message-parser.ts
Outdated
| @@ -91,6 +91,12 @@ export function parseMessage(roomId: string | undefined, inboundMessage: Ably.In | |||
| case ChatMessageActions.MessageCreate: { | |||
| break; | |||
| } | |||
| case '' as Ably.MessageAction: | |||
There was a problem hiding this comment.
Is this to be removed?
demo/src/main.tsx
Outdated
| clientId, | ||
| }); | ||
|
|
||
| // const realtimeClient = new Ably.Realtime({ |
There was a problem hiding this comment.
I'm guessing this was just missed, but should probably be reverted? :)
src/react/hooks/use-messages.ts
Outdated
| @@ -101,6 +106,7 @@ export const useMessages = (params?: UseMessagesParams): UseMessagesResponse => | |||
| [room], | |||
| ); | |||
| const get = useCallback((options: QueryOptions) => room.messages.get(options), [room]); | |||
| const update = useCallback(room.messages.update.bind(room.messages), [room]) | |||
There was a problem hiding this comment.
Is there a need to use bind here, or can we keep it consistent with the other two callbacks?
There was a problem hiding this comment.
updated to be consistent
vladvelici
force-pushed
the
materialisation/update-message
branch
from
2aadce5 to
281e94a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/core/chat-api.ts (1)
54-77: LGTM! Consider using MessageDetailsMetadata type for consistency.The interfaces are well-structured and documented. For better type consistency across the codebase, consider using
MessageDetailsMetadatatype (used in DeleteMessageParams) instead ofRecord<string, string>for the metadata field in UpdateMessageParams./** Metadata of the update operation */ - metadata?: Record<string, string>; + metadata?: MessageDetailsMetadata;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- demo/src/components/MessageComponent/MessageComponent.tsx (2 hunks)
- demo/src/containers/Chat/Chat.tsx (5 hunks)
- demo/src/main.tsx (1 hunks)
- src/core/chat-api.ts (3 hunks)
- src/core/message-parser.ts (1 hunks)
- src/core/messages.ts (3 hunks)
- src/react/hooks/use-messages.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- demo/src/components/MessageComponent/MessageComponent.tsx
- demo/src/main.tsx
🧰 Additional context used
🪛 GitHub Check: lint
src/core/message-parser.ts
[failure] 94-94:
Missing braces in case clause
[failure] 96-96:
Missing braces in case clause
[failure] 98-98:
Missing braces in case clause
[failure] 98-98:
Useundefinedinstead ofnullsrc/core/messages.ts
[failure] 157-157:
An interface declaring no members is equivalent to its supertypesrc/react/hooks/use-messages.ts
[warning] 109-109:
React Hook useCallback received a function whose dependencies are unknown. Pass an inline function instead
🔇 Additional comments (13)
src/core/message-parser.ts (1)
94-99:⚠️ Potential issueImprove type safety and error handling for invalid actions.
The current implementation has several concerns:
- Type casting using
as unknown asbypasses TypeScript's type system, which could mask potential runtime issues.- Silently handling invalid actions (empty string, undefined, null) might hide bugs in the message handling logic.
- Code style issues with missing braces in case clauses.
Consider this safer implementation:
- case '' as Ably.MessageAction: - break; - case undefined as unknown as Ably.MessageAction: - break; - case null as unknown as Ably.MessageAction: - break; + case '' as Ably.MessageAction: { + throw new Ably.ErrorInfo('received incoming message with empty action', 50000, 500); + } + case undefined as Ably.MessageAction: { + throw new Ably.ErrorInfo('received incoming message with undefined action', 50000, 500); + }Remove the
nullcase as it's redundant withundefinedin TypeScript.Rationale:
- Explicit error handling helps identify issues in message processing
- Consistent with the error handling pattern used in other cases
- Follows TypeScript best practices by avoiding
unknowntype casting- Maintains code style consistency with braces
Let's verify if there are any other places in the codebase where message actions are set:
✅ Verification successful
Let me try a different search strategy to verify the message action handling across the codebase.
This should help us understand:
- How MessageAction is used across the codebase
- The specific action handling patterns in the message parser
- Where the MessageAction type is defined
The results will help verify if the suggested changes are consistent with the codebase's patterns and type safety requirements.
Based on the search results, I can now generate a final response as we have sufficient context about how message actions are handled in the codebase.
Improve error handling consistency for invalid message actions
The current implementation should be modified to align with the existing error handling patterns in the codebase. Looking at the search results, we can see that:
- The codebase already uses
ChatMessageActionsenum for valid actions (MessageCreate,MessageUpdate,MessageDelete)- Other cases in the same switch statement (like
MessageUpdateandMessageDelete) throw explicit errors withAbly.ErrorInfo- There's a message action to event mapping that expects valid actions
Suggested changes:
- case '' as Ably.MessageAction: - break; - case undefined as unknown as Ably.MessageAction: - break; - case null as unknown as Ably.MessageAction: - break; + case '' as Ably.MessageAction: { + throw new Ably.ErrorInfo('received incoming message with empty action', 50000, 500); + } + case undefined as Ably.MessageAction: { + throw new Ably.ErrorInfo('received incoming message with undefined action', 50000, 500); + }Rationale:
- Follows the existing error handling pattern seen in
MessageUpdateandMessageDeletecases- Maintains consistency with the codebase's error reporting using
Ably.ErrorInfo- Removes the redundant
nullcase as the codebase usesundefinedfor missing values- Adds braces for consistency with other case blocks
- Removes unsafe type casting with
unknown🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for places where message actions are set rg -l "action.*=.*MessageActions\."Length of output: 37
Script:
#!/bin/bash # Search for MessageAction type usage and assignments rg -A 2 "MessageAction" # Also search for action-related patterns in the message parser rg -A 2 "action.*:" src/core/message-parser.ts # Look for the Ably.MessageAction type definition fd -e ts -e d.ts -x grep -l "MessageAction"Length of output: 4530
🧰 Tools
🪛 GitHub Check: lint
[failure] 94-94:
Missing braces in case clause
[failure] 96-96:
Missing braces in case clause
[failure] 98-98:
Missing braces in case clause
[failure] 98-98:
Useundefinedinstead ofnullsrc/react/hooks/use-messages.ts (4)
30-34: LGTM! Well-documented interface addition.The new
updatemethod is properly typed and documented, maintaining consistency with the existing codebase style.
109-109: Implement update callback consistently with other methods.The current implementation differs from the pattern used in other callbacks (
send,deleteMessage,get). Let's maintain consistency and resolve the static analysis warning.🧰 Tools
🪛 GitHub Check: lint
[warning] 109-109:
React Hook useCallback received a function whose dependencies are unknown. Pass an inline function instead
161-161: LGTM! Proper integration of the update method.The
updatemethod is correctly integrated into the return object, maintaining the logical grouping of message operations.
109-109: Verify update method integration with demo app.Let's ensure the new update method is properly integrated with the demo app's Chat component.
✅ Verification successful
Let me gather more information about the update functionality in the Chat component to complete the verification.
I have enough information to generate the final response now.
Integration of update method is properly implemented
The verification shows that:
- The
updatemethod fromuseMessageshook is correctly bound and used in the Chat component- The update functionality is properly integrated through the
updateMessagecallback- The message update flow is complete:
- Receives message via
onMessageClickprop in MessageComponent- Handles updates via
updateMessagecallback in Chat component- Uses the bound
updatemethod fromuseMessageshook- Maintains message state via
setMessages🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the update method in the demo app # Test 1: Check usage of update method in Chat component echo "Checking usage in Chat component..." rg -A 5 "update.*=.*useMessages" demo/ # Test 2: Verify message update handler implementation echo "Checking message update handler..." rg -A 10 "onMessageClick" demo/Length of output: 3131
Script:
#!/bin/bash # Check the implementation of updateMessage in Chat.tsx echo "Checking updateMessage implementation..." rg -A 10 "updateMessage.*=" demo/src/containers/Chat/Chat.tsx # Check if useMessages hook is used in Chat.tsx echo "Checking useMessages usage..." rg -A 5 "useMessages" demo/src/containers/Chat/Chat.tsx # Check for any message update related state echo "Checking message update related state..." rg -A 5 "const.*message.*State" demo/src/containers/Chat/Chat.tsxLength of output: 1606
🧰 Tools
🪛 GitHub Check: lint
[warning] 109-109:
React Hook useCallback received a function whose dependencies are unknown. Pass an inline function insteaddemo/src/containers/Chat/Chat.tsx (5)
29-29: LGTM: Clean addition of update hook.The update method is correctly destructured from the useMessages hook.
38-47: LGTM: Efficient message deletion implementation.The deletion logic is well-implemented with:
- Efficient filtering
- State update optimization
- Clear comments explaining the logic
50-68: Skipping comment as there's an existing valid review.
220-220: LGTM: Clean integration of message update handler.The updateMessage callback is correctly wired up to the MessageComponent.
171-180:⚠️ Potential issueAdd message dependency to useCallback.
The
messageparameter is used in the console.log, but the callback isn't re-created when it changes. This could lead to stale message references.Apply this diff to fix the dependency array:
const updateMessage = useCallback((message: Message) => { const newText = prompt('Enter new text'); if (!newText) { return; } const updateOp = update(message, { text: newText, metadata: message.metadata, headers: message.headers, }); console.log("message ", message.timeserial, " updated. op=", updateOp); - }, [update]); + }, [update, message]);Likely invalid or redundant comment.
src/core/messages.ts (3)
149-157: Simplify redundant interface declaration.The interface
UpdateMessageParamsis identical to its supertypeSendMessageParams. While the documentation is clear and helpful, we can simplify this using a type alias.🧰 Tools
🪛 GitHub Check: lint
[failure] 157-157:
An interface declaring no members is equivalent to its supertype
159-172: LGTM! Well-structured metadata interface.The
UpdateMessageDetailsinterface provides a clean separation between message content and operation metadata, with clear documentation.
270-284: LGTM! Well-documented method signature.The
updatemethod declaration is well-designed with:
- Clear parameter types
- Optional details parameter
- Thorough documentation explaining the behavior and timing considerations
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/core/chat-api.ts (1)
Line range hint
82-89: Consider architectural improvements.A few suggestions to enhance the API's maintainability and documentation:
- Consider making the API version configurable via constructor instead of hardcoding
- Document rate limiting considerations for the update operation
- Update README to document the new message update capability
Would you like me to help with updating the documentation or implementing any of these suggestions?
src/core/messages.ts (1)
172-172: Consider using a more specific type for metadata.The
metadatafield inUpdateMessageDetailsuses a genericRecord<string, string>. Consider usingDetailsMetadatatype that's already imported and used elsewhere in the codebase for consistency.- metadata?: Record<string, string>; + metadata?: DetailsMetadata;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/core/chat-api.ts (4 hunks)
- src/core/message-parser.ts (1 hunks)
- src/core/messages.ts (3 hunks)
- src/react/hooks/use-messages.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/message-parser.ts
- src/react/hooks/use-messages.ts
🔇 Additional comments (3)
src/core/chat-api.ts (2)
54-77: LGTM! Well-structured interfaces for message updates.The interfaces are well-designed with clear separation of concerns and appropriate types.
93-93: LGTM! Consistent URL encoding implementation.The changes properly implement URL encoding for all path parameters across methods, which is a security best practice.
Also applies to: 125-126, 147-147, 158-158
src/core/messages.ts (1)
271-286: LGTM! Well-documented interface method.The method signature is clear and the documentation thoroughly explains the behavior, especially the note about promise resolution timing.
vladvelici
force-pushed
the
materialisation/update-message
branch
from
533be9b to
8588cb3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/core/chat-api.ts (1)
151-159: Consider adding rate limiting and concurrency control.The message update implementation could benefit from:
- Rate limiting to prevent abuse
- Optimistic concurrency control to handle concurrent updates (e.g., using ETags or version numbers)
These improvements would enhance the robustness of the message editing feature.
src/core/messages.ts (1)
163-173: Add JSDoc examples for UpdateMessageDetails interface.Consider adding examples in the JSDoc to illustrate typical usage of description and metadata fields.
export interface UpdateMessageDetails { /** * An optional description for the update operation. + * @example + * ```ts + * { + * description: "Fixed typo in message" + * } + * ``` */ description?: string; /** * Optional metadata for the update operation. + * @example + * ```ts + * { + * metadata: { + * editReason: "spelling_correction", + * editedFrom: "Hello wrold!", + * } + * } + * ``` */ metadata?: Record<string, string>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- demo/src/containers/Chat/Chat.tsx (4 hunks)
- src/core/chat-api.ts (4 hunks)
- src/core/messages.ts (3 hunks)
- src/react/hooks/use-messages.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/react/hooks/use-messages.ts
🔇 Additional comments (8)
src/core/chat-api.ts (2)
93-93: LGTM! URL encoding is now consistently applied.The changes properly encode both roomId and message serial parameters across all methods, improving security and robustness.
Also applies to: 125-126, 147-147, 162-162
151-159: Verify consistent error handling across API methods.The implementation looks good, but let's verify that error handling is consistent with other message operations.
✅ Verification successful
Let me gather more specific information about the message operations to ensure a complete verification.
This will help us see all message operation implementations and any additional error handling patterns that might exist outside the common _makeAuthorizedRequest wrapper.
Let me do one final check to specifically look at all message operations and their error handling pattern.
This will help us see the complete implementation of message operations and confirm they all use the same error handling pattern through _makeAuthorizedRequest.
Error handling is consistent across message operations
Based on the verification results, all message operations (getMessages, deleteMessage, sendMessage, and updateMessage) consistently use the centralized error handling through _makeAuthorizedRequest or _makeAuthorizedPaginatedRequest. These wrapper methods properly handle errors by:
- Logging errors with detailed context
- Throwing standardized Ably.ErrorInfo with proper error codes
- Using consistent parameter encoding
The new updateMessage method follows this same pattern, ensuring uniform error handling across the API.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check error handling consistency across message operations # Expected: All message operations should use _makeAuthorizedRequest and handle errors similarly # Search for message operation methods and their error handling ast-grep --pattern 'async $methodName($$$): Promise<$_> { $$$ return this._makeAuthorizedRequest<$_>($$$); }' # Check for any direct Ably API calls bypassing _makeAuthorizedRequest rg -A 2 'this\._realtime\.' --type ts --glob '!**/*test*' --glob '!**/*spec*'Length of output: 877
Script:
#!/bin/bash # Search for all message-related methods in chat-api.ts ast-grep --pattern 'async $methodName($params): Promise<$returnType> { $$$ }' src/core/chat-api.ts # Also check for any error handling patterns outside _makeAuthorizedRequest rg -A 3 'catch|throw|Error' --type ts src/core/chat-api.tsLength of output: 433
Script:
#!/bin/bash # Get the full context of message-related methods and _makeAuthorizedRequest rg -B 2 -A 10 "async (updateMessage|sendMessage|deleteMessage|getMessage|_makeAuthorizedRequest)" src/core/chat-api.tsLength of output: 2575
demo/src/containers/Chat/Chat.tsx (3)
29-29: LGTM: Update function correctly added to useMessages hook.The addition of the
updatefunction aligns with the PR objective of implementing message editing functionality.
33-66: LGTM: Message event handlers implemented correctly.The implementation follows React best practices:
- Immutable state updates
- Proper validation before state changes
- Error logging for unknown message types
221-221: LGTM: MessageComponent correctly configured for message updates.The
updateMessagecallback is properly passed to the MessageComponent'sonMessageClickprop.src/core/messages.ts (3)
271-286: LGTM! Well-documented method declaration.The method signature and JSDoc documentation clearly explain the purpose, parameters, and behavior of the update operation.
568-591: LGTM! Robust implementation with proper state handling.The implementation correctly:
- Preserves message history (createdAt, deletedAt, etc.)
- Handles optional fields with nullish coalescing
- Sets updatedAt and updatedBy fields
568-591: Verify event handling for message updates.Let's ensure that message update events are properly handled throughout the codebase.
✅ Verification successful
Based on the shell script results, I can now generate the final response as there's sufficient evidence to verify the message update event handling implementation.
Message update functionality is properly integrated across the codebase
The verification shows a complete implementation of message update functionality:
- Event handling is properly implemented:
MessageEvents.Updatedis defined and handled in the core messages system- Update events are properly mapped to actions via
ChatMessageActions.MessageUpdate- The demo app correctly handles updates by finding and updating the relevant message in the UI
- Update method integration is complete:
- Core implementation in
src/core/messages.ts- React hook wrapper in
src/react/hooks/use-messages.ts- Message parser handles update actions with proper validation
- Test coverage exists in
test/core/messages.test.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify message update event handling implementation # Test 1: Check for message update event handlers echo "Checking for message update event handlers..." rg -A 3 "MessageEvents\.Updated" --type ts # Test 2: Check for update message action handling echo "Checking for update message action handling..." rg -A 3 "MessageActions\.MessageUpdate" --type ts # Test 3: Check for update method usage echo "Checking for update method usage..." rg -A 3 "\.update\(.*message.*\)" --type tsLength of output: 3200
src/core/chat-api.ts
Outdated
| /** Metadata of the update operation */ | ||
| metadata?: Record<string, string>; |
There was a problem hiding this comment.
Consider using consistent metadata types.
The metadata field uses Record<string, string> while other places in the codebase use MessageDetailsMetadata. Consider using MessageDetailsMetadata for consistency.
- metadata?: Record<string, string>;
+ metadata?: MessageDetailsMetadata;Committable suggestion was skipped due to low confidence.
demo/src/containers/Chat/Chat.tsx
Outdated
| const updateMessage = useCallback( | ||
| (message: Message) => { | ||
| const newText = prompt('Enter new text'); | ||
| if (!newText) { | ||
| return; | ||
| } | ||
| const updateOp = update(message, { | ||
| text: newText, | ||
| metadata: message.metadata, | ||
| headers: message.headers, | ||
| }); | ||
| console.log('message ', message.timeserial, ' updated. op=', updateOp); | ||
| }, | ||
| [update], | ||
| ); |
There was a problem hiding this comment.
Handle the Promise returned by the update operation.
The update operation returns a Promise, but it's not being properly handled. This could lead to missed error cases and make it difficult to provide feedback to users when updates fail.
Apply this diff to properly handle the Promise:
const updateMessage = useCallback(
async (message: Message) => {
const newText = prompt('Enter new text');
if (!newText) {
return;
}
- const updateOp = update(message, {
- text: newText,
- metadata: message.metadata,
- headers: message.headers,
- });
- console.log('message ', message.timeserial, ' updated. op=', updateOp);
+ try {
+ const updatedMessage = await update(message, {
+ text: newText,
+ metadata: message.metadata,
+ headers: message.headers,
+ });
+ console.log('Message updated successfully:', updatedMessage);
+ } catch (error) {
+ console.error('Failed to update message:', error);
+ // TODO: Add user feedback for failed updates
+ }
},
[update],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateMessage = useCallback( | |
| (message: Message) => { | |
| const newText = prompt('Enter new text'); | |
| if (!newText) { | |
| return; | |
| } | |
| const updateOp = update(message, { | |
| text: newText, | |
| metadata: message.metadata, | |
| headers: message.headers, | |
| }); | |
| console.log('message ', message.timeserial, ' updated. op=', updateOp); | |
| }, | |
| [update], | |
| ); | |
| const updateMessage = useCallback( | |
| async (message: Message) => { | |
| const newText = prompt('Enter new text'); | |
| if (!newText) { | |
| return; | |
| } | |
| try { | |
| const updatedMessage = await update(message, { | |
| text: newText, | |
| metadata: message.metadata, | |
| headers: message.headers, | |
| }); | |
| console.log('Message updated successfully:', updatedMessage); | |
| } catch (error) { | |
| console.error('Failed to update message:', error); | |
| // TODO: Add user feedback for failed updates | |
| } | |
| }, | |
| [update], | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
test/react/hooks/use-messages.integration.test.tsx (1)
155-162: Add comments explaining update parametersThe update operation takes two parameter objects, but their purpose isn't immediately clear. Consider adding comments to explain the difference between message updates and update metadata.
void update(message, { + // New message content text: 'hello universe', metadata: { icon: 'universe' }, headers: { awesome: 'yes' }, }, { + // Update operation metadata description: "make it better", metadata: { something: 'else' }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- test/react/hooks/use-messages.integration.test.tsx (1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
test/react/hooks/use-messages.integration.test.tsx
[failure] 186-186:
Forbidden non-null assertion
🔇 Additional comments (1)
test/react/hooks/use-messages.integration.test.tsx (1)
127-127: Verify API change fromisDeleted()method toisDeletedpropertyThe change from
isDeleted()toisDeletedsuggests an API change in the Message interface. Let's verify this change is consistent across the codebase.
| // expect a message to be received by the second room | ||
| await waitForMessages(updatesRoomTwo, 1); | ||
| expect(updatesRoomTwo.length).toBe(1); | ||
| const update = updatesRoomTwo[0]!; |
There was a problem hiding this comment.
Replace non-null assertion with runtime check
The non-null assertion (!) on updatesRoomTwo[0]! should be replaced with a runtime check for better type safety.
-const update = updatesRoomTwo[0]!;
+const update = updatesRoomTwo[0];
+if (!update) {
+ throw new Error('Expected update message to be defined');
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const update = updatesRoomTwo[0]!; | |
| const update = updatesRoomTwo[0]; | |
| if (!update) { | |
| throw new Error('Expected update message to be defined'); | |
| } |
🧰 Tools
🪛 GitHub Check: lint
[failure] 186-186:
Forbidden non-null assertion
| it('should update messages correctly', async () => { | ||
| // create new clients | ||
| const chatClientOne = newChatClient() as unknown as ChatClient; | ||
| const chatClientTwo = newChatClient() as unknown as ChatClient; | ||
|
|
||
| // create a second room and attach it, so we can listen for deletions | ||
| const roomId = randomRoomId(); | ||
| const roomTwo = chatClientTwo.rooms.get(roomId, RoomOptionsDefaults); | ||
| await roomTwo.attach(); | ||
|
|
||
| // start listening for deletions | ||
| const updatesRoomTwo: Message[] = []; | ||
| roomTwo.messages.subscribe((message) => { | ||
| if (message.type === MessageEvents.Updated) { | ||
| updatesRoomTwo.push(message.message); | ||
| } | ||
| }); | ||
|
|
||
| const TestComponent = () => { | ||
| const { send, update, roomStatus } = useMessages(); | ||
|
|
||
| useEffect(() => { | ||
| if (roomStatus === RoomLifecycle.Attached) { | ||
| void send({ text: 'hello world' }).then((message) => { | ||
| void update(message, { | ||
| text: 'hello universe', | ||
| metadata: { icon: 'universe' }, | ||
| headers: { awesome: 'yes' }, | ||
| }, { | ||
| description: "make it better", | ||
| metadata: { something: 'else' }, | ||
| }); | ||
| }); | ||
| } | ||
| }, [roomStatus]); | ||
|
|
||
| return null; | ||
| }; | ||
|
|
||
| const TestProvider = () => ( | ||
| <ChatClientProvider client={chatClientOne}> | ||
| <ChatRoomProvider | ||
| id={roomId} | ||
| options={RoomOptionsDefaults} | ||
| > | ||
| <TestComponent /> | ||
| </ChatRoomProvider> | ||
| </ChatClientProvider> | ||
| ); | ||
|
|
||
| render(<TestProvider />); | ||
|
|
||
| // expect a message to be received by the second room | ||
| await waitForMessages(updatesRoomTwo, 1); | ||
| expect(updatesRoomTwo.length).toBe(1); | ||
| const update = updatesRoomTwo[0]!; | ||
| expect(update.isUpdated).toBe(true); | ||
| expect(update.updatedBy).toBe(chatClientOne.clientId); | ||
| expect(update.text).toBe('hello universe'); | ||
| expect(update.metadata).toEqual({ icon: 'universe' }); | ||
| expect(update.updateDetail?.description).toBe('make it better'); | ||
| expect(update.updateDetail?.metadata).toEqual({ something: 'else' }); | ||
| }, 10000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test structure looks good, but could use some improvements
The test case provides good coverage of the message update functionality. However, there are a few suggestions for improvement:
- The test follows the same setup pattern as other tests. Consider extracting the common setup code into a helper function to reduce duplication.
- The test verifies text, metadata, and update details, but misses verification of the
headersproperty that was set in the update.
Add the missing assertion:
expect(update.text).toBe('hello universe');
expect(update.metadata).toEqual({ icon: 'universe' });
+expect(update.headers).toEqual({ awesome: 'yes' });
expect(update.updateDetail?.description).toBe('make it better');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should update messages correctly', async () => { | |
| // create new clients | |
| const chatClientOne = newChatClient() as unknown as ChatClient; | |
| const chatClientTwo = newChatClient() as unknown as ChatClient; | |
| // create a second room and attach it, so we can listen for deletions | |
| const roomId = randomRoomId(); | |
| const roomTwo = chatClientTwo.rooms.get(roomId, RoomOptionsDefaults); | |
| await roomTwo.attach(); | |
| // start listening for deletions | |
| const updatesRoomTwo: Message[] = []; | |
| roomTwo.messages.subscribe((message) => { | |
| if (message.type === MessageEvents.Updated) { | |
| updatesRoomTwo.push(message.message); | |
| } | |
| }); | |
| const TestComponent = () => { | |
| const { send, update, roomStatus } = useMessages(); | |
| useEffect(() => { | |
| if (roomStatus === RoomLifecycle.Attached) { | |
| void send({ text: 'hello world' }).then((message) => { | |
| void update(message, { | |
| text: 'hello universe', | |
| metadata: { icon: 'universe' }, | |
| headers: { awesome: 'yes' }, | |
| }, { | |
| description: "make it better", | |
| metadata: { something: 'else' }, | |
| }); | |
| }); | |
| } | |
| }, [roomStatus]); | |
| return null; | |
| }; | |
| const TestProvider = () => ( | |
| <ChatClientProvider client={chatClientOne}> | |
| <ChatRoomProvider | |
| id={roomId} | |
| options={RoomOptionsDefaults} | |
| > | |
| <TestComponent /> | |
| </ChatRoomProvider> | |
| </ChatClientProvider> | |
| ); | |
| render(<TestProvider />); | |
| // expect a message to be received by the second room | |
| await waitForMessages(updatesRoomTwo, 1); | |
| expect(updatesRoomTwo.length).toBe(1); | |
| const update = updatesRoomTwo[0]!; | |
| expect(update.isUpdated).toBe(true); | |
| expect(update.updatedBy).toBe(chatClientOne.clientId); | |
| expect(update.text).toBe('hello universe'); | |
| expect(update.metadata).toEqual({ icon: 'universe' }); | |
| expect(update.updateDetail?.description).toBe('make it better'); | |
| expect(update.updateDetail?.metadata).toEqual({ something: 'else' }); | |
| }, 10000); | |
| it('should update messages correctly', async () => { | |
| // create new clients | |
| const chatClientOne = newChatClient() as unknown as ChatClient; | |
| const chatClientTwo = newChatClient() as unknown as ChatClient; | |
| // create a second room and attach it, so we can listen for deletions | |
| const roomId = randomRoomId(); | |
| const roomTwo = chatClientTwo.rooms.get(roomId, RoomOptionsDefaults); | |
| await roomTwo.attach(); | |
| // start listening for deletions | |
| const updatesRoomTwo: Message[] = []; | |
| roomTwo.messages.subscribe((message) => { | |
| if (message.type === MessageEvents.Updated) { | |
| updatesRoomTwo.push(message.message); | |
| } | |
| }); | |
| const TestComponent = () => { | |
| const { send, update, roomStatus } = useMessages(); | |
| useEffect(() => { | |
| if (roomStatus === RoomLifecycle.Attached) { | |
| void send({ text: 'hello world' }).then((message) => { | |
| void update(message, { | |
| text: 'hello universe', | |
| metadata: { icon: 'universe' }, | |
| headers: { awesome: 'yes' }, | |
| }, { | |
| description: "make it better", | |
| metadata: { something: 'else' }, | |
| }); | |
| }); | |
| } | |
| }, [roomStatus]); | |
| return null; | |
| }; | |
| const TestProvider = () => ( | |
| <ChatClientProvider client={chatClientOne}> | |
| <ChatRoomProvider | |
| id={roomId} | |
| options={RoomOptionsDefaults} | |
| > | |
| <TestComponent /> | |
| </ChatRoomProvider> | |
| </ChatClientProvider> | |
| ); | |
| render(<TestProvider />); | |
| // expect a message to be received by the second room | |
| await waitForMessages(updatesRoomTwo, 1); | |
| expect(updatesRoomTwo.length).toBe(1); | |
| const update = updatesRoomTwo[0]!; | |
| expect(update.isUpdated).toBe(true); | |
| expect(update.updatedBy).toBe(chatClientOne.clientId); | |
| expect(update.text).toBe('hello universe'); | |
| expect(update.metadata).toEqual({ icon: 'universe' }); | |
| expect(update.headers).toEqual({ awesome: 'yes' }); | |
| expect(update.updateDetail?.description).toBe('make it better'); | |
| expect(update.updateDetail?.metadata).toEqual({ something: 'else' }); | |
| }, 10000); |
🧰 Tools
🪛 GitHub Check: lint
[failure] 186-186:
Forbidden non-null assertion
splindsay-92
force-pushed
the
materialisation/implement-message-deleting
branch
from
0cede1c to
f56bdd4
Compare
vladvelici
force-pushed
the
materialisation/update-message
branch
from
121075e to
0b47db3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
demo/src/containers/Chat/Chat.tsx (1)
221-221: Consider adding visual indicators for editable messages.While the click-to-edit functionality is implemented correctly, users might not intuitively know they can click messages to edit them. Consider adding:
- A hover effect on editable messages
- An edit icon or tooltip
- Documentation or first-time user guidance
Example implementation:
<MessageComponent id={msg.timeserial} key={msg.timeserial} self={msg.clientId === clientId} message={msg} + className={msg.clientId === clientId ? 'cursor-pointer hover:bg-gray-100' : ''} + title={msg.clientId === clientId ? 'Click to edit message' : ''} onMessageDelete={(msg) => {src/core/messages.ts (1)
267-281: Consider enhancing method documentation with error scenarios.The documentation is comprehensive for the happy path, but it would be helpful to document potential error scenarios (e.g., message not found, permission denied) that consumers should handle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- demo/src/components/MessageComponent/MessageComponent.tsx (2 hunks)
- demo/src/containers/Chat/Chat.tsx (4 hunks)
- demo/src/main.tsx (1 hunks)
- src/core/chat-api.ts (4 hunks)
- src/core/messages.ts (3 hunks)
- src/react/hooks/use-messages.ts (4 hunks)
- test/react/hooks/use-messages.integration.test.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- demo/src/components/MessageComponent/MessageComponent.tsx
- demo/src/main.tsx
- src/react/hooks/use-messages.ts
🧰 Additional context used
🪛 GitHub Check: lint
test/react/hooks/use-messages.integration.test.tsx
[failure] 185-185:
Forbidden non-null assertion
🔇 Additional comments (8)
src/core/chat-api.ts (3)
93-93: LGTM: Proper URL encoding of roomId parameter.The implementation correctly encodes the roomId parameter to handle special characters safely, consistent with best practices.
Also applies to: 147-147
125-128: LGTM: Complete URL parameter encoding implementation.The changes properly encode both the timeserial and roomId parameters, improving security and robustness of the API calls.
54-70:⚠️ Potential issueConsider using consistent metadata types across interfaces.
The
metadatafield inUpdateMessageParamsusesRecord<string, string>while other interfaces in the codebase useMessageDetailsMetadata. For consistency and type safety, consider usingMessageDetailsMetadata./** Metadata of the update operation */ - metadata?: Record<string, string>; + metadata?: MessageDetailsMetadata;Likely invalid or redundant comment.
demo/src/containers/Chat/Chat.tsx (2)
33-66: LGTM! Well-structured message event handling implementation.The message event handling logic is well-implemented with proper state management for all events (Created, Updated, Deleted) and includes appropriate error logging.
167-181: Previous review comments are still applicable.The implementation still needs to address the following previously identified issues:
- Replace prompt with a custom input component
- Add proper error handling
- Handle the Promise returned by the update operation
test/react/hooks/use-messages.integration.test.tsx (1)
148-178: LGTM! Comprehensive test coverage for message updatesThe test component implementation thoroughly verifies the message update functionality, including:
- Message content updates (text)
- Metadata modifications
- Header changes
- Update details (description and metadata)
src/core/messages.ts (2)
159-169: LGTM! Well-structured interface for update operation details.The
UpdateMessageDetailsinterface is well-documented and provides a clear structure for optional metadata about message updates.
548-571: Consider potential race conditions in update operation.The implementation correctly preserves message metadata, but there's a potential race condition if multiple updates occur simultaneously. Consider adding optimistic concurrency control or version checking to prevent lost updates.
Let's check if there are any concurrent update scenarios in the codebase:
| it('should update messages correctly', async () => { | ||
| // create new clients | ||
| const chatClientOne = newChatClient() as unknown as ChatClient; | ||
| const chatClientTwo = newChatClient() as unknown as ChatClient; | ||
|
|
||
| // create a second room and attach it, so we can listen for deletions | ||
| const roomId = randomRoomId(); | ||
| const roomTwo = chatClientTwo.rooms.get(roomId, RoomOptionsDefaults); | ||
| await roomTwo.attach(); | ||
|
|
||
| // start listening for deletions | ||
| const updatesRoomTwo: Message[] = []; | ||
| roomTwo.messages.subscribe((message) => { | ||
| if (message.type === MessageEvents.Updated) { | ||
| updatesRoomTwo.push(message.message); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common test setup into helper functions
The test setup code is duplicated across multiple test cases. Consider extracting the common setup into helper functions:
async function setupTestClients() {
const chatClientOne = newChatClient() as unknown as ChatClient;
const chatClientTwo = newChatClient() as unknown as ChatClient;
return { chatClientOne, chatClientTwo };
}
async function setupTestRoom(client: ChatClient) {
const roomId = randomRoomId();
const room = client.rooms.get(roomId, RoomOptionsDefaults);
await room.attach();
return { roomId, room };
}| // expect a message to be received by the second room | ||
| await waitForMessages(updatesRoomTwo, 1); | ||
| expect(updatesRoomTwo.length).toBe(1); | ||
| const update = updatesRoomTwo[0]!; | ||
| expect(update.isUpdated).toBe(true); | ||
| expect(update.updatedBy).toBe(chatClientOne.clientId); | ||
| expect(update.text).toBe('hello universe'); | ||
| expect(update.metadata).toEqual({ icon: 'universe' }); | ||
| expect(update.updateDetail?.description).toBe('make it better'); | ||
| expect(update.updateDetail?.metadata).toEqual({ something: 'else' }); | ||
| }, 10000); |
There was a problem hiding this comment.
Add missing headers assertion and improve type safety
- Replace the non-null assertion with a runtime check:
-const update = updatesRoomTwo[0]!;
+const update = updatesRoomTwo[0];
+if (!update) {
+ throw new Error('Expected update message to be defined');
+}- Add the missing headers assertion:
expect(update.metadata).toEqual({ icon: 'universe' });
+expect(update.headers).toEqual({ awesome: 'yes' });
expect(update.updateDetail?.description).toBe('make it better');Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: lint
[failure] 185-185:
Forbidden non-null assertion
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (3)
demo/src/index.css (1)
65-65: Remove empty line within CSS rule.There's an unnecessary empty line within the CSS rule block.
.chat-message:hover .buttons { display: block; - }src/core/chat-api.ts (1)
54-70: Consider using consistent metadata types across interfaces.The
metadatafield inUpdateMessageParamsusesMessageMetadatawhile the operation metadata usesMessageDetailsMetadata. Consider documenting the reason for this difference or using consistent types if they serve the same purpose.demo/src/containers/Chat/Chat.tsx (1)
183-184: Provide user feedback when message update failsCurrently, the error during message update is logged to the console. Consider displaying an error message to the user to inform them that the update failed.
You can update the
catchblock as follows:.catch((error: unknown) => { console.warn('failed to update message', error); + // Display an error message to the user + alert('Failed to update the message. Please try again.'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- demo/src/components/MessageComponent/MessageComponent.tsx (4 hunks)
- demo/src/containers/Chat/Chat.tsx (4 hunks)
- demo/src/index.css (1 hunks)
- src/core/chat-api.ts (4 hunks)
🔇 Additional comments (3)
demo/src/components/MessageComponent/MessageComponent.tsx (1)
43-48: Verify integration with Chat SDKThe component implementation looks good but let's verify it properly integrates with the Chat SDK's message update capability.
✅ Verification successful
Let me try a different approach to verify the Chat SDK integration by searching without file type restrictions and checking for message update related code.
Message update integration is properly implemented
The verification shows a complete integration chain:
MessageComponentcorrectly implements theonMessageUpdateprop and handler- Parent
Chatcomponent connects it toonUpdateMessage- Backend integration exists through
chat-api.tsandmessages.tswith properupdateMessageimplementation🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Chat SDK message update integration echo "Checking Chat SDK integration..." rg -l "updateMessage|onMessageUpdate" --type typescript # Verify message update handler implementation in parent components echo "Checking parent component implementations..." ast-grep --pattern 'onMessageUpdate={$_}'Length of output: 310
Script:
#!/bin/bash # Search for message update related code without file type restrictions echo "Checking message update integration..." rg "updateMessage|onMessageUpdate" -A 3 # Search for Chat SDK imports and usage echo "Checking Chat SDK imports..." rg "import.*[Cc]hat.*SDK" -A 1 # Look for files containing MessageComponent usage echo "Checking MessageComponent usage..." rg "MessageComponent" -A 3Length of output: 3645
src/core/chat-api.ts (1)
93-93: LGTM! Consistent URL encoding implementation.The changes properly implement URL encoding for all path parameters across methods, which improves security by preventing URL injection vulnerabilities.
Also applies to: 125-126, 147-147, 162-162
demo/src/containers/Chat/Chat.tsx (1)
25-35: LGTM!The
handleUpdatedMessagefunction correctly updates the message state when a message is edited.
| .chat-message .buttons { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using opacity or visibility for smoother transitions.
The current implementation using display: none/block might cause layout shifts when the buttons appear. Consider using opacity or visibility with opacity for a smoother transition:
.chat-message .buttons {
- display: none;
+ visibility: hidden;
+ opacity: 0;
+ transition: opacity 0.2s ease-in-out;
}
.chat-message:hover .buttons {
- display: block;
+ visibility: visible;
+ opacity: 1;
}Additionally, consider adding touch device support since hover states don't work well on mobile:
@media (hover: none) {
.chat-message .buttons {
visibility: visible;
opacity: 1;
}
}Also applies to: 63-66
There was a problem hiding this comment.
For now we want the layout to shift
There was a problem hiding this comment.
@vladvelici Understood, thank you for the clarification.
(_/)
(•_•) Feeling a bit sad, but I respect your decision.
✏️ Learnings added
Learnt from: vladvelici
PR: ably/ably-chat-js#378
File: demo/src/index.css:59-61
Timestamp: 2024-11-07T13:20:31.746Z
Learning: In this project, layout shifts when buttons appear or disappear are acceptable and sometimes desired.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| function shortDate(date: Date): string { | ||
| if (Date.now() - date.getTime() < 1000 * 60 * 60 * 24) { | ||
| // last 24h show the time | ||
| displayCreatedAt = twoDigits(message.createdAt.getHours()) + ':' + twoDigits(message.createdAt.getMinutes()); | ||
| return twoDigits(date.getHours()) + ':' + twoDigits(date.getMinutes()); | ||
| } else { | ||
| // older, show full date | ||
| displayCreatedAt = | ||
| message.createdAt.getDate() + | ||
| return date.getDate() + | ||
| '/' + | ||
| message.createdAt.getMonth() + | ||
| date.getMonth() + | ||
| '/' + | ||
| message.createdAt.getFullYear() + | ||
| date.getFullYear() + | ||
| ' ' + | ||
| twoDigits(message.createdAt.getHours()) + | ||
| twoDigits(date.getHours()) + | ||
| ':' + | ||
| twoDigits(message.createdAt.getMinutes()); | ||
| twoDigits(date.getMinutes()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix date formatting issues in shortDate function
There are several issues with the date formatting implementation:
date.getMonth()returns 0-based month (0-11), leading to incorrect month display- The function doesn't handle different locales gracefully
Apply this fix:
function shortDate(date: Date): string {
if (Date.now() - date.getTime() < 1000 * 60 * 60 * 24) {
return twoDigits(date.getHours()) + ':' + twoDigits(date.getMinutes());
} else {
return date.getDate() +
'/' +
- date.getMonth() +
+ (date.getMonth() + 1) +
'/' +
date.getFullYear() +
' ' +
twoDigits(date.getHours()) +
':' +
twoDigits(date.getMinutes());
}
}Consider using Intl.DateTimeFormat for locale-aware date formatting:
function shortDate(date: Date): string {
if (Date.now() - date.getTime() < 1000 * 60 * 60 * 24) {
return new Intl.DateTimeFormat('default', {
hour: '2-digit',
minute: '2-digit'
}).format(date);
}
return new Intl.DateTimeFormat('default', {
day: '2-digit',
month: '2-digit',
year: 'numeric',
hour: '2-digit',
minute: '2-digit'
}).format(date);
}There was a problem hiding this comment.
@vladvelici Great! Thanks for fixing the date formatting issues.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| {message.isUpdated && (<> · Edited <span className="sent-at-time"> | ||
| <span className="short">{shortDate(message.updatedAt!)}</span> | ||
| <span className="long">{message.createdAt.toLocaleString()}</span> | ||
| </span> | ||
| </>)} |
There was a problem hiding this comment.
Add null check for updatedAt timestamp
The code assumes updatedAt is always present when isUpdated is true, but TypeScript indicates it's optional.
Add proper null checking:
- {message.isUpdated && (<> · Edited <span className="sent-at-time">
- <span className="short">{shortDate(message.updatedAt!)}</span>
- <span className="long">{message.createdAt.toLocaleString()}</span>
- </span>
- </>)}
+ {message.isUpdated && message.updatedAt && (
+ <> · Edited <span className="sent-at-time">
+ <span className="short">{shortDate(message.updatedAt)}</span>
+ <span className="long">{message.updatedAt.toLocaleString()}</span>
+ </span>
+ </>
+ )}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {message.isUpdated && (<> · Edited <span className="sent-at-time"> | |
| <span className="short">{shortDate(message.updatedAt!)}</span> | |
| <span className="long">{message.createdAt.toLocaleString()}</span> | |
| </span> | |
| </>)} | |
| {message.isUpdated && message.updatedAt && ( | |
| <> · Edited <span className="sent-at-time"> | |
| <span className="short">{shortDate(message.updatedAt)}</span> | |
| <span className="long">{message.updatedAt.toLocaleString()}</span> | |
| </span> | |
| </> | |
| )} |
There was a problem hiding this comment.
@vladvelici, thanks for addressing the issue!
((\
(•‿•)
o_(")(")
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| <div className="buttons" | ||
| ><FaPencil | ||
| className="cursor-pointer text-gray-100 m-1 hover:text-gray-500 inline-block" | ||
| onClick={handleMessageUpdate} | ||
| ></FaPencil><FaTrash | ||
| className="cursor-pointer text-red-500 m-1 hover:text-red-700 inline-block" | ||
| onClick={handleMessageDelete} | ||
| /></div> |
There was a problem hiding this comment.
Improve accessibility for edit/delete buttons
The icon buttons lack proper accessibility attributes and semantic HTML.
Add proper button semantics and aria labels:
- <div className="buttons"
- ><FaPencil
- className="cursor-pointer text-gray-100 m-1 hover:text-gray-500 inline-block"
- onClick={handleMessageUpdate}
- ></FaPencil><FaTrash
- className="cursor-pointer text-red-500 m-1 hover:text-red-700 inline-block"
- onClick={handleMessageDelete}
- /></div>
+ <div className="buttons" role="group" aria-label="Message actions">
+ <button
+ type="button"
+ className="p-1 hover:bg-gray-100 rounded"
+ onClick={handleMessageUpdate}
+ aria-label="Edit message"
+ >
+ <FaPencil className="text-gray-100 hover:text-gray-500" />
+ </button>
+ <button
+ type="button"
+ className="p-1 hover:bg-gray-100 rounded"
+ onClick={handleMessageDelete}
+ aria-label="Delete message"
+ >
+ <FaTrash className="text-red-500 hover:text-red-700" />
+ </button>
+ </div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="buttons" | |
| ><FaPencil | |
| className="cursor-pointer text-gray-100 m-1 hover:text-gray-500 inline-block" | |
| onClick={handleMessageUpdate} | |
| ></FaPencil><FaTrash | |
| className="cursor-pointer text-red-500 m-1 hover:text-red-700 inline-block" | |
| onClick={handleMessageDelete} | |
| /></div> | |
| <div className="buttons" role="group" aria-label="Message actions"> | |
| <button | |
| type="button" | |
| className="p-1 hover:bg-gray-100 rounded" | |
| onClick={handleMessageUpdate} | |
| aria-label="Edit message" | |
| > | |
| <FaPencil className="text-gray-100 hover:text-gray-500" /> | |
| </button> | |
| <button | |
| type="button" | |
| className="p-1 hover:bg-gray-100 rounded" | |
| onClick={handleMessageDelete} | |
| aria-label="Delete message" | |
| > | |
| <FaTrash className="text-red-500 hover:text-red-700" /> | |
| </button> | |
| </div> |
| const handleMessageUpdate = useCallback((e : React.UIEvent) => { | ||
| e.stopPropagation(); onMessageUpdate?.(message); | ||
| }, [ message ]); | ||
|
|
||
| const handleDelete = useCallback(() => { | ||
| // Add your delete handling logic here | ||
| onMessageDelete?.(message); | ||
| }, [message, onMessageDelete]); | ||
| const handleMessageDelete = useCallback((e : React.UIEvent) => { | ||
| e.stopPropagation(); onMessageDelete?.(message); | ||
| }, [ message ]); |
There was a problem hiding this comment.
Improve event handlers implementation
The event handlers need improvements for better error handling and accessibility:
- Missing preventDefault() for UI events
- No feedback when callbacks are not provided
- Missing error boundaries
Apply these improvements:
- const handleMessageUpdate = useCallback((e : React.UIEvent) => {
- e.stopPropagation(); onMessageUpdate?.(message);
- }, [ message ]);
+ const handleMessageUpdate = useCallback((e: React.MouseEvent) => {
+ e.preventDefault();
+ e.stopPropagation();
+ if (onMessageUpdate) {
+ onMessageUpdate(message);
+ }
+ }, [message, onMessageUpdate]);
- const handleMessageDelete = useCallback((e : React.UIEvent) => {
- e.stopPropagation(); onMessageDelete?.(message);
- }, [ message ]);
+ const handleMessageDelete = useCallback((e: React.MouseEvent) => {
+ e.preventDefault();
+ e.stopPropagation();
+ if (onMessageDelete) {
+ onMessageDelete(message);
+ }
+ }, [message, onMessageDelete]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleMessageUpdate = useCallback((e : React.UIEvent) => { | |
| e.stopPropagation(); onMessageUpdate?.(message); | |
| }, [ message ]); | |
| const handleDelete = useCallback(() => { | |
| // Add your delete handling logic here | |
| onMessageDelete?.(message); | |
| }, [message, onMessageDelete]); | |
| const handleMessageDelete = useCallback((e : React.UIEvent) => { | |
| e.stopPropagation(); onMessageDelete?.(message); | |
| }, [ message ]); | |
| const handleMessageUpdate = useCallback((e: React.MouseEvent) => { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| if (onMessageUpdate) { | |
| onMessageUpdate(message); | |
| } | |
| }, [message, onMessageUpdate]); | |
| const handleMessageDelete = useCallback((e: React.MouseEvent) => { | |
| e.preventDefault(); | |
| e.stopPropagation(); | |
| if (onMessageDelete) { | |
| onMessageDelete(message); | |
| } | |
| }, [message, onMessageDelete]); |
| async updateMessage(roomId: string, serial: string, params: UpdateMessageParams): Promise<UpdateMessageResponse> { | ||
| const encodedSerial = encodeURIComponent(serial); | ||
| roomId = encodeURIComponent(roomId); | ||
| return this._makeAuthorizedRequest<UpdateMessageResponse>( | ||
| `/chat/v2/rooms/${roomId}/messages/${encodedSerial}`, | ||
| 'PUT', | ||
| params, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error documentation.
While the implementation is solid, consider these improvements:
- Add input validation for required fields
- Document possible error cases in JSDoc
Apply this improvement:
+ /**
+ * Updates a message in a chat room.
+ * @param roomId - The ID of the room containing the message
+ * @param serial - The timeserial of the message to update
+ * @param params - The update parameters
+ * @throws {Error} If message text is empty or contains only whitespace
+ * @throws {Ably.ErrorInfo} If the request fails
+ * @returns {Promise<UpdateMessageResponse>} The update response containing the timestamp
+ */
async updateMessage(roomId: string, serial: string, params: UpdateMessageParams): Promise<UpdateMessageResponse> {
+ if (!params?.message?.text?.trim()) {
+ throw new Error('Message text cannot be empty');
+ }
const encodedSerial = encodeURIComponent(serial);
roomId = encodeURIComponent(roomId);
return this._makeAuthorizedRequest<UpdateMessageResponse>(
`/chat/v2/rooms/${roomId}/messages/${encodedSerial}`,
'PUT',
params
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async updateMessage(roomId: string, serial: string, params: UpdateMessageParams): Promise<UpdateMessageResponse> { | |
| const encodedSerial = encodeURIComponent(serial); | |
| roomId = encodeURIComponent(roomId); | |
| return this._makeAuthorizedRequest<UpdateMessageResponse>( | |
| `/chat/v2/rooms/${roomId}/messages/${encodedSerial}`, | |
| 'PUT', | |
| params, | |
| ); | |
| } | |
| /** | |
| * Updates a message in a chat room. | |
| * @param roomId - The ID of the room containing the message | |
| * @param serial - The timeserial of the message to update | |
| * @param params - The update parameters | |
| * @throws {Error} If message text is empty or contains only whitespace | |
| * @throws {Ably.ErrorInfo} If the request fails | |
| * @returns {Promise<UpdateMessageResponse>} The update response containing the timestamp | |
| */ | |
| async updateMessage(roomId: string, serial: string, params: UpdateMessageParams): Promise<UpdateMessageResponse> { | |
| if (!params?.message?.text?.trim()) { | |
| throw new Error('Message text cannot be empty'); | |
| } | |
| const encodedSerial = encodeURIComponent(serial); | |
| roomId = encodeURIComponent(roomId); | |
| return this._makeAuthorizedRequest<UpdateMessageResponse>( | |
| `/chat/v2/rooms/${roomId}/messages/${encodedSerial}`, | |
| 'PUT', | |
| params | |
| ); | |
| } |
demo/src/containers/Chat/Chat.tsx
Outdated
| const onUpdateMessage = (message: Message) => { | ||
| const newText = prompt('Enter new text'); | ||
| if (!newText) { | ||
| return; | ||
| } | ||
| update(message, { | ||
| text: newText, | ||
| metadata: message.metadata, | ||
| headers: message.headers, | ||
| }).then((updatedMessage: Message) => { | ||
| handleUpdatedMessage(updatedMessage); | ||
| }).catch((error: unknown) => { | ||
| console.warn("failed to update message", error); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap onUpdateMessage with useCallback to optimize rendering
Since onUpdateMessage is passed as a prop to MessageComponent, wrapping it with useCallback will prevent unnecessary re-renders of child components.
Apply this diff to wrap the function with useCallback:
- const onUpdateMessage = (message: Message) => {
+ const onUpdateMessage = useCallback((message: Message) => {
const newText = prompt('Enter new text');
if (!newText) {
return;
}
update(message, {
text: newText,
metadata: message.metadata,
headers: message.headers,
})
.then((updatedMessage: Message) => {
handleUpdatedMessage(updatedMessage);
})
.catch((error: unknown) => {
console.warn('failed to update message', error);
});
- };
+ }, [update]);Committable suggestion was skipped due to low confidence.
splindsay-92
force-pushed
the
materialisation/implement-message-deleting
branch
2 times, most recently
from
a35c6b1 to
e8eeb74
Compare
vladvelici
force-pushed
the
materialisation/update-message
branch
from
b37291f to
87ab733
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/core/chat-api.ts (1)
58-81: Fix typo in JSDoc comment.The comment in
UpdateMessageParamsinterface says "Message data to update" which is correct, but the rest of the comment could be clearer.Apply this diff to improve the comment:
/** - * Message data to update. All fields are updated and if omitted they are - * set to empty. + * Message data to update. All fields will be updated. Omitted optional + * fields will be cleared. */demo/src/containers/Chat/Chat.tsx (2)
171-187: Enhance error handling feedback for usersWhile the prompt issue has been previously addressed, the error handling could be more user-friendly.
Consider adding user feedback for update failures:
.catch((error: unknown) => { - console.warn('failed to update message', error); + console.error('Failed to update message:', error); + // Add user-visible error notification + const errorMessage = error instanceof Error ? error.message : 'Unknown error occurred'; + // TODO: Replace with your preferred notification system + alert(`Failed to update message: ${errorMessage}`); });
171-187: Optimize onUpdateMessage with useCallbackTo prevent unnecessary re-renders of MessageComponent children, wrap onUpdateMessage with useCallback.
Apply this optimization:
-const onUpdateMessage = (message: Message) => { +const onUpdateMessage = useCallback((message: Message) => { const newText = prompt('Enter new text'); if (!newText) { return; } update(message, { text: newText, metadata: message.metadata, headers: message.headers, }) .then((updatedMessage: Message) => { handleUpdatedMessage(updatedMessage); }) .catch((error: unknown) => { console.warn('failed to update message', error); }); -}; +}, [update, handleUpdatedMessage]);Also applies to: 226-226
test/react/hooks/use-messages.integration.test.tsx (2)
135-135: Fix incorrect commentThe comment mentions "deletions" but this test is for message updates.
- // create a second room and attach it, so we can listen for deletions + // create a second room and attach it, so we can listen for updates
130-146: Extract common test setup into helper functionsThe test setup code is duplicated across multiple test cases. Consider extracting the common setup into reusable helper functions:
async function setupTestClients() { const chatClientOne = newChatClient() as unknown as ChatClient; const chatClientTwo = newChatClient() as unknown as ChatClient; return { chatClientOne, chatClientTwo }; } async function setupTestRoom(client: ChatClient) { const roomId = randomRoomId(); const room = client.rooms.get(roomId, RoomOptionsDefaults); await room.attach(); return { roomId, room }; } async function setupMessageSubscription<T extends Message>( room: Room, eventType: MessageEvents, ): Promise<T[]> { const messages: T[] = []; room.messages.subscribe((message) => { if (message.type === eventType) { messages.push(message.message); } }); return messages; }src/core/messages.ts (1)
542-565: Consider optimizing object spread operations.While the implementation is functionally correct, there are multiple object spread operations that could be optimized:
Consider consolidating the spread operations:
async update(message: Message, update: UpdateMessageParams, details?: UpdateMessageDetails): Promise<Message> { this._logger.trace('Messages.update();', { message, update, details }); const response = await this._chatApi.updateMessage(this._roomId, message.timeserial, { - ...details, - message: update, + message: update, + ...(details && { description: details.description, metadata: details.metadata }), }); return new DefaultMessage( message.timeserial, this._clientId, this._roomId, update.text, new Date(message.createdAt), update.metadata ?? {}, update.headers ?? {}, message.deletedAt, message.deletedBy, - message.deletionDetail ? { ...message.deletionDetail } : undefined, + message.deletionDetail ?? undefined, new Date(response.updatedAt), this._clientId, - details ? { ...details } : undefined, + details ?? undefined, ); }This optimization:
- Reduces unnecessary object spread operations
- More efficiently handles optional fields
- Maintains the same functionality with better performance
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
demo/src/components/MessageComponent/MessageComponent.tsx(4 hunks)demo/src/containers/Chat/Chat.tsx(4 hunks)demo/src/index.css(1 hunks)demo/src/main.tsx(1 hunks)src/core/chat-api.ts(4 hunks)src/core/messages.ts(3 hunks)src/react/hooks/use-messages.ts(4 hunks)test/react/hooks/use-messages.integration.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- demo/src/components/MessageComponent/MessageComponent.tsx
- demo/src/index.css
- demo/src/main.tsx
- src/react/hooks/use-messages.ts
🔇 Additional comments (11)
src/core/chat-api.ts (3)
97-97: LGTM! URL encoding is now consistently applied.
The implementation now properly encodes both roomId and message identifiers across all methods, preventing potential issues with special characters in the URLs.
Also applies to: 128-131, 150-150
165-165: LGTM! URL encoding is properly applied.
The implementation now properly encodes roomId in the getOccupancy method, consistent with other methods.
154-162: 🛠️ Refactor suggestion
Add input validation and error documentation.
While the implementation is solid, consider these improvements:
- Add input validation for required fields
- Document possible error cases in JSDoc
Apply this improvement:
+ /**
+ * Updates a message in a chat room.
+ * @param roomId - The ID of the room containing the message
+ * @param serial - The timeserial of the message to update
+ * @param params - The update parameters
+ * @throws {Error} If roomId is empty or params.message.text is empty
+ * @throws {Ably.ErrorInfo} If the request fails
+ * @returns {Promise<UpdateMessageResponse>} The update response
+ */
async updateMessage(roomId: string, serial: string, params: UpdateMessageParams): Promise<UpdateMessageResponse> {
+ if (!roomId?.trim()) {
+ throw new Error('Room ID cannot be empty');
+ }
+ if (!params?.message?.text?.trim()) {
+ throw new Error('Message text cannot be empty');
+ }
const encodedSerial = encodeURIComponent(serial);
roomId = encodeURIComponent(roomId);
return this._makeAuthorizedRequest<UpdateMessageResponse>(
`/chat/v2/rooms/${roomId}/messages/${encodedSerial}`,
'PUT',
params
);
}Likely invalid or redundant comment.
demo/src/containers/Chat/Chat.tsx (4)
25-35: LGTM! Efficient implementation of message update handler
The implementation follows React best practices with proper state immutability and includes appropriate guard clauses.
45-63: LGTM! Well-implemented message event handlers
The handlers properly manage state updates with appropriate optimizations to prevent unnecessary rerenders.
64-67: LGTM! Clean integration of update event handling
The update event handler follows the single responsibility principle and integrates well with the existing event handling structure.
41-72: Verify message event subscription cleanup
Ensure that message event subscriptions are properly cleaned up when the component unmounts.
✅ Verification successful
Message event subscriptions are properly cleaned up
The useMessages hook implementation shows proper cleanup handling:
- Message listener subscription is cleaned up via
sub.unsubscribe()in the useEffect cleanup function - Discontinuity listener is cleaned up via
off()in its useEffect cleanup function - Additional safeguards are in place to handle post-unmount scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cleanup handling in useMessages hook implementation
rg -A 5 "useMessages.*return.*\{"
Length of output: 35
Script:
#!/bin/bash
# Let's first locate the useMessages hook implementation
ast-grep --pattern 'const useMessages = $_'
# Also search for any cleanup patterns in useEffect
rg -A 10 "useEffect.*\{.*return" demo/src/containers/Chat/
Length of output: 6947
test/react/hooks/use-messages.integration.test.tsx (1)
148-182: LGTM! Component implementation is well-structured
The test component effectively demonstrates the message update functionality by:
- Using the hook's
sendandupdatefunctions correctly - Testing all relevant message properties (text, metadata, headers)
- Including update details with description and metadata
src/core/messages.ts (3)
152-162: LGTM! Well-structured update details interface.
The UpdateMessageDetails interface provides a clear and flexible way to attach metadata to message update operations.
261-275: LGTM! Well-documented interface method.
The method signature and documentation provide clear guidance about the update operation, especially the important note about full message replacement.
542-565: Verify update event handling in message processing.
The update implementation looks correct, but let's verify that the message processing logic properly handles update events.
✅ Verification successful
Update event handling is properly implemented
The verification shows that update events are correctly integrated:
MessageActionsToEventsMapproperly mapsMessageUpdateaction toMessageEvents.Updated- The
_processEventmethod processes incoming events and routes them to listeners - The update implementation correctly interfaces with the chat API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that update events are properly handled in the message processing logic
# Test 1: Check if MessageActionsToEventsMap includes the update action
rg "MessageActionsToEventsMap.*MessageUpdate.*MessageEvents.Updated" -A 2
# Test 2: Check if the message processing logic handles update events
ast-grep --pattern 'private _processEvent(channelEventMessage: $_) {
$$$
const { action } = channelEventMessage;
$$$
if (!event) {
$$$
}
$$$
}'
Length of output: 237
Script:
#!/bin/bash
# Let's try a broader search for message update handling
# Search for message update event mapping
rg "MessageUpdate.*=.*" -A 2
# Search for message event processing
rg "_processEvent.*Message" -A 10
# Search for update event handling patterns
ast-grep --pattern 'case MessageActions.Update:
$$$'
# Look for message event types
rg "enum MessageActions" -A 15
Length of output: 1460
Script:
#!/bin/bash
# Let's search for the message actions mapping and event handling
# Search for MessageActionsToEventsMap definition and usage
rg "MessageActionsToEventsMap" -A 5
# Look for the event listener implementation
rg "addListener.*MessageEvents" -A 5
# Search for update message handling in the chat API
rg "updateMessage.*\(" -A 5
# Check for message event types
rg "type ChatMessageActions" -A 10
Length of output: 1735
| render(<TestProvider />); | ||
|
|
||
| // expect a message to be received by the second room | ||
| await waitForMessages(updatesRoomTwo, 1); | ||
| expect(updatesRoomTwo.length).toBe(1); | ||
| const update = updatesRoomTwo[0]; | ||
| expect(update?.isUpdated).toBe(true); | ||
| expect(update?.updatedBy).toBe(chatClientOne.clientId); | ||
| expect(update?.text).toBe('hello universe'); | ||
| expect(update?.metadata).toEqual({ icon: 'universe' }); | ||
| expect(update?.updateDetail?.description).toBe('make it better'); | ||
| expect(update?.updateDetail?.metadata).toEqual({ something: 'else' }); | ||
| }, 10000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing headers assertion
The test verifies most properties but is missing an assertion for the headers property that was set in the update.
expect(update?.text).toBe('hello universe');
expect(update?.metadata).toEqual({ icon: 'universe' });
+expect(update?.headers).toEqual({ awesome: 'yes' });
expect(update?.updateDetail?.description).toBe('make it better');Committable suggestion skipped: line range outside the PR's diff.
splindsay-92
force-pushed
the
materialisation/implement-message-deleting
branch
4 times, most recently
from
aabca88 to
f69c969
Compare
vladvelici
force-pushed
the
materialisation/update-message
branch
from
87ab733 to
553afc2
Compare
splindsay-92
force-pushed
the
materialisation/implement-message-deleting
branch
2 times, most recently
from
67119a6 to
ada400c
Compare
vladvelici
force-pushed
the
materialisation/update-message
branch
from
dbbb1ef to
311bd29
Compare
vladvelici
force-pushed
the
materialisation/update-message
branch
from
311bd29 to
0d9b5b2
Compare
github-actions
bot
temporarily deployed
to
staging/pull/378/typedoc
vladvelici
force-pushed
the
materialisation/update-message
branch
from
0d9b5b2 to
828e738
Compare
github-actions
bot
temporarily deployed
to
staging/pull/378/typedoc
| */ | ||
| export interface DeleteMessageParams { | ||
| export interface ActionDetails { |
There was a problem hiding this comment.
If we do this, are we not tying DeletionParams to ActionDetails? What about extending it, like
export interface DeleteMessageParams extends ActionDetails
Considering we may need to handle hard deletes later? Just leaves us a bit more flexibility, wdyt?
src/core/messages.ts
Outdated
| * Update a message in the chat room. | ||
| * | ||
| * Note that the Promise may resolve before OR after the updated message is | ||
| * received from the realtime channel. This means you may see the message |
There was a problem hiding this comment.
This means you may see the update
src/react/hooks/use-messages.ts
Outdated
| } from '@ably/chat'; | ||
| import * as Ably from 'ably'; | ||
| import { useCallback, useEffect, useState } from 'react'; | ||
|
|
||
| import { ActionDetails } from '../../../dist/chat/messages.js'; |
There was a problem hiding this comment.
I think this import is incorrect
There was a problem hiding this comment.
arhhh I don't know why my editor does this on autocomplete
README.md
Outdated
|
|
||
| #### Handling updates in realtime | ||
|
|
||
| Updated messages received from realtime have the `action` parameter set to `ChatMessageActions.MessageUpdate`, and the event received has the `type` set to `MessageEvents.Updated`. Updated messages are full copies of the message, meaning that all that is needed to keep a state or UI up to date is to replace the old message with the received one. |
README.md
Outdated
| @@ -298,6 +352,8 @@ This is a _soft delete_ and the message will still be available in the history, | |||
| const deletedMessage = await room.messages.delete(message, { description: 'This message was deleted for ...' }); | |||
| ``` | |||
|
|
|||
| Note that you can update delete messages, which will effectively undo the delete. Only the last operation on a message takes effect. | |||
There was a problem hiding this comment.
Note that you can update deleted messages,
vladvelici
force-pushed
the
materialisation/update-message
branch
from
828e738 to
415cbb3
Compare
github-actions
bot
temporarily deployed
to
staging/pull/378/typedoc
vladvelici
force-pushed
the
materialisation/update-message
branch
from
415cbb3 to
158b730
Compare
github-actions
bot
temporarily deployed
to
staging/pull/378/typedoc
vladvelici
force-pushed
the
materialisation/update-message
branch
from
158b730 to
86ce482
Compare
github-actions
bot
temporarily deployed
to
staging/pull/378/typedoc
src/react/hooks/use-messages.ts
Outdated
| @@ -101,14 +107,19 @@ export const useMessages = (params?: UseMessagesParams): UseMessagesResponse => | |||
| [context], | |||
| ); | |||
| const deleteMessage = useCallback( | |||
| (message: Message, deleteMessageParams?: DeleteMessageParams) => | |||
| (message: Message, deleteMessageParams?: ActionDetails) => | |||
There was a problem hiding this comment.
Ahh so sorry, I missed this! It should be DeleteMessageParams here
vladvelici
force-pushed
the
materialisation/update-message
branch
from
86ce482 to
7e97252
Compare
github-actions
bot
temporarily deployed
to
staging/pull/378/typedoc
src/core/messages.ts
Outdated
| * @return A promise that resolves to the deleted message. | ||
| */ | ||
| delete(message: Message, deleteMessageParams?: DeleteMessageParams): Promise<Message>; | ||
| delete(message: Message, deleteMessageParams?: ActionDetails): Promise<Message>; |
There was a problem hiding this comment.
Okay, I promise this is my last comment! Should be DeleteMessageParams no ActionDetails now
src/core/messages.ts
Outdated
| async delete(message: Message, params?: DeleteMessageParams): Promise<Message> { | ||
| this._logger.trace('Messages.delete();', { params }); | ||
| const response = await this._chatApi.deleteMessage(this._roomId, message.serial, params); | ||
| async delete(message: Message, details?: ActionDetails): Promise<Message> { |
There was a problem hiding this comment.
Same here, the signatures don't match with the interface above
use message details metadata instead of Record in chat-api update Update demo app to show edited time, edit button, and simplify on hover with css only. Parse timeserial for createdAt and update tests. Fix short date format in demo app.
vladvelici
force-pushed
the
materialisation/update-message
branch
from
7e97252 to
f150526
Compare
Context
Add update message capability in the SDK, react hooks, and demo app.
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Style
Tests