Repository navigation
Move sidebar kind selection to titlebar menu - #5044
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe sidebar extension menu now displays SF Symbol images. The sidebar footer button is refactored to open a browser action instead of displaying a menu. Right-click menu presentation is integrated into the titlebar toggle button and minimal-mode sidebar controls, with context menu support enabled for the toggle-sidebar action slot. ChangesSidebar Menu and Right-Click Controls
Possibly Related PRs
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (15 passed)
✨ 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 |
Greptile SummaryThis PR moves sidebar kind selection (default workspace vs. extension-based sidebar) from the bottom puzzle button to a right-click menu on the titlebar sidebar toggle, and rewires the puzzle button to open the Sidebar Extensions browser pane directly.
Confidence Score: 4/5Safe to merge; the refactoring is internally consistent across all three sidebar-control surfaces and introduces no new state or concurrency concerns. The titlebar-toggle right-click wiring is added in both the full-chrome and minimal-mode paths and acceptsContextMenu is kept in sync. The puzzle-button pivot to the Extension Browser is clean. The two flagged items are both non-blocking: the accessibilityDescription nil on new menu icons is a cosmetic accessibility gap, and the Manage Extensions reachability gap in minimal mode is a design trade-off worth confirming intentional. Sources/Update/MinimalModeSidebarControls.swift — confirm that the Extensions browser is reachable from minimal mode without first opening the sidebar panel. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User: left-click titlebar sidebar button] --> B[Toggle sidebar open/closed]
C[User: right-click titlebar sidebar button] --> D[CmuxExtensionSidebarSelection.showMenu]
D --> E[Kind selector menu\nDefault Workspace / Extension-based]
E --> F[CmuxExtensionSidebarMenuTarget.selectProvider]
G[User: click puzzle button\nin sidebar footer] --> H[AppDelegate.openSidebarExtensionBrowser]
H --> I[Sidebar Extensions browser pane]
subgraph MinimalMode [Minimal Mode]
C2[right-click toggleSidebar slot] --> D
end
subgraph FullChrome [Full Chrome]
C3[right-click via TitlebarControlRightClickView] --> D
end
Reviews (1): Last reviewed commit: "Move sidebar kind selection to titlebar ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| item.representedObject = descriptor.id | ||
| item.target = CmuxExtensionSidebarMenuTarget.shared | ||
| item.state = selectedProviderId == descriptor.id ? .on : .off | ||
| item.image = NSImage(systemSymbolName: descriptor.systemImageName, accessibilityDescription: nil) |
There was a problem hiding this comment.
NSImage is created with accessibilityDescription: nil, which suppresses VoiceOver from reading out the icon for assistive-technology users navigating the menu. Since the menu item already carries a localised title, screen readers will still announce the text, but providing a matching description (or reusing the item title) is the correct pattern for NSMenuItem icons.
| item.image = NSImage(systemSymbolName: descriptor.systemImageName, accessibilityDescription: nil) | |
| item.image = NSImage(systemSymbolName: descriptor.systemImageName, accessibilityDescription: localizedTitle(for: descriptor)) |
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!
| case .toggleSidebar: | ||
| CmuxExtensionSidebarSelection.showMenu(anchorView: self, event: event) |
There was a problem hiding this comment.
After this PR the sidebar kind menu no longer contains a "Manage Extensions" entry, and the replacement path is the puzzle button in SidebarFooterButtons. That button lives inside the main sidebar panel, which is hidden in minimal mode. In minimal mode the right-click on the sidebar toggle only shows the kind selector, leaving no discoverable way to reach the Extensions browser without first switching out of minimal mode.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
12998-13014:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing i18n entries for
sidebar.extensions.browser.title
Sources/Resources/Localizable.xcstringscontainssidebar.extensions.browser.titlewith translations for ar, bs, da, de, en, es, fr, it, ja, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant. Add the same key toPrototypes/SettingsShellLab/Resources/Localizable.xcstringsandResources/InfoPlist.xcstrings(currentlykey MISSING) for every supported locale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/ContentView.swift` around lines 12998 - 13014, The localized key sidebar.extensions.browser.title used in ContentView must be added to the other resource string tables: add the same key and translations that exist in the main Localizable.xcstrings to both the SettingsShellLab Localizable.xcstrings and the InfoPlist.xcstrings files, replacing any "key MISSING" placeholders; ensure the key name exactly matches sidebar.extensions.browser.title and include entries for every supported locale present in the main file so runtime lookups and accessibility/safeHelp strings resolve correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 12998-13014: The localized key sidebar.extensions.browser.title
used in ContentView must be added to the other resource string tables: add the
same key and translations that exist in the main Localizable.xcstrings to both
the SettingsShellLab Localizable.xcstrings and the InfoPlist.xcstrings files,
replacing any "key MISSING" placeholders; ensure the key name exactly matches
sidebar.extensions.browser.title and include entries for every supported locale
present in the main file so runtime lookups and accessibility/safeHelp strings
resolve correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56000b93-a70b-4ab0-8a38-63920a944f99
📒 Files selected for processing (4)
Sources/ContentView.swiftSources/Update/MinimalModeSidebarControls.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/WindowDragHandleView.swift
This branch was successfully deployed
1 participant
Summary
Verification
./scripts/reload.sh --tag sbkind --swift-frontend-workaround --launchDogfood Checks
Summary by CodeRabbit