Copilot commented on code in PR #38258:
URL: https://github.com/apache/superset/pull/38258#discussion_r2897303373
##########
superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx:
##########
@@ -105,6 +105,26 @@ export const DatasourceItems = ({
),
);
+ // Sync collapsed state when folders change to prevent orphaned folder IDs
+ useEffect(() => {
+ const currentFolderIds = new Set(folders.map(folder => folder.id));
+ setCollapsedFolderIds(prevIds => {
+ const validIds = new Set<string>();
+ prevIds.forEach(id => {
+ if (currentFolderIds.has(id)) {
+ validIds.add(id);
+ }
Review Comment:
`currentFolderIds` only includes IDs from the top-level `folders` array, but
`Folder` supports nested `subFolders` (see `flattenFolderStructure`). This
effect will incorrectly drop collapsed state for nested folders (and won’t
remove orphaned nested IDs correctly). Consider collecting IDs recursively
across the full folder tree before filtering `prevIds`/adding defaults.
##########
superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx:
##########
@@ -105,6 +105,26 @@ export const DatasourceItems = ({
),
);
+ // Sync collapsed state when folders change to prevent orphaned folder IDs
+ useEffect(() => {
+ const currentFolderIds = new Set(folders.map(folder => folder.id));
+ setCollapsedFolderIds(prevIds => {
+ const validIds = new Set<string>();
+ prevIds.forEach(id => {
+ if (currentFolderIds.has(id)) {
+ validIds.add(id);
+ }
+ });
+ // Add any new folders that should be collapsed by default
+ folders.forEach(folder => {
+ if (folder.isCollapsed && !validIds.has(folder.id)) {
+ validIds.add(folder.id);
+ }
+ });
Review Comment:
The “add new folders collapsed by default” step re-applies
`folder.isCollapsed` on every `folders` prop change, which can override a
user’s expanded state for an existing folder (e.g., user expands it, then
search/datasource update rebuilds `folders` and it gets re-collapsed). To truly
apply defaults only for *new* folder IDs, track the previous folder-id set
(e.g., in a ref) and only add `isCollapsed` for IDs that weren’t present before.
##########
superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx:
##########
@@ -105,6 +105,26 @@ export const DatasourceItems = ({
),
);
+ // Sync collapsed state when folders change to prevent orphaned folder IDs
+ useEffect(() => {
+ const currentFolderIds = new Set(folders.map(folder => folder.id));
+ setCollapsedFolderIds(prevIds => {
+ const validIds = new Set<string>();
+ prevIds.forEach(id => {
+ if (currentFolderIds.has(id)) {
+ validIds.add(id);
+ }
+ });
+ // Add any new folders that should be collapsed by default
+ folders.forEach(folder => {
+ if (folder.isCollapsed && !validIds.has(folder.id)) {
+ validIds.add(folder.id);
+ }
+ });
Review Comment:
This effect always returns a new `Set` instance, even when the contents are
unchanged. Since `folders` changes frequently (e.g., during search), this can
trigger unnecessary re-renders and recomputation/reset of the virtualized list.
Consider returning `prevIds` when `validIds` is equal (same size + all entries)
to avoid redundant state updates.
```suggestion
});
// Avoid unnecessary state updates when the contents are unchanged
if (validIds.size === prevIds.size) {
let isSame = true;
prevIds.forEach(id => {
if (!validIds.has(id)) {
isSame = false;
}
});
if (isSame) {
return prevIds;
}
}
```
--
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]
