From 26c74c96a28ced7cfa8d095e321aa956b710c58d Mon Sep 17 00:00:00 2001 From: fra-shipper Date: Sun, 30 Aug 2026 18:06:29 -0700 Subject: [PATCH 1/2] fix(auth): read login error param synchronously to close a render race Login read the ?error= query param via a useEffect, so the first render always had error=''. That flowed into LoginForm's own useState(error) initial value, and LoginForm only caught up once both components' effects had flushed -- a two-hop async chain before the [role=alert] error message ever appears in the DOM. Initialize the state from the query param directly (lazy useState initializer) so the first render already carries it, removing one of the two async hops. The existing effect is kept to re-sync error if the query changes after mount. Added Login.spec.tsx: mounts with createRoot + flushSync, bypassing Testing Library's act()-wrapped render() (which flushes effects synchronously and would hide the bug), and asserts the [role="alert"] node is present on the very first commit. Confirmed it fails on the pre-fix code and passes with this change. This is the same class of bug as the thread_resume race fixed in #3021 (a stale-DOM assertion racing an async render). It is a plausible but unconfirmed contributor to #3023: a single windows-latest CI occurrence where the oauth_auth spec's 'shows a specific message for oauthSignin error' test hit the 30s Cypress command timeout on attempt 1 and passed on retry, filed needs-triage with the root cause still unconfirmed. The render gap this fix closes resolves within a single synchronous commit, orders of magnitude smaller than a 30s command timeout, so this is not a confirmed diagnosis of #3023 -- it is a genuine render-timing bug worth fixing on its own merits, in the same code path #3023's test exercises. --- frontend/src/pages/Login.tsx | 7 ++- frontend/tests/Login.spec.tsx | 98 +++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 frontend/tests/Login.spec.tsx diff --git a/frontend/src/pages/Login.tsx b/frontend/src/pages/Login.tsx index 9bfae649f2..701eb4dc0b 100644 --- a/frontend/src/pages/Login.tsx +++ b/frontend/src/pages/Login.tsx @@ -16,7 +16,12 @@ export const LoginError = new Error( export default function Login() { const query = useQuery(); const { data: config, user, setUserFromAPI } = useAuth(); - const [error, setError] = useState(''); + // Read the error param synchronously on mount instead of via effect, so the + // first render already carries it. Otherwise `error` starts empty, flows + // into `LoginForm`'s own `useState(error)` initial value, and only catches + // up once both components' effects have flushed -- a two-hop async chain + // that a slow test runner can lose a race against (chainlit#3023). + const [error, setError] = useState(() => query.get('error') || ''); const apiClient = useContext(ChainlitContext); const navigate = useNavigate(); const { variant } = useTheme(); diff --git a/frontend/tests/Login.spec.tsx b/frontend/tests/Login.spec.tsx new file mode 100644 index 0000000000..94c3669e1d --- /dev/null +++ b/frontend/tests/Login.spec.tsx @@ -0,0 +1,98 @@ +import { i18nSetupLocalization } from '@/i18n'; +import { flushSync } from 'react-dom'; +import { createRoot } from 'react-dom/client'; +import { MemoryRouter } from 'react-router-dom'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import Login from '@/pages/Login'; + +// Real i18next init (no backend plugin, so it resolves synchronously) -- +// LoginForm's calls i18n.exists() at render time and throws +// if no instance was ever initialized. +i18nSetupLocalization(); + +vi.mock('client-types/*', async () => { + const { createContext } = await import('react'); + const mockApiClient = { + buildEndpoint: (path: string) => `http://localhost:8000${path}`, + getOAuthEndpoint: (provider: string) => + `http://localhost:8000/auth/oauth/${provider}` + }; + return { + ChainlitContext: createContext(mockApiClient), + useAuth: () => ({ + data: { requireLogin: true, oauthProviders: [] }, + user: null, + setUserFromAPI: vi.fn() + }) + }; +}); + +// Logo pulls useConfig() from @chainlit/react-client, which needs a +// RecoilRoot ancestor. It is unrelated to the error-render race under test, +// so stub it out rather than wiring up Recoil for this test. +vi.mock('@/components/Logo', () => ({ + Logo: () => null +})); + +// jsdom does not implement matchMedia, and Login -> useTheme() calls it +// synchronously during render (not from an effect), so stub it the same way +// FavoriteButton.spec.tsx stubs ResizeObserver. +window.matchMedia = + window.matchMedia || + vi.fn().mockImplementation((query: string) => ({ + matches: false, + media: query, + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + dispatchEvent: vi.fn() + })); + +describe('Login', () => { + let container: HTMLDivElement | null = null; + + afterEach(() => { + if (container) { + document.body.removeChild(container); + container = null; + } + }); + + // Regression test for the render race fixed by reading the ?error= query + // param via a lazy useState initializer instead of an effect. Uses + // createRoot + flushSync directly instead of Testing Library's render(), + // because render() wraps in act(), which flushes passive effects + // synchronously and hides the gap this test is pinning: what is in the DOM + // on the very first commit, before any useEffect has run. + it('renders the [role="alert"] error message on the first synchronous commit', () => { + container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + + flushSync(() => { + root.render( + + + + ); + }); + + expect(container.querySelector('[role="alert"]')).not.toBeNull(); + }); + + it('renders no [role="alert"] on the first synchronous commit when there is no error param', () => { + container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + + flushSync(() => { + root.render( + + + + ); + }); + + expect(container.querySelector('[role="alert"]')).toBeNull(); + }); +}); From 10428c5732e7936a217d4f0fccebc0c371248e7a Mon Sep 17 00:00:00 2001 From: fra-shipper Date: Wed, 2 Sep 2026 17:16:46 -0700 Subject: [PATCH 2/2] test(auth): unmount manual createRoot roots in Login.spec.tsx Both createRoot-based tests left their React root mounted after the test; the shared cleanup() in tests/setup-tests.ts only handles trees rendered via Testing Library's render(), so the manual roots and Login's pending passive effect leaked across tests. Track the root and unmount it in afterEach, before removing the container. --- frontend/tests/Login.spec.tsx | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/frontend/tests/Login.spec.tsx b/frontend/tests/Login.spec.tsx index 94c3669e1d..b21b891530 100644 --- a/frontend/tests/Login.spec.tsx +++ b/frontend/tests/Login.spec.tsx @@ -50,8 +50,13 @@ window.matchMedia = describe('Login', () => { let container: HTMLDivElement | null = null; + let root: ReturnType | null = null; afterEach(() => { + if (root) { + root.unmount(); + root = null; + } if (container) { document.body.removeChild(container); container = null; @@ -67,7 +72,7 @@ describe('Login', () => { it('renders the [role="alert"] error message on the first synchronous commit', () => { container = document.createElement('div'); document.body.appendChild(container); - const root = createRoot(container); + root = createRoot(container); flushSync(() => { root.render( @@ -83,7 +88,7 @@ describe('Login', () => { it('renders no [role="alert"] on the first synchronous commit when there is no error param', () => { container = document.createElement('div'); document.body.appendChild(container); - const root = createRoot(container); + root = createRoot(container); flushSync(() => { root.render(