Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughAdds a Monaco-based editor panel (with diff mode), a CLI Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Monaco as Monaco Editor
participant Bridge as JS Bridge (editor-bridge.js)
participant EditorPanel as EditorPanel (Swift)
participant Notification as NotificationCenter
participant TerminalCtrl as TerminalController
participant Terminal as Terminal Surface
User->>Monaco: select text + trigger "send" (Ctrl/Cmd+S or UI)
Monaco->>Bridge: postMessage { type: "sendSelection", content }
Bridge->>EditorPanel: JS message handler invokes sendSelection(content)
EditorPanel->>Notification: post editorDidSendSelection (workspace/editor/return IDs + content)
Notification->>TerminalCtrl: deliver selection notification
TerminalCtrl->>Terminal: resolve target surface and inject/paste content
sequenceDiagram
participant CLI as CLI (cmux)
participant Controller as TerminalController
participant Git as Git
participant Workspace as Workspace
participant EditorPanel as EditorPanel (Swift)
participant Monaco as Monaco Editor
CLI->>Controller: RPC editor.diff { path, base? }
Controller->>Controller: validate absolute path & readability
alt base provided
Controller->>Controller: use provided base content
else
Controller->>Git: git show HEAD:<relPath>
Git-->>Controller: base content or error
end
Controller->>Workspace: create/focus editor panel for path
Workspace->>EditorPanel: open file and enterDiffMode(baseContent, baseLabel)
EditorPanel->>Monaco: setDiffContent(original=base, modified=current)
Monaco-->>EditorPanel: ready / events
Controller-->>CLI: OK { window_id, workspace_id, surface_id, pane_id }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
CLI/cmux.swift-3157-3180 (1)
3157-3180:⚠️ Potential issue | 🟡 MinorExtra positional args are silently dropped.
Only
subArgs.firstis consumed downstream as the path, but several branches assign multi-element arrays tosubArgswithout rejecting trailing positionals:
- Branch 1 (
open/diff):cmux editor open file.txt junk1 junk2→subArgs = ["file.txt", "junk1", "junk2"];junk1/junk2are silently ignored.- Branch 4 (path shorthand):
cmux editor file.txt junk1→subArgs = args; same issue.This makes typos and stray shell-glob expansions fail silently instead of erroring, which can be confusing (e.g.,
cmux editor *.swiftwould only open the first match).🛠️ Suggested fix: error on extra positional args
guard let rawPath = subArgs.first, !rawPath.isEmpty else { throw CLIError(message: "editor \(subcommand) requires a file path. Usage: cmux editor \(subcommand) <path>") } + if subArgs.count > 1 { + let extras = subArgs.dropFirst().joined(separator: " ") + throw CLIError( + message: "editor \(subcommand) accepts a single <path>; unexpected extra arguments: \(extras)" + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 3157 - 3180, The code allows extra positional arguments to be silently dropped by assigning multi-element arrays to subArgs and only using subArgs.first; update the parsing after you determine subcommand/subArgs (symbols: subcommand, subArgs, rawPath) to validate that subArgs.count == 1 (or else throw a CLIError) so any trailing positional args cause a clear error message like "editor <subcommand> expects a single path argument; got N arguments"; ensure this check runs for the branches that set subArgs (the branch handling ["open","diff"] and the path-shorthand branch that assigns subArgs = args) before using rawPath, and reuse looksLikePath where needed to detect the path branch.Resources/editor-monaco/vs/basic-languages/rust/rust.js-1-3 (1)
1-3:⚠️ Potential issue | 🟡 MinorAdd vendor documentation for Monaco bundle or commit the regeneration source.
Version consistency confirmed: all 95 files in
Resources/editor-monaco/self-reportVersion: 0.52.2(404545bded1df6ffa41ea0af4e8ddb219018c6c1), including the loader, worker, editor.main, all language modules, and localization files. The bundle appears to be an unmodified vendor drop from upstream.However, there is no vendor manifest, README, or regeneration steps documented anywhere in the repository. To make future maintenance and updates easier (e.g., upgrading Monaco or verifying the bundle hasn't drifted), either:
- Add a
Resources/editor-monaco/VENDOR.mdor similar documenting the upstream source (commit/tag), how the bundle was extracted, and steps to regenerate it- Or document this in the main
README.mdif it covers vendor dependenciesThis prevents silent bugs from mismatched Monaco versions or forked/modified files going unnoticed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/editor-monaco/vs/basic-languages/rust/rust.js` around lines 1 - 3, The Monaco bundle files (e.g., Resources/editor-monaco/vs/basic-languages/rust/rust.js which contains the Version: 0.52.2(404545bded1df6ffa41ea0af4e8ddb219018c6c1) header) are a vendor drop but lack any upstream provenance or regeneration steps; add a vendor manifest (e.g., Resources/editor-monaco/VENDOR.md) or update the repository README to document the upstream source (repo URL and commit/tag), the exact commit/id shown in the Version header, how the bundle was extracted or built, and step-by-step regeneration instructions (commands used, any build flags, and how to validate versions) so future maintainers can verify or regenerate the bundle.Resources/editor-monaco/editor-bridge.js-105-110 (1)
105-110:⚠️ Potential issue | 🟡 MinorTheme override is dropped on diff→content recreation.
Re-creating the editor with
theme: detectTheme()ignores any explicit theme previously set by Swift viacmuxEditor.setTheme(isDark), so a user-forced theme reverts to the system preference after the first diff transition. Cache the last theme set (initialized fromdetectTheme()and updated insetTheme) and use that here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/editor-monaco/editor-bridge.js` around lines 105 - 110, The editor recreation currently always uses theme: detectTheme(), which ignores any explicit theme set via cmuxEditor.setTheme(isDark); change the implementation to cache the last theme choice (initialize a module-level variable from detectTheme() on load), update that cached value inside the setTheme (or cmuxEditor.setTheme) handler when Swift forces a theme, and then pass the cached theme value instead of detectTheme() when calling monaco.editor.create so forced themes persist across diff→content re-creations.Sources/TerminalController.swift-7897-7902 (1)
7897-7902:⚠️ Potential issue | 🟡 MinorParity gaps with
v2EditorOpenvalidation.Two minor inconsistencies vs.
v2EditorOpen:
isReadableFile(atPath:)returnstruefor readable directories too.editor.openrejects directories explicitly (lines 7810–7816);editor.diffdoes not, so passing a directory will fall through to the git subprocess and surface a confusinggit_error.editor.diffdoes not assertws.panels[sourceSurfaceId] != nil(lines 7835–7838 inv2EditorOpen), so an unknownsurface_idreachesopenOrFocusEditorSplit(from:)and is silently used as the split origin.Worth tightening for symmetry and clearer error codes.
♻️ Suggested adjustment
guard filePath.hasPrefix("/") else { return .err(code: "invalid_params", message: "Path must be absolute: \(filePath)", data: ["path": filePath]) } + var isDir: ObjCBool = false + guard FileManager.default.fileExists(atPath: filePath, isDirectory: &isDir) else { + return .err(code: "not_found", message: "File not found: \(filePath)", data: ["path": filePath]) + } + guard !isDir.boolValue else { + return .err(code: "invalid_params", message: "Path is a directory, not a file: \(filePath)", data: ["path": filePath]) + } guard FileManager.default.isReadableFile(atPath: filePath) else { - return .err(code: "not_found", message: "File not found or not readable: \(filePath)", data: ["path": filePath]) + return .err(code: "permission_denied", message: "File not readable: \(filePath)", data: ["path": filePath]) }let sourceSurfaceId = v2UUID(params, "surface_id") ?? ws.focusedPanelId guard let sourceSurfaceId else { result = .err(code: "not_found", message: "No focused surface to split", data: nil) return } + guard ws.panels[sourceSurfaceId] != nil else { + result = .err(code: "not_found", message: "Source surface not found", data: ["surface_id": sourceSurfaceId.uuidString]) + return + }Also applies to: 7963-7967
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 7897 - 7902, The editor.diff validation should mirror v2EditorOpen: after checking filePath is absolute, replace the current isReadableFile check with a check that the path exists, is readable, and is NOT a directory (use FileManager.fileExists(atPath:isDirectory:) to detect directories) and return a clear error like "not_found" or "invalid_params" if it's a directory; additionally, before calling openOrFocusEditorSplit(from:) ensure ws.panels[sourceSurfaceId] != nil and return a specific error (e.g. "unknown_surface") if the surface_id is missing so an invalid surface doesn't silently propagate—apply the same fixes to the other occurrence around lines 7963–7967.Sources/Panels/EditorPanel.swift-436-487 (1)
436-487:⚠️ Potential issue | 🟡 MinorExtensionless filenames (Dockerfile, Makefile, etc.) fall through to plaintext.
monacoLanguagederives the key from(filePath as NSString).pathExtension, so forDockerfile,Makefile,.gitignore,CMakeLists.txt(where the canonical extension carries no information), the"dockerfile"/ similar branches are unreachable and the editor renders as plaintext. Consider matching on basename first when there's no extension.♻️ Suggested addition
var monacoLanguage: String { guard !filePath.isEmpty else { return "plaintext" } - return Self.monacoLanguageFromExtension((filePath as NSString).pathExtension) + let ns = filePath as NSString + let ext = ns.pathExtension + if ext.isEmpty { + return Self.monacoLanguageFromBasename(ns.lastPathComponent) + } + return Self.monacoLanguageFromExtension(ext) } + + private static func monacoLanguageFromBasename(_ name: String) -> String { + switch name.lowercased() { + case "dockerfile", "containerfile": return "dockerfile" + case "makefile", "gnumakefile": return "makefile" + case "cmakelists.txt": return "cmake" + default: return "plaintext" + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/EditorPanel.swift` around lines 436 - 487, monacoLanguageFromExtension currently only matches on the extension so extensionless filenames like "Dockerfile", "Makefile", "CMakeLists.txt", ".gitignore", etc. fall through to "plaintext"; update monacoLanguageFromExtension (or create a thin wrapper like monacoLanguageFromPath) to check the basename when the extracted extension is empty: compute lastPathComponent / basename (lowercased) and compare against known filenames ("dockerfile", "makefile", "cmakelists.txt", ".gitignore", "gemfile", etc.) before returning "plaintext", preserving all existing extension-based cases.Sources/Panels/EditorPanelView.swift-58-62 (1)
58-62:⚠️ Potential issue | 🟡 MinorEditorPanelView must listen to CmuxWebView clicks to focus the panel.
The
.onTapGestureon the VStack won't fire when the user clicks inside the Monaco editor because WKWebView consumes pointer events. EditorPanel creates a CmuxWebView (line 202 in EditorPanel.swift), which posts.webViewDidReceiveClickonmouseDown, but EditorPanelView doesn't listen to this notification.BrowserPanelView solves this by adding
.onReceive(NotificationCenter.default.publisher(for: .webViewDidReceiveClick))to filter clicks from its own webView and callonRequestPanelFocus(). Apply the same pattern to EditorPanelView.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/EditorPanelView.swift` around lines 58 - 62, EditorPanelView currently uses .onTapGesture which won't trigger for clicks inside the Monaco editor (CmuxWebView); add a .onReceive(NotificationCenter.default.publisher(for: .webViewDidReceiveClick)) to EditorPanelView and, when receiving the notification, verify the notification.object matches the EditorPanel's CmuxWebView instance (same instance created in EditorPanel) then call onRequestPanelFocus() so clicks inside the web view focus the panel; reference .webViewDidReceiveClick, EditorPanelView, CmuxWebView, and onRequestPanelFocus when locating where to add this listener.Sources/Panels/EditorPanel.swift-327-341 (1)
327-341:⚠️ Potential issue | 🟡 Minor
jsEscapeStringmisses U+2028 / U+2029 and other control chars — JS parse failure risk for arbitrary file content.Files can legitimately contain U+2028 (LINE SEPARATOR) and U+2029 (PARAGRAPH SEPARATOR), which are line terminators in JavaScript per ECMA-262 and cause a SyntaxError when unescaped in string literals. The current escape table also omits
\b,\f,\v, and other C0 control characters. SincesetContentandsetDiffContentinterpolate directly into JavaScript string literals, a file containing U+2028/U+2029 will break the expression.Use the existing
JSONSerializationpattern (already used in AppDelegate.swift and TerminalController.swift) with manual U+2028/U+2029 escaping, since JSON allows these literally (RFC 8259) but JavaScript source parsing does not:Proposed replacement
- private static func jsEscapeString(_ str: String) -> String { - var result = "" - result.reserveCapacity(str.count + str.count / 10) - for ch in str { - switch ch { - case "\\": result += "\\\\" - case "\"": result += "\\\"" - case "\n": result += "\\n" - case "\r": result += "\\r" - case "\t": result += "\\t" - default: result.append(ch) - } - } - return result - } + /// Returns a JS string literal (including surrounding quotes) safe to interpolate + /// into evaluateJavaScript expressions. Handles U+2028/U+2029, control chars, etc. + private static func jsLiteral(_ str: String) -> String { + guard let data = try? JSONSerialization.data( + withJSONObject: [str], options: [.fragmentsAllowed] + ) else { return "\"\"" } + // Strip surrounding [ ] from the single-element array encoding. + guard let s = String(data: data, encoding: .utf8), + s.hasPrefix("["), s.hasSuffix("]") else { return "\"\"" } + var literal = String(s.dropFirst().dropLast()) + // JSON allows raw U+2028/U+2029 in strings; JS (pre-ES2019) does not. + literal = literal + .replacingOccurrences(of: "\u{2028}", with: "\\u2028") + .replacingOccurrences(of: "\u{2029}", with: "\\u2029") + return literal + }…and adjust call sites to drop the manual quotes (e.g.
"cmuxEditor.setContent(\(Self.jsLiteral(content)), \(Self.jsLiteral(lang)), \(Self.jsLiteral(filePath)))"or use template literals).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/EditorPanel.swift` around lines 327 - 341, Replace the fragile jsEscapeString implementation with a JSON-based string literal generator: serialize the Swift String using JSONSerialization (or JSONEncoder) to get a properly escaped JSON string, then post-process that JSON output to additionally replace U+2028 and U+2029 with the explicit escapes "\\u2028" and "\\u2029" (and ensure any remaining C0 controls are preserved/escaped by the JSON serializer); keep the helper named jsEscapeString (or introduce jsLiteral) so call sites find it, and update call sites like setContent and setDiffContent to stop wrapping the result in extra manual quotes (use the JSON-produced literal directly in the JS interpolation).
🧹 Nitpick comments (11)
Resources/editor-monaco/index.html (1)
1-30: LGTM — minimal Monaco bootstrap. The loader and bridge scripts are loaded in document order soeditor-bridge.jscan rely onrequire/AMD being available.Optional nit:
#loading'scolor:#888`` and the white default<body>background will flash briefly against the editor's eventual theme on dark mode. If the loading flash becomes noticeable, consider `prefers-color-scheme` styling (or letting Monaco fade in over a transparent background). Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/editor-monaco/index.html` around lines 1 - 30, The loading splash (`#loading`) can flash against the editor's dark theme; add a small CSS tweak to respect system color scheme or make the spinner background transparent so the editor can fade in smoothly: update the stylesheet where `#loading`, body, and `#editor-container` are defined to include a prefers-color-scheme media query (or set body/background transparent) so `#loading` uses a darker text color on dark mode and/or no white background, ensuring the existing script order (vs/loader.js and editor-bridge.js) remains unchanged.Resources/editor-monaco/vs/basic-languages/mips/mips.js (1)
1-11: Vendored Monaco language module — skipping detailed review.Verbatim third-party minified asset (
monaco-editorv0.52.2, MIT). No actionable review.One cross-cutting note for the PR overall (not specific to this file): consider documenting the Monaco version (
0.52.2 / 404545bded1df6ffa41ea0af4e8ddb219018c6c1) and the vendor/sync procedure in aResources/editor-monaco/README.mdorVERSIONfile so future upgrades and license attribution are traceable. This avoids drift between the dozens of bundled language modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/editor-monaco/vs/basic-languages/mips/mips.js` around lines 1 - 11, This vendored minified Monaco language module (notice the version string "0.52.2(404545bded1df6ffa41ea0af4e8ddb219018c6c1)" inside mips.js) needs an accompanying vendor metadata file documenting the upstream monaco-editor version, commit hash, license, and the exact sync/update procedure; add a small README or VERSION file in the same vendor area that states the monaco-editor version/commit, license attribution (MIT), the source URL, and step-by-step instructions used to pull/update language modules so future maintainers can reproduce upgrades and track provenance.CLI/cmux.swift (2)
3203-3204:editor diffdoes not expose--base/--base-label.
v2EditorDiffinSources/TerminalController.swiftaccepts bothbase(explicit base content) andbase_label(the label printed back asdiff_base), but the CLI never forwards them — the user can only diff againstHEADwith the literal labelHEAD. Consider adding--base-label <label>(and optionally--base-file <path>to read base content from a file) so the CLI surfaces the full handler capability. Not blocking — fine as a follow-up if scope is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 3203 - 3204, The CLI currently always calls sendV2 with params that lack the base/base_label fields, so extend the CMUX/command handling that sets method and params (the area creating `let method = subcommand == "diff" ? "editor.diff" : "editor.open"` and `let payload = try client.sendV2(method: method, params: params)`) to parse new flags `--base-label <label>` and optionally `--base-file <path>` (or `--base <content>` if preferred), read file content when `--base-file` is provided, and inject `base` (string content) and `base_label` (string label) into the params dictionary before calling `client.sendV2`; ensure the flag names map to the v2EditorDiff handler fields and preserve existing behavior when flags are omitted (defaulting to HEAD/label "HEAD").
3169-3171: Consider consolidating the path-heuristic logic intolooksLikePath.The check
looksLikePath(first) || first.contains(".")appears in both theeditorcommand (line 3169) andmarkdowncommand (line 3082) with identical logic. SincelooksLikePathexplicitly handles prefixes and slashes but deliberately omits dots, thefirst.contains(".")fallback was added to catch file extensions like.txtand.json. However, this creates two issues:
- Duplication: The same OR pattern is repeated across multiple command handlers.
- Broad matching:
first.contains(".")also matches unrelated tokens likev1.0ora.bthat may not be file paths.Extend
looksLikePathto include the dot check (e.g.,if arg.contains(".") { return true }), then replace both occurrences with justlooksLikePath(first). This centralizes the heuristic logic and makes its intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 3169 - 3171, Extend the path heuristic by modifying the looksLikePath(_:) function to return true when the argument contains a dot (e.g., add an early check like if arg.contains(".") { return true }), then replace the duplicated OR pattern in both command handlers (the editor branch that currently does `if let first = args.first, looksLikePath(first) || first.contains(".")` and the markdown branch with the same logic) with a single `looksLikePath(first)` check; this centralizes the heuristic and removes the duplicated `first.contains(".")` fallback.Sources/FileExplorerView.swift (1)
13-13: Double-click is a silent no-op whenonOpenFileis nil.
onOpenFiledefaults tonil, so any caller that omits it (or constructsFileExplorerPanelViewfrom a path that doesn't wire it up) will havedoubleActionplumbed but no behavior — including no fallback to the same.fileExplorerOpenInCodeViewernotification used by the context-menu path. Consider posting the notification as a fallback so double-click and "Open in Code Viewer" stay consistent regardless of caller wiring.🔧 Suggested fallback
- onOpenFile?(node.path) + if let onOpenFile { + onOpenFile(node.path) + } else { + NotificationCenter.default.post( + name: .fileExplorerOpenInCodeViewer, + object: nil, + userInfo: ["path": node.path] + ) + }Also applies to: 212-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/FileExplorerView.swift` at line 13, The double-click handler currently calls the optional closure onOpenFile and silently does nothing when it's nil; modify the double-action logic in FileExplorerView/FileExplorerPanelView so that if onOpenFile is nil it posts the existing .fileExplorerOpenInCodeViewer Notification as a fallback (use the same payload the context-menu "Open in Code Viewer" path uses), and apply the same fallback behavior where similar optional open handlers are used (see the doubleAction handler and the code around the onOpenFile declaration and the block handling rows 212-218) to keep double-click and context-menu behavior consistent.Resources/editor-monaco/editor-bridge.js (1)
105-143: Editor creation options and selection handler are duplicated.The full options dictionary at lines 105-134 and the
onDidChangeCursorSelectionwiring at lines 135-143 mirror lines 43-76 verbatim. Drift between the two will silently produce inconsistent behavior between "first open" and "diff→content transition". Consider extracting both into helpers (e.g.createNormalEditor(container, value, language)andattachSelectionTracking(editorInstance)), and reuse them frominitMonacoandsetContent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/editor-monaco/editor-bridge.js` around lines 105 - 143, The editor options object and the selection-change wiring are duplicated; extract the options into a single helper (e.g., createEditorOptions or createNormalEditor(container, value, language)) and move the selection handler into a reusable function (e.g., attachSelectionTracking(editorInstance) which subscribes to editor.onDidChangeCursorSelection and posts the same selectionChanged message). Replace the duplicated blocks in initMonaco and setContent to call these helpers (use detectTheme(), the same font settings and scrollbar options, and the existing postToNative payload) so both code paths share one source of truth and avoid drift.Sources/CmuxConfig.swift (1)
2112-2129: Default-resolved-buttons fallback duplicatesCmuxSurfaceTabBarButton.defaults.The
try?on Line 2113 silently swallows any throw fromresolved(...), falling through to a hardcoded literal list at Lines 2116-2120. That list now must be kept in lockstep withCmuxSurfaceTabBarButton.defaults(Lines 1087-1093) — adding.newEditorcorrectly here only works because someone remembered both spots. The next person adding a built-in is unlikely to.Two cleaner options:
- Replace the literal fallback with an unconditional default builder that can't throw (since builtin resolution doesn't actually need the actions map):
♻️ Proposed refactor
- let defaultResolvedButtons = (try? CmuxSurfaceTabBarButton.defaults.map { - try $0.resolved(actions: resolvedActionLookup, codingPath: []) - }) ?? [ - .builtIn(.newTerminal), - .builtIn(.newBrowser), - .builtIn(.newEditor), - .builtIn(.splitRight), - .builtIn(.splitDown) - ] + let defaultResolvedButtons: [CmuxSurfaceTabBarButton] = { + do { + return try CmuxSurfaceTabBarButton.defaults.map { + try $0.resolved(actions: resolvedActionLookup, codingPath: []) + } + } catch { + NSLog("[CmuxConfig] failed to resolve default surface tab bar buttons: %@", String(describing: error)) + return CmuxSurfaceTabBarBuiltInAction.allCases.map { .builtIn($0) } + } + }()This way the fallback derives from
allCasesinstead of a literal list, and any resolution error gets a log line instead of being lost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 2112 - 2129, The fallback for defaultResolvedButtons duplicates CmuxSurfaceTabBarButton.defaults and swallows errors; replace the hardcoded literal array with a non-throwing construction derived from CmuxSurfaceTabBarButton.defaults (e.g. map defaults to resolved variants using a safe/never-throwing path or per-item try? and filter/fallback) so you don't need to keep two lists in sync; update the defaultResolvedButtons assignment used by resolvedSurfaceTabBarButtons to build ResolvedSurfaceTabBarButtons from CmuxSurfaceTabBarButton.defaults and surface any per-item resolution errors to the logger instead of silently falling through.Resources/editor-monaco/vs/loader.js (1)
1-11: Vendored upstream asset — skip line-level review.This is the upstream Microsoft Monaco AMD loader (
v0.52.2, minified, MIT). Reviewing it line-by-line isn't useful, but committing minified vendor code raises supply-chain/maintenance concerns:
- There's no pinned source-of-truth (e.g.,
package.jsonentry, vendor script, orMONACO_VERSIONfile) recording where these bytes came from or how to refresh them.- Future Monaco upgrades will be hard to diff against the previous drop.
- Source maps reference
../../min-maps/vs/loader.js.map, which presumably isn't bundled — fine for prod, but worth confirming it's intentional.Consider adding a short
Resources/editor-monaco/VENDOR.md(or ascripts/update-monaco.sh) documenting the upstream version, commit hash, and the npmmonaco-editor@0.52.2extraction steps so refreshes are reproducible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/editor-monaco/vs/loader.js` around lines 1 - 11, The committed minified Monaco AMD loader is an upstream vendored asset with no recorded provenance; add a short vendoring artifact (e.g., Resources/editor-monaco/VENDOR.md and/or a scripts/update-monaco.sh) that pins the upstream package/version/commit (reference the embedded "Version: 0.52.2(404545bded1df6ffa41ea0af4e8ddb219018c6c1)" string and the sourceMappingURL comment) and documents the exact extraction steps (npm package name/tag like monaco-editor@0.52.2, commands used, and any file transforms), update project metadata (add a MONACO_VERSION or package.json entry) so future diffs are reproducible, and confirm whether the referenced source map ../../min-maps/vs/loader.js.map should be bundled or intentionally omitted and document that decision in the VENDOR.md.Sources/Panels/EditorPanelView.swift (2)
89-113: Workspace mode never triggersonRequestPanelFocus.
workspaceEditorViewdoesn't callonRequestPanelFocusanywhere — clicking on the file explorer side, the placeholder, or the file-unavailable view won't bring the editor pane into focus. OnlymonacoEditorView(when nested insideworkspaceEditorView) carries theonTapGesture. Consider attaching a.contentShape(Rectangle()).onTapGesture { onRequestPanelFocus() }to the outerHStack, or routing focus from the file explorer / placeholder containers, so workspace-mode panels can be focused without first navigating to a file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/EditorPanelView.swift` around lines 89 - 113, workspaceEditorView never invokes onRequestPanelFocus, so clicks on the FileExplorerPanelView, workspacePlaceholderView, or fileUnavailableView don't focus the panel; update workspaceEditorView to forward focus by adding a tap handler to the outer container (e.g., attach .contentShape(Rectangle()).onTapGesture { onRequestPanelFocus() } to the HStack) or alternatively ensure the child containers (FileExplorerPanelView, workspacePlaceholderView, fileUnavailableView) call onRequestPanelFocus when tapped so the focus behavior matches monacoEditorView.
34-39: UpdateonChangeto use the non‑deprecated two‑parameter overload.The single-parameter closure form of
onChange(of:perform:)is deprecated in macOS 14.0. Since this project targets macOS 14.0, use the two-parameter(oldValue, newValue)signature instead.♻️ Proposed change
- .onChange(of: panel.focusFlashToken) { _ in - triggerFocusFlashAnimation() - } - .onChange(of: colorScheme) { newScheme in - panel.setTheme(isDark: newScheme == .dark) - } + .onChange(of: panel.focusFlashToken) { _, _ in + triggerFocusFlashAnimation() + } + .onChange(of: colorScheme) { _, newScheme in + panel.setTheme(isDark: newScheme == .dark) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/EditorPanelView.swift` around lines 34 - 39, Update the deprecated single-parameter .onChange calls to the macOS 14 two-parameter overload: for the focusFlashToken observer, change .onChange(of: panel.focusFlashToken) { _ in triggerFocusFlashAnimation() } to the two-arg form and ignore oldValue if unused but call triggerFocusFlashAnimation() with the newValue context as needed; for the colorScheme observer, change .onChange(of: colorScheme) { newScheme in panel.setTheme(isDark: newScheme == .dark) } to the two-parameter signature and use the newValue (or compare oldValue and newValue) to call panel.setTheme(isDark: newValue == .dark). Ensure you update the closures for the symbols panel.focusFlashToken, triggerFocusFlashAnimation(), colorScheme, and panel.setTheme(isDark:) to the (oldValue, newValue) parameter list.Sources/Panels/EditorPanel.swift (1)
365-403: File watch event handler hops throughDispatchQueue.main.asyncto reach MainActor state.The DispatchSource handler runs on
watchQueueand then dispatches toDispatchQueue.main.asyncto invoke MainActor-isolated methods (stopFileWatcher,loadFileContent,startFileWatcher,scheduleReattach). This works in Swift 5 mode but Swift 6 strict concurrency will flag the implicit MainActor crossing. PreferTask {@mainactorin … }(orMainActor.assumeIsolatedfrom the queue) so the isolation is explicit and the file remains forward‑compatible with strict concurrency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/EditorPanel.swift` around lines 365 - 403, The event handler currently uses DispatchQueue.main.async to call MainActor-isolated methods, which will break under Swift 6 strict concurrency; replace both DispatchQueue.main.async { ... } blocks inside source.setEventHandler with Task { `@MainActor` in ... } so the calls to stopFileWatcher(), loadFileContent(), startFileWatcher(), and scheduleReattach(attempt:) are executed with explicit MainActor isolation; keep the existing [weak self] capture and the guard let self unwrap, and make the same swap for both the delete/rename branch and the else branch so fileWatchSource, fileDescriptor handling, and cancel behavior remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/editor-monaco/editor-bridge.js`:
- Around line 99-150: The diff teardown leaks Monaco models: when switching from
diff mode in setContent you dispose diffEditor but do not dispose the
originalModel/modifiedModel created in setDiffContent; update the teardown logic
(in the block handling isDiffMode and in the start of setDiffContent) to track
and explicitly dispose originalModel and modifiedModel alongside diffEditor
(clear their references after dispose) so both models are disposed before
creating new ones or leaving diff mode.
- Around line 154-197: setDiffContent currently only disposes the normal editor
and never cleans up an existing diffEditor or its models, causing leaks; before
wiping the DOM or creating a new diff editor, check if diffEditor exists and
properly dispose it and its models: call diffEditor.getModel() to retrieve
original/modified models and dispose each (originalModel.dispose(),
modifiedModel.dispose()), then call diffEditor.dispose() and set diffEditor =
null (and clear isDiffMode appropriately), then proceed to clear
container.innerHTML and create the new models and diffEditor; update
setDiffContent to perform this cleanup at its start to prevent leaking
originalModel/modifiedModel and the previous diffEditor.
In `@Sources/AppDelegate.swift`:
- Around line 6271-6277: handleFileExplorerOpenInCodeViewer currently returns
early when workspace.focusedPanelId is nil which drops valid file-open requests;
instead call a fallback that opens the file via the workspace-level API
(Workspace.openEditor(filePath:)) when focusedPanelId is nil, and change
Workspace.openEditor(filePath:) to select a pane id by using
bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first, and
ensure it reuses an existing editor if one for the same filePath is already open
rather than creating a duplicate.
In `@Sources/FileExplorerView.swift`:
- Around line 249-256: Add the missing localization entry for the key used in
String(localized: "fileExplorer.contextMenu.openInCodeViewer", defaultValue:
"Open in Code Viewer") (triggered by the contextMenuOpenInCodeViewer(_: )
NSMenuItem) to Resources/Localizable.xcstrings: add an English entry "Open in
Code Viewer" and the corresponding Japanese translation following the project's
.xcstrings formatting and conventions, matching existing key/value syntax for
both locales so the String(localized: ...) call resolves correctly at runtime.
- Around line 212-218: The double-click handler openSelectedFileFromOutline
currently returns early for directories, breaking NSOutlineView's expected
expand/collapse behavior; update openSelectedFileFromOutline (in
FileExplorerView) so when the clicked/selected node is a directory it toggles
expansion instead of returning: use the outline view methods isItemExpanded(_:)
and expandItem(_:expandChildren:) / collapseItem(_:) (or expandItem(_) and
collapseItem(_)) on the sender to expand or collapse the directory node, and
only call onOpenFile?(node.path) for non-directory nodes.
- Around line 296-303: The context menu action contextMenuOpenInCodeViewer posts
.fileExplorerOpenInCodeViewer globally with only "path", which causes
handleFileExplorerOpenInCodeViewer to open files in the currently active
tabManager.selectedTab instead of the originating window; modify
contextMenuOpenInCodeViewer to include a deterministic source (e.g., add
"windowId" or the FileExplorerStore/FileExplorerState reference) in the
Notification userInfo, and update handleFileExplorerOpenInCodeViewer to read
that windowId/store and route the open request to that window's workspace/tab
manager (or alternatively make the file-explorer observe events on its
per-window FileExplorerState instead of the global NotificationCenter) so the
file opens in the originating window.
In `@Sources/Panels/EditorPanel.swift`:
- Around line 202-219: createWebView currently always creates a new CmuxWebView
and assigns it to self.webView, which orphanes Monaco state; change
createWebView to be idempotent by returning the existing self.webView if it
already exists and is valid (do not recreate or reassign), only constructing a
new CmuxWebView (and adding the EditorBridgeMessageHandler to
config.userContentController) when there is no current webView or it has been
explicitly torn down, and when you do recreate it ensure you reset isMonacoReady
(or equivalent readiness flag) at that moment so readiness is cleared only on
actual rebuilds; update any logic that adds the bridge handler to avoid
double-adding the same handler for the existing webView.
In `@Sources/Panels/EditorPanelView.swift`:
- Around line 201-219: makeNSView currently recreates the Monaco WKWebView every
time it's mounted by unconditionally calling panel.createWebView() and
panel.loadMonacoPage(), which discards state; change makeNSView to reuse an
existing panel.webView if present (use it as the webView to add to the
container) and only call panel.createWebView() and panel.loadMonacoPage() when
panel.webView is nil (i.e., a fresh creation); also update createWebView() to
short‑circuit and return early when webView != nil so it doesn't overwrite an
existing instance, and ensure subsequent uses respect panel.isMonacoReady (don't
assume readiness until the new web page posts its ready event).
In `@Sources/TerminalController.swift`:
- Around line 7928-7935: The bug is that filePath.hasPrefix(repoRoot) can match
sibling directories; update the check around repoRoot/filePath (the guard that
computes relativePath) to require an exact match or a path-component boundary
(e.g. filePath == repoRoot or filePath.hasPrefix(repoRoot + "/") or compare
pathComponents) before computing relativePath, then compute relativePath from
the confirmed boundary; optionally normalize both repoRoot and filePath with
resolvingSymlinksInPath if you want symlink-robustness (apply this change where
repoRoot, filePath, relativePath, and the hasPrefix check are used).
- Around line 7933-7949: The git read can deadlock because waitUntilExit() is
called before consuming the pipe; change the sequence in the Process block (the
Process instance created in this diff) to read the standardOutput data first
from pipe.fileHandleForReading.readDataToEndOfFile() (or use the asynchronous
read API) and only then call process.waitUntilExit(), then check
process.terminationStatus and convert the data to String to set baseContent;
also preserve the existing error handling for the catch and ensure the
standardError Pipe remains attached so stderr can be captured if needed.
- Around line 8056-8079: Replace the direct casts using params["line"] and
params["column"] with the v2Int helper: call v2Int(params, "line") and
v2Int(params, "column") inside the v2MainSync block (keeping v2ResolveWorkspace,
v2UUID, and editorPanel.goToLine usage), validate that line and, if provided,
column are > 0, and if any are missing/invalid return .err(code:
"invalid_params", ...) instead of falling back to defaults; ensure you still set
result = .ok with "surface_id", "line", and "column" (defaulting column to 1
only when it was not supplied at all).
In `@Sources/Workspace.swift`:
- Around line 545-550: The editor snapshot only stores filePath so editor panels
created in "picker" mode (from workspaceRootDirectory) are lost; update
SessionEditorPanelSnapshot to include an optional workspaceRootDirectory
property and populate it in the .editor serialization path (set
workspaceRootDirectory = editorPanel.workspaceRootDirectory when filePath is
nil) in the block that creates SessionEditorPanelSnapshot and the similar block
around the 788-798 region, and then update the session-restore logic that
currently requires snapshot.editor?.filePath to accept either
snapshot.editor?.filePath or snapshot.editor?.workspaceRootDirectory and
recreate the EditorPanel in picker mode when only workspaceRootDirectory is
present (instead of dropping or mis-restoring the panel).
- Around line 9881-9903: attachDetachedSurface(_:inPane:atIndex:focus:) doesn’t
reinstall the EditorPanel Combine observers, so reattached EditorPanel instances
retain stale workspaceId and stop updating tab titles; update
attachDetachedSurface to handle the EditorPanel branch by calling
editorPanel.updateWorkspaceId(self.id) and then call
installEditorPanelSubscription(editorPanel) (and ensure any previous
subscription in panelSubscriptions[editorPanel.id] is cancelled/replaced) so
panelTitles, panelCustomTitles, and bonsplitController.updateTab calls continue
to reflect displayTitle changes; use surfaceIdFromPanelId(editorPanel.id) to
resolve the tab and maintain the same subscription lifecycle as in
installEditorPanelSubscription(_:).
- Around line 9767-9789: Both helpers drop requests when there's no focused pane
or when newEditorSplit fails; change newEditorSurfaceInFocusedPane to use
bonsplitController.allPaneIds.first as a fallback instead of returning nil (call
newEditorSurface(inPane:workspaceRootDirectory:focus:)), and change
newEditorSplitFromFocusedPanel to, after computing directory and resolving
sourcePanelId (using the same fallback via newEditorSurfaceInFocusedPane),
attempt
newEditorSplit(from:orientation:insertFirst:workspaceRootDirectory:focus:) and
if that returns nil immediately fall back to calling newEditorSurface(inPane:
<paneIdUsed>, workspaceRootDirectory: directory, focus: focus) so file-open
actions are not silently dropped; reference functions:
newEditorSurfaceInFocusedPane, newEditorSplitFromFocusedPanel,
bonsplitController.focusedPaneId, bonsplitController.allPaneIds.first,
newEditorSplit, newEditorSurface.
In `@vendor/bonsplit`:
- Line 1: The submodule pointer in vendor/bonsplit references a non-existent
commit (90a3acf0dbd0d410c100edd5fb68eaca266aa9b8); either push that commit to
the bonsplit remote (main) or update the parent repo to point to a valid commit
hash. Locate the vendor/bonsplit submodule entry and fix it by (a) pushing the
missing commit to the bonsplit remote branch that the submodule tracks, or (b)
changing the submodule pointer to an existing commit (verify with the bonsplit
repo’s refs) and update the parent repo's submodule commit, then run submodule
sync/update so other developers get the correct commit.
---
Minor comments:
In `@CLI/cmux.swift`:
- Around line 3157-3180: The code allows extra positional arguments to be
silently dropped by assigning multi-element arrays to subArgs and only using
subArgs.first; update the parsing after you determine subcommand/subArgs
(symbols: subcommand, subArgs, rawPath) to validate that subArgs.count == 1 (or
else throw a CLIError) so any trailing positional args cause a clear error
message like "editor <subcommand> expects a single path argument; got N
arguments"; ensure this check runs for the branches that set subArgs (the branch
handling ["open","diff"] and the path-shorthand branch that assigns subArgs =
args) before using rawPath, and reuse looksLikePath where needed to detect the
path branch.
In `@Resources/editor-monaco/editor-bridge.js`:
- Around line 105-110: The editor recreation currently always uses theme:
detectTheme(), which ignores any explicit theme set via
cmuxEditor.setTheme(isDark); change the implementation to cache the last theme
choice (initialize a module-level variable from detectTheme() on load), update
that cached value inside the setTheme (or cmuxEditor.setTheme) handler when
Swift forces a theme, and then pass the cached theme value instead of
detectTheme() when calling monaco.editor.create so forced themes persist across
diff→content re-creations.
In `@Resources/editor-monaco/vs/basic-languages/rust/rust.js`:
- Around line 1-3: The Monaco bundle files (e.g.,
Resources/editor-monaco/vs/basic-languages/rust/rust.js which contains the
Version: 0.52.2(404545bded1df6ffa41ea0af4e8ddb219018c6c1) header) are a vendor
drop but lack any upstream provenance or regeneration steps; add a vendor
manifest (e.g., Resources/editor-monaco/VENDOR.md) or update the repository
README to document the upstream source (repo URL and commit/tag), the exact
commit/id shown in the Version header, how the bundle was extracted or built,
and step-by-step regeneration instructions (commands used, any build flags, and
how to validate versions) so future maintainers can verify or regenerate the
bundle.
In `@Sources/Panels/EditorPanel.swift`:
- Around line 436-487: monacoLanguageFromExtension currently only matches on the
extension so extensionless filenames like "Dockerfile", "Makefile",
"CMakeLists.txt", ".gitignore", etc. fall through to "plaintext"; update
monacoLanguageFromExtension (or create a thin wrapper like
monacoLanguageFromPath) to check the basename when the extracted extension is
empty: compute lastPathComponent / basename (lowercased) and compare against
known filenames ("dockerfile", "makefile", "cmakelists.txt", ".gitignore",
"gemfile", etc.) before returning "plaintext", preserving all existing
extension-based cases.
- Around line 327-341: Replace the fragile jsEscapeString implementation with a
JSON-based string literal generator: serialize the Swift String using
JSONSerialization (or JSONEncoder) to get a properly escaped JSON string, then
post-process that JSON output to additionally replace U+2028 and U+2029 with the
explicit escapes "\\u2028" and "\\u2029" (and ensure any remaining C0 controls
are preserved/escaped by the JSON serializer); keep the helper named
jsEscapeString (or introduce jsLiteral) so call sites find it, and update call
sites like setContent and setDiffContent to stop wrapping the result in extra
manual quotes (use the JSON-produced literal directly in the JS interpolation).
In `@Sources/Panels/EditorPanelView.swift`:
- Around line 58-62: EditorPanelView currently uses .onTapGesture which won't
trigger for clicks inside the Monaco editor (CmuxWebView); add a
.onReceive(NotificationCenter.default.publisher(for: .webViewDidReceiveClick))
to EditorPanelView and, when receiving the notification, verify the
notification.object matches the EditorPanel's CmuxWebView instance (same
instance created in EditorPanel) then call onRequestPanelFocus() so clicks
inside the web view focus the panel; reference .webViewDidReceiveClick,
EditorPanelView, CmuxWebView, and onRequestPanelFocus when locating where to add
this listener.
In `@Sources/TerminalController.swift`:
- Around line 7897-7902: The editor.diff validation should mirror v2EditorOpen:
after checking filePath is absolute, replace the current isReadableFile check
with a check that the path exists, is readable, and is NOT a directory (use
FileManager.fileExists(atPath:isDirectory:) to detect directories) and return a
clear error like "not_found" or "invalid_params" if it's a directory;
additionally, before calling openOrFocusEditorSplit(from:) ensure
ws.panels[sourceSurfaceId] != nil and return a specific error (e.g.
"unknown_surface") if the surface_id is missing so an invalid surface doesn't
silently propagate—apply the same fixes to the other occurrence around lines
7963–7967.
---
Nitpick comments:
In `@CLI/cmux.swift`:
- Around line 3203-3204: The CLI currently always calls sendV2 with params that
lack the base/base_label fields, so extend the CMUX/command handling that sets
method and params (the area creating `let method = subcommand == "diff" ?
"editor.diff" : "editor.open"` and `let payload = try client.sendV2(method:
method, params: params)`) to parse new flags `--base-label <label>` and
optionally `--base-file <path>` (or `--base <content>` if preferred), read file
content when `--base-file` is provided, and inject `base` (string content) and
`base_label` (string label) into the params dictionary before calling
`client.sendV2`; ensure the flag names map to the v2EditorDiff handler fields
and preserve existing behavior when flags are omitted (defaulting to HEAD/label
"HEAD").
- Around line 3169-3171: Extend the path heuristic by modifying the
looksLikePath(_:) function to return true when the argument contains a dot
(e.g., add an early check like if arg.contains(".") { return true }), then
replace the duplicated OR pattern in both command handlers (the editor branch
that currently does `if let first = args.first, looksLikePath(first) ||
first.contains(".")` and the markdown branch with the same logic) with a single
`looksLikePath(first)` check; this centralizes the heuristic and removes the
duplicated `first.contains(".")` fallback.
In `@Resources/editor-monaco/editor-bridge.js`:
- Around line 105-143: The editor options object and the selection-change wiring
are duplicated; extract the options into a single helper (e.g.,
createEditorOptions or createNormalEditor(container, value, language)) and move
the selection handler into a reusable function (e.g.,
attachSelectionTracking(editorInstance) which subscribes to
editor.onDidChangeCursorSelection and posts the same selectionChanged message).
Replace the duplicated blocks in initMonaco and setContent to call these helpers
(use detectTheme(), the same font settings and scrollbar options, and the
existing postToNative payload) so both code paths share one source of truth and
avoid drift.
In `@Resources/editor-monaco/index.html`:
- Around line 1-30: The loading splash (`#loading`) can flash against the editor's
dark theme; add a small CSS tweak to respect system color scheme or make the
spinner background transparent so the editor can fade in smoothly: update the
stylesheet where `#loading`, body, and `#editor-container` are defined to include a
prefers-color-scheme media query (or set body/background transparent) so
`#loading` uses a darker text color on dark mode and/or no white background,
ensuring the existing script order (vs/loader.js and editor-bridge.js) remains
unchanged.
In `@Resources/editor-monaco/vs/basic-languages/mips/mips.js`:
- Around line 1-11: This vendored minified Monaco language module (notice the
version string "0.52.2(404545bded1df6ffa41ea0af4e8ddb219018c6c1)" inside
mips.js) needs an accompanying vendor metadata file documenting the upstream
monaco-editor version, commit hash, license, and the exact sync/update
procedure; add a small README or VERSION file in the same vendor area that
states the monaco-editor version/commit, license attribution (MIT), the source
URL, and step-by-step instructions used to pull/update language modules so
future maintainers can reproduce upgrades and track provenance.
In `@Resources/editor-monaco/vs/loader.js`:
- Around line 1-11: The committed minified Monaco AMD loader is an upstream
vendored asset with no recorded provenance; add a short vendoring artifact
(e.g., Resources/editor-monaco/VENDOR.md and/or a scripts/update-monaco.sh) that
pins the upstream package/version/commit (reference the embedded "Version:
0.52.2(404545bded1df6ffa41ea0af4e8ddb219018c6c1)" string and the
sourceMappingURL comment) and documents the exact extraction steps (npm package
name/tag like monaco-editor@0.52.2, commands used, and any file transforms),
update project metadata (add a MONACO_VERSION or package.json entry) so future
diffs are reproducible, and confirm whether the referenced source map
../../min-maps/vs/loader.js.map should be bundled or intentionally omitted and
document that decision in the VENDOR.md.
In `@Sources/CmuxConfig.swift`:
- Around line 2112-2129: The fallback for defaultResolvedButtons duplicates
CmuxSurfaceTabBarButton.defaults and swallows errors; replace the hardcoded
literal array with a non-throwing construction derived from
CmuxSurfaceTabBarButton.defaults (e.g. map defaults to resolved variants using a
safe/never-throwing path or per-item try? and filter/fallback) so you don't need
to keep two lists in sync; update the defaultResolvedButtons assignment used by
resolvedSurfaceTabBarButtons to build ResolvedSurfaceTabBarButtons from
CmuxSurfaceTabBarButton.defaults and surface any per-item resolution errors to
the logger instead of silently falling through.
In `@Sources/FileExplorerView.swift`:
- Line 13: The double-click handler currently calls the optional closure
onOpenFile and silently does nothing when it's nil; modify the double-action
logic in FileExplorerView/FileExplorerPanelView so that if onOpenFile is nil it
posts the existing .fileExplorerOpenInCodeViewer Notification as a fallback (use
the same payload the context-menu "Open in Code Viewer" path uses), and apply
the same fallback behavior where similar optional open handlers are used (see
the doubleAction handler and the code around the onOpenFile declaration and the
block handling rows 212-218) to keep double-click and context-menu behavior
consistent.
In `@Sources/Panels/EditorPanel.swift`:
- Around line 365-403: The event handler currently uses DispatchQueue.main.async
to call MainActor-isolated methods, which will break under Swift 6 strict
concurrency; replace both DispatchQueue.main.async { ... } blocks inside
source.setEventHandler with Task { `@MainActor` in ... } so the calls to
stopFileWatcher(), loadFileContent(), startFileWatcher(), and
scheduleReattach(attempt:) are executed with explicit MainActor isolation; keep
the existing [weak self] capture and the guard let self unwrap, and make the
same swap for both the delete/rename branch and the else branch so
fileWatchSource, fileDescriptor handling, and cancel behavior remain unchanged.
In `@Sources/Panels/EditorPanelView.swift`:
- Around line 89-113: workspaceEditorView never invokes onRequestPanelFocus, so
clicks on the FileExplorerPanelView, workspacePlaceholderView, or
fileUnavailableView don't focus the panel; update workspaceEditorView to forward
focus by adding a tap handler to the outer container (e.g., attach
.contentShape(Rectangle()).onTapGesture { onRequestPanelFocus() } to the HStack)
or alternatively ensure the child containers (FileExplorerPanelView,
workspacePlaceholderView, fileUnavailableView) call onRequestPanelFocus when
tapped so the focus behavior matches monacoEditorView.
- Around line 34-39: Update the deprecated single-parameter .onChange calls to
the macOS 14 two-parameter overload: for the focusFlashToken observer, change
.onChange(of: panel.focusFlashToken) { _ in triggerFocusFlashAnimation() } to
the two-arg form and ignore oldValue if unused but call
triggerFocusFlashAnimation() with the newValue context as needed; for the
colorScheme observer, change .onChange(of: colorScheme) { newScheme in
panel.setTheme(isDark: newScheme == .dark) } to the two-parameter signature and
use the newValue (or compare oldValue and newValue) to call
panel.setTheme(isDark: newValue == .dark). Ensure you update the closures for
the symbols panel.focusFlashToken, triggerFocusFlashAnimation(), colorScheme,
and panel.setTheme(isDark:) to the (oldValue, newValue) parameter list.
🪄 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: 5721214d-4f1b-4293-936c-97926690bdfc
⛔ Files ignored due to path filters (1)
Resources/editor-monaco/vs/base/browser/ui/codicons/codicon/codicon.ttfis excluded by!**/*.ttf
📒 Files selected for processing (110)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojResources/editor-monaco/editor-bridge.jsResources/editor-monaco/index.htmlResources/editor-monaco/vs/base/worker/workerMain.jsResources/editor-monaco/vs/basic-languages/abap/abap.jsResources/editor-monaco/vs/basic-languages/apex/apex.jsResources/editor-monaco/vs/basic-languages/azcli/azcli.jsResources/editor-monaco/vs/basic-languages/bat/bat.jsResources/editor-monaco/vs/basic-languages/bicep/bicep.jsResources/editor-monaco/vs/basic-languages/cameligo/cameligo.jsResources/editor-monaco/vs/basic-languages/clojure/clojure.jsResources/editor-monaco/vs/basic-languages/coffee/coffee.jsResources/editor-monaco/vs/basic-languages/cpp/cpp.jsResources/editor-monaco/vs/basic-languages/csharp/csharp.jsResources/editor-monaco/vs/basic-languages/csp/csp.jsResources/editor-monaco/vs/basic-languages/css/css.jsResources/editor-monaco/vs/basic-languages/cypher/cypher.jsResources/editor-monaco/vs/basic-languages/dart/dart.jsResources/editor-monaco/vs/basic-languages/dockerfile/dockerfile.jsResources/editor-monaco/vs/basic-languages/ecl/ecl.jsResources/editor-monaco/vs/basic-languages/elixir/elixir.jsResources/editor-monaco/vs/basic-languages/flow9/flow9.jsResources/editor-monaco/vs/basic-languages/freemarker2/freemarker2.jsResources/editor-monaco/vs/basic-languages/fsharp/fsharp.jsResources/editor-monaco/vs/basic-languages/go/go.jsResources/editor-monaco/vs/basic-languages/graphql/graphql.jsResources/editor-monaco/vs/basic-languages/handlebars/handlebars.jsResources/editor-monaco/vs/basic-languages/hcl/hcl.jsResources/editor-monaco/vs/basic-languages/html/html.jsResources/editor-monaco/vs/basic-languages/ini/ini.jsResources/editor-monaco/vs/basic-languages/java/java.jsResources/editor-monaco/vs/basic-languages/javascript/javascript.jsResources/editor-monaco/vs/basic-languages/julia/julia.jsResources/editor-monaco/vs/basic-languages/kotlin/kotlin.jsResources/editor-monaco/vs/basic-languages/less/less.jsResources/editor-monaco/vs/basic-languages/lexon/lexon.jsResources/editor-monaco/vs/basic-languages/liquid/liquid.jsResources/editor-monaco/vs/basic-languages/lua/lua.jsResources/editor-monaco/vs/basic-languages/m3/m3.jsResources/editor-monaco/vs/basic-languages/markdown/markdown.jsResources/editor-monaco/vs/basic-languages/mdx/mdx.jsResources/editor-monaco/vs/basic-languages/mips/mips.jsResources/editor-monaco/vs/basic-languages/msdax/msdax.jsResources/editor-monaco/vs/basic-languages/mysql/mysql.jsResources/editor-monaco/vs/basic-languages/objective-c/objective-c.jsResources/editor-monaco/vs/basic-languages/pascal/pascal.jsResources/editor-monaco/vs/basic-languages/pascaligo/pascaligo.jsResources/editor-monaco/vs/basic-languages/perl/perl.jsResources/editor-monaco/vs/basic-languages/pgsql/pgsql.jsResources/editor-monaco/vs/basic-languages/php/php.jsResources/editor-monaco/vs/basic-languages/pla/pla.jsResources/editor-monaco/vs/basic-languages/postiats/postiats.jsResources/editor-monaco/vs/basic-languages/powerquery/powerquery.jsResources/editor-monaco/vs/basic-languages/powershell/powershell.jsResources/editor-monaco/vs/basic-languages/protobuf/protobuf.jsResources/editor-monaco/vs/basic-languages/pug/pug.jsResources/editor-monaco/vs/basic-languages/python/python.jsResources/editor-monaco/vs/basic-languages/qsharp/qsharp.jsResources/editor-monaco/vs/basic-languages/r/r.jsResources/editor-monaco/vs/basic-languages/razor/razor.jsResources/editor-monaco/vs/basic-languages/redis/redis.jsResources/editor-monaco/vs/basic-languages/redshift/redshift.jsResources/editor-monaco/vs/basic-languages/restructuredtext/restructuredtext.jsResources/editor-monaco/vs/basic-languages/ruby/ruby.jsResources/editor-monaco/vs/basic-languages/rust/rust.jsResources/editor-monaco/vs/basic-languages/sb/sb.jsResources/editor-monaco/vs/basic-languages/scala/scala.jsResources/editor-monaco/vs/basic-languages/scheme/scheme.jsResources/editor-monaco/vs/basic-languages/scss/scss.jsResources/editor-monaco/vs/basic-languages/shell/shell.jsResources/editor-monaco/vs/basic-languages/solidity/solidity.jsResources/editor-monaco/vs/basic-languages/sophia/sophia.jsResources/editor-monaco/vs/basic-languages/sparql/sparql.jsResources/editor-monaco/vs/basic-languages/sql/sql.jsResources/editor-monaco/vs/basic-languages/st/st.jsResources/editor-monaco/vs/basic-languages/swift/swift.jsResources/editor-monaco/vs/basic-languages/systemverilog/systemverilog.jsResources/editor-monaco/vs/basic-languages/tcl/tcl.jsResources/editor-monaco/vs/basic-languages/twig/twig.jsResources/editor-monaco/vs/basic-languages/typescript/typescript.jsResources/editor-monaco/vs/basic-languages/typespec/typespec.jsResources/editor-monaco/vs/basic-languages/vb/vb.jsResources/editor-monaco/vs/basic-languages/wgsl/wgsl.jsResources/editor-monaco/vs/basic-languages/xml/xml.jsResources/editor-monaco/vs/basic-languages/yaml/yaml.jsResources/editor-monaco/vs/editor/editor.main.cssResources/editor-monaco/vs/editor/editor.main.jsResources/editor-monaco/vs/loader.jsResources/editor-monaco/vs/nls.messages.de.jsResources/editor-monaco/vs/nls.messages.es.jsResources/editor-monaco/vs/nls.messages.fr.jsResources/editor-monaco/vs/nls.messages.it.jsResources/editor-monaco/vs/nls.messages.ja.jsResources/editor-monaco/vs/nls.messages.ko.jsResources/editor-monaco/vs/nls.messages.ru.jsResources/editor-monaco/vs/nls.messages.zh-cn.jsResources/editor-monaco/vs/nls.messages.zh-tw.jsSources/AppDelegate.swiftSources/CmuxConfig.swiftSources/ContentView.swiftSources/FileExplorerView.swiftSources/Panels/EditorPanel.swiftSources/Panels/EditorPanelView.swiftSources/Panels/Panel.swiftSources/Panels/PanelContentView.swiftSources/SessionPersistence.swiftSources/TerminalController.swiftSources/Workspace.swiftvendor/bonsplit
Comment on lines
+249
to
+256
| let openInViewerItem = NSMenuItem( | ||
| title: String(localized: "fileExplorer.contextMenu.openInCodeViewer", defaultValue: "Open in Code Viewer"), | ||
| action: #selector(contextMenuOpenInCodeViewer(_:)), | ||
| keyEquivalent: "" | ||
| ) | ||
| openInViewerItem.target = self | ||
| openInViewerItem.representedObject = node | ||
| menu.addItem(openInViewerItem) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the new key has English + Japanese translations in the xcstrings catalog.
KEY="fileExplorer.contextMenu.openInCodeViewer"
python3 - <<PY
import json, sys
p = "Resources/Localizable.xcstrings"
with open(p) as f:
data = json.load(f)
entry = data.get("strings", {}).get("$KEY".replace("\$",""))
key = "$KEY"
entry = data.get("strings", {}).get(key)
if entry is None:
print(f"MISSING: key '{key}' is not present in {p}")
sys.exit(1)
locs = entry.get("localizations", {}) or {}
print(f"key='{key}' locales={sorted(locs.keys())}")
for need in ("en", "ja"):
if need not in locs:
print(f"MISSING locale: {need}")
PYRepository: manaflow-ai/cmux
Length of output: 167
Add the missing localization key to Resources/Localizable.xcstrings.
The code uses String(localized: "fileExplorer.contextMenu.openInCodeViewer", defaultValue: "Open in Code Viewer") correctly, but the key is entirely absent from Resources/Localizable.xcstrings. Add an entry for this key with translations for both English ("Open in Code Viewer") and Japanese per project conventions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/FileExplorerView.swift` around lines 249 - 256, Add the missing
localization entry for the key used in String(localized:
"fileExplorer.contextMenu.openInCodeViewer", defaultValue: "Open in Code
Viewer") (triggered by the contextMenuOpenInCodeViewer(_: ) NSMenuItem) to
Resources/Localizable.xcstrings: add an English entry "Open in Code Viewer" and
the corresponding Japanese translation following the project's .xcstrings
formatting and conventions, matching existing key/value syntax for both locales
so the String(localized: ...) call resolves correctly at runtime.
Comment on lines
+8056
to
+8079
| guard let line = params["line"] as? Int, line > 0 else { | ||
| return .err(code: "invalid_params", message: "Missing or invalid 'line' parameter (must be positive integer)", data: nil) | ||
| } | ||
| let column = params["column"] as? Int | ||
|
|
||
| var result: V2CallResult = .err(code: "internal_error", message: "Failed to goto", data: nil) | ||
| v2MainSync { | ||
| guard let ws = v2ResolveWorkspace(params: params, tabManager: tabManager) else { | ||
| result = .err(code: "not_found", message: "Workspace not found", data: nil) | ||
| return | ||
| } | ||
|
|
||
| let surfaceId = v2UUID(params, "surface_id") ?? ws.focusedPanelId | ||
| guard let surfaceId, let editorPanel = ws.editorPanel(for: surfaceId) else { | ||
| result = .err(code: "not_found", message: "Editor panel not found", data: nil) | ||
| return | ||
| } | ||
|
|
||
| editorPanel.goToLine(line, column: column) | ||
|
|
||
| result = .ok([ | ||
| "surface_id": editorPanel.id.uuidString, | ||
| "line": line, | ||
| "column": column ?? 1, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm v2Int helper exists and check how other handlers parse numeric params.
rg -nP --type=swift -C2 '\bfunc\s+v2Int\b|\bfunc\s+v2StrictInt\b'
rg -nP --type=swift -C1 'params\["[a-z_]+"\]\s+as\?\s+Int\b'Repository: manaflow-ai/cmux
Length of output: 1523
🏁 Script executed:
#!/bin/bash
# Read the full function context
sed -n '8050,8080p' Sources/TerminalController.swift
# Check the goToLine method signature
rg -nP --type=swift -A5 'func goToLine\(' Sources/Repository: manaflow-ai/cmux
Length of output: 1885
Use v2Int for numeric params and validate column when provided.
params["line"] as? Int and params["column"] as? Int will fail for inputs that JSONSerialization decodes as NSNumber/Double. Per repo convention, use the v2Int helper instead. Additionally, when column is supplied, validate that it is a positive integer (> 0); if invalid, return invalid_params rather than silently falling back to the default.
🛠️ Proposed fix
- guard let line = params["line"] as? Int, line > 0 else {
+ guard let line = v2Int(params, "line"), line > 0 else {
return .err(code: "invalid_params", message: "Missing or invalid 'line' parameter (must be positive integer)", data: nil)
}
- let column = params["column"] as? Int
+ let column = v2Int(params, "column")
+ if let column, column <= 0 {
+ return .err(code: "invalid_params", message: "Invalid 'column' parameter (must be positive integer)", data: nil)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 8056 - 8079, Replace the
direct casts using params["line"] and params["column"] with the v2Int helper:
call v2Int(params, "line") and v2Int(params, "column") inside the v2MainSync
block (keeping v2ResolveWorkspace, v2UUID, and editorPanel.goToLine usage),
validate that line and, if provided, column are > 0, and if any are
missing/invalid return .err(code: "invalid_params", ...) instead of falling back
to defaults; ensure you still set result = .ok with "surface_id", "line", and
"column" (defaulting column to 1 only when it was not supplied at all).
| @@ -1 +1 @@ | |||
| Subproject commit fc54dcc3735741279274169f8c30a7897e5d55d3 | |||
| Subproject commit 90a3acf0dbd0d410c100edd5fb68eaca266aa9b8 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the bonsplit submodule commit is on remote main branch.
cd vendor/bonsplit || { echo "Failed to cd into vendor/bonsplit"; exit 1; }
# Fetch latest from remote to ensure we have up-to-date refs
git fetch origin main 2>&1
# Check if the commit exists in the repository
if ! git cat-file -e 90a3acf0dbd0d410c100edd5fb68eaca266aa9b8^{commit} 2>/dev/null; then
echo "❌ FAIL: Commit 90a3acf0dbd0d410c100edd5fb68eaca266aa9b8 does not exist in the submodule"
exit 1
fi
# Check if the commit is an ancestor of origin/main (i.e., merged to main)
if git merge-base --is-ancestor 90a3acf0dbd0d410c100edd5fb68eaca266aa9b8 origin/main 2>/dev/null; then
echo "✅ PASS: Commit 90a3acf0dbd0d410c100edd5fb68eaca266aa9b8 is on remote main branch"
else
echo "❌ FAIL: Commit 90a3acf0dbd0d410c100edd5fb68eaca266aa9b8 is NOT on remote main branch (orphaned/detached)"
echo "Push the submodule commit to origin/main before updating the pointer in the parent repo"
exit 1
fiRepository: manaflow-ai/cmux
Length of output: 235
Push the submodule commit to vendor/bonsplit remote or verify the commit hash.
Commit 90a3acf0dbd0d410c100edd5fb68eaca266aa9b8 does not exist in the vendor/bonsplit repository. Updating the parent repo's submodule pointer to a non-existent commit will break the build for all developers. Either push this commit to the vendor/bonsplit remote main branch, or verify that the correct commit hash is being used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/bonsplit` at line 1, The submodule pointer in vendor/bonsplit
references a non-existent commit (90a3acf0dbd0d410c100edd5fb68eaca266aa9b8);
either push that commit to the bonsplit remote (main) or update the parent repo
to point to a valid commit hash. Locate the vendor/bonsplit submodule entry and
fix it by (a) pushing the missing commit to the bonsplit remote branch that the
submodule tracks, or (b) changing the submodule pointer to an existing commit
(verify with the bonsplit repo’s refs) and update the parent repo's submodule
commit, then run submodule sync/update so other developers get the correct
commit.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/GhosttyTerminalView.swift (1)
8411-8425:⚠️ Potential issue | 🟠 MajorRespect the markdown cmd-click route before the generic editor route.
This new early return makes
CmdClickMarkdownRouteSettings.shouldRoute(path:)unreachable for local markdown paths whenever the editor opens successfully, so bareREADME.mdclicks now ignore the existing markdown-preview setting whilefile://.../README.mdlinks above still honor it. If that toggle is meant to keep working, reorder these branches or gate the editor path behind the markdown check.Suggested ordering
- if let termSurface = terminalSurface, - let workspace = termSurface.owningWorkspace(), - !workspace.isRemoteTerminalSurface(termSurface.id), - workspace.openOrFocusWorkspaceEditor(from: termSurface.id, filePath: resolution.path) != nil { - return resolution - } - if let termSurface = terminalSurface, let workspace = termSurface.owningWorkspace(), !workspace.isRemoteTerminalSurface(termSurface.id), CmdClickMarkdownRouteSettings.shouldRoute(path: resolution.path), workspace.openOrFocusMarkdownSplit(from: termSurface.id, filePath: resolution.path) != nil { return resolution } + + if let termSurface = terminalSurface, + let workspace = termSurface.owningWorkspace(), + !workspace.isRemoteTerminalSurface(termSurface.id), + workspace.openOrFocusWorkspaceEditor(from: termSurface.id, filePath: resolution.path) != nil { + return resolution + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/GhosttyTerminalView.swift` around lines 8411 - 8425, The early-return branch that calls workspace.openOrFocusWorkspaceEditor(from:filePath:) runs before checking CmdClickMarkdownRouteSettings.shouldRoute(path:), making markdown cmd-click routing unreachable for local markdown files; change the order so that CmdClickMarkdownRouteSettings.shouldRoute(path:) is evaluated first (and if true call workspace.openOrFocusMarkdownSplit(from:filePath:) and return), or add a guard that skips openOrFocusWorkspaceEditor when CmdClickMarkdownRouteSettings.shouldRoute(path:) is true, referencing terminalSurface.owningWorkspace(), workspace.isRemoteTerminalSurface(_:), workspace.openOrFocusWorkspaceEditor(from:filePath:), CmdClickMarkdownRouteSettings.shouldRoute(path:), and workspace.openOrFocusMarkdownSplit(from:filePath:) to locate the branches to reorder or gate.Sources/Workspace.swift (1)
498-570:⚠️ Potential issue | 🟠 MajorPersist diff-mode editor state too.
SessionEditorPanelSnapshotand the.editorrestore branch only round-tripfilePath/workspaceRootDirectory, and restore always recreates a plain editor surface. That means panels opened through the new diff flow will come back as regular file views after session restore instead of restoring the diff state.Also applies to: 791-818
♻️ Duplicate comments (5)
Sources/AppDelegate.swift (1)
5984-5988:⚠️ Potential issue | 🟠 MajorRoute file opens through
Workspace.openEditor(filePath:).Both paths still depend on
focusedPanelId, so they can silently drop valid file-open requests when the focused surface is nil or not pane-backed. The workspace-level API already carries the fallback/reuse behavior for this case.Based on learnings: "In Sources/Workspace.swift, Workspace.openEditor(filePath:) must fall back to bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is nil so file-open requests aren’t dropped; it also reuses an existing editor if one is already open for the same file path."
Also applies to: 6971-6974
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 5984 - 5988, The open-via-workspace path is still depending on workspace.focusedPanelId and can drop requests when the focused surface is nil; update Workspace.openEditor(filePath:) (and any usages like Workspace.openOrFocusWorkspaceEditor) to fall back to bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is nil and to reuse an existing editor for the same file path rather than creating a new one. Locate the implementation of Workspace.openEditor(filePath:) and change its pane-selection logic to use bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first and add lookup logic that returns the existing editor panel if one already exists for filePath before instantiating a new editor panel.Sources/Workspace.swift (1)
10713-10723:⚠️ Potential issue | 🟠 MajorDon't drop editor-open requests when there is no focused pane or the split fails.
These paths still return
nileven when the workspace has live panes: the fallback only checksfocusedPaneId, andnewEditorSplitFromFocusedPanel(...)still gives up ifnewEditorSplit(...)fails. Mirror the existing file-open fallback pattern and usebonsplitController.allPaneIds.firstplusnewEditorSurface(...)as the last resort.Based on learnings: `Workspace.openEditor(filePath:) must fall back to bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is nil so file-open requests aren’t dropped`; `openFileInTextEditor(_:) must attempt workspace.newTextEditorSplit(from:orientation:filePath:focus:) and, if that returns nil, fall back to workspace.newTextEditorSurface(inPane:filePath:focus:)`.Suggested fix
`@discardableResult` func newEditorSurfaceInFocusedPane(focus: Bool = true) -> EditorPanel? { - guard let paneId = bonsplitController.focusedPaneId else { return nil } + guard let paneId = bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first else { + return nil + } let directory = normalizedSidebarDirectory(currentDirectory) ?? FileManager.default.homeDirectoryForCurrentUser.path return newEditorSurface(inPane: paneId, workspaceRootDirectory: directory, focus: focus) } `@discardableResult` func newEditorSplitFromFocusedPanel(focus: Bool = true) -> EditorPanel? { let directory = normalizedSidebarDirectory(currentDirectory) ?? FileManager.default.homeDirectoryForCurrentUser.path + guard let paneId = bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first else { + return nil + } guard let sourcePanelId = focusedPanelId else { - return newEditorSurfaceInFocusedPane(focus: focus) + return newEditorSurface(inPane: paneId, workspaceRootDirectory: directory, focus: focus) } - return newEditorSplit( + if let panel = newEditorSplit( from: sourcePanelId, orientation: .horizontal, insertFirst: false, workspaceRootDirectory: directory, focus: focus - ) + ) { + return panel + } + return newEditorSurface(inPane: paneId, workspaceRootDirectory: directory, focus: focus) }Also applies to: 10856-10878
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 10713 - 10723, The current open-editor paths drop requests when focusedPaneId is nil or when split creation fails; update Workspace.openEditor(filePath:) to fall back to bonsplitController.allPaneIds.first and call newEditorSurface(inPane:workspaceRootDirectory:focus:) as the last resort, and update any split-then-surface flows (e.g. newEditorSplitFromFocusedPanel(...) and openFileInTextEditor(_:) ) so that if newEditorSplit(...) or newTextEditorSplit(from:orientation:filePath:focus:) returns nil you then call newEditorSurface(inPane:workspaceRootDirectory:focus:) or newTextEditorSurface(inPane:filePath:focus:) respectively; ensure you reference and use bonsplitController.allPaneIds.first when focusedPaneId is nil and return the surface instead of nil.Sources/FileExplorerView.swift (2)
810-820:⚠️ Potential issue | 🟠 MajorFallback notification is still missing source-window routing context.
Line 815 posts a global event with only
path, so multi-window routing can still open in the wrong workspace whenonOpenFileis nil.🔧 Suggested fix (this file + paired AppDelegate handler update)
private func openFileInCodeViewer(_ path: String) { if let onOpenFile { onOpenFile(path) return } + var userInfo: [String: Any] = ["path": path] + if let windowId = containerView?.window?.windowNumber { + userInfo["windowId"] = windowId + } NotificationCenter.default.post( name: .fileExplorerOpenInCodeViewer, object: nil, - userInfo: ["path": path] + userInfo: userInfo ) }Based on learnings, file-explorer actions should stay aligned with the originating window context (AppDelegate fileExplorerState/window-context synchronization pattern).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/FileExplorerView.swift` around lines 810 - 820, openFileInCodeViewer's fallback NotificationCenter.post currently sends only ["path": path], losing the originating window context and causing multi-window routing to open files in the wrong workspace; update the fallback to include the source window/fileExplorerState context (the same routing key used by the AppDelegate handler) when onOpenFile is nil so .fileExplorerOpenInCodeViewer notifications carry both "path" and the originating window context, and then update the paired AppDelegate handler that observes .fileExplorerOpenInCodeViewer to read that new context field and use it to resolve the correct window/workspace.
522-528:⚠️ Potential issue | 🟠 MajorDouble-click on directories still loses expand/collapse behavior.
Line 967 overrides
NSOutlineView’s default double-click behavior, and Line 526 returns early for directories. Result: double-clicking a folder no longer toggles expansion.🔧 Suggested fix
`@objc` func openSelectedFileFromOutline(_ sender: NSOutlineView) { let row = sender.clickedRow >= 0 ? sender.clickedRow : sender.selectedRow guard row >= 0, - let node = sender.item(atRow: row) as? FileExplorerNode, - !node.isDirectory else { return } + let node = sender.item(atRow: row) as? FileExplorerNode else { return } + if node.isDirectory { + if sender.isItemExpanded(node) { + sender.collapseItem(node) + } else { + sender.expandItem(node) + } + return + } openFileInCodeViewer(node.path) }Also applies to: 966-967
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/FileExplorerView.swift` around lines 522 - 528, The handler openSelectedFileFromOutline currently returns early for directories, which prevents NSOutlineView's double-click expand/collapse behavior; change the directory branch to call the outline view's toggle method instead of returning (e.g., if let node = sender.item(atRow: row) as? FileExplorerNode, node.isDirectory { sender.toggleItem(node); return }), leaving non-directory behavior calling openFileInCodeViewer(node.path).Sources/TerminalController.swift (1)
8504-8525:⚠️ Potential issue | 🟡 MinorReject non-positive
columnvalues when the key is present.
linenow validates correctly, butcolumnstill accepts0/negative values and returns success. Keep the same contract here and only default to1whencolumnis omitted.Based on learnings: in `Sources/TerminalController.swift`, numeric JSON params should use `v2Int(...)`, and parsed values should be validated/clamped as appropriate.🛠️ Suggested guard
let column = v2Int(params, "column") + if let column, column <= 0 { + return .err(code: "invalid_params", message: "Invalid 'column' parameter (must be positive integer)", data: nil) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalController.swift` around lines 8504 - 8525, Reject non-positive "column" values when the key is present by checking whether the "column" key exists in params and validating the parsed value from v2Int(params, "column"); inside the v2MainSync block after let column = v2Int(params, "column") bail out with result = .err(code: "invalid_args", message: "Invalid column", data: nil) if params contains "column" but column != nil && column! <= 0, and only use column ?? 1 to default to 1 when the key is omitted; update the branch around editorPanel.goToLine(...) and the final result to reflect this validation.
🧹 Nitpick comments (1)
CLI/cmux.swift (1)
3525-3527: Validate--directionlocally against documented values.Line 3525 accepts any string, but help text constrains it to
left|right|up|down. Early validation gives clearer CLI errors and avoids avoidable RPC calls.Suggested fix
- let direction = directionOpt ?? "right" + let direction = (directionOpt ?? "right").lowercased() + let allowedDirections: Set<String> = ["left", "right", "up", "down"] + guard allowedDirections.contains(direction) else { + throw CLIError( + message: "editor: invalid --direction '\(direction)'. Expected one of: left, right, up, down." + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 3525 - 3527, Validate the incoming directionOpt before building params: restrict directionOpt to the allowed set ["left","right","up","down"], fall back to "right" when nil, and if an invalid value is provided emit a clear CLI error (referencing directionOpt / direction) and exit/fail early instead of proceeding to create params and make RPCs; update the code around where directionOpt, direction and params are set (using absolutePath and params) to perform this check and error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 3501-3522: The editor subcommand parsing currently only consumes
subArgs.first and silently ignores extra positional tokens (args), so update the
logic around subArgs/args handling to reject any additional positional
arguments: after you set subArgs (in the branches that set subcommand/open/diff
and the branch that assigns subArgs = args), validate that subArgs.count == 1
(or throw a CLIError with the same Usage text) before proceeding to unwrap
rawPath; specifically add a check using subArgs.count and throw an error if > 1
to prevent trailing tokens from being dropped (references: variables subArgs,
args, subcommand, rawPath and the existing error messages).
In `@Resources/editor-monaco/editor-bridge.js`:
- Around line 36-63: The editorOptions function currently sets editor mutability
flags to allow editing; change the options in editorOptions so the Monaco
instance is truly read-only by setting readOnly to true and domReadOnly to true
(update the properties named readOnly and domReadOnly inside the editorOptions
return object); ensure these two flags reflect the PR objective of a read-only
Monaco editor panel.
- Around line 158-173: setContent currently no-ops when isDiffMode is false and
editor is null, silently dropping updates; modify setContent so that when
isDiffMode is false and editor is null it creates a new normal editor (use
monaco.editor.create with editorOptions(content, currentLanguage)) and calls
wireNormalEditor (mirroring the isDiffMode branch) instead of doing nothing,
otherwise keep the existing replaceEditorContent(path) behavior; reference the
setContent function, the editor variable, isDiffMode flag, replaceEditorContent,
editorOptions and wireNormalEditor when making the change.
In `@Sources/ContentView.swift`:
- Around line 2674-2680: openFileInSelectedWorkspaceFileArea is dropping
file-open requests when the current focused pane is nil; update the call path in
openOrFocusWorkspaceEditor (or the code that creates a new editor) to fall back
to bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is
nil: use bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first
as the paneId when calling newEditorSurface(inPane:...,
workspaceRootDirectory:..., focus: true), then call navigateToFile(filePath) on
the created editorPanel so the file opens in the first pane if no pane is
focused.
In `@Sources/Panels/EditorPanel.swift`:
- Around line 434-447: loadFileContent() currently returns early when isDirty is
true, which prevents detecting a deleted/renamed file and setting
isFileUnavailable; update loadFileContent() (and the similar block around the
second occurrence) so existence checks run even if isDirty: move or add a
FileManager.default.fileExists(atPath: filePath) check before the isDirty guard
(or, if isDirty, still call FileManager.default.contents(atPath:)/fileExists and
set isFileUnavailable when missing) so delete/rename handlers can mark the file
unavailable and allow startFileWatcher()/reattach logic to proceed correctly.
- Around line 161-163: monacoLanguage currently returns "plaintext" for files
like Dockerfile because it only uses (filePath as NSString).pathExtension;
update monacoLanguage to check the filename when pathExtension is empty (or
always check lastPathComponent) and return "dockerfile" when the basename equals
"Dockerfile" (case-insensitive). Specifically, modify the monacoLanguage
computed property to call Self.monacoLanguageFromExtension for non-empty
extensions but also handle the special-case by inspecting (filePath as
NSString).lastPathComponent (or comparing filePath's last path component) and
mapping "Dockerfile" to "dockerfile" before falling back to plaintext; reference
monacoLanguage and monacoLanguageFromExtension to locate the code.
In `@Sources/TerminalController.swift`:
- Around line 8271-8280: ws.newEditorSplit(...) can return nil when the current
surface can't anchor a split; instead of immediately returning the
internal_error from the guard in the editor-open path, implement the existing
pane-fallback: have openFileInTextEditor(_:) fall back to creating a new text
editor surface via workspace.newTextEditorSurface(...) if newEditorSplit returns
nil, and have Workspace.openEditor(filePath:) use
bonsplitController.allPaneIds.first as the focused pane fallback before failing.
Update the guard handling around createdPanel?.id to attempt these fallbacks
(newTextEditorSurface then bonsplitController.allPaneIds.first) and only return
the internal_error if both fallbacks fail.
- Around line 8251-8258: The current handlers use v2UUID(params, "surface_id")
?? ws.focusedPanelId which treats a malformed or unresolvable "surface_id" as if
the key were omitted; change the logic in each handler (e.g., the split handler
and v2WorkspaceClearTags(params:)) to first check whether "surface_id" exists in
params, then: if absent, fall back to ws.focusedPanelId; if present but v2UUID
returns nil, return .err(code: "invalid_params", message: "Invalid surface_id",
data: ["surface_id": params["surface_id"]]); only when the key is missing should
you use ws.focusedPanelId, and otherwise validate using v2UUID and verify
ws.panels[...] before proceeding.
- Around line 8434-8446: The handler currently resolves a TabManager via
v2ResolveTabManager(params:) and then only searches that workspace, causing
surface-only requests to be biased to the active window; update the logic after
obtaining surfaceId (from v2UUID(params, "surface_id") ?? ws.focusedPanelId) to
check ownership of surfaceId on the resolved tabManager (e.g., whether
ws.editorPanel(for: surfaceId) exists) and if it does not, call
AppDelegate.shared?.locateSurface(surfaceId:) to get the correct owning
workspace/tabManager and re-resolve the workspace/editorPanel there (mirroring
the ownership-check/fallback pattern used by the panel-id routes); apply the
same change to the other occurrences noted (around the blocks at 8462-8475 and
8498-8515) so resolution uses the located surface when the initial manager
doesn’t own it.
- Around line 8480-8484: The current logic silently falls back to
ws.focusedTerminalPanel when target_surface_id is present but invalid or points
to a non-terminal; change it so: check whether params contains the
"target_surface_id" key first; if the key is missing, use
ws.focusedTerminalPanel as now; if the key is present but v2UUID(params,
"target_surface_id") returns nil, set result = .err(code: "invalid_params",
message: "Malformed target_surface_id", data: nil) and return; if v2UUID returns
a UUID but ws.terminalPanel(for: thatUUID) is nil, set result = .err(code:
"not_found", message: "No target terminal panel found", data: nil) and return;
otherwise proceed with the found targetTerminal. Ensure you update the guard
around targetSurfaceId, ws.terminalPanel(for:), and ws.focusedTerminalPanel to
implement these three branches.
---
Outside diff comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 8411-8425: The early-return branch that calls
workspace.openOrFocusWorkspaceEditor(from:filePath:) runs before checking
CmdClickMarkdownRouteSettings.shouldRoute(path:), making markdown cmd-click
routing unreachable for local markdown files; change the order so that
CmdClickMarkdownRouteSettings.shouldRoute(path:) is evaluated first (and if true
call workspace.openOrFocusMarkdownSplit(from:filePath:) and return), or add a
guard that skips openOrFocusWorkspaceEditor when
CmdClickMarkdownRouteSettings.shouldRoute(path:) is true, referencing
terminalSurface.owningWorkspace(), workspace.isRemoteTerminalSurface(_:),
workspace.openOrFocusWorkspaceEditor(from:filePath:),
CmdClickMarkdownRouteSettings.shouldRoute(path:), and
workspace.openOrFocusMarkdownSplit(from:filePath:) to locate the branches to
reorder or gate.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 5984-5988: The open-via-workspace path is still depending on
workspace.focusedPanelId and can drop requests when the focused surface is nil;
update Workspace.openEditor(filePath:) (and any usages like
Workspace.openOrFocusWorkspaceEditor) to fall back to
bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is nil
and to reuse an existing editor for the same file path rather than creating a
new one. Locate the implementation of Workspace.openEditor(filePath:) and change
its pane-selection logic to use bonsplitController.focusedPaneId ??
bonsplitController.allPaneIds.first and add lookup logic that returns the
existing editor panel if one already exists for filePath before instantiating a
new editor panel.
In `@Sources/FileExplorerView.swift`:
- Around line 810-820: openFileInCodeViewer's fallback NotificationCenter.post
currently sends only ["path": path], losing the originating window context and
causing multi-window routing to open files in the wrong workspace; update the
fallback to include the source window/fileExplorerState context (the same
routing key used by the AppDelegate handler) when onOpenFile is nil so
.fileExplorerOpenInCodeViewer notifications carry both "path" and the
originating window context, and then update the paired AppDelegate handler that
observes .fileExplorerOpenInCodeViewer to read that new context field and use it
to resolve the correct window/workspace.
- Around line 522-528: The handler openSelectedFileFromOutline currently returns
early for directories, which prevents NSOutlineView's double-click
expand/collapse behavior; change the directory branch to call the outline view's
toggle method instead of returning (e.g., if let node = sender.item(atRow: row)
as? FileExplorerNode, node.isDirectory { sender.toggleItem(node); return }),
leaving non-directory behavior calling openFileInCodeViewer(node.path).
In `@Sources/TerminalController.swift`:
- Around line 8504-8525: Reject non-positive "column" values when the key is
present by checking whether the "column" key exists in params and validating the
parsed value from v2Int(params, "column"); inside the v2MainSync block after let
column = v2Int(params, "column") bail out with result = .err(code:
"invalid_args", message: "Invalid column", data: nil) if params contains
"column" but column != nil && column! <= 0, and only use column ?? 1 to default
to 1 when the key is omitted; update the branch around editorPanel.goToLine(...)
and the final result to reflect this validation.
In `@Sources/Workspace.swift`:
- Around line 10713-10723: The current open-editor paths drop requests when
focusedPaneId is nil or when split creation fails; update
Workspace.openEditor(filePath:) to fall back to
bonsplitController.allPaneIds.first and call
newEditorSurface(inPane:workspaceRootDirectory:focus:) as the last resort, and
update any split-then-surface flows (e.g. newEditorSplitFromFocusedPanel(...)
and openFileInTextEditor(_:) ) so that if newEditorSplit(...) or
newTextEditorSplit(from:orientation:filePath:focus:) returns nil you then call
newEditorSurface(inPane:workspaceRootDirectory:focus:) or
newTextEditorSurface(inPane:filePath:focus:) respectively; ensure you reference
and use bonsplitController.allPaneIds.first when focusedPaneId is nil and return
the surface instead of nil.
---
Nitpick comments:
In `@CLI/cmux.swift`:
- Around line 3525-3527: Validate the incoming directionOpt before building
params: restrict directionOpt to the allowed set ["left","right","up","down"],
fall back to "right" when nil, and if an invalid value is provided emit a clear
CLI error (referencing directionOpt / direction) and exit/fail early instead of
proceeding to create params and make RPCs; update the code around where
directionOpt, direction and params are set (using absolutePath and params) to
perform this check and error handling.
🪄 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: 3352e9f0-e589-45d1-af28-615e1368a508
📒 Files selected for processing (15)
CLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsResources/editor-monaco/editor-bridge.jsSources/AppDelegate.swiftSources/ContentView.swiftSources/FileExplorerView.swiftSources/GhosttyTerminalView.swiftSources/Panels/EditorPanel.swiftSources/Panels/EditorPanelView.swiftSources/RightSidebarPanelView.swiftSources/SessionPersistence.swiftSources/TerminalController.swiftSources/Workspace.swiftvendor/bonsplit
✅ Files skipped from review due to trivial changes (2)
- vendor/bonsplit
- Resources/Localizable.xcstrings
🚧 Files skipped from review as they are similar to previous changes (1)
- GhosttyTabs.xcodeproj/project.pbxproj
Comment on lines
+3501
to
+3522
| if let first = args.first, ["open", "diff"].contains(first.lowercased()) { | ||
| subcommand = first.lowercased() | ||
| subArgs = Array(args.dropFirst()) | ||
| } else if args.count == 1, let first = args.first, !first.hasPrefix("-") { | ||
| subcommand = "open" | ||
| subArgs = [first] | ||
| } else if let first = args.first, first.hasPrefix("-") { | ||
| throw CLIError( | ||
| message: "editor: unknown flag '\(first)'. Usage: cmux editor open <path> | cmux editor diff <path>" | ||
| ) | ||
| } else if let first = args.first, looksLikePath(first) || first.contains(".") { | ||
| subcommand = "open" | ||
| subArgs = args | ||
| } else if let first = args.first { | ||
| throw CLIError(message: "Unknown editor subcommand: \(first). Usage: cmux editor open <path> | cmux editor diff <path>") | ||
| } else { | ||
| throw CLIError(message: "editor requires a subcommand. Usage: cmux editor open <path> | cmux editor diff <path>") | ||
| } | ||
|
|
||
| guard let rawPath = subArgs.first, !rawPath.isEmpty else { | ||
| throw CLIError(message: "editor \(subcommand) requires a file path. Usage: cmux editor \(subcommand) <path>") | ||
| } |
There was a problem hiding this comment.
Reject extra editor arguments instead of silently ignoring them.
At Line 3520, only subArgs.first is consumed, so trailing tokens are dropped. That makes malformed invocations appear successful and hides user mistakes.
Suggested fix
- guard let rawPath = subArgs.first, !rawPath.isEmpty else {
+ guard let rawPath = subArgs.first, !rawPath.isEmpty else {
throw CLIError(message: "editor \(subcommand) requires a file path. Usage: cmux editor \(subcommand) <path>")
}
+ if subArgs.count > 1 {
+ let extras = Array(subArgs.dropFirst())
+ if let badFlag = extras.first(where: { $0.hasPrefix("-") }) {
+ throw CLIError(
+ message: "editor: unknown flag '\(badFlag)'. Usage: cmux editor open <path> | cmux editor diff <path>"
+ )
+ }
+ throw CLIError(
+ message: "editor \(subcommand) accepts exactly one <path>; unexpected arguments: \(extras.joined(separator: " "))"
+ )
+ }Based on learnings: In CLI/cmux.swift, set-workspace-color enforces exactly one trailing argument and rejects unexpected positional arguments to prevent malformed input.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 3501 - 3522, The editor subcommand parsing
currently only consumes subArgs.first and silently ignores extra positional
tokens (args), so update the logic around subArgs/args handling to reject any
additional positional arguments: after you set subArgs (in the branches that set
subcommand/open/diff and the branch that assigns subArgs = args), validate that
subArgs.count == 1 (or throw a CLIError with the same Usage text) before
proceeding to unwrap rawPath; specifically add a check using subArgs.count and
throw an error if > 1 to prevent trailing tokens from being dropped (references:
variables subArgs, args, subcommand, rawPath and the existing error messages).
Comment on lines
+36
to
+63
| function editorOptions(value, language) { | ||
| return { | ||
| value: value || "", | ||
| language: language || "plaintext", | ||
| theme: detectTheme(), | ||
| readOnly: false, | ||
| automaticLayout: true, | ||
| minimap: { enabled: true }, | ||
| scrollBeyondLastLine: false, | ||
| fontSize: 12, | ||
| fontFamily: '"SF Mono", Menlo, Monaco, "Courier New", monospace', | ||
| lineNumbers: "on", | ||
| renderWhitespace: "none", | ||
| wordWrap: "off", | ||
| folding: true, | ||
| glyphMargin: false, | ||
| largeFileOptimizations: true, | ||
| maxTokenizationLineLength: 20000, | ||
| scrollbar: { | ||
| verticalScrollbarSize: 10, | ||
| horizontalScrollbarSize: 10, | ||
| }, | ||
| overviewRulerLanes: 0, | ||
| hideCursorInOverviewRuler: true, | ||
| contextmenu: false, | ||
| domReadOnly: false, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Normal editor is writable, which conflicts with the read-only panel scope.
Line 41 (readOnly: false) and Line 61 (domReadOnly: false) currently allow edits in normal mode. That contradicts the PR objective (“read-only Monaco editor panel”).
♻️ Proposed fix
function editorOptions(value, language) {
return {
value: value || "",
language: language || "plaintext",
theme: detectTheme(),
- readOnly: false,
+ readOnly: true,
automaticLayout: true,
@@
- domReadOnly: false,
+ domReadOnly: true,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function editorOptions(value, language) { | |
| return { | |
| value: value || "", | |
| language: language || "plaintext", | |
| theme: detectTheme(), | |
| readOnly: false, | |
| automaticLayout: true, | |
| minimap: { enabled: true }, | |
| scrollBeyondLastLine: false, | |
| fontSize: 12, | |
| fontFamily: '"SF Mono", Menlo, Monaco, "Courier New", monospace', | |
| lineNumbers: "on", | |
| renderWhitespace: "none", | |
| wordWrap: "off", | |
| folding: true, | |
| glyphMargin: false, | |
| largeFileOptimizations: true, | |
| maxTokenizationLineLength: 20000, | |
| scrollbar: { | |
| verticalScrollbarSize: 10, | |
| horizontalScrollbarSize: 10, | |
| }, | |
| overviewRulerLanes: 0, | |
| hideCursorInOverviewRuler: true, | |
| contextmenu: false, | |
| domReadOnly: false, | |
| }; | |
| } | |
| function editorOptions(value, language) { | |
| return { | |
| value: value || "", | |
| language: language || "plaintext", | |
| theme: detectTheme(), | |
| readOnly: true, | |
| automaticLayout: true, | |
| minimap: { enabled: true }, | |
| scrollBeyondLastLine: false, | |
| fontSize: 12, | |
| fontFamily: '"SF Mono", Menlo, Monaco, "Courier New", monospace', | |
| lineNumbers: "on", | |
| renderWhitespace: "none", | |
| wordWrap: "off", | |
| folding: true, | |
| glyphMargin: false, | |
| largeFileOptimizations: true, | |
| maxTokenizationLineLength: 20000, | |
| scrollbar: { | |
| verticalScrollbarSize: 10, | |
| horizontalScrollbarSize: 10, | |
| }, | |
| overviewRulerLanes: 0, | |
| hideCursorInOverviewRuler: true, | |
| contextmenu: false, | |
| domReadOnly: true, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resources/editor-monaco/editor-bridge.js` around lines 36 - 63, The
editorOptions function currently sets editor mutability flags to allow editing;
change the options in editorOptions so the Monaco instance is truly read-only by
setting readOnly to true and domReadOnly to true (update the properties named
readOnly and domReadOnly inside the editorOptions return object); ensure these
two flags reflect the PR objective of a read-only Monaco editor panel.
Comment on lines
+158
to
+173
| setContent: function (content, language, filePath) { | ||
| currentLanguage = language || "plaintext"; | ||
| currentFilePath = filePath || ""; | ||
|
|
||
| if (isDiffMode && diffEditor) { | ||
| disposeDiffEditor(); | ||
| document.getElementById("editor-container").innerHTML = ""; | ||
| editor = monaco.editor.create( | ||
| document.getElementById("editor-container"), | ||
| editorOptions(content, currentLanguage) | ||
| ); | ||
| wireNormalEditor(); | ||
| } else if (editor) { | ||
| replaceEditorContent(content, currentLanguage); | ||
| } | ||
| }, |
There was a problem hiding this comment.
setContent can silently drop updates when no editor instance exists.
If isDiffMode is false and editor is null, the current logic does nothing. This creates a silent failure path for content application.
♻️ Proposed hardening
setContent: function (content, language, filePath) {
currentLanguage = language || "plaintext";
currentFilePath = filePath || "";
if (isDiffMode && diffEditor) {
disposeDiffEditor();
document.getElementById("editor-container").innerHTML = "";
editor = monaco.editor.create(
document.getElementById("editor-container"),
editorOptions(content, currentLanguage)
);
wireNormalEditor();
} else if (editor) {
replaceEditorContent(content, currentLanguage);
+ } else {
+ var container = document.getElementById("editor-container");
+ if (!container) return;
+ container.innerHTML = "";
+ editor = monaco.editor.create(
+ container,
+ editorOptions(content, currentLanguage)
+ );
+ wireNormalEditor();
}
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resources/editor-monaco/editor-bridge.js` around lines 158 - 173, setContent
currently no-ops when isDiffMode is false and editor is null, silently dropping
updates; modify setContent so that when isDiffMode is false and editor is null
it creates a new normal editor (use monaco.editor.create with
editorOptions(content, currentLanguage)) and calls wireNormalEditor (mirroring
the isDiffMode branch) instead of doing nothing, otherwise keep the existing
replaceEditorContent(path) behavior; reference the setContent function, the
editor variable, isDiffMode flag, replaceEditorContent, editorOptions and
wireNormalEditor when making the change.
Comment on lines
+2674
to
+2680
| private func openFileInSelectedWorkspaceFileArea(_ path: String) { | ||
| guard let workspace = tabManager.selectedWorkspace, | ||
| !workspace.isRemoteWorkspace else { return } | ||
| _ = workspace.openOrFocusWorkspaceEditor( | ||
| from: workspace.focusedPanelId, | ||
| filePath: path | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
workspace_file="$(fd -i 'Workspace.swift' Sources | head -n1)"
content_file="$(fd -i 'ContentView.swift' Sources | head -n1)"
echo "workspace_file=${workspace_file}"
echo "content_file=${content_file}"
echo "--- Editor open helpers ---"
rg -n -C4 'func\s+openOrFocusWorkspaceEditor|func\s+openEditor\(' "$workspace_file"
echo "--- Fallback signals (split/surface/focusedPane/allPaneIds/existing editor) ---"
rg -n -C4 'newTextEditorSplit|newTextEditorSurface|focusedPaneId|allPaneIds\.first|existing.*editor|reuse' "$workspace_file"
echo "--- Sidebar call site ---"
rg -n -C2 'openFileInSelectedWorkspaceFileArea|openOrFocusWorkspaceEditor' "$content_file"Repository: manaflow-ai/cmux
Length of output: 28216
🏁 Script executed:
sed -n '10631,10724p' Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 3839
Missing fallback to first pane when focused pane is unavailable.
The method openOrFocusWorkspaceEditor correctly reuses existing editors and attempts to create a split, but line 10718 must fall back to bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is nil, otherwise file-open requests are dropped. Update:
if let paneId = bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first,
let editorPanel = newEditorSurface(
inPane: paneId,
workspaceRootDirectory: rootDirectory,
focus: true
) {
_ = editorPanel.navigateToFile(filePath)
return editorPanel
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 2674 - 2680,
openFileInSelectedWorkspaceFileArea is dropping file-open requests when the
current focused pane is nil; update the call path in openOrFocusWorkspaceEditor
(or the code that creates a new editor) to fall back to
bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is
nil: use bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first
as the paneId when calling newEditorSurface(inPane:...,
workspaceRootDirectory:..., focus: true), then call navigateToFile(filePath) on
the created editorPanel so the file opens in the first pane if no pane is
focused.
Comment on lines
+161
to
+163
| var monacoLanguage: String { | ||
| guard !filePath.isEmpty else { return "plaintext" } | ||
| return Self.monacoLanguageFromExtension((filePath as NSString).pathExtension) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Swift/Foundation, what does NSString.pathExtensionreturn for a filename likeDockerfile, and would a switch case on "dockerfile"ever match when the code passes(filePath as NSString).pathExtension?
💡 Result:
For the filename "Dockerfile", NSString.pathExtension returns an empty string (""). A switch case on "dockerfile" would never match when using (filePath as NSString).pathExtension, because the result is empty, not "dockerfile".
Citations:
- 1: https://developer.apple.com/documentation/foundation/nsstring/pathextension
- 2: https://developer.apple.com/documentation/foundation/nsstring/pathextension?language=objc
🏁 Script executed:
# First, let's examine the code around lines 161-163 in EditorPanel.swift
head -n 170 Sources/Panels/EditorPanel.swift | tail -n 20Repository: manaflow-ai/cmux
Length of output: 741
🏁 Script executed:
# Also check around line 566
sed -n '560,570p' Sources/Panels/EditorPanel.swiftRepository: manaflow-ai/cmux
Length of output: 485
🏁 Script executed:
# Search for the monacoLanguageFromExtension function to verify if "dockerfile" case exists
rg -A 50 "monacoLanguageFromExtension" Sources/Panels/EditorPanel.swiftRepository: manaflow-ai/cmux
Length of output: 4089
Dockerfile cannot be detected by extension and always falls back to plaintext.
monacoLanguage passes pathExtension to monacoLanguageFromExtension, which tries to match against the "dockerfile" case. However, Dockerfile has no extension, so pathExtension returns an empty string, making the case unreachable and causing all Dockerfile files to display as plaintext instead of with Dockerfile syntax highlighting.
♻️ Proposed fix
var monacoLanguage: String {
guard !filePath.isEmpty else { return "plaintext" }
+ let basename = (filePath as NSString).lastPathComponent.lowercased()
+ if basename == "dockerfile" {
+ return "dockerfile"
+ }
return Self.monacoLanguageFromExtension((filePath as NSString).pathExtension)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/EditorPanel.swift` around lines 161 - 163, monacoLanguage
currently returns "plaintext" for files like Dockerfile because it only uses
(filePath as NSString).pathExtension; update monacoLanguage to check the
filename when pathExtension is empty (or always check lastPathComponent) and
return "dockerfile" when the basename equals "Dockerfile" (case-insensitive).
Specifically, modify the monacoLanguage computed property to call
Self.monacoLanguageFromExtension for non-empty extensions but also handle the
special-case by inspecting (filePath as NSString).lastPathComponent (or
comparing filePath's last path component) and mapping "Dockerfile" to
"dockerfile" before falling back to plaintext; reference monacoLanguage and
monacoLanguageFromExtension to locate the code.
Comment on lines
+8251
to
+8258
| let sourceSurfaceId = v2UUID(params, "surface_id") ?? ws.focusedPanelId | ||
| guard let sourceSurfaceId else { | ||
| result = .err(code: "not_found", message: "No focused surface to split", data: nil) | ||
| return | ||
| } | ||
| guard ws.panels[sourceSurfaceId] != nil else { | ||
| result = .err(code: "not_found", message: "Source surface not found", data: ["surface_id": sourceSurfaceId.uuidString]) | ||
| return |
There was a problem hiding this comment.
Don't treat a malformed surface_id as “use the focused panel”.
These handlers all do v2UUID(params, "surface_id") ?? ws.focusedPanelId. If the caller explicitly passes a bad handle/UUID, the request can act on the focused surface instead of failing fast. Only fall back when the key is absent; if surface_id is present but unresolved, return invalid_params.
Based on learnings: v2WorkspaceClearTags(params:) only falls back when the key is absent; if the key is present but invalid, it should return invalid_params.
Also applies to: 8404-8410, 8445-8447, 8473-8475, 8513-8515
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 8251 - 8258, The current
handlers use v2UUID(params, "surface_id") ?? ws.focusedPanelId which treats a
malformed or unresolvable "surface_id" as if the key were omitted; change the
logic in each handler (e.g., the split handler and
v2WorkspaceClearTags(params:)) to first check whether "surface_id" exists in
params, then: if absent, fall back to ws.focusedPanelId; if present but v2UUID
returns nil, return .err(code: "invalid_params", message: "Invalid surface_id",
data: ["surface_id": params["surface_id"]]); only when the key is missing should
you use ws.focusedPanelId, and otherwise validate using v2UUID and verify
ws.panels[...] before proceeding.
Comment on lines
+8271
to
+8280
| let createdPanel = ws.newEditorSplit( | ||
| from: sourceSurfaceId, | ||
| orientation: orientation, | ||
| insertFirst: insertFirst, | ||
| filePath: filePath, | ||
| focus: v2FocusAllowed() | ||
| ) | ||
|
|
||
| guard let editorPanelId = createdPanel?.id else { | ||
| result = .err(code: "internal_error", message: "Failed to create editor panel", data: nil) |
There was a problem hiding this comment.
Add the existing pane-fallback path before returning internal_error.
ws.newEditorSplit(...) can legitimately return nil when the current surface cannot anchor a split. In that case editor.open drops the request even though the workspace may still have a focused pane or allPaneIds.first available. Mirror the repo's existing editor-open fallback instead of hard-failing here.
Based on learnings: openFileInTextEditor(_:) must fall back to workspace.newTextEditorSurface(...) when split creation returns nil, and Workspace.openEditor(filePath:) must fall back to bonsplitController.allPaneIds.first so file-open requests aren’t dropped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 8271 - 8280,
ws.newEditorSplit(...) can return nil when the current surface can't anchor a
split; instead of immediately returning the internal_error from the guard in the
editor-open path, implement the existing pane-fallback: have
openFileInTextEditor(_:) fall back to creating a new text editor surface via
workspace.newTextEditorSurface(...) if newEditorSplit returns nil, and have
Workspace.openEditor(filePath:) use bonsplitController.allPaneIds.first as the
focused pane fallback before failing. Update the guard handling around
createdPanel?.id to attempt these fallbacks (newTextEditorSurface then
bonsplitController.allPaneIds.first) and only return the internal_error if both
fallbacks fail.
Comment on lines
+8434
to
+8446
| guard let tabManager = v2ResolveTabManager(params: params) else { | ||
| return .err(code: "unavailable", message: "TabManager not available", data: nil) | ||
| } | ||
|
|
||
| var result: V2CallResult = .err(code: "internal_error", message: "Failed to get content", data: nil) | ||
| v2MainSync { | ||
| guard let ws = v2ResolveWorkspace(params: params, tabManager: tabManager) else { | ||
| result = .err(code: "not_found", message: "Workspace not found", data: nil) | ||
| return | ||
| } | ||
|
|
||
| let surfaceId = v2UUID(params, "surface_id") ?? ws.focusedPanelId | ||
| guard let surfaceId, let editorPanel = ws.editorPanel(for: surfaceId) else { |
There was a problem hiding this comment.
Surface-only editor routes still have active-window bias.
When callers provide only surface_id, these handlers start from v2ResolveTabManager(params:) and then search that workspace only. A request targeting an editor in another window can therefore miss the real owner and report not_found. Use the same ownership check/fallback pattern as the other panel-id routes: if the resolved manager doesn't own the surface, re-locate it via AppDelegate.shared?.locateSurface(surfaceId:).
Based on learnings: for panel_id-only routes, first attempt v2ResolveTabManager(params:), but if that manager does not own the panel, fall back to AppDelegate.shared?.locateSurface(surfaceId:) to avoid active-window bias.
Also applies to: 8462-8475, 8498-8515
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 8434 - 8446, The handler
currently resolves a TabManager via v2ResolveTabManager(params:) and then only
searches that workspace, causing surface-only requests to be biased to the
active window; update the logic after obtaining surfaceId (from v2UUID(params,
"surface_id") ?? ws.focusedPanelId) to check ownership of surfaceId on the
resolved tabManager (e.g., whether ws.editorPanel(for: surfaceId) exists) and if
it does not, call AppDelegate.shared?.locateSurface(surfaceId:) to get the
correct owning workspace/tabManager and re-resolve the workspace/editorPanel
there (mirroring the ownership-check/fallback pattern used by the panel-id
routes); apply the same change to the other occurrences noted (around the blocks
at 8462-8475 and 8498-8515) so resolution uses the located surface when the
initial manager doesn’t own it.
Comment on lines
+8480
to
+8484
| let targetSurfaceId = v2UUID(params, "target_surface_id") | ||
| guard let targetTerminal = (targetSurfaceId.flatMap { ws.terminalPanel(for: $0) } ?? ws.focusedTerminalPanel) else { | ||
| result = .err(code: "not_found", message: "No target terminal panel found", data: nil) | ||
| return | ||
| } |
There was a problem hiding this comment.
Don't reroute an explicit target to the focused terminal.
If target_surface_id is present but malformed or points to a non-terminal panel, this silently falls back to ws.focusedTerminalPanel and can send the selection to the wrong shell. Only fall back when the key is absent; otherwise return invalid_params / not_found.
🛠️ Suggested guard
- let targetSurfaceId = v2UUID(params, "target_surface_id")
- guard let targetTerminal = (targetSurfaceId.flatMap { ws.terminalPanel(for: $0) } ?? ws.focusedTerminalPanel) else {
+ let hasTargetSurfaceKey = params.keys.contains("target_surface_id")
+ guard !hasTargetSurfaceKey || v2UUID(params, "target_surface_id") != nil else {
+ result = .err(code: "invalid_params", message: "Invalid 'target_surface_id' parameter", data: nil)
+ return
+ }
+ let targetTerminal =
+ v2UUID(params, "target_surface_id").flatMap { ws.terminalPanel(for: $0) }
+ ?? (!hasTargetSurfaceKey ? ws.focusedTerminalPanel : nil)
+ guard let targetTerminal else {
result = .err(code: "not_found", message: "No target terminal panel found", data: nil)
return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalController.swift` around lines 8480 - 8484, The current logic
silently falls back to ws.focusedTerminalPanel when target_surface_id is present
but invalid or points to a non-terminal; change it so: check whether params
contains the "target_surface_id" key first; if the key is missing, use
ws.focusedTerminalPanel as now; if the key is present but v2UUID(params,
"target_surface_id") returns nil, set result = .err(code: "invalid_params",
message: "Malformed target_surface_id", data: nil) and return; if v2UUID returns
a UUID but ws.terminalPanel(for: thatUUID) is nil, set result = .err(code:
"not_found", message: "No target terminal panel found", data: nil) and return;
otherwise proceed with the found targetTerminal. Ensure you update the guard
around targetSurfaceId, ws.terminalPanel(for:), and ws.focusedTerminalPanel to
implement these three branches.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Workspace.swift (1)
10855-10877:⚠️ Potential issue | 🟠 MajorRestore the same no-drop fallback here.
newEditorSurfaceInFocusedPanestill returnsnilwhenfocusedPaneIdis missing, andnewEditorSplitFromFocusedPanelstill drops the request whennewEditorSplit(...)fails. That reintroduces the silent no-op the editor/text-editor open flows were already fixed to avoid.🛠️ Proposed fix
func newEditorSurfaceInFocusedPane(focus: Bool = true) -> EditorPanel? { - guard let paneId = bonsplitController.focusedPaneId else { return nil } + guard let paneId = bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first else { + return nil + } let directory = normalizedSidebarDirectory(currentDirectory) ?? FileManager.default.homeDirectoryForCurrentUser.path return newEditorSurface(inPane: paneId, workspaceRootDirectory: directory, focus: focus) } `@discardableResult` func newEditorSplitFromFocusedPanel(focus: Bool = true) -> EditorPanel? { let directory = normalizedSidebarDirectory(currentDirectory) ?? FileManager.default.homeDirectoryForCurrentUser.path + guard let paneId = bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first else { + return nil + } guard let sourcePanelId = focusedPanelId else { return newEditorSurfaceInFocusedPane(focus: focus) } - return newEditorSplit( + if let panel = newEditorSplit( from: sourcePanelId, orientation: .horizontal, insertFirst: false, workspaceRootDirectory: directory, focus: focus - ) + ) { + return panel + } + return newEditorSurface(inPane: paneId, workspaceRootDirectory: directory, focus: focus) }Based on learnings:
Workspace.openEditor(filePath:) must fall back to bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is nil so file-open requests aren’t dropped, andopenFileInTextEditor(_:) must attempt workspace.newTextEditorSplit(...) and, if that returns nil, fall back to workspace.newTextEditorSurface(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 10855 - 10877, newEditorSurfaceInFocusedPane currently returns nil when bonsplitController.focusedPaneId is missing; change it to fall back to bonsplitController.allPaneIds.first (use the same normalizedSidebarDirectory/currentDirectory fallback) and call newEditorSurface with that pane id so requests aren't dropped. In newEditorSplitFromFocusedPanel, keep using focusedPanelId if present but after calling newEditorSplit(from:orientation:insertFirst:workspaceRootDirectory:focus:), check for a nil return and if nil call newEditorSurface(inPane:workspaceRootDirectory:focus:) with the same workspaceRootDirectory to ensure a surface is created when the split creation fails.
🧹 Nitpick comments (2)
Sources/CmuxConfig.swift (1)
2113-2121: Consider deriving fallback built-ins fromallCasesto avoid drift.The hardcoded fallback list now has to be updated every time a built-in is added. Using enum
allCaseskeeps this path automatically in sync.♻️ Suggested refactor
- }) ?? [ - .builtIn(.newTerminal), - .builtIn(.newBrowser), - .builtIn(.newEditor), - .builtIn(.splitRight), - .builtIn(.splitDown) - ] + }) ?? CmuxSurfaceTabBarBuiltInAction.allCases.map { .builtIn($0) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/CmuxConfig.swift` around lines 2113 - 2121, The fallback list in defaultResolvedButtons is hardcoded and can drift; replace the hardcoded array with a derivation from the BuiltIn enum's allCases so new built-ins auto-appear. Concretely, update the fallback branch used when CmuxSurfaceTabBarButton.defaults mapping fails to generate values by iterating CmuxSurfaceTabBarButton.BuiltIn.allCases (or the actual BuiltIn enum type used by CmuxSurfaceTabBarButton) to produce [.builtIn(<case>)] entries and then call resolved(actions: resolvedActionLookup, codingPath: []) (the same resolution used in CmuxSurfaceTabBarButton.defaults) so the fallback uses the same resolution path as the primary code paths (affecting defaultResolvedButtons and the resolved(actions:) usage).Sources/Workspace.swift (1)
7750-7768: Route these background diagnostics through the repo’s DEBUG logging path.These expanded log branches still live outside
#if DEBUGand still useGhosttyApp.shared.logBackground(...)instead ofcmuxDebugLog(...). Please keep debug-only diagnostics compile-time gated here too.As per coding guidelines, "All debug logging must be wrapped in
#if DEBUG/#endifguards, use the free functioncmuxDebugLog("message")from CMUXDebugLog package, and ensure all call sites are protected by the same guards".Also applies to: 7780-7792, 7811-7825, 7834-7841
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 7750 - 7768, The background diagnostic block (and the other similar blocks at the noted ranges) is emitting debug-only info via GhosttyApp.shared.logBackground instead of the repo DEBUG path; wrap the whole logging sequence in a compile-time guard and use the CMUX debug helper: enclose the code that computes currentColorsDescription (Self.bonsplitChromeColorsLogDescription), nextColorsDescription, currentTabFont/nextTabFont, paneBackdrop (Self.usesBonsplitPaneTerminalBackdrop) and the final log call in `#if` DEBUG / `#endif` and replace GhosttyApp.shared.logBackground(...) with a single cmuxDebugLog("...") call that formats the same message (including id.uuidString, reason, sharesWindowBackdrop, isNoOp, etc.); apply the same change to the other occurrences you noted (lines ~7780-7792, 7811-7825, 7834-7841).
🤖 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/Workspace.swift`:
- Around line 10727-10745: openOrFocusEditorSplit currently only resolves
symlinks when comparing file paths which can miss equivalents like ~/foo.swift
or ./foo.swift; replace the manual resolving with the existing
canonicalEditorPath(_:) helper so comparisons normalize tilde, relative segments
and symlinks. Specifically, call canonicalEditorPath(filePath) for the incoming
path and canonicalEditorPath(ed.filePath) when iterating panels (inside the for
(existingId, panel) loop casting to EditorPanel), then use that comparison to
decide to focusPanel(existingId) or else call newEditorSplit(...) as before.
Ensure you keep the same focus and parameters when creating the new editor
split.
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 10855-10877: newEditorSurfaceInFocusedPane currently returns nil
when bonsplitController.focusedPaneId is missing; change it to fall back to
bonsplitController.allPaneIds.first (use the same
normalizedSidebarDirectory/currentDirectory fallback) and call newEditorSurface
with that pane id so requests aren't dropped. In newEditorSplitFromFocusedPanel,
keep using focusedPanelId if present but after calling
newEditorSplit(from:orientation:insertFirst:workspaceRootDirectory:focus:),
check for a nil return and if nil call
newEditorSurface(inPane:workspaceRootDirectory:focus:) with the same
workspaceRootDirectory to ensure a surface is created when the split creation
fails.
---
Nitpick comments:
In `@Sources/CmuxConfig.swift`:
- Around line 2113-2121: The fallback list in defaultResolvedButtons is
hardcoded and can drift; replace the hardcoded array with a derivation from the
BuiltIn enum's allCases so new built-ins auto-appear. Concretely, update the
fallback branch used when CmuxSurfaceTabBarButton.defaults mapping fails to
generate values by iterating CmuxSurfaceTabBarButton.BuiltIn.allCases (or the
actual BuiltIn enum type used by CmuxSurfaceTabBarButton) to produce
[.builtIn(<case>)] entries and then call resolved(actions: resolvedActionLookup,
codingPath: []) (the same resolution used in CmuxSurfaceTabBarButton.defaults)
so the fallback uses the same resolution path as the primary code paths
(affecting defaultResolvedButtons and the resolved(actions:) usage).
In `@Sources/Workspace.swift`:
- Around line 7750-7768: The background diagnostic block (and the other similar
blocks at the noted ranges) is emitting debug-only info via
GhosttyApp.shared.logBackground instead of the repo DEBUG path; wrap the whole
logging sequence in a compile-time guard and use the CMUX debug helper: enclose
the code that computes currentColorsDescription
(Self.bonsplitChromeColorsLogDescription), nextColorsDescription,
currentTabFont/nextTabFont, paneBackdrop (Self.usesBonsplitPaneTerminalBackdrop)
and the final log call in `#if` DEBUG / `#endif` and replace
GhosttyApp.shared.logBackground(...) with a single cmuxDebugLog("...") call that
formats the same message (including id.uuidString, reason, sharesWindowBackdrop,
isNoOp, etc.); apply the same change to the other occurrences you noted (lines
~7780-7792, 7811-7825, 7834-7841).
🪄 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: f937bb15-0a88-49b9-9fd6-e725a61bd5e9
📒 Files selected for processing (3)
Sources/CmuxConfig.swiftSources/Workspace.swiftvendor/bonsplit
✅ Files skipped from review due to trivial changes (1)
- vendor/bonsplit
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
Sources/AppDelegate.swift (1)
11939-11941:⚠️ Potential issue | 🟠 MajorDon’t consume
.newEditorwhen no pane can host it.
Workspace.newEditorSurfaceInFocusedPane()currently returnsnilwhenbonsplitController.focusedPaneIdis missing, but this branch still returnstrue. That turns the shortcut into a silent no-op in valid “no focused pane” states instead of falling back to another pane.Based on learnings: "In Sources/Workspace.swift, Workspace.openEditor(filePath:) must fall back to bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is nil so file-open requests aren’t dropped; it also reuses an existing editor if one is already open for the same file path."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 11939 - 11941, The .newEditor shortcut handler currently returns true even when newEditorSurfaceInFocusedPane(focus:) returns nil; change the handler to treat a nil result as failure (do not consume the shortcut) and instead attempt the fallback logic used in Workspace.openEditor(filePath:): resolve a target pane id using bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first and create/open the editor in that pane, and ensure Workspace.openEditor(filePath:) itself implements this fallback and reuses an existing editor for the same file path rather than creating a duplicate.Sources/Workspace.swift (2)
10700-10722:⚠️ Potential issue | 🟠 MajorDon’t drop editor-open requests when no pane is focused.
These paths still return
nilwhenfocusedPaneIdis absent even if the workspace has panes, andnewEditorSplitFromFocusedPanelalso loses the request whennewEditorSplit(...)fails instead of falling back to a surface create. That makes editor-open flows silently no-op from non-pane-backed focus states.Suggested fix
- let targetPaneId = (panelId ?? focusedPanelId).flatMap { paneId(forPanelId: $0) } - ?? bonsplitController.focusedPaneId + let targetPaneId = (panelId ?? focusedPanelId).flatMap { paneId(forPanelId: $0) } + ?? bonsplitController.focusedPaneId + ?? bonsplitController.allPaneIds.first @@ - if let paneId = bonsplitController.focusedPaneId, + if let paneId = bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first, let editorPanel = newEditorSurface( inPane: paneId, workspaceRootDirectory: rootDirectory, focus: true ) { @@ - guard let paneId = bonsplitController.focusedPaneId else { return nil } + guard let paneId = bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first else { + return nil + } @@ - guard let sourcePanelId = focusedPanelId else { + guard let paneId = bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first else { + return nil + } + guard let sourcePanelId = focusedPanelId else { return newEditorSurfaceInFocusedPane(focus: focus) } - return newEditorSplit( + if let panel = newEditorSplit( from: sourcePanelId, orientation: .horizontal, insertFirst: false, workspaceRootDirectory: directory, focus: focus - ) + ) { + return panel + } + return newEditorSurface(inPane: paneId, workspaceRootDirectory: directory, focus: focus)Based on learnings:
Workspace.openEditor(filePath:)must fall back tobonsplitController.allPaneIds.firstwhenfocusedPaneIdis nil so file-open requests aren’t dropped, and text-editor open flows should try split first then fall back to surface creation when the split fails.Also applies to: 10856-10878
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 10700 - 10722, Workspace.openEditor(filePath:) currently drops requests when bonsplitController.focusedPaneId is nil and when newEditorSplit(...) fails; change it to use bonsplitController.allPaneIds.first as a fallback targetPaneId when focusedPaneId is nil (i.e., compute targetPaneId = (panelId ?? focusedPanelId ?? bonsplitController.allPaneIds.first).flatMap { paneId(forPanelId: $0) } ?? bonsplitController.focusedPaneId) and update the split-then-create logic (used by newEditorSplitFromFocusedPanel / newEditorSplit(...)) so that if newEditorSplit(...) returns nil you then call newEditorSurface(inPane:..., workspaceRootDirectory:..., focus: true) as a fallback and continue to navigateToFile(filePath) before returning the editor panel.
10727-10746:⚠️ Potential issue | 🟠 MajorMake
openOrFocusEditorSplitactually create a split.When reuse misses, this helper opens a same-pane tab via
newEditorSurface(...), so callers asking for a split won’t get one. Its dedupe check also bypassescanonicalEditorPath(_:), which can still miss~/...and relative-path equivalents.Suggested fix
- let canonical = (filePath as NSString).resolvingSymlinksInPath + let canonical = canonicalEditorPath(filePath) for (existingId, panel) in panels { guard let ed = panel as? EditorPanel else { continue } - if (ed.filePath as NSString).resolvingSymlinksInPath == canonical { + if canonicalEditorPath(ed.filePath) == canonical { focusPanel(existingId) return ed } } - guard let paneId = paneId(forPanelId: panelId) ?? bonsplitController.focusedPaneId else { - return nil - } - return newEditorSurface( - inPane: paneId, + return newEditorSplit( + from: panelId, + orientation: .horizontal, + insertFirst: false, filePath: filePath, focus: true )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 10727 - 10746, openOrFocusEditorSplit currently opens a same-pane tab on a miss and compares file paths without using canonicalEditorPath(_:), so callers asking for a split never get one and dedupe can miss equivalents; update the function to (1) normalize paths using canonicalEditorPath(_:) (instead of manual resolvingSymlinksInPath) when comparing existing EditorPanel.filePath to detect duplicates, and (2) when no reuse is found, create a new split from the resolved paneId (use the existing paneId(forPanelId:) or bonsplitController.focusedPaneId) by invoking the controller/path that actually creates a split (e.g. call the bonsplitController split API or newEditorSurface variant that creates a split) so the new editor is opened in a new split rather than a same-pane tab.Sources/Panels/EditorPanel.swift (2)
439-452:⚠️ Potential issue | 🟠 Major
isDirtyearly-return blocks missing-file detection during external changes.When the file is deleted/renamed while dirty,
loadFileContent()exits before settingisFileUnavailable, so downstream reattach flow can take the wrong branch and recovery becomes unreliable.🐛 Proposed fix
private func loadFileContent() { guard !filePath.isEmpty else { return } - guard !isDirty else { return } + guard FileManager.default.fileExists(atPath: filePath) else { + isFileUnavailable = true + return + } + if isFileUnavailable { + isFileUnavailable = false + } + guard !isDirty else { return } let newContent: String do { newContent = try String(contentsOfFile: filePath, encoding: .utf8)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/EditorPanel.swift` around lines 439 - 452, The early-return on isDirty in loadFileContent prevents detecting missing/renamed files when the buffer is dirty; before returning for isDirty you must first check whether the file at filePath still exists (e.g., FileManager.default.fileExists(atPath:)) and if it does not set isFileUnavailable = true and return — or reorder the guards so existence is validated before the isDirty guard; update loadFileContent to use filePath, isDirty, and isFileUnavailable accordingly so external deletions are detected even when the editor is dirty.
166-169:⚠️ Potential issue | 🟡 Minor
Dockerfilelanguage mapping is unreachable viapathExtension.
monacoLanguageonly passespathExtension, butDockerfilehas no extension, so the"dockerfile"case in the extension switch never executes.♻️ Proposed fix
var monacoLanguage: String { guard !filePath.isEmpty else { return "plaintext" } + let basename = (filePath as NSString).lastPathComponent.lowercased() + if basename == "dockerfile" { + return "dockerfile" + } return Self.monacoLanguageFromExtension((filePath as NSString).pathExtension) }In Foundation, what does NSString.pathExtension return for a filename like "Dockerfile"?Also applies to: 624-624
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/EditorPanel.swift` around lines 166 - 169, monacoLanguage currently maps only on pathExtension so files like "Dockerfile" (no extension) never hit the "dockerfile" case; modify the logic in the monacoLanguage computed property (or inside Self.monacoLanguageFromExtension) to detect when pathExtension is empty and instead inspect the filename (e.g., lastPathComponent or (filePath as NSString).lastPathComponent) and map exact names like "Dockerfile" to "dockerfile"; update references to Self.monacoLanguageFromExtension((filePath as NSString).pathExtension) to pass the fallback filename or add a branch before calling it to cover known extensionless filenames.
🧹 Nitpick comments (1)
Sources/FileExplorerView.swift (1)
530-550: Single-click-to-open differs from standard file explorer UX.Calling
openFileInCodeVieweron every single click (line 549) is intentional per PR objectives, but users accustomed to click-to-select / double-click-to-open may find this surprising. If this is the desired behavior for the code viewer context, consider adding a brief comment explaining the design choice.📝 Suggested documentation
func performPrimaryClick(row: Int, in outlineView: NSOutlineView) { guard row >= 0, row < outlineView.numberOfRows, let node = outlineView.item(atRow: row) as? FileExplorerNode else { return } store.select(node: node) if node.isDirectory { if outlineView.isItemExpanded(node) { outlineView.collapseItem(node) store.collapse(node: node) } else { store.expand(node: node) outlineView.expandItem(node) } return } + // Design choice: single-click opens files immediately in code viewer, + // matching the "select to preview" pattern of the Monaco editor panel. openFileInCodeViewer(node.path) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/FileExplorerView.swift` around lines 530 - 550, In FileExplorerView.performPrimaryClick, the call to openFileInCodeViewer(node.path) triggers a single-click-to-open behavior that differs from standard click-to-select/double-click-to-open UX; add a concise comment above the openFileInCodeViewer call (and/or at the top of performPrimaryClick) stating this is an intentional single-click design for the code viewer context, referencing performPrimaryClick, openFileInCodeViewer, and FileExplorerNode so future readers understand the decision (or note that switching to double-click behavior would require handling NSOutlineView double-click actions instead).
🤖 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 6986-6997: The current logic falls back to
workspace.panels.values.first (dictionary-order dependent) which can paste into
the wrong terminal; change the selection-send target to use the editor's
recorded return terminal if available and otherwise bail. Concretely, in the
block that computes preferredTerminalId (currently using
workspace.focusedTerminalPanel?.id ?? workspace.panels.values.compactMap { ...
}.first) replace the fallback with the editor's recorded return-terminal
identifier (e.g., a property like editorPanel.recordedReturnTerminalId or
editorPanel.returnTerminalId) and if that property is nil, return false; keep
the rest of the flow (editorPanel.getSelection, tabManagerFor(tabId:),
sendTextWhenReady(selection,...)) unchanged so selection is only sent to a
deterministic terminal.
In `@Sources/FileExplorerView.swift`:
- Around line 1809-1818: Single-line summary: double-click triggers two opens
because mouseDown calls fileExplorerCoordinator?.performPrimaryClick(row:in:) on
clickCount == 1 and doubleAction also calls openSelectedFileFromOutline. Fix by
either removing the doubleAction assignment where the outline view's
doubleAction is set (so only mouseDown → performPrimaryClick opens files), or
implement a short debounce guard in the Coordinator/openSelectedFileFromOutline:
add a lastOpenedPath and lastOpenedAt timestamp on Coordinator, and in
openSelectedFileFromOutline check if the requested path equals lastOpenedPath
and (now - lastOpenedAt) < ~500ms then return no-op; otherwise update
lastOpenedPath/lastOpenedAt and proceed to open. Ensure references: mouseDown,
performPrimaryClick(row:in:), doubleAction, openSelectedFileFromOutline,
Coordinator, lastOpenedPath, lastOpenedAt.
In `@Sources/Workspace.swift`:
- Around line 14115-14121: executeSurfaceTabBarCommandButton currently seeds the
new editor's root from workspace-wide currentDirectory; change it to first try
the target pane's selected terminal working directory (derive from the PaneID's
selected terminal/session) and only then fall back to the workspace-level
currentDirectory or home directory. Locate the newEditor branch in
executeSurfaceTabBarCommandButton (and the analogous block later in the file
handling built-in editor entry points) and replace the directory resolution to
check the pane's selected terminal cwd, then normalizedSidebarDirectory(paneCwd)
?? normalizedSidebarDirectory(currentDirectory) ??
FileManager.default.homeDirectoryForCurrentUser.path before calling
newEditorSurface.
---
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 11939-11941: The .newEditor shortcut handler currently returns
true even when newEditorSurfaceInFocusedPane(focus:) returns nil; change the
handler to treat a nil result as failure (do not consume the shortcut) and
instead attempt the fallback logic used in Workspace.openEditor(filePath:):
resolve a target pane id using bonsplitController.focusedPaneId ??
bonsplitController.allPaneIds.first and create/open the editor in that pane, and
ensure Workspace.openEditor(filePath:) itself implements this fallback and
reuses an existing editor for the same file path rather than creating a
duplicate.
In `@Sources/Panels/EditorPanel.swift`:
- Around line 439-452: The early-return on isDirty in loadFileContent prevents
detecting missing/renamed files when the buffer is dirty; before returning for
isDirty you must first check whether the file at filePath still exists (e.g.,
FileManager.default.fileExists(atPath:)) and if it does not set
isFileUnavailable = true and return — or reorder the guards so existence is
validated before the isDirty guard; update loadFileContent to use filePath,
isDirty, and isFileUnavailable accordingly so external deletions are detected
even when the editor is dirty.
- Around line 166-169: monacoLanguage currently maps only on pathExtension so
files like "Dockerfile" (no extension) never hit the "dockerfile" case; modify
the logic in the monacoLanguage computed property (or inside
Self.monacoLanguageFromExtension) to detect when pathExtension is empty and
instead inspect the filename (e.g., lastPathComponent or (filePath as
NSString).lastPathComponent) and map exact names like "Dockerfile" to
"dockerfile"; update references to Self.monacoLanguageFromExtension((filePath as
NSString).pathExtension) to pass the fallback filename or add a branch before
calling it to cover known extensionless filenames.
In `@Sources/Workspace.swift`:
- Around line 10700-10722: Workspace.openEditor(filePath:) currently drops
requests when bonsplitController.focusedPaneId is nil and when
newEditorSplit(...) fails; change it to use bonsplitController.allPaneIds.first
as a fallback targetPaneId when focusedPaneId is nil (i.e., compute targetPaneId
= (panelId ?? focusedPanelId ?? bonsplitController.allPaneIds.first).flatMap {
paneId(forPanelId: $0) } ?? bonsplitController.focusedPaneId) and update the
split-then-create logic (used by newEditorSplitFromFocusedPanel /
newEditorSplit(...)) so that if newEditorSplit(...) returns nil you then call
newEditorSurface(inPane:..., workspaceRootDirectory:..., focus: true) as a
fallback and continue to navigateToFile(filePath) before returning the editor
panel.
- Around line 10727-10746: openOrFocusEditorSplit currently opens a same-pane
tab on a miss and compares file paths without using canonicalEditorPath(_:), so
callers asking for a split never get one and dedupe can miss equivalents; update
the function to (1) normalize paths using canonicalEditorPath(_:) (instead of
manual resolvingSymlinksInPath) when comparing existing EditorPanel.filePath to
detect duplicates, and (2) when no reuse is found, create a new split from the
resolved paneId (use the existing paneId(forPanelId:) or
bonsplitController.focusedPaneId) by invoking the controller/path that actually
creates a split (e.g. call the bonsplitController split API or newEditorSurface
variant that creates a split) so the new editor is opened in a new split rather
than a same-pane tab.
---
Nitpick comments:
In `@Sources/FileExplorerView.swift`:
- Around line 530-550: In FileExplorerView.performPrimaryClick, the call to
openFileInCodeViewer(node.path) triggers a single-click-to-open behavior that
differs from standard click-to-select/double-click-to-open UX; add a concise
comment above the openFileInCodeViewer call (and/or at the top of
performPrimaryClick) stating this is an intentional single-click design for the
code viewer context, referencing performPrimaryClick, openFileInCodeViewer, and
FileExplorerNode so future readers understand the decision (or note that
switching to double-click behavior would require handling NSOutlineView
double-click actions instead).
🪄 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: de7f06cd-7bb0-4646-acc7-61237e3b448d
📒 Files selected for processing (5)
Sources/AppDelegate.swiftSources/FileExplorerView.swiftSources/Panels/EditorPanel.swiftSources/TerminalController.swiftSources/Workspace.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/TerminalController.swift
Comment on lines
+6986
to
+6997
| let preferredTerminalId = workspace.focusedTerminalPanel?.id | ||
| ?? workspace.panels.values.compactMap { ($0 as? TerminalPanel)?.id }.first | ||
| guard let preferredTerminalId else { return false } | ||
|
|
||
| editorPanel.getSelection { [weak self, weak workspace] selection in | ||
| guard let self, | ||
| let workspace, | ||
| let selection, | ||
| !selection.isEmpty else { return } | ||
| self.tabManagerFor(tabId: workspace.id)?.focusTab(workspace.id, surfaceId: preferredTerminalId, suppressFlash: true) | ||
| self.sendTextWhenReady(selection, to: workspace, preferredPanelId: preferredTerminalId) | ||
| } |
There was a problem hiding this comment.
Avoid sending editor selection to an arbitrary terminal.
If focusedTerminalPanel is nil, workspace.panels.values.first is dictionary-order dependent, so this can paste into the wrong session. Prefer the editor’s recorded return terminal, or bail out when there isn’t a deterministic target.
💡 Minimal safe fix
- let preferredTerminalId = workspace.focusedTerminalPanel?.id
- ?? workspace.panels.values.compactMap { ($0 as? TerminalPanel)?.id }.first
- guard let preferredTerminalId else { return false }
+ guard let preferredTerminalId = workspace.focusedTerminalPanel?.id else {
+ return false
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 6986 - 6997, The current logic falls
back to workspace.panels.values.first (dictionary-order dependent) which can
paste into the wrong terminal; change the selection-send target to use the
editor's recorded return terminal if available and otherwise bail. Concretely,
in the block that computes preferredTerminalId (currently using
workspace.focusedTerminalPanel?.id ?? workspace.panels.values.compactMap { ...
}.first) replace the fallback with the editor's recorded return-terminal
identifier (e.g., a property like editorPanel.recordedReturnTerminalId or
editorPanel.returnTerminalId) and if that property is nil, return false; keep
the rest of the flow (editorPanel.getSelection, tabManagerFor(tabId:),
sendTextWhenReady(selection,...)) unchanged so selection is only sent to a
deterministic terminal.
Comment on lines
+1809
to
+1818
| override func mouseDown(with event: NSEvent) { | ||
| let point = convert(event.locationInWindow, from: nil) | ||
| let row = row(at: point) | ||
| let isDisclosureClick = row >= 0 && frameOfOutlineCell(atRow: row).contains(point) | ||
|
|
||
| super.mouseDown(with: event) | ||
|
|
||
| guard event.clickCount == 1, row >= 0, !isDisclosureClick else { return } | ||
| fileExplorerCoordinator?.performPrimaryClick(row: row, in: self) | ||
| } |
There was a problem hiding this comment.
Double-click on a file triggers two opens.
The mouseDown override calls performPrimaryClick on the first click (clickCount == 1), which opens the file. Then doubleAction fires openSelectedFileFromOutline, opening it again. While openOrFocusWorkspaceEditor may be idempotent (focusing an existing editor), this still performs redundant work.
Since single-click-to-open appears intentional (per PR objectives), consider removing the doubleAction assignment or making openSelectedFileFromOutline a no-op for the case where the file was already opened by the preceding single-click.
🔧 Option 1: Remove doubleAction entirely (single-click is sufficient)
outlineView.dataSource = coordinator
outlineView.delegate = coordinator
outlineView.target = coordinator
- outlineView.doubleAction = `#selector`(FileExplorerPanelView.Coordinator.openSelectedFileFromOutline(_:))
coordinator.outlineView = outlineView🔧 Option 2: Guard against double-open by tracking last-opened path
In Coordinator, track the last-opened path and timestamp, then skip re-opening within a short debounce window.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/FileExplorerView.swift` around lines 1809 - 1818, Single-line
summary: double-click triggers two opens because mouseDown calls
fileExplorerCoordinator?.performPrimaryClick(row:in:) on clickCount == 1 and
doubleAction also calls openSelectedFileFromOutline. Fix by either removing the
doubleAction assignment where the outline view's doubleAction is set (so only
mouseDown → performPrimaryClick opens files), or implement a short debounce
guard in the Coordinator/openSelectedFileFromOutline: add a lastOpenedPath and
lastOpenedAt timestamp on Coordinator, and in openSelectedFileFromOutline check
if the requested path equals lastOpenedPath and (now - lastOpenedAt) < ~500ms
then return no-op; otherwise update lastOpenedPath/lastOpenedAt and proceed to
open. Ensure references: mouseDown, performPrimaryClick(row:in:), doubleAction,
openSelectedFileFromOutline, Coordinator, lastOpenedPath, lastOpenedAt.
Comment on lines
14115
to
+14121
| private func executeSurfaceTabBarCommandButton(identifier: String, inPane pane: PaneID) { | ||
| if CmuxSurfaceTabBarBuiltInAction(configID: identifier) == .newEditor { | ||
| let directory = normalizedSidebarDirectory(currentDirectory) | ||
| ?? FileManager.default.homeDirectoryForCurrentUser.path | ||
| _ = newEditorSurface(inPane: pane, workspaceRootDirectory: directory, focus: true) | ||
| return | ||
| } |
There was a problem hiding this comment.
Resolve the new editor root from the target pane, not currentDirectory.
Both built-in editor entry points always seed the panel from the workspace-wide focused directory. If the action is invoked on a different pane, the editor can open on the wrong folder.
Suggested fix
+ let directory =
+ selectedTerminalPanel(inPane: pane).flatMap { terminal in
+ [panelDirectories[terminal.id], terminal.requestedWorkingDirectory]
+ .compactMap { normalizedSidebarDirectory($0) }
+ .first
+ }
+ ?? normalizedSidebarDirectory(currentDirectory)
+ ?? FileManager.default.homeDirectoryForCurrentUser.path
+
if CmuxSurfaceTabBarBuiltInAction(configID: identifier) == .newEditor {
- let directory = normalizedSidebarDirectory(currentDirectory)
- ?? FileManager.default.homeDirectoryForCurrentUser.path
_ = newEditorSurface(inPane: pane, workspaceRootDirectory: directory, focus: true)
return
}
@@
case "editor":
- let directory = normalizedSidebarDirectory(currentDirectory)
- ?? FileManager.default.homeDirectoryForCurrentUser.path
_ = newEditorSurface(inPane: pane, workspaceRootDirectory: directory, focus: true)Based on learnings: executeSurfaceTabBarCommandButton(identifier:inPane:) should derive its base cwd from the selected terminal in the target pane before falling back to workspace state.
Also applies to: 14193-14203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 14115 - 14121,
executeSurfaceTabBarCommandButton currently seeds the new editor's root from
workspace-wide currentDirectory; change it to first try the target pane's
selected terminal working directory (derive from the PaneID's selected
terminal/session) and only then fall back to the workspace-level
currentDirectory or home directory. Locate the newEditor branch in
executeSurfaceTabBarCommandButton (and the analogous block later in the file
handling built-in editor entry points) and replace the directory resolution to
check the pane's selected terminal cwd, then normalizedSidebarDirectory(paneCwd)
?? normalizedSidebarDirectory(currentDirectory) ??
FileManager.default.homeDirectoryForCurrentUser.path before calling
newEditorSurface.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
Resources/editor-monaco/editor-bridge.js (1)
36-62:⚠️ Potential issue | 🟠 MajorKeep the normal Monaco instance read-only.
Line 41 and Line 61 still opt the non-diff editor into editing, so this panel can mutate content even though the PR is scoped as a read-only editor. That also leaves
contentChanged/saveRequestedreachable from local edits in normal mode.♻️ Proposed fix
function editorOptions(value, language) { return { value: value || "", language: language || "plaintext", theme: detectTheme(), - readOnly: false, + readOnly: true, automaticLayout: true, @@ overviewRulerLanes: 0, hideCursorInOverviewRuler: true, contextmenu: false, - domReadOnly: false, + domReadOnly: true, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/editor-monaco/editor-bridge.js` around lines 36 - 62, The editorOptions function currently sets the normal (non-diff) Monaco instance to editable by using readOnly: false and domReadOnly: false; update editorOptions to make the normal editor truly read-only by setting both readOnly and domReadOnly to true (so local edits cannot mutate content or trigger contentChanged/saveRequested), ensuring the diff mode behavior is unchanged and only the non-diff panel is locked down.Sources/AppDelegate.swift (2)
5934-5990:⚠️ Potential issue | 🟠 MajorDon't silently drop explicit file-open requests.
Both paths discard a
nilresult fromopenOrFocusWorkspaceEditor(...), so an external open or file-explorer click can no-op when split placement fails or there is no usable focused surface. Please fall back to the workspace-level open path here instead of treating failure as success.Based on learnings: "In Sources/Workspace.swift, Workspace.openEditor(filePath:) must fall back to bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is nil so file-open requests aren’t dropped; it also reuses an existing editor if one is already open for the same file path."
Also applies to: 6967-6978
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 5934 - 5990, preferredWorkspaceForExternalFile/openFileForExternalURL currently treat a nil return from workspace.openOrFocusWorkspaceEditor(...) as success and drop the open request; change openFileForExternalURL to detect when openOrFocusWorkspaceEditor returns nil and fall back to the workspace-level open (e.g., call Workspace.openEditor(filePath:) which itself should use bonsplitController.allPaneIds.first when focusedPaneId is nil) and then focus that returned editor panel via context.tabManager.focusTab(..., surfaceId: returnedPanel.id, ...); reference workspace.openOrFocusWorkspaceEditor, Workspace.openEditor(filePath:), and bonsplitController.focusedPaneId/allPaneIds to find where to add the fallback.
6990-6992:⚠️ Potential issue | 🟠 MajorAvoid dictionary-order terminal fallback here.
If no terminal is focused, Line 6990 falls back to the first terminal in
workspace.panels.values, which is not a deterministic return target. Use the editor’s recorded return-terminal id, or bail out instead of pasting into an arbitrary session.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6990 - 6992, Replace the nondeterministic fallback to workspace.panels.values by first checking the editor’s recorded return-terminal id: instead of computing preferredTerminalId as workspace.focusedTerminalPanel?.id ?? workspace.panels.values.compactMap { ($0 as? TerminalPanel)?.id }.first, query the editor's stored return-terminal identifier (e.g., editor.returnTerminalId or the API that holds the last-return-terminal) and use that if present; only if that recorded id is nil should you bail out (return false). Update the code paths using preferredTerminalId, referencing workspace.focusedTerminalPanel, the editor’s recorded return-terminal id, and TerminalPanel.id so the selection is deterministic and avoids dictionary-order fallbacks.Sources/FileExplorerView.swift (1)
720-726:⚠️ Potential issue | 🟠 MajorDouble-click still opens the same file twice.
The first click already goes through
mouseDown→performPrimaryClick(...)→openFileInCodeViewer(...). KeepingdoubleActionwired toopenSelectedFileFromOutline(_:)means AppKit opens it again on the second click. If single-click-to-open is the intended behavior,doubleActionshould be removed or explicitly deduped.Minimal fix
outlineView.dataSource = coordinator outlineView.delegate = coordinator outlineView.target = coordinator - outlineView.doubleAction = `#selector`(FileExplorerPanelView.Coordinator.openSelectedFileFromOutline(_:)) coordinator.outlineView = outlineViewAlso applies to: 1254-1255, 2195-2204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/FileExplorerView.swift` around lines 720 - 726, The outline view's doubleAction is causing files to open twice because openSelectedFileFromOutline(_:) is invoked on both mouseDown->performPrimaryClick->openFileInCodeViewer and again by the doubleAction; update openSelectedFileFromOutline(_:) to ignore double-clicks by checking the current event's clickCount (e.g., guard sender.window?.currentEvent?.clickCount == 1 else { return }) before calling openFileInCodeViewer(node.path), or remove the outline view's doubleAction wiring; reference symbols: openSelectedFileFromOutline(_:), performPrimaryClick(...), openFileInCodeViewer(...), and doubleAction.Sources/Workspace.swift (3)
14122-14128:⚠️ Potential issue | 🟡 MinorResolve the new editor root from the target pane, not
currentDirectory.Both entry points seed the editor from the workspace-global cwd, so invoking the action on another pane can open the explorer on the wrong folder.
♻️ Suggested fix
+ let directory = + selectedTerminalPanel(inPane: pane).flatMap { terminal in + [panelDirectories[terminal.id], terminal.requestedWorkingDirectory] + .compactMap { normalizedSidebarDirectory($0) } + .first + } + ?? normalizedSidebarDirectory(currentDirectory) + ?? FileManager.default.homeDirectoryForCurrentUser.path + if CmuxSurfaceTabBarBuiltInAction(configID: identifier) == .newEditor { - let directory = normalizedSidebarDirectory(currentDirectory) - ?? FileManager.default.homeDirectoryForCurrentUser.path _ = newEditorSurface(inPane: pane, workspaceRootDirectory: directory, focus: true) return } @@ case "editor": - let directory = normalizedSidebarDirectory(currentDirectory) - ?? FileManager.default.homeDirectoryForCurrentUser.path _ = newEditorSurface(inPane: pane, workspaceRootDirectory: directory, focus: true)Based on learnings:
executeSurfaceTabBarCommandButton(identifier:inPane:) must focus the target pane and apply its selected tab, and compute baseCwd from that pane’s selected terminal (panelDirectories[...] or requestedWorkingDirectory, falling back to workspace.currentDirectory) before invoking CmuxConfigExecutor.execute for workspaceCommand actions.Also applies to: 14206-14209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 14122 - 14128, The handler executeSurfaceTabBarCommandButton uses the global currentDirectory when creating a new editor; change it to compute the base CWD from the target pane’s selected terminal before executing actions: focus the target pane and apply its selected tab, determine baseCwd by checking panelDirectories[pane] (or using the tab/terminal’s requestedWorkingDirectory) and fall back to workspace.currentDirectory only if neither is available, then pass that baseCwd into newEditorSurface/CmuxConfigExecutor.execute for workspaceCommand actions (also update the similar logic at the other occurrence around lines 14206–14209).
10734-10753:⚠️ Potential issue | 🟡 MinorUse
canonicalEditorPath(_:)for this reuse check too.This still only resolves symlinks, so
~/foo.swift,./foo.swift, and an already-standardized absolute path can miss each other and open duplicate editor tabs.♻️ Suggested fix
- let canonical = (filePath as NSString).resolvingSymlinksInPath + let canonical = canonicalEditorPath(filePath) for (existingId, panel) in panels { guard let ed = panel as? EditorPanel else { continue } - if (ed.filePath as NSString).resolvingSymlinksInPath == canonical { + if canonicalEditorPath(ed.filePath) == canonical { focusPanel(existingId) return ed } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 10734 - 10753, openOrFocusEditorSplit currently compares paths using resolvingSymlinksInPath which misses differences like "~" or "./"; replace that reuse check to call canonicalEditorPath(_:) for both the incoming filePath and each ed.filePath so the comparison uses the same normalized canonical form, then continue to focus existing panel or call newEditorSurface as before (keep paneId resolution via paneId(forPanelId:) ?? bonsplitController.focusedPaneId and existing focusPanel(existingId) usage).
10863-10885:⚠️ Potential issue | 🟠 MajorKeep the same split→surface fallback here.
These helpers still return
nilwhen there is nofocusedPaneId, andnewEditorSplitFromFocusedPanelstill drops the request ifnewEditorSplit(...)fails. That reintroduces the silent no-op path from non-pane-backed focus states.♻️ Suggested fix
`@discardableResult` func newEditorSurfaceInFocusedPane(focus: Bool = true) -> EditorPanel? { - guard let paneId = bonsplitController.focusedPaneId else { return nil } + guard let paneId = bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first else { + return nil + } let directory = normalizedSidebarDirectory(currentDirectory) ?? FileManager.default.homeDirectoryForCurrentUser.path return newEditorSurface(inPane: paneId, workspaceRootDirectory: directory, focus: focus) } `@discardableResult` func newEditorSplitFromFocusedPanel(focus: Bool = true) -> EditorPanel? { let directory = normalizedSidebarDirectory(currentDirectory) ?? FileManager.default.homeDirectoryForCurrentUser.path + guard let paneId = bonsplitController.focusedPaneId ?? bonsplitController.allPaneIds.first else { + return nil + } guard let sourcePanelId = focusedPanelId else { return newEditorSurfaceInFocusedPane(focus: focus) } - return newEditorSplit( + if let panel = newEditorSplit( from: sourcePanelId, orientation: .horizontal, insertFirst: false, workspaceRootDirectory: directory, focus: focus - ) + ) { + return panel + } + return newEditorSurface(inPane: paneId, workspaceRootDirectory: directory, focus: focus) }Based on learnings:
Workspace.openEditor(filePath:) must fall back to bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is nil so file-open requests aren’t dropped, andopenFileInTextEditor(_:) must attempt ... split ... and, if that returns nil, fall back to ... newTextEditorSurface(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 10863 - 10885, newEditorSurfaceInFocusedPane and newEditorSplitFromFocusedPanel currently drop requests when focusedPaneId is nil or when newEditorSplit(...) returns nil, reintroducing a silent no-op; update newEditorSurfaceInFocusedPane to fall back to bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is nil, and update newEditorSplitFromFocusedPanel to, when focusedPanelId is nil, call the same surface fallback (newEditorSurfaceInFocusedPane) and when newEditorSplit(from:...) returns nil, fall back to creating a surface via newEditorSurface(...) (same parameters) so split failures don’t silently drop the request—also ensure openEditor(filePath:) uses bonsplitController.allPaneIds.first when focusedPaneId is nil and openFileInTextEditor(_:) tries newEditorSplit(...) then falls back to newTextEditorSurface(...) on nil.
🧹 Nitpick comments (1)
Sources/Panels/EditorPanel.swift (1)
661-662: Consider adding Vue/Svelte language detection (optional).Currently
.vueand.sveltefiles fall back to"html". Monaco has built-in TypeScript/JavaScript mode support that could provide better highlighting for the script sections. If Monaco is bundled with vue/svelte language support, consider returning those identifiers instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Panels/EditorPanel.swift` around lines 661 - 662, The switch currently maps 'case "vue": return "html"' and 'case "svelte": return "html"' which forces HTML highlighting; change those cases to return the Monaco language identifiers "vue" and "svelte" when those modes are available (or, if you prefer more precise highlighting, detect script blocks (e.g., <script lang="ts">) in the file content and return "typescript" or "javascript" accordingly), updating the switch that handles file-extension-to-language resolution so .vue/.svelte files use the proper Monaco language id instead of falling back to "html".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/editor-monaco/editor-bridge.js`:
- Around line 28-34: The theme override from Swift isn't persisted, so
detectTheme(), editorOptions(), and diff editor creation always use system
preference; introduce a module-level variable (e.g., overriddenTheme or
selectedTheme) and update it inside setTheme() so setTheme() both applies the
theme to the current editor and stores the choice; change detectTheme() to
return the overridden theme when set (falling back to prefers-color-scheme if
not), update editorOptions() and the diff editor creation logic to call
detectTheme() (or read the selectedTheme) so newly created editors honor the
Swift override, and modify the media-query listener to only update theme when no
overriddenTheme is set.
In `@Sources/FileExplorerView.swift`:
- Around line 130-186: The background file-tree walk started by
search(query:rootPath:isLocal:) must be made cooperatively cancellable: update
Self.collectFiles(query:rootPath:maxResults:) to accept a cancellation callback
or token (e.g., add a parameter cancelIf: () -> Bool or a CancellationToken) and
make collectFiles periodically call it and throw a CancellationError if it
returns true; in search(...) capture the current searchGeneration and pass a
cancelIf closure that returns self.generation != searchGeneration so the
in-flight scan aborts when generation is bumped (also apply the same pattern to
the other call sites noted around lines 189-196 and 207-270); keep
cancel(clear:) logic as-is (it still increments generation) so existing cancel
behavior triggers the new cooperative cancellation.
- Around line 1650-1666: The status strings in the switch (.searching, .matches,
.limited) are using a single "%d files" key and will render "1 files"; update
the localization to use ICU-style plural entries (add .one and .other forms for
keys fileExplorer.search.filesSearching, fileExplorer.search.filesMatches and
fileExplorer.search.filesLimit in the .stringsdict), then change the string
construction to pass the count to the localized lookup (keep using the same call
sites in FileExplorerView.swift — the cases for .searching, .matches and
.limited — but supply snapshot.results.count or limit as the count argument so
the pluralized .one/.other forms are chosen at runtime).
In `@Sources/Workspace.swift`:
- Around line 10707-10729: The current logic in Workspace.openEditor(filePath:)
computes targetPaneId and tries
newEditorSurface(inPane:workspaceRootDirectory:focus:) using
bonsplitController.focusedPaneId, but if focusedPaneId is nil it returns nil
even when other panes exist; change the fallback so that when
bonsplitController.focusedPaneId is nil you use
bonsplitController.allPaneIds.first as the pane to create the editor.
Concretely, ensure the targetPaneId resolution (the let targetPaneId / second
if-let branch) falls back to bonsplitController.allPaneIds.first before
returning nil, and then call newEditorSurface(inPane: ..., focus: true) and
navigateToFile(filePath) as shown.
---
Duplicate comments:
In `@Resources/editor-monaco/editor-bridge.js`:
- Around line 36-62: The editorOptions function currently sets the normal
(non-diff) Monaco instance to editable by using readOnly: false and domReadOnly:
false; update editorOptions to make the normal editor truly read-only by setting
both readOnly and domReadOnly to true (so local edits cannot mutate content or
trigger contentChanged/saveRequested), ensuring the diff mode behavior is
unchanged and only the non-diff panel is locked down.
In `@Sources/AppDelegate.swift`:
- Around line 5934-5990:
preferredWorkspaceForExternalFile/openFileForExternalURL currently treat a nil
return from workspace.openOrFocusWorkspaceEditor(...) as success and drop the
open request; change openFileForExternalURL to detect when
openOrFocusWorkspaceEditor returns nil and fall back to the workspace-level open
(e.g., call Workspace.openEditor(filePath:) which itself should use
bonsplitController.allPaneIds.first when focusedPaneId is nil) and then focus
that returned editor panel via context.tabManager.focusTab(..., surfaceId:
returnedPanel.id, ...); reference workspace.openOrFocusWorkspaceEditor,
Workspace.openEditor(filePath:), and bonsplitController.focusedPaneId/allPaneIds
to find where to add the fallback.
- Around line 6990-6992: Replace the nondeterministic fallback to
workspace.panels.values by first checking the editor’s recorded return-terminal
id: instead of computing preferredTerminalId as
workspace.focusedTerminalPanel?.id ?? workspace.panels.values.compactMap { ($0
as? TerminalPanel)?.id }.first, query the editor's stored return-terminal
identifier (e.g., editor.returnTerminalId or the API that holds the
last-return-terminal) and use that if present; only if that recorded id is nil
should you bail out (return false). Update the code paths using
preferredTerminalId, referencing workspace.focusedTerminalPanel, the editor’s
recorded return-terminal id, and TerminalPanel.id so the selection is
deterministic and avoids dictionary-order fallbacks.
In `@Sources/FileExplorerView.swift`:
- Around line 720-726: The outline view's doubleAction is causing files to open
twice because openSelectedFileFromOutline(_:) is invoked on both
mouseDown->performPrimaryClick->openFileInCodeViewer and again by the
doubleAction; update openSelectedFileFromOutline(_:) to ignore double-clicks by
checking the current event's clickCount (e.g., guard
sender.window?.currentEvent?.clickCount == 1 else { return }) before calling
openFileInCodeViewer(node.path), or remove the outline view's doubleAction
wiring; reference symbols: openSelectedFileFromOutline(_:),
performPrimaryClick(...), openFileInCodeViewer(...), and doubleAction.
In `@Sources/Workspace.swift`:
- Around line 14122-14128: The handler executeSurfaceTabBarCommandButton uses
the global currentDirectory when creating a new editor; change it to compute the
base CWD from the target pane’s selected terminal before executing actions:
focus the target pane and apply its selected tab, determine baseCwd by checking
panelDirectories[pane] (or using the tab/terminal’s requestedWorkingDirectory)
and fall back to workspace.currentDirectory only if neither is available, then
pass that baseCwd into newEditorSurface/CmuxConfigExecutor.execute for
workspaceCommand actions (also update the similar logic at the other occurrence
around lines 14206–14209).
- Around line 10734-10753: openOrFocusEditorSplit currently compares paths using
resolvingSymlinksInPath which misses differences like "~" or "./"; replace that
reuse check to call canonicalEditorPath(_:) for both the incoming filePath and
each ed.filePath so the comparison uses the same normalized canonical form, then
continue to focus existing panel or call newEditorSurface as before (keep paneId
resolution via paneId(forPanelId:) ?? bonsplitController.focusedPaneId and
existing focusPanel(existingId) usage).
- Around line 10863-10885: newEditorSurfaceInFocusedPane and
newEditorSplitFromFocusedPanel currently drop requests when focusedPaneId is nil
or when newEditorSplit(...) returns nil, reintroducing a silent no-op; update
newEditorSurfaceInFocusedPane to fall back to
bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is
nil, and update newEditorSplitFromFocusedPanel to, when focusedPanelId is nil,
call the same surface fallback (newEditorSurfaceInFocusedPane) and when
newEditorSplit(from:...) returns nil, fall back to creating a surface via
newEditorSurface(...) (same parameters) so split failures don’t silently drop
the request—also ensure openEditor(filePath:) uses
bonsplitController.allPaneIds.first when focusedPaneId is nil and
openFileInTextEditor(_:) tries newEditorSplit(...) then falls back to
newTextEditorSurface(...) on nil.
---
Nitpick comments:
In `@Sources/Panels/EditorPanel.swift`:
- Around line 661-662: The switch currently maps 'case "vue": return "html"' and
'case "svelte": return "html"' which forces HTML highlighting; change those
cases to return the Monaco language identifiers "vue" and "svelte" when those
modes are available (or, if you prefer more precise highlighting, detect script
blocks (e.g., <script lang="ts">) in the file content and return "typescript" or
"javascript" accordingly), updating the switch that handles
file-extension-to-language resolution so .vue/.svelte files use the proper
Monaco language id instead of falling back to "html".
🪄 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: e7ee075e-0029-4e17-a38d-6a8f483dabf1
📒 Files selected for processing (9)
Resources/Localizable.xcstringsResources/editor-monaco/editor-bridge.jsSources/AppDelegate.swiftSources/ContentView.swiftSources/FileExplorerView.swiftSources/Panels/EditorPanel.swiftSources/Panels/EditorPanelView.swiftSources/RightSidebarPanelView.swiftSources/Workspace.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/ContentView.swift
- Resources/Localizable.xcstrings
- Sources/Panels/EditorPanelView.swift
Comment on lines
+28
to
+34
| // Detect dark mode from prefers-color-scheme or Swift override. | ||
| function detectTheme() { | ||
| return window.matchMedia && | ||
| window.matchMedia("(prefers-color-scheme: dark)").matches | ||
| ? "vs-dark" | ||
| : "vs"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f "editor-bridge.js"Repository: manaflow-ai/cmux
Length of output: 231
🏁 Script executed:
fd --type file "editor-bridge.js"Repository: manaflow-ai/cmux
Length of output: 101
🏁 Script executed:
cat -n "Resources/editor-monaco/editor-bridge.js"Repository: manaflow-ai/cmux
Length of output: 10143
Persist Swift theme override across editor recreation.
setTheme() at line 265 updates the current editor but doesn't affect newly created editors. Both editorOptions() (used at lines 40, 167) and the diff editor creation at line 189 call detectTheme(), which always returns the system preference. The media-query listener at lines 141–149 also unconditionally reapplies the system theme. After calling setTheme() from Swift, reopening content or switching into diff mode snaps back to prefers-color-scheme instead of respecting the native selection.
♻️ Proposed fix
- // Detect dark mode from prefers-color-scheme or Swift override.
- function detectTheme() {
+ function detectSystemTheme() {
return window.matchMedia &&
window.matchMedia("(prefers-color-scheme: dark)").matches
? "vs-dark"
: "vs";
}
+
+ var currentTheme = detectSystemTheme();
+ var followsSystemTheme = true;
@@
theme: detectTheme(),
+ theme: currentTheme,
@@
- var theme = e.matches ? "vs-dark" : "vs";
- monaco.editor.setTheme(theme);
+ if (followsSystemTheme) {
+ currentTheme = e.matches ? "vs-dark" : "vs";
+ monaco.editor.setTheme(currentTheme);
+ }
});
@@
- theme: detectTheme(),
+ theme: currentTheme,
@@
// Set theme explicitly from Swift.
setTheme: function (isDark) {
- monaco.editor.setTheme(isDark ? "vs-dark" : "vs");
+ followsSystemTheme = false;
+ currentTheme = isDark ? "vs-dark" : "vs";
+ monaco.editor.setTheme(currentTheme);
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resources/editor-monaco/editor-bridge.js` around lines 28 - 34, The theme
override from Swift isn't persisted, so detectTheme(), editorOptions(), and diff
editor creation always use system preference; introduce a module-level variable
(e.g., overriddenTheme or selectedTheme) and update it inside setTheme() so
setTheme() both applies the theme to the current editor and stores the choice;
change detectTheme() to return the overridden theme when set (falling back to
prefers-color-scheme if not), update editorOptions() and the diff editor
creation logic to call detectTheme() (or read the selectedTheme) so newly
created editors honor the Swift override, and modify the media-query listener to
only update theme when no overriddenTheme is set.
Comment on lines
+10707
to
+10729
| let targetPaneId = (panelId ?? focusedPanelId).flatMap { paneId(forPanelId: $0) } | ||
| ?? bonsplitController.focusedPaneId | ||
| if let targetPaneId, | ||
| let editorPanel = newEditorSurface( | ||
| inPane: targetPaneId, | ||
| workspaceRootDirectory: rootDirectory, | ||
| focus: true | ||
| ) { | ||
| _ = editorPanel.navigateToFile(filePath) | ||
| return finish(editorPanel) | ||
| } | ||
|
|
||
| if let paneId = bonsplitController.focusedPaneId, | ||
| let editorPanel = newEditorSurface( | ||
| inPane: paneId, | ||
| workspaceRootDirectory: rootDirectory, | ||
| focus: true | ||
| ) { | ||
| _ = editorPanel.navigateToFile(filePath) | ||
| return finish(editorPanel) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Fall back to the first pane instead of dropping editor opens.
If neither the source panel nor focusedPaneId is available, this path returns nil even when the workspace still has panes. That makes file-open requests silently no-op from unfocused or non-pane-backed states.
♻️ Suggested fix
- let targetPaneId = (panelId ?? focusedPanelId).flatMap { paneId(forPanelId: $0) }
- ?? bonsplitController.focusedPaneId
+ let targetPaneId = (panelId ?? focusedPanelId).flatMap { paneId(forPanelId: $0) }
+ ?? bonsplitController.focusedPaneId
+ ?? bonsplitController.allPaneIds.first
if let targetPaneId,
let editorPanel = newEditorSurface(
inPane: targetPaneId,
workspaceRootDirectory: rootDirectory,
focus: true
) {
_ = editorPanel.navigateToFile(filePath)
return finish(editorPanel)
}
-
- if let paneId = bonsplitController.focusedPaneId,
- let editorPanel = newEditorSurface(
- inPane: paneId,
- workspaceRootDirectory: rootDirectory,
- focus: true
- ) {
- _ = editorPanel.navigateToFile(filePath)
- return finish(editorPanel)
- }
return nilBased on learnings: Workspace.openEditor(filePath:) must fall back to bonsplitController.allPaneIds.first when bonsplitController.focusedPaneId is nil so file-open requests aren’t dropped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 10707 - 10729, The current logic in
Workspace.openEditor(filePath:) computes targetPaneId and tries
newEditorSurface(inPane:workspaceRootDirectory:focus:) using
bonsplitController.focusedPaneId, but if focusedPaneId is nil it returns nil
even when other panes exist; change the fallback so that when
bonsplitController.focusedPaneId is nil you use
bonsplitController.allPaneIds.first as the pane to create the editor.
Concretely, ensure the targetPaneId resolution (the let targetPaneId / second
if-let branch) falls back to bonsplitController.allPaneIds.first before
returning nil, and then call newEditorSurface(inPane: ..., focus: true) and
navigateToFile(filePath) as shown.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Jiiiin
changed the title
Jiiiin
changed the title
teamleaderleo
added
S2: major
This branch has not been 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
2 participants
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
This PR adds a Files tab / Monaco editor workflow for browsing and editing project files inside cmux.
The original version of this PR was closer to a standalone Monaco editor / split action. After iteration, the scope shifted toward a browser-like file tab experience:
Filesas a first-class Bonsplit tab action, alongside Terminal / Browser / Split Right / Split Down.Difference from #2864
#2864 is a fuller text-editor workflow for opening and editing files from the file explorer.
This PR is currently focused on a dedicated Files-tab workflow:
If #2864 is the preferred long-term direction, I am happy to align this PR or extract only the useful pieces.
Testing
./scripts/reload.sh --tag fix-search-sidebar --launch