Skip to content

chore: Initial database scaffolding - #2

Merged
kylecarbs merged 8 commits into
mainfrom
database
Jan 5, 2022
Merged

kylecarbs merged 8 commits into
mainfrom
database

Conversation

@kylecarbs

@kylecarbs kylecarbs commented Jan 4, 2022

Copy link
Copy Markdown
Member

This implements migrations and code generation for interfacing with a PostgreSQL database.

A dependency is added for the "postgres" binary on the host, but that seems like an acceptable requirement considering it's our primary database. We decided Docker was a more reliable and ubiquitous dependency, so we launch a PostgreSQL Docker container instead.

An in-memory database object can be created for simple cross-OS and fast testing.

This implements migrations and code generation for interfacing with a PostgreSQL database.

A dependency is added for the "postgres" binary on the host, but that seems like an acceptable requirement considering it's our primary database.

An in-memory database object can be created for simple cross-OS and fast testing.
@kylecarbs
kylecarbs requested a review from bryphe-coder January 4, 2022 15:25
@kylecarbs kylecarbs self-assigned this Jan 4, 2022
Comment thread .github/workflows/coder.yaml Outdated
# Check that go is available
# TODO: Implement actual test run
- run: go version
- run: go test -v ./...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, our CI is actually doing something now 🎉

if err != nil {
return "", nil, xerrors.Errorf("create pool: %w", err)
}
resource, err := pool.RunWithOptions(&dockertest.RunOptions{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me for now! As we discussed, if Docker is too heavy a dependency... we could look at running psql manually in the future.

I think this approach makes for now, though, since the team is familiar with this approach from coderd. And if we decide Docker is too heavy, there might be other approaches we take (like revisiting a cloud solution...)

@kylecarbs
kylecarbs merged commit 025b55f into main Jan 5, 2022
@kylecarbs
kylecarbs deleted the database branch January 20, 2022 16:01
bryphe-coder added a commit that referenced this pull request Feb 2, 2022
BrunoQuaresma added a commit that referenced this pull request May 8, 2025
@bpmct bpmct mentioned this pull request Dec 6, 2025
2 tasks
david-fraley added a commit that referenced this pull request Apr 30, 2026
Two regressions from 8c0a1b2 that fail markdownlint and would block CI:

- MD009: trailing space after the new getting-started link
- MD051: stale '#2-configure-an-llm-provider' fragment in the testing
  section now that the experiment step is removed and Configure an LLM
  provider is step 1

> Coder Agent generated this commit on @david-fraley's behalf.
kylecarbs added a commit that referenced this pull request May 7, 2026
… history

Replaces the previous draft of this PR (a backend workspaces.claimed_at
column plus migration plus SDK plumbing) with a frontend-only
heuristic per Cian's review.

The /agents archive-and-delete molly-guard previously compared
workspace.created_at against chat.created_at to decide whether to
require typing the workspace name. ClaimPrebuiltWorkspace never
updates workspace.created_at, so claimed prebuilds always looked
pre-existing and the dialog misfired.

Build history already records the truth: build #1's initiator is the
prebuilds system user iff the workspace was a prebuild, and build #2
is the claim. Compute that in the resolver and compare its created_at
against the chat. From-scratch workspaces fall through to
workspace.created_at as before.

The prebuilds system user UUID is hardcoded on the frontend; it lives
in coderd/database/constants.go on the backend and has not changed
since the prebuild feature shipped. If it ever moves, both sides have
to move together.

🤖 Generated with the help of Coder Agents.
bpmct added a commit that referenced this pull request May 21, 2026
mafredri added a commit that referenced this pull request May 25, 2026
waitForTaskIdle used time.NewTicker(5s) which delays the first poll
by 5 seconds. Debugger tracing proved the failure mechanism: on slow
CI (Windows), the first poll at 5s sees "working" (idle patch has not
landed due to goroutine scheduling), needs poll #2 at 10s, but the
25s context expires before it fires.

Two changes:

1. Use r.clock.NewTicker (quartz) with time.Nanosecond initial
   interval and Reset(5s) for immediate first poll. Tests inject a
   mock clock via clitest.NewWithClock for deterministic control.
2. Rewrite WaitsForWorkingAppState test with quartz traps
   (NewTicker + TickerReset) for deterministic synchronization
   instead of racing goroutines. Fix PausedDuringWaitForReady
   sync point.

Closes https://linear.app/codercom/issue/DEVEX-381
mafredri added a commit that referenced this pull request May 25, 2026
#25648)

waitForTaskIdle used time.NewTicker(5s) which delays the first poll
by 5 seconds. Debugger tracing proved the failure mechanism: on slow
CI (Windows), the first poll at 5s sees "working" (idle patch has not
landed due to goroutine scheduling), needs poll #2 at 10s, but the
25s context expires before it fires.

Two changes:

1. Use r.clock.NewTicker (quartz) with time.Nanosecond initial
   interval and Reset(5s) for immediate first poll. Tests inject a
   mock clock via clitest.NewWithClock for deterministic control.
2. Rewrite WaitsForWorkingAppState test with quartz traps
   (NewTicker + TickerReset) for deterministic synchronization
   instead of racing goroutines. Fix PausedDuringWaitForReady
   sync point.

Closes DEVEX-381
tracyjohnsonux added a commit that referenced this pull request Jun 3, 2026
…PR tab for demo

Hardcodes 'fix: increase icon sizes for wcag 2.2 compliance' (PR #2
from coder-ux-prototypes) as a second PR tab in the dropdown to
demonstrate multi-PR switching UI. The extra tab reuses the remote
diff content panel.
f0ssel pushed a commit that referenced this pull request Jun 11, 2026
…#26296)

Backport of #26204 (commit b5ef700) to `release/2.34`

Replace RequestHost with httpmw.EffectiveHost, which honors
X-Forwarded-Host only when the original socket peer is a configured
trusted origin, otherwise falling back to the received Host header.

Cherry-pick was clean; tests pass on this branch.

> [!NOTE]
> Breaking change. See the original PR for the breaking-change details.
f0ssel pushed a commit that referenced this pull request Jun 27, 2026
#26604)

Cherry-pick of #26313

Original PR: #26313 — fix(site): set external auth provider polling
status individually

Co-authored-by: Andrew Aquino <[email protected]>
ibetitsmike pushed a commit that referenced this pull request Jul 21, 2026
…nges (#27059)

Closes CODAGT-592.

## Problem

The advisor tool sometimes fails with the opaque error `advisor produced
no text output`. Live reproduction against `claude-sonnet-4-6` showed
the cause: `BuildAdvisorMessages` forwards the parent conversation's raw
`tool_use`/`tool_result` blocks into the nested advisor call, which
defines no tools. The nested model imitates the forwarded pattern and
spends its turn committing to a tool call it cannot make (captured
reasoning from a failing run: "The user wants me to make another tool
call to the advisor about writing a poem about cucumbers."), so the step
ends with reasoning-only or empty content and no advice. Because each
chat step currently rebuilds the advisor runtime and snapshot
(CODAGT-593), the second advisor call in a run reliably sees the first
call's exchange, which is why the first call succeeds and later ones
fail.

## Fix

- `BuildAdvisorMessages` rewrites tool activity as plain-text notes:
assistant tool-call parts are removed and folded, together with their
matching result, into a single user-role note of the form `[The parent
agent ran the X tool with input {...}. Result: ...]`. No raw tool blocks
and no bare call lines reach the tool-less nested request. This also
removes the provider requirement that `tool_result` blocks pair with a
`tool_use`, so results orphaned by window truncation are kept as notes
instead of dropped.
- The `advisor produced no text output` error now appends the finish
reason and content-part kinds, e.g. `advisor produced no text output
(finish_reason=stop; parts: reasoning=1)`, so field reports distinguish
tool-call mimicry, reasoning-only turns, and truncation.

Validated live by driving the production `RunAdvisor` path against
`claude-sonnet-4-6` through the dev.coder.com AI gateway: the failing
scenario went from 3/3 errors to 6/6 genuine advice (with and without
extended thinking), with the control scenario unaffected.

Related: CODAGT-593 (per-step advisor runtime recreation, addressed
separately) and CODAGT-742 (advisor tool call design).

<details>
<summary>Investigation and validation details</summary>

### Reproduction

A CLI prototype constructed the exact conversation snapshot the
generation preparer hands the advisor tool and called the real
`chatadvisor.NewRuntime` / `Runtime.RunAdvisor` / `BuildAdvisorMessages`
/ `chatloop.GenerateAssistant` chain against live `claude-sonnet-4-6`,
with a stream-teeing model wrapper capturing what `runner.go` discards
(finish reason, part kinds, reasoning text).

| Scenario (snapshot contents) | Thinking | Before fix | After fix |
|---|---|---|---|
| control: call #1 state, no prior advisor exchange | on | 3/3 advice |
2/2 advice |
| repro: call #2 state, prior advisor `tool_use`/`tool_result` pair
forwarded | on | 3/3 `advisor produced no text output` | 3/3 genuine
advice |
| repro | off | 2/3 same error, 1/3 degenerate advice ("I'll ask the
advisor...") | 3/3 genuine advice |

Every failing response was a tiny thinking block, zero text, zero
tool-call stream parts, finish reason `stop`; the model's own reasoning
text showed it deciding to "make the second tool call" in a request with
`tools=0`. The refunded `remaining_uses: 1200` in the failing
tool-result JSON matches the original issue screenshot.

### Decision log

- Tool exchanges are folded into a single user-role note per call/result
pair. A first attempt rendered assistant-authored `[tool call:
name(input)]` text lines plus separate result messages; live runs then
returned the literal `[tool call: advisor(...)]` line as the advice 6/6
times. The bare assistant call line is itself an imitable pattern, so no
assistant-authored tool artifact may survive the handoff. The folded
user-role note produced 6/6 genuine advice.
- An assistant message that carried only tool calls is dropped entirely;
the folded notes preserve the information.
- `dropOrphanToolMessages` was removed: without raw tool blocks there is
no provider pairing constraint, and an orphaned result note retains
context value.
- A reasoning-budget-starvation hypothesis (thinking budget consuming
`MaxOutputTokens`) did not reproduce on `claude-sonnet-4-6`; the model
adapts thinking length to the cap. The enriched error would identify
such cases on other models via `finish_reason=length`.
- CODAGT-593 (persisting the advisor runtime across steps) is
intentionally not addressed here; it shrinks the priming window but the
handoff fix is what removes the failure mode.

</details>

---

*This PR was generated by Coder Agents on behalf of @ThomasK33 (Linear
agent session for CODAGT-592).*
aqandrew added a commit that referenced this pull request Aug 6, 2026
…sidebar row (#27351)

## What

Clicking a selected module in the right-hand `SelectionSummary` sidebar
now jumps to the module settings step and scrolls that module's card
into view.

Addresses [DEVEX-587](https://linear.app/codercom/issue/DEVEX-587). This
is an isolated slice of #27077 (which bundles several unrelated
changes); only the "click a module to scroll to it" behavior is included
here.

## Changes

- `SelectionSummary`: gains an optional `onNavigateModule` prop. When
provided, each selected module row renders as an accessible `<button>`
(hover + focus-ring) labeled `Configure <name>`; otherwise rows stay
non-interactive.
- `ModuleSettingsStep`: each module card wrapper gets a stable
`id={module-config-<id>}` scroll anchor plus `scroll-mt-24` so the
sticky top nav does not cover the title.
- `TemplateBuilderPageView`: adds `navigateToModule`, which switches to
the module settings step and scrolls the target module into view once it
renders. If the module settings step is skipped (no configurable
variables), the click is a no-op.
- `SelectionSummary.stories`: adds a `NavigateModuleClick` interaction
story and updates `WithLongNameModule` to the new button semantics.

## Explicitly out of scope

The rest of #27077 is not included: gallery height/card clamp,
sensitive-var banner relocation, trash-icon removal wiring, the
scroll-past required-field highlight subsystem, and the broader
navigable-sidebar work (step labels, base-row navigation, back-stack
semantics).

## Testing

- `pnpm check` (biome) clean
- `pnpm lint:types` (tsc) clean
- `pnpm vitest run --project=unit src/pages/TemplateBuilder` — 46 pass
- `pnpm vitest run --project=storybook src/pages/TemplateBuilder` — 34
pass (incl. new `NavigateModuleClick`)

<details>
<summary>Implementation plan / decision log</summary>

### Goal

Open a new PR containing only the changes that satisfy DEVEX-587:
clicking a selected module in the right-hand `SelectionSummary` sidebar
should jump to the module settings step and scroll that module's card
into view.

### Base has moved since #27077

PR #27077 was cut against an older `main`. Today's `main` was
refactored:

- Steps are URL-driven; `steps.ts` already provides `StepId`, per-step
`group` (1/2/3), and `nearestVisible()`.
- `TemplateBuilderPageView` already has `navigateToStep(index: number)`
and a `useEffect` that resets window scroll on every `currentStep.id`
change.
- The sidebar no longer has a deselect ("X") button. The PR's
entanglement between "make row a nav button" and "move deselect to a
trash icon" therefore does not exist on current `main`, so module
navigation can be added without removing behavior and without pulling in
the trash-icon item.

So the isolated diff was written against current `main`, not reused
verbatim from the PR. It is smaller than the PR's own hunks and does not
include `maxReachedStep`, step-label navigation, or base-row navigation.

### Decisions

- Scope for this PR: module-row navigation only.
- Skipped-settings fallback: no-op. When no selected module exposes
configurable variables, the `module-settings` step is skipped and
clicking a module row does nothing (there is no card to scroll to).

### Scroll timing

`navigateToStep` triggers a window scroll reset via an existing effect
keyed on `currentStep.id`. To cooperate, `navigateToModule` stores the
target module id in a ref and a follow-up effect (declared after the
scroll-reset effect, so it runs second) calls `scrollIntoView` inside
`requestAnimationFrame` once `module-settings` has rendered. When
already on `module-settings`, it scrolls immediately.

### Follow-up (deferred): full navigable sidebar

Not part of this PR, documented for later. The remainder of #27077's
item #2, rebased onto current `main`:

- `onNavigateStep?: (stepId: StepId) => void` on `SelectionSummary`.
- Clickable step labels: `Base Template` -> `base-infra`, `Modules` ->
`module-select`, `Customizations` -> `customizations`.
- Clickable selected base-template row -> `base-parameters` (fall back
to `base-infra` when that step is skipped for the chosen base).
- Back-stack semantics via a `maxReachedStep` prop so steps at or below
the furthest-reached group stay `complete` and clickable even after
navigating backward, while strictly-higher groups render as inert
`upcoming`.
- `StepIndicator` and `BaseTemplateSelection` render as `<button>` when
a navigation handler is supplied, else stay inert.
- Stories: `NavigationClicks`, `BackwardNavigation`, and
`UpcomingStepsInert`.

</details>

---

Coder Agents generated, on behalf of @aqandrew.
aqandrew added a commit that referenced this pull request Aug 17, 2026
…28153)

## What

Makes the remaining `SelectionSummary` sidebar elements clickable jump
targets on `/templates/new/builder`, continuing the work from #27351
(which made module rows navigable).

Clickable now:

| Sidebar element | Jumps to |
|---|---|
| `Base Template` label | `base-infra` |
| Selected base-template row | `base-parameters` (falls back to
`base-infra` when that step is skipped) |
| `Modules` label | `module-select` |
| Each module row | `module-settings` + scroll (already shipped in
#27351) |
| `Customizations` label | `customizations` |

## Back-stack behavior

The sidebar previously colored groups purely from the current step, so
jumping backward would grey out and disable steps you had already
reached. This adds a `maxReachedGroup` that never shrinks on backward
navigation:

- Groups at or below the furthest-reached group stay `complete` (green)
and clickable, like a browser back-stack.
- Groups strictly above render as `upcoming` and inert (no button, no
hover, not focusable).
- The connecting divider color keys off `maxReachedGroup`, not the
current step, so it stays green after navigating backward.

Clickability is gated on `maxReachedGroup` (you can only jump to steps
you have already reached).

## Changes

- `SelectionSummary.tsx`: new required `maxReachedStep` and
`onNavigateStep` props. Split the single `variant()` into
`indicatorVariant` (label circle), `dividerVariant` (connecting line),
and a `reachable()` gate. `StepIndicator` and `BaseTemplateSelection`
render as `<button>` (hover + focus ring, `aria-label`) when a reachable
handler is supplied, else stay inert.
- `TemplateBuilderPageView.tsx`: track `maxReachedGroup`; add
`navigateToStepId(stepId)` that resolves skipped steps via
`nearestVisible` (so `base-parameters` falls back to `base-infra`) and
mirrors the existing customizations reset when leaving that step. Wire
both new props into `SelectionSummary`.
- `SelectionSummary.stories.tsx`: add `onNavigateStep` to meta and
`maxReachedStep` to existing stories; add `NavigationClicks` (asserts
each label/base/module callback), `BackwardNavigation` (dividers stay
green), and `UpcomingStepsInert` (steps above max-reached are not
buttons).

## Out of scope

Everything else from #27077 stays out: gallery height, sensitive-var
banner relocation, trash-icon wiring, and the scroll-past required-field
subsystem.

## Testing

- `pnpm check` (biome) clean
- `pnpm lint:types` (tsc) clean
- `pnpm vitest run --project=storybook src/pages/TemplateBuilder` — 37
pass
- `pnpm vitest run --project=unit src/pages/TemplateBuilder` — 55 pass

<details>
<summary>Implementation plan / decision log</summary>

### Origin

This is the remainder of PR #27077's item #2 (navigable selection
summary), rebased onto current `main` after #27351 shipped the
module-row navigation.

### Why a `maxReachedGroup` back-stack

`furthestAllowedIndex(state)` on current `main` is all-or-nothing (0
without a base selected, otherwise the last step), so it cannot express
"how far the user has progressed" for the sidebar coloring. A monotonic
`maxReachedGroup` (bumped when the current group advances, never shrunk)
is needed to keep completed steps green and clickable after backward
navigation, matching #27077.

### Decisions

- Reachability gating: gate both coloring and clickability on
`maxReachedGroup` (only jump to steps already reached), rather than the
looser `furthestAllowedIndex` (which would let users skip required steps
once a base is chosen).
- Base-template row target: jump to `base-parameters` and let
`nearestVisible` fall back to `base-infra` when the base has no
parameters/prerequisites.
- Module rows: keep `onNavigateModule` passed directly (not re-gated on
reachability), since a module can only be selected after reaching group
2, so `reachable(2)` is always true when module rows render. This
preserves the earlier decision to keep the module row's handler required
with no inert branch.

</details>

---

Coder Agents generated, on behalf of @aqandrew.
jeremyruppel added a commit that referenced this pull request Aug 17, 2026
## 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants