feat(desktop): v2 chrome — sidebar-hosted toggle/nav, sidebar OpenIn,…#3983
Merged
Conversation
… drop ResourceConsumption - Hide ResourceConsumption in the TopBar on v2 workspace routes. - When v2 + sidebar expanded, lift the sidebar out of the TopBar column so the TopBar starts to the right of it. Collapsed/closed sidebars stay inside the column so the TopBar runs full-width across the top. - Move SidebarToggle + NavigationControls into the expanded DashboardSidebar header (next to the macOS traffic lights). Bring them back into the TopBar when the sidebar is closed/collapsed. Pads aligned at 80px on Mac for consistent toggle position across both states. - Move V2WorkspaceOpenInButton into a top row of the v2 right sidebar; keep it in the TopBar only when the right sidebar is closed. Row collapses via empty:hidden when the button has nothing to render.
Contributor
|
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 (12)
📝 WalkthroughWalkthroughThis pull request restructures the dashboard layout for V2 cloud workspace support by conditionally lifting the workspace sidebar outside the TopBar, detecting platform via Electron TRPC to adjust macOS padding, and simplifying search bar logic by removing the ChangesV2 Cloud Layout Restructuring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing [Slack Agent](https://www.coderabbit.ai/agent): Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Contributor
Greptile SummaryThis PR refactors the v2 desktop chrome layout: the expanded left sidebar is lifted out of the TopBar's flex column so the TopBar only spans the remaining width, and Confidence Score: 4/5Safe to merge — all changes are UI layout adjustments with no logic regressions; only P2 style observations noted. No P0 or P1 findings. The layout lifting logic is symmetric between layout.tsx and TopBar.tsx. The two P2 comments cover a silent padding dependency and an unconditional hook call that is guarded at render-time — neither affects correctness. DashboardSidebarHeader.tsx — implicit 72+8=80px padding arithmetic; TopBar.tsx — unconditional useV2UserPreferences call on non-v2 routes.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsx | Extracts sidebarPanel into a variable and conditionally renders it outside vs inside the flex column based on v2 + expanded state; logic matches TopBar's sidebarHostsChrome derivation. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsx | Introduces sidebarHostsChrome to hide SidebarToggle/NavigationControls from the TopBar when the expanded v2 sidebar hosts them; splits V2WorkspaceOpenInButton rendering behind !isRightSidebarOpen; minor implicit padding dependency. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx | Adds a traffic-light-aware drag row hosting SidebarToggle + NavigationControls when the sidebar is expanded; paddingLeft of 72px + parent px-2 (8px) = 80px total, matching the TopBar pad. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx | Adds a top row with V2WorkspaceOpenInButton inside the right sidebar; empty:hidden correctly collapses the row when the button returns null. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DashboardLayout render] --> B{isV2CloudEnabled AND sidebar open AND not collapsed?}
B -- Yes sidebarOutsideColumn=true --> C[sidebarPanel rendered OUTSIDE flex column]
C --> D[TopBar: paddingLeft=16px, no SidebarToggle or NavControls]
C --> F[DashboardSidebarHeader: SidebarToggle + NavControls at 72+8=80px]
B -- No sidebarOutsideColumn=false --> G[sidebarPanel rendered INSIDE flex column]
G --> I[TopBar: paddingLeft=80px on Mac, shows SidebarToggle + NavControls]
A --> J{isV2WorkspaceRoute AND isRightSidebarOpen?}
J -- Yes --> K[V2WorkspaceOpenInButton in WorkspaceSidebar top row]
J -- No --> L[V2WorkspaceOpenInButton in TopBar]
A --> M{isV2WorkspaceRoute?}
M -- Yes --> N[ResourceConsumption hidden]
M -- No --> O[ResourceConsumption shown in TopBar]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx:212-218
**Implicit padding dependency — 72 + 8 ≠ comment**
The `72px` value only works as intended because the parent container carries `px-2` (8 px), making the effective inset 80 px — matching the TopBar's pad. This coupling is silent: if the parent's horizontal padding changes, the traffic-light alignment breaks with no obvious link to this constant. A short inline comment (e.g. `/* 72 + 8px (parent px-2) = 80px to clear traffic lights */`) would make the arithmetic explicit for future readers.
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsx:37
**`isRightSidebarOpen` read unconditionally — guards only at render time**
`useV2UserPreferences()` is called on every TopBar render, including routes that are not v2 workspace routes, even though `isRightSidebarOpen` is only used in the JSX condition `isV2WorkspaceRoute && !isRightSidebarOpen`. There's no correctness bug (the branch is properly gated), but if the preferences hook becomes expensive or its collection is not yet initialised on non-v2 routes, the unconditional read may add unnecessary overhead. Consider guarding the hook or noting the intentional early-read for the sake of hook rules.
Reviews (1): Last reviewed commit: "feat(desktop): add top border to v2 righ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Kitenite
force-pushed
the
hide-perf-measurer
branch
from
fd81bdc to
038c16a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsx`:
- Around line 212-218: The left padding for the drag row in
DashboardSidebarHeader is using 72px which misaligns with the TopBar's 80px
padding; update the inline style in the DashboardSidebarHeader component (the
div with className "drag flex h-8 items-center gap-1.5") to use paddingLeft:
isMac ? "80px" : "0px" so the SidebarToggle and NavigationControls stay aligned
when the sidebar changes state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 903ecf69-ec3d-4690-87eb-28d8ec03949a
📒 Files selected for processing (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarHeader/DashboardSidebarHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/layout.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx
…esourceConsumption on all v2 routes - Promote SidebarToggle and NavigationControls from TopBar/components/ to _dashboard/components/ since they're now shared with DashboardSidebarHeader (per AGENTS.md co-location rule). Update import sites. - Delete unused V2WorkspaceSearchBarTrigger. - Self-contain the toggle row's traffic-light pad in DashboardSidebarHeader (-mx-2 cancels parent px-2; row owns 80px on Mac, 8px otherwise) so the alignment doesn't depend on parent padding. - Hide ResourceConsumption on all v2 cloud routes, not just v2 workspace pages (matches the original "hide perf measurer in v2" intent). - Drop the V2WorkspaceOpenInButton-in-sidebar gate; OpenIn button stays in the TopBar for v2 routes regardless of right-sidebar state.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
…ual port superset-sh#3983 (v2 chrome sidebar-hosted toggle/nav) skipped permanently: the fork keeps its v1/v2/scratch/tearoff/KeepAliveWorkspaces dual-chrome structure deliberately, and the upstream PR rearranges the topbar + sidebar layout in a way that's incompatible with the fork's `!isTearoff && <TopBar />` / `!isTearoff && !isScratchRoute && isWorkspaceSidebarOpen` conditional rendering and `WorkspaceCreatesManager` mount. Adopting it would either delete fork-only chrome or fight the PR's sidebar lifting on every render. The cosmetic gain (sidebar- hosted toggle, ResourceConsumption hide on v2) is not worth the structural churn; the fork will evolve its v2 chrome on its own. superset-sh#4211 (pick-workspace prompt) ported manually instead of cherry-picking since superset-sh#4211 sits on top of superset-sh#3983's layout. The behaviour port is small and self-contained: - new components/CrossVersionMismatchState/{CrossVersionMismatchState.tsx,index.ts} copied verbatim from upstream (clean cherry-pick of the two new files via `git checkout 56a1f50 -- ...`) - layout.tsx: add v2WorkspaceMatch + onV1WorkspaceRoute + onV2WorkspaceRoute + versionMismatch derivation alongside the existing currentWorkspaceMatch / isV2CloudEnabled - layout.tsx: wrap the inner <KeepAliveWorkspaces /> in `{versionMismatch ? <CrossVersionMismatchState /> : <KeepAliveWorkspaces />}` so cross-version routes show the picker prompt instead of rendering an empty page from the wrong-version sidebar. KeepAliveWorkspaces / WorkspaceCreatesManager / isTearoff / isScratchRoute unchanged: cross-version mismatch is a Bad State that bypasses the keep-alive flow entirely (the user has to pick a fresh workspace anyway).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… drop ResourceConsumption
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by cubic
Update v2 desktop chrome so the expanded left sidebar hosts the toggle + navigation and the TopBar starts to its right. Open In now stays in the TopBar on v2 routes, and ResourceConsumption is hidden across all v2 pages.
New Features
SidebarToggle+NavigationControlsnext to macOS traffic lights; they return to the TopBar when the sidebar is closed/collapsed. Mac padding defaults while loading and the row self-owns the 80px pad for consistent placement.Refactors
SidebarToggleandNavigationControlsto shared_dashboard/components/; removed unusedV2WorkspaceSearchBarTrigger.Written for commit 7c3058a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Refactor