fix: performance improvements on input widgets

[geotrev]

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:

• [x] String (new)
• [x] Text (new)
• [x] Color (new)

Checklist

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

🐶 🐱 🐰 🦔 🐀 🐁 🐜

[netlify[bot]]

✅ Deploy Preview for decap-www canceled.

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

[demshy]

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[bot]]

✅ Deploy Preview for cms-demo ready!

Name	Link
🔨 Latest commit	798921d50160c6f78458dc286762b31a048bb0a7
🔍 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... Use your smartphone camera to open QR code link.

---

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

[geotrev]

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. :)

[netlify[bot]]

✅ Deploy Preview for decap-www canceled.

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

[geotrev]

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

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]

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]

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]

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.

[demshy]

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]

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

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]

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

[geotrev]

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