Copilot/display exercises logs inline#145
Conversation
Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/7b49756a-799a-4ee3-8af7-ce72e9526653 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/7b49756a-799a-4ee3-8af7-ce72e9526653 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/7b49756a-799a-4ee3-8af7-ce72e9526653 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdd pointer gestures to completed exercise tiles: swipe-right to edit, tap to replay (when allowed), swipe-left then hold to delete with visual progress; update tile RSX and SCSS for layout/delete-progress; show a conditional gesture hint with EN/ES/FR translations. ChangesGesture-based completed exercise log interactions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 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: 3774/5029 (75.04474050507059%) ⏱️ Tests: 255 tests in 0.590s
|
![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
🤖 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 `@src/components/completed_exercise_log.rs`:
- Around line 91-207: The gesture-only handlers
(onpointerdown/onpointermove/onpointerup/onpointerleave/onpointercancel) make
replay/edit/delete unreachable to keyboard users; add accessible alternatives by
introducing visible or visually-hidden focusable controls (e.g., buttons or
links) in the tile header that call start_edit and on_replay and invoke the same
delete flow, and add keyboard event handlers (onkeydown/onkeyup or onkeypress)
on the article or those controls to trigger edit (Enter/Space), replay
(Enter/Space), and start a confirmable delete (Delete/Backspace or a focused
“Delete” button) while preserving the existing state variables
(delete_armed/delete_progress/delete_hold_gen/pointer_*); also add appropriate
ARIA attributes/roles and aria-labels to these controls so non-pointer users can
discover and perform the same actions as pointer gestures.
- Around line 101-103: Tap detection only uses horizontal slop (pointer_start_x
and drag_delta_x), allowing vertical scrolls with little horizontal drift to
trigger on_replay; add vertical tracking by introducing pointer_start_y and
drag_delta_y, set pointer_start_y to Some(evt.client_coordinates().y) and
drag_delta_y.set(0.0) at the same places where pointer_start_x and drag_delta_x
are initialized (also clear/reset them where delete_armed is cleared), and
update the tap/replay condition to require both horizontal and vertical deltas
be below the tap threshold before calling on_replay (apply same change for the
other occurrence referenced around lines 177-179).
- Around line 139-141: The async delete path currently clones session.read()
into current_session and calls current_session.exercise_logs.remove(idx), which
can panic or remove the wrong entry if logs changed; instead, validate the index
against current_session.exercise_logs.len() before calling remove, or better:
locate the log to delete by a stable identifier (e.g., log.id) using
position/find and remove_by_ident (or retain/filter to drop matching id) to
ensure identity-based removal; after safely removing the intended entry, call
storage::save_session(current_session). Ensure all references to session.read(),
current_session, exercise_logs, remove(idx), and storage::save_session are
updated accordingly.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ac2af03-6134-4ceb-8d3d-be2293d08720
📒 Files selected for processing (6)
assets/_component.scssassets/en.ftlassets/es.ftlassets/fr.ftlsrc/components/active_session/completed_exercises.rssrc/components/completed_exercise_log.rs
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 3 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
|
@copilot Keep the previous button-based UI for pointer devices, use this new gesture UI for touchscreens only |
Fixed 1 file(s) based on 3 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
📊 Coverage ReportLines: / (%) ⏱️ Tests: tests in s |
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)
src/components/completed_exercise_log.rs (2)
120-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the swipe/hold logic to touch input only.
These handlers currently run for every pointer type, so desktop mouse drags can now shift the tile and trigger edit/delete as well. That conflicts with the explicit PR requirement to keep the previous button-based UI for pointer devices and reserve the new gesture flow for touchscreens.
🤖 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 `@src/components/completed_exercise_log.rs` around lines 120 - 244, The handlers implement swipe/hold for all pointer types; restrict them to touch-only by tracking pointer type: in onpointerdown (the move |evt| closure) check evt.pointer_type() == "touch" and set a new flag (e.g., pointer_is_touch.set(true)) only for touch pointers, and early-return for non-touch; in onpointerup/onpointerleave/onpointercancel and onpointermove, short-circuit the logic unless pointer_is_touch.read() is true (and clear pointer_is_touch.set(false) on up/leave/cancel). Update references in the existing closures (onpointerdown, onpointermove, onpointerup, onpointerleave, onpointercancel) and coordinate with pointer_down, delete_hold_gen, delete_armed, and delete_progress to ensure state is only modified for touch-driven gestures.
189-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRead
dybefore clearing the drag state.Line 196 resets
drag_delta_ybefore Line 212 reads it, so the vertical tap guard always sees0and a mostly vertical scroll can still replay the log. Capturedyalongsidedxbefore resetting the gesture signals.Proposed fix
let dx = *drag_delta_x.read(); + let dy = *drag_delta_y.read(); let armed_delete = *delete_armed.read(); let completed_delete = *delete_progress.read() >= 1.0; pointer_down.set(false); pointer_start_x.set(None); pointer_start_y.set(None); drag_delta_x.set(0.0); drag_delta_y.set(0.0); @@ - let dy = *drag_delta_y.read(); if show_replay && dx.abs() <= TAP_SLOP_PX && dy.abs() <= TAP_SLOP_PX { on_replay.call(()); }🤖 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 `@src/components/completed_exercise_log.rs` around lines 189 - 213, The code clears drag state before reading vertical delta, so dy is always zero; change the gesture reset sequence in completed_exercise_log.rs to read both dx and dy from drag_delta_x.read() and drag_delta_y.read() into local variables (e.g., let dx = *drag_delta_x.read(); let dy = *drag_delta_y.read();) before mutating pointer_down, pointer_start_x/y, drag_delta_x/y, delete_armed, delete_progress, and delete_hold_gen; then use those captured dx and dy in the subsequent checks (completed_delete, armed_delete, swipe/edit guard, and the show_replay vertical tap guard) so vertical taps are detected correctly.
🤖 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 `@src/components/completed_exercise_log.rs`:
- Line 92: The code uses a non-existent identifier on the model (ExerciseLog ->
log.id / l.id), so the delete paths fail to compile; either restore the previous
bounds-checked removal using remove(idx) (replace uses of log.id / l.id with
index-based deletion where variable names like log or l are used) or add a
stable id field to the ExerciseLog model and update every delete path that
references log.id / l.id to use that new id consistently (ensure the model
definition, any serialization/migrations, and all three delete call sites are
changed together).
- Around line 263-285: The visible button labels ("Edit", "Replay", "Delete")
are hard-coded while only their aria-labels use localization; update the button
contents to use the same translation macro calls (t!()) as the aria-labels so
the rendered text is localized. Locate the button blocks in
completed_exercise_log.rs where is_editing, start_edit, show_replay, on_replay,
toast, session, storage::save_session and log_id are used and replace the
literal strings with the appropriate t!("edit-log"), t!("log-replay-title") and
t!("delete-log") (or the correct keys) so both the visible label and aria-label
are translated.
- Around line 98-118: The current key handling matches on evt.key().as_str();
change it to match on the Key enum returned by evt.key() (use Key::Enter,
Key::Space, Key::Character(c) with c == "e" or c == "E" to trigger start_edit,
and Key::Delete | Key::Backspace to perform the delete logic that pushes the
toast, removes the log from session.exercise_logs using log_id, and calls
storage::save_session). Keep the existing calls to on_replay.call(),
start_edit(()), toast.write().push_back(...), and storage::save_session(...) but
move them into the appropriate Key variant arms to be type-safe and idiomatic.
---
Outside diff comments:
In `@src/components/completed_exercise_log.rs`:
- Around line 120-244: The handlers implement swipe/hold for all pointer types;
restrict them to touch-only by tracking pointer type: in onpointerdown (the move
|evt| closure) check evt.pointer_type() == "touch" and set a new flag (e.g.,
pointer_is_touch.set(true)) only for touch pointers, and early-return for
non-touch; in onpointerup/onpointerleave/onpointercancel and onpointermove,
short-circuit the logic unless pointer_is_touch.read() is true (and clear
pointer_is_touch.set(false) on up/leave/cancel). Update references in the
existing closures (onpointerdown, onpointermove, onpointerup, onpointerleave,
onpointercancel) and coordinate with pointer_down, delete_hold_gen,
delete_armed, and delete_progress to ensure state is only modified for
touch-driven gestures.
- Around line 189-213: The code clears drag state before reading vertical delta,
so dy is always zero; change the gesture reset sequence in
completed_exercise_log.rs to read both dx and dy from drag_delta_x.read() and
drag_delta_y.read() into local variables (e.g., let dx = *drag_delta_x.read();
let dy = *drag_delta_y.read();) before mutating pointer_down, pointer_start_x/y,
drag_delta_x/y, delete_armed, delete_progress, and delete_hold_gen; then use
those captured dx and dy in the subsequent checks (completed_delete,
armed_delete, swipe/edit guard, and the show_replay vertical tap guard) so
vertical taps are detected correctly.
🪄 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: 60bb0c99-4f11-44f7-877b-75750c0e0904
📒 Files selected for processing (1)
src/components/completed_exercise_log.rs
| onkeydown: move |evt| { | ||
| if *is_editing.read() { | ||
| return; | ||
| } | ||
| match evt.key().as_str() { | ||
| "Enter" | " " => { | ||
| if show_replay { | ||
| on_replay.call(()); | ||
| } | ||
| } | ||
| button { | ||
| class: "edit", | ||
| onclick: start_edit, | ||
| title: t!("log-edit-title"), | ||
| "✏️" | ||
| "e" | "E" => { | ||
| start_edit(()); | ||
| } | ||
| HoldDeleteButton { | ||
| title: t!("log-delete-title").to_string(), | ||
| on_delete: move |()| { | ||
| consume_context::<crate::ToastSignal>() | ||
| .0 | ||
| .write() | ||
| .push_back(t!("toast-log-deleted").to_string()); | ||
| "Delete" | "Backspace" => { | ||
| toast.write().push_back(t!("toast-log-deleted").to_string()); | ||
| let mut current_session = session.read().clone(); | ||
| current_session.exercise_logs.retain(|l| l.id != log_id); | ||
| storage::save_session(current_session); | ||
| } | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Dioxus 0.7.9, what type does KeyboardEvent::key()return, and what is the idiomatic way to handle Enter, Space, Delete, Backspace, and character keys like "e" in anonkeydown handler?
💡 Result:
In Dioxus 0.7.9, the key() method on KeyboardEvent returns a Key enum [1][2]. The idiomatic way to handle keyboard input is to use the onkeydown event handler and match on the value returned by evt.key() [3][4]. The Key type provides a structured, type-safe way to represent keyboard input, which is preferred over manual string matching [1]. Below is the idiomatic approach to handling specific keys: rsx! { div { tabindex: 0, onkeydown: move |evt| { use dioxus::prelude::Key; match evt.key { Key::Enter => println!("Enter pressed"), Key::Space => println!("Space pressed"), Key::Backspace => println!("Backspace pressed"), Key::Delete => println!("Delete pressed"), Key::Character(c) if c == "e" => println!("'e' pressed"), _ => {} } } } } Key Handling Tips: 1. Identification: Use the Key enum for named keys (e.g., Key::Enter, Key::Backspace) [1]. For character inputs, match against Key::Character(s) [1]. 2. Focus: Remember that onkeydown events only fire on focusable elements (like input, textarea, or elements with a tabindex attribute) [4]. 3. Physical Keys vs. Input: If you need to detect the physical location of a key regardless of the user's keyboard layout, you can use evt.code() instead of evt.key(), which returns a Code enum [1]. The KeyboardEvent in Dioxus is a type alias for Event<KeyboardData> [2], and the KeyboardData struct exposes both key() (for the logical character/function) and code() (for the physical key position) [1].
Citations:
- 1: https://docs.rs/dioxus/latest/dioxus/events/struct.KeyboardData.html
- 2: https://docs.rs/dioxus/latest/dioxus/events/type.KeyboardEvent.html
- 3: https://dioxuslabs.com/learn/0.7/essentials/basics/event_handlers/
- 4: https://stackoverflow.com/questions/74833748/how-can-i-add-a-global-keydown-event-listener-in-dioxus
🏁 Script executed:
# First, check if the file exists and view the relevant code
fd "completed_exercise_log.rs" --type fRepository: gfauredev/LogOut
Length of output: 101
🏁 Script executed:
# Search for KeyboardEvent and key() usage patterns in the codebase
rg "evt\.key\(\)" --type rs -A 3 -B 3Repository: gfauredev/LogOut
Length of output: 87
🏁 Script executed:
# Find and display the relevant code section from the file
cat -n src/components/completed_exercise_log.rs | sed -n '90,125p'Repository: gfauredev/LogOut
Length of output: 1673
🏁 Script executed:
# Search for evt.key usage with correct rg syntax
rg "evt\.key" --type-add='rs:*.rs' -A 3 -B 3Repository: gfauredev/LogOut
Length of output: 2539
🏁 Script executed:
# Check Dioxus version in Cargo.toml
cat Cargo.toml | grep -A 5 "dioxus"Repository: gfauredev/LogOut
Length of output: 615
🏁 Script executed:
# Check imports in completed_exercise_log.rs to see if Key is imported
head -n 30 src/components/completed_exercise_log.rsRepository: gfauredev/LogOut
Length of output: 1282
🏁 Script executed:
# Check the session_exercise_form.rs to see how Key is imported
head -n 30 src/components/session_exercise_form.rs | grep -A 20 "use"Repository: gfauredev/LogOut
Length of output: 1187
🏁 Script executed:
# Check if Key is imported in completed_exercise_log.rs
rg "Key" src/components/completed_exercise_log.rsRepository: gfauredev/LogOut
Length of output: 42
🏁 Script executed:
# Check entire imports section of completed_exercise_log.rs
head -n 50 src/components/completed_exercise_log.rsRepository: gfauredev/LogOut
Length of output: 2242
🏁 Script executed:
# Check if the code actually compiles by looking for build output or test results
ls -la | grep -E "(Cargo.lock|target|build)"Repository: gfauredev/LogOut
Length of output: 184
🏁 Script executed:
# Check if the project actually builds with dx build for wasm
cd . && cat Cargo.toml | head -20Repository: gfauredev/LogOut
Length of output: 715
🏁 Script executed:
# Check if there are any other files using evt.key().as_str() to see if it's a widespread pattern
rg "\.as_str\(\)" src/components/ -A 2 -B 2Repository: gfauredev/LogOut
Length of output: 3363
🏁 Script executed:
# Look for any compilation errors or warnings in target directory
find target -name "*.rmeta" -o -name "*.rlib" | head -5Repository: gfauredev/LogOut
Length of output: 341
🏁 Script executed:
# Check if there's a Key import or alias somewhere
rg "Key" src/components/completed_exercise_log.rs -B 5 -A 5Repository: gfauredev/LogOut
Length of output: 42
🏁 Script executed:
# Check the session_exercise_form.rs to see if Key is imported there
head -n 15 src/components/session_exercise_form.rsRepository: gfauredev/LogOut
Length of output: 675
🏁 Script executed:
# Look at all imports in both files more carefully
grep -n "use" src/components/completed_exercise_log.rs | head -20Repository: gfauredev/LogOut
Length of output: 1048
🏁 Script executed:
# Check if dioxus-0.7.9 docs are available locally
find . -name "*.md" -o -name "CHANGELOG*" | grep -i dioxus | head -5Repository: gfauredev/LogOut
Length of output: 42
🏁 Script executed:
# Try to find any test files or examples that show key handling patterns
find . -type f -name "*.rs" -exec grep -l "Key::" {} \;Repository: gfauredev/LogOut
Length of output: 102
🏁 Script executed:
# Check the session_exercise_form.rs file where Key:: is used
grep -n "Key::" src/components/session_exercise_form.rs -B 5 -A 5Repository: gfauredev/LogOut
Length of output: 1887
🏁 Script executed:
# Also check for the exact line where Key is matched
grep -n "evt.key()" src/components/session_exercise_form.rs -B 2 -A 2Repository: gfauredev/LogOut
Length of output: 984
🏁 Script executed:
# Find where Key is imported in session_exercise_form.rs
head -n 30 src/components/session_exercise_form.rs | grep -E "use|Key"Repository: gfauredev/LogOut
Length of output: 250
🏁 Script executed:
# Check if there are compile errors or warnings in the project
cat Cargo.lock | grep -A 2 "name = \"dioxus\"" | head -10Repository: gfauredev/LogOut
Length of output: 159
🏁 Script executed:
# Look at any test or example files that might show the correct pattern
find . -type f -name "*.rs" -path "*/examples/*" -o -path "*/tests/*" 2>/dev/null | head -5Repository: gfauredev/LogOut
Length of output: 42
Match on the Key enum directly instead of calling .as_str().
In Dioxus 0.7.9, evt.key() returns a Key enum, and the idiomatic approach is to match directly on the enum variants rather than converting to a string. This is demonstrated in session_exercise_form.rs (lines 244, 317, 384) which uses if evt.key() == Key::Enter.
Update the match statement to use:
match evt.key {
Key::Enter | Key::Space => { /* replay */ },
Key::Character(c) if c == "e" => { /* edit */ },
Key::Delete | Key::Backspace => { /* delete */ },
_ => {}
}This pattern is more type-safe and aligns with idiomatic Dioxus event handling across the codebase.
🧰 Tools
🪛 GitHub Actions: .github/workflows/ci.yml / 0_android-build.txt
[error] 102-102: Rust compiler error E0599: no method named as_str found for enum dioxus::prelude::Key in the current scope (method exists on std::string::String).
[error] 114-114: Rust compiler error E0609: no field id on type &log::ExerciseLog when calling current_session.exercise_logs.retain(|l| l.id != log_id).
🪛 GitHub Actions: .github/workflows/ci.yml / 1_coverage.txt
[error] 114-114: Rust compile error (E0609): no field id on type &log::ExerciseLog. Code: current_session.exercise_logs.retain(|l| l.id != log_id);
🪛 GitHub Actions: .github/workflows/ci.yml / android-build
[error] 102-102: Rust compile error E0599: no method named as_str found for enum dioxus::prelude::Key in the current scope.
[error] 114-114: Rust compile error E0609: no field id on type &log::ExerciseLog (unknown field).
🪛 GitHub Actions: .github/workflows/ci.yml / coverage
[error] 114-114: Rust compile error (E0609): no field id on type &log::ExerciseLog at current_session.exercise_logs.retain(|l| l.id != log_id);. Available fields include exercise_id, exercise_name, category, start_time, end_time, etc.
🤖 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 `@src/components/completed_exercise_log.rs` around lines 98 - 118, The current
key handling matches on evt.key().as_str(); change it to match on the Key enum
returned by evt.key() (use Key::Enter, Key::Space, Key::Character(c) with c ==
"e" or c == "E" to trigger start_edit, and Key::Delete | Key::Backspace to
perform the delete logic that pushes the toast, removes the log from
session.exercise_logs using log_id, and calls storage::save_session). Keep the
existing calls to on_replay.call(), start_edit(()),
toast.write().push_back(...), and storage::save_session(...) but move them into
the appropriate Key variant arms to be type-safe and idiomatic.
| if !*is_editing.read() { | ||
| div { class: "log-actions", | ||
| button { | ||
| "aria-label": t!("edit-log"), | ||
| onclick: move |_| start_edit(()), | ||
| "Edit" | ||
| } | ||
| if show_replay { | ||
| button { | ||
| "aria-label": t!("log-replay-title"), | ||
| onclick: move |_| on_replay.call(()), | ||
| "Replay" | ||
| } | ||
| } | ||
| button { | ||
| "aria-label": t!("delete-log"), | ||
| onclick: move |_| { | ||
| toast.write().push_back(t!("toast-log-deleted").to_string()); | ||
| let mut current_session = session.read().clone(); | ||
| current_session.exercise_logs.retain(|l| l.id != log_id); | ||
| storage::save_session(current_session); | ||
| }, | ||
| "Delete" |
There was a problem hiding this comment.
Localise the visible action labels as well.
The new buttons translate their aria-labels but still render hard-coded "Edit", "Replay", and "Delete". In non-English locales that leaves the accessible name translated whilst the visible UI regresses to English.
🧰 Tools
🪛 GitHub Actions: .github/workflows/ci.yml / 0_android-build.txt
[error] 282-282: Rust compiler error E0609: no field id on type &log::ExerciseLog when calling current_session.exercise_logs.retain(|l| l.id != log_id).
🪛 GitHub Actions: .github/workflows/ci.yml / android-build
[error] 282-282: Rust compile error E0609: no field id on type &log::ExerciseLog (unknown field).
🤖 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 `@src/components/completed_exercise_log.rs` around lines 263 - 285, The visible
button labels ("Edit", "Replay", "Delete") are hard-coded while only their
aria-labels use localization; update the button contents to use the same
translation macro calls (t!()) as the aria-labels so the rendered text is
localized. Locate the button blocks in completed_exercise_log.rs where
is_editing, start_edit, show_replay, on_replay, toast, session,
storage::save_session and log_id are used and replace the literal strings with
the appropriate t!("edit-log"), t!("log-replay-title") and t!("delete-log") (or
the correct keys) so both the visible label and aria-label are translated.
Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/a6444465-e6d7-41c1-8865-621558c901aa Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/components/completed_exercise_log.rs (2)
137-142:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid index-based removal after the hold delay.
Both delete completions still remove by
idxafter a hold interval. Ifexercise_logschanges before the callback fires, this can panic or delete the wrong log. Remove by a stable identity captured fromlog; a bounds check only covers the panic case.Also applies to: 286-291
🤖 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 `@src/components/completed_exercise_log.rs` around lines 137 - 142, The callback removes an entry by the captured idx which can be invalid after the hold delay; instead capture a stable identifier from the log (e.g., log.id or another unique key) when setting up the callback and, inside the closure that currently reads session_for_touch_delete and calls current_session.exercise_logs.remove(idx), locate the log by matching that identifier (e.g., find_position(|l| l.id == captured_id)) and remove by position only if found, then call storage::save_session with the updated session; apply the same change to the other delete callback that also uses idx (the similar block mentioned in the comment).
41-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrack vertical travel before treating a touch as replay.
This still classifies a near-vertical scroll as a tap whenever the horizontal drift stays within
TAP_SLOP_PX, so browsing the log list can fireon_replayaccidentally. Record Y as well and require both axes to stay within the tap slop before replaying.Also applies to: 89-96, 105-113, 205-206
🤖 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 `@src/components/completed_exercise_log.rs` around lines 41 - 43, The touch handling only tracks horizontal movement (touch_start_x, drag_delta_x) so near-vertical scrolls can still be treated as taps; add symmetric vertical tracking (e.g., touch_start_y: use_signal(|| None::<f64>) and drag_delta_y: use_signal(|| 0.0f64)), update touchstart/touchmove/touchend handlers that currently read/write touch_start_x, touch_active, and drag_delta_x to also record and compute Y deltas, and change the tap/replay check to require both |drag_delta_x| and |drag_delta_y| to be <= TAP_SLOP_PX before firing on_replay; apply the same changes in the other touch handler blocks that mirror the logic (the blocks referenced around the other occurrences using touch_start_x/drag_delta_x).assets/_component.scss (1)
303-317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not hide the only focusable action controls on coarse pointers.
This media query makes replay/edit/delete gesture-only again on touch devices by setting
.log-pointer-actionstodisplay: none. That drops the fallback controls for keyboard, switch, and screen-reader users on phones and tablets with external input. Keep a focusable control path available here.🤖 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 `@assets/_component.scss` around lines 303 - 317, The rule that sets .log-pointer-actions to display: none for coarse pointers removes the only focusable controls on touch devices; instead of display:none, replace that with a visual-hide technique that preserves focusability (e.g., apply your project’s .sr-only/visually-hidden utility or the clip/absolute 1px technique) so keyboard/switch/screen-reader users still can tab to .log-pointer-actions while keeping it visually hidden on coarse pointers; keep .log-delete-progress and p.log-gestures-hint behavior as-is and verify .log-pointer-actions remains in the DOM and reachable by keyboard/screen readers.
🤖 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.
Duplicate comments:
In `@assets/_component.scss`:
- Around line 303-317: The rule that sets .log-pointer-actions to display: none
for coarse pointers removes the only focusable controls on touch devices;
instead of display:none, replace that with a visual-hide technique that
preserves focusability (e.g., apply your project’s .sr-only/visually-hidden
utility or the clip/absolute 1px technique) so keyboard/switch/screen-reader
users still can tab to .log-pointer-actions while keeping it visually hidden on
coarse pointers; keep .log-delete-progress and p.log-gestures-hint behavior
as-is and verify .log-pointer-actions remains in the DOM and reachable by
keyboard/screen readers.
In `@src/components/completed_exercise_log.rs`:
- Around line 137-142: The callback removes an entry by the captured idx which
can be invalid after the hold delay; instead capture a stable identifier from
the log (e.g., log.id or another unique key) when setting up the callback and,
inside the closure that currently reads session_for_touch_delete and calls
current_session.exercise_logs.remove(idx), locate the log by matching that
identifier (e.g., find_position(|l| l.id == captured_id)) and remove by position
only if found, then call storage::save_session with the updated session; apply
the same change to the other delete callback that also uses idx (the similar
block mentioned in the comment).
- Around line 41-43: The touch handling only tracks horizontal movement
(touch_start_x, drag_delta_x) so near-vertical scrolls can still be treated as
taps; add symmetric vertical tracking (e.g., touch_start_y: use_signal(||
None::<f64>) and drag_delta_y: use_signal(|| 0.0f64)), update
touchstart/touchmove/touchend handlers that currently read/write touch_start_x,
touch_active, and drag_delta_x to also record and compute Y deltas, and change
the tap/replay check to require both |drag_delta_x| and |drag_delta_y| to be <=
TAP_SLOP_PX before firing on_replay; apply the same changes in the other touch
handler blocks that mirror the logic (the blocks referenced around the other
occurrences using touch_start_x/drag_delta_x).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: af23e192-0544-4f0c-b365-84d6c2003e14
📒 Files selected for processing (2)
assets/_component.scsssrc/components/completed_exercise_log.rs
Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/a6444465-e6d7-41c1-8865-621558c901aa Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
Done in |
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
Localization
Style