codeant-ai-for-open-source[bot] commented on code in PR #38258:
URL: https://github.com/apache/superset/pull/38258#discussion_r2855367909
##########
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);
+ }
+ });
+ return validIds;
+ });
+ }, [folders]);
Review Comment:
**Suggestion:** The sync effect only collects IDs from the top-level
`folders` array, so any collapsed state for nested subfolders is treated as
orphaned and removed whenever `folders` changes, causing nested folders to
unexpectedly expand or lose their collapse state after searches or datasource
updates; instead, you should traverse the full folder tree (including
`subFolders`) both when building the set of current IDs and when applying
default collapsed states. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Nested folders in Explore datasource panel lose collapse state.
- ⚠️ Users must re-collapse nested metric/column groups after updates.
```
</details>
```suggestion
// Sync collapsed state when folders change to prevent orphaned folder IDs
useEffect(() => {
const collectFolderIds = (foldersToCollect: Folder[], ids = new
Set<string>()): Set<string> => {
foldersToCollect.forEach(folder => {
ids.add(folder.id);
if (folder.subFolders && folder.subFolders.length > 0) {
collectFolderIds(folder.subFolders, ids);
}
});
return ids;
};
const currentFolderIds = collectFolderIds(folders);
setCollapsedFolderIds(prevIds => {
const validIds = new Set<string>();
prevIds.forEach(id => {
if (currentFolderIds.has(id)) {
validIds.add(id);
}
});
// Add any new folders (including subfolders) that should be collapsed
by default
const addCollapsedFolders = (foldersToProcess: Folder[]) => {
foldersToProcess.forEach(folder => {
if (folder.isCollapsed && !validIds.has(folder.id)) {
validIds.add(folder.id);
}
if (folder.subFolders && folder.subFolders.length > 0) {
addCollapsedFolders(folder.subFolders);
}
});
};
addCollapsedFolders(folders);
return validIds;
});
}, [folders]);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `DatasourceItems`
(`superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx`),
render
the component with a `folders` prop that contains at least one top-level
folder which
itself has `subFolders` (nested folders with their own `id`s), so that
`flattenFolderStructure` can recurse into them.
2. Interact with the UI so that a nested subfolder is collapsed, causing
`handleToggleCollapse` (defined later in the same file) to be called with
that subfolder's
`id`, which adds the subfolder `id` to `collapsedFolderIds` state.
3. Trigger a change to the `folders` prop (for example via the Explore Data
panel search
or datasource update as described in the PR text, which causes the parent
component to
pass a new `folders` array with the same folder/subfolder IDs). This causes
the sync
`useEffect` at lines 108–126 in `DatasourceItems.tsx` to run.
4. Inside that effect, `currentFolderIds` is computed only from
`folders.map(folder =>
folder.id)`, which contains only top-level folder IDs;
`setCollapsedFolderIds` then
filters `prevIds` against `currentFolderIds`, so the previously collapsed
subfolder `id`
(not present in `currentFolderIds`) is dropped. On the next render,
`flattenFolderStructure` no longer finds the subfolder `id` in
`collapsedFolderIds`, so
the nested folder appears expanded and the user's collapse state for nested
folders is
lost after each `folders` update.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx
**Line:** 108:126
**Comment:**
*Logic Error: The sync effect only collects IDs from the top-level
`folders` array, so any collapsed state for nested subfolders is treated as
orphaned and removed whenever `folders` changes, causing nested folders to
unexpectedly expand or lose their collapse state after searches or datasource
updates; instead, you should traverse the full folder tree (including
`subFolders`) both when building the set of current IDs and when applying
default collapsed states.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38258&comment_hash=eb90e45f2a408b85f7e07e7c69a8af302d14394f1241c521b11a221c7c14752a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38258&comment_hash=eb90e45f2a408b85f7e07e7c69a8af302d14394f1241c521b11a221c7c14752a&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]
