Changes before error encountered#142
Conversation
Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/0fa7bfd2-30f6-431b-8cc3-ae8db43b741e Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds two new localisation keys for deletion toasts; emits those toasts on session or exercise-log deletion; implements press‑and‑hold continuous increment/decrement and improved cardio distance/placeholder handling in the session exercise form; and adds an unmount/cancellation token to prevent stale timer notifications. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Comp as Component
participant App as AppState / ToastSignal
participant Storage
Note over User,Comp: Delete exercise log or session
User->>Comp: Click Delete
Comp->>App: enqueue toast (`toast-log-deleted` / `toast-session-deleted`)
App->>Storage: persist deletion (save_session / enqueue_delete_session)
App->>App: ToastSignal broadcasts toast -> UI shows notification
Note over Comp: Timers use cancel token to avoid stray notifications
User->>Comp: Unmount component
Comp->>Comp: set cancel token -> schedule_duration_notification returns early
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
📊 Coverage ReportLines: 3757/5000 (75.14%) ⏱️ Tests: 255 tests in 0.615s
|
![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 (1)
src/components/completed_exercise_log.rs (1)
85-93:⚠️ Potential issue | 🟡 MinorMisleading “Exercise log deleted” toast when the screen-lock guard rejects the write.
The toast is enqueued unconditionally at the top of the closure, but
storage::save_session(insrc/services/app_state.rs) short-circuits and pushestoast-screen-lockedwhenscreen_is_locked()is true and the session is not the currently active one. In that case the user sees both “🗑️ Exercise log deleted” and “🔒 Screen locked …”, falsely confirming a deletion that never happened.Consider enqueuing the success toast only after the guards have passed (i.e. after
save_sessionin a path that returns success), or moving the guard/confirmation inside this component before emitting the toast.🔧 One possible adjustment
on_delete: move |()| { - consume_context::<crate::ToastSignal>() - .0 - .write() - .push_back(t!("toast-log-deleted").to_string()); let mut current_session = session.read().clone(); current_session.exercise_logs.remove(idx); storage::save_session(current_session); + consume_context::<crate::ToastSignal>() + .0 + .write() + .push_back(t!("toast-log-deleted").to_string()); },Note:
save_sessionstill returns()on a guard rejection, so ideally it should expose success/failure to the caller so the toast is only shown on an actual save. Happy to sketch that refactor if useful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/completed_exercise_log.rs` around lines 85 - 93, The toast "toast-log-deleted" is enqueued before attempting to persist changes, causing a misleading success message when storage::save_session short-circuits (e.g., screen lock); move the consume_context::<crate::ToastSignal>().0.write().push_back(...) call so it runs only after storage::save_session(...) completes successfully (i.e., after you mutate current_session.exercise_logs and call save_session), and if possible refactor storage::save_session to return a Result or boolean success flag that on_delete can check before enqueuing the success toast; update the on_delete closure accordingly (referencing on_delete, current_session.exercise_logs.remove, storage::save_session and consume_context::<crate::ToastSignal>).
🤖 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/session_exercise_form.rs`:
- Around line 98-131: Extract the duplicated press-and-hold logic into two small
helpers (e.g. fn start_hold_repeat(mut gen_sig: Signal<u32>, mut tick: impl
FnMut() + 'static) and fn cancel_hold_repeat(mut gen_sig: Signal<u32>)) that
perform the immediate tick, bump the generation, spawn the delayed repeat loop
(HOLD_INITIAL_DELAY_MS / HOLD_REPEAT_INTERVAL_MS) and cancel by bumping the gen;
then replace each button's
onpointerdown/onpointerup/onpointerleave/onpointercancel blocks (references:
time_btn_gen, rep_btn_gen, sets_btn_gen, etc.) with calls to
start_hold_repeat(gen_sig, move || { /* original tick body using time_input /
rep_input / sets_input */ }) and cancel_hold_repeat(gen_sig) so each ± button
only contains its tick closure and uses the existing parse_duration_seconds /
format_time call sites.
In `@src/components/session_timers.rs`:
- Around line 15-34: Replace the custom CancelOnDrop type and its Clone/Drop
logic with Dioxus lifecycle primitives: in ExerciseElapsedTimer and
InlineExerciseTimer stop using Arc<AtomicBool> cancel tokens and the
CancelOnDrop owner/non-owner pattern and instead either (a) call use_drop to run
cleanup on unmount or (b) prefer dioxus::prelude::spawn (or use_future) inside
the existing use_effect to run the timeout so the task is automatically
cancelled on unmount/re-run; remove the CancelOnDrop struct and all
_cancel_guard/cloning usage, and if you retain a cancel-token approach extract
the duplicated cancel-token boilerplate from ExerciseElapsedTimer and
InlineExerciseTimer into a shared helper function called by both.
- Around line 85-94: The effect in use_effect that calls
schedule_duration_notification spawns wasm_bindgen_futures::spawn_local tasks
(guarded by cancel and duration_bell_rung) on every render, leaking pending
futures; modify the use_effect invocation that wraps
schedule_duration_notification to include a dependency array [exercise_start,
last_duration] (or replace with yew_hooks::use_future/use_future_with_deps) so
tasks only spawn when those values change, ensuring no accumulation of in-flight
spawn_local timers tied to re-renders.
---
Outside diff comments:
In `@src/components/completed_exercise_log.rs`:
- Around line 85-93: The toast "toast-log-deleted" is enqueued before attempting
to persist changes, causing a misleading success message when
storage::save_session short-circuits (e.g., screen lock); move the
consume_context::<crate::ToastSignal>().0.write().push_back(...) call so it runs
only after storage::save_session(...) completes successfully (i.e., after you
mutate current_session.exercise_logs and call save_session), and if possible
refactor storage::save_session to return a Result or boolean success flag that
on_delete can check before enqueuing the success toast; update the on_delete
closure accordingly (referencing on_delete,
current_session.exercise_logs.remove, storage::save_session and
consume_context::<crate::ToastSignal>).
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25b1a3ae-8a9e-49df-8ea9-907965270493
📒 Files selected for processing (7)
assets/en.ftlassets/es.ftlassets/fr.ftlsrc/components/completed_exercise_log.rssrc/components/session_exercise_form.rssrc/components/session_timers.rssrc/services/app_state.rs
|
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 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
📊 Coverage ReportLines: 3757/5000 (75.14%) ⏱️ Tests: 255 tests in 0.600s
|
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/session_exercise_form.rs`:
- Around line 105-110: Remove the redundant mutability on the per-field
generation signal bindings: change the declarations of time_btn_gen,
weight_btn_gen, dist_btn_gen, and reps_btn_gen so they are created with
use_signal(|| 0u32) without the mut keyword; leave start_hold_repeat and
cancel_hold_repeat usage unchanged (these functions accept their own mut
parameters), so only remove mut from the local bindings in the
session_exercise_form component.
In `@src/components/session_timers.rs`:
- Around line 41-44: Update the stale intra-doc link that points to
`CancelOnDrop` in the doc comment describing the cancel token: replace the
reference with the new helper `use_cancel_token` (or an intra-doc link like
[`use_cancel_token`]) and, if helpful, mention it uses `use_drop`; modify the
comment block around the paragraph that currently mentions `CancelOnDrop` in
src/components/session_timers.rs so the link is correct and `rustdoc`/`cargo
clippy` no longer reports a broken_intra_doc_links warning.
🪄 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: 9de9b992-8934-42c6-ba70-67d715049cbb
📒 Files selected for processing (2)
src/components/session_exercise_form.rssrc/components/session_timers.rs
📊 Coverage ReportLines: 3757/5000 (75.14%) ⏱️ Tests: 255 tests in 0.593s
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/session_exercise_form.rs`:
- Around line 9-38: Refactor the spawned repeat loop in start_hold_repeat to use
a while loop with the cancellation predicate instead of loop { if … break; … }
for clarity: inside the async move block replace the current loop/if-break
pattern with while *gen_sig.peek() == next_gen { tick();
crate::utils::sleep_ms(HOLD_REPEAT_INTERVAL_MS).await; } so the cancellation
condition is self-evident (keep the existing tick(), sleep, and use of next_gen
as-is).
In `@src/components/session_timers.rs`:
- Around line 158-171: Extract the duplicated cancel-token + scheduling effect
into a shared hook called e.g. use_duration_notification and replace the inline
effect in both ExerciseElapsedTimer and InlineExerciseTimer with a single call
to that hook; implement use_duration_notification to call use_cancel_token(),
then use_effect(use_reactive!(|(exercise_start, last_duration)| {
schedule_duration_notification(exercise_start, last_duration,
duration_bell_rung, cancel.clone()); })), and keep the same parameter types
(exercise_start: Option<u64>, last_duration: Option<u64>, duration_bell_rung:
Signal<bool>) so existing calls to schedule_duration_notification,
use_cancel_token, use_effect and use_reactive continue to work without other
changes.
- Around line 164-171: The effect spawns a gloo_timers task that isn’t canceled
on effect re-run, causing stale notifications; update the reactive closure
around use_effect/use_reactive! that calls schedule_duration_notification so
each re-run cancels the previous task: either (A) create a per-effect cancel
token (e.g., a Signal/Arc<AtomicBool> stored via a use_hook or Signal) and flip
it before spawning the new task and still flip on unmount (replacing the shared
cancel used by schedule_duration_notification), or (B, preferred) convert the
logic to use_resource(use_reactive!(...)) for schedule_duration_notification so
Dioxus auto-cancels in-flight tasks on dependency changes and unmount; apply the
same fix to InlineExerciseTimer and remove reliance on the global
use_cancel_token cancel flag for these effects.
🪄 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: 97cd8701-56c9-4a61-95be-aa1c4f68387e
📒 Files selected for processing (2)
src/components/session_exercise_form.rssrc/components/session_timers.rs
| /// Initial hold delay before continuous increment/decrement starts (ms). | ||
| const HOLD_INITIAL_DELAY_MS: u32 = 400; | ||
| /// Interval between repeated increments/decrements while holding (ms). | ||
| const HOLD_REPEAT_INTERVAL_MS: u32 = 100; | ||
|
|
||
| /// Start press-and-hold repeat behavior for increment/decrement buttons. | ||
| /// | ||
| /// Performs an immediate tick, bumps the generation signal to cancel any | ||
| /// in-flight hold task, then spawns a delayed repeat loop. | ||
| fn start_hold_repeat(mut gen_sig: Signal<u32>, mut tick: impl FnMut() + 'static) { | ||
| tick(); | ||
| let next_gen = gen_sig.peek().wrapping_add(1); | ||
| gen_sig.set(next_gen); | ||
| spawn(async move { | ||
| crate::utils::sleep_ms(HOLD_INITIAL_DELAY_MS).await; | ||
| loop { | ||
| if *gen_sig.peek() != next_gen { | ||
| break; | ||
| } | ||
| tick(); | ||
| crate::utils::sleep_ms(HOLD_REPEAT_INTERVAL_MS).await; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /// Cancel press-and-hold repeat by bumping the generation signal. | ||
| fn cancel_hold_repeat(mut gen_sig: Signal<u32>) { | ||
| let next = gen_sig.peek().wrapping_add(1); | ||
| gen_sig.set(next); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hold-to-repeat extraction looks good.
The two-helper pattern with a per-field generation signal is sound: the immediate tick() runs synchronously before the gen bump, so any in-flight task with the previous generation cleanly exits on its next peek check (no double-tick is possible under Dioxus' single-threaded async runtime). Cancellation via wrapping_add means at most one extra tick can fire after release (within the 100 ms interval), which is acceptable.
Optional nit: the loop { if … break; … } could be expressed more idiomatically as a while to make the cancellation predicate self-evident.
♻️ Optional refactor
spawn(async move {
crate::utils::sleep_ms(HOLD_INITIAL_DELAY_MS).await;
- loop {
- if *gen_sig.peek() != next_gen {
- break;
- }
+ while *gen_sig.peek() == next_gen {
tick();
crate::utils::sleep_ms(HOLD_REPEAT_INTERVAL_MS).await;
}
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/session_exercise_form.rs` around lines 9 - 38, Refactor the
spawned repeat loop in start_hold_repeat to use a while loop with the
cancellation predicate instead of loop { if … break; … } for clarity: inside the
async move block replace the current loop/if-break pattern with while
*gen_sig.peek() == next_gen { tick();
crate::utils::sleep_ms(HOLD_REPEAT_INTERVAL_MS).await; } so the cancellation
condition is self-evident (keep the existing tick(), sleep, and use of next_gen
as-is).
| // Cancel token: set to `true` when this component unmounts so any pending | ||
| // `spawn_local` duration notification is discarded. | ||
| let cancel = use_cancel_token(); | ||
| // Schedule a precise one-shot notification (WASM only; native uses tick). | ||
| use_effect(move || { | ||
| schedule_duration_notification(exercise_start, last_duration, duration_bell_rung); | ||
| }); | ||
| // Only run when exercise_start or last_duration changes to avoid spawning | ||
| // duplicate tasks on re-renders. | ||
| use_effect(use_reactive!(|(exercise_start, last_duration)| { | ||
| schedule_duration_notification( | ||
| exercise_start, | ||
| last_duration, | ||
| duration_bell_rung, | ||
| cancel.clone(), | ||
| ); | ||
| })); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the duplicated cancel-token + scheduling effect into a shared hook.
Lines 158–171 (ExerciseElapsedTimer) and 223–237 (InlineExerciseTimer) are byte-for-byte identical. Per the coding guidelines (idiomatic Rust, DRY), pull this into a helper hook so future changes — e.g., the per-effect-cancel fix above — only need to be made in one place.
♻️ Proposed helper
/// Wires the one-shot duration notification with unmount cancellation,
/// rescheduling only when `exercise_start` or `last_duration` changes.
fn use_duration_notification(
exercise_start: Option<u64>,
last_duration: Option<u64>,
duration_bell_rung: Signal<bool>,
) {
let cancel = use_cancel_token();
use_effect(use_reactive!(|(exercise_start, last_duration)| {
schedule_duration_notification(
exercise_start,
last_duration,
duration_bell_rung,
cancel.clone(),
);
}));
}Then both timer components collapse to a single call:
- let cancel = use_cancel_token();
- use_effect(use_reactive!(|(exercise_start, last_duration)| {
- schedule_duration_notification(
- exercise_start,
- last_duration,
- duration_bell_rung,
- cancel.clone(),
- );
- }));
+ use_duration_notification(exercise_start, last_duration, duration_bell_rung);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/session_timers.rs` around lines 158 - 171, Extract the
duplicated cancel-token + scheduling effect into a shared hook called e.g.
use_duration_notification and replace the inline effect in both
ExerciseElapsedTimer and InlineExerciseTimer with a single call to that hook;
implement use_duration_notification to call use_cancel_token(), then
use_effect(use_reactive!(|(exercise_start, last_duration)| {
schedule_duration_notification(exercise_start, last_duration,
duration_bell_rung, cancel.clone()); })), and keep the same parameter types
(exercise_start: Option<u64>, last_duration: Option<u64>, duration_bell_rung:
Signal<bool>) so existing calls to schedule_duration_notification,
use_cancel_token, use_effect and use_reactive continue to work without other
changes.
| use_effect(use_reactive!(|(exercise_start, last_duration)| { | ||
| schedule_duration_notification( | ||
| exercise_start, | ||
| last_duration, | ||
| duration_bell_rung, | ||
| cancel.clone(), | ||
| ); | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Dioxus 0.7 use_future use_reactive cancellation on dependency change and on unmount
💡 Result:
In Dioxus 0.7: use_future: - Cancellation on unmount: Yes, futures are automatically cancelled when the component unmounts (like all spawn-based tasks). - Cancellation on dependency change: No automatic behavior. use_future is non-reactive and runs once on mount. For dependency-driven restarts, manually call restart (which cancels the current future first) or use use_resource instead. - Manual control: UseFuture provides cancel, restart (cancels + new future), pause, resume. use_reactive: - This hook makes non-reactive data reactive by returning a closure that tracks dependencies when called in reactive contexts (e.g., inside use_memo or use_effect). - It does not directly handle async/cancellation. Use it to make props reactive, e.g., in combination with use_resource: use_resource(use_reactive!(|(dep,)| async { ... })) where dep changes trigger rerun + cancellation. Key quotes: - "Futures can be cancelled before they complete... automatically be cancelled when the component is unmounted." - use_future restart: "Cancel the current future and spawn a new one." - use_resource: "Previous futures are automatically cancelled when restarting" on dependency changes. For reactive async with auto-cancel on deps: prefer use_resource over use_future + deps.
Citations:
- 1: https://dioxuslabs.com/learn/0.7/essentials/basics/async/
- 2: cancelling & restarting TextStream using
use_future()leaks connections & breaks down if you do it really fast DioxusLabs/dioxus#3300 - 3: https://docs.rs/dioxus/latest/dioxus/prelude/fn.use_future.html
- 4: https://dioxuslabs.com/learn/0.7/essentials/basics/effects/
- 5: https://docs.rs/dioxus-hooks/latest/dioxus_hooks/fn.use_reactive.html
- 6: https://docs.rs/dioxus-hooks/latest/dioxus_hooks/struct.UseFuture.html
- 7: https://github.com/DioxusLabs/dioxus/blob/cece12b5/packages/hooks/src/use_future.rs
- 8: https://mintlify.com/DioxusLabs/dioxus/async/futures
- 9: https://www.mintlify.com/DioxusLabs/dioxus/api/hooks/use-resource
- 10: https://dioxuslabs.com/learn/0.7/essentials/basics/resources/
🏁 Script executed:
head -n 250 src/components/session_timers.rs | tail -n +150Repository: gfauredev/LogOut
Length of output: 3940
🏁 Script executed:
rg -n "schedule_duration_notification" src/components/session_timers.rs | head -20Repository: gfauredev/LogOut
Length of output: 186
🏁 Script executed:
sed -n '47,120p' src/components/session_timers.rsRepository: gfauredev/LogOut
Length of output: 3030
Effect re-run leaks the previous spawn_local task and can fire a stale notification.
use_cancel_token only flips on component unmount (via use_drop), not on effect re-execution. When exercise_start or last_duration changes during the component's lifetime, use_effect re-runs and schedule_duration_notification spawns a new gloo_timers future, but the previously-spawned future is still pending against the old deadline. The shared cancel token remains false, so whichever task wakes up first will pass the cancel.load(...) check, set duration_bell_rung, and send a notification at the outdated time. The duration_bell_rung.peek() guard then suppresses the correct one. Same root cause applies to the effect in InlineExerciseTimer.
Two idiomatic fixes (per coding guidelines, prefer modern Dioxus patterns):
- Use a per-effect cancel token: store an
Arc<AtomicBool>in aSignal/use_hook, flip the previous one before spawning the new one inside the reactive closure (in addition to flipping on unmount). - Replace
spawn_localwithuse_resource(use_reactive!(...))so Dioxus automatically cancels the in-flight task on dependency change and on unmount, eliminating the cancel token entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/session_timers.rs` around lines 164 - 171, The effect spawns a
gloo_timers task that isn’t canceled on effect re-run, causing stale
notifications; update the reactive closure around use_effect/use_reactive! that
calls schedule_duration_notification so each re-run cancels the previous task:
either (A) create a per-effect cancel token (e.g., a Signal/Arc<AtomicBool>
stored via a use_hook or Signal) and flip it before spawning the new task and
still flip on unmount (replacing the shared cancel used by
schedule_duration_notification), or (B, preferred) convert the logic to
use_resource(use_reactive!(...)) for schedule_duration_notification so Dioxus
auto-cancels in-flight tasks on dependency changes and unmount; apply the same
fix to InlineExerciseTimer and remove reliance on the global use_cancel_token
cancel flag for these effects.
Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/0fa7bfd2-30f6-431b-8cc3-ae8db43b741e
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