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

fix: performance improvements on input widgets #6565

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

geotrev
Copy link

@geotrev geotrev commented Sep 28, 2022

fixes #3415.

Summary

Update 5/8/2024: List/Object widget changes were removed from the scope of this PR as they aren't relevant to the input performance changes needed, and have an overall minimal impact on performance anyway. If it becomes a huge concern it can be fixed in a follow on PR.

Performance of list and object widgets degrade quickly given large sub-widget composition. See above issue with reproductions.

After pondering and exploring this on and off for a few weeks, I've concluded the issue is largely to do with how the app is providing the onChange handler for input widgets (among others). The onChange handler then updates on every key stroke, which is bad as Redux runs synchronously, and for such a large app, that will cause delays/stuttering when typing.

A separate, but related, issue is that collapsed list/object widgets are still rendering despite being visibly hidden. It's a minor perf improvement but any reduction in reconciliation is a win, IMHO, so I went ahead and hid those component trees where appropriate.

Proposed fixes

  1. Refactor each input widget to rely on self-contained React state for their value, then call the provided props.onChange on a 300ms trailing debounce to save changes back to the Redux store. I used 300ms to account for relatively slow typing, but am open to suggestions on other increments. Also updated existing debounce durations on the Markdown widget for consistency.

    Affected widgets:

    • Code
    • Color
    • DateTime
    • Markdown (removed from scope)
    • Number
    • String
    • Text
    • List
    • Object
  2. Prevent collapsed List and Object widgets from rendering when not in opened state. This is a much smaller improvement to clean up the DOM and reduce React reconciliation time (noticable delay is noticed on widget expansion without this fix).

Test plan

Fix 1: The main way I'm testing this is by loading up the kitchen sink collection and testing responsiveness of form fields with large collections of inputs and visible list items. Then refreshing and making sure unsaved changes can be re-applied from localStorage (I believe that's what the CMS uses to retain those changes). Added jest.runAllTimers to relevant unit tests to fulfill existing test cases.

Fix 2: Manual testing for speed improvements. Tests that used to assert against now-removed DOM nodes in collapsed state are removed and replaced by tests checking a null render.

Otherwise, relevant unit tests for each widget have been updated.

Unit tests:

  • String (new)
  • Text (new)
  • Color (new)

Checklist

  • I have read the contribution guidelines.
  • Code is formatted via running yarn format.
  • Tests are passing via running yarn test.
  • The status checks are successful (continuous integration). Those can be seen below.

🐶 🐱 🐰 🦔 🐀 🐁 🐜

@geotrev geotrev requested a review from a team September 28, 2022 05:24
@geotrev geotrev changed the title fix: performance improvements on List and Object widgets fix: performance improvements on input widgets Oct 10, 2022
@netlify
Copy link

netlify bot commented Mar 17, 2023

Deploy Preview for decap-www canceled.

Name Link
🔨 Latest commit 44f3d99
🔍 Latest deploy log https://app.netlify.com/sites/decap-www/deploys/645c4185961c3200082659c1

@demshy
Copy link
Member

demshy commented Aug 24, 2023

Hey @geotrev this looks very cool and we'd love to merge the PR. Would you mind resolving the conflict that the package rename caused?
About the markdown widget (that was basically rewritten) - you can leave that one out if you want and I can handle it in a separate PR.

@netlify
Copy link

netlify bot commented Oct 17, 2023

Deploy Preview for cms-demo ready!

Name Link
🔨 Latest commit 798921d
🔍 Latest deploy log https://app.netlify.com/sites/cms-demo/deploys/652fceff734fb50008cdcd20
😎 Deploy Preview https://deploy-preview-6565--cms-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@geotrev
Copy link
Author

geotrev commented Dec 30, 2023

Hey @geotrev this looks very cool and we'd love to merge the PR. Would you mind resolving the conflict that the package rename caused? About the markdown widget (that was basically rewritten) - you can leave that one out if you want and I can handle it in a separate PR.

Thanks @demshy, looks like some change sare being pushed by @martinjagodic is also pushing some changes. I'm not sure where the code is at, so I will make time to review the head of the branch shortly so I know what's going on with it. :)

Copy link

netlify bot commented Jan 1, 2024

Deploy Preview for decap-www canceled.

Name Link
🔨 Latest commit f468127
🔍 Latest deploy log https://app.netlify.com/sites/decap-www/deploys/65af65eaf4738d000881373c

@geotrev
Copy link
Author

geotrev commented Jan 1, 2024

@demshy I've updated datetime and rechecked the other input-based components, and it looks good to go from my end. let me know if there are any other concerns and I can address them as needed. Also removed Markdown widget from scope.

Also, something I'm now realizing after making this PR is that, basically all these components fully ignore props.value and pass the local value back up to global state independent of the UI/local state. There is a short window (the 300ms delay) where if a refresh happens, the value could be lost.

This goes back to the original issue in this CMS which is that all content was centrally managed by redux, which is the cause of the intense delay on input in the first place. As long as the content can be "trusted" to match the UI, then I still stand by this PR, but it's just another thing to consider on the overall architecture of the CMS and solve for in the future. Though y'all may already be on top of it, and this PR may be the lesser of two evils, so forgive me if I'm spouting redundancy. :D

@geotrev
Copy link
Author

geotrev commented Jan 3, 2024

Late night inspiration for decap devs: I actually really like the idea of experimenting with global state separating from ui state overall, as long as the checks are robust and frequent enough to ensure UI and final CMS state are in sync.

the key of course is probably engineering a solution that can receive the CMS stored value & reflect it in relevant text input fields, periodically. Perhaps through a custom hook that can mix the controlled props.value and compare that/pass it into the local widget state.value. Mixed controlled/uncontrolled is not encouraged generally but this might be a rare exception for the sake of keeping the data correct & retaining performant user input.

@demshy
Copy link
Member

demshy commented Jan 20, 2024

Hey there, sorry for taking so long, but a combination of circumstances kept me off the project for longer than I anticipated.

We are actually contemplating a complete rework of state management and will consider your insights when the discussion resumes (or even better we open up a thread on discord).

For now, I will merge the PR once the tests go through. Thank you for this!

@demshy
Copy link
Member

demshy commented Jan 20, 2024

Seems like a few more clock advances will be needed in e2e tests now with the debounce.

Calling flushClockAndSave instead of cy.contains('button', 'Save').click(); in a few places seems to fix most of them, but I didn't have a chance to find them all (yet).

@geotrev
Copy link
Author

geotrev commented Jan 20, 2024

Seems like a few more clock advances will be needed in e2e tests now with the debounce.

Calling flushClockAndSave instead of cy.contains('button', 'Save').click(); in a few places seems to fix most of them, but I didn't have a chance to find them all (yet).

Darn. I can try and fix those locally when I have more time, if you haven't gotten to it.

@geotrev geotrev requested a review from a team as a code owner January 22, 2024 11:34
@demshy
Copy link
Member

demshy commented Jan 22, 2024

I have pushed an update that fixes some of the tests, but for now it still leaves out three of them:
i18n_editorial_workflow_spec_test_backend.js and i18n_simple_workflow_spec_proxy_fs_backend.js that I did not get to yet

and a bug that the test actually helped find (within field_validations_spec.js). It's a great reminder why e2e tests help us :)

You can reproduce the bug like this:

  • open settings > authors
  • add a new author
  • collapse & expand the new author form
  • after that even after you fill in the fields, the validation fails

@geotrev
Copy link
Author

geotrev commented Feb 21, 2024

@demshy that's great!

Anything else I can help with? Looks like CI is still failing, happy to jump on a bandwagon if needed.

@demshy
Copy link
Member

demshy commented Feb 22, 2024

Oh right, I noticed now that I crossed out the text in a way that made it ambiguous 😅

The bug with field_validations_spec that I described is still there, because I haven't been able to get around to it yet.
I'd be more than happy if you give it a go

@geotrev
Copy link
Author

geotrev commented May 8, 2024

Apologies, I also haven't been able to get to this. Not sure when I will. Will update if that changes!

@geotrev
Copy link
Author

geotrev commented May 8, 2024

@demshy So I ended up having time immediately as I posted my previous comment. I ended up reverting the list/object widget changes since I don't think they have a significant performance impact compared to the input widgets. I see the cypress tests for field_validations pass locally, so let's see if they pass in CI!

@herpster
Copy link

Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Editing peformance is rough for large lists
4 participants