fix: abracadabra page

[kirillzyusko]

Details

The fix consist from two parts:

• preload the component in advance to be able to mount it synchronously on a demand;
• clean up the state in asynchronous way if it's a real component unmount.

Let's consider each part in more details.

1️⃣ preload the component in advance to be able to mount it synchronously on a demand

If we mount the component and the component is lazy and haven't been loaded yet, then when we switch navigator we remove old navigator, but a new one navigator doesn't have routes yet, so react-navigation clears a state, and when new routes are attached then a default route will be selected, because previous state was lost.

So we need to mount the screen/navigator synchronously.

2️⃣ clean up the state in asynchronous way if it's a real component unmount

The synchronous clean up was added here - #42592

The key thing is that now unmount is getting fired two times. If we have an asyncronous removal, then navigation just stops working (because we perform a navigation and then instantly run unmount - unmount fires asynchronously and removes just created new state).

To fix this problem I patched react-navigation and added useFullyMountedRef hook. It's returning ref.current=false if component is not fully mounted (i. e. unmount is about to be fired) and will update ref.current=true when component finished it's mounting.

This solution is not perfect, but currently react-navigation@6 is not fully compatible with StrictMode, so we have to patch it. Most likely with react-navigation@7 this patch will not be needed, because react-navigation@7 supporting StrictMode.

Fixed Issues

$ #44600
PROPOSAL: N/A

Tests

1. Go to https://staging.new.expensify.com/
2. Log in with new Expensifail account
3. Copy the Magic link and change it to staging
4. Open a new tab and navigate to the staging magic link
5. Abracadabra page is displayed and prev tab gets redirected to inbox screen

Warning

Please, test a release version locally.

Offline tests

N/A

QA Steps

1. Go to https://staging.new.expensify.com/
2. Log in with new Expensifail account
3. Copy the Magic link and change it to staging
4. Open a new tab and navigate to the staging magic link
5. Abracadabra page is displayed and prev tab gets redirected to inbox screen

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (see Reviewing the code)
  • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • I verified that comments were added to code that is not self explanatory
  • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• If any new file was added I verified that:
  • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• If a new CSS style is added I verified that:
  • A similar style doesn't already exist
  • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
• If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
• If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
  • I verified that all the inputs inside a form are aligned with each other.
  • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Untitled.mov

MacOS: Desktop

[kirillzyusko]

@dangrous can we prepare a build based on this PR and ask QA to test if these changes actually fix the reported problem and don't introduce new problems?

Slightly afraid, that we are touching react-navigation core so some random issues could pop up 😅

[dangrous]

sure! Let me get that build started

[github-actions]

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/48899/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/48899/index.html
Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/48899/NewExpensify.dmg	https://48899.pr-testing.expensify.com

---

👀 View the workflow run that generated this build 👀

[dangrous]

Do you mind double checking that that looks good @kirillzyusko and then I can send over to QA?

[kirillzyusko]

@dangrous I tested and for me it worked well in release variant 👀 I also think that potentially it may fix other issues such as https://github.com/Expensify/Expensify/issues/519304

[dangrous]

Just requested qa's help! https://expensify.slack.com/archives/C9YU7BX5M/p1726092553187509

[dangrous]

Sounds like this passed QA, with one remaining known issue. Will get the details shortly!

[kirillzyusko]

Sounds like this passed QA, with one remaining known issue. Will get the details shortly!

@dangrous does it mean that I can proceed with this PR further, clean it up and prepare for a review? 👀

[dangrous]

Yeah I think go ahead! I haven't heard back yet from QA on which issue still exists (I'm assuming it's not #44600 which is the only one that would be concerning) but I will ping them again.

[melvin-bot]

@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[kirillzyusko]

@aimane-chnaif @dangrous this PR is ready for a review 🎉

[aimane-chnaif]

Please merge main. 1.7k commits are behind

[aimane-chnaif]

I think this should follow the New Library Process

[kirillzyusko]

Submitted an issue here: #49519

[kirillzyusko]

Please merge main. 1.7k commits are behind

Done 🙌

[aimane-chnaif]

On hold for #49519

[dangrous]

Hey @kirillzyusko finally got the response from QA - looks like the PR still reproduces these issues:
https://github.com/Expensify/Expensify/issues/519304
#48960

Any chance you could take a look at those?

[kirillzyusko]

@dangrous so we want to fix all of these issues in a single PR? Or do you want me to look and discover a root problem for these bugs?

[kirillzyusko]

https://github.com/Expensify/Expensify/issues/519304

I can confirm this is still reproducible in this branch.

#48960

I can not reproduce it from this branch 🤷‍♂️

[dangrous]

Okay so one of those was closed and QA suggested this PR would reopen it. That one we should definitely make sure we don't reintroduce with this.

The other one was open, I was going to say we don't have to handle it here BUT they just closed it in favor of this PR haha. It sounds like it might not be reproducible anymore.

So TLDR - let's see if either of those occur with this PR, and we only need to fix if they still do.

[kirillzyusko]

Okay so #48960 was closed and QA suggested this PR would reopen it. That one we should definitely make sure we don't reintroduce with this.

Well, I can rebase this PR to latest main because right now it's ~1000 commits behind. I couldn't reproduce the problem, but I hardly believe this PR can introduce such behavior 😅

P. S. I just re-tested this issue on updated branch and I can not reproduce it 🤷‍♂️ @dangrous maybe you can re-test it as well to confirm that it's not reproducible?

[dangrous]

Confirmed, I don't see that happening either. I think we're okay?

[dangrous]

As for https://github.com/Expensify/Expensify/issues/519304 - you said you could reproduce that one here? Let's see if we can fix it too, since even if we don't show the inbox (this issue) we probably should make sure we show the correct (non-expired) page, too. Does that make sense?

[garrettmknight]

@kirillzyusko any update here?

[kirillzyusko]

@garrettmknight thanks for reminder! The thread was lost somehow 🙈

you said you could reproduce that one here? Let's see if we can fix it too, since even if we don't show the inbox (this issue) we probably should make sure we show the correct (non-expired) page, too. Does that make sense?

Yeah, I can reproduce and you it totally makes sense what you are saying. But do I correctly udnderstand that you want to fix it in this PR?
At the moment I'm busy with other tasks, so not sure if I can spend more time on fixing other issues in this PR anytime soon. Would it make sense to review/merge this PR in its current state and then revisit other bugs and provide a fix later on?

[dangrous]

It looks actually like it's being fixed separately so yeah, you can ignore it here!

[kirillzyusko]

It looks actually like it's being fixed separately so yeah, you can ignore it here!

Cool! So we can review this PR then, right? 👀

[dangrous]

Yep! though we're waiting on approval of that library right?

[kirillzyusko]

Yep! though we're waiting on approval of that library right?

Oh, yeah, right!

[kirillzyusko]

Closing PR because the behavior was re-defined: #44600 (comment)