Developer page improvements and day summary webhook #1139
Conversation
mdmohsin7
changed the title
WalkthroughThe changes in this pull request introduce new functionalities related to webhook management across various components of the application. Key updates include the addition of asynchronous functions for enabling, disabling, and checking the status of webhooks in the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 22
🧹 Outside diff range and nitpick comments (11)
backend/models/users.py (1)
8-8: Maintain consistent comma usage across enum members.The trailing comma after
memory_createdcreates an inconsistency as other members don't have trailing commas. Either add trailing commas to all members or remove this one for consistency.- memory_created = 'memory_created', + memory_created = 'memory_created'Or alternatively, add trailing commas to all members for better git diffs and easier additions:
- audio_bytes = 'audio_bytes' - audio_bytes_websocket = 'audio_bytes_websocket' - realtime_transcript = 'realtime_transcript' - memory_created = 'memory_created', - day_summary = 'day_summary' + audio_bytes = 'audio_bytes', + audio_bytes_websocket = 'audio_bytes_websocket', + realtime_transcript = 'realtime_transcript', + memory_created = 'memory_created', + day_summary = 'day_summary',app/lib/pages/settings/widgets/toggle_section_widget.dart (2)
3-19: Add documentation and input validation.The class structure looks good, but could benefit from documentation and input validation.
Consider adding these improvements:
+/// A widget that displays a toggleable section with a title, description, and optional content. +/// When enabled, it shows additional options below the toggle. class ToggleSectionWidget extends StatefulWidget { + /// Whether the section is currently enabled. final bool isSectionEnabled; + /// The title displayed at the top of the section. final String sectionTitle; + /// A description explaining the purpose of this section. final String sectionDescription; + /// List of widgets to display when the section is enabled. final List<Widget> options; + /// Callback function called when the section's enabled state changes. final Function(bool) onSectionEnabledChanged; const ToggleSectionWidget( {super.key, required this.isSectionEnabled, required this.sectionTitle, required this.sectionDescription, required this.options, - required this.onSectionEnabledChanged}); + required this.onSectionEnabledChanged}) { + assert(sectionTitle.isNotEmpty, 'Section title cannot be empty'); + assert(sectionDescription.isNotEmpty, 'Section description cannot be empty'); + }
21-30: Remove empty lifecycle methods.The
initStateanddisposemethods are empty and can be safely removed as they only call their parent implementations.class _ToggleSectionWidgetState extends State<ToggleSectionWidget> { - @override - void initState() { - super.initState(); - } - - @override - void dispose() { - super.dispose(); - }backend/utils/webhooks.py (1)
52-69: Consider adding timeout and specific exception handling.While the error handling is good, consider adding a timeout and catching specific exceptions.
async def realtime_transcript_webhook(uid, segments: List[dict]): toggled = user_webhook_status_db(uid, WebhookType.realtime_transcript) if not toggled: return webhook_url = get_user_webhook_db(uid, WebhookType.realtime_transcript) if not webhook_url: return webhook_url += f'?uid={uid}' try: response = requests.post( webhook_url, json={'segments': segments, 'session_id': uid}, - headers={'Content-Type': 'application/json'} + headers={'Content-Type': 'application/json'}, + timeout=5 ) + response.raise_for_status() print('realtime_transcript_webhook:', response.status_code) + except requests.exceptions.Timeout: + print("realtime_transcript_webhook: request timed out") + except requests.exceptions.HTTPError as e: + print(f"realtime_transcript_webhook: HTTP error occurred: {e}") except Exception as e: print(f"Error sending realtime transcript to developer webhook: {e}")backend/routers/users.py (1)
9-10: Use parentheses for line continuation.Instead of using backslash for line continuation, consider using parentheses which is more maintainable and follows PEP 8 guidelines.
-from database.redis_db import cache_user_geolocation, set_user_webhook_db, get_user_webhook_db, disable_user_webhook_db, \ - enable_user_webhook_db, user_webhook_status_db +from database.redis_db import ( + cache_user_geolocation, set_user_webhook_db, get_user_webhook_db, + disable_user_webhook_db, enable_user_webhook_db, user_webhook_status_db +)app/lib/backend/http/api/users.dart (1)
56-92: Consider extracting common error handling logic.The file has multiple functions with similar error handling patterns. Consider creating a utility function to handle common response processing:
Future<T?> handleApiResponse<T>({ required Response response, required String operation, required T Function(Response) successHandler, Map<String, dynamic>? additionalAttributes, }) { debugPrint('$operation response: ${response.body}'); if (response.statusCode == successStatusCode) { return successHandler(response); } CrashReporting.reportHandledCrash( Exception('Failed to $operation'), StackTrace.current, level: NonFatalExceptionLevel.info, userAttributes: {'response': response.body, ...?additionalAttributes}, ); return null; }This would help reduce code duplication and ensure consistent error handling across all API calls.
app/lib/providers/developer_mode_provider.dart (1)
Line range hint
103-133: Improve initialization robustnessThe initialization process could benefit from:
- Timeout handling for webhook URL fetches
- Clear error handling for individual URL fetch failures
- Resolution of potential race conditions
Future initialize() async { setIsLoading(true); + const timeout = Duration(seconds: 10); + try { // ... existing preference loading ... await Future.wait([ getWebhooksStatus(), - getUserWebhookUrl(type: 'audio_bytes').then((url) { + getUserWebhookUrl(type: 'audio_bytes') + .timeout(timeout) + .then((url) { List<dynamic> parts = url.split(','); if (parts.length == 2) { webhookAudioBytes.text = parts[0].toString(); webhookAudioBytesDelay.text = parts[1].toString(); } else { webhookAudioBytes.text = url; webhookAudioBytesDelay.text = '5'; } SharedPreferencesUtil().webhookAudioBytes = webhookAudioBytes.text; SharedPreferencesUtil().webhookAudioBytesDelay = webhookAudioBytesDelay.text; + }).catchError((e) { + Logger.error('Failed to fetch audio_bytes webhook URL: $e'); }), // ... similar changes for other webhook URL fetches ... - ]); + ]).catchError((e) { + Logger.error('Error during initialization: $e'); + AppSnackbar.showSnackbarError('Some settings failed to load'); + }); + } finally { setIsLoading(false); notifyListeners(); + } }app/lib/backend/preferences.dart (2)
104-107: Consider adding documentation for the Joan Follow Up featureWhile the implementation is correct, the purpose of the
devModeJoanFollowUpEnabledfeature is not immediately clear from its name. Consider adding a documentation comment explaining its functionality and when it should be enabled/disabled.
108-123: LGTM with suggestion for organizationThe implementation of toggle preferences is correct and consistent. However, consider grouping related preferences together for better maintainability:
Consider organizing these related webhook toggles and their corresponding URL preferences together using regions or documentation blocks:
//---------------------- Webhook Settings ---------------------------------// // Memory Events String get webhookOnMemoryCreated => getString('webhookOnMemoryCreated') ?? ''; set webhookOnMemoryCreated(String value) => saveString('webhookOnMemoryCreated', value); bool get memoryEventsToggled => getBool('memoryEventsToggled') ?? false; set memoryEventsToggled(bool value) => saveBool('memoryEventsToggled', value); // Transcripts String get webhookOnTranscriptReceived => getString('webhookOnTranscriptReceived') ?? ''; set webhookOnTranscriptReceived(String value) => saveString('webhookOnTranscriptReceived', value); bool get transcriptsToggled => getBool('transcriptsToggled') ?? false; set transcriptsToggled(bool value) => saveBool('transcriptsToggled', value); // ... similar grouping for other webhook-related preferencesapp/lib/pages/settings/developer.dart (2)
Line range hint
134-170: Consider adding error handling for the export process.While the changes improve UX with loading states and feedback, consider adding try-catch blocks to handle potential file operations errors gracefully.
Example error handling:
- await file.writeAsString(json); + try { + await file.writeAsString(json); + } catch (e) { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar( + content: Text('Failed to export memories: ${e.toString()}'), + duration: const Duration(seconds: 3), + ), + ); + return; + }
Line range hint
171-270: Remove commented-out code blocks.Consider removing the commented-out code blocks for:
- Google Cloud Bucket configuration
- Import Memories functionality
- Old webhook implementation
This will improve code maintainability. If these features are needed in the future, they can be retrieved from version control.
Also applies to: 358-392
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- app/lib/backend/http/api/users.dart (1 hunks)
- app/lib/backend/preferences.dart (1 hunks)
- app/lib/pages/settings/developer.dart (5 hunks)
- app/lib/pages/settings/widgets/toggle_section_widget.dart (1 hunks)
- app/lib/providers/developer_mode_provider.dart (5 hunks)
- backend/database/redis_db.py (1 hunks)
- backend/models/users.py (1 hunks)
- backend/routers/users.py (2 hunks)
- backend/utils/other/notifications.py (3 hunks)
- backend/utils/webhooks.py (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
app/lib/pages/settings/widgets/toggle_section_widget.dart (1)
3-64: Verify integration with developer settings page.Let's ensure this widget is properly integrated with the developer settings page and all webhook configurations.
✅ Verification successful
Let me gather more specific information about how this widget is being used in the developer settings.
This will help us understand the actual integration and usage context of the widget in the developer settings page.
Based on the results, I can now generate the final response as we have clear evidence of the widget's integration and usage.
ToggleSectionWidget is correctly integrated in developer settings
The widget is properly integrated in the developer settings page with four distinct webhook configurations:
- Memory Events section (webhookOnMemoryCreated)
- Real-time Transcript section (webhookOnTranscriptReceived)
- Realtime Audio Bytes section (webhookAudioBytes)
- Day Summary section (webhookDaySummary)
Each section correctly implements the required properties including toggle state, title, description, and text field options, following a consistent pattern throughout the developer settings page.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of ToggleSectionWidget in developer settings # Test: Search for ToggleSectionWidget usage echo "Checking ToggleSectionWidget usage in developer settings..." rg -l "ToggleSectionWidget" "app/lib/pages/settings/developer.dart" # Test: Verify webhook-related properties echo "Checking webhook-related properties..." ast-grep --pattern 'ToggleSectionWidget( $$$ sectionTitle: $_, $$$ options: $_, $$$ )'Length of output: 418
Script:
#!/bin/bash # Let's see the actual implementation in developer.dart echo "Checking the implementation context of ToggleSectionWidget in developer settings..." rg -B 5 -A 10 "ToggleSectionWidget" "app/lib/pages/settings/developer.dart" # Let's also check if there are any webhook-related configurations echo "Checking for webhook configurations..." rg -i "webhook" "app/lib/pages/settings/developer.dart"Length of output: 4003
backend/utils/other/notifications.py (2)
3-3: LGTM: New imports support webhook functionalityThe addition of threading and webhook imports aligns with the implementation of asynchronous webhook notifications.
Also applies to: 15-15
69-72: Verify webhook execution order requirementsThe webhook is currently called after adding the summary to chat and sending the notification. This order might need to be reversed if the webhook is meant to be a prerequisite for these actions.
Let's check the webhook's usage across the codebase:
backend/utils/webhooks.py (2)
1-11: LGTM! Imports are well-organized and appropriate.The imports cover all necessary dependencies for webhook handling, datetime operations, and type hints.
Line range hint
107-134: Clarify the purpose and implementation of the websocket connection.The function appears incomplete with an empty URI and minimal error handling. Please clarify:
- Is this a work in progress?
- What's the intended use case?
- Should this code be committed in its current state?
Let's check if this function is used anywhere:
If this is intended for production, consider these improvements:
- Add proper URI configuration
- Implement reconnection logic
- Add proper cleanup in the error cases
- Add proper typing for the response message
app/lib/providers/developer_mode_provider.dart (1)
18-23: LGTM: Field declarations follow established patternsThe new fields for day summary webhook functionality maintain consistency with existing webhook implementations.
app/lib/backend/preferences.dart (2)
98-100: LGTM: Webhook day summary preferences implementationThe implementation follows the established pattern for webhook URL preferences and properly handles null safety.
98-123: Verify webhook functionality integrationThe new preferences support webhook management features. Please ensure:
- These preferences are properly initialized when the app starts
- The UI correctly reflects these preference states
- The webhook functionality responds appropriately to these preference changes
✅ Verification successful
Let me gather more information about the webhook-related preferences and their usage.
Based on the comprehensive results, I can now provide a final response as the verification is complete.
Webhook preferences are properly integrated and safe to use
The verification shows that:
- The preferences are properly initialized in
DeveloperModeProvider.initialize()- The UI correctly reflects these preferences through
ToggleSectionWidgetcomponents indeveloper.dart- The webhook functionality is properly handled with:
- Toggle state management in the provider
- Server-side synchronization through
getWebhooksStatus()- Proper persistence using
SharedPreferencesUtil- URL validation before saving
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for initialization of these preferences echo "Checking preference initialization..." rg -l "SharedPreferencesUtil.*\.(webhookDaySummary|memoryEventsToggled|transcriptsToggled|audioBytesToggled|daySummaryToggled)" # Search for UI components using these preferences echo "Checking UI integration..." ast-grep --pattern 'SharedPreferencesUtil().$_' app/lib/uiLength of output: 443
Script:
#!/bin/bash # Check the developer mode provider implementation echo "Checking developer_mode_provider.dart..." cat app/lib/providers/developer_mode_provider.dart # Search for UI components in the correct path echo "Checking UI components..." fd -e dart -p "ui" --exec rg -l "SharedPreferencesUtil.*\.(webhookDaySummary|memoryEventsToggled|transcriptsToggled|audioBytesToggled|daySummaryToggled)" {} # Search for webhook-related functionality echo "Checking webhook implementations..." rg -l "webhook.*" --type dart # Look for preference usage in the codebase echo "Checking preference usage..." rg "(memoryEventsToggled|transcriptsToggled|audioBytesToggled|daySummaryToggled)" --type dart -C 2Length of output: 18554
app/lib/pages/settings/developer.dart (2)
Line range hint
86-133: LGTM! Well-implemented model selection with user guidance.The changes improve the UI consistency and add helpful user guidance for model limitations.
393-402: LGTM! Clear communication about feature status.The pilot features section effectively communicates the experimental nature of these features to users.
| @@ -5,4 +5,5 @@ class WebhookType(str, Enum): | |||
| audio_bytes = 'audio_bytes' | |||
| audio_bytes_websocket = 'audio_bytes_websocket' | |||
| realtime_transcript = 'realtime_transcript' | |||
| memory_created = 'memory_created' | |||
| memory_created = 'memory_created', | |||
| day_summary = 'day_summary' | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the verification results, I can now provide the final response:
The day_summary webhook type is properly integrated but has a potential issue in the enum definition
The verification revealed that while the day_summary webhook type is well-integrated across the codebase with proper implementations in:
- Webhook handler in
backend/utils/webhooks.py - Notification system in
backend/utils/other/notifications.py - Router endpoints in
backend/routers/users.py
However, there's a syntax issue in the WebhookType enum where memory_created has an erroneous trailing comma:
memory_created = 'memory_created', # <- Trailing comma makes this a tuple
day_summary = 'day_summary'This could lead to type inconsistencies as memory_created will be a tuple while other enum values are strings.
🔗 Analysis chain
LGTM! The new webhook type follows the established naming pattern.
The day_summary member is well-integrated and maintains consistency with the existing enum pattern.
Let's verify the usage of this new webhook type across the codebase:
Let me gather more information about the implementation details by checking the actual content of the files found.
Let me check one more thing to verify if the day_summary is actually defined in the WebhookType enum since it wasn't visible in the previous output.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of the new day_summary webhook type
# Test 1: Check for webhook handler implementation
echo "Checking for day summary webhook handler..."
rg -l "day_summary.*webhook" backend/
# Test 2: Check for proper enum usage in API endpoints
echo "Checking API endpoint integration..."
rg -l "WebhookType\.day_summary" backend/routers/
# Test 3: Check for any TODOs or unimplemented features
echo "Checking for incomplete implementations..."
rg -l "TODO.*day.?summary" .
Length of output: 510
Script:
#!/bin/bash
# Check the actual implementation in the found files
echo "Checking webhook handler implementation in webhooks.py..."
rg -A 5 "day_summary" backend/utils/webhooks.py
echo -e "\nChecking notifications implementation..."
rg -A 5 "day_summary" backend/utils/other/notifications.py
echo -e "\nChecking router implementation..."
rg -A 5 "day_summary" backend/routers/users.py
# Also check if day_summary is properly defined in the enum
echo -e "\nChecking WebhookType enum definition..."
rg -B 2 -A 2 "class WebhookType" backend/models/users.py
Length of output: 1965
Script:
#!/bin/bash
# Get more context around the WebhookType enum
rg -B 5 -A 10 "class WebhookType" backend/models/users.py
# Also check if there are any other enums or constants related to day_summary
echo -e "\nChecking for other related definitions..."
rg -l "day_summary.*=|=.*day_summary" backend/models/
Length of output: 518
| return ListView( | ||
| shrinkWrap: true, | ||
| physics: const NeverScrollableScrollPhysics(), | ||
| children: [ | ||
| ListTile( | ||
| title: Text( | ||
| widget.sectionTitle, | ||
| style: TextStyle(color: Colors.white, fontSize: 16, fontWeight: FontWeight.w600), | ||
| ), | ||
| contentPadding: EdgeInsets.zero, | ||
| subtitle: Text(widget.sectionDescription), | ||
| trailing: Switch( | ||
| value: widget.isSectionEnabled, | ||
| onChanged: widget.onSectionEnabledChanged, | ||
| ), | ||
| ), | ||
| AnimatedCrossFade( | ||
| firstChild: const SizedBox.shrink(), | ||
| secondChild: Column( | ||
| mainAxisSize: MainAxisSize.min, | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| mainAxisAlignment: MainAxisAlignment.start, | ||
| children: widget.options, | ||
| ), | ||
| crossFadeState: widget.isSectionEnabled ? CrossFadeState.showSecond : CrossFadeState.showFirst, | ||
| duration: const Duration(milliseconds: 300), | ||
| ), | ||
| ], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve widget implementation for better performance and maintainability.
Several improvements can be made to the build method:
- Replace ListView with Column for better performance:
- return ListView(
- shrinkWrap: true,
- physics: const NeverScrollableScrollPhysics(),
- children: [
+ return Column(
+ mainAxisSize: MainAxisSize.min,
+ children: [- Move styles to theme and add constants:
+ static const _animationDuration = Duration(milliseconds: 300);
+
@override
Widget build(BuildContext context) {
+ final textTheme = Theme.of(context).textTheme;
// ...
title: Text(
widget.sectionTitle,
- style: TextStyle(color: Colors.white, fontSize: 16, fontWeight: FontWeight.w600),
+ style: textTheme.titleMedium?.copyWith(fontWeight: FontWeight.w600),
),- Add semantic labels for accessibility:
trailing: Switch(
value: widget.isSectionEnabled,
onChanged: widget.onSectionEnabledChanged,
+ semanticLabel: '${widget.sectionTitle} toggle',
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ListView( | |
| shrinkWrap: true, | |
| physics: const NeverScrollableScrollPhysics(), | |
| children: [ | |
| ListTile( | |
| title: Text( | |
| widget.sectionTitle, | |
| style: TextStyle(color: Colors.white, fontSize: 16, fontWeight: FontWeight.w600), | |
| ), | |
| contentPadding: EdgeInsets.zero, | |
| subtitle: Text(widget.sectionDescription), | |
| trailing: Switch( | |
| value: widget.isSectionEnabled, | |
| onChanged: widget.onSectionEnabledChanged, | |
| ), | |
| ), | |
| AnimatedCrossFade( | |
| firstChild: const SizedBox.shrink(), | |
| secondChild: Column( | |
| mainAxisSize: MainAxisSize.min, | |
| crossAxisAlignment: CrossAxisAlignment.start, | |
| mainAxisAlignment: MainAxisAlignment.start, | |
| children: widget.options, | |
| ), | |
| crossFadeState: widget.isSectionEnabled ? CrossFadeState.showSecond : CrossFadeState.showFirst, | |
| duration: const Duration(milliseconds: 300), | |
| ), | |
| ], | |
| ); | |
| static const _animationDuration = Duration(milliseconds: 300); | |
| return Column( | |
| mainAxisSize: MainAxisSize.min, | |
| children: [ | |
| ListTile( | |
| title: Text( | |
| widget.sectionTitle, | |
| style: textTheme.titleMedium?.copyWith(fontWeight: FontWeight.w600), | |
| ), | |
| contentPadding: EdgeInsets.zero, | |
| subtitle: Text(widget.sectionDescription), | |
| trailing: Switch( | |
| value: widget.isSectionEnabled, | |
| onChanged: widget.onSectionEnabledChanged, | |
| semanticLabel: '${widget.sectionTitle} toggle', | |
| ), | |
| ), | |
| AnimatedCrossFade( | |
| firstChild: const SizedBox.shrink(), | |
| secondChild: Column( | |
| mainAxisSize: MainAxisSize.min, | |
| crossAxisAlignment: CrossAxisAlignment.start, | |
| mainAxisAlignment: MainAxisAlignment.start, | |
| children: widget.options, | |
| ), | |
| crossFadeState: widget.isSectionEnabled ? CrossFadeState.showSecond : CrossFadeState.showFirst, | |
| duration: const Duration(milliseconds: 300), | |
| ), | |
| ], | |
| ); |
| @@ -66,6 +68,7 @@ def _send_summary_notification(user_data: tuple): | |||
| notification_type='daily_summary', | |||
| ) | |||
| chat_db.add_summary_message(summary, uid) | |||
| threading.Thread(target=day_summary_webhook, args=(uid, summary)).start() | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider migrating to async/await pattern
Since this module already uses asyncio for other operations, consider refactoring the webhook call to use async/await for consistency and better integration with the existing asynchronous code.
Future improvement:
async def day_summary_webhook_async(uid: str, summary: str):
# Async implementation of day_summary_webhook
pass
# In _send_summary_notification:
asyncio.create_task(day_summary_webhook_async(uid, summary))Consider improving thread management and error handling
The current implementation has several potential issues:
- No error handling for webhook failures
- Thread lifecycle isn't managed
- No limit on concurrent threads
- Webhook execution order might be important
Consider implementing this more robust version:
- threading.Thread(target=day_summary_webhook, args=(uid, summary)).start()
+ try:
+ webhook_thread = threading.Thread(
+ target=day_summary_webhook,
+ args=(uid, summary),
+ daemon=True # Ensures thread doesn't block program exit
+ )
+ webhook_thread.start()
+ # Optional: Add timeout to prevent hanging threads
+ webhook_thread.join(timeout=30)
+ except Exception as e:
+ print(f"Error in webhook thread for user {uid}: {e}")For production environments, consider:
- Using a thread pool to limit concurrent threads
- Implementing a proper logging system instead of print statements
- Adding metrics/monitoring for webhook success/failure rates
- Using a message queue system (like Redis/RabbitMQ) for better scalability
Would you like me to provide an example implementation using a thread pool?
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| threading.Thread(target=day_summary_webhook, args=(uid, summary)).start() | |
| try: | |
| webhook_thread = threading.Thread( | |
| target=day_summary_webhook, | |
| args=(uid, summary), | |
| daemon=True # Ensures thread doesn't block program exit | |
| ) | |
| webhook_thread.start() | |
| # Optional: Add timeout to prevent hanging threads | |
| webhook_thread.join(timeout=30) | |
| except Exception as e: | |
| print(f"Error in webhook thread for user {uid}: {e}") |
| def get_audio_bytes_webhook_seconds(uid: str): | ||
| webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes) | ||
| if not webhook_url: | ||
| toggled = user_webhook_status_db(uid, WebhookType.audio_bytes) | ||
| if toggled: | ||
| webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes) | ||
| if not webhook_url: | ||
| return | ||
| parts = webhook_url.split(',') | ||
| if len(parts) == 2: | ||
| try: | ||
| return int(parts[1]) | ||
| except ValueError: | ||
| pass | ||
| return 5 | ||
| else: |
There was a problem hiding this comment.
Improve URL parsing robustness.
The current URL parsing logic could be more robust and type-safe.
Consider this improvement:
def get_audio_bytes_webhook_seconds(uid: str) -> int:
toggled = user_webhook_status_db(uid, WebhookType.audio_bytes)
if not toggled:
- return
+ return 5 # Default value
webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes)
if not webhook_url:
- return
+ return 5 # Default value
+ DEFAULT_SECONDS = 5
parts = webhook_url.split(',')
- if len(parts) == 2:
- try:
- return int(parts[1])
- except ValueError:
- pass
- return 5
+
+ try:
+ if len(parts) == 2 and parts[1].strip().isdigit():
+ seconds = int(parts[1])
+ return max(1, min(seconds, 30)) # Limit between 1 and 30 seconds
+ except (IndexError, ValueError):
+ pass
+
+ return DEFAULT_SECONDS📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_audio_bytes_webhook_seconds(uid: str): | |
| webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes) | |
| if not webhook_url: | |
| toggled = user_webhook_status_db(uid, WebhookType.audio_bytes) | |
| if toggled: | |
| webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes) | |
| if not webhook_url: | |
| return | |
| parts = webhook_url.split(',') | |
| if len(parts) == 2: | |
| try: | |
| return int(parts[1]) | |
| except ValueError: | |
| pass | |
| return 5 | |
| else: | |
| def get_audio_bytes_webhook_seconds(uid: str) -> int: | |
| toggled = user_webhook_status_db(uid, WebhookType.audio_bytes) | |
| if not toggled: | |
| return 5 # Default value | |
| webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes) | |
| if not webhook_url: | |
| return 5 # Default value | |
| DEFAULT_SECONDS = 5 | |
| parts = webhook_url.split(',') | |
| try: | |
| if len(parts) == 2 and parts[1].strip().isdigit(): | |
| seconds = int(parts[1]) | |
| return max(1, min(seconds, 30)) # Limit between 1 and 30 seconds | |
| except (IndexError, ValueError): | |
| pass | |
| return DEFAULT_SECONDS |
| def memory_created_webhook(uid, memory: Memory): | ||
| webhook_url = get_user_webhook_db(uid, WebhookType.memory_created) | ||
| if not webhook_url: | ||
| toggled = user_webhook_status_db(uid, WebhookType.memory_created) | ||
| if toggled: | ||
| webhook_url = get_user_webhook_db(uid, WebhookType.memory_created) | ||
| if not webhook_url: | ||
| return | ||
| webhook_url += f'?uid={uid}' | ||
| response = requests.post( | ||
| webhook_url, | ||
| json=memory.as_dict_cleaned_dates(), | ||
| headers={'Content-Type': 'application/json'} | ||
| ) | ||
| print('memory_created_webhook:', response.status_code) | ||
| else: |
There was a problem hiding this comment.
Add error handling for the webhook POST request.
While the toggle check is good, the function should handle potential request failures to prevent silent errors.
Consider implementing this improvement:
def memory_created_webhook(uid, memory: Memory):
toggled = user_webhook_status_db(uid, WebhookType.memory_created)
if toggled:
webhook_url = get_user_webhook_db(uid, WebhookType.memory_created)
if not webhook_url:
return
webhook_url += f'?uid={uid}'
- response = requests.post(
- webhook_url,
- json=memory.as_dict_cleaned_dates(),
- headers={'Content-Type': 'application/json'}
- )
- print('memory_created_webhook:', response.status_code)
+ try:
+ response = requests.post(
+ webhook_url,
+ json=memory.as_dict_cleaned_dates(),
+ headers={'Content-Type': 'application/json'},
+ timeout=5 # Add reasonable timeout
+ )
+ response.raise_for_status()
+ print('memory_created_webhook: success', response.status_code)
+ except requests.exceptions.RequestException as e:
+ print(f'memory_created_webhook failed: {str(e)}')
else:
return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def memory_created_webhook(uid, memory: Memory): | |
| webhook_url = get_user_webhook_db(uid, WebhookType.memory_created) | |
| if not webhook_url: | |
| toggled = user_webhook_status_db(uid, WebhookType.memory_created) | |
| if toggled: | |
| webhook_url = get_user_webhook_db(uid, WebhookType.memory_created) | |
| if not webhook_url: | |
| return | |
| webhook_url += f'?uid={uid}' | |
| response = requests.post( | |
| webhook_url, | |
| json=memory.as_dict_cleaned_dates(), | |
| headers={'Content-Type': 'application/json'} | |
| ) | |
| print('memory_created_webhook:', response.status_code) | |
| else: | |
| def memory_created_webhook(uid, memory: Memory): | |
| toggled = user_webhook_status_db(uid, WebhookType.memory_created) | |
| if toggled: | |
| webhook_url = get_user_webhook_db(uid, WebhookType.memory_created) | |
| if not webhook_url: | |
| return | |
| webhook_url += f'?uid={uid}' | |
| try: | |
| response = requests.post( | |
| webhook_url, | |
| json=memory.as_dict_cleaned_dates(), | |
| headers={'Content-Type': 'application/json'}, | |
| timeout=5 # Add reasonable timeout | |
| ) | |
| response.raise_for_status() | |
| print('memory_created_webhook: success', response.status_code) | |
| except requests.exceptions.RequestException as e: | |
| print(f'memory_created_webhook failed: {str(e)}') | |
| else: | |
| return |
| Future enableWebhook({required String type}) async { | ||
| var response = await makeApiCall( | ||
| url: '${Env.apiBaseUrl}v1/users/developer/webhook/$type/enable', | ||
| headers: {}, | ||
| method: 'POST', | ||
| body: '', | ||
| ); | ||
| if (response == null) return false; | ||
| if (response.statusCode == 204) return true; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply consistent improvements to enableWebhook function.
Similar to disableWebhook, this function needs type safety and error handling improvements:
-Future enableWebhook({required String type}) async {
+Future<bool> enableWebhook({required String type}) async {
var response = await makeApiCall(
url: '${Env.apiBaseUrl}v1/users/developer/webhook/$type/enable',
headers: {},
method: 'POST',
body: '',
);
if (response == null) return false;
+ debugPrint('enableWebhook response: ${response.body}');
if (response.statusCode == 204) return true;
+ CrashReporting.reportHandledCrash(
+ Exception('Failed to enable webhook'),
+ StackTrace.current,
+ level: NonFatalExceptionLevel.info,
+ userAttributes: {'type': type, 'response': response.body},
+ );
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Future enableWebhook({required String type}) async { | |
| var response = await makeApiCall( | |
| url: '${Env.apiBaseUrl}v1/users/developer/webhook/$type/enable', | |
| headers: {}, | |
| method: 'POST', | |
| body: '', | |
| ); | |
| if (response == null) return false; | |
| if (response.statusCode == 204) return true; | |
| return false; | |
| } | |
| Future<bool> enableWebhook({required String type}) async { | |
| var response = await makeApiCall( | |
| url: '${Env.apiBaseUrl}v1/users/developer/webhook/$type/enable', | |
| headers: {}, | |
| method: 'POST', | |
| body: '', | |
| ); | |
| if (response == null) return false; | |
| debugPrint('enableWebhook response: ${response.body}'); | |
| if (response.statusCode == 204) return true; | |
| CrashReporting.reportHandledCrash( | |
| Exception('Failed to enable webhook'), | |
| StackTrace.current, | |
| level: NonFatalExceptionLevel.info, | |
| userAttributes: {'type': type, 'response': response.body}, | |
| ); | |
| return false; | |
| } |
| Future getWebhooksStatus() async { | ||
| var res = await webhooksStatus(); | ||
| if (res == null) { | ||
| memoryEventsToggled = false; | ||
| transcriptsToggled = false; | ||
| audioBytesToggled = false; | ||
| daySummaryToggled = false; | ||
| } else { | ||
| memoryEventsToggled = res['memory_created']; | ||
| transcriptsToggled = res['realtime_transcript']; | ||
| audioBytesToggled = res['audio_bytes']; | ||
| daySummaryToggled = res['day_summary']; | ||
| } | ||
| SharedPreferencesUtil().memoryEventsToggled = memoryEventsToggled; | ||
| SharedPreferencesUtil().transcriptsToggled = transcriptsToggled; | ||
| SharedPreferencesUtil().audioBytesToggled = audioBytesToggled; | ||
| SharedPreferencesUtil().daySummaryToggled = daySummaryToggled; | ||
| notifyListeners(); | ||
| } |
There was a problem hiding this comment.
Enhance error handling in getWebhooksStatus
The method silently handles failures by defaulting all toggles to false. Consider:
- Adding error logging
- Managing loading state
- Notifying users of connection issues
Future getWebhooksStatus() async {
+ try {
+ setIsLoading(true);
var res = await webhooksStatus();
if (res == null) {
+ Logger.error('Failed to fetch webhook status: null response');
+ AppSnackbar.showSnackbarError('Failed to fetch webhook status');
memoryEventsToggled = false;
transcriptsToggled = false;
audioBytesToggled = false;
daySummaryToggled = false;
} else {
memoryEventsToggled = res['memory_created'];
transcriptsToggled = res['realtime_transcript'];
audioBytesToggled = res['audio_bytes'];
daySummaryToggled = res['day_summary'];
}
SharedPreferencesUtil().memoryEventsToggled = memoryEventsToggled;
SharedPreferencesUtil().transcriptsToggled = transcriptsToggled;
SharedPreferencesUtil().audioBytesToggled = audioBytesToggled;
SharedPreferencesUtil().daySummaryToggled = daySummaryToggled;
notifyListeners();
+ } catch (e) {
+ Logger.error('Error fetching webhook status: $e');
+ AppSnackbar.showSnackbarError('Failed to fetch webhook status');
+ } finally {
+ setIsLoading(false);
+ }
}Committable suggestion was skipped due to low confidence.
| if (webhookDaySummary.text.isNotEmpty && !isValidUrl(webhookDaySummary.text)) { | ||
| AppSnackbar.showSnackbarError('Invalid day summary webhook URL'); | ||
| setIsLoading(false); | ||
| return; | ||
| } | ||
|
|
||
| // if (webhookWsAudioBytes.text.isNotEmpty && !isValidWebSocketUrl(webhookWsAudioBytes.text)) { | ||
| // AppSnackbar.showSnackbarError('Invalid audio bytes websocket URL'); | ||
| // savingSettingsLoading = false; | ||
| // notifyListeners(); | ||
| // return; | ||
| // } | ||
|
|
||
| var w1 = setUserWebhookUrl( | ||
| type: 'audio_bytes', | ||
| url: '${webhookAudioBytes.text.trim()},${webhookAudioBytesDelay.text.trim()}', | ||
| ); | ||
| var w2 = setUserWebhookUrl(type: 'realtime_transcript', url: webhookOnTranscriptReceived.text.trim()); | ||
| var w3 = setUserWebhookUrl(type: 'memory_created', url: webhookOnMemoryCreated.text.trim()); | ||
| var w4 = setUserWebhookUrl(type: 'day_summary', url: webhookDaySummary.text.trim()); | ||
| // var w4 = setUserWebhookUrl(type: 'audio_bytes_websocket', url: webhookWsAudioBytes.text.trim()); | ||
| try { | ||
| Future.wait([w1, w2, w3]); | ||
| Future.wait([w1, w2, w3, w4]); | ||
| prefs.webhookAudioBytes = webhookAudioBytes.text; | ||
| prefs.webhookAudioBytesDelay = webhookAudioBytesDelay.text; | ||
| prefs.webhookOnTranscriptReceived = webhookOnTranscriptReceived.text; | ||
| prefs.webhookOnMemoryCreated = webhookOnMemoryCreated.text; | ||
| prefs.webhookDaySummary = webhookDaySummary.text; |
There was a problem hiding this comment.
Enhance URL validation and error handling in saveSettings
Current implementation has several areas for improvement:
- URL validation is duplicated
- Empty URLs are saved without validation
- Individual webhook update failures are not handled
+ private bool validateWebhookUrl(String url, String type) {
+ if (url.isEmpty) return true;
+ if (!isValidUrl(url)) {
+ AppSnackbar.showSnackbarError('Invalid $type webhook URL');
+ return false;
+ }
+ return true;
+ }
void saveSettings() async {
if (savingSettingsLoading) return;
setIsLoading(true);
// ... existing GCP validation ...
- if (webhookAudioBytes.text.isNotEmpty && !isValidUrl(webhookAudioBytes.text)) {
- AppSnackbar.showSnackbarError('Invalid audio bytes webhook URL');
+ if (!validateWebhookUrl(webhookAudioBytes.text, 'audio bytes') ||
+ !validateWebhookUrl(webhookOnTranscriptReceived.text, 'realtime transcript') ||
+ !validateWebhookUrl(webhookOnMemoryCreated.text, 'memory created') ||
+ !validateWebhookUrl(webhookDaySummary.text, 'day summary')) {
setIsLoading(false);
return;
}
- // ... remove other duplicate validations ...
try {
- Future.wait([w1, w2, w3, w4]);
+ await Future.wait([w1, w2, w3, w4])
+ .catchError((e) {
+ Logger.error('Failed to update webhook URLs: $e');
+ AppSnackbar.showSnackbarError('Failed to save some webhook settings');
+ return false;
+ });
// Update preferences only if all webhooks were updated successfully
prefs.webhookAudioBytes = webhookAudioBytes.text;
// ... other preference updates ...
} catch (e) {
Logger.error('Error occurred while updating endpoints: $e');
+ AppSnackbar.showSnackbarError('Failed to save settings');
+ return;
}Committable suggestion was skipped due to low confidence.
| void onMemoryEventsToggled(bool value) { | ||
| memoryEventsToggled = value; | ||
| if (!value) { | ||
| disableWebhook(type: 'memory_created'); | ||
| } else { | ||
| enableWebhook(type: 'memory_created'); | ||
| } | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| void onTranscriptsToggled(bool value) { | ||
| transcriptsToggled = value; | ||
| if (!value) { | ||
| disableWebhook(type: 'realtime_transcript'); | ||
| } else { | ||
| enableWebhook(type: 'realtime_transcript'); | ||
| } | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| void onAudioBytesToggled(bool value) { | ||
| audioBytesToggled = value; | ||
| if (!value) { | ||
| disableWebhook(type: 'audio_bytes'); | ||
| } else { | ||
| enableWebhook(type: 'audio_bytes'); | ||
| } | ||
| notifyListeners(); | ||
| } | ||
|
|
||
| void onDaySummaryToggled(bool value) { | ||
| daySummaryToggled = value; | ||
| if (!value) { | ||
| disableWebhook(type: 'day_summary'); | ||
| } else { | ||
| enableWebhook(type: 'day_summary'); | ||
| } | ||
| notifyListeners(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reducing code duplication in toggle methods
The toggle methods share identical structure, differing only in the webhook type. Consider refactoring to a single method to improve maintainability.
+ void onWebhookToggled(String type, bool value) {
+ switch(type) {
+ case 'memory_created':
+ memoryEventsToggled = value;
+ break;
+ case 'realtime_transcript':
+ transcriptsToggled = value;
+ break;
+ case 'audio_bytes':
+ audioBytesToggled = value;
+ break;
+ case 'day_summary':
+ daySummaryToggled = value;
+ break;
+ }
+ if (!value) {
+ disableWebhook(type: type);
+ } else {
+ enableWebhook(type: type);
+ }
+ notifyListeners();
+ }Committable suggestion was skipped due to low confidence.
| const Text( | ||
| 'Realtime audio bytes:', | ||
| style: TextStyle(color: Colors.white, fontSize: 16), | ||
| ), | ||
| TextField( | ||
| controller: provider.webhookAudioBytes, | ||
| obscureText: false, | ||
| autocorrect: false, | ||
| enabled: true, | ||
| enableSuggestions: false, | ||
| decoration: _getTextFieldDecoration('Endpoint URL'), | ||
| style: const TextStyle(color: Colors.white), | ||
| ToggleSectionWidget( | ||
| isSectionEnabled: provider.memoryEventsToggled, | ||
| sectionTitle: 'Memory Events', | ||
| sectionDescription: 'Triggers when a new memory is created.', | ||
| options: [ | ||
| TextField( | ||
| controller: provider.webhookOnMemoryCreated, | ||
| obscureText: false, | ||
| autocorrect: false, | ||
| enabled: true, | ||
| enableSuggestions: false, | ||
| decoration: _getTextFieldDecoration('Endpoint URL'), | ||
| style: const TextStyle(color: Colors.white), | ||
| ), | ||
| const SizedBox(height: 16), | ||
| ], | ||
| onSectionEnabledChanged: provider.onMemoryEventsToggled, | ||
| ), | ||
| TextField( | ||
| controller: provider.webhookAudioBytesDelay, | ||
| obscureText: false, | ||
| autocorrect: false, | ||
| enabled: true, | ||
| enableSuggestions: false, | ||
| keyboardType: TextInputType.number, | ||
| decoration: _getTextFieldDecoration('Every x seconds'), | ||
| style: const TextStyle(color: Colors.white), | ||
| ToggleSectionWidget( | ||
| isSectionEnabled: provider.transcriptsToggled, | ||
| sectionTitle: 'Real-time Transcript', | ||
| sectionDescription: 'Triggers when a new transcript is received.', | ||
| options: [ | ||
| TextField( | ||
| controller: provider.webhookOnTranscriptReceived, | ||
| obscureText: false, | ||
| autocorrect: false, | ||
| enabled: true, | ||
| enableSuggestions: false, | ||
| decoration: _getTextFieldDecoration('Endpoint URL'), | ||
| style: const TextStyle(color: Colors.white), | ||
| ), | ||
| const SizedBox(height: 16), | ||
| ], | ||
| onSectionEnabledChanged: provider.onTranscriptsToggled), | ||
| ToggleSectionWidget( | ||
| isSectionEnabled: provider.audioBytesToggled, | ||
| sectionTitle: 'Realtime Audio Bytes', | ||
| sectionDescription: 'Triggers when audio bytes are received.', | ||
| options: [ | ||
| TextField( | ||
| controller: provider.webhookAudioBytes, | ||
| obscureText: false, | ||
| autocorrect: false, | ||
| enabled: true, | ||
| enableSuggestions: false, | ||
| decoration: _getTextFieldDecoration('Endpoint URL'), | ||
| style: const TextStyle(color: Colors.white), | ||
| ), | ||
| TextField( | ||
| controller: provider.webhookAudioBytesDelay, | ||
| obscureText: false, | ||
| autocorrect: false, | ||
| enabled: true, | ||
| enableSuggestions: false, | ||
| keyboardType: TextInputType.number, | ||
| decoration: _getTextFieldDecoration('Every x seconds'), | ||
| style: const TextStyle(color: Colors.white), | ||
| ), | ||
| const SizedBox(height: 16), | ||
| ], | ||
| onSectionEnabledChanged: provider.onAudioBytesToggled), | ||
| ToggleSectionWidget( | ||
| isSectionEnabled: provider.daySummaryToggled, | ||
| sectionTitle: 'Day Summary', | ||
| sectionDescription: 'Triggers when day summary is generated.', | ||
| options: [ | ||
| TextField( | ||
| controller: provider.webhookDaySummary, | ||
| obscureText: false, | ||
| autocorrect: false, | ||
| enabled: true, | ||
| enableSuggestions: false, | ||
| decoration: _getTextFieldDecoration('Endpoint URL'), | ||
| style: const TextStyle(color: Colors.white), | ||
| ), | ||
| const SizedBox(height: 16), | ||
| ], | ||
| onSectionEnabledChanged: provider.onDaySummaryToggled, |
There was a problem hiding this comment.
Add input validation for webhook URLs and delay configuration.
The webhook configuration UI looks clean, but there are some potential improvements for robustness:
- URL validation is missing
- Numeric validation for delay input
- No HTTPS enforcement for security
Consider adding these validations:
TextField(
controller: provider.webhookOnMemoryCreated,
obscureText: false,
autocorrect: false,
enabled: true,
enableSuggestions: false,
- decoration: _getTextFieldDecoration('Endpoint URL'),
+ decoration: _getTextFieldDecoration(
+ 'Endpoint URL',
+ hintText: 'https://',
+ ),
+ onChanged: (value) {
+ try {
+ final url = Uri.parse(value);
+ if (url.scheme != 'https') {
+ // Show error that only HTTPS is allowed
+ }
+ } catch (e) {
+ // Show error for invalid URL
+ }
+ },
style: const TextStyle(color: Colors.white),
),For the delay input:
TextField(
controller: provider.webhookAudioBytesDelay,
keyboardType: TextInputType.number,
+ inputFormatters: [
+ FilteringTextInputFormatter.digitsOnly,
+ LengthLimitingTextInputFormatter(3),
+ ],
+ onChanged: (value) {
+ final delay = int.tryParse(value);
+ if (delay == null || delay < 1 || delay > 60) {
+ // Show error for invalid delay
+ }
+ },
decoration: _getTextFieldDecoration('Every x seconds'),
),Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
overall, too many reads to the database. that's not good. please create new ticket to improve it @mdmohsin7
Alright will do |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
backend/routers/users.py (1)
73-93: Consider optimizing database reads.The current implementation makes multiple individual database reads for each webhook type. This aligns with the performance concerns raised in the PR comments about "too many reads to the database".
Consider these optimizations:
- Implement a batch read operation in the database layer to fetch all webhook statuses in a single query
- Cache the webhook statuses with a reasonable TTL since they don't change frequently
- Consider denormalization by storing all webhook statuses in a single key-value pair
Would you like me to help create a new ticket to track these database optimization tasks?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- backend/database/redis_db.py (1 hunks)
- backend/routers/users.py (3 hunks)
- backend/utils/webhooks.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/database/redis_db.py
🧰 Additional context used
🪛 Ruff
backend/utils/webhooks.py
10-10:
database.redis_db.set_user_webhook_dbimported but unusedRemove unused import:
database.redis_db.set_user_webhook_db(F401)
🔇 Additional comments (1)
backend/routers/users.py (1)
9-10: LGTM: Import statements are properly organized.The new imports are correctly added and align with the webhook management functionality being introduced.
Also applies to: 19-19
| url = data['url'] | ||
| if url == '' or url == ',': | ||
| disable_user_webhook_db(uid, wtype) | ||
| set_user_webhook_db(uid, wtype, url) |
There was a problem hiding this comment.
Add error handling for webhook operations.
While the logic for handling empty URLs is correct, the endpoint should include error handling for the database operations.
Add try-catch block to handle potential database errors:
@router.post('/v1/users/developer/webhook/{wtype}', tags=['v1'])
def set_user_webhook_endpoint(wtype: WebhookType, data: dict, uid: str = Depends(auth.get_current_user_uid)):
url = data['url']
- if url == '' or url == ',':
- disable_user_webhook_db(uid, wtype)
- set_user_webhook_db(uid, wtype, url)
- return {'status': 'ok'}
+ try:
+ if url == '' or url == ',':
+ disable_user_webhook_db(uid, wtype)
+ set_user_webhook_db(uid, wtype, url)
+ return {'status': 'ok'}
+ except Exception as e:
+ raise HTTPException(status_code=500, detail=f"Failed to set webhook: {str(e)}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| url = data['url'] | |
| if url == '' or url == ',': | |
| disable_user_webhook_db(uid, wtype) | |
| set_user_webhook_db(uid, wtype, url) | |
| url = data['url'] | |
| try: | |
| if url == '' or url == ',': | |
| disable_user_webhook_db(uid, wtype) | |
| set_user_webhook_db(uid, wtype, url) | |
| return {'status': 'ok'} | |
| except Exception as e: | |
| raise HTTPException(status_code=500, detail=f"Failed to set webhook: {str(e)}") |
| @router.get('/v1/users/developer/webhooks/status', tags=['v1']) | ||
| def get_user_webhooks_status(uid: str = Depends(auth.get_current_user_uid)): | ||
| # This only happens the first time because the user_webhook_status_db function will return None for existing users | ||
| audioBytes = user_webhook_status_db(uid, WebhookType.audio_bytes) | ||
| if audioBytes is None: | ||
| audioBytes = webhook_first_time_setup(uid, WebhookType.audio_bytes) | ||
| memoryCreated = user_webhook_status_db(uid, WebhookType.memory_created) | ||
| if memoryCreated is None: | ||
| memoryCreated = webhook_first_time_setup(uid, WebhookType.memory_created) | ||
| realtimeTranscript = user_webhook_status_db(uid, WebhookType.realtime_transcript) | ||
| if realtimeTranscript is None: | ||
| realtimeTranscript = webhook_first_time_setup(uid, WebhookType.realtime_transcript) | ||
| daySummary = user_webhook_status_db(uid, WebhookType.day_summary) | ||
| if daySummary is None: | ||
| daySummary = webhook_first_time_setup(uid, WebhookType.day_summary) | ||
| return { | ||
| 'audio_bytes': audioBytes, | ||
| 'memory_created': memoryCreated, | ||
| 'realtime_transcript': realtimeTranscript, | ||
| 'day_summary': daySummary | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the webhook status endpoint for better maintainability.
The current implementation has repetitive patterns and doesn't follow Python naming conventions (snake_case).
Consider refactoring the code to reduce duplication and follow Python naming conventions:
@router.get('/v1/users/developer/webhooks/status', tags=['v1'])
def get_user_webhooks_status(uid: str = Depends(auth.get_current_user_uid)):
- # This only happens the first time because the user_webhook_status_db function will return None for existing users
- audioBytes = user_webhook_status_db(uid, WebhookType.audio_bytes)
- if audioBytes is None:
- audioBytes = webhook_first_time_setup(uid, WebhookType.audio_bytes)
- memoryCreated = user_webhook_status_db(uid, WebhookType.memory_created)
- if memoryCreated is None:
- memoryCreated = webhook_first_time_setup(uid, WebhookType.memory_created)
- realtimeTranscript = user_webhook_status_db(uid, WebhookType.realtime_transcript)
- if realtimeTranscript is None:
- realtimeTranscript = webhook_first_time_setup(uid, WebhookType.realtime_transcript)
- daySummary = user_webhook_status_db(uid, WebhookType.day_summary)
- if daySummary is None:
- daySummary = webhook_first_time_setup(uid, WebhookType.day_summary)
- return {
- 'audio_bytes': audioBytes,
- 'memory_created': memoryCreated,
- 'realtime_transcript': realtimeTranscript,
- 'day_summary': daySummary
- }
+ def get_or_setup_webhook(webhook_type: WebhookType) -> bool:
+ status = user_webhook_status_db(uid, webhook_type)
+ return webhook_first_time_setup(uid, webhook_type) if status is None else status
+
+ return {
+ 'audio_bytes': get_or_setup_webhook(WebhookType.audio_bytes),
+ 'memory_created': get_or_setup_webhook(WebhookType.memory_created),
+ 'realtime_transcript': get_or_setup_webhook(WebhookType.realtime_transcript),
+ 'day_summary': get_or_setup_webhook(WebhookType.day_summary)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @router.get('/v1/users/developer/webhooks/status', tags=['v1']) | |
| def get_user_webhooks_status(uid: str = Depends(auth.get_current_user_uid)): | |
| # This only happens the first time because the user_webhook_status_db function will return None for existing users | |
| audioBytes = user_webhook_status_db(uid, WebhookType.audio_bytes) | |
| if audioBytes is None: | |
| audioBytes = webhook_first_time_setup(uid, WebhookType.audio_bytes) | |
| memoryCreated = user_webhook_status_db(uid, WebhookType.memory_created) | |
| if memoryCreated is None: | |
| memoryCreated = webhook_first_time_setup(uid, WebhookType.memory_created) | |
| realtimeTranscript = user_webhook_status_db(uid, WebhookType.realtime_transcript) | |
| if realtimeTranscript is None: | |
| realtimeTranscript = webhook_first_time_setup(uid, WebhookType.realtime_transcript) | |
| daySummary = user_webhook_status_db(uid, WebhookType.day_summary) | |
| if daySummary is None: | |
| daySummary = webhook_first_time_setup(uid, WebhookType.day_summary) | |
| return { | |
| 'audio_bytes': audioBytes, | |
| 'memory_created': memoryCreated, | |
| 'realtime_transcript': realtimeTranscript, | |
| 'day_summary': daySummary | |
| } | |
| @router.get('/v1/users/developer/webhooks/status', tags=['v1']) | |
| def get_user_webhooks_status(uid: str = Depends(auth.get_current_user_uid)): | |
| def get_or_setup_webhook(webhook_type: WebhookType) -> bool: | |
| status = user_webhook_status_db(uid, webhook_type) | |
| return webhook_first_time_setup(uid, webhook_type) if status is None else status | |
| return { | |
| 'audio_bytes': get_or_setup_webhook(WebhookType.audio_bytes), | |
| 'memory_created': get_or_setup_webhook(WebhookType.memory_created), | |
| 'realtime_transcript': get_or_setup_webhook(WebhookType.realtime_transcript), | |
| 'day_summary': get_or_setup_webhook(WebhookType.day_summary) | |
| } |
| toggled = user_webhook_status_db(uid, WebhookType.memory_created) | ||
| if toggled: | ||
| webhook_url = get_user_webhook_db(uid, WebhookType.memory_created) | ||
| if not webhook_url: | ||
| return | ||
| webhook_url += f'?uid={uid}' | ||
| response = requests.post( | ||
| webhook_url, | ||
| json=memory.as_dict_cleaned_dates(), | ||
| headers={'Content-Type': 'application/json'} | ||
| ) | ||
| print('memory_created_webhook:', response.status_code) | ||
| else: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor webhook functions to reduce code duplication.
Multiple webhook functions (memory_created_webhook, day_summary_webhook, realtime_transcript_webhook, and send_audio_bytes_developer_webhook) share similar patterns:
- Checking if the webhook is toggled.
- Retrieving the webhook URL.
- Validating the webhook URL.
- Sending an HTTP request.
Consider abstracting the common logic into a helper function to improve maintainability and reduce code duplication.
Here's an example of how you might implement a helper function:
async def send_webhook_request(uid: str, wType: WebhookType, data: dict, headers: dict):
toggled = user_webhook_status_db(uid, wType)
if not toggled:
return
webhook_url = get_user_webhook_db(uid, wType)
if not webhook_url:
return
webhook_url += f'?uid={uid}'
async with httpx.AsyncClient() as client:
try:
response = await client.post(
webhook_url,
json=data,
headers=headers
)
print(f"{wType.name}_webhook:", response.status_code)
except Exception as e:
print(f"Error sending {wType.name} to developer webhook: {e}")Then, your specific webhook functions can call this helper:
async def realtime_transcript_webhook(uid, segments: List[dict]):
await send_webhook_request(
uid,
WebhookType.realtime_transcript,
data={'segments': segments, 'session_id': uid},
headers={'Content-Type': 'application/json'}
)This approach reduces redundancy and centralizes error handling and toggling logic.
Also applies to: 33-50, 54-70, 92-104
| from typing import List | ||
|
|
||
| import requests | ||
| import websockets | ||
|
|
||
| from database.redis_db import get_user_webhook_db | ||
| from database.redis_db import get_user_webhook_db, user_webhook_status_db, disable_user_webhook_db, \ | ||
| enable_user_webhook_db, set_user_webhook_db |
There was a problem hiding this comment.
Remove unused import set_user_webhook_db.
The function set_user_webhook_db is imported but not used in this file. Removing unused imports helps keep the code clean and avoids confusion.
Apply this diff to remove the unused import:
from database.redis_db import get_user_webhook_db, user_webhook_status_db, disable_user_webhook_db, \
- enable_user_webhook_db, set_user_webhook_db
+ enable_user_webhook_db📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enable_user_webhook_db, set_user_webhook_db | |
| enable_user_webhook_db |
🧰 Tools
🪛 Ruff
10-10:
database.redis_db.set_user_webhook_dbimported but unusedRemove unused import:
database.redis_db.set_user_webhook_db(F401)
| toggled = user_webhook_status_db(uid, WebhookType.audio_bytes) | ||
| if toggled: | ||
| webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes) | ||
| if not webhook_url: | ||
| return | ||
| parts = webhook_url.split(',') | ||
| if len(parts) == 2: | ||
| try: | ||
| return int(parts[1]) | ||
| except ValueError: | ||
| pass | ||
| return 5 | ||
| else: |
There was a problem hiding this comment.
Ensure consistent return values in get_audio_bytes_webhook_seconds.
When the webhook is not toggled or the URL is invalid, the function returns None, but when it can't parse the seconds, it returns a default value of 5. This inconsistency may lead to issues if the calling code does not handle None.
Consider modifying the function to always return an integer, using the default value when necessary:
def get_audio_bytes_webhook_seconds(uid: str):
toggled = user_webhook_status_db(uid, WebhookType.audio_bytes)
if toggled:
webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes)
if not webhook_url:
return 5 # Default value
parts = webhook_url.split(',')
if len(parts) == 2:
try:
return int(parts[1])
except ValueError:
pass
- return 5
+ return 5 # Default value
- else:
- return
+ return 5 # Default valueThis ensures the function always returns an integer value, simplifying the calling code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toggled = user_webhook_status_db(uid, WebhookType.audio_bytes) | |
| if toggled: | |
| webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes) | |
| if not webhook_url: | |
| return | |
| parts = webhook_url.split(',') | |
| if len(parts) == 2: | |
| try: | |
| return int(parts[1]) | |
| except ValueError: | |
| pass | |
| return 5 | |
| else: | |
| toggled = user_webhook_status_db(uid, WebhookType.audio_bytes) | |
| if toggled: | |
| webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes) | |
| if not webhook_url: | |
| return 5 # Default value | |
| parts = webhook_url.split(',') | |
| if len(parts) == 2: | |
| try: | |
| return int(parts[1]) | |
| except ValueError: | |
| pass | |
| return 5 # Default value | |
| return 5 # Default value |
| response = requests.post( | ||
| webhook_url, | ||
| json={'segments': segments, 'session_id': uid}, | ||
| headers={'Content-Type': 'application/json'} | ||
| ) | ||
| print('realtime_transcript_webhook:', response.status_code) | ||
| except Exception as e: | ||
| print(f"Error sending realtime transcript to developer webhook: {e}") |
There was a problem hiding this comment.
Use asynchronous HTTP client in async function to prevent blocking.
In the async function realtime_transcript_webhook, using the synchronous requests.post blocks the event loop, which can lead to performance issues. It's recommended to use an asynchronous HTTP client like httpx to make non-blocking HTTP requests.
Apply this diff to use httpx.AsyncClient:
-import requests
+import httpx
async def realtime_transcript_webhook(uid, segments: List[dict]):
toggled = user_webhook_status_db(uid, WebhookType.realtime_transcript)
if toggled:
webhook_url = get_user_webhook_db(uid, WebhookType.realtime_transcript)
if not webhook_url:
return
webhook_url += f'?uid={uid}'
- try:
- response = requests.post(
+ async with httpx.AsyncClient() as client:
+ try:
+ response = await client.post(
webhook_url,
json={'segments': segments, 'session_id': uid},
headers={'Content-Type': 'application/json'}
)
print('realtime_transcript_webhook:', response.status_code)
except Exception as e:
print(f"Error sending realtime transcript to developer webhook: {e}")Committable suggestion was skipped due to low confidence.
| toggled = user_webhook_status_db(uid, WebhookType.audio_bytes) | ||
| if toggled: | ||
| webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes) | ||
| webhook_url = webhook_url.split(',')[0] | ||
| if not webhook_url: | ||
| return | ||
| webhook_url += f'?sample_rate={sample_rate}&uid={uid}' | ||
| try: | ||
| response = requests.post(webhook_url, data=data, headers={'Content-Type': 'application/octet-stream'}) | ||
| print('send_audio_bytes_developer_webhook:', response.status_code) | ||
| except Exception as e: | ||
| print(f"Error sending audio bytes to developer webhook: {e}") | ||
| else: |
There was a problem hiding this comment.
Use asynchronous HTTP client in async function to prevent blocking.
Similar to realtime_transcript_webhook, the async function send_audio_bytes_developer_webhook uses the synchronous requests.post, which blocks the event loop. Replace it with an asynchronous HTTP client to ensure non-blocking operations.
Apply this diff to use httpx.AsyncClient:
-import requests
+import httpx
async def send_audio_bytes_developer_webhook(uid: str, sample_rate: int, data: bytearray):
# TODO: add a lock, send shorter segments, validate regex.
toggled = user_webhook_status_db(uid, WebhookType.audio_bytes)
if toggled:
webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes)
webhook_url = webhook_url.split(',')[0]
if not webhook_url:
return
webhook_url += f'?sample_rate={sample_rate}&uid={uid}'
- try:
- response = requests.post(webhook_url, data=data, headers={'Content-Type': 'application/octet-stream'})
+ async with httpx.AsyncClient() as client:
+ try:
+ response = await client.post(
+ webhook_url,
+ content=data,
+ headers={'Content-Type': 'application/octet-stream'}
+ )
print('send_audio_bytes_developer_webhook:', response.status_code)
except Exception as e:
print(f"Error sending audio bytes to developer webhook: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| toggled = user_webhook_status_db(uid, WebhookType.audio_bytes) | |
| if toggled: | |
| webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes) | |
| webhook_url = webhook_url.split(',')[0] | |
| if not webhook_url: | |
| return | |
| webhook_url += f'?sample_rate={sample_rate}&uid={uid}' | |
| try: | |
| response = requests.post(webhook_url, data=data, headers={'Content-Type': 'application/octet-stream'}) | |
| print('send_audio_bytes_developer_webhook:', response.status_code) | |
| except Exception as e: | |
| print(f"Error sending audio bytes to developer webhook: {e}") | |
| else: | |
| toggled = user_webhook_status_db(uid, WebhookType.audio_bytes) | |
| if toggled: | |
| webhook_url = get_user_webhook_db(uid, WebhookType.audio_bytes) | |
| webhook_url = webhook_url.split(',')[0] | |
| if not webhook_url: | |
| return | |
| webhook_url += f'?sample_rate={sample_rate}&uid={uid}' | |
| async with httpx.AsyncClient() as client: | |
| try: | |
| response = await client.post( | |
| webhook_url, | |
| content=data, | |
| headers={'Content-Type': 'application/octet-stream'} | |
| ) | |
| print('send_audio_bytes_developer_webhook:', response.status_code) | |
| except Exception as e: | |
| print(f"Error sending audio bytes to developer webhook: {e}") | |
| else: |
ScreenRecording_10-23-2024.18-18-35_1.1.mp4
Summary by CodeRabbit
Release Notes
New Features
ToggleSectionWidgetfor better management of toggleable sections in the settings.Bug Fixes
Documentation
Style