fix(view query): Update style for code viewer container#39635
Conversation
Co-authored-by: Copilot <copilot@github.com>
Code Review Agent Run #e81fe5Actionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| flex: 1, | ||
| marginBottom: theme.sizeUnit * 3, | ||
| fontSize: theme.fontSize * 0.75, | ||
| padding: 0, |
There was a problem hiding this comment.
Suggestion: Removing all syntax block padding causes the absolutely positioned copy button to overlap the rendered SQL text, which can hide the beginning of lines and interfere with text readability/click selection. Keep top/right padding (or add explicit right padding) to reserve space for the overlay button. [possible bug]
Severity Level: Major ⚠️
- ❌ ViewQuery modal copy button overlaps SQL content.
- ⚠️ SQL text partially obscured, harder to read.
- ⚠️ Text selection interfered near top-right code area.
- ⚠️ Affects Explore and Dashboard view-query flows.Steps of Reproduction ✅
1. From the Explore UI, open the "View query" modal via the "View query" menu option,
wired in `src/explore/components/useExploreAdditionalActionsMenu/index.tsx:5–25` where
`ModalTrigger` renders `<ViewQueryModal latestQueryFormData={latestQueryFormData as
QueryFormData} />` as its `modalBody`.
2. `ViewQueryModal` (`src/explore/components/controls/ViewQueryModal.tsx:44–49, 91–108`)
maps the backend `result` array and, for each `item.query`, renders the `ViewQuery`
component, so any SQL query displayed in this modal goes through `ViewQuery`.
3. In `ViewQuery` (`src/explore/components/controls/ViewQuery.tsx:160–177`), the SQL is
rendered with `<StyledThemedSyntaxHighlighter language={language} customStyle={{ flex: 1,
marginBottom: theme.sizeUnit * 3, fontSize: theme.fontSize * 0.75, padding: 0, }}>`, where
the `padding: 0` line at 172 overrides the default padding of `CodeSyntaxHighlighter`.
4. The `CodeSyntaxHighlighter` implementation
(`packages/superset-ui-core/src/components/CodeSyntaxHighlighter/index.tsx:121–127,
129–160, 185–201`) builds `defaultCustomStyle` with `padding: theme.sizeUnit * 4` but then
spreads `customStyle` last (line 126), so `padding: 0` removes all top/right padding. The
component also renders an internal copy button absolutely positioned at the top-right
(`position: absolute; top: ${theme.sizeUnit}px; right: ${theme.sizeUnit}px;` with its own
padding, lines 129–152). With no padding on the code block, the first lines of SQL extend
fully into the top-right corner under this absolute button, causing the overlay copy
control to sit on top of the rendered SQL text and interfere with readability and mouse
selection in both Explore and Dashboard "View query" modals (Dashboard wiring via
`src/dashboard/components/SliceHeaderControls/index.tsx:7–23`).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/explore/components/controls/ViewQuery.tsx
**Line:** 172:172
**Comment:**
*Possible Bug: Removing all syntax block padding causes the absolutely positioned copy button to overlap the rendered SQL text, which can hide the beginning of lines and interfere with text readability/click selection. Keep top/right padding (or add explicit right padding) to reserve space for the overlay button.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39635 +/- ##
=======================================
Coverage 64.51% 64.52%
=======================================
Files 2557 2557
Lines 133049 133053 +4
Branches 30901 30901
=======================================
+ Hits 85843 85847 +4
Misses 45716 45716
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:
|
jaymasiwal
left a comment
jaymasiwal
left a comment
There was a problem hiding this comment.
Great catch by the bot on the overlap issue. Instead of padding: 0, should we use something like padding: theme.sizeUnit * 2 or paddingRight: theme.sizeUnit * 10? This would keep the text clear of the absolute positioned copy button.
Also, for the font size, theme.fontSize * 0.75 (~10.5px) might be a bit too small for readability. Would it be better to use a standard token like theme.typography.sizes.s (12px) to stay consistent with other SQL viewers in the app?
Lastly, curious about the -8 * theme.sizeUnit margin in HighlightedSql. Is that to offset a specific parent container's padding, or could we achieve the same look by adjusting the StyledTabs header directly?
michael-s-molina
moved this from In Progress
to Non-blockers
in Superset 6.1.0 Release Bugs
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
LGTM... though the sizeUnit * 26 height seems a bit... specific. Should we do something based on modal/viewport height?
|
@justinpark should we merge this or do you wanna make the number less "magic"? |
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
4 participants
SUMMARY
Following up #39075, this commit updates additional styling for sql code viewers
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Check stying for view query modal
ADDITIONAL INFORMATION