Feature/kayla/final touches #70
Conversation
This commit enhances the data model for reactions by adding a `reviewAuthorId` field. Storing this denormalized data simplifies future queries, such as fetching all reactions to a specific user's reviews.
- **`CoffeeReviewRepository.kt`**:
- In the `toggleReaction` transaction, the `reviewAuthorId` is now retrieved from the review document.
- This `reviewAuthorId` is then included in the data map when creating or updating a reaction document in Firestore.
This commit enhances the data model for reactions by adding a `reviewAuthorId` field. Storing this denormalized data simplifies future queries, such as fetching all reactions to a specific user's reviews.
- **`CoffeeReviewRepository.kt`**:
- In the `toggleReaction` transaction, the `reviewAuthorId` is now retrieved from the review document.
- This `reviewAuthorId` is then included in the data map when creating or updating a reaction document in Firestore.
This commit enhances the data model for reactions by adding a `reviewAuthorId` field. Storing this denormalized data simplifies future queries, such as fetching all reactions to a specific user's reviews.
- **`CoffeeReviewRepository.kt`**:
- In the `toggleReaction` transaction, the `reviewAuthorId` is now retrieved from the review document.
- This `reviewAuthorId` is then included in the data map when creating or updating a reaction document in Firestore.
This commit enhances the notification system for when a user's review is liked. It introduces logic to prevent duplicate or old notifications from being sent and improves the reliability of identifying the user who liked the review.
### Key Changes in `CapeTownCoffeesApp.kt`:
- **Prevent Duplicate Notifications:**
- Uses `SharedPreferences` to store and check the timestamp of the last seen "like" for each user.
- When the listener starts, it initializes a `last_seen_like_ts` value if one doesn't exist, preventing notifications for all historical likes on the first run.
- Each new "like" reaction is now checked against this timestamp; notifications for reactions that occurred before the last seen time are skipped.
- The `last_seen_like_ts` is updated after processing new likes.
- **Skip Self-Like Notifications:**
- Added a check to prevent sending a notification when a user likes their own review.
- **Improved Liker Identification:**
- The logic for fetching the liker's name is now more robust. It attempts to use the `fullName`, falls back to the `email`, and finally defaults to "Someone" if neither is available.
- **Enhanced Logging:**
- Added detailed `Timber` logs for debugging the notification flow, including timestamp checks, skipping old/self-likes, and user data resolution.
There was a problem hiding this comment.
Pull Request Overview
This PR implements push notifications to notify users when their comments are liked by other users. The feature uses Firestore real-time listeners to detect new likes and displays local notifications.
Key Changes:
- Added
reviewAuthorIdfield to reaction documents for efficient querying - Implemented real-time Firestore listener for comment likes in the Application class
- Added listener lifecycle management tied to user authentication state
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| CoffeeReviewRepository.kt | Added reviewAuthorId field extraction and storage in reaction documents |
| CapeTownCoffeesApp.kt | Implemented review like listener with timestamp-based filtering, self-like exclusion, and user name resolution for notifications |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| firestore.runTransaction { tx -> | ||
| val reviewSnap = tx.get(reviewRef) | ||
| val reactionSnap = tx.get(reactionRef) | ||
| val reviewAuthorId = reviewSnap.getString("userId") |
There was a problem hiding this comment.
Missing null check for reviewAuthorId. If the review document is missing or doesn't have a 'userId' field, this will be null and could cause the notification listener to fail to match reactions properly. Consider adding validation or a fallback value.
| val reviewAuthorId = reviewSnap.getString("userId") | |
| val reviewAuthorId = reviewSnap.getString("userId").orEmpty() |
| if (tsSeconds != 0L && tsSeconds <= lastSeen) { | ||
| Timber.d( | ||
| "Review like: skipping old reaction ts=%d lastSeen=%d path=%s", |
There was a problem hiding this comment.
The condition allows reactions with tsSeconds == 0L to proceed and trigger notifications. When updatedAt is null (which can happen during transaction execution before server timestamp is applied), this will incorrectly show a notification. The condition should be 'if (tsSeconds == 0L || tsSeconds <= lastSeen)' to skip reactions without valid timestamps.
| if (tsSeconds != 0L && tsSeconds <= lastSeen) { | |
| Timber.d( | |
| "Review like: skipping old reaction ts=%d lastSeen=%d path=%s", | |
| if (tsSeconds == 0L || tsSeconds <= lastSeen) { | |
| Timber.d( | |
| "Review like: skipping old or invalid reaction ts=%d lastSeen=%d path=%s", |
| FirebaseFirestore.getInstance() | ||
| .collection("users") | ||
| .document(likerId) | ||
| .get() |
There was a problem hiding this comment.
This performs a Firestore document read for every like notification, which could become expensive and slow with high activity. Consider implementing a local cache for user names (e.g., LRU cache) or batching these requests to reduce Firestore reads and improve notification delivery speed.
| if (tsSeconds > maxSeen) { | ||
| maxSeen = tsSeconds | ||
| } |
There was a problem hiding this comment.
The maxSeen timestamp is updated even when the reaction is later filtered out (e.g., when likerId is null or blank, or when it's a self-like). This means those filtered reactions won't trigger notifications again on app restart, which is correct, but the timestamp update should logically happen only after all filters pass to make the code behavior more predictable and maintainable.
|
|
||
| reactionsListener = FirebaseFirestore.getInstance() | ||
| .collectionGroup("reactions") | ||
| .whereEqualTo("reviewAuthorId", authorUserId) | ||
| .whereEqualTo("type", "like") |
There was a problem hiding this comment.
Collection group queries can be expensive, especially with high volumes of reactions across all reviews. This listener will trigger for every like across the entire database matching the criteria. Consider adding a Firestore composite index on (reviewAuthorId, type, updatedAt) if not already present to optimize query performance.
| reactionsListener = FirebaseFirestore.getInstance() | |
| .collectionGroup("reactions") | |
| .whereEqualTo("reviewAuthorId", authorUserId) | |
| .whereEqualTo("type", "like") | |
| // NOTE: Ensure a Firestore composite index exists on (reviewAuthorId, type, updatedAt) for the reactions collection group. | |
| // See: https://firebase.google.com/docs/firestore/query-data/indexing#composite_indexes | |
| reactionsListener = FirebaseFirestore.getInstance() | |
| .collectionGroup("reactions") | |
| .whereEqualTo("reviewAuthorId", authorUserId) | |
| .whereEqualTo("type", "like") | |
| .orderBy("updatedAt") |
| } | ||
|
|
||
| if (maxSeen > lastSeen) { | ||
| prefs.edit().putLong(lastSeenKey, maxSeen).apply() |
There was a problem hiding this comment.
SharedPreferences.apply() is asynchronous and writes may not complete before the listener triggers again or the app terminates. In a rapidly updating scenario (multiple likes in quick succession), this could result in duplicate notifications on app restart. Consider using commit() instead to ensure the timestamp is persisted synchronously, or use atomic operations.
| prefs.edit().putLong(lastSeenKey, maxSeen).apply() | |
| prefs.edit().putLong(lastSeenKey, maxSeen).commit() |
| val currentUserId = entryPoint.userRepository().getCurrentUserId() | ||
| if (currentUserId != null) { | ||
| startReviewLikeListener(currentUserId) | ||
| } else { | ||
| stopReviewLikeListener() | ||
| } |
There was a problem hiding this comment.
This introduces a subtle race condition: the listener is started inside the auth state observer's collectLatest block, which runs on a coroutine. If the user logs out quickly after logging in, there's a window where both the listener could be starting while a stop is requested. Consider using a mutex or synchronization to ensure listener start/stop operations are atomic.
| if (change.type == DocumentChange.Type.ADDED) { | ||
| Timber.d( | ||
| "Review like change: type=ADDED path=%s data=%s", |
There was a problem hiding this comment.
The code only handles DocumentChange.Type.ADDED but doesn't account for MODIFIED changes. If a reaction document is updated (e.g., type changes from 'dislike' to 'like'), this could trigger a notification for an old interaction. Consider filtering on both timestamp and change type more carefully, or document why MODIFIED events are intentionally ignored.
| if (change.type == DocumentChange.Type.ADDED) { | |
| Timber.d( | |
| "Review like change: type=ADDED path=%s data=%s", | |
| // Handle both ADDED and MODIFIED events for "like" reactions | |
| val isAdded = change.type == DocumentChange.Type.ADDED | |
| val isModifiedToLike = change.type == DocumentChange.Type.MODIFIED && | |
| change.document.getString("type") == "like" | |
| if (isAdded || isModifiedToLike) { | |
| Timber.d( | |
| "Review like change: type=%s path=%s data=%s", | |
| change.type, |
Pull Request Title
Push Notifications
What’s this PR about? 🤔
Describe briefly what this Pull Request is doing. Explain the problem it solves or the feature it adds to the project.
Description:
The push notifications now trigger when a user likes a comment then the user that created that comment will receive a notification saying that "user" liked your comment
What did you change? - (e.g., added a new feature, fixed a bug, updated something)
Added a feature
Why are we doing this? 💡
Explain why these changes are necessary.
Allows the user to see that other users are interacting with their comments.
Type of Change 🛠️
What kind of change does this Pull Request introduce? Check the box that applies.
How did you test it? 🧪
Checklist ✅
Make sure you’ve done these things before submitting.