[HOLD for payment 2022-05-25][$1000] Non-unique keys React warning. Possibly related to RenderHTML ?

[marcaaron]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Build the app on dev in iOS

Expected Result:

1. No console warnings are seen

Actual Result:

1. Console is complaining with this:

Workaround:

Yes. This doesn't seem to affect app performance, but may indicate a serious problem.

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number:
Reproducible in staging?: Pretty sure the warning is silenced there.
Reproducible in production?: Pretty sure the warning is silenced there.
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:
Slack conversation:

View all open jobs on GitHub

[melvin-bot]

Triggered auto assignment to @JmillsExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[JmillsExpensify]

Wow, interesting find. Posted the upwork job here for proposals: https://www.upwork.com/jobs/~01e7b5db822c3f7489

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

[melvin-bot]

Triggered auto assignment to @AndrewGable (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[muas19]

Where is this happening? Which screen?

[parasharrajat]

This is what needs to be debugged and solved.

[JmillsExpensify]

Doubling price to $500.

[eVoloshchak]

1. I'm unable to obtain logs without the @expensify e-mail. Is there a way to register such an email to get access to logs?
2. If it's possible, could we get screenshot/video of full Component Stack?

[parasharrajat]

Sorry, those logs are internal. You should be able to see these warnings on running the app on Android.

[JmillsExpensify]

Doubling the price to $1000.

[eVoloshchak]

I was able to reproduce this problem when chat contains a message with two (or more) code blocks with the line break between them. For example, if you paste and send the following into the composer:

The problem occurs inside the renderChildren function of react-native-render-html. It calls mapCollapsibleChildren function for every child, inside of which every child is assigned the key

const key = childTnode.nodeIndex;

The problem is, TPhrasing render nodes always have a nodeIndex = 0

Proposal:
Use element's index additionally to nodeIndex when assigning keys to children

const key = `${childTnode.nodeIndex}_${n}`

This ensures that every child always has a unique key

Before:

22-05-02-14-02-57.mp4

After:

22-05-02-14-08-49.mp4

[parasharrajat]

Good catch @eVoloshchak. @eVoloshchak did you try upgrading the lib version to see if this issue still exists?

[eVoloshchak]

@eVoloshchak did you try upgrading the lib version to see if this issue still exists?

Yup, tried upgrading to the latest version (6.3.1 -> 6.3.4), issue is still present

[parasharrajat]

Ok, Thanks. I am fine with @eVoloshchak 's proposal. The next steps would be:

1. Create the issue on the react-native-render-html repo.
2. Create a PR with the fix on the react-native-render-html repo.
3. Upgrade the lib on this repo when solution is merged/applied.

cc: @AndrewGable

🎀 👀 🎀 C+ reviewed

[AndrewGable]

Sounds good to me!

[melvin-bot]

📣 @eVoloshchak You have been assigned to this job by @AndrewGable!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[eVoloshchak]

Submitted proposal on Upwork

[JmillsExpensify]

@eVoloshchak Hired and offer sent. @parasharrajat I've invited you to the job as well.

[eVoloshchak]

Created the issue on react-native-render-html repo.
BTW, while creating minimal reproducible example I found out that the issue occurs only if you set dangerouslydisablewhitespacecollapsing prop to true

[eVoloshchak]

@parasharrajat, it's been a week and the issue has no response from the maintainer. Should we wait more?

[parasharrajat]

I thought you created the PR as well. #8655 (comment).

Also, please change the title to `Non-unique React keys warning when using dangerouslyDisableWhitespaceCollapsing

[eVoloshchak]

I thought you created the PR as well. #8655 (comment)

Ok, will do that shortly

[parasharrajat]

Also, the maintainer is very active on the repo. He must be busy with some stuff but he will take a look. Try joining the discord and mention this issue.

[mdneyazahmad]

I earlier posted this issue https://expensify.slack.com/archives/C01GTK53T8Q/p1644402310844119. Am I eligible for reporting bonus?

[eVoloshchak]

So I've received an answer from repo maintainer

The real issue is that dangerously (which means in this context: not really tested) disabling whitespace collapsing would cause @native-html/transient-render-engine to produce a faulty tree in certain situations, where indexes are not reflecting real position. Hence, this should be fixed in this repository instead of implementing a hack around here: https://github.com/native-html/core/tree/master/packages/transient-render-engine.
As a side note regarding using this not-so-recommended prop, in most of the cases you'll be better off with a global style which sets white-space: pre

I searched and found out that dangerouslyDisableWhitespaceCollapsing was set to true in order to fix #3859.
So, as was suggested, I set dangerouslyDisableWhitespaceCollapsing to false and added a whiteSpace: 'pre' to TRenderEngineProvider's baseStyle and it worked. It resolves this issue without breaking #3859. Tested on web and native. So if there is no other issues besides #3859 that require dangerouslyDisableWhitespaceCollapsing to be enabled, we can resolve this.

[parasharrajat]

@eVoloshchak Could you share a video showing the effect of the new change? Please use a combination of messages in any chat and scroll to show those messages in the video. Thanks.

[eVoloshchak]

@eVoloshchak Could you share a video showing the effect of the new change? Please use a combination of messages in any chat and scroll to show those messages in the video. Thanks.

Sure thing

Web:

cinnamon-20220515-1.mp4

Android

22-05-15-21-25-45.mp4

[parasharrajat]

Ok, looking good to me. Go ahead and create the PR.

[eVoloshchak]

Ok, looking good to me. Go ahead and create the PR.

PR is up

[JmillsExpensify]

Looks like the linked PR made it to production last week. I'm mainly updating the issue title to account for the regression period. Payment later this week!

[JmillsExpensify]

Payment issued for @eVoloshchak. @parasharrajat I'll circle back on you tomorrow, as I just sent the offer. Thanks both!

[JmillsExpensify]

@parasharrajat is now covered, so I'm closing this out.