[HOLD #45289][$250] Dev-Unable to send a message

[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: Reproducible in staging?: N/A Reproducible in production?: N/A 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: @ishpaul777 Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1722528633557129

Action Performed:

1. Open the app
2. Compose a message to any receiver and click Send button

   Expected Result:

   Message delivered to recipient

   Actual Result:

   Unable to send

   Workaround:

   Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [x] Android: Native
• [ ] Android: mWeb Chrome
• [x] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/37cb635f-8575-40b6-94d4-04b1d3a6e788

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01fb2184646c58f366
• Upwork Job ID: 1820927587952085072
• Last Price Increase: 2024-08-13

Issue Owner

Current Issue Owner: @

[melvin-bot[bot]]

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

[MelvinBot]

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

[ishpaul777]

This was fixed in https://github.com/Expensify/App/pull/46626

[Christinadobrzyn]

oh awesome! Closing without next steps.

[ishpaul777]

Actually sorry for speaking too soon, this was fixed but now its reproducable again

https://github.com/user-attachments/assets/1bc06b8e-73a9-47a0-9281-097dd61c258d

@Christinadobrzyn Can you please reopen 🙇

[Christinadobrzyn]

@ishpaul777 do you want to fix this issue here or is there another GH?

If we're fixing here, let me know and I can assign you to this GH.

[ishpaul777]

There's no other GH for the issue, i dont bandwidth to investigate the issue myself but happy to take this as c+ and review any incoming proposals,

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Current assignee @ishpaul777 is eligible for the External assigner, not assigning anyone new.

[Christinadobrzyn]

awesome! Thanks @ishpaul777 - let's get some proposals.

[dominictb]

Edited by proposal-police: This proposal was edited at 2024-08-12 04:38:45 UTC.

Proposal

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

Unable to click on send message in web, when running in Strict Mode dev environment

What is the root cause of that problem?

We're using tap gesture detector in SendButton component here, and because react-native-gesture-handler 2.14.1 doesn't play well with strict mode. Only after 2.16.0 release that this is fixed in https://github.com/software-mansion/react-native-gesture-handler/pull/2815.

A short explanation is that: this effect will run twice in strict mode. That means we register 1 gesture handler, drop it immediately and register another new one. However, since the gesture handler is only removed from the global registry but still attach to the View component's event listener, so this will be called once for each handler. And since the user presses the send button, it will trigger onPointerDown here for each handler, and 2 handlers will be registered in the GestureHandlerOrchestrator.

Now, in here, we have the logic to cancel out overlapping gesture handler here. That being said, the first (outdated) gesture handler will cancel the second gesture handler, hence, the final state of the second handler will be CANCELLED.

And since both gesture handlers listen for END state (check here), but only the second gesture is in the registry with the final CANCEL state (and the previous state is BEGAN, not ACTIVE), hence this will never be triggered, i.e: Message is not sent, and button is not active.

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

Upgrade the react-native-gesture-handler. Or we can patch based on this PR: https://github.com/software-mansion/react-native-gesture-handler/pull/2815

Also, to avoid issue where the gesture detector attach the event listener to the tooltip content instead of the inner Pressable view here, we should move the Tooltip wrapper to the outside of the root View here

<Tooltip>
 <View>
     <GestureDetector>....
  </View>
</Tooltip>

What alternative solutions did you explore? (Optional)

[Christinadobrzyn]

@ishpaul777 can you check out this proposal when possible? https://github.com/Expensify/App/issues/46823#issuecomment-2273916239

[ishpaul777]

Thanks for your proposal @dominictb, I upgraded to latest version of react-native-gesture-handler, i am still facing this issue

https://github.com/user-attachments/assets/d82ffda1-62eb-446f-a1b8-402588b5aa57

[dominictb]

@ishpaul777 sorry, I have tested on small screen using web inspector, so I might forget about the Tooltip here. The point is that when we click, the tooltip will render causing the the GestureDetector's children to change, so the event listener will be attached to the tooltip content instead of the send button, hence subsequent click won't work.

Hence, after the lib upgrade, we should move the Tooltip wrapper outside the root view of the SendButton.tsx here. The UI doesn't change, but the gesture detector won't be affected.

[dominictb]

@ishpaul777 https://github.com/Expensify/App/issues/46823 proposal updated!

[QichenZhu]

FYI, newer react-native-gesture-handler versions require React Native 0.74 or higher. Otherwise, the iOS app will crash as described in the issue below.

https://github.com/software-mansion/react-native-gesture-handler/issues/2927#issuecomment-2144763757

Our current policy regarding the new architecture support is to support only the most recent version, which at the time of writing is 0.74.

[dominictb]

@ishpaul777 in this case, shall we apply the patch until the RN upgrade PR is merged?

[ishpaul777]

@dominictb Will you be able to share a test branch with the patch so i can test it? if patch fix works as expected without RN upgrade then we should patch for now

[dominictb]

@ishpaul777 sure, I'll send you a test branch in a bit.

[melvin-bot[bot]]

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

[Christinadobrzyn]

Update for Melvin - we're reviewing proposals

[dominictb]

@ishpaul777 https://github.com/Expensify/App/compare/main...dominictb:epsf-app:fix/46823-gesture-handler?expand=1 testing branch

A small note, in 2.14.1 version we don't need to update the change in SendButton.tsx as described in my proposal

Also, to avoid issue where the gesture detector attach the event listener to the tooltip content instead of the inner Pressable view here, we should move the Tooltip wrapper to the outside of the root View here

because the GestureDetector implementation at this version still allow us to maintain a stable gesture-handler view instead of switching to the tooltip content view during pressing. However, updating to the latest version requires the change in SendButton.tsx

[Christinadobrzyn]

just checking in on this - how is the review going @ishpaul777?

[ishpaul777]

i'll provide feedback today 🙇

[ishpaul777]

I recently stumbled upon PR https://github.com/Expensify/App/pull/40548 for RN upgrade 0.74 we are also upgrading the rn gesture handler there https://github.com/Expensify/App/pull/40548/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519 I think we are close there. I think Holding this make more sense sorry i was not aware that we already have a PR for upgrade in works. Otherwise i would have suggested to HOLD before asking for branch 🙇 Appreciate the works @dominictb

[Christinadobrzyn]

Sounds good @ishpaul777 - thanks for the investigation. I added a HOLD to this GH while https://github.com/Expensify/App/pull/40548 is in the works.

[Christinadobrzyn]

https://github.com/Expensify/App/pull/40548 is closed. In my testing this is resolved - asking QA to test again. https://expensify.slack.com/archives/C9YU7BX5M/p1724188333800229

[ishpaul777]

it appears it was closed it so a we can focus on upgrading RN to version 0.75 PR-> https://github.com/Expensify/App/pull/45289

so for now issue still exists

https://github.com/user-attachments/assets/0c6e8d66-d0de-4058-813e-a255d81ffbc4

[Christinadobrzyn]

Sorry @ishpaul777 I'm not sure I understand what is next for this. Are we reviewing proposals or waiting for something?

[ishpaul777]

We should hold for https://github.com/Expensify/App/pull/45289

[Christinadobrzyn]

Looks like https://github.com/Expensify/App/pull/45289 is in staging so I think we can test this again. I'll move to daily to try and test today/tomorrow unless you can get to it @ishpaul777

[Christinadobrzyn]

Hum, I think this is resolved, @ishpaul777 can you double-check?

[ishpaul777]

yes this is resolved now We can close this