status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.91k stars 984 forks source link

fix: avoid re-renders inside contact-list for new-chat sheet #21640

Open seanstrom opened 2 days ago

seanstrom commented 2 days ago

fixes https://github.com/status-im/status-mobile/issues/20532

Summary

Platforms

Areas that maybe impacted

Functional

Steps to test

Before and after screenshots comparison

Note, that in these screen recordings I've intentionally duplicated my contacts in the contact list because I didn't have enough contacts added to my profiles. So that's why there's repeated data and multiple items selected when selecting a single contact.

Status iOS (if available) Android (if available)
Before
After

status: ready

status-im-auto commented 2 days ago

Jenkins Builds

Click to see older builds (12) | :grey_question: | Commit | :hash: | Finished (UTC) | Duration | Platform | Result | |-|-|-|-|-|-|-| | :x: | 014f4468 | [#1](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21640/1/) | 2024-11-19 21:09:48 | ~3 min | `tests` | [:page_facing_up:`log`](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21640/1/consoleText) | | :heavy_check_mark: | 014f4468 | [#1](https://ci.status.im/job/status-mobile/job/prs/job/android-e2e/job/PR-21640/1/) | 2024-11-19 21:15:41 | ~9 min | `android-e2e` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241119-210633-014f44-pr21640-x86_64.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241119-210633-014f44-pr21640-x86_64.apk)| | :heavy_check_mark: | 014f4468 | [#1](https://ci.status.im/job/status-mobile/job/prs/job/ios/job/PR-21640/1/) | 2024-11-19 21:15:41 | ~9 min | `ios` | [:iphone:`ipa`](https://i.diawi.com/1nzKS6) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fi.diawi.com%2F1nzKS6)| | :heavy_check_mark: | 014f4468 | [#1](https://ci.status.im/job/status-mobile/job/prs/job/android/job/PR-21640/1/) | 2024-11-19 21:16:11 | ~9 min | `android` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241119-210633-014f44-pr21640-arm64-v8a.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241119-210633-014f44-pr21640-arm64-v8a.apk)| | | | | | | | | | :heavy_check_mark: | 7021d5ec | [#2](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21640/2/) | 2024-11-19 21:55:39 | ~4 min | `tests` | [:page_facing_up:`log`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241119-215103-7021d5-pr21640-tests.log) | | :heavy_check_mark: | 7021d5ec | [#2](https://ci.status.im/job/status-mobile/job/prs/job/android-e2e/job/PR-21640/2/) | 2024-11-19 21:58:42 | ~7 min | `android-e2e` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241119-215103-7021d5-pr21640-x86_64.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241119-215103-7021d5-pr21640-x86_64.apk)| | :heavy_check_mark: | 7021d5ec | [#2](https://ci.status.im/job/status-mobile/job/prs/job/android/job/PR-21640/2/) | 2024-11-19 21:59:12 | ~8 min | `android` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241119-215103-7021d5-pr21640-arm64-v8a.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241119-215103-7021d5-pr21640-arm64-v8a.apk)| | :heavy_check_mark: | 7021d5ec | [#2](https://ci.status.im/job/status-mobile/job/prs/job/ios/job/PR-21640/2/) | 2024-11-19 21:59:36 | ~8 min | `ios` | [:iphone:`ipa`](https://i.diawi.com/uDvyfM) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fi.diawi.com%2FuDvyfM)| | | | | | | | | | :x: | 85cab428 | [#3](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21640/3/) | 2024-11-21 18:21:34 | ~3 min | `tests` | [:page_facing_up:`log`](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21640/3/consoleText) | | :heavy_check_mark: | 85cab428 | [#3](https://ci.status.im/job/status-mobile/job/prs/job/android-e2e/job/PR-21640/3/) | 2024-11-21 18:27:01 | ~8 min | `android-e2e` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241121-181815-85cab4-pr21640-x86_64.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241121-181815-85cab4-pr21640-x86_64.apk)| | :heavy_check_mark: | 85cab428 | [#3](https://ci.status.im/job/status-mobile/job/prs/job/ios/job/PR-21640/3/) | 2024-11-21 18:27:05 | ~8 min | `ios` | [:iphone:`ipa`](https://i.diawi.com/y1ufSb) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fi.diawi.com%2Fy1ufSb)| | :heavy_check_mark: | 85cab428 | [#3](https://ci.status.im/job/status-mobile/job/prs/job/android/job/PR-21640/3/) | 2024-11-21 18:27:30 | ~9 min | `android` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241121-181815-85cab4-pr21640-arm64-v8a.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241121-181815-85cab4-pr21640-arm64-v8a.apk)|
:grey_question: Commit :hash: Finished (UTC) Duration Platform Result
:x: 5e52f2e7 #4 2024-11-21 19:01:59 ~4 min tests :page_facing_up:log
:heavy_check_mark: 5e52f2e7 #4 2024-11-21 19:06:12 ~8 min android-e2e :robot:apk :calling:
:heavy_check_mark: 5e52f2e7 #4 2024-11-21 19:06:40 ~9 min android :robot:apk :calling:
:heavy_check_mark: 5e52f2e7 #4 2024-11-21 19:07:52 ~10 min ios :iphone:ipa :calling:
:x: afe90d0e #5 2024-11-21 19:31:19 ~2 min tests :page_facing_up:log
:heavy_check_mark: afe90d0e #5 2024-11-21 19:36:09 ~7 min android-e2e :robot:apk :calling:
:heavy_check_mark: afe90d0e #5 2024-11-21 19:36:39 ~8 min android :robot:apk :calling:
:heavy_check_mark: afe90d0e #5 2024-11-21 19:36:52 ~8 min ios :iphone:ipa :calling:
ulisesmac commented 2 days ago

Hi @seanstrom !

Thank you for this PR.

The avatar component blinks all over the app, I was recently working in the onboarding and found the avatar is using:

https://github.com/status-im/status-mobile/blob/a1e3bb3bd6906c8b74f0f9ded660e9f9d8a8095e/src/quo/components/common/no_flicker_image.cljs#L35

but I don't think that approach is being effective.

I also noticed this avatar rerender flickering in my latest PR:


About the solution, I have several things to say.

A flat-list shouldn't re-render its content when we disable/enable its scroll. Are we sure a plain React Native's FlatList behaves like that? we have a wrapper that I've always suspected is doing some extra computations, so I'd start answering this question. If the blame is from our wrapper:

We could check what are the dependencies causing the re-renders and fix them (or directly fix the wrapper).

Otherwise, even though the flat-list items are being re-rendered, the avatar is receiving the exact same props, so we are probably passing some extra data that is causing the re-render.

So, I'd say the root cause and solution both rely in a deeper issue. WDYT?

seanstrom commented 20 hours ago

@ulisesmac I decided to take a deeper look at what was happening inside these components based on your feedback.

There seems to be a variety of issues, which I've tried to address in some new changes. For example:

Thank you again for the feedback! It helped uncover some weird glitches that may now be resolved, and it seems we can even keep using scroll-enabled 🙌

ulisesmac commented 14 hours ago

@seanstrom

Great! seems you've found the underlying issues :tada:

For:

We need to use a memoized function inside our flat-list/section-list wrapper, because otherwise we'll be passing a new function reference whenever it re-renders.

We need to use a memoized function when passing around the on-scroll handler to bottom-sheets, otherwise we'll be passing a new function reference to the section-list / flat-list wrapper on each render.

This is what I was recently talking about with @ilmotta, at the end seems problems are related to passing unstable references. Let's just be careful about adding extra complexity to the implementations, just in case that happens.

And it seems like we've been generating/formatting urls to profile images on each render, which should have been fine, but it seems we were embedding a timestamp in the url. And that that timestamp seemed to be generated on each render from inside the format function. So if the user avatar component would re-render, then the image would reload because the timestamp would change. Moving forward we'll allow for passing in the timestamp to the function instead of generating one from the inside.

Great finding! We could ask what is the purpose of that timestamp and if it could be simplified. cc: @ilmotta Do you know why do we need it?

ilmotta commented 13 hours ago

We could ask what is the purpose of that timestamp and if it could be simplified. cc: ilmotta Do you know why do we need it?

@ulisesmac @seanstrom I don't see the clock URL parameter being used by the handlers.ParseImageParams function in status-go, which is used to process endpoints like /accountImages and /contactImages. It looks like a leftover in the mobile client which was useful in the past, but can be safely removed now. I'd still check if everything works before removing clock.