Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

Parkreiner
Copy link
Member

@Parkreiner Parkreiner commented Mar 4, 2024

No issue to link – realized we had a bug when I was looking at some of the Slack messages today

Changes made

  • Makes it so that auto workspace creation waits until userParametersQuery succeeds with fetching its data before it's allowed to start making a new workspace
    • To be extra, super cautious, I also made it so auto-creation can only ever be kicked off once, no matter what happens in the useEffect dependency array

Comment on lines +131 to +132
const autoStartReady =
mode === "auto" && (!autofillEnabled || userParametersQuery.isSuccess);
Copy link
Member Author

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:

  1. Mounting render
  2. Value flips from false to true
  3. Value flips from true to false

Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma left a 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.

@Parkreiner
Copy link
Member Author

@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)

@Parkreiner Parkreiner added site Area: frontend dashboard hotfix labels Mar 5, 2024
Copy link

@cdr-bot cdr-bot bot left a 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

@Parkreiner Parkreiner merged commit a92853c into main Mar 5, 2024
@Parkreiner Parkreiner deleted the mes/auto-workspace-fix branch March 5, 2024 23:42
@github-actions github-actions bot locked and limited conversation to collaborators Mar 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
site Area: frontend dashboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants