fix(site): deflake adjust user theme preference by jeremyruppel · Pull Request #28219 · coder/coder

jeremyruppel · 2026-08-17T16:24:27Z

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 <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 chore: Initial database scaffolding #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:

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.
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):

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):

<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:

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.

linear-code · 2026-08-17T16:24:30Z

DEVEX-415

The "adjust user theme preference" e2e 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, so the reloaded document embedded the stale dark preference and the final assertion flaked. Wait for the appearance form's save spinner to clear before the hard reload, which mirrors 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, and a repeat run that is already light never shows it, so the test is idempotent. To give the test a UI signal (instead of a direct API call), add an opt-in `label` prop to Spinner that exposes it as a role=status live region with an aria-label; spinners stay decorative when no label is given. Move Loader's label onto its own status container so it keeps a single status region, and label both appearance save spinners.

jeremyruppel · 2026-08-17T18:39:28Z

I decided to update the <Spinner> API (in a backwards-compatible fashion) to accept a label prop. Many times the <Spinner> is inside a <Loader> so the Loader element gets aria-label and a role=status, but the <Spinner> outside a <Loader> does not have such attributes and therefore it's hard to assert the presence/absence of the element in tests. This makes it so <Spinner> with a label=fnord applies the aria-label and role=status to the Spinner element (which passes through to its svg)

untra · 2026-08-17T18:45:59Z

+	// 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.


so I've noticed claude comments a lot of code, and excessive comments aren't exactly helpful, especially if they are documenting code shorter than what is written or edited.

Comments can't be tested for accuracy, and they can rot faster than the code. They also add to the code that LLMs read, using more context in the future. When letting AI take a pass on writing code, consider removing or trimming down the comments, especially to just function headers, and what is necessary to document best usage.

for sure, I left these in because I personally found them helpful, and this one in particular because it describes a failure mode the tests don't exercise. totes happy to remove it though!

Make Spinner's label prop description less LLM-y Co-authored-by: Samuel Volin <[email protected]>

untra

thanks for letting me pedantic about code comments 😅 👍

github-actions Bot assigned jeremyruppel Aug 17, 2026

jeremyruppel changed the title ~~fix(site/e2e/tests/users): deflake adjust user theme preference~~ fix(site): deflake adjust user theme preference Aug 17, 2026

jeremyruppel force-pushed the jeremy/devex-415-flake-adjust-user-theme-preference-playwright-e2e branch from 55b39e7 to f0e009c Compare August 17, 2026 18:32

jeremyruppel marked this pull request as ready for review August 17, 2026 18:39

jeremyruppel requested review from aslilac and untra August 17, 2026 18:39

untra reviewed Aug 17, 2026

View reviewed changes

Comment thread site/src/components/Spinner/Spinner.tsx

jeremyruppel and others added 2 commits August 17, 2026 14:51

Update site/src/components/Spinner/Spinner.tsx

c46ef99

Make Spinner's label prop description less LLM-y Co-authored-by: Samuel Volin <[email protected]>

chore(site/src/components/Spinner): fix comment formatting

11618ac

untra approved these changes Aug 17, 2026

View reviewed changes

jeremyruppel merged commit 46ec620 into main Aug 17, 2026
26 checks passed

jeremyruppel deleted the jeremy/devex-415-flake-adjust-user-theme-preference-playwright-e2e branch August 17, 2026 19:36

github-actions Bot locked and limited conversation to collaborators Aug 17, 2026

Navigation Menu

Uh oh!

fix(site): deflake adjust user theme preference - #28219

fix(site): deflake adjust user theme preference#28219
jeremyruppel merged 3 commits into
mainfrom
jeremy/devex-415-flake-adjust-user-theme-preference-playwright-e2e

jeremyruppel commented Aug 17, 2026

Uh oh!

linear-code Bot commented Aug 17, 2026

Uh oh!

jeremyruppel commented Aug 17, 2026

Uh oh!

untra Aug 17, 2026

Uh oh!

jeremyruppel Aug 17, 2026

Uh oh!

Uh oh!

untra left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

jeremyruppel commented Aug 17, 2026

Summary

Fix

Changes

Validation

DEVEX-415: Fix flake in "adjust user theme preference" e2e test

Problem

Root cause (confirmed by reading the code)

Implemented fix: wait for the saving spinner (UI signal) before navigation

Implemented changes

Alternatives considered

Files touched

Uh oh!

linear-code Bot commented Aug 17, 2026

Uh oh!

jeremyruppel commented Aug 17, 2026

Uh oh!

untra Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

jeremyruppel Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

untra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants