From 074efeec57374c7e580dc208ea5921f99cab8018 Mon Sep 17 00:00:00 2001 From: Naiyuan Qing <145280634+NevilleQingNY@users.noreply.github.com> Date: Tue, 7 Jul 2026 14:28:42 +0800 Subject: [PATCH] fix(web): stop login arrival effect from racing fresh form logins (#5009) (#5019) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The /login page's already-authenticated effect fired on any user change, including the one verifyCode writes mid form-login while handleVerify is still fetching the workspace list. It then read the cold list cache (getQueryData ?? []) and raced handleSuccess with a replace to /workspaces/new — users with existing workspaces could land on the create-workspace page after login. Two changes, both in the arrival effect: - Ownership: latch once auth settles as logged-out on this page; any user appearing afterwards came from the login form, whose handleSuccess owns post-login navigation. The effect now only serves visitors who arrived authenticated. The desktop-handoff branch stays above the latch so form logins with platform=desktop still mint the deep-link token. - Correct data: for genuine arrived-authenticated visitors, fetch the list via ensureQueryData instead of reading a possibly-cold cache, which misrouted workspace owners to /workspaces/new on fresh page loads. Regression tests verified red against the previous implementation. Fixes #5009 Co-authored-by: Claude Fable 5 --- apps/web/app/(auth)/login/page.test.tsx | 81 +++++++++++++++++++++++-- apps/web/app/(auth)/login/page.tsx | 45 +++++++++++--- 2 files changed, 111 insertions(+), 15 deletions(-) diff --git a/apps/web/app/(auth)/login/page.test.tsx b/apps/web/app/(auth)/login/page.test.tsx index a033e984c1..cb1f3d73ed 100644 --- a/apps/web/app/(auth)/login/page.test.tsx +++ b/apps/web/app/(auth)/login/page.test.tsx @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import { render, screen, waitFor } from "@testing-library/react"; +import { act, render, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { I18nProvider } from "@multica/core/i18n/react"; @@ -25,26 +25,35 @@ const { mockSendCode, mockVerifyCode, mockIssueCliToken, + mockListWorkspaces, + mockListMyInvitations, + mockPush, + mockReplace, searchParamsState, authStateRef, } = vi.hoisted(() => ({ mockSendCode: vi.fn(), mockVerifyCode: vi.fn(), mockIssueCliToken: vi.fn(), + mockListWorkspaces: vi.fn(), + mockListMyInvitations: vi.fn(), + mockPush: vi.fn(), + mockReplace: vi.fn(), searchParamsState: { params: new URLSearchParams() }, authStateRef: { state: { sendCode: vi.fn(), verifyCode: vi.fn(), - user: null as null | { id: string; email: string }, + user: null as null | { id: string; email: string; onboarded_at?: string | null }, isLoading: false, }, }, })); -// Mock next/navigation +// Mock next/navigation — router spies are hoisted so tests can assert +// which navigation (if any) the page issued. vi.mock("next/navigation", () => ({ - useRouter: () => ({ push: vi.fn(), replace: vi.fn() }), + useRouter: () => ({ push: mockPush, replace: mockReplace }), usePathname: () => "/login", useSearchParams: () => searchParamsState.params, })); @@ -76,7 +85,8 @@ vi.mock("@/features/auth/auth-cookie", () => ({ // Mock api vi.mock("@multica/core/api", () => ({ api: { - listWorkspaces: vi.fn().mockResolvedValue([]), + listWorkspaces: mockListWorkspaces, + listMyInvitations: mockListMyInvitations, verifyCode: vi.fn(), setToken: vi.fn(), getMe: vi.fn(), @@ -92,6 +102,8 @@ describe("LoginPage", () => { searchParamsState.params = new URLSearchParams(); authStateRef.state.user = null; authStateRef.state.isLoading = false; + mockListWorkspaces.mockResolvedValue([]); + mockListMyInvitations.mockResolvedValue([]); }); it("renders login form with email input and continue button", () => { @@ -204,4 +216,63 @@ describe("LoginPage", () => { }); } }); + + // Regression: #5009 — the "already authenticated on arrival" effect used to + // fire for fresh form logins too. verifyCode writes `user` while handleVerify + // is still fetching the workspace list, so the effect read an empty cache and + // raced handleSuccess with replace("/workspaces/new"); depending on the + // interleaving the user could end up stuck on the create-workspace page + // despite having workspaces. + describe("post-login redirect ownership (#5009)", () => { + const onboardedUser = { + id: "u1", + email: "test@multica.ai", + onboarded_at: "2026-01-01T00:00:00Z", + }; + + it("does not redirect from the arrival effect when the user logs in via the form", async () => { + // Auth settles as logged-out first — the page latches "any user from + // now on came from the form". + const wrapper = createWrapper(); + const { rerender } = render(, { wrapper }); + // verifyCode set the user; the workspace list fetch is still in flight + // (cache cold). The arrival effect must stay silent — handleSuccess + // owns this navigation. + authStateRef.state.user = onboardedUser; + rerender(); + + await act(async () => {}); + expect(mockReplace).not.toHaveBeenCalled(); + expect(mockPush).not.toHaveBeenCalled(); + expect(mockListWorkspaces).not.toHaveBeenCalled(); + }); + + it("fetches the workspace list before redirecting a visitor who arrived authenticated", async () => { + // Cold Query cache on a fresh page load: reading it would say "no + // workspaces" and misroute to /workspaces/new. The effect must fetch. + authStateRef.state.user = onboardedUser; + mockListWorkspaces.mockResolvedValue([{ id: "ws-1", slug: "acme" }]); + + render(, { wrapper: createWrapper() }); + + await waitFor(() => { + expect(mockReplace).toHaveBeenCalledWith("/acme/issues"); + }); + expect(mockListWorkspaces).toHaveBeenCalledTimes(1); + }); + + it("still honors ?next= for a visitor who arrived authenticated", async () => { + searchParamsState.params = new URLSearchParams({ + next: "/invite/abc", + }); + authStateRef.state.user = onboardedUser; + + render(, { wrapper: createWrapper() }); + + await waitFor(() => { + expect(mockReplace).toHaveBeenCalledWith("/invite/abc"); + }); + expect(mockListWorkspaces).not.toHaveBeenCalled(); + }); + }); }); diff --git a/apps/web/app/(auth)/login/page.tsx b/apps/web/app/(auth)/login/page.tsx index 6fb37033e5..ec949537bb 100644 --- a/apps/web/app/(auth)/login/page.tsx +++ b/apps/web/app/(auth)/login/page.tsx @@ -1,11 +1,14 @@ "use client"; -import { Suspense, useEffect, useState } from "react"; +import { Suspense, useEffect, useRef, useState } from "react"; import { useSearchParams, useRouter } from "next/navigation"; import { useQueryClient, type QueryClient } from "@tanstack/react-query"; import { sanitizeNextUrl, useAuthStore } from "@multica/core/auth"; import { useConfigStore } from "@multica/core/config"; -import { workspaceKeys } from "@multica/core/workspace/queries"; +import { + workspaceKeys, + workspaceListOptions, +} from "@multica/core/workspace/queries"; import { paths, resolvePostAuthDestination, @@ -77,11 +80,21 @@ function LoginPageContent() { const [desktopError, setDesktopError] = useState(""); const hasOnboarded = useHasOnboarded(); - // Already authenticated — honor ?next= or fall back to first workspace - // (or /onboarding if the user has none). Skip this entire path when - // the user arrived to authorize the CLI. + // Latched once auth has been observed settled as logged-out on this page. + // Any `user` that appears afterwards came from the login form in this + // session — not from an existing session found on arrival. + const settledLoggedOutRef = useRef(false); + + // Already authenticated ON ARRIVAL — honor ?next= or fall back to first + // workspace (or /onboarding if the user has none). Skip this entire path + // when the user arrived to authorize the CLI. useEffect(() => { - if (isLoading || !user || cliCallbackRaw) return; + if (isLoading) return; + if (!user) { + settledLoggedOutRef.current = true; + return; + } + if (cliCallbackRaw) return; if (isDesktopHandoff) { // Desktop opened the browser for login but the web session is already // authenticated — mint a bearer token from the cookie session and hand @@ -101,14 +114,26 @@ function LoginPageContent() { }); return; } + // Fresh form login (issue #5009): `user` was written by verifyCode while + // handleVerify was still fetching the workspace list, so this effect used + // to read the not-yet-seeded list cache and race handleSuccess with a + // replace to /workspaces/new. handleSuccess owns post-login navigation; + // this effect only serves visitors who arrived already authenticated. + if (settledLoggedOutRef.current) return; if (nextUrl) { router.replace(nextUrl); return; } - const list = qc.getQueryData(workspaceKeys.list()) ?? []; - void resolveLoggedInDestination(qc, hasOnboarded, list).then((dest) => - router.replace(dest), - ); + // Fetch instead of reading the cache: on a fresh page load the cache is + // cold, and `getQueryData() ?? []` would misroute a user who does have + // workspaces to /workspaces/new. On fetch failure fall back to [] — + // same destination the cold-cache read produced, rather than trapping + // the user on the login page. + void qc + .ensureQueryData(workspaceListOptions()) + .catch(() => [] as Workspace[]) + .then((list) => resolveLoggedInDestination(qc, hasOnboarded, list)) + .then((dest) => router.replace(dest)); }, [isLoading, user, router, nextUrl, cliCallbackRaw, isDesktopHandoff, hasOnboarded, qc]); const handleSuccess = async () => {