Skip to content

Fix 10 critical bugs and performance issues in Yew and Dioxus backends #22

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
davidcforbes wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix 10 critical bugs and performance issues in Yew and Dioxus backends #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/dioxus/body.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
11 changes: 6 additions & 5 deletions src/dioxus/controls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand Down
102 changes: 62 additions & 40 deletions src/dioxus/table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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(&params.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(&params.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<usize> = 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() {
Expand Down
4 changes: 3 additions & 1 deletion src/yew/body.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ pub fn body(props: &TableBodyProps) -> Html {
texts,
} = props;

let empty_string = String::new();

html! {
<tbody class={classes.tbody}>
{ if *loading {
Expand All @@ -80,7 +82,7 @@ pub fn body(props: &TableBodyProps) -> Html {
for row in rows.iter() {
<tr class={classes.row} role="row">
for col in columns.iter() {
<td class={classes.body_cell} role="cell">{ row.get(col.id).unwrap_or(&"".to_string()) }</td>
<td class={classes.body_cell} role="cell">{ row.get(col.id).unwrap_or(&empty_string) }</td>
}
</tr>
}
Expand Down
13 changes: 11 additions & 2 deletions src/yew/controls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
<div class={classes.pagination}>
<button class={classes.pagination_button} onclick={on_prev} disabled={page_val == 0}>
{ texts.previous_button }
</button>
<span>
{ texts.page_indicator.replace("{current}", &(page_val + 1).to_string()).replace("{total}", &total_pages.to_string()) }
{ page_indicator_text }
</span>
<button
class={classes.pagination_button}
Expand Down
124 changes: 78 additions & 46 deletions src/yew/table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,29 +91,45 @@ pub fn table(props: &TableProps) -> Html {
let sort_column = use_state(|| None::<&'static str>);
let sort_order = use_state(|| SortOrder::Asc);
let search_query = use_state(|| {
let window = web_sys::window().unwrap();
let search_params =
UrlSearchParams::new_with_str(&window.location().search().unwrap_or_default()).unwrap();
search_params.get("search").unwrap_or_default()
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"))
.unwrap_or_default()
});

let debounced_search = use_state(|| None::<Timeout>);
let debounced_search = use_mut_ref(|| None::<Timeout>);

// Reset page to 0 when search query changes to prevent invalid page states
{
let page = page.clone();
let search_query = search_query.clone();
use_effect_with(search_query, move |_| {
page.set(0);
});
}

let update_search_url = {
let search_query = search_query.clone();
Callback::from(move |query: String| {
let window = web_sys::window().unwrap();
let url = window.location().href().unwrap();
let url_obj = web_sys::Url::new(&url).unwrap();
let params = url_obj.search_params();
params.set("search", &query);
url_obj.set_search(&params.to_string().as_string().unwrap_or_default());
window
.history()
.unwrap()
.replace_state_with_url(&JsValue::NULL, "", Some(&url_obj.href()))
.unwrap();
search_query.set(query);
let result = web_sys::window()
.and_then(|window| {
let url = window.location().href().ok()?;
let url_obj = web_sys::Url::new(&url).ok()?;
let params = url_obj.search_params();
params.set("search", &query);
url_obj.set_search(&params.to_string().as_string().unwrap_or_default());
window
.history()
.ok()?
.replace_state_with_url(&JsValue::NULL, "", Some(&url_obj.href()))
.ok()
});

// Only update search_query if URL update succeeded or if we're not in a browser environment
if result.is_some() || web_sys::window().is_none() {
search_query.set(query);
}
})
};

Expand All @@ -122,44 +138,51 @@ pub fn table(props: &TableProps) -> Html {
let update_search_url = update_search_url.clone();
Callback::from(move |e: InputEvent| {
let update_search_url = update_search_url.clone();
// TODO: Add debounce
// let debounced_search_ref = debounced_search.clone();
let input: web_sys::HtmlInputElement = e.target_unchecked_into();
let value = input.value();

// let prev_timeout = {
// debounced_search_ref.take()
// };
// Safely get the input element, return early if not an HtmlInputElement
let Some(input) = e.target_dyn_into::<web_sys::HtmlInputElement>() else {
return;
};
let value = input.value();

// if let Some(prev) = prev_timeout {
// prev.cancel();
// }
// Cancel previous timeout to prevent multiple URL updates
let prev_timeout = debounced_search.borrow_mut().take();
if let Some(prev) = prev_timeout {
prev.cancel();
}

let timeout = Timeout::new(50, move || {
// Create new debounced timeout (300ms delay)
let timeout = Timeout::new(300, move || {
update_search_url.emit(value.clone());
});

debounced_search.set(Some(timeout));
*debounced_search.borrow_mut() = Some(timeout);
})
};

let mut filtered_rows = data.clone();
if !search_query.is_empty() {
filtered_rows.retain(|row| {
columns.iter().any(|col| {
row.get(col.id)
.map(|v| v.to_lowercase().contains(&search_query.to_lowercase()))
.unwrap_or(false)
// Work with indices instead of cloning data to reduce memory allocations
let mut filtered_indices: Vec<usize> = 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) {
filtered_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);
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),
Expand All @@ -168,10 +191,19 @@ pub fn table(props: &TableProps) -> Html {
}
}

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];
// 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);

// 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 on_sort_column = {
let sort_column = sort_column.clone();
Expand Down