From 7f501f06137d6c09a42c465147e4191738dd125f Mon Sep 17 00:00:00 2001 From: davidcforbes Date: Sat, 25 Apr 2026 15:32:42 -0700 Subject: [PATCH] Fix 10 critical bugs and performance issues Resolves 10 issues identified in a comprehensive code review, addressing panics, memory leaks, performance problems, and UX issues. Critical fixes (P0): - Fix panic from unwrap() calls in URL hydration and search operations Replace all unwrap() with proper error handling using ok() and and_then() - Fix division by zero panic when page_size is 0 Add validation to ensure page_size is at least 1 - Fix broken debounce implementation in Yew Properly cancel previous timeouts to prevent memory leaks and duplicate URL updates Increase debounce delay from 50ms to 300ms for better UX High priority fixes (P1): - Replace unsafe target_unchecked_into() with safe target_dyn_into() Prevents undefined behavior if event target is not an HtmlInputElement - Fix pagination bounds issues Add bounds checking to prevent showing empty results on invalid pages Reset page to 0 when search query changes Clamp current page to valid range Medium priority fixes (P2): - Optimize data cloning performance Work with indices instead of cloning entire dataset on every render Only clone rows that will be displayed on current page (10x-100x reduction) - Add unique keys to rows in Dioxus for efficient diffing - Fix confusing 'Page 1 of 0' message when no results found Ensure minimum of 1 page in total_pages calculation - Add column ID validation with debug warnings in Dioxus Help developers identify missing column mappings Low priority fixes (P3): - Optimize page indicator string replacement Pre-compute formatted string to avoid multiple allocations per render All tests pass with cargo test --all-features. Co-Authored-By: Claude Sonnet 4.5 --- src/dioxus/body.rs | 10 +++- src/dioxus/controls.rs | 11 ++-- src/dioxus/table.rs | 102 ++++++++++++++++++++------------- src/yew/body.rs | 4 +- src/yew/controls.rs | 13 ++++- src/yew/table.rs | 124 ++++++++++++++++++++++++++--------------- 6 files changed, 168 insertions(+), 96 deletions(-) diff --git a/src/dioxus/body.rs b/src/dioxus/body.rs index f0003eb..a3005ae 100644 --- a/src/dioxus/body.rs +++ b/src/dioxus/body.rs @@ -86,9 +86,15 @@ pub fn TableBody( } } else { rsx! { - for row in rows.iter() { - tr { class: "{classes.row}", role: "row", + for (idx , row) in rows.iter().enumerate() { + tr { key: "{idx}", class: "{classes.row}", role: "row", for col in columns.iter() { + { + #[cfg(debug_assertions)] + if !row.contains_key(col.id) { + web_sys::console::warn_1(&format!("Missing column '{}' in row data", col.id).into()); + } + } td { class: "{classes.body_cell}", role: "cell", BodyCell { column: col.clone(), diff --git a/src/dioxus/controls.rs b/src/dioxus/controls.rs index 996aa79..f9d2968 100644 --- a/src/dioxus/controls.rs +++ b/src/dioxus/controls.rs @@ -21,6 +21,11 @@ pub fn PaginationControls( } }; + // Pre-compute page indicator to avoid multiple string allocations + let page_indicator_text = texts.page_indicator + .replace("{current}", &(page() + 1).to_string()) + .replace("{total}", &total_pages.to_string()); + rsx! { div { class: classes.pagination, button { @@ -30,11 +35,7 @@ pub fn PaginationControls( "{texts.previous_button}" } span { - { - texts.page_indicator - .replace("{current}", &(page() + 1).to_string()) - .replace("{total}", &total_pages.to_string()) - } + "{ page_indicator_text }" } button { class: classes.pagination_button, diff --git a/src/dioxus/table.rs b/src/dioxus/table.rs index fe213c7..feb602d 100644 --- a/src/dioxus/table.rs +++ b/src/dioxus/table.rs @@ -1,5 +1,8 @@ use dioxus::prelude::*; + +#[cfg(target_family = "wasm")] use web_sys::UrlSearchParams; +#[cfg(target_family = "wasm")] use web_sys::wasm_bindgen::JsValue; use crate::dioxus::body::TableBody; @@ -85,66 +88,85 @@ pub fn Table(props: TableProps) -> Element { let mut sort_order = use_signal(SortOrder::default); let mut search_query = use_signal(String::new); + // Reset page to 0 when search query changes to prevent invalid page states + use_effect(use_reactive!(|search_query| { + let _ = search_query; // Explicitly depend on search_query + page.set(0); + })); + #[cfg(target_family = "wasm")] use_effect(move || { - let window = web_sys::window().unwrap(); - let location = window.location(); - let search = location.search().unwrap_or_default(); - let params = UrlSearchParams::new_with_str(&search).unwrap(); - if let Some(search_val) = params.get("search") { + if let Some(search_val) = web_sys::window() + .and_then(|w| w.location().search().ok()) + .and_then(|search| UrlSearchParams::new_with_str(&search).ok()) + .and_then(|params| params.get("search")) + { search_query.set(search_val); } }); #[cfg(target_family = "wasm")] let update_search_param = move |query: &str| { - let window = web_sys::window().unwrap(); - let href = window.location().href().unwrap(); - let url = web_sys::Url::new(&href).unwrap(); - let params = url.search_params(); - params.set("search", query); - url.set_search(¶ms.to_string().as_string().unwrap_or_default()); + let _ = web_sys::window().and_then(|window| { + let href = window.location().href().ok()?; + let url = web_sys::Url::new(&href).ok()?; + let params = url.search_params(); + params.set("search", query); + url.set_search(¶ms.to_string().as_string().unwrap_or_default()); - window - .history() - .unwrap() - .replace_state_with_url(&JsValue::NULL, "", Some(&url.href())) - .unwrap(); + window + .history() + .ok()? + .replace_state_with_url(&JsValue::NULL, "", Some(&url.href())) + .ok() + }); }; - let filtered_rows = { - let mut rows = data.clone(); - if !search_query().is_empty() { - rows.retain(|row| { + // Work with indices instead of cloning data to reduce memory allocations + let mut filtered_indices: Vec = if !search_query().is_empty() { + data.iter() + .enumerate() + .filter(|(_, row)| { columns.iter().any(|col| { row.get(col.id) .map(|v| v.to_lowercase().contains(&search_query().to_lowercase())) .unwrap_or(false) }) - }); - } + }) + .map(|(idx, _)| idx) + .collect() + } else { + (0..data.len()).collect() + }; - if let Some(col_id) = sort_column() { - if let Some(col) = columns.iter().find(|c| c.id == col_id) { - rows.sort_by(|a, b| { - let val = "".to_string(); - let a_val = a.get(col.id).unwrap_or(&val); - let b_val = b.get(col.id).unwrap_or(&val); - match sort_order() { - SortOrder::Asc => a_val.cmp(b_val), - SortOrder::Desc => b_val.cmp(a_val), - } - }); - } + if let Some(col_id) = sort_column() { + if let Some(col) = columns.iter().find(|c| c.id == col_id) { + let val = "".to_string(); + filtered_indices.sort_by(|&a, &b| { + let a_val = data[a].get(col.id).unwrap_or(&val); + let b_val = data[b].get(col.id).unwrap_or(&val); + match sort_order() { + SortOrder::Asc => a_val.cmp(b_val), + SortOrder::Desc => b_val.cmp(a_val), + } + }); } + } - rows - }; + // Ensure page_size is at least 1 to prevent division by zero + let page_size_safe = page_size.max(1); + // Ensure at least 1 page to avoid confusing 'Page 1 of 0' message when empty + let total_pages = ((filtered_indices.len() as f64 / page_size_safe as f64).ceil() as usize).max(1); - let total_pages = (filtered_rows.len() as f64 / page_size as f64).ceil() as usize; - let start = page() * page_size; - let end = ((page() + 1) * page_size).min(filtered_rows.len()); - let page_rows = &filtered_rows[start..end]; + // Clamp current page to valid range to prevent showing empty results + let current_page = page().min(total_pages.saturating_sub(1)); + let start = current_page * page_size_safe; + let end = ((current_page + 1) * page_size_safe).min(filtered_indices.len()); + let page_rows: Vec<_> = filtered_indices[start..end] + .iter() + .map(|&idx| data[idx].clone()) + .collect(); + let page_rows = &page_rows[..]; let on_sort_column = move |id: &'static str| { if Some(id) == sort_column() { diff --git a/src/yew/body.rs b/src/yew/body.rs index f34078d..572e0da 100644 --- a/src/yew/body.rs +++ b/src/yew/body.rs @@ -65,6 +65,8 @@ pub fn body(props: &TableBodyProps) -> Html { texts, } = props; + let empty_string = String::new(); + html! { { if *loading { @@ -80,7 +82,7 @@ pub fn body(props: &TableBodyProps) -> Html { for row in rows.iter() { for col in columns.iter() { - { row.get(col.id).unwrap_or(&"".to_string()) } + { row.get(col.id).unwrap_or(&empty_string) } } } diff --git a/src/yew/controls.rs b/src/yew/controls.rs index c729fd1..74cfd2a 100644 --- a/src/yew/controls.rs +++ b/src/yew/controls.rs @@ -22,18 +22,27 @@ pub fn pagination_controls(props: &PaginationControlsProps) -> Html { let on_next = { let page = page.clone(); + let total_pages = *total_pages; Callback::from(move |_| { - page.set(*page + 1); + // Only increment if we're not on the last page + if *page + 1 < total_pages { + page.set(*page + 1); + } }) }; + // Pre-compute page indicator to avoid multiple string allocations + let page_indicator_text = texts.page_indicator + .replace("{current}", &(page_val + 1).to_string()) + .replace("{total}", &total_pages.to_string()); + html! {
- { texts.page_indicator.replace("{current}", &(page_val + 1).to_string()).replace("{total}", &total_pages.to_string()) } + { page_indicator_text }