Repository navigation
Open extension browser as pane tab - #5038
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughThis PR implements a sidebar extension browser panel type integrated into the workspace/pane tab system. It replaces the floating presenter API with pane-tab routing, adds a new panel model and card container for embedding the browser, refactors continuation coordination, and extends UI recognition across command palette, search, history, events, and drag/drop systems. ChangesSidebar Extension Browser Panel System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 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 |
azooz2003-bit
force-pushed
the
feat-extension-browser-tab
branch
from
May 31, 2026 00:46
3807273 to
ee3fca9
Compare
Contributor
Greptile SummaryThis PR replaces the floating
Confidence Score: 4/5Safe to merge after adding deduplication to openSidebarExtensionBrowser — without it every tap on "Manage Sidebar Extensions" accumulates an unbounded number of identical tabs. The structural work — new PanelType case, AppKit container, session-restore exclusion, and full i18n — is solid. The one functional regression is that Sources/AppDelegate.swift needs a deduplication guard mirroring the Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant SidebarHostView as CMUXInstalledExtensionSidebarHostView
participant AppDelegate
participant Workspace
participant Panel as CMUXSidebarExtensionBrowserPanel
participant BonsplitController
User->>SidebarHostView: tap "Manage Sidebar Extensions"
SidebarHostView->>AppDelegate: openSidebarExtensionBrowser(from:title:)
AppDelegate->>AppDelegate: synchronizeActiveMainWindowContext()
AppDelegate->>Workspace: newSidebarExtensionBrowserSurface(inPane:title:focus:)
Workspace->>Panel: init(title:)
Panel->>Panel: CMUXSidebarExtensionBrowserPresenter.makeViewController(title:)
Workspace->>Workspace: "panels[id] = panel"
Workspace->>BonsplitController: createTab(...)
BonsplitController-->>Workspace: newTabId
Workspace->>Workspace: "surfaceIdToPanelId[newTabId] = panel.id"
Workspace->>BonsplitController: focusPane / selectTab
Workspace->>Workspace: applyTabSelection(...)
Workspace-->>AppDelegate: extensionBrowserPanel
AppDelegate-->>SidebarHostView: UUID?
Reviews (2): Last reviewed commit: "Open extension browser as pane tab" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1497
to
+1498
| cardView.layer?.backgroundColor = NSColor.windowBackgroundColor.withAlphaComponent(Self.backgroundAlpha).cgColor | ||
| cardView.layer?.cornerRadius = Self.cornerRadius |
Contributor
There was a problem hiding this comment.
CGColor on appearance switch
NSColor.windowBackgroundColor.withAlphaComponent(...).cgColor resolves to a static CGColor at the moment of the call; it does not track the system appearance dynamically. When the user switches between Light and Dark mode, cardView.layer?.backgroundColor will stay on the colour from the previous appearance until the next time updateLayoutForCurrentBounds is triggered by layout or a SwiftUI update. CMUXSidebarExtensionBrowserContainerViewController needs to override viewDidChangeEffectiveAppearance() and call updateLayoutForCurrentBounds() (or re-resolve the CGColor there) so the card repaints correctly on every appearance change.
Comment on lines
+10
to
+26
| struct MainWindowBootstrapView: View { | ||
| var body: some View { | ||
| Color.clear | ||
| .frame(width: 1, height: 1) | ||
| .background(WindowAccessor { window in | ||
| window.identifier = NSUserInterfaceItemIdentifier("cmux.bootstrap") | ||
| window.isRestorable = false | ||
| window.orderOut(nil) | ||
| Task { @MainActor [weak window] in | ||
| window?.orderOut(nil) | ||
| window?.close() | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
|
|
Contributor
There was a problem hiding this comment.
MainWindowBootstrapView and cmuxAuxiliaryWindowIdentifiers were private struct / private let in cmuxApp.swift. After extraction to this file they have no access modifier, making them internal and accessible from anywhere in the module. This is an unintentional visibility widening — these implementation-detail types should be marked private (used only within this file) or fileprivate if they're shared with other helpers.
Comment on lines
+1590
to
+1601
| func updateLayoutForCurrentBounds() { | ||
| cardWidthConstraint?.constant = Self.width(for: rootView.bounds.width) | ||
| cardHeightConstraint?.constant = Self.height(for: rootView.bounds.height) | ||
| cardTopConstraint?.constant = Self.topInset | ||
| cardBottomSafetyConstraint?.constant = -Self.bottomInset | ||
| cardHorizontalSafetyConstraints.first?.constant = Self.sideInset | ||
| cardHorizontalSafetyConstraints.dropFirst().first?.constant = -Self.sideInset | ||
|
|
||
| cardView.layer?.backgroundColor = NSColor.windowBackgroundColor.withAlphaComponent(Self.backgroundAlpha).cgColor | ||
| cardView.layer?.cornerRadius = Self.cornerRadius | ||
| browserViewController.view.layer?.cornerRadius = Self.cornerRadius | ||
| } |
Contributor
There was a problem hiding this comment.
viewDidChangeEffectiveAppearance() so the card's CGColor repaints when the system appearance changes. Without this, the CALayer.backgroundColor set in loadView / updateLayoutForCurrentBounds remains pinned to the colour from the original appearance for the lifetime of the view controller.
Suggested change
| func updateLayoutForCurrentBounds() { | |
| cardWidthConstraint?.constant = Self.width(for: rootView.bounds.width) | |
| cardHeightConstraint?.constant = Self.height(for: rootView.bounds.height) | |
| cardTopConstraint?.constant = Self.topInset | |
| cardBottomSafetyConstraint?.constant = -Self.bottomInset | |
| cardHorizontalSafetyConstraints.first?.constant = Self.sideInset | |
| cardHorizontalSafetyConstraints.dropFirst().first?.constant = -Self.sideInset | |
| cardView.layer?.backgroundColor = NSColor.windowBackgroundColor.withAlphaComponent(Self.backgroundAlpha).cgColor | |
| cardView.layer?.cornerRadius = Self.cornerRadius | |
| browserViewController.view.layer?.cornerRadius = Self.cornerRadius | |
| } | |
| override func viewDidChangeEffectiveAppearance() { | |
| super.viewDidChangeEffectiveAppearance() | |
| updateLayoutForCurrentBounds() | |
| } | |
| func updateLayoutForCurrentBounds() { | |
| cardWidthConstraint?.constant = Self.width(for: rootView.bounds.width) | |
| cardHeightConstraint?.constant = Self.height(for: rootView.bounds.height) | |
| cardTopConstraint?.constant = Self.topInset | |
| cardBottomSafetyConstraint?.constant = -Self.bottomInset | |
| cardHorizontalSafetyConstraints.first?.constant = Self.sideInset | |
| cardHorizontalSafetyConstraints.dropFirst().first?.constant = -Self.sideInset | |
| cardView.layer?.backgroundColor = NSColor.windowBackgroundColor.withAlphaComponent(Self.backgroundAlpha).cgColor | |
| cardView.layer?.cornerRadius = Self.cornerRadius | |
| browserViewController.view.layer?.cornerRadius = Self.cornerRadius | |
| } |
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/cmuxAppSettingsSupportViews.swift (1)
1-873: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit this new settings support file before merge.
This new
Sources/file is already 873 lines and bundles reusable card/picker primitives, section-specific settings rows, window/root navigation, and mutable draft state. Please extract at least the reusable controls and the root-navigation/state pieces into separate files so future settings changes do not keep compounding here. As per coding guidelines "A new production Swift file must not exceed 400 lines without a clear single responsibility, or 800 lines even when the responsibility is mostly coherent" and "Do not mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one Swift file."🤖 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/cmuxAppSettingsSupportViews.swift` around lines 1 - 873, This file is too large and mixes reusable UI primitives with root navigation/state; split it into focused files by extracting reusable controls (e.g. SettingsCard, SettingsCardRow, SettingsPickerRow, SettingsCardDivider, ThemeWindowThumbnail, applyIf View extension, SettingsConfigurationReview and related enums) into one or more UI component files, and move root/navigation and state pieces (e.g. SettingsWindowRootView, SettingsRootView, SettingsDraftState, SettingsDraftState.syncBrowserInsecureHTTPAllowlistFromSavedValue, SettingsSidebarEntryRow, and SettingsSearch-related bindings/logic) into a separate SettingsRoot/State file; ensure each new file declares the same types and imports used here, update any internal access control if needed, and keep each file under ~400 lines with single responsibility while preserving all references (e.g. ThemePickerRow, AppIconPickerRow, GlobalHotkeySection still import and use the extracted components).Sources/CMUXInstalledExtensionSidebarHostView.swift (1)
1012-1044: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReplace
NSLockwith actor isolation inMonitorContinuationBox.
Sources/CMUXInstalledExtensionSidebarHostView.swiftdefinesprivate final class MonitorContinuationBox:@uncheckedSendablethat usesprivate let lock = NSLock()to guardcontinuation/isCancelled; this violates the “no locks in new Swift code” guideline—switch the box to anactor(or equivalent actor-isolated state) and remove the lock and@unchecked Sendable.🤖 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/CMUXInstalledExtensionSidebarHostView.swift` around lines 1012 - 1044, Replace the lock-guarded class MonitorContinuationBox with an actor: remove `@unchecked Sendable`, delete `private let lock = NSLock()`, and make `MonitorContinuationBox` an `actor` that holds `var continuation: CheckedContinuation<Void, Never>?` and `var isCancelled = false`; implement actor-isolated methods `set(_:)`, `resume()`, and `cancel()` that perform the same logic (check and set `isCancelled`, store/clear `continuation`, and call `continuation?.resume()`), so all state is protected by actor isolation instead of `NSLock`.
🤖 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.
Inline comments:
In `@Sources/ClosedItemHistory.swift`:
- Around line 717-718: The new localized key "sidebar.extensions.browser.title"
used in ClosedItemHistory.swift (case .extensionBrowser) is missing entries in
Resources/Localizable.xcstrings for multiple locales; add the key with
appropriate translated values for each missing locale (ar, bs, da, de, es, fr,
it, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant) to
Resources/Localizable.xcstrings so that String(localized:
"sidebar.extensions.browser.title", defaultValue: "Sidebar Extensions") resolves
for all supported languages; ensure the same key string is present in each
locale block and follow the existing file formatting and quoting conventions.
In `@Sources/cmuxAppSettingsSupportViews.swift`:
- Around line 38-39: The NotificationCenter publishers in the onReceive handlers
call reload(), syncFromDefaults(), and navigate(...) which may run off the main
thread; wrap those calls in an explicit main-actor hop (for example use Task {
await MainActor.run { reload() } } or DispatchQueue.main.async { reload() }) so
state mutations happen on the main thread; update the onReceive blocks that call
reload(), syncFromDefaults(), and navigate(...) (and the other handlers in this
file noted in the review) to perform their work inside the main-actor hop.
- Around line 535-537: The ThemePickerRow and AppIconPickerRow Button controls
currently call .focusable(false) which prevents keyboard focus; remove the
.focusable(false) modifier from both ThemePickerRow and AppIconPickerRow (the
Button chain where .buttonStyle(.plain) and .accessibilityAddTraits(...) are
applied) so the buttons remain keyboard-focusable and retain the existing
.buttonStyle and accessibility traits.
- Line 25: Replace the C-style formatting in the Text view so the displayed
recordCount uses locale-aware number formatting; instead of Text(String(format:
"%d", recordCount)) update the Text that references recordCount (the Text(...)
call in cmuxAppSettingsSupportViews.swift) to use Swift’s locale-aware
formatting APIs such as Text(recordCount, format: .number) or
Text(recordCount.formatted(.number)) so grouping/decimal rules follow the user
locale.
In `@Sources/CMUXInstalledExtensionSidebarHostView.swift`:
- Around line 1368-1636: The new extension-browser panel types
(CMUXSidebarExtensionBrowserPanel, CMUXSidebarExtensionBrowserPanelView, and
CMUXSidebarExtensionBrowserContainerViewController) should be moved into their
own Swift source file: create a new file and cut these three type definitions
out of the oversized host file, preserving `@MainActor` and access levels
(final/private as declared), necessary imports (AppKit/SwiftUI), and any helper
types they reference; update any callers/imports to reference the moved types
(no API changes) and run a build to fix any missing symbol errors, adjusting
visibility if the compiler reports access issues.
- Around line 1496-1515: The card is being sized from layoutMetrics(for:
rootView.bounds.size) when rootView.bounds may be .zero, causing fixed
height/top/bottom constraints that conflict; update the sizing so the card is
clamped to the available pane instead of forcing a constant size: compute
metrics from the actual available size during layout (e.g., recalc in
viewDidLayout or updateConstraints using layoutMetrics(for:
rootView.bounds.size)) and change the fixed constraints on cardView
(cardWidthConstraint, cardHeightConstraint) to use inequality constraints
(constraint(lessThanOrEqualToConstant:) and
constraint(greaterThanOrEqualToConstant:) or
heightAnchor.constraint(lessThanOrEqualTo:rootView.heightAnchor, constant:
-insets)) and/or lower priority for the fixed constant constraints so the card
will shrink to fit the pane; apply the same pattern where layoutMetrics is used
later (around the block that creates cardTopConstraint,
cardBottomSafetyConstraint and the similar section at lines ~1614-1635).
In `@Sources/Workspace.swift`:
- Around line 13866-13870: Add a Swift-DocC comment above the
newSidebarExtensionBrowserSurface(inPane:title:focus:) function describing its
purpose (creating/returning a CMUXSidebarExtensionBrowserPanel for a given
PaneID), document parameters (paneId: PaneID, title: String, focus: Bool with
default true) and return value (optional CMUXSidebarExtensionBrowserPanel), and
note the focus behavior and any session-exclusion semantics so callers
understand when the panel will be focused and when it may be omitted due to
session rules.
---
Outside diff comments:
In `@Sources/cmuxAppSettingsSupportViews.swift`:
- Around line 1-873: This file is too large and mixes reusable UI primitives
with root navigation/state; split it into focused files by extracting reusable
controls (e.g. SettingsCard, SettingsCardRow, SettingsPickerRow,
SettingsCardDivider, ThemeWindowThumbnail, applyIf View extension,
SettingsConfigurationReview and related enums) into one or more UI component
files, and move root/navigation and state pieces (e.g. SettingsWindowRootView,
SettingsRootView, SettingsDraftState,
SettingsDraftState.syncBrowserInsecureHTTPAllowlistFromSavedValue,
SettingsSidebarEntryRow, and SettingsSearch-related bindings/logic) into a
separate SettingsRoot/State file; ensure each new file declares the same types
and imports used here, update any internal access control if needed, and keep
each file under ~400 lines with single responsibility while preserving all
references (e.g. ThemePickerRow, AppIconPickerRow, GlobalHotkeySection still
import and use the extracted components).
In `@Sources/CMUXInstalledExtensionSidebarHostView.swift`:
- Around line 1012-1044: Replace the lock-guarded class MonitorContinuationBox
with an actor: remove `@unchecked Sendable`, delete `private let lock =
NSLock()`, and make `MonitorContinuationBox` an `actor` that holds `var
continuation: CheckedContinuation<Void, Never>?` and `var isCancelled = false`;
implement actor-isolated methods `set(_:)`, `resume()`, and `cancel()` that
perform the same logic (check and set `isCancelled`, store/clear `continuation`,
and call `continuation?.resume()`), so all state is protected by actor isolation
instead of `NSLock`.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e6b591b-2cbe-48fb-853e-1a233f329333
📒 Files selected for processing (18)
Packages/CMUXExtensionClient/Sources/CMUXExtensionClient/Browser/CMUXSidebarExtensionBrowserPresenter.swiftSources/AppDelegate.swiftSources/CMUXInstalledExtensionSidebarHostView.swiftSources/ClosedItemHistory.swiftSources/CmuxLifecycleEventPublishing.swiftSources/ContentView.swiftSources/Panels/Panel.swiftSources/Panels/PanelContentView.swiftSources/Search/GlobalSearchDocuments.swiftSources/TerminalPaneDropTargetView.swiftSources/Workspace.swiftSources/cmuxApp.swiftSources/cmuxAppAuxiliaryViews.swiftSources/cmuxAppSettingsSupportViews.swiftSources/cmuxAppSettingsViews.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspaceUnitTests.swiftscripts/reload.sh
| searchAnchorID: SettingsSearchIndex.settingID(for: .terminal, idSuffix: "resume-commands") | ||
| ) { | ||
| HStack(spacing: 8) { | ||
| Text(String(format: "%d", recordCount)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the exact referenced line context
FILE="Sources/cmuxAppSettingsSupportViews.swift"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "---- $FILE (lines 1-120) ----"
nl -ba "$FILE" | sed -n '1,120p'
echo "---- $FILE (around line 25) ----"
nl -ba "$FILE" | sed -n '15,40p'
# 2) Find all occurrences of recordCount and number formatting usage
echo "---- Usages of recordCount in $FILE ----"
rg -n "recordCount" "$FILE" || true
echo "---- Usages of Text(..., format: .number) in Sources ----"
rg -n "Text\\([^\\)]*format:\\s*\\.number" Sources || true
# 3) Confirm whether String(format: "%d", ...) is used elsewhere
echo "---- Usages of String(format: \"%d\", ...) in Sources ----"
rg -n "String\\(format:\\s*\"%d\"" Sources || trueRepository: manaflow-ai/cmux
Length of output: 188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Sources/cmuxAppSettingsSupportViews.swift"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "---- $FILE (lines 1-120) ----"
python3 - <<PY
from pathlib import Path
p = Path("$FILE")
lines = p.read_text(encoding="utf-8").splitlines()
for i in range(1, min(120, len(lines)) + 1):
print(f"{i:5d}\t{lines[i-1]}")
PY
echo "---- $FILE (around line 25) ----"
python3 - <<PY
from pathlib import Path
p = Path("$FILE")
lines = p.read_text(encoding="utf-8").splitlines()
start, end = 15, 40
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:5d}\t{lines[i-1]}")
PY
echo "---- Usages of recordCount in $FILE ----"
rg -n "recordCount" "$FILE" || true
echo "---- Usages of Text(..., format: .number) in Sources ----"
rg -n "Text\\([^\\)]*format:\\s*\\.number" Sources || true
echo "---- Usages of String(format: \"%d\", ...) in Sources ----"
rg -n "String\\(format:\\s*\"%d\"" Sources || trueRepository: manaflow-ai/cmux
Length of output: 6858
Use locale-aware number formatting for recordCount
File: Sources/cmuxAppSettingsSupportViews.swift
Lines: 25-25
Text(String(format: "%d", recordCount)) uses C-style formatting and bypasses locale-aware digit/grouping rules for a user-visible Settings count.
Proposed fix
- Text(String(format: "%d", recordCount))
+ Text(recordCount, format: .number)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Text(String(format: "%d", recordCount)) | |
| Text(recordCount, format: .number) |
🤖 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/cmuxAppSettingsSupportViews.swift` at line 25, Replace the C-style
formatting in the Text view so the displayed recordCount uses locale-aware
number formatting; instead of Text(String(format: "%d", recordCount)) update the
Text that references recordCount (the Text(...) call in
cmuxAppSettingsSupportViews.swift) to use Swift’s locale-aware formatting APIs
such as Text(recordCount, format: .number) or
Text(recordCount.formatted(.number)) so grouping/decimal rules follow the user
locale.
Comment on lines
+38
to
+39
| .onReceive(NotificationCenter.default.publisher(for: SurfaceResumeApprovalStore.didChangeNotification)) { _ in | ||
| reload() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Marshal notification-driven state updates onto the main actor.
These onReceive(NotificationCenter.default.publisher(...)) handlers mutate @State/@SceneStorage directly, but NotificationCenter publishers deliver on the posting thread. The supplied SurfaceResumeApprovalStore snippet posts with a plain NotificationCenter.default.post(...), so a background writer can end up updating SwiftUI state off-main. Please add an explicit main-thread hop before calling reload(), syncFromDefaults(), and navigate(...).
Example pattern
- .onReceive(NotificationCenter.default.publisher(for: SurfaceResumeApprovalStore.didChangeNotification)) { _ in
+ .onReceive(
+ NotificationCenter.default
+ .publisher(for: SurfaceResumeApprovalStore.didChangeNotification)
+ .receive(on: RunLoop.main)
+ ) { _ in
reload()
}Apply the same pattern to the other notification handlers in this file.
Also applies to: 705-706, 819-824
🤖 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/cmuxAppSettingsSupportViews.swift` around lines 38 - 39, The
NotificationCenter publishers in the onReceive handlers call reload(),
syncFromDefaults(), and navigate(...) which may run off the main thread; wrap
those calls in an explicit main-actor hop (for example use Task { await
MainActor.run { reload() } } or DispatchQueue.main.async { reload() }) so state
mutations happen on the main thread; update the onReceive blocks that call
reload(), syncFromDefaults(), and navigate(...) (and the other handlers in this
file noted in the review) to perform their work inside the main-actor hop.
Comment on lines
+535
to
+537
| .buttonStyle(.plain) | ||
| .focusable(false) | ||
| .accessibilityAddTraits(isSelected ? .isSelected : []) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Sources/cmuxAppSettingsSupportViews.swift"
echo "== file = $file =="
wc -l "$file"
# Show the relevant sections around the reported line ranges
sed -n '500,570p' "$file" | cat -n
echo "----"
sed -n '600,670p' "$file" | cat -nRepository: manaflow-ai/cmux
Length of output: 7341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Sources/cmuxAppSettingsSupportViews.swift"
# Locate the exact Button wrapper context for the focusable(false) lines.
# (We print a wider window so we can see whether modifiers apply to the Button itself
# or only to its label.)
rg -n "struct (ThemePickerRow|AppIconPickerRow)|focusable\\(false\\)" "$file"
echo "---- ThemePickerRow area around focusable(false) ----"
sed -n '460,555p' "$file" | cat -n
echo "---- AppIconPickerRow area around focusable(false) ----"
sed -n '580,660p' "$file" | cat -nRepository: manaflow-ai/cmux
Length of output: 10192
Keep theme/app-icon option buttons keyboard-focusable
ThemePickerRow and AppIconPickerRow are Button-based selection controls, but both explicitly disable focus via .focusable(false), making the choices unreachable for keyboard users.
Minimal fix
- .focusable(false)
.accessibilityAddTraits(isSelected ? .isSelected : [])Apply the same removal to the corresponding .focusable(false) in AppIconPickerRow as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .buttonStyle(.plain) | |
| .focusable(false) | |
| .accessibilityAddTraits(isSelected ? .isSelected : []) | |
| .buttonStyle(.plain) | |
| .accessibilityAddTraits(isSelected ? .isSelected : []) |
🤖 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/cmuxAppSettingsSupportViews.swift` around lines 535 - 537, The
ThemePickerRow and AppIconPickerRow Button controls currently call
.focusable(false) which prevents keyboard focus; remove the .focusable(false)
modifier from both ThemePickerRow and AppIconPickerRow (the Button chain where
.buttonStyle(.plain) and .accessibilityAddTraits(...) are applied) so the
buttons remain keyboard-focusable and retain the existing .buttonStyle and
accessibility traits.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/CMUXInstalledExtensionSidebarHostView.swift (1)
671-679:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't make the empty-state Manage action depend on
browserAnchorView.
browserAnchorViewis only set fromextensionControlStrip, so whenidentity == nilthe empty-state “Manage” button falls through this guard and does nothing. Fall back to the key/main window content view, or disable the action until an anchor exists.Suggested fix
private func presentExtensionBrowser() { - guard let browserAnchorView else { return } + guard let anchorView = browserAnchorView + ?? NSApp.keyWindow?.contentView + ?? NSApp.mainWindow?.contentView else { return } AppDelegate.shared?.openSidebarExtensionBrowser( - from: browserAnchorView, + from: anchorView, title: String( localized: "sidebar.extensions.browser.title", defaultValue: "Sidebar Extensions"🤖 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/CMUXInstalledExtensionSidebarHostView.swift` around lines 671 - 679, The empty-state Manage action currently returns early in presentExtensionBrowser() when browserAnchorView is nil, causing the button to do nothing for cases like identity == nil; update presentExtensionBrowser() to fall back to a safe anchor (e.g., keyWindow?.contentView or NSApp.mainWindow?.contentView) when browserAnchorView is nil, then call AppDelegate.shared?.openSidebarExtensionBrowser(from: fallbackAnchor, title: ...) so the browser always opens, or alternatively disable the Manage control until browserAnchorView exists; refer to presentExtensionBrowser(), browserAnchorView, and AppDelegate.shared?.openSidebarExtensionBrowser to implement the fallback or disable behavior.
🤖 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.
Inline comments:
In `@scripts/reload.sh`:
- Line 601: The script currently overwrites OTHER_SWIFT_FLAGS and loses the -D
CMUX_RESCUE_BUILD define when later settings are applied; update the places that
set OTHER_SWIFT_FLAGS (the printf call that emits "OTHER_SWIFT_FLAGS =
$(inherited) -D CMUX_RESCUE_BUILD" and the later assignment around the other
workaround flags) to preserve any existing flags by appending or conditionally
including "-D CMUX_RESCUE_BUILD" instead of replacing the variable; specifically
ensure the OTHER_SWIFT_FLAGS emission and the later workaround flag block both
merge with $(inherited) and include "-D CMUX_RESCUE_BUILD" if rescue mode is
enabled so the define is never dropped.
- Around line 590-603: The rescue xcconfig file path is created predictably via
RESCUE_XCCONFIG_FILE using TMPDIR and TAG_SLUG; replace that predictable path
creation by creating a secure temporary file with mktemp (e.g. call mktemp to
produce RESCUE_XCCONFIG_FILE before writing) and fail fast if mktemp returns
empty, write the config into that secure file as now, export XCODE_XCCONFIG_FILE
to it, and ensure the new temporary file is deleted in reload_finalize (add
cleanup logic there and handle reload_finalize being called on error paths);
also check and log mktemp/create failures so you don't proceed with an invalid
path.
In `@Sources/cmuxAppSettingsSupportViews.swift`:
- Around line 1-8: The file cmuxAppSettingsSupportViews.swift is too large and
mixes multiple responsibilities; split it into focused Swift files as suggested
and move the related types: create SettingsCardComponents.swift containing
SettingsCard, SettingsCardRow, SettingsPickerRow, SettingsCardDivider,
SettingsConfigurationReview and applyIf; create SettingsThemePickers.swift
containing ThemeWindowThumbnail, ThemePickerRow and AppIconPickerRow; create
SettingsSpecializedRows.swift containing AuthSettingsRow, GlobalHotkeySection,
WorkspaceGroupNewWorkspacePlacementSettingsRow and
SurfaceResumeApprovalSettingsCard; and create SettingsNavigation.swift
containing SettingsWindowRootView, SettingsRootView, SettingsDraftState and
SettingsSidebarEntryRow; ensure each new file only imports the frameworks it
needs, preserve access control (public/internal/private) and any helper
functions/state are moved with their consumers to avoid cross-file coupling,
then run the build to fix any missing references and update tests/imports
accordingly.
---
Outside diff comments:
In `@Sources/CMUXInstalledExtensionSidebarHostView.swift`:
- Around line 671-679: The empty-state Manage action currently returns early in
presentExtensionBrowser() when browserAnchorView is nil, causing the button to
do nothing for cases like identity == nil; update presentExtensionBrowser() to
fall back to a safe anchor (e.g., keyWindow?.contentView or
NSApp.mainWindow?.contentView) when browserAnchorView is nil, then call
AppDelegate.shared?.openSidebarExtensionBrowser(from: fallbackAnchor, title:
...) so the browser always opens, or alternatively disable the Manage control
until browserAnchorView exists; refer to presentExtensionBrowser(),
browserAnchorView, and AppDelegate.shared?.openSidebarExtensionBrowser to
implement the fallback or disable behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d5b2253-0177-4c6e-96c2-5fc27b3e32b1
📒 Files selected for processing (18)
Packages/CMUXExtensionClient/Sources/CMUXExtensionClient/Browser/CMUXSidebarExtensionBrowserPresenter.swiftSources/AppDelegate.swiftSources/CMUXInstalledExtensionSidebarHostView.swiftSources/ClosedItemHistory.swiftSources/CmuxLifecycleEventPublishing.swiftSources/ContentView.swiftSources/Panels/Panel.swiftSources/Panels/PanelContentView.swiftSources/Search/GlobalSearchDocuments.swiftSources/TerminalPaneDropTargetView.swiftSources/Workspace.swiftSources/cmuxApp.swiftSources/cmuxAppAuxiliaryViews.swiftSources/cmuxAppSettingsSupportViews.swiftSources/cmuxAppSettingsViews.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspaceUnitTests.swiftscripts/reload.sh
Comment on lines
+590
to
+603
| RESCUE_XCCONFIG_FILE="${TMPDIR:-/tmp}/cmux-reload-${TAG_SLUG}-rescue.xcconfig" | ||
| { | ||
| if [[ -n "${XCODE_XCCONFIG_FILE:-}" ]]; then | ||
| printf '#include "%s"\n' "$XCODE_XCCONFIG_FILE" | ||
| fi | ||
| printf 'SWIFT_ENABLE_BATCH_MODE = NO\n' | ||
| printf 'SWIFT_COMPILATION_MODE = singlefile\n' | ||
| printf 'COMPILER_INDEX_STORE_ENABLE = NO\n' | ||
| printf 'SWIFT_INDEX_STORE_ENABLE = NO\n' | ||
| printf 'GCC_GENERATE_DEBUGGING_SYMBOLS = NO\n' | ||
| printf 'DEBUG_INFORMATION_FORMAT = dwarf\n' | ||
| printf 'OTHER_SWIFT_FLAGS = $(inherited) -D CMUX_RESCUE_BUILD\n' | ||
| } > "$RESCUE_XCCONFIG_FILE" | ||
| export XCODE_XCCONFIG_FILE="$RESCUE_XCCONFIG_FILE" |
There was a problem hiding this comment.
Use a secure temp file for the rescue xcconfig path.
Line 590 writes to a predictable /tmp filename, which allows symlink clobbering/collision risks. Generate a unique file with mktemp and clean it up in reload_finalize.
Suggested fix
-if [[ "$RESCUE_BUILD" -eq 1 ]]; then
- RESCUE_XCCONFIG_FILE="${TMPDIR:-/tmp}/cmux-reload-${TAG_SLUG}-rescue.xcconfig"
+if [[ "$RESCUE_BUILD" -eq 1 ]]; then
+ RESCUE_XCCONFIG_FILE="$(mktemp "${TMPDIR:-/tmp}/cmux-reload-${TAG_SLUG}-rescue.XXXXXX.xcconfig")"
{
@@
} > "$RESCUE_XCCONFIG_FILE"
export XCODE_XCCONFIG_FILE="$RESCUE_XCCONFIG_FILE"
echo "==> rescue build enabled: Swift batch compilation disabled" >&3
fi reload_finalize() {
local rc=$?
trap - EXIT
+ if [[ -n "${RESCUE_XCCONFIG_FILE:-}" && -f "$RESCUE_XCCONFIG_FILE" ]]; then
+ rm -f "$RESCUE_XCCONFIG_FILE" || true
+ fi
exec 1>&3 2>&4🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 601-601: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
🤖 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 `@scripts/reload.sh` around lines 590 - 603, The rescue xcconfig file path is
created predictably via RESCUE_XCCONFIG_FILE using TMPDIR and TAG_SLUG; replace
that predictable path creation by creating a secure temporary file with mktemp
(e.g. call mktemp to produce RESCUE_XCCONFIG_FILE before writing) and fail fast
if mktemp returns empty, write the config into that secure file as now, export
XCODE_XCCONFIG_FILE to it, and ensure the new temporary file is deleted in
reload_finalize (add cleanup logic there and handle reload_finalize being called
on error paths); also check and log mktemp/create failures so you don't proceed
with an invalid path.
| printf 'SWIFT_INDEX_STORE_ENABLE = NO\n' | ||
| printf 'GCC_GENERATE_DEBUGGING_SYMBOLS = NO\n' | ||
| printf 'DEBUG_INFORMATION_FORMAT = dwarf\n' | ||
| printf 'OTHER_SWIFT_FLAGS = $(inherited) -D CMUX_RESCUE_BUILD\n' |
There was a problem hiding this comment.
Preserve CMUX_RESCUE_BUILD when workaround flags are also enabled.
If rescue mode is on and Line 640 sets OTHER_SWIFT_FLAGS, the command-line setting overrides Line 601, so -D CMUX_RESCUE_BUILD is lost.
Suggested fix
if [[ "$SWIFT_FRONTEND_WORKAROUND" -eq 1 || "${CMUX_SWIFT_FRONTEND_WORKAROUND:-}" == "1" || "${CMUX_SWIFT_DISABLE_GLOBAL_ISEL:-}" == "1" ]]; then
@@
- XCODEBUILD_ARGS+=('OTHER_SWIFT_FLAGS=$(inherited) -Xllvm -aarch64-enable-global-isel-at-O=-1')
+ if [[ "$RESCUE_BUILD" -eq 1 ]]; then
+ XCODEBUILD_ARGS+=('OTHER_SWIFT_FLAGS=$(inherited) -D CMUX_RESCUE_BUILD -Xllvm -aarch64-enable-global-isel-at-O=-1')
+ else
+ XCODEBUILD_ARGS+=('OTHER_SWIFT_FLAGS=$(inherited) -Xllvm -aarch64-enable-global-isel-at-O=-1')
+ fi
elseAlso applies to: 634-641
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 601-601: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
🤖 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 `@scripts/reload.sh` at line 601, The script currently overwrites
OTHER_SWIFT_FLAGS and loses the -D CMUX_RESCUE_BUILD define when later settings
are applied; update the places that set OTHER_SWIFT_FLAGS (the printf call that
emits "OTHER_SWIFT_FLAGS = $(inherited) -D CMUX_RESCUE_BUILD" and the later
assignment around the other workaround flags) to preserve any existing flags by
appending or conditionally including "-D CMUX_RESCUE_BUILD" instead of replacing
the variable; specifically ensure the OTHER_SWIFT_FLAGS emission and the later
workaround flag block both merge with $(inherited) and include "-D
CMUX_RESCUE_BUILD" if rescue mode is enabled so the define is never dropped.
Comment on lines
+1
to
+8
| import AppKit | ||
| import CmuxSettings | ||
| import CmuxSettingsUI | ||
| import SwiftUI | ||
| import Observation | ||
| import Darwin | ||
| import Bonsplit | ||
| import UniformTypeIdentifiers |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
New file exceeds 800-line limit and mixes multiple responsibilities.
This 873-line file combines generic card/row components, theme/icon pickers, specialized settings rows (auth, hotkey, placement), navigation views, and draft state. Per coding guidelines, a new production Swift file should not exceed 400 lines without a clear single responsibility, or 800 lines even when mostly coherent.
Consider splitting into focused files:
SettingsCardComponents.swift— genericSettingsCard,SettingsCardRow,SettingsPickerRow,SettingsCardDivider,SettingsConfigurationReview,applyIfSettingsThemePickers.swift—ThemeWindowThumbnail,ThemePickerRow,AppIconPickerRowSettingsSpecializedRows.swift—AuthSettingsRow,GlobalHotkeySection,WorkspaceGroupNewWorkspacePlacementSettingsRow,SurfaceResumeApprovalSettingsCardSettingsNavigation.swift—SettingsWindowRootView,SettingsRootView,SettingsDraftState,SettingsSidebarEntryRow
As per coding guidelines: "A new production Swift file must not exceed 400 lines without a clear single responsibility, or 800 lines even when the responsibility is mostly coherent" and "Do not mix UI rendering, state ownership... in one Swift file."
🤖 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/cmuxAppSettingsSupportViews.swift` around lines 1 - 8, The file
cmuxAppSettingsSupportViews.swift is too large and mixes multiple
responsibilities; split it into focused Swift files as suggested and move the
related types: create SettingsCardComponents.swift containing SettingsCard,
SettingsCardRow, SettingsPickerRow, SettingsCardDivider,
SettingsConfigurationReview and applyIf; create SettingsThemePickers.swift
containing ThemeWindowThumbnail, ThemePickerRow and AppIconPickerRow; create
SettingsSpecializedRows.swift containing AuthSettingsRow, GlobalHotkeySection,
WorkspaceGroupNewWorkspacePlacementSettingsRow and
SurfaceResumeApprovalSettingsCard; and create SettingsNavigation.swift
containing SettingsWindowRootView, SettingsRootView, SettingsDraftState and
SettingsSidebarEntryRow; ensure each new file only imports the frameworks it
needs, preserve access control (public/internal/private) and any helper
functions/state are moved with their consumers to avoid cross-file coupling,
then run the build to fix any missing references and update tests/imports
accordingly.
azooz2003-bit
force-pushed
the
feat-extension-browser-tab
branch
from
May 31, 2026 02:41
ee3fca9 to
89a849f
Compare
azooz2003-bit
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
May 31, 2026 04:49
Stale bot review on superseded commit. The current head 89a849f removes the settings/reload-script churn and addresses the extension-browser comments.
This was referenced May 31, 2026
This branch was successfully deployed
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
Verification
git diff --checkbash -n scripts/reload.shswift test --package-path Packages/CMUXExtensionClient./scripts/reload.sh --tag extbr --rescue-buildDogfood
Tagged build: http://127.0.0.1:17320/extbr
Check that opening Manage Sidebar Extensions creates a
Sidebar Extensionstab in the focused pane. Double-click that tab to zoom it and confirm the browser remains visible.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Open the Sidebar Extensions browser as a tab in the focused pane instead of a floating window. It stays visible during tab zoom/remount and fits into normal tab focus, history, and search flows.
New Features
.extensionBrowserpanel; “Manage Sidebar Extensions” opens a “Sidebar Extensions” tab in the focused pane with proper focus and zoom behavior, and it is excluded from session restore.CMUXSidebarExtensionBrowserPanelwith a stable AppKit container embedding the ExtensionKit browser.AppDelegate.openSidebarExtensionBrowser(from:title:)andWorkspace.newSidebarExtensionBrowserSurface(inPane:title:focus:); presenter now providesmakeViewController(title:)for host embedding.scripts/reload.shsupports--no-batch/--rescue-build; splitscmuxApp.swiftinto auxiliary/settings views (no UI changes).Migration
CMUXSidebarExtensionBrowserPresenter.present(...)withAppDelegate.shared?.openSidebarExtensionBrowser(from:title:). The old presenter entry point is now unavailable.Written for commit 89a849f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores