[HOLD for payment 2024-08-29] [$500] previous view is shown quickly after navigating back to expense report

[m-natarajan]

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.2-2
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @marcochavezf
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1719442541994939

Action Performed:

1. Submit a couple of expenses in a workspace chat
2. Open the expense report
3. Tap on the first expense
4. Navigate back to the expense report

Expected Result:

There should be no glitches

Actual Result:

Previous view is shown quickly when navigating back to the expense report

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

RPReplay_Final1719442140.MP4

XZHV6030.1.MP4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01be54bab9b49fe845
• Upwork Job ID: 1806376578077397365
• Last Price Increase: 2024-07-29
• Automatic offers:
• ishpaul777 | Reviewer | 103312743

[melvin-bot]

Triggered auto assignment to @greg-schroeder (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]

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

[melvin-bot]

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

[nkdengineer]

Proposal

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

Previous view is shown quickly when navigating back to the expense report

What is the root cause of that problem?

This is a known issue that we thought we fixed it. Read more in https://github.com/Expensify/App/blob/main/src/components/ScreenWrapper.tsx#L193-L194

But it was not working as expected because the Keyboard.dismiss(); in

App/src/components/ScreenWrapper.tsx

Line 201 in 3622deb

Keyboard.dismiss();

will happen at the same time as the going back navigation, so still a jarring experience.

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

We need to make sure the keyboard is fully dismissed first before navigating back animation happens

We'll make use of InteractionManager.runAfterInteractions for this. This block of code can be updated to:

? navigation.addListener('beforeRemove', (e) => {                
    if (!isKeyboardShownRef.current) {
        return;
    }
    e.preventDefault();
    
    Keyboard.dismiss();

    InteractionManager.runAfterInteractions(() => {
      navigation.dispatch(e.data.action);
    });
})

First we prevent the default navigation back, and after the Keyboard is fully dismissed, we'll dispatch the e.data.action so it will go back now.

This method of preventing default navigation batch and then dispatching is recommended in react-navigation doc https://reactnavigation.org/docs/preventing-going-back/

Above logic can be isolated only to Mobile platforms (not web) if necessary.

What alternative solutions did you explore? (Optional)

None

[melvin-bot]

@marcochavezf, @greg-schroeder, @ishpaul777 Eep! 4 days overdue now. Issues have feelings too...

[marcochavezf]

Hi @ishpaul777, what do you think about this proposal?

[ishpaul777]

I haven't get the chance to look into this, i'll priortize this tomorrow when i start, Apologies for delay. 🙇

[ishpaul777]

hello @nkdengineer, Thanks for your proposal, but i seems changes from your proposal doesn't seem to fix the issue.

see video from 0:16 - 0:18-

Screen.Recording.2024-07-04.at.3.02.53.AM.mov

[drminh2807]

Proposal

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

Previous view is shown quickly when navigating back to the expense report

What is the root cause of that problem?

will happen at the same time as the going back navigation, so still a jarring experience.

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

? navigation.addListener('beforeRemove', (e) => {                
    if (!isKeyboardShownRef.current) {
        return;
    }
    e.preventDefault();
})

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

📣 @drminh2807! 📣
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>

[layacat]

Proposal

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

shouldEnableKeyboardAvoidingView enable and disable so quick makes the screen glitch

What is the root cause of that problem?

isTopMostReportId is changed very much when navigating back

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

only change isTopMostReportId when the screen is focused
https://github.com/Expensify/App/blob/main/src/pages/home/ReportScreen.tsx#L334

    const [isTopMostReportId, setIsTopMostReportId] = useState(currentReportID === reportIDFromRoute);
    useEffect(() => {
        if (!isFocused) {
           return 
        }
        setIsTopMostReportId(currentReportID === reportIDFromRoute);
    }, [currentReportID, isFocused, reportIDFromRoute]);

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

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

[ishpaul777]

Hello @drminh2807 and @layacat,

Thank you for your interest in resolving this issue. We appreciate your effort. However, I need a bit more detail on root cause to be able to review your proposals better. Could you please review our contributing guidelines and provide a detailed Root Cause Analysis (RCA)?

---

Also I have noticed that solution from @layacat is same as earlier proposal from @drminh2807

@drminh2807 Any reason you chose to withdraw the proposal? Does it cause any bugs?

[layacat]

RCA is the reportIDFromRoute (route.params.reportID) changed when navigating back

[ishpaul777]

why this glitch happens only when navigating from transaction report screen -> expense report screen, Without a proper understanding of root cause i am not confident moving forward the proposal solution.

RCA is the reportIDFromRoute (route.params.reportID) changed when navigating back

For now i can only consider this as observation not a root cause analysis

[melvin-bot]

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

[melvin-bot]

@marcochavezf @greg-schroeder @ishpaul777 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[ishpaul777]

Awaiting proposal

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

⚠️ 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]

@marcochavezf, @kirillzyusko, @greg-schroeder, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ishpaul777]

Unfortunately PR got reverted again we need to be very carefull with a new PR

---

@marcochavezf would it be good idea to do mini QA by Applause specific to the new PR for this on adhoc before rolling it out to staging, testing surface is pretty large since this touched everywhere we use soft keyboard ?

[marcochavezf]

@marcochavezf would it be good idea to do mini QA by Applause specific to the new PR for this on adhoc before rolling it out to staging, testing surface is pretty large since this touched everywhere we use soft keyboard ?

Yeah that's a good idea, given the complexity of the solution I can ask applause to test the ad-hoc build first before we merge

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

• fix: use KeyboardAvoidingView from react-native-keyboard-controller #47096

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

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

• @kirillzyusko does not require payment (Contractor)
• @ishpaul777 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:

• [@ishpaul777] The PR that introduced the bug has been identified. Link to the PR:
• [@ishpaul777] 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:
• [@ishpaul777] 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:
• [@ishpaul777] Determine if we should create a regression test for this bug.
• [@ishpaul777] 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.
• [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[greg-schroeder]

Still awaiting regression period as this one has been reverted a couple of times

[kirillzyusko]

@ishpaul777 I tested a build after RN 0.75 merge (latest main) and I don't see a problem of flashing anymore 🤔

kav-0.75-no-jump.mp4

Can you test and let me know whether it still needs to be fixed? I think the reported problem is not present anymore but curious to know your opinion 😊

[ishpaul777]

I tested as well its not reproducable anymore, i think we can close this now.

ScreenRecording_08-31-2024.01-06-20_1.MP4

wdyt @marcochavezf, Also if we close this will i be eligble to the payment

[ishpaul777]

not overdue on me

[marcochavezf]

wdyt @marcochavezf, Also if we close this will i be eligble to the payment

Yes that sounds fair, @greg-schroeder can you handle the payment for @ishpaul777 as C+ reviewer 🙏🏽?

[greg-schroeder]

Yes, handling today

[greg-schroeder]

Paid