feat(web): add light theme with dark/light/system toggle#853
feat(web): add light theme with dark/light/system toggle#853CPU-216 wants to merge 2 commits intonearai:stagingfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a comprehensive light theme for the Web Gateway UI, alongside a three-state toggle that allows users to switch between dark, light, and system-preferred themes. The implementation prioritizes a pure CSS and vanilla JavaScript approach, ensuring no new dependencies are added. Key design decisions focused on maintaining zero visual regression in dark mode, providing a real-time system theme following OS preferences, and preventing Flash of Unstyled Content (FOUC) for a seamless user experience. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a well-implemented theme-switching feature with dark, light, and system modes. The use of CSS custom properties for theming is a significant improvement, making the styles much more maintainable. The FOUC prevention via an inline script in the <head> is a great touch. I've left a few comments with suggestions for minor improvements, mainly regarding JavaScript variable declarations for consistency and a potential issue with the global CSS transition rule.
| function applyTheme(mode) { | ||
| var resolved = resolveTheme(mode); | ||
| document.documentElement.setAttribute('data-theme', resolved); | ||
| document.documentElement.setAttribute('data-theme-mode', mode); | ||
| var titles = { dark: 'Theme: Dark (click for Light)', light: 'Theme: Light (click for System)', system: 'Theme: System (click for Dark)' }; | ||
| var btn = document.getElementById('theme-toggle'); | ||
| if (btn) btn.title = titles[mode] || ''; | ||
| } | ||
|
|
||
| function toggleTheme() { | ||
| var cycle = { dark: 'light', light: 'system', system: 'dark' }; | ||
| var current = getThemeMode(); | ||
| var next = cycle[current] || 'dark'; | ||
| localStorage.setItem('ironclaw-theme', next); | ||
| applyTheme(next); | ||
| } |
There was a problem hiding this comment.
For consistency with the existing codebase and modern JavaScript best practices, it's better to use const for variables that are not reassigned. In the newly added functions, all instances of var can be replaced with const.
| function applyTheme(mode) { | |
| var resolved = resolveTheme(mode); | |
| document.documentElement.setAttribute('data-theme', resolved); | |
| document.documentElement.setAttribute('data-theme-mode', mode); | |
| var titles = { dark: 'Theme: Dark (click for Light)', light: 'Theme: Light (click for System)', system: 'Theme: System (click for Dark)' }; | |
| var btn = document.getElementById('theme-toggle'); | |
| if (btn) btn.title = titles[mode] || ''; | |
| } | |
| function toggleTheme() { | |
| var cycle = { dark: 'light', light: 'system', system: 'dark' }; | |
| var current = getThemeMode(); | |
| var next = cycle[current] || 'dark'; | |
| localStorage.setItem('ironclaw-theme', next); | |
| applyTheme(next); | |
| } | |
| function applyTheme(mode) { | |
| const resolved = resolveTheme(mode); | |
| document.documentElement.setAttribute('data-theme', resolved); | |
| document.documentElement.setAttribute('data-theme-mode', mode); | |
| const titles = { dark: 'Theme: Dark (click for Light)', light: 'Theme: Light (click for System)', system: 'Theme: System (click for Dark)' }; | |
| const btn = document.getElementById('theme-toggle'); | |
| if (btn) btn.title = titles[mode] || ''; | |
| } | |
| function toggleTheme() { | |
| const cycle = { dark: 'light', light: 'system', system: 'dark' }; | |
| const current = getThemeMode(); | |
| const next = cycle[current] || 'dark'; | |
| localStorage.setItem('ironclaw-theme', next); | |
| applyTheme(next); | |
| } |
src/channels/web/static/index.html
Outdated
| (function() { | ||
| var mode = localStorage.getItem('ironclaw-theme') || 'system'; | ||
| var resolved = mode; | ||
| if (mode === 'system') { | ||
| resolved = window.matchMedia('(prefers-color-scheme: light)').matches ? 'light' : 'dark'; | ||
| } | ||
| document.documentElement.setAttribute('data-theme', resolved); | ||
| document.documentElement.setAttribute('data-theme-mode', mode); | ||
| })(); |
There was a problem hiding this comment.
Similar to my comment in app.js, it would be best to use const and let instead of var for consistency and to follow modern JavaScript practices. This helps prevent potential issues related to variable hoisting and scope.
| (function() { | |
| var mode = localStorage.getItem('ironclaw-theme') || 'system'; | |
| var resolved = mode; | |
| if (mode === 'system') { | |
| resolved = window.matchMedia('(prefers-color-scheme: light)').matches ? 'light' : 'dark'; | |
| } | |
| document.documentElement.setAttribute('data-theme', resolved); | |
| document.documentElement.setAttribute('data-theme-mode', mode); | |
| })(); | |
| (function() { | |
| const mode = localStorage.getItem('ironclaw-theme') || 'system'; | |
| let resolved = mode; | |
| if (mode === 'system') { | |
| resolved = window.matchMedia('(prefers-color-scheme: light)').matches ? 'light' : 'dark'; | |
| } | |
| document.documentElement.setAttribute('data-theme', resolved); | |
| document.documentElement.setAttribute('data-theme-mode', mode); | |
| })(); |
src/channels/web/static/style.css
Outdated
|
|
||
| body.theme-transition, | ||
| body.theme-transition *:not(svg):not(path):not(line):not(circle):not(rect) { | ||
| transition: background-color 0.3s ease, color 0.3s ease, border-color 0.3s ease !important; |
There was a problem hiding this comment.
Using the universal selector (*) with !important for transitions can be problematic. It can lead to performance issues on complex pages and make future CSS debugging difficult by overriding more specific transition rules unexpectedly. I'd recommend removing !important. If this causes issues, it might indicate that a more targeted approach to applying transitions is needed, rather than a global override.
| transition: background-color 0.3s ease, color 0.3s ease, border-color 0.3s ease !important; | |
| transition: background-color 0.3s ease, color 0.3s ease, border-color 0.3s ease; |
zmanian
left a comment
There was a problem hiding this comment.
Overall this is a well-structured PR. The CSS variable extraction, FOUC prevention, and CSS-only icon switching are all done correctly. No XSS vectors, no Rust changes, no new dependencies. A few issues to address before merging:
Bug: deny button text color in dark mode
The deny button was changed from color: #fff to color: var(--text-on-accent). In dark mode, --text-on-accent resolves to #09090b (near-black). The deny button has a var(--danger) (red) background -- black text on a red button is unreadable. This needs a separate variable (e.g. --text-on-danger: #fff for dark, #fff for light) or keep it as #fff since white-on-red works in both themes.
Same issue applies to the .toast notification (color: var(--text-on-accent) on danger/warning toasts).
Incomplete variable extraction
The PR claims to extract "101 hardcoded color values" but many identical values remain unconverted. For example, #auth-screen input:focus still uses rgba(52, 211, 153, 0.3) instead of var(--accent-border-subtle), and #auth-screen button still uses color: #09090b instead of var(--text-on-accent). There are roughly 40-50 remaining instances of hardcoded rgba(52, 211, 153, ...), #09090b, #fff, #1a1a1a, #333, #666, #888 values that should use the new CSS variables. These will render incorrectly in light mode since they won't respond to the theme override.
I'd recommend doing a sweep of the full style.css to replace all remaining instances. If intentionally left for a follow-up, note that in the PR description and file a tracking issue.
Minor: theme transition selector is broad
body.theme-transition *:not(svg):not(path):not(line):not(circle):not(rect) {
transition: background-color 0.3s ease, color 0.3s ease, border-color 0.3s ease !important;
}The * selector with !important overrides any existing transition properties on elements. This could interfere with existing animations (e.g., the indeterminate progress bar, button hover transitions). Consider scoping to specific properties or using a more targeted selector. Not a blocker but worth testing the restart modal animation with theme transitions enabled.
Accessibility
The toggle button has aria-label="Toggle theme" which is good. Consider also adding aria-live="polite" on a visually-hidden element that announces the current theme state after toggling, so screen reader users know which theme was selected. Not a blocker.
zmanian
left a comment
There was a problem hiding this comment.
Re-review: All previous feedback addressed
The fix commit (8e677c1) addresses every item from my previous review:
Previously requested changes -- all resolved
-
Deny button text color (bug): Fixed. Added
--text-on-danger: #fffvariable used on.denybutton and.toastnotification. White-on-red is now correct in both themes. -
Incomplete variable extraction: Fixed. All remaining hardcoded color values have been replaced with CSS variables. The only hex/rgba values in the diff are inside CSS variable definitions (
:rootand[data-theme="light"]), which is correct. The third-party brand colors (Telegram#0088cc, Signal#3b76f0, Slack#e01e5a) are appropriately left as-is since they are brand-specific and theme-independent. -
!importanton*transition selector: Fixed.!importantremoved from thebody.theme-transitionrule. -
Accessibility (
aria-live): Fixed. Added<span id="theme-announce" class="sr-only" aria-live="polite">that announces the current theme on toggle. The.sr-onlystyle is inlined in<head>to avoid FOUC dependency. -
vartoconst/let(gemini-code-assist feedback): Fixed. All 6vardeclarations inapp.jsreplaced withconst. In the FOUC script,var mode->const mode,var resolved->let resolved.
Minor note (non-blocking)
.image-preview-remove:hover changed from background: #c33 to background: var(--danger). Since the non-hover state is already var(--danger), hover no longer has a visual distinction. Low-impact since it is a small remove button on image previews, but could be addressed in a follow-up with a --danger-hover variable if desired.
Overall
Clean implementation. No XSS vectors, no Rust changes, no new dependencies. The CSS variable extraction is now comprehensive and the light theme values look well-chosen. LGTM.
zmanian
left a comment
There was a problem hiding this comment.
Re-review: All previous feedback addressed
The fix commit (8e677c1) addresses every item from my previous review:
Previously requested changes -- all resolved
-
Deny button text color (bug): Fixed. Added --text-on-danger: #fff variable used on .deny button and .toast notification. White-on-red is now correct in both themes.
-
Incomplete variable extraction: Fixed. All remaining hardcoded color values have been replaced with CSS variables. The only hex/rgba values in the diff are inside CSS variable definitions (:root and [data-theme="light"]), which is correct. The third-party brand colors (Telegram #0088cc, Signal #3b76f0, Slack #e01e5a) are appropriately left as-is since they are brand-specific and theme-independent.
-
!important on * transition selector: Fixed. !important removed from the body.theme-transition rule.
-
Accessibility (aria-live): Fixed. Added span#theme-announce with sr-only class and aria-live="polite" that announces the current theme on toggle. The .sr-only style is inlined in head to avoid FOUC dependency.
-
var to const/let (gemini-code-assist feedback): Fixed. All 6 var declarations in app.js replaced with const. In the FOUC script, var mode -> const mode, var resolved -> let resolved.
Minor note (non-blocking)
.image-preview-remove:hover changed from background: #c33 to background: var(--danger). Since the non-hover state is already var(--danger), hover no longer has a visual distinction. Low-impact since it is a small remove button on image previews, but could be addressed in a follow-up with a --danger-hover variable if desired.
Overall
Clean implementation. No XSS vectors, no Rust changes, no new dependencies. The CSS variable extraction is now comprehensive and the light theme values look well-chosen. LGTM.
250bace to
59d110f
Compare
|
@zmanian Rebased onto latest staging to resolve merge conflicts. All three commits preserved cleanly. Could you re-approve when you get a chance? Thanks! |
Add three-state theme toggle (dark → light → system) to the Web Gateway: - Extract 101 hardcoded CSS colors into 30+ CSS custom properties - Add [data-theme='light'] overrides for all variables - Add theme toggle button in tab-bar (moon/sun/monitor icons) - Theme persists via localStorage, defaults to 'system' - System mode follows OS prefers-color-scheme in real-time - FOUC prevention via inline script in <head> - Delayed CSS transition to avoid flash on initial load - Pure CSS icon switching via data-theme-mode attribute Closes nearai#761
bd2ae84 to
53986c9
Compare
- Fix dark-mode readability bug: .stepper-step.failed and .image-preview-remove used --text-on-accent (#09090b) on var(--danger) background, making text unreadable. Changed to --text-on-danger (#fff). - Restore hover visual feedback on .image-preview-remove:hover using filter: brightness(1.2) instead of redundant var(--danger). - Use const/let instead of var in theme-init.js for consistency with app.js (per gemini-code-assist review feedback). Co-Authored-By: CPU-216 <3125034290@stu.cpu.edu.cn> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the work on this, @CPU-216! I've picked up your changes and continued them in #1457. The new PR includes your original work plus:
You're credited as co-author on the fix commit. Feel free to review the new PR! |
- Fix missing `fallback_deliverable` field in job_monitor test constructors (pre-existing staging issue surfaced by merge) - Validate localStorage theme value against whitelist in both theme-init.js and app.js to prevent broken state from invalid values - Add matchMedia addEventListener fallback for older Safari/WebKit - Add i18n keys for theme tooltip and aria-live announcement strings (en + zh-CN) to match existing localization patterns - Move .sr-only utility from inline <style> to style.css [skip-regression-check] Co-Authored-By: CPU-216 <3125034290@stu.cpu.edu.cn> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(web): add light theme with dark/light/system toggle (#761) Add three-state theme toggle (dark → light → system) to the Web Gateway: - Extract 101 hardcoded CSS colors into 30+ CSS custom properties - Add [data-theme='light'] overrides for all variables - Add theme toggle button in tab-bar (moon/sun/monitor icons) - Theme persists via localStorage, defaults to 'system' - System mode follows OS prefers-color-scheme in real-time - FOUC prevention via inline script in <head> - Delayed CSS transition to avoid flash on initial load - Pure CSS icon switching via data-theme-mode attribute Closes #761 * fix: address review feedback and code improvements (takeover #853) - Fix dark-mode readability bug: .stepper-step.failed and .image-preview-remove used --text-on-accent (#09090b) on var(--danger) background, making text unreadable. Changed to --text-on-danger (#fff). - Restore hover visual feedback on .image-preview-remove:hover using filter: brightness(1.2) instead of redundant var(--danger). - Use const/let instead of var in theme-init.js for consistency with app.js (per gemini-code-assist review feedback). Co-Authored-By: CPU-216 <3125034290@stu.cpu.edu.cn> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address CI failures and Copilot review feedback (takeover #853) - Fix missing `fallback_deliverable` field in job_monitor test constructors (pre-existing staging issue surfaced by merge) - Validate localStorage theme value against whitelist in both theme-init.js and app.js to prevent broken state from invalid values - Add matchMedia addEventListener fallback for older Safari/WebKit - Add i18n keys for theme tooltip and aria-live announcement strings (en + zh-CN) to match existing localization patterns - Move .sr-only utility from inline <style> to style.css [skip-regression-check] Co-Authored-By: CPU-216 <3125034290@stu.cpu.edu.cn> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gao Zheng <3125034290@stu.cpu.edu.cn> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(web): add light theme with dark/light/system toggle (#761) Add three-state theme toggle (dark → light → system) to the Web Gateway: - Extract 101 hardcoded CSS colors into 30+ CSS custom properties - Add [data-theme='light'] overrides for all variables - Add theme toggle button in tab-bar (moon/sun/monitor icons) - Theme persists via localStorage, defaults to 'system' - System mode follows OS prefers-color-scheme in real-time - FOUC prevention via inline script in <head> - Delayed CSS transition to avoid flash on initial load - Pure CSS icon switching via data-theme-mode attribute Closes #761 * fix: address review feedback and code improvements (takeover #853) - Fix dark-mode readability bug: .stepper-step.failed and .image-preview-remove used --text-on-accent (#09090b) on var(--danger) background, making text unreadable. Changed to --text-on-danger (#fff). - Restore hover visual feedback on .image-preview-remove:hover using filter: brightness(1.2) instead of redundant var(--danger). - Use const/let instead of var in theme-init.js for consistency with app.js (per gemini-code-assist review feedback). Co-Authored-By: CPU-216 <3125034290@stu.cpu.edu.cn> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address CI failures and Copilot review feedback (takeover #853) - Fix missing `fallback_deliverable` field in job_monitor test constructors (pre-existing staging issue surfaced by merge) - Validate localStorage theme value against whitelist in both theme-init.js and app.js to prevent broken state from invalid values - Add matchMedia addEventListener fallback for older Safari/WebKit - Add i18n keys for theme tooltip and aria-live announcement strings (en + zh-CN) to match existing localization patterns - Move .sr-only utility from inline <style> to style.css [skip-regression-check] Co-Authored-By: CPU-216 <3125034290@stu.cpu.edu.cn> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gao Zheng <3125034290@stu.cpu.edu.cn> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat(web): add light theme with dark/light/system toggle (#761) Add three-state theme toggle (dark → light → system) to the Web Gateway: - Extract 101 hardcoded CSS colors into 30+ CSS custom properties - Add [data-theme='light'] overrides for all variables - Add theme toggle button in tab-bar (moon/sun/monitor icons) - Theme persists via localStorage, defaults to 'system' - System mode follows OS prefers-color-scheme in real-time - FOUC prevention via inline script in <head> - Delayed CSS transition to avoid flash on initial load - Pure CSS icon switching via data-theme-mode attribute Closes #761 * fix: address review feedback and code improvements (takeover #853) - Fix dark-mode readability bug: .stepper-step.failed and .image-preview-remove used --text-on-accent (#09090b) on var(--danger) background, making text unreadable. Changed to --text-on-danger (#fff). - Restore hover visual feedback on .image-preview-remove:hover using filter: brightness(1.2) instead of redundant var(--danger). - Use const/let instead of var in theme-init.js for consistency with app.js (per gemini-code-assist review feedback). Co-Authored-By: CPU-216 <3125034290@stu.cpu.edu.cn> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address CI failures and Copilot review feedback (takeover #853) - Fix missing `fallback_deliverable` field in job_monitor test constructors (pre-existing staging issue surfaced by merge) - Validate localStorage theme value against whitelist in both theme-init.js and app.js to prevent broken state from invalid values - Add matchMedia addEventListener fallback for older Safari/WebKit - Add i18n keys for theme tooltip and aria-live announcement strings (en + zh-CN) to match existing localization patterns - Move .sr-only utility from inline <style> to style.css [skip-regression-check] Co-Authored-By: CPU-216 <3125034290@stu.cpu.edu.cn> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gao Zheng <3125034290@stu.cpu.edu.cn> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Closes #761
Add a three-state theme toggle (dark → light → system) to the Web Gateway UI.
Changes
style.css:root[data-theme="light"]block overriding all variables with light-appropriate values.theme-toggle-btnstyles and CSS-only icon switching via[data-theme-mode]attribute.theme-transitionclass (delayed via JS to prevent load-time flash)index.html<script>in<head>— readslocalStorageand setsdata-theme+data-theme-modebefore first paintapp.jsgetThemeMode(),resolveTheme(),applyTheme(),toggleTheme()system(follows OSprefers-color-scheme)localStorage("ironclaw-theme")systemmoderequestAnimationFramex 2 to delay enabling CSS transitionsDesign Decisions
matchMedialistener[data-theme-mode]attribute on<html>+ CSS rules instead of JSstyle.displaymanipulationTesting