melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Closed lanitochka17 closed 1 year ago
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
melvin-bot[bot]
commented
1 year ago Platforms in OP are ✅)
akinwale
commented
1 year ago A user is able to select and send 2 emojis at the same time before the emoji picker closes.
After the user selects an emoji, the other emojis can still accept touch or press input during the brief period while the emoji picker is still open. Since the other emojis can still accept touch or press input, the onPress event handler will be invoked for any subsequent emoji pressed. Note that this only happens on iOS and Android native.
Since this happens on iOS and Android native (all web platforms and desktop are fine), only EmojiPickerMenu/index.native.js needs to be updated.
Solution 1 Use a reference to store a flag that an emoji has already been pressed. The following changes can be made to achieve this:
useRef: const emojiPickedRef = useRef(null);onPress handler:
onPress={(emoji) => {
if (emojiPickedRef.current) {
return;
}
emojiPickedRef.current = true;
addToFrequentAndSelectEmoji(emoji, item);
}}Solution 2
Debounce (combined with useCallback if necessary), or throttle the onPress handler for the emoji picker item in the EmojiPickerMenu component.
I tried to use a state variable to track the emoji picked boolean flag, but it is possible for a user to tap fast enough to send a subsequent onPress event before React updates the state.
melvin-bot[bot]
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~01d67b0c351a8b8c5d
melvin-bot[bot]
commented
1 year ago Current assignee @lschurr is eligible for the External assigner, not assigning anyone new.
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @robertKozik (External)
FabianKoder
commented
1 year ago A user is able to select 2 emojis at the same time before the emoji picker closes.
The onEmojiSelected call toggles the visibility of the emoji picker, but since it it a state, it only will be updated on the next render (when the emoji picker disappears).
It is also possible to add more than 2 emojis if the device is slow enough.
Check if emojiPopoverAnchor.current is null, instead of checking if !isEmojiPickerVisibile. This will ensure that even before the next render, the selectEmoji function will know that it needs to prevent adding another emoji. This would also solve the bug if it would occur on web (which probably just not occurs because the state update happens faster, e.g. because of faster devices).
N/A
Note: I edited this proposal, since my first proposal was not tested. The new one is tested and confirmed that it works.
tienifr
commented
1 year ago The user is allowed to select 2 emojis at a time.
In here https://github.com/Expensify/App/blob/ab99bf4748d03e309257e24a8828b48a118ba5b9/src/components/EmojiPicker/EmojiPickerMenu/index.native.js#L153, if we press emojis quickly, the onPress will be triggered multiple times, causing many emojis to be selected.
We need to prevent selection of emojis if the last emoji selection didn't finish executing. After this issue is fixed, we'll have a useSingleExecution hook that can be used to limit execution to one at a time (see detailed explanation here).
Then we can use that hook to wrap the callback here https://github.com/Expensify/App/blob/ab99bf4748d03e309257e24a8828b48a118ba5b9/src/components/EmojiPicker/EmojiPickerMenu/index.native.js#L153, so we can ensure the emoji can only be selected one at a time.
There'll be small changes needed in that useSingleExecution definition to allow passing in params
(action) => (...params) => {and
const execution = action(params);Then it will be a 1-liner change in here:
onPress={singleExecution((emoji) => addToFrequentAndSelectEmoji(emoji, item))}and it uses a general approach that's also used in other places in the app (useSingleExecution).
NA
jeet-dhandha
commented
1 year ago addToFrequentAndSelectEmoji method is not debounced or handled in any way to prevent fast selecting emoji's.I have three solutions:
addToFrequentAndSelectEmoji method using _.debounce from lodash.ref to check if the emoji is already selected.InteractionManager.runAfterInteractions to prevent fast selecting emoji's.
gzqyl
commented
1 year ago Proposal Please re-state the problem that we are trying to solve in this issue. A user is able to select and send 2 emojis at the same time before the emoji picker closes.
What is the root cause of that problem? before the component become invisible, the user has clicked more than once, then the function selectEmoji execute twice or more, just depends on component visibility states not the best solution, and that is why the user now could still select two emoji if he clicks very fast.
[App/src/components/EmojiPicker/EmojiPicker.js]
Line 98
if (!isEmojiPickerVisible) { return; }
What changes do you think we should make in order to solve the problem? I have found a very simple while effective way to solve such problems.
Solution video demo, I tested it on android, it works fine, after the user click the emoji, the modal hides then, and the user has no way to select two or more emoji at one time.
If I have been chosen, kindly let me know, the project is great, I see only Android is checked, if it requires all platform demo, I will upload the demos later, and PR later.
Contributor details Your Expensify account email: allen.gzqyl@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01e99782a01937e92c
melvin-bot[bot]
commented
1 year ago 📣 @gzqyl! 📣 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:
Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>Contributor details Expensify account email: allen.gzqyl@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01e99782a01937e92c Just for confirmation, if it duplicates, ignore it, thanks. just find that @foxmail.com can not be added slack channel, so I use google mail as a replace.
lschurr
commented
1 year ago @robertKozik could you review these proposals?
lschurr
commented
1 year ago Bumped in Slack as well - https://expensify.slack.com/archives/C01GTK53T8Q/p1691770938283059
robertKozik
commented
1 year ago Thank you all for your proposals! 🙇🏼
@jeet-dhandha - using debounce is almost the same as setTimeout usage. So in case we have other possible solutions it's better to not use it. The other approaches you include in your proposal, were already posted by other users (using ref by @akinwale & @Fabb111)
@gzqyl - Your proposal lacks explanation of the solution. I cannot tell from your comment whether or not your solution is better/worst than others as you just include the video of the working solution
@akinwale & @Fabb111 both your solutions are quite similar - they're just using the different refs to detect whether or not modal will be soon closed. But even though this approach would be successful I believe that it's not tackling the root cause directly - which is that user can click multiple times before modal close. We are just ensuring that only the first would be handled (I'm aware that it means exactly the same as prohibiting multiple clicks, but the approach suggested by @tienifr will be the better option, as it can be used in multiple places and standardise the approach to such a cases across the app)
@tienifr implementing and standardising on one function for prohibiting multiple function calls is the way to go in my opinion. It tackles the problem at it roots removing to call function multiple times
This said I believe that proposal from @tienifr is the way to go here, so we would have the same approach (useSingleExecution hook) used across the app.
Selected Proposal: Proposal Author of Proposal: @tienifr
🎀 👀 🎀 C+ reviewed
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @marcochavezf, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
jeet-dhandha
commented
1 year ago Okay @robertKozik. Keep up the Good Work @tienifr 🥂.
gzqyl
commented
1 year ago @robertKozik the solution chosen looks very good, while I had tested it just now, it seems the bug still occurs, I understand that the video I uploaded is a little hard to check whether it works or not, as when the user clicks slower, the bug will not occur. After use the hook "useSingleExecution", sometimes it is ok, while sometimes click very fast, the user could still select two emoji. By the way, I only tested it as the comments guides.
akinwale
commented
1 year ago @robertKozik the solution chosen looks very good, while I had tested it just now, it seems the bug still occurs, I understand that the video I uploaded is a little hard to check whether it works or not, as when the user clicks slower, the bug will not occur. After use the hook "useSingleExecution", sometimes it is ok, while sometimes click very fast, the user could still select two emoji. By the way, I only tested it as the comments guides.
Yeah, it seems it is very possible for a user to be able to click faster than the state updates, since React state updates are not immediate. Using a ref would be a better solution in that regard.
jeet-dhandha
commented
1 year ago @robertKozik @tienifr Tried useSingleExecution not working.
https://github.com/Expensify/App/assets/78416198/393dd4ca-d42a-4592-837a-6b75cb5b719e
jeet-dhandha
commented
1 year ago @robertKozik I would like to update my proposal finally found the root cause.
jeet-dhandha
commented
1 year ago hideEmojiPicker below, we are setting:
onModalHide.current = () => {}; - To its default state when navigating.emojiPopoverAnchor.current = null; - To null when hiding the emoji picker.onEmojiSelected.current = () => {}; is not set to its default state after calling it once, so when we select second emoji, as function is still available, it will execute for second time.onEmojiSelected.current to its default value after calling it once.https://github.com/Expensify/App/blob/ed2c287daf6a1d59f4f43a9bfc84128597575730/src/components/EmojiPicker/EmojiPicker.js#L34 (Above code is to show what the default value of onEmojiSelected was.)
https://github.com/Expensify/App/blob/ed2c287daf6a1d59f4f43a9bfc84128597575730/src/components/EmojiPicker/EmojiPicker.js#L96-L107 (Above code for reference where we are changing.)
const selectEmoji = (emoji, emojiObject) => {
// Prevent fast click / multiple emoji selection;
// The first click will hide the emoji picker by calling the hideEmojiPicker() function
if (!isEmojiPickerVisible) {
return;
}
hideEmojiPicker(false);
if (_.isFunction(onEmojiSelected.current)) {
onEmojiSelected.current(emoji, emojiObject);
+ onEmojiSelected.current = () => {}; // Set to default state
}
};Dang, let me double check the singleExecution before it looked like it was working...
gzqyl
commented
1 year ago before post the proposal, maybe we should test the solution on the platform which the issue occurs, so that, once our proposal was chosen, we can quickly fix the issue, and PR soon.
robertKozik
commented
1 year ago Thank you all for pointing that out 🙇🏼
Please @tienifr cound you refer to it? During initial proposal testing I was mainly clicking on frequently used emojis which looks like fixed, but clicking on any other emoji is still buggy, and easily reproduces the bug, video attached:
https://github.com/Expensify/App/assets/61146594/ec4d605f-5d0b-4eac-96c2-a8af7fc69afb
It looks like the problem strictly inside the useSingleExecution hook - maybe it's valuable insight for initial issue https://github.com/Expensify/App/issues/23867 which is going to introduce this hook.
If you couldn't answer this problem we will have to go with another proposal
robertKozik
commented
1 year ago @gzqyl I see on your video that your proposal is working, but you didn't stated how you fixed the bug. If you want your proposal to be considered here or in the other issues please add to What changes do you think we should make in order to solve the problem? section the description of how your fix is implemented
jeet-dhandha
commented
1 year ago @robertKozik Any thoughts on my proposal https://github.com/Expensify/App/issues/24261#issuecomment-1675710952
gzqyl
commented
1 year ago @robertKozik My solution is useRef, which does not need to create a new hook, as state change takes some time, ref value changes immediate, so kindly check the follow image, it works.
FabianKoder
commented
1 year ago In my opinion, the useSingleExecution hook won’t work for this case as it uses an internal state which is needed as the return value can be used to change properties (or styles) of other components. If it would use a ref, no re-render would be triggered which is wanted in that case. Here, we only want to ensure that no other click goes through, which could be easily and effectively solved without introducing any overhead by just changing one line, as I already proposed.
tienifr
commented
1 year ago Please @tienifr cound you refer to it? During initial proposal testing I was mainly clicking on frequently used emojis which looks like fixed, but clicking on any other emoji is still buggy, and easily reproduces the bug, video attached:
Thanks, I'll take a look and post an update soon.
akinwale
commented
1 year ago Thank you all for pointing that out 🙇🏼
Please @tienifr cound you refer to it? During initial proposal testing I was mainly clicking on
frequently used emojiswhich looks like fixed, but clicking on any other emoji is still buggy, and easily reproduces the bug, video attached:Screen.Recording.2023-08-13.at.21.12.04.mov It looks like the problem strictly inside the
useSingleExecutionhook - maybe it's valuable insight for initial issue #23867 which is going to introduce this hook. If you couldn't answer this problem we will have to go with another proposal
@robertKozik Based on my experience with Expensify contributions so far, the first proposal that actually fixes the issue tends to be the one selected. While the useSingleExecution hook solution is great in theory, it doesn't actually fix the bug for this issue. Asking a contributor to update their proposal while there is already a functional one doesn't seem to be fair in this regard. Thanks.
tienifr
commented
1 year ago Please @tienifr cound you refer to it? During initial proposal testing I was mainly clicking on frequently used emojis which looks like fixed, but clicking on any other emoji is still buggy, and easily reproduces the bug
@robertKozik this is due to if clicking very quickly, the callback definition accesses the old isExecuting state value (which is still false), causing the second trigger. So the problem here is not with the useSingleExecution approach, but the actual implementation of it.
The fix is very simple, we can simply add a isExecutingRef and use the value of the ref when checking the isExecuting so we always have the most updated value of isExecuting.
The useSingleExecution will now be:
function useSingleExecution() {
const [isExecuting, setIsExecuting] = useState(false);
const isExecutingRef = useRef();
isExecutingRef.current = isExecuting;
const singleExecution = useCallback(
(action) => (...params) => {
if (isExecutingRef.current) {
return;
}
setIsExecuting(true);
const execution = action(params);
InteractionManager.runAfterInteractions(() => {
if (!(execution instanceof Promise)) {
setIsExecuting(false);
return;
}
execution.finally(() => {
setIsExecuting(false);
});
});
},
[],
);
return {isExecuting, singleExecution};
}We can even make the above cleaner by adding a useStateRef hook so we can do things like const [isExecuting, setIsExecuting, isExecutingRef] = useStateRef(false); (the implementation is here, the use case is so popular there's even a library for it).
Now this is ready to tackle any double press issue without requiring case-by-case patches 🚀 .
gzqyl
commented
1 year ago It is a bad experience, when I posted my worked solution codes, I got nothing, I think I will not contribute any more, it is not worthing. If I do not post the solution codes, it will not be taken in consideration, while if I did, others could take it as a reference, just waste of my time.
robertKozik
commented
1 year ago Sorry all for the confusion on this one. I didn't mean to favor any contributor proposal over another, just wanted to make sure that that we didn't miss something obvious while checking on the @tienifr proposal.
@akinwale I got your point but I personally think that it's better to choose the most optimal solution when the root cause is well defined by more than one proposal. And I'm still thinking that useSingleExecution is the right way to take, as long as we can get it working properly.
But as we have now CME here too, @marcochavezf (sorry for the problem which I caused here) what you think about this? For me the best proposals are from @tienifr and @jeet-dhandha
robertKozik
commented
1 year ago @gzqyl Sorry to hear that, but in my eyes your accusations are unfounded for me. Please compare your initial proposal to other. Every proposal should have the clear Root cause of the problem specified and the solution described - it doesn't always mean code solutions, but rather the more detailed idea how to solve the issue. Your initial proposal had only the video of working solution thus I asked you to provide more details, without that I cannot refer to it at all. I don't see any proposal which took yours as reference.
gzqyl
commented
1 year ago @robertKozik How can you say no one take my codes as a reference?
@tienifr your original solution is not working, but after you revised your codes, you indeed used my solution, use a ref while not state, if you check your codes, it almost does the same logic as my solution codes shows.
@jeet-dhandha Even if you find the reason, it should use ref not state, you have shown us the wrong code + onEmojiSelected.current = () => {}; // Set to default state, so from your codes, I could not see it will work.
I have posted a video demonstrated that I have solved this issue, and from my later codes, you can demonstrate that my codes work well.
I have already said, " just depends on component visibility states not the best solution, and that is why the user now could still select two emoji if he clicks very fast."
So, you should know the root cause is that we should not depends on state change here, as it will keep unchanged when user clicks two or more times.
I do not want to contribute any more, before I posted the codes, even if I have demonstrated that I have solved the issue, it is still not enough, while after I posted the codes, others' original not worked solution can take my codes as a reference, my work is meaningless for me.
I also post another proposal for another issue, but when I posted the solution codes, it seems that the issue will not be continue with the UpWork, as the issue could be solved by one line code changed.
If this is true for me, even if I solved more issues, when my solution will be selected? It is too hard to have a definite result, so I will not waste of my time.
akinwale
commented
1 year ago @akinwale I got your point but I personally think that it's better to choose the most optimal solution when the root cause is well defined by more than one proposal. And I'm still thinking that
useSingleExecutionis the right way to take, as long as we can get it working properly.
My concern here is that the useSingleExecution implementation initially posted didn't fix the bug. Eventually, the right solution involves having to make use of useRef to solve the problem which I had already pointed out in my proposal.
The contributor already has the same proposal elsewhere, so they will get assigned two issues based on a proposal which was originally untested and incorrect.
Either way, it's all good. On to the next I guess.
marcochavezf
commented
1 year ago Hi everyone, I just wanted to express my appreciation for all the hard work that's gone into posting the proposals. A special shout-out to @robertKozik for the time and effort in selecting and reviewing the proposals, as well as clarifying others. Things may have gotten a bit heated, but I want to affirm that I agree with the selection made and I'm leaning towards choosing @tienifr's proposal.
@gzqyl I'm also sorry to hear that you are feeling disheartened. Many of our successful contributors have tried numerous times before finding the right fit. We invite you to keep learning from other contributors and continue growing 🚀
melvin-bot[bot]
commented
1 year ago 📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
jeet-dhandha
commented
1 year ago @marcochavezf a request before continuing https://github.com/Expensify/App/issues/24261#issuecomment-1675710952 can you give this a quick look ?
jeet-dhandha
commented
1 year ago As i m sure we want to find the root cause to solve the issue and not create something which is just gonna make it more complex as if we start using this hook we won’t be able to find out the root cause which is causing the main issue.
jeet-dhandha
commented
1 year ago @robertKozik I would like you that you also compare the root cause before comparing the solutions.
Also don’t tell contributors to fix the issue if their code is not working like here https://github.com/Expensify/App/issues/24261#issuecomment-1676442232 . You could just say we can’t move forward with your proposal. In future if they wanted to fix their proposal they can do it by themselves only.
gzqyl
commented
1 year ago Anyway, It is truly meaningless even if I solved the issue, I am afraid of such case will happen again if I solved more issues. I will have a look at another proposal I have posted on another issue, that proposal had also solved the issue, and I posted the codes too. It has been some days waiting for the C+ review, if it is not be selected too, I think solving more issues will be meaningless for me.
FabianKoder
commented
1 year ago Please stay objective and not think about the missed job or opportunity. The most important thing is to improve the Expensify app and the respective code, still. So naturally the best solution should be picked. And @tienifr is the best solution concept, just that it initially didn’t work. If it wouldn’t have been easily fixable, someone else would’ve been picked. But since it was easily fixable and is a re-usable hook that can be used in many different parts of the app, this is also in my opinion the way to go. The problem probably exists in more components, so having a consistent way to fix the issue is great.
gzqyl
commented
1 year ago Please stay objective and not think about the missed job or opportunity. The most important thing is to improve the Expensify app and the respective code, still. So naturally the best solution should be picked. And @tienifr is the best solution concept, just that it initially didn’t work. If it wouldn’t have been easily fixable, someone else would’ve been picked. But since it was easily fixable and is a re-usable hook that can be used in many different parts of the app, this is also in my opinion the way to go. The problem probably exists in more components, so having a consistent way to fix the issue is great.
I do not think so, when the issue was not really solved, it is hard to tell whether the issue could be easily fixed or not. Find a workable way to solve the issue in actual is not that easy as we thought before the issue was solved. After the issue was solved, then we find it is such easy then ignore the man who has solved the issue initially is not fair for that man!
FabianKoder
commented
1 year ago Please stay objective and not think about the missed job or opportunity. The most important thing is to improve the Expensify app and the respective code, still. So naturally the best solution should be picked. And @tienifr is the best solution concept, just that it initially didn’t work. If it wouldn’t have been easily fixable, someone else would’ve been picked. But since it was easily fixable and is a re-usable hook that can be used in many different parts of the app, this is also in my opinion the way to go. The problem probably exists in more components, so having a consistent way to fix the issue is great.
I do not think so, when the issue was not really solved, it is hard to tell whether the issue could be easily fixed or not. Find a workable way to solve the issue in actual is not that easy as we thought before the issue was solved. After the issue was solved, then we find it is such easy then ignore the man who has solved the issue initially is not fair for that man!
I understand what you are talking about, but states and refs are two very basic things in React and it is common knowledge for everyone who calls themselves React developer. Obviously @tienifr could have looked in the first proposal, or I even wrote
the
useSingleExecutionhook won’t work for this case as it uses an internal state which is needed as the return value can be used to change properties (or styles) of other components. If it would use a ref, no re-render would be triggered which is wanted in that case
The original proposal had another purpose, and the purpose was adapted with a common concept (state + ref combined) after it was pointed out that the original variant wouldn’t exactly work here. Everyone, including me, could have proposed such a re-usable hook solution, but nobody except @tienifr did. So, nothing was copied. Nobody said how to fix the hook.
jeet-dhandha
commented
1 year ago Also this is more of a major code change instead of fixing the underlying issue.
gzqyl
commented
1 year ago Please stay objective and not think about the missed job or opportunity. The most important thing is to improve the Expensify app and the respective code, still. So naturally the best solution should be picked. And @tienifr is the best solution concept, just that it initially didn’t work. If it wouldn’t have been easily fixable, someone else would’ve been picked. But since it was easily fixable and is a re-usable hook that can be used in many different parts of the app, this is also in my opinion the way to go. The problem probably exists in more components, so having a consistent way to fix the issue is great.
I do not think so, when the issue was not really solved, it is hard to tell whether the issue could be easily fixed or not. Find a workable way to solve the issue in actual is not that easy as we thought before the issue was solved. After the issue was solved, then we find it is such easy then ignore the man who has solved the issue initially is not fair for that man!
I understand what you are talking about, but states and refs are two very basic things in React and it is common knowledge for everyone who calls themselves React developer. Obviously @tienifr could have looked in the first proposal, or I even wrote
the
useSingleExecutionhook won’t work for this case as it uses an internal state which is needed as the return value can be used to change properties (or styles) of other components. If it would use a ref, no re-render would be triggered which is wanted in that caseThe original proposal had another purpose, and the purpose was adapted with a common concept (state + ref combined) after it was pointed out that the original variant wouldn’t exactly work here. Everyone, including me, could have proposed such a re-usable hook solution, but nobody except @tienifr did. So, nothing was copied. Nobody said how to fix the hook.
What I could not accept is that @tienifr ’s workable solution was posted after my workable codes posted. Have you checked the two solution codes? Though @tienifr use a hook, the hook can only work with the useRef which I have posted it before, the logic to stop double click is the same with mine. Other people also proposed useRef, but how to use the useRef, the right way to go is mine, and @tienifr does the same logic as mine!
tienifr
commented
1 year ago the PR is ready for review https://github.com/Expensify/App/pull/24614
jeet-dhandha
commented
1 year ago @robertKozik Waiting for a response...
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
The user should be able to select only one emoji at a time
Actual Result:
The user is allowed to select 2 emojis at a time
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.51.0
Reproducible in staging?: Yes
Reproducible in production?: Yes
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
Notes/Photos/Videos: Any additional supporting documentation
https://github.com/Expensify/App/assets/78819774/207e5e4c-83c4-496d-9afb-84d08234c787
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit