Repository navigation
chore: migrate loadNextMessages and loadSurroundingMessages methods to rooms.history rest - #7793
Conversation
The loadNextMessages DDP method is removed in server 9.0.0. On servers >= 8.9.0 the Newer Loader now loads through GET /v1/rooms.history; older servers keep the DDP call. An empty response removes the Newer Loader row instead of leaving it stuck.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
WalkthroughThe change adds the ChangesRoom message history
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Other Sequence Diagram(s)sequenceDiagram
participant Loader as Message loader
participant Retrieval as History retrieval service
participant REST as rooms.history
participant DDP as DDP methods
Loader->>Retrieval: Request messages with room and pagination parameters
alt Server version 8.9.0 or later
Retrieval->>REST: Request room history
REST-->>Retrieval: Messages and cursors
else Older server
Retrieval->>DDP: Call legacy history method
DDP-->>Retrieval: EJSON response
end
Retrieval-->>Loader: Messages and pagination data
Suggested labels: Suggested reviewers: Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 13 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: migrating both message-loading methods to the rooms.history REST endpoint.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Errors were encountered while retrieving linked issues.
Errors: 1
-
- JIRA integration encountered authorization issues. Please disconnect and reconnect the integration in the CodeRabbit UI.
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 help to get the list of available commands.
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
…dating updateMessages compares stored thread rows against the incoming _updatedAt with <, which is always false for an ISO string. The DDP reply carried Dates, so the REST path stopped updating existing threads and thread_messages rows from the Newer Loader.
GitHub Actions
Error
GitHub Actions
Error
updateMessages compares stored thread rows against the incoming _updatedAt with <, which is always false for an ISO string. Convert it where the message is normalized instead of in getNextMessages, so every REST loader feeding updateMessages gets the comparison right. Dates, numbers and a missing value are left as they were.
GitHub Actions
Error
GitHub Actions
Error
…real buildMessage The existing thread tests mock buildMessage as identity and use numeric _updatedAt, so they pass with or without the Date conversion. Run the thread and thread message update paths through the real buildMessage with a stored Date and an incoming ISO string.
GitHub Actions
Error
GitHub Actions
Error
…lizeMessage The rooms.history response carries ISO strings, which never compare as newer in the thread branches of updateMessages. Converting in the REST branch of the wrapper keeps normalizeMessage, loadMessagesForRoom and loadMissedMessages unchanged, so those loaders do not start refreshing stored thread rows and overwriting their downloaded file:// title_link.
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
… CONTEXT.md terms in test titles
|
@coderabbitai review |
✅ Action performedReview finished.
|
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
… fetch same-second messages
GitHub Actions
Error
GitHub Actions
Error
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Maintainability review. I found no release blockers: I checked the rooms.history contract against the server source. The 8.9.0 gate matches the release that shipped aroundId. The next cursor is an exclusive millisecond timestamp, so loaderTs - 1 neither skips nor duplicates messages. cursor.next/cursor.previous map to newer/older on both the next and aroundId paths. The jump target appears only once.
The comments below are structural. Most are one change seen from different angles. Return one result shape from the history adapters and build loader rows in one factory, and both loaders shrink to fetch → sort → add loaders → store.
| messages.push(loadMoreItem); | ||
| } | ||
| } | ||
| if (moreBefore) { |
There was a problem hiding this comment.
The loader-row literal { _id: generateLoadMoreId(anchor._id), rid, ts: dayjs(anchor.ts) ±1ms .toDate(), t, msg } as IMessage now appears 5 times: twice here, once in loadNextMessages, twice in loadMessagesForRoom. The copies have already drifted (the loadNextMessages one leaves out msg). Each type implies its offset: MORE/PREVIOUS_CHUNK are −1ms and NEXT_CHUNK is +1ms. Could we add a buildLoaderRow(anchor, type) next to generateLoadMoreId? That also removes the five as IMessage casts.
Once getNextMessages returns the same shape as this adapter (see the comment on restApi.ts), the two more* && !(await getMessageById(...)) blocks can become a single shared withChunkLoaders(messages, { moreBefore, moreAfter }) that both loaders call.
| }); | ||
| const lastMessage = messages[messages.length - 1]; | ||
| if (hasMore && !(await getMessageById(lastMessage._id))) { | ||
| messages.push({ |
There was a problem hiding this comment.
Same loader literal as in loadSurroundingMessages, minus msg. One factory would stop this drift. Separately, ILoadMoreMessage in app/definitions/IMessage.ts declares ts: string, which is wrong now that every loader stores a Date. Either have the factory return that type (with ts: Date) or delete it.
| try { | ||
| const { messages: fetched, hasMore } = await getNextMessages({ rid: args.rid, after: args.ts, count: COUNT }); | ||
| const messages = orderBy(fetched, 'ts'); | ||
| if (!messages.length) { |
There was a problem hiding this comment.
This branch only exists because updateMessages returns early when update and remove are both empty, even when a loaderItem was passed. If we add loaderItem to that guard, the branch goes away: updateMessages({ rid, update: messages, loaderItem }) already handles the empty case, and every caller gets the "remove an empty loader" behavior, not just this one.
| return { messages: messages as IMessage[], cursor: history.cursor }; | ||
| }; | ||
|
|
||
| export const getNextMessages = async ({ rid, after, count }: { rid: string; after: Date; count: number }) => { |
There was a problem hiding this comment.
getNextMessages and getSurroundingMessages duplicate each other: same version gate, same DDP fallback (methodCallWrapper → null check → throw → EJSON.fromJSONValue), but different return shapes ({ messages, hasMore } vs { messages, moreBefore, moreAfter }). That shape mismatch is what keeps the two loaders from sharing code. Suggestion: have both return { messages, moreBefore, moreAfter }. On the DDP next path that's moreBefore: false, moreAfter: messages.length === count.
On placement: only getRoomHistory is a REST wrapper. The transport switch, DDP calls, EJSON decoding and cursor interpretation are chunk-loading policy, and restApi.ts is already over 1.3k lines. A small roomHistory module with one supportsRoomsHistory() gate and one fetchHistoryChunk(rid, position) would keep the whole feature in one file.
| export const getNextMessages = async ({ rid, after, count }: { rid: string; after: Date; count: number }) => { | ||
| const serverVersion = reduxStore.getState().server.version; | ||
| if (compareServerVersion(serverVersion, 'greaterThanOrEqualTo', '8.9.0')) { | ||
| const newestLoadedTs = after.getTime() - 1; |
There was a problem hiding this comment.
This - 1 undoes the + 1ms that loadNextMessages applies when it stamps the Newer Loader. That's correct, since the server's next cursor is exclusive. But the two halves are in different files with nothing linking them, and each file's tests only check its own half. If someone changes either offset alone, Load more silently skips or duplicates a message. The parameter name after is also misleading, because the value is one ms past the newest loaded message. Could the offset and its inverse live together in the loader-row helper, or could this take { newestLoadedTs } directly?
| throw new Error('Unable to load room history'); | ||
| } | ||
| const messages = history.messages.map(message => ({ ...message, _updatedAt: new Date(message._updatedAt) })); | ||
| return { messages: messages as IMessage[], cursor: history.cursor }; |
There was a problem hiding this comment.
Only _updatedAt is converted here: ts stays an ISO string, and then everything is cast to IMessage[]. The DDP path returns Date for both fields, so the same return type carries different runtime types depending on the transport. It works today because dayjs and orderBy accept ISO strings. Could we either normalize ts too and drop the cast, or name the mapper after what it guarantees (for example, thread rows need a Date _updatedAt)?
| }) | ||
| ); | ||
|
|
||
| it('stamps the trailing Older Loader one millisecond before the oldest Message of the Chunk', async () => { |
There was a problem hiding this comment.
Minor: these new tests go into a file outside __tests__, but this PR already moved loadNextMessages.test.ts into app/lib/methods/__tests__/. Could this file move too?
GitHub Actions
Waiting
* chore: migrate getThreadMessages ddp method to chat.getThreadMessages rest api (#7773) * chore: migrate loadNextMessages and loadSurroundingMessages methods to rooms.history rest (#7793) * chore: Bump version to 4.77.2 * chore: migrate DDP methods to REST endpoints (#7783) --------- Co-authored-by: Rohit Bansal <40559587+Rohit3523@users.noreply.github.com>
GitHub Actions
Waiting
GitHub Actions
Waiting
* chore: migrate getThreadMessages ddp method to chat.getThreadMessages rest api (#7773) * chore: migrate loadNextMessages and loadSurroundingMessages methods to rooms.history rest (#7793) * chore: Bump version to 4.77.2 * chore: migrate DDP methods to REST endpoints (#7783) --------- Co-authored-by: Rohit Bansal <40559587+Rohit3523@users.noreply.github.com>
This branch is waiting to be deployed
2 participants
Proposed changes
The
loadNextMessagesandloadSurroundingMessagesDDP methods are deprecated and the server removes them in 9.0.0, which would break Jump to Message and the Newer Loader. On servers 8.9.0 and above both now useGET /v1/rooms.history(nextandaroundId), and older servers keep the DDP calls. A Newer Loader whose next page comes back empty is now removed instead of staying stuck, on both transports.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1726
https://rocketchat.atlassian.net/browse/NATIVE-1752
How to test or reproduce
m001tom120) while the app is closed, and post a link tom003from another room using the server's Site_Urlm027m001tom027, then rejoins the live tail with no loader row leftScreenshots
N/A
Types of changes
Checklist
Further comments
On 8.9.0 and above the endpoint is requested with
showThreadMessages: true, so Thread Messages are stored exactly as the DDP methods stored them, and the room list shows the same messages either way because it hides Thread Messages that weren't also sent to the channel (loadMessagesForRoomkeepsfalse, which is out of scope here). It skips the server's hidden system message types, so those system messages are no longer stored by these two paths. If the message to jump to can't be loaded, the jump is cancelled on every server version instead of falling back to the message's timestamp:rooms.historyanswers 404 (not visible) or 403 (nopreview-c-room) on 8.9.0 and above, and the DDP method returnsfalsebelow it. A DDP reply withoutmessagesnow rejects in both loaders instead of resolving empty; the Newer Loader row stays as before and the error is logged. Tested on the Android emulator against a local 8.10 server (REST) and mobile.qa 8.4 (DDP). The empty page was produced by stubbing the response, and iOS was not tested.Summary by CodeRabbit