Fix offline persisted drafts not cleared on logout#6379
Fix offline persisted drafts not cleared on logout#6379VelikovPetar merged 3 commits intodevelopfrom
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
|
WalkthroughThe PR extends the offline message repository's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/offline/repository/domain/message/internal/DatabaseMessageRepository.kt (1)
166-172:⚠️ Potential issue | 🟡 MinorSerialize draft insert/delete operations with the same mutex used by
clear().Draft writes at lines 166–172 bypass
dbMutex, whileclear()(line 263) usesdbMutex.withLockto delete drafts. During logout,userScope.cancelChildren()is called beforerepositoryFacade.clear(), but cooperative cancellation does not guarantee that in-flight draft saves will be interrupted. A draft write that completes after cancellation is requested but beforedeleteAllDrafts()executes will survive into the next session. WrapinsertDraftMessage()anddeleteDraftMessage()with the samedbMutexasclear()for consistency and safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/offline/repository/domain/message/internal/DatabaseMessageRepository.kt` around lines 166 - 172, insertDraftMessage and deleteDraftMessage perform draft writes without using the same dbMutex used by clear(), which can let drafts survive logout; wrap both functions' bodies in dbMutex.withLock (the same mutex used by clear()) so insertDraftMessage (including insertMessage call for replyMessage and messageDao.insertDraftMessages(message.toEntity())) and deleteDraftMessage (messageDao.deleteDraftMessage(message.id)) are executed under the mutex to serialize with clear()/deleteAllDrafts() and prevent race conditions during cancellation.
🧹 Nitpick comments (1)
stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/offline/repository/MessageRepositoryTests.kt (1)
221-240: Add a behavior-level regression for persisted drafts.This mock test verifies the DAO call, but it would not catch a table/query mismatch or a logout/offline boundary regression. Consider adding a small fake or Room-backed test that inserts a draft, runs the clear/logout path, and asserts
selectDraftMessages()is empty. Based on learnings, “Exercise chat flows with provided fakes fromstream-chat-android-testand ensure offline ↔ client boundaries are covered.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/offline/repository/MessageRepositoryTests.kt` around lines 221 - 240, Add a behavior-level regression test that uses a real/fake Room-backed storage (e.g., provided fakes from stream-chat-android-test) to persist a draft message, then call DatabaseMessageRepository.clear() and assert that the persisted drafts are truly removed by querying the DAO (e.g., selectDraftMessages() returns empty) rather than only verifying mocks; locate the repository usage in DatabaseMessageRepository and the DAO methods deleteAllDrafts() / selectDraftMessages() to implement the integration-style test exercising offline ↔ client logout/clear boundary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/offline/repository/domain/message/internal/DatabaseMessageRepository.kt`:
- Around line 166-172: insertDraftMessage and deleteDraftMessage perform draft
writes without using the same dbMutex used by clear(), which can let drafts
survive logout; wrap both functions' bodies in dbMutex.withLock (the same mutex
used by clear()) so insertDraftMessage (including insertMessage call for
replyMessage and messageDao.insertDraftMessages(message.toEntity())) and
deleteDraftMessage (messageDao.deleteDraftMessage(message.id)) are executed
under the mutex to serialize with clear()/deleteAllDrafts() and prevent race
conditions during cancellation.
---
Nitpick comments:
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/offline/repository/MessageRepositoryTests.kt`:
- Around line 221-240: Add a behavior-level regression test that uses a
real/fake Room-backed storage (e.g., provided fakes from
stream-chat-android-test) to persist a draft message, then call
DatabaseMessageRepository.clear() and assert that the persisted drafts are truly
removed by querying the DAO (e.g., selectDraftMessages() returns empty) rather
than only verifying mocks; locate the repository usage in
DatabaseMessageRepository and the DAO methods deleteAllDrafts() /
selectDraftMessages() to implement the integration-style test exercising offline
↔ client logout/clear boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aeedfa05-4deb-45e8-8753-c309a675089a
📒 Files selected for processing (3)
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/offline/repository/domain/message/internal/DatabaseMessageRepository.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/offline/repository/domain/message/internal/MessageDao.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/offline/repository/MessageRepositoryTests.kt
|
🚀 Available in v7.0.1 |


Goal
We were not clearing the drafts offline storage on user logout. This resulted in a stuck draft message between sessions.
Ports the #6372
V6fix todevelopImplementation
Clear the drafts table on logout (same as all other tables)
Testing
Summary by CodeRabbit