Look-for-regressions PR#408
Conversation
…ted casts in generated_form Phase 2B: Moved filter/sort/reorder logic from apps.dart into a pure AppListBuilder class in lib/components/app_list_builder.dart. Also moved AppsFilter class there to avoid circular imports. Reduces apps.dart build() method by ~150 lines. Phase 3A: Introduced 'final item = widget.items[r][e]' at the top of the generated_form build loop and replaced ~18 repeated casts with direct property access on the narrowed variable. Phase 3B: Fixed multi-line cast patterns that survived the replaceAll.
…ler methods Extracted: - _shareToAppVerifier(): self-contained AppVerifier sharing logic from installApk() - _resolveAppsToInstall(): app validation/sorting loop from downloadAndInstallLatestApps() into a method returning a Dart record (appsToInstall, trackOnlyAppsToUpdate)
→ in corrupt JSON log e is ClientException → err is ClientException in retry logic Bounds check apkUrls before preferredApkIndex access Copy SharedPreferences keys before removeWhere Null-safe pickedSource?.runtimeType Empty string guards in _splitAlphaNumeric and _isNumeric
- Add GoogleSans-Variable.ttf as the new primary font asset - Update font family configuration in pubspec.yaml - Replace font family references with GoogleSans in main.dart - Remove Hebrew language-specific font fallback logic - Update fontFamilyFallback lists to use GoogleSans as default
… and release.yml) Add debug APKs for Nightly build Refactor GitHub Actions workflows Updated workflow name and added caching for Flutter and Gradle dependencies. Introduced a prepare job to check for code changes and auto-fix linting issues. Create gitguardian.yaml Refactor nightly.yml for improved workflow and caching Refactor nightly workflow to include preparation job and caching for Flutter and Gradle dependencies. Added checks for code changes and auto-fix linting issues. Update ci.yml Remove key.properties verification step Removed verification step for key.properties and its contents. Remove key.properties verification step Removed verification step for key.properties and related environment variable checks. Change release tag name from 'nightly-build' to 'nightly' ci(workflow): remove disk space cleanup step from ubuntu runner Removed the `jlumbroso/free-disk-space` action step from the Ubuntu runner configuration in the CI workflow. This step was previously used to free up disk space during builds but has been removed to simplify the workflow configuration.
And update dependencies
Add some tests and improve existing ones
Update CODE_OF_CONDUCT.md Update CODE_OF_CONDUCT.md Update bug_report.md
Improve TV support style: auto-fix linting and formatting issues
Update strings Another update
Added a note about Safe Mode feature in README.
Updated contribution guidance and support options.
* Move "Source override" to source_provider * Update "Open Source" badge to include IzzyOnDroid * Revise openSourceNote * Update README
*Total -- 951.59kb -> 772.52kb (18.82%) /assets/screenshots/3.material_you.png -- 152.77kb -> 116.16kb (23.96%) /assets/screenshots/5.app_opts.png -- 187.20kb -> 143.78kb (23.2%) /assets/screenshots/2.dark_theme.png -- 147.44kb -> 115.47kb (21.68%) /assets/screenshots/4.app.png -- 139.96kb -> 111.35kb (20.44%) /assets/screenshots/1.apps.png -- 167.83kb -> 145.27kb (13.44%) /assets/screenshots/6.grid_view.png -- 156.39kb -> 140.49kb (10.16%) Signed-off-by: ImgBotApp <ImgBotHelp@gmail.com> Co-authored-by: ImgBotApp <ImgBotHelp@gmail.com>
Replaced setup-java action with system JRE installation.
✅ CI Build Passed - Automatically dismissing previous rejection as the build now succeeds.
There was a problem hiding this comment.
Pull Request Overview
This PR is not up to quality standards. The implementation of the GitHub search filter introduces a critical N+1 network request pattern that will likely lead to immediate API rate limiting for users. Furthermore, a shell injection vulnerability was identified in the release workflow. The PR's scope is excessively broad, spanning core architecture, new app sources, CI/CD hardening, and UI/UX improvements, which increases regression risk—especially since complex logic in 'apps_provider.dart' lacks test coverage and is prone to OutOfMemory crashes. Acceptance criteria for Safe Mode password protection, GitHub repository rename detection, and DNS-over-HTTPS provider switching lack verifying test scenarios. The aggressive 'anti-cheat' logic in 'DeviceAdminReceiver.kt' also requires re-evaluation to ensure platform compliance and stability. These issues should be resolved before merging.
About this PR
- Systemic code duplication between the Bitbucket and Gitea source providers suggests that repository search and release parsing logic should be abstracted into a common base class to improve maintainability.
- The PR scope is extremely broad, combining core feature additions, CI/CD changes, documentation, and mass localization updates. This significantly increases regression risk and complicates the review process.
- The PR title 'Look-for-regressions PR' is generic and does not reflect the significant feature additions, security hardening, and breaking changes such as the deep link scheme migration.
1 comment outside of the diff
.github/workflows/release.yml
line 66🔴 HIGH RISK
Using variable interpolation with 'github' context data in a 'run:' step can lead to shell injection. Map the context data to environment variables and use those variables with double quotes instead.
Test suggestions
- Verify GitHub repository rename detection correctly identifies moved repos and prompts for URL update.
- Test Safe Mode activation with password and verify it blocks the 'Add App' functionality.
- Confirm DNS resolution works correctly when switching between different DoH providers in settings.
- Verify that the new app sources (e.g., Bitbucket, Gitea) correctly parse versions and download APKs.
- Test tarball (.tar.gz) extraction and APK filtering logic.
- Verify Android TV specific UI focus and navigation (Shortcuts and TV-specific form focus).
- Implement unit tests for 'lib/providers/apps_provider.dart' to cover complex archive extraction logic and prevent OOM regressions.
- Implement unit tests for 'lib/providers/source_provider.dart' to address high complexity and lack of coverage.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify GitHub repository rename detection correctly identifies moved repos and prompts for URL update.
2. Test Safe Mode activation with password and verify it blocks the 'Add App' functionality.
3. Confirm DNS resolution works correctly when switching between different DoH providers in settings.
4. Test tarball (.tar.gz) extraction and APK filtering logic.
5. Verify Android TV specific UI focus and navigation (Shortcuts and TV-specific form focus).
6. Implement unit tests for 'lib/providers/apps_provider.dart' to cover complex archive extraction logic and prevent OOM regressions.
7. Implement unit tests for 'lib/providers/source_provider.dart' to address high complexity and lack of coverage.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| String filePath, | ||
| String destinationPath, | ||
| ) async { | ||
| final bytes = await File(filePath).readAsBytes(); |
There was a problem hiding this comment.
🔴 HIGH RISK
The 'extractTarballFile' method reads the entire archive into memory as a byte array, which will result in 'OutOfMemoryError' crashes on mobile devices for large archives. Refactor this to use a streaming approach with 'InputFileStream' from the archive package.
| String url = entry.key; | ||
| // Extract owner/repo from https://github.com/owner/repo | ||
| String repoFullName = url | ||
| .substring('https://${hosts[0]}'.length) | ||
| .split('?')[0] | ||
| .split('#')[0]; | ||
|
|
||
| try { | ||
| Response res = await sourceRequest( | ||
| '${await getAPIHost({})}/repos/$repoFullName/releases', | ||
| {}, | ||
| ); | ||
| if (res.statusCode == 200) { | ||
| var releases = jsonDecode(res.body) as List<dynamic>; | ||
| if (releases.isNotEmpty) { | ||
| return entry; | ||
| } | ||
| } | ||
| } catch (e) { | ||
| // Ignore errors and filter out | ||
| } | ||
| return null; | ||
| }); |
There was a problem hiding this comment.
🔴 HIGH RISK
This implementation introduces an N+1 network request pattern during search when 'filterGitHubReleases' is enabled. This will significantly slow down search results and may cause users to hit GitHub API rate limits almost immediately. Instead of verifying all search results concurrently, fetch release data only when a specific app is selected, or implement strict batching and throttling.
| @Suppress("DEPRECATION") | ||
| activityManager.killBackgroundProcesses(packageName) | ||
| } | ||
| } catch (e: Exception) { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Catching a generic Exception can obscure the underlying cause of failure when interacting with system services. Consider catching specific exceptions like 'SecurityException' and 'RemoteException', and use a logging utility instead of 'e.printStackTrace()'.
| @@ -266,6 +279,58 @@ class GitHub extends AppSource { | |||
| ) async => | |||
There was a problem hiding this comment.
🟡 MEDIUM RISK
The logic to inject 'GHReqPrefix' into API requests has been removed, but the setting remains in the configuration UI. Users relying on GitHub proxies will find the functionality broken; restore the logic in the 'getAPIHost' method.
| switch (targetProvider) { | ||
| case DNSProvider.system: | ||
| // System default - use Google's constructor as base | ||
| _dnsOverHttps = DnsOverHttps.google(); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
When 'system' is selected, the service should bypass the DoH client to use the device's native DNS resolution instead of hardcoding Google's DoH servers, which may bypass local network configurations and impact privacy.
| splits { | ||
| abi { | ||
| isEnable = true | ||
| reset() | ||
| include("arm64-v8a", "armeabi-v7a") | ||
| isUniversalApk = false | ||
| } | ||
| } | ||
| */ |
There was a problem hiding this comment.
🟡 MEDIUM RISK
ABI splits are commented out in the Gradle configuration, but the CI scripts (ci.yml, nightly.yml) are configured to expect split-per-abi outputs. This will likely cause build or artifact collection failures.
| <category android:name="android.intent.category.DEFAULT" /> | ||
| <category android:name="android.intent.category.BROWSABLE" /> | ||
| <data android:scheme="obtainium" /> | ||
| <data android:scheme="updatium" /> |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Changing the URI scheme from 'obtainium' to 'updatium' is a breaking change for existing deep links. Ensure there is a migration strategy or documentation for users relying on 'obtainium://' links.
| (e.key == list.length - 1 | ||
| ? '' | ||
| : e.key == list.length - 2 | ||
| ? '${isUsingEnglish ? ',' : ''} and ' |
There was a problem hiding this comment.
⚪ LOW RISK
Replace the hardcoded ' and ' string literal with a call to the 'and' translation key to ensure proper localization in non-English languages.
|
|
||
| # Configuration | ||
| PROJECT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| BUILD_DIR="${PROJECT_DIR}/build" |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: The variable 'BUILD_DIR' is assigned but never used. If it is not required for external processes, it should be removed.
…ving deprecated workflows - Update actions/attest to v4.1.1 in nightly and release workflows - Pin actions/checkout to v6.0.2 in validate-translations workflow - Pin jlumbroso/free-disk-space to v1.3.1 in release workflow - Remove deprecated translation-validate.yaml workflow - Remove unused apk4free.dart source file - Remove buildAdvancedSettingsWidget from AppSource class - Fix SourceForge URL regex pattern - Fix boolean string comparison in release workflow prerelease condition
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.
…tency - Added APK filtering configuration options (apkFilterRegEx, invertAPKFilter) - Added appId validation with regex pattern - Improved state management in AddAppPage by splitting loading states - Enhanced UI consistency with consistent icon sizing (48px) and padding adjustments - Combined explanation text in settings for better readability - Added DNS service test suite and test utilities BREAKING CHANGE: Removed deprecated `advancedAppSpecificSourceAgnosticSettingFormItems` and `combinedAdvancedSettingFormItems` properties from AppSource
✅ CI Build Passed - Automatically dismissing previous rejection as the build now succeeds.
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.
Replace custom installed apps retrieval with android_package_manager package for improved reliability and functionality. The new implementation uses getAllInstalledInfo() to fetch installed applications with better compatibility and metadata access. BREAKING CHANGE: The installed apps dialog now uses android_package_manager and may display different apps or behavior compared to the previous implementation
- Replace prefetch-first-N with on-demand loading via _onNeedLoad - Remove isPrefetching state and prefetching logic - Update InstalledAppTile to accept onNeedLoad callback - Adjust icon sizes from 14 to 16 in apps page - Refactor grid view layout in apps page from SliverGrid to Wrap - Adjust tile padding and spacing for improved UI consistency
✅ CI Build Passed - Automatically dismissing previous rejection as the build now succeeds.
Update APKMirror source description in README to indicate it is now track-only. Add enforceTrackOnly setting in APKMirror source configuration.
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.
Summary by CodeRabbit