Open
Conversation
Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/40bc2fb9-14e8-45d2-ae38-0a176a42ad30 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/52809504-2e7f-4036-b1d3-90bcffc95004 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/a737d2ea-3836-4f57-8292-4613d92f9ce4 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughChanges encompass Android platform enablement through configuration updates, a new MainActivity implementation managing WebView lifecycle synchronization, refactored session note synchronization logic based on session ID changes, batch completion tracking for image downloads, and cache control headers for error responses on mobile platforms. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
📊 Coverage ReportLines: 3754/5005 (75.004995004995%) ⏱️ Tests: 255 tests in 0.662s
|
![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: 2
🤖 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/components/active_session/mod.rs`:
- Around line 132-170: The one-off textarea initialisation currently uses
use_hook (variable initial_notes) but should run in a post-render effect; remove
the use_hook block and instead add a use_effect that runs once after mount to
set the DOM value of the textarea (element id "session-notes-input") when
initial_notes is not empty, using the same serde_json::to_string and
document::eval pattern; keep the existing session-ID logic with
last_synced_session_id and notes_input intact so the existing use_effect for
session changes still distinguishes between same-session vs different-session
updates and avoids cursor resets.
In `@src/components/exercise_card.rs`:
- Around line 43-76: The effect that bumps completed_batches currently
increments on mount if the global progress signal is already None; change the
use_effect for ImageDownloadProgressSignal so it tracks the previous progress
value and only increments completed_batches when the progress transitions from
Some(...) to None (i.e. prev.is_some() && current.is_none()); update the tracked
previous value inside the same effect after checking so future transitions are
detected correctly. Ensure you reference the existing progress
(use_context::<crate::ImageDownloadProgressSignal>().0), completed_batches, and
the use_effect closure when applying 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc777f51-c44f-4605-92b4-6a19b0da3613
📒 Files selected for processing (5)
Dioxus.tomlandroid/MainActivity.ktsrc/components/active_session/mod.rssrc/components/exercise_card.rssrc/services/imgcache.rs
Comment on lines
+132
to
170
| // Initialise the uncontrolled textarea on first mount. Because we never | ||
| // bind `value:` to the textarea, the DOM starts blank even when the | ||
| // session already has notes (e.g. after reopening the app). This hook | ||
| // fires once and sets the initial DOM value via JavaScript. | ||
| let initial_notes = session.peek().notes.clone(); | ||
| use_hook(move || { | ||
| if !initial_notes.is_empty() { | ||
| let val_js = serde_json::to_string(&initial_notes).unwrap_or_default(); | ||
| spawn(async move { | ||
| document::eval(&format!( | ||
| "var el=document.getElementById('session-notes-input');if(el)el.value={val_js};" | ||
| )); | ||
| }); | ||
| } | ||
| }); | ||
| // Track the session ID so we can distinguish between: | ||
| // (a) the debounce saving the user's own input for the *same* session | ||
| // → do NOT touch the DOM (would reset cursor on Android) | ||
| // (b) a *different* session being loaded | ||
| // → update both the signal and the DOM | ||
| let mut last_synced_session_id = use_signal(|| session.read().id.clone()); | ||
| use_effect(move || { | ||
| let session_notes = session.read().notes.clone(); | ||
| if session_notes != *notes_input.peek() { | ||
| notes_input.set(session_notes.clone()); | ||
| // Update the textarea value via JavaScript so the DOM is updated | ||
| // without a full re-render. A full re-render would reset the | ||
| // cursor to the end of the text on Android's WebView. | ||
| let s = session.read(); | ||
| let new_id = s.id.clone(); | ||
| let new_notes = s.notes.clone(); | ||
| if new_id != *last_synced_session_id.peek() { | ||
| // Different session loaded – update signal and DOM. | ||
| last_synced_session_id.set(new_id); | ||
| notes_input.set(new_notes.clone()); | ||
| spawn(async move { | ||
| let val_js = serde_json::to_string(&session_notes).unwrap_or_default(); | ||
| let val_js = serde_json::to_string(&new_notes).unwrap_or_default(); | ||
| document::eval(&format!( | ||
| "var el=document.getElementById('session-notes-input');if(el)el.value={val_js};" | ||
| )); | ||
| }); | ||
| } | ||
| // Same session: notes changed because the debounce saved the user's | ||
| // own input. Leave the DOM alone to avoid resetting the cursor. | ||
| }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use use_effect for the one-off textarea initialisation.
Dioxus documents use_hook as hook-state initialisation, while direct DOM reads and writes belong in use_effect, which runs after the UI has been rendered. The session-ID gate below is a sensible way to avoid cursor jumps, but the initial fill should move into an effect so it uses the post-render lifecycle instead. (dioxuslabs.com)
♻️ Suggested change
let initial_notes = session.peek().notes.clone();
- use_hook(move || {
+ use_effect(move || {
if !initial_notes.is_empty() {
let val_js = serde_json::to_string(&initial_notes).unwrap_or_default();
spawn(async move {
document::eval(&format!(
"var el=document.getElementById('session-notes-input');if(el)el.value={val_js};"
));
});
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Initialise the uncontrolled textarea on first mount. Because we never | |
| // bind `value:` to the textarea, the DOM starts blank even when the | |
| // session already has notes (e.g. after reopening the app). This hook | |
| // fires once and sets the initial DOM value via JavaScript. | |
| let initial_notes = session.peek().notes.clone(); | |
| use_hook(move || { | |
| if !initial_notes.is_empty() { | |
| let val_js = serde_json::to_string(&initial_notes).unwrap_or_default(); | |
| spawn(async move { | |
| document::eval(&format!( | |
| "var el=document.getElementById('session-notes-input');if(el)el.value={val_js};" | |
| )); | |
| }); | |
| } | |
| }); | |
| // Track the session ID so we can distinguish between: | |
| // (a) the debounce saving the user's own input for the *same* session | |
| // → do NOT touch the DOM (would reset cursor on Android) | |
| // (b) a *different* session being loaded | |
| // → update both the signal and the DOM | |
| let mut last_synced_session_id = use_signal(|| session.read().id.clone()); | |
| use_effect(move || { | |
| let session_notes = session.read().notes.clone(); | |
| if session_notes != *notes_input.peek() { | |
| notes_input.set(session_notes.clone()); | |
| // Update the textarea value via JavaScript so the DOM is updated | |
| // without a full re-render. A full re-render would reset the | |
| // cursor to the end of the text on Android's WebView. | |
| let s = session.read(); | |
| let new_id = s.id.clone(); | |
| let new_notes = s.notes.clone(); | |
| if new_id != *last_synced_session_id.peek() { | |
| // Different session loaded – update signal and DOM. | |
| last_synced_session_id.set(new_id); | |
| notes_input.set(new_notes.clone()); | |
| spawn(async move { | |
| let val_js = serde_json::to_string(&session_notes).unwrap_or_default(); | |
| let val_js = serde_json::to_string(&new_notes).unwrap_or_default(); | |
| document::eval(&format!( | |
| "var el=document.getElementById('session-notes-input');if(el)el.value={val_js};" | |
| )); | |
| }); | |
| } | |
| // Same session: notes changed because the debounce saved the user's | |
| // own input. Leave the DOM alone to avoid resetting the cursor. | |
| }); | |
| // Initialise the uncontrolled textarea on first mount. Because we never | |
| // bind `value:` to the textarea, the DOM starts blank even when the | |
| // session already has notes (e.g. after reopening the app). This hook | |
| // fires once and sets the initial DOM value via JavaScript. | |
| let initial_notes = session.peek().notes.clone(); | |
| use_effect(move || { | |
| if !initial_notes.is_empty() { | |
| let val_js = serde_json::to_string(&initial_notes).unwrap_or_default(); | |
| spawn(async move { | |
| document::eval(&format!( | |
| "var el=document.getElementById('session-notes-input');if(el)el.value={val_js};" | |
| )); | |
| }); | |
| } | |
| }); | |
| // Track the session ID so we can distinguish between: | |
| // (a) the debounce saving the user's own input for the *same* session | |
| // → do NOT touch the DOM (would reset cursor on Android) | |
| // (b) a *different* session being loaded | |
| // → update both the signal and the DOM | |
| let mut last_synced_session_id = use_signal(|| session.read().id.clone()); | |
| use_effect(move || { | |
| let s = session.read(); | |
| let new_id = s.id.clone(); | |
| let new_notes = s.notes.clone(); | |
| if new_id != *last_synced_session_id.peek() { | |
| // Different session loaded – update signal and DOM. | |
| last_synced_session_id.set(new_id); | |
| notes_input.set(new_notes.clone()); | |
| spawn(async move { | |
| let val_js = serde_json::to_string(&new_notes).unwrap_or_default(); | |
| document::eval(&format!( | |
| "var el=document.getElementById('session-notes-input');if(el)el.value={val_js};" | |
| )); | |
| }); | |
| } | |
| // Same session: notes changed because the debounce saved the user's | |
| // own input. Leave the DOM alone to avoid resetting the cursor. | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/active_session/mod.rs` around lines 132 - 170, The one-off
textarea initialisation currently uses use_hook (variable initial_notes) but
should run in a post-render effect; remove the use_hook block and instead add a
use_effect that runs once after mount to set the DOM value of the textarea
(element id "session-notes-input") when initial_notes is not empty, using the
same serde_json::to_string and document::eval pattern; keep the existing
session-ID logic with last_synced_session_id and notes_input intact so the
existing use_effect for session changes still distinguishes between same-session
vs different-session updates and avoids cursor resets.
Comment on lines
+43
to
+76
| // Number of downloads completed so far in this session. Incremented | ||
| // whenever the progress signal transitions from Some(...) to None (i.e. | ||
| // the download batch finishes) so the URL memo re-evaluates and picks up | ||
| // newly-cached images without flickering on every individual file. | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| let mut completed_batches: Signal<u32> = use_signal(|| 0); | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| { | ||
| let progress = use_context::<crate::ImageDownloadProgressSignal>().0; | ||
| use_effect(move || { | ||
| let current = *progress.read(); | ||
| if current.is_none() { | ||
| // A batch just finished – bump the counter so sync_url | ||
| // re-evaluates and can switch to the imgcache:// URL. | ||
| let prev = *completed_batches.peek(); | ||
| completed_batches.set(prev + 1); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // Synchronous URL via the shared model method (covers all non-idb: keys). | ||
| // Only re-evaluated when the user cycles through images (img_index changes). | ||
| // Deliberately does NOT subscribe to the download-progress signal: switching | ||
| // the src from a remote URL to an imgcache:// URL mid-display causes the | ||
| // Android WebView to blank the image briefly while the custom protocol handler | ||
| // serves the new request. Images cached during this session are picked up on | ||
| // the next component mount (e.g. after scrolling or the next app launch). | ||
| // Re-evaluated when the user cycles images OR when a download batch ends. | ||
| let sync_url = { | ||
| let ex = exercise.clone(); | ||
| use_memo(move || ex.get_image_url(*img_index.read())) | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| { | ||
| use_memo(move || { | ||
| let _batch = *completed_batches.read(); // subscribe to batch completions | ||
| ex.get_image_url(*img_index.read()) | ||
| }) | ||
| } | ||
| #[cfg(target_arch = "wasm32")] | ||
| { | ||
| use_memo(move || ex.get_image_url(*img_index.read())) |
Contributor
There was a problem hiding this comment.
Only bump completed_batches on a real Some -> None transition.
This effect increments the counter on first mount whenever the global progress signal is already None, so every card does one needless recompute before any download batch has actually finished. Track the previous download state and only increment on an actual completion transition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/exercise_card.rs` around lines 43 - 76, The effect that bumps
completed_batches currently increments on mount if the global progress signal is
already None; change the use_effect for ImageDownloadProgressSignal so it tracks
the previous progress value and only increments completed_batches when the
progress transitions from Some(...) to None (i.e. prev.is_some() &&
current.is_none()); update the tracked previous value inside the same effect
after checking so future transitions are detected correctly. Ensure you
reference the existing progress
(use_context::<crate::ImageDownloadProgressSignal>().0), completed_batches, and
the use_effect closure when applying the change.
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
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
Bug Fixes