Copilot commented on code in PR #38865: URL: https://github.com/apache/superset/pull/38865#discussion_r3017344984
########## superset-frontend/src/dashboard/util/dynamicTitle.ts: ########## @@ -0,0 +1,146 @@ +/** + * 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 { ChartCustomization } from '@superset-ui/core'; +import uniq from 'lodash/uniq'; +import snakeCase from 'lodash/snakeCase'; +import { ChartCustomizationPlugins } from 'src/constants'; + +export type DynamicTitleTokenMappings = Record<string, string>; + +export interface DynamicTitleControlValues { + template?: string; + tokenMappings?: DynamicTitleTokenMappings; +} + +export interface DynamicTitleScopeCandidate { + id: string; + chartsInScope: number[]; + template?: string; +} + +const PLACEHOLDER_REGEX = /\{\{\s*([a-zA-Z0-9_]+)\s*\}\}/g; Review Comment: `PLACEHOLDER_REGEX` is a shared global RegExp with the `g` flag, so its `lastIndex` is mutable state across calls. While you reset `lastIndex` at the end, it’s safer to also reset before the first `exec` (and ideally in a `finally`) or to instantiate a new RegExp per call to avoid any cross-call interference if this function is ever refactored, re-entered, or invoked after other code uses the same RegExp. ########## superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx: ########## @@ -722,6 +898,350 @@ const FiltersConfigForm = ( const DateFilterComponent = DateFilterControlExtension ?? DateFilterControl; + const getDynamicTitleScopeCandidate = useCallback( + (customizationId: string): DynamicTitleScopeCandidate | undefined => { + if (removedFilters[customizationId]) { + return undefined; + } Review Comment: A large amount of dynamic-title-specific logic (scope derivation, available token computation, preview rendering, and validation) is embedded directly into `FiltersConfigForm`, increasing component size and cognitive load. Consider extracting the dynamic title editor into a dedicated component or custom hook (e.g., `useDynamicTitleEditor(...)`) that returns derived state + handlers; this will make the form easier to reason about, reduce the risk of regressions when editing unrelated filter functionality, and simplify targeted testing. ########## superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.tsx: ########## @@ -467,6 +483,72 @@ const Chart = (props: ChartProps) => { ], ); + const displaySliceName = useMemo(() => { + if (editMode) { + return props.sliceName; + } + + const matchingCustomizations = dynamicTitleCustomizations.filter( + customization => + !customization.removed && + customization.chartsInScope?.includes(props.id), + ); + const matchingCustomization = + matchingCustomizations[matchingCustomizations.length - 1]; + + if (!matchingCustomization) { + return props.sliceName; + } + + const { template, tokenMappings = {} } = getDynamicTitleControlValues( + matchingCustomization, + ); + + if ( + !template || + !canApplyDynamicTitleToScope( + template, + matchingCustomization.chartsInScope, + ) + ) { + return props.sliceName; + } + + const aliasValues = { + ...Object.entries(tokenMappings).reduce< + Record<string, string | undefined> + >((acc, [alias, filterId]) => { + const filter = nativeFilters?.[filterId]; + + if ( + !filter || + filter.type !== NativeFilterType.NativeFilter || + (filter.chartsInScope != null && + !filter.chartsInScope.includes(props.id)) + ) { + acc[alias] = undefined; + return acc; + } + + const filterState = dataMask[filterId]?.filterState; + const label = extractLabel(filterState); + const value = label ?? getFilterValueForDisplay(filterState?.value); Review Comment: `label ?? ...` treats an empty string label as a valid value, which then gets converted to `undefined` via `value || undefined`, potentially dropping a present filter value from the rendered title. To avoid false-empties, align with the preview logic and prefer a falsy check (or trim) so empty labels fall back to `getFilterValueForDisplay(...)` (e.g., use `label || ...`). This makes dynamic titles render consistently when labels are empty but values exist. ```suggestion const value = label || getFilterValueForDisplay(filterState?.value); ``` ########## superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts: ########## @@ -140,7 +140,7 @@ export default function transformProps( const compareLag = Number(compareLag_) || 0; let formattedSubheader = subheader; - const { r, g, b } = colorPicker; + const { r, g, b } = colorPicker ?? { r: 0, g: 0, b: 0 }; const mainColor = `rgb(${r}, ${g}, ${b})`; Review Comment: Falling back to pure black (`rgb(0, 0, 0)`) can create low-contrast trendlines/areas in dark themes (and may be visually inconsistent with the rest of the chart theme). Consider using a theme-derived fallback (e.g., a primary/text color from `theme`, or an existing default color used elsewhere in the plugin) so the visualization remains readable across themes. ```suggestion const themeTextColor = chartProps.theme?.echartsTheme?.textStyle?.color || chartProps.theme?.echartsTheme?.color?.[0]; const mainColor = colorPicker ? `rgb(${colorPicker.r}, ${colorPicker.g}, ${colorPicker.b})` : themeTextColor || 'currentColor'; ``` ########## superset-frontend/src/dashboard/util/dynamicTitle.ts: ########## @@ -0,0 +1,146 @@ +/** + * 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 { ChartCustomization } from '@superset-ui/core'; +import uniq from 'lodash/uniq'; +import snakeCase from 'lodash/snakeCase'; +import { ChartCustomizationPlugins } from 'src/constants'; + +export type DynamicTitleTokenMappings = Record<string, string>; + +export interface DynamicTitleControlValues { + template?: string; + tokenMappings?: DynamicTitleTokenMappings; +} + +export interface DynamicTitleScopeCandidate { + id: string; + chartsInScope: number[]; + template?: string; +} + +const PLACEHOLDER_REGEX = /\{\{\s*([a-zA-Z0-9_]+)\s*\}\}/g; +export const DYNAMIC_TITLE_CHART_TITLE_ALIAS = 'chart_title'; +const RESERVED_DYNAMIC_TITLE_ALIASES = new Set([ + DYNAMIC_TITLE_CHART_TITLE_ALIAS, +]); + +export const isDynamicTitleCustomization = ( + customization?: Partial<ChartCustomization> | null, +): customization is ChartCustomization => + customization?.filterType === ChartCustomizationPlugins.DynamicTitle; + +export const getDynamicTitleControlValues = ( + customization?: Partial<ChartCustomization> | null, +): DynamicTitleControlValues => { + const controlValues = + (customization?.controlValues as DynamicTitleControlValues | undefined) || + {}; + + return { + ...controlValues, + tokenMappings: Object.fromEntries( + Object.entries(controlValues.tokenMappings || {}).filter( + ([alias]) => !RESERVED_DYNAMIC_TITLE_ALIASES.has(alias), + ), + ), + }; +}; + +export const extractDynamicTitleAliases = (template?: string): string[] => { + if (!template) { + return []; + } + + const aliases: string[] = []; + let match = PLACEHOLDER_REGEX.exec(template); + while (match) { + aliases.push(match[1]); + match = PLACEHOLDER_REGEX.exec(template); + } + PLACEHOLDER_REGEX.lastIndex = 0; + return uniq(aliases); +}; Review Comment: `PLACEHOLDER_REGEX` is a shared global RegExp with the `g` flag, so its `lastIndex` is mutable state across calls. While you reset `lastIndex` at the end, it’s safer to also reset before the first `exec` (and ideally in a `finally`) or to instantiate a new RegExp per call to avoid any cross-call interference if this function is ever refactored, re-entered, or invoked after other code uses the same RegExp. -- 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]
