ulisesmac commented 5 days ago

Bug Report

Problem

The avatas are constantly blinking all over the app, this leads to a bad UX and also shows that probably we are re-rendering more ofthen than needed.

Expected behavior

Not to re-render

Actual behavior

Re-renders all over the app, check this video with a few examples:

https://github.com/user-attachments/assets/4e4e57c0-6b55-41aa-b89f-b80df35d9d7a

Reproduction

Additional Information

This issue has been created to investigsate the root cause, there's a no-flicker-image component, so we should check if that is being properly used.

ulisesmac commented 4 days ago

Update:

Investigated the issue for the recent chats listing, and, at least for this usage, it's due to a flat-list misuse. IMO since our flat-list wrapper renames props and performs some transformations on the props passed, its usage is more confusing.

I do understand that probably this wrapper was made to help developers use a flat-list in Clojure + reagent, but I'd probably simplify our wrapper and let the developer decide how to use a flat-list.

Anyway, that is a discussion we can have later. In the meantime I noticed each time a message arrives a rerender happens:

https://github.com/user-attachments/assets/e1df2034-be63-426a-9e02-370d0e89e1b1

Now by providing a stable reference to the render function (a "reagent component"), the issue's been solved:

https://github.com/user-attachments/assets/005d905b-3eed-46c7-ae3c-c74ddc0391bd

This issue is pretty similar to what happened a while ago, where we created functional components inline as:

[:f> (fn [] [my-hiccup])]

So the component was recreated in each re-render.

BTW, I've seen our bottom sheets API is also indirectly encouraging this to happen since we pass a component defined inline, and sometimes bottom-sheets blink, we should investigate this too! 👀

CC: @flexsurfer @seanstrom @ilmotta

ilmotta commented 4 days ago

Hey, to add to this issue and to this other issue https://github.com/status-im/status-mobile/pull/21640, it does seem like in some cases the implementation is not following some basic guidelines.

I fixed the avatar blink issue in the chat Recent tab by fixing the code that was not using the :render-data prop for the rn/flat-list component and was calling the component as if it's a function, not with the vector syntax. If you look at the code surrounding the component below you will see that it's passing certain props that are always different (like uncached functions) but this was not the cause of the flicker.

This is the whole diff/fix:

modified   src/status_im/contexts/chat/home/chat_list_item/view.cljs
@@ -282,7 +282,8 @@

 (defn chat-list-item
   [{:keys [chat-id chat-type]
-    :as   item} theme]
+    :as   item}
+   _ _ {:keys [theme]}]
   (let [customization-color (rf/sub [:profile/customization-color])]
     [rn/touchable-highlight
      {:style          style/container
modified   src/status_im/contexts/chat/home/view.cljs
@@ -62,8 +62,8 @@
         :on-end-reached                    #(re-frame/dispatch [:chat/show-more-chats])
         :keyboard-should-persist-taps      :always
         :data                              items
-        :render-fn                         (fn [item]
-                                             (chat-list-item/chat-list-item item theme))
+        :render-data                       {:theme theme}
+        :render-fn                         chat-list-item/chat-list-item

ulisesmac commented 4 days ago

@ilmotta Yeah, now you are providing a stable reference as component 👍 This is the fix I also applied.