-
Notifications
You must be signed in to change notification settings - Fork 1.3k
detect and link calendar event to conversations #3747
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: main
Are you sure you want to change the base?
Conversation
aaravgarg
commented
Dec 13, 2025
- Auto-link conversations to calendar events — Conversations automatically link to Google Calendar events when they overlap
- Calendar event card — View event details, open in Google Calendar, unlink, add summary, and share with attendees
- Add summaries to calendar events — Append conversation summaries directly to linked calendar event descriptions
- Share conversations with attendees — One-tap email sharing to all event attendees
- Improved markdown formatting — Better typography and spacing for conversation summaries
- Conversation titles are changes to be calendar event title for better organization but can be changed ofc
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.
Code Review
This pull request introduces a significant new feature to automatically link conversations with Google Calendar events. The changes span both the backend and frontend, including new API endpoints, data models, and UI components for displaying and interacting with linked events. My review focuses on improving the robustness and maintainability of the new code. Specifically, I've provided suggestions to make data deserialization in Dart safer, to improve the maintainability of state updates by introducing a copyWith pattern, and to make the Python backend's error handling more explicit and easier to debug, particularly by avoiding silent failures in exception blocks.
| factory CalendarEventLink.fromJson(Map<String, dynamic> json) { | ||
| return CalendarEventLink( | ||
| eventId: json['event_id'] ?? '', | ||
| title: json['title'] ?? '', | ||
| attendees: ((json['attendees'] ?? []) as List<dynamic>).map((e) => e.toString()).toList(), | ||
| attendeeEmails: ((json['attendee_emails'] ?? []) as List<dynamic>).map((e) => e.toString()).toList(), | ||
| startTime: json['start_time'] != null ? DateTime.parse(json['start_time']).toLocal() : DateTime.now(), | ||
| endTime: json['end_time'] != null ? DateTime.parse(json['end_time']).toLocal() : DateTime.now(), | ||
| htmlLink: json['html_link'], | ||
| ); | ||
| } |
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.
The fromJson factory uses default values like '' and DateTime.now() for required fields (eventId, title, startTime, endTime) when they are null in the JSON. This can hide issues from the backend and lead to incorrect data being displayed to the user (e.g., a wrong event time). It's better to fail fast if required data is missing. Consider throwing an ArgumentError if these fields are null to ensure data integrity.
factory CalendarEventLink.fromJson(Map<String, dynamic> json) {
if (json['event_id'] == null || (json['event_id'] as String).isEmpty) {
throw ArgumentError('event_id is required for CalendarEventLink');
}
if (json['title'] == null) {
throw ArgumentError('title is required for CalendarEventLink');
}
if (json['start_time'] == null) {
throw ArgumentError('start_time is required for CalendarEventLink');
}
if (json['end_time'] == null) {
throw ArgumentError('end_time is required for CalendarEventLink');
}
return CalendarEventLink(
eventId: json['event_id'],
title: json['title'],
attendees: ((json['attendees'] ?? []) as List<dynamic>).map((e) => e.toString()).toList(),
attendeeEmails: ((json['attendee_emails'] ?? []) as List<dynamic>).map((e) => e.toString()).toList(),
startTime: DateTime.parse(json['start_time']).toLocal(),
endTime: DateTime.parse(json['end_time']).toLocal(),
htmlLink: json['html_link'],
);
}| if (_cachedConversation != null) { | ||
| // Create a new ServerConversation without the calendar event | ||
| final updatedConversation = ServerConversation( | ||
| id: _cachedConversation!.id, | ||
| createdAt: _cachedConversation!.createdAt, | ||
| structured: _cachedConversation!.structured, | ||
| startedAt: _cachedConversation!.startedAt, | ||
| finishedAt: _cachedConversation!.finishedAt, | ||
| transcriptSegments: _cachedConversation!.transcriptSegments, | ||
| appResults: _cachedConversation!.appResults, | ||
| suggestedSummarizationApps: _cachedConversation!.suggestedSummarizationApps, | ||
| geolocation: _cachedConversation!.geolocation, | ||
| photos: _cachedConversation!.photos, | ||
| discarded: _cachedConversation!.discarded, | ||
| deleted: _cachedConversation!.deleted, | ||
| source: _cachedConversation!.source, | ||
| language: _cachedConversation!.language, | ||
| externalIntegration: _cachedConversation!.externalIntegration, | ||
| calendarEvent: null, // Remove calendar event | ||
| status: _cachedConversation!.status, | ||
| isLocked: _cachedConversation!.isLocked, | ||
| ); | ||
| _cachedConversation = updatedConversation; | ||
|
|
||
| // Also update in the conversation provider | ||
| conversationProvider?.updateConversation(updatedConversation); | ||
| } |
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.
This manual recreation of the ServerConversation object is verbose and error-prone. If new fields are added to ServerConversation in the future, this code must be manually updated, and it's easy to forget. A better approach is to add a copyWith method to the ServerConversation class. This is a standard pattern in Dart for working with immutable objects and would make this code much cleaner and more maintainable.
For example, you could add a copyWith method to ServerConversation and then simplify this block to something like:
if (_cachedConversation != null) {
final updatedConversation = _cachedConversation!.copyWith(calendarEvent: () => null);
_cachedConversation = updatedConversation;
conversationProvider?.updateConversation(updatedConversation);
}Using a ValueGetter (like () => null) is a good pattern for allowing nullable fields to be explicitly set to null.
| # Try to refresh token if authentication failed | ||
| if "401" in error_msg or "Authentication" in error_msg.lower(): | ||
| new_token = refresh_google_token(uid, integration) | ||
| if new_token: | ||
| try: | ||
| return _add_summary_to_calendar_event_with_token(new_token, event_id, conversation_id) | ||
| except Exception as retry_error: | ||
| raise HTTPException(status_code=500, detail=f"Failed after token refresh: {str(retry_error)}") | ||
|
|
||
| raise HTTPException(status_code=500, detail=f"Failed to update calendar event: {error_msg}") |
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.
The error handling for a failed token refresh can be improved. If refresh_google_token returns None, the code falls through and raises a generic 500 error with the original "401" message. This can be misleading for the client and for debugging. It would be better to explicitly handle the token refresh failure and return a 401 status code with a more specific error message.
| # Try to refresh token if authentication failed | |
| if "401" in error_msg or "Authentication" in error_msg.lower(): | |
| new_token = refresh_google_token(uid, integration) | |
| if new_token: | |
| try: | |
| return _add_summary_to_calendar_event_with_token(new_token, event_id, conversation_id) | |
| except Exception as retry_error: | |
| raise HTTPException(status_code=500, detail=f"Failed after token refresh: {str(retry_error)}") | |
| raise HTTPException(status_code=500, detail=f"Failed to update calendar event: {error_msg}") | |
| # Try to refresh token if authentication failed | |
| if "401" in error_msg or "Authentication" in error_msg.lower(): | |
| new_token = refresh_google_token(uid, integration) | |
| if new_token: | |
| try: | |
| return _add_summary_to_calendar_event_with_token(new_token, event_id, conversation_id) | |
| except Exception as retry_error: | |
| raise HTTPException(status_code=500, detail=f"Failed after token refresh: {str(retry_error)}") | |
| else: | |
| raise HTTPException(status_code=401, detail="Failed to refresh Google token. Please re-authenticate.") |
| except Exception as e: | ||
| error_msg = str(e) | ||
| # Try to refresh token if authentication failed | ||
| if "Authentication failed" in error_msg or "401" in error_msg: | ||
| new_token = refresh_google_token(uid, integration) | ||
| if new_token: | ||
| try: | ||
| events = get_google_calendar_events( | ||
| access_token=new_token, | ||
| time_min=search_start, | ||
| time_max=search_end, | ||
| max_results=20, | ||
| ) | ||
| except Exception: | ||
| return None | ||
| else: | ||
| return None | ||
| else: | ||
| return None |
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.
The exception handling here is too broad and silently fails by returning None. Using except Exception: can hide various issues, from network errors to bugs in the called functions. When an error occurs, especially after a token refresh attempt, it's better to log it for debugging purposes instead of just returning None. Silent failures make it very difficult to diagnose problems with this feature.
except Exception as e:
error_msg = str(e)
# Try to refresh token if authentication failed
if "Authentication failed" in error_msg or "401" in error_msg:
new_token = refresh_google_token(uid, integration)
if new_token:
try:
events = get_google_calendar_events(
access_token=new_token,
time_min=search_start,
time_max=search_end,
max_results=20,
)
except Exception as retry_e:
print(f"Error fetching calendar events after token refresh: {retry_e}")
return None
else:
print("Failed to refresh Google token.")
return None
else:
print(f"Error fetching calendar events: {e}")
return NoneCo-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
man pls add the demo |
Sorry was still WIP, changed from draft by mistake |
Resolved conflicts: - app/lib/backend/schema/conversation.dart: kept both CalendarEventLink and AudioFile classes - app/lib/pages/conversation_detail/page.dart: merged CalendarEventCard with GetGeolocationWidgets - backend/models/conversation.py: kept both CalendarEventLink and CalendarMeetingContext classes - backend/utils/conversations/process_conversation.py: kept both imports
…edHardware/omi into detect-and-link-to-cal-event
…hanges for connected calendar widget.