fix(dashboard): make JSON metadata editor resizable (#38728)#38745
fix(dashboard): make JSON metadata editor resizable (#38728)#38745Mayankaggarwal8055 wants to merge 4 commits intoapache:masterfrom
Conversation
Code Review Agent Run #06c616Actionable Suggestions - 0Additional Suggestions - 1
Review 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 |
dosubot
bot
added
dashboard:editmode
codeant-ai-for-open-source
bot
added
the
size:S
Sequence DiagramThis PR makes the dashboard properties modal resizable and draggable by passing new modal capabilities through StandardModal. It also makes the JSON metadata editor expand with the modal so larger configurations are easier to edit. sequenceDiagram
participant User
participant PropertiesModal
participant StandardModal
participant Modal
participant JsonEditor
User->>PropertiesModal: Open dashboard properties
PropertiesModal->>StandardModal: Render modal with resizable draggable and width 900
StandardModal->>Modal: Apply resizable draggable and larger body height
Modal-->>User: Show movable and resizable modal
User->>Modal: Resize modal for more space
Modal->>JsonEditor: Expand editor container and fill available height
JsonEditor-->>User: Display larger JSON editing area
Generated by CodeAnt AI |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const StyledModal = styled(Modal)` | ||
| .ant-modal-body { | ||
| max-height: 60vh; | ||
| max-height: 80vh; |
There was a problem hiding this comment.
Suggestion: The fixed max-height: 80vh on the modal body breaks the new resizable behavior: once users resize past that height, the body stops growing and leaves unusable empty space, so large JSON editors still cannot fully expand. Make the max-height conditional so it is only applied when the modal is not resizable. [logic error]
Severity Level: Critical 🚨
- ⚠️ Dashboard properties modal wastes space when resized taller.
- ⚠️ JSON metadata editor expansion is capped unexpectedly.| max-height: 80vh; | |
| max-height: ${({ resizable }) => (resizable ? 'none' : '80vh')}; |
Steps of Reproduction ✅
1. Open a dashboard in edit mode and click "Edit properties"; this action is wired in
`superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx:93-95`
(`MenuKeys.EditProperties -> showPropertiesModal()`), which toggles modal state in
`superset-frontend/src/dashboard/components/Header/index.tsx:512-513`.
2. Confirm the modal render path: `Header/index.tsx:844-850` conditionally renders
`<PropertiesModal ... show={showingPropertiesModal} />`, and
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:621-631` passes
`resizable` and `draggable` to `<StandardModal>`.
3. In `superset-frontend/src/components/Modal/StandardModal.tsx:54-56`, `.ant-modal-body`
is capped with `max-height: 80vh`; meanwhile core resizable logic sets body fill-height in
`superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:175-179`.
4. Drag-resize the properties modal vertically beyond ~80vh; the body stops growing
(capped), leaving unused space while advanced JSON editor container
(`superset-frontend/src/dashboard/components/PropertiesModal/sections/AdvancedSection.tsx:78-89`)
cannot fully expand with modal height.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Modal/StandardModal.tsx
**Line:** 55:55
**Comment:**
*Logic Error: The fixed `max-height: 80vh` on the modal body breaks the new resizable behavior: once users resize past that height, the body stops growing and leaves unusable empty space, so large JSON editors still cannot fully expand. Make the max-height conditional so it is only applied when the modal is not resizable.
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.| resizable | ||
| draggable |
There was a problem hiding this comment.
Suggestion: Enabling both drag/resize on this modal changes core modal behavior to remove the backdrop mask, so users can interact with the dashboard behind an open edit form and trigger conflicting state changes while unsaved properties are being edited. Keep this modal non-draggable/non-resizable to preserve modal isolation. [logic error]
Severity Level: Major ⚠️
- ⚠️ Dashboard header actions clickable during open properties modal.
- ⚠️ Unsaved properties can conflict with background dashboard edits.| resizable | |
| draggable |
Steps of Reproduction ✅
1. Enter dashboard edit mode, then open the header actions menu and click **Edit
properties**; this executes `showPropertiesModal()` in
`superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx:93-95`.
2. `showPropertiesModal` sets state in
`superset-frontend/src/dashboard/components/Header/index.tsx:512-514`, then
`PropertiesModal` is rendered at `Header/index.tsx:844-850`.
3. The modal is created with `resizable` and `draggable` in
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:629-631`.
4. Core modal logic computes `const shouldShowMask = !(resizable || draggable)` at
`superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:280` and
applies `mask={shouldShowMask}` at `Modal.tsx:360`, so mask becomes `false`.
5. While the properties modal stays open, click background dashboard controls like
`Discard`/`Save` (`superset-frontend/src/dashboard/components/Header/index.tsx:724-737`);
these remain clickable because no mask blocks background interaction.
6. Observe conflicting edit flow: background dashboard actions run while properties form
is still unsaved/open, breaking modal isolation.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
**Line:** 629:630
**Comment:**
*Logic Error: Enabling both drag/resize on this modal changes core modal behavior to remove the backdrop mask, so users can interact with the dashboard behind an open edit form and trigger conflicting state changes while unsaved properties are being edited. Keep this modal non-draggable/non-resizable to preserve modal isolation.
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.
Mayankaggarwal8055
force-pushed
the
fix/dashboard-json-editor
branch
from
c662236 to
9bd75a6
Compare
Mayankaggarwal8055
changed the title
| title | ||
| ) | ||
| } | ||
| $resizable={resizable} |
There was a problem hiding this comment.
wrongly duplicate resizable attribute here
There was a problem hiding this comment.
wrongly duplicate
resizableattribute here
Ah, good catch. That duplication wasn’t intentional—I’ll clean it up and make sure the prop is passed correctly.
| height="200px" | ||
| annotations={toEditorAnnotations(jsonAnnotations)} | ||
| /> | ||
| <div style={{ minHeight: 300, height: '60vh' }}> |
There was a problem hiding this comment.
why is this wrapped by a generic div?
There was a problem hiding this comment.
I added that wrapper to handle flexible height after removing the fixed 200px, so the editor could grow with the modal.
But yeah, I see your point—it’s not the cleanest approach. I’ll refactor this and handle the sizing more properly without the extra div.
Code Review Agent Run #ed4884Actionable Suggestions - 0Additional Suggestions - 1
Review 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 |
Mayankaggarwal8055
force-pushed
the
fix/dashboard-json-editor
branch
from
e1a03a8 to
4192515
Compare
|
Thanks for the feedback! I've pushed updates addressing the comments:
Please take another look when you get a chance. |
Mayankaggarwal8055
force-pushed
the
fix/dashboard-json-editor
branch
from
75d1ded to
25a1a2d
Compare
![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 #c9868a
Actionable Suggestions - 1
-
superset-frontend/src/components/Modal/StandardModal.tsx - 1
- Missing resizable prop forwarding · Line 44-44
Review Details
-
Files reviewed - 2 · Commit Range:
4192515..25a1a2d- superset-frontend/src/components/Modal/StandardModal.tsx
- superset-frontend/src/dashboard/components/PropertiesModal/sections/AdvancedSection.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
| maskClosable?: boolean; | ||
| wrapProps?: object; | ||
| contentLoading?: boolean; | ||
| resizable?: boolean; |
There was a problem hiding this comment.
The resizable prop has been added to the StandardModalProps interface, but it's not being passed to the underlying Modal component. Without passing resizable={resizable}, the modal will not be resizable even when the prop is set to true. The Modal component from @superset-ui/core supports resizing via the re-resizable library.
Code Review Run #c9868a
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
can you please follow up with Bito's review as well?
There was a problem hiding this comment.
The review suggests making max-height conditional with 80vh for non-resizable modals to fix the resizable behavior issue. The PR already applies conditional logic but uses 60vh; updating to 80vh matches the suggestion.
superset-frontend/src/components/Modal/StandardModal.tsx
const StyledModal = styled(Modal)<{ $resizable?: boolean }>`
.ant-modal-body {
max-height: ${({ $resizable }) => ($resizable ? 'none' : '80vh')};
height: auto;
overflow-y: auto;
padding: 0;
25a1a2d to
a190c58
Compare
codeant-ai-for-open-source
bot
added
size:M
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| title | ||
| ) | ||
| } | ||
| $resizable={resizable} |
There was a problem hiding this comment.
Suggestion: The resizable flag is only used for styling and is never forwarded to the underlying Modal component, so resize handles and resize behavior are never activated. Pass resizable through to StyledModal to enable the actual modal resizing logic. [logic error]
Severity Level: Critical 🚨
- ⚠️ Dashboard PropertiesModal resize handles never appear.
- ⚠️ Modal.tsx resizable wrapper path never executes.
- ⚠️ Large JSON editing remains constrained to static modal.| $resizable={resizable} | |
| resizable={resizable} |
Steps of Reproduction ✅
1. In dashboard header actions, click "Edit properties"; `handleMenuClick` routes
`MenuKeys.EditProperties` to `showPropertiesModal()` at
`superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx:34-36`.
2. `showPropertiesModal()` toggles modal state, then `Header` renders `<PropertiesModal
... show={showingPropertiesModal} />` at
`superset-frontend/src/dashboard/components/Header/index.tsx:25-35` (shown in returned JSX
block lines 25-35 of the read section).
3. `PropertiesModal` renders `<StandardModal ... resizable draggable ...>` at
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:22-32`, so resize is
expected on this path.
4. In `StandardModal`, only `$resizable={resizable}` is passed
(`superset-frontend/src/components/Modal/StandardModal.tsx:145`), but functional resize
logic in core `Modal` requires `resizable` prop
(`superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:225,339,346-348`);
result: no `Resizable` wrapper/handles appear, modal cannot be resized.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Modal/StandardModal.tsx
**Line:** 145:145
**Comment:**
*Logic Error: The `resizable` flag is only used for styling and is never forwarded to the underlying `Modal` component, so resize handles and resize behavior are never activated. Pass `resizable` through to `StyledModal` to enable the actual modal resizing logic.
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.
Mayankaggarwal8055
force-pushed
the
fix/dashboard-json-editor
branch
from
a190c58 to
8bb4ad5
Compare
Mayankaggarwal8055
requested review from
eschutho,
geido,
michael-s-molina,
rusackas and
villebro
as code owners
Mayankaggarwal8055
force-pushed
the
fix/dashboard-json-editor
branch
from
4dd45b5 to
bfaf3c7
Compare
Code Review Agent Run #0df75fActionable Suggestions - 0Additional Suggestions - 1
Review 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 |
|
Thanks again for all the feedback earlier @hainenber — I’ve gone through everything and pushed the fixes:
All checks are passing now ✅ Whenever you have a moment, could you take another look? Happy to tweak anything if needed. |
| show={show} | ||
| width={`${width}px`} | ||
| wrapProps={wrapProps} | ||
| $resizable={resizable} |
There was a problem hiding this comment.
This wrongly named$resizable attribute is still present.
| maskClosable?: boolean; | ||
| wrapProps?: object; | ||
| contentLoading?: boolean; | ||
| resizable?: boolean; |
There was a problem hiding this comment.
can you please follow up with Bito's review as well?
codeant-ai-for-open-source
bot
added
size:XS
| show={show} | ||
| width={`${width}px`} | ||
| wrapProps={wrapProps} | ||
| resizable={resizable} |
There was a problem hiding this comment.
Suggestion: The centered prop is defined with a default of true but never forwarded to the underlying modal, so dialogs are rendered with the base modal default instead of being centered as intended. [logic error]
Severity Level: Major ⚠️
- ⚠️ StandardModal dialogs ignore intended centered default behavior.
- ⚠️ Modal positioning inconsistent across dashboard and explore flows.| resizable={resizable} | |
| centered={centered} |
Steps of Reproduction ✅
1. Open any `StandardModal` consumer (e.g. dashboard properties at
`superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:621`, explore
properties at `superset-frontend/src/explore/components/PropertiesModal/index.tsx:324`,
refresh interval at
`superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:113`).
2. `StandardModal` sets `centered = true` by default in props destructuring at
`superset-frontend/src/components/Modal/StandardModal.tsx:115`.
3. The prop is never forwarded to underlying `Modal` (`StandardModal.tsx:125-136`), so
downstream `Modal` receives `centered` as undefined.
4. Core modal applies `centered={!!centered}` at
`superset-frontend/packages/superset-ui-core/src/components/Modal/Modal.tsx:119`, so
undefined becomes false and dialogs render non-centered.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Modal/StandardModal.tsx
**Line:** 135:135
**Comment:**
*Logic Error: The `centered` prop is defined with a default of `true` but never forwarded to the underlying modal, so dialogs are rendered with the base modal default instead of being centered as intended.
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.
Mayankaggarwal8055
force-pushed
the
fix/dashboard-json-editor
branch
from
31227eb to
82a74c8
Compare
Code Review Agent Run #4a2c95Actionable 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 |
Mayankaggarwal8055
force-pushed
the
fix/dashboard-json-editor
branch
from
4884323 to
bfaf3c7
Compare
Code Review Agent Run #a1126cActionable 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 |
|
Hi @hainenber, From what I can see in the logs, it looks like the failures are related to restricted GitHub Actions (like docker/setup-qemu-action, buildx, and login-action) not being allowed for forked PRs. All the other checks are passing successfully, so I just wanted to confirm — is this expected behavior for external contributors? And should this be ignored, or do maintainers need to trigger these workflows from their side? Happy to make any changes if I’m missing something here. |
|
I'm working to getting the CI unblocked. Please stand still until there are new developments. Edit: CI is unblocked, you can continue. Please also provide Before and After screenshots/videos as evident for your fix, not just this PR but also your others. |
2 participants
User description
Improves the usability of the JSON metadata editor in the dashboard properties modal.
Currently, the editor has a fixed height (200px), making it difficult to work with larger JSON configurations. Additionally, the modal itself is not resizable, further limiting usability.
CHANGES
resizableanddraggablepropsBEFORE
AFTER
TESTING
Fixes #38728
CodeAnt-AI Description
Make the dashboard JSON editor usable in a larger modal
What Changed
Impact
✅ Easier editing of large dashboard JSON✅ Fewer scroll interruptions in dashboard settings✅ More usable properties dialog💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.