korbit-ai[bot] commented on code in PR #35349:
URL: https://github.com/apache/superset/pull/35349#discussion_r2393281075
##########
superset/config.py:
##########
@@ -566,6 +566,9 @@ class D3TimeFormat(TypedDict, total=False):
"CACHE_QUERY_BY_USER": False,
# Enable sharing charts with embedding
"EMBEDDABLE_CHARTS": True,
+ # Enhanced theme validation in theme editor with real-time token validation
+ # and partial theme loading for invalid tokens
+ "ENHANCED_THEME_VALIDATION": False,
Review Comment:
### Enhanced theme validation disabled by default <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The feature flag is set to False by default, which means the enhanced theme
validation functionality will be disabled by default, potentially contradicting
the developer's intent to provide robust validation.
###### Why this matters
Users and administrators may not be aware that this enhanced validation
feature exists since it's disabled by default, leading to continued use of less
robust theme validation without the benefits of token-level validation and
safer recovery mechanisms.
###### Suggested change ∙ *Feature Preview*
Consider setting the default value to True to enable the enhanced validation
by default, or ensure proper documentation exists to inform users about this
feature:
```python
"ENHANCED_THEME_VALIDATION": True,
```
Alternatively, if keeping it False by default is intentional for gradual
rollout, ensure the feature is well-documented in deployment guides.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45129f56-8027-4e77-a080-d39433e188e4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45129f56-8027-4e77-a080-d39433e188e4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45129f56-8027-4e77-a080-d39433e188e4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45129f56-8027-4e77-a080-d39433e188e4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45129f56-8027-4e77-a080-d39433e188e4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0e036085-7624-4586-8e04-28606d5061db -->
[](0e036085-7624-4586-8e04-28606d5061db)
##########
superset-frontend/src/features/themes/ThemeModal.tsx:
##########
@@ -222,20 +238,39 @@ const ThemeModal: FunctionComponent<ThemeModalProps> = ({
}
};
+ const isEmptyTheme = (jsonData?: string) => {
+ if (!jsonData?.trim()) return true;
+ try {
+ const parsed = JSON.parse(jsonData);
+ // Check if it's an empty object or array
+ return Object.keys(parsed).length === 0;
+ } catch (e) {
+ return false; // If it's not valid JSON, let other validation handle it
+ }
+ };
Review Comment:
### Repeated JSON parsing without memoization <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The isEmptyTheme function performs JSON.parse on every validation call
without memoization.
###### Why this matters
JSON parsing is computationally expensive and this function is called during
validation which happens on every render when theme data changes, leading to
redundant parsing operations.
###### Suggested change ∙ *Feature Preview*
Memoize the function using useCallback and consider caching the parsed
result:
```typescript
const isEmptyTheme = useCallback((jsonData?: string) => {
if (!jsonData?.trim()) return true;
try {
const parsed = JSON.parse(jsonData);
return Object.keys(parsed).length === 0;
} catch (e) {
return false;
}
}, []);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/322d810a-5e1c-4d0e-a223-41a6d5db5fd3/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/322d810a-5e1c-4d0e-a223-41a6d5db5fd3?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/322d810a-5e1c-4d0e-a223-41a6d5db5fd3?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/322d810a-5e1c-4d0e-a223-41a6d5db5fd3?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/322d810a-5e1c-4d0e-a223-41a6d5db5fd3)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e9277c8a-f7d5-487b-b739-beac791b9837 -->
[](e9277c8a-f7d5-487b-b739-beac791b9837)
##########
superset-frontend/src/features/themes/ThemeModal.tsx:
##########
@@ -115,12 +120,23 @@ const ThemeModal: FunctionComponent<ThemeModalProps> = ({
SupersetText?.THEME_MODAL?.DOCUMENTATION_URL ||
'https://superset.apache.org/docs/configuration/theming/';
- // JSON validation annotations using reusable hook
+ // Enhanced theme validation with feature flag support
+ const themeValidation = useThemeValidation(currentTheme?.json_data || '', {
+ enabled: !isReadOnly && Boolean(currentTheme?.json_data),
+ themeName: currentTheme?.theme_name || 'Unknown Theme',
+ });
Review Comment:
### Unnecessary validation hook execution <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The useThemeValidation hook is called unconditionally but only used when
enhanced validation is enabled, potentially causing unnecessary processing and
validation work.
###### Why this matters
This leads to wasted computational resources and potential side effects from
validation logic running even when the results won't be used, impacting
performance especially for large theme configurations.
###### Suggested change ∙ *Feature Preview*
Conditionally call the hook based on the enhanced validation flag:
```typescript
const themeValidation = useThemeValidation(
isEnhancedValidationEnabled ? currentTheme?.json_data || '' : '',
{
enabled: isEnhancedValidationEnabled && !isReadOnly &&
Boolean(currentTheme?.json_data),
themeName: currentTheme?.theme_name || 'Unknown Theme',
}
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/61effb87-8c1e-4ee7-bd78-8fe1008282c4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/61effb87-8c1e-4ee7-bd78-8fe1008282c4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/61effb87-8c1e-4ee7-bd78-8fe1008282c4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/61effb87-8c1e-4ee7-bd78-8fe1008282c4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/61effb87-8c1e-4ee7-bd78-8fe1008282c4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:dfd88427-7bcd-4aee-8343-f590b57fc1a1 -->
[](dfd88427-7bcd-4aee-8343-f590b57fc1a1)
##########
superset-frontend/src/theme/hooks/useThemeValidation.ts:
##########
@@ -0,0 +1,251 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useMemo, useState, useEffect } from 'react';
+import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+import { useJsonValidation } from
'@superset-ui/core/components/AsyncAceEditor';
+import type { JsonValidationAnnotation } from
'@superset-ui/core/components/AsyncAceEditor';
+import type { AnyThemeConfig } from '@superset-ui/core/theme/types';
+import {
+ validateThemeTokens,
+ formatValidationErrors,
+} from '../utils/themeTokenValidation';
+
+/**
+ * Find the line number where a specific token appears in JSON string
+ * Uses improved logic to handle nested objects and avoid false positives
+ */
+function findTokenLineInJson(jsonString: string, tokenName: string): number {
+ if (!jsonString || !tokenName) {
+ return 0;
+ }
+
+ const lines = jsonString.split('\n');
+
+ // Look for the token name as a JSON property key
+ const searchPattern = `"${tokenName}"`;
+
+ for (let i = 0; i < lines.length; i += 1) {
+ const line = lines[i].trim();
+
+ // Check if this line contains our token as a property key
+ // Pattern: "tokenName" followed by : (with possible whitespace)
+ const propertyPattern = new RegExp(
+ `"${tokenName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}"\\s*:`,
+ );
+
+ if (propertyPattern.test(line)) {
+ return i; // Return 0-based line number for AceEditor
+ }
+ }
+
+ // Fallback: simple string search for edge cases
+ for (let i = 0; i < lines.length; i += 1) {
+ if (lines[i].includes(searchPattern)) {
+ return i;
+ }
+ }
+
+ // If token not found, return line 0
+ return 0;
+}
+
+export interface ThemeValidationResult {
+ annotations: JsonValidationAnnotation[];
+ hasErrors: boolean;
+ hasWarnings: boolean;
+ validTokenCount: number;
+ invalidTokenCount: number;
+ errorMessages: string[];
+}
+
+export interface UseThemeValidationOptions {
+ /** Whether to enable validation. Default: true */
+ enabled?: boolean;
+ /** Custom error message prefix for JSON syntax errors. Default: 'Invalid
JSON syntax' */
+ jsonErrorPrefix?: string;
+ /** Custom error message prefix for theme token errors. Default: 'Invalid
theme token' */
+ tokenErrorPrefix?: string;
+ /** Theme name for error messages */
+ themeName?: string;
+ /** Debounce delay in milliseconds for validation. Default: 300 */
+ debounceMs?: number;
+}
+
+/**
+ * Enhanced theme validation hook that combines JSON syntax validation with
theme token validation.
+ * Uses feature flag to enable/disable enhanced token validation.
+ *
+ * @param jsonValue - The JSON string to validate
+ * @param options - Validation options
+ * @returns Enhanced validation result with annotations and metadata
+ */
+export function useThemeValidation(
+ jsonValue?: string,
+ options: UseThemeValidationOptions = {},
+): ThemeValidationResult {
+ const {
+ enabled = true,
+ jsonErrorPrefix = 'Invalid JSON syntax',
+ tokenErrorPrefix = 'Invalid theme token',
+ themeName,
+ debounceMs = 300,
+ } = options;
+
+ // Debounced JSON value for validation
+ const [debouncedJsonValue, setDebouncedJsonValue] = useState(jsonValue);
+
+ // Debounce the JSON value to avoid excessive validation calls
+ useEffect(() => {
+ const timer = setTimeout(() => {
+ setDebouncedJsonValue(jsonValue);
+ }, debounceMs);
+
+ return () => clearTimeout(timer);
+ }, [jsonValue, debounceMs]);
+
+ // Get basic JSON validation annotations
+ const jsonAnnotations = useJsonValidation(jsonValue, {
+ enabled,
+ errorPrefix: jsonErrorPrefix,
+ });
+
+ // Enhanced theme token validation (feature flag controlled)
+ const enhancedValidation = useMemo(() => {
+ // Skip if basic validation is disabled or JSON has syntax errors
+ if (!enabled || jsonAnnotations.length > 0 || !debouncedJsonValue?.trim())
{
+ return {
+ annotations: [],
+ validTokenCount: 0,
+ invalidTokenCount: 0,
+ errorMessages: [],
+ };
+ }
+
+ // Only run enhanced validation if feature flag is enabled
+ try {
+ const isEnabled = isFeatureEnabled(FeatureFlag.EnhancedThemeValidation);
+ if (!isEnabled) {
+ return {
+ annotations: [],
+ validTokenCount: 0,
+ invalidTokenCount: 0,
+ errorMessages: [],
+ };
+ }
+ } catch (error) {
+ // Feature flag check failed - assume disabled
+ return {
+ annotations: [],
+ validTokenCount: 0,
+ invalidTokenCount: 0,
+ errorMessages: [],
+ };
+ }
+
+ try {
+ const themeConfig: AnyThemeConfig = JSON.parse(debouncedJsonValue);
+
+ // Additional null safety check
+ if (!themeConfig || typeof themeConfig !== 'object') {
+ return {
+ annotations: [],
+ validTokenCount: 0,
+ invalidTokenCount: 0,
+ errorMessages: [],
+ };
+ }
+
+ const validationResult = validateThemeTokens(themeConfig);
+
+ const errorMessages = formatValidationErrors(
+ validationResult.errors,
+ themeName,
+ );
+
+ // Convert validation errors to AceEditor annotations with line mapping
+ const tokenAnnotations: JsonValidationAnnotation[] =
+ validationResult.errors.map(error => {
+ // Find the line where this token appears in the JSON
+ const tokenLine = findTokenLineInJson(
+ debouncedJsonValue,
+ error.tokenName,
+ );
+
+ return {
+ type: 'warning' as const, // Use warnings so users can still save
+ row: tokenLine,
+ column: 0,
+ text: `${tokenErrorPrefix}: ${error.message}`,
+ };
+ });
+
+ return {
+ annotations: tokenAnnotations,
+ validTokenCount: Object.keys(validationResult.validTokens ||
{}).length,
+ invalidTokenCount: Object.keys(validationResult.invalidTokens || {})
+ .length,
+ errorMessages,
+ };
+ } catch (error) {
+ // JSON parsing error should be caught by jsonAnnotations
+ return {
+ annotations: [],
+ validTokenCount: 0,
+ invalidTokenCount: 0,
+ errorMessages: [],
+ };
+ }
+ }, [
+ enabled,
+ debouncedJsonValue,
+ jsonAnnotations,
+ tokenErrorPrefix,
+ themeName,
+ ]);
+
+ return useMemo(() => {
+ const allAnnotations = [
+ ...jsonAnnotations,
+ ...enhancedValidation.annotations,
+ ];
+
+ return {
+ annotations: allAnnotations,
+ hasErrors: jsonAnnotations.some(ann => ann.type === 'error'),
+ hasWarnings: allAnnotations.some(ann => ann.type === 'warning'),
+ validTokenCount: enhancedValidation.validTokenCount,
+ invalidTokenCount: enhancedValidation.invalidTokenCount,
+ errorMessages: enhancedValidation.errorMessages,
+ };
+ }, [jsonAnnotations, enhancedValidation]);
+}
+
+/**
+ * Helper hook to check if enhanced theme validation is enabled
+ */
+export function useIsEnhancedValidationEnabled(): boolean {
+ return useMemo(() => {
+ try {
+ return isFeatureEnabled(FeatureFlag.EnhancedThemeValidation);
+ } catch (error) {
+ // Feature flag check failed - assume disabled
+ return false;
+ }
+ }, []);
+}
Review Comment:
### Duplicated Feature Flag Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The useIsEnhancedValidationEnabled hook duplicates feature flag checking
logic that already exists in the main useThemeValidation hook.
###### Why this matters
Having multiple implementations of the same feature flag check creates
maintenance overhead and potential inconsistencies in how feature flags are
handled.
###### Suggested change ∙ *Feature Preview*
Extract the feature flag check into a shared hook:
```typescript
export function useIsEnhancedValidationEnabled(): boolean {
return useMemo(() => {
try {
return isFeatureEnabled(FeatureFlag.EnhancedThemeValidation);
} catch {
return false;
}
}, []);
}
// Then use this hook in useThemeValidation
const isEnabled = useIsEnhancedValidationEnabled();
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c2940c56-fde0-4d89-bf9d-e9e535ae1597/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c2940c56-fde0-4d89-bf9d-e9e535ae1597?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c2940c56-fde0-4d89-bf9d-e9e535ae1597?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c2940c56-fde0-4d89-bf9d-e9e535ae1597?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c2940c56-fde0-4d89-bf9d-e9e535ae1597)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:762d3f85-885f-4d90-b6a0-d28f1d74c6a8 -->
[](762d3f85-885f-4d90-b6a0-d28f1d74c6a8)
##########
superset-frontend/src/features/themes/ThemeModal.tsx:
##########
@@ -222,20 +238,39 @@ const ThemeModal: FunctionComponent<ThemeModalProps> = ({
}
};
+ const isEmptyTheme = (jsonData?: string) => {
+ if (!jsonData?.trim()) return true;
+ try {
+ const parsed = JSON.parse(jsonData);
+ // Check if it's an empty object or array
+ return Object.keys(parsed).length === 0;
+ } catch (e) {
+ return false; // If it's not valid JSON, let other validation handle it
+ }
+ };
+
const validate = () => {
- if (isReadOnly) {
+ if (isReadOnly || !currentTheme) {
setDisableSave(true);
return;
}
- if (
- currentTheme?.theme_name.length &&
- currentTheme?.json_data?.length &&
- isValidJson(currentTheme.json_data)
- ) {
- setDisableSave(false);
+ const hasValidName = Boolean(currentTheme?.theme_name?.length);
+ const hasValidJsonData = Boolean(currentTheme?.json_data?.length);
+ const isValidJsonSyntax = isValidJson(currentTheme?.json_data);
+ const isNotEmpty = !isEmptyTheme(currentTheme?.json_data);
+
+ // Basic validation requirements
+ const basicValidation =
+ hasValidName && hasValidJsonData && isValidJsonSyntax && isNotEmpty;
+
+ if (isEnhancedValidationEnabled && currentTheme) {
+ // Enhanced validation: allow saving even with token warnings, but block
on JSON syntax errors
+ const enhancedValidation = basicValidation && !themeValidation.hasErrors;
+ setDisableSave(!enhancedValidation);
} else {
- setDisableSave(true);
+ // Original validation logic
+ setDisableSave(!basicValidation);
}
};
Review Comment:
### Validation Logic Not Separated from State Management <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The validation logic is mixed with state management and contains multiple
responsibilities within a single function.
###### Why this matters
This violates the Single Responsibility Principle and makes the code harder
to test and maintain. The validation rules are tightly coupled with the UI
state management.
###### Suggested change ∙ *Feature Preview*
Extract validation logic into a separate pure function:
```typescript
const isThemeValid = (theme: ThemeObject, options: { isEnhancedValidation:
boolean, hasValidationErrors: boolean }) => {
const basicValidation = isThemeBasicallyValid(theme);
return options.isEnhancedValidation
? basicValidation && !options.hasValidationErrors
: basicValidation;
};
const validate = () => {
if (isReadOnly || !currentTheme) {
setDisableSave(true);
return;
}
const isValid = isThemeValid(currentTheme, {
isEnhancedValidation: isEnhancedValidationEnabled,
hasValidationErrors: themeValidation.hasErrors
});
setDisableSave(!isValid);
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fc3a9f0-9738-495a-b412-42ffdb2c80fd/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fc3a9f0-9738-495a-b412-42ffdb2c80fd?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fc3a9f0-9738-495a-b412-42ffdb2c80fd?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fc3a9f0-9738-495a-b412-42ffdb2c80fd?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2fc3a9f0-9738-495a-b412-42ffdb2c80fd)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1a7074cc-93a2-4948-82be-225e1351e29b -->
[](1a7074cc-93a2-4948-82be-225e1351e29b)
##########
superset-frontend/src/theme/hooks/useThemeValidation.ts:
##########
@@ -0,0 +1,251 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useMemo, useState, useEffect } from 'react';
+import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+import { useJsonValidation } from
'@superset-ui/core/components/AsyncAceEditor';
+import type { JsonValidationAnnotation } from
'@superset-ui/core/components/AsyncAceEditor';
+import type { AnyThemeConfig } from '@superset-ui/core/theme/types';
+import {
+ validateThemeTokens,
+ formatValidationErrors,
+} from '../utils/themeTokenValidation';
+
+/**
+ * Find the line number where a specific token appears in JSON string
+ * Uses improved logic to handle nested objects and avoid false positives
+ */
+function findTokenLineInJson(jsonString: string, tokenName: string): number {
+ if (!jsonString || !tokenName) {
+ return 0;
+ }
+
+ const lines = jsonString.split('\n');
+
+ // Look for the token name as a JSON property key
+ const searchPattern = `"${tokenName}"`;
+
+ for (let i = 0; i < lines.length; i += 1) {
+ const line = lines[i].trim();
+
+ // Check if this line contains our token as a property key
+ // Pattern: "tokenName" followed by : (with possible whitespace)
+ const propertyPattern = new RegExp(
+ `"${tokenName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}"\\s*:`,
+ );
+
+ if (propertyPattern.test(line)) {
+ return i; // Return 0-based line number for AceEditor
+ }
+ }
+
+ // Fallback: simple string search for edge cases
+ for (let i = 0; i < lines.length; i += 1) {
+ if (lines[i].includes(searchPattern)) {
+ return i;
+ }
+ }
+
+ // If token not found, return line 0
+ return 0;
+}
+
+export interface ThemeValidationResult {
+ annotations: JsonValidationAnnotation[];
+ hasErrors: boolean;
+ hasWarnings: boolean;
+ validTokenCount: number;
+ invalidTokenCount: number;
+ errorMessages: string[];
+}
+
+export interface UseThemeValidationOptions {
+ /** Whether to enable validation. Default: true */
+ enabled?: boolean;
+ /** Custom error message prefix for JSON syntax errors. Default: 'Invalid
JSON syntax' */
+ jsonErrorPrefix?: string;
+ /** Custom error message prefix for theme token errors. Default: 'Invalid
theme token' */
+ tokenErrorPrefix?: string;
+ /** Theme name for error messages */
+ themeName?: string;
+ /** Debounce delay in milliseconds for validation. Default: 300 */
+ debounceMs?: number;
+}
+
+/**
+ * Enhanced theme validation hook that combines JSON syntax validation with
theme token validation.
+ * Uses feature flag to enable/disable enhanced token validation.
+ *
+ * @param jsonValue - The JSON string to validate
+ * @param options - Validation options
+ * @returns Enhanced validation result with annotations and metadata
+ */
+export function useThemeValidation(
+ jsonValue?: string,
+ options: UseThemeValidationOptions = {},
+): ThemeValidationResult {
+ const {
+ enabled = true,
+ jsonErrorPrefix = 'Invalid JSON syntax',
+ tokenErrorPrefix = 'Invalid theme token',
+ themeName,
+ debounceMs = 300,
+ } = options;
+
+ // Debounced JSON value for validation
+ const [debouncedJsonValue, setDebouncedJsonValue] = useState(jsonValue);
+
+ // Debounce the JSON value to avoid excessive validation calls
+ useEffect(() => {
+ const timer = setTimeout(() => {
+ setDebouncedJsonValue(jsonValue);
+ }, debounceMs);
+
+ return () => clearTimeout(timer);
+ }, [jsonValue, debounceMs]);
+
+ // Get basic JSON validation annotations
+ const jsonAnnotations = useJsonValidation(jsonValue, {
+ enabled,
+ errorPrefix: jsonErrorPrefix,
+ });
+
+ // Enhanced theme token validation (feature flag controlled)
+ const enhancedValidation = useMemo(() => {
+ // Skip if basic validation is disabled or JSON has syntax errors
+ if (!enabled || jsonAnnotations.length > 0 || !debouncedJsonValue?.trim())
{
+ return {
+ annotations: [],
+ validTokenCount: 0,
+ invalidTokenCount: 0,
+ errorMessages: [],
+ };
+ }
+
+ // Only run enhanced validation if feature flag is enabled
+ try {
+ const isEnabled = isFeatureEnabled(FeatureFlag.EnhancedThemeValidation);
+ if (!isEnabled) {
+ return {
+ annotations: [],
+ validTokenCount: 0,
+ invalidTokenCount: 0,
+ errorMessages: [],
+ };
+ }
+ } catch (error) {
+ // Feature flag check failed - assume disabled
+ return {
+ annotations: [],
+ validTokenCount: 0,
+ invalidTokenCount: 0,
+ errorMessages: [],
+ };
+ }
Review Comment:
### Repeated Default Object Structure <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code repeats the same default return object structure multiple times
within the enhancedValidation useMemo hook.
###### Why this matters
Duplicating the same object structure increases maintenance burden and risk
of inconsistency when the structure needs to be modified.
###### Suggested change ∙ *Feature Preview*
Extract the default object into a constant:
```typescript
const DEFAULT_VALIDATION_RESULT = {
annotations: [],
validTokenCount: 0,
invalidTokenCount: 0,
errorMessages: [],
} as const;
const enhancedValidation = useMemo(() => {
if (!enabled || jsonAnnotations.length > 0 || !debouncedJsonValue?.trim())
{
return DEFAULT_VALIDATION_RESULT;
}
// ... rest of the code using DEFAULT_VALIDATION_RESULT
}, [/*...*/]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a1042a66-8426-4b3b-91ac-0541e7c86ee4/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a1042a66-8426-4b3b-91ac-0541e7c86ee4?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a1042a66-8426-4b3b-91ac-0541e7c86ee4?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a1042a66-8426-4b3b-91ac-0541e7c86ee4?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a1042a66-8426-4b3b-91ac-0541e7c86ee4)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:33b43928-4b5c-4aa7-866a-bf3957f51493 -->
[](33b43928-4b5c-4aa7-866a-bf3957f51493)
##########
superset-frontend/src/theme/hooks/useThemeValidation.ts:
##########
@@ -0,0 +1,251 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useMemo, useState, useEffect } from 'react';
+import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+import { useJsonValidation } from
'@superset-ui/core/components/AsyncAceEditor';
+import type { JsonValidationAnnotation } from
'@superset-ui/core/components/AsyncAceEditor';
+import type { AnyThemeConfig } from '@superset-ui/core/theme/types';
+import {
+ validateThemeTokens,
+ formatValidationErrors,
+} from '../utils/themeTokenValidation';
+
+/**
+ * Find the line number where a specific token appears in JSON string
+ * Uses improved logic to handle nested objects and avoid false positives
+ */
+function findTokenLineInJson(jsonString: string, tokenName: string): number {
+ if (!jsonString || !tokenName) {
+ return 0;
+ }
+
+ const lines = jsonString.split('\n');
+
+ // Look for the token name as a JSON property key
+ const searchPattern = `"${tokenName}"`;
+
+ for (let i = 0; i < lines.length; i += 1) {
+ const line = lines[i].trim();
+
+ // Check if this line contains our token as a property key
+ // Pattern: "tokenName" followed by : (with possible whitespace)
+ const propertyPattern = new RegExp(
+ `"${tokenName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}"\\s*:`,
+ );
+
+ if (propertyPattern.test(line)) {
+ return i; // Return 0-based line number for AceEditor
+ }
+ }
+
+ // Fallback: simple string search for edge cases
+ for (let i = 0; i < lines.length; i += 1) {
+ if (lines[i].includes(searchPattern)) {
+ return i;
+ }
+ }
+
+ // If token not found, return line 0
+ return 0;
+}
+
+export interface ThemeValidationResult {
+ annotations: JsonValidationAnnotation[];
+ hasErrors: boolean;
+ hasWarnings: boolean;
+ validTokenCount: number;
+ invalidTokenCount: number;
+ errorMessages: string[];
+}
+
+export interface UseThemeValidationOptions {
+ /** Whether to enable validation. Default: true */
+ enabled?: boolean;
+ /** Custom error message prefix for JSON syntax errors. Default: 'Invalid
JSON syntax' */
+ jsonErrorPrefix?: string;
+ /** Custom error message prefix for theme token errors. Default: 'Invalid
theme token' */
+ tokenErrorPrefix?: string;
+ /** Theme name for error messages */
+ themeName?: string;
+ /** Debounce delay in milliseconds for validation. Default: 300 */
+ debounceMs?: number;
+}
+
+/**
+ * Enhanced theme validation hook that combines JSON syntax validation with
theme token validation.
+ * Uses feature flag to enable/disable enhanced token validation.
+ *
+ * @param jsonValue - The JSON string to validate
+ * @param options - Validation options
+ * @returns Enhanced validation result with annotations and metadata
+ */
+export function useThemeValidation(
+ jsonValue?: string,
+ options: UseThemeValidationOptions = {},
+): ThemeValidationResult {
+ const {
+ enabled = true,
+ jsonErrorPrefix = 'Invalid JSON syntax',
+ tokenErrorPrefix = 'Invalid theme token',
+ themeName,
+ debounceMs = 300,
+ } = options;
+
+ // Debounced JSON value for validation
+ const [debouncedJsonValue, setDebouncedJsonValue] = useState(jsonValue);
+
+ // Debounce the JSON value to avoid excessive validation calls
+ useEffect(() => {
+ const timer = setTimeout(() => {
+ setDebouncedJsonValue(jsonValue);
+ }, debounceMs);
+
+ return () => clearTimeout(timer);
+ }, [jsonValue, debounceMs]);
+
+ // Get basic JSON validation annotations
+ const jsonAnnotations = useJsonValidation(jsonValue, {
+ enabled,
+ errorPrefix: jsonErrorPrefix,
+ });
+
+ // Enhanced theme token validation (feature flag controlled)
+ const enhancedValidation = useMemo(() => {
+ // Skip if basic validation is disabled or JSON has syntax errors
+ if (!enabled || jsonAnnotations.length > 0 || !debouncedJsonValue?.trim())
{
+ return {
+ annotations: [],
+ validTokenCount: 0,
+ invalidTokenCount: 0,
+ errorMessages: [],
+ };
+ }
Review Comment:
### Validation results lost on JSON errors <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The validation logic skips enhanced validation when there are JSON syntax
errors, but this creates inconsistent behavior where token validation results
are lost when JSON errors are present.
###### Why this matters
Users may fix JSON syntax errors but lose visibility into token validation
issues that were previously detected, requiring them to re-trigger validation
to see token-specific problems.
###### Suggested change ∙ *Feature Preview*
Modify the logic to preserve token validation results even when JSON errors
exist:
```typescript
if (!enabled || !debouncedJsonValue?.trim()) {
return {
annotations: [],
validTokenCount: 0,
invalidTokenCount: 0,
errorMessages: [],
};
}
// Only skip token validation if JSON is unparseable, not just if
annotations exist
let themeConfig: AnyThemeConfig;
try {
themeConfig = JSON.parse(debouncedJsonValue);
} catch (error) {
// JSON parsing failed - skip token validation
return {
annotations: [],
validTokenCount: 0,
invalidTokenCount: 0,
errorMessages: [],
};
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/12f6fa04-575f-4e2f-8c38-6a1198589719/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/12f6fa04-575f-4e2f-8c38-6a1198589719?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/12f6fa04-575f-4e2f-8c38-6a1198589719?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/12f6fa04-575f-4e2f-8c38-6a1198589719?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/12f6fa04-575f-4e2f-8c38-6a1198589719)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e32c6bee-b5ef-4794-939a-c7b99b861014 -->
[](e32c6bee-b5ef-4794-939a-c7b99b861014)
##########
superset-frontend/src/theme/hooks/useThemeValidation.ts:
##########
@@ -0,0 +1,251 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useMemo, useState, useEffect } from 'react';
+import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core';
+import { useJsonValidation } from
'@superset-ui/core/components/AsyncAceEditor';
+import type { JsonValidationAnnotation } from
'@superset-ui/core/components/AsyncAceEditor';
+import type { AnyThemeConfig } from '@superset-ui/core/theme/types';
+import {
+ validateThemeTokens,
+ formatValidationErrors,
+} from '../utils/themeTokenValidation';
+
+/**
+ * Find the line number where a specific token appears in JSON string
+ * Uses improved logic to handle nested objects and avoid false positives
+ */
+function findTokenLineInJson(jsonString: string, tokenName: string): number {
+ if (!jsonString || !tokenName) {
+ return 0;
+ }
+
+ const lines = jsonString.split('\n');
+
+ // Look for the token name as a JSON property key
+ const searchPattern = `"${tokenName}"`;
+
+ for (let i = 0; i < lines.length; i += 1) {
+ const line = lines[i].trim();
+
+ // Check if this line contains our token as a property key
+ // Pattern: "tokenName" followed by : (with possible whitespace)
+ const propertyPattern = new RegExp(
+ `"${tokenName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}"\\s*:`,
+ );
Review Comment:
### Inefficient regex pattern creation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The regex pattern for finding token lines creates a new RegExp object on
every function call, which is inefficient and may cause performance issues with
frequent validation calls.
###### Why this matters
This creates unnecessary object allocation overhead and regex compilation
cost, especially problematic when validation runs frequently due to user typing
or debounced updates.
###### Suggested change ∙ *Feature Preview*
Cache the regex pattern or use a more efficient approach:
```typescript
function findTokenLineInJson(jsonString: string, tokenName: string): number {
if (!jsonString || !tokenName) {
return 0;
}
const lines = jsonString.split('\n');
const escapedTokenName = tokenName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const searchPattern = `"${escapedTokenName}"\s*:`;
for (let i = 0; i < lines.length; i += 1) {
const line = lines[i].trim();
if (line.match(searchPattern)) {
return i;
}
}
// Fallback: simple string search
const simplePattern = `"${tokenName}"`;
for (let i = 0; i < lines.length; i += 1) {
if (lines[i].includes(simplePattern)) {
return i;
}
}
return 0;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31233829-0431-4795-bb27-1426cebe8661/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31233829-0431-4795-bb27-1426cebe8661?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31233829-0431-4795-bb27-1426cebe8661?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31233829-0431-4795-bb27-1426cebe8661?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31233829-0431-4795-bb27-1426cebe8661)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2d9f2f93-e5cb-4a30-ac91-92078a2317cd -->
[](2d9f2f93-e5cb-4a30-ac91-92078a2317cd)
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -823,12 +855,89 @@ export class ThemeController {
}
const data = await response.json();
- const themeConfig = JSON.parse(data.result.json_data);
+ const rawThemeConfig = JSON.parse(data.result.json_data);
+ const themeName = data.result.theme_name || `Theme ${themeId}`;
+
+ if (!rawThemeConfig || typeof rawThemeConfig !== 'object') {
+ console.error(`Invalid theme configuration for theme: ${themeName}`);
+ return null;
+ }
- return themeConfig;
+ // Enhanced validation is behind a feature flag
+ if (isFeatureEnabled(FeatureFlag.EnhancedThemeValidation)) {
+ const validationResult = validateThemeTokens(rawThemeConfig);
+
+ if (validationResult.errors.length > 0) {
+ const errorMessages = formatValidationErrors(
+ validationResult.errors,
+ themeName,
+ );
+ errorMessages.forEach(message => {
+ console.warn(message);
+ });
+ }
+
+ const partialThemeConfig = getPartialThemeConfig(rawThemeConfig);
+
+ if (
+ Object.keys(partialThemeConfig.token || {}).length === 0 &&
+ validationResult.errors.length > 0
+ ) {
+ console.warn(
+ `Theme "${themeName}" has no valid tokens, falling back to system
default`,
+ );
+ return null;
+ }
+
+ return partialThemeConfig;
+ }
+
+ // Fallback to original behavior when feature flag is disabled
+ return rawThemeConfig;
} catch (error) {
console.error('Failed to fetch CRUD theme:', error);
return null;
}
}
+
+ /**
+ * Fetches a fresh system default theme from the API for runtime recovery.
+ * Tries multiple fallback strategies to find a valid theme.
+ * @returns The system default theme configuration or null if not found
+ */
+ private async fetchSystemDefaultTheme(): Promise<AnyThemeConfig | null> {
+ try {
+ // Try to fetch theme marked as system default (is_system_default=true)
+ const defaultResponse = await fetch(
+
'/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t)))',
+ );
+ if (defaultResponse.ok) {
+ const data = await defaultResponse.json();
+ if (data.result?.length > 0) {
+ const themeConfig = JSON.parse(data.result[0].json_data);
+ if (themeConfig && typeof themeConfig === 'object') {
+ return themeConfig;
+ }
+ }
+ }
+
+ // Fallback: Try to fetch system theme named 'THEME_DEFAULT'
+ const fallbackResponse = await fetch(
+
'/api/v1/theme/?q=(filters:!((col:theme_name,opr:eq,value:THEME_DEFAULT),(col:is_system,opr:eq,value:!t)))',
+ );
Review Comment:
### Sequential API calls in theme fallback logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The fallback mechanism makes two sequential API calls even when the first
call succeeds but returns an empty result set, creating unnecessary network
overhead.
###### Why this matters
When the first API call returns successfully but with no matching themes,
the code still makes a second API call. This doubles the network latency and
server load during theme recovery scenarios, which are already error conditions
that should be handled efficiently.
###### Suggested change ∙ *Feature Preview*
Combine both queries into a single API call using OR logic, or add an early
return when the first call indicates no themes exist:
```typescript
// Single API call with OR condition for both system default and named
fallback
const response = await fetch(
'/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t),(col:theme_name,opr:eq,value:THEME_DEFAULT)))',
);
if (response.ok) {
const data = await response.json();
if (data.result?.length > 0) {
// Prioritize system default if available
const systemDefault = data.result.find(theme => theme.is_system_default);
const theme = systemDefault || data.result[0];
const themeConfig = JSON.parse(theme.json_data);
if (themeConfig && typeof themeConfig === 'object') {
return themeConfig;
}
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ff792be-ca3f-40d1-8aa8-9819c0c0f502/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ff792be-ca3f-40d1-8aa8-9819c0c0f502?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ff792be-ca3f-40d1-8aa8-9819c0c0f502?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ff792be-ca3f-40d1-8aa8-9819c0c0f502?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7ff792be-ca3f-40d1-8aa8-9819c0c0f502)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a1bda6fa-6ef1-4cb5-a636-93de4ff441c6 -->
[](a1bda6fa-6ef1-4cb5-a636-93de4ff441c6)
##########
superset-frontend/src/theme/utils/themeTokenValidation.ts:
##########
@@ -0,0 +1,708 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { allowedAntdTokens } from '@superset-ui/core/theme/types';
+import type { AnyThemeConfig } from '@superset-ui/core/theme/types';
+
+type TokenValue = string | number | boolean;
+
+export interface ValidationError {
+ tokenName: string;
+ tokenValue: TokenValue;
+ category: TokenCategory;
+ errorType: ErrorType;
+ message: string;
+}
+
+export interface ValidationResult {
+ isValid: boolean;
+ errors: ValidationError[];
+ validTokens: Record<string, TokenValue>;
+ invalidTokens: Record<string, TokenValue>;
+}
+
+export enum TokenCategory {
+ COLOR = 'color',
+ SIZE = 'size',
+ FONT = 'font',
+ SHADOW = 'shadow',
+ BRAND = 'brand',
+ MOTION = 'motion',
+ NUMERIC = 'numeric',
+ BOOLEAN = 'boolean',
+ TEXT = 'text',
+ UNKNOWN = 'unknown',
+}
+
+export enum ErrorType {
+ InvalidColor = 'invalid_color',
+ InvalidSize = 'invalid_size',
+ InvalidFont = 'invalid_font',
+ InvalidShadow = 'invalid_shadow',
+ InvalidUrl = 'invalid_url',
+ InvalidMotion = 'invalid_motion',
+ InvalidNumeric = 'invalid_numeric',
+ InvalidBoolean = 'invalid_boolean',
+ InvalidString = 'invalid_string',
+ UnknownToken = 'unknown_token',
+ TypeMismatch = 'type_mismatch',
+}
+
+const COLOR_TOKENS = new Set([
+ 'colorError',
+ 'colorErrorActive',
+ 'colorErrorBg',
+ 'colorErrorBgActive',
+ 'colorErrorBgHover',
+ 'colorErrorBorder',
+ 'colorErrorBorderHover',
+ 'colorErrorHover',
+ 'colorErrorOutline',
+ 'colorErrorText',
+ 'colorErrorTextActive',
+ 'colorErrorTextHover',
+ 'colorPrimary',
+ 'colorPrimaryActive',
+ 'colorPrimaryBg',
+ 'colorPrimaryBgHover',
+ 'colorPrimaryBorder',
+ 'colorPrimaryBorderHover',
+ 'colorPrimaryHover',
+ 'colorPrimaryText',
+ 'colorPrimaryTextActive',
+ 'colorPrimaryTextHover',
+ 'colorSuccess',
+ 'colorSuccessActive',
+ 'colorSuccessBg',
+ 'colorSuccessBgHover',
+ 'colorSuccessBorder',
+ 'colorSuccessBorderHover',
+ 'colorSuccessHover',
+ 'colorSuccessText',
+ 'colorSuccessTextActive',
+ 'colorSuccessTextHover',
+ 'colorBgBase',
+ 'colorBgBlur',
+ 'colorBgContainer',
+ 'colorBgContainerDisabled',
+ 'colorBgElevated',
+ 'colorBgLayout',
+ 'colorBgMask',
+ 'colorBgSpotlight',
+ 'colorBgTextActive',
+ 'colorBgTextHover',
+ 'colorBorder',
+ 'colorBorderBg',
+ 'colorBorderSecondary',
+ 'colorFill',
+ 'colorFillAlter',
+ 'colorFillContent',
+ 'colorFillContentHover',
+ 'colorFillQuaternary',
+ 'colorFillSecondary',
+ 'colorFillTertiary',
+ 'colorHighlight',
+ 'colorIcon',
+ 'colorIconHover',
+ 'colorInfo',
+ 'colorInfoActive',
+ 'colorInfoBg',
+ 'colorInfoBgHover',
+ 'colorInfoBorder',
+ 'colorInfoBorderHover',
+ 'colorInfoHover',
+ 'colorInfoText',
+ 'colorInfoTextActive',
+ 'colorInfoTextHover',
+ 'colorLink',
+ 'colorLinkActive',
+ 'colorLinkHover',
+ 'colorSplit',
+ 'colorText',
+ 'colorTextBase',
+ 'colorTextDescription',
+ 'colorTextDisabled',
+ 'colorTextHeading',
+ 'colorTextLabel',
+ 'colorTextLightSolid',
+ 'colorTextPlaceholder',
+ 'colorTextQuaternary',
+ 'colorTextSecondary',
+ 'colorTextTertiary',
+ 'colorWarning',
+ 'colorWarningActive',
+ 'colorWarningBg',
+ 'colorWarningBgHover',
+ 'colorWarningBorder',
+ 'colorWarningBorderHover',
+ 'colorWarningHover',
+ 'colorWarningOutline',
+ 'colorWarningText',
+ 'colorWarningTextActive',
+ 'colorWarningTextHover',
+ 'colorWhite',
+ 'controlOutline',
+ 'controlTmpOutline',
+ 'controlItemBgActive',
+ 'controlItemBgActiveDisabled',
+ 'controlItemBgActiveHover',
+ 'controlItemBgHover',
+]);
+
+const SIZE_TOKENS = new Set([
+ 'borderRadius',
+ 'borderRadiusLG',
+ 'borderRadiusOuter',
+ 'borderRadiusSM',
+ 'borderRadiusXS',
+ 'controlHeight',
+ 'controlHeightLG',
+ 'controlHeightSM',
+ 'controlHeightXS',
+ 'controlInteractiveSize',
+ 'controlOutlineWidth',
+ 'controlPaddingHorizontal',
+ 'controlPaddingHorizontalSM',
+ 'fontHeight',
+ 'fontHeightLG',
+ 'fontHeightSM',
+ 'fontSize',
+ 'fontSizeHeading1',
+ 'fontSizeHeading2',
+ 'fontSizeHeading3',
+ 'fontSizeHeading4',
+ 'fontSizeHeading5',
+ 'fontSizeIcon',
+ 'fontSizeLG',
+ 'fontSizeSM',
+ 'fontSizeXL',
+ 'lineHeight',
+ 'lineHeightHeading1',
+ 'lineHeightHeading2',
+ 'lineHeightHeading3',
+ 'lineHeightHeading4',
+ 'lineHeightHeading5',
+ 'lineHeightLG',
+ 'lineHeightSM',
+ 'lineWidth',
+ 'lineWidthBold',
+ 'lineWidthFocus',
+ 'margin',
+ 'marginLG',
+ 'marginMD',
+ 'marginSM',
+ 'marginXL',
+ 'marginXS',
+ 'marginXXL',
+ 'marginXXS',
+ 'padding',
+ 'paddingContentHorizontal',
+ 'paddingContentHorizontalLG',
+ 'paddingContentHorizontalSM',
+ 'paddingContentVertical',
+ 'paddingContentVerticalLG',
+ 'paddingContentVerticalSM',
+ 'paddingLG',
+ 'paddingMD',
+ 'paddingSM',
+ 'paddingXL',
+ 'paddingXS',
+ 'paddingXXS',
+ 'screenLG',
+ 'screenLGMax',
+ 'screenLGMin',
+ 'screenMD',
+ 'screenMDMax',
+ 'screenMDMin',
+ 'screenSM',
+ 'screenSMMax',
+ 'screenSMMin',
+ 'screenXL',
+ 'screenXLMax',
+ 'screenXLMin',
+ 'screenXS',
+ 'screenXSMax',
+ 'screenXSMin',
+ 'screenXXL',
+ 'screenXXLMin',
+ 'size',
+ 'sizeLG',
+ 'sizeMD',
+ 'sizeMS',
+ 'sizePopupArrow',
+ 'sizeSM',
+ 'sizeStep',
+ 'sizeUnit',
+ 'sizeXL',
+ 'sizeXS',
+ 'sizeXXL',
+ 'sizeXXS',
+ 'fontSizeXS',
+ 'fontSizeXXL',
+ 'brandIconMaxWidth',
+ 'brandLogoHeight',
+]);
+
+const FONT_TOKENS = new Set([
+ 'fontFamily',
+ 'fontFamilyCode',
+ 'fontWeightStrong',
+ 'fontWeightNormal',
+ 'fontWeightLight',
+]);
+
+const SHADOW_TOKENS = new Set([
+ 'boxShadow',
+ 'boxShadowDrawerLeft',
+ 'boxShadowDrawerRight',
+ 'boxShadowDrawerUp',
+ 'boxShadowPopoverArrow',
+ 'boxShadowSecondary',
+ 'boxShadowTabsOverflowBottom',
+ 'boxShadowTabsOverflowLeft',
+ 'boxShadowTabsOverflowRight',
+ 'boxShadowTabsOverflowTop',
+ 'boxShadowTertiary',
+]);
+
+const BRAND_TOKENS = new Set([
+ 'brandLogoAlt',
+ 'brandLogoUrl',
+ 'brandLogoMargin',
+ 'brandLogoHref',
+ 'brandSpinnerUrl',
+ 'brandSpinnerSvg',
+]);
+
+const MOTION_TOKENS = new Set([
+ 'motion',
+ 'motionBase',
+ 'motionDurationFast',
+ 'motionDurationMid',
+ 'motionDurationSlow',
+ 'motionEaseInBack',
+ 'motionEaseInOut',
+ 'motionEaseInOutCirc',
+ 'motionEaseInQuint',
+ 'motionEaseOut',
+ 'motionEaseOutBack',
+ 'motionEaseOutCirc',
+ 'motionEaseOutQuint',
+ 'motionUnit',
+]);
+
+const NUMERIC_TOKENS = new Set([
+ 'opacityImage',
+ 'opacityLoading',
+ 'zIndexBase',
+ 'zIndexPopupBase',
+]);
+
+const BOOLEAN_TOKENS = new Set(['wireframe']);
+
+const TEXT_TOKENS = new Set([
+ 'lineType',
+ 'linkDecoration',
+ 'linkFocusDecoration',
+ 'linkHoverDecoration',
+]);
+
+function getTokenCategory(tokenName: string): TokenCategory {
+ if (COLOR_TOKENS.has(tokenName)) return TokenCategory.COLOR;
+ if (SIZE_TOKENS.has(tokenName)) return TokenCategory.SIZE;
+ if (FONT_TOKENS.has(tokenName)) return TokenCategory.FONT;
+ if (SHADOW_TOKENS.has(tokenName)) return TokenCategory.SHADOW;
+ if (BRAND_TOKENS.has(tokenName)) return TokenCategory.BRAND;
+ if (MOTION_TOKENS.has(tokenName)) return TokenCategory.MOTION;
+ if (NUMERIC_TOKENS.has(tokenName)) return TokenCategory.NUMERIC;
+ if (BOOLEAN_TOKENS.has(tokenName)) return TokenCategory.BOOLEAN;
+ if (TEXT_TOKENS.has(tokenName)) return TokenCategory.TEXT;
+ return TokenCategory.UNKNOWN;
+}
+
+function isValidColor(value: TokenValue): boolean {
+ if (typeof value !== 'string') return false;
+
+ const hexPattern = /^#([0-9A-Fa-f]{3}|[0-9A-Fa-f]{6}|[0-9A-Fa-f]{8})$/;
+ if (hexPattern.test(value)) return true;
+
+ const rgbPattern = /^rgb\(\s*(\d+)\s*,\s*(\d+)\s*,\s*(\d+)\s*\)$/;
+ const rgbaPattern =
+ /^rgba\(\s*(\d+)\s*,\s*(\d+)\s*,\s*(\d+)\s*,\s*(0|1|0?\.\d+)\s*\)$/;
+ if (rgbPattern.test(value) || rgbaPattern.test(value)) {
+ const matches = value.match(/\d+(\.\d+)?/g);
+ if (matches) {
+ const nums = matches.map(Number);
+ return (
+ nums[0] <= 255 &&
+ nums[1] <= 255 &&
+ nums[2] <= 255 &&
+ (nums.length === 3 || (nums[3] >= 0 && nums[3] <= 1))
+ );
+ }
+ }
+
+ const hslPattern = /^hsl\(\s*(\d+)\s*,\s*(\d+)%\s*,\s*(\d+)%\s*\)$/;
+ const hslaPattern =
+ /^hsla\(\s*(\d+)\s*,\s*(\d+)%\s*,\s*(\d+)%\s*,\s*(0|1|0?\.\d+)\s*\)$/;
+ if (hslPattern.test(value) || hslaPattern.test(value)) {
+ const matches = value.match(/\d+(\.\d+)?/g);
+ if (matches) {
+ const nums = matches.map(Number);
+ return (
+ nums[0] <= 360 &&
+ nums[1] <= 100 &&
+ nums[2] <= 100 &&
+ (nums.length === 3 || (nums[3] >= 0 && nums[3] <= 1))
+ );
+ }
+ }
+
+ const namedColors = new Set([
+ 'transparent',
+ 'inherit',
+ 'currentColor',
+ 'initial',
+ 'unset',
+ 'black',
+ 'white',
+ 'red',
+ 'green',
+ 'blue',
+ 'yellow',
+ 'cyan',
+ 'magenta',
+ 'gray',
+ 'grey',
+ 'darkgray',
+ 'darkgrey',
+ 'lightgray',
+ 'lightgrey',
+ 'orange',
+ 'purple',
+ 'brown',
+ 'pink',
+ 'lime',
+ 'navy',
+ 'teal',
+ 'silver',
+ 'maroon',
+ 'olive',
+ 'aqua',
+ 'fuchsia',
+ ]);
Review Comment:
### Inefficient Set recreation in color validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The namedColors Set is recreated on every call to isValidColor function
instead of being defined as a module-level constant.
###### Why this matters
This causes unnecessary memory allocation and Set construction overhead for
each color validation, which could be significant when validating many theme
tokens.
###### Suggested change ∙ *Feature Preview*
Move the namedColors Set declaration outside the function as a module-level
constant:
```typescript
const NAMED_COLORS = new Set([
'transparent',
'inherit',
// ... rest of colors
]);
function isValidColor(value: TokenValue): boolean {
// ... other validation logic
return NAMED_COLORS.has(value.toLowerCase());
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/777c4703-7fbc-4c65-87a7-c251def11b8f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/777c4703-7fbc-4c65-87a7-c251def11b8f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/777c4703-7fbc-4c65-87a7-c251def11b8f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/777c4703-7fbc-4c65-87a7-c251def11b8f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/777c4703-7fbc-4c65-87a7-c251def11b8f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d8ae0c7c-710f-4f1d-8e50-6bec1733c87e -->
[](d8ae0c7c-710f-4f1d-8e50-6bec1733c87e)
--
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]
