[$250] Expense - Page blinks when opening Hold RHP from report details page

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.1-0
Reproducible in staging?: y
Reproducible in production?: no, new feature
If this was caught during regression testing, add the test name, ID and link from TestRail: Exp around https://expensify.testrail.io/index.php?/tests/view/4661954
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

1. Go to staging.new.expensify.com
2. Go to any DM.
3. Submit two expenses.
4. Go to transaction thread of any expense.
5. Click on the chat header.
6. Click Hold.

Expected Result:

Page will not blink when opening Hold RHP from report details page.

Actual Result:

Page blinks when opening Hold RHP from report details page.

Workaround:

unknown

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

Bug6521446_1719056245054.20240622_193147.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01849f37018df4c9e8
• Upwork Job ID: 1805127337823292445
• Last Price Increase: 2024-06-24

Issue Owner

Current Issue Owner: @ishpaul777

[melvin-bot]

Triggered auto assignment to @stephanieelliott (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.

[melvin-bot]

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

[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.

[m-natarajan]

@nkuoch FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[m-natarajan]

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

[bernhardoj]

Proposal

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

The page behind the RHP looks like blinking when pressing the hold button in details page.

What is the root cause of that problem?

When we choose the hold button, we do 2 things. First, dismiss the modal, and second, navigate to the hold reason page.

App/src/components/PromotedActionsBar.tsx

Lines 68 to 71 in 0176052

	onSelected: () => {
	Navigation.dismissModal();
	ReportUtils.changeMoneyRequestHoldStatus(reportAction);
	},

App/src/libs/ReportUtils.ts

Lines 2808 to 2809 in 0176052

	const activeRoute = encodeURIComponent(Navigation.getActiveRouteWithoutParams());
	Navigation.navigate(ROUTES.MONEY_REQUEST_HOLD_REASON.getRoute(policy?.type ?? CONST.POLICY.TYPE.PERSONAL, transactionID, reportAction.childReportID ?? '', activeRoute));

If we see the video slowly, we can notice that it's not blinking but the multiple navigations are happening at the same time causing the RHP overlay to become darker and then lighter.

Screen.Recording.2024-06-23.at.18.26.47.mov

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

I don't know if it's expected to dismiss the modal when choosing the hold button, but if it's not, then we can remove it.

Screen.Recording.2024-06-23.at.18.32.56.mov

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

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

[melvin-bot]

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

[ishpaul777]

Thanks for your proposal @bernhardoj, I noticed that if we use yor solution then we'll have this issue we'll have this navigation glitch issue after submitting the hold reason

Screen.Recording.2024-06-24.at.3.11.49.PM.mov

[bernhardoj]

@ishpaul777 I checked that it's because of the focus trap initial focus on the hold explanation page. That kind of sliding issue is similar to #43806 where we focus immediately while an animation is ongoing. This issue doesn't happen when you submit the hold with a mouse. One way to solve this is to delay the focus-trap by CONST.ANIMATED_TRANSITION (300ms), adding the below code to focusTrapOptions.
checkCanFocusTrap: () => new Promise((resolve) => setTimeout(resolve, 300)),

App/src/components/FocusTrap/FocusTrapForScreen/index.web.tsx

Lines 43 to 47 in d7db203

	<FocusTrap
	active={isActive}
	paused={!isFocused}
	focusTrapOptions={{
	trapStack: sharedTrapStack,

[oleksolv]

Contributor details
Your Expensify account email: oleksolv@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01b8e1d347467f7557

Proposal

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

Page blinks when opening Hold RHP from report details page

What is the root cause of that problem?

The issue happens because the 2 Navigation Modal Items overlaps each other during close/open animation effect. So, the backdrop opacity of the 1st one overlaps the 2nd one during the opacity animation effect here:

App/src/styles/index.ts

Line 1906 in d7db203

opacity: current.progress.interpolate({

At some point the opacity sum of backdrops of the 1st modal and 2nd modal can give more than 0.72 (as was prescribed) and make the glitch effect of increasing backdrop opacity to 1 and back.

so we have the glitch effect.

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

Using the index property of useCardAnimation in BaseOverlay

App/src/libs/Navigation/AppNavigator/Navigators/Overlay/BaseOverlay.tsx

Line 22 in d7db203

const {current} = useCardAnimation();

and pass it to style can help us to undentify if we open next modal and turn off backdrop opacity animation, so we will have smooth effect without glitches

Demo

Demo-ExpenseApp-24.06.2024-oleksolv-Upwork.mov

[melvin-bot]

📣 @oleksolv! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[ishpaul777]

i am not confident with your RCA and solution for second problem @bernhardoj, mainly because i dont understand why this only happen after changes suggested in your Proposal, I guess the problem lies in this part of code that we'd have to return to report screen to navigate to hold educational page

App/src/components/MoneyRequestHeader.tsx

Lines 187 to 199 in 64dec34

	useEffect(() => {
	if (!shouldShowHoldMenu) {
	return;
	}
	if (isSmallScreenWidth) {
	if (Navigation.getActiveRoute().slice(1) === ROUTES.PROCESS_MONEY_REQUEST_HOLD) {
	Navigation.goBack();
	}
	} else {
	Navigation.navigate(ROUTES.PROCESS_MONEY_REQUEST_HOLD);
	}
	}, [isSmallScreenWidth, shouldShowHoldMenu]);

[oleksolv]

@ishpaul777

Sorry, I am first time contributing to Expense, do I need anything to add to my previous post, so you can test that my solution works for modals stack? :)

[bernhardoj]

@ishpaul777 after my change, pressing the Hold button won't close the details page.

[details, hold reason]

Submitting the hold reason will pop the hold reason page which animates the page to slide from right to left and the details page from left to right, and at the same time the hold explanation page is showing with animation interrupted.

[ishpaul777]

@oleksolv Sorry for not providing the feedback earlier, I believe your proposal dont fix the issue at its root so for now i have not considered your proposal for this issue.

[ishpaul777]

Submitting the hold reason will pop the hold reason page

I could be wrong @bernhardoj but as far as i understand Submitting the hold reason will navigate to report screen and then it navigates to hold reason page currently (backTo param), after removing Navigation.dismissModal() the backTo param on reason page will be details page and then in the background this useEffect run to navigate to navigated to Hold explaination page. Am I following correctly ?

[oleksolv]

@ishpaul777 If you do not mind I will provide some more details why it is the root issue and it will fix:

1. The project uses React Navigation and all navigation modals are separate views. This rules is applicable also for react-native-web.
2. When we call the navigate to modal we will open the new modal screen. In case of this issue we call:

     Navigation.dismissModal(); 
     ReportUtils.changeMoneyRequestHoldStatus(reportAction); 

First call will trigger close modal. It spends up to 600ms to close the existed modal and trigger open new modal up to 600ms to open the new one.

In this case each modal will have its backdrop: you can debug this in browser and see that the animation is in progress for both of the modals. One modal is interpolating from 0.72 - 0 and other from 0 - 0.72.

Here the code you can test, the PR is not ready for review but you can see the changes and the issue fixed for all modals that are calling one by one:
oleksolv#1

This solution will turn off the backdrop entering animation if the next screen is also modal.

[bernhardoj]

Yes, correct.

App/src/pages/iou/HoldReasonPage.tsx

Lines 56 to 69 in 2526e31

	const navigateBack = () => {
	Navigation.navigate(backTo);
	};
	const onSubmit = (values: FormOnyxValues<typeof ONYXKEYS.FORMS.MONEY_REQUEST_HOLD_FORM>) => {
	// We have extra isWorkspaceRequest condition since, for 1:1 requests, canEditMoneyRequest will rightly return false
	// as we do not allow requestee to edit fields like description and amount.
	// But, we still want the requestee to be able to put the request on hold
	if (ReportActionsUtils.isMoneyRequestAction(parentReportAction) && !ReportUtils.canEditMoneyRequest(parentReportAction) && isWorkspaceRequest) {
	return;
	}
	IOU.putOnHold(transactionID, values.comment, reportID);
	navigateBack();

[ishpaul777]

Hm then why not we directly navigate to Hold explaination page after submitting hold reason without the background useEffect, like we usually do for every page? Anything benifit of doing it the way it is now ?

[bernhardoj]

If you open a transaction report that is on hold, the hold explanation page will show because of the useEffect, so I believe that's the reason.

[ishpaul777]

@bernhardoj Can we disable the background navigation when the report screen is background, possibly by using useIsFocussed hook and then navigate to the page direct as we normally do?

[bernhardoj]

I think we can do that, but I see this PR is working on a fix for this issue too.

[ishpaul777]

Oh lets Hold for the PR to merge and then retest if there's any issue left us to solve. Thanks!

[stephanieelliott]

That PR was merged to prod yesterday! I'm not able to repro this one -- @ishpaul777 can you try testing and see if it's still happening for you?

[melvin-bot]

@nkuoch, @stephanieelliott, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ishpaul777]

Issue is fixed 🎉

Screen.Recording.2024-06-29.at.4.30.39.AM.mov

[stephanieelliott]

Yay! Thanks @ishpaul777