fix: Thread's jump to message action changes main channel scroll position#39092
Merged
dionisio-bot[bot] merged 14 commits intodevelopfrom Mar 10, 2026
Merged
fix: Thread's jump to message action changes main channel scroll position#39092dionisio-bot[bot] merged 14 commits intodevelopfrom
dionisio-bot[bot] merged 14 commits intodevelopfrom
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an optional showThreadMessages flag propagated from client (RoomHistoryManager/legacyJumpToMessage) through the DDP/server method loadSurroundingMessages into message model queries to allow excluding thread messages; also adds DOM/connectivity guards in a scroll hook to avoid layout reads/restores during navigation. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant RHM as RoomHistoryManager
participant DDP as DDP Client
participant SRV as Server Method
participant MSG as Messages Model
UI->>RHM: getSurroundingMessages(message, showThreadMessages)
RHM->>DDP: callWithErrorHandling('loadSurroundingMessages', message, limit, showThreadMessages)
DDP->>SRV: loadSurroundingMessages(message, limit, showThreadMessages)
SRV->>MSG: findVisibleByRoomIdBeforeTimestamp(rid, ts, opts, showThreadMessages)
SRV->>MSG: findVisibleByRoomIdAfterTimestamp(rid, ts, opts, showThreadMessages)
MSG-->>SRV: messages (threaded messages filtered when showThreadMessages = false)
SRV-->>DDP: { messages, moreBefore, moreAfter }
DDP-->>RHM: result
RHM-->>UI: deliver surrounding messages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
aleksandernsilva
force-pushed
the
fix/jump-msg-erratic-scroll
branch
from
ef13fd0 to
d328c8a
Compare
🦋 Changeset detectedLatest commit: 168a569 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #39092 +/- ##
===========================================
+ Coverage 70.87% 70.90% +0.03%
===========================================
Files 3196 3196
Lines 113288 113309 +21
Branches 20531 20535 +4
===========================================
+ Hits 80294 80347 +53
+ Misses 30943 30913 -30
+ Partials 2051 2049 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
aleksandernsilva
force-pushed
the
fix/jump-msg-erratic-scroll
branch
from
d328c8a to
66897a8
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 1
🧹 Nitpick comments (1)
apps/meteor/client/lib/utils/legacyJumpToMessage.ts (1)
3-3: Remove commented-out import.This commented-out import is dead code. As per coding guidelines, avoid code comments in the implementation.
🧹 Proposed fix
-// import { flushSync } from 'react-dom';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/lib/utils/legacyJumpToMessage.ts` at line 3, Remove the dead commented-out import line "// import { flushSync } from 'react-dom';" from apps/meteor/client/lib/utils/legacyJumpToMessage.ts so the file contains no commented implementation imports; simply delete that commented line to comply with the coding guidelines and keep the module clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/server/methods/loadSurroundingMessages.ts`:
- Around line 29-31: The loadSurroundingMessages method is missing validation
for the showThreadMessages parameter; add a Meteor check for it (e.g.,
check(showThreadMessages, Boolean)) in the loadSurroundingMessages function
alongside the existing checks for message and limit so the parameter is
validated before use and client input can't pass malformed values.
---
Nitpick comments:
In `@apps/meteor/client/lib/utils/legacyJumpToMessage.ts`:
- Line 3: Remove the dead commented-out import line "// import { flushSync }
from 'react-dom';" from apps/meteor/client/lib/utils/legacyJumpToMessage.ts so
the file contains no commented implementation imports; simply delete that
commented line to comply with the coding guidelines and keep the module clean.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.tsapps/meteor/client/lib/utils/legacyJumpToMessage.tsapps/meteor/client/views/room/body/hooks/useGetMore.tsapps/meteor/server/methods/loadSurroundingMessages.tspackages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.tsapps/meteor/client/lib/utils/legacyJumpToMessage.tspackages/model-typings/src/models/IMessagesModel.tsapps/meteor/client/views/room/body/hooks/useGetMore.tspackages/models/src/models/Messages.tsapps/meteor/server/methods/loadSurroundingMessages.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.tsapps/meteor/client/lib/utils/legacyJumpToMessage.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/lib/utils/legacyJumpToMessage.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/utils/legacyJumpToMessage.tsapps/meteor/client/views/room/body/hooks/useGetMore.ts
🧬 Code graph analysis (2)
apps/meteor/client/lib/utils/legacyJumpToMessage.ts (1)
apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts (1)
RoomHistoryManager(341-341)
apps/meteor/client/views/room/body/hooks/useGetMore.ts (1)
apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts (3)
RoomHistoryManager(341-341)hasMore(260-263)hasMoreNext(265-268)
🔇 Additional comments (8)
packages/model-typings/src/models/IMessagesModel.ts (1)
179-216: LGTM!The interface additions for
showThreadMessages?: booleanon bothfindVisibleByRoomIdAfterTimestampandfindVisibleByRoomIdBeforeTimestampare consistent with existing patterns in the interface (e.g.,findVisibleByRoomIdNotContainingTypesAndUsers,findVisibleByRoomIdBeforeTimestampNotContainingTypes).packages/models/src/models/Messages.ts (2)
892-919: LGTM!The implementation correctly adds the
showThreadMessagesfilter. The$orcondition pattern (tmid: { $exists: false }ORtshow: true) is consistent with other methods in this file (e.g.,findVisibleByRoomIdNotContainingTypesat lines 873-882).
931-958: LGTM!Consistent implementation with
findVisibleByRoomIdAfterTimestamp. The thread filtering logic is identical and correctly applied.apps/meteor/server/methods/loadSurroundingMessages.ts (1)
64-86: LGTM!The
showThreadMessagesparameter is correctly propagated to bothfindVisibleByRoomIdBeforeTimestampandfindVisibleByRoomIdAfterTimestampcalls, maintaining consistent filtering behavior for surrounding messages.apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts (1)
312-312: LGTM!Passing
falseforshowThreadMessagescorrectly ensures that when loading surrounding messages for the main channel view, thread-only messages are excluded. This aligns with the PR objective of preventing main channel scroll position changes when jumping to threaded messages.apps/meteor/client/lib/utils/legacyJumpToMessage.ts (1)
23-31: LGTM!The conditional logic correctly handles the two cases:
- Thread parent messages (
message.tcounttruthy): loads surrounding messages to position the view- Thread reply messages (no
tcount): loads room history if not already loaded, since the thread panel will handle displaying the specific messageThis ensures proper message context is loaded without affecting main channel scroll position.
apps/meteor/client/views/room/body/hooks/useGetMore.ts (2)
28-34: LGTM!The early guards effectively prevent redundant operations:
jumpToRef.currentcheck avoids loading during jump-to-message actionsisLoading(rid)check prevents concurrent load requestsMoving these checks before
getBoundingClientRectis a good optimization that avoids unnecessary layout calculations.
53-56: LGTM!Good defensive check. Since
getMoreis async, the element could become disconnected during the await. CheckingisConnectedbefore callingflushSyncandrestoreScrollprevents operating on stale elements.
aleksandernsilva
force-pushed
the
fix/jump-msg-erratic-scroll
branch
from
173d035 to
614af86
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/lib/utils/legacyJumpToMessage.ts (1)
26-27: Remove the inline implementation comment at Line 26.This comment restates obvious control flow and conflicts with the repository rule for TS/JS implementation files.
Proposed cleanup
- } else if (!RoomHistoryManager.isLoaded(message.rid)) { - // Load room history if room is not loaded + } else if (!RoomHistoryManager.isLoaded(message.rid)) { await RoomHistoryManager.getMore(message.rid); }As per coding guidelines "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/lib/utils/legacyJumpToMessage.ts` around lines 26 - 27, Remove the redundant inline implementation comment that restates control flow above the call to RoomHistoryManager.getMore in legacyJumpToMessage.ts; edit the function containing the call to RoomHistoryManager.getMore(message.rid) to delete the comment line so only the await RoomHistoryManager.getMore(message.rid); statement remains, keeping all surrounding logic and behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/client/lib/utils/legacyJumpToMessage.ts`:
- Around line 26-27: Remove the redundant inline implementation comment that
restates control flow above the call to RoomHistoryManager.getMore in
legacyJumpToMessage.ts; edit the function containing the call to
RoomHistoryManager.getMore(message.rid) to delete the comment line so only the
await RoomHistoryManager.getMore(message.rid); statement remains, keeping all
surrounding logic and behavior unchanged.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.changeset/bright-dots-march.mdapps/meteor/app/ui-utils/client/lib/RoomHistoryManager.tsapps/meteor/client/lib/utils/legacyJumpToMessage.tsapps/meteor/client/views/room/body/hooks/useGetMore.tsapps/meteor/server/methods/loadSurroundingMessages.tspackages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/bright-dots-march.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/room/body/hooks/useGetMore.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/lib/utils/legacyJumpToMessage.tsapps/meteor/app/ui-utils/client/lib/RoomHistoryManager.tspackages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.tsapps/meteor/server/methods/loadSurroundingMessages.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/client/lib/utils/legacyJumpToMessage.tsapps/meteor/app/ui-utils/client/lib/RoomHistoryManager.tsapps/meteor/server/methods/loadSurroundingMessages.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/utils/legacyJumpToMessage.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/client/lib/utils/legacyJumpToMessage.tsapps/meteor/app/ui-utils/client/lib/RoomHistoryManager.tspackages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.tsapps/meteor/server/methods/loadSurroundingMessages.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/client/lib/utils/legacyJumpToMessage.tsapps/meteor/app/ui-utils/client/lib/RoomHistoryManager.tspackages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.tsapps/meteor/server/methods/loadSurroundingMessages.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.tspackages/model-typings/src/models/IMessagesModel.ts
🧬 Code graph analysis (2)
apps/meteor/client/lib/utils/legacyJumpToMessage.ts (2)
apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts (1)
RoomHistoryManager(341-341)apps/meteor/client/lib/utils/goToRoomById.ts (1)
goToRoomById(16-30)
packages/model-typings/src/models/IMessagesModel.ts (1)
packages/ui-contexts/src/index.ts (1)
FindOptions(110-110)
🔇 Additional comments (5)
apps/meteor/client/lib/utils/legacyJumpToMessage.ts (1)
34-35: Good use ofshowThreadMessages = falsefor surrounding fetches.This aligns with the bugfix goal by preventing thread replies from influencing main-channel surrounding-message windows during jump flows.
Also applies to: 40-40
apps/meteor/app/ui-utils/client/lib/RoomHistoryManager.ts (1)
298-313:showThreadMessagespropagation is implemented cleanly.The defaulted parameter and pass-through to
loadSurroundingMessagespreserve existing behavior while enabling caller control.packages/models/src/models/Messages.ts (1)
892-919: Thread-visibility filtering is correctly integrated into timestamp queries.Both before/after methods now expose the same opt-in behavior and apply the expected filter when thread messages are hidden.
Also applies to: 931-958
packages/model-typings/src/models/IMessagesModel.ts (1)
179-184: Interface updates are consistent with model implementation.The optional
showThreadMessagesargument keeps compatibility while exposing the new behavior through typings.Also applies to: 211-216
apps/meteor/server/methods/loadSurroundingMessages.ts (1)
17-17: Server-side threading flag handling looks correct end-to-end.Validation is in place and the
showThreadMessagesvalue is consistently forwarded to both surrounding timestamp queries.Also applies to: 29-33, 65-70, 82-87
KevLehman
requested changes
Feb 27, 2026
This was referenced Mar 2, 2026
aleksandernsilva
force-pushed
the
fix/jump-msg-erratic-scroll
branch
from
92c796f to
7824f57
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/methods.ts (1)
3451-3472: Refactor repeated sleeps in setup into a typed helper.Lines 3454, 3457, 3461, 3464, 3467, and 3470 repeat the same timing primitive. A local helper reduces duplication and clarifies intent.
♻️ Proposed refactor
describe('[`@loadSurroundingMessages`]', () => { let rid: IRoom['_id']; let middleMessage: IMessage; let channelName: string; + const waitForMessageOrdering = (): Promise<void> => new Promise((resolve) => setTimeout(resolve, 50)); before('create room', (done) => { channelName = `methods-test-channel-${Date.now()}`; @@ before('send messages', async () => { await sendMessage({ message: { rid, msg: 'Message 1' } }); - await new Promise((resolve) => setTimeout(resolve, 50)); + await waitForMessageOrdering(); await sendMessage({ message: { rid, msg: 'Message 2' } }); - await new Promise((resolve) => setTimeout(resolve, 50)); + await waitForMessageOrdering(); const msg3 = await sendMessage({ message: { rid, msg: 'Message 3' } }); middleMessage = msg3.body.message; - await new Promise((resolve) => setTimeout(resolve, 50)); + await waitForMessageOrdering(); const threadMsg = await sendMessage({ message: { rid, msg: 'Message 4 (Thread)' } }); - await new Promise((resolve) => setTimeout(resolve, 50)); + await waitForMessageOrdering(); await sendMessage({ message: { rid, msg: 'Message 4.1 (Reply)', tmid: threadMsg.body.message._id } }); - await new Promise((resolve) => setTimeout(resolve, 50)); + await waitForMessageOrdering(); await sendMessage({ message: { rid, msg: 'Message 4.2 (Reply)', tmid: threadMsg.body.message._id } }); - await new Promise((resolve) => setTimeout(resolve, 50)); + await waitForMessageOrdering(); await sendMessage({ message: { rid, msg: 'Message 5' } }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/methods.ts` around lines 3451 - 3472, The test repeats the same "await new Promise(resolve => setTimeout(resolve, 50))" timing primitive multiple times inside the before('send messages') setup; define a small typed helper like wait(ms: number): Promise<void> at the top of the test file (or within the describe scope) and replace each repeated new Promise/setTimeout call with await wait(50) to remove duplication and clarify intent around sendMessage, middleMessage and threadMsg setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/tests/end-to-end/api/methods.ts`:
- Around line 3451-3472: The test repeats the same "await new Promise(resolve =>
setTimeout(resolve, 50))" timing primitive multiple times inside the
before('send messages') setup; define a small typed helper like wait(ms:
number): Promise<void> at the top of the test file (or within the describe
scope) and replace each repeated new Promise/setTimeout call with await wait(50)
to remove duplication and clarify intent around sendMessage, middleMessage and
threadMsg setup.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/methods.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/end-to-end/api/methods.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2026-03-02T16:31:30.612Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39250
File: apps/meteor/tests/end-to-end/api/livechat/07-queue.ts:1084-1094
Timestamp: 2026-03-02T16:31:30.612Z
Learning: In E2E API tests at apps/meteor/tests/end-to-end/api/livechat/, using sleep(1000) after updateSetting() or updateEESetting() calls in test setup hooks is acceptable and intentional to allow omnichannel settings to propagate their side effects.
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2026-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/methods.ts
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/methods.ts (2)
apps/meteor/tests/data/rooms.helper.ts (1)
deleteRoom(98-99)packages/ddp-client/src/legacy/RocketchatSDKLegacy.ts (1)
methodCall(186-188)
🪛 Biome (2.4.4)
apps/meteor/tests/end-to-end/api/methods.ts
[error] 3451-3472: Duplicate before hook found.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (1)
apps/meteor/tests/end-to-end/api/methods.ts (1)
3428-3675: Good coverage forloadSurroundingMessagesthread filtering behavior.This suite validates the key branches (auth, param validation, default windowing, limit handling, and
showThreadMessagestoggles) and directly supports the regression target.
KevLehman
requested changes
Mar 4, 2026
tassoevan
requested changes
Mar 6, 2026
tassoevan
reviewed
Mar 6, 2026
KevLehman
requested changes
Mar 9, 2026
aleksandernsilva
force-pushed
the
fix/jump-msg-erratic-scroll
branch
from
53b4f43 to
168a569
Compare
tassoevan
approved these changes
Mar 9, 2026
KevLehman
approved these changes
Mar 10, 2026
tassoevan
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Open
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Proposed changes (including videos or screenshots)
This PR fixes an issue causing the main channel to scroll when trying to jump to a message outside the viewport.
To address the issue a few adjustments were made:
getSurroundingMessagesis skipped for messages withtmid).RoomHistoryManager.getSurroundingMessagesmethod allows filtering out thread messages. This prevents the scenario where the limit of messages surrounding the main message is taken up by thread messages and improves scroll by giving the appropriate buffer for the lazy loading.legacyJumpToMessagenow callsgetSurroundingMessageswithshowThreadMessages=false.legacyJumpToMessagenow callsRoomHistoryManager.getMorewhen jumping to thread messages if room history is not loaded. This covers the behavior sinceuseGetMoreskips this fetch for rooms being accessed through message jumps.useGetMoreto reduce unnecessary calculations.IMessagesModelinterface and itsMessagesRawimplementation to add ashowThreadMessagesparameter tofindVisibleByRoomIdAfterTimestampandfindVisibleByRoomIdBeforeTimestampmethods. This allows callers to specify whether to include thread messages in results.showThreadMessages = falsewhen querying for messages before and after a given timestamp, ensuring correct message filtering.Issue(s)
CORE-1184
Steps to test or reproduce
Further comments
Note
Due to the async nature of the jump to message and the tendency to generate flaky e2e tests it will be implemented in a separate PR
Note
A sparate refactor PR will be opened to deal with the deprecated meteor method still being used.
https://rocketchat.atlassian.net/browse/CORE-1893
Summary by CodeRabbit
New Features
Improvements
Tests