status-im / status-mobile

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

Fix blink while propagating re-frame state on Wallet #18702

Open ulisesmac opened 5 months ago

ulisesmac commented 5 months ago

Bug Report

Problem

When we update re-frame's app-db and navigate in the same event handler, sometimes the screen we navigate to is subscribed to the previously updated values, and sometimes the new screen has nil subscription values when it is initalized.

In this (long) investigation/discussion: https://github.com/status-im/status-mobile/pull/18532#issuecomment-1909279658 there were two solutions proposed:

  1. Delay the navigation for some ms to properly update re-frame's app-db state.
  2. Navigate to the next screen with incomplete data, but show a loader (or something similar).

To fix the issue #18532 the second approach was taken, and the screen has a quick blink:

Screencast from 2024-02-01 20-23-33.webm

Why is this important?

The main reasons are:

  1. The blink may create a bad UX, because these values are loading too fast (around 25ms in my local measurements), so it's too short to show a loader.
  2. We have to extremely flexibilize the components:
    • Any component can take a subscription's value when initialized, and that value could be nil, so we should make the components able to receive nil for all of their props.
    • We won't be able to write components listening for different props on initalization and on re-render (an example in comments).

So we should solve it prioritizing 1 but ideally also avoiding 2.

ulisesmac commented 5 months ago

Example component:

;; In this example, we don't want to react to changes on `data-or-fns`.
;;
;; Probably because we know they won't change during this component's lifecycle
;; and maybe we apply a costly transformation on it. 
;;
;; If used well, it can be used as an optimization technique.

(defn my-component [_ data-or-fns] ;; <- 2 params
  (let [derived-data-or-fns (something-maybe-expensive data-or-fns)]
    (fn [props] ;; <- 1 param, we won't rerender on `data-or-fns` change
      (let [derived-props (something-else props)]
        [my-other-component
         {:props derived-props
          :data  derived-data-or-fns}])))) ; <- No matter how many times `props` change,
                                           ;    `derived-data-or-fns` won't be recalculated again.

We wouldn't be able to write this kind of components, because data-or-fns might depend on a subscription's value that is nil the first render, and we will need to trigger a re-render when the value is no longer nil. So components must be written as:

;; Form 1 component
(defn my-component [props data-or-fns] ;; <- 2 params
  (let [derived-data-or-fns (expensive-something data-or-fns)
        derived-props       (something-else props)]
    [my-other-component
     {:props derived-props
      :data  derived-data-or-fns}]))

We listen to changes in all the props (those 2 params) the reason is that any param (data-or-fns included) can potentially be nil if it comes from re-frame or it's re-frame derived. Notice this restriction extends to the internal my-other-component too.

ulisesmac commented 5 months ago

Tagging involved devs in the previous discussion: :eyes: @ilmotta @Parveshdhull @briansztamfater @OmarBasem