korbit-ai[bot] commented on code in PR #35663:
URL: https://github.com/apache/superset/pull/35663#discussion_r2433004450
##########
superset-frontend/src/pages/ThemeList/index.tsx:
##########
@@ -120,6 +120,30 @@ function ThemesList({
// Use Modal.useModal hook to ensure proper theming
const [modal, contextHolder] = Modal.useModal();
+ const clearAppliedTheme = useCallback(() => {
+ setAppliedThemeId(null);
+ try {
+ localStorage.removeItem('superset-applied-theme-id');
+ } catch (error) {
+ // Ignore localStorage errors
+ }
+ }, []);
+
+ useEffect(() => {
+ if (hasDevOverride()) {
+ try {
+ const storedThemeId =
localStorage.getItem('superset-applied-theme-id');
+ if (storedThemeId) {
+ setAppliedThemeId(parseInt(storedThemeId, 10));
+ }
+ } catch (error) {
+ clearAppliedTheme();
+ }
+ } else {
+ clearAppliedTheme();
+ }
+ }, [clearAppliedTheme, hasDevOverride]);
Review Comment:
### Unstable function dependency in useEffect <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The useEffect hook depends on hasDevOverride function which is likely not
memoized, causing unnecessary re-executions on every render.
###### Why this matters
This will trigger localStorage reads and state updates on every component
render, degrading performance especially when the component re-renders
frequently due to parent updates or other state changes.
###### Suggested change ∙ *Feature Preview*
Wrap hasDevOverride in useCallback in the ThemeProvider or use a stable
reference. Alternatively, if hasDevOverride returns a boolean value, consider
extracting that value into a separate variable:
```typescript
const devOverrideEnabled = hasDevOverride();
useEffect(() => {
if (devOverrideEnabled) {
// ... rest of logic
}
}, [clearAppliedTheme, devOverrideEnabled]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc186942-b3ea-43b2-9668-0ba6cc92e58d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc186942-b3ea-43b2-9668-0ba6cc92e58d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc186942-b3ea-43b2-9668-0ba6cc92e58d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc186942-b3ea-43b2-9668-0ba6cc92e58d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc186942-b3ea-43b2-9668-0ba6cc92e58d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d1ab0f23-ecdc-43c9-acd7-9d52f09ffb2f -->
[](d1ab0f23-ecdc-43c9-acd7-9d52f09ffb2f)
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -303,7 +303,12 @@ export class ThemeController {
public setThemeMode(mode: ThemeMode): void {
this.validateModeUpdatePermission(mode);
- if (this.currentMode === mode) return;
+ if (
+ this.currentMode === mode &&
+ !this.devThemeOverride &&
+ !this.crudThemeId
+ )
+ return;
Review Comment:
### Unnecessary property checks in early return condition <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The early return condition in setThemeMode now performs additional property
checks that may execute unnecessarily on every call, even when the mode hasn't
changed.
###### Why this matters
This adds computational overhead by checking devThemeOverride and
crudThemeId properties even in the common case where the mode is the same and
no override conditions exist. The original single comparison was more efficient
for the typical use case.
###### Suggested change ∙ *Feature Preview*
Restructure the condition to check the mode first, then only check override
conditions if the mode matches:
```typescript
if (this.currentMode === mode) {
if (!this.devThemeOverride && !this.crudThemeId) {
return;
}
}
```
This ensures the more expensive property checks only run when the mode
actually matches.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bea9052d-2b1b-4732-b903-7dc5c4395563/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bea9052d-2b1b-4732-b903-7dc5c4395563?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bea9052d-2b1b-4732-b903-7dc5c4395563?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bea9052d-2b1b-4732-b903-7dc5c4395563?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bea9052d-2b1b-4732-b903-7dc5c4395563)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:fef6cbbc-ee78-414a-9ffd-ea85e3c203eb -->
[](fef6cbbc-ee78-414a-9ffd-ea85e3c203eb)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
