Repository navigation
Add settings sidebar shell - #3244
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors titlebar debug from a combined Settings/About implementation to About-only; replaces AppKit Settings window controller with a SwiftUI WindowGroup and searchable NavigationSplitView sidebar; adds Settings navigation/search models and prototype SettingsShellLab app; updates localization and several call-sites to use the new presenter/navigation APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sidebar as Sidebar List
participant Detail as Detail View
participant Notifier as NotificationCenter
participant Presenter as SettingsWindowPresenter
User->>Sidebar: Select section / use search
Sidebar->>Detail: Bind selection -> render section
User->>Notifier: Post SettingsNavigationRequest(target)
Notifier->>Sidebar: Update selection / scroll to .id(...)
Sidebar->>Detail: Render requested section
User->>Detail: Click "Reset" / "Reset All"
Detail->>Notifier: Post SettingsResetRequest
Notifier->>Detail: Invoke reset handlers
User->>Presenter: Open Settings (programmatic)
Presenter->>Sidebar: Ensure WindowGroup shows selected target
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 SummaryThis PR wraps the existing Confidence Score: 4/5Safe to merge; findings are cosmetic layout edge-cases only. Only P2 findings: the NSWindow minSize (820) is set 40 px below the SwiftUI frame minimum (860), allowing the window to be resized to a width that slightly compresses the NavigationSplitView. No logic errors, data loss, or security concerns. Sources/cmuxApp.swift — the configureSettingsWindow minSize and the .frame(minWidth:) value should be aligned. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant SidebarList
participant sidebarSelection as sidebarSelection Binding
participant NotificationCenter
participant SettingsRootView
participant SettingsView
User->>SidebarList: tap sidebar item
SidebarList->>sidebarSelection: set(newValue)
sidebarSelection->>SettingsRootView: selectedSidebarItem = newValue
sidebarSelection->>NotificationCenter: post(cmux.settings.navigate)
NotificationCenter->>SettingsView: onReceive → proxy.scrollTo(target)
NotificationCenter->>SettingsRootView: onReceive → selectedSidebarItem = target (same value, no-op)
Note over User,SettingsWindowController: External navigation (e.g. menu item)
participant SettingsWindowController
SettingsWindowController->>NotificationCenter: post(target) after 50ms delay
NotificationCenter->>SettingsRootView: onReceive → selectedSidebarItem = target (sidebar highlights)
NotificationCenter->>SettingsView: onReceive → proxy.scrollTo(target)
Reviews (1): Last reviewed commit: "Add settings sidebar shell" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| private func configureSettingsWindow(_ window: NSWindow) { | ||
| window.identifier = NSUserInterfaceItemIdentifier("cmux.settings") | ||
| window.minSize = NSSize(width: 820, height: 540) |
Contributor
There was a problem hiding this comment.
window.minSize is set to 820 but the NavigationSplitView has .frame(minWidth: 860). The OS-enforced window resize limit (820) is 40 px narrower than the SwiftUI layout minimum, so dragging the window to 820–860 px will compress the split view beyond what it was designed for — the sidebar or detail content may clip or reflow unexpectedly.
Suggested change
| window.minSize = NSSize(width: 820, height: 540) | |
| window.minSize = NSSize(width: 860, height: 540) |
| SettingsView() | ||
| } | ||
| .navigationSplitViewStyle(.balanced) | ||
| .frame(minWidth: 860, minHeight: 540) |
Contributor
There was a problem hiding this comment.
.frame(minWidth: 860) is the declared minimum for the whole NavigationSplitView. More critically, when the window is at 820 px (the NSWindow minimum set just below), the .frame(minWidth: 860) constraint is exceeded and SwiftUI may silently drop it — consider aligning both to the same value.
Contributor
Author
There was a problem hiding this comment.
Fixed in the current code: Settings now uses one shared minimum size through SettingsWindowPresenter.minimumSize, the SwiftUI root frame, and NSWindow minSize/contentMinSize.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Fixed in the current code: Settings now uses one shared minimum size through SettingsWindowPresenter.minimumSize, the SwiftUI root frame, and NSWindow minSize/contentMinSize.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Line 8506: The view uses two different minimum widths (the SwiftUI modifier
.frame(minWidth: 860, minHeight: 540) and the AppKit window minSize.width =
820), causing inconsistent constraints; replace the hard-coded literals by a
single shared constant (e.g., let kMinWindowWidth / minWindowSize) and use that
constant in both the .frame(...) call and the window.minSize.width assignment
(also mirror minHeight if needed) so both UI layers enforce the same minimum
width.
- Around line 8507-8509: When handling SettingsNavigationRequest notifications
in the onReceive closure, clear the sidebar filter by setting searchText = ""
before assigning selectedSidebarItem so the target row is not hidden; update the
closure that listens to SettingsNavigationRequest.notificationName to first
reset searchText (or otherwise ensure the filter is cleared) and then set
selectedSidebarItem = SettingsNavigationRequest.target(from: notification) ??
selectedSidebarItem.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95d4ca1edd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+8469
to
+8474
| @State private var searchText = "" | ||
|
|
||
| private var sidebarItems: [SettingsNavigationTarget] { | ||
| let query = searchText.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard !query.isEmpty else { return SettingsNavigationTarget.allCases } | ||
| return SettingsNavigationTarget.allCases.filter { item in |
There was a problem hiding this comment.
searchText is stored as @State on SettingsRootView, but this window is reused (SettingsWindowController keeps a single hosting view alive), so closing and reopening Settings preserves the previous search query. In practice, reopening the window can show a partially filtered sidebar with missing sections, which looks like settings disappeared rather than a stale filter. Reset the query when the settings window is shown/closed, or store it in ephemeral view state tied to each presentation.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Fixed in the current code: SettingsRootView clears searchText on appear before replaying navigation, so reopened Settings does not keep a stale filter.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Fixed in the current code: SettingsRootView clears searchText on appear before replaying navigation, so reopened Settings does not keep a stale filter.
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Prototypes/SettingsShellLab/Sources/SettingsShellLab/App/SettingsShellLabApp.swift (1)
9-13: Consider enforcing a minimum window size (not just default size).
defaultSizeonly seeds initial geometry. Add content min-size constraints so the shell remains usable when resized very small.💡 Suggested fix
WindowGroup(String(localized: "app.window.title", defaultValue: "Settings Shell Lab")) { - SettingsShellView() + SettingsShellView() + .frame(minWidth: 760, minHeight: 520) } .defaultSize(width: 900, height: 620) + .windowResizability(.contentMinSize) .commands { SidebarCommands() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Prototypes/SettingsShellLab/Sources/SettingsShellLab/App/SettingsShellLabApp.swift` around lines 9 - 13, The WindowGroup currently only sets a .defaultSize which doesn't prevent the user from resizing the window too small; update the layout by enforcing a minimum content size—apply a minWidth/minHeight constraint to SettingsShellView (e.g., via a .frame(minWidth: ..., minHeight: ...)) or otherwise ensure SettingsShellView's root view enforces minimum sizes so the shell remains usable when resized; modify the WindowGroup/SettingsShellView usage around the existing defaultSize call to include these min size constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@Prototypes/SettingsShellLab/Sources/SettingsShellLab/Views/SettingsDetailView.swift`:
- Around line 41-45: The two interactive Button instances in SettingsDetailView
(the ones created with localized titles "detail.primaryAction" and
"detail.secondaryAction") have empty action closures and should not be
clickable; either remove them until implemented or make them non-interactive by
replacing with Text or adding .disabled(true) and/or a conditional that hides
them until wired. Locate the Button initializers in SettingsDetailView.swift and
replace the empty closures with a non-interactive placeholder (Text) or append
.disabled(true) / wrap in an if-guard that checks a ready-to-use flag so users
cannot click a no-op control.
- Around line 14-67: The Localizable.xcstrings file is missing nine localization
keys used by SettingsDetailView (strings referenced inline like
String(localized: "detail.section", ...), "detail.toggle", "detail.mode",
"detail.slider", "detail.primaryAction", "detail.secondaryAction") and the Mode
enum titles ("mode.system", "mode.compact", "mode.expanded"); add these keys to
Resources/Localizable.xcstrings with English and Japanese translations matching
the defaultValue text used in SettingsDetailView and Mode.title so the
String(localized: ...) lookups resolve at runtime.
In
`@Prototypes/SettingsShellLab/Sources/SettingsShellLab/Views/SettingsShellView.swift`:
- Around line 39-45: The Reset toolbar Button currently has an empty action;
either wire it to a concrete reset handler or disable it until implemented.
Implement by calling an existing or new reset method (e.g., add a
resetToDefaults() or resetSettings() on your view model/owner and invoke it
inside the Button action), or if you prefer to block interaction for now, add
.disabled(true) (or conditionally disable using a viewModel.canReset flag) to
the Button/Label so the visible control is not a no-op; update SettingsShellView
to call the chosen reset handler or apply .disabled accordingly.
---
Nitpick comments:
In
`@Prototypes/SettingsShellLab/Sources/SettingsShellLab/App/SettingsShellLabApp.swift`:
- Around line 9-13: The WindowGroup currently only sets a .defaultSize which
doesn't prevent the user from resizing the window too small; update the layout
by enforcing a minimum content size—apply a minWidth/minHeight constraint to
SettingsShellView (e.g., via a .frame(minWidth: ..., minHeight: ...)) or
otherwise ensure SettingsShellView's root view enforces minimum sizes so the
shell remains usable when resized; modify the WindowGroup/SettingsShellView
usage around the existing defaultSize call to include these min size
constraints.
🪄 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: ffc512cf-5317-4092-8533-868e0b82898a
📒 Files selected for processing (8)
Prototypes/SettingsShellLab/.codex/environments/environment.tomlPrototypes/SettingsShellLab/.gitignorePrototypes/SettingsShellLab/Package.swiftPrototypes/SettingsShellLab/Sources/SettingsShellLab/App/SettingsShellLabApp.swiftPrototypes/SettingsShellLab/Sources/SettingsShellLab/Models/SettingsSection.swiftPrototypes/SettingsShellLab/Sources/SettingsShellLab/Views/SettingsDetailView.swiftPrototypes/SettingsShellLab/Sources/SettingsShellLab/Views/SettingsShellView.swiftPrototypes/SettingsShellLab/script/build_and_run.sh
✅ Files skipped from review due to trivial changes (4)
- Prototypes/SettingsShellLab/.gitignore
- Prototypes/SettingsShellLab/Package.swift
- Prototypes/SettingsShellLab/.codex/environments/environment.toml
- Prototypes/SettingsShellLab/Sources/SettingsShellLab/Models/SettingsSection.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f28dc75a87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+8391
to
+8393
| .onChange(of: selectedSectionRaw) { _, newValue in | ||
| guard let target = SettingsNavigationTarget(rawValue: newValue) else { return } | ||
| SettingsNavigationRequest.post(target) |
There was a problem hiding this comment.
selectedSectionRaw is restored from @SceneStorage, but the detail view only receives navigation events via the onChange publisher. Because onChange does not fire for the initial restored value, opening Settings can show a sidebar selection (for example, "Keyboard Shortcuts") while the scroll content remains at the top section until another selection change occurs. Trigger the same navigation post on first appearance (or otherwise initialize the scroll target) so the highlighted sidebar item and visible section stay consistent.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Fixed in the current code: SettingsRootView now replays the restored selected section on appear when there is no pending targeted navigation.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Fixed in the current code: SettingsRootView now replays the restored selected section on appear when there is no pending targeted navigation.
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Sources/cmuxApp.swift (2)
8408-8410:⚠️ Potential issue | 🟡 MinorClear sidebar search before applying incoming navigation target
If a navigation request targets a section filtered out by
searchText, selection updates but the row remains hidden in the sidebar.💡 Suggested fix
.onReceive(NotificationCenter.default.publisher(for: SettingsNavigationRequest.notificationName)) { notification in - selectedSectionRaw = SettingsNavigationRequest.target(from: notification)?.rawValue ?? selectedSectionRaw + guard let target = SettingsNavigationRequest.target(from: notification) else { return } + if !searchText.isEmpty { + searchText = "" + } + selectedSectionRaw = target.rawValue }Based on learnings: In
Sources/cmuxApp.swift, clear sidebar search before handling navigation requests so target entries/anchors are guaranteed to exist and remain visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 8408 - 8410, In the notification handler for SettingsNavigationRequest within the onReceive modifier, clear the searchText before updating selectedSectionRaw to ensure the target section is not filtered out by an active search. Add a line to set searchText to an empty string before the selectedSectionRaw assignment so that when the navigation target is applied, the sidebar is not filtered and the selected section remains visible to the user.
2464-2474:⚠️ Potential issue | 🟠 MajorEnforce the settings window minimum size explicitly
Line 2464 sets only the initial size. There’s no
minSize/contentMinSizeclamp here, so users can resize below the intended settings layout floor.💡 Suggested fix
private init() { + let defaultSize = NSSize(width: 900, height: 620) + let minimumSize = NSSize(width: 820, height: 540) let window = NSWindow( - contentRect: NSRect(x: 0, y: 0, width: 900, height: 620), + contentRect: NSRect(origin: .zero, size: defaultSize), styleMask: [.titled, .closable, .miniaturizable, .resizable], backing: .buffered, defer: false ) + window.minSize = minimumSize + window.contentMinSize = minimumSize window.isReleasedWhenClosed = false window.identifier = NSUserInterfaceItemIdentifier("cmux.settings")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 2464 - 2474, The settings window only sets an initial size but doesn't enforce a minimum; update the initialization around the NSWindow named window (before super.init(window: window)) to set window.minSize and window.contentMinSize to the intended minimum NSSize (e.g., width: 900, height: 620) so SettingsRootView's layout cannot be resized below the floor; ensure both properties are assigned on the same window instance created there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 8356-8361: Normalize the stored SceneStorage value by validating
selectedSectionRaw on view load and replacing invalid values with a sane
default: check whether SettingsNavigationTarget(rawValue: selectedSectionRaw)
returns nil and if so assign selectedSectionRaw =
SettingsNavigationTarget.account.rawValue (or another chosen default). Do this
validation in the view's initializer or onAppear so selectedSection (which
derives from selectedSectionRaw) always yields a valid enum and the sidebar
shows a selected row; reference selectedSectionRaw, selectedSection, and
SettingsNavigationTarget when locating where to add the check.
In `@Sources/WindowDecorationsController.swift`:
- Around line 71-73: The trafficLightOffset(for:) implementation was changed to
always return .zero, disabling offsets globally; revert this to only return
.zero for the settings window by checking the window identifier (e.g., compare
window.identifier?.rawValue to "cmux.settings") and otherwise compute/return the
original offset value used previously so applyTrafficLightOffset(on:hidden:)
still applies offsets for non-settings windows; update the logic inside
trafficLightOffset(for:) (and ensure any helper used there is preserved) rather
than changing applyTrafficLightOffset(on:hidden:).
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 8408-8410: In the notification handler for
SettingsNavigationRequest within the onReceive modifier, clear the searchText
before updating selectedSectionRaw to ensure the target section is not filtered
out by an active search. Add a line to set searchText to an empty string before
the selectedSectionRaw assignment so that when the navigation target is applied,
the sidebar is not filtered and the selected section remains visible to the
user.
- Around line 2464-2474: The settings window only sets an initial size but
doesn't enforce a minimum; update the initialization around the NSWindow named
window (before super.init(window: window)) to set window.minSize and
window.contentMinSize to the intended minimum NSSize (e.g., width: 900, height:
620) so SettingsRootView's layout cannot be resized below the floor; ensure both
properties are assigned on the same window instance created there.
🪄 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: d8a8f5d0-8943-4996-bd25-11baab51baa2
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/TerminalController.swiftSources/WindowDecorationsController.swiftSources/cmuxApp.swift
✅ Files skipped from review due to trivial changes (1)
- Resources/Localizable.xcstrings
Comment on lines
71
to
73
| private func trafficLightOffset(for window: NSWindow) -> NSPoint { | ||
| guard window.identifier?.rawValue == "cmux.settings" else { return .zero } | ||
| // Nudge controls slightly right/down to align with the custom Settings title row. | ||
| return NSPoint(x: 7, y: -4) | ||
| return .zero | ||
| } |
There was a problem hiding this comment.
Keep the zero offset scoped to the settings window.
trafficLightOffset(for:) now returns .zero for every window, so applyTrafficLightOffset(on:hidden:) stops applying any offset anywhere this controller is used. If the intent is only to neutralize cmux.settings, keep the identifier check here and preserve the existing offset for other windows.
🔧 Proposed fix
private func trafficLightOffset(for window: NSWindow) -> NSPoint {
- return .zero
+ if window.identifier?.rawValue == "cmux.settings" {
+ return .zero
+ }
+ return /* existing non-settings offset logic */
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/WindowDecorationsController.swift` around lines 71 - 73, The
trafficLightOffset(for:) implementation was changed to always return .zero,
disabling offsets globally; revert this to only return .zero for the settings
window by checking the window identifier (e.g., compare
window.identifier?.rawValue to "cmux.settings") and otherwise compute/return the
original offset value used previously so applyTrafficLightOffset(on:hidden:)
still applies offsets for non-settings windows; update the logic inside
trafficLightOffset(for:) (and ensure any helper used there is preserved) rather
than changing applyTrafficLightOffset(on:hidden:).
Contributor
Author
There was a problem hiding this comment.
Verified as not applicable. On main, non-settings windows already returned zero offset; this PR intentionally removes the Settings-only traffic-light offset, so no non-settings offset behavior was lost.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Verified as not applicable. On main, non-settings windows already returned zero offset; this PR intentionally removes the Settings-only traffic-light offset, so no non-settings offset behavior was lost.
— Claude Code
There was a problem hiding this comment.
@lawrencecchen, understood — the previous implementation had .zero as the default for all non-settings windows, so collapsing the method to always return .zero correctly preserves that behavior while intentionally dropping the Settings-specific nudge (now unnecessary with the native system titlebar via the SwiftUI WindowGroup). My comment was a false positive. Thanks for the clarification!
(ᓀ‸ᓂ)
✏️ Learnings added
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3244
File: Sources/WindowDecorationsController.swift:71-73
Timestamp: 2026-04-29T06:24:51.350Z
Learning: Repo: manaflow-ai/cmux — Sources/WindowDecorationsController.swift — `trafficLightOffset(for:)` returned `.zero` for all non-settings windows on `main`; the only non-zero offset was for `cmux.settings` (nudge right/down to align with the custom Settings title row). PR `#3244` intentionally removes that offset because Settings now uses a SwiftUI WindowGroup with a native macOS system titlebar, making the nudge unnecessary. Do not flag `trafficLightOffset(for:)` always returning `.zero` as a regression.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:3586-3604
Timestamp: 2026-04-14T20:00:21.500Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — On macOS 26+, the app intentionally keeps NSWindow.StyleMask.fullSizeContentView enabled even with the native (non-transparent) system titlebar. The titlebar handles dragging natively; isMovable=true and isMovableByWindowBackground=false are used. Any comment suggesting “without fullSizeContentView” is outdated.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2124
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-25T08:05:26.034Z
Learning: Repo: manaflow-ai/cmux — File: Sources/AppDelegate.swift — Pattern for “New Window” geometry seed:
In AppDelegate.createMainWindow(initialWorkingDirectory:sessionWindowSnapshot:), compute the existingFrame using preferredMainWindowContextForWorkspaceCreation(debugSource: "createMainWindow.initialGeometry") and then resolvedWindow(for:) rather than relying on NSApp.keyWindow or the first registered mainWindowContext. Rationale: ensures the new window inherits size from the intended main-terminal window even when an auxiliary window is key, and keeps behavior consistent with showOpenFolderPanel().
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3179
File: Sources/ContentView.swift:4005-4021
Timestamp: 2026-04-27T11:59:15.622Z
Learning: Repo: manaflow-ai/cmux — PR `#3179` — Sources/ContentView.swift
Learning: For separate-surface transparent windows, installNativeTitlebarBackdrop intentionally inserts NativeTitlebarBackdropView below contentView (NSHostingView) — positioned .below relativeTo: contentView — so the backdrop fills through transparent hosting without tinting SwiftUI titlebar text/buttons. The titlebar‑chrome ancestor fallback is only for edge cases where contentView isn’t directly under the NSThemeFrame. This layering was pixel‑verified in PR `#3179`; do not “fix” it to sit below the titlebar chrome on this path.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3179
File: Sources/ContentView.swift:3991-4002
Timestamp: 2026-04-27T09:15:53.326Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — Native titlebar backdrop layering: installNativeTitlebarBackdrop must place the backdrop between the NSThemeFrame’s background material and the standard titlebar chrome (insert .below the close button’s ancestor in the theme frame). When the titlebar-chrome ancestor lookup fails, fall back to inserting .below relativeTo: nil so the backdrop is behind existing siblings and does not overpaint the SwiftUI titlebar/sidebar strip. Visual matching has been verified via pixel samples.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/ContentView.swift:3194-3196
Timestamp: 2026-03-04T14:05:48.668Z
Learning: In manaflow-ai/cmux (PR `#819`), Sources/ContentView.swift: The command palette’s external window labels intentionally use the global window index from the full orderedSummaries (index + 1), matching the Window menu in AppDelegate. Do not reindex after filtering out the current window to avoid mismatches (“Window 2” for an external window is expected).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3166
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-27T06:58:21.434Z
Learning: In the Swift UI code (e.g., Sources/ContentView.swift), when `preferLiquidGlass`/`materialPolicy.preferLiquidGlass` is enabled for sidebar Liquid Glass, only omit the `Color` tint overlay if `NSGlassEffectView` (native Liquid Glass) is actually available. Compute `usingNativeLiquidGlass = materialPolicy.preferLiquidGlass && SidebarVisualEffectBackground.liquidGlassAvailable`, and when `usingNativeLiquidGlass` is false keep the overlay so the non-native `NSVisualEffectView` fallback still receives the configured tint.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3218
File: Sources/AppDelegate.swift:3456-3469
Timestamp: 2026-04-28T11:35:31.575Z
Learning: Repo: manaflow-ai/cmux — Sources/AppDelegate.swift (Swift)
Learning: The DEBUG-only helper unregisterMainWindowContextForTesting(windowId:) should mirror production teardown: after removing matching mainWindowContexts entries, call activateMainWindowContext(mainWindowContexts.values.first) when the active context was removed so tabManager, sidebarState, sidebarSelectionState, and fileExplorerState are repointed (or cleared when none remain). This prevents stale active pointers in tests.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3166
File: Sources/Windowing/WindowAppearanceSnapshot.swift:185-210
Timestamp: 2026-04-27T06:58:26.079Z
Learning: Repo: manaflow-ai/cmux — Sources/GhosttyTerminalView.swift & Sources/Windowing/WindowAppearanceSnapshot.swift — Snapshot refresh contract for usesHostLayerBackground:
`notifyDefaultBackgroundDidChange()` (and therefore the titlebar/window snapshot refresh in ContentView) must fire whenever `usesHostLayerBackground` changes, even if terminal background color and opacity are unchanged. The prior `hasChanged` check in `updateDefaultBackground()` only compared colors; it was extended so that a toggle of `usesHostLayerBackground` alone also triggers the notification. ContentView listens at the window level to rebuild `WindowAppearanceSnapshot` (including `terminalRenderingMode`) on any such notification. Fixed in commits 4913d17–979a683 (PR `#3166`).
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:15597-15615
Timestamp: 2026-04-14T20:05:53.504Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — On macOS 26 in this app, the only NSSplitView in the window’s view tree is the one owned by NavigationSplitView. Therefore SplitViewDividerHiderView.hideDividers()/patchSplitViews(in:) effectively scope to that single split. Do not flag “global divider clearing” for this path unless additional split views are introduced later.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/AppDelegate.swift:10168-10189
Timestamp: 2026-04-14T20:02:41.558Z
Learning: Repo manaflow-ai/cmux — On macOS 26, the notifications popover is owned by ContentView (State isNotificationsPopoverPresented). AppDelegate APIs (toggle/show/dismiss/isShown) must route via NotificationCenter with a window-scoped userInfo["windowId"] and track visibility per-window, not call UpdateTitlebarAccessoryController. ContentView should observe show/dismiss/toggle and post a visibilityDidChange notification {windowId, visible}.
Learnt from: tranquillum
Repo: manaflow-ai/cmux PR: 2827
File: Sources/Sidebar/ProviderAccountsFooterPanel.swift:174-198
Timestamp: 2026-04-17T15:46:53.298Z
Learning: Repo: manaflow-ai/cmux — Sources/Sidebar/ProviderAccountsFooterPanel.swift + Sources/Sidebar/ProviderAccountsPopover.swift (PR `#2827`):
- `ProviderStatusRanking.impactSeverity(_:)` intentionally returns `-1` for any unrecognized/`"none"` impact string.
- When `worstImpactSeverity == -1` (every incident in the list has an unknown/none impact), `ProviderStatusLabel.statusText` and `.dotColor` fall through to the `default` branch, which shows "Operational" / green.
- This is a deliberate design choice: `StatuspageIOFetcher.fetch` already defaults missing impact fields to `"none"`, so the only way to reach `worstImpactSeverity == -1` is a genuinely benign incident array; showing "Operational" here avoids false-positive yellow.
- The popover always surfaces the full raw incident rows independently, so no incident is invisible to the user.
- Do NOT flag the `-1 → Operational` fallback as a bug or suggest raising unknown impacts to `minor` severity.
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-04-28T22:24:17.987Z
Learning: Repo: manaflow-ai/cmux — In Sources/Workspace.swift (PR `#3237`, commit 9dcb9c2), the `newTerminalSurface(...)` method gained an `allowInitialInputWithRemoteStartupCommand: Bool` parameter (default `false`). The session-restore call site in `createPanel(from:inPane:)` passes `true` only when `!panelWasRemoteBacked && combinedInitialInput != nil`, making the per-panel snapshot flag the sole authority for restore-time `initialInput` injection. All 12 other call sites keep `false`, preserving the existing remote-drop defense.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-16T08:02:06.824Z
Learning: In Swift sources, for any panel_id-only route handling in v2PanelMarkBackground(params:) and v2PanelMarkForeground(params:), first attempt v2ResolveTabManager(params:). Use the manager only if it actually owns the panelId; otherwise fall back to AppDelegate.shared?.locateSurface(surfaceId:) to locate the correct TabManager across windows. Apply this pattern to all panel_id-only routes to avoid active-window bias.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3164
File: Sources/cmuxApp.swift:189-197
Timestamp: 2026-04-26T06:03:08.863Z
Learning: Repo: manaflow-ai/cmux — PR `#3164` — Bootstrap contract: In cmuxApp.bootstrapMainWindowScene(), AppDelegate.bootstrapInitialMainWindowIfNeeded(...) calls ensureInitialMainWindowIfNeeded() to create (if needed) the real AppKit main window via AppDelegate.createMainWindow, which wires CmuxConfigStore and FileExplorerState and calls registerMainWindow(... fileExplorerState:). Only after registration does updateSocketController() start TerminalController using AppDelegate.synchronizeActiveMainWindowContext(...). Conclusion: per‑window config/file‑explorer/directory initialization is handled in AppDelegate and occurs before TerminalController starts; do not flag missing setup in WindowGroup.onAppear.
Learnt from: arieltobiana
Repo: manaflow-ai/cmux PR: 1873
File: Sources/TerminalController.swift:4071-4087
Timestamp: 2026-03-20T17:18:30.333Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceAction(params:) -> case "set_color": palette names are resolved via WorkspaceTabColorSettings.defaultPaletteWithOverrides(), whose entries are always valid hex (validated by the UI). Therefore, additional normalization of entry.hex is unnecessary.
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-04-28T22:24:17.987Z
Learning: Repo: manaflow-ai/cmux — In Sources/GhosttyTerminalView.swift (PR `#3237`, commit 9dcb9c2), the explicit-vs-baseConfig.initialInput resolution logic was extracted from `TerminalSurface.createSurface(for:)` into a standalone `TerminalSurface.resolveInitialInput(...)` static/instance method for unit-testability. Behavior is unchanged. A companion `TerminalSurfaceResolveInitialInputTests` suite asserts the contract, including byte-for-byte preservation of Ghostty raw-bytes startup-input via `baseConfig.initialInput`.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/AppDelegate.swift:2196-2200
Timestamp: 2026-04-03T03:36:45.112Z
Learning: Repo: manaflow-ai/cmux — In Sources/AppDelegate.swift, when KeyboardShortcutSettings.didChangeNotification fires, AppDelegate must clear configured-chord caches (pendingConfiguredShortcutChord and activeConfiguredShortcutChordPrefixForCurrentEvent) via clearConfiguredShortcutChordState() before refreshing tooltips/UI. Also clear chord state on applicationWillResignActive to avoid cross-activity leakage. Verified by cmuxTests/AppDelegateShortcutRoutingTests.swift::testShortcutChangeClearsPendingConfiguredChord.
Learnt from: Horacehxw
Repo: manaflow-ai/cmux PR: 1980
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-04-06T09:33:43.688Z
Learning: Repo: manaflow-ai/cmux — File: Sources/AppDelegate.swift — UI test launch stabilization: in stabilizeUITestLaunchWindowAndForeground(attempt:), call moveUITestWindowToTargetDisplayIfNeeded() only on the first pass (attempt == 0) so the helper’s own 20-step retry handles display moves; subsequent attempts only activate the app and write diagnostics. This avoids nested overlapping retries and stage-log churn.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:15597-15614
Timestamp: 2026-04-14T20:29:58.471Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift (PR `#2647`, macOS 26): SplitViewDividerHiderView hides split dividers by calling the private selector setDividerColor: only when responds(to:) is true (no KVC). Scope is effectively the single NSSplitView owned by NavigationSplitView. Do not flag KVC misuse or “global divider clearing” for this path unless additional NSSplitViews are added.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:2889-2911
Timestamp: 2026-04-14T19:59:54.878Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — On macOS 26, the NavigationSplitView sidebar width is synchronized back to model state via a GeometryReader that updates sidebarWidth and SidebarState.persistedWidth on width changes. Do not flag “write-only sidebar width” drift for this path going forward.
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-04-28T22:24:17.987Z
Learning: Repo: manaflow-ai/cmux — In Sources/GhosttyTerminalView.swift (PR `#3237`, commit 9dcb9c2), `private var initialInput` on `TerminalSurface` was changed to `private(set) var initialInput` to allow `testable` consumers to assert the post-normalization value. The setter remains class-private.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3166
File: Sources/Windowing/WindowAppearanceSnapshot.swift:185-210
Timestamp: 2026-04-27T06:58:26.717Z
Learning: Repo: manaflow-ai/cmux — Sources/GhosttyTerminalView.swift & Sources/ContentView.swift — Snapshot refresh contract for usesHostLayerBackground (post PR `#3166`):
- `updateDefaultBackground()` must fire `notifyDefaultBackgroundDidChange()` (or equivalent generation bump) whenever `usesHostLayerBackground` changes, even if terminal background color and opacity are unchanged, because `WindowAppearanceSnapshot.terminalRenderingMode(usesHostLayerBackground:)` depends on it.
- ContentView must subscribe to the Ghostty default-background notification at the window level (not just color/opacity changes) so the titlebar/window snapshot (`windowAppearanceSnapshot`) is recomputed whenever the rendering mode toggles.
- Do NOT gate the notification solely on color hasChanged; rendering-mode-only transitions must also propagate through the same notification channel.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2575
File: Sources/cmuxApp.swift:0-0
Timestamp: 2026-04-06T08:59:21.923Z
Learning: Repo: manaflow-ai/cmux — File: Sources/cmuxApp.swift — In SettingsView, when handling SettingsNavigationRequest, clear the sidebar search first (settingsSearchText = "") before calling scrollTo; sections are conditionally rendered based on the active query, so clearing ensures the target anchors (e.g., SettingsSection.browser, SettingsSection.keyboardShortcuts, SettingsNavigationTarget.browserImport) exist.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:2918-2932
Timestamp: 2026-04-14T20:05:27.511Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift (PR `#2647`): On macOS 26, NavigationSplitView provides a native collapse chevron in the sidebar column header when the sidebar is visible. The code intentionally: (1) shows a custom toolbar toggle only when the sidebar is hidden to reopen it, and (2) uses SystemSidebarToggleStripper to remove the system-injected toolbar toggle to prevent duplication. Do not flag “no collapse affordance” while the sidebar is open.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2575
File: Sources/cmuxApp.swift:0-0
Timestamp: 2026-04-06T09:01:51.979Z
Learning: Repo: manaflow-ai/cmux — File: Sources/cmuxApp.swift — SettingsView now uses SwiftUI `.searchable(text:placement:prompt:)` for the sidebar search, which provides a native (accessible) clear button. Do not flag missing accessibility on a custom clear button in this view going forward.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
Learnt from: zlatkoc
Repo: manaflow-ai/cmux PR: 1368
File: Sources/Panels/BrowserPanel.swift:69-69
Timestamp: 2026-03-13T13:46:01.733Z
Learning: Do not wrap engine/brand name literals (e.g., displayName values such as Google, DuckDuckGo, Bing, Kagi, Startpage) in String(localized: ...). These are brand/product names that are not translatable UI text. Localization should apply to generic UI strings (labels, buttons, error messages, etc.). Apply this guideline across Swift source files under Sources/ (notably in BrowserPanel.swift and similar UI/engine-related strings) and flag only brand-name strings that are part of user-facing UI text appropriately for translation scope.
Learnt from: kjb0787
Repo: manaflow-ai/cmux PR: 1461
File: Sources/GhosttyTerminalView.swift:5904-5905
Timestamp: 2026-03-15T19:22:32.330Z
Learning: In Swift files under the Sources directory that manage terminal/scroll behavior, ensure the following: when preserving scroll across workspace switches, save savedScrollRow only if the scrollbar offset is greater than 0 (indicating the user has scrolled up). On restore, call scroll_to_row only if savedScrollRow is non-nil; if it is nil, rely on synchronizeScrollView() to keep bottom-pinned sessions following new output. This pattern should be applied wherever GhosttyTerminalView-like views implement setVisibleInUI(_:) to maintain consistent user scroll state across workspace switches.
Learnt from: pratikpakhale
Repo: manaflow-ai/cmux PR: 2011
File: Resources/Localizable.xcstrings:15256-15368
Timestamp: 2026-03-23T21:39:50.795Z
Learning: When reviewing this repo’s Swift localization usage, do not flag missing `String.localizedStringWithFormat` for calls that use the modern overload `String(localized: "key", defaultValue: "...\(variable)")` (where `defaultValue` is a `String.LocalizationValue` built with `\(…)`). That overload natively supports interpolation and the xcstrings/runtime substitution handles the resulting placeholders automatically. Only require `String.localizedStringWithFormat` when using the older `String(localized:)` overload that takes a plain `String` (i.e., where format arguments must be passed separately), such as for keys like `clipboard.sshError.single`.
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: Sources/TerminalController.swift:3620-3622
Timestamp: 2026-03-25T00:32:54.735Z
Learning: When validating or reporting workspace/tab colors in this repo, only accept and use 6-digit hex colors in the form `#RRGGBB` (no alpha, i.e., do not allow `#RRGGBBAA`). Ensure validation logic matches the existing behavior (e.g., WorkspaceTabColorSettings.normalizedHex(...) and TabManager.setTabColor(tabId:color:) as well as CLI/cmux.swift). Update any error/help text for workspace color to reference only `#RRGGBB` (not `#RRGGBBAA`).
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 2545
File: Sources/GhosttyTerminalView.swift:3891-3903
Timestamp: 2026-04-02T21:37:21.463Z
Learning: In Swift source files like Sources/GhosttyTerminalView.swift, avoid logging raw startup commands or initialInput even in DEBUG (to prevent leaking sensitive paths/tokens and multiline content). If you need to diagnose startup/input, log only non-sensitive metadata such as (1) presence flags (e.g., hasStartupCommand/hasInitialInput), (2) byte counts, and (3) the relevant surface id (so issues can be correlated without exposing the underlying strings).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/cmuxApp.swift:6439-6444
Timestamp: 2026-04-03T03:35:54.082Z
Learning: In this repo’s keyboard shortcut implementation, ensure `KeyboardShortcutSettings.setShortcut(...)` does nothing (no-op) when `KeyboardShortcutSettings.isManagedBySettingsFile(action)` returns `true` (i.e., the shortcut is managed via `settings.json`). This prevents writing back overrides into `UserDefaults` and keeps `settings.json` as the source of truth.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2964
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-17T21:35:25.493Z
Learning: In this repo’s shell session resume flow, always build resume commands using the cwd guard helper exposed by SessionEntry (e.g., resumeCommandWithCwd). The helper should produce a command of the form `cd <shell-quoted cwd> && <resumeCommand>`. Update all call sites that generate “resume” commands (e.g., clipboard actions, drag-drop terminal, and in-app resume) to use this helper so that rc files and newly spawned shells cannot start outside the intended directory. Avoid constructing resume commands directly without the guarded `cd` + shell-quoting + `&&` composition.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2978
File: Sources/Workspace.swift:0-0
Timestamp: 2026-04-22T08:13:36.833Z
Learning: In manaflow-ai/cmux, note that Sources/RestorableAgentSession.swift’s `SessionRestorableAgentSnapshot.resumeCommand` already includes a cwd guard when `workingDirectory` is present (it returns a string like `cd <shell-quoted cwd> && <resumeCommand>`). At call sites (e.g., `Workspace.createPanel(...)`), pass `.resumeCommand` through directly and do not prepend another `cd`/cwd guard or wrap it with an additional `cd <...> &&`—otherwise the working directory may be applied twice or incorrectly.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2978
File: Sources/Workspace.swift:0-0
Timestamp: 2026-04-22T08:14:04.901Z
Learning: In the cmux Swift sources, when restoring a restorable agent session, call sites should pass `resumeCommand` directly (ensuring it has the expected trailing newline if required) to `sendInputWhenReady`. Do not wrap `resumeCommand` with an additional `cd '<cwd>' && ...` guard, because `SessionRestorableAgentSnapshot.resumeCommand` already returns a `cwd`-guarded command; adding another guard can result in `double-cd`. Apply this especially along restore paths (e.g., `Sources/Workspace.swift` restore logic) whenever using `resumeCommand`.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3084
File: Sources/AppDelegate.swift:4958-4975
Timestamp: 2026-04-22T11:37:36.238Z
Learning: In Swift code, when re-registering or updating an existing window/session context (e.g., in AppDelegate.registerMainWindow or similar flows), only update an existing *cmuxConfigStore* (or equivalent per-window configuration store) if the incoming configuration/store value is non-nil. Do not overwrite an existing per-window store with nil, so the previous per-window configuration is preserved across re-registration paths.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3128
File: Sources/Panels/BrowserPanelView.swift:4271-4290
Timestamp: 2026-04-23T11:23:49.934Z
Learning: In OmnibarSuggestionsView (and other omnibar-related debug/telemetry logging), never log raw omnibar suggestion content (e.g., URLs, titles, queries). Instead, log only non-sensitive metadata such as suggestion kind/category and the byte length of the text (e.g., "browser.suggestionClick kind=<kind> textBytes=<len>"). Apply this rule consistently to all omnibar-related debug logs to avoid leaking user/search data.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3046
File: Sources/TerminalController.swift:2096-2136
Timestamp: 2026-04-24T22:17:52.550Z
Learning: In Swift request/JSON handlers (e.g., v2 JSON-socket handlers) in Sources, prefer using the v2 helpers for numeric parsing—use `v2Int(params, "<key>")` for normal integer inputs and `v2StrictInt(...)` when strictness is required—rather than casting with `as? Int`. JSONSerialization may yield NSNumber/Double for numeric fields, so v2Int/v2StrictInt ensures correct extraction and type handling. If parsing is used for safety (e.g., timeouts), clamp/validate the parsed value as appropriate (as in `vm.exec` parsing `timeout_ms` and enforcing `>= 1`).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3182
File: Sources/ContentView.swift:10850-10875
Timestamp: 2026-04-27T10:11:36.830Z
Learning: When computing NSTextView content height for NSTextView-based editors (e.g., a method like naturalDocumentHeight(...)), account for trailing newline layout. Specifically, include `layoutManager.extraLineFragmentRect.height` in the measured height only when `extraLineFragmentTextContainer == textContainer`. If you don’t, the caret on the final blank line can be clipped. Apply this rule to future NSTextView-based editors in this repo.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3139
File: Sources/Panels/FilePreviewPanel.swift:534-537
Timestamp: 2026-04-28T05:45:32.192Z
Learning: When implementing workspace/panel teardown or close-confirmation logic in Sources (e.g., close/collapse/workspace-close flows), rely on the shared dirty-state gate driven by the panel’s `isDirty` property rather than adding panel-specific teardown special-casing. Ensure each panel (including `FilePreviewPanel`) exposes and keeps its `isDirty` state up to date (e.g., via `Published private(set) var isDirty` and any subscriptions/synchronization logic), so the generic `panel.isDirty` check correctly covers all panel types during teardown.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3218
File: Sources/AppDelegate.swift:4800-4802
Timestamp: 2026-04-28T11:43:53.356Z
Learning: In Swift code that selects or activates the next main-window context (e.g., iterating window-context collections), avoid iterating `mainWindowContexts.values` directly while calling `resolvedWindow(for:)`. Since `resolvedWindow(for:)` may reindex/mutate `mainWindowContexts`, this can cause mutation-during-enumeration issues. Instead, snapshot first with `Array(mainWindowContexts.values)`, then resolve/reindex against that snapshot, and only then call `activateMainWindowContext(_:)` using the resolved result.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3247
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-29T01:08:39.652Z
Learning: Maintain the behavior contract for “Copy Workspace ID(s)”: when triggered from the sidebar context menu (e.g., in Sources/ContentView.swift TabItemView), the command must copy plain UUIDs (IDs-only), not references/refs. For command palette identifier-copy commands where refs are required, ensure the implementation explicitly passes includeRefs: true. This preserves backward compatibility for scripts expecting UUID-only output while allowing the palette to return richer payloads when needed.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3256
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-04-29T01:20:59.683Z
Learning: In this repo’s Swift implementation, keep browser-creation/open behavior consistent when `BrowserAvailabilitySettings` is disabled. For both V1 and V2, any command that includes a URL when creating/opening a browser (e.g., `open_browser` with a URL, `v2 surface.create` with `type=browser` and `url`, `browser.tab.new` with `url`, `v2 browser.open_split` with `url`) must open the URL externally using `NSWorkspace.shared.open(...)` and return appropriate success metadata. Only URL-less/blank browser creations should fail with the `browser_disabled` error.
There was a problem hiding this comment.
@lawrencecchen, thanks for clarifying. You're right — looking at the original implementation, trafficLightOffset(for:) returned .zero for all non-settings windows already, so the only offset ever applied was the NSPoint(x: 7, y: -4) nudge specific to cmux.settings. Since PR #3244 migrates Settings to a native SwiftUI WindowGroup titlebar (eliminating the need for that manual alignment), collapsing the whole function to return .zero is correct and loses nothing. I'll record this for future reviews.
✏️ Learnings added
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3244
File: Sources/WindowDecorationsController.swift:71-73
Timestamp: 2026-04-29T06:25:03.715Z
Learning: Repo: manaflow-ai/cmux — Sources/WindowDecorationsController.swift — `trafficLightOffset(for:)` previously returned a non-zero offset (`NSPoint(x: 7, y: -4)`) only for the `cmux.settings` window identifier (to align traffic-light buttons with a custom Settings title row), and `.zero` for all other windows. PR `#3244` intentionally removed this Settings-specific offset by collapsing the function to `return .zero`, because Settings was migrated to a SwiftUI `WindowGroup` with a native system titlebar that no longer needs manual traffic-light nudging. Do not flag the unconditional `return .zero` in this method as a regression.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:3586-3604
Timestamp: 2026-04-14T20:00:21.500Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — On macOS 26+, the app intentionally keeps NSWindow.StyleMask.fullSizeContentView enabled even with the native (non-transparent) system titlebar. The titlebar handles dragging natively; isMovable=true and isMovableByWindowBackground=false are used. Any comment suggesting “without fullSizeContentView” is outdated.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2124
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-25T08:05:26.034Z
Learning: Repo: manaflow-ai/cmux — File: Sources/AppDelegate.swift — Pattern for “New Window” geometry seed:
In AppDelegate.createMainWindow(initialWorkingDirectory:sessionWindowSnapshot:), compute the existingFrame using preferredMainWindowContextForWorkspaceCreation(debugSource: "createMainWindow.initialGeometry") and then resolvedWindow(for:) rather than relying on NSApp.keyWindow or the first registered mainWindowContext. Rationale: ensures the new window inherits size from the intended main-terminal window even when an auxiliary window is key, and keeps behavior consistent with showOpenFolderPanel().
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3179
File: Sources/ContentView.swift:4005-4021
Timestamp: 2026-04-27T11:59:15.622Z
Learning: Repo: manaflow-ai/cmux — PR `#3179` — Sources/ContentView.swift
Learning: For separate-surface transparent windows, installNativeTitlebarBackdrop intentionally inserts NativeTitlebarBackdropView below contentView (NSHostingView) — positioned .below relativeTo: contentView — so the backdrop fills through transparent hosting without tinting SwiftUI titlebar text/buttons. The titlebar‑chrome ancestor fallback is only for edge cases where contentView isn’t directly under the NSThemeFrame. This layering was pixel‑verified in PR `#3179`; do not “fix” it to sit below the titlebar chrome on this path.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3179
File: Sources/ContentView.swift:3991-4002
Timestamp: 2026-04-27T09:15:53.326Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — Native titlebar backdrop layering: installNativeTitlebarBackdrop must place the backdrop between the NSThemeFrame’s background material and the standard titlebar chrome (insert .below the close button’s ancestor in the theme frame). When the titlebar-chrome ancestor lookup fails, fall back to inserting .below relativeTo: nil so the backdrop is behind existing siblings and does not overpaint the SwiftUI titlebar/sidebar strip. Visual matching has been verified via pixel samples.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/ContentView.swift:3194-3196
Timestamp: 2026-03-04T14:05:48.668Z
Learning: In manaflow-ai/cmux (PR `#819`), Sources/ContentView.swift: The command palette’s external window labels intentionally use the global window index from the full orderedSummaries (index + 1), matching the Window menu in AppDelegate. Do not reindex after filtering out the current window to avoid mismatches (“Window 2” for an external window is expected).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3166
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-27T06:58:21.434Z
Learning: In the Swift UI code (e.g., Sources/ContentView.swift), when `preferLiquidGlass`/`materialPolicy.preferLiquidGlass` is enabled for sidebar Liquid Glass, only omit the `Color` tint overlay if `NSGlassEffectView` (native Liquid Glass) is actually available. Compute `usingNativeLiquidGlass = materialPolicy.preferLiquidGlass && SidebarVisualEffectBackground.liquidGlassAvailable`, and when `usingNativeLiquidGlass` is false keep the overlay so the non-native `NSVisualEffectView` fallback still receives the configured tint.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3218
File: Sources/AppDelegate.swift:3456-3469
Timestamp: 2026-04-28T11:35:31.575Z
Learning: Repo: manaflow-ai/cmux — Sources/AppDelegate.swift (Swift)
Learning: The DEBUG-only helper unregisterMainWindowContextForTesting(windowId:) should mirror production teardown: after removing matching mainWindowContexts entries, call activateMainWindowContext(mainWindowContexts.values.first) when the active context was removed so tabManager, sidebarState, sidebarSelectionState, and fileExplorerState are repointed (or cleared when none remain). This prevents stale active pointers in tests.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3166
File: Sources/Windowing/WindowAppearanceSnapshot.swift:185-210
Timestamp: 2026-04-27T06:58:26.079Z
Learning: Repo: manaflow-ai/cmux — Sources/GhosttyTerminalView.swift & Sources/Windowing/WindowAppearanceSnapshot.swift — Snapshot refresh contract for usesHostLayerBackground:
`notifyDefaultBackgroundDidChange()` (and therefore the titlebar/window snapshot refresh in ContentView) must fire whenever `usesHostLayerBackground` changes, even if terminal background color and opacity are unchanged. The prior `hasChanged` check in `updateDefaultBackground()` only compared colors; it was extended so that a toggle of `usesHostLayerBackground` alone also triggers the notification. ContentView listens at the window level to rebuild `WindowAppearanceSnapshot` (including `terminalRenderingMode`) on any such notification. Fixed in commits 4913d17–979a683 (PR `#3166`).
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:15597-15615
Timestamp: 2026-04-14T20:05:53.504Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — On macOS 26 in this app, the only NSSplitView in the window’s view tree is the one owned by NavigationSplitView. Therefore SplitViewDividerHiderView.hideDividers()/patchSplitViews(in:) effectively scope to that single split. Do not flag “global divider clearing” for this path unless additional split views are introduced later.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/AppDelegate.swift:10168-10189
Timestamp: 2026-04-14T20:02:41.558Z
Learning: Repo manaflow-ai/cmux — On macOS 26, the notifications popover is owned by ContentView (State isNotificationsPopoverPresented). AppDelegate APIs (toggle/show/dismiss/isShown) must route via NotificationCenter with a window-scoped userInfo["windowId"] and track visibility per-window, not call UpdateTitlebarAccessoryController. ContentView should observe show/dismiss/toggle and post a visibilityDidChange notification {windowId, visible}.
Learnt from: tranquillum
Repo: manaflow-ai/cmux PR: 2827
File: Sources/Sidebar/ProviderAccountsFooterPanel.swift:174-198
Timestamp: 2026-04-17T15:46:53.298Z
Learning: Repo: manaflow-ai/cmux — Sources/Sidebar/ProviderAccountsFooterPanel.swift + Sources/Sidebar/ProviderAccountsPopover.swift (PR `#2827`):
- `ProviderStatusRanking.impactSeverity(_:)` intentionally returns `-1` for any unrecognized/`"none"` impact string.
- When `worstImpactSeverity == -1` (every incident in the list has an unknown/none impact), `ProviderStatusLabel.statusText` and `.dotColor` fall through to the `default` branch, which shows "Operational" / green.
- This is a deliberate design choice: `StatuspageIOFetcher.fetch` already defaults missing impact fields to `"none"`, so the only way to reach `worstImpactSeverity == -1` is a genuinely benign incident array; showing "Operational" here avoids false-positive yellow.
- The popover always surfaces the full raw incident rows independently, so no incident is invisible to the user.
- Do NOT flag the `-1 → Operational` fallback as a bug or suggest raising unknown impacts to `minor` severity.
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-04-28T22:24:17.987Z
Learning: Repo: manaflow-ai/cmux — In Sources/Workspace.swift (PR `#3237`, commit 9dcb9c2), the `newTerminalSurface(...)` method gained an `allowInitialInputWithRemoteStartupCommand: Bool` parameter (default `false`). The session-restore call site in `createPanel(from:inPane:)` passes `true` only when `!panelWasRemoteBacked && combinedInitialInput != nil`, making the per-panel snapshot flag the sole authority for restore-time `initialInput` injection. All 12 other call sites keep `false`, preserving the existing remote-drop defense.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3164
File: Sources/cmuxApp.swift:189-197
Timestamp: 2026-04-26T06:03:08.863Z
Learning: Repo: manaflow-ai/cmux — PR `#3164` — Bootstrap contract: In cmuxApp.bootstrapMainWindowScene(), AppDelegate.bootstrapInitialMainWindowIfNeeded(...) calls ensureInitialMainWindowIfNeeded() to create (if needed) the real AppKit main window via AppDelegate.createMainWindow, which wires CmuxConfigStore and FileExplorerState and calls registerMainWindow(... fileExplorerState:). Only after registration does updateSocketController() start TerminalController using AppDelegate.synchronizeActiveMainWindowContext(...). Conclusion: per‑window config/file‑explorer/directory initialization is handled in AppDelegate and occurs before TerminalController starts; do not flag missing setup in WindowGroup.onAppear.
Learnt from: pstanton237
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-04-06T12:03:16.921Z
Learning: Repo: manaflow-ai/cmux — In CLI/cmux.swift, `claudeTeamsLaunchArguments` must use two independent branches: (1) prepend `--teammate-mode auto` only when the caller has NOT already supplied `--teammate-mode`; (2) append `--settings <claudeHooksJSON>` unconditionally (modulo `CMUX_CLAUDE_HOOKS_DISABLED=1` opt-out). These two branches must never be coupled in a single early-return or conditional block — coupling them was the original `#2229` regression (hook JSON silently bypassed when `--teammate-mode auto|manual` was explicit). Mirrors `Resources/bin/claude:208`: `exec "$REAL_CLAUDE" --settings "$HOOKS_JSON" "$@"` where `$@` content never gates hook injection.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-16T08:02:06.824Z
Learning: In Swift sources, for any panel_id-only route handling in v2PanelMarkBackground(params:) and v2PanelMarkForeground(params:), first attempt v2ResolveTabManager(params:). Use the manager only if it actually owns the panelId; otherwise fall back to AppDelegate.shared?.locateSurface(surfaceId:) to locate the correct TabManager across windows. Apply this pattern to all panel_id-only routes to avoid active-window bias.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/AppDelegate.swift:2196-2200
Timestamp: 2026-04-03T03:36:45.112Z
Learning: Repo: manaflow-ai/cmux — In Sources/AppDelegate.swift, when KeyboardShortcutSettings.didChangeNotification fires, AppDelegate must clear configured-chord caches (pendingConfiguredShortcutChord and activeConfiguredShortcutChordPrefixForCurrentEvent) via clearConfiguredShortcutChordState() before refreshing tooltips/UI. Also clear chord state on applicationWillResignActive to avoid cross-activity leakage. Verified by cmuxTests/AppDelegateShortcutRoutingTests.swift::testShortcutChangeClearsPendingConfiguredChord.
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-04-28T22:24:17.987Z
Learning: Repo: manaflow-ai/cmux — In Sources/GhosttyTerminalView.swift (PR `#3237`, commit 9dcb9c2), the explicit-vs-baseConfig.initialInput resolution logic was extracted from `TerminalSurface.createSurface(for:)` into a standalone `TerminalSurface.resolveInitialInput(...)` static/instance method for unit-testability. Behavior is unchanged. A companion `TerminalSurfaceResolveInitialInputTests` suite asserts the contract, including byte-for-byte preservation of Ghostty raw-bytes startup-input via `baseConfig.initialInput`.
Learnt from: arieltobiana
Repo: manaflow-ai/cmux PR: 1873
File: Sources/TerminalController.swift:4071-4087
Timestamp: 2026-03-20T17:18:30.333Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceAction(params:) -> case "set_color": palette names are resolved via WorkspaceTabColorSettings.defaultPaletteWithOverrides(), whose entries are always valid hex (validated by the UI). Therefore, additional normalization of entry.hex is unnecessary.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:15597-15614
Timestamp: 2026-04-14T20:29:58.471Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift (PR `#2647`, macOS 26): SplitViewDividerHiderView hides split dividers by calling the private selector setDividerColor: only when responds(to:) is true (no KVC). Scope is effectively the single NSSplitView owned by NavigationSplitView. Do not flag KVC misuse or “global divider clearing” for this path unless additional NSSplitViews are added.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:2889-2911
Timestamp: 2026-04-14T19:59:54.878Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift — On macOS 26, the NavigationSplitView sidebar width is synchronized back to model state via a GeometryReader that updates sidebarWidth and SidebarState.persistedWidth on width changes. Do not flag “write-only sidebar width” drift for this path going forward.
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-04-28T22:24:17.987Z
Learning: Repo: manaflow-ai/cmux — In Sources/GhosttyTerminalView.swift (PR `#3237`, commit 9dcb9c2), `private var initialInput` on `TerminalSurface` was changed to `private(set) var initialInput` to allow `testable` consumers to assert the post-normalization value. The setter remains class-private.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3166
File: Sources/Windowing/WindowAppearanceSnapshot.swift:185-210
Timestamp: 2026-04-27T06:58:26.717Z
Learning: Repo: manaflow-ai/cmux — Sources/GhosttyTerminalView.swift & Sources/ContentView.swift — Snapshot refresh contract for usesHostLayerBackground (post PR `#3166`):
- `updateDefaultBackground()` must fire `notifyDefaultBackgroundDidChange()` (or equivalent generation bump) whenever `usesHostLayerBackground` changes, even if terminal background color and opacity are unchanged, because `WindowAppearanceSnapshot.terminalRenderingMode(usesHostLayerBackground:)` depends on it.
- ContentView must subscribe to the Ghostty default-background notification at the window level (not just color/opacity changes) so the titlebar/window snapshot (`windowAppearanceSnapshot`) is recomputed whenever the rendering mode toggles.
- Do NOT gate the notification solely on color hasChanged; rendering-mode-only transitions must also propagate through the same notification channel.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2575
File: Sources/cmuxApp.swift:0-0
Timestamp: 2026-04-06T08:59:21.923Z
Learning: Repo: manaflow-ai/cmux — File: Sources/cmuxApp.swift — In SettingsView, when handling SettingsNavigationRequest, clear the sidebar search first (settingsSearchText = "") before calling scrollTo; sections are conditionally rendered based on the active query, so clearing ensures the target anchors (e.g., SettingsSection.browser, SettingsSection.keyboardShortcuts, SettingsNavigationTarget.browserImport) exist.
Learnt from: rodchristiansen
Repo: manaflow-ai/cmux PR: 2647
File: Sources/ContentView.swift:2918-2932
Timestamp: 2026-04-14T20:05:27.511Z
Learning: Repo: manaflow-ai/cmux — Sources/ContentView.swift (PR `#2647`): On macOS 26, NavigationSplitView provides a native collapse chevron in the sidebar column header when the sidebar is visible. The code intentionally: (1) shows a custom toolbar toggle only when the sidebar is hidden to reopen it, and (2) uses SystemSidebarToggleStripper to remove the system-injected toolbar toggle to prevent duplication. Do not flag “no collapse affordance” while the sidebar is open.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2575
File: Sources/cmuxApp.swift:0-0
Timestamp: 2026-04-06T09:01:51.979Z
Learning: Repo: manaflow-ai/cmux — File: Sources/cmuxApp.swift — SettingsView now uses SwiftUI `.searchable(text:placement:prompt:)` for the sidebar search, which provides a native (accessible) clear button. Do not flag missing accessibility on a custom clear button in this view going forward.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
Learnt from: zlatkoc
Repo: manaflow-ai/cmux PR: 1368
File: Sources/Panels/BrowserPanel.swift:69-69
Timestamp: 2026-03-13T13:46:01.733Z
Learning: Do not wrap engine/brand name literals (e.g., displayName values such as Google, DuckDuckGo, Bing, Kagi, Startpage) in String(localized: ...). These are brand/product names that are not translatable UI text. Localization should apply to generic UI strings (labels, buttons, error messages, etc.). Apply this guideline across Swift source files under Sources/ (notably in BrowserPanel.swift and similar UI/engine-related strings) and flag only brand-name strings that are part of user-facing UI text appropriately for translation scope.
Learnt from: kjb0787
Repo: manaflow-ai/cmux PR: 1461
File: Sources/GhosttyTerminalView.swift:5904-5905
Timestamp: 2026-03-15T19:22:32.330Z
Learning: In Swift files under the Sources directory that manage terminal/scroll behavior, ensure the following: when preserving scroll across workspace switches, save savedScrollRow only if the scrollbar offset is greater than 0 (indicating the user has scrolled up). On restore, call scroll_to_row only if savedScrollRow is non-nil; if it is nil, rely on synchronizeScrollView() to keep bottom-pinned sessions following new output. This pattern should be applied wherever GhosttyTerminalView-like views implement setVisibleInUI(_:) to maintain consistent user scroll state across workspace switches.
Learnt from: pratikpakhale
Repo: manaflow-ai/cmux PR: 2011
File: Resources/Localizable.xcstrings:15256-15368
Timestamp: 2026-03-23T21:39:50.795Z
Learning: When reviewing this repo’s Swift localization usage, do not flag missing `String.localizedStringWithFormat` for calls that use the modern overload `String(localized: "key", defaultValue: "...\(variable)")` (where `defaultValue` is a `String.LocalizationValue` built with `\(…)`). That overload natively supports interpolation and the xcstrings/runtime substitution handles the resulting placeholders automatically. Only require `String.localizedStringWithFormat` when using the older `String(localized:)` overload that takes a plain `String` (i.e., where format arguments must be passed separately), such as for keys like `clipboard.sshError.single`.
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: Sources/TerminalController.swift:3620-3622
Timestamp: 2026-03-25T00:32:54.735Z
Learning: When validating or reporting workspace/tab colors in this repo, only accept and use 6-digit hex colors in the form `#RRGGBB` (no alpha, i.e., do not allow `#RRGGBBAA`). Ensure validation logic matches the existing behavior (e.g., WorkspaceTabColorSettings.normalizedHex(...) and TabManager.setTabColor(tabId:color:) as well as CLI/cmux.swift). Update any error/help text for workspace color to reference only `#RRGGBB` (not `#RRGGBBAA`).
Learnt from: mrosnerr
Repo: manaflow-ai/cmux PR: 2545
File: Sources/GhosttyTerminalView.swift:3891-3903
Timestamp: 2026-04-02T21:37:21.463Z
Learning: In Swift source files like Sources/GhosttyTerminalView.swift, avoid logging raw startup commands or initialInput even in DEBUG (to prevent leaking sensitive paths/tokens and multiline content). If you need to diagnose startup/input, log only non-sensitive metadata such as (1) presence flags (e.g., hasStartupCommand/hasInitialInput), (2) byte counts, and (3) the relevant surface id (so issues can be correlated without exposing the underlying strings).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2528
File: Sources/cmuxApp.swift:6439-6444
Timestamp: 2026-04-03T03:35:54.082Z
Learning: In this repo’s keyboard shortcut implementation, ensure `KeyboardShortcutSettings.setShortcut(...)` does nothing (no-op) when `KeyboardShortcutSettings.isManagedBySettingsFile(action)` returns `true` (i.e., the shortcut is managed via `settings.json`). This prevents writing back overrides into `UserDefaults` and keeps `settings.json` as the source of truth.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2964
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-17T21:35:25.493Z
Learning: In this repo’s shell session resume flow, always build resume commands using the cwd guard helper exposed by SessionEntry (e.g., resumeCommandWithCwd). The helper should produce a command of the form `cd <shell-quoted cwd> && <resumeCommand>`. Update all call sites that generate “resume” commands (e.g., clipboard actions, drag-drop terminal, and in-app resume) to use this helper so that rc files and newly spawned shells cannot start outside the intended directory. Avoid constructing resume commands directly without the guarded `cd` + shell-quoting + `&&` composition.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2978
File: Sources/Workspace.swift:0-0
Timestamp: 2026-04-22T08:13:36.833Z
Learning: In manaflow-ai/cmux, note that Sources/RestorableAgentSession.swift’s `SessionRestorableAgentSnapshot.resumeCommand` already includes a cwd guard when `workingDirectory` is present (it returns a string like `cd <shell-quoted cwd> && <resumeCommand>`). At call sites (e.g., `Workspace.createPanel(...)`), pass `.resumeCommand` through directly and do not prepend another `cd`/cwd guard or wrap it with an additional `cd <...> &&`—otherwise the working directory may be applied twice or incorrectly.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2978
File: Sources/Workspace.swift:0-0
Timestamp: 2026-04-22T08:14:04.901Z
Learning: In the cmux Swift sources, when restoring a restorable agent session, call sites should pass `resumeCommand` directly (ensuring it has the expected trailing newline if required) to `sendInputWhenReady`. Do not wrap `resumeCommand` with an additional `cd '<cwd>' && ...` guard, because `SessionRestorableAgentSnapshot.resumeCommand` already returns a `cwd`-guarded command; adding another guard can result in `double-cd`. Apply this especially along restore paths (e.g., `Sources/Workspace.swift` restore logic) whenever using `resumeCommand`.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3084
File: Sources/AppDelegate.swift:4958-4975
Timestamp: 2026-04-22T11:37:36.238Z
Learning: In Swift code, when re-registering or updating an existing window/session context (e.g., in AppDelegate.registerMainWindow or similar flows), only update an existing *cmuxConfigStore* (or equivalent per-window configuration store) if the incoming configuration/store value is non-nil. Do not overwrite an existing per-window store with nil, so the previous per-window configuration is preserved across re-registration paths.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3128
File: Sources/Panels/BrowserPanelView.swift:4271-4290
Timestamp: 2026-04-23T11:23:49.934Z
Learning: In OmnibarSuggestionsView (and other omnibar-related debug/telemetry logging), never log raw omnibar suggestion content (e.g., URLs, titles, queries). Instead, log only non-sensitive metadata such as suggestion kind/category and the byte length of the text (e.g., "browser.suggestionClick kind=<kind> textBytes=<len>"). Apply this rule consistently to all omnibar-related debug logs to avoid leaking user/search data.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3046
File: Sources/TerminalController.swift:2096-2136
Timestamp: 2026-04-24T22:17:52.550Z
Learning: In Swift request/JSON handlers (e.g., v2 JSON-socket handlers) in Sources, prefer using the v2 helpers for numeric parsing—use `v2Int(params, "<key>")` for normal integer inputs and `v2StrictInt(...)` when strictness is required—rather than casting with `as? Int`. JSONSerialization may yield NSNumber/Double for numeric fields, so v2Int/v2StrictInt ensures correct extraction and type handling. If parsing is used for safety (e.g., timeouts), clamp/validate the parsed value as appropriate (as in `vm.exec` parsing `timeout_ms` and enforcing `>= 1`).
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3182
File: Sources/ContentView.swift:10850-10875
Timestamp: 2026-04-27T10:11:36.830Z
Learning: When computing NSTextView content height for NSTextView-based editors (e.g., a method like naturalDocumentHeight(...)), account for trailing newline layout. Specifically, include `layoutManager.extraLineFragmentRect.height` in the measured height only when `extraLineFragmentTextContainer == textContainer`. If you don’t, the caret on the final blank line can be clipped. Apply this rule to future NSTextView-based editors in this repo.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3139
File: Sources/Panels/FilePreviewPanel.swift:534-537
Timestamp: 2026-04-28T05:45:32.192Z
Learning: When implementing workspace/panel teardown or close-confirmation logic in Sources (e.g., close/collapse/workspace-close flows), rely on the shared dirty-state gate driven by the panel’s `isDirty` property rather than adding panel-specific teardown special-casing. Ensure each panel (including `FilePreviewPanel`) exposes and keeps its `isDirty` state up to date (e.g., via `Published private(set) var isDirty` and any subscriptions/synchronization logic), so the generic `panel.isDirty` check correctly covers all panel types during teardown.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3218
File: Sources/AppDelegate.swift:4800-4802
Timestamp: 2026-04-28T11:43:53.356Z
Learning: In Swift code that selects or activates the next main-window context (e.g., iterating window-context collections), avoid iterating `mainWindowContexts.values` directly while calling `resolvedWindow(for:)`. Since `resolvedWindow(for:)` may reindex/mutate `mainWindowContexts`, this can cause mutation-during-enumeration issues. Instead, snapshot first with `Array(mainWindowContexts.values)`, then resolve/reindex against that snapshot, and only then call `activateMainWindowContext(_:)` using the resolved result.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3247
File: Sources/ContentView.swift:0-0
Timestamp: 2026-04-29T01:08:39.652Z
Learning: Maintain the behavior contract for “Copy Workspace ID(s)”: when triggered from the sidebar context menu (e.g., in Sources/ContentView.swift TabItemView), the command must copy plain UUIDs (IDs-only), not references/refs. For command palette identifier-copy commands where refs are required, ensure the implementation explicitly passes includeRefs: true. This preserves backward compatibility for scripts expecting UUID-only output while allowing the palette to return richer payloads when needed.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 3256
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-04-29T01:20:59.683Z
Learning: In this repo’s Swift implementation, keep browser-creation/open behavior consistent when `BrowserAvailabilitySettings` is disabled. For both V1 and V2, any command that includes a URL when creating/opening a browser (e.g., `open_browser` with a URL, `v2 surface.create` with `type=browser` and `url`, `browser.tab.new` with `url`, `v2 browser.open_split` with `url`) must open the URL externally using `NSWorkspace.shared.open(...)` and return appropriate success metadata. Only URL-less/blank browser creations should fail with the `browser_disabled` error.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/cmuxApp.swift (1)
1112-1243:⚠️ Potential issue | 🟡 MinorLocalize the renamed About titlebar debug UI.
This rename still leaves new user-facing strings hard-coded in English (
About Window,Hidden,Visible,Reset All, etc.). The window title key was localized, but the rest of the new surface still bypasses xcstrings.As per coding guidelines, "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")with keys inResources/Localizable.xcstringsfor all supported languages (English and Japanese), never use bare string literals in SwiftUIText(),Button(), alert titles, etc."Also applies to: 1404-1493
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 1112 - 1243, The UI strings introduced in AboutWindowKind.displayTitle, TitlebarVisibilityOption.displayTitle, TitlebarToolbarStyleOption.displayTitle and any hard-coded labels used with AboutTitlebarDebugOptions (e.g., "About Window", "Hidden", "Visible", "Reset All", the default windowTitle "About cmux", etc.) must be replaced with localized lookups using String(localized: "key.name", defaultValue: "English text"); update each enum's displayTitle and the default windowTitle in AboutTitlebarDebugOptions.defaults to call String(localized:..., defaultValue:...), create corresponding keys in Resources/Localizable.xcstrings for English and Japanese, and ensure any SwiftUI Text/Button/Alert usages refer to these localized values rather than raw string literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 2427-2438: The Settings window is missing a minimum size, so add a
constraint on the NSWindow instance (the window created and passed to
super.init) to prevent the split view from collapsing: set window.minSize (or
window.contentMinSize) to the intended minimum dimensions for the two-column
layout after creating the window and before assigning contentViewController —
reference the existing window variable and SettingsRootView to determine
appropriate minimum width/height and apply via window.minSize = NSSize(width:
..., height: ...) (or window.contentMinSize) so the UI cannot be resized smaller
than the intended layout.
- Around line 8354-8364: The toolbar currently only adds a Reset action; add a
second ToolbarItem in the same .toolbar block that exposes the quick
"settings.json" action so users can open/edit the raw settings from everywhere
in Settings. Locate the existing ToolbarItem/ Button that calls
SettingsResetRequest.post() and add another ToolbarItem (or inline sibling) with
a Button that calls the settings-json action (e.g.
SettingsOpenJSONRequest.post() or the existing method used elsewhere to open the
settings.json editor), using a clear Label like String(localized:
"settings.section.openJSON", defaultValue: "Edit settings.json") and an
appropriate systemImage such as "doc.text" so it mirrors the Keyboard Shortcuts
entry point.
- Around line 2623-2647: The sidebar searchText property currently appends
hard-coded English keyword strings (in the switch cases of the searchText
computed property) which must be localized; replace each literal keyword string
in every case (e.g., in the .account, .app, .terminal, .workspaceColors,
.sidebarAppearance, .automation, .browser, .browserImport, .globalHotkey,
.keyboardShortcuts, .reset branches of var searchText) with String(localized:
"sidebar.searchKeywords.<case>", defaultValue: "<English keywords>") and ensure
matching keys and Japanese translations are added to
Resources/Localizable.xcstrings so the search corpus works in localized builds.
- Around line 8320-8374: The stored selectedSectionRaw isn't being replayed when
the Settings view first appears, so add a one-time replay on appearance: in the
Settings view (where selectedSectionRaw, selectedSection and filteredSections
are defined) call SettingsNavigationRequest.post(selectedSection) inside an
.onAppear or .task handler so the sidebar/detail scroll sync runs on initial
presentation; use the existing selectedSection computed property and the
SettingsNavigationRequest.post(...) API to trigger the same behavior you already
use in the onChange handler.
---
Outside diff comments:
In `@Sources/cmuxApp.swift`:
- Around line 1112-1243: The UI strings introduced in
AboutWindowKind.displayTitle, TitlebarVisibilityOption.displayTitle,
TitlebarToolbarStyleOption.displayTitle and any hard-coded labels used with
AboutTitlebarDebugOptions (e.g., "About Window", "Hidden", "Visible", "Reset
All", the default windowTitle "About cmux", etc.) must be replaced with
localized lookups using String(localized: "key.name", defaultValue: "English
text"); update each enum's displayTitle and the default windowTitle in
AboutTitlebarDebugOptions.defaults to call String(localized:...,
defaultValue:...), create corresponding keys in Resources/Localizable.xcstrings
for English and Japanese, and ensure any SwiftUI Text/Button/Alert usages refer
to these localized values rather than raw string literals.
🪄 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: f16ea4f1-76ef-4fa3-a27a-d0a8a5a46cf0
📒 Files selected for processing (2)
Resources/Localizable.xcstringsSources/cmuxApp.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Resources/Localizable.xcstrings
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 187987d1b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| @MainActor | ||
| enum SettingsWindowPresenter { | ||
| static let windowID = "settings" |
There was a problem hiding this comment.
This migration stops assigning the settings window the legacy cmux.settings identifier, but shortcut routing still keys off that value (for example cmuxWindowShouldOwnCloseShortcut and the closeOtherTabsInPane branch in AppDelegate). When Settings is focused, commands like Cmd+W can now be misrouted to the active terminal workspace instead of closing the Settings window, which can close user tabs unexpectedly.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Fixed: the SwiftUI Settings window is now configured through WindowAccessor with the legacy cmux.settings NSWindow identifier for shortcut routing.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Fixed: the SwiftUI Settings window is now configured through WindowAccessor with the legacy cmux.settings NSWindow identifier for shortcut routing.
— Claude Code
| cmuxDebugLog("settings.window.show path=swiftuiWindowGroup") | ||
| #endif | ||
| if let navigationTarget { | ||
| pendingNavigationTarget = navigationTarget |
There was a problem hiding this comment.
show(navigationTarget:) always stores pendingNavigationTarget, then also posts navigation immediately. If Settings is already open, onAppear never runs, so that pending value is never consumed. The next time Settings is reopened without a target, consumePendingNavigationTarget() applies that stale value and jumps to an unexpected section. This is reproducible by opening Settings, sending a targeted open request (e.g. keyboard shortcuts), closing Settings, then reopening normally.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Fixed: targeted opens only leave pendingNavigationTarget for new or queued windows; existing windows clear pending state and receive the navigation request directly.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Fixed: targeted opens only leave pendingNavigationTarget for new or queued windows; existing windows clear pending state and receive the navigation request directly.
— Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a918a4cbb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let navigationTarget { | ||
| pendingNavigationTarget = navigationTarget | ||
| } | ||
| openWindow?() |
There was a problem hiding this comment.
SettingsWindowPresenter.show silently no-ops when openWindow hasn’t been configured yet (openWindow?()), but callers like presentPreferencesWindow and the socket settings.open path treat show as guaranteed to display Settings. Because configuration only happens later in MainWindowBootstrapView.onAppear, any early call before that lifecycle point drops the request and leaves users with no window (only app activation). Add a fallback or queue-open behavior when openWindow is still nil.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Fixed: SettingsWindowPresenter now queues an open request if openWindow is not configured yet, then replays it when configure(openWindow:) runs.
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/cmuxApp.swift (1)
1129-1204:⚠️ Potential issue | 🟡 MinorLocalize the new About-titlebar debug labels and fallback titles.
This rename introduced several new raw UI strings (
About Window,Hidden,Visible,Reset All,Apply Now,About cmux, etc.). They will stay English in localized builds.As per coding guidelines, "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")with keys inResources/Localizable.xcstringsfor all supported languages (English and Japanese), never use bare string literals in SwiftUIText(),Button(), alert titles, etc."Also applies to: 1237-1242, 1427-1505
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 1129 - 1204, Replace all bare user-facing string literals in the shown enums and related UI code with localized lookups using String(localized: "key.name", defaultValue: "English text"); specifically update TitlebarVisibilityOption.displayTitle ("Hidden", "Visible"), TitlebarToolbarStyleOption.displayTitle (all case titles), the About-related properties displayTitle and fallbackTitle ("About Window", "About cmux"), and any other new UI labels like "Reset All" and "Apply Now" to use String(localized:..., defaultValue:...); add corresponding keys to Resources/Localizable.xcstrings for English and Japanese and keep the same defaultValue English text so localized builds pick up translations.
♻️ Duplicate comments (3)
Sources/cmuxApp.swift (3)
2502-2526:⚠️ Potential issue | 🟠 MajorLocalize the sidebar search keyword corpus too.
These search synonyms are still hard-coded English. In Japanese builds, sidebar search will only reliably match the localized titles and miss most of the extra keywords introduced here.
As per coding guidelines, "All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")with keys inResources/Localizable.xcstringsfor all supported languages (English and Japanese), never use bare string literals in SwiftUIText(),Button(), alert titles, etc."Also applies to: 2596-2668
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 2502 - 2526, The hard-coded English keyword corpus in the searchText computed property (and the similar block around lines 2596-2668) must be localized: replace each literal return string in the switch cases (e.g., cases .account, .app, .terminal, .workspaceColors, .sidebarAppearance, .automation, .browser, .browserImport, .globalHotkey, .keyboardShortcuts, .reset) with calls to String(localized: "sidebar.search.<case>.keywords", defaultValue: "English keywords…") using unique keys added to Resources/Localizable.xcstrings; keep the title interpolation (e.g., "\(title) …") but localize the rest of the keyword text so Japanese builds get translated synonyms.
8451-8457:⚠️ Potential issue | 🟠 MajorReplay the restored section when Settings first appears.
When
selectedSectionRawrestores to something other than.account, this path only reselects the sidebar row. The detailSettingsViewnever gets a navigation request, so the window can reopen with “Browser” selected while the scroll position is still at the top.💡 Suggested fix
.onAppear { if let target = SettingsWindowPresenter.consumePendingNavigationTarget() { navigate(to: target, postRequest: true) } else if !isSearching { selectedSidebarEntryID = SettingsSearchIndex.sectionID(for: selectedSection) + DispatchQueue.main.async { + SettingsNavigationRequest.post(selectedSection) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 8451 - 8457, When Settings appears and selectedSectionRaw restored to something other than .account, the code only sets selectedSidebarEntryID and never sends a navigation request to the SettingsView; update the .onAppear block so that when there's no pending navigation target AND selectedSectionRaw (or selectedSection) is not .account you explicitly call navigate(to: ..., postRequest: true) with a Settings navigation target (use the same target shape used elsewhere, e.g. the enum/case passed into navigate(to:postRequest:)) or obtain a target from SettingsWindowPresenter and pass it into navigate(to:postRequest:) so the detail SettingsView receives the navigation request and restores its scroll/state.
743-749:⚠️ Potential issue | 🟠 MajorReapply the Settings minimum size on the SwiftUI scene.
defaultSizeonly affects the first presentation. The newWindowGrouppath no longer enforces the old minimum, so the split view can still be dragged below the intended two-column layout and start clipping.💡 Suggested fix
WindowGroup(String(localized: "settings.title", defaultValue: "Settings"), id: SettingsWindowPresenter.windowID) { SettingsRootView() + .frame(minWidth: 820, minHeight: 540) } .defaultSize(width: 980, height: 680) .commands { SidebarCommands() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 743 - 749, The Settings WindowGroup (the one with SettingsWindowPresenter.windowID and SettingsRootView) currently uses .defaultSize which only affects the first presentation; instead locate the actual NSWindow for that window ID when the scene appears (e.g., in an .onAppear or .task attached to the WindowGroup / SettingsRootView) and set its minSize to CGSize(width: 980, height: 680) (and optionally maxSize if needed) so the split view cannot be resized below the intended two-column layout; ensure you reference SettingsWindowPresenter.windowID when finding the NSWindow and apply the minSize change there.
🧹 Nitpick comments (1)
Prototypes/SettingsShellLab/Sources/SettingsShellLab/Views/SettingsShellView.swift (1)
22-27: Show an explicit empty-search state instead of a blank sidebar.When
filteredSectionsis empty, the list currently renders nothing, which makes search feel broken. Add a localized “No Results” row/state for clearer UX.Suggested tweak
List(selection: $selectedSectionRaw) { - ForEach(filteredSections) { section in - Label(section.title, systemImage: section.symbolName) - .tag(section.rawValue) + if filteredSections.isEmpty { + Text(String(localized: "settings.search.noResults", defaultValue: "No Results")) + .foregroundStyle(.secondary) + } else { + ForEach(filteredSections) { section in + Label(section.title, systemImage: section.symbolName) + .tag(section.rawValue) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Prototypes/SettingsShellLab/Sources/SettingsShellLab/Views/SettingsShellView.swift` around lines 22 - 27, When filteredSections is empty the sidebar shows nothing; update the List in SettingsShellView to display a localized "No Results" row instead of rendering nothing: check filteredSections.isEmpty and, when true, add a single Label row (e.g. using Label(NSLocalizedString("No Results", comment: ""), systemImage: "magnifyingglass") or a localized string key) that is not selectable (doesn't modify selectedSectionRaw) so users see an explicit empty-search state; keep the existing ForEach(section in filteredSections) path unchanged for non-empty results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 6601-6614: presentPreferencesWindow currently calls
SettingsWindowPresenter.show(navigationTarget:) which forwards to openWindow?()
without checking existing windows; update show() (and/or
presentPreferencesWindow) to first search for an existing Settings window
instance (by window title, windowScene identifier, or a stored window identifier
used by the Settings WindowGroup), and if found: if window.isMiniaturized call
window.deminiaturize(nil) and then call
window.makeKeyAndOrderFront(nil)/orderFrontRegardless (or
NSRunningApplication.current.activate with .activateAllWindows) to re-raise it
instead of opening a new window; only call openWindow?() to create a new
settings window when no existing window is found. Ensure changes touch
SettingsWindowPresenter.show and presentPreferencesWindow (and any stored
identifier used by the WindowGroup) so repeated calls deduplicate and re-front
minimized windows.
In `@Sources/cmuxApp.swift`:
- Around line 2741-2751: The show(navigationTarget:) function leaves stale
pendingNavigationTarget when called without a target; change it so that when
navigationTarget is non-nil you set pendingNavigationTarget = navigationTarget,
and when navigationTarget is nil you explicitly clear pendingNavigationTarget
(set to nil) before calling openWindow, then only call
SettingsNavigationRequest.post(navigationTarget) when navigationTarget is
non-nil; update the logic around pendingNavigationTarget, openWindow, and
SettingsNavigationRequest.post to perform the clear-first-or-set behavior.
---
Outside diff comments:
In `@Sources/cmuxApp.swift`:
- Around line 1129-1204: Replace all bare user-facing string literals in the
shown enums and related UI code with localized lookups using String(localized:
"key.name", defaultValue: "English text"); specifically update
TitlebarVisibilityOption.displayTitle ("Hidden", "Visible"),
TitlebarToolbarStyleOption.displayTitle (all case titles), the About-related
properties displayTitle and fallbackTitle ("About Window", "About cmux"), and
any other new UI labels like "Reset All" and "Apply Now" to use
String(localized:..., defaultValue:...); add corresponding keys to
Resources/Localizable.xcstrings for English and Japanese and keep the same
defaultValue English text so localized builds pick up translations.
---
Duplicate comments:
In `@Sources/cmuxApp.swift`:
- Around line 2502-2526: The hard-coded English keyword corpus in the searchText
computed property (and the similar block around lines 2596-2668) must be
localized: replace each literal return string in the switch cases (e.g., cases
.account, .app, .terminal, .workspaceColors, .sidebarAppearance, .automation,
.browser, .browserImport, .globalHotkey, .keyboardShortcuts, .reset) with calls
to String(localized: "sidebar.search.<case>.keywords", defaultValue: "English
keywords…") using unique keys added to Resources/Localizable.xcstrings; keep the
title interpolation (e.g., "\(title) …") but localize the rest of the keyword
text so Japanese builds get translated synonyms.
- Around line 8451-8457: When Settings appears and selectedSectionRaw restored
to something other than .account, the code only sets selectedSidebarEntryID and
never sends a navigation request to the SettingsView; update the .onAppear block
so that when there's no pending navigation target AND selectedSectionRaw (or
selectedSection) is not .account you explicitly call navigate(to: ...,
postRequest: true) with a Settings navigation target (use the same target shape
used elsewhere, e.g. the enum/case passed into navigate(to:postRequest:)) or
obtain a target from SettingsWindowPresenter and pass it into
navigate(to:postRequest:) so the detail SettingsView receives the navigation
request and restores its scroll/state.
- Around line 743-749: The Settings WindowGroup (the one with
SettingsWindowPresenter.windowID and SettingsRootView) currently uses
.defaultSize which only affects the first presentation; instead locate the
actual NSWindow for that window ID when the scene appears (e.g., in an .onAppear
or .task attached to the WindowGroup / SettingsRootView) and set its minSize to
CGSize(width: 980, height: 680) (and optionally maxSize if needed) so the split
view cannot be resized below the intended two-column layout; ensure you
reference SettingsWindowPresenter.windowID when finding the NSWindow and apply
the minSize change there.
---
Nitpick comments:
In
`@Prototypes/SettingsShellLab/Sources/SettingsShellLab/Views/SettingsShellView.swift`:
- Around line 22-27: When filteredSections is empty the sidebar shows nothing;
update the List in SettingsShellView to display a localized "No Results" row
instead of rendering nothing: check filteredSections.isEmpty and, when true, add
a single Label row (e.g. using Label(NSLocalizedString("No Results", comment:
""), systemImage: "magnifyingglass") or a localized string key) that is not
selectable (doesn't modify selectedSectionRaw) so users see an explicit
empty-search state; keep the existing ForEach(section in filteredSections) path
unchanged for non-empty results.
🪄 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: 73a23c29-b54d-4dd1-b60e-9b98b751c1a0
📒 Files selected for processing (6)
Prototypes/SettingsShellLab/Sources/SettingsShellLab/App/SettingsShellLabApp.swiftPrototypes/SettingsShellLab/Sources/SettingsShellLab/Views/SettingsShellView.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/TerminalController.swiftSources/cmuxApp.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Prototypes/SettingsShellLab/Sources/SettingsShellLab/App/SettingsShellLabApp.swift
- Sources/TerminalController.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10a792d1d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| WindowGroup(String(localized: "settings.title", defaultValue: "Settings"), id: SettingsWindowPresenter.windowID) { |
There was a problem hiding this comment.
Defining Settings as a WindowGroup allows repeated openWindow(id:) calls to create additional Settings windows, which regresses the previous singleton behavior and makes global navigation broadcasts (SettingsNavigationRequest.post) affect every open Settings window at once. This is user-visible when opening Settings multiple times from menu/shortcuts or socket commands, because section selection and scroll routing no longer target a single canonical window.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Fixed: the Settings scene is now Window rather than WindowGroup, so repeated openWindow calls target one Settings window.
— Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec7ca8ff8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+204
to
+205
| TimelineView(.animation) { context in | ||
| let opacity = highlightOpacity(at: context.date, for: highlightState) |
There was a problem hiding this comment.
Gate the TimelineView(.animation) behind an active highlight check, because this modifier is attached to nearly every searchable row/section and currently creates a continuously animating timeline even when highlightOpacity is 0. In practice, simply opening Settings with no active search highlight still drives periodic redraw work across the full settings tree, which can cause unnecessary CPU/GPU usage and UI jank in this window.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Fixed: the highlight modifier now creates TimelineView only for the active matching search highlight row.
— Claude Code
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 440d5cce94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+2514
to
+2516
| NSRunningApplication.current.activate(options: [.activateAllWindows, .activateIgnoringOtherApps]) | ||
| window.makeKeyAndOrderFront(nil) | ||
| window.orderFrontRegardless() |
There was a problem hiding this comment.
settings.open is routed through the non-activating path when activate is false (v2SettingsOpen calls SettingsWindowPresenter.show), but focus(_:) now unconditionally calls NSRunningApplication.current.activate(...) and orderFrontRegardless(). This makes background automation requests steal macOS focus whenever a Settings window already exists (visible or minimized), which breaks the command’s non-activation contract and the socket focus policy.
Useful? React with 👍 / 👎.
This was referenced Apr 30, 2026
This was referenced May 8, 2026
This was referenced Jul 10, 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
Summary by cubic
Moves Settings to a SwiftUI
WindowGroupwith a native split‑view sidebar and smarter search. Adds programmatic navigation, asettings.jsonentry, and fixes an activation warning when opening the window.New Features
NavigationSplitViewsidebar with search: tokenized match, click‑only fade highlights, “No Results,” latest‑wins auto‑scroll; moved Workspace Colors below Shortcuts; wand icon for Automation.settings.jsonsection to open the active cmux settings file; docs updated with#settings-jsonanchor; adds lightweightSettingsShellLabprototype withscript/build_and_run.sh.Refactors
SettingsWindowControllerwithSettingsWindowPresenterin a SwiftUIWindowGroup; wired to SwiftUIopenWindow; updated AppDelegate and terminal RPC; fixed Settings window activation warning and settings shortcut routing test cleanup.SettingsNavigationfor reuse.Written for commit 0e607d2. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
New Prototype
Bug Fixes
Localization