feat(web): add light theme with dark/light/system toggle#1457
feat(web): add light theme with dark/light/system toggle#1457ilblackdragon merged 6 commits intostagingfrom
Conversation
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
# Conflicts: # src/channels/web/static/style.css
- 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>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Pull request overview
Adds a three-state (dark/light/system) theme system to the Web Gateway UI, including light theme variable overrides and a pre-paint initialization script to prevent theme flash on load.
Changes:
- Introduces a synchronous
theme-init.jsloaded in<head>to apply the saved/system theme before first paint. - Refactors many hardcoded colors into CSS custom properties and adds a
[data-theme="light"]override block. - Adds a theme toggle button to the tab bar and client-side theme management logic in
app.js, plus a new static route fortheme-init.js.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/channels/web/static/theme-init.js | Pre-paint theme initialization based on persisted mode / system preference |
| src/channels/web/static/style.css | CSS variable extraction, light theme overrides, toggle button + transition styling |
| src/channels/web/static/index.html | Loads theme-init.js synchronously; adds toggle button + aria-live announcer |
| src/channels/web/static/app.js | Implements theme mode cycling/persistence and system-theme change handling |
| src/channels/web/server.rs | Serves theme-init.js via an embedded static handler |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/channels/web/static/app.js
Outdated
| // Listen for OS theme changes — only re-apply when in 'system' mode. | ||
| window.matchMedia('(prefers-color-scheme: light)').addEventListener('change', function() { | ||
| if (getThemeMode() === 'system') { | ||
| applyTheme('system'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
matchMedia(...).addEventListener('change', ...) can throw on browsers that still expose MediaQueryList.addListener/removeListener but not addEventListener (notably older Safari/WebKit). Since an exception here would break all subsequent JS initialization, add a compatibility fallback (feature-detect addEventListener vs addListener) and ideally reuse a single MediaQueryList instance.
| function getThemeMode() { | ||
| return localStorage.getItem('ironclaw-theme') || 'system'; | ||
| } | ||
|
|
||
| function resolveTheme(mode) { | ||
| return mode === 'system' ? getSystemTheme() : mode; | ||
| } |
There was a problem hiding this comment.
Theme mode read from localStorage is used without validation. If the stored value is anything other than dark/light/system, this will set data-theme/data-theme-mode to unexpected values and the UI can end up in a broken/unspecified theme state. Consider normalizing/whitelisting the mode and falling back to system (or dark) when the stored value is invalid.
src/channels/web/static/app.js
Outdated
| 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] || ''; | ||
| const announce = document.getElementById('theme-announce'); | ||
| if (announce) announce.textContent = 'Theme: ' + mode; |
There was a problem hiding this comment.
New user-visible strings for the theme button (tooltip titles and aria-live announcement) are hardcoded in English. Since the rest of this file consistently uses I18n.t(...) for UI copy, consider moving these strings into the i18n tables so non-English users get localized tooltips/announcements.
| 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] || ''; | |
| const announce = document.getElementById('theme-announce'); | |
| if (announce) announce.textContent = 'Theme: ' + mode; | |
| const titles = { | |
| dark: I18n.t('theme.button.tooltip.dark'), | |
| light: I18n.t('theme.button.tooltip.light'), | |
| system: I18n.t('theme.button.tooltip.system') | |
| }; | |
| const btn = document.getElementById('theme-toggle'); | |
| if (btn) btn.title = titles[mode] || ''; | |
| const announceTexts = { | |
| dark: I18n.t('theme.announcement.dark'), | |
| light: I18n.t('theme.announcement.light'), | |
| system: I18n.t('theme.announcement.system') | |
| }; | |
| const announce = document.getElementById('theme-announce'); | |
| if (announce) announce.textContent = announceTexts[mode] || ''; |
| 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); |
There was a problem hiding this comment.
theme-init.js uses the persisted theme mode value directly as both data-theme and data-theme-mode. If localStorage contains an unexpected value, the page can render without matching theme variables (and with no icon shown). Consider whitelisting dark/light/system and falling back to system (or dark) when the stored value is invalid.
| .tools-table { | ||
| width: 100%; | ||
| border-collapse: collapse; | ||
| } | ||
|
|
||
| .tools-table th, | ||
| .tools-table td { | ||
| padding: 8px 12px; | ||
| text-align: left; | ||
| border-bottom: 1px solid var(--border); | ||
| font-size: 13px; | ||
| } | ||
|
|
||
| .tools-table th { | ||
| color: var(--text-secondary); | ||
| font-weight: 500; | ||
| text-transform: uppercase; | ||
| font-size: 11px; | ||
| letter-spacing: 0.5px; | ||
| } | ||
|
|
||
| .tools-table tr:hover td { | ||
| background: var(--hover-surface); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
A new .tools-table styling block is added, but there are no references to tools-table in the HTML/JS in this PR (and a repo-wide search under static/ only finds it in this CSS). If this table isn’t actually rendered anymore, consider removing these rules to avoid accumulating dead CSS; otherwise, add/point to the corresponding markup so the styles are exercised.
| .tools-table { | |
| width: 100%; | |
| border-collapse: collapse; | |
| } | |
| .tools-table th, | |
| .tools-table td { | |
| padding: 8px 12px; | |
| text-align: left; | |
| border-bottom: 1px solid var(--border); | |
| font-size: 13px; | |
| } | |
| .tools-table th { | |
| color: var(--text-secondary); | |
| font-weight: 500; | |
| text-transform: uppercase; | |
| font-size: 11px; | |
| letter-spacing: 0.5px; | |
| } | |
| .tools-table tr:hover td { | |
| background: var(--hover-surface); | |
| } |
src/channels/web/static/index.html
Outdated
| <style>.sr-only{position:absolute;width:1px;height:1px;padding:0;margin:-1px;overflow:hidden;clip:rect(0,0,0,0);white-space:nowrap;border:0}</style> | ||
| <script src="/theme-init.js"></script> |
There was a problem hiding this comment.
The new .sr-only utility is injected as a minified inline <style> in the document head. Since it’s a general-purpose utility class (and the app already has a main stylesheet), consider moving it into style.css for maintainability and to keep all styling in one place.
…ight-theme-toggle
- 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>
Summary
Continuation of #853 by @CPU-216.
Adds a three-state theme toggle (dark / light / system) to the Web Gateway UI. Extracts ~100 hardcoded CSS color values into 30+ CSS custom properties, adds a complete
[data-theme="light"]override block, and implements FOUC prevention via a synchronous init script. Pure CSS + vanilla JS, no new dependencies.Changes from original
.stepper-step.failed .stepper-circleand.image-preview-removeused--text-on-accent(near-black) onvar(--danger)(red) background — changed to--text-on-danger(#fff).image-preview-remove:hoverusingfilter: brightness(1.2)vartoconst/letintheme-init.jsfor consistency withapp.jsOriginal PR
#853 - feat(web): add light theme with dark/light/system toggle
Review comments addressed
--text-on-dangervariable. Extended the fix to two additional elements (stepper circle, image remove button) that had the same class of bug.!importanton*transition selector (gemini-code-assist, zmanian): Already removed by author.aria-live(zmanian): Already added by author (#theme-announcespan).vartoconst/let(gemini-code-assist): Already fixed inapp.jsby author. Extended totheme-init.js.Test plan
cargo fmtcleancargo clippy --all --all-featurescleanCo-Authored-By: CPU-216 3125034290@stu.cpu.edu.cn
Generated with Claude Code