Bad#349
Conversation
Removed the prepare job and its associated steps from the release workflow.
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Omer I.S. <137101815+omeritzics@users.noreply.github.com>
|
Warning Review limit reached
More reviews will be available in 7 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughRelease 26.5.0: version bump and CI git identity change; LogsProvider becomes a ChangeNotifier with provider wiring/watch updates; installed-apps dialog constrained; APK size selection tied to preferred index with refresh; F‑Droid author scrape moved before GitLab metadata; native font-load short‑circuited; minor locale and analyzer tweaks. ChangesUpdatium 26.5.0 Update
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
Review Summary by QodoAdd installed apps list, fix crashes, and enhance app details display
WalkthroughsDescription• Add "Installed Apps" list feature to quickly populate app search • Fix app crashes and improve error handling for Android features • Enhance F-Droid metadata fetching with fallback author extraction • Display APK file size information on app details page • Fix locale codes for Indonesian and Kurdish languages • Update dependencies and improve code quality Diagramflowchart LR
A["User Interface"] -->|"Add Installed Apps Button"| B["Installed Apps Dialog"]
B -->|"Select App"| C["Pre-fill Search"]
D["App Details Page"] -->|"Fetch APK Size"| E["HTTP HEAD Request"]
E -->|"Display"| F["File Size Info"]
G["Android Features"] -->|"Error Handling"| H["Graceful Fallback"]
I["F-Droid Source"] -->|"Enhanced Metadata"| J["Author & Changelog"]
File Changes1. lib/app_sources/codeberg.dart
|
Code Review by Qodo
1.
|
![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 several features and bug fixes, including a new dialog to select from installed apps, APK file size retrieval, improved F-Droid metadata fallback parsing, and robust system font loading. However, several critical issues were identified in the code review: using context.watch instead of context.read on several providers in root and page widgets will cause severe performance degradation and unnecessary rebuilds; changing the MethodChannel name for SAF will break the directory picker on Android; globally ignoring the use_build_context_synchronously lint can hide dangerous bugs; nesting a ListView inside a scrollable AlertDialog is a performance anti-pattern; removing the !hostChanged check on F-Droid metadata queries causes redundant failing requests for custom hosts; and raw http.head requests lack timeouts and standard headers.
| Future<int?> getApkFileSize(String url) async { | ||
| try { | ||
| final response = await http.head(Uri.parse(url)); | ||
| if (response.statusCode == 200) { | ||
| final contentLength = response.headers['content-length']; | ||
| if (contentLength != null) { | ||
| return int.tryParse(contentLength); | ||
| } | ||
| } | ||
| } catch (e) { | ||
| // Ignore errors, file size will just not be displayed | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Using raw http.head without a timeout can cause the app to hang indefinitely if the server is unresponsive. Additionally, some servers (like GitHub) block requests that do not provide a User-Agent header, returning a 403 Forbidden or 400 Bad Request.
To make this more robust, we should add a standard User-Agent header and a reasonable timeout (e.g., 5 seconds).
Future<int?> getApkFileSize(String url) async {
try {
final response = await http.head(
Uri.parse(url),
headers: const {'User-Agent': 'Updatium'},
).timeout(const Duration(seconds: 5));
if (response.statusCode == 200) {
final contentLength = response.headers['content-length'];
if (contentLength != null) {
return int.tryParse(contentLength);
}
}
} catch (e) {
// Ignore errors, file size will just not be displayed
}
return null;
}
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>
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/pages/add_app.dart (2)
909-913:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForce URL field refresh when selecting an installed app.
At Line 909, the selection updates
userInputbut does not bumpurlInputKey, so the visible URL field may not reflect the chosen package.Suggested change
changeUserInput( app.packageName ?? '', true, false, + updateUrlInput: true, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/pages/add_app.dart` around lines 909 - 913, The selection handler calls changeUserInput(app.packageName ?? '', true, false) but does not force the URL TextField to rebuild, so the visible URL may not update; after the changeUserInput call in the installed-app selection branch update the urlInputKey (e.g. assign a new UniqueKey() or increment whatever key value is used for the URL field) inside the same state update so the URL TextField tied to urlInputKey is recreated and reflects the new userInput; ensure you update urlInputKey in the same widget state (setState / equivalent) so the UI refreshes.
875-930:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle installed-app fetch failures before opening the dialog.
getAllInstalledInfo()is awaited withouttry/catch. A platform/plugin exception here leaves the action failing without user feedback.Suggested change
onPressed: () async { - final installedApps = await getAllInstalledInfo(); - if (!context.mounted) return; + List<dynamic> installedApps; + try { + installedApps = await getAllInstalledInfo(); + } catch (e) { + if (!context.mounted) return; + showError(e, context); + return; + } + if (!context.mounted) return; showDialog(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/pages/add_app.dart` around lines 875 - 930, The code calls getAllInstalledInfo() without error handling, so platform/plugin exceptions can abort the flow and still attempt UI actions; wrap the await getAllInstalledInfo() call in a try/catch, handle failures by showing a short user-visible error (e.g., SnackBar or an AlertDialog) and return early if fetching fails, and only call showDialog when the fetch succeeds; update references in this block (getAllInstalledInfo, the surrounding onPressed callback, and later changeUserInput usage) so the dialog/population logic runs only on a successful fetch.
🧹 Nitpick comments (1)
lib/pages/add_app.dart (1)
143-143: ⚡ Quick winUse
readinstead ofwatchforNotificationsProviderhere.
NotificationsProvideris not aListenable/ChangeNotifier, sowatchwon’t make this widget react to its internal state mutations; this creates misleading reactivity assumptions at Line 143.Suggested change
- NotificationsProvider notificationsProvider = context - .watch<NotificationsProvider>(); + NotificationsProvider notificationsProvider = context + .read<NotificationsProvider>();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/pages/add_app.dart` at line 143, Replace the incorrect .watch<NotificationsProvider>() call with a non-listening read access since NotificationsProvider is not a Listenable/ChangeNotifier; locate the usage of .watch<NotificationsProvider>() in add_app.dart (the widget build/context usage) and change it to context.read<NotificationsProvider>() (or Provider.of<NotificationsProvider>(context, listen: false)) so the code obtains the provider without expecting widget reactivity from NotificationsProvider.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/main.dart`:
- Line 65: The Locale entry for Kurdish is incorrect: update the MapEntry
Locale('en', 'KMR') used in supportedLocales (and any forcedLocale logic relying
on fl.toLanguageTag()) to Locale('kmr') so the language tag is "kmr"; then
rename or add the translation asset to match (e.g.,
assets/translations/kmr.json) and update any references that load
assets/translations/en-KMR.json or use "en-KMR" so they point to the new "kmr"
file/name.
In `@lib/pages/settings.dart`:
- Line 1422: The dialog is calling context.watch<LogsProvider>() but
LogsProvider is registered via Provider(create: ...) and is not reactive, so
mutations like LogsProvider.add()/clear() won't trigger rebuilds; either change
the usage to context.read<LogsProvider>() if you only need a one-time fetch
(e.g., in filterLogs() to set logString), or make LogsProvider reactive by
converting it into a ChangeNotifier/ValueNotifier (update its class to extend
ChangeNotifier, call notifyListeners() in add()/clear(), and register it with
ChangeNotifierProvider) so context.watch<LogsProvider>() will rebuild the dialog
on log changes.
In `@pubspec.yaml`:
- Line 19: The pubspec.yaml version build number regresses (currently "version:
26.5.0+260526") which will break Android upgrades because Android versionCode
must always increase; update the version's build number to a value greater than
the prior +2604141 (e.g., bump to +2604142 or higher) so the generated Android
versionCode increases, and ensure the "version: 26.5.0+<new_build>" line is
updated accordingly.
---
Outside diff comments:
In `@lib/pages/add_app.dart`:
- Around line 909-913: The selection handler calls
changeUserInput(app.packageName ?? '', true, false) but does not force the URL
TextField to rebuild, so the visible URL may not update; after the
changeUserInput call in the installed-app selection branch update the
urlInputKey (e.g. assign a new UniqueKey() or increment whatever key value is
used for the URL field) inside the same state update so the URL TextField tied
to urlInputKey is recreated and reflects the new userInput; ensure you update
urlInputKey in the same widget state (setState / equivalent) so the UI
refreshes.
- Around line 875-930: The code calls getAllInstalledInfo() without error
handling, so platform/plugin exceptions can abort the flow and still attempt UI
actions; wrap the await getAllInstalledInfo() call in a try/catch, handle
failures by showing a short user-visible error (e.g., SnackBar or an
AlertDialog) and return early if fetching fails, and only call showDialog when
the fetch succeeds; update references in this block (getAllInstalledInfo, the
surrounding onPressed callback, and later changeUserInput usage) so the
dialog/population logic runs only on a successful fetch.
---
Nitpick comments:
In `@lib/pages/add_app.dart`:
- Line 143: Replace the incorrect .watch<NotificationsProvider>() call with a
non-listening read access since NotificationsProvider is not a
Listenable/ChangeNotifier; locate the usage of .watch<NotificationsProvider>()
in add_app.dart (the widget build/context usage) and change it to
context.read<NotificationsProvider>() (or
Provider.of<NotificationsProvider>(context, listen: false)) so the code obtains
the provider without expecting widget reactivity from NotificationsProvider.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46db5fce-1b81-4e9e-8485-7b50ea5e8da2
📒 Files selected for processing (7)
.github/workflows/nightly.ymlassets/translations/en-KMR.jsonlib/main.dartlib/pages/add_app.dartlib/pages/settings.dartlib/providers/settings_provider.dartpubspec.yaml
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/pages/app.dart (1)
68-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrevent stale APK-size writes from out-of-order async responses.
If
_fetchApkFileSize()is triggered multiple times quickly (preferred index changes), an older request can finish later and overwrite_apkFileSizewith the wrong value.Suggested fix
class _AppPageState extends State<AppPage> { AppInMemory? prevApp; bool updating = false; bool _iconRequested = false; Future<void>? _iconFuture; int? _apkFileSize; int? _prevPreferredApkIndex; + int _apkSizeRequestId = 0; Future<void> _fetchApkFileSize() async { + final requestId = ++_apkSizeRequestId; var appsProvider = context.read<AppsProvider>(); AppInMemory? app = appsProvider.apps[widget.appId]; if (app != null && app.app.apkUrls.isNotEmpty) { final idx = (app.app.preferredApkIndex >= 0 && app.app.preferredApkIndex < app.app.apkUrls.length) ? app.app.preferredApkIndex : 0; final size = await getApkFileSize(app.app.apkUrls[idx].value); - if (mounted) { + if (mounted && requestId == _apkSizeRequestId) { setState(() { _apkFileSize = size; }); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/pages/app.dart` around lines 68 - 77, Older async responses from _fetchApkFileSize can overwrite _apkFileSize if the preferred APK index changes mid-request; capture the request-specific identity (e.g., store the chosen idx and the URL string in local variables before awaiting getApkFileSize) and after the await, before calling setState, verify the app still exists and that its current preferredApkIndex (or the current URL) still matches the captured value; only then update _apkFileSize. This ensures out-of-order responses do not write stale values.lib/providers/logs_provider.dart (1)
131-142:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait
add(...)inclear()(and avoid duplicatenotifyListeners)
LogsProvider.add(...)is async: it awaits the DBinsert(...)and then callsnotifyListeners().clear()currently callsadd(...)withoutawaitand immediately callsnotifyListeners(), so listeners can fire before the “clearedNLogs…” entry is actually written (and notifications can occur twice for successful clears). Updateclear()toawait add(...)and rely on the notification ordering (skip/move the extranotifyListeners()accordingly).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/providers/logs_provider.dart` around lines 131 - 142, The clear() method calls the async add(...) but doesn't await it and then calls notifyListeners() again, causing out-of-order notifications and duplicates; update clear() to await add(plural(...)) so the "clearedNLogsBeforeXAfterY" entry is inserted before listeners run, and remove or avoid the extra notifyListeners() after the await (since add(...) already calls notifyListeners()), ensuring a single notification in the correct order; refer to the clear() and add() methods and the notifyListeners() calls when making the change.lib/app_sources/fdroid.dart (1)
141-161:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix missing return in
FDroid.getLatestAPKDetails
lib/app_sources/fdroid.dart’sFuture<APKDetails> getLatestAPKDetails(...)has noreturnbefore it ends (right before the next@override), so it violates the non-nullable return contract and will fail analysis/compile. Addreturn details;(or otherwise return/throw) before exiting the method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/app_sources/fdroid.dart` around lines 141 - 161, The method FDroid.getLatestAPKDetails currently falls through without returning its APKDetails result; locate the end of the getLatestAPKDetails(...) function (just before the following `@override`) and add a return of the constructed details (e.g., return details;) so the Future<APKDetails> always returns a non-null APKDetails; ensure this occurs after the gitLabSuccess fallback block and inside any relevant try/catch flow so all code paths return or rethrow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/providers/native_provider.dart`:
- Around line 33-34: The code unconditionally sets _systemFontLoaded = true,
preventing any future retry after a transient failure; change this so the flag
is only set when font loading actually succeeds (e.g., move the
_systemFontLoaded = true assignment into the success branch of the font-loading
logic or set it after successful return from the load method), and ensure
error/exception paths leave _systemFontLoaded false (or alternatively introduce
a separate _systemFontLoadAttempted/backoff mechanism if you want to limit
immediate retries); locate the assignment to _systemFontLoaded in the
font-loading method (e.g., _loadSystemFonts or the constructor where the diff
appears) and update the control flow accordingly so failures do not permanently
disable retries.
---
Outside diff comments:
In `@lib/app_sources/fdroid.dart`:
- Around line 141-161: The method FDroid.getLatestAPKDetails currently falls
through without returning its APKDetails result; locate the end of the
getLatestAPKDetails(...) function (just before the following `@override`) and add
a return of the constructed details (e.g., return details;) so the
Future<APKDetails> always returns a non-null APKDetails; ensure this occurs
after the gitLabSuccess fallback block and inside any relevant try/catch flow so
all code paths return or rethrow.
In `@lib/pages/app.dart`:
- Around line 68-77: Older async responses from _fetchApkFileSize can overwrite
_apkFileSize if the preferred APK index changes mid-request; capture the
request-specific identity (e.g., store the chosen idx and the URL string in
local variables before awaiting getApkFileSize) and after the await, before
calling setState, verify the app still exists and that its current
preferredApkIndex (or the current URL) still matches the captured value; only
then update _apkFileSize. This ensures out-of-order responses do not write stale
values.
In `@lib/providers/logs_provider.dart`:
- Around line 131-142: The clear() method calls the async add(...) but doesn't
await it and then calls notifyListeners() again, causing out-of-order
notifications and duplicates; update clear() to await add(plural(...)) so the
"clearedNLogsBeforeXAfterY" entry is inserted before listeners run, and remove
or avoid the extra notifyListeners() after the await (since add(...) already
calls notifyListeners()), ensuring a single notification in the correct order;
refer to the clear() and add() methods and the notifyListeners() calls when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 294e82a1-2956-4091-a85a-6e49e89a1f54
📒 Files selected for processing (8)
analysis_options.yamllib/app_sources/fdroid.dartlib/main.dartlib/pages/add_app.dartlib/pages/app.dartlib/providers/logs_provider.dartlib/providers/native_provider.dartpubspec.yaml
💤 Files with no reviewable changes (1)
- analysis_options.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- pubspec.yaml
Removed the prepare job and its associated steps from the CI workflow.
This reverts commit e44254e.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores