Hold for #38909 [$500] IOU - Map is blinking when navigate to Distance request

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.56-0
Reproducible in staging?: y
Reproducible in production?: n
Issue reported by: Applause - Internal Team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to FAB> +> Distance> Complete the IOU flow
3. Navigate to created Distance request
4. Go to details page and go back to Distance card page
5. Repeat the step 4 and take a not of map behavior

Expected Result:

Map should be stable

Actual Result:

Map is blinking when navigate to created Distance request

Workaround:

n/a

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6423445_1711129043210.Recording__2673.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01b572848dc86f02bf
• Upwork Job ID: 1771279748516020224
• Last Price Increase: 2024-03-29
• Automatic offers:
• paultsimura | Contributor | 0

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[melvin-bot]

Triggered auto assignment to @jasperhuangg (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[kbecciv]

We think that this bug might be related to #wave-collect RELEASE 1

[jasperhuangg]

Definitely don't need to block deploy as this is a display bug that doesn't really break usage of this feature

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mkhutornyi (External)

[lanitochka17]

During testing, another issue was found:
Map reloads with "Route pending" when clicking Save button without any changes
Steps:

1. Go to staging.new.expensify.com.
2. Go to workspace chat.
3. Create a distance request.
4. Go to request details page.
5. Click Distance.
6. Click Save button without editing the addresses.

Bug6423602_1711135650912.20240323_032324.mp4

Is this the same issue or do I need to open a separate one?

[paultsimura]

@lanitochka17 that is a different issue – the original one is about the Images, and it's happening all across the App (reported similar here: https://expensify.slack.com/archives/C049HHMV9SM/p1711121057387809).

The second one is about the broken logic of the "Edit Distance" flow - it modifies the Transaction while it shouldn't – a regression from #34610.

[ebcabaybay]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Map is blinking when navigating to created Distance request

What is the root cause of that problem?

In ReportScreen.tsx, this useEffect() triggers unneeded re-render to the map image component when Report.openReport(report.reportID) is called.

useEffect(() => {
        if (!isFocused || prevIsFocused || !ReportUtils.isChatThread(report) || report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
            return;
        }
        Report.openReport(report.reportID);

        // We don't want to run this useEffect every time `report` is changed
        // eslint-disable-next-line react-hooks/exhaustive-deps
    }, [prevIsFocused, report.notificationPreference, isFocused]);

What changes do you think we should make in order to solve the problem?

Add a new condition called isMoneyRequestView that determines if the current screen is a MoneyRequestView

Add this new condition to this line to immediately return instead of calling Report.openReport



if (isMoneyRequestView || !isFocused || prevIsFocused || !ReportUtils.isChatThread(report) || report.notificationPreference !== CONST.REPORT.NOTIFICATION_PREFERENCE.HIDDEN) {
            return;
        }

Result

expensify.mov

[WikusKriek]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Map and other attachment previews are blinking/ re rendering

What is the root cause of that problem?

This PR here #30269 introduced the issue. the problem is

App/src/pages/home/report/ReportActionsView.tsx

Line 432 in 4f22381

listID={listID}

The listID changes every time isLoadingInitialReportActions changes and that happens when we call Report.openReport which is a lot. When listID is updated the flatList renders again and we get the flashing of theReportActionsList images and preview images.

What changes do you think we should make in order to solve the problem?

We should only re render the ReportActionsList when it is needed, listID should therefore only be changed when we need to re render. I am having trouble Identifying exaclty what the solution should be.

App/src/pages/home/report/ReportActionsView.tsx

Lines 135 to 142 in 4f22381

	const listID = useMemo(() => {
	isFirstLinkedActionRender.current = true;
	const newID = generateNewRandomInt(listOldID, 1, Number.MAX_SAFE_INTEGER);
	listOldID = newID;
	return newID;
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [route, isLoadingInitialReportActions]);

We can change the dependency array to not include isLoadingInitialReportActions, but rather use allReportActions. My gut tells me this should only re render when the ReportActions are different or newer.
However the code is not commented and I am struggling to see the need for listID, also when and why to change it. I think the Author of the PR can maybe assist here.

What alternative solutions did you explore? (Optional)

Early return listOldID when we don't want to re render the flatList.

if (!actionListUpdated) {
     return listOldID;
}
        

[WikusKriek]

Screen.Recording.2024-03-23.at.21.26.05.mov

This issue happens in the report action view and you can see the avatars and everything flash when switching chats. It is not just the distance request.

[mkhutornyi]

@paultsimura looks like you have better context. Are you interested in taking this as C+?

[paultsimura]

I can take it, thanks @mkhutornyi

[ikevin127]

Proposal

Please re-state the problem that we are trying to solve in this issue

Map is blinking when navigate to Distance request.

What is the root cause of that problem?

TLDR: The RCA was already mentioned in a previous proposal.

Offending PR #30269 introduced the following logic:

App/src/pages/home/report/ReportActionsView.tsx

Lines 135 to 141 in fed9d6c

	const listID = useMemo(() => {
	isFirstLinkedActionRender.current = true;
	const newID = generateNewRandomInt(listOldID, 1, Number.MAX_SAFE_INTEGER);
	listOldID = newID;
	return newID;
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, [route, isLoadingInitialReportActions]);

which generates a new listID everytime OpenReport is called (isLoadingInitialReportActions).

The problem lies in the fact that listID is passed as key to the FlatList component here, which force re-renders the entire component (with all its children), creating the flicker behaviour.

What changes do you think we should make in order to solve the problem?

We should pass the listID to the FlatList's extraData array here instead of passing it as a Reeact.key to the FlatList.

Why use extraData instead of key ?

.
extraData should be used in a FlatList to inform the component to re-render when there is a change in state or props outside of the data prop, and the appearance of our items depends on this state or props. Since extraData is explicitly designed for this purpose, when it changes, React Native will only re-render the items that are affected by the change. This is generally efficient and avoids unnecessary renders.

On the other hand, the key prop is meant for a unique identifier for the entire list to help React with the reconciliation process. It is not intended to be used as a mechanism to control the re-render of a FlatList. Changing keys as a way to force a re-render is generally not recommended because it tells React that the entire component (with all its children) is completely different from the one before. This causes React to unmount the old component and mount a new component instead of efficiently updating the existing one. This process is more expensive in terms of performance, can lead to a flicker as mentioned, and can reset internal component state.

Using the key prop on the FlatList itself to force rerenders can lead to a visible flicker because React will remount the entire list and its items, which is a more costly operation than simply re-rendering items that have changed. The remounting process would cause a complete rebuild of the list structure, leading to visible flicker as old items are removed and new ones are added.

Videos (before / after)

MacOS: Chrome / Safari

Before	After
before.mov	after.mov

[melvin-bot]

Triggered auto assignment to @laurenreidexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

📣 @paultsimura 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[ikevin127]

I'd like to ask @perunt as author of PR #30269 why the listID was passed to FlatList key instead of being passed to the existing extraData FlatList prop.

It would be good to get some context on this so we can avoid a conflict between the PR's intended functionality and this issue's possible fix.

[paultsimura]

Thanks for the proposals folks, apparently the original PR author @perunt was working on a fix in parallel – so this issue should be resolved by #38909.
Unfortunately, none of the proposals was 100% similar to the final solution, even though they were close.

[ikevin127]

I think this should be put on hold for until the PR #39040 is merged then re-test as I'm not entirely sure passing listID as (React) key to the FlatList is the best idea when we have the extraData prop. Also the mentioned PR is fixing #38909 (issue), but might not fix this issue.

I commented on the PR #39040 (comment) kindly asking for a comment to be added above the declaration of listID explaining its role and the reason behind why it's passed to the FlatList as key.

[paultsimura]

I've tested and that PR fixes this issue, but I don't mind putting this on hold for #38909 just to be safe.
cc: @laurenreidexpensify

[perunt]

I'd like to ask @perunt as author of PR #30269 why the listID was passed to FlatList key instead of being passed to the existing extraData FlatList prop.

I'll dupe my message here as well:

So, here's the deal: I need to make the whole list refresh itself from scratch. It's all about the virtualization thing the list does. Basically, this refresh trick is mostly needed when you're linking comments, especially when jumping from looking at a bunch of messages to just a few. The virtualization in FlatList gets a bit wonky when the amount of stuff shown shrinks as you move to a specific message. So, to keep everything looking right, we gotta refresh the whole list, not just poke the FlatList to update what it's already showing. So if we put it to extraData the comment linking would lead to wrong place in some specific cases

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[perunt]

Hey guys! Just a quick heads-up, this PR has been merged. So this shouldn't be an issue anymore

[paultsimura]

@laurenreidexpensify we can close this issue – it's not reproducible on my end anymore