share important convo with contacts #3879
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to notify users about important (long) conversations and allows them to share a summary via SMS with their contacts. This includes adding contacts permissions, a new dependency for accessing contacts, a new UI for contact selection, and backend logic to trigger these notifications. The changes are extensive and well-implemented for the most part.
My main feedback is on the backend notification sending logic. The new and modified functions for sending data-only FCM messages are inefficient and have regressed in error handling, specifically in removing invalid device tokens. I've suggested refactoring these to use an existing helper function that handles batching and token cleanup correctly.
backend/utils/notifications.py
Outdated
| except Exception as e: | ||
| _handle_send_error(e, token) | ||
| print(f'FCM send error for merge completed: {e}') |
There was a problem hiding this comment.
This change removes the _handle_send_error call, which likely handled the removal of invalid FCM tokens. Simply printing the error is a regression that can lead to repeatedly trying to send notifications to invalid tokens, wasting resources and creating log noise.
The surrounding function send_merge_completed_message is also inefficient because it sends messages one by one in a loop instead of batching them.
Consider refactoring the entire send_merge_completed_message function to use the _send_to_user helper, which handles batch sending and invalid token removal correctly. This would make the implementation more robust and efficient.
Here's an example of how it could be refactored:
def send_merge_completed_message(user_id: str, merged_conversation_id: str, removed_conversation_ids: list):
"""
Sends a data-only FCM message when conversation merge completes.
...
"""
data = {
'type': 'merge_completed',
'merged_conversation_id': merged_conversation_id,
'removed_conversation_ids': ','.join(removed_conversation_ids),
}
tag = _generate_tag(f'{user_id}:merge_completed:{merged_conversation_id}')
_send_to_user(user_id, tag, data=data, is_background=True, priority='high')| def send_important_conversation_message(user_id: str, conversation_id: str): | ||
| """ | ||
| Sends a data-only FCM message when a long conversation (>30 min) completes. | ||
| The app receives this and: | ||
| - Shows a local notification: "You just had an important convo, click to share summary" | ||
| - On tap: navigates to conversation detail with share sheet auto-open | ||
| Args: | ||
| user_id: The user's Firebase UID | ||
| conversation_id: ID of the completed conversation | ||
| """ | ||
| tokens = notification_db.get_all_tokens(user_id) | ||
| if not tokens: | ||
| print(f"No notification tokens found for user {user_id} for important conversation notification") | ||
| return | ||
|
|
||
| # FCM data values must be strings | ||
| data = { | ||
| 'type': 'important_conversation', | ||
| 'conversation_id': conversation_id, | ||
| 'navigate_to': f'/conversation/{conversation_id}?share=1', | ||
| } | ||
|
|
||
| for token in tokens: | ||
| message = messaging.Message( | ||
| data=data, | ||
| token=token, | ||
| android=messaging.AndroidConfig(priority='high'), | ||
| apns=messaging.APNSConfig( | ||
| headers={ | ||
| 'apns-priority': '10', | ||
| }, | ||
| payload=messaging.APNSPayload(aps=messaging.Aps(content_available=True)), | ||
| ), | ||
| ) | ||
|
|
||
| try: | ||
| response = messaging.send(message) | ||
| print(f'Important conversation message sent to device: {response}') | ||
| except Exception as e: | ||
| print(f'FCM send error for important conversation: {e}') |
There was a problem hiding this comment.
This function manually iterates through tokens and sends messages one by one. This is inefficient and lacks proper error handling for invalid tokens, which can lead to wasted resources and repeated failures.
To improve this, you should use the existing _send_to_user helper function. It handles batch sending and removes invalid tokens, making the code more robust and efficient. You'll also need to generate a unique tag for the notification to support platform-side deduplication.
def send_important_conversation_message(user_id: str, conversation_id: str):
"""
Sends a data-only FCM message when a long conversation (>30 min) completes.
The app receives this and:
- Shows a local notification: "You just had an important convo, click to share summary"
- On tap: navigates to conversation detail with share sheet auto-open
Args:
user_id: The user's Firebase UID
conversation_id: ID of the completed conversation
"""
data = {
'type': 'important_conversation',
'conversation_id': conversation_id,
'navigate_to': f'/conversation/{conversation_id}?share=1',
}
tag = _generate_tag(f'{user_id}:important_conversation:{conversation_id}')
_send_to_user(user_id, tag, data=data, is_background=True, priority='high')
|
@aaravgarg conflicts, also why does it take so long after clicking share with 1 contact button |
in video or locally? video coz my phone sucks |
If convo over 30 mins, send notif to suggest sharing convo with contacts
trim.95379F87-4C7C-480D-B5CF-16AF3DF6E26E.MOV