-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: offline sync queue - reconcile TEMP messages on restart, auto-r… #6964
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…etry ERROR on reconnect - Export changeMessageStatus from sendMessage for reconciliation - Add messageSync.ts: reconcileTempMessages (TEMP older than 5min) and retryErrorMessages - Run reconcileTempMessages on login success (handleLoginSuccess) - Add messageSync saga: retry ERROR messages on METEOR.SUCCESS when user logged in - Register messageSync saga in root
WalkthroughAdds message reconciliation and retry utilities and integrates them into sagas: new Changes
Sequence DiagramsequenceDiagram
actor User
participant Sagas as "Sagas / Redux"
participant LocalDB as "Local DB (messages)"
participant Server as "Server API"
User->>Sagas: Login Success
activate Sagas
Sagas->>LocalDB: reconcileTempMessages()
activate LocalDB
LocalDB->>LocalDB: Query TEMP messages older than 5min
loop per TEMP message
LocalDB->>Server: getSingleMessage(id)
alt Server returns message (exists)
Server-->>LocalDB: message found
LocalDB->>LocalDB: changeMessageStatus(id, SENT)
else Server not found / lookup error
Server-->>LocalDB: not-found / error
LocalDB->>Server: resendMessage()
Server-->>LocalDB: resend result (success/fail)
end
end
deactivate LocalDB
deactivate Sagas
User->>Sagas: METEOR.SUCCESS
activate Sagas
Sagas->>LocalDB: retryErrorMessages()
activate LocalDB
LocalDB->>LocalDB: Query ERROR messages
loop per ERROR message
LocalDB->>Server: resendMessage()
Server-->>LocalDB: resend result (success/fail)
end
deactivate LocalDB
deactivate Sagas
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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. No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/lib/methods/messageSync.ts`:
- Around line 33-49: The outer catch around getSingleMessage treats any error as
"message missing" and triggers resendMessage, which can create duplicates on
transient failures; update the error handling in the try/catch that surrounds
getSingleMessage (and the subsequent changeMessageStatus call) to inspect the
thrown error (e.g. check err.response?.status, err.status, or err.code) and only
call resendMessage(record, record.tmid ?? undefined) when the error indicates
the message is truly not found (e.g. 404 or specific "not found" code); for
other errors (network timeouts, 5xx, auth failures) either rethrow or log and
abort without resending so resendMessage is not invoked erroneously. Ensure you
reference getSingleMessage, changeMessageStatus, resendMessage, messagesStatus
and preserve the inner catch(e) { log(e) } behavior for the actual resend path.
🧹 Nitpick comments (2)
app/sagas/messageSync.js (1)
21-23: Consider usingtakeLatestinstead oftakeEveryto prevent concurrent retries.
METEOR.SUCCESScan fire multiple times during reconnection scenarios. UsingtakeEverycould spawn multiple concurrentretryErrorMessagesSagainstances, potentially causing the same ERROR messages to be retried simultaneously, leading to duplicate sends or race conditions.♻️ Proposed fix
-const root = function* root() { - yield takeEvery(METEOR.SUCCESS, retryErrorMessagesSaga); -}; +import { call, select, takeLatest } from 'redux-saga/effects'; + +const root = function* root() { + yield takeLatest(METEOR.SUCCESS, retryErrorMessagesSaga); +};app/sagas/login.js (1)
251-251: Note: Reconciliation may be cancelled after 2 seconds.The
handleLoginSuccesstask is cancelled after a 2-second timeout (see Lines 426-430). IfreconcileTempMessagesis processing many stuck messages, it may be interrupted mid-execution. This follows the existing pattern for other forked tasks, but consider whether the reconciliation should complete independently.If uninterrupted execution is important, consider spawning instead of forking:
💡 Alternative using spawn for detached execution
- yield fork(reconcileTempMessagesSaga); + yield spawn(reconcileTempMessagesSaga);Note:
spawncreates a detached task that won't be cancelled when the parent is cancelled. You'd need to importspawnfromredux-saga/effects.
|
@deepak0x This is awesome! There's this old issue about the same thing, so add to the What's missing here, since the PR is draft? |
hey @diegolmello |
Only trigger TEMP-message resend when lookup errors indicate message-not-found, preventing duplicate sends during transient network/auth/server failures. Co-authored-by: Cursor <[email protected]>
|
hey @diegolmello |
Removes branch-added inline/doc comments while preserving behavior, and keeps sequential processing via a helper to satisfy lint rules. Co-authored-by: Cursor <[email protected]>
This PR adds a offline/sync flow to ensure messages don’t remain stuck after app restarts, crashes, or network changes.
Stuck TEMP messages: On login, messages that have been in
TEMPstate for more than 5 minutes are reconciled:chat.getMessage), its status is updated toSENT.ERROR.Auto-retry ERROR messages: When the app reconnects (
METEOR.SUCCESS), all messages inERRORstate are automatically retried so users don’t need to manually tap Resend after going back online.** Details**:
messageSync.tswithreconcileTempMessages()andretryErrorMessages().changeMessageStatusfromsendMessage.tsfor reuse.messageSyncsaga that retries ERROR messages on connection success when the user is logged in.Issue(s)
Closes #6928, #4471
TEMPremained in a perpetual Sending… state after app crash or force kill.ERRORstate were never retried automatically after network reconnection.How to Test / Reproduce
Stuck TEMP Messages
Expected result:
The message is reconciled:
SENTif it exists on the server, orERRORif resend fails.Auto-retry ERROR Messages
ERROR).result:
Screenshots/Videos
WhatsApp.Video.2026-02-10.at.04.06.18.mp4
Types of Changes
Checklist
Summary by CodeRabbit