Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avatars blink all over the app #21658

Open
ulisesmac opened this issue Nov 20, 2024 · 4 comments
Open

Avatars blink all over the app #21658

ulisesmac opened this issue Nov 20, 2024 · 4 comments
Assignees
Labels
performance ui-polish Aesthetics; not a functional bug
Milestone

Comments

@ulisesmac
Copy link
Contributor

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:

video_2024-11-20_17-11-34.mp4

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 ulisesmac added performance ui-polish Aesthetics; not a functional bug labels Nov 20, 2024
@ulisesmac ulisesmac added this to the 2.32.0 milestone Nov 20, 2024
@ulisesmac ulisesmac self-assigned this Nov 20, 2024
@ulisesmac
Copy link
Contributor Author

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:

before.mp4

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

after.mp4

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
Copy link
Contributor

ilmotta commented Nov 21, 2024

Hey, to add to this issue and to this other issue #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
Copy link
Contributor Author

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

@ilmotta
Copy link
Contributor

ilmotta commented Nov 21, 2024

Our guidelines seem to cover already the problem of calling components as functions. Maybe worth improving the guideline or mentioning it more often https://github.com/status-im/status-mobile/blob/5442a8567c8ca3979a619d7356cd16b090312857/doc/new-guidelines.md#use--instead-of--in-reagent-components

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance ui-polish Aesthetics; not a functional bug
Projects
Status: No status
Development

No branches or pull requests

2 participants