Fix 613 - remove random react key generation

[chgiesse]

Hey!
After some lucky coincidence and investigation, I think I found the issue leading to the rendering issues. I just had to change the key prop in newDashRenderComponent from Math().random() to a stable value (index ?? 0).

• Random keys cause React to treat components as completely new instances on every render
• Stable keys allow React to properly identify and reconcile existing components
• React can efficiently update only changed props instead of destroying and recreating components

you can find details in the official react docs https://react.dev/learn/rendering-lists#rules-of-keys

I could notice overall rendering performance improvements and these rendering artefacts mentioned in the issue are removed to 99.9%.

Best regards,
Christian

[BSd3v]

You'll need to check against the Switch, this was a major issue there.

[chgiesse]

I remember, but works, also in a menu. I ran the tests and dmc docs with the index.

Here are the test results:
Results (373.22s):
90 passed
9 failed
- tests/dates/test_date_picker.py:73 test_001dp_DatePickerInput
- tests/dates/test_date_picker.py:136 test_002dp_DatePickerInput
- tests/dates/test_month_picker.py:65 test_001dp_MonthPickerInput
- tests/dates/test_month_picker.py:126 test_002dp_MonthPickerInput
- tests/test_carousel.py:7 test_001ca_carousel
- tests/test_carousel.py:47 test_002ca_carousel
- tests/test_new_notification.py:11 test_001na_notification_new_noreopen
- tests/test_new_notification.py:94 test_003nc_notification_new_clear
- tests/test_sliders.py:11 test_001sl_slider
2 skipped

[chgiesse]

When I run the tests individually - the tests pass:

collecting ...
tests/dates/test_date_picker.py ✓✓ 17% █▋
tests/dates/test_month_picker.py ✓✓ 33% ███▍
tests/test_carousel.py ✓✓ 50% █████
tests/test_new_notification.py ✓✓✓✓✓ 92% █████████▎
tests/test_sliders.py ✓ 100% █████████

[AnnMarieW]

Hi @chgiesse

Thanks for the PR! It makes sense that it would be an improvement.
Are you referring to the rendering issue with how the Stepper flickers when updated in a callback? (as described issue #613)

I tried the Stepper example in the docs both with the current release and with your change, and I don't see much improvement. It's usually smoother when run locally, rather than in the live docs site, but I still see the flicker. Can you describe the difference you see and how you measured the performance improvement?

[chgiesse]

Hi @AnnMarieW

Yes so the stepper is fixed for me now and I think the main reason why I face the issue more than the examples in the docs is that the stepper updates the URL and the whole pages gets loaded again. With the random ID, although the components are mostly the same react treats them as completely new component leading to the rendering issues.

Beside that, I could notice better memory management with quicker going to base level memory usage after heavy loads. Around 20% less memory usage overall. You can see these metrics in the dev tools perfomance monitor. I compared the docs with each version.

Also lighthouse score went from 83 to 99 - with first contentfull paint from 1.2s to 0.5s and largest contentful paint from 2.4 to 1.0! I used slow 4g throttling and cache clearing to get the same request times like your deployed version.

After all these test the patched version dropped down to 108mb while the dash-mantine-components.com version is still at 158mb.

I think this is all due to better caching and less rendering. Also easier for the heap to garbage collect unused elements/components.

[AnnMarieW]

Thanks for the detailed explanation! I tried it in all the pages that used the function and they all worked (at least as well as the current version), plus all the tests pass. I think there is no harm in merging this.

I have few other open PRs that are ready for review, and I'll add this one to the queue for the next release. Before merging, I like to see if @alexcjohnson has time to take a look because he provides such great feedback. 🙂

[alexcjohnson]

This looks great, with one speculative issue: the plural newRenderDashComponents uses null for the index, so all the components created this way will have the same key 0. Generally these components will not be rendered as children in an array - which is typically where the key matters. But can we be sure they will never be rendered this way? For example, in the DCC dropdown you're allowed to provide options as a dictionary of value: label, and the labels will ultimately be placed in an array within the dropdown. If these all got the same key it would cause an error from React. I suppose you could say that Mantine shouldn't let that happen, they should give those elements unique keys - or wrap them in elements with unique keys - but to be on the safe side we could use the index i that we already have when newRenderDashComponents is calling newRenderDashComponent.

[chgiesse]

Hi @alexcjohnson,
that makes sense, I adjusted it accordingly. Thanks for the input!

[alexcjohnson]

💃 Looks good, thanks for the tweak @chgiesse!

[AnnMarieW]

Thanks @chgiesse

Before merging could you please update the CHANGELOG.md? At the top of the file, it should look like

# Change Log

# [Unreleased]

### Fixed

Then a short description, PR # and your GitHub username.

[chgiesse]

Hi! I just wanted to checkin on a snippet from the documentation

• "You might be tempted to use an item’s index in the array as its key. In fact, that’s what React will use if you don’t specify a key at all. But the order in which you render items will change over time if an item is inserted, deleted, or if the array gets reordered. Index as a key often leads to subtle and confusing bugs."

That shouldn't be a problem, because the dashRenderer renders the whole list of items also with patch operations - right? I have multiple callbacks with patch operations on mantine components with the updated version and they work as expected.

[AnnMarieW]

I think this is OK because the index in this case is the order of the props that need to be rendered. For example, the Stepper can have components as props for ['label', 'description', 'icon']

But I'll leave this open for a bit in case @alexcjohnson wants to weigh in again

[chgiesse]

Sounds good!

[alexcjohnson]

Agreed, index is the best we can do given the information we have at this stage.

[AnnMarieW]

@alexcjohnson - thanks for double checking

@chgiesse - thanks again for this PR - it's a nice performance improvement for rendering components-as-props