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

[seanstrom]

fixes #20532

Summary

• This PR attempts to fix the contact list re-rendering issue when panning in the contact list or selecting a contact from the contact list inside the New Chat screen.
  • It seems that the contact-list is inside a bottom-sheet, and the bottom-sheet supports a drag gesture for closing the sheet. When this gesture occurs, it seems we attempt to prevent scrolling in the scroll-view by setting :scroll-enabled to false. However, when this prop is changed the scroll-view will re-render all the list items, which can cause the contact images to flicker and it seems to interfere with selecting a contact.
    • Here's a snippet of the code that handles setting the scroll-enabled reagent atom:

      status-mobile/src/status_im/common/bottom_sheet_screen/view.cljs

      Lines 16 to 36 in 014f446

      	(defn drag-gesture
      	[{:keys [translate-y opacity scroll-enabled? curr-scroll close reset-open-sheet set-animating-true]}]
      	(-> (gesture/gesture-pan)
      	(gesture/on-start (fn [e]
      	(set-animating-true)
      	(when (< (oops/oget e "velocityY") 0)
      	(reset! scroll-enabled? true))))
      	(gesture/on-update (fn [e]
      	(let [translation (oops/oget e "translationY")
      	progress (Math/abs (/ translation drag-threshold))]
      	(when (pos? translation)
      	(reanimated/set-shared-value translate-y translation)
      	(reanimated/set-shared-value opacity (- 1 (/ progress 5)))))))
      	(gesture/on-end (fn [e]
      	(if (> (oops/oget e "translationY") drag-threshold)
      	(close)
      	(reset-open-sheet))))
      	(gesture/on-finalize (fn [e]
      	(when (and (>= (oops/oget e "velocityY") 0)
      	(<= @curr-scroll (if platform/ios? -1 0)))
      	(reset! scroll-enabled? false))))))

  • One solution would be to re-write the drag gesture or the composition of the gesture handler with a scroll view. It's possible this could be done in a way that avoids disabling the scrolling and causing a re-render in the scroll-view.
  • Another solution would be to avoid setting scroll-enabled for the contact list, which solves the re-rendering issue, but does not allow the drag gesture to override the scroll gesture.
  • In this PR we currently use the second approach, which matches the way we implement this functionality for the manage-embers screen for group chats.
    • Note, the drag gesture still will work on the top portion of the bottom-sheet, and the user can still close the sheet by pressing the "X" icon.

Platforms

• Android
• iOS

Areas that maybe impacted

Functional

• Creating a new-chat

Steps to test

• Open the Status Mobile app
• Login to a profile
• Navigate to the messenger home screen
• Press the "+" icon and select "New Chat" from the bottom drawer
• If there's a contact list with many contacts, attempt to scroll the contact list by scrolling up and down
• Attempt to select a contact from the contact list
• Attempt to close the bottom-sheet by pulling the top of the bottom sheet down

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	Screen.Recording.2024-11-19.at.13.02.38.mov	Screen.Recording.2024-11-19.at.13.01.56.mov
After	Screen.Recording.2024-11-19.at.12.56.12.mov	Screen.Recording.2024-11-19.at.12.54.51.mov

status: ready

[status-im-auto]

Jenkins Builds

Click to see older builds (16)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	014f446	#1	2024-11-19 21:09:48	~3 min	tests	📄log
✔️	014f446	#1	2024-11-19 21:15:41	~9 min	android-e2e	🤖apk 📲
✔️	014f446	#1	2024-11-19 21:15:41	~9 min	ios	📱ipa 📲
✔️	014f446	#1	2024-11-19 21:16:11	~9 min	android	🤖apk 📲
✔️	7021d5e	#2	2024-11-19 21:55:39	~4 min	tests	📄log
✔️	7021d5e	#2	2024-11-19 21:58:42	~7 min	android-e2e	🤖apk 📲
✔️	7021d5e	#2	2024-11-19 21:59:12	~8 min	android	🤖apk 📲
✔️	7021d5e	#2	2024-11-19 21:59:36	~8 min	ios	📱ipa 📲
❌	85cab42	#3	2024-11-21 18:21:34	~3 min	tests	📄log
✔️	85cab42	#3	2024-11-21 18:27:01	~8 min	android-e2e	🤖apk 📲
✔️	85cab42	#3	2024-11-21 18:27:05	~8 min	ios	📱ipa 📲
✔️	85cab42	#3	2024-11-21 18:27:30	~9 min	android	🤖apk 📲
❌	5e52f2e	#4	2024-11-21 19:01:59	~4 min	tests	📄log
✔️	5e52f2e	#4	2024-11-21 19:06:12	~8 min	android-e2e	🤖apk 📲
✔️	5e52f2e	#4	2024-11-21 19:06:40	~9 min	android	🤖apk 📲
✔️	5e52f2e	#4	2024-11-21 19:07:52	~10 min	ios	📱ipa 📲

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	afe90d0	#5	2024-11-21 19:31:19	~2 min	tests	📄log
✔️	afe90d0	#5	2024-11-21 19:36:09	~7 min	android-e2e	🤖apk 📲
✔️	afe90d0	#5	2024-11-21 19:36:39	~8 min	android	🤖apk 📲
✔️	afe90d0	#5	2024-11-21 19:36:52	~8 min	ios	📱ipa 📲
❌	d98de2e	#6	2024-11-22 17:26:49	~3 min	tests	📄log
✔️	d98de2e	#6	2024-11-22 17:30:28	~7 min	android-e2e	🤖apk 📲
✔️	d98de2e	#6	2024-11-22 17:34:54	~11 min	android	🤖apk 📲
✔️	d98de2e	#6	2024-11-22 17:35:25	~12 min	ios	📱ipa 📲

[ulisesmac]

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:

status-mobile/src/quo/components/common/no_flicker_image.cljs

Line 35 in a1e3bb3

(defn image

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

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

• [#21436] List of users overlap composer input #21641 (if you want to check it)

---

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]

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

• 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.
• 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.

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 🙌

[ilmotta]

What is p4 if there are only 3 deps?

[seanstrom]

So I think p1 p2 p3 p4 are the arguments that are passed to the render function. So in this case p1 is the item in the list, p2 is the index, p3 is a reference to the separators in the list, and p4 is the render-data that's passed from the list component.

More details can be found here:

status-mobile/src/react_native/flat_list.cljs

Lines 8 to 13 in afe90d0

	(defn- wrap-render-fn
	[f render-data]
	(fn [^js data]
	(reagent/as-element [f (.-item data) (.-index data)
	(.-separators data) render-data
	(.-isActive data) (.-drag data)])))

[ilmotta]

Thanks, my brain farted and was confused with use-callback.

[ilmotta]

This is exactly the fix I found here #21658 (comment) coincidentally. Thanks for the fix @seanstrom

[ulisesmac]

And the same I'm applying in my WIP for:

• Avatars blink all over the app #21658

@seanstrom Are you fixing all the avatar usages in this PR or just this one? I'm asking because we don't want to duplicate work

[seanstrom]

This PR is only intending to fix the usage of the reloading of the avatar images inside the contact-list view for a new-chat, but it's possible that some of the changes related to the image source uri will affect other usages too.

[ulisesmac]

Got it, then I'll continue with

• Avatars blink all over the app #21658

Once this PR is merged. Thanks for all the fixes

[ulisesmac]

@seanstrom

Great! seems you've found the underlying issues 🎉

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]

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.