korbit-ai[bot] commented on code in PR #33890:
URL: https://github.com/apache/superset/pull/33890#discussion_r2164889517
##########
superset-frontend/src/setup/setupApp.ts:
##########
@@ -59,6 +60,28 @@
);
}
+function syncBrowserThemePreferenceWithCookie() {
+ try {
+ const getCurrentPreference = () =>
+ window.matchMedia('(prefers-color-scheme: dark)').matches
Review Comment:
### Missing OS Theme Change Listener <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code doesn't set up a listener for OS theme changes, only checks the
initial state.
###### Why this matters
Users changing their OS theme while the application is running won't trigger
a theme update, breaking the intended synchronization functionality.
###### Suggested change ∙ *Feature Preview*
Add a MediaQueryList listener to handle OS theme changes:
```typescript
function syncBrowserThemePreferenceWithCookie() {
try {
const darkModeMediaQuery = window.matchMedia('(prefers-color-scheme:
dark)');
const handleThemeChange = (e: MediaQueryListEvent | MediaQueryList) => {
const newTheme = e.matches ? 'dark' : 'light';
const cookies = parseCookie();
if (cookies.superset_theme !== newTheme) {
document.cookie = `superset_theme=${newTheme}; path=/; SameSite=Lax;
secure`;
}
};
darkModeMediaQuery.addEventListener('change', handleThemeChange);
handleThemeChange(darkModeMediaQuery); // Handle initial state
} catch (err) {
console.warn('Failed to sync theme preference', err);
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd17281e-cbec-43ae-a5c3-a62d43606704/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd17281e-cbec-43ae-a5c3-a62d43606704?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd17281e-cbec-43ae-a5c3-a62d43606704?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd17281e-cbec-43ae-a5c3-a62d43606704?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bd17281e-cbec-43ae-a5c3-a62d43606704)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2bf2c876-5d45-4797-b03b-a4b492ce64cd -->
[](2bf2c876-5d45-4797-b03b-a4b492ce64cd)
##########
superset/views/base.py:
##########
@@ -292,6 +293,27 @@ def menu_data(user: User) -> dict[str, Any]:
}
+def get_theme_bootstrap_data() -> dict[str, Any]:
+ """
+ On frst call, the cookie related to light/dark may not be set,
Review Comment:
### Documentation Accuracy Issue <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Typo in docstring comment ('frst' instead of 'first') could cause confusion
in documentation generation and code comprehension.
###### Why this matters
While this doesn't affect functionality directly, incorrect documentation
can lead to misunderstanding of the code's behavior and purpose, potentially
causing implementation errors by other developers.
###### Suggested change ∙ *Feature Preview*
Correct the typo in the docstring:
```python
"""On first call, the cookie related to light/dark may not be set,"""
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/08ec21d4-34ac-4706-8c2e-a3ae295866c0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/08ec21d4-34ac-4706-8c2e-a3ae295866c0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/08ec21d4-34ac-4706-8c2e-a3ae295866c0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/08ec21d4-34ac-4706-8c2e-a3ae295866c0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/08ec21d4-34ac-4706-8c2e-a3ae295866c0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e13609ae-72c9-40a5-9111-4c429e5cf2f6 -->
[](e13609ae-72c9-40a5-9111-4c429e5cf2f6)
##########
superset/config.py:
##########
@@ -669,17 +669,17 @@ class D3TimeFormat(TypedDict, total=False):
# This is merely a default
EXTRA_CATEGORICAL_COLOR_SCHEMES: list[dict[str, Any]] = []
-# THEME is used for setting a custom theme to Superset, it follows the ant
design
-# theme structure
+# THEME and THEME_DARK are used for setting a custom theme to Superset,
+# it follows the ant design theme structure.
# You can use the AntDesign theme editor to generate a theme structure
# https://ant.design/theme-editor
-# To expose a JSON theme editor modal that can be triggered from the navbar
-# set the `ENABLE_THEME_EDITOR` feature flag to True.
-#
-# To set up the dark theme:
-# THEME = {"algorithm": "dark"}
+# The config are set as a callable returning an antd-compatible theme object
+# so that themes can be hot-swapped by fetching a theme object definition
remotely
-THEME: dict[str, Any] = {}
+# Whether to respect the user's OS dark mode setting. If True, THEME_DARK must
be set
+THEME_RESPECT_USER_OS_DARK_SETTING = False
+THEME: dict[str, Any] = lambda: {} # NOQA
+THEME_DARK: dict[str, Any] = lambda: {"algorithm": "dark"} # NOQA
Review Comment:
### Unnecessary Lambda Functions for Static Config Values <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using lambda functions for THEME and THEME_DARK configuration values adds
unnecessary complexity and potential confusion since lambdas are typically used
for dynamic values but these appear to be static.
###### Why this matters
Using lambda functions here makes the code less readable and adds a layer of
indirection without providing any clear benefit since the values being returned
are static dictionaries.
###### Suggested change ∙ *Feature Preview*
Replace lambdas with direct dictionary assignments:
```python
THEME: dict[str, Any] = {}
THEME_DARK: dict[str, Any] = {"algorithm": "dark"}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e3bec6f-8094-492b-bcd0-75a036f3faa8/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e3bec6f-8094-492b-bcd0-75a036f3faa8?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e3bec6f-8094-492b-bcd0-75a036f3faa8?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e3bec6f-8094-492b-bcd0-75a036f3faa8?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e3bec6f-8094-492b-bcd0-75a036f3faa8)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4f4079d4-0e6d-48f7-82a5-9cc8eb841204 -->
[](4f4079d4-0e6d-48f7-82a5-9cc8eb841204)
##########
superset-frontend/src/setup/setupApp.ts:
##########
@@ -59,6 +60,28 @@ function toggleCheckbox(apiUrlPrefix: string, selector:
string) {
);
}
+function syncBrowserThemePreferenceWithCookie() {
+ try {
+ const getCurrentPreference = () =>
+ window.matchMedia('(prefers-color-scheme: dark)').matches
+ ? 'dark'
+ : 'light';
+
+ const setThemeCookie = theme => {
+ document.cookie = `superset_theme=${theme}; path=/; SameSite=Lax;
secure`;
Review Comment:
### Unvalidated Cookie Value Setting <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The theme parameter is directly interpolated into the cookie string without
validation.
###### Why this matters
While the theme value is derived from system preferences in this case, the
setThemeCookie function could be reused elsewhere with untrusted input,
potentially leading to cookie manipulation.
###### Suggested change ∙ *Feature Preview*
```typescript
const setThemeCookie = (theme: 'dark' | 'light') => {
if (theme !== 'dark' && theme !== 'light') {
throw new Error('Invalid theme value');
}
document.cookie = `superset_theme=${theme}; path=/; SameSite=Lax;
secure`;
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7aa3bb0d-ea47-4e3f-9d9f-1372b1ea18dd/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7aa3bb0d-ea47-4e3f-9d9f-1372b1ea18dd?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7aa3bb0d-ea47-4e3f-9d9f-1372b1ea18dd?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7aa3bb0d-ea47-4e3f-9d9f-1372b1ea18dd?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7aa3bb0d-ea47-4e3f-9d9f-1372b1ea18dd)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:72e29f9d-331d-4631-8fcf-4118c5a710dc -->
[](72e29f9d-331d-4631-8fcf-4118c5a710dc)
##########
superset-frontend/src/setup/setupApp.ts:
##########
@@ -59,6 +60,28 @@
);
}
+function syncBrowserThemePreferenceWithCookie() {
+ try {
+ const getCurrentPreference = () =>
+ window.matchMedia('(prefers-color-scheme: dark)').matches
+ ? 'dark'
+ : 'light';
+
+ const setThemeCookie = theme => {
+ document.cookie = `superset_theme=${theme}; path=/; SameSite=Lax;
secure`;
+ };
+
+ const currentPreference = getCurrentPreference();
+ const cookies = parseCookie();
+
+ if (cookies.superset_theme !== currentPreference) {
+ setThemeCookie(currentPreference);
Review Comment:
### No UI Theme Update <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The theme change doesn't trigger a UI update, only sets the cookie.
###### Why this matters
Users won't see immediate theme changes in the application interface when
their OS theme changes.
###### Suggested change ∙ *Feature Preview*
Add a mechanism to update the UI theme immediately after setting the cookie:
```typescript
const setThemeCookie = (theme: string) => {
document.cookie = `superset_theme=${theme}; path=/; SameSite=Lax; secure`;
// Assuming there's a theme manager or event system
window.postMessage({ type: 'THEME_CHANGE', theme }, '*');
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b953dee-a289-4780-bb65-d80dd2066aee/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b953dee-a289-4780-bb65-d80dd2066aee?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b953dee-a289-4780-bb65-d80dd2066aee?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b953dee-a289-4780-bb65-d80dd2066aee?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6b953dee-a289-4780-bb65-d80dd2066aee)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:064ea0c6-8cb3-43fc-a4b5-72aab13448d6 -->
[](064ea0c6-8cb3-43fc-a4b5-72aab13448d6)
##########
superset-frontend/src/theme/ThemeController.tsx:
##########
@@ -103,8 +110,35 @@ export class ThemeController {
}
/**
- * Cleans up listeners. Should be called when the controller is no longer
needed.
+ * Load theme object directly from bootstrapData if not provided explicitly
+ * Note that there is special logic/handling to support getting the first
request
+ * where the backend doesn't know about user preferences yet.
+ * In that case, since the backend doesn't know about the user preferences,
+ * it will return both themes to the back as part of the bootstrap data,
leaving
+ * the decision to the frontend to pick the right one under
`bootstrapData.themes`
+ * once the backend knows about the user preferences, it will return only
`bootstrapData.theme`
+ * which takes precedence over `bootstrapData.themes` (not available in this
case)
*/
+ private static loadThemeFromBootstrap(): Theme {
+ const bootstrapData = getBootstrapData().common;
+
+ let themeConfig: AnyThemeConfig = {};
+
+ if (bootstrapData.theme) {
+ themeConfig = bootstrapData.theme;
+ } else if (bootstrapData.themes) {
+ const systemMode = ThemeController.getSystemMode();
+ const preferred = systemMode;
+ if (bootstrapData.themes[preferred]) {
+ themeConfig = bootstrapData.themes[preferred];
+ }
Review Comment:
### Missing Fallback Theme Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The loadThemeFromBootstrap method does not handle the case where the
preferred theme is not available in bootstrapData.themes, potentially leading
to an empty theme configuration.
###### Why this matters
If the preferred theme is not found, the application will continue with an
empty themeConfig object, which could cause rendering issues or unexpected
visual behavior.
###### Suggested change ∙ *Feature Preview*
Add fallback logic to use an alternative theme when preferred is not
available:
```typescript
if (bootstrapData.themes[preferred]) {
themeConfig = bootstrapData.themes[preferred];
} else if (bootstrapData.themes[ThemeMode.LIGHT]) {
themeConfig = bootstrapData.themes[ThemeMode.LIGHT];
console.warn('Preferred theme not found, falling back to light theme');
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8656db64-6302-4616-9ad1-213afc9a3a06/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8656db64-6302-4616-9ad1-213afc9a3a06?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8656db64-6302-4616-9ad1-213afc9a3a06?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8656db64-6302-4616-9ad1-213afc9a3a06?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8656db64-6302-4616-9ad1-213afc9a3a06)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8eb58959-a739-41b2-ab3f-b8acdc9d92fd -->
[](8eb58959-a739-41b2-ab3f-b8acdc9d92fd)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
