-
Notifications
You must be signed in to change notification settings - Fork 572
Add configurable default theme support #4384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add configurable default theme support #4384
Conversation
Signed-off-by: Guillaume BERNARD <[email protected]>
Signed-off-by: Guillaume BERNARD <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: guillaumebernard84 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @guillaumebernard84 . I think this falls in the category of distributed settings that we'd like to have soon. See #3979 . @sniok , WDYT? |
skoeva
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see some errors in the CI, could you run these commands locally to try to fix those?
make frontend-testmake backend-lint-fix
Signed-off-by: Guillaume BERNARD <[email protected]>
|
Three fixes applied:
|
Yes, this could be an good solution too. I'll comment in #3979 |
|
/retest |
|
@guillaumebernard84: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/honk |
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hello @skoeva, can you please rerun the tests? Thanks |
| IsDynamicClusterEnabled bool `json:"isDynamicClusterEnabled"` | ||
| DefaultLightTheme string `json:"defaultLightTheme,omitempty"` | ||
| DefaultDarkTheme string `json:"defaultDarkTheme,omitempty"` | ||
| ForceTheme string `json:"forceTheme,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be a boolean? so that default light/dark theme can be forced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, it could.
It felt more natural for me to set --force-theme corporate-light instead of --default-light-theme corporate-light --force-theme
But that's a matter of taste. I can change that if you want.
|
TIL /honk lol :) |
illume
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this.
There's a GitHub check failing.
Can you please check the issue out locally?
npm run backend:lint
npm run backend:format
npm run backend:test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds runtime configuration for default themes via command-line arguments and environment variables, addressing feedback from PR #4307. The implementation allows administrators to configure default themes based on OS preference and to force a specific theme, overriding user preferences.
Changes:
- Adds three new CLI flags and corresponding environment variables for theme configuration (--default-light-theme, --default-dark-theme, --force-theme)
- Implements backend configuration parsing and API endpoints to pass theme configuration to the frontend
- Updates frontend theme selection logic to respect backend configuration with proper precedence order
- Adds UI indication and disables theme selection when a theme is forced by an administrator
- Includes comprehensive test coverage for both backend and frontend changes
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/pkg/config/config.go | Adds theme configuration flags and struct fields |
| backend/pkg/config/config_theme_test.go | Comprehensive backend tests for theme configuration parsing |
| backend/pkg/headlampconfig/headlampConfig.go | Extends HeadlampCFG with theme configuration fields |
| backend/cmd/server.go | Maps theme config from Config to HeadlampCFG |
| backend/cmd/headlamp.go | Updates clientConfig struct to include theme fields and getConfig endpoint |
| backend/cmd/stateless.go | Updates parseKubeConfig to include theme configuration |
| frontend/src/redux/configSlice.ts | Adds theme configuration to Redux state |
| frontend/src/lib/themes.ts | Implements theme precedence logic with backend configuration support |
| frontend/src/lib/themes.test.ts | Comprehensive frontend tests for theme selection logic |
| frontend/src/components/App/themeSlice.ts | Adds applyBackendThemeConfig action and exports ensureValidThemeName |
| frontend/src/components/App/Layout.tsx | Applies backend theme config when fetched from backend |
| frontend/src/plugin/index.ts | Reapplies backend theme config after plugins load |
| frontend/src/components/App/Settings/Settings.tsx | Disables theme selection UI and shows forced theme message |
| frontend/src/i18n/locales/*/translation.json | Adds localization for forced theme message |
| frontend/src/components/App/Settings/snapshots/Settings.General.stories.storyshot | Updates snapshot for UI styling changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Ok, this makes sense Co-authored-by: Copilot <[email protected]> Signed-off-by: Guillaume BERNARD <[email protected]>
Ok, this copilot suggestion makes sense Co-authored-by: Copilot <[email protected]> Signed-off-by: Guillaume BERNARD <[email protected]>
|
Hello @illume, I fixed the backend linter error, and accepted the two changes from the copilot review. It seems the third one from copilot encountered an error. Do you want to rerun it? I passed the frontend linter and tests after accepting the changes, it works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Theme config | ||
| DefaultLightTheme string `koanf:"default-light-theme"` | ||
| DefaultDarkTheme string `koanf:"default-dark-theme"` | ||
| ForceTheme string `koanf:"force-theme"` |
Copilot
AI
Feb 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no validation to check if both forceTheme and default themes are set together. While the code handles this scenario (forceTheme takes precedence), it would be more user-friendly to warn administrators if they set both forceTheme and defaultLightTheme/defaultDarkTheme, as the default themes will be ignored. Consider adding a validation warning or documentation about this precedence.
| applyBackendThemeConfig( | ||
| state, | ||
| action: PayloadAction<{ | ||
| defaultLightTheme?: string; | ||
| defaultDarkTheme?: string; | ||
| forceTheme?: string; | ||
| }> | ||
| ) { | ||
| const backendConfig = action.payload; | ||
| const newThemeName = getThemeName(backendConfig); | ||
|
|
||
| // Only update if theme has changed | ||
| if (newThemeName !== state.name) { | ||
| state.name = newThemeName; | ||
| // Only persist to localStorage if not forced | ||
| if (!backendConfig.forceTheme) { | ||
| setAppTheme(newThemeName); | ||
| } else { | ||
| // Clear any previously stored user theme preference when a forced theme is applied | ||
| try { | ||
| localStorage.removeItem('headlampThemePreference'); | ||
| } catch (e) {} | ||
| } | ||
| } | ||
| }, |
Copilot
AI
Feb 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The applyBackendThemeConfig action is exported and used in multiple places (Layout.tsx and plugin/index.ts) but there are no unit tests for this reducer action. While the getThemeName function has comprehensive tests, the Redux action's behavior (updating state, conditionally persisting to localStorage, handling theme changes) is not tested. Consider adding tests to verify that the action correctly handles forced themes, updates state appropriately, and clears localStorage when a forced theme is applied.
Summary
This PR adds runtime configuration for default themes via command-line arguments, addressing the feedback from #4307
Related Issue
Addresses feedback from #4307
Changes
Adds three new CLI flags:
--default-light-theme- Default theme when OS prefers light mode--default-dark-theme- Default theme when OS prefers dark mode--force-theme- Force specific theme (overrides user preference)Also available as environment variables:
HEADLAMP_CONFIG_DEFAULT_LIGHT_THEME,HEADLAMP_CONFIG_DEFAULT_DARK_THEME,HEADLAMP_CONFIG_FORCE_THEMEWhen a theme is forced, the theme selection menu is greyed out, and "Theme has been forced by your administrator" is displayed. I localized it to French as I'm a native French speaker.
Steps to Test
delete localStorage.headlampThemePreferencefrom your browser JS console--force-theme="Headlamp Classic"I also added unit tests:
go test ./pkg/config -run TestParseThemeConfigurationnpm test -- themes.test.ts