leptos-rs / leptos

Build fast web applications with Rust.
https://leptos.dev
MIT License
15.28k stars 599 forks source link

Grid is not rendered correctly when treating vec of signals as slices #2635

Closed Neighborkid01 closed 2 weeks ago

Neighborkid01 commented 2 weeks ago

Describe the bug I've created a sample project to demonstrate this issue here. I am creating a Minesweeper clone and I have a vec of signals that contains all the cells in the game. When I pass that into a row component, I use .chunks() to split the vec into slices based on the grid width for the rows and each row renders the individual cells. When I go from a smaller grid size (n1 x m1 = area1) to a larger one (n2 x m2 = area2), the cells render correctly for the first area1 cells but on the row containing the cell with index area1 - 1, the cells after that are not present. Starting on the next row, all the cells render as expected.

When I was logging things out to see where the breakdown was happening, I discovered that if I read (and use) the cells.len() value when generating the cells for a row, then it behaves as expected, but if I comment that out or name the variable _len, it breaks again. I believe this is because the compiler is optimizing out the "unused" call to .len()

P.S. On a side note, I also don't understand why the cells behave differently on that row between the two tables and why sometimes the array seems to be completely cleared out on resize and other times it isn't. It seems like in GoodTable the cells on that row are all new and don't sync state with BadTable but the cells on the other rows do...

I'm fairly inexperienced with Leptos so if I'm doing something super wrong I'm happy to know about it.

Leptos Dependencies

Please copy and paste the Leptos dependencies and features from your Cargo.toml.

leptos = { version = "0.6.12", features = ["csr", "nightly"] }

To Reproduce Steps to reproduce the behavior:

  1. Clone the project
  2. Run trunk serve and open the page in the browser
  3. The page will default to a 5x5 grid
  4. Click the 10x10 button
  5. See that in BadTable, the cells on the 3rd row stop after cell 24 (the blue number on the left is the length of the row)
  6. Click the 15x15 button
  7. See that in BadTable, the cells on the 7th row stop after cell 99
  8. Click the 10x10 button
  9. See that now both tables render correctly as the length is less than the previous length

Expected behavior The tables should render with the correct number of elements in each row regardless of the call to .len()

Screenshots

image
gbj commented 2 weeks ago

Steps I took to debug:

  1. Saw that this was a keyed list, and that the issue was solved by fixing the keys. Wondered whether there could be duplicated keys.
  2. In <BadTable/>, I added an effect to log out the keys:
    create_effect(move |_| {
        leptos::logging::log!(
            "{:?}",
            rows().into_iter().map(|tuple| tuple.0).collect::<Vec<_>>()
        );
    });
  3. Saw that this logs twice when I resize. This is because you update two separate signals in the event handler, and read from both of those signals in the let rows definition. In Leptos 0.6 and earlier, signal updates/effects are not batched by default but run synchronously — so the rows are basically updated twice when you click, one for each signal they depend on. Note that rows reads from the grid signal, but the keys don't depend on the grid signal (unlike in GoodTable), so an "old" row sticks around.
  4. I added batch(move || { ... }) inside the resize function:
    let resize = move |dim: Dimensions| {
        batch(move || {
            set_dimensions.update(|dimensions| *dimensions = dim); // this can be .set() btw
            let mut new_vec = vec![];
            for _ in 0..dimensions.with(|d| d.width * d.height) {
                new_vec.push(create_signal(Cell { example: false }));
            }
            set_grid.update(|grid| *grid = new_vec); // this can be .set() btw
        });
    };

    Works as expected.


Broader point: There's no reason to use <For/> here, as you are invalidating every key on every dimension change anyway, so all of them will be replaced when the dimensions change. If you use plain iteration it will perform exactly the same and you won't have the key or batching issue.

Neighborkid01 commented 2 weeks ago

Thanks so much!