react-admin icon indicating copy to clipboard operation
react-admin copied to clipboard

SimpleFormIterator very slow

Open HansKre opened this issue 1 year ago • 6 comments

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:

  • Preferably, a sandbox forked from https://stackblitz.com/~/github.com/HansKre/ra-demo

Environment

  • React-admin version: 5.4.0 with react-hook-form: 7.53.2
  • React version: 18.3.1
  • Browser: Chrome, Safari

HansKre avatar Dec 17 '24 09:12 HansKre

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.

slax57 avatar Dec 17 '24 10:12 slax57

@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?

SKupisz avatar Mar 13 '25 16:03 SKupisz

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

djhi avatar Mar 13 '25 16:03 djhi

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

SKupisz avatar Mar 13 '25 17:03 SKupisz

@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

SKupisz avatar Mar 22 '25 13:03 SKupisz

@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

slax57 avatar Mar 24 '25 10:03 slax57

@HansKre @SKupisz We have just merged #10926 which contains a fix to speed up the ArrayInput/SimpleFormIterator. Can you check once react-admin 5.13.0 is released if this also improves performances in your use-cases? FYI 5.13.0 should be released by the end of this week. Thank you.

slax57 avatar Nov 05 '25 13:11 slax57

Thanks for getting back on this issue, @slax57 Can't really confirm, that slowlyness has been resolved, tested with 5.13.1. However, we noticed that the reported performance problems are not nearly as bad in production as they are when running on localhost. Since our users are not effect as badly as us developers, it's less sever for us.

HansKre avatar Nov 20 '25 13:11 HansKre

@HansKre OK, thanks for taking the time to provide this feedback! May I consider this issue solved then?

slax57 avatar Nov 21 '25 08:11 slax57