sadpandajoe commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3477765359
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -810,12 +768,7 @@ function OwnersSelector({
.filter(item => item.extra.active)
.map(item => ({
value: item.value as number,
- label: OwnerSelectLabel({
- name: item.text as string,
- email: item.extra?.email as string | undefined,
- }),
- [OWNER_TEXT_LABEL_PROP]: item.text as string,
- [OWNER_EMAIL_PROP]: (item.extra?.email as string) ?? '',
+ label: item.text as string,
})),
Review Comment:
This `OwnersSelector` conversion drops the owner-option label and email
search that exist on master and at this branch's merge-base. There, each option
is built as `label: OwnerSelectLabel({ name, email })` with the
`OWNER_TEXT_LABEL_PROP` / `OWNER_EMAIL_PROP` extra fields, and the
`AsyncSelect` below sets `optionFilterProps={OWNER_OPTION_FILTER_PROPS}`. Here
the label is reduced to `item.text`, the extra fields are gone, the
`optionFilterProps` prop on the select (around line 788) is gone, and the four
imports from `src/features/owners/OwnerSelectLabel` are removed.
User-visible regressions in the dataset editor owner picker:
1. Owner options no longer show the email under the name (name only).
2. Owners can no longer be found by typing an email, since
`optionFilterProps` was what extended the filter to the email field.
These fields are present at this branch's merge-base, so the drop is
introduced here. Please restore `OwnerSelectLabel(...)`, the two extra-field
props, `optionFilterProps={OWNER_OPTION_FILTER_PROPS}` on the select, and the
imports.
##########
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:
##########
@@ -52,18 +59,18 @@ const StyledButtonWrapper = styled.span`
`}
`;
-type CollectionItem = { id: string | number; [key: string]: any };
+type CollectionItem = { id: string | number; [key: string]: unknown };
function createKeyedCollection(arr: Array<object>) {
const collectionArray = arr.map(
- (o: any) =>
+ (o: Record<string, unknown>) =>
({
...o,
id: o.id || nanoid(),
Review Comment:
`id: o.id || nanoid()` treats a valid numeric id of `0` as missing and
assigns a fresh `nanoid()`. Any item whose id is `0` gets a new key on every
`createKeyedCollection` call, so its identity is unstable across prop syncs
(mismatched updates, phantom rows, or lost edits). Use a null check: `id: o.id
!= null ? o.id : nanoid()`.
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -363,24 +338,9 @@ const FlexRowContainer = styled.div`
`;
const StyledTableTabs = styled(Tabs)`
- flex: 1;
- min-height: 0;
- display: flex;
- flex-direction: column;
-
+ overflow: visible;
.ant-tabs-content-holder {
- flex: 1;
- min-height: 0;
- overflow: auto;
- padding-top: ${({ theme }) => theme.paddingMD}px;
- }
-
- .ant-tabs-content {
- height: 100%;
- }
-
- .ant-tabs-tabpane-active {
- height: 100%;
+ overflow: visible;
}
`;
Review Comment:
This replaces the self-scrolling flex chain the class version had (`flex: 1;
min-height: 0` on the container and tabs, plus `overflow: auto` on
`.ant-tabs-content-holder`) with `overflow: visible`. If the editor sits in a
fixed-height container (the dataset-edit modal), the tab content now overflows
instead of scrolling and rows below the fold become unreachable. Please confirm
the hosting modal provides the scroll context, or restore `overflow: auto` on
`.ant-tabs-content-holder` and the `flex: 1; min-height: 0` chain.
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopover/index.tsx:
##########
@@ -102,373 +90,337 @@ const LayerSelectContainer = styled.div`
margin-bottom: ${({ theme }) => theme.marginXXL}px;
`;
-export default class AdhocFilterEditPopover extends Component<
- AdhocFilterEditPopoverProps,
- AdhocFilterEditPopoverState
-> {
- popoverContentRef: RefObject<HTMLDivElement>;
-
- dragStartX = 0;
-
- dragStartY = 0;
-
- dragStartWidth = 0;
-
- dragStartHeight = 0;
-
- constructor(props: AdhocFilterEditPopoverProps) {
- super(props);
- this.onSave = this.onSave.bind(this);
- this.onDragDown = this.onDragDown.bind(this);
- this.onMouseMove = this.onMouseMove.bind(this);
- this.onMouseUp = this.onMouseUp.bind(this);
- this.onAdhocFilterChange = this.onAdhocFilterChange.bind(this);
- this.setSimpleTabIsValid = this.setSimpleTabIsValid.bind(this);
- this.adjustHeight = this.adjustHeight.bind(this);
- this.onTabChange = this.onTabChange.bind(this);
- this.loadLayerOptions = this.loadLayerOptions.bind(this);
- this.onLayerChange = this.onLayerChange.bind(this);
-
- this.state = {
- adhocFilter: this.props.adhocFilter,
- width: POPOVER_INITIAL_WIDTH,
- height: POPOVER_INITIAL_HEIGHT,
- activeKey: this.props?.adhocFilter?.expressionType || 'SIMPLE',
- isSimpleTabValid: true,
- selectedLayers: [{ id: null, value: -1, label: 'All' }],
- layerOptions: [],
- hasLayerFilterScopeChanged: false,
- };
+function AdhocFilterEditPopover({
+ adhocFilter: propsAdhocFilter,
+ onChange,
+ onClose,
+ onResize,
+ options,
+ datasource,
+ partitionColumn,
+ operators,
+ requireSave,
+ ...popoverProps
+}: AdhocFilterEditPopoverProps) {
+ const popoverContentRef = useRef<HTMLDivElement>(null);
+
+ const dragStartRef = useRef({
+ x: 0,
+ y: 0,
+ width: 0,
+ height: 0,
+ });
+
+ const [adhocFilter, setAdhocFilter] =
useState<AdhocFilter>(propsAdhocFilter);
+ const [width, setWidth] = useState(POPOVER_INITIAL_WIDTH);
+ const [height, setHeight] = useState(POPOVER_INITIAL_HEIGHT);
+ const [isSimpleTabValid, setIsSimpleTabValid] = useState(true);
+ const [selectedLayers, setSelectedLayers] = useState<LayerOption[]>([
+ { id: null, value: -1, label: 'All' },
+ ]);
+ const [layerOptions, setLayerOptions] = useState<LayerOption[]>([]);
+ const [hasLayerFilterScopeChanged, setHasLayerFilterScopeChanged] =
+ useState(false);
+
+ const loadLayerOptions = useCallback(
+ (page: number, pageSize: number) => {
+ const query = rison.encode({
+ columns: ['id', 'slice_name', 'viz_type'],
+ filters: [{ col: 'viz_type', opr: 'sw', value: 'deck' }],
+ page,
+ page_size: pageSize,
+ order_column: 'slice_name',
+ order_direction: 'asc',
+ });
- this.popoverContentRef = createRef();
- }
+ return SupersetClient.get({
+ endpoint: `/api/v1/chart/?q=${query}`,
+ }).then(response => {
+ if (!response?.json?.result) {
+ return {
+ data: [
+ {
+ id: null,
+ value: -1,
+ label: 'All',
+ },
+ ],
+ totalCount: 1,
+ };
+ }
+
+ const deckSlices = (propsAdhocFilter?.deck_slices || []) as number[];
+
+ const list = [
+ {
+ id: null,
+ value: -1,
+ label: 'All',
+ },
+ ...response.json.result
+ .map((item: { id: number; slice_name: string }) => {
+ const sliceIndex = deckSlices.indexOf(item.id);
+ return {
+ id: item.id,
+ value: sliceIndex >= 0 ? sliceIndex : item.id,
+ label: item.slice_name,
+ sliceIndex,
+ };
+ })
+ .filter((item: { sliceIndex: number }) => item.sliceIndex !== -1)
+ .map(
+ ({
+ sliceIndex,
+ ...item
+ }: {
+ sliceIndex: number;
+ id: number;
+ value: number;
+ label: string;
+ }) => item,
+ ),
+ ];
- componentDidMount() {
- document.addEventListener('mouseup', this.onMouseUp);
+ return {
+ data: list,
+ totalCount: list.length,
+ };
+ });
+ },
+ [propsAdhocFilter?.deck_slices],
+ );
+
+ const onMouseMove = useCallback(
+ (e: MouseEvent) => {
+ onResize();
+ setWidth(
+ Math.max(
+ dragStartRef.current.width + (e.clientX - dragStartRef.current.x),
+ POPOVER_INITIAL_WIDTH,
+ ),
+ );
+ setHeight(
+ Math.max(
+ dragStartRef.current.height + (e.clientY - dragStartRef.current.y),
+ POPOVER_INITIAL_HEIGHT,
+ ),
+ );
+ },
+ [onResize],
+ );
+
+ const onMouseUp = useCallback(() => {
+ document.removeEventListener('mousemove', onMouseMove);
+ }, [onMouseMove]);
+
+ useEffect(() => {
+ document.addEventListener('mouseup', onMouseUp);
// Load layer options if deck_slices exist
- const deckSlices = this.props.adhocFilter?.deck_slices as
- | number[]
- | undefined;
+ const deckSlices = propsAdhocFilter?.deck_slices as number[] | undefined;
if (deckSlices && deckSlices.length > 0) {
- this.loadLayerOptions(0, 100).then(result => {
- this.setState({ layerOptions: result.data });
- const layerFilterScope = this.props.adhocFilter?.layerFilterScope as
+ loadLayerOptions(0, 100).then(result => {
+ setLayerOptions(result.data);
+ const layerFilterScope = propsAdhocFilter?.layerFilterScope as
| number[]
| undefined;
if (layerFilterScope) {
- const selectedLayers = layerFilterScope.map(item => {
- const layerOption = result.data.find(
- option => option.value === item,
- );
- return layerOption;
- });
- this.setState({
- selectedLayers: selectedLayers.filter(Boolean) as LayerOption[],
- });
+ const layers = layerFilterScope
+ .map(item => result.data.find(option => option.value === item))
+ .filter(Boolean) as LayerOption[];
+ setSelectedLayers(layers);
}
});
}
- }
- componentWillUnmount() {
- document.removeEventListener('mouseup', this.onMouseUp);
- document.removeEventListener('mousemove', this.onMouseMove);
- }
+ return () => {
+ document.removeEventListener('mouseup', onMouseUp);
+ document.removeEventListener('mousemove', onMouseMove);
+ };
+ }, [loadLayerOptions, onMouseMove, onMouseUp, propsAdhocFilter]);
Review Comment:
This mount effect depends on the entire `propsAdhocFilter` object. If the
parent re-creates that reference on a render (common when a selector returns a
new object), the effect tears down and re-adds the `mouseup` listener and
re-hits the deck-layer options API on every such render. The cleanup also
removes `mousemove`, so if it fires mid-resize it interrupts the drag started
by `onDragDown`. The class version registered `mouseup` once on mount. Suggest
splitting into two effects: one keyed on `[onMouseUp, onMouseMove]` for the
listener lifecycle, and a separate one keyed on the deck-slices value for the
one-time options load.
##########
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:
##########
@@ -74,270 +81,317 @@ function createKeyedCollection(arr: Array<object>) {
};
}
-export default class CRUDCollection extends PureComponent<
- CRUDCollectionProps,
- CRUDCollectionState
-> {
- constructor(props: CRUDCollectionProps) {
- super(props);
-
- const { collection, collectionArray } = createKeyedCollection(
- props.collection,
- );
-
- // Get initial page size from pagination prop
- const initialPageSize =
- typeof props.pagination === 'object' && props.pagination?.pageSize
- ? props.pagination.pageSize
- : 10;
-
- this.state = {
- expandedColumns: {},
- collection,
- collectionArray,
- sortColumn: '',
- sort: 0,
- currentPage: 1,
- pageSize: initialPageSize,
- };
- this.onAddItem = this.onAddItem.bind(this);
- this.renderExpandableSection = this.renderExpandableSection.bind(this);
- this.getLabel = this.getLabel.bind(this);
- this.onFieldsetChange = this.onFieldsetChange.bind(this);
- this.changeCollection = this.changeCollection.bind(this);
- this.handleTableChange = this.handleTableChange.bind(this);
- this.buildTableColumns = this.buildTableColumns.bind(this);
- this.toggleExpand = this.toggleExpand.bind(this);
+export default function CRUDCollection({
+ allowAddItem = false,
+ allowDeletes = false,
+ collection: propsCollection,
+ columnLabels,
+ columnLabelTooltips,
+ emptyMessage = t('No items'),
+ expandFieldset,
+ itemGenerator,
+ itemCellProps,
+ itemRenderers,
+ onChange,
+ tableColumns,
+ sortColumns = [],
+ stickyHeader = false,
+ pagination = false,
+ filterTerm,
+ filterFields,
+}: CRUDCollectionProps) {
+ const [expandedColumns, setExpandedColumns] = useState<
+ Record<PropertyKey, boolean>
+ >({});
+ // Seed both pieces of state from a single createKeyedCollection() pass so
+ // that items lacking an `id` get one consistent set of synthetic ids
+ // (matching the prior class component, which keyed the collection once).
+ const initialKeyed = useRef<ReturnType<typeof createKeyedCollection>>();
+ if (!initialKeyed.current) {
+ initialKeyed.current = createKeyedCollection(propsCollection);
}
+ const [collection, setCollection] = useState<
+ Record<PropertyKey, CollectionItem>
+ >(() => initialKeyed.current!.collection);
+ const [collectionArray, setCollectionArray] = useState<CollectionItem[]>(
+ () => initialKeyed.current!.collectionArray,
+ );
+ const [sortColumn, setSortColumn] = useState<string>('');
+ const [sort, setSort] = useState<SortOrderEnum>(SortOrderEnum.Unsorted);
+ // Controlled pagination: tracked so that filtering can clamp currentPage
+ // back to a valid page (avoids the user being stranded on an empty page
+ // when filterTerm shrinks the result set).
+ const [pageSize, setPageSize] = useState<number>(() =>
+ typeof pagination === 'object' && pagination?.pageSize
+ ? pagination.pageSize
+ : 10,
+ );
+ const [currentPage, setCurrentPage] = useState<number>(1);
+
+ // Sync with props.collection changes
+ useEffect(() => {
+ const { collection: newCollection, collectionArray: newCollectionArray } =
+ createKeyedCollection(propsCollection);
+ setCollection(newCollection);
+ setCollectionArray(newCollectionArray);
+ }, [propsCollection]);
+
+ const onCellChange = useCallback(
+ (id: string | number, col: string, val: unknown) => {
+ setCollection(prevCollection => {
+ const updatedCollection = {
+ ...prevCollection,
+ [id]: {
+ ...prevCollection[id],
+ [col]: val,
+ },
+ };
+ return updatedCollection;
+ });
- componentDidUpdate(prevProps: CRUDCollectionProps) {
- if (this.props.collection !== prevProps.collection) {
- const { collection, collectionArray } = createKeyedCollection(
- this.props.collection,
- );
+ setCollectionArray(prevCollectionArray => {
+ const updatedCollectionArray = prevCollectionArray.map(item => {
+ if (item.id === id) {
+ return {
+ ...item,
+ [col]: val,
+ };
+ }
+ return item;
+ });
- this.setState(prevState => ({
- collection,
- collectionArray,
- expandedColumns: prevState.expandedColumns,
- }));
- }
- }
+ if (onChange) {
+ onChange(updatedCollectionArray);
+ }
- onCellChange(id: string | number, col: string, val: unknown) {
- this.setState(prevState => {
- const updatedCollection = {
- ...prevState.collection,
- [id]: {
- ...prevState.collection[id],
- [col]: val,
- },
- };
- const updatedCollectionArray = prevState.collectionArray.map(item =>
- item.id === id ? updatedCollection[id] : item,
- );
+ return updatedCollectionArray;
+ });
+ },
+ [onChange],
+ );
- if (this.props.onChange) {
- this.props.onChange(updatedCollectionArray);
+ const changeCollection = useCallback(
+ (
+ newCollection: Record<PropertyKey, CollectionItem>,
+ currentCollectionArray: CollectionItem[],
+ ) => {
+ // Preserve existing order instead of recreating from Object.keys()
+ const existingIds = new Set(currentCollectionArray.map(item => item.id));
+ const newCollectionArray: CollectionItem[] = [];
+
+ // First pass: preserve existing order and update items
+ for (const existingItem of currentCollectionArray) {
+ if (newCollection[existingItem.id]) {
+ newCollectionArray.push(newCollection[existingItem.id]);
+ }
}
- return {
- collection: updatedCollection,
- collectionArray: updatedCollectionArray,
- };
- });
- }
- onAddItem() {
- if (this.props.itemGenerator) {
- let newItem = this.props.itemGenerator();
- const shouldStartExpanded = newItem.expanded === true;
- if (!newItem.id) {
- newItem = { ...newItem, id: nanoid() };
+ // Second pass: add new items
+ for (const item of Object.values(newCollection)) {
+ if (!existingIds.has(item.id)) {
+ newCollectionArray.push(item);
+ }
}
- delete newItem.expanded;
- this.setState(
- prevState => {
- const newCollection = {
- ...prevState.collection,
- [newItem.id]: newItem,
- };
- const newExpandedColumns = shouldStartExpanded
- ? { ...prevState.expandedColumns, [newItem.id]: true }
- : prevState.expandedColumns;
- const newCollectionArray = [newItem, ...prevState.collectionArray];
-
- return {
- collection: newCollection,
- collectionArray: newCollectionArray,
- expandedColumns: newExpandedColumns,
- };
- },
- () => {
- if (this.props.onChange) {
- this.props.onChange(this.state.collectionArray);
- }
- },
- );
- }
- }
+ setCollection(newCollection);
+ setCollectionArray(newCollectionArray);
- onFieldsetChange(item: any) {
- this.changeCollection({
- ...this.state.collection,
- [item.id]: item,
- });
- }
+ if (onChange) {
+ onChange(newCollectionArray);
+ }
+ },
+ [onChange],
+ );
- getLabel(col: any): string {
- const { columnLabels } = this.props;
- let label = columnLabels?.[col] ? columnLabels[col] : col;
- if (label.startsWith('__')) {
- label = '';
- }
- return label;
- }
+ const deleteItem = useCallback(
+ (id: string | number) => {
+ setCollection(prevCollection => {
+ const newColl = { ...prevCollection };
+ delete newColl[id];
+ return newColl;
+ });
- getTooltip(col: string): string | undefined {
- const { columnLabelTooltips } = this.props;
- return columnLabelTooltips?.[col];
- }
+ setCollectionArray(prevCollectionArray => {
+ const newCollectionArray = prevCollectionArray.filter(
+ item => item.id !== id,
+ );
- changeCollection(collection: any) {
- // Preserve existing order instead of recreating from Object.keys()
- const existingIds = new Set(
- this.state.collectionArray.map(item => item.id),
- );
- const newCollectionArray: CollectionItem[] = [];
-
- // First pass: preserve existing order and update items
- for (const existingItem of this.state.collectionArray) {
- if (collection[existingItem.id]) {
- newCollectionArray.push(collection[existingItem.id]);
- }
- }
+ if (onChange) {
+ onChange(newCollectionArray);
+ }
- // Second pass: add new items
- for (const item of Object.values(collection) as CollectionItem[]) {
- if (!existingIds.has(item.id)) {
- newCollectionArray.push(item);
- }
- }
+ return newCollectionArray;
+ });
+ },
+ [onChange],
+ );
- this.setState({ collection, collectionArray: newCollectionArray });
+ const onAddItem = useCallback(() => {
+ if (itemGenerator) {
+ let newItem = itemGenerator() as CollectionItem;
+ const shouldStartExpanded = newItem.expanded === true;
+ if (!newItem.id) {
+ newItem = { ...newItem, id: nanoid() };
+ }
+ delete newItem.expanded;
- if (this.props.onChange) {
- this.props.onChange(newCollectionArray);
- }
- }
+ setCollection(prevCollection => ({
+ ...prevCollection,
+ [newItem.id]: newItem,
+ }));
- deleteItem(id: string | number) {
- const newColl = { ...this.state.collection };
- delete newColl[id];
- this.changeCollection(newColl);
- }
+ setCollectionArray(prevCollectionArray => {
+ const newCollectionArray = [newItem, ...prevCollectionArray];
- toggleExpand(id: any) {
- this.setState(prevState => ({
- expandedColumns: {
- ...prevState.expandedColumns,
- [id]: !prevState.expandedColumns[id],
- },
- }));
- }
+ if (onChange) {
+ onChange(newCollectionArray);
+ }
- handleTableChange(
- pagination: TablePaginationConfig,
- _filters: Record<string, FilterValue | null>,
- sorter: SorterResult<CollectionItem> | SorterResult<CollectionItem>[],
- ) {
- // Handle pagination changes
- if (pagination.current !== undefined && pagination.pageSize !== undefined)
{
- this.setState({
- currentPage: pagination.current,
- pageSize: pagination.pageSize,
+ return newCollectionArray;
});
+
+ if (shouldStartExpanded) {
+ setExpandedColumns(prev => ({ ...prev, [newItem.id]: true }));
+ }
}
+ }, [itemGenerator, onChange]);
+
+ const onFieldsetChange = useCallback(
+ (item: CollectionItem) => {
+ changeCollection(
+ {
+ ...collection,
+ [item.id]: item,
+ },
+ collectionArray,
+ );
+ },
+ [changeCollection, collection, collectionArray],
+ );
- // Handle sorting changes
- const columnSorter = Array.isArray(sorter) ? sorter[0] : sorter;
- let newSortColumn = '';
- let newSortOrder = 0;
+ const getLabel = useCallback(
+ (col: string): string => {
+ let label = columnLabels?.[col] ? columnLabels[col] : col;
+ if (label.startsWith('__')) {
+ label = '';
+ }
+ return label;
+ },
+ [columnLabels],
+ );
- if (columnSorter?.columnKey && columnSorter?.order) {
- newSortColumn = columnSorter.columnKey as string;
- newSortOrder = columnSorter.order === 'ascend' ? 1 : 2;
- }
+ const getTooltip = useCallback(
+ (col: string): string | undefined => columnLabelTooltips?.[col],
+ [columnLabelTooltips],
+ );
- const { sortColumns } = this.props;
- const col = newSortColumn;
+ const toggleExpand = useCallback((id: string | number) => {
+ setExpandedColumns(prev => ({
+ ...prev,
+ [id]: !prev[id],
+ }));
+ }, []);
+
+ const handleTableChange = useCallback(
+ (
+ paginationEvt: TablePaginationConfig,
+ _filters: Record<string, FilterValue | null>,
+ sorter: SorterResult<CollectionItem> | SorterResult<CollectionItem>[],
+ ) => {
+ if (
+ paginationEvt.current !== undefined &&
+ paginationEvt.pageSize !== undefined
+ ) {
+ setCurrentPage(paginationEvt.current);
+ setPageSize(paginationEvt.pageSize);
+ }
+ const columnSorter = Array.isArray(sorter) ? sorter[0] : sorter;
+ let newSortColumn = '';
+ let newSortOrder = SortOrderEnum.Unsorted;
+
+ if (columnSorter?.columnKey && columnSorter?.order) {
+ newSortColumn = columnSorter.columnKey as string;
+ newSortOrder =
+ columnSorter.order === 'ascend'
+ ? SortOrderEnum.Asc
+ : SortOrderEnum.Desc;
+ }
- if (sortColumns?.includes(col) || newSortOrder === 0) {
- let sortedArray = [...this.props.collection];
+ const col = newSortColumn;
+
+ if (
+ sortColumns?.includes(col) ||
+ newSortOrder === SortOrderEnum.Unsorted
+ ) {
+ let sortedArray = [...propsCollection] as CollectionItem[];
Review Comment:
`handleTableChange` sorts from `propsCollection` rather than the current
`collectionArray` state. Local edits, additions, or deletions held in state but
not yet pushed up via `onChange` are invisible here, so clicking a sortable
column header silently discards them. Sort from `collectionArray`. The reset
branch at lines 351-353 reads `propsCollection` for the same reason and has the
same gap.
--
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]
