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 pagination in loadMessagesForRoom: repeatedly calls fetchBatch starting from an optional latest timestamp, aggregates all messages across batches, counts non-thread (no tmid) messages to meet COUNT, and stops when criteria are met or a batch is not full. Returns the combined messages array. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Loader as loadMessagesForRoom
participant API as fetchBatch (server)
Client->>Loader: loadMessagesForRoom(roomId, COUNT, latest?)
activate Loader
note right of Loader: Initialize allMessages = [], mainCount = 0<br/>Set start = latest?timestamp : undefined
loop While mainCount < COUNT and last batch was full
Loader->>API: fetchBatch({ roomId, count: COUNT, latest: start })
API-->>Loader: batch { messages[], isFull }
note right of Loader: Append messages to allMessages<br/>Increment mainCount by messages without tmid<br/>Update start to oldest timestamp in batch
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)
✨ 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 |
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.
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
experimental_ios_build
GitHub Actions
Error
OtavioStasiak
had a problem deploying
to
official_ios_build
GitHub Actions
Error
OtavioStasiak
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
OtavioStasiak
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
official_android_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
OtavioStasiak
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
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)
71-83: Critical: "Load more" indicator logic is broken with batching.Line 74 checks
data.length === COUNTto determine if a "load more" message should be added. However, with the new batching logic,datanow contains accumulated messages from multiple API calls, sodata.lengthcan be much larger thanCOUNT(50).This breaks the "load more" indicator: the condition will rarely be true even when more messages exist, preventing users from loading additional history.
Fix: Track whether the last batch was full to determine if more messages exist:
async function load({ rid: roomId, latest, t }: { rid: string; latest?: Date; t: RoomTypes }): Promise<IMessage[]> { const apiType = roomTypeToApiType(t); if (!apiType) { return []; } const allMessages: IMessage[] = []; let mainMessagesCount = 0; + let lastBatchWasFull = false; async function fetchBatch(lastTs?: string): Promise<void> { const params: HistoryParams = { roomId, count: COUNT }; if (lastTs) { params.latest = lastTs; } const data = await sdk.get(`${apiType}.history`, params); if (!data?.success || !data.messages?.length) { return; } const batch = data.messages as IMessage[]; allMessages.push(...batch); const mainMessagesInBatch = batch.filter(message => !message.tmid); mainMessagesCount += mainMessagesInBatch.length; const needsMoreMainMessages = mainMessagesCount < COUNT; const hasMoreMessages = batch.length === COUNT; + lastBatchWasFull = hasMoreMessages; if (needsMoreMainMessages && hasMoreMessages) { const lastMessage = batch[batch.length - 1]; return fetchBatch(lastMessage.ts as string); } } const startTimestamp = latest ? new Date(latest).toISOString() : undefined; await fetchBatch(startTimestamp); - return allMessages; + return { messages: allMessages, hasMore: lastBatchWasFull }; }Then update the caller:
export function loadMessagesForRoom(args: { rid: string; t: RoomTypes; latest?: Date; loaderItem?: TMessageModel; }): Promise<void> { return new Promise(async (resolve, reject) => { try { - const data = await load(args); - if (data?.length) { - const lastMessage = data[data.length - 1]; + const result = await load(args); + if (result?.messages?.length) { + const data = result.messages; + const lastMessage = data[data.length - 1]; const lastMessageRecord = await getMessageById(lastMessage._id as string); - if (!lastMessageRecord && data.length === COUNT) { + if (!lastMessageRecord && result.hasMore) { const loadMoreMessage = {
🧹 Nitpick comments (2)
app/lib/methods/loadMessagesForRoom.ts (2)
29-54: Consider iterative approach to avoid potential stack overflow.The recursive implementation could theoretically cause a stack overflow if many batches are needed (e.g., a room with thousands of thread messages and few main messages). An iterative approach would be safer:
-async function fetchBatch(lastTs?: string): Promise<void> { - const params: HistoryParams = { roomId, count: COUNT }; - if (lastTs) { - params.latest = lastTs; - } - - const data = await sdk.get(`${apiType}.history`, params); - - if (!data?.success || !data.messages?.length) { - return; - } - - const batch = data.messages as IMessage[]; - allMessages.push(...batch); - - const mainMessagesInBatch = batch.filter(message => !message.tmid); - mainMessagesCount += mainMessagesInBatch.length; - - const needsMoreMainMessages = mainMessagesCount < COUNT; - const hasMoreMessages = batch.length === COUNT; - - if (needsMoreMainMessages && hasMoreMessages) { - const lastMessage = batch[batch.length - 1]; - return fetchBatch(lastMessage.ts as string); - } -} - -const startTimestamp = latest ? new Date(latest).toISOString() : undefined; -await fetchBatch(startTimestamp); +let lastTs = latest ? new Date(latest).toISOString() : undefined; + +while (true) { + const params: HistoryParams = { roomId, count: COUNT }; + if (lastTs) { + params.latest = lastTs; + } + + const data = await sdk.get(`${apiType}.history`, params); + + if (!data?.success || !data.messages?.length) { + break; + } + + const batch = data.messages as IMessage[]; + allMessages.push(...batch); + + const mainMessagesInBatch = batch.filter(message => !message.tmid); + mainMessagesCount += mainMessagesInBatch.length; + + const needsMoreMainMessages = mainMessagesCount < COUNT; + const hasMoreMessages = batch.length === COUNT; + + if (!needsMoreMainMessages || !hasMoreMessages) { + break; + } + + const lastMessage = batch[batch.length - 1]; + lastTs = lastMessage.ts as string; +}
50-53: Convert timestamp to ISO string rather than casting. ReplacefetchBatch(lastMessage.ts as string)with
fetchBatch(new Date(lastMessage.ts).toISOString())to safely handle both
string | Dateforts.
📜 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/lib/methods/roomTypeToApiType.ts (2)
RoomTypes(7-7)roomTypeToApiType(26-26)app/definitions/IMessage.ts (1)
IMessage(126-156)
⏰ 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). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (3)
app/lib/methods/loadMessagesForRoom.ts (3)
14-18: LGTM! Clear interface definition.The
HistoryParamsinterface accurately represents the pagination parameters for the API request.
26-27: LGTM! Proper initialization for batch accumulation.The variables are correctly initialized to accumulate messages across multiple API calls and track main (non-thread) messages separately.
56-59: LGTM! Correct initiation and return of batched results.The function properly initiates the recursive batch fetching with an optional start timestamp and returns all accumulated messages.
There was a problem hiding this comment.
Shouldn't we make something similar on loadMissedMessages?
loadMessagesForRoom runs on the first room open (if there's no messages loaded, but sub.lastMessage already breaks this rule since it's saved on messages table) and then it loads on scroll.
I think loadMissedMessages is triggered a lot more often, since it runs every time we open the room.
When we reproduce the error on those internal channels, aren't we basically calling loadMissedMessages?
I could be wrong though, but I'd like to hear from you 😬
I'm missing e2e tests, so it never happens again (it's an edge case logic)
We can make a logic similar to room from jump to message tests. It's not supposed to create any new messages or add users to the room. We just login as a user that's already in the room and navigate. The user should be able to read the oldest message.
| if (latest) { | ||
| params = { ...params, latest: new Date(latest).toISOString() }; | ||
| } | ||
| interface HistoryParams { |
There was a problem hiding this comment.
There's this non-written convention
interface: ISomething
type: TSomething
That said, it's only working because sdk.get accepts anything, because it should really check definitions/rest/v1
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