Closed
Conversation
Co-authored-by: qodo-free-for-open-source-projects[bot] <189517486+qodo-free-for-open-source-projects[bot]@users.noreply.github.com>
Co-authored-by: qodo-free-for-open-source-projects[bot] <189517486+qodo-free-for-open-source-projects[bot]@users.noreply.github.com>
Co-authored-by: qodo-free-for-open-source-projects[bot] <189517486+qodo-free-for-open-source-projects[bot]@users.noreply.github.com>
Co-authored-by: qodo-free-for-open-source-projects[bot] <189517486+qodo-free-for-open-source-projects[bot]@users.noreply.github.com>
Co-authored-by: qodo-free-for-open-source-projects[bot] <189517486+qodo-free-for-open-source-projects[bot]@users.noreply.github.com>
Co-authored-by: qodo-free-for-open-source-projects[bot] <189517486+qodo-free-for-open-source-projects[bot]@users.noreply.github.com>
Co-authored-by: qodo-free-for-open-source-projects[bot] <189517486+qodo-free-for-open-source-projects[bot]@users.noreply.github.com>
Co-authored-by: qodo-free-for-open-source-projects[bot] <189517486+qodo-free-for-open-source-projects[bot]@users.noreply.github.com>
Co-authored-by: qodo-free-for-open-source-projects[bot] <189517486+qodo-free-for-open-source-projects[bot]@users.noreply.github.com>
Co-authored-by: qodo-free-for-open-source-projects[bot] <189517486+qodo-free-for-open-source-projects[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* Fix icon caching * Fix * style: auto-fix linting and formatting issues * Quality * style: auto-fix linting and formatting issues * Optimize speed * style: auto-fix linting and formatting issues * Revert "style: auto-fix linting and formatting issues" This reverts commit ab9727a. * Revert "Optimize speed" This reverts commit ea53327. * Update nightly.yml * Update README.md * style: auto-fix linting and formatting issues * Unify Flutter icon cache * style: auto-fix linting and formatting issues * Fix icon cache * Migrate to built-in Flutter's design components * Update Flutter * style: auto-fix linting and formatting issues * Fix builds * Fixed icons * style: auto-fix linting and formatting issues * Fix build? * Change app ID to io.github.omeritzics.updatium * style: auto-fix linting and formatting issues * Update README.md * Update README.md * Update README.md * Attempt to fix Material You colors breaking design * Move filter icon to the top * style: auto-fix linting and formatting issues * Move the grid view button to the top * UI improvments * style: auto-fix linting and formatting issues * Update readme * Update readme * Update readme * Fix build * style: auto-fix linting and formatting issues * Fix overflow * style: auto-fix linting and formatting issues * Fix signing problem(?) * Fix build? * Fix * Release build fix * Update Nightly builds * Update Nightly builds * Revert "Release build fix" This reverts commit 257307d. * Revert "Update Nightly builds" This reverts commit 91b8f47. * Fix release build? * Update release.yml * Update release.yml * Update README.md * Update release.yml * UI fix * Update Hebrew * Bump SDK version * Update Hebrew * Fix Waydroid * style: auto-fix linting and formatting issues * style: auto-fix linting and formatting issues * Revert "Fix Waydroid" This reverts commit d879ba1. * Add nightly-signed.yml * Update nightly-signed.yml * Update workflows * Fix * Should fix signed builds * style: auto-fix linting and formatting issues * Update pubspec.lock * Replace flutter_keyboard_visibility * style: auto-fix linting and formatting issues * Replace shared_storage with docman * style: auto-fix linting and formatting issues * Fix typos * style: auto-fix linting and formatting issues * Another fix * style: auto-fix linting and formatting issues * Update dependencies * Fix * Delete .github/workflows/qama-unsigned.yml * Fix? * style: auto-fix linting and formatting issues * Fix errors? * Now it should fix the signing * Another fix * Another fix attempt * Fix nightly.yml * Try to add a different icon for the Nighly builds. #165 * Nightly new branding * Sign Nightly builds by default * Prepare for the new release * A message about unofficial sources * Fix the nightly build * Some bug fixes * style: auto-fix linting and formatting issues * Revert "Fix the nightly build" This reverts commit 9a1d9cd. * Revert "Fix nightly.yml" This reverts commit 0a68335. * Revert "Nightly new branding" This reverts commit 3e141c6. * Revert "Try to add a different icon for the Nighly builds. #165" This reverts commit 02dd713. * Some more fixes * Improve pure black theme * Attempt to fix grid view bug * Material You bug fix * style: auto-fix linting and formatting issues * Update badge style for GitHub release link * Bug fixes * style: auto-fix linting and formatting issues * Selection fix * UX/UI fixes * style: auto-fix linting and formatting issues * Update supported app sources in README * Fix build * style: auto-fix linting and formatting issues * Selection fix #2 * style: auto-fix linting and formatting issues * Fix spacing * Revert "Fix spacing" This reverts commit 1289296. * Add consistent spacing constants to app pages * style: auto-fix linting and formatting issues * Fix duplicate spacing constants compilation errors * Fix height16 scope issue in showChangeLogDialog * style: auto-fix linting and formatting issues * Add Fastlane supply metadata validation step Added a step to validate Fastlane supply metadata in the lint workflow. * Add Fastlane Supply Metadata validation job Added a new job to validate Fastlane Supply Metadata in the lint workflow. * Remove 'go' job from lint workflow Removed the 'go' job and its associated steps from the lint workflow. * Enhance nightly workflow with linting and formatting Add steps to auto-fix linting issues and format code in nightly workflow. * Enhance CI workflow with linting and formatting steps Added steps to auto-fix linting issues and format code in CI workflow. * Add auto-fix linting and formatting steps to workflow Added steps to auto-fix linting issues and format code before committing changes. * Delete .github/workflows/lint.yml * Delete .github/workflows/dependency-review.yml * Fix updateAppIcon method parameter reference error * Fix #168 * Fix icons in the app view page * style: auto-fix linting and formatting issues * style: auto-fix linting and formatting issues * Remove grid view from the code * Re-add grid view from Qama v26.1 * style: auto-fix linting and formatting issues * Fix typo * Fix formatting in localization section of README * Fix grid * Fix design inconsistency * style: auto-fix linting and formatting issues * Fix app.dart * Delete renovate.json * style: auto-fix linting and formatting issues * fix export error * style: auto-fix linting and formatting issues * Quick bug fix * style: auto-fix linting and formatting issues * Add renovate.json * chore(deps): update google/osv-scanner-action action to v2.3.3 * chore(deps): update gradle to v9.3.1 * chore(deps): update plugin org.jetbrains.kotlin.android to v2.3.10 * chore(deps): update actions/github-script action to v8 * chore(deps): update actions/upload-artifact action to v7 * chore(deps): update stefanzweifel/git-auto-commit-action action to v7 * Another bugfix * Update lib/providers/apps_provider.dart Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update BUTTON_MIGRATION_GUIDE.md Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update lib/examples/refactored_icon_pipeline_example.dart Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update lib/examples/refactored_icon_pipeline_example.dart Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update assets/translations/README.md Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * chore(deps): update actions/checkout action to v6 * chore(deps): update actions/github-script action to v8 * style: auto-fix linting and formatting issues * Fix workflows * chore(deps): update dependency node to v24 * chore(deps): update actions/setup-node action to v6 * Update translations.yml * chore(deps): update dependency node to v24 * Migrate some design components to the official Flutter ones * 🌐 Add missing translation keys for export functionality * Fix workflow * chore(deps): update actions/checkout action to v6 * style: auto-fix linting and formatting issues * add about (#196) * M3 Expressive update (#194) * M3 Expressive update * Update lib/main.dart Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * commit --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * chore(deps): update stefanzweifel/git-auto-commit-action action to v7 (#198) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update peter-evans/create-pull-request action to v8 (#197) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a YARA-based malware scanner and security settings provider, integrates pre-install APK scanning and quarantine, exposes UI controls and About dialog, updates theme styles, expands EN/DE/FR translations, and adjusts dependencies and removed platform-specific install/export flows. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AppsProvider
participant SecuritySettingsProvider
participant YARAScanner
participant FileSystem
User->>AppsProvider: installApk(apkPath, additionalAPKs?)
AppsProvider->>AppsProvider: _scanAPKForMalware(apkPath,...)
AppsProvider->>SecuritySettingsProvider: scanAPK(apkPath)
SecuritySettingsProvider->>YARAScanner: scanFile(apkPath)
YARAScanner->>FileSystem: read file bytes
FileSystem-->>YARAScanner: file bytes
YARAScanner->>YARAScanner: evaluate rules -> YARAScanResult
YARAScanner-->>SecuritySettingsProvider: YARAScanResult
alt infected and quarantine enabled
SecuritySettingsProvider->>FileSystem: move file -> quarantine, write report
end
SecuritySettingsProvider-->>AppsProvider: scan result
alt infected or error
AppsProvider-->>User: block install / throw error
else
AppsProvider->>AppsProvider: proceed with installation
AppsProvider-->>User: installation success
end
sequenceDiagram
actor User
participant SettingsUI
participant SecuritySettingsProvider
participant YARAScanner
participant RemoteSource
User->>SettingsUI: tap "Update Now"
SettingsUI->>SecuritySettingsProvider: updateRules()
SecuritySettingsProvider->>YARAScanner: updateRules()
YARAScanner->>RemoteSource: fetch rule sources
RemoteSource-->>YARAScanner: rule files/responses
YARAScanner->>YARAScanner: persist files, reload rules
alt success
YARAScanner-->>SecuritySettingsProvider: success
SecuritySettingsProvider->>SettingsUI: setLastUpdate/setRulesVersion
SettingsUI-->>User: success SnackBar
else failure
YARAScanner-->>SecuritySettingsProvider: YARARuleUpdateError
SecuritySettingsProvider-->>SettingsUI: error
SettingsUI-->>User: error SnackBar
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust YARA-based malware scanning capability to enhance application security, alongside a comprehensive update to the user interface following Material Design 3 principles. It also includes significant updates to the Android build environment and a new, streamlined internationalization workflow, ensuring the application remains modern, secure, and maintainable. Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚫 CI Build Failed
The automated build process failed. Please review the build logs and fix the issues before requesting another review.
Next steps:
- Check the build logs for specific errors
- Fix the identified issues
- Push your fixes to this branch
- The CI will automatically re-run
Once the build passes, this review will be dismissed automatically.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new YARA-based malware scanning feature, including auto-scan, auto-update of definitions, threat level filtering, and quarantine settings, along with corresponding UI elements and localization keys. It also updates the app's build configuration, increasing minSdk to 26, compileSdk and targetSdk to 36, and Java/Kotlin toolchains to version 21, while also updating Gradle and Flutter subproject commits. The package name has been changed from com.omeritzics.updatium to io.github.omeritzics.updatium across Android manifests and build files. UI/UX improvements include migrating from custom TextButton helpers to new AppTextButton widgets, replacing DropdownButtonFormField with MenuAnchor for better Material Design 3 compliance, and removing the webview_flutter dependency, which simplifies the app detail page. Translation management scripts were refactored to support new features like unused key detection and auto-translation. Review comments highlight potential issues such as Color.withValues not existing, unsanitized appId in icon caching, potential user tracking/network scanning via remote icon URLs, OutOfMemoryError risk from reading large APKs into memory for scanning, hardcoded SDK versions, incomplete Hebrew pluralizations, removal of grid view for categorized apps, and a significant increase in the minimum SDK for silent installs and system font access.
I am having trouble creating individual review comments. Click here to see my feedback.
lib/main.dart (575)
The method withValues does not exist on the Color class. This will cause a build error. To set the opacity, you should use withOpacity(0.2).
: Colors.black.withOpacity(0.2),
lib/services/unified_icon_service.dart (339)
The appId is used to construct a cache file path without sanitization in the new saveIcon method. If a malicious appId containing path traversal characters (e.g., ../../) is provided, it could allow writing files outside the intended cache directory, potentially overwriting other app data.
Remediation: Sanitize the appId to ensure it only contains allowed characters or use path.basename() to extract only the filename component before constructing the path.
lib/providers/apps_provider.dart (1879)
The updateAppIcon method now directly fetches app icons from a remote URL provided in the app's metadata (app.remoteIconUrl) using http.get. Since app metadata can be imported from untrusted sources (e.g., via the Import feature), an attacker can specify an arbitrary URL. This can be used to track users (via IP disclosure) or potentially perform internal network scanning if the device is connected to a private network.
Remediation: Validate that the remoteIconUrl uses a safe protocol (HTTPS) and consider restricting it to trusted domains or using a dedicated image loading library that handles security concerns.
lib/security/yara_scanner.dart (297)
The scanFile method reads the entire content of the file being scanned into memory using file.readAsBytes(). Since this is used to scan APK files, which can be very large (hundreds of megabytes), this can easily lead to an OutOfMemoryError and cause the application to crash, resulting in a denial of service. This is particularly relevant as the scan is now performed automatically before every installation.
Remediation: Use a streaming approach to scan files or read them in smaller chunks to avoid loading the entire file into memory.
android/app/build.gradle.kts (32)
Hardcoding the compileSdk to 36 decouples it from the version specified by the Flutter framework. It's recommended to use flutter.compileSdkVersion to ensure compatibility and easier maintenance when updating Flutter.
compileSdk = flutter.compileSdkVersionandroid/app/build.gradle.kts (50)
Similar to compileSdk, it's better to use flutter.targetSdkVersion instead of hardcoding the targetSdk to 36. This ensures that the target SDK version is aligned with the Flutter framework's recommendations.
targetSdk = flutter.targetSdkVersionassets/translations/he.json (422-424)
The pluralization for xAndNMoreUpdatesPossiblyInstalled is incomplete. The two and other cases are still in English. Please provide the Hebrew translations for these cases to ensure consistent localization.
lib/pages/apps.dart (993)
The grid view option for apps within a category has been removed. Previously, if grid view was enabled globally, categorized apps would also display in a grid. Now, they always render as a list. If this simplification was intentional, it's fine. Otherwise, the logic to support a grid layout here should be restored to maintain consistent view behavior throughout the app.
lib/providers/apps_provider.dart (858)
The minimum SDK version for silent installs has been raised from 31 to 36. This is a significant change that will disable silent background updates for users on Android 12, 13, and 14. Please verify if this is the intended behavior and if this restriction is accurate for these Android versions, as it could be a major regression in functionality for many users.
lib/pages/settings.dart (1031)
The condition for showing the "Use system font" option has been changed from sdkInt >= 34 to sdkInt >= 36. This might be an unnecessary restriction, as system font access is not typically limited to such a high API level. Please confirm if this change was intentional, as it might hide a useful feature from users on Android 14 and 15.
There was a problem hiding this comment.
Code Review
This pull request introduces a new YARA-powered malware scanning feature, adding security settings and functionality to automatically scan downloaded APKs before installation. The changes include new translation keys across German, English, and French for the security features, and a new SecuritySettingsProvider to manage YARA scanner configurations like auto-scan, auto-update, update intervals, and quarantine settings. A YARAScanner class was added to handle rule loading, remote updates from GitHub, and basic string/byte pattern matching for malware detection, along with file hashing capabilities. The AppsProvider was updated to integrate this scanning into the APK installation process, blocking installations if malware is detected. The UI in SettingsPage was extended to include controls for these new security features, and a new 'About' dialog was added. The main.dart file received styling updates for dropdown menus. Review comments highlight several issues: duplicate dropdownMenuTheme definitions in main.dart, an uninitialized _securityProvider in settings.dart leading to runtime errors and disabled UI, a missing path_provider import in security_settings_provider.dart, duplicate dispose methods and global dispose methods in yara_scanner.dart, and conflicting path and crypto dependency versions in pubspec.yaml. Additionally, there are suggestions to check mounted before setState in settings.dart and to rethrow exceptions in _scanAPKForMalware and updateRules for better error handling, along with a clarification needed for the dispose method comment in security_settings_provider.dart.
Comment on lines
+660
to
699
| dropdownMenuTheme: DropdownMenuThemeData( | ||
| inputDecorationTheme: InputDecorationTheme( | ||
| filled: true, | ||
| fillColor: scheme.surfaceContainerLow, | ||
| border: OutlineInputBorder( | ||
| borderRadius: BorderRadius.circular(20), | ||
| borderSide: BorderSide( | ||
| color: scheme.outline.withValues(alpha: 0.2), | ||
| width: 1, | ||
| ), | ||
| ), | ||
| enabledBorder: OutlineInputBorder( | ||
| borderRadius: BorderRadius.circular(20), | ||
| borderSide: BorderSide( | ||
| color: scheme.outline.withValues(alpha: 0.2), | ||
| width: 1, | ||
| ), | ||
| ), | ||
| focusedBorder: OutlineInputBorder( | ||
| borderRadius: BorderRadius.circular(20), | ||
| borderSide: BorderSide(color: scheme.primary, width: 2), | ||
| ), | ||
| contentPadding: const EdgeInsets.symmetric( | ||
| horizontal: 24, | ||
| vertical: 18, | ||
| ), | ||
| hintStyle: TextStyle( | ||
| color: scheme.onSurfaceVariant.withValues(alpha: 0.7), | ||
| fontWeight: FontWeight.w400, | ||
| ), | ||
| labelStyle: TextStyle( | ||
| color: scheme.onSurfaceVariant, | ||
| fontWeight: FontWeight.w500, | ||
| ), | ||
| floatingLabelStyle: TextStyle( | ||
| color: scheme.primary, | ||
| fontWeight: FontWeight.w600, | ||
| ), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
The dropdownMenuTheme property is being set twice on the ThemeData object. The second definition on line 702 will overwrite this one. This is a syntax error in Dart and will cause unexpected behavior. Please merge the two definitions or remove the redundant one.
Comment on lines
+30
to
+31
| late SecuritySettingsProvider _securityProvider; | ||
| bool _securityProviderInitialized = false; |
There was a problem hiding this comment.
The _securityProvider is declared as late but it's never initialized. This will cause a LateInitializationError at runtime when it's first accessed. You should initialize it in the initState method of the _SettingsPageState. Also, _securityProviderInitialized will always be false, causing the security settings UI to be disabled.
Here's a suggestion for how to initialize it:
@override
void initState() {
super.initState();
_initializeSecurityProvider();
}
Future<void> _initializeSecurityProvider() async {
try {
_securityProvider = await SecuritySettingsProvider.create();
await _securityProvider.initialize();
if (mounted) {
setState(() {
_securityProviderInitialized = true;
});
}
} catch (e) {
// Handle initialization error, e.g., show a message to the user
if (mounted) {
showError(UpdatiumError('Failed to initialize security module: $e'), context);
}
}
}|
|
||
| static Future<SecuritySettingsProvider> create() async { | ||
| final prefs = await SharedPreferences.getInstance(); | ||
| final appDir = await getApplicationSupportDirectory(); |
There was a problem hiding this comment.
The function getApplicationSupportDirectory() is used here, but it's not defined because the path_provider package is not imported. This will cause a compilation error. Please add import 'package:path_provider/path_provider.dart'; at the top of the file.
Comment on lines
+166
to
+179
| /// Dispose singleton instance | ||
| static void disposeInstance() { | ||
| _instance?.dispose(); | ||
| _instance = null; | ||
| } | ||
|
|
||
| /// Private constructor for singleton | ||
| YARAScanner._(this.config); | ||
|
|
||
| /// Dispose resources and cancel timers | ||
| void dispose() { | ||
| _updateTimer?.cancel(); | ||
| _updateTimer = null; | ||
| } |
There was a problem hiding this comment.
This class appears to contain duplicate method definitions. For example, dispose() is defined on line 176 and again on line 431. Similarly, disposeInstance() on line 167 and disposeGlobal() on line 436 seem to be duplicates. This will cause a compilation error. Please remove the duplicate definitions.
| html: ^0.15.6 | ||
| shared_preferences: ^2.5.4 | ||
| url_launcher: ^6.3.2 | ||
| path: ^1.9.0 |
There was a problem hiding this comment.
The path dependency is declared here, but it's also declared on line 99 as path: any. Please remove one of the declarations to avoid confusion and potential version conflicts. It's generally better to use a version constraint rather than any.
pubspec.yaml
Outdated
| path: ^1.9.0 | ||
| permission_handler: ^12.0.1 | ||
| fluttertoast: ^9.0.0 | ||
| crypto: ^3.0.6 |
There was a problem hiding this comment.
The crypto dependency is declared here as ^3.0.6, but it's also declared on line 71 as ^3.0.7. Please remove one of the declarations to avoid conflicts. It's recommended to keep the one with the higher version constraint.
Comment on lines
+983
to
+986
| } catch (e) { | ||
| logs.add('Security scan failed: $e'); | ||
| return false; // Block installation on scan failure | ||
| } finally { |
There was a problem hiding this comment.
This catch block swallows all exceptions from the security scan and returns false. This causes the caller in installApk to incorrectly report that malware was detected, even if the scan failed for another reason (like a network error or file issue). It would be better to rethrow the exception so the caller can provide a more accurate error message to the user.
} catch (e) {
logs.add('Security scan failed: $e');
rethrow; // Re-throw to allow the caller to handle the error appropriately.
}
Comment on lines
+153
to
+157
| void dispose() { | ||
| // Cancel auto-update timer to prevent memory leaks | ||
| // Note: We don't dispose the singleton here as it might be used by other instances | ||
| // Instead, callers should call YARAScanner.disposeInstance() when the app is shutting down | ||
| } |
There was a problem hiding this comment.
The comment here says 'Cancel auto-update timer to prevent memory leaks', but the method body is empty. Since YARAScanner is a singleton, this dispose method (which is called after every scan) should not dispose the scanner or its timer. The empty implementation is correct, but the comment is misleading and should be removed or updated to reflect that this provider does not own the scanner's lifecycle.
void dispose() {
// This provider does not own the YARAScanner singleton, so it does not dispose it here.
// The scanner's lifecycle is managed globally.
}Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/translations/de.json`:
- Around line 31-43: Add the missing German translation keys used by the
settings flow to assets/translations/de.json: specifically add translations for
"updateNow", "rulesUpdatedSuccessfully", "rulesUpdateFailed" and the
security-related keys introduced in en.json such as "threat", "quarantine",
"database" (and any sibling keys under the same feature group). Use the existing
phrasing style from nearby keys like "enableAutoScanDescription" and
"enableAutoUpdateDescription" to create concise German strings, ensuring keys
match exactly ("updateNow", "rulesUpdatedSuccessfully", "rulesUpdateFailed",
"threat", "quarantine", "database") so the feature no longer falls back to
English.
In `@assets/translations/fr.json`:
- Around line 31-43: The French translation file only includes initial YARA
labels and is missing the new security UI keys; add French translations for the
missing keys used by the new flow (e.g., updateNow, rulesUpdatedSuccessfully,
rulesUpdateFailed, threat, quarantine, database and any related labels such as
threatDetected, quarantineAction, availableDefinitions, lastUpdated) so the
feature doesn't fall back to English; locate the keys near yaraMalwareScanner /
enableAutoUpdate in assets/translations/fr.json and add concise French strings
matching the intent of the corresponding en.json entries.
- Around line 46-47: The JSON contains a duplicate key
"trackOnlyAppDescription"; remove the redundant duplicate entry (the second
occurrence) so only a single "trackOnlyAppDescription" key/value pair remains,
ensuring the translations JSON is valid and tooling won't reject it.
In `@lib/main.dart`:
- Around line 659-698: The ThemeData instantiation contains duplicate named
arguments for dropdownMenuTheme causing a compile error; locate both
dropdownMenuTheme blocks in the ThemeData and merge their settings into one
unified DropdownMenuThemeData instance (preserve InputDecorationTheme, borders,
contentPadding, hintStyle, labelStyle, floatingLabelStyle and any other
properties) so only a single dropdownMenuTheme is passed to ThemeData; ensure
you remove the duplicate block and keep all unique property assignments from
both occurrences.
In `@lib/pages/settings.dart`:
- Around line 101-106: The About dialog currently hardcodes applicationVersion
in _showAboutDialog causing drift; change _showAboutDialog to async, import
package_info_plus (PackageInfo), call PackageInfo.fromPlatform() and set
applicationVersion to '${packageInfo.version}+${packageInfo.buildNumber}' (or
fallback to packageInfo.version if buildNumber absent), then pass that string to
showAboutDialog; ensure to handle exceptions (fallback to 'unknown' or existing
hardcoded value) and update any callers if you change the signature.
- Around line 30-31: The SecuritySettingsProvider (_securityProvider) and its
flag (_securityProviderInitialized) are never set, so enable the security
controls by instantiating the provider and marking it initialized in the widget
lifecycle (implement initState in this SettingsState/SettingsPage class), e.g.,
create a SecuritySettingsProvider instance, assign it to _securityProvider, set
_securityProviderInitialized = true, and call any async init/load methods (await
or handle futures) before enabling UI; also ensure you clean up in dispose if
needed. Locate references to _securityProvider and _securityProviderInitialized
(used by the YARA toggles, update interval dropdown, and manual update button)
to confirm they now read the initialized provider rather than remaining
disabled.
In `@lib/providers/apps_provider.dart`:
- Line 1030: The thrown UpdatiumError message is hard-coded in English; change
it to use the app localization helper like the surrounding install errors (wrap
the message with tr(...)) so the blocked-install error is localized; update the
throw in apps_provider.dart where UpdatiumError is raised for "Security scan
detected malware. Installation blocked for safety." to call tr("...") with the
same phrasing/key pattern used by nearby install error messages.
- Around line 957-973: The primary APK scan currently only checks
scanResult.isInfected but not scanResult.error, causing inconsistent behavior
versus additionalScanResult handling; update the code around scanAPK and
scanResult to also detect scanResult.error (same as additionalScanResult.error),
log a descriptive message via logs.add including the error, and return false
when an error is present so both primary and additional APK scans are treated
consistently; reference the scanAPK call, the scanResult variable, and the
isInfected and error fields when making this change.
- Around line 1028-1031: The malware scan block in installApk() calls
_scanAPKForMalware(...) and throws without cleaning up downloaded artifacts,
leaving files in APKDir (and original archives for XAPK/ZIP since
installApkDir() only deletes on success); before throwing, delete the rejected
APK path (file.file.path), any additionalAPKs paths, and if this install path
originated from an extracted archive ensure that the original archive is removed
(or call the same cleanup helper used by installApkDir()); update installApk(),
and if needed installApkDir(), to perform this cleanup when _scanAPKForMalware
returns false (or wrap the scan in a try/finally that removes rejected
artifacts).
In `@lib/security/security_settings_provider.dart`:
- Around line 24-30: The constructor creates a YARAScanner singleton from prefs
but the setters only persist to SharedPreferences, so toggling controls doesn't
update the running scanner; in each setter (e.g., setAutoUpdateEnabled(),
setUpdateInterval() and the other setters around lines 52–57) after saving prefs
update the in-memory _config (modify updateInterval and enableAutoUpdate) and
reapply it to the running scanner by either calling a scanner update method if
available (e.g., _scanner.updateConfig(_config) or
_scanner.setUpdateInterval(...)/setAutoUpdate(...)) or by reinitializing
_scanner with the new config via YARAScanner.getInstance(_config) so the scanner
honors the new settings immediately.
- Around line 85-98: The scanAPK flow must not treat scan failures as "clean":
after calling _scanner.scanFile(apkPath) check the returned YARAScanResult for
an error marker (e.g. result.error != null or whatever field indicates a
failure) and throw an exception for real scan failures so callers (like
apps_provider) don't treat them as safe; when auto-scan is disabled
(getAutoScanEnabled() == false) return a non-error "skipped" or explicit clean
result (not YARAScanResult.error) so it is distinguishable from a failure; keep
the quarantine logic using result.isInfected and _quarantineFile as-is.
- Around line 1-5: The file calls getApplicationSupportDirectory() from create()
and _getRulesDirectory() but lacks the import for
package:path_provider/path_provider.dart; add the missing import
(package:path_provider/path_provider.dart) to the top of the file so
getApplicationSupportDirectory() resolves correctly and ensure the import
appears before or alongside the other imports to satisfy the analyzer for
functions create() and _getRulesDirectory().
In `@lib/security/yara_scanner.dart`:
- Around line 430-438: There is a duplicate instance method dispose() in the
class; remove the second/shorter dispose() implementation that only calls
_updateTimer?.cancel() and keep the original, complete dispose() (the one that
also sets _updateTimer = null); ensure disposeGlobal() continues to call
_instance?.dispose() and that _instance is set to null after calling dispose.
- Around line 156-162: The singleton getter YARAScanner.getInstance attempts to
assign to a non-existent field `_config`; instead update the existing instance's
public `config` field (or rename the class field consistently) — in other words,
change the else branch in getInstance to set `_instance!.config = config` (or
rename the declaration at class-level to `_config` and adjust all usages) so the
assignment matches the actual field name used in the class.
- Around line 194-210: The rule loader and parser are incorrectly treating
entire .yar files as single YARARule objects and using naive byte-pattern
matching (see _loadRules and YARARule.fromString), and the hex parsing logic is
broken because it relies on utf8.encode throwing exceptions instead of
explicitly detecting and parsing {hex} tokens; update YARARule.fromString to (1)
parse the .yar file into proper rule sections (meta, strings, condition), (2)
parse string definitions to support quoted strings, hex blocks like {6A 40 68}
and modifiers (wide, ascii, nocase) into a normalized byte/sequence
representation, and (3) implement condition parsing to evaluate boolean
expressions (all of, any of, counts, offsets, negation) against the parsed
strings instead of naive byte matching; replace the utf8-based fallback with a
deterministic hex-token parser that recognizes braces and hex bytes and converts
them into bytes, and ensure _loadRules adds fully parsed rules that expose both
strings and a compiled condition evaluator for correct matching.
In `@pubspec.yaml`:
- Around line 48-51: The pubspec has duplicate dependency keys for path and
crypto; remove the redundant entries so each package appears only once (keep the
intended version constraint for path and for crypto), ensuring the remaining
declarations are the correct versions and properly indented in the dependencies
map; after editing, run pub get to verify no duplicate-key errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b323c44-b3cd-45d7-9dc3-532effee819d
📒 Files selected for processing (9)
assets/translations/de.jsonassets/translations/en.jsonassets/translations/fr.jsonlib/main.dartlib/pages/settings.dartlib/providers/apps_provider.dartlib/security/security_settings_provider.dartlib/security/yara_scanner.dartpubspec.yaml
Comment on lines
+31
to
+43
| "yaraMalwareScanner": "Automatische Malware-Erkennung (Powered by YARA)", | ||
| "yaraScannerDescription": "Branchenstandardmäßige Malware-Erkennung mit YARA-kompatiblen Regeln für umfassenden Schutz vor Viren, Trojanern und anderen Bedrohungen. Kein Scanner kann 100% Erkennung garantieren, bietet aber starken Schutz vor bekannter Malware. Die Verantwortung für die sichere App-Nutzung liegt letztlich beim Benutzer.", | ||
| "securitySettings": "Sicherheitseinstellungen", | ||
| "xIsTrackOnly": "{} ist nur zur Nachverfolgung", | ||
| "source": "Quelle", | ||
| "app": "App", | ||
| "enableAutoScan": "Automatische Scan aktivieren", | ||
| "enableAutoScanDescription": "Heruntergeladene APKs automatisch auf Malware scannen, vor der Installation", | ||
| "enableAutoUpdate": "Automatische Updates aktivieren", | ||
| "enableAutoUpdateDescription": "Malware-Definitions-Datenbank automatisch aktualisieren", | ||
| "updateInterval": "Update-Intervall", | ||
| "updateIntervalDescription": "Wie oft nach neuen Malware-Definitionen gesucht werden soll", | ||
| "hours": "Stunden", |
There was a problem hiding this comment.
Add the remaining security strings here too.
This locale only covers the first auto-scan/auto-update labels. The settings flow also references updateNow, rulesUpdatedSuccessfully, rulesUpdateFailed, and the threat/quarantine/database keys added in en.json, so German will still fall back for part of the new feature and the translation consistency job stays red.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/translations/de.json` around lines 31 - 43, Add the missing German
translation keys used by the settings flow to assets/translations/de.json:
specifically add translations for "updateNow", "rulesUpdatedSuccessfully",
"rulesUpdateFailed" and the security-related keys introduced in en.json such as
"threat", "quarantine", "database" (and any sibling keys under the same feature
group). Use the existing phrasing style from nearby keys like
"enableAutoScanDescription" and "enableAutoUpdateDescription" to create concise
German strings, ensuring keys match exactly ("updateNow",
"rulesUpdatedSuccessfully", "rulesUpdateFailed", "threat", "quarantine",
"database") so the feature no longer falls back to English.
Comment on lines
+194
to
+210
| Future<void> _loadRules() async { | ||
| try { | ||
| final rulesDir = Directory(config.rulesDirectory); | ||
| if (!await rulesDir.exists()) { | ||
| await rulesDir.create(recursive: true); | ||
| } | ||
|
|
||
| _rules.clear(); | ||
| int loadedCount = 0; | ||
| int errorCount = 0; | ||
|
|
||
| await for (final entity in rulesDir.list()) { | ||
| if (entity is File && entity.path.endsWith('.yar')) { | ||
| try { | ||
| final content = await entity.readAsString(); | ||
| final rule = YARARule.fromString(content); | ||
| _rules.add(rule); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
printf 'Relevant parsing/matching code:\n'
rg -n 'YARARule\.fromString|ruleName =|condition:|utf8\.encode|_containsBytes' lib/security/yara_scanner.dartRepository: omeritzics/Updatium
Length of output: 512
🏁 Script executed:
sed -n '115,170p' lib/security/yara_scanner.dartRepository: omeritzics/Updatium
Length of output: 1699
🏁 Script executed:
sed -n '320,370p' lib/security/yara_scanner.dartRepository: omeritzics/Updatium
Length of output: 2108
🏁 Script executed:
sed -n '280,320p' lib/security/yara_scanner.dartRepository: omeritzics/Updatium
Length of output: 1066
The YARA rule matcher has critical implementation flaws that compromise malware detection.
The code loads entire .yar files as single YARARule objects and applies only naive byte-pattern matching. The condition: field is explicitly commented as unused, so complex rules with multiple blocks, quoted strings, modifiers, or all of/any of conditions will not match correctly.
More critically, the hex string parsing at lines 350-363 is broken: utf8.encode() never throws exceptions, so the catch block that attempts hex parsing is unreachable. Hex patterns like {6A 40 68} will be encoded as UTF-8 text bytes instead of parsed as hexadecimal bytes, causing detection failures. This makes the install-time malware gate unreliable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/security/yara_scanner.dart` around lines 194 - 210, The rule loader and
parser are incorrectly treating entire .yar files as single YARARule objects and
using naive byte-pattern matching (see _loadRules and YARARule.fromString), and
the hex parsing logic is broken because it relies on utf8.encode throwing
exceptions instead of explicitly detecting and parsing {hex} tokens; update
YARARule.fromString to (1) parse the .yar file into proper rule sections (meta,
strings, condition), (2) parse string definitions to support quoted strings, hex
blocks like {6A 40 68} and modifiers (wide, ascii, nocase) into a normalized
byte/sequence representation, and (3) implement condition parsing to evaluate
boolean expressions (all of, any of, counts, offsets, negation) against the
parsed strings instead of naive byte matching; replace the utf8-based fallback
with a deterministic hex-token parser that recognizes braces and hex bytes and
converts them into bytes, and ensure _loadRules adds fully parsed rules that
expose both strings and a compiled condition evaluator for correct matching.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
lib/security/yara_scanner.dart (2)
430-433:⚠️ Potential issue | 🔴 CriticalRemove the duplicate
dispose()method.Dart does not allow duplicate instance method declarations. There are two
dispose()methods: one at line 176 (complete, sets_updateTimer = null) and this one at line 431. This will cause a compile-time error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/security/yara_scanner.dart` around lines 430 - 433, There are two declarations of the instance method dispose() causing a compile error; remove the duplicate disposal at the end of the file (the one that only calls _updateTimer?.cancel()) and keep the full dispose() implementation (the earlier method that cancels the timer and sets _updateTimer = null) so only a single dispose() exists and _updateTimer is properly nulled.
349-363:⚠️ Potential issue | 🔴 CriticalThe hex string parsing catch block is unreachable.
utf8.encode()never throws exceptions—it always returns bytes. The catch block intended to handle hex strings like{6A 40 68}is dead code. Hex patterns will be incorrectly encoded as UTF-8 text bytes instead of parsed as hexadecimal, causing detection failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/security/yara_scanner.dart` around lines 349 - 363, The catch block is unreachable because utf8.encode never throws, so detect hex-formatted patterns in searchString before calling utf8.encode and parse them into searchBytes; specifically, check searchString against a hex pattern (e.g., brace-enclosed or whitespace-separated hex like "{6A 40 68}" or "6A4068") and if it matches, remove braces/spaces and convert each hex pair to an int into searchBytes, otherwise set searchBytes = utf8.encode(searchString). Replace the existing try/catch surrounding utf8.encode with this conditional logic referencing searchString and searchBytes to ensure hex patterns are parsed correctly.lib/providers/apps_provider.dart (1)
1042-1042:⚠️ Potential issue | 🟡 MinorLocalize the new blocked-install error.
This
UpdatiumErroris user-facing, but it is hard-coded in English instead of usingtr(...)like the surrounding install errors. That will surface a mixed-language message in non-English locales.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/apps_provider.dart` at line 1042, The user-facing error thrown as UpdatiumError('Security scan detected malware. Installation blocked for safety.') is hard-coded English; update it to use the app localization helper (wrap the string in tr(...)) like the surrounding install errors so it appears localized across locales. Locate the throw statement for UpdatiumError in apps_provider.dart (the install/security-scan path) and replace the literal string with tr('Security scan detected malware. Installation blocked for safety.') (or the appropriate localized key), ensuring any required localization import is present and the message format matches other calls to tr(...) in this file.lib/security/security_settings_provider.dart (1)
52-58:⚠️ Potential issue | 🟠 MajorReapply scanner config when these settings change.
The singleton is created once from the current prefs in the constructor, but
setAutoUpdateEnabled()andsetUpdateInterval()only persist toSharedPreferences. The runningYARAScannerkeeps its old interval/auto-update timer, so toggling these controls does not take effect until the scanner is recreated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/security/security_settings_provider.dart` around lines 52 - 58, setAutoUpdateEnabled and setUpdateInterval only persist to SharedPreferences but do not update the running YARAScanner, so toggles don't take effect until scanner recreation; after successfully writing to _prefs in setAutoUpdateEnabled and setUpdateInterval, call into the YARAScanner singleton (e.g. YARAScanner.instance or YARAScanner.shared) to reapply the new settings (using existing methods like applyConfig/updateSettings or add a new public method such as updateSettings(bool autoUpdate, int interval) that reads getAutoUpdateEnabled/getUpdateInterval or accepts values directly), making sure to handle errors and avoid blocking the prefs write path.
🧹 Nitpick comments (1)
lib/security/security_settings_provider.dart (1)
42-46: Useconfig.rulesDirectoryinstead of recalculating the path.
_getRulesDirectory()callsgetApplicationSupportDirectory()again, but the constructor already receives and stores the rules directory in_config.rulesDirectory. This could lead to inconsistency if the paths ever differ, and adds unnecessary async overhead.♻️ Suggested fix
/// Get the directory for YARA rules - Future<String> _getRulesDirectory() async { - final appDir = await getApplicationSupportDirectory(); - final rulesDir = Directory('${appDir.path}/yara_rules'); - return rulesDir.path; + String _getRulesDirectory() { + return config.rulesDirectory; }Then update
_quarantineFileaccordingly:Future<void> _quarantineFile(String filePath, YARAScanResult result) async { try { - final rulesDir = await _getRulesDirectory(); + final rulesDir = _getRulesDirectory(); final quarantineDir = Directory('$rulesDir/quarantine');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/security/security_settings_provider.dart` around lines 42 - 46, Replace the async recomputation of the rules path in _getRulesDirectory() with a synchronous return of the already-provided config path: use _config.rulesDirectory instead of calling getApplicationSupportDirectory(); then update any callers such as _quarantineFile to use the returned value (or read _config.rulesDirectory directly) so you remove the redundant async call and ensure consistent rule directory usage across SecuritySettingsProvider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/security/yara_scanner.dart`:
- Around line 286-291: The periodic Timer callback in _startAutoUpdate calls
updateRules() without awaiting and doesn't handle exceptions, so make the
Timer.periodic callback async and wrap the await updateRules() call in a
try/catch; catch exceptions (e, stack) and log them via the module's logger (or
rethrow/handle appropriately) to avoid unhandled async errors from _updateTimer
and updateRules while still using config.updateInterval.
- Around line 232-257: The HTTP GET calls in the loop over config.ruleSources
lack a timeout and can hang; update the http.get(Uri.parse(source)) call to
include a .timeout(Duration(seconds: <appropriate_seconds>)) and import
dart:async if needed, then handle TimeoutException specifically in the try/catch
around that call (add a catch for TimeoutException or handle via onTimeout) so
you add the source to failedSources, append a clear timeout message to
errorDetails, and log a non-sensitive message to _logs (consistent with existing
basename/fileName usage) while preserving existing success/failure flow in the
loop that writes files (see the loop over config.ruleSources, http.get usage,
failedSources, errorDetails, _logs, and File.writeAsString).
---
Duplicate comments:
In `@lib/providers/apps_provider.dart`:
- Line 1042: The user-facing error thrown as UpdatiumError('Security scan
detected malware. Installation blocked for safety.') is hard-coded English;
update it to use the app localization helper (wrap the string in tr(...)) like
the surrounding install errors so it appears localized across locales. Locate
the throw statement for UpdatiumError in apps_provider.dart (the
install/security-scan path) and replace the literal string with tr('Security
scan detected malware. Installation blocked for safety.') (or the appropriate
localized key), ensuring any required localization import is present and the
message format matches other calls to tr(...) in this file.
In `@lib/security/security_settings_provider.dart`:
- Around line 52-58: setAutoUpdateEnabled and setUpdateInterval only persist to
SharedPreferences but do not update the running YARAScanner, so toggles don't
take effect until scanner recreation; after successfully writing to _prefs in
setAutoUpdateEnabled and setUpdateInterval, call into the YARAScanner singleton
(e.g. YARAScanner.instance or YARAScanner.shared) to reapply the new settings
(using existing methods like applyConfig/updateSettings or add a new public
method such as updateSettings(bool autoUpdate, int interval) that reads
getAutoUpdateEnabled/getUpdateInterval or accepts values directly), making sure
to handle errors and avoid blocking the prefs write path.
In `@lib/security/yara_scanner.dart`:
- Around line 430-433: There are two declarations of the instance method
dispose() causing a compile error; remove the duplicate disposal at the end of
the file (the one that only calls _updateTimer?.cancel()) and keep the full
dispose() implementation (the earlier method that cancels the timer and sets
_updateTimer = null) so only a single dispose() exists and _updateTimer is
properly nulled.
- Around line 349-363: The catch block is unreachable because utf8.encode never
throws, so detect hex-formatted patterns in searchString before calling
utf8.encode and parse them into searchBytes; specifically, check searchString
against a hex pattern (e.g., brace-enclosed or whitespace-separated hex like
"{6A 40 68}" or "6A4068") and if it matches, remove braces/spaces and convert
each hex pair to an int into searchBytes, otherwise set searchBytes =
utf8.encode(searchString). Replace the existing try/catch surrounding
utf8.encode with this conditional logic referencing searchString and searchBytes
to ensure hex patterns are parsed correctly.
---
Nitpick comments:
In `@lib/security/security_settings_provider.dart`:
- Around line 42-46: Replace the async recomputation of the rules path in
_getRulesDirectory() with a synchronous return of the already-provided config
path: use _config.rulesDirectory instead of calling
getApplicationSupportDirectory(); then update any callers such as
_quarantineFile to use the returned value (or read _config.rulesDirectory
directly) so you remove the redundant async call and ensure consistent rule
directory usage across SecuritySettingsProvider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45aca296-5462-4b80-a335-a0110b28e482
📒 Files selected for processing (3)
lib/providers/apps_provider.dartlib/security/security_settings_provider.dartlib/security/yara_scanner.dart
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
lib/providers/apps_provider.dart (1)
1032-1046:⚠️ Potential issue | 🟠 MajorRejected XAPK/ZIP archives still survive this failure path.
This cleanup deletes the extracted APKs, but when the block originates from
installApkDir()the originalDownloadedDir.filearchive is still only deleted on successful install. That leaves the rejected XAPK/ZIP on disk and lets later retries reuse the same artifact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/apps_provider.dart` around lines 1032 - 1046, The security-scan failure path in _scanAPKForMalware handling currently deletes extracted APK files but does not remove the original archive handled by installApkDir (the DownloadedDir.file), leaving rejected XAPK/ZIP artifacts on disk; update the cleanup in the failure branch to also check and delete the original archive (referencing file.file and the DownloadedDir/file artifact used by installApkDir) and any associated extracted-archive temp (e.g., additionalAPKs and their files) using the existing deleteFile helper so the archive cannot be reused on retry.lib/security/yara_scanner.dart (1)
116-144:⚠️ Potential issue | 🔴 CriticalThis still isn't evaluating real YARA rules correctly.
The scanner is still treating each
.yarfile like a single lightweight pattern blob: multiple rules per file, quoted strings/modifiers, andconditionexpressions are not evaluated, and the hex fallback is dead becauseutf8.encode()does not throw. That makes the install blocker miss real matches while appearing to enforce YARA.Also applies to: 195-211, 345-390
🧹 Nitpick comments (1)
assets/translations/de.json (1)
37-54: Fix the German wording in these new security labels.
Automatische Scan aktivierenandInfizierte Dateien quarantänenread as broken German in the Settings UI. Please tighten these before shipping.Suggested wording
- "enableAutoScan": "Automatische Scan aktivieren", + "enableAutoScan": "Automatischen Scan aktivieren", ... - "quarantineInfected": "Infizierte Dateien quarantänen", + "quarantineInfected": "Infizierte Dateien in Quarantäne verschieben",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/translations/de.json` around lines 37 - 54, The German labels are grammatically incorrect; update the translation values for the keys "enableAutoScan" to "Automatischen Scan aktivieren" and "quarantineInfected" to "Infizierte Dateien in Quarantäne verschieben" (or "Infizierte Dateien unter Quarantäne stellen") so the Settings UI shows correct German phrasing while leaving the other keys unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/translations/en.json`:
- Around line 31-61: The en.json file introduced new translation keys (e.g.,
"yaraMalwareScanner", "yaraScannerDescription", "securitySettings",
"enableAutoScan", "enableAutoScanDescription", "enableAutoUpdate",
"enableAutoUpdateDescription", "updateInterval", "updateIntervalDescription",
"hours", "threatLevelFilter", "threatLevelFilterDescription", "level1",
"lowThreat", "level2", "mediumThreat", "level3", "highThreat",
"quarantineSettings", "quarantineInfected", "quarantineInfectedDescription",
"viewQuarantine", "viewQuarantineDescription", "databaseInformation",
"lastUpdate", "rulesVersion", "updateNow", "updating",
"rulesUpdatedSuccessfully", "rulesUpdateFailed", "quarantineViewComingSoon")
that are missing from other locale files; add these same keys to every file
under assets/translations/*.json (including es.json and he.json) with
appropriate translated strings or placeholder English strings so each locale has
the full set, preserving key names and JSON structure to satisfy the
translation-management job and prevent fallback to English.
In `@lib/pages/settings.dart`:
- Around line 934-959: The SwitchListTile and DropdownButton onChanged handlers
call setState() after awaiting async operations and must guard with mounted to
avoid calling setState() after dispose; update the handlers for
_securityProvider.setAutoUpdateEnabled and _securityProvider.setUpdateInterval
to check if (mounted) before calling setState() (or return early if not mounted)
and ensure these checks reference the same StatefulWidget's mounted property so
the UI update only runs when the widget is still alive.
- Around line 164-170: The SecuritySettingsProvider instance returned by
SecuritySettingsProvider.create() isn't fully ready until its initialize()
method runs; update _initializeSecurityProvider to call await
_securityProvider.initialize() (or equivalent) after creating the provider and
before setting _securityProviderInitialized = true or calling setState — ensure
any errors from initialize() are handled so the UI only enables YARA controls
after initialize() completes successfully.
In `@lib/providers/apps_provider.dart`:
- Around line 950-993: The _scanAPKForMalware function currently conflates
scanner errors and true infections by returning false for both; change it to
return a richer result (e.g., an enum/class like ScanStatus {clean, infected,
error} or throw a dedicated ScanException) so callers can distinguish causes:
when SecuritySettingsProvider.scanAPK returns scanResult.error or when an
exception is caught, return/throw an error status (including the error details)
instead of treating it as an infection, keep infected detection using
scanResult.isInfected, and update installApk to handle the new
ScanStatus/exception (show a distinct "scan failed" message for errors and only
show "malware-blocked" for infected). Ensure you update all call sites
(including the other block around lines 1032-1046) to check the new
result/exception instead of assuming false == infected.
In `@lib/security/yara_scanner.dart`:
- Around line 183-223: _initialize() and _loadRules() leave the scanner with an
empty rule set on fresh installs; fix by adding a safe fallback load when no
on-disk rules are found: after iterating rulesDir in _loadRules(), if
loadedCount == 0 call a new helper (e.g., _loadBundledRules or
_loadDefaultRulesFromAssets) to load embedded/default YARA rules packaged with
the app (or alternatively set a safe "fail-closed" flag that makes scanFile()
treat unknown/empty rule-set as matched), and ensure _startAutoUpdate() still
runs; implement the helper to parse embedded rule strings into YARARule
instances and add to _rules, update _logs with counts, and reuse the same error
handling used in _loadRules(); also apply the same empty-dir fallback where
similar logic appears (lines ~320-339).
---
Duplicate comments:
In `@lib/providers/apps_provider.dart`:
- Around line 1032-1046: The security-scan failure path in _scanAPKForMalware
handling currently deletes extracted APK files but does not remove the original
archive handled by installApkDir (the DownloadedDir.file), leaving rejected
XAPK/ZIP artifacts on disk; update the cleanup in the failure branch to also
check and delete the original archive (referencing file.file and the
DownloadedDir/file artifact used by installApkDir) and any associated
extracted-archive temp (e.g., additionalAPKs and their files) using the existing
deleteFile helper so the archive cannot be reused on retry.
---
Nitpick comments:
In `@assets/translations/de.json`:
- Around line 37-54: The German labels are grammatically incorrect; update the
translation values for the keys "enableAutoScan" to "Automatischen Scan
aktivieren" and "quarantineInfected" to "Infizierte Dateien in Quarantäne
verschieben" (or "Infizierte Dateien unter Quarantäne stellen") so the Settings
UI shows correct German phrasing while leaving the other keys unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4869e002-5567-42f8-9e42-c78da8c21e1d
📒 Files selected for processing (9)
assets/translations/de.jsonassets/translations/en.jsonassets/translations/fr.jsonlib/main.dartlib/pages/settings.dartlib/providers/apps_provider.dartlib/security/security_settings_provider.dartlib/security/yara_scanner.dartpubspec.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/security/security_settings_provider.dart
- assets/translations/fr.json
- pubspec.yaml
Comment on lines
+31
to
+61
| "yaraMalwareScanner": "Auto Malware Scanning (Powered by YARA)", | ||
| "yaraScannerDescription": "Industry-standard malware detection using YARA rules for comprehensive protection against viruses, trojans, and other threats. While no scanner can guarantee 100% detection, this provides strong protection against known malware. The responsibility for safe app usage ultimately is on the user.", | ||
| "securitySettings": "Security Settings", | ||
| "enableAutoScan": "Enable Auto-Scan", | ||
| "enableAutoScanDescription": "Automatically scan downloaded APKs for malware before installation", | ||
| "enableAutoUpdate": "Enable Auto-Update", | ||
| "enableAutoUpdateDescription": "Automatically update malware definition database", | ||
| "updateInterval": "Update Interval", | ||
| "updateIntervalDescription": "How often to check for new malware definitions", | ||
| "hours": "hours", | ||
| "threatLevelFilter": "Threat Level Filter", | ||
| "threatLevelFilterDescription": "Minimum threat level to trigger alerts", | ||
| "level1": "Level 1", | ||
| "lowThreat": "Low Threat", | ||
| "level2": "Level 2", | ||
| "mediumThreat": "Medium Threat", | ||
| "level3": "Level 3", | ||
| "highThreat": "High Threat", | ||
| "quarantineSettings": "Quarantine Settings", | ||
| "quarantineInfected": "Quarantine Infected Files", | ||
| "quarantineInfectedDescription": "Move detected malware to quarantine for safety", | ||
| "viewQuarantine": "View Quarantine", | ||
| "viewQuarantineDescription": "Manage quarantined files", | ||
| "databaseInformation": "Database Information", | ||
| "lastUpdate": "Last Update", | ||
| "rulesVersion": "Rules Version", | ||
| "updateNow": "Update Now", | ||
| "updating": "Updating...", | ||
| "rulesUpdatedSuccessfully": "Malware definitions updated successfully", | ||
| "rulesUpdateFailed": "Failed to update malware definitions", | ||
| "quarantineViewComingSoon": "Quarantine view coming soon", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import glob, json, os
with open('assets/translations/en.json', encoding='utf-8') as f:
base_keys = set(json.load(f).keys())
for path in sorted(glob.glob('assets/translations/*.json')):
with open(path, encoding='utf-8') as f:
keys = set(json.load(f).keys())
missing = sorted(base_keys - keys)
if missing:
print(f'{os.path.basename(path)}: {len(missing)} missing')
for key in missing[:40]:
print(f' - {key}')
if len(missing) > 40:
print(' ...')
PYRepository: omeritzics/Updatium
Length of output: 22367
Backfill these new keys into all translation locale files.
The translation-management job is failing because these source keys were added to en.json (lines 31–61) but are not present in the remaining assets/translations/*.json locale files. Currently, 26 locale files are missing 49 keys each, and es.json and he.json are missing 31 keys each. Until these keys are added to all locales, the PR remains red and the new security UI will fall back to English per locale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/translations/en.json` around lines 31 - 61, The en.json file
introduced new translation keys (e.g., "yaraMalwareScanner",
"yaraScannerDescription", "securitySettings", "enableAutoScan",
"enableAutoScanDescription", "enableAutoUpdate", "enableAutoUpdateDescription",
"updateInterval", "updateIntervalDescription", "hours", "threatLevelFilter",
"threatLevelFilterDescription", "level1", "lowThreat", "level2", "mediumThreat",
"level3", "highThreat", "quarantineSettings", "quarantineInfected",
"quarantineInfectedDescription", "viewQuarantine", "viewQuarantineDescription",
"databaseInformation", "lastUpdate", "rulesVersion", "updateNow", "updating",
"rulesUpdatedSuccessfully", "rulesUpdateFailed", "quarantineViewComingSoon")
that are missing from other locale files; add these same keys to every file
under assets/translations/*.json (including es.json and he.json) with
appropriate translated strings or placeholder English strings so each locale has
the full set, preserving key names and JSON structure to satisfy the
translation-management job and prevent fallback to English.
Comment on lines
+934
to
+959
| SwitchListTile( | ||
| title: Text(tr('enableAutoUpdate')), | ||
| subtitle: Text(tr('enableAutoUpdateDescription')), | ||
| value: _securityProviderInitialized ? _securityProvider.getAutoUpdateEnabled() : false, | ||
| onChanged: _securityProviderInitialized ? (value) async { | ||
| await _securityProvider.setAutoUpdateEnabled(value); | ||
| setState(() {}); | ||
| } : null, | ||
| ), | ||
| ListTile( | ||
| title: Text(tr('updateInterval')), | ||
| subtitle: Text(tr('updateIntervalDescription')), | ||
| trailing: DropdownButton<int>( | ||
| value: _securityProviderInitialized ? _securityProvider.getUpdateInterval() : 1, | ||
| items: [1, 6, 12, 24, 48, 72].map((hours) { | ||
| return DropdownMenuItem<int>( | ||
| value: hours, | ||
| child: Text('$hours ${tr('hours')}'), | ||
| ); | ||
| }).toList(), | ||
| onChanged: _securityProviderInitialized ? (value) async { | ||
| if (value != null) { | ||
| await _securityProvider.setUpdateInterval(value); | ||
| setState(() {}); | ||
| } | ||
| } : null, |
There was a problem hiding this comment.
Guard the post-await setState() calls with mounted.
Unlike the first switch, the auto-update toggle and interval dropdown call setState() after an await without checking whether the page is still alive. Navigating away mid-write can trip setState() called after dispose().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/pages/settings.dart` around lines 934 - 959, The SwitchListTile and
DropdownButton onChanged handlers call setState() after awaiting async
operations and must guard with mounted to avoid calling setState() after
dispose; update the handlers for _securityProvider.setAutoUpdateEnabled and
_securityProvider.setUpdateInterval to check if (mounted) before calling
setState() (or return early if not mounted) and ensure these checks reference
the same StatefulWidget's mounted property so the UI update only runs when the
widget is still alive.
Comment on lines
+950
to
+993
| /// Scan APK for malware before installation | ||
| Future<bool> _scanAPKForMalware(String apkPath, {List<String>? additionalApkPaths}) async { | ||
| SecuritySettingsProvider? securityProvider; | ||
| try { | ||
| securityProvider = await SecuritySettingsProvider.create(); | ||
| await securityProvider.initialize(); | ||
| final scanResult = await securityProvider.scanAPK(apkPath); | ||
|
|
||
| if (scanResult.error != null) { | ||
| logs.add('Security scan error for primary APK: ${scanResult.error}'); | ||
| return false; | ||
| } | ||
|
|
||
| if (scanResult.isInfected) { | ||
| logs.add( | ||
| 'Security scan detected malware in APK: ${scanResult.matches.map((m) => m.ruleName).join(', ')}', | ||
| ); | ||
| return false; | ||
| } | ||
|
|
||
| if (additionalApkPaths != null) { | ||
| for (final additionalApkPath in additionalApkPaths) { | ||
| final additionalScanResult = await securityProvider.scanAPK(additionalApkPath); | ||
|
|
||
| if (additionalScanResult.error != null) { | ||
| logs.add('Security scan error for additional APK: ${additionalScanResult.error}'); | ||
| return false; | ||
| } | ||
|
|
||
| if (additionalScanResult.isInfected) { | ||
| logs.add('Security scan detected malware in additional APK: ${additionalScanResult.matches.map((m) => m.ruleName).join(', ')}'); | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } catch (e) { | ||
| logs.add('Security scan failed: $e'); | ||
| return false; // Block installation on scan failure | ||
| } finally { | ||
| securityProvider?.dispose(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't report scan failures as confirmed malware.
_scanAPKForMalware() returns false for both real detections and scanner errors (scanResult.error / caught exceptions), and installApk() always turns that into the malware-blocked message. A parser/init/IO failure will therefore be surfaced to the user as a confirmed infection. Return a richer result or rethrow the scan error so the blocked-install reason stays accurate.
Also applies to: 1032-1046
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/providers/apps_provider.dart` around lines 950 - 993, The
_scanAPKForMalware function currently conflates scanner errors and true
infections by returning false for both; change it to return a richer result
(e.g., an enum/class like ScanStatus {clean, infected, error} or throw a
dedicated ScanException) so callers can distinguish causes: when
SecuritySettingsProvider.scanAPK returns scanResult.error or when an exception
is caught, return/throw an error status (including the error details) instead of
treating it as an infection, keep infected detection using
scanResult.isInfected, and update installApk to handle the new
ScanStatus/exception (show a distinct "scan failed" message for errors and only
show "malware-blocked" for infected). Ensure you update all call sites
(including the other block around lines 1032-1046) to check the new
result/exception instead of assuming false == infected.
Comment on lines
+183
to
+223
| Future<void> initialize() async { | ||
| await _loadRules(); | ||
|
|
||
| // Cancel existing timer before starting new one | ||
| _updateTimer?.cancel(); | ||
|
|
||
| if (config.enableAutoUpdate) { | ||
| _startAutoUpdate(); | ||
| } | ||
| } | ||
|
|
||
| /// Load YARA rules from local directory | ||
| Future<void> _loadRules() async { | ||
| try { | ||
| final rulesDir = Directory(config.rulesDirectory); | ||
| if (!await rulesDir.exists()) { | ||
| await rulesDir.create(recursive: true); | ||
| } | ||
|
|
||
| _rules.clear(); | ||
| int loadedCount = 0; | ||
| int errorCount = 0; | ||
|
|
||
| await for (final entity in rulesDir.list()) { | ||
| if (entity is File && entity.path.endsWith('.yar')) { | ||
| try { | ||
| final content = await entity.readAsString(); | ||
| final rule = YARARule.fromString(content); | ||
| _rules.add(rule); | ||
| loadedCount++; | ||
| } catch (e) { | ||
| errorCount++; | ||
| // Log error without exposing sensitive file paths or rule content | ||
| _logs.add('Error loading YARA rule file: ${path.basename(entity.path)}'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| _logs.add('YARA rules loaded: $loadedCount successful, $errorCount failed'); | ||
| } catch (e) { | ||
| _logs.add('Error loading YARA rules: ${e.toString()}'); |
There was a problem hiding this comment.
Fresh installs currently scan with an empty rule set.
initialize() only loads on-disk rules and starts the timer. On a clean install, the rules directory is empty, so _rules stays empty and scanFile() reports every APK as clean until the user manually updates rules or the first scheduled update happens.
One safe way to fail closed on first use
Future<void> initialize() async {
await _loadRules();
+ if (_rules.isEmpty) {
+ await updateRules();
+ }
// Cancel existing timer before starting new one
_updateTimer?.cancel();
if (config.enableAutoUpdate) {
_startAutoUpdate();
}
}
...
Future<YARAScanResult> scanFile(String filePath) async {
+ if (_rules.isEmpty) {
+ return YARAScanResult.error(filePath, 'No YARA rules are loaded');
+ }
try {
final file = File(filePath);Also applies to: 320-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/security/yara_scanner.dart` around lines 183 - 223, _initialize() and
_loadRules() leave the scanner with an empty rule set on fresh installs; fix by
adding a safe fallback load when no on-disk rules are found: after iterating
rulesDir in _loadRules(), if loadedCount == 0 call a new helper (e.g.,
_loadBundledRules or _loadDefaultRulesFromAssets) to load embedded/default YARA
rules packaged with the app (or alternatively set a safe "fail-closed" flag that
makes scanFile() treat unknown/empty rule-set as matched), and ensure
_startAutoUpdate() still runs; implement the helper to parse embedded rule
strings into YARARule instances and add to _rules, update _logs with counts, and
reuse the same error handling used in _loadRules(); also apply the same
empty-dir fallback where similar logic appears (lines ~320-339).
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/pages/settings.dart (1)
939-942:⚠️ Potential issue | 🟡 MinorGuard the remaining post-
awaitsetState()calls.The auto-update switch and interval dropdown still call
setState()after awaiting provider writes. Navigating away mid-write can still tripsetState() called after dispose().Minimal fix
onChanged: _securityProviderInitialized ? (value) async { await _securityProvider.setAutoUpdateEnabled(value); - setState(() {}); + if (!mounted) return; + setState(() {}); } : null, ... onChanged: _securityProviderInitialized ? (value) async { if (value != null) { await _securityProvider.setUpdateInterval(value); - setState(() {}); + if (!mounted) return; + setState(() {}); } } : null,Also applies to: 955-959
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings.dart` around lines 939 - 942, The onChanged handlers (e.g. the auto-update switch using _securityProviderInitialized and calling _securityProvider.setAutoUpdateEnabled(...) then setState()) perform an await and then call setState() without guarding against widget disposal; after the await return early if the State is no longer mounted (e.g. check mounted or an equivalent _isDisposed) and only then call setState(); apply the same pattern to the interval dropdown handler (the block around lines referencing _securityProvider.setAutoUpdateInterval or similar) so all post-await setState() calls are protected.
🧹 Nitpick comments (1)
lib/pages/settings.dart (1)
968-985: Differentiate partial rule updates from hard failures.
updateRules()can report a mixed outcome and then throw with both successful and failed sources, but this branch collapses every exception into the same generic failure snackbar. Catch the typed update error separately and summarize the result so a partial refresh doesn't look like a total failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings.dart` around lines 968 - 985, The onPressed handler currently treats all exceptions from _securityProvider.updateRules() as a total failure; instead catch the specific typed exception that updateRules throws (e.g., UpdateRulesException or the concrete exception type returned by updateRules) and inspect its payload to determine which sources succeeded vs failed, then show a Snackbar summarizing "X updated, Y failed" for partial updates; only show the generic failure Snackbar for true hard failures (fallthrough exceptions). Keep the existing mounted checks and ScaffoldMessenger.of(context).showSnackBar usage and update the catch block to handle both the typed partial-result case and a generic catch for other exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/pages/settings.dart`:
- Around line 152-181: The Settings screen currently creates and initializes
SecuritySettingsProvider in initState via _initializeSecurityProvider but never
cleans it up or surfaces errors; update the code so dispose properly tears down
the provider and the UI exposes a retry/error state: in dispose call the
provider's shutdown/close/dispose method (the one implemented by
SecuritySettingsProvider, invoked on the instance returned from
SecuritySettingsProvider.create) and null out _securityProvider to stop
background timers, and in the catch block set an error flag (or reuse
_securityProviderInitialized) and expose a retry action in the widget that
re-invokes _initializeSecurityProvider to allow retries; also guard
_initializeSecurityProvider to avoid double-init when already initializing.
- Around line 949-953: Replace the hardcoded label that produces "1 hours"
inside the items map for the DropdownMenuItem<int> with the existing plural
helper: in the mapping that creates DropdownMenuItem<int> use plural('hour',
hours) (or wrap the count as your plural helper expects) instead of '$hours
${tr('hours')}' so the label inflects correctly for 1 and for other locales.
---
Duplicate comments:
In `@lib/pages/settings.dart`:
- Around line 939-942: The onChanged handlers (e.g. the auto-update switch using
_securityProviderInitialized and calling
_securityProvider.setAutoUpdateEnabled(...) then setState()) perform an await
and then call setState() without guarding against widget disposal; after the
await return early if the State is no longer mounted (e.g. check mounted or an
equivalent _isDisposed) and only then call setState(); apply the same pattern to
the interval dropdown handler (the block around lines referencing
_securityProvider.setAutoUpdateInterval or similar) so all post-await setState()
calls are protected.
---
Nitpick comments:
In `@lib/pages/settings.dart`:
- Around line 968-985: The onPressed handler currently treats all exceptions
from _securityProvider.updateRules() as a total failure; instead catch the
specific typed exception that updateRules throws (e.g., UpdateRulesException or
the concrete exception type returned by updateRules) and inspect its payload to
determine which sources succeeded vs failed, then show a Snackbar summarizing "X
updated, Y failed" for partial updates; only show the generic failure Snackbar
for true hard failures (fallthrough exceptions). Keep the existing mounted
checks and ScaffoldMessenger.of(context).showSnackBar usage and update the catch
block to handle both the typed partial-result case and a generic catch for other
exceptions.
Comment on lines
+152
to
+181
| @override | ||
| void initState() { | ||
| super.initState(); | ||
| _initializeSecurityProvider(); | ||
| } | ||
|
|
||
| @override | ||
| void dispose() { | ||
| // Clean up resources if needed | ||
| super.dispose(); | ||
| } | ||
|
|
||
| Future<void> _initializeSecurityProvider() async { | ||
| try { | ||
| _securityProvider = await SecuritySettingsProvider.create(); | ||
| await _securityProvider.initialize(); | ||
| if (mounted) { | ||
| setState(() { | ||
| _securityProviderInitialized = true; | ||
| }); | ||
| } | ||
| } catch (e) { | ||
| // Handle initialization error | ||
| if (mounted) { | ||
| setState(() { | ||
| _securityProviderInitialized = false; | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't make this screen the unrecoverable owner of the scanner.
initState() now boots SecuritySettingsProvider.initialize(), but dispose() never tears it down and the catch path only leaves _securityProviderInitialized false. Since scanner init can start the periodic auto-update timer, opening Settings can leave background work alive after the screen closes, and bootstrap failures have no retry path. Move ownership up to an app-scoped service/provider or add explicit cleanup plus user-visible error/retry handling here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/pages/settings.dart` around lines 152 - 181, The Settings screen
currently creates and initializes SecuritySettingsProvider in initState via
_initializeSecurityProvider but never cleans it up or surfaces errors; update
the code so dispose properly tears down the provider and the UI exposes a
retry/error state: in dispose call the provider's shutdown/close/dispose method
(the one implemented by SecuritySettingsProvider, invoked on the instance
returned from SecuritySettingsProvider.create) and null out _securityProvider to
stop background timers, and in the catch block set an error flag (or reuse
_securityProviderInitialized) and expose a retry action in the widget that
re-invokes _initializeSecurityProvider to allow retries; also guard
_initializeSecurityProvider to avoid double-init when already initializing.
Comment on lines
+949
to
+953
| items: [1, 6, 12, 24, 48, 72].map((hours) { | ||
| return DropdownMenuItem<int>( | ||
| value: hours, | ||
| child: Text('$hours ${tr('hours')}'), | ||
| ); |
There was a problem hiding this comment.
Use pluralization for the interval labels.
'$hours ${tr('hours')}' renders 1 hours for the first option and won't inflect correctly in other locales. Reuse the existing plural('hour', hours) path here.
Minimal fix
return DropdownMenuItem<int>(
value: hours,
- child: Text('$hours ${tr('hours')}'),
+ child: Text(plural('hour', hours)),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/pages/settings.dart` around lines 949 - 953, Replace the hardcoded label
that produces "1 hours" inside the items map for the DropdownMenuItem<int> with
the existing plural helper: in the mapping that creates DropdownMenuItem<int>
use plural('hour', hours) (or wrap the count as your plural helper expects)
instead of '$hours ${tr('hours')}' so the label inflects correctly for 1 and for
other locales.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/providers/apps_provider.dart (2)
2346-2355:⚠️ Potential issue | 🔴 Critical
export()never writes anything anymore.Both the auto-export branch and the manual export branch now return
nullbefore persistingfinalExport, so exports silently do nothing even when anexportDiralready exists. Either keep the old write path or hide/disable export until a replacement is ready.Also applies to: 2370-2374
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/apps_provider.dart` around lines 2346 - 2355, The export() implementation returns null unconditionally in both the auto-export branch and the manual-export branch, so finalExport is never persisted; remove the premature returns and restore the write/persist logic that writes finalExport to exportDir (or if you prefer to disable exports, gate the UI instead). Specifically, update the isAuto branch in export() (check symbols isAuto, settingsProvider.autoExportOnChanges, exportDir, finalExport) to only early-return when preconditions fail, then call the existing write/export routine used for manual exports; also fix the same premature-return pattern around the other block referenced (lines ~2370-2374) so that the code that writes/persists finalExport to exportDir executes when exportDir exists.
1058-1074:⚠️ Potential issue | 🔴 CriticalFix the type error in the install branch.
codeis declared asint?but both branches assign the string'not_implemented'. This is a Dart type error. Additionally, the string assignment makes the conditioncode != null && code != 0 && code != 3always true, causingInstallErrorto be thrown unconditionally. Every install path will fail after the APK is downloaded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/apps_provider.dart` around lines 1058 - 1074, The variable code is declared as int? but assigned the string 'not_implemented', causing a type error and making the error-check condition always true; change the assignments in the two branches (the ones guarded by settingsProvider.useShizuku) to an int-compatible value (e.g., null if you want to treat "not implemented" as no installer error, or a numeric sentinel like -1/NOT_IMPLEMENTED) and keep the declaration as int?; ensure the subsequent check (code != null && code != 0 && code != 3) only matches real numeric error codes, and preserve cleanup by still calling deleteFile(file.file) and throwing InstallError(code) when an actual numeric error occurs.
♻️ Duplicate comments (3)
lib/pages/settings.dart (2)
163-179:⚠️ Potential issue | 🟠 MajorDon't silently disable the YARA section on bootstrap failure.
If
SecuritySettingsProvider.create()orinitialize()throws, the catch block only leaves_securityProviderInitialized = false. The scanner section then stays disabled with no error copy or retry path, so first-run setup failures look like a permanently unavailable feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings.dart` around lines 163 - 179, The catch in _initializeSecurityProvider currently just sets _securityProviderInitialized = false, which silently hides the YARA section; instead capture the thrown error from SecuritySettingsProvider.create() / initialize(), store it into a new state field (e.g., _securityProviderError) and set _securityProviderInitialized = false, then update the UI to show an error message and a retry action that calls _initializeSecurityProvider again; ensure the mounted checks and setState calls remain around both the error state and retry enabling so users see why YARA is disabled and can retry initialization.
904-929:⚠️ Potential issue | 🟡 MinorGuard the remaining post-
awaitsetState()calls.Unlike the auto-scan toggle above, the auto-update switch and interval dropdown still call
setState()after awaiting provider writes without checkingmounted. Leaving the page mid-write can still triggersetState() called after dispose().Minimal fix
onChanged: _securityProviderInitialized ? (value) async { await _securityProvider.setAutoUpdateEnabled(value); + if (!mounted) return; setState(() {}); } : null, ... onChanged: _securityProviderInitialized ? (value) async { if (value != null) { await _securityProvider.setUpdateInterval(value); + if (!mounted) return; setState(() {}); } } : null,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/pages/settings.dart` around lines 904 - 929, Both the SwitchListTile handler (calling _securityProvider.setAutoUpdateEnabled) and the DropdownButton onChanged handler (calling _securityProvider.setUpdateInterval) await provider writes then call setState() without checking mounted; wrap those post-await setState() calls with a mounted check (e.g., if (!mounted) return; or if (mounted) setState(() { ... });) so that setState is only invoked when the widget is still mounted (apply the change in the async handlers for the SwitchListTile and the DropdownButton).lib/providers/apps_provider.dart (1)
939-982:⚠️ Potential issue | 🟠 MajorDon't collapse scanner errors into "malware detected".
SecuritySettingsProvider.scanAPK()already throws on scan errors (lib/security/security_settings_provider.dart:104-116), so this helper'sboolresult ends up meaning both "infected" and "scanner failed".installApk()then turnsfalseinto the malware-blocked message on Line 1035, which mislabels rule/IO/bootstrap failures as confirmed infections.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/providers/apps_provider.dart` around lines 939 - 982, _scanAPKForMalware is conflating scanner runtime errors with confirmed infections; since SecuritySettingsProvider.scanAPK throws on scan failures, remove or stop treating scanResult.error checks as infection signals and instead let scanAPK exceptions propagate (or rethrow a distinct ScanFailureException) so callers like installApk can differentiate failure vs infected. Concretely, in _scanAPKForMalware (and its loop over additionalApkPaths) stop returning false on scan errors detected via scanResult.error, only return false for scanResult.isInfected, and either rethrow caught exceptions from scanAPK or wrap them in a named exception; update installApk to catch that exception and produce a scanner-failure message rather than the malware-blocked message. Ensure references: _scanAPKForMalware, SecuritySettingsProvider.scanAPK, and installApk are changed accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/pages/home.dart`:
- Around line 147-149: initDeepLinks() currently drops deep-link handling so
interpretLink() is never called and "add"/"app"/"apps" links break; either
restore link sources or remove the parser. To restore, in initDeepLinks() call
the platform API to fetch the initial URI (e.g.,
getInitialUri()/getInitialLink()) and pass it to interpretLink(), then subscribe
to the link stream (e.g., uriLinkStream or onLink/onAppLink) and call
interpretLink() for each incoming URI; ensure you cancel the subscription on
dispose. If the feature is deprecated instead, remove interpretLink(), related
parsing code and any UI that depends on "add"/"app"/"apps" links (also remove
the duplicate dead code around lines 254-258). Reference symbols: initDeepLinks,
interpretLink, uriLinkStream/getInitialUri/getInitialLink, and the link handler
subscription variable.
In `@lib/providers/native_provider.dart`:
- Around line 13-17: The current loadSystemFont() unconditionally sets
_systemFontLoaded = true even when no font is registered; change it to only set
_systemFontLoaded after a successful registration via the
FontLoader('SystemFont') (or return early if registration fails), and ensure it
throws or returns false on failure so callers (e.g., theme switch logic that
expects 'SystemFont') can detect and avoid selecting the theme. Update
loadSystemFont() to await fontLoader.load()/registration and only toggle
_systemFontLoaded on success, and propagate the success/failure to callers so
the UI toggle in settings and the theme switch logic can handle the unsupported
case.
In `@pubspec.yaml`:
- Around line 21-23: Update the Flutter SDK lower bound in the pubspec
environment constraint to require at least 3.27.0 because the code uses APIs
that need that version (e.g., WidgetStateProperty in lib/main.dart,
Color.withValues in lib/main.dart, and Column.spacing in lib/pages/home.dart);
change the flutter constraint from ">=3.19.0" to ">=3.27.0" so package
resolution and builds use a compatible Flutter SDK.
---
Outside diff comments:
In `@lib/providers/apps_provider.dart`:
- Around line 2346-2355: The export() implementation returns null
unconditionally in both the auto-export branch and the manual-export branch, so
finalExport is never persisted; remove the premature returns and restore the
write/persist logic that writes finalExport to exportDir (or if you prefer to
disable exports, gate the UI instead). Specifically, update the isAuto branch in
export() (check symbols isAuto, settingsProvider.autoExportOnChanges, exportDir,
finalExport) to only early-return when preconditions fail, then call the
existing write/export routine used for manual exports; also fix the same
premature-return pattern around the other block referenced (lines ~2370-2374) so
that the code that writes/persists finalExport to exportDir executes when
exportDir exists.
- Around line 1058-1074: The variable code is declared as int? but assigned the
string 'not_implemented', causing a type error and making the error-check
condition always true; change the assignments in the two branches (the ones
guarded by settingsProvider.useShizuku) to an int-compatible value (e.g., null
if you want to treat "not implemented" as no installer error, or a numeric
sentinel like -1/NOT_IMPLEMENTED) and keep the declaration as int?; ensure the
subsequent check (code != null && code != 0 && code != 3) only matches real
numeric error codes, and preserve cleanup by still calling deleteFile(file.file)
and throwing InstallError(code) when an actual numeric error occurs.
---
Duplicate comments:
In `@lib/pages/settings.dart`:
- Around line 163-179: The catch in _initializeSecurityProvider currently just
sets _securityProviderInitialized = false, which silently hides the YARA
section; instead capture the thrown error from SecuritySettingsProvider.create()
/ initialize(), store it into a new state field (e.g., _securityProviderError)
and set _securityProviderInitialized = false, then update the UI to show an
error message and a retry action that calls _initializeSecurityProvider again;
ensure the mounted checks and setState calls remain around both the error state
and retry enabling so users see why YARA is disabled and can retry
initialization.
- Around line 904-929: Both the SwitchListTile handler (calling
_securityProvider.setAutoUpdateEnabled) and the DropdownButton onChanged handler
(calling _securityProvider.setUpdateInterval) await provider writes then call
setState() without checking mounted; wrap those post-await setState() calls with
a mounted check (e.g., if (!mounted) return; or if (mounted) setState(() { ...
});) so that setState is only invoked when the widget is still mounted (apply
the change in the async handlers for the SwitchListTile and the DropdownButton).
In `@lib/providers/apps_provider.dart`:
- Around line 939-982: _scanAPKForMalware is conflating scanner runtime errors
with confirmed infections; since SecuritySettingsProvider.scanAPK throws on scan
failures, remove or stop treating scanResult.error checks as infection signals
and instead let scanAPK exceptions propagate (or rethrow a distinct
ScanFailureException) so callers like installApk can differentiate failure vs
infected. Concretely, in _scanAPKForMalware (and its loop over
additionalApkPaths) stop returning false on scan errors detected via
scanResult.error, only return false for scanResult.isInfected, and either
rethrow caught exceptions from scanAPK or wrap them in a named exception; update
installApk to catch that exception and produce a scanner-failure message rather
than the malware-blocked message. Ensure references: _scanAPKForMalware,
SecuritySettingsProvider.scanAPK, and installApk are changed accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41a8b271-8664-4fef-9170-0e9619b80e93
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
android/AndroidManifest.xmllib/custom_errors.dartlib/main.dartlib/pages/app.dartlib/pages/apps.dartlib/pages/home.dartlib/pages/settings.dartlib/providers/apps_provider.dartlib/providers/native_provider.dartlib/providers/settings_provider.dartpubspec.yaml
💤 Files with no reviewable changes (3)
- lib/pages/app.dart
- lib/pages/apps.dart
- lib/custom_errors.dart
✅ Files skipped from review due to trivial changes (1)
- android/AndroidManifest.xml
Comment on lines
13
to
17
| static Future loadSystemFont() async { | ||
| if (_systemFontLoaded) return; | ||
| var fontLoader = FontLoader('SystemFont'); | ||
| var fontFilePath = await AndroidSystemFont().getFilePath(); | ||
| fontLoader.addFont(_readFileBytes(fontFilePath!)); | ||
| fontLoader.load(); | ||
| // AndroidSystemFont functionality removed | ||
| _systemFontLoaded = true; |
There was a problem hiding this comment.
Don't mark the system font as loaded when no font was registered.
loadSystemFont() now just flips _systemFontLoaded to true, but lib/main.dart:413-422 still switches the theme to 'SystemFont' and lib/pages/settings.dart:1070-1079 still lets users enable it. That makes the toggle a silent no-op at best and an undefined font-family fallback at worst.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/providers/native_provider.dart` around lines 13 - 17, The current
loadSystemFont() unconditionally sets _systemFontLoaded = true even when no font
is registered; change it to only set _systemFontLoaded after a successful
registration via the FontLoader('SystemFont') (or return early if registration
fails), and ensure it throws or returns false on failure so callers (e.g., theme
switch logic that expects 'SystemFont') can detect and avoid selecting the
theme. Update loadSystemFont() to await fontLoader.load()/registration and only
toggle _systemFontLoaded on success, and propagate the success/failure to
callers so the UI toggle in settings and the theme switch logic can handle the
unsupported case.
Comment on lines
21
to
+23
| environment: | ||
| sdk: ^3.10.0 | ||
| flutter: ">=3.38.0" | ||
| sdk: ^3.3.0 | ||
| flutter: ">=3.19.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Do Flutter 3.19.0 / Dart 3.3 support Color.withValues, WidgetStateProperty, and the spacingparameter onColumn/Row? If not, what minimum Flutter version introduced them?
💡 Result:
Web search failed: Server error: no LLM provider could handle the message
🌐 Web query:
Flutter Color.withValues API introduced version
💡 Result:
Color.withValues was introduced as part of Flutter’s wide‑gamut Color API changes—it landed in 3.26.0-0.1.pre and first shipped in a stable release in Flutter 3.27.0. [1]
Sources
[1] Flutter breaking change doc (“Migration guide for wide gamut Color”, Timeline section) (docs.flutter.dev)
Citations:
🌐 Web query:
Flutter WidgetStateProperty introduced version
💡 Result:
WidgetStateProperty was introduced on the Flutter pre-release channel in 3.21.0-11.0.pre, and it reached the stable channel in Flutter 3.22.0 (as part of renaming/moving MaterialState* APIs to WidgetState*). [1]
🌐 Web query:
Flutter Column spacing Row spacing parameter introduced version
💡 Result:
The spacing parameter for Row and Column (implemented as Flex.spacing) was introduced in Flutter 3.27.0. [1] (It briefly landed, was reverted, then re-landed during the 3.27 cycle. [1])
Raise Flutter minimum version to 3.27.0.
The flutter: ">=3.19.0" floor is incompatible with APIs already used in this codebase:
WidgetStateProperty(inlib/main.dart) requires Flutter 3.22.0+Color.withValues(inlib/main.dart) requires Flutter 3.27.0+Column.spacing(inlib/pages/home.dart) requires Flutter 3.27.0+
Update the pubspec.yaml constraint to flutter: ">=3.27.0".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pubspec.yaml` around lines 21 - 23, Update the Flutter SDK lower bound in the
pubspec environment constraint to require at least 3.27.0 because the code uses
APIs that need that version (e.g., WidgetStateProperty in lib/main.dart,
Color.withValues in lib/main.dart, and Column.spacing in lib/pages/home.dart);
change the flutter constraint from ">=3.19.0" to ">=3.27.0" so package
resolution and builds use a compatible Flutter SDK.
Owner
Author
|
OK, I'm closing this PR because it lags my computer. Will open a new one |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Localization
UX
Style