-
Notifications
You must be signed in to change notification settings - Fork 13k
regression: Jump to message on sent message not working #38297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 0ffafec The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes modify message event handling to differentiate between incoming and sent messages. Sent messages now trigger automatic scroll-to-bottom behavior through the Changes
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.1.0 #38297 +/- ##
================================================
Coverage ? 70.81%
================================================
Files ? 3158
Lines ? 109356
Branches ? 19700
================================================
Hits ? 77441
Misses ? 29886
Partials ? 2029
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 2 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts (1)
67-77: Missing room and message type guards inafterSaveMessagecallback.The callback should include the same guards as
streamNewMessageabove (and as done inuseLegacyThreadMessageListScrolling.ts):
- Room ID check (
rid !== msg.rid) — without this, messages from other rooms could trigger scroll- Edited message check (
isEditedMessage(msg)) — edits shouldn't trigger scroll- Thread message check (
msg.tmid) — thread replies shouldn't scroll the main roomProposed fix
clientCallbacks.add( 'afterSaveMessage', (msg: IMessage) => { + if (rid !== msg.rid || isEditedMessage(msg) || msg.tmid) { + return; + } + if (msg.u._id === uid) { sendToBottom(); setHasNewMessages(false); } }, clientCallbacks.priority.MEDIUM, rid, );
a36ab9d to
0ffafec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts (1)
67-77: Missing guard conditions inafterSaveMessagecallback.The
afterSaveMessagecallback is missing the same guard conditions present in thestreamNewMessagecallback (lines 55-57). Without these checks,sendToBottom()will be incorrectly triggered when:
- The user sends a message in a different room
- The user edits an existing message
- The user sends a thread reply (which shouldn't scroll the main room)
Note that
useLegacyThreadMessageListScrolling.tscorrectly includes these guards in itsafterSaveMessagecallback.🐛 Proposed fix
clientCallbacks.add( 'afterSaveMessage', (msg: IMessage) => { + if (rid !== msg.rid || isEditedMessage(msg) || msg.tmid) { + return; + } + if (msg.u._id === uid) { sendToBottom(); setHasNewMessages(false); } }, clientCallbacks.priority.MEDIUM, rid, );
Proposed changes (including videos or screenshots)
In #38067 we removed a delay when sending a message, but the
streamNewMessagecallback happened to run between this delay, considering the recently sent message as a new message, with the delay removed, the message is upserted before the callback run, so it does not consider it a new message and does not do thesendToBottom()callIssue(s)
CORE-1748
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.