[HOLD for payment 21 June] iOS - Attachment picker flickers and unable to select for choose document

[isagoico]

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

---

Expected Result:

Attachment picker displayed and user able to select attachment

Actual Result:

Attachment picker flickers and unable to choose any document

Action Performed:

1. Launch the app and login
2. Choose any chat from LHN
3. Tap the + > Add Attachment > Choose Document

Workaround:

Unknown

Platform:

Where is this issue occurring?

Web
iOS ✔️
Android
Desktop App
Mobile Web

Version Number: 1.0.67-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

Bug5107955_Aiiachment_picker.mp4

Expensify/Expensify Issue URL:

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @cead22 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[OSBotify]

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

[Julesssss]

The react-native-web update seems to have introduced this regression. The issue occurs on this branch, but not on the previous commit 0c8dd5a54.

[parasharrajat]

@Julesssss React-native-web does not affect IOS. It's only active on the web.

[Julesssss]

I agree and that makes sense, but perhaps one of the other code changes in that PR introduced the issue?

I can't think of any other explanation for why the issue is not reproducible on 0c8dd5a54, but is on e75416eb9.

RPReplay_Final1623424408.MP4

[parasharrajat]

I mostly believe it's due to react version upgrade.

[marcaaron]

@Julesssss did you happen to test changing the react version to see if it fixed it?

[Julesssss]

Nope, but I think that is a likely fix. Perhaps it would be better to remove just that library, but honestly, I think that will take longer as I would need to work out which changes are relevant. Realistically we'll need to revert the full PR.

Also I wasn't clear, but by 'The react-native-web update' I meant the PR, not that specific library.

[marcaaron]

it would be better to remove just that library

Which library?

[roryabraham]

I actually think that #3446 is more likely responsible for this issue. Looks like others have had issues with react-native-document picker after upgrading to RN 64.1: react-native-documents/document-picker#405

[roryabraham]

@isagoico do you know what iOS version was used by the tester here?

[Julesssss]

@roryabraham I reproduced on a physical device running iOS 11

Which library?

react-native-web

[marcaaron]

Ah no, we can't remove react-native-web. Just confirmed document picker works on main + react-native:0.63.4 + simulator

[roryabraham]

I just confirmed that the document picker works on main + react-native:0.64.1 + react-native-document-picker:5.1.0, so we should do that instead of downgrading RN

[marcaaron]

FWIW, it also works fine for me with 0.64.1 + react-native-document-picker:4.0.0 😫

[roryabraham]

I was able to reproduce w/ iOS 14, 0.64.1 + react-native-document-picker:4.0.0

[marcaaron]

testing on iOS 14.5

[Julesssss]

Is anyone able to test a physical device? I have a horrible and hopefully incorrect feeling that a previous attachment bug only affected physical devices. If not I'll do this first thing Monday.

[Julesssss]

Yep, I can reproduce still on iOS 11, a physical device. I should have clarified on Friday that this was only reproducible on iOS (and possibly only on physical devices).

[Julesssss]

Okay, this is only sometimes occurring, but it's a nightmare to verify 😭. In some circumstances it will work the first time you attempt to add an attachment, but then almost all later attempts fail. The only hint is that if you enter a new chat after it fails to display, the Attachment picker will then display -- it's appearing at completely the wrong time, but it remains on screen 😖

Works first time

ios-attachment-main.mp4

Appearing when opening a new chat

ios-attachment-newchat.mp4

main

• This works the first time, but any later attempt fails

main + downgrade 'react-native' to .63.3

• This seems to have no change at all

[cead22]

@Julesssss it looks like you're on this so I'm gonna unassign myself. If you decide to unassign, please let me know and I'll find someone to work on this (which is pretty much all I did Friday)

[Julesssss]

I tested builds to locate the introducing commit:

• 5ce0b06a03: Issue does not occur
• 30d66b717: Issue does not occur
• 822acf0b56d: (main was merged into PR) Issue does not occur
• [c0046a2fd]: (main was merged into PR with multiple library changes) Issue is introduced... SO it wasn't the bump react-native/react-native-web PR after all.

There's a high likelihood that one of the following library changes introduced the issue, so next step will be to review them 1 by 1:

• "react-native-onyx"
• "react-native-reanimated": "^2.1.0",
• "react-native-screens": "^3.0.0",
• "react-native": "0.64.1"
• "@react-native-masked-view/masked-view": "^0.2.4",
• "@react-navigation/drawer": "^6.0.0-next.17"
• "@react-navigation/native": "^6.0.0-next.13"
• "@react-navigation/stack": "^6.0.0-next.25"
• "babel-plugin-transform-remove-console": "^6.9.4",

My main takeaway is that it is frustratingly time-consuming working out which commit/PR introduced a regression. Ideally we would detect more of these regressions at the PR review stage.

[Julesssss]

OKAY. Here's the PR which introduced the issue: Upgrade react-navigation to 6.x

• 37b3591ac: Issue does not occur
• c0766f5: Issue occurs

Specifically, it turns out that bumping react-native down to 0.63.3 did solve the problem, which is odd because I saw otherwise when testing that earlier. Now I'll try bumping down the library once more on main to confirm.

[Julesssss]

The above comment is not quite accurate, this PR introduced the react-native bump.

It gets worse. I can confirm that simply downgrading the 'react-native' library to 0.63.3 on main does not fix the issue, as I discovered earlier today.

The issue is only fixed when I downgrade both 'react-native' and 'Onyx'! Reverting Onyx is out of the question as we've already merged a bunch of fixes and would add a lot of extra work, so I think we should revisit the decision to launch with this issue and remove the deployBlocker label.

Please let me know if you agree/disagree. Thanks.

CC @trjExpensify

[parasharrajat]

@Julesssss I think I found the root cause here. just want you to confirm. Again, it seems to be the RN bug where two models can't exist simultaneously. I know we are waiting here for the modal transition to end but still, this timeout's existence says that there was an issue here but now it needs more delay.

https://github.com/Expensify/Expensify.cash/blob/bdf2144b6eb1fd561905ea6e3450e8b6997a7a60/src/components/AttachmentPicker/index.native.js#L225-L231

If we increase the Delay here - for eg I set 1 sec, It works fine.

Please check?

[Julesssss]

Well done @parasharrajat. Can confirm.

Maybe that's why it sometimes worked correctly the first time? I guess that also explains why two different changes introduced the same bug 🤯

We had better open up another UpWork contract for you 😅

[Julesssss]

I'm assuming you want to take the job? but it would be great if you could raise a PR for this.

If so we'll get an UpWork task created immediately. Please tag me on the PR so I can review first thing tomorrow 👍

[parasharrajat]

Yeah. PR will be up in no time. Upwork can wait.

[MelvinBot]

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[Julesssss]

Hey @stephanieelliott. This one is slightly different as we already have the solution and we just need an UpWork issue created to track payment.

[stephanieelliott]

Cool, that's easy! Posted to Upwork, please apply for this one @parasharrajat: https://www.upwork.com/jobs/~01cb0fd992029800cc

[parasharrajat]

@Julesssss PR is up. This deployblocker is done. 🧀

[marcaaron]

@Julesssss so it seems like this isn't related to dependencies at all? Or possibly could be, but not in any obvious way?

[Julesssss]

@Julesssss so it seems like this isn't related to dependencies at all? Or possibly could be, but not in any obvious way?

We've seen this issue before when trying to open a second Modal from another Modal. onModalHide is called on the first model, but as the second Modal has also started to display, it also is immediately closed.

I believe both dependencies somehow sped up the displaying of the second Modal, which meant that 10ms was no longer long enough of an artificial delay to wait before showing the second Modal. So yeah, I think both dependencies introduced the problem as a side effect of speeding up the app (or something similar).