[$250] [HOLD for payment 2024-11-13] Fix performance tests failing when migrating to useOnyx()

[lakchote]

In #50544 issue , migrating from withOnyx() to useOnyx() caused the performance tests to fail (see discussion here).

Investigate why and fix it in order to be able to use useOnyx() and solve the lint issue.

Issue Owner

Current Issue Owner: @

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021856147537785934932
• Upwork Job ID: 1856147537785934932
• Last Price Increase: 2024-11-12

[lakchote]

@blazejkustra or @fabioh8010 which one of you two would like to work on this?

[blazejkustra]

I can look into this 👀

[rojiphil]

@lakchote Do you need a C+ here? If needed, I can help as I have the context.

[melvin-bot]

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lakchote]

@rojiphil yes I'm assigning you as the C+ reviewer, as you have context on the underlying issue.

[blazejkustra]

Update: I started investigating yesterday (WIP PR). It looks like the only hook that increases re-renders is the modal one:

const [modal] = useOnyx(ONYXKEYS.MODAL);

I tried moving this hook to a wrapper, checking if value is loaded but always with the same result - more renders and failing perf tests. That's why I started digging deeper in react-native-onyx and reassure libraries.

For now my findings are that modal hook make the component re-render even when value is left unchanged. This is the reason why we see this weird pattern in the app (it looks like a perfect example where useOnyx(ONYXKEYS.MODAL), should be used):

    const [modal, setModal] = useState<Modal>({
        willAlertModalBecomeVisible: false,
        isVisible: false,
    });

    useEffect(() => {
        const unsubscribeOnyxModal = onyxSubscribe({
            key: ONYXKEYS.MODAL,
            callback: (modalArg) => {
                if (modalArg === undefined) {
                    return;
                }
                setModal(modalArg);
            },
        });
        return () => {
            unsubscribeOnyxModal();
        };
    }, []);

My guess is that there is a problem in react-native-onyx implementation but I'll keep digging.

Maybe you have some additional context that you'd like to share? @rojiphil @daledah (as you introduced onyxSubscribe in EdicationalTooltip)

[blazejkustra]

Update:
Internally useOnyx uses useSyncExternalStore, and there is a known issue with useSyncExternalStore that will be fixed in React 19. Basically useSyncExternalStore purposely tries to render as quick as possible, sometimes it won’t batch the updates - this means that a slower withOnyx might be causing less re-renders (and same when using onyxSubscribe as it doesn't use useSyncExternalStore)

From my research I observed that there are a lot of frequent updates to the ONYXKEYS.MODAL key, meaning more renders caused by useOnyx. However that's only the case in performance tests, in the app I don't see any differences in re-renders which is making it really hard to debug.

useOnyx

use-onyx.mov

withOnyx

with-onyx.mov

My proposal is to ignore failing perf tests and accept more re-renders, my reasoning it that in real environment there is no difference in render count.

[lakchote]

@blazejkustra so when the known issue with useSyncExternalStore is solved with React 19, we shouldn't have the problem anymore in our performance tests, right?

Does this mean we'll may encounter the same problem down the road with faulty performance tests that are not truly indicative of a re-rendering performance issue until then? If yes, I think we should get more eyes on this by posting in #expensify-open-source.

[blazejkustra]

so when the facebook/react#24831 is solved with React 19, we shouldn't have the problem anymore in our performance tests, right?

Yes, that's my assumption. If these onyx updates are triggered in a short period of time React is going to batch them, resulting in less renders.

Does this mean we'll may encounter the same problem down the road with faulty performance tests that are not truly indicative of a re-rendering performance issue

@lakchote It's more complicated than that, performance tests try to recreate the real environment as best as possible, but we can't avoid mocking a bunch of libraries, providers and components. It usually doesn't make a big difference, but I'm sure we have some tests that doesn't bring much value OR more importantly doesn't recreate the real scenario. There is even a proposal that aims to fix this, and issue, therefore I don't think we need to post it on open-source channel.

[blazejkustra]

On top of that, the failing test will be removed as a result of this PR 😅

I'll finish the PR tomorrow morning (draft available here)

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.57-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Migrate ComposerWithSuggestions to useOnyx #51573

If no regressions arise, payment will be issued on 2024-11-13. 🎊

For reference, here are some details about the assignees on this issue:

• @rojiphil requires payment (Needs manual offer from BZ)
• @blazejkustra does not require payment (Contractor)

[melvin-bot]

@rojiphil @sakluger The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[rojiphil]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:
  This is part of the migration work from withOnyx to useOnyx

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment:

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion: Not applicable here.

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

As this was migration work, we can skip this step as no new test case is introduced here.

Do we agree 👍 or 👎

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~021856147537785934932

[melvin-bot]

Current assignee @rojiphil is eligible for the External assigner, not assigning anyone new.

[sakluger]

Summarizing payment on this issue:

Contributor: @blazejkustra - No payment required, contractor
Contributor+: @ $250, Sent offer via Upwork: https://www.upwork.com/nx/wm/offer/104853498

[rojiphil]

@sakluger Accepted the offer. Thanks.

[sakluger]

All paid, thanks!