Copilot commented on code in PR #43938: URL: https://github.com/apache/superset/pull/43938#discussion_r3944658342
########## superset-frontend/packages/superset-ui-chart-controls/src/utils/headerGroups.ts: ########## @@ -0,0 +1,595 @@ +/** + * 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 { + ensureIsArray, + getColumnLabel, + getMetricLabel, + QueryFormColumn, + QueryFormMetric, + SqlaFormData, +} from '@superset-ui/core'; +import { isEmpty, last } from 'lodash-es'; +import { + isPercentMetric, + isRegularMetric, + shouldSkipMetricColumn, +} from './metricColumnFilter'; + +export type HeaderGroupLabelAlign = 'left' | 'center' | 'right'; + +export type HeaderGroupPlacement = 'left' | 'right'; + +export type HeaderGroupConfig = { + id: string; + label: string; + columns: string[]; + labelAlign?: HeaderGroupLabelAlign; + placement?: HeaderGroupPlacement; + source?: 'time_compare'; + children?: HeaderGroupConfig[]; +}; + +export type HeaderGroupCell = { + key: string; + label: string; + colSpan: number; + rowSpan: number; + columnIndex: number; + labelAlign?: HeaderGroupLabelAlign; + isLastColumn: boolean; +}; + +export function getTimeComparisonColumnKeys(colname: string): string[] { + return [`Main ${colname}`, `# ${colname}`, `△ ${colname}`, `% ${colname}`]; +} + +export function expandGroupColumnKey( + identifier: string, + visibleKeys: string[], +): string[] { + const visible = new Set(visibleKeys); + const candidates = [ + identifier, + `%${identifier}`, + ...getTimeComparisonColumnKeys(identifier), + ]; Review Comment: When both a regular metric and its percent metric are visible (for example `revenue` and `%revenue`), selecting only `revenue` matches both candidates here. The percent column is then reordered and rendered inside the regular metric's group even though it was not selected. Use the prefixed fallback only when the exact identifier is absent. This issue also appears in the following locations of the same file: - line 209 - line 304 - line 591 ########## superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts: ########## @@ -240,10 +241,10 @@ const processComparisonColumns = ( originalLabel, metricName: col.key, label: t('Main'), - key: `${t('Main')} ${col.key}`, - config: getComparisonColConfig(t('Main'), col.key, columnConfig), + key: `Main ${col.key}`, + config: getComparisonColConfig('Main', col.key, columnConfig), formatter: getComparisonColFormatter( - t('Main'), + 'Main', Review Comment: Changing the main comparison column's persisted key from `t('Main')` to literal `Main` breaks existing non-English AG Grid charts: their `column_config` entries (and potentially saved grid state) are keyed by the previously localized value, so widths, formats, visibility, sorting, or filters no longer resolve after upgrade. Keep a backward-compatible lookup/migration for the localized key while using the stable key for new state. ########## superset-frontend/src/explore/components/controls/HeaderGroupsControl/utils.ts: ########## @@ -0,0 +1,143 @@ +/** + * 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 { nanoid } from 'nanoid'; +import { + headerGroupsHaveSameColumns, + syncTimeComparisonGroups, +} from '@superset-ui/chart-controls'; +import { HeaderGroupColumnOption, HeaderGroupConfig } from './types'; + +export { headerGroupsHaveSameColumns, syncTimeComparisonGroups }; + +export function createHeaderGroup(): HeaderGroupConfig { + return { + id: nanoid(), + label: '', + columns: [], + labelAlign: 'center', + placement: 'right', + children: [], + }; +} + +export function canSaveHeaderGroup(group: HeaderGroupConfig): boolean { + return Boolean(group.label?.trim()) && (group.columns ?? []).length > 0; Review Comment: Validation stops at the current group, so an add-mode parent becomes applicable even when one of its newly added subgroups is still unnamed or has no columns. This directly violates the stated invariant for subgroups and persists invalid nested configuration. Validate every child recursively before enabling/applying the group. ########## superset-frontend/src/explore/components/controls/HeaderGroupsControl/utils.ts: ########## @@ -0,0 +1,143 @@ +/** + * 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 { nanoid } from 'nanoid'; +import { + headerGroupsHaveSameColumns, + syncTimeComparisonGroups, +} from '@superset-ui/chart-controls'; +import { HeaderGroupColumnOption, HeaderGroupConfig } from './types'; + +export { headerGroupsHaveSameColumns, syncTimeComparisonGroups }; + +export function createHeaderGroup(): HeaderGroupConfig { + return { + id: nanoid(), + label: '', + columns: [], + labelAlign: 'center', + placement: 'right', + children: [], + }; +} + +export function canSaveHeaderGroup(group: HeaderGroupConfig): boolean { + return Boolean(group.label?.trim()) && (group.columns ?? []).length > 0; +} + +export function moveHeaderGroup( + groups: HeaderGroupConfig[], + fromIndex: number, + toIndex: number, +): HeaderGroupConfig[] { + if ( + fromIndex === toIndex || + fromIndex < 0 || + toIndex < 0 || + fromIndex >= groups.length || + toIndex >= groups.length + ) { + return groups; + } + const next = [...groups]; + const [moved] = next.splice(fromIndex, 1); + next.splice(toIndex, 0, moved); + return next; +} + +export function normalizeSelectedColumns(columns: unknown): string[] { + return (Array.isArray(columns) ? columns : []).map(column => + typeof column === 'object' && column !== null && 'value' in column + ? String((column as { value: unknown }).value) + : String(column), + ); +} + +export function collectHeaderGroupColumns( + groups: HeaderGroupConfig[] = [], +): string[] { + return groups.flatMap(group => [ + ...(group.columns ?? []), + ...collectHeaderGroupColumns(group.children), + ]); +} + +export function updateHeaderGroupAt( + groups: HeaderGroupConfig[], + path: number[], + updater: (group: HeaderGroupConfig) => HeaderGroupConfig, +): HeaderGroupConfig[] { + if (path.length === 0) { + return groups; + } + const [head, ...rest] = path; + return groups.map((group, index) => { + if (index !== head) { + return group; + } + if (rest.length === 0) { + return updater(group); + } + return { + ...group, + children: updateHeaderGroupAt(group.children ?? [], rest, updater), + }; + }); +} + +export function removeHeaderGroupAt( + groups: HeaderGroupConfig[], + path: number[], +): HeaderGroupConfig[] { + if (path.length === 0) { + return groups; + } + if (path.length === 1) { + return groups.filter((_, index) => index !== path[0]); + } + const [head, ...rest] = path; + return groups.map((group, index) => { + if (index !== head) { + return group; + } + return { + ...group, + children: removeHeaderGroupAt(group.children ?? [], rest), + }; + }); +} + +export function pruneStaleHeaderGroupColumns( + groups: HeaderGroupConfig[], + columnOptions: HeaderGroupColumnOption[], +): HeaderGroupConfig[] { + const validKeys = new Set(columnOptions.map(option => option.value)); + return groups.map(group => { + if (group.source === 'time_compare') { + return group; + } + return { + ...group, + columns: (group.columns ?? []).filter(column => validKeys.has(column)), Review Comment: This treats a saved base metric key as stale once time comparison is enabled because the options then contain only `Main metric`, `# metric`, etc. The rendering utilities intentionally expand a base identifier onto those comparison columns, so merely opening Customize prunes a valid pre-existing user group and changes the chart configuration. Preserve base identifiers that resolve to available comparison keys, or include those identifiers in the generated options. ########## superset-frontend/src/explore/components/controls/HeaderGroupsControl/HeaderGroupEditor.tsx: ########## @@ -0,0 +1,427 @@ +/** + * 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 { useState, type ReactNode } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, styled } from '@apache-superset/core/theme'; +import { Button, Input, Popover, Select } from '@superset-ui/core/components'; +import { Radio } from '@superset-ui/core/components/Radio'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { + HeaderGroupColumnOption, + HeaderGroupConfig, + HeaderGroupLabelAlign, + HeaderGroupPlacement, + MAX_HEADER_GROUP_DEPTH, +} from './types'; +import { + canSaveHeaderGroup, + createHeaderGroup, + normalizeSelectedColumns, + removeHeaderGroupAt, + updateHeaderGroupAt, +} from './utils'; + +export type HeaderGroupEditorProps = { + group?: HeaderGroupConfig; + path: number[]; + columnOptions: HeaderGroupColumnOption[]; + usedColumns: Set<string>; + onChange?: (path: number[], next: HeaderGroupConfig) => void; + onAddChild?: (path: number[]) => void; + onRemove?: (path: number[]) => void; + onSave?: (group: HeaderGroupConfig) => void; + mode?: 'add' | 'edit'; + children?: ReactNode; +}; + +const FormStack = styled.div` + ${({ theme }) => css` + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit * 3}px; + min-width: ${theme.sizeUnit * 92}px; + `} +`; + +const FieldRow = styled.div` + ${({ theme }) => css` + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit}px; + `} +`; + +const InlineFields = styled.div` + ${({ theme }) => css` + display: flex; + flex-wrap: nowrap; + align-items: flex-start; + gap: ${theme.sizeUnit * 3}px; + + & > *:first-of-type { + flex: 1.4 1 auto; + } + + & > *:last-of-type { + flex: 1 1 auto; + } + `} +`; + +const CompactRadioGroup = styled.div` + ${({ theme }) => css` + .ant-radio-group { + display: flex; + flex-wrap: nowrap; + width: 100%; + } + + .ant-radio-button-wrapper { + flex: 1 1 auto; + height: ${theme.sizeUnit * 6}px; + line-height: ${theme.sizeUnit * 6 - 2}px; + padding-inline: ${theme.sizeUnit}px; + font-size: ${theme.fontSizeSM}px; + text-align: center; + } + `} +`; + +const FieldLabel = styled.span` + ${({ theme }) => css` + color: ${theme.colorTextSecondary}; + font-size: ${theme.fontSizeSM}px; + `} +`; + +const NestedCard = styled.div` + ${({ theme }) => css` + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit * 2}px; + padding: ${theme.sizeUnit * 2}px; + border: 1px solid ${theme.colorBorder}; + border-radius: ${theme.borderRadius}px; + `} +`; + +const NestedHeader = styled.div` + ${({ theme }) => css` + display: flex; + align-items: center; + justify-content: space-between; + font-weight: ${theme.fontWeightStrong}; + `} +`; + +const ApplyRow = styled.div` + display: flex; + justify-content: flex-end; +`; + +const LABEL_ALIGN_OPTIONS: { label: string; value: HeaderGroupLabelAlign }[] = [ + { label: t('Left'), value: 'left' }, + { label: t('Center'), value: 'center' }, + { label: t('Right'), value: 'right' }, +]; + +const PLACEMENT_OPTIONS: { label: string; value: HeaderGroupPlacement }[] = [ + { label: t('Left'), value: 'left' }, + { label: t('Right'), value: 'right' }, +]; + +export function getGroupTitle(path: number[]): string { + const numberedPath = path.map(index => index + 1).join('.'); + return path.length === 1 + ? t('Group %s', numberedPath) + : t('Subgroup %s', numberedPath); +} + +function HeaderGroupForm({ + group, + path, + columnOptions, + usedColumns, + onChange, + onAddChild, + onRemove, + onApply, + showRemove = false, +}: { + group: HeaderGroupConfig; + path: number[]; + columnOptions: HeaderGroupColumnOption[]; + usedColumns: Set<string>; + onChange: (path: number[], next: HeaderGroupConfig) => void; + onAddChild: (path: number[]) => void; + onRemove: (path: number[]) => void; + onApply?: () => void; + showRemove?: boolean; +}) { + const availableOptions = columnOptions.filter( + option => + (group.columns ?? []).includes(option.value) || + !usedColumns.has(option.value), + ); + const canSave = canSaveHeaderGroup(group); + const isTimeCompareGroup = group.source === 'time_compare'; + const isTopLevel = path.length === 1; + + return ( + <FormStack data-test="header-group-editor"> + {showRemove && ( + <NestedHeader> + <span>{getGroupTitle(path)}</span> + <Button + buttonStyle="link" + buttonSize="small" + aria-label={t('Remove group')} + onClick={() => onRemove(path)} + icon={<Icons.DeleteOutlined iconSize="s" />} + /> + </NestedHeader> + )} + <FieldRow> + <FieldLabel>{t('Name')}</FieldLabel> + <Input + aria-label={t('Group name')} + value={group.label} + placeholder={t('Enter group name')} + onChange={event => + onChange(path, { ...group, label: event.target.value }) + } + /> + </FieldRow> + <FieldRow> + <FieldLabel>{t('Columns')}</FieldLabel> + <Select + ariaLabel={t('Group columns')} + mode="multiple" + allowClear={!isTimeCompareGroup} + showSearch={!isTimeCompareGroup} + disabled={isTimeCompareGroup} + value={group.columns ?? []} + options={availableOptions} + placeholder={t('Select columns')} + maxTagCount={3} + onChange={columns => { + onChange(path, { + ...group, + columns: normalizeSelectedColumns(columns), + }); + }} + /> + </FieldRow> + <InlineFields> + <FieldRow> + <FieldLabel>{t('Label position')}</FieldLabel> + <CompactRadioGroup> + <Radio.Group + size="small" + optionType="button" + value={group.labelAlign ?? 'center'} + onChange={event => + onChange(path, { + ...group, + labelAlign: event.target.value as HeaderGroupLabelAlign, + }) + } + > + {LABEL_ALIGN_OPTIONS.map(option => ( + <Radio.Button key={option.value} value={option.value}> + {option.label} + </Radio.Button> + ))} + </Radio.Group> + </CompactRadioGroup> + </FieldRow> + {isTopLevel && ( + <FieldRow> + <FieldLabel>{t('Table side')}</FieldLabel> + <CompactRadioGroup> + <Radio.Group + size="small" + optionType="button" + value={group.placement ?? 'right'} + onChange={event => + onChange(path, { + ...group, + placement: event.target.value as HeaderGroupPlacement, + }) + } + > + {PLACEMENT_OPTIONS.map(option => ( + <Radio.Button key={option.value} value={option.value}> + {option.label} + </Radio.Button> + ))} + </Radio.Group> + </CompactRadioGroup> + </FieldRow> + )} + </InlineFields> + {(group.children ?? []).length > 0 && ( + <FieldRow> + {(group.children ?? []).map((child, index) => ( + <NestedCard key={child.id}> + <HeaderGroupForm + group={child} + path={[...path, index]} + columnOptions={columnOptions} + usedColumns={usedColumns} + onChange={onChange} + onAddChild={onAddChild} + onRemove={onRemove} + showRemove + /> + </NestedCard> + ))} + </FieldRow> + )} + {path.length < MAX_HEADER_GROUP_DEPTH && !isTimeCompareGroup && ( + <Button + buttonStyle="dashed" + buttonSize="small" + disabled={!canSave} + icon={<Icons.PlusOutlined iconSize="s" />} + onClick={() => { + if (canSave) { + onAddChild(path); + } + }} + > + {t('Add subgroup')} + </Button> + )} + {onApply && ( + <ApplyRow> + <Button buttonStyle="primary" disabled={!canSave} onClick={onApply}> + {t('Apply')} + </Button> + </ApplyRow> + )} + </FormStack> + ); +} + +export default function HeaderGroupEditor({ + children, + group, + path, + columnOptions, + usedColumns, + onChange, + onAddChild, + onRemove, + onSave, + mode = 'edit', +}: HeaderGroupEditorProps) { + const [visible, setVisible] = useState(false); + const [draft, setDraft] = useState<HeaderGroupConfig>( + group ?? createHeaderGroup(), + ); + + const isAddMode = mode === 'add'; + const currentGroup = draft; + + const toDraftPath = (nextPath: number[]) => + isAddMode ? nextPath : [0, ...nextPath.slice(path.length)]; + + const handleOpenChange = (open: boolean) => { + setVisible(open); + if (open) { + setDraft( + isAddMode ? createHeaderGroup() : (group ?? createHeaderGroup()), + ); + } + }; + + const handleChange = (nextPath: number[], next: HeaderGroupConfig) => { + setDraft( + updateHeaderGroupAt([draft], toDraftPath(nextPath), () => next)[0], + ); + if (isAddMode) { + return; + } + if (canSaveHeaderGroup(next)) { + onChange?.(nextPath, next); + } + }; + + const handleAddChild = (nextPath: number[]) => { + const nextDraft = updateHeaderGroupAt( + [draft], + toDraftPath(nextPath), + current => ({ + ...current, + children: [...(current.children ?? []), createHeaderGroup()], + }), + )[0]; + if (nextDraft) { + setDraft(nextDraft); + } + if (!isAddMode) { + onAddChild?.(nextPath); + } Review Comment: In edit mode this immediately writes the newly created, empty subgroup to form data. If the popover is closed before that subgroup receives a name and column, the invalid subgroup remains saved; the later `canSaveHeaderGroup` guard only suppresses its field edits and does not roll back its creation. Keep the child as a draft until it is valid, or remove it when editing is abandoned. -- 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]
