- Notifications
You must be signed in to change notification settings - Fork928
fix: ensure auto-workspace creation waits until all parameters are ready#12419
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| const autoStartReady = | ||
| mode === "auto" && (!autofillEnabled || userParametersQuery.isSuccess); |
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.
Defining this outside seemed like the more manageable thing to do, since this needs to evaluate twelve combinations:
- The value of mode (form, auto, duplicate)
- The value of autofillEnabled (true or false)
- The value of userParametersQuery.isSuccess (true or false)
Combining them into a single boolean makes sure the effect only has to worry about three situations:
- Mounting render
- Value flips from false to true
- Value flips from true to false
BrunoQuaresma left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 it is a good change but I would add a test for this to ensure things are working properly to prevent the bug you mentioned in the description so we don't accidentally remove this code in the future.
@BrunoQuaresma Yeah, that's perfectly reasonable. I was off today, so I'm going to reclassify this change as a hotfix to make it more clear that it's a small-scale change for now, and get it in faster. And then I'll make a separate PR with the new test assertions (realistically it'll be Sunday before I start working on that) |
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 PR is a hotfix and has been automatically approved.
- ✅ Base is main
- ✅ Has hotfix label
- ✅ Head is from coder/coder
- ✅ Less than 100 lines
Uh oh!
There was an error while loading.Please reload this page.
No issue to link – realized we had a bug when I was looking at some of the Slack messages today
Changes made
userParametersQuerysucceeds with fetching its data before it's allowed to start making a new workspaceuseEffectdependency array