feat(desktop): re-add Settings to org dropdown for v1#4093
Merged
Conversation
Settings was removed from the OrganizationDropdown when it moved to the sidebar footer. v1 still relies on the dropdown, so add it back with a TODO to remove once v1 is gone. Item appears in both the topbar dropdown and the sidebar header dropdown.
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 (1)
📝 WalkthroughWalkthroughAdds a "Settings" entry to the OrganizationDropdown menu: imports a cog icon and the ChangesOrganization Dropdown Settings Item
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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)
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 |
Contributor
Greptile Summary
Confidence Score: 4/5Safe to merge; only a style-level duplication of an existing abstraction. The change is straightforward and functionally correct. The only issue is that the inline hotkey-display pattern duplicates the existing HotkeyMenuShortcut component, which is a P2 style concern. No files require special attention beyond the single changed file.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OrganizationDropdown/OrganizationDropdown.tsx | Adds a Settings menu item with hotkey shortcut to OrganizationDropdown for v1 compatibility; the hotkey display logic duplicates the existing HotkeyMenuShortcut abstraction. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[OrganizationDropdown rendered] --> B{variant?}
B -->|topbar| C[TopBar]
B -->|expanded| D[DashboardSidebarHeader expanded]
B -->|collapsed| E[DashboardSidebarHeader collapsed]
C & D & E --> F[DropdownMenu opens]
F --> G[Settings item - NEW]
G --> H{settingsHotkey !== 'Unassigned'?}
H -->|yes| I[Render DropdownMenuShortcut]
H -->|no| J[No shortcut shown]
I & J --> K[onSelect → navigate to /settings/account]
F --> L[Manage members]
F --> M[Switch organization]
F --> N[Log out]
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/TopBar/components/OrganizationDropdown/OrganizationDropdown.tsx:25
**Duplicates existing `HotkeyMenuShortcut` abstraction**
`HotkeyMenuShortcut` in `renderer/components/HotkeyMenuShortcut` already encapsulates the `useHotkeyDisplay` call, the `"Unassigned"` guard, and the `DropdownMenuShortcut` render. The inline expansion here duplicates that logic and leaves the `settingsHotkey` variable unused after the refactor.
```suggestion
import { HotkeyMenuShortcut } from "renderer/components/HotkeyMenuShortcut/HotkeyMenuShortcut";
```
### Issue 2 of 2
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OrganizationDropdown/OrganizationDropdown.tsx:139-147
**Use `HotkeyMenuShortcut` instead of inline expansion**
The codebase has a dedicated `HotkeyMenuShortcut` component that handles the `useHotkeyDisplay` + `"Unassigned"` guard + `DropdownMenuShortcut` pattern. Using it here removes the need for the `settingsHotkey` local variable and keeps the pattern consistent across the app.
```suggestion
<DropdownMenuItem
onSelect={() => navigate({ to: "/settings/account" })}
>
<HiOutlineCog6Tooth className="h-4 w-4" />
<span>Settings</span>
<HotkeyMenuShortcut hotkeyId="OPEN_SETTINGS" />
</DropdownMenuItem>
```
Reviews (1): Last reviewed commit: "feat(desktop): re-add Settings entry to ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| HiOutlineCog6Tooth, | ||
| } from "react-icons/hi2"; | ||
| import { useCurrentPlan } from "renderer/hooks/useCurrentPlan"; | ||
| import { useHotkeyDisplay } from "renderer/hotkeys"; |
Contributor
There was a problem hiding this comment.
HotkeyMenuShortcut abstraction
HotkeyMenuShortcut in renderer/components/HotkeyMenuShortcut already encapsulates the useHotkeyDisplay call, the "Unassigned" guard, and the DropdownMenuShortcut render. The inline expansion here duplicates that logic and leaves the settingsHotkey variable unused after the refactor.
Suggested change
| import { useHotkeyDisplay } from "renderer/hotkeys"; | |
| import { HotkeyMenuShortcut } from "renderer/components/HotkeyMenuShortcut/HotkeyMenuShortcut"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OrganizationDropdown/OrganizationDropdown.tsx
Line: 25
Comment:
**Duplicates existing `HotkeyMenuShortcut` abstraction**
`HotkeyMenuShortcut` in `renderer/components/HotkeyMenuShortcut` already encapsulates the `useHotkeyDisplay` call, the `"Unassigned"` guard, and the `DropdownMenuShortcut` render. The inline expansion here duplicates that logic and leaves the `settingsHotkey` variable unused after the refactor.
```suggestion
import { HotkeyMenuShortcut } from "renderer/components/HotkeyMenuShortcut/HotkeyMenuShortcut";
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+139
to
+147
| <DropdownMenuItem | ||
| onSelect={() => navigate({ to: "/settings/account" })} | ||
| > | ||
| <HiOutlineCog6Tooth className="h-4 w-4" /> | ||
| <span>Settings</span> | ||
| {settingsHotkey !== "Unassigned" && ( | ||
| <DropdownMenuShortcut>{settingsHotkey}</DropdownMenuShortcut> | ||
| )} | ||
| </DropdownMenuItem> |
Contributor
There was a problem hiding this comment.
HotkeyMenuShortcut instead of inline expansion
The codebase has a dedicated HotkeyMenuShortcut component that handles the useHotkeyDisplay + "Unassigned" guard + DropdownMenuShortcut pattern. Using it here removes the need for the settingsHotkey local variable and keeps the pattern consistent across the app.
Suggested change
| <DropdownMenuItem | |
| onSelect={() => navigate({ to: "/settings/account" })} | |
| > | |
| <HiOutlineCog6Tooth className="h-4 w-4" /> | |
| <span>Settings</span> | |
| {settingsHotkey !== "Unassigned" && ( | |
| <DropdownMenuShortcut>{settingsHotkey}</DropdownMenuShortcut> | |
| )} | |
| </DropdownMenuItem> | |
| <DropdownMenuItem | |
| onSelect={() => navigate({ to: "/settings/account" })} | |
| > | |
| <HiOutlineCog6Tooth className="h-4 w-4" /> | |
| <span>Settings</span> | |
| <HotkeyMenuShortcut hotkeyId="OPEN_SETTINGS" /> | |
| </DropdownMenuItem> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/OrganizationDropdown/OrganizationDropdown.tsx
Line: 139-147
Comment:
**Use `HotkeyMenuShortcut` instead of inline expansion**
The codebase has a dedicated `HotkeyMenuShortcut` component that handles the `useHotkeyDisplay` + `"Unassigned"` guard + `DropdownMenuShortcut` pattern. Using it here removes the need for the `settingsHotkey` local variable and keeps the pattern consistent across the app.
```suggestion
<DropdownMenuItem
onSelect={() => navigate({ to: "/settings/account" })}
>
<HiOutlineCog6Tooth className="h-4 w-4" />
<span>Settings</span>
<HotkeyMenuShortcut hotkeyId="OPEN_SETTINGS" />
</DropdownMenuItem>
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
saddlepaddle
pushed a commit
that referenced
this pull request
May 6, 2026
* feat(desktop): re-add Settings entry to org dropdown for v1 Settings was removed from the OrganizationDropdown when it moved to the sidebar footer. v1 still relies on the dropdown, so add it back with a TODO to remove once v1 is gone. Item appears in both the topbar dropdown and the sidebar header dropdown. * refactor(desktop): use HotkeyMenuShortcut helper in org dropdown
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
* feat(desktop): re-add Settings entry to org dropdown for v1 Settings was removed from the OrganizationDropdown when it moved to the sidebar footer. v1 still relies on the dropdown, so add it back with a TODO to remove once v1 is gone. Item appears in both the topbar dropdown and the sidebar header dropdown. * refactor(desktop): use HotkeyMenuShortcut helper in org dropdown
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 21, 2026
…o-v2 superset-sh#4113 remove skip-all onboarding affordance: integrate fork's githubSyncService import alongside upstream's selectExternalWorktreesForImport in workspaces/procedures/git-status.ts. superset-sh#4115 default new users to v2 + v2 banner in v1: - Adopt upstream's V2_CLOUD flag removal: collapse useIsV2CloudEnabled to a plain boolean (drop isRemoteV2Enabled) and update every callsite back to scalar destructure (TopBar, settings/{behavior,experimental, git,links,terminal}/page, SettingsSidebar/{GeneralSettings,SettingsSidebar}). - ExperimentalSettings.tsx: drop isRemoteV2Enabled-gated 'Early access not enabled' note and Switch disabled prop; simplify track call. - v2-local-override.ts: keep fork's IS_DEV strong default (dev installs land on v2) alongside upstream's hasPriorSupersetUsage probe (initialOptInV2 = IS_DEV ? true : !hasPriorSupersetUsage()). - TopBar.tsx: integrate upstream's useWorkspaceSidebarStore reads with fork's existing chrome layout. - useMigrateV1DataToV2/useMigrateV1DataToV2.ts: re-delete (DU; upstream attempted to revive via superset-sh#4115 vocabulary cleanup; cycle 38 rationale still applies — fork trpc lacks the required procedures). superset-sh#4125 onboarding auto-skip drop, SetupButton design alignment: clean cherry-pick. superset-sh#4175 keep Skip-for-now visible during loading: clean cherry-pick. Replaces v2Projects live query with imperative v2Project.list trpc call; backfill list procedure (jwtProcedure variant) into fork's v2-project router (mirrors get pattern using ctx.organizationIds membership check). superset-sh#4214 stable Adopt button + Adopt-all (skipped): touches v1 ImportWorkspacesPage/V1ImportModal which the fork has removed. The upstream improvements (idempotent adopt-all guard, smarter skip-when- already-running) have no shipping surface in this fork; pick up if and when v1 import modal is reintroduced. superset-sh#4093 re-add Settings to org dropdown for v1: clean cherry-pick.
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.
Summary
OrganizationDropdownso v1 users can reach/settings/accountfrom the org menu (it was removed when Settings moved to the v2 sidebar footer).TODO(v1)comment so it can be removed once v1 is gone.TopBar) and the sidebar header (DashboardSidebarHeader, collapsed + expanded).Test plan
OPEN_SETTINGShotkey shortcut and navigates to/settings/account.bun run typecheckandbun run lintare clean.Summary by cubic
Re-added a Settings item to the organization dropdown so v1 users can open
/settings/accountfrom the menu again. It appears in both the topbar and sidebar header dropdowns, shows theOPEN_SETTINGSshortcut viaHotkeyMenuShortcut, and is taggedTODO(v1)for removal after v1.Written for commit c30e646. Summary will update on new commits.
Summary by CodeRabbit