Skip to content
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

Recordedit performance improvements #2389

Merged
merged 21 commits into from
Jan 17, 2024
Merged

Conversation

jrchudy
Copy link
Member

@jrchudy jrchudy commented Jan 13, 2024

The following changes are made in this pull request to address different performance issues in the recordedit app:

  • changed the setValueForAllInputs function to use setValue instead of reset so only the values for that column are updated in each form instead of updating the whole form
  • changed DateTimeField to use useWatch instead of watch so it will trigger on the value changing for the individual date and time values instead of any value in the form
  • changed MultiFormInputRow to use useWatch instead of watch so it will trigger on the value changing for the multi form input value instead of any value in the form
  • remove activeMultiForm and toggleActiveMultiForm from RecordeditProvider
    • this state variable and function are moved to RecordeditInner component
    • this prevents the FormRow component from re-rendering when the RecordeditProvider changed the activeMultiForm value
    • update props for KeyColumn, FormContainer, FormRow, and MultiFormInputRow
  • use React.memo for FormRow component to prevent re-renders when FormContainer re-renders
  • use React.memo for InputSwitch component to prevent re-renders when FormRow re-renders
  • bug found in RangeInputs where the wrong format was being used for timestamp and timestampTZ columns

Potential Further Performance Improvements

I also noted another potential performance improvement for the callAddForm function in RecordeditInner. Instead of updating all of the values and calling reset, we might want to consider iterating over the forms being added and set each value individually using setValue.

In the code I added a lengthy comment that details how this might improve the performance and a contradicting thought. In the end, I decided to leave this unchanged and only added memo to the InputSwitch component so when reset is called, this component isn't being re-rendered.

… triggering the overflow logic when the rows would dissapear so the reference to rowConainter.current would still be present. Now that it's delayed, current is updated (next state update cycle) and should be checked before trying to iterate
…f forms in this function since it hasn't changed yet anyways. This was a bug that was hidden in previous implementation
… to only occur when an error is present for the current input. Moved logic for activeMultiForm to outside of form-row to prevent rerendering every form row when this prop changes
…Still need to change how avtiveMultiForm is being shared. For a boolean that is controling what row displays or not, a lot of rerendering is occurring since the entire recordedit provider is changing
…provider. Add these as a state variable for recordEditInner to pass to form container and key column for use in those components. This prevents multiple rerenders of the form row components when the row is open/closed
@jrchudy jrchudy requested a review from RFSH January 13, 2024 03:23
@jrchudy jrchudy self-assigned this Jan 13, 2024
…he above 'datetime.return' is intended for timestampTZ columns and 'timestamp' is intended for timestamp columns
…etting values instead of updating the whole form context
@jrchudy jrchudy merged commit dd18bdd into master Jan 17, 2024
@jrchudy jrchudy deleted the recordedit-performance-improvement branch January 17, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants