codeant-ai-for-open-source[bot] commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3481945013
##########
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() };
+ }
Review Comment:
**Suggestion:** The falsy check treats `0` or empty-string IDs as missing
and replaces them with a new generated ID, which can corrupt valid identifiers
and break downstream updates/deletes keyed by ID. Use a null/undefined check
instead of a generic falsy check. [falsy zero check]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Future CRUDCollection callers with id 0 lose stable IDs.
- ⚠️ Delete/edit handlers may target unexpected rows by ID.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:62-71`,
`CollectionItem` is defined as `{ id: string | number; [key: string]:
unknown }` and
`createKeyedCollection` preserves any existing `id` when `o.id != null`,
explicitly
allowing `0` and `''` to be valid identifiers.
2. In the `onAddItem` handler at lines 229-257, a new item is created from
the
caller-provided `itemGenerator`, and the code checks `if (!newItem.id) {
newItem = {
...newItem, id: nanoid() }; }` (lines 233-235), treating any falsy `id` as
missing.
3. This falsy check directly conflicts with `createKeyedCollection`'s
semantics: an item
created by `itemGenerator` with a valid `id` of `0` or `''` (both allowed by
the
`CollectionItem` type) will have that identifier silently overwritten with a
new
`nanoid()` value in `onAddItem`, even though other parts of the component
treat those
values as legitimate IDs.
4. Once this happens, downstream behaviors that key off `id`—such as
`deleteItem` at
`CollectionTable/index.tsx:206-227` (which deletes from the `collection` map
by `id`) and
`rowKey={(record) => String(record.id)}` at lines 151-156—will operate on
the synthetic ID
rather than the original, which can break expectations for any caller that
relies on
stable, caller-assigned IDs. Aligning this check with the `!= null`
semantics used
elsewhere would avoid corrupting valid falsy identifiers.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6831e66e68864826a9ad9ef1f055f80e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=6831e66e68864826a9ad9ef1f055f80e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx
**Line:** 233:235
**Comment:**
*Falsy Zero Check: The falsy check treats `0` or empty-string IDs as
missing and replaces them with a new generated ID, which can corrupt valid
identifiers and break downstream updates/deletes keyed by ID. Use a
null/undefined check instead of a generic falsy check.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=bcd05ede0bc3e6c2c76ec37d2c7c51f9c3d2c0bc31aec86c8199e40253dccdce&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39461&comment_hash=bcd05ede0bc3e6c2c76ec37d2c7c51f9c3d2c0bc31aec86c8199e40253dccdce&reaction=dislike'>👎</a>
--
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]
