melvin-bot[bot]
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~01a7c5579c82436a8d
Closed kavimuru closed 11 months ago
melvin-bot[bot]
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~01a7c5579c82436a8d
melvin-bot[bot]
commented
1 year ago Current assignee @slafortune 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 - @mananjadhav (External)
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @roryabraham (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
esh-g
commented
1 year ago Solution
We can add a callback function to the createWorkspace method that disables the button and enables it only after the workspace is successfully created. This can be wither implemented via a callback method or promise
Option 1: (callback)
diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js
index c269accf6..de927f481 100644
--- a/src/libs/actions/Policy.js
+++ b/src/libs/actions/Policy.js
@@ -940,6 +940,7 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
Navigation.dismissModal(); // Dismiss /transition route for OldDot to NewDot transitions
}
Navigation.navigate(ROUTES.getWorkspaceInitialRoute(policyID));
+ callback();
});
}
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 5ef22eea7..d394a3240 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -93,9 +93,16 @@ class WorkspacesListPage extends Component {
constructor(props) {
super(props);
+ this.state = {
+ isDisabled: false,
+ }
+
+
this.getWalletBalance = this.getWalletBalance.bind(this);
this.getWorkspaces = this.getWorkspaces.bind(this);
this.getMenuItem = this.getMenuItem.bind(this);
+ this.createNewWorkspace = this.createNewWorkspace.bind(this);
+
}
/**
@@ -171,6 +178,11 @@ class WorkspacesListPage extends Component {
);
}
+ createNewWorkspace() {
+ this.setState({ isDisabled: true });
+ Policy.createWorkspace(undefined, undefined, undefined, undefined, () => this.setState({ isDisabled: false }));
+ }
+
render() {
const workspaces = this.getWorkspaces();
return (
@@ -195,8 +207,9 @@ class WorkspacesListPage extends Component {
<FixedFooter style={[styles.flexGrow0]}>
<Button
success
+ isDisabled={this.state.isDisabled}
text={this.props.translate('workspace.new.newWorkspace')}
- onPress={() => Policy.createWorkspace()}
+ onPress={this.createNewWorkspace}
/>
</FixedFooter>
</ScreenWrapper>
Option 2: (Promise)
diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js
index c269accf6..032d4f27e 100644
--- a/src/libs/actions/Policy.js
+++ b/src/libs/actions/Policy.js
@@ -934,7 +934,7 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
],
});
- Navigation.isNavigationReady()
+ return Navigation.isNavigationReady()
.then(() => {
if (transitionFromOldDot) {
Navigation.dismissModal(); // Dismiss /transition route for OldDot to NewDot transitions
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 5ef22eea7..faf2fb523 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -93,9 +93,16 @@ class WorkspacesListPage extends Component {
constructor(props) {
super(props);
+ this.state = {
+ isDisabled: false,
+ }
+
+
this.getWalletBalance = this.getWalletBalance.bind(this);
this.getWorkspaces = this.getWorkspaces.bind(this);
this.getMenuItem = this.getMenuItem.bind(this);
+ this.createNewWorkspace = this.createNewWorkspace.bind(this);
+
}
/**
@@ -135,6 +142,8 @@ class WorkspacesListPage extends Component {
.value();
}
+ componentDidUpdate
+
/**
* Gets the menu item for each workspace
*
@@ -171,6 +180,11 @@ class WorkspacesListPage extends Component {
);
}
+ createNewWorkspace() {
+ this.setState({ isDisabled: true });
+ Policy.createWorkspace().then(() => this.setState({ isDisabled: false }));
+ }
+
render() {
const workspaces = this.getWorkspaces();
return (
@@ -195,8 +209,9 @@ class WorkspacesListPage extends Component {
<FixedFooter style={[styles.flexGrow0]}>
<Button
success
+ isDisabled={this.state.isDisabled}
text={this.props.translate('workspace.new.newWorkspace')}
- onPress={() => Policy.createWorkspace()}
+ onPress={this.createNewWorkspace}
/>
</FixedFooter>
</ScreenWrapper>
After Fix
melvin-bot[bot]
commented
1 year ago Looks like something related to react-navigation may have been mentioned in this issue discussion.
As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.
Feel free to drop a note in #expensify-open-source with any questions.
Puneet-here
commented
1 year ago We can use a new onyx key to check if the workspace is being created and use that info to disable the button or set it to loading state
Add a new key IS_CREATING_NEW_WORKSPACE at ONYXKEYS
Change it's value to true when user presses the new workspace button like below at Policy.js
--- a/src/libs/actions/Policy.js
+++ b/src/libs/actions/Policy.js
@@ -762,6 +762,12 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
},
{
optimisticData: [
+ {
+ onyxMethod: CONST.ONYX.METHOD.SET,
+ key: ONYXKEYS.IS_CREATING_NEW_WORKSPACE,
+ value: true,
+ },
+
{
onyxMethod: CONST.ONYX.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
@@ -832,6 +838,11 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
},
],
successData: [
+ {
+ onyxMethod: CONST.ONYX.METHOD.SET,
+ key: ONYXKEYS.IS_CREATING_NEW_WORKSPACE,
+ value: false,
+ },
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
@@ -896,6 +907,11 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
},
],
failureData: [
+ {
+ onyxMethod: CONST.ONYX.METHOD.SET,
+ key: ONYXKEYS.IS_CREATING_NEW_WORKSPACE,
+ value: false,
+ },
{
onyxMethod: CONST.ONYX.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${policyID}`,
Use the new key to set the button to loading (we can also disable it) like below at WorkspacesListPage
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -197,6 +197,7 @@ class WorkspacesListPage extends Component {
success
text={this.props.translate('workspace.new.newWorkspace')}
onPress={() => Policy.createWorkspace()}
+ isLoading={this.props.isCreatingNewWorkspace}
/>
</FixedFooter>
</ScreenWrapper>
@@ -223,5 +224,8 @@ export default compose(
betas: {
key: ONYXKEYS.BETAS,
},
+ isCreatingNewWorkspace: {
+ key: ONYXKEYS.IS_CREATING_NEW_WORKSPACE,
+ },
}),
)(WorkspacesListPage);@Puneet-here What would be the need to create an Onyx key for this? Why not just implement it in the state of the component itself?
rushatgabhane
commented
1 year ago How about we debounce the button?
onPress=_.debounce(createNewWorkspace, 250)
edit: third param can be set as true to trigger the on press call immediately
https://www.freecodecamp.org/news/javascript-debounce-example/
priyeshshah11
commented
1 year ago This issue will exist for all the buttons used throughout the app, thus I think we should fix this in the Button component itself.
We should disable the button until the onPress is completed, we can do this by passing a promise and wait for business logic before re-enabling the button where needed. Also, I am not sure whether this issue exists on 'Enter' presses too but if it is then we can apply the same logic there.
Note: We are applying a similar fix here https://github.com/Expensify/App/pull/14426
diff --git a/src/components/Button.js b/src/components/Button.js
index 6e6fbade7..6854ae096 100644
--- a/src/components/Button.js
+++ b/src/components/Button.js
@@ -1,5 +1,7 @@
import React, {Component} from 'react';
-import {Pressable, ActivityIndicator, View} from 'react-native';
+import {
+ Pressable, ActivityIndicator, View, InteractionManager,
+} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../styles/styles';
import themeColors from '../styles/themes/default';
@@ -144,6 +146,9 @@ const defaultProps = {
class Button extends Component {
constructor(props) {
super(props);
+ this.state = {
+ isDisabled: props.isDisabled,
+ };
this.renderContent = this.renderContent.bind(this);
}
@@ -157,7 +162,7 @@ class Button extends Component {
// Setup and attach keypress handler for pressing the button with Enter key
this.unsubscribe = KeyboardShortcut.subscribe(shortcutConfig.shortcutKey, (e) => {
:...skipping...
diff --git a/src/components/Button.js b/src/components/Button.js
index 6e6fbade7..6854ae096 100644
--- a/src/components/Button.js
+++ b/src/components/Button.js
@@ -1,5 +1,7 @@
import React, {Component} from 'react';
-import {Pressable, ActivityIndicator, View} from 'react-native';
+import {
+ Pressable, ActivityIndicator, View, InteractionManager,
+} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../styles/styles';
import themeColors from '../styles/themes/default';
@@ -144,6 +146,9 @@ const defaultProps = {
class Button extends Component {
constructor(props) {
super(props);
+ this.state = {
+ isDisabled: props.isDisabled,
+ };
this.renderContent = this.renderContent.bind(this);
}
@@ -157,7 +162,7 @@ class Button extends Component {
// Setup and attach keypress handler for pressing the button with Enter key
this.unsubscribe = KeyboardShortcut.subscribe(shortcutConfig.shortcutKey, (e) => {
- if (!this.props.isFocused || this.props.isDisabled || this.props.isLoading || (e && e.target.nodeName === 'TEXTAREA')) {
+ if (!this.props.isFocused || this.state.isDisabled || this.props.isLoading || (e && e.target.nodeName === 'TEXTAREA')) {
return;
}
e.preventDefault();
@@ -165,6 +170,14 @@ class Button extends Component {
}, shortcutConfig.descriptionKey, shortcutConfig.modifiers, true, false, this.props.enterKeyEventListenerPriority, false);
}
+ componentDidUpdate(prevProps) {
+ if (this.props.isDisabled === prevProps.isDisabled) {
+ return;
+ }
+
+ this.setState({isDisabled: this.props.isDisabled});
+ }
+
componentWillUnmount() {
// Cleanup event listeners
if (!this.unsubscribe) {
@@ -234,6 +247,7 @@ class Button extends Component {
return (
<Pressable
onPress={(e) => {
+ this.setState({isDisabled: true});
if (e && e.type === 'click') {
e.currentTarget.blur();
}
@@ -241,7 +255,16 @@ class Button extends Component {
if (this.props.shouldEnableHapticFeedback) {
HapticFeedback.trigger();
}
- this.props.onPress(e);
+ const onPress = this.props.onPress(e);
+ InteractionManager.runAfterInteractions(() => {
+ if (!(onPress instanceof Promise)) {
+ this.setState({isDisabled: this.props.isDisabled});
+ return;
+ }
+ onPress.then(() => {
+ this.setState({isDisabled: this.props.isDisabled});
+ });
+ });
}}
onLongPress={(e) => {
if (this.props.shouldEnableHapticFeedback) {
@@ -252,9 +275,9 @@ class Button extends Component {
onPressIn={this.props.onPressIn}
onPressOut={this.props.onPressOut}
onMouseDown={this.props.onMouseDown}
- disabled={this.props.isLoading || this.props.isDisabled}
+ disabled={this.props.isLoading || this.state.isDisabled}
style={[
- this.props.isDisabled ? {...styles.cursorDisabled, ...styles.noSelect} : {},
+ this.state.isDisabled ? {...styles.cursorDisabled, ...styles.noSelect} : {},
styles.buttonContainer,
this.props.shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
this.props.shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
@@ -263,7 +286,7 @@ class Button extends Component {
nativeID={this.props.nativeID}
>
{({pressed, hovered}) => {
- const activeAndHovered = !this.props.isDisabled && hovered;
+ const activeAndHovered = !this.state.isDisabled && hovered;
return (
<OpacityView
shouldDim={pressed}
@@ -274,9 +297,9 @@ class Button extends Component {
this.props.large ? styles.buttonLarge : undefined,
this.props.success ? styles.buttonSuccess : undefined,
this.props.danger ? styles.buttonDanger : undefined,
- (this.props.isDisabled && this.props.success) ? styles.buttonSuccessDisabled : undefined,
- (this.props.isDisabled && this.props.danger) ? styles.buttonDangerDisabled : undefined,
- (this.props.isDisabled && !this.props.danger && !this.props.success) ? styles.buttonDisable : undefined,
+ (this.state.isDisabled && this.props.success) ? styles.buttonSuccessDisabled : undefined,
+ (this.state.isDisabled && this.props.danger) ? styles.buttonDangerDisabled : undefined,
+ (this.state.isDisabled && !this.props.danger && !this.props.success) ? styles.buttonDisable : undefined,
(this.props.success && activeAndHovered) ? styles.buttonSuccessHovered : undefined,
(this.props.danger && activeAndHovered) ? styles.buttonDangerHovered : undefined,
this.props.shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js
index 5a2de0e7e..a47777d0c 100644
--- a/src/libs/actions/Policy.js
+++ b/src/libs/actions/Policy.js
@@ -698,6 +698,7 @@ function generatePolicyID() {
* @param {Boolean} [makeMeAdmin] Optional, leave the calling account as an admin on the policy
* @param {String} [policyName] Optional, custom policy name we will use for created workspace
* @param {Boolean} [transitionFromOldDot] Optional, if the user is transitioning from old dot
+ * @returns {Promise} Navigation.isNavigationReady promise
*/
function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '', transitionFromOldDot = false) {
const policyID = generatePolicyID();
@@ -899,7 +900,7 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
}],
});
- Navigation.isNavigationReady()
+ return Navigation.isNavigationReady()
.then(() => {
if (transitionFromOldDot) {
Navigation.dismissModal(); // Dismiss /transition route for OldDot to NewDot transitions
@mananjadhav @roryabraham
sbsoso0411
commented
1 year ago hey, @kavimuru I've looked at the post at Upwork and now writing a proposal.
Above all, I can't really understand why all of the above proposals contain the code-panel. I don't wonna do that.
disable flag of the New Workspace Button. Now it is not working correctly when you click n times on the button in a very short time. correct?
ref feature in React. So createRef for class components and useRef for functional components.debounce? So we can avoid multiple calls of event listeners by debouncing the listener functions. Anyway the first solution would be better I think.state in react components to disable the button status, you can't avoid triple button click events. Because react state is only chagned when the component is re-rendered. But the event listeners can be called several times before re-rendering! So you can't avoid the issue using react state.ref is different. It uses the reference of the variable on memory. Which means it has its own absolute address on the memory. So even the action listener is called several times, once the variable changes, it is updated on the next-coming action listeners.Hope my description is well-written and acceptable for you. Looking forward to hearing from you. Thank you.
priyeshshah11
commented
1 year ago Above all, I can't really understand why all of the above proposals contain the code-panel. I don't wonna do that.
Hi @sbsoso0411, this might help you understand the process.
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
eh2077
commented
1 year ago Proposal
I'd like to propose a simple solution using _.debounce to address this issue. See below one line diff
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 5ef22eea79..13b3722ef4 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -196,7 +196,7 @@ class WorkspacesListPage extends Component {
<Button
success
text={this.props.translate('workspace.new.newWorkspace')}
- onPress={() => Policy.createWorkspace()}
+ onPress={_.debounce(() => Policy.createWorkspace(), 1000, false)}
/>
</FixedFooter>
</ScreenWrapper>
fedirjh
commented
1 year ago When creating a new workspace, we set optimisticData for the new policy with a pendingAction of add. In WorkspacesListPage.js, we've already subscribed to the policies collection and have access to the new optimisticData. To avoid creating a new key as suggested by @Puneet, we can filter the policies based on their pendingAction. When we find a policy with a pendingAction of add, then we set the button's isLoading prop to true.
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 5ef22eea79..23049e0b1a 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -195,6 +195,10 @@ class WorkspacesListPage extends Component {
<FixedFooter style={[styles.flexGrow0]}>
<Button
success
+ isLoading={_.some(
+ workspaces,
+ workspace => workspace && workspace.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
+ )}
text={this.props.translate('workspace.new.newWorkspace')}
onPress={() => Policy.createWorkspace()}
/>
roryabraham
commented
1 year ago We can add a callback function to [or return a promise from] the createWorkspace method that disables the button and enables it only after the workspace is successfully created ... What would be the need to create an Onyx key for this? Why not just implement it in the state of the component itself?
@esh-g We have established a pattern in this repo that:
src/libs/actionssrc/libs/actions should not return a promise or be "chained"This sometimes leads to extra complexity (like when we create Onyx keys that really don't need to be written to disk), but sometimes all that's needed is a bit more creative thinking to find a clean solution that fits within our app's existing patterns.
So while your proposal is totally reasonable, it doesn't follow our patterns as closely as some others.
We can use a new onyx key to check if the workspace is being created and use that info to disable the button or set it to loading state
@Puneet-here this proposal shows a better understanding of our patterns, but overall I would really like to avoid adding any new Onyx keys for values that could instead be in-memory-only. We've discussed creating "ram-only" Onyx keys, but that project hasn't happened yet.
So overall, I personally would prefer @fedirjh's proposal that uses the pendingAction field of the workspace instead of a new IS_CREATING_NEW_WORKSPACE Onyx key. I think that would help keep our codebase cleaner. However, there might be a performance downside to that approach (explained below) and we're discussing in slack, so stay tuned.
@fedirjh My only concern about your idea to use the pendingAction field as written is that it requires us to loop over all policies. I think it might be too expensive to loop over all policies in a render function, since we have some customers that have thousands of policies.
I'm trying to think of a good way to handle this.
How about we debounce the button?
@rushatgabhane This might work, but overall it feels less robust than other solutions. What if you're on a slow internet connection and it takes 1600ms to create the policy?
This issue will exist for all the buttons used throughout the app, thus I think we should fix this in the Button component itself. We should disable the button until the onPress is completed, we can do this by passing a promise and wait for business logic before re-enabling the button where needed. Also, I am not sure whether this issue exists on 'Enter' presses too but if it is then we can apply the same logic there. Note: We are applying a similar fix here https://github.com/Expensify/App/pull/14426
@priyeshshah11 I like this approach a lot, like that it integrates InteractionManager, and like that it follows a pattern set by an existing PR. I also like that it solves the problem more globally and creates a blueprint to solve similar problems in the same way.
The main problem I see with your proposal is that it returns a Promise from an action in src/libs/actions, which as I said above to @esh-g is something we try to avoid in this repo.
[@sbsoso0411] Above all, I can't really understand why all of the above proposals contain the code-panel [@priyeshshah11] Hi @sbsoso0411, this might help you understand the process.
I just wanted to set the record straight here. There is absolutely no requirement that you include actual code in your proposal. In fact, we consider writing a proposal in plain English to be a best practice. So I applaud @sbsoso0411 for simply explaining their proposal without just posting the code.
That said, if you're just joining us from Upwork, welcome! @priyeshshah11 gives good advice –read the contributing guidelines and join our slack rooms for the best contributing experience.
@sbsoso0411 I think your proposal is missing some important information - when do we disable/enable the button? What is the source of truth for when the button is enabled or disabled? However...
If you use state in react components to disable the button status, you can't avoid triple button click events. Because react state is only chagned when the component is re-rendered. But the event listeners can be called several times before re-rendering! So you can't avoid the issue using react state. But ref is different. It uses the reference of the variable on memory. Which means it has its own absolute address on the memory. So even the action listener is called several times, once the variable changes, it is updated on the next-coming action listeners.
This is a great point, and something I wished we had thought of before implementing https://github.com/Expensify/App/pull/14426 (cc @syedsaroshfarrukhdot @eVoloshchak)
In conclusion, many of the pieces of an ideal proposal are here, but I don't think any one proposal is quite right yet. Thanks for all your contributions so far, everyone!
Puneet-here
commented
1 year ago Thanks for the detailed feedback @roryabraham
roryabraham
commented
1 year ago @fedirjh My only concern about your idea to use the pendingAction field as written is that it requires us to loop over all policies. I think it might be too expensive to loop over all policies in a render function, since we have some customers that have thousands of policies.
Following up on this, I was wrong and I don't think we'll have a performance issue here. I tested it with this tiny script:
const _ = require('underscore');
const policies = {};
for (let i=0; i < 10000; i++) {
if (i === 9999) {
policies[i] = {pendingAction: 'add'};
} else {
policies[i] = {};
}
}
const startTime = Date.now();
const isAnyPolicyPending = _.some(policies, policy => policy && policy.pendingAction === 'add');
const endTime = Date.now();
console.log(`Search took ${endTime - startTime} milliseconds`);Even with 10000 items and a worst-case scenario where the random-generated policyID Onyx key is higher than any existing policyID, it only took like 3ms, so that should be fine.
rushatgabhane
commented
1 year ago How about we debounce the button? @rushatgabhane This might work, but overall it feels less robust than other solutions. What if you're on a slow internet connection and it takes 1600ms to create the policy?
@roryabraham i don't think it matters how long it takes for a policy to create.
When the debounce time is 250ms -> all button clicks that happen within 250ms will trigger a single onPress callback only.
https://www.freecodecamp.org/news/javascript-debounce-example/
please correct if I'm wrong.
roryabraham
commented
1 year ago @roryabraham i don't think it matters how long it takes for a policy to create. When the debounce time is 250ms -> all button clicks that happen within 250ms will trigger a single onPress callback only.
Right, but what if:
I think that would trigger another API command to create a second new policy, because the 250ms debounce period would already have elapsed.
rushatgabhane
commented
1 year ago Oh yeah you're right! I see it now
fedirjh
commented
1 year ago I think it might be too expensive to loop over all policies in a render function, since we have some customers that have thousands of policies.
@roryabraham we are already looping over all policies inside getWorkspaces , we can use same loop to detect if policy have a pending action of add and make getWorkspaces return both workspaces and hasPendingAddAction boolean
getWorkspacesWithPendingAction() {
let hasPendingAddAction = false;
const workspaces = _.chain(this.props.policies)
.filter(policy => PolicyUtils.shouldShowPolicy(policy, this.props.network.isOffline))
.map((policy) => {
const workspace = {
title: policy.name,
icon: policy.avatar ? policy.avatar : Expensicons.Building,
iconType: policy.avatar ? CONST.ICON_TYPE_AVATAR : CONST.ICON_TYPE_ICON,
action: () => Navigation.navigate(ROUTES.getWorkspaceInitialRoute(policy.id)),
iconStyles: policy.avatar ? [] : [styles.popoverMenuIconEmphasized],
iconFill: themeColors.textLight,
fallbackIcon: Expensicons.FallbackWorkspaceAvatar,
brickRoadIndicator: PolicyUtils.getPolicyBrickRoadIndicatorStatus(policy, this.props.policyMembers),
pendingAction: policy.pendingAction,
errors: policy.errors,
dismissError: () => dismissWorkspaceError(policy.id, policy.pendingAction),
disabled: policy.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
};
if (policy.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
hasPendingAddAction = true;
}
return workspace;
})
.sortBy(policy => policy.title)
.value();
return [hasPendingAddAction, workspaces];
}then inside render use
const [hasPendingAddAction, workspaces] = this.getWorkspacesWithPendingAction();@roryabraham Thanks for the detailed feedback on all the proposals, I feel @fedirjh's latest solution will solve this specific issue & combining that with my proposal without the Promise bit would solve the multiple clicking issue in general throughout the app for all Buttons.
mananjadhav
commented
1 year ago Thanks @roryabraham for the detailed feedback, sorry I just got to this.
Based on all the proposals so far, I am inclined towards @fedirjh's proposal.
@priyeshshah11 Can you share an example of 1-2 other places where the similar issue exists?
combining that with my proposal without the Promise bit
Also can you elaborate on this statement? Are you suggesting using both the proposals in one approach? Could you explain how do you plan to use your proposal without the Promise?
priyeshshah11
commented
1 year ago @priyeshshah11 Can you share an example of 1-2 other places where the similar issue exists?
@mananjadhav I am not aware of the specific places in the app but this bug would appear anywhere in the app where we display the button until the onPress action is completed. This bug had the same root cause and this might be easily reproducible with slow internet/device.
Also can you elaborate on this statement? Are you suggesting using both the proposals in one approach?
So what I am suggesting is to combine @fedirjh's proposal to fix this specific issue but that doesn't really fix the root cause which is that you can click a button multiple times even while that action is being executed. Thus, my proposal would fix that by disabling the button until the action is completed.
Could you explain how do you plan to use your proposal without the Promise?
I am just saying not to include this change in my proposal
- Navigation.isNavigationReady()
+ return Navigation.isNavigationReady()I'll post another diff below to make it clear.
priyeshshah11
commented
1 year ago I would suggest using state for storing workspaces & pending flag and also to use component life cycle methods rather than getting workspaces on every re-render. We can modify the props check in componentDidUpdate to only get workspaces when necessary.
Apply the below diff in addition to my previous proposal to fix the issue in general.
Apply the below diff just by itself to fix this specific issue.
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 5ef22eea7..3ee558459 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -96,6 +96,20 @@ class WorkspacesListPage extends Component {
this.getWalletBalance = this.getWalletBalance.bind(this);
this.getWorkspaces = this.getWorkspaces.bind(this);
this.getMenuItem = this.getMenuItem.bind(this);
+ this.state = {workspaces: [], hasPendingAddAction: false};
+ }
+
+ componentDidMount() {
+ const [isPending, workspaces] = this.getWorkspaces();
+ this.setState({hasPendingAddAction: isPending, workspaces});
+ }
+
+ componentDidUpdate(prevProps) {
+ if (_.isEqual(prevProps, this.props)) {
+ return;
+ }
+ const [isPending, workspaces] = this.getWorkspaces();
+ this.setState({hasPendingAddAction: isPending, workspaces});
}
/**
@@ -115,24 +129,32 @@ class WorkspacesListPage extends Component {
* @returns {Array} the menu item list
*/
getWorkspaces() {
- return _.chain(this.props.policies)
+ let isPending = false;
+ const workspaces = _.chain(this.props.policies)
.filter(policy => PolicyUtils.shouldShowPolicy(policy, this.props.network.isOffline))
- .map(policy => ({
- title: policy.name,
- icon: policy.avatar ? policy.avatar : Expensicons.Building,
- iconType: policy.avatar ? CONST.ICON_TYPE_AVATAR : CONST.ICON_TYPE_ICON,
- action: () => Navigation.navigate(ROUTES.getWorkspaceInitialRoute(policy.id)),
- iconStyles: policy.avatar ? [] : [styles.popoverMenuIconEmphasized],
- iconFill: themeColors.textLight,
- fallbackIcon: Expensicons.FallbackWorkspaceAvatar,
- brickRoadIndicator: PolicyUtils.getPolicyBrickRoadIndicatorStatus(policy, this.props.policyMembers),
- pendingAction: policy.pendingAction,
- errors: policy.errors,
- dismissError: () => dismissWorkspaceError(policy.id, policy.pendingAction),
- disabled: policy.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
- }))
+ .map((policy) => {
+ const workspace = {
+ title: policy.name,
+ icon: policy.avatar ? policy.avatar : Expensicons.Building,
+ iconType: policy.avatar ? CONST.ICON_TYPE_AVATAR : CONST.ICON_TYPE_ICON,
+ action: () => Navigation.navigate(ROUTES.getWorkspaceInitialRoute(policy.id)),
+ iconStyles: policy.avatar ? [] : [styles.popoverMenuIconEmphasized],
+ iconFill: themeColors.textLight,
+ fallbackIcon: Expensicons.FallbackWorkspaceAvatar,
+ brickRoadIndicator: PolicyUtils.getPolicyBrickRoadIndicatorStatus(policy, this.props.policyMembers),
+ pendingAction: policy.pendingAction,
+ errors: policy.errors,
+ dismissError: () => dismissWorkspaceError(policy.id, policy.pendingAction),
+ disabled: policy.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
+ };
+ if (policy.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
+ isPending = true;
+ }
+ return workspace;
+ })
.sortBy(policy => policy.title)
.value();
+ return [isPending, workspaces];
}
/**
@@ -172,7 +194,6 @@ class WorkspacesListPage extends Component {
}
render() {
- const workspaces = this.getWorkspaces();
return (
<ScreenWrapper>
<HeaderWithCloseButton
@@ -181,7 +202,7 @@ class WorkspacesListPage extends Component {
onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS)}
onCloseButtonPress={() => Navigation.dismissModal(true)}
/>
- {_.isEmpty(workspaces) ? (
+ {_.isEmpty(this.state.workspaces) ? (
<BlockingView
icon={Expensicons.Building}
title={this.props.translate('workspace.emptyWorkspace.title')}
@@ -189,13 +210,14 @@ class WorkspacesListPage extends Component {
/>
) : (
<ScrollView style={styles.flex1}>
- {_.map(workspaces, (item, index) => this.getMenuItem(item, index))}
+ {_.map(this.state.workspaces, (item, index) => this.getMenuItem(item, index))}
</ScrollView>
)}
<FixedFooter style={[styles.flexGrow0]}>
<Button
success
text={this.props.translate('workspace.new.newWorkspace')}
+ isDisabled={this.state.hasPendingAddAction}
onPress={() => Policy.createWorkspace()}
/>
</FixedFooter>
@mananjadhav
fedirjh
commented
1 year ago to fix the issue in general.
@priyeshshah11 your first proposal require changing all Button usage in the app to include a promise as an onPress prop . Does that sound reasonable to you ? .In other words, Implementing these changes without refactoring all button usage will break it.
priyeshshah11
commented
1 year ago to fix the issue in general.
@priyeshshah11 your first proposal require changing all Button usage in the app to include a promise as an onPress prop . Does that sound reasonable to you ? .In other words, Implementing these changes without refactoring all button usage will break it.
Nope @fedirjh, it doesn't require us to update all occurrences to pass in a Promise to the onPress prop, we only need to do that where-ever we would like to wait for the business logic, no changes are required in other places.
fedirjh
commented
1 year ago we only need to do that where-ever we would like to wait for the business logic.
We can use isLoading or isDisabled props
no changes are required in other places.
Calling .then on non promises will throw an error
priyeshshah11
commented
1 year ago Calling
.thenon non promises will throw an error
@fedirjh This is from my proposal above 😅
fedirjh
commented
1 year ago Hmmm then onPress won't be triggered unless it's a promise ?
priyeshshah11
commented
1 year ago Hmmm then onPress won't be triggered unless it's a promise ?
It will be triggered, the very first line in the diff above triggers it.
MelvinBot
commented
1 year ago @mananjadhav, @slafortune, @roryabraham Eep! 4 days overdue now. Issues have feelings too...
eVoloshchak
commented
1 year ago Will review proposals tomorrow
UPD: oops, nevermind, just noticed there is already C+ and a review)
MelvinBot
commented
1 year ago @mananjadhav @slafortune @roryabraham 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!
MelvinBot
commented
1 year ago @mananjadhav, @slafortune, @roryabraham 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
MelvinBot
commented
1 year ago Upwork job price has been updated to $2000
mananjadhav
commented
1 year ago Sorry folks I was out sick and now back at tracking the issues.
I had mentioned this earlier too that I am fine with @fedirjh's proposal here.
@priyeshshah11 While the solution of making it generic looks good, I don't think having checks like instanceof Promise is something we follow here. Also I am not sure what you meant by not to include the return change. @roryabraham can decide this further, but generally I would avoid having complex logic of storing isDisabled in the state. I would personally want this to be driven by props only! Secondly about the revised proposal, I think it is on the lines with @fedirjh's proposal with some enhancements.
@roryabraham All yours.
priyeshshah11
commented
1 year ago @priyeshshah11 While the solution of making it generic looks good, I don't think having checks like instanceof Promise is something we follow here.
@mananjadhav We already applied a similar fix here thus I was just trying to follow the same approach here.
Also I am not sure what you meant by not to include the
returnchange.
All I meant was not to return a promise if that goes against our patterns. However, I still think that would be beneficial to wait for business logic to be completed in such situations rather than relying on just state updates. I don't know why we don't like to wait for API responses but sometimes it can be totally acceptable/needed to wait for one API to be completed before performing other actions. But I understand if it's too big of an issue & we don't want to reconsider/change that guideline or policy.
generally I would avoid having complex logic of storing isDisabled in the state. I would personally want this to be driven by props only!
I agree generally that's a good rule, but I feel it's ok to let the component handle that state in such situations where that state depends on some other prop too (i.e. waiting for onPress callback to be completed) so that we don't have to manage that (isDisabled) state in all it's consuming components
slafortune
commented
1 year ago @roryabraham are the next steps here for you?
MelvinBot
commented
1 year ago @mananjadhav @slafortune @roryabraham 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!
MelvinBot
commented
1 year ago @mananjadhav, @slafortune, @roryabraham Huh... This is 4 days overdue. Who can take care of this?
cubuspl42
commented
1 year ago There are multiple problems that we're observing and/or discussing.
The first one is a problem of bouncing, i.e. an unintended double-click or double-tap. It's a common problem where, because of the imperfectness of the input device (a mouse, or even a finger) what was meant to be one action is registered as two of more. That's how I understand the user scenario in the bug description. This assumes that the double tap is indeed unintended.
Another, hypothetical problem (and probably a much bigger one) would appear if we assumed that the double-tap is intentional and is performed out of frustration ("why is this workspace not created yet?!"). I'm not sure if it's the case. But if it was, please note that the root cause doesn't lie in anything network-related. On the video, we can see that (visually) the second tap is performed on a dimmed button. This isn't related to the pending network request, as this code fragment doesn't have any logic like that implemented. As far as I can understand, it's a hang on the JavaScript code side.
The root cause of the first problem (bouncing) is the lack of any mechanism to prevent it.
The root cause of the second problem would be inefficient code. I haven't determined the precise location of the inefficiency (and there might not be a single location), but in general it seems that most time of the hang is spent in React renders that are a consequence of workspace creation.
The solution to the first problem, problem of "bouncing", is called "debouncing". Like @roryabraham mentioned, typical solution to the problem of debouncing is using ReactiveX/lodash operator debounce.
Ironically, this is often not the best approach. Debouncing has a delay & starvation issue. It means that a) no reaction can happen before the debouncing window passes and b) in theory, no reaction could happen ever (!) if one were to press the button again and again within the debouncing window. In practice, using it would meant that we would add an additional delay in every situation, including the typical, optimistic case (single click/tap).
In this particular case, what would work best is an operator called throttle, with parameters {leading: true, trailing: false}). It means that the first press is reacted to immediately. Within the throttling window, all other actions are ignored. After the throttling window ends, actions are processed again.
The only catch is that the throttling window has to be adjusted to take the before-mentioned performance situation into consideration.
I have tested this approach with artificial extreme network delays. No practical issue occurs, as the "optimistic Onyx write" kicks in and the user is presented with an client-side preview of the workspace right after the button press, before the network request finished.
Fixing the second problem (code inefficiency) could be very difficult and deep inside the code base. I'd assume that it's out of the scope of this issue, but it's a matter open to discussion.
Another option, which came to my mind when testing the above solution, would be just unconditionally disabling the button after it's pressed, purely on the view side. This state would reset each time WorkspacesListPage route would be entered. This solution is simple and relatively elegant. Doesn't require creating new Onyx keys or changing the existing code organization patterns. If this approach would be considered preferred to the above one, I could go into details.
This goes under assumption that the only possible application behavior after pressing the button is entering the WorkspaceInitialPage, but with the optymistic Onyx write it seems to be the case.
Edit: Edited the descriptions with more details and observations.
MelvinBot
commented
1 year ago @mananjadhav @slafortune @roryabraham this issue is now 4 weeks old and preventing us from maintaining WAQ, can you:
Thanks!
MelvinBot
commented
1 year ago Current assignee @mananjadhav is eligible for the Internal assigner, not assigning anyone new.
MelvinBot
commented
1 year ago @mananjadhav, @slafortune, @roryabraham Still overdue 6 days?! Let's take care of this!
mananjadhav
commented
1 year ago @roryabraham Can you take a look at this one?
MelvinBot
commented
1 year ago 📣 @mananjadhav! 📣
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>@roryabraham This issue has been here for some time! Would you consider commenting on my proposal? I've updated it to include more observations, and I tried to take into consideration your comments on other proposals.
MelvinBot
commented
1 year ago Upwork job price has been updated to $2000
roryabraham
commented
1 year ago Really sorry this issue has stalled on my account.
I think for the sake of #focus I'm going to award this job to @fedirjh for this proposal.
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
HOLD on https://github.com/Expensify/App/pull/17404
Action Performed:
Expected Result:
Note: The issue is reproducible with all buttons e.g. the IOU's I'll settle up elsewhere button as reported on https://github.com/Expensify/App/issues/17167.
Actual Result:
Clicking ‘New workspace’ button ‘n’ number of times creates ‘n’ number of workspaces
If you follow the same step from 1-4 on web or mweb , then clicking the new workspace button ‘n’ number of times just creates a single workspace and not ‘n’ number of workspaces
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.59-1 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:
https://user-images.githubusercontent.com/43996225/214705874-d2b1fe24-327f-45d0-ae28-1b9c6d142cdf.mp4
https://user-images.githubusercontent.com/43996225/214707380-d4d32ef9-8634-41b9-ac81-b13861680387.mp4
Expensify/Expensify Issue URL: Issue reported by: @priya-zha Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674643973375769
View all open jobs on GitHub
Upwork Automation - Do Not Edit