BA-2234 Optimistic updates #212
Conversation
tsl-ps2
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThe changes update the GraphQL subscription and mutation definitions. The Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/messages/common/graphql/mutations/SendMessage.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/messages/web/SendMessage/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/components/modules/messages/graphql/subscriptions/useMessagesListSubscription.tsx (1)
20-32: Update useMemo dependency array to include profileId.The config memo's dependency array should include profileId since it's used in the variables object.
- }, [roomId]) + }, [roomId, profileId])
🧹 Nitpick comments (1)
packages/components/modules/messages/SendMessage/index.tsx (1)
126-137: Consolidate error handling to avoid duplicate notifications.Currently, similar toast messages are shown in both error branches. Consider consolidating the error handling.
onCompleted: (response, errors) => { - if (errors) { - // TODO: handle errors - console.error(errors) - sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) - } const mutationErrors = response?.chatRoomSendMessage?.errors - if (mutationErrors) { + if (errors || mutationErrors) { console.log(mutationErrors) + console.error(errors) setFormRelayErrors(form, mutationErrors) sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/components/__generated__/useMessagesListSubscription.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (5)
packages/components/modules/messages/MessagesList/index.tsx(1 hunks)packages/components/modules/messages/SendMessage/index.tsx(4 hunks)packages/components/modules/messages/graphql/mutations/SendMessage.ts(1 hunks)packages/components/modules/messages/graphql/subscriptions/useMessagesListSubscription.tsx(2 hunks)packages/components/schema.graphql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
packages/components/modules/messages/graphql/subscriptions/useMessagesListSubscription.tsx (1)
6-7: LGTM! Added profileId parameter to subscription.The subscription now correctly includes profileId for filtering messages based on user context.
packages/components/modules/messages/graphql/mutations/SendMessage.ts (1)
8-9: LGTM! Added raw_response_type directive.The directive ensures raw GraphQL response handling, which is beneficial for optimistic updates and error handling.
packages/components/modules/messages/SendMessage/index.tsx (2)
107-125: LGTM! Well-structured optimistic response.The optimistic response includes all necessary fields for immediate UI feedback, improving user experience.
145-145: LGTM! Immediate form reset.Resetting the form immediately after sending aligns with the PR objective of clearing the message box without delay.
packages/components/schema.graphql (1)
1651-1653: Updated Subscription Signatures with Profile Filtering
The subscription definitions for chat-related events have been updated to include a required profileId parameter. This change enforces that subscriptions such as chatRoomOnNewMessage, chatRoomOnRoomUpdate, and chatRoomOnMessagesCountUpdate now need both a profileId and, where applicable, a roomId. This aligns with the optimistic update requirement where the client must clearly identify the user context (via profileId) when subscribing to chat events.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/modules/messages/SendMessage/index.tsx (3)
128-139: Consider consolidating error handling logic.The error handling is duplicated between the
errorsandmutationErrorsblocks, both showing similar toast messages.onCompleted: (response, errors) => { - if (errors) { - // TODO: handle errors - console.error(errors) - sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) - } const mutationErrors = response?.chatRoomSendMessage?.errors if (mutationErrors) { console.log(mutationErrors) setFormRelayErrors(form, mutationErrors) - sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) } + if (errors || mutationErrors) { + sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) + } }
129-133: Remove TODO comment as it's already implemented.The error handling is now implemented with toast notifications, so the TODO comment can be removed.
if (errors) { - // TODO: handle errors console.error(errors) sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) }
141-141: Remove redundant TODO comment.The error handling is already implemented in the
onErrorcallback below.-// TODO: handle errors
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/components/__generated__/SendMessageMutation.graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (2)
packages/components/modules/messages/SendMessage/index.tsx(4 hunks)packages/components/modules/messages/graphql/mutations/SendMessage.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: Component Test Applications and Packages
🔇 Additional comments (6)
packages/components/modules/messages/graphql/mutations/SendMessage.ts (3)
8-9: LGTM! The @raw_response_type directive addition is appropriate.The directive ensures raw response handling, which is beneficial for optimistic updates implementation.
14-14: LGTM! The messageType field addition is well-placed.This field helps distinguish between different types of messages in the chat interface.
38-40: LGTM! Error handling with notifications aligns with requirements.The implementation correctly shows toast notifications for both mutation errors and network errors, satisfying the business requirement to inform users of message send failures.
Also applies to: 44-47
packages/components/modules/messages/SendMessage/index.tsx (3)
107-127: LGTM! Optimistic response implementation is thorough.The optimistic response provides immediate feedback by showing the message in chat history before server confirmation, meeting the requirement for instant message display.
147-147: LGTM! Form reset implementation meets requirements.The form is reset immediately after sending, satisfying the requirement to clear the message box instantly.
88-89: LGTM! Send button disable logic is correctly implemented.The
isMutationInFlightcheck prevents multiple submissions and disables the send button during the request, meeting the requirement.
tsl-ps2
force-pushed
the
BA-2234-optimistic-update-when-sending-message
branch
from
bf75314 to
ce4d20e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/modules/messages/web/MessagesList/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (1)
packages/components/modules/messages/web/MessagesList/index.tsx (1)
1-160: Verify optimistic updates implementation in message input component.While this component handles message display and subscription, the PR objectives require implementing optimistic updates for message sending. Please ensure that the message input component implements:
- Immediate message box clearing after send
- Instant message display in chat history
- Failed message removal with toast notification
- Send button disabling during request
Let's verify the implementation in the message input component:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for message input component and mutation handling echo "Searching for message input component..." fd -e tsx -e ts "MessageInput|SendMessage" packages/components/modules/messages echo "Searching for optimistic response handling..." rg -A 10 "optimisticResponse|onCompleted|onError" packages/components/modules/messagesLength of output: 28891
Optimistic updates are implemented in the message input component.
We verified that the
SendMessagecomponent (located atpackages/components/modules/messages/web/SendMessage/index.tsx) incorporates anoptimisticResponsethat immediately inserts a temporary message into the chat history. Additionally, it callsform.reset()to clear the input, handles errors with toast notifications, and utilizesisMutationInFlightto disable the send button during the request.
- The optimistic update creates a client ID using
client:new_message:${Date.now()}.- The mutation callbacks (
onCompletedandonError) manage error reporting withsendToastand reset the form.- The send button is disabled during the mutation via the
isMutationInFlightprop passed to the UI component.This confirms that the optimistic updates are handled in the message input component as required.
tsl-ps2
force-pushed
the
BA-2234-optimistic-update-when-sending-message
branch
from
6365331 to
da38f90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/components/modules/messages/web/SendMessage/index.tsx (1)
127-145: Refactor error handling to reduce duplication.The error handling code has duplicate toast messages and similar logic across different error paths.
Consider consolidating the error handling:
+const handleError = () => { + sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) + // Remove the optimistic message from the store + ConnectionHandler.deleteNode(connectionID, response?.chatRoomSendMessage?.message?.node?.id) +} onCompleted: (response, errors) => { if (errors) { console.error(errors) - sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) + handleError() } const mutationErrors = response?.chatRoomSendMessage?.errors if (mutationErrors) { console.log(mutationErrors) setFormRelayErrors(form, mutationErrors) - sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) + handleError() } }, onError: (errors) => { console.error(errors) - sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) + handleError() },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/components/modules/messages/common/graphql/mutations/SendMessage.ts(1 hunks)packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx(2 hunks)packages/components/modules/messages/web/MessagesList/index.tsx(1 hunks)packages/components/modules/messages/web/SendMessage/index.tsx(4 hunks)packages/components/schema.graphql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/components/schema.graphql
- packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx
- packages/components/modules/messages/common/graphql/mutations/SendMessage.ts
- packages/components/modules/messages/web/MessagesList/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Lint Packages
🔇 Additional comments (3)
packages/components/modules/messages/web/SendMessage/index.tsx (3)
6-6: LGTM! Well-structured imports and hook setup.The addition of
useNotificationand error handling utilities aligns well with the requirement to show toast notifications for failed messages.Also applies to: 18-18, 79-79
106-126: Consider handling thepkfield more robustly.The optimistic response implementation looks good and aligns with the requirement for immediate message display. However, setting
pk: 0for all optimistic updates could potentially cause issues if multiple messages are sent in quick succession.Consider generating a unique temporary
pkvalue, perhaps using a negative counter to ensure no conflicts with real message IDs:- pk: 0, // This property is required, so we need to provide something to keep typescript happy + pk: -nextClientMutationId, // Use negative values to avoid conflicts with real message IDs
149-157: LGTM! Component implementation meets requirements.The component correctly implements:
- Disabling the send button during mutation with
isLoading={isMutationInFlight}- Immediate message box clearing with
form.reset()
tsl-ps2
force-pushed
the
BA-2234-optimistic-update-when-sending-message
branch
from
da38f90 to
d643485
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/CHANGELOG.md (1)
7-8: Enhance Changelog Entry DetailThe changelog entry currently reads:
- Use optimistic updates when sending messagesWhile this succinctly communicates the update, consider expanding this entry to capture key behaviors such as:
- Immediately clearing the message box after sending.
- Displaying the sent message optimistically in the chat history.
- Disabling the send button until the message request (success or failure) resolves.
- Removing the message and showing a toast notification on failure.
This added detail can help users understand the UX improvements without needing to refer to external documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/common/graphql/mutations/SendMessage.ts(1 hunks)packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx(2 hunks)packages/components/modules/messages/web/MessagesList/index.tsx(1 hunks)packages/components/modules/messages/web/SendMessage/index.tsx(4 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/components/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/components/modules/messages/web/MessagesList/index.tsx
- packages/components/schema.graphql
- packages/components/modules/messages/common/graphql/mutations/SendMessage.ts
- packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx
- packages/components/modules/messages/web/SendMessage/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (2)
packages/components/CHANGELOG.md (2)
3-4: Version Bump ConfirmationThe new version header
## 1.0.16is correctly added and follows semantic versioning for a patch release.
5-6: Patch Changes Section HeaderThe addition of the
### Patch Changesheader clearly categorizes these changes and maintains the consistency of the changelog format.
anicioalexandre
added
the
approved
tsl-ps2
force-pushed
the
BA-2234-optimistic-update-when-sending-message
branch
from
3f1473c to
ec9ca48
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/components/modules/messages/web/SendMessage/index.tsx (1)
130-132:⚠️ Potential issueAdd logic to remove failed messages from chat history.
The error handling implementation is missing a crucial requirement: removing the message from chat history when the send operation fails.
Consider adding cleanup logic in the error handlers:
if (errors) { // TODO: handle errors sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) + // Remove the optimistic message from the store + ConnectionHandler.deleteNode(connectionID, response?.chatRoomSendMessage?.message?.node?.id) } const mutationErrors = response?.chatRoomSendMessage?.errors if (mutationErrors) { setFormRelayErrors(form, mutationErrors) sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) + // Remove the optimistic message from the store + ConnectionHandler.deleteNode(connectionID, response?.chatRoomSendMessage?.message?.node?.id) }And in the onError handler:
onError: () => { sendToast('Your last message could not be sent. Please try again.', { type: 'error' }) + // Remove the optimistic message from the store using the temporary ID + ConnectionHandler.deleteNode(connectionID, `client:new_message:${Date.now()}`) },Also applies to: 134-137, 140-142
🧹 Nitpick comments (4)
packages/components/CHANGELOG.md (1)
7-7: Grammar Correction in Optimistic Update DescriptionThe changelog bullet
- Use optimistic update when sending message
has a grammatical inconsistency regarding number agreement. Consider revising it to:- Use optimistic update when sending message + Implement optimistic updates for sending messagesThis change improves clarity and ensures proper pluralization consistent with standard documentation practices.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...s - Use optimistic update when sending message ## 1.0.20 ### Patch Changes - Update...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
packages/components/modules/messages/web/SendMessage/index.tsx (3)
130-130: Remove TODO comments before finalizing PRThe TODO comment should be addressed or removed, as it appears to be a remnant from development.
139-139: Remove TODO comments before finalizing PRThe TODO comment should be addressed or removed, as it appears to be a remnant from development.
110-110: Potential issue with optimistic update ID generationUsing
Date.now()for generating IDs could potentially cause conflicts if multiple messages are sent in the same millisecond. Consider using a more robust ID generation method, such as UUID or incrementing a counter.Additionally, make sure to use the same ID format in both the optimistic response and the error cleanup code to properly identify and remove failed messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/common/graphql/mutations/SendMessage.ts(1 hunks)packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx(2 hunks)packages/components/modules/messages/web/MessagesList/index.tsx(1 hunks)packages/components/modules/messages/web/SendMessage/index.tsx(4 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/components/package.json
- packages/components/modules/messages/web/MessagesList/index.tsx
- packages/components/schema.graphql
- packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx
- packages/components/modules/messages/common/graphql/mutations/SendMessage.ts
🧰 Additional context used
🪛 LanguageTool
packages/components/CHANGELOG.md
[uncategorized] ~7-~7: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...s - Use optimistic update when sending message ## 1.0.20 ### Patch Changes - Update...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (4)
packages/components/CHANGELOG.md (1)
3-5: Proper Version Header UpdateThe new version header "## 1.0.21" along with the "### Patch Changes" title is correctly added to indicate the start of a new release entry.
packages/components/modules/messages/web/SendMessage/index.tsx (3)
106-127: Implementation of optimistic updates looks good!The optimistic response provides immediate UI feedback by simulating the message before server confirmation. This addresses the requirement to display messages in chat history without delay, creating a more responsive user experience.
144-144: Nice implementation of immediate form resetResetting the form immediately after sending satisfies the requirement to clear the message box without waiting for server response.
152-152: Verify disable send button implementationThe code passes
isMutationInFlightto theSocialInputcomponent asisLoading. This likely handles disabling the send button during message sending, which meets the requirement. Please verify that theSocialInputcomponent properly disables the button whenisLoadingis true.
tsl-ps2
force-pushed
the
BA-2234-optimistic-update-when-sending-message
branch
from
ec9ca48 to
a507aea
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/CHANGELOG.md (1)
3-8: Grammar Correction Needed.
The bullet point currently reads:
- Use optimistic update when sending message
For better clarity and consistency with the plural nature of the feature, consider updating it to:
- Use optimistic updates when sending messages- - Use optimistic update when sending message + - Use optimistic updates when sending messages🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...s - Use optimistic update when sending message ## 1.0.25 ### Patch Changes - Forwar...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/messages/common/graphql/mutations/SendMessage.ts(1 hunks)packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx(2 hunks)packages/components/modules/messages/web/MessagesList/index.tsx(1 hunks)packages/components/modules/messages/web/SendMessage/index.tsx(4 hunks)packages/components/package.json(1 hunks)packages/components/schema.graphql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/components/package.json
- packages/components/modules/messages/web/SendMessage/index.tsx
- packages/components/schema.graphql
- packages/components/modules/messages/common/graphql/mutations/SendMessage.ts
- packages/components/modules/messages/web/MessagesList/index.tsx
- packages/components/modules/messages/common/graphql/subscriptions/useMessagesListSubscription.tsx
🧰 Additional context used
🪛 LanguageTool
packages/components/CHANGELOG.md
[uncategorized] ~7-~7: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...s - Use optimistic update when sending message ## 1.0.25 ### Patch Changes - Forwar...
(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
Acceptance Criteria
Context
Loom: https://www.loom.com/share/9bcd130aa70f476fa3cebbb53ba338e0
Business Rules
Given I input text into the message box, when I send the message, then the message box should become empty immediately
Given I have sent a message, the message should be displayed in the chat history quickly.
If a message fails to be send, just remove the message from the chat history.
Disable the send button until we know the message sending request was resolved, either successful or failed,
Approvd
https://app.approvd.io/silverlogic/BA/stories/38918
Summary by CodeRabbit
New Features
Version Updates