melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @michaelhaxhiu (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Closed kavimuru closed 1 year ago
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @michaelhaxhiu (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
melvin-bot[bot]
commented
1 year ago Platforms in OP are ✅)
kavimuru
commented
1 year ago Inconsistent cursor behavior when a user long-presses the back button on the 'Split Bill/Request Money' page between Android app and mobile web.
React's state management involves batching state updates for performance optimization, and in the current implementation, the setSelectionfunction in setNewAmount (MoneyRequestAmountPagecomponent) function seems to be getting called more than once. This leads to unpredictable cursor behavior due to the asynchronous nature of state updates in React.
The solution focuses on preventing multiple invocations of setSelectionwithin setAmount. We introduce a flag hasSelectionBeenSetthat allows setSelectionto be called only once, thereby aligning cursor behavior across different platforms.
Original code block:
const setNewAmount = (newAmount) => {
const newAmountWithoutSpaces = stripSpacesFromAmount(newAmount);
if (!validateAmount(newAmountWithoutSpaces)) {
setAmount((prevAmount) => prevAmount);
setSelection((prevSelection) => ({...prevSelection}));
return;
}
setAmount((prevAmount) => {
setSelection((prevSelection) => getNewSelection(prevSelection, prevAmount.length, newAmountWithoutSpaces.length));
return stripCommaFromAmount(newAmountWithoutSpaces);
});
};Modified code block:
const setNewAmount = (newAmount) => {
const newAmountWithoutSpaces = stripSpacesFromAmount(newAmount);
if (!validateAmount(newAmountWithoutSpaces)) {
setAmount((prevAmount) => prevAmount);
setSelection((prevSelection) => ({...prevSelection}));
return;
}
let hasSelectionBeenSet = false;
setAmount((prevAmount) => {
if (!hasSelectionBeenSet) {
setSelection((prevSelection) => {
hasSelectionBeenSet = true;
return getNewSelection(prevSelection, prevAmount.length, newAmountWithoutSpaces.length);
});
}
return stripCommaFromAmount(newAmountWithoutSpaces);
});
};
ygshbht
commented
1 year ago Thanks Kavi.
To add more details https://github.com/Expensify/App/blob/df6d03bef398d101a7340351603b937a4b23b59f/src/pages/iou/steps/MoneyRequestAmountPage.js#L291-L306
A bit more complex solution involves moving setSelection out of setAmount and doing some restructuring to acheive the same result
Working solution:
https://github.com/Expensify/App/assets/65986357/a50b6971-39c7-44eb-bece-6b2ffde67114
You could also club amount and selection in one state variable as they are almost always updated together So the new modification instead of this
let hasSelectionBeenSet = false;
setAmount((prevAmount) => {
if (!hasSelectionBeenSet) {
setSelection((prevSelection) => {
hasSelectionBeenSet = true;
return getNewSelection(prevSelection, prevAmount.length, newAmountWithoutSpaces.length);
});
}
return stripCommaFromAmount(newAmountWithoutSpaces);
});can be:
setAmountAndSelection(({amount, selection})=>{
return { amount:stripCommaFromAmount(newAmountWithoutSpaces),
selection: getNewSelection(selection, amount.length, newAmountWithoutSpaces.length)
}
})You could also club amount and selection in one state variable as they are almost always updated together So the new modification instead of this
let hasSelectionBeenSet = false;
setAmount((prevAmount) => {
if (!hasSelectionBeenSet) {
setSelection((prevSelection) => {
hasSelectionBeenSet = true;
return getNewSelection(prevSelection, prevAmount.length, newAmountWithoutSpaces.length);
});
}
return stripCommaFromAmount(newAmountWithoutSpaces);
});can be:
setAmountAndSelection(({amount, selection})=>{
return { amount:stripCommaFromAmount(newAmountWithoutSpaces),
selection: getNewSelection(selection, amount.length, newAmountWithoutSpaces.length)
}
})@michaelhaxhiu Eep! 4 days overdue now. Issues have feelings too...
melvin-bot[bot]
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~0186b5465f1e6964a7
melvin-bot[bot]
commented
1 year ago Current assignee @michaelhaxhiu 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 - @abdulrahuman5196 (External)
michaelhaxhiu
commented
1 year ago Passing to External
abdulrahuman5196
commented
1 year ago Reviewing today
michaelhaxhiu
commented
1 year ago How's review coming @abdulrahuman5196 🐰 ?
melvin-bot[bot]
commented
1 year ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
abdulrahuman5196
commented
1 year ago Will check in my morning. Was working on WAQ crossed issues and deploy blockers.
melvin-bot[bot]
commented
1 year ago @michaelhaxhiu @abdulrahuman5196 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!
melvin-bot[bot]
commented
1 year ago Upwork job price has been updated to $2000
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @garrettmknight (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
michaelhaxhiu
commented
1 year ago Note: I'm preparing to go OOO for ~2 weeks and need a BZ buddy to watch over this in the meantime. 🙏
Next step: Please ensure that we push this forward and double price if no winning proposal in the next week.
Thanks in advance @garrettmknight, I can take this back when I get back if it's not complete before then.
ygshbht
commented
1 year ago Hi, I have been waiting for my proposal to be reviewed. I am open to any questions or clarifications on my proposal https://github.com/Expensify/App/issues/23300#issuecomment-1644464026 or alternative solutions https://github.com/Expensify/App/issues/23300#issuecomment-1644517120 https://github.com/Expensify/App/issues/23300#issuecomment-1646833780 or any discussion at all. I have also attached the video showing my solution @abdulrahuman5196 @michaelhaxhiu @garrettmknight
abdulrahuman5196
commented
1 year ago @ygshbht I am unable to understand the root cause provided on the proposal - https://github.com/Expensify/App/issues/23300#issuecomment-1644464026
React's state management involves batching state updates for performance optimization, and in the current implementation, the setSelection function in setNewAmount (MoneyRequestAmountPage component) function seems to be getting called more than once. This leads to unpredictable cursor behavior due to the asynchronous nature of state updates in React.
React's state management involves batching state updates for performance optimization
Can you provide information on this?
the setSelection function in setNewAmount (MoneyRequestAmountPage component) function seems to be getting called more than once
Why does calling this function more than once causes inconsistency? Why this function should be called only one?
Not just the above questions. Could you kindly provide more clear and rigid information on root cause?
And kindly have all the information in one proposal comment(this is also the proposal guidelines.). Post a proposal comment here and add all your information to the same.
ygshbht
commented
1 year ago @abdulrahuman5196
And kindly have all the information in one proposal comment(this is also the proposal guidelines.). Post a proposal comment here and add all your information to the same.
I can't modify the proposal as it is written by kavimuru, however i have merged the following 2 comments into one.
Why does calling this function more than once causes inconsistency? Why this function should be called only one?
In the context of pressing the back button, calling setSelectionis designed to adjust the cursor's position based on the current change. Specifically, the getNewSelectionfunction will move the cursor back by one step, which is the expected behavior. However, due to the current arrangement in the code, setSelectiongets invoked twice in quick succession. This results in the cursor's position being adjusted twice, effectively moving it back by two steps instead of one. This unintended double adjustment is causing the inconsistency in cursor position that we are observing.
Not just the above questions. Could you kindly provide more clear and rigid information on root cause?
In our original implementation, we had a nested state update. Specifically, inside our setAmountstate update, we were calling another state update function, setSelection. This is problematic due to the asynchronous nature of React's state updates.
When we call setAmount, it schedules a state update. Before this update is processed and the component is re-rendered, we're also calling setSelection, effectively scheduling another state update. Given that these updates are asynchronous, the exact timing and sequence of their processing can overlap or vary. This can lead to unexpected behaviors, such as our setSelectionbeing invoked multiple times in quick succession.
In the context of our application, this meant that the cursor's position was being adjusted more than once, leading to the inconsistency observed.
The solution, as demonstrated in the modified code, was to introduce a mechanism (hasSelectionBeenSetflag) to ensure that setSelectionis invoked only once per change, avoiding the complications introduced by the nested state updates. We could also use something like setAmountAndSelectionas mentioned here https://github.com/Expensify/App/issues/23300#issuecomment-1644517120
https://github.com/facebook/react/issues/8132#issuecomment-257088287 https://github.com/facebook/react/issues/8132#issuecomment-334169842
s-alves10
commented
1 year ago Inconsistent behavior when long pressing the back button in Android app and mWeb
I think this isn't an issue due to the strict mode of react because this happens only inside onLongPress handler.
This looks like a bug of react native but I haven't found the similar issue in react native or react native web repo.
Please check the below code snippet
https://github.com/Expensify/App/blob/5f668c669a501a94caebbb434a656580e3bcfb24/src/pages/iou/steps/MoneyRequestAmountForm.js#L130-L135
Inside onLongPress handler, the updater function is called twice, that is, the updater of setCurrentAmount runs twice. react would take any one result as a next state.
This doesn't affect the next state of the currentAmount state because this updater is called with the same prevAmount parameter and its result would be the same. The problem is in the updater of the setSelection. This updater is called with the latest prevSelection value because this is a normal state update.
This is an example of impure updater functions and is the root cause of this issue
And we have a new issue on ios native that the cursor jumps in one number before. As you can see above, we update selection first and update the amount. Updating the selection causes the re-render. When a value is 12345678 and cursor is at 8, updating the selection makes the value 12345678 and cursor 7. So the position of the cursor is between 7 and 8. Next the currentAmount is changed from 12345678 to 1234567. At this moment, the cursor moves by one as well, so the cursor is positioned between 6 and 7. And after that cursor is moved to the last position by the selection props(7). That's why the cursor position flickers sometimes. And the cursor sometimes doesn't move to the last position. This behavior looks like an issue of react-native but I wasn't able to find the issue in the react-native repo
This is the root cause of the new issue
We can solve both the issues by combining two states(currentAmount and selection) into one
const [amountAndSelection, setAmountAndSelection] = useState({
amount: selectedAmountAsString,
selection: {start: selectedAmountAsString.length, end: selectedAmountAsString.length},
});Update the setNewAmount function
setAmountAndSelection((prevAmountAndSelection) => {
const currentAmount = prevAmountAndSelection.amount;
const selection = prevAmountAndSelection.selection;
const strippedAmount = MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces);
const isForwardDelete = currentAmount.length > strippedAmount.length && forwardDeletePressedRef.current;
return {
amount: strippedAmount,
selection: getNewSelection(selection, isForwardDelete ? strippedAmount.length : currentAmount.length, strippedAmount.length),
}
});We need to change other parts of the component using currentAmount and selection
This updates prevent the double call from changing the selection twice and fix the issue due to the inconsistency of amount and selection.
This works perfect on all platforms
Note: If you want to check how it's running, please refer https://github.com/s-alves10/App/blob/fix/issue-23300/src/pages/iou/steps/MoneyRequestAmountForm.js
garrettmknight
commented
1 year ago @abdulrahuman5196 any thoughts on these updated proposals?
melvin-bot[bot]
commented
1 year ago @garrettmknight @michaelhaxhiu @abdulrahuman5196 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!
melvin-bot[bot]
commented
1 year ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
abdulrahuman5196
commented
1 year ago Reviewing now
abdulrahuman5196
commented
1 year ago @ygshbht I tried out this https://github.com/Expensify/App/issues/23300#issuecomment-1644464026 solution and its causing issues for me
https://github.com/Expensify/App/assets/46707890/b1d17bed-cff6-4b2b-9443-9ecf6ac1b98b
Can you put all your proposal in a single comment as per the guideline? You can submit a new proposal comment in your name with all the information
abdulrahuman5196
commented
1 year ago On @s-alves10 's proposal here https://github.com/Expensify/App/issues/23300#issuecomment-1667075869 I don't think we directly convert the functions as mentioned in the proposed changes. I think the prevSelection have some role to play in the cursor position computation. If we change as per the proposal it is causing the below issue.
https://github.com/Expensify/App/assets/46707890/a2e2209f-d3b9-42f1-bed1-21891040a465
ygshbht
commented
1 year ago Original Proposal: https://github.com/Expensify/App/issues/23300#issuecomment-1644464026 https://github.com/Expensify/App/issues/23300#issuecomment-1644517120
Inconsistent cursor behavior when a user long-presses the back button on the 'Split Bill/Request Money' page between Android app and mobile web.
React's state management involves batching state updates for performance optimization, and in the current implementation, the setSelectionfunction in setNewAmount (MoneyRequestAmountPagecomponent) function seems to be getting called more than once. This leads to unpredictable cursor behavior due to the asynchronous nature of state updates in React.
The solution focuses on preventing multiple invocations of setSelectionwithin setAmount. We introduce a flag hasSelectionBeenSetthat allows setSelectionto be called only once, thereby aligning cursor behavior across different platforms. Original code block:
const setNewAmount = (newAmount) => {
const newAmountWithoutSpaces = stripSpacesFromAmount(newAmount);
if (!validateAmount(newAmountWithoutSpaces)) {
setAmount((prevAmount) => prevAmount);
setSelection((prevSelection) => ({...prevSelection}));
return;
}
setAmount((prevAmount) => {
setSelection((prevSelection) => getNewSelection(prevSelection, prevAmount.length, newAmountWithoutSpaces.length));
return stripCommaFromAmount(newAmountWithoutSpaces);
});
};Modified code block:
const setNewAmount = (newAmount) => {
const newAmountWithoutSpaces = stripSpacesFromAmount(newAmount);
if (!validateAmount(newAmountWithoutSpaces)) {
setAmount((prevAmount) => prevAmount);
setSelection((prevSelection) => ({...prevSelection}));
return;
}
let hasSelectionBeenSet = false;
setAmount((prevAmount) => {
if (!hasSelectionBeenSet) {
setSelection((prevSelection) => {
hasSelectionBeenSet = true;
return getNewSelection(prevSelection, prevAmount.length, newAmountWithoutSpaces.length);
});
}
return stripCommaFromAmount(newAmountWithoutSpaces);
});
};After the update to MoneyRequestAmountForm.js, the new code should look like:
const setNewAmount = (newAmount) => {
// Remove spaces from the newAmount value because Safari on iOS adds spaces
// when pasting a copied value
// More info: https://github.com/Expensify/App/issues/16974
const newAmountWithoutSpaces = MoneyRequestUtils.stripSpacesFromAmount(newAmount);
// Use a shallow copy of selection to trigger setSelection
// More info: https://github.com/Expensify/App/issues/16385
if (!MoneyRequestUtils.validateAmount(newAmountWithoutSpaces)) {
setCurrentAmount((prevAmount) => prevAmount);
setSelection((prevSelection) => ({...prevSelection}));
return;
}
let hasSelectionBeenSet = false;
setCurrentAmount((prevAmount) => {
if (!hasSelectionBeenSet) {
setSelection((prevSelection) => {
hasSelectionBeenSet = true;
return getNewSelection(prevSelection, prevAmount.length, newAmountWithoutSpaces.length);
});
}
return MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces);
});
};
__
To add more details https://github.com/Expensify/App/blob/df6d03bef398d101a7340351603b937a4b23b59f/src/pages/iou/steps/MoneyRequestAmountPage.js#L291-L306
A bit more complex solution involves moving setSelection out of setAmount and doing some restructuring to acheive the same result
Working solution:
https://github.com/Expensify/App/assets/65986357/a50b6971-39c7-44eb-bece-6b2ffde67114
You could also club amount and selection in one state variable as they are almost always updated together So the new modification instead of this
let hasSelectionBeenSet = false;
setAmount((prevAmount) => {
if (!hasSelectionBeenSet) {
setSelection((prevSelection) => {
hasSelectionBeenSet = true;
return getNewSelection(prevSelection, prevAmount.length, newAmountWithoutSpaces.length);
});
}
return stripCommaFromAmount(newAmountWithoutSpaces);
});can be:
setAmountAndSelection(({amount, selection})=>{
return { amount:stripCommaFromAmount(newAmountWithoutSpaces),
selection: getNewSelection(selection, amount.length, newAmountWithoutSpaces.length)
}
})Alternate Solution Code: MoneyRequestAmountForm.txt
ygshbht
commented
1 year ago @ygshbht I tried out this #23300 (comment) solution and its causing issues for me
az_recorder_20230813_202032.mp4 Can you put all your proposal in a single comment as per the guideline? You can submit a new proposal comment in your name with all the information
@abdulrahuman5196 What is the code you are using as I am unable to see the issue.
After the update to MoneyRequestAmountForm.js, the new code should look like:
const setNewAmount = (newAmount) => {
// Remove spaces from the newAmount value because Safari on iOS adds spaces
// when pasting a copied value
// More info: https://github.com/Expensify/App/issues/16974
const newAmountWithoutSpaces = MoneyRequestUtils.stripSpacesFromAmount(newAmount);
// Use a shallow copy of selection to trigger setSelection
// More info: https://github.com/Expensify/App/issues/16385
if (!MoneyRequestUtils.validateAmount(newAmountWithoutSpaces)) {
setCurrentAmount((prevAmount) => prevAmount);
setSelection((prevSelection) => ({...prevSelection}));
return;
}
let hasSelectionBeenSet = false;
setCurrentAmount((prevAmount) => {
if (!hasSelectionBeenSet) {
setSelection((prevSelection) => {
hasSelectionBeenSet = true;
return getNewSelection(prevSelection, prevAmount.length, newAmountWithoutSpaces.length);
});
}
return MoneyRequestUtils.stripCommaFromAmount(newAmountWithoutSpaces);
});
};
This is the original solution. Alternate solution can be modified similarly based on the changes happened to the page since the proposal was submitted Alternate solution code: MoneyRequestAmountForm.txt
s-alves10
commented
1 year ago If we change as per the proposal it is causing the below issue.
@abdulrahuman5196
This issue is not related to my proposal. Please check https://expensify.slack.com/archives/C049HHMV9SM/p1691982118109469
ygshbht
commented
1 year ago @abdulrahuman5196 I've also attached code for Alternate Solution. Please feel free to replace this with your existing file and test MoneyRequestAmountForm.txt
melvin-bot[bot]
commented
1 year ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
abdulrahuman5196
commented
1 year ago I need to check on the latest proposal updates. Will review in my morning
melvin-bot[bot]
commented
1 year ago @garrettmknight, @michaelhaxhiu, @abdulrahuman5196 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
garrettmknight
commented
1 year ago @abdulrahuman5196 please review this one too.
abdulrahuman5196
commented
1 year ago I am testing this out. Will need to re-verify if the mentioned issues are present in main as well and not related to this proposal. Will update here in sometime.
ygshbht
commented
1 year ago I can also confirm that It is not related to this issue's root cause or proposals.
abdulrahuman5196
commented
1 year ago Hey, quick update. I think the contributors are correct, it couldn't be related to proposals. I will re-verify both the proposals and come back with an update.
s-alves10
commented
1 year ago On @s-alves10 's proposal here #23300 (comment) I don't think we directly convert the functions as mentioned in the proposed changes. I think the prevSelection have some role to play in the cursor position computation. If we change as per the proposal it is causing the below issue.
Yeah, prevSelection plays some role. prevSelection is the previous value of selection. selection would play the same role in my solution.
ygshbht
commented
1 year ago I'd like to mention that in the alternate solution that I propose, the code gets the prevSelectionvalue from the setStatefunction like it currently does, so there's no potential issues here.
melvin-bot[bot]
commented
1 year ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
abdulrahuman5196
commented
1 year ago Melvin just updated after weekend. Will review and update.
abdulrahuman5196
commented
1 year ago I am seeing couple more inconsistent issues with the back button. But mostly present in main as well. I will try to see if something should be fixed alongside this or should we fix only the issue mentioned using either of the above proposals.
michaelhaxhiu
commented
1 year ago Sounds good @abdulrahuman5196 let us know what you think is ideal to proceed (e.g. individual one-off fix or group a series issue to fix).
melvin-bot[bot]
commented
1 year ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
abdulrahuman5196
commented
1 year ago Will do a another check today. I found couple of previous issues, mostly they where taken one-off. So I am more inclined to same(Atleast for now). I will do another testing and will open slack thread or approve any proposal from here(Last I checked we had proposals works) by today
melvin-bot[bot]
commented
1 year ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
abdulrahuman5196
commented
1 year ago I will take this as one off issue. Re-tested both the solutions(by @ygshbht and @s-alves10 ) and both are working. IMO both are relatively same concept on high level.
Anyways, I was going through the docs shared by @s-alves10
This is a behavior for development and helps us avoid accident impure updaters. Please refer https://react.dev/reference/react/useState#my-initializer-or-updater-function-runs-twice
This behaviour mentioned as a development only on react website This development-only behavior helps you [keep components pure.](https://react.dev/learn/keeping-components-pure) React uses the result of one of the calls, and ignores the result of the other call. As long as your component, initializer, and updater functions are pure
Any idea why this issue seen in our prod as well?
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:
Last digit is removed and cursor is still at last place like Android app
Actual Result:
Last digit is removed but cursor is in front of last character
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?
Version Number: 1.3.43-2 Reproducible in staging?: y Reproducible in production?: y 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/43996225/97e50faa-fe0e-485e-aa9f-055266662742
https://github.com/Expensify/App/assets/43996225/dd064c2c-a3ff-4e05-a436-854e29cbe978
Expensify/Expensify Issue URL: Issue reported by: @ygshbht Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1689740152146159
View all open jobs on GitHub
Upwork Automation - Do Not Edit