feat: Add GUI for label_colors in Dashboard Properties Modal#39434
feat: Add GUI for label_colors in Dashboard Properties Modal#39434jaymasiwal wants to merge 6 commits intoapache:masterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39434 +/- ##
==========================================
- Coverage 64.49% 64.49% -0.01%
==========================================
Files 2557 2558 +1
Lines 133097 133177 +80
Branches 30910 30946 +36
==========================================
+ Hits 85846 85895 +49
- Misses 45761 45792 +31
Partials 1490 1490
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dosubot
bot
added
change:frontend
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #a41682
Actionable Suggestions - 1
-
superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx - 1
- Broad Exception Handling · Line 87-91
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx - 2
- Type Safety Violation · Line 23-71
- Type Safety Violation · Line 133-136
Review Details
-
Files reviewed - 2 · Commit Range:
e6d79cf..26651e9- superset-frontend/src/dashboard/components/PropertiesModal/sections/AdvancedSection.tsx
- superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| try { | ||
| return jsonMetadata ? JSON.parse(jsonMetadata) : {}; | ||
| } catch (e) { | ||
| return {}; // Fallback if JSON is currently invalid in the AceEditor | ||
| } |
There was a problem hiding this comment.
Catch specific SyntaxError from JSON.parse instead of broad Exception to avoid masking other errors and improve error handling clarity.
Code suggestion
Check the AI-generated fix before applying
--- a/superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx
+++ b/superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx
@@ -86,7 +86,9 @@
const metadataObj = useMemo(() => {
try {
return jsonMetadata ? JSON.parse(jsonMetadata) : {};
- } catch (e) {
+ } catch (error: unknown) {
+ if (error instanceof SyntaxError) {
return {}; // Fallback if JSON is currently invalid in the AceEditor
+ }
+ throw error;
}
}, [jsonMetadata]);
Code Review Run #a41682
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
@jaymasiwal I have an idea for how you can work on this if you still want to. You could design the feature on your fork, but instead of submitting code at this stage for review, present screenshots of your implementation, any design decisions you made, and any remaining points you think need input. I think that will make it easier for people to weigh in, if they have something to react to. Then once we iterate on the UI and UX, the implementation and code review can follow. What do you think? |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| margin-bottom: ${({ theme }: any) => (theme?.gridUnit || 4) * 4}px; | ||
| padding: ${({ theme }: any) => (theme?.gridUnit || 4) * 4}px; | ||
| background-color: ${({ theme }: any) => theme?.colors?.grayscale?.light4 || '#f6f6f6'}; | ||
| border-radius: ${({ theme }: any) => theme?.borderRadius || 4}px; |
There was a problem hiding this comment.
Suggestion: Remove explicit any from the Container styled-component theme callbacks so the theme type is inferred instead of using an unsafe type. [custom_rule]
Severity Level: Minor
| margin-bottom: ${({ theme }: any) => (theme?.gridUnit || 4) * 4}px; | |
| padding: ${({ theme }: any) => (theme?.gridUnit || 4) * 4}px; | |
| background-color: ${({ theme }: any) => theme?.colors?.grayscale?.light4 || '#f6f6f6'}; | |
| border-radius: ${({ theme }: any) => theme?.borderRadius || 4}px; | |
| margin-bottom: ${({ theme }) => (theme?.gridUnit || 4) * 4}px; | |
| padding: ${({ theme }) => (theme?.gridUnit || 4) * 4}px; | |
| background-color: ${({ theme }) => theme?.colors?.grayscale?.light4 || '#f6f6f6'}; | |
| border-radius: ${({ theme }) => theme?.borderRadius || 4}px; |
Why it matters? 🤔
The existing code uses any in styled-component theme interpolations, which matches the custom rule concern.
The replacement removes the explicit any and relies on inferred typing, and the syntax is valid.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx
**Line:** 24:27
**Comment:**
*Custom Rule: Remove explicit `any` from the `Container` styled-component theme callbacks so the theme type is inferred instead of using an unsafe type.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| margin-bottom: ${({ theme }: any) => (theme?.gridUnit || 4) * 2}px; | ||
| gap: ${({ theme }: any) => (theme?.gridUnit || 4) * 2}px; | ||
| `; | ||
|
|
||
| const HeaderRow = styled.div` | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| margin-bottom: ${({ theme }: any) => (theme?.gridUnit || 4) * 4}px; |
There was a problem hiding this comment.
Suggestion: Remove any from the Row and HeaderRow theme interpolations to avoid unsafe typing in styled-components. [custom_rule]
Severity Level: Minor
| margin-bottom: ${({ theme }: any) => (theme?.gridUnit || 4) * 2}px; | |
| gap: ${({ theme }: any) => (theme?.gridUnit || 4) * 2}px; | |
| `; | |
| const HeaderRow = styled.div` | |
| display: flex; | |
| justify-content: space-between; | |
| align-items: center; | |
| margin-bottom: ${({ theme }: any) => (theme?.gridUnit || 4) * 4}px; | |
| margin-bottom: ${({ theme }) => (theme?.gridUnit || 4) * 2}px; | |
| gap: ${({ theme }) => (theme?.gridUnit || 4) * 2}px; | |
| `; | |
| const HeaderRow = styled.div` | |
| display: flex; | |
| justify-content: space-between; | |
| align-items: center; | |
| margin-bottom: ${({ theme }) => (theme?.gridUnit || 4) * 4}px; |
Why it matters? 🤔
The original code explicitly annotates the theme parameter as any in both components, so the rule violation is real.
Removing the annotation is a valid TypeScript/styled-components fix and does not change runtime behavior.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx
**Line:** 33:41
**Comment:**
*Custom Rule: Remove `any` from the `Row` and `HeaderRow` theme interpolations to avoid unsafe typing in styled-components.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| padding: ${({ theme }: any) => (theme?.gridUnit || 4) * 1.5}px ${({ theme }: any) => (theme?.gridUnit || 4) * 2}px; | ||
| font-size: 14px; | ||
| border-radius: ${({ theme }: any) => theme?.borderRadius || 4}px; | ||
| border: 1px solid ${({ theme }: any) => theme?.colors?.grayscale?.light2 || '#e0e0e0'}; | ||
| color: ${({ theme }: any) => theme?.colors?.grayscale?.dark1 || '#333333'}; | ||
| outline: none; | ||
|
|
||
| &:focus { | ||
| border-color: ${({ theme }: any) => theme?.colors?.primary?.base || '#20a7c9'}; | ||
| } | ||
| `; | ||
|
|
||
| const StyledColorInput = styled.input` | ||
| cursor: pointer; | ||
| height: 34px; | ||
| width: 40px; | ||
| padding: 0; | ||
| border: 1px solid ${({ theme }: any) => theme?.colors?.grayscale?.light2 || '#e0e0e0'}; | ||
| border-radius: ${({ theme }: any) => theme?.borderRadius || 4}px; | ||
| outline: none; |
There was a problem hiding this comment.
Suggestion: Remove any from StyledInput and StyledColorInput theme callback parameters to keep typing safe and consistent. [custom_rule]
Severity Level: Minor
| padding: ${({ theme }: any) => (theme?.gridUnit || 4) * 1.5}px ${({ theme }: any) => (theme?.gridUnit || 4) * 2}px; | |
| font-size: 14px; | |
| border-radius: ${({ theme }: any) => theme?.borderRadius || 4}px; | |
| border: 1px solid ${({ theme }: any) => theme?.colors?.grayscale?.light2 || '#e0e0e0'}; | |
| color: ${({ theme }: any) => theme?.colors?.grayscale?.dark1 || '#333333'}; | |
| outline: none; | |
| &:focus { | |
| border-color: ${({ theme }: any) => theme?.colors?.primary?.base || '#20a7c9'}; | |
| } | |
| `; | |
| const StyledColorInput = styled.input` | |
| cursor: pointer; | |
| height: 34px; | |
| width: 40px; | |
| padding: 0; | |
| border: 1px solid ${({ theme }: any) => theme?.colors?.grayscale?.light2 || '#e0e0e0'}; | |
| border-radius: ${({ theme }: any) => theme?.borderRadius || 4}px; | |
| outline: none; | |
| padding: ${({ theme }) => (theme?.gridUnit || 4) * 1.5}px ${({ theme }) => (theme?.gridUnit || 4) * 2}px; | |
| font-size: 14px; | |
| border-radius: ${({ theme }) => theme?.borderRadius || 4}px; | |
| border: 1px solid ${({ theme }) => theme?.colors?.grayscale?.light2 || '#e0e0e0'}; | |
| color: ${({ theme }) => theme?.colors?.grayscale?.dark1 || '#333333'}; | |
| outline: none; | |
| &:focus { | |
| border-color: ${({ theme }) => theme?.colors?.primary?.base || '#20a7c9'}; | |
| } | |
| `; | |
| const StyledColorInput = styled.input` | |
| cursor: pointer; | |
| height: 34px; | |
| width: 40px; | |
| padding: 0; | |
| border: 1px solid ${({ theme }) => theme?.colors?.grayscale?.light2 || '#e0e0e0'}; | |
| border-radius: ${({ theme }) => theme?.borderRadius || 4}px; |
Why it matters? 🤔
The existing code repeatedly uses any in the style interpolations, which is exactly the unsafe typing the suggestion targets.
The updated code removes those annotations and remains syntactically correct.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx
**Line:** 48:67
**Comment:**
*Custom Rule: Remove `any` from `StyledInput` and `StyledColorInput` theme callback parameters to keep typing safe and consistent.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| padding: ${({ theme }: any) => theme?.gridUnit || 4}px ${({ theme }: any) => (theme?.gridUnit || 4) * 3}px; | ||
| font-size: 12px; | ||
| font-weight: bold; | ||
| text-transform: uppercase; | ||
| cursor: pointer; | ||
| border: none; | ||
| border-radius: ${({ theme }: any) => theme?.borderRadius || 4}px; | ||
| background-color: ${({ theme, variant }: any) => | ||
| variant === 'danger' |
There was a problem hiding this comment.
Suggestion: Remove any from StyledButton callback parameters so style interpolation does not rely on an unsafe type. [custom_rule]
Severity Level: Minor
| padding: ${({ theme }: any) => theme?.gridUnit || 4}px ${({ theme }: any) => (theme?.gridUnit || 4) * 3}px; | |
| font-size: 12px; | |
| font-weight: bold; | |
| text-transform: uppercase; | |
| cursor: pointer; | |
| border: none; | |
| border-radius: ${({ theme }: any) => theme?.borderRadius || 4}px; | |
| background-color: ${({ theme, variant }: any) => | |
| variant === 'danger' | |
| padding: ${({ theme }) => theme?.gridUnit || 4}px ${({ theme }) => (theme?.gridUnit || 4) * 3}px; | |
| font-size: 12px; | |
| font-weight: bold; | |
| text-transform: uppercase; | |
| cursor: pointer; | |
| border: none; | |
| border-radius: ${({ theme }) => theme?.borderRadius || 4}px; | |
| background-color: ${({ theme, variant }) => |
Why it matters? 🤔
The original styled button uses any for both the theme and variant interpolation parameters, so it violates the unsafe typing rule.
The proposed change removes any and keeps the same styling logic, which is valid TypeScript.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx
**Line:** 83:91
**Comment:**
*Custom Rule: Remove `any` from `StyledButton` callback parameters so style interpolation does not rely on an unsafe type.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
| {colorEntries.map(([label, color], index) => ( | ||
| <Row key={`${label}-${index}`}> | ||
| <StyledInput | ||
| type="text" | ||
| value={label} | ||
| onChange={(e: any) => handleUpdate(label, e.target.value, color as string)} |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
Each mapping row uses a React key derived from the editable label (key={${label}-${index}}), so typing in the label changes the key on every keystroke, causing the row (and its inputs) to be remounted and breaking normal in-place label editing.
Suggestion: Track each mapping row with a stable internal id independent of the label text, and use that id as the React key while updating the label and color values as regular editable fields.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx
**Line:** 185:190
**Comment:**
*HIGH: Each mapping row uses a React key derived from the editable label (`key={`${label}-${index}`}`), so typing in the label changes the key on every keystroke, causing the row (and its inputs) to be remounted and breaking normal in-place label editing.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
| const metadataObj = useMemo(() => { | ||
| try { | ||
| return jsonMetadata ? JSON.parse(jsonMetadata) : {}; | ||
| } catch (error: unknown) { | ||
| if (error instanceof SyntaxError) { | ||
| return {}; | ||
| } | ||
| throw error; | ||
| } | ||
| }, [jsonMetadata]); | ||
|
|
||
| const labelColors = metadataObj.label_colors || {}; |
There was a problem hiding this comment.
Suggestion: JSON.parse can return null or non-object values (for example when the editor contains null), and metadataObj.label_colors will then throw at runtime. Normalize parsed metadata to a plain object before reading label_colors. [null pointer]
Severity Level: Major ⚠️
- ❌ Advanced settings tab crashes for primitive JSON metadata.
- ⚠️ Dashboard properties modal unusable until metadata manually fixed.| const metadataObj = useMemo(() => { | |
| try { | |
| return jsonMetadata ? JSON.parse(jsonMetadata) : {}; | |
| } catch (error: unknown) { | |
| if (error instanceof SyntaxError) { | |
| return {}; | |
| } | |
| throw error; | |
| } | |
| }, [jsonMetadata]); | |
| const labelColors = metadataObj.label_colors || {}; | |
| const metadataObj = useMemo<Record<string, unknown>>(() => { | |
| try { | |
| const parsed = jsonMetadata ? JSON.parse(jsonMetadata) : {}; | |
| return parsed && typeof parsed === 'object' && !Array.isArray(parsed) | |
| ? (parsed as Record<string, unknown>) | |
| : {}; | |
| } catch (error: unknown) { | |
| if (error instanceof SyntaxError) { | |
| return {}; | |
| } | |
| throw error; | |
| } | |
| }, [jsonMetadata]); | |
| const labelColors = | |
| metadataObj.label_colors && | |
| typeof metadataObj.label_colors === 'object' && | |
| !Array.isArray(metadataObj.label_colors) | |
| ? (metadataObj.label_colors as Record<string, string>) | |
| : {}; |
Steps of Reproduction ✅
1. Open any dashboard in edit mode and open the Properties modal; the advanced tab is
rendered by `PropertiesModal` at
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:825-830`, which
renders `<AdvancedSection jsonMetadata={jsonMetadata} ... />`.
2. `AdvancedSection` at
`superset-frontend/src/dashboard/components/PropertiesModal/sections/AdvancedSection.tsx:51-68`
passes the `jsonMetadata` string prop directly into `<LabelColorMapping
jsonMetadata={jsonMetadata} onJsonMetadataChange={onJsonMetadataChange} />`.
3. In the Advanced tab UI, use the JSON editor (rendered by `StyledEditorHost` in
`AdvancedSection.tsx:84-95`) to replace the metadata with a valid but non-object JSON
value, e.g. `null`, `[]`, or `"string"`. This updates the `jsonMetadata` state in
`PropertiesModal` via `onJsonMetadataChange`.
4. On re-render, `LabelColorMapping` at `LabelColorMapping.tsx:113-122` executes `const
metadataObj = useMemo(() => jsonMetadata ? JSON.parse(jsonMetadata) : {} ...)`; for
`jsonMetadata = "null"`, `metadataObj` is `null`, so `const labelColors =
metadataObj.label_colors || {};` at `LabelColorMapping.tsx:124` attempts to read
`.label_colors` of `null` (or a primitive), throwing a runtime error and breaking the
Advanced settings tab / modal.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx
**Line:** 113:124
**Comment:**
*Null Pointer: `JSON.parse` can return `null` or non-object values (for example when the editor contains `null`), and `metadataObj.label_colors` will then throw at runtime. Normalize parsed metadata to a plain object before reading `label_colors`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| }; | ||
|
|
||
| const handleAdd = () => { | ||
| const newColors = { ...labelColors, 'New Label': DEFAULT_NEW_COLOR }; |
There was a problem hiding this comment.
Suggestion: Adding always with the fixed key 'New Label' overwrites the previous unsaved mapping, so users cannot add multiple rows reliably. Generate a unique default key before inserting. [logic error]
Severity Level: Major ⚠️
- ⚠️ Users cannot add multiple mappings without renaming placeholder first.
- ⚠️ Label colors GUI behaves inconsistently when adding several mappings.| const newColors = { ...labelColors, 'New Label': DEFAULT_NEW_COLOR }; | |
| let labelIndex = 1; | |
| let newLabel = 'New Label'; | |
| while (Object.prototype.hasOwnProperty.call(labelColors, newLabel)) { | |
| labelIndex += 1; | |
| newLabel = `New Label ${labelIndex}`; | |
| } | |
| const newColors = { ...labelColors, [newLabel]: DEFAULT_NEW_COLOR }; |
Steps of Reproduction ✅
1. Open a dashboard's Properties modal and navigate to the Advanced tab so that
`AdvancedSection` renders `LabelColorMapping` (`AdvancedSection.tsx:64-68`), with
`jsonMetadata` initially set to an object (`PropertiesModal/index.tsx:179-38`).
2. Ensure there are no existing label color mappings so `labelColors` is `{}` and
`colorEntries.length === 0` in `LabelColorMapping.tsx:124-125`, then click the "Add
Mapping" button rendered at `LabelColorMapping.tsx:173-176`.
3. The click invokes `handleAdd` at `LabelColorMapping.tsx:150-152`, which constructs
`newColors = { ...labelColors, 'New Label': DEFAULT_NEW_COLOR }` and calls
`updateLabelColors`, resulting in a single row for `'New Label'` in the UI.
4. Click "Add Mapping" again without changing the existing `'New Label'` value;
`labelColors` now already contains `'New Label'`, so `handleAdd` again spreads
`labelColors` and sets the same fixed key `'New Label'`, producing an unchanged mapping
object and calling `updateLabelColors` with effectively the same data, meaning no
additional row appears and the user cannot add a second mapping until they manually rename
the first one.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/PropertiesModal/sections/LabelColorMapping.tsx
**Line:** 151:151
**Comment:**
*Logic Error: Adding always with the fixed key `'New Label'` overwrites the previous unsaved mapping, so users cannot add multiple rows reliably. Generate a unique default key before inserting.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hey @sfirke, thanks again for pointing me in the right direction! I spun up my fork locally and applied Superset's native design system (Emotion theme variables, grid units, and standard component styling) to build out the UI. I placed it directly inside the Advanced settings accordion right above the AceEditor, so users keep the context of the underlying JSON. Here is the proposed UX:
Key Design Decisions: Native Look & Feel: Mimicked the standard input, button, and typography styling from the src/components directory so it blends seamlessly with the rest of the modal. Auto-Sync: The GUI and the JSON editor are two-way bound. Editing the UI safely parses and updates the JSON without the user ever having to worry about syntax errors. Does this UX layout align with what you had in mind? I'm happy to tweak the layout or styling before I push the final React code to the branch! |
Code Review Agent Run #4b1d0dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
SUMMARY
Addresses the UX pain point of manually editing
label_colorsin JSON (as discussed on Slack). This adds a dynamic UI component to the Advanced settings tab that safely parses and updates thejson_metadata.Note: Opening as a Draft to let CI run before requesting review. I will swap the native HTML inputs to Superset's internal React components once the logic is validated by the CI pipeline.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
(Will add screenshots once CI passes and native UI components are swapped in)
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION