New Tools Added to Chat (Calendar, Whoop, Notion, X) #3399
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 significant new functionality by adding integrations for Google Calendar, Whoop, and Notion. The changes span both the frontend and backend, including new API endpoints, services, providers, and UI components. While the implementation appears functional, there are several critical areas with substantial code duplication across the new services and UI components. Addressing these duplications by refactoring into shared base classes, helper methods, and reusable widgets would greatly improve the maintainability and robustness of the codebase. I've also identified opportunities for performance improvement in asynchronous operations and a potential security enhancement in data serialization.
| import 'package:flutter/material.dart'; | ||
| import 'package:omi/backend/http/api/integrations.dart'; | ||
| import 'package:omi/backend/preferences.dart'; | ||
| import 'package:url_launcher/url_launcher.dart'; | ||
|
|
||
| class GoogleCalendarService { | ||
| static const String _appKey = 'google_calendar'; | ||
| static const String _prefKey = 'google_calendar_connected'; | ||
|
|
||
| bool get isAuthenticated { | ||
| // Check both preference and backend | ||
| return SharedPreferencesUtil().getBool(_prefKey) ?? false; | ||
| } | ||
|
|
||
| Future<void> refreshConnectionStatus() async { | ||
| await checkConnection(); | ||
| } | ||
|
|
||
| Future<bool> authenticate() async { | ||
| try { | ||
| final authUrl = await getIntegrationOAuthUrl(_appKey); | ||
| if (authUrl == null) { | ||
| debugPrint('Failed to get Google Calendar OAuth URL'); | ||
| return false; | ||
| } | ||
|
|
||
| final uri = Uri.parse(authUrl); | ||
| if (await canLaunchUrl(uri)) { | ||
| await launchUrl(uri, mode: LaunchMode.externalApplication); | ||
| // Note: OAuth callback will save connection to Firebase | ||
| // The connection status will be updated when user returns to the app | ||
| return true; | ||
| } else { | ||
| debugPrint('Cannot launch Google Calendar OAuth URL'); | ||
| return false; | ||
| } | ||
| } catch (e) { | ||
| debugPrint('Error during Google Calendar authentication: $e'); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| Future<bool> checkConnection() async { | ||
| try { | ||
| final response = await getIntegration(_appKey); | ||
| final isConnected = response != null && response.connected; | ||
| await SharedPreferencesUtil().saveBool(_prefKey, isConnected); | ||
| return isConnected; | ||
| } catch (e) { | ||
| debugPrint('Error checking Google Calendar connection: $e'); | ||
| await SharedPreferencesUtil().saveBool(_prefKey, false); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| Future<bool> disconnect() async { | ||
| try { | ||
| final success = await deleteIntegration(_appKey); | ||
| if (success) { | ||
| await SharedPreferencesUtil().saveBool(_prefKey, false); | ||
| return true; | ||
| } | ||
| return false; | ||
| } catch (e) { | ||
| debugPrint('Error disconnecting Google Calendar: $e'); | ||
| return false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This file, along with notion_service.dart and whoop_service.dart, contains a large amount of duplicated code. All three service classes have the same structure and methods (isAuthenticated, refreshConnectionStatus, authenticate, checkConnection, disconnect). This is a significant maintenance concern. This structure should be refactored into an abstract BaseIntegrationService class that contains the common logic, and each specific service should then extend this base class, providing only the unique appKey and prefKey.
app/lib/core/app_shell.dart
Outdated
| } else if (uri.host == 'notion' && uri.pathSegments.isNotEmpty && uri.pathSegments.first == 'callback') { | ||
| // Handle Notion OAuth callback | ||
| final error = uri.queryParameters['error']; | ||
| if (error != null) { | ||
| debugPrint('Notion OAuth error: $error'); | ||
| AppSnackbar.showSnackbarError('Failed to connect to Notion: $error'); | ||
| return; | ||
| } | ||
|
|
||
| final success = uri.queryParameters['success']; | ||
| if (success == 'true') { | ||
| debugPrint('Notion OAuth successful (tokens in Firebase)'); | ||
| _handleNotionCallback(); | ||
| } else { | ||
| debugPrint('Notion callback received but no success flag'); | ||
| } | ||
| } else if (uri.host == 'google_calendar' && uri.pathSegments.isNotEmpty && uri.pathSegments.first == 'callback') { | ||
| // Handle Google Calendar OAuth callback | ||
| final error = uri.queryParameters['error']; | ||
| if (error != null) { | ||
| debugPrint('Google Calendar OAuth error: $error'); | ||
| AppSnackbar.showSnackbarError('Failed to connect to Google Calendar: $error'); | ||
| return; | ||
| } | ||
|
|
||
| final success = uri.queryParameters['success']; | ||
| if (success == 'true') { | ||
| debugPrint('Google Calendar OAuth successful (tokens in Firebase)'); | ||
| _handleGoogleCalendarCallback(); | ||
| } else { | ||
| debugPrint('Google Calendar callback received but no success flag'); | ||
| } | ||
| } else if (uri.host == 'whoop' && uri.pathSegments.isNotEmpty && uri.pathSegments.first == 'callback') { | ||
| // Handle Whoop OAuth callback | ||
| final error = uri.queryParameters['error']; | ||
| if (error != null) { | ||
| debugPrint('Whoop OAuth error: $error'); | ||
| AppSnackbar.showSnackbarError('Failed to connect to Whoop: $error'); | ||
| return; | ||
| } | ||
|
|
||
| final success = uri.queryParameters['success']; | ||
| if (success == 'true') { | ||
| debugPrint('Whoop OAuth successful (tokens in Firebase)'); | ||
| _handleWhoopCallback(); | ||
| } else { | ||
| debugPrint('Whoop callback received but no success flag'); | ||
| } |
There was a problem hiding this comment.
There is significant code duplication in handling the OAuth callbacks for Notion, Google Calendar, and Whoop. The logic for parsing the error and success parameters from the URI is identical for all three. This should be refactored into a single helper method to improve maintainability and reduce redundancy. A single method could take the uri, integration name, and a success handler callback as arguments.
| // Check if Google Calendar requires authentication | ||
| if (app == IntegrationApp.googleCalendar) { | ||
| final googleCalendarService = GoogleCalendarService(); | ||
| if (!googleCalendarService.isAuthenticated) { | ||
| final shouldAuth = await _showAuthDialog(app); | ||
| if (shouldAuth == true) { | ||
| final success = await googleCalendarService.authenticate(); | ||
| if (success) { | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Please complete authentication in your browser. Once done, return to the app.'), | ||
| duration: Duration(seconds: 5), | ||
| ), | ||
| ); | ||
| } | ||
| // Refresh connection status | ||
| await _loadFromBackend(); | ||
| debugPrint('✓ Integration enabled: ${app.displayName} (${app.key}) - authentication in progress'); | ||
| } else { | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Failed to start Google Calendar authentication'), | ||
| backgroundColor: Colors.red, | ||
| duration: Duration(seconds: 3), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Check if Whoop requires authentication | ||
| if (app == IntegrationApp.whoop) { | ||
| final whoopService = WhoopService(); | ||
| if (!whoopService.isAuthenticated) { | ||
| final shouldAuth = await _showAuthDialog(app); | ||
| if (shouldAuth == true) { | ||
| final success = await whoopService.authenticate(); | ||
| if (success) { | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Please complete authentication in your browser. Once done, return to the app.'), | ||
| duration: Duration(seconds: 5), | ||
| ), | ||
| ); | ||
| } | ||
| // Refresh connection status | ||
| await _loadFromBackend(); | ||
| debugPrint('✓ Integration enabled: ${app.displayName} (${app.key}) - authentication in progress'); | ||
| } else { | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Failed to start Whoop authentication'), | ||
| backgroundColor: Colors.red, | ||
| duration: Duration(seconds: 3), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Check if Notion requires authentication | ||
| if (app == IntegrationApp.notion) { | ||
| final notionService = NotionService(); | ||
| if (!notionService.isAuthenticated) { | ||
| final shouldAuth = await _showAuthDialog(app); | ||
| if (shouldAuth == true) { | ||
| final success = await notionService.authenticate(); | ||
| if (success) { | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Please complete authentication in your browser. Once done, return to the app.'), | ||
| duration: Duration(seconds: 5), | ||
| ), | ||
| ); | ||
| } | ||
| // Refresh connection status | ||
| await _loadFromBackend(); | ||
| debugPrint('✓ Integration enabled: ${app.displayName} (${app.key}) - authentication in progress'); | ||
| } else { | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Failed to start Notion authentication'), | ||
| backgroundColor: Colors.red, | ||
| duration: Duration(seconds: 3), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The _connectApp method contains a large block of duplicated code for handling the authentication flow for Google Calendar, Whoop, and Notion. The logic for checking authentication status, showing a dialog, and handling the success/failure of the authentication call is nearly identical for each service. This should be refactored into a generic helper method that accepts the integration app and a service instance conforming to a common interface. This will significantly reduce code duplication and make it easier to add new integrations in the future.
| if (confirmed == true) { | ||
| if (app == IntegrationApp.googleCalendar) { | ||
| final googleCalendarService = GoogleCalendarService(); | ||
| final success = await googleCalendarService.disconnect(); | ||
| if (success) { | ||
| await context.read<IntegrationProvider>().deleteConnection(app.key); | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text('Disconnected from ${app.displayName}'), | ||
| duration: const Duration(seconds: 2), | ||
| ), | ||
| ); | ||
| } | ||
| } else { | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Failed to disconnect'), | ||
| backgroundColor: Colors.red, | ||
| duration: Duration(seconds: 3), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| } else if (app == IntegrationApp.whoop) { | ||
| final whoopService = WhoopService(); | ||
| final success = await whoopService.disconnect(); | ||
| if (success) { | ||
| await context.read<IntegrationProvider>().deleteConnection(app.key); | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text('Disconnected from ${app.displayName}'), | ||
| duration: const Duration(seconds: 2), | ||
| ), | ||
| ); | ||
| } | ||
| } else { | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Failed to disconnect'), | ||
| backgroundColor: Colors.red, | ||
| duration: Duration(seconds: 3), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| } else if (app == IntegrationApp.notion) { | ||
| final notionService = NotionService(); | ||
| final success = await notionService.disconnect(); | ||
| if (success) { | ||
| await context.read<IntegrationProvider>().deleteConnection(app.key); | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text('Disconnected from ${app.displayName}'), | ||
| duration: const Duration(seconds: 2), | ||
| ), | ||
| ); | ||
| } | ||
| } else { | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Failed to disconnect'), | ||
| backgroundColor: Colors.red, | ||
| duration: Duration(seconds: 3), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the _connectApp method, the _disconnectApp method also has a lot of duplicated code for handling the disconnection logic for each integration. The pattern of calling the service's disconnect method, updating the provider, and showing a snackbar is repeated. This can be refactored into a single helper method to improve maintainability.
| Future<void> loadFromBackend() async { | ||
| try { | ||
| // Check Google Calendar connection | ||
| final googleCalendarResponse = await getIntegration('google_calendar'); | ||
| if (googleCalendarResponse != null) { | ||
| _integrations['google_calendar'] = googleCalendarResponse.connected; | ||
| } else { | ||
| _integrations['google_calendar'] = false; | ||
| } | ||
|
|
||
| // Check Whoop connection | ||
| final whoopResponse = await getIntegration('whoop'); | ||
| if (whoopResponse != null) { | ||
| _integrations['whoop'] = whoopResponse.connected; | ||
| } else { | ||
| _integrations['whoop'] = false; | ||
| } | ||
|
|
||
| // Check Notion connection | ||
| final notionResponse = await getIntegration('notion'); | ||
| if (notionResponse != null) { | ||
| _integrations['notion'] = notionResponse.connected; | ||
| } else { | ||
| _integrations['notion'] = false; | ||
| } | ||
|
|
||
| notifyListeners(); | ||
| } catch (e) { | ||
| debugPrint('Error loading integrations from backend: $e'); | ||
| } |
There was a problem hiding this comment.
The loadFromBackend method makes three separate await calls to getIntegration sequentially. This means each network request waits for the previous one to complete, slowing down the data loading process. These calls are independent and can be run in parallel using Future.wait to improve performance.
Future<void> loadFromBackend() async {
try {
final responses = await Future.wait([
getIntegration('google_calendar'),
getIntegration('whoop'),
getIntegration('notion'),
]);
_integrations['google_calendar'] = responses[0]?.connected ?? false;
_integrations['whoop'] = responses[1]?.connected ?? false;
_integrations['notion'] = responses[2]?.connected ?? false;
notifyListeners();
} catch (e) {
debugPrint('Error loading integrations from backend: $e');
}
}
backend/database/users.py
Outdated
| Connection details or None if not found | ||
| """ | ||
| user_ref = db.collection('users').document(uid) | ||
| integration_ref = user_ref.collection('calendar_integrations').document(app_key) |
There was a problem hiding this comment.
The Firestore collection for integrations is named calendar_integrations. This is misleading as it's being used to store connections for Whoop and Notion as well, not just calendars. The collection should be renamed to something more generic, like integrations, to accurately reflect its purpose and avoid confusion for future development.
| integration_ref = user_ref.collection('calendar_integrations').document(app_key) | |
| integration_ref = user_ref.collection('integrations').document(app_key) |
backend/routers/integrations.py
Outdated
| state_data = ast.literal_eval(state_data_str.decode() if isinstance(state_data_str, bytes) else state_data_str) | ||
| return state_data |
There was a problem hiding this comment.
Using ast.literal_eval to deserialize data from Redis is not ideal and can be unsafe if the string is not a well-formed literal. It's better to use a standard serialization format like JSON. The state data should be stored in Redis as a JSON string using json.dumps and then parsed here using json.loads. This is a more robust and secure approach.
| state_data = ast.literal_eval(state_data_str.decode() if isinstance(state_data_str, bytes) else state_data_str) | |
| return state_data | |
| state_data = json.loads(state_data_str.decode() if isinstance(state_data_str, bytes) else state_data_str) |
backend/routers/integrations.py
Outdated
| if app_key == 'google_calendar': | ||
| client_id = os.getenv('GOOGLE_CLIENT_ID') | ||
| if not client_id: | ||
| print(f'ERROR: GOOGLE_CLIENT_ID not configured for integration OAuth') | ||
| raise HTTPException(status_code=500, detail="Google Calendar not configured - GOOGLE_CLIENT_ID missing") | ||
|
|
||
| # Remove trailing slash from base_url if present | ||
| base_url_clean = base_url.rstrip('/') | ||
| redirect_uri = f'{base_url_clean}/v2/integrations/google-calendar/callback' | ||
| # Use calendar scope (includes read and write access) and contacts scopes for People API | ||
| # This allows creating events and looking up contacts by name in both My Contacts and Other Contacts | ||
| scope = 'https://www.googleapis.com/auth/calendar https://www.googleapis.com/auth/contacts.readonly https://www.googleapis.com/auth/contacts.other.readonly' | ||
| from urllib.parse import quote | ||
|
|
||
| auth_url = f'https://accounts.google.com/o/oauth2/v2/auth?client_id={client_id}&redirect_uri={quote(redirect_uri)}&response_type=code&scope={quote(scope)}&access_type=offline&prompt=consent&state={state_token}' | ||
| print(f'Generated Google Calendar OAuth URL for user {uid}') | ||
|
|
||
| elif app_key == 'whoop': | ||
| client_id = os.getenv('WHOOP_CLIENT_ID') | ||
| if not client_id: | ||
| print(f'ERROR: WHOOP_CLIENT_ID not configured for Whoop integration OAuth') | ||
| raise HTTPException(status_code=500, detail="Whoop not configured - WHOOP_CLIENT_ID missing") | ||
|
|
||
| # Remove trailing slash from base_url if present | ||
| base_url_clean = base_url.rstrip('/') | ||
| redirect_uri = f'{base_url_clean}/v2/integrations/whoop/callback' | ||
|
|
||
| # Whoop scopes: read:recovery, read:cycles, read:workout, read:sleep, read:profile, read:body_measurement | ||
| scopes = 'read:recovery read:cycles read:workout read:sleep read:profile read:body_measurement' | ||
| from urllib.parse import quote | ||
|
|
||
| auth_url = f'https://api.prod.whoop.com/oauth/oauth2/auth?client_id={client_id}&redirect_uri={quote(redirect_uri)}&response_type=code&scope={quote(scopes)}&state={state_token}' | ||
| print(f'Generated Whoop OAuth URL for user {uid}') | ||
|
|
||
| elif app_key == 'notion': | ||
| client_id = os.getenv('NOTION_CLIENT_ID') | ||
| if not client_id: | ||
| print(f'ERROR: NOTION_CLIENT_ID not configured for Notion integration OAuth') | ||
| raise HTTPException(status_code=500, detail="Notion not configured - NOTION_CLIENT_ID missing") | ||
|
|
||
| # Remove trailing slash from base_url if present | ||
| base_url_clean = base_url.rstrip('/') | ||
| redirect_uri = f'{base_url_clean}/v2/integrations/notion/callback' | ||
| from urllib.parse import quote | ||
|
|
||
| auth_url = f'https://api.notion.com/v1/oauth/authorize?client_id={client_id}&response_type=code&owner=user&redirect_uri={quote(redirect_uri)}&state={state_token}' | ||
| print(f'Generated Notion OAuth URL for user {uid}') | ||
|
|
There was a problem hiding this comment.
There is significant code duplication in the get_oauth_url function for generating the authorization URLs for Google Calendar, Whoop, and Notion. The logic for retrieving the client ID, base URL, and constructing the final URL is very similar for each provider. This can be refactored by using a configuration dictionary that holds the provider-specific details (e.g., client ID environment variable name, auth URL template, scopes), which would make the code more concise and easier to extend with new integrations.
backend/routers/integrations.py
Outdated
| @router.get( | ||
| '/v2/integrations/google-calendar/callback', | ||
| response_class=HTMLResponse, | ||
| tags=['integrations', 'oauth'], | ||
| ) | ||
| async def google_calendar_oauth_callback( | ||
| request: Request, | ||
| code: Optional[str] = Query(None), | ||
| state: Optional[str] = Query(None), | ||
| ): | ||
| """OAuth callback endpoint for Google Calendar integration.""" | ||
| client_id = os.getenv('GOOGLE_CLIENT_ID') | ||
| client_secret = os.getenv('GOOGLE_CLIENT_SECRET') | ||
| base_url = os.getenv('BASE_API_URL') | ||
|
|
||
| if not all([client_id, client_secret, base_url]): | ||
| return render_oauth_response(request, 'google_calendar', success=False, error_type='config_error') | ||
|
|
||
| # Remove trailing slash from base_url if present | ||
| base_url_clean = base_url.rstrip('/') | ||
| redirect_uri = f'{base_url_clean}/v2/integrations/google-calendar/callback' | ||
|
|
||
| config = OAuthProviderConfig( | ||
| token_endpoint='https://oauth2.googleapis.com/token', | ||
| token_request_type='form', | ||
| token_request_data={ | ||
| 'code': code, | ||
| 'client_id': client_id, | ||
| 'client_secret': client_secret, | ||
| 'redirect_uri': redirect_uri, | ||
| 'grant_type': 'authorization_code', | ||
| }, | ||
| ) | ||
|
|
||
| return await handle_oauth_callback(request, 'google_calendar', code, state, config) | ||
|
|
||
|
|
||
| @router.get( | ||
| '/v2/integrations/whoop/callback', | ||
| response_class=HTMLResponse, | ||
| tags=['integrations', 'oauth'], | ||
| ) | ||
| async def whoop_oauth_callback( | ||
| request: Request, | ||
| code: Optional[str] = Query(None), | ||
| state: Optional[str] = Query(None), | ||
| ): | ||
| """OAuth callback endpoint for Whoop integration.""" | ||
| client_id = os.getenv('WHOOP_CLIENT_ID') | ||
| client_secret = os.getenv('WHOOP_CLIENT_SECRET') | ||
| base_url = os.getenv('BASE_API_URL') | ||
|
|
||
| if not all([client_id, client_secret, base_url]): | ||
| return render_oauth_response(request, 'whoop', success=False, error_type='config_error') | ||
|
|
||
| # Remove trailing slash from base_url if present | ||
| base_url_clean = base_url.rstrip('/') | ||
| redirect_uri = f'{base_url_clean}/v2/integrations/whoop/callback' | ||
|
|
||
| config = OAuthProviderConfig( | ||
| token_endpoint='https://api.prod.whoop.com/oauth/oauth2/token', | ||
| token_request_type='form', | ||
| token_request_data={ | ||
| 'code': code, | ||
| 'client_id': client_id, | ||
| 'client_secret': client_secret, | ||
| 'redirect_uri': redirect_uri, | ||
| 'grant_type': 'authorization_code', | ||
| }, | ||
| ) | ||
|
|
||
| return await handle_oauth_callback(request, 'whoop', code, state, config) | ||
|
|
||
|
|
||
| @router.get( | ||
| '/v2/integrations/notion/callback', | ||
| response_class=HTMLResponse, | ||
| tags=['integrations', 'oauth'], | ||
| ) | ||
| async def notion_oauth_callback( | ||
| request: Request, | ||
| code: Optional[str] = Query(None), | ||
| state: Optional[str] = Query(None), | ||
| ): | ||
| """OAuth callback endpoint for Notion integration.""" | ||
| client_id = os.getenv('NOTION_CLIENT_ID') | ||
| client_secret = os.getenv('NOTION_CLIENT_SECRET') | ||
| base_url = os.getenv('BASE_API_URL') | ||
|
|
||
| if not all([client_id, client_secret, base_url]): | ||
| return render_oauth_response(request, 'notion', success=False, error_type='config_error') | ||
|
|
||
| # Remove trailing slash from base_url if present | ||
| base_url_clean = base_url.rstrip('/') | ||
| redirect_uri = f'{base_url_clean}/v2/integrations/notion/callback' | ||
|
|
||
| # Notion requires Basic Auth with base64(client_id:client_secret) in Authorization header | ||
| # and only grant_type, code, redirect_uri in the JSON body | ||
| credentials = f'{client_id}:{client_secret}' | ||
| encoded_credentials = base64.b64encode(credentials.encode()).decode() | ||
|
|
||
| config = OAuthProviderConfig( | ||
| token_endpoint='https://api.notion.com/v1/oauth/token', | ||
| token_request_type='json', # Use JSON instead of form | ||
| token_request_data={ | ||
| 'grant_type': 'authorization_code', | ||
| 'code': code, | ||
| 'redirect_uri': redirect_uri, | ||
| }, | ||
| additional_headers={ | ||
| 'Content-Type': 'application/json', | ||
| 'Authorization': f'Basic {encoded_credentials}', | ||
| }, | ||
| ) | ||
|
|
||
| return await handle_oauth_callback(request, 'notion', code, state, config) |
There was a problem hiding this comment.
The OAuth callback endpoints for Google Calendar, Whoop, and Notion are defined as three separate routes, leading to significant code duplication. The logic within each is nearly identical, only differing in the provider-specific configuration. This should be refactored into a single dynamic route, such as /v2/integrations/{app_key}/callback. The app_key from the path can then be used to retrieve the correct provider configuration and process the callback generically.
|
it's interesting, man. bwt, let's just make sure we don't add tools that are not relevant to users. for example, you might need 3, but i just need 1; other guys need 7, not all of these integrations. |
|
calendar, whoop and notion are all super popular apps, nothing very niche and exactly because of the reason you mentioned, we need to add as many tools as possible over time so that for every single user we have their use case covered lets try to build for the 99th percentile of users and not the median user (learning from wispr flow founder) |
krushnarout
changed the title
There was a problem hiding this comment.
Code Review
This pull request introduces several new tool integrations for the chat feature, including Whoop, Google Calendar, Notion, and Twitter. The changes span both the frontend and backend, adding new API endpoints, services, providers, and UI components for managing and displaying data from these integrations. While the new functionality is a great addition, the review identified several high-impact maintainability issues. These include significant code duplication in both the Dart frontend and Python backend, a fragile implementation for fetching integration statuses, and a missing token refresh mechanism for the Twitter integration. Addressing these points will be crucial for the long-term health and scalability of the codebase.
| class GoogleCalendarService { | ||
| static const String _appKey = 'google_calendar'; | ||
| static const String _prefKey = 'google_calendar_connected'; | ||
|
|
||
| bool get isAuthenticated { | ||
| // Check both preference and backend | ||
| return SharedPreferencesUtil().getBool(_prefKey) ?? false; | ||
| } | ||
|
|
||
| Future<void> refreshConnectionStatus() async { | ||
| await checkConnection(); | ||
| } | ||
|
|
||
| Future<bool> authenticate() async { | ||
| try { | ||
| final authUrl = await getIntegrationOAuthUrl(_appKey); | ||
| if (authUrl == null) { | ||
| debugPrint('Failed to get Google Calendar OAuth URL'); | ||
| return false; | ||
| } | ||
|
|
||
| final uri = Uri.parse(authUrl); | ||
| if (await canLaunchUrl(uri)) { | ||
| await launchUrl(uri, mode: LaunchMode.externalApplication); | ||
| // Note: OAuth callback will save connection to Firebase | ||
| // The connection status will be updated when user returns to the app | ||
| return true; | ||
| } else { | ||
| debugPrint('Cannot launch Google Calendar OAuth URL'); | ||
| return false; | ||
| } | ||
| } catch (e) { | ||
| debugPrint('Error during Google Calendar authentication: $e'); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| Future<bool> checkConnection() async { | ||
| try { | ||
| final response = await getIntegration(_appKey); | ||
| final isConnected = response != null && response.connected; | ||
| await SharedPreferencesUtil().saveBool(_prefKey, isConnected); | ||
| return isConnected; | ||
| } catch (e) { | ||
| debugPrint('Error checking Google Calendar connection: $e'); | ||
| await SharedPreferencesUtil().saveBool(_prefKey, false); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| Future<bool> disconnect() async { | ||
| try { | ||
| final success = await deleteIntegration(_appKey); | ||
| if (success) { | ||
| await SharedPreferencesUtil().saveBool(_prefKey, false); | ||
| return true; | ||
| } | ||
| return false; | ||
| } catch (e) { | ||
| debugPrint('Error disconnecting Google Calendar: $e'); | ||
| return false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The GoogleCalendarService duplicates all the logic from the BaseIntegrationService abstract class instead of extending it. This completely defeats the purpose of having a base class for common integration logic and introduces significant code duplication.
All other new services (NotionService, WhoopService, TwitterService) correctly extend BaseIntegrationService. This service should be refactored to do the same to improve maintainability and reduce redundancy.
import 'package:omi/services/base_integration_service.dart';
class GoogleCalendarService extends BaseIntegrationService {
static const String _appKey = 'google_calendar';
static const String _prefKey = 'google_calendar_connected';
GoogleCalendarService() : super(appKey: _appKey, prefKey: _prefKey);
}| Future<void> _connectApp(IntegrationApp app) async { | ||
| if (!app.isAvailable) { | ||
| return; | ||
| } | ||
|
|
||
| if (app == IntegrationApp.googleCalendar) { | ||
| final service = GoogleCalendarService(); | ||
| final handled = await _handleAuthFlow(app, service.isAuthenticated, service.authenticate); | ||
| if (handled) return; | ||
| } | ||
|
|
||
| if (app == IntegrationApp.whoop) { | ||
| final service = WhoopService(); | ||
| final handled = await _handleAuthFlow(app, service.isAuthenticated, service.authenticate); | ||
| if (handled) return; | ||
| } | ||
|
|
||
| if (app == IntegrationApp.notion) { | ||
| final service = NotionService(); | ||
| final handled = await _handleAuthFlow(app, service.isAuthenticated, service.authenticate); | ||
| if (handled) return; | ||
| } | ||
|
|
||
| if (app == IntegrationApp.twitter) { | ||
| final service = TwitterService(); | ||
| final handled = await _handleAuthFlow(app, service.isAuthenticated, service.authenticate); | ||
| if (handled) return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The _connectApp and _disconnectApp methods use sequential if statements to handle different integration types. This pattern is not easily scalable and leads to repetitive code. As more integrations are added, these chains will become longer and harder to maintain.
A better approach would be to use a more data-driven pattern, such as a Map that associates each IntegrationApp enum with its corresponding service instance. This would allow you to retrieve the correct service and call its methods (authenticate, disconnect) without a long chain of conditional statements, making the code cleaner and more extensible.
| final responses = await Future.wait([ | ||
| getIntegration('google_calendar'), | ||
| getIntegration('whoop'), | ||
| getIntegration('notion'), | ||
| getIntegration('twitter'), | ||
| ]); | ||
|
|
||
| _integrations['google_calendar'] = responses[0]?.connected ?? false; | ||
| _integrations['whoop'] = responses[1]?.connected ?? false; | ||
| _integrations['notion'] = responses[2]?.connected ?? false; | ||
| _integrations['twitter'] = responses[3]?.connected ?? false; |
There was a problem hiding this comment.
The loadFromBackend method uses Future.wait with a fixed list of futures and then assigns the results to the _integrations map using hardcoded indices. This is a fragile pattern; if the order of the getIntegration calls changes, it will lead to incorrect data being assigned to the wrong integration.
To make this more robust and maintainable, I suggest iterating over IntegrationApp.values to build the list of futures and then process the results. This ensures that the response is always mapped to the correct app key, regardless of order.
| final responses = await Future.wait([ | |
| getIntegration('google_calendar'), | |
| getIntegration('whoop'), | |
| getIntegration('notion'), | |
| getIntegration('twitter'), | |
| ]); | |
| _integrations['google_calendar'] = responses[0]?.connected ?? false; | |
| _integrations['whoop'] = responses[1]?.connected ?? false; | |
| _integrations['notion'] = responses[2]?.connected ?? false; | |
| _integrations['twitter'] = responses[3]?.connected ?? false; | |
| final apps = IntegrationApp.values; | |
| final responses = await Future.wait(apps.map((app) => getIntegration(app.key))); | |
| for (int i = 0; i < apps.length; i++) { | |
| _integrations[apps[i].key] = responses[i]?.connected ?? false; | |
| } |
| def refresh_google_token(uid: str, integration: dict) -> Optional[str]: | ||
| """ | ||
| Refresh Google access token using refresh token. | ||
| Works for both Calendar and Gmail since they use the same OAuth. | ||
| Returns: | ||
| New access token or None if refresh failed | ||
| """ | ||
| refresh_token = integration.get('refresh_token') | ||
| if not refresh_token: | ||
| return None | ||
|
|
||
| client_id = os.getenv('GOOGLE_CLIENT_ID') | ||
| client_secret = os.getenv('GOOGLE_CLIENT_SECRET') | ||
|
|
||
| if not all([client_id, client_secret]): | ||
| return None | ||
|
|
||
| try: | ||
| response = requests.post( | ||
| 'https://oauth2.googleapis.com/token', | ||
| data={ | ||
| 'client_id': client_id, | ||
| 'client_secret': client_secret, | ||
| 'refresh_token': refresh_token, | ||
| 'grant_type': 'refresh_token', | ||
| }, | ||
| timeout=10.0, | ||
| ) | ||
|
|
||
| if response.status_code == 200: | ||
| token_data = response.json() | ||
| new_access_token = token_data.get('access_token') | ||
|
|
||
| if new_access_token: | ||
| # Update stored token | ||
| integration['access_token'] = new_access_token | ||
| users_db.set_integration(uid, 'google_calendar', integration) | ||
| return new_access_token | ||
| except Exception as e: | ||
| print(f"Error refreshing Google token: {e}") | ||
|
|
||
| return None | ||
|
|
There was a problem hiding this comment.
The refresh_google_token function is duplicated here and in backend/utils/retrieval/tools/calendar_tools.py. Duplicating code for common functionality increases maintenance overhead and can lead to inconsistencies if one is updated and the other is not.
This function should be extracted into a shared utility module (e.g., backend/utils/retrieval/tools/google_utils.py) and imported by both gmail_tools.py and calendar_tools.py.
| @tool | ||
| def get_twitter_tweets_tool( | ||
| username: Optional[str] = None, | ||
| max_results: int = 10, | ||
| start_time: Optional[str] = None, | ||
| end_time: Optional[str] = None, | ||
| config: RunnableConfig = None, | ||
| ) -> str: | ||
| """ | ||
| Retrieve tweets from Twitter/X. | ||
| Use this tool when: | ||
| - User asks about their tweets or what they tweeted | ||
| - User asks about someone else's tweets (e.g., "what did @username tweet?") | ||
| - User wants to see recent tweets from a specific user | ||
| - User asks "show me tweets" or "what are my tweets?" | ||
| - **ALWAYS use this tool when the user asks about Twitter/X tweets** | ||
| Args: | ||
| username: Optional Twitter username (without @) to get tweets for. If not provided, returns authenticated user's tweets. | ||
| max_results: Maximum number of tweets to return (default: 10, max: 100) | ||
| start_time: Optional start time in ISO 8601 format (YYYY-MM-DDTHH:MM:SSZ) | ||
| end_time: Optional end time in ISO 8601 format (YYYY-MM-DDTHH:MM:SSZ) | ||
| Returns: | ||
| Formatted list of tweets with author, text, timestamp, and engagement metrics. | ||
| """ | ||
| print(f"🔧 get_twitter_tweets_tool called - username: {username}, max_results: {max_results}") | ||
|
|
||
| # Get config from parameter or context variable | ||
| if config is None: | ||
| try: | ||
| config = agent_config_context.get() | ||
| if config: | ||
| print(f"🔧 get_twitter_tweets_tool - got config from context variable") | ||
| except LookupError: | ||
| print(f"❌ get_twitter_tweets_tool - config not found in context variable") | ||
| config = None | ||
|
|
||
| if config is None: | ||
| print(f"❌ get_twitter_tweets_tool - config is None") | ||
| return "Error: Configuration not available" | ||
|
|
||
| try: | ||
| uid = config['configurable'].get('user_id') | ||
| except (KeyError, TypeError) as e: | ||
| print(f"❌ get_twitter_tweets_tool - error accessing config: {e}") | ||
| return "Error: Configuration not available" | ||
|
|
||
| if not uid: | ||
| print(f"❌ get_twitter_tweets_tool - no user_id in config") | ||
| return "Error: User ID not found in configuration" | ||
|
|
||
| print(f"✅ get_twitter_tweets_tool - uid: {uid}, max_results: {max_results}") | ||
|
|
||
| try: | ||
| # Cap at 100 per call | ||
| if max_results > 100: | ||
| print(f"⚠️ get_twitter_tweets_tool - max_results capped from {max_results} to 100") | ||
| max_results = 100 | ||
|
|
||
| # Check if user has Twitter connected | ||
| print(f"🐦 Checking Twitter connection for user {uid}...") | ||
| try: | ||
| integration = users_db.get_integration(uid, 'twitter') | ||
| print(f"🐦 Integration data retrieved: {integration is not None}") | ||
| if integration: | ||
| print(f"🐦 Integration connected status: {integration.get('connected')}") | ||
| print(f"🐦 Integration has access_token: {bool(integration.get('access_token'))}") | ||
| else: | ||
| print(f"❌ No integration found for user {uid}") | ||
| return "Twitter is not connected. Please connect your Twitter account from settings to view tweets." | ||
| except Exception as e: | ||
| print(f"❌ Error checking Twitter integration: {e}") | ||
| import traceback | ||
|
|
||
| traceback.print_exc() | ||
| return f"Error checking Twitter connection: {str(e)}" | ||
|
|
||
| if not integration or not integration.get('connected'): | ||
| print(f"❌ Twitter not connected for user {uid}") | ||
| return "Twitter is not connected. Please connect your Twitter account from settings to view tweets." | ||
|
|
||
| access_token = integration.get('access_token') | ||
| if not access_token: | ||
| print(f"❌ No access token found in integration data") | ||
| return "Twitter access token not found. Please reconnect your Twitter account from settings." | ||
|
|
||
| print(f"✅ Access token found, length: {len(access_token)}") | ||
|
|
||
| # Fetch tweets | ||
| try: | ||
| tweets_data = get_twitter_tweets( | ||
| access_token=access_token, | ||
| username=username, | ||
| max_results=max_results, | ||
| start_time=start_time, | ||
| end_time=end_time, | ||
| ) | ||
|
|
||
| print(f"✅ Successfully fetched Twitter tweets") | ||
| except Exception as e: | ||
| error_msg = str(e) | ||
| print(f"❌ Error fetching Twitter tweets: {error_msg}") | ||
| import traceback | ||
|
|
||
| traceback.print_exc() | ||
| return f"Error fetching tweets: {error_msg}" | ||
|
|
||
| tweets = tweets_data.get('data', []) | ||
| users = tweets_data.get('includes', {}).get('users', []) | ||
|
|
||
| # Create user lookup map | ||
| user_map = {user['id']: user for user in users} | ||
|
|
||
| tweets_count = len(tweets) if tweets else 0 | ||
| print(f"📊 get_twitter_tweets_tool - found {tweets_count} tweets") | ||
|
|
||
| if not tweets: | ||
| username_info = f" for @{username}" if username else "" | ||
| return f"No tweets found{username_info}." | ||
|
|
||
| # Format tweets | ||
| result = f"Twitter Tweets ({tweets_count} found):\n\n" | ||
|
|
||
| for i, tweet in enumerate(tweets, 1): | ||
| tweet_id = tweet.get('id', '') | ||
| text = tweet.get('text', '') | ||
| created_at = tweet.get('created_at', '') | ||
| author_id = tweet.get('author_id', '') | ||
| metrics = tweet.get('public_metrics', {}) | ||
|
|
||
| # Get author info | ||
| author_name = "Unknown" | ||
| author_username = "unknown" | ||
| if author_id and author_id in user_map: | ||
| author = user_map[author_id] | ||
| author_name = author.get('name', 'Unknown') | ||
| author_username = author.get('username', 'unknown') | ||
|
|
||
| # Format timestamp | ||
| try: | ||
| if created_at: | ||
| dt = datetime.fromisoformat(created_at.replace('Z', '+00:00')) | ||
| time_str = dt.strftime('%Y-%m-%d %H:%M:%S UTC') | ||
| else: | ||
| time_str = "Unknown time" | ||
| except Exception: | ||
| time_str = created_at if created_at else "Unknown time" | ||
|
|
||
| result += f"{i}. @{author_username} ({author_name})\n" | ||
| result += f" {text}\n" | ||
| result += f" Posted: {time_str}\n" | ||
|
|
||
| # Add engagement metrics | ||
| if metrics: | ||
| likes = metrics.get('like_count', 0) | ||
| retweets = metrics.get('retweet_count', 0) | ||
| replies = metrics.get('reply_count', 0) | ||
| if likes > 0 or retweets > 0 or replies > 0: | ||
| result += f" Likes: {likes}, Retweets: {retweets}, Replies: {replies}\n" | ||
|
|
||
| result += "\n" | ||
|
|
||
| return result.strip() | ||
| except Exception as e: | ||
| print(f"❌ Unexpected error in get_twitter_tweets_tool: {e}") | ||
| import traceback | ||
|
|
||
| traceback.print_exc() | ||
| return f"Unexpected error fetching tweets: {str(e)}" |
There was a problem hiding this comment.
The get_twitter_tweets_tool does not handle access token refresh. The Twitter OAuth 2.0 flow provides a refresh token, but there is no logic here to use it when an access token expires. This is inconsistent with the Google and Whoop tools, which do implement token refresh logic.
Without token refresh, the Twitter integration will fail permanently once the initial access token expires, requiring the user to re-authenticate. Please add token refresh handling to ensure the integration remains functional.
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces several new integrations (Google Calendar, Whoop, Notion, Twitter) and the framework to support them. The changes include new API services, providers, and UI pages for managing these integrations. The backend is also updated with new endpoints and tools for the agent to use. My review focuses on improving code maintainability, fixing potential race conditions with BuildContext, and optimizing data fetching logic. I've identified a few high-severity issues related to code duplication and incorrect asynchronous handling of BuildContext that could lead to runtime errors.
| Future<void> _handleNotionCallback() async { | ||
| if (!mounted) return; | ||
|
|
||
| try { | ||
| // Refresh connection status from backend | ||
| await NotionService().refreshConnectionStatus(); | ||
| await context.read<IntegrationProvider>().loadFromBackend(); | ||
|
|
||
| debugPrint('✓ Notion authentication completed successfully'); | ||
| AppSnackbar.showSnackbar('Successfully connected to Notion!'); | ||
| } catch (e) { | ||
| debugPrint('Error handling Notion callback: $e'); | ||
| AppSnackbar.showSnackbarError('Failed to refresh Notion connection status.'); | ||
| } | ||
| } | ||
|
|
||
| Future<void> _handleGoogleCalendarCallback() async { | ||
| if (!mounted) return; | ||
|
|
||
| try { | ||
| // Refresh connection status from backend | ||
| await GoogleCalendarService().refreshConnectionStatus(); | ||
| await context.read<IntegrationProvider>().loadFromBackend(); | ||
|
|
||
| debugPrint('✓ Google authentication completed successfully'); | ||
| AppSnackbar.showSnackbar('Successfully connected to Google!'); | ||
| } catch (e) { | ||
| debugPrint('Error handling Google Calendar callback: $e'); | ||
| AppSnackbar.showSnackbarError('Failed to refresh Google connection status.'); | ||
| } | ||
| } | ||
|
|
||
| Future<void> _handleWhoopCallback() async { | ||
| if (!mounted) return; | ||
|
|
||
| try { | ||
| // Refresh connection status from backend | ||
| await WhoopService().refreshConnectionStatus(); | ||
| await context.read<IntegrationProvider>().loadFromBackend(); | ||
|
|
||
| debugPrint('✓ Whoop authentication completed successfully'); | ||
| AppSnackbar.showSnackbar('Successfully connected to Whoop!'); | ||
| } catch (e) { | ||
| debugPrint('Error handling Whoop callback: $e'); | ||
| AppSnackbar.showSnackbarError('Failed to refresh Whoop connection status.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new callback handlers _handleNotionCallback, _handleGoogleCalendarCallback, and _handleWhoopCallback use context.read after an await call. This is an anti-pattern (use_build_context_synchronously) and can lead to a crash if the widget is unmounted during the asynchronous gap. You should capture the IntegrationProvider instance before the await to ensure it's safe to use.
The same issue exists in other callback handlers in this file, but this change should be applied to the new ones to prevent propagating the issue.
Future<void> _handleNotionCallback() async {
if (!mounted) return;
final integrationProvider = context.read<IntegrationProvider>();
try {
// Refresh connection status from backend
await NotionService().refreshConnectionStatus();
await integrationProvider.loadFromBackend();
debugPrint('✓ Notion authentication completed successfully');
AppSnackbar.showSnackbar('Successfully connected to Notion!');
} catch (e) {
debugPrint('Error handling Notion callback: $e');
AppSnackbar.showSnackbarError('Failed to refresh Notion connection status.');
}
}
Future<void> _handleGoogleCalendarCallback() async {
if (!mounted) return;
final integrationProvider = context.read<IntegrationProvider>();
try {
// Refresh connection status from backend
await GoogleCalendarService().refreshConnectionStatus();
await integrationProvider.loadFromBackend();
debugPrint('✓ Google authentication completed successfully');
AppSnackbar.showSnackbar('Successfully connected to Google!');
} catch (e) {
debugPrint('Error handling Google Calendar callback: $e');
AppSnackbar.showSnackbarError('Failed to refresh Google connection status.');
}
}
Future<void> _handleWhoopCallback() async {
if (!mounted) return;
final integrationProvider = context.read<IntegrationProvider>();
try {
// Refresh connection status from backend
await WhoopService().refreshConnectionStatus();
await integrationProvider.loadFromBackend();
debugPrint('✓ Whoop authentication completed successfully');
AppSnackbar.showSnackbar('Successfully connected to Whoop!');
} catch (e) {
debugPrint('Error handling Whoop callback: $e');
AppSnackbar.showSnackbarError('Failed to refresh Whoop connection status.');
}
}| Future<void> _loadFromBackend() async { | ||
| await context.read<IntegrationProvider>().loadFromBackend(); | ||
| // Also refresh the service's connection status | ||
| await GoogleCalendarService().refreshConnectionStatus(); | ||
| await WhoopService().refreshConnectionStatus(); | ||
| await NotionService().refreshConnectionStatus(); | ||
| await TwitterService().refreshConnectionStatus(); | ||
| } |
There was a problem hiding this comment.
The _loadFromBackend function makes redundant API calls. The call to context.read<IntegrationProvider>().loadFromBackend() already fetches the connection status for all integrations. The subsequent calls to refreshConnectionStatus() on each individual service perform the same getIntegration API call again.
This is inefficient and also points to a confusing state management pattern where both the IntegrationProvider and SharedPreferences (via services) are acting as sources of truth. This can lead to inconsistencies.
You should remove the redundant calls to refreshConnectionStatus() and rely on IntegrationProvider as the single source of truth for the UI. If services need the connection status, they should obtain it from the provider.
Future<void> _loadFromBackend() async {
await context.read<IntegrationProvider>().loadFromBackend();
}| Future<void> _handleDisconnect(IntegrationApp app, Future<bool> Function() disconnect) async { | ||
| final success = await disconnect(); | ||
| if (success) { | ||
| if (mounted) { | ||
| await context.read<IntegrationProvider>().deleteConnection(app.key); | ||
| } | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text('Disconnected from ${app.displayName}'), | ||
| duration: const Duration(seconds: 2), | ||
| ), | ||
| ); | ||
| } | ||
| } else { | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| const SnackBar( | ||
| content: Text('Failed to disconnect'), | ||
| backgroundColor: Colors.red, | ||
| duration: Duration(seconds: 3), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In _handleDisconnect, context.read and ScaffoldMessenger.of(context) are used after an await disconnect() call. This is an instance of the use_build_context_synchronously anti-pattern and can cause a crash if the widget is unmounted during the asynchronous operation.
To fix this, you should capture the IntegrationProvider and ScaffoldMessenger instances from the BuildContext before the await call.
Future<void> _handleDisconnect(IntegrationApp app, Future<bool> Function() disconnect) async {
if (!mounted) return;
final integrationProvider = context.read<IntegrationProvider>();
final scaffoldMessenger = ScaffoldMessenger.of(context);
final success = await disconnect();
if (!mounted) return;
if (success) {
await integrationProvider.deleteConnection(app.key);
scaffoldMessenger.showSnackBar(
SnackBar(
content: Text('Disconnected from ${app.displayName}'),
duration: const Duration(seconds: 2),
),
);
} else {
scaffoldMessenger.showSnackBar(
const SnackBar(
content: Text('Failed to disconnect'),
backgroundColor: Colors.red,
duration: Duration(seconds: 3),
),
);
}
}
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature, adding integrations for Google Calendar, Whoop, Notion, and Twitter. The backend work includes new API endpoints, OAuth flows, and agentic tools. The frontend includes a new integrations settings page and UI enhancements for the chat. The overall structure is good, with base classes for services and a provider for state management. However, I've identified several high-severity correctness issues in the new backend tools where API pagination is not handled, which could lead to incomplete data being returned. I've also found a significant code duplication in the frontend UI logic that should be refactored to improve maintainability.
| response = requests.get( | ||
| 'https://www.googleapis.com/calendar/v3/calendars/primary/events', | ||
| headers={'Authorization': f'Bearer {access_token}'}, | ||
| params=params, | ||
| timeout=10.0, | ||
| ) |
There was a problem hiding this comment.
The get_google_calendar_events function does not handle pagination. The Google Calendar API may return a nextPageToken if there are more results than maxResults. By not using this token to fetch subsequent pages, the tool may return an incomplete list of events, leading to incorrect or incomplete answers from the AI. This is a critical correctness issue.
Please update this function to loop through all pages of results using the nextPageToken until all events for the requested range are fetched or a reasonable upper limit is reached.
| response = requests.get( | ||
| 'https://www.googleapis.com/gmail/v1/users/me/messages', | ||
| headers={'Authorization': f'Bearer {access_token}'}, | ||
| params=params, | ||
| timeout=10.0, | ||
| ) |
There was a problem hiding this comment.
The get_gmail_messages function does not handle pagination. The Gmail API's messages.list endpoint returns a nextPageToken when there are more messages to fetch. This implementation only processes the first page of message IDs, which can lead to missed emails and incomplete search results. This is a critical correctness issue.
Please update this function to loop through all pages using the nextPageToken to collect all message IDs before proceeding to fetch their details.
|
|
||
| print(f"🐦 Calling Twitter Tweets API for user_id={user_id}, max_results={params['max_results']}") | ||
|
|
||
| try: |
There was a problem hiding this comment.
The get_twitter_tweets function does not handle pagination. The Twitter API v2 uses a pagination_token (available in the meta field of the response) to fetch subsequent pages of results. This implementation only fetches the first page, which will lead to incomplete data if a user has more tweets than the max_results limit for the given timeframe. This is a critical correctness issue.
Please update this function to check for meta['next_token'] in the response and make subsequent requests to fetch all pages of tweets.
| response = requests.get( | ||
| 'https://api.prod.whoop.com/developer/v2/activity/sleep', | ||
| headers={'Authorization': f'Bearer {access_token}'}, | ||
| params=params, | ||
| timeout=10.0, | ||
| ) |
There was a problem hiding this comment.
The Whoop data fetching functions (get_whoop_sleep_data, get_whoop_recovery_data, get_whoop_workout_data, and get_whoop_cycle_data) do not handle pagination. The Whoop API uses a next_token field for paginating through results. By only fetching the first page, these tools will return incomplete data if more records exist than the requested limit. This is a critical correctness issue.
Please update all data fetching functions in this file to implement a loop that checks for and uses the next_token to retrieve all pages of data for the requested time range.
NEW TOOLS