Restore theme picker with system dark support - #6951
Conversation
Brings back theme controls in Settings with Light, Dark, and System options, and wires theme through app appearance updates so document state and host window appearance stay in sync. Theme persistence now accepts valid stored values (instead of forcing light), with a default fallback for invalid values. Updates shared config contracts to include `theme`, adds new i18n strings, and adjusts dark-mode CSS selectors to treat `data-theme="system"` as OS-following dark mode. Tests were rewritten to validate the restored theme UI and the new theme persistence/application behavior.
|
Thanks @piyushsarkar-dev — restoring the theme picker and wiring system-dark behavior back through Settings/App/config is a meaningful fix, and I’ll take care of the initial triage from here. 💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …): |
lefarcen
added
size/L
|
🧪 This PR has changes that need a manual QA pass before merge — please hold off self-merging for now; we'll loop QA in once it's merge-ready (and design/product have signed off, where applicable). |
lefarcen
added
needs-design-review
lefarcen
left a comment
lefarcen
left a comment
There was a problem hiding this comment.
Hey @piyushsarkar-dev — bringing the theme controls back and wiring the system-dark path through Settings/App/config is a solid direction. Before pool review can scope this cleanly, could you fill in the missing PR-body pieces: Why, What users will see, Surface area, and Validation with what you actually ran? Related: #6168 removed this setting and forced light, so it would also help to say explicitly whether this PR is restoring that earlier behavior and what changed since then.
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
The restored picker currently leaves system mode inconsistent across first paint, runtime theme consumers, and the typed locale contract. The inline findings below need to be addressed before this can merge.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| getOpenDesignHost()?.appearance?.setTheme(FORCED_APP_THEME); | ||
| const resolvedTheme = resolveAppTheme(theme); | ||
|
|
||
| root.setAttribute('data-theme', resolvedTheme); |
There was a problem hiding this comment.
BLOCKING: Preserve the existing system-theme contract across every consumer before stamping data-theme=system. Several unchanged consumers only implement system dark as html:not([data-theme]): for example apps/web/src/styles/workspace/artifacts.css lines 4224-4237 and apps/web/src/components/workspace/TerminalViewer.module.css lines 81-82. With this new attribute present on a dark OS, the main token files become dark but those controls retain their light-specific styling, producing a mixed theme. The changed tests only assert the attribute value, so they do not catch this. Either keep system mode represented by an absent attribute and store the preference separately, or update the complete selector/reader matrix to treat system as OS-following and add a dark-OS fixture matrix covering representative CSS-module and global-style consumers.
| useLayoutEffect(() => { | ||
| applyAppearanceToDocument({ accentColor: config.accentColor }); | ||
| }, [config.accentColor]); | ||
| applyAppearanceToDocument({ accentColor: config.accentColor, theme: config.theme }); |
There was a problem hiding this comment.
BLOCKING: Apply the persisted theme in the pre-hydration path as well as this layout effect. apps/web/app/layout.tsx still unconditionally executes document.documentElement.setAttribute('data-theme','light'), so a stored dark/system preference paints light before React reaches this changed line; the comment there explicitly describes the now-obsolete light-only invariant. This reintroduces the startup flash that the pre-hydration script exists to prevent, especially in the packaged app. Update themeInitScript to validate and apply the stored light | dark | system value consistently with resolveAppTheme, update its documentation, and restore a test that executes the script for stored dark and system-on-dark-OS cases.
| 'settings.agentInstallGroup': string; | ||
| 'settings.agentAuthRequired': string; | ||
| 'settings.agentAuthUnknown': string; | ||
| 'settings.theme': string; |
There was a problem hiding this comment.
BLOCKING: Define these new required dictionary keys in every locale. This interface makes all five keys mandatory, but the patch adds them only to en.ts and zh-CN.ts; the other 17 apps/web/src/i18n/locales/*.ts files still export objects explicitly typed as Dict, so the web typecheck cannot succeed and non-English locales cannot render the new labels through the typed contract. Add translations (or the repository-approved fallback values) to all 19 locale files, as required by the root i18n rule.
|
Thanks for the quick pass here, @PerishCode. @piyushsarkar-dev, the current blockers are covered in that review on this head: the |
Visual regression reviewHead: 5 changed · 44 unchanged · 0 new without baseline · 0 failed Changed cases
Unchanged cases
Visual diff is advisory only and does not block merging. |
3 participants
Brings back theme controls in Settings with Light, Dark, and System options, and wires theme through app appearance updates so document state and host window appearance stay in sync. Theme persistence now accepts valid stored values (instead of forcing light), with a default fallback for invalid values.
Updates shared config contracts to include
theme, adds new i18n strings, and adjusts dark-mode CSS selectors to treatdata-theme="system"as OS-following dark mode. Tests were rewritten to validate the restored theme UI and the new theme persistence/application behavior.Fixes #
Why
What users will see
Surface area
apps/weborapps/desktop(including Electron menu bar)odsubcommand or flag, newtools-dev/tools-packflag, or newOD_*env var/api/*endpoint, new SSE event, or changed shape inpackages/contractsskills/,design-systems/,design-templates/, orcraft/, or change to the skills protocolTRANSLATIONS.mdfor the locale workflow)package.json(dependenciesordevDependencies); workspace-packagepackage.jsonfiles are out of scope. Include a paragraph on what we get vs. what bytes we ship (seeCONTRIBUTING.md→ Code style)Screenshots
Bug fix verification
Validation