mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix: Login - fix /api/v2/user cache race condition (#47)
While I was testing the Sign-out functionality in #46 , I found a bug on login. Sometimes, intermittently, the login wouldn't 'take' even though the login POST was successful and the subsequent GET to `/api/v2/user` was successful - the user would still be brought to the `/login` screen a second time:  ^ I log-in, and then the login screen just resets, and logging in a second time works 🤔 I tracked this down and found it is a new race condition that I inadvertently introduced when I migrated to SWR in #46. Because SWR caches responses based on the API path - we have to invalidate the cache for `/api/v2/user` when the user logins, otherwise we'll continue to serve the user-not-found error until SWR decides to retry. However, we weren't waiting for that cache-invalidation to go through - so there was the chance that an `/api/v2/user` request was still in-flight while we were redirecting after a successful login. If the request made it back prior to the redirect - login would work on the first try. If the request took longer - SWR would serve the stale, erroneous user from the cache. Luckily the fix is simple - `mutate` in the SWR API returns a promise, so we can just wait for that to go through before redirecting. Unfortunately, though, this is tricky to exercise in a unit test because with the SWR model, the login logic is spread between `_app.tsx`, `UserContext.tsx`, and `SignInForm.tsx`, and the race condition involves an integration of all of those places. I found that there is a lint rule that can help protect us from making this mistake - [`typescript-eslint/no-floating-promises`](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-floating-promises.md) (it's a lint rule we wanted to turn on in `coder/m`, too, actually - just a lot of work to clean up the code for it!), so I turned it on as part of this PR, and it caught a couple extra places to check.
This commit is contained in:
@@ -23,7 +23,9 @@ export const useUser = (redirectOnError = false): UserContext => {
|
||||
const requestError = ctx.error
|
||||
useEffect(() => {
|
||||
if (redirectOnError && requestError) {
|
||||
router.push({
|
||||
// 'void' means we are ignoring handling the promise returned
|
||||
// from router.push (and lets the linter know we're OK with that!)
|
||||
void router.push({
|
||||
pathname: "/login",
|
||||
query: {
|
||||
redirect: router.asPath,
|
||||
|
||||
Reference in New Issue
Block a user