fix(desktop): place new v2 workspace at top of sidebar#4139
Conversation
The revert of #4120 (#4135) restored a hardcoded `tabOrder: 0` in the v2 workspace.create path. Existing rows default to `tabOrder: 0` too, so new workspaces tied with them and ordering was non-deterministic. Restores the `getPrependTabOrder` helper from #4120 as a shared util in `dashboardSidebarLocal/`, and uses it from `useWorkspaceCreates` so new rows land strictly above every existing top-level item — matching what the pending-row injection in `useDashboardSidebarData` already assumes.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughTab-ordering utility helpers are extracted from ChangesTab-Order Utility Centralization & Dynamic Workspace Ordering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Greptile SummaryThis PR fixes new v2 workspaces landing in a non-deterministic position in the sidebar by replacing the hardcoded
Confidence Score: 4/5The change is safe to merge — the bug fix is correct and the refactoring is mechanical with no behaviour change to existing paths. The topLevelItems filter in useWorkspaceCreates.ts is a hand-rolled duplicate of the private getProjectTopLevelItems in useDashboardSidebarState.ts. If the two ever drift (e.g., a new visibility condition is added in one place but not the other), getPrependTabOrder will compute from a different item set than the rest of the ordering logic, silently placing new workspaces at the wrong position. Everything else in the PR is clean and mechanically correct. useWorkspaceCreates.ts — the duplicated filter logic is worth a second look before future changes to getProjectTopLevelItems.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/tabOrder.ts | New shared utility module exporting getPrependTabOrder and getNextTabOrder; logic is identical to what was previously local in useDashboardSidebarState. |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/index.ts | Re-exports the new tabOrder utilities; straightforward barrel-file change. |
| apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts | Removes local getNextTabOrder / getPrependTabOrder definitions and imports them from the shared module; no behaviour change. |
| apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts | Core fix: computes top-level items and calls getPrependTabOrder before inserting the new workspace local state row; duplicates the topLevelItems filter logic that already exists in getProjectTopLevelItems. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[useWorkspaceCreates: submit] --> B[workspaces.create API call]
B --> C{existing local record?}
C -- yes --> D[update paneLayout only\ntabOrder unchanged]
C -- no --> E[collect topLevelItems\nworkspaces sectionId=null\n+ sections for projectId]
E --> F[getPrependTabOrder\nmin tabOrder - 1]
F --> G[insert new workspace\nwith prepended tabOrder]
G --> H{alreadyExists and\nid mismatch?}
H -- yes --> I[remove optimistic\nin-flight entry]
H -- no --> J[return ok]
I --> J
subgraph dashboardSidebarLocal/tabOrder.ts
F2[getPrependTabOrder\nlen=0 → 1\nelse min-1]
G2[getNextTabOrder\nmax + 1]
end
useDashboardSidebarState --> F2
useDashboardSidebarState --> G2
useWorkspaceCreates --> F2
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts:90-102
**Duplicated top-level-items filter logic**
The workspace + section filtering here is a near-verbatim copy of the private `getProjectTopLevelItems` function in `useDashboardSidebarState.ts`. If that function's filter criteria evolve (e.g., a new visibility condition is added), this site will silently diverge, causing `getPrependTabOrder` to compute from a different set than everything else — leading to subtle ordering bugs. Consider extracting `getProjectTopLevelItems` (or a lightweight equivalent that accepts collections + projectId and returns `Array<{ tabOrder: number }>`) into the shared `dashboardSidebarLocal/` module alongside the tab-order utilities.
Reviews (1): Last reviewed commit: "fix(desktop): place new v2 workspace at ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const topLevelItems = [ | ||
| ...Array.from(collections.v2WorkspaceLocalState.state.values()) | ||
| .filter( | ||
| (item) => | ||
| item.sidebarState.projectId === projectId && | ||
| item.sidebarState.sectionId === null && | ||
| isSidebarWorkspaceVisible(item), | ||
| ) | ||
| .map((item) => ({ tabOrder: item.sidebarState.tabOrder })), | ||
| ...Array.from(collections.v2SidebarSections.state.values()) | ||
| .filter((item) => item.projectId === projectId) | ||
| .map((item) => ({ tabOrder: item.tabOrder })), | ||
| ]; |
There was a problem hiding this comment.
The workspace + section filtering here is a near-verbatim copy of the private getProjectTopLevelItems function in useDashboardSidebarState.ts. If that function's filter criteria evolve (e.g., a new visibility condition is added), this site will silently diverge, causing getPrependTabOrder to compute from a different set than everything else — leading to subtle ordering bugs. Consider extracting getProjectTopLevelItems (or a lightweight equivalent that accepts collections + projectId and returns Array<{ tabOrder: number }>) into the shared dashboardSidebarLocal/ module alongside the tab-order utilities.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/stores/workspace-creates/useWorkspaceCreates.ts
Line: 90-102
Comment:
**Duplicated top-level-items filter logic**
The workspace + section filtering here is a near-verbatim copy of the private `getProjectTopLevelItems` function in `useDashboardSidebarState.ts`. If that function's filter criteria evolve (e.g., a new visibility condition is added), this site will silently diverge, causing `getPrependTabOrder` to compute from a different set than everything else — leading to subtle ordering bugs. Consider extracting `getProjectTopLevelItems` (or a lightweight equivalent that accepts collections + projectId and returns `Array<{ tabOrder: number }>`) into the shared `dashboardSidebarLocal/` module alongside the tab-order utilities.
How can I resolve this? If you propose a fix, please make it concise.
) The revert of superset-sh#4120 (superset-sh#4135) restored a hardcoded `tabOrder: 0` in the v2 workspace.create path. Existing rows default to `tabOrder: 0` too, so new workspaces tied with them and ordering was non-deterministic. Restores the `getPrependTabOrder` helper from superset-sh#4120 as a shared util in `dashboardSidebarLocal/`, and uses it from `useWorkspaceCreates` so new rows land strictly above every existing top-level item — matching what the pending-row injection in `useDashboardSidebarData` already assumes.
1 participant
Summary
tabOrder: 0in the v2workspace.createpath. Existing rows also default to0, so new workspaces tied with them and didn't deterministically land at the top.getPrependTabOrderfrom feat(desktop): optimistic v2 workspace.create #4120 as a shared util indashboardSidebarLocal/, and calls it fromuseWorkspaceCreatesso new rows pickmin(existing tabOrder) - 1and always sit strictly above every other top-level item — which is what the pending-row injection inuseDashboardSidebarDataalready assumes (see comment atuseDashboardSidebarData.ts:24-26).useDashboardSidebarStatenow imports the helpers from the shared module instead of keeping local copies.Test plan
tabOrder >= 1rows).alreadyExists) — confirm we don't move the existing row.Summary by cubic
Ensure new v2 workspaces always appear at the top of the project sidebar by computing a prepend tab order instead of defaulting to 0. Fixes nondeterministic ordering when existing items also had
tabOrder: 0.getPrependTabOrder/getNextTabOrderutilities indashboardSidebarLocal/tabOrder.tsand re-exported.useWorkspaceCreatesnow setstabOrdertomin(top-level items) - 1, checking visible workspaces and sections in the project.useDashboardSidebarStatenow imports these helpers and removes local duplicates.Written for commit f1ae9fc. Summary will update on new commits.
Summary by CodeRabbit