Conversation
WalkthroughThe changes in this pull request involve significant updates across multiple files, primarily enhancing the functionality related to app submission and plugin management. Key modifications include new API endpoints, improved error handling, and the introduction of new classes and methods for managing app data. A new form for app submission has been implemented, allowing immediate testing for submitters while maintaining a review process for broader access. Additionally, several utility functions for validation and data retrieval have been added, streamlining the overall architecture. Changes
Assessment against linked issues
Possibly related PRs
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: 43
🧹 Outside diff range and nitpick comments (42)
app/lib/utils/alerts/app_snackbar.dart (1)
Line range hint
1-29: Consider improving testability and null safety.The class's dependency on
MyApp.navigatorKeymakes it challenging to test and could potentially throw runtime errors. Consider these improvements:
- Inject the
BuildContextinstead of using a global key:class AppSnackbar { static void showSnackbar( BuildContext context, String message, { Color? color, Duration? duration, }) { if (message.isEmpty) return; ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text(message), backgroundColor: color, duration: duration ?? const Duration(seconds: 2), ), ); } static void showSnackbarError( BuildContext context, String message, { Duration? duration, }) { showSnackbar( context, message, color: Colors.red, duration: duration, ); } static void showSnackbarSuccess( BuildContext context, String message, { Duration? duration, }) { showSnackbar( context, message, color: Colors.green, duration: duration, ); } }
- Alternatively, consider using a service-based approach with dependency injection:
abstract class SnackbarService { void showSnackbar(String message, {Color? color, Duration? duration}); void showError(String message, {Duration? duration}); void showSuccess(String message, {Duration? duration}); } class AppSnackbarService implements SnackbarService { final GlobalKey<NavigatorState> navigatorKey; AppSnackbarService(this.navigatorKey); @override void showSnackbar(String message, {Color? color, Duration? duration}) { final context = navigatorKey.currentState?.context; if (context == null || message.isEmpty) return; ScaffoldMessenger.of(context).showSnackBar(/*...*/); } // Implement other methods... }app/lib/utils/other/validators.dart (1)
1-20: Consider implementing a more robust validation frameworkThe current implementation could benefit from a more structured approach:
- Consider creating a
Validatorclass to encapsulate these methods and provide detailed validation results- Add specific error messages for different validation failures
- Implement unit tests covering edge cases
Example structure:
class ValidationResult { final bool isValid; final String? errorMessage; const ValidationResult(this.isValid, [this.errorMessage]); } class Validator { static ValidationResult validateUrl(String url) { // Implementation } static ValidationResult validateEmail(String email) { // Implementation } // ... other validation methods }Would you like me to help create comprehensive unit tests for these validators?
app/lib/pages/apps/widgets/prompt_text_field.dart (1)
3-7: Consider making the widget more generic and reusable.The widget name
PromptTextFieldand validation message suggest it's specifically for prompts, but it appears to be a generic text field component. Consider renaming it to something likeCustomTextFieldand making the validation message configurable.-class PromptTextField extends StatelessWidget { +class CustomTextField extends StatelessWidget { final TextEditingController controller; final String label; final IconData icon; - const PromptTextField({super.key, required this.controller, required this.label, required this.icon}); + final String? validationMessage; + const CustomTextField({ + super.key, + required this.controller, + required this.label, + required this.icon, + this.validationMessage = 'Please enter a valid value', + });app/lib/pages/apps/widgets/capabilities_chips_widget.dart (1)
12-15: Consider using Row instead of ListView.Since you have a fixed number of chips (3), using a
Rowwidget would be more appropriate and performant than aListView. WhileListViewwithshrinkWrap: trueworks, it adds unnecessary overhead for this use case.- return ListView( - shrinkWrap: true, - scrollDirection: Axis.horizontal, - children: [ + return Row( + children: [app/lib/backend/http/api/messages.dart (2)
62-64: Consider error message improvement.The error handling is basic but functional. Consider enhancing the error message to include the plugin_id for better debugging.
Here's a suggested improvement:
- throw Exception('Failed to send message'); + throw Exception('Failed to get initial message for plugin: $appId');
Line range hint
9-14: Add pagination to message retrieval.The TODO comment indicates missing pagination. This could lead to performance issues with large message histories.
Would you like me to help implement pagination for the message retrieval endpoint? This would include:
- Adding limit/offset parameters
- Implementing cursor-based pagination
- Adding proper response metadata
app/lib/providers/app_provider.dart (4)
Line range hint
89-127: Add bounds checking in toggleApp methodThe
toggleAppmethod assumesidxis valid but doesn't verify this assumption, which could lead to runtime errors.Add validation at the start of the method:
Future<void> toggleApp(String appId, bool isEnabled, int idx) async { + if (idx < 0 || idx >= appLoading.length) { + AppDialog.show( + title: 'Error', + content: 'Invalid app index', + singleButton: true, + ); + return; + } if (appLoading[idx]) return;
50-50: Refactor duplicate initialization logicThe
appLoadinglist initialization is duplicated in bothgetAppsandinitializemethods.Consider extracting this into a private method:
+ void _initializeAppLoading() { + appLoading = List.filled(apps.length, false); + }Then call this method instead of duplicating the initialization.
Also applies to: 82-82
Line range hint
1-14: Consider implementing proper disposal of resourcesThe provider manages state but doesn't implement disposal logic. This could lead to memory leaks, especially with the growing
appLoadinglist.Add disposal logic:
+ @override + void dispose() { + apps.clear(); + appLoading.clear(); + super.dispose(); + }
Line range hint
89-127: Improve error handling in toggleAppThe error handling in
toggleAppcould be more informative and consistent.Consider enhancing the error dialog:
- AppDialog.show( - title: 'Error activating the app', - content: 'If this is an integration app, make sure the setup is completed.', - singleButton: true, - ); + AppDialog.show( + title: 'Error Activating ${apps[idx].name}', + content: ''' + Unable to activate the app. Please ensure: + - The app setup is complete + - You have necessary permissions + - The server is responding + + Try again later or contact support if the issue persists. + ''', + singleButton: true, + );app/lib/providers/message_provider.dart (1)
133-135: Add documentation for the new method.The implementation looks good and follows the existing patterns in the codebase. Consider adding documentation to explain the method's purpose and parameters.
+ /// Returns the App associated with the given appId. + /// If either appId is null or no matching app is found, returns null. + /// @param appId The unique identifier of the app to find + /// @return The matching App object or null if not found App? messageSenderApp(String? appId) { return appProvider?.apps.firstWhereOrNull((p) => p.id == appId); }app/pubspec.yaml (1)
96-96: Consider organizing image_picker under an appropriate category.The
image_pickerdependency should be grouped with other similar packages for better maintainability. Consider moving it under a relevant category section (e.g., "# Utils" or create a new "# Media" category).# Utils collection: 1.18.0 equatable: 2.0.5 + image_picker: ^1.1.2 flutter_blue_plus: ^1.33.4 http: ^1.2.1 - - - image_picker: ^1.1.2app/lib/backend/http/api/apps.dart (2)
167-184: Consider adding caching for app categories.The categories are likely to change infrequently. Consider caching the results in SharedPreferences to:
- Improve performance
- Reduce server load
- Support offline functionality
Here's a suggested implementation:
Future<List<Category>> getAppCategories() async { + // Try to get cached categories first + final cached = SharedPreferencesUtil().getAppCategories(); + if (cached.isNotEmpty) { + return cached; + } + var response = await makeApiCall( url: '${Env.apiBaseUrl}v1/plugin-categories', headers: {}, body: '', method: 'GET', ); try { if (response == null || response.statusCode != 200) return []; log('getAppCategories: ${response.body}'); var res = jsonDecode(response.body); - return Category.fromJsonList(res); + final categories = Category.fromJsonList(res); + // Cache the results + SharedPreferencesUtil().setAppCategories(categories); + return categories; } catch (e, stackTrace) { debugPrint(e.toString()); CrashReporting.reportHandledCrash(e, stackTrace); return []; } }
186-203: Improve naming consistency and consider caching.
- The
Serversuffix in the function name isn't consistent with other API functions in this file. Consider renaming togetTriggerEventsfor consistency.- Like categories, trigger events could benefit from caching.
Here's a suggested improvement:
-Future<List<TriggerEvent>> getTriggerEventsServer() async { +Future<List<TriggerEvent>> getTriggerEvents() async { + // Try to get cached trigger events first + final cached = SharedPreferencesUtil().getTriggerEvents(); + if (cached.isNotEmpty) { + return cached; + } + var response = await makeApiCall( url: '${Env.apiBaseUrl}v1/plugin-triggers', headers: {}, body: '', method: 'GET', ); try { if (response == null || response.statusCode != 200) return []; log('getTriggerEvents: ${response.body}'); var res = jsonDecode(response.body); - return TriggerEvent.fromJsonList(res); + final events = TriggerEvent.fromJsonList(res); + // Cache the results + SharedPreferencesUtil().setTriggerEvents(events); + return events; } catch (e, stackTrace) { debugPrint(e.toString()); CrashReporting.reportHandledCrash(e, stackTrace); return []; } }app/lib/pages/apps/list_item.dart (2)
61-61: Consider improving privacy status visibility.While adding "(private)" to the app name works, it might not be the most user-friendly way to display privacy status. Consider using a dedicated privacy indicator icon or badge for better visual hierarchy.
-app.name + (app.private ? ' (private)' : ''), +Row( + children: [ + Flexible(child: Text(app.name, maxLines: 1)), + if (app.private) + Padding( + padding: const EdgeInsets.only(left: 4), + child: Icon(Icons.lock, size: 16, color: Colors.grey), + ), + ], +),
Line range hint
134-157: Enhance external app authorization security.The authorization dialog for external apps should provide more detailed information about:
- What specific data will be shared
- How the data will be used
- The external server's identity
Consider enhancing the dialog message to be more specific:
-'Do you allow this app to access your memories, transcripts, and recordings? Your data will be sent to the app\'s server for processing.', +'Do you authorize "${app.name}" to access your:\n\n' + +'• Chat memories\n' + +'• Voice transcripts\n' + +'• Audio recordings\n\n' + +'This data will be processed on ${app.serverDomain}. The app\'s privacy policy and terms of service apply to shared data.',app/lib/backend/schema/app.dart (2)
212-218: Enhance image URL handling robustnessWhile the conditional logic for handling both full URLs and relative paths is good, there are a few improvements to consider:
Consider these enhancements:
String getImageUrl() { + if (image.isEmpty) { + return ''; // or a default image URL + } if (image.startsWith('http')) { return image; } else { - return 'https://raw.githubusercontent.com/BasedHardware/Omi/main$image'; + final baseUrl = const String.fromEnvironment('IMAGE_BASE_URL', + defaultValue: 'https://raw.githubusercontent.com/BasedHardware/Omi/main'); + return '$baseUrl$image'; } }
244-269: Add documentation and field validation to Category classThe Category class implementation is clean but could benefit from additional documentation and validation.
Consider these improvements:
+/// Represents a category for grouping apps +/// +/// Used by the app store to organize apps into different sections class Category { + /// The display title of the category String title; + /// Unique identifier for the category String id; Category({ required this.title, required this.id, }); factory Category.fromJson(Map<String, dynamic> json) { + if (!json.containsKey('title') || !json.containsKey('id')) { + throw FormatException('Missing required fields in Category JSON'); + } return Category( title: json['title'], id: json['id'], ); }app/lib/pages/apps/widgets/external_trigger_fields_widget.dart (2)
9-14: Consider improving type safety and maintainability.The capability check uses a string literal which could lead to typos or maintenance issues.
Consider defining capabilities as constants or an enum:
// In a separate constants file class AppCapabilities { static const String externalIntegration = 'external_integration'; // ... other capabilities } // Then use it in the widget if (!provider.isCapabilitySelected(AppCapabilities.externalIntegration))
111-151: Maintain consistent error messages and validation.The error message differs from other URL fields and could be more specific to the setup completed URL context.
Update the error message to be more specific:
validator: (value) { if (value == null || !isValidUrl(value)) { - return 'Please enter a valid URL'; + return 'Please enter a valid setup completed URL'; } return null; },backend/utils/plugins.py (1)
38-70: Consider implementing a Plugin Service layer.The current implementation mixes concerns across data retrieval, caching, and business logic. Consider:
- Creating a dedicated Plugin Service to encapsulate plugin-related operations
- Implementing proper error boundaries and monitoring
- Adding metrics for cache hit/miss ratios and plugin usage patterns
- Using dependency injection for better testability
This would improve maintainability and make the system more robust.
app/lib/providers/developer_mode_provider.dart (4)
Line range hint
196-203: Remove commented-out WebSocket code.The commented WebSocket-related code should be removed if it's no longer needed. If it's for future implementation, consider tracking it with a TODO issue instead.
Line range hint
204-218: Consider implementing structured error handling and validation.The current implementation has several areas for improvement:
- Sequential validations could be grouped for better maintainability
- No rollback mechanism if some webhook updates fail
- Error handling could be more structured
Consider refactoring to:
- Group validations into a dedicated method
- Implement a transaction-like pattern for webhook updates
- Use structured error types
Example approach:
Future<ValidationResult> validateSettings() async { final errors = <String>[]; // Group all validations if (gcpCredentialsController.text.isNotEmpty && gcpBucketNameController.text.isNotEmpty) { try { await authenticateGCP(base64: gcpCredentialsController.text.trim()); } catch (e) { errors.add('Invalid GCP credentials or bucket name'); } } // Group webhook validations final webhooks = { 'audio_bytes': webhookAudioBytes.text, 'realtime_transcript': webhookOnTranscriptReceived.text, 'memory_created': webhookOnMemoryCreated.text, 'day_summary': webhookDaySummary.text, }; for (final entry in webhooks.entries) { if (entry.value.isNotEmpty && !isValidUrl(entry.value)) { errors.add('Invalid ${entry.key} webhook URL'); } } return ValidationResult(errors.isEmpty, errors); }
Line range hint
219-237: Improve webhook update error handling.The current implementation using
Future.waitcould fail silently if some webhook updates fail. Consider implementing proper error handling and rollback mechanism.- try { - Future.wait([w1, w2, w3, w4]); + try { + final results = await Future.wait([w1, w2, w3, w4], eagerError: true); prefs.webhookAudioBytes = webhookAudioBytes.text; prefs.webhookAudioBytesDelay = webhookAudioBytesDelay.text; prefs.webhookOnTranscriptReceived = webhookOnTranscriptReceived.text; prefs.webhookOnMemoryCreated = webhookOnMemoryCreated.text; prefs.webhookDaySummary = webhookDaySummary.text; - } catch (e) { + } catch (e, stackTrace) { Logger.error('Error occurred while updating endpoints: $e'); + Logger.error('Stack trace: $stackTrace'); + AppSnackbar.showSnackbarError('Failed to update some webhook settings'); + // Rollback to previous values + await initialize(); + return; }
Based on the search results, I can now generate the final response as we have sufficient information about the audio bytes webhook delay implementation:
Add validation for audio bytes webhook delay is necessary
The code shows that:
- The delay value is used in backend calculations (
sample_rate * audio_bytes_webhook_delay_seconds * 2)- The input field uses
TextInputType.numberbut lacks client-side validation- Invalid values could cause unexpected behavior in audio processing
The suggested validation in the review comment is appropriate as it would:
- Prevent null or negative values that could break the audio buffer calculations
- Provide clear feedback to users through the snackbar
- Maintain consistency with other webhook validations in the file
🔗 Analysis chain
Line range hint
180-184: Add validation for audio bytes webhook delay.The delay value is set to '5' by default but lacks validation for valid numeric input.
Consider adding validation:
if (webhookAudioBytes.text.isNotEmpty && webhookAudioBytesDelay.text.isEmpty) { webhookAudioBytesDelay.text = '5'; } + if (webhookAudioBytesDelay.text.isNotEmpty) { + final delay = int.tryParse(webhookAudioBytesDelay.text); + if (delay == null || delay < 0) { + AppSnackbar.showSnackbarError('Invalid delay value. Please enter a positive number.'); + setIsLoading(false); + return; + } + }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there's any validation for the delay value in the validators utility rg -A 5 "webhookAudioBytesDelay|audio.*delay"Length of output: 8412
app/lib/pages/apps/page.dart (1)
3-3: Review usage of temp.dart utility.The import from
temp.dartsuggests temporary/transitional code. Consider moving therouteToPageutility to a permanent location in the codebase, such as a dedicated navigation utilities file.Also applies to: 5-5, 8-8
backend/utils/memories/process_memory.py (1)
Line range hint
105-119: Add thread safety, timeouts, and error handling for plugin execution.While the plugin execution logic is functional, consider these improvements for robustness:
- Use thread-safe operations for appending results
- Add timeout mechanism for plugin threads
- Implement error handling for plugin execution
Consider this implementation:
def _trigger_plugins(uid: str, memory: Memory, is_reprocess: bool = False): plugins: List[Plugin] = get_plugins_data_from_db(uid) filtered_plugins = [plugin for plugin in plugins if plugin.works_with_memories() and plugin.enabled] - memory.plugins_results = [] + from threading import Lock + from queue import Queue + results_queue = Queue() + results_lock = Lock() threads = [] def execute_plugin(plugin): - if result := get_plugin_result(memory.get_transcript(False), plugin).strip(): - memory.plugins_results.append(PluginResult(plugin_id=plugin.id, content=result)) - if not is_reprocess: - record_plugin_usage(uid, plugin.id, UsageHistoryType.memory_created_prompt, memory_id=memory.id) + try: + if result := get_plugin_result(memory.get_transcript(False), plugin).strip(): + results_queue.put(PluginResult(plugin_id=plugin.id, content=result)) + if not is_reprocess: + record_plugin_usage(uid, plugin.id, UsageHistoryType.memory_created_prompt, memory_id=memory.id) + except Exception as e: + print(f"Error executing plugin {plugin.id}: {str(e)}") for plugin in filtered_plugins: - threads.append(threading.Thread(target=execute_plugin, args=(plugin,))) + thread = threading.Thread(target=execute_plugin, args=(plugin,)) + thread.daemon = True # Allow main program to exit even if threads are running + threads.append(thread) [t.start() for t in threads] - [t.join() for t in threads] + # Wait for threads with timeout + timeout = 30 # seconds + [t.join(timeout) for t in threads] + + # Collect results + memory.plugins_results = [] + while not results_queue.empty(): + memory.plugins_results.append(results_queue.get())🧰 Tools
🪛 Ruff
104-104:
Memorymay be undefined, or defined from star imports(F405)
105-105:
Listmay be undefined, or defined from star imports(F405)
app/lib/backend/preferences.dart (1)
132-135: Consider documenting the preference's purposeWhile the implementation is correct, adding documentation comments would help other developers understand the purpose and usage of this preference setting.
Consider adding documentation:
+ /// Whether to show a confirmation dialog when submitting an app. + /// Defaults to true if not set. bool get showSubmitAppConfirmation => getBool('showSubmitAppConfirmation') ?? true; + /// Sets whether to show a confirmation dialog when submitting an app. set showSubmitAppConfirmation(bool value) => saveBool('showSubmitAppConfirmation', value);app/lib/pages/chat/page.dart (2)
Line range hint
359-359: Track streaming capabilities restoration.This TODO appears important for the chat experience, especially with the new app/plugin functionality being added.
Would you like me to:
- Create a GitHub issue to track the restoration of streaming capabilities?
- Help implement the streaming functionality with an initial empty message approach?
Line range hint
1-424: Consider further architectural improvements.To enhance maintainability, consider:
- Extracting message sending logic to a dedicated service
- Creating constants for magic numbers (e.g., padding values)
- Using a ScrollController provider to manage scroll state
app/lib/pages/apps/app_detail.dart (2)
282-292: Improve URL handling robustness.While the conditional handling of different URL types is good, consider these improvements:
- Use a more robust method to determine if it's a GitHub raw URL, such as parsing the URL and checking the host.
- Add error handling for URL launches.
Consider this implementation:
if (widget.app.externalIntegration != null) { - if (widget.app.externalIntegration!.setupInstructionsFilePath - .contains('raw.githubusercontent.com')) { + final uri = Uri.tryParse(widget.app.externalIntegration!.setupInstructionsFilePath); + if (uri?.host == 'raw.githubusercontent.com') { await routeToPage( context, AppSetupInstructions(markdown: instructionsMarkdown ?? ''), ); } else { - await launchUrl(Uri.parse(widget.app.externalIntegration!.setupInstructionsFilePath)); + try { + final success = await launchUrl( + Uri.parse(widget.app.externalIntegration!.setupInstructionsFilePath) + ); + if (!success && mounted) { + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar(content: Text('Could not open URL')), + ); + } + } catch (e) { + if (mounted) { + ScaffoldMessenger.of(context).showSnackBar( + const SnackBar(content: Text('Invalid URL')), + ); + } + } } }
Line range hint
456-498: Enhance error handling in app toggling.The error handling for app activation is good, but consider these improvements:
- Add specific error messages based on failure reasons
- Add error handling for app disabling
- Consider adding retry logic for failed network operations
Consider this implementation:
Future<void> _toggleApp(String appId, bool isEnabled) async { var prefs = SharedPreferencesUtil(); setState(() => appLoading = true); if (isEnabled) { var enabled = await enableAppServer(appId); if (!enabled) { if (mounted) { showDialog( context: context, builder: (c) => getDialog( context, () => Navigator.pop(context), () => Navigator.pop(context), - 'Error activating the app', - 'If this is an integration app, make sure the setup is completed.', + 'Failed to activate the app', + widget.app.worksExternally() + ? 'Please complete the setup steps and try again.' + : 'Please check your internet connection and try again.', singleButton: true, ), ); } setState(() => appLoading = false); return; } prefs.enableApp(appId); MixpanelManager().appEnabled(appId); } else { prefs.disableApp(appId); - await enableAppServer(appId); + try { + final success = await enableAppServer(appId); + if (!success && mounted) { + showDialog( + context: context, + builder: (c) => getDialog( + context, + () => Navigator.pop(context), + () { + Navigator.pop(context); + // Revert local state since server update failed + prefs.enableApp(appId); + setState(() => widget.app.enabled = true); + }, + 'Failed to disable the app', + 'The app will remain enabled. Please try again.', + singleButton: true, + ), + ); + return; + } + } catch (e) { + if (mounted) { + showDialog( + context: context, + builder: (c) => getDialog( + context, + () => Navigator.pop(context), + () { + Navigator.pop(context); + // Revert local state since server update failed + prefs.enableApp(appId); + setState(() => widget.app.enabled = true); + }, + 'Error', + 'An unexpected error occurred. Please try again.', + singleButton: true, + ), + ); + return; + } + } MixpanelManager().appDisabled(appId); } setState(() => widget.app.enabled = isEnabled); setState(() => appLoading = false); }app/lib/pages/memory_detail/widgets.dart (1)
530-582: Refactor the unknown app ListTile implementationWhile the implementation provides good user feedback, there are several improvements that could be made:
- Extract hardcoded asset paths to constants
- Extract duplicate copy functionality into a shared method
- Define container dimensions as constants
+ // At the top of the file + const kBackgroundImagePath = "assets/images/background.png"; + const kHeroLogoPath = "assets/images/herologo.png"; + const kUnknownAppContainerSize = 30.0; + const kUnknownAppLogoSize = 24.0; + + void _copyAppResponse(BuildContext context, String content, ServerMemory memory) { + Clipboard.setData(ClipboardData(text: content.trim())); + ScaffoldMessenger.of(context).showSnackBar(const SnackBar( + content: Text('App response copied to clipboard'), + )); + MixpanelManager().copiedMemoryDetails(memory, source: 'App Response'); + } // In the widget ListTile( contentPadding: EdgeInsets.zero, leading: Container( decoration: const BoxDecoration( image: DecorationImage( - image: AssetImage("assets/images/background.png"), + image: AssetImage(kBackgroundImagePath), fit: BoxFit.cover, ), borderRadius: BorderRadius.all(Radius.circular(16.0)), ), - height: 30, - width: 30, + height: kUnknownAppContainerSize, + width: kUnknownAppContainerSize, child: Stack( alignment: Alignment.center, children: [ Image.asset( - "assets/images/herologo.png", - height: 24, - width: 24, + kHeroLogoPath, + height: kUnknownAppLogoSize, + width: kUnknownAppLogoSize, ), ], ), ), // ... other properties ... trailing: IconButton( icon: const Icon(Icons.copy_rounded, color: Colors.white, size: 20), onPressed: () { - Clipboard.setData(ClipboardData(text: appResponse.content.trim())); - ScaffoldMessenger.of(context).showSnackBar(const SnackBar( - content: Text('App response copied to clipboard'), - )); - MixpanelManager() - .copiedMemoryDetails(provider.memory, source: 'App Response'); + _copyAppResponse(context, appResponse.content, provider.memory); }, ), ),backend/routers/chat.py (1)
8-8: Import statement order and consistency.Ensure that imports are grouped and ordered consistently for better readability. Standard library imports should be separated from third-party and local imports.
Consider rearranging the imports like this:
import uuid from datetime import datetime, timezone from typing import List, Optional +from fastapi import APIRouter, Depends, HTTPException + import database.chat as chat_db from database.plugins import record_plugin_usage, get_plugin_by_id_db from models.chat import Message, SendMessageRequest, MessageSender from models.plugin import UsageHistoryType, Plugin from models.memory import Memory +from utils.llm import initial_chat_message +from utils.other import endpoints as auth +from utils.plugins import get_plugin_by_id +from utils.retrieval.graph import execute_graph_chatapp/lib/pages/apps/providers/add_app_provider.dart (4)
47-52: Perform asynchronous initialization tasks in parallel to improve efficiency.In the
initmethod,getCategories()andgetTriggerEvents()are called sequentially usingawait, causing the second call to wait for the first to complete. Since these two methods are independent, running them concurrently can reduce initialization time and improve the user experience.Apply this diff to run asynchronous tasks in parallel:
def init() async { - await getCategories(); - await getTriggerEvents(); + await Future.wait([ + getCategories(), + getTriggerEvents(), + ]); creatorNameController.text = SharedPreferencesUtil().givenName; creatorEmailController.text = SharedPreferencesUtil().email; }
87-144: Refactor thevalidateFormmethod to improve readability and maintainability.The
validateFormmethod contains multiple repeated code blocks for checking if fields are empty and displaying error messages. Extracting common validation logic into helper methods can reduce code duplication and enhance readability.Create a helper method for validating text fields:
bool validateTextField(TextEditingController controller, String errorMessage) { if (controller.text.isEmpty) { AppSnackbar.showSnackbarError(errorMessage); return false; } return true; }Refactor the
validateFormmethod to use the helper:bool validateForm() { if (formKey.currentState!.validate()) { if (!termsAgreed) { AppSnackbar.showSnackbarError('Please agree to the terms and conditions to proceed'); return false; } if (!capabilitySelected()) { AppSnackbar.showSnackbarError('Please select at least one capability for your app'); return false; } if (imageFile == null) { AppSnackbar.showSnackbarError('Please select a logo for your app'); return false; } if (isCapabilitySelected('external_integration')) { if (triggerEvent == null) { AppSnackbar.showSnackbarError('Please select a trigger event for your app'); return false; } if (!validateTextField(webhookUrlController, 'Please enter a webhook URL for your app')) { return false; } if (!validateTextField(setupCompletedController, 'Please enter a setup completed URL for your app')) { return false; } if (!validateTextField(instructionsController, 'Please enter a setup instructions URL for your app')) { return false; } } if (isCapabilitySelected('chat')) { if (!validateTextField(chatPromptController, 'Please enter a chat prompt for your app')) { return false; } } if (isCapabilitySelected('memories')) { if (!validateTextField(memoryPromptController, 'Please enter a memory prompt for your app')) { return false; } } if (appCategory == null) { AppSnackbar.showSnackbarError('Please select a category for your app'); return false; } if (privacyLevel == null) { AppSnackbar.showSnackbarError('Please select a privacy level for your app'); return false; } return true; } else { AppSnackbar.showSnackbarError('Please fill in all the required fields correctly'); return false; } }
188-202: Enhance exception handling in thepickImagemethod.Currently, the
pickImagemethod only catchesPlatformExceptionforphoto_access_denied. Expanding the exception handling to catch all exceptions will ensure that unexpected errors are handled gracefully, improving app stability.Apply this diff to enhance exception handling:
def pickImage() async { ImagePicker imagePicker = ImagePicker(); try { var file = await imagePicker.pickImage(source: ImageSource.gallery); if (file != null) { imageFile = File(file.path); } - notifyListeners(); - } on PlatformException catch (e) { - if (e.code == 'photo_access_denied') { + notifyListeners(); + } catch (e) { + if (e is PlatformException && e.code == 'photo_access_denied') { AppSnackbar.showSnackbarError('Photos permission denied. Please allow access to photos to select an image'); } else { AppSnackbar.showSnackbarError('An unexpected error occurred while selecting an image'); } } notifyListeners(); }
190-202: OptimizenotifyListeners()calls in thepickImagemethod.The
notifyListeners()method is called twice inpickImage, once inside thetryblock and once after thecatchblock. Since it needs to be called regardless of the outcome, you can move it outside thetry-catchblock to avoid redundancy.Apply this diff to streamline the code:
def pickImage() async { ImagePicker imagePicker = ImagePicker(); try { var file = await imagePicker.pickImage(source: ImageSource.gallery); if (file != null) { imageFile = File(file.path); } - notifyListeners(); } on PlatformException catch (e) { if (e.code == 'photo_access_denied') { AppSnackbar.showSnackbarError('Photos permission denied. Please allow access to photos to select an image'); } } - notifyListeners(); + notifyListeners(); }backend/routers/plugins.py (2)
173-173: Remove unnecessary f-string prefixThe string at line 173 does not contain any placeholders, so the
fprefix is unnecessary. Removing it improves readability.Apply this diff:
-os.makedirs(f'_temp/plugins', exist_ok=True) +os.makedirs('_temp/plugins', exist_ok=True)🧰 Tools
🪛 Ruff
173-173: f-string without any placeholders
Remove extraneous
fprefix(F541)
166-167: Sanitize plugin IDs to avoid invalid charactersWhen generating
data['id']fromdata['name'], special characters in the name might result in invalid IDs. Sanitize the name to include only allowed characters.Consider using:
import re data['id'] = re.sub(r'[^a-z0-9-]', '', data['name'].replace(' ', '-').lower())app/lib/pages/apps/add_app.dart (2)
109-111: Handle errors during image selectionWhen the user taps to upload an app icon, there is no error handling for potential issues during image selection. It's important to handle exceptions to provide a smooth user experience.
Apply this diff to handle exceptions:
onTap: () async { - await provider.pickImage(); + try { + await provider.pickImage(); + } catch (e) { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text('Image selection failed: \$e')), + ); + } },
26-28: Handle exceptions during initializationIn the
initState()method, the asynchronous call toprovider.init()might fail. It's advisable to handle potential exceptions to prevent the app from crashing.Apply this diff to catch exceptions:
WidgetsBinding.instance.addPostFrameCallback((timeStamp) async { - await Provider.of<AddAppProvider>(context, listen: false).init(); + try { + await Provider.of<AddAppProvider>(context, listen: false).init(); + } catch (e) { + ScaffoldMessenger.of(context).showSnackBar( + SnackBar(content: Text('Initialization failed: \$e')), + ); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (27)
app/lib/backend/http/api/apps.dart(5 hunks)app/lib/backend/http/api/messages.dart(2 hunks)app/lib/backend/preferences.dart(1 hunks)app/lib/backend/schema/app.dart(4 hunks)app/lib/main.dart(2 hunks)app/lib/pages/apps/add_app.dart(1 hunks)app/lib/pages/apps/app_detail.dart(1 hunks)app/lib/pages/apps/list_item.dart(1 hunks)app/lib/pages/apps/page.dart(2 hunks)app/lib/pages/apps/providers/add_app_provider.dart(1 hunks)app/lib/pages/apps/widgets/capabilities_chips_widget.dart(1 hunks)app/lib/pages/apps/widgets/external_trigger_fields_widget.dart(1 hunks)app/lib/pages/apps/widgets/prompt_text_field.dart(1 hunks)app/lib/pages/chat/page.dart(1 hunks)app/lib/pages/memory_detail/widgets.dart(2 hunks)app/lib/providers/app_provider.dart(1 hunks)app/lib/providers/developer_mode_provider.dart(1 hunks)app/lib/providers/message_provider.dart(1 hunks)app/lib/utils/alerts/app_snackbar.dart(1 hunks)app/lib/utils/other/validators.dart(1 hunks)app/pubspec.yaml(2 hunks)backend/database/plugins.py(2 hunks)backend/routers/chat.py(2 hunks)backend/routers/plugins.py(2 hunks)backend/utils/memories/process_memory.py(2 hunks)backend/utils/other/storage.py(1 hunks)backend/utils/plugins.py(4 hunks)
🧰 Additional context used
🪛 Ruff
backend/routers/chat.py
111-111: Redefinition of unused send_message from line 99
(F811)
116-116: Redefinition of unused send_message from line 111
(F811)
backend/routers/plugins.py
122-122: Redefinition of unused enable_plugin_endpoint from line 25
(F811)
138-138: Redefinition of unused disable_plugin_endpoint from line 40
(F811)
148-148: Redefinition of unused get_plugins from line 60
(F811)
163-163: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
163-163: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
173-173: f-string without any placeholders
Remove extraneous f prefix
(F541)
backend/utils/memories/process_memory.py
28-28: utils.plugins.get_plugins_data imported but unused
Remove unused import: utils.plugins.get_plugins_data
(F401)
105-105: List may be undefined, or defined from star imports
(F405)
🔇 Additional comments (27)
app/lib/utils/alerts/app_snackbar.dart (1)
23-29: LGTM! The implementation follows consistent patterns.
The new showSnackbarSuccess method maintains consistency with existing methods and provides a clear way to show success notifications.
app/lib/pages/apps/widgets/prompt_text_field.dart (1)
1-54:
Add input sanitization for security.
Since this widget is used in app/plugin submission forms, consider adding input sanitization to prevent potential security issues like XSS when the input is used in HTML contexts or SQL injection if stored in a database.
Let's check if there are any existing input sanitization utilities in the codebase:
app/lib/pages/apps/widgets/capabilities_chips_widget.dart (3)
1-5: LGTM! Clean and well-organized imports.
The imports are minimal and follow Flutter conventions.
6-8: LGTM! Well-structured class declaration.
The class follows Flutter best practices with proper constructor implementation.
11-60: Verify error handling and validation requirements.
Please consider the following aspects:
- How should the widget handle cases when AddAppProvider is not available?
- Are there any requirements for minimum/maximum number of capabilities that can be selected?
- Should the widget show any feedback when selection limits are reached?
Let's check if there are any existing validation rules:
app/lib/backend/http/api/messages.dart (2)
40-42: Verify plugin_id parameter handling.
The endpoint now includes a plugin_id query parameter. Ensure that the backend properly validates this parameter.
#!/bin/bash
# Description: Check plugin_id validation in the backend
# Expected: Find validation logic for plugin_id parameter
# Search for plugin_id validation in Python route handlers
echo "Checking plugin_id validation:"
rg -B 2 -A 5 "plugin_id.*=.*request\.(args|query_params|params)\.get\(['\"]plugin_id['\"]" --type pyLine range hint 1-38: Inconsistent API versioning across endpoints.
Some endpoints are still using v1 while others have been upgraded to v2. This inconsistency could lead to maintenance issues.
Let's verify the API versioning across the codebase:
app/lib/providers/app_provider.dart (1)
50-50: Verify usage patterns of AppProvider
Let's verify how this provider is used across the codebase to ensure our changes won't cause issues.
✅ Verification successful
Based on the shell script results, I can now provide a final response:
AppProvider's appLoading array initialization is safe
The initialization of appLoading array is properly synchronized with the apps list:
- The array is initialized in
getApps()method right after apps are retrieved - The
toggleApp()method has proper index bounds checking and loading state management - No concurrent modifications of the array were found in the codebase
- The array is only accessed within the AppProvider class with proper state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AppProvider usage patterns
echo "Searching for AppProvider instantiation and usage..."
rg "AppProvider" -A 5
echo "\nChecking for concurrent operations on apps..."
rg "toggleApp|getApps" -A 5
echo "\nLooking for potential race conditions..."
ast-grep --pattern 'Future<void> $method($_) {
$$$
appLoading[$_] = $_;
$$$
}'
Length of output: 18708
app/lib/providers/message_provider.dart (1)
133-135: Verify the usage of messageSenderApp method.
Let's ensure this new method is being used consistently across the codebase where app lookups are needed.
✅ Verification successful
Based on the search results, I can generate the final response as we have clear evidence about both the implementation and usage of messageSenderApp:
The messageSenderApp method is being used appropriately and consistently
The codebase scan shows that:
- The method is only implemented once in
message_provider.dart - It's being used in
chat/page.dartfor app lookup - No other direct app lookups using similar pattern (
firstWhereOrNullwith app ID) exist in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all potential places where app lookups are performed to ensure consistent usage
# Search for direct app lookups that could use messageSenderApp instead
echo "Searching for potential places to use messageSenderApp..."
rg -l "firstWhereOrNull.*p\.id.*appId" --type dart
# Search for current usage of messageSenderApp
echo "Checking current usage of messageSenderApp..."
rg "messageSenderApp" --type dart
Length of output: 559
app/pubspec.yaml (1)
33-33: LGTM: dotted_border dependency addition.
The addition of dotted_border: ^2.1.0 is appropriately placed under the UI section and aligns with the PR's app management UI enhancements.
app/lib/pages/apps/list_item.dart (1)
Line range hint 1-157: Verify app schema compatibility.
The code assumes the App model has a private field. Let's verify this assumption and ensure backward compatibility.
✅ Verification successful
The private field is properly defined and used in the App model
The verification confirms that:
- The
privatefield is properly defined as a boolean property in theAppclass - It's initialized with a default value of
false - The field is properly handled in JSON deserialization with a fallback logic:
private: json['private'] ?? json['id'].toString().contains('private')
The code's assumption about the private field is correct and there are no compatibility issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify App model schema and usage
# Expected: Find the private field definition and any migration code
# Check App model definition
echo "Checking App model definition..."
rg -A 5 "class App" app/lib/backend/schema/
# Check for migration code
echo "Checking migration handling..."
rg -l "private.*migration" app/lib/backend/
Length of output: 1407
Script:
#!/bin/bash
# Let's check the complete App class definition and its properties
echo "Checking complete App class definition..."
rg -A 30 "^class App" app/lib/backend/schema/app.dart
# Also search for any private field usage in the App class
echo "Checking private field usage..."
rg "private" app/lib/backend/schema/app.dart
Length of output: 2115
app/lib/backend/schema/app.dart (1)
142-142: 🛠️ Refactor suggestion
Review the fallback logic for private field initialization
The fallback logic json['id'].toString().contains('private') for determining privacy status based on the ID string seems fragile. This could lead to false positives if an app's ID happens to contain the word "private" for other reasons.
Let's check if there are any existing apps that might be affected:
Consider one of these alternatives:
- Remove the fallback and default to
false - Add a migration script to explicitly set the
privatefield - Document why this fallback exists if it's intentionally handling legacy data
Also applies to: 176-176, 208-208
app/lib/pages/apps/widgets/external_trigger_fields_widget.dart (1)
1-8: LGTM! Clean imports and proper widget declaration.
The imports are well-organized and the widget is correctly declared as a StatelessWidget since it relies on Provider for state management.
backend/utils/other/storage.py (1)
239-244: Verify plugin logo cleanup process.
Based on the PR objectives mentioning "deleting the plugins_data collection and the associated images in the bucket", we should ensure there's a cleanup function for plugin logos.
Let's check if there's a cleanup function elsewhere in the codebase:
Would you like me to generate a complementary function to handle plugin logo deletion?
backend/utils/plugins.py (2)
8-8: LGTM: Import changes align with new database-driven approach.
The addition of database-specific imports supports the transition from external URL-based plugin data retrieval to database-driven retrieval.
147-148: LGTM: Usage tracking implementation.
The plugin usage tracking is correctly implemented with proper parameters.
app/lib/providers/developer_mode_provider.dart (1)
9-9: LGTM! Good architectural improvement.
Moving URL validation to a dedicated validators utility improves code organization and ensures consistent validation across the application.
backend/utils/memories/process_memory.py (2)
Line range hint 166-182: Verify plugin data consistency after migration.
The PR objectives mention that plugins need to be migrated before deployment. Consider adding validation checks to ensure plugin data consistency.
Let's verify plugin data structure:
#!/bin/bash
# Check plugin data structure in the codebase
echo "Analyzing plugin data structure..."
rg -A 5 "class Plugin" --type py
# Look for plugin validation code
echo "Checking for plugin validation..."
rg -l "validate.*plugin" --type pyConsider implementing these safeguards:
- Add a version field to plugin data to track migrations
- Implement validation checks for plugin data structure
- Add a mechanism to verify plugin data consistency after migration
🧰 Tools
🪛 Ruff
28-28: utils.plugins.get_plugins_data imported but unused
Remove unused import: utils.plugins.get_plugins_data
(F401)
Line range hint 166-182: Add error handling for threaded operations and consider plugin migration.
- The threaded operations for vector storage and fact extraction lack error handling
- The PR objectives mention the need to migrate plugins before deployment, but there's no mechanism to handle failed plugin data migration
Let's verify the plugin migration requirements:
Consider adding error handling for threaded operations:
if not discarded:
_trigger_plugins(uid, memory, is_reprocess=is_reprocess)
- threading.Thread(target=save_structured_vector, args=(uid, memory,)).start() if not is_reprocess else None
- threading.Thread(target=_extract_facts, args=(uid, memory)).start()
+ def run_with_error_handling(func, *args):
+ try:
+ func(*args)
+ except Exception as e:
+ print(f"Error in {func.__name__}: {str(e)}")
+ # Consider adding error reporting mechanism
+
+ if not is_reprocess:
+ thread = threading.Thread(target=run_with_error_handling, args=(save_structured_vector, uid, memory))
+ thread.daemon = True
+ thread.start()
+
+ facts_thread = threading.Thread(target=run_with_error_handling, args=(_extract_facts, uid, memory))
+ facts_thread.daemon = True
+ facts_thread.start()🧰 Tools
🪛 Ruff
28-28: utils.plugins.get_plugins_data imported but unused
Remove unused import: utils.plugins.get_plugins_data
(F401)
app/lib/main.dart (2)
18-18: LGTM: Import follows proper conventions
The import statement follows Flutter's conventions and maintains proper code organization.
194-198: Verify provider dependencies and ordering
The AddAppProvider configuration looks good and follows the established pattern for proxy providers. However, let's verify:
- If any other providers need access to
AddAppProvider - If the provider order affects any dependent providers
Let's check for potential provider dependencies:
✅ Verification successful
Let me check the provider dependencies more thoroughly by examining the actual usage context:
Based on the results, I can now generate the final response:
Provider configuration is correctly placed and integrated
The AddAppProvider configuration is properly positioned and integrated:
- It only depends on
AppProviderwhich is initialized early in the provider chain - No other providers depend on
AddAppProvider - It's used correctly in its consumer widgets (
add_app.dart,capabilities_chips_widget.dart,external_trigger_fields_widget.dart) - The provider order is appropriate as it's placed after its dependency (
AppProvider)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for AddAppProvider usage in other providers
# Look for imports and class dependencies
echo "Checking for AddAppProvider imports:"
rg -l "import.*add_app_provider"
echo -e "\nChecking for AddAppProvider usage in provider classes:"
rg -l "AddAppProvider" --type dart
Length of output: 670
Script:
#!/bin/bash
# Check how AddAppProvider is used in the files
echo "=== Checking AddAppProvider usage context ==="
rg "AddAppProvider" -B 2 -A 2 --type dart
# Check provider order in main.dart
echo -e "\n=== Checking provider configuration in main.dart ==="
rg "Provider[^a-zA-Z]" -A 1 app/lib/main.dart
Length of output: 6574
app/lib/backend/preferences.dart (2)
132-133: LGTM: Getter follows established patterns
The getter implementation follows the class's established patterns for boolean preferences, including proper default value handling.
134-135: LGTM: Setter follows established patterns
The setter implementation correctly uses the existing saveBool helper method and follows the class's established patterns for preference persistence.
app/lib/pages/chat/page.dart (1)
220-220: LGTM! Good refactoring to centralize app sender logic.
Moving the app sender determination logic to the provider improves separation of concerns and maintainability.
Let's verify the provider implementation to ensure consistent behavior:
✅ Verification successful
Let me try a different search pattern to locate the provider implementation.
Let me check the appProvider implementation to ensure the apps list is properly maintained.
Refactoring verified - implementation maintains consistent behavior
The verification shows that:
messageSenderAppin MessageProvider correctly uses AppProvider to look up apps- Apps list is properly maintained through:
- Initial loading from API (
retrieveApps) - Persistent storage via SharedPreferences
- Updates through notifyListeners pattern
- Initial loading from API (
- The refactoring preserves the same app lookup logic while improving code organization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify MessageProvider's messageSenderApp implementation
# Expected: Should find the implementation and any related tests
# Look for the messageSenderApp method implementation
echo "Searching for messageSenderApp implementation..."
ast-grep --pattern 'messageSenderApp($id) {
$$$
}'
# Look for related tests
echo "Searching for related tests..."
fd -e dart -e test "message_provider_test" --exec rg -l "messageSenderApp"
Length of output: 305
Script:
#!/bin/bash
# Search for any class that contains messageSenderApp method
ast-grep --pattern 'class $_ {
$$$
App? messageSenderApp($_) {
$$$
}
$$$
}'
# Also try to find any usage of messageSenderApp
rg "messageSenderApp" -A 3
Length of output: 836
Script:
#!/bin/bash
# Find the AppProvider class implementation
ast-grep --pattern 'class AppProvider $_$ {
$$$
}'
# Look for where apps list is populated/updated
rg "apps =" -A 3
Length of output: 3034
app/lib/pages/memory_detail/widgets.dart (1)
204-204: LGTM: Checkbox shape optimization
Making the shape constant is a good optimization as it prevents unnecessary widget rebuilds.
backend/routers/chat.py (1)
151-152: 🛠️ Refactor suggestion
Ensure plugin is valid before instantiating Plugin.
Similar to the previous issue, plugin might be None, which will raise an exception when unpacked. Validate plugin before proceeding.
Apply this diff to handle the potential None value:
plugin = get_plugin_by_id_db(plugin_id, uid)
+if plugin is None:
+ raise HTTPException(status_code=404, detail='Plugin not found')
plugin = Plugin(**plugin)Likely invalid or redundant comment.
app/lib/pages/apps/providers/add_app_provider.dart (1)
13-252: Overall, the code is well-structured and implements the required functionality effectively.
The AddAppProvider class is comprehensive, handling state management, form validation, and interaction with backend services appropriately. The use of ChangeNotifier and separation of concerns enhances maintainability.
| bool isValidEmail(String email) { | ||
| const emailPattern = r'^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$'; | ||
| return RegExp(emailPattern).hasMatch(email); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update email validation to follow RFC 5322 standards
The current email validation is overly restrictive and doesn't comply with email standards:
- Doesn't support valid characters like
+, quotes, or international characters - Incorrect TLD length restriction
- Doesn't handle comments or special cases defined in RFC 5322
Consider this improved implementation:
bool isValidEmail(String email) {
+ if (email.length > 254) return false; // RFC 5321
+ const pattern = r'^[\w!#$%&\'*+/=?^`{|}~-]+(?:\.[\w!#$%&\'*+/=?^`{|}~-]+)*'
+ r'@(?:[\w](?:[\w-]*[\w])?\.)+[\w](?:[\w-]*[\w])?$';
- const emailPattern = r'^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$';
- return RegExp(emailPattern).hasMatch(email);
+ return RegExp(pattern, unicode: true).hasMatch(email);
}This implementation:
- Follows RFC 5322 standards
- Supports international characters
- Includes proper length validation
- Allows all valid special characters
📝 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.
| bool isValidEmail(String email) { | |
| const emailPattern = r'^[\w-\.]+@([\w-]+\.)+[\w-]{2,4}$'; | |
| return RegExp(emailPattern).hasMatch(email); | |
| } | |
| bool isValidEmail(String email) { | |
| if (email.length > 254) return false; // RFC 5321 | |
| const pattern = r'^[\w!#$%&\'*+/=?^`{|}~-]+(?:\.[\w!#$%&\'*+/=?^`{|}~-]+)*' | |
| r'@(?:[\w](?:[\w-]*[\w])?\.)+[\w](?:[\w-]*[\w])?$'; | |
| return RegExp(pattern, unicode: true).hasMatch(email); | |
| } |
| bool isValidWebSocketUrl(String url) { | ||
| const webSocketPattern = r'^(wss?:\/\/)?([a-zA-Z0-9.-]+(:[a-zA-Z0-9.&%$-]+)*@)?' | ||
| r'((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}' | ||
| r'(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)|' | ||
| r'([a-zA-Z0-9-]+\.)+[a-zA-Z]{2,63}(:[0-9]+)?(\/.*)?$'; | ||
| return RegExp(webSocketPattern).hasMatch(url); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve WebSocket URL validation robustness
The WebSocket URL validation has similar limitations to the URL validator and needs stricter protocol enforcement.
Consider this improved implementation:
bool isValidWebSocketUrl(String url) {
+ if (url.length > 2048) return false;
+ try {
+ final uri = Uri.parse(url);
+ return uri.hasScheme &&
+ (uri.scheme == 'ws' || uri.scheme == 'wss') &&
+ uri.host.isNotEmpty;
+ } catch (e) {
+ return false;
+ }
- const webSocketPattern = r'^(wss?:\/\/)?([a-zA-Z0-9.-]+(:[a-zA-Z0-9.&%$-]+)*@)?'
- r'((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}'
- r'(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)|'
- r'([a-zA-Z0-9-]+\.)+[a-zA-Z]{2,63}(:[0-9]+)?(\/.*)?$';
- return RegExp(webSocketPattern).hasMatch(url);
}📝 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.
| bool isValidWebSocketUrl(String url) { | |
| const webSocketPattern = r'^(wss?:\/\/)?([a-zA-Z0-9.-]+(:[a-zA-Z0-9.&%$-]+)*@)?' | |
| r'((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}' | |
| r'(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)|' | |
| r'([a-zA-Z0-9-]+\.)+[a-zA-Z]{2,63}(:[0-9]+)?(\/.*)?$'; | |
| return RegExp(webSocketPattern).hasMatch(url); | |
| } | |
| bool isValidWebSocketUrl(String url) { | |
| if (url.length > 2048) return false; | |
| try { | |
| final uri = Uri.parse(url); | |
| return uri.hasScheme && | |
| (uri.scheme == 'ws' || uri.scheme == 'wss') && | |
| uri.host.isNotEmpty; | |
| } catch (e) { | |
| return false; | |
| } | |
| } |
| bool isValidUrl(String url) { | ||
| const urlPattern = r'^(https?:\/\/)?([a-zA-Z0-9.-]+(:[a-zA-Z0-9.&%$-]+)*@)?' | ||
| r'((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}' | ||
| r'(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)|' | ||
| r'([a-zA-Z0-9-]+\.)+[a-zA-Z]{2,63}(:[0-9]+)?(\/.*)?$'; | ||
| return RegExp(urlPattern).hasMatch(url); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance URL validation for better security and compliance
The current URL validation has several limitations that could affect security and functionality:
- Missing IPv6 support
- No maximum length validation
- Ambiguous protocol handling
- Limited support for percent-encoded characters
- Potential for invalid TLD acceptance
Consider using this improved implementation:
bool isValidUrl(String url) {
+ if (url.length > 2048) return false; // Common URL length limit
+ try {
+ final uri = Uri.parse(url);
+ return uri.hasScheme &&
+ (uri.scheme == 'http' || uri.scheme == 'https') &&
+ uri.host.isNotEmpty;
+ } catch (e) {
+ return false;
+ }
- const urlPattern = r'^(https?:\/\/)?([a-zA-Z0-9.-]+(:[a-zA-Z0-9.&%$-]+)*@)?'
- r'((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}'
- r'(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)|'
- r'([a-zA-Z0-9-]+\.)+[a-zA-Z]{2,63}(:[0-9]+)?(\/.*)?$';
- return RegExp(urlPattern).hasMatch(url);
}This implementation:
- Uses Dart's built-in Uri parser for robust URL validation
- Enforces HTTPS/HTTP schemes
- Handles IPv6, percent-encoding, and international domains correctly
- Includes length validation
📝 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.
| bool isValidUrl(String url) { | |
| const urlPattern = r'^(https?:\/\/)?([a-zA-Z0-9.-]+(:[a-zA-Z0-9.&%$-]+)*@)?' | |
| r'((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}' | |
| r'(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)|' | |
| r'([a-zA-Z0-9-]+\.)+[a-zA-Z]{2,63}(:[0-9]+)?(\/.*)?$'; | |
| return RegExp(urlPattern).hasMatch(url); | |
| } | |
| bool isValidUrl(String url) { | |
| if (url.length > 2048) return false; // Common URL length limit | |
| try { | |
| final uri = Uri.parse(url); | |
| return uri.hasScheme && | |
| (uri.scheme == 'http' || uri.scheme == 'https') && | |
| uri.host.isNotEmpty; | |
| } catch (e) { | |
| return false; | |
| } | |
| } |
| return TextFormField( | ||
| maxLines: null, | ||
| validator: (value) { | ||
| if (value == null || value.isEmpty) { | ||
| return 'Please enter a valid prompt'; | ||
| } | ||
| return null; | ||
| }, | ||
| controller: controller, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance input validation and constraints.
The current validation is basic and could be improved in several ways:
- Add trim() to prevent whitespace-only inputs
- Consider adding maxLength constraint
- Add optional validation pattern support
return TextFormField(
maxLines: null,
+ maxLength: 1000, // Or any appropriate limit
validator: (value) {
- if (value == null || value.isEmpty) {
- return 'Please enter a valid prompt';
+ if (value == null || value.trim().isEmpty) {
+ return validationMessage;
}
return null;
},Committable suggestion skipped: line range outside the PR's diff.
| decoration: InputDecoration( | ||
| border: const OutlineInputBorder( | ||
| borderRadius: BorderRadius.all(Radius.circular(10)), | ||
| ), | ||
| focusedBorder: const OutlineInputBorder( | ||
| borderRadius: BorderRadius.all(Radius.circular(10)), | ||
| borderSide: BorderSide( | ||
| color: Colors.white, | ||
| ), | ||
| ), | ||
| label: Row( | ||
| mainAxisSize: MainAxisSize.min, | ||
| children: [ | ||
| Icon(icon, | ||
| color: WidgetStateColor.resolveWith( | ||
| (states) => states.contains(WidgetState.focused) ? Colors.white : Colors.grey)), | ||
| const SizedBox( | ||
| width: 8, | ||
| ), | ||
| Text( | ||
| label, | ||
| ), | ||
| ], | ||
| ), | ||
| alignLabelWithHint: true, | ||
| labelStyle: const TextStyle( | ||
| color: Colors.grey, | ||
| ), | ||
| floatingLabelStyle: const TextStyle( | ||
| color: Colors.white, | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use theme-based styling instead of hard-coded colors.
The current implementation uses hard-coded colors which may not adapt well to different themes or color schemes. Consider:
- Using Theme.of(context) for colors
- Extracting repeated border radius to a constant
- Supporting both light and dark modes
+ static const borderRadius = BorderRadius.all(Radius.circular(10));
+
decoration: InputDecoration(
border: const OutlineInputBorder(
- borderRadius: BorderRadius.all(Radius.circular(10)),
+ borderRadius: borderRadius,
),
focusedBorder: const OutlineInputBorder(
- borderRadius: BorderRadius.all(Radius.circular(10)),
+ borderRadius: borderRadius,
borderSide: BorderSide(
- color: Colors.white,
+ color: Theme.of(context).colorScheme.primary,
),
),
// ... other decorations ...
labelStyle: TextStyle(
- color: Colors.grey,
+ color: Theme.of(context).colorScheme.onSurface.withOpacity(0.6),
),
floatingLabelStyle: TextStyle(
- color: Colors.white,
+ color: Theme.of(context).colorScheme.primary,
),Committable suggestion skipped: line range outside the PR's diff.
| def enable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | ||
| plugin = get_plugin_by_id_db(plugin_id, uid) | ||
| plugin = Plugin(**plugin) | ||
| if not plugin: | ||
| raise HTTPException(status_code=404, detail='Plugin not found') | ||
| if plugin.works_externally() and plugin.external_integration.setup_completed_url: | ||
| res = requests.get(plugin.external_integration.setup_completed_url + f'?uid={uid}') | ||
| print('enable_plugin_endpoint', res.status_code, res.content) | ||
| if res.status_code != 200 or not res.json().get('is_setup_completed', False): | ||
| raise HTTPException(status_code=400, detail='Plugin setup is not completed') | ||
| increase_plugin_installs_count(plugin_id) | ||
| enable_plugin(uid, plugin_id) | ||
| return {'status': 'ok'} | ||
|
|
There was a problem hiding this comment.
Avoid redefining enable_plugin_endpoint; function already defined earlier
Redefining enable_plugin_endpoint at line 122 overwrites the existing function defined earlier, which can lead to unexpected behavior. Consider renaming the new function to prevent conflicts.
Apply this diff to rename the function:
-def enable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)):
+def enable_plugin_v2_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)):📝 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 enable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | |
| plugin = get_plugin_by_id_db(plugin_id, uid) | |
| plugin = Plugin(**plugin) | |
| if not plugin: | |
| raise HTTPException(status_code=404, detail='Plugin not found') | |
| if plugin.works_externally() and plugin.external_integration.setup_completed_url: | |
| res = requests.get(plugin.external_integration.setup_completed_url + f'?uid={uid}') | |
| print('enable_plugin_endpoint', res.status_code, res.content) | |
| if res.status_code != 200 or not res.json().get('is_setup_completed', False): | |
| raise HTTPException(status_code=400, detail='Plugin setup is not completed') | |
| increase_plugin_installs_count(plugin_id) | |
| enable_plugin(uid, plugin_id) | |
| return {'status': 'ok'} | |
| def enable_plugin_v2_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | |
| plugin = get_plugin_by_id_db(plugin_id, uid) | |
| plugin = Plugin(**plugin) | |
| if not plugin: | |
| raise HTTPException(status_code=404, detail='Plugin not found') | |
| if plugin.works_externally() and plugin.external_integration.setup_completed_url: | |
| res = requests.get(plugin.external_integration.setup_completed_url + f'?uid={uid}') | |
| print('enable_plugin_endpoint', res.status_code, res.content) | |
| if res.status_code != 200 or not res.json().get('is_setup_completed', False): | |
| raise HTTPException(status_code=400, detail='Plugin setup is not completed') | |
| increase_plugin_installs_count(plugin_id) | |
| enable_plugin(uid, plugin_id) | |
| return {'status': 'ok'} |
🧰 Tools
🪛 Ruff
122-122: Redefinition of unused enable_plugin_endpoint from line 25
(F811)
| color: WidgetStateColor.resolveWith( | ||
| (states) => states.contains(WidgetState.focused) ? Colors.white : Colors.grey)), | ||
| const SizedBox( |
There was a problem hiding this comment.
Incorrect usage of WidgetStateColor and WidgetState
The classes WidgetStateColor and WidgetState do not exist in Flutter. Instead, you should use MaterialStateColor and MaterialState to properly handle color changes based on widget states.
Apply this diff to fix the issue:
- color: WidgetStateColor.resolveWith(
- (states) => states.contains(WidgetState.focused) ? Colors.white : Colors.grey)),
+ color: MaterialStateColor.resolveWith(
+ (states) => states.contains(MaterialState.focused) ? Colors.white : Colors.grey)),Make this change in all the affected lines where WidgetStateColor and WidgetState are used:
- Lines 204-206
- Lines 247-249
- Lines 299-301
- Lines 343-345
- Lines 387-389
- Lines 467-469
Also applies to: 247-249, 299-301, 343-345, 387-389, 467-469
| TextFormField( | ||
| controller: provider.appNameController, | ||
| validator: (value) { | ||
| if (value == null || value.isEmpty) { | ||
| return 'Please enter app name'; | ||
| } | ||
| return null; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation to ensure an app icon is uploaded
Currently, there's no validation to check if an app icon has been uploaded before form submission. This could lead to apps without icons being submitted.
Consider adding a validator in the validateForm method to check if provider.imageFile is not null. For example:
if (provider.imageFile == null) {
// Show an error message
return false;
}| value: provider.termsAgreed, | ||
| onChanged: provider.setTermsAgreed, | ||
| ), |
There was a problem hiding this comment.
Ensure terms are agreed before submission
There is no validation to confirm that the user has agreed to the Terms of Service and Privacy Policy before submitting the app.
Add validation to check provider.termsAgreed in the validateForm method and prevent form submission if it's false.
Example:
// In validateForm method
if (!termsAgreed) {
// Show an error message
return false;
}And provide feedback to the user in case they haven't agreed:
if (!provider.termsAgreed) {
ScaffoldMessenger.of(context).showSnackBar(
SnackBar(content: Text('You must agree to the Terms of Service and Privacy Policy')),
);
return;
}Also applies to: 531-563
| if (isValid) { | ||
| showDialog( | ||
| context: context, | ||
| builder: (ctx) { | ||
| return ConfirmationDialog( | ||
| title: 'Submit App?', | ||
| description: provider.privacyLevel == 'public' | ||
| ? 'Your app will be reviewed and made public. You can start using it immediately, even during the review!' | ||
| : 'Your app will be reviewed and made available to you privately. You can start using it immediately, even during the review!', | ||
| checkboxText: "Don't show it again", | ||
| checkboxValue: !showSubmitAppConfirmation, | ||
| updateCheckboxValue: (value) { | ||
| if (value != null) { | ||
| setState(() { | ||
| showSubmitAppConfirmation = !value; | ||
| }); | ||
| } | ||
| }, | ||
| onConfirm: () async { | ||
| SharedPreferencesUtil().showSubmitAppConfirmation = showSubmitAppConfirmation; | ||
|
|
||
| Navigator.pop(context); | ||
| await provider.submitApp(); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors during app submission
While submitting the app using provider.submitApp(), consider handling exceptions and providing user feedback in case of failures.
Apply this diff to enhance error handling:
onPressed: () {
var isValid = provider.validateForm();
if (isValid) {
+ try {
if (showSubmitAppConfirmation) {
// Existing code to show confirmation dialog
} else {
// Proceed with submission
+ await provider.submitApp();
+ // Provide success feedback
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(content: Text('App submitted successfully')),
+ );
}
+ } catch (e) {
+ // Handle submission error
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(content: Text('Failed to submit app: \$e')),
+ );
+ }
}
},Committable suggestion skipped: line range outside the PR's diff.
|
1/ provide steps to deploy please |
|
6/ could we navigate the submit.omi.me to the app? how about our web portal to submit new apps ? *i will not edit the previous comment to ensure you dont miss anything as updates wouldnt be sent as a notification. |
|
1/ Same existing backend deploy steps sir Will ping you once I make the necessary changes @beastoin |
|
Made the necessary changes, review pls @beastoin |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (3)
backend/database/plugins.py (2)
16-16: Define bucket name as a constantConsider defining the bucket name as a module-level constant for better maintainability:
-omi_plugins_bucket = os.getenv('BUCKET_PLUGINS_LOGOS') +PLUGINS_BUCKET_NAME = os.getenv('BUCKET_PLUGINS_LOGOS') +if not PLUGINS_BUCKET_NAME: + raise ValueError("BUCKET_PLUGINS_LOGOS environment variable is not set")
Line range hint
1-126: Enhance code documentation and type safetyThe codebase would benefit from:
- Adding docstrings to all functions
- Consistent type hints
- Return type annotations
Example for one function:
-def change_plugin_approval_status(plugin_id: str, approved: bool): +def change_plugin_approval_status(plugin_id: str, approved: bool) -> None: + """Update the approval status of a plugin. + + Args: + plugin_id (str): The ID of the plugin to update + approved (bool): The new approval status + + Raises: + ValueError: If plugin_id is empty or plugin doesn't exist + """ + if not plugin_id: + raise ValueError("plugin_id cannot be empty") plugin_ref = db.collection('plugins_data').document(plugin_id) plugin_ref.update({'approved': approved})backend/routers/plugins.py (1)
218-218: Address TODO for audio_bytes triggerThe comment indicates that audio_bytes trigger functionality needs to be implemented.
Would you like me to help implement the audio_bytes trigger functionality or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
backend/.env.template(1 hunks)backend/database/plugins.py(2 hunks)backend/routers/chat.py(4 hunks)backend/routers/plugins.py(2 hunks)backend/utils/other/storage.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/.env.template
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/routers/chat.py
- backend/utils/other/storage.py
🧰 Additional context used
🪛 Ruff
backend/routers/plugins.py
122-122: Redefinition of unused enable_plugin_endpoint from line 25
(F811)
138-138: Redefinition of unused disable_plugin_endpoint from line 40
(F811)
148-148: Redefinition of unused get_plugins from line 60
(F811)
163-163: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
163-163: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
173-173: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (2)
backend/database/plugins.py (1)
102-104: LGTM!
The implementation is correct and efficient.
backend/routers/plugins.py (1)
200-207: LGTM! Well-implemented visibility change endpoint
The implementation includes proper authentication, error handling, and maintains the plugin's visibility state correctly.
| def get_private_plugins_db(uid: str) -> List: | ||
| private_plugins = db.collection('users').document(uid).collection('plugins').stream() | ||
| data = [doc.to_dict() for doc in private_plugins] | ||
| return data | ||
|
|
||
|
|
||
| def get_unapproved_public_plugins_db() -> List: | ||
| public_plugins = db.collection('plugins_data').where('approved', '==', False).stream() | ||
| return [doc.to_dict() for doc in public_plugins] | ||
|
|
||
|
|
||
| def get_public_plugins_db(uid: str) -> List: | ||
| public_plugins = db.collection('plugins_data').where('approved', '==', True).stream() | ||
| data = [doc.to_dict() for doc in public_plugins] | ||
|
|
||
| # Include the doc if it is not approved but uid matches | ||
| unapproved = db.collection('plugins_data').where('approved', '==', False).where('uid', '==', uid).stream() | ||
| data.extend([doc.to_dict() for doc in unapproved]) | ||
|
|
||
| return data |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement pagination for plugin retrieval
These functions fetch all documents at once, which could cause performance issues with large datasets. Consider implementing pagination.
-def get_public_plugins_db(uid: str) -> List:
+def get_public_plugins_db(uid: str, page_size: int = 20, last_doc = None) -> tuple[List, any]:
public_plugins = db.collection('plugins_data').where('approved', '==', True)
+ if last_doc:
+ public_plugins = public_plugins.start_after(last_doc)
+ public_plugins = public_plugins.limit(page_size).stream()
- data = [doc.to_dict() for doc in public_plugins]
+ docs = list(public_plugins)
+ data = [doc.to_dict() for doc in docs]
+ last_doc = docs[-1] if docs else None
- return data
+ return data, last_docCommittable suggestion skipped: line range outside the PR's diff.
| def get_plugin_by_id_db(plugin_id: str, uid: str): | ||
| if 'private' in plugin_id: | ||
| plugin_ref = db.collection('users').document(uid).collection('plugins').document(plugin_id) | ||
| else: | ||
| plugin_ref = db.collection('plugins_data').document(plugin_id) | ||
| return plugin_ref.get().to_dict() |
There was a problem hiding this comment.
Add error handling and validation for plugin retrieval
The function needs proper error handling for non-existent documents and uid validation.
-def get_plugin_by_id_db(plugin_id: str, uid: str):
+def get_plugin_by_id_db(plugin_id: str, uid: str | None = None):
+ if not plugin_id:
+ raise ValueError("plugin_id cannot be empty")
+ if 'private' in plugin_id and not uid:
+ raise ValueError("uid is required for private plugins")
if 'private' in plugin_id:
plugin_ref = db.collection('users').document(uid).collection('plugins').document(plugin_id)
else:
plugin_ref = db.collection('plugins_data').document(plugin_id)
- return plugin_ref.get().to_dict()
+ doc = plugin_ref.get()
+ if not doc.exists:
+ return None
+ return doc.to_dict()📝 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_plugin_by_id_db(plugin_id: str, uid: str): | |
| if 'private' in plugin_id: | |
| plugin_ref = db.collection('users').document(uid).collection('plugins').document(plugin_id) | |
| else: | |
| plugin_ref = db.collection('plugins_data').document(plugin_id) | |
| return plugin_ref.get().to_dict() | |
| def get_plugin_by_id_db(plugin_id: str, uid: str | None = None): | |
| if not plugin_id: | |
| raise ValueError("plugin_id cannot be empty") | |
| if 'private' in plugin_id and not uid: | |
| raise ValueError("uid is required for private plugins") | |
| if 'private' in plugin_id: | |
| plugin_ref = db.collection('users').document(uid).collection('plugins').document(plugin_id) | |
| else: | |
| plugin_ref = db.collection('plugins_data').document(plugin_id) | |
| doc = plugin_ref.get() | |
| if not doc.exists: | |
| return None | |
| return doc.to_dict() |
| def change_plugin_visibility_db(plugin_id: str, private: bool, was_public: bool, uid: str): | ||
| if was_public and private: # public -> private | ||
| plugin_ref = db.collection('plugins_data').document(plugin_id) | ||
| plugin = plugin_ref.get().to_dict() | ||
| plugin_ref.delete() | ||
| plugin_ref = db.collection('users').document(uid).collection('plugins').document(plugin_id) | ||
| plugin['private'] = private | ||
| plugin_ref.set(plugin) | ||
| elif not was_public and not private: # private -> public | ||
| plugin_ref = db.collection('users').document(uid).collection('plugins').document(plugin_id) | ||
| plugin = plugin_ref.get().to_dict() | ||
| plugin_ref.delete() | ||
| plugin_ref = db.collection('plugins_data').document(plugin_id) | ||
| plugin_ref.set(plugin) | ||
|
|
There was a problem hiding this comment.
Use transactions for atomic visibility changes
The current implementation risks data loss if the operation fails between delete and create operations. Use a transaction to ensure atomicity.
def change_plugin_visibility_db(plugin_id: str, private: bool, was_public: bool, uid: str):
+ @db.transaction
+ def update_visibility_atomic(transaction):
if was_public and private: # public -> private
plugin_ref = db.collection('plugins_data').document(plugin_id)
- plugin = plugin_ref.get().to_dict()
+ plugin = plugin_ref.get(transaction=transaction).to_dict()
plugin_ref.delete()
plugin_ref = db.collection('users').document(uid).collection('plugins').document(plugin_id)
plugin['private'] = private
- plugin_ref.set(plugin)
+ transaction.set(plugin_ref, plugin)
elif not was_public and not private: # private -> public
plugin_ref = db.collection('users').document(uid).collection('plugins').document(plugin_id)
- plugin = plugin_ref.get().to_dict()
+ plugin = plugin_ref.get(transaction=transaction).to_dict()
plugin_ref.delete()
plugin_ref = db.collection('plugins_data').document(plugin_id)
- plugin_ref.set(plugin)
+ transaction.set(plugin_ref, plugin)
+
+ update_visibility_atomic()Committable suggestion skipped: line range outside the PR's diff.
| # TODO: only temporary, to move from the json file to firestore. Remove after the migration | ||
| def add_plugin_script(plugin_data: dict): | ||
| img = requests.get("https://raw.githubusercontent.com/BasedHardware/Omi/main/" + plugin_data['image'], stream=True) | ||
| bucket = storage_client.bucket(omi_plugins_bucket) | ||
| path = plugin_data['image'].split('/plugins/logos/')[1] | ||
| blob = bucket.blob(path) | ||
| blob.upload_from_file(img.raw) | ||
| plugin_data['image'] = f'https://storage.googleapis.com/{omi_plugins_bucket}/{path}' | ||
| plugin_data['private'] = False | ||
| plugin_data['approved'] = True | ||
|
|
||
| if "external_integration" in plugin_data['capabilities']: | ||
| plugin_data['external_integration'][ | ||
| 'setup_instructions_file_path'] = "https://raw.githubusercontent.com/BasedHardware/Omi/main/" + \ | ||
| plugin_data['external_integration']['setup_instructions_file_path'] | ||
| plugin_ref = db.collection('plugins_data').document(plugin_data['id']) | ||
| if plugin_ref.get().exists: | ||
| plugin_ref.update(plugin_data) | ||
| else: | ||
| plugin_ref.set(plugin_data) |
There was a problem hiding this comment.
Add comprehensive error handling and validation
The function needs better error handling and input validation to prevent potential issues.
def add_plugin_script(plugin_data: dict):
+ if not isinstance(plugin_data, dict) or 'image' not in plugin_data or 'id' not in plugin_data:
+ raise ValueError("Invalid plugin data structure")
+
+ try:
img = requests.get(
"https://raw.githubusercontent.com/BasedHardware/Omi/main/" + plugin_data['image'],
stream=True
)
+ img.raise_for_status()
+ except requests.RequestException as e:
+ raise RuntimeError(f"Failed to download image: {e}")
+
+ try:
bucket = storage_client.bucket(omi_plugins_bucket)
- path = plugin_data['image'].split('/plugins/logos/')[1]
+ path = os.path.basename(plugin_data['image'])
blob = bucket.blob(path)
blob.upload_from_file(img.raw)
+ except Exception as e:
+ raise RuntimeError(f"Failed to upload image to storage: {e}")Committable suggestion skipped: line range outside the PR's diff.
| @router.get('/v3/plugins', tags=['v3'], response_model=List[Plugin]) | ||
| def get_plugins(uid: str = Depends(auth.get_current_user_uid), include_reviews: bool = True): | ||
| return get_plugins_data_from_db(uid, include_reviews=include_reviews) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename v3 endpoint function to avoid conflicts
The get_plugins function is redefined from its v1 and v2 versions.
Apply this diff:
@router.get('/v3/plugins', tags=['v3'], response_model=List[Plugin])
-def get_plugins(uid: str = Depends(auth.get_current_user_uid), include_reviews: bool = True):
+def get_plugins_v3(uid: str = Depends(auth.get_current_user_uid), include_reviews: bool = True):📝 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('/v3/plugins', tags=['v3'], response_model=List[Plugin]) | |
| def get_plugins(uid: str = Depends(auth.get_current_user_uid), include_reviews: bool = True): | |
| return get_plugins_data_from_db(uid, include_reviews=include_reviews) | |
| @router.get('/v3/plugins', tags=['v3'], response_model=List[Plugin]) | |
| def get_plugins_v3(uid: str = Depends(auth.get_current_user_uid), include_reviews: bool = True): | |
| return get_plugins_data_from_db(uid, include_reviews=include_reviews) |
🧰 Tools
🪛 Ruff
148-148: Redefinition of unused get_plugins from line 60
(F811)
| data['approved'] = False | ||
| data['id'] = data['name'].replace(' ', '-').lower() | ||
| data['uid'] = uid | ||
| if data['private']: | ||
| data['id'] = data['id'] + '-private' | ||
| else: | ||
| if plugin_id_exists_db(data['id']): | ||
| data['id'] = data['id'] + '-' + ''.join([str(random.randint(0, 9)) for _ in range(5)]) |
There was a problem hiding this comment.
Improve plugin ID generation security
The current ID generation method could be vulnerable to path traversal attacks and collisions.
Apply this diff to improve security:
data['approved'] = False
- data['id'] = data['name'].replace(' ', '-').lower()
+ # Sanitize plugin ID to prevent path traversal and ensure uniqueness
+ import re
+ import uuid
+ base_id = re.sub(r'[^a-z0-9-]', '-', data['name'].lower())
+ base_id = re.sub(r'-+', '-', base_id).strip('-')
+ data['id'] = f"{base_id}-{str(uuid.uuid4())[:8]}"
data['uid'] = uid
if data['private']:
data['id'] = data['id'] + '-private'
- else:
- if plugin_id_exists_db(data['id']):
- data['id'] = data['id'] + '-' + ''.join([str(random.randint(0, 9)) for _ in range(5)])📝 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.
| data['approved'] = False | |
| data['id'] = data['name'].replace(' ', '-').lower() | |
| data['uid'] = uid | |
| if data['private']: | |
| data['id'] = data['id'] + '-private' | |
| else: | |
| if plugin_id_exists_db(data['id']): | |
| data['id'] = data['id'] + '-' + ''.join([str(random.randint(0, 9)) for _ in range(5)]) | |
| data['approved'] = False | |
| # Sanitize plugin ID to prevent path traversal and ensure uniqueness | |
| import re | |
| import uuid | |
| base_id = re.sub(r'[^a-z0-9-]', '-', data['name'].lower()) | |
| base_id = re.sub(r'-+', '-', base_id).strip('-') | |
| data['id'] = f"{base_id}-{str(uuid.uuid4())[:8]}" | |
| data['uid'] = uid | |
| if data['private']: | |
| data['id'] = data['id'] + '-private' |
| @router.get('/v1/migrate-plugins', tags=['v1']) | ||
| def migrate_plugins(): | ||
| response = requests.get('https://raw.githubusercontent.com/BasedHardware/Omi/main/community-plugins.json') | ||
| if response.status_code != 200: | ||
| return [] | ||
| data = response.json() | ||
| for plugin in data: | ||
| add_plugin_script(plugin) | ||
|
|
There was a problem hiding this comment.
Add error handling for plugin migration
The migration endpoint lacks proper error handling for JSON parsing and validation of plugin data structure.
Apply this diff to add proper error handling:
@router.get('/v1/migrate-plugins', tags=['v1'])
def migrate_plugins():
response = requests.get('https://raw.githubusercontent.com/BasedHardware/Omi/main/community-plugins.json')
if response.status_code != 200:
return []
- data = response.json()
- for plugin in data:
- add_plugin_script(plugin)
+ try:
+ data = response.json()
+ for plugin in data:
+ if not isinstance(plugin, dict) or 'name' not in plugin:
+ continue
+ add_plugin_script(plugin)
+ return {'status': 'ok', 'count': len(data)}
+ except json.JSONDecodeError:
+ raise HTTPException(status_code=500, detail='Invalid JSON data from source')📝 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/migrate-plugins', tags=['v1']) | |
| def migrate_plugins(): | |
| response = requests.get('https://raw.githubusercontent.com/BasedHardware/Omi/main/community-plugins.json') | |
| if response.status_code != 200: | |
| return [] | |
| data = response.json() | |
| for plugin in data: | |
| add_plugin_script(plugin) | |
| @router.get('/v1/migrate-plugins', tags=['v1']) | |
| def migrate_plugins(): | |
| response = requests.get('https://raw.githubusercontent.com/BasedHardware/Omi/main/community-plugins.json') | |
| if response.status_code != 200: | |
| return [] | |
| try: | |
| data = response.json() | |
| for plugin in data: | |
| if not isinstance(plugin, dict) or 'name' not in plugin: | |
| continue | |
| add_plugin_script(plugin) | |
| return {'status': 'ok', 'count': len(data)} | |
| except json.JSONDecodeError: | |
| raise HTTPException(status_code=500, detail='Invalid JSON data from source') |
| def enable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | ||
| plugin = get_plugin_by_id_db(plugin_id, uid) | ||
| plugin = Plugin(**plugin) | ||
| if not plugin: | ||
| raise HTTPException(status_code=404, detail='Plugin not found') | ||
| if plugin.works_externally() and plugin.external_integration.setup_completed_url: | ||
| res = requests.get(plugin.external_integration.setup_completed_url + f'?uid={uid}') | ||
| print('enable_plugin_endpoint', res.status_code, res.content) | ||
| if res.status_code != 200 or not res.json().get('is_setup_completed', False): | ||
| raise HTTPException(status_code=400, detail='Plugin setup is not completed') | ||
| increase_plugin_installs_count(plugin_id) | ||
| enable_plugin(uid, plugin_id) | ||
| return {'status': 'ok'} |
There was a problem hiding this comment.
Fix critical error in plugin existence check
The error check is performed after attempting to create the Plugin instance, which could cause a runtime error if the plugin doesn't exist.
Apply this diff to fix the error handling flow:
def enable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)):
plugin = get_plugin_by_id_db(plugin_id, uid)
- plugin = Plugin(**plugin)
if not plugin:
raise HTTPException(status_code=404, detail='Plugin not found')
+ plugin = Plugin(**plugin)
if plugin.works_externally() and plugin.external_integration.setup_completed_url:📝 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 enable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | |
| plugin = get_plugin_by_id_db(plugin_id, uid) | |
| plugin = Plugin(**plugin) | |
| if not plugin: | |
| raise HTTPException(status_code=404, detail='Plugin not found') | |
| if plugin.works_externally() and plugin.external_integration.setup_completed_url: | |
| res = requests.get(plugin.external_integration.setup_completed_url + f'?uid={uid}') | |
| print('enable_plugin_endpoint', res.status_code, res.content) | |
| if res.status_code != 200 or not res.json().get('is_setup_completed', False): | |
| raise HTTPException(status_code=400, detail='Plugin setup is not completed') | |
| increase_plugin_installs_count(plugin_id) | |
| enable_plugin(uid, plugin_id) | |
| return {'status': 'ok'} | |
| def enable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | |
| plugin = get_plugin_by_id_db(plugin_id, uid) | |
| if not plugin: | |
| raise HTTPException(status_code=404, detail='Plugin not found') | |
| plugin = Plugin(**plugin) | |
| if plugin.works_externally() and plugin.external_integration.setup_completed_url: | |
| res = requests.get(plugin.external_integration.setup_completed_url + f'?uid={uid}') | |
| print('enable_plugin_endpoint', res.status_code, res.content) | |
| if res.status_code != 200 or not res.json().get('is_setup_completed', False): | |
| raise HTTPException(status_code=400, detail='Plugin setup is not completed') | |
| increase_plugin_installs_count(plugin_id) | |
| enable_plugin(uid, plugin_id) | |
| return {'status': 'ok'} |
🧰 Tools
🪛 Ruff
122-122: Redefinition of unused enable_plugin_endpoint from line 25
(F811)
| @router.post('/v2/plugins/enable') | ||
| def enable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | ||
| plugin = get_plugin_by_id_db(plugin_id, uid) | ||
| plugin = Plugin(**plugin) | ||
| if not plugin: | ||
| raise HTTPException(status_code=404, detail='Plugin not found') | ||
| if plugin.works_externally() and plugin.external_integration.setup_completed_url: | ||
| res = requests.get(plugin.external_integration.setup_completed_url + f'?uid={uid}') | ||
| print('enable_plugin_endpoint', res.status_code, res.content) | ||
| if res.status_code != 200 or not res.json().get('is_setup_completed', False): | ||
| raise HTTPException(status_code=400, detail='Plugin setup is not completed') | ||
| increase_plugin_installs_count(plugin_id) | ||
| enable_plugin(uid, plugin_id) | ||
| return {'status': 'ok'} | ||
|
|
||
|
|
||
| @router.post('/v2/plugins/disable') | ||
| def disable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | ||
| plugin = get_plugin_by_id_db(plugin_id, uid) | ||
| if not plugin: | ||
| raise HTTPException(status_code=404, detail='Plugin not found') | ||
| disable_plugin(uid, plugin_id) | ||
| decrease_plugin_installs_count(plugin_id) | ||
| return {'status': 'ok'} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename v2 endpoint functions to avoid conflicts
Both enable_plugin_endpoint and disable_plugin_endpoint are redefined from their v1 versions, which could lead to confusion and potential bugs.
Apply this diff to rename the functions:
@router.post('/v2/plugins/enable')
-def enable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)):
+def enable_plugin_v2_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)):
@router.post('/v2/plugins/disable')
-def disable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)):
+def disable_plugin_v2_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)):📝 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.post('/v2/plugins/enable') | |
| def enable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | |
| plugin = get_plugin_by_id_db(plugin_id, uid) | |
| plugin = Plugin(**plugin) | |
| if not plugin: | |
| raise HTTPException(status_code=404, detail='Plugin not found') | |
| if plugin.works_externally() and plugin.external_integration.setup_completed_url: | |
| res = requests.get(plugin.external_integration.setup_completed_url + f'?uid={uid}') | |
| print('enable_plugin_endpoint', res.status_code, res.content) | |
| if res.status_code != 200 or not res.json().get('is_setup_completed', False): | |
| raise HTTPException(status_code=400, detail='Plugin setup is not completed') | |
| increase_plugin_installs_count(plugin_id) | |
| enable_plugin(uid, plugin_id) | |
| return {'status': 'ok'} | |
| @router.post('/v2/plugins/disable') | |
| def disable_plugin_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | |
| plugin = get_plugin_by_id_db(plugin_id, uid) | |
| if not plugin: | |
| raise HTTPException(status_code=404, detail='Plugin not found') | |
| disable_plugin(uid, plugin_id) | |
| decrease_plugin_installs_count(plugin_id) | |
| return {'status': 'ok'} | |
| @router.post('/v2/plugins/enable') | |
| def enable_plugin_v2_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | |
| plugin = get_plugin_by_id_db(plugin_id, uid) | |
| plugin = Plugin(**plugin) | |
| if not plugin: | |
| raise HTTPException(status_code=404, detail='Plugin not found') | |
| if plugin.works_externally() and plugin.external_integration.setup_completed_url: | |
| res = requests.get(plugin.external_integration.setup_completed_url + f'?uid={uid}') | |
| print('enable_plugin_endpoint', res.status_code, res.content) | |
| if res.status_code != 200 or not res.json().get('is_setup_completed', False): | |
| raise HTTPException(status_code=400, detail='Plugin setup is not completed') | |
| increase_plugin_installs_count(plugin_id) | |
| enable_plugin(uid, plugin_id) | |
| return {'status': 'ok'} | |
| @router.post('/v2/plugins/disable') | |
| def disable_plugin_v2_endpoint(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)): | |
| plugin = get_plugin_by_id_db(plugin_id, uid) | |
| if not plugin: | |
| raise HTTPException(status_code=404, detail='Plugin not found') | |
| disable_plugin(uid, plugin_id) | |
| decrease_plugin_installs_count(plugin_id) | |
| return {'status': 'ok'} |
🧰 Tools
🪛 Ruff
122-122: Redefinition of unused enable_plugin_endpoint from line 25
(F811)
138-138: Redefinition of unused disable_plugin_endpoint from line 40
(F811)
|
1/ deploy steps, for example:
please notice about the downtime, try avoiding it. 2/ ok
5/ ok, add the auth - admin only and create new ticket to remove it. @mdmohsin7 fighting |
|
1/ Oh got it. It will be like this:
4/ Alright will check and make the necessary changes |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (3)
backend/database/plugins.py (1)
102-109: Consider caching plugin existence checksThese functions might be called frequently during plugin operations. Consider implementing caching to reduce database reads.
Consider using a distributed cache like Redis or Memcached to cache existence checks with a short TTL (e.g., 5 minutes).
app/lib/backend/http/api/apps.dart (1)
167-203: Improve error handling and response parsing.Both functions could benefit from more robust error handling and response parsing:
- Add specific error types for better error handling
- Validate response structure before parsing
- Add retry mechanism for transient failures
Example improvement for
getAppCategories(apply similar pattern togetTriggerEventsServer):Future<List<Category>> getAppCategories() async { + const maxRetries = 3; + var attempt = 0; + + while (attempt < maxRetries) { + try { var response = await makeApiCall( url: '${Env.apiBaseUrl}v1/plugin-categories', headers: {}, body: '', method: 'GET', ); - try { - if (response == null || response.statusCode != 200) return []; - log('getAppCategories: ${response.body}'); - var res = jsonDecode(response.body); + if (response == null) { + throw ApiException('No response from server'); + } + + if (response.statusCode != 200) { + throw ApiException('Server returned ${response.statusCode}'); + } + + if (response.body.isEmpty) { + return []; + } + + log('getAppCategories: ${response.body}'); + var res = jsonDecode(response.body); + + if (!res.containsKey('categories')) { + throw FormatException('Invalid response format'); + } + return Category.fromJsonList(res); - } catch (e, stackTrace) { + } catch (e, stackTrace) { + attempt++; + if (e is ApiException || attempt >= maxRetries) { debugPrint(e.toString()); CrashReporting.reportHandledCrash(e, stackTrace); return []; - } + } + // Wait before retry + await Future.delayed(Duration(seconds: attempt)); + } + } + return []; }backend/routers/plugins.py (1)
196-196: Track TODO for audio_bytes trigger implementationThe TODO comment indicates pending work for the audio_bytes trigger implementation.
Would you like me to create a GitHub issue to track this TODO item?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/lib/backend/http/api/apps.dart(4 hunks)backend/database/plugins.py(2 hunks)backend/routers/plugins.py(3 hunks)
🧰 Additional context used
🪛 Ruff
backend/routers/plugins.py
135-135: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
135-135: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
147-147: f-string without any placeholders
Remove extraneous f prefix
(F541)
163-163: Redefinition of unused get_plugins from line 63
(F811)
🔇 Additional comments (4)
app/lib/backend/http/api/apps.dart (1)
74-74: LGTM! URL simplification improves flexibility.
The direct use of appMarkdownPath removes hardcoding and allows for more flexible markdown source locations.
backend/routers/plugins.py (3)
178-185: LGTM! Well-implemented visibility management
The implementation includes proper authentication, error handling, and state tracking.
188-191:
Add authentication for accessing unapproved plugins
The endpoint exposes unapproved plugins without authentication, which could leak sensitive information.
Apply this diff:
@router.get('/v1/plugins/public/unapproved', tags=['v1'])
-def get_unapproved_public_plugins():
+def get_unapproved_public_plugins(uid: str = Depends(auth.get_current_user_uid)):
+ if not is_user_admin(uid):
+ raise HTTPException(status_code=403, detail='Not authorized to view unapproved plugins')
plugins = get_unapproved_public_plugins_db()
return pluginsLikely invalid or redundant comment.
166-175:
Add authentication and authorization checks
The approve/reject endpoints lack authentication and authorization checks, allowing any user to modify plugin approval status.
Apply this diff to both endpoints:
@router.post('/v1/plugins/{plugin_id}/approve', tags=['v1'])
-def approve_plugin(plugin_id: str):
+def approve_plugin(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)):
+ if not is_user_admin(uid):
+ raise HTTPException(status_code=403, detail='Not authorized to approve plugins')
change_plugin_approval_status(plugin_id, True)
return {'status': 'ok'}
@router.post('/v1/plugins/{plugin_id}/reject', tags=['v1'])
-def reject_plugin(plugin_id: str):
+def reject_plugin(plugin_id: str, uid: str = Depends(auth.get_current_user_uid)):
+ if not is_user_admin(uid):
+ raise HTTPException(status_code=403, detail='Not authorized to reject plugins')
change_plugin_approval_status(plugin_id, False)
return {'status': 'ok'}Likely invalid or redundant comment.
| # ********** CRUD ************* | ||
| # ***************************** | ||
|
|
||
| omi_plugins_bucket = os.getenv('BUCKET_PLUGINS_LOGOS') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add environment variable validation
The omi_plugins_bucket environment variable is critical for plugin logo storage. Add validation to ensure it's properly set.
-omi_plugins_bucket = os.getenv('BUCKET_PLUGINS_LOGOS')
+omi_plugins_bucket = os.getenv('BUCKET_PLUGINS_LOGOS')
+if not omi_plugins_bucket:
+ raise ValueError('BUCKET_PLUGINS_LOGOS environment variable must be set')📝 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.
| omi_plugins_bucket = os.getenv('BUCKET_PLUGINS_LOGOS') | |
| omi_plugins_bucket = os.getenv('BUCKET_PLUGINS_LOGOS') | |
| if not omi_plugins_bucket: | |
| raise ValueError('BUCKET_PLUGINS_LOGOS environment variable must be set') |
| def change_plugin_approval_status(plugin_id: str, approved: bool): | ||
| plugin_ref = db.collection('plugins_data').document(plugin_id) | ||
| plugin_ref.update({'approved': approved}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for approval status changes
The function should verify the document exists and handle potential Firestore exceptions.
def change_plugin_approval_status(plugin_id: str, approved: bool):
plugin_ref = db.collection('plugins_data').document(plugin_id)
- plugin_ref.update({'approved': approved})
+ doc = plugin_ref.get()
+ if not doc.exists:
+ raise ValueError(f"Plugin with id {plugin_id} does not exist")
+ try:
+ plugin_ref.update({'approved': approved})
+ except Exception as e:
+ raise RuntimeError(f"Failed to update plugin approval status: {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.
| def change_plugin_approval_status(plugin_id: str, approved: bool): | |
| plugin_ref = db.collection('plugins_data').document(plugin_id) | |
| plugin_ref.update({'approved': approved}) | |
| def change_plugin_approval_status(plugin_id: str, approved: bool): | |
| plugin_ref = db.collection('plugins_data').document(plugin_id) | |
| doc = plugin_ref.get() | |
| if not doc.exists: | |
| raise ValueError(f"Plugin with id {plugin_id} does not exist") | |
| try: | |
| plugin_ref.update({'approved': approved}) | |
| except Exception as e: | |
| raise RuntimeError(f"Failed to update plugin approval status: {e}") |
| def get_unapproved_public_plugins_db() -> List: | ||
| public_plugins = db.collection('plugins_data').where('approved', '==', False).stream() | ||
| return [doc.to_dict() for doc in public_plugins] |
There was a problem hiding this comment.
Add access control for unapproved plugins
The get_unapproved_public_plugins_db function allows unrestricted access to unapproved plugins. This could expose sensitive information.
-def get_unapproved_public_plugins_db() -> List:
+def get_unapproved_public_plugins_db(admin_uid: str = None) -> List:
+ if not admin_uid:
+ raise ValueError("Admin UID is required to access unapproved plugins")
+ admin_ref = db.collection('admins').document(admin_uid).get()
+ if not admin_ref.exists:
+ raise ValueError("Unauthorized access to unapproved plugins")
public_plugins = db.collection('plugins_data').where('approved', '==', False).stream()
return [doc.to_dict() for doc in public_plugins]Committable suggestion skipped: line range outside the PR's diff.
| def add_plugin_from_community_json(plugin_data: dict): | ||
| img = requests.get("https://raw.githubusercontent.com/BasedHardware/Omi/main/" + plugin_data['image'], stream=True) | ||
| bucket = storage_client.bucket(omi_plugins_bucket) | ||
| path = plugin_data['image'].split('/plugins/logos/')[1] | ||
| blob = bucket.blob(path) | ||
| blob.upload_from_file(img.raw) | ||
| plugin_data['image'] = f'https://storage.googleapis.com/{omi_plugins_bucket}/{path}' |
There was a problem hiding this comment.
Add file type validation for plugin logos
The function should validate the image file type and size before uploading to prevent security issues and ensure consistent quality.
def add_plugin_from_community_json(plugin_data: dict):
+ ALLOWED_EXTENSIONS = {'png', 'jpg', 'jpeg', 'gif'}
+ MAX_SIZE_BYTES = 5 * 1024 * 1024 # 5MB
+
+ # Validate image path
+ if not plugin_data.get('image'):
+ raise ValueError("Plugin image is required")
+
+ file_ext = plugin_data['image'].split('.')[-1].lower()
+ if file_ext not in ALLOWED_EXTENSIONS:
+ raise ValueError(f"Invalid file type. Allowed types: {ALLOWED_EXTENSIONS}")
+
img = requests.get("https://raw.githubusercontent.com/BasedHardware/Omi/main/" + plugin_data['image'], stream=True)
+ img.raise_for_status()
+
+ # Check file size
+ content_length = int(img.headers.get('content-length', 0))
+ if content_length > MAX_SIZE_BYTES:
+ raise ValueError(f"File size exceeds maximum limit of {MAX_SIZE_BYTES/1024/1024}MB")
+
bucket = storage_client.bucket(omi_plugins_bucket)
path = plugin_data['image'].split('/plugins/logos/')[1]
blob = bucket.blob(path)
blob.upload_from_file(img.raw)📝 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 add_plugin_from_community_json(plugin_data: dict): | |
| img = requests.get("https://raw.githubusercontent.com/BasedHardware/Omi/main/" + plugin_data['image'], stream=True) | |
| bucket = storage_client.bucket(omi_plugins_bucket) | |
| path = plugin_data['image'].split('/plugins/logos/')[1] | |
| blob = bucket.blob(path) | |
| blob.upload_from_file(img.raw) | |
| plugin_data['image'] = f'https://storage.googleapis.com/{omi_plugins_bucket}/{path}' | |
| def add_plugin_from_community_json(plugin_data: dict): | |
| ALLOWED_EXTENSIONS = {'png', 'jpg', 'jpeg', 'gif'} | |
| MAX_SIZE_BYTES = 5 * 1024 * 1024 # 5MB | |
| # Validate image path | |
| if not plugin_data.get('image'): | |
| raise ValueError("Plugin image is required") | |
| file_ext = plugin_data['image'].split('.')[-1].lower() | |
| if file_ext not in ALLOWED_EXTENSIONS: | |
| raise ValueError(f"Invalid file type. Allowed types: {ALLOWED_EXTENSIONS}") | |
| img = requests.get("https://raw.githubusercontent.com/BasedHardware/Omi/main/" + plugin_data['image'], stream=True) | |
| img.raise_for_status() | |
| # Check file size | |
| content_length = int(img.headers.get('content-length', 0)) | |
| if content_length > MAX_SIZE_BYTES: | |
| raise ValueError(f"File size exceeds maximum limit of {MAX_SIZE_BYTES/1024/1024}MB") | |
| bucket = storage_client.bucket(omi_plugins_bucket) | |
| path = plugin_data['image'].split('/plugins/logos/')[1] | |
| blob = bucket.blob(path) | |
| blob.upload_from_file(img.raw) | |
| plugin_data['image'] = f'https://storage.googleapis.com/{omi_plugins_bucket}/{path}' |
| Future<List<App>> retrieveApps() async { | ||
| var response = await makeApiCall( | ||
| url: '${Env.apiBaseUrl}v2/plugins', | ||
| url: '${Env.apiBaseUrl}v3/plugins', |
There was a problem hiding this comment.
Maintain API version consistency across endpoints.
The plugins endpoint has been upgraded to v3, but other related endpoints still use v1/v2. This inconsistency should be addressed:
submitAppServer: v3enableAppServer: v1- Other plugin endpoints: v1
Please ensure all plugin-related endpoints are consistently versioned. If v3 introduces breaking changes, document the migration path for clients.
| Future<bool> submitAppServer(File file, Map<String, dynamic> appData) async { | ||
| var request = http.MultipartRequest( | ||
| 'POST', | ||
| Uri.parse('${Env.apiBaseUrl}v3/plugins'), | ||
| ); | ||
| request.files.add(await http.MultipartFile.fromPath('file', file.path, filename: basename(file.path))); | ||
| request.headers.addAll({'Authorization': await getAuthHeader()}); | ||
| request.fields.addAll({'plugin_data': jsonEncode(appData)}); | ||
| print(jsonEncode(appData)); | ||
| try { | ||
| var streamedResponse = await request.send(); | ||
| var response = await http.Response.fromStream(streamedResponse); | ||
|
|
||
| if (response.statusCode == 200) { | ||
| debugPrint('submitAppServer Response body: ${jsonDecode(response.body)}'); | ||
| return true; | ||
| } else { | ||
| debugPrint('Failed to submit app. Status code: ${response.statusCode}'); | ||
| return false; | ||
| } | ||
| } catch (e) { | ||
| debugPrint('An error occurred submitAppServer: $e'); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Enhance file upload security and robustness.
The file upload implementation needs several critical improvements:
- Add file validation:
- File size limits
- File type validation
- Content type verification
- Improve error handling:
- Replace debug prints with proper logging
- Add specific error types
- Add upload timeout
- Add security headers
Here's the suggested implementation:
Future<bool> submitAppServer(File file, Map<String, dynamic> appData) async {
+ const maxSizeBytes = 10 * 1024 * 1024; // 10MB
+ if (await file.length() > maxSizeBytes) {
+ log('Error: File size exceeds maximum allowed size');
+ return false;
+ }
+
+ final allowedTypes = ['.zip', '.tar.gz'];
+ if (!allowedTypes.contains(extension(file.path).toLowerCase())) {
+ log('Error: Invalid file type');
+ return false;
+ }
+
var request = http.MultipartRequest(
'POST',
Uri.parse('${Env.apiBaseUrl}v3/plugins'),
);
request.files.add(await http.MultipartFile.fromPath('file', file.path, filename: basename(file.path)));
- request.headers.addAll({'Authorization': await getAuthHeader()});
+ request.headers.addAll({
+ 'Authorization': await getAuthHeader(),
+ 'Content-Type': 'multipart/form-data',
+ 'Accept': 'application/json',
+ });
request.fields.addAll({'plugin_data': jsonEncode(appData)});
- print(jsonEncode(appData));
+ log('Submitting app data: ${jsonEncode(appData)}');
try {
- var streamedResponse = await request.send();
+ var streamedResponse = await request.send().timeout(
+ const Duration(minutes: 5),
+ onTimeout: () {
+ log('Error: Upload timeout');
+ throw TimeoutException('Upload timed out');
+ },
+ );
var response = await http.Response.fromStream(streamedResponse);
if (response.statusCode == 200) {
- debugPrint('submitAppServer Response body: ${jsonDecode(response.body)}');
+ log('App submitted successfully');
return true;
} else {
- debugPrint('Failed to submit app. Status code: ${response.statusCode}');
+ log('Error: Failed to submit app. Status: ${response.statusCode}, Body: ${response.body}');
return false;
}
} catch (e) {
- debugPrint('An error occurred submitAppServer: $e');
+ log('Error submitting app: $e');
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<bool> submitAppServer(File file, Map<String, dynamic> appData) async { | |
| var request = http.MultipartRequest( | |
| 'POST', | |
| Uri.parse('${Env.apiBaseUrl}v3/plugins'), | |
| ); | |
| request.files.add(await http.MultipartFile.fromPath('file', file.path, filename: basename(file.path))); | |
| request.headers.addAll({'Authorization': await getAuthHeader()}); | |
| request.fields.addAll({'plugin_data': jsonEncode(appData)}); | |
| print(jsonEncode(appData)); | |
| try { | |
| var streamedResponse = await request.send(); | |
| var response = await http.Response.fromStream(streamedResponse); | |
| if (response.statusCode == 200) { | |
| debugPrint('submitAppServer Response body: ${jsonDecode(response.body)}'); | |
| return true; | |
| } else { | |
| debugPrint('Failed to submit app. Status code: ${response.statusCode}'); | |
| return false; | |
| } | |
| } catch (e) { | |
| debugPrint('An error occurred submitAppServer: $e'); | |
| return false; | |
| } | |
| } | |
| Future<bool> submitAppServer(File file, Map<String, dynamic> appData) async { | |
| const maxSizeBytes = 10 * 1024 * 1024; // 10MB | |
| if (await file.length() > maxSizeBytes) { | |
| log('Error: File size exceeds maximum allowed size'); | |
| return false; | |
| } | |
| final allowedTypes = ['.zip', '.tar.gz']; | |
| if (!allowedTypes.contains(extension(file.path).toLowerCase())) { | |
| log('Error: Invalid file type'); | |
| return false; | |
| } | |
| var request = http.MultipartRequest( | |
| 'POST', | |
| Uri.parse('${Env.apiBaseUrl}v3/plugins'), | |
| ); | |
| request.files.add(await http.MultipartFile.fromPath('file', file.path, filename: basename(file.path))); | |
| request.headers.addAll({ | |
| 'Authorization': await getAuthHeader(), | |
| 'Content-Type': 'multipart/form-data', | |
| 'Accept': 'application/json', | |
| }); | |
| request.fields.addAll({'plugin_data': jsonEncode(appData)}); | |
| log('Submitting app data: ${jsonEncode(appData)}'); | |
| try { | |
| var streamedResponse = await request.send().timeout( | |
| const Duration(minutes: 5), | |
| onTimeout: () { | |
| log('Error: Upload timeout'); | |
| throw TimeoutException('Upload timed out'); | |
| }, | |
| ); | |
| var response = await http.Response.fromStream(streamedResponse); | |
| if (response.statusCode == 200) { | |
| log('App submitted successfully'); | |
| return true; | |
| } else { | |
| log('Error: Failed to submit app. Status: ${response.statusCode}, Body: ${response.body}'); | |
| return false; | |
| } | |
| } catch (e) { | |
| log('Error submitting app: $e'); | |
| return false; | |
| } | |
| } |
| plugin = get_plugin_by_id_db(plugin_id, uid) | ||
| plugin = Plugin(**plugin) |
There was a problem hiding this comment.
Fix potential runtime error in plugin existence check
The plugin instance is created before checking if the plugin exists, which could cause a runtime error if plugin is None.
Apply this diff to both enable and disable endpoints:
- plugin = get_plugin_by_id_db(plugin_id, uid)
- plugin = Plugin(**plugin)
+ plugin_data = get_plugin_by_id_db(plugin_id, uid)
if not plugin:
raise HTTPException(status_code=404, detail='Plugin not found')
+ plugin = Plugin(**plugin_data)Also applies to: 43-44
| os.makedirs(f'_temp/plugins', exist_ok=True) | ||
| file_path = f"_temp/plugins/{file.filename}" | ||
| with open(file_path, 'wb') as f: | ||
| f.write(file.file.read()) | ||
| imgUrl = upload_plugin_logo(file_path, data['id']) | ||
| data['image'] = imgUrl |
There was a problem hiding this comment.
Add cleanup for temporary files
The endpoint creates temporary files but doesn't clean them up after upload, which could lead to disk space issues.
Apply this diff:
with open(file_path, 'wb') as f:
f.write(file.file.read())
imgUrl = upload_plugin_logo(file_path, data['id'])
+ try:
+ os.remove(file_path)
+ except OSError:
+ pass # Best effort cleanup📝 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.
| os.makedirs(f'_temp/plugins', exist_ok=True) | |
| file_path = f"_temp/plugins/{file.filename}" | |
| with open(file_path, 'wb') as f: | |
| f.write(file.file.read()) | |
| imgUrl = upload_plugin_logo(file_path, data['id']) | |
| data['image'] = imgUrl | |
| os.makedirs(f'_temp/plugins', exist_ok=True) | |
| file_path = f"_temp/plugins/{file.filename}" | |
| with open(file_path, 'wb') as f: | |
| f.write(file.file.read()) | |
| imgUrl = upload_plugin_logo(file_path, data['id']) | |
| try: | |
| os.remove(file_path) | |
| except OSError: | |
| pass # Best effort cleanup | |
| data['image'] = imgUrl |
🧰 Tools
🪛 Ruff
147-147: f-string without any placeholders
Remove extraneous f prefix
(F541)
| data['id'] = data['name'].replace(' ', '-').lower() | ||
| data['uid'] = uid | ||
| if data['private']: | ||
| data['id'] = data['id'] + '-private' | ||
| if private_plugin_id_exists_db(data['id'], uid): | ||
| data['id'] = data['id'] + '-' + ''.join([str(random.randint(0, 9)) for _ in range(5)]) | ||
| else: | ||
| if public_plugin_id_exists_db(data['id']): | ||
| data['id'] = data['id'] + '-' + ''.join([str(random.randint(0, 9)) for _ in range(5)]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve plugin ID generation security
The current ID generation method could lead to collisions and potential security issues.
Consider using a more robust ID generation method:
- data['id'] = data['name'].replace(' ', '-').lower()
+ import re
+ import uuid
+ # Sanitize and create unique ID
+ base_id = re.sub(r'[^a-z0-9-]', '-', data['name'].lower())
+ base_id = re.sub(r'-+', '-', base_id).strip('-')
+ data['id'] = f"{base_id}-{str(uuid.uuid4())[:8]}"
data['uid'] = uid
if data['private']:
data['id'] = data['id'] + '-private'
- if private_plugin_id_exists_db(data['id'], uid):
- data['id'] = data['id'] + '-' + ''.join([str(random.randint(0, 9)) for _ in range(5)])
- else:
- if public_plugin_id_exists_db(data['id']):
- data['id'] = data['id'] + '-' + ''.join([str(random.randint(0, 9)) for _ in range(5)])Committable suggestion skipped: line range outside the PR's diff.
| def add_plugin(plugin_data: str = Form(...), file: UploadFile = File(...), uid=Depends(auth.get_current_user_uid)): | ||
| data = json.loads(plugin_data) | ||
| data['approved'] = False |
There was a problem hiding this comment.
Add file validation
The endpoint accepts file uploads without validating the file type or size, which could pose security risks.
Add validation before processing the file:
def add_plugin(plugin_data: str = Form(...), file: UploadFile = File(...), uid=Depends(auth.get_current_user_uid)):
+ # Validate file size (e.g., limit to 1MB)
+ if len(await file.read()) > 1_048_576:
+ await file.seek(0) # Reset file pointer
+ raise HTTPException(status_code=400, detail='File too large')
+
+ # Validate file type
+ allowed_types = ['image/png', 'image/jpeg', 'image/jpg']
+ if file.content_type not in allowed_types:
+ raise HTTPException(status_code=400, detail='Invalid file type')
+
data = json.loads(plugin_data)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff
135-135: Do not perform function call File in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
135-135: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
|
All changes are done! Don't have to run the migration script as well. Covered some edge case as well. Final review please @beastoin |
|
lgtm @mdmohsin7 🥳 pull the main and leave it a final check. rolling out to production in next 2 hours. |
|
Alright will have a final check |
2 participants
Closes #1128
Note: Before deploying these changes, migrate the plugins again (after deleting the plugins_data collection and also the images in the bucket) to have fresh and new copy of data and images
add_plugin_demo.mp4
plugin_trigger_demo.mp4
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores