Copilot/add daily volume average weight selectors#143
Merged
gfauredev merged 12 commits intoMay 7, 2026
Conversation
…vg weight metrics Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/7cdbcc43-a72a-4b79-8557-de1d62900618 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
…tion Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/7cdbcc43-a72a-4b79-8557-de1d62900618 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
…geDailyWeight→AverageSessionWeight Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/00d6bd52-a26e-4687-b142-85e220950f97 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
…c match arms Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/00d6bd52-a26e-4687-b142-85e220950f97 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds a Volume metric and AnalyticsMode, expands analytics availability from 4 → 5 metric slots, updates chart layout to optionally render a third chart region, implements mode-dependent aggregation (Set / SessionAverage / SessionTotal) including a computed Volume series, updates selector types, makes ATH UI cells clickable, and adds related translations and styles. ChangesAnalytics core, charting, selector, and models
UX, forms, styles, translations
sequenceDiagram
actor User
participant SessionForm as Session Exercise Form
participant MetricSelector as Metric Selector
participant Analytics as Analytics Component
participant Chart as Chart Renderer
User->>SessionForm: Click ATH value (weight/distance/reps/duration)
SessionForm->>SessionForm: Convert units (HG_PER_KG / M_PER_KM)
SessionForm->>SessionForm: Populate input field
User->>MetricSelector: Select metric / mode (e.g., Volume, SessionAverage)
MetricSelector->>Analytics: Signal metric/mode change
Analytics->>Analytics: Evaluate available_by_metric (5 slots)
Analytics->>Analytics: Aggregate points per AnalyticsMode (Set / SessionAverage / SessionTotal)
Analytics->>Analytics: Compute Volume = weight_kg * reps where applicable
Analytics->>Chart: Send chart_data with 5-metric mapping
Chart->>Chart: Route metric indices to chart 1/2/3, compute axis scales
Chart->>Chart: Render series, axes, ticks, and cursor guides across charts
Chart->>User: Display updated analytics with Volume and selected mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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/5005 (75.06493506493507%) ⏱️ Tests: 255 tests in 0.614s
|
![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: 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/analytics/chart.rs`:
- Around line 247-258: The loop over axis_data (for i in 0..7_usize) computes
axis side with is_right = i % 2 == 1 which causes chart 3 axes (indices 4 and 6)
to collide on the left; update the logic in that loop to detect the chart-3
collision case and handle it: if i == 6 and axis_data[4].is_some() then either
force is_right = true for index 6 (render SessionReps on the right) or compute a
slight inward offset for x_pos when both left-side axes for chart3 exist; modify
the x_pos calculation (and any downstream label/tick placement that uses x_pos)
so ticks/labels for index 6 are drawn without overlapping axis 4.
In `@src/components/session_exercise_form.rs`:
- Around line 269-276: The three span.ath elements (weight, distance, reps) and
the time.ath element need keyboard accessibility: update each span bearing class
"ath" (e.g., the span that calls weight_input.set(...), the ones that set
distance_input and reps_input, and the time.ath span) to include tabindex: 0,
role: "button", and add an onkeydown handler that triggers the same fill action
when Enter or Space is pressed (mirroring the existing onkeydown pattern already
used elsewhere in this file). Ensure the onkeydown logic invokes the same setter
(weight_input.set(...), distance_input.set(...), reps_input.set(...),
time_input.set(...)) as the onclick so keyboard users can activate the ATH
shortcut.
- Around line 194-206: The time element for bests.duration is styled with class
"ath" and given an onclick that only acts when time_input is Some, causing a
misleading interactive affordance in view-only/perform modes; change the
rendering so the "ath" class and the onclick handler are only applied when
time_input.is_some(), i.e. detect time_input (the Option bound in this
component) and conditionally add class: "ath" and the onclick closure that calls
time_input.set(format_time(dur)) only when time_input is Some (otherwise render
a plain time element showing format_time(dur)), updating the code around
bests.duration / time and the usage of format_time and time_input accordingly.
🪄 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: cdffac86-c7de-46e7-bc82-b3e5263f7d28
📒 Files selected for processing (9)
assets/_component.scssassets/en.ftlassets/es.ftlassets/fr.ftlsrc/components/analytics/chart.rssrc/components/analytics/mod.rssrc/components/analytics/selector.rssrc/components/session_exercise_form.rssrc/models/analytics.rs
Comment on lines
194
to
206
| if let Some(dur) = bests.duration { | ||
| time { "{format_time(dur)}" } | ||
| time { | ||
| class: "ath", | ||
| onclick: move |_| { | ||
| if let Some(mut ti) = time_input { | ||
| ti.set(format_time(dur)); | ||
| } | ||
| }, | ||
| "{format_time(dur)}" | ||
| } | ||
| } else { | ||
| time { "–" } | ||
| } |
Contributor
There was a problem hiding this comment.
Duration ATH styled as interactive but silently no-ops when time_input is None.
The time element unconditionally receives class: "ath" (pointer cursor + dotted underline from the stylesheet), but its onclick body is guarded by if let Some(mut ti) = time_input { … }. In perform mode and view-only mode, time_input is None, so clicking the visually interactive element does nothing — a misleading affordance.
🐛 Proposed fix — conditionally apply the `ath` class
- time {
- class: "ath",
- onclick: move |_| {
- if let Some(mut ti) = time_input {
- ti.set(format_time(dur));
- }
- },
- "{format_time(dur)}"
- }
+ time {
+ class: if time_input.is_some() { "ath" } else { "" },
+ onclick: move |_| {
+ if let Some(mut ti) = time_input {
+ ti.set(format_time(dur));
+ }
+ },
+ "{format_time(dur)}"
+ }🤖 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 194 - 206, The time
element for bests.duration is styled with class "ath" and given an onclick that
only acts when time_input is Some, causing a misleading interactive affordance
in view-only/perform modes; change the rendering so the "ath" class and the
onclick handler are only applied when time_input.is_some(), i.e. detect
time_input (the Option bound in this component) and conditionally add class:
"ath" and the onclick closure that calls time_input.set(format_time(dur)) only
when time_input is Some (otherwise render a plain time element showing
format_time(dur)), updating the code around bests.duration / time and the usage
of format_time and time_input accordingly.
Comment on lines
+269
to
+276
| span { | ||
| class: "ath", | ||
| onclick: move |_| { | ||
| weight_input | ||
| .set(format!("{:.1}", f64::from(best.0) / HG_PER_KG)); | ||
| }, | ||
| "{best}" | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
ATH click-to-fill elements are unreachable via keyboard.
All three span.ath elements (weight, distance, reps) — and the time.ath element at Line 195 — lack tabindex, role="button", and an onkeydown handler. Since onclick does not fire on Enter/Space for non-interactive HTML elements without tabindex, keyboard-only users cannot trigger the fill-from-ATH shortcut.
Focusing is necessary to catch keyboard events on non-interactive elements, so tabindex: 0 is the minimum required. The pattern for onkeydown already exists in the file (e.g. Lines 243–247).
♻️ Illustrative fix for the weight ATH span (apply the same pattern to distance, reps, and time.ath)
if let Some(best) = bests.weight_hg {
span {
class: "ath",
+ tabindex: 0,
+ role: "button",
onclick: move |_| {
weight_input
.set(format!("{:.1}", f64::from(best.0) / HG_PER_KG));
},
+ onkeydown: move |evt| {
+ if evt.key() == Key::Enter {
+ weight_input
+ .set(format!("{:.1}", f64::from(best.0) / HG_PER_KG));
+ }
+ },
"{best}"
}Also applies to: 342-349, 409-415
🤖 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 269 - 276, The three
span.ath elements (weight, distance, reps) and the time.ath element need
keyboard accessibility: update each span bearing class "ath" (e.g., the span
that calls weight_input.set(...), the ones that set distance_input and
reps_input, and the time.ath span) to include tabindex: 0, role: "button", and
add an onkeydown handler that triggers the same fill action when Enter or Space
is pressed (mirroring the existing onkeydown pattern already used elsewhere in
this file). Ensure the onkeydown logic invokes the same setter
(weight_input.set(...), distance_input.set(...), reps_input.set(...),
time_input.set(...)) as the onclick so keyboard users can activate the ATH
shortcut.
…toggle (Set/SessionAverage/SessionTotal) and dedicated Volume chart Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/07d5a767-7b0e-4202-acdb-85424116769d Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/analytics/chart.rs`:
- Around line 82-86: chart3_top is always computed from chart2_bottom causing an
empty gap when has_chart3 is true but has_chart2 is false; change the logic to
position chart3 immediately after the last visible chart by computing chart3_top
from chart2_bottom when has_chart2 is true, otherwise from chart1_bottom (e.g.
chart3_top = if has_chart2 { chart2_bottom + x_gap } else { chart1_bottom +
x_gap }), then compute chart3_bottom = chart3_top + chart_height and set
total_height to chart3_bottom + chart2_bottom_margin when has_chart3.
In `@src/components/analytics/mod.rs`:
- Around line 65-90: The current mapping logic uses weight_hg.0 > 0 to decide
metric support, which misclassifies many exercises; replace those boolean checks
with the metric-driven predicate metric.extract_value(log).is_some() when
populating the maps array (the array initialized as maps and indexed per
Metric::to_index()), leaving Volume as the only special-case rule (keep Volume
populated when is_weighted && log.reps.is_some()); also update the corresponding
selectors/filters used by Set, SessionAverage, and SessionTotal to use the same
metric.extract_value(log).is_some() predicate so the selector and chart data
remain consistent.
🪄 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: 02a027db-f23b-4618-b9e4-bd14c31fa5da
📒 Files selected for processing (8)
assets/_unique.scssassets/en.ftlassets/es.ftlassets/fr.ftlsrc/components/analytics/chart.rssrc/components/analytics/mod.rssrc/components/analytics/selector.rssrc/models/analytics.rs
…e, simplify maps, add axis/mode docs Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/07d5a767-7b0e-4202-acdb-85424116769d Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
Copilot stopped work on behalf of
gfauredev due to an error
May 5, 2026 16:44
Copilot stopped work on behalf of
gfauredev due to an error
May 5, 2026 19:32
…ercises Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/ce75a3ab-7b9c-4b14-bbf1-aa931d8aada5 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
…order in session detail Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/ce75a3ab-7b9c-4b14-bbf1-aa931d8aada5 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
📊 Coverage ReportLines: 3774/5029 (75.04474050507059%) ⏱️ Tests: 255 tests in 0.560s
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
📊 Coverage ReportLines: 3774/5029 (75.04474050507059%) ⏱️ Tests: 255 tests in 0.571s
|
Flake lock file updates:
• Updated input 'crane':
'github:ipetkov/crane/7cf72d978629469c4bd4206b95c402514c1f6000?narHash=sha256-SPm9ck7jh3Un9nwPuMGbRU04UroFmOHjLP56T10MOeM%3D' (2026-04-10)
→ 'github:ipetkov/crane/d459c1350e96ce1a7e3859c513ef5e9869d67d6f?narHash=sha256-2uoQAqUk2H0ijQtGiWAyNeQYGYc6yfAcRRLlJAz4Gp8%3D' (2026-05-03)
• Updated input 'nixpkgs':
'github:NixOS/nixpkgs/4c1018dae018162ec878d42fec712642d214fdfa?narHash=sha256-ar3rofg%2BawPB8QXDaFJhJ2jJhu%2BKqN/PRCXeyuXR76E%3D' (2026-04-09)
→ 'github:NixOS/nixpkgs/549bd84d6279f9852cae6225e372cc67fb91a4c1?narHash=sha256-hGdgeU2Nk87RAuZyYjyDjFL6LK7dAZN5RE9%2BhrDTkDU%3D' (2026-05-05)
• Updated input 'rust-overlay':
'github:oxalica/rust-overlay/3c27f4c92a7d977556dd2c10bb564d9c61b375e9?narHash=sha256-/f/6/1WOfBJaGMfqV3VxWD9lpFRbPpF%2BCx4MO%2B0mGok%3D' (2026-04-13)
→ 'github:oxalica/rust-overlay/adf987c76af8d17b8256d23631bcf203f81e1a63?narHash=sha256-EZnAOkPgEeOO2rCRhwkTvesCq/E6dbsyxhMyaefgIWM%3D' (2026-05-06)
📊 Coverage ReportLines: / (%) ⏱️ Tests: tests in s |
📊 Coverage ReportLines: 3774/5029 (75.04474050507059%) ⏱️ Tests: 255 tests in 0.584s
|
Copilot stopped work on behalf of
gfauredev due to an error
May 6, 2026 16:10
Agent-Logs-Url: https://github.com/gfauredev/LogOut/sessions/e35f3cc3-5bcf-47df-8b23-160682423ef9 Co-authored-by: gfauredev <19304085+gfauredev@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/analytics/chart.rs (1)
70-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty chart-1 region when only Volume is selected.
If no series in slots 0..7 has data but the dedicated Volume series does (
metric_has_data = [F,F,F,F,T]),has_chart2 = false,has_chart3 = true, but chart 1 has nohas_chart1gate: the unconditional baseline (lines 220-227) and cursor guide (lines 418-428) still render, andchart3_top = chart1_bottom + x_gapplaces the Volume chart below an empty ~342px chart-1 area. Mirrors the chart-2-absent case the previous review fixed.Suggest gating chart 1 the same way and collapsing higher charts upward when prior charts are empty:
💡 Proposed direction
+ let has_chart1 = metric_has_data[0] || metric_has_data[1]; let has_chart2 = metric_has_data[2] || metric_has_data[3]; let has_chart3 = metric_has_data[4]; @@ - let chart1_top = top_pad; - let chart1_bottom = top_pad + chart_height; - let chart2_top = chart1_bottom + x_gap; - let chart2_bottom = chart2_top + chart_height; - let chart3_top = if has_chart2 { - chart2_bottom + x_gap - } else { - chart1_bottom + x_gap - }; - let chart3_bottom = chart3_top + chart_height; + let chart1_top = top_pad; + let chart1_bottom = if has_chart1 { chart1_top + chart_height } else { chart1_top }; + let chart2_top = if has_chart1 { chart1_bottom + x_gap } else { chart1_top }; + let chart2_bottom = if has_chart2 { chart2_top + chart_height } else { chart2_top }; + let chart3_top = if has_chart2 { chart2_bottom + x_gap } else { chart2_top }; + let chart3_bottom = chart3_top + chart_height;Then guard the chart-1 baseline (lines 220-227) and the chart-1 cursor guide (lines 418-428) with
if has_chart1 { … }, and updateinteract_height/xlabel_yto use the topmost rendered chart.Also applies to: 220-247, 418-454
🤖 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/analytics/chart.rs` around lines 70 - 94, Define a has_chart1 boolean (e.g., based on metric_has_data for slots used by chart1) and use it to gate rendering and layout for the first chart: compute has_chart1 before computing chart1_top/chart1_bottom and update chart3_top/chart3_bottom/total_height logic to collapse subsequent charts upward when has_chart1 or has_chart2 is false (replace the unconditional chart1 spacing with conditional branches using has_chart1/has_chart2/has_chart3); wrap the chart-1 baseline and chart-1 cursor guide rendering blocks (previously unconditional around the baseline and cursor guide) with if has_chart1 { ... } guards; and finally adjust interact_height and xlabel_y calculations to pick the topmost rendered chart (use chart1_top if has_chart1, else chart2_top if has_chart2, else chart3_top) so interaction region/x-labels align with the first visible chart.src/components/analytics/mod.rs (1)
100-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove or mark unreachable the
Metric::Volumebranches inlog_matchesandSessionTotalarms.
MetricSelectorexcludesVolumefrom the 8 regular metric slots — the dropdown offers only Weight, Reps, Distance, and Duration. Volume is handled exclusively through the dedicatedvolume_exerciseslot. This means theMetric::Volumecase inlog_matches(line 121) and theMetric::Volume => 0.0arm in theSessionTotalmatch (line 172) are unreachable dead code. Remove these branches or mark them withunreachable!()to clarify intent and improve code maintainability.🤖 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/analytics/mod.rs` around lines 100 - 179, The Metric::Volume arms are dead code; update the log_matches closure and the AnalyticsMode::SessionTotal match to either remove the Metric::Volume branches or replace them with unreachable!() to indicate they cannot occur. Specifically, edit the log_matches closure defined inside the chart_data mapping (the closure named log_matches) to eliminate the Metric::Volume => false branch (or change it to unreachable!()), and in the AnalyticsMode::SessionTotal match inside the same mapping, replace the Metric::Volume => 0.0 arm with unreachable!() (or remove it and ensure match remains exhaustive via a wildcard/unreachable), so intent is explicit and dead branches are removed.
🤖 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/home.rs`:
- Around line 383-405: Clicks inside the detail panel are bubbling to the outer
toggle and collapsing the view; wrap the detailed block emitted when
*show_detail.read() is true (the inner article/ul/li produced from
resolved_logs.iter().rev().enumerate()) in a container element that calls
evt.stop_propagation() on onclick (e.g., an inner div/span with onclick: |evt|
evt.stop_propagation()) so inner taps don't reach the outer article toggle, or
alternatively move the toggle logic off the article body into a dedicated
control/button; also replace the fragile key: "{idx}" with a stable identity
from the log (e.g., log.start_time or a composite like (log.exercise_id,
log.start_time)) so diffs track the correct items.
- Line 322: The article toggle is not accessible and the detail-related
computation runs every render and clicks bubble out; update the article element
(where onclick: move |_| show_detail.toggle()) to include role: "button",
tabindex: 0, aria_expanded: "{*show_detail.read()}", and an onkeydown handler
that checks Key::Enter and Key::Character(" ") to prevent_default and toggle
show_detail so keyboard and screen readers see and can activate it; move the
resolved_logs computation (the block computing resolved_logs used only by the
detail view) behind a guard that checks show_detail.read() so it only computes
when detail is visible; and in the detail container click handler(s) add
stop_propagation() to prevent clicks inside the detail from bubbling up and
collapsing the view.
---
Outside diff comments:
In `@src/components/analytics/chart.rs`:
- Around line 70-94: Define a has_chart1 boolean (e.g., based on metric_has_data
for slots used by chart1) and use it to gate rendering and layout for the first
chart: compute has_chart1 before computing chart1_top/chart1_bottom and update
chart3_top/chart3_bottom/total_height logic to collapse subsequent charts upward
when has_chart1 or has_chart2 is false (replace the unconditional chart1 spacing
with conditional branches using has_chart1/has_chart2/has_chart3); wrap the
chart-1 baseline and chart-1 cursor guide rendering blocks (previously
unconditional around the baseline and cursor guide) with if has_chart1 { ... }
guards; and finally adjust interact_height and xlabel_y calculations to pick the
topmost rendered chart (use chart1_top if has_chart1, else chart2_top if
has_chart2, else chart3_top) so interaction region/x-labels align with the first
visible chart.
In `@src/components/analytics/mod.rs`:
- Around line 100-179: The Metric::Volume arms are dead code; update the
log_matches closure and the AnalyticsMode::SessionTotal match to either remove
the Metric::Volume branches or replace them with unreachable!() to indicate they
cannot occur. Specifically, edit the log_matches closure defined inside the
chart_data mapping (the closure named log_matches) to eliminate the
Metric::Volume => false branch (or change it to unreachable!()), and in the
AnalyticsMode::SessionTotal match inside the same mapping, replace the
Metric::Volume => 0.0 arm with unreachable!() (or remove it and ensure match
remains exhaustive via a wildcard/unreachable), so intent is explicit and dead
branches are removed.
🪄 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: 8f76d6f5-1f6c-4892-8649-af2dc089260c
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockflake.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
flake.nixgarnix.yamlsrc/components/analytics/chart.rssrc/components/analytics/mod.rssrc/components/home.rssrc/components/more.rssrc/components/session_timers.rssrc/models/analytics.rssrc/models/session.rssrc/services/app_state.rssrc/services/exercise_db.rssrc/services/storage.rs
| }; | ||
| rsx! { | ||
| article { | ||
| article { onclick: move |_| show_detail.toggle(), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Dioxus 0.7 KeyboardData event Key enum variants for Enter and Space
💡 Result:
In Dioxus 0.7, the KeyboardData accessor.key returns a dioxus::prelude::Key enum. The variants you want are: 1) Enter -> dioxus::prelude::Key::Enter [1] 2) Space -> dioxus::prelude::Key::Character(" ") (space as a character) rather than a dedicated Key::Space variant [1] Notes: - KeyboardData::key is “the value of the key pressed by the user”, considering modifier keys/layout, so the spacebar is represented as the character produced: " " [2]. - If you instead want the physical key (layout-independent), use KeyboardData::code which returns a dioxus::prelude::Code enum, where Space is Code::Space [3].
Citations:
- 1: https://docs.rs/dioxus/latest/dioxus/prelude/enum.Key.html
- 2: https://docs.rs/dioxus/latest/dioxus/events/struct.KeyboardData.html
- 3: https://docs.rs/dioxus/latest/dioxus/events/enum.KeyCode.html
🏁 Script executed:
wc -l src/components/home.rsRepository: gfauredev/LogOut
Length of output: 87
🏁 Script executed:
sed -n '310,330p' src/components/home.rsRepository: gfauredev/LogOut
Length of output: 784
🏁 Script executed:
sed -n '380,410p' src/components/home.rsRepository: gfauredev/LogOut
Length of output: 1294
🏁 Script executed:
sed -n '300,325p' src/components/home.rsRepository: gfauredev/LogOut
Length of output: 1039
Article element lacks keyboard and screen-reader accessibility.
The <article> at line 322 has onclick: move |_| show_detail.toggle() but no role, tabindex, aria_expanded, or onkeydown handler. This makes the toggle inaccessible to keyboard and assistive technology users—the element is not focusable and does not announce as interactive.
Add keyboard support by including:
role: "button"tabindex: 0(to enable keyboard focus)aria_expanded: "{*show_detail.read()}"(to indicate state)onkeydown: move |evt| { if matches!(evt.key(), Key::Enter | Key::Character(" ")) { evt.prevent_default(); show_detail.toggle(); } }
The web search confirms Dioxus 0.7 supports Key::Enter and Key::Character(" ") for these events.
Additionally, the resolved_logs computation (lines 303–320) executes on every render despite only being used when show_detail is true; gate it on the signal to avoid unnecessary allocations and signal reads. Similarly, clicking elements in the detail view (lines 383–405) bubbles up and collapses the detail; add stop_propagation() to the detail container to prevent this.
🤖 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/home.rs` at line 322, The article toggle is not accessible and
the detail-related computation runs every render and clicks bubble out; update
the article element (where onclick: move |_| show_detail.toggle()) to include
role: "button", tabindex: 0, aria_expanded: "{*show_detail.read()}", and an
onkeydown handler that checks Key::Enter and Key::Character(" ") to
prevent_default and toggle show_detail so keyboard and screen readers see and
can activate it; move the resolved_logs computation (the block computing
resolved_logs used only by the detail view) behind a guard that checks
show_detail.read() so it only computes when detail is visible; and in the detail
container click handler(s) add stop_propagation() to prevent clicks inside the
detail from bubbling up and collapsing the view.
Comment on lines
+383
to
+405
| if *show_detail.read() { | ||
| for (idx, (name, log)) in resolved_logs.iter().rev().enumerate() { | ||
| article { key: "{idx}", | ||
| header { | ||
| h4 { "{name}" } | ||
| } | ||
| ul { | ||
| if log.weight_hg.0 > 0 { | ||
| li { "{log.weight_hg}" } | ||
| } | ||
| if let Some(reps) = log.reps { | ||
| li { "{reps} reps" } | ||
| } | ||
| if let Some(d) = log.distance_m { | ||
| li { "{d}" } | ||
| } | ||
| if let Some(dur) = log.duration_seconds() { | ||
| li { "{crate::models::format_time(dur)}" } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Clicks inside the detail view bubble up and immediately collapse it.
The inner <article> / <ul> / <li> elements emitted under if *show_detail.read() have no stop_propagation, so any tap inside the detail (e.g. to read or select a metric) bubbles to the outer <article> onclick and toggles show_detail off. Either wrap the detail block in a span/div with onclick: |evt| evt.stop_propagation(), or move the toggle to a dedicated control so the body is not part of the click target.
Also note key: "{idx}" after .rev().enumerate() produces stable keys per render but is not tied to log identity; preferring log.start_time (or a composite of exercise_id and start_time) gives more meaningful diffs if the detail content ever changes between renders.
🤖 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/home.rs` around lines 383 - 405, Clicks inside the detail
panel are bubbling to the outer toggle and collapsing the view; wrap the
detailed block emitted when *show_detail.read() is true (the inner article/ul/li
produced from resolved_logs.iter().rev().enumerate()) in a container element
that calls evt.stop_propagation() on onclick (e.g., an inner div/span with
onclick: |evt| evt.stop_propagation()) so inner taps don't reach the outer
article toggle, or alternatively move the toggle logic off the article body into
a dedicated control/button; also replace the fragile key: "{idx}" with a stable
identity from the log (e.g., log.start_time or a composite like
(log.exercise_id, log.start_time)) so diffs track the correct items.
📊 Coverage ReportLines: 3774/5029 (75.04474050507059%) ⏱️ Tests: 255 tests in 0.611s
|
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
Enhancements
Style
Localization