Update seen by id field in message threads #156
Conversation
WalkthroughThis pull request introduces modifications to the messaging system's architecture, focusing on how message visibility and thread status are managed. Key changes include adding a new parameter Changes
Sequence DiagramsequenceDiagram
participant User
participant MessagesViewModel
participant MessagesRepository
participant ApiMessagesService
participant Thread
User->>MessagesViewModel: Send/View Messages
MessagesViewModel->>Thread: Check seen_by_ids
MessagesViewModel->>MessagesRepository: Mark Messages Seen
MessagesRepository->>ApiMessagesService: Update Thread Seen Status
ApiMessagesService->>Thread: Update seen_by_ids
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/src/main/java/com/canopas/yourspace/ui/flow/messages/chat/MessagesScreen.kt(1 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/messages/chat/MessagesViewModel.kt(3 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/messages/chat/components/MessagesList.kt(5 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/messages/thread/ThreadsScreen.kt(1 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/messages/thread/ThreadsViewModel.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/models/messages/ApiThread.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/repository/MessagesRepository.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/service/messages/ApiMessagesService.kt(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/canopas/yourspace/ui/flow/messages/thread/ThreadsViewModel.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (10)
data/src/main/java/com/canopas/yourspace/data/repository/MessagesRepository.kt (2)
61-62: LGTM! Clean implementation of thread-level message tracking.The simplified function signature and implementation aligns well with the architectural change from per-message to thread-level seen status tracking. This change reduces complexity and improves efficiency by updating a single document instead of performing batch updates.
61-62: Verify data migration strategy for existing messages.Since this is a breaking change that moves from per-message
seen_byto thread-levelseen_by_ids, please ensure there's a migration strategy in place for existing messages.Let's verify the migration approach:
data/src/main/java/com/canopas/yourspace/data/service/messages/ApiMessagesService.kt (3)
Line range hint
93-100: 'sendMessage' method updated correctly to removeseen_byfieldThe
sendMessagemethod has been updated to constructApiThreadMessagewithout theseen_byfield, which aligns with the new approach of tracking message visibility at the thread level usingseen_by_idsin the thread document.
Line range hint
105-111:generateMessagemethod modification is consistent with the new data modelThe
generateMessagemethod now createsApiThreadMessageinstances without theseen_byfield. This change is consistent with the updated data model where the seen status is managed at the thread level.
116-117: Ensure thread-levelseen_by_idsupdates handle concurrency appropriatelyThe
markMessagesAsSeenmethod now updates theseen_by_idsfield in the thread document to track which users have seen the messages. It's important to ensure that concurrent updates from multiple users do not cause data inconsistencies or overwrite issues.Please run the following script to verify that
seen_by_idsis consistently used across the codebase and that there are no remaining references to the oldseen_byfield:✅ Verification successful
Thread-level
seen_by_idsupdates are properly implemented with atomic operationsThe implementation uses Firestore's atomic
arrayUnionoperation which safely handles concurrent updates without data inconsistencies. The field is consistently used across the codebase with no remaining references to the oldseen_byfield.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all references to 'seen_by' have been updated to 'seen_by_ids' and check for proper usage. # Expectation: No occurrences of 'seen_by' except in field definitions or comments explaining the change. # Search for any remaining references to 'seen_by' outside of model definitions. rg 'seen_by' --type kotlin # Confirm that 'seen_by_ids' is used in the appropriate places. rg 'seen_by_ids' --type kotlinLength of output: 2153
data/src/main/java/com/canopas/yourspace/data/models/messages/ApiThread.kt (1)
17-17: LGTM! Consider data migration impact.The consolidation of message visibility tracking at thread level is a good architectural decision that reduces data duplication.
Please ensure there's a migration plan to handle existing data. Run this script to check for any direct usage of the removed
seen_byfield:✅ Verification successful
No migration concerns found
The change only affects the API model and the UI is already aligned with the new structure. No local database migrations are needed as the project appears to use remote storage only.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of the removed seen_by field # Test: Search for seen_by references that might need updating rg "seen_by\W" --type kotlinLength of output: 172
Script:
#!/bin/bash # Check data layer for seen_by field usage echo "=== Checking Room DAOs ===" rg "seen_by\W" --type kotlin -g "*Dao.kt" echo -e "\n=== Checking data mappers ===" rg "seen_by\W" --type kotlin -g "*Mapper.kt" echo -e "\n=== Checking database migrations ===" fd "Migration" --type f --exec rg "seen_by\W" {} \; echo -e "\n=== Checking database schemas ===" fd "schema" --type f --extension json --exec cat {} \;Length of output: 811
Script:
#!/bin/bash # Find database and persistence related files echo "=== Checking for database related files ===" fd "\.kt$" --type f --exec rg -l "import.*\b(Room|SQLite|Database)\b" {} \; echo -e "\n=== Checking for thread/message persistence ===" rg -l "Thread|Message" --type kotlin -g "*Repository.kt" echo -e "\n=== Looking for data storage implementations ===" fd "storage|database|dao|repository" --type f --extension ktLength of output: 914
app/src/main/java/com/canopas/yourspace/ui/flow/messages/chat/MessagesScreen.kt (1)
146-146: LGTM! Clean integration of thread parameter.The addition of the thread parameter to MessageList is a necessary change to support the new visibility tracking mechanism.
app/src/main/java/com/canopas/yourspace/ui/flow/messages/chat/components/MessagesList.kt (2)
103-107: LGTM! Robust null safety handling.The seenBy calculation properly handles the null case for thread and correctly filters the members list.
240-244: Improved message text color contrast.The message text color now properly adapts based on the sender status, improving readability.
app/src/main/java/com/canopas/yourspace/ui/flow/messages/thread/ThreadsScreen.kt (1)
386-386: LGTM! Improved architecture for tracking message visibility.The change from message-level
seen_byto thread-levelseen_by_idsis a good architectural improvement that reduces data duplication.Please verify that existing threads have been properly migrated to include
seen_by_ids. Run this script to check for any threads without the field:✅ Verification successful
Implementation verified - Properly handles message visibility tracking
The
seen_by_idsfield is properly implemented with:
- Default initialization in the data model
- Correct updates in cloud functions
- Proper handling in the UI
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for threads that might be missing seen_by_ids field # Note: Adjust the pattern based on your data storage implementation # Search for thread definitions or database schemas rg -l "seen_by_ids" # Search for migration files fd -e kt -e java -e xml "migration|schema"Length of output: 496
Script:
#!/bin/bash # Check the data model implementation echo "=== ApiThread.kt ===" cat data/src/main/java/com/canopas/yourspace/data/models/messages/ApiThread.kt echo -e "\n=== Cloud Functions ===" cat functions/index.jsLength of output: 19378
app/src/main/java/com/canopas/yourspace/ui/flow/messages/chat/MessagesViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/flow/messages/chat/MessagesViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/flow/messages/chat/MessagesViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/flow/messages/chat/MessagesViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/flow/messages/thread/ThreadsViewModel.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
app/src/main/java/com/canopas/yourspace/ui/flow/messages/chat/MessagesViewModel.kt (3)
95-104:⚠️ Potential issueRace condition still needs to be addressed.
While the early return optimization is good, the race condition concern from previous reviews still exists. Consider using a transaction to handle concurrent updates to
seen_by_ids.Suggested implementation using transaction:
private fun markMessagesAsSeen() = viewModelScope.launch(appDispatcher.IO) { try { val thread = state.value.thread ?: return@launch val userId = state.value.currentUserId if (thread.seen_by_ids.contains(userId)) { return@launch } - messagesRepository.markMessagesAsSeen(threadId, userId) + messagesRepository.runTransaction { transaction -> + val threadRef = messagesRepository.getThreadRef(threadId) + val currentThread = transaction.get(threadRef).toObject(ApiThread::class.java) + ?: return@runTransaction + + if (!currentThread.seen_by_ids.contains(userId)) { + transaction.update(threadRef, "seen_by_ids", + currentThread.seen_by_ids + userId) + } + } } catch (e: Exception) { Timber.e(e, "Error marking messages as seen") } }
310-312:⚠️ Potential issueRemove unnecessary seen status reset.
Clearing
seen_by_idsbefore sending a message is incorrect as it causes loss of existing seen status. The Firebase backend already handles seen status initialization for new messages.Apply this diff:
- val thread = state.value.thread?.copy(seen_by_ids = emptyList()) + val thread = state.value.thread _state.emit(_state.value.copy(newMessagesToAppend = newMessages, thread = thread))
178-178: 🛠️ Refactor suggestionRemove duplicate markMessagesAsSeen call.
The call to
markMessagesAsSeen()infetchThreadis unnecessary sincelistenMessages()already handles marking messages as seen.Apply this diff:
- markMessagesAsSeen() if (messagesJob == null) listenMessages()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/canopas/yourspace/ui/flow/messages/chat/MessagesViewModel.kt(3 hunks)data/src/main/java/com/canopas/yourspace/data/models/messages/ApiThread.kt(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Android Build
data/src/main/java/com/canopas/yourspace/data/models/messages/ApiThread.kt
[error] 16-16: Missing spacing after ':' at column 22
[error] 21-21: Unnecessary trailing comma before ')' at column 38
🔇 Additional comments (2)
data/src/main/java/com/canopas/yourspace/data/models/messages/ApiThread.kt (2)
16-21: LGTM! Thread-level message tracking implementation looks good.The new properties support tracking message status at thread level:
last_messagestores the latest message contentseen_by_idstracks who has seen the threadlast_message_attracks the timestamp of the last message🧰 Tools
🪛 GitHub Actions: Android Build
[error] 16-16: Missing spacing after ':' at column 22
[error] 21-21: Unnecessary trailing comma before ')' at column 38
Line range hint
29-36: LGTM! Removal of message-level seen tracking.The removal of
seen_byfromApiThreadMessagealigns with the architectural change to track message visibility at thread level instead of individual messages.🧰 Tools
🪛 GitHub Actions: Android Build
[error] 16-16: Missing spacing after ':' at column 22
[error] 21-21: Unnecessary trailing comma before ')' at column 38
data/src/main/java/com/canopas/yourspace/data/models/messages/ApiThread.kt
Outdated
Show resolved
Hide resolved
Changelog
Sender device:
message.test.webm
Receiver device:
Record_2025-01-07-16-48-47.mp4
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
The update focuses on refining the messaging experience with more efficient status tracking and improved visual indicators.