mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix(site/src): refresh provider state after device-flow exchange (#26795)
> 🤖 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 <details> <summary>Why <code>useEffect</code> rather than a query-level callback</summary> 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. </details> <details> <summary>Why a separate PR</summary> 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. </details>
This commit is contained in:
@@ -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],
|
||||
});
|
||||
},
|
||||
};
|
||||
};
|
||||
|
||||
|
||||
@@ -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(<ExternalAuthPage />, {
|
||||
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<ExternalAuth>({
|
||||
...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);
|
||||
});
|
||||
});
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user