Skip to content

SimpleFormIterator very slow #10419

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.

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
HansKre opened this issue Dec 17, 2024 · 6 comments
Open

SimpleFormIterator very slow #10419

HansKre opened this issue Dec 17, 2024 · 6 comments

Comments

@HansKre
Copy link

HansKre commented Dec 17, 2024

What you were expecting:
SimpleFormIterator should be able to handle reasonable sized tables without performance issues.

What happened instead:
We are using SimpleFormIterator to edit 7x TextInput and 1x SelectInput. Unfortunately, even for a single entry in the ArrayInput-Field, we are seeing warnings like this in the console when editing:

[Violation] 'message' handler took <N>ms
[Violation] 'input' handler took 347ms
chunk-276SZO74.js?v=198b0983:18625 [Violation] 'mousedown' handler took 346ms
chunk-276SZO74.js?v=198b0983:18625 [Violation] 'click' handler took 356ms
5[Violation] 'focusout' handler took <N>ms
chunk-276SZO74.js?v=198b0983:18625 [Violation] 'click' handler took 361ms
chunk-276SZO74.js?v=198b0983:18625 [Violation] 'click' handler took 420ms
[Violation] 'input' handler took 365ms
[Violation] 'input' handler took 360ms
chunk-276SZO74.js?v=198b0983:18625 [Violation] 'setTimeout' handler took 380ms

The slowness is more or less linearly increasing with every form-component, in other words, it looks like react-admin doesn't really work well / scale with larger data-sets / forms. We really hope that's not the case, as this for us is a show-stopper.

Here is a demo: https://stackblitz.com/~/github.com/HansKre/ra-demo

Steps to reproduce:
Go to the demo-link and try using the form. Ideally, open dev-tools to see the warnings.

Related code:

Environment

  • React-admin version: 5.4.0 with react-hook-form: 7.53.2
  • React version: 18.3.1
  • Browser: Chrome, Safari
@slax57
Copy link
Contributor

slax57 commented Dec 17, 2024

Thanks for the report and the sandbox.
A sandbox forked from the simple demo would have been easier to work on, since we wouldn't have to rule out all the custom code you have in your project first.
But in any case, I tried to look a bit deeper in it, and indeed it seems there are a lot of unnecessary rerenders of the ArrayInput children.
These rerenders seem to be due to updates in the formState, e.g. when inputs get dirty. And also some components seem to render because React thinks their children have changed, which may indicate a non-stable reference to children function.

In any case, this deserves more investigation, but I'm convinced there are things to optimize there, so I'm labeling as bug.

@SKupisz
Copy link

SKupisz commented Mar 13, 2025

@slax57 I know it's been quite a while since this bug was opened, but I'd like to verify one thing regarding react-admin in terms of the issue discussed (I'm writing, as I think there might be potential improvement in terms of react-admin, if you'll confirm what I suppose might be the case for improvement, I can open a separate issue for that and take a glance on it)

I've took a look at the example of @HansKre and indeed, it works slowly for the bigger number of elements. However, it's not only about the rendering itself, it's about switching between the tabs of the TabbedForm component. Therefore, in case the react-admin uses react18 or later at its foundation, it's possible to speed up switching between tabs with the use of useTransition hook

What do you think about it?

@djhi
Copy link
Collaborator

djhi commented Mar 13, 2025

I'd love to see a PR! We require at least React 18 so that should work.

@SKupisz
Copy link

SKupisz commented Mar 13, 2025

Sure, I'll see what I can do and link a PR with it if I solve it

@SKupisz
Copy link

SKupisz commented Mar 22, 2025

@djhi I've taken a deep dive into this issue and it looks like this might be the bigger issue than I thought it would be - in the code, there is the following part of it stated in comments in the file packages/ra-ui-materialui/src/form/TabbedFormView.tsx:
/* All tabs are rendered (not only the one in focus), to allow validation on tabs not in focus. The tabs receive a hidden property, which they'll use to hide the tab using CSS if it's not the one in focus. See https://github.com/marmelab/react-admin/issues/1866 */
This turns out to be blocking the addition of useTransition - I wanted to select one particular tab and then render it, which would be the wrapped with startTransition. However, if we render all at once, then it's impossible to do it and therefore deprioritize its render in comparison to switching the tabs. So, I think that handling this at first would be better.
I had some ideas how we can walk this issue over, including the following: What if we validate the tabs not in the moment of saving, but in the moment of changing the tab? if this would be possible, we can validate each tab changes either when changing to the other tab or (in case of the last tab remaining) when we save the changes. In case a tab is invalid, the user should not be able to save or change to the other tab. This would make it possible to use the useTransition hook in a convenient way, wdyt?

Also, regarding the @HansKre issue, I took a look at his example and was concerned whether for long lists and tables, react-admin implements some kind of a list virtualization or any analogical solution? I think this would also improve the performance

@slax57
Copy link
Contributor

slax57 commented Mar 24, 2025

@SKupisz Thank you for taking a look into this.

What if we validate the tabs not in the moment of saving, but in the moment of changing the tab?

This doesn't cover the case when a tab the user has not yet opened contains an error.

In case a tab is invalid, the user should not be able to save or change to the other tab.

IMHO this is bad UX. The user should not be prevented to change tabs, even if the current tab is invalid. That kind of behavior would fit more to a wizard-step form, but tabbed forms are not necessarily wizard forms.

react-admin implements some kind of a list virtualization or any analogical solution? I think this would also improve the performance

While I agree with the statement, I thought I'd just mention two solutions react-admin already offers:

  • <Datagrid> with the optimized prop
  • <DatagridAG>, an Enterprise component, based on AG Grid, which offers DOM virtualization out of the box

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants