-
Notifications
You must be signed in to change notification settings - Fork 962
fix(desktop): v2 sidebar section count matches visual grouping #3544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -324,9 +324,31 @@ export function useDashboardSidebarData() { | |
| sectionMap: _sectionMap, | ||
| ...sidebarProject | ||
| } = resolvedProject; | ||
| sidebarProject.children = childEntries | ||
|
|
||
| const sortedChildren = childEntries | ||
| .sort((left, right) => left.tabOrder - right.tabOrder) | ||
| .map(({ child }) => child); | ||
|
|
||
| // Ungrouped workspaces rendered after a section header are visually | ||
| // grouped with that section (shared accent, collapse-together) and will | ||
| // be committed into it on next DnD. Reparent them here so section counts | ||
| // match what the user sees. | ||
| const children: DashboardSidebarProjectChild[] = []; | ||
| let currentSection: DashboardSidebarSection | null = null; | ||
| for (const child of sortedChildren) { | ||
| if (child.type === "section") { | ||
| currentSection = child.section; | ||
| children.push(child); | ||
| } else if (currentSection) { | ||
| currentSection.workspaces.push({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Reparented workspaces are always appended to Prompt for AI agents |
||
| ...child.workspace, | ||
| accentColor: currentSection.color, | ||
| }); | ||
| } else { | ||
| children.push(child); | ||
| } | ||
|
Comment on lines
+338
to
+349
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If the visual render intentionally places pending workspaces under the last section header (which would make this data change correct and desirable), this is fine. But if pending workspaces are meant to always float at the top level regardless of sections, a guard is needed: } else if (currentSection && child.type === "workspace" && !child.workspace.creationStatus) {
currentSection.workspaces.push({
...child.workspace,
accentColor: currentSection.color,
});
} else {
children.push(child);
}Worth double-checking whether the sidebar renderer visually groups pending workspaces with the last section before merging. Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts
Line: 338-349
Comment:
**Pending workspaces will be reparented into the last section**
`pendingWorkspaces` are injected into `childEntries` with `tabOrder: PENDING_WORKSPACE_TAB_ORDER` (`Number.MAX_SAFE_INTEGER`), so they always sort to the very end. If any section exists in the project, the reparenting loop will inevitably assign them to `currentSection` — meaning a "Creating…" or "Failed" workspace row will:
1. Receive the last section's accent color instead of `null`.
2. Count toward that section's workspace total.
3. Disappear when the user collapses that section.
If the visual render intentionally places pending workspaces under the last section header (which would make this data change correct and desirable), this is fine. But if pending workspaces are meant to always float at the top level regardless of sections, a guard is needed:
```typescript
} else if (currentSection && child.type === "workspace" && !child.workspace.creationStatus) {
currentSection.workspaces.push({
...child.workspace,
accentColor: currentSection.color,
});
} else {
children.push(child);
}
```
Worth double-checking whether the sidebar renderer visually groups pending workspaces with the last section before merging.
How can I resolve this? If you propose a fix, please make it concise. |
||
| } | ||
| sidebarProject.children = children; | ||
|
Comment on lines
+328
to
+351
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # Find and examine the useDashboardSidebarData.ts file to understand the pending workspace handling
fd -t f "useDashboardSidebarData.ts" apps/desktopRepository: superset-sh/superset Length of output: 207 🏁 Script executed: # Read the file to see the full context around pending workspaces and the reparenting logic
cat -n "apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts" | head -360 | tail -100Repository: superset-sh/superset Length of output: 3448 🏁 Script executed: # Find where PENDING_WORKSPACE_TAB_ORDER is defined
rg "PENDING_WORKSPACE_TAB_ORDER" apps/desktop/srcRepository: superset-sh/superset Length of output: 455 🏁 Script executed: # Search for creationStatus usage to understand the discriminator
rg "creationStatus" apps/desktop/src -A 2 -B 2Repository: superset-sh/superset Length of output: 24687 🏁 Script executed: # Check for git history or comments around the reparenting logic
git log --oneline -20 apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts 2>/dev/null || echo "Git history not available"Repository: superset-sh/superset Length of output: 148 🏁 Script executed: # Look for additional context around the reparenting section—check broader context
sed -n '315,355p' apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsRepository: superset-sh/superset Length of output: 1265 🏁 Script executed: # Search for any TODOs or design notes related to pending workspaces and sections
rg "pending.*section|section.*pending" apps/desktop/src -i -A 2 -B 2Repository: superset-sh/superset Length of output: 709 Pending workspaces are silently reparented into the last section, inflating its count and visually grouping transient items incorrectly. Pending workspaces are added to The reparenting logic was designed to sync section counts with visual grouping for ungrouped user-placed workspaces awaiting DnD commit. Pending workspaces are intentionally transient (marked with Separately, when reparenting does occur, items are appended at the end of Exclude pending items from reparenting using the 🛠 Guard to exclude pending workspaces for (const child of sortedChildren) {
if (child.type === "section") {
currentSection = child.section;
children.push(child);
- } else if (currentSection) {
+ } else if (currentSection && !child.workspace.creationStatus) {
currentSection.workspaces.push({
...child.workspace,
accentColor: currentSection.color,
});
} else {
children.push(child);
}
}🤖 Prompt for AI Agents |
||
| return [sidebarProject]; | ||
| }); | ||
| }, [ | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Pending workspaces (
creationStatusset) havetabOrder = Number.MAX_SAFE_INTEGER, so they always sort to the end and will be reparented into the last section by this branch. This causes them to inherit the section's accent color, inflate its count, and hide when the section is collapsed. Add a guard to exclude pending workspaces from reparenting.Prompt for AI agents