fix(android): notif toast, images, import#135
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a cross‑platform notification permission probe and Android settings opener, increases logging across file‑import, image‑download and storage code paths, refactors native storage/image directory resolution, routes local image writes through a dedicated helper, and comments out the top‑level Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (src/main.rs)
participant NotifSvc as Notification Service (src/services/notifications.rs)
participant AndroidJNI as Android JNI
participant WebAPI as Web API (web_sys)
rect rgba(100,150,255,0.5)
Note over App,NotifSvc: Permission probe invoked at startup/use
App->>NotifSvc: is_notification_permission_granted()
alt Android
NotifSvc->>AndroidJNI: check_android_notification_permission()
AndroidJNI-->>NotifSvc: bool
else wasm32
NotifSvc->>WebAPI: Notification::permission()
WebAPI-->>NotifSvc: "granted"/...
else other native
NotifSvc-->>App: true
end
NotifSvc-->>App: permission bool
end
rect rgba(200,150,100,0.5)
Note over App,NotifSvc: User triggers permission request via toast
App->>App: show NotificationPermissionToast (if not granted)
App->>App: user clicks toast
alt Android
App->>NotifSvc: open_notification_settings()
NotifSvc->>AndroidJNI: build/start APP_NOTIFICATION_SETTINGS intent
else wasm32
App->>WebAPI: Notification::request_permission()
WebAPI-->>App: result
end
App->>App: hide toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
📊 Coverage ReportLines: 3754/4971 (75.51800442566888%) ⏱️ Tests: 255 tests in 0.583s
|
📊 Coverage ReportLines: 3754/4969 (75.54840008049909%) ⏱️ Tests: 255 tests in 0.580s
|
❌ Maestro Web E2E Failures📜 Maestro Console Output (Last 100 lines)📸 Screenshots |
❌ Maestro Web E2E Failures📜 Maestro Console Output (Last 100 lines)📸 Screenshots |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dioxus.toml`:
- Around line 52-53: Remove the legacy
"android.permission.READ_EXTERNAL_STORAGE" entry from Dioxus.toml since the
WebView/HTML file input used by read_file_input() (in src/components/more.rs) is
handled by the browser and not the native filesystem permission; update or leave
the TODO comment to reference using the Storage Access Framework
(ACTION_OPEN_DOCUMENT) if native document access is later required, ensuring no
code relies on that permission string (search for the exact symbol
"android.permission.READ_EXTERNAL_STORAGE" to remove usages).
In `@src/main.rs`:
- Around line 231-238: The toast is hidden unconditionally in the click handler,
so if the user dismisses the browser prompt or returns from Android settings
without enabling notifications the denied state is lost; update the click
handler/flow that currently sets NotificationPermissionToastSignal (notif_toast)
to false to instead re-check
services::notifications::is_notification_permission_granted() after the
prompt/settings flow and set notif_toast to true if permission is still denied
(else set false), and apply the same change to the second occurrence of this
logic (the other use_effect/click handler block).
- Line 132: Replace the hard-coded debug level in the dioxus logger init so
release builds don't always use DEBUG: change the call to
dioxus_logger::init(...) to choose the level at runtime/build (for example use
cfg!(debug_assertions) ? dioxus_logger::tracing::Level::DEBUG :
dioxus_logger::tracing::Level::INFO or consult RUST_LOG/env first) instead of
always passing dioxus_logger::tracing::Level::DEBUG; keep the symbol
dioxus_logger::init and the Level enum usage so you adjust only the level
selection logic and default to INFO for non-debug builds.
In `@src/services/exercise_db.rs`:
- Around line 253-258: When std::fs::create_dir_all(&images_dir) fails, do not
continue: clear the progress state (call progress.clear()) and return early with
an error (propagate or return Err) instead of proceeding into the download loop
and logging success; update the error branch that currently logs "Failed to
create main images directory {}: {e}" to also clear progress and return the
failure so the UI won't show a false success.
In `@src/services/notifications.rs`:
- Around line 17-21: The Android branch in is_notification_permission_granted
currently hides JNI errors by using unwrap_or(true); change it to treat errors
as not granted: call check_android_notification_permission(), log the error when
it returns Err (use the crate's logger, e.g., log::error! or tracing::error!),
and return false on error instead of true so failures do not suppress the
permission prompt. Keep the existing return behavior for success (true/false
from the check) and only alter the error path in
is_notification_permission_granted and its Android-specific block.
- Around line 33-44: Guard ctx.vm() and ctx.context() for null before calling
unsafe JavaVM::from_raw and JObject::from_raw in all four Android-only functions
(e.g., check_android_notification_permission) in notifications.rs; mirror the
null-check pattern used in src/services/storage.rs (lines 898–900): if
ctx.vm().is_null() or ctx.context().is_null() return Err with a clear message,
and only then call unsafe JavaVM::from_raw(ctx.vm().cast()) and unsafe
JObject::from_raw(ctx.context() as jni::sys::jobject); apply this to the call
sites that currently call JavaVM::from_raw and JObject::from_raw (the blocks
around lines 38–44, 83–89, 170–176, 247–253).
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a440cbf0-818f-42ec-a5cc-57684ca21295
📒 Files selected for processing (8)
Dioxus.tomlsrc/components/exercise_form_fields.rssrc/components/more.rssrc/main.rssrc/services/exercise_db.rssrc/services/imgcache.rssrc/services/notifications.rssrc/services/storage.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/services/notifications.rs (2)
17-21:⚠️ Potential issue | 🟠 MajorDo not mask Android permission-probe failures as granted.
Line 20 still falls back to
trueon JNI errors. Because this value gates the Android notification-permission toast, a broken probe suppresses the only recovery path when it should do the opposite. Log the failure and returnfalseon the error path instead.Proposed fix
#[cfg(target_os = "android")] { - check_android_notification_permission().unwrap_or(true) + check_android_notification_permission().unwrap_or_else(|e| { + log::warn!("Failed to check Android notification permission: {e}"); + false + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/notifications.rs` around lines 17 - 21, The Android permission probe in is_notification_permission_granted currently masks JNI errors by using check_android_notification_permission().unwrap_or(true); change this so that any Err from check_android_notification_permission() is logged (e.g., process or crate logger via error!) and the function returns false on error instead of true, while keeping the successful Ok(bool) behavior; update the #[cfg(target_os = "android")] block around is_notification_permission_granted and reference check_android_notification_permission to implement the logging-and-false-on-error behavior.
38-44:⚠️ Potential issue | 🔴 CriticalGuard the raw Android handles before calling
from_raw.Lines 39-44 and Lines 84-89 pass
ctx.vm()andctx.context()straight into unsafe JNI conversions. If either pointer is null during an Android lifecycle edge, this becomes undefined behaviour instead of a recoverable error. Mirror the early-return guard already used insrc/services/storage.rsbefore both call sites.Proposed fix
let ctx = android_context(); + if ctx.vm().is_null() || ctx.context().is_null() { + return Err("Android context not available".into()); + } let vm = unsafe { JavaVM::from_raw(ctx.vm().cast()) } .map_err(|e| format!("JavaVM::from_raw: {e}"))?;Verification
Expected:
src/services/storage.rsshows the null-guard pattern; both call sites above currently do not.#!/bin/bash rg -n -C2 'ctx\.vm\(\)\.is_null|ctx\.context\(\)\.is_null|JavaVM::from_raw|JObject::from_raw' \ src/services/notifications.rs src/services/storage.rsAlso applies to: 83-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/notifications.rs` around lines 38 - 44, The code calls JavaVM::from_raw and JObject::from_raw with ctx.vm() and ctx.context() unsafely; add null-pointer guards like the pattern in src/services/storage.rs before converting: check ctx.vm().is_null() and ctx.context().is_null() and return an Err (with a clear message) if null, then perform unsafe JavaVM::from_raw, call attach_current_thread, and unsafe JObject::from_raw only after the non-null checks; reference symbols: android_context(), ctx.vm(), JavaVM::from_raw, attach_current_thread, ctx.context(), JObject::from_raw, and the variables vm and activity when applying the guards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/notifications.rs`:
- Line 78: Change open_notification_settings() to return Result<(), String> (and
do the same for the other similar function around lines 130-132), stop
swallowing failures by logging only, and instead return an Err containing the
error's to_string(); update callers (e.g., the toast click handler in main.rs)
to handle the Result (use ? or match to keep the toast visible or show a
fallback on Err) so the UI can decide whether to hide the prompt when launching
Settings fails.
---
Duplicate comments:
In `@src/services/notifications.rs`:
- Around line 17-21: The Android permission probe in
is_notification_permission_granted currently masks JNI errors by using
check_android_notification_permission().unwrap_or(true); change this so that any
Err from check_android_notification_permission() is logged (e.g., process or
crate logger via error!) and the function returns false on error instead of
true, while keeping the successful Ok(bool) behavior; update the #[cfg(target_os
= "android")] block around is_notification_permission_granted and reference
check_android_notification_permission to implement the
logging-and-false-on-error behavior.
- Around line 38-44: The code calls JavaVM::from_raw and JObject::from_raw with
ctx.vm() and ctx.context() unsafely; add null-pointer guards like the pattern in
src/services/storage.rs before converting: check ctx.vm().is_null() and
ctx.context().is_null() and return an Err (with a clear message) if null, then
perform unsafe JavaVM::from_raw, call attach_current_thread, and unsafe
JObject::from_raw only after the non-null checks; reference symbols:
android_context(), ctx.vm(), JavaVM::from_raw, attach_current_thread,
ctx.context(), JObject::from_raw, and the variables vm and activity when
applying the guards.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: df04338f-4509-4a1f-85e0-2bee1e71145c
📒 Files selected for processing (1)
src/services/notifications.rs
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>
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
📊 Coverage ReportLines: 3754/4971 (75.51800442566888%) ⏱️ Tests: 255 tests in 0.620s
|
📊 Coverage ReportLines: 3754/4971 (75.51800442566888%) ⏱️ Tests: 255 tests in 0.490s
|
📊 Coverage ReportLines: 3754/4971 (75.51800442566888%) ⏱️ Tests: 255 tests in 0.601s
|
📊 Coverage ReportLines: 3754/4971 (75.51800442566888%) ⏱️ Tests: 255 tests in 0.612s
|
❌ Maestro Web E2E Failures📜 Maestro Console Output (Last 100 lines)📸 Screenshots |
❌ Maestro Web E2E Failures📜 Maestro Console Output (Last 100 lines)📸 Screenshots |
❌ Maestro Web E2E Failures📜 Maestro Console Output (Last 100 lines)📸 Screenshots |
❌ Maestro Web E2E Failures📜 Maestro Console Output (Last 100 lines)📸 Screenshots |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/services/notifications.rs (2)
88-95:⚠️ Potential issue | 🔴 CriticalMissing null-pointer guard before unsafe JNI conversions.
open_notification_settings()callsJavaVM::from_rawandJObject::from_rawwithout first checking ifctx.vm()orctx.context()is null. This causes undefined behaviour if the Android context is unavailable, unlikecheck_android_notification_permission()which has proper guards at lines 42-44.Proposed fix
let result = (|| -> Result<(), String> { let ctx = android_context(); + if ctx.vm().is_null() || ctx.context().is_null() { + return Err("Android context not available".into()); + } let vm = unsafe { JavaVM::from_raw(ctx.vm().cast()) } .map_err(|e| format!("JavaVM::from_raw: {e}"))?; let mut env = vm .attach_current_thread() .map_err(|e| format!("attach_current_thread: {e}"))?; let activity = unsafe { JObject::from_raw(ctx.context() as jni::sys::jobject) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/notifications.rs` around lines 88 - 95, The code in the closure used by open_notification_settings() unsafely calls JavaVM::from_raw and JObject::from_raw without null checks on ctx.vm() and ctx.context(); add guards like those in check_android_notification_permission() to verify ctx.vm() and ctx.context() are non-null and return an Err (with a clear message) if either is null before performing the unsafe conversions (references: android_context(), JavaVM::from_raw, JObject::from_raw, and attach_current_thread()).
84-139: 🧹 Nitpick | 🔵 TrivialConsider returning
Resultto allow caller to handle failures.The function only logs on error (line 137), but the caller in
src/main.rs(line 579-581) hides the permission toast before invoking this. If launching Settings fails, users lose both the prompt and the recovery action with no indication of failure.Returning
Result<(), String>would let the UI keep the toast visible or show a fallback message.Proposed refactor
/// Opens the system notification settings for the application. #[cfg(target_os = "android")] -pub fn open_notification_settings() { +pub fn open_notification_settings() -> Result<(), String> { use jni::{objects::JObject, JavaVM}; use ndk_context::android_context; - let result = (|| -> Result<(), String> { + (|| -> Result<(), String> { let ctx = android_context(); + if ctx.vm().is_null() || ctx.context().is_null() { + return Err("Android context not available".into()); + } // ... rest of implementation unchanged ... Ok(()) - })(); - - if let Err(e) = result { - log::error!("Failed to open Android notification settings: {e}"); - } + })() }Then update the caller in
src/main.rsto handle the result appropriately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/notifications.rs` around lines 84 - 139, open_notification_settings currently swallows errors and only logs them; change its signature from fn open_notification_settings() to pub fn open_notification_settings() -> Result<(), String> and move the inner closure's Result up to return Err(...) on failure instead of logging; remove the final if-let Err logging and let the caller decide. Update the caller site that invokes open_notification_settings (where the permission toast is hidden) to handle the Result: on Ok() proceed as before, on Err(e) keep the toast visible or show a fallback UI/error message and log the error. Ensure unique symbols: modify the function open_notification_settings to return Result<(), String> and adjust the invocation of open_notification_settings in the caller to match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dioxus.toml`:
- Around line 38-39: The Dioxus.toml declares an unused permission entry "photos
= { access = \"read\", description = \"Custom exercises images\" }" which
triggers unnecessary photo library prompts; remove that "photos" permission
declaration from Dioxus.toml so the app no longer requests photo access (leave
other permission entries intact) since image handling uses std::fs::copy and
HTML file inputs instead.
---
Duplicate comments:
In `@src/services/notifications.rs`:
- Around line 88-95: The code in the closure used by
open_notification_settings() unsafely calls JavaVM::from_raw and
JObject::from_raw without null checks on ctx.vm() and ctx.context(); add guards
like those in check_android_notification_permission() to verify ctx.vm() and
ctx.context() are non-null and return an Err (with a clear message) if either is
null before performing the unsafe conversions (references: android_context(),
JavaVM::from_raw, JObject::from_raw, and attach_current_thread()).
- Around line 84-139: open_notification_settings currently swallows errors and
only logs them; change its signature from fn open_notification_settings() to pub
fn open_notification_settings() -> Result<(), String> and move the inner
closure's Result up to return Err(...) on failure instead of logging; remove the
final if-let Err logging and let the caller decide. Update the caller site that
invokes open_notification_settings (where the permission toast is hidden) to
handle the Result: on Ok() proceed as before, on Err(e) keep the toast visible
or show a fallback UI/error message and log the error. Ensure unique symbols:
modify the function open_notification_settings to return Result<(), String> and
adjust the invocation of open_notification_settings in the caller to match.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aedf0561-bccd-4fda-8e38-417ef0d9e0ee
📒 Files selected for processing (3)
Dioxus.tomlsrc/services/exercise_db.rssrc/services/notifications.rs
📊 Coverage ReportLines: 3754/4971 (75.51800442566888%) ⏱️ Tests: 255 tests in 0.602s
|
❌ Maestro Web E2E Failures📜 Maestro Console Output (Last 100 lines)📸 Screenshots |
9 tasks
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.
This Pull Request…
Engineering Principles
scope is focused (no unrelated dependency updates or formatting)
README’s Engineering PrinciplesCI/CD Readiness
feat/…,fix/…,refactor/…, …dx fmt; cargo fmtnix flake checkssucceeds without warningsdx buildwith necessary platform flags succeedscargo clippy -- -D warnings -W clippy::all -W clippy::pedanticproduces zero warnings
cargo llvm-cov nextest --ignore-filename-regex '(src/components/|\.cargo/registry/|nix/store)'maestro test --headless maestro/webmaestro test --headless maestro/androidSummary by CodeRabbit
New Features
Improvements