From b448de670b98517e60a8d6b74078e0b8240ad2a4 Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Tue, 28 Jul 2026 13:53:18 +1000 Subject: [PATCH] fix(site/src): refresh provider state after device-flow exchange (#26795) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > 🤖 This PR was modified by Coder Agents on behalf of Jake Howell. Stack: 1. #26575 `fix(site/e2e): close mock external-auth servers in teardown` 2. #26793 `fix(site/e2e): accept 404 from external auth reset hook` 3. #26795 `fix(site/src): refresh provider state after device-flow exchange` ← this PR 4. #26798 `fix(site/e2e): reset both providers in external auth hook` 5. #26648 `chore(site/e2e): re-enable externalAuth suite` #18039 (May 2025) upgraded `@tanstack/react-query` from v4 to v5, which [removed `onSuccess`/`onError`/`onSettled` from `useQuery`](https://tanstack.com/query/v5/docs/react/guides/migrating-to-v5#callbacks-on-usequery-and-queryobserver-have-been-removed). The migration updated the `invalidateQueries` argument shape but left the now-dead `onSuccess` in place on `exchangeExternalAuthDevice`, which is consumed by `useQuery` in `ExternalAuthPage.tsx`. The relevant lines from #18039 in `site/src/api/queries/externalAuth.ts`: ```diff queryKey: ["external-auth", providerId, "device", deviceCode], onSuccess: async () => { // Force a refresh of the Git auth status. - await queryClient.invalidateQueries(["external-auth", providerId]); + await queryClient.invalidateQueries({ + queryKey: ["external-auth", providerId], + }); }, ``` Result: after a successful device-flow exchange the `externalAuthProvider` query is never invalidated, so `externalAuthProviderQuery.data.authenticated` stays `false` and the UI is stuck on "Checking for authentication..." until a manual page refresh. This breaks real users who go through the device flow today, not just the e2e suite that re-enables in #26648. This PR drops the dead `onSuccess` (and the `queryClient` param it depended on), and moves the invalidation into a `useEffect` in `ExternalAuthPage.tsx` that fires when `exchangeExternalAuthDeviceQuery.isSuccess` flips true. Matches the existing pattern in `LoginOAuthDevicePage.tsx`. Verified against the failing CI run on #26648 ([job 83970095566](https://github.com/coder/coder/actions/runs/28346242963/job/83970095566?pr=26648)): the trace shows `POST /api/v2/external-auth/device/device` returning `204` (exchange succeeded) followed by no subsequent `GET /api/v2/external-auth/device` to refresh the provider state. With this PR the `isSuccess` effect runs, the provider query refetches, and the UI flips to the authorized state. ## Regression test Added `site/src/pages/ExternalAuthPage/ExternalAuthPage.test.tsx` (the first test for this page). It renders the device flow with a stateful provider handler that returns `authenticated: false` until the exchange `POST` lands, then asserts the UI flips from the "Authenticate with GitHub" polling screen to "You've authenticated with GitHub!" without a manual refresh, and that the provider endpoint is refetched after the exchange. Confirmed red against the pre-fix code (stuck on the polling screen) and green with the fix. Refs https://linear.app/codercom/issue/DEVEX-413 Refs https://github.com/coder/coder/pull/18039
Why useEffect rather than a query-level callback react-query v5 removed `onSuccess`/`onError`/`onSettled` from `useQuery` because the v4 behaviour was unsound: the callbacks fired per-observer rather than per-query, so they ran twice when two components observed the same query and not at all when a component unmounted and cached data was reused. [TKDodo's "Breaking React Query's API on Purpose"](https://tkdodo.eu/blog/breaking-react-querys-api-on-purpose) and the [official v5 migration guide](https://tanstack.com/query/v5/docs/react/guides/migrating-to-v5#callbacks-on-usequery-and-queryobserver-have-been-removed) both recommend `useEffect` on `isSuccess` as the replacement. Alternatives considered: | Option | Why not | | --- | --- | | Side-effect in `queryFn` | Re-adds the `queryClient` dependency we just removed, and fires on every retry and cached re-read, not just first success. | | Convert to `useMutation` | Wrong semantics. This is a polling query with `retry: isExchangeErrorRetryable` and `retryDelay`; mutations are one-shot and don't have retry-on-pending machinery. | | Global `QueryCache` `onSuccess` | Runs for every query in the app; filtering by queryKey or `meta` for a single per-page side effect is more code than the effect it replaces. | | Custom hook wrapping `useQuery` | Only one consumer, so the abstraction would have one caller. | Local precedent: `LoginOAuthDevicePage.tsx` already uses the same `isSuccess` → `useEffect` pattern for the post-success `location.href` redirect.
Why a separate PR Keeps the bisection signal clean. #26575 fixes the EADDRINUSE flake, #26793 fixes the 404 hook contract drift, this PR fixes the dropped invalidation, and #26648 just flips `.skip`. Each PR addresses one independent root cause that piled up while the externalAuth suite was skipped.
--- site/src/api/queries/externalAuth.ts | 7 -- .../ExternalAuthPage.test.tsx | 81 +++++++++++++++++++ .../ExternalAuthPage/ExternalAuthPage.tsx | 25 +++++- 3 files changed, 103 insertions(+), 10 deletions(-) create mode 100644 site/src/pages/ExternalAuthPage/ExternalAuthPage.test.tsx diff --git a/site/src/api/queries/externalAuth.ts b/site/src/api/queries/externalAuth.ts index b0cd753cda..310df8ac95 100644 --- a/site/src/api/queries/externalAuth.ts +++ b/site/src/api/queries/externalAuth.ts @@ -27,7 +27,6 @@ export const externalAuthDevice = (providerId: string) => { export const exchangeExternalAuthDevice = ( providerId: string, deviceCode: string, - queryClient: QueryClient, ) => { return { queryFn: () => @@ -35,12 +34,6 @@ export const exchangeExternalAuthDevice = ( device_code: deviceCode, }), queryKey: ["external-auth", providerId, "device", deviceCode], - onSuccess: async () => { - // Force a refresh of the Git auth status. - await queryClient.invalidateQueries({ - queryKey: ["external-auth", providerId], - }); - }, }; }; diff --git a/site/src/pages/ExternalAuthPage/ExternalAuthPage.test.tsx b/site/src/pages/ExternalAuthPage/ExternalAuthPage.test.tsx new file mode 100644 index 0000000000..4144e147ea --- /dev/null +++ b/site/src/pages/ExternalAuthPage/ExternalAuthPage.test.tsx @@ -0,0 +1,81 @@ +import { screen, waitFor } from "@testing-library/react"; +import { HttpResponse, http } from "msw"; +import type { ExternalAuth, ExternalAuthDevice } from "#/api/typesGenerated"; +import { renderWithAuth } from "#/testHelpers/renderHelpers"; +import { server } from "#/testHelpers/server"; +import ExternalAuthPage from "./ExternalAuthPage"; + +const provider = "github"; + +const deviceResponse: ExternalAuthDevice = { + device_code: "device-code", + user_code: "1234-5678", + verification_uri: "https://github.com/login/device", + expires_in: 900, + interval: 0, +}; + +const baseProvider: ExternalAuth = { + authenticated: false, + device: true, + display_name: "GitHub", + supports_revocation: false, + user: null, + app_installable: false, + installations: [], + app_install_url: "", +}; + +const renderPage = () => + renderWithAuth(, { + route: `/external-auth/${provider}`, + path: "/external-auth/:provider", + }); + +describe("ExternalAuthPage", () => { + // Regression test for the device flow: after a successful device-code + // exchange the provider query must be invalidated so the page refetches and + // flips to the authenticated view. Prior to the fix the dropped react-query + // `onSuccess` left the UI stuck on the "Checking for authentication..." + // polling screen until a manual refresh. + it("refreshes the provider state after a successful device exchange", async () => { + let exchanged = false; + let providerRequests = 0; + + server.use( + http.get(`/api/v2/external-auth/${provider}`, () => { + providerRequests++; + return HttpResponse.json({ + ...baseProvider, + // The exchange marks the account authenticated. The page only + // observes this after it invalidates and refetches the query. + authenticated: exchanged, + }); + }), + http.get(`/api/v2/external-auth/${provider}/device`, () => + HttpResponse.json(deviceResponse), + ), + http.post(`/api/v2/external-auth/${provider}/device`, () => { + exchanged = true; + return new HttpResponse(null, { status: 204 }); + }), + ); + + renderPage(); + + // The polling screen renders first while the exchange is pending. + await screen.findByText("Authenticate with GitHub"); + + // Once the exchange succeeds, the provider query is invalidated and + // refetched, so the authenticated view appears without a manual refresh. + await waitFor(() => { + expect( + screen.getByText("You've authenticated with GitHub!"), + ).toBeInTheDocument(); + }); + + // The provider endpoint is hit more than once: the initial load plus the + // refetch triggered by the post-exchange invalidation. + expect(providerRequests).toBeGreaterThan(1); + }); +}); diff --git a/site/src/pages/ExternalAuthPage/ExternalAuthPage.tsx b/site/src/pages/ExternalAuthPage/ExternalAuthPage.tsx index f66a09c2da..2873063abe 100644 --- a/site/src/pages/ExternalAuthPage/ExternalAuthPage.tsx +++ b/site/src/pages/ExternalAuthPage/ExternalAuthPage.tsx @@ -1,6 +1,6 @@ import { isAxiosError } from "axios"; import type { FC } from "react"; -import { useMemo } from "react"; +import { useEffect, useMemo } from "react"; import { useQuery, useQueryClient } from "react-query"; import { useParams, useSearchParams } from "react-router"; import type { ApiErrorResponse } from "#/api/errors"; @@ -24,7 +24,10 @@ const ExternalAuthPage: FC = () => { const [searchParams] = useSearchParams(); const { permissions } = useAuthenticated(); const queryClient = useQueryClient(); - const externalAuthProviderOpts = externalAuthProvider(provider); + const externalAuthProviderOpts = useMemo( + () => externalAuthProvider(provider), + [provider], + ); const externalAuthProviderQuery = useQuery({ ...externalAuthProviderOpts, refetchOnWindowFocus: true, @@ -45,7 +48,6 @@ const ExternalAuthPage: FC = () => { ...exchangeExternalAuthDevice( provider, externalAuthDeviceQuery.data?.device_code ?? "", - queryClient, ), enabled: Boolean(externalAuthDeviceQuery.data), retry: isExchangeErrorRetryable, @@ -55,6 +57,23 @@ const ExternalAuthPage: FC = () => { refetchOnWindowFocus: false, }); + // Flip the UI out of polling once the exchange succeeds. Replaces the + // `onSuccess` that react-query v5 dropped from `useQuery`. `exact` avoids + // re-POSTing the one-time device code. + useEffect(() => { + if (!exchangeExternalAuthDeviceQuery.isSuccess) { + return; + } + queryClient.invalidateQueries({ + queryKey: externalAuthProviderOpts.queryKey, + exact: true, + }); + }, [ + exchangeExternalAuthDeviceQuery.isSuccess, + externalAuthProviderOpts.queryKey, + queryClient, + ]); + if (externalAuthProviderQuery.isLoading || !externalAuthProviderQuery.data) { return null; }