mirror of
https://github.com/coder/coder.git
synced 2026-09-21 12:44:32 +08:00
fix(site): deflake adjust user theme preference (#28219)
## Summary Deflakes the `adjust user theme preference` Playwright test (`site/e2e/tests/users/userSettings.spec.ts`), tracked in DEVEX-415. The test selected the Light theme and then hard-navigated with `page.goto` before the optimistic appearance update was persisted. The navigation could cancel the in-flight `PUT /api/v2/users/me/appearance`, so the reloaded document embedded the stale `dark` preference and the final assertion flaked. `toPass` retries could not help because retrying the reload only re-reads the still-stale persisted state. ## Fix Wait for the appearance form's save spinner to clear before the hard reload, mirroring how other settings tests wait for a visible save confirmation. Asserting the optimistic light class first guarantees the spinner is already showing if a save started; a repeat run that is already light never shows it, so the test is idempotent and there are no direct API calls. To give the test a UI signal, `Spinner` gets an opt-in `label` prop that exposes it as a `role="status"` live region with an `aria-label` (decorative otherwise). `Loader` moves its label onto its own status container so it keeps a single status region. ## Changes - `site/src/components/Spinner/Spinner.tsx`: opt-in `label` prop. - `site/src/components/Loader/Loader.tsx`: label on its own status region. - `site/src/pages/UserSettingsPage/AppearancePage/AppearanceForm.tsx`: label both appearance save spinners. - `site/e2e/tests/users/userSettings.spec.ts`: wait for the save spinner. ## Validation - `biome check` and `tsc -p .` pass. - Loader + AppearancePage unit tests and the affected storybook tests pass. - The e2e test passed 20/20 under `pnpm playwright:test -g "adjust user theme preference" --repeat-each 20`. <details> <summary>Implementation plan & decision log</summary> # DEVEX-415: Fix flake in "adjust user theme preference" e2e test ## Problem Playwright test `site/e2e/tests/users/userSettings.spec.ts` → `adjust user theme preference` flakes. After selecting the Light theme and hard-navigating to `/`, the reloaded page sometimes stays `dark`, failing the final assertion. CI Flake Bot has recorded repeated recurrences on `main` (latest 2026-08-17, runs `32004239187`, `31745261984`) even after PR #25183 added `toPass` retries. ## Root cause (confirmed by reading the code) The appearance update is optimistic and its persistence is not awaited before navigation: - `updateAppearanceSettings` (`site/src/api/queries/users.ts`) has an `onMutate` that optimistically writes the new theme into the React Query cache. The `<html>` class flips to `light` immediately, before the `PUT /api/v2/users/me/appearance` completes. - `useQueuedAppearanceSubmit` (`site/src/pages/UserSettingsPage/AppearancePage/AppearancePage.tsx`) serializes submits: if a request is in flight, the next is queued and only fires after the first settles. - A fresh `member` user has **empty** appearance settings. `migrateLegacyPreference` (`site/src/theme/themeMode.ts`) maps empty settings to `{ mode: "single", theme: "dark" }` (`DEFAULT_THEME = "dark"`). So the "Theme mode" dropdown already starts on **Single theme** and `<html>` starts `dark`. Selecting "Single theme" in the test is therefore a **no-op** (`onChangeMode` early-returns when `mode === draft.mode`) and fires **no** PUT. The only appearance PUT in the test is the one from clicking "Light default" (`onSelectSingle("light")` → `theme_preference: "light"`). - The test's first `expectLightThemeClasses(page)` passes purely from the optimistic cache. It then calls `page.goto("/")` almost immediately (~20 ms after the click). If PUT #2 (the light one) has not persisted server-side, the new document loads the still-persisted `dark` preference from embedded metadata, and every retry of the post-navigation assertion sees `dark` for the full 10 s window. `toPass` cannot help post-navigation because it only re-reads stale, already persisted state; it cannot make an unfinished/queued PUT complete. ## Implemented fix: wait for the saving spinner (UI signal) before navigation > Iteration history: (1) An earlier attempt waited on the appearance `PUT` via > a `waitForApiCall` helper, but that couples the test to a state transition > and is non-idempotent (a repeat run that is already light fires no PUT, so > the wait times out). CI retries reuse the same ephemeral server, so this is a > real hazard. (2) A second attempt confirmed persistence with > `page.request.get(...)`, but that calls the API directly and stops being a > site test. Both were rejected. The repo's non-flaky settings tests wait for a **visible save confirmation** (e.g. "settings updated successfully" toasts) before trusting the result. The appearance theme form has no toast; its only save-in-progress feedback is the `<Spinner>`. So the fix mirrors that pattern using the spinner: 1. Make the theme section's spinner identifiable via a new opt-in `label` prop on `Spinner` (sets `role="status"` + `aria-label` only when provided; decorative otherwise). `Loader` moves its label onto its own `status` container so it keeps a single status region and its `getByLabelText` queries keep working. 2. In the test, after clicking "Light default", assert the optimistic light class, then wait for that spinner to be hidden before `page.goto("/")`. Why this is correct and idempotent: - React Query sets `isPending` before `onMutate` applies the optimistic cache update, so by the time the optimistic light class is visible the spinner is already showing if a save started. Waiting for it to clear guarantees the PUT settled (and was not canceled by navigation) before the reload. - A repeat run that is already light: clicking "Light default" is a no-op radio change, no PUT fires, the spinner never shows, and `toBeHidden` passes immediately. The reload still shows light. - No direct API calls: the test only observes site UI. ### Implemented changes `Spinner` gains an opt-in `label` prop (`site/src/components/Spinner/Spinner.tsx`): ```tsx role={label ? "status" : undefined} aria-label={label} ``` `Loader` carries the label on its own status container (`site/src/components/Loader/Loader.tsx`), and the appearance save spinners use the new prop (`AppearanceForm.tsx`): ```tsx <Spinner loading={isUpdating} size="sm" label="Saving theme preference" /> <Spinner loading={isUpdating} size="sm" label="Saving terminal font" /> ``` `site/e2e/tests/users/userSettings.spec.ts`: ```ts await expect( page.getByRole("combobox", { name: /theme mode/i }), ).toContainText("Single theme"); // precondition: single mode const singleThemeGroup = page.getByRole("group", { name: "Theme" }); await expect(singleThemeGroup).toBeVisible(); await singleThemeGroup.getByText("Light default", { exact: true }).click(); await expectLightThemeClasses(page); // optimistic DOM => spinner showing if saving await expect( page.getByRole("status", { name: "Saving theme preference" }), ).toBeHidden(); // save settled before the hard reload await page.goto("/", { waitUntil: "domcontentloaded" }); await expectLightThemeClasses(page); ``` Validation: `biome check`, `tsc -p .` pass; Loader + AppearancePage unit tests and the affected storybook tests pass; the e2e test passed 20/20 under `pnpm playwright:test -g "adjust user theme preference" --repeat-each 20` (idempotent). ## Alternatives considered - **Wait on the appearance `PUT` via a `waitForApiCall` helper**: rejected. It couples the test to a state transition and is non-idempotent, a repeat run that is already light fires no PUT so the wait times out (14/15 repeats failed). CI retries reuse the same ephemeral server, so this is a real hazard, not just a local-repeat artifact. - **Confirm persistence with `page.request.get(...)`**: rejected. It calls the API directly and stops being a site test. - **Default `role="status"` on the shared `Spinner`**: rejected. `Loader` wraps `Spinner` in its own `status` div, so a default would nest two status regions and break `Loader.test.tsx`. The opt-in `label` prop avoids this. - **Add a networkidle wait or sleep before navigation**: rejected. Violates the repo guidance against `time.Sleep`-style timing hacks and is inherently racy. - **Fix product behavior instead of the test**: out of scope. Related bug DEVEX-94 ("Light Theme setting not respected until Appearance page opened") tracks product-side persistence/embedding behavior; this task is scoped to stabilizing the e2e test. ## Files touched - `site/src/components/Spinner/Spinner.tsx` (new opt-in `label` prop). - `site/src/components/Loader/Loader.tsx` (label on its own status region). - `site/src/pages/UserSettingsPage/AppearancePage/AppearanceForm.tsx` (use the `label` prop on both appearance save spinners). - `site/e2e/tests/users/userSettings.spec.ts` (wait for the spinner). </details> --- This PR was created by Coder Agents on behalf of @jeremyruppel. --------- Co-authored-by: Samuel Volin <sam.volin@coder.com>
This commit is contained in:
Jeremy Ruppel
co-authored by
Samuel Volin
Samuel Volin
parent
039c0da5ae
commit
46ec620767
@@ -32,8 +32,12 @@ test("adjust user theme preference", async ({ page }) => {
|
||||
|
||||
await page.goto("/settings/appearance", { waitUntil: "domcontentloaded" });
|
||||
|
||||
await page.getByRole("combobox", { name: /theme mode/i }).click();
|
||||
await page.getByRole("option", { name: /single theme/i }).click();
|
||||
// Precondition: the theme mode must start on "Single theme" so that picking
|
||||
// a single theme below takes effect immediately. A fresh member defaults to
|
||||
// single mode; assert it so the test fails loudly if that default changes.
|
||||
await expect(
|
||||
page.getByRole("combobox", { name: /theme mode/i }),
|
||||
).toContainText("Single theme");
|
||||
|
||||
const singleThemeGroup = page.getByRole("group", { name: "Theme" });
|
||||
await expect(singleThemeGroup).toBeVisible();
|
||||
@@ -41,6 +45,16 @@ test("adjust user theme preference", async ({ page }) => {
|
||||
|
||||
await expectLightThemeClasses(page);
|
||||
|
||||
// The theme is saved optimistically, so the DOM turns light before the
|
||||
// preference is persisted. The form shows a spinner while the save is in
|
||||
// flight; wait for it to clear so the save has completed (and was not
|
||||
// canceled by navigation) before the hard reload. Asserting the optimistic
|
||||
// class first guarantees the spinner is already showing if a save started,
|
||||
// and a repeat run that is already light simply never shows it.
|
||||
await expect(
|
||||
page.getByRole("status", { name: "Saving theme preference" }),
|
||||
).toBeHidden();
|
||||
|
||||
await page.goto("/", { waitUntil: "domcontentloaded" });
|
||||
|
||||
// Make sure the page is still using the light theme after reloading and
|
||||
|
||||
@@ -25,6 +25,7 @@ export const Loader: FC<LoaderProps> = ({
|
||||
{...attrs}
|
||||
role="status"
|
||||
aria-live="polite"
|
||||
aria-label={resolvedLabel}
|
||||
data-testid="loader"
|
||||
className={cn(
|
||||
"flex items-center justify-center",
|
||||
@@ -32,7 +33,7 @@ export const Loader: FC<LoaderProps> = ({
|
||||
className,
|
||||
)}
|
||||
>
|
||||
<Spinner aria-label={resolvedLabel} size={size} loading />
|
||||
<Spinner size={size} loading />
|
||||
</div>
|
||||
);
|
||||
};
|
||||
|
||||
@@ -27,6 +27,11 @@ type SpinnerProps = React.SVGProps<SVGSVGElement> &
|
||||
VariantProps<typeof spinnerVariants> & {
|
||||
children?: ReactNode;
|
||||
loading?: boolean;
|
||||
/**
|
||||
* Exposes the spinner as an accessible live region labelled with this text. Leave undefined for
|
||||
* decorative spinners, e.g. inside a component that already provides its own status region.
|
||||
*/
|
||||
label?: string;
|
||||
};
|
||||
|
||||
export function Spinner({
|
||||
@@ -34,6 +39,7 @@ export function Spinner({
|
||||
size,
|
||||
loading,
|
||||
children,
|
||||
label,
|
||||
...props
|
||||
}: SpinnerProps) {
|
||||
if (!loading) {
|
||||
@@ -45,6 +51,8 @@ export function Spinner({
|
||||
viewBox="0 0 24 24"
|
||||
xmlns="http://www.w3.org/2000/svg"
|
||||
fill="currentColor"
|
||||
role={label ? "status" : undefined}
|
||||
aria-label={label}
|
||||
className={cn(spinnerVariants({ size, className }))}
|
||||
{...props}
|
||||
>
|
||||
|
||||
@@ -161,7 +161,11 @@ export const AppearanceForm: FC<AppearanceFormProps> = ({
|
||||
title={
|
||||
<div className="flex flex-row items-center gap-2">
|
||||
<span>Theme</span>
|
||||
<Spinner loading={isUpdating} size="sm" />
|
||||
<Spinner
|
||||
loading={isUpdating}
|
||||
size="sm"
|
||||
label="Saving theme preference"
|
||||
/>
|
||||
</div>
|
||||
}
|
||||
layout="fluid"
|
||||
@@ -217,7 +221,11 @@ export const AppearanceForm: FC<AppearanceFormProps> = ({
|
||||
title={
|
||||
<div className="flex flex-row items-center gap-2">
|
||||
<span id={fontGroupLabelId}>Terminal Font</span>
|
||||
<Spinner loading={isUpdating} size="sm" />
|
||||
<Spinner
|
||||
loading={isUpdating}
|
||||
size="sm"
|
||||
label="Saving terminal font"
|
||||
/>
|
||||
</div>
|
||||
}
|
||||
layout="fluid"
|
||||
|
||||
Reference in New Issue
Block a user