korbit-ai[bot] commented on code in PR #34028: URL: https://github.com/apache/superset/pull/34028#discussion_r2180856124
########## superset-frontend/packages/superset-ui-core/src/components/CodeSyntaxHighlighter/index.tsx: ########## @@ -0,0 +1,148 @@ +/** + * 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 { useEffect, useState } from 'react'; +import SyntaxHighlighterBase from 'react-syntax-highlighter/dist/cjs/light'; +import github from 'react-syntax-highlighter/dist/cjs/styles/hljs/github'; +import atomOneDark from 'react-syntax-highlighter/dist/cjs/styles/hljs/atom-one-dark'; +import { themeObject } from '@superset-ui/core'; + +export type SupportedLanguage = 'sql' | 'htmlbars' | 'markdown' | 'json'; + +export interface CodeSyntaxHighlighterProps { + children: string; + language?: SupportedLanguage; + customStyle?: React.CSSProperties; + showLineNumbers?: boolean; + wrapLines?: boolean; + style?: any; // Override theme style if needed +} + +// Track which languages have been registered to avoid duplicate registrations +const registeredLanguages = new Set<SupportedLanguage>(); + +// Language import functions - these will be called lazily +const languageImporters = { + sql: () => import('react-syntax-highlighter/dist/cjs/languages/hljs/sql'), + htmlbars: () => + import('react-syntax-highlighter/dist/cjs/languages/hljs/htmlbars'), + markdown: () => + import('react-syntax-highlighter/dist/cjs/languages/hljs/markdown'), + json: () => import('react-syntax-highlighter/dist/cjs/languages/hljs/json'), +}; + +/** + * Lazily register a language for syntax highlighting + */ +const registerLanguage = async (language: SupportedLanguage): Promise<void> => { + if (registeredLanguages.has(language)) { + return; // Already registered + } + + try { + const languageModule = await languageImporters[language](); + SyntaxHighlighterBase.registerLanguage(language, languageModule.default); + registeredLanguages.add(language); + } catch (error) { + console.warn(`Failed to load language ${language}:`, error); + } Review Comment: ### Improper Error Logging Mechanism <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Using console.warn for error logging instead of a proper logging framework ###### Why this matters Console statements are not suitable for production environments as they don't integrate with log aggregation systems and may be stripped out during builds ###### Suggested change ∙ *Feature Preview* ```typescript try { const languageModule = await languageImporters[language](); SyntaxHighlighterBase.registerLanguage(language, languageModule.default); registeredLanguages.add(language); } catch (error) { logger.error(`Failed to load syntax highlighting language: ${language}`, { error, component: 'CodeSyntaxHighlighter' }); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/313afd4d-2bc3-4ee4-aac5-c45cf242c4ea/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/313afd4d-2bc3-4ee4-aac5-c45cf242c4ea?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/313afd4d-2bc3-4ee4-aac5-c45cf242c4ea?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/313afd4d-2bc3-4ee4-aac5-c45cf242c4ea?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/313afd4d-2bc3-4ee4-aac5-c45cf242c4ea) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:d46f2d2a-6605-488f-b1b8-b16cf6c62952 --> [](d46f2d2a-6605-488f-b1b8-b16cf6c62952) ########## superset-frontend/src/features/queries/SyntaxHighlighterCopy.tsx: ########## @@ -16,58 +16,79 @@ * specific language governing permissions and limitations * under the License. */ +import { useEffect } from 'react'; import { styled, t } from '@superset-ui/core'; -import { SyntaxHighlighterProps } from 'react-syntax-highlighter'; -import sqlSyntax from 'react-syntax-highlighter/dist/cjs/languages/hljs/sql'; -import htmlSyntax from 'react-syntax-highlighter/dist/cjs/languages/hljs/htmlbars'; -import markdownSyntax from 'react-syntax-highlighter/dist/cjs/languages/hljs/markdown'; -import jsonSyntax from 'react-syntax-highlighter/dist/cjs/languages/hljs/json'; -import github from 'react-syntax-highlighter/dist/cjs/styles/hljs/github'; -import SyntaxHighlighter from 'react-syntax-highlighter/dist/cjs/light'; +import CodeSyntaxHighlighter, { + SupportedLanguage, + ThemedCodeSyntaxHighlighterProps, + preloadLanguages, +} from '@superset-ui/core/components/CodeSyntaxHighlighter'; import { Icons } from '@superset-ui/core/components/Icons'; import { ToastProps } from 'src/components/MessageToasts/withToasts'; import copyTextToClipboard from 'src/utils/copy'; -SyntaxHighlighter.registerLanguage('sql', sqlSyntax); -SyntaxHighlighter.registerLanguage('markdown', markdownSyntax); -SyntaxHighlighter.registerLanguage('html', htmlSyntax); -SyntaxHighlighter.registerLanguage('json', jsonSyntax); - const SyntaxHighlighterWrapper = styled.div` + position: relative; margin-top: -24px; &:hover { - svg { + .copy-button { visibility: visible; } } - svg { - position: relative; + .copy-button { + position: absolute; top: 40px; - left: 512px; + right: 16px; + z-index: 10; visibility: hidden; margin: -4px; + padding: 4px; + background: ${({ theme }) => theme.colors.grayscale.light4}; + border-radius: ${({ theme }) => theme.borderRadius}px; color: ${({ theme }) => theme.colors.grayscale.base}; + cursor: pointer; + transition: all 0.2s ease; + + &:hover { + background: ${({ theme }) => theme.colors.grayscale.light2}; + color: ${({ theme }) => theme.colors.grayscale.dark1}; + } + + &:focus { + visibility: visible; + outline: 2px solid ${({ theme }) => theme.colors.primary.base}; + outline-offset: 2px; + } } `; +interface SyntaxHighlighterCopyProps + extends Omit<ThemedCodeSyntaxHighlighterProps, 'children'> { + children: string; + addDangerToast?: ToastProps['addDangerToast']; + addSuccessToast?: ToastProps['addSuccessToast']; + language: SupportedLanguage; +} + export default function SyntaxHighlighterCopy({ addDangerToast, addSuccessToast, children, + language, ...syntaxHighlighterProps -}: SyntaxHighlighterProps & { - children: string; - addDangerToast?: ToastProps['addDangerToast']; - addSuccessToast?: ToastProps['addSuccessToast']; - language: 'sql' | 'markdown' | 'html' | 'json'; -}) { +}: SyntaxHighlighterCopyProps) { + // Preload the language when component mounts + useEffect(() => { + preloadLanguages([language]); + }, [language]); + function copyToClipboard(textToCopy: string) { copyTextToClipboard(() => Promise.resolve(textToCopy)) Review Comment: ### Unnecessary Promise Creation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The clipboard operation creates a new Promise for each copy operation, which is unnecessary since the text is already available. ###### Why this matters Creating unnecessary Promises adds overhead to the garbage collector and increases memory usage. ###### Suggested change ∙ *Feature Preview* Directly pass the text to copyTextToClipboard: ```typescript copyTextToClipboard(textToCopy) ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f86cee15-01af-4bce-b9ad-378be9ee624e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f86cee15-01af-4bce-b9ad-378be9ee624e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f86cee15-01af-4bce-b9ad-378be9ee624e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f86cee15-01af-4bce-b9ad-378be9ee624e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f86cee15-01af-4bce-b9ad-378be9ee624e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:4c47b917-7337-4bee-b2d3-64301e8deb61 --> [](4c47b917-7337-4bee-b2d3-64301e8deb61) ########## superset-frontend/packages/superset-ui-core/src/components/CodeSyntaxHighlighter/CodeSyntaxHighlighter.stories.tsx: ########## @@ -0,0 +1,328 @@ +/** + * 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 { Typography, Flex, Space } from '@superset-ui/core/components'; +import CodeSyntaxHighlighter from '.'; +import type { ThemedCodeSyntaxHighlighterProps, SupportedLanguage } from '.'; + +const { Title, Text, Paragraph } = Typography; + +const languages: SupportedLanguage[] = ['sql', 'json', 'htmlbars', 'markdown']; + +// Sample code for each language +const sampleCode = { + sql: `-- Complex SQL Query Example +SELECT + u.id, + u.username, + u.email, + COUNT(o.id) as total_orders, + SUM(o.amount) as total_spent, + AVG(o.amount) as avg_order_value +FROM users u +LEFT JOIN orders o ON u.id = o.user_id +WHERE u.created_at >= '2023-01-01' + AND u.status = 'active' +GROUP BY u.id, u.username, u.email +HAVING COUNT(o.id) > 0 +ORDER BY total_spent DESC, total_orders DESC +LIMIT 50;`, + + json: `{ + "user": { + "id": 12345, + "username": "john_doe", + "email": "[email protected]", + "profile": { + "firstName": "John", + "lastName": "Doe", + "age": 30, + "preferences": { + "theme": "dark", + "language": "en", + "notifications": true + } + }, + "orders": [ + { + "id": "order_001", + "amount": 99.99, + "status": "completed", + "items": ["laptop", "mouse"] + }, + { + "id": "order_002", + "amount": 49.99, + "status": "pending", + "items": ["keyboard"] + } + ] + } +}`, + + htmlbars: `{{!-- Handlebars Template Example --}} +<div class="user-profile"> + <h1>Welcome, {{user.firstName}} {{user.lastName}}!</h1> + + {{#if user.orders}} + <div class="orders-section"> + <h2>Your Orders ({{user.orders.length}})</h2> + + {{#each user.orders}} + <div class="order-card {{status}}"> + <h3>Order #{{id}}</h3> + <p class="amount">\${{amount}}</p> + <p class="status">Status: {{capitalize status}}</p> + + {{#if items}} + <ul class="items"> + {{#each items}} + <li>{{this}}</li> + {{/each}} + </ul> + {{/if}} + </div> + {{/each}} + </div> + {{else}} + <p class="no-orders">No orders found.</p> + {{/if}} +</div>`, + + markdown: `# CodeSyntaxHighlighter Component + +A **themed syntax highlighter** for Superset that supports multiple languages and automatic theme switching. + +## Features + +- 🎨 **Automatic theming** - Adapts to light/dark modes +- ⚡ **Lazy loading** - Languages load on-demand for better performance +- 🔧 **TypeScript support** - Full type safety +- 📱 **Responsive** - Works on all screen sizes + +## Supported Languages + +| Language | Extension | Use Case | +|----------|-----------|----------| +| SQL | \`.sql\` | Database queries | +| JSON | \`.json\` | Data interchange | +| HTML/Handlebars | \`.hbs\` | Templates | +| Markdown | \`.md\` | Documentation | + +## Usage + +\`\`\`typescript +import CodeSyntaxHighlighter from '@superset-ui/core/components/CodeSyntaxHighlighter'; + +<CodeSyntaxHighlighter language="sql"> + SELECT * FROM users WHERE active = true; +</CodeSyntaxHighlighter> +\`\`\` + +> **Note**: Languages are loaded lazily for optimal performance!`, +}; + +export default { + title: 'Components/CodeSyntaxHighlighter', + component: CodeSyntaxHighlighter, + parameters: { + docs: { + description: { + component: + "A themed syntax highlighter component that automatically adapts to Superset's light/dark themes and supports lazy loading of languages.", + }, + }, + }, +}; + +// Gallery showing all supported languages +export const LanguageGallery = () => ( + <Space direction="vertical" size="large" style={{ width: '100%' }}> + {languages.map(language => ( + <div key={language}> + <Title + level={3} + style={{ textTransform: 'capitalize', marginBottom: 16 }} + > + {language.toUpperCase()} Example + </Title> + <CodeSyntaxHighlighter language={language}> + {sampleCode[language]} + </CodeSyntaxHighlighter> + </div> + ))} + </Space> +); + +// Interactive playground +export const InteractivePlayground = ( + args: ThemedCodeSyntaxHighlighterProps, +) => ( + <CodeSyntaxHighlighter {...args}> + {args.children || sampleCode[args.language || 'sql']} + </CodeSyntaxHighlighter> +); + +InteractivePlayground.args = { + language: 'sql', + showLineNumbers: false, + wrapLines: true, + children: sampleCode.sql, +}; + +InteractivePlayground.argTypes = { + language: { + control: { type: 'select' }, + options: languages, + description: 'Programming language for syntax highlighting', + }, + showLineNumbers: { + control: { type: 'boolean' }, + description: 'Display line numbers alongside the code', + }, + wrapLines: { + control: { type: 'boolean' }, + description: 'Wrap long lines instead of showing horizontal scroll', + }, + children: { + control: { type: 'text' }, + description: 'Code content to highlight', + }, + customStyle: { + control: { type: 'object' }, + description: 'Custom CSS styles to apply to the syntax highlighter', + }, +}; + +// Showcase different styling options +export const StylingExamples = () => ( + <Space direction="vertical" size="large" style={{ width: '100%' }}> + {/* Default styling */} + <div> + <Title level={3}>Default Styling</Title> + <CodeSyntaxHighlighter language="sql"> + SELECT id, name FROM users WHERE active = true; + </CodeSyntaxHighlighter> + </div> + + {/* With line numbers */} + <div> + <Title level={3}>With Line Numbers</Title> + <CodeSyntaxHighlighter language="sql" showLineNumbers> + {sampleCode.sql} + </CodeSyntaxHighlighter> + </div> + + {/* Custom styling */} + <div> + <Title level={3}>Custom Styling (Compact)</Title> + <CodeSyntaxHighlighter + language="json" + customStyle={{ + fontSize: '12px', + padding: '12px', + maxHeight: '200px', + overflow: 'auto', + }} + > + {sampleCode.json} + </CodeSyntaxHighlighter> + </div> + + {/* No line wrapping */} + <div> + <Title level={3}>No Line Wrapping</Title> + <CodeSyntaxHighlighter + language="sql" + wrapLines={false} + customStyle={{ maxWidth: '400px' }} + > + {`SELECT very_long_column_name, another_very_long_column_name, yet_another_extremely_long_column_name FROM very_long_table_name WHERE condition = 'this is a very long condition';`} + </CodeSyntaxHighlighter> + </div> + </Space> +); + +// Performance and edge cases +export const EdgeCases = () => ( + <Space direction="vertical" size="large" style={{ width: '100%' }}> + {/* Empty content */} + <div> + <Title level={3}>Empty Content</Title> + <CodeSyntaxHighlighter language="sql" /> + </div> + + {/* Very long single line */} + <div> + <Title level={3}>Very Long Single Line</Title> + <CodeSyntaxHighlighter language="sql"> + {`SELECT ${'very_long_column_name, '.repeat(20)}id FROM users;`} Review Comment: ### Cryptic test data generation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Test data generation uses a cryptic string repeat pattern that's hard to understand at a glance. ###### Why this matters Complex one-liners in test data make it difficult to understand the purpose of the test case and maintain the code. ###### Suggested change ∙ *Feature Preview* ```typescript const LONG_LINE_TEST_QUERY = ` SELECT ${Array(20).fill('very_long_column_name').join(', ')}, id FROM users; `; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d026408-6f64-45b1-ba25-b00fbd8914d3/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d026408-6f64-45b1-ba25-b00fbd8914d3?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d026408-6f64-45b1-ba25-b00fbd8914d3?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d026408-6f64-45b1-ba25-b00fbd8914d3?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4d026408-6f64-45b1-ba25-b00fbd8914d3) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:856bcf39-a99d-4f0b-9e8a-317285fc8b25 --> [](856bcf39-a99d-4f0b-9e8a-317285fc8b25) -- 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]
