Closed m-natarajan closed 1 month ago
Triggered auto assignment to @garrettmknight (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.
@garrettmknight FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
We think that this bug might be related to #vip-vsb
Triggered auto assignment to @stephanieelliott (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.
@stephanieelliott wasn't sure if this was internal/external, but I'm gonna be off till Thurs. Can you repro and open it up? I'll pick it up when I get back, thanks!
@garrettmknight, @stephanieelliott Whoops! This issue is 2 days overdue. Let's get this updated quick!
Site was down most of Monday so just now got the opportunity to test, and I was able to repro this. Pretty sure this can be worked on externally, adding labels to get some proposals!
Job added to Upwork: https://www.upwork.com/jobs/~014d8f21d1c3cead99
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External
)
Waiting for proposals.
Still waiting for proposals.
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
Hi, I’m Michael (Mykhailo) from Callstack and I would like to work on this issue.
@garrettmknight @eVoloshchak 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!
@garrettmknight, @eVoloshchak Whoops! This issue is 2 days overdue. Let's get this updated quick!
Just reproduced - @rezkiy37 if you want to work on this one please put forward a root cause analysis/proposal. Thanks!
Yes, I am actively working on the issue.
I've found a solution how to fix it. I need to test performance and measure it. Also, I still don't understand the root cause. Work in progress.
After my investigations, I faced a problem with react-native-gesture-handler itself. In a few words, the problem is with GestureDetector in SendButton.tsx. I've created an issue (https://github.com/software-mansion/react-native-gesture-handler/issues/2920) for them.
Meanwhile, I see that the current flow is tricky to handle a press on the send button. I am preparing a proposal to simplify the current behavior and fix the bug immediately. So we don't need to wait until they fix react-native-gesture-handler
.
@garrettmknight, @eVoloshchak Whoops! This issue is 2 days overdue. Let's get this updated quick!
I've sent a proposal for the internal review.
@rezkiy37 where's that proposal so I can follow along?
The send button is not responsive after returning from a thread.
Currently, the app handles send button presses via GestureDetecor from react-native-gesture-handler
. It allows running a handle function as a worklet
from react-native-reanimated
.
The problem with GestureDetecor. It does not react to tap actions, when the user goes to thread (nested) screens and returns to the main report. The React DOM still has the component and Tap gesture. The configs are correct. Something happens under the hood of the third-party package.
When I tried to force update GestureDetecor via setting a new key
on each screen focus, it started to work.
So it is a problem with react-native-gesture-handler
. I created an issue for them - https://github.com/software-mansion/react-native-gesture-handler/issues/2920.
Meanwhile, I realized that the bug can be fixed if we improve our codebase.
The basic idea of a worklet
is to run code on the UI (main) thread.
As we can see above, this worklet
runs 4 functions. However, 3 of them and it delegates back to the JS thread. So it looks strange from the thread’s perspective.
The component already uses PressableWithFeedback. So it has onPress
. Definitely, onPress
does not allow to run worklets
. However, we don’t need it anymore. I mentioned before that only 1 function needs to be launched on the UI thread.
I propose to use onPress
to call handleSendMessage
. Right after that, we can simplify the function and remove worklet
and runOnJS
calls. Then we can use runOnUI
only for setNativeProps
.
const handleSendMessage = useCallback(() => {
if (isSendDisabled || !isReportReadyForDisplay) {
return;
}
// We are setting the isCommentEmpty flag to true so the status of it will be in sync of the native text input state
setIsCommentEmpty(true);
resetFullComposerSize();
// Run the callback on the UI thread
runOnUI(setNativeProps)(animatedRef, {text: ‘’}); // clears native text input on the UI thread
submitForm();
}, [isSendDisabled, resetFullComposerSize, submitForm, animatedRef, isReportReadyForDisplay]);
This approach does work properly. Also, it does not play ping-pong between threads. It begins on the JS thread, executes JS tasks here, and requires running only this one setNativeProps
on the UI thread.
@rezkiy37 where's that proposal so I can follow along?
Just posted above.
Awesome, thanks!
@eVoloshchak mind reviewing when you've got a chance?
As we can see above, this worklet runs 4 functions. However, 3 of them and it delegates back to the JS thread. So it looks strange from the thread’s perspective.
The component already uses PressableWithFeedback. So it has onPress. Definitely, onPress does not allow to run worklets. However, we don’t need it anymore. I mentioned before that only 1 function needs to be launched on the UI thread.
I propose to use onPress to call handleSendMessage. Right after that, we can simplify the function and remove worklet and runOnJS calls. Then we can use runOnUI only for setNativeProps.
It was originally done in a similar way (without runOnUI
bit) but it was causing an issue where a message would stay in the compose after being sent - https://github.com/Expensify/App/pull/22227.
I believe that the proposal is to effectively restore the previous behavior. The important bit in the current implementation is not that the text is cleared on the UI thread, but it's cleared synchronously on the UI thread, skipping the JS thread entirely.
Using runOnUI
would nullify that, as it would schedule the call to clear the composer asynchronously from JS thread to UI thread.
As for the fix inside RNGH, I've got an idea for how to fix this, but from my early tests, it doesn't work on RN 0.73 (which is currently used in the App). I'll try to find whether something can be backported from 0.74 to make it work, but I cannot promise anything there, so it may turn out to be necessary to wait for https://github.com/Expensify/App/pull/40548.
If you need it fixed quickly, patching out this part inside the core of RN will get rid of the problem, but may negatively impact performance since suspended views will still be mounted on the native side.
Alright, thank you! So I can try to test with RN 0.74. Also, there is a confirmation from RNGH team that the bug is replicated on their side.
If you need it fixed quickly, patching out this part inside the core of RN will get rid of the problem, but may negatively impact...
As I see it is a part for Android only.
Btw, regarding the initial bug that was fixed https://github.com/Expensify/App/pull/22227. I would like to test if it is still works properly with my proposal.
As I see it is a part for Android only.
It's behind #ifndef ANDROID
(if not defined) so it's for everything but Android.
As I see it is a part for Android only.
It's behind
#ifndef ANDROID
(if not defined) so it's for everything but Android.
Ah, my C++ knowledge leaves much to be desired 🙂
@garrettmknight, @eVoloshchak, @rezkiy37 Whoops! This issue is 2 days overdue. Let's get this updated quick!
@eVoloshchak bump on the review, please.
There is a PR (https://github.com/software-mansion/react-native-gesture-handler/pull/2925) where they have fixed the bug on the RNGH side. I am going to test it.
Note that this will only fix the issue on RN 0.74, since useLayoutEffect is broken on versions below that.
Seems like we need to wait for this PR to bump the RN version - https://github.com/Expensify/App/pull/40548.
@rezkiy37, this does introduce the bug from https://github.com/Expensify/App/issues/10148 back
https://github.com/Expensify/App/assets/9059945/072deaa9-c9f7-45ce-ad16-b26d6d9bac97
Okay, so I will consider another solution.
@garrettmknight @eVoloshchak @rezkiy37 this issue is now 4 weeks old, please consider:
Thanks!
Awaiting another proposal.
I tried https://github.com/Expensify/App/pull/40548 and https://github.com/software-mansion/react-native-gesture-handler/pull/2925 together. Unfortunately, these fixes haven't solved the current problem.
@rezkiy37, this does introduce the bug from #10148 back
RPReplay_Final1716974607.MP4
@eVoloshchak, can you try to with the next change where setNativeProps
is being called first:
if (isSendDisabled || !isReportReadyForDisplay) {
return;
}
// Run before others
runOnUI(setNativeProps)(animatedRef, {text: ''}); // clears native text input on the UI thread
// We are setting the isCommentEmpty flag to true so the status of it will be in sync of the native text input state
setIsCommentEmpty(true);
resetFullComposerSize();
submitForm();
I've tried and looks like it works properly on my side.
@eVoloshchak mind giving this a look?
@rezkiy37, it's harder to reproduce, but still reproducible if you click 'Send' fast enough
https://github.com/Expensify/App/assets/9059945/898a6f56-a797-4ac9-8e1c-76b88c19c784
Okay, so I would like to wait until https://github.com/Expensify/App/pull/40548 is merged. Hopefully, the RN upgrade and RNGH fix can help us.
@eVoloshchak do you agree? I can put this on hold if so.
@garrettmknight, agree, let's hold for https://github.com/Expensify/App/pull/40548
Dropping to weekly while we hold.
Still on hold for https://github.com/Expensify/App/pull/40548
Still on hold for https://github.com/Expensify/App/pull/40548
Still on hold for https://github.com/Expensify/App/pull/40548
Hello! FYI: I have a vacation from 11.07 to 26.07. Meanwhile, someone from Callstack can help with this issue.
Still on hold for https://github.com/Expensify/App/pull/40548
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: 1.4.70-0 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: n/a Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: applause internal team Slack conversation:
Action Performed:
Expected Result:
The message is sent.
Actual Result:
The send button is not responsive.
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/38435837/477b0ecb-6590-4b37-83ff-48170f9712e4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @garrettmknight