fix: Pin icon not appearing immediately after pinning #6936
+102
−3
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
- Fix duplicate pin icon issue by removing redundant RightIcons rendering on header messages - Implement optimistic updates for pin action to show immediate UI feedback - Fix message color update when status changes from TEMP to SENT by adding isTemp to React.memo comparison - Add optimisticUpdates helper to prevent server race conditions from overwriting user actions
Contributor
WalkthroughImplements per-message optimistic updates for pin actions: adds an in-memory optimistic-update helper, applies optimistic DB updates and registration in MessageActions with rollback on failure, preserves optimistic pinned state in room subscription reconciliation, and introduces a small clarity change in the Message component conditional. Changes
Sequence DiagramsequenceDiagram
actor User
participant MessageActions
participant LocalDB as Local DB
participant OptimisticStore as Optimistic Store
participant API as Server API
participant RoomSub as Room Subscription
participant Store as Message Store
User->>MessageActions: Trigger pin/unpin
MessageActions->>LocalDB: Optimistically set message.pinned = willPin
MessageActions->>OptimisticStore: registerOptimisticUpdate(messageId, { pinned: willPin })
MessageActions->>API: Call togglePinMessage(currentPinned)
alt API Success
API-->>MessageActions: Success
MessageActions->>OptimisticStore: clearOptimisticUpdate(messageId)
else API Failure
API-->>MessageActions: Error
MessageActions->>LocalDB: Revert message.pinned = currentPinned
MessageActions->>OptimisticStore: clearOptimisticUpdate(messageId)
end
RoomSub->>RoomSub: Receive server message update
RoomSub->>OptimisticStore: isRecentOptimisticUpdate(messageId)?
alt Recent optimistic update exists
OptimisticStore-->>RoomSub: getOptimisticUpdate(messageId) -> { pinned }
RoomSub->>Store: Apply server data but preserve optimistic pinned
RoomSub->>OptimisticStore: clearOptimisticUpdate(messageId)
else No recent update
RoomSub->>Store: Apply server data as-is
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 5
🤖 Fix all issues with AI agents
In `@app/containers/message/Message.tsx`:
- Around line 216-226: The custom memo comparator for the Message component only
compares eight props and so can prevent necessary re-renders; update the
comparator (the function passed as the second argument to React.memo around
Message) to either remove the custom comparator and rely on React's default
shallow comparison or expand the comparator to include all props used during
render (e.g., type, isInfo, isIgnored, isThreadReply, isThreadSequential,
unread, isReadReceiptEnabled, ts, author, mentions, channels, attachments,
isBeingEdited, highlighted, and any other props referenced in Message.tsx) so
changes to those fields trigger re-renders.
In `@app/containers/MessageActions/index.tsx`:
- Around line 328-343: The optimistic update is registered before the DB write,
so if database write in database.active / getMessageById / messageRecord.update
(inside protectedFunction) fails the optimistic state remains inconsistent;
change the flow to perform the DB write first and only call
registerOptimisticUpdate(message.id, { pinned: willPin }) after the await
db.write(...) completes successfully (or alternatively call
registerOptimisticUpdate then rollback/unregister in the catch), ensuring
registerOptimisticUpdate is invoked only on successful messageRecord.update.
- Around line 324-325: The code casts message.pinned to boolean using "as
boolean" which is misleading when pinned can be undefined/null; change the logic
in the block that sets currentPinned and willPin to explicitly coerce the value
(e.g., use Boolean(message.pinned) or !!message.pinned) so currentPinned is a
real boolean and willPin = !currentPinned remains correct; update the assignment
sites for currentPinned and willPin accordingly (referencing the message.pinned,
currentPinned and willPin variables).
In `@app/lib/methods/helpers/optimisticUpdates.ts`:
- Around line 1-26: optimisticUpdates can grow unbounded because stale entries
are only removed via clearOptimisticUpdate; add an automatic eviction routine
that deletes entries older than a configurable TTL (default e.g. 2000ms) to
prevent memory leaks. Implement a cleanup function (e.g.,
cleanupStaleOptimisticUpdates) that iterates the optimisticUpdates Map and
deletes entries where Date.now() - entry.timestamp > TTL, call this on a
periodic timer (setInterval) started when the module loads, and provide a way to
stop the timer for cleanup (export stopOptimisticUpdateCleanup) if needed; keep
existing APIs registerOptimisticUpdate, getOptimisticUpdate,
clearOptimisticUpdate, and isRecentOptimisticUpdate but ensure
registerOptimisticUpdate and isRecentOptimisticUpdate respect the same TTL.
In `@app/lib/methods/subscriptions/room.ts`:
- Around line 288-305: Remove the redundant early assignment of m.pinned: the
block that checks message.pinned and sets m.pinned before the Object.assign is
immediately overwritten by the later identical block after Object.assign; keep
the latter block (the assignment after Object.assign) and delete the first one
to avoid duplicate logic involving message.pinned, isRecentOptimistic and
optimisticUpdate when updating m (the variables m, message, optimisticUpdate,
isRecentOptimistic and the Object.assign(m, restMessage) call identify the
location).
🧹 Nitpick comments (2)
app/containers/MessageActions/index.tsx (1)
345-365: MissingclearOptimisticUpdatecall on successful server response.When
togglePinMessagesucceeds (no exception), the optimistic update entry remains in the Map until the room subscription handler clears it. If the subscription update is delayed or never arrives, the stale entry persists. Consider clearing explicitly on success.♻️ Suggested improvement
try { await togglePinMessage(message.id, currentPinned); + // Clear optimistic update on success - subscription will provide authoritative state + clearOptimisticUpdate(message.id); } catch (e) {This requires importing
clearOptimisticUpdate:-import { registerOptimisticUpdate } from '../../lib/methods/helpers/optimisticUpdates'; +import { registerOptimisticUpdate, clearOptimisticUpdate } from '../../lib/methods/helpers/optimisticUpdates';app/lib/methods/subscriptions/room.ts (1)
307-309: Optimistic update not cleared when server response arrives after the 2000ms window.If the server update arrives after
maxAge(2000ms),isRecentOptimisticisfalse, so the entry won't be cleared here. Combined with no explicit clear on success inhandlePin, this entry will persist indefinitely. Consider clearing unconditionally when a server update for the same message arrives, or implementing automatic cleanup in the optimisticUpdates module.♻️ Suggested fix: Clear unconditionally when processing server update
if (message.pinned !== undefined) { if (isRecentOptimistic && optimisticUpdate?.pinned !== undefined) { m.pinned = optimisticUpdate.pinned; } else { m.pinned = message.pinned; } } -if (isRecentOptimistic) { - clearOptimisticUpdate(message._id); -} +// Always clear optimistic update when server provides authoritative state +if (optimisticUpdate) { + clearOptimisticUpdate(message._id); +}
- Fix duplicate pin icon issue by removing redundant RightIcons rendering on header messages - Implement optimistic updates for pin action to show immediate UI feedback - Fix message color update when status changes from TEMP to SENT by adding isTemp to React.memo comparison - Add optimisticUpdates helper to prevent server race conditions from overwriting user actions - Fix type coercion by using Boolean() instead of 'as boolean' assertion - Register optimistic update only after successful DB write to prevent inconsistency - Add automatic cleanup for stale optimistic updates to prevent memory leaks - Remove duplicate pinned assignment logic in updateMessage - Remove incomplete React.memo comparator to prevent stale UI
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 the bug where the pin icon does not appear immediately after a user pins a message. The fix implements optimistic UI updates to provide instant visual feedback too.
Issue(s)
Closes #6927
How to test or reproduce
Test pin icon display:
Test optimistic updates:
Screenshots/Video
Screen.Recording.2026-01-21.at.5.15.35.PM.mov
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.