feat: add theme mode dropdown - #25183
Conversation
Adds three new UserAppearanceSettings fields (theme_mode, theme_light, theme_dark) on top of the existing theme_preference and terminal_font. Replaces GetUserThemePreference and GetUserTerminalFont with a single GetUserAppearanceSettings aggregate query. The PUT handler is wrapped in db.InTx so sync-mode's mode + slot writes can never half-apply.
|
/coder-agents-review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid work wiring the theme mode dropdown. The submit-queue mechanism (at-most-one in-flight, latest-wins coalescing, rollback on failure) is well-designed, and the test coverage targets the hard concurrency paths rather than surface details. Story coverage is thorough: 16 named variants covering single, sync, accessibility themes, legacy migration, hover preview, and font selection. The e2e test was updated to match the new flow.
8 P3, 1 P4, 3 Nits below. The P3s cluster around the submit state machine's edge cases: an unguarded form-submit path, a stale closure, missing documentation, and silent queue drops on failure. None are crash bugs today; all are maintenance hazards in code that is already the most complex part of this file.
Process note: the commit subjects fix: improve form handling and chore: cleanup don't convey what changed. Something like fix(site): queue rapid theme changes to prevent dropped updates would help git bisect and blame.
"Formik is introduced without using any of its features, then worked around." (Pariston)
🤖 This review was automatically generated with Coder Agents.
…-conflicts # Conflicts: # coderd/apidoc/docs.go # coderd/apidoc/swagger.json # coderd/database/dbauthz/dbauthz.go # coderd/database/dbmetrics/querymetrics.go # coderd/database/dbmock/dbmock.go # coderd/database/querier.go # coderd/database/queries.sql.go # coderd/database/queries/users.sql # coderd/users.go # codersdk/users.go # docs/reference/api/schemas.md # site/src/api/queries/users.test.ts # site/src/api/typesGenerated.ts # site/src/pages/UserSettingsPage/AppearancePage/AppearanceForm.stories.tsx # site/src/pages/UserSettingsPage/AppearancePage/AppearanceForm.tsx # site/src/pages/UserSettingsPage/AppearancePage/AppearancePage.test.tsx # site/src/pages/UserSettingsPage/AppearancePage/AppearancePage.tsx # site/src/pages/UserSettingsPage/AppearancePage/SingleModeSection.tsx # site/src/pages/UserSettingsPage/AppearancePage/SyncModeSection.tsx # site/src/pages/UserSettingsPage/Section.tsx # site/src/theme/darkTritan/roles.ts # site/src/theme/lightTritan/roles.ts # site/src/theme/themeMode.test.ts # site/src/theme/themeMode.ts # site/src/theme/usePreferredColorScheme.ts
jaaydenh
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/coder-agents-review |
There was a problem hiding this comment.
All 13 R1 findings addressed cleanly. Formik removed, submit queue simplified, documentation added, test coverage expanded. The rewrite is well-executed.
2 P3, 3 Nits new this round. The P3s are about the confirmedValuesRef being contaminated by optimistic cache updates (one-line fix) and queue tests stopping one assertion short of proving the terminal UI state.
"The ref's name is
confirmedValuesRef. The useEffect overwrites it with unconfirmed values." (Mafuuu)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
All 18 findings across 4 rounds addressed. The submit queue is clean: confirmedValuesRef guard is correctly placed, onSubmitRef prevents stale closures, native submit is prevented, the queue is documented, and test coverage proves the terminal state. Formik removal simplified the state management without losing any capability. Story and test coverage is thorough (4 vitest tests, 7 interactive stories, 24 pure-logic unit tests, 1 e2e).
No open findings.
🤖 This review was automatically generated with Coder Agents.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3971f2661
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) => void; | ||
|
|
||
| export const useQueuedAppearanceSubmit = (mutate: MutateAppearanceSettings) => { | ||
| const submitInFlightRef = useRef(false); |
There was a problem hiding this comment.
Is there any reason why we can't just pass in the isPending from the mutation passed through?
There was a problem hiding this comment.
isPending only reflects the state of the last render. Basically, i think would need to stay like this unless the hook is restructured somehow.
DanielleMaywood
left a comment
DanielleMaywood
left a comment
There was a problem hiding this comment.
Approving but I definitely think a better design exists such that we don't have to rely on useRefs as much
2 participants
Summary
Dependencies
pr25077/05-theme-mode-dropdown-base, which contains dependency commits only, so this PR diff stays focused on final dropdown wiring. Rebase and retarget after the dependency PRs merge.Validation
pnpm -C site exec vitest run --project=unit src/pages/UserSettingsPage/AppearancePage/AppearancePage.test.tsx src/theme/themeMode.test.ts src/api/queries/users.test.tspnpm -C site lint:typespnpm -C site storybook:cipnpm -C site buildpnpm -C site playwright:test -- e2e/tests/users/userSettings.spec.ts