-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(project-creation): Button should be busy until all mutations are complete #92183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(project-creation): Button should be busy until all mutations are complete #92183
Conversation
69e7bb6 to
1d33844
Compare
1d33844 to
ccd66cb
Compare
| disabled={!(canUserCreateProject && formErrorCount === 0)} | ||
| busy={createProjectAndRules.isPending} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make a distinction between busy and disabled here
|
|
||
| const handleProjectCreation = useCallback( | ||
| async (data: FormData) => { | ||
| setErrors(undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only part slightly outside the scope of this PR, but I think it's reasonable to include it. We should clear the errors when the submit button is clicked again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m wondering why we keep track of the error state manually? useMutation handles errors and returns them as part of the return value:
const { error } = useMutation(...)
and it will get re-set automatically if you fire the mutation again. So I think we should be able to get rid of that useState completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TkDodo yes! great idea. now that we have all mutations in useCreateProjectAndRules we can get rid of setErrors. will update the code
| const notificationRule = await createNotificationAction({ | ||
| shouldCreateRule: alertRuleConfig?.shouldCreateRule, | ||
| name: project.name, | ||
| projectSlug: project.slug, | ||
| conditions: alertRuleConfig?.conditions, | ||
| actionMatch: alertRuleConfig?.actionMatch, | ||
| frequency: alertRuleConfig?.frequency, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this mutation isn’t dependent on createProjectRules, but it still fires after that one has finished, and it won’t run at all if that one error’d.
We could probably fire createProjectRules.mutateAsync and createNotificationAction in parallel, and then await both promises with Promise.all at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the creation of a rule requires a project slug and name, that is why the project's mutation has to run first. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that makes sense, but I’m talking about this code block:
if (alertRuleConfig?.shouldCreateCustomRule) {
const ruleData = await createProjectRules.mutateAsync({
projectSlug: project.slug,
name: project.name,
conditions: alertRuleConfig?.conditions,
actions: alertRuleConfig?.actions,
actionMatch: alertRuleConfig?.actionMatch,
frequency: alertRuleConfig?.frequency,
});
ruleIds.push(ruleData.id);
}
if shouldCreateCustomRule is true, we call await createProjectRules.mutateAsync, and then await createNotificationAction, so they run in serial. but since createNotificationAction does not depend on any result from createProjectRules.mutateAsync, they could run in parallel. Something like:
const projectRulesPromise = alertRuleConfig?.shouldCreateCustomRule ? createProjectRuels.mutateAsync(...) : undefined
const notificationRulePromise = createNotificationAction(...)
const rules = await Promise.all([
projectRulesPromise,
notificationRulePromise,
]);
then, you can get ruleIds by mapping over rules
|
|
||
| const handleProjectCreation = useCallback( | ||
| async (data: FormData) => { | ||
| setErrors(undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m wondering why we keep track of the error state manually? useMutation handles errors and returns them as part of the return value:
const { error } = useMutation(...)
and it will get re-set automatically if you fire the mutation again. So I think we should be able to get rid of that useState completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome work 🙌
…e-create-project-rules-hook
…ect-rules-hook' of github.com:getsentry/sentry into priscila/ref/project-creation/introduce-use-create-project-rules-hook
…ect-rules-hook' into priscila/fix/project-creation/disable-the-button-until-all-mutations-are-complete
This hook was introduced to follow our standard use of react-query and will be used in this [PR](#92183).
This hook was introduced to follow our standard use of react-query and will be used in this [PR](#92183).
…button-until-all-mutations-are-complete
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #92183 +/- ##
==========================================
- Coverage 87.89% 86.80% -1.10%
==========================================
Files 10253 10170 -83
Lines 587976 582124 -5852
Branches 22831 22526 -305
==========================================
- Hits 516818 505304 -11514
- Misses 70712 76373 +5661
- Partials 446 447 +1 |
…complete (#92183) Currently, the 'Create Project' and 'Configure SDK' buttons are disabled/busy only while the project creation process is ongoing. However, in addition to the project creation, there are two other asynchronous requests that may be triggered and could potentially fail. Because these additional requests are not accounted for, the buttons may become enabled prematurely and users could again fire other requests. Ideally, the buttons should remain disabled and indicate a busy state until all related mutations have fully completed closes TET-532
Currently, the 'Create Project' and 'Configure SDK' buttons are disabled/busy only while the project creation process is ongoing. However, in addition to the project creation, there are two other asynchronous requests that may be triggered and could potentially fail. Because these additional requests are not accounted for, the buttons may become enabled prematurely and users could again fire other requests. Ideally, the buttons should remain disabled and indicate a busy state until all related mutations have fully completed - successfully or not - before allowing further user interaction. This PR updates the code fixing it.
Before
Screen.Recording.2025-05-23.at.09.36.42.mov
After
Screen.Recording.2025-05-23.at.09.28.52.mov
closes TET-532