You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ma...@apache.org on 2026/04/10 19:11:13 UTC
(superset) branch master updated: fix(SqlLab): improve SQL diff modal — responsive width, padding, tabs, and copy button (#39246)
This is an automated email from the ASF dual-hosted git repository.
maximebeauchemin pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 450701ecec3 fix(SqlLab): improve SQL diff modal — responsive width, padding, tabs, and copy button (#39246)
450701ecec3 is described below
commit 450701ecec37773d1fbccf42abad805c3b8f0725
Author: Maxime Beauchemin <ma...@gmail.com>
AuthorDate: Fri Apr 10 12:11:05 2026 -0700
fix(SqlLab): improve SQL diff modal — responsive width, padding, tabs, and copy button (#39246)
Co-authored-by: Claude Opus 4.6 <no...@anthropic.com>
---
.../CodeSyntaxHighlighter/index.test.tsx | 42 +++++++-
.../src/components/CodeSyntaxHighlighter/index.tsx | 107 +++++++++++++++++----
.../superset-ui-core}/src/utils/copy.ts | 5 +-
.../packages/superset-ui-core/src/utils/index.ts | 1 +
.../HighlightedSql/HighlightedSql.test.tsx | 37 ++++++-
.../src/SqlLab/components/HighlightedSql/index.tsx | 61 ++++++++----
.../src/features/queries/SyntaxHighlighterCopy.tsx | 6 +-
superset-frontend/src/utils/copy.ts | 75 +--------------
8 files changed, 212 insertions(+), 122 deletions(-)
diff --git a/superset-frontend/packages/superset-ui-core/src/components/CodeSyntaxHighlighter/index.test.tsx b/superset-frontend/packages/superset-ui-core/src/components/CodeSyntaxHighlighter/index.test.tsx
index 8734447c302..c70da8df1c7 100644
--- a/superset-frontend/packages/superset-ui-core/src/components/CodeSyntaxHighlighter/index.test.tsx
+++ b/superset-frontend/packages/superset-ui-core/src/components/CodeSyntaxHighlighter/index.test.tsx
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { render, screen } from '../../spec';
+import { render, screen, fireEvent } from '../../spec';
import CodeSyntaxHighlighter from './index';
// Simple mock that just returns the content
@@ -153,4 +153,44 @@ describe('CodeSyntaxHighlighter', () => {
expect(screen.getByText('SELECT * FROM users;')).toBeInTheDocument();
});
+
+ test('shows copy button by default', () => {
+ render(
+ <CodeSyntaxHighlighter language="sql">SELECT 1;</CodeSyntaxHighlighter>,
+ );
+
+ expect(screen.getByTitle('Copy to clipboard')).toBeInTheDocument();
+ });
+
+ test('hides copy button when showCopyButton is false', () => {
+ render(
+ <CodeSyntaxHighlighter language="sql" showCopyButton={false}>
+ SELECT 1;
+ </CodeSyntaxHighlighter>,
+ );
+
+ expect(screen.queryByTitle('Copy to clipboard')).not.toBeInTheDocument();
+ });
+
+ test('copy button does not throw when clipboard API is unavailable', () => {
+ const originalClipboard = navigator.clipboard;
+ Object.defineProperty(navigator, 'clipboard', {
+ value: undefined,
+ configurable: true,
+ });
+ document.execCommand = jest.fn().mockReturnValue(true);
+
+ render(
+ <CodeSyntaxHighlighter language="sql">SELECT 1;</CodeSyntaxHighlighter>,
+ );
+
+ expect(() =>
+ fireEvent.click(screen.getByTitle('Copy to clipboard')),
+ ).not.toThrow();
+
+ Object.defineProperty(navigator, 'clipboard', {
+ value: originalClipboard,
+ configurable: true,
+ });
+ });
});
diff --git a/superset-frontend/packages/superset-ui-core/src/components/CodeSyntaxHighlighter/index.tsx b/superset-frontend/packages/superset-ui-core/src/components/CodeSyntaxHighlighter/index.tsx
index 131abad4304..29a035885f4 100644
--- a/superset-frontend/packages/superset-ui-core/src/components/CodeSyntaxHighlighter/index.tsx
+++ b/superset-frontend/packages/superset-ui-core/src/components/CodeSyntaxHighlighter/index.tsx
@@ -16,11 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { useEffect, useState } from 'react';
+import { useCallback, useEffect, useRef, useState } from 'react';
import SyntaxHighlighterBase from 'react-syntax-highlighter/dist/cjs/light';
import github from 'react-syntax-highlighter/dist/cjs/styles/hljs/github';
import tomorrow from 'react-syntax-highlighter/dist/cjs/styles/hljs/tomorrow-night';
-import { isThemeDark, useTheme } from '@apache-superset/core/theme';
+import { css, isThemeDark, useTheme } from '@apache-superset/core/theme';
+import { t } from '@apache-superset/core/translation';
+import copyTextToClipboard from '../../utils/copy';
+import { Icons } from '../Icons';
export type SupportedLanguage = 'sql' | 'htmlbars' | 'markdown' | 'json';
@@ -31,6 +34,7 @@ export interface CodeSyntaxHighlighterProps {
showLineNumbers?: boolean;
wrapLines?: boolean;
style?: any; // Override theme style if needed
+ showCopyButton?: boolean;
}
// Track which languages have been registered to avoid duplicate registrations
@@ -76,11 +80,14 @@ export const CodeSyntaxHighlighter: React.FC<CodeSyntaxHighlighterProps> = ({
showLineNumbers = false,
wrapLines = true,
style: overrideStyle,
+ showCopyButton = true,
}) => {
const theme = useTheme();
const [isLanguageReady, setIsLanguageReady] = useState(
registeredLanguages.has(language),
);
+ const [copied, setCopied] = useState(false);
+ const copyTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
useEffect(() => {
const loadLanguage = async () => {
@@ -93,6 +100,21 @@ export const CodeSyntaxHighlighter: React.FC<CodeSyntaxHighlighterProps> = ({
loadLanguage();
}, [language]);
+ useEffect(
+ () => () => {
+ if (copyTimeoutRef.current) clearTimeout(copyTimeoutRef.current);
+ },
+ [],
+ );
+
+ const handleCopy = useCallback(() => {
+ copyTextToClipboard(() => Promise.resolve(children)).then(() => {
+ if (copyTimeoutRef.current) clearTimeout(copyTimeoutRef.current);
+ setCopied(true);
+ copyTimeoutRef.current = setTimeout(() => setCopied(false), 1500);
+ });
+ }, [children]);
+
const isDark = isThemeDark(theme);
const themeStyle = overrideStyle || (isDark ? tomorrow : github);
@@ -104,32 +126,79 @@ export const CodeSyntaxHighlighter: React.FC<CodeSyntaxHighlighterProps> = ({
...customStyle,
};
+ const copyButton = showCopyButton && (
+ <button
+ type="button"
+ onClick={(e: React.MouseEvent) => {
+ e.stopPropagation();
+ handleCopy();
+ }}
+ title={copied ? t('Copied!') : t('Copy to clipboard')}
+ css={css`
+ position: absolute;
+ top: ${theme.sizeUnit}px;
+ right: ${theme.sizeUnit}px;
+ background: none;
+ border: none;
+ cursor: pointer;
+ padding: ${theme.sizeUnit}px;
+ color: ${copied ? theme.colorSuccess : theme.colorTextSecondary};
+ line-height: 1;
+ border-radius: ${theme.borderRadius}px;
+ &:hover {
+ color: ${copied ? theme.colorSuccess : theme.colorText};
+ background: ${theme.colorBgTextHover};
+ }
+ `}
+ >
+ {copied ? (
+ <Icons.CheckOutlined style={{ fontSize: theme.fontSizeSM }} />
+ ) : (
+ <Icons.CopyOutlined style={{ fontSize: theme.fontSizeSM }} />
+ )}
+ </button>
+ );
+
// Show a simple pre-formatted text while language is loading
if (!isLanguageReady) {
return (
- <pre
- style={{
- ...defaultCustomStyle,
- fontFamily: 'monospace',
- whiteSpace: 'pre-wrap',
- margin: 0,
- }}
+ <div
+ css={css`
+ position: relative;
+ `}
>
- {children}
- </pre>
+ {copyButton}
+ <pre
+ style={{
+ ...defaultCustomStyle,
+ fontFamily: 'monospace',
+ whiteSpace: 'pre-wrap',
+ margin: 0,
+ }}
+ >
+ {children}
+ </pre>
+ </div>
);
}
return (
- <SyntaxHighlighterBase
- language={language}
- style={themeStyle}
- customStyle={defaultCustomStyle}
- showLineNumbers={showLineNumbers}
- wrapLines={wrapLines}
+ <div
+ css={css`
+ position: relative;
+ `}
>
- {children}
- </SyntaxHighlighterBase>
+ {copyButton}
+ <SyntaxHighlighterBase
+ language={language}
+ style={themeStyle}
+ customStyle={defaultCustomStyle}
+ showLineNumbers={showLineNumbers}
+ wrapLines={wrapLines}
+ >
+ {children}
+ </SyntaxHighlighterBase>
+ </div>
);
};
diff --git a/superset-frontend/src/utils/copy.ts b/superset-frontend/packages/superset-ui-core/src/utils/copy.ts
similarity index 95%
copy from superset-frontend/src/utils/copy.ts
copy to superset-frontend/packages/superset-ui-core/src/utils/copy.ts
index 0980f2ab170..f39f5be0b47 100644
--- a/superset-frontend/src/utils/copy.ts
+++ b/superset-frontend/packages/superset-ui-core/src/utils/copy.ts
@@ -17,7 +17,10 @@
* under the License.
*/
-import { isSafari } from './common';
+const isSafari = (): boolean => {
+ const { userAgent } = navigator;
+ return Boolean(userAgent && /^((?!chrome|android).)*safari/i.test(userAgent));
+};
// Use the new Clipboard API if the browser supports it
const copyTextWithClipboardApi = async (getText: () => Promise<string>) => {
diff --git a/superset-frontend/packages/superset-ui-core/src/utils/index.ts b/superset-frontend/packages/superset-ui-core/src/utils/index.ts
index 4d6e869cd0c..7f461f28fef 100644
--- a/superset-frontend/packages/superset-ui-core/src/utils/index.ts
+++ b/superset-frontend/packages/superset-ui-core/src/utils/index.ts
@@ -17,6 +17,7 @@
* under the License.
*/
export { default as convertKeysToCamelCase } from './convertKeysToCamelCase';
+export { default as copyTextToClipboard } from './copy';
export { default as ensureIsArray } from './ensureIsArray';
export { default as ensureIsInt } from './ensureIsInt';
export { default as isDefined } from './isDefined';
diff --git a/superset-frontend/src/SqlLab/components/HighlightedSql/HighlightedSql.test.tsx b/superset-frontend/src/SqlLab/components/HighlightedSql/HighlightedSql.test.tsx
index 70f63a2d0ff..2b4c841a040 100644
--- a/superset-frontend/src/SqlLab/components/HighlightedSql/HighlightedSql.test.tsx
+++ b/superset-frontend/src/SqlLab/components/HighlightedSql/HighlightedSql.test.tsx
@@ -36,13 +36,40 @@ test('renders a ModalTrigger component with shrink prop and maxWidth prop set to
);
expect(getByTestId('span-modal-trigger')).toBeInTheDocument();
});
-test('renders two code elements in modal when rawSql prop is provided', () => {
- const { getByRole, queryByRole, getByTestId } = render(
- <HighlightedSql sql={sql} rawSql="SELECT * FORM foo" shrink maxWidth={5} />,
+test('renders single SQL block with no tabs when rawSql equals sql', () => {
+ const { queryByRole, getByTestId, queryByText } = render(
+ <HighlightedSql sql={sql} rawSql={sql} shrink maxWidth={5} />,
);
expect(queryByRole('dialog')).not.toBeInTheDocument();
fireEvent.click(getByTestId('span-modal-trigger'));
expect(queryByRole('dialog')).toBeInTheDocument();
- const codeElements = getByRole('dialog').getElementsByTagName('code');
- expect(codeElements.length).toEqual(2);
+ expect(queryByText('Executed SQL')).not.toBeInTheDocument();
+ expect(queryByText('Source SQL')).toBeInTheDocument();
+});
+
+test('renders tabs when rawSql differs from sql', () => {
+ const { queryByRole, getByTestId, getByText } = render(
+ <HighlightedSql sql={sql} rawSql="SELECT * FROM foo" shrink maxWidth={5} />,
+ );
+ expect(queryByRole('dialog')).not.toBeInTheDocument();
+ fireEvent.click(getByTestId('span-modal-trigger'));
+ expect(queryByRole('dialog')).toBeInTheDocument();
+ expect(getByText('Executed SQL')).toBeInTheDocument();
+ expect(getByText('Source SQL')).toBeInTheDocument();
+});
+
+test('renders tabs when rawSql has an added LIMIT', () => {
+ const { queryByRole, getByTestId, getByText } = render(
+ <HighlightedSql
+ sql={sql}
+ rawSql={`${sql} LIMIT 1000`}
+ shrink
+ maxWidth={5}
+ />,
+ );
+ expect(queryByRole('dialog')).not.toBeInTheDocument();
+ fireEvent.click(getByTestId('span-modal-trigger'));
+ expect(queryByRole('dialog')).toBeInTheDocument();
+ expect(getByText('Executed SQL')).toBeInTheDocument();
+ expect(getByText('Source SQL')).toBeInTheDocument();
});
diff --git a/superset-frontend/src/SqlLab/components/HighlightedSql/index.tsx b/superset-frontend/src/SqlLab/components/HighlightedSql/index.tsx
index ef4831a9945..03adf270043 100644
--- a/superset-frontend/src/SqlLab/components/HighlightedSql/index.tsx
+++ b/superset-frontend/src/SqlLab/components/HighlightedSql/index.tsx
@@ -16,9 +16,9 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { css, styled, useTheme } from '@apache-superset/core/theme';
+import { styled, useTheme } from '@apache-superset/core/theme';
import { t } from '@apache-superset/core/translation';
-import { ModalTrigger } from '@superset-ui/core/components';
+import { ModalTrigger, Tabs } from '@superset-ui/core/components';
import CodeSyntaxHighlighter from '@superset-ui/core/components/CodeSyntaxHighlighter';
export interface HighlightedSqlProps {
@@ -63,7 +63,7 @@ const shrinkSql = (sql: string, maxLines: number, maxWidth: number) => {
function TriggerNode({ shrink, sql, maxLines, maxWidth }: TriggerNodeProps) {
return (
- <CodeSyntaxHighlighter language="sql">
+ <CodeSyntaxHighlighter language="sql" showCopyButton={false}>
{shrink ? shrinkSql(sql, maxLines, maxWidth) : sql}
</CodeSyntaxHighlighter>
);
@@ -80,25 +80,43 @@ function HighlightSqlModal({ rawSql, sql }: HighlightedSqlModalTypes) {
padding: theme.sizeUnit * 2,
};
+ const isDifferent = !!rawSql && rawSql !== sql;
+
+ if (!isDifferent) {
+ return (
+ <div>
+ <Title>{t('Source SQL')}</Title>
+ <CodeSyntaxHighlighter language="sql" customStyle={codeBlockStyle}>
+ {sql}
+ </CodeSyntaxHighlighter>
+ </div>
+ );
+ }
+
return (
- <div
- css={css`
- margin: -${theme.sizeUnit * 6}px;
- `}
- >
- <Title>{t('Source SQL')}</Title>
- <CodeSyntaxHighlighter language="sql" customStyle={codeBlockStyle}>
- {sql}
- </CodeSyntaxHighlighter>
- {rawSql && rawSql !== sql && (
- <div>
- <Title>{t('Executed SQL')}</Title>
- <CodeSyntaxHighlighter language="sql" customStyle={codeBlockStyle}>
- {rawSql}
- </CodeSyntaxHighlighter>
- </div>
- )}
- </div>
+ <Tabs
+ defaultActiveKey="executed"
+ items={[
+ {
+ key: 'executed',
+ label: t('Executed SQL'),
+ children: (
+ <CodeSyntaxHighlighter language="sql" customStyle={codeBlockStyle}>
+ {rawSql!}
+ </CodeSyntaxHighlighter>
+ ),
+ },
+ {
+ key: 'source',
+ label: t('Source SQL'),
+ children: (
+ <CodeSyntaxHighlighter language="sql" customStyle={codeBlockStyle}>
+ {sql}
+ </CodeSyntaxHighlighter>
+ ),
+ },
+ ]}
+ />
);
}
@@ -121,6 +139,7 @@ function HighlightedSql({
maxWidth={maxWidth}
/>
}
+ responsive
/>
);
}
diff --git a/superset-frontend/src/features/queries/SyntaxHighlighterCopy.tsx b/superset-frontend/src/features/queries/SyntaxHighlighterCopy.tsx
index 1daecd6d594..f247c51f61f 100644
--- a/superset-frontend/src/features/queries/SyntaxHighlighterCopy.tsx
+++ b/superset-frontend/src/features/queries/SyntaxHighlighterCopy.tsx
@@ -124,7 +124,11 @@ export default function SyntaxHighlighterCopy({
onClick={handleCopyClick}
onKeyDown={handleKeyDown}
/>
- <CodeSyntaxHighlighter language={language} {...syntaxHighlighterProps}>
+ <CodeSyntaxHighlighter
+ language={language}
+ showCopyButton={false}
+ {...syntaxHighlighterProps}
+ >
{children}
</CodeSyntaxHighlighter>
</SyntaxHighlighterWrapper>
diff --git a/superset-frontend/src/utils/copy.ts b/superset-frontend/src/utils/copy.ts
index 0980f2ab170..dd2cb7efa20 100644
--- a/superset-frontend/src/utils/copy.ts
+++ b/superset-frontend/src/utils/copy.ts
@@ -17,79 +17,6 @@
* under the License.
*/
-import { isSafari } from './common';
-
-// Use the new Clipboard API if the browser supports it
-const copyTextWithClipboardApi = async (getText: () => Promise<string>) => {
- // Safari (WebKit) does not support delayed generation of clipboard.
- // This means that writing to the clipboard, from the moment the user
- // interacts with the app, must be instantaneous.
- // However, neither writeText nor write accepts a Promise, so
- // we need to create a ClipboardItem that accepts said Promise to
- // delay the text generation, as needed.
- // Source: https://bugs.webkit.org/show_bug.cgi?id=222262P
- if (isSafari()) {
- try {
- const clipboardItem = new ClipboardItem({
- 'text/plain': getText(),
- });
- await navigator.clipboard.write([clipboardItem]);
- } catch {
- // Fallback to default clipboard API implementation
- const text = await getText();
- await navigator.clipboard.writeText(text);
- }
- } else {
- // For Blink, the above method won't work, but we can use the
- // default (intended) API, since the delayed generation of the
- // clipboard is now supported.
- // Source: https://bugs.chromium.org/p/chromium/issues/detail?id=1014310
- const text = await getText();
- await navigator.clipboard.writeText(text);
- }
-};
-
-const copyTextToClipboard = (getText: () => Promise<string>) =>
- copyTextWithClipboardApi(getText)
- // If the Clipboard API is not supported, fallback to the older method.
- .catch(() =>
- getText().then(
- text =>
- new Promise<void>((resolve, reject) => {
- const selection: Selection | null = document.getSelection();
- if (selection) {
- selection.removeAllRanges();
- const range = document.createRange();
- const span = document.createElement('span');
- span.textContent = text;
- span.style.position = 'fixed';
- span.style.top = '0';
- span.style.clip = 'rect(0, 0, 0, 0)';
- span.style.whiteSpace = 'pre';
-
- document.body.appendChild(span);
- range.selectNode(span);
- selection.addRange(range);
-
- try {
- if (!document.execCommand('copy')) {
- reject();
- }
- } catch (err) {
- reject();
- }
-
- document.body.removeChild(span);
- if (selection.removeRange) {
- selection.removeRange(range);
- } else {
- selection.removeAllRanges();
- }
- }
-
- resolve();
- }),
- ),
- );
+import copyTextToClipboard from '@superset-ui/core/utils/copy';
export default copyTextToClipboard;