Copilot commented on code in PR #36349: URL: https://github.com/apache/superset/pull/36349#discussion_r2582387642
########## superset-frontend/packages/superset-ui-core/src/glossary/glossaryModels.ts: ########## @@ -0,0 +1,153 @@ +/** + * 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 { Glossary } from "./glossaryUtils"; + Review Comment: Circular dependency detected: `glossaryModels.ts` imports from `glossaryUtils.ts` (line 20), and `glossaryUtils.ts` imports from `glossaryModels.ts` (line 20). This can cause initialization issues and makes the code harder to maintain. Consider restructuring to remove the circular dependency, perhaps by moving the `Glossary` type to a separate file or into `glossaryModels.ts`. ```suggestion // Removed circular import of Glossary from glossaryUtils // Define the Glossary type locally to avoid circular dependency /** * A Glossary is a map of topic names to GlossaryTopic instances. */ export type Glossary = Record<string, GlossaryTopic>; ``` ########## superset-frontend/packages/superset-ui-core/src/components/Tooltip/index.tsx: ########## @@ -19,14 +19,51 @@ import { Tooltip as AntdTooltip } from 'antd'; import type { TooltipProps, TooltipPlacement } from './types'; +import { resolveGlossaryString } from '../../glossary/glossaryUtils'; + +export const Tooltip = ({ + overlayStyle, + title, + children, + ...props +}: TooltipProps) => { + // Check if the title matches a glossary term and get the URL if it does + + if(typeof title !== 'string') { + return <>{children}</>; + } + + const [glossaryUrl, description] = resolveGlossaryString(title as string); + const wrappedChildren = glossaryUrl ? ( + <a href={glossaryUrl} target="_blank" rel="noopener noreferrer"> + {children} + </a> + ) : ( + children + ); + + const wrappedDescription = glossaryUrl ? ( + <> + {description} + <hr style={{ margin: '8px 0', border: 'none', borderTop: '1px solid rgba(255, 255, 255, 0.2)' }} /> Review Comment: Inline styles should be extracted to a constant or use a CSS-in-JS solution for better maintainability and consistency with the rest of the codebase. ########## superset-frontend/packages/superset-ui-core/src/glossary/glossary.ts: ########## @@ -0,0 +1,113 @@ +/** + * 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. + */ + +/** + * Glossary definition containing terms organized by topic. + * + * ## How to add new glossary entries: + * + * 1. Add a new topic (if needed) or use an existing one + * 2. Add a term under the topic with a key (term name) and value object containing: + * - short: A brief description (displayed in tooltips) + * - extended (optional): An extended description (displayed in documentation) + * + * ## Example: + * export const glossaryDefinition: GlossaryDefinition = { + * Query: { + * Row_Limit: { + * short: t('Limits the number of rows...'), + * extended: t('Additional details...'), // optional + * }, + * }, + * }; + * + * ## Formatting Notes: + * - Term names with underscores (e.g., `Row_Limit`) will be displayed with spaces + * (e.g., "Row Limit") when rendered in the UI and documentation + */ + +export const glossaryDefinition: GlossaryDefinition = { + Query: { + Dimension: { + short: t( + 'Dimensions contain qualitative values such as names, dates, or geographical data. ' + + 'Use dimensions to categorize, segment, and reveal the details in your data. ' + + 'Dimensions affect the level of detail in the view.' + ), + }, + Metric: { + short: t( + 'Select one or many metrics to display. ' + + 'You can use an aggregation function on a column or write custom SQL to create a metric.' + ), + }, + Series: { + short: t( + 'Limits the number of series that get displayed. ' + + 'A joined subquery (or an extra phase where subqueries are not supported) is applied ' + + 'to limit the number of series that get fetched and rendered. ' + + 'This feature is useful when grouping by high cardinality column(s) ' + + 'though does increase the query complexity and cost.' + ), + }, + Row_Limit: { + short: t( + 'Limits the number of rows that get displayed. ' + + 'This feature is useful when grouping by high cardinality column(s) ' + + 'though does increase the query complexity and cost.' + ), + }, + Sort: { + short: t( + 'Orders the query result that generates the source data for this chart. ' + + 'If a series or row limit is reached, this determines what data are truncated. ' + + 'If undefined, defaults to the first metric (where appropriate).' + ), + }, + }, + Advanced_Analytics: { + Time_Shift: { + short: t( + 'Overlay results from a relative time period. ' + + 'Expects relative time deltas in natural language (example: 24 hours, 7 days, ' + + '52 weeks, 365 days). Free text is supported. ' + + 'Use "Inherit range from time filters" to shift the comparison time range ' + + 'by the same length as your time range and use "Custom" to set a custom comparison range.' + ), + }, + }, +}; + +/** + * Use this identity translate function as some environments (docs) do not have i18n setup. + */ +function t(message: string): string { + return message; +} Review Comment: The local `t()` function shadows the imported `t` function from `@superset-ui/core` (used in `glossaryUtils.ts` line 22). This can cause confusion and translation issues. The local identity function should be renamed (e.g., to `identity` or `noTranslate`) to avoid shadowing the actual translation function, or the real translation function should be used consistently throughout. ########## docs/package.json: ########## @@ -49,8 +49,8 @@ "@storybook/preview-api": "^8.6.11", "@storybook/theming": "^8.6.11", "@superset-ui/core": "^0.20.4", - "antd": "^5.29.1", - "caniuse-lite": "^1.0.30001757", + "antd": "^5.28.1", + "caniuse-lite": "^1.0.30001754", Review Comment: The `caniuse-lite` package is being downgraded from `^1.0.30001757` to `^1.0.30001754`. Downgrading this package means using outdated browser compatibility data, which could lead to incorrect feature detection or polyfill decisions. Consider keeping or upgrading to the latest version for the most accurate browser support information. ```suggestion "caniuse-lite": "^1.0.30001757", ``` ########## superset-frontend/packages/superset-ui-core/src/glossary/glossaryUtils.ts: ########## @@ -0,0 +1,84 @@ +/** + * 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 { GlossaryMap, GlossaryTerm, type GlossaryTopic } from './glossaryModels'; +import { glossaryDefinition } from './glossary'; +import { t } from '@superset-ui/core'; + +export const GLOSSARY_BASE_URL = 'http://localhost:3000/docs'; // TODO: Change to use the url for the environment. Review Comment: Hardcoded localhost URL will not work in production environments. This should be configured based on the environment (development, staging, production). Consider using an environment variable or configuration value that can be set per deployment. ```suggestion // Use environment variable if available, otherwise default to '/docs' for local development. export const GLOSSARY_BASE_URL = (typeof process !== 'undefined' && process.env && process.env.GLOSSARY_BASE_URL) || '/docs'; ``` ########## superset-frontend/packages/superset-ui-core/src/components/Tooltip/index.tsx: ########## @@ -19,14 +19,51 @@ import { Tooltip as AntdTooltip } from 'antd'; import type { TooltipProps, TooltipPlacement } from './types'; +import { resolveGlossaryString } from '../../glossary/glossaryUtils'; + +export const Tooltip = ({ + overlayStyle, + title, + children, + ...props +}: TooltipProps) => { + // Check if the title matches a glossary term and get the URL if it does + + if(typeof title !== 'string') { Review Comment: Missing space after `if` keyword. Should be `if (typeof title !== 'string')` according to JavaScript/TypeScript formatting conventions. ```suggestion if (typeof title !== 'string') { ``` ########## superset-frontend/packages/superset-ui-core/src/glossary/glossaryModels.ts: ########## @@ -0,0 +1,153 @@ +/** + * 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 { Glossary } from "./glossaryUtils"; + +// Encoding format prefix for glossary strings +export const GLOSSARY_ENCODING_PREFIX = '[GLOSSARY]|'; + +export class GlossaryTerm { + /** + * The topic under which the term is categorized. + */ + private readonly topic: string; + + /** + * The name of the term being defined. + */ + private readonly title: string; + + /** + * A short description of the term. Displayed on the frontend as a tooltip. + */ + private readonly short: string; + + /** + * An extended description of the term, shown alongside short on the documentation. + */ + private readonly extended?: string; + + constructor(options: { + topic: string; + title: string; + short: string; + extended?: string; + }) { + this.topic = options.topic; + this.title = options.title; + this.short = options.short; + this.extended = options.extended; + } + + getTopic(): string { + return this.topic; + } + + getTitle(): string { + return this.title; + } + + /** + * Returns a formatted display version of the title with underscores replaced by spaces. + */ + getDisplayTitle(): string { + return this.title.replace(/_/g, ' '); + } + + /** + * Returns the short description, optionally transformed by a provided translation function. + */ + getShort(t?: (value: string) => string): string { + if (!t) { + return this.short; + } + return t(this.short); + } + + getExtended(t?: (value: string) => string): string | undefined { + if (!t) { + return this.extended; + } + if (!this.extended) { + return undefined; + } + return t(this.extended); + } + + /** + * Encodes the glossary term into a string format that can be resolved later. + * Format: [GLOSSARY]|topic|title|description Review Comment: Incorrect comment - the encode format shows 4 parts (`[GLOSSARY]|topic|title|description`) but the implementation only uses 3 parts (`[GLOSSARY]|topic|title`). The comment should match the actual implementation. ```suggestion * Format: [GLOSSARY]|topic|title ``` ########## docs/docs/glossary.mdx: ########## @@ -0,0 +1,108 @@ +--- +title: Glossary +hide_title: true +sidebar_position: 10 +--- + +import { getAllGlossaryTopics } from '../../superset-frontend/packages/superset-ui-core/src/glossary'; +import { Table, ConfigProvider, theme } from 'antd'; +import { useColorMode } from '@docusaurus/theme-common'; +import { useEffect, useRef } from 'react'; + +export const GlossaryStructure = [ + { + title: 'Term', + dataIndex: 'title', + key: 'title', + width: 200, + }, + { + title: 'Short Description', + dataIndex: 'short', + key: 'short', + }, +]; + +export const GlossaryContent = () => { + const { colorMode } = useColorMode(); + const isDark = colorMode === 'dark'; + const tableRefs = useRef({}); + + const scrollToRow = (topic, rowKey) => { + const topicId = encodeURIComponent(topic); + const row = tableRefs.current[topicId]?.[rowKey]; + if (row) { + row.scrollIntoView({ behavior: 'smooth', block: 'center' }); + row.classList.add('table-row-highlight'); + setTimeout(() => row.classList.remove('table-row-highlight'), 2000); + } + }; + + useEffect(() => { + const hash = decodeURIComponent(window.location.hash.slice(1)); + if (!hash) return; + + const [topic, term] = hash.split('__'); + if (topic && term) scrollToRow(topic, hash); + }, []); Review Comment: The `useEffect` hook is missing `scrollToRow` in its dependency array, which violates React's rules of hooks. This could lead to stale closures or unexpected behavior. Add `scrollToRow` to the dependency array or use `useCallback` to memoize the `scrollToRow` function. ########## superset-frontend/packages/superset-ui-core/src/components/Tooltip/index.tsx: ########## @@ -19,14 +19,51 @@ import { Tooltip as AntdTooltip } from 'antd'; import type { TooltipProps, TooltipPlacement } from './types'; +import { resolveGlossaryString } from '../../glossary/glossaryUtils'; + +export const Tooltip = ({ + overlayStyle, + title, + children, + ...props +}: TooltipProps) => { + // Check if the title matches a glossary term and get the URL if it does + + if(typeof title !== 'string') { + return <>{children}</>; + } + + const [glossaryUrl, description] = resolveGlossaryString(title as string); Review Comment: Redundant type assertion `as string` - TypeScript already knows `title` is a string due to the type check on line 32. ```suggestion const [glossaryUrl, description] = resolveGlossaryString(title); ``` ########## docs/package.json: ########## @@ -49,8 +49,8 @@ "@storybook/preview-api": "^8.6.11", "@storybook/theming": "^8.6.11", "@superset-ui/core": "^0.20.4", - "antd": "^5.29.1", - "caniuse-lite": "^1.0.30001757", + "antd": "^5.28.1", Review Comment: The `antd` package is being downgraded from `^5.29.1` to `^5.28.1`. Downgrading dependencies can introduce known bugs or security vulnerabilities that were fixed in newer versions. Unless there's a specific compatibility issue, consider keeping or upgrading to the latest stable version instead. ```suggestion "antd": "^5.29.1", ``` ########## superset-frontend/packages/superset-ui-core/src/components/Tooltip/index.tsx: ########## @@ -19,14 +19,51 @@ import { Tooltip as AntdTooltip } from 'antd'; import type { TooltipProps, TooltipPlacement } from './types'; +import { resolveGlossaryString } from '../../glossary/glossaryUtils'; + +export const Tooltip = ({ + overlayStyle, + title, + children, + ...props +}: TooltipProps) => { + // Check if the title matches a glossary term and get the URL if it does + + if(typeof title !== 'string') { + return <>{children}</>; Review Comment: When title is not a string, the Tooltip returns only the children without rendering any tooltip. This breaks the expected behavior for non-glossary tooltips that may have React nodes as titles. Consider rendering a standard tooltip for non-string titles instead of bypassing the tooltip entirely. ```suggestion if (typeof title !== 'string') { // For non-string titles (e.g., React nodes), render a standard tooltip return ( <AntdTooltip title={title} styles={{ body: { overflow: 'hidden', textOverflow: 'ellipsis' }, root: overlayStyle ?? {}, }} {...props} > {children} </AntdTooltip> ); ``` -- 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]
