fix: cannot load previous messages when lastMessage thread exceeds 50 messages #6680
Conversation
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughImplements batched history loading in loadMessagesForRoom: repeatedly calls fetchBatch across channel/group/IM endpoints, aggregates messages into an allMessages array, counts non-thread messages until COUNT or an overall cap, and returns the combined results. Adds a Maestro UI test for a last-message thread with >50 messages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Loader as loadMessagesForRoom
participant API_channels as API (channels)
participant API_groups as API (groups)
participant API_im as API (im)
Client->>Loader: loadMessagesForRoom(roomId, COUNT, latest?)
activate Loader
note right of Loader #DDEBF7: init allMessages=[], mainCount=0, start=latest?timestamp:null
loop while mainCount < COUNT and not terminated
alt apiType == channels
Loader->>API_channels: fetchBatch({roomId, count, latest: start})
API_channels-->>Loader: batch(messages[], metadata)
else apiType == groups
Loader->>API_groups: fetchBatch({roomId, count, latest: start})
API_groups-->>Loader: batch(messages[], metadata)
else apiType == im
Loader->>API_im: fetchBatch({roomId, count, latest: start})
API_im-->>Loader: batch(messages[], metadata)
end
note right of Loader #F7F3DD: append messages to allMessages\nincrement mainCount by messages without tmid\nupdate start to oldest timestamp\ncheck cap (COUNT * 10) or no more main messages
end
Loader-->>Client: allMessages (aggregated)
deactivate Loader
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
|
iOS Build Available Rocket.Chat Experimental 4.65.0.107410 |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
OtavioStasiak
changed the title
|
iOS Build Available Rocket.Chat Experimental 4.65.0.107411 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/methods/loadMessagesForRoom.ts (1)
45-64: Fix regression: load-more sentinel never created
uniqueTmidsis always truthy (arrays never coerce to falsy), so!uniqueTmidsis now permanentlyfalse. That prevents us from ever pushing the synthetic load-more record, which in turn blocks pagination for rooms that hit the 50-message batch size—the very scenario this PR is addressing. Please gate on the array length instead so the sentinel is emitted when there are no thread replies, and tighten the later guard likewise.- const uniqueTmids = [...new Set(data.map(m => m.tmid).filter(Boolean))]; - if (!lastMessageRecord && data.length === COUNT && !uniqueTmids) { + const uniqueTmids = [...new Set(data.map(m => m.tmid).filter(Boolean))]; + if (!lastMessageRecord && data.length === COUNT && uniqueTmids.length === 0) { const loadMoreMessage = { _id: generateLoadMoreId(lastMessage._id as string), rid: lastMessage.rid, ts: moment(lastMessage.ts).subtract(1, 'millisecond').toString(), t: MessageTypeLoad.MORE, msg: lastMessage.msg } as IMessage; data.push(loadMoreMessage); } const onlyThreadMessages = !data.find(item => !item.tmid); - if (uniqueTmids && onlyThreadMessages) { + if (uniqueTmids.length > 0 && onlyThreadMessages) {
🧹 Nitpick comments (1)
app/views/RoomView/List/hooks/useMessages.ts (1)
98-100: Remove leftover debug logThis
console.logwill spam every subscription tick in production. Please drop it before merging.- console.log('here');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/lib/methods/loadMessagesForRoom.ts(2 hunks)app/lib/methods/loadPreviousMessages.ts(1 hunks)app/views/RoomView/List/hooks/useMessages.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/lib/methods/loadPreviousMessages.ts (4)
app/lib/database/services/Subscription.ts (1)
getSubscriptionByRoomId(8-17)app/lib/store/auxStore.ts (1)
store(6-6)app/lib/methods/helpers/compareServerVersion.ts (1)
compareServerVersion(10-15)app/lib/methods/updateMessages.ts (1)
updateMessages(20-214)
app/lib/methods/loadMessagesForRoom.ts (2)
app/containers/message/interfaces.ts (1)
IMessage(116-129)app/definitions/IMessage.ts (1)
IMessage(126-156)
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/views/RoomView/List/hooks/useMessages.ts (4)
89-89: Consider using a Set for improved performance.The
includes()check has O(n) complexity for each element, resulting in O(n²) overall complexity. For large message lists, this could impact performance.Apply this diff to use a Set for O(n) complexity:
- const addedIds = messagesIds.current?.filter(id => !prevIds.includes(id)); + const prevIdsSet = new Set(prevIds); + const addedIds = messagesIds.current?.filter(id => !prevIdsSet.has(id));
93-93: Remove type assertion for better type safety.The
as anycast bypasses TypeScript's type checking. Consider using proper typing or a type guard instead to maintain type safety.If the
TAnyMessageModeltype doesn't include thetsproperty in its type definition, consider:
- Updating the type definition to include
ts- Using a type guard to narrow the type
- Accessing the property in a type-safe manner
Example using type guard:
const tsNumbers = newMessages.map(m => { if ('ts' in m && m.ts instanceof Date) { return m.ts.getTime(); } return undefined; }).filter((ts): ts is number => ts !== undefined);
98-98: Add error handling around loadPreviousMessages call.If
loadPreviousMessagesthrows an error, it could crash the subscription handler and prevent further message updates. Consider adding try-catch to handle potential failures gracefully.Apply this diff to add error handling:
if (oldestTsNumber) { - await loadPreviousMessages({ rid, lastOpen: new Date(oldestTsNumber) }); + try { + await loadPreviousMessages({ rid, lastOpen: new Date(oldestTsNumber) }); + } catch (error) { + // Log error for debugging but don't break the subscription + console.error('Failed to load previous messages:', error); + } }
88-100: Consider adding a comment to explain the logic.The logic for detecting when to load previous messages (when no new IDs are added) is not immediately obvious. A brief comment would help future maintainers understand the intent and how this fixes the thread message loading issue.
Example comment:
// If no new messages were added to the local database, it means we've reached // the limit of locally cached messages. This can happen when the last message // is a thread with >50 messages. In this case, we need to fetch more messages // from the server using the oldest timestamp as the reference point.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/views/RoomView/List/hooks/useMessages.ts(4 hunks)
⏰ 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: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (3)
app/views/RoomView/List/hooks/useMessages.ts (3)
12-12: LGTM!The import is correctly added to support the new functionality.
36-36: LGTM!Capturing the previous IDs before state update enables proper detection of newly added messages in the subscription handler.
71-71: LGTM!Converting the subscription handler to async is necessary to support the
await loadPreviousMessagescall introduced later in this function.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/lib/methods/loadMessagesForRoom.ts (1)
23-61: Fix type safety issue when passing timestamp to recursive call.The batching logic correctly addresses the bug where threads exceeding 50 messages prevent loading previous messages. However, line 59 has a type safety issue that was flagged in previous reviews.
The
lastMessage.tsfield can be eitherstringorDateaccording to theIMessageinterface, but it's being cast tostringwithout runtime validation. Iftsis aDateobject, the API call will receive an invalid parameter value.Apply this diff to handle both types safely:
if (needsMoreMainMessages) { const lastMessage = batch[batch.length - 1]; - await fetchBatch(lastMessage.ts as string); + const lastTs = lastMessage.ts instanceof Date ? lastMessage.ts.toISOString() : lastMessage.ts; + await fetchBatch(lastTs); }Based on learnings from previous review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/lib/methods/loadMessagesForRoom.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/lib/methods/loadMessagesForRoom.ts (2)
app/containers/message/interfaces.ts (1)
IMessage(116-129)app/definitions/IMessage.ts (1)
IMessage(126-156)
🪛 Biome (2.1.2)
app/lib/methods/loadMessagesForRoom.ts
[error] 68-68: This code will never be reached ...
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 64-64: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 65-65: Shouldn't redeclare 'startTimestamp'. Consider to delete it or rename it.
'startTimestamp' is defined here:
(lint/suspicious/noRedeclare)
🪛 ESLint
app/lib/methods/loadMessagesForRoom.ts
[error] 65-65: 'startTimestamp' is already defined.
(no-redeclare)
[error] 68-68: Unreachable code.
(no-unreachable)
🔇 Additional comments (1)
app/lib/methods/loadMessagesForRoom.ts (1)
20-22: LGTM! Clear variable declarations for batching logic.The
allMessagesaccumulator andmainMessagesCountcounter are well-named and support the batched message loading strategy to handle threads exceeding 50 messages.
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
OtavioStasiak
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
There was a problem hiding this comment.
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)
app/lib/methods/loadMessagesForRoom.ts (1)
80-80: Update the condition to account for the new batching logic.The condition
data.length === COUNTassumes theload()function returns at mostCOUNTmessages. However, the new batching implementation can return up toCOUNT * 10messages, so this condition will rarely be true, potentially preventing the "load more" message from being added even when more messages exist in history.Consider changing the condition to check if we fetched at least
COUNTmessages:-if (!lastMessageRecord && data.length === COUNT) { +if (!lastMessageRecord && data.length >= COUNT) {Alternatively, modify the
load()function to return metadata indicating whether more messages are available (e.g., return{ messages: IMessage[], hasMore: boolean }), though this would require a larger refactoring.Do you want me to help implement a more robust solution that tracks whether we've reached the end of history?
♻️ Duplicate comments (1)
app/lib/methods/loadMessagesForRoom.ts (1)
59-59: HandleDate | stringtype at runtime instead of casting.The
tsfield can beDate | stringper theIMessagetype definition (seeapp/definitions/IMessage.ts). Simply casting tostringwithout conversion will pass aDateobject to the API iftsis aDate, causing incorrect pagination.This issue was flagged in previous reviews but remains unaddressed.
Apply this diff to add runtime type checking:
if (needsMoreMainMessages) { const lastMessage = batch[batch.length - 1]; - await fetchBatch(lastMessage.ts as string); + const lastTs = lastMessage.ts instanceof Date ? lastMessage.ts.toISOString() : lastMessage.ts; + await fetchBatch(lastTs); }
🧹 Nitpick comments (1)
app/lib/methods/loadMessagesForRoom.ts (1)
55-60: Consider checkingbatch.length < COUNTto avoid redundant API calls.When
batch.length < COUNT, it indicates the API has returned fewer messages than requested, meaning we've likely reached the end of message history. Recursing in this case will result in an extra API call that returns empty results.Apply this diff to optimize the recursion condition:
-const needsMoreMainMessages = mainMessagesCount < COUNT; +const needsMoreMainMessages = mainMessagesCount < COUNT && batch.length === COUNT; if (needsMoreMainMessages) { const lastMessage = batch[batch.length - 1]; const lastTs = lastMessage.ts instanceof Date ? lastMessage.ts.toISOString() : lastMessage.ts; await fetchBatch(lastTs); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/lib/methods/loadMessagesForRoom.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/lib/methods/loadMessagesForRoom.ts (2)
app/containers/message/interfaces.ts (1)
IMessage(116-129)app/definitions/IMessage.ts (1)
IMessage(126-156)
🔇 Additional comments (2)
app/lib/methods/loadMessagesForRoom.ts (2)
24-26: LGTM! Safety limit prevents infinite recursion.The
COUNT * 10limit (500 messages) provides a safety net against edge cases where every batch contains only thread replies, which was raised in past reviews. This is a sensible upper bound.
23-61: LGTM! Batching logic correctly solves the stated problem.The recursive batching strategy successfully addresses the issue where the last message is a thread with >50 messages:
- Accumulates messages across multiple API calls until
COUNTmain (non-thread) messages are gathered- Properly handles different room types (channels, groups, IM) via the switch statement
- Terminates when sufficient main messages are collected or safety limits are reached
OtavioStasiak
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.66.0.107570 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTNmvO7jsoIXxrl6jKxQMIF5GtqA2LJa0T1jgqtgkcq2F9jWEtCB1cnqVs2FhlxQdG-stcQua0bBuC_Sz_D |
|
iOS Build Available Rocket.Chat Experimental 4.66.0.107571 |
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
deployed
to
experimental_ios_build
GitHub Actions
Active
OtavioStasiak
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_android_build
GitHub Actions
Error
|
iOS Build Available Rocket.Chat Experimental 4.66.0.107584 |
OtavioStasiak
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
official_android_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
| break; | ||
| default: | ||
| return; | ||
| } |
There was a problem hiding this comment.
Did you intentionally rewrote this?
Don't you think the old version is clear enough?
const data = await sdk.get(`${apiType}.history`, params);
| import { generateLoadMoreId } from './helpers/generateLoadMoreId'; | ||
|
|
||
| const COUNT = 50; | ||
|
|
There was a problem hiding this comment.
| const COUNT_LIMIT = COUNT * 10; |
I think we do this and apply it on both places below.
2 participants
Proposed changes
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1035
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit