From 46ec620767dc684917f9d466b22b78055bb5075f Mon Sep 17 00:00:00 2001 From: Jeremy Ruppel Date: Mon, 17 Aug 2026 15:36:46 -0400 Subject: [PATCH] fix(site): deflake adjust user theme preference (#28219) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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`.
Implementation plan & decision log # 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 `` 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 `` 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 ``. 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 ``` `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).
--- This PR was created by Coder Agents on behalf of @jeremyruppel. --------- Co-authored-by: Samuel Volin --- site/e2e/tests/users/userSettings.spec.ts | 18 ++++++++++++++++-- site/src/components/Loader/Loader.tsx | 3 ++- site/src/components/Spinner/Spinner.tsx | 8 ++++++++ .../AppearancePage/AppearanceForm.tsx | 12 ++++++++++-- 4 files changed, 36 insertions(+), 5 deletions(-) diff --git a/site/e2e/tests/users/userSettings.spec.ts b/site/e2e/tests/users/userSettings.spec.ts index ff419f89ea..601086b911 100644 --- a/site/e2e/tests/users/userSettings.spec.ts +++ b/site/e2e/tests/users/userSettings.spec.ts @@ -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 diff --git a/site/src/components/Loader/Loader.tsx b/site/src/components/Loader/Loader.tsx index 3ba241dcf3..684e9532d2 100644 --- a/site/src/components/Loader/Loader.tsx +++ b/site/src/components/Loader/Loader.tsx @@ -25,6 +25,7 @@ export const Loader: FC = ({ {...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 = ({ className, )} > - + ); }; diff --git a/site/src/components/Spinner/Spinner.tsx b/site/src/components/Spinner/Spinner.tsx index 847aa26ab7..5a69afd7fd 100644 --- a/site/src/components/Spinner/Spinner.tsx +++ b/site/src/components/Spinner/Spinner.tsx @@ -27,6 +27,11 @@ type SpinnerProps = React.SVGProps & VariantProps & { 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} > diff --git a/site/src/pages/UserSettingsPage/AppearancePage/AppearanceForm.tsx b/site/src/pages/UserSettingsPage/AppearancePage/AppearanceForm.tsx index 06752bf4c1..15ac0c5da0 100644 --- a/site/src/pages/UserSettingsPage/AppearancePage/AppearanceForm.tsx +++ b/site/src/pages/UserSettingsPage/AppearancePage/AppearanceForm.tsx @@ -161,7 +161,11 @@ export const AppearanceForm: FC = ({ title={
Theme - +
} layout="fluid" @@ -217,7 +221,11 @@ export const AppearanceForm: FC = ({ title={
Terminal Font - +
} layout="fluid"