fix: add retry logic to loadThreadMessages to prevent missing thread …#7016
fix: add retry logic to loadThreadMessages to prevent missing thread …#7016deepak0x wants to merge 4 commits intoRocketChat:developfrom
Conversation
…messages When loadThreadMessages fails (network error, timeout), it silently returns [] causing the thread view to show only a subset of messages. The web client shows all messages correctly for the same thread. This adds retry logic (3 attempts with increasing backoff) and propagates errors to the caller (RoomView.init) which has its own retry mechanism, ensuring thread messages are reliably loaded.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📜 Recent review details🔇 Additional comments (1)
WalkthroughReplaces a single call to fetch thread messages with a retry loop (MAX_ATTEMPTS = 4) that awaits each attempt, returns parsed EJSON on success, returns Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/methods/loadThreadMessages.ts`:
- Around line 14-31: Remove the unreachable trailing "return []" after the retry
loop in loadThreadMessages; the for-loop either returns a value on success or
re-throws on the final failed attempt, so delete the extraneous return to avoid
dead code (references: loadThreadMessages, MAX_RETRIES, the for (let attempt...)
retry loop and its try/catch).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/lib/methods/loadThreadMessages.ts
📜 Review details
🧰 Additional context used
🪛 ESLint
app/lib/methods/loadThreadMessages.ts
[error] 18-18: Unexpected await inside a loop.
(no-await-in-loop)
[error] 25-25: Unexpected await inside a loop.
(no-await-in-loop)
🔇 Additional comments (1)
app/lib/methods/loadThreadMessages.ts (1)
34-95: LGTM on the error propagation integration.The outer function correctly catches errors thrown from
load()after retry exhaustion and propagates them viareject(e). This enables the caller (RoomView.init) to handle the failure appropriately, which aligns with the PR objective.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/methods/loadThreadMessages.ts`:
- Around line 14-25: The loop currently sets MAX_RETRIES = 3 which yields 3
total attempts; to get the required "initial try + 3 retries" with
500/1000/1500ms backoff, update MAX_RETRIES to 4 in loadThreadMessages.ts (so
the for loop for (let attempt = 1; attempt <= MAX_RETRIES; attempt++) will run 4
times) and keep the existing delay logic (await new Promise(resolve =>
setTimeout(resolve, attempt * 500))) so retries occur with 500ms, 1000ms, 1500ms
delays after the initial failure of sdk.methodCallWrapper('getThreadMessages', {
tmid }).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/lib/methods/loadThreadMessages.ts
📜 Review details
🧰 Additional context used
🪛 ESLint
app/lib/methods/loadThreadMessages.ts
[error] 18-18: Unexpected await inside a loop.
(no-await-in-loop)
[error] 25-25: Unexpected await inside a loop.
(no-await-in-loop)
🔇 Additional comments (1)
app/lib/methods/loadThreadMessages.ts (1)
18-22: Verify whethermethodCallWrapperthrows on transient failures or returns null/undefined.The code exhibits an asymmetry: exceptions in the DDP/REST call trigger retries (lines 23–28), but if
methodCallWrapperreturns a falsy value (null/undefined), the function returns[]immediately at line 20 without retrying. This is problematic only ifmethodCallWrapperuses null/undefined to signal transient failures rather than throwing an exception.The current code structure treats falsy as a valid "no messages" response, and the outer function also handles empty arrays gracefully (line 37). However, without confirmation of
methodCallWrapper's actual contract—specifically whether it throws on transient failures or returns falsy—it cannot be determined if this is a real failure-handling gap or intentional design. Check the SDK or server-side implementation to clarify whether transient failures result in exceptions (which would be caught and retried) or falsy returns (which currently bypass retries).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/methods/loadThreadMessages.ts (1)
14-32: Retry implementation looks good.The retry logic is well-structured with appropriate exponential backoff. The ESLint rule disable is justified here since sequential awaits are required for retry semantics.
Minor observation: The function lacks an explicit return type. Since it can return
[],EJSON.fromJSONValue(result)(which isany), or throw, consider adding a return type annotation for clarity and type safety.,
💡 Optional: Add explicit return type
-async function load({ tmid }: { tmid: string }) { +async function load({ tmid }: { tmid: string }): Promise<TThreadMessageModel[]> {This would require ensuring
EJSON.fromJSONValue(result)returns the expected type, potentially with a type assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/loadThreadMessages.ts` around lines 14 - 32, The function loadThreadMessages should declare an explicit return type; add a return type annotation to its signature (e.g., Promise<MessageType[]>) and ensure the call to EJSON.fromJSONValue(result) is typed or asserted to that same type (or map/validate the result to MessageType[]), so the function consistently returns the annotated array type or throws; update any imports/types (e.g., MessageType) and adjust the sdk.methodCallWrapper generic/typing if needed to propagate the correct type to EJSON.fromJSONValue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/lib/methods/loadThreadMessages.ts`:
- Around line 14-32: The function loadThreadMessages should declare an explicit
return type; add a return type annotation to its signature (e.g.,
Promise<MessageType[]>) and ensure the call to EJSON.fromJSONValue(result) is
typed or asserted to that same type (or map/validate the result to
MessageType[]), so the function consistently returns the annotated array type or
throws; update any imports/types (e.g., MessageType) and adjust the
sdk.methodCallWrapper generic/typing if needed to propagate the correct type to
EJSON.fromJSONValue.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/lib/methods/loadThreadMessages.ts
📜 Review details
🔇 Additional comments (1)
app/lib/methods/loadThreadMessages.ts (1)
35-38: Error propagation is now correctly handled.The
try/catchblock properly catches errors thrown byload()after retry exhaustion and rejects the promise, allowing callers (e.g.,RoomView.init) to handle failures. This addresses the PR objective of preventing silent data loss.
1 participant
When
loadThreadMessagesfails (due to network error, timeout, or server error), theload()function silently catches the error and returns an empty array[].This causes the thread view in the native app to display only a subset of messages.
Specifically, only messages that were:
tshow: true/ "Also send to channel")The web client does not have this issue because it uses a different fetching strategy with proper error handling.
Cause
The
load()function contains a silent catch block:loadThreadMessagesare swallowed[]is returnedThis makes the failure invisible and results in incomplete thread data.
Closes #6311
Proposed changes
This PR improves reliability by:
Adding retry logic
Propagating errors after retries fail
[], the error is thrownRoomView.init()) to trigger its own retry mechanismRemoving silent error swallowing
catch { return []; }How to Test / Reproduce
Go to Settings > Preferences > Also send thread message to channel behavior.
Choose "Selected by default".
Create a thread message.
Reply in that thread with "Also send to channel" enabled.
Reply in that thread with "Also send to channel" disabled.
Repeat with other preference settings:
Compare the thread view in the native app vs the web version.
Before Fix
getThreadMessagescall fails.After Fix
Screenshots
N/A — No UI changes. This is a data-fetching reliability improvement.
Type of Change
Checklist
Summary by CodeRabbit