[HOLD for payment 2024-08-09] [$250] Categorizing - Page blinks after selecting a category during categorization

[lanitochka17]

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.11-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

Precondition:

• Account has no workspace

1. Go to staging.new.expensify.com
2. Go to self DM
3. Track a manual expense
4. Click Categorize it from the actionable whisper
5. Select any category

Expected Result:

Page will not blink after selecting a category during categorization

Actual Result:

Page blinks after selecting a category during categorization

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

Add any screenshot/video evidence

Bug6550721_1721752108588.bandicam_2024-07-24_00-25-27-455.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f0f7b8664b135266
• Upwork Job ID: 1815855751043659627
• Last Price Increase: 2024-07-23
• Automatic offers:
• eh2077 | Reviewer | 103288936

Issue Owner

Current Issue Owner: @jliexpensify

[melvin-bot]

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

[lanitochka17]

We think that this bug might be related to #vip-vsp

[francoisl]

The flicker is barely noticeable, not sure it's worth blocking on this.

There's also this open issue about the chat view flickering - not sure it's related but could be.

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @jliexpensify (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 Contributor-plus team member for initial proposal review - @eh2077 (External)

[bernhardoj]

Proposal

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

The page blinks when selecting category in categorize track expense flow.

What is the root cause of that problem?

When we don't have a workspace and trying to categorize an expense, it will open a category page first. Selecting a category will:

1. Go back
2. Navigate to confirmation page

App/src/pages/iou/request/step/IOURequestStepCategory.tsx

Lines 147 to 151 in 2bb1d00

	if (action === CONST.IOU.ACTION.CATEGORIZE) {
	Navigation.goBack();
	Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(action, iouType, transactionID, report?.reportID ?? '-1'));
	return;
	}

We go back to close the category selector page. But because the category page is last RHP page at the moment, going back means closing the RHP modal. Then, it's followed by a navigation to a confirmation page, which is an RHP too, so both RHP overlay "conflicting" each other.

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

When the category page is the only route in the navigator, instead of performing a go back and navigate action, we can just replace it with a REPLACE action, which will replace the category page with the confirmation page.

goBack(route, true);

Using goBack with forceFallback to true will do a REPLACE action.

OR

We can use navigate with a type of 'UP'.

To make it reusable, we can create a new function in Navigation lib called closeAndNavigate.

function closeAndNavigate(route: Route) {
    if (!navigationRef.current) return;

    const isFirstRouteInNavigator = !getActiveRouteIndex(navigationRef.current.getState());
    if (isFirstRouteInNavigator) {
        goBack(route, true);
        return;
    }
    goBack();
    navigate(route);
}

[eh2077]

@bernhardoj Thanks for the proposal.

Then, it's followed by a navigation to a confirmation page, which is an RHP too, so both RHP overlay "conflicting" each other.

Is it possible to explore a holistic approach? So that we can optimize this typical issue app wide. Maybe we can do some improvements for the navigation manager, wdyt?

[bernhardoj]

Hmm, can you explain more in what thing should we improve? There is 2 navigation action happen in this issue, are you suggesting that if there is a 'back' action followed by a 'navigate' action, then the navigation manager should convert them to 'replace' automatically instead of the developer needs to do it?

[eh2077]

are you suggesting that if there is a 'back' action followed by a 'navigate' action, then the navigation manager should convert them to 'replace' automatically instead of the developer needs to do it?

I think so. Does it make sense to you?

[dominictb]

@eh2077 My proposal has the holistic approach you're looking for!

Proposal

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

Page blinks after selecting a category during categorization

What is the root cause of that problem?

When we go back and navigate here

App/src/pages/iou/request/step/IOURequestStepCategory.tsx

Lines 147 to 151 in 2bb1d00

	if (action === CONST.IOU.ACTION.CATEGORIZE) {
	Navigation.goBack();
	Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(action, iouType, transactionID, report?.reportID ?? '-1'));
	return;
	}

, the category is the only page in the RHN, so the RHN is closed and reopens again and there's a brief period that 2 RHN overlays show at the same time, causing the visible blinking.

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

Create a method replaceRHNScreen like below so that in case the route being replaced is the only one in RHN, we'll use replace and not go back and navigate

const replaceRHNScreen = (currentScreen: string, nextRoute: string) => {
    const rhnInitialScreen = navigationRef.getState().routes.find((routes) => routes.name === NAVIGATORS.RIGHT_MODAL_NAVIGATOR)?.params?.screen;
    
    if (rhnInitialScreen === currentScreen) {
        Navigation.goBack(nextRoute, true);
    } else {
        Navigation.goBack();
        Navigation.navigate(nextRoute);
    }
}

Similar approach to search for a screen in RHN is used here.

Please note in case the route being replaced is not the only one in RHN, we must still use go back and navigate, if not there'll be a regression in the case of [screen A, screen B], if we do replace of screen B in this case, it will become [screen A, screen A], and when go back from screen A it will go to itself. Specifically in this case there'll be regression in case of going from confirmation page to the category, then select another category again.

Then we can replace

App/src/pages/iou/request/step/IOURequestStepCategory.tsx

Lines 148 to 149 in 2bb1d00

	Navigation.goBack();
	Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(action, iouType, transactionID, report?.reportID ?? '-1'));

with

replaceRHNScreen(screen, ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(action, iouType, transactionID, report?.reportID ?? '-1'));

The screen used is a child of the route.params that can be added here

What alternative solutions did you explore? (Optional)

Some enhancements to the above check:

• We can make it more specific by searching in the route tree to the exact route of RHN, in this case the result will be RightModalNavigator -> MoneyRequest -> Money_Request_Step_Category, if Money_Request_Step_Category is the only route in MoneyRequest and MoneyRequest is the only route in RightModalNavigator, we're sure if goBack from Money_Request_Step_Category will lead to RHN closing and we need to use the replace approach
• Using navigate with 'UP' could also work

[dominictb]

Updated proposal for more details

[melvin-bot]

Current assignee @roryabraham is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[dominictb]

My 2 cents:

@dominictb 's #46063 (comment) was posted later (during the discussion on the first proposal) and provides a more completed solution - they discussed detail differences of go back and navigate and replace.

@eh2077 Just to clarify @bernhardoj 's initial proposal causes regressions (the updated proposal has similar approach to mine).

a reusable solution though I believe we'll also get to it in PR.

I agree reusable part could be reached during PR review although it will still cause regressions, the approach suggested in my proposal

Create a method replaceRHNScreen like below so that in case the route being replaced is the only one in RHN, we'll use replace and not go back and navigate

is the only one that's regression free, and is first. My proposal also has correct RCA too.

And the difference between the approaches are major, not minor (my approach involves the first-route-in-RHN check which is not straight-forward or intuitive to figure out).

Looking forward to the final decision from @roryabraham 🙏

[roryabraham]

Well this is a tough call, because both proposals are good and multiple contributors brought value to the issue:

• @bernhardoj was first to post the root cause
• @eh2077 provided feedback on the proposal
• @dominictb was first to create a generalized solution (building off of the root cause and the feedback that was posted)
• @bernhardoj iterated on his proposal after @dominictb's proposal was posted (definitely a substantive edit, though it's not clear to me that the previous solution would've caused a regression)
• The final solutions (at the time I'm writing this) are not exactly the same. @dominictb's conditionally uses replace if the screen is the first in the RHP. @bernhardoj conditionally uses replace if the screen is the first in the current navigator. This could mean that @bernhardoj's solution works in LHP or other modals, and not just the RHP. Though it's not clear if we have any use-cases where that would come into play in the app today.

I hope you both can appreciate that I feel like there's no clear answer for which proposal to select; I'm not a perfect judge, and I have to just do my best to consider what's most fair and best for the end-result. In this case I'm going to select @bernhardoj's proposal.

Thank you all for working together to come to a good solution. 🙇🏼

[melvin-bot]

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

Offer link
Upwork job

[bernhardoj]

Thanks! 🙇
PR is ready.

cc: @eh2077

[dominictb]

definitely a substantive edit, though it's not clear to me that the previous solution would've caused a regression

I can provide reproducible steps to show that the previous solution has regressions if necessary.

The final solutions (at the time I'm writing this) are not exactly the same. @dominictb's conditionally uses replace if the screen is the first in the RHP. @bernhardoj conditionally uses replace if the screen is the first in the current navigator. This could mean that @bernhardoj's solution works in LHP or other modals, and not just the RHP. Though it's not clear if we have any use-cases where that would come into play in the app today.

TBH I don't think such a difference is significant in proposal stage, and can be polished in PR phase. So it can be said that the final solution used is mostly influenced from my solution.

because both proposals are good and multiple contributors brought value to the issue:

@roryabraham @eh2077 Would it be more fair if we split the compensation here?

It's fine if not 👍 , I'm just thinking if an alternate arrangement might work better for everyone here, considering the value and efforts both contributors put into helping resolve this are mostly equal.

[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.15-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix page blinks after selecting a category during categorization #46360

If no regressions arise, payment will be issued on 2024-08-09. 🎊

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

• @bernhardoj requires payment through NewDot Manual Requests
• @eh2077 requires payment automatic offer (Reviewer)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@eh2077] The PR that introduced the bug has been identified. Link to the PR:
• [@eh2077] 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:
• [@eh2077] A discussion in #expensify-bugs 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:
• [@eh2077] Determine if we should create a regression test for this bug.
• [@eh2077] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@jliexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[eh2077]

Checklist

• [@eh2077] The PR that introduced the bug has been identified. Link to the PR: I don't think there's a PR that's responsible for this. It's an edge use case we want to polish.
• [@eh2077] 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: No
• [@eh2077] A discussion in #expensify-bugs 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: No
• [@eh2077] Determine if we should create a regression test for this bug. Yes, it's actually a new feature we added though it's an edge case

Regression test

Precondition:

• Account has no workspace

1. Go to self DM
2. Track a manual expense
3. Click Categorize it from the actionable whisper
4. Select any category
5. Verify the confirmation page opens without any blink
6. Press the Category field to edit it
7. Change to a different category
8. Press the back button
9. Verify the confirmation page closes

[jliexpensify]

Payment Summary

• C: @bernhardoj $250 (New.Dot payment)
• C+: @eh2077 $250 Upwork job

[bernhardoj]

Requested in ND.

[JmillsExpensify]

$250 approved for @bernhardoj

[jliexpensify]

Apologies for the delay @eh2077 - paid and job closed!