fix: Add star icon display and optimistic updates for star/pin actions #6933
+193
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add Starred component to display star icon on messages - Implement optimistic updates for star/pin actions to show immediate UI feedback - Fix message color update when status changes from TEMP to SENT - Add isTemp prop to React.memo comparison to ensure re-renders on status change - Fix duplicate pin icon issue by removing redundant RightIcons rendering
Contributor
WalkthroughImplements optimistic UI updates for star and pin message actions to immediately reflect state changes in the interface. Adds a Starred component for star icon display and creates an optimistic updates tracking system with server mutation failure revert logic. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Message Actions UI
participant OptTracker as Optimistic Tracker
participant LocalDB as Local Database
participant Server as Server API
participant Sub as Room Subscription
UI->>OptTracker: registerOptimisticUpdate(messageId, {starred/pinned})
UI->>LocalDB: Update message with new starred/pinned value
UI->>Server: POST toggleStar/togglePin mutation
alt Mutation Success
Server-->>Sub: New message via stream-room-messages
Sub->>LocalDB: updateMessage with server data
Sub->>OptTracker: clearOptimisticUpdate(messageId)
else Mutation Failure
Server-->>UI: Error response
UI->>LocalDB: Revert to original starred/pinned value
UI->>OptTracker: clearOptimisticUpdate(messageId)
end
sequenceDiagram
participant Sub as Room Subscription
participant OptTracker as Optimistic Tracker
participant LocalDB as Local Database
participant UI as Message Component
Sub->>Sub: Receive updated message from stream
Sub->>OptTracker: isRecentOptimisticUpdate(messageId)
alt Recent Optimistic Update Exists
Sub->>OptTracker: getOptimisticUpdate(messageId)
Sub->>LocalDB: Merge with optimistic values preserved
Sub->>OptTracker: clearOptimisticUpdate(messageId)
else No Recent Update
Sub->>LocalDB: Standard merge of all fields
end
LocalDB-->>UI: Trigger re-render with updated message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@app/containers/message/Components/RightIcons/Starred.tsx`:
- Around line 6-8: Remove the invalid string literal directive 'use memo' from
the Starred component declaration (the function named Starred that accepts
starred and testID props); if memoization is desired wrap the exported component
with React.memo(Starred) instead of using a string directive, and ensure imports
include React and memoization is applied where the component is exported.
In `@app/containers/message/Message.tsx`:
- Around line 106-108: The React.memo comparator for the Message component
currently omits the unread and isReadReceiptEnabled props, causing stale UI when
those values change; update the comparator used with React.memo for Message to
include both props (unread and isReadReceiptEnabled) in its equality checks,
ensuring changes trigger re-renders where those props are used (e.g.,
accessibility label logic and RightIcons rendering), or alternatively remove the
custom comparator to rely on default shallow comparison if you prefer; locate
the comparator attached to the Message component and add checks for props.unread
and props.isReadReceiptEnabled (and the corresponding prevProps values) to
return false when they differ.
In `@app/containers/MessageActions/index.tsx`:
- Around line 312-358: The optimistic update registered by
registerOptimisticUpdate in handleStar isn't cleared on failures, causing stale
optimistic state to reapply; update handleStar to call the optimistic-clear API
(e.g., clearOptimisticUpdate or unregisterOptimisticUpdate with the same
messageId) in both the optimisticError catch block and the server-failure catch
branch after you revert the local DB change, and also clear it after a
successful toggleStarMessage call; apply the same changes to the corresponding
pin handler (handlePin/togglePinMessage) so optimistic entries are always
removed on success or any failure.
In `@app/lib/methods/helpers/optimisticUpdates.ts`:
- Around line 9-13: registerOptimisticUpdate currently overwrites any existing
optimistic entry for a messageId, clobbering fields from prior quick updates;
change it to first read the existing entry from optimisticUpdates
(optimisticUpdates.get(messageId)), shallow-merge its fields with the incoming
update so unspecified fields are preserved, update the timestamp to Date.now(),
and then write the merged object back with optimisticUpdates.set(messageId,
mergedEntry) so consecutive calls (e.g., star then pin) don't revert earlier
fields.
🧹 Nitpick comments (1)
app/lib/methods/subscriptions/room.ts (1)
283-321: Remove duplicated pinned/starred merge block.The pinned/starred assignments run twice (before and after
Object.assign). The first block is redundant and can be removed to keep the merge logic single-source.♻️ Suggested simplification
- const optimisticUpdate = getOptimisticUpdate(message._id); - const isRecentOptimistic = isRecentOptimisticUpdate(message._id, 2000); - - if (message.pinned !== undefined) { - if (isRecentOptimistic && optimisticUpdate?.pinned !== undefined) { - m.pinned = optimisticUpdate.pinned; - } else { - m.pinned = message.pinned; - } - } - if (message.starred !== undefined) { - if (isRecentOptimistic && optimisticUpdate?.starred !== undefined) { - m.starred = optimisticUpdate.starred; - } else { - m.starred = message.starred; - } - } - - const { pinned: _pinned, starred: _starred, ...restMessage } = message; - Object.assign(m, restMessage); + const optimisticUpdate = getOptimisticUpdate(message._id); + const isRecentOptimistic = isRecentOptimisticUpdate(message._id, 2000); + + const { pinned: _pinned, starred: _starred, ...restMessage } = message; + Object.assign(m, restMessage);
Comment on lines
+6
to
+8
| const Starred = ({ starred, testID }: { starred?: boolean; testID?: string }): React.ReactElement | null => { | ||
| 'use memo'; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for 'use memo' usage throughout the codebase
echo "=== Searching for 'use memo' usage ==="
rg -n "'use memo'" --type ts --type tsx 2>/dev/null || echo "No matches found"
echo -e "\n=== Checking for React Compiler configuration ==="
# Look for React Compiler or compiler-related config
fd -e 'babel.config.*' -o -e 'next.config.*' -o -e '.reactcompilerconfig.*' -o -e 'tsconfig.json' | head -20
echo -e "\n=== Checking next.config or babel config for React Compiler ==="
git ls-files | grep -E '(next\.config|babel\.config|tsconfig)' | head -10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Check if any config files mention 'use memo' or React Compiler
echo "=== Searching config files for compiler directives ==="
rg -l "compiler|use memo" . -g '*.json' -g '*.js' -g '*.ts' -g '*.config.*' --max-depth 3 | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 212
🏁 Script executed:
#!/bin/bash
# Look at the actual file context to understand the component structure
echo "=== Reading the Starred.tsx file ==="
cat -n "app/containers/message/Components/RightIcons/Starred.tsx" | head -30Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 212
Remove the 'use memo' string literal—it is not a recognized directive.
The string 'use memo' does not match any standard React, Babel, or React Compiler directive. It is a no-op and should be removed. If the component needs memoization, wrap it with React.memo() instead.
🧹 Suggested cleanup
- 'use memo';📝 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.
Suggested change
| const Starred = ({ starred, testID }: { starred?: boolean; testID?: string }): React.ReactElement | null => { | |
| 'use memo'; | |
| const Starred = ({ starred, testID }: { starred?: boolean; testID?: string }): React.ReactElement | null => { | |
🤖 Prompt for AI Agents
In `@app/containers/message/Components/RightIcons/Starred.tsx` around lines 6 - 8,
Remove the invalid string literal directive 'use memo' from the Starred
component declaration (the function named Starred that accepts starred and
testID props); if memoization is desired wrap the exported component with
React.memo(Starred) instead of using a string directive, and ensure imports
include React and memoization is applied where the component is exported.
Comment on lines
+106
to
+108
| const Message = React.memo( | ||
| (props: IMessageTouchable & IMessage) => { | ||
| const handleMentionsOnAccessibilityLabel = (label: string) => { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l app/containers/message/Message.tsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 212
🏁 Script executed:
cat -n app/containers/message/Message.tsx | sed -n '100,240p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 5781
React.memo comparator omits unread and isReadReceiptEnabled props → stale UI risk.
These props drive the accessibility label and are passed directly to RightIcons (lines 207–208), but the comparator omits them, so changes won't trigger re-renders. Add them to the comparator or revisit the memoization strategy.
🩹 Minimal fix (include read‑receipt props)
(prevProps, nextProps) =>
prevProps.starred === nextProps.starred &&
prevProps.pinned === nextProps.pinned &&
prevProps.id === nextProps.id &&
prevProps.msg === nextProps.msg &&
prevProps.isEdited === nextProps.isEdited &&
prevProps.hasError === nextProps.hasError &&
prevProps.isTemp === nextProps.isTemp &&
prevProps.isTranslated === nextProps.isTranslated &&
- prevProps.isHeader === nextProps.isHeader
+ prevProps.isHeader === nextProps.isHeader &&
+ prevProps.unread === nextProps.unread &&
+ prevProps.isReadReceiptEnabled === nextProps.isReadReceiptEnabledAlso applies to: 219-228
🤖 Prompt for AI Agents
In `@app/containers/message/Message.tsx` around lines 106 - 108, The React.memo
comparator for the Message component currently omits the unread and
isReadReceiptEnabled props, causing stale UI when those values change; update
the comparator used with React.memo for Message to include both props (unread
and isReadReceiptEnabled) in its equality checks, ensuring changes trigger
re-renders where those props are used (e.g., accessibility label logic and
RightIcons rendering), or alternatively remove the custom comparator to rely on
default shallow comparison if you prefer; locate the comparator attached to the
Message component and add checks for props.unread and props.isReadReceiptEnabled
(and the corresponding prevProps values) to return false when they differ.
Comment on lines
312
to
358
| const handleStar = async (messageId: string, starred: boolean) => { | ||
| const newStarred = !starred; | ||
| logEvent(starred ? events.ROOM_MSG_ACTION_UNSTAR : events.ROOM_MSG_ACTION_STAR); | ||
|
|
||
| try { | ||
| const db = database.active; | ||
| registerOptimisticUpdate(messageId, { starred: newStarred }); | ||
|
|
||
| let messageRecord = await getMessageById(messageId); | ||
| if (!messageRecord) { | ||
| await new Promise(resolve => setTimeout(resolve, 100)); | ||
| messageRecord = await getMessageById(messageId); | ||
| } | ||
|
|
||
| if (messageRecord) { | ||
| await db.write(async () => { | ||
| await messageRecord.update( | ||
| protectedFunction((m: any) => { | ||
| m.starred = newStarred; | ||
| }) | ||
| ); | ||
| }); | ||
| } | ||
| } catch (optimisticError) { | ||
| // Do nothing | ||
| } | ||
|
|
||
| try { | ||
| await toggleStarMessage(messageId, starred); | ||
| EventEmitter.emit(LISTENER, { message: starred ? I18n.t('Message_unstarred') : I18n.t('Message_starred') }); | ||
| } catch (e) { | ||
| try { | ||
| const db = database.active; | ||
| const messageRecord = await getMessageById(messageId); | ||
| if (messageRecord) { | ||
| await db.write(async () => { | ||
| await messageRecord.update( | ||
| protectedFunction((m: any) => { | ||
| m.starred = starred; | ||
| }) | ||
| ); | ||
| }); | ||
| } | ||
| } catch (revertError) { | ||
| // Do nothing | ||
| } | ||
| logEvent(events.ROOM_MSG_ACTION_STAR_F); |
Contributor
There was a problem hiding this comment.
Clear optimistic state when the server mutation fails.
If the API call fails and you revert local state, the optimistic map entry remains and can reapply stale values during stream merges. Clear the optimistic update in failure paths for both star and pin.
🛠️ Suggested fix
-import { registerOptimisticUpdate } from '../../lib/methods/helpers/optimisticUpdates';
+import { registerOptimisticUpdate, clearOptimisticUpdate } from '../../lib/methods/helpers/optimisticUpdates'; } catch (optimisticError) {
- // Do nothing
+ clearOptimisticUpdate(messageId);
}
@@
} catch (e) {
try {
const db = database.active;
const messageRecord = await getMessageById(messageId);
if (messageRecord) {
@@
}
} catch (revertError) {
// Do nothing
+ } finally {
+ clearOptimisticUpdate(messageId);
}
logEvent(events.ROOM_MSG_ACTION_STAR_F);
log(e);
} } catch (optimisticError) {
- // Do nothing
+ clearOptimisticUpdate(message.id);
}
@@
} catch (e) {
try {
const db = database.active;
const messageRecord = await getMessageById(message.id);
if (messageRecord) {
@@
}
} catch (revertError) {
// Do nothing
+ } finally {
+ clearOptimisticUpdate(message.id);
}
logEvent(events.ROOM_MSG_ACTION_PIN_F);
log(e);
}Also applies to: 363-405
🤖 Prompt for AI Agents
In `@app/containers/MessageActions/index.tsx` around lines 312 - 358, The
optimistic update registered by registerOptimisticUpdate in handleStar isn't
cleared on failures, causing stale optimistic state to reapply; update
handleStar to call the optimistic-clear API (e.g., clearOptimisticUpdate or
unregisterOptimisticUpdate with the same messageId) in both the optimisticError
catch block and the server-failure catch branch after you revert the local DB
change, and also clear it after a successful toggleStarMessage call; apply the
same changes to the corresponding pin handler (handlePin/togglePinMessage) so
optimistic entries are always removed on success or any failure.
Comment on lines
+9
to
+13
| export const registerOptimisticUpdate = (messageId: string, update: { starred?: boolean; pinned?: boolean }) => { | ||
| optimisticUpdates.set(messageId, { | ||
| ...update, | ||
| timestamp: Date.now() | ||
| }); |
Contributor
There was a problem hiding this comment.
Avoid clobbering other optimistic fields on consecutive updates.
If a user stars then pins quickly, the second call overwrites the first update and can cause the earlier field to revert during merge. Merge with the existing entry before storing.
🛠️ Suggested fix
export const registerOptimisticUpdate = (messageId: string, update: { starred?: boolean; pinned?: boolean }) => {
- optimisticUpdates.set(messageId, {
- ...update,
- timestamp: Date.now()
- });
+ const previous = optimisticUpdates.get(messageId) ?? {};
+ optimisticUpdates.set(messageId, {
+ ...previous,
+ ...update,
+ timestamp: Date.now()
+ });
};📝 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.
Suggested change
| export const registerOptimisticUpdate = (messageId: string, update: { starred?: boolean; pinned?: boolean }) => { | |
| optimisticUpdates.set(messageId, { | |
| ...update, | |
| timestamp: Date.now() | |
| }); | |
| export const registerOptimisticUpdate = (messageId: string, update: { starred?: boolean; pinned?: boolean }) => { | |
| const previous = optimisticUpdates.get(messageId) ?? {}; | |
| optimisticUpdates.set(messageId, { | |
| ...previous, | |
| ...update, | |
| timestamp: Date.now() | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In `@app/lib/methods/helpers/optimisticUpdates.ts` around lines 9 - 13,
registerOptimisticUpdate currently overwrites any existing optimistic entry for
a messageId, clobbering fields from prior quick updates; change it to first read
the existing entry from optimisticUpdates (optimisticUpdates.get(messageId)),
shallow-merge its fields with the incoming update so unspecified fields are
preserved, update the timestamp to Date.now(), and then write the merged object
back with optimisticUpdates.set(messageId, mergedEntry) so consecutive calls
(e.g., star then pin) don't revert earlier fields.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes multiple UI synchronization issues related to message star and pin icons, and improves the user experience with optimistic updates.
Key fixes:
Starredcomponent and integrated it into the message UI to display star icons when messages are starredIssue(s)
Closes #6927
How to test or reproduce
Test star icon display:
Test pin icon display:
Test optimistic updates:
Screenshots/Video
ezyZip.mp4
Types of changes
Checklist
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.