Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a configurable setting for how inline VS Code opens (right, left, bottom, top, or tab) with config parsing, UserDefaults persistence, Settings UI, ContentView open logic changes, enum/settings types, localization (en/ja), and accompanying tests. Changes
Sequence DiagramsequenceDiagram
actor User
participant Settings as "Settings UI\n(SettingsPickerRow)"
participant App as "AppStorage / cmuxApp"
participant Content as "ContentView"
participant Config as "VSCodeInlineSplitDirectionSettings"
participant TabMgr as "TabManager"
participant Browser as "BrowserPanel"
User->>Settings: choose direction (right/left/bottom/top/tab)
Settings->>App: write raw value to AppStorage
User->>Content: trigger Open Current Directory in VS Code (Inline)
Content->>Config: resolve current direction (AppStorage/UserDefaults/config)
Config-->>Content: VSCodeInlineSplitDirection
alt direction maps to a SplitDirection
Content->>TabMgr: newBrowserSplit(orientation, insertFirst)
else direction is tab (no SplitDirection)
Content->>TabMgr: newBrowserSurface(inPane: resolvedPaneId)
end
TabMgr->>Browser: create/display browser surface (VS Code)
Browser-->>User: VS Code panel shown in configured position
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR adds a configurable Key changes:
Issue found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[openFocusedDirectoryInInlineVSCode] --> B{guard: appURL\n+ workspace\n+ sourcePanelId\n+ sourcePaneId?}
B -- nil --> C[return false]
B -- ok --> D[resolve vscodeInlineSplitDirection\nfrom @AppStorage]
D --> E{ensureServeWebURL}
E -- failure --> F[NSSound.beep]
E -- success --> G{openDirection\n.splitDirection?}
G -- non-nil\nright/left/top/bottom --> H[newBrowserSplit\nfromPanelId: sourcePanelId\norientation + insertFirst]
G -- nil\ntab --> I[newBrowserSurface\ninPane: sourcePaneId\nfocus: true]
H --> J{createdPanelId != nil?}
I --> J
J -- nil --> F
J -- ok --> K[return true]
subgraph Settings Load
L[GhosttyConfig.loadFromDisk] --> M[parse config file\nvscode-inline-split-direction]
M --> N[applyVSCodeInlineSplitDirectionToUserDefaults]
N --> O[UserDefaults\nvscodeInlineSplitDirection]
end
subgraph Settings UI
P[SettingsView @AppStorage] --> Q[SettingsPickerRow\nright/left/bottom/top/tab]
Q --> O
end
O --> D
Reviews (1): Last reviewed commit: "fix: make inline vscode placement config..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
7362
to
7368
| private func openFocusedDirectoryInInlineVSCode(_ directoryURL: URL) -> Bool { | ||
| guard let vscodeApplicationURL = TerminalDirectoryOpenTarget.vscodeInline.applicationURL(), | ||
| let workspace = tabManager.selectedWorkspace, | ||
| let sourcePanelId = workspace.focusedPanelId else { | ||
| let sourcePanelId = workspace.focusedPanelId, | ||
| let sourcePaneId = workspace.paneId(forPanelId: sourcePanelId) else { | ||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
sourcePaneId required unconditionally but only used in the tab branch
sourcePaneId is resolved in the guard clause and will cause an early false return if paneId(forPanelId:) comes back nil — but it is only consumed in the .tab branch below. For any of the four split directions (right, left, bottom, top), the function will silently fail via NSSound.beep() if paneId(forPanelId:) ever returns nil (e.g. because surfaceIdFromPanelId has no entry for the panel), even though those code paths never touch sourcePaneId.
This is a behavioural regression from the previous code, which only required sourcePanelId and had no dependency on pane lookup.
Consider resolving sourcePaneId lazily, only when the tab branch is actually taken:
guard let vscodeApplicationURL = TerminalDirectoryOpenTarget.vscodeInline.applicationURL(),
let workspace = tabManager.selectedWorkspace,
let sourcePanelId = workspace.focusedPanelId else {
return false
}
let sourceTabId = workspace.id
let openDirection = vscodeInlineSplitDirection
let tabManager = tabManager
VSCodeServeWebController.shared.ensureServeWebURL(vscodeApplicationURL: vscodeApplicationURL) { serveWebURL in
// ...
let createdPanelId: UUID? = {
if let splitDirection = openDirection.splitDirection {
return tabManager.newBrowserSplit(
tabId: sourceTabId,
fromPanelId: sourcePanelId,
orientation: splitDirection.orientation,
insertFirst: splitDirection.insertFirst,
url: openFolderURL,
focus: true
)
}
guard let sourcePaneId = workspace.paneId(forPanelId: sourcePanelId) else {
NSSound.beep()
return nil
}
return tabManager.newBrowserSurface(
tabId: sourceTabId,
inPane: sourcePaneId,
url: openFolderURL,
focus: true
)
}()
// ...
}![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
7363-7399:⚠️ Potential issue | 🟠 MajorAvoid making every placement depend on
paneId(forPanelId:).Line 7366 now short-circuits the whole action when the focused panel is temporarily not pane-backed. That makes left/right/top/bottom fail even though those branches only need
sourcePanelId, and it skips the split-then-surface fallback pattern this file already relies on for file-open flows.♻️ Suggested fallback pattern
private func openFocusedDirectoryInInlineVSCode(_ directoryURL: URL) -> Bool { guard let vscodeApplicationURL = TerminalDirectoryOpenTarget.vscodeInline.applicationURL(), let workspace = tabManager.selectedWorkspace, - let sourcePanelId = workspace.focusedPanelId, - let sourcePaneId = workspace.paneId(forPanelId: sourcePanelId) else { + let sourcePanelId = workspace.focusedPanelId else { return false } let sourceTabId = workspace.id let openDirection = vscodeInlineSplitDirection let tabManager = tabManager @@ let createdPanelId: UUID? = { - if let splitDirection = openDirection.splitDirection { - return tabManager.newBrowserSplit( + if let splitDirection = openDirection.splitDirection, + let panelId = tabManager.newBrowserSplit( tabId: sourceTabId, fromPanelId: sourcePanelId, orientation: splitDirection.orientation, insertFirst: splitDirection.insertFirst, url: openFolderURL, focus: true - ) + ) { + return panelId } + guard let focusedPaneId = workspace.bonsplitController.focusedPaneId else { return nil } return tabManager.newBrowserSurface( tabId: sourceTabId, - inPane: sourcePaneId, + inPane: focusedPaneId, url: openFolderURL, focus: true ) }()Based on learnings:
openFileInTextEditor(_:)must fall back toworkspace.newTextEditorSurface(inPane: bonsplitController.focusedPaneId)so file-open requests aren’t dropped when the focused panel is not pane-backed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 7363 - 7399, The guard currently requires paneId(forPanelId:) up front which makes the whole action fail when the focused panel isn't pane-backed; change openFileInTextEditor(_:) (and the VSCode open-folder flow) to only require workspace.focusedPanelId initially, defer resolving paneId by calling workspace.paneId(forPanelId: sourcePanelId) only where a pane-backed surface is actually needed (e.g., before calling newBrowserSurface or workspace.newTextEditorSurface), and implement the fallback path: if paneId is nil use bonsplitController.focusedPaneId or call workspace.newTextEditorSurface(inPane: bonsplitController.focusedPaneId) so file-open and split branches still work when the focused panel is not pane-backed; keep newBrowserSplit logic unchanged (it only needs sourcePanelId) and only branch to newBrowserSurface when you have a paneId or the fallback pane.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 7363-7399: The guard currently requires paneId(forPanelId:) up
front which makes the whole action fail when the focused panel isn't
pane-backed; change openFileInTextEditor(_:) (and the VSCode open-folder flow)
to only require workspace.focusedPanelId initially, defer resolving paneId by
calling workspace.paneId(forPanelId: sourcePanelId) only where a pane-backed
surface is actually needed (e.g., before calling newBrowserSurface or
workspace.newTextEditorSurface), and implement the fallback path: if paneId is
nil use bonsplitController.focusedPaneId or call
workspace.newTextEditorSurface(inPane: bonsplitController.focusedPaneId) so
file-open and split branches still work when the focused panel is not
pane-backed; keep newBrowserSplit logic unchanged (it only needs sourcePanelId)
and only branch to newBrowserSurface when you have a paneId or the fallback
pane.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47b380d2-fa29-474c-aff9-f53eb5b1f0c9
📒 Files selected for processing (8)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/GhosttyConfig.swiftSources/Panels/BrowserPanel.swiftSources/TabManager.swiftSources/cmuxApp.swiftcmuxTests/BrowserConfigTests.swiftcmuxTests/GhosttyConfigTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ContentView.swift`:
- Around line 7381-7403: When creating a new browser panel, if
tabManager.newBrowserSplit(...) returns nil (e.g., focused panel isn't
pane-backed) fall back to creating a surface in the focused pane instead of
bailing out; update the closure that computes createdPanelId so when
openDirection.splitDirection exists but newBrowserSplit returns nil you call
tabManager.newBrowserSurface(inPane: focusedPaneId, url: ..., focus: true) using
bonsplitController.focusedPaneId (or fallbackPaneId if needed), and similarly
when resolving sourcePaneId fallback to the focused pane before returning nil,
so newBrowserSurface is attempted whenever a split cannot be created.
Comment on lines
+7381
to
7403
| let createdPanelId: UUID? = { | ||
| if let splitDirection = openDirection.splitDirection { | ||
| return tabManager.newBrowserSplit( | ||
| tabId: sourceTabId, | ||
| fromPanelId: sourcePanelId, | ||
| orientation: splitDirection.orientation, | ||
| insertFirst: splitDirection.insertFirst, | ||
| url: openFolderURL, | ||
| focus: true | ||
| ) | ||
| } | ||
| guard let sourcePaneId = workspace.paneId(forPanelId: sourcePanelId) ?? fallbackPaneId else { | ||
| return nil | ||
| } | ||
| return tabManager.newBrowserSurface( | ||
| tabId: sourceTabId, | ||
| inPane: sourcePaneId, | ||
| url: openFolderURL, | ||
| focus: true | ||
| ) | ||
| }() | ||
| guard createdPanelId != nil else { | ||
| NSSound.beep() |
There was a problem hiding this comment.
Fall back to opening in the focused pane when split creation fails.
If newBrowserSplit(...) returns nil because the focused panel is not pane-backed, this still drops the inline VS Code open even though fallbackPaneId is already available. Falling back to newBrowserSurface(inPane:) here would keep left/right/top/bottom placements from failing unnecessarily.
Suggested fallback
let createdPanelId: UUID? = {
- if let splitDirection = openDirection.splitDirection {
- return tabManager.newBrowserSplit(
+ if let splitDirection = openDirection.splitDirection,
+ let splitPanelId = tabManager.newBrowserSplit(
tabId: sourceTabId,
fromPanelId: sourcePanelId,
orientation: splitDirection.orientation,
insertFirst: splitDirection.insertFirst,
url: openFolderURL,
focus: true
- )
+ ) {
+ return splitPanelId
}
guard let sourcePaneId = workspace.paneId(forPanelId: sourcePanelId) ?? fallbackPaneId else {
return nil
}
return tabManager.newBrowserSurface(Based on learnings, In Sources/ContentView.swift, openFileInTextEditor(_:) must attempt workspace.newTextEditorSplit(from:orientation:filePath:focus:) and, if that returns nil, fall back to workspace.newTextEditorSurface(inPane:filePath:focus:) using bonsplitController.focusedPaneId.
📝 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
| let createdPanelId: UUID? = { | |
| if let splitDirection = openDirection.splitDirection { | |
| return tabManager.newBrowserSplit( | |
| tabId: sourceTabId, | |
| fromPanelId: sourcePanelId, | |
| orientation: splitDirection.orientation, | |
| insertFirst: splitDirection.insertFirst, | |
| url: openFolderURL, | |
| focus: true | |
| ) | |
| } | |
| guard let sourcePaneId = workspace.paneId(forPanelId: sourcePanelId) ?? fallbackPaneId else { | |
| return nil | |
| } | |
| return tabManager.newBrowserSurface( | |
| tabId: sourceTabId, | |
| inPane: sourcePaneId, | |
| url: openFolderURL, | |
| focus: true | |
| ) | |
| }() | |
| guard createdPanelId != nil else { | |
| NSSound.beep() | |
| let createdPanelId: UUID? = { | |
| if let splitDirection = openDirection.splitDirection, | |
| let splitPanelId = tabManager.newBrowserSplit( | |
| tabId: sourceTabId, | |
| fromPanelId: sourcePanelId, | |
| orientation: splitDirection.orientation, | |
| insertFirst: splitDirection.insertFirst, | |
| url: openFolderURL, | |
| focus: true | |
| ) { | |
| return splitPanelId | |
| } | |
| guard let sourcePaneId = workspace.paneId(forPanelId: sourcePanelId) ?? fallbackPaneId else { | |
| return nil | |
| } | |
| return tabManager.newBrowserSurface( | |
| tabId: sourceTabId, | |
| inPane: sourcePaneId, | |
| url: openFolderURL, | |
| focus: true | |
| ) | |
| }() | |
| guard createdPanelId != nil else { | |
| NSSound.beep() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 7381 - 7403, When creating a new
browser panel, if tabManager.newBrowserSplit(...) returns nil (e.g., focused
panel isn't pane-backed) fall back to creating a surface in the focused pane
instead of bailing out; update the closure that computes createdPanelId so when
openDirection.splitDirection exists but newBrowserSplit returns nil you call
tabManager.newBrowserSurface(inPane: focusedPaneId, url: ..., focus: true) using
bonsplitController.focusedPaneId (or fallbackPaneId if needed), and similarly
when resolving sourcePaneId fallback to the focused pane before returning nil,
so newBrowserSurface is attempted whenever a split cannot be created.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
vscode-inline-split-directionsetting withright,left,bottom,top, andtabvaluesCloses #1977
Verification
./scripts/reload.sh --tag issue-1977-vscode-split-directionSummary by cubic
Make inline VS Code placement configurable: open as a split (right/left/top/bottom) or as a tab. Defaults to right; configurable in Browser Settings and via
vscode-inline-split-direction(closes #1977).New Features
vscode-inline-split-directionwithright,left,bottom(down),top(up),tab.tabopens in the current pane.UserDefaultson load; tests added for parsing, aliases, defaults, and canonical writes.Bug Fixes
tabplacement to reliably find the source pane.Written for commit 2919545. Summary will update on new commits.
Summary by CodeRabbit
New Features
Behavior
Tests